git-grep's "-z" option misbehaves in subdirectory

git-grep's "-z" option misbehaves in subdirectory

[Greg Hurrell]

Hi,

It seems that `git-grep -lz` behaves differently depending on whether
it is inside a subdirectory:

$ mkdir demo
$ cd demo
$ git init
$ echo content > 'an "example".txt'
$ mkdir nested
$ echo content > 'nested/other "example".txt'
$ git add .
$ git commit -m Initial
$ git grep -lz content
an "example".txt^@nested/other "example".txt^@

Note that, as expected, the files are NUL-terminated and not wrapped
in quotes. ("^@" represents NUL byte.)

$ cd nested
$ git grep -lz content
"other \"example\".txt"^@

As soon as we move into a subdirectory, files are wrapped in quotes
and contain escapes, despite the "-z" switch. git-ls-files doesn't
exhibit this behavior:

$ git ls-files -z
other "example".txt

And git-grep doesn't either, if you pass "--full-name":

$ git grep -lz --full-name content
nested/other "example".txt^@

Seeing this on Git v2.25.0 on macOS (10.13.6).

Does this seem like a bug, or expected behavior?

Cheers,
Greg

Re: git-grep's "-z" option misbehaves in subdirectory

[Junio C Hamano]

Greg Hurrell <greg@hurrell.net> writes:

> $ git grep -lz --full-name content
> nested/other "example".txt^@
>
> Seeing this on Git v2.25.0 on macOS (10.13.6).
>
> Does this seem like a bug, or expected behavior?

Sounds like a very ancient bug, possibly as old as the -z option
itself.

(no subject)

[Matheus Tavares]

On Mon, Apr 13, 2020 at 6:57 PM Greg Hurrell <greg@hurrell.net> wrote:
>
> It seems that `git-grep -lz` behaves differently depending on whether
> it is inside a subdirectory:
[...]
> $ git grep -lz content
> an "example".txt^@nested/other "example".txt^@
>
> Note that, as expected, the files are NUL-terminated and not wrapped
> in quotes. ("^@" represents NUL byte.)
>
> $ cd nested
> $ git grep -lz content
> "other \"example\".txt"^@
>
> As soon as we move into a subdirectory, files are wrapped in quotes
> and contain escapes, despite the "-z" switch.

I think this is a bug in "plain" git-grep, not in -z specifically. In some git
commands, -z has the effect of unquoting the output paths. For example, the
docs for git-ls-files says: "-z: \0 line termination on output and do not quote
filenames." In git-grep, however, the -z doc entry only says: "Output \0
instead of the character that normally follows a file name."

So -z does not seem to affect quoting in git-grep. But should we change this, to
quote unusual pathnames (relative or not) by default, and let -z fall back to
the old behavior? This would be more consistent with other commands such as
git-ls-files and to the core.quotePath setting.

However, perhaps output paths are never intended to be displayed quoted in
git-grep (with or without -z) in order to be consistent with GNU grep. I don't
know which of these options is correct (if any), so I would love to hear what
others have to say about it.

But if the second is correct, I think we can use the following patch to solve
the reported bug:

(I'm just wondering: should we also add the information at core.quotePath that
git-grep does not comply with this setting, to be consistent with GNU grep {or
for any other reason}?)

-- >8 --
Subject: [RFC PATCH] grep: fix inconsistent output of unusual pathnames

When grepping from the repository root, paths that contain unusual
characters are not output quoted. However, they are quoted when grepping
from a subdir without --full-name. For example:

$ echo content >'an "unusual" name.txt'
$ mkdir subdir
$ echo content >'subdir/another "unusual" name.txt'
$ git add .
$ git commit -m content

Then:
$ git grep content
an "unusual" name.txt:1:content
subdir/another "unusual" name.txt:1:content

But:
$ cd subdir
$ git grep content
"another \"unusual\" name.txt":1:content

Fix this inconsistency by not quoting unusual pathnames (relative or
not), which is also the default behavior in GNU grep. Also add some
tests to prevent regressions.

