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

Reply via email to