| Message ID | cover.1633727270.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | No more adding submodule ODB as alternate | expand |
Jonathan Tan <jonathantanmy@google.com> writes: > Thanks everyone for your reviews. Here's an updated patch set, including > Carlo's fixup squashed. This series LGTM. My comments on v3 center around the remaining references to the_repository, but as you have noted, we aren't at a stage where we can remove the_repository. Rather, we are making the_repository explicit in anticipation of removing the_repository from the entire ref writing system. I have some reservations about adding the backpointer from ref_store to repository. I don't think this is the best long-term API, but it is a reasonable step towards removing the_repository from the ref writing system, and as Josh mentioned: 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. Reviewed-by: Glen Choo <chooglen@google.com>
On 2021.10.08 14:08, Jonathan Tan wrote: > Thanks everyone for your reviews. Here's an updated patch set, including > Carlo's fixup squashed. > > 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 looks good to me. All my concerns from v3 have been addressed, so: Reviewed-by: Josh Steadmon <steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes: > On 2021.10.08 14:08, Jonathan Tan wrote: >> Thanks everyone for your reviews. Here's an updated patch set, including >> Carlo's fixup squashed. >> >> 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 looks good to me. All my concerns from v3 have been addressed, so: > > Reviewed-by: Josh Steadmon <steadmon@google.com> Thanks, all. Let's mark the topic for 'next' now.
Thanks everyone for your reviews. Here's an updated patch set, including Carlo's fixup squashed. 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 merge-ort.c | 18 +++-------- merge-recursive.c | 41 +++++++++++++------------- object-file.c | 9 +++++- refs.c | 32 +++++++++++++++----- refs/files-backend.c | 16 ++++++---- refs/packed-backend.c | 13 ++++++-- refs/packed-backend.h | 4 ++- refs/ref-cache.c | 10 +++++++ refs/ref-cache.h | 1 + refs/refs-internal.h | 11 +++++-- strbuf.c | 12 ++++++-- strbuf.h | 6 ++-- submodule.c | 18 +++++++++-- t/README | 7 ++--- t/t5526-fetch-submodules.sh | 3 ++ t/t5531-deep-submodule-push.sh | 3 ++ t/t5545-push-options.sh | 3 ++ t/t5572-pull-submodule.sh | 3 ++ t/t6437-submodule-merge.sh | 3 ++ t/t7418-submodule-sparse-gitmodules.sh | 3 ++ 20 files changed, 148 insertions(+), 68 deletions(-) Range-diff against v3: 1: 878c4dd288 ! 1: f050191d4c refs: plumb repo into ref stores @@ Commit message arbitrary repositories to ref iterators, plumb a repository into all ref stores. There are no changes to program logic. - (The repository is plumbed into the ref stores instead of directly into - the ref iterators themselves, so that existing code that operates on ref - stores do not need to be modified to also handle repositories.) - Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> @@ refs/packed-backend.c: static int release_snapshot(struct snapshot *snapshot) ## refs/packed-backend.h ## +@@ + #ifndef REFS_PACKED_BACKEND_H + #define REFS_PACKED_BACKEND_H + ++struct repository; + struct ref_transaction; + + /* @@ refs/packed-backend.h: struct ref_transaction; * even among packed refs. */ 2: 7180f622b1 = 2: 6418256919 refs: teach arbitrary repo support to iterators 3: 1a2e2e3e08 = 3: d624c198d6 refs: peeling non-the_repository iterators is BUG 4: 89347503af = 4: f3df7a31cb merge-{ort,recursive}: remove add_submodule_odb() 5: 17d6c0a793 ! 5: 78473b0f89 object-file: only register submodule ODB if needed @@ object-file.c: static int do_oid_object_info_extended(struct repository *r, } - if (register_all_submodule_odb_as_alternates()) ++ /* ++ * If r is the_repository, this might be an attempt at ++ * accessing a submodule object as if it were in the_repository ++ * (having called add_submodule_odb() on that submodule's ODB). ++ * If any such ODBs exist, register them and try again. ++ */ + if (r == the_repository && + register_all_submodule_odb_as_alternates()) /* We added some alternates; retry */ 6: 1eb2dda2dc = 6: f4241ea2e7 submodule: pass repo to check_has_commit() 7: 36e741dda8 ! 7: 8922bf48a2 submodule: trace adding submodule ODB as alternate @@ submodule.c: int register_all_submodule_odb_as_alternates(void) if (git_env_bool("GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB", 0)) BUG("register_all_submodule_odb_as_alternates() called"); } + + ## t/README ## +@@ t/README: GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=<boolean>, when true, makes + registering submodule ODBs as alternates a fatal action. Support for + this environment variable can be removed once the migration to + explicitly providing repositories when accessing submodule objects is +-complete (in which case we might want to replace this with a trace2 +-call so that users can make it visible if accessing submodule objects +-without an explicit repository still happens) or needs to be abandoned +-for whatever reason (in which case the migrated codepaths still retain +-their performance benefits). ++complete or needs to be abandoned for whatever reason (in which case the ++migrated codepaths still retain their performance benefits). + + Naming Tests + ------------