Re: [PATCH 2/3] mergetool: dissect strings with shell variable magic instead of `expr`

[Junio C Hamano <gitster@pobox.com>]

Johannes Sixt <j6t@kdbg.org> writes:

> git-mergetool spawns an enormous amount of processes. For this reason,
> the test script, t7610, is exceptionally slow, in particular, on
> Windows. Most of the processes are invocations of git, but there are
> also some that can be replaced with shell builtins. Do so with `expr`.

I see these as improvements independent of whatever test may or may
not be slow ;-)  s/^.*/but there are/There are/.  Thanks for working
on it.

>  checkout_staged_file () {
> -	tmpfile=$(expr \
> -		"$(git checkout-index --temp --stage="$1" "$2" 2>/dev/null)" \
> -		: '\([^	]*\)	')

So this wants to grab leading non-HT substring that comes before an
HT; we are trying to grab the name of the temorary picked by the
checkout-index command, the output is ".merge_file_XXXXXX" followed
by HT followed by the original filename "$2".

> +	tmpfile="$(git checkout-index --temp --stage="$1" "$2" 2>/dev/null)" &&
> +	tmpfile=${tmpfile%%'	'*}

And this obviously is an equivalent, at least in the successful
case.  The ".merge_file_XXXXXX" temporary filename never has HT in
it, and we are stripping everything after the first HT.

And this rewrite makes the error behaviour much better.  In the
original, the exit code checked in the next "if test $? -eq 0" is
that of "expr" (i.e. does the pattern match?); with this version, we
are looking at the exit status of the checkout-index command.

Good.

> @@ -255,13 +254,16 @@ merge_file () {
>  		return 1
>  	fi
>  
> -	if BASE=$(expr "$MERGED" : '\(.*\)\.[^/]*$')
> -	then
> -		ext=$(expr "$MERGED" : '.*\(\.[^/]*\)$')
> -	else
> +	# extract file extension from the last path component
> +	case "${MERGED##*/}" in
> +	*.*)
> +		ext=.${MERGED##*.}
> +		BASE=${MERGED%"$ext"}

This rewrite can potentially change the behaviour, when $ext has
glob metacharacters.  Wouldn't BASE=${MERGED%.*} be more faithful
conversion?

> +		;;
> +	*)
>  		BASE=$MERGED
>  		ext=
> -	fi
> +	esac
> @@ -406,7 +408,7 @@ main () {
>  		-t|--tool*)
>  			case "$#,$1" in
>  			*,*=*)
> -				merge_tool=$(expr "z$1" : 'z-[^=]*=\(.*\)')
> +				merge_tool=${1#*=}

OK, we strip leading substring before the first '=' out of "$1" and
the case/esac ensures that there is such an equal '=' sign in "$1",
so the rewrite is correct.

Looks good.  Thanks.