[PATCH] mergetool--lib: add diffmerge as a pre-configured mergetool option

[PATCH] mergetool--lib: add diffmerge as a pre-configured mergetool option

[Jay Soffian]

Add SourceGear DiffMerge to the set of built-in diff/merge tools, and update
bash completion and documentation.
---
 Documentation/git-difftool.txt         |    2 +-
 Documentation/git-mergetool.txt        |    2 +-
 Documentation/merge-config.txt         |    4 ++--
 contrib/completion/git-completion.bash |    2 +-
 git-mergetool--lib.sh                  |   22 ++++++++++++++++++++--
 5 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 8e9aed6..28178da 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -31,7 +31,7 @@ OPTIONS
 	Use the diff tool specified by <tool>.
 	Valid merge tools are:
 	kdiff3, kompare, tkdiff, meld, xxdiff, emerge, vimdiff, gvimdiff,
-	ecmerge, diffuse, opendiff, p4merge and araxis.
+	ecmerge, diffuse, opendiff, p4merge, araxis and diffmerge.
 +
 If a diff tool is not specified, 'git-difftool'
 will use the configuration variable `diff.tool`.  If the
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 4a6f7f3..7f00269 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -27,7 +27,7 @@ OPTIONS
 	Use the merge resolution program specified by <tool>.
 	Valid merge tools are:
 	kdiff3, tkdiff, meld, xxdiff, emerge, vimdiff, gvimdiff, ecmerge,
-	diffuse, tortoisemerge, opendiff, p4merge and araxis.
+	diffuse, tortoisemerge, opendiff, p4merge, araxis and diffmerge.
 +
 If a merge resolution program is not specified, 'git-mergetool'
 will use the configuration variable `merge.tool`.  If the
diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index a403155..a68a205 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -23,8 +23,8 @@ merge.tool::
 	Controls which merge resolution program is used by
 	linkgit:git-mergetool[1].  Valid built-in values are: "kdiff3",
 	"tkdiff", "meld", "xxdiff", "emerge", "vimdiff", "gvimdiff",
-	"diffuse", "ecmerge", "tortoisemerge", "p4merge", "araxis" and
-	"opendiff".  Any other value is treated is custom merge tool
+	"diffuse", "ecmerge", "tortoisemerge", "p4merge", "araxis", "opendiff"
+	and "diffmerge".  Any other value is treated is custom merge tool
 	and there must be a corresponding mergetool.<tool>.cmd option.
 
 merge.verbosity::
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7c18b0c..5cc5ee7 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -975,7 +975,7 @@ _git_diff ()
 }
 
 __git_mergetools_common="diffuse ecmerge emerge kdiff3 meld opendiff
-			tkdiff vimdiff gvimdiff xxdiff araxis p4merge
+			tkdiff vimdiff gvimdiff xxdiff araxis p4merge diffmerge
 "
 
 _git_difftool ()
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 5b62785..5b29fef 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -46,7 +46,8 @@ check_unchanged () {
 valid_tool () {
 	case "$1" in
 	kdiff3 | tkdiff | xxdiff | meld | opendiff | \
-	emerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis | p4merge)
+	emerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis | p4merge | \
+	diffmerge)
 		;; # happy
 	tortoisemerge)
 		if ! merge_mode; then
@@ -297,6 +298,23 @@ run_merge_tool () {
 				>/dev/null 2>&1
 		fi
 		;;
+	diffmerge)
+		if merge_mode; then
+			if $base_present; then
+				"$merge_tool_path" -nosplash -merge -result="$MERGED" \
+					"$LOCAL" "$BASE" "$REMOTE"
+					>/dev/null 2>&1
+			else
+				"$merge_tool_path" -nosplash -merge \
+					"$LOCAL" "$MERGED" "$REMOTE"
+					>/dev/null 2>&1
+			fi
+			status=$?
+		else
+			"$merge_tool_path" -nosplash "$LOCAL" "$REMOTE" \
+				>/dev/null 2>&1
+		fi
+		;;
 	*)
 		merge_tool_cmd="$(get_merge_tool_cmd "$1")"
 		if test -z "$merge_tool_cmd"; then
@@ -336,7 +354,7 @@ guess_merge_tool () {
 		else
 			tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
 		fi
-		tools="$tools gvimdiff diffuse ecmerge p4merge araxis"
+		tools="$tools gvimdiff diffuse ecmerge p4merge araxis diffmerge"
 	fi
 	case "${VISUAL:-$EDITOR}" in
 	*vim*)
-- 
1.6.6.rc1.296.ge77fc.dirty

Re: [PATCH] mergetool--lib: add diffmerge as a pre-configured mergetool option

[Charles Bailey]

On Tue, Dec 08, 2009 at 12:01:17PM -0800, Jay Soffian wrote:
> Add SourceGear DiffMerge to the set of built-in diff/merge tools, and update
> bash completion and documentation.
> ---
>  Documentation/git-difftool.txt         |    2 +-
>  Documentation/git-mergetool.txt        |    2 +-
>  Documentation/merge-config.txt         |    4 ++--
>  contrib/completion/git-completion.bash |    2 +-
>  git-mergetool--lib.sh                  |   22 ++++++++++++++++++++--
>  5 files changed, 25 insertions(+), 7 deletions(-)

I'm not a diffmerge user but the patch looks fine to me.

Is diffmerge free but not Free?

Any reason for holding back on sign-off?

Charles.

