Re: [PATCH v2 01/11] index-pack, unpack-objects: use size_t for object size

From: Junio C Hamano <gitster@pobox.com>
Date: May 10, 2026, 02:41 UTC
Message-ID: <xmqqik8w9eb8.fsf@gitster.g>
In-Reply-To: <20260508190947.GA25792@tb-raspi4>

Torsten Bögershausen <tboegi@web.de> writes:

Show 37 quoted lines

> On Fri, May 08, 2026 at 09:36:53AM +0200, Johannes Schindelin wrote:
>> Hi Torsten,
>> 
>> On Tue, 5 May 2026, Torsten Bögershausen wrote:
>> 
>> > On Mon, May 04, 2026 at 05:08:18PM +0000, Johannes Schindelin via GitGitGadget wrote:
>> > > From: Johannes Schindelin <johannes.schindelin@gmx.de>
>> > > 
>> > > [...]
>> > > @@ -524,7 +524,8 @@ static void *unpack_raw_entry(struct object_entry *obj,
>> > >  			      struct object_id *oid)
>> > >  {
>> > >  	unsigned char *p;
>> > > -	unsigned long size, c;
>> > > +	size_t size;
>> > > +	unsigned long c;
>> >
>> > Does this look a little bit strange ?
>> 
>> Good point.
>> 
>> > p points to an unsigned char (better would be *uint8_t)
>> > then it is dereferenced into an "unsigned long".
>> > Then it is masked with 0x7f
>> > In short: should "c" be declared as uint8_t ?
>> 
>> Almost. It should be a `size_t`, so that we don't have to cast it when
>> shifting it. I'll include a fix in the next iteration.
>
> I think I was not very clear here.
> De-referencing a long (or size_t) from any address is something
> I would try to avoid:
> Some processors do not like to read a 32 or 64 bit value from
> an uneven address (and throw an exeption).
> x86 processors just handle it, using more than one bus cycle.
>
> In short: Please keep the up-cast and simply read an uint8_t.

Hmph, I do not think there is "up-cast" to keep. And we do not dereference a random pointer that would be suitable for unsigned char * as if it were "unsigned long *" or "size_t *" in this code.

This came from commit 48fb7deb5bbd87933e7d314b73d7c1b52667f80f

Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed Jun 17 17:22:27 2009 -0700

    Fix big left-shifts of unsigned char
    
    Shifting 'unsigned char' or 'unsigned short' left can result in sign
    extension errors, since the C integer promotion rules means that the
    unsigned char/short will get implicitly promoted to a signed 'int' due to
    the shift (or due to other operations).
    
    This normally doesn't matter, but if you shift things up sufficiently, it
    will now set the sign bit in 'int', and a subsequent cast to a bigger type
    (eg 'long' or 'unsigned long') will now sign-extend the value despite the
    original expression being unsigned.
    
    One example of this would be something like
    
            unsigned long size;
            unsigned char c;
    
            size += c << 24;
    
    where despite all the variables being unsigned, 'c << 24' ends up being a
    signed entity, and will get sign-extended when then doing the addition in
    an 'unsigned long' type.

You could rewrite Linus's example to

	unsigned char *cp;
	unsigned long size;
	unsigned char c;

	c = *cp;
	size += ((unsigned long)c) << 24;

While I am sympathetic to that position, I also would not mind

	unsigned char *cp;
	unsigned long size;
	unsigned long c;

	c = *cp;
	size += c << 24;

all that much. In any case, such a "clean-up" has little to do with the topic under discussion, and itshould be discussed separately on its own merit, most likely when the dust settles after this topic lands. Let's not contaminate the patches that is "a trivial rewrite that is so obviously correct to fix the assumption that ulong and size_t are of the same size everywhere" with unrelated clean-up.

Thanks.