Denton Liu <liu.denton@xxxxxxxxx> 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)"
