threads / patch / 7561

patch, 5 partsmerge-recursive: handle D/F conflict case more carefully.

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

## tl;dr

5 messages between Apr 7, 2007 and Apr 10, 2007. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Junio C Hamano· Apr 7, 2007, 14:42 UTC · lore

When a path D that originally was blob in the ancestor was modified on our branch while it was removed on the other branch, we keep stages 1 and 2, and leave our version in the working tree. If the other branch created a path D/F, however, that path can cleanly be resolved in the index (after all, the ancestor nor we do not have it and only the other side added), but it cannot be checked out. The issue is the same when the other branch had D and we had renamed it to D/F, or the ancestor had D/F instead of D (so there are four combinations).

Do not stop the merge, but leave both D and D/F paths in the index so that the user can clear things up.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 * This one is quite iffy, as I suspect it can leave higher
   stages for intermediate merges in its current form, and I
   suspect that the caller of this function should be checking
   for D/F conflicts (there are some provisions in the existing
   code, but apparently they are not good enough to do the right
   thing to pass t3030 test which is the next patch).
 merge-recursive.c |   29 ++++++++++++++++++++++++++---
 1 files changed, 26 insertions(+), 3 deletions(-)
Show changes to merge-recursive.c +26 −3
diff --git a/merge-recursive.c b/merge-recursive.c
index aa3d913..857eeee 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -580,9 +580,31 @@ static void update_file_flags(const unsigned char *sha,
 
 		if (S_ISREG(mode) || (!has_symlinks && S_ISLNK(mode))) {
 			int fd;
-			if (mkdir_p(path, 0777))
-				die("failed to create path %s: %s", path, strerror(errno));
-			unlink(path);
+			int status;
+			const char *msg = "failed to create path '%s'%s";
+
+			status = mkdir_p(path, 0777);
+			if (status) {
+				if (status == -3) {
+					/* something else exists */
+					error(msg, path, ": perhaps a D/F conflict?");
+					update_wd = 0;
+					goto update_index;
+				}
+				die(msg, path, "");
+			}
+			if (unlink(path)) {
+				if (errno == EISDIR) {
+					/* something else exists */
+					error(msg, path, ": perhaps a D/F conflict?");
+					update_wd = 0;
+					goto update_index;
+				}
+				if (errno != ENOENT)
+					die("failed to unlink %s "
+					    "in preparation to update: %s",
+					    path, strerror(errno));
+			}
 			if (mode & 0100)
 				mode = 0777;
 			else
@@ -604,6 +626,7 @@ static void update_file_flags(const unsigned char *sha,
 			die("do not know what to do with %06o %s '%s'",
 			    mode, sha1_to_hex(sha), path);
 	}
+ update_index:
 	if (update_cache)
 		add_cacheinfo(mode, sha, path, 0, update_wd, ADD_CACHE_OK_TO_ADD);
 }

-- 
1.5.1.81.gee969
Alex Riesen· Apr 9, 2007, 18:58 UTC · re: Junio C Hamano · lore

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

Junio C Hamano, Sat, Apr 07, 2007 16:42:55 +0200:
> +			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?
Junio C Hamano· Apr 9, 2007, 19:53 UTC · re: Alex Riesen · lore

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

Alex Riesen <raa.lkml@gmail.com> writes:
Show 7 quoted lines
> Junio C Hamano, Sat, Apr 07, 2007 16:42:55 +0200:
>> +			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 <junkio@cox.net>
   Subject: Re: [PATCH 5/5] t3030: merge-recursive backend test.
   Date: Sat, 07 Apr 2007 22:17:25 -0700
   Message-ID: <7vmz1ja1i2.fsf@assigned-by-dhcp.cox.net>
   > +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.
Alex Riesen· Apr 10, 2007, 08:40 UTC · re: Junio C Hamano · lore

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

On 4/9/07, Junio C Hamano <junkio@cox.net> wrote:
Show 14 quoted lines
> Alex Riesen <raa.lkml@gmail.com> writes:
>
> > Junio C Hamano, Sat, Apr 07, 2007 16:42:55 +0200:
> >> +                    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.

Junio C Hamano· Apr 10, 2007, 18:20 UTC · re: Alex Riesen · lore

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

"Alex Riesen" <raa.lkml@gmail.com> writes:
Show 22 quoted lines
> On 4/9/07, Junio C Hamano <junkio@cox.net> wrote:
>> Alex Riesen <raa.lkml@gmail.com> writes:
>>
>> > Junio C Hamano, Sat, Apr 07, 2007 16:42:55 +0200:
>> >> +                    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.

← back to recent threads