Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Jeff King <peff@peff.net>]

On Tue, Mar 27, 2018 at 04:56:00PM +0200, Duy Nguyen wrote:

> The way setup_work_tree() does it is just bad to me. When it calls
> set_git_dir() again, the assumption is the new path is exactly the
> same as the old one. The only difference is relative vs absolute. But
> this is super hard to see from set_git_dir implementation. The new
> struct repository design also inherits this (i.e. allowing to call
> set_git_dir multiple times, which really does not make sense), but
> this won't fly long term. When cwd moves, all cached relative paths
> must be adjusted, we need a better mechanism to tell everybody that,
> not just do it for $GIT_DIR and related paths.

Yeah, I agree it's questionable.

> I am planning to fix this. This is part of the "setup cleanup" effort
> to keep repository.c design less messy and hopefully unify how the
> main repo and submodule repo are created/set up. But frankly that may
> take a long while before I could submit anything substantial (I also
> have the "report multiple worktree's HEADs correctly and make fsck
> count all HEADs" task, which is not small and to me have higher
> priority).
> 
> So I would not mind papering over it right now (with an understanding
> that absolute path pays some more overhead in path walking, which was
> th reason we tried to avoid it in setup code). A slightly better patch
> is trigger this path absolutization from setup_work_tree(), near
> set_git_dir(). But then you face some ugliness there: how to find out
> all ref stores to update, or just update the main ref store alone.

I don't quite get why f57f37e2 doesn't want to call git_path(). Is it to
avoid the way the path is munged? Or is it to avoid some lazy-setup that
is triggered by calling get_git_dir() at all (which doesn't make much
sense to me, because we'd already have called get_git_dir() much
earlier). Or is it just because we may sometimes fill in refs->git_dir
with something besides get_git_dir() (for a submodule or worktree or
something)?

I.e., can we do one of (depending on which of those answers is "yes"):

  1. Stop caching the value of get_git_dir(), and instead call it
     on-demand instead of looking at refs->git_dir? (If we just want to
     avoid git_path()).

  2. If we need to avoid even calling get_git_dir(), can we add a
     "light" version of it that avoids whatever side effects we're
     trying to skip?

  3. If the problem is just that sometimes we need get_git_dir() and
     sometimes not, could we perhaps store NULL as a sentinel to mean
     "look up get_git_dir() when you need it"?

     That would let submodules and worktrees fill in their paths as
     necessary (assuming they never change after init), but handle the
     case of get_git_dir() changing.

Hmm. Typing that out, it seems like (3) is probably the right path.
Something like the patch below seems to fix it and passes the tests.

diff --git a/refs.c b/refs.c
index 20ba82b434..4094f0e7d4 100644
--- a/refs.c
+++ b/refs.c
@@ -1656,7 +1656,7 @@ struct ref_store *get_main_ref_store(void)
 	if (main_ref_store)
 		return main_ref_store;
 
-	main_ref_store = ref_store_init(get_git_dir(), REF_STORE_ALL_CAPS);
+	main_ref_store = ref_store_init(NULL, REF_STORE_ALL_CAPS);
 	return main_ref_store;
 }
 
diff --git a/refs/files-backend.c b/refs/files-backend.c
index bec8e30e9e..22118b5764 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -69,6 +69,7 @@ struct files_ref_store {
 	struct ref_store base;
 	unsigned int store_flags;
 
+	/* may be NULL to force lookup of get_git_dir() */
 	char *gitdir;
 	char *gitcommondir;
 
@@ -94,17 +95,23 @@ static struct ref_store *files_ref_store_create(const char *gitdir,
 {
 	struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
 	struct ref_store *ref_store = (struct ref_store *)refs;
-	struct strbuf sb = STRBUF_INIT;
 
 	base_ref_store_init(ref_store, &refs_be_files);
 	refs->store_flags = flags;
 
-	refs->gitdir = xstrdup(gitdir);
-	get_common_dir_noenv(&sb, gitdir);
-	refs->gitcommondir = strbuf_detach(&sb, NULL);
-	strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir);
-	refs->packed_ref_store = packed_ref_store_create(sb.buf, flags);
-	strbuf_release(&sb);
+	if (gitdir) {
+		struct strbuf sb = STRBUF_INIT;
+		refs->gitdir = xstrdup(gitdir);
+		get_common_dir_noenv(&sb, gitdir);
+		refs->gitcommondir = strbuf_detach(&sb, NULL);
+		strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir);
+		refs->packed_ref_store = packed_ref_store_create(sb.buf, flags);
+		strbuf_release(&sb);
+	} else {
+		refs->gitdir = NULL;
+		refs->gitcommondir = NULL;
+		refs->packed_ref_store = packed_ref_store_create(NULL, flags);
+	}
 
 	return ref_store;
 }
