Re: [PATCH] merge-recursive: handle file mode changes
- From
Clemens Buchacher <drizzd@aon.at>
- Date
- Mar 14, 2008, 12:17 UTC
- Message-ID
- <20080314121752.GB3315@localhost>
- In-Reply-To
- <7vhcf9r4qp.fsf@gitster.siamese.dyndns.org>
On Fri, Mar 14, 2008 at 03:15:26AM -0700, Junio C Hamano wrote:
Show 20 quoted lines
> > 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.
Show 22 quoted lines
> > + 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