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

[Junio C Hamano <gitster@pobox.com>]

Andrew Keller <andrew@kellerfarm.com> 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.