Re: [PATCH v5 4/6] pack-bitmap: prepare to read lookup table extension

[Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>]

On Tue, Jul 26, 2022 at 6:43 AM Taylor Blau <me@ttaylorr.com> wrote:
> Just noticing this now, but I wonder if we could avoid incrementing `p`
> here and instead write something like:
>
>     triplet->commit_pos = get_be32(p);
>     triplet->offset = get_be64(p + sizeof(uint32_t));
>     triplet->xor_row = get_be64(p + sizeof(uint64_t) + sizeof(uint32_t));
>
> I don't have a strong feeling about it, though, it just seems to read a
> little more directly to me and avoid modifying a variable that is only
> going to live as long as the function executes (p).

Ok, will update.

> > +/*
> > + * This function gets the raw triplet from `row`'th row in the
> > + * lookup table and fills that data to the `triplet`.
> > + */
> > +static int lookup_table_get_triplet(struct bitmap_index *bitmap_git,
> > +                                 uint32_t pos,
> > +                                 struct bitmap_lookup_table_triplet *triplet)
> > +{
> > +     unsigned char *p = NULL;
> > +     if (pos >= bitmap_git->entry_count)
> > +             return error(_("corrupt bitmap lookup table: triplet position out of index"));
> > +
> > +     p = bitmap_git->table_lookup + st_mult(pos, BITMAP_LOOKUP_TABLE_TRIPLET_WIDTH);
> > +
> > +     return lookup_table_get_triplet_by_pointer(triplet, p);
> > +}
>
> Very nice. This cleans things up nicely by being able to call
> lookup_table_get_triplet_by_pointer().
>
> Since these are static functions, it doesn't really matter whether or
> not they are prefixed with 'bitmap_', since they won't be visible
> outside of pack-bitmap.c's compilation unit. But it may be nice to
> prefix them with 'bitmap_' just to make it extra clear that these are
> internal functions meant to be used within the bitmap machinery only.

Yeah, sure!

> > +     static int is_corrupt = 0;
> > +
> > +     if (is_corrupt)
> > +             return NULL;
>
> What is the purpose of this conditional? We don't modify `is_corrupt`
> before reading it here, so this should be dead code, unless I'm missing
> something.

My intention behind this code was -
Initially `is_corrupt` is 0, so the above code will not execute for
the first `lazy_bitmap...()` call. Now, for some reason, if we get to
know that the `.bitmap` file is corrupted, the function will `goto
corrupt` and `is_corrupt` will be set to 1.
As `is_corrupt` is a static variable, its value will be preserved. So,
whenr we call `lazy_bitmap...()` function for the second time (or
third time etc.; i.e. `bitmap_for_commit` under a for loop), we
instantly know that `.bitmap` file is corrupt (by seeing `is_corrupt`)
and we will not do all the computations any more.

>
> > +     offset = triplet.offset;
> > +     xor_row = triplet.xor_row;
> > +
> > +     if (xor_row != 0xffffffff) {
>
> Is this outer conditional needed? I don't think it is. If xor_row is
> 0xffffffff, then the while loop below won't be entered, and
> xor_items_nr will be zero, meaning that the second while loop will also
> be skipped.

Yes, you're right - it is not needed. But it guarantees that all the
code inside its braces will be run only if has a `xor offset` causing
the allocation of `xor_items` array as lazy as possible.

Should I remove it?

> > +                     bitmap_git->map_pos = bitmap_git->map_pos + sizeof(uint32_t) + sizeof(uint8_t);
>
> Could we write:
>
>     bitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t)

Sure!

Thanks :)