{"thread":{"id":"42858","subject":"obsolete index in wt_status_print after pre-commit hook runs","startedAt":"2016-07-15T16:49:38Z","lastAt":"2016-08-05T13:22:26Z","messageCount":13,"participants":["Andrew Keller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"291547","messageId":"5988D847-25A2-4997-9601-083772689879@covenanteyes.com","threadId":"42858","inReplyTo":null,"subject":"obsolete index in wt_status_print after pre-commit hook runs","fromName":"Andrew Keller","fromEmail":"andrew.keller@covenanteyes.com","sentAt":"2016-07-15T16:34:19Z","receivedAt":"2016-07-15T16:49:38Z","isPatch":false,"sender":{"key":"andrew.keller@covenanteyes.com","avatar":null},"body":"Hi everyone,\n\nI have observed an interesting scenario.  Here are example reproduction steps:\n\n1. new repository\n2. create new pre-commit hook that invokes `git mv one two`\n3. touch one\n4. git add one\n5. git commit\n\nExpected outcome: In the commit message template, I expect to see “Changes to be committed: new file: two\"\n\nFound outcome: In the commit message template, I see “Changes to be committed: new file: one\"\n\nThis behavior seems to be reproducible in versions 2.9.1, 2.8.1, 2.0.0, and 1.6.0.\n\nSkip the next 3 paragraphs if you are in a hurry.\n\nI pulled out the source for version 2.9.1 and briefly skimmed how run_commit and\nprepare_to_commit work.  It seems that Git already understands that a pre-commit\nhook can change the index, and it rereads the index before running the\nprepare-commit-msg hook: https://github.com/git/git/blob/v2.9.1/builtin/commit.c#L941-L951\n\nDuring the prepare-commit-msg hook, it seems that the index (according to Git\ncommands) is correct and up-to-date, but the textual message inside the commit\nmessage template is out-of-date (it references the file `one` as a change to be\ncommitted).\n\nIn builtin/commit.c, it seems that the commit message template is rendered\nimmediately after the pre-commit hook is ran, and immediately before the index is\nreread.  If I move the small block of code that rereads the index up, to just after\nthe pre-commit hook is ran, the commit message template seems to be as I would\nexpect, both in .git/COMMIT_EDITMSG during the prepare-commit-msg hook and\nin the editor for the commit message itself.\n\nI am putting together a 2-patch series that includes a failing test, and then this\nchange (which fixes the test), but while I do that, I figure I may as well ping the\ncommunity to make sure that this behavior is not intentional.  I’d wager that this\nchange is for the better, but since this behavior has been around so long (I stopped\nchecking at 1.6.0), it doesn’t hurt to make sure.\n\nAny comments, concerns, or advice?\n\nThanks,\n - Andrew Keller\n\n"},{"id":"291550","messageId":"xmqq1t2uomw3.fsf@gitster.mtv.corp.google.com","threadId":"42858","inReplyTo":"5988D847-25A2-4997-9601-083772689879@covenanteyes.com","subject":"Re: obsolete index in wt_status_print after pre-commit hook runs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-15T17:02:20Z","receivedAt":"2016-07-15T17:02:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrew Keller <andrew.keller@covenanteyes.com> writes:\n\n> I have observed an interesting scenario.  Here are example reproduction steps:\n>\n> 1. new repository\n> 2. create new pre-commit hook that invokes `git mv one two`\n> 3. touch one\n> 4. git add one\n> 5. git commit\n>\n> Expected outcome: In the commit message template, I expect to see\n> “Changes to be committed: new file: two\"\n\nExpected outcome is an error saying \"do not modify the index inside\npre-commit hook\", and a rejection.  It was meant as a verification\nmechansim (hence it can be bypassed with --no-verify), not as a way\nto make changes that the user didn't tell \"git commit\" to make.\n\nIt is just the implementation that dates back to the old days were\ntoo trusting that all users would behave (with its own definition of\n\"behaving well\", which may or may not match your expectation), did\nnot anticipate that people would try to muck with the contents being\ncommited in the hook, and did not implement such verification.\n\n\n"},{"id":"291552","messageId":"B3E20AFF-7661-43A0-A715-F0B9F3CD58DC@kellerfarm.com","threadId":"42858","inReplyTo":"xmqq1t2uomw3.fsf@gitster.mtv.corp.google.com","subject":"Re: obsolete index in wt_status_print after pre-commit hook runs","fromName":"Andrew Keller","fromEmail":"andrew@kellerfarm.com","sentAt":"2016-07-15T17:20:28Z","receivedAt":"2016-07-15T17:20:35Z","isPatch":false,"sender":{"key":"andrew@kellerfarm.com","avatar":"https://avatars.githubusercontent.com/u/304204?v=4"},"body":"On 15.07.2016, at 1:02 nachm., Junio C Hamano <gitster@pobox.com> wrote:\n\n> Expected outcome is an error saying \"do not modify the index inside\n> pre-commit hook\", and a rejection.  It was meant as a verification\n> mechansim (hence it can be bypassed with --no-verify), not as a way\n> to make changes that the user didn't tell \"git commit\" to make.\n\nAh!  Good to know, then.  I’ll rewrite my hook to behave more correctly.\n\nThanks,\n - Andrew Keller\n\n"},{"id":"291554","messageId":"xmqqshvan73f.fsf@gitster.mtv.corp.google.com","threadId":"42858","inReplyTo":"B3E20AFF-7661-43A0-A715-F0B9F3CD58DC@kellerfarm.com","subject":"Re: obsolete index in wt_status_print after pre-commit hook runs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-15T17:28:52Z","receivedAt":"2016-07-15T17:29:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrew Keller <andrew@kellerfarm.com> writes:\n\n> On 15.07.2016, at 1:02 nachm., Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> Expected outcome is an error saying \"do not modify the index inside\n>> pre-commit hook\", and a rejection.  It was meant as a verification\n>> mechansim (hence it can be bypassed with --no-verify), not as a way\n>> to make changes that the user didn't tell \"git commit\" to make.\n>\n> Ah!  Good to know, then.  I’ll rewrite my hook to behave more correctly.\n\nNo problem.\n\n>> It is just the implementation that dates back to the old days were\n>> too trusting that all users would behave (with its own definition of\n>> \"behaving well\", which may or may not match your expectation), did\n>> not anticipate that people would try to muck with the contents being\n>> commited in the hook, and did not implement such verification.\n\nEarlier you said you are working on a patch series.  Since you have\nalready looked at the codepath already, perhaps you may want to try\na patch series to add the missing error-return instead, if you are\ninterested?\n\nThanks.\n"},{"id":"291555","messageId":"96E73F11-B24B-4305-8297-3308986E542F@kellerfarm.com","threadId":"42858","inReplyTo":"xmqqshvan73f.fsf@gitster.mtv.corp.google.com","subject":"Re: obsolete index in wt_status_print after pre-commit hook runs","fromName":"Andrew Keller","fromEmail":"andrew@kellerfarm.com","sentAt":"2016-07-15T17:42:08Z","receivedAt":"2016-07-15T17:42:35Z","isPatch":false,"sender":{"key":"andrew@kellerfarm.com","avatar":"https://avatars.githubusercontent.com/u/304204?v=4"},"body":"On 15.07.2016, at 1:28 nachm., Junio C Hamano <gitster@pobox.com> wrote:\n\n> Earlier you said you are working on a patch series.  Since you have\n> already looked at the codepath already, perhaps you may want to try\n> a patch series to add the missing error-return instead, if you are\n> interested?\n\nDefinitely interested — Sounds like a great learning experience.\n\nThanks,\n - Andrew Keller\n\n"},{"id":"291562","messageId":"2ED67396-2530-4D1C-8F21-1C30983DB9DC@kellerfarm.com","threadId":"42858","inReplyTo":"5988D847-25A2-4997-9601-083772689879@covenanteyes.com","subject":"Re: obsolete index in wt_status_print after pre-commit hook runs","fromName":"Andrew Keller","fromEmail":"andrew@kellerfarm.com","sentAt":"2016-07-15T20:30:28Z","receivedAt":"2016-07-15T20:30:41Z","isPatch":false,"sender":{"key":"andrew@kellerfarm.com","avatar":"https://avatars.githubusercontent.com/u/304204?v=4"},"body":"Am 15.07.2016 um 12:34 nachm. schrieb Andrew Keller <andrew@kellerfarm.com>:\n\n> I pulled out the source for version 2.9.1 and briefly skimmed how run_commit and\n> prepare_to_commit work.  It seems that Git already understands that a pre-commit\n> hook can change the index, and it rereads the index before running the\n> prepare-commit-msg hook: https://github.com/git/git/blob/v2.9.1/builtin/commit.c#L941-L951\n\nQuick question: Why does Git reread the index after the pre-commit hook runs?\n\nThanks,\n - Andrew Keller\n\n"},{"id":"291567","messageId":"CAPc5daWZofdZnE0VQyFX2sBQyEDvAPmU+4rmHe5rvh7eH001ZA@mail.gmail.com","threadId":"42858","inReplyTo":"2ED67396-2530-4D1C-8F21-1C30983DB9DC@kellerfarm.com","subject":"Re: obsolete index in wt_status_print after pre-commit hook runs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-15T21:19:04Z","receivedAt":"2016-07-15T21:19:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Fri, Jul 15, 2016 at 1:30 PM, Andrew Keller <andrew@kellerfarm.com> wrote:\n> Am 15.07.2016 um 12:34 nachm. schrieb Andrew Keller <andrew@kellerfarm.com>:\n>\n>> I pulled out the source for version 2.9.1 and briefly skimmed how run_commit and\n>> prepare_to_commit work.  It seems that Git already understands that a pre-commit\n>> hook can change the index, and it rereads the index before running the\n>> prepare-commit-msg hook: https://github.com/git/git/blob/v2.9.1/builtin/commit.c#L941-L951\n>\n> Quick question: Why does Git reread the index after the pre-commit hook runs?\n\nOffhand I do not think of a good reason to do so; does something break\nif you took it out?\n"},{"id":"291570","messageId":"xmqqh9bqlfto.fsf@gitster.mtv.corp.google.com","threadId":"42858","inReplyTo":"CAPc5daWZofdZnE0VQyFX2sBQyEDvAPmU+4rmHe5rvh7eH001ZA@mail.gmail.com","subject":"Re: obsolete index in wt_status_print after pre-commit hook runs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-15T22:03:15Z","receivedAt":"2016-07-15T22:03:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> On Fri, Jul 15, 2016 at 1:30 PM, Andrew Keller <andrew@kellerfarm.com> wrote:\n>> Am 15.07.2016 um 12:34 nachm. schrieb Andrew Keller <andrew@kellerfarm.com>:\n>>\n>>> I pulled out the source for version 2.9.1 and briefly skimmed how run_commit and\n>>> prepare_to_commit work.  It seems that Git already understands that a pre-commit\n>>> hook can change the index, and it rereads the index before running the\n>>> prepare-commit-msg hook: https://github.com/git/git/blob/v2.9.1/builtin/commit.c#L941-L951\n>>\n>> Quick question: Why does Git reread the index after the pre-commit hook runs?\n>\n> Offhand I do not think of a good reason to do so; does something break\n> if you took it out?\n\nAhh, I misremembered.  2888605c (builtin-commit: fix partial-commit\nsupport, 2007-11-18) does consider the possibility that pre-commit\nmay have modified the index contents after we take control back from\nthat hook, so that is probably a good place to enumerate what got\nchanged.  Getting the list before running the hook can give an\nout-of-date list, as you said.\n\nThanks.\n"},{"id":"291577","messageId":"36F872B1-D5C4-4FC5-9B9E-5297C4B01950@kellerfarm.com","threadId":"42858","inReplyTo":"CAPc5daWZofdZnE0VQyFX2sBQyEDvAPmU+4rmHe5rvh7eH001ZA@mail.gmail.com","subject":"Re: obsolete index in wt_status_print after pre-commit hook runs","fromName":"Andrew Keller","fromEmail":"andrew@kellerfarm.com","sentAt":"2016-07-16T02:23:03Z","receivedAt":"2016-07-16T02:23:13Z","isPatch":false,"sender":{"key":"andrew@kellerfarm.com","avatar":"https://avatars.githubusercontent.com/u/304204?v=4"},"body":"Am 15.07.2016 um 5:19 nachm. schrieb Junio C Hamano <gitster@pobox.com>:\n> \n> On Fri, Jul 15, 2016 at 1:30 PM, Andrew Keller <andrew@kellerfarm.com> wrote:\n>> Am 15.07.2016 um 12:34 nachm. schrieb Andrew Keller <andrew@kellerfarm.com>:\n>> \n>>> I pulled out the source for version 2.9.1 and briefly skimmed how run_commit and\n>>> prepare_to_commit work.  It seems that Git already understands that a pre-commit\n>>> hook can change the index, and it rereads the index before running the\n>>> prepare-commit-msg hook: https://github.com/git/git/blob/v2.9.1/builtin/commit.c#L941-L951\n>> \n>> Quick question: Why does Git reread the index after the pre-commit hook runs?\n> \n> Offhand I do not think of a good reason to do so; does something break\n> if you took it out?\n\nAccording to only test failures, it seems that only the `update_main_cache_tree(0)` invocation\nis needed to avoid a torrent of test failures (490 failures across 102 tests).  Removing lines\n946, 947, 949, and 950 do not cause test breakages (although my computer is not set up to\nrun all of the tests).\n\nHowever, there seems to be an interaction between lines 946-947 and `update_main_cache_tree(0)`\non line 948: although lines 946-947 can be removed by themselves without test breakages,\nwhen 946-948 are all disabled together (and, in turn, lines 949-950 never run), one additional test\nfailure is registered (t2203.5).\n\nThanks,\n - Andrew Keller\n\n"},{"id":"291578","messageId":"06EA5AA7-3E0B-4C2E-B5C3-89399F9890D1@kellerfarm.com","threadId":"42858","inReplyTo":"xmqqh9bqlfto.fsf@gitster.mtv.corp.google.com","subject":"Re: obsolete index in wt_status_print after pre-commit hook runs","fromName":"Andrew Keller","fromEmail":"andrew@kellerfarm.com","sentAt":"2016-07-16T02:39:23Z","receivedAt":"2016-07-16T02:39:35Z","isPatch":false,"sender":{"key":"andrew@kellerfarm.com","avatar":"https://avatars.githubusercontent.com/u/304204?v=4"},"body":"Am 15.07.2016 um 6:03 nachm. schrieb Junio C Hamano <gitster@pobox.com>:\n> Junio C Hamano <gitster@pobox.com> writes:\n>> On Fri, Jul 15, 2016 at 1:30 PM, Andrew Keller <andrew@kellerfarm.com> wrote:\n>>> Am 15.07.2016 um 12:34 nachm. schrieb Andrew Keller <andrew@kellerfarm.com>:\n>>> \n>>>> I pulled out the source for version 2.9.1 and briefly skimmed how run_commit and\n>>>> prepare_to_commit work.  It seems that Git already understands that a pre-commit\n>>>> hook can change the index, and it rereads the index before running the\n>>>> prepare-commit-msg hook: https://github.com/git/git/blob/v2.9.1/builtin/commit.c#L941-L951\n>>> \n>>> Quick question: Why does Git reread the index after the pre-commit hook runs?\n>> \n>> Offhand I do not think of a good reason to do so; does something break\n>> if you took it out?\n> \n> Ahh, I misremembered.  2888605c (builtin-commit: fix partial-commit\n> support, 2007-11-18) does consider the possibility that pre-commit\n> may have modified the index contents after we take control back from\n> that hook, so that is probably a good place to enumerate what got\n> changed.  Getting the list before running the hook can give an\n> out-of-date list, as you said.\n\nInteresting.  So, the implication is that disallowing the pre-commit hook\nto change the index may cause some problems (491 problems, if my run\nof the tests was accurate).\n\nDoes that mean it would be desirable to update the index before the\ncommit message template is rendered?\n\nThanks,\n - Andrew Keller\n\n"},{"id":"292961","messageId":"CDE30958-C112-4C26-A0EA-499BFCD4E07F@kellerfarm.com","threadId":"42858","inReplyTo":"xmqqh9bqlfto.fsf@gitster.mtv.corp.google.com","subject":"Re: obsolete index in wt_status_print after pre-commit hook runs","fromName":"Andrew Keller","fromEmail":"andrew@kellerfarm.com","sentAt":"2016-08-03T18:25:22Z","receivedAt":"2016-08-03T18:30:46Z","isPatch":false,"sender":{"key":"andrew@kellerfarm.com","avatar":"https://avatars.githubusercontent.com/u/304204?v=4"},"body":"Am 15.07.2016 um 6:03 nachm. schrieb Junio C Hamano <gitster@pobox.com>:\n> \n> Ahh, I misremembered.  2888605c (builtin-commit: fix partial-commit\n> support, 2007-11-18) does consider the possibility that pre-commit\n> may have modified the index contents after we take control back from\n> that hook, so that is probably a good place to enumerate what got\n> changed.  Getting the list before running the hook can give an\n> out-of-date list, as you said.\n\nI’ve been experimenting with two different workflows recently:\n\n    (1) Identify problem files during the pre-commit hook;\n        when found, fix them automatically in the index and let the commit continue.\n    (2) Identify problem files during the pre-commit hook;\n        when found, provide instructions to fix the problem (and possibly set a helpful\n        Git alias to do it in one command), and abort the commit.  Require that the user fixup\n        the index and try the commit again.\n\nAnd here are my thoughts:\n\n#1 seems to be quick and simple for the user, and it plays (mostly) nice with scripts\nand IDEs that do commits autonomously, but I’m having trouble trusting that my\npre-commit hook made the *correct* changes (even though it’s worked nicely so far)\n(i.e., I keep looking at the new HEAD commit to make sure it looks right, where\nnormally I just look at the index and make sure it looks right).\n\n#2 is slightly more difficult to implement just because it has more moving parts,\nhowever I’m finding that because I can interrogate the index after I manually run\nthe command to make the required changes to the index, and *before* I commit\nagain, I feel much more confident that I know what is going to be in my commit.\nHowever, this approach doesn’t play well with automated scripts that assume that\na commit operation will always work.\n\nIn summary, I think I prefer #2 from a usability point of view, however I’m having\ntrouble proving that #1 is actually *bad* and should be disallowed.\n\nAny thoughts?  Would it be better for the pre-commit hook to be officially allowed to\nedit the index [1], or would it be better for the pre-commit hook to explicitly *not* be\nallowed to edit the index [2], or would it be yet even better to simply leave it as it is?\n\n[1] and possibly create a patch that teaches builtin/commit.c to reread the index\n    after the pre-commit hook runs and before rendering the commit message template\n[2] and possibly create a patch that teaches builtin/commit.c to detect changes to the\n    index after the pre-commit hook runs\n\nThanks,\n - Andrew Keller\n\n"},{"id":"293100","messageId":"xmqq60rg5vq5.fsf@gitster.mtv.corp.google.com","threadId":"42858","inReplyTo":"CDE30958-C112-4C26-A0EA-499BFCD4E07F@kellerfarm.com","subject":"Re: obsolete index in wt_status_print after pre-commit hook runs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-04T16:45:22Z","receivedAt":"2016-08-04T16:46:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrew Keller <andrew@kellerfarm.com> writes:\n\n> In summary, I think I prefer #2 from a usability point of view, however I’m having\n> trouble proving that #1 is actually *bad* and should be disallowed.\n\nYeah, I agree with your argument from the usability and safety point\nof view.\n\n> Any thoughts?  Would it be better for the pre-commit hook to be\n> officially allowed to edit the index [1], or would it be better\n> for the pre-commit hook to explicitly *not* be allowed to edit the\n> index [2], or would it be yet even better to simply leave it as it\n> is?\n\nIt is clear that our stance has been the third one so far.\n\nAnother thing I did not see in your analysis is what happens if the\nuser is doing a partial commit, and how the changes made by\npre-commit hook is propagated back to the main index and the working\ntree.\n\nThe HEAD may have a file with contents in the \"original\" state, the\nindex may have the file with \"update 1\", and the working tree file\nmay have it with \"update 2\".  After the commit is made, the user\nwill continue working from a state where the HEAD and the index have\n\"update 1\", and the working tree has \"update 2\".  \"git diff file\"\noutput before and after the commit will be identical (i.e. the\ndifference between \"update 1\" and \"update 2\") as expected.\n\nIf pre-commit were allowed to munge the index to have the file in\nthe \"update 3\" state, the resulting commit would have that version\nof the file in its tree.  By definition, \"update 1\" and \"update 3\"\nare different (that is what it means to allow pre-commit to munge\nthe index); where should the differences between \"update 1\" and\n\"update 3\" go?  It is clear that pre-commit thought that the\ncontents in the \"update 1\" state is bad and \"update 3\" state is\nbetter (that is why it made that fix), so after the commit is made,\nwe would want to have \"update 3\" in the index.  But what would you\ndo to the working tree file, which is in \"update 2\" state?  If you\ndo not do anything, \"git diff\" would show the remaining edit the\nuser had before starting the commit (i.e. difference between \"update\n1\" and \"update 2\") plus a reversion of the edit pre-commit made\nbecause what the working tree has, \"update 2\", is based on \"update 1\"\nand has never heard of the change pre-commit did.\n\nBut leaving the working tree file as-is is the only safe choice, as\nI do not think we want \"git commit\" to _create_ new conflict in the\nworking tree by attempting to merge (we _could_, and implementing it\nwould be a trivial thing to do by calling ll_merge() to three-way\nmerge \"update 2\" and \"update 3\" that are both based on \"update 1\",\nbut the result from the end-user's point of view is too _weird_).\n\nSo, I tend to think we should not allow pre-commit to munge the\nindex.  We should be able to detect fairly cheaply if pre-commit\nmunged the index by remembering the trailing SHA-1 of the index file\ngiven to the pre-commit hook before running it, and reading the\ntrailing SHA-1 of the index file left after the pre-commit hook and\ncomparing them.  And we would yell at the user that his pre-commit\nmunged the index and abort.\n\nOr something like that.\n\n\n\n"},{"id":"293189","messageId":"B126EBED-AA93-4B5B-A932-149E4CB88C2B@kellerfarm.com","threadId":"42858","inReplyTo":"xmqq60rg5vq5.fsf@gitster.mtv.corp.google.com","subject":"Re: obsolete index in wt_status_print after pre-commit hook runs","fromName":"Andrew Keller","fromEmail":"andrew@kellerfarm.com","sentAt":"2016-08-05T13:22:14Z","receivedAt":"2016-08-05T13:22:26Z","isPatch":false,"sender":{"key":"andrew@kellerfarm.com","avatar":"https://avatars.githubusercontent.com/u/304204?v=4"},"body":"\nAm 04.08.2016 um 12:45 nachm. schrieb Junio C Hamano <gitster@pobox.com>:\n> Andrew Keller <andrew@kellerfarm.com> writes:\n> \n>> In summary, I think I prefer #2 from a usability point of view, however I’m having\n>> trouble proving that #1 is actually *bad* and should be disallowed.\n> \n> Yeah, I agree with your argument from the usability and safety point\n> of view.\n> \n>> Any thoughts?  Would it be better for the pre-commit hook to be\n>> officially allowed to edit the index [1], or would it be better\n>> for the pre-commit hook to explicitly *not* be allowed to edit the\n>> index [2], or would it be yet even better to simply leave it as it\n>> is?\n> \n> It is clear that our stance has been the third one so far.\n> \n> Another thing I did not see in your analysis is what happens if the\n> user is doing a partial commit, and how the changes made by\n> pre-commit hook is propagated back to the main index and the working\n> tree.\n> \n> The HEAD may have a file with contents in the \"original\" state, the\n> index may have the file with \"update 1\", and the working tree file\n> may have it with \"update 2\".  After the commit is made, the user\n> will continue working from a state where the HEAD and the index have\n> \"update 1\", and the working tree has \"update 2\".  \"git diff file\"\n> output before and after the commit will be identical (i.e. the\n> difference between \"update 1\" and \"update 2\") as expected.\n\nExcellent point — one I had discovered myself but neglected to include in\nmy email.  In my post-commit hook, I have logic in both versions of my\nexperiment that disallows [1] fixing up diffs that are partially staged.  Both\nscripts then update both the index and the working copy.  (Sort of like how\nrebase works — clean working directory required, and then it updates the\nindex and the work tree)\n\n[1] In version #1, if any files it wants to change are partially staged, it\n    prints a detailed error message and aborts the commit outright.  In\n    version #2, the pre-commit hook sees the change it _wants_ to make,\n    informs the user that he/she should run the fixup command, aborts\n    the commit, and when the user runs the fixup command, the fixup\n    command sees the partially staged file, prints the same detailed error\n    message, and dies.\n\nThanks for your help on this.  it’s really been interesting.  I’ll leave it as-is\nfor now.\n\nThanks,\n - Andrew Keller\n\n"}]}