On 01/02/2017 06:09 PM, Jeff King wrote:
> [...]
> But I think the thing that gives me the most pause is that the oid
> version of the function feels bolted-on. The nth_packed_object_sha1()
> function is there specifically to give access to the mmap'd pack-index
> data. And at least for now, that only stores sha1s, not any kind of
> struct. If and when it does learn about other hashes, I'm not sure if
> we're going to want a generic nth_packed_object_oid() function, or if
> the callers would need to handle the various cases specially.
>
> Given that this patch only converts one caller, I'd be more inclined to
> just have the caller do its own hashcpy. Like:
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 1173071859..16345688b5 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -3769,12 +3769,14 @@ static int for_each_object_in_pack(struct packed_git *p, each_packed_object_fn c
>
> for (i = 0; i < p->num_objects; i++) {
> const unsigned char *sha1 = nth_packed_object_sha1(p, i);
> + struct object_id oid;
>
> if (!sha1)
> return error("unable to get sha1 of object %u in %s",
> i, p->pack_name);
>
> - r = cb(sha1, p, i, data);
> + hashcpy(oid.hash, sha1);
> + r = cb(&oid, p, i, data);
> if (r)
> break;
> }
>
> That punts on the issue for all the other callers, but like I said, I'm
> not quite sure if, when, and how it would make sense to convert them to
> using a "struct oid".
Your change is not safe if any of the callback functions ("cb") tuck
away a copy of the pointer that they are passed, expecting it to contain
the same object id later.
Michael