{"thread":{"id":"56248","subject":"[PATCH 4/7] grep: read submodule entry with explicit repo","startedAt":"2021-08-10T18:29:07Z","lastAt":"2021-09-27T17:46:11Z","messageCount":71,"participants":["Jonathan Tan","Junio C Hamano","Emily Shaffer","Matheus Tavares Bernardino","Josh Steadmon","Ramsay Jones","Ævar Arnfjörð Bjarmason","Matheus Tavares"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"432392","messageId":"30ead880b38c5f572e609554b075ef81ff80ad87.1628618950.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1628618950.git.jonathantanmy@google.com","subject":"[PATCH 4/7] grep: read submodule entry with explicit repo","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-10T18:28:42Z","receivedAt":"2021-08-10T18:29:07Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Replace an existing parse_object_or_die() call (which implicitly works\non the_repository) with a function call that allows a repository to be\npassed in. There is no such direct equivalent to parse_object_or_die(),\nbut we only need the type of the object, so replace with\noid_object_info().\n\nSigned-off-by: Jonathan Tan \n---\n builtin/grep.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex e454335e9d..9e61c7c993 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -457,27 +457,27 @@ static int grep_submodule(struct grep_opt *opt,\n \tsubopt.repo = &subrepo;\n \n \tif (oid) {\n-\t\tstruct object *object;\n+\t\tenum object_type object_type;\n \t\tstruct tree_desc tree;\n \t\tvoid *data;\n \t\tunsigned long size;\n \t\tstruct strbuf base = STRBUF_INIT;\n \n \t\tobj_read_lock();\n-\t\tobject = parse_object_or_die(oid, NULL);\n+\t\tobject_type = oid_object_info(&subrepo, oid, NULL);\n \t\tobj_read_unlock();\n \t\tdata = read_object_with_reference(&subrepo,\n-\t\t\t\t\t\t &object->oid, tree_type,\n+\t\t\t\t\t\t oid, tree_type,\n \t\t\t\t\t\t &size, NULL);\n \t\tif (!data)\n-\t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(&object->oid));\n+\t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(oid));\n \n \t\tstrbuf_addstr(&base, filename);\n \t\tstrbuf_addch(&base, '/');\n \n \t\tinit_tree_desc(&tree, data, size);\n \t\thit = grep_tree(&subopt, pathspec, &tree, &base, base.len,\n-\t\t\t\tobject->type == OBJ_COMMIT);\n+\t\t\t\tobject_type == OBJ_COMMIT);\n \t\tstrbuf_release(&base);\n \t\tfree(data);\n \t} else {\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432393","messageId":"e5e6a0dc1ef59b2ab419816e463814d93115e7f6.1628618950.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1628618950.git.jonathantanmy@google.com","subject":"[PATCH 3/7] grep: typesafe versions of grep_source_init","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-10T18:28:41Z","receivedAt":"2021-08-10T18:29:08Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"grep_source_init() can create \"struct grep_source\" objects and,\ndepending on the value of the type passed, some void-pointer parameters have\ndifferent meanings. Because one of these types (GREP_SOURCE_OID) will\nrequire an additional parameter in a subsequent patch, take the\nopportunity to increase clarity and type safety by replacing this\nfunction with individual functions for each type.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/grep.c | 4 ++--\n grep.c | 43 +++++++++++++++++++++++++++----------------\n grep.h | 8 +++++---\n 3 files changed, 34 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 87bcb934a2..e454335e9d 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -333,7 +333,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n \tstruct grep_source gs;\n \n \tgrep_source_name(opt, filename, tree_name_len, &pathbuf);\n-\tgrep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);\n+\tgrep_source_init_oid(&gs, pathbuf.buf, path, oid);\n \tstrbuf_release(&pathbuf);\n \n \tif (num_threads > 1) {\n@@ -359,7 +359,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \tstruct grep_source gs;\n \n \tgrep_source_name(opt, filename, 0, &buf);\n-\tgrep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);\n+\tgrep_source_init_file(&gs, buf.buf, filename);\n \tstrbuf_release(&buf);\n \n \tif (num_threads > 1) {\ndiff --git a/grep.c b/grep.c\nindex 424a39591b..ba3711dc56 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1830,7 +1830,7 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n \tstruct grep_source gs;\n \tint r;\n \n-\tgrep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL);\n+\tgrep_source_init_buf(&gs);\n \tgs.buf = buf;\n \tgs.size = size;\n \n@@ -1840,28 +1840,39 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n \treturn r;\n }\n \n-void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n-\t\t const char *name, const char *path,\n-\t\t const void *identifier)\n+void grep_source_init_file(struct grep_source *gs, const char *name,\n+\t\t\t const char *path)\n {\n-\tgs->type = type;\n+\tgs->type = GREP_SOURCE_FILE;\n \tgs->name = xstrdup_or_null(name);\n \tgs->path = xstrdup_or_null(path);\n \tgs->buf = NULL;\n \tgs->size = 0;\n \tgs->driver = NULL;\n+\tgs->identifier = xstrdup(path);\n+}\n \n-\tswitch (type) {\n-\tcase GREP_SOURCE_FILE:\n-\t\tgs->identifier = xstrdup(identifier);\n-\t\tbreak;\n-\tcase GREP_SOURCE_OID:\n-\t\tgs->identifier = oiddup(identifier);\n-\t\tbreak;\n-\tcase GREP_SOURCE_BUF:\n-\t\tgs->identifier = NULL;\n-\t\tbreak;\n-\t}\n+void grep_source_init_oid(struct grep_source *gs, const char *name,\n+\t\t\t const char *path, const struct object_id *oid)\n+{\n+\tgs->type = GREP_SOURCE_OID;\n+\tgs->name = xstrdup_or_null(name);\n+\tgs->path = xstrdup_or_null(path);\n+\tgs->buf = NULL;\n+\tgs->size = 0;\n+\tgs->driver = NULL;\n+\tgs->identifier = oiddup(oid);\n+}\n+\n+void grep_source_init_buf(struct grep_source *gs)\n+{\n+\tgs->type = GREP_SOURCE_BUF;\n+\tgs->name = NULL;\n+\tgs->path = NULL;\n+\tgs->buf = NULL;\n+\tgs->size = 0;\n+\tgs->driver = NULL;\n+\tgs->identifier = NULL;\n }\n \n void grep_source_clear(struct grep_source *gs)\ndiff --git a/grep.h b/grep.h\nindex 72f82b1e30..f4a3090f1c 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -195,9 +195,11 @@ struct grep_source {\n \tstruct userdiff_driver *driver;\n };\n \n-void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n-\t\t const char *name, const char *path,\n-\t\t const void *identifier);\n+void grep_source_init_file(struct grep_source *gs, const char *name,\n+\t\t\t const char *path);\n+void grep_source_init_oid(struct grep_source *gs, const char *name,\n+\t\t\t const char *path, const struct object_id *oid);\n+void grep_source_init_buf(struct grep_source *gs);\n void grep_source_clear_data(struct grep_source *gs);\n void grep_source_clear(struct grep_source *gs);\n void grep_source_load_driver(struct grep_source *gs,\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432394","messageId":"31e9b914c4bb500a98fbb14674c93a0d8ece47a2.1628618950.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1628618950.git.jonathantanmy@google.com","subject":"[PATCH 2/7] grep: use submodule-ODB-as-alternate lazy-addition","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-10T18:28:40Z","receivedAt":"2021-08-10T18:29:09Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In the parent commit, Git was taught to add submodule ODBs as alternates\nlazily, but grep does not use this because it computes the path to add\ndirectly, not going through add_submodule_odb(). Add an equivalent to\nadd_submodule_odb() that takes the exact ODB path and teach grep to use\nit.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/grep.c | 2 +-\n submodule.c | 5 +++++\n submodule.h | 1 +\n 3 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 7d2f8e5adb..87bcb934a2 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -450,7 +450,7 @@ static int grep_submodule(struct grep_opt *opt,\n \t * store is no longer global and instead is a member of the repository\n \t * object.\n \t */\n-\tadd_to_alternates_memory(subrepo.objects->odb->path);\n+\tadd_submodule_odb_by_path(subrepo.objects->odb->path);\n \tobj_read_unlock();\n \n \tmemcpy(&subopt, opt, sizeof(subopt));\ndiff --git a/submodule.c b/submodule.c\nindex 8fde90e906..8de1aecaeb 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -187,6 +187,11 @@ int add_submodule_odb(const char *path)\n \treturn ret;\n }\n \n+void add_submodule_odb_by_path(const char *path)\n+{\n+\tstring_list_insert(&added_submodule_odb_paths, xstrdup(path));\n+}\n+\n int register_all_submodule_odb_as_alternates(void)\n {\n \tint i;\ndiff --git a/submodule.h b/submodule.h\nindex c252784bc2..17a06cc43b 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -104,6 +104,7 @@ int bad_to_remove_submodule(const char *path, unsigned flags);\n * the_repository.\n */\n int add_submodule_odb(const char *path);\n+void add_submodule_odb_by_path(const char *path);\n int register_all_submodule_odb_as_alternates(void);\n \n /*\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432395","messageId":"5994a517e8afc345e8f649b2368756e22b0e9ebe.1628618950.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1628618950.git.jonathantanmy@google.com","subject":"[PATCH 1/7] submodule: lazily add submodule ODBs as alternates","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-10T18:28:39Z","receivedAt":"2021-08-10T18:29:09Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Teach Git to add submodule ODBs as alternates to the object store of\nthe_repository only upon the first access of an object not in\nthe_repository, and not when add_submodule_odb() is called.\n\nThis provides a means of gradually migrating from accessing a\nsubmodule's object through alternates to accessing a submodule's object\nby explicitly passing its repository object. Any Git command can declare\nthat it might access submodule objects by calling add_submodule_odb()\n(as they do now), but the submodule ODBs themselves will not be added\nuntil needed, so individual commands and/or combinations of arguments\ncan be migrated one by one.\n\n[The advantage of explicit repository-object passing is code clarity (it\nis clear which repository an object read is from), performance (there is\nno need to linearly search through all submodule ODBs whenever an object\nis accessed from any repository, whether superproject or submodule), and\nthe possibility of future features like partial clone submodules (which\nright now is not possible because if an object is missing, we do not\nknow which repository to lazy-fetch into).]\n\nThis commit also introduces an environment variable that a test may set\nto make the actual registration of alternates fatal, in order to\ndemonstrate that its codepaths do not need this registration.\n\nSigned-off-by: Jonathan Tan \n---\n object-file.c | 5 +++++\n submodule.c | 20 +++++++++++++++++++-\n submodule.h | 7 +++++++\n t/README | 10 ++++++++++\n 4 files changed, 41 insertions(+), 1 deletion(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 3d27dc8dea..621b121bcb 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -32,6 +32,7 @@\n #include \"packfile.h\"\n #include \"object-store.h\"\n #include \"promisor-remote.h\"\n+#include \"submodule.h\"\n \n /* The maximum size for an object header. */\n #define MAX_HEADER_LEN 32\n@@ -1592,6 +1593,10 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\t\t\tbreak;\n \t\t}\n \n+\t\tif (register_all_submodule_odb_as_alternates())\n+\t\t\t/* We added some alternates; retry */\n+\t\t\tcontinue;\n+\n \t\t/* Check if it is a missing object */\n \t\tif (fetch_if_missing && repo_has_promisor_remote(r) &&\n \t\t !already_retried &&\ndiff --git a/submodule.c b/submodule.c\nindex 8e611fe1db..8fde90e906 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -165,6 +165,8 @@ void stage_updated_gitmodules(struct index_state *istate)\n \t\tdie(_(\"staging updated .gitmodules failed\"));\n }\n \n+static struct string_list added_submodule_odb_paths = STRING_LIST_INIT_NODUP;\n+\n /* TODO: remove this function, use repo_submodule_init instead. */\n int add_submodule_odb(const char *path)\n {\n@@ -178,12 +180,28 @@ int add_submodule_odb(const char *path)\n \t\tret = -1;\n \t\tgoto done;\n \t}\n-\tadd_to_alternates_memory(objects_directory.buf);\n+\tstring_list_insert(&added_submodule_odb_paths,\n+\t\t\t strbuf_detach(&objects_directory, NULL));\n done:\n \tstrbuf_release(&objects_directory);\n \treturn ret;\n }\n \n+int register_all_submodule_odb_as_alternates(void)\n+{\n+\tint i;\n+\tint ret = added_submodule_odb_paths.nr;\n+\n+\tfor (i = 0; i < added_submodule_odb_paths.nr; i++)\n+\t\tadd_to_alternates_memory(added_submodule_odb_paths.items[i].string);\n+\tif (ret) {\n+\t\tstring_list_clear(&added_submodule_odb_paths, 0);\n+\t\tif (git_env_bool(\"GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB\", 0))\n+\t\t\tBUG(\"register_all_submodule_odb_as_alternates() called\");\n+\t}\n+\treturn ret;\n+}\n+\n void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n \t\t\t\t\t const char *path)\n {\ndiff --git a/submodule.h b/submodule.h\nindex 84640c49c1..c252784bc2 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -97,7 +97,14 @@ int submodule_uses_gitfile(const char *path);\n #define SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED (1<<2)\n int bad_to_remove_submodule(const char *path, unsigned flags);\n \n+/*\n+ * Call add_submodule_odb() to add the submodule at the given path to a list.\n+ * When register_all_submodule_odb_as_alternates() is called, the object stores\n+ * of all submodules in that list will be added as alternates in\n+ * the_repository.\n+ */\n int add_submodule_odb(const char *path);\n+int register_all_submodule_odb_as_alternates(void);\n \n /*\n * Checks if there are submodule changes in a..b. If a is the null OID,\ndiff --git a/t/README b/t/README\nindex 9e70122302..8b67b4f00b 100644\n--- a/t/README\n+++ b/t/README\n@@ -448,6 +448,16 @@ GIT_TEST_CHECKOUT_WORKERS= overrides the 'checkout.workers' setting\n to and 'checkout.thresholdForParallelism' to 0, forcing the\n execution of the parallel-checkout code.\n \n+GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=, when true, makes\n+registering submodule ODBs as alternates a fatal action. Support for\n+this environment variable can be removed once the migration to\n+explicitly providing repositories when accessing submodule objects is\n+complete (in which case we might want to replace this with a trace2\n+call so that users can make it visible if accessing submodule objects\n+without an explicit repository still happens) or needs to be abandoned\n+for whatever reason (in which case the migrated codepaths still retain\n+their performance benefits).\n+\n Naming Tests\n ------------\n \n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432396","messageId":"cover.1628618950.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":null,"subject":"[PATCH 0/7] In grep, no adding submodule ODB as alternates","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-10T18:28:38Z","receivedAt":"2021-08-10T18:29:12Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This patch series removes the need to add submodule ODBs as alternates\nin all codepaths covered by t7814. I believe that that is also all the\ncodepaths covered by \"git grep\", but if it isn't, the uncovered\ncodepaths will still work - they will just not benefit from the\nperformance improvements.\n\nIn doing this work of migrating away from adding submodule ODBs as\nalternates, I'm mainly motivated by the possibility of adding partial\nclone submodule support, but this has benefits even for those who do not\nuse partial clones, as described in the documentation in patch 1.\n\nTo reviewers: you can cherry-pick the last patch onto one of the earlier\nones to observe what happens when the code still accesses a submodule\nobject as if it were in the_repository.\n\nJonathan Tan (7):\n submodule: lazily add submodule ODBs as alternates\n grep: use submodule-ODB-as-alternate lazy-addition\n grep: typesafe versions of grep_source_init\n grep: read submodule entry with explicit repo\n grep: allocate subrepos on heap\n grep: add repository to OID grep sources\n t7814: show lack of alternate ODB-adding\n\n builtin/grep.c | 49 +++++++++++++++++++++---------\n grep.c | 48 ++++++++++++++++++-----------\n grep.h | 10 ++++--\n object-file.c | 5 +++\n submodule.c | 25 ++++++++++++++-\n submodule.h | 8 +++++\n t/README | 10 ++++++\n t/t7814-grep-recurse-submodules.sh | 3 ++\n 8 files changed, 122 insertions(+), 36 deletions(-)\n\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432397","messageId":"f62608e88f125096c1f236a47e3ee670104c7b78.1628618950.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1628618950.git.jonathantanmy@google.com","subject":"[PATCH 5/7] grep: allocate subrepos on heap","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-10T18:28:43Z","receivedAt":"2021-08-10T18:29:17Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, struct repository objects corresponding to submodules are\nallocated on the stack in grep_submodule(). This currently works because\nthey will not be used once grep_submodule() exits, but a subsequent\npatch will require these structs to be accessible for longer (perhaps\neven in another thread). Allocate them on the heap and clear them only\nat the very end.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/grep.c | 37 ++++++++++++++++++++++++++++---------\n 1 file changed, 28 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 9e61c7c993..5a40e18e47 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -65,6 +65,9 @@ static int todo_done;\n /* Has all work items been added? */\n static int all_work_added;\n \n+static struct repository **repos_to_free;\n+static size_t repos_to_free_nr, repos_to_free_alloc;\n+\n /* This lock protects all the variables above. */\n static pthread_mutex_t grep_mutex;\n \n@@ -168,6 +171,17 @@ static void work_done(struct work_item *w)\n \tgrep_unlock();\n }\n \n+static void free_repos(void)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < repos_to_free_nr; i++) {\n+\t\trepo_clear(repos_to_free[i]);\n+\t\tfree(repos_to_free[i]);\n+\t}\n+\tfree(repos_to_free);\n+}\n+\n static void *run(void *arg)\n {\n \tint hit = 0;\n@@ -415,19 +429,24 @@ static int grep_submodule(struct grep_opt *opt,\n \t\t\t const struct object_id *oid,\n \t\t\t const char *filename, const char *path, int cached)\n {\n-\tstruct repository subrepo;\n+\tstruct repository *subrepo;\n \tstruct repository *superproject = opt->repo;\n \tconst struct submodule *sub;\n \tstruct grep_opt subopt;\n-\tint hit;\n+\tint hit = 0;\n \n \tsub = submodule_from_path(superproject, null_oid(), path);\n \n \tif (!is_submodule_active(superproject, path))\n \t\treturn 0;\n \n-\tif (repo_submodule_init(&subrepo, superproject, sub))\n+\tsubrepo = xmalloc(sizeof(*subrepo));\n+\tif (repo_submodule_init(subrepo, superproject, sub)) {\n+\t\tfree(subrepo);\n \t\treturn 0;\n+\t}\n+\tALLOC_GROW(repos_to_free, repos_to_free_nr + 1, repos_to_free_alloc);\n+\trepos_to_free[repos_to_free_nr++] = subrepo;\n \n \t/*\n \t * NEEDSWORK: repo_read_gitmodules() might call\n@@ -438,7 +457,7 @@ static int grep_submodule(struct grep_opt *opt,\n \t * subrepo's odbs to the in-memory alternates list.\n \t */\n \tobj_read_lock();\n-\trepo_read_gitmodules(&subrepo, 0);\n+\trepo_read_gitmodules(subrepo, 0);\n \n \t/*\n \t * NEEDSWORK: This adds the submodule's object directory to the list of\n@@ -450,11 +469,11 @@ static int grep_submodule(struct grep_opt *opt,\n \t * store is no longer global and instead is a member of the repository\n \t * object.\n \t */\n-\tadd_submodule_odb_by_path(subrepo.objects->odb->path);\n+\tadd_submodule_odb_by_path(subrepo->objects->odb->path);\n \tobj_read_unlock();\n \n \tmemcpy(&subopt, opt, sizeof(subopt));\n-\tsubopt.repo = &subrepo;\n+\tsubopt.repo = subrepo;\n \n \tif (oid) {\n \t\tenum object_type object_type;\n@@ -464,9 +483,9 @@ static int grep_submodule(struct grep_opt *opt,\n \t\tstruct strbuf base = STRBUF_INIT;\n \n \t\tobj_read_lock();\n-\t\tobject_type = oid_object_info(&subrepo, oid, NULL);\n+\t\tobject_type = oid_object_info(subrepo, oid, NULL);\n \t\tobj_read_unlock();\n-\t\tdata = read_object_with_reference(&subrepo,\n+\t\tdata = read_object_with_reference(subrepo,\n \t\t\t\t\t\t oid, tree_type,\n \t\t\t\t\t\t &size, NULL);\n \t\tif (!data)\n@@ -484,7 +503,6 @@ static int grep_submodule(struct grep_opt *opt,\n \t\thit = grep_cache(&subopt, pathspec, cached);\n \t}\n \n-\trepo_clear(&subrepo);\n \treturn hit;\n }\n \n@@ -1182,5 +1200,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\trun_pager(&opt, prefix);\n \tclear_pathspec(&pathspec);\n \tfree_grep_patterns(&opt);\n+\tfree_repos();\n \treturn !hit;\n }\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432398","messageId":"b2df2455871dc5c94ec804e9c0ce935c19da335f.1628618950.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1628618950.git.jonathantanmy@google.com","subject":"[PATCH 6/7] grep: add repository to OID grep sources","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-10T18:28:44Z","receivedAt":"2021-08-10T18:29:19Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Record the repository whenever an OID grep source is created, and teach\nthe worker threads to explicitly provide the repository when accessing\nobjects.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/grep.c | 2 +-\n grep.c | 7 +++++--\n grep.h | 4 +++-\n 3 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 5a40e18e47..ea6df6dca4 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -347,7 +347,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n \tstruct grep_source gs;\n \n \tgrep_source_name(opt, filename, tree_name_len, &pathbuf);\n-\tgrep_source_init_oid(&gs, pathbuf.buf, path, oid);\n+\tgrep_source_init_oid(&gs, pathbuf.buf, path, oid, opt->repo);\n \tstrbuf_release(&pathbuf);\n \n \tif (num_threads > 1) {\ndiff --git a/grep.c b/grep.c\nindex ba3711dc56..14c677b4ae 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1853,7 +1853,8 @@ void grep_source_init_file(struct grep_source *gs, const char *name,\n }\n \n void grep_source_init_oid(struct grep_source *gs, const char *name,\n-\t\t\t const char *path, const struct object_id *oid)\n+\t\t\t const char *path, const struct object_id *oid,\n+\t\t\t struct repository *repo)\n {\n \tgs->type = GREP_SOURCE_OID;\n \tgs->name = xstrdup_or_null(name);\n@@ -1862,6 +1863,7 @@ void grep_source_init_oid(struct grep_source *gs, const char *name,\n \tgs->size = 0;\n \tgs->driver = NULL;\n \tgs->identifier = oiddup(oid);\n+\tgs->repo = repo;\n }\n \n void grep_source_init_buf(struct grep_source *gs)\n@@ -1901,7 +1903,8 @@ static int grep_source_load_oid(struct grep_source *gs)\n {\n \tenum object_type type;\n \n-\tgs->buf = read_object_file(gs->identifier, &type, &gs->size);\n+\tgs->buf = repo_read_object_file(gs->repo, gs->identifier, &type,\n+\t\t\t\t\t&gs->size);\n \tif (!gs->buf)\n \t\treturn error(_(\"'%s': unable to read %s\"),\n \t\t\t gs->name,\ndiff --git a/grep.h b/grep.h\nindex f4a3090f1c..c5234f9b38 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -187,6 +187,7 @@ struct grep_source {\n \t\tGREP_SOURCE_BUF,\n \t} type;\n \tvoid *identifier;\n+\tstruct repository *repo; /* if GREP_SOURCE_OID */\n \n \tchar *buf;\n \tunsigned long size;\n@@ -198,7 +199,8 @@ struct grep_source {\n void grep_source_init_file(struct grep_source *gs, const char *name,\n \t\t\t const char *path);\n void grep_source_init_oid(struct grep_source *gs, const char *name,\n-\t\t\t const char *path, const struct object_id *oid);\n+\t\t\t const char *path, const struct object_id *oid,\n+\t\t\t struct repository *repo);\n void grep_source_init_buf(struct grep_source *gs);\n void grep_source_clear_data(struct grep_source *gs);\n void grep_source_clear(struct grep_source *gs);\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432399","messageId":"f1fc89894b8832ab0f64f301b1621ae15654e08c.1628618950.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1628618950.git.jonathantanmy@google.com","subject":"[PATCH 7/7] t7814: show lack of alternate ODB-adding","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-10T18:28:45Z","receivedAt":"2021-08-10T18:29:20Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The previous patches have made \"git grep\" no longer need to add\nsubmodule ODBs as alternates, at least for the code paths tested in\nt7814. Demonstrate this by making adding a submodule ODB as an alternate\nfatal in this test.\n\nSigned-off-by: Jonathan Tan \n---\n t/t7814-grep-recurse-submodules.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 828cb3ba58..3172f5b936 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -8,6 +8,9 @@ submodules.\n \n . ./test-lib.sh\n \n+GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1\n+export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB\n+\n test_expect_success 'setup directory structure and submodule' '\n \techo \"(1|2)d(3|4)\" >a &&\n \tmkdir b &&\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432418","messageId":"xmqqtujxc8a3.fsf@gitster.g","threadId":"56248","inReplyTo":"5994a517e8afc345e8f649b2368756e22b0e9ebe.1628618950.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/7] submodule: lazily add submodule ODBs as alternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-10T21:13:56Z","receivedAt":"2021-08-10T21:14:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> @@ -1592,6 +1593,10 @@ static int do_oid_object_info_extended(struct repository *r,\n> \t\t\t\tbreak;\n> \t\t}\n> \n> +\t\tif (register_all_submodule_odb_as_alternates())\n> +\t\t\t/* We added some alternates; retry */\n> +\t\t\tcontinue;\n> +\n\nOK. Unless we are running in the \"we no longer are relying on the\nsubmodule-odb-as-alternates hack\" mode, the control may reach this\npoint number of times, so this caller expects the function to return\nhow many new odbs were added with this single invocation.\n\n> diff --git a/submodule.c b/submodule.c\n> index 8e611fe1db..8fde90e906 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -165,6 +165,8 @@ void stage_updated_gitmodules(struct index_state *istate)\n> \t\tdie(_(\"staging updated .gitmodules failed\"));\n> }\n> \n> +static struct string_list added_submodule_odb_paths = STRING_LIST_INIT_NODUP;\n> +\n\nI see 2/7 allows callers to add paths for submodule odb to this\nlist.\n\n> @@ -178,12 +180,28 @@ int add_submodule_odb(const char *path)\n> \t\tret = -1;\n> \t\tgoto done;\n> \t}\n> -\tadd_to_alternates_memory(objects_directory.buf);\n> +\tstring_list_insert(&added_submodule_odb_paths,\n> +\t\t\t strbuf_detach(&objects_directory, NULL));\n\nOK. By default, any codepath that still call add_submodule_odb()\nwill add the paths to submodules to the list (so that they will\nlazily be loaded).\n\n> +int register_all_submodule_odb_as_alternates(void)\n> +{\n> +\tint i;\n> +\tint ret = added_submodule_odb_paths.nr;\n> +\n> +\tfor (i = 0; i < added_submodule_odb_paths.nr; i++)\n> +\t\tadd_to_alternates_memory(added_submodule_odb_paths.items[i].string);\n> +\tif (ret) {\n> +\t\tstring_list_clear(&added_submodule_odb_paths, 0);\n> +\t\tif (git_env_bool(\"GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB\", 0))\n> +\t\t\tBUG(\"register_all_submodule_odb_as_alternates() called\");\n> +\t}\n> +\treturn ret;\n> +}\n\nOK. We add new ones that were added to the list since we were\ncalled for the last time and clear the list. When we didn't add\nanything, we will return 0. Seems to mean well.\n\nI wonder if we need to prepare ourselves to catch callers that call\nadd_submodule_odb() to the same path twice or more. Probably not.\n\nThanks.\n\nThe \"pretend as if objects in submodule odb are locally available\",\neven though it may have been useful, was an ugly hack invented\nbefore we started adding the \"access more than one repository at the\ntime with the 'repo' arg\" extended API. It is pleasing to see this\nstep shows an approach to incrementally migrate the users of the\nhack.\n\n"},{"id":"432420","messageId":"xmqqlf59c759.fsf@gitster.g","threadId":"56248","inReplyTo":"e5e6a0dc1ef59b2ab419816e463814d93115e7f6.1628618950.git.jonathantanmy@google.com","subject":"Re: [PATCH 3/7] grep: typesafe versions of grep_source_init","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-10T21:38:26Z","receivedAt":"2021-08-10T21:38:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> grep_source_init() can create \"struct grep_source\" objects and,\n> depending on the value of the type passed, some void-pointer parameters have\n> different meanings. Because one of these types (GREP_SOURCE_OID) will\n> require an additional parameter in a subsequent patch, take the\n> opportunity to increase clarity and type safety by replacing this\n> function with individual functions for each type.\n\nNice clean-up.\n"},{"id":"432522","messageId":"YRRBOkqQSvhwsgrC@google.com","threadId":"56248","inReplyTo":"cover.1628618950.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/7] In grep, no adding submodule ODB as alternates","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-08-11T21:29:30Z","receivedAt":"2021-08-11T21:29:40Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, Aug 10, 2021 at 11:28:38AM -0700, Jonathan Tan wrote:\n> \n> This patch series removes the need to add submodule ODBs as alternates\n> in all codepaths covered by t7814. I believe that that is also all the\n> codepaths covered by \"git grep\", but if it isn't, the uncovered\n> codepaths will still work - they will just not benefit from the\n> performance improvements.\n> \n> In doing this work of migrating away from adding submodule ODBs as\n> alternates, I'm mainly motivated by the possibility of adding partial\n> clone submodule support, but this has benefits even for those who do not\n> use partial clones, as described in the documentation in patch 1.\n> \n> To reviewers: you can cherry-pick the last patch onto one of the earlier\n> ones to observe what happens when the code still accesses a submodule\n> object as if it were in the_repository.\n\nOne note - I liked this tip, I think that is a useful thing for\nreviewers to see. Thanks.\n\n - Emily\n"},{"id":"432523","messageId":"YRRCRHTgXU2x+s0l@google.com","threadId":"56248","inReplyTo":"5994a517e8afc345e8f649b2368756e22b0e9ebe.1628618950.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/7] submodule: lazily add submodule ODBs as alternates","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-08-11T21:33:56Z","receivedAt":"2021-08-11T21:34:07Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, Aug 10, 2021 at 11:28:39AM -0700, Jonathan Tan wrote:\n> diff --git a/object-file.c b/object-file.c\n> index 3d27dc8dea..621b121bcb 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -32,6 +32,7 @@\n> #include \"packfile.h\"\n> #include \"object-store.h\"\n> #include \"promisor-remote.h\"\n> +#include \"submodule.h\"\n> \n> /* The maximum size for an object header. */\n> #define MAX_HEADER_LEN 32\n> @@ -1592,6 +1593,10 @@ static int do_oid_object_info_extended(struct repository *r,\n> \t\t\t\tbreak;\n> \t\t}\n> \n> +\t\tif (register_all_submodule_odb_as_alternates())\n> +\t\t\t/* We added some alternates; retry */\n> +\t\t\tcontinue;\n> +\n\nOk, this is where we finally get around to loading the alternate much\nlater. Fine.\n\n> \t\t/* Check if it is a missing object */\n> \t\tif (fetch_if_missing && repo_has_promisor_remote(r) &&\n> \t\t !already_retried &&\n> diff --git a/submodule.c b/submodule.c\n> index 8e611fe1db..8fde90e906 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -165,6 +165,8 @@ void stage_updated_gitmodules(struct index_state *istate)\n> \t\tdie(_(\"staging updated .gitmodules failed\"));\n> }\n> \n> +static struct string_list added_submodule_odb_paths = STRING_LIST_INIT_NODUP;\n> +\n> /* TODO: remove this function, use repo_submodule_init instead. */\n> int add_submodule_odb(const char *path)\n> {\n> @@ -178,12 +180,28 @@ int add_submodule_odb(const char *path)\n> \t\tret = -1;\n> \t\tgoto done;\n> \t}\n> -\tadd_to_alternates_memory(objects_directory.buf);\n> +\tstring_list_insert(&added_submodule_odb_paths,\n> +\t\t\t strbuf_detach(&objects_directory, NULL));\n\nAnd here is where we hijack the usual alternate load and put the path\ninto the lazy load list instead. Ok.\n\n> done:\n> \tstrbuf_release(&objects_directory);\n> \treturn ret;\n> }\n> \n> +int register_all_submodule_odb_as_alternates(void)\n> +{\n> +\tint i;\n> +\tint ret = added_submodule_odb_paths.nr;\n> +\n> +\tfor (i = 0; i < added_submodule_odb_paths.nr; i++)\n> +\t\tadd_to_alternates_memory(added_submodule_odb_paths.items[i].string);\n> +\tif (ret) {\n> +\t\tstring_list_clear(&added_submodule_odb_paths, 0);\n> +\t\tif (git_env_bool(\"GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB\", 0))\n> +\t\t\tBUG(\"register_all_submodule_odb_as_alternates() called\");\n\nNice, and this is the flag you mentioned in the cover letter to complain\nif we're trying to use alternates to access submodule objects. This will\nbe useful for finding out other random places where we are using that\nweird hack.\n\n> +\t}\n> +\treturn ret;\n> +}\n> +\n> void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n> \t\t\t\t\t const char *path)\n> {\n> diff --git a/submodule.h b/submodule.h\n> index 84640c49c1..c252784bc2 100644\n> --- a/submodule.h\n> +++ b/submodule.h\n> @@ -97,7 +97,14 @@ int submodule_uses_gitfile(const char *path);\n> #define SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED (1<<2)\n> int bad_to_remove_submodule(const char *path, unsigned flags);\n> \n> +/*\n> + * Call add_submodule_odb() to add the submodule at the given path to a list.\n> + * When register_all_submodule_odb_as_alternates() is called, the object stores\n> + * of all submodules in that list will be added as alternates in\n> + * the_repository.\n> + */\n> int add_submodule_odb(const char *path);\n> +int register_all_submodule_odb_as_alternates(void);\n\nDoes it need to be public? Could this be a static in submodule.c\ninstead?\n\n> \n> /*\n> * Checks if there are submodule changes in a..b. If a is the null OID,\n> diff --git a/t/README b/t/README\n> index 9e70122302..8b67b4f00b 100644\n> --- a/t/README\n> +++ b/t/README\n> @@ -448,6 +448,16 @@ GIT_TEST_CHECKOUT_WORKERS= overrides the 'checkout.workers' setting\n> to and 'checkout.thresholdForParallelism' to 0, forcing the\n> execution of the parallel-checkout code.\n> \n> +GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=, when true, makes\n> +registering submodule ODBs as alternates a fatal action. Support for\n> +this environment variable can be removed once the migration to\n> +explicitly providing repositories when accessing submodule objects is\n> +complete (in which case we might want to replace this with a trace2\n> +call so that users can make it visible if accessing submodule objects\n> +without an explicit repository still happens) or needs to be abandoned\n> +for whatever reason (in which case the migrated codepaths still retain\n> +their performance benefits).\n> +\n> Naming Tests\n> ------------\n> \n> -- \n> 2.33.0.rc1.237.g0d66db33f3-goog\n> \n\nReviewed-by: Emily Shaffer \n"},{"id":"432524","messageId":"YRRC528p06ByShE5@google.com","threadId":"56248","inReplyTo":"31e9b914c4bb500a98fbb14674c93a0d8ece47a2.1628618950.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/7] grep: use submodule-ODB-as-alternate lazy-addition","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-08-11T21:36:39Z","receivedAt":"2021-08-11T21:36:50Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, Aug 10, 2021 at 11:28:40AM -0700, Jonathan Tan wrote:\n> \n> In the parent commit, Git was taught to add submodule ODBs as alternates\n> lazily, but grep does not use this because it computes the path to add\n> directly, not going through add_submodule_odb(). Add an equivalent to\n> add_submodule_odb() that takes the exact ODB path and teach grep to use\n> it.\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> builtin/grep.c | 2 +-\n> submodule.c | 5 +++++\n> submodule.h | 1 +\n> 3 files changed, 7 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 7d2f8e5adb..87bcb934a2 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -450,7 +450,7 @@ static int grep_submodule(struct grep_opt *opt,\n> \t * store is no longer global and instead is a member of the repository\n> \t * object.\n> \t */\n> -\tadd_to_alternates_memory(subrepo.objects->odb->path);\n> +\tadd_submodule_odb_by_path(subrepo.objects->odb->path);\n\nI had wondered whether we can entirely drop add_to_alternates_memory()\nbut I see on second reading that that's still used by\nregister_all_submodule_odb...(). I wonder if it can become static in\nsubmodule.c to prevent users from dodging the envvar+BUG()?\n\n> \tobj_read_unlock();\n> \n> \tmemcpy(&subopt, opt, sizeof(subopt));\n> diff --git a/submodule.c b/submodule.c\n> index 8fde90e906..8de1aecaeb 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -187,6 +187,11 @@ int add_submodule_odb(const char *path)\n> \treturn ret;\n> }\n> \n> +void add_submodule_odb_by_path(const char *path)\n> +{\n> +\tstring_list_insert(&added_submodule_odb_paths, xstrdup(path));\n> +}\n> +\n> int register_all_submodule_odb_as_alternates(void)\n> {\n> \tint i;\n> diff --git a/submodule.h b/submodule.h\n> index c252784bc2..17a06cc43b 100644\n> --- a/submodule.h\n> +++ b/submodule.h\n> @@ -104,6 +104,7 @@ int bad_to_remove_submodule(const char *path, unsigned flags);\n> * the_repository.\n> */\n> int add_submodule_odb(const char *path);\nCurious whether add_submodule_odb() can be reduced by calling\nadd_submodule_odb_by_path() internally.\n> +void add_submodule_odb_by_path(const char *path);\n> int register_all_submodule_odb_as_alternates(void);\n> \n> /*\n> -- \n> 2.33.0.rc1.237.g0d66db33f3-goog\n> \n\nThese are all pretty minor comments; with or without them,\n\nReviewed-by: Emily Shaffer \n"},{"id":"432526","messageId":"YRREPNA8GPyKdWBr@google.com","threadId":"56248","inReplyTo":"e5e6a0dc1ef59b2ab419816e463814d93115e7f6.1628618950.git.jonathantanmy@google.com","subject":"Re: [PATCH 3/7] grep: typesafe versions of grep_source_init","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-08-11T21:42:20Z","receivedAt":"2021-08-11T21:42:31Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, Aug 10, 2021 at 11:28:41AM -0700, Jonathan Tan wrote:\n> \n> grep_source_init() can create \"struct grep_source\" objects and,\n> depending on the value of the type passed, some void-pointer parameters have\n> different meanings. Because one of these types (GREP_SOURCE_OID) will\n> require an additional parameter in a subsequent patch, take the\n> opportunity to increase clarity and type safety by replacing this\n> function with individual functions for each type.\n> \n> Signed-off-by: Jonathan Tan \nLike Junio said, it is very neat.\n\nReviewed-by: Emily Shaffer \n"},{"id":"432527","messageId":"YRREyrAXBmxVoxRr@google.com","threadId":"56248","inReplyTo":"30ead880b38c5f572e609554b075ef81ff80ad87.1628618950.git.jonathantanmy@google.com","subject":"Re: [PATCH 4/7] grep: read submodule entry with explicit repo","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-08-11T21:44:42Z","receivedAt":"2021-08-11T21:44:59Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, Aug 10, 2021 at 11:28:42AM -0700, Jonathan Tan wrote:\n> \n> Replace an existing parse_object_or_die() call (which implicitly works\n> on the_repository) with a function call that allows a repository to be\n> passed in. There is no such direct equivalent to parse_object_or_die(),\n> but we only need the type of the object, so replace with\n> oid_object_info().\n\nAlways exciting to see less implicit use of the_repository ;)\n\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> builtin/grep.c | 10 +++++-----\n> 1 file changed, 5 insertions(+), 5 deletions(-)\n> \n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index e454335e9d..9e61c7c993 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -457,27 +457,27 @@ static int grep_submodule(struct grep_opt *opt,\n> \tsubopt.repo = &subrepo;\n> \n> \tif (oid) {\n> -\t\tstruct object *object;\n> +\t\tenum object_type object_type;\n> \t\tstruct tree_desc tree;\n> \t\tvoid *data;\n> \t\tunsigned long size;\n> \t\tstruct strbuf base = STRBUF_INIT;\n> \n> \t\tobj_read_lock();\n> -\t\tobject = parse_object_or_die(oid, NULL);\n> +\t\tobject_type = oid_object_info(&subrepo, oid, NULL);\n\nOne thing I wonder is whether we are missing out on some error\nconditions we used to get with parse_object_or_die() by using\noid_object_info() instead. Do we need to be more defensive in\ninvestigating 'oid' before calling that helper, now?\n\n> \t\tobj_read_unlock();\n> \t\tdata = read_object_with_reference(&subrepo,\n> -\t\t\t\t\t\t &object->oid, tree_type,\n> +\t\t\t\t\t\t oid, tree_type,\n\nAnd a handful of instances where we were using object->oid instead of\nthe oid we were already passed. Ok.\n\n> \t\t\t\t\t\t &size, NULL);\n> \t\tif (!data)\n> -\t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(&object->oid));\n> +\t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(oid));\n> \n> \t\tstrbuf_addstr(&base, filename);\n> \t\tstrbuf_addch(&base, '/');\n> \n> \t\tinit_tree_desc(&tree, data, size);\n> \t\thit = grep_tree(&subopt, pathspec, &tree, &base, base.len,\n> -\t\t\t\tobject->type == OBJ_COMMIT);\n> +\t\t\t\tobject_type == OBJ_COMMIT);\n\nAnd finally, using the type we got from oid_object_info instead. Ok.\n\n> \t\tstrbuf_release(&base);\n> \t\tfree(data);\n> \t} else {\n> -- \n> 2.33.0.rc1.237.g0d66db33f3-goog\n> \n\nLGTM. Thanks.\n\nReviewed-by: Emily Shaffer \n"},{"id":"432528","messageId":"YRRGKKYYQiYTxtJ5@google.com","threadId":"56248","inReplyTo":"f62608e88f125096c1f236a47e3ee670104c7b78.1628618950.git.jonathantanmy@google.com","subject":"Re: [PATCH 5/7] grep: allocate subrepos on heap","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-08-11T21:50:32Z","receivedAt":"2021-08-11T21:50:51Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, Aug 10, 2021 at 11:28:43AM -0700, Jonathan Tan wrote:\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 9e61c7c993..5a40e18e47 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -65,6 +65,9 @@ static int todo_done;\n> /* Has all work items been added? */\n> static int all_work_added;\n> \n> +static struct repository **repos_to_free;\n> +static size_t repos_to_free_nr, repos_to_free_alloc;\n\nOne thing I was curious about was whether it would make more sense to\nuse an existing utility library in Git to handle this. But I think we\nkind of are, since ALLOC_GROW is used, so this is idiomatic for Git\nproject. Ok, fine, I guess this is life at C ;)\n\n> +\n> /* This lock protects all the variables above. */\n> static pthread_mutex_t grep_mutex;\n> \n> @@ -168,6 +171,17 @@ static void work_done(struct work_item *w)\n> \tgrep_unlock();\n> }\n> \n> +static void free_repos(void)\n> +{\n> +\tint i;\n> +\n> +\tfor (i = 0; i < repos_to_free_nr; i++) {\n> +\t\trepo_clear(repos_to_free[i]);\n> +\t\tfree(repos_to_free[i]);\n> +\t}\n> +\tfree(repos_to_free);\n> +}\n\nShould repos_to_free_nr be reset here? I guess it doesn't make sense to,\nsince we'd be trying to use-after-free the initial repos_to_free head\npointer too if we wanted to reuse this array.\n\n> +\n> static void *run(void *arg)\n> {\n> \tint hit = 0;\n> @@ -415,19 +429,24 @@ static int grep_submodule(struct grep_opt *opt,\n> \t\t\t const struct object_id *oid,\n> \t\t\t const char *filename, const char *path, int cached)\n> {\n> -\tstruct repository subrepo;\n> +\tstruct repository *subrepo;\n> \tstruct repository *superproject = opt->repo;\n> \tconst struct submodule *sub;\n> \tstruct grep_opt subopt;\n> -\tint hit;\n> +\tint hit = 0;\n> \n> \tsub = submodule_from_path(superproject, null_oid(), path);\n> \n> \tif (!is_submodule_active(superproject, path))\n> \t\treturn 0;\n> \n> -\tif (repo_submodule_init(&subrepo, superproject, sub))\n> +\tsubrepo = xmalloc(sizeof(*subrepo));\n> +\tif (repo_submodule_init(subrepo, superproject, sub)) {\n> +\t\tfree(subrepo);\n> \t\treturn 0;\n> +\t}\n> +\tALLOC_GROW(repos_to_free, repos_to_free_nr + 1, repos_to_free_alloc);\n> +\trepos_to_free[repos_to_free_nr++] = subrepo;\n\nIs this the only place we add to the repos_to_free array? It looks like\nyes, so I guess this doesn't need a helper.\n\n> \n> \t/*\n> \t * NEEDSWORK: repo_read_gitmodules() might call\n> @@ -438,7 +457,7 @@ static int grep_submodule(struct grep_opt *opt,\n> \t * subrepo's odbs to the in-memory alternates list.\n> \t */\n> \tobj_read_lock();\n> -\trepo_read_gitmodules(&subrepo, 0);\n> +\trepo_read_gitmodules(subrepo, 0);\n> \n> \t/*\n> \t * NEEDSWORK: This adds the submodule's object directory to the list of\n> @@ -450,11 +469,11 @@ static int grep_submodule(struct grep_opt *opt,\n> \t * store is no longer global and instead is a member of the repository\n> \t * object.\n> \t */\n> -\tadd_submodule_odb_by_path(subrepo.objects->odb->path);\n> +\tadd_submodule_odb_by_path(subrepo->objects->odb->path);\n> \tobj_read_unlock();\n> \n> \tmemcpy(&subopt, opt, sizeof(subopt));\n> -\tsubopt.repo = &subrepo;\n> +\tsubopt.repo = subrepo;\n> \n> \tif (oid) {\n> \t\tenum object_type object_type;\n> @@ -464,9 +483,9 @@ static int grep_submodule(struct grep_opt *opt,\n> \t\tstruct strbuf base = STRBUF_INIT;\n> \n> \t\tobj_read_lock();\n> -\t\tobject_type = oid_object_info(&subrepo, oid, NULL);\n> +\t\tobject_type = oid_object_info(subrepo, oid, NULL);\n> \t\tobj_read_unlock();\n> -\t\tdata = read_object_with_reference(&subrepo,\n> +\t\tdata = read_object_with_reference(subrepo,\n> \t\t\t\t\t\t oid, tree_type,\n> \t\t\t\t\t\t &size, NULL);\n> \t\tif (!data)\n> @@ -484,7 +503,6 @@ static int grep_submodule(struct grep_opt *opt,\n> \t\thit = grep_cache(&subopt, pathspec, cached);\n> \t}\n> \n> -\trepo_clear(&subrepo);\n> \treturn hit;\n> }\n> \n> @@ -1182,5 +1200,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \t\trun_pager(&opt, prefix);\n> \tclear_pathspec(&pathspec);\n> \tfree_grep_patterns(&opt);\n> +\tfree_repos();\n> \treturn !hit;\n> }\n\nThe rest of the diff seems to be a pretty clear object-becomes-reference\nconversion, so...\n\nReviewed-by: Emily Shaffer \n"},{"id":"432529","messageId":"YRRGrKztksYi8of2@google.com","threadId":"56248","inReplyTo":"b2df2455871dc5c94ec804e9c0ce935c19da335f.1628618950.git.jonathantanmy@google.com","subject":"Re: [PATCH 6/7] grep: add repository to OID grep sources","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-08-11T21:52:44Z","receivedAt":"2021-08-11T21:52:55Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, Aug 10, 2021 at 11:28:44AM -0700, Jonathan Tan wrote:\n> \n> Record the repository whenever an OID grep source is created, and teach\n> the worker threads to explicitly provide the repository when accessing\n> objects.\n\nSince this extra information is not used by this series at all, does it\nmake sense to only include this patch with the series covering the rest\nof your partial-clone-with-submodules work?\n\n - Emily\n"},{"id":"432530","messageId":"YRRHO0h+eDH8oxGX@google.com","threadId":"56248","inReplyTo":"f1fc89894b8832ab0f64f301b1621ae15654e08c.1628618950.git.jonathantanmy@google.com","subject":"Re: [PATCH 7/7] t7814: show lack of alternate ODB-adding","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-08-11T21:55:07Z","receivedAt":"2021-08-11T21:55:17Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, Aug 10, 2021 at 11:28:45AM -0700, Jonathan Tan wrote:\n> \n> The previous patches have made \"git grep\" no longer need to add\n> submodule ODBs as alternates, at least for the code paths tested in\n> t7814. Demonstrate this by making adding a submodule ODB as an alternate\n> fatal in this test.\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> t/t7814-grep-recurse-submodules.sh | 3 +++\n> 1 file changed, 3 insertions(+)\n> \n> diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\n> index 828cb3ba58..3172f5b936 100755\n> --- a/t/t7814-grep-recurse-submodules.sh\n> +++ b/t/t7814-grep-recurse-submodules.sh\n> @@ -8,6 +8,9 @@ submodules.\n> \n> . ./test-lib.sh\n> \n> +GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1\n> +export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB\n> +\n> test_expect_success 'setup directory structure and submodule' '\n> \techo \"(1|2)d(3|4)\" >a &&\n> \tmkdir b &&\n> -- \n> 2.33.0.rc1.237.g0d66db33f3-goog\n> \n\nThis proof seems pretty handy, assuming nobody else is directly calling\nadd_to_alternates_memory() (and therefore skipping the envvar check). I\nwould feel slightly more convinced if that function was file-static\nsomewhere, but I am not familiar with the problem space enough to say\nwhether that's possible.\n\nThis patch by itself, though:\nReviewed-by: Emily Shaffer \n\n - Emily\n"},{"id":"432534","messageId":"CAHd-oW7NKNeujverFiRUR2p1nfOpjrv1xDrsvTVa1Yw5jC1D0w@mail.gmail.com","threadId":"56248","inReplyTo":"YRRHO0h+eDH8oxGX@google.com","subject":"Re: [PATCH 7/7] t7814: show lack of alternate ODB-adding","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-08-11T22:22:49Z","receivedAt":"2021-08-11T22:23:03Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Wed, Aug 11, 2021 at 6:55 PM Emily Shaffer wrote:\n>\n> On Tue, Aug 10, 2021 at 11:28:45AM -0700, Jonathan Tan wrote:\n> >\n> > The previous patches have made \"git grep\" no longer need to add\n> > submodule ODBs as alternates, at least for the code paths tested in\n> > t7814. Demonstrate this by making adding a submodule ODB as an alternate\n> > fatal in this test.\n> >\n> > Signed-off-by: Jonathan Tan \n> > ---\n> > t/t7814-grep-recurse-submodules.sh | 3 +++\n> > 1 file changed, 3 insertions(+)\n> >\n> > diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\n> > index 828cb3ba58..3172f5b936 100755\n> > --- a/t/t7814-grep-recurse-submodules.sh\n> > +++ b/t/t7814-grep-recurse-submodules.sh\n> > @@ -8,6 +8,9 @@ submodules.\n> >\n> > . ./test-lib.sh\n> >\n> > +GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1\n> > +export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB\n> > +\n> > test_expect_success 'setup directory structure and submodule' '\n> > echo \"(1|2)d(3|4)\" >a &&\n> > mkdir b &&\n> > --\n> > 2.33.0.rc1.237.g0d66db33f3-goog\n> >\n>\n> This proof seems pretty handy, assuming nobody else is directly calling\n> add_to_alternates_memory() (and therefore skipping the envvar check).\n\nHmm, there is at least one call chain in grep which might end up\ncalling add_to_alternates_memory() directly (although it only seems to\nhappen on a very specific case):\n\n grep_submodule > repo_read_gitmodules >\n config_from_gitmodules > add_to_alternates_memory\n\nWe can check that with the following:\n\ngit init A\ngit init A/B\ngit init A/B/C\n\necho f >A/B/C/f\ngit -C A/B/C add f\ngit -C A/B/C commit -m f\n\ngit -C A/B submodule add ./C\ngit -C A/B commit -m C\n\ngit -C A submodule add ./B\ngit -C A commit -m B\n\nrm B/.gitmodules\n\ngdb -ex 'break add_to_alternates_memory' -ex 'run' --args \\\n git grep --recurse-submodules .\n"},{"id":"432536","messageId":"CAHd-oW4tLJf1VxWDOkzKtJCV9X_rqfWPg_yOONYo7S8O1kCjGQ@mail.gmail.com","threadId":"56248","inReplyTo":"e5e6a0dc1ef59b2ab419816e463814d93115e7f6.1628618950.git.jonathantanmy@google.com","subject":"Re: [PATCH 3/7] grep: typesafe versions of grep_source_init","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-08-11T22:45:04Z","receivedAt":"2021-08-11T22:45:18Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Tue, Aug 10, 2021 at 3:29 PM Jonathan Tan wrote:\n>\n> diff --git a/grep.c b/grep.c\n> index 424a39591b..ba3711dc56 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -1830,7 +1830,7 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n> struct grep_source gs;\n> int r;\n>\n> - grep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL);\n> + grep_source_init_buf(&gs);\n> gs.buf = buf;\n> gs.size = size;\n\nSmall nit: perhaps `grep_source_init_buf()` could take `buf` and\n`size` too, so that all the fields get initialized by the same\nfunction.\n"},{"id":"432537","messageId":"YRRT4I3MPoZXJZEl@google.com","threadId":"56248","inReplyTo":"cover.1628618950.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/7] In grep, no adding submodule ODB as alternates","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2021-08-11T22:49:04Z","receivedAt":"2021-08-11T22:49:14Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2021.08.10 11:28, Jonathan Tan wrote:\n> This patch series removes the need to add submodule ODBs as alternates\n> in all codepaths covered by t7814. I believe that that is also all the\n> codepaths covered by \"git grep\", but if it isn't, the uncovered\n> codepaths will still work - they will just not benefit from the\n> performance improvements.\n> \n> In doing this work of migrating away from adding submodule ODBs as\n> alternates, I'm mainly motivated by the possibility of adding partial\n> clone submodule support, but this has benefits even for those who do not\n> use partial clones, as described in the documentation in patch 1.\n> \n> To reviewers: you can cherry-pick the last patch onto one of the earlier\n> ones to observe what happens when the code still accesses a submodule\n> object as if it were in the_repository.\n\nThanks for this series. I am very excited to see steps toward enabling\npartial clones for submodules! And having fewer implicit references to\nthe_repository is great as well.\n\nI don't have any comments that other reviewers haven't already\nmentioned, so I'll just add:\n\nReviewed-by: Josh Steadmon \n"},{"id":"432538","messageId":"YRRYOmwW9yKhvldn@satellite","threadId":"56248","inReplyTo":"YRREPNA8GPyKdWBr@google.com","subject":"Re: [PATCH 3/7] grep: typesafe versions of grep_source_init","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2021-08-11T23:07:38Z","receivedAt":"2021-08-11T23:07:44Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On Wed, Aug 11, 2021 at 02:42:20PM -0700, Emily Shaffer wrote:\n> On Tue, Aug 10, 2021 at 11:28:41AM -0700, Jonathan Tan wrote:\n> > \n> > grep_source_init() can create \"struct grep_source\" objects and,\n> > depending on the value of the type passed, some void-pointer parameters have\n> > different meanings. Because one of these types (GREP_SOURCE_OID) will\n> > require an additional parameter in a subsequent patch, take the\n> > opportunity to increase clarity and type safety by replacing this\n> > function with individual functions for each type.\n> > \n> > Signed-off-by: Jonathan Tan \n> Like Junio said, it is very neat.\n\n[Sorry for piggy-backing, I have already deleted the original mail :( ]\n\nJust a quick note: grep_source_init_buf() is only called from\ngrep.c:1833, before its definition at grep.c:1869, so it could be marked\nas static (as things stand). Do you anticipate any future callers from\noutside of grep.c? (after removing the declaration from grep.h, you\nwould need to add a forward declaration or, better, move the definition\nto before the call (or the call (and grep_buffer()) after the definition)).\n\nATB,\nRamsay Jones\n\n\n"},{"id":"432544","messageId":"CAHd-oW4ntWB7KOKxhZU4wM-Xg9MARVN2EmpxEg-rEfc4i_SUnw@mail.gmail.com","threadId":"56248","inReplyTo":"b2df2455871dc5c94ec804e9c0ce935c19da335f.1628618950.git.jonathantanmy@google.com","subject":"Re: [PATCH 6/7] grep: add repository to OID grep sources","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-08-11T23:28:54Z","receivedAt":"2021-08-11T23:29:08Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Tue, Aug 10, 2021 at 3:29 PM Jonathan Tan wrote:\n>\n> Record the repository whenever an OID grep source is created, and teach\n> the worker threads to explicitly provide the repository when accessing\n> objects.\n\nThis patch fixes the second NEEDSWORK comment at grep_submodule(),\nright? Maybe this comment could be replaced with a mention that\nadd_submodule_odb_by_path() is called for testing purposes, and it\nshould no longer produce a real addition to the alternates list?\n\n> diff --git a/grep.h b/grep.h\n> index f4a3090f1c..c5234f9b38 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -187,6 +187,7 @@ struct grep_source {\n> GREP_SOURCE_BUF,\n> } type;\n> void *identifier;\n> + struct repository *repo; /* if GREP_SOURCE_OID */\n\nHmm, the grep threads now have two `struct repository` references, one\nin `struct grep_source` and one in `struct grep_opt` (see\nbuiltin/grep.c:run()). The one from `struct grep_opt` will always be\n`the_repository` (in the worker threads). I wonder if, in the long\nrun, we could instruct the worker threads to use the new reference\nfrom `struct grep_source` throughout grep.c, and perhaps even remove\nthe one from `struct grep_opt`.\n\nThis would solve the issue I mentioned in [1], about git grep\ncurrently ignoring the textconv attributes from submodules, and\ninstead applying the ones from the superproject in the submodules'\nfiles. (Here there is an example of this bug: [2] .)\n\nIt would also allow grep to use the textconv cache from each\nsubmodule, instead of saving/reading everything from the\nsuperproject's textconv cache.\n\nWhile this doesn't happen, though, could we perhaps add a comment\nsomewhere to avoid any confusion regarding the two different\nrepository pointers that the worker threads hold?\n\n[1]: https://lore.kernel.org/git/CAHd-oW5iEQarYVxEXoTG-ua2zdoybTrSjCBKtO0YT292fm0NQQ@mail.gmail.com/\n[2]: https://gitlab.com/-/snippets/1896951\n\n> char *buf;\n> unsigned long size;\n> @@ -198,7 +199,8 @@ struct grep_source {\n> void grep_source_init_file(struct grep_source *gs, const char *name,\n> const char *path);\n> void grep_source_init_oid(struct grep_source *gs, const char *name,\n> - const char *path, const struct object_id *oid);\n> + const char *path, const struct object_id *oid,\n> + struct repository *repo);\n> void grep_source_init_buf(struct grep_source *gs);\n> void grep_source_clear_data(struct grep_source *gs);\n> void grep_source_clear(struct grep_source *gs);\n> --\n> 2.33.0.rc1.237.g0d66db33f3-goog\n>\n"},{"id":"432584","messageId":"xmqqwnoq6217.fsf@gitster.g","threadId":"56248","inReplyTo":"CAHd-oW4tLJf1VxWDOkzKtJCV9X_rqfWPg_yOONYo7S8O1kCjGQ@mail.gmail.com","subject":"Re: [PATCH 3/7] grep: typesafe versions of grep_source_init","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-12T16:49:56Z","receivedAt":"2021-08-12T16:50:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares Bernardino writes:\n\n> On Tue, Aug 10, 2021 at 3:29 PM Jonathan Tan wrote:\n>>\n>> diff --git a/grep.c b/grep.c\n>> index 424a39591b..ba3711dc56 100644\n>> --- a/grep.c\n>> +++ b/grep.c\n>> @@ -1830,7 +1830,7 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n>> struct grep_source gs;\n>> int r;\n>>\n>> - grep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL);\n>> + grep_source_init_buf(&gs);\n>> gs.buf = buf;\n>> gs.size = size;\n>\n> Small nit: perhaps `grep_source_init_buf()` could take `buf` and\n> `size` too, so that all the fields get initialized by the same\n> function.\n\nSounds sensible. Thanks.\n"},{"id":"432681","messageId":"20210813162358.3210807-1-jonathantanmy@google.com","threadId":"56248","inReplyTo":"YRRCRHTgXU2x+s0l@google.com","subject":"Re: [PATCH 1/7] submodule: lazily add submodule ODBs as alternates","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T16:23:58Z","receivedAt":"2021-08-13T16:24:04Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > +int register_all_submodule_odb_as_alternates(void);\n> \n> Does it need to be public? Could this be a static in submodule.c\n> instead?\n\nThanks for taking a look at this patch series.\n\nTo answer this question: no - in this patch, I need to use it from\nobject-file.c to actually register the submodule ODBs as alternates when\nwe try to read an object that is not in the superproject.\n"},{"id":"432682","messageId":"20210813163111.3212129-1-jonathantanmy@google.com","threadId":"56248","inReplyTo":"YRRC528p06ByShE5@google.com","subject":"Re: [PATCH 2/7] grep: use submodule-ODB-as-alternate lazy-addition","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T16:31:11Z","receivedAt":"2021-08-13T16:31:22Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> I had wondered whether we can entirely drop add_to_alternates_memory()\n> but I see on second reading that that's still used by\n> register_all_submodule_odb...(). I wonder if it can become static in\n> submodule.c to prevent users from dodging the envvar+BUG()?\n\nYou mean make add_to_alternates_memory() static? I'm not sure how we can\ndo that - alternates is a concern that extends beyond submodules.\n\n> Curious whether add_submodule_odb() can be reduced by calling\n> add_submodule_odb_by_path() internally.\n\nThat's a reasonable idea, but I don't think it works in this case - in\nparticular, add_submodule_odb_by_path() dups its argument whereas\nadd_submodule_odb() already has an allocated string that it is prepared\nto give up ownership of. Also, add_submodule_odb_by_path() is only one\nline, so it won't save us much.\n"},{"id":"432683","messageId":"20210813163241.3213428-1-jonathantanmy@google.com","threadId":"56248","inReplyTo":"YRRYOmwW9yKhvldn@satellite","subject":"Re: [PATCH 3/7] grep: typesafe versions of grep_source_init","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T16:32:41Z","receivedAt":"2021-08-13T16:32:46Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Just a quick note: grep_source_init_buf() is only called from\n> grep.c:1833, before its definition at grep.c:1869, so it could be marked\n> as static (as things stand). Do you anticipate any future callers from\n> outside of grep.c? (after removing the declaration from grep.h, you\n> would need to add a forward declaration or, better, move the definition\n> to before the call (or the call (and grep_buffer()) after the definition)).\n\nNo, I do not - thanks for the note, and I'll make it static in the next\nreroll.\n"},{"id":"432684","messageId":"20210813163318.3213751-1-jonathantanmy@google.com","threadId":"56248","inReplyTo":"xmqqwnoq6217.fsf@gitster.g","subject":"Re: [PATCH 3/7] grep: typesafe versions of grep_source_init","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T16:33:18Z","receivedAt":"2021-08-13T16:33:25Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > Small nit: perhaps `grep_source_init_buf()` could take `buf` and\n> > `size` too, so that all the fields get initialized by the same\n> > function.\n> \n> Sounds sensible. Thanks.\n\nMakes sense - I'll do this.\n"},{"id":"432685","messageId":"20210813163901.3214789-1-jonathantanmy@google.com","threadId":"56248","inReplyTo":"YRREyrAXBmxVoxRr@google.com","subject":"Re: [PATCH 4/7] grep: read submodule entry with explicit repo","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T16:39:01Z","receivedAt":"2021-08-13T16:39:05Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > -\t\tobject = parse_object_or_die(oid, NULL);\n> > +\t\tobject_type = oid_object_info(&subrepo, oid, NULL);\n> \n> One thing I wonder is whether we are missing out on some error\n> conditions we used to get with parse_object_or_die() by using\n> oid_object_info() instead. Do we need to be more defensive in\n> investigating 'oid' before calling that helper, now?\n\nFor the purposes of grep, I don't think that this matters - we just want\nto find the ultimate tree that this object represents.\n"},{"id":"432686","messageId":"20210813164247.3218774-1-jonathantanmy@google.com","threadId":"56248","inReplyTo":"YRRGKKYYQiYTxtJ5@google.com","subject":"Re: [PATCH 5/7] grep: allocate subrepos on heap","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T16:42:47Z","receivedAt":"2021-08-13T16:42:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > +static struct repository **repos_to_free;\n> > +static size_t repos_to_free_nr, repos_to_free_alloc;\n> \n> One thing I was curious about was whether it would make more sense to\n> use an existing utility library in Git to handle this. But I think we\n> kind of are, since ALLOC_GROW is used, so this is idiomatic for Git\n> project. Ok, fine, I guess this is life at C ;)\n\nYeah, this *is* the utility library :-)\n\n> > +static void free_repos(void)\n> > +{\n> > +\tint i;\n> > +\n> > +\tfor (i = 0; i < repos_to_free_nr; i++) {\n> > +\t\trepo_clear(repos_to_free[i]);\n> > +\t\tfree(repos_to_free[i]);\n> > +\t}\n> > +\tfree(repos_to_free);\n> > +}\n> \n> Should repos_to_free_nr be reset here? I guess it doesn't make sense to,\n> since we'd be trying to use-after-free the initial repos_to_free head\n> pointer too if we wanted to reuse this array.\n\nHmm...I guess that if I'm going through the trouble of clearing the\nrepos instead of just letting all the memory be collected at the end of\na process, I should also reset _nr and _alloc. I'll make that change.\n"},{"id":"432687","messageId":"20210813164410.3219275-1-jonathantanmy@google.com","threadId":"56248","inReplyTo":"YRRGrKztksYi8of2@google.com","subject":"Re: [PATCH 6/7] grep: add repository to OID grep sources","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T16:44:10Z","receivedAt":"2021-08-13T16:44:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Tue, Aug 10, 2021 at 11:28:44AM -0700, Jonathan Tan wrote:\n> > \n> > Record the repository whenever an OID grep source is created, and teach\n> > the worker threads to explicitly provide the repository when accessing\n> > objects.\n> \n> Since this extra information is not used by this series at all, does it\n> make sense to only include this patch with the series covering the rest\n> of your partial-clone-with-submodules work?\n\nIt is used by this series - it prevents an instance in which the\nsubmodule ODBs would be lazily registered as alternates. I'll add a note\nin the commit message to this effect.\n"},{"id":"432688","messageId":"20210813164734.3219965-1-jonathantanmy@google.com","threadId":"56248","inReplyTo":"CAHd-oW4ntWB7KOKxhZU4wM-Xg9MARVN2EmpxEg-rEfc4i_SUnw@mail.gmail.com","subject":"Re: [PATCH 6/7] grep: add repository to OID grep sources","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T16:47:34Z","receivedAt":"2021-08-13T16:47:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Tue, Aug 10, 2021 at 3:29 PM Jonathan Tan wrote:\n> >\n> > Record the repository whenever an OID grep source is created, and teach\n> > the worker threads to explicitly provide the repository when accessing\n> > objects.\n> \n> This patch fixes the second NEEDSWORK comment at grep_submodule(),\n> right? Maybe this comment could be replaced with a mention that\n> add_submodule_odb_by_path() is called for testing purposes, and it\n> should no longer produce a real addition to the alternates list?\n\nGood catch. I'll update the comment.\n\n> > diff --git a/grep.h b/grep.h\n> > index f4a3090f1c..c5234f9b38 100644\n> > --- a/grep.h\n> > +++ b/grep.h\n> > @@ -187,6 +187,7 @@ struct grep_source {\n> > GREP_SOURCE_BUF,\n> > } type;\n> > void *identifier;\n> > + struct repository *repo; /* if GREP_SOURCE_OID */\n> \n> Hmm, the grep threads now have two `struct repository` references, one\n> in `struct grep_source` and one in `struct grep_opt` (see\n> builtin/grep.c:run()). The one from `struct grep_opt` will always be\n> `the_repository` (in the worker threads). I wonder if, in the long\n> run, we could instruct the worker threads to use the new reference\n> from `struct grep_source` throughout grep.c, and perhaps even remove\n> the one from `struct grep_opt`.\n> \n> This would solve the issue I mentioned in [1], about git grep\n> currently ignoring the textconv attributes from submodules, and\n> instead applying the ones from the superproject in the submodules'\n> files. (Here there is an example of this bug: [2] .)\n> \n> It would also allow grep to use the textconv cache from each\n> submodule, instead of saving/reading everything from the\n> superproject's textconv cache.\n> \n> While this doesn't happen, though, could we perhaps add a comment\n> somewhere to avoid any confusion regarding the two different\n> repository pointers that the worker threads hold?\n> \n> [1]: https://lore.kernel.org/git/CAHd-oW5iEQarYVxEXoTG-ua2zdoybTrSjCBKtO0YT292fm0NQQ@mail.gmail.com/\n> [2]: https://gitlab.com/-/snippets/1896951\n\nAh...another good catch. Thanks also for a link to your email - I'll\nmention it when I add the comment you suggested.\n"},{"id":"432689","messageId":"20210813165045.3220845-1-jonathantanmy@google.com","threadId":"56248","inReplyTo":"CAHd-oW7NKNeujverFiRUR2p1nfOpjrv1xDrsvTVa1Yw5jC1D0w@mail.gmail.com","subject":"Re: [PATCH 7/7] t7814: show lack of alternate ODB-adding","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T16:50:45Z","receivedAt":"2021-08-13T16:50:52Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Hmm, there is at least one call chain in grep which might end up\n> calling add_to_alternates_memory() directly (although it only seems to\n> happen on a very specific case):\n> \n> grep_submodule > repo_read_gitmodules >\n> config_from_gitmodules > add_to_alternates_memory\n> \n> We can check that with the following:\n\n[snip reproduction recipe]\n\nThanks - it looks like my patch set is incomplete then. I'll make\nconfig_from_gitmodules() use my new function and if the existing grep\ntest cases don't cover your reproduction recipe, I'll add yours in.\n"},{"id":"432690","messageId":"20210813165343.3225573-1-jonathantanmy@google.com","threadId":"56248","inReplyTo":"xmqqtujxc8a3.fsf@gitster.g","subject":"Re: [PATCH 1/7] submodule: lazily add submodule ODBs as alternates","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T16:53:43Z","receivedAt":"2021-08-13T16:53:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > +static struct string_list added_submodule_odb_paths = STRING_LIST_INIT_NODUP;\n> > +\n> \n> I see 2/7 allows callers to add paths for submodule odb to this\n> list.\n\nYes, and as you said below, add_submodule_odb() has been modified to add\nto this list too.\n\n> The \"pretend as if objects in submodule odb are locally available\",\n> even though it may have been useful, was an ugly hack invented\n> before we started adding the \"access more than one repository at the\n> time with the 'repo' arg\" extended API. It is pleasing to see this\n> step shows an approach to incrementally migrate the users of the\n> hack.\n\nThanks.\n"},{"id":"432708","messageId":"cover.1628888668.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1628618950.git.jonathantanmy@google.com","subject":"[PATCH v2 0/8] In grep, no adding submodule ODB as alternates","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T21:05:15Z","receivedAt":"2021-08-13T21:05:29Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks everyone for your reviews. I believe I've addressed all of them.\n\nThe main change is that I've also made a change in submodule-config to\navoid registering submodule ODBs as alternates (thanks, Matheus, for\nnoticing this).\n\nJonathan Tan (8):\n submodule: lazily add submodule ODBs as alternates\n grep: use submodule-ODB-as-alternate lazy-addition\n grep: typesafe versions of grep_source_init\n grep: read submodule entry with explicit repo\n grep: allocate subrepos on heap\n grep: add repository to OID grep sources\n submodule-config: pass repo upon blob config read\n t7814: show lack of alternate ODB-adding\n\n builtin/grep.c | 64 +++++++++++++++++++-----------\n config.c | 18 ++++++---\n config.h | 3 ++\n grep.c | 51 +++++++++++++++---------\n grep.h | 18 +++++++--\n object-file.c | 5 +++\n submodule-config.c | 5 ++-\n submodule.c | 25 +++++++++++-\n submodule.h | 8 ++++\n t/README | 10 +++++\n t/t7814-grep-recurse-submodules.sh | 3 ++\n 11 files changed, 156 insertions(+), 54 deletions(-)\n\nRange-diff against v1:\n1: 5994a517e8 = 1: 5994a517e8 submodule: lazily add submodule ODBs as alternates\n2: 31e9b914c4 = 2: 31e9b914c4 grep: use submodule-ODB-as-alternate lazy-addition\n3: e5e6a0dc1e ! 3: aa3f1f3c89 grep: typesafe versions of grep_source_init\n @@ builtin/grep.c: static int grep_file(struct grep_opt *opt, const char *filename)\n \tif (num_threads > 1) {\n \n ## grep.c ##\n -@@ grep.c: int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n +@@ grep.c: int grep_source(struct grep_opt *opt, struct grep_source *gs)\n + \treturn grep_source_1(opt, gs, 0);\n + }\n + \n ++static void grep_source_init_buf(struct grep_source *gs, char *buf,\n ++\t\t\t\t unsigned long size)\n ++{\n ++\tgs->type = GREP_SOURCE_BUF;\n ++\tgs->name = NULL;\n ++\tgs->path = NULL;\n ++\tgs->buf = buf;\n ++\tgs->size = size;\n ++\tgs->driver = NULL;\n ++\tgs->identifier = NULL;\n ++}\n ++\n + int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n + {\n \tstruct grep_source gs;\n \tint r;\n \n -\tgrep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL);\n -+\tgrep_source_init_buf(&gs);\n - \tgs.buf = buf;\n - \tgs.size = size;\n +-\tgs.buf = buf;\n +-\tgs.size = size;\n ++\tgrep_source_init_buf(&gs, buf, size);\n + \n + \tr = grep_source(opt, &gs);\n \n @@ grep.c: int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n \treturn r;\n @@ grep.c: int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n +\tgs->size = 0;\n +\tgs->driver = NULL;\n +\tgs->identifier = oiddup(oid);\n -+}\n -+\n -+void grep_source_init_buf(struct grep_source *gs)\n -+{\n -+\tgs->type = GREP_SOURCE_BUF;\n -+\tgs->name = NULL;\n -+\tgs->path = NULL;\n -+\tgs->buf = NULL;\n -+\tgs->size = 0;\n -+\tgs->driver = NULL;\n -+\tgs->identifier = NULL;\n }\n \n void grep_source_clear(struct grep_source *gs)\n @@ grep.h: struct grep_source {\n +\t\t\t const char *path);\n +void grep_source_init_oid(struct grep_source *gs, const char *name,\n +\t\t\t const char *path, const struct object_id *oid);\n -+void grep_source_init_buf(struct grep_source *gs);\n void grep_source_clear_data(struct grep_source *gs);\n void grep_source_clear(struct grep_source *gs);\n void grep_source_load_driver(struct grep_source *gs,\n4: 30ead880b3 = 4: 050deacfb7 grep: read submodule entry with explicit repo\n5: f62608e88f ! 5: 3f24815224 grep: allocate subrepos on heap\n @@ builtin/grep.c: static void work_done(struct work_item *w)\n +\t\tfree(repos_to_free[i]);\n +\t}\n +\tfree(repos_to_free);\n ++\trepos_to_free_nr = 0;\n ++\trepos_to_free_alloc = 0;\n +}\n +\n static void *run(void *arg)\n6: b2df245587 ! 6: 50c69a988b grep: add repository to OID grep sources\n @@ builtin/grep.c: static int grep_oid(struct grep_opt *opt, const struct object_id\n \tstrbuf_release(&pathbuf);\n \n \tif (num_threads > 1) {\n +@@ builtin/grep.c: static int grep_submodule(struct grep_opt *opt,\n + \trepo_read_gitmodules(subrepo, 0);\n + \n + \t/*\n +-\t * NEEDSWORK: This adds the submodule's object directory to the list of\n +-\t * alternates for the single in-memory object store. This has some bad\n +-\t * consequences for memory (processed objects will never be freed) and\n +-\t * performance (this increases the number of pack files git has to pay\n +-\t * attention to, to the sum of the number of pack files in all the\n +-\t * repositories processed so far). This can be removed once the object\n +-\t * store is no longer global and instead is a member of the repository\n +-\t * object.\n ++\t * All code paths tested by test code no longer need submodule ODBs to\n ++\t * be added as alternates, but add it to the list just in case.\n ++\t * Submodule ODBs added through add_submodule_odb_by_path() will be\n ++\t * lazily registered as alternates when needed (and except in an\n ++\t * unexpected code interaction, it won't be needed).\n + \t */\n + \tadd_submodule_odb_by_path(subrepo->objects->odb->path);\n + \tobj_read_unlock();\n \n ## grep.c ##\n @@ grep.c: void grep_source_init_file(struct grep_source *gs, const char *name,\n @@ grep.c: void grep_source_init_oid(struct grep_source *gs, const char *name,\n +\tgs->repo = repo;\n }\n \n - void grep_source_init_buf(struct grep_source *gs)\n + void grep_source_clear(struct grep_source *gs)\n @@ grep.c: static int grep_source_load_oid(struct grep_source *gs)\n {\n \tenum object_type type;\n @@ grep.c: static int grep_source_load_oid(struct grep_source *gs)\n \t\t\t gs->name,\n \n ## grep.h ##\n +@@ grep.h: struct grep_opt {\n + \tstruct grep_pat *header_list;\n + \tstruct grep_pat **header_tail;\n + \tstruct grep_expr *pattern_expression;\n ++\n ++\t/*\n ++\t * NEEDSWORK: See if we can remove this field, because the repository\n ++\t * should probably be per-source, not per-repo. This is potentially the\n ++\t * cause of at least one bug - \"git grep\" ignoring the textconv\n ++\t * attributes from submodules. See [1] for more information.\n ++\t * [1] https://lore.kernel.org/git/CAHd-oW5iEQarYVxEXoTG-ua2zdoybTrSjCBKtO0YT292fm0NQQ@mail.gmail.com/\n ++\t */\n + \tstruct repository *repo;\n ++\n + \tconst char *prefix;\n + \tint prefix_length;\n + \tregex_t regexp;\n @@ grep.h: struct grep_source {\n \t\tGREP_SOURCE_BUF,\n \t} type;\n @@ grep.h: struct grep_source {\n -\t\t\t const char *path, const struct object_id *oid);\n +\t\t\t const char *path, const struct object_id *oid,\n +\t\t\t struct repository *repo);\n - void grep_source_init_buf(struct grep_source *gs);\n void grep_source_clear_data(struct grep_source *gs);\n void grep_source_clear(struct grep_source *gs);\n + void grep_source_load_driver(struct grep_source *gs,\n-: ---------- > 7: 94db10a4e5 submodule-config: pass repo upon blob config read\n7: f1fc89894b = 8: 4a51fcfb77 t7814: show lack of alternate ODB-adding\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432709","messageId":"31e9b914c4bb500a98fbb14674c93a0d8ece47a2.1628888668.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1628888668.git.jonathantanmy@google.com","subject":"[PATCH v2 2/8] grep: use submodule-ODB-as-alternate lazy-addition","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T21:05:17Z","receivedAt":"2021-08-13T21:05:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In the parent commit, Git was taught to add submodule ODBs as alternates\nlazily, but grep does not use this because it computes the path to add\ndirectly, not going through add_submodule_odb(). Add an equivalent to\nadd_submodule_odb() that takes the exact ODB path and teach grep to use\nit.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/grep.c | 2 +-\n submodule.c | 5 +++++\n submodule.h | 1 +\n 3 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 7d2f8e5adb..87bcb934a2 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -450,7 +450,7 @@ static int grep_submodule(struct grep_opt *opt,\n \t * store is no longer global and instead is a member of the repository\n \t * object.\n \t */\n-\tadd_to_alternates_memory(subrepo.objects->odb->path);\n+\tadd_submodule_odb_by_path(subrepo.objects->odb->path);\n \tobj_read_unlock();\n \n \tmemcpy(&subopt, opt, sizeof(subopt));\ndiff --git a/submodule.c b/submodule.c\nindex 8fde90e906..8de1aecaeb 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -187,6 +187,11 @@ int add_submodule_odb(const char *path)\n \treturn ret;\n }\n \n+void add_submodule_odb_by_path(const char *path)\n+{\n+\tstring_list_insert(&added_submodule_odb_paths, xstrdup(path));\n+}\n+\n int register_all_submodule_odb_as_alternates(void)\n {\n \tint i;\ndiff --git a/submodule.h b/submodule.h\nindex c252784bc2..17a06cc43b 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -104,6 +104,7 @@ int bad_to_remove_submodule(const char *path, unsigned flags);\n * the_repository.\n */\n int add_submodule_odb(const char *path);\n+void add_submodule_odb_by_path(const char *path);\n int register_all_submodule_odb_as_alternates(void);\n \n /*\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432710","messageId":"5994a517e8afc345e8f649b2368756e22b0e9ebe.1628888668.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1628888668.git.jonathantanmy@google.com","subject":"[PATCH v2 1/8] submodule: lazily add submodule ODBs as alternates","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T21:05:16Z","receivedAt":"2021-08-13T21:05:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Teach Git to add submodule ODBs as alternates to the object store of\nthe_repository only upon the first access of an object not in\nthe_repository, and not when add_submodule_odb() is called.\n\nThis provides a means of gradually migrating from accessing a\nsubmodule's object through alternates to accessing a submodule's object\nby explicitly passing its repository object. Any Git command can declare\nthat it might access submodule objects by calling add_submodule_odb()\n(as they do now), but the submodule ODBs themselves will not be added\nuntil needed, so individual commands and/or combinations of arguments\ncan be migrated one by one.\n\n[The advantage of explicit repository-object passing is code clarity (it\nis clear which repository an object read is from), performance (there is\nno need to linearly search through all submodule ODBs whenever an object\nis accessed from any repository, whether superproject or submodule), and\nthe possibility of future features like partial clone submodules (which\nright now is not possible because if an object is missing, we do not\nknow which repository to lazy-fetch into).]\n\nThis commit also introduces an environment variable that a test may set\nto make the actual registration of alternates fatal, in order to\ndemonstrate that its codepaths do not need this registration.\n\nSigned-off-by: Jonathan Tan \n---\n object-file.c | 5 +++++\n submodule.c | 20 +++++++++++++++++++-\n submodule.h | 7 +++++++\n t/README | 10 ++++++++++\n 4 files changed, 41 insertions(+), 1 deletion(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 3d27dc8dea..621b121bcb 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -32,6 +32,7 @@\n #include \"packfile.h\"\n #include \"object-store.h\"\n #include \"promisor-remote.h\"\n+#include \"submodule.h\"\n \n /* The maximum size for an object header. */\n #define MAX_HEADER_LEN 32\n@@ -1592,6 +1593,10 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\t\t\tbreak;\n \t\t}\n \n+\t\tif (register_all_submodule_odb_as_alternates())\n+\t\t\t/* We added some alternates; retry */\n+\t\t\tcontinue;\n+\n \t\t/* Check if it is a missing object */\n \t\tif (fetch_if_missing && repo_has_promisor_remote(r) &&\n \t\t !already_retried &&\ndiff --git a/submodule.c b/submodule.c\nindex 8e611fe1db..8fde90e906 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -165,6 +165,8 @@ void stage_updated_gitmodules(struct index_state *istate)\n \t\tdie(_(\"staging updated .gitmodules failed\"));\n }\n \n+static struct string_list added_submodule_odb_paths = STRING_LIST_INIT_NODUP;\n+\n /* TODO: remove this function, use repo_submodule_init instead. */\n int add_submodule_odb(const char *path)\n {\n@@ -178,12 +180,28 @@ int add_submodule_odb(const char *path)\n \t\tret = -1;\n \t\tgoto done;\n \t}\n-\tadd_to_alternates_memory(objects_directory.buf);\n+\tstring_list_insert(&added_submodule_odb_paths,\n+\t\t\t strbuf_detach(&objects_directory, NULL));\n done:\n \tstrbuf_release(&objects_directory);\n \treturn ret;\n }\n \n+int register_all_submodule_odb_as_alternates(void)\n+{\n+\tint i;\n+\tint ret = added_submodule_odb_paths.nr;\n+\n+\tfor (i = 0; i < added_submodule_odb_paths.nr; i++)\n+\t\tadd_to_alternates_memory(added_submodule_odb_paths.items[i].string);\n+\tif (ret) {\n+\t\tstring_list_clear(&added_submodule_odb_paths, 0);\n+\t\tif (git_env_bool(\"GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB\", 0))\n+\t\t\tBUG(\"register_all_submodule_odb_as_alternates() called\");\n+\t}\n+\treturn ret;\n+}\n+\n void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n \t\t\t\t\t const char *path)\n {\ndiff --git a/submodule.h b/submodule.h\nindex 84640c49c1..c252784bc2 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -97,7 +97,14 @@ int submodule_uses_gitfile(const char *path);\n #define SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED (1<<2)\n int bad_to_remove_submodule(const char *path, unsigned flags);\n \n+/*\n+ * Call add_submodule_odb() to add the submodule at the given path to a list.\n+ * When register_all_submodule_odb_as_alternates() is called, the object stores\n+ * of all submodules in that list will be added as alternates in\n+ * the_repository.\n+ */\n int add_submodule_odb(const char *path);\n+int register_all_submodule_odb_as_alternates(void);\n \n /*\n * Checks if there are submodule changes in a..b. If a is the null OID,\ndiff --git a/t/README b/t/README\nindex 9e70122302..8b67b4f00b 100644\n--- a/t/README\n+++ b/t/README\n@@ -448,6 +448,16 @@ GIT_TEST_CHECKOUT_WORKERS= overrides the 'checkout.workers' setting\n to and 'checkout.thresholdForParallelism' to 0, forcing the\n execution of the parallel-checkout code.\n \n+GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=, when true, makes\n+registering submodule ODBs as alternates a fatal action. Support for\n+this environment variable can be removed once the migration to\n+explicitly providing repositories when accessing submodule objects is\n+complete (in which case we might want to replace this with a trace2\n+call so that users can make it visible if accessing submodule objects\n+without an explicit repository still happens) or needs to be abandoned\n+for whatever reason (in which case the migrated codepaths still retain\n+their performance benefits).\n+\n Naming Tests\n ------------\n \n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432711","messageId":"aa3f1f3c8908bd497ef506db466fc123eaccf21d.1628888668.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1628888668.git.jonathantanmy@google.com","subject":"[PATCH v2 3/8] grep: typesafe versions of grep_source_init","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T21:05:18Z","receivedAt":"2021-08-13T21:05:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"grep_source_init() can create \"struct grep_source\" objects and,\ndepending on the value of the type passed, some void-pointer parameters have\ndifferent meanings. Because one of these types (GREP_SOURCE_OID) will\nrequire an additional parameter in a subsequent patch, take the\nopportunity to increase clarity and type safety by replacing this\nfunction with individual functions for each type.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/grep.c | 4 ++--\n grep.c | 46 ++++++++++++++++++++++++++++------------------\n grep.h | 7 ++++---\n 3 files changed, 34 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 87bcb934a2..e454335e9d 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -333,7 +333,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n \tstruct grep_source gs;\n \n \tgrep_source_name(opt, filename, tree_name_len, &pathbuf);\n-\tgrep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);\n+\tgrep_source_init_oid(&gs, pathbuf.buf, path, oid);\n \tstrbuf_release(&pathbuf);\n \n \tif (num_threads > 1) {\n@@ -359,7 +359,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \tstruct grep_source gs;\n \n \tgrep_source_name(opt, filename, 0, &buf);\n-\tgrep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);\n+\tgrep_source_init_file(&gs, buf.buf, filename);\n \tstrbuf_release(&buf);\n \n \tif (num_threads > 1) {\ndiff --git a/grep.c b/grep.c\nindex 424a39591b..8a8105c2eb 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1825,14 +1825,24 @@ int grep_source(struct grep_opt *opt, struct grep_source *gs)\n \treturn grep_source_1(opt, gs, 0);\n }\n \n+static void grep_source_init_buf(struct grep_source *gs, char *buf,\n+\t\t\t\t unsigned long size)\n+{\n+\tgs->type = GREP_SOURCE_BUF;\n+\tgs->name = NULL;\n+\tgs->path = NULL;\n+\tgs->buf = buf;\n+\tgs->size = size;\n+\tgs->driver = NULL;\n+\tgs->identifier = NULL;\n+}\n+\n int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n {\n \tstruct grep_source gs;\n \tint r;\n \n-\tgrep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL);\n-\tgs.buf = buf;\n-\tgs.size = size;\n+\tgrep_source_init_buf(&gs, buf, size);\n \n \tr = grep_source(opt, &gs);\n \n@@ -1840,28 +1850,28 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n \treturn r;\n }\n \n-void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n-\t\t const char *name, const char *path,\n-\t\t const void *identifier)\n+void grep_source_init_file(struct grep_source *gs, const char *name,\n+\t\t\t const char *path)\n {\n-\tgs->type = type;\n+\tgs->type = GREP_SOURCE_FILE;\n \tgs->name = xstrdup_or_null(name);\n \tgs->path = xstrdup_or_null(path);\n \tgs->buf = NULL;\n \tgs->size = 0;\n \tgs->driver = NULL;\n+\tgs->identifier = xstrdup(path);\n+}\n \n-\tswitch (type) {\n-\tcase GREP_SOURCE_FILE:\n-\t\tgs->identifier = xstrdup(identifier);\n-\t\tbreak;\n-\tcase GREP_SOURCE_OID:\n-\t\tgs->identifier = oiddup(identifier);\n-\t\tbreak;\n-\tcase GREP_SOURCE_BUF:\n-\t\tgs->identifier = NULL;\n-\t\tbreak;\n-\t}\n+void grep_source_init_oid(struct grep_source *gs, const char *name,\n+\t\t\t const char *path, const struct object_id *oid)\n+{\n+\tgs->type = GREP_SOURCE_OID;\n+\tgs->name = xstrdup_or_null(name);\n+\tgs->path = xstrdup_or_null(path);\n+\tgs->buf = NULL;\n+\tgs->size = 0;\n+\tgs->driver = NULL;\n+\tgs->identifier = oiddup(oid);\n }\n \n void grep_source_clear(struct grep_source *gs)\ndiff --git a/grep.h b/grep.h\nindex 72f82b1e30..480b3f5bba 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -195,9 +195,10 @@ struct grep_source {\n \tstruct userdiff_driver *driver;\n };\n \n-void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n-\t\t const char *name, const char *path,\n-\t\t const void *identifier);\n+void grep_source_init_file(struct grep_source *gs, const char *name,\n+\t\t\t const char *path);\n+void grep_source_init_oid(struct grep_source *gs, const char *name,\n+\t\t\t const char *path, const struct object_id *oid);\n void grep_source_clear_data(struct grep_source *gs);\n void grep_source_clear(struct grep_source *gs);\n void grep_source_load_driver(struct grep_source *gs,\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432712","messageId":"050deacfb7df41192b57ac42c5ea8fb646106d78.1628888668.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1628888668.git.jonathantanmy@google.com","subject":"[PATCH v2 4/8] grep: read submodule entry with explicit repo","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T21:05:19Z","receivedAt":"2021-08-13T21:05:41Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Replace an existing parse_object_or_die() call (which implicitly works\non the_repository) with a function call that allows a repository to be\npassed in. There is no such direct equivalent to parse_object_or_die(),\nbut we only need the type of the object, so replace with\noid_object_info().\n\nSigned-off-by: Jonathan Tan \n---\n builtin/grep.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex e454335e9d..9e61c7c993 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -457,27 +457,27 @@ static int grep_submodule(struct grep_opt *opt,\n \tsubopt.repo = &subrepo;\n \n \tif (oid) {\n-\t\tstruct object *object;\n+\t\tenum object_type object_type;\n \t\tstruct tree_desc tree;\n \t\tvoid *data;\n \t\tunsigned long size;\n \t\tstruct strbuf base = STRBUF_INIT;\n \n \t\tobj_read_lock();\n-\t\tobject = parse_object_or_die(oid, NULL);\n+\t\tobject_type = oid_object_info(&subrepo, oid, NULL);\n \t\tobj_read_unlock();\n \t\tdata = read_object_with_reference(&subrepo,\n-\t\t\t\t\t\t &object->oid, tree_type,\n+\t\t\t\t\t\t oid, tree_type,\n \t\t\t\t\t\t &size, NULL);\n \t\tif (!data)\n-\t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(&object->oid));\n+\t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(oid));\n \n \t\tstrbuf_addstr(&base, filename);\n \t\tstrbuf_addch(&base, '/');\n \n \t\tinit_tree_desc(&tree, data, size);\n \t\thit = grep_tree(&subopt, pathspec, &tree, &base, base.len,\n-\t\t\t\tobject->type == OBJ_COMMIT);\n+\t\t\t\tobject_type == OBJ_COMMIT);\n \t\tstrbuf_release(&base);\n \t\tfree(data);\n \t} else {\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432713","messageId":"3f2481522404bcf66d83d02bd63b6bf682cee0ef.1628888668.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1628888668.git.jonathantanmy@google.com","subject":"[PATCH v2 5/8] grep: allocate subrepos on heap","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T21:05:20Z","receivedAt":"2021-08-13T21:05:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, struct repository objects corresponding to submodules are\nallocated on the stack in grep_submodule(). This currently works because\nthey will not be used once grep_submodule() exits, but a subsequent\npatch will require these structs to be accessible for longer (perhaps\neven in another thread). Allocate them on the heap and clear them only\nat the very end.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/grep.c | 39 ++++++++++++++++++++++++++++++---------\n 1 file changed, 30 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 9e61c7c993..69d8ea0808 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -65,6 +65,9 @@ static int todo_done;\n /* Has all work items been added? */\n static int all_work_added;\n \n+static struct repository **repos_to_free;\n+static size_t repos_to_free_nr, repos_to_free_alloc;\n+\n /* This lock protects all the variables above. */\n static pthread_mutex_t grep_mutex;\n \n@@ -168,6 +171,19 @@ static void work_done(struct work_item *w)\n \tgrep_unlock();\n }\n \n+static void free_repos(void)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < repos_to_free_nr; i++) {\n+\t\trepo_clear(repos_to_free[i]);\n+\t\tfree(repos_to_free[i]);\n+\t}\n+\tfree(repos_to_free);\n+\trepos_to_free_nr = 0;\n+\trepos_to_free_alloc = 0;\n+}\n+\n static void *run(void *arg)\n {\n \tint hit = 0;\n@@ -415,19 +431,24 @@ static int grep_submodule(struct grep_opt *opt,\n \t\t\t const struct object_id *oid,\n \t\t\t const char *filename, const char *path, int cached)\n {\n-\tstruct repository subrepo;\n+\tstruct repository *subrepo;\n \tstruct repository *superproject = opt->repo;\n \tconst struct submodule *sub;\n \tstruct grep_opt subopt;\n-\tint hit;\n+\tint hit = 0;\n \n \tsub = submodule_from_path(superproject, null_oid(), path);\n \n \tif (!is_submodule_active(superproject, path))\n \t\treturn 0;\n \n-\tif (repo_submodule_init(&subrepo, superproject, sub))\n+\tsubrepo = xmalloc(sizeof(*subrepo));\n+\tif (repo_submodule_init(subrepo, superproject, sub)) {\n+\t\tfree(subrepo);\n \t\treturn 0;\n+\t}\n+\tALLOC_GROW(repos_to_free, repos_to_free_nr + 1, repos_to_free_alloc);\n+\trepos_to_free[repos_to_free_nr++] = subrepo;\n \n \t/*\n \t * NEEDSWORK: repo_read_gitmodules() might call\n@@ -438,7 +459,7 @@ static int grep_submodule(struct grep_opt *opt,\n \t * subrepo's odbs to the in-memory alternates list.\n \t */\n \tobj_read_lock();\n-\trepo_read_gitmodules(&subrepo, 0);\n+\trepo_read_gitmodules(subrepo, 0);\n \n \t/*\n \t * NEEDSWORK: This adds the submodule's object directory to the list of\n@@ -450,11 +471,11 @@ static int grep_submodule(struct grep_opt *opt,\n \t * store is no longer global and instead is a member of the repository\n \t * object.\n \t */\n-\tadd_submodule_odb_by_path(subrepo.objects->odb->path);\n+\tadd_submodule_odb_by_path(subrepo->objects->odb->path);\n \tobj_read_unlock();\n \n \tmemcpy(&subopt, opt, sizeof(subopt));\n-\tsubopt.repo = &subrepo;\n+\tsubopt.repo = subrepo;\n \n \tif (oid) {\n \t\tenum object_type object_type;\n@@ -464,9 +485,9 @@ static int grep_submodule(struct grep_opt *opt,\n \t\tstruct strbuf base = STRBUF_INIT;\n \n \t\tobj_read_lock();\n-\t\tobject_type = oid_object_info(&subrepo, oid, NULL);\n+\t\tobject_type = oid_object_info(subrepo, oid, NULL);\n \t\tobj_read_unlock();\n-\t\tdata = read_object_with_reference(&subrepo,\n+\t\tdata = read_object_with_reference(subrepo,\n \t\t\t\t\t\t oid, tree_type,\n \t\t\t\t\t\t &size, NULL);\n \t\tif (!data)\n@@ -484,7 +505,6 @@ static int grep_submodule(struct grep_opt *opt,\n \t\thit = grep_cache(&subopt, pathspec, cached);\n \t}\n \n-\trepo_clear(&subrepo);\n \treturn hit;\n }\n \n@@ -1182,5 +1202,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\trun_pager(&opt, prefix);\n \tclear_pathspec(&pathspec);\n \tfree_grep_patterns(&opt);\n+\tfree_repos();\n \treturn !hit;\n }\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432714","messageId":"50c69a988b2afca68f06e36949e4a8cc3a93940a.1628888668.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1628888668.git.jonathantanmy@google.com","subject":"[PATCH v2 6/8] grep: add repository to OID grep sources","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T21:05:21Z","receivedAt":"2021-08-13T21:05:45Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Record the repository whenever an OID grep source is created, and teach\nthe worker threads to explicitly provide the repository when accessing\nobjects.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/grep.c | 15 ++++++---------\n grep.c | 7 +++++--\n grep.h | 13 ++++++++++++-\n 3 files changed, 23 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 69d8ea0808..d27e95e092 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -349,7 +349,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n \tstruct grep_source gs;\n \n \tgrep_source_name(opt, filename, tree_name_len, &pathbuf);\n-\tgrep_source_init_oid(&gs, pathbuf.buf, path, oid);\n+\tgrep_source_init_oid(&gs, pathbuf.buf, path, oid, opt->repo);\n \tstrbuf_release(&pathbuf);\n \n \tif (num_threads > 1) {\n@@ -462,14 +462,11 @@ static int grep_submodule(struct grep_opt *opt,\n \trepo_read_gitmodules(subrepo, 0);\n \n \t/*\n-\t * NEEDSWORK: This adds the submodule's object directory to the list of\n-\t * alternates for the single in-memory object store. This has some bad\n-\t * consequences for memory (processed objects will never be freed) and\n-\t * performance (this increases the number of pack files git has to pay\n-\t * attention to, to the sum of the number of pack files in all the\n-\t * repositories processed so far). This can be removed once the object\n-\t * store is no longer global and instead is a member of the repository\n-\t * object.\n+\t * All code paths tested by test code no longer need submodule ODBs to\n+\t * be added as alternates, but add it to the list just in case.\n+\t * Submodule ODBs added through add_submodule_odb_by_path() will be\n+\t * lazily registered as alternates when needed (and except in an\n+\t * unexpected code interaction, it won't be needed).\n \t */\n \tadd_submodule_odb_by_path(subrepo->objects->odb->path);\n \tobj_read_unlock();\ndiff --git a/grep.c b/grep.c\nindex 8a8105c2eb..79598f245f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1863,7 +1863,8 @@ void grep_source_init_file(struct grep_source *gs, const char *name,\n }\n \n void grep_source_init_oid(struct grep_source *gs, const char *name,\n-\t\t\t const char *path, const struct object_id *oid)\n+\t\t\t const char *path, const struct object_id *oid,\n+\t\t\t struct repository *repo)\n {\n \tgs->type = GREP_SOURCE_OID;\n \tgs->name = xstrdup_or_null(name);\n@@ -1872,6 +1873,7 @@ void grep_source_init_oid(struct grep_source *gs, const char *name,\n \tgs->size = 0;\n \tgs->driver = NULL;\n \tgs->identifier = oiddup(oid);\n+\tgs->repo = repo;\n }\n \n void grep_source_clear(struct grep_source *gs)\n@@ -1900,7 +1902,8 @@ static int grep_source_load_oid(struct grep_source *gs)\n {\n \tenum object_type type;\n \n-\tgs->buf = read_object_file(gs->identifier, &type, &gs->size);\n+\tgs->buf = repo_read_object_file(gs->repo, gs->identifier, &type,\n+\t\t\t\t\t&gs->size);\n \tif (!gs->buf)\n \t\treturn error(_(\"'%s': unable to read %s\"),\n \t\t\t gs->name,\ndiff --git a/grep.h b/grep.h\nindex 480b3f5bba..24c6b64cea 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -120,7 +120,16 @@ struct grep_opt {\n \tstruct grep_pat *header_list;\n \tstruct grep_pat **header_tail;\n \tstruct grep_expr *pattern_expression;\n+\n+\t/*\n+\t * NEEDSWORK: See if we can remove this field, because the repository\n+\t * should probably be per-source, not per-repo. This is potentially the\n+\t * cause of at least one bug - \"git grep\" ignoring the textconv\n+\t * attributes from submodules. See [1] for more information.\n+\t * [1] https://lore.kernel.org/git/CAHd-oW5iEQarYVxEXoTG-ua2zdoybTrSjCBKtO0YT292fm0NQQ@mail.gmail.com/\n+\t */\n \tstruct repository *repo;\n+\n \tconst char *prefix;\n \tint prefix_length;\n \tregex_t regexp;\n@@ -187,6 +196,7 @@ struct grep_source {\n \t\tGREP_SOURCE_BUF,\n \t} type;\n \tvoid *identifier;\n+\tstruct repository *repo; /* if GREP_SOURCE_OID */\n \n \tchar *buf;\n \tunsigned long size;\n@@ -198,7 +208,8 @@ struct grep_source {\n void grep_source_init_file(struct grep_source *gs, const char *name,\n \t\t\t const char *path);\n void grep_source_init_oid(struct grep_source *gs, const char *name,\n-\t\t\t const char *path, const struct object_id *oid);\n+\t\t\t const char *path, const struct object_id *oid,\n+\t\t\t struct repository *repo);\n void grep_source_clear_data(struct grep_source *gs);\n void grep_source_clear(struct grep_source *gs);\n void grep_source_load_driver(struct grep_source *gs,\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432715","messageId":"94db10a4e5943d689113693c64633ddffa5508cc.1628888668.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1628888668.git.jonathantanmy@google.com","subject":"[PATCH v2 7/8] submodule-config: pass repo upon blob config read","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T21:05:22Z","receivedAt":"2021-08-13T21:05:47Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When reading the config of a submodule, if reading from a blob, read\nusing an explicitly specified repository instead of by adding the\nsubmodule's ODB as an alternate and then reading an object from\nthe_repository.\n\nSigned-off-by: Jonathan Tan \n---\n config.c | 18 ++++++++++++------\n config.h | 3 +++\n submodule-config.c | 5 +++--\n 3 files changed, 18 insertions(+), 8 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex f33abeab85..a85c12e6cc 100644\n--- a/config.c\n+++ b/config.c\n@@ -1796,6 +1796,7 @@ int git_config_from_mem(config_fn_t fn,\n \n int git_config_from_blob_oid(config_fn_t fn,\n \t\t\t const char *name,\n+\t\t\t struct repository *repo,\n \t\t\t const struct object_id *oid,\n \t\t\t void *data)\n {\n@@ -1804,7 +1805,7 @@ int git_config_from_blob_oid(config_fn_t fn,\n \tunsigned long size;\n \tint ret;\n \n-\tbuf = read_object_file(oid, &type, &size);\n+\tbuf = repo_read_object_file(repo, oid, &type, &size);\n \tif (!buf)\n \t\treturn error(_(\"unable to load config blob object '%s'\"), name);\n \tif (type != OBJ_BLOB) {\n@@ -1820,6 +1821,7 @@ int git_config_from_blob_oid(config_fn_t fn,\n }\n \n static int git_config_from_blob_ref(config_fn_t fn,\n+\t\t\t\t struct repository *repo,\n \t\t\t\t const char *name,\n \t\t\t\t void *data)\n {\n@@ -1827,7 +1829,7 @@ static int git_config_from_blob_ref(config_fn_t fn,\n \n \tif (get_oid(name, &oid) < 0)\n \t\treturn error(_(\"unable to resolve config blob '%s'\"), name);\n-\treturn git_config_from_blob_oid(fn, name, &oid, data);\n+\treturn git_config_from_blob_oid(fn, name, repo, &oid, data);\n }\n \n char *git_system_config(void)\n@@ -1958,12 +1960,16 @@ int config_with_options(config_fn_t fn, void *data,\n \t * If we have a specific filename, use it. Otherwise, follow the\n \t * regular lookup sequence.\n \t */\n-\tif (config_source && config_source->use_stdin)\n+\tif (config_source && config_source->use_stdin) {\n \t\treturn git_config_from_stdin(fn, data);\n-\telse if (config_source && config_source->file)\n+\t} else if (config_source && config_source->file) {\n \t\treturn git_config_from_file(fn, config_source->file, data);\n-\telse if (config_source && config_source->blob)\n-\t\treturn git_config_from_blob_ref(fn, config_source->blob, data);\n+\t} else if (config_source && config_source->blob) {\n+\t\tstruct repository *repo = config_source->repo ?\n+\t\t\tconfig_source->repo : the_repository;\n+\t\treturn git_config_from_blob_ref(fn, repo, config_source->blob,\n+\t\t\t\t\t\tdata);\n+\t}\n \n \treturn do_git_config_sequence(opts, fn, data);\n }\ndiff --git a/config.h b/config.h\nindex a2200f3111..147f5e0490 100644\n--- a/config.h\n+++ b/config.h\n@@ -49,6 +49,8 @@ const char *config_scope_name(enum config_scope scope);\n struct git_config_source {\n \tunsigned int use_stdin:1;\n \tconst char *file;\n+\t/* The repository if blob is not NULL; leave blank for the_repository */\n+\tstruct repository *repo;\n \tconst char *blob;\n \tenum config_scope scope;\n };\n@@ -136,6 +138,7 @@ int git_config_from_mem(config_fn_t fn,\n \t\t\tconst char *buf, size_t len,\n \t\t\tvoid *data, const struct config_options *opts);\n int git_config_from_blob_oid(config_fn_t fn, const char *name,\n+\t\t\t struct repository *repo,\n \t\t\t const struct object_id *oid, void *data);\n void git_config_push_parameter(const char *text);\n void git_config_push_env(const char *spec);\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 2026120fb3..f95344028b 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -649,9 +649,10 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void\n \t\t\tconfig_source.file = file;\n \t\t} else if (repo_get_oid(repo, GITMODULES_INDEX, &oid) >= 0 ||\n \t\t\t repo_get_oid(repo, GITMODULES_HEAD, &oid) >= 0) {\n+\t\t\tconfig_source.repo = repo;\n \t\t\tconfig_source.blob = oidstr = xstrdup(oid_to_hex(&oid));\n \t\t\tif (repo != the_repository)\n-\t\t\t\tadd_to_alternates_memory(repo->objects->odb->path);\n+\t\t\t\tadd_submodule_odb_by_path(repo->objects->odb->path);\n \t\t} else {\n \t\t\tgoto out;\n \t\t}\n@@ -702,7 +703,7 @@ void gitmodules_config_oid(const struct object_id *commit_oid)\n \n \tif (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {\n \t\tgit_config_from_blob_oid(gitmodules_cb, rev.buf,\n-\t\t\t\t\t &oid, the_repository);\n+\t\t\t\t\t the_repository, &oid, the_repository);\n \t}\n \tstrbuf_release(&rev);\n \n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432716","messageId":"4a51fcfb77e4d13597db62e5dc697e8c94cf06fb.1628888668.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1628888668.git.jonathantanmy@google.com","subject":"[PATCH v2 8/8] t7814: show lack of alternate ODB-adding","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-13T21:05:23Z","receivedAt":"2021-08-13T21:05:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The previous patches have made \"git grep\" no longer need to add\nsubmodule ODBs as alternates, at least for the code paths tested in\nt7814. Demonstrate this by making adding a submodule ODB as an alternate\nfatal in this test.\n\nSigned-off-by: Jonathan Tan \n---\n t/t7814-grep-recurse-submodules.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 828cb3ba58..3172f5b936 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -8,6 +8,9 @@ submodules.\n \n . ./test-lib.sh\n \n+GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1\n+export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB\n+\n test_expect_success 'setup directory structure and submodule' '\n \techo \"(1|2)d(3|4)\" >a &&\n \tmkdir b &&\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432717","messageId":"xmqqh7ftuihx.fsf@gitster.g","threadId":"56248","inReplyTo":"3f2481522404bcf66d83d02bd63b6bf682cee0ef.1628888668.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 5/8] grep: allocate subrepos on heap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-13T21:44:58Z","receivedAt":"2021-08-13T21:45:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> +static void free_repos(void)\n> +{\n> +\tint i;\n> +\n> +\tfor (i = 0; i < repos_to_free_nr; i++) {\n> +\t\trepo_clear(repos_to_free[i]);\n> +\t\tfree(repos_to_free[i]);\n> +\t}\n> +\tfree(repos_to_free);\n> +\trepos_to_free_nr = 0;\n> +\trepos_to_free_alloc = 0;\n\nThe clearing of nr/alloc is new in this round.\n\nIt does not matter if we won't using anything that allocates\nrepositories and accumulates them in repos_to_free after we call\nfree_repos() once, but then clearing the nr/alloc would not matter,\neither, so it may be more consistent to FREE_AND_NULL(repos_to_free)\nhere, not just free(), to prepare for another call to ALLOC_GROW()\non the tuple,\nwhich eventually will call into REALLOC_ARRAY() on the pointer, I\nwould think.\n\n\n"},{"id":"432802","messageId":"CAHd-oW7nv-Y_QmhA2gcX5GoWxus88inmbcXQ5kPiSRLBdhtoUw@mail.gmail.com","threadId":"56248","inReplyTo":"94db10a4e5943d689113693c64633ddffa5508cc.1628888668.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 7/8] submodule-config: pass repo upon blob config read","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-08-16T14:32:43Z","receivedAt":"2021-08-16T14:32:57Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Fri, Aug 13, 2021 at 6:05 PM Jonathan Tan wrote:\n>\n> When reading the config of a submodule, if reading from a blob, read\n> using an explicitly specified repository instead of by adding the\n> submodule's ODB as an alternate and then reading an object from\n> the_repository.\n\nGreat!\n\nAt first, I thought this would also allow us to remove another\nNEEDSWORK comment in grep_submodule(), together with a lock\nprotection:\n\n/*\n * NEEDSWORK: repo_read_gitmodules() might call\n * add_to_alternates_memory() via config_from_gitmodules(). This\n * operation causes a race condition with concurrent object readings\n * performed by the worker threads. That's why we need obj_read_lock()\n * here. It should be removed once it's no longer necessary to add the\n * subrepo's odbs to the in-memory alternates list.\n */\nobj_read_lock();\nrepo_read_gitmodules(subrepo, 0);\n\nBack when I wrote this comment, my conclusion was that the alternates\nmechanics were the only thread-unsafe object-reading operations in\nrepo_read_gitmodules()'s call chains. So once the add-to-alternates\nmechanics were gone, we could also remove the lock.\n\nBut with further inspection now, I see that this is not really the\ncase. For example, we have a few global variables in packfile.c\ncollecting some statistics (pack_mmap_calls, pack_open_windows, etc.)\nwhich are updated on obj readings from both the_repository *and*\nsubmodules. So I no longer think its safe to remove the\nobj_read_lock() protection here, as the NEEDSWORK comment suggests,\neven if we are not using the alternates list anymore.\n\nDo you want to remove this comment in your patchset? I can also send a\nfollow-up patch explaining this situation and removing the comment\n(but not the locking), if you prefer.\n\n> Signed-off-by: Jonathan Tan \n> ---\n[...]\n> diff --git a/submodule-config.c b/submodule-config.c\n> index 2026120fb3..f95344028b 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -649,9 +649,10 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void\n> config_source.file = file;\n> } else if (repo_get_oid(repo, GITMODULES_INDEX, &oid) >= 0 ||\n> repo_get_oid(repo, GITMODULES_HEAD, &oid) >= 0) {\n> + config_source.repo = repo;\n> config_source.blob = oidstr = xstrdup(oid_to_hex(&oid));\n> if (repo != the_repository)\n> - add_to_alternates_memory(repo->objects->odb->path);\n> + add_submodule_odb_by_path(repo->objects->odb->path);\n\nOk. Like in grep_submodule(), this should no longer add the submodule\nODB to the alternates list, so this call is now mostly used as a\nfallback and also for testing.\n\nTo see if we are indeed testing this add-to-alternates case, I\nreverted the change that made the code read from the submodule instead\nof the_repository:\n\ndiff --git a/config.c b/config.c\nindex a85c12e6cc..cd37a9dcd9 100644\n--- a/config.c\n+++ b/config.c\n@@ -1805,7 +1805,7 @@ int git_config_from_blob_oid(config_fn_t fn,\n unsigned long size;\n int ret;\n\n- buf = repo_read_object_file(repo, oid, &type, &size);\n+ buf = read_object_file(oid, &type, &size);\n\nThen, I ran t7814-grep-recurse-submodules.sh , where you've added the\nGIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1 envvar. This correctly\nproduced the following error:\n\nBUG: submodule.c:205: register_all_submodule_odb_as_alternates() called\n[...]\nnot ok 23 - grep --recurse-submodules with submodules without\n.gitmodules in the working tree\n\nNice! So the change made by this patch is covered by test 23. I think\nit would be nice to mention that in this patch's message.\n"},{"id":"432803","messageId":"CAHd-oW7cLgZZhGdvONfujDBpNtG1xXfECE=1hLeUzCm8U-N9QQ@mail.gmail.com","threadId":"56248","inReplyTo":"50c69a988b2afca68f06e36949e4a8cc3a93940a.1628888668.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 6/8] grep: add repository to OID grep sources","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-08-16T14:48:17Z","receivedAt":"2021-08-16T14:48:32Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Fri, Aug 13, 2021 at 6:05 PM Jonathan Tan wrote:\n>\n> Record the repository whenever an OID grep source is created, and teach\n> the worker threads to explicitly provide the repository when accessing\n> objects.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> builtin/grep.c | 15 ++++++---------\n> grep.c | 7 +++++--\n> grep.h | 13 ++++++++++++-\n> 3 files changed, 23 insertions(+), 12 deletions(-)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 69d8ea0808..d27e95e092 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -349,7 +349,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n> struct grep_source gs;\n>\n> grep_source_name(opt, filename, tree_name_len, &pathbuf);\n> - grep_source_init_oid(&gs, pathbuf.buf, path, oid);\n> + grep_source_init_oid(&gs, pathbuf.buf, path, oid, opt->repo);\n> strbuf_release(&pathbuf);\n>\n> if (num_threads > 1) {\n> @@ -462,14 +462,11 @@ static int grep_submodule(struct grep_opt *opt,\n> repo_read_gitmodules(subrepo, 0);\n>\n> /*\n> - * NEEDSWORK: This adds the submodule's object directory to the list of\n> - * alternates for the single in-memory object store. This has some bad\n> - * consequences for memory (processed objects will never be freed) and\n> - * performance (this increases the number of pack files git has to pay\n> - * attention to, to the sum of the number of pack files in all the\n> - * repositories processed so far). This can be removed once the object\n> - * store is no longer global and instead is a member of the repository\n> - * object.\n> + * All code paths tested by test code no longer need submodule ODBs to\n> + * be added as alternates, but add it to the list just in case.\n> + * Submodule ODBs added through add_submodule_odb_by_path() will be\n> + * lazily registered as alternates when needed (and except in an\n> + * unexpected code interaction, it won't be needed).\n\nNice.\n\n> */\n> add_submodule_odb_by_path(subrepo->objects->odb->path);\n> obj_read_unlock();\n> diff --git a/grep.c b/grep.c\n> index 8a8105c2eb..79598f245f 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -1863,7 +1863,8 @@ void grep_source_init_file(struct grep_source *gs, const char *name,\n> }\n>\n> void grep_source_init_oid(struct grep_source *gs, const char *name,\n> - const char *path, const struct object_id *oid)\n> + const char *path, const struct object_id *oid,\n> + struct repository *repo)\n> {\n> gs->type = GREP_SOURCE_OID;\n> gs->name = xstrdup_or_null(name);\n> @@ -1872,6 +1873,7 @@ void grep_source_init_oid(struct grep_source *gs, const char *name,\n> gs->size = 0;\n> gs->driver = NULL;\n> gs->identifier = oiddup(oid);\n> + gs->repo = repo;\n> }\n>\n> void grep_source_clear(struct grep_source *gs)\n> @@ -1900,7 +1902,8 @@ static int grep_source_load_oid(struct grep_source *gs)\n> {\n> enum object_type type;\n>\n> - gs->buf = read_object_file(gs->identifier, &type, &gs->size);\n> + gs->buf = repo_read_object_file(gs->repo, gs->identifier, &type,\n> + &gs->size);\n> if (!gs->buf)\n> return error(_(\"'%s': unable to read %s\"),\n> gs->name,\n> diff --git a/grep.h b/grep.h\n> index 480b3f5bba..24c6b64cea 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -120,7 +120,16 @@ struct grep_opt {\n> struct grep_pat *header_list;\n> struct grep_pat **header_tail;\n> struct grep_expr *pattern_expression;\n> +\n> + /*\n> + * NEEDSWORK: See if we can remove this field, because the repository\n> + * should probably be per-source, not per-repo.\n\nHmm, I think the \"not per-repo\" part is a bit confusing, as it refers\nto \"the repository\" (\"the repository should not be per-repo\"?) Could\nwe remove that part?\n\nMaybe we could also be a bit more specific regarding the suggested\nconversion: \"See if we can remove this field, because the repository\nshould probably be per-source. That is, grep.c functions using\n`grep_opt.repo` should probably start using `grep_source.repo`\ninstead.\" (But that's nitpicking from my part, feel free to ignore\nit.)\n\n> [...] This is potentially the\n> + * cause of at least one bug - \"git grep\" ignoring the textconv\n> + * attributes from submodules. See [1] for more information.\n> + * [1] https://lore.kernel.org/git/CAHd-oW5iEQarYVxEXoTG-ua2zdoybTrSjCBKtO0YT292fm0NQQ@mail.gmail.com/\n> + */\n> struct repository *repo;\n> +\n> const char *prefix;\n> int prefix_length;\n> regex_t regexp;\n> @@ -187,6 +196,7 @@ struct grep_source {\n> GREP_SOURCE_BUF,\n> } type;\n> void *identifier;\n> + struct repository *repo; /* if GREP_SOURCE_OID */\n>\n> char *buf;\n> unsigned long size;\n> @@ -198,7 +208,8 @@ struct grep_source {\n> void grep_source_init_file(struct grep_source *gs, const char *name,\n> const char *path);\n> void grep_source_init_oid(struct grep_source *gs, const char *name,\n> - const char *path, const struct object_id *oid);\n> + const char *path, const struct object_id *oid,\n> + struct repository *repo);\n> void grep_source_clear_data(struct grep_source *gs);\n> void grep_source_clear(struct grep_source *gs);\n> void grep_source_load_driver(struct grep_source *gs,\n> --\n> 2.33.0.rc1.237.g0d66db33f3-goog\n>\n"},{"id":"432804","messageId":"CAHd-oW4BruuST61WqguX8Ror0sMmcaErXykv4B5Qb2tv8rszcw@mail.gmail.com","threadId":"56248","inReplyTo":"aa3f1f3c8908bd497ef506db466fc123eaccf21d.1628888668.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 3/8] grep: typesafe versions of grep_source_init","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-08-16T15:06:39Z","receivedAt":"2021-08-16T15:06:54Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Fri, Aug 13, 2021 at 6:05 PM Jonathan Tan wrote:\n>\n> diff --git a/grep.c b/grep.c\n> index 424a39591b..8a8105c2eb 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -1840,28 +1850,28 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n> return r;\n> }\n>\n> -void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n> - const char *name, const char *path,\n> - const void *identifier)\n> +void grep_source_init_file(struct grep_source *gs, const char *name,\n> + const char *path)\n> {\n> - gs->type = type;\n> + gs->type = GREP_SOURCE_FILE;\n> gs->name = xstrdup_or_null(name);\n> gs->path = xstrdup_or_null(path);\n> gs->buf = NULL;\n> gs->size = 0;\n> gs->driver = NULL;\n> + gs->identifier = xstrdup(path);\n\nAt first, it seemed a bit odd to me that we use\n`xstrdup_or_null(path)` first but `xtsrdup(path)` later. But, I saw\nthat we already did this before this patch, and that the second call\nis fine as `path` is required to be non-NULL in this case (i.e.\nGREP_SOURCE_FILE), because we need it for grep_source_load_file()\nlater. (Also, the only caller is correctly passing a non-NULL buffer,\nso all is good.)\n"},{"id":"432805","messageId":"CAHd-oW52gv7_bYHnay12+fPTdCgxsgRDT0yLAxpBH6SjMbwPSg@mail.gmail.com","threadId":"56248","inReplyTo":"cover.1628888668.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 0/8] In grep, no adding submodule ODB as alternates","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-08-16T15:14:09Z","receivedAt":"2021-08-16T15:14:24Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Fri, Aug 13, 2021 at 6:05 PM Jonathan Tan wrote:\n>\n> Thanks everyone for your reviews. I believe I've addressed all of them.\n>\n> The main change is that I've also made a change in submodule-config to\n> avoid registering submodule ODBs as alternates (thanks, Matheus, for\n> noticing this).\n\nThanks for working on this! I reviewed the entire series and left a\nfew comments on some small things, but overall it looks good to me.\n\nReviewed-by: Matheus Tavares \n"},{"id":"432806","messageId":"CAHd-oW4rd6=9WP-7k-Z9V1Z-4nDXR13-45cjyB6GEczxD3+56g@mail.gmail.com","threadId":"56248","inReplyTo":"94db10a4e5943d689113693c64633ddffa5508cc.1628888668.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 7/8] submodule-config: pass repo upon blob config read","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-08-16T15:48:37Z","receivedAt":"2021-08-16T15:51:38Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Fri, Aug 13, 2021 at 6:05 PM Jonathan Tan wrote:\n>\n\nOops, I accidentally deleted this part in my previous reply:\n\n> diff --git a/config.c b/config.c\n> index f33abeab85..a85c12e6cc 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1820,6 +1821,7 @@ int git_config_from_blob_oid(config_fn_t fn,\n> }\n>\n> static int git_config_from_blob_ref(config_fn_t fn,\n> + struct repository *repo,\n> const char *name,\n> void *data)\n> {\n> @@ -1827,7 +1829,7 @@ static int git_config_from_blob_ref(config_fn_t fn,\n>\n> if (get_oid(name, &oid) < 0)\n\nThis should be `repo_get_oid(repo, ...)` now.\n\n> return error(_(\"unable to resolve config blob '%s'\"), name);\n> - return git_config_from_blob_oid(fn, name, &oid, data);\n> + return git_config_from_blob_oid(fn, name, repo, &oid, data);\n> }\n"},{"id":"432834","messageId":"20210816194255.1635406-1-jonathantanmy@google.com","threadId":"56248","inReplyTo":"xmqqh7ftuihx.fsf@gitster.g","subject":"Re: [PATCH v2 5/8] grep: allocate subrepos on heap","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-16T19:42:55Z","receivedAt":"2021-08-16T19:43:03Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> > +static void free_repos(void)\n> > +{\n> > +\tint i;\n> > +\n> > +\tfor (i = 0; i < repos_to_free_nr; i++) {\n> > +\t\trepo_clear(repos_to_free[i]);\n> > +\t\tfree(repos_to_free[i]);\n> > +\t}\n> > +\tfree(repos_to_free);\n> > +\trepos_to_free_nr = 0;\n> > +\trepos_to_free_alloc = 0;\n> \n> The clearing of nr/alloc is new in this round.\n> \n> It does not matter if we won't using anything that allocates\n> repositories and accumulates them in repos_to_free after we call\n> free_repos() once, but then clearing the nr/alloc would not matter,\n> either, so it may be more consistent to FREE_AND_NULL(repos_to_free)\n> here, not just free(), to prepare for another call to ALLOC_GROW()\n> on the tuple,\n> which eventually will call into REALLOC_ARRAY() on the pointer, I\n> would think.\n\nYes, FREE_AND_NULL is more consistent. I'll change it.\n"},{"id":"432835","messageId":"20210816194435.1636117-1-jonathantanmy@google.com","threadId":"56248","inReplyTo":"CAHd-oW7cLgZZhGdvONfujDBpNtG1xXfECE=1hLeUzCm8U-N9QQ@mail.gmail.com","subject":"Re: [PATCH v2 6/8] grep: add repository to OID grep sources","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-16T19:44:35Z","receivedAt":"2021-08-16T19:44:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > @@ -120,7 +120,16 @@ struct grep_opt {\n> > struct grep_pat *header_list;\n> > struct grep_pat **header_tail;\n> > struct grep_expr *pattern_expression;\n> > +\n> > + /*\n> > + * NEEDSWORK: See if we can remove this field, because the repository\n> > + * should probably be per-source, not per-repo.\n> \n> Hmm, I think the \"not per-repo\" part is a bit confusing, as it refers\n> to \"the repository\" (\"the repository should not be per-repo\"?) Could\n> we remove that part?\n> \n> Maybe we could also be a bit more specific regarding the suggested\n> conversion: \"See if we can remove this field, because the repository\n> should probably be per-source. That is, grep.c functions using\n> `grep_opt.repo` should probably start using `grep_source.repo`\n> instead.\" (But that's nitpicking from my part, feel free to ignore\n> it.)\n\nThanks - your suggestion is much clearer, so I'll use it.\n"},{"id":"432836","messageId":"CAHd-oW6o2nSd15NPE3H8zjTtTehVWLsxF6oR5cjYw8uKUEVhUQ@mail.gmail.com","threadId":"56248","inReplyTo":"CAHd-oW7nv-Y_QmhA2gcX5GoWxus88inmbcXQ5kPiSRLBdhtoUw@mail.gmail.com","subject":"Re: [PATCH v2 7/8] submodule-config: pass repo upon blob config read","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-08-16T19:57:05Z","receivedAt":"2021-08-16T19:57:20Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Mon, Aug 16, 2021 at 11:32 AM Matheus Tavares Bernardino\n wrote:\n>\n> On Fri, Aug 13, 2021 at 6:05 PM Jonathan Tan wrote:\n> >\n> > When reading the config of a submodule, if reading from a blob, read\n> > using an explicitly specified repository instead of by adding the\n> > submodule's ODB as an alternate and then reading an object from\n> > the_repository.\n>\n> Great!\n>\n> At first, I thought this would also allow us to remove another\n> NEEDSWORK comment in grep_submodule(), together with a lock\n> protection:\n>\n> /*\n> * NEEDSWORK: repo_read_gitmodules() might call\n> * add_to_alternates_memory() via config_from_gitmodules(). This\n> * operation causes a race condition with concurrent object readings\n> * performed by the worker threads. That's why we need obj_read_lock()\n> * here. It should be removed once it's no longer necessary to add the\n> * subrepo's odbs to the in-memory alternates list.\n> */\n> obj_read_lock();\n> repo_read_gitmodules(subrepo, 0);\n>\n> Back when I wrote this comment, my conclusion was that the alternates\n> mechanics were the only thread-unsafe object-reading operations in\n> repo_read_gitmodules()'s call chains. So once the add-to-alternates\n> mechanics were gone, we could also remove the lock.\n>\n> But with further inspection now, I see that this is not really the\n> case. For example, we have a few global variables in packfile.c\n> collecting some statistics (pack_mmap_calls, pack_open_windows, etc.)\n> which are updated on obj readings from both the_repository *and*\n> submodules.\n\nSorry, this is incorrect. I forgot that repo_read_object_file() (which\nis part of repo_read_gitmodules()'s call chain) also acquires the\nobj_read_mutex before accessing those global variables. So the\nNEEDSWORK might be right.\n\nNevertheless, I think it might be better to look into\nrepo_read_gitmodules() more carefully before removing this lock. And\nthis is something for another series. Sorry about the noise.\n"},{"id":"432837","messageId":"20210816200225.1637935-1-jonathantanmy@google.com","threadId":"56248","inReplyTo":"CAHd-oW7nv-Y_QmhA2gcX5GoWxus88inmbcXQ5kPiSRLBdhtoUw@mail.gmail.com","subject":"Re: [PATCH v2 7/8] submodule-config: pass repo upon blob config read","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-16T20:02:25Z","receivedAt":"2021-08-16T20:02:29Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> /*\n> * NEEDSWORK: repo_read_gitmodules() might call\n> * add_to_alternates_memory() via config_from_gitmodules(). This\n> * operation causes a race condition with concurrent object readings\n> * performed by the worker threads. That's why we need obj_read_lock()\n> * here. It should be removed once it's no longer necessary to add the\n> * subrepo's odbs to the in-memory alternates list.\n> */\n> obj_read_lock();\n> repo_read_gitmodules(subrepo, 0);\n> \n> Back when I wrote this comment, my conclusion was that the alternates\n> mechanics were the only thread-unsafe object-reading operations in\n> repo_read_gitmodules()'s call chains. So once the add-to-alternates\n> mechanics were gone, we could also remove the lock.\n> \n> But with further inspection now, I see that this is not really the\n> case. For example, we have a few global variables in packfile.c\n> collecting some statistics (pack_mmap_calls, pack_open_windows, etc.)\n> which are updated on obj readings from both the_repository *and*\n> submodules. So I no longer think its safe to remove the\n> obj_read_lock() protection here, as the NEEDSWORK comment suggests,\n> even if we are not using the alternates list anymore.\n> \n> Do you want to remove this comment in your patchset? I can also send a\n> follow-up patch explaining this situation and removing the comment\n> (but not the locking), if you prefer.\n\nI think you can make the patch yourself - the comment change you\ndescribe seems unrelated to this patch set.\n\n> Ok. Like in grep_submodule(), this should no longer add the submodule\n> ODB to the alternates list, so this call is now mostly used as a\n> fallback and also for testing.\n> \n> To see if we are indeed testing this add-to-alternates case, I\n> reverted the change that made the code read from the submodule instead\n> of the_repository:\n> \n> diff --git a/config.c b/config.c\n> index a85c12e6cc..cd37a9dcd9 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1805,7 +1805,7 @@ int git_config_from_blob_oid(config_fn_t fn,\n> unsigned long size;\n> int ret;\n> \n> - buf = repo_read_object_file(repo, oid, &type, &size);\n> + buf = read_object_file(oid, &type, &size);\n> \n> Then, I ran t7814-grep-recurse-submodules.sh , where you've added the\n> GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1 envvar. This correctly\n> produced the following error:\n> \n> BUG: submodule.c:205: register_all_submodule_odb_as_alternates() called\n> [...]\n> not ok 23 - grep --recurse-submodules with submodules without\n> .gitmodules in the working tree\n> \n> Nice! So the change made by this patch is covered by test 23. I think\n> it would be nice to mention that in this patch's message.\n\nThanks for checking this. I'll mention this in the commit message.\n"},{"id":"432838","messageId":"20210816200938.1639871-1-jonathantanmy@google.com","threadId":"56248","inReplyTo":"CAHd-oW4rd6=9WP-7k-Z9V1Z-4nDXR13-45cjyB6GEczxD3+56g@mail.gmail.com","subject":"Re: [PATCH v2 7/8] submodule-config: pass repo upon blob config read","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-16T20:09:38Z","receivedAt":"2021-08-16T20:09:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > @@ -1827,7 +1829,7 @@ static int git_config_from_blob_ref(config_fn_t fn,\n> >\n> > if (get_oid(name, &oid) < 0)\n> \n> This should be `repo_get_oid(repo, ...)` now.\n\nAh, good catch! This wasn't caught by the tests because the submodule\nconfig mechanism always passes a full-length hexadecimal string hash as\n\"name\" - and probably would never be caught because\ngit_config_from_blob_ref() is only called from config_with_options(),\nwhich is called with non-NULL source from only 2 files:\nsubmodule-config.c (this one) and builtin/config.c (which most likely\nwill never operate on any repo other than the_repository). I'll refactor\nthe API to avoid this situation in the first place.\n"},{"id":"432867","messageId":"20210816205713.2924648-1-jonathantanmy@google.com","threadId":"56248","inReplyTo":"20210816200938.1639871-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 7/8] submodule-config: pass repo upon blob config read","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-16T20:57:13Z","receivedAt":"2021-08-16T20:57:18Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > > @@ -1827,7 +1829,7 @@ static int git_config_from_blob_ref(config_fn_t fn,\n> > >\n> > > if (get_oid(name, &oid) < 0)\n> > \n> > This should be `repo_get_oid(repo, ...)` now.\n> \n> Ah, good catch! This wasn't caught by the tests because the submodule\n> config mechanism always passes a full-length hexadecimal string hash as\n> \"name\" - and probably would never be caught because\n> git_config_from_blob_ref() is only called from config_with_options(),\n> which is called with non-NULL source from only 2 files:\n> submodule-config.c (this one) and builtin/config.c (which most likely\n> will never operate on any repo other than the_repository). I'll refactor\n> the API to avoid this situation in the first place.\n\nThe refactoring I was thinking of was parsing the OID in\nbuiltin/config.c and then passing only the OID (instead of a\nuser-provided string that could contain anything), but that doesn't\nreally work because the user-provided string is used in certain outputs.\nI'll just change it to repo_get_oid() in this patch.\n"},{"id":"432869","messageId":"cover.1629148153.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1628618950.git.jonathantanmy@google.com","subject":"[PATCH v3 0/8] In grep, no adding submodule ODB as alternates","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-16T21:09:50Z","receivedAt":"2021-08-16T21:10:04Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks for reviewing, everyone. Here are the requested changes.\n\nJonathan Tan (8):\n submodule: lazily add submodule ODBs as alternates\n grep: use submodule-ODB-as-alternate lazy-addition\n grep: typesafe versions of grep_source_init\n grep: read submodule entry with explicit repo\n grep: allocate subrepos on heap\n grep: add repository to OID grep sources\n submodule-config: pass repo upon blob config read\n t7814: show lack of alternate ODB-adding\n\n builtin/grep.c | 64 +++++++++++++++++++-----------\n config.c | 20 ++++++----\n config.h | 3 ++\n grep.c | 51 +++++++++++++++---------\n grep.h | 22 ++++++++--\n object-file.c | 5 +++\n submodule-config.c | 5 ++-\n submodule.c | 25 +++++++++++-\n submodule.h | 8 ++++\n t/README | 10 +++++\n t/t7814-grep-recurse-submodules.sh | 3 ++\n 11 files changed, 161 insertions(+), 55 deletions(-)\n\nRange-diff against v2:\n1: 5994a517e8 = 1: 5994a517e8 submodule: lazily add submodule ODBs as alternates\n2: 31e9b914c4 = 2: 31e9b914c4 grep: use submodule-ODB-as-alternate lazy-addition\n3: aa3f1f3c89 = 3: aa3f1f3c89 grep: typesafe versions of grep_source_init\n4: 050deacfb7 = 4: 050deacfb7 grep: read submodule entry with explicit repo\n5: 3f24815224 ! 5: 7d1eeac4b5 grep: allocate subrepos on heap\n @@ builtin/grep.c: static void work_done(struct work_item *w)\n +\t\trepo_clear(repos_to_free[i]);\n +\t\tfree(repos_to_free[i]);\n +\t}\n -+\tfree(repos_to_free);\n ++\tFREE_AND_NULL(repos_to_free);\n +\trepos_to_free_nr = 0;\n +\trepos_to_free_alloc = 0;\n +}\n6: 50c69a988b ! 6: f362fc278c grep: add repository to OID grep sources\n @@ grep.h: struct grep_opt {\n +\n +\t/*\n +\t * NEEDSWORK: See if we can remove this field, because the repository\n -+\t * should probably be per-source, not per-repo. This is potentially the\n -+\t * cause of at least one bug - \"git grep\" ignoring the textconv\n -+\t * attributes from submodules. See [1] for more information.\n ++\t * should probably be per-source. That is, grep.c functions using this\n ++\t * field should probably start using \"repo\" in \"struct grep_source\"\n ++\t * instead.\n ++\t *\n ++\t * This is potentially the cause of at least one bug - \"git grep\"\n ++\t * ignoring the textconv attributes from submodules. See [1] for more\n ++\t * information.\n +\t * [1] https://lore.kernel.org/git/CAHd-oW5iEQarYVxEXoTG-ua2zdoybTrSjCBKtO0YT292fm0NQQ@mail.gmail.com/\n +\t */\n \tstruct repository *repo;\n7: 94db10a4e5 ! 7: 8b86618531 submodule-config: pass repo upon blob config read\n @@ Commit message\n submodule's ODB as an alternate and then reading an object from\n the_repository.\n \n + This makes the \"grep --recurse-submodules with submodules without\n + .gitmodules in the working tree\" test in t7814 work when\n + GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB is true.\n +\n Signed-off-by: Jonathan Tan \n \n ## config.c ##\n @@ config.c: int git_config_from_blob_oid(config_fn_t fn,\n \t\t\t\t const char *name,\n \t\t\t\t void *data)\n {\n -@@ config.c: static int git_config_from_blob_ref(config_fn_t fn,\n + \tstruct object_id oid;\n \n - \tif (get_oid(name, &oid) < 0)\n +-\tif (get_oid(name, &oid) < 0)\n ++\tif (repo_get_oid(repo, name, &oid) < 0)\n \t\treturn error(_(\"unable to resolve config blob '%s'\"), name);\n -\treturn git_config_from_blob_oid(fn, name, &oid, data);\n +\treturn git_config_from_blob_oid(fn, name, repo, &oid, data);\n8: 4a51fcfb77 = 8: 4b3176f99e t7814: show lack of alternate ODB-adding\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432870","messageId":"5994a517e8afc345e8f649b2368756e22b0e9ebe.1629148153.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1629148153.git.jonathantanmy@google.com","subject":"[PATCH v3 1/8] submodule: lazily add submodule ODBs as alternates","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-16T21:09:51Z","receivedAt":"2021-08-16T21:10:05Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Teach Git to add submodule ODBs as alternates to the object store of\nthe_repository only upon the first access of an object not in\nthe_repository, and not when add_submodule_odb() is called.\n\nThis provides a means of gradually migrating from accessing a\nsubmodule's object through alternates to accessing a submodule's object\nby explicitly passing its repository object. Any Git command can declare\nthat it might access submodule objects by calling add_submodule_odb()\n(as they do now), but the submodule ODBs themselves will not be added\nuntil needed, so individual commands and/or combinations of arguments\ncan be migrated one by one.\n\n[The advantage of explicit repository-object passing is code clarity (it\nis clear which repository an object read is from), performance (there is\nno need to linearly search through all submodule ODBs whenever an object\nis accessed from any repository, whether superproject or submodule), and\nthe possibility of future features like partial clone submodules (which\nright now is not possible because if an object is missing, we do not\nknow which repository to lazy-fetch into).]\n\nThis commit also introduces an environment variable that a test may set\nto make the actual registration of alternates fatal, in order to\ndemonstrate that its codepaths do not need this registration.\n\nSigned-off-by: Jonathan Tan \n---\n object-file.c | 5 +++++\n submodule.c | 20 +++++++++++++++++++-\n submodule.h | 7 +++++++\n t/README | 10 ++++++++++\n 4 files changed, 41 insertions(+), 1 deletion(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 3d27dc8dea..621b121bcb 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -32,6 +32,7 @@\n #include \"packfile.h\"\n #include \"object-store.h\"\n #include \"promisor-remote.h\"\n+#include \"submodule.h\"\n \n /* The maximum size for an object header. */\n #define MAX_HEADER_LEN 32\n@@ -1592,6 +1593,10 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\t\t\tbreak;\n \t\t}\n \n+\t\tif (register_all_submodule_odb_as_alternates())\n+\t\t\t/* We added some alternates; retry */\n+\t\t\tcontinue;\n+\n \t\t/* Check if it is a missing object */\n \t\tif (fetch_if_missing && repo_has_promisor_remote(r) &&\n \t\t !already_retried &&\ndiff --git a/submodule.c b/submodule.c\nindex 8e611fe1db..8fde90e906 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -165,6 +165,8 @@ void stage_updated_gitmodules(struct index_state *istate)\n \t\tdie(_(\"staging updated .gitmodules failed\"));\n }\n \n+static struct string_list added_submodule_odb_paths = STRING_LIST_INIT_NODUP;\n+\n /* TODO: remove this function, use repo_submodule_init instead. */\n int add_submodule_odb(const char *path)\n {\n@@ -178,12 +180,28 @@ int add_submodule_odb(const char *path)\n \t\tret = -1;\n \t\tgoto done;\n \t}\n-\tadd_to_alternates_memory(objects_directory.buf);\n+\tstring_list_insert(&added_submodule_odb_paths,\n+\t\t\t strbuf_detach(&objects_directory, NULL));\n done:\n \tstrbuf_release(&objects_directory);\n \treturn ret;\n }\n \n+int register_all_submodule_odb_as_alternates(void)\n+{\n+\tint i;\n+\tint ret = added_submodule_odb_paths.nr;\n+\n+\tfor (i = 0; i < added_submodule_odb_paths.nr; i++)\n+\t\tadd_to_alternates_memory(added_submodule_odb_paths.items[i].string);\n+\tif (ret) {\n+\t\tstring_list_clear(&added_submodule_odb_paths, 0);\n+\t\tif (git_env_bool(\"GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB\", 0))\n+\t\t\tBUG(\"register_all_submodule_odb_as_alternates() called\");\n+\t}\n+\treturn ret;\n+}\n+\n void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,\n \t\t\t\t\t const char *path)\n {\ndiff --git a/submodule.h b/submodule.h\nindex 84640c49c1..c252784bc2 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -97,7 +97,14 @@ int submodule_uses_gitfile(const char *path);\n #define SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED (1<<2)\n int bad_to_remove_submodule(const char *path, unsigned flags);\n \n+/*\n+ * Call add_submodule_odb() to add the submodule at the given path to a list.\n+ * When register_all_submodule_odb_as_alternates() is called, the object stores\n+ * of all submodules in that list will be added as alternates in\n+ * the_repository.\n+ */\n int add_submodule_odb(const char *path);\n+int register_all_submodule_odb_as_alternates(void);\n \n /*\n * Checks if there are submodule changes in a..b. If a is the null OID,\ndiff --git a/t/README b/t/README\nindex 9e70122302..8b67b4f00b 100644\n--- a/t/README\n+++ b/t/README\n@@ -448,6 +448,16 @@ GIT_TEST_CHECKOUT_WORKERS= overrides the 'checkout.workers' setting\n to and 'checkout.thresholdForParallelism' to 0, forcing the\n execution of the parallel-checkout code.\n \n+GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=, when true, makes\n+registering submodule ODBs as alternates a fatal action. Support for\n+this environment variable can be removed once the migration to\n+explicitly providing repositories when accessing submodule objects is\n+complete (in which case we might want to replace this with a trace2\n+call so that users can make it visible if accessing submodule objects\n+without an explicit repository still happens) or needs to be abandoned\n+for whatever reason (in which case the migrated codepaths still retain\n+their performance benefits).\n+\n Naming Tests\n ------------\n \n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432871","messageId":"31e9b914c4bb500a98fbb14674c93a0d8ece47a2.1629148153.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1629148153.git.jonathantanmy@google.com","subject":"[PATCH v3 2/8] grep: use submodule-ODB-as-alternate lazy-addition","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-16T21:09:52Z","receivedAt":"2021-08-16T21:10:06Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In the parent commit, Git was taught to add submodule ODBs as alternates\nlazily, but grep does not use this because it computes the path to add\ndirectly, not going through add_submodule_odb(). Add an equivalent to\nadd_submodule_odb() that takes the exact ODB path and teach grep to use\nit.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/grep.c | 2 +-\n submodule.c | 5 +++++\n submodule.h | 1 +\n 3 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 7d2f8e5adb..87bcb934a2 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -450,7 +450,7 @@ static int grep_submodule(struct grep_opt *opt,\n \t * store is no longer global and instead is a member of the repository\n \t * object.\n \t */\n-\tadd_to_alternates_memory(subrepo.objects->odb->path);\n+\tadd_submodule_odb_by_path(subrepo.objects->odb->path);\n \tobj_read_unlock();\n \n \tmemcpy(&subopt, opt, sizeof(subopt));\ndiff --git a/submodule.c b/submodule.c\nindex 8fde90e906..8de1aecaeb 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -187,6 +187,11 @@ int add_submodule_odb(const char *path)\n \treturn ret;\n }\n \n+void add_submodule_odb_by_path(const char *path)\n+{\n+\tstring_list_insert(&added_submodule_odb_paths, xstrdup(path));\n+}\n+\n int register_all_submodule_odb_as_alternates(void)\n {\n \tint i;\ndiff --git a/submodule.h b/submodule.h\nindex c252784bc2..17a06cc43b 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -104,6 +104,7 @@ int bad_to_remove_submodule(const char *path, unsigned flags);\n * the_repository.\n */\n int add_submodule_odb(const char *path);\n+void add_submodule_odb_by_path(const char *path);\n int register_all_submodule_odb_as_alternates(void);\n \n /*\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432872","messageId":"aa3f1f3c8908bd497ef506db466fc123eaccf21d.1629148153.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1629148153.git.jonathantanmy@google.com","subject":"[PATCH v3 3/8] grep: typesafe versions of grep_source_init","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-16T21:09:53Z","receivedAt":"2021-08-16T21:10:09Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"grep_source_init() can create \"struct grep_source\" objects and,\ndepending on the value of the type passed, some void-pointer parameters have\ndifferent meanings. Because one of these types (GREP_SOURCE_OID) will\nrequire an additional parameter in a subsequent patch, take the\nopportunity to increase clarity and type safety by replacing this\nfunction with individual functions for each type.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/grep.c | 4 ++--\n grep.c | 46 ++++++++++++++++++++++++++++------------------\n grep.h | 7 ++++---\n 3 files changed, 34 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 87bcb934a2..e454335e9d 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -333,7 +333,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n \tstruct grep_source gs;\n \n \tgrep_source_name(opt, filename, tree_name_len, &pathbuf);\n-\tgrep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);\n+\tgrep_source_init_oid(&gs, pathbuf.buf, path, oid);\n \tstrbuf_release(&pathbuf);\n \n \tif (num_threads > 1) {\n@@ -359,7 +359,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \tstruct grep_source gs;\n \n \tgrep_source_name(opt, filename, 0, &buf);\n-\tgrep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);\n+\tgrep_source_init_file(&gs, buf.buf, filename);\n \tstrbuf_release(&buf);\n \n \tif (num_threads > 1) {\ndiff --git a/grep.c b/grep.c\nindex 424a39591b..8a8105c2eb 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1825,14 +1825,24 @@ int grep_source(struct grep_opt *opt, struct grep_source *gs)\n \treturn grep_source_1(opt, gs, 0);\n }\n \n+static void grep_source_init_buf(struct grep_source *gs, char *buf,\n+\t\t\t\t unsigned long size)\n+{\n+\tgs->type = GREP_SOURCE_BUF;\n+\tgs->name = NULL;\n+\tgs->path = NULL;\n+\tgs->buf = buf;\n+\tgs->size = size;\n+\tgs->driver = NULL;\n+\tgs->identifier = NULL;\n+}\n+\n int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n {\n \tstruct grep_source gs;\n \tint r;\n \n-\tgrep_source_init(&gs, GREP_SOURCE_BUF, NULL, NULL, NULL);\n-\tgs.buf = buf;\n-\tgs.size = size;\n+\tgrep_source_init_buf(&gs, buf, size);\n \n \tr = grep_source(opt, &gs);\n \n@@ -1840,28 +1850,28 @@ int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size)\n \treturn r;\n }\n \n-void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n-\t\t const char *name, const char *path,\n-\t\t const void *identifier)\n+void grep_source_init_file(struct grep_source *gs, const char *name,\n+\t\t\t const char *path)\n {\n-\tgs->type = type;\n+\tgs->type = GREP_SOURCE_FILE;\n \tgs->name = xstrdup_or_null(name);\n \tgs->path = xstrdup_or_null(path);\n \tgs->buf = NULL;\n \tgs->size = 0;\n \tgs->driver = NULL;\n+\tgs->identifier = xstrdup(path);\n+}\n \n-\tswitch (type) {\n-\tcase GREP_SOURCE_FILE:\n-\t\tgs->identifier = xstrdup(identifier);\n-\t\tbreak;\n-\tcase GREP_SOURCE_OID:\n-\t\tgs->identifier = oiddup(identifier);\n-\t\tbreak;\n-\tcase GREP_SOURCE_BUF:\n-\t\tgs->identifier = NULL;\n-\t\tbreak;\n-\t}\n+void grep_source_init_oid(struct grep_source *gs, const char *name,\n+\t\t\t const char *path, const struct object_id *oid)\n+{\n+\tgs->type = GREP_SOURCE_OID;\n+\tgs->name = xstrdup_or_null(name);\n+\tgs->path = xstrdup_or_null(path);\n+\tgs->buf = NULL;\n+\tgs->size = 0;\n+\tgs->driver = NULL;\n+\tgs->identifier = oiddup(oid);\n }\n \n void grep_source_clear(struct grep_source *gs)\ndiff --git a/grep.h b/grep.h\nindex 72f82b1e30..480b3f5bba 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -195,9 +195,10 @@ struct grep_source {\n \tstruct userdiff_driver *driver;\n };\n \n-void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n-\t\t const char *name, const char *path,\n-\t\t const void *identifier);\n+void grep_source_init_file(struct grep_source *gs, const char *name,\n+\t\t\t const char *path);\n+void grep_source_init_oid(struct grep_source *gs, const char *name,\n+\t\t\t const char *path, const struct object_id *oid);\n void grep_source_clear_data(struct grep_source *gs);\n void grep_source_clear(struct grep_source *gs);\n void grep_source_load_driver(struct grep_source *gs,\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432873","messageId":"050deacfb7df41192b57ac42c5ea8fb646106d78.1629148153.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1629148153.git.jonathantanmy@google.com","subject":"[PATCH v3 4/8] grep: read submodule entry with explicit repo","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-16T21:09:54Z","receivedAt":"2021-08-16T21:10:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Replace an existing parse_object_or_die() call (which implicitly works\non the_repository) with a function call that allows a repository to be\npassed in. There is no such direct equivalent to parse_object_or_die(),\nbut we only need the type of the object, so replace with\noid_object_info().\n\nSigned-off-by: Jonathan Tan \n---\n builtin/grep.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex e454335e9d..9e61c7c993 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -457,27 +457,27 @@ static int grep_submodule(struct grep_opt *opt,\n \tsubopt.repo = &subrepo;\n \n \tif (oid) {\n-\t\tstruct object *object;\n+\t\tenum object_type object_type;\n \t\tstruct tree_desc tree;\n \t\tvoid *data;\n \t\tunsigned long size;\n \t\tstruct strbuf base = STRBUF_INIT;\n \n \t\tobj_read_lock();\n-\t\tobject = parse_object_or_die(oid, NULL);\n+\t\tobject_type = oid_object_info(&subrepo, oid, NULL);\n \t\tobj_read_unlock();\n \t\tdata = read_object_with_reference(&subrepo,\n-\t\t\t\t\t\t &object->oid, tree_type,\n+\t\t\t\t\t\t oid, tree_type,\n \t\t\t\t\t\t &size, NULL);\n \t\tif (!data)\n-\t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(&object->oid));\n+\t\t\tdie(_(\"unable to read tree (%s)\"), oid_to_hex(oid));\n \n \t\tstrbuf_addstr(&base, filename);\n \t\tstrbuf_addch(&base, '/');\n \n \t\tinit_tree_desc(&tree, data, size);\n \t\thit = grep_tree(&subopt, pathspec, &tree, &base, base.len,\n-\t\t\t\tobject->type == OBJ_COMMIT);\n+\t\t\t\tobject_type == OBJ_COMMIT);\n \t\tstrbuf_release(&base);\n \t\tfree(data);\n \t} else {\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432874","messageId":"7d1eeac4b50abd46c99f61eaddcc72f8bb1de043.1629148153.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1629148153.git.jonathantanmy@google.com","subject":"[PATCH v3 5/8] grep: allocate subrepos on heap","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-16T21:09:55Z","receivedAt":"2021-08-16T21:10:12Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, struct repository objects corresponding to submodules are\nallocated on the stack in grep_submodule(). This currently works because\nthey will not be used once grep_submodule() exits, but a subsequent\npatch will require these structs to be accessible for longer (perhaps\neven in another thread). Allocate them on the heap and clear them only\nat the very end.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/grep.c | 39 ++++++++++++++++++++++++++++++---------\n 1 file changed, 30 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 9e61c7c993..fa7fd08150 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -65,6 +65,9 @@ static int todo_done;\n /* Has all work items been added? */\n static int all_work_added;\n \n+static struct repository **repos_to_free;\n+static size_t repos_to_free_nr, repos_to_free_alloc;\n+\n /* This lock protects all the variables above. */\n static pthread_mutex_t grep_mutex;\n \n@@ -168,6 +171,19 @@ static void work_done(struct work_item *w)\n \tgrep_unlock();\n }\n \n+static void free_repos(void)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < repos_to_free_nr; i++) {\n+\t\trepo_clear(repos_to_free[i]);\n+\t\tfree(repos_to_free[i]);\n+\t}\n+\tFREE_AND_NULL(repos_to_free);\n+\trepos_to_free_nr = 0;\n+\trepos_to_free_alloc = 0;\n+}\n+\n static void *run(void *arg)\n {\n \tint hit = 0;\n@@ -415,19 +431,24 @@ static int grep_submodule(struct grep_opt *opt,\n \t\t\t const struct object_id *oid,\n \t\t\t const char *filename, const char *path, int cached)\n {\n-\tstruct repository subrepo;\n+\tstruct repository *subrepo;\n \tstruct repository *superproject = opt->repo;\n \tconst struct submodule *sub;\n \tstruct grep_opt subopt;\n-\tint hit;\n+\tint hit = 0;\n \n \tsub = submodule_from_path(superproject, null_oid(), path);\n \n \tif (!is_submodule_active(superproject, path))\n \t\treturn 0;\n \n-\tif (repo_submodule_init(&subrepo, superproject, sub))\n+\tsubrepo = xmalloc(sizeof(*subrepo));\n+\tif (repo_submodule_init(subrepo, superproject, sub)) {\n+\t\tfree(subrepo);\n \t\treturn 0;\n+\t}\n+\tALLOC_GROW(repos_to_free, repos_to_free_nr + 1, repos_to_free_alloc);\n+\trepos_to_free[repos_to_free_nr++] = subrepo;\n \n \t/*\n \t * NEEDSWORK: repo_read_gitmodules() might call\n@@ -438,7 +459,7 @@ static int grep_submodule(struct grep_opt *opt,\n \t * subrepo's odbs to the in-memory alternates list.\n \t */\n \tobj_read_lock();\n-\trepo_read_gitmodules(&subrepo, 0);\n+\trepo_read_gitmodules(subrepo, 0);\n \n \t/*\n \t * NEEDSWORK: This adds the submodule's object directory to the list of\n@@ -450,11 +471,11 @@ static int grep_submodule(struct grep_opt *opt,\n \t * store is no longer global and instead is a member of the repository\n \t * object.\n \t */\n-\tadd_submodule_odb_by_path(subrepo.objects->odb->path);\n+\tadd_submodule_odb_by_path(subrepo->objects->odb->path);\n \tobj_read_unlock();\n \n \tmemcpy(&subopt, opt, sizeof(subopt));\n-\tsubopt.repo = &subrepo;\n+\tsubopt.repo = subrepo;\n \n \tif (oid) {\n \t\tenum object_type object_type;\n@@ -464,9 +485,9 @@ static int grep_submodule(struct grep_opt *opt,\n \t\tstruct strbuf base = STRBUF_INIT;\n \n \t\tobj_read_lock();\n-\t\tobject_type = oid_object_info(&subrepo, oid, NULL);\n+\t\tobject_type = oid_object_info(subrepo, oid, NULL);\n \t\tobj_read_unlock();\n-\t\tdata = read_object_with_reference(&subrepo,\n+\t\tdata = read_object_with_reference(subrepo,\n \t\t\t\t\t\t oid, tree_type,\n \t\t\t\t\t\t &size, NULL);\n \t\tif (!data)\n@@ -484,7 +505,6 @@ static int grep_submodule(struct grep_opt *opt,\n \t\thit = grep_cache(&subopt, pathspec, cached);\n \t}\n \n-\trepo_clear(&subrepo);\n \treturn hit;\n }\n \n@@ -1182,5 +1202,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\trun_pager(&opt, prefix);\n \tclear_pathspec(&pathspec);\n \tfree_grep_patterns(&opt);\n+\tfree_repos();\n \treturn !hit;\n }\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432875","messageId":"f362fc278cc2d0291c24097a75f8cfdfe88c27d6.1629148153.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1629148153.git.jonathantanmy@google.com","subject":"[PATCH v3 6/8] grep: add repository to OID grep sources","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-16T21:09:56Z","receivedAt":"2021-08-16T21:10:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Record the repository whenever an OID grep source is created, and teach\nthe worker threads to explicitly provide the repository when accessing\nobjects.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/grep.c | 15 ++++++---------\n grep.c | 7 +++++--\n grep.h | 17 ++++++++++++++++-\n 3 files changed, 27 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex fa7fd08150..51278b01fa 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -349,7 +349,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n \tstruct grep_source gs;\n \n \tgrep_source_name(opt, filename, tree_name_len, &pathbuf);\n-\tgrep_source_init_oid(&gs, pathbuf.buf, path, oid);\n+\tgrep_source_init_oid(&gs, pathbuf.buf, path, oid, opt->repo);\n \tstrbuf_release(&pathbuf);\n \n \tif (num_threads > 1) {\n@@ -462,14 +462,11 @@ static int grep_submodule(struct grep_opt *opt,\n \trepo_read_gitmodules(subrepo, 0);\n \n \t/*\n-\t * NEEDSWORK: This adds the submodule's object directory to the list of\n-\t * alternates for the single in-memory object store. This has some bad\n-\t * consequences for memory (processed objects will never be freed) and\n-\t * performance (this increases the number of pack files git has to pay\n-\t * attention to, to the sum of the number of pack files in all the\n-\t * repositories processed so far). This can be removed once the object\n-\t * store is no longer global and instead is a member of the repository\n-\t * object.\n+\t * All code paths tested by test code no longer need submodule ODBs to\n+\t * be added as alternates, but add it to the list just in case.\n+\t * Submodule ODBs added through add_submodule_odb_by_path() will be\n+\t * lazily registered as alternates when needed (and except in an\n+\t * unexpected code interaction, it won't be needed).\n \t */\n \tadd_submodule_odb_by_path(subrepo->objects->odb->path);\n \tobj_read_unlock();\ndiff --git a/grep.c b/grep.c\nindex 8a8105c2eb..79598f245f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1863,7 +1863,8 @@ void grep_source_init_file(struct grep_source *gs, const char *name,\n }\n \n void grep_source_init_oid(struct grep_source *gs, const char *name,\n-\t\t\t const char *path, const struct object_id *oid)\n+\t\t\t const char *path, const struct object_id *oid,\n+\t\t\t struct repository *repo)\n {\n \tgs->type = GREP_SOURCE_OID;\n \tgs->name = xstrdup_or_null(name);\n@@ -1872,6 +1873,7 @@ void grep_source_init_oid(struct grep_source *gs, const char *name,\n \tgs->size = 0;\n \tgs->driver = NULL;\n \tgs->identifier = oiddup(oid);\n+\tgs->repo = repo;\n }\n \n void grep_source_clear(struct grep_source *gs)\n@@ -1900,7 +1902,8 @@ static int grep_source_load_oid(struct grep_source *gs)\n {\n \tenum object_type type;\n \n-\tgs->buf = read_object_file(gs->identifier, &type, &gs->size);\n+\tgs->buf = repo_read_object_file(gs->repo, gs->identifier, &type,\n+\t\t\t\t\t&gs->size);\n \tif (!gs->buf)\n \t\treturn error(_(\"'%s': unable to read %s\"),\n \t\t\t gs->name,\ndiff --git a/grep.h b/grep.h\nindex 480b3f5bba..128007db65 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -120,7 +120,20 @@ struct grep_opt {\n \tstruct grep_pat *header_list;\n \tstruct grep_pat **header_tail;\n \tstruct grep_expr *pattern_expression;\n+\n+\t/*\n+\t * NEEDSWORK: See if we can remove this field, because the repository\n+\t * should probably be per-source. That is, grep.c functions using this\n+\t * field should probably start using \"repo\" in \"struct grep_source\"\n+\t * instead.\n+\t *\n+\t * This is potentially the cause of at least one bug - \"git grep\"\n+\t * ignoring the textconv attributes from submodules. See [1] for more\n+\t * information.\n+\t * [1] https://lore.kernel.org/git/CAHd-oW5iEQarYVxEXoTG-ua2zdoybTrSjCBKtO0YT292fm0NQQ@mail.gmail.com/\n+\t */\n \tstruct repository *repo;\n+\n \tconst char *prefix;\n \tint prefix_length;\n \tregex_t regexp;\n@@ -187,6 +200,7 @@ struct grep_source {\n \t\tGREP_SOURCE_BUF,\n \t} type;\n \tvoid *identifier;\n+\tstruct repository *repo; /* if GREP_SOURCE_OID */\n \n \tchar *buf;\n \tunsigned long size;\n@@ -198,7 +212,8 @@ struct grep_source {\n void grep_source_init_file(struct grep_source *gs, const char *name,\n \t\t\t const char *path);\n void grep_source_init_oid(struct grep_source *gs, const char *name,\n-\t\t\t const char *path, const struct object_id *oid);\n+\t\t\t const char *path, const struct object_id *oid,\n+\t\t\t struct repository *repo);\n void grep_source_clear_data(struct grep_source *gs);\n void grep_source_clear(struct grep_source *gs);\n void grep_source_load_driver(struct grep_source *gs,\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432876","messageId":"8b866185315d14196d191fbafebbd7887b675004.1629148153.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1629148153.git.jonathantanmy@google.com","subject":"[PATCH v3 7/8] submodule-config: pass repo upon blob config read","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-16T21:09:57Z","receivedAt":"2021-08-16T21:10:16Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When reading the config of a submodule, if reading from a blob, read\nusing an explicitly specified repository instead of by adding the\nsubmodule's ODB as an alternate and then reading an object from\nthe_repository.\n\nThis makes the \"grep --recurse-submodules with submodules without\n.gitmodules in the working tree\" test in t7814 work when\nGIT_TEST_FATAL_REGISTER_SUBMODULE_ODB is true.\n\nSigned-off-by: Jonathan Tan \n---\n config.c | 20 +++++++++++++-------\n config.h | 3 +++\n submodule-config.c | 5 +++--\n 3 files changed, 19 insertions(+), 9 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex f33abeab85..f401c1d24e 100644\n--- a/config.c\n+++ b/config.c\n@@ -1796,6 +1796,7 @@ int git_config_from_mem(config_fn_t fn,\n \n int git_config_from_blob_oid(config_fn_t fn,\n \t\t\t const char *name,\n+\t\t\t struct repository *repo,\n \t\t\t const struct object_id *oid,\n \t\t\t void *data)\n {\n@@ -1804,7 +1805,7 @@ int git_config_from_blob_oid(config_fn_t fn,\n \tunsigned long size;\n \tint ret;\n \n-\tbuf = read_object_file(oid, &type, &size);\n+\tbuf = repo_read_object_file(repo, oid, &type, &size);\n \tif (!buf)\n \t\treturn error(_(\"unable to load config blob object '%s'\"), name);\n \tif (type != OBJ_BLOB) {\n@@ -1820,14 +1821,15 @@ int git_config_from_blob_oid(config_fn_t fn,\n }\n \n static int git_config_from_blob_ref(config_fn_t fn,\n+\t\t\t\t struct repository *repo,\n \t\t\t\t const char *name,\n \t\t\t\t void *data)\n {\n \tstruct object_id oid;\n \n-\tif (get_oid(name, &oid) < 0)\n+\tif (repo_get_oid(repo, name, &oid) < 0)\n \t\treturn error(_(\"unable to resolve config blob '%s'\"), name);\n-\treturn git_config_from_blob_oid(fn, name, &oid, data);\n+\treturn git_config_from_blob_oid(fn, name, repo, &oid, data);\n }\n \n char *git_system_config(void)\n@@ -1958,12 +1960,16 @@ int config_with_options(config_fn_t fn, void *data,\n \t * If we have a specific filename, use it. Otherwise, follow the\n \t * regular lookup sequence.\n \t */\n-\tif (config_source && config_source->use_stdin)\n+\tif (config_source && config_source->use_stdin) {\n \t\treturn git_config_from_stdin(fn, data);\n-\telse if (config_source && config_source->file)\n+\t} else if (config_source && config_source->file) {\n \t\treturn git_config_from_file(fn, config_source->file, data);\n-\telse if (config_source && config_source->blob)\n-\t\treturn git_config_from_blob_ref(fn, config_source->blob, data);\n+\t} else if (config_source && config_source->blob) {\n+\t\tstruct repository *repo = config_source->repo ?\n+\t\t\tconfig_source->repo : the_repository;\n+\t\treturn git_config_from_blob_ref(fn, repo, config_source->blob,\n+\t\t\t\t\t\tdata);\n+\t}\n \n \treturn do_git_config_sequence(opts, fn, data);\n }\ndiff --git a/config.h b/config.h\nindex a2200f3111..147f5e0490 100644\n--- a/config.h\n+++ b/config.h\n@@ -49,6 +49,8 @@ const char *config_scope_name(enum config_scope scope);\n struct git_config_source {\n \tunsigned int use_stdin:1;\n \tconst char *file;\n+\t/* The repository if blob is not NULL; leave blank for the_repository */\n+\tstruct repository *repo;\n \tconst char *blob;\n \tenum config_scope scope;\n };\n@@ -136,6 +138,7 @@ int git_config_from_mem(config_fn_t fn,\n \t\t\tconst char *buf, size_t len,\n \t\t\tvoid *data, const struct config_options *opts);\n int git_config_from_blob_oid(config_fn_t fn, const char *name,\n+\t\t\t struct repository *repo,\n \t\t\t const struct object_id *oid, void *data);\n void git_config_push_parameter(const char *text);\n void git_config_push_env(const char *spec);\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 2026120fb3..f95344028b 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -649,9 +649,10 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void\n \t\t\tconfig_source.file = file;\n \t\t} else if (repo_get_oid(repo, GITMODULES_INDEX, &oid) >= 0 ||\n \t\t\t repo_get_oid(repo, GITMODULES_HEAD, &oid) >= 0) {\n+\t\t\tconfig_source.repo = repo;\n \t\t\tconfig_source.blob = oidstr = xstrdup(oid_to_hex(&oid));\n \t\t\tif (repo != the_repository)\n-\t\t\t\tadd_to_alternates_memory(repo->objects->odb->path);\n+\t\t\t\tadd_submodule_odb_by_path(repo->objects->odb->path);\n \t\t} else {\n \t\t\tgoto out;\n \t\t}\n@@ -702,7 +703,7 @@ void gitmodules_config_oid(const struct object_id *commit_oid)\n \n \tif (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {\n \t\tgit_config_from_blob_oid(gitmodules_cb, rev.buf,\n-\t\t\t\t\t &oid, the_repository);\n+\t\t\t\t\t the_repository, &oid, the_repository);\n \t}\n \tstrbuf_release(&rev);\n \n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"432877","messageId":"4b3176f99e315170740a38527d8f894ce2c411d4.1629148153.git.jonathantanmy@google.com","threadId":"56248","inReplyTo":"cover.1629148153.git.jonathantanmy@google.com","subject":"[PATCH v3 8/8] t7814: show lack of alternate ODB-adding","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-16T21:09:58Z","receivedAt":"2021-08-16T21:10:17Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The previous patches have made \"git grep\" no longer need to add\nsubmodule ODBs as alternates, at least for the code paths tested in\nt7814. Demonstrate this by making adding a submodule ODB as an alternate\nfatal in this test.\n\nSigned-off-by: Jonathan Tan \n---\n t/t7814-grep-recurse-submodules.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 828cb3ba58..3172f5b936 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -8,6 +8,9 @@ submodules.\n \n . ./test-lib.sh\n \n+GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1\n+export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB\n+\n test_expect_success 'setup directory structure and submodule' '\n \techo \"(1|2)d(3|4)\" >a &&\n \tmkdir b &&\n-- \n2.33.0.rc1.237.g0d66db33f3-goog\n\n"},{"id":"433020","messageId":"CAHd-oW6Va31PaTQeoSrwi09wjHk63S-Xd_PYtW1trhSokoKizQ@mail.gmail.com","threadId":"56248","inReplyTo":"cover.1629148153.git.jonathantanmy@google.com","subject":"Re: [PATCH v3 0/8] In grep, no adding submodule ODB as alternates","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-08-17T19:29:38Z","receivedAt":"2021-08-17T19:29:53Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Mon, Aug 16, 2021 at 6:10 PM Jonathan Tan wrote:\n>\n> Thanks for reviewing, everyone. Here are the requested changes.\n\nThanks. You've addressed all my comments from the previous rounds.\n\nOne minor suggestion which is not worth a re-roll on its own:\n\n> Range-diff against v2:\n[...]\n> 7: 94db10a4e5 ! 7: 8b86618531 submodule-config: pass repo upon blob config read\n> @@ Commit message\n> When reading the config of a submodule, if reading from a blob, read\n> using an explicitly specified repository instead of by adding the\n> submodule's ODB as an alternate and then reading an object from\n> the_repository.\n>\n> + This makes the \"grep --recurse-submodules with submodules without\n> + .gitmodules in the working tree\" test in t7814 work when\n> + GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB is true.\n\nThis sounds to me as if the test was previously failing when\nGIT_TEST_FATAL_REGISTER_SUBMODULE_ODB is true, which is not the case.\nI think an alternative could be:\n\nThis code path is exercised by the \"grep --recurse-submodules with\nsubmodules without .gitmodules in the working tree\" test in t7814. The\ntest passes with GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB=1,\nshowing that we indeed are no longer accessing the submodule's ODB\nthrough the alternates list.\n\nBut it's really a minor thing. With or without this change:\n\nReviewed-by: Matheus Tavares \n"},{"id":"434972","messageId":"xmqq8s079al1.fsf@gitster.g","threadId":"56248","inReplyTo":"cover.1629148153.git.jonathantanmy@google.com","subject":"Re: [PATCH v3 0/8] In grep, no adding submodule ODB as alternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-08T00:26:18Z","receivedAt":"2021-09-08T00:26:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Thanks for reviewing, everyone. Here are the requested changes.\n>\n> Jonathan Tan (8):\n> submodule: lazily add submodule ODBs as alternates\n> grep: use submodule-ODB-as-alternate lazy-addition\n> grep: typesafe versions of grep_source_init\n> grep: read submodule entry with explicit repo\n> grep: allocate subrepos on heap\n> grep: add repository to OID grep sources\n> submodule-config: pass repo upon blob config read\n> t7814: show lack of alternate ODB-adding\n\nI didn't see anybody comment on this round (and do not think I saw\nanything glaringly wrong).\n\nIs everybody happy with this version? I am about to mark it for\n'next' in the next issue of \"What's cooking\" report, so please\nholler if I should wait.\n\nThanks.\n"},{"id":"435105","messageId":"CAHd-oW5VBzZ+mgzsQ65eTD70q53+-Yri=pFW7ffvt95AmC7rVQ@mail.gmail.com","threadId":"56248","inReplyTo":"xmqq8s079al1.fsf@gitster.g","subject":"Re: [PATCH v3 0/8] In grep, no adding submodule ODB as alternates","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-09-08T15:31:51Z","receivedAt":"2021-09-08T15:32:27Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Tue, Sep 7, 2021 at 9:26 PM Junio C Hamano wrote:\n>\n> Jonathan Tan writes:\n>\n> > Thanks for reviewing, everyone. Here are the requested changes.\n> >\n> > Jonathan Tan (8):\n> > submodule: lazily add submodule ODBs as alternates\n> > grep: use submodule-ODB-as-alternate lazy-addition\n> > grep: typesafe versions of grep_source_init\n> > grep: read submodule entry with explicit repo\n> > grep: allocate subrepos on heap\n> > grep: add repository to OID grep sources\n> > submodule-config: pass repo upon blob config read\n> > t7814: show lack of alternate ODB-adding\n>\n> I didn't see anybody comment on this round (and do not think I saw\n> anything glaringly wrong).\n>\n> Is everybody happy with this version? I am about to mark it for\n> 'next' in the next issue of \"What's cooking\" report, so please\n> holler if I should wait.\n\nI think it's ready for 'next'. Jonathan addressed all my comments\nfrom previous rounds; I also just read the patches again and all\nlooks good to me. Feel free to include my:\n\nReviewed-by: Matheus Tavares \n"},{"id":"435137","messageId":"xmqqee9y52jv.fsf@gitster.g","threadId":"56248","inReplyTo":"CAHd-oW5VBzZ+mgzsQ65eTD70q53+-Yri=pFW7ffvt95AmC7rVQ@mail.gmail.com","subject":"Re: [PATCH v3 0/8] In grep, no adding submodule ODB as alternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-08T18:45:40Z","receivedAt":"2021-09-08T18:45:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares Bernardino writes:\n\n> On Tue, Sep 7, 2021 at 9:26 PM Junio C Hamano wrote:\n>>\n>> Jonathan Tan writes:\n>>\n>> > Thanks for reviewing, everyone. Here are the requested changes.\n>> >\n>> > Jonathan Tan (8):\n>> > submodule: lazily add submodule ODBs as alternates\n>> > grep: use submodule-ODB-as-alternate lazy-addition\n>> > grep: typesafe versions of grep_source_init\n>> > grep: read submodule entry with explicit repo\n>> > grep: allocate subrepos on heap\n>> > grep: add repository to OID grep sources\n>> > submodule-config: pass repo upon blob config read\n>> > t7814: show lack of alternate ODB-adding\n>>\n>> I didn't see anybody comment on this round (and do not think I saw\n>> anything glaringly wrong).\n>>\n>> Is everybody happy with this version? I am about to mark it for\n>> 'next' in the next issue of \"What's cooking\" report, so please\n>> holler if I should wait.\n>\n> I think it's ready for 'next'. Jonathan addressed all my comments\n> from previous rounds; I also just read the patches again and all\n> looks good to me. Feel free to include my:\n>\n> Reviewed-by: Matheus Tavares \n\nThanks, both.\n"},{"id":"437132","messageId":"878rzi1ape.fsf@evledraar.gmail.com","threadId":"56248","inReplyTo":"f362fc278cc2d0291c24097a75f8cfdfe88c27d6.1629148153.git.jonathantanmy@google.com","subject":"Re: [PATCH v3 6/8] grep: add repository to OID grep sources","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-27T12:08:20Z","receivedAt":"2021-09-27T12:09:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Aug 16 2021, Jonathan Tan wrote:\n\n> Record the repository whenever an OID grep source is created, and teach\n> the worker threads to explicitly provide the repository when accessing\n> objects.\n> [...]\n> diff --git a/grep.h b/grep.h\n> index 480b3f5bba..128007db65 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -120,7 +120,20 @@ struct grep_opt {\n> \tstruct grep_pat *header_list;\n> \tstruct grep_pat **header_tail;\n> \tstruct grep_expr *pattern_expression;\n> +\n> +\t/*\n> +\t * NEEDSWORK: See if we can remove this field, because the repository\n> +\t * should probably be per-source. That is, grep.c functions using this\n> +\t * field should probably start using \"repo\" in \"struct grep_source\"\n> +\t * instead.\n> +\t *\n> +\t * This is potentially the cause of at least one bug - \"git grep\"\n> +\t * ignoring the textconv attributes from submodules. See [1] for more\n> +\t * information.\n> +\t * [1] https://lore.kernel.org/git/CAHd-oW5iEQarYVxEXoTG-ua2zdoybTrSjCBKtO0YT292fm0NQQ@mail.gmail.com/\n> +\t */\n> \tstruct repository *repo;\n> +\n\nI ran into this comment and read the linked E-Mail, and then the\ndownthread\nhttps://lore.kernel.org/git/CAHd-oW6uG1fap-T4UF17bJmjoHAqWCDq9KbY+_8a3cEnnfATxg@mail.gmail.com/;\n\nGiven Matheus's \"I've somehow missed this guard and the...\" there I'm\nnot quite sure what/if we should be doing here & what this comment is\nrecommending? I.e. do we still need to adjust the call chains as noted\nin the E-Mail the comment links to, or not?\n"},{"id":"437164","messageId":"296230071f2cabda213b2a5f6f435f9308718569.1632754555.git.matheus.bernardino@usp.br","threadId":"56248","inReplyTo":"878rzi1ape.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH 0/3] grep: don'\\''t add subrepos to in-memory alternates","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-09-27T16:45:30Z","receivedAt":"2021-09-27T16:45:40Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Mon, Sep 27, 2021 at 9:09 AM Ævar Arnfjörð Bjarmason wrote:\n>\n>\n> On Mon, Aug 16 2021, Jonathan Tan wrote:\n>\n> > Record the repository whenever an OID grep source is created, and teach\n> > the worker threads to explicitly provide the repository when accessing\n> > objects.\n> > [...]\n> > diff --git a/grep.h b/grep.h\n> > index 480b3f5bba..128007db65 100644\n> > --- a/grep.h\n> > +++ b/grep.h\n> > @@ -120,7 +120,20 @@ struct grep_opt {\n> >       struct grep_pat *header_list;\n> >       struct grep_pat **header_tail;\n> >       struct grep_expr *pattern_expression;\n> > +\n> > +     /*\n> > +      * NEEDSWORK: See if we can remove this field, because the repository\n> > +      * should probably be per-source. That is, grep.c functions using this\n> > +      * field should probably start using \"repo\" in \"struct grep_source\"\n> > +      * instead.\n> > +      *\n> > +      * This is potentially the cause of at least one bug - \"git grep\"\n> > +      * ignoring the textconv attributes from submodules. See [1] for more\n> > +      * information.\n> > +      * [1] https://lore.kernel.org/git/CAHd-oW5iEQarYVxEXoTG-ua2zdoybTrSjCBKtO0YT292fm0NQQ@mail.gmail.com/\n> > +      */\n> >       struct repository *repo;\n> > +\n>\n> I ran into this comment and read the linked E-Mail, and then the\n> downthread\n> https://lore.kernel.org/git/CAHd-oW6uG1fap-T4UF17bJmjoHAqWCDq9KbY+_8a3cEnnfATxg@mail.gmail.com/;\n>\n> Given Matheus's \"I've somehow missed this guard and the...\" there I'm\n> not quite sure what/if we should be doing here & what this comment is\n> recommending? I.e. do we still need to adjust the call chains as noted\n> in the E-Mail the comment links to, or not?\n\nI think we should still adjust the call chains, yes. The downthread\nmessage you mentioned is kind of a tangent about performance, where\nJunio helped me understand something I had previously missed in the\ncode, regarding the persistence of the attributes stack.\n\nBut the issue that started the thread was about a correctness problem:\nthe superproject textconv attributes are being used on submodules'\nfiles when running `git grep` with `--recurse-submodules --textconv`.\nThe three cases to consider are:\n\n- .gitattributes from the working tree\n- .gitattributes from the index\n- .git/info/attributes\n\nOn all these cases, the superproject attributes are being used on the\nsubmodule. Additionally, if the superproject does not define any\nattribute, the submodule attributes are being ignored in all cases\nexcept by the first one (but that is only because the code sees the\n.gitattributes file on the submodule as if it were a \"regular\"\nsubdirectory of the surperproject. So the submodule's .gitattribures\ntakes higher precedence when evaluating the attributes for files in\nthat directory).\n\nAnother issue is that the textconv cache is always saved to (and read\nfrom) the superproject gitdir, even for submodules' files.\n\nHere are some test cases that demonstrate these issues:\n\n-- snipsnap --\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 3172f5b936..d01a3bc5d8 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -441,4 +441,104 @@ test_expect_success 'grep --recurse-submodules with --cached ignores worktree mo\n \ttest_must_fail git grep --recurse-submodules --cached \"A modified line in submodule\" >actual 2>&1 &&\n \ttest_must_be_empty actual\n '\n+\n+test_expect_failure 'grep --textconv: superproject .gitattributes does not affect submodules' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\techo \"a diff=d2x\" >.gitattributes &&\n+\n+\tcat >expect <<-\\EOF &&\n+\ta:(1|2)x(3|4)\n+\tEOF\n+\tgit grep --textconv --recurse-submodules x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'grep --textconv: superproject .gitattributes (from index) does not affect submodules' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\techo \"a diff=d2x\" >.gitattributes &&\n+\tgit add .gitattributes &&\n+\trm .gitattributes &&\n+\n+\tcat >expect <<-\\EOF &&\n+\ta:(1|2)x(3|4)\n+\tEOF\n+\tgit grep --textconv --recurse-submodules x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'grep --textconv: superproject .git/info/attributes does not affect submodules' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\tsuper_attr=\"$(git rev-parse --path-format=relative --git-path info/attributes)\" &&\n+\ttest_when_finished rm -f \"$super_attr\" &&\n+\techo \"a diff=d2x\" >\"$super_attr\" &&\n+\n+\tcat >expect <<-\\EOF &&\n+\ta:(1|2)x(3|4)\n+\tEOF\n+\tgit grep --textconv --recurse-submodules x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep --textconv corectly reads submodule .gitattributes' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\techo \"a diff=d2x\" >submodule/.gitattributes &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tsubmodule/a:(1|2)x(3|4)\n+\tEOF\n+\tgit grep --textconv --recurse-submodules x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'grep --textconv corectly reads submodule .gitattributes (from index)' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\techo \"a diff=d2x\" >submodule/.gitattributes &&\n+\tgit -C submodule add .gitattributes &&\n+\trm submodule/.gitattributes &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tsubmodule/a:(1|2)x(3|4)\n+\tEOF\n+\tgit grep --textconv --recurse-submodules x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'grep --textconv corectly reads submodule .git/info/attributes' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\n+\t# Workaround: we use --path-format=relative because the absolute path\n+\t# contains whitespaces and that seems to confuse test_when_finished\n+\t#\n+\tsubmodule_attr=\"submodule/$(git -C submodule rev-parse --path-format=relative --git-path info/attributes)\" &&\n+\ttest_when_finished rm -f \"$submodule_attr\" &&\n+\techo \"a diff=d2x\" >\"$submodule_attr\" &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tsubmodule/a:(1|2)x(3|4)\n+\tEOF\n+\tgit grep --textconv --recurse-submodules x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'grep saves textconv cache in the appropriated repository' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x_cached.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\ttest_config_global diff.d2x_cached.cachetextconv true &&\n+\techo \"a diff=d2x_cached\" >submodule/.gitattributes &&\n+\n+\t# Note: we only read/write to the textconv cache when grepping from an\n+\t# OID as the working tree file might have modifications. That is why\n+\t# we use --cached here.\n+\t#\n+\tgit grep --textconv --cached --recurse-submodules x &&\n+\ttest_path_is_missing \"$(git rev-parse --git-path refs/notes/textconv/d2x_cached)\" &&\n+\ttest_path_is_file \"$(git -C submodule rev-parse --git-path refs/notes/textconv/d2x_cached)\"\n+'\n+\n test_done\n--\n\nJunio seemed to agree that the behavior I described above is not the\ncorrect one:\n\n\"None of the attributes defined in the superproject\nshould affect the paths in the submodule, as it is a totally\nseparate project, oblivious to the existence of enclosing the\nsuperproject.\"\n\nBut he raised an important concern regarding how to fix this without\naffecting [too much] performance:\n\n\"As there is only one attribute cache IIUC, invalidating\nthe whole cache for the top-level and replacing it with the one for\na submodule, every time we cross the module boundary, would probably\nhave a negative effect on the performance, and I am not sure what\nwould happen if you run more than one threads working in different\nrepositories (i.e. top-level and submodules).\"\n\nMaybe we would need a different attributes stack for each repository?\n"},{"id":"437167","messageId":"87zgryylfx.fsf@evledraar.gmail.com","threadId":"56248","inReplyTo":"296230071f2cabda213b2a5f6f435f9308718569.1632754555.git.matheus.bernardino@usp.br","subject":"Re: [RFC PATCH 0/3] grep: don'\\''t add subrepos to in-memory alternates","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-27T17:30:56Z","receivedAt":"2021-09-27T17:46:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 27 2021, Matheus Tavares wrote:\n\n> On Mon, Sep 27, 2021 at 9:09 AM Ævar Arnfjörð Bjarmason wrote:\n>>\n>>\n>> On Mon, Aug 16 2021, Jonathan Tan wrote:\n>>\n>> > Record the repository whenever an OID grep source is created, and teach\n>> > the worker threads to explicitly provide the repository when accessing\n>> > objects.\n>> > [...]\n>> > diff --git a/grep.h b/grep.h\n>> > index 480b3f5bba..128007db65 100644\n>> > --- a/grep.h\n>> > +++ b/grep.h\n>> > @@ -120,7 +120,20 @@ struct grep_opt {\n>> >       struct grep_pat *header_list;\n>> >       struct grep_pat **header_tail;\n>> >       struct grep_expr *pattern_expression;\n>> > +\n>> > +     /*\n>> > +      * NEEDSWORK: See if we can remove this field, because the repository\n>> > +      * should probably be per-source. That is, grep.c functions using this\n>> > +      * field should probably start using \"repo\" in \"struct grep_source\"\n>> > +      * instead.\n>> > +      *\n>> > +      * This is potentially the cause of at least one bug - \"git grep\"\n>> > +      * ignoring the textconv attributes from submodules. See [1] for more\n>> > +      * information.\n>> > +      * [1] https://lore.kernel.org/git/CAHd-oW5iEQarYVxEXoTG-ua2zdoybTrSjCBKtO0YT292fm0NQQ@mail.gmail.com/\n>> > +      */\n>> >       struct repository *repo;\n>> > +\n>>\n>> I ran into this comment and read the linked E-Mail, and then the\n>> downthread\n>> https://lore.kernel.org/git/CAHd-oW6uG1fap-T4UF17bJmjoHAqWCDq9KbY+_8a3cEnnfATxg@mail.gmail.com/;\n>>\n>> Given Matheus's \"I've somehow missed this guard and the...\" there I'm\n>> not quite sure what/if we should be doing here & what this comment is\n>> recommending? I.e. do we still need to adjust the call chains as noted\n>> in the E-Mail the comment links to, or not?\n>\n> I think we should still adjust the call chains, yes. The downthread\n> message you mentioned is kind of a tangent about performance, where\n> Junio helped me understand something I had previously missed in the\n> code, regarding the persistence of the attributes stack.\n>\n> But the issue that started the thread was about a correctness problem:\n> the superproject textconv attributes are being used on submodules'\n> files when running `git grep` with `--recurse-submodules --textconv`.\n> The three cases to consider are:\n>\n> - .gitattributes from the working tree\n> - .gitattributes from the index\n> - .git/info/attributes\n>\n> On all these cases, the superproject attributes are being used on the\n> submodule. Additionally, if the superproject does not define any\n> attribute, the submodule attributes are being ignored in all cases\n> except by the first one (but that is only because the code sees the\n> .gitattributes file on the submodule as if it were a \"regular\"\n> subdirectory of the surperproject. So the submodule's .gitattribures\n> takes higher precedence when evaluating the attributes for files in\n> that directory).\n>\n> Another issue is that the textconv cache is always saved to (and read\n> from) the superproject gitdir, even for submodules' files.\n>\n> Here are some test cases that demonstrate these issues:\n>\n> -- snipsnap --\n> diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\n> index 3172f5b936..d01a3bc5d8 100755\n> --- a/t/t7814-grep-recurse-submodules.sh\n> +++ b/t/t7814-grep-recurse-submodules.sh\n> @@ -441,4 +441,104 @@ test_expect_success 'grep --recurse-submodules with --cached ignores worktree mo\n> \ttest_must_fail git grep --recurse-submodules --cached \"A modified line in submodule\" >actual 2>&1 &&\n> \ttest_must_be_empty actual\n> '\n> +\n> +test_expect_failure 'grep --textconv: superproject .gitattributes does not affect submodules' '\n> +\treset_and_clean &&\n> +\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n> +\techo \"a diff=d2x\" >.gitattributes &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\ta:(1|2)x(3|4)\n> +\tEOF\n> +\tgit grep --textconv --recurse-submodules x >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_failure 'grep --textconv: superproject .gitattributes (from index) does not affect submodules' '\n> +\treset_and_clean &&\n> +\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n> +\techo \"a diff=d2x\" >.gitattributes &&\n> +\tgit add .gitattributes &&\n> +\trm .gitattributes &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\ta:(1|2)x(3|4)\n> +\tEOF\n> +\tgit grep --textconv --recurse-submodules x >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_failure 'grep --textconv: superproject .git/info/attributes does not affect submodules' '\n> +\treset_and_clean &&\n> +\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n> +\tsuper_attr=\"$(git rev-parse --path-format=relative --git-path info/attributes)\" &&\n> +\ttest_when_finished rm -f \"$super_attr\" &&\n> +\techo \"a diff=d2x\" >\"$super_attr\" &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\ta:(1|2)x(3|4)\n> +\tEOF\n> +\tgit grep --textconv --recurse-submodules x >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'grep --textconv corectly reads submodule .gitattributes' '\n> +\treset_and_clean &&\n> +\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n> +\techo \"a diff=d2x\" >submodule/.gitattributes &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\tsubmodule/a:(1|2)x(3|4)\n> +\tEOF\n> +\tgit grep --textconv --recurse-submodules x >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_failure 'grep --textconv corectly reads submodule .gitattributes (from index)' '\n> +\treset_and_clean &&\n> +\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n> +\techo \"a diff=d2x\" >submodule/.gitattributes &&\n> +\tgit -C submodule add .gitattributes &&\n> +\trm submodule/.gitattributes &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\tsubmodule/a:(1|2)x(3|4)\n> +\tEOF\n> +\tgit grep --textconv --recurse-submodules x >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_failure 'grep --textconv corectly reads submodule .git/info/attributes' '\n> +\treset_and_clean &&\n> +\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n> +\n> +\t# Workaround: we use --path-format=relative because the absolute path\n> +\t# contains whitespaces and that seems to confuse test_when_finished\n> +\t#\n> +\tsubmodule_attr=\"submodule/$(git -C submodule rev-parse --path-format=relative --git-path info/attributes)\" &&\n> +\ttest_when_finished rm -f \"$submodule_attr\" &&\n> +\techo \"a diff=d2x\" >\"$submodule_attr\" &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\tsubmodule/a:(1|2)x(3|4)\n> +\tEOF\n> +\tgit grep --textconv --recurse-submodules x >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_failure 'grep saves textconv cache in the appropriated repository' '\n> +\treset_and_clean &&\n> +\ttest_config_global diff.d2x_cached.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n> +\ttest_config_global diff.d2x_cached.cachetextconv true &&\n> +\techo \"a diff=d2x_cached\" >submodule/.gitattributes &&\n> +\n> +\t# Note: we only read/write to the textconv cache when grepping from an\n> +\t# OID as the working tree file might have modifications. That is why\n> +\t# we use --cached here.\n> +\t#\n> +\tgit grep --textconv --cached --recurse-submodules x &&\n> +\ttest_path_is_missing \"$(git rev-parse --git-path refs/notes/textconv/d2x_cached)\" &&\n> +\ttest_path_is_file \"$(git -C submodule rev-parse --git-path refs/notes/textconv/d2x_cached)\"\n> +'\n> +\n> test_done\n\nThanks! I think it would be very good to have these tests in-tree along\nwith an updated comment pointing to them.\n\n"}]}