From: Jonathan Tan <jonathantanmy@google.com>
To: Brandon Williams <bmwill@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 4/5] grep: optionally recurse into submodules
Date: Fri, 4 Nov 2016 22:09:00 -0700 [thread overview]
Message-ID: <CAGf8dgKMEm0JUqMV=yfhAfDxyPnp2Xoq9-RKDVTMomcMnPr2_w@mail.gmail.com> (raw)
In-Reply-To: <20161027223834.35312-5-bmwill@google.com>
On Thu, Oct 27, 2016 at 3:38 PM, Brandon Williams <bmwill@google.com> wrote:
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 8887b6add..f34f16df9 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -18,12 +18,20 @@
> #include "quote.h"
> #include "dir.h"
> #include "pathspec.h"
> +#include "submodule.h"
>
> static char const * const grep_usage[] = {
> N_("git grep [<options>] [-e] <pattern> [<rev>...] [[--] <path>...]"),
> NULL
> };
>
> +static const char *super_prefix;
I think that the super_prefix changes could be in its own patch.
> +static int recurse_submodules;
> +static struct argv_array submodule_options = ARGV_ARRAY_INIT;
I guess this has to be static because it is shared by multiple threads.
> +
> +static int grep_submodule_launch(struct grep_opt *opt,
> + const struct grep_source *gs);
> +
> #define GREP_NUM_THREADS_DEFAULT 8
> static int num_threads;
>
> @@ -174,7 +182,10 @@ static void *run(void *arg)
> break;
>
> opt->output_priv = w;
> - hit |= grep_source(opt, &w->source);
> + if (w->source.type == GREP_SOURCE_SUBMODULE)
> + hit |= grep_submodule_launch(opt, &w->source);
> + else
> + hit |= grep_source(opt, &w->source);
It seems to me that GREP_SOURCE_SUBMODULE is of a different nature
than the other GREP_SOURCE_.* - in struct work_item, could we instead
have another variable that distinguishes between submodules and
"native" sources? This might also assuage Junio's concerns in
<xmqq37jbqf83.fsf@gitster.mtv.corp.google.com> about the nature of the
sources.
That variable could also be the discriminant for a tagged union, such
that we have "struct grep_source" for the "native" sources and a new
struct (holding only submodule-relevant information) for the
submodule.
> +/*
> + * Prep grep structures for a submodule grep
> + * sha1: the sha1 of the submodule or NULL if using the working tree
> + * filename: name of the submodule including tree name of parent
> + * path: location of the submodule
> + */
> +static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,
> + const char *filename, const char *path)
> +{
> + if (!(is_submodule_initialized(path) &&
> + is_submodule_checked_out(path))) {
> + warning("skiping submodule '%s%s' since it is not initialized and checked out",
> + super_prefix ? super_prefix: "",
> + path);
> + return 0;
> + }
> +
> +#ifndef NO_PTHREADS
> + if (num_threads) {
> + add_work(opt, GREP_SOURCE_SUBMODULE, filename, path, sha1);
> + return 0;
> + } else
> +#endif
> + {
> + struct work_item w;
> + int hit;
> +
> + grep_source_init(&w.source, GREP_SOURCE_SUBMODULE,
> + filename, path, sha1);
> + strbuf_init(&w.out, 0);
> + opt->output_priv = &w;
> + hit = grep_submodule_launch(opt, &w.source);
> +
> + write_or_die(1, w.out.buf, w.out.len);
> +
> + grep_source_clear(&w.source);
> + strbuf_release(&w.out);
> + return hit;
> + }
This is at least the third invocation of this "if pthreads, add work,
otherwise do it now" pattern - could this be extracted into its own
function (in another patch)? Ideally, there would also be exactly one
function in which the grep_source.* functions are invoked, and both
"run" and the non-pthread code path can use it.
> +}
> +
> +static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec,
> + int cached)
This line isn't modified other than the line break, as far as I can
tell, so I wouldn't break it.
> diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
> new file mode 100755
> index 000000000..b670c70cb
> --- /dev/null
> +++ b/t/t7814-grep-recurse-submodules.sh
> @@ -0,0 +1,99 @@
> +#!/bin/sh
> +
> +test_description='Test grep recurse-submodules feature
> +
> +This test verifies the recurse-submodules feature correctly greps across
> +submodules.
> +'
> +
> +. ./test-lib.sh
> +
Would it be possible to also test it while num_threads is zero? (Or,
if num_threads is already zero, to test it while it is not zero?)
next prev parent reply other threads:[~2016-11-05 5:09 UTC|newest]
Thread overview: 126+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-27 22:38 [RFC PATCH 0/5] recursively grep across submodules Brandon Williams
2016-10-27 22:38 ` [PATCH 1/5] submodules: add helper functions to determine presence of submodules Brandon Williams
2016-10-27 22:38 ` [PATCH 2/5] submodules: load gitmodules file from commit sha1 Brandon Williams
2016-10-27 22:38 ` [PATCH 3/5] grep: add submodules as a grep source type Brandon Williams
2016-10-27 22:38 ` [PATCH 4/5] grep: optionally recurse into submodules Brandon Williams
2016-11-05 5:09 ` Jonathan Tan [this message]
2016-10-27 22:38 ` [PATCH 5/5] grep: enable recurse-submodules to work on <tree> objects Brandon Williams
2016-10-28 19:35 ` Brandon Williams
2016-10-27 23:26 ` [RFC PATCH 0/5] recursively grep across submodules Junio C Hamano
2016-10-28 0:59 ` Stefan Beller
2016-10-28 2:50 ` Junio C Hamano
2016-10-28 3:46 ` Stefan Beller
2016-10-28 15:06 ` Philip Oakley
2016-10-28 17:02 ` Brandon Williams
2016-10-28 17:21 ` Junio C Hamano
2016-10-31 22:38 ` [PATCH v2 0/6] " Brandon Williams
2016-10-31 22:38 ` [PATCH v2 1/6] submodules: add helper functions to determine presence of submodules Brandon Williams
2016-10-31 23:34 ` Stefan Beller
2016-11-01 17:20 ` Junio C Hamano
2016-11-01 17:24 ` Brandon Williams
2016-11-01 17:31 ` Stefan Beller
2016-11-06 7:42 ` Jacob Keller
2016-11-01 17:23 ` Brandon Williams
2016-11-05 2:34 ` Jonathan Tan
2016-10-31 22:38 ` [PATCH v2 2/6] submodules: load gitmodules file from commit sha1 Brandon Williams
2016-11-01 16:39 ` Stefan Beller
2016-10-31 22:38 ` [PATCH v2 3/6] grep: add submodules as a grep source type Brandon Williams
2016-11-01 16:53 ` Stefan Beller
2016-11-01 17:31 ` Junio C Hamano
2016-10-31 22:38 ` [PATCH v2 4/6] grep: optionally recurse into submodules Brandon Williams
2016-11-01 17:26 ` Stefan Beller
2016-11-01 20:25 ` Brandon Williams
2016-10-31 22:38 ` [PATCH v2 5/6] grep: enable recurse-submodules to work on <tree> objects Brandon Williams
2016-11-11 23:09 ` Jonathan Tan
2016-10-31 22:38 ` [PATCH v2 6/6] grep: search history of moved submodules Brandon Williams
2016-11-11 23:51 ` [PATCH v3 0/6] recursively grep across submodules Brandon Williams
2016-11-11 23:51 ` [PATCH v3 1/6] submodules: add helper functions to determine presence of submodules Brandon Williams
2016-11-15 23:49 ` Stefan Beller
2016-11-11 23:51 ` [PATCH v3 2/6] submodules: load gitmodules file from commit sha1 Brandon Williams
2016-11-12 0:22 ` Stefan Beller
2016-11-11 23:51 ` [PATCH v3 3/6] grep: add submodules as a grep source type Brandon Williams
2016-11-11 23:51 ` [PATCH v3 4/6] grep: optionally recurse into submodules Brandon Williams
2016-11-16 0:07 ` Stefan Beller
2016-11-17 22:13 ` Brandon Williams
2016-11-11 23:51 ` [PATCH v3 5/6] grep: enable recurse-submodules to work on <tree> objects Brandon Williams
2016-11-14 18:10 ` Junio C Hamano
2016-11-14 18:44 ` Jonathan Tan
2016-11-14 18:56 ` Junio C Hamano
2016-11-14 19:08 ` Jonathan Tan
2016-11-14 19:14 ` Brandon Williams
2016-11-16 1:09 ` Stefan Beller
2016-11-17 23:34 ` Brandon Williams
2016-11-11 23:51 ` [PATCH v3 6/6] grep: search history of moved submodules Brandon Williams
2016-11-12 0:30 ` Stefan Beller
2016-11-14 17:43 ` Brandon Williams
2016-11-15 17:42 ` [PATCH v3 0/6] recursively grep across submodules Stefan Beller
2016-11-18 19:58 ` [PATCH v4 " Brandon Williams
2016-11-18 19:58 ` [PATCH v4 1/6] submodules: add helper functions to determine presence of submodules Brandon Williams
2016-11-18 19:58 ` [PATCH v4 2/6] submodules: load gitmodules file from commit sha1 Brandon Williams
2016-11-18 19:58 ` [PATCH v4 3/6] grep: add submodules as a grep source type Brandon Williams
2016-11-18 21:37 ` Junio C Hamano
2016-11-18 22:56 ` Brandon Williams
2016-11-18 19:58 ` [PATCH v4 4/6] grep: optionally recurse into submodules Brandon Williams
2016-11-18 21:48 ` Junio C Hamano
2016-11-18 22:01 ` Junio C Hamano
2016-11-18 22:14 ` Junio C Hamano
2016-11-18 22:58 ` Brandon Williams
2016-11-18 19:58 ` [PATCH v4 5/6] grep: enable recurse-submodules to work on <tree> objects Brandon Williams
2016-11-18 22:19 ` Junio C Hamano
2016-11-18 22:52 ` Brandon Williams
2016-11-21 18:14 ` Brandon Williams
2016-11-18 19:58 ` [PATCH v4 6/6] grep: search history of moved submodules Brandon Williams
2016-11-18 20:10 ` [PATCH v4 0/6] recursively grep across submodules Stefan Beller
2016-11-22 18:46 ` [PATCH v5 " Brandon Williams
2016-11-22 18:46 ` [PATCH v5 1/6] submodules: add helper functions to determine presence of submodules Brandon Williams
2016-11-22 18:46 ` [PATCH v5 2/6] submodules: load gitmodules file from commit sha1 Brandon Williams
2016-11-22 18:46 ` [PATCH v5 3/6] grep: add submodules as a grep source type Brandon Williams
2016-11-22 18:46 ` [PATCH v5 4/6] grep: optionally recurse into submodules Brandon Williams
2016-11-22 18:46 ` [PATCH v5 5/6] grep: enable recurse-submodules to work on <tree> objects Brandon Williams
2016-11-22 22:59 ` Junio C Hamano
2016-11-22 23:21 ` Brandon Williams
2016-11-22 23:28 ` Brandon Williams
2016-11-22 23:37 ` Junio C Hamano
2016-11-22 23:54 ` Brandon Williams
2016-11-22 18:46 ` [PATCH v5 6/6] grep: search history of moved submodules Brandon Williams
2016-12-01 1:28 ` [PATCH v6 0/6] recursively grep across submodules Brandon Williams
2016-12-01 1:28 ` [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules Brandon Williams
2016-12-01 4:29 ` Jeff King
2016-12-01 18:31 ` Stefan Beller
2016-12-01 18:46 ` Junio C Hamano
2016-12-01 19:09 ` Jeff King
2016-12-01 19:16 ` Brandon Williams
2016-12-01 20:54 ` Brandon Williams
2016-12-01 20:59 ` Jeff King
2016-12-01 21:56 ` Stefan Beller
2016-12-01 21:59 ` Jeff King
2016-12-02 18:36 ` Brandon Williams
2016-12-02 18:44 ` Jacob Keller
2016-12-02 18:49 ` Brandon Williams
2016-12-02 19:20 ` Jacob Keller
2016-12-02 19:28 ` Stefan Beller
2016-12-02 21:31 ` Jacob Keller
2016-12-02 21:46 ` Brandon Williams
2016-12-02 21:45 ` Jeff King
2016-12-03 0:16 ` Brandon Williams
2016-12-01 1:28 ` [PATCH v6 2/6] submodules: load gitmodules file from commit sha1 Brandon Williams
2016-12-01 1:28 ` [PATCH v6 3/6] grep: add submodules as a grep source type Brandon Williams
2016-12-01 1:28 ` [PATCH v6 4/6] grep: optionally recurse into submodules Brandon Williams
2016-12-01 1:28 ` [PATCH v6 5/6] grep: enable recurse-submodules to work on <tree> objects Brandon Williams
2016-12-01 7:25 ` Johannes Sixt
2016-12-01 17:51 ` Brandon Williams
2016-12-01 18:49 ` Junio C Hamano
2016-12-01 18:52 ` Jeff King
2016-12-01 1:28 ` [PATCH v6 6/6] grep: search history of moved submodules Brandon Williams
2016-12-01 4:22 ` [PATCH v6 0/6] recursively grep across submodules Jeff King
2016-12-01 17:45 ` Brandon Williams
2016-12-01 19:03 ` Jeff King
2016-12-16 19:03 ` [PATCH v7 0/7] " Brandon Williams
2016-12-16 19:03 ` [PATCH v7 1/7] submodules: add helper to determine if a submodule is populated Brandon Williams
2016-12-16 19:03 ` [PATCH v7 2/7] submodules: add helper to determine if a submodule is initialized Brandon Williams
2016-12-16 19:03 ` [PATCH v7 3/7] submodules: load gitmodules file from commit sha1 Brandon Williams
2016-12-16 19:03 ` [PATCH v7 4/7] grep: add submodules as a grep source type Brandon Williams
2016-12-16 19:03 ` [PATCH v7 5/7] grep: optionally recurse into submodules Brandon Williams
2016-12-16 19:03 ` [PATCH v7 6/7] grep: enable recurse-submodules to work on <tree> objects Brandon Williams
2016-12-16 19:03 ` [PATCH v7 7/7] grep: search history of moved submodules Brandon Williams
2016-12-16 21:42 ` [PATCH v7 0/7] recursively grep across submodules Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAGf8dgKMEm0JUqMV=yfhAfDxyPnp2Xoq9-RKDVTMomcMnPr2_w@mail.gmail.com' \
--to=jonathantanmy@google.com \
--cc=bmwill@google.com \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).