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

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(-)

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

Am 3/1/2011 2:08, schrieb Elijah Newren:

Use
	test-chmtime =1000000000 rename &&

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

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

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

(that's TAB after the %%)

	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.

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.

-- Hannes

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

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

Hi,

On Tue, Mar 1, 2011 at 12:33 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:

<snip>

<snip>

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

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(-)

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(-)

Elijah Newren <newren@gmail.com> writes:

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?

On Wed, Mar 2, 2011 at 1:19 PM, Junio C Hamano <gitster@pobox.com> wrote:

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.

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?

Elijah Newren <newren@gmail.com> writes:

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.

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

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

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

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

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

Hi,

Thanks for all the feedback.

On Tue, Mar 1, 2011 at 12:31 PM, Jeff King <peff@peff.net> wrote: ...

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

Elijah Newren <newren@gmail.com> writes:

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.