* [PATCH] mergetool: Use merge.tool config option.
@ 2007-03-18 16:13 James Bowes
2007-03-19 0:18 ` Junio C Hamano
0 siblings, 1 reply; 5+ messages in thread
From: James Bowes @ 2007-03-18 16:13 UTC (permalink / raw
To: git; +Cc: junkio, tytso
If no merge program was supplied on the commandline, and the config option
merge.tool was set to a valid value, then mergetool would unset $merge_tool
and instead try to find an installed merge program. This patch removes the code
that unset $merge_tool, so the merge.tool config option will always be used, if
set.
Signed-off-by: James Bowes <jbowes@dangerouslyinc.com>
---
git-mergetool.sh | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 52386a5..19788a1 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -288,10 +288,6 @@ done
if test -z "$merge_tool"; then
merge_tool=`git-config merge.tool`
- if test $merge_tool = kdiff3 -o $merge_tool = tkdiff -o \
- $merge_tool = xxdiff -o $merge_tool = meld ; then
- unset merge_tool
- fi
fi
if test -z "$merge_tool" ; then
--
1.5.0.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mergetool: Use merge.tool config option.
2007-03-18 16:13 [PATCH] mergetool: Use merge.tool config option James Bowes
@ 2007-03-19 0:18 ` Junio C Hamano
2007-03-19 1:11 ` James Bowes
0 siblings, 1 reply; 5+ messages in thread
From: Junio C Hamano @ 2007-03-19 0:18 UTC (permalink / raw
To: James Bowes; +Cc: git, junkio, tytso
James Bowes <jbowes@dangerouslyinc.com> writes:
> If no merge program was supplied on the commandline, and the config option
> merge.tool was set to a valid value, then mergetool would unset $merge_tool
> and instead try to find an installed merge program. This patch removes the code
> that unset $merge_tool, so the merge.tool config option will always be used, if
> set.
The problem description looks correct, but I think the original
meant to reject configuration value for merge_tool that is not
supported with the version of the script (and screwed up).
> Signed-off-by: James Bowes <jbowes@dangerouslyinc.com>
> ---
> git-mergetool.sh | 4 ----
> 1 files changed, 0 insertions(+), 4 deletions(-)
>
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 52386a5..19788a1 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -288,10 +288,6 @@ done
>
> if test -z "$merge_tool"; then
> merge_tool=`git-config merge.tool`
> - if test $merge_tool = kdiff3 -o $merge_tool = tkdiff -o \
> - $merge_tool = xxdiff -o $merge_tool = meld ; then
> - unset merge_tool
> - fi
> fi
>
> if test -z "$merge_tool" ; then
IOW, wouldn't this be a better way?
if test -z "$merge_tool"
then
merge_tool=`git-config merge.tool`
case "$merge_tool" in
kdiff3 | tkdiff | xxdiff | meld | emerge)
;; # happy
*)
echo >&2 "We do not know how to drive $merge_tool"
echo >&2 "Resetting to default..."
unset merge_tool
;;
esac
fi
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mergetool: Use merge.tool config option.
2007-03-19 0:18 ` Junio C Hamano
@ 2007-03-19 1:11 ` James Bowes
2007-03-19 2:32 ` Theodore Tso
0 siblings, 1 reply; 5+ messages in thread
From: James Bowes @ 2007-03-19 1:11 UTC (permalink / raw
To: Junio C Hamano; +Cc: git, tytso
On 3/18/07, Junio C Hamano <junkio@cox.net> wrote:
> The problem description looks correct, but I think the original
> meant to reject configuration value for merge_tool that is not
> supported with the version of the script (and screwed up).
There's a bit later on in mergetool that errors out if you have
provided an unknown merge program (either via the command line or
through your config). The command line and the config ways should
probably behave the same, eh? If so, the case block should be brought
up one level like so:
> IOW, wouldn't this be a better way?
>
> if test -z "$merge_tool"
> then
> merge_tool=`git-config merge.tool`
fi
case "$merge_tool" in
kdiff3 | tkdiff | xxdiff | meld | emerge)
;; # happy
*)
echo >&2 "We do not know how to drive $merge_tool"
echo >&2 "Resetting to default..."
unset merge_tool
;;
esac
And then remove the 'Unknown mergetool' bit.
I think either way is fine since they both let you know that you've
entered gobbledeegook or forgot to install something, so I'll defer to
you all for the choice on which way to go.
-James
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mergetool: Use merge.tool config option.
2007-03-19 1:11 ` James Bowes
@ 2007-03-19 2:32 ` Theodore Tso
2007-03-19 4:09 ` Junio C Hamano
0 siblings, 1 reply; 5+ messages in thread
From: Theodore Tso @ 2007-03-19 2:32 UTC (permalink / raw
To: James Bowes; +Cc: Junio C Hamano, git
It seemed to me that Junio's suggestion made the most amount of sense,
but I tinkered with the with the warning message to make it clear that
the cause of the warning was a bugus tool in the merge.tool
configuration parameter.
This has also been pushed out to git://repo.or.cz/git/mergetool.git
Junio, please pull if you approve...
- Ted
commit d6678c28e30e836449092a2917d4b0bd6254b06c
Author: Theodore Ts'o <tytso@mit.edu>
Date: Sun Mar 18 22:30:10 2007 -0400
mergetool: print an appropriate warning if merge.tool is unknown
Also add support for vimdiff
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 563c5c0..7942fd0 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -288,10 +288,15 @@ done
if test -z "$merge_tool"; then
merge_tool=`git-config merge.tool`
- if test $merge_tool = kdiff3 -o $merge_tool = tkdiff -o \
- $merge_tool = xxdiff -o $merge_tool = meld ; then
- unset merge_tool
- fi
+ case "$merge_tool" in
+ kdiff3 | tkdiff | xxdiff | meld | emerge | vimdiff)
+ ;; # happy
+ *)
+ echo >&2 "git config option merge.tool set to unknown tool: $merge_tool"
+ echo >&2 "Resetting to default..."
+ unset merge_tool
+ ;;
+ esac
fi
if test -z "$merge_tool" ; then
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mergetool: Use merge.tool config option.
2007-03-19 2:32 ` Theodore Tso
@ 2007-03-19 4:09 ` Junio C Hamano
0 siblings, 0 replies; 5+ messages in thread
From: Junio C Hamano @ 2007-03-19 4:09 UTC (permalink / raw
To: Theodore Tso; +Cc: James Bowes, git
Theodore Tso <tytso@mit.edu> writes:
> It seemed to me that Junio's suggestion made the most amount of sense,
> but I tinkered with the with the warning message to make it clear that
> the cause of the warning was a bugus tool in the merge.tool
> configuration parameter.
>
> This has also been pushed out to git://repo.or.cz/git/mergetool.git
>
> Junio, please pull if you approve...
Surely, and thanks. I think your message is much nicer.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2007-03-19 4:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-03-18 16:13 [PATCH] mergetool: Use merge.tool config option James Bowes
2007-03-19 0:18 ` Junio C Hamano
2007-03-19 1:11 ` James Bowes
2007-03-19 2:32 ` Theodore Tso
2007-03-19 4:09 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).