threads / patch / 33089

patch, 2 partsp4merge: create a virtual base if none available

Subject: [PATCH 2/2] p4merge: create a virtual base if none available

## tl;dr

40 messages between Mar 6, 2013 and Mar 25, 2013. Diffs are folded; open one to read it.

replies: 39people: 4as markdown or json

Kevin Bracey· Mar 6, 2013, 20:32 UTC · lore

[PATCH 0/2] Improve P4Merge mergetool invocation

Two changes to the same piece of code that have greatly improved the behaviour of P4Merge for me. Some of it may also be applicable to other mergetools.

I've put probably overly-long-winded explanations in the commit messages.

Comments welcome. In particular, I know almost nothing of sh, so I may have made some blunder there.

Kevin Bracey (2):
  p4merge: swap LOCAL and REMOTE for mergetool
  p4merge: create a virtual base if none available
 git-mergetool--lib.sh | 14 ++++++++++++++
 mergetools/p4merge    |  4 ++--
 2 files changed, 16 insertions(+), 2 deletions(-)
-- 
1.8.2.rc2.5.g1a80410.dirty
Kevin Bracey· Mar 6, 2013, 20:32 UTC · re: Kevin Bracey · lore

[PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool

Reverse LOCAL and REMOTE when invoking P4Merge as a mergetool, so that the incoming branch is now in the left-hand, blue triangle pane, and the current branch is in the right-hand, green circle pane.

This change makes use of P4Merge consistent with its built-in help, its reference documentation, and Perforce itself. But most importantly, it makes merge results clearer. P4Merge is not totally symmetrical between left and right; despite changing a few text labels from "theirs/ours" to "left/right" when invoked manually, it still retains its original Perforce "theirs/ours" viewpoint.

Most obviously, in the result pane P4Merge shows changes that are common to both branches in green. This is on the basis of the current branch being green, as it is when invoked from Perforce; it means that lines in the result are blue if and only if they are being changed by the merge, making the resulting diff clearer. Whereas if you use blue as the current branch, then there is no single colour highlighting changes - a green line in the result could be a change, but it could also be something already in the current branch that isn't changed by the merge.

There is no need to swap LOCAL/REMOTE order for difftool; P4Merge is symmetrical in this case, and a 0- or 1-revision difftool invocation already gives the working tree ("ours") on the right in green, matching Perforce's equivalent "Diff Against Have Revision". And you couldn't swap it anyway, as it would make 2-revision difftool invocation back-to-front.

Note that P4Merge now shows "ours" on the right for both diff and merge, unlike other diff/mergetools, which always have REMOTE on the right. But observe that REMOTE is the working tree (ie "ours") for a diff, while it's another branch (ie "theirs") for a merge.

Signed-off-by: Kevin Bracey <kevin@bracey.fi>
---
 mergetools/p4merge | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to mergetools/p4merge +1 −1
diff --git a/mergetools/p4merge b/mergetools/p4merge
index 8a36916..46b3a5a 100644
--- a/mergetools/p4merge
+++ b/mergetools/p4merge
@@ -22,7 +22,7 @@ diff_cmd () {
 merge_cmd () {
 	touch "$BACKUP"
 	$base_present || >"$BASE"
-	"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
+	"$merge_tool_path" "$BASE" "$REMOTE" "$LOCAL" "$MERGED"
 	check_unchanged
 }
 
-- 
1.8.2.rc2.5.g1a80410.dirty
Junio C Hamano· Mar 7, 2013, 00:36 UTC · re: Kevin Bracey · lore

Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool

Kevin Bracey <kevin@bracey.fi> writes:
> Reverse LOCAL and REMOTE when invoking P4Merge as a mergetool, so that
> the incoming branch is now in the left-hand, blue triangle pane, and the
> current branch is in the right-hand, green circle pane.

Given that the ordering of the three variants has been the way it is since the very initial version by Scott, I'll sit on this patch until hearing from those Cc'ed (who presumably do use p4merge, unlike I who don't) that it is a good change.

Thanks.
Kevin Bracey· Mar 7, 2013, 06:16 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool

On 07/03/2013 02:36, Junio C Hamano wrote:
Show 10 quoted lines
> Kevin Bracey <kevin@bracey.fi> writes:
>
>> Reverse LOCAL and REMOTE when invoking P4Merge as a mergetool, so that
>> the incoming branch is now in the left-hand, blue triangle pane, and the
>> current branch is in the right-hand, green circle pane.
> Given that the ordering of the three variants has been the way it is
> since the very initial version by Scott, I'll sit on this patch
> until hearing from those Cc'ed (who presumably do use p4merge,
> unlike I who don't) that it is a good change.
>

I agree that this is the controversial patch of the two. It's going to chuck away 3-4 years of what Git users are used to, albeit in favour of a decade of what Perforce users are used to. And it also makes it inconsistent with all the other mergetools (at least assuming their display matches their command line).

I checked for any historical discussion from when this was added about the order, and there was none. So I'm assuming it was just done to match the other tools, maybe not realising P4Merge's "theirs/ours" convention. There was no explicit recognition at the time that they were breaking the Perforce convention, or that the order had an effect.

I've used both Git and Perforce for quite a while, but have only just started using P4Merge with Git. It seemed weirdly off and unintuitive to me at first, until I suddenly realised that it was just backwards. I would have settled for just having to get used to driving on the other side of the road, and matching other mergetools, until I realised that it effectively broke display of common changes. That's a problem.

On consistency, personally, I think there's an argument for reversing all the mergetools to match this way, as I find this orientation more intuitively aligns with difftool. But I'm not bold enough to suggest that. Yet.

Kevin
Junio C Hamano· Mar 7, 2013, 07:23 UTC · re: Kevin Bracey · lore

Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool

Kevin Bracey <kevin@bracey.fi> writes:
Show 5 quoted lines
> I agree that this is the controversial patch of the two. It's going to
> chuck away 3-4 years of what Git users are used to, albeit in favour
> of a decade of what Perforce users are used to. And it also makes it
> inconsistent with all the other mergetools (at least assuming their
> display matches their command line).

If p4merge GUI labels one side clearly as "theirs" and the other "ours", and the way we feed the inputs to it makes the side that is actually "ours" appear in p4merge GUI labelled as "theirs", then I do not think backward compatibility argument does not hold water. It is just correcting a longstanding 3-4 year old bug in a tool that nobody noticed.

For people who are very used to the way p4merge shows the merged contents by theirs-base-yours order in side-by-side view, I do not think it is unreasonable to introduce the "mergetool.$name.reverse" configuration variable and teach the mergetool frontend to pay attention to it. That will allow them to see their merge in reverse order even when they are using a backend other than p4merge.

With such a mechanism in place, by default, we can just declare that mergetool.p4merge.reverse is "true" when unset, while making mergetool.$name.reverse for all the other tools default to "false". People who are already used to the way our p4merge integration works can set mergetool.p4merge.reverse to "false" explicitly to retain the historical behaviour that you are declaring "buggy" with such a change.

How does that sound?  David?
Kevin Bracey· Mar 7, 2013, 17:14 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool

On 07/03/2013 09:23, Junio C Hamano wrote:
Show 6 quoted lines
> If p4merge GUI labels one side clearly as "theirs" and the other 
> "ours", and the way we feed the inputs to it makes the side that is 
> actually "ours" appear in p4merge GUI labelled as "theirs", then I do 
> not think backward compatibility argument does not hold water. It is 
> just correcting a longstanding 3-4 year old bug in a tool that nobody 
> noticed.

It's not quite that clear-cut. Some years ago, and before p4merge was added as a Git mergetool, P4Merge was changed so its main GUI text says "left" and "right" instead of "theirs" and "ours" when invoked manually.

But it appears that's as far as they went. It doesn't seem any of its asymmetric diff display logic was changed; it works better with ours on the right, and the built-in help all remains written on the theirs/ours basis. And even little details like the icons imply it (a square for the base, a downward-pointing triangle for their incoming stuff, and a circle for the version we hold).

Show 14 quoted lines
> For people who are very used to the way p4merge shows the merged
> contents by theirs-base-yours order in side-by-side view, I do not
> think it is unreasonable to introduce the "mergetool.$name.reverse"
> configuration variable and teach the mergetool frontend to pay
> attention to it.  That will allow them to see their merge in reverse
> order even when they are using a backend other than p4merge.
>
> With such a mechanism in place, by default, we can just declare that
> mergetool.p4merge.reverse is "true" when unset, while making
> mergetool.$name.reverse for all the other tools default to "false".
> People who are already used to the way our p4merge integration works
> can set mergetool.p4merge.reverse to "false" explicitly to retain
> the historical behaviour that you are declaring "buggy" with such a
> change.

I like this idea as a user - having made this change to p4merge, it does throw me when I decide to attempt a particularly tricky merge with bc3 instead, and get the other order. The user config options you suggest sound good to me.

For completion on this idea, I'd suggest difftool.xxx.reverse, to allow the orientation for 0- and 1-revision diffs to be chosen - allow the implied working tree version to be on the left or right. That would allow "ours-theirs" order, which some would view as being more consistent with the "ours-base-theirs" default for mergetool.

Would it be going too far to also have "xxxtool.reverse" to choose the 
global default? Then the choice hierarchy would be "xxxtool.xxx.reverse 
if set" > "optional inbuilt tool preference" > "xxxtool.reverse if set" 
 > "false". So the user could request a global swap, except that they'd 
have to explicitly override any tools that have a preferred orientation.

My only reservation is that I assume it would be implemented by swapping what's passed in $LOCAL and $REMOTE. Which seems a bit icky: $LOCAL="a.REMOTE.1234.c". On the other hand, $LOCAL and $REMOTE are already not very meaningful names for difftool... Maybe we should change to using $LEFT and $RIGHT, acknowledging the existing difftool situation, and that the user can now swap merges too.

I'd be happy to prepare a fuller patch on this sort of basis.
Kevin
Junio C Hamano· Mar 7, 2013, 19:10 UTC · re: Kevin Bracey · lore

Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool

Kevin Bracey <kevin@bracey.fi> writes:
Show 19 quoted lines
> On 07/03/2013 09:23, Junio C Hamano wrote:
>> If p4merge GUI labels one side clearly as "theirs" and the other
>> "ours", and the way we feed the inputs to it makes the side that is
>> actually "ours" appear in p4merge GUI labelled as "theirs", then I
>> do not think backward compatibility argument does not hold water. It
>> is just correcting a longstanding 3-4 year old bug in a tool that
>> nobody noticed.
>
> It's not quite that clear-cut. Some years ago, and before p4merge was
> added as a Git mergetool, P4Merge was changed so its main GUI text
> says "left" and "right" instead of "theirs" and "ours" when invoked
> manually.
>
> But it appears that's as far as they went. It doesn't seem any of its
> asymmetric diff display logic was changed; it works better with ours
> on the right, and the built-in help all remains written on the
> theirs/ours basis. And even little details like the icons imply it (a
> square for the base, a downward-pointing triangle for their incoming
> stuff, and a circle for the version we hold).

So in short, a user of p4merge can see that left side is intended as "theirs", even though recent p4merge sometimes calls it "left". And your description on the coloring (green vs blue) makes it clear that "left" and "theirs" are still intended to be synonyms.

If that is the case I would think you can still argue such a change as "correcting a 3-4-year old bug".

> Would it be going too far to also have "xxxtool.reverse" to choose the
> global default?

It would be a natural thing to do. I left it out because I thought it would go without saying, given that precedences already exist, e.g. mergetool.keepBackup etc.

> My only reservation is that I assume it would be implemented by
> swapping what's passed in $LOCAL and $REMOTE. Which seems a bit icky:
> $LOCAL="a.REMOTE.1234.c".

Doesn't the UI show the actual temporary filename? When merging my version of hello.c with your version, showing them as hello.LOCAL.c and hello.REMOTE.c is an integral part of the UI experience, I think, even if the GUI tool does not give its own labels (and behaviour differences as you mentioned for p4merge) to mark which side is theirs and which side is ours. The temporary file that holds their version should still be named with REMOTE, even when the mergetool.reverse option is in effect.

As to the name of the variable, I do not care too deeply about it myself, but I think keeping the current LOCAL and REMOTE would help people following the code, especially given the option is called "reverse", meaning that there is an internal convention that the order is "LOCAL and then REMOTE".

One thing to watch out for is from which temporary file we take the merged results. You can present the two sides swapped, but if the tool always writes the results out by updating the second file, the caller needs to be prepared to read from the one that gets changed.

David Aguilar· Mar 7, 2013, 19:50 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool

On Thu, Mar 7, 2013 at 11:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 29 quoted lines
> Kevin Bracey <kevin@bracey.fi> writes:
>
>> On 07/03/2013 09:23, Junio C Hamano wrote:
>>> If p4merge GUI labels one side clearly as "theirs" and the other
>>> "ours", and the way we feed the inputs to it makes the side that is
>>> actually "ours" appear in p4merge GUI labelled as "theirs", then I
>>> do not think backward compatibility argument does not hold water. It
>>> is just correcting a longstanding 3-4 year old bug in a tool that
>>> nobody noticed.
>>
>> It's not quite that clear-cut. Some years ago, and before p4merge was
>> added as a Git mergetool, P4Merge was changed so its main GUI text
>> says "left" and "right" instead of "theirs" and "ours" when invoked
>> manually.
>>
>> But it appears that's as far as they went. It doesn't seem any of its
>> asymmetric diff display logic was changed; it works better with ours
>> on the right, and the built-in help all remains written on the
>> theirs/ours basis. And even little details like the icons imply it (a
>> square for the base, a downward-pointing triangle for their incoming
>> stuff, and a circle for the version we hold).
>
> So in short, a user of p4merge can see that left side is intended as
> "theirs", even though recent p4merge sometimes calls it "left".  And
> your description on the coloring (green vs blue) makes it clear that
> "left" and "theirs" are still intended to be synonyms.
>
> If that is the case I would think you can still argue such a change
> as "correcting a 3-4-year old bug".

I would prefer to treat this as a bugfix rather than introducing a new set of configuration knobs if possible. It really does seem like a correction.

Users that want the traditional behavior can get that by configuring a custom mergetool.p4merge.cmd, so we're not completely losing the ability to get at the old behavior.

Users that want to see a reverse diff with difftool can already say "--reverse", so there's even less reason to have it there (though I know we're talking about mergetool only).

Show 6 quoted lines
>> Would it be going too far to also have "xxxtool.reverse" to choose the
>> global default?
>
> It would be a natural thing to do.  I left it out because I thought
> it would go without saying, given that precedences already exist,
> e.g. mergetool.keepBackup etc.
Medium NACK.  If we can do without configuration all the better.

I would much rather prefer to have the default/mainstream behavior be the best out-of-the-box sans configuration.

The reasoning behind swapping them for p4merge makes sense for p4merge only. I don't think we're quite ready to declare that all the merge tools need to be swapped or that we need a mechanism for swapping the order.

Show 23 quoted lines
>> My only reservation is that I assume it would be implemented by
>> swapping what's passed in $LOCAL and $REMOTE. Which seems a bit icky:
>> $LOCAL="a.REMOTE.1234.c".
>
> Doesn't the UI show the actual temporary filename?  When merging my
> version of hello.c with your version, showing them as hello.LOCAL.c
> and hello.REMOTE.c is an integral part of the UI experience, I
> think, even if the GUI tool does not give its own labels (and
> behaviour differences as you mentioned for p4merge) to mark which
> side is theirs and which side is ours.  The temporary file that
> holds their version should still be named with REMOTE, even when the
> mergetool.reverse option is in effect.
>
> As to the name of the variable, I do not care too deeply about it
> myself, but I think keeping the current LOCAL and REMOTE would help
> people following the code, especially given the option is called
> "reverse", meaning that there is an internal convention that the
> order is "LOCAL and then REMOTE".
>
> One thing to watch out for is from which temporary file we take the
> merged results.  You can present the two sides swapped, but if the
> tool always writes the results out by updating the second file, the
> caller needs to be prepared to read from the one that gets changed.
-- 
David
Junio C Hamano· Mar 7, 2013, 20:31 UTC · re: David Aguilar · lore

Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool

David Aguilar <davvid@gmail.com> writes:
Show 19 quoted lines
> I would prefer to treat this as a bugfix rather than introducing
> a new set of configuration knobs if possible.  It really does
> seem like a correction.
> 
> Users that want the traditional behavior can get that by
> configuring a custom mergetool.p4merge.cmd, so we're not
> completely losing the ability to get at the old behavior.
>
> Users that want to see a reverse diff with difftool can
> already say "--reverse", so there's even less reason to
> have it there (though I know we're talking about mergetool only).
> ...
> I would much rather prefer to have the default/mainstream
> behavior be the best out-of-the-box sans configuration.
>
> The reasoning behind swapping them for p4merge makes sense
> for p4merge only.  I don't think we're quite ready to declare
> that all the merge tools need to be swapped or that we need a
> mechanism for swapping the order.
Thanks for an injection of sanity.
Kevin Bracey· Mar 6, 2013, 20:32 UTC · re: Kevin Bracey · lore
Originally, with no base, Git gave P4Merge $LOCAL as a dummy base:
   p4merge "$LOCAL" "$LOCAL" "$REMOTE" "$MERGED"
Commit 0a0ec7bd changed this to:
   p4merge "empty file" "$LOCAL" "$REMOTE" "$MERGED"
to avoid the problem of being unable to save in some circumstances.

Unfortunately this approach does not produce good results at all on differing inputs. P4Merge really regards the blank file as the base, and once you have just a couple of differences between the two branches you end up with one a massive full-file conflict. The diff is not readable, and you have to invoke "difftool MERGE_HEAD HEAD" manually to see a 2-way diff.

The original form appears to have invoked special 2-way comparison behaviour that occurs only if the base filename is "" or equal to the left input. You get a good diff, and it does not auto-resolve in one direction or the other. (Normally if one branch equals the base, it would autoresolve to the other branch).

But there appears to be no way of getting this 2-way behaviour and being able to reliably save. Having base=left appears to be triggering other assumptions. There are tricks the user can use to force the save icon on, but it's not intuitive.

So we now follow a suggestion given in the original patch's discussion: generate a virtual base, consisting of the lines common to the two branches. It produces a much nicer 3-way diff view than either of the original forms, and than I suspect other mergetools are managing.

Signed-off-by: Kevin Bracey <kevin@bracey.fi>
---
 git-mergetool--lib.sh | 14 ++++++++++++++
 mergetools/p4merge    |  2 +-
 2 files changed, 15 insertions(+), 1 deletion(-)
Show changes to 2 files +15 −1

git-mergetool--lib.sh, mergetools/p4merge

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index e338be5..5b60cf5 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -108,6 +108,20 @@ check_unchanged () {
 	fi
 }
 
+make_virtual_base() {
+		# Copied from git-merge-one-file.sh.
+		# This starts with $LOCAL, and uses git apply to
+		# remove lines that are not in $REMOTE.
+		cp -- "$LOCAL" "$BASE"
+		sz0=`wc -c <"$BASE"`
+		@@DIFF@@ -u -L"a/$BASE" -L"b/$BASE" "$BASE" "$REMOTE" | git apply --no-add
+		sz1=`wc -c <"$BASE"`
+
+		# If we do not have enough common material, it is not
+		# worth trying two-file merge using common subsections.
+		expr $sz0 \< $sz1 \* 2 >/dev/null || : >"$BASE"
+}
+
 valid_tool () {
 	setup_tool "$1" && return 0
 	cmd=$(get_merge_tool_cmd "$1")
diff --git a/mergetools/p4merge b/mergetools/p4merge
index 46b3a5a..f0a893b 100644
--- a/mergetools/p4merge
+++ b/mergetools/p4merge
@@ -21,7 +21,7 @@ diff_cmd () {
 
 merge_cmd () {
 	touch "$BACKUP"
-	$base_present || >"$BASE"
+	$base_present || make_virtual_base
 	"$merge_tool_path" "$BASE" "$REMOTE" "$LOCAL" "$MERGED"
 	check_unchanged
 }
-- 
1.8.2.rc2.5.g1a80410.dirty
David Aguilar· Mar 7, 2013, 02:23 UTC · re: Kevin Bracey · lore

Re: [PATCH 2/2] p4merge: create a virtual base if none available

On Wed, Mar 6, 2013 at 12:32 PM, Kevin Bracey <kevin@bracey.fi> wrote:
Show 49 quoted lines
> Originally, with no base, Git gave P4Merge $LOCAL as a dummy base:
>
>    p4merge "$LOCAL" "$LOCAL" "$REMOTE" "$MERGED"
>
> Commit 0a0ec7bd changed this to:
>
>    p4merge "empty file" "$LOCAL" "$REMOTE" "$MERGED"
>
> to avoid the problem of being unable to save in some circumstances.
>
> Unfortunately this approach does not produce good results at all on
> differing inputs. P4Merge really regards the blank file as the base, and
> once you have just a couple of differences between the two branches you
> end up with one a massive full-file conflict. The diff is not readable,
> and you have to invoke "difftool MERGE_HEAD HEAD" manually to see a
> 2-way diff.
>
> The original form appears to have invoked special 2-way comparison
> behaviour that occurs only if the base filename is "" or equal to the
> left input.  You get a good diff, and it does not auto-resolve in one
> direction or the other. (Normally if one branch equals the base, it
> would autoresolve to the other branch).
>
> But there appears to be no way of getting this 2-way behaviour and being
> able to reliably save. Having base=left appears to be triggering other
> assumptions. There are tricks the user can use to force the save icon
> on, but it's not intuitive.
>
> So we now follow a suggestion given in the original patch's discussion:
> generate a virtual base, consisting of the lines common to the two
> branches. It produces a much nicer 3-way diff view than either of the
> original forms, and than I suspect other mergetools are managing.
>
> Signed-off-by: Kevin Bracey <kevin@bracey.fi>
> ---
>  git-mergetool--lib.sh | 14 ++++++++++++++
>  mergetools/p4merge    |  2 +-
>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index e338be5..5b60cf5 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -108,6 +108,20 @@ check_unchanged () {
>         fi
>  }
>
> +make_virtual_base() {
> +               # Copied from git-merge-one-file.sh.
I think the reasoning behind these patches is good.

How do we feel about this duplication? Should we make a common function in the git-sh-setup.sh, or is it okay to have a slightly modified version of this function in two places?

Show 31 quoted lines
> +               # This starts with $LOCAL, and uses git apply to
> +               # remove lines that are not in $REMOTE.
> +               cp -- "$LOCAL" "$BASE"
> +               sz0=`wc -c <"$BASE"`
> +               @@DIFF@@ -u -L"a/$BASE" -L"b/$BASE" "$BASE" "$REMOTE" | git apply --no-add
> +               sz1=`wc -c <"$BASE"`
> +
> +               # If we do not have enough common material, it is not
> +               # worth trying two-file merge using common subsections.
> +               expr $sz0 \< $sz1 \* 2 >/dev/null || : >"$BASE"
> +}
> +
>  valid_tool () {
>         setup_tool "$1" && return 0
>         cmd=$(get_merge_tool_cmd "$1")
> diff --git a/mergetools/p4merge b/mergetools/p4merge
> index 46b3a5a..f0a893b 100644
> --- a/mergetools/p4merge
> +++ b/mergetools/p4merge
> @@ -21,7 +21,7 @@ diff_cmd () {
>
>  merge_cmd () {
>         touch "$BACKUP"
> -       $base_present || >"$BASE"
> +       $base_present || make_virtual_base
>         "$merge_tool_path" "$BASE" "$REMOTE" "$LOCAL" "$MERGED"
>         check_unchanged
>  }
> --
> 1.8.2.rc2.5.g1a80410.dirty
>
-- 
David
Kevin Bracey· Mar 7, 2013, 06:28 UTC · re: David Aguilar · lore

Re: [PATCH 2/2] p4merge: create a virtual base if none available

On 07/03/2013 04:23, David Aguilar wrote:
Show 6 quoted lines
> On Wed, Mar 6, 2013 at 12:32 PM, Kevin Bracey <kevin@bracey.fi> wrote:
>> +make_virtual_base() {
>> +               # Copied from git-merge-one-file.sh.
> I think the reasoning behind these patches is good.
>
> How do we feel about this duplication?
Bad.
> Should we make a common function in the git-sh-setup.sh,
> or is it okay to have a slightly modified version of this
> function in two places?

I'd prefer to have a common function, I just didn't know if there was somewhere appropriate to place it, available from both files. And I'm going to have to learn a bit more sh to get it right.

> Also, the "@@DIFF@@" string may not work here.
> This is a template string that is replaced by the Makefile.
It does work in git-mergetool--lib.sh, but not in mergetools/p4merge.
Show 5 quoted lines
> We prefer $(command) instead of `command`.
> These should be adjusted.
>
> Can the same thing be accomplished using "git diff --no-index"
> so that we do not need a dependency on an external "diff" command here?

Do these comments still apply if it's a common function in git-sh-setup.sh that git-one-merge-file.sh will use? I'm wary of layering violations.

Kevin
Junio C Hamano· Mar 7, 2013, 07:25 UTC · re: David Aguilar · lore

Re: [PATCH 2/2] p4merge: create a virtual base if none available

David Aguilar <davvid@gmail.com> writes:
> How do we feel about this duplication?
> Should we make a common function in the git-sh-setup.sh,
> or is it okay to have a slightly modified version of this
> function in two places?

It probably is a good idea to have it in one place. That would also solve the @@DIFF@@ replacement issue you noticed at the same time.

David Aguilar· Mar 7, 2013, 03:33 UTC · re: Kevin Bracey · lore

Re: [PATCH 2/2] p4merge: create a virtual base if none available

On Wed, Mar 6, 2013 at 12:32 PM, Kevin Bracey <kevin@bracey.fi> wrote:
Show 13 quoted lines
> +make_virtual_base() {
> +               # Copied from git-merge-one-file.sh.
> +               # This starts with $LOCAL, and uses git apply to
> +               # remove lines that are not in $REMOTE.
> +               cp -- "$LOCAL" "$BASE"
> +               sz0=`wc -c <"$BASE"`
> +               @@DIFF@@ -u -L"a/$BASE" -L"b/$BASE" "$BASE" "$REMOTE" | git apply --no-add
> +               sz1=`wc -c <"$BASE"`
> +
> +               # If we do not have enough common material, it is not
> +               # worth trying two-file merge using common subsections.
> +               expr $sz0 \< $sz1 \* 2 >/dev/null || : >"$BASE"
> +}

This seems to be indented deeper then the other functions (or gmail is whitespace damaging my view). Please use one hard tab to indent here.

We prefer $(command) instead of `command`. These should be adjusted.

Also, the "@@DIFF@@" string may not work here. This is a template string that is replaced by the Makefile.

I don't think the tools in the mergetools/ directory go through cmd_munge_script so this is not going to work as-is.

Can the same thing be accomplished using "git diff --no-index" so that we do not need a dependency on an external "diff" command here?

I am not a regular p4merge user myself so I'll defer to others on the cc: list for their thoughts. It does seem like a good idea, though.

-- 
David
Kevin Bracey· Mar 9, 2013, 19:20 UTC · re: Kevin Bracey · lore

[PATCH v2 0/3] Improve P4Merge mergetool invocation

Incorporated comments on the previous patches, and one new patch addressing a problem I spotted while testing git-merge-one-file.

I couldn't figure out how to use git diff to achieve the effect of the external diff here - we'd need some alternative to achieve what it does with the -L option, and I failed to come up with anything remotely elegant.

Kevin Bracey (3):
  mergetools/p4merge: swap LOCAL and REMOTE
  mergetools/p4merge: create a base if none available
  git-merge-one-file: revise merge error reporting
 git-merge-one-file.sh | 38 ++++++++++++--------------------------
 git-sh-setup.sh       | 13 +++++++++++++
 mergetools/p4merge    |  8 ++++++--
 3 files changed, 31 insertions(+), 28 deletions(-)
-- 
1.8.2.rc3.7.g77aeedb
Kevin Bracey· Mar 9, 2013, 19:20 UTC · re: Kevin Bracey · lore

[PATCH v2 1/3] mergetools/p4merge: swap LOCAL and REMOTE

Reverse LOCAL and REMOTE when invoking P4Merge as a mergetool, so that the incoming branch is now in the left-hand, blue triangle pane, and the current branch is in the right-hand, green circle pane.

This change makes use of P4Merge consistent with its built-in help, its reference documentation, and Perforce itself. But most importantly, it makes merge results clearer. P4Merge is not totally symmetrical between left and right; despite changing a few text labels from "theirs/ours" to "left/right" when invoked manually, it still retains its original Perforce "theirs/ours" viewpoint.

Most obviously, in the result pane P4Merge shows changes that are common to both branches in green. This is on the basis of the current branch being green, as it is when invoked from Perforce; it means that lines in the result are blue if and only if they are being changed by the merge, making the resulting diff clearer.

Note that P4Merge now shows "ours" on the right for both diff and merge, unlike other diff/mergetools, which always have REMOTE on the right. But observe that REMOTE is the working tree (ie "ours") for a diff, while it's another branch (ie "theirs") for a merge.

Ours and theirs are reversed for a rebase - see "git help rebase". However, this does produce the desired "show the results of this commit" effect in P4Merge - changes that remain in the rebased commit (in your branch, but not in the new base) appear in blue; changes that do not appear in the rebased commit (from the new base, or common to both) are in green. If Perforce had rebase, they'd probably not swap ours/theirs, but make P4Merge show common changes in blue, picking out our changes in green. We can't do that, so this is next best.

Signed-off-by: Kevin Bracey <kevin@bracey.fi>
---
 mergetools/p4merge | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to mergetools/p4merge +1 −1
diff --git a/mergetools/p4merge b/mergetools/p4merge
index 8a36916..46b3a5a 100644
--- a/mergetools/p4merge
+++ b/mergetools/p4merge
@@ -22,7 +22,7 @@ diff_cmd () {
 merge_cmd () {
 	touch "$BACKUP"
 	$base_present || >"$BASE"
-	"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
+	"$merge_tool_path" "$BASE" "$REMOTE" "$LOCAL" "$MERGED"
 	check_unchanged
 }
 
-- 
1.8.2.rc3.7.g77aeedb
Kevin Bracey· Mar 9, 2013, 19:20 UTC · re: Kevin Bracey · lore

[PATCH v2 2/3] mergetools/p4merge: create a base if none available

Originally, with no base, Git gave P4Merge $LOCAL as a dummy base:
   p4merge "$LOCAL" "$LOCAL" "$REMOTE" "$MERGED"
Commit 0a0ec7bd changed this to:
   p4merge "empty file" "$LOCAL" "$REMOTE" "$MERGED"

to avoid the problem of being unable to save in some circumstances with similar inputs.

Unfortunately this approach produces much worse results on differing inputs. P4Merge really regards the blank file as the base, and once you have just a couple of differences between the two branches you end up with one a massive full-file conflict. The 3-way diff is not readable, and you have to invoke "difftool MERGE_HEAD HEAD" manually to get a useful view.

The original approach appears to have invoked special 2-way merge behaviour in P4Merge that occurs only if the base filename is "" or equal to the left input. You get a good visual comparison, and it does not auto-resolve differences. (Normally if one branch matched the base, it would autoresolve to the other branch).

But there appears to be no way of getting this 2-way behaviour and being able to reliably save. Having base==left appears to be triggering other assumptions. There are tricks the user can use to force the save icon on, but it's not intuitive.

So we now follow a suggestion given in the original patch's discussion: generate a virtual base, consisting of the lines common to the two branches. This is the same as the technique used in resolve and octopus merges, so we relocate that code to a shared function.

Note that if there are no differences at the same location, this technique can lead to automatic resolution without conflict, combining everything from the 2 files. As with the other merges using this technique, we assume the user will inspect the result before saving.

Signed-off-by: Kevin Bracey <kevin@bracey.fi>
---
 git-merge-one-file.sh | 18 +++++-------------
 git-sh-setup.sh       | 13 +++++++++++++
 mergetools/p4merge    |  6 +++++-
 3 files changed, 23 insertions(+), 14 deletions(-)
Show changes to 3 files +23 −14

git-merge-one-file.sh, git-sh-setup.sh, mergetools/p4merge

diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index f612cb8..1236fbf 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -104,30 +104,22 @@ case "${1:-.}${2:-.}${3:-.}" in
 		;;
 	esac
 
-	src2=`git-unpack-file $3`
+	src1=$(git-unpack-file $2)
+	src2=$(git-unpack-file $3)
 	case "$1" in
 	'')
 		echo "Added $4 in both, but differently."
-		# This extracts OUR file in $orig, and uses git apply to
-		# remove lines that are unique to ours.
-		orig=`git-unpack-file $2`
-		sz0=`wc -c <"$orig"`
-		@@DIFF@@ -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
-		sz1=`wc -c <"$orig"`
-
-		# If we do not have enough common material, it is not
-		# worth trying two-file merge using common subsections.
-		expr $sz0 \< $sz1 \* 2 >/dev/null || : >$orig
+		orig=$(git-unpack-file $2)
+		create_virtual_base "$orig" "$src1" "$src2"
 		;;
 	*)
 		echo "Auto-merging $4"
-		orig=`git-unpack-file $1`
+		orig=$(git-unpack-file $1)
 		;;
 	esac
 
 	# Be careful for funny filename such as "-L" in "$4", which
 	# would confuse "merge" greatly.
-	src1=`git-unpack-file $2`
 	git merge-file "$src1" "$orig" "$src2"
 	ret=$?
 	msg=
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 795edd2..aa9a732 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -249,6 +249,19 @@ clear_local_git_env() {
 	unset $(git rev-parse --local-env-vars)
 }
 
+# Generate a virtual base file for a two-file merge. On entry the
+# base file $1 should be a copy of $2. Uses git apply to remove
+# lines from $1 that are not in $3, leaving only common lines.
+create_virtual_base() {
+	sz0=$(wc -c <"$1")
+	@@DIFF@@ -u -La/"$1" -Lb/"$1" "$2" "$3" | git apply --no-add
+	sz1=$(wc -c <"$1")
+
+	# If we do not have enough common material, it is not
+	# worth trying two-file merge using common subsections.
+	expr $sz0 \< $sz1 \* 2 >/dev/null || : >"$1"
+}
+
 
 # Platform specific tweaks to work around some commands
 case $(uname -s) in
diff --git a/mergetools/p4merge b/mergetools/p4merge
index 46b3a5a..16ae0cc 100644
--- a/mergetools/p4merge
+++ b/mergetools/p4merge
@@ -21,7 +21,11 @@ diff_cmd () {
 
 merge_cmd () {
 	touch "$BACKUP"
-	$base_present || >"$BASE"
+	if ! $base_present
+	then
+		cp -- "$LOCAL" "$BASE"
+		create_virtual_base "$BASE" "$LOCAL" "$REMOTE"
+	fi
 	"$merge_tool_path" "$BASE" "$REMOTE" "$LOCAL" "$MERGED"
 	check_unchanged
 }
-- 
1.8.2.rc3.7.g77aeedb
Junio C Hamano· Mar 10, 2013, 04:55 UTC · re: Kevin Bracey · lore

Re: [PATCH v2 2/3] mergetools/p4merge: create a base if none available

Kevin Bracey <kevin@bracey.fi> writes:
Show 21 quoted lines
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 795edd2..aa9a732 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -249,6 +249,19 @@ clear_local_git_env() {
>  	unset $(git rev-parse --local-env-vars)
>  }
>  
> +# Generate a virtual base file for a two-file merge. On entry the
> +# base file $1 should be a copy of $2. Uses git apply to remove
> +# lines from $1 that are not in $3, leaving only common lines.
> +create_virtual_base() {
> +	sz0=$(wc -c <"$1")
> +	@@DIFF@@ -u -La/"$1" -Lb/"$1" "$2" "$3" | git apply --no-add
> +	sz1=$(wc -c <"$1")
> +
> +	# If we do not have enough common material, it is not
> +	# worth trying two-file merge using common subsections.
> +	expr $sz0 \< $sz1 \* 2 >/dev/null || : >"$1"
> +}
> +
This rewrite is wrong.  It should be
> +	sz0=$(wc -c <"$1")
> +	@@DIFF@@ -u -La/"$1" -Lb/"$1" "$1" "$3" | git apply --no-add
> +	sz1=$(wc -c <"$1")

for it to make sense. "diff $1 $3" is a change to go from $1 to $3; with "-La/$1 -Lb/$1", we declare that the change is to be applied to $1, and use --no-add to only use the removal from the diff when we edit $1 using this mechanism.

The end effect is to in-place edit "$1" to remove what is not common with "$3", and sz0/sz1 computation is done on "$1" for this reason. Does it (i.e. "$1") shrink sufficiently when we remove the material that is not common in it (i.e. "$1") and "$3"?

This part is a two-file operation between $1 and $3; there is nothing you would want to pass $2 to influence what the above three lines do.

It may happen that the caller has two copies of the same thing, $orig and $src1, and uses one for $1 and the other for $2, so you won't observe the damage from the incorrect rewriting of the above logic, but it invites the next caller to incorrectly feed something totally unrelated to $1 and $2.

Please fix it to a function that takes two temporary paths, not three.

Kevin Bracey· Mar 9, 2013, 19:21 UTC · re: Kevin Bracey · lore

[PATCH v2 3/3] git-merge-one-file: revise merge error reporting

Commit 718135e improved the merge error reporting for the resolve strategy's merge conflict and permission conflict cases, but led to a malformed "ERROR: in myfile.c" message in the case of a file added differently.

This commit reverts that change, and uses an alternative approach without this flaw.

Signed-off-by: Kevin Bracey <kevin@bracey.fi>
---
 git-merge-one-file.sh | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)
Show changes to git-merge-one-file.sh +7 −13
diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 1236fbf..70f36f1 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -104,11 +104,13 @@ case "${1:-.}${2:-.}${3:-.}" in
 		;;
 	esac
 
+	ret=0
 	src1=$(git-unpack-file $2)
 	src2=$(git-unpack-file $3)
 	case "$1" in
 	'')
-		echo "Added $4 in both, but differently."
+		echo "ERROR: Added $4 in both, but differently."
+		ret=1
 		orig=$(git-unpack-file $2)
 		create_virtual_base "$orig" "$src1" "$src2"
 		;;
@@ -121,10 +123,9 @@ case "${1:-.}${2:-.}${3:-.}" in
 	# Be careful for funny filename such as "-L" in "$4", which
 	# would confuse "merge" greatly.
 	git merge-file "$src1" "$orig" "$src2"
-	ret=$?
-	msg=
-	if [ $ret -ne 0 ]; then
-		msg='content conflict'
+	if [ $? -ne 0 ]; then
+		echo "ERROR: Content conflict in $4"
+		ret=1
 	fi
 
 	# Create the working tree file, using "our tree" version from the
@@ -133,18 +134,11 @@ case "${1:-.}${2:-.}${3:-.}" in
 	rm -f -- "$orig" "$src1" "$src2"
 
 	if [ "$6" != "$7" ]; then
-		if [ -n "$msg" ]; then
-			msg="$msg, "
-		fi
-		msg="${msg}permissions conflict: $5->$6,$7"
-		ret=1
-	fi
-	if [ "$1" = '' ]; then
+		echo "ERROR: Permissions conflict: $5->$6,$7"
 		ret=1
 	fi
 
 	if [ $ret -ne 0 ]; then
-		echo "ERROR: $msg in $4"
 		exit 1
 	fi
 	exec git update-index -- "$4"
-- 
1.8.2.rc3.7.g77aeedb
Kevin Bracey· Mar 13, 2013, 01:12 UTC · re: Kevin Bracey · lore

[PATCH v3 1/3] mergetools/p4merge: swap LOCAL and REMOTE

Reverse LOCAL and REMOTE when invoking P4Merge as a mergetool, so that the incoming branch is now in the left-hand, blue triangle pane, and the current branch is in the right-hand, green circle pane.

This change makes use of P4Merge consistent with its built-in help, its reference documentation, and Perforce itself. But most importantly, it makes merge results clearer. P4Merge is not totally symmetrical between left and right; despite changing a few text labels from "theirs/ours" to "left/right" when invoked manually, it still retains its original Perforce "theirs/ours" viewpoint.

Most obviously, in the result pane P4Merge shows changes that are common to both branches in green. This is on the basis of the current branch being green, as it is when invoked from Perforce; it means that lines in the result are blue if and only if they are being changed by the merge, making the resulting diff clearer.

Note that P4Merge now shows "ours" on the right for both diff and merge, unlike other diff/mergetools, which always have REMOTE on the right. But observe that REMOTE is the working tree (ie "ours") for a diff, while it's another branch (ie "theirs") for a merge.

Ours and theirs are reversed for a rebase - see "git help rebase". However, this does produce the desired "show the results of this commit" effect in P4Merge - changes that remain in the rebased commit (in your branch, but not in the new base) appear in blue; changes that do not appear in the rebased commit (from the new base, or common to both) are in green. If Perforce had rebase, they'd probably not swap ours/theirs, but make P4Merge show common changes in blue, picking out our changes in green. We can't do that, so this is next best.

Signed-off-by: Kevin Bracey <kevin@bracey.fi>
---
 mergetools/p4merge | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to mergetools/p4merge +1 −1
diff --git a/mergetools/p4merge b/mergetools/p4merge
index 8a36916..46b3a5a 100644
--- a/mergetools/p4merge
+++ b/mergetools/p4merge
@@ -22,7 +22,7 @@ diff_cmd () {
 merge_cmd () {
 	touch "$BACKUP"
 	$base_present || >"$BASE"
-	"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
+	"$merge_tool_path" "$BASE" "$REMOTE" "$LOCAL" "$MERGED"
 	check_unchanged
 }
 
-- 
1.8.2.rc3.7.g1100d09.dirty
Kevin Bracey· Mar 13, 2013, 01:12 UTC · re: Kevin Bracey · lore

[PATCH v3 2/3] mergetools/p4merge: create a base if none available

Originally, with no base, Git gave P4Merge $LOCAL as a dummy base:
   p4merge "$LOCAL" "$LOCAL" "$REMOTE" "$MERGED"
Commit 0a0ec7bd changed this to:
   p4merge "empty file" "$LOCAL" "$REMOTE" "$MERGED"

to avoid the problem of being unable to save in some circumstances with similar inputs.

Unfortunately this approach produces much worse results on differing inputs. P4Merge really regards the blank file as the base, and once you have just a couple of differences between the two branches you end up with one a massive full-file conflict. The 3-way diff is not readable, and you have to invoke "difftool MERGE_HEAD HEAD" manually to get a useful view.

The original approach appears to have invoked special 2-way merge behaviour in P4Merge that occurs only if the base filename is "" or equal to the left input. You get a good visual comparison, and it does not auto-resolve differences. (Normally if one branch matched the base, it would autoresolve to the other branch).

But there appears to be no way of getting this 2-way behaviour and being able to reliably save. Having base==left appears to be triggering other assumptions. There are tricks the user can use to force the save icon on, but it's not intuitive.

So we now follow a suggestion given in the original patch's discussion: generate a virtual base, consisting of the lines common to the two branches. This is the same as the technique used in resolve and octopus merges, so we relocate that code to a shared function.

Note that if there are no differences at the same location, this technique can lead to automatic resolution without conflict, combining everything from the 2 files. As with the other merges using this technique, we assume the user will inspect the result before saving.

Signed-off-by: Kevin Bracey <kevin@bracey.fi>
---
 Documentation/git-sh-setup.txt |  6 ++++++
 git-merge-one-file.sh          | 18 +++++-------------
 git-sh-setup.sh                | 12 ++++++++++++
 mergetools/p4merge             |  6 +++++-
 4 files changed, 28 insertions(+), 14 deletions(-)
Show changes to 4 files +28 −14

Documentation/git-sh-setup.txt, git-merge-one-file.sh, git-sh-setup.sh, mergetools/p4merge

diff --git a/Documentation/git-sh-setup.txt b/Documentation/git-sh-setup.txt
index 6a9f66d..5d709d0 100644
--- a/Documentation/git-sh-setup.txt
+++ b/Documentation/git-sh-setup.txt
@@ -82,6 +82,12 @@ get_author_ident_from_commit::
 	outputs code for use with eval to set the GIT_AUTHOR_NAME,
 	GIT_AUTHOR_EMAIL and GIT_AUTHOR_DATE variables for a given commit.
 
+create_virtual_base::
+	modifies the first file so only lines in common with the
+	second file remain. If there is insufficient common material,
+	then the first file is left empty. The result is suitable
+	as a virtual base input for a 3-way merge.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index f612cb8..0f164e5 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -104,30 +104,22 @@ case "${1:-.}${2:-.}${3:-.}" in
 		;;
 	esac
 
-	src2=`git-unpack-file $3`
+	src1=$(git-unpack-file $2)
+	src2=$(git-unpack-file $3)
 	case "$1" in
 	'')
 		echo "Added $4 in both, but differently."
-		# This extracts OUR file in $orig, and uses git apply to
-		# remove lines that are unique to ours.
-		orig=`git-unpack-file $2`
-		sz0=`wc -c <"$orig"`
-		@@DIFF@@ -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
-		sz1=`wc -c <"$orig"`
-
-		# If we do not have enough common material, it is not
-		# worth trying two-file merge using common subsections.
-		expr $sz0 \< $sz1 \* 2 >/dev/null || : >$orig
+		orig=$(git-unpack-file $2)
+		create_virtual_base "$orig" "$src2"
 		;;
 	*)
 		echo "Auto-merging $4"
-		orig=`git-unpack-file $1`
+		orig=$(git-unpack-file $1)
 		;;
 	esac
 
 	# Be careful for funny filename such as "-L" in "$4", which
 	# would confuse "merge" greatly.
-	src1=`git-unpack-file $2`
 	git merge-file "$src1" "$orig" "$src2"
 	ret=$?
 	msg=
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 795edd2..349a5d4 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -249,6 +249,18 @@ clear_local_git_env() {
 	unset $(git rev-parse --local-env-vars)
 }
 
+# Generate a virtual base file for a two-file merge. Uses git apply to
+# remove lines from $1 that are not in $2, leaving only common lines.
+create_virtual_base() {
+	sz0=$(wc -c <"$1")
+	@@DIFF@@ -u -La/"$1" -Lb/"$1" "$1" "$2" | git apply --no-add
+	sz1=$(wc -c <"$1")
+
+	# If we do not have enough common material, it is not
+	# worth trying two-file merge using common subsections.
+	expr $sz0 \< $sz1 \* 2 >/dev/null || : >"$1"
+}
+
 
 # Platform specific tweaks to work around some commands
 case $(uname -s) in
diff --git a/mergetools/p4merge b/mergetools/p4merge
index 46b3a5a..5a608ab 100644
--- a/mergetools/p4merge
+++ b/mergetools/p4merge
@@ -21,7 +21,11 @@ diff_cmd () {
 
 merge_cmd () {
 	touch "$BACKUP"
-	$base_present || >"$BASE"
+	if ! $base_present
+	then
+		cp -- "$LOCAL" "$BASE"
+		create_virtual_base "$BASE" "$REMOTE"
+	fi
 	"$merge_tool_path" "$BASE" "$REMOTE" "$LOCAL" "$MERGED"
 	check_unchanged
 }
-- 
1.8.2.rc3.7.g1100d09.dirty
Kevin Bracey· Mar 13, 2013, 01:12 UTC · re: Kevin Bracey · lore

[PATCH v3 3/3] git-merge-one-file: revise merge error reporting

Commit 718135e improved the merge error reporting for the resolve strategy's merge conflict and permission conflict cases, but led to a malformed "ERROR: in myfile.c" message in the case of a file added differently.

This commit reverts that change, and uses an alternative approach without this flaw.

Signed-off-by: Kevin Bracey <kevin@bracey.fi>
---
 git-merge-one-file.sh | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)
Show changes to git-merge-one-file.sh +7 −13
diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 0f164e5..78b07a8 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -104,11 +104,13 @@ case "${1:-.}${2:-.}${3:-.}" in
 		;;
 	esac
 
+	ret=0
 	src1=$(git-unpack-file $2)
 	src2=$(git-unpack-file $3)
 	case "$1" in
 	'')
-		echo "Added $4 in both, but differently."
+		echo "ERROR: Added $4 in both, but differently."
+		ret=1
 		orig=$(git-unpack-file $2)
 		create_virtual_base "$orig" "$src2"
 		;;
@@ -121,10 +123,9 @@ case "${1:-.}${2:-.}${3:-.}" in
 	# Be careful for funny filename such as "-L" in "$4", which
 	# would confuse "merge" greatly.
 	git merge-file "$src1" "$orig" "$src2"
-	ret=$?
-	msg=
-	if [ $ret -ne 0 ]; then
-		msg='content conflict'
+	if [ $? -ne 0 ]; then
+		echo "ERROR: Content conflict in $4"
+		ret=1
 	fi
 
 	# Create the working tree file, using "our tree" version from the
@@ -133,18 +134,11 @@ case "${1:-.}${2:-.}${3:-.}" in
 	rm -f -- "$orig" "$src1" "$src2"
 
 	if [ "$6" != "$7" ]; then
-		if [ -n "$msg" ]; then
-			msg="$msg, "
-		fi
-		msg="${msg}permissions conflict: $5->$6,$7"
-		ret=1
-	fi
-	if [ "$1" = '' ]; then
+		echo "ERROR: Permissions conflict: $5->$6,$7"
 		ret=1
 	fi
 
 	if [ $ret -ne 0 ]; then
-		echo "ERROR: $msg in $4"
 		exit 1
 	fi
 	exec git update-index -- "$4"
-- 
1.8.2.rc3.7.g1100d09.dirty
David Aguilar· Mar 13, 2013, 09:03 UTC · re: Kevin Bracey · lore

Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting

On Tue, Mar 12, 2013 at 6:12 PM, Kevin Bracey <kevin@bracey.fi> wrote:
Show 10 quoted lines
> Commit 718135e improved the merge error reporting for the resolve
> strategy's merge conflict and permission conflict cases, but led to a
> malformed "ERROR:  in myfile.c" message in the case of a file added
> differently.
>
> This commit reverts that change, and uses an alternative approach without
> this flaw.
>
> Signed-off-by: Kevin Bracey <kevin@bracey.fi>
> ---

I wonder whether before these changes we should update the style in this file to follow Documentation/CodingGuidelines.

Not in this patch, but in the file right now there's this part that stands out:

	if [ "$2" ]; then
		echo "Removing $4"
I think that expression would read more clearly as:
	if test -n "$2"
	then
		echo "Removing $4"
Ditto `if [ "$1" = '' ]` is better written as `test -z "$1"`.
Can you please send a patch to true these up?

It'd be especially nice if the style patch could come first, followed by the fixes/features ;-)

Show 33 quoted lines
>  git-merge-one-file.sh | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
> index 0f164e5..78b07a8 100755
> --- a/git-merge-one-file.sh
> +++ b/git-merge-one-file.sh
> @@ -104,11 +104,13 @@ case "${1:-.}${2:-.}${3:-.}" in
>                 ;;
>         esac
>
> +       ret=0
>         src1=$(git-unpack-file $2)
>         src2=$(git-unpack-file $3)
>         case "$1" in
>         '')
> -               echo "Added $4 in both, but differently."
> +               echo "ERROR: Added $4 in both, but differently."
> +               ret=1
>                 orig=$(git-unpack-file $2)
>                 create_virtual_base "$orig" "$src2"
>                 ;;
> @@ -121,10 +123,9 @@ case "${1:-.}${2:-.}${3:-.}" in
>         # Be careful for funny filename such as "-L" in "$4", which
>         # would confuse "merge" greatly.
>         git merge-file "$src1" "$orig" "$src2"
> -       ret=$?
> -       msg=
> -       if [ $ret -ne 0 ]; then
> -               msg='content conflict'
> +       if [ $? -ne 0 ]; then
> +               echo "ERROR: Content conflict in $4"
> +               ret=1

if test $? != 0 then

Also.. should this error not go to stderr? I guess the existing script was not doing that, but it seems like anything that says "ERROR" should go there.

Show 23 quoted lines
>         fi
>
>         # Create the working tree file, using "our tree" version from the
> @@ -133,18 +134,11 @@ case "${1:-.}${2:-.}${3:-.}" in
>         rm -f -- "$orig" "$src1" "$src2"
>
>         if [ "$6" != "$7" ]; then
> -               if [ -n "$msg" ]; then
> -                       msg="$msg, "
> -               fi
> -               msg="${msg}permissions conflict: $5->$6,$7"
> -               ret=1
> -       fi
> -       if [ "$1" = '' ]; then
> +               echo "ERROR: Permissions conflict: $5->$6,$7"
>                 ret=1
>         fi
>
>         if [ $ret -ne 0 ]; then
> -               echo "ERROR: $msg in $4"
>                 exit 1
>         fi
>         exec git update-index -- "$4"
same notes as above.  I think a style patch should come first.
-- 
David
Kevin Bracey· Mar 24, 2013, 12:26 UTC · re: David Aguilar · lore

[PATCH v2 0/3] git-merge-one-file error reporting

Style clean up, as requested, followed by the fix to the "both sides added" handling for git-merge-one-file.

This is based on v4 of my p4merge series, as they touch the same area.
Kevin Bracey (3):
  git-merge-one-file: style cleanup
  git-merge-one-file: send "ERROR:" messages to stderr
  git-merge-one-file: revise merge error reporting
 git-merge-one-file.sh | 50 +++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)
-- 
1.8.2.rc3.21.g744ac65
Kevin Bracey· Mar 24, 2013, 12:26 UTC · re: Kevin Bracey · lore

[PATCH v2 1/3] git-merge-one-file: style cleanup

Update style to match Documentation/CodingGuidelines.
Signed-off-by: Kevin Bracey <kevin@bracey.fi>
---
 git-merge-one-file.sh | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)
Show changes to git-merge-one-file.sh +17 −9
diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 255c07a..2382b1f 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -27,7 +27,7 @@ SUBDIRECTORY_OK=Yes
 cd_to_toplevel
 require_work_tree
 
-if ! test "$#" -eq 7
+if test $# != 7
 then
 	echo "$LONG_USAGE"
 	exit 1
@@ -38,7 +38,8 @@ case "${1:-.}${2:-.}${3:-.}" in
 # Deleted in both or deleted in one and unchanged in the other
 #
 "$1.." | "$1.$1" | "$1$1.")
