On Mon, Oct 10, 2011 at 2:05 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Jay Soffian <jaysoffian@xxxxxxxxx> writes:
>> +static void prepare_to_commit(void)
>> +{
>> + struct strbuf msg = STRBUF_INIT;
>> + strbuf_addbuf(&msg, &merge_msg);
>> + strbuf_addch(&msg, '\n');
>> + write_merge_msg(&msg);
>> run_hook(get_index_file(), "prepare-commit-msg",
>> git_path("MERGE_MSG"), "merge", NULL, NULL);
>> - read_merge_msg();
>> + if (option_edit) {
>> + if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
>> + abort_commit(NULL);
>> + }
>> + read_merge_msg(&msg);
>> + stripspace(&msg, option_edit);
>> + if (!msg.len)
>> + abort_commit(_("Empty commit message."));
>> + strbuf_release(&merge_msg);
>> + strbuf_addbuf(&merge_msg, &msg);
>> + strbuf_release(&msg);
>> }
>
> An abstraction very nicely done.
<blush> :-)
> I am not sure about the '\n' you unconditionally added at the end of the
> existing message.
Right, the old code does that when the merge fails, counting on (I
think) git-commit to then take care of any extra newlines. My
reasoning was tack it on before running prepare-commit-msg, then run
stripspace() after the hook and and editor, which will take care of
any excess newlines. I guess this would be a regression if someone's
prepare-commit-msg hook blindly appends to the commit message.
> I think running stripspace(&msg, option_edit) is a good change, even
> though some people might feel it is a regression. "git commit" also cleans
> up the whitespace cruft left by prepare-commit-message hook when the
> editor is not in use, and this change makes it consistent.
Correct.
>> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
>> index 87aac835a1..8c6b811718 100755
>> --- a/t/t7600-merge.sh
>> +++ b/t/t7600-merge.sh
>> @@ -643,4 +643,19 @@ test_expect_success 'amending no-ff merge commit' '
>>
>> test_debug 'git log --graph --decorate --oneline --all'
>>
>> +cat >editor <<\EOF
>> +#!/bin/sh
>> +# strip comments and blank lines from end of message
>> +sed -e '/^#/d' < "$1" | sed -e :a -e '/^\n*$/{$d;N;ba' -e '}' > expected
>> +EOF
>> +chmod 755 editor
>
> I am not sure about this one. Wouldn't this want to be editing the given
> file to make sure that the edited content appear in the result, not just
> testing the additional stripspace() call you added in the codepath?
Yep.
I added this test under the previous iteration of the patch when I was
concerned that commit message make it through the external commit code
path correctly. It doesn't really make sense with this iteration now
that I think about it. The part about stripping comments and newlines
is no longer needed.
>> +test_expect_success 'merge --no-ff --edit' '
>> + git reset --hard c0 &&
>> + EDITOR=./editor git merge --no-ff --edit c1 &&
>> + verify_parents $c0 $c1 &&
>> + git cat-file commit HEAD | sed "1,/^$/d" > actual &&
>> + test_cmp actual expected
>> +'
>> +
>> test_done
>
> So perhaps this one on top? I am just suspecting that your additional '\n'
> is to make sure we do not write out a file with an incomplete line with
> this patch, but that change is not explained in your commit log message,
> so I am not sure.
I assumed the '\n' was needed as it's added (unconditionally) before
writing MERGE_MSG when the merge fails. I didn't notice that when I
added the prepare-commit-msg hook support to merge.c (65969d43d1).
> builtin/merge.c | 3 ++-
> t/t7600-merge.sh | 10 +++++++++-
> 2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index a8dbf4a..09ffc07 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -867,7 +867,8 @@ static void prepare_to_commit(void)
> {
> struct strbuf msg = STRBUF_INIT;
> strbuf_addbuf(&msg, &merge_msg);
> - strbuf_addch(&msg, '\n');
> + if (msg.len && msg.buf[msg.len-1] != '\n')
> + strbuf_addch(&msg, '\n');
> write_merge_msg(&msg);
> run_hook(get_index_file(), "prepare-commit-msg",
> git_path("MERGE_MSG"), "merge", NULL, NULL);
I'm guessing the '\n' is always needed (per above), but I'm not sure.
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 8c6b811..3008e4e 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -645,6 +645,12 @@ test_debug 'git log --graph --decorate --oneline --all'
>
> cat >editor <<\EOF
> #!/bin/sh
> +# Add a new message string that was not in the template
> +(
> + echo "Merge work done on the side branch c1"
> + echo
> + cat <"$1"
> +) >"$1.tmp" && mv "$1.tmp" "$1"
> # strip comments and blank lines from end of message
> sed -e '/^#/d' < "$1" | sed -e :a -e '/^\n*$/{$d;N;ba' -e '}' > expected
> EOF
> @@ -654,7 +660,9 @@ test_expect_success 'merge --no-ff --edit' '
> git reset --hard c0 &&
> EDITOR=./editor git merge --no-ff --edit c1 &&
> verify_parents $c0 $c1 &&
> - git cat-file commit HEAD | sed "1,/^$/d" > actual &&
> + git cat-file commit HEAD >raw &&
> + grep "work done on the side branch" raw &&
> + sed "1,/^$/d" >actual raw &&
> test_cmp actual expected
> '
Okay. A test that the merge is aborted if the message is empty would
also be good.
I'll try to find time to send another iteration with better tests. May
not be till next week though.
j.
--
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