threads / patch / 7295

patchmergetool: Use merge.tool config option.

Subject: [PATCH] mergetool: Use merge.tool config option.

## tl;dr

5 messages between Mar 18, 2007 and Mar 19, 2007. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

James Bowes· Mar 18, 2007, 16:13 UTC · lore

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(-)

Show changes to git-mergetool.sh +0 −4

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

Junio C Hamano· Mar 19, 2007, 00:18 UTC · re: James Bowes · lore

Re: [PATCH] mergetool: Use merge.tool config option.

James Bowes <jbowes@dangerouslyinc.com> writes:

Show 5 quoted lines

> 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).

Show 20 quoted lines

> 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

James Bowes· Mar 19, 2007, 01:11 UTC · re: Junio C Hamano · lore

Re: [PATCH] mergetool: Use merge.tool config option.

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:

Show 5 quoted lines

> 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

Theodore Tso· Mar 19, 2007, 02:32 UTC · re: James Bowes · lore

Re: [PATCH] mergetool: Use merge.tool config option.

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>

Show changes to git-mergetool.sh +9 −4

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

Junio C Hamano· Mar 19, 2007, 04:09 UTC · re: Theodore Tso · lore

Re: [PATCH] mergetool: Use merge.tool config option.

Theodore Tso <tytso@mit.edu> writes:

Show 8 quoted lines

> 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.