Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting
On 13/03/2013 19:57, Junio C Hamano wrote: Kevin Bracey ke...@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 |XABC|DEFG|JH 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting
Kevin Bracey ke...@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 |XABC|DEFG|JH 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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting
On 14/03/2013 16:56, Junio C Hamano wrote: Kevin Bracey ke...@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 |XABC|DEFG|JH 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 ABCDEFGH|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: LeftaabaacaaABCEFGHeee Baseeee - zdiff3 aaadab|a=faacaaABC|DEFG|JHeee Right aaadaafaABCDEFJHeee 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. EFG|=JH reads like an assertion that the base has EFH. Whereas EFG|JH 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting
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 ABCDEFGH|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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting
On Tue, Mar 12, 2013 at 6:12 PM, Kevin Bracey ke...@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 ke...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] git-merge-one-file: revise merge error reporting
Kevin Bracey ke...@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 ke...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/3] git-merge-one-file: revise merge error reporting
Commit 718135e improved the merge error reporting for the resolve
strategy's merge conflict and permission conflict cases, but led to a
malformed ERROR: in myfile.c message in the case of a file added
differently.
This commit reverts that change, and uses an alternative approach without
this flaw.
Signed-off-by: Kevin Bracey ke...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
