Re: [PATCH v2 3/4] merge: make prepare_to_commit responsible for write_merge_state

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

Ryan Biesemeyer <ryan@yaauie.com> writes:

> When merging, make the prepare-commit-msg hook have access to the merge
> state in order to make decisions about the commit message it is preparing.

What information is currently not available, and if available how
would that help the hook to formulate a better message?

	I am not asking you to answer the question to me in an
	e-mail response. The above is an example of a natural
	question a reader of the above statement would have and
	would wish the log message already answered when the reader
	read it.

> Since `abort_commit` is *only* called after `write_merge_state`, and a
> successful commit *always* calls `drop_save` to clear the saved state, this
> change effectively ensures that the merge state is also available to the
> prepare-commit-msg hook and while the message is being edited.
>
> Signed-off-by: Ryan Biesemeyer <ryan@yaauie.com>
> ---

OK.  The most important part is that this makes sure that these
intermediate state files never remains after the normal codepath
finishes what it does.

You seem to be only interested in prepare-commit-msg hook, but
because of your calling write_merge_state() early, these state files
will persist while we call finish() and they are also visible while
the post-merge hook is run.  While I think it may be a good thing
that the post-merge hook too can view that information, the log
message makes it sound as if it is an unintended side effect of the
change.

>  builtin/merge.c                    |  3 ++-
>  t/t7505-prepare-commit-msg-hook.sh | 21 +++++++++++++++++++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 4941a6c..b7bfc9c 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -802,7 +802,6 @@ static void abort_commit(struct commit_list *remoteheads, const char *err_msg)
>  		error("%s", err_msg);
>  	fprintf(stderr,
>  		_("Not committing merge; use 'git commit' to complete the merge.\n"));
> -	write_merge_state(remoteheads);
>  	exit(1);
>  }
>  
> @@ -816,6 +815,8 @@ N_("Please enter a commit message to explain why this merge is necessary,\n"
>  static void prepare_to_commit(struct commit_list *remoteheads)
>  {
>  	struct strbuf msg = STRBUF_INIT;
> +
> +	write_merge_state(remoteheads);
>  	strbuf_addbuf(&msg, &merge_msg);
>  	strbuf_addch(&msg, '\n');
>  	if (0 < option_edit)
> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
> index 697ecc0..7ccf870 100755
> --- a/t/t7505-prepare-commit-msg-hook.sh
> +++ b/t/t7505-prepare-commit-msg-hook.sh
> @@ -183,4 +183,25 @@ test_expect_success 'with failing hook (merge)' '
>    )
>  '
>  
> +test_expect_success 'should have MERGE_HEAD (merge)' '
> +
> +	git checkout -B other HEAD@{1} &&
> +	echo "more" >> file &&

Style: no SP between the redirection operator and its target, i.e.

	echo more >>file &&

> +	git add file &&
> +	rm -f "$HOOK" &&
> +	git commit -m other &&
> +	git checkout - &&
> +	write_script "$HOOK" <<-EOF &&
> +	if [ -s "$(git rev-parse --git-dir)/MERGE_HEAD" ]; then

Style: no [], and no semicolon to join two lines of control
statement into one, i.e.

	if test -s ...
	then

> +		exit 0
> +	else
> +		exit 1
> +	fi
> +	EOF
> +	git merge other &&
> +	test "`git log -1 --pretty=format:%s`" = "Merge branch '"'"'other'"'"'" &&

Style:

    - After "sh t7505-*.sh v -i" fails for whatever reason, being
      able to inspect the trash directory to see what actually was
      produced is much easier way to debug than having to re-run the
      command with "sh -x" to peek into what the "test" statement is
      getting.

    - $(...) is much easier to read than `...`, but you do not have
      to use either if you follow the above.

i.e.

	git log -1 --format=%s >actual &&
        echo "Merge branch '\''other'\''" >expect &&
        test_cmp expect actual &&

> +	test ! -s "$(git rev-parse --git-dir)/MERGE_HEAD"
> +
> +'
> +
>  test_done