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

From: Duy Nguyen <pclouds@gmail.com>
Date: Feb 26, 2017, 09:53 UTC
Message-ID: <CACsJy8Df6hhTzx5BNx385T4cuCg5w3nvOioXB=q+NCoW9kA6_w@mail.gmail.com>
In-Reply-To: <20170224235100.52627-2-bmwill@google.com>

On Sat, Feb 25, 2017 at 6:50 AM, Brandon Williams <bmwill@google.com> wrote:

Show 10 quoted lines

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

Show 41 quoted lines

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

Show 9 quoted lines

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

Show 13 quoted lines

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