Re: [PATCH 3/3] checkout -m: attempt merge when deletion of path was staged
Jonathan Nieder jrnie...@gmail.com writes:
twoway_merge() is missing an o-gently check in the case where a file
that needs to be modified is missing from the index but present in the
old and new trees. As a result, in this case 'git checkout -m' errors
out instead of trying to perform a merge.
I see two hunks in threeway_merge(), so two existing callers there
will not change their behaviour. Two hunks in twoway_merge() means
that among three existing callers in that function, this one at the
end (not shown in your patch) changes its behaviour:
else if (newtree) {
if (oldtree !o-initial_checkout) {
/*
* deletion of the path was staged;
*/
if (same(oldtree, newtree))
return 1;
return reject_merge(oldtree, o);
}
return merged_entry(newtree, current, o);
}
return deleted_entry(oldtree, current, o);
This is the most iffy of the three patches, mostly because I was too
lazy to write a test.
You would trigger this codepath by jumping from an old revision to a
new revision after git rm $path any path that has been modified
between the two. The only behaviour difference is that it will stop
issuing an error message---the checkout -m will successfully switch
between the revs and leave the index in a we modified, they removed
conflicting state with or without your patch.
--
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 3/3] checkout -m: attempt merge when deletion of path was staged
Junio C Hamano gits...@pobox.com writes:
Jonathan Nieder jrnie...@gmail.com writes:
twoway_merge() is missing an o-gently check in the case where a file
that needs to be modified is missing from the index but present in the
old and new trees. As a result, in this case 'git checkout -m' errors
out instead of trying to perform a merge.
I see two hunks in threeway_merge(), so two existing callers there
will not change their behaviour. Two hunks in twoway_merge() means
that among three existing callers in that function, this one at the
end (not shown in your patch) changes its behaviour:
else if (newtree) {
if (oldtree !o-initial_checkout) {
/*
* deletion of the path was staged;
*/
if (same(oldtree, newtree))
return 1;
return reject_merge(oldtree, o);
}
return merged_entry(newtree, current, o);
}
return deleted_entry(oldtree, current, o);
This is the most iffy of the three patches, mostly because I was too
lazy to write a test.
You would trigger this codepath by jumping from an old revision to a
new revision after git rm $path any path that has been modified
between the two. The only behaviour difference is that it will stop
issuing an error message---the checkout -m will successfully switch
between the revs and leave the index in a we modified, they removed
conflicting state with or without your patch.
IOW, something like this perhaps?
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 0c9ec0a..cedbb6a 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -223,6 +223,23 @@ test_expect_success 'checkout --merge --conflict=diff3
branch' '
test_cmp two expect
'
+test_expect_success 'switch to another branch while carrying a deletion' '
+
+ git checkout -f master git reset --hard git clean -f
+ git rm two
+
+ test_must_fail git checkout simple 2errs
+ test_i18ngrep overwritten errs
+
+ git checkout --merge simple 2errs
+ ! test_i18ngrep overwritten errs
+ git ls-files -u
+ test_must_fail git cat-file -t :0:two
+ test $(git cat-file -t :1:two) = blob
+ test $(git cat-file -t :2:two) = blob
+ test_must_fail git cat-file -t :3:two
+'
+
test_expect_success 'checkout to detach HEAD (with advice declined)' '
git config advice.detachedHead false
--
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 3/3] checkout -m: attempt merge when deletion of path was staged
Am 13.08.2014 20:59, schrieb Junio C Hamano: diff --git a/t/t7201-co.sh b/t/t7201-co.sh index 0c9ec0a..cedbb6a 100755 --- a/t/t7201-co.sh +++ b/t/t7201-co.sh @@ -223,6 +223,23 @@ test_expect_success 'checkout --merge --conflict=diff3 branch' ' test_cmp two expect ' +test_expect_success 'switch to another branch while carrying a deletion' ' + + git checkout -f master git reset --hard git clean -f + git rm two + + test_must_fail git checkout simple 2errs + test_i18ngrep overwritten errs + + git checkout --merge simple 2errs + ! test_i18ngrep overwritten errs This must be written as test_i18ngrep ! overwritten errs + git ls-files -u + test_must_fail git cat-file -t :0:two + test $(git cat-file -t :1:two) = blob + test $(git cat-file -t :2:two) = blob + test_must_fail git cat-file -t :3:two +' + test_expect_success 'checkout to detach HEAD (with advice declined)' ' git config advice.detachedHead false I see a few wrong usages in the current code base. Here's a fix. --- 8 --- Subject: [PATCH] tests: fix negated test_i18ngrep calls The helper function test_i18ngrep pretends that it found the expected results when it is running under GETTEXT_POISON. For this reason, it must not be used negated like so ! test_i18ngrep foo bar because the test case would fail under GETTEXT_POISON. The function offers a special syntax to test that a pattern is *not* found: test_i18ngrep ! foo bar Convert incorrect uses to this syntax. Signed-off-by: Johannes Sixt j...@kdbg.org --- t/t4018-diff-funcname.sh | 8 t/t9800-git-p4-basic.sh | 2 +- t/t9807-git-p4-submit.sh | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh index 34591c2..1dbaa38 100755 --- a/t/t4018-diff-funcname.sh +++ b/t/t4018-diff-funcname.sh @@ -52,15 +52,15 @@ do echo *.java diff=$p .gitattributes test_expect_code 1 git diff --no-index \ A.java B.java 2msg - ! test_i18ngrep fatal msg - ! test_i18ngrep error msg + test_i18ngrep ! fatal msg + test_i18ngrep ! error msg ' test_expect_success builtin $p wordRegex pattern compiles ' echo *.java diff=$p .gitattributes test_expect_code 1 git diff --no-index --word-diff \ A.java B.java 2msg - ! test_i18ngrep fatal msg - ! test_i18ngrep error msg + test_i18ngrep ! fatal msg + test_i18ngrep ! error msg ' done diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh index 665607c..5b56212 100755 --- a/t/t9800-git-p4-basic.sh +++ b/t/t9800-git-p4-basic.sh @@ -145,7 +145,7 @@ test_expect_success 'exit when p4 fails to produce marshaled output' ' test_expect_code 1 git p4 clone --dest=$git //depot errs 21 ) cat errs - ! test_i18ngrep Traceback errs + test_i18ngrep ! Traceback errs ' # Hide a file from p4d, make sure we catch its complaint. This won't fail in diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh index 7fab2ed..1f74a88 100755 --- a/t/t9807-git-p4-submit.sh +++ b/t/t9807-git-p4-submit.sh @@ -404,7 +404,7 @@ test_expect_success 'submit --prepare-p4-only' ' git p4 submit --prepare-p4-only out test_i18ngrep prepared for submission out test_i18ngrep must be deleted out - ! test_i18ngrep everything below this line is just the diff out + test_i18ngrep ! everything below this line is just the diff out ) ( cd $cli -- 2.0.0.12.gbcf935e -- 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 3/3] checkout -m: attempt merge when deletion of path was staged
Johannes Sixt j...@kdbg.org writes: Am 13.08.2014 20:59, schrieb Junio C Hamano: diff --git a/t/t7201-co.sh b/t/t7201-co.sh index 0c9ec0a..cedbb6a 100755 --- a/t/t7201-co.sh +++ b/t/t7201-co.sh @@ -223,6 +223,23 @@ test_expect_success 'checkout --merge --conflict=diff3 branch' ' test_cmp two expect ' +test_expect_success 'switch to another branch while carrying a deletion' ' + +git checkout -f master git reset --hard git clean -f +git rm two + +test_must_fail git checkout simple 2errs +test_i18ngrep overwritten errs + +git checkout --merge simple 2errs +! test_i18ngrep overwritten errs This must be written as test_i18ngrep ! overwritten errs Oops. Thanks for spotting. I see a few wrong usages in the current code base. Here's a fix. Will apply; thanks. --- 8 --- Subject: [PATCH] tests: fix negated test_i18ngrep calls The helper function test_i18ngrep pretends that it found the expected results when it is running under GETTEXT_POISON. For this reason, it must not be used negated like so ! test_i18ngrep foo bar because the test case would fail under GETTEXT_POISON. The function offers a special syntax to test that a pattern is *not* found: test_i18ngrep ! foo bar Convert incorrect uses to this syntax. Signed-off-by: Johannes Sixt j...@kdbg.org --- t/t4018-diff-funcname.sh | 8 t/t9800-git-p4-basic.sh | 2 +- t/t9807-git-p4-submit.sh | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh index 34591c2..1dbaa38 100755 --- a/t/t4018-diff-funcname.sh +++ b/t/t4018-diff-funcname.sh @@ -52,15 +52,15 @@ do echo *.java diff=$p .gitattributes test_expect_code 1 git diff --no-index \ A.java B.java 2msg - ! test_i18ngrep fatal msg - ! test_i18ngrep error msg + test_i18ngrep ! fatal msg + test_i18ngrep ! error msg ' test_expect_success builtin $p wordRegex pattern compiles ' echo *.java diff=$p .gitattributes test_expect_code 1 git diff --no-index --word-diff \ A.java B.java 2msg - ! test_i18ngrep fatal msg - ! test_i18ngrep error msg + test_i18ngrep ! fatal msg + test_i18ngrep ! error msg ' done diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh index 665607c..5b56212 100755 --- a/t/t9800-git-p4-basic.sh +++ b/t/t9800-git-p4-basic.sh @@ -145,7 +145,7 @@ test_expect_success 'exit when p4 fails to produce marshaled output' ' test_expect_code 1 git p4 clone --dest=$git //depot errs 21 ) cat errs - ! test_i18ngrep Traceback errs + test_i18ngrep ! Traceback errs ' # Hide a file from p4d, make sure we catch its complaint. This won't fail in diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh index 7fab2ed..1f74a88 100755 --- a/t/t9807-git-p4-submit.sh +++ b/t/t9807-git-p4-submit.sh @@ -404,7 +404,7 @@ test_expect_success 'submit --prepare-p4-only' ' git p4 submit --prepare-p4-only out test_i18ngrep prepared for submission out test_i18ngrep must be deleted out - ! test_i18ngrep everything below this line is just the diff out + test_i18ngrep ! everything below this line is just the diff out ) ( cd $cli -- 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 3/3] checkout -m: attempt merge when deletion of path was staged
twoway_merge() is missing an o-gently check in the case where a file
that needs to be modified is missing from the index but present in the
old and new trees. As a result, in this case 'git checkout -m' errors
out instead of trying to perform a merge.
Fix it by checking o-gently. While at it, inline the o-gently check
into reject_merge to prevent future call sites from making the same
mistake.
Noticed by code inspection. The motivating case hasn't been tested.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
This is the most iffy of the three patches, mostly because I was too
lazy to write a test. I believe it's safe as-is nonetheless.
Thanks for reading.
unpack-trees.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 187b15b..6c45af7 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1178,7 +1178,8 @@ return_failed:
static int reject_merge(const struct cache_entry *ce,
struct unpack_trees_options *o)
{
- return add_rejected_path(o, ERROR_WOULD_OVERWRITE, ce-name);
+ return o-gently ? -1 :
+ add_rejected_path(o, ERROR_WOULD_OVERWRITE, ce-name);
}
static int same(const struct cache_entry *a, const struct cache_entry *b)
@@ -1633,7 +1634,7 @@ int threeway_merge(const struct cache_entry * const
*stages,
/* #14, #14ALT, #2ALT */
if (remote !df_conflict_head head_match !remote_match) {
if (index !same(index, remote) !same(index, head))
- return o-gently ? -1 : reject_merge(index, o);
+ return reject_merge(index, o);
return merged_entry(remote, index, o);
}
/*
@@ -1641,7 +1642,7 @@ int threeway_merge(const struct cache_entry * const
*stages,
* make sure that it matches head.
*/
if (index !same(index, head))
- return o-gently ? -1 : reject_merge(index, o);
+ return reject_merge(index, o);
if (head) {
/* #5ALT, #15 */
@@ -1770,7 +1771,7 @@ int twoway_merge(const struct cache_entry * const *src,
else
return merged_entry(newtree, current,
o);
}
- return o-gently ? -1 : reject_merge(current, o);
+ return reject_merge(current, o);
} else if ((!oldtree !newtree) || /* 4 and 5 */
(!oldtree newtree
same(current, newtree)) || /* 6 and 7 */
@@ -1788,7 +1789,7 @@ int twoway_merge(const struct cache_entry * const *src,
/* 20 or 21 */
return merged_entry(newtree, current, o);
} else
- return o-gently ? -1 : reject_merge(current, o);
+ return reject_merge(current, o);
}
else if (newtree) {
if (oldtree !o-initial_checkout) {
--
--
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 3/3] checkout -m: attempt merge when deletion of path was staged
On Tue, Aug 12, 2014 at 5:03 PM, Jonathan Nieder jrnie...@gmail.com wrote:
twoway_merge() is missing an o-gently check in the case where a file
that needs to be modified is missing from the index but present in the
old and new trees. As a result, in this case 'git checkout -m' errors
out instead of trying to perform a merge.
Fix it by checking o-gently. While at it, inline the o-gently check
into reject_merge to prevent future call sites from making the same
mistake.
Noticed by code inspection. The motivating case hasn't been tested.
That sounds sloppy X-. I may comment more after figuring out
what _other_ reject_merge() caller that does not appear in the
patch would change its behaviour with this patch.
side note: of course, if this were two patches, one that adds the
same o-gently ? -1 : reject thing to places where they forget to
do so, and the other that moves the gently thing to reject helper,
then we can read all the necessary information to judge the change
in the patch ;-)
Thanks.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
This is the most iffy of the three patches, mostly because I was too
lazy to write a test. I believe it's safe as-is nonetheless.
Thanks for reading.
unpack-trees.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 187b15b..6c45af7 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1178,7 +1178,8 @@ return_failed:
static int reject_merge(const struct cache_entry *ce,
struct unpack_trees_options *o)
{
- return add_rejected_path(o, ERROR_WOULD_OVERWRITE, ce-name);
+ return o-gently ? -1 :
+ add_rejected_path(o, ERROR_WOULD_OVERWRITE, ce-name);
}
static int same(const struct cache_entry *a, const struct cache_entry *b)
@@ -1633,7 +1634,7 @@ int threeway_merge(const struct cache_entry * const
*stages,
/* #14, #14ALT, #2ALT */
if (remote !df_conflict_head head_match !remote_match) {
if (index !same(index, remote) !same(index, head))
- return o-gently ? -1 : reject_merge(index, o);
+ return reject_merge(index, o);
return merged_entry(remote, index, o);
}
/*
@@ -1641,7 +1642,7 @@ int threeway_merge(const struct cache_entry * const
*stages,
* make sure that it matches head.
*/
if (index !same(index, head))
- return o-gently ? -1 : reject_merge(index, o);
+ return reject_merge(index, o);
if (head) {
/* #5ALT, #15 */
@@ -1770,7 +1771,7 @@ int twoway_merge(const struct cache_entry * const *src,
else
return merged_entry(newtree, current,
o);
}
- return o-gently ? -1 : reject_merge(current, o);
+ return reject_merge(current, o);
} else if ((!oldtree !newtree) || /* 4 and 5 */
(!oldtree newtree
same(current, newtree)) || /* 6 and 7 */
@@ -1788,7 +1789,7 @@ int twoway_merge(const struct cache_entry * const *src,
/* 20 or 21 */
return merged_entry(newtree, current, o);
} else
- return o-gently ? -1 : reject_merge(current, o);
+ return reject_merge(current, o);
}
else if (newtree) {
if (oldtree !o-initial_checkout) {
--
--
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
