Re: [PATCH] merge-recursive: handle file mode changes

[Clemens Buchacher <drizzd@aon.at>]

On Fri, Mar 14, 2008 at 03:15:26AM -0700, Junio C Hamano wrote:
> > diff --git a/merge-recursive.c b/merge-recursive.c
> > index 34e3167..d8938cc 100644
> > --- a/merge-recursive.c
> > +++ b/merge-recursive.c
> > @@ -1025,12 +1025,21 @@ static struct merge_file_info merge_file(struct diff_filespec *o,
> >  			hashcpy(result.sha, b->sha1);
> >  		}
> >  	} else {
> > -		if (!sha_eq(a->sha1, o->sha1) && !sha_eq(b->sha1, o->sha1))
> > -			result.merge = 1;
> > -
> > -		result.mode = a->mode == o->mode ? b->mode: a->mode;
> > +		/*
> > +		 * If mode changed in only one branch, keep the changed
> > +		 * version. Otherwise, keep remote version and create a
> > +		 * conflict.
> > +		 */
> 
> Reading the rest of the function, I notice that it consistently favor "a"
> over "b", when a conflict cannot be reconciled.

Indeed. I think "b" should be favored over "a", however.

> > +		if (a->mode != o->mode && b->mode != o->mode &&
> > +				a->mode != b->mode) {
> > +			result.clean = 0;
> > +			result.mode = b->mode;
> > +		} else
> > +			result.mode = o->mode == a->mode ? b->mode : a->mode;
> 
> I think this is much easier to read:
> 
> 		if (a->mode == b->mode || a->mode == o->mode)
> 			result.mode = b->mode;
> 		else {
> 			result.mode = a->mode;
> 			if (b->mode != o->mode) {
> 				result.clean = 0;
> 				result.merge = 1;
> 			}
> 		}
> 
> We keep "b" only if "a" hasn't touched the mode, or it happens to be the
> same as "a".  Otherwise we take "a" anyway, but taking "a" when "b" also
> touched means we detected an unreconcilable conflict.

Yes, but if "b" and "a" both changed in the same way, we should still set
result.merge = 1, because that's more akin to an automatic merge than a
fast-forward merge, IMO.

Clemens