| Message ID | c799e871e2a6a6a7fcca45aad71f5a0c406ba3d7.1556185345.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | difftool and mergetool improvements | expand |
On Thu, Apr 25, 2019 at 02:54:41AM -0700, Denton Liu wrote: > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > index 68ff26a0f7..c4b16c5e59 100644 > --- a/git-mergetool--lib.sh > +++ b/git-mergetool--lib.sh > @@ -350,20 +350,34 @@ 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 This line looks suspect. How about, if test "$is_gui" = true instead? This expression could also be lifted out to an "is_gui" helper function. > 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 > + > + merge_tool=$( > + IFS=' ' > + for key in $keys > + do > + for section in $sections > + do > + if selected=$(git config $section.$key) Would it be simpler to split this conditional into two lines? selected=$(git config ...) if test -n "$selected" then ... fi Yes, it stops looking at the exit code, but it instead focuses on the result, which is slightly more bulletproof against a funky user configuration. Regarding the two loops above, what would it look like if we unrolled the logic and just spelled out the keys up front that it's a little easier to follow? I agree it is nicer from an implementation sense to use loops, but we really shouldn't be planning to extend to more permutations in the future beyond the diff/merge duality, so being explicit and spelling out each config lookup permutation is simpler to understand since we only have 4 states. We should be discouraged from adding any more ;-) Something like, keys= if merge_mode then if gui_mode # probably worth adding this function then keys="merge.guitool merge.tool" else keys="merge.tool" fi else if gui_mode then keys="diff.guitool merge.guitool diff.tool merge.tool" else keys="diff.tool merge.tool" fi fi .. and then just have a single loop over $keys.
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..c4b16c5e59 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -350,20 +350,34 @@ 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 + + merge_tool=$( + IFS=' ' + for key in $keys + do + for section in $sections + do + if selected=$(git config $section.$key) + then + echo "$selected" + return + fi + 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, when called with `--gui`, were to use `get_configured_mergetool` in a future patch, it would also get the fallback behavior in the following precedence: 1. diff.guitool 2. merge.guitool 3. diff.tool 4. merge.tool Note that the behavior for when difftool or mergetool are called without `--gui` should be identical with or without this patch. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- Documentation/git-mergetool.txt | 4 +++- git-mergetool--lib.sh | 32 +++++++++++++++++++++++--------- t/t7610-mergetool.sh | 19 +++++++++++++++++++ 3 files changed, 45 insertions(+), 10 deletions(-)