Re: [BUG] add_again() off-by-one error in custom format

From: René Scharfe <l.s.r@web.de>
Date: Jun 22, 2017, 18:19 UTC
Message-ID: <87848e2d-9766-7dad-62cf-ded0c18f6dc3@web.de>
In-Reply-To: <20170618135623.3b27zhzdxur6gpg3@sigill.intra.peff.net>

Am 18.06.2017 um 15:56 schrieb Jeff King:

Show 20 quoted lines

> On Sun, Jun 18, 2017 at 02:59:04PM +0200, René Scharfe wrote:
> 
>>>> @@ -1586,6 +1587,9 @@ extern struct alternate_object_database {
>>>>    	struct strbuf scratch;
>>>>    	size_t base_len;
>>>> +	uint32_t loose_objects_subdir_bitmap[8];
>>>
>>> Is it worth the complexity of having an actual bitmap and not just an
>>> array of char? I guess it's not _that_ complex to access the bits, but
>>> there are a lot of magic numbers involved.
>>
>> That would be 224 bytes more per alternate_object_database, and we'd
>> gain simpler code.  Hmm.  We could add some bitmap helper macros, but
>> you're probably right that the first version should use the simplest
>> form for representing small bitmaps that we currently have.
> 
> I'd be OK with keeping it if we could reduce the number of magic
> numbers. E.g,. rather than 32 elsewhere use:
> 
>    (sizeof(*loose_objects_subdir_bitmap) * CHAR_BIT)

We have a bitsizeof macro for that.

> and similarly rather than 8 here use
> 
>    256 / sizeof(*loose_objects_subdir_bitmap) / CHAR_BIT

If we're pretending not to know the number of bits in a byte then we need to round up, and we have DIV_ROUND_UP for that. :)

> There's also already a bitmap data structure in ewah/bitmap.c. It's a
> little bit overkill, though, because it mallocs and will grow the bitmap
> as needed.

Yes, I feel that's too big a hammer for this purpose.

There is another example of a bitmap in shallow_c (just search for "32"). It would benefit from the macros mentioned above. That might make it easier to switch to the native word size (unsigned int) instead of using uint32_t everywhere.

But perhaps this one is actually a candidate for using EWAH, depending on the number of refs the code is supposed to handle.

Show 16 quoted lines

>>>> +static void read_loose_object_subdir(struct alternate_object_database *alt,
>>>> +				     int subdir_nr)
>>>
>>> I think it's nice to pull this out into a helper function. I do wonder
>>> if it should just be reusing for_each_loose_file_in_objdir(). You'd just
>>> need to expose the in_obj_subdir() variant publicly.
>>>
>>> It does do slightly more than we need (for the callbacks it actually
>>> builds the filename), but I doubt memcpy()ing a few bytes would be
>>> measurable.
>>
>> Good point.  The function also copies the common first two hex digits
>> for each entry.  But all that extra work is certainly dwarfed by the
>> readdir calls.
> 
> Yes. You're welcome to micro-optimize that implementation if you want. ;)

My knee-jerk reaction was that this would lead to ugliness, but on second look it might actually be doable. Will check, but I don't expect much of a speedup.

René