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.
--
David
