[PATCH 2/5] Treat D/F conflict entry more carefully in unpack-trees.c::threeway_merge()

[PATCH 2/5] Treat D/F conflict entry more carefully in unpack-trees.c::threeway_merge()

[Junio C Hamano]

This fixes three buglets in threeway_merge() regarding D/F
conflict entries.

* After finishing with path D and handling path D/F, some stages
  have D/F conflict entry which are obviously non-NULL.  For the
  purpose of determining if the path D/F is missing in the
  ancestor, they should not be taken into account.

* D/F conflict entry is a phony entry and does not record the
  path being processed, so do not pick up the name from there.

* D/F conflict entry is a marker to say "this stage does _not_
  have the path", so do not send them to keep_entry().

There might be more glitches, but I am slowly digging this mess
through, which unfortunately was made even more work since
merge-recursive is a built-in now.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 unpack-trees.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index a0b6769..4039653 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -675,10 +675,10 @@ int threeway_merge(struct cache_entry **stages,
 	int i;
 
 	for (i = 1; i < o->head_idx; i++) {
-		if (!stages[i])
+		if (!stages[i] || stages[i] == o->df_conflict_entry)
 			any_anc_missing = 1;
 		else {
-			if (!path)
+			if (!path && stages[i] != o->df_conflict_entry)
 				path = stages[i]->name;
 			no_anc_exists = 0;
 		}
@@ -790,7 +790,7 @@ int threeway_merge(struct cache_entry **stages,
 	count = 0;
 	if (!head_match || !remote_match) {
 		for (i = 1; i < o->head_idx; i++) {
-			if (stages[i]) {
+			if (stages[i] && stages[i] != o->df_conflict_entry) {
 				keep_entry(stages[i], o);
 				count++;
 				break;
-- 
1.5.1.81.gee969

Re: [PATCH 2/5] Treat D/F conflict entry more carefully in unpack-trees.c::threeway_merge()

[Daniel Barkalow]

On Sat, 7 Apr 2007, Junio C Hamano wrote:

> This fixes three buglets in threeway_merge() regarding D/F
> conflict entries.
> 
> * After finishing with path D and handling path D/F, some stages
>   have D/F conflict entry which are obviously non-NULL.  For the
>   purpose of determining if the path D/F is missing in the
>   ancestor, they should not be taken into account.
> 
> * D/F conflict entry is a phony entry and does not record the
>   path being processed, so do not pick up the name from there.

This bit is unnecessary, because the first bit means we treat D/F conflict 
as missing in that conditional, and don't count it as an entry at all, let 
alone one with a useful name.

> * D/F conflict entry is a marker to say "this stage does _not_
>   have the path", so do not send them to keep_entry().
> 
> There might be more glitches, but I am slowly digging this mess
> through, which unfortunately was made even more work since
> merge-recursive is a built-in now.

Looks good, although it might be wise to add an "exists" function that 
returns false for df_conflict_entry and for NULL, to make the tests 
clearer, and to get a comment to point out the meaning of 
df_conflict_entry. Also, I think df_conflict_entry can be static bss in 
unpack-trees and not accessed through o->df_conflict_entry, since it's 
always the same value (being now universally initialized to a static 
pointer to heap...).

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 2/5] Treat D/F conflict entry more carefully in unpack-trees.c::threeway_merge()

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Sat, 7 Apr 2007, Junio C Hamano wrote:
>
>> This fixes three buglets in threeway_merge() regarding D/F
>> conflict entries.
>> 
>> * After finishing with path D and handling path D/F, some stages
>>   have D/F conflict entry which are obviously non-NULL.  For the
>>   purpose of determining if the path D/F is missing in the
>>   ancestor, they should not be taken into account.
>> 
>> * D/F conflict entry is a phony entry and does not record the
>>   path being processed, so do not pick up the name from there.
>
> This bit is unnecessary, because the first bit means we treat D/F conflict 
> as missing in that conditional, and don't count it as an entry at all, let 
> alone one with a useful name.

I am not sure about what you mean by "first bit", but I added
this after noticing there was a case where the path was not
picked up from index/head/remote correctly because the "pick up
the path from ancestors, while we check for any_anc_missing and
no_anc_exists condition" loop at the beginning already set it to
point at stages[i]->name (which is an empty string "" name of
the df-conflict entry).

But I notice that "aggressive" codepath is the only thing that
wants to use the path (it uses it to check the working tree
status), so maybe we should move that path computation business
inside the aggressive codepath.

Re: [PATCH 2/5] Treat D/F conflict entry more carefully in unpack-trees.c::threeway_merge()

[Daniel Barkalow]

On Sat, 7 Apr 2007, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > On Sat, 7 Apr 2007, Junio C Hamano wrote:
> >
> >> This fixes three buglets in threeway_merge() regarding D/F
> >> conflict entries.
> >> 
> >> * After finishing with path D and handling path D/F, some stages
> >>   have D/F conflict entry which are obviously non-NULL.  For the
> >>   purpose of determining if the path D/F is missing in the
> >>   ancestor, they should not be taken into account.
> >> 
> >> * D/F conflict entry is a phony entry and does not record the
> >>   path being processed, so do not pick up the name from there.
> >
> > This bit is unnecessary, because the first bit means we treat D/F conflict 
> > as missing in that conditional, and don't count it as an entry at all, let 
> > alone one with a useful name.
> 
> I am not sure about what you mean by "first bit"

Your first bullet point. You must have added the check to the path line 
before you added the same check to the condition a few lines above, making 
this check no longer necessary.

                if (!stages[i] || stages[i] == o->df_conflict_entry)
                        any_anc_missing = 1;
                else {

If we get here, stages[i] != o->df_conflict_entry...

                        if (!path && stages[i] != o->df_conflict_entry)
                                path = stages[i]->name;
                        no_anc_exists = 0;
                }

So you don't need the second check if you've got the first one, and the 
first one makes more sense anyway; for the purposes of this entire 
section, we want df_conflict_entry to count as missing, and we don't look 
at the paths of missing entries regardless of whether they're specially 
marked.

	-Daniel
*This .sig left intentionally blank*