{"thread":{"id":"37374","subject":"Issuing warning when hook does not have execution permission","startedAt":"2014-08-19T06:05:21Z","lastAt":"2014-08-20T17:51:53Z","messageCount":6,"participants":["Babak M","Jeff King","Junio C Hamano","Chris Packham"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"247892","messageId":"CAPyMVO+a+R-+UDVuNSfhyB=PX-NUZbt6VGXfOv19-mDKTqfj6g@mail.gmail.com","threadId":"37374","inReplyTo":null,"subject":"Issuing warning when hook does not have execution permission","fromName":"Babak M","fromEmail":"babak@melon.com.au","sentAt":"2014-08-19T06:05:21Z","receivedAt":"2014-08-19T06:05:21Z","isPatch":false,"sender":{"key":"babak@melon.com.au","avatar":null},"body":"Hi,\n\nI saw that if a hook file is present in .git/hooks and it does not\nhave execution permissions it is silently ignored.\n\nI thought it might be worthwhile issuing a warning such as \"Warning:\npre-commit hook exists but it cannot be executed due to insufficient\npermissions\".\n\nNot sure if this has been discussed before. I searched the archive but\ndidn't see anything.\n\nThoughts, suggestions? Is there anything like that already?\n\nCheers,\nBabak\n"},{"id":"247896","messageId":"20140819080002.GB7712@peff.net","threadId":"37374","inReplyTo":"CAPyMVO+a+R-+UDVuNSfhyB=PX-NUZbt6VGXfOv19-mDKTqfj6g@mail.gmail.com","subject":"Re: Issuing warning when hook does not have execution permission","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-19T08:00:02Z","receivedAt":"2014-08-19T08:00:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 19, 2014 at 04:05:21PM +1000, Babak M wrote:\n\n> I saw that if a hook file is present in .git/hooks and it does not\n> have execution permissions it is silently ignored.\n> \n> I thought it might be worthwhile issuing a warning such as \"Warning:\n> pre-commit hook exists but it cannot be executed due to insufficient\n> permissions\".\n> \n> Not sure if this has been discussed before. I searched the archive but\n> didn't see anything.\n> \n> Thoughts, suggestions? Is there anything like that already?\n\nOnce upon a time we shipped sample hooks with their execute bits turned\noff, and such a warning would have been very bad.\n\nThese days we give them a \".sample\" extension (because Windows installs\nhad trouble with the execute bit :) ), so I think it should be OK in\ntheory. Installing a new version of git on top of an old one with \"make\ninstall\" does not clean up old files. So somebody who has continuously\nupgraded their git via \"make install\" to the same directory would have\nthe old-style sample files. Under your proposal, they would get a lot of\nwarnings.\n\nHowever, that change came in v1.6.0, just over 6 years ago. We can\nprobably discount that (and if it does happen, maybe it is time for that\nsomeone to clean up the other leftover cruft from past git installs).\n\n-Peff\n"},{"id":"247950","messageId":"xmqqegwcwhfc.fsf@gitster.dls.corp.google.com","threadId":"37374","inReplyTo":"20140819080002.GB7712@peff.net","subject":"Re: Issuing warning when hook does not have execution permission","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-19T16:52:23Z","receivedAt":"2014-08-19T16:52:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Aug 19, 2014 at 04:05:21PM +1000, Babak M wrote:\n>\n>> I saw that if a hook file is present in .git/hooks and it does not\n>> have execution permissions it is silently ignored.\n>> \n>> I thought it might be worthwhile issuing a warning such as \"Warning:\n>> pre-commit hook exists but it cannot be executed due to insufficient\n>> permissions\".\n>> \n>> Not sure if this has been discussed before. I searched the archive but\n>> didn't see anything.\n>> \n>> Thoughts, suggestions? Is there anything like that already?\n>\n> Once upon a time we shipped sample hooks with their execute bits turned\n> off, and such a warning would have been very bad.\n>\n> These days we give them a \".sample\" extension (because Windows installs\n> had trouble with the execute bit :) ), so I think it should be OK in\n> theory. Installing a new version of git on top of an old one with \"make\n> install\" does not clean up old files. So somebody who has continuously\n> upgraded their git via \"make install\" to the same directory would have\n> the old-style sample files. Under your proposal, they would get a lot of\n> warnings.\n>\n> However, that change came in v1.6.0, just over 6 years ago. We can\n> probably discount that (and if it does happen, maybe it is time for that\n> someone to clean up the other leftover cruft from past git installs).\n\nThe above all sounds very sensible.\n\nWe have another code path that looks for an executable, finds one\nwith no execute permission and decides not to execute it, and I\nwonder if we should use the same criteria to decide to give (or not\ngive) a warning as the one used in the other code path (i.e. looking\nup \"git-foo\" executable when the user runs \"git foo\").\n"},{"id":"248007","messageId":"CAFOYHZCGQj1vaZfOpFXHpuf-OjTB5RXRN_37O2EkQFRvow-RfA@mail.gmail.com","threadId":"37374","inReplyTo":"xmqqegwcwhfc.fsf@gitster.dls.corp.google.com","subject":"Re: Issuing warning when hook does not have execution permission","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2014-08-20T08:55:52Z","receivedAt":"2014-08-20T08:55:52Z","isPatch":false,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"On Wed, Aug 20, 2014 at 4:52 AM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> On Tue, Aug 19, 2014 at 04:05:21PM +1000, Babak M wrote:\n>>\n>>> I saw that if a hook file is present in .git/hooks and it does not\n>>> have execution permissions it is silently ignored.\n>>>\n>>> I thought it might be worthwhile issuing a warning such as \"Warning:\n>>> pre-commit hook exists but it cannot be executed due to insufficient\n>>> permissions\".\n>>>\n>>> Not sure if this has been discussed before. I searched the archive but\n>>> didn't see anything.\n>>>\n>>> Thoughts, suggestions? Is there anything like that already?\n>>\n>> Once upon a time we shipped sample hooks with their execute bits turned\n>> off, and such a warning would have been very bad.\n>>\n>> These days we give them a \".sample\" extension (because Windows installs\n>> had trouble with the execute bit :) ), so I think it should be OK in\n>> theory. Installing a new version of git on top of an old one with \"make\n>> install\" does not clean up old files. So somebody who has continuously\n>> upgraded their git via \"make install\" to the same directory would have\n>> the old-style sample files. Under your proposal, they would get a lot of\n>> warnings.\n>>\n>> However, that change came in v1.6.0, just over 6 years ago. We can\n>> probably discount that (and if it does happen, maybe it is time for that\n>> someone to clean up the other leftover cruft from past git installs).\n>\n> The above all sounds very sensible.\n>\n> We have another code path that looks for an executable, finds one\n> with no execute permission and decides not to execute it, and I\n> wonder if we should use the same criteria to decide to give (or not\n> give) a warning as the one used in the other code path (i.e. looking\n> up \"git-foo\" executable when the user runs \"git foo\").\n> --\n\nI actually find the existing behaviour useful. If I want to disable a\nhook to I can just chmod -x .git/hook/... and I then chmod +x it when\nI want to re-enable it. I guess I could live with an extra warning as\nlong as the command still succeeds.\n"},{"id":"248010","messageId":"20140820101921.GA21527@peff.net","threadId":"37374","inReplyTo":"CAFOYHZCGQj1vaZfOpFXHpuf-OjTB5RXRN_37O2EkQFRvow-RfA@mail.gmail.com","subject":"Re: Issuing warning when hook does not have execution permission","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-20T10:19:22Z","receivedAt":"2014-08-20T10:19:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 20, 2014 at 08:55:52PM +1200, Chris Packham wrote:\n\n> I actually find the existing behaviour useful. If I want to disable a\n> hook to I can just chmod -x .git/hook/... and I then chmod +x it when\n> I want to re-enable it. I guess I could live with an extra warning as\n> long as the command still succeeds.\n\nYou could do the same thing \"mv $hook $hook.disabled\" but it involves\nretraining your fingers. I kind of agree that the existing system of\nrespecting the executable bit is nice, though: it does what you told it\nto do, and a misconfiguration is your problem, not the system's. It's\nperhaps worth changing if people frequently get the executable-bit thing\nwrong, but I don't know whether they do or not.\n\nI kind of feel like we had a similar discussion around items in PATH,\nbut I don't remember how it resolved.\n\n-Peff\n"},{"id":"248023","messageId":"xmqqzjezt5fq.fsf@gitster.dls.corp.google.com","threadId":"37374","inReplyTo":"20140820101921.GA21527@peff.net","subject":"Re: Issuing warning when hook does not have execution permission","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-20T17:51:53Z","receivedAt":"2014-08-20T17:51:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Aug 20, 2014 at 08:55:52PM +1200, Chris Packham wrote:\n>\n>> I actually find the existing behaviour useful. If I want to disable a\n>> hook to I can just chmod -x .git/hook/... and I then chmod +x it when\n>> I want to re-enable it. I guess I could live with an extra warning as\n>> long as the command still succeeds.\n>\n> You could do the same thing \"mv $hook $hook.disabled\" but it involves\n> retraining your fingers. I kind of agree that the existing system of\n> respecting the executable bit is nice, though: it does what you told it\n> to do, and a misconfiguration is your problem, not the system's. It's\n> perhaps worth changing if people frequently get the executable-bit thing\n> wrong, but I don't know whether they do or not.\n>\n> I kind of feel like we had a similar discussion around items in PATH,\n> but I don't remember how it resolved.\n\nI don't either, but IIRC the primary tricky point was what happens\nwhen a component of $PATH list is inaccessible, making us unable to\neven know if an executable we are looking for exists there or not,\nwhich is slightly a different issue.\n\nAnd I also kind of agree that the existing system is nice. It may\nsound like a good idea to warn when there is even a slight chance of\nmisconfiguration on the user's side, but for this particular one, it\nhas been a designed-in behaviour for a long time, and it may be\nunwise to change it.\n"}]}