threads / patch / 21390

patchmergetool--lib: add p4merge as a pre-configured mergetool option

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

## tl;dr

18 messages between Oct 27, 2009 and Oct 30, 2009. Diffs are folded; open one to read it.

replies: 17people: 7as markdown or json

Scott Chacon· Oct 27, 2009, 22:36 UTC · lore
p4merge is now a built-in diff/merge tool.
This adds p4merge to git-completion and updates
the documentation to mention p4merge.
---
 Documentation/git-difftool.txt         |    2 +-
 Documentation/git-mergetool.txt        |    2 +-
 Documentation/merge-config.txt         |    2 +-
 contrib/completion/git-completion.bash |    2 +-
 git-mergetool--lib.sh                  |   17 +++++++++++++++--
 5 files changed, 19 insertions(+), 6 deletions(-)
Show changes to 5 files +19 −6

Documentation/git-difftool.txt, Documentation/git-mergetool.txt, Documentation/merge-config.txt, contrib/completion/git-completion.bash, 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/Documentation/merge-config.txt b/Documentation/merge-config.txt
index c0f96e7..a403155 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -23,7 +23,7 @@ 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", "araxis", and
+	"diffuse", "ecmerge", "tortoisemerge", "p4merge", "araxis" and
 	"opendiff".  Any other value is treated is custom merge tool
 	and there must be a corresponding mergetool.<tool>.cmd option.

diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index d3fec32..5fb6017 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -953,7 +953,7 @@ _git_diff ()
 }

 __git_mergetools_common="diffuse ecmerge emerge kdiff3 meld opendiff
-			tkdiff vimdiff gvimdiff xxdiff araxis
+			tkdiff vimdiff gvimdiff xxdiff araxis p4merge
 "

 _git_difftool ()
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index bfb01f7..f7c571e 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -46,7 +46,7 @@ check_unchanged () {
 valid_tool () {
 	case "$1" in
 	kdiff3 | tkdiff | xxdiff | meld | opendiff | \
-	emerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis)
+	emerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis | p4merge)
 		;; # happy
 	tortoisemerge)
 		if ! merge_mode; then
@@ -130,6 +130,19 @@ run_merge_tool () {
 			"$merge_tool_path" "$LOCAL" "$REMOTE"
 		fi
 		;;
+	p4merge)
+		if merge_mode; then
+		    touch "$BACKUP"
+			if $base_present; then
+				"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
+			else
+				"$merge_tool_path" "$LOCAL" "$LOCAL" "$REMOTE" "$MERGED"
+			fi
+			check_unchanged
+		else
+			"$merge_tool_path" "$LOCAL" "$REMOTE"
+		fi
+		;;
 	meld)
 		if merge_mode; then
 			touch "$BACKUP"
