test: Use permissions from git in lint-files.py #25015

pull laanwj wants to merge 2 commits into bitcoin:master from laanwj:2022-04-lint-permissions-git changing 1 files +45 −32
  1. laanwj commented at 11:29 AM on April 28, 2022: member

    Improvements to the lint-files.py script:

    • Avoid use of shell=True.
    • Check the permissions in git's metadata instead of in the filesystem. This stops the umask or filesystem from interfering. It's also more efficient as it only needs a single call to git ls-files.

    (what triggered this change was File "..." contains a shebang line, but has the file permission 775 instead of the expected executable permission 755. errors running the script locally).

  2. test: Don't use shell=True in `lint-files.py`
    Avoid the use of shell=True.
    48d2e80a74
  3. laanwj added the label Tests on Apr 28, 2022
  4. laanwj force-pushed on Apr 28, 2022
  5. laanwj force-pushed on Apr 28, 2022
  6. in test/lint/lint-files.py:157 in 9706c69a9f outdated
     161 | @@ -145,26 +162,26 @@ def check_all_file_permissions() -> int:
     162 |              continue
     163 |          else:
     164 |              print(
    


    laanwj commented at 11:44 AM on April 28, 2022:

    Could put assert(0) here as well, as this is guaranteed to never happen. Git only stores an 'executable' bit, and its expanded permissions will always be 644 or 755. Then again, maybe future git might change this :scream:

  7. in test/lint/lint-files.py:76 in 9706c69a9f outdated
      69 | @@ -63,17 +70,29 @@ def full_extension(self) -> Optional[str]:
      70 |      @property
      71 |      def permissions(self) -> int:
      72 |          """
      73 | -        Returns the octal file permission of the file
      74 | +        Returns the octal file permission of the file. Internally, git only
      75 | +        keeps an 'executable' bit, so this will always return 0o644 or 0o755.
      76 |          """
      77 | -        return int(oct(os.stat(self.file_path).st_mode)[-3:])
      78 | +        return self._permissions
    


    laanwj commented at 11:55 AM on April 28, 2022:

    I changed this because I didn't like the octal-as-decimal representation.

  8. laanwj force-pushed on Apr 28, 2022
  9. vincenzopalazzo approved
  10. in test/lint/lint-files.py:192 in 2feb6b64a2 outdated
     188 | @@ -178,7 +189,7 @@ def check_shebang_file_permissions() -> int:
     189 |                      continue
     190 |  
     191 |              print(
     192 | -                f"""File "{filename}" contains a shebang line, but has the file permission {file_meta.permissions} instead of the expected executable permission {ALLOWED_PERMISSION_EXECUTABLES}. Do "chmod {ALLOWED_PERMISSION_EXECUTABLES} {filename}" (or remove the shebang line)."""
     193 | +                    f"""File "{filename}" contains a shebang line, but has the file permission {file_meta.permissions:03o} instead of the expected executable permission {ALLOWED_PERMISSION_EXECUTABLES}. Do "chmod {ALLOWED_PERMISSION_EXECUTABLES} {filename}" (or remove the shebang line)."""
    


    laanwj commented at 6:46 PM on April 28, 2022:

    I see I accidentally indented here and forgot to add {:03o} for the example permissions. Will fix this on next update.

  11. laanwj force-pushed on Apr 28, 2022
  12. fanquake commented at 8:35 AM on May 5, 2022: member

    @vincenzopalazzo want to take another look here?

  13. vincenzopalazzo approved
  14. laanwj commented at 9:12 AM on May 9, 2022: member

    ACK https://github.com/bitcoin/bitcoin/commit/48d2e80a7479a44b0ab09e87542c8cb7a8f72223

    Thanks! But that's the old commit :smile: Edit: wait, no, it's the first commit, 91c78b114ab2e0767e478caf33138147fa1dfff9 is the second and top one.

  15. willcl-ark commented at 11:18 AM on May 16, 2022: member

    Found this PR after I wasn't able to run test-files.py locally. This patch does indeed fix it.

    non-code review ACK.

    I'm curious, how exactly does changing to 0o755 fix this? it somehow causes it to respect the default umask as it's now an octal number?

  16. in test/lint/lint-files.py:21 in 91c78b114a outdated
      20 | -CMD_SHEBANG_FILES = "git grep --full-name --line-number -I '^#!'"
      21 | +CMD_ALL_FILES = ["git", "ls-files", "-z", "--full-name", "--stage"]
      22 | +CMD_SHEBANG_FILES = ["git", "grep", "--full-name", "--line-number", "-I", "^#!"]
      23 | +
      24 | +# this intentionally includes uppercase variants, as to be able to flag them
      25 | +ALL_SOURCE_FILENAMES_REGEXP = r"^.*\.([cC][pP][pP]|[hH]|[pP][yY]|[sS][hH])$"
    


    MarcoFalke commented at 11:39 AM on May 16, 2022:

    nit: Could this use re.IGNORECASE?


    laanwj commented at 8:59 AM on May 23, 2022:

    yes, good idea

  17. laanwj commented at 8:58 AM on May 23, 2022: member

    I'm curious, how exactly does changing to 0o755 fix this? it somehow causes it to respect the default umask as it's now an octal number?

    No, that change is an ancillary change (representing octal numbers as decimal is just weird). The main change here is, as noted in the OP, that permissions are queried from git instead of the local filesystem.

  18. test: Use permissions from git in `lint-files.py`
    Instead of using permissions from the local file system, which might
    depend on the umask, directly check the permissions from git's metadata.
    908fb7e2ec
  19. laanwj force-pushed on May 23, 2022
  20. laanwj commented at 9:12 AM on May 23, 2022: member

    Re-pushed 91c78b114ab2e0767e478caf33138147fa1dfff9 → 908fb7e2ec37fe68675d38dbfee4df9f861bb2b5

    • Use re.IGNORECASE for ALL_SOURCE_FILENAMES_REGEXP
  21. vincenzopalazzo approved
  22. vincenzopalazzo commented at 2:46 PM on May 23, 2022: none

    Ok LGTM, sorry for the delay and for the error in the commit link

    re-tACK https://github.com/bitcoin/bitcoin/commit/908fb7e2ec37fe68675d38dbfee4df9f861bb2b5

  23. MarcoFalke merged this on May 23, 2022
  24. MarcoFalke closed this on May 23, 2022

  25. DrahtBot locked this on May 23, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me