{"thread":{"id":"28623","subject":"[PATCH] Teach merge the '[-e|--edit]' option","startedAt":"2011-10-07T15:29:07Z","lastAt":"2011-10-10T15:23:18Z","messageCount":15,"participants":["Jay Soffian","Junio C Hamano","Jakub Narebski","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"177156","messageId":"1318001347-11347-1-git-send-email-jaysoffian@gmail.com","threadId":"28623","inReplyTo":null,"subject":"[PATCH] Teach merge the '[-e|--edit]' option","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-10-07T15:29:07Z","receivedAt":"2011-10-07T15:29:07Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"Implement \"git merge [-e|--edit]\" as \"git merge --no-commit && git commit\"\nas a convenience for the user.\n\nSigned-off-by: Jay Soffian <jaysoffian@gmail.com>\n---\n Documentation/merge-options.txt |    6 ++++++\n builtin/merge.c                 |   14 ++++++++++++++\n t/t7600-merge.sh                |   15 +++++++++++++++\n 3 files changed, 35 insertions(+), 0 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..815e151487 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -46,6 +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 option_edit = 0;\n static int fast_forward_only;\n static int allow_trivial = 1, have_message;\n static struct strbuf merge_msg;\n@@ -190,6 +191,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@@ -1092,6 +1095,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\toption_commit = 0;\n \t}\n \n+\t/* if not committing, edit is nonsensical */\n+\tif (!option_commit)\n+\t\toption_edit = 0;\n+\t/* if editing, invoke 'git commit -e' after successful merge */\n+\tif (option_edit)\n+\t\toption_commit = 0;\n+\n \tif (!allow_fast_forward && fast_forward_only)\n \t\tdie(_(\"You cannot combine --no-ff with --ff-only.\"));\n \n@@ -1447,6 +1457,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (merge_was_ok) {\n+\t\tif (option_edit) {\n+\t\t\tconst char *args[] = {\"commit\", \"-e\", NULL};\n+\t\t\treturn run_command_v_opt(args, RUN_GIT_CMD);\n+\t\t}\n \t\tfprintf(stderr, _(\"Automatic merge went well; \"\n \t\t\t\"stopped before committing as requested\\n\"));\n \t\treturn 0;\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.g00fdf\n"},{"id":"177165","messageId":"7vk48gwvyd.fsf@alter.siamese.dyndns.org","threadId":"28623","inReplyTo":"1318001347-11347-1-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH] Teach merge the '[-e|--edit]' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-07T17:30:34Z","receivedAt":"2011-10-07T17:30:34Z","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> Implement \"git merge [-e|--edit]\" as \"git merge --no-commit && git commit\"\n> as a convenience for the user.\n>\n> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>\n> ---\n> ...\n> @@ -1447,6 +1457,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t}\n>  \n>  \tif (merge_was_ok) {\n> +\t\tif (option_edit) {\n> +\t\t\tconst char *args[] = {\"commit\", \"-e\", NULL};\n> +\t\t\treturn run_command_v_opt(args, RUN_GIT_CMD);\n> +\t\t}\n>  \t\tfprintf(stderr, _(\"Automatic merge went well; \"\n>  \t\t\t\"stopped before committing as requested\\n\"));\n>  \t\treturn 0;\n\n\nI wanted to like this approach, thinking this approach might be safer and\nwith the least chance of breaking other codepaths, but this feels like an\nugly hack.\n\nAre we still honoring all the hooks \"git merge\" honors?  More importantly,\nisn't this make it impossible for future maintainers of this command to\nenhance the command by adding other hooks after the commit is made?\n\nIf we wanted to do this properly, we should update builtin/merge.c to call\nlaunch_editor() before it runs commit_tree(), in a way similar to how\nprepare_to_commit() in builtin/commit.c does so when e.g. \"commit -m foo -e\"\nis run. An editmsg is prepared (you already have it in MERGE_MSG), the\neditor is allowed to update it, and then the original code before such a\npatch will run using the updated contents of MERGE_MSG. That way, the _only_\nchange in behaviour when \"-e\" is used is to let the user update the message\nused in the commit log, and everything else would run exactly the same way\nas if no \"-e\" was given, including the invocation of hooks.\n"},{"id":"177168","messageId":"CAG+J_Dz7-tTdgT=cqoKhK+fAhmESLnp93yHyxOF_NOY5Wx01+w@mail.gmail.com","threadId":"28623","inReplyTo":"7vk48gwvyd.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach merge the '[-e|--edit]' option","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-10-07T18:01:00Z","receivedAt":"2011-10-07T18:01:00Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Fri, Oct 7, 2011 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jay Soffian <jaysoffian@gmail.com> writes:\n>\n>> Implement \"git merge [-e|--edit]\" as \"git merge --no-commit && git commit\"\n>> as a convenience for the user.\n>>\n>> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>\n>> ---\n>> ...\n>> @@ -1447,6 +1457,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>>       }\n>>\n>>       if (merge_was_ok) {\n>> +             if (option_edit) {\n>> +                     const char *args[] = {\"commit\", \"-e\", NULL};\n>> +                     return run_command_v_opt(args, RUN_GIT_CMD);\n>> +             }\n>>               fprintf(stderr, _(\"Automatic merge went well; \"\n>>                       \"stopped before committing as requested\\n\"));\n>>               return 0;\n>\n>\n> I wanted to like this approach, thinking this approach might be safer and\n> with the least chance of breaking other codepaths, but this feels like an\n> ugly hack.\n>\n> Are we still honoring all the hooks \"git merge\" honors?  More importantly,\n> isn't this make it impossible for future maintainers of this command to\n> enhance the command by adding other hooks after the commit is made?\n\nGit is already inconsistent with respect to which hooks are called\nwhen. Shouldn't post-merge be called on a merge commit regardless of\nwhether you use --no-commit or not? Well, it isn't, it's only called\nwhen merge performs the commit internally. The post-merge hook was\nprobably a mistake -- git calls the post-commit hook passing the\ncontext as an argument, so probably merge should just call post-commit\n\"merge\". But that ship has sailed.\n\nSee also 65969d43d1 (merge: honor prepare-commit-msg hook, 2011-02-14).\n\n> If we wanted to do this properly, we should update builtin/merge.c to call\n> launch_editor() before it runs commit_tree(), in a way similar to how\n> prepare_to_commit() in builtin/commit.c does so when e.g. \"commit -m foo -e\"\n> is run. An editmsg is prepared (you already have it in MERGE_MSG), the\n> editor is allowed to update it, and then the original code before such a\n> patch will run using the updated contents of MERGE_MSG. That way, the _only_\n> change in behaviour when \"-e\" is used is to let the user update the message\n> used in the commit log, and everything else would run exactly the same way\n> as if no \"-e\" was given, including the invocation of hooks.\n\nI find git very difficult to reason about (and inconsistent in its\nbehavior) due to piecemeal hoisting of some functionality into\nporcelain commands (another example, revert.c building in the\nrecursive merge strategy but not any others).\n\nI actually think a better choice would be to remove commit_tree() from\nmerge and always have it run commit externally. I'm not seriously\nsuggesting that be done, but it would make git more consistent. But\nI'm not going to send in a patch which makes the situation worse.\n\nj.\n"},{"id":"177173","messageId":"7vobxsvd69.fsf@alter.siamese.dyndns.org","threadId":"28623","inReplyTo":"CAG+J_Dz7-tTdgT=cqoKhK+fAhmESLnp93yHyxOF_NOY5Wx01+w@mail.gmail.com","subject":"Re: [PATCH] Teach merge the '[-e|--edit]' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-07T19:01:34Z","receivedAt":"2011-10-07T19:01:34Z","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 actually think a better choice would be to remove commit_tree() from\n> merge and always have it run commit externally. I'm not seriously\n> suggesting that be done, but it would make git more consistent. But\n> I'm not going to send in a patch which makes the situation worse.\n\nThink about it. What I suggested does no way make the situation\nworse. Your patch _does_ make it worse by changing the hook behaviour\nbetween \"merge -m 'foo'\" vs \"merge -m 'foo' -e\".\n"},{"id":"177174","messageId":"CAG+J_DxrQCS8zn5KJ8HnpqShVbMw=zCbqDVa=w08EEibw=tsAA@mail.gmail.com","threadId":"28623","inReplyTo":"CAG+J_Dz7-tTdgT=cqoKhK+fAhmESLnp93yHyxOF_NOY5Wx01+w@mail.gmail.com","subject":"Re: [PATCH] Teach merge the '[-e|--edit]' option","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-10-07T19:07:42Z","receivedAt":"2011-10-07T19:07:42Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Fri, Oct 7, 2011 at 2:01 PM, Jay Soffian <jaysoffian@gmail.com> wrote:\n> I actually think a better choice would be to remove commit_tree() from\n> merge and always have it run commit externally. I'm not seriously\n> suggesting that be done, but it would make git more consistent. But\n> I'm not going to send in a patch which makes the situation worse.\n\nThe other inconsistencies I'm aware of between \"merge --no-commit &&\ncommit\" vs \"merge\" on a clean merge are:\n\n* reflog\n  - merge uses either \"Merge made by the '...' strategy.\" OR \"In-index merge\"\n  - commit uses \"commit (merge) <subject>\"\n\n* hooks\n  - merge calls\n    1) \"prepare-commit-msg MERGE_MSG merge\"\n    2) \"post-merge [0|1]\" where [0|1] indicates a squash or not.\n  - commit calls\n    1) \"pre-commit\"\n    2) \"prepare-commit-msg COMMIT_EDITMSG merge\"\n    3) \"commit-msg COMMIT_EDITMSG\"\n    4) \"post-commit\"\n\n* gc\n  - merge calls \"git gc --auto\" after a successful merge unless\n--squash was used\n  - commit does not call \"git gc --auto\"\n\n* diffstat: merge shows it, commit does not\n\nj.\n"},{"id":"177175","messageId":"CAG+J_Dzr188_sLCv+3BXP5M9d2by1VNiOGMcpewi4S4GMnOy2Q@mail.gmail.com","threadId":"28623","inReplyTo":"7vobxsvd69.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach merge the '[-e|--edit]' option","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-10-07T19:22:22Z","receivedAt":"2011-10-07T19:22:22Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Fri, Oct 7, 2011 at 3:01 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Think about it. What I suggested does no way make the situation\n> worse. Your patch _does_ make it worse by changing the hook behaviour\n> between \"merge -m 'foo'\" vs \"merge -m 'foo' -e\"\n\nI think it's arguable how -e should behave. With -e opening my editor,\nnow I really feel like I'm making a commit and would be surprised by\nnot having the various commit hooks run.\n\nj.\n"},{"id":"177176","messageId":"7vd3e8vbck.fsf@alter.siamese.dyndns.org","threadId":"28623","inReplyTo":"CAG+J_DxrQCS8zn5KJ8HnpqShVbMw=zCbqDVa=w08EEibw=tsAA@mail.gmail.com","subject":"Re: [PATCH] Teach merge the '[-e|--edit]' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-07T19:40:59Z","receivedAt":"2011-10-07T19:40:59Z","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> The other inconsistencies I'm aware of between \"merge --no-commit &&\n> commit\" vs \"merge\" on a clean merge are:\n\nPerhaps you would want to add these to a list of todo items when gitwiki\ncomes back.\n"},{"id":"177190","messageId":"1318023997-54810-1-git-send-email-jaysoffian@gmail.com","threadId":"28623","inReplyTo":"7vk48gwvyd.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] Teach merge the '[-e|--edit]' option","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-10-07T21:46:37Z","receivedAt":"2011-10-07T21:46:37Z","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\nSigned-off-by: Jay Soffian <jaysoffian@gmail.com>\n---\nOn Fri, Oct 7, 2011 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> If we wanted to do this properly, we should update builtin/merge.c to call\n> launch_editor() before it runs commit_tree(), in a way similar to how\n\nI disagree that this is the proper way to do it. --edit is a new option, there's\nno obviously \"correct\" behavior. You think 'merge --edit' should behave just\nlike 'merge', I think 'merge --edit' should behave like 'merge --no-commit &&\ncommit'.\n\nThe commit performed internally by git-merge is already wildly inconsistent with\ngit-commit. If anything, --edit is a perfect excuse to say \"we're trying to make\ngit-merge perform commits more consistently with git-commit, so we've\nimplemented 'merge --edit' in terms of git-commit.\"\n\nI didn't bother with the commit status, it's more code than I wanted\nto deal with duplicating/refactoring from commit.c.\n\n Documentation/merge-options.txt |    6 ++\n builtin/merge.c                 |  108 +++++++++++++++++++++++++--------------\n t/t7600-merge.sh                |   15 +++++\n 3 files changed, 91 insertions(+), 38 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..0dee53b7e4 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -46,6 +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 option_edit = 0;\n static int fast_forward_only;\n static int allow_trivial = 1, have_message;\n static struct strbuf merge_msg;\n@@ -190,6 +191,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 +845,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();\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 +906,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 +934,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 +1042,36 @@ static int setup_with_upstream(const char ***argv)\n \treturn i;\n }\n \n+static void write_merge_state()\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 +1475,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.g3a3ce\n"},{"id":"177193","messageId":"7vfwj4tplw.fsf@alter.siamese.dyndns.org","threadId":"28623","inReplyTo":"1318023997-54810-1-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH v2] Teach merge the '[-e|--edit]' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-07T22:15:55Z","receivedAt":"2011-10-07T22:15:55Z","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> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>\n> ---\n> On Fri, Oct 7, 2011 at 1:30 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> If we wanted to do this properly, we should update builtin/merge.c to call\n>> launch_editor() before it runs commit_tree(), in a way similar to how\n>\n> I disagree that this is the proper way to do it. --edit is a new option, there's\n> no obviously \"correct\" behavior. You think 'merge --edit' should behave just\n> like 'merge', I think 'merge --edit' should behave like 'merge --no-commit &&\n> commit'.\n>\n> The commit performed internally by git-merge is already wildly inconsistent with\n> git-commit.\n\nThink and look forward.\n\nYou are complaining that the \"commit\" does not know enough to behave as if\nit were a part of the merge command workflow if you split a usual merge\ninto two steps \"merge --no-commit; commit\".\n\nHow would you make it better? Would you strip all the things usual \"merge\"\ndoes, so that it would work identically to the split one, losing some hook\nsupport and such, or would you rather make the split case work similar to\nthe usual merge?\n\nI'd say between \"merge\" and \"merge --no-commit ; commit\", the latter is\nwhat needs to be fixed. Viewed that way, why would you even consider\nmaking the new option behave similar to the _wrong_ one?\n\n> I didn't bother with the commit status, it's more code than I wanted\n> to deal with duplicating/refactoring from commit.c.\n\nWhat do you mean by \"commit status\"? If you mean this patch is incomplete,\nit would have been nicer if it were labeled with [PATCH/RFC].\n\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index ee56974371..0dee53b7e4 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -46,6 +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 option_edit = 0;\n\nNo need to move this into .data segment when it can be in .bss\nsegment. Drop the unnecessary \" = 0\" before \";\".\n\n> @@ -842,30 +845,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();\n\ns/()/(void)/;\n\nThanks.\n"},{"id":"177240","messageId":"CAG+J_Dzrk5x0+JRC8EbrAxjZE+hD+-5mp+H=F=M8Su2WosPfmg@mail.gmail.com","threadId":"28623","inReplyTo":"7vfwj4tplw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Teach merge the '[-e|--edit]' option","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-10-08T18:11:16Z","receivedAt":"2011-10-08T18:11:16Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Fri, Oct 7, 2011 at 6:15 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Think and look forward.\n>\n> You are complaining that the \"commit\" does not know enough to behave as if\n> it were a part of the merge command workflow if you split a usual merge\n> into two steps \"merge --no-commit; commit\".\n\nNo Junio, you have my argument completely reversed.\n\nI am complaining that git-merge implements commits internally, which\ngives it unique behavior from git-{commit/cherry-pick/revert} (the\nlatter two of which just run external git-commit). I'm saying merge is\nfundamentally broken to do it this way. And maybe that's something\nthat should be fixed in 2.0 -- that git-merge should just call out to\ngit-commit, just like cherry-pick/revert do.\n\nIn case that's not clear: I think that git-merge should eventually\nbehave identically to \"merge --no-commit; commit\".\n\n> How would you make it better? Would you strip all the things usual \"merge\"\n> does, so that it would work identically to the split one,\n\nYes.\n\n> losing some hook support and such.\n\nYes, I would lose the post-merge hook and such.\n\n>, or would you rather make the split case work similar to the usual merge?\n\nNo, I would not do that.\n\nBTW, the same arguments apply to git-am, which uses git-commit-tree,\nand so implements its own set of hooks.\n\n> I'd say between \"merge\" and \"merge --no-commit ; commit\", the latter is\n> what needs to be fixed. Viewed that way, why would you even consider\n> making the new option behave similar to the _wrong_ one?\n\nStrongly disagree. I think it would make much more sense for all\ncommits to flow through git-commit, which would ensure consistent\nbehavior. I think we've got a mishmash of hooks which evolved over\ntime.\n\n>> I didn't bother with the commit status, it's more code than I wanted\n>> to deal with duplicating/refactoring from commit.c.\n>\n> What do you mean by \"commit status\"? If you mean this patch is incomplete,\n> it would have been nicer if it were labeled with [PATCH/RFC].\n\nNo, I meant that git-commit includes status information about the\ncommit itself as comments in the commit message (git config\ncommit.status), and I didn't implement that. I don't think that makes\nthis patch incomplete however, that could be added by a later patch.\n\nI'll send another iteration with your comments below addressed.\n\nj.\n\n>> diff --git a/builtin/merge.c b/builtin/merge.c\n>> index ee56974371..0dee53b7e4 100644\n>> --- a/builtin/merge.c\n>> +++ b/builtin/merge.c\n>> @@ -46,6 +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 option_edit = 0;\n>\n> No need to move this into .data segment when it can be in .bss\n> segment. Drop the unnecessary \" = 0\" before \";\".\n>\n>> @@ -842,30 +845,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>>       int fd = open(git_path(\"MERGE_MSG\"), O_WRONLY | O_CREAT, 0666);\n>>       if (fd < 0)\n>>               die_errno(_(\"Could not open '%s' for writing\"),\n>>                         git_path(\"MERGE_MSG\"));\n>> -     if (write_in_full(fd, merge_msg.buf, merge_msg.len) != merge_msg.len)\n>> +     if (write_in_full(fd, msg->buf, msg->len) != msg->len)\n>>               die_errno(_(\"Could not write to '%s'\"), git_path(\"MERGE_MSG\"));\n>>       close(fd);\n>>  }\n>>\n>> -static void read_merge_msg(void)\n>> +static void read_merge_msg(struct strbuf *msg)\n>>  {\n>> -     strbuf_reset(&merge_msg);\n>> -     if (strbuf_read_file(&merge_msg, git_path(\"MERGE_MSG\"), 0) < 0)\n>> +     strbuf_reset(msg);\n>> +     if (strbuf_read_file(msg, git_path(\"MERGE_MSG\"), 0) < 0)\n>>               die_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();\n>\n> s/()/(void)/;\n>\n> Thanks.\n>\n>\n"},{"id":"177291","messageId":"7v8votpx4n.fsf@alter.siamese.dyndns.org","threadId":"28623","inReplyTo":"CAG+J_Dzrk5x0+JRC8EbrAxjZE+hD+-5mp+H=F=M8Su2WosPfmg@mail.gmail.com","subject":"Re: [PATCH v2] Teach merge the '[-e|--edit]' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-09T23:23:52Z","receivedAt":"2011-10-09T23:23:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"[Adding Ram to the Cc: list as this topic has much to do with resuming a\nsequencer-driven workflow, and dropping Todd who does not seem to have\nmuch to say on this topic]\n\nJay Soffian <jaysoffian@gmail.com> writes:\n\n> I am complaining that git-merge implements commits internally, which\n> gives it unique behavior from git-{commit/cherry-pick/revert} (the\n> latter two of which just run external git-commit). I'm saying merge is\n> fundamentally broken to do it this way. And maybe that's something\n> that should be fixed in 2.0 -- that git-merge should just call out to\n> git-commit, just like cherry-pick/revert do.\n\nThe purpose of the plumbing commands, e.g. commit-tree, is to implement a\nunit of logical step at the mechanical level well without enforcing policy\ndecisions that may not apply to all situations.\n\nOn the other hand, the purpose of the Porcelain commands is to support a\nconcrete workflow element. \"git commit\" should support what users want to\nhappen when advancing the history by one commit, \"git pull\" should support\nwhat users want to happen when integrating work done in another repository\nto the history you are currently growing, etc. It is where we should and\ndo allow users to implement their own policy with hooks and configuration\nvariables when they want to, and it is even fine if we implemented sane\ndefault policies with ways to override them (e.g. \"commit --allow-empty\",\n\"merge --no-ff\").\n\nSome Porcelain commands cannot complete their workflow element by\nthemselves in certain situations without getting help from users, and they\ngive control back to the user when they need such help. \"git rebase\", \"git\nam\", \"git merge\", etc. can and do stop and ask the user to help resolving\nconflicts.\n\nThe unfortunate historical accident that we may want to correct is that\nsome of these \"we stopped in the middle and asked the user to help before\ncontinuing\" situation were presented as if \"we stopped and aborted in the\nmiddle, leaving the user to fix up the mess\", which is a completely wrong\nmental model. \"Upon conflicts, 'git merge' stops in the middle, and you\ncomplete it with 'git commit'\" is a prime example of this. We even wrongly\nlabel such a situation as \"failed merge\". It is not failed---it merely is\nnot auto-completed and waiting to be completed with user's help.\n\nTo understand why it is a wrong mental model, you need to imagine a world\nwhere the logic to resolve conflicts in \"git merge\" is improved so that it\nneeds less help from the users. rerere.autoupdate is half-way there---the\nuser allows the merge machinery to take advantage of conflict resolutions\nthat the user has performed previously. Even though we currently do not\nlet \"git merge\" proceed to commit the result, it is entirely plausible to\ngo one step further and treat the resulting tree from applying the rerere\ninformation as the result of the automerge. When that happens, it is very\nnatural for the user to expect that the rest of what \"git merge\" does for\na clean automerge to be carried out. After all, from the end user's point\nof view, it _is_ a clean auto-merge. The only difference is how the user\nhelped the automerge machinery.\n\nThe root cause of the inconsistencies you are bringing up (which I agree\nare annoying and I further agree that it is a worthy thing to address) is\nthat even though we tell the users \"after helping the 'git merge', you\nconclude it with 'git commit'\", the concluding 'git commit' does _not_\nperform what the user configured 'git merge' to do before a merge is\nconcluded, unlike a cleanly resolved 'git merge'.\n\nThis is merely an unfortunate historical accident. Because \"git merge\" did\nnot have any user configurable policy decisions (read: hooks) when this\n\"conclude with commit\" was coded, \"conclude with commit\" was sufficient to\nemulate the case where the merge did not need any help from the user.\n\nBut it no longer is true with modern Git.\n\nWith more recent changes, e.g. the sequencer work and \"git cherry-pick\"\nthat takes multiple commits, \"conclude with commit\" is becoming less and\nless correct thing to say. The workflow elements these commands implement\ndo have \"create a commit\" as one essential part, but that is not the only\nthing they do. If anything, I think the right way forward is to update the\nUI with this rule for consistency:\n\n  Some tools can stop in the middle when they cannot automatically compute\n  the outcome, and give control back to the user asking for help. After\n  helping these tool, the way to resume what was being done is to invoke\n  the tool with the \"--continue\" option. All user level policy decisions\n  implemented by hooks and configurations the tool normally obey when it\n  does not need such help from the end user are obeyed when continuing.\n\nI wouldn't mind if that is \"invoke 'git continue' command\", even though I\nsuspect that may make the implementation slightly more complex (I haven't\nthought things through). \"git commit\" as a way to conclude a merge that\nwas stopped in the middle due to a conflict should be deprecated in the\nlonger term, like say in Git 2.0 someday.\n\n\n[Footnote]\n\n*1* By the way, \"git merge --no-commit\" is an oddball. It primarily is\nused when the user does _not_ want the resulting commit but wants to\nfurther modify the tree state (e.g. cherry picking a part of what was done\nin the side branch). At the philosophical level, the user should be using\nmerge machinery at the \"plumbing\" level (e.g. merge-recursive backend),\nbut the interface to invoke the plumbing level merge machinery is so\narcane (they are after all designed for scripts not for humans) that\nnobody does so in practice. And for that purpose, I think it is Ok for the\nuser to do anything after \"git merge --no-commit\" finishes (either leaving\nconflicts or leaving a cleanly merged state), including \"git commit\".\nBecause that \"git commit\" is very different from the \"conclude conflicted\nmerge with commit\" which is a poor substitute for \"git merge --continue\"\nin modern Git, I think it is perfectly fine and even preferable if it does\nnot obey any \"git merge\" semantics (i.e. user defined policy that pertains\nto \"merge\" operations).\n"},{"id":"177295","messageId":"7vr52lo1m3.fsf@alter.siamese.dyndns.org","threadId":"28623","inReplyTo":"7v8votpx4n.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Teach merge the '[-e|--edit]' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-10T05:29:56Z","receivedAt":"2011-10-10T05:29:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> To understand why it is a wrong mental model, you need to imagine a world\n> where the logic to resolve conflicts in \"git merge\" is improved so that it\n> needs less help from the users. rerere.autoupdate is half-way there---the\n> user allows the merge machinery to take advantage of conflict resolutions\n> that the user has performed previously. Even though we currently do not\n> let \"git merge\" proceed to commit the result, it is entirely plausible to\n> go one step further and treat the resulting tree from applying the rerere\n> information as the result of the automerge. When that happens, it is very\n> natural for the user to expect that the rest of what \"git merge\" does for\n> a clean automerge to be carried out. After all, from the end user's point\n> of view, it _is_ a clean auto-merge. The only difference is how the user\n> helped the automerge machinery.\n\nAddendum.\n\nI am not suggesting that we should change rerere.autoupdate to go all the\nway and record a merge commit by default automatically when rerere applies\ncleanly.\n\nI personally think that it is a sensible default to set rerere.autoupdate\nto false (or not to set the variable at all) to ensure that a merge that\nconflicts is always inspected by the end user, given that rerere is merely\na heuristic (even though it is a damn good one) and produces a surprising\nresult.\n\nBut that is a policy preference; some people want to trust rerere more\nthan I do and that is a valid choice for them to make. To support such a\npolicy preference, I am perfectly fine with introducing a third value to\nrerere.autoupdate in addition to yes/no to allow commands (e.g. \"merge\",\n\"am\", etc.) to continue when rerere resolved conflicts cleanly in a\nsituation where they would have stopped and asked user to help resolving.\n\nBy the way, on the other side of this same coin lies another use case\n(different from the one in the footnote in the previous message) for\n\"merge --no-commit\". When you know that a particular merge _will_ need\nsemantic adjustments, even if it were to textually merge cleanly, you\nwould want the command to ask you for help to come up with the final tree,\ninstead of trusting the clean automerge result. This often happens when\nthe topic branch you are about to merge has changed the semantics of an\nexisting function (e.g. adding a new parameter) while the branch you are\non has added new callsite to the function (or the other way around). In\nsuch a merge, you would need to adjust the new callsite that does not know\nabout the additional parameter to the new function signature.  For exactly\nthe same reason, it is not a kosher advice to give to users of modern Git\nto \"interfere with the merge with 'merge --no-commit', and then conclude\nwith 'commit'\", as 'commit' has less information than 'merge' itself what\n'merge' wants to do in addition to recording the result as a 'commit'.\n\nEither the 'commit' command needs to detect that it is conclusing the\nmerge and trigger the merge hooks the same way as 'merge' itself does,\n(which is a bad design, as 'commit' will need to know about the clean-up\noperations of all the other commands that may ask users to help and let\n'commit' conclude it), or the end user instruction needs to be updated so\nthat 'merge --continue' is used in such a situation to give 'merge' a\nchance to finish up. Again we could have \"git continue\" wrapper that knows\nhow to tell what operation was in progress and invokes \"merge --continue\"\nwhen it detects that it was a 'merge' that was in progress, but that is a\nmere fluff.\n"},{"id":"177302","messageId":"m3ehyl1g5v.fsf@localhost.localdomain","threadId":"28623","inReplyTo":"7vr52lo1m3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Teach merge the '[-e|--edit]' option","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-10-10T07:05:02Z","receivedAt":"2011-10-10T07:05:02Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> By the way, on the other side of this same coin lies another use case\n> (different from the one in the footnote in the previous message) for\n> \"merge --no-commit\". When you know that a particular merge _will_ need\n> semantic adjustments, even if it were to textually merge cleanly, you\n> would want the command to ask you for help to come up with the final tree,\n> instead of trusting the clean automerge result. This often happens when\n> the topic branch you are about to merge has changed the semantics of an\n> existing function (e.g. adding a new parameter) while the branch you are\n> on has added new callsite to the function (or the other way around). In\n> such a merge, you would need to adjust the new callsite that does not know\n> about the additional parameter to the new function signature.  For exactly\n> the same reason, it is not a kosher advice to give to users of modern Git\n> to \"interfere with the merge with 'merge --no-commit', and then conclude\n> with 'commit'\", as 'commit' has less information than 'merge' itself what\n> 'merge' wants to do in addition to recording the result as a 'commit'.\n\nYet another issue is if we should blindly trust automatic merge resolution.\nIt is considered a good practice by some to always check (e.g. by compiling\nand possibly also running tests) the result of merge, whether it required\nmerge conflict resolution or not.\n\nIIRC Linus lately said that making \"git merge\" automatically commit\nwas one of bad design decisions of git, for the above reason...\n\n-- \nJakub Narębski\n"},{"id":"177303","messageId":"vpqty7h2sla.fsf@bauges.imag.fr","threadId":"28623","inReplyTo":"m3ehyl1g5v.fsf@localhost.localdomain","subject":"Re: [PATCH v2] Teach merge the '[-e|--edit]' option","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-10-10T07:50:25Z","receivedAt":"2011-10-10T07:50:25Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> Yet another issue is if we should blindly trust automatic merge resolution.\n> It is considered a good practice by some to always check (e.g. by compiling\n> and possibly also running tests) the result of merge, whether it required\n> merge conflict resolution or not.\n\nI agree that trusting merge blindly is bad, but still, if there are no\nmerge conflicts, and if the merge is broken, I'd prefer commiting a\nfixup patch right after the merge than fixing it before committing.\nBecause if the merge needs a fix, it usually means something tricky that\ndeserves its own patch and commit message. At worse, one can still reset\n--merge HEAD^.\n\nOne other issue with not committing automatically is for beginners. I\nsee that all the time when the merge has conflicts. newbies fix the\nconflicts, and when they're done: \"fine, conflicts solved, let's\ncontinue hacking\" without committing. The resulting history is totally\nmessy because it mixes merges and actual edits. For these users, not\ncommitting automatically in the absence of conflict would make the\nsituation even worse.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"177316","messageId":"7vmxd8oopl.fsf@alter.siamese.dyndns.org","threadId":"28623","inReplyTo":"m3ehyl1g5v.fsf@localhost.localdomain","subject":"Re: [PATCH v2] Teach merge the '[-e|--edit]' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-10T15:23:18Z","receivedAt":"2011-10-10T15:23:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> Yet another issue is if we should blindly trust automatic merge resolution.\n> It is considered a good practice by some to always check (e.g. by compiling\n> and possibly also running tests) the result of merge, whether it required\n> merge conflict resolution or not.\n>\n> IIRC Linus lately said that making \"git merge\" automatically commit\n> was one of bad design decisions of git, for the above reason...\n\nI think your recalling this discussion\n\n    http://thread.gmane.org/gmane.linux.kernel/1191100/focus=181362\n\nWhile I agree with what Linus said in the message, I think you are not\nremembering the discussion correctly. It was about bad commit _message_,\nand an improvement is not to let users tweak a cleanly automerged result,\nbut is to allow users or force them to always write their own message,\nperhaps with \"merge --[no-]edit\", which is exactly the point of Jay's\npatch in this thread.\n"}]}