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

[Brandon Williams <bmwill@google.com>]

On 11/15, Stefan Beller wrote:
> On Fri, Nov 11, 2016 at 3:51 PM, Brandon Williams <bmwill@google.com> wrote:
> > +       /*
> > +        * 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?)

Yeah you're right, the angle brackets don't really add anything to the
comment.  I'll drop them.

> >  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?

Actually you're right, this would only happen if we are passing a
tree-ish, which has a tree-name prefixed to the filename.  I'll add that
as an additional check to ensure that this handles file names with a
colon correctly....though why you have a colon in a filename is beyond
me :P

> > +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

I'll add more tests too!

-- 
Brandon Williams