-	if [ "$2" ]; then
+	if test -n "$2"
+	then
 		echo "Removing $4"
 	else
 		# read-tree checked that index matches HEAD already,
@@ -48,7 +49,8 @@ case "${1:-.}${2:-.}${3:-.}" in
 		# we do not have it in the index, though.
 		exec git update-index --remove -- "$4"
 	fi
-	if test -f "$4"; then
+	if test -f "$4"
+	then
 		rm -f -- "$4" &&
 		rmdir -p "$(expr "z$4" : 'z\(.*\)/')" 2>/dev/null || :
 	fi &&
@@ -78,7 +80,8 @@ case "${1:-.}${2:-.}${3:-.}" in
 # Added in both, identically (check for same permissions).
 #
 ".$3$2")
-	if [ "$6" != "$7" ]; then
+	if test "$6" != "$7"
+	then
 		echo "ERROR: File $4 added identically in both branches,"
 		echo "ERROR: but permissions conflict $6->$7."
 		exit 1
@@ -123,7 +126,8 @@ case "${1:-.}${2:-.}${3:-.}" in
 	git merge-file "$src1" "$orig" "$src2"
 	ret=$?
 	msg=
-	if [ $ret -ne 0 ]; then
+	if test $ret != 0
+	then
 		msg='content conflict'
 	fi
 
