Skip to contentSkip to conversation

Detached checkout will clobber branch head when using symlink HEAD

PATCH New Matt Draisey 4 participants 15 messages Jun 15, 2016

lore
Share & tools
lore

Matt Draisey

author
#0 lore raw

The faulty code appears to be this

git 1.6.0.2.526.g5c283
builtin-checkout.c

Quoted 5 lines
   481	static void update_refs_for_switch(struct checkout_opts *opts,

   500		if (new->path) {

   511		} else if (strcmp(new->name, "HEAD")) {
Quoted 8 lines
   512			update_ref(msg.buf, "HEAD", new->commit->object.sha1, NULL,
   513				   REF_NODEREF, DIE_ON_ERR);
   514			if (!opts->quiet) {
   515				if (old->path)
   516					fprintf(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);
   517				describe_detached_head("HEAD is now at", new->commit);
   518			}
   519		}

If HEAD is a symlink rather than a "ref:" style link this is really bad.

#1 lore raw
Quoted 2 lines · Matt Draisey

If HEAD is a symlink rather than a "ref:" style link this is really bad.

Hmm. I don't think we have shipped with a symlink HEAD for quite a long time. Using one obviously doesn't work with detached HEAD, but also would fail with packed refs. I don't know if we ever deprecated it.

Still, this is a pretty nasty effect, in that it kills whatever was in the existing ref. There is a fix below which refuses to lock any ref if it is a symlink and we have requested not to dereference (actually, it is overly cautious -- the symlink flag is triggered when we see a symlink anywhere in the chain, though we need only prevent the situation when the _first_ symref is a symlink. I don't know if it is worth differentiating, since they are both "should never happen anymore" situations, I think).

---

refs.c +5 -1
index b680750..43568e7 100644
@@ -447,7 +447,7 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
 				strcpy(ref_buffer, buffer);
 				ref = ref_buffer;
 				if (flag)
-					*flag |= REF_ISSYMREF;
+					*flag |= REF_ISSYMREF | REF_ISSYMLINK;
 				continue;
 			}
 		}

@@ -817,6 +817,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
 		}
 		ref = resolve_ref(orig_ref, lock->old_sha1, mustexist, &type);
 	}