Re: [PATCH] mergetool--lib: add diffmerge as a pre-configured mergetool option

[Jay Soffian]

On Wed, Dec 9, 2009 at 2:34 PM, Charles Bailey <charles@hashpling.org> wrote:
> Is diffmerge free but not Free?

Correct, it's free as in beer, not as in speech. It is multi-platform
though (Linux, Windows, OS X). I've tested only on MacOS X. If someone
else wants to give it a test on Linux, that would be good. Looks like
there's both a deb and an rpm:

http://www.sourcegear.com/diffmerge/downloads.html

> Any reason for holding back on sign-off?

Oversight:

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>

j.

Re: [PATCH] mergetool--lib: add diffmerge as a pre-configured mergetool option

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 5b62785..5b29fef 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -46,7 +46,8 @@ check_unchanged () {
>  valid_tool () {
>  	case "$1" in
>  	kdiff3 | tkdiff | xxdiff | meld | opendiff | \
> -	emerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis | p4merge)
> +	emerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis | p4merge | \
> +	diffmerge)
>  		;; # happy
>  	tortoisemerge)
>  		if ! merge_mode; then

As we are in pre-release feature freeze, it doesn't matter very much if I
take this patch now, so I'll let it sit in the list archive for now.

But I have to wonder about the maintainability of this file, if we have to
add every time somebody finds yet another diff/merge backends that could
be used, even a closed-source one.

There are only a handful of entry points that mergetool--lib defines, and
by overriding what should happen when these entry points are called, an end
user should be able to tell mergetool/difftool to use a new backend.

Perhaps it is a better approach to first eject bulk of code for the
backends we currently support under these case statements into separate
files, one per backend, move them to mergetool/ subdirectory in the source
tree, install them as "$(share)/git-core/mergetool/$toolname", and at
runtime source them?  That way, a patch to add a new backend can be as
simple as adding a new file in mergetool/ and doing nothing else.  Also an
end user can privately add support to a new backend much more easily.

Anybody want to try that approach?

Re: [PATCH] mergetool--lib: add diffmerge as a pre-configured mergetool option

[Jay Soffian]

On Wed, Dec 9, 2009 at 4:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Perhaps it is a better approach to first eject bulk of code for the
> backends we currently support under these case statements into separate
> files, one per backend, move them to mergetool/ subdirectory in the source
> tree, install them as "$(share)/git-core/mergetool/$toolname", and at
> runtime source them?  That way, a patch to add a new backend can be as
> simple as adding a new file in mergetool/ and doing nothing else.  Also an
> end user can privately add support to a new backend much more easily.
>
> Anybody want to try that approach?

Sure. There may also be value in emulating the mercurial approach:

  http://mercurial.selenic.com/wiki/MergeToolConfiguration

I'll give both a shot.

j.

Re: [PATCH] mergetool--lib: add diffmerge as a pre-configured mergetool option

[David Aguilar]

(continuing an old thread...)
http://thread.gmane.org/gmane.comp.version-control.git/134906/focus=135006

On Wed, Dec 09, 2009 at 04:08:55PM -0800, Junio C Hamano wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
> 
> > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> > index 5b62785..5b29fef 100644
> > --- a/git-mergetool--lib.sh
> > +++ b/git-mergetool--lib.sh
> > @@ -46,7 +46,8 @@ check_unchanged () {
> >  valid_tool () {
> >  	case "$1" in
> >  	kdiff3 | tkdiff | xxdiff | meld | opendiff | \
> > -	emerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis | p4merge)
> > +	emerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis | p4merge | \
> > +	diffmerge)
> >  		;; # happy
> >  	tortoisemerge)
> >  		if ! merge_mode; then
> 
> As we are in pre-release feature freeze, it doesn't matter very much if I
> take this patch now, so I'll let it sit in the list archive for now.
> 
> But I have to wonder about the maintainability of this file, if we have to
> add every time somebody finds yet another diff/merge backends that could
> be used, even a closed-source one.
> 
> There are only a handful of entry points that mergetool--lib defines, and
> by overriding what should happen when these entry points are called, an end
> user should be able to tell mergetool/difftool to use a new backend.
> 
> Perhaps it is a better approach to first eject bulk of code for the
> backends we currently support under these case statements into separate
> files, one per backend, move them to mergetool/ subdirectory in the source
> tree, install them as "$(share)/git-core/mergetool/$toolname", and at
> runtime source them?  That way, a patch to add a new backend can be as
> simple as adding a new file in mergetool/ and doing nothing else.  Also an
> end user can privately add support to a new backend much more easily.
> 
> Anybody want to try that approach?

I've started working on splitting this file apart and it is
coming along nicely.  I should have some patches soon.

We're approaching feature freeze so I will hold onto them for a
bit.  I worked the "meld should use --output with >= 1.5.0"
check into it too, which worked out nicely with the refactored
setup as that logic is neatly tucked away into the meld file.

Is it okay if we install the files into
$(git --exec-path)/mergetools/ instead?  I didn't spot an
obvious way to construct $(share)/git-core/ at runtime
(without dirname tricks that assume share=$(prefix)/share).
-- 
					David

Re: [PATCH] mergetool--lib: add diffmerge as a pre-configured mergetool option

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> Is it okay if we install the files into
> $(git --exec-path)/mergetools/ instead?

Surely, and thanks.

It is much better than "$(git --exec-path)/mergetools--$backend" ;-).