On Fri, Jan 25, 2013 at 12:56:37PM -0800, Junio C Hamano wrote:
> John Keeping <john@xxxxxxxxxxxxx> writes:
>
> > On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:
> >> John Keeping <john@xxxxxxxxxxxxx> writes:
> >>
> >> > Actually, can we just change all of the above part of the loop to:
> >> >
> >> > test "$tool" = defaults && continue
> >> >
> >> > merge_tool_path=$(
> >> > setup_tool "$tool" >/dev/null 2>&1 &&
> >> > translate_merge_tool_path "$tool"
> >> > ) || continue
> >>
> >> Meaning "setup_tool ought to know which mode we are in and should
> >> fail if we are in merge mode and it does not support merging"? That
> >> line of reasoning makes tons of sense to me, compared to this script
> >> implementing that logic for these scriptlets.
> >
> > Yes, that's part of what setup_tool does. It actually calls "exit" if
> > the "mode? && can_mode" test fails, which is why we need to call it in
> > the subshell.
> >
> > I think this would get even better if we add a preparatory patch like
> > this, so we can just call setup_tool and then set merge_tool_path:
> >
> > -- >8 --
> >
> > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> > index 888ae3e..4644cbf 100644
> > --- a/git-mergetool--lib.sh
> > +++ b/git-mergetool--lib.sh
> > @@ -67,11 +67,11 @@ setup_tool () {
> > if merge_mode && ! can_merge
> > then
> > echo "error: '$tool' can not be used to resolve merges" >&2
> > - exit 1
> > + return 1
> > elif diff_mode && ! can_diff
> > then
> > echo "error: '$tool' can only be used to resolve merges" >&2
> > - exit 1
> > + return 1
> > fi
> > return 0
> > }
> > @@ -100,7 +100,7 @@ run_merge_tool () {
> > status=0
> >
> > # Bring tool-specific functions into scope
> > - setup_tool "$1"
> > + setup_tool "$1" || return
> >
> > if merge_mode
> > then
> >
> > -- 8< --
> >
> >> How/when does translate_merge_tool_path fail?
> >
> > It doesn't - the "|| continue" is to catch errors from setup_tool.
>
> Ugh.
Is that targeted at my suggestion at the top of this email or calling
exit in setup_tool?
With the patch above, the block of code at the top becomes:
test "$tool" = defaults && continue
setup_tool "$tool" 2>/dev/null || continue
merge_tool_path=$(translate_merge_tool_path "$tool")
which IMHO is pretty readable.
John
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html