{"thread":{"id":"15916","subject":"Detached checkout will clobber branch head when using symlink HEAD","startedAt":"2008-10-15T18:24:47Z","lastAt":"2008-10-19T19:36:08Z","messageCount":15,"participants":["Matt Draisey","Jeff King","Nicolas Pitre","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"93112","messageId":"1224095087.5366.19.camel@localhost","threadId":"15916","inReplyTo":null,"subject":"Detached checkout will clobber branch head when using symlink HEAD","fromName":"Matt Draisey","fromEmail":"matt@draisey.ca","sentAt":"2008-10-15T18:24:47Z","receivedAt":"2008-10-15T18:24:47Z","isPatch":false,"sender":{"key":"matt@draisey.ca","avatar":null},"body":"The faulty code appears to be this\n\ngit 1.6.0.2.526.g5c283\nbuiltin-checkout.c\n\n>    481\tstatic void update_refs_for_switch(struct checkout_opts *opts,\n\n>    500\t\tif (new->path) {\n> \n>    511\t\t} else if (strcmp(new->name, \"HEAD\")) {\n>    512\t\t\tupdate_ref(msg.buf, \"HEAD\", new->commit->object.sha1, NULL,\n>    513\t\t\t\t   REF_NODEREF, DIE_ON_ERR);\n>    514\t\t\tif (!opts->quiet) {\n>    515\t\t\t\tif (old->path)\n>    516\t\t\t\t\tfprintf(stderr, \"Note: moving to \\\"%s\\\" which isn't a local branch\\nIf you want to create a new branch from this checkout, you may do so\\n(now or later) by using -b with the checkout command again. Example:\\n  git checkout -b <new_branch_name>\\n\", new->name);\n>    517\t\t\t\tdescribe_detached_head(\"HEAD is now at\", new->commit);\n>    518\t\t\t}\n>    519\t\t}\n\nIf HEAD is a symlink rather than a \"ref:\" style link this is really bad.\n"},{"id":"93222","messageId":"20081016191751.GB14707@coredump.intra.peff.net","threadId":"15916","inReplyTo":"1224095087.5366.19.camel@localhost","subject":"Re: Detached checkout will clobber branch head when using symlink HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-16T19:17:51Z","receivedAt":"2008-10-16T19:17:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 15, 2008 at 02:24:47PM -0400, Matt Draisey wrote:\n\n> If HEAD is a symlink rather than a \"ref:\" style link this is really\n> bad.\n\nHmm. I don't think we have shipped with a symlink HEAD for quite a long\ntime. Using one obviously doesn't work with detached HEAD, but also\nwould fail with packed refs. I don't know if we ever deprecated it.\n\nStill, this is a pretty nasty effect, in that it kills whatever was in\nthe existing ref. There is a fix below which refuses to lock any ref if\nit is a symlink and we have requested not to dereference (actually, it\nis overly cautious -- the symlink flag is triggered when we see a\nsymlink anywhere in the chain, though we need only prevent the situation\nwhen the _first_ symref is a symlink. I don't know if it is worth\ndifferentiating, since they are both \"should never happen anymore\"\nsituations, I think).\n\n---\ndiff --git a/refs.c b/refs.c\nindex b680750..43568e7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -447,7 +447,7 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t\t\t\tstrcpy(ref_buffer, buffer);\n \t\t\t\tref = ref_buffer;\n \t\t\t\tif (flag)\n-\t\t\t\t\t*flag |= REF_ISSYMREF;\n+\t\t\t\t\t*flag |= REF_ISSYMREF | REF_ISSYMLINK;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n@@ -817,6 +817,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char\n \t\t}\n \t\tref = resolve_ref(orig_ref, lock->old_sha1, mustexist, &type);\n \t}\n+\tif (type & REF_ISSYMLINK && flags & REF_NODEREF) {\n+\t\terror(\"unable to directly lock symbolic link '%s'\", orig_ref);\n+\t\tgoto error_return;\n+\t}\n \tif (type_p)\n \t    *type_p = type;\n \tif (!ref) {\ndiff --git a/refs.h b/refs.h\nindex 06ad260..6356a6a 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -12,6 +12,7 @@ struct ref_lock {\n \n #define REF_ISSYMREF 01\n #define REF_ISPACKED 02\n+#define REF_ISSYMLINK 04\n \n /*\n  * Calls the specified function for each ref file until it returns nonzero,\n"},{"id":"93226","messageId":"1224187863.2796.15.camel@localhost","threadId":"15916","inReplyTo":"20081016191751.GB14707@coredump.intra.peff.net","subject":"Re: Detached checkout will clobber branch head when using symlink HEAD","fromName":"Matt Draisey","fromEmail":"matt@draisey.ca","sentAt":"2008-10-16T20:11:03Z","receivedAt":"2008-10-16T20:11:03Z","isPatch":false,"sender":{"key":"matt@draisey.ca","avatar":null},"body":"On Thu, 2008-10-16 at 15:17 -0400, Jeff King wrote:\n\n> Hmm. I don't think we have shipped with a symlink HEAD for quite a long\n> time. Using one obviously doesn't work with detached HEAD, but also\n> would fail with packed refs. I don't know if we ever deprecated it.\n\nI am using the following system defaults:\n\ncore.prefersymlinkrefs=true\ngc.packrefs=false\n\nso almost all my git repositories are still using a symlink HEAD.\nI have some old scripts That I use occasionally and still depend on it.\nUsing detached checkout is the only problem I've had.\n"},{"id":"93228","messageId":"alpine.LFD.2.00.0810161619330.26244@xanadu.home","threadId":"15916","inReplyTo":"1224187863.2796.15.camel@localhost","subject":"Re: Detached checkout will clobber branch head when using symlink HEAD","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-16T20:20:32Z","receivedAt":"2008-10-16T20:20:32Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 16 Oct 2008, Matt Draisey wrote:\n\n> On Thu, 2008-10-16 at 15:17 -0400, Jeff King wrote:\n> \n> > Hmm. I don't think we have shipped with a symlink HEAD for quite a long\n> > time. Using one obviously doesn't work with detached HEAD, but also\n> > would fail with packed refs. I don't know if we ever deprecated it.\n> \n> I am using the following system defaults:\n> \n> core.prefersymlinkrefs=true\n> gc.packrefs=false\n> \n> so almost all my git repositories are still using a symlink HEAD.\n> I have some old scripts That I use occasionally and still depend on it.\n> Using detached checkout is the only problem I've had.\n\nA symlink HEAD and detached checkouts are simply incompatible.\n\n\nNicolas\n"},{"id":"93229","messageId":"20081016202202.GA9487@coredump.intra.peff.net","threadId":"15916","inReplyTo":"alpine.LFD.2.00.0810161619330.26244@xanadu.home","subject":"Re: Detached checkout will clobber branch head when using symlink HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-16T20:22:02Z","receivedAt":"2008-10-16T20:22:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 16, 2008 at 04:20:32PM -0400, Nicolas Pitre wrote:\n\n> > so almost all my git repositories are still using a symlink HEAD.\n> > I have some old scripts That I use occasionally and still depend on it.\n> > Using detached checkout is the only problem I've had.\n> \n> A symlink HEAD and detached checkouts are simply incompatible.\n\nAgreed, but I think the complaint is not that it doesn't work, but that\nit silently clobbers the current branch when you try it.\n\n-Peff\n"},{"id":"93236","messageId":"1224188939.2796.22.camel@localhost","threadId":"15916","inReplyTo":"alpine.LFD.2.00.0810161619330.26244@xanadu.home","subject":"Re: Detached checkout will clobber branch head when using symlink HEAD","fromName":"Matt Draisey","fromEmail":"matt@draisey.ca","sentAt":"2008-10-16T20:28:59Z","receivedAt":"2008-10-16T20:28:59Z","isPatch":false,"sender":{"key":"matt@draisey.ca","avatar":null},"body":"On Thu, 2008-10-16 at 16:20 -0400, Nicolas Pitre wrote:\n> A symlink HEAD and detached checkouts are simply incompatible.\n\nNot necessarily.  The symlinking code will unlink the original inode\neach time it creates a new symlink anyway.  It is simply a matter of\ncreating a new file in its place.\n"},{"id":"93238","messageId":"alpine.LFD.2.00.0810161629550.26244@xanadu.home","threadId":"15916","inReplyTo":"20081016202202.GA9487@coredump.intra.peff.net","subject":"Re: Detached checkout will clobber branch head when using symlink HEAD","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-16T20:30:24Z","receivedAt":"2008-10-16T20:30:24Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 16 Oct 2008, Jeff King wrote:\n\n> On Thu, Oct 16, 2008 at 04:20:32PM -0400, Nicolas Pitre wrote:\n> \n> > > so almost all my git repositories are still using a symlink HEAD.\n> > > I have some old scripts That I use occasionally and still depend on it.\n> > > Using detached checkout is the only problem I've had.\n> > \n> > A symlink HEAD and detached checkouts are simply incompatible.\n> \n> Agreed, but I think the complaint is not that it doesn't work, but that\n> it silently clobbers the current branch when you try it.\n\nThat is unacceptable indeed.\n\n\nNicolas\n"},{"id":"93240","messageId":"20081016203916.GB9487@coredump.intra.peff.net","threadId":"15916","inReplyTo":"1224187863.2796.15.camel@localhost","subject":"Re: Detached checkout will clobber branch head when using symlink HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-16T20:39:16Z","receivedAt":"2008-10-16T20:39:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 16, 2008 at 04:11:03PM -0400, Matt Draisey wrote:\n\n> I am using the following system defaults:\n> \n> core.prefersymlinkrefs=true\n> gc.packrefs=false\n> \n> so almost all my git repositories are still using a symlink HEAD.\n> I have some old scripts That I use occasionally and still depend on it.\n> Using detached checkout is the only problem I've had.\n\nIn your position I would consider updating my scripts. But I guess you\ncould also try to work up a patch that makes detached HEAD work\n(replacing the symlink with a file, then bringing back the symlink when\nyou're on a branch). In the meantime, here is a cleaned-up version of my\npatch, with a proper commit message and a test.\n\n-- >8 --\ndo not clobber symlinked ref with detached HEAD\n\nThe default configuration for git uses a symref for HEAD.\nWhen we detach the HEAD, we can simply write the detached\ncommit sha1 into the HEAD file.\n\nIt is still possible to use symlinks for HEAD (either by\nsetting it up manually, or by using core.prefersymlinkrefs).\nIn that case, moving to a detached HEAD is impossible, since\nwe have nowhere to store the sha1.\n\nHowever, the current code doesn't realize this and actually\nwrites into the HEAD file anyway, meaning that it overwrites\nthe value of the currently checked out branch.\n\nInstead, let's detect in the locking mechanism that we have\na symlink but the caller requested that we lock the original\nref name instead of its linked destination. This has two\nadvantages:\n\n  - we don't have to add an extra stat call, since we\n    discover the symlink during normal ref resolution\n\n  - any code to update a ref should lock it, meaning that we\n    should catch any other similar instances\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c                      |    9 ++++++++-\n refs.h                      |    1 +\n t/t7202-checkout-symlink.sh |   19 +++++++++++++++++++\n 3 files changed, 28 insertions(+), 1 deletions(-)\n create mode 100755 t/t7202-checkout-symlink.sh\n\ndiff --git a/refs.c b/refs.c\nindex b680750..b4b3865 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -446,8 +446,11 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t\t\t\tbuffer[len] = 0;\n \t\t\t\tstrcpy(ref_buffer, buffer);\n \t\t\t\tref = ref_buffer;\n-\t\t\t\tif (flag)\n+\t\t\t\tif (flag) {\n+\t\t\t\t\tif (!(*flag & REF_ISSYMREF))\n+\t\t\t\t\t\t*flag |= REF_OUTER_IS_SYMLINK;\n \t\t\t\t\t*flag |= REF_ISSYMREF;\n+\t\t\t\t}\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n@@ -817,6 +820,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char\n \t\t}\n \t\tref = resolve_ref(orig_ref, lock->old_sha1, mustexist, &type);\n \t}\n+\tif (type & REF_OUTER_IS_SYMLINK && flags & REF_NODEREF) {\n+\t\terror(\"unable to directly lock symbolic link '%s'\", orig_ref);\n+\t\tgoto error_return;\n+\t}\n \tif (type_p)\n \t    *type_p = type;\n \tif (!ref) {\ndiff --git a/refs.h b/refs.h\nindex 06ad260..9b0dcd9 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -12,6 +12,7 @@ struct ref_lock {\n \n #define REF_ISSYMREF 01\n #define REF_ISPACKED 02\n+#define REF_OUTER_IS_SYMLINK 04\n \n /*\n  * Calls the specified function for each ref file until it returns nonzero,\ndiff --git a/t/t7202-checkout-symlink.sh b/t/t7202-checkout-symlink.sh\nnew file mode 100755\nindex 0000000..cf09f5f\n--- /dev/null\n+++ b/t/t7202-checkout-symlink.sh\n@@ -0,0 +1,19 @@\n+#!/bin/sh\n+\n+test_description='checkout with symlinked HEAD'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo one > file && git add file && git commit -m one &&\n+\techo two > file && git add file && git commit -m two &&\n+\tln -sf refs/heads/master .git/HEAD\n+'\n+\n+test_expect_success 'checkout detached HEAD does not clobber ref' '\n+\ttest_must_fail git checkout HEAD^ &&\n+\techo two >expect &&\n+\tgit log -1 --pretty=tformat:%s >actual\n+\ttest_cmp actual expect\n+'\n+\n+test_done\n-- \n1.6.0.2.711.gf1ba4.dirty\n"},{"id":"93243","messageId":"alpine.LFD.2.00.0810161646550.26244@xanadu.home","threadId":"15916","inReplyTo":"1224188939.2796.22.camel@localhost","subject":"Re: Detached checkout will clobber branch head when using symlink HEAD","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-16T20:47:48Z","receivedAt":"2008-10-16T20:47:48Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 16 Oct 2008, Matt Draisey wrote:\n\n> On Thu, 2008-10-16 at 16:20 -0400, Nicolas Pitre wrote:\n> > A symlink HEAD and detached checkouts are simply incompatible.\n> \n> Not necessarily.  The symlinking code will unlink the original inode\n> each time it creates a new symlink anyway.  It is simply a matter of\n> creating a new file in its place.\n\nTrue.  I didn't think it all through initially.\n\n\nNicolas\n"},{"id":"93313","messageId":"7vfxmuhlad.fsf@gitster.siamese.dyndns.org","threadId":"15916","inReplyTo":"20081016203916.GB9487@coredump.intra.peff.net","subject":"Re: Detached checkout will clobber branch head when using symlink HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-17T23:08:42Z","receivedAt":"2008-10-17T23:08:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I do not think that is a correct approach.\n\nThe offending callchain is:\n\n  update_ref(..., \"HEAD\", REF_NODEREF, ...);\n  -> lock_any_ref_for_update(\"HEAD\", ..., REF_NODEREF);\n   -> lock_ref_sha1_basic(\"HEAD\", ..., REF_NODEREF, ...);\n      . calls resolve_ref() to read HEAD to arrive at\n        refs/heads/master\n      . however, it notices REF_NODEREF and adjusts the ref to be updated\n        back to \"HEAD\";\n    -> hold_lock_file_for_update(..., \"HEAD\", 1);\n     -> lock_file(..., \"HEAD\");\n        . resolves symlink \"HEAD\" to \"refs/heads/master\", and\n          locks it!  This creates \"refs/heads/master.lock\", that is\n          then renamed to \"refs/heads/master\" when unlocked.\n\nIn other words, the breakage is in lock_file() and not in resolve_ref().\nThe latter gives the same output to the caller whether the HEAD is\nsymbolic link or textual symref -- at least it should.\n\nThe behaviour of lock_file() to resolve symlink at this point in the code\ncomes from d58e8d3 (When locking in a symlinked repository, try to lock\nthe original, 2007-07-25), and as explained in the log message of that\ncommit, we cannot unconditionally remove it.\n\nThree patches to fix this issue, that come on top of the fix to t7201\n(--track from detached HEAD should fail) I sent out last night, will\nfollow.\n"},{"id":"93314","messageId":"7vbpxihl87.fsf_-_@gitster.siamese.dyndns.org","threadId":"15916","inReplyTo":"7vfxmuhlad.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 1/3] demonstrate breakage of detached checkout with symbolic link HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-17T23:10:00Z","receivedAt":"2008-10-17T23:10:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When core.prefersymlinkrefs is in use, detaching the HEAD by\ncheckout incorrectly clobbers the tip of the current branch.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t7201-co.sh |   12 ++++++++++++\n 1 files changed, 12 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex ee2cab6..01304d7 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -339,6 +339,18 @@ test_expect_success 'checkout w/--track from non-branch HEAD fails' '\n     test \"z$(git rev-parse master^0)\" = \"z$(git rev-parse HEAD)\"\n '\n \n+test_expect_failure 'detch a symbolic link HEAD' '\n+    git checkout master &&\n+    git config --bool core.prefersymlinkrefs yes &&\n+    git checkout side &&\n+    git checkout master &&\n+    it=$(git symbolic-ref HEAD) &&\n+    test \"z$it\" = zrefs/heads/master &&\n+    here=$(git rev-parse --verify refs/heads/master) &&\n+    git checkout side^ &&\n+    test \"z$(git rev-parse --verify refs/heads/master)\" = \"z$here\"\n+'\n+\n test_expect_success 'checkout an unmerged path should fail' '\n \trm -f .git/index &&\n \tO=$(echo original | git hash-object -w --stdin) &&\n-- \n1.6.0.2.734.gae0be\n"},{"id":"93315","messageId":"7v7i86hl7m.fsf_-_@gitster.siamese.dyndns.org","threadId":"15916","inReplyTo":"7vfxmuhlad.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 2/3] Enhance hold_lock_file_for_{update,append}() API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-17T23:10:21Z","receivedAt":"2008-10-17T23:10:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This changes the \"die_on_error\" boolean parameter to a mere \"flags\", and\nchanges the existing callers of hold_lock_file_for_update/append()\nfunctions to pass LOCK_DIE_ON_ERROR.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-commit.c     |    3 ++-\n builtin-fetch-pack.c |    3 ++-\n builtin-revert.c     |    3 ++-\n bundle.c             |    3 ++-\n cache.h              |    1 +\n lockfile.c           |   26 +++++++++++++++-----------\n pack-refs.c          |    3 ++-\n refs.c               |    3 ++-\n rerere.c             |    3 ++-\n sha1_file.c          |    2 +-\n 10 files changed, 31 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex e2a7e48..b563a0d 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -320,7 +320,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)\n \t\tdie(\"unable to write new_index file\");\n \n \tfd = hold_lock_file_for_update(&false_lock,\n-\t\t\t\t       git_path(\"next-index-%d\", getpid()), 1);\n+\t\t\t\t       git_path(\"next-index-%d\", getpid()),\n+\t\t\t\t       LOCK_DIE_ON_ERROR);\n \n \tcreate_base_index();\n \tadd_remove_files(&partial);\ndiff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c\nindex 85509f5..21ce3e0 100644\n--- a/builtin-fetch-pack.c\n+++ b/builtin-fetch-pack.c\n@@ -813,7 +813,8 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,\n \t\t\t  )\n \t\t\tdie(\"shallow file was changed during fetch\");\n \n-\t\tfd = hold_lock_file_for_update(&lock, shallow, 1);\n+\t\tfd = hold_lock_file_for_update(&lock, shallow,\n+\t\t\t\t\t       LOCK_DIE_ON_ERROR);\n \t\tif (!write_shallow_commits(fd, 0)) {\n \t\t\tunlink(shallow);\n \t\t\trollback_lock_file(&lock);\ndiff --git a/builtin-revert.c b/builtin-revert.c\nindex 27881e9..e839387 100644\n--- a/builtin-revert.c\n+++ b/builtin-revert.c\n@@ -338,7 +338,8 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \t * reverse of it if we are revert.\n \t */\n \n-\tmsg_fd = hold_lock_file_for_update(&msg_file, defmsg, 1);\n+\tmsg_fd = hold_lock_file_for_update(&msg_file, defmsg,\n+\t\t\t\t\t   LOCK_DIE_ON_ERROR);\n \n \tencoding = get_encoding(message);\n \tif (!encoding)\ndiff --git a/bundle.c b/bundle.c\nindex 00b2aab..7d17a1f 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -186,7 +186,8 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tif (bundle_to_stdout)\n \t\tbundle_fd = 1;\n \telse\n-\t\tbundle_fd = hold_lock_file_for_update(&lock, path, 1);\n+\t\tbundle_fd = hold_lock_file_for_update(&lock, path,\n+\t\t\t\t\t\t      LOCK_DIE_ON_ERROR);\n \n \t/* write signature */\n \twrite_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));\ndiff --git a/cache.h b/cache.h\nindex 884fae8..941a9dc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -411,6 +411,7 @@ struct lock_file {\n \tchar on_list;\n \tchar filename[PATH_MAX];\n };\n+#define LOCK_DIE_ON_ERROR 1\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n extern int commit_lock_file(struct lock_file *);\ndiff --git a/lockfile.c b/lockfile.c\nindex 4023797..bc1b585 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -121,9 +121,10 @@ static char *resolve_symlink(char *p, size_t s)\n }\n \n \n-static int lock_file(struct lock_file *lk, const char *path)\n+static int lock_file(struct lock_file *lk, const char *path, int flags)\n {\n-\tif (strlen(path) >= sizeof(lk->filename)) return -1;\n+\tif (strlen(path) >= sizeof(lk->filename))\n+\t\treturn -1;\n \tstrcpy(lk->filename, path);\n \t/*\n \t * subtract 5 from size to make sure there's room for adding\n@@ -155,21 +156,21 @@ static int lock_file(struct lock_file *lk, const char *path)\n \treturn lk->fd;\n }\n \n-int hold_lock_file_for_update(struct lock_file *lk, const char *path, int die_on_error)\n+int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)\n {\n-\tint fd = lock_file(lk, path);\n-\tif (fd < 0 && die_on_error)\n+\tint fd = lock_file(lk, path, flags);\n+\tif (fd < 0 && (flags & LOCK_DIE_ON_ERROR))\n \t\tdie(\"unable to create '%s.lock': %s\", path, strerror(errno));\n \treturn fd;\n }\n \n-int hold_lock_file_for_append(struct lock_file *lk, const char *path, int die_on_error)\n+int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n {\n \tint fd, orig_fd;\n \n-\tfd = lock_file(lk, path);\n+\tfd = lock_file(lk, path, flags);\n \tif (fd < 0) {\n-\t\tif (die_on_error)\n+\t\tif (flags & LOCK_DIE_ON_ERROR)\n \t\t\tdie(\"unable to create '%s.lock': %s\", path, strerror(errno));\n \t\treturn fd;\n \t}\n@@ -177,13 +178,13 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int die_on\n \torig_fd = open(path, O_RDONLY);\n \tif (orig_fd < 0) {\n \t\tif (errno != ENOENT) {\n-\t\t\tif (die_on_error)\n+\t\t\tif (flags & LOCK_DIE_ON_ERROR)\n \t\t\t\tdie(\"cannot open '%s' for copying\", path);\n \t\t\tclose(fd);\n \t\t\treturn error(\"cannot open '%s' for copying\", path);\n \t\t}\n \t} else if (copy_fd(orig_fd, fd)) {\n-\t\tif (die_on_error)\n+\t\tif (flags & LOCK_DIE_ON_ERROR)\n \t\t\texit(128);\n \t\tclose(fd);\n \t\treturn -1;\n@@ -215,7 +216,10 @@ int commit_lock_file(struct lock_file *lk)\n \n int hold_locked_index(struct lock_file *lk, int die_on_error)\n {\n-\treturn hold_lock_file_for_update(lk, get_index_file(), die_on_error);\n+\treturn hold_lock_file_for_update(lk, get_index_file(),\n+\t\t\t\t\t die_on_error\n+\t\t\t\t\t ? LOCK_DIE_ON_ERROR\n+\t\t\t\t\t : 0);\n }\n \n void set_alternate_index_output(const char *name)\ndiff --git a/pack-refs.c b/pack-refs.c\nindex 848d311..2c76fb1 100644\n--- a/pack-refs.c\n+++ b/pack-refs.c\n@@ -89,7 +89,8 @@ int pack_refs(unsigned int flags)\n \tmemset(&cbdata, 0, sizeof(cbdata));\n \tcbdata.flags = flags;\n \n-\tfd = hold_lock_file_for_update(&packed, git_path(\"packed-refs\"), 1);\n+\tfd = hold_lock_file_for_update(&packed, git_path(\"packed-refs\"),\n+\t\t\t\t       LOCK_DIE_ON_ERROR);\n \tcbdata.refs_file = fdopen(fd, \"w\");\n \tif (!cbdata.refs_file)\n \t\tdie(\"unable to create ref-pack file structure (%s)\",\ndiff --git a/refs.c b/refs.c\nindex 39a3b23..5467e98 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -845,7 +845,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char\n \t\terror(\"unable to create directory for %s\", ref_file);\n \t\tgoto error_return;\n \t}\n-\tlock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, 1);\n+\tlock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file,\n+\t\t\t\t\t\t  LOCK_DIE_ON_ERROR);\n \n \treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n \ndiff --git a/rerere.c b/rerere.c\nindex 323e493..2b7a99d 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -346,7 +346,8 @@ int setup_rerere(struct string_list *merge_rr)\n \t\treturn -1;\n \n \tmerge_rr_path = xstrdup(git_path(\"MERGE_RR\"));\n-\tfd = hold_lock_file_for_update(&write_lock, merge_rr_path, 1);\n+\tfd = hold_lock_file_for_update(&write_lock, merge_rr_path,\n+\t\t\t\t       LOCK_DIE_ON_ERROR);\n \tread_rr(merge_rr);\n \treturn fd;\n }\ndiff --git a/sha1_file.c b/sha1_file.c\nindex e2cb342..5cfae5d 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -385,7 +385,7 @@ static void read_info_alternates(const char * relative_base, int depth)\n void add_to_alternates_file(const char *reference)\n {\n \tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n-\tint fd = hold_lock_file_for_append(lock, git_path(\"objects/info/alternates\"), 1);\n+\tint fd = hold_lock_file_for_append(lock, git_path(\"objects/info/alternates\"), LOCK_DIE_ON_ERROR);\n \tchar *alt = mkpath(\"%s/objects\\n\", reference);\n \twrite_or_die(fd, alt, strlen(alt));\n \tif (commit_lock_file(lock))\n-- \n1.6.0.2.734.gae0be\n"},{"id":"93316","messageId":"7v3aiuhl5a.fsf_-_@gitster.siamese.dyndns.org","threadId":"15916","inReplyTo":"7vfxmuhlad.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 3/3] Fix checkout not to clobber the branch when using symlinked HEAD upon detaching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-17T23:11:45Z","receivedAt":"2008-10-17T23:11:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When you are using core.prefersymlinkrefs (i.e. your \".git/HEAD\" is a\nsymlink to \"refs/heads/$current_branch\"), attempt to detach HEAD resulted\nin clobbering the tip of the current branch.\n\nThe offending callchain is:\n\n  update_ref(..., \"HEAD\", REF_NODEREF, ...);\n  -> lock_any_ref_for_update(\"HEAD\", ..., REF_NODEREF);\n   -> lock_ref_sha1_basic(\"HEAD\", ..., REF_NODEREF, ...);\n      . calls resolve_ref() to read HEAD to arrive at\n        refs/heads/master\n      . however, it notices REF_NODEREF and adjusts the ref to be updated\n        back to \"HEAD\";\n    -> hold_lock_file_for_update(..., \"HEAD\", 1);\n     -> lock_file(..., \"HEAD\");\n        . resolves symlink \"HEAD\" to \"refs/heads/master\", and\n          locks it!  This creates \"refs/heads/master.lock\", that is\n          then renamed to \"refs/heads/master\" when unlocked.\n\nThe behaviour of lock_file() to resolve symlink at this point in the code\ncomes from d58e8d3 (When locking in a symlinked repository, try to lock\nthe original, 2007-07-25), and as explained in the log message of that\ncommit, we cannot unconditionally remove it.\n\nThis patch fixes this.  It teaches lock_file() not to dereference the\nsymbolic link when LOCK_NODEREF is given, and uses this new flag in\nlock_ref_sha1_basic() when it is operating directly on HEAD (iow when\nREF_NODEREF was given to it).\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n I haven't tested it beyond running the full testsuite, which it does pass,\n but I can't give any more guarantee than that.  Testing is for wimps ;-)\n\n cache.h       |    1 +\n lockfile.c    |    3 ++-\n refs.c        |   10 ++++++----\n t/t7201-co.sh |    2 +-\n 4 files changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 941a9dc..8ab2fd8 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -412,6 +412,7 @@ struct lock_file {\n \tchar filename[PATH_MAX];\n };\n #define LOCK_DIE_ON_ERROR 1\n+#define LOCK_NODEREF 2\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n extern int commit_lock_file(struct lock_file *);\ndiff --git a/lockfile.c b/lockfile.c\nindex bc1b585..6d75608 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -130,7 +130,8 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t * subtract 5 from size to make sure there's room for adding\n \t * \".lock\" for the lock file name\n \t */\n-\tresolve_symlink(lk->filename, sizeof(lk->filename)-5);\n+\tif (!(flags & LOCK_NODEREF))\n+\t\tresolve_symlink(lk->filename, sizeof(lk->filename)-5);\n \tstrcat(lk->filename, \".lock\");\n \tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (0 <= lk->fd) {\ndiff --git a/refs.c b/refs.c\nindex 5467e98..9e422dc 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -790,7 +790,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char\n \tstruct ref_lock *lock;\n \tstruct stat st;\n \tint last_errno = 0;\n-\tint type;\n+\tint type, lflags;\n \tint mustexist = (old_sha1 && !is_null_sha1(old_sha1));\n \n \tlock = xcalloc(1, sizeof(struct ref_lock));\n@@ -830,8 +830,11 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char\n \n \tlock->lk = xcalloc(1, sizeof(struct lock_file));\n \n-\tif (flags & REF_NODEREF)\n+\tlflags = LOCK_DIE_ON_ERROR;\n+\tif (flags & REF_NODEREF) {\n \t\tref = orig_ref;\n+\t\tlflags |= LOCK_NODEREF;\n+\t}\n \tlock->ref_name = xstrdup(ref);\n \tlock->orig_ref_name = xstrdup(orig_ref);\n \tref_file = git_path(\"%s\", ref);\n@@ -845,9 +848,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char\n \t\terror(\"unable to create directory for %s\", ref_file);\n \t\tgoto error_return;\n \t}\n-\tlock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file,\n-\t\t\t\t\t\t  LOCK_DIE_ON_ERROR);\n \n+\tlock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);\n \treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n \n  error_return:\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 01304d7..d9a80aa 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -339,7 +339,7 @@ test_expect_success 'checkout w/--track from non-branch HEAD fails' '\n     test \"z$(git rev-parse master^0)\" = \"z$(git rev-parse HEAD)\"\n '\n \n-test_expect_failure 'detch a symbolic link HEAD' '\n+test_expect_success 'detch a symbolic link HEAD' '\n     git checkout master &&\n     git config --bool core.prefersymlinkrefs yes &&\n     git checkout side &&\n-- \n1.6.0.2.734.gae0be\n"},{"id":"93431","messageId":"20081019130050.GA1822@coredump.intra.peff.net","threadId":"15916","inReplyTo":"7vfxmuhlad.fsf@gitster.siamese.dyndns.org","subject":"Re: Detached checkout will clobber branch head when using symlink HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-10-19T13:00:50Z","receivedAt":"2008-10-19T13:00:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 17, 2008 at 04:08:42PM -0700, Junio C Hamano wrote:\n\n> I do not think that is a correct approach.\n> \n> The offending callchain is:\n> \n>   update_ref(..., \"HEAD\", REF_NODEREF, ...);\n>   -> lock_any_ref_for_update(\"HEAD\", ..., REF_NODEREF);\n>    -> lock_ref_sha1_basic(\"HEAD\", ..., REF_NODEREF, ...);\n>       . calls resolve_ref() to read HEAD to arrive at\n>         refs/heads/master\n>       . however, it notices REF_NODEREF and adjusts the ref to be updated\n>         back to \"HEAD\";\n>     -> hold_lock_file_for_update(..., \"HEAD\", 1);\n>      -> lock_file(..., \"HEAD\");\n>         . resolves symlink \"HEAD\" to \"refs/heads/master\", and\n>           locks it!  This creates \"refs/heads/master.lock\", that is\n>           then renamed to \"refs/heads/master\" when unlocked.\n> \n> In other words, the breakage is in lock_file() and not in resolve_ref().\n> The latter gives the same output to the caller whether the HEAD is\n> symbolic link or textual symref -- at least it should.\n\nTo be fair, my patch fixed the problem in lock_ref_sha1_basic, not\nresolve_ref. It merely asked resolve_ref to annotate the resolution to\nshow whether a symlink was found, since it already had that information.\n\nBut if I am reading your patch right, you are actually enabling detached\nHEAD in this instance, which is much better. Unfortunately, I had quite\na few conflicts in applying your patches on top of master (with or\nwithout the patch from \"checkout --track -b broken\"), and when I thought\nI had fixed up the result, the test actually still failed.\n\nSo I will take your word that it actually works, and that I screwed up\nresolving the conflicts.\n\n-Peff\n\nPS If you are rebasing or resolving anyway, as I suspect you will have\nto, there is a typo in the test: s/detch/detach/\n"},{"id":"93463","messageId":"7vskqs74yf.fsf@gitster.siamese.dyndns.org","threadId":"15916","inReplyTo":"20081019130050.GA1822@coredump.intra.peff.net","subject":"Re: Detached checkout will clobber branch head when using symlink HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-19T19:36:08Z","receivedAt":"2008-10-19T19:36:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> But if I am reading your patch right, you are actually enabling detached\n> HEAD in this instance, which is much better. Unfortunately, I had quite\n> a few conflicts in applying your patches on top of master (with or\n> without the patch from \"checkout --track -b broken\"), and when I thought\n> I had fixed up the result, the test actually still failed.\n>\n> So I will take your word that it actually works, and that I screwed up\n> resolving the conflicts.\n\nIt's the three-patch series that leads to 684968c (Fix checkout not to\nclobber the branch when using symlinked HEAD upon detaching, 2008-10-17).\n\n> PS If you are rebasing or resolving anyway, as I suspect you will have\n> to, there is a typo in the test: s/detch/detach/\n\nThanks; the series was only in 'pu', so I will.\n"}]}