Re: [PATCH v3 4/6] mergetool: fallback to tool when guitool unavailable

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

Denton Liu <liu.denton@gmail.com> writes:

> In git-difftool, if the tool is called with --gui but `diff.guitool` is
> not set, it falls back to `diff.tool`. Make git-mergetool also fallback
> from `merge.guitool` to `merge.tool` if the former is undefined.
>
> If git-difftool were to use `get_configured_mergetool`, it would also

I agree that the precedence order below makes sense, but I am a bit
confused by "were to use" here.  Do you mean you'll make the change
to make difftool to look at mergetool configuraiton in a later step
in the series?  Or is there a way for the user to say "I want my
difftool to also pay attention to the mergetool configurations" (and
another "I do not want that" option)?  I'll come back to this later.

> get the fallback behaviour in the following precedence:
>
> 1. diff.guitool
> 2. merge.guitool
> 3. diff.tool
> 4. merge.tool
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  Documentation/git-mergetool.txt |  4 +++-
>  git-mergetool--lib.sh           | 27 ++++++++++++++++++---------
>  t/t7610-mergetool.sh            | 19 +++++++++++++++++++
>  3 files changed, 40 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
> index 0c7975a050..6b14702e78 100644
> --- a/Documentation/git-mergetool.txt
> +++ b/Documentation/git-mergetool.txt
> @@ -83,7 +83,9 @@ success of the resolution after the custom tool has exited.
>  --gui::
>  	When 'git-mergetool' is invoked with the `-g` or `--gui` option
>  	the default merge tool will be read from the configured
> -	`merge.guitool` variable instead of `merge.tool`.
> +	`merge.guitool` variable instead of `merge.tool`. If
> +	`merge.guitool` is not set, we will fallback to the tool
> +	configured under `merge.tool`.

Makes sense.  So "mergetool --gui" from the command line would look
at guitool and then tool in the merge section.

> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 68ff26a0f7..ada16acffc 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -350,20 +350,29 @@ guess_merge_tool () {
>  }
>  
>  get_configured_merge_tool () {
> -	# If first argument is true, find the guitool instead
> -	if test "$1" = true
> +	is_gui="$1"
> +	sections="merge"
> +	keys="tool"
> +
> +	if diff_mode
>  	then
> -		gui_prefix=gui
> +		sections="diff $sections"
>  	fi
>  
> -	# Diff mode first tries diff.(gui)tool and falls back to merge.(gui)tool.
> -	# Merge mode only checks merge.(gui)tool
> -	if diff_mode
> +	if "$is_gui" = true
>  	then
> -		merge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)
> -	else
> -		merge_tool=$(git config merge.${gui_prefix}tool)
> +		keys="guitool $keys"
>  	fi

OK, so $sections is "diff merge" for difftool and just "merge" for mergetool.
$tool is "guitool tool" under "--gui", and just "tool" otherwise.

> +	IFS=' '
> +	for key in $keys
> +	do
> +		for section in $sections
> +		do
> +			merge_tool=$(git config $section.$key) && break 2
> +		done
> +	done

And you do up to four iterations to cover the combination in the
precedence order.  Which makes sense.

I am not sure about the wisdom of setting IFS here, though.

As far as I can see, both $keys and $sections do not take any
arbitrary values, but just the two (for each) values you know that
do not have any funny characters, so I am not sure what you are
trying to achieve by that (i.e. benefit is unclear).

As long as the get_configured_merge_tool function is called always
for string_emitted_to_stdout=$(that function), the updated setting
will not leak to the outside world so there is no risk to break its
callers, but it is not immediately obvious if helper functions
called in the remainder of this function are OK with the modified
value of IFS (i.e. safety is not obvious).

Now for the promised "come back to this later", I think you meant
"the get_configured_merge_tool function is already prepared to be
used from difftool in this step and when difftool starts to call it
here is what happens".  But I wonder if it makes the evolution of
the topic easier to follow if you defer it to a later step when you
actually make difftool to start calling it?  In other words, in this
step, your get_configured_merge_tool would look like

	sections=merge

	case "$1" in
	true)
		keys="guitool tool" ;;
	*)
		keys="tool" ;;
	esac

	for key in $keys
	do
		for section in $sections
		do
			merge_tool=$(git config ...) && break 2
		done
	done
	...

and then in a later step (6/6?), the unconditional setting of
sections to 'merge' would be updated so that in diff_mode, you'll
iterate over two sections.

I dunno.