Re: [PATCH v2 9/9] pack-objects: improve partial packfile reuse

From: Jeff King <peff@peff.net>
Date: Oct 19, 2019, 23:23 UTC
Message-ID: <20191019232322.GB32408@sigill.intra.peff.net>
In-Reply-To: <CAP8UFD2rsZj3=KoPCEWw2sTXFhNkynrJLeAGWK2vEbD5GU8chA@mail.gmail.com>

On Sat, Oct 19, 2019 at 09:20:11PM +0200, Christian Couder wrote:

Show 21 quoted lines

> > > +static void write_reused_pack_one(size_t pos, struct hashfile *out,
> > > +                               struct pack_window **w_curs)
> > > +{
> > > +     off_t offset, next, cur;
> > > +     enum object_type type;
> > > +     unsigned long size;
> >
> > Is this a mem_sized size or a counter for less that 4GiB items?
> 
> What I can see is that `&size` is passed as the last argument to
> unpack_object_header() below. And unpack_object_header() is defined in
> packfile.h like this:
> 
> int unpack_object_header(struct packed_git *, struct pack_window **,
> off_t *, unsigned long *);
> 
> since at least 336226c259 (packfile.h: drop extern from function
> declarations, 2019-04-05)
> 
> So fixing this, if it needs to be fixed, should probably be part of a
> separate topic fixing unpack_object_header().

Yeah, this one definitely should be moved to whatever we used to represent object sizes in the future (size_t, or I guess off_t if we really want to handle huge objects on 32-bit systems too). But definitely it shouldn't happen in this series, and I don't think anybody interested in the other topic (converting the integer type for object sizes) needs to keep tabs on it. When they convert unpack_object_header(), the compiler will complain because of passing it as a pointer (the more insidious ones will be where we return an unsigned long to represent an object type, and somebody will have to look into every caller).

-Peff