[PATCH v2 0/9] Store submodules in a hash, not a linked list

[PATCH v2 0/9] Store submodules in a hash, not a linked list

[Michael Haggerty]

This is v2 of the patch series, considerably reorganized but not that
different codewise. Thanks to Stefan, Junio, and Peff for their
feedback about v1 [1]. I think I have addressed all of your comments.

Changes since v1:

* Rebase from `master` onto `maint` (to match Junio).

* Reorder some commits to make the presentation more logical.

* Added an explicit preparatory commit that just reorders some
  function definitions, because it makes the diff for the subsequent
  commit a lot easier to read.

* Make some preexisting functions private:
  * lookup_ref_store()
  * ref_store_init()

* Remove some unnecessary handling of `submodule == ""` when it is
  already known to have been converted to `NULL`. (Some of the
  purported handling also happened to be broken.)

* Introduce function `register_ref_store()` in a separate step, before
  switching to hashmaps.

* Don't initialize hashmap in `lookup_ref_store()`. (Just return
  `NULL`; the hashmap will be initialized in `register_ref_store()` a
  moment later.)

* Make code in `submodule_hash_cmp()` clearer.

* Use `FLEX_ALLOC_STR()` in `alloc_submodule_hash_entry()`.

* Don't specify an initial size for the submodule hashmap (the default
  is OK).

This patch series is also available from my fork on GitHub [2] as
branch "submodule-hash".

Michael

[1] http://public-inbox.org/git/cover.1486629195.git.mhagger@alum.mit.edu/T/#u
[2] https://github.com/mhagger/git

Michael Haggerty (9):
  refs: reorder some function definitions
  refs: make some ref_store lookup functions private
  refs: remove some unnecessary handling of submodule == ""
  register_ref_store(): new function
  refs: store submodule ref stores in a hashmap
  refs: push the submodule attribute down
  base_ref_store_init(): remove submodule argument
  files_ref_store::submodule: use NULL for the main repository
  read_loose_refs(): read refs using resolve_ref_recursively()

 refs.c               | 107 +++++++++++++++++++++++++++++++++++----------------
 refs/files-backend.c |  77 +++++++++++++++++++++---------------
 refs/refs-internal.h |  48 ++++-------------------
 3 files changed, 127 insertions(+), 105 deletions(-)

-- 
2.9.3

[PATCH v2 1/9] refs: reorder some function definitions

[Michael Haggerty]

This avoids the need to add forward declarations in the next step.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 64 ++++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/refs.c b/refs.c
index 9bd0bc1..707092f 100644
--- a/refs.c
+++ b/refs.c
@@ -1358,27 +1358,19 @@ static struct ref_store *main_ref_store;
 /* A linked list of ref_stores for submodules: */
 static struct ref_store *submodule_ref_stores;
 
-void base_ref_store_init(struct ref_store *refs,
-			 const struct ref_storage_be *be,
-			 const char *submodule)
+struct ref_store *lookup_ref_store(const char *submodule)
 {
-	refs->be = be;
-	if (!submodule) {
-		if (main_ref_store)
-			die("BUG: main_ref_store initialized twice");
+	struct ref_store *refs;
 
-		refs->submodule = "";
-		refs->next = NULL;
-		main_ref_store = refs;
-	} else {
-		if (lookup_ref_store(submodule))
-			die("BUG: ref_store for submodule '%s' initialized twice",
-			    submodule);
+	if (!submodule || !*submodule)
+		return main_ref_store;
 
-		refs->submodule = xstrdup(submodule);
-		refs->next = submodule_ref_stores;
-		submodule_ref_stores = refs;
+	for (refs = submodule_ref_stores; refs; refs = refs->next) {
+		if (!strcmp(submodule, refs->submodule))
+			return refs;
 	}
+
+	return NULL;
 }
 
 struct ref_store *ref_store_init(const char *submodule)
@@ -1395,21 +1387,6 @@ struct ref_store *ref_store_init(const char *submodule)
 		return be->init(submodule);
 }
 
-struct ref_store *lookup_ref_store(const char *submodule)
-{
-	struct ref_store *refs;
-
-	if (!submodule || !*submodule)
-		return main_ref_store;
-
-	for (refs = submodule_ref_stores; refs; refs = refs->next) {
-		if (!strcmp(submodule, refs->submodule))
-			return refs;
-	}
-
-	return NULL;
-}
-
 struct ref_store *get_ref_store(const char *submodule)
 {
 	struct ref_store *refs;
@@ -1435,6 +1412,29 @@ struct ref_store *get_ref_store(const char *submodule)
 	return refs;
 }
 
