{"thread":{"id":"21399","subject":"[PATCH] mergetool--lib: add support for p4merge","startedAt":"2009-10-28T09:11:27Z","lastAt":"2009-10-28T10:02:45Z","messageCount":6,"participants":["Jay Soffian","David Aguilar"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"126116","messageId":"1256721087-72534-1-git-send-email-jaysoffian@gmail.com","threadId":"21399","inReplyTo":null,"subject":"[PATCH] mergetool--lib: add support for p4merge","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-10-28T09:11:27Z","receivedAt":"2009-10-28T09:11:27Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"Add native support for p4merge as a diff / merge tool. There are two\noddities. First, launching p4merge on Mac OS X requires running a helper\nshim (which in turn is looked for in a couple common locations, as it's\nvery unlikely to be in PATH). Second, p4merge seems to require its file\narguments be absolute paths.\n\nSigned-off-by: Jay Soffian <jaysoffian@gmail.com>\n---\nAs requested by an Android developer at GitTogether09.\n\nOnly tested on Mac OS X 10.6.1. Feedback from anyone running p4merge on other\nOS's would be appreciated.\n\n Documentation/git-difftool.txt  |    2 +-\n Documentation/git-mergetool.txt |    2 +-\n git-mergetool--lib.sh           |   44 ++++++++++++++++++++++++++++++++++++++-\n 3 files changed, 45 insertions(+), 3 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 <tool>.\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 <tool>.\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/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex bfb01f7..23b2816 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -7,6 +7,12 @@ merge_mode() {\n \ttest \"$TOOL_MODE\" = merge\n }\n \n+abspath() {\n+\td=$(dirname \"$1\")\n+\td=$(cd \"$d\" && pwd -P)\n+\techo \"$d/$(basename \"$1\")\"\n+}\n+\n translate_merge_tool_path () {\n \tcase \"$1\" in\n \tvimdiff)\n@@ -21,6 +27,19 @@ translate_merge_tool_path () {\n \taraxis)\n \t\techo compare\n \t\t;;\n+\tp4merge)\n+\t\t# Look for the Mac OS X P4Merge shim\n+\t\tfor app_dir in \"/Applications\" \"$HOME/Applications\"\n+\t\tdo\n+\t\t\t\tlaunchp4merge=\"$app_dir/p4merge.app/Contents/Resources/launchp4merge\"\n+\t\t\t\tif type \"$launchp4merge\" > /dev/null 2>&1; then\n+\t\t\t\t\techo \"$launchp4merge\"\n+\t\t\t\t\treturn 0\n+\t\t\t\tfi\n+\t\tdone\n+\t\t# Otherwise assume we're not on Mac OS X and just return p4merge\n+\t\techo \"$1\"\n+\t\t;;\n \t*)\n \t\techo \"$1\"\n \t\t;;\n@@ -45,7 +64,7 @@ check_unchanged () {\n \n valid_tool () {\n \tcase \"$1\" in\n-\tkdiff3 | tkdiff | xxdiff | meld | opendiff | \\\n+\tkdiff3 | tkdiff | xxdiff | meld | opendiff | p4merge | \\\n \temerge | vimdiff | gvimdiff | ecmerge | diffuse | araxis)\n \t\t;; # happy\n \ttortoisemerge)\n@@ -284,6 +303,29 @@ run_merge_tool () {\n \t\t\t\t>/dev/null 2>&1\n \t\tfi\n \t\t;;\n+\tp4merge)\n+\t\t# At least on Mac OS X p4merge wants absolute paths\n+\t\tABS_LOCAL=$(abspath \"$LOCAL\")\n+\t\tABS_REMOTE=$(abspath \"$REMOTE\")\n+\t\tif merge_mode; then\n+\t\t\tABS_MERGED=$(abspath \"$MERGED\")\n+\t\t\ttouch \"$BACKUP\"\n+\t\t\tif $base_present; then\n+\t\t\t\tABS_BASE=$(abspath \"$BASE\")\n+\t\t\t\t\"$merge_tool_path\" \\\n+\t\t\t\t\t\"$ABS_BASE\" \"$ABS_LOCAL\" \"$ABS_REMOTE\" \"$ABS_MERGED\" \\\n+\t\t\t\t\t>/dev/null 2>&1\n+\t\t\telse\n+\t\t\t\t\"$merge_tool_path\" \\\n+\t\t\t\t\t\"/dev/null\" \"$ABS_LOCAL\" \"$ABS_REMOTE\" \"$ABS_MERGED\" \\\n+\t\t\t\t\t>/dev/null 2>&1\n+\t\t\tfi\n+\t\t\tcheck_unchanged\n+\t\telse\n+\t\t\t\"$merge_tool_path\" \"$ABS_LOCAL\" \"$ABS_REMOTE\" \\\n+\t\t\t\t>/dev/null 2>&1\n+\t\tfi\n+\t\t;;\n \t*)\n \t\tmerge_tool_cmd=\"$(get_merge_tool_cmd \"$1\")\"\n \t\tif test -z \"$merge_tool_cmd\"; then\n-- \n1.6.5.2.74.g610f9.dirty\n"},{"id":"126117","messageId":"76718490910280221u4e1d3e78me7f9b0b45f590e56@mail.gmail.com","threadId":"21399","inReplyTo":"1256721087-72534-1-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH] mergetool--lib: add support for p4merge","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-10-28T09:21:46Z","receivedAt":"2009-10-28T09:21:46Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Wed, Oct 28, 2009 at 2:11 AM, Jay Soffian <jaysoffian@gmail.com> wrote:\n> Add native support for p4merge as a diff / merge tool. There are two\n> oddities. First, launching p4merge on Mac OS X requires running a helper\n> shim (which in turn is looked for in a couple common locations, as it's\n> very unlikely to be in PATH). Second, p4merge seems to require its file\n> arguments be absolute paths.\n>\n> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>\n> ---\n\nAmusing. I didn't see Scott's patch.\n\nBut, in my testing, for things to work properly I needed to use\nlaunchp4merge per:\n\nhttp://kb.perforce.com/AllPerforceApplications/StandAloneClients/P4merge/CommandLineP..rgeOnMacOsX\n\nAnd I also found things didn't work properly unless I provided an absolute path.\n\n(Aside, the \"right\" way to launch p4merge, at least on 10.6 would be:\n\n/usr/bin/open -b com.perforce.p4merge -W -n --args <args to p4merge...>\n\nThis way OS X's launch services would find p4merge.app wherever it is\non the user's system. But, I think some of these options to open are\n10.6 specific and in practice looking in /Applications and\n$HOME/Applications I think is a sane enough default.)\n\nj.\n"},{"id":"126118","messageId":"20091028092718.GB90780@gmail.com","threadId":"21399","inReplyTo":"76718490910280221u4e1d3e78me7f9b0b45f590e56@mail.gmail.com","subject":"Re: [PATCH] mergetool--lib: add support for p4merge","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-10-28T09:27:19Z","receivedAt":"2009-10-28T09:27:19Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Oct 28, 2009 at 02:21:46AM -0700, Jay Soffian wrote:\n> On Wed, Oct 28, 2009 at 2:11 AM, Jay Soffian <jaysoffian@gmail.com> wrote:\n> > Add native support for p4merge as a diff / merge tool. There are two\n> > oddities. First, launching p4merge on Mac OS X requires running a helper\n> > shim (which in turn is looked for in a couple common locations, as it's\n> > very unlikely to be in PATH). Second, p4merge seems to require its file\n> > arguments be absolute paths.\n> >\n> > Signed-off-by: Jay Soffian <jaysoffian@gmail.com>\n> > ---\n> \n> Amusing. I didn't see Scott's patch.\n\nApologies Jay!\n\nIt's late and I saw Scott listed in the CC: and guessed wrong ;)\n\nThanks for the patch again, man.  Good stuff.\n\n\n\n> \n> But, in my testing, for things to work properly I needed to use\n> launchp4merge per:\n> \n> http://kb.perforce.com/AllPerforceApplications/StandAloneClients/P4merge/CommandLineP..rgeOnMacOsX\n> \n> And I also found things didn't work properly unless I provided an absolute path.\n> \n> (Aside, the \"right\" way to launch p4merge, at least on 10.6 would be:\n> \n> /usr/bin/open -b com.perforce.p4merge -W -n --args <args to p4merge...>\n> \n> This way OS X's launch services would find p4merge.app wherever it is\n> on the user's system. But, I think some of these options to open are\n> 10.6 specific and in practice looking in /Applications and\n> $HOME/Applications I think is a sane enough default.)\n> \n> j.\n\n-- \n\t\tDavid\n"},{"id":"126120","messageId":"20091028093655.GC90780@gmail.com","threadId":"21399","inReplyTo":"76718490910280221u4e1d3e78me7f9b0b45f590e56@mail.gmail.com","subject":"Re: [PATCH] mergetool--lib: add support for p4merge","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-10-28T09:36:56Z","receivedAt":"2009-10-28T09:36:56Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Oct 28, 2009 at 02:21:46AM -0700, Jay Soffian wrote:\n> \n> But, in my testing, for things to work properly I needed to use\n> launchp4merge per:\n> \n> http://kb.perforce.com/AllPerforceApplications/StandAloneClients/P4merge/CommandLineP..rgeOnMacOsX\n> \n> And I also found things didn't work properly unless I provided an absolute path.\n> \n> (Aside, the \"right\" way to launch p4merge, at least on 10.6 would be:\n> \n> /usr/bin/open -b com.perforce.p4merge -W -n --args <args to p4merge...>\n\n\n:(\n\nI tested on 10.5, so there's definitely some difference in\nbehavior since difftool.p4merge.path is all that was needed here\n(with an absolute path as I mentioned).\n\n\n> This way OS X's launch services would find p4merge.app wherever it is\n> on the user's system. But, I think some of these options to open are\n> 10.6 specific and in practice looking in /Applications and\n> $HOME/Applications I think is a sane enough default.)\n\nWe've stayed away from hard-coding any platform-specific paths\nin mergetool--lib in the past.  It's a practicality thing --\ntrying to guess all of the possible installation locations is\nsimply untenable.\n\nHere's an old thread where we talked about this in the context\nof ecmerge:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/118125/focus=118182\n\n\nLet me know what you think.\n\n-- \n\t\tDavid\n"},{"id":"126122","messageId":"76718490910280252n7a2c41baj5e220c784f1f3617@mail.gmail.com","threadId":"21399","inReplyTo":"20091028093655.GC90780@gmail.com","subject":"Re: [PATCH] mergetool--lib: add support for p4merge","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-10-28T09:52:28Z","receivedAt":"2009-10-28T09:52:28Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Wed, Oct 28, 2009 at 2:36 AM, David Aguilar <davvid@gmail.com> wrote:\n> I tested on 10.5, so there's definitely some difference in\n> behavior since difftool.p4merge.path is all that was needed here\n> (with an absolute path as I mentioned).\n\nThere are two issues here:\n\n1) Is it necessary to use launchp4merge, or is calling p4merge directly okay:\n\nCalling p4merge directly works, but the advantage of calling\nlaunchp4merge instead is that it works a little more elegantly:\n\n- If p4merge is already running, using launchp4merge opens a new\nwindow inside the running instance, instead of  launching p4merge\nanew.\n\n- After performing the merge, it is sufficient to just close the merge\nwindow, as opposed to having to quit the p4merge application (which\nyou have to do if you run p4merge directly).\n\n2) Does p4merge/launchp4merge require absolute paths for its arguments.\n\nI found that it does. So I wonder whether possibly you tested with\ndifftool, which uses absolute arguments, as opposed to mergetool,\nwhich uses relative arguments. You'd only see the issue with\nmergetool.\n\n> We've stayed away from hard-coding any platform-specific paths\n> in mergetool--lib in the past.  It's a practicality thing --\n> trying to guess all of the possible installation locations is\n> simply untenable.\n>\n> Here's an old thread where we talked about this in the context\n> of ecmerge:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/118125/focus=118182\n>\n> Let me know what you think.\n\nDisagree with the conclusion of that thread. On Linux, PATH is very\nlikely to have the merge/diff binary in it. On OS X, this is highly\nunlikely, but there are two very common/likely locations we can look,\n/Applications and $HOME/Applications.\n\nj.\n"},{"id":"126123","messageId":"76718490910280302o7db153dah50d7c2c4afece8e6@mail.gmail.com","threadId":"21399","inReplyTo":"76718490910280252n7a2c41baj5e220c784f1f3617@mail.gmail.com","subject":"Re: [PATCH] mergetool--lib: add support for p4merge","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-10-28T10:02:45Z","receivedAt":"2009-10-28T10:02:45Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Wed, Oct 28, 2009 at 2:52 AM, Jay Soffian <jaysoffian@gmail.com> wrote:\n> 2) Does p4merge/launchp4merge require absolute paths for its arguments.\n>\n> I found that it does.\n\nI lie. Running p4merge directly works with relative paths. Launching\np4merge via launchp4merge (or via open) requires absolute paths, which\nI guess makes sense.\n\nI'd still lean toward using launchp4merge with absolute paths.\n\n$0.02.\n\nj.\n"}]}