Re: [PATCH 2/2] t4010: match_pathspec_depth() and trailing slash after submodule

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 23, 2014, 21:09 UTC
Message-ID: <xmqqa9emtngr.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1390483326-32258-2-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> When you do "git diff HEAD submodule/", "submodule" from the index is
> picked out and match_pathspec_depth() in charge of matching it with
> the pathspec "submodule/".

Is "... is called" or something missing at the end of this sentence?

> Unlike tree_entry_interesting(), match_pathspec_depth() has no
> knowledge about entry mode to realize "submodule" is a directory and
> treat the trailing slash specially. And it does not have too, mostly,

s/too/to/, I think.

Show 23 quoted lines

> because the index only contains files, not directories (not until
> submodules come)
>
> I have no solutions for it (no, stripping '/' at pathspec
> preprocessing phase seems like a workaround than a solution). So let's
> mark it. Maybe I or somebody else could revisit it later.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  t/t4010-diff-pathspec.sh | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh
> index 15a4912..b54251a 100755
> --- a/t/t4010-diff-pathspec.sh
> +++ b/t/t4010-diff-pathspec.sh
> @@ -127,4 +127,10 @@ test_expect_success 'diff-tree ignores trailing slash on submodule path' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_failure 'diff-cache ignores trailing slash on submodule path' '
> +	git diff --name-only HEAD^ submod >expect &&
> +	git diff --name-only HEAD^ submod/ >actual &&

I actually doubt that the second line is expecting the right behaviour in the first place. As far as the top-level project is concerned, "submod" is the name it wants, as there is nothing underneath it. Even if asked to recurse infinite levels, the caller shouldn't be feeding paths like "submod/a/b/c" to match_pathspec_depth() in the first place, no?

> +	test_cmp expect actual
> +'
> +
>  test_done