Re: [PATCH] merge-tree: sometimes, d/f conflict is not an issue

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 8, 2007, 02:00 UTC
Message-ID: <Pine.LNX.4.64.0707080248320.4093@racer.site>
In-Reply-To: <7vabu765r0.fsf@assigned-by-dhcp.cox.net>

Hi,

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

Show 17 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > @@ -643,6 +643,20 @@ int threeway_merge(struct cache_entry **stages,
> >  	index = stages[0];
> >  	head = stages[o->head_idx];
> >  
> > +	/*
> > +	 * Special case: do not care about a dir/file conflict, when
> > +	 * the entries have not been touched.
> > +	 * IOW if the ancestors are identical to the remote, and the
> > +	 * index is the same as head, just take head.
> > +	 */
> 
> Suppose paths "A" and "A/B" are involved, and you resolved with
> this logic to have "A" as a blob (so your HEAD does not have
> "A/B").  If the remote adds "A/B", what prevents the resulting
> index to have both "A" and "A/B" resolved at stage #0?

Hmm.

Show 5 quoted lines

> A logic to do "if it is unchanged on one and changed in another,
> take changed one" already exists in later part of the code; your
> patch just circumvents D/F checks built into threeway_merge for
> this one case, and only because this one case happens to have
> reported.  It doesn't feel right.

Well, for me the code in threeway_merge does not feel right. There is a table in technical/trivial-merge.txt (which I not fully understand, since nowhere in the table, there is a mention of the index, and nowhere is explained what "ALT" is supposed to mean), but threeway_merge only references the numbers. The code is not obvious to me.

I spent some hours staring on, and trying to make sense of it. Alas, I am an idiot or something, since my brain feels like a mashed potato, and I still do not understand the code. For example, it is not apparent to me why the variable "head" should be set to NULL, when it was the df_conflict_entry.

So yeah, this patch was marked as "PATCH", but the subject line is not long enough for the proper prefix, which would have started like "[This PATCH works for me, and I do not know how to make it better, since I do not understand the code in threeway_merge(), and that does not make me happy, but that is the way things are, and maybe someone more intelligent than me recognizes what is meant by my little patch, and can fix it up, ...]".

Show 5 quoted lines

> IOW, don't make unpack-trees to make policy decisions on final 
> resolution, unless it is operating under aggressive rule (where the 
> caller explicitly allows it to make more than the "trivial" decisions).  
> The caller (in this case, merge-recursive) should see A at stage #2 with 
> A/B at stages #1 and #3 and decide what to do.

Okay, so you're saying that merge-recursive should use the aggressive strategy?

Ciao, Dscho