[PATCH] mergetool--lib: add support for p4merge

[PATCH] mergetool--lib: add support for p4merge

[Jay Soffian]

Add native support for p4merge as a diff / merge tool. There are two
oddities. First, launching p4merge on Mac OS X requires running a helper
shim (which in turn is looked for in a couple common locations, as it's
very unlikely to be in PATH). Second, p4merge seems to require its file
arguments be absolute paths.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
As requested by an Android developer at GitTogether09.

Only tested on Mac OS X 10.6.1. Feedback from anyone running p4merge on other
OS's would be appreciated.

 Documentation/git-difftool.txt  |    2 +-
 Documentation/git-mergetool.txt |    2 +-
 git-mergetool--lib.sh           |   44 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 96a6c51..8e9aed6 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 and araxis.
+	ecmerge, diffuse, opendiff, p4merge and araxis.
 +
 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 68ed6c0..4a6f7f3 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 and araxis.
+	diffuse, tortoisemerge, opendiff, p4merge and araxis.
 +
 If a merge resolution program is not specified, 'git-mergetool'
 will use the configuration variable `merge.tool`.  If the
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index bfb01f7..23b2816 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -7,6 +7,12 @@ merge_mode() {
 	test "$TOOL_MODE" = merge
 }
 
+abspath() {
+	d=$(dirname "$1")
+	d=$(cd "$d" && pwd -P)
+	echo "$d/$(basename "$1")"
+}
+
 translate_merge_tool_path () {
 	case "$1" in
 	vimdiff)
@@ -21,6 +27,19 @@ translate_merge_tool_path () {
 	araxis)
 		echo compare
 		;;
+	p4merge)
+		# Look for the Mac OS X P4Merge shim
+		for app_dir in "/Applications" "$HOME/Applications"
+		do
+				launchp4merge="$app_dir/p4merge.app/Contents/Resources/launchp4merge"
+				if type "$launchp4merge" > /dev/null 2>&1; then
+					echo "$launchp4merge"
+					return 0
+				fi
+		done
+		# Otherwise assume we're not on Mac OS X and just return p4merge
+		echo "$1"
+		;;
 	*)
 		echo "$1"
 		;;
@@ -45,7 +64,7 @@ check_unchanged () {
 
 valid_tool () {
 	case "$1" in
-	kdiff3 | tkdiff | xxdiff | meld | opendiff | \
+	kdiff3 | tkdiff | xxdiff | meld | opendiff | p4merge | \
 	emerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis)
 		;; # happy
 	tortoisemerge)
@@ -284,6 +303,29 @@ run_merge_tool () {
 				>/dev/null 2>&1
 		fi
 		;;
+	p4merge)
+		# At least on Mac OS X p4merge wants absolute paths
+		ABS_LOCAL=$(abspath "$LOCAL")
+		ABS_REMOTE=$(abspath "$REMOTE")
+		if merge_mode; then
+			ABS_MERGED=$(abspath "$MERGED")
+			touch "$BACKUP"
+			if $base_present; then
+				ABS_BASE=$(abspath "$BASE")
+				"$merge_tool_path" \
+					"$ABS_BASE" "$ABS_LOCAL" "$ABS_REMOTE" "$ABS_MERGED" \
+					>/dev/null 2>&1
+			else
+				"$merge_tool_path" \
+					"/dev/null" "$ABS_LOCAL" "$ABS_REMOTE" "$ABS_MERGED" \
+					>/dev/null 2>&1
+			fi
+			check_unchanged
+		else
+			"$merge_tool_path" "$ABS_LOCAL" "$ABS_REMOTE" \
+				>/dev/null 2>&1
+		fi
+		;;
 	*)
 		merge_tool_cmd="$(get_merge_tool_cmd "$1")"
 		if test -z "$merge_tool_cmd"; then
-- 
1.6.5.2.74.g610f9.dirty

Re: [PATCH] mergetool--lib: add support for p4merge

[Jay Soffian]

On Wed, Oct 28, 2009 at 2:11 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> Add native support for p4merge as a diff / merge tool. There are two
> oddities. First, launching p4merge on Mac OS X requires running a helper
> shim (which in turn is looked for in a couple common locations, as it's
> very unlikely to be in PATH). Second, p4merge seems to require its file
> arguments be absolute paths.
>
> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
> ---

Amusing. I didn't see Scott's patch.

But, in my testing, for things to work properly I needed to use
launchp4merge per:

http://kb.perforce.com/AllPerforceApplications/StandAloneClients/P4merge/CommandLineP..rgeOnMacOsX

And I also found things didn't work properly unless I provided an absolute path.

(Aside, the "right" way to launch p4merge, at least on 10.6 would be:

/usr/bin/open -b com.perforce.p4merge -W -n --args <args to p4merge...>

This way OS X's launch services would find p4merge.app wherever it is
on the user's system. But, I think some of these options to open are
10.6 specific and in practice looking in /Applications and
$HOME/Applications I think is a sane enough default.)

j.

