[PATCHv3 0/3] Fix unnecessary (mtime) updates of files during merge

[PATCHv3 0/3] Fix unnecessary (mtime) updates of files during merge

[Elijah Newren]

This patch series fixes a bug reported by Stephen Rothwell -- that
during merges git would unnecessarily update modification times of
files.  As noted by both Jeff and Junio, it's a bit of a band-aid
since it doesn't dive into fixing make_room_for_path() and
make_room_for_directories_of_df_conflicts(), but that would be a
much bigger and more invasive change.

Changes since last version:
  * Portability fixes for the tests suggested by Hannes (thanks!)
  * Additional test cleanups suggested by Jeff
  * Jeff's acks have been added

Elijah Newren (3):
  t6022: New test checking for unnecessary updates of renamed+modified files
  t6022: New test checking for unnecessary updates of files in D/F conflicts
  merge-recursive: When we detect we can skip an update, actually skip it

 merge-recursive.c       |   17 ++++++++----
 t/t6022-merge-rename.sh |   65 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 6 deletions(-)

-- 
1.7.4

[PATCHv3 1/3] t6022: New test checking for unnecessary updates of renamed+modified files

[Elijah Newren]

Signed-off-by: Elijah Newren <newren@gmail.com>
Acked-by: Jeff King <peff@peff.net>
---
 t/t6022-merge-rename.sh |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh
index 1ed259d..16330a6 100755
--- a/t/t6022-merge-rename.sh
+++ b/t/t6022-merge-rename.sh
@@ -609,4 +609,35 @@ test_expect_success 'check handling of differently renamed file with D/F conflic
 	! test -f original
 '
 
+test_expect_success 'setup avoid unnecessary update, normal rename' '
+	git reset --hard &&
+	git checkout --orphan avoid-unnecessary-update-1 &&
+	git rm -rf . &&
+	git clean -fdqx &&
+
+	printf "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n" >original &&
+	git add -A &&
+	git commit -m "Common commmit" &&
+
+	git mv original rename &&
+	echo 11 >>rename &&
+	git add -u &&
+	git commit -m "Renamed and modified" &&
+
+	git checkout -b merge-branch-1 HEAD~1 &&
+	echo "random content" >random-file &&
+	git add -A &&
+	git commit -m "Random, unrelated changes"
+'
+
+test_expect_failure 'avoid unnecessary update, normal rename' '
+	git checkout -q avoid-unnecessary-update-1^0 &&
+	test-chmtime =1 rename &&
+	test-chmtime -v +0 rename >expect &&
+	git merge merge-branch-1 &&
+	git diff-files --exit-code &&   # Is "rename" clean, or stat dirty?
+	test-chmtime -v +0 rename >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.4

[PATCHv3 2/3] t6022: New test checking for unnecessary updates of files in D/F conflicts

[Elijah Newren]

Signed-off-by: Elijah Newren <newren@gmail.com>
Acked-by: Jeff King <peff@peff.net>
---
 t/t6022-merge-rename.sh |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh
index 16330a6..88cc410 100755
--- a/t/t6022-merge-rename.sh
+++ b/t/t6022-merge-rename.sh
@@ -640,4 +640,38 @@ test_expect_failure 'avoid unnecessary update, normal rename' '
 	test_cmp expect actual
 '
 
+test_expect_success 'setup avoid unnecessary update, with D/F conflict' '
+	git reset --hard &&
+	git checkout --orphan avoid-unnecessary-update-2 &&
+	git rm -rf . &&
+	git clean -fdqx &&
+
+	mkdir df &&
+	printf "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n" >df/file &&
+	git add -A &&
+	git commit -m "Common commmit" &&
+
+	git mv df/file temp &&
+	rm -rf df &&
+	git mv temp df &&
+	echo 11 >>df &&
+	git add -u &&
+	git commit -m "Renamed and modified" &&
+
+	git checkout -b merge-branch-2 HEAD~1 &&
+	>unrelated-change &&
+	git add unrelated-change &&
+	git commit -m "Only unrelated changes"
+'
+
+test_expect_failure 'avoid unnecessary update, with D/F conflict' '
+	git checkout -q avoid-unnecessary-update-2^0 &&
+	test-chmtime =1 rename &&
+	test-chmtime -v +0 rename >expect &&
+	git merge merge-branch-2 &&
+	git diff-files --exit-code &&   # Is "rename" clean, or stat dirty?
+	test-chmtime -v +0 rename >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.4

