# [PATCH v2 2/9] refs: make some ref_store lookup functions private 18 messages from 2017-02-10 to 2017-02-13. Participants: Michael Haggerty, Jeff King, Stefan Beller, Junio C Hamano, David Turner. Thread: https://gitlist.dev/t/45099 ## Michael Haggerty, 2017-02-10 11:16 Subject: [PATCH v2 2/9] refs: make some ref_store lookup functions private Message-ID: <439c55a57aee7bd5c34c1a39cefcf35daab78f62.1486724698.git.mhagger@alum.mit.edu> URL: https://gitlist.dev/e/439c55a57aee7bd5c34c1a39cefcf35daab78f62.1486724698.git.mhagger%40alum.mit.edu In-Reply-To: ``` 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 --- 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 ``` ## Michael Haggerty, 2017-02-10 11:16 Subject: [PATCH v2 0/9] Store submodules in a hash, not a linked list Message-ID: URL: https://gitlist.dev/e/cover.1486724698.git.mhagger%40alum.mit.edu ``` 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 ``` ## Michael Haggerty, 2017-02-10 11:16 Subject: [PATCH v2 7/9] base_ref_store_init(): remove submodule argument Message-ID: <43f78e3e765605c021f4f4138b15e4db5b7a3508.1486724698.git.mhagger@alum.mit.edu> URL: https://gitlist.dev/e/43f78e3e765605c021f4f4138b15e4db5b7a3508.1486724698.git.mhagger%40alum.mit.edu In-Reply-To: ``` This is another step towards weakening the 1:1 relationship between ref_stores and submodules. Signed-off-by: Michael Haggerty --- 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 ``` ## Michael Haggerty, 2017-02-10 11:16 Subject: [PATCH v2 9/9] read_loose_refs(): read refs using resolve_ref_recursively() Message-ID: URL: https://gitlist.dev/e/d8e906d969700acbca8dc717673d0a9cdc910f62.1486724698.git.mhagger%40alum.mit.edu In-Reply-To: ``` 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 --- 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 ``` ## Michael Haggerty, 2017-02-10 11:16 Subject: [PATCH v2 6/9] refs: push the submodule attribute down Message-ID: URL: https://gitlist.dev/e/db8892633bc1984e66fac0e4ae5e13ae4b526502.1486724698.git.mhagger%40alum.mit.edu In-Reply-To: ``` 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 --- 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 ``` ## Michael Haggerty, 2017-02-10 11:16 Subject: [PATCH v2 4/9] register_ref_store(): new function Message-ID: <5569188834dd39bf40bcae034a8d23d21d31a7e9.1486724698.git.mhagger@alum.mit.edu> URL: https://gitlist.dev/e/5569188834dd39bf40bcae034a8d23d21d31a7e9.1486724698.git.mhagger%40alum.mit.edu In-Reply-To: ``` 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 --- 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 ``` ## Michael Haggerty, 2017-02-10 11:16 Subject: [PATCH v2 8/9] files_ref_store::submodule: use NULL for the main repository Message-ID: URL: https://gitlist.dev/e/a7d07a3b3374e399e0ac04b57802d247ad05a9cb.1486724698.git.mhagger%40alum.mit.edu In-Reply-To: ``` 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 --- 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 ``` ## Michael Haggerty, 2017-02-10 11:16 Subject: [PATCH v2 3/9] refs: remove some unnecessary handling of submodule == "" Message-ID: URL: https://gitlist.dev/e/c005d19654fd1aae2d542560911b2a92437b89fe.1486724698.git.mhagger%40alum.mit.edu In-Reply-To: ``` 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 --- 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 ``` ## Michael Haggerty, 2017-02-10 11:16 Subject: [PATCH v2 1/9] refs: reorder some function definitions Message-ID: <661ef87844918501e84b43d254305e1997af9b57.1486724698.git.mhagger@alum.mit.edu> URL: https://gitlist.dev/e/661ef87844918501e84b43d254305e1997af9b57.1486724698.git.mhagger%40alum.mit.edu In-Reply-To: ``` This avoids the need to add forward declarations in the next step. Signed-off-by: Michael Haggerty --- 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 ``` ## Michael Haggerty, 2017-02-10 11:16 Subject: [PATCH v2 5/9] refs: store submodule ref stores in a hashmap Message-ID: <3f4b16b0816d13283cb25885eb3fdafd70caa2e7.1486724698.git.mhagger@alum.mit.edu> URL: https://gitlist.dev/e/3f4b16b0816d13283cb25885eb3fdafd70caa2e7.1486724698.git.mhagger%40alum.mit.edu In-Reply-To: ``` 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 --- 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 ``` ## Jeff King, 2017-02-10 15:56 Subject: Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list Message-ID: <20170210155651.lecjrzmoux5mcm5d@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170210155651.lecjrzmoux5mcm5d%40sigill.intra.peff.net In-Reply-To: ``` 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 ``` ## Stefan Beller, 2017-02-10 16:44 Subject: Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list Message-ID: URL: https://gitlist.dev/e/CAGZ79kak%3Dj7u3N0WvUYwUwA-b%2Bb%3DDNrMpG_iRHnd6ZLj4T2Q0Q%40mail.gmail.com In-Reply-To: <20170210155651.lecjrzmoux5mcm5d@sigill.intra.peff.net> ``` On Fri, Feb 10, 2017 at 7:56 AM, Jeff King 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 ``` ## Junio C Hamano, 2017-02-10 19:14 Subject: Re: [PATCH v2 1/9] refs: reorder some function definitions Message-ID: URL: https://gitlist.dev/e/xmqqfujlluxe.fsf%40gitster.mtv.corp.google.com In-Reply-To: <661ef87844918501e84b43d254305e1997af9b57.1486724698.git.mhagger@alum.mit.edu> ``` Michael Haggerty writes: > This avoids the need to add forward declarations in the next step. > > Signed-off-by: Michael Haggerty > --- > 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) ``` ## Junio C Hamano, 2017-02-10 19:22 Subject: Re: [PATCH v2 9/9] read_loose_refs(): read refs using resolve_ref_recursively() Message-ID: URL: https://gitlist.dev/e/xmqqbmu9lul6.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Michael Haggerty 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 > --- > 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? ``` ## Junio C Hamano, 2017-02-10 19:23 Subject: Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list Message-ID: URL: https://gitlist.dev/e/xmqq7f4xluiq.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Michael Haggerty 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. ``` ## David Turner, 2017-02-13 03:09 Subject: Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list Message-ID: <1486955355.1929.14.camel@novalis.org> URL: https://gitlist.dev/e/1486955355.1929.14.camel%40novalis.org In-Reply-To: ``` 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. ``` ## Michael Haggerty, 2017-02-13 06:40 Subject: Re: [PATCH v2 9/9] read_loose_refs(): read refs using resolve_ref_recursively() Message-ID: URL: https://gitlist.dev/e/ff0b0df6-9aed-9417-d9d4-1234d53f05c3%40alum.mit.edu In-Reply-To: ``` On 02/10/2017 08:22 PM, Junio C Hamano wrote: > Michael Haggerty 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 ``` ## Junio C Hamano, 2017-02-13 23:18 Subject: Re: [PATCH v2 9/9] read_loose_refs(): read refs using resolve_ref_recursively() Message-ID: URL: https://gitlist.dev/e/xmqqh93xbrys.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Michael Haggerty 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. ```