Re: [PATCH RFC v2 03/19] rebase -i: reword executes pre-commit hook on interim commit

[Fabian Ruch <bafain@gmail.com>]

Hi Junio,

Junio C Hamano writes:
> Fabian Ruch <bafain@gmail.com> writes:
>> The to-do list command `reword` replays a commit like `pick` but lets
>> the user also edit the commit's log message. This happens in two
>> steps. Firstly, the named commit is cherry-picked. Secondly, the
>> commit created in the first step is amended using an unchanged index
>> to edit the log message. The pre-commit hook is meant to verify the
>> changes introduced by a commit (for instance, catching inserted
>> trailing white space). Since the committed tree is not changed
>> between the two steps, do not execute the pre-commit hook in the
>> second step.
> 
> It is not like the second step is built as a child commit of the
> result from the first step, recording the same tree without any
> change.  Why is it a good thing not to run the pre-commit hook (or
> other hooks for that matter)?  After all, the result of the second
> step is what is recorded in the final history; it just feels wrong
> not to test that one, even if it were a good idea to test only once.

if I understand correctly, the tree of the (amended) commit isn't tested
at all by git-rebase--interactive because git-cherry-pick, and therefore
pick_one, commits with both the pre-commit and commit-msg hook disabled
(see the "git commit -n" command line being built in
sequencer.c#run_git_commit since revision 9fa4db5).

The "commit --amend" we are concerned with here amends using an
untouched tree so that the history ought to record exactly the same
trees as prior to "commit --amend". If git-cherry-pick was testing the
tree, then it would be unnecessary to test the tree a second time. Since
git-cherry-pick is not testing as of yet, testing and possibly rejecting
a tree in the second step would actually be wrong. I totally neglected
to look for a test case when I submitted this patch, so I'd like to
supply one late now:

> test_expect_success 'reword a commit violating pre-commit' '
> 	mkdir -p .git/hooks &&
> 	PRE_COMMIT=.git/hooks/pre-commit &&
> 	cat >"$PRE_COMMIT" <<EOF
> #!/bin/sh
> echo running pre-commit
> exit 1
> EOF
> 	chmod +x "$PRE_COMMIT" &&
> 	test_must_fail test_commit file &&
> 	test_commit --no-verify file &&
> 	set_fake_editor &&
> 	FAKE_LINES="pick 1" git rebase -i HEAD^ &&
> 	FAKE_LINES="pick 1" git rebase -i --no-ff HEAD^ &&
> 	FAKE_LINES="reword 1" git rebase -i HEAD^
> '

(This addition to t3404-rebase--interactive.sh is based on the test case
"rebase a commit violating pre-commit"; "test_commit --no-verify" will
be implemented the obvious way.)

In both cases, it's correct to disable the pre-commit hook when editing
the log message and it just makes sense to test changes where you make
them. Unfortunately, it is not obvious that "git commit --amend" merely
edits the log message rather than committing a new tree.

For what it's worth, if we were to create an empty child commit and
squash it into the parent instead of amending immediately, then
git-rebase--interactive would disable tree verification in the second
step as well. This behaviour was introduced by commit c5b09fe. Although
the change seems to have been triggered by something completely
different back then, the correctness criterion remains the same, i.e.
recording previously committed trees.

Best regards,
   Fabian

>> Specify the git-commit option `--no-verify` to disable the pre-commit
>> hook when editing the log message. Because `--no-verify` also skips
>> the commit-msg hook, execute the hook from within
>> git-rebase--interactive after the commit is created. Fortunately, the
>> commit message is still available in `$GIT_DIR/COMMIT_EDITMSG` after
>> git-commit terminates. Caveat: In case the commit-msg hook finds the
>> new log message ill-formatted, the user is only notified of the
>> failed commit-msg hook but the log message is used for the commit
>> anyway. git-commit ought to offer more fine-grained control over
>> which hooks are executed.
>>
>> Signed-off-by: Fabian Ruch <bafain@gmail.com>
>> ---
>>  git-rebase--interactive.sh | 17 +++++++++++++----
>>  1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index 689400e..b50770d 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -504,10 +504,19 @@ do_next () {
>>  
>>  		mark_action_done
>>  		do_pick $sha1 "$rest"
>> -		git commit --allow-empty --amend --no-post-rewrite ${gpg_sign_opt:+"$gpg_sign_opt"} || {
>> -			warn "Could not amend commit after successfully picking $sha1... $rest"
>> -			exit_with_patch $sha1 1
>> -		}
>> +		# TODO: Work around the fact that git-commit lets us
>> +		# disable either both the pre-commit and the commit-msg
>> +		# hook or none. Disable the pre-commit hook because the
>> +		# tree is left unchanged but run the commit-msg hook
>> +		# from here because the log message is altered.
>> +		git commit --allow-empty --amend --no-post-rewrite -n ${gpg_sign_opt:+"$gpg_sign_opt"} &&
>> +			if test -x "$GIT_DIR"/hooks/commit-msg
>> +			then
>> +				"$GIT_DIR"/hooks/commit-msg "$GIT_DIR"/COMMIT_EDITMSG
>> +			fi || {
>> +				warn "Could not amend commit after successfully picking $sha1... $rest"
>> +				exit_with_patch $sha1 1
>> +			}
>>  		record_in_rewritten $sha1
>>  		;;
>>  	edit|e)