{"thread":{"id":"28635","subject":"[PATCH v3] Teach merge the '[-e|--edit]' option","startedAt":"2011-10-08T18:39:52Z","lastAt":"2011-10-11T13:57:47Z","messageCount":6,"participants":["Jay Soffian","Junio C Hamano","Peter Krefting"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"177248","messageId":"1318099192-60860-1-git-send-email-jaysoffian@gmail.com","threadId":"28635","inReplyTo":null,"subject":"[PATCH v3] Teach merge the '[-e|--edit]' option","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-10-08T18:39:52Z","receivedAt":"2011-10-08T18:39:52Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"Implemented internally instead of as \"git merge --no-commit && git commit\"\nso that \"merge --edit\" is otherwise consistent (hooks, etc) with \"merge\".\n\nNote: the edit message does not include the status information that one\ngets with \"commit --status\" and it is cleaned up after editing like one\ngets with \"commit --cleanup=default\". A later patch could add the status\ninformation if desired.\n\nNote: previously we were not calling stripspace() after running the\nprepare-commit-msg hook. Now we are, stripping comments and\nleading/trailing whitespace lines if --edit is given, otherwise only\nstripping leading/trailing whitespace lines if not given --edit.\n\nSigned-off-by: Jay Soffian <jaysoffian@gmail.com>\n---\nI probably can't spend more time on this patch any time soon. Hopefully\nthis iteration is close enough. If not, maybe Todd can help.\n\n Documentation/merge-options.txt |    6 ++\n builtin/merge.c                 |  109 +++++++++++++++++++++++++--------------\n t/t7600-merge.sh                |   15 +++++\n 3 files changed, 91 insertions(+), 39 deletions(-)\n\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex b613d4ed08..6bd0b041c3 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -7,6 +7,12 @@ With --no-commit perform the merge but pretend the merge\n failed and do not autocommit, to give the user a chance to\n inspect and further tweak the merge result before committing.\n \n+--edit::\n+-e::\n++\n+\tInvoke editor before committing successful merge to further\n+\tedit the default merge message.\n+\n --ff::\n --no-ff::\n \tDo not generate a merge commit if the merge resolved as\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex ee56974371..fcb7a60bfa 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -46,7 +46,7 @@ static const char * const builtin_merge_usage[] = {\n \n static int show_diffstat = 1, shortlog_len, squash;\n static int option_commit = 1, allow_fast_forward = 1;\n-static int fast_forward_only;\n+static int fast_forward_only, option_edit;\n static int allow_trivial = 1, have_message;\n static struct strbuf merge_msg;\n static struct commit_list *remoteheads;\n@@ -190,6 +190,8 @@ static struct option builtin_merge_options[] = {\n \t\t\"create a single commit instead of doing a merge\"),\n \tOPT_BOOLEAN(0, \"commit\", &option_commit,\n \t\t\"perform a commit if the merge succeeds (default)\"),\n+\tOPT_BOOLEAN('e', \"edit\", &option_edit,\n+\t\t\"edit message before committing\"),\n \tOPT_BOOLEAN(0, \"ff\", &allow_fast_forward,\n \t\t\"allow fast-forward (default)\"),\n \tOPT_BOOLEAN(0, \"ff-only\", &fast_forward_only,\n@@ -842,30 +844,54 @@ static void add_strategies(const char *string, unsigned attr)\n \n }\n \n-static void write_merge_msg(void)\n+static void write_merge_msg(struct strbuf *msg)\n {\n \tint fd = open(git_path(\"MERGE_MSG\"), O_WRONLY | O_CREAT, 0666);\n \tif (fd < 0)\n \t\tdie_errno(_(\"Could not open '%s' for writing\"),\n \t\t\t  git_path(\"MERGE_MSG\"));\n-\tif (write_in_full(fd, merge_msg.buf, merge_msg.len) != merge_msg.len)\n+\tif (write_in_full(fd, msg->buf, msg->len) != msg->len)\n \t\tdie_errno(_(\"Could not write to '%s'\"), git_path(\"MERGE_MSG\"));\n \tclose(fd);\n }\n \n-static void read_merge_msg(void)\n+static void read_merge_msg(struct strbuf *msg)\n {\n-\tstrbuf_reset(&merge_msg);\n-\tif (strbuf_read_file(&merge_msg, git_path(\"MERGE_MSG\"), 0) < 0)\n+\tstrbuf_reset(msg);\n+\tif (strbuf_read_file(msg, git_path(\"MERGE_MSG\"), 0) < 0)\n \t\tdie_errno(_(\"Could not read from '%s'\"), git_path(\"MERGE_MSG\"));\n }\n \n-static void run_prepare_commit_msg(void)\n+static void write_merge_state(void);\n+static void abort_commit(const char *err_msg)\n {\n-\twrite_merge_msg();\n+\tif (err_msg)\n+\t\terror(\"%s\", err_msg);\n+\tfprintf(stderr,\n+\t\t_(\"Not committing merge; use 'git commit' to complete the merge.\\n\"));\n+\twrite_merge_state();\n+\texit(1);\n+}\n+\n+static void prepare_to_commit(void)\n+{\n+\tstruct strbuf msg = STRBUF_INIT;\n+\tstrbuf_addbuf(&msg, &merge_msg);\n+\tstrbuf_addch(&msg, '\\n');\n+\twrite_merge_msg(&msg);\n \trun_hook(get_index_file(), \"prepare-commit-msg\",\n \t\t git_path(\"MERGE_MSG\"), \"merge\", NULL, NULL);\n-\tread_merge_msg();\n+\tif (option_edit) {\n+\t\tif (launch_editor(git_path(\"MERGE_MSG\"), NULL, NULL))\n+\t\t\tabort_commit(NULL);\n+\t}\n+\tread_merge_msg(&msg);\n+\tstripspace(&msg, option_edit);\n+\tif (!msg.len)\n+\t\tabort_commit(_(\"Empty commit message.\"));\n+\tstrbuf_release(&merge_msg);\n+\tstrbuf_addbuf(&merge_msg, &msg);\n+\tstrbuf_release(&msg);\n }\n \n static int merge_trivial(void)\n@@ -879,7 +905,7 @@ static int merge_trivial(void)\n \tparent->next = xmalloc(sizeof(*parent->next));\n \tparent->next->item = remoteheads->item;\n \tparent->next->next = NULL;\n-\trun_prepare_commit_msg();\n+\tprepare_to_commit();\n \tcommit_tree(merge_msg.buf, result_tree, parent, result_commit, NULL);\n \tfinish(result_commit, \"In-index merge\");\n \tdrop_save();\n@@ -907,9 +933,9 @@ static int finish_automerge(struct commit_list *common,\n \t\tfor (j = remoteheads; j; j = j->next)\n \t\t\tpptr = &commit_list_insert(j->item, pptr)->next;\n \t}\n-\tfree_commit_list(remoteheads);\n \tstrbuf_addch(&merge_msg, '\\n');\n-\trun_prepare_commit_msg();\n+\tprepare_to_commit();\n+\tfree_commit_list(remoteheads);\n \tcommit_tree(merge_msg.buf, result_tree, parents, result_commit, NULL);\n \tstrbuf_addf(&buf, \"Merge made by the '%s' strategy.\", wt_strategy);\n \tfinish(result_commit, buf.buf);\n@@ -1015,6 +1041,36 @@ static int setup_with_upstream(const char ***argv)\n \treturn i;\n }\n \n+static void write_merge_state(void)\n+{\n+\tint fd;\n+\tstruct commit_list *j;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tfor (j = remoteheads; j; j = j->next)\n+\t\tstrbuf_addf(&buf, \"%s\\n\",\n+\t\t\tsha1_to_hex(j->item->object.sha1));\n+\tfd = open(git_path(\"MERGE_HEAD\"), O_WRONLY | O_CREAT, 0666);\n+\tif (fd < 0)\n+\t\tdie_errno(_(\"Could not open '%s' for writing\"),\n+\t\t\t  git_path(\"MERGE_HEAD\"));\n+\tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n+\t\tdie_errno(_(\"Could not write to '%s'\"), git_path(\"MERGE_HEAD\"));\n+\tclose(fd);\n+\tstrbuf_addch(&merge_msg, '\\n');\n+\twrite_merge_msg(&merge_msg);\n+\tfd = open(git_path(\"MERGE_MODE\"), O_WRONLY | O_CREAT | O_TRUNC, 0666);\n+\tif (fd < 0)\n+\t\tdie_errno(_(\"Could not open '%s' for writing\"),\n+\t\t\t  git_path(\"MERGE_MODE\"));\n+\tstrbuf_reset(&buf);\n+\tif (!allow_fast_forward)\n+\t\tstrbuf_addf(&buf, \"no-ff\");\n+\tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n+\t\tdie_errno(_(\"Could not write to '%s'\"), git_path(\"MERGE_MODE\"));\n+\tclose(fd);\n+}\n+\n int cmd_merge(int argc, const char **argv, const char *prefix)\n {\n \tunsigned char result_tree[20];\n@@ -1418,33 +1474,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \n \tif (squash)\n \t\tfinish(NULL, NULL);\n-\telse {\n-\t\tint fd;\n-\t\tstruct commit_list *j;\n-\n-\t\tfor (j = remoteheads; j; j = j->next)\n-\t\t\tstrbuf_addf(&buf, \"%s\\n\",\n-\t\t\t\tsha1_to_hex(j->item->object.sha1));\n-\t\tfd = open(git_path(\"MERGE_HEAD\"), O_WRONLY | O_CREAT, 0666);\n-\t\tif (fd < 0)\n-\t\t\tdie_errno(_(\"Could not open '%s' for writing\"),\n-\t\t\t\t  git_path(\"MERGE_HEAD\"));\n-\t\tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n-\t\t\tdie_errno(_(\"Could not write to '%s'\"), git_path(\"MERGE_HEAD\"));\n-\t\tclose(fd);\n-\t\tstrbuf_addch(&merge_msg, '\\n');\n-\t\twrite_merge_msg();\n-\t\tfd = open(git_path(\"MERGE_MODE\"), O_WRONLY | O_CREAT | O_TRUNC, 0666);\n-\t\tif (fd < 0)\n-\t\t\tdie_errno(_(\"Could not open '%s' for writing\"),\n-\t\t\t\t  git_path(\"MERGE_MODE\"));\n-\t\tstrbuf_reset(&buf);\n-\t\tif (!allow_fast_forward)\n-\t\t\tstrbuf_addf(&buf, \"no-ff\");\n-\t\tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n-\t\t\tdie_errno(_(\"Could not write to '%s'\"), git_path(\"MERGE_MODE\"));\n-\t\tclose(fd);\n-\t}\n+\telse\n+\t\twrite_merge_state();\n \n \tif (merge_was_ok) {\n \t\tfprintf(stderr, _(\"Automatic merge went well; \"\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 87aac835a1..8c6b811718 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -643,4 +643,19 @@ test_expect_success 'amending no-ff merge commit' '\n \n test_debug 'git log --graph --decorate --oneline --all'\n \n+cat >editor <<\\EOF\n+#!/bin/sh\n+# strip comments and blank lines from end of message\n+sed -e '/^#/d' < \"$1\" | sed -e :a -e '/^\\n*$/{$d;N;ba' -e '}' > expected\n+EOF\n+chmod 755 editor\n+\n+test_expect_success 'merge --no-ff --edit' '\n+\tgit reset --hard c0 &&\n+\tEDITOR=./editor git merge --no-ff --edit c1 &&\n+\tverify_parents $c0 $c1 &&\n+\tgit cat-file commit HEAD | sed \"1,/^$/d\" > actual &&\n+\ttest_cmp actual expected\n+'\n+\n test_done\n-- \n1.7.7.147.g4f6dc9\n"},{"id":"177332","messageId":"7vd3e4k162.fsf@alter.siamese.dyndns.org","threadId":"28635","inReplyTo":"1318099192-60860-1-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH v3] Teach merge the '[-e|--edit]' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-10T21:05:25Z","receivedAt":"2011-10-10T21:05:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian <jaysoffian@gmail.com> writes:\n\n> Implemented internally instead of as \"git merge --no-commit && git commit\"\n> so that \"merge --edit\" is otherwise consistent (hooks, etc) with \"merge\".\n>\n> Note: the edit message does not include the status information that one\n> gets with \"commit --status\" and it is cleaned up after editing like one\n> gets with \"commit --cleanup=default\". A later patch could add the status\n> information if desired.\n>\n> Note: previously we were not calling stripspace() after running the\n> prepare-commit-msg hook. Now we are, stripping comments and\n> leading/trailing whitespace lines if --edit is given, otherwise only\n> stripping leading/trailing whitespace lines if not given --edit.\n>\n> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>\n> ---\n\nThanks.\n\n> +static void prepare_to_commit(void)\n> +{\n> +\tstruct strbuf msg = STRBUF_INIT;\n> +\tstrbuf_addbuf(&msg, &merge_msg);\n> +\tstrbuf_addch(&msg, '\\n');\n> +\twrite_merge_msg(&msg);\n>  \trun_hook(get_index_file(), \"prepare-commit-msg\",\n>  \t\t git_path(\"MERGE_MSG\"), \"merge\", NULL, NULL);\n> -\tread_merge_msg();\n> +\tif (option_edit) {\n> +\t\tif (launch_editor(git_path(\"MERGE_MSG\"), NULL, NULL))\n> +\t\t\tabort_commit(NULL);\n> +\t}\n> +\tread_merge_msg(&msg);\n> +\tstripspace(&msg, option_edit);\n> +\tif (!msg.len)\n> +\t\tabort_commit(_(\"Empty commit message.\"));\n> +\tstrbuf_release(&merge_msg);\n> +\tstrbuf_addbuf(&merge_msg, &msg);\n> +\tstrbuf_release(&msg);\n>  }\n\nAn abstraction very nicely done.\n\nI am not sure about the '\\n' you unconditionally added at the end of the\nexisting message.\n\nI think running stripspace(&msg, option_edit) is a good change, even\nthough some people might feel it is a regression. \"git commit\" also cleans\nup the whitespace cruft left by prepare-commit-message hook when the\neditor is not in use, and this change makes it consistent.\n\n> @@ -1015,6 +1041,36 @@ static int setup_with_upstream(const char ***argv)\n> ...\n> +static void write_merge_state(void)\n> +{\n> +\tint fd;\n> +\tstruct commit_list *j;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\tfor (j = remoteheads; j; j = j->next)\n> +\t\tstrbuf_addf(&buf, \"%s\\n\",\n> +\t\t\tsha1_to_hex(j->item->object.sha1));\n> +\tfd = open(git_path(\"MERGE_HEAD\"), O_WRONLY | O_CREAT, 0666);\n> +\tif (fd < 0)\n> +\t\tdie_errno(_(\"Could not open '%s' for writing\"),\n> +\t\t\t  git_path(\"MERGE_HEAD\"));\n> +\tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n> +\t\tdie_errno(_(\"Could not write to '%s'\"), git_path(\"MERGE_HEAD\"));\n> +\tclose(fd);\n> +\tstrbuf_addch(&merge_msg, '\\n');\n> +\twrite_merge_msg(&merge_msg);\n> +\tfd = open(git_path(\"MERGE_MODE\"), O_WRONLY | O_CREAT | O_TRUNC, 0666);\n> +\tif (fd < 0)\n> +\t\tdie_errno(_(\"Could not open '%s' for writing\"),\n> +\t\t\t  git_path(\"MERGE_MODE\"));\n> +\tstrbuf_reset(&buf);\n> +\tif (!allow_fast_forward)\n> +\t\tstrbuf_addf(&buf, \"no-ff\");\n> +\tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n> +\t\tdie_errno(_(\"Could not write to '%s'\"), git_path(\"MERGE_MODE\"));\n> +\tclose(fd);\n> +}\n\nAgain very nicely done.\n\n> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\n> index 87aac835a1..8c6b811718 100755\n> --- a/t/t7600-merge.sh\n> +++ b/t/t7600-merge.sh\n> @@ -643,4 +643,19 @@ test_expect_success 'amending no-ff merge commit' '\n>  \n>  test_debug 'git log --graph --decorate --oneline --all'\n>  \n> +cat >editor <<\\EOF\n> +#!/bin/sh\n> +# strip comments and blank lines from end of message\n> +sed -e '/^#/d' < \"$1\" | sed -e :a -e '/^\\n*$/{$d;N;ba' -e '}' > expected\n> +EOF\n> +chmod 755 editor\n\nI am not sure about this one. Wouldn't this want to be editing the given\nfile to make sure that the edited content appear in the result, not just\ntesting the additional stripspace() call you added in the codepath?\n\n> +test_expect_success 'merge --no-ff --edit' '\n> +\tgit reset --hard c0 &&\n> +\tEDITOR=./editor git merge --no-ff --edit c1 &&\n> +\tverify_parents $c0 $c1 &&\n> +\tgit cat-file commit HEAD | sed \"1,/^$/d\" > actual &&\n> +\ttest_cmp actual expected\n> +'\n> +\n>  test_done\n\nSo perhaps this one on top? I am just suspecting that your additional '\\n'\nis to make sure we do not write out a file with an incomplete line with\nthis patch, but that change is not explained in your commit log message,\nso I am not sure.\n\n builtin/merge.c  |    3 ++-\n t/t7600-merge.sh |   10 +++++++++-\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a8dbf4a..09ffc07 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -867,7 +867,8 @@ static void prepare_to_commit(void)\n {\n \tstruct strbuf msg = STRBUF_INIT;\n \tstrbuf_addbuf(&msg, &merge_msg);\n-\tstrbuf_addch(&msg, '\\n');\n+\tif (msg.len && msg.buf[msg.len-1] != '\\n')\n+\t\tstrbuf_addch(&msg, '\\n');\n \twrite_merge_msg(&msg);\n \trun_hook(get_index_file(), \"prepare-commit-msg\",\n \t\t git_path(\"MERGE_MSG\"), \"merge\", NULL, NULL);\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 8c6b811..3008e4e 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -645,6 +645,12 @@ test_debug 'git log --graph --decorate --oneline --all'\n \n cat >editor <<\\EOF\n #!/bin/sh\n+# Add a new message string that was not in the template\n+(\n+\techo \"Merge work done on the side branch c1\"\n+\techo\n+\tcat <\"$1\"\n+) >\"$1.tmp\" && mv \"$1.tmp\" \"$1\"\n # strip comments and blank lines from end of message\n sed -e '/^#/d' < \"$1\" | sed -e :a -e '/^\\n*$/{$d;N;ba' -e '}' > expected\n EOF\n@@ -654,7 +660,9 @@ test_expect_success 'merge --no-ff --edit' '\n \tgit reset --hard c0 &&\n \tEDITOR=./editor git merge --no-ff --edit c1 &&\n \tverify_parents $c0 $c1 &&\n-\tgit cat-file commit HEAD | sed \"1,/^$/d\" > actual &&\n+\tgit cat-file commit HEAD >raw &&\n+\tgrep \"work done on the side branch\" raw &&\n+\tsed \"1,/^$/d\" >actual raw &&\n \ttest_cmp actual expected\n '\n \n"},{"id":"177336","messageId":"CAG+J_Dz37etot0nNkq+1gTUy8R0vVJpsRQuvwrTSczXRWy7mkA@mail.gmail.com","threadId":"28635","inReplyTo":"7vd3e4k162.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] Teach merge the '[-e|--edit]' option","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-10-10T22:53:16Z","receivedAt":"2011-10-10T22:53:16Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, Oct 10, 2011 at 2:05 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jay Soffian <jaysoffian@gmail.com> writes:\n>> +static void prepare_to_commit(void)\n>> +{\n>> +     struct strbuf msg = STRBUF_INIT;\n>> +     strbuf_addbuf(&msg, &merge_msg);\n>> +     strbuf_addch(&msg, '\\n');\n>> +     write_merge_msg(&msg);\n>>       run_hook(get_index_file(), \"prepare-commit-msg\",\n>>                git_path(\"MERGE_MSG\"), \"merge\", NULL, NULL);\n>> -     read_merge_msg();\n>> +     if (option_edit) {\n>> +             if (launch_editor(git_path(\"MERGE_MSG\"), NULL, NULL))\n>> +                     abort_commit(NULL);\n>> +     }\n>> +     read_merge_msg(&msg);\n>> +     stripspace(&msg, option_edit);\n>> +     if (!msg.len)\n>> +             abort_commit(_(\"Empty commit message.\"));\n>> +     strbuf_release(&merge_msg);\n>> +     strbuf_addbuf(&merge_msg, &msg);\n>> +     strbuf_release(&msg);\n>>  }\n>\n> An abstraction very nicely done.\n\n<blush> :-)\n\n> I am not sure about the '\\n' you unconditionally added at the end of the\n> existing message.\n\nRight, the old code does that when the merge fails, counting on (I\nthink) git-commit to then take care of any extra newlines. My\nreasoning was tack it on before running prepare-commit-msg, then run\nstripspace() after the hook and and editor, which will take care of\nany excess newlines. I guess this would be a regression if someone's\nprepare-commit-msg hook blindly appends to the commit message.\n\n> I think running stripspace(&msg, option_edit) is a good change, even\n> though some people might feel it is a regression. \"git commit\" also cleans\n> up the whitespace cruft left by prepare-commit-message hook when the\n> editor is not in use, and this change makes it consistent.\n\nCorrect.\n\n>> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\n>> index 87aac835a1..8c6b811718 100755\n>> --- a/t/t7600-merge.sh\n>> +++ b/t/t7600-merge.sh\n>> @@ -643,4 +643,19 @@ test_expect_success 'amending no-ff merge commit' '\n>>\n>>  test_debug 'git log --graph --decorate --oneline --all'\n>>\n>> +cat >editor <<\\EOF\n>> +#!/bin/sh\n>> +# strip comments and blank lines from end of message\n>> +sed -e '/^#/d' < \"$1\" | sed -e :a -e '/^\\n*$/{$d;N;ba' -e '}' > expected\n>> +EOF\n>> +chmod 755 editor\n>\n> I am not sure about this one. Wouldn't this want to be editing the given\n> file to make sure that the edited content appear in the result, not just\n> testing the additional stripspace() call you added in the codepath?\n\nYep.\n\nI added this test under the previous iteration of the patch when I was\nconcerned that commit message make it through the external commit code\npath correctly. It doesn't really make sense with this iteration now\nthat I think about it. The part about stripping comments and newlines\nis no longer needed.\n\n>> +test_expect_success 'merge --no-ff --edit' '\n>> +     git reset --hard c0 &&\n>> +     EDITOR=./editor git merge --no-ff --edit c1 &&\n>> +     verify_parents $c0 $c1 &&\n>> +     git cat-file commit HEAD | sed \"1,/^$/d\" > actual &&\n>> +     test_cmp actual expected\n>> +'\n>> +\n>>  test_done\n>\n> So perhaps this one on top? I am just suspecting that your additional '\\n'\n> is to make sure we do not write out a file with an incomplete line with\n> this patch, but that change is not explained in your commit log message,\n> so I am not sure.\n\nI assumed the '\\n' was needed as it's added (unconditionally) before\nwriting MERGE_MSG when the merge fails. I didn't notice that when I\nadded the prepare-commit-msg hook support to merge.c (65969d43d1).\n\n>  builtin/merge.c  |    3 ++-\n>  t/t7600-merge.sh |   10 +++++++++-\n>  2 files changed, 11 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index a8dbf4a..09ffc07 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -867,7 +867,8 @@ static void prepare_to_commit(void)\n>  {\n>        struct strbuf msg = STRBUF_INIT;\n>        strbuf_addbuf(&msg, &merge_msg);\n> -       strbuf_addch(&msg, '\\n');\n> +       if (msg.len && msg.buf[msg.len-1] != '\\n')\n> +               strbuf_addch(&msg, '\\n');\n>        write_merge_msg(&msg);\n>        run_hook(get_index_file(), \"prepare-commit-msg\",\n>                 git_path(\"MERGE_MSG\"), \"merge\", NULL, NULL);\n\nI'm guessing the '\\n' is always needed (per above), but I'm not sure.\n\n> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\n> index 8c6b811..3008e4e 100755\n> --- a/t/t7600-merge.sh\n> +++ b/t/t7600-merge.sh\n> @@ -645,6 +645,12 @@ test_debug 'git log --graph --decorate --oneline --all'\n>\n>  cat >editor <<\\EOF\n>  #!/bin/sh\n> +# Add a new message string that was not in the template\n> +(\n> +       echo \"Merge work done on the side branch c1\"\n> +       echo\n> +       cat <\"$1\"\n> +) >\"$1.tmp\" && mv \"$1.tmp\" \"$1\"\n>  # strip comments and blank lines from end of message\n>  sed -e '/^#/d' < \"$1\" | sed -e :a -e '/^\\n*$/{$d;N;ba' -e '}' > expected\n>  EOF\n> @@ -654,7 +660,9 @@ test_expect_success 'merge --no-ff --edit' '\n>        git reset --hard c0 &&\n>        EDITOR=./editor git merge --no-ff --edit c1 &&\n>        verify_parents $c0 $c1 &&\n> -       git cat-file commit HEAD | sed \"1,/^$/d\" > actual &&\n> +       git cat-file commit HEAD >raw &&\n> +       grep \"work done on the side branch\" raw &&\n> +       sed \"1,/^$/d\" >actual raw &&\n>        test_cmp actual expected\n>  '\n\nOkay. A test that the merge is aborted if the message is empty would\nalso be good.\n\nI'll try to find time to send another iteration with better tests. May\nnot be till next week though.\n\nj.\n"},{"id":"177348","messageId":"7v1uukieh2.fsf@alter.siamese.dyndns.org","threadId":"28635","inReplyTo":"CAG+J_Dz37etot0nNkq+1gTUy8R0vVJpsRQuvwrTSczXRWy7mkA@mail.gmail.com","subject":"Re: [PATCH v3] Teach merge the '[-e|--edit]' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-11T00:00:57Z","receivedAt":"2011-10-11T00:00:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian <jaysoffian@gmail.com> writes:\n\n>> I am not sure about the '\\n' you unconditionally added at the end of the\n>> existing message.\n>\n> Right, the old code does that when the merge fails, counting on (I\n> think) git-commit to then take care of any extra newlines.\n\nAhh, that explains it. I was originally about to suggest running the\nstripspace() only when we run the editor, and saw a failure from an\nunrelated test and realized that running stripspace() on the result of\nprepare-commit-msg is the right thing to do after all, because that is\nwhat is done by \"git commit\". Yes, the current code does rely on the\nstripspace to remove it, so there is no need to make it conditional.\n\nSo if we drop the \"conditionally add '\\n'\" part in builtin/merge.c from my\n\"how about this on top\" patch and we should be ready to go, right?\n\nThanks.\n"},{"id":"177350","messageId":"CAG+J_Dz4x7CmHKXx-9p-ZxmiuFyE2v3TFwkXfYgyc-p37ONinQ@mail.gmail.com","threadId":"28635","inReplyTo":"7v1uukieh2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] Teach merge the '[-e|--edit]' option","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-10-11T00:09:53Z","receivedAt":"2011-10-11T00:09:53Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, Oct 10, 2011 at 5:00 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> So if we drop the \"conditionally add '\\n'\" part in builtin/merge.c from my\n> \"how about this on top\" patch and we should be ready to go, right?\n\nYes. I can send a followup patch adding an additional test case next\nweek (for the case where the editor zeros out the message).\n\nThanks Junio!\n\nj.\n"},{"id":"177368","messageId":"alpine.DEB.2.00.1110111454490.8685@ds9.cixit.se","threadId":"28635","inReplyTo":"1318099192-60860-1-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH v3] Teach merge the '[-e|--edit]' option","fromName":"Peter Krefting","fromEmail":"peter@softwolves.pp.se","sentAt":"2011-10-11T13:57:47Z","receivedAt":"2011-10-11T13:57:47Z","isPatch":true,"sender":{"key":"peter@softwolves.pp.se","avatar":"https://avatars.githubusercontent.com/u/990764?v=4"},"body":"Jay Soffian:\n\n> +--edit::\n> +-e::\n> ++\n> +\tInvoke editor before committing successful merge to further\n> +\tedit the default merge message.\n\nI have a feature request, and that is to also add a configuration option to \nmake this the default behaviour.\n\n-- \n\\\\// Peter - http://www.softwolves.pp.se/\n"}]}