@@ -132,18 +136,22 @@ case "${1:-.}${2:-.}${3:-.}" in
 	git checkout-index -f --stage=2 -- "$4" && cat "$src1" >"$4" || exit 1
 	rm -f -- "$orig" "$src1" "$src2"
 
-	if [ "$6" != "$7" ]; then
-		if [ -n "$msg" ]; then
+	if test "$6" != "$7"
+	then
+		if test -n "$msg"
+		then
 			msg="$msg, "
 		fi
 		msg="${msg}permissions conflict: $5->$6,$7"
 		ret=1
 	fi
-	if [ "$1" = '' ]; then
+	if test -z "$1"
+	then
 		ret=1
 	fi
 
-	if [ $ret -ne 0 ]; then
+	if test $ret != 0
+	then
 		echo "ERROR: $msg in $4"
 		exit 1
 	fi
-- 
1.8.2.rc3.21.g744ac65
Kevin Bracey· Mar 24, 2013, 12:26 UTC · re: Kevin Bracey · lore

[PATCH v2 2/3] git-merge-one-file: send "ERROR:" messages to stderr

Signed-off-by: Kevin Bracey <kevin@bracey.fi>
---
 git-merge-one-file.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
Show changes to git-merge-one-file.sh +7 −7
diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 2382b1f..39b7799 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -69,7 +69,7 @@ case "${1:-.}${2:-.}${3:-.}" in
 	echo "Adding $4"
 	if test -f "$4"
 	then
