{"thread":{"id":"55065","subject":"[PATCH 0/6] convert hash_pos() to oid_pos()","startedAt":"2021-01-28T06:13:09Z","lastAt":"2021-01-29T01:19:54Z","messageCount":15,"participants":["Jeff King","Junio C Hamano","Derrick Stolee","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"415483","messageId":"YBJVuckmbGriVa//@coredump.intra.peff.net","threadId":"55065","inReplyTo":null,"subject":"[PATCH 0/6] convert hash_pos() to oid_pos()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-28T06:12:09Z","receivedAt":"2021-01-28T06:13:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I started this series in early December, after getting annoyed that we\nstill have a function called sha1_pos(). But before I got a chance to\npolish it, it became hash_pos(). That removed most of my annoyance,\nthough I do still like the type-safety that using object_id brings.\n\nAnd of course there were some interesting cleanups along the way. So I\ndecided to rebase and send it anyway. I think one could argue that\nhash_pos() is a better interface because a caller _could_ use it without\nan object_id.  But in practice we do not seem to do so (the one\nexception is rerere, but I ended up refactoring that away anyway).\n\nSo here it is. Possibly code churn, possibly cleanup. Opinions welcome. :)\n\n  [1/6]: commit_graft_pos(): take an oid instead of a bare hash\n  [2/6]: rerere: check dirname format while iterating rr_cache directory\n  [3/6]: rerere: tighten rr-cache dirname check\n  [4/6]: rerere: use strmap to store rerere directories\n  [5/6]: hash_pos(): convert to oid_pos()\n  [6/6]: oid_pos(): access table through const pointers\n\n builtin/name-rev.c  | 10 +++---\n commit-graph.c      | 30 +++++++++---------\n commit.c            | 18 +++++------\n commit.h            |  2 +-\n hash-lookup.c       | 18 +++++------\n hash-lookup.h       | 10 +++---\n oid-array.c         |  8 ++---\n pack-bitmap-write.c |  8 ++---\n rerere.c            | 75 ++++++++++++++++++++-------------------------\n shallow.c           |  2 +-\n 10 files changed, 87 insertions(+), 94 deletions(-)\n\n-Peff\n"},{"id":"415484","messageId":"YBJV07YU1Y+siwZc@coredump.intra.peff.net","threadId":"55065","inReplyTo":"YBJVuckmbGriVa//@coredump.intra.peff.net","subject":"[PATCH 1/6] commit_graft_pos(): take an oid instead of a bare hash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-28T06:12:35Z","receivedAt":"2021-01-28T06:13:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"All of our callers have an object_id, and are just dereferencing the\nhash field to pass to us. Let's take the actual object_id instead. We\nstill access the hash to pass to hash_pos, but it's a step in the right\ndirection.\n\nThis makes the callers slightly simpler, but also gets rid of the\nuntyped pointer, as well as the now-inaccurate name \"sha1\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI think this one is an obvious cleanup that we'd want whether we go\nfurther in the series or not.\n\n commit.c  | 8 ++++----\n commit.h  | 2 +-\n shallow.c | 2 +-\n 3 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex bab8d5ab07..fa26729ba5 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -111,17 +111,17 @@ static const unsigned char *commit_graft_sha1_access(size_t index, void *table)\n \treturn commit_graft_table[index]->oid.hash;\n }\n \n-int commit_graft_pos(struct repository *r, const unsigned char *sha1)\n+int commit_graft_pos(struct repository *r, const struct object_id *oid)\n {\n-\treturn hash_pos(sha1, r->parsed_objects->grafts,\n+\treturn hash_pos(oid->hash, r->parsed_objects->grafts,\n \t\t\tr->parsed_objects->grafts_nr,\n \t\t\tcommit_graft_sha1_access);\n }\n \n int register_commit_graft(struct repository *r, struct commit_graft *graft,\n \t\t\t  int ignore_dups)\n {\n-\tint pos = commit_graft_pos(r, graft->oid.hash);\n+\tint pos = commit_graft_pos(r, &graft->oid);\n \n \tif (0 <= pos) {\n \t\tif (ignore_dups)\n@@ -232,7 +232,7 @@ struct commit_graft *lookup_commit_graft(struct repository *r, const struct obje\n {\n \tint pos;\n \tprepare_commit_graft(r);\n-\tpos = commit_graft_pos(r, oid->hash);\n+\tpos = commit_graft_pos(r, oid);\n \tif (pos < 0)\n \t\treturn NULL;\n \treturn r->parsed_objects->grafts[pos];\ndiff --git a/commit.h b/commit.h\nindex f4e7b0158e..ecacf9ade3 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -239,7 +239,7 @@ typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n \n struct commit_graft *read_graft_line(struct strbuf *line);\n /* commit_graft_pos returns an index into r->parsed_objects->grafts. */\n-int commit_graft_pos(struct repository *r, const unsigned char *sha1);\n+int commit_graft_pos(struct repository *r, const struct object_id *oid);\n int register_commit_graft(struct repository *r, struct commit_graft *, int);\n void prepare_commit_graft(struct repository *r);\n struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);\ndiff --git a/shallow.c b/shallow.c\nindex 91b9e1073c..9ed18eb884 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -41,7 +41,7 @@ int register_shallow(struct repository *r, const struct object_id *oid)\n \n int unregister_shallow(const struct object_id *oid)\n {\n-\tint pos = commit_graft_pos(the_repository, oid->hash);\n+\tint pos = commit_graft_pos(the_repository, oid);\n \tif (pos < 0)\n \t\treturn -1;\n \tif (pos + 1 < the_repository->parsed_objects->grafts_nr)\n-- \n2.30.0.758.gfe500d6872\n\n"},{"id":"415486","messageId":"YBJWM0v/D6afqBLI@coredump.intra.peff.net","threadId":"55065","inReplyTo":"YBJVuckmbGriVa//@coredump.intra.peff.net","subject":"[PATCH 2/6] rerere: check dirname format while iterating rr_cache directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-28T06:14:11Z","receivedAt":"2021-01-28T06:15:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In rerere_gc(), we walk over the .git/rr_cache directory and create a\nstruct for each entry we find. We feed any name we get from readdir() to\nfind_rerere_dir(), which then calls get_sha1_hex() on it (since we use\nthe binary hash as a lookup key). If that fails (i.e., the directory\nname is not what we expected), it returns NULL. But the comment in\nfind_rerere_dir() says \"BUG\".\n\nIt _would_ be a bug for the call from new_rerere_id_hex(), the only\nother code path, to fail here; it's generating the hex internally. But\nthe call in rerere_gc() is using it say \"is this a plausible directory\nname\".\n\nLet's instead have rerere_gc() do its own \"is this plausible\" check.\nThat has two benefits:\n\n  - we can now reliably BUG() inside find_rerere_dir(), which would\n    catch bugs in the other code path (and we now will never return NULL\n    from the function, which makes it easier to see that a rerere_id\n    struct will always have a non-NULL \"collection\" field).\n\n  - it makes the use of the binary hash an implementation detail of\n    find_rerere_dir(), not known by callers. That will free us up to\n    change it in a future patch.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nObviously I have an ulterior motive here, but mostly this just seemed\nto make the code clearer to me. The separation of concerns might also\nhelp if we did something more exotic with rerere conflict hashes (which\njust use the_hash_algo now, but really don't have to).\n\n rerere.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex d6928c1b5c..7b0c262ac6 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -146,7 +146,7 @@ static struct rerere_dir *find_rerere_dir(const char *hex)\n \tint pos;\n \n \tif (get_sha1_hex(hex, hash))\n-\t\treturn NULL; /* BUG */\n+\t\tBUG(\"cannot parse rerere dir hex?\");\n \tpos = hash_pos(hash, rerere_dir, rerere_dir_nr, rerere_dir_hash);\n \tif (pos < 0) {\n \t\trr_dir = xmalloc(sizeof(*rr_dir));\n@@ -1178,6 +1178,13 @@ static void prune_one(struct rerere_id *id,\n \t\tunlink_rr_item(id);\n }\n \n+/* Does the basename in \"path\" look plausibly like an rr-cache entry? */\n+static int is_rr_cache_dirname(const char *path)\n+{\n+\tunsigned char hash[GIT_MAX_RAWSZ];\n+\treturn !get_sha1_hex(path, hash);\n+}\n+\n void rerere_gc(struct repository *r, struct string_list *rr)\n {\n \tstruct string_list to_remove = STRING_LIST_INIT_DUP;\n@@ -1205,10 +1212,11 @@ void rerere_gc(struct repository *r, struct string_list *rr)\n \n \t\tif (is_dot_or_dotdot(e->d_name))\n \t\t\tcontinue;\n-\t\trr_dir = find_rerere_dir(e->d_name);\n-\t\tif (!rr_dir)\n+\t\tif (!is_rr_cache_dirname(e->d_name))\n \t\t\tcontinue; /* or should we remove e->d_name? */\n \n+\t\trr_dir = find_rerere_dir(e->d_name);\n+\n \t\tnow_empty = 1;\n \t\tfor (id.variant = 0, id.collection = rr_dir;\n \t\t     id.variant < id.collection->status_nr;\n-- \n2.30.0.758.gfe500d6872\n\n"},{"id":"415487","messageId":"YBJW0gRn9SnP6N9d@coredump.intra.peff.net","threadId":"55065","inReplyTo":"YBJVuckmbGriVa//@coredump.intra.peff.net","subject":"[PATCH 3/6] rerere: tighten rr-cache dirname check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-28T06:16:50Z","receivedAt":"2021-01-28T06:17:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We check only that get_sha1_hex() doesn't complain, which means we'd\nmatch an all-hex name with trailing cruft after it. This probably\ndoesn't matter much in practice, since there shouldn't be anything else\nni the rr-cache directory, but it could possibly cause us to mix up sha1\nand sha256 entries (which also shouldn't be intermingled, but could be\nleftovers from a repository conversion).\n\nNote that \"get_sha1_hex()\" is a confusing historical name. It is\nactually using the_hash_algo, so it would be sha256 in a sha256 repo.\nWe'll switch to using parse_oid_hex(), because that conveniently\nadvances our pointer. But it also gets rid of the sha1 name. Arguably\nit's a little funny to use \"object_id\" here for something that isn't\nactually naming an object, but it's unlikely to be a problem (and is\ncontained in a single function).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is mostly just an oddity I noticed while touching the code, and\nI've never seen triggered in real life.\n\n rerere.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex 7b0c262ac6..e92e305f96 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -1181,8 +1181,9 @@ static void prune_one(struct rerere_id *id,\n /* Does the basename in \"path\" look plausibly like an rr-cache entry? */\n static int is_rr_cache_dirname(const char *path)\n {\n-\tunsigned char hash[GIT_MAX_RAWSZ];\n-\treturn !get_sha1_hex(path, hash);\n+\tstruct object_id oid;\n+\tconst char *end;\n+\treturn !parse_oid_hex(path, &oid, &end) && !*end;\n }\n \n void rerere_gc(struct repository *r, struct string_list *rr)\n-- \n2.30.0.758.gfe500d6872\n\n"},{"id":"415488","messageId":"YBJXYMFu8dhOabDv@coredump.intra.peff.net","threadId":"55065","inReplyTo":"YBJVuckmbGriVa//@coredump.intra.peff.net","subject":"Re: [PATCH 4/6] rerere: use strmap to store rerere directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-28T06:19:12Z","receivedAt":"2021-01-28T06:20:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 03, 2020 at 11:28:14AM -0500, Jeff King wrote:\n\n> We store a struct for each directory we access under .git/rr-cache. The\n> structs are kept in an array sorted by the binary hash associated with\n> their name (and we do lookups with a binary search).\n> \n> This works OK, but there are a few small downsides:\n> \n>  - the amount of code isn't huge, but it's more than we'd need using one\n>    of our other stock data structures\n> \n>  - the insertion into a sorted array is quadratic (though in practice\n>    it's unlikely anybody has enough conflicts for this to matter)\n> \n>  - it's intimately tied to the representation of an object hash. This\n>    isn't a big deal, as the conflict ids we generate use the same hash,\n>    but it produces a few awkward bits (e.g., we are the only user of\n>    hash_pos() that is not using object_id).\n> \n> Let's instead just treat the directory names as strings, and store them\n> in a strmap. This is less code, and removes the use of hash_pos().\n> \n> Insertion is now non-quadratic, though we probably use a bit more\n> memory. Besides the hash table overhead, and storing hex bytes instead\n> of a binary hash, we actually store each name twice. Other code expects\n> to access the name of a rerere_dir struct from the struct itself, so we\n> need a copy there. But strmap keeps its own copy of the name, as well.\n> \n> Using a bare hashmap instead of strmap means we could use the name for\n> both, but at the cost of extra code (e.g., our own comparison function).\n> Likewise, strmap has a feature to use a pointer to the in-struct name at\n> the cost of a little extra code. I didn't do either here, as simple code\n> seemed more important than squeezing out a few bytes of efficiency.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\nThis one might be controversial, or at least considered unnecessary\nchurn. Because the benefits I listed above are pretty negligible, and\nreally my ulterior motive is getting rid of the call to hash_pos().\n\nStill, it was nice to try out strmap a bit more, and the resulting code\nis shorter. It also abstracts things more, if we were to do something\nmore exotic with hashes (right now I think a sha256 repo just has sha256\nconflict hashes, and everything would just work. I don't have any plans\nto change that, but this would be a better base if somebody wanted to do\nso later).\n\n>  rerere.c | 62 +++++++++++++++++++++-----------------------------------\n>  1 file changed, 23 insertions(+), 39 deletions(-)\n> \n> diff --git a/rerere.c b/rerere.c\n> index e92e305f96..dee60dc6df 100644\n> --- a/rerere.c\n> +++ b/rerere.c\n> @@ -11,6 +11,7 @@\n>  #include \"pathspec.h\"\n>  #include \"object-store.h\"\n>  #include \"hash-lookup.h\"\n> +#include \"strmap.h\"\n>  \n>  #define RESOLVED 0\n>  #define PUNTED 1\n> @@ -23,26 +24,27 @@ static int rerere_enabled = -1;\n>  /* automatically update cleanly resolved paths to the index */\n>  static int rerere_autoupdate;\n>  \n> -static int rerere_dir_nr;\n> -static int rerere_dir_alloc;\n> -\n>  #define RR_HAS_POSTIMAGE 1\n>  #define RR_HAS_PREIMAGE 2\n> -static struct rerere_dir {\n> -\tunsigned char hash[GIT_MAX_HEXSZ];\n> +struct rerere_dir {\n>  \tint status_alloc, status_nr;\n>  \tunsigned char *status;\n> -} **rerere_dir;\n> +\tchar name[FLEX_ARRAY];\n> +};\n> +\n> +static struct strmap rerere_dirs = STRMAP_INIT;\n>  \n>  static void free_rerere_dirs(void)\n>  {\n> -\tint i;\n> -\tfor (i = 0; i < rerere_dir_nr; i++) {\n> -\t\tfree(rerere_dir[i]->status);\n> -\t\tfree(rerere_dir[i]);\n> +\tstruct hashmap_iter iter;\n> +\tstruct strmap_entry *ent;\n> +\n> +\tstrmap_for_each_entry(&rerere_dirs, &iter, ent) {\n> +\t\tstruct rerere_dir *rr_dir = ent->value;\n> +\t\tfree(rr_dir->status);\n> +\t\tfree(rr_dir);\n>  \t}\n> -\tFREE_AND_NULL(rerere_dir);\n> -\trerere_dir_nr = rerere_dir_alloc = 0;\n> +\tstrmap_clear(&rerere_dirs, 0);\n>  }\n>  \n>  static void free_rerere_id(struct string_list_item *item)\n> @@ -52,7 +54,7 @@ static void free_rerere_id(struct string_list_item *item)\n>  \n>  static const char *rerere_id_hex(const struct rerere_id *id)\n>  {\n> -\treturn hash_to_hex(id->collection->hash);\n> +\treturn id->collection->name;\n>  }\n>  \n>  static void fit_variant(struct rerere_dir *rr_dir, int variant)\n> @@ -115,7 +117,7 @@ static int is_rr_file(const char *name, const char *filename, int *variant)\n>  static void scan_rerere_dir(struct rerere_dir *rr_dir)\n>  {\n>  \tstruct dirent *de;\n> -\tDIR *dir = opendir(git_path(\"rr-cache/%s\", hash_to_hex(rr_dir->hash)));\n> +\tDIR *dir = opendir(git_path(\"rr-cache/%s\", rr_dir->name));\n>  \n>  \tif (!dir)\n>  \t\treturn;\n> @@ -133,39 +135,21 @@ static void scan_rerere_dir(struct rerere_dir *rr_dir)\n>  \tclosedir(dir);\n>  }\n>  \n> -static const unsigned char *rerere_dir_hash(size_t i, void *table)\n> -{\n> -\tstruct rerere_dir **rr_dir = table;\n> -\treturn rr_dir[i]->hash;\n> -}\n> -\n>  static struct rerere_dir *find_rerere_dir(const char *hex)\n>  {\n> -\tunsigned char hash[GIT_MAX_RAWSZ];\n>  \tstruct rerere_dir *rr_dir;\n> -\tint pos;\n> -\n> -\tif (get_sha1_hex(hex, hash))\n> -\t\tBUG(\"cannot parse rerere dir hex?\");\n> -\tpos = hash_pos(hash, rerere_dir, rerere_dir_nr, rerere_dir_hash);\n> -\tif (pos < 0) {\n> -\t\trr_dir = xmalloc(sizeof(*rr_dir));\n> -\t\thashcpy(rr_dir->hash, hash);\n> +\n> +\trr_dir = strmap_get(&rerere_dirs, hex);\n> +\tif (!rr_dir) {\n> +\t\tFLEX_ALLOC_STR(rr_dir, name, hex);\n>  \t\trr_dir->status = NULL;\n>  \t\trr_dir->status_nr = 0;\n>  \t\trr_dir->status_alloc = 0;\n> -\t\tpos = -1 - pos;\n> -\n> -\t\t/* Make sure the array is big enough ... */\n> -\t\tALLOC_GROW(rerere_dir, rerere_dir_nr + 1, rerere_dir_alloc);\n> -\t\t/* ... and add it in. */\n> -\t\trerere_dir_nr++;\n> -\t\tMOVE_ARRAY(rerere_dir + pos + 1, rerere_dir + pos,\n> -\t\t\t   rerere_dir_nr - pos - 1);\n> -\t\trerere_dir[pos] = rr_dir;\n> +\t\tstrmap_put(&rerere_dirs, hex, rr_dir);\n> +\n>  \t\tscan_rerere_dir(rr_dir);\n>  \t}\n> -\treturn rerere_dir[pos];\n> +\treturn rr_dir;\n>  }\n>  \n>  static int has_rerere_resolution(const struct rerere_id *id)\n> -- \n> 2.30.0.758.gfe500d6872\n> \n"},{"id":"415489","messageId":"YBJXfl86Juv5kn16@coredump.intra.peff.net","threadId":"55065","inReplyTo":"YBJVuckmbGriVa//@coredump.intra.peff.net","subject":"[PATCH 5/6] hash_pos(): convert to oid_pos()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-28T06:19:42Z","receivedAt":"2021-01-28T06:20:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"All of our callers are actually looking up an object_id, not a bare\nhash. Likewise, the arrays they are looking in are actual arrays of\nobject_id (not just raw bytes of hashes, as we might find in a pack\n.idx; those are handled by bsearch_hash()).\n\nUsing an object_id gives us more type safety, and makes the callers\nslightly shorter. It also gets rid of the word \"sha1\" from several\naccess functions, though we could obviously also rename those with\ns/sha1/hash/.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIf we don't want to make this change, I think it's still worth sweeping\nthrough the callers and changing the names of their access functions.\n\n builtin/name-rev.c  |  8 ++++----\n commit-graph.c      | 28 ++++++++++++++--------------\n commit.c            | 10 +++++-----\n hash-lookup.c       | 18 +++++++++---------\n hash-lookup.h       | 10 +++++-----\n oid-array.c         |  6 +++---\n pack-bitmap-write.c |  6 +++---\n 7 files changed, 43 insertions(+), 43 deletions(-)\n\ndiff --git a/builtin/name-rev.c b/builtin/name-rev.c\nindex 3fe71a8c01..27138fdce4 100644\n--- a/builtin/name-rev.c\n+++ b/builtin/name-rev.c\n@@ -390,10 +390,10 @@ static void name_tips(void)\n \t}\n }\n \n-static const unsigned char *nth_tip_table_ent(size_t ix, void *table_)\n+static const struct object_id *nth_tip_table_ent(size_t ix, void *table_)\n {\n \tstruct tip_table_entry *table = table_;\n-\treturn table[ix].oid.hash;\n+\treturn &table[ix].oid;\n }\n \n static const char *get_exact_ref_match(const struct object *o)\n@@ -408,8 +408,8 @@ static const char *get_exact_ref_match(const struct object *o)\n \t\ttip_table.sorted = 1;\n \t}\n \n-\tfound = hash_pos(o->oid.hash, tip_table.table, tip_table.nr,\n-\t\t\t nth_tip_table_ent);\n+\tfound = oid_pos(&o->oid, tip_table.table, tip_table.nr,\n+\t\t\tnth_tip_table_ent);\n \tif (0 <= found)\n \t\treturn tip_table.table[found].refname;\n \treturn NULL;\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f3486ec18f..248f1efb73 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1012,10 +1012,10 @@ static int write_graph_chunk_oids(struct hashfile *f,\n \treturn 0;\n }\n \n-static const unsigned char *commit_to_sha1(size_t index, void *table)\n+static const struct object_id *commit_to_oid(size_t index, void *table)\n {\n \tstruct commit **commits = table;\n-\treturn commits[index]->object.oid.hash;\n+\treturn &commits[index]->object.oid;\n }\n \n static int write_graph_chunk_data(struct hashfile *f,\n@@ -1043,10 +1043,10 @@ static int write_graph_chunk_data(struct hashfile *f,\n \t\tif (!parent)\n \t\t\tedge_value = GRAPH_PARENT_NONE;\n \t\telse {\n-\t\t\tedge_value = hash_pos(parent->item->object.oid.hash,\n-\t\t\t\t\t      ctx->commits.list,\n-\t\t\t\t\t      ctx->commits.nr,\n-\t\t\t\t\t      commit_to_sha1);\n+\t\t\tedge_value = oid_pos(&parent->item->object.oid,\n+\t\t\t\t\t     ctx->commits.list,\n+\t\t\t\t\t     ctx->commits.nr,\n+\t\t\t\t\t     commit_to_oid);\n \n \t\t\tif (edge_value >= 0)\n \t\t\t\tedge_value += ctx->new_num_commits_in_base;\n@@ -1074,10 +1074,10 @@ static int write_graph_chunk_data(struct hashfile *f,\n \t\telse if (parent->next)\n \t\t\tedge_value = GRAPH_EXTRA_EDGES_NEEDED | num_extra_edges;\n \t\telse {\n-\t\t\tedge_value = hash_pos(parent->item->object.oid.hash,\n-\t\t\t\t\t      ctx->commits.list,\n-\t\t\t\t\t      ctx->commits.nr,\n-\t\t\t\t\t      commit_to_sha1);\n+\t\t\tedge_value = oid_pos(&parent->item->object.oid,\n+\t\t\t\t\t     ctx->commits.list,\n+\t\t\t\t\t     ctx->commits.nr,\n+\t\t\t\t\t     commit_to_oid);\n \n \t\t\tif (edge_value >= 0)\n \t\t\t\tedge_value += ctx->new_num_commits_in_base;\n@@ -1143,10 +1143,10 @@ static int write_graph_chunk_extra_edges(struct hashfile *f,\n \n \t\t/* Since num_parents > 2, this initializer is safe. */\n \t\tfor (parent = (*list)->parents->next; parent; parent = parent->next) {\n-\t\t\tint edge_value = hash_pos(parent->item->object.oid.hash,\n-\t\t\t\t\t\t  ctx->commits.list,\n-\t\t\t\t\t\t  ctx->commits.nr,\n-\t\t\t\t\t\t  commit_to_sha1);\n+\t\t\tint edge_value = oid_pos(&parent->item->object.oid,\n+\t\t\t\t\t\t ctx->commits.list,\n+\t\t\t\t\t\t ctx->commits.nr,\n+\t\t\t\t\t\t commit_to_oid);\n \n \t\t\tif (edge_value >= 0)\n \t\t\t\tedge_value += ctx->new_num_commits_in_base;\ndiff --git a/commit.c b/commit.c\nindex fa26729ba5..39eab5b36b 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -105,17 +105,17 @@ static timestamp_t parse_commit_date(const char *buf, const char *tail)\n \treturn parse_timestamp(dateptr, NULL, 10);\n }\n \n-static const unsigned char *commit_graft_sha1_access(size_t index, void *table)\n+static const struct object_id *commit_graft_oid_access(size_t index, void *table)\n {\n \tstruct commit_graft **commit_graft_table = table;\n-\treturn commit_graft_table[index]->oid.hash;\n+\treturn &commit_graft_table[index]->oid;\n }\n \n int commit_graft_pos(struct repository *r, const struct object_id *oid)\n {\n-\treturn hash_pos(oid->hash, r->parsed_objects->grafts,\n-\t\t\tr->parsed_objects->grafts_nr,\n-\t\t\tcommit_graft_sha1_access);\n+\treturn oid_pos(oid, r->parsed_objects->grafts,\n+\t\t       r->parsed_objects->grafts_nr,\n+\t\t       commit_graft_oid_access);\n }\n \n int register_commit_graft(struct repository *r, struct commit_graft *graft,\ndiff --git a/hash-lookup.c b/hash-lookup.c\nindex 1191856a32..d15bb34574 100644\n--- a/hash-lookup.c\n+++ b/hash-lookup.c\n@@ -1,9 +1,9 @@\n #include \"cache.h\"\n #include \"hash-lookup.h\"\n \n-static uint32_t take2(const unsigned char *hash)\n+static uint32_t take2(const struct object_id *oid, size_t ofs)\n {\n-\treturn ((hash[0] << 8) | hash[1]);\n+\treturn ((oid->hash[ofs] << 8) | oid->hash[ofs + 1]);\n }\n \n /*\n@@ -47,11 +47,11 @@ static uint32_t take2(const unsigned char *hash)\n  */\n /*\n  * The table should contain \"nr\" elements.\n- * The hash of element i (between 0 and nr - 1) should be returned\n+ * The oid of element i (between 0 and nr - 1) should be returned\n  * by \"fn(i, table)\".\n  */\n-int hash_pos(const unsigned char *hash, void *table, size_t nr,\n-\t     hash_access_fn fn)\n+int oid_pos(const struct object_id *oid, void *table, size_t nr,\n+\t    oid_access_fn fn)\n {\n \tsize_t hi = nr;\n \tsize_t lo = 0;\n@@ -64,9 +64,9 @@ int hash_pos(const unsigned char *hash, void *table, size_t nr,\n \t\tsize_t lov, hiv, miv, ofs;\n \n \t\tfor (ofs = 0; ofs < the_hash_algo->rawsz - 2; ofs += 2) {\n-\t\t\tlov = take2(fn(0, table) + ofs);\n-\t\t\thiv = take2(fn(nr - 1, table) + ofs);\n-\t\t\tmiv = take2(hash + ofs);\n+\t\t\tlov = take2(fn(0, table), ofs);\n+\t\t\thiv = take2(fn(nr - 1, table), ofs);\n+\t\t\tmiv = take2(oid, ofs);\n \t\t\tif (miv < lov)\n \t\t\t\treturn -1;\n \t\t\tif (hiv < miv)\n@@ -88,7 +88,7 @@ int hash_pos(const unsigned char *hash, void *table, size_t nr,\n \n \tdo {\n \t\tint cmp;\n-\t\tcmp = hashcmp(fn(mi, table), hash);\n+\t\tcmp = oidcmp(fn(mi, table), oid);\n \t\tif (!cmp)\n \t\t\treturn mi;\n \t\tif (cmp > 0)\ndiff --git a/hash-lookup.h b/hash-lookup.h\nindex 5d476dec72..7b3ecad1f0 100644\n--- a/hash-lookup.h\n+++ b/hash-lookup.h\n@@ -1,12 +1,12 @@\n #ifndef HASH_LOOKUP_H\n #define HASH_LOOKUP_H\n \n-typedef const unsigned char *hash_access_fn(size_t index, void *table);\n+typedef const struct object_id *oid_access_fn(size_t index, void *table);\n \n-int hash_pos(const unsigned char *hash,\n-\t     void *table,\n-\t     size_t nr,\n-\t     hash_access_fn fn);\n+int oid_pos(const struct object_id *oid,\n+\t    void *table,\n+\t    size_t nr,\n+\t    oid_access_fn fn);\n \n /*\n  * Searches for hash in table, using the given fanout table to determine the\ndiff --git a/oid-array.c b/oid-array.c\nindex 889b311f22..a19235afbf 100644\n--- a/oid-array.c\n+++ b/oid-array.c\n@@ -22,16 +22,16 @@ void oid_array_sort(struct oid_array *array)\n \tarray->sorted = 1;\n }\n \n-static const unsigned char *sha1_access(size_t index, void *table)\n+static const struct object_id *oid_access(size_t index, void *table)\n {\n \tstruct object_id *array = table;\n-\treturn array[index].hash;\n+\treturn &array[index];\n }\n \n int oid_array_lookup(struct oid_array *array, const struct object_id *oid)\n {\n \toid_array_sort(array);\n-\treturn hash_pos(oid->hash, array->oid, array->nr, sha1_access);\n+\treturn oid_pos(oid, array->oid, array->nr, oid_access);\n }\n \n void oid_array_clear(struct oid_array *array)\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex 92460a6126..f21259dfc8 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -610,10 +610,10 @@ static inline void dump_bitmap(struct hashfile *f, struct ewah_bitmap *bitmap)\n \t\tdie(\"Failed to write bitmap index\");\n }\n \n-static const unsigned char *sha1_access(size_t pos, void *table)\n+static const struct object_id *oid_access(size_t pos, void *table)\n {\n \tstruct pack_idx_entry **index = table;\n-\treturn index[pos]->oid.hash;\n+\treturn &index[pos]->oid;\n }\n \n static void write_selected_commits_v1(struct hashfile *f,\n@@ -626,7 +626,7 @@ static void write_selected_commits_v1(struct hashfile *f,\n \t\tstruct bitmapped_commit *stored = &writer.selected[i];\n \n \t\tint commit_pos =\n-\t\t\thash_pos(stored->commit->object.oid.hash, index, index_nr, sha1_access);\n+\t\t\toid_pos(&stored->commit->object.oid, index, index_nr, oid_access);\n \n \t\tif (commit_pos < 0)\n \t\t\tBUG(\"trying to write commit not in index\");\n-- \n2.30.0.758.gfe500d6872\n\n"},{"id":"415490","messageId":"YBJXp9Tr9kHrgYsp@coredump.intra.peff.net","threadId":"55065","inReplyTo":"YBJVuckmbGriVa//@coredump.intra.peff.net","subject":"[PATCH 6/6] oid_pos(): access table through const pointers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-28T06:20:23Z","receivedAt":"2021-01-28T06:21:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are looking up an oid in an array, we obviously don't need to\nwrite to the array. Let's mark it as const in the function interfaces,\nas well as in the local variables we use to derference the void pointer\n(note a few cases use pointers-to-pointers, so we mark everything\nconst).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is just a hygiene thing. I think it's pretty unlikely for one of\nthese 2-line access functions to somehow forget that the table is\nsupposed to be const.\n\n builtin/name-rev.c  | 4 ++--\n commit-graph.c      | 4 ++--\n commit.c            | 4 ++--\n hash-lookup.c       | 2 +-\n hash-lookup.h       | 4 ++--\n oid-array.c         | 4 ++--\n pack-bitmap-write.c | 4 ++--\n 7 files changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/name-rev.c b/builtin/name-rev.c\nindex 27138fdce4..b221d30014 100644\n--- a/builtin/name-rev.c\n+++ b/builtin/name-rev.c\n@@ -390,9 +390,9 @@ static void name_tips(void)\n \t}\n }\n \n-static const struct object_id *nth_tip_table_ent(size_t ix, void *table_)\n+static const struct object_id *nth_tip_table_ent(size_t ix, const void *table_)\n {\n-\tstruct tip_table_entry *table = table_;\n+\tconst struct tip_table_entry *table = table_;\n \treturn &table[ix].oid;\n }\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex 248f1efb73..b09fce5f57 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1012,9 +1012,9 @@ static int write_graph_chunk_oids(struct hashfile *f,\n \treturn 0;\n }\n \n-static const struct object_id *commit_to_oid(size_t index, void *table)\n+static const struct object_id *commit_to_oid(size_t index, const void *table)\n {\n-\tstruct commit **commits = table;\n+\tconst struct commit * const *commits = table;\n \treturn &commits[index]->object.oid;\n }\n \ndiff --git a/commit.c b/commit.c\nindex 39eab5b36b..fd2831dad3 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -105,9 +105,9 @@ static timestamp_t parse_commit_date(const char *buf, const char *tail)\n \treturn parse_timestamp(dateptr, NULL, 10);\n }\n \n-static const struct object_id *commit_graft_oid_access(size_t index, void *table)\n+static const struct object_id *commit_graft_oid_access(size_t index, const void *table)\n {\n-\tstruct commit_graft **commit_graft_table = table;\n+\tconst struct commit_graft * const *commit_graft_table = table;\n \treturn &commit_graft_table[index]->oid;\n }\n \ndiff --git a/hash-lookup.c b/hash-lookup.c\nindex d15bb34574..b98ed5e11e 100644\n--- a/hash-lookup.c\n+++ b/hash-lookup.c\n@@ -50,7 +50,7 @@ static uint32_t take2(const struct object_id *oid, size_t ofs)\n  * The oid of element i (between 0 and nr - 1) should be returned\n  * by \"fn(i, table)\".\n  */\n-int oid_pos(const struct object_id *oid, void *table, size_t nr,\n+int oid_pos(const struct object_id *oid, const void *table, size_t nr,\n \t    oid_access_fn fn)\n {\n \tsize_t hi = nr;\ndiff --git a/hash-lookup.h b/hash-lookup.h\nindex 7b3ecad1f0..dbd71ebaf7 100644\n--- a/hash-lookup.h\n+++ b/hash-lookup.h\n@@ -1,10 +1,10 @@\n #ifndef HASH_LOOKUP_H\n #define HASH_LOOKUP_H\n \n-typedef const struct object_id *oid_access_fn(size_t index, void *table);\n+typedef const struct object_id *oid_access_fn(size_t index, const void *table);\n \n int oid_pos(const struct object_id *oid,\n-\t    void *table,\n+\t    const void *table,\n \t    size_t nr,\n \t    oid_access_fn fn);\n \ndiff --git a/oid-array.c b/oid-array.c\nindex a19235afbf..73ba76e9e9 100644\n--- a/oid-array.c\n+++ b/oid-array.c\n@@ -22,9 +22,9 @@ void oid_array_sort(struct oid_array *array)\n \tarray->sorted = 1;\n }\n \n-static const struct object_id *oid_access(size_t index, void *table)\n+static const struct object_id *oid_access(size_t index, const void *table)\n {\n-\tstruct object_id *array = table;\n+\tconst struct object_id *array = table;\n \treturn &array[index];\n }\n \ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex f21259dfc8..88d9e696a5 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -610,9 +610,9 @@ static inline void dump_bitmap(struct hashfile *f, struct ewah_bitmap *bitmap)\n \t\tdie(\"Failed to write bitmap index\");\n }\n \n-static const struct object_id *oid_access(size_t pos, void *table)\n+static const struct object_id *oid_access(size_t pos, const void *table)\n {\n-\tstruct pack_idx_entry **index = table;\n+\tconst struct pack_idx_entry * const *index = table;\n \treturn &index[pos]->oid;\n }\n \n-- \n2.30.0.758.gfe500d6872\n"},{"id":"415496","messageId":"YBJa98B1a6mP/Bsx@coredump.intra.peff.net","threadId":"55065","inReplyTo":"YBJXYMFu8dhOabDv@coredump.intra.peff.net","subject":"Re: [PATCH 4/6] rerere: use strmap to store rerere directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-28T06:34:31Z","receivedAt":"2021-01-28T06:35:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 28, 2021 at 01:19:12AM -0500, Jeff King wrote:\n\n> On Thu, Dec 03, 2020 at 11:28:14AM -0500, Jeff King wrote:\n> \n> > We store a struct for each directory we access under .git/rr-cache. The\n> > structs are kept in an array sorted by the binary hash associated with\n> > their name (and we do lookups with a binary search).\n\nHmph. Apparently I accidentally sent this as a reply instead of sending\nthe original format-patch output. Oops.\n\nHere it is minus the quoting, which should make it easier to apply. :)\n\n-- >8 --\nSubject: [PATCH] rerere: use strmap to store rerere directories\n\nWe store a struct for each directory we access under .git/rr-cache. The\nstructs are kept in an array sorted by the binary hash associated with\ntheir name (and we do lookups with a binary search).\n\nThis works OK, but there are a few small downsides:\n\n - the amount of code isn't huge, but it's more than we'd need using one\n   of our other stock data structures\n\n - the insertion into a sorted array is quadratic (though in practice\n   it's unlikely anybody has enough conflicts for this to matter)\n\n - it's intimately tied to the representation of an object hash. This\n   isn't a big deal, as the conflict ids we generate use the same hash,\n   but it produces a few awkward bits (e.g., we are the only user of\n   hash_pos() that is not using object_id).\n\nLet's instead just treat the directory names as strings, and store them\nin a strmap. This is less code, and removes the use of hash_pos().\n\nInsertion is now non-quadratic, though we probably use a bit more\nmemory. Besides the hash table overhead, and storing hex bytes instead\nof a binary hash, we actually store each name twice. Other code expects\nto access the name of a rerere_dir struct from the struct itself, so we\nneed a copy there. But strmap keeps its own copy of the name, as well.\n\nUsing a bare hashmap instead of strmap means we could use the name for\nboth, but at the cost of extra code (e.g., our own comparison function).\nLikewise, strmap has a feature to use a pointer to the in-struct name at\nthe cost of a little extra code. I didn't do either here, as simple code\nseemed more important than squeezing out a few bytes of efficiency.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis one might be controversial, or at least considered unnecessary\nchurn. Because the benefits I listed above are pretty negligible, and\nreally my ulterior motive is getting rid of the call to hash_pos().\n\nStill, it was nice to try out strmap a bit more, and the resulting code\nis shorter. It also abstracts things more, if we were to do something\nmore exotic with hashes (right now I think a sha256 repo just has sha256\nconflict hashes, and everything would just work. I don't have any plans\nto change that, but this would be a better base if somebody wanted to do\nso later).\n\n rerere.c | 62 +++++++++++++++++++++-----------------------------------\n 1 file changed, 23 insertions(+), 39 deletions(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex e92e305f96..dee60dc6df 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -11,6 +11,7 @@\n #include \"pathspec.h\"\n #include \"object-store.h\"\n #include \"hash-lookup.h\"\n+#include \"strmap.h\"\n \n #define RESOLVED 0\n #define PUNTED 1\n@@ -23,26 +24,27 @@ static int rerere_enabled = -1;\n /* automatically update cleanly resolved paths to the index */\n static int rerere_autoupdate;\n \n-static int rerere_dir_nr;\n-static int rerere_dir_alloc;\n-\n #define RR_HAS_POSTIMAGE 1\n #define RR_HAS_PREIMAGE 2\n-static struct rerere_dir {\n-\tunsigned char hash[GIT_MAX_HEXSZ];\n+struct rerere_dir {\n \tint status_alloc, status_nr;\n \tunsigned char *status;\n-} **rerere_dir;\n+\tchar name[FLEX_ARRAY];\n+};\n+\n+static struct strmap rerere_dirs = STRMAP_INIT;\n \n static void free_rerere_dirs(void)\n {\n-\tint i;\n-\tfor (i = 0; i < rerere_dir_nr; i++) {\n-\t\tfree(rerere_dir[i]->status);\n-\t\tfree(rerere_dir[i]);\n+\tstruct hashmap_iter iter;\n+\tstruct strmap_entry *ent;\n+\n+\tstrmap_for_each_entry(&rerere_dirs, &iter, ent) {\n+\t\tstruct rerere_dir *rr_dir = ent->value;\n+\t\tfree(rr_dir->status);\n+\t\tfree(rr_dir);\n \t}\n-\tFREE_AND_NULL(rerere_dir);\n-\trerere_dir_nr = rerere_dir_alloc = 0;\n+\tstrmap_clear(&rerere_dirs, 0);\n }\n \n static void free_rerere_id(struct string_list_item *item)\n@@ -52,7 +54,7 @@ static void free_rerere_id(struct string_list_item *item)\n \n static const char *rerere_id_hex(const struct rerere_id *id)\n {\n-\treturn hash_to_hex(id->collection->hash);\n+\treturn id->collection->name;\n }\n \n static void fit_variant(struct rerere_dir *rr_dir, int variant)\n@@ -115,7 +117,7 @@ static int is_rr_file(const char *name, const char *filename, int *variant)\n static void scan_rerere_dir(struct rerere_dir *rr_dir)\n {\n \tstruct dirent *de;\n-\tDIR *dir = opendir(git_path(\"rr-cache/%s\", hash_to_hex(rr_dir->hash)));\n+\tDIR *dir = opendir(git_path(\"rr-cache/%s\", rr_dir->name));\n \n \tif (!dir)\n \t\treturn;\n@@ -133,39 +135,21 @@ static void scan_rerere_dir(struct rerere_dir *rr_dir)\n \tclosedir(dir);\n }\n \n-static const unsigned char *rerere_dir_hash(size_t i, void *table)\n-{\n-\tstruct rerere_dir **rr_dir = table;\n-\treturn rr_dir[i]->hash;\n-}\n-\n static struct rerere_dir *find_rerere_dir(const char *hex)\n {\n-\tunsigned char hash[GIT_MAX_RAWSZ];\n \tstruct rerere_dir *rr_dir;\n-\tint pos;\n-\n-\tif (get_sha1_hex(hex, hash))\n-\t\tBUG(\"cannot parse rerere dir hex?\");\n-\tpos = hash_pos(hash, rerere_dir, rerere_dir_nr, rerere_dir_hash);\n-\tif (pos < 0) {\n-\t\trr_dir = xmalloc(sizeof(*rr_dir));\n-\t\thashcpy(rr_dir->hash, hash);\n+\n+\trr_dir = strmap_get(&rerere_dirs, hex);\n+\tif (!rr_dir) {\n+\t\tFLEX_ALLOC_STR(rr_dir, name, hex);\n \t\trr_dir->status = NULL;\n \t\trr_dir->status_nr = 0;\n \t\trr_dir->status_alloc = 0;\n-\t\tpos = -1 - pos;\n-\n-\t\t/* Make sure the array is big enough ... */\n-\t\tALLOC_GROW(rerere_dir, rerere_dir_nr + 1, rerere_dir_alloc);\n-\t\t/* ... and add it in. */\n-\t\trerere_dir_nr++;\n-\t\tMOVE_ARRAY(rerere_dir + pos + 1, rerere_dir + pos,\n-\t\t\t   rerere_dir_nr - pos - 1);\n-\t\trerere_dir[pos] = rr_dir;\n+\t\tstrmap_put(&rerere_dirs, hex, rr_dir);\n+\n \t\tscan_rerere_dir(rr_dir);\n \t}\n-\treturn rerere_dir[pos];\n+\treturn rr_dir;\n }\n \n static int has_rerere_resolution(const struct rerere_id *id)\n-- \n2.30.0.758.gfe500d6872\n\n"},{"id":"415498","messageId":"xmqqzh0twopk.fsf@gitster.c.googlers.com","threadId":"55065","inReplyTo":"YBJXYMFu8dhOabDv@coredump.intra.peff.net","subject":"Re: [PATCH 4/6] rerere: use strmap to store rerere directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-28T06:38:31Z","receivedAt":"2021-01-28T06:39:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Dec 03, 2020 at 11:28:14AM -0500, Jeff King wrote:\n\nCuriously, the reference of the message points at the cover letter\nof this series, and there is no mention of a message from early last\nDecember.\n\nI think this is good.  These \"conflict IDs\" are not even object\nnames, so it would be strange to use \"struct object_id\" for them\n(for that matter, packfile ids are not object names, either, but I\nsuspect we might use object_id there).\n\nIf there were a patch that can readily applied ;-)\n"},{"id":"415499","messageId":"YBJe+8mAF65sFKW+@coredump.intra.peff.net","threadId":"55065","inReplyTo":"xmqqzh0twopk.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 4/6] rerere: use strmap to store rerere directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-28T06:51:39Z","receivedAt":"2021-01-28T06:52:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 27, 2021 at 10:38:31PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Thu, Dec 03, 2020 at 11:28:14AM -0500, Jeff King wrote:\n> \n> Curiously, the reference of the message points at the cover letter\n> of this series, and there is no mention of a message from early last\n> December.\n\nI think that is the author-date generated by format-patch, since that's\nwhen I originally wrote it. I generally format the messages into an\nmbox, load that in mutt, and then use mutt's \"bounce\" feature to send\nthem, which rewrites the date header. In this case, though, I somehow\naccidentally hit \"reply\" (more astounding is that I didn't notice it as\nI was sending; I think I might be getting senile).\n\n> I think this is good.  These \"conflict IDs\" are not even object\n> names, so it would be strange to use \"struct object_id\" for them\n> (for that matter, packfile ids are not object names, either, but I\n> suspect we might use object_id there).\n\nI suspected that, too, but when I checked I couldn't find any instance\nwhere we do. I might not have looked hard enough, or perhaps whoever\nconverted the pack code had better taste than me (see the similar use of\nobject_id in the previous patch).\n\n> If there were a patch that can readily applied ;-)\n\nYep, see my reply. :)\n\n-Peff\n"},{"id":"415500","messageId":"493fcc80-c793-0738-3b8d-b486fd05355b@gmail.com","threadId":"55065","inReplyTo":"YBJV07YU1Y+siwZc@coredump.intra.peff.net","subject":"Re: [PATCH 1/6] commit_graft_pos(): take an oid instead of a bare hash","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-01-28T12:57:25Z","receivedAt":"2021-01-28T12:58:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/28/2021 1:12 AM, Jeff King wrote:\n> All of our callers have an object_id, and are just dereferencing the\n> hash field to pass to us. Let's take the actual object_id instead. We\n> still access the hash to pass to hash_pos, but it's a step in the right\n> direction.\n> \n> This makes the callers slightly simpler, but also gets rid of the\n> untyped pointer, as well as the now-inaccurate name \"sha1\".\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I think this one is an obvious cleanup that we'd want whether we go\n> further in the series or not.\n\nI agree. Thanks,\n-Stolee\n\n"},{"id":"415522","messageId":"xmqq8s8cx2iz.fsf@gitster.c.googlers.com","threadId":"55065","inReplyTo":"YBJa98B1a6mP/Bsx@coredump.intra.peff.net","subject":"Re: [PATCH 4/6] rerere: use strmap to store rerere directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-28T19:52:20Z","receivedAt":"2021-01-28T20:00:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Subject: [PATCH] rerere: use strmap to store rerere directories\n>\n> We store a struct for each directory we access under .git/rr-cache. The\n> structs are kept in an array sorted by the binary hash associated with\n> their name (and we do lookups with a binary search).\n>\n> This works OK, but there are a few small downsides:\n>\n>  - the amount of code isn't huge, but it's more than we'd need using one\n>    of our other stock data structures\n>\n>  - the insertion into a sorted array is quadratic (though in practice\n>    it's unlikely anybody has enough conflicts for this to matter)\n>\n>  - it's intimately tied to the representation of an object hash. This\n>    isn't a big deal, as the conflict ids we generate use the same hash,\n>    but it produces a few awkward bits (e.g., we are the only user of\n>    hash_pos() that is not using object_id).\n>\n> Let's instead just treat the directory names as strings, and store them\n> in a strmap. This is less code, and removes the use of hash_pos().\n\nNice.  I didn't realize that this part is so well isolated that\nanother table implementation can easily be dropped in (it was a long\ntime since I stared at this part of the system, back when I was\nextending it to handle <preimage,postimage> pairs that share the\nsame conflict IDs, which was quite tricky to get right).\n\n> This one might be controversial, or at least considered unnecessary\n> churn. Because the benefits I listed above are pretty negligible, and\n> really my ulterior motive is getting rid of the call to hash_pos().\n\nNo objection from me.  Thanks.\n"},{"id":"415530","messageId":"YBM8G/N/Oo1BMWKa@nand.local","threadId":"55065","inReplyTo":"YBJW0gRn9SnP6N9d@coredump.intra.peff.net","subject":"Re: [PATCH 3/6] rerere: tighten rr-cache dirname check","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-01-28T22:35:07Z","receivedAt":"2021-01-28T22:35:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jan 28, 2021 at 01:16:50AM -0500, Jeff King wrote:\n> Note that \"get_sha1_hex()\" is a confusing historical name. It is\n> actually using the_hash_algo, so it would be sha256 in a sha256 repo.\n> We'll switch to using parse_oid_hex(), because that conveniently\n> advances our pointer. But it also gets rid of the sha1 name. Arguably\n> it's a little funny to use \"object_id\" here for something that isn't\n> actually naming an object, but it's unlikely to be a problem (and is\n> contained in a single function).\n\nNot that it matters, but I think the argument would be that having \"oid\"\nin the function call probably isn't ideal, but it's closer to the ideal\nthan having \"sha1\" in the same call (which isn't always the case).\n\nLikewise about advancing the pointer, since everything is pass-by-value,\nmodifying the stack local copy of path doesn't matter to callers of\nis_rr_cache_dirname().\n\nSo I think that this change is an improvement over the status-quo here.\nThanks,\nTaylor\n"},{"id":"415532","messageId":"YBM/NpLeS0uFzVb3@nand.local","threadId":"55065","inReplyTo":"YBJXfl86Juv5kn16@coredump.intra.peff.net","subject":"Re: [PATCH 5/6] hash_pos(): convert to oid_pos()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-01-28T22:48:22Z","receivedAt":"2021-01-28T22:50:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jan 28, 2021 at 01:19:42AM -0500, Jeff King wrote:\n> All of our callers are actually looking up an object_id, not a bare\n> hash. Likewise, the arrays they are looking in are actual arrays of\n> object_id (not just raw bytes of hashes, as we might find in a pack\n> .idx; those are handled by bsearch_hash()).\n>\n> Using an object_id gives us more type safety, and makes the callers\n> slightly shorter. It also gets rid of the word \"sha1\" from several\n> access functions, though we could obviously also rename those with\n> s/sha1/hash/.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> If we don't want to make this change, I think it's still worth sweeping\n> through the callers and changing the names of their access functions.\n>\n>  builtin/name-rev.c  |  8 ++++----\n>  commit-graph.c      | 28 ++++++++++++++--------------\n>  commit.c            | 10 +++++-----\n>  hash-lookup.c       | 18 +++++++++---------\n>  hash-lookup.h       | 10 +++++-----\n\nI wondered briefly if we should rename this to oid-lookup.{c,h}, but I\nthink the answer is \"no\", since we still have bsearch_hash() in this\nheader.\n\nProbably a single header with \"hash\" in the name is better than two\nheaders each containing a single function (and one containing an\nadditional typedef).\n\nSo, I think that what you did here is good.\n\nThanks,\nTaylor\n"},{"id":"415551","messageId":"YBNifzRKYOjaZ4eV@coredump.intra.peff.net","threadId":"55065","inReplyTo":"YBM/NpLeS0uFzVb3@nand.local","subject":"Re: [PATCH 5/6] hash_pos(): convert to oid_pos()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-29T01:18:55Z","receivedAt":"2021-01-29T01:19:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 28, 2021 at 05:48:22PM -0500, Taylor Blau wrote:\n\n> >  builtin/name-rev.c  |  8 ++++----\n> >  commit-graph.c      | 28 ++++++++++++++--------------\n> >  commit.c            | 10 +++++-----\n> >  hash-lookup.c       | 18 +++++++++---------\n> >  hash-lookup.h       | 10 +++++-----\n> \n> I wondered briefly if we should rename this to oid-lookup.{c,h}, but I\n> think the answer is \"no\", since we still have bsearch_hash() in this\n> header.\n> \n> Probably a single header with \"hash\" in the name is better than two\n> headers each containing a single function (and one containing an\n> additional typedef).\n> \n> So, I think that what you did here is good.\n\nYeah. I'd be more inclined to do so if renames weren't still a minor\npain when following history, etc. Getting rid of \"sha1\" crosses my bar\nof worth doing. The distinction between \"hash\" and \"oid\" here doesn't.\n(plus as you note, we'd really have to split it, which is even more\nannoying).\n\n-Peff\n"}]}