Re: [PATCH 4/5] grep: optionally recurse into submodules
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Nov 5, 2016, 05:09 UTC
- Message-ID
- <CAGf8dgKMEm0JUqMV=yfhAfDxyPnp2Xoq9-RKDVTMomcMnPr2_w@mail.gmail.com>
- In-Reply-To
- <20161027223834.35312-5-bmwill@google.com>
On Thu, Oct 27, 2016 at 3:38 PM, Brandon Williams <bmwill@google.com> wrote:
Show 16 quoted lines
> 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.
Show 16 quoted lines
> + > +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.
Show 39 quoted lines
> +/*
> + * 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.
Show 16 quoted lines
> 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?)