[PATCHv3 3/3] merge-recursive: When we detect we can skip an update, actually skip it

[Elijah Newren]

In 882fd11 (merge-recursive: Delay content merging for renames 2010-09-20),
there was code that checked for whether we could skip updating a file in
the working directory, based on whether the merged version matched the
current working copy.  Due to the desire to handle directory/file conflicts
that were resolvable, that commit deferred content merging by first
updating the index with the unmerged entries and then moving the actual
merging (along with the skip-the-content-update check) to another function
that ran later in the merge process.  As part moving the content merging
code, a bug was introduced such that although the message about skipping
the update would be printed (whenever GIT_MERGE_VERBOSITY was sufficiently
high), the file would be unconditionally updated in the working copy
anyway.

When we detect that the file does not need to be updated in the working
copy, update the index appropriately and then return early before updating
the working copy.

Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Elijah Newren <newren@gmail.com>
Acked-by: Jeff King <peff@peff.net>
---
 merge-recursive.c       |   17 +++++++++++------
 t/t6022-merge-rename.sh |    2 +-
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 16c2dbe..ec07a30 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -354,10 +354,11 @@ static void make_room_for_directories_of_df_conflicts(struct merge_options *o,
 	 * make room for the corresponding directory.  Such paths will
 	 * later be processed in process_df_entry() at the end.  If
 	 * the corresponding directory ends up being removed by the
-	 * merge, then the file will be reinstated at that time;
-	 * otherwise, if the file is not supposed to be removed by the
-	 * merge, the contents of the file will be placed in another
-	 * unique filename.
+	 * merge, then the file will be reinstated at that time
+	 * (albeit with a different timestamp!); otherwise, if the
+	 * file is not supposed to be removed by the merge, the
+	 * contents of the file will be placed in another unique
+	 * filename.
 	 *
 	 * NOTE: This function relies on the fact that entries for a
 	 * D/F conflict will appear adjacent in the index, with the
@@ -1274,9 +1275,13 @@ static int merge_content(struct merge_options *o,
 	}
 
 	if (mfi.clean && !df_conflict_remains &&
-	    sha_eq(mfi.sha, a_sha) && mfi.mode == a.mode)
+	    sha_eq(mfi.sha, a_sha) && mfi.mode == a.mode &&
+	    lstat(path, &st) == 0) {
 		output(o, 3, "Skipped %s (merged same as existing)", path);
-	else
+		add_cacheinfo(mfi.mode, mfi.sha, path,
+			      0 /*stage*/, 1 /*refresh*/, 0 /*options*/);
+		return mfi.clean;
+	} else
 		output(o, 2, "Auto-merging %s", path);
 
 	if (!mfi.clean) {
diff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh
index 88cc410..6f9df68 100755
--- a/t/t6022-merge-rename.sh
+++ b/t/t6022-merge-rename.sh
@@ -630,7 +630,7 @@ test_expect_success 'setup avoid unnecessary update, normal rename' '
 	git commit -m "Random, unrelated changes"
 '
 
-test_expect_failure 'avoid unnecessary update, normal rename' '
+test_expect_success 'avoid unnecessary update, normal rename' '
 	git checkout -q avoid-unnecessary-update-1^0 &&
 	test-chmtime =1 rename &&
 	test-chmtime -v +0 rename >expect &&
-- 
1.7.4

Re: [PATCHv3 0/3] Fix unnecessary (mtime) updates of files during merge

[Junio C Hamano]

This seems to more-or-less match what I queued in 'pu' yesterday (v2 with
fix-ups from the list).

Thanks.