Jeff King [off-list ref] writes:
On Tue, Aug 19, 2014 at 04:05:21PM +1000, Babak M wrote:
quoted
I saw that if a hook file is present in .git/hooks and it does not
have execution permissions it is silently ignored.
I thought it might be worthwhile issuing a warning such as "Warning:
pre-commit hook exists but it cannot be executed due to insufficient
permissions".
Not sure if this has been discussed before. I searched the archive but
didn't see anything.
Thoughts, suggestions? Is there anything like that already?
Once upon a time we shipped sample hooks with their execute bits turned
off, and such a warning would have been very bad.
These days we give them a ".sample" extension (because Windows installs
had trouble with the execute bit :) ), so I think it should be OK in
theory. Installing a new version of git on top of an old one with "make
install" does not clean up old files. So somebody who has continuously
upgraded their git via "make install" to the same directory would have
the old-style sample files. Under your proposal, they would get a lot of
warnings.
However, that change came in v1.6.0, just over 6 years ago. We can
probably discount that (and if it does happen, maybe it is time for that
someone to clean up the other leftover cruft from past git installs).
The above all sounds very sensible.
We have another code path that looks for an executable, finds one
with no execute permission and decides not to execute it, and I
wonder if we should use the same criteria to decide to give (or not
give) a warning as the one used in the other code path (i.e. looking
up "git-foo" executable when the user runs "git foo").
On Wed, Aug 20, 2014 at 4:52 AM, Junio C Hamano [off-list ref] wrote:
Jeff King [off-list ref] writes:
quoted
On Tue, Aug 19, 2014 at 04:05:21PM +1000, Babak M wrote:
quoted
I saw that if a hook file is present in .git/hooks and it does not
have execution permissions it is silently ignored.
I thought it might be worthwhile issuing a warning such as "Warning:
pre-commit hook exists but it cannot be executed due to insufficient
permissions".
Not sure if this has been discussed before. I searched the archive but
didn't see anything.
Thoughts, suggestions? Is there anything like that already?
Once upon a time we shipped sample hooks with their execute bits turned
off, and such a warning would have been very bad.
These days we give them a ".sample" extension (because Windows installs
had trouble with the execute bit :) ), so I think it should be OK in
theory. Installing a new version of git on top of an old one with "make
install" does not clean up old files. So somebody who has continuously
upgraded their git via "make install" to the same directory would have
the old-style sample files. Under your proposal, they would get a lot of
warnings.
However, that change came in v1.6.0, just over 6 years ago. We can
probably discount that (and if it does happen, maybe it is time for that
someone to clean up the other leftover cruft from past git installs).
The above all sounds very sensible.
We have another code path that looks for an executable, finds one
with no execute permission and decides not to execute it, and I
wonder if we should use the same criteria to decide to give (or not
give) a warning as the one used in the other code path (i.e. looking
up "git-foo" executable when the user runs "git foo").
--
I actually find the existing behaviour useful. If I want to disable a
hook to I can just chmod -x .git/hook/... and I then chmod +x it when
I want to re-enable it. I guess I could live with an extra warning as
long as the command still succeeds.
On Wed, Aug 20, 2014 at 08:55:52PM +1200, Chris Packham wrote:
I actually find the existing behaviour useful. If I want to disable a
hook to I can just chmod -x .git/hook/... and I then chmod +x it when
I want to re-enable it. I guess I could live with an extra warning as
long as the command still succeeds.
You could do the same thing "mv $hook $hook.disabled" but it involves
retraining your fingers. I kind of agree that the existing system of
respecting the executable bit is nice, though: it does what you told it
to do, and a misconfiguration is your problem, not the system's. It's
perhaps worth changing if people frequently get the executable-bit thing
wrong, but I don't know whether they do or not.
I kind of feel like we had a similar discussion around items in PATH,
but I don't remember how it resolved.
-Peff