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

[Duy Nguyen <pclouds@gmail.com>]

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

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.

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

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