{"thread":{"id":"48266","subject":"[Git] recursive merge on 'master' severely broken?","startedAt":"2018-04-11T07:19:39Z","lastAt":"2018-04-16T21:21:15Z","messageCount":13,"participants":["Junio C Hamano","Elijah Newren","Stefan Beller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"344446","messageId":"xmqqmuya43cs.fsf@gitster-ct.c.googlers.com","threadId":"48266","inReplyTo":null,"subject":"[Git] recursive merge on 'master' severely broken?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-11T07:19:31Z","receivedAt":"2018-04-11T07:19:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"It appears that a topic recently graduated to 'master' introduces a\nsevere regression to \"git merge\" when it merges a side branch that\nrenames paths while the trunk has further updates.\n\nThe symptom can easily be seen by trying to recreate the merge I\nmade at the tip of 'pu' 29dea678 (\"Merge branch\n'sb/filenames-with-dashes' into pu\", 2018-04-11) that I'll be.\npushing out shortly.  The side branch renames a file exec_cmd.h to\nexec-cmd.h (an underscore changed to a dash) without changing any\ncontents, while the branch being merged to has made some changes to\nthe contents while keeping the original pathname.\n\nA clean automerged result should leave the identical contents from\nHEAD:exec_cmd.h in :exec-cmd.h in the index, which is what happens\nwhen using Git v2.17.0 proper, but with today's master', there are\ncontent changes that cannot be explained--the merge is simply broken\nand worse yet, the command pretends that everything went well and\nmerged cleanly in that path.  Overly clever tool that behaves in a\nbuggy and unexplainable way is bad enough, doing so silently is\nunexcusable.\n\nI suspect that the culprit is the merge e4bb62fa (\"Merge branch\n'en/rename-directory-detection'\", 2018-04-10), and I am planning to\nrevert it out of 'master' as soon as possible, but it is already\nburied deep in other topics, so I may not be able to get to it until\nlater this week.  Sorry about that.\n\n"},{"id":"344448","messageId":"xmqqin8y3yf1.fsf@gitster-ct.c.googlers.com","threadId":"48266","inReplyTo":"xmqqmuya43cs.fsf@gitster-ct.c.googlers.com","subject":"Re: [Git] recursive merge on 'master' severely broken?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-11T09:06:10Z","receivedAt":"2018-04-11T09:06:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> It appears that a topic recently graduated to 'master' introduces a\n> severe regression to \"git merge\" when it merges a side branch that\n> renames paths while the trunk has further updates.\n> ...\n> I suspect that the culprit is the merge e4bb62fa (\"Merge branch\n> 'en/rename-directory-detection'\", 2018-04-10), and I am planning to\n> revert it out of 'master' as soon as possible, but it is already\n> buried deep in other topics, so I may not be able to get to it until\n> later this week.  Sorry about that.\n\nAn interim report.\n\nIt indeed is that merge with the topic.  I rebuilt an equivalent of\n'master' without that topic on top of v2.17.0 and tried the same\nmerge of sb/filenames-with-dashes fd055186 (\"replace_object.c:\nrename to use dash in file name\", 2018-04-10) topic into pu^\n35f7512e (\"Merge branch 'fg/completion-external' into pu\",\n2018-04-11), and that version of Git does not exhibit the problem.\n\nI do not yet know which one of the ~30 individual commmits is the\nreal culprit, but in the meantime, I'll revert the merge out of\n'master' and push the result out, so that real Git-using projects\nwon't get silent mismerges.  Those who run in-house version of Git\nthat is based on anything newer than the released version may want\nto do the same.\n\nSorry for the crappy 'master' X-<.\n"},{"id":"344450","messageId":"xmqqefjm3w1h.fsf@gitster-ct.c.googlers.com","threadId":"48266","inReplyTo":"xmqqin8y3yf1.fsf@gitster-ct.c.googlers.com","subject":"Re: [Git] recursive merge on 'master' severely broken?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-11T09:57:30Z","receivedAt":"2018-04-11T09:57:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Another interim report.\n>\n> I do not yet know which one of the ~30 individual commmits is the\n> real culprit, ...\n\nIt bisects down to c5b761fb (\"merge-recursive: ensure we write\nupdates for directory-renamed file\", 2018-02-14); given that a part\nof the symptom I saw was a few messages like these:\n\n        ...\n        CONFLICT (content): Merge conflict in upload-pack.c\n        error: addinfo_cache failed for path 'sha1-name.c'\n        error: addinfo_cache failed for path 'sha1-file.c'\n        Auto-merging sequencer.c\n        ...\n\nand the patch does change code around the callsites to\nadd_cacheinfo(), it does look like a plausible culprit.\n\n"},{"id":"344463","messageId":"CABPp-BHL4P0RxQ6OAuDSev9BXVM0uKTYD3M4JGTQvSwcBv4K0Q@mail.gmail.com","threadId":"48266","inReplyTo":"xmqqmuya43cs.fsf@gitster-ct.c.googlers.com","subject":"Re: [Git] recursive merge on 'master' severely broken?","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-11T15:51:48Z","receivedAt":"2018-04-11T15:51:55Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Apr 11, 2018 at 12:19 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> It appears that a topic recently graduated to 'master' introduces a\n> severe regression to \"git merge\" when it merges a side branch that\n> renames paths while the trunk has further updates.\n>\n> The symptom can easily be seen by trying to recreate the merge I\n> made at the tip of 'pu' 29dea678 (\"Merge branch\n> 'sb/filenames-with-dashes' into pu\", 2018-04-11) that I'll be.\n> pushing out shortly.  The side branch renames a file exec_cmd.h to\n> exec-cmd.h (an underscore changed to a dash) without changing any\n> contents, while the branch being merged to has made some changes to\n> the contents while keeping the original pathname.\n>\n> A clean automerged result should leave the identical contents from\n> HEAD:exec_cmd.h in :exec-cmd.h in the index, which is what happens\n> when using Git v2.17.0 proper, but with today's master', there are\n> content changes that cannot be explained--the merge is simply broken\n> and worse yet, the command pretends that everything went well and\n> merged cleanly in that path.  Overly clever tool that behaves in a\n> buggy and unexplainable way is bad enough, doing so silently is\n> unexcusable.\n\nI agree, that is _really_ bad.  My sincerest apologies.  I'll dig into it.\n"},{"id":"344527","messageId":"xmqqo9ip2oik.fsf@gitster-ct.c.googlers.com","threadId":"48266","inReplyTo":"CABPp-BHL4P0RxQ6OAuDSev9BXVM0uKTYD3M4JGTQvSwcBv4K0Q@mail.gmail.com","subject":"Re: [Git] recursive merge on 'master' severely broken?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-12T01:37:39Z","receivedAt":"2018-04-12T01:37:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> On Wed, Apr 11, 2018 at 12:19 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> It appears that a topic recently graduated to 'master' introduces a\n>> severe regression to \"git merge\" when it merges a side branch that\n>> renames paths while the trunk has further updates.\n>>\n>> The symptom can easily be seen by trying to recreate the merge I\n>> made at the tip of 'pu' 29dea678 (\"Merge branch\n>> 'sb/filenames-with-dashes' into pu\", 2018-04-11) that I'll be.\n>> pushing out shortly.  The side branch renames a file exec_cmd.h to\n>> exec-cmd.h (an underscore changed to a dash) without changing any\n>> contents, while the branch being merged to has made some changes to\n>> the contents while keeping the original pathname.\n> ...\n> I agree, that is _really_ bad.  My sincerest apologies.  I'll dig into it.\n\nThanks.  It is not unusual for a moderately large set of changes to\nhave glitches that need time to be discovered.  I was hoping that\nplacing it in 'next' and keeping it there for several weeks would\nhave been sufficient for people who do use stuff from there in real\nlife but apparently that wasn't the case.\n\n"},{"id":"344630","messageId":"20180413195607.18091-5-newren@gmail.com","threadId":"48266","inReplyTo":"20180413195607.18091-1-newren@gmail.com","subject":"[PATCH v9 30/30] merge-recursive: fix check for skipability of working tree updates","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-13T19:56:07Z","receivedAt":"2018-04-13T19:56:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"The can-working-tree-updates-be-skipped check has had a long and blemished\nhistory.  The update can be skipped iff:\n  a) The merged contents match what was in HEAD\n  b) The merged mode matches what was in HEAD\n  c) The target path is usable and matches what was in HEAD\n\nSteps a & b are easy to check; we have always gotten those right.  Step c\nis just:\n  c1) Nothing else is in the way of putting the content at the target path\n      (i.e. it isn't involved in any D/F conflicts)\n  c2) target path was tracked in the index before the merge\n\nWhile it is easy to overlook c1, this was fixed seven years ago with\ncommit 4ab9a157d069 (\"merge_content(): Check whether D/F conflicts are\nstill present\", 2010-09-20).  merge-recursive didn't have a readily\navailable way to directly check c2, so various approximations were\nused:\n\n  * In commit b2c8c0a76274 (\"merge-recursive: When we detect we can skip\n    an update, actually skip it\", 2011-02-28), it was noted that although\n    the code claimed it was skipping the update, it did not actually skip\n    the update.  The code was made to skip it, but used lstat(path, ...)\n    as an approximation to path-was-tracked-in-index-before-merge.\n\n  * In commit 5b448b853030 (\"merge-recursive: When we detect we can skip\n    an update, actually skip it\", 2011-08-11), the problem with using\n    lstat was noted.  It was changed to the approximation\n       path2 && strcmp(path, path2)\n    which is also wrong.  !path2 || strcmp(path, path2) would have been\n    better, but would have fallen short with directory renames.\n\n  * In c5b761fb2711 (\"merge-recursive: ensure we write updates for\n    directory-renamed file\", 2018-02-14), the problem with the previous\n    approximation was noted and changed to\n       was_tracked(path)\n    That looks like exactly what we were trying to answer, and is what\n    was_tracked() was *intended* for, but not what was_tracked()\n    actually returned.\n\nSince the previous commit made was_tracked(path) actually mean \"path was\ntracked in the index before the merge\", we can now use it instead of\nother approximations to answer the question \"was path tracked in the\nindex before the merge?\"  So, although the code change in this commit is\nthe same one made in c5b761fb2711, it now safe and correct due to the\nprior fix to was_tracked().\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n merge-recursive.c                      | 20 +++++++++++++-------\n t/t6043-merge-rename-directories.sh    |  2 +-\n t/t6046-merge-skip-unneeded-updates.sh |  4 ++--\n 3 files changed, 16 insertions(+), 10 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex adc800f188..1b71d00fdb 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2786,16 +2786,22 @@ static int merge_content(struct merge_options *o,\n \t\t\t\t       o->branch2, path2, &mfi))\n \t\treturn -1;\n \n+\t/*\n+\t * We can skip updating the working tree file iff:\n+\t *   a) The merged contents match what was in HEAD\n+\t *   b) The merged mode matches what was in HEAD\n+\t *   c) The target path is usable and matches what was in HEAD\n+\t * We test (a) & (b) here.\n+\t */\n \tif (mfi.clean && oid_eq(&mfi.oid, a_oid) && mfi.mode == a_mode) {\n-\t\tint path_renamed_outside_HEAD;\n \t\t/*\n-\t\t * The content merge resulted in the same file contents we\n-\t\t * already had.  We can return early if those file contents\n-\t\t * are recorded at the correct path (which may not be true\n-\t\t * if the merge involves a rename or there's a D/F conflict).\n+\t\t * Case c has two pieces:\n+\t\t *   c1) Nothing else is in the way of writing the merged\n+\t\t *       results to path (i.e. it isn't involved in any\n+\t\t *       D/F conflict)\n+\t\t *   c2) path was tracked in the index before the merge\n \t\t */\n-\t\tpath_renamed_outside_HEAD = !path2 || !strcmp(path, path2);\n-\t\tif (!df_conflict_remains && !path_renamed_outside_HEAD) {\n+\t\tif (!df_conflict_remains && was_tracked(o, path)) {\n \t\t\toutput(o, 3, _(\"Skipped %s (merged same as existing)\"), path);\n \t\t\tadd_cacheinfo(o, mfi.mode, &mfi.oid, path,\n \t\t\t\t      0, (!o->call_depth), 0);\ndiff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh\nindex 45f620633f..2e28f2908d 100755\n--- a/t/t6043-merge-rename-directories.sh\n+++ b/t/t6043-merge-rename-directories.sh\n@@ -3884,7 +3884,7 @@ test_expect_success '12b-setup: Moving one directory hierarchy into another' '\n \t)\n '\n \n-test_expect_failure '12b-check: Moving one directory hierarchy into another' '\n+test_expect_success '12b-check: Moving one directory hierarchy into another' '\n \t(\n \t\tcd 12b &&\n \ndiff --git a/t/t6046-merge-skip-unneeded-updates.sh b/t/t6046-merge-skip-unneeded-updates.sh\nindex 89c3a953ae..4dcec7226c 100755\n--- a/t/t6046-merge-skip-unneeded-updates.sh\n+++ b/t/t6046-merge-skip-unneeded-updates.sh\n@@ -64,7 +64,7 @@ test_expect_success '1a-setup: Modify(A)/Modify(B), change on B subset of A' '\n \t)\n '\n \n-test_expect_failure '1a-check-L: Modify(A)/Modify(B), change on B subset of A' '\n+test_expect_success '1a-check-L: Modify(A)/Modify(B), change on B subset of A' '\n \ttest_when_finished \"git -C 1a reset --hard\" &&\n \t(\n \t\tcd 1a &&\n@@ -346,7 +346,7 @@ test_expect_success '3a-setup: bq_1->foo/bq_2 on A, foo/->bar/ on B' '\n \t)\n '\n \n-test_expect_failure '3a-check-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' '\n+test_expect_success '3a-check-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' '\n \ttest_when_finished \"git -C 3a reset --hard\" &&\n \t(\n \t\tcd 3a &&\n-- \n2.16.0.35.g6dd7ede834\n\n"},{"id":"344631","messageId":"20180413195607.18091-1-newren@gmail.com","threadId":"48266","inReplyTo":"xmqqmuya43cs.fsf@gitster-ct.c.googlers.com","subject":"[RFC PATCH v9 0/30] Add directory rename detection to git","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-13T19:56:03Z","receivedAt":"2018-04-13T19:56:24Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series builds on commit febb3a86098f (\"merge-recursive: avoid\nspurious rename/rename conflict from dir renames\", 2018-02-14), also known\nas patch 29/30 of en/rename-directory-detection.  That patch series has\nbeen reverted from master[1], due to a bug with patch 30/30, so does not\napply to current master.  But I didn't want to resend the whole series for\nan RFC.\n\nThese four patches replace that patch and:\n  - fixes Linus' rewriting-of-unchanged-files bug[1]\n  - fixes the problems that broke Junio's merges after my series[2]\n  - fixes the problem the original patch 30/30 was intended to solve\n  - adds lots of testcases to make sure this doesn't regress.\n\nLinus' alternative of stupid-brute-force[3], would also work here,\nthough I feel the first three patches are useful even if we take some\nform of his patch.  Long term, the most correct solution would involve\na rewrite to merge-recursive that would simplify this code[4], though\nI think the changes in this series brings this part of the code closer\nto that end state.\n\nThe big questions here are:\n  1) The last time my rename-directory-detection series was merged into\n     master it bit Junio badly.  I'm planning to redo all merges of\n     git.git and linux.git and comparing v2.17.0 to what I get after\n     my changes.  What else should I test?\n  2) What do folks thing about stupid-brute-force vs. the explanation in\n     my final patch?\n\n\n[1] https://public-inbox.org/git/CA+55aFzLZ3UkG5svqZwSnhNk75=fXJRkvU1m_RHBG54NOoaZPA@mail.gmail.com/\n[2] https://public-inbox.org/git/xmqqmuya43cs.fsf@gitster-ct.c.googlers.com/\n[3] https://public-inbox.org/git/CA+55aFwi9pTAJT_qtv=vHLgu=B1fdXBoD96i8Y5xnbS=zrfSzg@mail.gmail.com/\n[4] https://public-inbox.org/git/xmqqd147kpdm.fsf@gitster.mtv.corp.google.com/\n\n\nElijah Newren (4):\n  merge-recursive: improve output precision around skipping updates\n  t6046: testcases checking whether updates can be skipped in a merge\n  merge-recursive: Fix was_tracked() to quit lying with some renamed\n    paths\n  merge-recursive: fix check for skipability of working tree updates\n\n merge-recursive.c                      | 109 +++++---\n merge-recursive.h                      |   1 +\n t/t6022-merge-rename.sh                |   2 +-\n t/t6043-merge-rename-directories.sh    |   2 +-\n t/t6046-merge-skip-unneeded-updates.sh | 497 +++++++++++++++++++++++++++++++++\n 5 files changed, 575 insertions(+), 36 deletions(-)\n create mode 100755 t/t6046-merge-skip-unneeded-updates.sh\n\n-- \n2.16.0.35.g6dd7ede834\n\n"},{"id":"344632","messageId":"20180413195607.18091-4-newren@gmail.com","threadId":"48266","inReplyTo":"20180413195607.18091-1-newren@gmail.com","subject":"[PATCH v9 29.75/30] merge-recursive: Fix was_tracked() to quit lying with some renamed paths","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-13T19:56:06Z","receivedAt":"2018-04-13T19:56:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"In commit aacb82de3ff8 (\"merge-recursive: Split was_tracked() out of\nwould_lose_untracked()\", 2011-08-11), was_tracked() was split out of\nwould_lose_untracked() with the intent to provide a function that could\nanswer whether a path was tracked in the index before the merge.  Sadly,\nit instead returned whether the path was in the working tree due to having\nbeen tracked in the index before the merge OR having been written there by\nunpack_trees().  The distinction is important when renames are involved,\ne.g. for a merge where:\n\n   HEAD:  modifies path b\n   other: renames b->c\n\nIn this case, c was not tracked in the index before the merge, but would\nhave been added to the index at stage 0 and written to the working tree by\nunpack_trees().  would_lose_untracked() is more interested in the\nin-working-copy-for-either-reason behavior, while all other uses of\nwas_tracked() want just was-it-tracked-in-index-before-merge behavior.\n\nUnsplit would_lose_untracked() and write a new was_tracked() function\nwhich answers whether a path was tracked in the index before the merge\nstarted.\n\nThis will also make was_dirty() return better results, as it will be based\noff the original index rather than an index that possibly only copied over\nsome of the stat information.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n merge-recursive.c | 82 +++++++++++++++++++++++++++++++++++++++----------------\n merge-recursive.h |  1 +\n 2 files changed, 59 insertions(+), 24 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 05250939c7..adc800f188 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -344,6 +344,7 @@ static int git_merge_trees(struct merge_options *o,\n {\n \tint rc;\n \tstruct tree_desc t[3];\n+\tstruct index_state tmp_index = { NULL };\n \n \tmemset(&o->unpack_opts, 0, sizeof(o->unpack_opts));\n \tif (o->call_depth)\n@@ -354,7 +355,7 @@ static int git_merge_trees(struct merge_options *o,\n \to->unpack_opts.head_idx = 2;\n \to->unpack_opts.fn = threeway_merge;\n \to->unpack_opts.src_index = &the_index;\n-\to->unpack_opts.dst_index = &the_index;\n+\to->unpack_opts.dst_index = &tmp_index;\n \tsetup_unpack_trees_porcelain(&o->unpack_opts, \"merge\");\n \n \tinit_tree_desc_from_tree(t+0, common);\n@@ -362,13 +363,17 @@ static int git_merge_trees(struct merge_options *o,\n \tinit_tree_desc_from_tree(t+2, merge);\n \n \trc = unpack_trees(3, t, &o->unpack_opts);\n+\tcache_tree_free(&active_cache_tree);\n+\n+\to->orig_index = the_index;\n+\tthe_index = tmp_index;\n+\n \t/*\n-\t * unpack_trees NULLifies src_index, but it's used in verify_uptodate,\n-\t * so set to the new index which will usually have modification\n-\t * timestamp info copied over.\n+\t * src_index is used in verify_uptodate, but was NULLified in\n+\t * unpack_trees, so we need to set it back to the original index.\n \t */\n-\to->unpack_opts.src_index = &the_index;\n-\tcache_tree_free(&active_cache_tree);\n+\to->unpack_opts.src_index = &o->orig_index;\n+\n \treturn rc;\n }\n \n@@ -773,31 +778,51 @@ static int dir_in_way(const char *path, int check_working_copy, int empty_ok)\n \t\t!(empty_ok && is_empty_dir(path));\n }\n \n-static int was_tracked(const char *path)\n+/*\n+ * Returns whether path was tracked in the index before the merge started\n+ */\n+static int was_tracked(struct merge_options *o, const char *path)\n {\n-\tint pos = cache_name_pos(path, strlen(path));\n+\tint pos = index_name_pos(&o->orig_index, path, strlen(path));\n \n \tif (0 <= pos)\n-\t\t/* we have been tracking this path */\n+\t\t/* we were tracking this path before the merge */\n \t\treturn 1;\n \n-\t/*\n-\t * Look for an unmerged entry for the path,\n-\t * specifically stage #2, which would indicate\n-\t * that \"our\" side before the merge started\n-\t * had the path tracked (and resulted in a conflict).\n-\t */\n-\tfor (pos = -1 - pos;\n-\t     pos < active_nr && !strcmp(path, active_cache[pos]->name);\n-\t     pos++)\n-\t\tif (ce_stage(active_cache[pos]) == 2)\n-\t\t\treturn 1;\n \treturn 0;\n }\n \n static int would_lose_untracked(const char *path)\n {\n-\treturn !was_tracked(path) && file_exists(path);\n+\t/*\n+\t * This may look like it can be simplified to:\n+\t *   return !was_tracked(o, path) && file_exists(path)\n+\t * but it can't.  This function needs to know whether path was\n+\t * in the working tree due to EITHER having been tracked in the\n+\t * index before the merge OR having been put into the working copy\n+\t * and index by unpack_trees().  Due to that either-or requirement,\n+\t * we check the current index instead of the original one.\n+\t */\n+\tint pos = cache_name_pos(path, strlen(path));\n+\n+\tif (pos < 0)\n+\t\tpos = -1 - pos;\n+\twhile (pos < active_nr &&\n+\t       !strcmp(path, active_cache[pos]->name)) {\n+\t\t/*\n+\t\t * If stage #0, it is definitely tracked.\n+\t\t * If it has stage #2 then it was tracked\n+\t\t * before this merge started.  All other\n+\t\t * cases the path was not tracked.\n+\t\t */\n+\t\tswitch (ce_stage(active_cache[pos])) {\n+\t\tcase 0:\n+\t\tcase 2:\n+\t\t\treturn 0;\n+\t\t}\n+\t\tpos++;\n+\t}\n+\treturn file_exists(path);\n }\n \n static int was_dirty(struct merge_options *o, const char *path)\n@@ -805,7 +830,7 @@ static int was_dirty(struct merge_options *o, const char *path)\n \tstruct cache_entry *ce;\n \tint dirty = 1;\n \n-\tif (o->call_depth || !was_tracked(path))\n+\tif (o->call_depth || !was_tracked(o, path))\n \t\treturn !dirty;\n \n \tce = cache_file_exists(path, strlen(path), ignore_case);\n@@ -2407,7 +2432,7 @@ static int process_renames(struct merge_options *o,\n \t\t\t * add-source case).\n \t\t\t */\n \t\t\tremove_file(o, 1, ren1_src,\n-\t\t\t\t    renamed_stage == 2 || !was_tracked(ren1_src));\n+\t\t\t\t    renamed_stage == 2 || !was_tracked(o, ren1_src));\n \n \t\t\toidcpy(&src_other.oid,\n \t\t\t       &ren1->src_entry->stages[other_stage].oid);\n@@ -2800,7 +2825,7 @@ static int merge_content(struct merge_options *o,\n \t\t\t\tif (update_stages(o, path, &one, &a, &b))\n \t\t\t\t\treturn -1;\n \t\t\t} else {\n-\t\t\t\tint file_from_stage2 = was_tracked(path);\n+\t\t\t\tint file_from_stage2 = was_tracked(o, path);\n \t\t\t\tstruct diff_filespec merged;\n \t\t\t\toidcpy(&merged.oid, &mfi.oid);\n \t\t\t\tmerged.mode = mfi.mode;\n@@ -3075,6 +3100,15 @@ int merge_trees(struct merge_options *o,\n \telse\n \t\tclean = 1;\n \n+\t/* Free the extra index left from git_merge_trees() */\n+\t/*\n+\t * FIXME: Need to also data allocated by setup_unpack_trees_porcelain()\n+\t * tucked away in o->unpack_opts.msgs, but the problem is that only\n+\t * half of it refers to dynamically allocated data, while the other\n+\t * half points at static strings.\n+\t */\n+\tdiscard_index(&o->orig_index);\n+\n \tif (o->call_depth && !(*result = write_tree_from_memory(o)))\n \t\treturn -1;\n \ndiff --git a/merge-recursive.h b/merge-recursive.h\nindex d863cf8867..248093e407 100644\n--- a/merge-recursive.h\n+++ b/merge-recursive.h\n@@ -29,6 +29,7 @@ struct merge_options {\n \tstruct hashmap current_file_dir_set;\n \tstruct string_list df_conflict_file_set;\n \tstruct unpack_trees_options unpack_opts;\n+\tstruct index_state orig_index;\n };\n \n /*\n-- \n2.16.0.35.g6dd7ede834\n\n"},{"id":"344633","messageId":"20180413195607.18091-3-newren@gmail.com","threadId":"48266","inReplyTo":"20180413195607.18091-1-newren@gmail.com","subject":"[PATCH v9 29.50/30] t6046: testcases checking whether updates can be skipped in a merge","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-13T19:56:05Z","receivedAt":"2018-04-13T19:56:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Add several tests checking whether updates can be skipped in a merge.\nAlso add several similar testcases for where updates cannot be skipped in\na merge to make sure that we skip if and only if we should.\n\nIn particular:\n\n  * Testcase 1a (particularly 1a-check-L) would have pointed out the\n    problem Linus has been dealing with for year with his merges[1].\n\n  * Testcase 2a (particularly 2a-check-L) would have pointed out the\n    problem with my directory-rename-series before it broke master[2].\n\n  * Testcases 3[ab] (particularly 3a-check-L) provide a simpler testcase\n    than 12b of t6043 making that one easier to understand.\n\n  * There are several complementary testcases to make sure we're not just\n    fixing those particular issues while regressing in the opposite\n    direction.\n\n[1] https://public-inbox.org/git/CA+55aFzLZ3UkG5svqZwSnhNk75=fXJRkvU1m_RHBG54NOoaZPA@mail.gmail.com/\n[2] https://public-inbox.org/git/xmqqmuya43cs.fsf@gitster-ct.c.googlers.com/\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t6046-merge-skip-unneeded-updates.sh | 497 +++++++++++++++++++++++++++++++++\n 1 file changed, 497 insertions(+)\n create mode 100755 t/t6046-merge-skip-unneeded-updates.sh\n\ndiff --git a/t/t6046-merge-skip-unneeded-updates.sh b/t/t6046-merge-skip-unneeded-updates.sh\nnew file mode 100755\nindex 0000000000..89c3a953ae\n--- /dev/null\n+++ b/t/t6046-merge-skip-unneeded-updates.sh\n@@ -0,0 +1,497 @@\n+#!/bin/sh\n+\n+test_description=\"merge cases\"\n+\n+# The setup for all of them, pictorially, is:\n+#\n+#      A\n+#      o\n+#     / \\\n+#  O o   ?\n+#     \\ /\n+#      o\n+#      B\n+#\n+# To help make it easier to follow the flow of tests, they have been\n+# divided into sections and each test will start with a quick explanation\n+# of what commits O, A, and B contain.\n+#\n+# Notation:\n+#    z/{b,c}   means  files z/b and z/c both exist\n+#    x/d_1     means  file x/d exists with content d1.  (Purpose of the\n+#                     underscore notation is to differentiate different\n+#                     files that might be renamed into each other's paths.)\n+\n+. ./test-lib.sh\n+\n+\n+###########################################################################\n+# SECTION 1: Cases involving no renames (one side has subset of changes of\n+#            the other side)\n+###########################################################################\n+\n+# Testcase 1a, Changes on A, subset of changes on B\n+#   Commit O: b_1\n+#   Commit A: b_2\n+#   Commit B: b_3\n+#   Expected: b_2\n+\n+test_expect_success '1a-setup: Modify(A)/Modify(B), change on B subset of A' '\n+\ttest_create_repo 1a &&\n+\t(\n+\t\tcd 1a &&\n+\n+\t\ttest_write_lines 1 2 3 4 5 6 7 8 9 10 >b\n+\t\tgit add b &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"O\" &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit checkout A &&\n+\t\ttest_write_lines 1 2 3 4 5 5.5 6 7 8 9 10 10.5 >b &&\n+\t\tgit add b &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit checkout B &&\n+\t\ttest_write_lines 1 2 3 4 5 5.5 6 7 8 9 10 >b &&\n+\t\tgit add b &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"B\"\n+\t)\n+'\n+\n+test_expect_failure '1a-check-L: Modify(A)/Modify(B), change on B subset of A' '\n+\ttest_when_finished \"git -C 1a reset --hard\" &&\n+\t(\n+\t\tcd 1a &&\n+\n+\t\tgit checkout A^0 &&\n+\n+\t\tGIT_MERGE_VERBOSITY=3 git merge -s recursive B^0 >out 2>err &&\n+\n+\t\ttest_i18ngrep \"Skipped\" out &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\tgit ls-files -s >index_files &&\n+\t\ttest_line_count = 1 index_files &&\n+\n+\t\tgit rev-parse >actual HEAD:b &&\n+\t\tgit rev-parse >expect A:b &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit hash-object b   >actual &&\n+\t\tgit rev-parse   A:b >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '1a-check-R: Modify(A)/Modify(B), change on B subset of A' '\n+\ttest_when_finished \"git -C 1a reset --hard\" &&\n+\t(\n+\t\tcd 1a &&\n+\n+\t\tgit checkout B^0 &&\n+\n+\t\tGIT_MERGE_VERBOSITY=3 git merge -s recursive A^0 >out 2>err &&\n+\n+\t\ttest_i18ngrep \"Auto-merging\" out &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\tgit ls-files -s >index_files &&\n+\t\ttest_line_count = 1 index_files &&\n+\n+\t\tgit rev-parse >actual HEAD:b &&\n+\t\tgit rev-parse >expect A:b &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit hash-object b   >actual &&\n+\t\tgit rev-parse   A:b >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+###########################################################################\n+# SECTION 2: Cases involving basic renames\n+###########################################################################\n+\n+# Testcase 2a, Changes on A, rename on B\n+#   Commit O: b_1\n+#   Commit A: b_2\n+#   Commit B: c_1\n+#   Expected: c_2\n+\n+test_expect_success '2a-setup: Modify(A)/rename(B)' '\n+\ttest_create_repo 2a &&\n+\t(\n+\t\tcd 2a &&\n+\n+\t\ttest_seq 1 10 >b\n+\t\tgit add b &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"O\" &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit checkout A &&\n+\t\ttest_seq 1 11 > b &&\n+\t\tgit add b &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit checkout B &&\n+\t\tgit mv b c &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"B\"\n+\t)\n+'\n+\n+test_expect_success '2a-check-L: Modify/rename, merge into modify side' '\n+\ttest_when_finished \"git -C 2a reset --hard\" &&\n+\t(\n+\t\tcd 2a &&\n+\n+\t\tgit checkout A^0 &&\n+\n+\t\tGIT_MERGE_VERBOSITY=3 git merge -s recursive B^0 >out 2>err &&\n+\n+\t\ttest_i18ngrep \"Had correct contents\" out &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\tgit ls-files -s >index_files &&\n+\t\ttest_line_count = 1 index_files &&\n+\n+\t\tgit rev-parse >actual HEAD:c &&\n+\t\tgit rev-parse >expect A:b &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit hash-object c   >actual &&\n+\t\tgit rev-parse   A:b >expect &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\ttest_must_fail git rev-parse HEAD:b &&\n+\t\ttest_path_is_missing b\n+\t)\n+'\n+\n+test_expect_success '2a-check-R: Modify/rename, merge into rename side' '\n+\ttest_when_finished \"git -C 2a reset --hard\" &&\n+\t(\n+\t\tcd 2a &&\n+\n+\t\tgit checkout B^0 &&\n+\n+\t\tGIT_MERGE_VERBOSITY=3 git merge -s recursive A^0 >out 2>err &&\n+\n+\t\ttest_i18ngrep \"Auto-merging\" out &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\tgit ls-files -s >index_files &&\n+\t\ttest_line_count = 1 index_files &&\n+\n+\t\tgit rev-parse >actual HEAD:c &&\n+\t\tgit rev-parse >expect A:b &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit hash-object c   >actual &&\n+\t\tgit rev-parse   A:b >expect &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\ttest_must_fail git rev-parse HEAD:b &&\n+\t\ttest_path_is_missing b\n+\t)\n+'\n+\n+# Testcase 2b, Changed and renamed on A, subset of changes on B\n+#   Commit O: b_1\n+#   Commit A: c_2\n+#   Commit B: b_3\n+#   Expected: c_2\n+\n+test_expect_success '2b-setup: Modify(A)/Modify(B), change on B subset of A' '\n+\ttest_create_repo 2b &&\n+\t(\n+\t\tcd 2b &&\n+\n+\t\ttest_write_lines 1 2 3 4 5 6 7 8 9 10 >b\n+\t\tgit add b &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"O\" &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit checkout A &&\n+\t\ttest_write_lines 1 2 3 4 5 5.5 6 7 8 9 10 10.5 >b &&\n+\t\tgit add b &&\n+\t\tgit mv b c &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit checkout B &&\n+\t\ttest_write_lines 1 2 3 4 5 5.5 6 7 8 9 10 >b &&\n+\t\tgit add b &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"B\"\n+\t)\n+'\n+\n+test_expect_success '2b-check-L: Modify(A)/Modify(B), change on B subset of A' '\n+\ttest_when_finished \"git -C 2b reset --hard\" &&\n+\t(\n+\t\tcd 2b &&\n+\n+\t\tgit checkout A^0 &&\n+\n+\t\tGIT_MERGE_VERBOSITY=3 git merge -s recursive B^0 >out 2>err &&\n+\n+\t\ttest_i18ngrep \"Skipped\" out &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\tgit ls-files -s >index_files &&\n+\t\ttest_line_count = 1 index_files &&\n+\n+\t\tgit rev-parse >actual HEAD:c &&\n+\t\tgit rev-parse >expect A:c &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit hash-object c   >actual &&\n+\t\tgit rev-parse   A:c >expect &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\ttest_must_fail git rev-parse HEAD:b &&\n+\t\ttest_path_is_missing b\n+\t)\n+'\n+\n+test_expect_success '2b-check-R: Modify(A)/Modify(B), change on B subset of A' '\n+\ttest_when_finished \"git -C 2b reset --hard\" &&\n+\t(\n+\t\tcd 2b &&\n+\n+\t\tgit checkout B^0 &&\n+\n+\t\tGIT_MERGE_VERBOSITY=3 git merge -s recursive A^0 >out 2>err &&\n+\n+\t\ttest_i18ngrep \"Auto-merging\" out &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\tgit ls-files -s >index_files &&\n+\t\ttest_line_count = 1 index_files &&\n+\n+\t\tgit rev-parse >actual HEAD:c &&\n+\t\tgit rev-parse >expect A:c &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit hash-object c   >actual &&\n+\t\tgit rev-parse   A:c >expect &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\ttest_must_fail git rev-parse HEAD:b &&\n+\t\ttest_path_is_missing b\n+\t)\n+'\n+\n+###########################################################################\n+# SECTION 3: Cases involving directory renames\n+#\n+# NOTE:\n+#   Directory renames only apply when one side renames a directory, and the\n+#   other side adds or renames a path into that directory.  Applying the\n+#   directory rename to that new path creates a new pathname that didn't\n+#   exist on either side of history.  Thus, it is impossible for the\n+#   merge contents to already be at the right path, so all of these checks\n+#   exist just to make sure that updates are not skipped.\n+###########################################################################\n+\n+# Testcase 3a, Change + rename into dir foo on A, dir rename foo->bar on B\n+#   Commit O: bq_1, foo/whatever\n+#   Commit A: foo/{bq_2, whatever}\n+#   Commit B: bq_1, bar/whatever\n+#   Expected: bar/{bq_2, whatever}\n+\n+test_expect_success '3a-setup: bq_1->foo/bq_2 on A, foo/->bar/ on B' '\n+\ttest_create_repo 3a &&\n+\t(\n+\t\tcd 3a &&\n+\n+\t\tmkdir foo &&\n+\t\ttest_seq 1 10 >bq &&\n+\t\ttest_write_lines a b c d e f g h i j k >foo/whatever &&\n+\t\tgit add bq foo/whatever &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"O\" &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit checkout A &&\n+\t\ttest_seq 1 11 > bq &&\n+\t\tgit add bq &&\n+\t\tgit mv bq foo/ &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit checkout B &&\n+\t\tgit mv foo/ bar/ &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"B\"\n+\t)\n+'\n+\n+test_expect_failure '3a-check-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' '\n+\ttest_when_finished \"git -C 3a reset --hard\" &&\n+\t(\n+\t\tcd 3a &&\n+\n+\t\tgit checkout A^0 &&\n+\n+\t\tGIT_MERGE_VERBOSITY=3 git merge -s recursive B^0 >out 2>err &&\n+\n+\t\ttest_i18ngrep \"Had correct contents for bar/bq\" out &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\tgit ls-files -s >index_files &&\n+\t\ttest_line_count = 2 index_files &&\n+\n+\t\tgit rev-parse >actual HEAD:bar/bq HEAD:bar/whatever &&\n+\t\tgit rev-parse >expect A:foo/bq    A:foo/whatever &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit hash-object bar/bq   bar/whatever   >actual &&\n+\t\tgit rev-parse   A:foo/bq A:foo/whatever >expect &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\ttest_must_fail git rev-parse HEAD:bq HEAD:foo/bq &&\n+\t\ttest_path_is_missing bq foo/bq foo/whatever\n+\t)\n+'\n+\n+test_expect_success '3a-check-R: bq_1->foo/bq_2 on A, foo/->bar/ on B' '\n+\ttest_when_finished \"git -C 3a reset --hard\" &&\n+\t(\n+\t\tcd 3a &&\n+\n+\t\tgit checkout B^0 &&\n+\n+\t\tGIT_MERGE_VERBOSITY=3 git merge -s recursive A^0 >out 2>err &&\n+\n+\t\ttest_i18ngrep \"Auto-merging bar/bq\" out &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\tgit ls-files -s >index_files &&\n+\t\ttest_line_count = 2 index_files &&\n+\n+\t\tgit rev-parse >actual HEAD:bar/bq HEAD:bar/whatever &&\n+\t\tgit rev-parse >expect A:foo/bq    A:foo/whatever &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit hash-object bar/bq   bar/whatever   >actual &&\n+\t\tgit rev-parse   A:foo/bq A:foo/whatever >expect &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\ttest_must_fail git rev-parse HEAD:bq HEAD:foo/bq &&\n+\t\ttest_path_is_missing bq foo/bq foo/whatever\n+\t)\n+'\n+\n+# Testcase 3b, rename into dir foo on A, dir rename foo->bar + change on B\n+#   Commit O: bq_1, foo/whatever\n+#   Commit A: foo/{bq_1, whatever}\n+#   Commit B: bq_2, bar/whatever\n+#   Expected: bar/{bq_2, whatever}\n+\n+test_expect_success '3b-setup: bq_1->foo/bq_2 on A, foo/->bar/ on B' '\n+\ttest_create_repo 3b &&\n+\t(\n+\t\tcd 3b &&\n+\n+\t\tmkdir foo &&\n+\t\ttest_seq 1 10 >bq &&\n+\t\ttest_write_lines a b c d e f g h i j k >foo/whatever &&\n+\t\tgit add bq foo/whatever &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"O\" &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit checkout A &&\n+\t\tgit mv bq foo/ &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit checkout B &&\n+\t\ttest_seq 1 11 > bq &&\n+\t\tgit add bq &&\n+\t\tgit mv foo/ bar/ &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"B\"\n+\t)\n+'\n+\n+test_expect_success '3b-check-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' '\n+\ttest_when_finished \"git -C 3b reset --hard\" &&\n+\t(\n+\t\tcd 3b &&\n+\n+\t\tgit checkout A^0 &&\n+\n+\t\tGIT_MERGE_VERBOSITY=3 git merge -s recursive B^0 >out 2>err &&\n+\n+\t\ttest_i18ngrep \"Auto-merging bar/bq\" out &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\tgit ls-files -s >index_files &&\n+\t\ttest_line_count = 2 index_files &&\n+\n+\t\tgit rev-parse >actual HEAD:bar/bq HEAD:bar/whatever &&\n+\t\tgit rev-parse >expect B:bq        A:foo/whatever &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit hash-object bar/bq bar/whatever   >actual &&\n+\t\tgit rev-parse   B:bq   A:foo/whatever >expect &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\ttest_must_fail git rev-parse HEAD:bq HEAD:foo/bq &&\n+\t\ttest_path_is_missing bq foo/bq foo/whatever\n+\t)\n+'\n+\n+test_expect_success '3b-check-R: bq_1->foo/bq_2 on A, foo/->bar/ on B' '\n+\ttest_when_finished \"git -C 3b reset --hard\" &&\n+\t(\n+\t\tcd 3b &&\n+\n+\t\tgit checkout B^0 &&\n+\n+\t\tGIT_MERGE_VERBOSITY=3 git merge -s recursive A^0 >out 2>err &&\n+\n+\t\ttest_i18ngrep \"Had correct contents for bar/bq\" out &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\tgit ls-files -s >index_files &&\n+\t\ttest_line_count = 2 index_files &&\n+\n+\t\tgit rev-parse >actual HEAD:bar/bq HEAD:bar/whatever &&\n+\t\tgit rev-parse >expect B:bq        A:foo/whatever &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit hash-object bar/bq bar/whatever   >actual &&\n+\t\tgit rev-parse   B:bq   A:foo/whatever >expect &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\ttest_must_fail git rev-parse HEAD:bq HEAD:foo/bq &&\n+\t\ttest_path_is_missing bq foo/bq foo/whatever\n+\t)\n+'\n+\n+test_done\n-- \n2.16.0.35.g6dd7ede834\n\n"},{"id":"344634","messageId":"20180413195607.18091-2-newren@gmail.com","threadId":"48266","inReplyTo":"20180413195607.18091-1-newren@gmail.com","subject":"[PATCH v9 29.25/30] merge-recursive: improve output precision around skipping updates","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-13T19:56:04Z","receivedAt":"2018-04-13T19:56:32Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"When a merge results in contents that already existed in HEAD (because the\nchanges on a side branch were a subset of what was changed by HEAD), and\nthose contents were already at the right path, the working directory\nupdates can be skipped.  However, the relevant code would sometimes claim\nthe working directory update could be skipped despite the fact that the\ncontents were at the wrong path.  Make the output reflect the situation\nmore precisely, including an additional message that tests can check for\nto make sure we are getting correct behavior.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n merge-recursive.c       | 11 ++++++-----\n t/t6022-merge-rename.sh |  2 +-\n 2 files changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 4a1ecdea03..05250939c7 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2761,21 +2761,22 @@ static int merge_content(struct merge_options *o,\n \t\t\t\t       o->branch2, path2, &mfi))\n \t\treturn -1;\n \n-\tif (mfi.clean && !df_conflict_remains &&\n-\t    oid_eq(&mfi.oid, a_oid) && mfi.mode == a_mode) {\n+\tif (mfi.clean && oid_eq(&mfi.oid, a_oid) && mfi.mode == a_mode) {\n \t\tint path_renamed_outside_HEAD;\n-\t\toutput(o, 3, _(\"Skipped %s (merged same as existing)\"), path);\n \t\t/*\n \t\t * The content merge resulted in the same file contents we\n \t\t * already had.  We can return early if those file contents\n \t\t * are recorded at the correct path (which may not be true\n-\t\t * if the merge involves a rename).\n+\t\t * if the merge involves a rename or there's a D/F conflict).\n \t\t */\n \t\tpath_renamed_outside_HEAD = !path2 || !strcmp(path, path2);\n-\t\tif (!path_renamed_outside_HEAD) {\n+\t\tif (!df_conflict_remains && !path_renamed_outside_HEAD) {\n+\t\t\toutput(o, 3, _(\"Skipped %s (merged same as existing)\"), path);\n \t\t\tadd_cacheinfo(o, mfi.mode, &mfi.oid, path,\n \t\t\t\t      0, (!o->call_depth), 0);\n \t\t\treturn mfi.clean;\n+\t\t} else {\n+\t\t\toutput(o, 3, _(\"Had correct contents for %s, but not at right path\"), path);\n \t\t}\n \t} else\n \t\toutput(o, 2, _(\"Auto-merging %s\"), path);\ndiff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh\nindex 05ebba7afa..574088bfaf 100755\n--- a/t/t6022-merge-rename.sh\n+++ b/t/t6022-merge-rename.sh\n@@ -245,7 +245,7 @@ test_expect_success 'merge of identical changes in a renamed file' '\n \tGIT_MERGE_VERBOSITY=3 git merge change | test_i18ngrep \"^Skipped B\" &&\n \tgit reset --hard HEAD^ &&\n \tgit checkout change &&\n-\tGIT_MERGE_VERBOSITY=3 git merge change+rename | test_i18ngrep \"^Skipped B\"\n+\tGIT_MERGE_VERBOSITY=3 git merge change+rename | test_i18ngrep \"^Had correct contents for B, but not at right path\"\n '\n \n test_expect_success 'setup for rename + d/f conflicts' '\n-- \n2.16.0.35.g6dd7ede834\n\n"},{"id":"344637","messageId":"CAGZ79kaWnyuQXr8bHjrKL2xs+h-DQL8hekGJX8-prW+JY8kwUw@mail.gmail.com","threadId":"48266","inReplyTo":"20180413195607.18091-3-newren@gmail.com","subject":"Re: [PATCH v9 29.50/30] t6046: testcases checking whether updates can be skipped in a merge","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-13T21:03:03Z","receivedAt":"2018-04-13T21:03:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Apr 13, 2018 at 12:56 PM, Elijah Newren <newren@gmail.com> wrote:\n> Add several tests checking whether updates can be skipped in a merge.\n> Also add several similar testcases for where updates cannot be skipped in\n> a merge to make sure that we skip if and only if we should.\n>\n> In particular:\n>\n>   * Testcase 1a (particularly 1a-check-L) would have pointed out the\n>     problem Linus has been dealing with for year with his merges[1].\n>\n>   * Testcase 2a (particularly 2a-check-L) would have pointed out the\n>     problem with my directory-rename-series before it broke master[2].\n>\n>   * Testcases 3[ab] (particularly 3a-check-L) provide a simpler testcase\n>     than 12b of t6043 making that one easier to understand.\n>\n>   * There are several complementary testcases to make sure we're not just\n>     fixing those particular issues while regressing in the opposite\n>     direction.\n>\n> [1] https://public-inbox.org/git/CA+55aFzLZ3UkG5svqZwSnhNk75=fXJRkvU1m_RHBG54NOoaZPA@mail.gmail.com/\n> [2] https://public-inbox.org/git/xmqqmuya43cs.fsf@gitster-ct.c.googlers.com/\n>\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n\nThis is\nReviewed-by: Stefan Beller <sbeller@google.com>\n\n\n> +               test_tick &&\n> +               git commit -m \"O\" &&\n\nminor nit: I think you can omit the test_tick before creating O\nfor each setup.\n\nStefan\n"},{"id":"344763","messageId":"xmqqo9ikyojz.fsf@gitster-ct.c.googlers.com","threadId":"48266","inReplyTo":"20180413195607.18091-4-newren@gmail.com","subject":"Re: [PATCH v9 29.75/30] merge-recursive: Fix was_tracked() to quit lying with some renamed paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-16T00:37:04Z","receivedAt":"2018-04-16T00:37:14Z","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> @@ -362,13 +363,17 @@ static int git_merge_trees(struct merge_options *o,\n>  \tinit_tree_desc_from_tree(t+2, merge);\n>  \n>  \trc = unpack_trees(3, t, &o->unpack_opts);\n> +\tcache_tree_free(&active_cache_tree);\n> +\n> +\to->orig_index = the_index;\n> +\tthe_index = tmp_index;\n> +\n>  \t/*\n> -\t * unpack_trees NULLifies src_index, but it's used in verify_uptodate,\n> -\t * so set to the new index which will usually have modification\n> -\t * timestamp info copied over.\n> +\t * src_index is used in verify_uptodate, but was NULLified in\n> +\t * unpack_trees, so we need to set it back to the original index.\n>  \t */\n\nWas NULLified?  I thought that the point of src/dst distinction\nLinus introduced long time ago at 34110cd4 (\"Make 'unpack_trees()'\nhave a separate source and destination index\", 2008-03-06) was that\nwe can then keep the source side of the traversal unmodified.\n\n> -\to->unpack_opts.src_index = &the_index;\n> -\tcache_tree_free(&active_cache_tree);\n> +\to->unpack_opts.src_index = &o->orig_index;\n\n> -static int was_tracked(const char *path)\n> +/*\n> + * Returns whether path was tracked in the index before the merge started\n> + */\n> +static int was_tracked(struct merge_options *o, const char *path)\n>  {\n> -\tint pos = cache_name_pos(path, strlen(path));\n> +\tint pos = index_name_pos(&o->orig_index, path, strlen(path));\n>  \n>  \tif (0 <= pos)\n> -\t\t/* we have been tracking this path */\n> +\t\t/* we were tracking this path before the merge */\n>  \t\treturn 1;\n>  \n> -\t/*\n> -\t * Look for an unmerged entry for the path,\n> -\t * specifically stage #2, which would indicate\n> -\t * that \"our\" side before the merge started\n> -\t * had the path tracked (and resulted in a conflict).\n> -\t */\n> -\tfor (pos = -1 - pos;\n> -\t     pos < active_nr && !strcmp(path, active_cache[pos]->name);\n> -\t     pos++)\n> -\t\tif (ce_stage(active_cache[pos]) == 2)\n> -\t\t\treturn 1;\n>  \treturn 0;\n>  }\n\nI do agree with the simplicity of the new code that directly asks\nexactly what we want to ask.  However, there is one thing that is\npuzzling below...\n\n>  static int would_lose_untracked(const char *path)\n>  {\n> -\treturn !was_tracked(path) && file_exists(path);\n> +\t/*\n> +\t * This may look like it can be simplified to:\n> +\t *   return !was_tracked(o, path) && file_exists(path)\n> +\t * but it can't.  This function needs to know whether path was\n> +\t * in the working tree due to EITHER having been tracked in the\n> +\t * index before the merge OR having been put into the working copy\n> +\t * and index by unpack_trees().  Due to that either-or requirement,\n> +\t * we check the current index instead of the original one.\n> +\t */\n\nIf this path was created by merge-recursive, not by unpack_trees(),\nwhat does this function want to say?  Say, we are looking at path P,\nthe other branch we are merging moved some other path Q to P (while\nour side modified contents at path Q).  Then path P we are looking\nat has contents of Q at the merge base at stage #1, the contents of\nQ from our HEAD at stage #2 and the contents of P from the other\nbranch at stage #3.  The code below says \"path P is OK, we won't\nlose it\" in such a case, but it is unclear if the above comment\nwants to also cover that case.\n\n> +\tint pos = cache_name_pos(path, strlen(path));\n> +\n> +\tif (pos < 0)\n> +\t\tpos = -1 - pos;\n> +\twhile (pos < active_nr &&\n> +\t       !strcmp(path, active_cache[pos]->name)) {\n> +\t\t/*\n> +\t\t * If stage #0, it is definitely tracked.\n> +\t\t * If it has stage #2 then it was tracked\n> +\t\t * before this merge started.  All other\n> +\t\t * cases the path was not tracked.\n> +\t\t */\n> +\t\tswitch (ce_stage(active_cache[pos])) {\n> +\t\tcase 0:\n> +\t\tcase 2:\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t\tpos++;\n> +\t}\n> +\treturn file_exists(path);\n>  }\n>  \n>  static int was_dirty(struct merge_options *o, const char *path)\n"},{"id":"344821","messageId":"CABPp-BFPTJsTUVoPxxN=2u5jEqn1ngdDvMNhp+VLZKTgZaUkvw@mail.gmail.com","threadId":"48266","inReplyTo":"xmqqo9ikyojz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v9 29.75/30] merge-recursive: Fix was_tracked() to quit lying with some renamed paths","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-04-16T21:21:08Z","receivedAt":"2018-04-16T21:21:15Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Apr 15, 2018 at 5:37 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Elijah Newren <newren@gmail.com> writes:\n>\n>> @@ -362,13 +363,17 @@ static int git_merge_trees(struct merge_options *o,\n>>       init_tree_desc_from_tree(t+2, merge);\n>>\n>>       rc = unpack_trees(3, t, &o->unpack_opts);\n>> +     cache_tree_free(&active_cache_tree);\n>> +\n>> +     o->orig_index = the_index;\n>> +     the_index = tmp_index;\n>> +\n>>       /*\n>> -      * unpack_trees NULLifies src_index, but it's used in verify_uptodate,\n>> -      * so set to the new index which will usually have modification\n>> -      * timestamp info copied over.\n>> +      * src_index is used in verify_uptodate, but was NULLified in\n>> +      * unpack_trees, so we need to set it back to the original index.\n>>        */\n>\n> Was NULLified?  I thought that the point of src/dst distinction\n> Linus introduced long time ago at 34110cd4 (\"Make 'unpack_trees()'\n> have a separate source and destination index\", 2008-03-06) was that\n> we can then keep the source side of the traversal unmodified.\n\nThat comment is messed up; maybe I edited and re-edited the comment\nmultiple times and then didn't notice the big problems when\nre-reading?\n\nAnyway, I should move the comment a few lines up, and make the code\ninstead read:\n\n    /*\n     * Update the_index to match the new results, AFTER saving a copy\n     * in o->orig_index.  Update src_index to point to the saved copy.\n     * (verify_uptodate() checks src_index, and the original index is\n     * the one that had the necessary modification timestamps.)\n     */\n    o->orig_index = the_index;\n    the_index = tmp_index;\n    o->unpack_opts.src_index = &o->orig_index;\n\n>>  static int would_lose_untracked(const char *path)\n>>  {\n>> -     return !was_tracked(path) && file_exists(path);\n>> +     /*\n>> +      * This may look like it can be simplified to:\n>> +      *   return !was_tracked(o, path) && file_exists(path)\n>> +      * but it can't.  This function needs to know whether path was\n>> +      * in the working tree due to EITHER having been tracked in the\n>> +      * index before the merge OR having been put into the working copy\n>> +      * and index by unpack_trees().  Due to that either-or requirement,\n>> +      * we check the current index instead of the original one.\n>> +      */\n>\n> If this path was created by merge-recursive, not by unpack_trees(),\n> what does this function want to say?  Say, we are looking at path P,\n> the other branch we are merging moved some other path Q to P (while\n> our side modified contents at path Q).  Then path P we are looking\n> at has contents of Q at the merge base at stage #1, the contents of\n> Q from our HEAD at stage #2 and the contents of P from the other\n> branch at stage #3.  The code below says \"path P is OK, we won't\n> lose it\" in such a case, but it is unclear if the above comment\n> wants to also cover that case.\n\nOh, boy, here be dragons...\n\nThe comment as-is actually does cover your example case with Q and P:\nunpack_trees(), which is unaware of renames, will see that P only\nexists on one side of history and thus load it into the index at stage\n0 rather than stage 3.\n\nBut your general comment about whether something else in\nmerge-recursive could create a path in the current index after\nunpack_trees() is interesting...it touches on a pitfall that has bit\nme multiple times.  There is a required ordering in merge-recursive.c\nthat for any given path, the working directory must be updated before\nthe index is -- otherwise, would_lose_untracked() will return faulty\ninformation.  update_file_flags() has this ordering builtin,\nupdate_stages() has a big obnoxious comment at the beginning about how\nit should not be called until after update_file() is,\napply_directory_rename_modifications() has a big comment about this\n~80% of the way through the function (look for\n\"would_lose_untracked\"), and conflict_rename_rename_2to1() has a big\nobnoxious comment near the end painstakingly pointing out that some\ncode that feels like it would make more sense being combined with a\nprevious function cannot be due to the\nupdate-working-directory-before-index requirement.\n\nI should probably add to this comment something about this annoying\n(and error-prone) ordering restriction, since this function is the\nsource of those particular pains.  Your suggested ideal-world rewrite\n(run unpack_trees() with unpack_opts.index_only=1, do merge in memory,\nthen update working tree), would make this whole problem go away.\n"}]}