* [PATCH 1/7] git-mergetool: move show_tool_help to mergetool--lib
2013-01-25 9:43 [PATCH 0/7] mergetool-lib improvements for --tool-help David Aguilar
@ 2013-01-25 9:43 ` David Aguilar
2013-01-25 9:43 ` [PATCH 2/7] git-mergetool: remove redundant assignment David Aguilar
` (5 subsequent siblings)
6 siblings, 0 replies; 30+ messages in thread
From: David Aguilar @ 2013-01-25 9:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Keeping
From: John Keeping <john@keeping.me.uk>
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>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
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.1.367.g22b1720.dirty
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 2/7] git-mergetool: remove redundant assignment
2013-01-25 9:43 [PATCH 0/7] mergetool-lib improvements for --tool-help David Aguilar
2013-01-25 9:43 ` [PATCH 1/7] git-mergetool: move show_tool_help to mergetool--lib David Aguilar
@ 2013-01-25 9:43 ` David Aguilar
2013-01-25 9:43 ` [PATCH 3/7] git-mergetool: don't hardcode 'mergetool' in show_tool_help David Aguilar
` (4 subsequent siblings)
6 siblings, 0 replies; 30+ messages in thread
From: David Aguilar @ 2013-01-25 9:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Keeping
From: John Keeping <john@keeping.me.uk>
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>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
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.1.367.g22b1720.dirty
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 3/7] git-mergetool: don't hardcode 'mergetool' in show_tool_help
2013-01-25 9:43 [PATCH 0/7] mergetool-lib improvements for --tool-help David Aguilar
2013-01-25 9:43 ` [PATCH 1/7] git-mergetool: move show_tool_help to mergetool--lib David Aguilar
2013-01-25 9:43 ` [PATCH 2/7] git-mergetool: remove redundant assignment David Aguilar
@ 2013-01-25 9:43 ` David Aguilar
2013-01-25 9:43 ` [PATCH 4/7] git-difftool: use git-mergetool--lib for "--tool-help" David Aguilar
` (3 subsequent siblings)
6 siblings, 0 replies; 30+ messages in thread
From: David Aguilar @ 2013-01-25 9:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Keeping
From: John Keeping <john@keeping.me.uk>
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>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
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.1.367.g22b1720.dirty
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 4/7] git-difftool: use git-mergetool--lib for "--tool-help"
2013-01-25 9:43 [PATCH 0/7] mergetool-lib improvements for --tool-help David Aguilar
` (2 preceding siblings ...)
2013-01-25 9:43 ` [PATCH 3/7] git-mergetool: don't hardcode 'mergetool' in show_tool_help David Aguilar
@ 2013-01-25 9:43 ` David Aguilar
2013-01-25 9:43 ` [PATCH 5/7] mergetools/vim: Remove redundant diff command David Aguilar
` (2 subsequent siblings)
6 siblings, 0 replies; 30+ messages in thread
From: David Aguilar @ 2013-01-25 9:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Keeping
From: John Keeping <john@keeping.me.uk>
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>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
git-difftool.perl | 55 +++++++------------------------------------------------
1 file changed, 7 insertions(+), 48 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));
+ my $cmd = 'TOOL_MODE=diff';
+ $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"';
+ $cmd .= ' && show_tool_help';
- 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);
+ # 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.1.367.g22b1720.dirty
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 5/7] mergetools/vim: Remove redundant diff command
2013-01-25 9:43 [PATCH 0/7] mergetool-lib improvements for --tool-help David Aguilar
` (3 preceding siblings ...)
2013-01-25 9:43 ` [PATCH 4/7] git-difftool: use git-mergetool--lib for "--tool-help" David Aguilar
@ 2013-01-25 9:43 ` David Aguilar
2013-01-25 9:43 ` [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim David Aguilar
2013-01-25 9:43 ` [PATCH 7/7] mergetool--lib: Improve show_tool_help() output David Aguilar
6 siblings, 0 replies; 30+ messages in thread
From: David Aguilar @ 2013-01-25 9:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Keeping
vimdiff and vimdiff2 differ only by their merge command so remove the
logic in the diff command since it's not actually needed.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
mergetools/vim | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/mergetools/vim b/mergetools/vim
index 619594a..39d0327 100644
--- a/mergetools/vim
+++ b/mergetools/vim
@@ -1,14 +1,6 @@
diff_cmd () {
- case "$1" in
- gvimdiff|vimdiff)
- "$merge_tool_path" -R -f -d \
- -c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
- ;;
- gvimdiff2|vimdiff2)
- "$merge_tool_path" -R -f -d \
- -c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
- ;;
- esac
+ "$merge_tool_path" -R -f -d \
+ -c 'wincmd l' -c 'cd $GIT_PREFIX' "$LOCAL" "$REMOTE"
}
merge_cmd () {
--
1.8.1.1.367.g22b1720.dirty
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim
2013-01-25 9:43 [PATCH 0/7] mergetool-lib improvements for --tool-help David Aguilar
` (4 preceding siblings ...)
2013-01-25 9:43 ` [PATCH 5/7] mergetools/vim: Remove redundant diff command David Aguilar
@ 2013-01-25 9:43 ` David Aguilar
2013-01-25 10:23 ` Sebastian Schuberth
2013-01-25 10:38 ` John Keeping
2013-01-25 9:43 ` [PATCH 7/7] mergetool--lib: Improve show_tool_help() output David Aguilar
6 siblings, 2 replies; 30+ messages in thread
From: David Aguilar @ 2013-01-25 9:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Keeping
"git difftool --tool-help" and "git mergetool --tool-help" incorreclty
list "vim" as being an unavailable tool. This is because they attempt
to find a tool named according to the mergetool scriptlet instead of the Git-
recognized tool name.
vimdiff, vimdiff2, gvimdiff, and gvimdiff2 are all provided by the "vim"
scriptlet. This required git-mergetool--lib to special-case it when
setting up the tool.
Remove the exception for "vim" and allow the scriptlets to be found
naturally by using symlinks to a single "vimdiff" scriptlet. This
causes the --tool-help option to correctly list vimdiff, vimdiff2,
gvimdiff, and gvimdiff2 in its output.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
git-mergetool--lib.sh | 9 +--------
mergetools/gvimdiff | 1 +
mergetools/gvimdiff2 | 1 +
mergetools/{vim => vimdiff} | 0
mergetools/vimdiff2 | 1 +
5 files changed, 4 insertions(+), 8 deletions(-)
create mode 120000 mergetools/gvimdiff
create mode 120000 mergetools/gvimdiff2
rename mergetools/{vim => vimdiff} (100%)
create mode 120000 mergetools/vimdiff2
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 4c1e129..db8218a 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -44,14 +44,7 @@ valid_tool () {
}
setup_tool () {
- case "$1" in
- vim*|gvim*)
- tool=vim
- ;;
- *)
- tool="$1"
- ;;
- esac
+ tool="$1"
mergetools="$(git --exec-path)/mergetools"
# Load the default definitions
diff --git a/mergetools/gvimdiff b/mergetools/gvimdiff
new file mode 120000
index 0000000..2a72558
--- /dev/null
+++ b/mergetools/gvimdiff
@@ -0,0 +1 @@
+vimdiff
\ No newline at end of file
diff --git a/mergetools/gvimdiff2 b/mergetools/gvimdiff2
new file mode 120000
index 0000000..2a72558
--- /dev/null
+++ b/mergetools/gvimdiff2
@@ -0,0 +1 @@
+vimdiff
\ No newline at end of file
diff --git a/mergetools/vim b/mergetools/vimdiff
similarity index 100%
rename from mergetools/vim
rename to mergetools/vimdiff
diff --git a/mergetools/vimdiff2 b/mergetools/vimdiff2
new file mode 120000
index 0000000..2a72558
--- /dev/null
+++ b/mergetools/vimdiff2
@@ -0,0 +1 @@
+vimdiff
\ No newline at end of file
--
1.8.1.1.367.g22b1720.dirty
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim
2013-01-25 9:43 ` [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim David Aguilar
@ 2013-01-25 10:23 ` Sebastian Schuberth
2013-01-25 10:28 ` David Aguilar
2013-01-25 10:38 ` John Keeping
1 sibling, 1 reply; 30+ messages in thread
From: Sebastian Schuberth @ 2013-01-25 10:23 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git, John Keeping
On 2013/01/25 10:43 , David Aguilar wrote:
> Remove the exception for "vim" and allow the scriptlets to be found
> naturally by using symlinks to a single "vimdiff" scriptlet. This
I guess that won't work on platforms where Git does not support
symlinks, then, like Windows. But Windows has (g)vimdiff, so loosing
these on that platform would not be so nice.
--
Sebastian Schuberth
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim
2013-01-25 10:23 ` Sebastian Schuberth
@ 2013-01-25 10:28 ` David Aguilar
2013-01-25 10:34 ` Sebastian Schuberth
0 siblings, 1 reply; 30+ messages in thread
From: David Aguilar @ 2013-01-25 10:28 UTC (permalink / raw)
To: Sebastian Schuberth; +Cc: Junio C Hamano, git, John Keeping
On Fri, Jan 25, 2013 at 2:23 AM, Sebastian Schuberth
<sschuberth@gmail.com> wrote:
> On 2013/01/25 10:43 , David Aguilar wrote:
>
>> Remove the exception for "vim" and allow the scriptlets to be found
>> naturally by using symlinks to a single "vimdiff" scriptlet. This
>
>
> I guess that won't work on platforms where Git does not support symlinks,
> then, like Windows. But Windows has (g)vimdiff, so loosing these on that
> platform would not be so nice.
I thought Git did something sensible there like create a normal file?
If not then I was thinking we could add a provides_tools() function that
each scriptlet could supply.
--
David
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim
2013-01-25 9:43 ` [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim David Aguilar
2013-01-25 10:23 ` Sebastian Schuberth
@ 2013-01-25 10:38 ` John Keeping
2013-01-25 10:40 ` David Aguilar
1 sibling, 1 reply; 30+ messages in thread
From: John Keeping @ 2013-01-25 10:38 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git
On Fri, Jan 25, 2013 at 01:43:53AM -0800, David Aguilar wrote:
> "git difftool --tool-help" and "git mergetool --tool-help" incorreclty
> list "vim" as being an unavailable tool. This is because they attempt
> to find a tool named according to the mergetool scriptlet instead of the Git-
> recognized tool name.
Actually, after my patches both git-difftool and git-mergetool get this
right since list_merge_tool_candidates lists vimdiff and gvimdiff.
> vimdiff, vimdiff2, gvimdiff, and gvimdiff2 are all provided by the "vim"
> scriptlet. This required git-mergetool--lib to special-case it when
> setting up the tool.
>
> Remove the exception for "vim" and allow the scriptlets to be found
> naturally by using symlinks to a single "vimdiff" scriptlet.
I wonder if it would be better to make these single-line scripts instead
of symlinks:
. "$MERGE_TOOLS_DIR"/vimdiff
where we make git-mergetool--lib.sh export:
MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
John
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim
2013-01-25 10:38 ` John Keeping
@ 2013-01-25 10:40 ` David Aguilar
2013-01-25 19:11 ` Junio C Hamano
0 siblings, 1 reply; 30+ messages in thread
From: David Aguilar @ 2013-01-25 10:40 UTC (permalink / raw)
To: John Keeping; +Cc: Junio C Hamano, git
On Fri, Jan 25, 2013 at 2:38 AM, John Keeping <john@keeping.me.uk> wrote:
> On Fri, Jan 25, 2013 at 01:43:53AM -0800, David Aguilar wrote:
>> "git difftool --tool-help" and "git mergetool --tool-help" incorreclty
>> list "vim" as being an unavailable tool. This is because they attempt
>> to find a tool named according to the mergetool scriptlet instead of the Git-
>> recognized tool name.
>
> Actually, after my patches both git-difftool and git-mergetool get this
> right since list_merge_tool_candidates lists vimdiff and gvimdiff.
>
>> vimdiff, vimdiff2, gvimdiff, and gvimdiff2 are all provided by the "vim"
>> scriptlet. This required git-mergetool--lib to special-case it when
>> setting up the tool.
>>
>> Remove the exception for "vim" and allow the scriptlets to be found
>> naturally by using symlinks to a single "vimdiff" scriptlet.
>
> I wonder if it would be better to make these single-line scripts instead
> of symlinks:
>
> . "$MERGE_TOOLS_DIR"/vimdiff
>
> where we make git-mergetool--lib.sh export:
>
> MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
That sounds like the way to go.
--
David
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim
2013-01-25 10:40 ` David Aguilar
@ 2013-01-25 19:11 ` Junio C Hamano
0 siblings, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2013-01-25 19:11 UTC (permalink / raw)
To: David Aguilar; +Cc: John Keeping, git
David Aguilar <davvid@gmail.com> writes:
> On Fri, Jan 25, 2013 at 2:38 AM, John Keeping <john@keeping.me.uk> wrote:
>> On Fri, Jan 25, 2013 at 01:43:53AM -0800, David Aguilar wrote:
>>> "git difftool --tool-help" and "git mergetool --tool-help" incorreclty
>>> list "vim" as being an unavailable tool. This is because they attempt
>>> to find a tool named according to the mergetool scriptlet instead of the Git-
>>> recognized tool name.
>>
>> Actually, after my patches both git-difftool and git-mergetool get this
>> right since list_merge_tool_candidates lists vimdiff and gvimdiff.
>>
>>> vimdiff, vimdiff2, gvimdiff, and gvimdiff2 are all provided by the "vim"
>>> scriptlet. This required git-mergetool--lib to special-case it when
>>> setting up the tool.
>>>
>>> Remove the exception for "vim" and allow the scriptlets to be found
>>> naturally by using symlinks to a single "vimdiff" scriptlet.
>>
>> I wonder if it would be better to make these single-line scripts instead
>> of symlinks:
>>
>> . "$MERGE_TOOLS_DIR"/vimdiff
>>
>> where we make git-mergetool--lib.sh export:
>>
>> MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
>
> That sounds like the way to go.
Yup, I'll expect a reroll of this one and possibly the next one (I
haven't read yet).
1-5/7 looked all very sensible.
Thanks.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
2013-01-25 9:43 [PATCH 0/7] mergetool-lib improvements for --tool-help David Aguilar
` (5 preceding siblings ...)
2013-01-25 9:43 ` [PATCH 6/7] mergetools: Fix difftool/mergetool --tool-help listing for vim David Aguilar
@ 2013-01-25 9:43 ` David Aguilar
2013-01-25 19:54 ` John Keeping
6 siblings, 1 reply; 30+ messages in thread
From: David Aguilar @ 2013-01-25 9:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Keeping
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.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
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")"
+ if test "$tool" = "defaults"
+ then
+ continue
+ elif merge_mode && ! can_merge
+ then
+ continue
+ elif diff_mode && ! can_diff
+ then
+ continue
+ fi
+
+ 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
--
1.8.1.1.367.g22b1720.dirty
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
2013-01-25 9:43 ` [PATCH 7/7] mergetool--lib: Improve show_tool_help() output David Aguilar
@ 2013-01-25 19:54 ` John Keeping
2013-01-25 20:06 ` Junio C Hamano
2013-01-25 20:08 ` John Keeping
0 siblings, 2 replies; 30+ messages in thread
From: John Keeping @ 2013-01-25 19:54 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git
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 <davvid@gmail.com>
> ---
> 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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
2013-01-25 19:54 ` John Keeping
@ 2013-01-25 20:06 ` Junio C Hamano
2013-01-25 20:08 ` John Keeping
1 sibling, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2013-01-25 20:06 UTC (permalink / raw)
To: John Keeping; +Cc: David Aguilar, git
John Keeping <john@keeping.me.uk> 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?)
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
2013-01-25 19:54 ` John Keeping
2013-01-25 20:06 ` Junio C Hamano
@ 2013-01-25 20:08 ` John Keeping
2013-01-25 20:16 ` Junio C Hamano
1 sibling, 1 reply; 30+ messages in thread
From: John Keeping @ 2013-01-25 20:08 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git
On Fri, Jan 25, 2013 at 07:54:46PM +0000, 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 <davvid@gmail.com>
> > ---
> > 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
2013-01-25 20:08 ` John Keeping
@ 2013-01-25 20:16 ` Junio C Hamano
2013-01-25 20:46 ` John Keeping
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2013-01-25 20:16 UTC (permalink / raw)
To: John Keeping; +Cc: David Aguilar, git
John Keeping <john@keeping.me.uk> 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 majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
2013-01-25 20:16 ` Junio C Hamano
@ 2013-01-25 20:46 ` John Keeping
2013-01-25 20:56 ` Junio C Hamano
0 siblings, 1 reply; 30+ messages in thread
From: John Keeping @ 2013-01-25 20:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> 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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
2013-01-25 20:46 ` John Keeping
@ 2013-01-25 20:56 ` Junio C Hamano
2013-01-25 21:16 ` John Keeping
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2013-01-25 20:56 UTC (permalink / raw)
To: John Keeping; +Cc: David Aguilar, git
John Keeping <john@keeping.me.uk> writes:
> On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:
>> John Keeping <john@keeping.me.uk> 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.
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
2013-01-25 20:56 ` Junio C Hamano
@ 2013-01-25 21:16 ` John Keeping
2013-01-25 21:47 ` Junio C Hamano
0 siblings, 1 reply; 30+ messages in thread
From: John Keeping @ 2013-01-25 21:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
On Fri, Jan 25, 2013 at 12:56:37PM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>
> > On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:
> >> John Keeping <john@keeping.me.uk> 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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
2013-01-25 21:16 ` John Keeping
@ 2013-01-25 21:47 ` Junio C Hamano
2013-01-25 22:02 ` John Keeping
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2013-01-25 21:47 UTC (permalink / raw)
To: John Keeping; +Cc: David Aguilar, git
John Keeping <john@keeping.me.uk> 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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
2013-01-25 21:47 ` Junio C Hamano
@ 2013-01-25 22:02 ` John Keeping
2013-01-25 22:03 ` [PATCH 8/7] mergetool--lib: don't call "exit" in setup_tool John Keeping
2013-01-25 22:04 ` [PATCH 9/7] mergetool--lib: fix path lookup in guess_merge_tool John Keeping
0 siblings, 2 replies; 30+ messages in thread
From: John Keeping @ 2013-01-25 22:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
On Fri, Jan 25, 2013 at 01:47:59PM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 8/7] mergetool--lib: don't call "exit" in setup_tool
2013-01-25 22:02 ` John Keeping
@ 2013-01-25 22:03 ` John Keeping
2013-01-26 0:24 ` Junio C Hamano
2013-01-25 22:04 ` [PATCH 9/7] mergetool--lib: fix path lookup in guess_merge_tool John Keeping
1 sibling, 1 reply; 30+ messages in thread
From: John Keeping @ 2013-01-25 22:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
This will make it easier to use setup_tool in places where we expect
that the selected tool will not support the current mode.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
git-mergetool--lib.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 4c1e129..c6bd8ba 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
--
1.8.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 8/7] mergetool--lib: don't call "exit" in setup_tool
2013-01-25 22:03 ` [PATCH 8/7] mergetool--lib: don't call "exit" in setup_tool John Keeping
@ 2013-01-26 0:24 ` Junio C Hamano
2013-01-26 7:01 ` David Aguilar
2013-01-26 12:17 ` [PATCH 1/2 v2] " John Keeping
0 siblings, 2 replies; 30+ messages in thread
From: Junio C Hamano @ 2013-01-26 0:24 UTC (permalink / raw)
To: John Keeping; +Cc: David Aguilar, git
Applying this one on top of 1/7 thru 5/7 and 7/7 seems to break
t7610 rather badly.
--- >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 ---
...
ok 1 - setup
expecting success:
git checkout -b test1 branch1 &&
git submodule update -N &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "" | git mergetool both >/dev/null 2>&1 ) &&
( yes "" | git mergetool file1 file1 ) &&
( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) &&
( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
test "$(cat file1)" = "master updated" &&
test "$(cat file2)" = "master new" &&
test "$(cat subdir/file3)" = "master new sub" &&
test "$(cat submod/bar)" = "branch1 submodule" &&
git commit -m "branch1 resolved with mergetool"
M submod
Switched to a new branch 'test1'
Submodule path 'submod': checked out '39c7f044ed2e6a9cebd5266529badd181c8762b5'
not ok - 2 custom mergetool
#
# git checkout -b test1 branch1 &&
# git submodule update -N &&
# test_must_fail git merge master >/dev/null 2>&1 &&
# ( yes "" | git mergetool both >/dev/null 2>&1 ) &&
# ( yes "" | git mergetool file1 file1 ) &&
# ( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) &&
# ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
# ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
# ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
# ( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
# test "$(cat file1)" = "master updated" &&
# test "$(cat file2)" = "master new" &&
# test "$(cat subdir/file3)" = "master new sub" &&
# test "$(cat submod/bar)" = "branch1 submodule" &&
# git commit -m "branch1 resolved with mergetool"
#
--- 8< ------ 8< ------ 8< ------ 8< ------ 8< ------ 8< ---
Due to ">dev/null 2>&1", all of the error clues are hidden, and I
didn't dig further to see which one was failing (this is why tests
shouldn't do these in general).
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 8/7] mergetool--lib: don't call "exit" in setup_tool
2013-01-26 0:24 ` Junio C Hamano
@ 2013-01-26 7:01 ` David Aguilar
2013-01-26 12:17 ` [PATCH 1/2 v2] " John Keeping
1 sibling, 0 replies; 30+ messages in thread
From: David Aguilar @ 2013-01-26 7:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Keeping, git
On Fri, Jan 25, 2013 at 4:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Applying this one on top of 1/7 thru 5/7 and 7/7 seems to break
> t7610 rather badly.
I just sent a replacement for the vim/symlink issue stuff.
I tried to keep the patch small. John, can you rebase this
patch on top of it?
> --- >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 ---
> ...
> ok 1 - setup
>
> expecting success:
> git checkout -b test1 branch1 &&
> git submodule update -N &&
> test_must_fail git merge master >/dev/null 2>&1 &&
> ( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> ( yes "" | git mergetool file1 file1 ) &&
> ( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) &&
> ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
> ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
> ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
> ( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
> test "$(cat file1)" = "master updated" &&
> test "$(cat file2)" = "master new" &&
> test "$(cat subdir/file3)" = "master new sub" &&
> test "$(cat submod/bar)" = "branch1 submodule" &&
> git commit -m "branch1 resolved with mergetool"
>
> M submod
> Switched to a new branch 'test1'
> Submodule path 'submod': checked out '39c7f044ed2e6a9cebd5266529badd181c8762b5'
> not ok - 2 custom mergetool
> #
> # git checkout -b test1 branch1 &&
> # git submodule update -N &&
> # test_must_fail git merge master >/dev/null 2>&1 &&
> # ( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> # ( yes "" | git mergetool file1 file1 ) &&
> # ( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) &&
> # ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
> # ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
> # ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
> # ( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
> # test "$(cat file1)" = "master updated" &&
> # test "$(cat file2)" = "master new" &&
> # test "$(cat subdir/file3)" = "master new sub" &&
> # test "$(cat submod/bar)" = "branch1 submodule" &&
> # git commit -m "branch1 resolved with mergetool"
> #
> --- 8< ------ 8< ------ 8< ------ 8< ------ 8< ------ 8< ---
>
> Due to ">dev/null 2>&1", all of the error clues are hidden, and I
> didn't dig further to see which one was failing (this is why tests
> shouldn't do these in general).
--
David
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/2 v2] mergetool--lib: don't call "exit" in setup_tool
2013-01-26 0:24 ` Junio C Hamano
2013-01-26 7:01 ` David Aguilar
@ 2013-01-26 12:17 ` John Keeping
2013-01-26 13:50 ` [PATCH 1/2 v3] " John Keeping
1 sibling, 1 reply; 30+ messages in thread
From: John Keeping @ 2013-01-26 12:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
This will make it easier to use setup_tool in places where we expect
that the selected tool will not support the current mode.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
On Fri, Jan 25, 2013 at 04:24:03PM -0800, Junio C Hamano wrote:
> Applying this one on top of 1/7 thru 5/7 and 7/7 seems to break
> t7610 rather badly.
Sorry about that. The 'setup_tool' function should really be called
'setup_builtin_tool' - it isn't necessary when a custom mergetool is
configured and will return 1 when called with an argument that isn't a
builtin tool from $GIT_EXEC_PATH/mergetools.
The change is the second hunk below which now wraps the call to
setup_tool in an if block as well as adding the "|| return".
git-mergetool--lib.sh | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 4c1e129..8a5eaff 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,10 @@ run_merge_tool () {
status=0
# Bring tool-specific functions into scope
- setup_tool "$1"
+ if test -z "$merge_tool_path"
+ then
+ setup_tool "$1" || return
+ fi
if merge_mode
then
--
1.8.1.1.367.ga9c3dd4.dirty
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 1/2 v3] mergetool--lib: don't call "exit" in setup_tool
2013-01-26 12:17 ` [PATCH 1/2 v2] " John Keeping
@ 2013-01-26 13:50 ` John Keeping
0 siblings, 0 replies; 30+ messages in thread
From: John Keeping @ 2013-01-26 13:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
This will make it easier to use setup_tool in places where we expect
that the selected tool will not support the current mode.
We need to introduce a new return code for setup_tool to differentiate
between the case of "the selected tool is invalid" and "the selected
tool is not a built-in" since we must call setup_tool when a custom
'merge.<tool>.path' is configured for a built-in tool but avoid failing
when the configured tool is not a built-in.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
> On Fri, Jan 25, 2013 at 04:24:03PM -0800, Junio C Hamano wrote:
> > Applying this one on top of 1/7 thru 5/7 and 7/7 seems to break
> > t7610 rather badly.
>
> Sorry about that. The 'setup_tool' function should really be called
> 'setup_builtin_tool' - it isn't necessary when a custom mergetool is
> configured and will return 1 when called with an argument that isn't a
> builtin tool from $GIT_EXEC_PATH/mergetools.
>
> The change is the second hunk below which now wraps the call to
> setup_tool in an if block as well as adding the "|| return".
Now that I've run the entire test suite, that still wasn't correct since
it did not correctly handle the case where the user overrides the path
for one of the built-in mergetools.
git-mergetool--lib.sh | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 4c1e129..dd4f088 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -58,7 +58,11 @@ setup_tool () {
. "$mergetools/defaults"
if ! test -f "$mergetools/$tool"
then
- return 1
+ # Use a special return code for this case since we want to
+ # source "defaults" even when an explicit tool path is
+ # configured since the user can use that to override the
+ # default path in the scriptlet.
+ return 2
fi
# Load the redefined functions
@@ -67,11 +71,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
}
@@ -101,6 +105,19 @@ run_merge_tool () {
# Bring tool-specific functions into scope
setup_tool "$1"
+ exitcode=$?
+ case $exitcode in
+ 0)
+ :
+ ;;
+ 2)
+ # The configured tool is not a built-in tool.
+ test -n "$merge_tool_path" || return 1
+ ;;
+ *)
+ return $exitcode
+ ;;
+ esac
if merge_mode
then
--
1.8.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 9/7] mergetool--lib: fix path lookup in guess_merge_tool
2013-01-25 22:02 ` John Keeping
2013-01-25 22:03 ` [PATCH 8/7] mergetool--lib: don't call "exit" in setup_tool John Keeping
@ 2013-01-25 22:04 ` John Keeping
1 sibling, 0 replies; 30+ messages in thread
From: John Keeping @ 2013-01-25 22:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
guess_merge_tool calls translate_merge_tool_path in order to get the
correct name of the tool to check whether it can be found on the user's
system. But this function is designed to be overridden by tool
scriptlets so it does nothing if the relevant scriptlet has not been
sourced.
Fix this by calling setup_tool before doing anything.
Signed-off-by: John Keeping <john@keeping.me.uk>
---
git-mergetool--lib.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index c6bd8ba..46860c5 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -219,6 +219,7 @@ guess_merge_tool () {
# Loop over each candidate and stop when a valid merge tool is found.
for i in $tools
do
+ setup_tool "$i" 2>&1 || continue
merge_tool_path="$(translate_merge_tool_path "$i")"
if type "$merge_tool_path" >/dev/null 2>&1
then
--
1.8.1
^ permalink raw reply related [flat|nested] 30+ messages in thread