Re: [PATCH v2 2/2] grep: use '/' delimiter for paths

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 20, 2017, 22:18 UTC
Message-ID: <xmqqpojhwf2r.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170120171126.16269-3-stefanha@redhat.com>

Stefan Hajnoczi <stefanha@redhat.com> writes:

Show 32 quoted lines

> If the tree contains a sub-directory then git-grep(1) output contains a
> colon character instead of a path separator:
>
>   $ git grep malloc v2.9.3:t
>   v2.9.3:t:test-lib.sh:	setup_malloc_check () {
>   $ git show v2.9.3:t:test-lib.sh
>   fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'
>
> This patch attempts to use the correct delimiter:
>
>   $ git grep malloc v2.9.3:t
>   v2.9.3:t/test-lib.sh:	setup_malloc_check () {
>   $ git show v2.9.3:t/test-lib.sh
>   (success)
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  builtin/grep.c  | 4 +++-
>  t/t7810-grep.sh | 5 +++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 90a4f3d..7a7aab9 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -494,7 +494,9 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
>  
>  			/* Add a delimiter if there isn't one already */
>  			if (name[len - 1] != '/' && name[len - 1] != ':') {
> -				strbuf_addch(&base, ':');
> +				/* rev: or rev:path/ */
> +				char delim = obj->type == OBJ_COMMIT ? ':' : '/';

Why check the equality with commit, rather than un-equality with tree? Wouldn't you want to treat $commit:path and $tag:path the same way?

I also think the two patches should be squashed together, as it is only after this patch that the code does the "right thing" by changing the delimiter depending on obj->type.

Show 17 quoted lines

> +				strbuf_addch(&base, delim);
>  			}
>  		}
>  		init_tree_desc(&tree, data, size);
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index e804a3f..8a58d5e 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -1445,6 +1445,11 @@ test_expect_success 'grep outputs valid <rev>:<path> for HEAD:t/' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'grep outputs valid <rev>:<path> for HEAD:t' '
> +	git grep vvv HEAD:t >actual &&
> +	test_cmp expected actual
> +'
> +

Perhaps you want an annotated tag object refs/tags/v1.0 that points at the commit at the HEAD, and then run the same grep on v1.0:t, in addition to the above test.

>  cat >expected <<EOF
>  HEAD:t/a/v:vvv
>  HEAD:t/v:vvv