Re: [PATCH v5 1/1] mergetool: add automerge configuration

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Dec 24, 2020, 00:14 UTC
Message-ID: <5fe3dd62e12f8_7855a2081f@natae.notmuch>
In-Reply-To: <xmqqblek8e94.fsf@gitster.c.googlers.com>

Junio C Hamano wrote:

Show 29 quoted lines

> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > Junio C Hamano wrote:
> >> Felipe Contreras <felipe.contreras@gmail.com> writes:
> >> 
> >> > +auto_merge () {
> >> > +	git merge-file --diff3 --marker-size=7 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3"
> >> > +	if test -s "$DIFF3"
> >> > +	then
> >> 
> >> We do not want to ignore the exit status from the command.  IOW, I
> >> think the above wants to be rather
> >> 
> >> 	if git merge-file ... >"$DIFF3" &&
> >> 	   test -s "$DIFF3"
> >> 	then
> >> 		...
> >
> > That doesn't work.
> >
> > "git merge-file" always returns non-zero status when it succeeds (it's
> > the number of conflicts generated).
> 
> Ah, I forgot about that one.  I think "the number of conflicts" was
> a UI mistake (the original that it mimics is "merge" from RCS suite,
> which uses 1 and 2 for "conflicts" and "trouble") but we know we
> will get conflicts, so it is wrong to expect success from the
> command.  Deliberately ignoring the return status is the right thing
> to do.

I agree. My bet is that nobody is checking the return status of "git merge-file" to find out the number of conflicts. Plus, how can you check the difference between 255 conflicts and error -1?

But that's the situation we are in now.

Show 17 quoted lines

> > What if the original file does have these markers?
> >
> > Which is probably something we should be checking beforehand and not
> > attempt an automerge in those cases.
> 
> Yes, that is a much better approach to avoid unnecessary work.
> 
> When we made the conflict marker length configurable, we were hoping
> that we no longer have to worry about the cases where payload files
> (original or ours or theirs) have lines that are confusingly similar
> to the conflict markers, but because we are interfacing external tools
> that are unaware of the facility, it probably would not help us in
> this case all that much.
> 
> FWIW, we use a fiarly large size for our own files in t/ and
> Documentation/ directories ourselves, and it does help topic branch
> merges somewhat frequently.

We could do something like --marker-size=13 to minimize the chances of that happening.

In that case I would prefer '/^<\{13\} /' (to avoid too many characters). I see those regexes used elsewhere in git, but I don't know how portable that is.

If we wanted to make sure none of those markers remain it's not enough to check for '^[<|=>]{13}', what follows up should be a space, or some delimiter, not another < for example. So maybe '^[<|=>]{13}[^<|=>]'?

So, do we want those three things?

 1. A non-standard marker-size
 2. Check beforehand the existence of those markers and disable
    automerge
 3. Check afterwards the existence of those markers and disable
    automerge

Cheers.

-- 
Felipe Contreras