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

Andrew Keller [off-list ref] 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.

Am 04.08.2016 um 12:45 nachm. schrieb Junio C Hamano [off-list ref]:

Andrew Keller [off-list ref] writes:

quoted

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.

quoted

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