-		echo "ERROR: untracked $4 is overwritten by the merge."
+		echo "ERROR: untracked $4 is overwritten by the merge." >&2
 		exit 1
 	fi
 	git update-index --add --cacheinfo "$7" "$3" "$4" &&
@@ -82,8 +82,8 @@ case "${1:-.}${2:-.}${3:-.}" in
 ".$3$2")
 	if test "$6" != "$7"
 	then
-		echo "ERROR: File $4 added identically in both branches,"
-		echo "ERROR: but permissions conflict $6->$7."
+		echo "ERROR: File $4 added identically in both branches," >&2
+		echo "ERROR: but permissions conflict $6->$7." >&2
 		exit 1
 	fi
 	echo "Adding $4"
@@ -98,11 +98,11 @@ case "${1:-.}${2:-.}${3:-.}" in
 
 	case ",$6,$7," in
 	*,120000,*)
-		echo "ERROR: $4: Not merging symbolic link changes."
+		echo "ERROR: $4: Not merging symbolic link changes." >&2
 		exit 1
 		;;
 	*,160000,*)
-		echo "ERROR: $4: Not merging conflicting submodule changes."
+		echo "ERROR: $4: Not merging conflicting submodule changes." >&2
 		exit 1
 		;;
 	esac
@@ -152,14 +152,14 @@ case "${1:-.}${2:-.}${3:-.}" in
 
 	if test $ret != 0
 	then
