{"thread":{"id":"19541","subject":"[PATCH] refuse to merge during a merge","startedAt":"2009-05-27T21:04:10Z","lastAt":"2009-06-01T09:20:56Z","messageCount":11,"participants":["Clemens Buchacher","Constantine Plotnikov","John Tapsell","Jakub Narebski","Thomas Rast","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"114839","messageId":"20090527210410.GA14742@localhost","threadId":"19541","inReplyTo":null,"subject":"[PATCH] refuse to merge during a merge","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-05-27T21:04:10Z","receivedAt":"2009-05-27T21:04:10Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"The following is an easy mistake to make for users coming from version\ncontrol systems with an \"update and commit\"-style workflow.\n\n\t1. git merge\n\t2. resolve conflicts\n\t3. git pull, instead of commit\n\nThis overrides MERGE_HEAD, starting a new merge with dirty index. IOW,\nprobably not what the user intented. Instead, refuse to merge again if a\nmerge is in progress.\n\nReported-by: Dave Olszewski <cxreg@pobox.com>\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n\n builtin-merge.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex 0b58e5e..74a8c8f 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -836,7 +836,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tstruct commit_list **remotes = &remoteheads;\n \n \tsetup_work_tree();\n-\tif (read_cache_unmerged())\n+\tif (read_cache_unmerged() || file_exists(git_path(\"MERGE_HEAD\")))\n \t\tdie(\"You are in the middle of a conflicted merge.\");\n \n \t/*\n-- \n1.6.3.1.147.g637c3\n"},{"id":"114927","messageId":"85647ef50905280900v274d87ag8d364ea7bd9a97ee@mail.gmail.com","threadId":"19541","inReplyTo":"20090527210410.GA14742@localhost","subject":"Re: [PATCH] refuse to merge during a merge","fromName":"Constantine Plotnikov","fromEmail":"constantine.plotnikov@gmail.com","sentAt":"2009-05-28T16:00:53Z","receivedAt":"2009-05-28T16:00:53Z","isPatch":true,"sender":{"key":"constantine.plotnikov@gmail.com","avatar":null},"body":"MERGE_HEAD file could also happen in case of --no-commit option. In\nthat case there might be no conflict and the message would look\nconfusing to the user. I suggest to change a message to \"You are in\nthe middle of a uncommitted or conflicted merge.\" or something like\nit.\n\nRegards,\nConstantine\n\nOn Thu, May 28, 2009 at 1:04 AM, Clemens Buchacher <drizzd@aon.at> wrote:\n> The following is an easy mistake to make for users coming from version\n> control systems with an \"update and commit\"-style workflow.\n>\n>        1. git merge\n>        2. resolve conflicts\n>        3. git pull, instead of commit\n>\n> This overrides MERGE_HEAD, starting a new merge with dirty index. IOW,\n> probably not what the user intented. Instead, refuse to merge again if a\n> merge is in progress.\n>\n> Reported-by: Dave Olszewski <cxreg@pobox.com>\n> Signed-off-by: Clemens Buchacher <drizzd@aon.at>\n> ---\n>\n>  builtin-merge.c |    2 +-\n>  1 files changed, 1 insertions(+), 1 deletions(-)\n>\n> diff --git a/builtin-merge.c b/builtin-merge.c\n> index 0b58e5e..74a8c8f 100644\n> --- a/builtin-merge.c\n> +++ b/builtin-merge.c\n> @@ -836,7 +836,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>        struct commit_list **remotes = &remoteheads;\n>\n>        setup_work_tree();\n> -       if (read_cache_unmerged())\n> +       if (read_cache_unmerged() || file_exists(git_path(\"MERGE_HEAD\")))\n>                die(\"You are in the middle of a conflicted merge.\");\n>\n>        /*\n> --\n> 1.6.3.1.147.g637c3\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n>\n"},{"id":"114928","messageId":"43d8ce650905280912q71c749bn7146210a5838a453@mail.gmail.com","threadId":"19541","inReplyTo":"20090527210410.GA14742@localhost","subject":"Re: [PATCH] refuse to merge during a merge","fromName":"John Tapsell","fromEmail":"johnflux@gmail.com","sentAt":"2009-05-28T16:12:40Z","receivedAt":"2009-05-28T16:12:40Z","isPatch":true,"sender":{"key":"johnflux@gmail.com","avatar":"https://gravatar.com/avatar/25f70d4c0f96396b84a2e34bcd9bdc233462c7b4be29b5fdca8266fc53f30b0c?d=mp&s=160"},"body":"> +       if (read_cache_unmerged() || file_exists(git_path(\"MERGE_HEAD\")))\n>                die(\"You are in the middle of a conflicted merge.\");\n\nCould the error message also give possible solutions?   \"Commit the\ncurrent merge first with 'git commit', or discard the current merge\nattempt with 'git reset --hard'\" or something.  Or at least a pointer\nto where to read for more info.\n\nJohn\n"},{"id":"115078","messageId":"20090530083721.GA12963@localhost","threadId":"19541","inReplyTo":"43d8ce650905280912q71c749bn7146210a5838a453@mail.gmail.com","subject":"Re: [PATCH] refuse to merge during a merge","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-05-30T08:37:21Z","receivedAt":"2009-05-30T08:37:21Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Thu, May 28, 2009 at 05:12:40PM +0100, John Tapsell wrote:\n> > +       if (read_cache_unmerged() || file_exists(git_path(\"MERGE_HEAD\")))\n> >                die(\"You are in the middle of a conflicted merge.\");\n> \n> Could the error message also give possible solutions?   \"Commit the\n> current merge first with 'git commit', or discard the current merge\n> attempt with 'git reset --hard'\" or something.  Or at least a pointer\n> to where to read for more info.\n\nHow about this.\n\nfatal: You are in the middle of a [conflicted] merge. To complete the merge\n[resolve conflicts and] commit the changes. To abort, use \"git reset HEAD\".\n\nThe part about resolving changes is only displayed if there are unmerged\nentries. I intentionally left out --hard, because it potentially removes\nchanges unrelated to the merge (if the work tree was dirty prior to the\nmerge). The user will find out how to reset the work tree by reading the\ndocs.\n\nClemens\n"},{"id":"115079","messageId":"m34ov2c1wx.fsf@localhost.localdomain","threadId":"19541","inReplyTo":"20090530083721.GA12963@localhost","subject":"Re: [PATCH] refuse to merge during a merge","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-05-30T10:38:02Z","receivedAt":"2009-05-30T10:38:02Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Clemens Buchacher <drizzd@aon.at> writes:\n\n> On Thu, May 28, 2009 at 05:12:40PM +0100, John Tapsell wrote:\n> > > +       if (read_cache_unmerged() || file_exists(git_path(\"MERGE_HEAD\")))\n> > >                die(\"You are in the middle of a conflicted merge.\");\n> > \n> > Could the error message also give possible solutions?   \"Commit the\n> > current merge first with 'git commit', or discard the current merge\n> > attempt with 'git reset --hard'\" or something.  Or at least a pointer\n> > to where to read for more info.\n> \n> How about this.\n> \n> fatal: You are in the middle of a [conflicted] merge. To complete the merge\n> [resolve conflicts and] commit the changes. To abort, use \"git reset HEAD\".\n> \n> The part about resolving changes is only displayed if there are unmerged\n> entries. I intentionally left out --hard, because it potentially removes\n> changes unrelated to the merge (if the work tree was dirty prior to the\n> merge). The user will find out how to reset the work tree by reading the\n> docs.\n\nWhy not advertise new \"git reset --merge HEAD\" then?\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"115081","messageId":"200905301257.27630.trast@student.ethz.ch","threadId":"19541","inReplyTo":"m34ov2c1wx.fsf@localhost.localdomain","subject":"Re: [PATCH] refuse to merge during a merge","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2009-05-30T10:57:23Z","receivedAt":"2009-05-30T10:57:23Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jakub Narebski wrote:\n> Clemens Buchacher <drizzd@aon.at> writes:\n> > fatal: You are in the middle of a [conflicted] merge. To complete the merge\n> > [resolve conflicts and] commit the changes. To abort, use \"git reset HEAD\".\n> > \n> > The part about resolving changes is only displayed if there are unmerged\n> > entries. I intentionally left out --hard, because it potentially removes\n> > changes unrelated to the merge (if the work tree was dirty prior to the\n> > merge). The user will find out how to reset the work tree by reading the\n> > docs.\n> \n> Why not advertise new \"git reset --merge HEAD\" then?\n\nThat doesn't deal with conflicts at all.  It fills the rather\ndifferent case where you did a clean merge with some uncommitted\nchanges in the worktree, but then want to discard the merge again\nwithout losing the uncommitted changes.  In absence of the changes,\nyou would just use --hard, but here you want to move the branch tip\nwhile merging them over, similar to what 'git checkout -m' does for\nmoving HEAD.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"115129","messageId":"20090531104359.GA19094@localhost","threadId":"19541","inReplyTo":"20090530083721.GA12963@localhost","subject":"[PATCH] refuse to merge during a merge","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-05-31T10:43:59Z","receivedAt":"2009-05-31T10:43:59Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"The following is an easy mistake to make for users coming from version\ncontrol systems with an \"update and commit\"-style workflow.\n\n        1. git pull\n        2. resolve conflicts\n        3. git pull\n\nStep 3 overrides MERGE_HEAD, starting a new merge with dirty index.\nIOW, probably not what the user intented. Instead, refuse to merge\nagain if a merge is in progress and present the user with his options.\n\n\"git reset --hard\" is not suggested, because it potentially removes\nchanges unrelated to the merge (if the work tree was dirty prior to\nthe merge).\n\nReported-by: Dave Olszewski <cxreg@pobox.com>\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n\nOk, since I'm not seeing any more objections. Here's the code.\n\nClemens\n\n builtin-merge.c |   10 ++++++++--\n 1 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex 0b58e5e..8169ded 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -834,10 +834,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tstruct commit_list *common = NULL;\n \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n \tstruct commit_list **remotes = &remoteheads;\n+\tint unmerged;\n \n \tsetup_work_tree();\n-\tif (read_cache_unmerged())\n-\t\tdie(\"You are in the middle of a conflicted merge.\");\n+\tunmerged = read_cache_unmerged();\n+\tif (unmerged || file_exists(git_path(\"MERGE_HEAD\")))\n+\t\tdie(\"You are in the middle of a %smerge. To complete \"\n+\t\t\t\"the merge %scommit the changes. To abort, \"\n+\t\t\t\"use \\\"git reset HEAD\\\".\",\n+\t\t\tunmerged ? \"conflicted \" : \"\",\n+\t\t\tunmerged ? \"resolve conflicts and \" : \"\");\n \n \t/*\n \t * Check if we are _not_ on a detached HEAD, i.e. if there is a\n-- \n1.6.3.1.147.g637c3\n"},{"id":"115130","messageId":"43d8ce650905310405k4fa17240se7701e5fab2b22a7@mail.gmail.com","threadId":"19541","inReplyTo":"20090531104359.GA19094@localhost","subject":"Re: [PATCH] refuse to merge during a merge","fromName":"John Tapsell","fromEmail":"johnflux@gmail.com","sentAt":"2009-05-31T11:05:31Z","receivedAt":"2009-05-31T11:05:31Z","isPatch":true,"sender":{"key":"johnflux@gmail.com","avatar":"https://gravatar.com/avatar/25f70d4c0f96396b84a2e34bcd9bdc233462c7b4be29b5fdca8266fc53f30b0c?d=mp&s=160"},"body":"> \"git reset --hard\" is not suggested, because it potentially removes\n> changes unrelated to the merge (if the work tree was dirty prior to\n> the merge).\n\nSorry could you just clarify...  if the user does \"git reset HEAD\"\nwill that sometimes always or never fail?\n\nIf it sometimes or always fails, then doesn't it seem kinda confusing\nif the user is told to run that command, but then when they do they\nget an error?\n\nJohn\n"},{"id":"115133","messageId":"20090531140500.GA24862@localhost","threadId":"19541","inReplyTo":"43d8ce650905310405k4fa17240se7701e5fab2b22a7@mail.gmail.com","subject":"Re: [PATCH] refuse to merge during a merge","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-05-31T14:05:00Z","receivedAt":"2009-05-31T14:05:00Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Sun, May 31, 2009 at 12:05:31PM +0100, John Tapsell wrote:\n> > \"git reset --hard\" is not suggested, because it potentially removes\n> > changes unrelated to the merge (if the work tree was dirty prior to\n> > the merge).\n> \n> Sorry could you just clarify...  if the user does \"git reset HEAD\"\n> will that sometimes always or never fail?\n\nIt will never fail. It aborts the merge as suggested. But \"git reset --hard\nHEAD\" would also reset the work tree, so that \"any changes to tracked files\nin the working tree since <commit> are lost.\" This is generally desireable,\nsince an incomplete merge also leaves the auto-merged files in the work\ntree.\n\nBut if the user does not already know that, it's better to leave the user\nwondering how to clean a dirty work tree, than to suggest a potentially\nharmful operation.\n\nClemens\n"},{"id":"115159","messageId":"7vd49prrne.fsf@alter.siamese.dyndns.org","threadId":"19541","inReplyTo":"20090531104359.GA19094@localhost","subject":"Re: [PATCH] refuse to merge during a merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-05-31T19:36:37Z","receivedAt":"2009-05-31T19:36:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher <drizzd@aon.at> writes:\n\n> The following is an easy mistake to make for users coming from version\n> control systems with an \"update and commit\"-style workflow.\n>\n>         1. git pull\n>         2. resolve conflicts\n>         3. git pull\n>\n> Step 3 overrides MERGE_HEAD, starting a new merge with dirty index.\n\nI think the new condition that you added to stop the merge is more in line\nwith the original intent of the check.  We never wanted to check \"do we\nstill have unmerged entries?\" but wanted to see \"is another merge in\nprogress?\"; not checking MERGE_HEAD was a simple omission.\n\nBut I do not necessarily agree with the combined check nor with the new\nmessage.  I think it would be more sensible to split the codepath like\nthis:\n\n\tif (we see MERGE_HEAD)\n\t\tdie(\"You have not concluded your merge (MERGE_HEAD exists).\");\n\tif (the index is unmerged)\n\t\tdie(\"You are in the middle of a conflicted merge (index unmerged).\");\n\nThen in a _later_ patch, you could try to be more helpful by paying more\nattention to the context.  E.g.\n\n\tif (we see MERGE_HEAD) {\n        \tfigure out what was attempted by looking at\n                MERGE_MSG and other cues;\n\t\tdie(\"You have not concluded your merge with %s.\\n\"\n\t\t    \"Perhaps you would want 'git reset' to recover?\"\n                    that);\n\t}\n\tif (the index is unmerged)\n\t\tdie(\"You are in the middle of a conflicted merge\");\n\nThe point is that combining the checks makes it harder to later give more\nappropriate diagnosis and suggestion to the end user.\n\nFor example, \"git merge\" may learn \"git merge --abort\" like other commands\nthat have \"attempt, stop, let the user fix up to conclude\" modes of\noperations (i.e. rebase and am), and we may suggest to use that to recover\nin the message, instead of 'git reset'.  But that can only be used if we\nstopped because we saw MERGE_HEAD; you definitely do not want to suggest\n\"git merge --abort\" if the index is unmerged due to a conflicted rebase in\nprogress.\n\nNote that I am not suggesting you to blow this up to one large patch by\nadding fancier \"what were we doing\" logic; I am perfectly OK with the\nminimum \"detect MERGE_HEAD and refuse\".  I am only saying that I am\nunhappy with the way two different error conditions are conflated.\n\nPersonally, I'd suggest not to give \"you can do this to recover\" message.\n"},{"id":"115186","messageId":"20090601092056.GA6933@localhost","threadId":"19541","inReplyTo":"7vd49prrne.fsf@alter.siamese.dyndns.org","subject":"[PATCH v3] refuse to merge during a merge","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-06-01T09:20:56Z","receivedAt":"2009-06-01T09:20:56Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"The following is an easy mistake to make for users coming from version\ncontrol systems with an \"update and commit\"-style workflow.\n\n        1. git pull\n        2. resolve conflicts\n        3. git pull\n\nStep 3 overrides MERGE_HEAD, starting a new merge with dirty index.\nIOW, probably not what the user intended. Instead, refuse to merge\nagain if a merge is in progress.\n\nReported-by: Dave Olszewski <cxreg@pobox.com>\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n\nOn Sun, May 31, 2009 at 12:36:37PM -0700, Junio C Hamano wrote:\n> For example, \"git merge\" may learn \"git merge --abort\" like other commands\n> that have \"attempt, stop, let the user fix up to conclude\" modes of\n> operations (i.e. rebase and am), and we may suggest to use that to recover\n> in the message, instead of 'git reset'.  But that can only be used if we\n> stopped because we saw MERGE_HEAD; you definitely do not want to suggest\n> \"git merge --abort\" if the index is unmerged due to a conflicted rebase in\n> progress.\n\nIndeed. I wasn't thinking.\n\nClemens\n\n builtin-merge.c            |    5 ++++-\n t/t3030-merge-recursive.sh |    3 +++\n 2 files changed, 7 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex 0b58e5e..9e9bd52 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -836,8 +836,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tstruct commit_list **remotes = &remoteheads;\n \n \tsetup_work_tree();\n+\tif (file_exists(git_path(\"MERGE_HEAD\")))\n+\t\tdie(\"You have not concluded your merge. (MERGE_HEAD exists)\");\n \tif (read_cache_unmerged())\n-\t\tdie(\"You are in the middle of a conflicted merge.\");\n+\t\tdie(\"You are in the middle of a conflicted merge.\"\n+\t\t\t\t\" (index unmerged)\");\n \n \t/*\n \t * Check if we are _not_ on a detached HEAD, i.e. if there is a\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex 0de613d..9b3fa2b 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -276,6 +276,9 @@ test_expect_success 'fail if the index has unresolved entries' '\n \n \ttest_must_fail git merge \"$c5\" &&\n \ttest_must_fail git merge \"$c5\" 2> out &&\n+\tgrep \"You have not concluded your merge\" out &&\n+\trm -f .git/MERGE_HEAD &&\n+\ttest_must_fail git merge \"$c5\" 2> out &&\n \tgrep \"You are in the middle of a conflicted merge\" out\n \n '\n-- \n1.6.3.1.147.g637c3\n"}]}