+	if (type & REF_ISSYMLINK && flags & REF_NODEREF) {
+		error("unable to directly lock symbolic link '%s'", orig_ref);
+		goto error_return;
+	}
 	if (type_p)
 	    *type_p = type;
 	if (!ref) {
refs.h +1 -0
index 06ad260..6356a6a 100644
@@ -12,6 +12,7 @@ struct ref_lock {
 
 #define REF_ISSYMREF 01
 #define REF_ISPACKED 02
+#define REF_ISSYMLINK 04
 
 /*
  * Calls the specified function for each ref file until it returns nonzero,
author
#2 lore raw
Quoted 3 lines · Jeff King

Hmm. I don't think we have shipped with a symlink HEAD for quite a long time. Using one obviously doesn't work with detached HEAD, but also would fail with packed refs. I don't know if we ever deprecated it.

I am using the following system defaults:

core.prefersymlinkrefs=true
gc.packrefs=false

so almost all my git repositories are still using a symlink HEAD. I have some old scripts That I use occasionally and still depend on it. Using detached checkout is the only problem I've had.

#3 lore raw
Quoted 8 lines · Matt Draisey

I am using the following system defaults:

core.prefersymlinkrefs=true
gc.packrefs=false

so almost all my git repositories are still using a symlink HEAD. I have some old scripts That I use occasionally and still depend on it. Using detached checkout is the only problem I've had.

In your position I would consider updating my scripts. But I guess you could also try to work up a patch that makes detached HEAD work (replacing the symlink with a file, then bringing back the symlink when you're on a branch). In the meantime, here is a cleaned-up version of my patch, with a proper commit message and a test.

-- >8 --
do not clobber symlinked ref with detached HEAD

The default configuration for git uses a symref for HEAD. When we detach the HEAD, we can simply write the detached commit sha1 into the HEAD file.

It is still possible to use symlinks for HEAD (either by setting it up manually, or by using core.prefersymlinkrefs). In that case, moving to a detached HEAD is impossible, since we have nowhere to store the sha1.

However, the current code doesn't realize this and actually writes into the HEAD file anyway, meaning that it overwrites the value of the currently checked out branch.

Instead, let's detect in the locking mechanism that we have a symlink but the caller requested that we lock the original ref name instead of its linked destination. This has two advantages:

- we don't have to add an extra stat call, since we discover the symlink during normal ref resolution

- any code to update a ref should lock it, meaning that we should catch any other similar instances

Signed-off-by Jeff King <[email protected]>
---
 refs.c                      |    9 ++++++++-
 refs.h                      |    1 +
 t/t7202-checkout-symlink.sh |   19 +++++++++++++++++++
 3 files changed, 28 insertions(+), 1 deletions(-)
 create mode 100755 t/t7202-checkout-symlink.sh
refs.c +8 -1
index b680750..b4b3865 100644
@@ -446,8 +446,11 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
 				buffer[len] = 0;
 				strcpy(ref_buffer, buffer);
 				ref = ref_buffer;
-				if (flag)
+				if (flag) {
+					if (!(*flag & REF_ISSYMREF))
+						*flag |= REF_OUTER_IS_SYMLINK;
 					*flag |= REF_ISSYMREF;
+				}
 				continue;
 			}
 		}

@@ -817,6 +820,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
 		}
 		ref = resolve_ref(orig_ref, lock->old_sha1, mustexist, &type);
 	}
+	if (type & REF_OUTER_IS_SYMLINK && flags & REF_NODEREF) {
+		error("unable to directly lock symbolic link '%s'", orig_ref);
+		goto error_return;
+	}
 	if (type_p)
 	    *type_p = type;
 	if (!ref) {
refs.h +1 -0
index 06ad260..9b0dcd9 100644
@@ -12,6 +12,7 @@ struct ref_lock {
 
 #define REF_ISSYMREF 01
 #define REF_ISPACKED 02
+#define REF_OUTER_IS_SYMLINK 04
 
 /*
  * Calls the specified function for each ref file until it returns nonzero,
t/t7202-checkout-symlink.sh new file +19 -0
new file mode 100755
index 0000000..cf09f5f
@@ -0,0 +1,19 @@
+#!/bin/sh
+
+test_description='checkout with symlinked HEAD'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo one > file && git add file && git commit -m one &&
+	echo two > file && git add file && git commit -m two &&
+	ln -sf refs/heads/master .git/HEAD
+'
+
+test_expect_success 'checkout detached HEAD does not clobber ref' '
+	test_must_fail git checkout HEAD^ &&
+	echo two >expect &&
+	git log -1 --pretty=tformat:%s >actual
+	test_cmp actual expect
+'
+
+test_done
Signature · 1 line
1.6.0.2.711.gf1ba4.dirty
#4 lore raw

I do not think that is a correct approach.

The offending callchain is:

update_ref(..., "HEAD", REF_NODEREF, ...);
-> lock_any_ref_for_update("HEAD", ..., REF_NODEREF);
-> lock_ref_sha1_basic("HEAD", ..., REF_NODEREF, ...);
. calls resolve_ref() to read HEAD to arrive at
refs/heads/master
. however, it notices REF_NODEREF and adjusts the ref to be updated
back to "HEAD";
-> hold_lock_file_for_update(..., "HEAD", 1);
-> lock_file(..., "HEAD");
. resolves symlink "HEAD" to "refs/heads/master", and
locks it! This creates "refs/heads/master.lock", that is then renamed to "refs/heads/master" when unlocked.

In other words, the breakage is in lock_file() and not in resolve_ref(). The latter gives the same output to the caller whether the HEAD is symbolic link or textual symref -- at least it should.

The behaviour of lock_file() to resolve symlink at this point in the code comes from d58e8d3 (When locking in a symlinked repository, try to lock the original, 2007-07-25), and as explained in the log message of that commit, we cannot unconditionally remove it.

Three patches to fix this issue, that come on top of the fix to t7201 (--track from detached HEAD should fail) I sent out last night, will follow.

#5 lore raw
Quoted 17 lines · Junio C Hamano

I do not think that is a correct approach.

The offending callchain is:

update_ref(..., "HEAD", REF_NODEREF, ...);
-> lock_any_ref_for_update("HEAD", ..., REF_NODEREF);
-> lock_ref_sha1_basic("HEAD", ..., REF_NODEREF, ...);
. calls resolve_ref() to read HEAD to arrive at
refs/heads/master
. however, it notices REF_NODEREF and adjusts the ref to be updated
back to "HEAD";
-> hold_lock_file_for_update(..., "HEAD", 1);
-> lock_file(..., "HEAD");
. resolves symlink "HEAD" to "refs/heads/master", and
locks it! This creates "refs/heads/master.lock", that is then renamed to "refs/heads/master" when unlocked.

Quoted 3 lines

In other words, the breakage is in lock_file() and not in resolve_ref(). The latter gives the same output to the caller whether the HEAD is symbolic link or textual symref -- at least it should.

To be fair, my patch fixed the problem in lock_ref_sha1_basic, not resolve_ref. It merely asked resolve_ref to annotate the resolution to show whether a symlink was found, since it already had that information.

But if I am reading your patch right, you are actually enabling detached HEAD in this instance, which is much better. Unfortunately, I had quite a few conflicts in applying your patches on top of master (with or without the patch from "checkout --track -b broken"), and when I thought I had fixed up the result, the test actually still failed.

So I will take your word that it actually works, and that I screwed up resolving the conflicts.

-Peff

PS If you are rebasing or resolving anyway, as I suspect you will have to, there is a typo in the test: s/detch/detach/

#6 lore raw
Quoted 8 lines · Jeff King

But if I am reading your patch right, you are actually enabling detached HEAD in this instance, which is much better. Unfortunately, I had quite a few conflicts in applying your patches on top of master (with or without the patch from "checkout --track -b broken"), and when I thought I had fixed up the result, the test actually still failed.

So I will take your word that it actually works, and that I screwed up resolving the conflicts.

It's the three-patch series that leads to 684968c (Fix checkout not to clobber the branch when using symlinked HEAD upon detaching, 2008-10-17).

Quoted 2 lines

PS If you are rebasing or resolving anyway, as I suspect you will have to, there is a typo in the test: s/detch/detach/

Thanks; the series was only in 'pu', so I will.

#7 lore raw

PATCH 1/3 demonstrate breakage of detached checkout with symbolic link HEAD

In reply to Junio C Hamano

When core.prefersymlinkrefs is in use, detaching the HEAD by checkout incorrectly clobbers the tip of the current branch.

Signed-off-by Junio C Hamano <[email protected]>
---
 t/t7201-co.sh |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
t/t7201-co.sh +12 -0
index ee2cab6..01304d7 100755
@@ -339,6 +339,18 @@ test_expect_success 'checkout w/--track from non-branch HEAD fails' '
     test "z$(git rev-parse master^0)" = "z$(git rev-parse HEAD)"
 '
 
+test_expect_failure 'detch a symbolic link HEAD' '
+    git checkout master &&
+    git config --bool core.prefersymlinkrefs yes &&
+    git checkout side &&
+    git checkout master &&
+    it=$(git symbolic-ref HEAD) &&
+    test "z$it" = zrefs/heads/master &&
+    here=$(git rev-parse --verify refs/heads/master) &&
+    git checkout side^ &&
+    test "z$(git rev-parse --verify refs/heads/master)" = "z$here"
+'
+
 test_expect_success 'checkout an unmerged path should fail' '
 	rm -f .git/index &&
 	O=$(echo original | git hash-object -w --stdin) &&
Signature · 1 line
1.6.0.2.734.gae0be
#8 lore raw

PATCH 2/3 Enhance hold_lock_file_for_{update,append}() API

In reply to Junio C Hamano

This changes the "die_on_error" boolean parameter to a mere "flags", and changes the existing callers of hold_lock_file_for_update/append() functions to pass LOCK_DIE_ON_ERROR.

Signed-off-by Junio C Hamano <[email protected]>
---
 builtin-commit.c     |    3 ++-
 builtin-fetch-pack.c |    3 ++-
 builtin-revert.c     |    3 ++-
 bundle.c             |    3 ++-
 cache.h              |    1 +
 lockfile.c           |   26 +++++++++++++++-----------
 pack-refs.c          |    3 ++-
 refs.c               |    3 ++-
 rerere.c             |    3 ++-
 sha1_file.c          |    2 +-
 10 files changed, 31 insertions(+), 19 deletions(-)
builtin-commit.c +2 -1
index e2a7e48..b563a0d 100644
@@ -320,7 +320,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 		die("unable to write new_index file");
 
 	fd = hold_lock_file_for_update(&false_lock,
-				       git_path("next-index-%d", getpid()), 1);
+				       git_path("next-index-%d", getpid()),
+				       LOCK_DIE_ON_ERROR);
 
 	create_base_index();
 	add_remove_files(&partial);
builtin-fetch-pack.c +2 -1
index 85509f5..21ce3e0 100644
@@ -813,7 +813,8 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 			  )
 			die("shallow file was changed during fetch");
 
-		fd = hold_lock_file_for_update(&lock, shallow, 1);
+		fd = hold_lock_file_for_update(&lock, shallow,
+					       LOCK_DIE_ON_ERROR);
 		if (!write_shallow_commits(fd, 0)) {
 			unlink(shallow);
 			rollback_lock_file(&lock);
builtin-revert.c +2 -1
index 27881e9..e839387 100644
@@ -338,7 +338,8 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	 * reverse of it if we are revert.
 	 */
 
-	msg_fd = hold_lock_file_for_update(&msg_file, defmsg, 1);
+	msg_fd = hold_lock_file_for_update(&msg_file, defmsg,
+					   LOCK_DIE_ON_ERROR);
 
 	encoding = get_encoding(message);
 	if (!encoding)
bundle.c +2 -1
index 00b2aab..7d17a1f 100644
@@ -186,7 +186,8 @@ int create_bundle(struct bundle_header *header, const char *path,
 	if (bundle_to_stdout)
 		bundle_fd = 1;
 	else
-		bundle_fd = hold_lock_file_for_update(&lock, path, 1);
+		bundle_fd = hold_lock_file_for_update(&lock, path,
+						      LOCK_DIE_ON_ERROR);
 
 	/* write signature */
 	write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));
cache.h +1 -0
index 884fae8..941a9dc 100644
@@ -411,6 +411,7 @@ struct lock_file {
 	char on_list;
 	char filename[PATH_MAX];
 };
+#define LOCK_DIE_ON_ERROR 1
 extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
 extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
 extern int commit_lock_file(struct lock_file *);
lockfile.c +15 -11
index 4023797..bc1b585 100644
@@ -121,9 +121,10 @@ static char *resolve_symlink(char *p, size_t s)
 }
 
 
