Detached checkout will clobber branch head when using symlink HEAD

Detached checkout will clobber branch head when using symlink HEAD

[Matt Draisey]

The faulty code appears to be this

git 1.6.0.2.526.g5c283
builtin-checkout.c

>    481	static void update_refs_for_switch(struct checkout_opts *opts,

>    500		if (new->path) {
> 
>    511		} else if (strcmp(new->name, "HEAD")) {
>    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.

Re: Detached checkout will clobber branch head when using symlink HEAD

[Jeff King]

On Wed, Oct 15, 2008 at 02:24:47PM -0400, Matt Draisey wrote:

> 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).

---
diff --git a/refs.c b/refs.c
index b680750..43568e7 100644
--- a/refs.c
+++ b/refs.c
@@ -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) {
diff --git a/refs.h b/refs.h
index 06ad260..6356a6a 100644
--- a/refs.h
+++ b/refs.h
@@ -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,

Re: Detached checkout will clobber branch head when using symlink HEAD

[Matt Draisey]

On Thu, 2008-10-16 at 15:17 -0400, Jeff King wrote:

> 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.

Re: Detached checkout will clobber branch head when using symlink HEAD

[Nicolas Pitre]

On Thu, 16 Oct 2008, Matt Draisey wrote:

> On Thu, 2008-10-16 at 15:17 -0400, Jeff King wrote:
> 
> > 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.

A symlink HEAD and detached checkouts are simply incompatible.


Nicolas

Re: Detached checkout will clobber branch head when using symlink HEAD

[Jeff King]

On Thu, Oct 16, 2008 at 04:20:32PM -0400, Nicolas Pitre wrote:

> > 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

Re: Detached checkout will clobber branch head when using symlink HEAD

[Nicolas Pitre]

On Thu, 16 Oct 2008, Jeff King wrote:

> On Thu, Oct 16, 2008 at 04:20:32PM -0400, Nicolas Pitre wrote:
> 
> > > 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

Re: Detached checkout will clobber branch head when using symlink HEAD

[Matt Draisey]

On Thu, 2008-10-16 at 16:20 -0400, Nicolas Pitre wrote:
> 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.

Re: Detached checkout will clobber branch head when using symlink HEAD

[Nicolas Pitre]

On Thu, 16 Oct 2008, Matt Draisey wrote:

> On Thu, 2008-10-16 at 16:20 -0400, Nicolas Pitre wrote:
> > 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

Re: Detached checkout will clobber branch head when using symlink HEAD

[Jeff King]

On Thu, Oct 16, 2008 at 04:11:03PM -0400, Matt Draisey wrote:

> 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 <peff@peff.net>
---
 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

diff --git a/refs.c b/refs.c
index b680750..b4b3865 100644
--- a/refs.c
+++ b/refs.c
@@ -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) {
diff --git a/refs.h b/refs.h
index 06ad260..9b0dcd9 100644
--- a/refs.h
+++ b/refs.h
@@ -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,
diff --git a/t/t7202-checkout-symlink.sh b/t/t7202-checkout-symlink.sh
new file mode 100755
index 0000000..cf09f5f
--- /dev/null
+++ b/t/t7202-checkout-symlink.sh
@@ -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
-- 
1.6.0.2.711.gf1ba4.dirty

Re: Detached checkout will clobber branch head when using symlink HEAD

[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.

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.

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

[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 <gitster@pobox.com>
---
 t/t7201-co.sh |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index ee2cab6..01304d7 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -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) &&
-- 
1.6.0.2.734.gae0be

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

[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 <gitster@pobox.com>
---
 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(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index e2a7e48..b563a0d 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -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);
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 85509f5..21ce3e0 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -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);
diff --git a/builtin-revert.c b/builtin-revert.c
index 27881e9..e839387 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -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)
diff --git a/bundle.c b/bundle.c
index 00b2aab..7d17a1f 100644
--- a/bundle.c
+++ b/bundle.c
@@ -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));
diff --git a/cache.h b/cache.h
index 884fae8..941a9dc 100644
--- a/cache.h
+++ b/cache.h
@@ -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 *);
diff --git a/lockfile.c b/lockfile.c
index 4023797..bc1b585 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -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)
diff --git a/pack-refs.c b/pack-refs.c
index 848d311..2c76fb1 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -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)",
diff --git a/refs.c b/refs.c
index 39a3b23..5467e98 100644
--- a/refs.c
+++ b/refs.c
@@ -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;
 
diff --git a/rerere.c b/rerere.c
index 323e493..2b7a99d 100644
--- a/rerere.c
+++ b/rerere.c
@@ -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;
 }
diff --git a/sha1_file.c b/sha1_file.c
index e2cb342..5cfae5d 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -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))
-- 
1.6.0.2.734.gae0be

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

[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 <gitster@pobox.com>
---

 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(-)

diff --git a/cache.h b/cache.h
index 941a9dc..8ab2fd8 100644
--- a/cache.h
+++ b/cache.h
@@ -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 *);
diff --git a/lockfile.c b/lockfile.c
index bc1b585..6d75608 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -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) {
diff --git a/refs.c b/refs.c
index 5467e98..9e422dc 100644
--- a/refs.c
+++ b/refs.c
@@ -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:
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 01304d7..d9a80aa 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -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 &&
-- 
1.6.0.2.734.gae0be

Re: Detached checkout will clobber branch head when using symlink HEAD

[Jeff King]

On Fri, Oct 17, 2008 at 04:08:42PM -0700, Junio C Hamano wrote:

> 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.

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/

Re: Detached checkout will clobber branch head when using symlink HEAD

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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).

> 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.