| Message ID | 20181024022500.GA29011@archbookpro.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] mergetool: Accept -g/--[no-]gui as arguments | expand |
Denton Liu <liu.denton@gmail.com> writes: > Subject: Re: [PATCH 1/2] mergetool: Accept -g/--[no-]gui as arguments Other people may point it out, but s/Accept/accept/. > In line with how difftool accepts a -g/--[no-]gui option, make mergetool > accept the same option in order to use the `merge.guitool` variable to > find the default mergetool instead of `merge.tool`. > ... > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > index 9a8b97a2a..e317ae003 100644 > --- a/git-mergetool--lib.sh > +++ b/git-mergetool--lib.sh > @@ -350,17 +350,23 @@ guess_merge_tool () { > } > > get_configured_merge_tool () { > - # Diff mode first tries diff.tool and falls back to merge.tool. > - # Merge mode only checks merge.tool > + # If first argument is true, find the guitool instead > + if [ "$1" = true ] Don't use [] in our shell script (see Documentation/CodingGuidelines); say if "$1" = true instead. > + then > + gui_prefix=gui > + 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 > then > - merge_tool=$(git config diff.tool || git config merge.tool) > + merge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool) > else > - merge_tool=$(git config merge.tool) > + merge_tool=$(git config merge.${gui_prefix}tool) > fi In mode_ok shell function, we seem to be prepared to deal with a case where neither diff_mode nor merge_mode is true. Should this codepath also be prepared to? This is not a comment to point out an issue with this patch. It is a genuine question on the code after (or before for that matter) this patch is applied. Thanks.
On Wed, Oct 24, 2018 at 02:10:14PM +0900, Junio C Hamano wrote: > In mode_ok shell function, we seem to be prepared to deal with a > case where neither diff_mode nor merge_mode is true. Should this > codepath also be prepared to? > According to Documentation/git-mergetool--lib.txt, >Before sourcing 'git-mergetool{litdd}lib', your script must set `TOOL_MODE` >to define the operation mode for the functions listed below. >'diff' and 'merge' are valid values. so I think that we can assume that the one of diff_mode or merge_mode will return true. In any case, it seems like the rest of the code was written under this assumption so if this needs to be changed then the whole library needs to be fixed as well.
On Wed, Oct 24, 2018 at 02:10:14PM +0900, Junio C Hamano wrote: > Denton Liu <liu.denton@gmail.com> writes: > > > Subject: Re: [PATCH 1/2] mergetool: Accept -g/--[no-]gui as arguments > > Other people may point it out, but s/Accept/accept/. > > > In line with how difftool accepts a -g/--[no-]gui option, make mergetool > > accept the same option in order to use the `merge.guitool` variable to > > find the default mergetool instead of `merge.tool`. > > ... > > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > > index 9a8b97a2a..e317ae003 100644 > > --- a/git-mergetool--lib.sh > > +++ b/git-mergetool--lib.sh > > @@ -350,17 +350,23 @@ guess_merge_tool () { > > } > > > > get_configured_merge_tool () { > > - # Diff mode first tries diff.tool and falls back to merge.tool. > > - # Merge mode only checks merge.tool > > + # If first argument is true, find the guitool instead > > + if [ "$1" = true ] > > Don't use [] in our shell script (see Documentation/CodingGuidelines); > say > > if "$1" = true > > instead. Perhaps a small typo dropped "test" -- I think it should be if test "$1" = true > > + # Diff mode first tries diff.(gui)tool and falls back to merge.(gui)tool. > > + # Merge mode only checks merge.(gui)tool > > if diff_mode > > then > > - merge_tool=$(git config diff.tool || git config merge.tool) > > + merge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool) > > else > > - merge_tool=$(git config merge.tool) > > + merge_tool=$(git config merge.${gui_prefix}tool) > > fi > > In mode_ok shell function, we seem to be prepared to deal with a > case where neither diff_mode nor merge_mode is true. Should this > codepath also be prepared to? > > This is not a comment to point out an issue with this patch. It is > a genuine question on the code after (or before for that matter) > this patch is applied. > > Thanks. I think the code is okay. mode_ok is setup that way to allow filtering for the other mode's tools, but this code path is only concerned with getting the default merge tool, which should only ever happen in one of the two modes. The bit about difftool falling back to mergetool's config is a convenience so it does make sense to keep that for guitool as well. The code after this part should handle merge_tool being empty just fine, so once the `[ ... ]` vs `test` bit is updated, please feel free to add: Acked-by: David Aguilar <davvid@gmail.com> cheers,
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt index 3622d6648..0c7975a05 100644 --- a/Documentation/git-mergetool.txt +++ b/Documentation/git-mergetool.txt @@ -79,6 +79,17 @@ success of the resolution after the custom tool has exited. Prompt before each invocation of the merge resolution program to give the user a chance to skip the path. +-g:: +--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`. + +--no-gui:: + This overrides a previous `-g` or `--gui` setting and reads the + default merge tool will be read from the configured `merge.tool` + variable. + -O<orderfile>:: Process files in the order specified in the <orderfile>, which has one shell glob pattern per line. diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 9a8b97a2a..e317ae003 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -350,17 +350,23 @@ guess_merge_tool () { } get_configured_merge_tool () { - # Diff mode first tries diff.tool and falls back to merge.tool. - # Merge mode only checks merge.tool + # If first argument is true, find the guitool instead + if [ "$1" = true ] + then + gui_prefix=gui + 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 then - merge_tool=$(git config diff.tool || git config merge.tool) + merge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool) else - merge_tool=$(git config merge.tool) + merge_tool=$(git config merge.${gui_prefix}tool) fi if test -n "$merge_tool" && ! valid_tool "$merge_tool" then - echo >&2 "git config option $TOOL_MODE.tool set to unknown tool: $merge_tool" + echo >&2 "git config option $TOOL_MODE.${gui_prefix}tool set to unknown tool: $merge_tool" echo >&2 "Resetting to default..." return 1 fi diff --git a/git-mergetool.sh b/git-mergetool.sh index d07c7f387..01b9ad59b 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -9,7 +9,7 @@ # at the discretion of Junio C Hamano. # -USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [-O<orderfile>] [file to merge] ...' +USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [-g|--gui|--no-gui] [-O<orderfile>] [file to merge] ...' SUBDIRECTORY_OK=Yes NONGIT_OK=Yes OPTIONS_SPEC= @@ -389,6 +389,7 @@ print_noop_and_exit () { main () { prompt=$(git config --bool mergetool.prompt) + gui_tool=false guessed_merge_tool=false orderfile= @@ -414,6 +415,12 @@ main () { shift ;; esac ;; + --no-gui) + gui_tool=false + ;; + -g|--gui) + gui_tool=true + ;; -y|--no-prompt) prompt=false ;; @@ -443,7 +450,7 @@ main () { if test -z "$merge_tool" then # Check if a merge tool has been configured - merge_tool=$(get_configured_merge_tool) + merge_tool=$(get_configured_merge_tool $gui_tool) # Try to guess an appropriate merge tool if no tool has been set. if test -z "$merge_tool" then