On Mon, Jan 02, 2017 at 07:18:58PM +0100, Michael Haggerty wrote:
> > 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.
I think it's generally a given that callback functions should not assume
the lifetime of parameters extend beyond the end of the callback. That
said, I didn't audit the callers (although I'm pretty sure I wrote all
of them myself in this particular case).
-Peff