* [PATCH 1/4] git-mergetool: move show_tool_help to mergetool--lib
2013-01-24 19:55 [PATCH 0/4] Fix "git difftool --tool-help" John Keeping
@ 2013-01-24 19:55 ` John Keeping
2013-01-24 19:55 ` [PATCH 2/4] git-mergetool: remove redundant assignment John Keeping
` (3 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: John Keeping @ 2013-01-24 19:55 UTC (permalink / raw)
To: git; +Cc: David Aguilar
This is the first step in unifying "git difftool --tool-help" and
"git mergetool --tool-help".
Signed-off-by: John Keeping <john@keeping.me.uk>
---
git-mergetool--lib.sh | 37 +++++++++++++++++++++++++++++++++++++
git-mergetool.sh | 37 -------------------------------------
2 files changed, 37 insertions(+), 37 deletions(-)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index f013a03..89a857f 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -174,6 +174,43 @@ list_merge_tool_candidates () {
esac
}
+show_tool_help () {
+ TOOL_MODE=merge
+ list_merge_tool_candidates
+ unavailable= available= LF='
+'
+ for i in $tools
+ do
+ merge_tool_path=$(translate_merge_tool_path "$i")
+ if type "$merge_tool_path" >/dev/null 2>&1
+ then
+ available="$available$i$LF"
+ else
+ unavailable="$unavailable$i$LF"
+ fi
+ done
+ if test -n "$available"
+ then
+ echo "'git mergetool --tool=<tool>' may be set to one of the following:"
+ echo "$available" | sort | sed -e 's/^/ /'
+ else
+ echo "No suitable tool for 'git mergetool --tool=<tool>' found."
+ fi
+ if test -n "$unavailable"
+ then
+ echo
+ echo 'The following tools are valid, but not currently available:'
+ echo "$unavailable" | sort | sed -e 's/^/ /'
+ fi
+ if test -n "$unavailable$available"
+ then
+ echo
+ echo "Some of the tools listed above only work in a windowed"
+ echo "environment. If run in a terminal-only session, they will fail."
+ fi
+ exit 0
+}
+
guess_merge_tool () {
list_merge_tool_candidates
echo >&2 "merge tool candidates: $tools"
diff --git a/git-mergetool.sh b/git-mergetool.sh
index c50e18a..c0ee9aa 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -315,43 +315,6 @@ merge_file () {
return 0
}
-show_tool_help () {
- TOOL_MODE=merge
- list_merge_tool_candidates
- unavailable= available= LF='
-'
- for i in $tools
- do
- merge_tool_path=$(translate_merge_tool_path "$i")
- if type "$merge_tool_path" >/dev/null 2>&1
- then
- available="$available$i$LF"
- else
- unavailable="$unavailable$i$LF"
- fi
- done
- if test -n "$available"
- then
- echo "'git mergetool --tool=<tool>' may be set to one of the following:"
- echo "$available" | sort | sed -e 's/^/ /'
- else
- echo "No suitable tool for 'git mergetool --tool=<tool>' found."
- fi
- if test -n "$unavailable"
- then
- echo
- echo 'The following tools are valid, but not currently available:'
- echo "$unavailable" | sort | sed -e 's/^/ /'
- fi
- if test -n "$unavailable$available"
- then
- echo
- echo "Some of the tools listed above only work in a windowed"
- echo "environment. If run in a terminal-only session, they will fail."
- fi
- exit 0
-}
-
prompt=$(git config --bool mergetool.prompt || echo true)
while test $# != 0
--
1.8.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 2/4] git-mergetool: remove redundant assignment
2013-01-24 19:55 [PATCH 0/4] Fix "git difftool --tool-help" John Keeping
2013-01-24 19:55 ` [PATCH 1/4] git-mergetool: move show_tool_help to mergetool--lib John Keeping
@ 2013-01-24 19:55 ` John Keeping
2013-01-24 19:55 ` [PATCH 3/4] git-mergetool: don't hardcode 'mergetool' in show_tool_help John Keeping
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: John Keeping @ 2013-01-24 19:55 UTC (permalink / raw)
To: git; +Cc: David Aguilar
TOOL_MODE is set at the top of git-mergetool.sh so there is no need to
set it again in show_tool_help. Removing this lets us re-use
show_tool_help in git-difftool.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
git-mergetool--lib.sh | 1 -
1 file changed, 1 deletion(-)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 89a857f..1748315 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -175,7 +175,6 @@ list_merge_tool_candidates () {
}
show_tool_help () {
- TOOL_MODE=merge
list_merge_tool_candidates
unavailable= available= LF='
'
--
1.8.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 3/4] git-mergetool: don't hardcode 'mergetool' in show_tool_help
2013-01-24 19:55 [PATCH 0/4] Fix "git difftool --tool-help" John Keeping
2013-01-24 19:55 ` [PATCH 1/4] git-mergetool: move show_tool_help to mergetool--lib John Keeping
2013-01-24 19:55 ` [PATCH 2/4] git-mergetool: remove redundant assignment John Keeping
@ 2013-01-24 19:55 ` John Keeping
2013-01-24 19:55 ` [PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help" John Keeping
2013-01-24 21:07 ` [PATCH 0/4] Fix "git difftool --tool-help" Junio C Hamano
4 siblings, 0 replies; 10+ messages in thread
From: John Keeping @ 2013-01-24 19:55 UTC (permalink / raw)
To: git; +Cc: David Aguilar
When using show_tool_help from git-difftool we will want it to print
"git difftool" not "git mergetool" so use "git ${TOOL_MODE}tool".
Signed-off-by: John Keeping <john@keeping.me.uk>
---
git-mergetool--lib.sh | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 1748315..4c1e129 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -188,12 +188,14 @@ show_tool_help () {
unavailable="$unavailable$i$LF"
fi
done
+
+ cmd_name=${TOOL_MODE}tool
if test -n "$available"
then
- echo "'git mergetool --tool=<tool>' may be set to one of the following:"
+ echo "'git $cmd_name --tool=<tool>' may be set to one of the following:"
echo "$available" | sort | sed -e 's/^/ /'
else
- echo "No suitable tool for 'git mergetool --tool=<tool>' found."
+ echo "No suitable tool for 'git $cmd_name --tool=<tool>' found."
fi
if test -n "$unavailable"
then
--
1.8.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help"
2013-01-24 19:55 [PATCH 0/4] Fix "git difftool --tool-help" John Keeping
` (2 preceding siblings ...)
2013-01-24 19:55 ` [PATCH 3/4] git-mergetool: don't hardcode 'mergetool' in show_tool_help John Keeping
@ 2013-01-24 19:55 ` John Keeping
2013-01-25 5:29 ` David Aguilar
2013-01-24 21:07 ` [PATCH 0/4] Fix "git difftool --tool-help" Junio C Hamano
4 siblings, 1 reply; 10+ messages in thread
From: John Keeping @ 2013-01-24 19:55 UTC (permalink / raw)
To: git; +Cc: David Aguilar
The "--tool-help" option to git-difftool currently displays incorrect
output since it uses the names of the files in
"$GIT_EXEC_PATH/mergetools/" rather than the list of command names in
git-mergetool--lib.
Fix this by simply delegating the "--tool-help" argument to the
show_tool_help function in git-mergetool--lib.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
git-difftool.perl | 57 ++++++++-----------------------------------------------
1 file changed, 8 insertions(+), 49 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index edd0493..0a90de4 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -59,57 +59,16 @@ sub find_worktree
return $worktree;
}
-sub filter_tool_scripts
-{
- my ($tools) = @_;
- if (-d $_) {
- if ($_ ne ".") {
- # Ignore files in subdirectories
- $File::Find::prune = 1;
- }
- } else {
- if ((-f $_) && ($_ ne "defaults")) {
- push(@$tools, $_);
- }
- }
-}
-
sub print_tool_help
{
- my ($cmd, @found, @notfound, @tools);
- my $gitpath = Git::exec_path();
-
- find(sub { filter_tool_scripts(\@tools) }, "$gitpath/mergetools");
-
- foreach my $tool (@tools) {
- $cmd = "TOOL_MODE=diff";
- $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"';
- $cmd .= " && get_merge_tool_path $tool >/dev/null 2>&1";
- $cmd .= " && can_diff >/dev/null 2>&1";
- if (system('sh', '-c', $cmd) == 0) {
- push(@found, $tool);
- } else {
- push(@notfound, $tool);
- }
- }
-
- print << 'EOF';
-'git difftool --tool=<tool>' may be set to one of the following:
-EOF
- print "\t$_\n" for (sort(@found));
-
- print << 'EOF';
-
-The following tools are valid, but not currently available:
-EOF
- print "\t$_\n" for (sort(@notfound));
-
- print << 'EOF';
-
-NOTE: Some of the tools listed above only work in a windowed
-environment. If run in a terminal-only session, they will fail.
-EOF
- exit(0);
+ my $cmd = 'TOOL_MODE=diff';
+ $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"';
+ $cmd .= ' && show_tool_help';
+
+ # See the comment at the bottom of file_diff() for the reason behind
+ # using system() followed by exit() instead of exec().
+ my $rc = system('sh', '-c', $cmd);
+ exit($rc | ($rc >> 8));
}
sub exit_cleanup
--
1.8.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help"
2013-01-24 19:55 ` [PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help" John Keeping
@ 2013-01-25 5:29 ` David Aguilar
2013-01-25 9:19 ` John Keeping
0 siblings, 1 reply; 10+ messages in thread
From: David Aguilar @ 2013-01-25 5:29 UTC (permalink / raw)
To: John Keeping; +Cc: git
On Thu, Jan 24, 2013 at 11:55 AM, John Keeping <john@keeping.me.uk> wrote:
> The "--tool-help" option to git-difftool currently displays incorrect
> output since it uses the names of the files in
> "$GIT_EXEC_PATH/mergetools/" rather than the list of command names in
> git-mergetool--lib.
>
> Fix this by simply delegating the "--tool-help" argument to the
> show_tool_help function in git-mergetool--lib.
Very nice.
One thought I had was that the unified show_tool_help should
probably check TOOL_MODE=diff and skip over the
!can_diff entries.
The current output of "git difftool --tool-help" before your
patches has the problem that it will list tools such as
"tortoisemerge" as "valid but not available" because it
does not differentiate between missing and !can_diff.
> diff --git a/git-difftool.perl b/git-difftool.perl
> index edd0493..0a90de4 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -59,57 +59,16 @@ sub find_worktree
> return $worktree;
> }
>
> -sub filter_tool_scripts
> -{
> - my ($tools) = @_;
> - if (-d $_) {
> - if ($_ ne ".") {
> - # Ignore files in subdirectories
> - $File::Find::prune = 1;
> - }
> - } else {
> - if ((-f $_) && ($_ ne "defaults")) {
> - push(@$tools, $_);
> - }
> - }
> -}
> -
> sub print_tool_help
> {
> - my ($cmd, @found, @notfound, @tools);
> - my $gitpath = Git::exec_path();
> -
> - find(sub { filter_tool_scripts(\@tools) }, "$gitpath/mergetools");
> -
> - foreach my $tool (@tools) {
> - $cmd = "TOOL_MODE=diff";
> - $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"';
> - $cmd .= " && get_merge_tool_path $tool >/dev/null 2>&1";
> - $cmd .= " && can_diff >/dev/null 2>&1";
> - if (system('sh', '-c', $cmd) == 0) {
> - push(@found, $tool);
> - } else {
> - push(@notfound, $tool);
> - }
> - }
This is where we conflated can_diff with "is the tool available?".
The nice thing is that the old code did actually check can_diff,
but since it conflated these things it ended up putting them
in the @notfound list. It should ignore those completely, IMO.
That could be a nice follow-up patch. What do you think?
--
David
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help"
2013-01-25 5:29 ` David Aguilar
@ 2013-01-25 9:19 ` John Keeping
2013-01-25 9:55 ` David Aguilar
0 siblings, 1 reply; 10+ messages in thread
From: John Keeping @ 2013-01-25 9:19 UTC (permalink / raw)
To: David Aguilar; +Cc: git
On Thu, Jan 24, 2013 at 09:29:58PM -0800, David Aguilar wrote:
> On Thu, Jan 24, 2013 at 11:55 AM, John Keeping <john@keeping.me.uk> wrote:
> > The "--tool-help" option to git-difftool currently displays incorrect
> > output since it uses the names of the files in
> > "$GIT_EXEC_PATH/mergetools/" rather than the list of command names in
> > git-mergetool--lib.
> >
> > Fix this by simply delegating the "--tool-help" argument to the
> > show_tool_help function in git-mergetool--lib.
>
> Very nice.
>
> One thought I had was that the unified show_tool_help should
> probably check TOOL_MODE=diff and skip over the
> !can_diff entries.
>
> The current output of "git difftool --tool-help" before your
> patches has the problem that it will list tools such as
> "tortoisemerge" as "valid but not available" because it
> does not differentiate between missing and !can_diff.
list_merge_tool_candidates does this for us, so it should Just Work
since we use that to generate the list of tools that we loop over.
John
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help"
2013-01-25 9:19 ` John Keeping
@ 2013-01-25 9:55 ` David Aguilar
2013-01-25 10:47 ` John Keeping
0 siblings, 1 reply; 10+ messages in thread
From: David Aguilar @ 2013-01-25 9:55 UTC (permalink / raw)
To: John Keeping; +Cc: git
On Fri, Jan 25, 2013 at 1:19 AM, John Keeping <john@keeping.me.uk> wrote:
> On Thu, Jan 24, 2013 at 09:29:58PM -0800, David Aguilar wrote:
>> On Thu, Jan 24, 2013 at 11:55 AM, John Keeping <john@keeping.me.uk> wrote:
>> > The "--tool-help" option to git-difftool currently displays incorrect
>> > output since it uses the names of the files in
>> > "$GIT_EXEC_PATH/mergetools/" rather than the list of command names in
>> > git-mergetool--lib.
>> >
>> > Fix this by simply delegating the "--tool-help" argument to the
>> > show_tool_help function in git-mergetool--lib.
>>
>> Very nice.
>>
>> One thought I had was that the unified show_tool_help should
>> probably check TOOL_MODE=diff and skip over the
>> !can_diff entries.
>>
>> The current output of "git difftool --tool-help" before your
>> patches has the problem that it will list tools such as
>> "tortoisemerge" as "valid but not available" because it
>> does not differentiate between missing and !can_diff.
>
> list_merge_tool_candidates does this for us, so it should Just Work
> since we use that to generate the list of tools that we loop over.
Yup, kind of. I looked more closely and found a lot of
special-cases around vim which I've eliminated in the series
I just sent (which includes your patches as its base).
list_merge_tool_candidates() has a bunch of other special cases
for $EDITOR, $DISPLAY, $GNOME-something and such so I think
we should keep using it only for the guess_merge_tool() path.
I honestly want to remove list_merge_tool_candidates every
time I read it, but I recognize that it does serve a purpose
for users who have not configured anything.
In order to be useful for the documentation I think the
code could be refactored a tiny bit more beyond what I've
sent so far. The gathering of available tools can be split
off from the reporting, and then show_tool_help() can use
the gatherer. With what I sent, though, there's at least
a 1:1 correspondance between the name of the scriptlets
and the names accepted by --tool=<tool>, which is the first
step towards doing that.
I have to check out for now but I'll keep poking at this
when the weekend rolls around.
cheers,
--
David
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help"
2013-01-25 9:55 ` David Aguilar
@ 2013-01-25 10:47 ` John Keeping
0 siblings, 0 replies; 10+ messages in thread
From: John Keeping @ 2013-01-25 10:47 UTC (permalink / raw)
To: David Aguilar; +Cc: git
On Fri, Jan 25, 2013 at 01:55:03AM -0800, David Aguilar wrote:
> list_merge_tool_candidates() has a bunch of other special cases
> for $EDITOR, $DISPLAY, $GNOME-something and such so I think
> we should keep using it only for the guess_merge_tool() path.
>
> I honestly want to remove list_merge_tool_candidates every
> time I read it, but I recognize that it does serve a purpose
> for users who have not configured anything.
Actually, I'm not sure it does. I asked one of my colleagues whether
he used git-mergetool the other day and he said no because he couldn't
understand the OS X FileMerge tool and was happier to edit things
manually in vim. I don't think he'd realised that he could configure a
different mergetool.
Perhaps we're trying to be too clever by guessing what the user wants
and should instead exit with a message saying:
You have not configured a merge tool to use. Please select one of
the following tools and configure it using:
git config merge.tool <tool>
These tools are availalble on your system:
...
These tools are supported but unavailable:
...
This may be too much of a regression for current users though.
John
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/4] Fix "git difftool --tool-help"
2013-01-24 19:55 [PATCH 0/4] Fix "git difftool --tool-help" John Keeping
` (3 preceding siblings ...)
2013-01-24 19:55 ` [PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help" John Keeping
@ 2013-01-24 21:07 ` Junio C Hamano
4 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2013-01-24 21:07 UTC (permalink / raw)
To: John Keeping; +Cc: git, David Aguilar
Nice code reduction ;-)
Thanks, will queue.
^ permalink raw reply [flat|nested] 10+ messages in thread