test: Use permissions from git in lint-files.py #25015
Conversation
Avoid the use of shell=True.
laanwj
force-pushed
the
2022-04-lint-permissions-git
branch
2 times, most recently
from
c2661b5 to
9706c69
Compare
test/lint/lint-files.py
Outdated
There was a problem hiding this comment.
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 😱
laanwj
force-pushed
the
2022-04-lint-permissions-git
branch
from
9706c69 to
2feb6b6
Compare
laanwj
force-pushed
the
2022-04-lint-permissions-git
branch
from
2feb6b6 to
91c78b1
Compare
|
@vincenzopalazzo want to take another look here? |
Thanks! But that's the old commit 😄 |
|
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 |
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. |
Instead of using permissions from the local file system, which might depend on the umask, directly check the permissions from git's metadata.
laanwj
force-pushed
the
2022-04-lint-permissions-git
branch
from
91c78b1 to
908fb7e
Compare
|
Re-pushed 91c78b114ab2e0767e478caf33138147fa1dfff9 → 908fb7e2ec37fe68675d38dbfee4df9f861bb2b5
|
vincenzopalazzo
left a comment
vincenzopalazzo
left a comment
There was a problem hiding this comment.
Ok LGTM, sorry for the delay and for the error in the commit link
re-tACK 908fb7e
5 participants
Improvements to the
lint-files.pyscript:shell=True.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).