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

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

Johannes Sixt <j6t@kdbg.org> writes:

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

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