Re: [PATCH 3/7] pack-revindex: make `load_pack_revindex` take a repository

[Taylor Blau <me@ttaylorr.com>]

On Tue, Apr 11, 2023 at 09:45:21AM -0400, Derrick Stolee wrote:
> On 4/10/2023 6:53 PM, Taylor Blau wrote:
> > In a future commit, we will introduce a `pack.readReverseIndex`
> > configuration, which forces Git to generate the reverse index from
> > scratch instead of loading it from disk.
> >
> > In order to avoid reading this configuration value more than once, we'll
> > use the `repo_settings` struct to lazily load this value.
> >
> > In order to access the `struct repo_settings`, add a repository argument
> > to `load_pack_revindex`, and update all callers to pass the correct
> > instance (in all cases, `the_repository`).
>
> If all callers use the_repository, then we could presumably use
> the_repository within the method directly. However, there are some
> cases where the call chain is less obvious that we have already
> entered something that is "repository scoped".
>
> The patch below applies on top of this one and is the result of
> exploring the two callers within pack-bitmap.c. Since they are
> static, I was able to only modify things within that file, but
> found two callers to _those_ methods that were repository scoped,
> so without making this connection we are losing that scope.
>
> There are other non-static methods in pack-bitmap.c that might
> benefit from wiring a repository pointer through (or adding a
> repository pointer to struct bitmap_index to get it for free),
> but I used the trick of defining a local repository pointer at
> the top of the method to make it easier to change in the future.
>
> Thanks,
> -Stolee

> @@ -581,7 +580,7 @@ struct bitmap_index *prepare_bitmap_git(struct repository *r)
>  {
>  	struct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));
>
> -	if (!open_bitmap(r, bitmap_git) && !load_bitmap(bitmap_git))
> +	if (!open_bitmap(r, bitmap_git) && !load_bitmap(r, bitmap_git))
>  		return bitmap_git;
>
>  	free_bitmap_index(bitmap_git);

Oops; we are indeed dropping the repository pointer that was given to
prepare_bitmap_git() here. It's unfortunate that we have to work through
so many layers to propagate it back down, but I agree that it's the
right thing to do.

> @@ -590,9 +589,10 @@ struct bitmap_index *prepare_bitmap_git(struct repository *r)
>
>  struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx)
>  {
> +	struct repository *r = the_repository;

OK; and here we're using the trick you mentioned in the patch message to
avoid having to propagate this even further out. The rest of the patch
looks sensible to me.

In terms of working this one in, it feels odd to include it as a
separate commit since we know the one immediately prior to it is kind of
broken.

Do you want to squash these together? Something else? Anything is fine
with me here.

Thanks,
Taylor