Re: obsolete index in wt_status_print after pre-commit hook runs
Am 04.08.2016 um 12:45 nachm. schrieb Junio C Hamano : > Andrew Keller writes: > >> In summary, I think I prefer #2 from a usability point of view, however I’m >> having >> trouble proving that #1 is actually *bad* and should be disallowed. > > Yeah, I agree with your argument from the usability and safety point > of view. > >> Any thoughts? Would it be better for the pre-commit hook to be >> officially allowed to edit the index [1], or would it be better >> for the pre-commit hook to explicitly *not* be allowed to edit the >> index [2], or would it be yet even better to simply leave it as it >> is? > > It is clear that our stance has been the third one so far. > > Another thing I did not see in your analysis is what happens if the > user is doing a partial commit, and how the changes made by > pre-commit hook is propagated back to the main index and the working > tree. > > The HEAD may have a file with contents in the "original" state, the > index may have the file with "update 1", and the working tree file > may have it with "update 2". After the commit is made, the user > will continue working from a state where the HEAD and the index have > "update 1", and the working tree has "update 2". "git diff file" > output before and after the commit will be identical (i.e. the > difference between "update 1" and "update 2") as expected. Excellent point — one I had discovered myself but neglected to include in my email. In my post-commit hook, I have logic in both versions of my experiment that disallows [1] fixing up diffs that are partially staged. Both scripts then update both the index and the working copy. (Sort of like how rebase works — clean working directory required, and then it updates the index and the work tree) [1] In version #1, if any files it wants to change are partially staged, it prints a detailed error message and aborts the commit outright. In version #2, the pre-commit hook sees the change it _wants_ to make, informs the user that he/she should run the fixup command, aborts the commit, and when the user runs the fixup command, the fixup command sees the partially staged file, prints the same detailed error message, and dies. Thanks for your help on this. it’s really been interesting. I’ll leave it as-is for now. Thanks, - Andrew Keller -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: obsolete index in wt_status_print after pre-commit hook runs
Andrew Keller writes: > In summary, I think I prefer #2 from a usability point of view, however I’m > having > trouble proving that #1 is actually *bad* and should be disallowed. Yeah, I agree with your argument from the usability and safety point of view. > Any thoughts? Would it be better for the pre-commit hook to be > officially allowed to edit the index [1], or would it be better > for the pre-commit hook to explicitly *not* be allowed to edit the > index [2], or would it be yet even better to simply leave it as it > is? It is clear that our stance has been the third one so far. Another thing I did not see in your analysis is what happens if the user is doing a partial commit, and how the changes made by pre-commit hook is propagated back to the main index and the working tree. The HEAD may have a file with contents in the "original" state, the index may have the file with "update 1", and the working tree file may have it with "update 2". After the commit is made, the user will continue working from a state where the HEAD and the index have "update 1", and the working tree has "update 2". "git diff file" output before and after the commit will be identical (i.e. the difference between "update 1" and "update 2") as expected. If pre-commit were allowed to munge the index to have the file in the "update 3" state, the resulting commit would have that version of the file in its tree. By definition, "update 1" and "update 3" are different (that is what it means to allow pre-commit to munge the index); where should the differences between "update 1" and "update 3" go? It is clear that pre-commit thought that the contents in the "update 1" state is bad and "update 3" state is better (that is why it made that fix), so after the commit is made, we would want to have "update 3" in the index. But what would you do to the working tree file, which is in "update 2" state? If you do not do anything, "git diff" would show the remaining edit the user had before starting the commit (i.e. difference between "update 1" and "update 2") plus a reversion of the edit pre-commit made because what the working tree has, "update 2", is based on "update 1" and has never heard of the change pre-commit did. But leaving the working tree file as-is is the only safe choice, as I do not think we want "git commit" to _create_ new conflict in the working tree by attempting to merge (we _could_, and implementing it would be a trivial thing to do by calling ll_merge() to three-way merge "update 2" and "update 3" that are both based on "update 1", but the result from the end-user's point of view is too _weird_). So, I tend to think we should not allow pre-commit to munge the index. We should be able to detect fairly cheaply if pre-commit munged the index by remembering the trailing SHA-1 of the index file given to the pre-commit hook before running it, and reading the trailing SHA-1 of the index file left after the pre-commit hook and comparing them. And we would yell at the user that his pre-commit munged the index and abort. Or something like that. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: obsolete index in wt_status_print after pre-commit hook runs
Am 15.07.2016 um 6:03 nachm. schrieb Junio C Hamano : > > Ahh, I misremembered. 2888605c (builtin-commit: fix partial-commit > support, 2007-11-18) does consider the possibility that pre-commit > may have modified the index contents after we take control back from > that hook, so that is probably a good place to enumerate what got > changed. Getting the list before running the hook can give an > out-of-date list, as you said. I’ve been experimenting with two different workflows recently: (1) Identify problem files during the pre-commit hook; when found, fix them automatically in the index and let the commit continue. (2) Identify problem files during the pre-commit hook; when found, provide instructions to fix the problem (and possibly set a helpful Git alias to do it in one command), and abort the commit. Require that the user fixup the index and try the commit again. And here are my thoughts: #1 seems to be quick and simple for the user, and it plays (mostly) nice with scripts and IDEs that do commits autonomously, but I’m having trouble trusting that my pre-commit hook made the *correct* changes (even though it’s worked nicely so far) (i.e., I keep looking at the new HEAD commit to make sure it looks right, where normally I just look at the index and make sure it looks right). #2 is slightly more difficult to implement just because it has more moving parts, however I’m finding that because I can interrogate the index after I manually run the command to make the required changes to the index, and *before* I commit again, I feel much more confident that I know what is going to be in my commit. However, this approach doesn’t play well with automated scripts that assume that a commit operation will always work. In summary, I think I prefer #2 from a usability point of view, however I’m having trouble proving that #1 is actually *bad* and should be disallowed. Any thoughts? Would it be better for the pre-commit hook to be officially allowed to edit the index [1], or would it be better for the pre-commit hook to explicitly *not* be allowed to edit the index [2], or would it be yet even better to simply leave it as it is? [1] and possibly create a patch that teaches builtin/commit.c to reread the index after the pre-commit hook runs and before rendering the commit message template [2] and possibly create a patch that teaches builtin/commit.c to detect changes to the index after the pre-commit hook runs Thanks, - Andrew Keller -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: obsolete index in wt_status_print after pre-commit hook runs
Am 15.07.2016 um 6:03 nachm. schrieb Junio C Hamano : > Junio C Hamano writes: >> On Fri, Jul 15, 2016 at 1:30 PM, Andrew Keller wrote: >>> Am 15.07.2016 um 12:34 nachm. schrieb Andrew Keller : >>> I pulled out the source for version 2.9.1 and briefly skimmed how run_commit and prepare_to_commit work. It seems that Git already understands that a pre-commit hook can change the index, and it rereads the index before running the prepare-commit-msg hook: https://github.com/git/git/blob/v2.9.1/builtin/commit.c#L941-L951 >>> >>> Quick question: Why does Git reread the index after the pre-commit hook >>> runs? >> >> Offhand I do not think of a good reason to do so; does something break >> if you took it out? > > Ahh, I misremembered. 2888605c (builtin-commit: fix partial-commit > support, 2007-11-18) does consider the possibility that pre-commit > may have modified the index contents after we take control back from > that hook, so that is probably a good place to enumerate what got > changed. Getting the list before running the hook can give an > out-of-date list, as you said. Interesting. So, the implication is that disallowing the pre-commit hook to change the index may cause some problems (491 problems, if my run of the tests was accurate). Does that mean it would be desirable to update the index before the commit message template is rendered? Thanks, - Andrew Keller -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: obsolete index in wt_status_print after pre-commit hook runs
Am 15.07.2016 um 5:19 nachm. schrieb Junio C Hamano : > > On Fri, Jul 15, 2016 at 1:30 PM, Andrew Keller wrote: >> Am 15.07.2016 um 12:34 nachm. schrieb Andrew Keller : >> >>> I pulled out the source for version 2.9.1 and briefly skimmed how >>> run_commit and >>> prepare_to_commit work. It seems that Git already understands that a >>> pre-commit >>> hook can change the index, and it rereads the index before running the >>> prepare-commit-msg hook: >>> https://github.com/git/git/blob/v2.9.1/builtin/commit.c#L941-L951 >> >> Quick question: Why does Git reread the index after the pre-commit hook runs? > > Offhand I do not think of a good reason to do so; does something break > if you took it out? According to only test failures, it seems that only the `update_main_cache_tree(0)` invocation is needed to avoid a torrent of test failures (490 failures across 102 tests). Removing lines 946, 947, 949, and 950 do not cause test breakages (although my computer is not set up to run all of the tests). However, there seems to be an interaction between lines 946-947 and `update_main_cache_tree(0)` on line 948: although lines 946-947 can be removed by themselves without test breakages, when 946-948 are all disabled together (and, in turn, lines 949-950 never run), one additional test failure is registered (t2203.5). Thanks, - Andrew Keller -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: obsolete index in wt_status_print after pre-commit hook runs
Junio C Hamano writes: > On Fri, Jul 15, 2016 at 1:30 PM, Andrew Keller wrote: >> Am 15.07.2016 um 12:34 nachm. schrieb Andrew Keller : >> >>> I pulled out the source for version 2.9.1 and briefly skimmed how >>> run_commit and >>> prepare_to_commit work. It seems that Git already understands that a >>> pre-commit >>> hook can change the index, and it rereads the index before running the >>> prepare-commit-msg hook: >>> https://github.com/git/git/blob/v2.9.1/builtin/commit.c#L941-L951 >> >> Quick question: Why does Git reread the index after the pre-commit hook runs? > > Offhand I do not think of a good reason to do so; does something break > if you took it out? Ahh, I misremembered. 2888605c (builtin-commit: fix partial-commit support, 2007-11-18) does consider the possibility that pre-commit may have modified the index contents after we take control back from that hook, so that is probably a good place to enumerate what got changed. Getting the list before running the hook can give an out-of-date list, as you said. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: obsolete index in wt_status_print after pre-commit hook runs
On Fri, Jul 15, 2016 at 1:30 PM, Andrew Keller wrote: > Am 15.07.2016 um 12:34 nachm. schrieb Andrew Keller : > >> I pulled out the source for version 2.9.1 and briefly skimmed how run_commit >> and >> prepare_to_commit work. It seems that Git already understands that a >> pre-commit >> hook can change the index, and it rereads the index before running the >> prepare-commit-msg hook: >> https://github.com/git/git/blob/v2.9.1/builtin/commit.c#L941-L951 > > Quick question: Why does Git reread the index after the pre-commit hook runs? Offhand I do not think of a good reason to do so; does something break if you took it out? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: obsolete index in wt_status_print after pre-commit hook runs
Am 15.07.2016 um 12:34 nachm. schrieb Andrew Keller : > I pulled out the source for version 2.9.1 and briefly skimmed how run_commit > and > prepare_to_commit work. It seems that Git already understands that a > pre-commit > hook can change the index, and it rereads the index before running the > prepare-commit-msg hook: > https://github.com/git/git/blob/v2.9.1/builtin/commit.c#L941-L951 Quick question: Why does Git reread the index after the pre-commit hook runs? Thanks, - Andrew Keller -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: obsolete index in wt_status_print after pre-commit hook runs
On 15.07.2016, at 1:28 nachm., Junio C Hamano wrote: > Earlier you said you are working on a patch series. Since you have > already looked at the codepath already, perhaps you may want to try > a patch series to add the missing error-return instead, if you are > interested? Definitely interested — Sounds like a great learning experience. Thanks, - Andrew Keller -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: obsolete index in wt_status_print after pre-commit hook runs
Andrew Keller writes: > On 15.07.2016, at 1:02 nachm., Junio C Hamano wrote: > >> Expected outcome is an error saying "do not modify the index inside >> pre-commit hook", and a rejection. It was meant as a verification >> mechansim (hence it can be bypassed with --no-verify), not as a way >> to make changes that the user didn't tell "git commit" to make. > > Ah! Good to know, then. I’ll rewrite my hook to behave more correctly. No problem. >> It is just the implementation that dates back to the old days were >> too trusting that all users would behave (with its own definition of >> "behaving well", which may or may not match your expectation), did >> not anticipate that people would try to muck with the contents being >> commited in the hook, and did not implement such verification. Earlier you said you are working on a patch series. Since you have already looked at the codepath already, perhaps you may want to try a patch series to add the missing error-return instead, if you are interested? Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: obsolete index in wt_status_print after pre-commit hook runs
On 15.07.2016, at 1:02 nachm., Junio C Hamano wrote: > Expected outcome is an error saying "do not modify the index inside > pre-commit hook", and a rejection. It was meant as a verification > mechansim (hence it can be bypassed with --no-verify), not as a way > to make changes that the user didn't tell "git commit" to make. Ah! Good to know, then. I’ll rewrite my hook to behave more correctly. Thanks, - Andrew Keller -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: obsolete index in wt_status_print after pre-commit hook runs
Andrew Keller writes: > I have observed an interesting scenario. Here are example reproduction steps: > > 1. new repository > 2. create new pre-commit hook that invokes `git mv one two` > 3. touch one > 4. git add one > 5. git commit > > Expected outcome: In the commit message template, I expect to see > “Changes to be committed: new file: two" Expected outcome is an error saying "do not modify the index inside pre-commit hook", and a rejection. It was meant as a verification mechansim (hence it can be bypassed with --no-verify), not as a way to make changes that the user didn't tell "git commit" to make. It is just the implementation that dates back to the old days were too trusting that all users would behave (with its own definition of "behaving well", which may or may not match your expectation), did not anticipate that people would try to muck with the contents being commited in the hook, and did not implement such verification. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html