-static int lock_file(struct lock_file *lk, const char *path)
+static int lock_file(struct lock_file *lk, const char *path, int flags)
 {
-	if (strlen(path) >= sizeof(lk->filename)) return -1;
+	if (strlen(path) >= sizeof(lk->filename))
+		return -1;
 	strcpy(lk->filename, path);
 	/*
 	 * subtract 5 from size to make sure there's room for adding

@@ -155,21 +156,21 @@ static int lock_file(struct lock_file *lk, const char *path)
 	return lk->fd;
 }
 
-int hold_lock_file_for_update(struct lock_file *lk, const char *path, int die_on_error)
+int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
 {
-	int fd = lock_file(lk, path);
-	if (fd < 0 && die_on_error)
+	int fd = lock_file(lk, path, flags);
+	if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
 		die("unable to create '%s.lock': %s", path, strerror(errno));
 	return fd;
 }
 
-int hold_lock_file_for_append(struct lock_file *lk, const char *path, int die_on_error)
+int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)
 {
 	int fd, orig_fd;
 
-	fd = lock_file(lk, path);
+	fd = lock_file(lk, path, flags);
 	if (fd < 0) {
-		if (die_on_error)
+		if (flags & LOCK_DIE_ON_ERROR)
 			die("unable to create '%s.lock': %s", path, strerror(errno));
 		return fd;
 	}

@@ -177,13 +178,13 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int die_on
 	orig_fd = open(path, O_RDONLY);
 	if (orig_fd < 0) {
 		if (errno != ENOENT) {
-			if (die_on_error)
+			if (flags & LOCK_DIE_ON_ERROR)
 				die("cannot open '%s' for copying", path);
 			close(fd);
 			return error("cannot open '%s' for copying", path);
 		}
 	} else if (copy_fd(orig_fd, fd)) {
-		if (die_on_error)
+		if (flags & LOCK_DIE_ON_ERROR)
 			exit(128);
 		close(fd);
 		return -1;

@@ -215,7 +216,10 @@ int commit_lock_file(struct lock_file *lk)
 
 int hold_locked_index(struct lock_file *lk, int die_on_error)
 {
-	return hold_lock_file_for_update(lk, get_index_file(), die_on_error);
+	return hold_lock_file_for_update(lk, get_index_file(),
+					 die_on_error
+					 ? LOCK_DIE_ON_ERROR
+					 : 0);
 }
 
 void set_alternate_index_output(const char *name)
pack-refs.c +2 -1
index 848d311..2c76fb1 100644
@@ -89,7 +89,8 @@ int pack_refs(unsigned int flags)
 	memset(&cbdata, 0, sizeof(cbdata));
 	cbdata.flags = flags;
 
-	fd = hold_lock_file_for_update(&packed, git_path("packed-refs"), 1);
+	fd = hold_lock_file_for_update(&packed, git_path("packed-refs"),
+				       LOCK_DIE_ON_ERROR);
 	cbdata.refs_file = fdopen(fd, "w");
 	if (!cbdata.refs_file)
 		die("unable to create ref-pack file structure (%s)",
refs.c +2 -1
index 39a3b23..5467e98 100644
@@ -845,7 +845,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
 		error("unable to create directory for %s", ref_file);
 		goto error_return;
 	}
-	lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, 1);
+	lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file,
+						  LOCK_DIE_ON_ERROR);
 
 	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
rerere.c +2 -1
index 323e493..2b7a99d 100644
@@ -346,7 +346,8 @@ int setup_rerere(struct string_list *merge_rr)
 		return -1;
 
 	merge_rr_path = xstrdup(git_path("MERGE_RR"));
-	fd = hold_lock_file_for_update(&write_lock, merge_rr_path, 1);
+	fd = hold_lock_file_for_update(&write_lock, merge_rr_path,
+				       LOCK_DIE_ON_ERROR);
 	read_rr(merge_rr);
 	return fd;
 }
sha1_file.c +1 -1
index e2cb342..5cfae5d 100644
@@ -385,7 +385,7 @@ static void read_info_alternates(const char * relative_base, int depth)
 void add_to_alternates_file(const char *reference)
 {
 	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
-	int fd = hold_lock_file_for_append(lock, git_path("objects/info/alternates"), 1);
+	int fd = hold_lock_file_for_append(lock, git_path("objects/info/alternates"), LOCK_DIE_ON_ERROR);
 	char *alt = mkpath("%s/objects\n", reference);
 	write_or_die(fd, alt, strlen(alt));
 	if (commit_lock_file(lock))
Signature · 1 line
1.6.0.2.734.gae0be
#9 lore raw

PATCH 3/3 Fix checkout not to clobber the branch when using symlinked HEAD upon detaching

In reply to Junio C Hamano

When you are using core.prefersymlinkrefs (i.e. your ".git/HEAD" is a symlink to "refs/heads/$current_branch"), attempt to detach HEAD resulted in clobbering the tip of the current branch.

The offending callchain is:

update_ref(..., "HEAD", REF_NODEREF, ...);
-> lock_any_ref_for_update("HEAD", ..., REF_NODEREF);
-> lock_ref_sha1_basic("HEAD", ..., REF_NODEREF, ...);
. calls resolve_ref() to read HEAD to arrive at
refs/heads/master
. however, it notices REF_NODEREF and adjusts the ref to be updated
back to "HEAD";
-> hold_lock_file_for_update(..., "HEAD", 1);
-> lock_file(..., "HEAD");
. resolves symlink "HEAD" to "refs/heads/master", and
locks it! This creates "refs/heads/master.lock", that is then renamed to "refs/heads/master" when unlocked.

The behaviour of lock_file() to resolve symlink at this point in the code comes from d58e8d3 (When locking in a symlinked repository, try to lock the original, 2007-07-25), and as explained in the log message of that commit, we cannot unconditionally remove it.

This patch fixes this. It teaches lock_file() not to dereference the symbolic link when LOCK_NODEREF is given, and uses this new flag in lock_ref_sha1_basic() when it is operating directly on HEAD (iow when REF_NODEREF was given to it).

Signed-off-by Junio C Hamano <[email protected]>

---

I haven't tested it beyond running the full testsuite, which it does pass, but I can't give any more guarantee than that. Testing is for wimps ;-)

 cache.h       |    1 +
 lockfile.c    |    3 ++-
 refs.c        |   10 ++++++----
 t/t7201-co.sh |    2 +-
 4 files changed, 10 insertions(+), 6 deletions(-)
cache.h +1 -0
index 941a9dc..8ab2fd8 100644
@@ -412,6 +412,7 @@ struct lock_file {
 	char filename[PATH_MAX];
 };
 #define LOCK_DIE_ON_ERROR 1
+#define LOCK_NODEREF 2
 extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
 extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
 extern int commit_lock_file(struct lock_file *);
lockfile.c +2 -1
index bc1b585..6d75608 100644
@@ -130,7 +130,8 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 	 * subtract 5 from size to make sure there's room for adding
 	 * ".lock" for the lock file name
 	 */
-	resolve_symlink(lk->filename, sizeof(lk->filename)-5);
+	if (!(flags & LOCK_NODEREF))
+		resolve_symlink(lk->filename, sizeof(lk->filename)-5);
 	strcat(lk->filename, ".lock");
 	lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);
 	if (0 <= lk->fd) {
refs.c +6 -4
index 5467e98..9e422dc 100644
@@ -790,7 +790,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
 	struct ref_lock *lock;
 	struct stat st;
 	int last_errno = 0;
-	int type;
+	int type, lflags;
 	int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
 
 	lock = xcalloc(1, sizeof(struct ref_lock));

@@ -830,8 +830,11 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
 
 	lock->lk = xcalloc(1, sizeof(struct lock_file));
 
-	if (flags & REF_NODEREF)
+	lflags = LOCK_DIE_ON_ERROR;
+	if (flags & REF_NODEREF) {
 		ref = orig_ref;
+		lflags |= LOCK_NODEREF;
+	}
 	lock->ref_name = xstrdup(ref);
 	lock->orig_ref_name = xstrdup(orig_ref);
 	ref_file = git_path("%s", ref);

@@ -845,9 +848,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
 		error("unable to create directory for %s", ref_file);
 		goto error_return;
 	}
