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

From: Johannes Sixt <j6t@kdbg.org>
Date: Jun 10, 2019, 21:34 UTC
Message-ID: <0a350943-9264-1a40-6452-91a990ec790d@kdbg.org>
In-Reply-To: <xmqqblz5nyve.fsf@gitster-ct.c.googlers.com>

Am 10.06.19 um 19:17 schrieb Junio C Hamano:

Show 9 quoted lines

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

Noted.

Show 17 quoted lines

>> @@ -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?

Since "$ext" is quoted inside the braces of the parameter expansion, the pattern counts as quoted, so all glob characters in $ext lose their special meaning. At least that's how I read the spec.

I do see the symmetry in your proposed version. Nevertheless, I have a slight preference for my version because it specifies exactly what is to be removed from the end of value.

-- Hannes