{"thread":{"id":"33089","subject":"[PATCH 2/2] p4merge: create a virtual base if none available","startedAt":"2013-03-06T20:32:56Z","lastAt":"2013-03-25T19:24:12Z","messageCount":40,"participants":["Kevin Bracey","Junio C Hamano","David Aguilar","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"210730","messageId":"1362601978-16911-1-git-send-email-kevin@bracey.fi","threadId":"33089","inReplyTo":null,"subject":"[PATCH 0/2] Improve P4Merge mergetool invocation","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-06T20:32:56Z","receivedAt":"2013-03-06T20:32:56Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"Two changes to the same piece of code that have greatly improved the behaviour\nof P4Merge for me. Some of it may also be applicable to other mergetools.\n\nI've put probably overly-long-winded explanations in the commit messages.\n\nComments welcome. In particular, I know almost nothing of sh, so I may have\nmade some blunder there.\n\nKevin Bracey (2):\n p4merge: swap LOCAL and REMOTE for mergetool\n p4merge: create a virtual base if none available\n\n git-mergetool--lib.sh | 14 ++++++++++++++\n mergetools/p4merge | 4 ++--\n 2 files changed, 16 insertions(+), 2 deletions(-)\n\n-- \n1.8.2.rc2.5.g1a80410.dirty\n"},{"id":"210719","messageId":"1362601978-16911-2-git-send-email-kevin@bracey.fi","threadId":"33089","inReplyTo":"1362601978-16911-1-git-send-email-kevin@bracey.fi","subject":"[PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-06T20:32:57Z","receivedAt":"2013-03-06T20:32:57Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"Reverse LOCAL and REMOTE when invoking P4Merge as a mergetool, so that\nthe incoming branch is now in the left-hand, blue triangle pane, and the\ncurrent branch is in the right-hand, green circle pane.\n\nThis change makes use of P4Merge consistent with its built-in help, its\nreference documentation, and Perforce itself. But most importantly, it\nmakes merge results clearer. P4Merge is not totally symmetrical between\nleft and right; despite changing a few text labels from \"theirs/ours\" to\n\"left/right\" when invoked manually, it still retains its original\nPerforce \"theirs/ours\" viewpoint.\n\nMost obviously, in the result pane P4Merge shows changes that are common\nto both branches in green. This is on the basis of the current branch\nbeing green, as it is when invoked from Perforce; it means that lines in\nthe result are blue if and only if they are being changed by the merge,\nmaking the resulting diff clearer. Whereas if you use blue as the\ncurrent branch, then there is no single colour highlighting changes -\na green line in the result could be a change, but it could also be\nsomething already in the current branch that isn't changed by the merge.\n\nThere is no need to swap LOCAL/REMOTE order for difftool; P4Merge is\nsymmetrical in this case, and a 0- or 1-revision difftool invocation\nalready gives the working tree (\"ours\") on the right in green, matching\nPerforce's equivalent \"Diff Against Have Revision\". And you couldn't\nswap it anyway, as it would make 2-revision difftool invocation\nback-to-front.\n\nNote that P4Merge now shows \"ours\" on the right for both diff and merge,\nunlike other diff/mergetools, which always have REMOTE on the right.\nBut observe that REMOTE is the working tree (ie \"ours\") for a diff,\nwhile it's another branch (ie \"theirs\") for a merge.\n\nSigned-off-by: Kevin Bracey \n---\n mergetools/p4merge | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/mergetools/p4merge b/mergetools/p4merge\nindex 8a36916..46b3a5a 100644\n--- a/mergetools/p4merge\n+++ b/mergetools/p4merge\n@@ -22,7 +22,7 @@ diff_cmd () {\n merge_cmd () {\n \ttouch \"$BACKUP\"\n \t$base_present || >\"$BASE\"\n-\t\"$merge_tool_path\" \"$BASE\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n+\t\"$merge_tool_path\" \"$BASE\" \"$REMOTE\" \"$LOCAL\" \"$MERGED\"\n \tcheck_unchanged\n }\n \n-- \n1.8.2.rc2.5.g1a80410.dirty\n"},{"id":"210716","messageId":"1362601978-16911-3-git-send-email-kevin@bracey.fi","threadId":"33089","inReplyTo":"1362601978-16911-1-git-send-email-kevin@bracey.fi","subject":"[PATCH 2/2] p4merge: create a virtual base if none available","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-06T20:32:58Z","receivedAt":"2013-03-06T20:32:58Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"Originally, with no base, Git gave P4Merge $LOCAL as a dummy base:\n\n p4merge \"$LOCAL\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n\nCommit 0a0ec7bd changed this to:\n\n p4merge \"empty file\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n\nto avoid the problem of being unable to save in some circumstances.\n\nUnfortunately this approach does not produce good results at all on\ndiffering inputs. P4Merge really regards the blank file as the base, and\nonce you have just a couple of differences between the two branches you\nend up with one a massive full-file conflict. The diff is not readable,\nand you have to invoke \"difftool MERGE_HEAD HEAD\" manually to see a\n2-way diff.\n\nThe original form appears to have invoked special 2-way comparison\nbehaviour that occurs only if the base filename is \"\" or equal to the\nleft input. You get a good diff, and it does not auto-resolve in one\ndirection or the other. (Normally if one branch equals the base, it\nwould autoresolve to the other branch).\n\nBut there appears to be no way of getting this 2-way behaviour and being\nable to reliably save. Having base=left appears to be triggering other\nassumptions. There are tricks the user can use to force the save icon\non, but it's not intuitive.\n\nSo we now follow a suggestion given in the original patch's discussion:\ngenerate a virtual base, consisting of the lines common to the two\nbranches. It produces a much nicer 3-way diff view than either of the\noriginal forms, and than I suspect other mergetools are managing.\n\nSigned-off-by: Kevin Bracey \n---\n git-mergetool--lib.sh | 14 ++++++++++++++\n mergetools/p4merge | 2 +-\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex e338be5..5b60cf5 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -108,6 +108,20 @@ check_unchanged () {\n \tfi\n }\n \n+make_virtual_base() {\n+\t\t# Copied from git-merge-one-file.sh.\n+\t\t# This starts with $LOCAL, and uses git apply to\n+\t\t# remove lines that are not in $REMOTE.\n+\t\tcp -- \"$LOCAL\" \"$BASE\"\n+\t\tsz0=`wc -c <\"$BASE\"`\n+\t\t@@DIFF@@ -u -L\"a/$BASE\" -L\"b/$BASE\" \"$BASE\" \"$REMOTE\" | git apply --no-add\n+\t\tsz1=`wc -c <\"$BASE\"`\n+\n+\t\t# If we do not have enough common material, it is not\n+\t\t# worth trying two-file merge using common subsections.\n+\t\texpr $sz0 \\< $sz1 \\* 2 >/dev/null || : >\"$BASE\"\n+}\n+\n valid_tool () {\n \tsetup_tool \"$1\" && return 0\n \tcmd=$(get_merge_tool_cmd \"$1\")\ndiff --git a/mergetools/p4merge b/mergetools/p4merge\nindex 46b3a5a..f0a893b 100644\n--- a/mergetools/p4merge\n+++ b/mergetools/p4merge\n@@ -21,7 +21,7 @@ diff_cmd () {\n \n merge_cmd () {\n \ttouch \"$BACKUP\"\n-\t$base_present || >\"$BASE\"\n+\t$base_present || make_virtual_base\n \t\"$merge_tool_path\" \"$BASE\" \"$REMOTE\" \"$LOCAL\" \"$MERGED\"\n \tcheck_unchanged\n }\n-- \n1.8.2.rc2.5.g1a80410.dirty\n"},{"id":"210735","messageId":"7vlia0nj0i.fsf@alter.siamese.dyndns.org","threadId":"33089","inReplyTo":"1362601978-16911-2-git-send-email-kevin@bracey.fi","subject":"Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-07T00:36:13Z","receivedAt":"2013-03-07T00:36:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Bracey writes:\n\n> Reverse LOCAL and REMOTE when invoking P4Merge as a mergetool, so that\n> the incoming branch is now in the left-hand, blue triangle pane, and the\n> current branch is in the right-hand, green circle pane.\n\nGiven that the ordering of the three variants has been the way it is\nsince the very initial version by Scott, I'll sit on this patch\nuntil hearing from those Cc'ed (who presumably do use p4merge,\nunlike I who don't) that it is a good change.\n\nThanks.\n"},{"id":"210741","messageId":"CAJDDKr6+VRnc-HK52woHHLtAqXau=76Gc+Ag=keiMGffuco64A@mail.gmail.com","threadId":"33089","inReplyTo":"1362601978-16911-3-git-send-email-kevin@bracey.fi","subject":"Re: [PATCH 2/2] p4merge: create a virtual base if none available","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-03-07T02:23:02Z","receivedAt":"2013-03-07T02:23:02Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Mar 6, 2013 at 12:32 PM, Kevin Bracey wrote:\n> Originally, with no base, Git gave P4Merge $LOCAL as a dummy base:\n>\n> p4merge \"$LOCAL\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n>\n> Commit 0a0ec7bd changed this to:\n>\n> p4merge \"empty file\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n>\n> to avoid the problem of being unable to save in some circumstances.\n>\n> Unfortunately this approach does not produce good results at all on\n> differing inputs. P4Merge really regards the blank file as the base, and\n> once you have just a couple of differences between the two branches you\n> end up with one a massive full-file conflict. The diff is not readable,\n> and you have to invoke \"difftool MERGE_HEAD HEAD\" manually to see a\n> 2-way diff.\n>\n> The original form appears to have invoked special 2-way comparison\n> behaviour that occurs only if the base filename is \"\" or equal to the\n> left input. You get a good diff, and it does not auto-resolve in one\n> direction or the other. (Normally if one branch equals the base, it\n> would autoresolve to the other branch).\n>\n> But there appears to be no way of getting this 2-way behaviour and being\n> able to reliably save. Having base=left appears to be triggering other\n> assumptions. There are tricks the user can use to force the save icon\n> on, but it's not intuitive.\n>\n> So we now follow a suggestion given in the original patch's discussion:\n> generate a virtual base, consisting of the lines common to the two\n> branches. It produces a much nicer 3-way diff view than either of the\n> original forms, and than I suspect other mergetools are managing.\n>\n> Signed-off-by: Kevin Bracey \n> ---\n> git-mergetool--lib.sh | 14 ++++++++++++++\n> mergetools/p4merge | 2 +-\n> 2 files changed, 15 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index e338be5..5b60cf5 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -108,6 +108,20 @@ check_unchanged () {\n> fi\n> }\n>\n> +make_virtual_base() {\n> + # Copied from git-merge-one-file.sh.\n\nI think the reasoning behind these patches is good.\n\nHow do we feel about this duplication?\nShould we make a common function in the git-sh-setup.sh,\nor is it okay to have a slightly modified version of this\nfunction in two places?\n\n> + # This starts with $LOCAL, and uses git apply to\n> + # remove lines that are not in $REMOTE.\n> + cp -- \"$LOCAL\" \"$BASE\"\n> + sz0=`wc -c <\"$BASE\"`\n> + @@DIFF@@ -u -L\"a/$BASE\" -L\"b/$BASE\" \"$BASE\" \"$REMOTE\" | git apply --no-add\n> + sz1=`wc -c <\"$BASE\"`\n> +\n> + # If we do not have enough common material, it is not\n> + # worth trying two-file merge using common subsections.\n> + expr $sz0 \\< $sz1 \\* 2 >/dev/null || : >\"$BASE\"\n> +}\n> +\n> valid_tool () {\n> setup_tool \"$1\" && return 0\n> cmd=$(get_merge_tool_cmd \"$1\")\n> diff --git a/mergetools/p4merge b/mergetools/p4merge\n> index 46b3a5a..f0a893b 100644\n> --- a/mergetools/p4merge\n> +++ b/mergetools/p4merge\n> @@ -21,7 +21,7 @@ diff_cmd () {\n>\n> merge_cmd () {\n> touch \"$BACKUP\"\n> - $base_present || >\"$BASE\"\n> + $base_present || make_virtual_base\n> \"$merge_tool_path\" \"$BASE\" \"$REMOTE\" \"$LOCAL\" \"$MERGED\"\n> check_unchanged\n> }\n> --\n> 1.8.2.rc2.5.g1a80410.dirty\n>\n\n\n\n-- \nDavid\n"},{"id":"210742","messageId":"CAJDDKr5YOONEKKXRH4yO55SdC235QROsnTa6o7UGzXtmgm6EWA@mail.gmail.com","threadId":"33089","inReplyTo":"1362601978-16911-3-git-send-email-kevin@bracey.fi","subject":"Re: [PATCH 2/2] p4merge: create a virtual base if none available","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-03-07T03:33:26Z","receivedAt":"2013-03-07T03:33:26Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Mar 6, 2013 at 12:32 PM, Kevin Bracey wrote:\n> +make_virtual_base() {\n> + # Copied from git-merge-one-file.sh.\n> + # This starts with $LOCAL, and uses git apply to\n> + # remove lines that are not in $REMOTE.\n> + cp -- \"$LOCAL\" \"$BASE\"\n> + sz0=`wc -c <\"$BASE\"`\n> + @@DIFF@@ -u -L\"a/$BASE\" -L\"b/$BASE\" \"$BASE\" \"$REMOTE\" | git apply --no-add\n> + sz1=`wc -c <\"$BASE\"`\n> +\n> + # If we do not have enough common material, it is not\n> + # worth trying two-file merge using common subsections.\n> + expr $sz0 \\< $sz1 \\* 2 >/dev/null || : >\"$BASE\"\n> +}\n\nThis seems to be indented deeper then the other functions\n(or gmail is whitespace damaging my view).\nPlease use one hard tab to indent here.\n\nWe prefer $(command) instead of `command`.\nThese should be adjusted.\n\nAlso, the \"@@DIFF@@\" string may not work here.\nThis is a template string that is replaced by the Makefile.\n\nI don't think the tools in the mergetools/ directory go through\ncmd_munge_script so this is not going to work as-is.\n\nCan the same thing be accomplished using \"git diff --no-index\"\nso that we do not need a dependency on an external \"diff\" command here?\n\n\nI am not a regular p4merge user myself so I'll defer to others\non the cc: list for their thoughts. It does seem like a good idea, though.\n-- \nDavid\n"},{"id":"210745","messageId":"513830AD.10302@bracey.fi","threadId":"33089","inReplyTo":"7vlia0nj0i.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-07T06:16:13Z","receivedAt":"2013-03-07T06:16:13Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"On 07/03/2013 02:36, Junio C Hamano wrote:\n> Kevin Bracey writes:\n>\n>> Reverse LOCAL and REMOTE when invoking P4Merge as a mergetool, so that\n>> the incoming branch is now in the left-hand, blue triangle pane, and the\n>> current branch is in the right-hand, green circle pane.\n> Given that the ordering of the three variants has been the way it is\n> since the very initial version by Scott, I'll sit on this patch\n> until hearing from those Cc'ed (who presumably do use p4merge,\n> unlike I who don't) that it is a good change.\n>\nI agree that this is the controversial patch of the two. It's going to \nchuck away 3-4 years of what Git users are used to, albeit in favour of \na decade of what Perforce users are used to. And it also makes it \ninconsistent with all the other mergetools (at least assuming their \ndisplay matches their command line).\n\nI checked for any historical discussion from when this was added about \nthe order, and there was none. So I'm assuming it was just done to match \nthe other tools, maybe not realising P4Merge's \"theirs/ours\" convention. \nThere was no explicit recognition at the time that they were breaking \nthe Perforce convention, or that the order had an effect.\n\nI've used both Git and Perforce for quite a while, but have only just \nstarted using P4Merge with Git. It seemed weirdly off and unintuitive to \nme at first, until I suddenly realised that it was just backwards. I \nwould have settled for just having to get used to driving on the other \nside of the road, and matching other mergetools, until I realised that \nit effectively broke display of common changes. That's a problem.\n\nOn consistency, personally, I think there's an argument for reversing \nall the mergetools to match this way, as I find this orientation more \nintuitively aligns with difftool. But I'm not bold enough to suggest \nthat. Yet.\n\nKevin\n"},{"id":"210755","messageId":"51383370.3050806@bracey.fi","threadId":"33089","inReplyTo":"CAJDDKr6+VRnc-HK52woHHLtAqXau=76Gc+Ag=keiMGffuco64A@mail.gmail.com","subject":"Re: [PATCH 2/2] p4merge: create a virtual base if none available","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-07T06:28:00Z","receivedAt":"2013-03-07T06:28:00Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"On 07/03/2013 04:23, David Aguilar wrote:\n> On Wed, Mar 6, 2013 at 12:32 PM, Kevin Bracey wrote:\n>> +make_virtual_base() {\n>> + # Copied from git-merge-one-file.sh.\n> I think the reasoning behind these patches is good.\n>\n> How do we feel about this duplication?\nBad.\n> Should we make a common function in the git-sh-setup.sh,\n> or is it okay to have a slightly modified version of this\n> function in two places?\nI'd prefer to have a common function, I just didn't know if there was \nsomewhere appropriate to place it, available from both files. And I'm \ngoing to have to learn a bit more sh to get it right.\n> Also, the \"@@DIFF@@\" string may not work here.\n> This is a template string that is replaced by the Makefile.\n\nIt does work in git-mergetool--lib.sh, but not in mergetools/p4merge.\n\n> We prefer $(command) instead of `command`.\n> These should be adjusted.\n>\n> Can the same thing be accomplished using \"git diff --no-index\"\n> so that we do not need a dependency on an external \"diff\" command here?\nDo these comments still apply if it's a common function in \ngit-sh-setup.sh that git-one-merge-file.sh will use? I'm wary of \nlayering violations.\n\nKevin\n"},{"id":"210747","messageId":"7vd2vboepi.fsf@alter.siamese.dyndns.org","threadId":"33089","inReplyTo":"513830AD.10302@bracey.fi","subject":"Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-07T07:23:53Z","receivedAt":"2013-03-07T07:23:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Bracey writes:\n\n> I agree that this is the controversial patch of the two. It's going to\n> chuck away 3-4 years of what Git users are used to, albeit in favour\n> of a decade of what Perforce users are used to. And it also makes it\n> inconsistent with all the other mergetools (at least assuming their\n> display matches their command line).\n\nIf p4merge GUI labels one side clearly as \"theirs\" and the other\n\"ours\", and the way we feed the inputs to it makes the side that is\nactually \"ours\" appear in p4merge GUI labelled as \"theirs\", then I\ndo not think backward compatibility argument does not hold water. It\nis just correcting a longstanding 3-4 year old bug in a tool that\nnobody noticed.\n\nFor people who are very used to the way p4merge shows the merged\ncontents by theirs-base-yours order in side-by-side view, I do not\nthink it is unreasonable to introduce the \"mergetool.$name.reverse\"\nconfiguration variable and teach the mergetool frontend to pay\nattention to it. That will allow them to see their merge in reverse\norder even when they are using a backend other than p4merge.\n\nWith such a mechanism in place, by default, we can just declare that\nmergetool.p4merge.reverse is \"true\" when unset, while making\nmergetool.$name.reverse for all the other tools default to \"false\".\nPeople who are already used to the way our p4merge integration works\ncan set mergetool.p4merge.reverse to \"false\" explicitly to retain\nthe historical behaviour that you are declaring \"buggy\" with such a\nchange.\n\nHow does that sound? David?\n"},{"id":"210748","messageId":"7v8v5zoem7.fsf@alter.siamese.dyndns.org","threadId":"33089","inReplyTo":"CAJDDKr6+VRnc-HK52woHHLtAqXau=76Gc+Ag=keiMGffuco64A@mail.gmail.com","subject":"Re: [PATCH 2/2] p4merge: create a virtual base if none available","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-07T07:25:52Z","receivedAt":"2013-03-07T07:25:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> How do we feel about this duplication?\n> Should we make a common function in the git-sh-setup.sh,\n> or is it okay to have a slightly modified version of this\n> function in two places?\n\nIt probably is a good idea to have it in one place. That would also\nsolve the @@DIFF@@ replacement issue you noticed at the same time.\n"},{"id":"210777","messageId":"5138CAFE.2010602@bracey.fi","threadId":"33089","inReplyTo":"7vd2vboepi.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-07T17:14:38Z","receivedAt":"2013-03-07T17:14:38Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"On 07/03/2013 09:23, Junio C Hamano wrote:\n> If p4merge GUI labels one side clearly as \"theirs\" and the other \n> \"ours\", and the way we feed the inputs to it makes the side that is \n> actually \"ours\" appear in p4merge GUI labelled as \"theirs\", then I do \n> not think backward compatibility argument does not hold water. It is \n> just correcting a longstanding 3-4 year old bug in a tool that nobody \n> noticed.\n\nIt's not quite that clear-cut. Some years ago, and before p4merge was \nadded as a Git mergetool, P4Merge was changed so its main GUI text says \n\"left\" and \"right\" instead of \"theirs\" and \"ours\" when invoked manually.\n\nBut it appears that's as far as they went. It doesn't seem any of its \nasymmetric diff display logic was changed; it works better with ours on \nthe right, and the built-in help all remains written on the theirs/ours \nbasis. And even little details like the icons imply it (a square for the \nbase, a downward-pointing triangle for their incoming stuff, and a \ncircle for the version we hold).\n\n> For people who are very used to the way p4merge shows the merged\n> contents by theirs-base-yours order in side-by-side view, I do not\n> think it is unreasonable to introduce the \"mergetool.$name.reverse\"\n> configuration variable and teach the mergetool frontend to pay\n> attention to it. That will allow them to see their merge in reverse\n> order even when they are using a backend other than p4merge.\n>\n> With such a mechanism in place, by default, we can just declare that\n> mergetool.p4merge.reverse is \"true\" when unset, while making\n> mergetool.$name.reverse for all the other tools default to \"false\".\n> People who are already used to the way our p4merge integration works\n> can set mergetool.p4merge.reverse to \"false\" explicitly to retain\n> the historical behaviour that you are declaring \"buggy\" with such a\n> change.\n\nI like this idea as a user - having made this change to p4merge, it does \nthrow me when I decide to attempt a particularly tricky merge with bc3 \ninstead, and get the other order. The user config options you suggest \nsound good to me.\n\nFor completion on this idea, I'd suggest difftool.xxx.reverse, to allow \nthe orientation for 0- and 1-revision diffs to be chosen - allow the \nimplied working tree version to be on the left or right. That would \nallow \"ours-theirs\" order, which some would view as being more \nconsistent with the \"ours-base-theirs\" default for mergetool.\n\nWould it be going too far to also have \"xxxtool.reverse\" to choose the \nglobal default? Then the choice hierarchy would be \"xxxtool.xxx.reverse \nif set\" > \"optional inbuilt tool preference\" > \"xxxtool.reverse if set\" \n > \"false\". So the user could request a global swap, except that they'd \nhave to explicitly override any tools that have a preferred orientation.\n\nMy only reservation is that I assume it would be implemented by swapping \nwhat's passed in $LOCAL and $REMOTE. Which seems a bit icky: \n$LOCAL=\"a.REMOTE.1234.c\". On the other hand, $LOCAL and $REMOTE are \nalready not very meaningful names for difftool... Maybe we should change \nto using $LEFT and $RIGHT, acknowledging the existing difftool \nsituation, and that the user can now swap merges too.\n\nI'd be happy to prepare a fuller patch on this sort of basis.\n\nKevin\n"},{"id":"210784","messageId":"7vboavm3fh.fsf@alter.siamese.dyndns.org","threadId":"33089","inReplyTo":"5138CAFE.2010602@bracey.fi","subject":"Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-07T19:10:26Z","receivedAt":"2013-03-07T19:10:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Bracey writes:\n\n> On 07/03/2013 09:23, Junio C Hamano wrote:\n>> If p4merge GUI labels one side clearly as \"theirs\" and the other\n>> \"ours\", and the way we feed the inputs to it makes the side that is\n>> actually \"ours\" appear in p4merge GUI labelled as \"theirs\", then I\n>> do not think backward compatibility argument does not hold water. It\n>> is just correcting a longstanding 3-4 year old bug in a tool that\n>> nobody noticed.\n>\n> It's not quite that clear-cut. Some years ago, and before p4merge was\n> added as a Git mergetool, P4Merge was changed so its main GUI text\n> says \"left\" and \"right\" instead of \"theirs\" and \"ours\" when invoked\n> manually.\n>\n> But it appears that's as far as they went. It doesn't seem any of its\n> asymmetric diff display logic was changed; it works better with ours\n> on the right, and the built-in help all remains written on the\n> theirs/ours basis. And even little details like the icons imply it (a\n> square for the base, a downward-pointing triangle for their incoming\n> stuff, and a circle for the version we hold).\n\nSo in short, a user of p4merge can see that left side is intended as\n\"theirs\", even though recent p4merge sometimes calls it \"left\". And\nyour description on the coloring (green vs blue) makes it clear that\n\"left\" and \"theirs\" are still intended to be synonyms.\n\nIf that is the case I would think you can still argue such a change\nas \"correcting a 3-4-year old bug\".\n\n> Would it be going too far to also have \"xxxtool.reverse\" to choose the\n> global default?\n\nIt would be a natural thing to do. I left it out because I thought\nit would go without saying, given that precedences already exist,\ne.g. mergetool.keepBackup etc.\n\n> My only reservation is that I assume it would be implemented by\n> swapping what's passed in $LOCAL and $REMOTE. Which seems a bit icky:\n> $LOCAL=\"a.REMOTE.1234.c\".\n\nDoesn't the UI show the actual temporary filename? When merging my\nversion of hello.c with your version, showing them as hello.LOCAL.c\nand hello.REMOTE.c is an integral part of the UI experience, I\nthink, even if the GUI tool does not give its own labels (and\nbehaviour differences as you mentioned for p4merge) to mark which\nside is theirs and which side is ours. The temporary file that\nholds their version should still be named with REMOTE, even when the\nmergetool.reverse option is in effect.\n\nAs to the name of the variable, I do not care too deeply about it\nmyself, but I think keeping the current LOCAL and REMOTE would help\npeople following the code, especially given the option is called\n\"reverse\", meaning that there is an internal convention that the\norder is \"LOCAL and then REMOTE\".\n\nOne thing to watch out for is from which temporary file we take the\nmerged results. You can present the two sides swapped, but if the\ntool always writes the results out by updating the second file, the\ncaller needs to be prepared to read from the one that gets changed.\n"},{"id":"210785","messageId":"CAJDDKr5-ttcU48r0-qTfov7q736Rj63rS33fTScSsvx53VG4pA@mail.gmail.com","threadId":"33089","inReplyTo":"7vboavm3fh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-03-07T19:50:36Z","receivedAt":"2013-03-07T19:50:36Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Mar 7, 2013 at 11:10 AM, Junio C Hamano wrote:\n> Kevin Bracey writes:\n>\n>> On 07/03/2013 09:23, Junio C Hamano wrote:\n>>> If p4merge GUI labels one side clearly as \"theirs\" and the other\n>>> \"ours\", and the way we feed the inputs to it makes the side that is\n>>> actually \"ours\" appear in p4merge GUI labelled as \"theirs\", then I\n>>> do not think backward compatibility argument does not hold water. It\n>>> is just correcting a longstanding 3-4 year old bug in a tool that\n>>> nobody noticed.\n>>\n>> It's not quite that clear-cut. Some years ago, and before p4merge was\n>> added as a Git mergetool, P4Merge was changed so its main GUI text\n>> says \"left\" and \"right\" instead of \"theirs\" and \"ours\" when invoked\n>> manually.\n>>\n>> But it appears that's as far as they went. It doesn't seem any of its\n>> asymmetric diff display logic was changed; it works better with ours\n>> on the right, and the built-in help all remains written on the\n>> theirs/ours basis. And even little details like the icons imply it (a\n>> square for the base, a downward-pointing triangle for their incoming\n>> stuff, and a circle for the version we hold).\n>\n> So in short, a user of p4merge can see that left side is intended as\n> \"theirs\", even though recent p4merge sometimes calls it \"left\". And\n> your description on the coloring (green vs blue) makes it clear that\n> \"left\" and \"theirs\" are still intended to be synonyms.\n>\n> If that is the case I would think you can still argue such a change\n> as \"correcting a 3-4-year old bug\".\n\nI would prefer to treat this as a bugfix rather than introducing\na new set of configuration knobs if possible. It really does\nseem like a correction.\n\nUsers that want the traditional behavior can get that by\nconfiguring a custom mergetool.p4merge.cmd, so we're not\ncompletely losing the ability to get at the old behavior.\n\nUsers that want to see a reverse diff with difftool can\nalready say \"--reverse\", so there's even less reason to\nhave it there (though I know we're talking about mergetool only).\n\n\n>> Would it be going too far to also have \"xxxtool.reverse\" to choose the\n>> global default?\n>\n> It would be a natural thing to do. I left it out because I thought\n> it would go without saying, given that precedences already exist,\n> e.g. mergetool.keepBackup etc.\n\nMedium NACK. If we can do without configuration all the better.\n\nI would much rather prefer to have the default/mainstream\nbehavior be the best out-of-the-box sans configuration.\n\nThe reasoning behind swapping them for p4merge makes sense\nfor p4merge only. I don't think we're quite ready to declare\nthat all the merge tools need to be swapped or that we need a\nmechanism for swapping the order.\n\n>> My only reservation is that I assume it would be implemented by\n>> swapping what's passed in $LOCAL and $REMOTE. Which seems a bit icky:\n>> $LOCAL=\"a.REMOTE.1234.c\".\n>\n> Doesn't the UI show the actual temporary filename? When merging my\n> version of hello.c with your version, showing them as hello.LOCAL.c\n> and hello.REMOTE.c is an integral part of the UI experience, I\n> think, even if the GUI tool does not give its own labels (and\n> behaviour differences as you mentioned for p4merge) to mark which\n> side is theirs and which side is ours. The temporary file that\n> holds their version should still be named with REMOTE, even when the\n> mergetool.reverse option is in effect.\n>\n> As to the name of the variable, I do not care too deeply about it\n> myself, but I think keeping the current LOCAL and REMOTE would help\n> people following the code, especially given the option is called\n> \"reverse\", meaning that there is an internal convention that the\n> order is \"LOCAL and then REMOTE\".\n>\n> One thing to watch out for is from which temporary file we take the\n> merged results. You can present the two sides swapped, but if the\n> tool always writes the results out by updating the second file, the\n> caller needs to be prepared to read from the one that gets changed.\n-- \nDavid\n"},{"id":"210786","messageId":"7v4ngnlznw.fsf@alter.siamese.dyndns.org","threadId":"33089","inReplyTo":"CAJDDKr5-ttcU48r0-qTfov7q736Rj63rS33fTScSsvx53VG4pA@mail.gmail.com","subject":"Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-07T20:31:47Z","receivedAt":"2013-03-07T20:31:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> I would prefer to treat this as a bugfix rather than introducing\n> a new set of configuration knobs if possible. It really does\n> seem like a correction.\n> \n> Users that want the traditional behavior can get that by\n> configuring a custom mergetool.p4merge.cmd, so we're not\n> completely losing the ability to get at the old behavior.\n>\n> Users that want to see a reverse diff with difftool can\n> already say \"--reverse\", so there's even less reason to\n> have it there (though I know we're talking about mergetool only).\n> ...\n> I would much rather prefer to have the default/mainstream\n> behavior be the best out-of-the-box sans configuration.\n>\n> The reasoning behind swapping them for p4merge makes sense\n> for p4merge only. I don't think we're quite ready to declare\n> that all the merge tools need to be swapped or that we need a\n> mechanism for swapping the order.\n\nThanks for an injection of sanity.\n"},{"id":"210932","messageId":"1362856860-15205-1-git-send-email-kevin@bracey.fi","threadId":"33089","inReplyTo":"1362601978-16911-1-git-send-email-kevin@bracey.fi","subject":"[PATCH v2 0/3] Improve P4Merge mergetool invocation","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-09T19:20:57Z","receivedAt":"2013-03-09T19:20:57Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"Incorporated comments on the previous patches, and one new patch\naddressing a problem I spotted while testing git-merge-one-file.\n\nI couldn't figure out how to use git diff to achieve the effect of the\nexternal diff here - we'd need some alternative to achieve what it does\nwith the -L option, and I failed to come up with anything remotely elegant.\n\nKevin Bracey (3):\n mergetools/p4merge: swap LOCAL and REMOTE\n mergetools/p4merge: create a base if none available\n git-merge-one-file: revise merge error reporting\n\n git-merge-one-file.sh | 38 ++++++++++++--------------------------\n git-sh-setup.sh | 13 +++++++++++++\n mergetools/p4merge | 8 ++++++--\n 3 files changed, 31 insertions(+), 28 deletions(-)\n\n-- \n1.8.2.rc3.7.g77aeedb\n"},{"id":"210909","messageId":"1362856860-15205-2-git-send-email-kevin@bracey.fi","threadId":"33089","inReplyTo":"1362856860-15205-1-git-send-email-kevin@bracey.fi","subject":"[PATCH v2 1/3] mergetools/p4merge: swap LOCAL and REMOTE","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-09T19:20:58Z","receivedAt":"2013-03-09T19:20:58Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"Reverse LOCAL and REMOTE when invoking P4Merge as a mergetool, so that\nthe incoming branch is now in the left-hand, blue triangle pane, and the\ncurrent branch is in the right-hand, green circle pane.\n\nThis change makes use of P4Merge consistent with its built-in help, its\nreference documentation, and Perforce itself. But most importantly, it\nmakes merge results clearer. P4Merge is not totally symmetrical between\nleft and right; despite changing a few text labels from \"theirs/ours\" to\n\"left/right\" when invoked manually, it still retains its original\nPerforce \"theirs/ours\" viewpoint.\n\nMost obviously, in the result pane P4Merge shows changes that are common\nto both branches in green. This is on the basis of the current branch\nbeing green, as it is when invoked from Perforce; it means that lines in\nthe result are blue if and only if they are being changed by the merge,\nmaking the resulting diff clearer.\n\nNote that P4Merge now shows \"ours\" on the right for both diff and merge,\nunlike other diff/mergetools, which always have REMOTE on the right.\nBut observe that REMOTE is the working tree (ie \"ours\") for a diff,\nwhile it's another branch (ie \"theirs\") for a merge.\n\nOurs and theirs are reversed for a rebase - see \"git help rebase\".\nHowever, this does produce the desired \"show the results of this commit\"\neffect in P4Merge - changes that remain in the rebased commit (in your\nbranch, but not in the new base) appear in blue; changes that do not\nappear in the rebased commit (from the new base, or common to both) are\nin green. If Perforce had rebase, they'd probably not swap ours/theirs,\nbut make P4Merge show common changes in blue, picking out our changes in\ngreen. We can't do that, so this is next best.\n\nSigned-off-by: Kevin Bracey \n---\n mergetools/p4merge | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/mergetools/p4merge b/mergetools/p4merge\nindex 8a36916..46b3a5a 100644\n--- a/mergetools/p4merge\n+++ b/mergetools/p4merge\n@@ -22,7 +22,7 @@ diff_cmd () {\n merge_cmd () {\n \ttouch \"$BACKUP\"\n \t$base_present || >\"$BASE\"\n-\t\"$merge_tool_path\" \"$BASE\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n+\t\"$merge_tool_path\" \"$BASE\" \"$REMOTE\" \"$LOCAL\" \"$MERGED\"\n \tcheck_unchanged\n }\n \n-- \n1.8.2.rc3.7.g77aeedb\n"},{"id":"210945","messageId":"1362856860-15205-3-git-send-email-kevin@bracey.fi","threadId":"33089","inReplyTo":"1362856860-15205-1-git-send-email-kevin@bracey.fi","subject":"[PATCH v2 2/3] mergetools/p4merge: create a base if none available","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-09T19:20:59Z","receivedAt":"2013-03-09T19:20:59Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"Originally, with no base, Git gave P4Merge $LOCAL as a dummy base:\n\n p4merge \"$LOCAL\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n\nCommit 0a0ec7bd changed this to:\n\n p4merge \"empty file\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n\nto avoid the problem of being unable to save in some circumstances with\nsimilar inputs.\n\nUnfortunately this approach produces much worse results on differing\ninputs. P4Merge really regards the blank file as the base, and once you\nhave just a couple of differences between the two branches you end up\nwith one a massive full-file conflict. The 3-way diff is not readable,\nand you have to invoke \"difftool MERGE_HEAD HEAD\" manually to get a\nuseful view.\n\nThe original approach appears to have invoked special 2-way merge\nbehaviour in P4Merge that occurs only if the base filename is \"\" or\nequal to the left input. You get a good visual comparison, and it does\nnot auto-resolve differences. (Normally if one branch matched the base,\nit would autoresolve to the other branch).\n\nBut there appears to be no way of getting this 2-way behaviour and being\nable to reliably save. Having base==left appears to be triggering other\nassumptions. There are tricks the user can use to force the save icon\non, but it's not intuitive.\n\nSo we now follow a suggestion given in the original patch's discussion:\ngenerate a virtual base, consisting of the lines common to the two\nbranches. This is the same as the technique used in resolve and octopus\nmerges, so we relocate that code to a shared function.\n\nNote that if there are no differences at the same location, this\ntechnique can lead to automatic resolution without conflict, combining\neverything from the 2 files. As with the other merges using this\ntechnique, we assume the user will inspect the result before saving.\n\nSigned-off-by: Kevin Bracey \n---\n git-merge-one-file.sh | 18 +++++-------------\n git-sh-setup.sh | 13 +++++++++++++\n mergetools/p4merge | 6 +++++-\n 3 files changed, 23 insertions(+), 14 deletions(-)\n\ndiff --git a/git-merge-one-file.sh b/git-merge-one-file.sh\nindex f612cb8..1236fbf 100755\n--- a/git-merge-one-file.sh\n+++ b/git-merge-one-file.sh\n@@ -104,30 +104,22 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \t\t;;\n \tesac\n \n-\tsrc2=`git-unpack-file $3`\n+\tsrc1=$(git-unpack-file $2)\n+\tsrc2=$(git-unpack-file $3)\n \tcase \"$1\" in\n \t'')\n \t\techo \"Added $4 in both, but differently.\"\n-\t\t# This extracts OUR file in $orig, and uses git apply to\n-\t\t# remove lines that are unique to ours.\n-\t\torig=`git-unpack-file $2`\n-\t\tsz0=`wc -c <\"$orig\"`\n-\t\t@@DIFF@@ -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add\n-\t\tsz1=`wc -c <\"$orig\"`\n-\n-\t\t# If we do not have enough common material, it is not\n-\t\t# worth trying two-file merge using common subsections.\n-\t\texpr $sz0 \\< $sz1 \\* 2 >/dev/null || : >$orig\n+\t\torig=$(git-unpack-file $2)\n+\t\tcreate_virtual_base \"$orig\" \"$src1\" \"$src2\"\n \t\t;;\n \t*)\n \t\techo \"Auto-merging $4\"\n-\t\torig=`git-unpack-file $1`\n+\t\torig=$(git-unpack-file $1)\n \t\t;;\n \tesac\n \n \t# Be careful for funny filename such as \"-L\" in \"$4\", which\n \t# would confuse \"merge\" greatly.\n-\tsrc1=`git-unpack-file $2`\n \tgit merge-file \"$src1\" \"$orig\" \"$src2\"\n \tret=$?\n \tmsg=\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 795edd2..aa9a732 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -249,6 +249,19 @@ clear_local_git_env() {\n \tunset $(git rev-parse --local-env-vars)\n }\n \n+# Generate a virtual base file for a two-file merge. On entry the\n+# base file $1 should be a copy of $2. Uses git apply to remove\n+# lines from $1 that are not in $3, leaving only common lines.\n+create_virtual_base() {\n+\tsz0=$(wc -c <\"$1\")\n+\t@@DIFF@@ -u -La/\"$1\" -Lb/\"$1\" \"$2\" \"$3\" | git apply --no-add\n+\tsz1=$(wc -c <\"$1\")\n+\n+\t# If we do not have enough common material, it is not\n+\t# worth trying two-file merge using common subsections.\n+\texpr $sz0 \\< $sz1 \\* 2 >/dev/null || : >\"$1\"\n+}\n+\n \n # Platform specific tweaks to work around some commands\n case $(uname -s) in\ndiff --git a/mergetools/p4merge b/mergetools/p4merge\nindex 46b3a5a..16ae0cc 100644\n--- a/mergetools/p4merge\n+++ b/mergetools/p4merge\n@@ -21,7 +21,11 @@ diff_cmd () {\n \n merge_cmd () {\n \ttouch \"$BACKUP\"\n-\t$base_present || >\"$BASE\"\n+\tif ! $base_present\n+\tthen\n+\t\tcp -- \"$LOCAL\" \"$BASE\"\n+\t\tcreate_virtual_base \"$BASE\" \"$LOCAL\" \"$REMOTE\"\n+\tfi\n \t\"$merge_tool_path\" \"$BASE\" \"$REMOTE\" \"$LOCAL\" \"$MERGED\"\n \tcheck_unchanged\n }\n-- \n1.8.2.rc3.7.g77aeedb\n"},{"id":"210922","messageId":"1362856860-15205-4-git-send-email-kevin@bracey.fi","threadId":"33089","inReplyTo":"1362856860-15205-1-git-send-email-kevin@bracey.fi","subject":"[PATCH v2 3/3] git-merge-one-file: revise merge error reporting","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-09T19:21:00Z","receivedAt":"2013-03-09T19:21:00Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"Commit 718135e improved the merge error reporting for the resolve\nstrategy's merge conflict and permission conflict cases, but led to a\nmalformed \"ERROR: in myfile.c\" message in the case of a file added\ndifferently.\n\nThis commit reverts that change, and uses an alternative approach without\nthis flaw.\n\nSigned-off-by: Kevin Bracey \n---\n git-merge-one-file.sh | 20 +++++++-------------\n 1 file changed, 7 insertions(+), 13 deletions(-)\n\ndiff --git a/git-merge-one-file.sh b/git-merge-one-file.sh\nindex 1236fbf..70f36f1 100755\n--- a/git-merge-one-file.sh\n+++ b/git-merge-one-file.sh\n@@ -104,11 +104,13 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \t\t;;\n \tesac\n \n+\tret=0\n \tsrc1=$(git-unpack-file $2)\n \tsrc2=$(git-unpack-file $3)\n \tcase \"$1\" in\n \t'')\n-\t\techo \"Added $4 in both, but differently.\"\n+\t\techo \"ERROR: Added $4 in both, but differently.\"\n+\t\tret=1\n \t\torig=$(git-unpack-file $2)\n \t\tcreate_virtual_base \"$orig\" \"$src1\" \"$src2\"\n \t\t;;\n@@ -121,10 +123,9 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \t# Be careful for funny filename such as \"-L\" in \"$4\", which\n \t# would confuse \"merge\" greatly.\n \tgit merge-file \"$src1\" \"$orig\" \"$src2\"\n-\tret=$?\n-\tmsg=\n-\tif [ $ret -ne 0 ]; then\n-\t\tmsg='content conflict'\n+\tif [ $? -ne 0 ]; then\n+\t\techo \"ERROR: Content conflict in $4\"\n+\t\tret=1\n \tfi\n \n \t# Create the working tree file, using \"our tree\" version from the\n@@ -133,18 +134,11 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \trm -f -- \"$orig\" \"$src1\" \"$src2\"\n \n \tif [ \"$6\" != \"$7\" ]; then\n-\t\tif [ -n \"$msg\" ]; then\n-\t\t\tmsg=\"$msg, \"\n-\t\tfi\n-\t\tmsg=\"${msg}permissions conflict: $5->$6,$7\"\n-\t\tret=1\n-\tfi\n-\tif [ \"$1\" = '' ]; then\n+\t\techo \"ERROR: Permissions conflict: $5->$6,$7\"\n \t\tret=1\n \tfi\n \n \tif [ $ret -ne 0 ]; then\n-\t\techo \"ERROR: $msg in $4\"\n \t\texit 1\n \tfi\n \texec git update-index -- \"$4\"\n-- \n1.8.2.rc3.7.g77aeedb\n"},{"id":"210947","messageId":"7v7glfetus.fsf@alter.siamese.dyndns.org","threadId":"33089","inReplyTo":"1362856860-15205-3-git-send-email-kevin@bracey.fi","subject":"Re: [PATCH v2 2/3] mergetools/p4merge: create a base if none available","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-10T04:55:55Z","receivedAt":"2013-03-10T04:55:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Bracey writes:\n\n> diff --git a/git-sh-setup.sh b/git-sh-setup.sh\n> index 795edd2..aa9a732 100644\n> --- a/git-sh-setup.sh\n> +++ b/git-sh-setup.sh\n> @@ -249,6 +249,19 @@ clear_local_git_env() {\n> \tunset $(git rev-parse --local-env-vars)\n> }\n> \n> +# Generate a virtual base file for a two-file merge. On entry the\n> +# base file $1 should be a copy of $2. Uses git apply to remove\n> +# lines from $1 that are not in $3, leaving only common lines.\n> +create_virtual_base() {\n> +\tsz0=$(wc -c <\"$1\")\n> +\t@@DIFF@@ -u -La/\"$1\" -Lb/\"$1\" \"$2\" \"$3\" | git apply --no-add\n> +\tsz1=$(wc -c <\"$1\")\n> +\n> +\t# If we do not have enough common material, it is not\n> +\t# worth trying two-file merge using common subsections.\n> +\texpr $sz0 \\< $sz1 \\* 2 >/dev/null || : >\"$1\"\n> +}\n> +\n\nThis rewrite is wrong. It should be\n\n> +\tsz0=$(wc -c <\"$1\")\n> +\t@@DIFF@@ -u -La/\"$1\" -Lb/\"$1\" \"$1\" \"$3\" | git apply --no-add\n> +\tsz1=$(wc -c <\"$1\")\n\nfor it to make sense. \"diff $1 $3\" is a change to go from $1 to $3;\nwith \"-La/$1 -Lb/$1\", we declare that the change is to be applied to\n$1, and use --no-add to only use the removal from the diff when we\nedit $1 using this mechanism.\n\nThe end effect is to in-place edit \"$1\" to remove what is not common\nwith \"$3\", and sz0/sz1 computation is done on \"$1\" for this reason.\nDoes it (i.e. \"$1\") shrink sufficiently when we remove the material\nthat is not common in it (i.e. \"$1\") and \"$3\"?\n\nThis part is a two-file operation between $1 and $3; there is\nnothing you would want to pass $2 to influence what the above three\nlines do.\n\nIt may happen that the caller has two copies of the same thing,\n$orig and $src1, and uses one for $1 and the other for $2, so you\nwon't observe the damage from the incorrect rewriting of the above\nlogic, but it invites the next caller to incorrectly feed something\ntotally unrelated to $1 and $2.\n\nPlease fix it to a function that takes two temporary paths, not\nthree.\n"},{"id":"211190","messageId":"1363137142-18606-1-git-send-email-kevin@bracey.fi","threadId":"33089","inReplyTo":"1362601978-16911-1-git-send-email-kevin@bracey.fi","subject":"[PATCH v3 1/3] mergetools/p4merge: swap LOCAL and REMOTE","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-13T01:12:20Z","receivedAt":"2013-03-13T01:12:20Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"Reverse LOCAL and REMOTE when invoking P4Merge as a mergetool, so that\nthe incoming branch is now in the left-hand, blue triangle pane, and the\ncurrent branch is in the right-hand, green circle pane.\n\nThis change makes use of P4Merge consistent with its built-in help, its\nreference documentation, and Perforce itself. But most importantly, it\nmakes merge results clearer. P4Merge is not totally symmetrical between\nleft and right; despite changing a few text labels from \"theirs/ours\" to\n\"left/right\" when invoked manually, it still retains its original\nPerforce \"theirs/ours\" viewpoint.\n\nMost obviously, in the result pane P4Merge shows changes that are common\nto both branches in green. This is on the basis of the current branch\nbeing green, as it is when invoked from Perforce; it means that lines in\nthe result are blue if and only if they are being changed by the merge,\nmaking the resulting diff clearer.\n\nNote that P4Merge now shows \"ours\" on the right for both diff and merge,\nunlike other diff/mergetools, which always have REMOTE on the right.\nBut observe that REMOTE is the working tree (ie \"ours\") for a diff,\nwhile it's another branch (ie \"theirs\") for a merge.\n\nOurs and theirs are reversed for a rebase - see \"git help rebase\".\nHowever, this does produce the desired \"show the results of this commit\"\neffect in P4Merge - changes that remain in the rebased commit (in your\nbranch, but not in the new base) appear in blue; changes that do not\nappear in the rebased commit (from the new base, or common to both) are\nin green. If Perforce had rebase, they'd probably not swap ours/theirs,\nbut make P4Merge show common changes in blue, picking out our changes in\ngreen. We can't do that, so this is next best.\n\nSigned-off-by: Kevin Bracey \n---\n mergetools/p4merge | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/mergetools/p4merge b/mergetools/p4merge\nindex 8a36916..46b3a5a 100644\n--- a/mergetools/p4merge\n+++ b/mergetools/p4merge\n@@ -22,7 +22,7 @@ diff_cmd () {\n merge_cmd () {\n \ttouch \"$BACKUP\"\n \t$base_present || >\"$BASE\"\n-\t\"$merge_tool_path\" \"$BASE\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n+\t\"$merge_tool_path\" \"$BASE\" \"$REMOTE\" \"$LOCAL\" \"$MERGED\"\n \tcheck_unchanged\n }\n \n-- \n1.8.2.rc3.7.g1100d09.dirty\n"},{"id":"211188","messageId":"1363137142-18606-2-git-send-email-kevin@bracey.fi","threadId":"33089","inReplyTo":"1363137142-18606-1-git-send-email-kevin@bracey.fi","subject":"[PATCH v3 2/3] mergetools/p4merge: create a base if none available","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-13T01:12:21Z","receivedAt":"2013-03-13T01:12:21Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"Originally, with no base, Git gave P4Merge $LOCAL as a dummy base:\n\n p4merge \"$LOCAL\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n\nCommit 0a0ec7bd changed this to:\n\n p4merge \"empty file\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n\nto avoid the problem of being unable to save in some circumstances with\nsimilar inputs.\n\nUnfortunately this approach produces much worse results on differing\ninputs. P4Merge really regards the blank file as the base, and once you\nhave just a couple of differences between the two branches you end up\nwith one a massive full-file conflict. The 3-way diff is not readable,\nand you have to invoke \"difftool MERGE_HEAD HEAD\" manually to get a\nuseful view.\n\nThe original approach appears to have invoked special 2-way merge\nbehaviour in P4Merge that occurs only if the base filename is \"\" or\nequal to the left input. You get a good visual comparison, and it does\nnot auto-resolve differences. (Normally if one branch matched the base,\nit would autoresolve to the other branch).\n\nBut there appears to be no way of getting this 2-way behaviour and being\nable to reliably save. Having base==left appears to be triggering other\nassumptions. There are tricks the user can use to force the save icon\non, but it's not intuitive.\n\nSo we now follow a suggestion given in the original patch's discussion:\ngenerate a virtual base, consisting of the lines common to the two\nbranches. This is the same as the technique used in resolve and octopus\nmerges, so we relocate that code to a shared function.\n\nNote that if there are no differences at the same location, this\ntechnique can lead to automatic resolution without conflict, combining\neverything from the 2 files. As with the other merges using this\ntechnique, we assume the user will inspect the result before saving.\n\nSigned-off-by: Kevin Bracey \n---\n Documentation/git-sh-setup.txt | 6 ++++++\n git-merge-one-file.sh | 18 +++++-------------\n git-sh-setup.sh | 12 ++++++++++++\n mergetools/p4merge | 6 +++++-\n 4 files changed, 28 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/git-sh-setup.txt b/Documentation/git-sh-setup.txt\nindex 6a9f66d..5d709d0 100644\n--- a/Documentation/git-sh-setup.txt\n+++ b/Documentation/git-sh-setup.txt\n@@ -82,6 +82,12 @@ get_author_ident_from_commit::\n \toutputs code for use with eval to set the GIT_AUTHOR_NAME,\n \tGIT_AUTHOR_EMAIL and GIT_AUTHOR_DATE variables for a given commit.\n \n+create_virtual_base::\n+\tmodifies the first file so only lines in common with the\n+\tsecond file remain. If there is insufficient common material,\n+\tthen the first file is left empty. The result is suitable\n+\tas a virtual base input for a 3-way merge.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/git-merge-one-file.sh b/git-merge-one-file.sh\nindex f612cb8..0f164e5 100755\n--- a/git-merge-one-file.sh\n+++ b/git-merge-one-file.sh\n@@ -104,30 +104,22 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \t\t;;\n \tesac\n \n-\tsrc2=`git-unpack-file $3`\n+\tsrc1=$(git-unpack-file $2)\n+\tsrc2=$(git-unpack-file $3)\n \tcase \"$1\" in\n \t'')\n \t\techo \"Added $4 in both, but differently.\"\n-\t\t# This extracts OUR file in $orig, and uses git apply to\n-\t\t# remove lines that are unique to ours.\n-\t\torig=`git-unpack-file $2`\n-\t\tsz0=`wc -c <\"$orig\"`\n-\t\t@@DIFF@@ -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add\n-\t\tsz1=`wc -c <\"$orig\"`\n-\n-\t\t# If we do not have enough common material, it is not\n-\t\t# worth trying two-file merge using common subsections.\n-\t\texpr $sz0 \\< $sz1 \\* 2 >/dev/null || : >$orig\n+\t\torig=$(git-unpack-file $2)\n+\t\tcreate_virtual_base \"$orig\" \"$src2\"\n \t\t;;\n \t*)\n \t\techo \"Auto-merging $4\"\n-\t\torig=`git-unpack-file $1`\n+\t\torig=$(git-unpack-file $1)\n \t\t;;\n \tesac\n \n \t# Be careful for funny filename such as \"-L\" in \"$4\", which\n \t# would confuse \"merge\" greatly.\n-\tsrc1=`git-unpack-file $2`\n \tgit merge-file \"$src1\" \"$orig\" \"$src2\"\n \tret=$?\n \tmsg=\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 795edd2..349a5d4 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -249,6 +249,18 @@ clear_local_git_env() {\n \tunset $(git rev-parse --local-env-vars)\n }\n \n+# Generate a virtual base file for a two-file merge. Uses git apply to\n+# remove lines from $1 that are not in $2, leaving only common lines.\n+create_virtual_base() {\n+\tsz0=$(wc -c <\"$1\")\n+\t@@DIFF@@ -u -La/\"$1\" -Lb/\"$1\" \"$1\" \"$2\" | git apply --no-add\n+\tsz1=$(wc -c <\"$1\")\n+\n+\t# If we do not have enough common material, it is not\n+\t# worth trying two-file merge using common subsections.\n+\texpr $sz0 \\< $sz1 \\* 2 >/dev/null || : >\"$1\"\n+}\n+\n \n # Platform specific tweaks to work around some commands\n case $(uname -s) in\ndiff --git a/mergetools/p4merge b/mergetools/p4merge\nindex 46b3a5a..5a608ab 100644\n--- a/mergetools/p4merge\n+++ b/mergetools/p4merge\n@@ -21,7 +21,11 @@ diff_cmd () {\n \n merge_cmd () {\n \ttouch \"$BACKUP\"\n-\t$base_present || >\"$BASE\"\n+\tif ! $base_present\n+\tthen\n+\t\tcp -- \"$LOCAL\" \"$BASE\"\n+\t\tcreate_virtual_base \"$BASE\" \"$REMOTE\"\n+\tfi\n \t\"$merge_tool_path\" \"$BASE\" \"$REMOTE\" \"$LOCAL\" \"$MERGED\"\n \tcheck_unchanged\n }\n-- \n1.8.2.rc3.7.g1100d09.dirty\n"},{"id":"211187","messageId":"1363137142-18606-3-git-send-email-kevin@bracey.fi","threadId":"33089","inReplyTo":"1363137142-18606-1-git-send-email-kevin@bracey.fi","subject":"[PATCH v3 3/3] git-merge-one-file: revise merge error reporting","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-13T01:12:22Z","receivedAt":"2013-03-13T01:12:22Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"Commit 718135e improved the merge error reporting for the resolve\nstrategy's merge conflict and permission conflict cases, but led to a\nmalformed \"ERROR: in myfile.c\" message in the case of a file added\ndifferently.\n\nThis commit reverts that change, and uses an alternative approach without\nthis flaw.\n\nSigned-off-by: Kevin Bracey \n---\n git-merge-one-file.sh | 20 +++++++-------------\n 1 file changed, 7 insertions(+), 13 deletions(-)\n\ndiff --git a/git-merge-one-file.sh b/git-merge-one-file.sh\nindex 0f164e5..78b07a8 100755\n--- a/git-merge-one-file.sh\n+++ b/git-merge-one-file.sh\n@@ -104,11 +104,13 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \t\t;;\n \tesac\n \n+\tret=0\n \tsrc1=$(git-unpack-file $2)\n \tsrc2=$(git-unpack-file $3)\n \tcase \"$1\" in\n \t'')\n-\t\techo \"Added $4 in both, but differently.\"\n+\t\techo \"ERROR: Added $4 in both, but differently.\"\n+\t\tret=1\n \t\torig=$(git-unpack-file $2)\n \t\tcreate_virtual_base \"$orig\" \"$src2\"\n \t\t;;\n@@ -121,10 +123,9 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \t# Be careful for funny filename such as \"-L\" in \"$4\", which\n \t# would confuse \"merge\" greatly.\n \tgit merge-file \"$src1\" \"$orig\" \"$src2\"\n-\tret=$?\n-\tmsg=\n-\tif [ $ret -ne 0 ]; then\n-\t\tmsg='content conflict'\n+\tif [ $? -ne 0 ]; then\n+\t\techo \"ERROR: Content conflict in $4\"\n+\t\tret=1\n \tfi\n \n \t# Create the working tree file, using \"our tree\" version from the\n@@ -133,18 +134,11 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \trm -f -- \"$orig\" \"$src1\" \"$src2\"\n \n \tif [ \"$6\" != \"$7\" ]; then\n-\t\tif [ -n \"$msg\" ]; then\n-\t\t\tmsg=\"$msg, \"\n-\t\tfi\n-\t\tmsg=\"${msg}permissions conflict: $5->$6,$7\"\n-\t\tret=1\n-\tfi\n-\tif [ \"$1\" = '' ]; then\n+\t\techo \"ERROR: Permissions conflict: $5->$6,$7\"\n \t\tret=1\n \tfi\n \n \tif [ $ret -ne 0 ]; then\n-\t\techo \"ERROR: $msg in $4\"\n \t\texit 1\n \tfi\n \texec git update-index -- \"$4\"\n-- \n1.8.2.rc3.7.g1100d09.dirty\n"},{"id":"211191","messageId":"CAJDDKr7NJsmB3R_kYtZeocSZAz-kfP9k6GssZ+AM-qfPCTzrdg@mail.gmail.com","threadId":"33089","inReplyTo":"1363137142-18606-1-git-send-email-kevin@bracey.fi","subject":"Re: [PATCH v3 1/3] mergetools/p4merge: swap LOCAL and REMOTE","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-03-13T02:05:19Z","receivedAt":"2013-03-13T02:05:19Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Mar 12, 2013 at 6:12 PM, Kevin Bracey wrote:\n> Reverse LOCAL and REMOTE when invoking P4Merge as a mergetool, so that\n> the incoming branch is now in the left-hand, blue triangle pane, and the\n> current branch is in the right-hand, green circle pane.\n>\n> This change makes use of P4Merge consistent with its built-in help, its\n> reference documentation, and Perforce itself. But most importantly, it\n> makes merge results clearer. P4Merge is not totally symmetrical between\n> left and right; despite changing a few text labels from \"theirs/ours\" to\n> \"left/right\" when invoked manually, it still retains its original\n> Perforce \"theirs/ours\" viewpoint.\n>\n> Most obviously, in the result pane P4Merge shows changes that are common\n> to both branches in green. This is on the basis of the current branch\n> being green, as it is when invoked from Perforce; it means that lines in\n> the result are blue if and only if they are being changed by the merge,\n> making the resulting diff clearer.\n>\n> Note that P4Merge now shows \"ours\" on the right for both diff and merge,\n> unlike other diff/mergetools, which always have REMOTE on the right.\n> But observe that REMOTE is the working tree (ie \"ours\") for a diff,\n> while it's another branch (ie \"theirs\") for a merge.\n>\n> Ours and theirs are reversed for a rebase - see \"git help rebase\".\n> However, this does produce the desired \"show the results of this commit\"\n> effect in P4Merge - changes that remain in the rebased commit (in your\n> branch, but not in the new base) appear in blue; changes that do not\n> appear in the rebased commit (from the new base, or common to both) are\n> in green. If Perforce had rebase, they'd probably not swap ours/theirs,\n> but make P4Merge show common changes in blue, picking out our changes in\n> green. We can't do that, so this is next best.\n>\n> Signed-off-by: Kevin Bracey \n> ---\n\nThis seems sensible to apply. The commit message is a bit long,\nbut I think it's justified since this is exactly the kind of thing\nI would tend to forget after enough time has passed.\n\nDitto on the create_virtual_base patch. Your latest patch\naddressed Junio's note about making it take 2 args.\n\nFWIW, please feel free to add:\n\nReviewed-by: David Aguilar \n\nThanks.\n\n> mergetools/p4merge | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/mergetools/p4merge b/mergetools/p4merge\n> index 8a36916..46b3a5a 100644\n> --- a/mergetools/p4merge\n> +++ b/mergetools/p4merge\n> @@ -22,7 +22,7 @@ diff_cmd () {\n> merge_cmd () {\n> touch \"$BACKUP\"\n> $base_present || >\"$BASE\"\n> - \"$merge_tool_path\" \"$BASE\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n> + \"$merge_tool_path\" \"$BASE\" \"$REMOTE\" \"$LOCAL\" \"$MERGED\"\n> check_unchanged\n> }\n>\n> --\n> 1.8.2.rc3.7.g1100d09.dirty\n>\n\n\n\n-- \nDavid\n"},{"id":"211202","messageId":"CAJDDKr4swZzzv3e+Huz72CVmisFKU8T74jFj3-uGmZHReRGVBw@mail.gmail.com","threadId":"33089","inReplyTo":"1363137142-18606-3-git-send-email-kevin@bracey.fi","subject":"Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-03-13T09:03:47Z","receivedAt":"2013-03-13T09:03:47Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Mar 12, 2013 at 6:12 PM, Kevin Bracey wrote:\n> Commit 718135e improved the merge error reporting for the resolve\n> strategy's merge conflict and permission conflict cases, but led to a\n> malformed \"ERROR: in myfile.c\" message in the case of a file added\n> differently.\n>\n> This commit reverts that change, and uses an alternative approach without\n> this flaw.\n>\n> Signed-off-by: Kevin Bracey \n> ---\n\nI wonder whether before these changes we should\nupdate the style in this file to follow Documentation/CodingGuidelines.\n\nNot in this patch, but in the file right now there's\nthis part that stands out:\n\n\tif [ \"$2\" ]; then\n\t\techo \"Removing $4\"\n\nI think that expression would read more clearly as:\n\n\tif test -n \"$2\"\n\tthen\n\t\techo \"Removing $4\"\n\nDitto `if [ \"$1\" = '' ]` is better written as `test -z \"$1\"`.\n\nCan you please send a patch to true these up?\n\nIt'd be especially nice if the style patch could come\nfirst, followed by the fixes/features ;-)\n\n\n> git-merge-one-file.sh | 20 +++++++-------------\n> 1 file changed, 7 insertions(+), 13 deletions(-)\n>\n> diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh\n> index 0f164e5..78b07a8 100755\n> --- a/git-merge-one-file.sh\n> +++ b/git-merge-one-file.sh\n> @@ -104,11 +104,13 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n> ;;\n> esac\n>\n> + ret=0\n> src1=$(git-unpack-file $2)\n> src2=$(git-unpack-file $3)\n> case \"$1\" in\n> '')\n> - echo \"Added $4 in both, but differently.\"\n> + echo \"ERROR: Added $4 in both, but differently.\"\n> + ret=1\n> orig=$(git-unpack-file $2)\n> create_virtual_base \"$orig\" \"$src2\"\n> ;;\n> @@ -121,10 +123,9 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n> # Be careful for funny filename such as \"-L\" in \"$4\", which\n> # would confuse \"merge\" greatly.\n> git merge-file \"$src1\" \"$orig\" \"$src2\"\n> - ret=$?\n> - msg=\n> - if [ $ret -ne 0 ]; then\n> - msg='content conflict'\n> + if [ $? -ne 0 ]; then\n> + echo \"ERROR: Content conflict in $4\"\n> + ret=1\n\nif test $? != 0\nthen\n\nAlso.. should this error not go to stderr?\nI guess the existing script was not doing that,\nbut it seems like anything that says \"ERROR\" should go there.\n\n> fi\n>\n> # Create the working tree file, using \"our tree\" version from the\n> @@ -133,18 +134,11 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n> rm -f -- \"$orig\" \"$src1\" \"$src2\"\n>\n> if [ \"$6\" != \"$7\" ]; then\n> - if [ -n \"$msg\" ]; then\n> - msg=\"$msg, \"\n> - fi\n> - msg=\"${msg}permissions conflict: $5->$6,$7\"\n> - ret=1\n> - fi\n> - if [ \"$1\" = '' ]; then\n> + echo \"ERROR: Permissions conflict: $5->$6,$7\"\n> ret=1\n> fi\n>\n> if [ $ret -ne 0 ]; then\n> - echo \"ERROR: $msg in $4\"\n> exit 1\n> fi\n> exec git update-index -- \"$4\"\n\nsame notes as above. I think a style patch should come first.\n-- \nDavid\n"},{"id":"211239","messageId":"7vehfj2neh.fsf@alter.siamese.dyndns.org","threadId":"33089","inReplyTo":"1363137142-18606-3-git-send-email-kevin@bracey.fi","subject":"Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-13T17:57:26Z","receivedAt":"2013-03-13T17:57:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Bracey writes:\n\n> Commit 718135e improved the merge error reporting for the resolve\n> strategy's merge conflict and permission conflict cases, but led to a\n> malformed \"ERROR: in myfile.c\" message in the case of a file added\n> differently.\n>\n> This commit reverts that change, and uses an alternative approach without\n> this flaw.\n>\n> Signed-off-by: Kevin Bracey \n> ---\n> git-merge-one-file.sh | 20 +++++++-------------\n> 1 file changed, 7 insertions(+), 13 deletions(-)\n>\n> diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh\n> index 0f164e5..78b07a8 100755\n> --- a/git-merge-one-file.sh\n> +++ b/git-merge-one-file.sh\n> @@ -104,11 +104,13 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n> \t\t;;\n> \tesac\n> \n> +\tret=0\n> \tsrc1=$(git-unpack-file $2)\n> \tsrc2=$(git-unpack-file $3)\n> \tcase \"$1\" in\n> \t'')\n> -\t\techo \"Added $4 in both, but differently.\"\n> +\t\techo \"ERROR: Added $4 in both, but differently.\"\n> +\t\tret=1\n\nThe problem you identified may be worth fixing, but I do not think\nthis change is correct.\n\nThis message is at the same severity level as the message on the\nother arm of this case that says \"Auto-merging $4\". In that other\ncase arm, we are attempting a true three-way merge, and in this case\narm, we are attempting a similar three-way merge using your \"virtual\nbase\".\n\nNeither has found any error in this case arm yet. The messages are\nboth \"informational\", not an error. I do not think you would want\nto set ret=1 until you see content conflict.\n"},{"id":"211274","messageId":"51416DD5.2030805@bracey.fi","threadId":"33089","inReplyTo":"7vehfj2neh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-14T06:27:33Z","receivedAt":"2013-03-14T06:27:33Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"On 13/03/2013 19:57, Junio C Hamano wrote:\n> Kevin Bracey writes:\n>\n>> -\t\techo \"Added $4 in both, but differently.\"\n>> +\t\techo \"ERROR: Added $4 in both, but differently.\"\n>> +\t\tret=1\n> The problem you identified may be worth fixing, but I do not think\n> this change is correct.\n>\n> This message is at the same severity level as the message on the\n> other arm of this case that says \"Auto-merging $4\". In that other\n> case arm, we are attempting a true three-way merge, and in this case\n> arm, we are attempting a similar three-way merge using your \"virtual\n> base\".\n>\n> Neither has found any error in this case arm yet. The messages are\n> both \"informational\", not an error. I do not think you would want\n> to set ret=1 until you see content conflict.\n\nI disagree here. At the minute, it does set ret to 1 (but further down \nthe code - bringing it up here next to the \"ERROR\" print clarifies \nthat), and will report the merge as failed, conflict in the 3-way merge \nor not. Which I think is correct.\n\nWe have to stop for user inspection here. We do have a fake base; we \ncan't trust the 3-way merge with it.\n\nThe virtual 3-way merge will take ABCDE and ABDE and produce ABCDE \nwithout conflict. That's flat wrong if the real base they failed to tell \nGit about was ABCDE.\n\nDespite being useful, I'm still slightly uncomfortable that it can \nproduce something without any conflict markers. The user really needs to \nlook at properly.\n\n(And one interesting related glitch, or at least thing that puzzled me \nwhen it happened. This is from memory, so may be slightly mistaken, but \nwhat seemed to happen was that if you have rerere enabled, then \nmergetool tends to say \"nothing to merge\", because it relies on \"rerere \nremaining\", which relies on conflict markers. I think you could still \nforce a mergetool up by specifying the specific file though.)\n\nMaybe the virtual base itself should be different. Maybe it should put a \n??????? marker in place of every unique line. So you get:\n\nLeft ABCEFGH\nRight XABCDEFJH -> Merge result <|X>ABC<|D>EFH\nVBase ?ABC?EF??H\n\nThat actually feels like it may be the correct answer here. And it's \neffectively what P4Merge does in its \"2-way\" mode I failed to invoke. \n(At least for the result view).\n\nKevin\n"},{"id":"211299","messageId":"7vr4jiyqrj.fsf@alter.siamese.dyndns.org","threadId":"33089","inReplyTo":"51416DD5.2030805@bracey.fi","subject":"Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-14T14:56:00Z","receivedAt":"2013-03-14T14:56:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Bracey writes:\n\n> I disagree here. At the minute, it does set ret to 1 (but further down\n> the code - bringing it up here next to the \"ERROR\" print clarifies\n> that), and will report the merge as failed, conflict in the 3-way\n> merge or not. Which I think is correct.\n\nOK. I agree that forcing users to always inspect the result of\n\"both side added\" resolution sounds like a good safety measure.\n\n> Maybe the virtual base itself should be different. Maybe it should put\n> a ??????? marker in place of every unique line. So you get:\n>\n> Left ABCEFGH\n> Right XABCDEFJH -> Merge result <|X>ABC<|D>EFH\n> VBase ?ABC?EF??H\n>\n> That actually feels like it may be the correct answer here.\n\nInteresting, though the approach has downsides with the diff3\nconflict style, no?\n"},{"id":"211314","messageId":"5142097B.1080105@bracey.fi","threadId":"33089","inReplyTo":"7vr4jiyqrj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-14T17:31:39Z","receivedAt":"2013-03-14T17:31:39Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"On 14/03/2013 16:56, Junio C Hamano wrote:\n> Kevin Bracey writes:\n>\n>> Maybe the virtual base itself should be different. Maybe it should put\n>> a ??????? marker in place of every unique line. So you get:\n>>\n>> Left ABCEFGH\n>> Right XABCDEFJH -> Merge result <|X>ABC<|D>EFH\n>> VBase ?ABC?EF??H\n>>\n>> That actually feels like it may be the correct answer here.\n> Interesting, though the approach has downsides with the diff3\n> conflict style, no?\n>\nWell, yes, but I would assume that we would forcibly select normal diff \nhere somehow, if we aren't already. We should be - turning ABCDEFGH vs \nABCD into ABCD is silly.\n\nThis topic has a lot in common with the zdiff3 discussion going on. The \nconcern there is about large chunks of similar code appearing on two \nsides, and not being in the base, leading to useless diff3.\n\nThis is just the special case of the base being totally empty.\n\nThe thought on zdiff3 philosophy was that common additions should be \ntreated as resolved, and not appear inside conflict markers. That's \nexactly what we'd be doing. So, same conflict as above, but this time \nembedded in a larger file, using zdiff3 logic:\n\nLeft aaaaaabaacaaABCEFGHeee\nBase aaaaaaaaaaaaeee -> zdiff3 \naaadaaacaaABC<|D>EFHeee\nRight aaadaafaaaaaABCDEFJHeee\n\nNote that I've chosen to suppress the = marker if the lines surrounding \nthe conflict are not in the base. I think that helps highlight the fact \nthat we're in a diff2 section. EFH reads like an assertion that \nthe base has EFH. Whereas EFH avoids that.\n\nSo, anyway, commonality with zdiff3 would be good. Even if we can't \nshare code, we should at least share the general style of result.\n\nKevin\n"},{"id":"211316","messageId":"51420B50.2030600@bracey.fi","threadId":"33089","inReplyTo":"5142097B.1080105@bracey.fi","subject":"Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-14T17:39:28Z","receivedAt":"2013-03-14T17:39:28Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"On 14/03/2013 19:31, Kevin Bracey wrote:\n> On 14/03/2013 16:56, Junio C Hamano wrote:\n>>\n> Well, yes, but I would assume that we would forcibly select normal \n> diff here somehow, if we aren't already. We should be - turning \n> ABCDEFGH vs ABCD into ABCD is silly.\n\nDoh. But anyway, we don't want to waste space with |= markers, and make \nthe same \"surrounding code is in the base\" suggestion. So we should be \nselecting diff.\n\nKevin\n"},{"id":"212087","messageId":"1364126098-10788-1-git-send-email-kevin@bracey.fi","threadId":"33089","inReplyTo":"1363137142-18606-1-git-send-email-kevin@bracey.fi","subject":"[PATCH v4 1/2] mergetools/p4merge: swap LOCAL and REMOTE","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-24T11:54:57Z","receivedAt":"2013-03-24T11:54:57Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"Reverse LOCAL and REMOTE when invoking P4Merge as a mergetool, so that\nthe incoming branch is now in the left-hand, blue triangle pane, and the\ncurrent branch is in the right-hand, green circle pane.\n\nThis change makes use of P4Merge consistent with its built-in help, its\nreference documentation, and Perforce itself. But most importantly, it\nmakes merge results clearer. P4Merge is not totally symmetrical between\nleft and right; despite changing a few text labels from \"theirs/ours\" to\n\"left/right\" when invoked manually, it still retains its original\nPerforce \"theirs/ours\" viewpoint.\n\nMost obviously, in the result pane P4Merge shows changes that are common\nto both branches in green. This is on the basis of the current branch\nbeing green, as it is when invoked from Perforce; it means that lines in\nthe result are blue if and only if they are being changed by the merge,\nmaking the resulting diff clearer.\n\nNote that P4Merge now shows \"ours\" on the right for both diff and merge,\nunlike other diff/mergetools, which always have REMOTE on the right.\nBut observe that REMOTE is the working tree (ie \"ours\") for a diff,\nwhile it's another branch (ie \"theirs\") for a merge.\n\nOurs and theirs are reversed for a rebase - see \"git help rebase\".\nHowever, this does produce the desired \"show the results of this commit\"\neffect in P4Merge - changes that remain in the rebased commit (in your\nbranch, but not in the new base) appear in blue; changes that do not\nappear in the rebased commit (from the new base, or common to both) are\nin green. If Perforce had rebase, they'd probably not swap ours/theirs,\nbut make P4Merge show common changes in blue, picking out our changes in\ngreen. We can't do that, so this is next best.\n\nSigned-off-by: Kevin Bracey \nReviewed-by: David Aguilar \n---\nNo change to part 1/2 from previous version, apart from Reviewed-by.\nPart 2/2 is modified.\n\n mergetools/p4merge | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/mergetools/p4merge b/mergetools/p4merge\nindex 8a36916..46b3a5a 100644\n--- a/mergetools/p4merge\n+++ b/mergetools/p4merge\n@@ -22,7 +22,7 @@ diff_cmd () {\n merge_cmd () {\n \ttouch \"$BACKUP\"\n \t$base_present || >\"$BASE\"\n-\t\"$merge_tool_path\" \"$BASE\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n+\t\"$merge_tool_path\" \"$BASE\" \"$REMOTE\" \"$LOCAL\" \"$MERGED\"\n \tcheck_unchanged\n }\n \n-- \n1.8.2.rc3.21.g744ac65\n"},{"id":"212095","messageId":"1364126098-10788-2-git-send-email-kevin@bracey.fi","threadId":"33089","inReplyTo":"1364126098-10788-1-git-send-email-kevin@bracey.fi","subject":"[PATCH v4 2/2] mergetools/p4merge: create a base if none available","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-24T11:54:58Z","receivedAt":"2013-03-24T11:54:58Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"Originally, with no base, Git gave P4Merge $LOCAL as a dummy base:\n\n p4merge \"$LOCAL\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n\nCommit 0a0ec7bd changed this to:\n\n p4merge \"empty file\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n\nto avoid the problem of being unable to save in some circumstances with\nsimilar inputs.\n\nUnfortunately this approach produces much worse results on differing\ninputs. P4Merge really regards the blank file as the base, and once you\nhave just a couple of differences between the two branches you end up\nwith one a massive full-file conflict. The 3-way diff is not readable,\nand you have to invoke \"difftool MERGE_HEAD HEAD\" manually to get a\nuseful view.\n\nThe original approach appears to have invoked special 2-way merge\nbehaviour in P4Merge that occurs only if the base filename is \"\" or\nequal to the left input. You get a good visual comparison, and it does\nnot auto-resolve differences. (Normally if one branch matched the base,\nit would autoresolve to the other branch).\n\nBut there appears to be no way of getting this 2-way behaviour and being\nable to reliably save. Having base==left appears to be triggering other\nassumptions. There are tricks the user can use to force the save icon\non, but it's not intuitive.\n\nSo we now follow a suggestion given in the original patch's discussion:\ngenerate a virtual base, consisting of the lines common to the two\nbranches. This is the same as the technique used in resolve and octopus\nmerges, so we relocate that code to a shared function.\n\nNote that if there are no differences at the same location, this\ntechnique can lead to automatic resolution without conflict, combining\neverything from the 2 files. As with the other merges using this\ntechnique, we assume the user will inspect the result before saving.\n\nSigned-off-by: Kevin Bracey \nReviewed-by: David Aguilar \n---\nMinor change from v3: that version moved initialisation of src1 higher up,\ndetaching it from its associated comment. This move was only required by\nearlier versions, so v4 leaves src1 in its original position.\n\nAdded Reviewed-by footer.\n\n Documentation/git-sh-setup.txt | 6 ++++++\n git-merge-one-file.sh | 18 +++++-------------\n git-sh-setup.sh | 12 ++++++++++++\n mergetools/p4merge | 6 +++++-\n 4 files changed, 28 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/git-sh-setup.txt b/Documentation/git-sh-setup.txt\nindex 6a9f66d..5d709d0 100644\n--- a/Documentation/git-sh-setup.txt\n+++ b/Documentation/git-sh-setup.txt\n@@ -82,6 +82,12 @@ get_author_ident_from_commit::\n \toutputs code for use with eval to set the GIT_AUTHOR_NAME,\n \tGIT_AUTHOR_EMAIL and GIT_AUTHOR_DATE variables for a given commit.\n \n+create_virtual_base::\n+\tmodifies the first file so only lines in common with the\n+\tsecond file remain. If there is insufficient common material,\n+\tthen the first file is left empty. The result is suitable\n+\tas a virtual base input for a 3-way merge.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/git-merge-one-file.sh b/git-merge-one-file.sh\nindex 3373c04..255c07a 100755\n--- a/git-merge-one-file.sh\n+++ b/git-merge-one-file.sh\n@@ -104,30 +104,22 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \t\t;;\n \tesac\n \n-\tsrc2=`git-unpack-file $3`\n+\tsrc2=$(git-unpack-file $3)\n \tcase \"$1\" in\n \t'')\n \t\techo \"Added $4 in both, but differently.\"\n-\t\t# This extracts OUR file in $orig, and uses git apply to\n-\t\t# remove lines that are unique to ours.\n-\t\torig=`git-unpack-file $2`\n-\t\tsz0=`wc -c <\"$orig\"`\n-\t\t@@DIFF@@ -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add\n-\t\tsz1=`wc -c <\"$orig\"`\n-\n-\t\t# If we do not have enough common material, it is not\n-\t\t# worth trying two-file merge using common subsections.\n-\t\texpr $sz0 \\< $sz1 \\* 2 >/dev/null || : >$orig\n+\t\torig=$(git-unpack-file $2)\n+\t\tcreate_virtual_base \"$orig\" \"$src2\"\n \t\t;;\n \t*)\n \t\techo \"Auto-merging $4\"\n-\t\torig=`git-unpack-file $1`\n+\t\torig=$(git-unpack-file $1)\n \t\t;;\n \tesac\n \n \t# Be careful for funny filename such as \"-L\" in \"$4\", which\n \t# would confuse \"merge\" greatly.\n-\tsrc1=`git-unpack-file $2`\n+\tsrc1=$(git-unpack-file $2)\n \tgit merge-file \"$src1\" \"$orig\" \"$src2\"\n \tret=$?\n \tmsg=\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 9cfbe7f..2f78359 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -249,6 +249,18 @@ clear_local_git_env() {\n \tunset $(git rev-parse --local-env-vars)\n }\n \n+# Generate a virtual base file for a two-file merge. Uses git apply to\n+# remove lines from $1 that are not in $2, leaving only common lines.\n+create_virtual_base() {\n+\tsz0=$(wc -c <\"$1\")\n+\t@@DIFF@@ -u -La/\"$1\" -Lb/\"$1\" \"$1\" \"$2\" | git apply --no-add\n+\tsz1=$(wc -c <\"$1\")\n+\n+\t# If we do not have enough common material, it is not\n+\t# worth trying two-file merge using common subsections.\n+\texpr $sz0 \\< $sz1 \\* 2 >/dev/null || : >\"$1\"\n+}\n+\n \n # Platform specific tweaks to work around some commands\n case $(uname -s) in\ndiff --git a/mergetools/p4merge b/mergetools/p4merge\nindex 46b3a5a..5a608ab 100644\n--- a/mergetools/p4merge\n+++ b/mergetools/p4merge\n@@ -21,7 +21,11 @@ diff_cmd () {\n \n merge_cmd () {\n \ttouch \"$BACKUP\"\n-\t$base_present || >\"$BASE\"\n+\tif ! $base_present\n+\tthen\n+\t\tcp -- \"$LOCAL\" \"$BASE\"\n+\t\tcreate_virtual_base \"$BASE\" \"$REMOTE\"\n+\tfi\n \t\"$merge_tool_path\" \"$BASE\" \"$REMOTE\" \"$LOCAL\" \"$MERGED\"\n \tcheck_unchanged\n }\n-- \n1.8.2.rc3.21.g744ac65\n"},{"id":"212086","messageId":"1364127985-13366-1-git-send-email-kevin@bracey.fi","threadId":"33089","inReplyTo":"CAJDDKr4swZzzv3e+Huz72CVmisFKU8T74jFj3-uGmZHReRGVBw@mail.gmail.com","subject":"[PATCH v2 0/3] git-merge-one-file error reporting","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-24T12:26:22Z","receivedAt":"2013-03-24T12:26:22Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"Style clean up, as requested, followed by the fix to the \"both sides added\"\nhandling for git-merge-one-file.\n\nThis is based on v4 of my p4merge series, as they touch the same area.\n\nKevin Bracey (3):\n git-merge-one-file: style cleanup\n git-merge-one-file: send \"ERROR:\" messages to stderr\n git-merge-one-file: revise merge error reporting\n\n git-merge-one-file.sh | 50 +++++++++++++++++++++++++-------------------------\n 1 file changed, 25 insertions(+), 25 deletions(-)\n\n-- \n1.8.2.rc3.21.g744ac65\n"},{"id":"212088","messageId":"1364127985-13366-2-git-send-email-kevin@bracey.fi","threadId":"33089","inReplyTo":"1364127985-13366-1-git-send-email-kevin@bracey.fi","subject":"[PATCH v2 1/3] git-merge-one-file: style cleanup","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-24T12:26:23Z","receivedAt":"2013-03-24T12:26:23Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"Update style to match Documentation/CodingGuidelines.\n\nSigned-off-by: Kevin Bracey \n---\n git-merge-one-file.sh | 26 +++++++++++++++++---------\n 1 file changed, 17 insertions(+), 9 deletions(-)\n\ndiff --git a/git-merge-one-file.sh b/git-merge-one-file.sh\nindex 255c07a..2382b1f 100755\n--- a/git-merge-one-file.sh\n+++ b/git-merge-one-file.sh\n@@ -27,7 +27,7 @@ SUBDIRECTORY_OK=Yes\n cd_to_toplevel\n require_work_tree\n \n-if ! test \"$#\" -eq 7\n+if test $# != 7\n then\n \techo \"$LONG_USAGE\"\n \texit 1\n@@ -38,7 +38,8 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n # Deleted in both or deleted in one and unchanged in the other\n #\n \"$1..\" | \"$1.$1\" | \"$1$1.\")\n-\tif [ \"$2\" ]; then\n+\tif test -n \"$2\"\n+\tthen\n \t\techo \"Removing $4\"\n \telse\n \t\t# read-tree checked that index matches HEAD already,\n@@ -48,7 +49,8 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \t\t# we do not have it in the index, though.\n \t\texec git update-index --remove -- \"$4\"\n \tfi\n-\tif test -f \"$4\"; then\n+\tif test -f \"$4\"\n+\tthen\n \t\trm -f -- \"$4\" &&\n \t\trmdir -p \"$(expr \"z$4\" : 'z\\(.*\\)/')\" 2>/dev/null || :\n \tfi &&\n@@ -78,7 +80,8 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n # Added in both, identically (check for same permissions).\n #\n \".$3$2\")\n-\tif [ \"$6\" != \"$7\" ]; then\n+\tif test \"$6\" != \"$7\"\n+\tthen\n \t\techo \"ERROR: File $4 added identically in both branches,\"\n \t\techo \"ERROR: but permissions conflict $6->$7.\"\n \t\texit 1\n@@ -123,7 +126,8 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \tgit merge-file \"$src1\" \"$orig\" \"$src2\"\n \tret=$?\n \tmsg=\n-\tif [ $ret -ne 0 ]; then\n+\tif test $ret != 0\n+\tthen\n \t\tmsg='content conflict'\n \tfi\n \n@@ -132,18 +136,22 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \tgit checkout-index -f --stage=2 -- \"$4\" && cat \"$src1\" >\"$4\" || exit 1\n \trm -f -- \"$orig\" \"$src1\" \"$src2\"\n \n-\tif [ \"$6\" != \"$7\" ]; then\n-\t\tif [ -n \"$msg\" ]; then\n+\tif test \"$6\" != \"$7\"\n+\tthen\n+\t\tif test -n \"$msg\"\n+\t\tthen\n \t\t\tmsg=\"$msg, \"\n \t\tfi\n \t\tmsg=\"${msg}permissions conflict: $5->$6,$7\"\n \t\tret=1\n \tfi\n-\tif [ \"$1\" = '' ]; then\n+\tif test -z \"$1\"\n+\tthen\n \t\tret=1\n \tfi\n \n-\tif [ $ret -ne 0 ]; then\n+\tif test $ret != 0\n+\tthen\n \t\techo \"ERROR: $msg in $4\"\n \t\texit 1\n \tfi\n-- \n1.8.2.rc3.21.g744ac65\n"},{"id":"212089","messageId":"1364127985-13366-3-git-send-email-kevin@bracey.fi","threadId":"33089","inReplyTo":"1364127985-13366-1-git-send-email-kevin@bracey.fi","subject":"[PATCH v2 2/3] git-merge-one-file: send \"ERROR:\" messages to stderr","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-24T12:26:24Z","receivedAt":"2013-03-24T12:26:24Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"Signed-off-by: Kevin Bracey \n---\n git-merge-one-file.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/git-merge-one-file.sh b/git-merge-one-file.sh\nindex 2382b1f..39b7799 100755\n--- a/git-merge-one-file.sh\n+++ b/git-merge-one-file.sh\n@@ -69,7 +69,7 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \techo \"Adding $4\"\n \tif test -f \"$4\"\n \tthen\n-\t\techo \"ERROR: untracked $4 is overwritten by the merge.\"\n+\t\techo \"ERROR: untracked $4 is overwritten by the merge.\" >&2\n \t\texit 1\n \tfi\n \tgit update-index --add --cacheinfo \"$7\" \"$3\" \"$4\" &&\n@@ -82,8 +82,8 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \".$3$2\")\n \tif test \"$6\" != \"$7\"\n \tthen\n-\t\techo \"ERROR: File $4 added identically in both branches,\"\n-\t\techo \"ERROR: but permissions conflict $6->$7.\"\n+\t\techo \"ERROR: File $4 added identically in both branches,\" >&2\n+\t\techo \"ERROR: but permissions conflict $6->$7.\" >&2\n \t\texit 1\n \tfi\n \techo \"Adding $4\"\n@@ -98,11 +98,11 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \n \tcase \",$6,$7,\" in\n \t*,120000,*)\n-\t\techo \"ERROR: $4: Not merging symbolic link changes.\"\n+\t\techo \"ERROR: $4: Not merging symbolic link changes.\" >&2\n \t\texit 1\n \t\t;;\n \t*,160000,*)\n-\t\techo \"ERROR: $4: Not merging conflicting submodule changes.\"\n+\t\techo \"ERROR: $4: Not merging conflicting submodule changes.\" >&2\n \t\texit 1\n \t\t;;\n \tesac\n@@ -152,14 +152,14 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \n \tif test $ret != 0\n \tthen\n-\t\techo \"ERROR: $msg in $4\"\n+\t\techo \"ERROR: $msg in $4\" >&2\n \t\texit 1\n \tfi\n \texec git update-index -- \"$4\"\n \t;;\n \n *)\n-\techo \"ERROR: $4: Not handling case $1 -> $2 -> $3\"\n+\techo \"ERROR: $4: Not handling case $1 -> $2 -> $3\" >&2\n \t;;\n esac\n exit 1\n-- \n1.8.2.rc3.21.g744ac65\n"},{"id":"212091","messageId":"1364127985-13366-4-git-send-email-kevin@bracey.fi","threadId":"33089","inReplyTo":"1364127985-13366-1-git-send-email-kevin@bracey.fi","subject":"[PATCH v2 3/3] git-merge-one-file: revise merge error reporting","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-03-24T12:26:25Z","receivedAt":"2013-03-24T12:26:25Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"Commit 718135e improved the merge error reporting for the resolve\nstrategy's merge conflict and permission conflict cases, but led to a\nmalformed \"ERROR: in myfile.c\" message in the case of a file added\ndifferently.\n\nThis commit reverts that change, and uses an alternative approach without\nthis flaw.\n\nSigned-off-by: Kevin Bracey \n---\n git-merge-one-file.sh | 22 +++++++---------------\n 1 file changed, 7 insertions(+), 15 deletions(-)\n\ndiff --git a/git-merge-one-file.sh b/git-merge-one-file.sh\nindex 39b7799..e231d20 100755\n--- a/git-merge-one-file.sh\n+++ b/git-merge-one-file.sh\n@@ -107,10 +107,12 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \t\t;;\n \tesac\n \n+\tret=0\n \tsrc2=$(git-unpack-file $3)\n \tcase \"$1\" in\n \t'')\n-\t\techo \"Added $4 in both, but differently.\"\n+\t\techo \"ERROR: Added $4 in both, but differently.\" >&2\n+\t\tret=1\n \t\torig=$(git-unpack-file $2)\n \t\tcreate_virtual_base \"$orig\" \"$src2\"\n \t\t;;\n@@ -124,11 +126,10 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \t# would confuse \"merge\" greatly.\n \tsrc1=$(git-unpack-file $2)\n \tgit merge-file \"$src1\" \"$orig\" \"$src2\"\n-\tret=$?\n-\tmsg=\n-\tif test $ret != 0\n+\tif test $? != 0\n \tthen\n-\t\tmsg='content conflict'\n+\t\techo \"ERROR: Content conflict in $4\" >&2\n+\t\tret=1\n \tfi\n \n \t# Create the working tree file, using \"our tree\" version from the\n@@ -138,21 +139,12 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \n \tif test \"$6\" != \"$7\"\n \tthen\n-\t\tif test -n \"$msg\"\n-\t\tthen\n-\t\t\tmsg=\"$msg, \"\n-\t\tfi\n-\t\tmsg=\"${msg}permissions conflict: $5->$6,$7\"\n-\t\tret=1\n-\tfi\n-\tif test -z \"$1\"\n-\tthen\n+\t\techo \"ERROR: Permissions conflict: $5->$6,$7\" >&2\n \t\tret=1\n \tfi\n \n \tif test $ret != 0\n \tthen\n-\t\techo \"ERROR: $msg in $4\" >&2\n \t\texit 1\n \tfi\n \texec git update-index -- \"$4\"\n-- \n1.8.2.rc3.21.g744ac65\n"},{"id":"212179","messageId":"7vk3ovz9zb.fsf@alter.siamese.dyndns.org","threadId":"33089","inReplyTo":"1364127985-13366-4-git-send-email-kevin@bracey.fi","subject":"Re: [PATCH v2 3/3] git-merge-one-file: revise merge error reporting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-25T17:04:56Z","receivedAt":"2013-03-25T17:04:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Bracey writes:\n\n> Commit 718135e improved the merge error reporting for the resolve\n> strategy's merge conflict and permission conflict cases, but led to a\n> malformed \"ERROR: in myfile.c\" message in the case of a file added\n> differently.\n>\n> This commit reverts that change, and uses an alternative approach without\n> this flaw.\n>\n> Signed-off-by: Kevin Bracey \n\nWe used to treat \"Both added differently\" as a separate \"info\"\nmessage, just like the \"Auto-merging\" message, and let \"content\nconflict\" that is an \"error\" to happen naturally by doing such a\nmerge, possibly followed by permission conflict which is another\nkind of \"error\". We coalesced these two into a single message.\n\nAnd this patch breaks them into separate messages. I am not sure if\nthat aspect of the change is desirable.\n\nThe source of \"malformed\" message seems suspicious. Isn't the root\ncause of $msg being empty that merge-file can (sometimes) cleanly\nmerge two files using the phoney base in the \"both added\ndifferently\" codepath?\n\nIf you resolve that issue by forcing a \"conflicted\" failure when we\nhandle \"add/add\" conflict, I think the behaviour of the remainder of\nthe code is better in the original than the updated one.\n\nPerhaps something like this (I am applying these on 'maint')?\n\n git-merge-one-file.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/git-merge-one-file.sh b/git-merge-one-file.sh\nindex 25d7714..aa06282 100755\n--- a/git-merge-one-file.sh\n+++ b/git-merge-one-file.sh\n@@ -107,6 +107,7 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \t\t;;\n \tesac\n \n+\tadd_add_conflict=\n \tsrc2=`git-unpack-file $3`\n \tcase \"$1\" in\n \t'')\n@@ -121,6 +122,7 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \t\t# If we do not have enough common material, it is not\n \t\t# worth trying two-file merge using common subsections.\n \t\texpr $sz0 \\< $sz1 \\* 2 >/dev/null || : >$orig\n+\t\tadd_add_conflict=yes\n \t\t;;\n \t*)\n \t\techo \"Auto-merging $4\"\n@@ -128,15 +130,13 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \t\t;;\n \tesac\n \n-\t# Be careful for funny filename such as \"-L\" in \"$4\", which\n-\t# would confuse \"merge\" greatly.\n \tsrc1=`git-unpack-file $2`\n-\tgit merge-file \"$src1\" \"$orig\" \"$src2\"\n-\tret=$?\n-\tmsg=\n-\tif test $ret != 0\n+\n+\tret=0 msg=\n+\tif git merge-file \"$src1\" \"$orig\" \"$src2\" || test -n \"$add_add_conflict\"\n \tthen\n \t\tmsg='content conflict'\n+\t\tret=1\n \tfi\n \n \t# Create the working tree file, using \"our tree\" version from the\n"},{"id":"212180","messageId":"7vfvzjz9ej.fsf@alter.siamese.dyndns.org","threadId":"33089","inReplyTo":"7vk3ovz9zb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/3] git-merge-one-file: revise merge error reporting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-25T17:17:24Z","receivedAt":"2013-03-25T17:17:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Kevin Bracey writes:\n>\n>> Commit 718135e improved the merge error reporting for the resolve\n>> strategy's merge conflict and permission conflict cases, but led to a\n>> malformed \"ERROR: in myfile.c\" message in the case of a file added\n>> differently.\n>>\n>> This commit reverts that change, and uses an alternative approach without\n>> this flaw.\n>>\n>> Signed-off-by: Kevin Bracey \n>\n> We used to treat \"Both added differently\" as a separate \"info\"\n> message, just like the \"Auto-merging\" message, and let \"content\n> conflict\" that is an \"error\" to happen naturally by doing such a\n> merge, possibly followed by permission conflict which is another\n> kind of \"error\". We coalesced these two into a single message.\n>\n> And this patch breaks them into separate messages. I am not sure if\n> that aspect of the change is desirable.\n>\n> The source of \"malformed\" message seems suspicious. Isn't the root\n> cause of $msg being empty that merge-file can (sometimes) cleanly\n> merge two files using the phoney base in the \"both added\n> differently\" codepath?\n>\n> If you resolve that issue by forcing a \"conflicted\" failure when we\n> handle \"add/add\" conflict, I think the behaviour of the remainder of\n> the code is better in the original than the updated one.\n>\n> Perhaps something like this (I am applying these on 'maint')?\n\nActually, this one is even better, I think. Again on top of your\ntwo patches applied on 'maint'.\n\nAlternatively, we can remove the whole \"if $1 is empty, error the\nmerge out\" logic, which would be more in line with the spirit of\nf7d24bbefb06 (merge with /dev/null as base, instead of punting\nO==empty case, 2005-11-07), but that will be a change in behaviour\n(a \"both side added, slightly differently\" case that can cleanly\nmerge will no longer fail), so I am not sure if it is worth it.\n\n-- >8 --\nSubject: [PATCH] merge-one-file: force content conflict for \"both side added\" case\n\nHistorically, we tried to be lenient to \"both side added, slightly\ndifferently\" case and as long as the files can be merged using a\nmade-up common ancestor cleanly, since f7d24bbefb06 (merge with\n/dev/null as base, instead of punting O==empty case, 2005-11-07).\nThis was later further refined to use a better made-up common file\nwith fd66dbf5297a (merge-one-file: use empty- or common-base\ncondintionally in two-stage merge., 2005-11-10), but the spirit has\nbeen the same.\n\nBut the original fix in f7d24bbefb06 to avoid punging on \"both sides\nadded\" case had a code to unconditionally error out the merge. When\nthis triggers, even though the content-level merge can be done\ncleanly, we end up not saying \"content conflict\" in the message, but\nstill issue the error message, showing \"ERROR: in \".\n\nSigned-off-by: Junio C Hamano \n---\n git-merge-one-file.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/git-merge-one-file.sh b/git-merge-one-file.sh\nindex 25d7714..62016f4 100755\n--- a/git-merge-one-file.sh\n+++ b/git-merge-one-file.sh\n@@ -155,6 +155,7 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \tfi\n \tif test -z \"$1\"\n \tthen\n+\t\tmsg='content conflict'\n \t\tret=1\n \tfi\n \n-- \n1.8.2-297-g51e0fcd\n"},{"id":"212181","messageId":"7vboa7z98v.fsf@alter.siamese.dyndns.org","threadId":"33089","inReplyTo":"7vfvzjz9ej.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/3] git-merge-one-file: revise merge error reporting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-25T17:20:48Z","receivedAt":"2013-03-25T17:20:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Actually, this one is even better, I think. Again on top of your\n> two patches applied on 'maint'.\n\nScratch that one. The \"if test -z \"$1\"\" block needs to be moved a\nbit higher, like this (the log message can stay the same):\n\ndiff --git a/git-merge-one-file.sh b/git-merge-one-file.sh\nindex 62016f4..a4ecf33 100755\n--- a/git-merge-one-file.sh\n+++ b/git-merge-one-file.sh\n@@ -134,9 +134,10 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \tgit merge-file \"$src1\" \"$orig\" \"$src2\"\n \tret=$?\n \tmsg=\n-\tif test $ret != 0\n+\tif test $ret != 0 || test -z \"$1\"\n \tthen\n \t\tmsg='content conflict'\n+\t\tret=1\n \tfi\n \n \t# Create the working tree file, using \"our tree\" version from the\n@@ -153,11 +154,6 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \t\tmsg=\"${msg}permissions conflict: $5->$6,$7\"\n \t\tret=1\n \tfi\n-\tif test -z \"$1\"\n-\tthen\n-\t\tmsg='content conflict'\n-\t\tret=1\n-\tfi\n \n \tif test $ret != 0\n \tthen\n"},{"id":"212182","messageId":"7v7gkvz80i.fsf@alter.siamese.dyndns.org","threadId":"33089","inReplyTo":"1364126098-10788-2-git-send-email-kevin@bracey.fi","subject":"Re: [PATCH v4 2/2] mergetools/p4merge: create a base if none available","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-25T17:47:25Z","receivedAt":"2013-03-25T17:47:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Bracey writes:\n\n> Minor change from v3: that version moved initialisation of src1 higher up,\n> detaching it from its associated comment. This move was only required by\n> earlier versions, so v4 leaves src1 in its original position.\n\nThe \"funny filename\" comment was from b539c5e8fbd3 (git-merge-one:\nnew merge world order., 2005-12-07) where the removed code just\nbefore that new comment ended with:\n\n\tmerge \"$4\" \"$orig\" \"$src2\"\n\n(yes, we used to use \"merge\" program from the RCS suite). The\ncomment refers to one of the bad side effect the old code used to\nhave and warns against such a practice, i.e. it was talking about\nthe code that no longer existed.\n\nI think the two-line comment should simply go.\n\nGiven that, I _think_ it is OK to move the initialization of src1\nnext to that of src2; that may make the result easier to read.\n\nThanks.\n"},{"id":"212201","messageId":"CAPig+cRu8-6pNWbDXrPqU-yW5NDKY9WsE2wFY65303t2RpzZvQ@mail.gmail.com","threadId":"33089","inReplyTo":"7vfvzjz9ej.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/3] git-merge-one-file: revise merge error reporting","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-03-25T19:24:12Z","receivedAt":"2013-03-25T19:24:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 25, 2013 at 1:17 PM, Junio C Hamano wrote:\n> Subject: [PATCH] merge-one-file: force content conflict for \"both side added\" case\n\ns/both side/both sides/\n\n> Historically, we tried to be lenient to \"both side added, slightly\n\nDitto.\n\n> differently\" case and as long as the files can be merged using a\n> made-up common ancestor cleanly, since f7d24bbefb06 (merge with\n> /dev/null as base, instead of punting O==empty case, 2005-11-07).\n> This was later further refined to use a better made-up common file\n> with fd66dbf5297a (merge-one-file: use empty- or common-base\n> condintionally in two-stage merge., 2005-11-10), but the spirit has\n> been the same.\n>\n> But the original fix in f7d24bbefb06 to avoid punging on \"both sides\n\ns/punging/punting/\n\n> added\" case had a code to unconditionally error out the merge. When\n> this triggers, even though the content-level merge can be done\n> cleanly, we end up not saying \"content conflict\" in the message, but\n> still issue the error message, showing \"ERROR: in \".\n>\n> Signed-off-by: Junio C Hamano \n"}]}