threads / patch / 21399

patchmergetool--lib: add support for p4merge

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

## tl;dr

6 messages between Oct 28, 2009 and Oct 28, 2009. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Jay Soffian· Oct 28, 2009, 09:11 UTC · lore

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(-)
Show changes to 3 files +45 −3

Documentation/git-difftool.txt, Documentation/git-mergetool.txt, git-mergetool--lib.sh

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
Jay Soffian· Oct 28, 2009, 09:21 UTC · re: Jay Soffian · lore

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

On Wed, Oct 28, 2009 at 2:11 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
Show 8 quoted lines
> 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.
David Aguilar· Oct 28, 2009, 09:27 UTC · re: Jay Soffian · lore

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

On Wed, Oct 28, 2009 at 02:21:46AM -0700, Jay Soffian wrote:
Show 11 quoted lines
> 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.
Show 18 quoted lines
> 
> 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
David Aguilar· Oct 28, 2009, 09:36 UTC · re: Jay Soffian · lore

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

On Wed, Oct 28, 2009 at 02:21:46AM -0700, Jay Soffian wrote:
Show 11 quoted lines
> 
> 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
Jay Soffian· Oct 28, 2009, 09:52 UTC · re: David Aguilar · lore

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

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.

Show 11 quoted lines
> 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.
Jay Soffian· Oct 28, 2009, 10:02 UTC · re: Jay Soffian · lore

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

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.

← back to recent threads