@@ -147,6 +154,16 @@ static struct files_ref_store *files_downcast(struct ref_store *ref_store,
 	return refs;
 }
 
+static const char *files_gitdir(const struct files_ref_store *refs)
+{
+	return refs->gitdir ? refs->gitdir : get_git_dir();
+}
+
+static const char *files_gitcommondir(const struct files_ref_store *refs)
+{
+	return refs->gitcommondir ? refs->gitcommondir : get_git_common_dir();
+}
+
 static void files_reflog_path(struct files_ref_store *refs,
 			      struct strbuf *sb,
 			      const char *refname)
@@ -154,10 +171,10 @@ static void files_reflog_path(struct files_ref_store *refs,
 	switch (ref_type(refname)) {
 	case REF_TYPE_PER_WORKTREE:
 	case REF_TYPE_PSEUDOREF:
-		strbuf_addf(sb, "%s/logs/%s", refs->gitdir, refname);
+		strbuf_addf(sb, "%s/logs/%s", files_gitdir(refs), refname);
 		break;
 	case REF_TYPE_NORMAL:
-		strbuf_addf(sb, "%s/logs/%s", refs->gitcommondir, refname);
+		strbuf_addf(sb, "%s/logs/%s", files_gitcommondir(refs), refname);
 		break;
 	default:
 		die("BUG: unknown ref type %d of ref %s",
@@ -172,10 +189,10 @@ static void files_ref_path(struct files_ref_store *refs,
 	switch (ref_type(refname)) {
 	case REF_TYPE_PER_WORKTREE:
 	case REF_TYPE_PSEUDOREF:
-		strbuf_addf(sb, "%s/%s", refs->gitdir, refname);
+		strbuf_addf(sb, "%s/%s", files_gitdir(refs), refname);
 		break;
 	case REF_TYPE_NORMAL:
-		strbuf_addf(sb, "%s/%s", refs->gitcommondir, refname);
+		strbuf_addf(sb, "%s/%s", files_gitcommondir(refs), refname);
 		break;
 	default:
 		die("BUG: unknown ref type %d of ref %s",
@@ -2151,13 +2168,13 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st
 		files_downcast(ref_store, REF_STORE_READ,
 			       "reflog_iterator_begin");
 
-	if (!strcmp(refs->gitdir, refs->gitcommondir)) {
-		return reflog_iterator_begin(ref_store, refs->gitcommondir);
+	if (!strcmp(files_gitdir(refs), files_gitcommondir(refs))) {
+		return reflog_iterator_begin(ref_store, files_gitcommondir(refs));
 	} else {
 		return merge_ref_iterator_begin(
 			0,
-			reflog_iterator_begin(ref_store, refs->gitdir),
-			reflog_iterator_begin(ref_store, refs->gitcommondir),
+			reflog_iterator_begin(ref_store, files_gitdir(refs)),
+			reflog_iterator_begin(ref_store, files_gitcommondir(refs)),
 			reflog_iterator_select, refs);
 	}
 }
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 65288c6472..e5a5af1285 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -158,6 +158,16 @@ static void acquire_snapshot(struct snapshot *snapshot)
 	snapshot->referrers++;
 }
 
+static const char *packed_refs_path(const struct packed_ref_store *refs)
+{
+	return refs->path ? refs->path : git_path("packed-refs");
+}
+
+static const char *snapshot_path(const struct snapshot *snapshot)
+{
+	return packed_refs_path(snapshot->refs);
+}
+
 /*
  * If the buffer in `snapshot` is active, then either munmap the
  * memory and close the file, or free the memory. Then set the buffer
@@ -168,7 +178,7 @@ static void clear_snapshot_buffer(struct snapshot *snapshot)
 	if (snapshot->mmapped) {
 		if (munmap(snapshot->buf, snapshot->eof - snapshot->buf))
 			die_errno("error ummapping packed-refs file %s",
-				  snapshot->refs->path);
+				  snapshot_path(snapshot));
 		snapshot->mmapped = 0;
 	} else {
 		free(snapshot->buf);
@@ -201,7 +211,7 @@ struct ref_store *packed_ref_store_create(const char *path,
 	base_ref_store_init(ref_store, &refs_be_packed);
 	refs->store_flags = store_flags;
 
-	refs->path = xstrdup(path);
+	refs->path = xstrdup_or_null(path);
 	return ref_store;
 }
 
@@ -342,7 +352,7 @@ static void sort_snapshot(struct snapshot *snapshot)
 			/* The safety check should prevent this. */
 			BUG("unterminated line found in packed-refs");
 		if (eol - pos < GIT_SHA1_HEXSZ + 2)
-			die_invalid_line(snapshot->refs->path,
+			die_invalid_line(snapshot_path(snapshot),
 					 pos, eof - pos);
 		eol++;
 		if (eol < eof && *eol == '^') {
@@ -454,7 +464,7 @@ static void verify_buffer_safe(struct snapshot *snapshot)
 
 	last_line = find_start_of_record(start, eof - 1);
 	if (*(eof - 1) != '\n' || eof - last_line < GIT_SHA1_HEXSZ + 2)
-		die_invalid_line(snapshot->refs->path,
+		die_invalid_line(snapshot_path(snapshot),
 				 last_line, eof - last_line);
 }
 
@@ -473,7 +483,7 @@ static int load_contents(struct snapshot *snapshot)
 	size_t size;
 	ssize_t bytes_read;
 
-	fd = open(snapshot->refs->path, O_RDONLY);
+	fd = open(snapshot_path(snapshot), O_RDONLY);
 	if (fd < 0) {
 		if (errno == ENOENT) {
 			/*
@@ -485,14 +495,14 @@ static int load_contents(struct snapshot *snapshot)
 			 */
 			return 0;
 		} else {
-			die_errno("couldn't read %s", snapshot->refs->path);
+			die_errno("couldn't read %s", snapshot_path(snapshot));
 		}
 	}
 
 	stat_validity_update(&snapshot->validity, fd);
 
 	if (fstat(fd, &st) < 0)
-		die_errno("couldn't stat %s", snapshot->refs->path);
+		die_errno("couldn't stat %s", snapshot_path(snapshot));
 	size = xsize_t(st.st_size);
 
 	if (!size) {
@@ -501,7 +511,7 @@ static int load_contents(struct snapshot *snapshot)
 		snapshot->buf = xmalloc(size);
 		bytes_read = read_in_full(fd, snapshot->buf, size);
 		if (bytes_read < 0 || bytes_read != size)
-			die_errno("couldn't read %s", snapshot->refs->path);
+			die_errno("couldn't read %s", snapshot_path(snapshot));
 		snapshot->mmapped = 0;
 	} else {
 		snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
@@ -629,14 +639,14 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs)
 		eol = memchr(snapshot->buf, '\n',
 			     snapshot->eof - snapshot->buf);
 		if (!eol)
-			die_unterminated_line(refs->path,
+			die_unterminated_line(packed_refs_path(refs),
 					      snapshot->buf,
 					      snapshot->eof - snapshot->buf);
 
 		tmp = xmemdupz(snapshot->buf, eol - snapshot->buf);
 
 		if (!skip_prefix(tmp, "# pack-refs with:", (const char **)&p))
-			die_invalid_line(refs->path,
+			die_invalid_line(packed_refs_path(refs),
 					 snapshot->buf,
 					 snapshot->eof - snapshot->buf);
 
@@ -695,7 +705,8 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs)
 static void validate_snapshot(struct packed_ref_store *refs)
 {
 	if (refs->snapshot &&
-	    !stat_validity_check(&refs->snapshot->validity, refs->path))
+	    !stat_validity_check(&refs->snapshot->validity,
+				 packed_refs_path(refs)))
 		clear_snapshot(refs);
 }
 
@@ -739,7 +750,7 @@ static int packed_read_raw_ref(struct ref_store *ref_store,
 	}
 
 	if (get_oid_hex(rec, oid))
-		die_invalid_line(refs->path, rec, snapshot->eof - rec);
+		die_invalid_line(packed_refs_path(refs), rec, snapshot->eof - rec);
 
 	*type = REF_ISPACKED;
 	return 0;
@@ -795,12 +806,12 @@ static int next_record(struct packed_ref_iterator *iter)
 	if (iter->eof - p < GIT_SHA1_HEXSZ + 2 ||
 	    parse_oid_hex(p, &iter->oid, &p) ||
 	    !isspace(*p++))
-		die_invalid_line(iter->snapshot->refs->path,
+		die_invalid_line(snapshot_path(iter->snapshot),
 				 iter->pos, iter->eof - iter->pos);
 
 	eol = memchr(p, '\n', iter->eof - p);
 	if (!eol)
-		die_unterminated_line(iter->snapshot->refs->path,
+		die_unterminated_line(snapshot_path(iter->snapshot),
 				      iter->pos, iter->eof - iter->pos);
 
 	strbuf_add(&iter->refname_buf, p, eol - p);
@@ -825,7 +836,7 @@ static int next_record(struct packed_ref_iterator *iter)
 		if (iter->eof - p < GIT_SHA1_HEXSZ + 1 ||
 		    parse_oid_hex(p, &iter->peeled, &p) ||
 		    *p++ != '\n')
-			die_invalid_line(iter->snapshot->refs->path,
+			die_invalid_line(snapshot_path(iter->snapshot),
 					 iter->pos, iter->eof - iter->pos);
 		iter->pos = p;
 
@@ -995,14 +1006,14 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
 	 */
 	if (hold_lock_file_for_update_timeout(
 			    &refs->lock,
-			    refs->path,
+			    packed_refs_path(refs),
 			    flags, timeout_value) < 0) {
-		unable_to_lock_message(refs->path, errno, err);
+		unable_to_lock_message(packed_refs_path(refs), errno, err);
 		return -1;
 	}
 
 	if (close_lock_file_gently(&refs->lock)) {
-		strbuf_addf(err, "unable to close %s: %s", refs->path, strerror(errno));
+		strbuf_addf(err, "unable to close %s: %s", packed_refs_path(refs), strerror(errno));
 		rollback_lock_file(&refs->lock);
 		return -1;
 	}
@@ -1471,21 +1482,21 @@ static int packed_transaction_finish(struct ref_store *ref_store,
 			REF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB,
 			"ref_transaction_finish");
 	int ret = TRANSACTION_GENERIC_ERROR;
-	char *packed_refs_path;
+	char *path;
 
 	clear_snapshot(refs);
 
-	packed_refs_path = get_locked_file_path(&refs->lock);
-	if (rename_tempfile(&refs->tempfile, packed_refs_path)) {
+	path = get_locked_file_path(&refs->lock);
+	if (rename_tempfile(&refs->tempfile, path)) {
 		strbuf_addf(err, "error replacing %s: %s",
-			    refs->path, strerror(errno));
+			    path, strerror(errno));
 		goto cleanup;
 	}
 
 	ret = 0;
 
 cleanup:
-	free(packed_refs_path);
+	free(path);
 	packed_transaction_cleanup(refs, transaction);
 	return ret;
 }