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

[Denton Liu <liu.denton@gmail.com>]

Hi Junio,

On Thu, Apr 25, 2019 at 12:02:23PM +0900, Junio C Hamano wrote:
> 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.

Correct, it means it'll be done in a future patch (i.e. 6/6).

I guess I wasn't fully clear in the message. I meant something like, "If
`git difftool --gui` were to use..." because difftool currently already
uses this function in the non-gui case.

[snip]
> 
> > +	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).

The reason why IFS is being set is because at the top of mergetool--lib,
we set IFS to '\n'. As a result, without setting IFS, the strings
won't parse properly into the for loop.

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

When I was writing this, I didn't realise that the value of IFS bleeds
out of this function. I'll reroll this to use a helper function just in
case.

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

As stated above, difftool currently uses this function in the non-gui
case. I think that clarifying the log message on my part should make it
easier to understand the evolution of this topic.

Thanks for the careful review,

Denton