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

[PATCHv2 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.

There are two testcases included in this patch series.  The first is a
simple case to test the originally reported bug; this testcase is
fixed in this series (as is Stephen's original linux-next testcase).
The second testcase suffers from the exact same problem, but arises
from a different situation and is not fixed in this series.  That
testcase is slightly harder to solve because:

  * unpack_trees + threeway_merge throws away the original index entry
    with stat information when it notices the directory/file conflict

  * make_room_for_directories_of_df_conflicts() must remove such files
    from the working copy or the corresponding directory and files
    below it will be unable to be written to the working copy (which
    can cause spurious conflicts, or make resolving conflicts very
    hard for users who don't know how to access the many files missing
    from their working copy).

We could fix this second testcase by recording stat information for
files removed by make_room_for_directories_of_df_conflicts(), and
then, if those files are reinstated at the end of conflict resolution
(i.e. the directory of the D/F conflict went away during the merge),
then call utime() to reset the modification times on those files back
to what they originally were.

(Technically, the second testcase that I left unfixed is not a
regression; prior versions of git would fail the merge and record the
file's content in an alternative file to "avoid" the directory it
thought was in the way, so the modification time was the least of the
worries.)


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 |   68 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 6 deletions(-)

-- 
1.7.4

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

[Elijah Newren]

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

diff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh
index 1ed259d..3667e18 100755
--- a/t/t6022-merge-rename.sh
+++ b/t/t6022-merge-rename.sh
@@ -609,4 +609,36 @@ 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 &&
+	touch -t 197001010000.01 rename &&
+	orig=$(stat --format="%Y" rename) &&
+	git merge merge-branch-1 &&
+	new=$(stat --format="%Y" rename) &&
+	echo "Checking whether stat times are same: $orig vs $new" &&
+	test "$orig" == "$new" &&
+	git diff-files --exit-code # Is "rename" clean, or only racily clean?
+'
+
 test_done
-- 
1.7.4

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

[Elijah Newren]

Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6022-merge-rename.sh |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh
index 3667e18..a5d252b 100755
--- a/t/t6022-merge-rename.sh
+++ b/t/t6022-merge-rename.sh
@@ -641,4 +641,40 @@ test_expect_failure 'avoid unnecessary update, normal rename' '
 	git diff-files --exit-code # Is "rename" clean, or only racily clean?
 '
 
+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 &&
+	touch -t 197001010000.01 df &&
+	orig=$(stat --format="%Y" df) &&
+	git merge merge-branch-2 &&
+	new=$(stat --format="%Y" df) &&
+	echo "Checking whether stat times are same: $orig vs $new" &&
+	test "$orig" == "$new" &&
+	git diff-files --exit-code # Is "rename" clean, or only racily clean?
+'
+
+
 test_done
-- 
1.7.4

[PATCHv2 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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 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 a5d252b..a44117a 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 &&
 	touch -t 197001010000.01 rename &&
 	orig=$(stat --format="%Y" rename) &&
-- 
1.7.4

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

[Johannes Sixt]

Am 3/1/2011 2:08, schrieb Elijah Newren:
> +test_expect_failure 'avoid unnecessary update, normal rename' '
> +	git checkout -q avoid-unnecessary-update-1^0 &&
> +	touch -t 197001010000.01 rename &&

Use
	test-chmtime =1000000000 rename &&

> +	orig=$(stat --format="%Y" rename) &&

	orig=$(test-chmtime -v +0 rename) &&

> +	git merge merge-branch-1 &&
> +	new=$(stat --format="%Y" rename) &&

	new=$(test-chmtime -v +0 rename) &&

> +	echo "Checking whether stat times are same: $orig vs $new" &&

	echo "Checking whether stat times are same: ${orig%%	*} vs ${new%%	*}" &&

(that's TAB after the %%)

> +	test "$orig" == "$new" &&

	test "${orig%%	*}" = "${new%%	*}" &&

== is not portable. Actually, since the file name is the same in both
$orig and $new, you wouldn't need the %% magic in this statement.

> +	git diff-files --exit-code # Is "rename" clean, or only racily clean?

What you see here, is not "racily clean", but "stat dirty". "Racily clean"
means that git thinks that the file is clean (because the stat information
matches), but in fact the file content is not identical to the content
recorded in the index.

> +'
> +
>  test_done

-- Hannes

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

[Jeff King]

On Mon, Feb 28, 2011 at 06:08:49PM -0700, Elijah Newren wrote:

> This patch series fixes a bug reported by Stephen Rothwell -- that
> during merges git would unnecessarily update modification times of
> files.

Thanks for the fix. Sorry I didn't have a chance to look at your interim
patch, but it seems you resolved the issue.

> There are two testcases included in this patch series.  The first is a
> simple case to test the originally reported bug; this testcase is
> fixed in this series (as is Stephen's original linux-next testcase).
> The second testcase suffers from the exact same problem, but arises
> from a different situation and is not fixed in this series.  That
> testcase is slightly harder to solve because:
> 
>   * unpack_trees + threeway_merge throws away the original index entry
>     with stat information when it notices the directory/file conflict
> 
>   * make_room_for_directories_of_df_conflicts() must remove such files
>     from the working copy or the corresponding directory and files
>     below it will be unable to be written to the working copy (which
>     can cause spurious conflicts, or make resolving conflicts very
>     hard for users who don't know how to access the many files missing
>     from their working copy).
> 
> We could fix this second testcase by recording stat information for
> files removed by make_room_for_directories_of_df_conflicts(), and
> then, if those files are reinstated at the end of conflict resolution
> (i.e. the directory of the D/F conflict went away during the merge),
> then call utime() to reset the modification times on those files back
> to what they originally were.

As you mention, this second testcase is not a regression at all, and
while it would be great if we could avoid touching those files at all, I
don't think anybody will be too upset at files being rewritten that were
actually involved in the conflict.

I think the fix you have for the first testcase is reasonable. It feels
a little like a band-aid, as we are throwing away stat information early
on and then pulling it again from the filesyste at the end. But from
your description, the real fix to that would probably involve some
changes to the way unpack_trees works, and that's probably complex
enough that the band-aid is a good fix for now.

-Peff

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

[Jeff King]

On Tue, Mar 01, 2011 at 02:31:42PM -0500, Jeff King wrote:

> I think the fix you have for the first testcase is reasonable. It feels
> a little like a band-aid, as we are throwing away stat information early
> on and then pulling it again from the filesyste at the end. But from
> your description, the real fix to that would probably involve some
> changes to the way unpack_trees works, and that's probably complex
> enough that the band-aid is a good fix for now.

Oh, and I confirmed that it does fix Stephen's testcase, as well. With
the exception of some portability issues in the test case that JSixt
already pointed out:

Acked-by: Jeff King <peff@peff.net>

For what that's worth. This is the first time I've had to look at the
merge-recursive code in any depth. :)

-Peff

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

[Jeff King]

On Tue, Mar 01, 2011 at 08:33:16AM +0100, Johannes Sixt wrote:

> > +	orig=$(stat --format="%Y" rename) &&
> 
> 	orig=$(test-chmtime -v +0 rename) &&
> 
> > +	git merge merge-branch-1 &&
> > +	new=$(stat --format="%Y" rename) &&
> 
> 	new=$(test-chmtime -v +0 rename) &&
> 
> > +	echo "Checking whether stat times are same: $orig vs $new" &&
> 
> 	echo "Checking whether stat times are same: ${orig%%	*} vs ${new%%	*}" &&
> 
> (that's TAB after the %%)
> 
> > +	test "$orig" == "$new" &&
> 
> 	test "${orig%%	*}" = "${new%%	*}" &&

Maybe this would be simpler as:

  test-chmtime -v +0 rename >orig &&
  ... do the merge ...
  test-chmtime -v +0 rename >new &&
  test_cmp orig new

And then you don't have to care about the format that test-chmtime
outputs at all, and test_cmp's diff output is usually self-explanatory
when there is a mismatch (it's even better if you name the files
"expect" and "actual").

-Peff

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> @@ -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

Hmmmm.  During a merge, we allow files missing from the working tree as if
it is not modified from the index.  Changing the behaviour based on the
existence of the path on the filesystem does not feel quite right.

Even if we decide that regressing in that use case were acceptable, what
guarantees that the path that happens to be in the work tree has the same
contents as what the merge result should be?  IOW, shouldn't we be looking
at the original index, making sure that our side (stage #2) at the path
had a file there that matches the merge result mfi.{sha,mode}, instead of
looking at the working tree?

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

[Elijah Newren]

On Wed, Mar 2, 2011 at 1:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:
>
>> @@ -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
>
> Hmmmm.  During a merge, we allow files missing from the working tree as if
> it is not modified from the index.  Changing the behaviour based on the
> existence of the path on the filesystem does not feel quite right.

Really?  Ouch.  Then things have been broken ever since
make_room_for_directories_of_df_conflicts() was introduced, as that
function deletes files intentionally and expects them to be reinstated
later when possible.  If a user has a file deleted that happens to be
involved in a D/F conflict before a merge, and the file is unchanged,
the merge will reinstate it.

> Even if we decide that regressing in that use case were acceptable, what
> guarantees that the path that happens to be in the work tree has the same
> contents as what the merge result should be?  IOW, shouldn't we be looking
> at the original index, making sure that our side (stage #2) at the path
> had a file there that matches the merge result mfi.{sha,mode}, instead of
> looking at the working tree?

We DID look at the original index, make sure that our side (stage #2)
at the path had a file there that matches the merge result
mfi.{sha,mode}.

But, that's not enough to know that we can skip the update of the
file.  The merged results being different are only one reason to
update the file; the other is that we may have deleted the file
ourselves in make_room_for_directories_of_df_conflicts() and need to
replace it.  This is the location in the code where such deleted files
are reinstated.


Suggestions?

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

[Elijah Newren]

Hi,

On Tue, Mar 1, 2011 at 12:33 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 3/1/2011 2:08, schrieb Elijah Newren:
> Use
>        test-chmtime =1000000000 rename &&
<snip>
>        orig=$(test-chmtime -v +0 rename) &&
<snip>
>> +     git diff-files --exit-code # Is "rename" clean, or only racily clean?
>
> What you see here, is not "racily clean", but "stat dirty". "Racily clean"
> means that git thinks that the file is clean (because the stat information
> matches), but in fact the file content is not identical to the content
> recorded in the index.

Thanks for the suggestions and explanation; I've got it and Jeff's
suggestions incorporated.

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

[Elijah Newren]

Hi,

Thanks for all the feedback.

On Tue, Mar 1, 2011 at 12:31 PM, Jeff King <peff@peff.net> wrote:
...
> As you mention, this second testcase is not a regression at all, and
> while it would be great if we could avoid touching those files at all, I
> don't think anybody will be too upset at files being rewritten that were
> actually involved in the conflict.

Well, technically they weren't involved in a conflict.  One side had
an unmodified directory of files, the other side removed that
directory and just put a file there.  Someone starts the merge on the
side that only has a file there.  There's no conflict, but we update
the file.

Still, it's a lot better than erroring out and claiming there was a
conflict, so it's still an improvement over what we had.

> I think the fix you have for the first testcase is reasonable. It feels
> a little like a band-aid, as we are throwing away stat information early
> on and then pulling it again from the filesyste at the end. But from
> your description, the real fix to that would probably involve some
> changes to the way unpack_trees works, and that's probably complex
> enough that the band-aid is a good fix for now.

I agree, it does seem like a bit of a band-aid, but as you say I was
worried about the complexity of changing unpack_trees and wanted a
good interim fix.  Of course, Junio's point about
deleted-as-unmodified files might be sticky enough that we have to
start looking at such alternatives or do something more.  We'll have
to see what he says.

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> We could fix this second testcase by recording stat information for
> files removed by make_room_for_directories_of_df_conflicts(), and
> then, if those files are reinstated at the end of conflict resolution
> (i.e. the directory of the D/F conflict went away during the merge),
> then call utime() to reset the modification times on those files back
> to what they originally were.

Which is a big Yuck.  I agree that we don't want to go there.

The real cause of the problem is that the code removes the files that
could potentially involved in conflict _way_ too early, and unfortunately
this is coming from the way merge-recursive tracks d/f conflicts, which is
to grab set of path that can potentially appear as directories by throwing
all directories into a single string list from both sides of the merge
(same for non directory paths using another single string list) upfront,
and after that point, it becomes very hard to tell if the potential
directory is on your (i.e. stage #2) side or on their side, and to make
things worse, it doesn't wait writing partial results out to the working
tree before it knows the result of the merge to say something intelligent
like "all these files under the directory will go away and we can safely
keep the file there".  Your earlier series, e.g. 2ff739f (merge-recursive:
New function to assist resolving renames in-core only, 2010-09-20), was a
step in the right direction, but we are not there yet while functions like
make_room_for_path() and make_room_for_directories_of_df_conflicts() are
used.

So I think it would need a lot major restructuring of the merge-recursive,
especially its d/f conflict logic, to fix the second one correctly.

Thanks.

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> Suggestions?

Other than not doing the remove way before the code knows that it is
necessary to get rid of the path because there needs to be a directory?
No.

Personally, I've long given up on d/f conflict codepath in merge-recursive
as unsalvageable, short of a total rewrite.  As I said in the other
message, the patch you sent in is a good enough fix within the context of
the current code.