-	lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file,
-						  LOCK_DIE_ON_ERROR);
 
+	lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);
 	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
 
  error_return:
t/t7201-co.sh +1 -1
index 01304d7..d9a80aa 100755
@@ -339,7 +339,7 @@ test_expect_success 'checkout w/--track from non-branch HEAD fails' '
     test "z$(git rev-parse master^0)" = "z$(git rev-parse HEAD)"
 '
 
-test_expect_failure 'detch a symbolic link HEAD' '
+test_expect_success 'detch a symbolic link HEAD' '
     git checkout master &&
     git config --bool core.prefersymlinkrefs yes &&
     git checkout side &&
Signature · 1 line
1.6.0.2.734.gae0be
#10 lore raw
In reply to Matt Draisey
Quoted 6 lines · Matt Draisey
Quoted 3 lines · Jeff King

Hmm. I don't think we have shipped with a symlink HEAD for quite a long time. Using one obviously doesn't work with detached HEAD, but also would fail with packed refs. I don't know if we ever deprecated it.

Quoted 8 lines

I am using the following system defaults:

core.prefersymlinkrefs=true
gc.packrefs=false

so almost all my git repositories are still using a symlink HEAD. I have some old scripts That I use occasionally and still depend on it. Using detached checkout is the only problem I've had.