Note: some non-related tests that compare git-grep output against
git-ls-files also had to be modified. This is because the testing repo
now contains some unusual pathnames. And git-ls-files will quote such
paths by default, whereas git-grep won't.

Reported-by: Greg Hurrell <greg@hurrell.net>
Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 builtin/grep.c  | 15 ++++++++++-----
 t/t7810-grep.sh | 33 ++++++++++++++++++++++++++++++---
 2 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 99e2685090..ca21f98315 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -303,8 +303,12 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,
 	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);
+		struct strbuf rel_buf = STRBUF_INIT;
+		const char *rel_name = relative_path(filename + tree_name_len,
+						     opt->prefix, &rel_buf);
+		strbuf_add(&pathbuf, filename, tree_name_len);
+		strbuf_addstr(&pathbuf, rel_name);
+		strbuf_release(&rel_buf);
 	} else {
 		strbuf_addstr(&pathbuf, filename);
 	}
@@ -333,13 +337,14 @@ static int grep_file(struct grep_opt *opt, const char *filename)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct grep_source gs;
+	const char *gs_name;

 	if (opt->relative && opt->prefix_length)
-		quote_path_relative(filename, opt->prefix, &buf);
+		gs_name = relative_path(filename, opt->prefix, &buf);
 	else
-		strbuf_addstr(&buf, filename);
+		gs_name = filename;

