Nit: Given that failed_tests number is computed, one can probably print the number of failed tests too.

Great idea, I've updated to keep track of the number of failures, and print a message with the total number of failures, if any :)

I think people mostly do something like this https://source.dot.net/#System.Private.CoreLib/Path.cs,467ebc0e33e0820c to find a file extension. That is to look for the first os.extsep but not from the beginning of the string (filename) but from the end of the filename.

A counter-example can be 'contrib/devtools/split-debug.my.nice.helper.sh.in' where you would return my.nice.helper.sh.in.

Your code may be perfectly fine for what you want to achieve in this PR, yet I would be hesitant to use get_filename_extension name.

I have no clear suggestion here, letting it up to you to decide. :)

Ah yes I originally was using os.path.splitext(self.file_path)[1].strip(".") which works similar to the linked implementation (only return the final extension, not everything from the first .)

I had changed this as I needed a way to detect .sh.in files to exclude them from one of the tests, but you bring up a very good point - with the change I had made it's possible certain files with weird extensions like split-debug.my.nice.helper.sh would not actually be detected as a sh file, and would not have certain tests enforced.

Ive updated the code to add a new FileMeta class, and have changed the logic to use the appropriate version of the extension in each test. Let me know if this makes sense or not :)

It looks fine to me. Thanks for updating it. :)

nit: The comment does not correspond with the code actually.

One could probably do: filenames = filenames[:-1]

right, I chose to remove the trailing element by checking for an empty string as I was worried about some case in the future where the output of the git ls-files command changes somehow and the last element is not actually empty. I'm probably overthinking it though.

On a review of this, I'm fairly certain I can just safely rewrite this code as this one liner, which still catches the original edge case of a file with whitespace at the end of the files list.

filenames = check_output(CMD_ALL_FILES, shell=True).decode("utf8").rstrip('\n').split("\n")

Happy to update this if people don't mind re-reviewing. Let me know.

Edit: though that said, apparently it is actually possible for a filename to have a newline in it so I guess that one-liner would not catch this case, though it does seem fairly unlikely to happen.

right, I chose to remove the trailing element by checking for an empty string as I was worried about some case in the future where the output of the git ls-files command changes somehow and the last element is not actually empty. I'm probably overthinking it though.

One would need to ask on a Git mailing list whether the text output of the command is stable or not. But your solution works well, so it's good as it is I think. 👍

If a filename had a newline in its name it would also be split by the split("\n")

I'll think about a way to handle this case. I think currently if this happens the test overall will still fail, as the permissions test would not be able to locate the file and should error out - we can definitely handle this better, though.

I think .rstrip('\n').split("\n") as you suggested is fine

Addressed in #21873

super nit: I'm not sure whether os.stat should behave have follow_symlinks equal to True (default) or False. Possibly relevant is https://stackoverflow.com/questions/954560/how-does-git-handle-symbolic-links/18791647#18791647.

I don't want to hold up this PR. Leaving this for comment here for the sake of completeness. Maybe it's stupid. IDK.

Hmm interesting. It seems that there are currently no symbolic links in the repository itself, at least none that I could find with:

ls -lR ~/code/bitcoin | grep ^l

I wonder if that is intentional, or it's just never come up before?

In any case, if there were symbolic links my thinking is that we probably would want to follow them, as it seems that symlinks themselves can't actually have their permissions changed (At least on linux), so either we would want to exclude symlinks entirely from the permissions test, or just do the default here and follow the link to check the linked file.

My take on this is: I think that adding a symbolic link should produce a warning so that others can notice the fact. The fact that there are no symbolic links at the moment hints that they are probably not necessary.

However, this is really out of scope of this PR, I guess. If somebody finds it important it's always possible to address this in a follow up PR. :)

Just a question (not a blocker): why does this need a bash wrapper?

I believe the main reason is the the lint-all.sh script that CI runs is currently setup to automatically run all .sh tests in the test/lint directory, but won't automatically run .py files.

In addition it allows lint tests to opt in or out of locale dependence

If we start to move more towards using python based linters, it probably makes sense to make some updates here, so we can get rid of the requirement for the .sh wrapper.

@practicalswift may have more context here.

If a filename had a newline in its name it would also be split by the split("\n")

nit: shouldn't open be used in a context manager, so that it is closed again?

Good call, I will address in a follow-up

nit: Doesn't matter here, but it could use startswith for type safety.

Since this PR, when I run the linter I see a flood of errors and have to disable it. It seems inappropriate to check user, group, read, and write permissions when the only permission git tracks is whether the file is marked executable. Other permissions are local to the checkout and don't affect the project or other developers. Errors I see are:

[...]
File "test/lint/lint-whitespace.sh" contains a shebang line, but has the file permission 700 instead of the expected executable permission 755. Do "chmod 755 test/lint/lint-whitespace.sh" (or remove the shebang line).
File "test/util/bitcoin-util-test.py" contains a shebang line, but has the file permission 700 instead of the expected executable permission 755. Do "chmod 755 test/util/bitcoin-util-test.py" (or remove the shebang line).
File "test/util/rpcauth-test.py" contains a shebang line, but has the file permission 700 instead of the expected executable permission 755. Do "chmod 755 test/util/rpcauth-test.py" (or remove the shebang line).
ERROR: There were 2464 failed tests in the lint-files.py lint test. Please resolve the above errors.
^---- failure generated from test/lint/lint-files.sh