From: Junio C Hamano <gitster@pobox.com>
To: git@vger.kernel.org
Subject: [PATCH] fetch: replace string-list used as a look-up table with a hashmap
Date: Wed, 26 Sep 2018 14:28:28 -0700 [thread overview]
Message-ID: <xmqqin2sj6df.fsf@gitster-ct.c.googlers.com> (raw)
In find_non_local_tags() helper function (used to implement the
"follow tags"), we use string_list_has_string() on two string lists
as a way to see if a refname has already been processed, etc.
All this code predates more modern in-core lookup API like hashmap;
replace them with two hashmaps and one string list---the hashmaps
are used for look-ups and the string list is to keep the order of
items in the returned result stable (which is the only single thing
hashmap does worse than lookups on string-list).
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Just to remind ourselves that we talked about getting rid of
string-list used as look-up tables by replacing them with hashmap
but haven't seen much effort expended on it. I think this is my
first semi-serious use of hashmap, and the code is expected to be
full of newbie mistakes, inefficiencies and ignorance of idioms;
pointing out any of which is much appreciated.
builtin/fetch.c | 116 +++++++++++++++++++++++++++++++++++++-----------
1 file changed, 90 insertions(+), 26 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 0696abfc2a..5d1fd8dd49 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -246,16 +246,73 @@ static int will_fetch(struct ref **head, const unsigned char *sha1)
return 0;
}
+struct refname_hash_entry {
+ struct hashmap_entry ent; /* must be the first member */
+ struct object_id oid;
+ char refname[FLEX_ARRAY];
+};
+
+static int refname_hash_entry_cmp(const void *hashmap_cmp_fn_data,
+ const struct refname_hash_entry *e1,
+ const struct refname_hash_entry *e2,
+ const void *keydata)
+{
+ return strcmp(e1->refname, keydata ? keydata : e2->refname);
+}
+
+static struct refname_hash_entry *refname_hash_add(struct hashmap *map,
+ const char *refname,
+ const struct object_id *oid)
+{
+ struct refname_hash_entry *ent;
+ size_t len = strlen(refname);
+
+ FLEX_ALLOC_MEM(ent, refname, refname, len);
+ hashmap_entry_init(ent, memhash(refname, len));
+ oidcpy(&ent->oid, oid);
+ hashmap_add(map, ent);
+ return ent;
+}
+
+static int add_one_refname(const char *refname,
+ const struct object_id *oid,
+ int flag, void *cbdata)
+{
+ struct hashmap *refname_map = cbdata;
+
+ (void) refname_hash_add(refname_map, refname, oid);
+ return 0;
+}
+
+static void refname_hash_init(struct hashmap *map)
+{
+ hashmap_init(map, (hashmap_cmp_fn)refname_hash_entry_cmp, NULL, 0);
+}
+
+static int refname_hash_exists(struct hashmap *map, const char *refname)
+{
+ struct hashmap_entry key;
+ size_t len = strlen(refname);
+ hashmap_entry_init(&key, memhash(refname, len));
+
+ return !!hashmap_get(map, &key, refname);
+}
+
static void find_non_local_tags(const struct ref *refs,
struct ref **head,
struct ref ***tail)
{
- struct string_list existing_refs = STRING_LIST_INIT_DUP;
- struct string_list remote_refs = STRING_LIST_INIT_NODUP;
+ struct hashmap existing_refs;
+ struct hashmap remote_refs;
+ struct string_list remote_refs_list = STRING_LIST_INIT_NODUP;
+ struct string_list_item *remote_ref_item;
const struct ref *ref;
- struct string_list_item *item = NULL;
+ struct refname_hash_entry *item = NULL;
- for_each_ref(add_existing, &existing_refs);
+ refname_hash_init(&existing_refs);
+ refname_hash_init(&remote_refs);
+
+ for_each_ref(add_one_refname, &existing_refs);
for (ref = refs; ref; ref = ref->next) {
if (!starts_with(ref->name, "refs/tags/"))
continue;
@@ -271,10 +328,10 @@ static void find_non_local_tags(const struct ref *refs,
!has_object_file_with_flags(&ref->old_oid,
OBJECT_INFO_QUICK) &&
!will_fetch(head, ref->old_oid.hash) &&
- !has_sha1_file_with_flags(item->util,
+ !has_sha1_file_with_flags(item->oid.hash,
OBJECT_INFO_QUICK) &&
- !will_fetch(head, item->util))
- item->util = NULL;
+ !will_fetch(head, item->oid.hash))
+ oidclr(&item->oid);
item = NULL;
continue;
}
@@ -286,47 +343,54 @@ static void find_non_local_tags(const struct ref *refs,
* fetch.
*/
if (item &&
- !has_sha1_file_with_flags(item->util, OBJECT_INFO_QUICK) &&
- !will_fetch(head, item->util))
- item->util = NULL;
+ !has_sha1_file_with_flags(item->oid.hash, OBJECT_INFO_QUICK) &&
+ !will_fetch(head, item->oid.hash))
+ oidclr(&item->oid);
item = NULL;
/* skip duplicates and refs that we already have */
- if (string_list_has_string(&remote_refs, ref->name) ||
- string_list_has_string(&existing_refs, ref->name))
+ if (refname_hash_exists(&remote_refs, ref->name) ||
+ refname_hash_exists(&existing_refs, ref->name))
continue;
- item = string_list_insert(&remote_refs, ref->name);
- item->util = (void *)&ref->old_oid;
+ item = refname_hash_add(&remote_refs, ref->name, &ref->old_oid);
+ string_list_insert(&remote_refs_list, ref->name);
}
- string_list_clear(&existing_refs, 1);
+ hashmap_free(&existing_refs, 1);
/*
* We may have a final lightweight tag that needs to be
* checked to see if it needs fetching.
*/
if (item &&
- !has_sha1_file_with_flags(item->util, OBJECT_INFO_QUICK) &&
- !will_fetch(head, item->util))
- item->util = NULL;
+ !has_sha1_file_with_flags(item->oid.hash, OBJECT_INFO_QUICK) &&
+ !will_fetch(head, item->oid.hash))
+ oidclr(&item->oid);
/*
- * For all the tags in the remote_refs string list,
+ * For all the tags in the remote_refs_list,
* add them to the list of refs to be fetched
*/
- for_each_string_list_item(item, &remote_refs) {
+ for_each_string_list_item(remote_ref_item, &remote_refs_list) {
+ const char *refname = remote_ref_item->string;
+ struct hashmap_entry key;
+
+ hashmap_entry_init(&key, memhash(refname, strlen(refname)));
+ item = hashmap_get(&remote_refs, &key, refname);
+ if (!item)
+ continue; /* can this happen??? */
+
/* Unless we have already decided to ignore this item... */
- if (item->util)
- {
- struct ref *rm = alloc_ref(item->string);
- rm->peer_ref = alloc_ref(item->string);
- oidcpy(&rm->old_oid, item->util);
+ if (!is_null_oid(&item->oid)) {
+ struct ref *rm = alloc_ref(item->refname);
+ rm->peer_ref = alloc_ref(item->refname);
+ oidcpy(&rm->old_oid, &item->oid);
**tail = rm;
*tail = &rm->next;
}
}
-
+ hashmap_free(&remote_refs, 1);
string_list_clear(&remote_refs, 0);
}
--
2.19.0-271-gfe8321ec05
next reply other threads:[~2018-09-26 21:28 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-26 21:28 Junio C Hamano [this message]
2018-09-26 22:59 ` [PATCH] fetch: replace string-list used as a look-up table with a hashmap Stefan Beller
2018-09-27 5:54 ` Jeff King
2018-09-29 18:31 ` Junio C Hamano
2018-09-30 4:57 ` Jeff King
2018-09-27 5:34 ` Jeff King
2018-09-30 2:11 ` Junio C Hamano
2018-10-19 3:48 ` [PATCH v3] " Junio C Hamano
2018-10-22 9:57 ` Johannes Schindelin
2018-10-27 6:47 ` Re*: " Junio C Hamano
2018-10-31 14:50 ` Johannes Schindelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqin2sj6df.fsf@gitster-ct.c.googlers.com \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).