Feature request: Please consider a change similar to #189 for allowing executable checking to work in more diverse contexts

[posita]

I understand the sentiment not to want to build in bug fixes for other entities' software, but I would like to make an appeal to accommodate a change similar to #189, originally authored by @geeky-smurf, albeit a slightly different approach.

Two different, but overlapping permission checks is not inconsistent with pre-commit.

@geeky-smurf's approach was quite elegant in that it does very nearly what the existing code does in precisely the same place. It's almost just as readable. There's no "if running in Docker" or "if running on MacOS" nonsense. It's using proximal primitives in an intended way.

It's understandable you wouldn't want to lose or replace the os.access(path, os.X_OK) check. In that case, there's a tweak worth considering:

executable = os.access(path, os.X_OK) or \
    os.stat(path).st_mode & stat.S_IXUSR  # no need to check group or other bits

That would allow the nuances of os.access to persist, but fall back to looking for executable permissions in @geeky-smurf's alternate way. Again, it's almost just as readable. Any impact to maintainability should be slight, if not negligible.

pre-commit is already using a proxy for the intended source of truth: the Git index.

pre-commit is looking to the filesystem as a proxy to checking whether Git thinks the files are executable. Strictly speaking, it's already compromising on that front. What it's really after is something like git ls-files --stage <path> …. Most of the time, the filesystem permissions are in concert with this value, but not always. Git is the source of truth. Expanding os.access(…) to inclusively check os.stat(…).st_mode gets closer to that on more systems than either standing alone.

The corresponding Docker issue is not easily dismissible as a bug. It's a deliberate tradeoff, and accommodating it may enhance pre-commit's utility in architectures beyond Docker on OS X.

@djs55 has a decent write up here. The short version is that, when running on OS X, Docker affords access to the host filesystem using FUSE as an intermediary, which also makes tradeoffs of complete executable permission detection and performance.

Docker is probably the most popular place this comes up, but it could come up elsewhere. The likelihood of encountering another case only increases as pre-commit gains popularity.

Being flexible on ferreting out the executable status improves usability without compromising correctness, which only serves pre-commit and its users.

To my knowledge, doing a os.access(…) or os.stat(…).st_mode & … is not over broad. In other words, I don't think the change will all of a sudden start identifying non-executable files as executable. Assuming that's correct, then such a change doesn't lead to correctness issues that take away from pre-commit's value. However, by adding the change, pre-commit becomes a smidge more flexible on systems where this problem might occur, which is of value to a greater audience of users.

If #189 does not appeal to you, please consider my proposed alternative.

If you are amenable, #290 is now up for your review. Thank you very much for your time and consideration.

[posita]

@asottile, I understand you don't want to keep this issue open, but can you offer some insight as to why?

[asottile]

there's several duplicates which answer that -- please search the issue tracker next time