Hi Junio, 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. [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
