Re: [PATCH v3 0/7] No more adding submodule ODB as alternate

From: Josh Steadmon <steadmon@google.com>
Date: Oct 7, 2021, 18:32 UTC
Message-ID: <YV89VKzIghVEPZ1K@google.com>
In-Reply-To: <cover.1632956589.git.jonathantanmy@google.com>

On 2021.09.29 16:06, Jonathan Tan wrote:

Show 20 quoted lines

> This is on a merge of jk/ref-paranoia and jt/add-submodule-odb-clean-up
> (same as v2).
> 
> Here's the same patch set except that the repo is plumbed into the ref
> stores. (Iterators currently do not have any reference to their ref
> stores, so some of them still need repo fields. But because the ref
> stores now know their repos, calling code does not need to pass a repo
> when these iterators are instantiated.)
> 
> As you can see from the shorter patch list, this eliminates the need for
> some patches.
> 
> Jonathan Tan (7):
>   refs: plumb repo into ref stores
>   refs: teach arbitrary repo support to iterators
>   refs: peeling non-the_repository iterators is BUG
>   merge-{ort,recursive}: remove add_submodule_odb()
>   object-file: only register submodule ODB if needed
>   submodule: pass repo to check_has_commit()
>   submodule: trace adding submodule ODB as alternate

This is a summary of discussion from our Review Club meeting today; feedback comes from Jonathan Nieder, Emily Shaffer, Glen Choo, and me.

(Side note for the list: we're trying to be more open with our Review Club meetings. If you're interested in having video-chat discussions of patch series with us every other week, please get in touch with me.)

Summary: this series looks good to me, with a few nits (replied to the
patches directly for these). So, once those are addressed, this series
has my
Reviewed-by: Josh Steadmon <steadmon@google.com>

Eliminating submodules as alternates has (at least) a couple of nice
benefits, namely:
* we don't have to scan through the alternates one by one looking
  for missing objects
* having submodule objects show up as alternates makes it tricky to
  support partial clones of submodules, since the partial clone
  machinery doesn't know to talk to the submodule remote to find its
  objects

This series builds on the approach in jt/grep-wo-submodule-odb-as-alternate, which recently graduated to master. Specifically, we plumb `struct repository` pointers to various places that previously relied on `the_repository`, then we use GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB to show that no code paths in the test suite still rely on registering submodules as alternates.

Specifically to this series, we're adding struct repository pointers to `struct ref_store` and various iterators. This seems reasonable as we don't keep a lot of these structs around, so the additional memory usage isn't much of a concern.

Thanks for this series, really glad to see the end of submodule alternates!