Re: [PATCH 00/20] packfile: avoid using the 'the_repository' global variable

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 28, 2024, 15:21 UTC
Message-ID: <Zx+r5gG8Kj1F0PHU@nand.local>
In-Reply-To: <CAOLa=ZRzM8tka_VLc7gmcukCMchKDQu0htF3LT9yiYWW7iXEow@mail.gmail.com>

On Mon, Oct 28, 2024 at 08:36:50AM -0500, karthik nayak wrote:

Show 31 quoted lines

> Jeff King <peff@peff.net> writes:
>
> > On Sun, Oct 27, 2024 at 05:23:24PM -0400, karthik nayak wrote:
> >
> >> While thinking about this over the last few days and also getting some
> >> advice from Patrick, I realized that we don't need to be this disruptive
> >> by simply adding the 'repository' variable to the already existing
> >> 'packed_git' struct. This allows us to leverage this information more
> >> easily, since most of the functions already have access to the
> >> 'packed_git' struct.
> >>
> >> This, plus the series by Jeff 'jk/dumb-http-finalize' which also removes
> >> some existing functions. We reduce the impact to only 3 functions being
> >> modified.
> >
> > Yeah, I noticed while working on that topic that we were dropping some
> > uses of the_repository. And FWIW I had the same notion, that packed_git
> > should perhaps refer to the repository struct in which it resides.
> >
> > As Taylor noted this is a tiny bit weird with respect to alternates,
> > which could exist in another repository (but don't have to! It could be
> > a bare objects/ directory). But I think from the perspective of a
> > particular process, we only have one repository struct that covers all
> > of its alternates for the duration of this process. So it would be OK in
> > practice. You might be able to get away with just storing a hash_algo
> > pointer in packed_git, which would be less weird (and is enough for the
> > bits I looked at, but perhaps not in the more general case).
>
> This was my thought as well regarding alternates. Also it should be
> noted that currently we're using the_repository anyways, so we will be
> in the same state as before.

Makes sense. Thanks, both, for thinking through it together.

> In a general case it seems more necessary to add the repo and not just
> the hash_algo. Mostly because there are parts which require access to
> the repository and also because some of my patches add config changes
> which also require access to the repository.

I could believe that ;-). I see that you posted some patches lower down in the thread, which I figure probably uncover some cases where we need more than just a pointer to the_hash_algo.

But let's read on and see exactly what shakes out.

Show 10 quoted lines

> > Looking at odb_pack_name(), it will still need to take a repository
> > struct, since we sometimes form it before having a packed_git. But for
> > most calls, I suspect you could have an alternate function that takes a
> > packed_git and uses both its "hash" member and the algorithm.
>
> I have four functions which would still need to take in a repository:
> 1. for_each_packed_object
> 2. has_object_pack
> 3. has_object_kept_pack
> 4. obd_pack_name

That matches my own thinking, but perhaps there are others that neither of us are coming up with off the tops of our heads. I think that as long as for_each_packed_object() continues to call prepare_packed_git (which sets up all of our alternates) and we continue to consult the packed_git_mru cache, we should be OK.

Show 5 quoted lines

> > Anyway, just my two cents having worked in the area recently.
> >
> > -Peff
>
> Thanks for your input!

Indeed. Thanks, both.

Thanks, Taylor