{"thread":{"id":"52877","subject":"[PATCH 0/10] removing nth_packed_object_sha1()","startedAt":"2020-02-24T04:26:27Z","lastAt":"2020-02-25T00:33:09Z","messageCount":14,"participants":["Jeff King","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"392376","messageId":"20200224042625.GA1015553@coredump.intra.peff.net","threadId":"52877","inReplyTo":null,"subject":"[PATCH 0/10] removing nth_packed_object_sha1()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-24T04:26:25Z","receivedAt":"2020-02-24T04:26:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series gets rid of nth_packed_object_sha1(), converting its\nremaining callers to nth_packed_object_oid(). The latter provides better\ntype-safety in general, and it also allowed me to clean up some tricky\nbits of the pack-verification code (patch 7 below).\n\nThis was inspired by brian's work in:\n\n https://lore.kernel.org/git/20200222201749.937983-2-sandals@crustytoothpaste.net/\n\nbut I decide not to base it on that series. It's semantically\nindependent, and with the exception of one line, textually independent\n(patch 4 here touches a different parameter in the same call as the\npatch linked above). So I think it's easier to handle them independently\nand just resolve the trivial conflict.\n\n  [01/10]: nth_packed_object_oid(): use customary integer return\n  [02/10]: pack-objects: read delta base oid into object_id struct\n  [03/10]: pack-objects: convert oe_set_delta_ext() to use object_id\n  [04/10]: pack-objects: use object_id struct in pack-reuse code\n  [05/10]: pack-bitmap: use object_id when loading on-disk bitmaps\n  [06/10]: pack-check: convert \"internal error\" die to a BUG()\n  [07/10]: pack-check: push oid lookup into loop\n  [08/10]: packed_object_info(): use object_id for returning delta base\n  [09/10]: packed_object_info(): use object_id internally for delta base\n  [10/10]: packfile: drop nth_packed_object_sha1()\n\n builtin/cat-file.c     |  2 +-\n builtin/pack-objects.c | 48 +++++++++++++++--------------\n midx.c                 |  2 +-\n object-store.h         |  2 +-\n pack-bitmap.c          | 16 +++++-----\n pack-check.c           | 22 ++++++--------\n pack-objects.c         |  4 +--\n pack-objects.h         |  2 +-\n packfile.c             | 69 ++++++++++++++++++------------------------\n packfile.h             | 15 +++------\n ref-filter.c           |  4 +--\n sha1-file.c            |  8 ++---\n sha1-name.c            | 13 ++++----\n 13 files changed, 94 insertions(+), 113 deletions(-)\n\n-Peff\n"},{"id":"392377","messageId":"20200224042736.GA1018190@coredump.intra.peff.net","threadId":"52877","inReplyTo":"20200224042625.GA1015553@coredump.intra.peff.net","subject":"[PATCH 01/10] nth_packed_object_oid(): use customary integer return","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-24T04:27:36Z","receivedAt":"2020-02-24T04:27:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our nth_packed_object_sha1() function returns NULL for error. So when we\nwrapped it with nth_packed_object_oid(), we kept the same semantics. But\nit's a bit funny, because the caller actually passes in an out\nparameter, and the pointer we return is just that same struct they\npassed to us (or NULL).\n\nIt's not too terrible, but it does make the interface a little\nnon-idiomatic. Let's switch to our usual \"0 for success, negative for\nerror\" return value. Most callers either don't check it, or are\ntrivially converted. The one that requires the biggest change is\nactually improved, as we can ditch an extra aliased pointer variable.\n\nSince we are changing the interface in a subtle way that the compiler\nwouldn't catch, let's also change the name to catch any topics in\nflight. We can drop the 'o' and make it nth_packed_object_id(). That's\nslightly shorter, but also less redundant since the 'o' stands for\n\"object\" already.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/pack-objects.c |  4 ++--\n midx.c                 |  2 +-\n pack-bitmap.c          |  4 ++--\n packfile.c             | 18 +++++++++---------\n packfile.h             |  5 ++---\n sha1-name.c            | 13 ++++++-------\n 6 files changed, 22 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 940fbcb7b3..de8335e2bd 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3053,7 +3053,7 @@ static void add_objects_in_unpacked_packs(void)\n \t\t\t   in_pack.alloc);\n \n \t\tfor (i = 0; i < p->num_objects; i++) {\n-\t\t\tnth_packed_object_oid(&oid, p, i);\n+\t\t\tnth_packed_object_id(&oid, p, i);\n \t\t\to = lookup_unknown_object(&oid);\n \t\t\tif (!(o->flags & OBJECT_ADDED))\n \t\t\t\tmark_in_pack_object(o, p, &in_pack);\n@@ -3157,7 +3157,7 @@ static void loosen_unused_packed_objects(void)\n \t\t\tdie(_(\"cannot open pack index\"));\n \n \t\tfor (i = 0; i < p->num_objects; i++) {\n-\t\t\tnth_packed_object_oid(&oid, p, i);\n+\t\t\tnth_packed_object_id(&oid, p, i);\n \t\t\tif (!packlist_find(&to_pack, &oid) &&\n \t\t\t    !has_sha1_pack_kept_or_nonlocal(&oid) &&\n \t\t\t    !loosened_object_can_be_discarded(&oid, p->mtime))\ndiff --git a/midx.c b/midx.c\nindex 37ec28623a..1527e464a7 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -534,7 +534,7 @@ static void fill_pack_entry(uint32_t pack_int_id,\n \t\t\t    uint32_t cur_object,\n \t\t\t    struct pack_midx_entry *entry)\n {\n-\tif (!nth_packed_object_oid(&entry->oid, p, cur_object))\n+\tif (nth_packed_object_id(&entry->oid, p, cur_object) < 0)\n \t\tdie(_(\"failed to locate object %d in packfile\"), cur_object);\n \n \tentry->pack_int_id = pack_int_id;\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 5a8689cdf8..c81d323329 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -658,7 +658,7 @@ static void show_objects_for_type(\n \t\t\toffset += ewah_bit_ctz64(word >> offset);\n \n \t\t\tentry = &bitmap_git->pack->revindex[pos + offset];\n-\t\t\tnth_packed_object_oid(&oid, bitmap_git->pack, entry->nr);\n+\t\t\tnth_packed_object_id(&oid, bitmap_git->pack, entry->nr);\n \n \t\t\tif (bitmap_git->hashes)\n \t\t\t\thash = get_be32(bitmap_git->hashes + entry->nr);\n@@ -1136,7 +1136,7 @@ int rebuild_existing_bitmaps(struct bitmap_index *bitmap_git,\n \t\tstruct object_entry *oe;\n \n \t\tentry = &bitmap_git->pack->revindex[i];\n-\t\tnth_packed_object_oid(&oid, bitmap_git->pack, entry->nr);\n+\t\tnth_packed_object_id(&oid, bitmap_git->pack, entry->nr);\n \t\toe = packlist_find(mapping, &oid);\n \n \t\tif (oe)\ndiff --git a/packfile.c b/packfile.c\nindex 99dd1a7d09..947c3f8e5d 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1261,7 +1261,7 @@ static int retry_bad_packed_offset(struct repository *r,\n \trevidx = find_pack_revindex(p, obj_offset);\n \tif (!revidx)\n \t\treturn OBJ_BAD;\n-\tnth_packed_object_oid(&oid, p, revidx->nr);\n+\tnth_packed_object_id(&oid, p, revidx->nr);\n \tmark_bad_packed_object(p, oid.hash);\n \ttype = oid_object_info(r, &oid, NULL);\n \tif (type <= OBJ_NONE)\n@@ -1693,7 +1693,7 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,\n \t\t\toff_t len = revidx[1].offset - obj_offset;\n \t\t\tif (check_pack_crc(p, &w_curs, obj_offset, len, revidx->nr)) {\n \t\t\t\tstruct object_id oid;\n-\t\t\t\tnth_packed_object_oid(&oid, p, revidx->nr);\n+\t\t\t\tnth_packed_object_id(&oid, p, revidx->nr);\n \t\t\t\terror(\"bad packed object CRC for %s\",\n \t\t\t\t      oid_to_hex(&oid));\n \t\t\t\tmark_bad_packed_object(p, oid.hash);\n@@ -1782,7 +1782,7 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,\n \t\t\tstruct object_id base_oid;\n \t\t\trevidx = find_pack_revindex(p, obj_offset);\n \t\t\tif (revidx) {\n-\t\t\t\tnth_packed_object_oid(&base_oid, p, revidx->nr);\n+\t\t\t\tnth_packed_object_id(&base_oid, p, revidx->nr);\n \t\t\t\terror(\"failed to read delta base object %s\"\n \t\t\t\t      \" at offset %\"PRIuMAX\" from %s\",\n \t\t\t\t      oid_to_hex(&base_oid), (uintmax_t)obj_offset,\n@@ -1890,15 +1890,15 @@ const unsigned char *nth_packed_object_sha1(struct packed_git *p,\n \t}\n }\n \n-const struct object_id *nth_packed_object_oid(struct object_id *oid,\n-\t\t\t\t\t      struct packed_git *p,\n-\t\t\t\t\t      uint32_t n)\n+int nth_packed_object_id(struct object_id *oid,\n+\t\t\t struct packed_git *p,\n+\t\t\t uint32_t n)\n {\n \tconst unsigned char *hash = nth_packed_object_sha1(p, n);\n \tif (!hash)\n-\t\treturn NULL;\n+\t\treturn -1;\n \thashcpy(oid->hash, hash);\n-\treturn oid;\n+\treturn 0;\n }\n \n void check_pack_index_ptr(const struct packed_git *p, const void *vptr)\n@@ -2077,7 +2077,7 @@ int for_each_object_in_pack(struct packed_git *p,\n \t\telse\n \t\t\tpos = i;\n \n-\t\tif (!nth_packed_object_oid(&oid, p, pos))\n+\t\tif (nth_packed_object_id(&oid, p, pos) < 0)\n \t\t\treturn error(\"unable to get sha1 of object %u in %s\",\n \t\t\t\t     pos, p->pack_name);\n \ndiff --git a/packfile.h b/packfile.h\nindex ec536a4ae5..95b83ba25b 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -129,10 +129,9 @@ int bsearch_pack(const struct object_id *oid, const struct packed_git *p, uint32\n const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t n);\n /*\n  * Like nth_packed_object_sha1, but write the data into the object specified by\n- * the the first argument.  Returns the first argument on success, and NULL on\n- * error.\n+ * the the first argument.  Returns 0 on success, negative otherwise.\n  */\n-const struct object_id *nth_packed_object_oid(struct object_id *, struct packed_git *, uint32_t n);\n+int nth_packed_object_id(struct object_id *, struct packed_git *, uint32_t n);\n \n /*\n  * Return the offset of the nth object within the specified packfile.\ndiff --git a/sha1-name.c b/sha1-name.c\nindex f2e24ea666..5bb006e5a9 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -155,7 +155,6 @@ static void unique_in_pack(struct packed_git *p,\n \t\t\t   struct disambiguate_state *ds)\n {\n \tuint32_t num, i, first = 0;\n-\tconst struct object_id *current = NULL;\n \n \tif (p->multi_pack_index)\n \t\treturn;\n@@ -173,10 +172,10 @@ static void unique_in_pack(struct packed_git *p,\n \t */\n \tfor (i = first; i < num && !ds->ambiguous; i++) {\n \t\tstruct object_id oid;\n-\t\tcurrent = nth_packed_object_oid(&oid, p, i);\n-\t\tif (!match_sha(ds->len, ds->bin_pfx.hash, current->hash))\n+\t\tnth_packed_object_id(&oid, p, i);\n+\t\tif (!match_sha(ds->len, ds->bin_pfx.hash, oid.hash))\n \t\t\tbreak;\n-\t\tupdate_candidates(ds, current);\n+\t\tupdate_candidates(ds, &oid);\n \t}\n }\n \n@@ -643,14 +642,14 @@ static void find_abbrev_len_for_pack(struct packed_git *p,\n \t */\n \tmad->init_len = 0;\n \tif (!match) {\n-\t\tif (nth_packed_object_oid(&oid, p, first))\n+\t\tif (!nth_packed_object_id(&oid, p, first))\n \t\t\textend_abbrev_len(&oid, mad);\n \t} else if (first < num - 1) {\n-\t\tif (nth_packed_object_oid(&oid, p, first + 1))\n+\t\tif (!nth_packed_object_id(&oid, p, first + 1))\n \t\t\textend_abbrev_len(&oid, mad);\n \t}\n \tif (first > 0) {\n-\t\tif (nth_packed_object_oid(&oid, p, first - 1))\n+\t\tif (!nth_packed_object_id(&oid, p, first - 1))\n \t\t\textend_abbrev_len(&oid, mad);\n \t}\n \tmad->init_len = mad->cur_len;\n-- \n2.25.1.823.g95c5488cf7\n\n"},{"id":"392378","messageId":"20200224042944.GB1018190@coredump.intra.peff.net","threadId":"52877","inReplyTo":"20200224042625.GA1015553@coredump.intra.peff.net","subject":"[PATCH 02/10] pack-objects: read delta base oid into object_id struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-24T04:29:44Z","receivedAt":"2020-02-24T04:29:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we're considering reusing an on-disk delta, we get the oid of the\nbase as a pointer to unsigned char bytes of the hash, either into the\npackfile itself (for REF_DELTA) or into the pack idx (using the revindex\nto convert the offset into an index entry).\n\nInstead, we'd prefer to use a more type-safe object_id as much as\npossible. We can get the pack idx using nth_packed_object_id() instead.\nFor the packfile bytes, we can copy them out using oidread().\n\nThis doesn't even incur an extra copy overall, since the next thing we'd\nalways do with that pointer is pass it to can_reuse_delta(), which needs\nan object_id anyway (and called oidread() itself). So this patch also\nconverts that function to take the object_id directly.\n\nNote that we did previously use NULL as a sentinel value when the object\nisn't a delta. We could probably get away with using the null oid for\nthis, but instead we'll use an explicit boolean flag, which should make\nthings more obvious for people reading the code later.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAstute readers may notice that if we didn't do the error-conversion in\nthe previous patch, we could keep the sentinel value semantics with\nsomething like:\n\n  struct object_id *base_ref = NULL;\n  struct object_id base_ref_storage;\n  ...\n  base_ref = nth_packed_object_oid(&base_ref_storage, p, nr);\n\nbut that's not any fewer lines, and IMHO it's much less obvious what's\ngoing on compared to the boolean flag I used here.\n\n builtin/pack-objects.c | 35 ++++++++++++++++++-----------------\n 1 file changed, 18 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex de8335e2bd..8692ab3fe6 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1618,23 +1618,17 @@ static void cleanup_preferred_base(void)\n  * deltify other objects against, in order to avoid\n  * circular deltas.\n  */\n-static int can_reuse_delta(const unsigned char *base_sha1,\n+static int can_reuse_delta(const struct object_id *base_oid,\n \t\t\t   struct object_entry *delta,\n \t\t\t   struct object_entry **base_out)\n {\n \tstruct object_entry *base;\n-\tstruct object_id base_oid;\n-\n-\tif (!base_sha1)\n-\t\treturn 0;\n-\n-\toidread(&base_oid, base_sha1);\n \n \t/*\n \t * First see if we're already sending the base (or it's explicitly in\n \t * our \"excluded\" list).\n \t */\n-\tbase = packlist_find(&to_pack, &base_oid);\n+\tbase = packlist_find(&to_pack, base_oid);\n \tif (base) {\n \t\tif (!in_same_island(&delta->idx.oid, &base->idx.oid))\n \t\t\treturn 0;\n@@ -1647,9 +1641,9 @@ static int can_reuse_delta(const unsigned char *base_sha1,\n \t * even if it was buried too deep in history to make it into the\n \t * packing list.\n \t */\n-\tif (thin && bitmap_has_oid_in_uninteresting(bitmap_git, &base_oid)) {\n+\tif (thin && bitmap_has_oid_in_uninteresting(bitmap_git, base_oid)) {\n \t\tif (use_delta_islands) {\n-\t\t\tif (!in_same_island(&delta->idx.oid, &base_oid))\n+\t\t\tif (!in_same_island(&delta->idx.oid, base_oid))\n \t\t\t\treturn 0;\n \t\t}\n \t\t*base_out = NULL;\n@@ -1666,7 +1660,8 @@ static void check_object(struct object_entry *entry)\n \tif (IN_PACK(entry)) {\n \t\tstruct packed_git *p = IN_PACK(entry);\n \t\tstruct pack_window *w_curs = NULL;\n-\t\tconst unsigned char *base_ref = NULL;\n+\t\tint have_base = 0;\n+\t\tstruct object_id base_ref;\n \t\tstruct object_entry *base_entry;\n \t\tunsigned long used, used_0;\n \t\tunsigned long avail;\n@@ -1707,9 +1702,13 @@ static void check_object(struct object_entry *entry)\n \t\t\tunuse_pack(&w_curs);\n \t\t\treturn;\n \t\tcase OBJ_REF_DELTA:\n-\t\t\tif (reuse_delta && !entry->preferred_base)\n-\t\t\t\tbase_ref = use_pack(p, &w_curs,\n-\t\t\t\t\t\tentry->in_pack_offset + used, NULL);\n+\t\t\tif (reuse_delta && !entry->preferred_base) {\n+\t\t\t\toidread(&base_ref,\n+\t\t\t\t\tuse_pack(p, &w_curs,\n+\t\t\t\t\t\t entry->in_pack_offset + used,\n+\t\t\t\t\t\t NULL));\n+\t\t\t\thave_base = 1;\n+\t\t\t}\n \t\t\tentry->in_pack_header_size = used + the_hash_algo->rawsz;\n \t\t\tbreak;\n \t\tcase OBJ_OFS_DELTA:\n@@ -1739,13 +1738,15 @@ static void check_object(struct object_entry *entry)\n \t\t\t\trevidx = find_pack_revindex(p, ofs);\n \t\t\t\tif (!revidx)\n \t\t\t\t\tgoto give_up;\n-\t\t\t\tbase_ref = nth_packed_object_sha1(p, revidx->nr);\n+\t\t\t\tif (!nth_packed_object_id(&base_ref, p, revidx->nr))\n+\t\t\t\t\thave_base = 1;\n \t\t\t}\n \t\t\tentry->in_pack_header_size = used + used_0;\n \t\t\tbreak;\n \t\t}\n \n-\t\tif (can_reuse_delta(base_ref, entry, &base_entry)) {\n+\t\tif (have_base &&\n+\t\t    can_reuse_delta(&base_ref, entry, &base_entry)) {\n \t\t\toe_set_type(entry, entry->in_pack_type);\n \t\t\tSET_SIZE(entry, in_pack_size); /* delta size */\n \t\t\tSET_DELTA_SIZE(entry, in_pack_size);\n@@ -1755,7 +1756,7 @@ static void check_object(struct object_entry *entry)\n \t\t\t\tentry->delta_sibling_idx = base_entry->delta_child_idx;\n \t\t\t\tSET_DELTA_CHILD(base_entry, entry);\n \t\t\t} else {\n-\t\t\t\tSET_DELTA_EXT(entry, base_ref);\n+\t\t\t\tSET_DELTA_EXT(entry, base_ref.hash);\n \t\t\t}\n \n \t\t\tunuse_pack(&w_curs);\n-- \n2.25.1.823.g95c5488cf7\n\n"},{"id":"392379","messageId":"20200224043007.GC1018190@coredump.intra.peff.net","threadId":"52877","inReplyTo":"20200224042625.GA1015553@coredump.intra.peff.net","subject":"[PATCH 03/10] pack-objects: convert oe_set_delta_ext() to use object_id","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-24T04:30:07Z","receivedAt":"2020-02-24T04:30:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We already store an object_id internally, and now our sole caller also\nhas one. Let's stop passing around the internal hash array, which adds a\nbit of type safety.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/pack-objects.c | 2 +-\n pack-objects.c         | 4 ++--\n pack-objects.h         | 2 +-\n 3 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 8692ab3fe6..44f44fcb1a 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1756,7 +1756,7 @@ static void check_object(struct object_entry *entry)\n \t\t\t\tentry->delta_sibling_idx = base_entry->delta_child_idx;\n \t\t\t\tSET_DELTA_CHILD(base_entry, entry);\n \t\t\t} else {\n-\t\t\t\tSET_DELTA_EXT(entry, base_ref.hash);\n+\t\t\t\tSET_DELTA_EXT(entry, &base_ref);\n \t\t\t}\n \n \t\t\tunuse_pack(&w_curs);\ndiff --git a/pack-objects.c b/pack-objects.c\nindex 5e5a3c62d9..f2a433885a 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -203,14 +203,14 @@ struct object_entry *packlist_alloc(struct packing_data *pdata,\n \n void oe_set_delta_ext(struct packing_data *pdata,\n \t\t      struct object_entry *delta,\n-\t\t      const unsigned char *sha1)\n+\t\t      const struct object_id *oid)\n {\n \tstruct object_entry *base;\n \n \tALLOC_GROW(pdata->ext_bases, pdata->nr_ext + 1, pdata->alloc_ext);\n \tbase = &pdata->ext_bases[pdata->nr_ext++];\n \tmemset(base, 0, sizeof(*base));\n-\thashcpy(base->idx.oid.hash, sha1);\n+\toidcpy(&base->idx.oid, oid);\n \n \t/* These flags mark that we are not part of the actual pack output. */\n \tbase->preferred_base = 1;\ndiff --git a/pack-objects.h b/pack-objects.h\nindex d3975e079b..9d88e3e518 100644\n--- a/pack-objects.h\n+++ b/pack-objects.h\n@@ -292,7 +292,7 @@ static inline void oe_set_delta(struct packing_data *pack,\n \n void oe_set_delta_ext(struct packing_data *pack,\n \t\t      struct object_entry *e,\n-\t\t      const unsigned char *sha1);\n+\t\t      const struct object_id *oid);\n \n static inline struct object_entry *oe_delta_child(\n \t\tconst struct packing_data *pack,\n-- \n2.25.1.823.g95c5488cf7\n\n"},{"id":"392380","messageId":"20200224043122.GD1018190@coredump.intra.peff.net","threadId":"52877","inReplyTo":"20200224042625.GA1015553@coredump.intra.peff.net","subject":"[PATCH 04/10] pack-objects: use object_id struct in pack-reuse code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-24T04:31:22Z","receivedAt":"2020-02-24T04:31:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the pack-reuse code is dumping an OFS_DELTA entry to a client that\ndoesn't support it, we re-write it as a REF_DELTA. To do so, we use\nnth_packed_object_sha1() to get the oid, but that function is soon going\naway in favor of the more type-safe nth_packed_object_id(). Let's switch\nnow in preparation.\n\nNote that this does incur an extra hash copy (from the pack idx mmap to\nthe object_id and then to the output, rather than straight from mmap to\nthe output). But this is not worth worrying about. It's probably not\nmeasurable even when it triggers, and this is fallback code that we\nexpect to trigger very rarely (since everybody supports OFS_DELTA these\ndays anyway).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIf you haven't read brian's series, yes, that ugly bare 20 should be\nthe_hash_algo->rawsz.\n\n builtin/pack-objects.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 44f44fcb1a..73fca2cb17 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -872,14 +872,15 @@ static void write_reused_pack_one(size_t pos, struct hashfile *out,\n \t\t/* Convert to REF_DELTA if we must... */\n \t\tif (!allow_ofs_delta) {\n \t\t\tint base_pos = find_revindex_position(reuse_packfile, base_offset);\n-\t\t\tconst unsigned char *base_sha1 =\n-\t\t\t\tnth_packed_object_sha1(reuse_packfile,\n-\t\t\t\t\t\t       reuse_packfile->revindex[base_pos].nr);\n+\t\t\tstruct object_id base_oid;\n+\n+\t\t\tnth_packed_object_id(&base_oid, reuse_packfile,\n+\t\t\t\t\t     reuse_packfile->revindex[base_pos].nr);\n \n \t\t\tlen = encode_in_pack_object_header(header, sizeof(header),\n \t\t\t\t\t\t\t   OBJ_REF_DELTA, size);\n \t\t\thashwrite(out, header, len);\n-\t\t\thashwrite(out, base_sha1, 20);\n+\t\t\thashwrite(out, base_oid.hash, 20);\n \t\t\tcopy_pack_data(out, reuse_packfile, w_curs, cur, next - cur);\n \t\t\treturn;\n \t\t}\n-- \n2.25.1.823.g95c5488cf7\n\n"},{"id":"392381","messageId":"20200224043227.GE1018190@coredump.intra.peff.net","threadId":"52877","inReplyTo":"20200224042625.GA1015553@coredump.intra.peff.net","subject":"[PATCH 05/10] pack-bitmap: use object_id when loading on-disk bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-24T04:32:27Z","receivedAt":"2020-02-24T04:32:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A pack bitmap file contains the index position of the commit for each\nbitmap, which we then translate into an object id via\nnth_packed_object_sha1(). In preparation for that function going away,\nwe can switch to the more type-safe nth_packed_object_id().\n\nNote that even though the result ends up in an object_id this does incur\nan extra copy of the hash (into our temporary object_id, and then into\nthe final malloc'd stored_bitmap struct). This shouldn't make any\nmeasurable difference. If it did, we could avoid this copy _and_ the\ncopy of the rest of the items by allocating the stored_bitmap struct\nbeforehand and reading directly into it from the bitmap file. Or better\nstill, if this is a bottleneck, we could introduce an on-disk index to\nthe bitmap file so we don't have to read every single entry to use just\none of them. So it's not worth worrying about micro-optimizing out this\none hash copy.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pack-bitmap.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex c81d323329..1a067885a1 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -169,7 +169,7 @@ static int load_bitmap_header(struct bitmap_index *index)\n \n static struct stored_bitmap *store_bitmap(struct bitmap_index *index,\n \t\t\t\t\t  struct ewah_bitmap *root,\n-\t\t\t\t\t  const unsigned char *hash,\n+\t\t\t\t\t  const struct object_id *oid,\n \t\t\t\t\t  struct stored_bitmap *xor_with,\n \t\t\t\t\t  int flags)\n {\n@@ -181,15 +181,15 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,\n \tstored->root = root;\n \tstored->xor = xor_with;\n \tstored->flags = flags;\n-\toidread(&stored->oid, hash);\n+\toidcpy(&stored->oid, oid);\n \n \thash_pos = kh_put_oid_map(index->bitmaps, stored->oid, &ret);\n \n \t/* a 0 return code means the insertion succeeded with no changes,\n \t * because the SHA1 already existed on the map. this is bad, there\n \t * shouldn't be duplicated commits in the index */\n \tif (ret == 0) {\n-\t\terror(\"Duplicate entry in bitmap index: %s\", hash_to_hex(hash));\n+\t\terror(\"Duplicate entry in bitmap index: %s\", oid_to_hex(oid));\n \t\treturn NULL;\n \t}\n \n@@ -221,13 +221,13 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \t\tstruct ewah_bitmap *bitmap = NULL;\n \t\tstruct stored_bitmap *xor_bitmap = NULL;\n \t\tuint32_t commit_idx_pos;\n-\t\tconst unsigned char *sha1;\n+\t\tstruct object_id oid;\n \n \t\tcommit_idx_pos = read_be32(index->map, &index->map_pos);\n \t\txor_offset = read_u8(index->map, &index->map_pos);\n \t\tflags = read_u8(index->map, &index->map_pos);\n \n-\t\tsha1 = nth_packed_object_sha1(index->pack, commit_idx_pos);\n+\t\tnth_packed_object_id(&oid, index->pack, commit_idx_pos);\n \n \t\tbitmap = read_bitmap_1(index);\n \t\tif (!bitmap)\n@@ -244,7 +244,7 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \t\t}\n \n \t\trecent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap(\n-\t\t\tindex, bitmap, sha1, xor_bitmap, flags);\n+\t\t\tindex, bitmap, &oid, xor_bitmap, flags);\n \t}\n \n \treturn 0;\n-- \n2.25.1.823.g95c5488cf7\n\n"},{"id":"392382","messageId":"20200224043318.GF1018190@coredump.intra.peff.net","threadId":"52877","inReplyTo":"20200224042625.GA1015553@coredump.intra.peff.net","subject":"[PATCH 06/10] pack-check: convert \"internal error\" die to a BUG()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-24T04:33:18Z","receivedAt":"2020-02-24T04:33:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we fail to load the oid from the index of a packfile, we'll die()\nwith an \"internal error\". But this should never happen: we'd fail here\nonly if the idx needed to be lazily opened (but we've already opened it)\nor if we asked for an out-of-range index (but we're iterating using the\nsame count that we'd check the range against). A corrupted index might\nhave a bogus count (e.g., too large for its size), but we'd have\ncomplained and aborted already when opening the index initially.\n\nWhile we're here, we can add a few details so that if this bug ever\n_does_ trigger, we'll have a bit more information.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nMost code in this situation doesn't even bother checking the return\nvalue. So I would also be tempted to simply drop the conditional\nentirely as unreachable.\n\n pack-check.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/pack-check.c b/pack-check.c\nindex e4ef71c673..39196ecfbc 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -99,7 +99,8 @@ static int verify_packfile(struct repository *r,\n \tfor (i = 0; i < nr_objects; i++) {\n \t\tentries[i].oid.hash = nth_packed_object_sha1(p, i);\n \t\tif (!entries[i].oid.hash)\n-\t\t\tdie(\"internal error pack-check nth-packed-object\");\n+\t\t\tBUG(\"unable to get oid of object %lu from %s\",\n+\t\t\t    (unsigned long)i, p->pack_name);\n \t\tentries[i].offset = nth_packed_object_offset(p, i);\n \t\tentries[i].nr = i;\n \t}\n-- \n2.25.1.823.g95c5488cf7\n\n"},{"id":"392383","messageId":"20200224043631.GG1018190@coredump.intra.peff.net","threadId":"52877","inReplyTo":"20200224042625.GA1015553@coredump.intra.peff.net","subject":"[PATCH 07/10] pack-check: push oid lookup into loop","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-24T04:36:31Z","receivedAt":"2020-02-24T04:36:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we're checking a pack with fsck or verify-pack, we first sort the\nidx entries by offset, since accessing them in pack order is more\nefficient. To do so, we loop over them and fill in an array of structs\nwith the offset, object_id, and index position of each, sort the result,\nand only then do we iterate over the sorted array and process each\nentry.\n\nIn order to avoid the memory cost of storing the hash of each object, we\njust store a pointer into the copy in the mmap'd pack index file. To\nkeep that property even as the rest of the code converted to \"struct\nobject_id\", commit 9fd750461b (Convert the verify_pack callback to\nstruct object_id, 2017-05-06) introduced a union in order to type-pun\nthe pointer-to-hash into an object_id struct.\n\nBut we can make this even simpler by observing that the sort operation\ndoesn't need the object id at all! We only need them one at a time while\nwe actually process each entry. So we can just omit the oid from the\nstruct entirely and load it on the fly into a local variable in the\nsecond loop.\n\nThis gets rid of the type-punning, and lets us directly use the more\ntype-safe nth_packed_object_id(), simplifying the code. And as a bonus,\nit saves 8 bytes of memory per object.\n\nNote that this does mean we'll do the offset lookup for each object\nbefore the oid lookup. The oid lookup has more safety checks in it\n(e.g., for looking past p->num_objects) which in theory protected the\noffset lookup. But since violating those checks was already a BUG()\ncondition (as described in the previous commit), it's not worth worrying\nabout.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pack-check.c | 23 ++++++++++-------------\n 1 file changed, 10 insertions(+), 13 deletions(-)\n\ndiff --git a/pack-check.c b/pack-check.c\nindex 39196ecfbc..dad6d8ae7f 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -8,10 +8,6 @@\n \n struct idx_entry {\n \toff_t                offset;\n-\tunion idx_entry_object {\n-\t\tconst unsigned char *hash;\n-\t\tstruct object_id *oid;\n-\t} oid;\n \tunsigned int nr;\n };\n \n@@ -97,30 +93,31 @@ static int verify_packfile(struct repository *r,\n \tentries[nr_objects].offset = pack_sig_ofs;\n \t/* first sort entries by pack offset, since unpacking them is more efficient that way */\n \tfor (i = 0; i < nr_objects; i++) {\n-\t\tentries[i].oid.hash = nth_packed_object_sha1(p, i);\n-\t\tif (!entries[i].oid.hash)\n-\t\t\tBUG(\"unable to get oid of object %lu from %s\",\n-\t\t\t    (unsigned long)i, p->pack_name);\n \t\tentries[i].offset = nth_packed_object_offset(p, i);\n \t\tentries[i].nr = i;\n \t}\n \tQSORT(entries, nr_objects, compare_entries);\n \n \tfor (i = 0; i < nr_objects; i++) {\n \t\tvoid *data;\n+\t\tstruct object_id oid;\n \t\tenum object_type type;\n \t\tunsigned long size;\n \t\toff_t curpos;\n \t\tint data_valid;\n \n+\t\tif (nth_packed_object_id(&oid, p, entries[i].nr) < 0)\n+\t\t\tBUG(\"unable to get oid of object %lu from %s\",\n+\t\t\t    (unsigned long)entries[i].nr, p->pack_name);\n+\n \t\tif (p->index_version > 1) {\n \t\t\toff_t offset = entries[i].offset;\n \t\t\toff_t len = entries[i+1].offset - offset;\n \t\t\tunsigned int nr = entries[i].nr;\n \t\t\tif (check_pack_crc(p, w_curs, offset, len, nr))\n \t\t\t\terr = error(\"index CRC mismatch for object %s \"\n \t\t\t\t\t    \"from %s at offset %\"PRIuMAX\"\",\n-\t\t\t\t\t    oid_to_hex(entries[i].oid.oid),\n+\t\t\t\t\t    oid_to_hex(&oid),\n \t\t\t\t\t    p->pack_name, (uintmax_t)offset);\n \t\t}\n \n@@ -143,14 +140,14 @@ static int verify_packfile(struct repository *r,\n \n \t\tif (data_valid && !data)\n \t\t\terr = error(\"cannot unpack %s from %s at offset %\"PRIuMAX\"\",\n-\t\t\t\t    oid_to_hex(entries[i].oid.oid), p->pack_name,\n+\t\t\t\t    oid_to_hex(&oid), p->pack_name,\n \t\t\t\t    (uintmax_t)entries[i].offset);\n-\t\telse if (check_object_signature(r, entries[i].oid.oid, data, size, type_name(type)))\n+\t\telse if (check_object_signature(r, &oid, data, size, type_name(type)))\n \t\t\terr = error(\"packed %s from %s is corrupt\",\n-\t\t\t\t    oid_to_hex(entries[i].oid.oid), p->pack_name);\n+\t\t\t\t    oid_to_hex(&oid), p->pack_name);\n \t\telse if (fn) {\n \t\t\tint eaten = 0;\n-\t\t\terr |= fn(entries[i].oid.oid, type, size, data, &eaten);\n+\t\t\terr |= fn(&oid, type, size, data, &eaten);\n \t\t\tif (eaten)\n \t\t\t\tdata = NULL;\n \t\t}\n-- \n2.25.1.823.g95c5488cf7\n\n"},{"id":"392384","messageId":"20200224043656.GH1018190@coredump.intra.peff.net","threadId":"52877","inReplyTo":"20200224042625.GA1015553@coredump.intra.peff.net","subject":"[PATCH 08/10] packed_object_info(): use object_id for returning delta base","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-24T04:36:56Z","receivedAt":"2020-02-24T04:36:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If a caller sets the object_info.delta_base_sha1 to a non-NULL pointer,\nwe'll write the oid of the object's delta base to it. But we can\nincrease our type safety by switching this to a real object_id struct.\nAll of our callers are just pointing into the hash member of an\nobject_id anyway, so there's no inconvenience.\n\nNote that we do still keep it as a pointer-to-struct, because the NULL\nsentinel value tells us whether the caller is even interested in the\ninformation.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/cat-file.c | 2 +-\n object-store.h     | 2 +-\n packfile.c         | 6 +++---\n ref-filter.c       | 4 ++--\n sha1-file.c        | 8 ++++----\n 5 files changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex d6a1aa74cd..272f9fc6d7 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -262,7 +262,7 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,\n \t\t\tstrbuf_addstr(sb, data->rest);\n \t} else if (is_atom(\"deltabase\", atom, len)) {\n \t\tif (data->mark_query)\n-\t\t\tdata->info.delta_base_sha1 = data->delta_base_oid.hash;\n+\t\t\tdata->info.delta_base_oid = &data->delta_base_oid;\n \t\telse\n \t\t\tstrbuf_addstr(sb,\n \t\t\t\t      oid_to_hex(&data->delta_base_oid));\ndiff --git a/object-store.h b/object-store.h\nindex 5b047637e3..be72fee7d5 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -300,7 +300,7 @@ struct object_info {\n \tenum object_type *typep;\n \tunsigned long *sizep;\n \toff_t *disk_sizep;\n-\tunsigned char *delta_base_sha1;\n+\tstruct object_id *delta_base_oid;\n \tstruct strbuf *type_name;\n \tvoid **contentp;\n \ndiff --git a/packfile.c b/packfile.c\nindex 947c3f8e5d..ec7349bb86 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1556,7 +1556,7 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t}\n \t}\n \n-\tif (oi->delta_base_sha1) {\n+\tif (oi->delta_base_oid) {\n \t\tif (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {\n \t\t\tconst unsigned char *base;\n \n@@ -1567,9 +1567,9 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \t\t\t\tgoto out;\n \t\t\t}\n \n-\t\t\thashcpy(oi->delta_base_sha1, base);\n+\t\t\thashcpy(oi->delta_base_oid->hash, base);\n \t\t} else\n-\t\t\thashclr(oi->delta_base_sha1);\n+\t\t\toidclr(oi->delta_base_oid);\n \t}\n \n \toi->whence = in_delta_base_cache(p, obj_offset) ? OI_DBCACHED :\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 6867e33648..79bb520678 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -279,9 +279,9 @@ static int deltabase_atom_parser(const struct ref_format *format, struct used_at\n \tif (arg)\n \t\treturn strbuf_addf_ret(err, -1, _(\"%%(deltabase) does not take arguments\"));\n \tif (*atom->name == '*')\n-\t\toi_deref.info.delta_base_sha1 = oi_deref.delta_base_oid.hash;\n+\t\toi_deref.info.delta_base_oid = &oi_deref.delta_base_oid;\n \telse\n-\t\toi.info.delta_base_sha1 = oi.delta_base_oid.hash;\n+\t\toi.info.delta_base_oid = &oi.delta_base_oid;\n \treturn 0;\n }\n \ndiff --git a/sha1-file.c b/sha1-file.c\nindex d785de8a85..616886799e 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1354,8 +1354,8 @@ static int loose_object_info(struct repository *r,\n \tstruct strbuf hdrbuf = STRBUF_INIT;\n \tunsigned long size_scratch;\n \n-\tif (oi->delta_base_sha1)\n-\t\thashclr(oi->delta_base_sha1);\n+\tif (oi->delta_base_oid)\n+\t\toidclr(oi->delta_base_oid);\n \n \t/*\n \t * If we don't care about type or size, then we don't\n@@ -1474,8 +1474,8 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\t\t*(oi->sizep) = co->size;\n \t\tif (oi->disk_sizep)\n \t\t\t*(oi->disk_sizep) = 0;\n-\t\tif (oi->delta_base_sha1)\n-\t\t\thashclr(oi->delta_base_sha1);\n+\t\tif (oi->delta_base_oid)\n+\t\t\toidclr(oi->delta_base_oid);\n \t\tif (oi->type_name)\n \t\t\tstrbuf_addstr(oi->type_name, type_name(co->type));\n \t\tif (oi->contentp)\n-- \n2.25.1.823.g95c5488cf7\n\n"},{"id":"392385","messageId":"20200224043731.GI1018190@coredump.intra.peff.net","threadId":"52877","inReplyTo":"20200224042625.GA1015553@coredump.intra.peff.net","subject":"[PATCH 09/10] packed_object_info(): use object_id internally for delta base","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-24T04:37:31Z","receivedAt":"2020-02-24T04:37:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The previous commit changed the public interface of packed_object_info()\nto return a struct object_id rather than a bare hash. That enables us to\nconvert our internal helper, as well. We can use nth_packed_object_id()\ndirectly for OFS_DELTA, but we'll still have to use oidread() to pull\nthe hash for a REF_DELTA out of the packfile.\n\nThere should be no additional cost, since we're copying directly into\nthe object_id the caller provided us (just as we did before; it's just\nhappening now via nth_packed_object_id()).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n packfile.c | 32 +++++++++++++++-----------------\n 1 file changed, 15 insertions(+), 17 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex ec7349bb86..90cb5a05ac 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1225,30 +1225,32 @@ off_t get_delta_base(struct packed_git *p,\n  * the final object lookup), but more expensive for OFS deltas (we\n  * have to load the revidx to convert the offset back into a sha1).\n  */\n-static const unsigned char *get_delta_base_sha1(struct packed_git *p,\n-\t\t\t\t\t\tstruct pack_window **w_curs,\n-\t\t\t\t\t\toff_t curpos,\n-\t\t\t\t\t\tenum object_type type,\n-\t\t\t\t\t\toff_t delta_obj_offset)\n+static int get_delta_base_oid(struct packed_git *p,\n+\t\t\t      struct pack_window **w_curs,\n+\t\t\t      off_t curpos,\n+\t\t\t      struct object_id *oid,\n+\t\t\t      enum object_type type,\n+\t\t\t      off_t delta_obj_offset)\n {\n \tif (type == OBJ_REF_DELTA) {\n \t\tunsigned char *base = use_pack(p, w_curs, curpos, NULL);\n-\t\treturn base;\n+\t\toidread(oid, base);\n+\t\treturn 0;\n \t} else if (type == OBJ_OFS_DELTA) {\n \t\tstruct revindex_entry *revidx;\n \t\toff_t base_offset = get_delta_base(p, w_curs, &curpos,\n \t\t\t\t\t\t   type, delta_obj_offset);\n \n \t\tif (!base_offset)\n-\t\t\treturn NULL;\n+\t\t\treturn -1;\n \n \t\trevidx = find_pack_revindex(p, base_offset);\n \t\tif (!revidx)\n-\t\t\treturn NULL;\n+\t\t\treturn -1;\n \n-\t\treturn nth_packed_object_sha1(p, revidx->nr);\n+\t\treturn nth_packed_object_id(oid, p, revidx->nr);\n \t} else\n-\t\treturn NULL;\n+\t\treturn -1;\n }\n \n static int retry_bad_packed_offset(struct repository *r,\n@@ -1558,16 +1560,12 @@ int packed_object_info(struct repository *r, struct packed_git *p,\n \n \tif (oi->delta_base_oid) {\n \t\tif (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {\n-\t\t\tconst unsigned char *base;\n-\n-\t\t\tbase = get_delta_base_sha1(p, &w_curs, curpos,\n-\t\t\t\t\t\t   type, obj_offset);\n-\t\t\tif (!base) {\n+\t\t\tif (get_delta_base_oid(p, &w_curs, curpos,\n+\t\t\t\t\t       oi->delta_base_oid,\n+\t\t\t\t\t       type, obj_offset) < 0) {\n \t\t\t\ttype = OBJ_BAD;\n \t\t\t\tgoto out;\n \t\t\t}\n-\n-\t\t\thashcpy(oi->delta_base_oid->hash, base);\n \t\t} else\n \t\t\toidclr(oi->delta_base_oid);\n \t}\n-- \n2.25.1.823.g95c5488cf7\n\n"},{"id":"392386","messageId":"20200224043754.GJ1018190@coredump.intra.peff.net","threadId":"52877","inReplyTo":"20200224042625.GA1015553@coredump.intra.peff.net","subject":"[PATCH 10/10] packfile: drop nth_packed_object_sha1()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-24T04:37:54Z","receivedAt":"2020-02-24T04:37:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Once upon a time, nth_packed_object_sha1() was the primary way to get\nthe oid of a packfile's index position. But these days we have the more\ntype-safe nth_packed_object_id() wrapper, and all callers have been\nconverted.\n\nLet's drop the \"sha1\" version (turning the safer wrapper into a single\nfunction) so that nobody is tempted to introduce new callers.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n packfile.c | 23 +++++++----------------\n packfile.h | 12 +++---------\n 2 files changed, 10 insertions(+), 25 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 90cb5a05ac..f4e752996d 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1867,35 +1867,26 @@ int bsearch_pack(const struct object_id *oid, const struct packed_git *p, uint32\n \t\t\t    index_lookup, index_lookup_width, result);\n }\n \n-const unsigned char *nth_packed_object_sha1(struct packed_git *p,\n-\t\t\t\t\t    uint32_t n)\n+int nth_packed_object_id(struct object_id *oid,\n+\t\t\t struct packed_git *p,\n+\t\t\t uint32_t n)\n {\n \tconst unsigned char *index = p->index_data;\n \tconst unsigned int hashsz = the_hash_algo->rawsz;\n \tif (!index) {\n \t\tif (open_pack_index(p))\n-\t\t\treturn NULL;\n+\t\t\treturn -1;\n \t\tindex = p->index_data;\n \t}\n \tif (n >= p->num_objects)\n-\t\treturn NULL;\n+\t\treturn -1;\n \tindex += 4 * 256;\n \tif (p->index_version == 1) {\n-\t\treturn index + (hashsz + 4) * n + 4;\n+\t\toidread(oid, index + (hashsz + 4) * n + 4);\n \t} else {\n \t\tindex += 8;\n-\t\treturn index + hashsz * n;\n+\t\toidread(oid, index + hashsz * n);\n \t}\n-}\n-\n-int nth_packed_object_id(struct object_id *oid,\n-\t\t\t struct packed_git *p,\n-\t\t\t uint32_t n)\n-{\n-\tconst unsigned char *hash = nth_packed_object_sha1(p, n);\n-\tif (!hash)\n-\t\treturn -1;\n-\thashcpy(oid->hash, hash);\n \treturn 0;\n }\n \ndiff --git a/packfile.h b/packfile.h\nindex 95b83ba25b..240aa73b95 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -121,15 +121,9 @@ void check_pack_index_ptr(const struct packed_git *p, const void *ptr);\n int bsearch_pack(const struct object_id *oid, const struct packed_git *p, uint32_t *result);\n \n /*\n- * Return the SHA-1 of the nth object within the specified packfile.\n- * Open the index if it is not already open.  The return value points\n- * at the SHA-1 within the mmapped index.  Return NULL if there is an\n- * error.\n- */\n-const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t n);\n-/*\n- * Like nth_packed_object_sha1, but write the data into the object specified by\n- * the the first argument.  Returns 0 on success, negative otherwise.\n+ * Write the oid of the nth object within the specified packfile into the first\n+ * parameter. Open the index if it is not already open.  Returns 0 on success,\n+ * negative otherwise.\n  */\n int nth_packed_object_id(struct object_id *, struct packed_git *, uint32_t n);\n \n-- \n2.25.1.823.g95c5488cf7\n"},{"id":"392420","messageId":"20200224182351.GA2182592@coredump.intra.peff.net","threadId":"52877","inReplyTo":"20200224042736.GA1018190@coredump.intra.peff.net","subject":"Re: [PATCH 01/10] nth_packed_object_oid(): use customary integer return","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-24T18:23:51Z","receivedAt":"2020-02-24T18:23:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 23, 2020 at 11:27:37PM -0500, Jeff King wrote:\n\n> Since we are changing the interface in a subtle way that the compiler\n> wouldn't catch, let's also change the name to catch any topics in\n> flight. We can drop the 'o' and make it nth_packed_object_id(). That's\n> slightly shorter, but also less redundant since the 'o' stands for\n> \"object\" already.\n\nNote that this does catch one topic that's in next:\njk/object-filter-with-bitmap. Since the newly added call there doesn't\ncheck the return value, it just needs s/oid/id/ in the function name.\n\n-Peff\n"},{"id":"392459","messageId":"20200225001924.GJ6462@camp.crustytoothpaste.net","threadId":"52877","inReplyTo":"20200224042944.GB1018190@coredump.intra.peff.net","subject":"Re: [PATCH 02/10] pack-objects: read delta base oid into object_id struct","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-02-25T00:19:24Z","receivedAt":"2020-02-25T00:19:30Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-02-24 at 04:29:44, Jeff King wrote:\n> @@ -1707,9 +1702,13 @@ static void check_object(struct object_entry *entry)\n>  \t\t\tunuse_pack(&w_curs);\n>  \t\t\treturn;\n>  \t\tcase OBJ_REF_DELTA:\n> -\t\t\tif (reuse_delta && !entry->preferred_base)\n> -\t\t\t\tbase_ref = use_pack(p, &w_curs,\n> -\t\t\t\t\t\tentry->in_pack_offset + used, NULL);\n> +\t\t\tif (reuse_delta && !entry->preferred_base) {\n> +\t\t\t\toidread(&base_ref,\n> +\t\t\t\t\tuse_pack(p, &w_curs,\n> +\t\t\t\t\t\t entry->in_pack_offset + used,\n> +\t\t\t\t\t\t NULL));\n> +\t\t\t\thave_base = 1;\n> +\t\t\t}\n\nThanks for using oidread here.  That makes my nascent future series that\nadds an algorithm member to struct object_id much easier.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"392460","messageId":"20200225003302.GK6462@camp.crustytoothpaste.net","threadId":"52877","inReplyTo":"20200224042625.GA1015553@coredump.intra.peff.net","subject":"Re: [PATCH 0/10] removing nth_packed_object_sha1()","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-02-25T00:33:02Z","receivedAt":"2020-02-25T00:33:09Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-02-24 at 04:26:25, Jeff King wrote:\n> This series gets rid of nth_packed_object_sha1(), converting its\n> remaining callers to nth_packed_object_oid(). The latter provides better\n> type-safety in general, and it also allowed me to clean up some tricky\n> bits of the pack-verification code (patch 7 below).\n> \n> This was inspired by brian's work in:\n> \n>  https://lore.kernel.org/git/20200222201749.937983-2-sandals@crustytoothpaste.net/\n> \n> but I decide not to base it on that series. It's semantically\n> independent, and with the exception of one line, textually independent\n> (patch 4 here touches a different parameter in the same call as the\n> patch linked above). So I think it's easier to handle them independently\n> and just resolve the trivial conflict.\n\nI think this is the right move.  The conflicts will be minor, as you\nsaid.\n\nThe series looked good to me, and as mentioned, I appreciate you using\noidread everywhere.  It's nice to get rid of more code with \"sha1\" in\nit.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"}]}