Re: [PATCH v3 5/6] grep: enable recurse-submodules to work on <tree> objects

From: Stefan Beller <sbeller@google.com>
Date: Nov 16, 2016, 01:09 UTC
Message-ID: <CAGZ79kbqZs+wQYUBnvdw1YhC4dtHeodJWdved+it2Zg076vyOA@mail.gmail.com>
In-Reply-To: <1478908273-190166-6-git-send-email-bmwill@google.com>

On Fri, Nov 11, 2016 at 3:51 PM, Brandon Williams <bmwill@google.com> wrote:

> to:
> HEAD:file
> HEAD:sub/file

  Maybe indent this ;)

Show 32 quoted lines

>  static struct argv_array submodule_options = ARGV_ARRAY_INIT;
> +static const char *parent_basename;
>
>  static int grep_submodule_launch(struct grep_opt *opt,
>                                  const struct grep_source *gs);
> @@ -535,19 +537,53 @@ static int grep_submodule_launch(struct grep_opt *opt,
>  {
>         struct child_process cp = CHILD_PROCESS_INIT;
>         int status, i;
> +       const char *end_of_base;
> +       const char *name;
>         struct work_item *w = opt->output_priv;
>
> +       end_of_base = strchr(gs->name, ':');
> +       if (end_of_base)
> +               name = end_of_base + 1;
> +       else
> +               name = gs->name;
> +
>         prepare_submodule_repo_env(&cp.env_array);
>
>         /* Add super prefix */
>         argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
>                          super_prefix ? super_prefix : "",
> -                        gs->name);
> +                        name);
>         argv_array_push(&cp.args, "grep");
>
> +       /*
> +        * Add basename of parent project
> +        * When performing grep on a <tree> object the filename is prefixed
> +        * with the object's name: '<tree-name>:filename'.

This comment is hard to read as it's unclear what the <angle brackets> mean. (Are the supposed to indicate a variable? If so why is file name not marked up?)

Show 9 quoted lines

>  In order to
> +        * provide uniformity of output we want to pass the name of the
> +        * parent project's object name to the submodule so the submodule can
> +        * prefix its output with the parent's name and not its own SHA1.
> +        */
> +       if (end_of_base)
> +               argv_array_pushf(&cp.args, "--parent-basename=%.*s",
> +                                (int) (end_of_base - gs->name),
> +                                gs->name);

Do we pass this only with the tree-ish? What if we are grepping the working tree and the file name contains a colon?

Show 30 quoted lines

> +test_expect_success 'grep tree HEAD^' '
> +       cat >expect <<-\EOF &&
> +       HEAD^:a:foobar
> +       HEAD^:b/b:bar
> +       HEAD^:submodule/a:foobar
> +       EOF
> +
> +       git grep -e "bar" --recurse-submodules HEAD^ > actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'grep tree HEAD^^' '
> +       cat >expect <<-\EOF &&
> +       HEAD^^:a:foobar
> +       HEAD^^:b/b:bar
> +       EOF
> +
> +       git grep -e "bar" --recurse-submodules HEAD^^ > actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'grep tree and pathspecs' '
> +       cat >expect <<-\EOF &&
> +       HEAD:submodule/a:foobar
> +       HEAD:submodule/sub/a:foobar
> +       EOF
> +
> +       git grep -e "bar" --recurse-submodules HEAD -- submodule > actual &&
> +       test_cmp expect actual
> +'

Mind to add tests for
* recursive submodules (say 2 levels), preferrably not having the
  gitlink at the root each, i.e. root has a sub1 at path subs/sub1 and
sub1 has a sub2
  at path subs/sub2, such that recursing would produce a path like
  HEAD:subs/sub1/subs/sub2/dir/file ?
* file names with a colon in it
* instead of just HEAD referencing trees, maybe a sha1 referenced test as well
  (though it is not immediately clear what the benefit would be)
* what if the submodule doesn't have the commit referenced in the given sha1

Thanks, Stefan