Re: [PATCH v3 2/6] pack-bitmap-write.c: write lookup table extension

[Taylor Blau <me@ttaylorr.com>]

On Fri, Jul 15, 2022 at 09:28:25PM +0530, Abhradeep Chakraborty wrote:
> > If that's the case, then I'd expect that printing `index[table[j]]` for
> > increasing `j` would output OIDs in increasing lexical order. But that
> > doesn't quite seem to be the case. From a debugger session that has a
> > breakpoint after computing and sorting table, along with building
> > `table_inv`:
> >
> >     (gdb) p oid_to_hex(&index[table[0]]->oid)
> >     $17 = 0x555555983ea0 <hexbuffer> "0006763074748d43b539c1c8e8882c08034ab178"
> >     (gdb) p oid_to_hex(&index[table[1]]->oid)
> >     $18 = 0x555555983ee1 <hexbuffer+65> "001ce83dd43f03dcfc67f29d38922e4a9682aab0"
> >     (gdb) p oid_to_hex(&index[table[2]]->oid)
> >     $19 = 0x555555983f22 <hexbuffer+130> "002db882ece2ab6a240e495a169c6e06422289c8"
> >     (gdb) p oid_to_hex(&index[table[3]]->oid)
> >     $20 = 0x555555983f63 <hexbuffer+195> "0007a5feb040e1ff704f3ad636619ddca3e7382b"
> >
> > that doesn't look like the OIDs are increasing in lexical order.
> >
> > I'm not quite sure if I'm even looking at the right thing, or if this is
> > to be expected, or if the comment isn't quite accurate. If you could
> > help clarify what's going on here, that would be great.
>
> I think you're not looking at the right thing. you should look at
> `writer.selected[table[i]].commit->object.oid` instead. I think the
> order of `index[]`
> is not the same as the pack index (or midx).
>
> I am saying this because if we use the `pos` variable (that we get
> from `commit_bitmap_writer_pos(&writer.selected[table[i]].commit->object.oid,
> index, index_nr)`) in `fprintf(stderr, "commit hex: %s\n",
> &index[pos]->oid);`, you'll see that `&index[pos]->oid` and
> `&writer.selected[table[i]].commit->object.oid` are not same. So, If
> you do -
>
>   int spos = commit_bitmap_writer_pos(&index[pos]->oid, index, index_nr);
>
> you'll see `spos` is not equal to `pos`.

`index` there comes from the list of objects that `pack-objects` or the
MIDX told us about, and it's sorted in lexical order (via
`write_pack_file()` -> `stage_tmp_packfiles()` -> `write_idx_file()`).

So I think this implementation is indexing the commits by the order they
appearn in the `writer.selected` array, *not* by the order they appear
in the index.

For what it's worth, I think the latter ordering makes more sense to use
to refer to individual objects. But we should be consistent with our
choice here and what's in the documentation. And right now I think we're
not, since the documentation change in the first patch says we write the
`commit_pos` field in order of the index:

    * {empty}
    commit_pos (4 byte integer, network byte order): ::
    It stores the object position of a commit (in the midx or pack
    index).

Thanks,
Taylor