-	grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);
+	grep_source_init(&gs, GREP_SOURCE_FILE, gs_name, filename, filename);
 	strbuf_release(&buf);

 	if (num_threads > 1) {
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 7d7b396c23..23911a3574 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/\"unusual\" pathname2" &&
 	git add . &&
 	test_tick &&
 	git commit -m initial
@@ -481,6 +483,26 @@ do
 		git grep --count -h -e b $H -- ab >actual &&
 		test_cmp expected actual
 	'
+
+	test_expect_success "grep $L should not quote unusual pathnames" '
+		cat >expected <<-EOF &&
+		${HC}"unusual" pathname:unusual
+		${HC}t/"unusual" pathname2:unusual
+		EOF
+		git grep unusual $H >actual &&
+		test_cmp expected actual
+	'
+
+	test_expect_success "grep $L should not quote unusual relative pathnames" '
+		cat >expected <<-EOF &&
+		${HC}"unusual" pathname2:unusual
+		EOF
+		(
+			cd t &&
+			git grep unusual $H
+		) >actual &&
+		test_cmp expected actual
+	'
 done

 cat >expected <<EOF
@@ -500,7 +522,8 @@ test_expect_success 'grep -c -C' '
 '

 test_expect_success 'grep -L -C' '
-	git ls-files >expected &&
+	git ls-files -z >ls-files-z &&
+	tr "\0" "\n" <ls-files-z >expected &&
 	git grep -L -C1 nonexistent_string >actual &&
 	test_cmp expected actual
 '
@@ -1686,7 +1709,9 @@ test_expect_success 'grep does not report i-t-a with -L --cached' '
 	echo "intend to add this" >intend-to-add &&
 	git add -N intend-to-add &&
 	test_when_finished "git rm -f intend-to-add" &&
-	git ls-files | grep -v "^intend-to-add\$" >expected &&
+	git ls-files -z >ls-files-z &&
+	tr "\0" "\n" <ls-files-z >files &&
+	grep -v "^intend-to-add\$" files >expected &&
 	git grep -L --cached "nonexistent_string" >actual &&
 	test_cmp expected actual
 '
@@ -1696,7 +1721,9 @@ test_expect_success 'grep does not report i-t-a and assume unchanged with -L' '
 	git add -N intend-to-add-assume-unchanged &&
 	test_when_finished "git rm -f intend-to-add-assume-unchanged" &&
 	git update-index --assume-unchanged intend-to-add-assume-unchanged &&
-	git ls-files | grep -v "^intend-to-add-assume-unchanged\$" >expected &&
+	git ls-files -z >ls-files-z &&
+	tr "\0" "\n" <ls-files-z >files &&
+	grep -v "^intend-to-add-assume-unchanged\$" files >expected &&
 	git grep -L "nonexistent_string" >actual &&
 	test_cmp expected actual
 '
--
2.26.0

Re: git-grep's "-z" option misbehaves in subdirectory

[Matheus Tavares Bernardino]

[Oops, I messed up the headers in my reply, so I'm not sure if the
message was properly delivered. I'm replying again, just in case.]

On Mon, Apr 13, 2020 at 6:57 PM Greg Hurrell <greg@hurrell.net> wrote:
>
> It seems that `git-grep -lz` behaves differently depending on whether
> it is inside a subdirectory:
[...]
> $ git grep -lz content
> an "example".txt^@nested/other "example".txt^@
>
> Note that, as expected, the files are NUL-terminated and not wrapped
> in quotes. ("^@" represents NUL byte.)
>
> $ cd nested
> $ git grep -lz content
> "other \"example\".txt"^@
>
> As soon as we move into a subdirectory, files are wrapped in quotes
> and contain escapes, despite the "-z" switch.

Note that, differently from other Git commands, "-z" doesn't affect
quoting/escaping in git-grep. For example, while git-ls-files' man
page says:

-z
      \0 line termination on output **and do not quote filenames**.

The git-grep one only says:

-z, --null
      Output \0 instead of the character that normally follows a file name.

Indeed, this inconsistency might be a little confusing. The reason for
it may be because "-z" was added in git-grep [1] to mimic the GNU grep
"-Z" flag, which outputs '\0' after file names. In GNU grep, pathnames
are never quoted (regardless of whether or not "-Z" is used). Also
note that, probably due to the same reason, git-grep doesn't obey the
core.quotePath setting (which would make it quote unusual pathnames by
default).

Nevertheless, quoting relative paths but not absolute ones is a bug.
From what I see, we have two options to fix it:

1.  Make git-grep more consistent with other Git commands by always
quoting/escaping unusual pathnames (relative or not), unless "-z" is
given or core.quotePath is set to "false".

2.  Keep git-grep compatible with GNU grep, ripgrep and other grepping
tools, removing the quotes/escaping from relative paths as well. In
this case, I think it is also a good idea to add a note about this
behavior in core.quotePath and in git-grep's description of "-z" (as
it may seem confusing to those who are used to the behavior of "-z" in
other Git commands).

In my previous [and possibly corrupted] reply, I included a patch[2]
in the direction of the second option. But thinking about it again,
I'm not really sure which one is the best way forward here. I'd love
to hear what others have to say about it, before sending a more
refined patch.

[1]: See 83caecc, (git grep: Add "-z/--null" option as in GNU's grep.,
2008-10-01)
[2]: https://public-inbox.org/git/20200414074204.677139-1-matheus.bernardino@usp.br/
(for some reason, the mail didn't reach lore.kernel.org. Probably my
fault, messing with the headers :/)

Re: git-grep's "-z" option misbehaves in subdirectory

[Junio C Hamano]

Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:

> Note that, differently from other Git commands, "-z" doesn't affect
> quoting/escaping in git-grep. For example, while git-ls-files' man
> page says:
>
> -z
>       \0 line termination on output **and do not quote filenames**.
>
> The git-grep one only says:
>
> -z, --null
>       Output \0 instead of the character that normally follows a file name.
>
> Indeed, this inconsistency might be a little confusing. The reason for
> it may be ...

Let me lift the need for speculation ;-) It was a simple oversight
when the documentation was writte, and those "add relative with
quoting" were bugs that not many people thought of testing with -z
output mode.  It is unfortunate, but it happens that things fall
into cracks without anybody noticing X-<.

The reason why we quote paths in output from various commands
without -z is to make them machine parseable; there is no reason to
quote if \0 delimiting is used as no paths contain NUL byte in it.

> 1.  Make git-grep more consistent with other Git commands by always
> quoting/escaping unusual pathnames (relative or not), unless "-z" is
> given or core.quotePath is set to "false".

This is the only sensible way forward, I would think.

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

[Matheus Tavares]

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(-)

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

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

[Junio C Hamano]

Matheus Tavares <matheus.bernardino@usp.br> writes:

> +	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);

