Re: [PATCH] zlib.c: use size_t for size

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Fri, 12 Oct 2018, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Hi Junio,
> >
> > On Fri, 12 Oct 2018, Junio C Hamano wrote:
> >
> >> From: Martin Koegler <martin.koegler@chello.at>
> >> Date: Thu, 10 Aug 2017 20:13:08 +0200
> >> 
> >> Signed-off-by: Martin Koegler <martin.koegler@chello.at>
> >> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> >> ---
> >> 
> >>  * I made minimal adjustments to make the change apply to today's
> >>    codebase.  I still find some choices and mixing of off_t and
> >>    size_t done by the patch a bit iffy, and some arith may need to
> >>    become st_addX().  Extra set of eyes are very much appreciated.
> >> 
> >>  builtin/pack-objects.c | 10 +++++-----
> >>  cache.h                | 10 +++++-----
> >>  pack-check.c           |  6 +++---
> >>  pack.h                 |  2 +-
> >>  packfile.h             |  2 +-
> >>  wrapper.c              |  8 ++++----
> >>  zlib.c                 |  8 ++++----
> >>  7 files changed, 23 insertions(+), 23 deletions(-)
> >> 
> >> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> >> index e6316d294d..b9ca04eb8a 100644
> >> --- a/builtin/pack-objects.c
> >> +++ b/builtin/pack-objects.c
> >> @@ -266,15 +266,15 @@ static void copy_pack_data(struct hashfile *f,
> >>  		struct packed_git *p,
> >>  		struct pack_window **w_curs,
> >>  		off_t offset,
> >> -		off_t len)
> >> +		size_t len)
> >>  {
> >>  	unsigned char *in;
> >> -	unsigned long avail;
> >> +	size_t avail;
> >>  
> >>  	while (len) {
> >>  		in = use_pack(p, w_curs, offset, &avail);
> >>  		if (avail > len)
> >
> > Do we have to be careful to handle sizeof(off_t) != sizeof(size_t) here? I
> > guess we would receive a compiler warning about different sizes in that
> > case, but it makes me worry.
> 
> Yup, you just said exactly the same thing as I already said in the
> part you quoted.  I still find the mixed use of off_t and size_t in
> this patch iffy, and that was the secondary reason why the patch was
> kept in the stalled state for so long.  The primary reason was that
> nobody tried to dust it off and reignite the topic so far---which I
> am trying to correct, but as I said, this is just minimally adjusted
> to today's codebase, without any attempt to improve relative to the
> original patch.
> 
> I think we are in agreement in that this is not making things worse,
> as we are already in the mixed arith territory before this patch.

I will *try* to find the time to audit this some more, then. Maybe next
week, maybe the one afterwards... ;-)

Ciao,
Dscho

> 
>