Skip to content

Conversation

isle2983 and others added 17 commits November 9, 2020 22:59
Years are set according to 'git log' history

Zcash: Only the scripts we have that are missing the header.
These are text files but their encoding does not depend on the locale.
Not all of them require utf8 but it is better to fix it at something
to remove potential unpredictability.

This is necessary on FreeBSD where no locale is set by default,
and apparently Python defaults not only the terminal encoding to the locale
but that of every text file. So without LOCALE environment it defaults text
file encoding to ASCII. This causes problems with e.g. `bitcoin.conf`.

Luckily the locale doesn't affect the default encoding for str.encode() and
bytes.decode() on Python 3, so this is the only change necessary.
Instead of calling sprintf for every byte, format the hex bytes
ourselves by help of HexStr and a reverse_iterator.
Zcash: Only the script we have.
We have several pieces of information about subtrees:
1) What their current directory contents is
2) What their directory contents was at the time of the last subtree merge
3) What the directory contents of the upstream project is in the commit referred to by the subtree merge.

Normally, all 3 should be identical. git-subtree-check.sh so far only compared (1) with (3) however.

Fix this by comparing all three, and give some more useful diff output in the case of mismatch.

The added benefit is that (1) and (2) can be compared without needing to see the upstream repository.
git-subtree-check fails if the directory is given with a trailing slash,
eg:

```
> test/lint/git-subtree-check.sh src/univalue/
ERROR: src/univalue/ is not a subtree
```

Shell autocompletes will add the trailing slash when autofilling the
path name, which will therefore cause the script to fail.

Just ignore any trailing slash.
These are external libraries, and it does not make sense to maintain an
otherwise-meaningless diff from upstream.

This partially reverts commit 1e6d183.
Fixes a false positive match to the locale-dependent trim function.
@str4d str4d added C-upstream-port Category: Changes that are ported from the Bitcoin Core codebase. C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Nov 9, 2020
@str4d str4d added this to the Core Sprint 2020-45 milestone Nov 9, 2020
@str4d
Copy link
Contributor Author

str4d commented Nov 9, 2020

Of the lints that are not fixed in this PR, many are addressed upstream in the arg-parsing and Python 3 refactors, which I want to backport in separate PRs.

The only remaining lints in code we introduced are:

  • src/wallet/rpcwallet.cpp: o.pushKV("fee", std::stod(FormatMoney(nFee)));
    • This might be addressed in the sendmany refactor?
  • Various shellcheck lints in the fuzzer scripts, which need to be updated anyway for the Clang migration; I'm leaving this to @defuse for a future PR (opened Fix lint errors in fuzzer scripts #4850).

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK with comments.

#!/bin/bash
# Copyright (c) 2013 The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be an https URL, and similarly for other cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to address these in another PR. We're going to keep getting these from upstream PRs, so may as well change them periodically in bulk.

HEADER_ID_SUFFIX="_H"

REGEXP_EXCLUDE_FILES_WITH_PREFIX="src/(leveldb/|secp256k1/|univalue/)"
REGEXP_EXCLUDE_FILES_WITH_PREFIX="src/(crypto/ctaes/|leveldb/|secp256k1/|tinyformat.h|univalue/)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these excluded rather than just fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the commit message:

lint: Re-exclude subtrees from lint-include-guards.sh

These are external libraries, and it does not make sense to maintain an
otherwise-meaningless diff from upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For more meaningful lint changes (like locale, or the changes we've made to secp256k1), sure. The include guards have zero impact on our codebase (they've been working fine as-is), so it just adds noise.

We don't maintain any of the current subtrees, but we might in future.

Co-authored-by: Daira Hopwood <[email protected]>
@str4d
Copy link
Contributor Author

str4d commented Nov 10, 2020

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Nov 10, 2020

📌 Commit e531d72 has been approved by str4d

@zkbot
Copy link
Contributor

zkbot commented Nov 10, 2020

⌛ Testing commit e531d72 with merge caed4ad...

@zkbot
Copy link
Contributor

zkbot commented Nov 10, 2020

☀️ Test successful - pr-merge
Approved by: str4d
Pushing caed4ad to master...

@zkbot zkbot merged commit caed4ad into zcash:master Nov 10, 2020
@str4d str4d deleted the lint-fixes branch November 10, 2020 03:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-cleanup Category: PRs that clean code up or issues documenting cleanup. C-upstream-port Category: Changes that are ported from the Bitcoin Core codebase.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants