Re: [PATCH v5 3/6] pack-bitmap-write: learn pack.writeBitmapLookupTable and add tests

From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
To: Derrick Stolee <derrickstolee@github.com>
Cc: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Abhradeep Chakraborty via GitGitGadget" <gitgitgadget@gmail.com>,
	git <git@vger.kernel.org>, "Taylor Blau" <me@ttaylorr.com>,
	"Kaartic Sivaram" <kaartic.sivaraam@gmail.com>,
	"Philip Oakley" <philipoakley@iee.email>,
	"Martin Ågren" <martin.agren@gmail.com>
Subject: Re: [PATCH v5 3/6] pack-bitmap-write: learn pack.writeBitmapLookupTable and add tests
Date: Sat, 13 Aug 2022 00:21:42 +0530	[thread overview]
Message-ID: <CAPOJW5x0coFREUPjFbF_zzQYbfEjOrL-j-G4N7MBUN4N6uS2jw@mail.gmail.com> (raw)
In-Reply-To: <805fb0df-45ab-7edd-8787-662b84201e2b@github.com>

Hello,

I think I have found the problem. Derrick was right that `mtime` part
is not the culprit. I tried to understand the whole midx workflow and
some questions were raised in my mind. I don't know whether those are
features or bugs (because I do not have much experience in
multi-pack-index code).

I am writing a brief description for the context of the issue and the
questions I have.

Let us start from the `write_midx_internal()` function. As
`packs_to_include` is null in our case, We can use the old midx to
write a new midx file. The line `ctx.m =
lookup_multi_pack_index(the_repository, object_dir)`[1]  does this. It
also loads packs that do not belong to any multi-pack-indexes. It also
sets `the_repository->objects->packed_git_intialized` to 1.  If we
look at our test case (`setup partial bitmap`) the last `.pack` file
(generated by `git repack &&` ) does not belong to any midx. So, that
pack will be loaded in this step.

[1] https://github.com/git/git/blob/5502f77b6944eda8e26813d8f542cffe7d110aea/midx.c#L1169

Next let us move to the `if (ctx.m)`[2] block. As we will be writing a
bitmap, `if (flags & MIDX_WRITE_REV_INDEX)` is true. Thus all packs
related to the old midx are loaded and `ctx.info[ctx.nr].p` stores the
pointers of these packs.

[2] https://github.com/git/git/blob/5502f77b6944eda8e26813d8f542cffe7d110aea/midx.c#L1182

After that we come to the `for_each_file_in_pack_dir(object_dir,
add_pack_to_midx, &ctx);` line[3] . The `add_pack_to_midx`[4] function
adds packs (that are not in the old midx) to `ctx.info`. Now I have a
question here - Why are we using  the `add_packed_git()`[5] function
provided we already loaded those packs in the
`lookup_multi_pack_index` step (i.e. 1st step)? These packs are not
added in `r->objects->packed_git`. This question is related to our
current issue.

I.e. instead of this -

   ctx->info[ctx->nr].p = add_packed_git(full_path,

full_path_len, 0);

Why not this (or similar) -

    for (cp = the_repository->objects->packed_git; cp; cp = cp->next)
        if (!cmp_idx_or_pack_name(cp->pack_name, full_path))
            ctx->info[ctx->nr].p = cp;

[3] https://github.com/git/git/blob/5502f77b6944eda8e26813d8f542cffe7d110aea/midx.c#L1221
[4] https://github.com/git/git/blob/5502f77b6944eda8e26813d8f542cffe7d110aea/midx.c#L462
[5] https://github.com/git/git/blob/5502f77b6944eda8e26813d8f542cffe7d110aea/midx.c#L492

 `write_midx_bitmap()` function is where bitmap related code starts.
let us directly jump into the `prepare_packed_git()` function (called
by `get_all_packs()`[6]). As I said previously,
`r->objects->packed_git_initialized` is already enabled so this
function becomes a no-op function. Which means it does not load the
newly written midx (by calling `prepare_multi_pack_index_one`
function) and uses old midx to write the bitmap (though we still have
new packs and they can be used with the old midx to generate the
bitmap, maybe?) . Here comes my second question - Is this the desired
case? or should we use the new midx to write the bitmaps?

One important point to note is that `get_all_packs()` returns
`r->objects->packed_git` which now stores pointers of all the packs
and only these packfiles have their `->index` set.

[6] https://github.com/git/git/blob/5502f77b6944eda8e26813d8f542cffe7d110aea/packfile.c#L1043

Now let us move to the last function - `oe_set_in_pack()` (called by
`prepare_midx_packing_data()`). Note that, we are passing
`ctx->info[ctx->pack_perm[from->pack_int_id]].p` along with other
parameters. As I have said in an earlier para (containing my first
question), `ctx->info` has some packs (i.e. newer packs that are not
related to the old midx) that are not installed in
`r->objects->packed_git` . In other words, we have two instances of
the same pack file - one in `r->objects->packed_git` list and another
in `ctx->info[id].p`. As `prepare_in_pack_by_idx` function only sets
`->index` for `r->objects->packed_git` packs, these packs (i.e.
`ctx.info[id].p`) do not have their p->index set and thus end up
calling the `oe_map_new_pack` function.