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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 17, 2008, 23:11 UTC
Message-ID: <7v3aiuhl5a.fsf_-_@gitster.siamese.dyndns.org>
In-Reply-To: <7vfxmuhlad.fsf@gitster.siamese.dyndns.org>

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