A symlink HEAD and detached checkouts are simply incompatible.

Nicolas

author
#11 lore raw
Quoted 1 line · Nicolas Pitre

A symlink HEAD and detached checkouts are simply incompatible.

Not necessarily. The symlinking code will unlink the original inode each time it creates a new symlink anyway. It is simply a matter of creating a new file in its place.

#12 lore raw
Quoted 6 lines · Matt Draisey
Quoted 1 line · Nicolas Pitre

A symlink HEAD and detached checkouts are simply incompatible.

Not necessarily. The symlinking code will unlink the original inode each time it creates a new symlink anyway. It is simply a matter of creating a new file in its place.

True. I didn't think it all through initially.

Nicolas

#13 lore raw
In reply to Nicolas Pitre
Quoted 5 lines · Nicolas Pitre
Quoted 3 lines

so almost all my git repositories are still using a symlink HEAD. I have some old scripts That I use occasionally and still depend on it. Using detached checkout is the only problem I've had.

A symlink HEAD and detached checkouts are simply incompatible.

Agreed, but I think the complaint is not that it doesn't work, but that it silently clobbers the current branch when you try it.

-Peff

#14 lore raw
Quoted 10 lines · Jeff King
Quoted 5 lines · Nicolas Pitre
Quoted 3 lines

so almost all my git repositories are still using a symlink HEAD. I have some old scripts That I use occasionally and still depend on it. Using detached checkout is the only problem I've had.

A symlink HEAD and detached checkouts are simply incompatible.

Agreed, but I think the complaint is not that it doesn't work, but that it silently clobbers the current branch when you try it.

That is unacceptable indeed.

Nicolas