Re: [PATCH v3 4/6] mergetool: fallback to tool when guitool unavailable
- Date: Wed, 24 Apr 2019 22:16:20 -0700
- From: Denton Liu <liu.denton@xxxxxxxxx>
- Subject: Re: [PATCH v3 4/6] mergetool: fallback to tool when guitool unavailable
On Thu, Apr 25, 2019 at 12:02:23PM +0900, Junio C Hamano wrote:
> Denton Liu <liu.denton@xxxxxxxxx> 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.
> > + 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
> 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
> case "$1" in
> keys="guitool tool" ;;
> keys="tool" ;;
> for key in $keys
> for section in $sections
> merge_tool=$(git config ...) && break 2
> 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,