Re: [PATCH v3] Teach merge the '[-e|--edit]' option
From: Jay Soffian <hidden>
Date: 2016-06-15 22:52:13
On Mon, Oct 10, 2011 at 2:05 PM, Junio C Hamano [off-list ref] wrote:
Jay Soffian [off-list ref] writes:quoted
+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.
quoted
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 editorI 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.
quoted
+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_doneSo 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).
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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.