{"thread":{"id":"49847","subject":"[PATCH] grep: use grep_opt->repo instead of explict repo argument","startedAt":"2018-11-18T16:39:02Z","lastAt":"2018-11-26T19:52:38Z","messageCount":2,"participants":["Nguyễn Thái Ngọc Duy","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"363582","messageId":"20181118163851.32178-1-pclouds@gmail.com","threadId":"49847","inReplyTo":null,"subject":"[PATCH] grep: use grep_opt->repo instead of explict repo argument","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-18T16:38:51Z","receivedAt":"2018-11-18T16:39:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This command is probably the first one that operates on a repository\nother than the_repository, in f9ee2fcdfa (grep: recurse in-process\nusing 'struct repository' - 2017-08-02). An explicit 'struct\nrepository *' was added in that commit to pass around the repository\nthat we're supposed to grep from.\n\nSince 38bbc2ea39 (grep.c: remove implicit dependency on the_index -\n2018-09-21). 'struct grep_opt *' carries in itself a repository\nparameter for grepping. We should now be able to reuse grep_opt to\nhold the submodule repo instead of a separate argument, which is just\nroom for mistakes.\n\nWhile at there, use the right reference instead of the_repository and\nthe_index in this code. I was a bit careless in my attempt to kick\nthe_repository / the_index out of library code. It's normally safe to\njust stick the_repository / the_index in bultin/ code, but it's not\nthe case for grep.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/grep.c | 41 ++++++++++++++++++++++++-----------------\n 1 file changed, 24 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 56e4a11052..bdc49cd34e 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -408,18 +408,20 @@ static void run_pager(struct grep_opt *opt, const char *prefix)\n \t\texit(status);\n }\n \n-static int grep_cache(struct grep_opt *opt, struct repository *repo,\n+static int grep_cache(struct grep_opt *opt,\n \t\t      const struct pathspec *pathspec, int cached);\n static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t     struct tree_desc *tree, struct strbuf *base, int tn_len,\n-\t\t     int check_attr, struct repository *repo);\n+\t\t     int check_attr);\n \n-static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n+static int grep_submodule(struct grep_opt *opt,\n \t\t\t  const struct pathspec *pathspec,\n \t\t\t  const struct object_id *oid,\n \t\t\t  const char *filename, const char *path)\n {\n+\tstruct repository *superproject = opt->repo;\n \tstruct repository submodule;\n+\tstruct grep_opt subopt;\n \tint hit;\n \n \t/*\n@@ -455,6 +457,9 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n \tadd_to_alternates_memory(submodule.objects->objectdir);\n \tgrep_read_unlock();\n \n+\tmemcpy(&subopt, opt, sizeof(subopt));\n+\tsubopt.repo = &submodule;\n+\n \tif (oid) {\n \t\tstruct object *object;\n \t\tstruct tree_desc tree;\n@@ -476,21 +481,22 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n \t\tstrbuf_addch(&base, '/');\n \n \t\tinit_tree_desc(&tree, data, size);\n-\t\thit = grep_tree(opt, pathspec, &tree, &base, base.len,\n-\t\t\t\tobject->type == OBJ_COMMIT, &submodule);\n+\t\thit = grep_tree(&subopt, pathspec, &tree, &base, base.len,\n+\t\t\t\tobject->type == OBJ_COMMIT);\n \t\tstrbuf_release(&base);\n \t\tfree(data);\n \t} else {\n-\t\thit = grep_cache(opt, &submodule, pathspec, 1);\n+\t\thit = grep_cache(&subopt, pathspec, 1);\n \t}\n \n \trepo_clear(&submodule);\n \treturn hit;\n }\n \n-static int grep_cache(struct grep_opt *opt, struct repository *repo,\n+static int grep_cache(struct grep_opt *opt,\n \t\t      const struct pathspec *pathspec, int cached)\n {\n+\tstruct repository *repo = opt->repo;\n \tint hit = 0;\n \tint nr;\n \tstruct strbuf name = STRBUF_INIT;\n@@ -528,7 +534,7 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,\n \t\t\t}\n \t\t} else if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&\n \t\t\t   submodule_path_match(repo->index, pathspec, name.buf, NULL)) {\n-\t\t\thit |= grep_submodule(opt, repo, pathspec, NULL, ce->name, ce->name);\n+\t\t\thit |= grep_submodule(opt, pathspec, NULL, ce->name, ce->name);\n \t\t} else {\n \t\t\tcontinue;\n \t\t}\n@@ -550,8 +556,9 @@ static int grep_cache(struct grep_opt *opt, struct repository *repo,\n \n static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t     struct tree_desc *tree, struct strbuf *base, int tn_len,\n-\t\t     int check_attr, struct repository *repo)\n+\t\t     int check_attr)\n {\n+\tstruct repository *repo = opt->repo;\n \tint hit = 0;\n \tenum interesting match = entry_not_interesting;\n \tstruct name_entry entry;\n@@ -597,10 +604,10 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t\tstrbuf_addch(base, '/');\n \t\t\tinit_tree_desc(&sub, data, size);\n \t\t\thit |= grep_tree(opt, pathspec, &sub, base, tn_len,\n-\t\t\t\t\t check_attr, repo);\n+\t\t\t\t\t check_attr);\n \t\t\tfree(data);\n \t\t} else if (recurse_submodules && S_ISGITLINK(entry.mode)) {\n-\t\t\thit |= grep_submodule(opt, repo, pathspec, entry.oid,\n+\t\t\thit |= grep_submodule(opt, pathspec, entry.oid,\n \t\t\t\t\t      base->buf, base->buf + tn_len);\n \t\t}\n \n@@ -642,7 +649,7 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t}\n \t\tinit_tree_desc(&tree, data, size);\n \t\thit = grep_tree(opt, pathspec, &tree, &base, base.len,\n-\t\t\t\tobj->type == OBJ_COMMIT, the_repository);\n+\t\t\t\tobj->type == OBJ_COMMIT);\n \t\tstrbuf_release(&base);\n \t\tfree(data);\n \t\treturn hit;\n@@ -659,12 +666,12 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,\n \n \tfor (i = 0; i < nr; i++) {\n \t\tstruct object *real_obj;\n-\t\treal_obj = deref_tag(the_repository, list->objects[i].item,\n+\t\treal_obj = deref_tag(opt->repo, list->objects[i].item,\n \t\t\t\t     NULL, 0);\n \n \t\t/* load the gitmodules file for this rev */\n \t\tif (recurse_submodules) {\n-\t\t\tsubmodule_free(the_repository);\n+\t\t\tsubmodule_free(opt->repo);\n \t\t\tgitmodules_config_oid(&real_obj->oid);\n \t\t}\n \t\tif (grep_object(opt, pathspec, real_obj, list->objects[i].name,\n@@ -689,9 +696,9 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,\n \tif (exc_std)\n \t\tsetup_standard_excludes(&dir);\n \n-\tfill_directory(&dir, &the_index, pathspec);\n+\tfill_directory(&dir, opt->repo->index, pathspec);\n \tfor (i = 0; i < dir.nr; i++) {\n-\t\tif (!dir_path_match(&the_index, dir.entries[i], pathspec, 0, NULL))\n+\t\tif (!dir_path_match(opt->repo->index, dir.entries[i], pathspec, 0, NULL))\n \t\t\tcontinue;\n \t\thit |= grep_file(opt, dir.entries[i]->name);\n \t\tif (hit && opt->status_only)\n@@ -1124,7 +1131,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tif (!cached)\n \t\t\tsetup_work_tree();\n \n-\t\thit = grep_cache(&opt, the_repository, &pathspec, cached);\n+\t\thit = grep_cache(&opt, &pathspec, cached);\n \t} else {\n \t\tif (cached)\n \t\t\tdie(_(\"both --cached and trees are given\"));\n-- \n2.19.1.1327.g328c130451.dirty\n\n"},{"id":"364094","messageId":"CAGZ79kZHFxahZMAZh2Ldscfqp0OWAae7dowAboLDac-UmTHwYw@mail.gmail.com","threadId":"49847","inReplyTo":"20181118163851.32178-1-pclouds@gmail.com","subject":"Re: [PATCH] grep: use grep_opt->repo instead of explict repo argument","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-26T19:52:23Z","receivedAt":"2018-11-26T19:52:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Nov 18, 2018 at 8:38 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:\n>\n> This command is probably the first one that operates on a repository\n> other than the_repository, in f9ee2fcdfa (grep: recurse in-process\n> using 'struct repository' - 2017-08-02). An explicit 'struct\n> repository *' was added in that commit to pass around the repository\n> that we're supposed to grep from.\n>\n> Since 38bbc2ea39 (grep.c: remove implicit dependency on the_index -\n> 2018-09-21). 'struct grep_opt *' carries in itself a repository\n> parameter for grepping. We should now be able to reuse grep_opt to\n> hold the submodule repo instead of a separate argument, which is just\n> room for mistakes.\n\nThat makes sense. Assuming we did not make a mistake yet, the\ntest suite would not need changes (further assuming we do test for it,\nbut we do as we have explicit submodule grep tests).\n\n>\n> While at there, use the right reference instead of the_repository and\n> the_index in this code. I was a bit careless in my attempt to kick\n> the_repository / the_index out of library code. It's normally safe to\n> just stick the_repository / the_index in bultin/ code, but it's not\n> the case for grep.\n\nReviewed-by: Stefan Beller <sbeller@google.com>\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n"}]}