Re: [PATCH v2 3/4] merge: make prepare_to_commit responsible for write_merge_state
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 11, 2014, 00:11 UTC
- Message-ID
- <xmqqlhyn74yo.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1389228344-38813-4-git-send-email-ryan@yaauie.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.
Show 7 quoted lines
> 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.
Show 37 quoted lines
> 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 &&
Show 6 quoted lines
> + 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
Show 7 quoted lines
> + 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 &&Show 5 quoted lines
> + test ! -s "$(git rev-parse --git-dir)/MERGE_HEAD" > + > +' > + > test_done