@@ -323,7 +336,7 @@ guess_merge_tool () {
 		else
 			tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
 		fi
-		tools="$tools gvimdiff diffuse ecmerge araxis"
+		tools="$tools gvimdiff diffuse ecmerge p4merge araxis"
 	fi
 	if echo "${VISUAL:-$EDITOR}" | grep emacs > /dev/null 2>&1; then
 		# $EDITOR is emacs so add emerge as a candidate
-- 
1.6.5.2.75.g16dea.dirty
Charles Bailey· Oct 27, 2009, 23:00 UTC · re: Scott Chacon · lore

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

On Tue, Oct 27, 2009 at 03:36:49PM -0700, Scott Chacon wrote:
> p4merge is now a built-in diff/merge tool.
> This adds p4merge to git-completion and updates
> the documentation to mention p4merge.
> ---

I approve (but haven't had a chance to test this). p4merge is a good mergetool, but now I'll have to find something else as an example that you need to use custom mergetool support for.

I'm just wondering, does this work well with unixes and Mac OS X? I think it's recommended install practice to symlink p4v as p4merge on *nix, but Mac OS X needs some sort of 'launchp4merge' to be called IIRC, or is this something that users can just configure with mergetool.p4diff.path?

-- 
Charles Bailey
http://ccgi.hashpling.plus.com/blog/
Junio C Hamano· Oct 28, 2009, 07:18 UTC · re: Charles Bailey · lore

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

Charles Bailey <charles@hashpling.org> writes:
Show 7 quoted lines
> On Tue, Oct 27, 2009 at 03:36:49PM -0700, Scott Chacon wrote:
>> p4merge is now a built-in diff/merge tool.
>> This adds p4merge to git-completion and updates
>> the documentation to mention p4merge.
>> ---
>
> I approve (but haven't had a chance to test this).

Thanks; eventually you two need Sign-off and Acked-by, then, but I sense that an undate to address the points below is in order?

Show 5 quoted lines
> I'm just wondering, does this work well with unixes and Mac OS X? I
> think it's recommended install practice to symlink p4v as p4merge on
> *nix, but Mac OS X needs some sort of 'launchp4merge' to be called
> IIRC, or is this something that users can just configure with
> mergetool.p4diff.path?
David Aguilar· Oct 28, 2009, 09:00 UTC · re: Charles Bailey · lore

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

On Tue, Oct 27, 2009 at 11:00:43PM +0000, Charles Bailey wrote:
Show 9 quoted lines
> On Tue, Oct 27, 2009 at 03:36:49PM -0700, Scott Chacon wrote:
> > p4merge is now a built-in diff/merge tool.
> > This adds p4merge to git-completion and updates
> > the documentation to mention p4merge.
> > ---
> 
> I approve (but haven't had a chance to test this). p4merge is a
> good mergetool, but now I'll have to find something else as an example
> that you need to use custom mergetool support for.
Ditto, looks good to me.
Show 5 quoted lines
> I'm just wondering, does this work well with unixes and Mac OS X? I
> think it's recommended install practice to symlink p4v as p4merge on
> *nix, but Mac OS X needs some sort of 'launchp4merge' to be called
> IIRC, or is this something that users can just configure with
> mergetool.p4diff.path?

I just tested this on Mac OS X with the latest version of p4merge. It worked great.

	$ git config difftool.p4merge.path \
	  /Applications/p4merge.app/Contents/MacOS/p4merge
	$ git difftool -t p4merge HEAD^
So...
Tested-by: David Aguilar <davvid@gmail.com>
P.S.  thanks for the patch, Scott.

Sorry I haven't gotten around to forking progit yet but we did at least get Disney Animation to go with git + github =)

http://github.com/wdas/ptex
(there's only some headers up there right now,
 but we'll have more to share soon)
Have fun,
-- 
		David
Scott Chacon· Oct 28, 2009, 15:37 UTC · re: David Aguilar · lore

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

Hey,
On Wed, Oct 28, 2009 at 2:00 AM, David Aguilar <davvid@gmail.com> wrote:
Show 14 quoted lines
>> I'm just wondering, does this work well with unixes and Mac OS X? I
>> think it's recommended install practice to symlink p4v as p4merge on
>> *nix, but Mac OS X needs some sort of 'launchp4merge' to be called
>> IIRC, or is this something that users can just configure with
>> mergetool.p4diff.path?
>
> I just tested this on Mac OS X with the latest version of
> p4merge.  It worked great.
>
>        $ git config difftool.p4merge.path \
>          /Applications/p4merge.app/Contents/MacOS/p4merge
>
>        $ git difftool -t p4merge HEAD^
>

This is how I have it setup as well and both diff and merge work for me. I had to do a weird thing with passing it $LOCAL twice if there was no merge base since otherwise it does a diff tool instead of a merge tool - the difference is based on the number of arguments, but it seems to work pretty well. I can try it on Linux a bit later, but I'm not sure why launchp4merge would be needed instead of setting the path like this on a Mac - if there is no serious objection, I can resend this with my Signed-Off-By (sorry, I forgot).

Thanks, Scott

Scott Chacon· Oct 28, 2009, 21:39 UTC · re: Scott Chacon · lore

p4merge is now a built-in diff/merge tool. This adds p4merge to git-completion and updates the documentation to mention p4merge.

Signed-Off-By: Scott Chacon <schacon@gmail.com>
---

This is the same patch, but I tested it on Linux as well as Mac and it works fine as long as the [difftool|mergetool].p4merge.path configs are set or it's in your path.

 Documentation/git-difftool.txt         |    2 +-
 Documentation/git-mergetool.txt        |    2 +-
 Documentation/merge-config.txt         |    2 +-
 contrib/completion/git-completion.bash |    2 +-
 git-mergetool--lib.sh                  |   17 +++++++++++++++--
 5 files changed, 19 insertions(+), 6 deletions(-)
Show changes to 5 files +19 −6

Documentation/git-difftool.txt, Documentation/git-mergetool.txt, Documentation/merge-config.txt, contrib/completion/git-completion.bash, 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/Documentation/merge-config.txt b/Documentation/merge-config.txt
index c0f96e7..a403155 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -23,7 +23,7 @@ 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", "araxis", and
+	"diffuse", "ecmerge", "tortoisemerge", "p4merge", "araxis" and
 	"opendiff".  Any other value is treated is custom merge tool
 	and there must be a corresponding mergetool.<tool>.cmd option.

diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index d3fec32..5fb6017 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -953,7 +953,7 @@ _git_diff ()
 }

 __git_mergetools_common="diffuse ecmerge emerge kdiff3 meld opendiff
-			tkdiff vimdiff gvimdiff xxdiff araxis
+			tkdiff vimdiff gvimdiff xxdiff araxis p4merge
 "

 _git_difftool ()
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index bfb01f7..f7c571e 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -46,7 +46,7 @@ check_unchanged () {
 valid_tool () {
 	case "$1" in
 	kdiff3 | tkdiff | xxdiff | meld | opendiff | \
-	emerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis)
+	emerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis | p4merge)
 		;; # happy
 	tortoisemerge)
 		if ! merge_mode; then
@@ -130,6 +130,19 @@ run_merge_tool () {
 			"$merge_tool_path" "$LOCAL" "$REMOTE"
 		fi
 		;;
+	p4merge)
+		if merge_mode; then
+		    touch "$BACKUP"
+			if $base_present; then
+				"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
+			else
+				"$merge_tool_path" "$LOCAL" "$LOCAL" "$REMOTE" "$MERGED"
+			fi
+			check_unchanged
+		else
+			"$merge_tool_path" "$LOCAL" "$REMOTE"
+		fi
+		;;
 	meld)
 		if merge_mode; then
 			touch "$BACKUP"
@@ -323,7 +336,7 @@ guess_merge_tool () {
 		else
 			tools="opendiff kdiff3 tkdiff xxdiff meld $tools"
 		fi
-		tools="$tools gvimdiff diffuse ecmerge araxis"
+		tools="$tools gvimdiff diffuse ecmerge p4merge araxis"
 	fi
 	if echo "${VISUAL:-$EDITOR}" | grep emacs > /dev/null 2>&1; then
 		# $EDITOR is emacs so add emerge as a candidate
-- 
1.6.5.2.75.gad2f8
Jay Soffian· Oct 29, 2009, 06:17 UTC · re: Junio C Hamano · lore

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

On Wed, Oct 28, 2009 at 4:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Thanks.  Is Jay happy with this version?

It's good enough, but I'm still going to send a follow up patch that looks in /Applications and $HOME/Applications since I don't think the user should have to set the full path (they don't on other platforms).

So:
Acked-by: Jay Soffian
j.
Charles Bailey· Oct 29, 2009, 22:12 UTC · re: Scott Chacon · lore

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

On Wed, Oct 28, 2009 at 02:39:32PM -0700, Scott Chacon wrote:
Show 10 quoted lines
> p4merge is now a built-in diff/merge tool.
> This adds p4merge to git-completion and updates
> the documentation to mention p4merge.
> 
> Signed-Off-By: Scott Chacon <schacon@gmail.com>
> ---
> 
> This is the same patch, but I tested it on Linux as well as Mac and it
> works fine as long as the [difftool|mergetool].p4merge.path configs
> are set or it's in your path.

I've examined the two patches and I feel more comfortable with Scott's, mainly due to it's simplicity.

I'm not sure I understand why only p4merge on Mac OS X is special, we don't seem to treat any other mergetool specially and we don't seem to need absolute paths anywhere else.

If it's a Mac OS X only thing, can we (and should we) avoid special treatment for p4merge on other platforms?

The only other question I have is what are the merits of using /dev/null as the base vs. a second copy of the local version in the baseless merge case? It's the only other difference between the two p4merge patches that I noticed.

Perhaps we could consider having both p4merge and launchp4merge as separate options?

-- 
Charles Bailey
http://ccgi.hashpling.plus.com/blog/
Jay Soffian· Oct 30, 2009, 00:47 UTC · re: Charles Bailey · lore

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

On Thu, Oct 29, 2009 at 6:12 PM, Charles Bailey <charles@hashpling.org> wrote:
> I'm not sure I understand why only p4merge on Mac OS X is special, we
> don't seem to treat any other mergetool specially and we don't seem to
> need absolute paths anywhere else.
On other platforms, the merge tool is very likely to be in your PATH.

On OS X, p4merge is going to be installed as part of an application bundle (/Applications/p4merge.app or $HOME/Applications/p4merge.app). This is virtually never going to be in a user's PATH.

So in order to provide equivalent behavior for OS X as Linux (i.e., so that you can just specify p4merge as the mergetool without having to provide it's path), we need to look in these additional locations.

> If it's a Mac OS X only thing, can we (and should we) avoid special
> treatment for p4merge on other platforms?

It is a Mac OS X only thing. Yes, we could avoid looking in these locations on other platforms, but why? Using type to look for the executable is virtually no cost. The alternative (calling uname to determine the platform) requires running a separate process.

> The only other question I have is what are the merits of using
> /dev/null as the base vs. a second copy of the local version in the
> baseless merge case? It's the only other difference between the two
> p4merge patches that I noticed.

p4merge's argument handling is stupid, you need to pass it a dummy argument in some cases. A second copy of the local version is probably better than /dev/null. Actually, I think just passing it an empty argument ("") works too.

> Perhaps we could consider having both p4merge and launchp4merge as
> separate options?

I think that would be over-engineered. Decide on one or the other. They have slightly different semantics as I've previously described. Personally I think calling launchp4merge provides a more Mac-like behavior, but honestly it doesn't make much difference.

j.
Markus Heidelberg· Oct 30, 2009, 01:02 UTC · re: Jay Soffian · lore

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

Jay Soffian, 30.10.2009:
Show 6 quoted lines
> On Thu, Oct 29, 2009 at 6:12 PM, Charles Bailey <charles@hashpling.org> wrote:
> > I'm not sure I understand why only p4merge on Mac OS X is special, we
> > don't seem to treat any other mergetool specially and we don't seem to
> > need absolute paths anywhere else.
> 
> On other platforms, the merge tool is very likely to be in your PATH.

He didn't mean p4merge on other platforms, but other merge tools on Mac OS X. What about all the other merge tools already in mergetool--lib? Should they get special handling, too?

Show 7 quoted lines
> On OS X, p4merge is going to be installed as part of an application
> bundle (/Applications/p4merge.app or $HOME/Applications/p4merge.app).
> This is virtually never going to be in a user's PATH.
> 
> So in order to provide equivalent behavior for OS X as Linux (i.e., so
> that you can just specify p4merge as the mergetool without having to
> provide it's path), we need to look in these additional locations.

And for Windows we could add C:\Program Files\MergeToolX\tool.exe for every merge tool.

But where will we end?
Markus
Jay Soffian· Oct 30, 2009, 03:00 UTC · re: Markus Heidelberg · lore

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

On Thu, Oct 29, 2009 at 9:02 PM, Markus Heidelberg <markus.heidelberg@web.de> wrote:

> He didn't mean p4merge on other platforms, but other merge tools on Mac
> OS X. What about all the other merge tools already in mergetool--lib?
> Should they get special handling, too?

If someone wants to scratch that itch, then yes. The default diff tool for OS X has its helper already in /usr/bin (opendiff). p4merge is arguably a better merge tool, and it installs as an app bundle in /Applications. I'm not sure about the other diff tools, I haven't looked.

> And for Windows we could add C:\Program Files\MergeToolX\tool.exe for
> every merge tool.

If it makes those tools easier to use with git, and if someone on Windows wants to scratch that itch, then yes, we should.

> But where will we end?

I don't understand this argument. It's a few lines of code to make git a little friendlier. We end when folks stop contributing patches because either no one cares of there's nothing left to improve.

j.
Markus Heidelberg· Oct 30, 2009, 10:35 UTC · re: Jay Soffian · lore

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

Jay Soffian, 30.10.2009:
Show 17 quoted lines
> On Thu, Oct 29, 2009 at 9:02 PM, Markus Heidelberg
> <markus.heidelberg@web.de> wrote:
> > He didn't mean p4merge on other platforms, but other merge tools on Mac
> > OS X. What about all the other merge tools already in mergetool--lib?
> > Should they get special handling, too?
> 
> If someone wants to scratch that itch, then yes. The default diff tool
> for OS X has its helper already in /usr/bin (opendiff). p4merge is
> arguably a better merge tool, and it installs as an app bundle in
> /Applications. I'm not sure about the other diff tools, I haven't
> looked.
> 
> > And for Windows we could add C:\Program Files\MergeToolX\tool.exe for
> > every merge tool.
> 
> If it makes those tools easier to use with git, and if someone on
> Windows wants to scratch that itch, then yes, we should.

Another possible problem: the user can change the installation destination on Windows. What's the behaviour of Mac OS here? Is the instalation path fixed or changeable?

Markus
Reece Dunn· Oct 30, 2009, 11:25 UTC · re: Markus Heidelberg · lore

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

2009/10/30 Markus Heidelberg <markus.heidelberg@web.de>:
Show 22 quoted lines
> Jay Soffian, 30.10.2009:
>> On Thu, Oct 29, 2009 at 9:02 PM, Markus Heidelberg
>> <markus.heidelberg@web.de> wrote:
>> > He didn't mean p4merge on other platforms, but other merge tools on Mac
>> > OS X. What about all the other merge tools already in mergetool--lib?
>> > Should they get special handling, too?
>>
>> If someone wants to scratch that itch, then yes. The default diff tool
>> for OS X has its helper already in /usr/bin (opendiff). p4merge is
>> arguably a better merge tool, and it installs as an app bundle in
>> /Applications. I'm not sure about the other diff tools, I haven't
>> looked.
>>
>> > And for Windows we could add C:\Program Files\MergeToolX\tool.exe for
>> > every merge tool.
>>
>> If it makes those tools easier to use with git, and if someone on
>> Windows wants to scratch that itch, then yes, we should.
>
> Another possible problem: the user can change the installation
> destination on Windows. What's the behaviour of Mac OS here? Is the
> instalation path fixed or changeable?

For Windows, the program should have an InstallDir or similar registry value in a fixed place in the registry to point to where it is installed (something like HKLM/Software/[Vendor]/[Application]/[Version]).

As for Linux, there is no guarantee that things like p4merge are in the path either. It could be placed under /opt/perforce or /home/perforce.

What would be sensible (for all platforms) is:
  1/  if [difftool|mergetool].toolname.path is set, use that (is this
documented?)
  2/  try looking for the tool in the system path
  3/  try some intelligent guessing
  4/  if none of these work, print out an error message -- ideally,
this should mention the configuration option in (1)

(3) is what is being discussed. It is good that it will work without any user configuration (especially for standard tools installed in standard places), but isn't really a big problem as long as the user is prompted to configure the tool path. Also, I'm not sure how this will work with multiple versions of the tools installed (e.g. vim/gvim and p4merge).

- Reece
Jay Soffian· Oct 30, 2009, 15:17 UTC · re: Reece Dunn · lore

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

On Fri, Oct 30, 2009 at 7:25 AM, Reece Dunn <msclrhd@googlemail.com> wrote:
> 2009/10/30 Markus Heidelberg <markus.heidelberg@web.de>:
>> Another possible problem: the user can change the installation
>> destination on Windows. What's the behaviour of Mac OS here? Is the
>> instalation path fixed or changeable?

This has already been answered. Yes the application can move on OS X, but 9/10 it will be in one of two standard locations. There are ways to find an application regardless of where it is, but it's maybe not worth the platform specific complexity for that 1/10 time.

> For Windows, the program should have an InstallDir or similar registry
> value in a fixed place in the registry to point to where it is
> installed (something like
> HKLM/Software/[Vendor]/[Application]/[Version]).

And if someone wants to contribute the code to grub around the registry on Windows, I'm all for it, as long as it doesn't negatively impact non-Windows users (and similarly for any other platform specific code -- don't impact users of other platforms negatively).

> As for Linux, there is no guarantee that things like p4merge are in
> the path either. It could be placed under /opt/perforce or
> /home/perforce.

No, of course not, but again, looking in PATH is likely to work in the common case. By looking in /Application and $HOME/Applications, that covers the common case on OS X.

Show 7 quoted lines
> What would be sensible (for all platforms) is:
>  1/  if [difftool|mergetool].toolname.path is set, use that (is this
> documented?)
>  2/  try looking for the tool in the system path
>  3/  try some intelligent guessing
>  4/  if none of these work, print out an error message -- ideally,
> this should mention the configuration option in (1)

This is basically what is already done, but (3) isn't yet platform specific in any way, and (4) doesn't mention the config option.

Show 6 quoted lines
> (3) is what is being discussed. It is good that it will work without
> any user configuration (especially for standard tools installed in
> standard places), but isn't really a big problem as long as the user
> is prompted to configure the tool path. Also, I'm not sure how this
> will work with multiple versions of the tools installed (e.g. vim/gvim
> and p4merge).
There's a fixed order of tools, first tool that's found wins.
Oh, and my favorite color paint is blue. :-)
j.
Markus Heidelberg· Oct 30, 2009, 15:30 UTC · re: Jay Soffian · lore

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

Jay Soffian, 30.10.2009:
Show 5 quoted lines
> On Fri, Oct 30, 2009 at 7:25 AM, Reece Dunn <msclrhd@googlemail.com> wrote:
> >  3/  try some intelligent guessing
> 
> This is basically what is already done, but (3) isn't yet platform
> specific in any way

Maybe this can be considered to be implemented. But since it's not p4merge specific, the p4merge patch should firstly be applied without the intelligence. The intelligence may be implemented mergetool agnostic for all at once, if that is possible?

Markus
Charles Bailey· Oct 30, 2009, 17:44 UTC · re: Scott Chacon · lore

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

On Wed, Oct 28, 2009 at 02:39:32PM -0700, Scott Chacon wrote:
Show 6 quoted lines
> p4merge is now a built-in diff/merge tool.
> This adds p4merge to git-completion and updates
> the documentation to mention p4merge.
> 
> Signed-Off-By: Scott Chacon <schacon@gmail.com>
> ---
Acked-by: Charles Bailey <charles@hashpling.org>

I'm aware that we haven't reached full agreement on the best way to make p4merge + git as Mac OS X friendly as possible, but Jay said that this patch is 'good enough' and I agree. If we go with this for now, we're not closing the door to further improvements.

I confirmed a (perhaps very minor) issue with the abspath approach, the parameters are used as the title of the pains in p4merge, so (IMHO) short paths look neater, especially if you've cd'ed down to a low level and don't have a 1920 pixel wide monitor. p4merge does truncate from the left so it's not a big thing.

The other thing which I confirmed is that p4merge on windows doesn't like /dev/null as an explicit parameter (it gets convered to nul: by msys, I believe, but it still doesn't like it).

p4merge does appear to do 'magic' when the base and left are the same parameter (not just the same file - the magic doesn't work if you, say, use a relative path and an absolute path to refer to the same file. This means that it doesn't just take the right changes which is what I feared it might do when I first saw the 'local' 'local' 'remote' pattern.

Charles.
-- 
Charles Bailey
http://ccgi.hashpling.plus.com/blog/
Junio C Hamano· Oct 30, 2009, 18:54 UTC · re: Charles Bailey · lore

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

Charles Bailey <charles@hashpling.org> writes:
Show 14 quoted lines
> On Wed, Oct 28, 2009 at 02:39:32PM -0700, Scott Chacon wrote:
>> p4merge is now a built-in diff/merge tool.
>> This adds p4merge to git-completion and updates
>> the documentation to mention p4merge.
>> 
>> Signed-Off-By: Scott Chacon <schacon@gmail.com>
>> ---
>
> Acked-by: Charles Bailey <charles@hashpling.org>
>
> I'm aware that we haven't reached full agreement on the best way to
> make p4merge + git as Mac OS X friendly as possible, but Jay said that
> this patch is 'good enough' and I agree. If we go with this for now,
> we're not closing the door to further improvements.

Will queue; the peculiarity of MacOS X may be annoying, but the annoyance is not limited to the topic of adding p4merge to this codepath.

← back to recent threads