[PATCH 0/2] Improve P4Merge mergetool invocation

[PATCH 0/2] Improve P4Merge mergetool invocation

[Kevin Bracey]

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

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

[Kevin Bracey]

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

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

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

[Junio C Hamano]

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.

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

[Kevin Bracey]

On 07/03/2013 02:36, Junio C Hamano wrote:
> 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

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

[Junio C Hamano]

Kevin Bracey <kevin@bracey.fi> writes:

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

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

[Kevin Bracey]

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

> 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

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

[Junio C Hamano]

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

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

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

[David Aguilar]

On Thu, Mar 7, 2013 at 11:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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).


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

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

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

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

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

[Kevin Bracey]

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

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

[David Aguilar]

On Wed, Mar 6, 2013 at 12:32 PM, Kevin Bracey <kevin@bracey.fi> wrote:
> 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?

> +               # 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

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

[Kevin Bracey]

On 07/03/2013 04:23, David Aguilar wrote:
> 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.

> 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

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

[Junio C Hamano]

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.

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

[David Aguilar]

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

[PATCH v2 0/3] Improve P4Merge mergetool invocation

[Kevin Bracey]

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

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

[Kevin Bracey]

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

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

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

[Kevin Bracey]

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

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

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

[Junio C Hamano]

Kevin Bracey <kevin@bracey.fi> writes:

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

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

[Kevin Bracey]

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

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

[Kevin Bracey]

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

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

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

[Kevin Bracey]

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

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

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

[Kevin Bracey]

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

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

[David Aguilar]

On Tue, Mar 12, 2013 at 6:12 PM, Kevin Bracey <kevin@bracey.fi> wrote:
> 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 ;-)


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

>         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

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

[Kevin Bracey]

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

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

[Kevin Bracey]

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

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

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

[Kevin Bracey]

Signed-off-by: Kevin Bracey <kevin@bracey.fi>
---
 git-merge-one-file.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

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

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

[Kevin Bracey]

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

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

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

[Junio C Hamano]

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

 git-merge-one-file.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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

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

[Junio C Hamano]

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

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

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

[Eric Sunshine]

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.

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

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

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

[Junio C Hamano]

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

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

[Kevin Bracey]

On 13/03/2013 19:57, Junio C Hamano wrote:
> 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

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

[Junio C Hamano]

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.

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

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

[Kevin Bracey]

On 14/03/2013 16:56, Junio C Hamano wrote:
> 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

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

[Kevin Bracey]

On 14/03/2013 19:31, Kevin Bracey wrote:
> 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

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

[David Aguilar]

On Tue, Mar 12, 2013 at 6:12 PM, Kevin Bracey <kevin@bracey.fi> wrote:
> 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.

>  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

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

[Kevin Bracey]

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

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

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

[Kevin Bracey]

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

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

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

[Junio C Hamano]

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.