-		echo "ERROR: $msg in $4"
+		echo "ERROR: $msg in $4" >&2
 		exit 1
 	fi
 	exec git update-index -- "$4"
 	;;
 
 *)
-	echo "ERROR: $4: Not handling case $1 -> $2 -> $3"
+	echo "ERROR: $4: Not handling case $1 -> $2 -> $3" >&2
 	;;
 esac
 exit 1
-- 
1.8.2.rc3.21.g744ac65
Kevin Bracey· Mar 24, 2013, 12:26 UTC · re: Kevin Bracey · lore

[PATCH v2 3/3] git-merge-one-file: revise merge error reporting

Commit 718135e improved the merge error reporting for the resolve strategy's merge conflict and permission conflict cases, but led to a malformed "ERROR: in myfile.c" message in the case of a file added differently.

This commit reverts that change, and uses an alternative approach without this flaw.

Signed-off-by: Kevin Bracey <kevin@bracey.fi>
---
 git-merge-one-file.sh | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)
Show changes to git-merge-one-file.sh +7 −15
diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 39b7799..e231d20 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -107,10 +107,12 @@ case "${1:-.}${2:-.}${3:-.}" in
 		;;
 	esac
 
+	ret=0
 	src2=$(git-unpack-file $3)
 	case "$1" in
 	'')
