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

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

[Scott Chacon]

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

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

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

[Charles Bailey]

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/

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

[Junio C Hamano]

Charles Bailey <charles@hashpling.org> writes:

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

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

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

[David Aguilar]

On Tue, Oct 27, 2009 at 11:00:43PM +0000, Charles Bailey wrote:
> 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.


> 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

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

[Scott Chacon]

Hey,

On Wed, Oct 28, 2009 at 2:00 AM, David Aguilar <davvid@gmail.com> wrote:
>> 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

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

[Scott Chacon]

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

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

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

[Junio C Hamano]

Thanks.  Is Jay happy with this version?

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

[Jay Soffian]

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.

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

[Charles Bailey]

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

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

[Jay Soffian]

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.

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

[Markus Heidelberg]

Jay Soffian, 30.10.2009:
> 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?

> 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

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

[Jay Soffian]

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.

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

[Markus Heidelberg]

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?

Markus

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

[Reece Dunn]

2009/10/30 Markus Heidelberg <markus.heidelberg@web.de>:
> 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

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

[Jay Soffian]

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.

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

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

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

[Markus Heidelberg]

Jay Soffian, 30.10.2009:
> 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

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

[Charles Bailey]

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.

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/

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

[Junio C Hamano]

Charles Bailey <charles@hashpling.org> writes:

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