Re: [PATCH v2 1/2] grep: only add delimiter if there isn't one already

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

Stefan Hajnoczi <stefanha@redhat.com> writes:

Show 14 quoted lines

> git-grep(1) output does not follow git's own syntax:
>
>   $ 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 avoids emitting the unnecessary ':' delimiter if the name
> already ends with ':' or '/':
>
>   $ git grep malloc v2.9.3:
>   v2.9.3:t/test-lib.sh:   setup_malloc_check () {
>   $ git show v2.9.3:t/test-lib.sh
>   (succeeds)

Show 21 quoted lines

>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  builtin/grep.c  |  6 +++++-
>  t/t7810-grep.sh | 21 +++++++++++++++++++++
>  2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 8887b6a..90a4f3d 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -491,7 +491,11 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
>  		strbuf_init(&base, PATH_MAX + len + 1);
>  		if (len) {
>  			strbuf_add(&base, name, len);
> -			strbuf_addch(&base, ':');
> +
> +			/* Add a delimiter if there isn't one already */
> +			if (name[len - 1] != '/' && name[len - 1] != ':') {
> +				strbuf_addch(&base, ':');
> +			}

I agree with peff and Brandon that checking treeness is the right way to make the decision, and you seem to have done that in the updated 2/2, but why doesn't it apply here as well?

>  		}
>  		init_tree_desc(&tree, data, size);
>  		hit = grep_tree(opt, pathspec, &tree, &base, base.len,