| Message ID | e928db892e35bcb68fcdb52c9bf7158dbbb46616.1556009181.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: > +get_merge_tool_guessed () { > + is_guessed=false > # Check if a merge tool has been configured > - merge_tool=$(get_configured_merge_tool) > + merge_tool=$(get_configured_merge_tool $GIT_MERGETOOL_GUI) > # Try to guess an appropriate merge tool if no tool has been set. > if test -z "$merge_tool" > then > merge_tool=$(guess_merge_tool) || exit > + is_guessed=true > fi > - echo "$merge_tool" > + echo "$is_guessed:$merge_tool" > +} > + > +get_merge_tool () { > + get_merge_tool_guessed | sed -e 's/^[a-z]*://' > } Yuck. Returning a:b is fine if the main use is to match that string using shell builtins like "test" and "case", but piping to "sed" feels a bit too much overhead. Especially given that the other reader in git-emrgetool.sh is not protected for $merge_tool that has a colon in it. Do not try to be too cute and end up with a hack that is both inefficient and brittle at the same time. Possible alternatives: - Because variables in bourne family of shells are global, the caller can easily peek at $is_guessed; or - One bit "did we guess, or did we get from the user?" boolean choice can sufficiently be conveyed by ending the fuction like so instead: ... fi echo "$merge_tool" test "$is_guessed" = true } > diff --git a/git-mergetool.sh b/git-mergetool.sh > index 01b9ad59b2..63e4da1b2f 100755 > --- a/git-mergetool.sh > +++ b/git-mergetool.sh > @@ -449,14 +449,9 @@ main () { > > if test -z "$merge_tool" > then > - # Check if a merge tool has been configured > - 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 > - merge_tool=$(guess_merge_tool) || exit > - guessed_merge_tool=true > - fi > + IFS=':' read guessed_merge_tool merge_tool <<-EOF > + $(GIT_MERGETOOL_GUI=$gui_tool get_merge_tool_guessed) > + EOF With the "let the return code speak" alternative, this would become something like if merge_tool=$(GIT_MERGETOOL_GUI=$gui_tool; get_merge_tool_guessed) then guessed_merge_tool=true else guessed_merge_tool=false fi I do not know what you are trying with GIT_MERGETOOL_GUI=$gui_tool before the shell function, though. It does not work as one-shot assignment to an environment variable. I _think_ it is to feed the all-caps variable to get_configured_merge_tool that is invoked by the get_merge_tool_guessed function, so it does not have to be exported as an environment in the first place, so in the above illustration, I simply wrote an assignment statement, followed by a separate statement that is a parameterless call of a shell function, separated by a semicolon. > fi > merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)" > merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt index 055550b2bc..343268d885 100644 --- a/Documentation/git-mergetool--lib.txt +++ b/Documentation/git-mergetool--lib.txt @@ -27,8 +27,15 @@ to define the operation mode for the functions listed below. FUNCTIONS --------- +get_merge_tool_guessed:: + returns '$is_guessed:$merge_tool'. '$is_guessed' is 'true' if + the tool was guessed, else 'false'. '$merge_tool' is the merge + tool to use. '$GIT_MERGETOOL_GUI' may be set to 'true' to search + for the appropriate guitool. + get_merge_tool:: - returns a merge tool. + returns a merge tool. '$GIT_MERGETOOL_GUI' may be set to 'true' + to search for the appropriate guitool. get_merge_tool_cmd:: returns the custom command for a merge tool. diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 83bf52494c..5eedb1a08a 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -402,15 +402,21 @@ get_merge_tool_path () { echo "$merge_tool_path" } -get_merge_tool () { +get_merge_tool_guessed () { + is_guessed=false # Check if a merge tool has been configured - merge_tool=$(get_configured_merge_tool) + merge_tool=$(get_configured_merge_tool $GIT_MERGETOOL_GUI) # Try to guess an appropriate merge tool if no tool has been set. if test -z "$merge_tool" then merge_tool=$(guess_merge_tool) || exit + is_guessed=true fi - echo "$merge_tool" + echo "$is_guessed:$merge_tool" +} + +get_merge_tool () { + get_merge_tool_guessed | sed -e 's/^[a-z]*://' } mergetool_find_win32_cmd () { diff --git a/git-mergetool.sh b/git-mergetool.sh index 01b9ad59b2..63e4da1b2f 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -449,14 +449,9 @@ main () { if test -z "$merge_tool" then - # Check if a merge tool has been configured - 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 - merge_tool=$(guess_merge_tool) || exit - guessed_merge_tool=true - fi + IFS=':' read guessed_merge_tool merge_tool <<-EOF + $(GIT_MERGETOOL_GUI=$gui_tool get_merge_tool_guessed) + EOF fi merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)" merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
In git-mergetool, the logic for getting which merge tool to use is duplicated in git-mergetool--lib, except for the fact that it needs to know whether the tool was guessed or not. Write `get_merge_tool_guessed` to return whether or not the tool was guessed in addition to the actual tool and make git-mergetool call this function instead of duplicating the logic. Also, let `$GIT_MERGETOOL_GUI` be set to determine whether or not the guitool will be selected. Make `get_merge_tool` use this function internally so that code duplication is reduced. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- After thinking about it for a while, I realised that if it was easy to find one (albeit old) public project using our code, there should be many others who we don't know about that will also be using our code. Let's save them the trouble and just introduce a new function instead of changing the behaviour of the old one. --- Documentation/git-mergetool--lib.txt | 9 ++++++++- git-mergetool--lib.sh | 12 +++++++++--- git-mergetool.sh | 11 +++-------- 3 files changed, 20 insertions(+), 12 deletions(-)