Yup.  This solves the discrepancy reported correctly (i.e. both
sides should do the quoting, the original only quoted when relative,
and the new code corrects the other side).

> +	if (tree_name_len)
> +		strbuf_insert(out, 0, filename, tree_name_len);

I am not quite sure about this part, though.  

Earlier we inserted the latter part of filename (after offset
tree_name_len) to strbuf "out" after quoting, and then we are
prefixing the earlier part of the filename without quoting to that
same "out".  Wouldn't a path ABCDEF (I do not literally mean that
these ascii alphabets need quoting---just imagine each of these
stands for a different letter and some causes the path to be quoted)
with tree_name_len pointing somewhere in the middle be added as (an
analog of) ABC"DEF", i.e. literal prefix with remainder quoted?  I
would (perhaps näively) expect that the whole thing would be placed
inside a dq pair in such a case, even if the prefix part alone would
not require quoting.

Thanks.

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

[Matheus Tavares Bernardino]

On Fri, Apr 17, 2020 at 3:45 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Matheus Tavares <matheus.bernardino@usp.br> writes:
>
> > +     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);
>
> Yup.  This solves the discrepancy reported correctly (i.e. both
> sides should do the quoting, the original only quoted when relative,
> and the new code corrects the other side).
>
> > +     if (tree_name_len)
> > +             strbuf_insert(out, 0, filename, tree_name_len);
>
> I am not quite sure about this part, though.
>
> Earlier we inserted the latter part of filename (after offset
> tree_name_len) to strbuf "out" after quoting, and then we are
> prefixing the earlier part of the filename without quoting to that
> same "out".  Wouldn't a path ABCDEF (I do not literally mean that
> these ascii alphabets need quoting---just imagine each of these
> stands for a different letter and some causes the path to be quoted)
> with tree_name_len pointing somewhere in the middle be added as (an
> analog of) ABC"DEF", i.e. literal prefix with remainder quoted?

Right. But the ABC prefix here is always a tree name, so the output is
one of the following:

<sha1>:"unusual path":line
<ref name>:"unusual path":line
"unusual path":line

(Always having the entire "unusual path" inside the double quotes, though)

> I would (perhaps näively) expect that the whole thing would be placed
> inside a dq pair in such a case, even if the prefix part alone would
> not require quoting.

Hm, it might be just me, but I think that including the tree name and
path in the same dq pair could be a bit confusing, as they can seem
like one unique thing.

There is also the case of ref names containing unusual chars, as dq's.
Such ref names are not currently escaped in the output of other
commands as checkout, branch or describe. But if we include the ref
name in grep's output quote, it would be escaped, which would be a
minor inconsistency (I don't know if that's relevant, though).

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

[Junio C Hamano]

Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:

> Right. But the ABC prefix here is always a tree name,...

Oh, then that is perfectly fine.  I did not expect that the code
passes <tree-name>:<pathnname> (like "HEAD~2:t/Makefile") to us as a
single string.

Thanks.

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

[Johannes Schindelin]

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
>
>
>

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

[Johannes Schindelin]

Hi Matheus,

On Sat, 18 Apr 2020, Johannes Schindelin wrote:

> 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.

I need this commit to fix it:
https://github.com/git-for-windows/git/commit/7ca815e1ab89d6ffdb1a17b3cbacdf22a508d33c

I'll paste it here, for your convenience:

-- snipsnap --
From 7ca815e1ab89d6ffdb1a17b3cbacdf22a508d33c Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sat, 18 Apr 2020 16:49:43 +0200
Subject: [PATCH] fixup??? grep: follow conventions for printing paths w/
 unusual chars

It is easy to be fooled by the Bash included in Git for Windows, which
leads you to believe that quotes are valid parts of file names.

On Windows, they are not. But Cygwin (which is the base of MSYS2, which
is the POSIX emulation layer used by that Bash) only _pretends_ that it
is a valid file name character. In reality, it will map the character
into the private Unicode page. Cygwin knows about this. The rest of
Windows applications (including Git for Windows), however, does not.

As a consequence, `>\"with\ quotes\"` will claim to succeed, but the
file on disk will have Unicode characters in place of those quotes that
literally no application but Cygwin ones can handle, and this leads to
those beautiful new tests to fail.

