{"thread":{"id":"21390","subject":"[PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","startedAt":"2009-10-27T22:36:49Z","lastAt":"2009-10-30T18:54:47Z","messageCount":18,"participants":["Scott Chacon","Charles Bailey","Junio C Hamano","David Aguilar","Jay Soffian","Markus Heidelberg","Reece Dunn"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"126064","messageId":"d411cc4a0910271536u5817802at43f7477dd8ccabc7@mail.gmail.com","threadId":"21390","inReplyTo":null,"subject":"[PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","fromName":"Scott Chacon","fromEmail":"schacon@gmail.com","sentAt":"2009-10-27T22:36:49Z","receivedAt":"2009-10-27T22:36:49Z","isPatch":true,"sender":{"key":"schacon@gmail.com","avatar":"https://gravatar.com/avatar/9b13a8a078e1dcf8588c4eea9554445d51ebed6c41b51f56f4d96738130b05c6?d=mp&s=160"},"body":"p4merge is now a built-in diff/merge tool.\nThis adds p4merge to git-completion and updates\nthe documentation to mention p4merge.\n---\n Documentation/git-difftool.txt | 2 +-\n Documentation/git-mergetool.txt | 2 +-\n Documentation/merge-config.txt | 2 +-\n contrib/completion/git-completion.bash | 2 +-\n git-mergetool--lib.sh | 17 +++++++++++++++--\n 5 files changed, 19 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex 96a6c51..8e9aed6 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -31,7 +31,7 @@ OPTIONS\n \tUse the diff tool specified by .\n \tValid merge tools are:\n \tkdiff3, kompare, tkdiff, meld, xxdiff, emerge, vimdiff, gvimdiff,\n-\tecmerge, diffuse, opendiff and araxis.\n+\tecmerge, diffuse, opendiff, p4merge and araxis.\n +\n If a diff tool is not specified, 'git-difftool'\n will use the configuration variable `diff.tool`. If the\ndiff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt\nindex 68ed6c0..4a6f7f3 100644\n--- a/Documentation/git-mergetool.txt\n+++ b/Documentation/git-mergetool.txt\n@@ -27,7 +27,7 @@ OPTIONS\n \tUse the merge resolution program specified by .\n \tValid merge tools are:\n \tkdiff3, tkdiff, meld, xxdiff, emerge, vimdiff, gvimdiff, ecmerge,\n-\tdiffuse, tortoisemerge, opendiff and araxis.\n+\tdiffuse, tortoisemerge, opendiff, p4merge and araxis.\n +\n If a merge resolution program is not specified, 'git-mergetool'\n will use the configuration variable `merge.tool`. If the\ndiff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\nindex c0f96e7..a403155 100644\n--- a/Documentation/merge-config.txt\n+++ b/Documentation/merge-config.txt\n@@ -23,7 +23,7 @@ merge.tool::\n \tControls which merge resolution program is used by\n \tlinkgit:git-mergetool[1]. Valid built-in values are: \"kdiff3\",\n \t\"tkdiff\", \"meld\", \"xxdiff\", \"emerge\", \"vimdiff\", \"gvimdiff\",\n-\t\"diffuse\", \"ecmerge\", \"tortoisemerge\", \"araxis\", and\n+\t\"diffuse\", \"ecmerge\", \"tortoisemerge\", \"p4merge\", \"araxis\" and\n \t\"opendiff\". Any other value is treated is custom merge tool\n \tand there must be a corresponding mergetool..cmd option.\n\ndiff --git a/contrib/completion/git-completion.bash\nb/contrib/completion/git-completion.bash\nindex d3fec32..5fb6017 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -953,7 +953,7 @@ _git_diff ()\n }\n\n __git_mergetools_common=\"diffuse ecmerge emerge kdiff3 meld opendiff\n-\t\t\ttkdiff vimdiff gvimdiff xxdiff araxis\n+\t\t\ttkdiff vimdiff gvimdiff xxdiff araxis p4merge\n \"\n\n _git_difftool ()\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex bfb01f7..f7c571e 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -46,7 +46,7 @@ check_unchanged () {\n valid_tool () {\n \tcase \"$1\" in\n \tkdiff3 | tkdiff | xxdiff | meld | opendiff | \\\n-\temerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis)\n+\temerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis | p4merge)\n \t\t;; # happy\n \ttortoisemerge)\n \t\tif ! merge_mode; then\n@@ -130,6 +130,19 @@ run_merge_tool () {\n \t\t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\"\n \t\tfi\n \t\t;;\n+\tp4merge)\n+\t\tif merge_mode; then\n+\t\t touch \"$BACKUP\"\n+\t\t\tif $base_present; then\n+\t\t\t\t\"$merge_tool_path\" \"$BASE\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n+\t\t\telse\n+\t\t\t\t\"$merge_tool_path\" \"$LOCAL\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n+\t\t\tfi\n+\t\t\tcheck_unchanged\n+\t\telse\n+\t\t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\"\n+\t\tfi\n+\t\t;;\n \tmeld)\n \t\tif merge_mode; then\n \t\t\ttouch \"$BACKUP\"\n@@ -323,7 +336,7 @@ guess_merge_tool () {\n \t\telse\n \t\t\ttools=\"opendiff kdiff3 tkdiff xxdiff meld $tools\"\n \t\tfi\n-\t\ttools=\"$tools gvimdiff diffuse ecmerge araxis\"\n+\t\ttools=\"$tools gvimdiff diffuse ecmerge p4merge araxis\"\n \tfi\n \tif echo \"${VISUAL:-$EDITOR}\" | grep emacs > /dev/null 2>&1; then\n \t\t# $EDITOR is emacs so add emerge as a candidate\n-- \n1.6.5.2.75.g16dea.dirty\n"},{"id":"126066","messageId":"20091027230043.GA11607@hashpling.org","threadId":"21390","inReplyTo":"d411cc4a0910271536u5817802at43f7477dd8ccabc7@mail.gmail.com","subject":"Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2009-10-27T23:00:43Z","receivedAt":"2009-10-27T23:00:43Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Tue, Oct 27, 2009 at 03:36:49PM -0700, Scott Chacon wrote:\n> p4merge is now a built-in diff/merge tool.\n> This adds p4merge to git-completion and updates\n> the documentation to mention p4merge.\n> ---\n\nI approve (but haven't had a chance to test this). p4merge is a\ngood mergetool, but now I'll have to find something else as an example\nthat you need to use custom mergetool support for.\n\nI'm just wondering, does this work well with unixes and Mac OS X? I\nthink it's recommended install practice to symlink p4v as p4merge on\n*nix, but Mac OS X needs some sort of 'launchp4merge' to be called\nIIRC, or is this something that users can just configure with\nmergetool.p4diff.path?\n\n-- \nCharles Bailey\nhttp://ccgi.hashpling.plus.com/blog/\n"},{"id":"126095","messageId":"7vy6mwt2af.fsf@alter.siamese.dyndns.org","threadId":"21390","inReplyTo":"20091027230043.GA11607@hashpling.org","subject":"Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-28T07:18:00Z","receivedAt":"2009-10-28T07:18:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey writes:\n\n> On Tue, Oct 27, 2009 at 03:36:49PM -0700, Scott Chacon wrote:\n>> p4merge is now a built-in diff/merge tool.\n>> This adds p4merge to git-completion and updates\n>> the documentation to mention p4merge.\n>> ---\n>\n> I approve (but haven't had a chance to test this).\n\nThanks; eventually you two need Sign-off and Acked-by, then, but I sense\nthat an undate to address the points below is in order?\n\n> I'm just wondering, does this work well with unixes and Mac OS X? I\n> think it's recommended install practice to symlink p4v as p4merge on\n> *nix, but Mac OS X needs some sort of 'launchp4merge' to be called\n> IIRC, or is this something that users can just configure with\n> mergetool.p4diff.path?\n"},{"id":"126115","messageId":"20091028090022.GA90780@gmail.com","threadId":"21390","inReplyTo":"20091027230043.GA11607@hashpling.org","subject":"Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-10-28T09:00:24Z","receivedAt":"2009-10-28T09:00:24Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Oct 27, 2009 at 11:00:43PM +0000, Charles Bailey wrote:\n> On Tue, Oct 27, 2009 at 03:36:49PM -0700, Scott Chacon wrote:\n> > p4merge is now a built-in diff/merge tool.\n> > This adds p4merge to git-completion and updates\n> > the documentation to mention p4merge.\n> > ---\n> \n> I approve (but haven't had a chance to test this). p4merge is a\n> good mergetool, but now I'll have to find something else as an example\n> that you need to use custom mergetool support for.\n\nDitto, looks good to me.\n\n\n> I'm just wondering, does this work well with unixes and Mac OS X? I\n> think it's recommended install practice to symlink p4v as p4merge on\n> *nix, but Mac OS X needs some sort of 'launchp4merge' to be called\n> IIRC, or is this something that users can just configure with\n> mergetool.p4diff.path?\n\nI just tested this on Mac OS X with the latest version of\np4merge. It worked great.\n\n\t$ git config difftool.p4merge.path \\\n\t /Applications/p4merge.app/Contents/MacOS/p4merge\n\n\t$ git difftool -t p4merge HEAD^\n\n\nSo...\n\nTested-by: David Aguilar \n\n\nP.S. thanks for the patch, Scott.\n\nSorry I haven't gotten around to forking progit yet\nbut we did at least get Disney Animation to go with\ngit + github =)\n\nhttp://github.com/wdas/ptex\n\n(there's only some headers up there right now,\n but we'll have more to share soon)\n\n\nHave fun,\n\n-- \n\t\tDavid\n"},{"id":"126153","messageId":"d411cc4a0910280837h52596089je9ab4d03383d43cc@mail.gmail.com","threadId":"21390","inReplyTo":"20091028090022.GA90780@gmail.com","subject":"Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","fromName":"Scott Chacon","fromEmail":"schacon@gmail.com","sentAt":"2009-10-28T15:37:06Z","receivedAt":"2009-10-28T15:37:06Z","isPatch":true,"sender":{"key":"schacon@gmail.com","avatar":"https://gravatar.com/avatar/9b13a8a078e1dcf8588c4eea9554445d51ebed6c41b51f56f4d96738130b05c6?d=mp&s=160"},"body":"Hey,\n\nOn Wed, Oct 28, 2009 at 2:00 AM, David Aguilar wrote:\n>> I'm just wondering, does this work well with unixes and Mac OS X? I\n>> think it's recommended install practice to symlink p4v as p4merge on\n>> *nix, but Mac OS X needs some sort of 'launchp4merge' to be called\n>> IIRC, or is this something that users can just configure with\n>> mergetool.p4diff.path?\n>\n> I just tested this on Mac OS X with the latest version of\n> p4merge.  It worked great.\n>\n>        $ git config difftool.p4merge.path \\\n>          /Applications/p4merge.app/Contents/MacOS/p4merge\n>\n>        $ git difftool -t p4merge HEAD^\n>\n\nThis is how I have it setup as well and both diff and merge work for\nme. I had to do a weird thing with passing it $LOCAL twice if there\nwas no merge base since otherwise it does a diff tool instead of a\nmerge tool - the difference is based on the number of arguments, but\nit seems to work pretty well. I can try it on Linux a bit later, but\nI'm not sure why launchp4merge would be needed instead of setting the\npath like this on a Mac - if there is no serious objection, I can\nresend this with my Signed-Off-By (sorry, I forgot).\n\nThanks,\nScott\n"},{"id":"126178","messageId":"d411cc4a0910281439v3388c243v42b3700f73744623@mail.gmail.com","threadId":"21390","inReplyTo":"d411cc4a0910280837h52596089je9ab4d03383d43cc@mail.gmail.com","subject":"[PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","fromName":"Scott Chacon","fromEmail":"schacon@gmail.com","sentAt":"2009-10-28T21:39:32Z","receivedAt":"2009-10-28T21:39:32Z","isPatch":true,"sender":{"key":"schacon@gmail.com","avatar":"https://gravatar.com/avatar/9b13a8a078e1dcf8588c4eea9554445d51ebed6c41b51f56f4d96738130b05c6?d=mp&s=160"},"body":"p4merge is now a built-in diff/merge tool.\nThis adds p4merge to git-completion and updates\nthe documentation to mention p4merge.\n\nSigned-Off-By: Scott Chacon \n---\n\nThis is the same patch, but I tested it on Linux as well as Mac and it\nworks fine as long as the [difftool|mergetool].p4merge.path configs\nare set or it's in your path.\n\n Documentation/git-difftool.txt | 2 +-\n Documentation/git-mergetool.txt | 2 +-\n Documentation/merge-config.txt | 2 +-\n contrib/completion/git-completion.bash | 2 +-\n git-mergetool--lib.sh | 17 +++++++++++++++--\n 5 files changed, 19 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex 96a6c51..8e9aed6 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -31,7 +31,7 @@ OPTIONS\n \tUse the diff tool specified by .\n \tValid merge tools are:\n \tkdiff3, kompare, tkdiff, meld, xxdiff, emerge, vimdiff, gvimdiff,\n-\tecmerge, diffuse, opendiff and araxis.\n+\tecmerge, diffuse, opendiff, p4merge and araxis.\n +\n If a diff tool is not specified, 'git-difftool'\n will use the configuration variable `diff.tool`. If the\ndiff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt\nindex 68ed6c0..4a6f7f3 100644\n--- a/Documentation/git-mergetool.txt\n+++ b/Documentation/git-mergetool.txt\n@@ -27,7 +27,7 @@ OPTIONS\n \tUse the merge resolution program specified by .\n \tValid merge tools are:\n \tkdiff3, tkdiff, meld, xxdiff, emerge, vimdiff, gvimdiff, ecmerge,\n-\tdiffuse, tortoisemerge, opendiff and araxis.\n+\tdiffuse, tortoisemerge, opendiff, p4merge and araxis.\n +\n If a merge resolution program is not specified, 'git-mergetool'\n will use the configuration variable `merge.tool`. If the\ndiff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\nindex c0f96e7..a403155 100644\n--- a/Documentation/merge-config.txt\n+++ b/Documentation/merge-config.txt\n@@ -23,7 +23,7 @@ merge.tool::\n \tControls which merge resolution program is used by\n \tlinkgit:git-mergetool[1]. Valid built-in values are: \"kdiff3\",\n \t\"tkdiff\", \"meld\", \"xxdiff\", \"emerge\", \"vimdiff\", \"gvimdiff\",\n-\t\"diffuse\", \"ecmerge\", \"tortoisemerge\", \"araxis\", and\n+\t\"diffuse\", \"ecmerge\", \"tortoisemerge\", \"p4merge\", \"araxis\" and\n \t\"opendiff\". Any other value is treated is custom merge tool\n \tand there must be a corresponding mergetool..cmd option.\n\ndiff --git a/contrib/completion/git-completion.bash\nb/contrib/completion/git-completion.bash\nindex d3fec32..5fb6017 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -953,7 +953,7 @@ _git_diff ()\n }\n\n __git_mergetools_common=\"diffuse ecmerge emerge kdiff3 meld opendiff\n-\t\t\ttkdiff vimdiff gvimdiff xxdiff araxis\n+\t\t\ttkdiff vimdiff gvimdiff xxdiff araxis p4merge\n \"\n\n _git_difftool ()\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex bfb01f7..f7c571e 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -46,7 +46,7 @@ check_unchanged () {\n valid_tool () {\n \tcase \"$1\" in\n \tkdiff3 | tkdiff | xxdiff | meld | opendiff | \\\n-\temerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis)\n+\temerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis | p4merge)\n \t\t;; # happy\n \ttortoisemerge)\n \t\tif ! merge_mode; then\n@@ -130,6 +130,19 @@ run_merge_tool () {\n \t\t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\"\n \t\tfi\n \t\t;;\n+\tp4merge)\n+\t\tif merge_mode; then\n+\t\t touch \"$BACKUP\"\n+\t\t\tif $base_present; then\n+\t\t\t\t\"$merge_tool_path\" \"$BASE\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n+\t\t\telse\n+\t\t\t\t\"$merge_tool_path\" \"$LOCAL\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n+\t\t\tfi\n+\t\t\tcheck_unchanged\n+\t\telse\n+\t\t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\"\n+\t\tfi\n+\t\t;;\n \tmeld)\n \t\tif merge_mode; then\n \t\t\ttouch \"$BACKUP\"\n@@ -323,7 +336,7 @@ guess_merge_tool () {\n \t\telse\n \t\t\ttools=\"opendiff kdiff3 tkdiff xxdiff meld $tools\"\n \t\tfi\n-\t\ttools=\"$tools gvimdiff diffuse ecmerge araxis\"\n+\t\ttools=\"$tools gvimdiff diffuse ecmerge p4merge araxis\"\n \tfi\n \tif echo \"${VISUAL:-$EDITOR}\" | grep emacs > /dev/null 2>&1; then\n \t\t# $EDITOR is emacs so add emerge as a candidate\n-- \n1.6.5.2.75.gad2f8\n"},{"id":"126191","messageId":"7v1vkngkdm.fsf@alter.siamese.dyndns.org","threadId":"21390","inReplyTo":"d411cc4a0910281439v3388c243v42b3700f73744623@mail.gmail.com","subject":"Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-28T23:37:57Z","receivedAt":"2009-10-28T23:37:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks. Is Jay happy with this version?\n"},{"id":"126234","messageId":"76718490910282317g56d97652k3b9fa1dbbe4abdbd@mail.gmail.com","threadId":"21390","inReplyTo":"7v1vkngkdm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-10-29T06:17:43Z","receivedAt":"2009-10-29T06:17:43Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Wed, Oct 28, 2009 at 4:37 PM, Junio C Hamano wrote:\n> Thanks.  Is Jay happy with this version?\n\nIt's good enough, but I'm still going to send a follow up patch that\nlooks in /Applications and $HOME/Applications since I don't think the\nuser should have to set the full path (they don't on other platforms).\n\nSo:\n\nAcked-by: Jay Soffian\n\nj.\n"},{"id":"126341","messageId":"20091029221234.GB32590@hashpling.org","threadId":"21390","inReplyTo":"d411cc4a0910281439v3388c243v42b3700f73744623@mail.gmail.com","subject":"Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2009-10-29T22:12:34Z","receivedAt":"2009-10-29T22:12:34Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Wed, Oct 28, 2009 at 02:39:32PM -0700, Scott Chacon wrote:\n> p4merge is now a built-in diff/merge tool.\n> This adds p4merge to git-completion and updates\n> the documentation to mention p4merge.\n> \n> Signed-Off-By: Scott Chacon \n> ---\n> \n> This is the same patch, but I tested it on Linux as well as Mac and it\n> works fine as long as the [difftool|mergetool].p4merge.path configs\n> are set or it's in your path.\n\nI've examined the two patches and I feel more comfortable with\nScott's, mainly due to it's simplicity.\n\nI'm not sure I understand why only p4merge on Mac OS X is special, we\ndon't seem to treat any other mergetool specially and we don't seem to\nneed absolute paths anywhere else.\n\nIf it's a Mac OS X only thing, can we (and should we) avoid special\ntreatment for p4merge on other platforms?\n\nThe only other question I have is what are the merits of using\n/dev/null as the base vs. a second copy of the local version in the\nbaseless merge case? It's the only other difference between the two\np4merge patches that I noticed.\n\nPerhaps we could consider having both p4merge and launchp4merge as\nseparate options?\n\n-- \nCharles Bailey\nhttp://ccgi.hashpling.plus.com/blog/\n"},{"id":"126346","messageId":"76718490910291747l165baf49tab781727d010610a@mail.gmail.com","threadId":"21390","inReplyTo":"20091029221234.GB32590@hashpling.org","subject":"Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-10-30T00:47:08Z","receivedAt":"2009-10-30T00:47:08Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Thu, Oct 29, 2009 at 6:12 PM, Charles Bailey wrote:\n> I'm not sure I understand why only p4merge on Mac OS X is special, we\n> don't seem to treat any other mergetool specially and we don't seem to\n> need absolute paths anywhere else.\n\nOn other platforms, the merge tool is very likely to be in your PATH.\n\nOn OS X, p4merge is going to be installed as part of an application\nbundle (/Applications/p4merge.app or $HOME/Applications/p4merge.app).\nThis is virtually never going to be in a user's PATH.\n\nSo in order to provide equivalent behavior for OS X as Linux (i.e., so\nthat you can just specify p4merge as the mergetool without having to\nprovide it's path), we need to look in these additional locations.\n\n> If it's a Mac OS X only thing, can we (and should we) avoid special\n> treatment for p4merge on other platforms?\n\nIt is a Mac OS X only thing. Yes, we could avoid looking in these\nlocations on other platforms, but why? Using type to look for the\nexecutable is virtually no cost. The alternative (calling uname to\ndetermine the platform) requires running a separate process.\n\n> The only other question I have is what are the merits of using\n> /dev/null as the base vs. a second copy of the local version in the\n> baseless merge case? It's the only other difference between the two\n> p4merge patches that I noticed.\n\np4merge's argument handling is stupid, you need to pass it a dummy\nargument in some cases. A second copy of the local version is probably\nbetter than /dev/null. Actually, I think just passing it an empty\nargument (\"\") works too.\n\n> Perhaps we could consider having both p4merge and launchp4merge as\n> separate options?\n\nI think that would be over-engineered. Decide on one or the other.\nThey have slightly different semantics as I've previously described.\nPersonally I think calling launchp4merge provides a more Mac-like\nbehavior, but honestly it doesn't make much difference.\n\nj.\n"},{"id":"126347","messageId":"200910300202.02016.markus.heidelberg@web.de","threadId":"21390","inReplyTo":"76718490910291747l165baf49tab781727d010610a@mail.gmail.com","subject":"Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-10-30T01:02:01Z","receivedAt":"2009-10-30T01:02:01Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"Jay Soffian, 30.10.2009:\n> On Thu, Oct 29, 2009 at 6:12 PM, Charles Bailey wrote:\n> > I'm not sure I understand why only p4merge on Mac OS X is special, we\n> > don't seem to treat any other mergetool specially and we don't seem to\n> > need absolute paths anywhere else.\n> \n> On other platforms, the merge tool is very likely to be in your PATH.\n\nHe didn't mean p4merge on other platforms, but other merge tools on Mac\nOS X. What about all the other merge tools already in mergetool--lib?\nShould they get special handling, too?\n\n> On OS X, p4merge is going to be installed as part of an application\n> bundle (/Applications/p4merge.app or $HOME/Applications/p4merge.app).\n> This is virtually never going to be in a user's PATH.\n> \n> So in order to provide equivalent behavior for OS X as Linux (i.e., so\n> that you can just specify p4merge as the mergetool without having to\n> provide it's path), we need to look in these additional locations.\n\nAnd for Windows we could add C:\\Program Files\\MergeToolX\\tool.exe for\nevery merge tool.\n\nBut where will we end?\n\nMarkus\n"},{"id":"126350","messageId":"76718490910292000t7b024b83y68d71b6ff810c15@mail.gmail.com","threadId":"21390","inReplyTo":"200910300202.02016.markus.heidelberg@web.de","subject":"Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-10-30T03:00:52Z","receivedAt":"2009-10-30T03:00:52Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Thu, Oct 29, 2009 at 9:02 PM, Markus Heidelberg\n wrote:\n> He didn't mean p4merge on other platforms, but other merge tools on Mac\n> OS X. What about all the other merge tools already in mergetool--lib?\n> Should they get special handling, too?\n\nIf someone wants to scratch that itch, then yes. The default diff tool\nfor OS X has its helper already in /usr/bin (opendiff). p4merge is\narguably a better merge tool, and it installs as an app bundle in\n/Applications. I'm not sure about the other diff tools, I haven't\nlooked.\n\n> And for Windows we could add C:\\Program Files\\MergeToolX\\tool.exe for\n> every merge tool.\n\nIf it makes those tools easier to use with git, and if someone on\nWindows wants to scratch that itch, then yes, we should.\n\n> But where will we end?\n\nI don't understand this argument. It's a few lines of code to make git\na little friendlier. We end when folks stop contributing patches\nbecause either no one cares of there's nothing left to improve.\n\nj.\n"},{"id":"126373","messageId":"200910301135.59831.markus.heidelberg@web.de","threadId":"21390","inReplyTo":"76718490910292000t7b024b83y68d71b6ff810c15@mail.gmail.com","subject":"Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-10-30T10:35:59Z","receivedAt":"2009-10-30T10:35:59Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"Jay Soffian, 30.10.2009:\n> On Thu, Oct 29, 2009 at 9:02 PM, Markus Heidelberg\n> wrote:\n> > He didn't mean p4merge on other platforms, but other merge tools on Mac\n> > OS X. What about all the other merge tools already in mergetool--lib?\n> > Should they get special handling, too?\n> \n> If someone wants to scratch that itch, then yes. The default diff tool\n> for OS X has its helper already in /usr/bin (opendiff). p4merge is\n> arguably a better merge tool, and it installs as an app bundle in\n> /Applications. I'm not sure about the other diff tools, I haven't\n> looked.\n> \n> > And for Windows we could add C:\\Program Files\\MergeToolX\\tool.exe for\n> > every merge tool.\n> \n> If it makes those tools easier to use with git, and if someone on\n> Windows wants to scratch that itch, then yes, we should.\n\nAnother possible problem: the user can change the installation\ndestination on Windows. What's the behaviour of Mac OS here? Is the\ninstalation path fixed or changeable?\n\nMarkus\n"},{"id":"126375","messageId":"3f4fd2640910300425q602471a6v1111a7dceee7746c@mail.gmail.com","threadId":"21390","inReplyTo":"200910301135.59831.markus.heidelberg@web.de","subject":"Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","fromName":"Reece Dunn","fromEmail":"msclrhd@googlemail.com","sentAt":"2009-10-30T11:25:25Z","receivedAt":"2009-10-30T11:25:25Z","isPatch":true,"sender":{"key":"msclrhd@googlemail.com","avatar":null},"body":"2009/10/30 Markus Heidelberg :\n> Jay Soffian, 30.10.2009:\n>> On Thu, Oct 29, 2009 at 9:02 PM, Markus Heidelberg\n>> wrote:\n>> > He didn't mean p4merge on other platforms, but other merge tools on Mac\n>> > OS X. What about all the other merge tools already in mergetool--lib?\n>> > Should they get special handling, too?\n>>\n>> If someone wants to scratch that itch, then yes. The default diff tool\n>> for OS X has its helper already in /usr/bin (opendiff). p4merge is\n>> arguably a better merge tool, and it installs as an app bundle in\n>> /Applications. I'm not sure about the other diff tools, I haven't\n>> looked.\n>>\n>> > And for Windows we could add C:\\Program Files\\MergeToolX\\tool.exe for\n>> > every merge tool.\n>>\n>> If it makes those tools easier to use with git, and if someone on\n>> Windows wants to scratch that itch, then yes, we should.\n>\n> Another possible problem: the user can change the installation\n> destination on Windows. What's the behaviour of Mac OS here? Is the\n> instalation path fixed or changeable?\n\nFor Windows, the program should have an InstallDir or similar registry\nvalue in a fixed place in the registry to point to where it is\ninstalled (something like\nHKLM/Software/[Vendor]/[Application]/[Version]).\n\nAs for Linux, there is no guarantee that things like p4merge are in\nthe path either. It could be placed under /opt/perforce or\n/home/perforce.\n\nWhat would be sensible (for all platforms) is:\n 1/ if [difftool|mergetool].toolname.path is set, use that (is this\ndocumented?)\n 2/ try looking for the tool in the system path\n 3/ try some intelligent guessing\n 4/ if none of these work, print out an error message -- ideally,\nthis should mention the configuration option in (1)\n\n(3) is what is being discussed. It is good that it will work without\nany user configuration (especially for standard tools installed in\nstandard places), but isn't really a big problem as long as the user\nis prompted to configure the tool path. Also, I'm not sure how this\nwill work with multiple versions of the tools installed (e.g. vim/gvim\nand p4merge).\n\n- Reece\n"},{"id":"126389","messageId":"76718490910300817w776bde48j40de31e5532b9fd4@mail.gmail.com","threadId":"21390","inReplyTo":"3f4fd2640910300425q602471a6v1111a7dceee7746c@mail.gmail.com","subject":"Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-10-30T15:17:09Z","receivedAt":"2009-10-30T15:17:09Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Fri, Oct 30, 2009 at 7:25 AM, Reece Dunn wrote:\n> 2009/10/30 Markus Heidelberg :\n>> Another possible problem: the user can change the installation\n>> destination on Windows. What's the behaviour of Mac OS here? Is the\n>> instalation path fixed or changeable?\n\nThis has already been answered. Yes the application can move on OS X,\nbut 9/10 it will be in one of two standard locations. There are ways\nto find an application regardless of where it is, but it's maybe not\nworth the platform specific complexity for that 1/10 time.\n\n> For Windows, the program should have an InstallDir or similar registry\n> value in a fixed place in the registry to point to where it is\n> installed (something like\n> HKLM/Software/[Vendor]/[Application]/[Version]).\n\nAnd if someone wants to contribute the code to grub around the\nregistry on Windows, I'm all for it, as long as it doesn't negatively\nimpact non-Windows users (and similarly for any other platform\nspecific code -- don't impact users of other platforms negatively).\n\n> As for Linux, there is no guarantee that things like p4merge are in\n> the path either. It could be placed under /opt/perforce or\n> /home/perforce.\n\nNo, of course not, but again, looking in PATH is likely to work in the\ncommon case. By looking in /Application and $HOME/Applications, that\ncovers the common case on OS X.\n\n> What would be sensible (for all platforms) is:\n>  1/  if [difftool|mergetool].toolname.path is set, use that (is this\n> documented?)\n>  2/  try looking for the tool in the system path\n>  3/  try some intelligent guessing\n>  4/  if none of these work, print out an error message -- ideally,\n> this should mention the configuration option in (1)\n\nThis is basically what is already done, but (3) isn't yet platform\nspecific in any way, and (4) doesn't mention the config option.\n\n> (3) is what is being discussed. It is good that it will work without\n> any user configuration (especially for standard tools installed in\n> standard places), but isn't really a big problem as long as the user\n> is prompted to configure the tool path. Also, I'm not sure how this\n> will work with multiple versions of the tools installed (e.g. vim/gvim\n> and p4merge).\n\nThere's a fixed order of tools, first tool that's found wins.\n\nOh, and my favorite color paint is blue. :-)\n\nj.\n"},{"id":"126390","messageId":"200910301630.30420.markus.heidelberg@web.de","threadId":"21390","inReplyTo":"76718490910300817w776bde48j40de31e5532b9fd4@mail.gmail.com","subject":"Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-10-30T15:30:29Z","receivedAt":"2009-10-30T15:30:29Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"Jay Soffian, 30.10.2009:\n> On Fri, Oct 30, 2009 at 7:25 AM, Reece Dunn wrote:\n> >  3/  try some intelligent guessing\n> \n> This is basically what is already done, but (3) isn't yet platform\n> specific in any way\n\nMaybe this can be considered to be implemented. But since it's not\np4merge specific, the p4merge patch should firstly be applied without\nthe intelligence.\nThe intelligence may be implemented mergetool agnostic for all at once,\nif that is possible?\n\nMarkus\n"},{"id":"126399","messageId":"20091030174421.GA21486@hashpling.org","threadId":"21390","inReplyTo":"d411cc4a0910281439v3388c243v42b3700f73744623@mail.gmail.com","subject":"Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2009-10-30T17:44:21Z","receivedAt":"2009-10-30T17:44:21Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Wed, Oct 28, 2009 at 02:39:32PM -0700, Scott Chacon wrote:\n> p4merge is now a built-in diff/merge tool.\n> This adds p4merge to git-completion and updates\n> the documentation to mention p4merge.\n> \n> Signed-Off-By: Scott Chacon \n> ---\n\nAcked-by: Charles Bailey \n\nI'm aware that we haven't reached full agreement on the best way to\nmake p4merge + git as Mac OS X friendly as possible, but Jay said that\nthis patch is 'good enough' and I agree. If we go with this for now,\nwe're not closing the door to further improvements.\n\nI confirmed a (perhaps very minor) issue with the abspath approach,\nthe parameters are used as the title of the pains in p4merge, so\n(IMHO) short paths look neater, especially if you've cd'ed down to a\nlow level and don't have a 1920 pixel wide monitor. p4merge does\ntruncate from the left so it's not a big thing.\n\nThe other thing which I confirmed is that p4merge on windows doesn't\nlike /dev/null as an explicit parameter (it gets convered to nul: by\nmsys, I believe, but it still doesn't like it).\n\np4merge does appear to do 'magic' when the base and left are the same\nparameter (not just the same file - the magic doesn't work if you,\nsay, use a relative path and an absolute path to refer to the same\nfile. This means that it doesn't just take the right changes which is\nwhat I feared it might do when I first saw the 'local' 'local'\n'remote' pattern.\n\nCharles.\n\n\n-- \nCharles Bailey\nhttp://ccgi.hashpling.plus.com/blog/\n"},{"id":"126407","messageId":"7vmy38lnk8.fsf@alter.siamese.dyndns.org","threadId":"21390","inReplyTo":"20091030174421.GA21486@hashpling.org","subject":"Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-30T18:54:47Z","receivedAt":"2009-10-30T18:54:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey writes:\n\n> On Wed, Oct 28, 2009 at 02:39:32PM -0700, Scott Chacon wrote:\n>> p4merge is now a built-in diff/merge tool.\n>> This adds p4merge to git-completion and updates\n>> the documentation to mention p4merge.\n>> \n>> Signed-Off-By: Scott Chacon \n>> ---\n>\n> Acked-by: Charles Bailey \n>\n> I'm aware that we haven't reached full agreement on the best way to\n> make p4merge + git as Mac OS X friendly as possible, but Jay said that\n> this patch is 'good enough' and I agree. If we go with this for now,\n> we're not closing the door to further improvements.\n\nWill queue; the peculiarity of MacOS X may be annoying, but the annoyance\nis not limited to the topic of adding p4merge to this codepath.\n"}]}