+void base_ref_store_init(struct ref_store *refs,
+			 const struct ref_storage_be *be,
+			 const char *submodule)
+{
+	refs->be = be;
+	if (!submodule) {
+		if (main_ref_store)
+			die("BUG: main_ref_store initialized twice");
+
+		refs->submodule = "";
+		refs->next = NULL;
+		main_ref_store = refs;
+	} else {
+		if (lookup_ref_store(submodule))
+			die("BUG: ref_store for submodule '%s' initialized twice",
+			    submodule);
+
+		refs->submodule = xstrdup(submodule);
+		refs->next = submodule_ref_stores;
+		submodule_ref_stores = refs;
+	}
+}
+
 void assert_main_repository(struct ref_store *refs, const char *caller)
 {
 	if (*refs->submodule)
-- 
2.9.3

[PATCH v2 2/9] refs: make some ref_store lookup functions private

[Michael Haggerty]

The following functions currently don't need to be exposed:

* ref_store_init()
* lookup_ref_store()

That might change in the future, but for now make them private.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c               | 19 +++++++++++++++++--
 refs/refs-internal.h | 19 -------------------
 2 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/refs.c b/refs.c
index 707092f..d7265cc 100644
--- a/refs.c
+++ b/refs.c
@@ -1358,7 +1358,15 @@ static struct ref_store *main_ref_store;
 /* A linked list of ref_stores for submodules: */
 static struct ref_store *submodule_ref_stores;
 
-struct ref_store *lookup_ref_store(const char *submodule)
+/*
+ * Return the ref_store instance for the specified submodule (or the
+ * main repository if submodule is NULL). If that ref_store hasn't
+ * been initialized yet, return NULL.
+ *
+ * For backwards compatibility, submodule=="" is treated the same as
+ * submodule==NULL.
+ */
+static struct ref_store *lookup_ref_store(const char *submodule)
 {
 	struct ref_store *refs;
 
@@ -1373,7 +1381,14 @@ struct ref_store *lookup_ref_store(const char *submodule)
 	return NULL;
 }
 
-struct ref_store *ref_store_init(const char *submodule)
+/*
+ * Create, record, and return a ref_store instance for the specified
+ * submodule (or the main repository if submodule is NULL).
+ *
+ * For backwards compatibility, submodule=="" is treated the same as
+ * submodule==NULL.
+ */
+static struct ref_store *ref_store_init(const char *submodule)
 {
 	const char *be_name = "files";
 	struct ref_storage_be *be = find_ref_storage_backend(be_name);
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 708b260..d8a7eb1 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -653,25 +653,6 @@ void base_ref_store_init(struct ref_store *refs,
 			 const char *submodule);
 
 /*
- * Create, record, and return a ref_store instance for the specified
- * submodule (or the main repository if submodule is NULL).
- *
- * For backwards compatibility, submodule=="" is treated the same as
- * submodule==NULL.
- */
-struct ref_store *ref_store_init(const char *submodule);
-
-/*
- * Return the ref_store instance for the specified submodule (or the
- * main repository if submodule is NULL). If that ref_store hasn't
- * been initialized yet, return NULL.
- *
- * For backwards compatibility, submodule=="" is treated the same as
- * submodule==NULL.
- */
-struct ref_store *lookup_ref_store(const char *submodule);
-
-/*
  * Return the ref_store instance for the specified submodule. For the
  * main repository, use submodule==NULL; such a call cannot fail. For
  * a submodule, the submodule must exist and be a nonbare repository,
-- 
2.9.3

[PATCH v2 3/9] refs: remove some unnecessary handling of submodule == ""

[Michael Haggerty]

The only external entry point to the ref_store lookup functions is
get_ref_store(), which ensures that submodule == "" is passed along as
NULL. So ref_store_init() and lookup_ref_store() don't have to handle
submodule being specified as the empty string.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/refs.c b/refs.c
index d7265cc..6348414 100644
--- a/refs.c
+++ b/refs.c
@@ -1362,15 +1362,12 @@ static struct ref_store *submodule_ref_stores;
  * Return the ref_store instance for the specified submodule (or the
  * main repository if submodule is NULL). If that ref_store hasn't
  * been initialized yet, return NULL.
- *
- * For backwards compatibility, submodule=="" is treated the same as
- * submodule==NULL.
  */
 static struct ref_store *lookup_ref_store(const char *submodule)
 {
 	struct ref_store *refs;
 
-	if (!submodule || !*submodule)
+	if (!submodule)
 		return main_ref_store;
 
 	for (refs = submodule_ref_stores; refs; refs = refs->next) {
@@ -1384,9 +1381,6 @@ static struct ref_store *lookup_ref_store(const char *submodule)
 /*
  * Create, record, and return a ref_store instance for the specified
  * submodule (or the main repository if submodule is NULL).
- *
- * For backwards compatibility, submodule=="" is treated the same as
- * submodule==NULL.
  */
 static struct ref_store *ref_store_init(const char *submodule)
 {
@@ -1396,10 +1390,7 @@ static struct ref_store *ref_store_init(const char *submodule)
 	if (!be)
 		die("BUG: reference backend %s is unknown", be_name);
 
-	if (!submodule || !*submodule)
-		return be->init(NULL);
-	else
-		return be->init(submodule);
+	return be->init(submodule);
 }
 
 struct ref_store *get_ref_store(const char *submodule)
-- 
2.9.3

[PATCH v2 4/9] register_ref_store(): new function

[Michael Haggerty]

Move the responsibility for registering the ref_store for a submodule
from base_ref_store_init() to a new function, register_ref_store(). Call
the latter from ref_store_init().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 43 +++++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/refs.c b/refs.c
index 6348414..d7158b6 100644
--- a/refs.c
+++ b/refs.c
@@ -1379,6 +1379,29 @@ static struct ref_store *lookup_ref_store(const char *submodule)
 }
 
 /*
+ * Register the specified ref_store to be the one that should be used
+ * for submodule (or the main repository if submodule is NULL). It is
+ * a fatal error to call this function twice for the same submodule.
+ */
+static void register_ref_store(struct ref_store *refs, const char *submodule)
+{
+	if (!submodule) {
+		if (main_ref_store)
+			die("BUG: main_ref_store initialized twice");
+
+		refs->next = NULL;
+		main_ref_store = refs;
+	} else {
+		if (lookup_ref_store(submodule))
+			die("BUG: ref_store for submodule '%s' initialized twice",
+			    submodule);
+
+		refs->next = submodule_ref_stores;
+		submodule_ref_stores = refs;
+	}
+}
+
+/*
  * Create, record, and return a ref_store instance for the specified
  * submodule (or the main repository if submodule is NULL).
  */
@@ -1386,11 +1409,14 @@ static struct ref_store *ref_store_init(const char *submodule)
 {
 	const char *be_name = "files";
 	struct ref_storage_be *be = find_ref_storage_backend(be_name);
+	struct ref_store *refs;
 
 	if (!be)
 		die("BUG: reference backend %s is unknown", be_name);
 
-	return be->init(submodule);
+	refs = be->init(submodule);
+	register_ref_store(refs, submodule);
+	return refs;
 }
 
 struct ref_store *get_ref_store(const char *submodule)
@@ -1423,22 +1449,11 @@ void base_ref_store_init(struct ref_store *refs,
 			 const char *submodule)
 {
 	refs->be = be;
-	if (!submodule) {
-		if (main_ref_store)
-			die("BUG: main_ref_store initialized twice");
 
+	if (!submodule)
 		refs->submodule = "";
-		refs->next = NULL;
-		main_ref_store = refs;
-	} else {
-		if (lookup_ref_store(submodule))
-			die("BUG: ref_store for submodule '%s' initialized twice",
-			    submodule);
-
+	else
 		refs->submodule = xstrdup(submodule);
-		refs->next = submodule_ref_stores;
-		submodule_ref_stores = refs;
-	}
 }
 
 void assert_main_repository(struct ref_store *refs, const char *caller)
-- 
2.9.3

[PATCH v2 5/9] refs: store submodule ref stores in a hashmap

[Michael Haggerty]

Aside from scaling better, this means that the submodule name needn't be
stored in the ref_store instance anymore (which will be changed in a
moment). This, in turn, will help loosen the strict 1:1 relationship
between ref_stores and submodules.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c               | 58 ++++++++++++++++++++++++++++++++++++++++------------
 refs/refs-internal.h |  6 ------
 2 files changed, 45 insertions(+), 19 deletions(-)

diff --git a/refs.c b/refs.c
index d7158b6..5121c57 100644
--- a/refs.c
+++ b/refs.c
@@ -3,6 +3,7 @@
  */
 
 #include "cache.h"
+#include "hashmap.h"
 #include "lockfile.h"
 #include "refs.h"
 #include "refs/refs-internal.h"
@@ -1352,11 +1353,41 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
 	return 0;
 }
 
+struct submodule_hash_entry
+{
+	struct hashmap_entry ent; /* must be the first member! */
+
+	struct ref_store *refs;
+
+	/* NUL-terminated name of submodule: */
+	char submodule[FLEX_ARRAY];
+};
+
+static int submodule_hash_cmp(const void *entry, const void *entry_or_key,
+			      const void *keydata)
+{
+	const struct submodule_hash_entry *e1 = entry, *e2 = entry_or_key;
+	const char *submodule = keydata ? keydata : e2->submodule;
+
+	return strcmp(e1->submodule, submodule);
+}
+
+static struct submodule_hash_entry *alloc_submodule_hash_entry(
+		const char *submodule, struct ref_store *refs)
+{
+	struct submodule_hash_entry *entry;
+
+	FLEX_ALLOC_STR(entry, submodule, submodule);
+	hashmap_entry_init(entry, strhash(submodule));
+	entry->refs = refs;
+	return entry;
+}
+
 /* A pointer to the ref_store for the main repository: */
 static struct ref_store *main_ref_store;
 
-/* A linked list of ref_stores for submodules: */
-static struct ref_store *submodule_ref_stores;
+/* A hashmap of ref_stores, stored by submodule name: */
+static struct hashmap submodule_ref_stores;
 
 /*
  * Return the ref_store instance for the specified submodule (or the
@@ -1365,17 +1396,18 @@ static struct ref_store *submodule_ref_stores;
  */
 static struct ref_store *lookup_ref_store(const char *submodule)
 {
-	struct ref_store *refs;
+	struct submodule_hash_entry *entry;
 
 	if (!submodule)
 		return main_ref_store;
 
-	for (refs = submodule_ref_stores; refs; refs = refs->next) {
-		if (!strcmp(submodule, refs->submodule))
-			return refs;
-	}
+	if (!submodule_ref_stores.tablesize)
+		/* It's initialized on demand in register_ref_store(). */
+		return NULL;
 
-	return NULL;
+	entry = hashmap_get_from_hash(&submodule_ref_stores,
+				      strhash(submodule), submodule);
+	return entry ? entry->refs : NULL;
 }
 
 /*
@@ -1389,15 +1421,15 @@ static void register_ref_store(struct ref_store *refs, const char *submodule)
 		if (main_ref_store)
 			die("BUG: main_ref_store initialized twice");
 
-		refs->next = NULL;
 		main_ref_store = refs;
 	} else {
-		if (lookup_ref_store(submodule))
+		if (!submodule_ref_stores.tablesize)
+			hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 0);
+
+		if (hashmap_put(&submodule_ref_stores,
+				alloc_submodule_hash_entry(submodule, refs)))
 			die("BUG: ref_store for submodule '%s' initialized twice",
 			    submodule);
-
-		refs->next = submodule_ref_stores;
-		submodule_ref_stores = refs;
 	}
 }
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index d8a7eb1..07fd208 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -636,12 +636,6 @@ struct ref_store {
 	 * reference store:
 	 */
 	const char *submodule;
-
-	/*
-	 * Submodule reference store instances are stored in a linked
-	 * list using this pointer.
-	 */
-	struct ref_store *next;
 };
 
 /*
-- 
2.9.3

[PATCH v2 6/9] refs: push the submodule attribute down

[Michael Haggerty]

Push the submodule attribute down from ref_store to files_ref_store.
This is another step towards loosening the 1:1 connection between
ref_stores and submodules.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c               | 11 ----------
 refs/files-backend.c | 61 ++++++++++++++++++++++++++++++++++++----------------
 refs/refs-internal.h | 13 -----------
 3 files changed, 43 insertions(+), 42 deletions(-)

diff --git a/refs.c b/refs.c
index 5121c57..07959ff 100644
--- a/refs.c
+++ b/refs.c
@@ -1481,17 +1481,6 @@ void base_ref_store_init(struct ref_store *refs,
 			 const char *submodule)
 {
 	refs->be = be;
-
-	if (!submodule)
-		refs->submodule = "";
-	else
-		refs->submodule = xstrdup(submodule);
-}
-
-void assert_main_repository(struct ref_store *refs, const char *caller)
-{
-	if (*refs->submodule)
-		die("BUG: %s called for a submodule", caller);
 }
 
 /* backend functions */
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f902393..5e135a4 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -912,6 +912,14 @@ struct packed_ref_cache {
  */
 struct files_ref_store {
 	struct ref_store base;
+
+	/*
+	 * The name of the submodule represented by this object, or
+	 * the empty string if it represents the main repository's
+	 * reference store:
+	 */
+	const char *submodule;
+
 	struct ref_entry *loose;
 	struct packed_ref_cache *packed;
 };
@@ -974,10 +982,23 @@ static struct ref_store *files_ref_store_create(const char *submodule)
 
 	base_ref_store_init(ref_store, &refs_be_files, submodule);
 
+	refs->submodule = submodule ? xstrdup(submodule) : "";
+
 	return ref_store;
 }
 
 /*
+ * Die if refs is for a submodule (i.e., not for the main repository).
+ * caller is used in any necessary error messages.
+ */
+static void files_assert_main_repository(struct files_ref_store *refs,
+					 const char *caller)
+{
+	if (*refs->submodule)
+		die("BUG: %s called for a submodule", caller);
+}
+
+/*
  * Downcast ref_store to files_ref_store. Die if ref_store is not a
  * files_ref_store. If submodule_allowed is not true, then also die if
  * files_ref_store is for a submodule (i.e., not for the main
@@ -987,14 +1008,18 @@ static struct files_ref_store *files_downcast(
 		struct ref_store *ref_store, int submodule_allowed,
 		const char *caller)
 {
+	struct files_ref_store *refs;
+
 	if (ref_store->be != &refs_be_files)
 		die("BUG: ref_store is type \"%s\" not \"files\" in %s",
 		    ref_store->be->name, caller);
 
+	refs = (struct files_ref_store *)ref_store;
+
 	if (!submodule_allowed)
-		assert_main_repository(ref_store, caller);
+		files_assert_main_repository(refs, caller);
 
-	return (struct files_ref_store *)ref_store;
+	return refs;
 }
 
 /* The length of a peeled reference line in packed-refs, including EOL: */
@@ -1133,8 +1158,8 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref
 {
 	char *packed_refs_file;
 
-	if (*refs->base.submodule)
-		packed_refs_file = git_pathdup_submodule(refs->base.submodule,
+	if (*refs->submodule)
+		packed_refs_file = git_pathdup_submodule(refs->submodule,
 							 "packed-refs");
 	else
 		packed_refs_file = git_pathdup("packed-refs");
@@ -1203,8 +1228,8 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
 	size_t path_baselen;
 	int err = 0;
 
-	if (*refs->base.submodule)
-		err = strbuf_git_path_submodule(&path, refs->base.submodule, "%s", dirname);
+	if (*refs->submodule)
+		err = strbuf_git_path_submodule(&path, refs->submodule, "%s", dirname);
 	else
 		strbuf_git_path(&path, "%s", dirname);
 	path_baselen = path.len;
@@ -1244,10 +1269,10 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
 		} else {
 			int read_ok;
 
-			if (*refs->base.submodule) {
+			if (*refs->submodule) {
 				hashclr(sha1);
 				flag = 0;
-				read_ok = !resolve_gitlink_ref(refs->base.submodule,
+				read_ok = !resolve_gitlink_ref(refs->submodule,
 							       refname.buf, sha1);
 			} else {
 				read_ok = !read_ref_full(refname.buf,
@@ -1358,8 +1383,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 	*type = 0;
 	strbuf_reset(&sb_path);
 
-	if (*refs->base.submodule)
-		strbuf_git_path_submodule(&sb_path, refs->base.submodule, "%s", refname);
+	if (*refs->submodule)
+		strbuf_git_path_submodule(&sb_path, refs->submodule, "%s", refname);
 	else
 		strbuf_git_path(&sb_path, "%s", refname);
 
@@ -1540,7 +1565,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
 	int ret = TRANSACTION_GENERIC_ERROR;
 
 	assert(err);
-	assert_main_repository(&refs->base, "lock_raw_ref");
+	files_assert_main_repository(refs, "lock_raw_ref");
 
 	*type = 0;
 
@@ -2006,7 +2031,7 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,
 	int attempts_remaining = 3;
 	int resolved;
 
-	assert_main_repository(&refs->base, "lock_ref_sha1_basic");
+	files_assert_main_repository(refs, "lock_ref_sha1_basic");
 	assert(err);
 
 	lock = xcalloc(1, sizeof(struct ref_lock));
@@ -2152,7 +2177,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)
 	static int timeout_value = 1000;
 	struct packed_ref_cache *packed_ref_cache;
 
-	assert_main_repository(&refs->base, "lock_packed_refs");
+	files_assert_main_repository(refs, "lock_packed_refs");
 
 	if (!timeout_configured) {
 		git_config_get_int("core.packedrefstimeout", &timeout_value);
@@ -2190,7 +2215,7 @@ static int commit_packed_refs(struct files_ref_store *refs)
 	int save_errno = 0;
 	FILE *out;
 
-	assert_main_repository(&refs->base, "commit_packed_refs");
+	files_assert_main_repository(refs, "commit_packed_refs");
 
 	if (!packed_ref_cache->lock)
 		die("internal error: packed-refs not locked");
@@ -2223,7 +2248,7 @@ static void rollback_packed_refs(struct files_ref_store *refs)
 	struct packed_ref_cache *packed_ref_cache =
 		get_packed_ref_cache(refs);
 
-	assert_main_repository(&refs->base, "rollback_packed_refs");
+	files_assert_main_repository(refs, "rollback_packed_refs");
 
 	if (!packed_ref_cache->lock)
 		die("internal error: packed-refs not locked");
@@ -2397,7 +2422,7 @@ static int repack_without_refs(struct files_ref_store *refs,
 	struct string_list_item *refname;
 	int ret, needs_repacking = 0, removed = 0;
 
-	assert_main_repository(&refs->base, "repack_without_refs");
+	files_assert_main_repository(refs, "repack_without_refs");
 	assert(err);
 
 	/* Look for a packed ref */
@@ -2930,7 +2955,7 @@ static int commit_ref_update(struct files_ref_store *refs,
 			     const unsigned char *sha1, const char *logmsg,
 			     struct strbuf *err)
 {
-	assert_main_repository(&refs->base, "commit_ref_update");
+	files_assert_main_repository(refs, "commit_ref_update");
 
 	clear_loose_ref_cache(refs);
 	if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err)) {
@@ -3560,7 +3585,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 	int ret;
 	struct ref_lock *lock;
 
-	assert_main_repository(&refs->base, "lock_ref_for_update");
+	files_assert_main_repository(refs, "lock_ref_for_update");
 
 	if ((update->flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1))
 		update->flags |= REF_DELETING;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 07fd208..008822d 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -629,13 +629,6 @@ extern struct ref_storage_be refs_be_files;
 struct ref_store {
 	/* The backend describing this ref_store's storage scheme: */
 	const struct ref_storage_be *be;
-
-	/*
-	 * The name of the submodule represented by this object, or
-	 * the empty string if it represents the main repository's
-	 * reference store:
-	 */
-	const char *submodule;
 };
 
 /*
@@ -658,10 +651,4 @@ void base_ref_store_init(struct ref_store *refs,
  */
 struct ref_store *get_ref_store(const char *submodule);
 
-/*
- * Die if refs is for a submodule (i.e., not for the main repository).
- * caller is used in any necessary error messages.
- */
-void assert_main_repository(struct ref_store *refs, const char *caller);
-
 #endif /* REFS_REFS_INTERNAL_H */
-- 
2.9.3

[PATCH v2 7/9] base_ref_store_init(): remove submodule argument

[Michael Haggerty]

This is another step towards weakening the 1:1 relationship between
ref_stores and submodules.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c               | 3 +--
 refs/files-backend.c | 2 +-
 refs/refs-internal.h | 7 +++----
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index 07959ff..05af56b 100644
--- a/refs.c
+++ b/refs.c
@@ -1477,8 +1477,7 @@ struct ref_store *get_ref_store(const char *submodule)
 }
 
 void base_ref_store_init(struct ref_store *refs,
-			 const struct ref_storage_be *be,
-			 const char *submodule)
+			 const struct ref_storage_be *be)
 {
 	refs->be = be;
 }
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5e135a4..c9d2d28 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -980,7 +980,7 @@ static struct ref_store *files_ref_store_create(const char *submodule)
 	struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
 	struct ref_store *ref_store = (struct ref_store *)refs;
 
-	base_ref_store_init(ref_store, &refs_be_files, submodule);
+	base_ref_store_init(ref_store, &refs_be_files);
 
 	refs->submodule = submodule ? xstrdup(submodule) : "";
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 008822d..793c850 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -632,12 +632,11 @@ struct ref_store {
 };
 
 /*
- * Fill in the generic part of refs for the specified submodule and
- * add it to our collection of reference stores.
+ * Fill in the generic part of refs and add it to our collection of
+ * reference stores.
  */
 void base_ref_store_init(struct ref_store *refs,
-			 const struct ref_storage_be *be,
-			 const char *submodule);
+			 const struct ref_storage_be *be);
 
 /*
  * Return the ref_store instance for the specified submodule. For the
-- 
2.9.3

[PATCH v2 8/9] files_ref_store::submodule: use NULL for the main repository

[Michael Haggerty]

The old practice of storing the empty string in this member for the main
repository was a holdover from before 00eebe3 (refs: create a base class
"ref_store" for files_ref_store, 2016-09-04), when the submodule was
stored in a flex array at the end of `struct files_ref_store`. Storing
NULL for this case is more idiomatic and a tiny bit less code.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs/files-backend.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index c9d2d28..4fe92f0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -915,8 +915,8 @@ struct files_ref_store {
 
 	/*
 	 * The name of the submodule represented by this object, or
-	 * the empty string if it represents the main repository's
-	 * reference store:
+	 * NULL if it represents the main repository's reference
+	 * store:
 	 */
 	const char *submodule;
 
@@ -982,7 +982,7 @@ static struct ref_store *files_ref_store_create(const char *submodule)
 
 	base_ref_store_init(ref_store, &refs_be_files);
 
-	refs->submodule = submodule ? xstrdup(submodule) : "";
+	refs->submodule = xstrdup_or_null(submodule);
 
 	return ref_store;
 }
@@ -994,7 +994,7 @@ static struct ref_store *files_ref_store_create(const char *submodule)
 static void files_assert_main_repository(struct files_ref_store *refs,
 					 const char *caller)
 {
-	if (*refs->submodule)
+	if (refs->submodule)
 		die("BUG: %s called for a submodule", caller);
 }
 
@@ -1158,7 +1158,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref
 {
 	char *packed_refs_file;
 
-	if (*refs->submodule)
+	if (refs->submodule)
 		packed_refs_file = git_pathdup_submodule(refs->submodule,
 							 "packed-refs");
 	else
@@ -1228,7 +1228,7 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
 	size_t path_baselen;
 	int err = 0;
 
-	if (*refs->submodule)
+	if (refs->submodule)
 		err = strbuf_git_path_submodule(&path, refs->submodule, "%s", dirname);
 	else
 		strbuf_git_path(&path, "%s", dirname);
@@ -1269,7 +1269,7 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
 		} else {
 			int read_ok;
 
-			if (*refs->submodule) {
+			if (refs->submodule) {
 				hashclr(sha1);
 				flag = 0;
 				read_ok = !resolve_gitlink_ref(refs->submodule,
@@ -1383,7 +1383,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 	*type = 0;
 	strbuf_reset(&sb_path);
 
-	if (*refs->submodule)
+	if (refs->submodule)
 		strbuf_git_path_submodule(&sb_path, refs->submodule, "%s", refname);
 	else
 		strbuf_git_path(&sb_path, "%s", refname);
-- 
2.9.3

[PATCH v2 9/9] read_loose_refs(): read refs using resolve_ref_recursively()

[Michael Haggerty]

There is no need to call read_ref_full() or resolve_gitlink_ref() from
read_loose_refs(), because we already have a ref_store object in hand.
So we can call resolve_ref_recursively() ourselves. Happily, this
unifies the code for the submodule vs. non-submodule cases.

This requires resolve_ref_recursively() to be exposed to the refs
subsystem, though not to non-refs code.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c               | 50 +++++++++++++++++++++++++-------------------------
 refs/files-backend.c | 18 ++++--------------
 refs/refs-internal.h |  5 +++++
 3 files changed, 34 insertions(+), 39 deletions(-)

diff --git a/refs.c b/refs.c
index 05af56b..f03dcf5 100644
--- a/refs.c
+++ b/refs.c
@@ -1230,10 +1230,10 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)
 }
 
 /* This function needs to return a meaningful errno on failure */
-static const char *resolve_ref_recursively(struct ref_store *refs,
-					   const char *refname,
-					   int resolve_flags,
-					   unsigned char *sha1, int *flags)
+const char *resolve_ref_recursively(struct ref_store *refs,
+				    const char *refname,
+				    int resolve_flags,
+				    unsigned char *sha1, int *flags)
 {
 	static struct strbuf sb_refname = STRBUF_INIT;
 	int unused_flags;
@@ -1390,27 +1390,6 @@ static struct ref_store *main_ref_store;
 static struct hashmap submodule_ref_stores;
 
 /*
- * Return the ref_store instance for the specified submodule (or the
- * main repository if submodule is NULL). If that ref_store hasn't
- * been initialized yet, return NULL.
- */
-static struct ref_store *lookup_ref_store(const char *submodule)
-{
-	struct submodule_hash_entry *entry;
-
-	if (!submodule)
-		return main_ref_store;
-
-	if (!submodule_ref_stores.tablesize)
-		/* It's initialized on demand in register_ref_store(). */
-		return NULL;
-
-	entry = hashmap_get_from_hash(&submodule_ref_stores,
-				      strhash(submodule), submodule);
-	return entry ? entry->refs : NULL;
-}
-
-/*
  * Register the specified ref_store to be the one that should be used
  * for submodule (or the main repository if submodule is NULL). It is
  * a fatal error to call this function twice for the same submodule.
@@ -1451,6 +1430,27 @@ static struct ref_store *ref_store_init(const char *submodule)
 	return refs;
 }
 
+/*
+ * Return the ref_store instance for the specified submodule (or the
+ * main repository if submodule is NULL). If that ref_store hasn't
+ * been initialized yet, return NULL.
+ */
+static struct ref_store *lookup_ref_store(const char *submodule)
+{
+	struct submodule_hash_entry *entry;
+
+	if (!submodule)
+		return main_ref_store;
+
+	if (!submodule_ref_stores.tablesize)
+		/* It's initialized on demand in register_ref_store(). */
+		return NULL;
+
+	entry = hashmap_get_from_hash(&submodule_ref_stores,
+				      strhash(submodule), submodule);
+	return entry ? entry->refs : NULL;
+}
+
 struct ref_store *get_ref_store(const char *submodule)
 {
 	struct ref_store *refs;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4fe92f0..cdb6b8f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1267,20 +1267,10 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
 					 create_dir_entry(refs, refname.buf,
 							  refname.len, 1));
 		} else {
-			int read_ok;
-
-			if (refs->submodule) {
-				hashclr(sha1);
-				flag = 0;
-				read_ok = !resolve_gitlink_ref(refs->submodule,
-							       refname.buf, sha1);
-			} else {
-				read_ok = !read_ref_full(refname.buf,
-							 RESOLVE_REF_READING,
-							 sha1, &flag);
-			}
-
-			if (!read_ok) {
+			if (!resolve_ref_recursively(&refs->base,
+						     refname.buf,
+						     RESOLVE_REF_READING,
+						     sha1, &flag)) {
 				hashclr(sha1);
 				flag |= REF_ISBROKEN;
 			} else if (is_null_sha1(sha1)) {
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 793c850..33adbf9 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -650,4 +650,9 @@ void base_ref_store_init(struct ref_store *refs,
  */
 struct ref_store *get_ref_store(const char *submodule);
 
+const char *resolve_ref_recursively(struct ref_store *refs,
+				    const char *refname,
+				    int resolve_flags,
+				    unsigned char *sha1, int *flags);
+
 #endif /* REFS_REFS_INTERNAL_H */
-- 
2.9.3

Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list

[Jeff King]

On Fri, Feb 10, 2017 at 12:16:10PM +0100, Michael Haggerty wrote:

> This is v2 of the patch series, considerably reorganized but not that
> different codewise. Thanks to Stefan, Junio, and Peff for their
> feedback about v1 [1]. I think I have addressed all of your comments.
> 
> Changes since v1:

I read through these and didn't didn't see any problems.

The reordering and new commits made sense to me.

-Peff

Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list

[Stefan Beller]

On Fri, Feb 10, 2017 at 7:56 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Feb 10, 2017 at 12:16:10PM +0100, Michael Haggerty wrote:
>
>> This is v2 of the patch series, considerably reorganized but not that
>> different codewise. Thanks to Stefan, Junio, and Peff for their
>> feedback about v1 [1]. I think I have addressed all of your comments.
>>
>> Changes since v1:
>
> I read through these and didn't didn't see any problems.
>
> The reordering and new commits made sense to me.

Same here.

Stefan

>
> -Peff

Re: [PATCH v2 1/9] refs: reorder some function definitions

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> This avoids the need to add forward declarations in the next step.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 64 ++++++++++++++++++++++++++++++++--------------------------------
>  1 file changed, 32 insertions(+), 32 deletions(-)

Makes sense, but the patch itself looks like ... unreadble ;-)

>
> diff --git a/refs.c b/refs.c
> index 9bd0bc1..707092f 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1358,27 +1358,19 @@ static struct ref_store *main_ref_store;
>  /* A linked list of ref_stores for submodules: */
>  static struct ref_store *submodule_ref_stores;
>  
> -void base_ref_store_init(struct ref_store *refs,
> -			 const struct ref_storage_be *be,
> -			 const char *submodule)
> +struct ref_store *lookup_ref_store(const char *submodule)
>  {
> -	refs->be = be;
> -	if (!submodule) {
> -		if (main_ref_store)
> -			die("BUG: main_ref_store initialized twice");
> +	struct ref_store *refs;
>  
> -		refs->submodule = "";
> -		refs->next = NULL;
> -		main_ref_store = refs;
> -	} else {
> -		if (lookup_ref_store(submodule))
> -			die("BUG: ref_store for submodule '%s' initialized twice",
> -			    submodule);
> +	if (!submodule || !*submodule)
> +		return main_ref_store;
>  
> -		refs->submodule = xstrdup(submodule);
> -		refs->next = submodule_ref_stores;
> -		submodule_ref_stores = refs;
> +	for (refs = submodule_ref_stores; refs; refs = refs->next) {
> +		if (!strcmp(submodule, refs->submodule))
> +			return refs;
>  	}
> +
> +	return NULL;
>  }
>  
>  struct ref_store *ref_store_init(const char *submodule)
> @@ -1395,21 +1387,6 @@ struct ref_store *ref_store_init(const char *submodule)
>  		return be->init(submodule);
>  }
>  
> -struct ref_store *lookup_ref_store(const char *submodule)
> -{
> -	struct ref_store *refs;
> -
> -	if (!submodule || !*submodule)
> -		return main_ref_store;
> -
> -	for (refs = submodule_ref_stores; refs; refs = refs->next) {
> -		if (!strcmp(submodule, refs->submodule))
> -			return refs;
> -	}
> -
> -	return NULL;
> -}
> -
>  struct ref_store *get_ref_store(const char *submodule)
>  {
>  	struct ref_store *refs;
> @@ -1435,6 +1412,29 @@ struct ref_store *get_ref_store(const char *submodule)
>  	return refs;
>  }
>  
> +void base_ref_store_init(struct ref_store *refs,
> +			 const struct ref_storage_be *be,
> +			 const char *submodule)
> +{
> +	refs->be = be;
> +	if (!submodule) {
> +		if (main_ref_store)
> +			die("BUG: main_ref_store initialized twice");
> +
> +		refs->submodule = "";
> +		refs->next = NULL;
> +		main_ref_store = refs;
> +	} else {
> +		if (lookup_ref_store(submodule))
> +			die("BUG: ref_store for submodule '%s' initialized twice",
> +			    submodule);
> +
> +		refs->submodule = xstrdup(submodule);
> +		refs->next = submodule_ref_stores;
> +		submodule_ref_stores = refs;
> +	}
> +}
> +
>  void assert_main_repository(struct ref_store *refs, const char *caller)
>  {
>  	if (*refs->submodule)

Re: [PATCH v2 9/9] read_loose_refs(): read refs using resolve_ref_recursively()

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> There is no need to call read_ref_full() or resolve_gitlink_ref() from
> read_loose_refs(), because we already have a ref_store object in hand.
> So we can call resolve_ref_recursively() ourselves. Happily, this
> unifies the code for the submodule vs. non-submodule cases.
>
> This requires resolve_ref_recursively() to be exposed to the refs
> subsystem, though not to non-refs code.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c               | 50 +++++++++++++++++++++++++-------------------------
>  refs/files-backend.c | 18 ++++--------------
>  refs/refs-internal.h |  5 +++++
>  3 files changed, 34 insertions(+), 39 deletions(-)

OK, but one thing puzzles me...

> @@ -1390,27 +1390,6 @@ static struct ref_store *main_ref_store;
>  static struct hashmap submodule_ref_stores;
>  
>  /*
> - * Return the ref_store instance for the specified submodule (or the
> - * main repository if submodule is NULL). If that ref_store hasn't
> - * been initialized yet, return NULL.
> - */
> -static struct ref_store *lookup_ref_store(const char *submodule)
> -{
> -	struct submodule_hash_entry *entry;
> -
> -	if (!submodule)
> -		return main_ref_store;
> -
> -	if (!submodule_ref_stores.tablesize)
> -		/* It's initialized on demand in register_ref_store(). */
> -		return NULL;
> -
> -	entry = hashmap_get_from_hash(&submodule_ref_stores,
> -				      strhash(submodule), submodule);
> -	return entry ? entry->refs : NULL;
> -}
> -
> -/*
>   * Register the specified ref_store to be the one that should be used
>   * for submodule (or the main repository if submodule is NULL). It is
>   * a fatal error to call this function twice for the same submodule.
> @@ -1451,6 +1430,27 @@ static struct ref_store *ref_store_init(const char *submodule)
>  	return refs;
>  }
>  
> +/*
> + * Return the ref_store instance for the specified submodule (or the
> + * main repository if submodule is NULL). If that ref_store hasn't
> + * been initialized yet, return NULL.
> + */
> +static struct ref_store *lookup_ref_store(const char *submodule)
> +{
> +	struct submodule_hash_entry *entry;
> +
> +	if (!submodule)
> +		return main_ref_store;
> +
> +	if (!submodule_ref_stores.tablesize)
> +		/* It's initialized on demand in register_ref_store(). */
> +		return NULL;
> +
> +	entry = hashmap_get_from_hash(&submodule_ref_stores,
> +				      strhash(submodule), submodule);
> +	return entry ? entry->refs : NULL;
> +}
> +

I somehow thought that we had an early "reorder the code" step to
avoid hunks like these?  Am I missing some subtle changes made while
moving the function down?

Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> This is v2 of the patch series, considerably reorganized but not that
> different codewise.

Thanks.  The way the series loses "!*submodule and !submodule are
the same thing, sometimes" is easier to follow when presented in
this order, at least to me.

Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list

[David Turner]

On Fri, 2017-02-10 at 12:16 +0100, Michael Haggerty wrote:
> This is v2 of the patch series, considerably reorganized but not that
> different codewise. Thanks to Stefan, Junio, and Peff for their
> feedback about v1 [1]. I think I have addressed all of your comments.

LGTM.

Re: [PATCH v2 9/9] read_loose_refs(): read refs using resolve_ref_recursively()

[Michael Haggerty]

On 02/10/2017 08:22 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> [...]
> 
> OK, but one thing puzzles me...
> 
>> @@ -1390,27 +1390,6 @@ static struct ref_store *main_ref_store;
>>  static struct hashmap submodule_ref_stores;
>>  
>>  /*
>> - * Return the ref_store instance for the specified submodule (or the
>> - * main repository if submodule is NULL). If that ref_store hasn't
>> - * been initialized yet, return NULL.
>> - */
>> -static struct ref_store *lookup_ref_store(const char *submodule)
>> -{
>> -	struct submodule_hash_entry *entry;
>> -
>> -	if (!submodule)
>> -		return main_ref_store;
>> -
>> -	if (!submodule_ref_stores.tablesize)
>> -		/* It's initialized on demand in register_ref_store(). */
>> -		return NULL;
>> -
>> -	entry = hashmap_get_from_hash(&submodule_ref_stores,
>> -				      strhash(submodule), submodule);
>> -	return entry ? entry->refs : NULL;
>> -}
>> -
>> -/*
>>   * Register the specified ref_store to be the one that should be used
>>   * for submodule (or the main repository if submodule is NULL). It is
>>   * a fatal error to call this function twice for the same submodule.
>> @@ -1451,6 +1430,27 @@ static struct ref_store *ref_store_init(const char *submodule)
>>  	return refs;
>>  }
>>  
>> +/*
>> + * Return the ref_store instance for the specified submodule (or the
>> + * main repository if submodule is NULL). If that ref_store hasn't
>> + * been initialized yet, return NULL.
>> + */
>> +static struct ref_store *lookup_ref_store(const char *submodule)
>> +{
>> +	struct submodule_hash_entry *entry;
>> +
>> +	if (!submodule)
>> +		return main_ref_store;
>> +
>> +	if (!submodule_ref_stores.tablesize)
>> +		/* It's initialized on demand in register_ref_store(). */
>> +		return NULL;
>> +
>> +	entry = hashmap_get_from_hash(&submodule_ref_stores,
>> +				      strhash(submodule), submodule);
>> +	return entry ? entry->refs : NULL;
>> +}
>> +
> 
> I somehow thought that we had an early "reorder the code" step to
> avoid hunks like these?  Am I missing some subtle changes made while
> moving the function down?

You are quite right; thanks for noticing. I forgot to un-move this
function when re-rolling. These two hunks can be discarded (the function
text is unchanged).

I pushed the fixed commit to branch `submodule-hash` in my fork [1]. If
you'd like me to send it to the mailing list again, please let me know.

Michael

[1] https://github.com/mhagger/git

Re: [PATCH v2 9/9] read_loose_refs(): read refs using resolve_ref_recursively()

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> I pushed the fixed commit to branch `submodule-hash` in my fork [1]. If
> you'd like me to send it to the mailing list again, please let me know.

I was tempted to ask you to send it again, because fetching,
comparing and then cherry-picking is a lot more work than just
replacing one, but just to make sure my intuition about the
work required is correct, I did it anyway and yes, fetching,
comparing, cherry-picking and then amending the sign-off in was a
lot more work ;-)

So no need to resend.  Thanks.