Re: [PATCH v2 4/7] odb: teach read_blob_entry to use size_t

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 28, 2021, 22:21 UTC
Message-ID: <nycvar.QRO.7.76.6.2110290019540.56@tvgsbejvaqbjf.bet>
In-Reply-To: <CAPUEsphWBuQybeBe8bjaUFZ=k+5maYCxiY4wche4XLVFPph+fw@mail.gmail.com>

Hi Carlo,

On Thu, 28 Oct 2021, Carlo Arenas wrote:

Show 25 quoted lines

> On Thu, Oct 28, 2021 at 1:50 PM Matt Cooper via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > diff --git a/entry.c b/entry.c
> > index 711ee0693c7..4cb3942dbdc 100644
> > --- a/entry.c
> > +++ b/entry.c
> > @@ -82,11 +82,13 @@ static int create_file(const char *path, unsigned int mode)
> >         return open(path, O_WRONLY | O_CREAT | O_EXCL, mode);
> >  }
> >
> > -void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)
> > +void *read_blob_entry(const struct cache_entry *ce, size_t *size)
> >  {
> >         enum object_type type;
> > -       void *blob_data = read_object_file(&ce->oid, &type, size);
> > +       unsigned long ul;
> > +       void *blob_data = read_object_file(&ce->oid, &type, &ul);
> >
> > +       *size = ul;
>
> Considering this unsigned long variable is obviously holding an
> incorrect value, might be worth adding a "TODO" comment here,
> mentioning it should be changed to a real size_t (probably after
> release, since it is obvious that change is too intrusive to need this
> as a kludge for now).

I do not think it would be a good idea to introduce a `TODO` comment here. Why _here_, of all places? Just because we use `size_t` correctly for a bit of the function?

No, that `TODO` would easily be forgotten when somebody tackles the big `unsigned long` -> `size_t` project. I therefore do not even want to add it. We know about this problem. No need for a code comment that is prone to become stale.

Ciao, Dscho