-		echo "Added $4 in both, but differently."
+		echo "ERROR: Added $4 in both, but differently." >&2
+		ret=1
 		orig=$(git-unpack-file $2)
 		create_virtual_base "$orig" "$src2"
 		;;
@@ -124,11 +126,10 @@ case "${1:-.}${2:-.}${3:-.}" in
 	# would confuse "merge" greatly.
 	src1=$(git-unpack-file $2)
 	git merge-file "$src1" "$orig" "$src2"
-	ret=$?
-	msg=
-	if test $ret != 0
+	if test $? != 0
 	then
-		msg='content conflict'
+		echo "ERROR: Content conflict in $4" >&2
+		ret=1
 	fi
 
 	# Create the working tree file, using "our tree" version from the
@@ -138,21 +139,12 @@ case "${1:-.}${2:-.}${3:-.}" in
 
 	if test "$6" != "$7"
 	then
-		if test -n "$msg"
-		then
-			msg="$msg, "
-		fi
-		msg="${msg}permissions conflict: $5->$6,$7"
-		ret=1
-	fi
-	if test -z "$1"
-	then
+		echo "ERROR: Permissions conflict: $5->$6,$7" >&2
 		ret=1
 	fi
 
 	if test $ret != 0
 	then
-		echo "ERROR: $msg in $4" >&2
 		exit 1
 	fi
 	exec git update-index -- "$4"
-- 
1.8.2.rc3.21.g744ac65
Junio C Hamano· Mar 25, 2013, 17:04 UTC · re: Kevin Bracey · lore

Re: [PATCH v2 3/3] git-merge-one-file: revise merge error reporting

Kevin Bracey <kevin@bracey.fi> writes:
Show 9 quoted lines
> Commit 718135e improved the merge error reporting for the resolve
> strategy's merge conflict and permission conflict cases, but led to a
> malformed "ERROR:  in myfile.c" message in the case of a file added
> differently.
>
> This commit reverts that change, and uses an alternative approach without
> this flaw.
>
> Signed-off-by: Kevin Bracey <kevin@bracey.fi>

We used to treat "Both added differently" as a separate "info" message, just like the "Auto-merging" message, and let "content conflict" that is an "error" to happen naturally by doing such a merge, possibly followed by permission conflict which is another kind of "error". We coalesced these two into a single message.

And this patch breaks them into separate messages. I am not sure if that aspect of the change is desirable.

The source of "malformed" message seems suspicious. Isn't the root cause of $msg being empty that merge-file can (sometimes) cleanly merge two files using the phoney base in the "both added differently" codepath?

If you resolve that issue by forcing a "conflicted" failure when we handle "add/add" conflict, I think the behaviour of the remainder of the code is better in the original than the updated one.

Perhaps something like this (I am applying these on 'maint')?
 git-merge-one-file.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
Show changes to git-merge-one-file.sh +6 −6
diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 25d7714..aa06282 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -107,6 +107,7 @@ case "${1:-.}${2:-.}${3:-.}" in
 		;;
 	esac
 
+	add_add_conflict=
 	src2=`git-unpack-file $3`
 	case "$1" in
 	'')
@@ -121,6 +122,7 @@ case "${1:-.}${2:-.}${3:-.}" in
 		# If we do not have enough common material, it is not
 		# worth trying two-file merge using common subsections.
 		expr $sz0 \< $sz1 \* 2 >/dev/null || : >$orig
+		add_add_conflict=yes
 		;;
 	*)
 		echo "Auto-merging $4"
@@ -128,15 +130,13 @@ case "${1:-.}${2:-.}${3:-.}" in
 		;;
 	esac
 
-	# Be careful for funny filename such as "-L" in "$4", which
-	# would confuse "merge" greatly.
 	src1=`git-unpack-file $2`
-	git merge-file "$src1" "$orig" "$src2"
-	ret=$?
-	msg=
-	if test $ret != 0
+
+	ret=0 msg=
+	if git merge-file "$src1" "$orig" "$src2" || test -n "$add_add_conflict"
 	then
 		msg='content conflict'
+		ret=1
 	fi
 
 	# Create the working tree file, using "our tree" version from the
Junio C Hamano· Mar 25, 2013, 17:17 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 3/3] git-merge-one-file: revise merge error reporting

Junio C Hamano <gitster@pobox.com> writes:
Show 31 quoted lines
> Kevin Bracey <kevin@bracey.fi> writes:
>
>> Commit 718135e improved the merge error reporting for the resolve
>> strategy's merge conflict and permission conflict cases, but led to a
>> malformed "ERROR:  in myfile.c" message in the case of a file added
>> differently.
>>
>> This commit reverts that change, and uses an alternative approach without
>> this flaw.
>>
>> Signed-off-by: Kevin Bracey <kevin@bracey.fi>
>
> We used to treat "Both added differently" as a separate "info"
> message, just like the "Auto-merging" message, and let "content
> conflict" that is an "error" to happen naturally by doing such a
> merge, possibly followed by permission conflict which is another
> kind of "error".  We coalesced these two into a single message.
>
> And this patch breaks them into separate messages.  I am not sure if
> that aspect of the change is desirable.
>
> The source of "malformed" message seems suspicious.  Isn't the root
> cause of $msg being empty that merge-file can (sometimes) cleanly
> merge two files using the phoney base in the "both added
> differently" codepath?
>
> If you resolve that issue by forcing a "conflicted" failure when we
> handle "add/add" conflict, I think the behaviour of the remainder of
> the code is better in the original than the updated one.
>
> Perhaps something like this (I am applying these on 'maint')?

Actually, this one is even better, I think. Again on top of your two patches applied on 'maint'.

Alternatively, we can remove the whole "if $1 is empty, error the merge out" logic, which would be more in line with the spirit of f7d24bbefb06 (merge with /dev/null as base, instead of punting O==empty case, 2005-11-07), but that will be a change in behaviour (a "both side added, slightly differently" case that can cleanly merge will no longer fail), so I am not sure if it is worth it.

-- >8 --
Subject: [PATCH] merge-one-file: force content conflict for "both side added" case

Historically, we tried to be lenient to "both side added, slightly differently" case and as long as the files can be merged using a made-up common ancestor cleanly, since f7d24bbefb06 (merge with /dev/null as base, instead of punting O==empty case, 2005-11-07). This was later further refined to use a better made-up common file with fd66dbf5297a (merge-one-file: use empty- or common-base condintionally in two-stage merge., 2005-11-10), but the spirit has been the same.

