Re: [PATCH v4 14/14] mergetool: fix running in subdir when rerere enabled

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 10, 2017, 19:25 UTC
Message-ID: <xmqqmveyson3.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <3a09e318-f14b-5f14-a992-3bd045f0a4c6@kdbg.org>

Johannes Sixt <j6t@kdbg.org> writes:

Show 12 quoted lines

>> +	prefix=$(git rev-parse --show-prefix) || exit 1
>> +	cd_to_toplevel
>> +
>> +	if test -n "$orderfile"
>> +	then
>> +		orderfile=$(git rev-parse --prefix "$prefix" -- "$orderfile") || exit 1
>> +		orderfile=$(printf %s\\n "$orderfile" | sed -e 1d)
>
> Is the purpose of this complication only to detect errors of the git
> invocation? IMHO, we could dispense with that, but others might
> disagree. I am arguing because this adds yet another process; but it
> is only paid when -O is used, so...

I do not terribly mind an added process, but this change makes it harder to read and also forces the readers to wonder if the quoting around printf is correct.

Show 15 quoted lines

>> @@ -461,14 +470,17 @@ main () {
>>  		then
>>  			print_noop_and_exit
>>  		fi
>> +	elif test $# -ge 0
>> +	then
>> +		files_quoted=$(git rev-parse --sq --prefix "$prefix" -- "$@") || exit 1
>> +		eval "set -- $files_quoted"
>
> BTW, the --sq and eval business is not required here. At this point,
> $IFS = $'\n', so
>
> 		set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")
>
> will do. (Except that it would not detect errors.)

I thought you are suggesting not to use --sq but it is still there. I think the original is written in such a way that any letter in each of "$@" is preserved, even an LF in the filename.

Such a pathname probably won't correctly be given from the "rerere remaining" side (i.e. when you are lazy and run mergetool without pathname), so it may appear not to matter, but being able to give an explicit pathname from the command line is a workaround for that, so in that sense, it has value to prepare this side of the codepath to be able to cope with such a pathname.

Unrelated, but I notice that in this:

    eval "set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")"
    shift

"set" will get one "--" from its direct command line argument, followed by "--" that comes out of rev-parse, and then the first pathname (i.e. "$prefix/$1", if "$1" is relative). Then the first "--" is discarded and the second "--" that came out of rev-parse becomes "$1", and the first pathname becomes "$2", etc. We use "shift" to get rid of "--" and move everything down by one.

It is my fault but it is a roundabout way to say:

    eval "set $(git rev-parse --sq --prefix "$prefix" -- "$@")"

I would think, and we should probably want to write this like so. [PATCH v4 02/14] would probably want to be updated as well.

I can locally update them if everybody agrees it is a good idea.