| Message ID | cover.1629148153.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | In grep, no adding submodule ODB as alternates | expand |
On Mon, Aug 16, 2021 at 6:10 PM Jonathan Tan <jonathantanmy@google.com> wrote: > > Thanks for reviewing, everyone. Here are the requested changes. Thanks. You've addressed all my comments from the previous rounds. One minor suggestion which is not worth a re-roll on its own: > Range-diff against v2: [...] > 7: 94db10a4e5 ! 7: 8b86618531 submodule-config: pass repo upon blob config read > @@ Commit message > When reading the config of a submodule, if reading from a blob, read > using an explicitly specified repository instead of by adding the > submodule's ODB as an alternate and then reading an object from > the_repository. > > + This makes the "grep --recurse-submodules with submodules without > + .gitmodules in the working tree" test in t7814 work when > + GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB is true. This sounds to me as if the test was previously failing when GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB is true, which is not the case. I think an alternative could be: This code path is exercised by the "grep --recurse-submodules with submodules without .gitmodules in the working tree" test in t7814. The test passes with GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1, showing that we indeed are no longer accessing the submodule's ODB through the alternates list. But it's really a minor thing. With or without this change: Reviewed-by: Matheus Tavares <matheus.bernardino@usp.br>
Jonathan Tan <jonathantanmy@google.com> writes: > Thanks for reviewing, everyone. Here are the requested changes. > > Jonathan Tan (8): > submodule: lazily add submodule ODBs as alternates > grep: use submodule-ODB-as-alternate lazy-addition > grep: typesafe versions of grep_source_init > grep: read submodule entry with explicit repo > grep: allocate subrepos on heap > grep: add repository to OID grep sources > submodule-config: pass repo upon blob config read > t7814: show lack of alternate ODB-adding I didn't see anybody comment on this round (and do not think I saw anything glaringly wrong). Is everybody happy with this version? I am about to mark it for 'next' in the next issue of "What's cooking" report, so please holler if I should wait. Thanks.
On Tue, Sep 7, 2021 at 9:26 PM Junio C Hamano <gitster@pobox.com> wrote: > > Jonathan Tan <jonathantanmy@google.com> writes: > > > Thanks for reviewing, everyone. Here are the requested changes. > > > > Jonathan Tan (8): > > submodule: lazily add submodule ODBs as alternates > > grep: use submodule-ODB-as-alternate lazy-addition > > grep: typesafe versions of grep_source_init > > grep: read submodule entry with explicit repo > > grep: allocate subrepos on heap > > grep: add repository to OID grep sources > > submodule-config: pass repo upon blob config read > > t7814: show lack of alternate ODB-adding > > I didn't see anybody comment on this round (and do not think I saw > anything glaringly wrong). > > Is everybody happy with this version? I am about to mark it for > 'next' in the next issue of "What's cooking" report, so please > holler if I should wait. I think it's ready for 'next'. Jonathan addressed all my comments from previous rounds; I also just read the patches again and all looks good to me. Feel free to include my: Reviewed-by: Matheus Tavares <matheus.bernardino@usp.br>
Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes: > On Tue, Sep 7, 2021 at 9:26 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Jonathan Tan <jonathantanmy@google.com> writes: >> >> > Thanks for reviewing, everyone. Here are the requested changes. >> > >> > Jonathan Tan (8): >> > submodule: lazily add submodule ODBs as alternates >> > grep: use submodule-ODB-as-alternate lazy-addition >> > grep: typesafe versions of grep_source_init >> > grep: read submodule entry with explicit repo >> > grep: allocate subrepos on heap >> > grep: add repository to OID grep sources >> > submodule-config: pass repo upon blob config read >> > t7814: show lack of alternate ODB-adding >> >> I didn't see anybody comment on this round (and do not think I saw >> anything glaringly wrong). >> >> Is everybody happy with this version? I am about to mark it for >> 'next' in the next issue of "What's cooking" report, so please >> holler if I should wait. > > I think it's ready for 'next'. Jonathan addressed all my comments > from previous rounds; I also just read the patches again and all > looks good to me. Feel free to include my: > > Reviewed-by: Matheus Tavares <matheus.bernardino@usp.br> Thanks, both.
Thanks for reviewing, everyone. Here are the requested changes. Jonathan Tan (8): submodule: lazily add submodule ODBs as alternates grep: use submodule-ODB-as-alternate lazy-addition grep: typesafe versions of grep_source_init grep: read submodule entry with explicit repo grep: allocate subrepos on heap grep: add repository to OID grep sources submodule-config: pass repo upon blob config read t7814: show lack of alternate ODB-adding builtin/grep.c | 64 +++++++++++++++++++----------- config.c | 20 ++++++---- config.h | 3 ++ grep.c | 51 +++++++++++++++--------- grep.h | 22 ++++++++-- object-file.c | 5 +++ submodule-config.c | 5 ++- submodule.c | 25 +++++++++++- submodule.h | 8 ++++ t/README | 10 +++++ t/t7814-grep-recurse-submodules.sh | 3 ++ 11 files changed, 161 insertions(+), 55 deletions(-) Range-diff against v2: 1: 5994a517e8 = 1: 5994a517e8 submodule: lazily add submodule ODBs as alternates 2: 31e9b914c4 = 2: 31e9b914c4 grep: use submodule-ODB-as-alternate lazy-addition 3: aa3f1f3c89 = 3: aa3f1f3c89 grep: typesafe versions of grep_source_init 4: 050deacfb7 = 4: 050deacfb7 grep: read submodule entry with explicit repo 5: 3f24815224 ! 5: 7d1eeac4b5 grep: allocate subrepos on heap @@ builtin/grep.c: static void work_done(struct work_item *w) + repo_clear(repos_to_free[i]); + free(repos_to_free[i]); + } -+ free(repos_to_free); ++ FREE_AND_NULL(repos_to_free); + repos_to_free_nr = 0; + repos_to_free_alloc = 0; +} 6: 50c69a988b ! 6: f362fc278c grep: add repository to OID grep sources @@ grep.h: struct grep_opt { + + /* + * NEEDSWORK: See if we can remove this field, because the repository -+ * should probably be per-source, not per-repo. This is potentially the -+ * cause of at least one bug - "git grep" ignoring the textconv -+ * attributes from submodules. See [1] for more information. ++ * should probably be per-source. That is, grep.c functions using this ++ * field should probably start using "repo" in "struct grep_source" ++ * instead. ++ * ++ * This is potentially the cause of at least one bug - "git grep" ++ * ignoring the textconv attributes from submodules. See [1] for more ++ * information. + * [1] https://lore.kernel.org/git/CAHd-oW5iEQarYVxEXoTG-ua2zdoybTrSjCBKtO0YT292fm0NQQ@mail.gmail.com/ + */ struct repository *repo; 7: 94db10a4e5 ! 7: 8b86618531 submodule-config: pass repo upon blob config read @@ Commit message submodule's ODB as an alternate and then reading an object from the_repository. + This makes the "grep --recurse-submodules with submodules without + .gitmodules in the working tree" test in t7814 work when + GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB is true. + Signed-off-by: Jonathan Tan <jonathantanmy@google.com> ## config.c ## @@ config.c: int git_config_from_blob_oid(config_fn_t fn, const char *name, void *data) { -@@ config.c: static int git_config_from_blob_ref(config_fn_t fn, + struct object_id oid; - if (get_oid(name, &oid) < 0) +- if (get_oid(name, &oid) < 0) ++ if (repo_get_oid(repo, name, &oid) < 0) return error(_("unable to resolve config blob '%s'"), name); - return git_config_from_blob_oid(fn, name, &oid, data); + return git_config_from_blob_oid(fn, name, repo, &oid, data); 8: 4a51fcfb77 = 8: 4b3176f99e t7814: show lack of alternate ODB-adding