Re: [PATCH] grep: follow conventions for printing paths w/ unusual chars

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Matheus Tavares <matheus.bernardino@usp.br>
Cc: gitster@pobox.com, asheiduk@gmail.com, git@vger.kernel.org,
	greg@hurrell.net, l.s.r@web.de
Subject: Re: [PATCH] grep: follow conventions for printing paths w/ unusual chars
Date: Sat, 18 Apr 2020 15:13:11 +0200 (CEST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2004181509350.46@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <eaae7214925189f562056b1ee6972c05dcf76a32.1587103366.git.matheus.bernardino@usp.br>

Hi Matheus,

On Fri, 17 Apr 2020, Matheus Tavares wrote:

> grep does not follow the conventions used by other Git commands when
> printing paths that contain unusual characters (as double-quotes or
> newlines). Commands such as ls-files, commit, status and diff will:
>
> - Quote and escape unusual pathnames, by default.
> - Print names verbatim and unquoted when "-z" is used.
>
> But grep *never* quotes/escapes absolute paths with unusual chars and
> *always* quotes/escapes relative ones, even with "-z". Besides being
> inconsistent in its own output, the deviation from other Git commands
> can be confusing. So let's make it follow the two rules above and add
> some tests for this new behavior. Note that, making grep quote/escape
> all unusual paths by default, also make it fully compliant with the
> core.quotePath configuration, which is currently ignored for absolute
> paths.
>
> Reported-by: Greg Hurrell <greg@hurrell.net>
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---
>  Documentation/git-grep.txt |  6 +++--
>  builtin/grep.c             | 46 ++++++++++++++++++++++++++++----------
>  t/t7810-grep.sh            | 44 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 82 insertions(+), 14 deletions(-)

Unfortunately, this causes eight test failures on Windows:
https://dev.azure.com/gitgitgadget/git/_build/results?buildId=38023&view=ms.vss-test-web.build-test-results-tab

Could you please have a look? I suspect that this has something to do with
those new tests needing the `FUNNYNAMES` prereq.

Ciao,
Dscho

>
> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> index ddb6acc025..3109ce8fbe 100644
> --- a/Documentation/git-grep.txt
> +++ b/Documentation/git-grep.txt
> @@ -206,8 +206,10 @@ providing this option will cause it to die.
>
>  -z::
>  --null::
> -	Output \0 instead of the character that normally follows a
> -	file name.
> +	Use \0 as the delimiter for pathnames in the output, and print
> +	them verbatim. Without this option, pathnames with "unusual"
> +	characters are quoted as explained for the configuration
> +	variable core.quotePath (see git-config(1)).
>
>  -o::
>  --only-matching::
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 99e2685090..bdf1a4bbc9 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -295,6 +295,38 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)
>  	return st;
>  }
>
> +static void grep_source_name(struct grep_opt *opt, const char *filename,
> +			     int tree_name_len, struct strbuf *out)
> +{
> +	strbuf_reset(out);
> +
> +	if (opt->null_following_name) {
> +		if (opt->relative && opt->prefix_length) {
> +			struct strbuf rel_buf = STRBUF_INIT;
> +			const char *rel_name =
> +				relative_path(filename + tree_name_len,
> +					      opt->prefix, &rel_buf);
> +
> +			if (tree_name_len)
> +				strbuf_add(out, filename, tree_name_len);
> +
> +			strbuf_addstr(out, rel_name);
> +			strbuf_release(&rel_buf);
> +		} else {
> +			strbuf_addstr(out, filename);
> +		}
> +		return;
> +	}
> +
> +	if (opt->relative && opt->prefix_length)
> +		quote_path_relative(filename + tree_name_len, opt->prefix, out);
> +	else
> +		quote_c_style(filename + tree_name_len, out, NULL, 0);
> +
> +	if (tree_name_len)
> +		strbuf_insert(out, 0, filename, tree_name_len);
> +}
> +
>  static int grep_oid(struct grep_opt *opt, const struct object_id *oid,
>  		     const char *filename, int tree_name_len,
>  		     const char *path)
> @@ -302,13 +334,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,
>  	struct strbuf pathbuf = STRBUF_INIT;
>  	struct grep_source gs;
>
> -	if (opt->relative && opt->prefix_length) {
> -		quote_path_relative(filename + tree_name_len, opt->prefix, &pathbuf);
> -		strbuf_insert(&pathbuf, 0, filename, tree_name_len);
> -	} else {
> -		strbuf_addstr(&pathbuf, filename);
> -	}
> -
> +	grep_source_name(opt, filename, tree_name_len, &pathbuf);
>  	grep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);
>  	strbuf_release(&pathbuf);
>
> @@ -334,11 +360,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)
>  	struct strbuf buf = STRBUF_INIT;
>  	struct grep_source gs;
>
> -	if (opt->relative && opt->prefix_length)
> -		quote_path_relative(filename, opt->prefix, &buf);
> -	else
> -		strbuf_addstr(&buf, filename);
> -
> +	grep_source_name(opt, filename, 0, &buf);
>  	grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);
>  	strbuf_release(&buf);
>
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 7d7b396c23..ab495dba28 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -72,6 +72,8 @@ test_expect_success setup '
>  	# Still a no-op.
>  	function dummy() {}
>  	EOF
> +	echo unusual >"\"unusual\" pathname" &&
> +	echo unusual >"t/nested \"unusual\" pathname" &&
>  	git add . &&
>  	test_tick &&
>  	git commit -m initial
> @@ -481,6 +483,48 @@ do
>  		git grep --count -h -e b $H -- ab >actual &&
>  		test_cmp expected actual
>  	'
> +
> +	test_expect_success "grep $L should quote unusual pathnames" '
> +		cat >expected <<-EOF &&
> +		${HC}"\"unusual\" pathname":unusual
> +		${HC}"t/nested \"unusual\" pathname":unusual
> +		EOF
> +		git grep unusual $H >actual &&
> +		test_cmp expected actual
> +	'
> +
> +	test_expect_success "grep $L in subdir should quote unusual relative pathnames" '
> +		cat >expected <<-EOF &&
> +		${HC}"nested \"unusual\" pathname":unusual
> +		EOF
> +		(
> +			cd t &&
> +			git grep unusual $H
> +		) >actual &&
> +		test_cmp expected actual
> +	'
> +
> +	test_expect_success "grep -z $L with unusual pathnames" '
> +		cat >expected <<-EOF &&
> +		${HC}"unusual" pathname:unusual
> +		${HC}t/nested "unusual" pathname:unusual
> +		EOF
> +		git grep -z unusual $H >actual &&
> +		tr "\0" ":" <actual >actual-replace-null &&
> +		test_cmp expected actual-replace-null
> +	'
> +
> +	test_expect_success "grep -z $L in subdir with unusual relative pathnames" '
> +		cat >expected <<-EOF &&
> +		${HC}nested "unusual" pathname:unusual
> +		EOF
> +		(
> +			cd t &&
> +			git grep -z unusual $H
> +		) >actual &&
> +		tr "\0" ":" <actual >actual-replace-null &&
> +		test_cmp expected actual-replace-null
> +	'
>  done
>
>  cat >expected <<EOF
> --
> 2.26.0
>
>
>