But the original fix in f7d24bbefb06 to avoid punging on "both sides added" case had a code to unconditionally error out the merge. When this triggers, even though the content-level merge can be done cleanly, we end up not saying "content conflict" in the message, but still issue the error message, showing "ERROR: in <pathname>".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-merge-one-file.sh | 1 +
 1 file changed, 1 insertion(+)
Show changes to git-merge-one-file.sh +1 −0
diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 25d7714..62016f4 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -155,6 +155,7 @@ case "${1:-.}${2:-.}${3:-.}" in
 	fi
 	if test -z "$1"
 	then
+		msg='content conflict'
 		ret=1
 	fi
 
-- 
1.8.2-297-g51e0fcd
Junio C Hamano· Mar 25, 2013, 17:20 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 3/3] git-merge-one-file: revise merge error reporting

Junio C Hamano <gitster@pobox.com> writes:
> Actually, this one is even better, I think.  Again on top of your
> two patches applied on 'maint'.

Scratch that one. The "if test -z "$1"" block needs to be moved a bit higher, like this (the log message can stay the same):

Show changes to git-merge-one-file.sh +2 −6
diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 62016f4..a4ecf33 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -134,9 +134,10 @@ case "${1:-.}${2:-.}${3:-.}" in
 	git merge-file "$src1" "$orig" "$src2"
 	ret=$?
 	msg=
-	if test $ret != 0
+	if test $ret != 0 || test -z "$1"
 	then
 		msg='content conflict'
+		ret=1
 	fi
 
 	# Create the working tree file, using "our tree" version from the
@@ -153,11 +154,6 @@ case "${1:-.}${2:-.}${3:-.}" in
 		msg="${msg}permissions conflict: $5->$6,$7"
 		ret=1
 	fi
-	if test -z "$1"
-	then
-		msg='content conflict'
-		ret=1
-	fi
 
 	if test $ret != 0
 	then
Eric Sunshine· Mar 25, 2013, 19:24 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 3/3] git-merge-one-file: revise merge error reporting

On Mon, Mar 25, 2013 at 1:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Subject: [PATCH] merge-one-file: force content conflict for "both side added" case
s/both side/both sides/
> Historically, we tried to be lenient to "both side added, slightly
Ditto.
Show 9 quoted lines
> differently" case and as long as the files can be merged using a
> made-up common ancestor cleanly, since f7d24bbefb06 (merge with
> /dev/null as base, instead of punting O==empty case, 2005-11-07).
> This was later further refined to use a better made-up common file
> with fd66dbf5297a (merge-one-file: use empty- or common-base
> condintionally in two-stage merge., 2005-11-10), but the spirit has
> been the same.
>
> But the original fix in f7d24bbefb06 to avoid punging on "both sides
s/punging/punting/
Show 6 quoted lines
> added" case had a code to unconditionally error out the merge.  When
> this triggers, even though the content-level merge can be done
> cleanly, we end up not saying "content conflict" in the message, but
> still issue the error message, showing "ERROR:  in <pathname>".
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano· Mar 13, 2013, 17:57 UTC · re: Kevin Bracey · lore

Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting

Kevin Bracey <kevin@bracey.fi> writes:
Show 29 quoted lines
> Commit 718135e improved the merge error reporting for the resolve
> strategy's merge conflict and permission conflict cases, but led to a
> malformed "ERROR:  in myfile.c" message in the case of a file added
> differently.
>
> This commit reverts that change, and uses an alternative approach without
> this flaw.
>
> Signed-off-by: Kevin Bracey <kevin@bracey.fi>
> ---
>  git-merge-one-file.sh | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
> index 0f164e5..78b07a8 100755
> --- a/git-merge-one-file.sh
> +++ b/git-merge-one-file.sh
> @@ -104,11 +104,13 @@ case "${1:-.}${2:-.}${3:-.}" in
>  		;;
>  	esac
>  
> +	ret=0
>  	src1=$(git-unpack-file $2)
>  	src2=$(git-unpack-file $3)
>  	case "$1" in
>  	'')
> -		echo "Added $4 in both, but differently."
> +		echo "ERROR: Added $4 in both, but differently."
> +		ret=1

The problem you identified may be worth fixing, but I do not think this change is correct.

This message is at the same severity level as the message on the other arm of this case that says "Auto-merging $4". In that other case arm, we are attempting a true three-way merge, and in this case arm, we are attempting a similar three-way merge using your "virtual base".

Neither has found any error in this case arm yet. The messages are both "informational", not an error. I do not think you would want to set ret=1 until you see content conflict.

Kevin Bracey· Mar 14, 2013, 06:27 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting

On 13/03/2013 19:57, Junio C Hamano wrote:
Show 17 quoted lines
> Kevin Bracey <kevin@bracey.fi> writes:
>
>> -		echo "Added $4 in both, but differently."
>> +		echo "ERROR: Added $4 in both, but differently."
>> +		ret=1
> The problem you identified may be worth fixing, but I do not think
> this change is correct.
>
> This message is at the same severity level as the message on the
> other arm of this case that says "Auto-merging $4".  In that other
> case arm, we are attempting a true three-way merge, and in this case
> arm, we are attempting a similar three-way merge using your "virtual
> base".
>
> Neither has found any error in this case arm yet.  The messages are
> both "informational", not an error.  I do not think you would want
> to set ret=1 until you see content conflict.

I disagree here. At the minute, it does set ret to 1 (but further down the code - bringing it up here next to the "ERROR" print clarifies that), and will report the merge as failed, conflict in the 3-way merge or not. Which I think is correct.

We have to stop for user inspection here. We do have a fake base; we can't trust the 3-way merge with it.

The virtual 3-way merge will take ABCDE and ABDE and produce ABCDE without conflict. That's flat wrong if the real base they failed to tell Git about was ABCDE.

Despite being useful, I'm still slightly uncomfortable that it can produce something without any conflict markers. The user really needs to look at properly.

(And one interesting related glitch, or at least thing that puzzled me when it happened. This is from memory, so may be slightly mistaken, but what seemed to happen was that if you have rerere enabled, then mergetool tends to say "nothing to merge", because it relies on "rerere remaining", which relies on conflict markers. I think you could still force a mergetool up by specifying the specific file though.)

Maybe the virtual base itself should be different. Maybe it should put a ??????? marker in place of every unique line. So you get:

Left ABCEFGH Right XABCDEFJH -> Merge result <|X>ABC<|D>EF<G|J>H VBase ?ABC?EF??H

That actually feels like it may be the correct answer here. And it's effectively what P4Merge does in its "2-way" mode I failed to invoke. (At least for the result view).

Kevin
Junio C Hamano· Mar 14, 2013, 14:56 UTC · re: Kevin Bracey · lore

Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting

Kevin Bracey <kevin@bracey.fi> writes:
> I disagree here. At the minute, it does set ret to 1 (but further down
> the code - bringing it up here next to the "ERROR" print clarifies
> that), and will report the merge as failed, conflict in the 3-way
> merge or not. Which I think is correct.

OK. I agree that forcing users to always inspect the result of "both side added" resolution sounds like a good safety measure.

Show 8 quoted lines
> Maybe the virtual base itself should be different. Maybe it should put
> a ??????? marker in place of every unique line. So you get:
>
> Left   ABCEFGH
> Right XABCDEFJH  -> Merge result <|X>ABC<|D>EF<G|J>H
> VBase ?ABC?EF??H
>
> That actually feels like it may be the correct answer here.

Interesting, though the approach has downsides with the diff3 conflict style, no?

Kevin Bracey· Mar 14, 2013, 17:31 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting

On 14/03/2013 16:56, Junio C Hamano wrote:
Show 13 quoted lines
> Kevin Bracey <kevin@bracey.fi> writes:
>
>> Maybe the virtual base itself should be different. Maybe it should put
>> a ??????? marker in place of every unique line. So you get:
>>
>> Left   ABCEFGH
>> Right XABCDEFJH  -> Merge result <|X>ABC<|D>EF<G|J>H
>> VBase ?ABC?EF??H
>>
>> That actually feels like it may be the correct answer here.
> Interesting, though the approach has downsides with the diff3
> conflict style, no?
>

Well, yes, but I would assume that we would forcibly select normal diff here somehow, if we aren't already. We should be - turning ABCDEFGH vs ABCD into ABCD<EFGH|EFGH=> is silly.

This topic has a lot in common with the zdiff3 discussion going on. The concern there is about large chunks of similar code appearing on two sides, and not being in the base, leading to useless diff3.

This is just the special case of the base being totally empty.

The thought on zdiff3 philosophy was that common additions should be treated as resolved, and not appear inside conflict markers. That's exactly what we'd be doing. So, same conflict as above, but this time embedded in a larger file, using zdiff3 logic:

Left aaaaaabaacaaABCEFGHeee Base aaaaaaaaaaaaeee -> zdiff3 aaada<b|a=f>aacaaABC<|D>EF<G|J>Heee Right aaadaafaaaaaABCDEFJHeee

Note that I've chosen to suppress the = marker if the lines surrounding the conflict are not in the base. I think that helps highlight the fact that we're in a diff2 section. EF<G|=J>H reads like an assertion that the base has EFH. Whereas EF<G|J>H avoids that.

So, anyway, commonality with zdiff3 would be good. Even if we can't share code, we should at least share the general style of result.

Kevin
Kevin Bracey· Mar 14, 2013, 17:39 UTC · re: Kevin Bracey · lore

Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting

On 14/03/2013 19:31, Kevin Bracey wrote:
Show 5 quoted lines
> On 14/03/2013 16:56, Junio C Hamano wrote:
>>
> Well, yes, but I would assume that we would forcibly select normal 
> diff here somehow, if we aren't already. We should be - turning 
> ABCDEFGH vs ABCD into ABCD<EFGH|EFGH=> is silly.

Doh. But anyway, we don't want to waste space with |= markers, and make the same "surrounding code is in the base" suggestion. So we should be selecting diff.

Kevin
David Aguilar· Mar 13, 2013, 02:05 UTC · re: Kevin Bracey · lore

Re: [PATCH v3 1/3] mergetools/p4merge: swap LOCAL and REMOTE

