packed_ref_store: handle a packed-refs file that is a symlink

mhagger · gitster · commit 198b808e207e · 2017-07-27T10:19:56.000-07:00
One of the tricks that `contrib/workdir/git-new-workdir` plays is to making `packed-refs` in the new workdir a symlink to the `packed-refs` file in the original repository. Before 42dfa7e ("commit_packed_refs(): use a staging file separate from the lockfile", 2017-06-23), a lockfile was used as the staging file, and because the `LOCK_NO_DEREF` was not used, the pointed-to file was locked and modified. But after that commit, the staging file was created using a tempfile, with the end result that rewriting the `packed-refs` file in the workdir overwrote the symlink rather than the original `packed-refs` file. Change `commit_packed_refs()` to use `get_locked_file_path()` to find the path of the file that it should overwrite. Since that path was properly resolved when the lockfile was created, this restores the pre-42dfa7ecef behavior. Also add a test case to document this use case and prevent a regression like this from recurring. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
@@ -610,19 +610,27 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 	struct packed_ref_cache *packed_ref_cache =
 		get_packed_ref_cache(refs);
 	int ok;
+	int ret = -1;
 	struct strbuf sb = STRBUF_INIT;
 	FILE *out;
 	struct ref_iterator *iter;
+	char *packed_refs_path;
 
 	if (!is_lock_file_locked(&refs->lock))
 		die("BUG: commit_packed_refs() called when unlocked");
 
-	strbuf_addf(&sb, "%s.new", refs->path);
+	/*
+	 * If packed-refs is a symlink, we want to overwrite the
+	 * symlinked-to file, not the symlink itself. Also, put the
+	 * staging file next to it:
+	 */
+	packed_refs_path = get_locked_file_path(&refs->lock);
+	strbuf_addf(&sb, "%s.new", packed_refs_path);
 	if (create_tempfile(&refs->tempfile, sb.buf) < 0) {
 		strbuf_addf(err, "unable to create file %s: %s",
 			    sb.buf, strerror(errno));
 		strbuf_release(&sb);
-		return -1;
+		goto out;
 	}
 	strbuf_release(&sb);
 
@@ -660,17 +668,21 @@ int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 		goto error;
 	}
 
-	if (rename_tempfile(&refs->tempfile, refs->path)) {
+	if (rename_tempfile(&refs->tempfile, packed_refs_path)) {
 		strbuf_addf(err, "error replacing %s: %s",
 			    refs->path, strerror(errno));
-		return -1;
+		goto out;
 	}
 
-	return 0;
+	ret = 0;
+	goto out;
 
 error:
 	delete_tempfile(&refs->tempfile);
-	return -1;
+
+out:
+	free(packed_refs_path);
+	return ret;
 }
 
 /*
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
@@ -238,4 +238,19 @@ test_expect_success 'retry acquiring packed-refs.lock' '
 	git -c core.packedrefstimeout=3000 pack-refs --all --prune
 '
 
+test_expect_success SYMLINKS 'pack symlinked packed-refs' '
+	# First make sure that symlinking works when reading:
+	git update-ref refs/heads/loosy refs/heads/master &&
+	git for-each-ref >all-refs-before &&
+	mv .git/packed-refs .git/my-deviant-packed-refs &&
+	ln -s my-deviant-packed-refs .git/packed-refs &&
+	git for-each-ref >all-refs-linked &&
+	test_cmp all-refs-before all-refs-linked &&
+	git pack-refs --all --prune &&
+	git for-each-ref >all-refs-packed &&
+	test_cmp all-refs-before all-refs-packed &&
+	test -h .git/packed-refs &&
+	test "$(readlink .git/packed-refs)" = "my-deviant-packed-refs"
+'
+
 test_done
