Andrew Keller <andrew@xxxxxxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html