On Tue, Mar 12, 2013 at 6:12 PM, Kevin Bracey <kevin@bracey.fi> wrote:
Show 33 quoted lines
> Reverse LOCAL and REMOTE when invoking P4Merge as a mergetool, so that
> the incoming branch is now in the left-hand, blue triangle pane, and the
> current branch is in the right-hand, green circle pane.
>
> This change makes use of P4Merge consistent with its built-in help, its
> reference documentation, and Perforce itself. But most importantly, it
> makes merge results clearer. P4Merge is not totally symmetrical between
> left and right; despite changing a few text labels from "theirs/ours" to
> "left/right" when invoked manually, it still retains its original
> Perforce "theirs/ours" viewpoint.
>
> Most obviously, in the result pane P4Merge shows changes that are common
> to both branches in green. This is on the basis of the current branch
> being green, as it is when invoked from Perforce; it means that lines in
> the result are blue if and only if they are being changed by the merge,
> making the resulting diff clearer.
>
> Note that P4Merge now shows "ours" on the right for both diff and merge,
> unlike other diff/mergetools, which always have REMOTE on the right.
> But observe that REMOTE is the working tree (ie "ours") for a diff,
> while it's another branch (ie "theirs") for a merge.
>
> Ours and theirs are reversed for a rebase - see "git help rebase".
> However, this does produce the desired "show the results of this commit"
> effect in P4Merge - changes that remain in the rebased commit (in your
> branch, but not in the new base) appear in blue; changes that do not
> appear in the rebased commit (from the new base, or common to both) are
> in green. If Perforce had rebase, they'd probably not swap ours/theirs,
> but make P4Merge show common changes in blue, picking out our changes in
> green. We can't do that, so this is next best.
>
> Signed-off-by: Kevin Bracey <kevin@bracey.fi>
> ---

This seems sensible to apply. The commit message is a bit long, but I think it's justified since this is exactly the kind of thing I would tend to forget after enough time has passed.

Ditto on the create_virtual_base patch. Your latest patch addressed Junio's note about making it take 2 args.

FWIW, please feel free to add:
Reviewed-by: David Aguilar <davvid@gmail.com>
Thanks.
Show 19 quoted lines
>  mergetools/p4merge | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mergetools/p4merge b/mergetools/p4merge
> index 8a36916..46b3a5a 100644
> --- a/mergetools/p4merge
> +++ b/mergetools/p4merge
> @@ -22,7 +22,7 @@ diff_cmd () {
>  merge_cmd () {
>         touch "$BACKUP"
>         $base_present || >"$BASE"
> -       "$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
> +       "$merge_tool_path" "$BASE" "$REMOTE" "$LOCAL" "$MERGED"
>         check_unchanged
>  }
>
> --
> 1.8.2.rc3.7.g1100d09.dirty
>
-- 
David
Kevin Bracey· Mar 24, 2013, 11:54 UTC · re: Kevin Bracey · lore

[PATCH v4 1/2] mergetools/p4merge: swap LOCAL and REMOTE

Reverse LOCAL and REMOTE when invoking P4Merge as a mergetool, so that the incoming branch is now in the left-hand, blue triangle pane, and the current branch is in the right-hand, green circle pane.

This change makes use of P4Merge consistent with its built-in help, its reference documentation, and Perforce itself. But most importantly, it makes merge results clearer. P4Merge is not totally symmetrical between left and right; despite changing a few text labels from "theirs/ours" to "left/right" when invoked manually, it still retains its original Perforce "theirs/ours" viewpoint.

Most obviously, in the result pane P4Merge shows changes that are common to both branches in green. This is on the basis of the current branch being green, as it is when invoked from Perforce; it means that lines in the result are blue if and only if they are being changed by the merge, making the resulting diff clearer.

Note that P4Merge now shows "ours" on the right for both diff and merge, unlike other diff/mergetools, which always have REMOTE on the right. But observe that REMOTE is the working tree (ie "ours") for a diff, while it's another branch (ie "theirs") for a merge.

Ours and theirs are reversed for a rebase - see "git help rebase". However, this does produce the desired "show the results of this commit" effect in P4Merge - changes that remain in the rebased commit (in your branch, but not in the new base) appear in blue; changes that do not appear in the rebased commit (from the new base, or common to both) are in green. If Perforce had rebase, they'd probably not swap ours/theirs, but make P4Merge show common changes in blue, picking out our changes in green. We can't do that, so this is next best.

Signed-off-by: Kevin Bracey <kevin@bracey.fi>
Reviewed-by: David Aguilar <davvid@gmail.com>
---
No change to part 1/2 from previous version, apart from Reviewed-by.
Part 2/2 is modified.
 mergetools/p4merge | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to mergetools/p4merge +1 −1
diff --git a/mergetools/p4merge b/mergetools/p4merge
index 8a36916..46b3a5a 100644
--- a/mergetools/p4merge
+++ b/mergetools/p4merge
@@ -22,7 +22,7 @@ diff_cmd () {
 merge_cmd () {
 	touch "$BACKUP"
 	$base_present || >"$BASE"
-	"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
+	"$merge_tool_path" "$BASE" "$REMOTE" "$LOCAL" "$MERGED"
 	check_unchanged
 }
 
-- 
1.8.2.rc3.21.g744ac65
Kevin Bracey· Mar 24, 2013, 11:54 UTC · re: Kevin Bracey · lore

[PATCH v4 2/2] mergetools/p4merge: create a base if none available

Originally, with no base, Git gave P4Merge $LOCAL as a dummy base:
   p4merge "$LOCAL" "$LOCAL" "$REMOTE" "$MERGED"
Commit 0a0ec7bd changed this to:
   p4merge "empty file" "$LOCAL" "$REMOTE" "$MERGED"

to avoid the problem of being unable to save in some circumstances with similar inputs.

Unfortunately this approach produces much worse results on differing inputs. P4Merge really regards the blank file as the base, and once you have just a couple of differences between the two branches you end up with one a massive full-file conflict. The 3-way diff is not readable, and you have to invoke "difftool MERGE_HEAD HEAD" manually to get a useful view.

The original approach appears to have invoked special 2-way merge behaviour in P4Merge that occurs only if the base filename is "" or equal to the left input. You get a good visual comparison, and it does not auto-resolve differences. (Normally if one branch matched the base, it would autoresolve to the other branch).

But there appears to be no way of getting this 2-way behaviour and being able to reliably save. Having base==left appears to be triggering other assumptions. There are tricks the user can use to force the save icon on, but it's not intuitive.

So we now follow a suggestion given in the original patch's discussion: generate a virtual base, consisting of the lines common to the two branches. This is the same as the technique used in resolve and octopus merges, so we relocate that code to a shared function.

Note that if there are no differences at the same location, this technique can lead to automatic resolution without conflict, combining everything from the 2 files. As with the other merges using this technique, we assume the user will inspect the result before saving.

Signed-off-by: Kevin Bracey <kevin@bracey.fi>
Reviewed-by: David Aguilar <davvid@gmail.com>
---
Minor change from v3: that version moved initialisation of src1 higher up,
detaching it from its associated comment. This move was only required by
earlier versions, so v4 leaves src1 in its original position.
Added Reviewed-by footer.
 Documentation/git-sh-setup.txt |  6 ++++++
 git-merge-one-file.sh          | 18 +++++-------------
 git-sh-setup.sh                | 12 ++++++++++++
 mergetools/p4merge             |  6 +++++-
 4 files changed, 28 insertions(+), 14 deletions(-)
Show changes to 4 files +28 −14

Documentation/git-sh-setup.txt, git-merge-one-file.sh, git-sh-setup.sh, mergetools/p4merge

diff --git a/Documentation/git-sh-setup.txt b/Documentation/git-sh-setup.txt
index 6a9f66d..5d709d0 100644
--- a/Documentation/git-sh-setup.txt
+++ b/Documentation/git-sh-setup.txt
@@ -82,6 +82,12 @@ get_author_ident_from_commit::
 	outputs code for use with eval to set the GIT_AUTHOR_NAME,
 	GIT_AUTHOR_EMAIL and GIT_AUTHOR_DATE variables for a given commit.
 
+create_virtual_base::
+	modifies the first file so only lines in common with the
+	second file remain. If there is insufficient common material,
+	then the first file is left empty. The result is suitable
+	as a virtual base input for a 3-way merge.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 3373c04..255c07a 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -104,30 +104,22 @@ case "${1:-.}${2:-.}${3:-.}" in
 		;;
 	esac
 
-	src2=`git-unpack-file $3`
+	src2=$(git-unpack-file $3)
 	case "$1" in
 	'')
 		echo "Added $4 in both, but differently."
-		# This extracts OUR file in $orig, and uses git apply to
-		# remove lines that are unique to ours.
-		orig=`git-unpack-file $2`
-		sz0=`wc -c <"$orig"`
-		@@DIFF@@ -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
-		sz1=`wc -c <"$orig"`
-
-		# If we do not have enough common material, it is not
-		# worth trying two-file merge using common subsections.
-		expr $sz0 \< $sz1 \* 2 >/dev/null || : >$orig
+		orig=$(git-unpack-file $2)
+		create_virtual_base "$orig" "$src2"
 		;;
 	*)
 		echo "Auto-merging $4"
-		orig=`git-unpack-file $1`
+		orig=$(git-unpack-file $1)
 		;;
 	esac
 
 	# Be careful for funny filename such as "-L" in "$4", which
 	# would confuse "merge" greatly.
-	src1=`git-unpack-file $2`
+	src1=$(git-unpack-file $2)
 	git merge-file "$src1" "$orig" "$src2"
 	ret=$?
 	msg=
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 9cfbe7f..2f78359 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -249,6 +249,18 @@ clear_local_git_env() {
 	unset $(git rev-parse --local-env-vars)
 }
 
+# Generate a virtual base file for a two-file merge. Uses git apply to
+# remove lines from $1 that are not in $2, leaving only common lines.
+create_virtual_base() {
+	sz0=$(wc -c <"$1")
+	@@DIFF@@ -u -La/"$1" -Lb/"$1" "$1" "$2" | git apply --no-add
+	sz1=$(wc -c <"$1")
+
+	# If we do not have enough common material, it is not
+	# worth trying two-file merge using common subsections.
+	expr $sz0 \< $sz1 \* 2 >/dev/null || : >"$1"
+}
+
 
 # Platform specific tweaks to work around some commands
 case $(uname -s) in
diff --git a/mergetools/p4merge b/mergetools/p4merge
index 46b3a5a..5a608ab 100644
--- a/mergetools/p4merge
+++ b/mergetools/p4merge
@@ -21,7 +21,11 @@ diff_cmd () {
 
 merge_cmd () {
 	touch "$BACKUP"
-	$base_present || >"$BASE"
+	if ! $base_present
+	then
+		cp -- "$LOCAL" "$BASE"
+		create_virtual_base "$BASE" "$REMOTE"
+	fi
 	"$merge_tool_path" "$BASE" "$REMOTE" "$LOCAL" "$MERGED"
 	check_unchanged
 }
-- 
1.8.2.rc3.21.g744ac65
Junio C Hamano· Mar 25, 2013, 17:47 UTC · re: Kevin Bracey · lore

Re: [PATCH v4 2/2] mergetools/p4merge: create a base if none available

Kevin Bracey <kevin@bracey.fi> writes:
> Minor change from v3: that version moved initialisation of src1 higher up,
> detaching it from its associated comment. This move was only required by
> earlier versions, so v4 leaves src1 in its original position.

The "funny filename" comment was from b539c5e8fbd3 (git-merge-one: new merge world order., 2005-12-07) where the removed code just before that new comment ended with:

	merge "$4" "$orig" "$src2"

(yes, we used to use "merge" program from the RCS suite). The comment refers to one of the bad side effect the old code used to have and warns against such a practice, i.e. it was talking about the code that no longer existed.

I think the two-line comment should simply go.

Given that, I _think_ it is OK to move the initialization of src1 next to that of src2; that may make the result easier to read.

Thanks.

← back to recent threads