{"thread":{"id":"26628","subject":"[PATCHv2 0/3] Fix unnecessary (mtime) updates of files during merge","startedAt":"2011-03-01T01:08:49Z","lastAt":"2011-03-02T23:23:53Z","messageCount":14,"participants":["Elijah Newren","Johannes Sixt","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"162502","messageId":"1298941732-19763-1-git-send-email-newren@gmail.com","threadId":"26628","inReplyTo":null,"subject":"[PATCHv2 0/3] Fix unnecessary (mtime) updates of files during merge","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2011-03-01T01:08:49Z","receivedAt":"2011-03-01T01:08:49Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This patch series fixes a bug reported by Stephen Rothwell -- that\nduring merges git would unnecessarily update modification times of\nfiles.\n\nThere are two testcases included in this patch series.  The first is a\nsimple case to test the originally reported bug; this testcase is\nfixed in this series (as is Stephen's original linux-next testcase).\nThe second testcase suffers from the exact same problem, but arises\nfrom a different situation and is not fixed in this series.  That\ntestcase is slightly harder to solve because:\n\n  * unpack_trees + threeway_merge throws away the original index entry\n    with stat information when it notices the directory/file conflict\n\n  * make_room_for_directories_of_df_conflicts() must remove such files\n    from the working copy or the corresponding directory and files\n    below it will be unable to be written to the working copy (which\n    can cause spurious conflicts, or make resolving conflicts very\n    hard for users who don't know how to access the many files missing\n    from their working copy).\n\nWe could fix this second testcase by recording stat information for\nfiles removed by make_room_for_directories_of_df_conflicts(), and\nthen, if those files are reinstated at the end of conflict resolution\n(i.e. the directory of the D/F conflict went away during the merge),\nthen call utime() to reset the modification times on those files back\nto what they originally were.\n\n(Technically, the second testcase that I left unfixed is not a\nregression; prior versions of git would fail the merge and record the\nfile's content in an alternative file to \"avoid\" the directory it\nthought was in the way, so the modification time was the least of the\nworries.)\n\n\nElijah Newren (3):\n  t6022: New test checking for unnecessary updates of renamed+modified files\n  t6022: New test checking for unnecessary updates of files in D/F conflicts\n  merge-recursive: When we detect we can skip an update, actually skip it\n\n merge-recursive.c       |   17 +++++++----\n t/t6022-merge-rename.sh |   68 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 79 insertions(+), 6 deletions(-)\n\n-- \n1.7.4\n"},{"id":"162503","messageId":"1298941732-19763-2-git-send-email-newren@gmail.com","threadId":"26628","inReplyTo":"1298941732-19763-1-git-send-email-newren@gmail.com","subject":"[PATCHv2 1/3] t6022: New test checking for unnecessary updates of renamed+modified files","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2011-03-01T01:08:50Z","receivedAt":"2011-03-01T01:08:50Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t6022-merge-rename.sh |   32 ++++++++++++++++++++++++++++++++\n 1 files changed, 32 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh\nindex 1ed259d..3667e18 100755\n--- a/t/t6022-merge-rename.sh\n+++ b/t/t6022-merge-rename.sh\n@@ -609,4 +609,36 @@ test_expect_success 'check handling of differently renamed file with D/F conflic\n \t! test -f original\n '\n \n+test_expect_success 'setup avoid unnecessary update, normal rename' '\n+\tgit reset --hard &&\n+\tgit checkout --orphan avoid-unnecessary-update-1 &&\n+\tgit rm -rf . &&\n+\tgit clean -fdqx &&\n+\n+\tprintf \"1\\n2\\n3\\n4\\n5\\n6\\n7\\n8\\n9\\n10\\n\" >original &&\n+\tgit add -A &&\n+\tgit commit -m \"Common commmit\" &&\n+\n+\tgit mv original rename &&\n+\techo 11 >>rename &&\n+\tgit add -u &&\n+\tgit commit -m \"Renamed and modified\" &&\n+\n+\tgit checkout -b merge-branch-1 HEAD~1 &&\n+\techo \"random content\" >random-file &&\n+\tgit add -A &&\n+\tgit commit -m \"Random, unrelated changes\"\n+'\n+\n+test_expect_failure 'avoid unnecessary update, normal rename' '\n+\tgit checkout -q avoid-unnecessary-update-1^0 &&\n+\ttouch -t 197001010000.01 rename &&\n+\torig=$(stat --format=\"%Y\" rename) &&\n+\tgit merge merge-branch-1 &&\n+\tnew=$(stat --format=\"%Y\" rename) &&\n+\techo \"Checking whether stat times are same: $orig vs $new\" &&\n+\ttest \"$orig\" == \"$new\" &&\n+\tgit diff-files --exit-code # Is \"rename\" clean, or only racily clean?\n+'\n+\n test_done\n-- \n1.7.4\n"},{"id":"162504","messageId":"1298941732-19763-3-git-send-email-newren@gmail.com","threadId":"26628","inReplyTo":"1298941732-19763-1-git-send-email-newren@gmail.com","subject":"[PATCHv2 2/3] t6022: New test checking for unnecessary updates of files in D/F conflicts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2011-03-01T01:08:51Z","receivedAt":"2011-03-01T01:08:51Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t6022-merge-rename.sh |   36 ++++++++++++++++++++++++++++++++++++\n 1 files changed, 36 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh\nindex 3667e18..a5d252b 100755\n--- a/t/t6022-merge-rename.sh\n+++ b/t/t6022-merge-rename.sh\n@@ -641,4 +641,40 @@ test_expect_failure 'avoid unnecessary update, normal rename' '\n \tgit diff-files --exit-code # Is \"rename\" clean, or only racily clean?\n '\n \n+test_expect_success 'setup avoid unnecessary update, with D/F conflict' '\n+\tgit reset --hard &&\n+\tgit checkout --orphan avoid-unnecessary-update-2 &&\n+\tgit rm -rf . &&\n+\tgit clean -fdqx &&\n+\n+\tmkdir df &&\n+\tprintf \"1\\n2\\n3\\n4\\n5\\n6\\n7\\n8\\n9\\n10\\n\" >df/file &&\n+\tgit add -A &&\n+\tgit commit -m \"Common commmit\" &&\n+\n+\tgit mv df/file temp &&\n+\trm -rf df &&\n+\tgit mv temp df &&\n+\techo 11 >>df &&\n+\tgit add -u &&\n+\tgit commit -m \"Renamed and modified\" &&\n+\n+\tgit checkout -b merge-branch-2 HEAD~1 &&\n+\t>unrelated-change &&\n+\tgit add unrelated-change &&\n+\tgit commit -m \"Only unrelated changes\"\n+'\n+\n+test_expect_failure 'avoid unnecessary update, with D/F conflict' '\n+\tgit checkout -q avoid-unnecessary-update-2^0 &&\n+\ttouch -t 197001010000.01 df &&\n+\torig=$(stat --format=\"%Y\" df) &&\n+\tgit merge merge-branch-2 &&\n+\tnew=$(stat --format=\"%Y\" df) &&\n+\techo \"Checking whether stat times are same: $orig vs $new\" &&\n+\ttest \"$orig\" == \"$new\" &&\n+\tgit diff-files --exit-code # Is \"rename\" clean, or only racily clean?\n+'\n+\n+\n test_done\n-- \n1.7.4\n"},{"id":"162505","messageId":"1298941732-19763-4-git-send-email-newren@gmail.com","threadId":"26628","inReplyTo":"1298941732-19763-1-git-send-email-newren@gmail.com","subject":"[PATCHv2 3/3] merge-recursive: When we detect we can skip an update, actually skip it","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2011-03-01T01:08:52Z","receivedAt":"2011-03-01T01:08:52Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"In 882fd11 (merge-recursive: Delay content merging for renames 2010-09-20),\nthere was code that checked for whether we could skip updating a file in\nthe working directory, based on whether the merged version matched the\ncurrent working copy.  Due to the desire to handle directory/file conflicts\nthat were resolvable, that commit deferred content merging by first\nupdating the index with the unmerged entries and then moving the actual\nmerging (along with the skip-the-content-update check) to another function\nthat ran later in the merge process.  As part moving the content merging\ncode, a bug was introduced such that although the message about skipping\nthe update would be printed (whenever GIT_MERGE_VERBOSITY was sufficiently\nhigh), the file would be unconditionally updated in the working copy\nanyway.\n\nWhen we detect that the file does not need to be updated in the working\ncopy, update the index appropriately and then return early before updating\nthe working copy.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n merge-recursive.c       |   17 +++++++++++------\n t/t6022-merge-rename.sh |    2 +-\n 2 files changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 16c2dbe..ec07a30 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -354,10 +354,11 @@ static void make_room_for_directories_of_df_conflicts(struct merge_options *o,\n \t * make room for the corresponding directory.  Such paths will\n \t * later be processed in process_df_entry() at the end.  If\n \t * the corresponding directory ends up being removed by the\n-\t * merge, then the file will be reinstated at that time;\n-\t * otherwise, if the file is not supposed to be removed by the\n-\t * merge, the contents of the file will be placed in another\n-\t * unique filename.\n+\t * merge, then the file will be reinstated at that time\n+\t * (albeit with a different timestamp!); otherwise, if the\n+\t * file is not supposed to be removed by the merge, the\n+\t * contents of the file will be placed in another unique\n+\t * filename.\n \t *\n \t * NOTE: This function relies on the fact that entries for a\n \t * D/F conflict will appear adjacent in the index, with the\n@@ -1274,9 +1275,13 @@ static int merge_content(struct merge_options *o,\n \t}\n \n \tif (mfi.clean && !df_conflict_remains &&\n-\t    sha_eq(mfi.sha, a_sha) && mfi.mode == a.mode)\n+\t    sha_eq(mfi.sha, a_sha) && mfi.mode == a.mode &&\n+\t    lstat(path, &st) == 0) {\n \t\toutput(o, 3, \"Skipped %s (merged same as existing)\", path);\n-\telse\n+\t\tadd_cacheinfo(mfi.mode, mfi.sha, path,\n+\t\t\t      0 /*stage*/, 1 /*refresh*/, 0 /*options*/);\n+\t\treturn mfi.clean;\n+\t} else\n \t\toutput(o, 2, \"Auto-merging %s\", path);\n \n \tif (!mfi.clean) {\ndiff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh\nindex a5d252b..a44117a 100755\n--- a/t/t6022-merge-rename.sh\n+++ b/t/t6022-merge-rename.sh\n@@ -630,7 +630,7 @@ test_expect_success 'setup avoid unnecessary update, normal rename' '\n \tgit commit -m \"Random, unrelated changes\"\n '\n \n-test_expect_failure 'avoid unnecessary update, normal rename' '\n+test_expect_success 'avoid unnecessary update, normal rename' '\n \tgit checkout -q avoid-unnecessary-update-1^0 &&\n \ttouch -t 197001010000.01 rename &&\n \torig=$(stat --format=\"%Y\" rename) &&\n-- \n1.7.4\n"},{"id":"162514","messageId":"4D6CA13C.1000006@viscovery.net","threadId":"26628","inReplyTo":"1298941732-19763-2-git-send-email-newren@gmail.com","subject":"Re: [PATCHv2 1/3] t6022: New test checking for unnecessary updates of renamed+modified files","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-03-01T07:33:16Z","receivedAt":"2011-03-01T07:33:16Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/1/2011 2:08, schrieb Elijah Newren:\n> +test_expect_failure 'avoid unnecessary update, normal rename' '\n> +\tgit checkout -q avoid-unnecessary-update-1^0 &&\n> +\ttouch -t 197001010000.01 rename &&\n\nUse\n\ttest-chmtime =1000000000 rename &&\n\n> +\torig=$(stat --format=\"%Y\" rename) &&\n\n\torig=$(test-chmtime -v +0 rename) &&\n\n> +\tgit merge merge-branch-1 &&\n> +\tnew=$(stat --format=\"%Y\" rename) &&\n\n\tnew=$(test-chmtime -v +0 rename) &&\n\n> +\techo \"Checking whether stat times are same: $orig vs $new\" &&\n\n\techo \"Checking whether stat times are same: ${orig%%\t*} vs ${new%%\t*}\" &&\n\n(that's TAB after the %%)\n\n> +\ttest \"$orig\" == \"$new\" &&\n\n\ttest \"${orig%%\t*}\" = \"${new%%\t*}\" &&\n\n== is not portable. Actually, since the file name is the same in both\n$orig and $new, you wouldn't need the %% magic in this statement.\n\n> +\tgit diff-files --exit-code # Is \"rename\" clean, or only racily clean?\n\nWhat you see here, is not \"racily clean\", but \"stat dirty\". \"Racily clean\"\nmeans that git thinks that the file is clean (because the stat information\nmatches), but in fact the file content is not identical to the content\nrecorded in the index.\n\n> +'\n> +\n>  test_done\n\n-- Hannes\n"},{"id":"162568","messageId":"20110301193142.GA10082@sigill.intra.peff.net","threadId":"26628","inReplyTo":"1298941732-19763-1-git-send-email-newren@gmail.com","subject":"Re: [PATCHv2 0/3] Fix unnecessary (mtime) updates of files during merge","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-01T19:31:42Z","receivedAt":"2011-03-01T19:31:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 28, 2011 at 06:08:49PM -0700, Elijah Newren wrote:\n\n> This patch series fixes a bug reported by Stephen Rothwell -- that\n> during merges git would unnecessarily update modification times of\n> files.\n\nThanks for the fix. Sorry I didn't have a chance to look at your interim\npatch, but it seems you resolved the issue.\n\n> There are two testcases included in this patch series.  The first is a\n> simple case to test the originally reported bug; this testcase is\n> fixed in this series (as is Stephen's original linux-next testcase).\n> The second testcase suffers from the exact same problem, but arises\n> from a different situation and is not fixed in this series.  That\n> testcase is slightly harder to solve because:\n> \n>   * unpack_trees + threeway_merge throws away the original index entry\n>     with stat information when it notices the directory/file conflict\n> \n>   * make_room_for_directories_of_df_conflicts() must remove such files\n>     from the working copy or the corresponding directory and files\n>     below it will be unable to be written to the working copy (which\n>     can cause spurious conflicts, or make resolving conflicts very\n>     hard for users who don't know how to access the many files missing\n>     from their working copy).\n> \n> We could fix this second testcase by recording stat information for\n> files removed by make_room_for_directories_of_df_conflicts(), and\n> then, if those files are reinstated at the end of conflict resolution\n> (i.e. the directory of the D/F conflict went away during the merge),\n> then call utime() to reset the modification times on those files back\n> to what they originally were.\n\nAs you mention, this second testcase is not a regression at all, and\nwhile it would be great if we could avoid touching those files at all, I\ndon't think anybody will be too upset at files being rewritten that were\nactually involved in the conflict.\n\nI think the fix you have for the first testcase is reasonable. It feels\na little like a band-aid, as we are throwing away stat information early\non and then pulling it again from the filesyste at the end. But from\nyour description, the real fix to that would probably involve some\nchanges to the way unpack_trees works, and that's probably complex\nenough that the band-aid is a good fix for now.\n\n-Peff\n"},{"id":"162569","messageId":"20110301193603.GB10082@sigill.intra.peff.net","threadId":"26628","inReplyTo":"20110301193142.GA10082@sigill.intra.peff.net","subject":"Re: [PATCHv2 0/3] Fix unnecessary (mtime) updates of files during merge","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-01T19:36:03Z","receivedAt":"2011-03-01T19:36:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 01, 2011 at 02:31:42PM -0500, Jeff King wrote:\n\n> I think the fix you have for the first testcase is reasonable. It feels\n> a little like a band-aid, as we are throwing away stat information early\n> on and then pulling it again from the filesyste at the end. But from\n> your description, the real fix to that would probably involve some\n> changes to the way unpack_trees works, and that's probably complex\n> enough that the band-aid is a good fix for now.\n\nOh, and I confirmed that it does fix Stephen's testcase, as well. With\nthe exception of some portability issues in the test case that JSixt\nalready pointed out:\n\nAcked-by: Jeff King <peff@peff.net>\n\nFor what that's worth. This is the first time I've had to look at the\nmerge-recursive code in any depth. :)\n\n-Peff\n"},{"id":"162572","messageId":"20110301193852.GC10082@sigill.intra.peff.net","threadId":"26628","inReplyTo":"4D6CA13C.1000006@viscovery.net","subject":"Re: [PATCHv2 1/3] t6022: New test checking for unnecessary updates of renamed+modified files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-01T19:38:52Z","receivedAt":"2011-03-01T19:38:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 01, 2011 at 08:33:16AM +0100, Johannes Sixt wrote:\n\n> > +\torig=$(stat --format=\"%Y\" rename) &&\n> \n> \torig=$(test-chmtime -v +0 rename) &&\n> \n> > +\tgit merge merge-branch-1 &&\n> > +\tnew=$(stat --format=\"%Y\" rename) &&\n> \n> \tnew=$(test-chmtime -v +0 rename) &&\n> \n> > +\techo \"Checking whether stat times are same: $orig vs $new\" &&\n> \n> \techo \"Checking whether stat times are same: ${orig%%\t*} vs ${new%%\t*}\" &&\n> \n> (that's TAB after the %%)\n> \n> > +\ttest \"$orig\" == \"$new\" &&\n> \n> \ttest \"${orig%%\t*}\" = \"${new%%\t*}\" &&\n\nMaybe this would be simpler as:\n\n  test-chmtime -v +0 rename >orig &&\n  ... do the merge ...\n  test-chmtime -v +0 rename >new &&\n  test_cmp orig new\n\nAnd then you don't have to care about the format that test-chmtime\noutputs at all, and test_cmp's diff output is usually self-explanatory\nwhen there is a mismatch (it's even better if you name the files\n\"expect\" and \"actual\").\n\n-Peff\n"},{"id":"162666","messageId":"7vwrkhb7ig.fsf@alter.siamese.dyndns.org","threadId":"26628","inReplyTo":"1298941732-19763-4-git-send-email-newren@gmail.com","subject":"Re: [PATCHv2 3/3] merge-recursive: When we detect we can skip an update, actually skip it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-02T20:19:03Z","receivedAt":"2011-03-02T20:19:03Z","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> @@ -1274,9 +1275,13 @@ static int merge_content(struct merge_options *o,\n>  \t}\n>  \n>  \tif (mfi.clean && !df_conflict_remains &&\n> -\t    sha_eq(mfi.sha, a_sha) && mfi.mode == a.mode)\n> +\t    sha_eq(mfi.sha, a_sha) && mfi.mode == a.mode &&\n> +\t    lstat(path, &st) == 0) {\n>  \t\toutput(o, 3, \"Skipped %s (merged same as existing)\", path);\n> -\telse\n> +\t\tadd_cacheinfo(mfi.mode, mfi.sha, path,\n> +\t\t\t      0 /*stage*/, 1 /*refresh*/, 0 /*options*/);\n> +\t\treturn mfi.clean;\n> +\t} else\n\nHmmmm.  During a merge, we allow files missing from the working tree as if\nit is not modified from the index.  Changing the behaviour based on the\nexistence of the path on the filesystem does not feel quite right.\n\nEven if we decide that regressing in that use case were acceptable, what\nguarantees that the path that happens to be in the work tree has the same\ncontents as what the merge result should be?  IOW, shouldn't we be looking\nat the original index, making sure that our side (stage #2) at the path\nhad a file there that matches the merge result mfi.{sha,mode}, instead of\nlooking at the working tree?\n"},{"id":"162685","messageId":"AANLkTin1EDUMRY7T+Jx64eG6R4mOZOeOukJWQHpFX+j5@mail.gmail.com","threadId":"26628","inReplyTo":"7vwrkhb7ig.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 3/3] merge-recursive: When we detect we can skip an update, actually skip it","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2011-03-02T22:22:49Z","receivedAt":"2011-03-02T22:22:49Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Mar 2, 2011 at 1:19 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Elijah Newren <newren@gmail.com> writes:\n>\n>> @@ -1274,9 +1275,13 @@ static int merge_content(struct merge_options *o,\n>>       }\n>>\n>>       if (mfi.clean && !df_conflict_remains &&\n>> -         sha_eq(mfi.sha, a_sha) && mfi.mode == a.mode)\n>> +         sha_eq(mfi.sha, a_sha) && mfi.mode == a.mode &&\n>> +         lstat(path, &st) == 0) {\n>>               output(o, 3, \"Skipped %s (merged same as existing)\", path);\n>> -     else\n>> +             add_cacheinfo(mfi.mode, mfi.sha, path,\n>> +                           0 /*stage*/, 1 /*refresh*/, 0 /*options*/);\n>> +             return mfi.clean;\n>> +     } else\n>\n> Hmmmm.  During a merge, we allow files missing from the working tree as if\n> it is not modified from the index.  Changing the behaviour based on the\n> existence of the path on the filesystem does not feel quite right.\n\nReally?  Ouch.  Then things have been broken ever since\nmake_room_for_directories_of_df_conflicts() was introduced, as that\nfunction deletes files intentionally and expects them to be reinstated\nlater when possible.  If a user has a file deleted that happens to be\ninvolved in a D/F conflict before a merge, and the file is unchanged,\nthe merge will reinstate it.\n\n> Even if we decide that regressing in that use case were acceptable, what\n> guarantees that the path that happens to be in the work tree has the same\n> contents as what the merge result should be?  IOW, shouldn't we be looking\n> at the original index, making sure that our side (stage #2) at the path\n> had a file there that matches the merge result mfi.{sha,mode}, instead of\n> looking at the working tree?\n\nWe DID look at the original index, make sure that our side (stage #2)\nat the path had a file there that matches the merge result\nmfi.{sha,mode}.\n\nBut, that's not enough to know that we can skip the update of the\nfile.  The merged results being different are only one reason to\nupdate the file; the other is that we may have deleted the file\nourselves in make_room_for_directories_of_df_conflicts() and need to\nreplace it.  This is the location in the code where such deleted files\nare reinstated.\n\n\nSuggestions?\n"},{"id":"162686","messageId":"AANLkTikG+DT3fz3ntqQAz7pzz+aDd2Q4db02C8ypQMx_@mail.gmail.com","threadId":"26628","inReplyTo":"4D6CA13C.1000006@viscovery.net","subject":"Re: [PATCHv2 1/3] t6022: New test checking for unnecessary updates of renamed+modified files","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2011-03-02T22:26:55Z","receivedAt":"2011-03-02T22:26:55Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Tue, Mar 1, 2011 at 12:33 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Am 3/1/2011 2:08, schrieb Elijah Newren:\n> Use\n>        test-chmtime =1000000000 rename &&\n<snip>\n>        orig=$(test-chmtime -v +0 rename) &&\n<snip>\n>> +     git diff-files --exit-code # Is \"rename\" clean, or only racily clean?\n>\n> What you see here, is not \"racily clean\", but \"stat dirty\". \"Racily clean\"\n> means that git thinks that the file is clean (because the stat information\n> matches), but in fact the file content is not identical to the content\n> recorded in the index.\n\nThanks for the suggestions and explanation; I've got it and Jeff's\nsuggestions incorporated.\n"},{"id":"162687","messageId":"AANLkTinuK=VbNVNraBnxoMHrDvSh-W7+QBEbOJWQaC=y@mail.gmail.com","threadId":"26628","inReplyTo":"20110301193142.GA10082@sigill.intra.peff.net","subject":"Re: [PATCHv2 0/3] Fix unnecessary (mtime) updates of files during merge","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2011-03-02T23:11:44Z","receivedAt":"2011-03-02T23:11:44Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nThanks for all the feedback.\n\nOn Tue, Mar 1, 2011 at 12:31 PM, Jeff King <peff@peff.net> wrote:\n...\n> As you mention, this second testcase is not a regression at all, and\n> while it would be great if we could avoid touching those files at all, I\n> don't think anybody will be too upset at files being rewritten that were\n> actually involved in the conflict.\n\nWell, technically they weren't involved in a conflict.  One side had\nan unmodified directory of files, the other side removed that\ndirectory and just put a file there.  Someone starts the merge on the\nside that only has a file there.  There's no conflict, but we update\nthe file.\n\nStill, it's a lot better than erroring out and claiming there was a\nconflict, so it's still an improvement over what we had.\n\n> I think the fix you have for the first testcase is reasonable. It feels\n> a little like a band-aid, as we are throwing away stat information early\n> on and then pulling it again from the filesyste at the end. But from\n> your description, the real fix to that would probably involve some\n> changes to the way unpack_trees works, and that's probably complex\n> enough that the band-aid is a good fix for now.\n\nI agree, it does seem like a bit of a band-aid, but as you say I was\nworried about the complexity of changing unpack_trees and wanted a\ngood interim fix.  Of course, Junio's point about\ndeleted-as-unmodified files might be sticky enough that we have to\nstart looking at such alternatives or do something more.  We'll have\nto see what he says.\n"},{"id":"162688","messageId":"7vaahdaz1j.fsf@alter.siamese.dyndns.org","threadId":"26628","inReplyTo":"1298941732-19763-1-git-send-email-newren@gmail.com","subject":"Re: [PATCHv2 0/3] Fix unnecessary (mtime) updates of files during merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-02T23:22:00Z","receivedAt":"2011-03-02T23:22:00Z","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> We could fix this second testcase by recording stat information for\n> files removed by make_room_for_directories_of_df_conflicts(), and\n> then, if those files are reinstated at the end of conflict resolution\n> (i.e. the directory of the D/F conflict went away during the merge),\n> then call utime() to reset the modification times on those files back\n> to what they originally were.\n\nWhich is a big Yuck.  I agree that we don't want to go there.\n\nThe real cause of the problem is that the code removes the files that\ncould potentially involved in conflict _way_ too early, and unfortunately\nthis is coming from the way merge-recursive tracks d/f conflicts, which is\nto grab set of path that can potentially appear as directories by throwing\nall directories into a single string list from both sides of the merge\n(same for non directory paths using another single string list) upfront,\nand after that point, it becomes very hard to tell if the potential\ndirectory is on your (i.e. stage #2) side or on their side, and to make\nthings worse, it doesn't wait writing partial results out to the working\ntree before it knows the result of the merge to say something intelligent\nlike \"all these files under the directory will go away and we can safely\nkeep the file there\".  Your earlier series, e.g. 2ff739f (merge-recursive:\nNew function to assist resolving renames in-core only, 2010-09-20), was a\nstep in the right direction, but we are not there yet while functions like\nmake_room_for_path() and make_room_for_directories_of_df_conflicts() are\nused.\n\nSo I think it would need a lot major restructuring of the merge-recursive,\nespecially its d/f conflict logic, to fix the second one correctly.\n\nThanks.\n"},{"id":"162690","messageId":"7v1v2payye.fsf@alter.siamese.dyndns.org","threadId":"26628","inReplyTo":"AANLkTin1EDUMRY7T+Jx64eG6R4mOZOeOukJWQHpFX+j5@mail.gmail.com","subject":"Re: [PATCHv2 3/3] merge-recursive: When we detect we can skip an update, actually skip it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-02T23:23:53Z","receivedAt":"2011-03-02T23:23:53Z","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> Suggestions?\n\nOther than not doing the remove way before the code knows that it is\nnecessary to get rid of the path because there needs to be a directory?\nNo.\n\nPersonally, I've long given up on d/f conflict codepath in merge-recursive\nas unsalvageable, short of a total rewrite.  As I said in the other\nmessage, the patch you sent in is a good enough fix within the context of\nthe current code.\n"}]}