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

[Felipe Contreras <felipe.contreras@gmail.com>]

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).

> > +		sed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\r\?$/,/^>>>>>>> /d' "$DIFF3" >"$BASE"
> 
> Does everybody's sed take "\?" and interprets it as zero-or-one?

I don't know.

> POSIX uses BRE and it doesn't like \? as far as I recall, and "-E"
> to force ERE is a GNUism.

Another possibility is \s\*. It's less specific though.

> > +		sed -e '/^||||||| /,/^>>>>>>> /d' -e '/^<<<<<<< /d' "$DIFF3" >"$LOCAL"
> > +		sed -e '/^<<<<<<< /,/^=======\r\?$/d' -e '/^>>>>>>> /d' "$DIFF3" >"$REMOTE"
> 
> I'd feel safer if these resulting $BASE, $LOCAL and $REMOTE are
> validated to be conflict-marker free (i.e. '^\([<|=>]\)\1\1\1\1\1\1'
> does not appear) to make sure there was no funny virtual ancestor
> that records a conflicted recursive merge result confused our logic.
> 
> When we see an unfortunate sign that it happened, we can revert the
> automerge and let the tool handle the original input.

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.

Or we could add the --base option to "git merge-file" so we don't have
to do that work by hand.

> > +	fi
> > +	rm -- "$DIFF3"
> > +}
> > +
> 
> "$DIFF3" is always created (unless shell redirection into it fails),
> so "rm --" would be fine in practice, I guess, but "rm -f --" would
> not hurt.

I just did the same as below:

  rm -- "$BACKUP"

> > +	DIFF3="$MERGETOOL_TMPDIR/${BASE}_DIFF3_$$$ext"
> 
> $MERGETOOL_TMPDIR is either "mktemp -d -t "git-mergetool-XXXXXX" or
> ".".  Also, we liberally pass "$DIFF3" to "sed" as an argument and
> assume that the command would take it as a filename and not an
> option.
> 
> For the above reason, "rm --", while it is not wrong per-se, can be
> just a simple "rm", as there is no funny leading letters in "$DIFF3"
> that requires disambiguation.

Other parts of the file do this:

  rm -f -- "$LOCAL" "$REMOTE" "$BASE" "$BACKUP"

I'm just following what the script already does.

Cheers.

-- 
Felipe Contreras