{"thread":{"id":"41164","subject":"[PATCH 0/2] fix corner case with lock_ref_sha1_basic and REF_NODEREF","startedAt":"2016-01-11T15:46:51Z","lastAt":"2016-01-13T11:38:38Z","messageCount":23,"participants":["Jeff King","Junio C Hamano","Michael Haggerty","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"275647","messageId":"20160111154651.GA25338@sigill.intra.peff.net","threadId":"41164","inReplyTo":null,"subject":"[PATCH 0/2] fix corner case with lock_ref_sha1_basic and REF_NODEREF","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-11T15:46:51Z","receivedAt":"2016-01-11T15:46:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I came across an interesting regression case with the new\ncreate_symref() that uses lock_ref_sha1_basic(). It turns out that the\nbug is actually in the latter function, but that it's slightly easier to\ntickle it since we added new callers.\n\nThe bug and fix are rather involved, so I won't repeat the explanation\nfrom patch 2/2 here. The first patch is just a cleanup necessary to\naccurately in test in the second.\n\n  [1/2]: checkout,clone: check return value of create_symref\n  [2/2]: lock_ref_sha1_basic: handle REF_NODEREF with invalid refs\n\n-Peff\n"},{"id":"275648","messageId":"20160111154935.GA22778@sigill.intra.peff.net","threadId":"41164","inReplyTo":"20160111154651.GA25338@sigill.intra.peff.net","subject":"[PATCH 1/2] checkout,clone: check return value of create_symref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-11T15:49:35Z","receivedAt":"2016-01-11T15:49:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's unlikely that we would fail to create or update a\nsymbolic ref (especially HEAD), but if we do, we should\nnotice and complain. Note that there's no need to give more\ndetails in our error message; create_symref will already\nhave done so.\n\nWhile we're here, let's also fix a minor memory leak in\nclone.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis patch could go to maint. I don't know if it's worth the trouble. I\nwas unable to figure out a way to trigger this reliably (hence no\ntests). The two ways I considered were:\n\n  - \"chmod -w .git\", but it results in a die() already\n\n  - the bug I'm fixing in 2/2; but we don't want to rely on that in our\n    test suite, since I'm about to fix it. :-/\n\n builtin/checkout.c |  3 ++-\n builtin/clone.c    | 11 +++++++----\n 2 files changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex e8110a9..5af84a3 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -661,7 +661,8 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\tdescribe_detached_head(_(\"HEAD is now at\"), new->commit);\n \t\t}\n \t} else if (new->path) {\t/* Switch branches. */\n-\t\tcreate_symref(\"HEAD\", new->path, msg.buf);\n+\t\tif (create_symref(\"HEAD\", new->path, msg.buf) < 0)\n+\t\t\tdie(\"unable to update HEAD\");\n \t\tif (!opts->quiet) {\n \t\t\tif (old->path && !strcmp(new->path, old->path)) {\n \t\t\t\tif (opts->new_branch_force)\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a0b3cd9..a7c8def 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -636,9 +636,11 @@ static void update_remote_refs(const struct ref *refs,\n \t\tstruct strbuf head_ref = STRBUF_INIT;\n \t\tstrbuf_addstr(&head_ref, branch_top);\n \t\tstrbuf_addstr(&head_ref, \"HEAD\");\n-\t\tcreate_symref(head_ref.buf,\n-\t\t\t      remote_head_points_at->peer_ref->name,\n-\t\t\t      msg);\n+\t\tif (create_symref(head_ref.buf,\n+\t\t\t\t  remote_head_points_at->peer_ref->name,\n+\t\t\t\t  msg) < 0)\n+\t\t\tdie(\"unable to update %s\", head_ref.buf);\n+\t\tstrbuf_release(&head_ref);\n \t}\n }\n \n@@ -648,7 +650,8 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \tconst char *head;\n \tif (our && skip_prefix(our->name, \"refs/heads/\", &head)) {\n \t\t/* Local default branch link */\n-\t\tcreate_symref(\"HEAD\", our->name, NULL);\n+\t\tif (create_symref(\"HEAD\", our->name, NULL) < 0)\n+\t\t\tdie(\"unable to update HEAD\");\n \t\tif (!option_bare) {\n \t\t\tupdate_ref(msg, \"HEAD\", our->old_oid.hash, NULL, 0,\n \t\t\t\t   UPDATE_REFS_DIE_ON_ERR);\n-- \n2.7.0.368.g04bc9ee\n"},{"id":"275650","messageId":"20160111155239.GB22778@sigill.intra.peff.net","threadId":"41164","inReplyTo":"20160111154651.GA25338@sigill.intra.peff.net","subject":"[PATCH 2/2] lock_ref_sha1_basic: handle REF_NODEREF with invalid refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-11T15:52:39Z","receivedAt":"2016-01-11T15:52:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We sometimes call lock_ref_sha1_basic both with REF_NODEREF\nto operate directly on a symbolic ref. This is used, for\nexample, to move to a detached HEAD, or when updating\nthe contents of HEAD via checkout or symbolic-ref.\n\nHowever, the first step of the function is to resolve the\nrefname to get the \"old\" sha1, and we do so without telling\nresolve_ref_unsafe() that we are only interested in the\nsymref. As a result, we may detect a problem there not with\nthe symref itself, but with something it points to.\n\nThe real-world example I found (and what is used in the test\nsuite) is a HEAD pointing to a ref that cannot exist,\nbecause it would cause a directory/file conflict with other\nexisting refs.  This situation is somewhat broken, of\ncourse, as trying to _commit_ on that HEAD would fail. But\nit's not explicitly forbidden, and we should be able to move\naway from it. However, neither \"git checkout\" nor \"git\nsymbolic-ref\" can do so. We try to take the lock on HEAD,\nwhich is pointing to a non-existent ref. We bail from\nresolve_ref_unsafe() with errno set to EISDIR, and the lock\ncode thinks we are attempting to create a d/f conflict.\n\nOf course we're not. The problem is that the lock code has\nno idea what level we were at when we got EISDIR, so trying\nto diagnose or remove empty directories for HEAD is not\nuseful.\n\nThe most obvious solution would be to call\nresolve_ref_unsafe() with RESOLVE_REF_NO_RECURSE, so that we\nnever look beyond the symref (and any problems we find must\nbe attributable to it). However, that means we would not\ncorrectly gather the \"old\" sha1. We do not typically care\nabout it for locking purposes with a symref (since the\nsymref has no value on its own), but it does affect what we\nwrite into the HEAD reflog.\n\nAnother possibility is to avoid the d/f check when\nREF_NORECURSE is set. But that would mean we fail to notice\na real d/f conflict. This is impossible with HEAD, but we\nwould not want to create refs/heads/origin/HEAD.lock if we\nalready have refs/heads/origin/HEAD/foo.\n\nSo instead, we attempt to resolve HEAD fully to get the old\nsha1, and only if that fails do we fallback to a\nnon-recursive resolution. We lose nothing to the fallback,\nsince we know the ref cannot be resolved, and thus we have\nno old sha1 in the first place. And we still get the benefit\nof the d/f-checking for the symref itself.\n\nThis does mean an extra round of filesystem lookups in some\ncases, but they should be rare. It only kicks in with\nREF_NODEREF, and then only when the existing ref cannot be\nresolved.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is prepared on top of the jk/symbolic-ref topic. As shown by the\ntests, though, this can be triggered when checking out a detached HEAD,\nso it existed prior to that. The fix is independent, but some of the\nadditions to the test suite rely on that topic.\n\nI can split it into separate patches, but I'm not sure it's worth the\ntrouble. The only case I found that triggers it is quite obscure, and\nprior to the jk/symbolic-ref topic, you can recover using a non-detached\ncheckout (or symbolic-ref).\n\n refs/files-backend.c             |  4 ++++\n t/t1401-symbolic-ref.sh          |  7 +++++++\n t/t2011-checkout-invalid-head.sh | 20 ++++++++++++++++++++\n 3 files changed, 31 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 180c837..ea67d82 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1901,6 +1901,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \n \trefname = resolve_ref_unsafe(refname, resolve_flags,\n \t\t\t\t     lock->old_oid.hash, &type);\n+\tif (!refname && (flags & REF_NODEREF))\n+\t\trefname = resolve_ref_unsafe(orig_refname,\n+\t\t\t\t\t     resolve_flags | RESOLVE_REF_NO_RECURSE,\n+\t\t\t\t\t     lock->old_oid.hash, &type);\n \tif (!refname && errno == EISDIR) {\n \t\t/*\n \t\t * we are trying to lock foo but we used to\ndiff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\nindex 5db876c..a713766 100755\n--- a/t/t1401-symbolic-ref.sh\n+++ b/t/t1401-symbolic-ref.sh\n@@ -122,4 +122,11 @@ test_expect_success 'symbolic-ref does not create ref d/f conflicts' '\n \ttest_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df\n '\n \n+test_expect_success 'symbolic-ref handles existing pointer to invalid name' '\n+\thead=$(git rev-parse HEAD) &&\n+\tgit symbolic-ref HEAD refs/heads/outer &&\n+\tgit update-ref refs/heads/outer/inner $head &&\n+\tgit symbolic-ref HEAD refs/heads/unrelated\n+'\n+\n test_done\ndiff --git a/t/t2011-checkout-invalid-head.sh b/t/t2011-checkout-invalid-head.sh\nindex 300f8bf..c6c11c4 100755\n--- a/t/t2011-checkout-invalid-head.sh\n+++ b/t/t2011-checkout-invalid-head.sh\n@@ -19,4 +19,24 @@ test_expect_success 'checkout master from invalid HEAD' '\n \tgit checkout master --\n '\n \n+test_expect_success 'create ref directory/file conflict scenario' '\n+\tgit update-ref refs/heads/outer/inner master &&\n+\n+\t# do not rely on symbolic-ref to get a known state,\n+\t# as it may use the same code we are testing\n+\treset_to_df () {\n+\t\techo \"ref: refs/heads/outer\" >.git/HEAD\n+\t}\n+'\n+\n+test_expect_failure 'checkout away from d/f HEAD (to branch)' '\n+\treset_to_df &&\n+\tgit checkout master\n+'\n+\n+test_expect_failure 'checkout away from d/f HEAD (to detached)' '\n+\treset_to_df &&\n+\tgit checkout --detach master\n+'\n+\n test_done\n-- \n2.7.0.368.g04bc9ee\n"},{"id":"275679","messageId":"xmqqziwcvulr.fsf@gitster.mtv.corp.google.com","threadId":"41164","inReplyTo":"20160111155239.GB22778@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] lock_ref_sha1_basic: handle REF_NODEREF with invalid refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-11T18:28:32Z","receivedAt":"2016-01-11T18:28:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So instead, we attempt to resolve HEAD fully to get the old\n> sha1, and only if that fails do we fallback to a\n> non-recursive resolution. We lose nothing to the fallback,\n> since we know the ref cannot be resolved, and thus we have\n> no old sha1 in the first place. And we still get the benefit\n> of the d/f-checking for the symref itself.\n>\n> This does mean an extra round of filesystem lookups in some\n> cases, but they should be rare. It only kicks in with\n> REF_NODEREF, and then only when the existing ref cannot be\n> resolved.\n\nMakes perfect sense to me.  Thanks.\n"},{"id":"275755","messageId":"56947C63.8000507@alum.mit.edu","threadId":"41164","inReplyTo":"20160111154935.GA22778@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] checkout,clone: check return value of create_symref","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-01-12T04:09:07Z","receivedAt":"2016-01-12T04:09:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/11/2016 04:49 PM, Jeff King wrote:\n> It's unlikely that we would fail to create or update a\n> symbolic ref (especially HEAD), but if we do, we should\n> notice and complain. Note that there's no need to give more\n> details in our error message; create_symref will already\n> have done so.\n> \n> While we're here, let's also fix a minor memory leak in\n> clone.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n\nLGTM.\n\n> ---\n> This patch could go to maint. I don't know if it's worth the trouble. I\n> was unable to figure out a way to trigger this reliably (hence no\n> tests). The two ways I considered were:\n> \n>   - \"chmod -w .git\", but it results in a die() already\n> \n>   - the bug I'm fixing in 2/2; but we don't want to rely on that in our\n>     test suite, since I'm about to fix it. :-/\n\nA locking conflict is an easy way to trigger this error:\n\n$ git --version\ngit version 2.7.0.rc2.31.g396da8f\n$ git branch foo\n$ touch .git/HEAD.lock\n$ git checkout foo\nerror: Unable to create '/home/mhagger/tmp/brokhead/.git/HEAD.lock':\nFile exists.\n\nIf no other git process is currently running, this probably means a\ngit process crashed in this repository earlier. Make sure no other git\nprocess is running and remove the file manually to continue.\nSwitched to branch 'foo'\n$ echo $?\n0\n\nAfter this patch, the above \"checkout\" command fails with retcode=128.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"275757","messageId":"5694873D.5000001@alum.mit.edu","threadId":"41164","inReplyTo":"20160111155239.GB22778@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] lock_ref_sha1_basic: handle REF_NODEREF with invalid refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-01-12T04:55:25Z","receivedAt":"2016-01-12T04:55:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/11/2016 04:52 PM, Jeff King wrote:\n> We sometimes call lock_ref_sha1_basic both with REF_NODEREF\n> to operate directly on a symbolic ref.\n\n^^^ This sentence seems to be missing some words.\n\n>                                        This is used, for\n> example, to move to a detached HEAD, or when updating\n> the contents of HEAD via checkout or symbolic-ref.\n> \n> However, the first step of the function is to resolve the\n> refname to get the \"old\" sha1, and we do so without telling\n> resolve_ref_unsafe() that we are only interested in the\n> symref. As a result, we may detect a problem there not with\n> the symref itself, but with something it points to.\n> \n> The real-world example I found (and what is used in the test\n> suite) is a HEAD pointing to a ref that cannot exist,\n> because it would cause a directory/file conflict with other\n> existing refs.  This situation is somewhat broken, of\n> course, as trying to _commit_ on that HEAD would fail. But\n> it's not explicitly forbidden, and we should be able to move\n> away from it. However, neither \"git checkout\" nor \"git\n> symbolic-ref\" can do so. We try to take the lock on HEAD,\n> which is pointing to a non-existent ref. We bail from\n> resolve_ref_unsafe() with errno set to EISDIR, and the lock\n> code thinks we are attempting to create a d/f conflict.\n> \n> Of course we're not. The problem is that the lock code has\n> no idea what level we were at when we got EISDIR, so trying\n> to diagnose or remove empty directories for HEAD is not\n> useful.\n> \n> The most obvious solution would be to call\n> resolve_ref_unsafe() with RESOLVE_REF_NO_RECURSE, so that we\n> never look beyond the symref (and any problems we find must\n> be attributable to it). However, that means we would not\n> correctly gather the \"old\" sha1. We do not typically care\n> about it for locking purposes with a symref (since the\n> symref has no value on its own), but it does affect what we\n> write into the HEAD reflog.\n> \n> Another possibility is to avoid the d/f check when\n> REF_NORECURSE is set. But that would mean we fail to notice\n> a real d/f conflict. This is impossible with HEAD, but we\n> would not want to create refs/heads/origin/HEAD.lock if we\n> already have refs/heads/origin/HEAD/foo.\n> \n> So instead, we attempt to resolve HEAD fully to get the old\n> sha1, and only if that fails do we fallback to a\n> non-recursive resolution. We lose nothing to the fallback,\n> since we know the ref cannot be resolved, and thus we have\n> no old sha1 in the first place. And we still get the benefit\n> of the d/f-checking for the symref itself.\n> \n> This does mean an extra round of filesystem lookups in some\n> cases, but they should be rare. It only kicks in with\n> REF_NODEREF, and then only when the existing ref cannot be\n> resolved.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> This is prepared on top of the jk/symbolic-ref topic. As shown by the\n> tests, though, this can be triggered when checking out a detached HEAD,\n> so it existed prior to that. The fix is independent, but some of the\n> additions to the test suite rely on that topic.\n> \n> I can split it into separate patches, but I'm not sure it's worth the\n> trouble. The only case I found that triggers it is quite obscure, and\n> prior to the jk/symbolic-ref topic, you can recover using a non-detached\n> checkout (or symbolic-ref).\n> \n>  refs/files-backend.c             |  4 ++++\n>  t/t1401-symbolic-ref.sh          |  7 +++++++\n>  t/t2011-checkout-invalid-head.sh | 20 ++++++++++++++++++++\n>  3 files changed, 31 insertions(+)\n> \n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 180c837..ea67d82 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1901,6 +1901,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>  \n>  \trefname = resolve_ref_unsafe(refname, resolve_flags,\n>  \t\t\t\t     lock->old_oid.hash, &type);\n> +\tif (!refname && (flags & REF_NODEREF))\n> +\t\trefname = resolve_ref_unsafe(orig_refname,\n> +\t\t\t\t\t     resolve_flags | RESOLVE_REF_NO_RECURSE,\n> +\t\t\t\t\t     lock->old_oid.hash, &type);\n> [...]\n\nThe main risk for this change would be that this new recovery code\nallows the function to continue, but one of the outputs of the second\nfunction invocation is not correct for the code that follows. Let me\nthink out loud:\n\n* refname -- now will be equal to orig_refname. I think the main effect\nis that it will be passed to verify_refname_available_dir(). This seems\nto be what we want.\n\n* type -- now reflects orig_refname; i.e., usually REF_ISSYMREF. This\nalso seems correct.\n\n* lock->old_oid.hash -- is now ZEROS. This might get compared to the\ncaller's old_sha1 in verify_lock(), and it will also be written to the\nreflog as the \"old\" value. I think this is also what we want.\n\nSo this change looks good to me.\n\nThanks for the good catch and especially the awesome commit message!\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"275770","messageId":"20160112094957.GA9855@sigill.intra.peff.net","threadId":"41164","inReplyTo":"56947C63.8000507@alum.mit.edu","subject":"Re: [PATCH 1/2] checkout,clone: check return value of create_symref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-12T09:49:58Z","receivedAt":"2016-01-12T09:49:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 12, 2016 at 05:09:07AM +0100, Michael Haggerty wrote:\n\n> > This patch could go to maint. I don't know if it's worth the trouble. I\n> > was unable to figure out a way to trigger this reliably (hence no\n> > tests). The two ways I considered were:\n> > \n> >   - \"chmod -w .git\", but it results in a die() already\n> > \n> >   - the bug I'm fixing in 2/2; but we don't want to rely on that in our\n> >     test suite, since I'm about to fix it. :-/\n> \n> A locking conflict is an easy way to trigger this error:\n> [...]\n\nThanks, that's a good suggestion; I'll add a test in the re-roll.\n\n-Peff\n"},{"id":"275771","messageId":"20160112095257.GB9855@sigill.intra.peff.net","threadId":"41164","inReplyTo":"5694873D.5000001@alum.mit.edu","subject":"Re: [PATCH 2/2] lock_ref_sha1_basic: handle REF_NODEREF with invalid refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-12T09:52:57Z","receivedAt":"2016-01-12T09:52:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 12, 2016 at 05:55:25AM +0100, Michael Haggerty wrote:\n\n> On 01/11/2016 04:52 PM, Jeff King wrote:\n> > We sometimes call lock_ref_sha1_basic both with REF_NODEREF\n> > to operate directly on a symbolic ref.\n> \n> ^^^ This sentence seems to be missing some words.\n\nI think it has one too many. :)\n\nIt was originally \"both with a regular ref and with a symref\", but I\nshortened it since we only care about the symref case. I think just\ngetting rid of \"both\" is the right thing.\n\n> > diff --git a/refs/files-backend.c b/refs/files-backend.c\n> > index 180c837..ea67d82 100644\n> > --- a/refs/files-backend.c\n> > +++ b/refs/files-backend.c\n> > @@ -1901,6 +1901,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n> >  \n> >  \trefname = resolve_ref_unsafe(refname, resolve_flags,\n> >  \t\t\t\t     lock->old_oid.hash, &type);\n> > +\tif (!refname && (flags & REF_NODEREF))\n> > +\t\trefname = resolve_ref_unsafe(orig_refname,\n> > +\t\t\t\t\t     resolve_flags | RESOLVE_REF_NO_RECURSE,\n> > +\t\t\t\t\t     lock->old_oid.hash, &type);\n> > [...]\n> \n> The main risk for this change would be that this new recovery code\n> allows the function to continue, but one of the outputs of the second\n> function invocation is not correct for the code that follows. Let me\n> think out loud:\n> \n> * refname -- now will be equal to orig_refname. I think the main effect\n> is that it will be passed to verify_refname_available_dir(). This seems\n> to be what we want.\n> \n> * type -- now reflects orig_refname; i.e., usually REF_ISSYMREF. This\n> also seems correct.\n> \n> * lock->old_oid.hash -- is now ZEROS. This might get compared to the\n> caller's old_sha1 in verify_lock(), and it will also be written to the\n> reflog as the \"old\" value. I think this is also what we want.\n> \n> So this change looks good to me.\n\nThanks. I had a nagging feeling that I hadn't considered all cases, but\nthe way you've framed it makes sense to me.\n\n-Peff\n"},{"id":"275772","messageId":"20160112095638.GA4773@sigill.intra.peff.net","threadId":"41164","inReplyTo":"20160111154651.GA25338@sigill.intra.peff.net","subject":"[PATCH v2 0/2] fix corner case with lock_ref_sha1_basic and REF_NODEREF","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-12T09:56:39Z","receivedAt":"2016-01-12T09:56:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 11, 2016 at 10:46:51AM -0500, Jeff King wrote:\n\n> I came across an interesting regression case with the new\n> create_symref() that uses lock_ref_sha1_basic(). It turns out that the\n> bug is actually in the latter function, but that it's slightly easier to\n> tickle it since we added new callers.\n> \n> The bug and fix are rather involved, so I won't repeat the explanation\n> from patch 2/2 here. The first patch is just a cleanup necessary to\n> accurately in test in the second.\n> \n>   [1/2]: checkout,clone: check return value of create_symref\n>   [2/2]: lock_ref_sha1_basic: handle REF_NODEREF with invalid refs\n\nHere's a re-roll incorporating feedback from Michael. Patch 1 now has a\ntest for git-checkout (doing one for clone seems rather hard without\nbeing racy). Patch 2 has a typo fix in the commit message, and the test\nscript is adjusted to account for the new test.\n\n-Peff\n"},{"id":"275773","messageId":"20160112095734.GA16501@sigill.intra.peff.net","threadId":"41164","inReplyTo":"20160112095638.GA4773@sigill.intra.peff.net","subject":"[PATCH v2 1/2] checkout,clone: check return value of create_symref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-12T09:57:34Z","receivedAt":"2016-01-12T09:57:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's unlikely that we would fail to create or update a\nsymbolic ref (especially HEAD), but if we do, we should\nnotice and complain. Note that there's no need to give more\ndetails in our error message; create_symref will already\nhave done so.\n\nWhile we're here, let's also fix a minor memory leak in\nclone.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/checkout.c               |  3 ++-\n builtin/clone.c                  | 11 +++++++----\n t/t2011-checkout-invalid-head.sh |  6 ++++++\n 3 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex e8110a9..5af84a3 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -661,7 +661,8 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\tdescribe_detached_head(_(\"HEAD is now at\"), new->commit);\n \t\t}\n \t} else if (new->path) {\t/* Switch branches. */\n-\t\tcreate_symref(\"HEAD\", new->path, msg.buf);\n+\t\tif (create_symref(\"HEAD\", new->path, msg.buf) < 0)\n+\t\t\tdie(\"unable to update HEAD\");\n \t\tif (!opts->quiet) {\n \t\t\tif (old->path && !strcmp(new->path, old->path)) {\n \t\t\t\tif (opts->new_branch_force)\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a0b3cd9..a7c8def 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -636,9 +636,11 @@ static void update_remote_refs(const struct ref *refs,\n \t\tstruct strbuf head_ref = STRBUF_INIT;\n \t\tstrbuf_addstr(&head_ref, branch_top);\n \t\tstrbuf_addstr(&head_ref, \"HEAD\");\n-\t\tcreate_symref(head_ref.buf,\n-\t\t\t      remote_head_points_at->peer_ref->name,\n-\t\t\t      msg);\n+\t\tif (create_symref(head_ref.buf,\n+\t\t\t\t  remote_head_points_at->peer_ref->name,\n+\t\t\t\t  msg) < 0)\n+\t\t\tdie(\"unable to update %s\", head_ref.buf);\n+\t\tstrbuf_release(&head_ref);\n \t}\n }\n \n@@ -648,7 +650,8 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \tconst char *head;\n \tif (our && skip_prefix(our->name, \"refs/heads/\", &head)) {\n \t\t/* Local default branch link */\n-\t\tcreate_symref(\"HEAD\", our->name, NULL);\n+\t\tif (create_symref(\"HEAD\", our->name, NULL) < 0)\n+\t\t\tdie(\"unable to update HEAD\");\n \t\tif (!option_bare) {\n \t\t\tupdate_ref(msg, \"HEAD\", our->old_oid.hash, NULL, 0,\n \t\t\t\t   UPDATE_REFS_DIE_ON_ERR);\ndiff --git a/t/t2011-checkout-invalid-head.sh b/t/t2011-checkout-invalid-head.sh\nindex 300f8bf..d444d5e 100755\n--- a/t/t2011-checkout-invalid-head.sh\n+++ b/t/t2011-checkout-invalid-head.sh\n@@ -19,4 +19,10 @@ test_expect_success 'checkout master from invalid HEAD' '\n \tgit checkout master --\n '\n \n+test_expect_success 'checkout notices failure to lock HEAD' '\n+\ttest_when_finished \"rm -f .git/HEAD.lock\" &&\n+\t>.git/HEAD.lock &&\n+\ttest_must_fail git checkout -b other\n+'\n+\n test_done\n-- \n2.7.0.368.g04bc9ee\n"},{"id":"275774","messageId":"20160112095804.GB16501@sigill.intra.peff.net","threadId":"41164","inReplyTo":"20160112095638.GA4773@sigill.intra.peff.net","subject":"[PATCH v2 2/2] lock_ref_sha1_basic: handle REF_NODEREF with invalid refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-12T09:58:05Z","receivedAt":"2016-01-12T09:58:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We sometimes call lock_ref_sha1_basic with REF_NODEREF\nto operate directly on a symbolic ref. This is used, for\nexample, to move to a detached HEAD, or when updating\nthe contents of HEAD via checkout or symbolic-ref.\n\nHowever, the first step of the function is to resolve the\nrefname to get the \"old\" sha1, and we do so without telling\nresolve_ref_unsafe() that we are only interested in the\nsymref. As a result, we may detect a problem there not with\nthe symref itself, but with something it points to.\n\nThe real-world example I found (and what is used in the test\nsuite) is a HEAD pointing to a ref that cannot exist,\nbecause it would cause a directory/file conflict with other\nexisting refs.  This situation is somewhat broken, of\ncourse, as trying to _commit_ on that HEAD would fail. But\nit's not explicitly forbidden, and we should be able to move\naway from it. However, neither \"git checkout\" nor \"git\nsymbolic-ref\" can do so. We try to take the lock on HEAD,\nwhich is pointing to a non-existent ref. We bail from\nresolve_ref_unsafe() with errno set to EISDIR, and the lock\ncode thinks we are attempting to create a d/f conflict.\n\nOf course we're not. The problem is that the lock code has\nno idea what level we were at when we got EISDIR, so trying\nto diagnose or remove empty directories for HEAD is not\nuseful.\n\nThe most obvious solution would be to call\nresolve_ref_unsafe() with RESOLVE_REF_NO_RECURSE, so that we\nnever look beyond the symref (and any problems we find must\nbe attributable to it). However, that means we would not\ncorrectly gather the \"old\" sha1. We do not typically care\nabout it for locking purposes with a symref (since the\nsymref has no value on its own), but it does affect what we\nwrite into the HEAD reflog.\n\nAnother possibility is to avoid the d/f check when\nREF_NORECURSE is set. But that would mean we fail to notice\na real d/f conflict. This is impossible with HEAD, but we\nwould not want to create refs/heads/origin/HEAD.lock if we\nalready have refs/heads/origin/HEAD/foo.\n\nSo instead, we attempt to resolve HEAD fully to get the old\nsha1, and only if that fails do we fallback to a\nnon-recursive resolution. We lose nothing to the fallback,\nsince we know the ref cannot be resolved, and thus we have\nno old sha1 in the first place. And we still get the benefit\nof the d/f-checking for the symref itself.\n\nThis does mean an extra round of filesystem lookups in some\ncases, but they should be rare. It only kicks in with\nREF_NODEREF, and then only when the existing ref cannot be\nresolved.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs/files-backend.c             |  4 ++++\n t/t1401-symbolic-ref.sh          |  7 +++++++\n t/t2011-checkout-invalid-head.sh | 20 ++++++++++++++++++++\n 3 files changed, 31 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 180c837..ea67d82 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1901,6 +1901,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \n \trefname = resolve_ref_unsafe(refname, resolve_flags,\n \t\t\t\t     lock->old_oid.hash, &type);\n+\tif (!refname && (flags & REF_NODEREF))\n+\t\trefname = resolve_ref_unsafe(orig_refname,\n+\t\t\t\t\t     resolve_flags | RESOLVE_REF_NO_RECURSE,\n+\t\t\t\t\t     lock->old_oid.hash, &type);\n \tif (!refname && errno == EISDIR) {\n \t\t/*\n \t\t * we are trying to lock foo but we used to\ndiff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\nindex 5db876c..a713766 100755\n--- a/t/t1401-symbolic-ref.sh\n+++ b/t/t1401-symbolic-ref.sh\n@@ -122,4 +122,11 @@ test_expect_success 'symbolic-ref does not create ref d/f conflicts' '\n \ttest_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df\n '\n \n+test_expect_success 'symbolic-ref handles existing pointer to invalid name' '\n+\thead=$(git rev-parse HEAD) &&\n+\tgit symbolic-ref HEAD refs/heads/outer &&\n+\tgit update-ref refs/heads/outer/inner $head &&\n+\tgit symbolic-ref HEAD refs/heads/unrelated\n+'\n+\n test_done\ndiff --git a/t/t2011-checkout-invalid-head.sh b/t/t2011-checkout-invalid-head.sh\nindex d444d5e..9c1fddf 100755\n--- a/t/t2011-checkout-invalid-head.sh\n+++ b/t/t2011-checkout-invalid-head.sh\n@@ -25,4 +25,24 @@ test_expect_success 'checkout notices failure to lock HEAD' '\n \ttest_must_fail git checkout -b other\n '\n \n+test_expect_success 'create ref directory/file conflict scenario' '\n+\tgit update-ref refs/heads/outer/inner master &&\n+\n+\t# do not rely on symbolic-ref to get a known state,\n+\t# as it may use the same code we are testing\n+\treset_to_df () {\n+\t\techo \"ref: refs/heads/outer\" >.git/HEAD\n+\t}\n+'\n+\n+test_expect_failure 'checkout away from d/f HEAD (to branch)' '\n+\treset_to_df &&\n+\tgit checkout master\n+'\n+\n+test_expect_failure 'checkout away from d/f HEAD (to detached)' '\n+\treset_to_df &&\n+\tgit checkout --detach master\n+'\n+\n test_done\n-- \n2.7.0.368.g04bc9ee\n"},{"id":"275805","messageId":"20160112132628.GA31854@sigill.intra.peff.net","threadId":"41164","inReplyTo":"20160112095804.GB16501@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] lock_ref_sha1_basic: handle REF_NODEREF with invalid refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-12T13:26:28Z","receivedAt":"2016-01-12T13:26:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 12, 2016 at 04:58:04AM -0500, Jeff King wrote:\n\n> +test_expect_failure 'checkout away from d/f HEAD (to branch)' '\n> +\treset_to_df &&\n> +\tgit checkout master\n> +'\n> +\n> +test_expect_failure 'checkout away from d/f HEAD (to detached)' '\n> +\treset_to_df &&\n> +\tgit checkout --detach master\n> +'\n\nThese should be expect_success, of course (I had originally planned to\nintroduce the tests and then fix them later, but it all ended up in the\nsame patch).\n\nUnfortunately, I think there is a case we're missing. I'm still digging,\nbut hope to have something soon. In the meantime, don't bother with v2.\n:)\n\n-Peff\n"},{"id":"275806","messageId":"20160112135522.GA23255@sigill.intra.peff.net","threadId":"41164","inReplyTo":"20160112132628.GA31854@sigill.intra.peff.net","subject":"[PATCH v3 2/2] lock_ref_sha1_basic: handle REF_NODEREF with invalid refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-12T13:55:22Z","receivedAt":"2016-01-12T13:55:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 12, 2016 at 08:26:28AM -0500, Jeff King wrote:\n\n> On Tue, Jan 12, 2016 at 04:58:04AM -0500, Jeff King wrote:\n> \n> > +test_expect_failure 'checkout away from d/f HEAD (to branch)' '\n> > +\treset_to_df &&\n> > +\tgit checkout master\n> > +'\n> > +\n> > +test_expect_failure 'checkout away from d/f HEAD (to detached)' '\n> > +\treset_to_df &&\n> > +\tgit checkout --detach master\n> > +'\n> \n> These should be expect_success, of course (I had originally planned to\n> introduce the tests and then fix them later, but it all ended up in the\n> same patch).\n> \n> Unfortunately, I think there is a case we're missing. I'm still digging,\n> but hope to have something soon. In the meantime, don't bother with v2.\n\nThis rabbit hole just keeps getting deeper. :)\n\nThe behavior I tested and fixed earlier only covers the _loose_ ref\ncase, where we actually see EISDIR, and resolve_ref returns NULL. But if\nthe refs are all packed, resolve_ref will happily report the pointed-to\nref, and we do not hit this fallback code path at all.\n\nHere's a v3 for the second patch (the first one is fine) that handles\nthis case. See the amended description in the commit message, and I\nadded some comments which hopefully make things more obvious.\n\nI also notice that if we are deleting, we _do_ set\nRESOLVE_REF_NO_RECURSE from the very beginning, which means we would\ngenerally not get a valid lock->old_oid.hash for a symref. But I'm not\nsure what it would mean to delete a symref while asking for its current\nvalue (it cannot have one!). So I don't think it is a bug.\n\n-- >8 --\nSubject: lock_ref_sha1_basic: handle REF_NODEREF with invalid refs\n\nWe sometimes call lock_ref_sha1_basic with REF_NODEREF\nto operate directly on a symbolic ref. This is used, for\nexample, to move to a detached HEAD, or when updating\nthe contents of HEAD via checkout or symbolic-ref.\n\nHowever, the first step of the function is to resolve the\nrefname to get the \"old\" sha1, and we do so without telling\nresolve_ref_unsafe() that we are only interested in the\nsymref. As a result, we may detect a problem there not with\nthe symref itself, but with something it points to.\n\nThe real-world example I found (and what is used in the test\nsuite) is a HEAD pointing to a ref that cannot exist,\nbecause it would cause a directory/file conflict with other\nexisting refs.  This situation is somewhat broken, of\ncourse, as trying to _commit_ on that HEAD would fail. But\nit's not explicitly forbidden, and we should be able to move\naway from it. However, neither \"git checkout\" nor \"git\nsymbolic-ref\" can do so. We try to take the lock on HEAD,\nwhich is pointing to a non-existent ref. We bail from\nresolve_ref_unsafe() with errno set to EISDIR, and the lock\ncode thinks we are attempting to create a d/f conflict.\n\nOf course we're not. The problem is that the lock code has\nno idea what level we were at when we got EISDIR, so trying\nto diagnose or remove empty directories for HEAD is not\nuseful.\n\nTo make things even more complicated, we only get EISDIR in\nthe loose-ref case. If the refs are packed, the resolution\nmay \"succeed\", giving us the pointed-to ref in \"refname\",\nbut a null oid. Later, we say \"ah, the null oid means we are\ncreating; let's make sure there is room for it\", but\nmistakenly check against the _resolved_ refname, not the\noriginal.\n\nThe most obvious solution would be to call\nresolve_ref_unsafe() with RESOLVE_REF_NO_RECURSE, so that we\nnever look beyond the symref (and any problems we find must\nbe attributable to it). However, that means we would not\ncorrectly gather the \"old\" sha1. We do not typically care\nabout it for locking purposes with a symref (since the\nsymref has no value on its own), but it does affect what we\nwrite into the HEAD reflog.\n\nAnother possibility is to avoid the d/f check when\nREF_NORECURSE is set. But that would mean we fail to notice\na real d/f conflict. This is impossible with HEAD, but we\nwould not want to create refs/heads/origin/HEAD.lock if we\nalready have refs/heads/origin/HEAD/foo.\n\nSo instead, we attempt to resolve HEAD fully to get the old\nsha1, and only if that fails do we fallback to a\nnon-recursive resolution. We lose nothing to the fallback,\nsince we know the ref cannot be resolved, and thus we have\nno old sha1 in the first place. And we still get the benefit\nof the d/f-checking for the symref itself.\n\nThis does mean an extra round of filesystem lookups in some\ncases, but they should be rare. It only kicks in with\nREF_NODEREF, and then only when the existing ref cannot be\nresolved.\n\nAnd if the lookup of HEAD _does_ succeed, we switch to using\norig_refname much sooner, to cover the packed-refs case.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs/files-backend.c             | 23 +++++++++++++++++++++++\n t/t1401-symbolic-ref.sh          |  7 +++++++\n t/t2011-checkout-invalid-head.sh | 34 ++++++++++++++++++++++++++++++++++\n 3 files changed, 64 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 180c837..291b18d 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1899,8 +1899,31 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\tresolve_flags |= RESOLVE_REF_NO_RECURSE;\n \t}\n \n+\t/*\n+\t * Resolve with recursion, even if REF_NODEREF was set,\n+\t * as we want to make sure the \"old\" hash is filled in.\n+\t */\n \trefname = resolve_ref_unsafe(refname, resolve_flags,\n \t\t\t\t     lock->old_oid.hash, &type);\n+\tif (flags & REF_NODEREF) {\n+\t\t/*\n+\t\t * If we got a refname, then we know that the outer symref\n+\t\t * exists (though it might not point to an existing ref).\n+\t\t * From here out, we care only about the original name, because\n+\t\t * of REF_NODEREF.\n+\t\t *\n+\t\t * Otherwise, we know there was some lookup error, and we want\n+\t\t * to know whether it was due to orig_refname, or something it\n+\t\t * pointed to. Repeat our lookup without recursion.\n+\t\t */\n+\t\tif (refname)\n+\t\t\trefname = orig_refname;\n+\t\telse\n+\t\t\trefname = resolve_ref_unsafe(orig_refname,\n+\t\t\t\t\t\t     resolve_flags | RESOLVE_REF_NO_RECURSE,\n+\t\t\t\t\t\t     lock->old_oid.hash, &type);\n+\t}\n+\n \tif (!refname && errno == EISDIR) {\n \t\t/*\n \t\t * we are trying to lock foo but we used to\ndiff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\nindex 5db876c..a713766 100755\n--- a/t/t1401-symbolic-ref.sh\n+++ b/t/t1401-symbolic-ref.sh\n@@ -122,4 +122,11 @@ test_expect_success 'symbolic-ref does not create ref d/f conflicts' '\n \ttest_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df\n '\n \n+test_expect_success 'symbolic-ref handles existing pointer to invalid name' '\n+\thead=$(git rev-parse HEAD) &&\n+\tgit symbolic-ref HEAD refs/heads/outer &&\n+\tgit update-ref refs/heads/outer/inner $head &&\n+\tgit symbolic-ref HEAD refs/heads/unrelated\n+'\n+\n test_done\ndiff --git a/t/t2011-checkout-invalid-head.sh b/t/t2011-checkout-invalid-head.sh\nindex d444d5e..762907d 100755\n--- a/t/t2011-checkout-invalid-head.sh\n+++ b/t/t2011-checkout-invalid-head.sh\n@@ -25,4 +25,38 @@ test_expect_success 'checkout notices failure to lock HEAD' '\n \ttest_must_fail git checkout -b other\n '\n \n+test_expect_success 'create ref directory/file conflict scenario' '\n+\tgit update-ref refs/heads/outer/inner master &&\n+\n+\t# do not rely on symbolic-ref to get a known state,\n+\t# as it may use the same code we are testing\n+\treset_to_df () {\n+\t\techo \"ref: refs/heads/outer\" >.git/HEAD\n+\t}\n+'\n+\n+test_expect_success 'checkout away from d/f HEAD (unpacked, to branch)' '\n+\treset_to_df &&\n+\tgit checkout master\n+'\n+\n+test_expect_success 'checkout away from d/f HEAD (unpacked, to detached)' '\n+\treset_to_df &&\n+\tgit checkout --detach master\n+'\n+\n+test_expect_success 'pack refs' '\n+\tgit pack-refs --all --prune\n+'\n+\n+test_expect_success 'checkout away from d/f HEAD (packed, to branch)' '\n+\treset_to_df &&\n+\tgit checkout master\n+'\n+\n+test_expect_success 'checkout away from d/f HEAD (packed, to detached)' '\n+\treset_to_df &&\n+\tgit checkout --detach master\n+'\n+\n test_done\n-- \n2.7.0.368.g04bc9ee\n"},{"id":"275820","messageId":"xmqq4meivfak.fsf@gitster.mtv.corp.google.com","threadId":"41164","inReplyTo":"20160112095257.GB9855@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] lock_ref_sha1_basic: handle REF_NODEREF with invalid refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-12T18:11:31Z","receivedAt":"2016-01-12T18:11:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Jan 12, 2016 at 05:55:25AM +0100, Michael Haggerty wrote:\n>\n>> On 01/11/2016 04:52 PM, Jeff King wrote:\n>> > We sometimes call lock_ref_sha1_basic both with REF_NODEREF\n>> > to operate directly on a symbolic ref.\n>> \n>> ^^^ This sentence seems to be missing some words.\n>\n> I think it has one too many. :)\n>\n> It was originally \"both with a regular ref and with a symref\", but I\n> shortened it since we only care about the symref case. I think just\n> getting rid of \"both\" is the right thing.\n\nThanks, I did notice this and wondered the same while reviewing, but\ntotally forgot about it when I queued X-<.\n\n>> > diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> > index 180c837..ea67d82 100644\n>> > --- a/refs/files-backend.c\n>> > +++ b/refs/files-backend.c\n>> > @@ -1901,6 +1901,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>> >  \n>> >  \trefname = resolve_ref_unsafe(refname, resolve_flags,\n>> >  \t\t\t\t     lock->old_oid.hash, &type);\n>> > +\tif (!refname && (flags & REF_NODEREF))\n>> > +\t\trefname = resolve_ref_unsafe(orig_refname,\n>> > +\t\t\t\t\t     resolve_flags | RESOLVE_REF_NO_RECURSE,\n>> > +\t\t\t\t\t     lock->old_oid.hash, &type);\n>> > [...]\n>> \n>> The main risk for this change would be that this new recovery code\n>> allows the function to continue, but one of the outputs of the second\n>> function invocation is not correct for the code that follows. Let me\n>> think out loud:\n>> \n>> * refname -- now will be equal to orig_refname. I think the main effect\n>> is that it will be passed to verify_refname_available_dir(). This seems\n>> to be what we want.\n>> \n>> * type -- now reflects orig_refname; i.e., usually REF_ISSYMREF. This\n>> also seems correct.\n>> \n>> * lock->old_oid.hash -- is now ZEROS. This might get compared to the\n>> caller's old_sha1 in verify_lock(), and it will also be written to the\n>> reflog as the \"old\" value. I think this is also what we want.\n>> \n>> So this change looks good to me.\n>\n> Thanks. I had a nagging feeling that I hadn't considered all cases, but\n> the way you've framed it makes sense to me.\n\nSounds good.  Thanks, both.\n"},{"id":"275828","messageId":"xmqqlh7utwki.fsf@gitster.mtv.corp.google.com","threadId":"41164","inReplyTo":"20160112135522.GA23255@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/2] lock_ref_sha1_basic: handle REF_NODEREF with invalid refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-12T19:41:17Z","receivedAt":"2016-01-12T19:41:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I also notice that if we are deleting, we _do_ set\n> RESOLVE_REF_NO_RECURSE from the very beginning, which means we would\n> generally not get a valid lock->old_oid.hash for a symref. But I'm not\n> sure what it would mean to delete a symref while asking for its current\n> value (it cannot have one!). So I don't think it is a bug.\n\nI started scratching my head after noticing that the NO_RECURSE bit\nset in the DELETING codepath before reading the above, and I am\nstill doing so.\n\nA transaction that attempts to delete an existing symref presumably\nwants to make sure that the \"old\" value it read hasn't changed, but\nensuring the object name (obtained by reading the ref that is\npointed by the symref by dereferencing) are the same is not the\nright way to ensure nobody raced with us in the meantime anyway (we\nshould rather be making sure that the symref is still pointing at\nthe same ref), so in that sense, in the context of acquiring the\nlock, old oid value is meaningless for symrefs.\n\nThis patch is a strict improvement as the behaviour for REF_DELETING\ncase is unchanged by it (an idempotent resolve-ref-unsafe may be\ncalled one more time in some cases), and other cases are better, I\nthink.\n"},{"id":"275832","messageId":"20160112202251.GA10843@sigill.intra.peff.net","threadId":"41164","inReplyTo":"xmqqlh7utwki.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/2] lock_ref_sha1_basic: handle REF_NODEREF with invalid refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-12T20:22:52Z","receivedAt":"2016-01-12T20:22:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 12, 2016 at 11:41:17AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I also notice that if we are deleting, we _do_ set\n> > RESOLVE_REF_NO_RECURSE from the very beginning, which means we would\n> > generally not get a valid lock->old_oid.hash for a symref. But I'm not\n> > sure what it would mean to delete a symref while asking for its current\n> > value (it cannot have one!). So I don't think it is a bug.\n> \n> I started scratching my head after noticing that the NO_RECURSE bit\n> set in the DELETING codepath before reading the above, and I am\n> still doing so.\n> \n> A transaction that attempts to delete an existing symref presumably\n> wants to make sure that the \"old\" value it read hasn't changed, but\n> ensuring the object name (obtained by reading the ref that is\n> pointed by the symref by dereferencing) are the same is not the\n> right way to ensure nobody raced with us in the meantime anyway (we\n> should rather be making sure that the symref is still pointing at\n> the same ref), so in that sense, in the context of acquiring the\n> lock, old oid value is meaningless for symrefs.\n\nRight, that's the point I was trying to make. Though I think there is\nsomething even more subtle going in (see the end of this message).\n\nIn theory you might want the old sha1 for logging purposes, but since we\ndelete the reflog along with the symref, I don't think it matters there,\neither.\n\nI'm not sure we actually delete symrefs very often, though. Grepping for\ndelete_ref and REF_NODEREF shows the callers expecting symrefs to mostly\nbe \"git remote\", which never passes in an old_sha1.\n\nThe only caller which does so is \"branch -d\", but I think it doesn't\naffect symrefs. It gets the \"old\" sha1 by calling resolve_ref_unsafe()\nitself with RESOLVE_REF_NO_RECURSE, so it will unconditionally remove a\nsymref you ask it to, even if somebody else raced and put something in\nit.\n\nYou can also call \"update-ref --no-ref -d\" with an \"old\" sha1, but I\ndoubt anyone ever does so.\n\n> This patch is a strict improvement as the behaviour for REF_DELETING\n> case is unchanged by it (an idempotent resolve-ref-unsafe may be\n> called one more time in some cases), and other cases are better, I\n> think.\n\nYeah. My gut feeling is that the REF_DELETING special-handling of\nREF_NODEREF could just be folded into what I've added in this series.\nBut absent a case that is demonstrably broken, I'm inclined not to muck\nwith it too much.\n\nI had thought that this:\n\n  git init\n  git commit --allow-empty -m foo\n  git symbolic-ref refs/foo refs/heads/master\n  old=$(git rev-parse foo)\n  git update-ref --no-deref -d refs/foo $old\n\nmight trigger a problem (because reading refs/foo with NODEREF will give\nus a blank sha1 to compare against). But of course that is nonsense. The\nactual lock verification is not done by this initial resolve_ref. It\nhappens _after_ we take the lock (as it must to avoid races), when\nverify_lock() calls read_ref_full().\n\nBut now I'm doubly confused. When we call read_ref_full(), it is _also_\ninto lock->old_oid.hash. Which should be overwriting what the earlier\nresolve_ref_unsafe() wrote into it. Which would mean my whole commit is\nwrong; we can just unconditionally do a non-recursive resolution in the\nfirst place. But when I did so, I ended up with funny reflog values\n(which is why I wrote the patch as I did).\n\nLet me try to dig a little further into that case and see what is going\non.\n\n-Peff\n"},{"id":"275833","messageId":"20160112204229.GA13706@sigill.intra.peff.net","threadId":"41164","inReplyTo":"20160112202251.GA10843@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/2] lock_ref_sha1_basic: handle REF_NODEREF with invalid refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-12T20:42:29Z","receivedAt":"2016-01-12T20:42:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 12, 2016 at 03:22:51PM -0500, Jeff King wrote:\n\n> I had thought that this:\n> \n>   git init\n>   git commit --allow-empty -m foo\n>   git symbolic-ref refs/foo refs/heads/master\n>   old=$(git rev-parse foo)\n>   git update-ref --no-deref -d refs/foo $old\n> \n> might trigger a problem (because reading refs/foo with NODEREF will give\n> us a blank sha1 to compare against). But of course that is nonsense. The\n> actual lock verification is not done by this initial resolve_ref. It\n> happens _after_ we take the lock (as it must to avoid races), when\n> verify_lock() calls read_ref_full().\n> \n> But now I'm doubly confused. When we call read_ref_full(), it is _also_\n> into lock->old_oid.hash. Which should be overwriting what the earlier\n> resolve_ref_unsafe() wrote into it. Which would mean my whole commit is\n> wrong; we can just unconditionally do a non-recursive resolution in the\n> first place. But when I did so, I ended up with funny reflog values\n> (which is why I wrote the patch as I did).\n> \n> Let me try to dig a little further into that case and see what is going\n> on.\n\nAh, I see. When calling git-symbolic-ref, we don't provide an old_sha1,\nand therefore never call verify_lock(). And we get whatever value in\nlock->old_oid we happened to read earlier in resolve_ref_unsafe(). Which\nhappened outside of a lock. Yikes. It seems like we could racily write\nthe wrong reflog entry in such a case.\n\nSo I think we'd want something like this:\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 291b18d..c6ce503 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1845,7 +1845,7 @@ static int verify_lock(struct ref_lock *lock,\n \t\terrno = save_errno;\n \t\treturn -1;\n \t}\n-\tif (hashcmp(lock->old_oid.hash, old_sha1)) {\n+\tif (old_sha1 && hashcmp(lock->old_oid.hash, old_sha1)) {\n \t\tstrbuf_addf(err, \"ref %s is at %s but expected %s\",\n \t\t\t    lock->ref_name,\n \t\t\t    sha1_to_hex(lock->old_oid.hash),\n@@ -2008,7 +1983,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\tgoto error_return;\n \t\t}\n \t}\n-\tif (old_sha1 && verify_lock(lock, old_sha1, mustexist, err)) {\n+\tif (verify_lock(lock, old_sha1, mustexist, err)) {\n \t\tlast_errno = errno;\n \t\tgoto error_return;\n \t}\n\nto make sure that the value in lock->old_oid always comes from what we\nread under the lock. And then the resolve_ref() calls in\nlock_ref_sha1_basic() really don't matter. They are just about making\nsure there is space to create the lockfile.\n\nThe patch above is not quite right; I'll work up a series that takes\nthis approach.\n\n-Peff\n"},{"id":"275835","messageId":"20160112214318.GA2527@sigill.intra.peff.net","threadId":"41164","inReplyTo":"20160112204229.GA13706@sigill.intra.peff.net","subject":"[PATCH v4 0/3] fix corner cases with lock_ref_sha1_basic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-12T21:43:18Z","receivedAt":"2016-01-12T21:43:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 12, 2016 at 03:42:29PM -0500, Jeff King wrote:\n\n> Ah, I see. When calling git-symbolic-ref, we don't provide an old_sha1,\n> and therefore never call verify_lock(). And we get whatever value in\n> lock->old_oid we happened to read earlier in resolve_ref_unsafe(). Which\n> happened outside of a lock. Yikes. It seems like we could racily write\n> the wrong reflog entry in such a case.\n>\n> [...]\n> The patch above is not quite right; I'll work up a series that takes\n> this approach.\n\nOK, here it is. This replaces the top two patches of jk/symbolic-ref\n(i.e., everything in this thread I've sent in the last day or two).\n\nBesides fixing the race (which is detailed in patch 2/3), I think the\nresulting 3/3 is much cleaner. Sorry for all the false starts. The more\nI looked at this, the more complex it seemed to get. But I _think_ this\nis the right solution. :)\n\n  [1/3]: checkout,clone: check return value of create_symref\n  [2/3]: lock_ref_sha1_basic: always fill old_oid while holding lock\n  [3/3]: lock_ref_sha1_basic: handle REF_NODEREF with invalid refs\n\n-Peff\n"},{"id":"275836","messageId":"20160112214415.GA2841@sigill.intra.peff.net","threadId":"41164","inReplyTo":"20160112214318.GA2527@sigill.intra.peff.net","subject":"[PATCH 1/3] checkout,clone: check return value of create_symref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-12T21:44:16Z","receivedAt":"2016-01-12T21:44:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's unlikely that we would fail to create or update a\nsymbolic ref (especially HEAD), but if we do, we should\nnotice and complain. Note that there's no need to give more\ndetails in our error message; create_symref will already\nhave done so.\n\nWhile we're here, let's also fix a minor memory leak in\nclone.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSame as before (but with the extra test added in v2, in case you didn't\nthat up yet).\n\n builtin/checkout.c               |  3 ++-\n builtin/clone.c                  | 11 +++++++----\n t/t2011-checkout-invalid-head.sh |  6 ++++++\n 3 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex e8110a9..5af84a3 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -661,7 +661,8 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\tdescribe_detached_head(_(\"HEAD is now at\"), new->commit);\n \t\t}\n \t} else if (new->path) {\t/* Switch branches. */\n-\t\tcreate_symref(\"HEAD\", new->path, msg.buf);\n+\t\tif (create_symref(\"HEAD\", new->path, msg.buf) < 0)\n+\t\t\tdie(\"unable to update HEAD\");\n \t\tif (!opts->quiet) {\n \t\t\tif (old->path && !strcmp(new->path, old->path)) {\n \t\t\t\tif (opts->new_branch_force)\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a0b3cd9..a7c8def 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -636,9 +636,11 @@ static void update_remote_refs(const struct ref *refs,\n \t\tstruct strbuf head_ref = STRBUF_INIT;\n \t\tstrbuf_addstr(&head_ref, branch_top);\n \t\tstrbuf_addstr(&head_ref, \"HEAD\");\n-\t\tcreate_symref(head_ref.buf,\n-\t\t\t      remote_head_points_at->peer_ref->name,\n-\t\t\t      msg);\n+\t\tif (create_symref(head_ref.buf,\n+\t\t\t\t  remote_head_points_at->peer_ref->name,\n+\t\t\t\t  msg) < 0)\n+\t\t\tdie(\"unable to update %s\", head_ref.buf);\n+\t\tstrbuf_release(&head_ref);\n \t}\n }\n \n@@ -648,7 +650,8 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \tconst char *head;\n \tif (our && skip_prefix(our->name, \"refs/heads/\", &head)) {\n \t\t/* Local default branch link */\n-\t\tcreate_symref(\"HEAD\", our->name, NULL);\n+\t\tif (create_symref(\"HEAD\", our->name, NULL) < 0)\n+\t\t\tdie(\"unable to update HEAD\");\n \t\tif (!option_bare) {\n \t\t\tupdate_ref(msg, \"HEAD\", our->old_oid.hash, NULL, 0,\n \t\t\t\t   UPDATE_REFS_DIE_ON_ERR);\ndiff --git a/t/t2011-checkout-invalid-head.sh b/t/t2011-checkout-invalid-head.sh\nindex 300f8bf..d444d5e 100755\n--- a/t/t2011-checkout-invalid-head.sh\n+++ b/t/t2011-checkout-invalid-head.sh\n@@ -19,4 +19,10 @@ test_expect_success 'checkout master from invalid HEAD' '\n \tgit checkout master --\n '\n \n+test_expect_success 'checkout notices failure to lock HEAD' '\n+\ttest_when_finished \"rm -f .git/HEAD.lock\" &&\n+\t>.git/HEAD.lock &&\n+\ttest_must_fail git checkout -b other\n+'\n+\n test_done\n-- \n2.7.0.368.g04bc9ee\n"},{"id":"275837","messageId":"20160112214439.GB2841@sigill.intra.peff.net","threadId":"41164","inReplyTo":"20160112214318.GA2527@sigill.intra.peff.net","subject":"[PATCH 2/3] lock_ref_sha1_basic: always fill old_oid while holding lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-12T21:44:39Z","receivedAt":"2016-01-12T21:44:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our basic strategy for taking a ref lock is:\n\n  1. Create $ref.lock to take the lock\n\n  2. Read the ref again while holding the lock (during which\n     time we know that nobody else can be updating it).\n\n  3. Compare the value we read to the expected \"old_sha1\"\n\nThe value we read in step (2) is returned to the caller via\nthe lock->old_oid field, who may use it for other purposes\n(such as writing a reflog).\n\nIf we have no \"old_sha1\" (i.e., we are unconditionally\ntaking the lock), then we obviously must omit step 3. But we\n_also_ omit step 2. This seems like a nice optimization, but\nit means that the caller sees only whatever was left in\nlock->old_oid from previous calls to resolve_ref_unsafe(),\nwhich happened outside of the lock.\n\nWe can demonstrate this race pretty easily. Imagine you have\nthree commits, $one, $two, and $three. One script just flips\nbetween $one and $two, without providing an old-sha1:\n\n  while true; do\n    git update-ref -m one refs/heads/foo $one\n    git update-ref -m two refs/heads/foo $two\n  done\n\nMeanwhile, another script tries to set the value to $three,\nalso not using an old-sha1:\n\n  while true; do\n    git update-ref -m three refs/heads/foo $three\n  done\n\nIf these run simultaneously, we'll see a lot of lock\ncontention, but each of the writes will succeed some of the\ntime. The reflog may record movements between any of the\nthree refs, but we would expect it to provide a consistent\nlog: the \"from\" field of each log entry should be the same\nas the \"two\" field of the previous one.\n\nBut if we check this:\n\n  perl -alne '\n    print \"mismatch on line $.\"\n            if defined $last && $F[0] ne $last;\n    $last = $F[1];\n  ' .git/logs/refs/heads/foo\n\nwe'll see many mismatches. Why?\n\nBecause sometimes, in the time between lock_ref_sha1_basic\nfilling lock->old_oid via resolve_ref_unsafe() and it taking\nthe lock, there may be a complete write by another process.\nAnd the \"from\" field in our reflog entry will be wrong, and\nwill refer to an older value.\n\nThis is probably quite rare in practice. It requires writers\nwhich do not provide an old-sha1 value, and it is a very\nquick race. However, it is easy to fix: we simply perform\nstep (2), the read-under-lock, whether we have an old-sha1\nor not. Then the value we hand back to the caller is always\natomic.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs/files-backend.c | 17 +++++++++++------\n 1 file changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 180c837..69c3ecf 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1840,12 +1840,17 @@ static int verify_lock(struct ref_lock *lock,\n \tif (read_ref_full(lock->ref_name,\n \t\t\t  mustexist ? RESOLVE_REF_READING : 0,\n \t\t\t  lock->old_oid.hash, NULL)) {\n-\t\tint save_errno = errno;\n-\t\tstrbuf_addf(err, \"can't verify ref %s\", lock->ref_name);\n-\t\terrno = save_errno;\n-\t\treturn -1;\n+\t\tif (old_sha1) {\n+\t\t\tint save_errno = errno;\n+\t\t\tstrbuf_addf(err, \"can't verify ref %s\", lock->ref_name);\n+\t\t\terrno = save_errno;\n+\t\t\treturn -1;\n+\t\t} else {\n+\t\t\thashclr(lock->old_oid.hash);\n+\t\t\treturn 0;\n+\t\t}\n \t}\n-\tif (hashcmp(lock->old_oid.hash, old_sha1)) {\n+\tif (old_sha1 && hashcmp(lock->old_oid.hash, old_sha1)) {\n \t\tstrbuf_addf(err, \"ref %s is at %s but expected %s\",\n \t\t\t    lock->ref_name,\n \t\t\t    sha1_to_hex(lock->old_oid.hash),\n@@ -1985,7 +1990,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\tgoto error_return;\n \t\t}\n \t}\n-\tif (old_sha1 && verify_lock(lock, old_sha1, mustexist, err)) {\n+\tif (verify_lock(lock, old_sha1, mustexist, err)) {\n \t\tlast_errno = errno;\n \t\tgoto error_return;\n \t}\n-- \n2.7.0.368.g04bc9ee\n"},{"id":"275838","messageId":"20160112214509.GC2841@sigill.intra.peff.net","threadId":"41164","inReplyTo":"20160112214318.GA2527@sigill.intra.peff.net","subject":"[PATCH 3/3] lock_ref_sha1_basic: handle REF_NODEREF with invalid refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-12T21:45:09Z","receivedAt":"2016-01-12T21:45:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We sometimes call lock_ref_sha1_basic with REF_NODEREF\nto operate directly on a symbolic ref. This is used, for\nexample, to move to a detached HEAD, or when updating\nthe contents of HEAD via checkout or symbolic-ref.\n\nHowever, the first step of the function is to resolve the\nrefname to get the \"old\" sha1, and we do so without telling\nresolve_ref_unsafe() that we are only interested in the\nsymref. As a result, we may detect a problem there not with\nthe symref itself, but with something it points to.\n\nThe real-world example I found (and what is used in the test\nsuite) is a HEAD pointing to a ref that cannot exist,\nbecause it would cause a directory/file conflict with other\nexisting refs.  This situation is somewhat broken, of\ncourse, as trying to _commit_ on that HEAD would fail. But\nit's not explicitly forbidden, and we should be able to move\naway from it. However, neither \"git checkout\" nor \"git\nsymbolic-ref\" can do so. We try to take the lock on HEAD,\nwhich is pointing to a non-existent ref. We bail from\nresolve_ref_unsafe() with errno set to EISDIR, and the lock\ncode thinks we are attempting to create a d/f conflict.\n\nOf course we're not. The problem is that the lock code has\nno idea what level we were at when we got EISDIR, so trying\nto diagnose or remove empty directories for HEAD is not\nuseful.\n\nTo make things even more complicated, we only get EISDIR in\nthe loose-ref case. If the refs are packed, the resolution\nmay \"succeed\", giving us the pointed-to ref in \"refname\",\nbut a null oid. Later, we say \"ah, the null oid means we are\ncreating; let's make sure there is room for it\", but\nmistakenly check against the _resolved_ refname, not the\noriginal.\n\nWe can fix this by making two tweaks:\n\n  1. Call resolve_ref_unsafe() with RESOLVE_REF_NO_RECURSE\n     when REF_NODEREF is set. This means any errors\n     we get will be from the orig_refname, and we can act\n     accordingly.\n\n     We already do this in the REF_DELETING case, but we\n     should do it for update, too.\n\n  2. If we do get a \"refname\" return from\n     resolve_ref_unsafe(), even with RESOLVE_REF_NO_RECURSE\n     it may be the name of the ref pointed-to by a symref.\n     We already normalize this back to orig_refname before\n     taking the lockfile, but we need to do so before the\n     null_oid check.\n\nWhile we're rearranging the REF_NODEREF handling, we can\nalso bump the initialization of lflags to the top of the\nfunction, where we are setting up other flags. This saves us\nfrom having yet another conditional block on REF_NODEREF\njust to set it later.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs/files-backend.c             | 19 ++++++++++---------\n t/t1401-symbolic-ref.sh          |  7 +++++++\n t/t2011-checkout-invalid-head.sh | 33 +++++++++++++++++++++++++++++++++\n 3 files changed, 50 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 69c3ecf..81c92b4 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1887,7 +1887,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tconst char *orig_refname = refname;\n \tstruct ref_lock *lock;\n \tint last_errno = 0;\n-\tint type, lflags;\n+\tint type;\n+\tint lflags = 0;\n \tint mustexist = (old_sha1 && !is_null_sha1(old_sha1));\n \tint resolve_flags = 0;\n \tint attempts_remaining = 3;\n@@ -1898,10 +1899,11 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \n \tif (mustexist)\n \t\tresolve_flags |= RESOLVE_REF_READING;\n-\tif (flags & REF_DELETING) {\n+\tif (flags & REF_DELETING)\n \t\tresolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;\n-\t\tif (flags & REF_NODEREF)\n-\t\t\tresolve_flags |= RESOLVE_REF_NO_RECURSE;\n+\tif (flags & REF_NODEREF) {\n+\t\tresolve_flags |= RESOLVE_REF_NO_RECURSE;\n+\t\tlflags |= LOCK_NO_DEREF;\n \t}\n \n \trefname = resolve_ref_unsafe(refname, resolve_flags,\n@@ -1937,6 +1939,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \n \t\tgoto error_return;\n \t}\n+\n+\tif (flags & REF_NODEREF)\n+\t\trefname = orig_refname;\n+\n \t/*\n \t * If the ref did not exist and we are creating it, make sure\n \t * there is no existing packed ref whose name begins with our\n@@ -1952,11 +1958,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \n \tlock->lk = xcalloc(1, sizeof(struct lock_file));\n \n-\tlflags = 0;\n-\tif (flags & REF_NODEREF) {\n-\t\trefname = orig_refname;\n-\t\tlflags |= LOCK_NO_DEREF;\n-\t}\n \tlock->ref_name = xstrdup(refname);\n \tlock->orig_ref_name = xstrdup(orig_refname);\n \tstrbuf_git_path(&ref_file, \"%s\", refname);\ndiff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\nindex 5db876c..a713766 100755\n--- a/t/t1401-symbolic-ref.sh\n+++ b/t/t1401-symbolic-ref.sh\n@@ -122,4 +122,11 @@ test_expect_success 'symbolic-ref does not create ref d/f conflicts' '\n \ttest_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df\n '\n \n+test_expect_success 'symbolic-ref handles existing pointer to invalid name' '\n+\thead=$(git rev-parse HEAD) &&\n+\tgit symbolic-ref HEAD refs/heads/outer &&\n+\tgit update-ref refs/heads/outer/inner $head &&\n+\tgit symbolic-ref HEAD refs/heads/unrelated\n+'\n+\n test_done\ndiff --git a/t/t2011-checkout-invalid-head.sh b/t/t2011-checkout-invalid-head.sh\nindex d444d5e..c5501b0 100755\n--- a/t/t2011-checkout-invalid-head.sh\n+++ b/t/t2011-checkout-invalid-head.sh\n@@ -25,4 +25,37 @@ test_expect_success 'checkout notices failure to lock HEAD' '\n \ttest_must_fail git checkout -b other\n '\n \n+test_expect_success 'create ref directory/file conflict scenario' '\n+\tgit update-ref refs/heads/outer/inner master &&\n+\n+\t# do not rely on symbolic-ref to get a known state,\n+\t# as it may use the same code we are testing\n+\treset_to_df () {\n+\t\techo \"ref: refs/heads/outer\" >.git/HEAD\n+\t}\n+'\n+\n+test_expect_success 'checkout away from d/f HEAD (unpacked, to branch)' '\n+\treset_to_df &&\n+\tgit checkout master\n+'\n+\n+test_expect_success 'checkout away from d/f HEAD (unpacked, to detached)' '\n+\treset_to_df &&\n+\tgit checkout --detach master\n+'\n+\n+test_expect_success 'pack refs' '\n+\tgit pack-refs --all --prune\n+'\n+\n+test_expect_success 'checkout away from d/f HEAD (packed, to branch)' '\n+\treset_to_df &&\n+\tgit checkout master\n+'\n+\n+test_expect_success 'checkout away from d/f HEAD (packed, to detached)' '\n+\treset_to_df &&\n+\tgit checkout --detach master\n+'\n test_done\n-- \n2.7.0.368.g04bc9ee\n"},{"id":"275857","messageId":"CAPig+cRHNa8b8TdoRFsYvBBOcHbJF1VCKNgmt4SYkQsMAsJjqA@mail.gmail.com","threadId":"41164","inReplyTo":"20160112214439.GB2841@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] lock_ref_sha1_basic: always fill old_oid while holding lock","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-13T01:25:45Z","receivedAt":"2016-01-13T01:25:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 12, 2016 at 4:44 PM, Jeff King <peff@peff.net> wrote:\n> Our basic strategy for taking a ref lock is:\n> [...]\n> If these run simultaneously, we'll see a lot of lock\n> contention, but each of the writes will succeed some of the\n> time. The reflog may record movements between any of the\n> three refs, but we would expect it to provide a consistent\n> log: the \"from\" field of each log entry should be the same\n> as the \"two\" field of the previous one.\n\ns/two/to/\n"},{"id":"275889","messageId":"20160113113837.GA9360@sigill.intra.peff.net","threadId":"41164","inReplyTo":"CAPig+cRHNa8b8TdoRFsYvBBOcHbJF1VCKNgmt4SYkQsMAsJjqA@mail.gmail.com","subject":"Re: [PATCH 2/3] lock_ref_sha1_basic: always fill old_oid while holding lock","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-13T11:38:38Z","receivedAt":"2016-01-13T11:38:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 12, 2016 at 08:25:45PM -0500, Eric Sunshine wrote:\n\n> On Tue, Jan 12, 2016 at 4:44 PM, Jeff King <peff@peff.net> wrote:\n> > Our basic strategy for taking a ref lock is:\n> > [...]\n> > If these run simultaneously, we'll see a lot of lock\n> > contention, but each of the writes will succeed some of the\n> > time. The reflog may record movements between any of the\n> > three refs, but we would expect it to provide a consistent\n> > log: the \"from\" field of each log entry should be the same\n> > as the \"two\" field of the previous one.\n> \n> s/two/to/\n\nWhoops. Was tweaking my scripts \"one\" and \"two\" to test the race while I\nwrote up the commit message. :)\n\n-Peff\n"}]}