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

From: Junio C Hamano <[email-masked]>
Date: Thu, 19 Jan 2017 10:39:02 -0800

Stefan Hajnoczi <[email-masked]> writes:

> git-grep(1) output does not follow git's own syntax:
>
>   $ git grep malloc v2.9.3:
>   v2.9.3::Makefile:       COMPAT_CFLAGS += -Icompat/nedmalloc
>   $ git show v2.9.3::Makefile
>   fatal: Path ':Makefile' 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:Makefile:       COMPAT_CFLAGS += -Icompat/nedmalloc
>   $ git show v2.9.3:Makefile
>   (succeeds)

I am mildly negative on this one.  I suspect that the above example
is a made-up one and you might have a more compelling real-world use
case in mind, but at least the above does not convince me.

The end-user explicitly gave the extra ':' because she wanted to
feed the tree object, not a tag that leads to the tree, for some
reason.  I think the output should respect that and show how she
spelled the starting point.  IOW, it is not "':' added unnecessarily
by Git".  It is ':' added by the end-user for whatever reason.

>
> Signed-off-by: Stefan Hajnoczi <[email-masked]>
> ---
>  builtin/grep.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 462e607..3643d8a 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -490,7 +490,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, ':');
> +			}
>  		}
>  		init_tree_desc(&tree, data, size);
>  		hit = grep_tree(opt, pathspec, &tree, &base, base.len,