Re: [PATCH] mergetool--lib: add support for p4merge

[David Aguilar]

On Wed, Oct 28, 2009 at 02:21:46AM -0700, Jay Soffian wrote:
> On Wed, Oct 28, 2009 at 2:11 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> > Add native support for p4merge as a diff / merge tool. There are two
> > oddities. First, launching p4merge on Mac OS X requires running a helper
> > shim (which in turn is looked for in a couple common locations, as it's
> > very unlikely to be in PATH). Second, p4merge seems to require its file
> > arguments be absolute paths.
> >
> > Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
> > ---
> 
> Amusing. I didn't see Scott's patch.

Apologies Jay!

It's late and I saw Scott listed in the CC: and guessed wrong ;)

Thanks for the patch again, man.  Good stuff.



> 
> But, in my testing, for things to work properly I needed to use
> launchp4merge per:
> 
> http://kb.perforce.com/AllPerforceApplications/StandAloneClients/P4merge/CommandLineP..rgeOnMacOsX
> 
> And I also found things didn't work properly unless I provided an absolute path.
> 
> (Aside, the "right" way to launch p4merge, at least on 10.6 would be:
> 
> /usr/bin/open -b com.perforce.p4merge -W -n --args <args to p4merge...>
> 
> This way OS X's launch services would find p4merge.app wherever it is
> on the user's system. But, I think some of these options to open are
> 10.6 specific and in practice looking in /Applications and
> $HOME/Applications I think is a sane enough default.)
> 
> j.

-- 
		David

Re: [PATCH] mergetool--lib: add support for p4merge

[David Aguilar]

On Wed, Oct 28, 2009 at 02:21:46AM -0700, Jay Soffian wrote:
> 
> But, in my testing, for things to work properly I needed to use
> launchp4merge per:
> 
> http://kb.perforce.com/AllPerforceApplications/StandAloneClients/P4merge/CommandLineP..rgeOnMacOsX
> 
> And I also found things didn't work properly unless I provided an absolute path.
> 
> (Aside, the "right" way to launch p4merge, at least on 10.6 would be:
> 
> /usr/bin/open -b com.perforce.p4merge -W -n --args <args to p4merge...>


:(

I tested on 10.5, so there's definitely some difference in
behavior since difftool.p4merge.path is all that was needed here
(with an absolute path as I mentioned).


> This way OS X's launch services would find p4merge.app wherever it is
> on the user's system. But, I think some of these options to open are
> 10.6 specific and in practice looking in /Applications and
> $HOME/Applications I think is a sane enough default.)

We've stayed away from hard-coding any platform-specific paths
in mergetool--lib in the past.  It's a practicality thing --
trying to guess all of the possible installation locations is
simply untenable.

Here's an old thread where we talked about this in the context
of ecmerge:

http://thread.gmane.org/gmane.comp.version-control.git/118125/focus=118182


Let me know what you think.

-- 
		David

Re: [PATCH] mergetool--lib: add support for p4merge

[Jay Soffian]

On Wed, Oct 28, 2009 at 2:36 AM, David Aguilar <davvid@gmail.com> wrote:
> I tested on 10.5, so there's definitely some difference in
> behavior since difftool.p4merge.path is all that was needed here
> (with an absolute path as I mentioned).

There are two issues here:

1) Is it necessary to use launchp4merge, or is calling p4merge directly okay:

Calling p4merge directly works, but the advantage of calling
launchp4merge instead is that it works a little more elegantly:

- If p4merge is already running, using launchp4merge opens a new
window inside the running instance, instead of  launching p4merge
anew.

- After performing the merge, it is sufficient to just close the merge
window, as opposed to having to quit the p4merge application (which
you have to do if you run p4merge directly).

2) Does p4merge/launchp4merge require absolute paths for its arguments.

I found that it does. So I wonder whether possibly you tested with
difftool, which uses absolute arguments, as opposed to mergetool,
which uses relative arguments. You'd only see the issue with
mergetool.

> We've stayed away from hard-coding any platform-specific paths
> in mergetool--lib in the past.  It's a practicality thing --
> trying to guess all of the possible installation locations is
> simply untenable.
>
> Here's an old thread where we talked about this in the context
> of ecmerge:
>
> http://thread.gmane.org/gmane.comp.version-control.git/118125/focus=118182
>
> Let me know what you think.

Disagree with the conclusion of that thread. On Linux, PATH is very
likely to have the merge/diff binary in it. On OS X, this is highly
unlikely, but there are two very common/likely locations we can look,
/Applications and $HOME/Applications.

j.

Re: [PATCH] mergetool--lib: add support for p4merge

[Jay Soffian]

On Wed, Oct 28, 2009 at 2:52 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> 2) Does p4merge/launchp4merge require absolute paths for its arguments.
>
> I found that it does.

I lie. Running p4merge directly works with relative paths. Launching
p4merge via launchp4merge (or via open) requires absolute paths, which
I guess makes sense.

I'd still lean toward using launchp4merge with absolute paths.

$0.02.

j.