Re: [PATCH 1/5] grep: illustrate bug when recursing with relative pathspec

[Brandon Williams <bmwill@google.com>]

On 02/26, Duy Nguyen wrote:
> On Sat, Feb 25, 2017 at 6:50 AM, Brandon Williams <bmwill@google.com> wrote:
> > When using the --recurse-submodules flag with a relative pathspec which
> > includes "..", an error is produced inside the child process spawned for
> > a submodule.  When creating the pathspec struct in the child, the ".."
> > is interpreted to mean "go up a directory" which causes an error stating
> > that the path ".." is outside of the repository.
> >
> > While it is true that ".." is outside the scope of the submodule, it is
> > confusing to a user who originally invoked the command where ".." was
> > indeed still inside the scope of the superproject.  Since the child
> > process luanched for the submodule has some context that it is operating
> 
> s/luanched/launched/
> 
> I would prefer 1/5 t to be merged with 3/5 though. The problem

We can definitely merge them and I agree that it may be easier for
looking through the logs if they are merged.

> description is very light there, and the test demonstration in the
> diff is simply switching from failure to success, which forces the
> reader to come back here. It's easier to find here now, but it'll be a
> bit harder when it enters master and we have to read it from git-log,
> I think.
> 
> I'm still munching through the super-prefix patches. From how you
> changed match_pathspec call in 0281e487fd (grep: optionally recurse
> into submodules - 2016-12-16), I guess pathspecs should be handled
> with super-prefix instead of the submodule's prefix (which is empty
> anyway, I guess). The right solution wrt. handling relative paths may
> be teach pathspec about super-prefix (and even original super's cwd)
> then let it processes path in supermodule's context.
> 
> Does it handle relative paths with wildcards correctly btw? Ones that
> cross submodules? I have a feeling it doesn't, but I haven't seen how
> exactly super-prefix works yet.

I'm not 100% sure about the relative paths with wildcards.  I did notice
that this series solves one problem and introduces another (recursing
from a subdirectory doesn't work with this series) so I need to
rethink the solution a little bit.  And I'll take into account wildcards
as well.

> 
> There's another problem with passing pathspec from one process to
> another. The issue with preserving the prefix, see 233c3e6c59
> (parse_pathspec: preserve prefix length via PATHSPEC_PREFIX_ORIGIN -
> 2013-07-14). :(icase) needs this because given a path
> "<prefix>/foobar", only the "foobar" part is considered case
> insensitive, the prefix part is always case-sensitive. For example, if
> you have 4 paths "abc/def", "abc/DEF", "ABC/def" and "ABC/DEF" and are
> standing at "abc", you would want ":(icase)def" to match the first two
> only, not all of them.

Hmm...yeah its a really difficult thing to get 100% correct (as I'm now
noticing).  It also makes it a little more challenging because you don't
have the same state in both processes (child and parent).  I'm thinking
I may have to a little bit more work, which is more involved, to
properly handle the pathspecs passed to the children.  As in we may not
be able to pass the raw pathspec used in the parent to the child and
instead may have to do a little pre-processing on it.

> > underneath a superproject, this error could be avoided.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >  t/t7814-grep-recurse-submodules.sh | 42 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >
> > diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
> > index 67247a01d..418ba68fe 100755
> > --- a/t/t7814-grep-recurse-submodules.sh
> > +++ b/t/t7814-grep-recurse-submodules.sh
> > @@ -227,6 +227,48 @@ test_expect_success 'grep history with moved submoules' '
> >         test_cmp expect actual
> >  '
> >
> > +test_expect_failure 'grep using relative path' '
> > +       test_when_finished "rm -rf parent sub" &&
> > +       git init sub &&
> > +       echo "foobar" >sub/file &&
> > +       git -C sub add file &&
> > +       git -C sub commit -m "add file" &&
> > +
> > +       git init parent &&
> > +       echo "foobar" >parent/file &&
> > +       git -C parent add file &&
> > +       mkdir parent/src &&
> > +       echo "foobar" >parent/src/file2 &&
> > +       git -C parent add src/file2 &&
> > +       git -C parent submodule add ../sub &&
> > +       git -C parent commit -m "add files and submodule" &&
> > +
> > +       # From top works
> > +       cat >expect <<-\EOF &&
> > +       file:foobar
> > +       src/file2:foobar
> > +       sub/file:foobar
> > +       EOF
> > +       git -C parent grep --recurse-submodules -e "foobar" >actual &&
> > +       test_cmp expect actual &&
> > +
> > +       # Relative path to top errors out
> 
> After 3/5, it's not "errors out" any more, is it?
> 

Correct, will fix the comment.

> > +       cat >expect <<-\EOF &&
> > +       ../file:foobar
> > +       file2:foobar
> > +       ../sub/file:foobar
> > +       EOF
> > +       git -C parent/src grep --recurse-submodules -e "foobar" -- .. >actual &&
> > +       test_cmp expect actual &&
> > +
> > +       # Relative path to submodule errors out
> 
> ditto
> 
> > +       cat >expect <<-\EOF &&
> > +       ../sub/file:foobar
> > +       EOF
> > +       git -C parent/src grep --recurse-submodules -e "foobar" -- ../sub >actual &&
> > +       test_cmp expect actual
> > +'
> > +
> >  test_incompatible_with_recurse_submodules ()
> >  {
> >         test_expect_success "--recurse-submodules and $1 are incompatible" "
> > --
> > 2.11.0.483.g087da7b7c-goog
> >
> 
> 
> 
> -- 
> Duy

-- 
Brandon Williams