Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
On Fri, Jan 25, 2013 at 01:43:54AM -0800, David Aguilar wrote:
> Check the can_diff and can_merge functions before deciding whether to
> add the tool to the available/unavailable lists. This makes --tool-help
> context-
> sensitive so that "git mergetool --tool-help" displays merge tools only
> and "git difftool --tool-help" displays diff tools only.
This log message is misleading - the existing code in
list_merge_tool_candidates already filters the tools like this, so the
change is more:
mergetool--lib: don't use a hardcoded list for "--tool-help"
Instead of using a list of tools in list_merge_tool_candidates, list
the available scriptlets and query each of those to know whether it
applies to diff mode and/or merge mode.
guess_merge_tool still relies on list_merge_tool_candidates so we
can't remove that function now.
The patch seems to do the right thing, although I have a couple of minor
nits...
> Signed-off-by: David Aguilar
> ---
> git-mergetool--lib.sh | 26 +-
> 1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index db8218a..c547c59 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -168,17 +168,33 @@ list_merge_tool_candidates () {
> }
>
> show_tool_help () {
> - list_merge_tool_candidates
> unavailable= available= LF='
> '
> - for i in $tools
> +
> + scriptlets="$(git --exec-path)"/mergetools
> + for i in "$scriptlets"/*
> do
> - merge_tool_path=$(translate_merge_tool_path "$i")
> + . "$scriptlets"/defaults
> + . "$i"
> +
> + tool="$(basename "$i")"
Quotes are unnecessary here.
> + if test "$tool" = "defaults"
> + then
> + continue
> + elif merge_mode && ! can_merge
> + then
> + continue
> + elif diff_mode && ! can_diff
> + then
> + continue
> + fi
Would this be better as:
test "$tool" = "defaults" && continue
can_merge || ! merge_mode || continue
can_diff || ! diff_mode || continue
or is that a bit too concise?
I'd prefer to see two separate if statements either way since the "test
$tool = defaults" case is different from the "does it apply to the
current mode?" case. The "$tool = defaults" case could even move to the
top of the loop.
> + merge_tool_path=$(translate_merge_tool_path "$tool")
> if type "$merge_tool_path" >/dev/null 2>&1
> then
> - available="$available$i$LF"
> + available="$available$tool$LF"
> else
> - unavailable="$unavailable$i$LF"
> + unavailable="$unavailable$tool$LF"
> fi
> done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
John Keeping writes: >> +tool="$(basename "$i")" > > Quotes are unnecessary here. Yeah, the outer quotes aren't needed; the inner ones are. >> +if test "$tool" = "defaults" >> +then >> +continue >> +elif merge_mode && ! can_merge >> +then >> +continue >> +elif diff_mode && ! can_diff >> +then >> +continue >> +fi > > Would this be better as: > > test "$tool" = "defaults" && continue > > can_merge || ! merge_mode || continue > can_diff || ! diff_mode || continue > > or is that a bit too concise? It is beyond "too concise"; it is unreadable, and more importantly, the latter two lines are illogical (why do you even ask if it can be used for merging, before asking merge_mode to see if the answer to that question matters to you?) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
On Fri, Jan 25, 2013 at 07:54:46PM +, John Keeping wrote:
> On Fri, Jan 25, 2013 at 01:43:54AM -0800, David Aguilar wrote:
> > Check the can_diff and can_merge functions before deciding whether to
> > add the tool to the available/unavailable lists. This makes --tool-help
> > context-
> > sensitive so that "git mergetool --tool-help" displays merge tools only
> > and "git difftool --tool-help" displays diff tools only.
>
> This log message is misleading - the existing code in
> list_merge_tool_candidates already filters the tools like this, so the
> change is more:
>
> mergetool--lib: don't use a hardcoded list for "--tool-help"
>
> Instead of using a list of tools in list_merge_tool_candidates, list
> the available scriptlets and query each of those to know whether it
> applies to diff mode and/or merge mode.
>
> guess_merge_tool still relies on list_merge_tool_candidates so we
> can't remove that function now.
>
>
> The patch seems to do the right thing, although I have a couple of minor
> nits...
>
> > Signed-off-by: David Aguilar
> > ---
> > git-mergetool--lib.sh | 26 +-
> > 1 file changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> > index db8218a..c547c59 100644
> > --- a/git-mergetool--lib.sh
> > +++ b/git-mergetool--lib.sh
> > @@ -168,17 +168,33 @@ list_merge_tool_candidates () {
> > }
> >
> > show_tool_help () {
> > - list_merge_tool_candidates
> > unavailable= available= LF='
> > '
> > - for i in $tools
> > +
> > + scriptlets="$(git --exec-path)"/mergetools
> > + for i in "$scriptlets"/*
> > do
> > - merge_tool_path=$(translate_merge_tool_path "$i")
> > + . "$scriptlets"/defaults
> > + . "$i"
> > +
> > + tool="$(basename "$i")"
>
> Quotes are unnecessary here.
>
> > + if test "$tool" = "defaults"
> > + then
> > + continue
> > + elif merge_mode && ! can_merge
> > + then
> > + continue
> > + elif diff_mode && ! can_diff
> > + then
> > + continue
> > + fi
>
> Would this be better as:
>
> test "$tool" = "defaults" && continue
>
> can_merge || ! merge_mode || continue
> can_diff || ! diff_mode || continue
>
> or is that a bit too concise?
>
> I'd prefer to see two separate if statements either way since the "test
> $tool = defaults" case is different from the "does it apply to the
> current mode?" case. The "$tool = defaults" case could even move to the
> top of the loop.
>
> > + merge_tool_path=$(translate_merge_tool_path "$tool")
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
> > if type "$merge_tool_path" >/dev/null 2>&1
> > then
> > - available="$available$i$LF"
> > + available="$available$tool$LF"
> > else
> > - unavailable="$unavailable$i$LF"
> > + unavailable="$unavailable$tool$LF"
> > fi
> > done
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
John Keeping 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. How/when does translate_merge_tool_path fail? > >> >if type "$merge_tool_path" >/dev/null 2>&1 >> >then >> > - available="$available$i$LF" >> > + available="$available$tool$LF" >> >else >> > - unavailable="$unavailable$i$LF" >> > + unavailable="$unavailable$tool$LF" >> >fi >> >done >> -- >> To unsubscribe from this list: send the line "unsubscribe git" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:
> John Keeping 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.
John
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
John Keeping writes:
> On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:
>> John Keeping 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
On Fri, Jan 25, 2013 at 12:56:37PM -0800, Junio C Hamano wrote:
> John Keeping writes:
>
> > On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:
> >> John Keeping 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
John Keeping writes: >> > 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? At the fact that you had to go a convoluted route because you cannot just run setup_tool in subshell and do translate_merge_tool_path after that, because the latter needs to look at the shell variable the former sets. > 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. Of course it is. The current callers of setup_tool may need some adjustments, but that should be fairly trivial, I hope. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
On Fri, Jan 25, 2013 at 01:47:59PM -0800, Junio C Hamano wrote: > John Keeping writes: > > 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. > > Of course it is. The current callers of setup_tool may need some > adjustments, but that should be fairly trivial, I hope. There are only two and one of them already seems like it doesn't want the command to cause the script to exit. David, can you incorporate the following two patches when you re-roll? Your original 7/7 with the change above will want to build on 8/7. John -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
