Re: [PATCH 4/5] merge-recursive: handle D/F conflict case more carefully.

Alex Riesen [off-list ref] writes:

Junio C Hamano, Sat, Apr 07, 2007 16:42:55 +0200:

quoted

+			if (unlink(path)) {
+				if (errno == EISDIR) {
+					/* something else exists */
+					error(msg, path, ": perhaps a D/F conflict?");

isn't this one an F/D conflict?

Yes, as I said in a separate message, the current D/F detector
code in merge-recursive does not catch this case in t3030 test
and comes to this codepath to write it out:

   From: Junio C Hamano [off-list ref]
   Subject: Re: [PATCH 5/5] t3030: merge-recursive backend test.
   Date: Sat, 07 Apr 2007 22:17:25 -0700
   Message-ID: [off-list ref]

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

The part you quoted is merely working this problem around.

On 4/9/07, Junio C Hamano [off-list ref] wrote:

Alex Riesen [off-list ref] writes:

quoted

Junio C Hamano, Sat, Apr 07, 2007 16:42:55 +0200:

quoted

+                    if (unlink(path)) {
+                            if (errno == EISDIR) {
+                                    /* something else exists */
+                                    error(msg, path, ": perhaps a D/F conflict?");

isn't this one an F/D conflict?

Yes, as I said in a separate message, the current D/F detector
code in merge-recursive does not catch this case in t3030 test
and comes to this codepath to write it out:

I mean, maybe the error could be spelled differently:
": perhaps an F/D conflict?"
Right now we have two exactly the same errors,
as seen on the output. The messages refer to different
conflicts, though. It is usually useful to know what is really
going on.

"Alex Riesen" [off-list ref] writes:

On 4/9/07, Junio C Hamano [off-list ref] wrote:

quoted

Alex Riesen [off-list ref] writes:

quoted

Junio C Hamano, Sat, Apr 07, 2007 16:42:55 +0200:

quoted

+                    if (unlink(path)) {
+                            if (errno == EISDIR) {
+                                    /* something else exists */
+                                    error(msg, path, ": perhaps a D/F conflict?");

isn't this one an F/D conflict?

Yes, as I said in a separate message, the current D/F detector
code in merge-recursive does not catch this case in t3030 test
and comes to this codepath to write it out:

I mean, maybe the error could be spelled differently:
": perhaps an F/D conflict?"
Right now we have two exactly the same errors,
as seen on the output. The messages refer to different
conflicts, though. It is usually useful to know what is really
going on.

Perhaps, but I think the bigger issue is that existing D/F or
F/D conflict detection is simply buggy, and this patch shouldn't
be needed if they were working correctly.