[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

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

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

On Wed, Mar 6, 2013 at 12:32 PM, Kevin Bracey [off-list ref] wrote:

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?

-- 
David

On Wed, Mar 6, 2013 at 12:32 PM, Kevin Bracey [off-list ref] 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

On 07/03/2013 04:23, David Aguilar wrote:

On Wed, Mar 6, 2013 at 12:32 PM, Kevin Bracey [off-list ref] wrote:

quoted

+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

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

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

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

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

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

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

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

On Tue, Mar 12, 2013 at 6:12 PM, Kevin Bracey [off-list ref] 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 <redacted>
---

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

Thanks.

-- 
David

On Tue, Mar 12, 2013 at 6:12 PM, Kevin Bracey [off-list ref] 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 <redacted>
---

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

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.

same notes as above.  I think a style patch should come first.
-- 
David

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

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 <redacted>
Reviewed-by: David Aguilar <redacted>
---
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

Update style to match Documentation/CodingGuidelines.

Signed-off-by: Kevin Bracey <redacted>
---
 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

Signed-off-by: Kevin Bracey <redacted>
---
 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

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

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 <redacted>
Reviewed-by: David Aguilar <redacted>
---
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