From: Jeff King <hidden> Date: 2016-06-15 23:07:41
I came across an interesting regression case with the new
create_symref() that uses lock_ref_sha1_basic(). It turns out that the
bug is actually in the latter function, but that it's slightly easier to
tickle it since we added new callers.
The bug and fix are rather involved, so I won't repeat the explanation
from patch 2/2 here. The first patch is just a cleanup necessary to
accurately in test in the second.
[1/2]: checkout,clone: check return value of create_symref
[2/2]: lock_ref_sha1_basic: handle REF_NODEREF with invalid refs
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:07:41
It's unlikely that we would fail to create or update a
symbolic ref (especially HEAD), but if we do, we should
notice and complain. Note that there's no need to give more
details in our error message; create_symref will already
have done so.
While we're here, let's also fix a minor memory leak in
clone.
Signed-off-by: Jeff King <redacted>
---
This patch could go to maint. I don't know if it's worth the trouble. I
was unable to figure out a way to trigger this reliably (hence no
tests). The two ways I considered were:
- "chmod -w .git", but it results in a die() already
- the bug I'm fixing in 2/2; but we don't want to rely on that in our
test suite, since I'm about to fix it. :-/
builtin/checkout.c | 3 ++-
builtin/clone.c | 11 +++++++----
2 files changed, 9 insertions(+), 5 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:07:41
We sometimes call lock_ref_sha1_basic both with REF_NODEREF
to operate directly on a symbolic ref. This is used, for
example, to move to a detached HEAD, or when updating
the contents of HEAD via checkout or symbolic-ref.
However, the first step of the function is to resolve the
refname to get the "old" sha1, and we do so without telling
resolve_ref_unsafe() that we are only interested in the
symref. As a result, we may detect a problem there not with
the symref itself, but with something it points to.
The real-world example I found (and what is used in the test
suite) is a HEAD pointing to a ref that cannot exist,
because it would cause a directory/file conflict with other
existing refs. This situation is somewhat broken, of
course, as trying to _commit_ on that HEAD would fail. But
it's not explicitly forbidden, and we should be able to move
away from it. However, neither "git checkout" nor "git
symbolic-ref" can do so. We try to take the lock on HEAD,
which is pointing to a non-existent ref. We bail from
resolve_ref_unsafe() with errno set to EISDIR, and the lock
code thinks we are attempting to create a d/f conflict.
Of course we're not. The problem is that the lock code has
no idea what level we were at when we got EISDIR, so trying
to diagnose or remove empty directories for HEAD is not
useful.
The most obvious solution would be to call
resolve_ref_unsafe() with RESOLVE_REF_NO_RECURSE, so that we
never look beyond the symref (and any problems we find must
be attributable to it). However, that means we would not
correctly gather the "old" sha1. We do not typically care
about it for locking purposes with a symref (since the
symref has no value on its own), but it does affect what we
write into the HEAD reflog.
Another possibility is to avoid the d/f check when
REF_NORECURSE is set. But that would mean we fail to notice
a real d/f conflict. This is impossible with HEAD, but we
would not want to create refs/heads/origin/HEAD.lock if we
already have refs/heads/origin/HEAD/foo.
So instead, we attempt to resolve HEAD fully to get the old
sha1, and only if that fails do we fallback to a
non-recursive resolution. We lose nothing to the fallback,
since we know the ref cannot be resolved, and thus we have
no old sha1 in the first place. And we still get the benefit
of the d/f-checking for the symref itself.
This does mean an extra round of filesystem lookups in some
cases, but they should be rare. It only kicks in with
REF_NODEREF, and then only when the existing ref cannot be
resolved.
Signed-off-by: Jeff King <redacted>
---
This is prepared on top of the jk/symbolic-ref topic. As shown by the
tests, though, this can be triggered when checking out a detached HEAD,
so it existed prior to that. The fix is independent, but some of the
additions to the test suite rely on that topic.
I can split it into separate patches, but I'm not sure it's worth the
trouble. The only case I found that triggers it is quite obscure, and
prior to the jk/symbolic-ref topic, you can recover using a non-detached
checkout (or symbolic-ref).
refs/files-backend.c | 4 ++++
t/t1401-symbolic-ref.sh | 7 +++++++
t/t2011-checkout-invalid-head.sh | 20 ++++++++++++++++++++
3 files changed, 31 insertions(+)
@@ -19,4 +19,24 @@ test_expect_success 'checkout master from invalid HEAD' 'gitcheckoutmaster--'+test_expect_success'create ref directory/file conflict scenario''+gitupdate-refrefs/heads/outer/innermaster&&++# do not rely on symbolic-ref to get a known state,+# as it may use the same code we are testing+reset_to_df(){+echo"ref: refs/heads/outer">.git/HEAD+}+'++test_expect_failure'checkout away from d/f HEAD (to branch)''+reset_to_df&&+gitcheckoutmaster+'++test_expect_failure'checkout away from d/f HEAD (to detached)''+reset_to_df&&+gitcheckout--detachmaster+'+ test_done
From: Michael Haggerty <hidden> Date: 2016-06-15 23:07:42
On 01/11/2016 04:49 PM, Jeff King wrote:
It's unlikely that we would fail to create or update a
symbolic ref (especially HEAD), but if we do, we should
notice and complain. Note that there's no need to give more
details in our error message; create_symref will already
have done so.
While we're here, let's also fix a minor memory leak in
clone.
Signed-off-by: Jeff King <redacted>
LGTM.
---
This patch could go to maint. I don't know if it's worth the trouble. I
was unable to figure out a way to trigger this reliably (hence no
tests). The two ways I considered were:
- "chmod -w .git", but it results in a die() already
- the bug I'm fixing in 2/2; but we don't want to rely on that in our
test suite, since I'm about to fix it. :-/
A locking conflict is an easy way to trigger this error:
$ git --version
git version 2.7.0.rc2.31.g396da8f
$ git branch foo
$ touch .git/HEAD.lock
$ git checkout foo
error: Unable to create '/home/mhagger/tmp/brokhead/.git/HEAD.lock':
File exists.
If no other git process is currently running, this probably means a
git process crashed in this repository earlier. Make sure no other git
process is running and remove the file manually to continue.
Switched to branch 'foo'
$ echo $?
0
After this patch, the above "checkout" command fails with retcode=128.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
From: Michael Haggerty <hidden> Date: 2016-06-15 23:07:42
On 01/11/2016 04:52 PM, Jeff King wrote:
We sometimes call lock_ref_sha1_basic both with REF_NODEREF
to operate directly on a symbolic ref.
^^^ This sentence seems to be missing some words.
quoted hunk
This is used, for
example, to move to a detached HEAD, or when updating
the contents of HEAD via checkout or symbolic-ref.
However, the first step of the function is to resolve the
refname to get the "old" sha1, and we do so without telling
resolve_ref_unsafe() that we are only interested in the
symref. As a result, we may detect a problem there not with
the symref itself, but with something it points to.
The real-world example I found (and what is used in the test
suite) is a HEAD pointing to a ref that cannot exist,
because it would cause a directory/file conflict with other
existing refs. This situation is somewhat broken, of
course, as trying to _commit_ on that HEAD would fail. But
it's not explicitly forbidden, and we should be able to move
away from it. However, neither "git checkout" nor "git
symbolic-ref" can do so. We try to take the lock on HEAD,
which is pointing to a non-existent ref. We bail from
resolve_ref_unsafe() with errno set to EISDIR, and the lock
code thinks we are attempting to create a d/f conflict.
Of course we're not. The problem is that the lock code has
no idea what level we were at when we got EISDIR, so trying
to diagnose or remove empty directories for HEAD is not
useful.
The most obvious solution would be to call
resolve_ref_unsafe() with RESOLVE_REF_NO_RECURSE, so that we
never look beyond the symref (and any problems we find must
be attributable to it). However, that means we would not
correctly gather the "old" sha1. We do not typically care
about it for locking purposes with a symref (since the
symref has no value on its own), but it does affect what we
write into the HEAD reflog.
Another possibility is to avoid the d/f check when
REF_NORECURSE is set. But that would mean we fail to notice
a real d/f conflict. This is impossible with HEAD, but we
would not want to create refs/heads/origin/HEAD.lock if we
already have refs/heads/origin/HEAD/foo.
So instead, we attempt to resolve HEAD fully to get the old
sha1, and only if that fails do we fallback to a
non-recursive resolution. We lose nothing to the fallback,
since we know the ref cannot be resolved, and thus we have
no old sha1 in the first place. And we still get the benefit
of the d/f-checking for the symref itself.
This does mean an extra round of filesystem lookups in some
cases, but they should be rare. It only kicks in with
REF_NODEREF, and then only when the existing ref cannot be
resolved.
Signed-off-by: Jeff King <redacted>
---
This is prepared on top of the jk/symbolic-ref topic. As shown by the
tests, though, this can be triggered when checking out a detached HEAD,
so it existed prior to that. The fix is independent, but some of the
additions to the test suite rely on that topic.
I can split it into separate patches, but I'm not sure it's worth the
trouble. The only case I found that triggers it is quite obscure, and
prior to the jk/symbolic-ref topic, you can recover using a non-detached
checkout (or symbolic-ref).
refs/files-backend.c | 4 ++++
t/t1401-symbolic-ref.sh | 7 +++++++
t/t2011-checkout-invalid-head.sh | 20 ++++++++++++++++++++
3 files changed, 31 insertions(+)
The main risk for this change would be that this new recovery code
allows the function to continue, but one of the outputs of the second
function invocation is not correct for the code that follows. Let me
think out loud:
* refname -- now will be equal to orig_refname. I think the main effect
is that it will be passed to verify_refname_available_dir(). This seems
to be what we want.
* type -- now reflects orig_refname; i.e., usually REF_ISSYMREF. This
also seems correct.
* lock->old_oid.hash -- is now ZEROS. This might get compared to the
caller's old_sha1 in verify_lock(), and it will also be written to the
reflog as the "old" value. I think this is also what we want.
So this change looks good to me.
Thanks for the good catch and especially the awesome commit message!
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
From: Jeff King <hidden> Date: 2016-06-15 23:07:42
On Tue, Jan 12, 2016 at 05:09:07AM +0100, Michael Haggerty wrote:
quoted
This patch could go to maint. I don't know if it's worth the trouble. I
was unable to figure out a way to trigger this reliably (hence no
tests). The two ways I considered were:
- "chmod -w .git", but it results in a die() already
- the bug I'm fixing in 2/2; but we don't want to rely on that in our
test suite, since I'm about to fix it. :-/
A locking conflict is an easy way to trigger this error:
[...]
Thanks, that's a good suggestion; I'll add a test in the re-roll.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:07:42
On Tue, Jan 12, 2016 at 05:55:25AM +0100, Michael Haggerty wrote:
On 01/11/2016 04:52 PM, Jeff King wrote:
quoted
We sometimes call lock_ref_sha1_basic both with REF_NODEREF
to operate directly on a symbolic ref.
^^^ This sentence seems to be missing some words.
I think it has one too many. :)
It was originally "both with a regular ref and with a symref", but I
shortened it since we only care about the symref case. I think just
getting rid of "both" is the right thing.
The main risk for this change would be that this new recovery code
allows the function to continue, but one of the outputs of the second
function invocation is not correct for the code that follows. Let me
think out loud:
* refname -- now will be equal to orig_refname. I think the main effect
is that it will be passed to verify_refname_available_dir(). This seems
to be what we want.
* type -- now reflects orig_refname; i.e., usually REF_ISSYMREF. This
also seems correct.
* lock->old_oid.hash -- is now ZEROS. This might get compared to the
caller's old_sha1 in verify_lock(), and it will also be written to the
reflog as the "old" value. I think this is also what we want.
So this change looks good to me.
Thanks. I had a nagging feeling that I hadn't considered all cases, but
the way you've framed it makes sense to me.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:07:42
On Mon, Jan 11, 2016 at 10:46:51AM -0500, Jeff King wrote:
I came across an interesting regression case with the new
create_symref() that uses lock_ref_sha1_basic(). It turns out that the
bug is actually in the latter function, but that it's slightly easier to
tickle it since we added new callers.
The bug and fix are rather involved, so I won't repeat the explanation
from patch 2/2 here. The first patch is just a cleanup necessary to
accurately in test in the second.
[1/2]: checkout,clone: check return value of create_symref
[2/2]: lock_ref_sha1_basic: handle REF_NODEREF with invalid refs
Here's a re-roll incorporating feedback from Michael. Patch 1 now has a
test for git-checkout (doing one for clone seems rather hard without
being racy). Patch 2 has a typo fix in the commit message, and the test
script is adjusted to account for the new test.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:07:42
It's unlikely that we would fail to create or update a
symbolic ref (especially HEAD), but if we do, we should
notice and complain. Note that there's no need to give more
details in our error message; create_symref will already
have done so.
While we're here, let's also fix a minor memory leak in
clone.
Signed-off-by: Jeff King <redacted>
---
builtin/checkout.c | 3 ++-
builtin/clone.c | 11 +++++++----
t/t2011-checkout-invalid-head.sh | 6 ++++++
3 files changed, 15 insertions(+), 5 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 23:07:42
We sometimes call lock_ref_sha1_basic with REF_NODEREF
to operate directly on a symbolic ref. This is used, for
example, to move to a detached HEAD, or when updating
the contents of HEAD via checkout or symbolic-ref.
However, the first step of the function is to resolve the
refname to get the "old" sha1, and we do so without telling
resolve_ref_unsafe() that we are only interested in the
symref. As a result, we may detect a problem there not with
the symref itself, but with something it points to.
The real-world example I found (and what is used in the test
suite) is a HEAD pointing to a ref that cannot exist,
because it would cause a directory/file conflict with other
existing refs. This situation is somewhat broken, of
course, as trying to _commit_ on that HEAD would fail. But
it's not explicitly forbidden, and we should be able to move
away from it. However, neither "git checkout" nor "git
symbolic-ref" can do so. We try to take the lock on HEAD,
which is pointing to a non-existent ref. We bail from
resolve_ref_unsafe() with errno set to EISDIR, and the lock
code thinks we are attempting to create a d/f conflict.
Of course we're not. The problem is that the lock code has
no idea what level we were at when we got EISDIR, so trying
to diagnose or remove empty directories for HEAD is not
useful.
The most obvious solution would be to call
resolve_ref_unsafe() with RESOLVE_REF_NO_RECURSE, so that we
never look beyond the symref (and any problems we find must
be attributable to it). However, that means we would not
correctly gather the "old" sha1. We do not typically care
about it for locking purposes with a symref (since the
symref has no value on its own), but it does affect what we
write into the HEAD reflog.
Another possibility is to avoid the d/f check when
REF_NORECURSE is set. But that would mean we fail to notice
a real d/f conflict. This is impossible with HEAD, but we
would not want to create refs/heads/origin/HEAD.lock if we
already have refs/heads/origin/HEAD/foo.
So instead, we attempt to resolve HEAD fully to get the old
sha1, and only if that fails do we fallback to a
non-recursive resolution. We lose nothing to the fallback,
since we know the ref cannot be resolved, and thus we have
no old sha1 in the first place. And we still get the benefit
of the d/f-checking for the symref itself.
This does mean an extra round of filesystem lookups in some
cases, but they should be rare. It only kicks in with
REF_NODEREF, and then only when the existing ref cannot be
resolved.
Signed-off-by: Jeff King <redacted>
---
refs/files-backend.c | 4 ++++
t/t1401-symbolic-ref.sh | 7 +++++++
t/t2011-checkout-invalid-head.sh | 20 ++++++++++++++++++++
3 files changed, 31 insertions(+)
@@ -25,4 +25,24 @@ test_expect_success 'checkout notices failure to lock HEAD' 'test_must_failgitcheckout-bother'+test_expect_success'create ref directory/file conflict scenario''+gitupdate-refrefs/heads/outer/innermaster&&++# do not rely on symbolic-ref to get a known state,+# as it may use the same code we are testing+reset_to_df(){+echo"ref: refs/heads/outer">.git/HEAD+}+'++test_expect_failure'checkout away from d/f HEAD (to branch)''+reset_to_df&&+gitcheckoutmaster+'++test_expect_failure'checkout away from d/f HEAD (to detached)''+reset_to_df&&+gitcheckout--detachmaster+'+ test_done
From: Jeff King <hidden> Date: 2016-06-15 23:07:42
On Tue, Jan 12, 2016 at 04:58:04AM -0500, Jeff King wrote:
+test_expect_failure 'checkout away from d/f HEAD (to branch)' '
+ reset_to_df &&
+ git checkout master
+'
+
+test_expect_failure 'checkout away from d/f HEAD (to detached)' '
+ reset_to_df &&
+ git checkout --detach master
+'
These should be expect_success, of course (I had originally planned to
introduce the tests and then fix them later, but it all ended up in the
same patch).
Unfortunately, I think there is a case we're missing. I'm still digging,
but hope to have something soon. In the meantime, don't bother with v2.
:)
-Peff
From: Jeff King <hidden> Date: 2016-06-15 23:07:42
On Tue, Jan 12, 2016 at 08:26:28AM -0500, Jeff King wrote:
On Tue, Jan 12, 2016 at 04:58:04AM -0500, Jeff King wrote:
quoted
+test_expect_failure 'checkout away from d/f HEAD (to branch)' '
+ reset_to_df &&
+ git checkout master
+'
+
+test_expect_failure 'checkout away from d/f HEAD (to detached)' '
+ reset_to_df &&
+ git checkout --detach master
+'
These should be expect_success, of course (I had originally planned to
introduce the tests and then fix them later, but it all ended up in the
same patch).
Unfortunately, I think there is a case we're missing. I'm still digging,
but hope to have something soon. In the meantime, don't bother with v2.
This rabbit hole just keeps getting deeper. :)
The behavior I tested and fixed earlier only covers the _loose_ ref
case, where we actually see EISDIR, and resolve_ref returns NULL. But if
the refs are all packed, resolve_ref will happily report the pointed-to
ref, and we do not hit this fallback code path at all.
Here's a v3 for the second patch (the first one is fine) that handles
this case. See the amended description in the commit message, and I
added some comments which hopefully make things more obvious.
I also notice that if we are deleting, we _do_ set
RESOLVE_REF_NO_RECURSE from the very beginning, which means we would
generally not get a valid lock->old_oid.hash for a symref. But I'm not
sure what it would mean to delete a symref while asking for its current
value (it cannot have one!). So I don't think it is a bug.
-- >8 --
Subject: lock_ref_sha1_basic: handle REF_NODEREF with invalid refs
We sometimes call lock_ref_sha1_basic with REF_NODEREF
to operate directly on a symbolic ref. This is used, for
example, to move to a detached HEAD, or when updating
the contents of HEAD via checkout or symbolic-ref.
However, the first step of the function is to resolve the
refname to get the "old" sha1, and we do so without telling
resolve_ref_unsafe() that we are only interested in the
symref. As a result, we may detect a problem there not with
the symref itself, but with something it points to.
The real-world example I found (and what is used in the test
suite) is a HEAD pointing to a ref that cannot exist,
because it would cause a directory/file conflict with other
existing refs. This situation is somewhat broken, of
course, as trying to _commit_ on that HEAD would fail. But
it's not explicitly forbidden, and we should be able to move
away from it. However, neither "git checkout" nor "git
symbolic-ref" can do so. We try to take the lock on HEAD,
which is pointing to a non-existent ref. We bail from
resolve_ref_unsafe() with errno set to EISDIR, and the lock
code thinks we are attempting to create a d/f conflict.
Of course we're not. The problem is that the lock code has
no idea what level we were at when we got EISDIR, so trying
to diagnose or remove empty directories for HEAD is not
useful.
To make things even more complicated, we only get EISDIR in
the loose-ref case. If the refs are packed, the resolution
may "succeed", giving us the pointed-to ref in "refname",
but a null oid. Later, we say "ah, the null oid means we are
creating; let's make sure there is room for it", but
mistakenly check against the _resolved_ refname, not the
original.
The most obvious solution would be to call
resolve_ref_unsafe() with RESOLVE_REF_NO_RECURSE, so that we
never look beyond the symref (and any problems we find must
be attributable to it). However, that means we would not
correctly gather the "old" sha1. We do not typically care
about it for locking purposes with a symref (since the
symref has no value on its own), but it does affect what we
write into the HEAD reflog.
Another possibility is to avoid the d/f check when
REF_NORECURSE is set. But that would mean we fail to notice
a real d/f conflict. This is impossible with HEAD, but we
would not want to create refs/heads/origin/HEAD.lock if we
already have refs/heads/origin/HEAD/foo.
So instead, we attempt to resolve HEAD fully to get the old
sha1, and only if that fails do we fallback to a
non-recursive resolution. We lose nothing to the fallback,
since we know the ref cannot be resolved, and thus we have
no old sha1 in the first place. And we still get the benefit
of the d/f-checking for the symref itself.
This does mean an extra round of filesystem lookups in some
cases, but they should be rare. It only kicks in with
REF_NODEREF, and then only when the existing ref cannot be
resolved.
And if the lookup of HEAD _does_ succeed, we switch to using
orig_refname much sooner, to cover the packed-refs case.
Signed-off-by: Jeff King <redacted>
---
refs/files-backend.c | 23 +++++++++++++++++++++++
t/t1401-symbolic-ref.sh | 7 +++++++
t/t2011-checkout-invalid-head.sh | 34 ++++++++++++++++++++++++++++++++++
3 files changed, 64 insertions(+)
@@ -25,4 +25,38 @@ test_expect_success 'checkout notices failure to lock HEAD' 'test_must_failgitcheckout-bother'+test_expect_success'create ref directory/file conflict scenario''+gitupdate-refrefs/heads/outer/innermaster&&++# do not rely on symbolic-ref to get a known state,+# as it may use the same code we are testing+reset_to_df(){+echo"ref: refs/heads/outer">.git/HEAD+}+'++test_expect_success'checkout away from d/f HEAD (unpacked, to branch)''+reset_to_df&&+gitcheckoutmaster+'++test_expect_success'checkout away from d/f HEAD (unpacked, to detached)''+reset_to_df&&+gitcheckout--detachmaster+'++test_expect_success'pack refs''+gitpack-refs--all--prune+'++test_expect_success'checkout away from d/f HEAD (packed, to branch)''+reset_to_df&&+gitcheckoutmaster+'++test_expect_success'checkout away from d/f HEAD (packed, to detached)''+reset_to_df&&+gitcheckout--detachmaster+'+ test_done