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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 23, 2020, 13:34 UTC
Message-ID: <xmqqblekabof.fsf@gitster.c.googlers.com>
In-Reply-To: <20201223045358.100754-2-felipe.contreras@gmail.com>

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

to catch a merge-file that writes halfway and then crashes (doing the same check in different ways are probably possible, of course)

> +		sed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\r\?$/,/^>>>>>>> /d' "$DIFF3" >"$BASE"

Does everybody's sed take "\?" and interprets it as zero-or-one? POSIX uses BRE and it doesn't like \? as far as I recall, and "-E" to force ERE is a GNUism.

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

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

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