On Sat, Jul 21, 2018 at 8:56 AM Elijah Newren <newren@xxxxxxxxx> wrote:
> > -- 8< --
> ...
> > diff --git a/pack-objects.h b/pack-objects.h
> > index 9f977ae800..11890e7217 100644
> > --- a/pack-objects.h
> > +++ b/pack-objects.h
> > @@ -15,7 +15,7 @@
> ...
> > @@ -353,37 +354,26 @@ static inline void oe_set_size(struct packing_data *pack,
> > static inline unsigned long oe_delta_size(struct packing_data *pack,
> > const struct object_entry *e)
> > {
> > - unsigned long size;
> > -
> > - packing_data_lock(pack);
> > - if (pack->delta_size)
> > - size = pack->delta_size[e - pack->objects];
> > + if (e->delta_size_valid)
> > + return e->delta_size_;
> > else
> > - size = e->delta_size_;
> > - packing_data_unlock(pack);
> > - return size;
> > + return pack->delta_size[e - pack->objects];
>
> Should this last line be wrapped with packing_data_lock/unlock calls?
> (And similarly in oe_set_delta_size when writing to this array?)
No. Data related to "e" alone could be accessed unprotected. This is
what try_delta has been doing (it's "trg_entry" in there). Because we
know that pack->delta_size[e - pack->objects] is for "e" and no other
entry, we can have the same treatment. If we access other entries'
data though like in my new_delta_size_array(), that could be racy.
--
Duy
