Re: obsolete index in wt_status_print after pre-commit hook runs

From: Andrew Keller <andrew@kellerfarm.com>
Date: Aug 3, 2016, 18:25 UTC
Message-ID: <CDE30958-C112-4C26-A0EA-499BFCD4E07F@kellerfarm.com>
In-Reply-To: <xmqqh9bqlfto.fsf@gitster.mtv.corp.google.com>

Am 15.07.2016 um 6:03 nachm. schrieb Junio C Hamano <gitster@pobox.com>:

Show 7 quoted lines

> 
> 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