Re: [PATCH 5/5] t3030: merge-recursive backend test.

From: Junio C Hamano <junkio@cox.net>
Date: Apr 8, 2007, 05:17 UTC
Message-ID: <7vmz1ja1i2.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <7v4pnsckju.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano <junkio@cox.net> writes:

Show 14 quoted lines

> +test_expect_success 'merge-recursive d/f conflict result' '
> +
> +	git ls-files -s >actual &&
> +	(
> +		echo "100644 $o0 0	A"
> +		echo "100644 $o0 1	a"
> +		echo "100644 $o1 2	a"
> +		echo "100644 $o4 0	a/c"
> +		echo "100644 $o0 0	b"
> +		echo "100644 $o1 0	d/e"
> +	) >expected &&
> +	git diff -u expected actual
> +
> +'

The expected result of this test is what I find very iffy. The input trees are:

 * Ancestor has a file "a"
 * Our tree has a file "a", which was modified from the
   ancestor's
 * The other tree has a file "a/c"

merge-recursive.c::git_merge_trees() calls unpack_trees() using the threeway_merge function and what it gets back are stage 1 and 2 entries for "a", and stage 3 entry for "a/c" (you can see it by running "git-read-tree -m $c0 $c1 $c4" instead). But the output from merge-recursive has "a/c" resolved down to stage 0.

The problem is that process_entry() finds that there is only stage 3 entry for that path and decides to resolve it. I think it should do the D/F check and leave "a/c" in stage 3.

Looking at the D/F conflict detection code in merge-recursive.c makes my head hurt, and I do not quite follow the logic there, but it seems to be doing the following to me.

 * current_directory_set is initialized to be union of all
   directories that appear in our tree and the other tree (I am
   not sure if that set is updated and if so how);

 * when a path is about to be resolved to a blob, this set is
   consulted, and if there is a directory there, declare D/F
   conflict and blob is added under a name that does not
   conflict with the directory;

 * otherwise the path is resolved down to stage 0.

However, in this case, there is no such directory "a/c" but the code forgets that adding a blob at "a/c" requires that there is no file "a" at all. I am not sure if this is deliberate or just a simple oversight.

Also I suspect this can be done more easily with just checking
the index.  We know blob "a" is in the index, unresolved, when
we try to resolve path "a/c".