| Message ID | e975fe4a8b206d8e40f9c4d7cd278fdb5c7358f0.1556142510.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | difftool and mergetool improvements | expand |
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.
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
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`. --no-gui:: This overrides a previous `-g` or `--gui` setting and reads the 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 + + IFS=' ' + for key in $keys + do + for section in $sections + do + merge_tool=$(git config $section.$key) && break 2 + done + done + if test -n "$merge_tool" && ! valid_tool "$merge_tool" then echo >&2 "git config option $TOOL_MODE.${gui_prefix}tool set to unknown tool: $merge_tool" diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh index dad607e186..5b61c10a9c 100755 --- a/t/t7610-mergetool.sh +++ b/t/t7610-mergetool.sh @@ -167,6 +167,25 @@ test_expect_success 'gui mergetool' ' git commit -m "branch1 resolved with mergetool" ' +test_expect_success 'gui mergetool without merge.guitool set falls back to merge.tool' ' + test_when_finished "git reset --hard" && + git checkout -b test$test_count branch1 && + git submodule update -N && + test_must_fail git merge master && + ( yes "" | git mergetool --gui both ) && + ( yes "" | git mergetool -g file1 file1 ) && + ( yes "" | git mergetool --gui file2 "spaced name" ) && + ( yes "" | git mergetool --gui subdir/file3 ) && + ( yes "d" | git mergetool --gui file11 ) && + ( yes "d" | git mergetool --gui file12 ) && + ( yes "l" | git mergetool --gui submod ) && + test "$(cat file1)" = "master updated" && + test "$(cat file2)" = "master new" && + test "$(cat subdir/file3)" = "master new sub" && + test "$(cat submod/bar)" = "branch1 submodule" && + git commit -m "branch1 resolved with mergetool" +' + test_expect_success 'mergetool crlf' ' test_when_finished "git reset --hard" && # This test_config line must go after the above reset line so that
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 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(-)