Let's just use the prereq we introduced to guard precisely against this
problem: `FUNNYNAMES`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t7810-grep.sh | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index ab495dba28a7..991d5bd9c03f 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -72,8 +72,11 @@ test_expect_success setup '
 	# Still a no-op.
 	function dummy() {}
 	EOF
-	echo unusual >"\"unusual\" pathname" &&
-	echo unusual >"t/nested \"unusual\" pathname" &&
+	if test_have_prereq FUNNYNAMES
+	then
+		echo unusual >"\"unusual\" pathname" &&
+		echo unusual >"t/nested \"unusual\" pathname"
+	fi &&
 	git add . &&
 	test_tick &&
 	git commit -m initial
@@ -484,7 +487,7 @@ do
 		test_cmp expected actual
 	'

-	test_expect_success "grep $L should quote unusual pathnames" '
+	test_expect_success FUNNYNAMES "grep $L should quote unusual pathnames" '
 		cat >expected <<-EOF &&
 		${HC}"\"unusual\" pathname":unusual
 		${HC}"t/nested \"unusual\" pathname":unusual
@@ -493,7 +496,7 @@ do
 		test_cmp expected actual
 	'

-	test_expect_success "grep $L in subdir should quote unusual relative pathnames" '
+	test_expect_success FUNNYNAMES "grep $L in subdir should quote unusual relative pathnames" '
 		cat >expected <<-EOF &&
 		${HC}"nested \"unusual\" pathname":unusual
 		EOF
@@ -504,7 +507,7 @@ do
 		test_cmp expected actual
 	'

-	test_expect_success "grep -z $L with unusual pathnames" '
+	test_expect_success FUNNYNAMES "grep -z $L with unusual pathnames" '
 		cat >expected <<-EOF &&
 		${HC}"unusual" pathname:unusual
 		${HC}t/nested "unusual" pathname:unusual
@@ -514,7 +517,7 @@ do
 		test_cmp expected actual-replace-null
 	'

-	test_expect_success "grep -z $L in subdir with unusual relative pathnames" '
+	test_expect_success FUNNYNAMES "grep -z $L in subdir with unusual relative pathnames" '
 		cat >expected <<-EOF &&
 		${HC}nested "unusual" pathname:unusual
 		EOF
--
2.26.1.windows.1

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

[Matheus Tavares Bernardino]

Hi, Dscho

On Sat, Apr 18, 2020 at 11:57 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> On Sat, 18 Apr 2020, Johannes Schindelin wrote:
> >
> > On Fri, 17 Apr 2020, Matheus Tavares wrote:
...
> > >
> > > 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.
>
> I need this commit to fix it:
> https://github.com/git-for-windows/git/commit/7ca815e1ab89d6ffdb1a17b3cbacdf22a508d33c
>
> I'll paste it here, for your convenience:
>
> -- snipsnap --

Thanks for reporting and correcting the testing issue. I'll send a v2
with your patch squashed in.

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

[Matheus Tavares]

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>
Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---

Changes in v2:
- Squashed in Dscho's patch fixing the tests' failures in Windows by
  adding the FUNNYNAMES prereq.


 Documentation/git-grep.txt |  6 +++--
 builtin/grep.c             | 46 +++++++++++++++++++++++++++----------
 t/t7810-grep.sh            | 47 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+), 14 deletions(-)

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..991d5bd9c0 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -72,6 +72,11 @@ test_expect_success setup '
 	# Still a no-op.
 	function dummy() {}
 	EOF
+	if test_have_prereq FUNNYNAMES
+	then
+		echo unusual >"\"unusual\" pathname" &&
+		echo unusual >"t/nested \"unusual\" pathname"
+	fi &&
 	git add . &&
 	test_tick &&
 	git commit -m initial
@@ -481,6 +486,48 @@ do
 		git grep --count -h -e b $H -- ab >actual &&
 		test_cmp expected actual
 	'
+
+	test_expect_success FUNNYNAMES "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 FUNNYNAMES "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 FUNNYNAMES "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 FUNNYNAMES "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