{"thread":{"id":"47469","subject":"git merge commits staged files (when two trees are identical)","startedAt":"2017-12-20T11:50:28Z","lastAt":"2018-01-09T18:49:38Z","messageCount":15,"participants":["Andreas Krey","Elijah Newren","Junio C Hamano","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"335070","messageId":"20171220114310.GA2049@inner.h.apk.li","threadId":"47469","inReplyTo":null,"subject":"git merge commits staged files (when two trees are identical)","fromName":"Andreas Krey","fromEmail":"a.krey@gmx.de","sentAt":"2017-12-20T11:43:10Z","receivedAt":"2017-12-20T11:50:28Z","isPatch":false,"sender":{"key":"a.krey@gmx.de","avatar":"https://avatars.githubusercontent.com/u/37810?v=4"},"body":"Hi everybody,\n\nwe just stumbled over a situation in which a merge commits\nstaged changes into the merge commit. This happens when the\nmerged-in branch does have commits ('main') but has the same\ntree ('--allow-empty') as the merge base:\n\n    git init\n    echo eins >a\n    git add a\n    git commit -m initial\n    git branch sub\n    git commit -m main --allow-empty\n    git checkout sub\n    : two\n    echo zwei >>a\n    git add a\n    git commit -m underway\n    : three\n    echo drei >>a\n    git add a              # important\n    git status\n    git diff --cached\n    git merge master -m 'merge'\n    git status\n    git log --cc -1\n\nIf the change isn't staged (comment out the '# important' line)\nthe change survives as unstaged.\n\nThat is a bug?\n\n- Andreas\n\n-- \n\"Totally trivial. Famous last words.\"\nFrom: Linus Torvalds <torvalds@*.org>\nDate: Fri, 22 Jan 2010 07:29:21 -0800\n"},{"id":"335136","messageId":"CABPp-BGy3_RyVQfCm+9O_AAfKA0_CZ5ajJE7NuLbToERWyWmqQ@mail.gmail.com","threadId":"47469","inReplyTo":"20171220114310.GA2049@inner.h.apk.li","subject":"Re: git merge commits staged files (when two trees are identical)","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-12-21T18:50:31Z","receivedAt":"2017-12-21T18:50:37Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Dec 20, 2017 at 3:43 AM, Andreas Krey <a.krey@gmx.de> wrote:\n> Hi everybody,\n>\n> we just stumbled over a situation in which a merge commits\n> staged changes into the merge commit. This happens when the\n> merged-in branch does have commits ('main') but has the same\n> tree ('--allow-empty') as the merge base:\n>\n>     git init\n>     echo eins >a\n>     git add a\n>     git commit -m initial\n>     git branch sub\n>     git commit -m main --allow-empty\n>     git checkout sub\n>     : two\n>     echo zwei >>a\n>     git add a\n>     git commit -m underway\n>     : three\n>     echo drei >>a\n>     git add a              # important\n>     git status\n>     git diff --cached\n>     git merge master -m 'merge'\n>     git status\n>     git log --cc -1\n>\n> If the change isn't staged (comment out the '# important' line)\n> the change survives as unstaged.\n>\n> That is a bug?\n\nYes, it's definitely a bug; thanks for reporting it.  We have a\nspecific set of tests for this type of situation in t6044, which was\nintended to test all the merge strategies and ensure they didn't have\nthis class of bug.  It turns out that there is an additional codepath\nwithin one of the strategies that wasn't tested, and which you've\ntripped over.  I've got some patches to fix this up that I'll respond\nwith shortly.\n"},{"id":"335144","messageId":"20171221191907.4251-3-newren@gmail.com","threadId":"47469","inReplyTo":"20171221191907.4251-1-newren@gmail.com","subject":"[PATCH 3/3] merge-recursive: Avoid incorporating uncommitted changes in a merge","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-12-21T19:19:07Z","receivedAt":"2017-12-21T19:19:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"builtin/merge.c contains this important requirement for merge strategies:\n\t/*\n\t * At this point, we need a real merge.  No matter what strategy\n\t * we use, it would operate on the index, possibly affecting the\n\t * working tree, and when resolved cleanly, have the desired\n\t * tree in the index -- this means that the index must be in\n\t * sync with the head commit.  The strategies are responsible\n\t * to ensure this.\n\t */\n\nmerge-recursive does not do this check directly, instead it relies on\nunpack_trees() to do it.  However, merge_trees() has a special check for\nthe merge branch exactly matching the merge base; when it detects that\nsituation, it returns early without calling unpack_trees(), because it\nknows that the HEAD commit already has the correct result.  Unfortunately,\nit didn't check that the index matched HEAD, so after it returned, the\nouter logic ended up creating a merge commit that included something\nother than HEAD.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n merge-recursive.c                        | 7 +++++++\n t/t6044-merge-unrelated-index-changes.sh | 2 +-\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 2ecf495cc2..780f81a8bd 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1952,6 +1952,13 @@ int merge_trees(struct merge_options *o,\n \t}\n \n \tif (oid_eq(&common->object.oid, &merge->object.oid)) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\n+\t\tif (index_has_changes(&sb)) {\n+\t\t\terr(o, _(\"Dirty index: cannot merge (dirty: %s)\"),\n+\t\t\t    sb.buf);\n+\t\t\treturn 0;\n+\t\t}\n \t\toutput(o, 0, _(\"Already up to date!\"));\n \t\t*result = head;\n \t\treturn 1;\ndiff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\nindex 5e472be92b..23b86fb977 100755\n--- a/t/t6044-merge-unrelated-index-changes.sh\n+++ b/t/t6044-merge-unrelated-index-changes.sh\n@@ -112,7 +112,7 @@ test_expect_success 'recursive' '\n \ttest_must_fail git merge -s recursive C^0\n '\n \n-test_expect_failure 'recursive, when merge branch matches merge base' '\n+test_expect_success 'recursive, when merge branch matches merge base' '\n \tgit reset --hard &&\n \tgit checkout B^0 &&\n \n-- \n2.15.1.436.g63a861020b\n\n"},{"id":"335145","messageId":"20171221191907.4251-2-newren@gmail.com","threadId":"47469","inReplyTo":"20171221191907.4251-1-newren@gmail.com","subject":"[PATCH 2/3] move index_has_changes() from builtin/am.c to merge.c for reuse","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-12-21T19:19:06Z","receivedAt":"2017-12-21T19:20:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"index_has_changes() is a function we want to reuse outside of just am,\nmaking it also available for merge-recursive and merge-ort.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/am.c | 37 -------------------------------------\n cache.h      |  9 +++++++++\n merge.c      | 33 +++++++++++++++++++++++++++++++++\n 3 files changed, 42 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 3d98e52085..a02d5186cb 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1142,43 +1142,6 @@ static void refresh_and_write_cache(void)\n \t\tdie(_(\"unable to write index file\"));\n }\n \n-/**\n- * Returns 1 if the index differs from HEAD, 0 otherwise. When on an unborn\n- * branch, returns 1 if there are entries in the index, 0 otherwise. If an\n- * strbuf is provided, the space-separated list of files that differ will be\n- * appended to it.\n- */\n-static int index_has_changes(struct strbuf *sb)\n-{\n-\tstruct object_id head;\n-\tint i;\n-\n-\tif (!get_oid_tree(\"HEAD\", &head)) {\n-\t\tstruct diff_options opt;\n-\n-\t\tdiff_setup(&opt);\n-\t\topt.flags.exit_with_status = 1;\n-\t\tif (!sb)\n-\t\t\topt.flags.quick = 1;\n-\t\tdo_diff_cache(&head, &opt);\n-\t\tdiffcore_std(&opt);\n-\t\tfor (i = 0; sb && i < diff_queued_diff.nr; i++) {\n-\t\t\tif (i)\n-\t\t\t\tstrbuf_addch(sb, ' ');\n-\t\t\tstrbuf_addstr(sb, diff_queued_diff.queue[i]->two->path);\n-\t\t}\n-\t\tdiff_flush(&opt);\n-\t\treturn opt.flags.has_changes != 0;\n-\t} else {\n-\t\tfor (i = 0; sb && i < active_nr; i++) {\n-\t\t\tif (i)\n-\t\t\t\tstrbuf_addch(sb, ' ');\n-\t\t\tstrbuf_addstr(sb, active_cache[i]->name);\n-\t\t}\n-\t\treturn !!active_nr;\n-\t}\n-}\n-\n /**\n  * Dies with a user-friendly message on how to proceed after resolving the\n  * problem. This message can be overridden with state->resolvemsg.\ndiff --git a/cache.h b/cache.h\nindex a2ec8c0b55..d8b975a571 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -644,6 +644,15 @@ extern int write_locked_index(struct index_state *, struct lock_file *lock, unsi\n extern int discard_index(struct index_state *);\n extern void move_index_extensions(struct index_state *dst, struct index_state *src);\n extern int unmerged_index(const struct index_state *);\n+\n+/**\n+ * Returns 1 if the index differs from HEAD, 0 otherwise. When on an unborn\n+ * branch, returns 1 if there are entries in the index, 0 otherwise. If an\n+ * strbuf is provided, the space-separated list of files that differ will be\n+ * appended to it.\n+ */\n+extern int index_has_changes(struct strbuf *sb);\n+\n extern int verify_path(const char *path);\n extern int strcmp_offset(const char *s1, const char *s2, size_t *first_change);\n extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);\ndiff --git a/merge.c b/merge.c\nindex e5d796c9f2..195b578700 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -1,4 +1,6 @@\n #include \"cache.h\"\n+#include \"diff.h\"\n+#include \"diffcore.h\"\n #include \"lockfile.h\"\n #include \"commit.h\"\n #include \"run-command.h\"\n@@ -15,6 +17,37 @@ static const char *merge_argument(struct commit *commit)\n \t\treturn EMPTY_TREE_SHA1_HEX;\n }\n \n+int index_has_changes(struct strbuf *sb)\n+{\n+\tstruct object_id head;\n+\tint i;\n+\n+\tif (!get_oid_tree(\"HEAD\", &head)) {\n+\t\tstruct diff_options opt;\n+\n+\t\tdiff_setup(&opt);\n+\t\topt.flags.exit_with_status = 1;\n+\t\tif (!sb)\n+\t\t\topt.flags.quick = 1;\n+\t\tdo_diff_cache(&head, &opt);\n+\t\tdiffcore_std(&opt);\n+\t\tfor (i = 0; sb && i < diff_queued_diff.nr; i++) {\n+\t\t\tif (i)\n+\t\t\t\tstrbuf_addch(sb, ' ');\n+\t\t\tstrbuf_addstr(sb, diff_queued_diff.queue[i]->two->path);\n+\t\t}\n+\t\tdiff_flush(&opt);\n+\t\treturn opt.flags.has_changes != 0;\n+\t} else {\n+\t\tfor (i = 0; sb && i < active_nr; i++) {\n+\t\t\tif (i)\n+\t\t\t\tstrbuf_addch(sb, ' ');\n+\t\t\tstrbuf_addstr(sb, active_cache[i]->name);\n+\t\t}\n+\t\treturn !!active_nr;\n+\t}\n+}\n+\n int try_merge_command(const char *strategy, size_t xopts_nr,\n \t\t      const char **xopts, struct commit_list *common,\n \t\t      const char *head_arg, struct commit_list *remotes)\n-- \n2.15.1.436.g63a861020b\n\n"},{"id":"335146","messageId":"20171221191907.4251-1-newren@gmail.com","threadId":"47469","inReplyTo":"CABPp-BGy3_RyVQfCm+9O_AAfKA0_CZ5ajJE7NuLbToERWyWmqQ@mail.gmail.com","subject":"[PATCH 1/3] t6044: recursive can silently incorporate dirty changes in a merge","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-12-21T19:19:05Z","receivedAt":"2017-12-21T19:20:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"The recursive merge strategy has some special handling when the tree for\nthe merge branch exactly matches the merge base, but that code path is\nmissing checks for the index having changes relative to HEAD.  Add a\ntestcase covering this scenario.\n\nReported-by: Andreas Krey <a.krey@gmx.de>\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t6044-merge-unrelated-index-changes.sh | 26 +++++++++++++++++++++-----\n 1 file changed, 21 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\nindex 01023486c5..5e472be92b 100755\n--- a/t/t6044-merge-unrelated-index-changes.sh\n+++ b/t/t6044-merge-unrelated-index-changes.sh\n@@ -6,18 +6,21 @@ test_description=\"merges with unrelated index changes\"\n \n # Testcase for some simple merges\n #   A\n-#   o-----o B\n+#   o-------o B\n #    \\\n-#     \\---o C\n+#     \\-----o C\n #      \\\n-#       \\-o D\n+#       \\---o D\n #        \\\n-#         o E\n+#         \\-o E\n+#          \\\n+#           o F\n #   Commit A: some file a\n #   Commit B: adds file b, modifies end of a\n #   Commit C: adds file c\n #   Commit D: adds file d, modifies beginning of a\n #   Commit E: renames a->subdir/a, adds subdir/e\n+#   Commit F: empty commit\n \n test_expect_success 'setup trivial merges' '\n \ttest_seq 1 10 >a &&\n@@ -29,6 +32,7 @@ test_expect_success 'setup trivial merges' '\n \tgit branch C &&\n \tgit branch D &&\n \tgit branch E &&\n+\tgit branch F &&\n \n \tgit checkout B &&\n \techo b >b &&\n@@ -52,7 +56,10 @@ test_expect_success 'setup trivial merges' '\n \tgit mv a subdir/a &&\n \techo e >subdir/e &&\n \tgit add subdir &&\n-\ttest_tick && git commit -m E\n+\ttest_tick && git commit -m E &&\n+\n+\tgit checkout F &&\n+\ttest_tick && git commit --allow-empty -m F\n '\n \n test_expect_success 'ff update' '\n@@ -105,6 +112,15 @@ test_expect_success 'recursive' '\n \ttest_must_fail git merge -s recursive C^0\n '\n \n+test_expect_failure 'recursive, when merge branch matches merge base' '\n+\tgit reset --hard &&\n+\tgit checkout B^0 &&\n+\n+\ttouch random_file && git add random_file &&\n+\n+\ttest_must_fail git merge -s recursive F^0\n+'\n+\n test_expect_success 'octopus, unrelated file touched' '\n \tgit reset --hard &&\n \tgit checkout B^0 &&\n-- \n2.15.1.436.g63a861020b\n\n"},{"id":"335148","messageId":"CABPp-BGwZq2m4fexVKThGHwSFM3i3xxy2x9cZhtQvSHZ07unYg@mail.gmail.com","threadId":"47469","inReplyTo":"20171221191907.4251-2-newren@gmail.com","subject":"Re: [PATCH 2/3] move index_has_changes() from builtin/am.c to merge.c for reuse","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-12-21T19:36:38Z","receivedAt":"2017-12-21T19:36:44Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Dec 21, 2017 at 11:19 AM, Elijah Newren <newren@gmail.com> wrote:\n> index_has_changes() is a function we want to reuse outside of just am,\n> making it also available for merge-recursive and merge-ort.\n>\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n\nNote: These patches built on master, and merge cleanly with next and\npu.  However, this patch has a minor conflict with maint.  If you'd\nprefer a version that applies on top of maint, let me know and I'll\nresubmit.\n"},{"id":"335190","messageId":"xmqqh8siqz0j.fsf@gitster.mtv.corp.google.com","threadId":"47469","inReplyTo":"20171221191907.4251-3-newren@gmail.com","subject":"Re: [PATCH 3/3] merge-recursive: Avoid incorporating uncommitted changes in a merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-22T20:38:20Z","receivedAt":"2017-12-22T20:38:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> builtin/merge.c contains this important requirement for merge strategies:\n> \t/*\n> \t * At this point, we need a real merge.  No matter what strategy\n> \t * we use, it would operate on the index, possibly affecting the\n> \t * working tree, and when resolved cleanly, have the desired\n> \t * tree in the index -- this means that the index must be in\n> \t * sync with the head commit.  The strategies are responsible\n> \t * to ensure this.\n> \t */\n>\n> merge-recursive does not do this check directly, instead it relies on\n> unpack_trees() to do it.  However, merge_trees() has a special check for\n> the merge branch exactly matching the merge base; when it detects that\n> situation, it returns early without calling unpack_trees(), because it\n> knows that the HEAD commit already has the correct result.  Unfortunately,\n> it didn't check that the index matched HEAD, so after it returned, the\n> outer logic ended up creating a merge commit that included something\n> other than HEAD.\n\nGood.\n\nI actually was imagining that you would shoot for creating an empty\ncommit and leaving a working tree and the index that are both dirty,\nbut I do not think it is worth the effort.  Besides, \"you have to\nstart from a clean index\" is a much simpler rule to explain than\nwith \"unless the resulting tree is the same as HEAD\", especially\nwhen that \"unless\" is highly unlikely to happen anyway.\n\nThanks.\n\n>\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n>  merge-recursive.c                        | 7 +++++++\n>  t/t6044-merge-unrelated-index-changes.sh | 2 +-\n>  2 files changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 2ecf495cc2..780f81a8bd 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -1952,6 +1952,13 @@ int merge_trees(struct merge_options *o,\n>  \t}\n>  \n>  \tif (oid_eq(&common->object.oid, &merge->object.oid)) {\n> +\t\tstruct strbuf sb = STRBUF_INIT;\n> +\n> +\t\tif (index_has_changes(&sb)) {\n> +\t\t\terr(o, _(\"Dirty index: cannot merge (dirty: %s)\"),\n> +\t\t\t    sb.buf);\n> +\t\t\treturn 0;\n> +\t\t}\n>  \t\toutput(o, 0, _(\"Already up to date!\"));\n>  \t\t*result = head;\n>  \t\treturn 1;\n> diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh\n> index 5e472be92b..23b86fb977 100755\n> --- a/t/t6044-merge-unrelated-index-changes.sh\n> +++ b/t/t6044-merge-unrelated-index-changes.sh\n> @@ -112,7 +112,7 @@ test_expect_success 'recursive' '\n>  \ttest_must_fail git merge -s recursive C^0\n>  '\n>  \n> -test_expect_failure 'recursive, when merge branch matches merge base' '\n> +test_expect_success 'recursive, when merge branch matches merge base' '\n>  \tgit reset --hard &&\n>  \tgit checkout B^0 &&\n"},{"id":"335193","messageId":"xmqqd136qymc.fsf@gitster.mtv.corp.google.com","threadId":"47469","inReplyTo":"CABPp-BGwZq2m4fexVKThGHwSFM3i3xxy2x9cZhtQvSHZ07unYg@mail.gmail.com","subject":"Re: [PATCH 2/3] move index_has_changes() from builtin/am.c to merge.c for reuse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-22T20:46:51Z","receivedAt":"2017-12-22T20:46:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> On Thu, Dec 21, 2017 at 11:19 AM, Elijah Newren <newren@gmail.com> wrote:\n>> index_has_changes() is a function we want to reuse outside of just am,\n>> making it also available for merge-recursive and merge-ort.\n>>\n>> Signed-off-by: Elijah Newren <newren@gmail.com>\n>> ---\n>\n> Note: These patches built on master, and merge cleanly with next and\n> pu.  However, this patch has a minor conflict with maint.  If you'd\n> prefer a version that applies on top of maint, let me know and I'll\n> resubmit.\n\nI think I managed to create two topics, one that is with these three\npatches (2/3 backported) on top of maint and the other one merges\nthe former on top of master.  Please see if you found a mismerge\nwhen I push the results out.\n\nThanks.\n"},{"id":"335215","messageId":"CABPp-BF4h3nBUhr-akw0SFy_RboRjf_gYXe-+ikgRhFNXf3C1Q@mail.gmail.com","threadId":"47469","inReplyTo":"xmqqd136qymc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] move index_has_changes() from builtin/am.c to merge.c for reuse","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-12-23T02:26:30Z","receivedAt":"2017-12-23T02:26:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Dec 22, 2017 at 12:46 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Elijah Newren <newren@gmail.com> writes:\n>\n>> On Thu, Dec 21, 2017 at 11:19 AM, Elijah Newren <newren@gmail.com> wrote:\n>>> index_has_changes() is a function we want to reuse outside of just am,\n>>> making it also available for merge-recursive and merge-ort.\n>>>\n>>> Signed-off-by: Elijah Newren <newren@gmail.com>\n>>> ---\n>>\n>> Note: These patches built on master, and merge cleanly with next and\n>> pu.  However, this patch has a minor conflict with maint.  If you'd\n>> prefer a version that applies on top of maint, let me know and I'll\n>> resubmit.\n>\n> I think I managed to create two topics, one that is with these three\n> patches (2/3 backported) on top of maint and the other one merges\n> the former on top of master.  Please see if you found a mismerge\n> when I push the results out.\n\nI'm about to head out on a multi-day trip, so I might not get to this\nuntil the middle of next week.  I'll try to take a look as soon as I\ncan, though.\n"},{"id":"336192","messageId":"xmqqbmi484tw.fsf@gitster.mtv.corp.google.com","threadId":"47469","inReplyTo":"20171221191907.4251-3-newren@gmail.com","subject":"Re: [PATCH 3/3] merge-recursive: Avoid incorporating uncommitted changes in a merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-08T20:37:31Z","receivedAt":"2018-01-08T20:37:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 2ecf495cc2..780f81a8bd 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -1952,6 +1952,13 @@ int merge_trees(struct merge_options *o,\n>  \t}\n>  \n>  \tif (oid_eq(&common->object.oid, &merge->object.oid)) {\n> +\t\tstruct strbuf sb = STRBUF_INIT;\n> +\n> +\t\tif (index_has_changes(&sb)) {\n> +\t\t\terr(o, _(\"Dirty index: cannot merge (dirty: %s)\"),\n> +\t\t\t    sb.buf);\n> +\t\t\treturn 0;\n> +\t\t}\n>  \t\toutput(o, 0, _(\"Already up to date!\"));\n>  \t\t*result = head;\n>  \t\treturn 1;\n\nI haven't come up with an addition to the test suite, but I suspect\nthis change is conceptually wrong.  What if a call to this function\nis made during a recursive, inner merge?\n\nPerhaps something like this is needed?\n\n merge-recursive.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 780f81a8bd..0fc580d8ca 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1954,7 +1954,7 @@ int merge_trees(struct merge_options *o,\n \tif (oid_eq(&common->object.oid, &merge->object.oid)) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n-\t\tif (index_has_changes(&sb)) {\n+\t\tif (!o->call_depth && index_has_changes(&sb)) {\n \t\t\terr(o, _(\"Dirty index: cannot merge (dirty: %s)\"),\n \t\t\t    sb.buf);\n \t\t\treturn 0;\n\n\n"},{"id":"336277","messageId":"xmqq7esq7v4j.fsf_-_@gitster.mtv.corp.google.com","threadId":"47469","inReplyTo":"xmqqbmi484tw.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] merge-recursive: do not look at the index during recursive merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-09T18:19:24Z","receivedAt":"2018-01-09T18:19:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When merging another branch into ours, if their tree is the same as\nthe common ancestor's, we can declare that our tree represents the\nresult of three-way merge.  In such a case, the recursive merge\nbackend incorrectly used to create a commit out of our index, even\nwhen the index has changes.\n\nA recent fix attempted to prevent this by adding a comparison\nbetween \"our\" tree and the index, but forgot that this check must be\nrestricted only to the outermost merge.  Inner merges performed by\nthe recursive backend across merge bases are by definition made from\nscratch without having any local changes added to the index.  The\ncall to index_has_changes() during an inner merge is working on the\nindex that has no relation to the merge being performed, preventing\nlegitimate merges from getting carried out.\n\nFix it by limiting the check to the outermost merge.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n    Junio C Hamano <gitster@pobox.com> writes:\n\n    > Elijah Newren <newren@gmail.com> writes:\n    >\n    >> diff --git a/merge-recursive.c b/merge-recursive.c\n    >> index 2ecf495cc2..780f81a8bd 100644\n    >> --- a/merge-recursive.c\n    >> +++ b/merge-recursive.c\n    >> @@ -1952,6 +1952,13 @@ int merge_trees(struct merge_options *o,\n    >>  \t}\n    >>  \n    >>  \tif (oid_eq(&common->object.oid, &merge->object.oid)) {\n    >> +\t\tstruct strbuf sb = STRBUF_INIT;\n    >> +\n    >> +\t\tif (index_has_changes(&sb)) {\n    >> +\t\t\terr(o, _(\"Dirty index: cannot merge (dirty: %s)\"),\n    >> +\t\t\t    sb.buf);\n    >> +\t\t\treturn 0;\n    >> +\t\t}\n    >>  \t\toutput(o, 0, _(\"Already up to date!\"));\n    >>  \t\t*result = head;\n    >>  \t\treturn 1;\n    >\n    > I haven't come up with an addition to the test suite, but I suspect\n    > this change is conceptually wrong.  What if a call to this function\n    > is made during a recursive, inner merge?\n\n    Now I have.\n\n merge-recursive.c          |  2 +-\n t/t3030-merge-recursive.sh | 50 ++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 51 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 780f81a8bd..0fc580d8ca 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1954,7 +1954,7 @@ int merge_trees(struct merge_options *o,\n \tif (oid_eq(&common->object.oid, &merge->object.oid)) {\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n-\t\tif (index_has_changes(&sb)) {\n+\t\tif (!o->call_depth && index_has_changes(&sb)) {\n \t\t\terr(o, _(\"Dirty index: cannot merge (dirty: %s)\"),\n \t\t\t    sb.buf);\n \t\t\treturn 0;\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex 9a893b5fe7..12e3b1392d 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -678,4 +678,54 @@ test_expect_success 'merge-recursive remembers the names of all base trees' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'merge-recursive internal merge resolves to the sameness' '\n+\tgit reset --hard HEAD &&\n+\n+\t# We are going to create a history leading to two criss-cross\n+\t# branches A and B.  The common ancestor at the bottom, O0,\n+\t# has two childs O1 and O2, both of which will be merge base\n+\t# between A and B, like so:\n+\t#\n+\t#       O1---A\n+\t#      /  \\ /\n+\t#    O0    .\n+\t#      \\  / \\\n+\t#       O2---B\n+\t#\n+\t# The recently added \"check to see if the index is different from\n+\t# the tree into which something else is getting merged into and\n+\t# reject\" check must NOT kick in when an inner merge between O1\n+\t# and O2 is made.  Both O1 and O2 happen to have the same tree\n+\t# as O0 in this test to trigger the bug---whether the inner merge\n+\t# is made by merging O2 into O1 or O1 into O2, their common ancestor\n+\t# O0 and the branch being merged have the same tree, and the code\n+\t# before fix will incorrectly try to look at the index.\n+\n+\techo \"zero\" >file &&\n+\tgit add file &&\n+\ttest_tick &&\n+\tgit commit -m \"O0\" &&\n+\tO0=$(git rev-parse HEAD) &&\n+\n+\ttest_tick &&\n+\tgit commit --allow-empty -m \"O2\" &&\n+\tO1=$(git rev-parse HEAD) &&\n+\n+\tgit reset --hard $O0 &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m \"O2\" &&\n+\tO2=$(git rev-parse HEAD) &&\n+\n+\ttest_tick &&\n+\tgit merge -s ours $O1 &&\n+\tB=$(git rev-parse HEAD) &&\n+\n+\tgit reset --hard $O1 &&\n+\ttest_tick &&\n+\tgit merge -s ours $O2 &&\n+\tA=$(git rev-parse HEAD) &&\n+\n+\tgit merge $B\n+'\n+\n test_done\n-- \n2.16.0-rc1-164-gb9fca19b00\n\n"},{"id":"336279","messageId":"xmqq373e7uu7.fsf@gitster.mtv.corp.google.com","threadId":"47469","inReplyTo":"xmqq7esq7v4j.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH] merge-recursive: do not look at the index during recursive merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-09T18:25:36Z","receivedAt":"2018-01-09T18:25:43Z","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> + ...\n> +\ttest_tick &&\n> +\tgit commit --allow-empty -m \"O2\" &&\n> +\tO1=$(git rev-parse HEAD) &&\n> +\n> +\tgit reset --hard $O0 &&\n> +\ttest_tick &&\n> +\tgit commit --allow-empty -m \"O2\" &&\n> +\tO2=$(git rev-parse HEAD) &&\n\nDoes not affect the validity of the test at all, but the log message\nof the $O1 should be made with -m \"O1\", not with -m \"O2\".  That fix\nwill be in the version I'll be queuing.\n\n> +\n> +\ttest_tick &&\n> +\tgit merge -s ours $O1 &&\n> +\tB=$(git rev-parse HEAD) &&\n> +\n> +\tgit reset --hard $O1 &&\n> +\ttest_tick &&\n> +\tgit merge -s ours $O2 &&\n> +\tA=$(git rev-parse HEAD) &&\n> +\n> +\tgit merge $B\n> +'\n> +\n>  test_done\n"},{"id":"336280","messageId":"CAPig+cQ7Xn_DXG1NSyaktzybNPxiNBgRe=qeuqxrm9Z+GxCROQ@mail.gmail.com","threadId":"47469","inReplyTo":"xmqq7esq7v4j.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH] merge-recursive: do not look at the index during recursive merge","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-01-09T18:27:17Z","receivedAt":"2018-01-09T18:27:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 9, 2018 at 1:19 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> When merging another branch into ours, if their tree is the same as\n> the common ancestor's, we can declare that our tree represents the\n> result of three-way merge.  In such a case, the recursive merge\n> backend incorrectly used to create a commit out of our index, even\n> when the index has changes.\n>\n> A recent fix attempted to prevent this by adding a comparison\n> between \"our\" tree and the index, but forgot that this check must be\n> restricted only to the outermost merge.  Inner merges performed by\n> the recursive backend across merge bases are by definition made from\n> scratch without having any local changes added to the index.  The\n> call to index_has_changes() during an inner merge is working on the\n> index that has no relation to the merge being performed, preventing\n> legitimate merges from getting carried out.\n>\n> Fix it by limiting the check to the outermost merge.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n> diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\n> @@ -678,4 +678,54 @@ test_expect_success 'merge-recursive remembers the names of all base trees' '\n> +test_expect_success 'merge-recursive internal merge resolves to the sameness' '\n> +       git reset --hard HEAD &&\n> +\n> +       # We are going to create a history leading to two criss-cross\n> +       # branches A and B.  The common ancestor at the bottom, O0,\n> +       # has two childs O1 and O2, both of which will be merge base\n\ns/childs/children,/\n\n> +       # between A and B, like so:\n> +       #\n> +       #       O1---A\n> +       #      /  \\ /\n> +       #    O0    .\n> +       #      \\  / \\\n> +       #       O2---B\n> +       #\n> +       # The recently added \"check to see if the index is different from\n> +       # the tree into which something else is getting merged into and\n\nToo many \"into\"s: s/merged into/merged/\n\n> +       # reject\" check must NOT kick in when an inner merge between O1\n> +       # and O2 is made.  Both O1 and O2 happen to have the same tree\n> +       # as O0 in this test to trigger the bug---whether the inner merge\n> +       # is made by merging O2 into O1 or O1 into O2, their common ancestor\n> +       # O0 and the branch being merged have the same tree, and the code\n> +       # before fix will incorrectly try to look at the index.\n\nNit: Does \"code before fix\" belong in this comment? It sounds more\nlike something you'd say in the commit message.\n\n> +\n> +       echo \"zero\" >file &&\n> +       git add file &&\n> +       test_tick &&\n> +       git commit -m \"O0\" &&\n> +       O0=$(git rev-parse HEAD) &&\n> +\n> +       test_tick &&\n> +       git commit --allow-empty -m \"O2\" &&\n\ns/O2/O1/\n\n> +       O1=$(git rev-parse HEAD) &&\n> +\n> +       git reset --hard $O0 &&\n> +       test_tick &&\n> +       git commit --allow-empty -m \"O2\" &&\n> +       O2=$(git rev-parse HEAD) &&\n> +\n> +       test_tick &&\n> +       git merge -s ours $O1 &&\n> +       B=$(git rev-parse HEAD) &&\n> +\n> +       git reset --hard $O1 &&\n> +       test_tick &&\n> +       git merge -s ours $O2 &&\n> +       A=$(git rev-parse HEAD) &&\n> +\n> +       git merge $B\n> +'\n"},{"id":"336282","messageId":"CABPp-BEJS+59FD-1WduHMmtnBBrgS7xDWJm8Z5URrthDp-0Bwg@mail.gmail.com","threadId":"47469","inReplyTo":"xmqq7esq7v4j.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH] merge-recursive: do not look at the index during recursive merge","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-01-09T18:29:50Z","receivedAt":"2018-01-09T18:30:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Tue, Jan 9, 2018 at 11:19 AM, Junio C Hamano <gitster@pobox.com> wrote:\n\n>     > I haven't come up with an addition to the test suite, but I suspect\n>     > this change is conceptually wrong.  What if a call to this function\n>     > is made during a recursive, inner merge?\n\nEek, good catch.\n\n>  merge-recursive.c          |  2 +-\n>  t/t3030-merge-recursive.sh | 50 ++++++++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 51 insertions(+), 1 deletion(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 780f81a8bd..0fc580d8ca 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -1954,7 +1954,7 @@ int merge_trees(struct merge_options *o,\n>         if (oid_eq(&common->object.oid, &merge->object.oid)) {\n>                 struct strbuf sb = STRBUF_INIT;\n>\n> -               if (index_has_changes(&sb)) {\n> +               if (!o->call_depth && index_has_changes(&sb)) {\n>                         err(o, _(\"Dirty index: cannot merge (dirty: %s)\"),\n>                             sb.buf);\n>                         return 0;\n\nYep, looks good to me; sorry for overlooking this.\n\nElijah\n"},{"id":"336284","messageId":"xmqqy3l66f5w.fsf@gitster.mtv.corp.google.com","threadId":"47469","inReplyTo":"CABPp-BEJS+59FD-1WduHMmtnBBrgS7xDWJm8Z5URrthDp-0Bwg@mail.gmail.com","subject":"Re: [PATCH] merge-recursive: do not look at the index during recursive merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-09T18:49:31Z","receivedAt":"2018-01-09T18:49:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> Hi,\n>\n> On Tue, Jan 9, 2018 at 11:19 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n>>     > I haven't come up with an addition to the test suite, but I suspect\n>>     > this change is conceptually wrong.  What if a call to this function\n>>     > is made during a recursive, inner merge?\n>\n> Eek, good catch.\n>\n>>  merge-recursive.c          |  2 +-\n>>  t/t3030-merge-recursive.sh | 50 ++++++++++++++++++++++++++++++++++++++++++++++\n>>  2 files changed, 51 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/merge-recursive.c b/merge-recursive.c\n>> index 780f81a8bd..0fc580d8ca 100644\n>> --- a/merge-recursive.c\n>> +++ b/merge-recursive.c\n>> @@ -1954,7 +1954,7 @@ int merge_trees(struct merge_options *o,\n>>         if (oid_eq(&common->object.oid, &merge->object.oid)) {\n>>                 struct strbuf sb = STRBUF_INIT;\n>>\n>> -               if (index_has_changes(&sb)) {\n>> +               if (!o->call_depth && index_has_changes(&sb)) {\n>>                         err(o, _(\"Dirty index: cannot merge (dirty: %s)\"),\n>>                             sb.buf);\n>>                         return 0;\n>\n> Yep, looks good to me; sorry for overlooking this.\n>\n> Elijah\n\nThanks.  The breakage is already in 'master' so this fix needs to be\nfast-tracked.\n"}]}