[Bug report] git status doesn't escape paths of untracked files

[Bug report] git status doesn't escape paths of untracked files

[Patrick Fong]

What did you do before the bug happened? (Steps to reproduce your issue)
touch "this is escaped"
touch "this is not escaped"
git add "this is escaped"
git status --short

What did you expect to happen? (Expected behavior)
I expected that git status --short would string escape both "this is
escaped" and "this is not escaped" since they both contain spaces that
need escaping.

What happened instead? (Actual behavior)
git status did not string escape "this is not escaped" but it does if
you add it to the index.

What's different between what you expected and what actually happened?

Anything else you want to add:
Once you add "this is not escaped", git status will output it with
quotes. git status --short seems to change its behavior of escaping
based on whether the path is tracked or untracked.

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.28.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Darwin 19.5.0 Darwin Kernel Version 19.5.0: Tue May 26 20:41:44
PDT 2020; root:xnu-6153.121.2~2/RELEASE_X86_64 x86_64
compiler info: clang: 11.0.3 (clang-1103.0.32.62)
libc info: no libc information available
$SHELL (typically, interactive shell): /usr/local/bin/fish


[Enabled Hooks]

Re: [Bug report] git status doesn't escape paths of untracked files

[Junio C Hamano]

Patrick Fong <patrickf3139@gmail.com> writes:

> What did you do before the bug happened? (Steps to reproduce your issue)
> touch "this is escaped"
> touch "this is not escaped"
> git add "this is escaped"
> git status --short
>
> What did you expect to happen? (Expected behavior)
> I expected that git status --short would string escape both "this is
> escaped" and "this is not escaped" since they both contain spaces that
> need escaping.
>
> What happened instead? (Actual behavior)
> git status did not string escape "this is not escaped" but it does if
> you add it to the index.

It indeed is a disturbing inconsistency, and the code that shows the
untracked paths should be fixed.  I wonder what is done to ignored
paths when the "--ignored" option is given.

wt-status.c::wt_shortstatus_status() has a strange special case that
says "whitespace alone does not usually deserve quoting, but we
special case it and manually quote"; this discrepancy/special casing
was introduced at dbfdc625 (status: Quote paths with spaces in short
format, 2010-11-08).  A fix would probably be to port the same fix
to wt-status.c::wt_shortstatus_other().

In the longer term, quote.c::quote_path() may want to learn an
optional flag to tell it to do the the special casing.

Thanks.  

Re: [Bug report] git status doesn't escape paths of untracked files

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]

On 2020-09-08 at 00:28:57, Patrick Fong wrote:
> What did you do before the bug happened? (Steps to reproduce your issue)
> touch "this is escaped"
> touch "this is not escaped"
> git add "this is escaped"
> git status --short
> 
> What did you expect to happen? (Expected behavior)
> I expected that git status --short would string escape both "this is
> escaped" and "this is not escaped" since they both contain spaces that
> need escaping.
> 
> What happened instead? (Actual behavior)
> git status did not string escape "this is not escaped" but it does if
> you add it to the index.
> 
> What's different between what you expected and what actually happened?
> 
> Anything else you want to add:
> Once you add "this is not escaped", git status will output it with
> quotes. git status --short seems to change its behavior of escaping
> based on whether the path is tracked or untracked.

git-status(1) says:

  If a filename contains whitespace or other nonprintable characters,
  that field will be quoted in the manner of a C string literal:
  surrounded by ASCII double quote (34) characters, and with interior
  special characters backslash-escaped.

Note that that differs from the standard behavior of not handling
spaces, which I expect is due to the need to handle renames
unambiguously.  We have some special handling in wt_shortstatus_status,
but not in wt_shortstatus_other.

Patch incoming once the testsuite finishes running on this commit.
-- 
brian m. carlson: Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [Bug report] git status doesn't escape paths of untracked files

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> git-status(1) says:
>
>   If a filename contains whitespace or other nonprintable characters,
>   that field will be quoted in the manner of a C string literal:
>   surrounded by ASCII double quote (34) characters, and with interior
>   special characters backslash-escaped.
>
> Note that that differs from the standard behavior of not handling
> spaces, which I expect is due to the need to handle renames
> unambiguously.

Not really.  We use "rename from" and "rename to" extended header
lines in our output to unambiguously handle renamed paths.

cquote was indeed invented to serve "diff" output (actually, nice
things in the internal implementation of Git from the early years
were all invented to serve the diff machinery), but we deliberately
excluded SP from the set of characters that needs quoting because
it was thought to be reasonably common, compared to things like HT.

I agree with your "the special case handling needs to be taught to
the wt_shortstatus_other()"; a refactored helper function called
by both places would help.

Thanks.

Re: [Bug report] git status doesn't escape paths of untracked files

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
>> git-status(1) says:
>>
>>   If a filename contains whitespace or other nonprintable characters,
>>   that field will be quoted in the manner of a C string literal:
>>   surrounded by ASCII double quote (34) characters, and with interior
>>   special characters backslash-escaped.
>>
>> Note that that differs from the standard behavior of not handling
>> spaces, which I expect is due to the need to handle renames
>> unambiguously.
>
> Not really.  We use "rename from" and "rename to" extended header
> lines in our output to unambiguously handle renamed paths.

Ah, I think I misunderstood you.  We don't special case SP in what
you called "the standard behaviour", i.e. "diff" output because we
use "rename from/to", but a short format "status" that does not use
NUL delimited output, we do need to give two paths, separate by
whitespaces, and the quoting each path separately does help.

Re: [Bug report] git status doesn't escape paths of untracked files

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I agree with your "the special case handling needs to be taught to
> the wt_shortstatus_other()"; a refactored helper function called
> by both places would help.

I came up with this.

 - I very much like the fact that I got rid of the "directly print
   dq and then feed the remainder of (un)quoted path plus trailing
   dq to the normal printing logic" from print_cquoted(), even
   though strbuf_insertstr() a single byte to the beginning of the
   buffer feels a bit wasteful.

 - I think the short status output for unmerged paths deserve the
   same quoting treatment, so an extra helper function pays off even
   better than our plan to fix "untracked/ignored".

 - I am undecided if I like that the helper formats and also prints;
   I was hoping I can come up with a pure formatting helper that
   does not do I/O but it seems to be hard to arrange for the
   current callers.

It seems to pass your tests, but I am not sure how good our test
coverage is around this area.

I see some mixed use of stdout and s->fp in the vicinity together
with "fprintf(stdout, ...)". We may want to clean them up someday,
by the way.

 wt-status.c | 56 ++++++++++++++++++++++----------------------------------
 1 file changed, 22 insertions(+), 34 deletions(-)

diff --git c/wt-status.c w/wt-status.c
index bb0f9120de..ff43932402 100644
--- c/wt-status.c
+++ w/wt-status.c
@@ -1829,6 +1829,21 @@ static void wt_longstatus_print(struct wt_status *s)
 		wt_longstatus_print_stash_summary(s);
 }
 
+static void print_cquoted(const char *fmt, const char *path, const char *prefix)
+{
+	struct strbuf onebuf = STRBUF_INIT;
+	const char *one;
+
+	one = quote_path(path, prefix, &onebuf);
+	if (*one != '"' && strchr(one, ' ')) {
+		strbuf_insertstr(&onebuf, 0, "\"");
+		strbuf_addch(&onebuf, '"');
+		one = onebuf.buf;
+	}
+	printf(fmt, one);
+	strbuf_release(&onebuf);
+}
+
 static void wt_shortstatus_unmerged(struct string_list_item *it,
 			   struct wt_status *s)
 {
@@ -1845,15 +1860,10 @@ static void wt_shortstatus_unmerged(struct string_list_item *it,
 	case 7: how = "UU"; break; /* both modified */
 	}
 	color_fprintf(s->fp, color(WT_STATUS_UNMERGED, s), "%s", how);
-	if (s->null_termination) {
+	if (s->null_termination)
 		fprintf(stdout, " %s%c", it->string, 0);
-	} else {
-		struct strbuf onebuf = STRBUF_INIT;
-		const char *one;
-		one = quote_path(it->string, s->prefix, &onebuf);
-		printf(" %s\n", one);
-		strbuf_release(&onebuf);
-	}
+	else
+		print_cquoted(" %s\n", it->string, s->prefix);
 }
 
 static void wt_shortstatus_status(struct string_list_item *it,
@@ -1875,27 +1885,9 @@ static void wt_shortstatus_status(struct string_list_item *it,
 		if (d->rename_source)
 			fprintf(stdout, "%s%c", d->rename_source, 0);
 	} else {
-		struct strbuf onebuf = STRBUF_INIT;
-		const char *one;
-
-		if (d->rename_source) {
-			one = quote_path(d->rename_source, s->prefix, &onebuf);
-			if (*one != '"' && strchr(one, ' ') != NULL) {
-				putchar('"');
-				strbuf_addch(&onebuf, '"');
-				one = onebuf.buf;
-			}
-			printf("%s -> ", one);
-			strbuf_release(&onebuf);
-		}
-		one = quote_path(it->string, s->prefix, &onebuf);
-		if (*one != '"' && strchr(one, ' ') != NULL) {
-			putchar('"');
-			strbuf_addch(&onebuf, '"');
-			one = onebuf.buf;
-		}
-		printf("%s\n", one);
-		strbuf_release(&onebuf);
+		if (d->rename_source)
+			print_cquoted("%s -> ", d->rename_source, s->prefix);
+		print_cquoted("%s\n", it->string, s->prefix);
 	}
 }
 
@@ -1905,12 +1897,8 @@ static void wt_shortstatus_other(struct string_list_item *it,
 	if (s->null_termination) {
 		fprintf(stdout, "%s %s%c", sign, it->string, 0);
 	} else {
-		struct strbuf onebuf = STRBUF_INIT;
-		const char *one;
-		one = quote_path(it->string, s->prefix, &onebuf);
 		color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "%s", sign);
-		printf(" %s\n", one);
-		strbuf_release(&onebuf);
+		print_cquoted(" %s\n", it->string, s->prefix);
 	}
 }
 

Re: [Bug report] git status doesn't escape paths of untracked files

[Martin Ågren]

On Tue, 8 Sep 2020 at 19:44, Junio C Hamano <gitster@pobox.com> wrote:
> I see some mixed use of stdout and s->fp in the vicinity together
> with "fprintf(stdout, ...)". We may want to clean them up someday,
> by the way.

I noticed the same thing. I've come up with a few patches to address
that and some other minor things I saw on the way. I'll need to test
some more before posting, so this is mostly a heads-up for now in case
others are thinking about jumping on this.

Will try to post it tomorrow. There's nothing there that needs to block
any fixes to the escaping, obviously.

Martin

Re: [Bug report] git status doesn't escape paths of untracked files

[René Scharfe]

Am 08.09.20 um 19:39 schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I agree with your "the special case handling needs to be taught to
>> the wt_shortstatus_other()"; a refactored helper function called
>> by both places would help.
>
> I came up with this.
>
>  - I very much like the fact that I got rid of the "directly print
>    dq and then feed the remainder of (un)quoted path plus trailing
>    dq to the normal printing logic" from print_cquoted(), even
>    though strbuf_insertstr() a single byte to the beginning of the
>    buffer feels a bit wasteful.

I don't particularly like how this print* function takes a printf
format, but doesn't interpret its remaining arguments like printf does.

>  - I think the short status output for unmerged paths deserve the
>    same quoting treatment, so an extra helper function pays off even
>    better than our plan to fix "untracked/ignored".
>
>  - I am undecided if I like that the helper formats and also prints;
>    I was hoping I can come up with a pure formatting helper that
>    does not do I/O but it seems to be hard to arrange for the
>    current callers.

How about something like this then?

diff --git a/wt-status.c b/wt-status.c
index ff43932402..e0711aff04 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1829,19 +1829,19 @@ static void wt_longstatus_print(struct wt_status *s)
 		wt_longstatus_print_stash_summary(s);
 }

-static void print_cquoted(const char *fmt, const char *path, const char *prefix)
+static const char *cquote(const char *in, const char *prefix, struct strbuf *out)
 {
-	struct strbuf onebuf = STRBUF_INIT;
-	const char *one;
-
-	one = quote_path(path, prefix, &onebuf);
-	if (*one != '"' && strchr(one, ' ')) {
-		strbuf_insertstr(&onebuf, 0, "\"");
-		strbuf_addch(&onebuf, '"');
-		one = onebuf.buf;
-	}
-	printf(fmt, one);
-	strbuf_release(&onebuf);
+	struct strbuf sb = STRBUF_INIT;
+	const char *rel = relative_path(in, prefix, &sb);
+	int need_quotes = *rel != '"' && strchr(rel, ' ');
+	strbuf_reset(out);
+	if (need_quotes)
+		strbuf_addch(out, '"');
+	quote_c_style(rel, out, NULL, 0);
+	if (need_quotes)
+		strbuf_addch(out, '"');
+	strbuf_release(&sb);
+	return out->buf;
 }

 static void wt_shortstatus_unmerged(struct string_list_item *it,
@@ -1849,6 +1849,7 @@ static void wt_shortstatus_unmerged(struct string_list_item *it,
 {
 	struct wt_status_change_data *d = it->util;
 	const char *how = "??";
+	struct strbuf sb = STRBUF_INIT;

 	switch (d->stagemask) {
 	case 1: how = "DD"; break; /* both deleted */
@@ -1863,13 +1864,15 @@ static void wt_shortstatus_unmerged(struct string_list_item *it,
 	if (s->null_termination)
 		fprintf(stdout, " %s%c", it->string, 0);
 	else
-		print_cquoted(" %s\n", it->string, s->prefix);
+		printf(" %s\n", cquote(it->string, s->prefix, &sb));
+	strbuf_release(&sb);
 }

 static void wt_shortstatus_status(struct string_list_item *it,
 			 struct wt_status *s)
 {
 	struct wt_status_change_data *d = it->util;
+	struct strbuf sb = STRBUF_INIT;

 	if (d->index_status)
 		color_fprintf(s->fp, color(WT_STATUS_UPDATED, s), "%c", d->index_status);
@@ -1886,20 +1889,24 @@ static void wt_shortstatus_status(struct string_list_item *it,
 			fprintf(stdout, "%s%c", d->rename_source, 0);
 	} else {
 		if (d->rename_source)
-			print_cquoted("%s -> ", d->rename_source, s->prefix);
-		print_cquoted("%s\n", it->string, s->prefix);
+			printf("%s -> ", cquote(d->rename_source, s->prefix, &sb));
+		printf("%s\n", cquote(it->string, s->prefix, &sb));
 	}
+	strbuf_release(&sb);
 }

 static void wt_shortstatus_other(struct string_list_item *it,
 				 struct wt_status *s, const char *sign)
 {
+	struct strbuf sb = STRBUF_INIT;
+
 	if (s->null_termination) {
 		fprintf(stdout, "%s %s%c", sign, it->string, 0);
 	} else {
 		color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "%s", sign);
-		print_cquoted(" %s\n", it->string, s->prefix);
+		printf(" %s\n", cquote(it->string, s->prefix, &sb));
 	}
+	strbuf_release(&sb);
 }

 static void wt_shortstatus_print_tracking(struct wt_status *s)

Re: [Bug report] git status doesn't escape paths of untracked files

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> I don't particularly like how this print* function takes a printf
> format, but doesn't interpret its remaining arguments like printf does.

Yes.  I think the more elaborate one that pushes the logic down to
quote_path() I posted earlier is much cleaner in this regard.

>>  - I am undecided if I like that the helper formats and also prints;
>>    I was hoping I can come up with a pure formatting helper that
>>    does not do I/O but it seems to be hard to arrange for the
>>    current callers.
>
> How about something like this then?

Thanks.  I kind of like the fact that it is isolated compared to the
"push the logic down to quote_path()" approach.

But it also is a double edged sword.  It makes it harder for other
codepaths to quote a path with SP in it by mistake, but if it turns
out to be a good longer term thing to quote a path with SP in it,
it makes it harder to do so.

> diff --git a/wt-status.c b/wt-status.c
> index ff43932402..e0711aff04 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1829,19 +1829,19 @@ static void wt_longstatus_print(struct wt_status *s)
>  		wt_longstatus_print_stash_summary(s);
>  }
>
> -static void print_cquoted(const char *fmt, const char *path, const char *prefix)
> +static const char *cquote(const char *in, const char *prefix, struct strbuf *out)
>  {
> -	struct strbuf onebuf = STRBUF_INIT;
> -	const char *one;
> -
> -	one = quote_path(path, prefix, &onebuf);
> -	if (*one != '"' && strchr(one, ' ')) {
> -		strbuf_insertstr(&onebuf, 0, "\"");
> -		strbuf_addch(&onebuf, '"');
> -		one = onebuf.buf;
> -	}
> -	printf(fmt, one);
> -	strbuf_release(&onebuf);
> +	struct strbuf sb = STRBUF_INIT;
> +	const char *rel = relative_path(in, prefix, &sb);
> +	int need_quotes = *rel != '"' && strchr(rel, ' ');

relative_path() does not quote, so "begins with a dq" is not a good
test to see "if we were to pass this string to quote_c_style(), would
we get it back quoted already so we won't have to surround the
result with an extra pair of dq ourselves?".

> +	strbuf_reset(out);
> +	if (need_quotes)
> +		strbuf_addch(out, '"');
> +	quote_c_style(rel, out, NULL, 0);
> +	if (need_quotes)
> +		strbuf_addch(out, '"');
> +	strbuf_release(&sb);
> +	return out->buf;
>  }

Re: [Bug report] git status doesn't escape paths of untracked files

[René Scharfe]

Am 10.09.20 um 00:22 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>> diff --git a/wt-status.c b/wt-status.c
>> index ff43932402..e0711aff04 100644
>> --- a/wt-status.c
>> +++ b/wt-status.c
>> @@ -1829,19 +1829,19 @@ static void wt_longstatus_print(struct wt_status *s)
>>  		wt_longstatus_print_stash_summary(s);
>>  }
>>
>> -static void print_cquoted(const char *fmt, const char *path, const char *prefix)
>> +static const char *cquote(const char *in, const char *prefix, struct strbuf *out)
>>  {
>> -	struct strbuf onebuf = STRBUF_INIT;
>> -	const char *one;
>> -
>> -	one = quote_path(path, prefix, &onebuf);
>> -	if (*one != '"' && strchr(one, ' ')) {
>> -		strbuf_insertstr(&onebuf, 0, "\"");
>> -		strbuf_addch(&onebuf, '"');
>> -		one = onebuf.buf;
>> -	}
>> -	printf(fmt, one);
>> -	strbuf_release(&onebuf);
>> +	struct strbuf sb = STRBUF_INIT;
>> +	const char *rel = relative_path(in, prefix, &sb);
>> +	int need_quotes = *rel != '"' && strchr(rel, ' ');
>
> relative_path() does not quote, so "begins with a dq" is not a good
> test to see "if we were to pass this string to quote_c_style(), would
> we get it back quoted already so we won't have to surround the
> result with an extra pair of dq ourselves?".

Ha!, that's true.  Makes me wonder how it was still able to pass the
test suite, though..

>
>> +	strbuf_reset(out);
>> +	if (need_quotes)
>> +		strbuf_addch(out, '"');
>> +	quote_c_style(rel, out, NULL, 0);
>> +	if (need_quotes)
>> +		strbuf_addch(out, '"');
>> +	strbuf_release(&sb);
>> +	return out->buf;
>>  }

Re: [Bug report] git status doesn't escape paths of untracked files

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

>>> +	struct strbuf sb = STRBUF_INIT;
>>> +	const char *rel = relative_path(in, prefix, &sb);
>>> +	int need_quotes = *rel != '"' && strchr(rel, ' ');
>>
>> relative_path() does not quote, so "begins with a dq" is not a good
>> test to see "if we were to pass this string to quote_c_style(), would
>> we get it back quoted already so we won't have to surround the
>> result with an extra pair of dq ourselves?".
>
> Ha!, that's true.  Makes me wonder how it was still able to pass the
> test suite, though..

The logic to refrain from ading extra dq-pair around the output from
quote-c-style is "if it is already in a dq-pair, don't bother".  The
bug will trigger only when the input string has a character that
needs to be quoted *and* SP in it.  In such a case, relative_path()
is likely to return *rel != '"' and you declare "we need to add a
dq-pair ourselves", quote-c-style would give us a result in a
dq-pair, and you add one extra layer.  

But without a test with a path with both '"' and ' ' in it, such a
bug would not trigger.  Brian's test only checks what happens to a
path with SP in it, without any other funny characters that need
quoting with quote-c-style.

[PATCH] wt-status: quote paths identically whether tracked or untracked

[brian m. carlson]

The documentation for git status --short says this:

  If a filename contains whitespace or other nonprintable characters,
  that field will be quoted in the manner of a C string literal:
  surrounded by ASCII double quote (34) characters, and with interior
  special characters backslash-escaped.

Note that this differs from our typical quoting rule, which does not
include spaces.  If we did not quote spaces, our output would be
ambiguous for renames.

However, we failed to do this correctly for untracked files.  If we list
an untracked file that contains spaces, we fail to quote it.  Since this
is both inconsistent and not what was documented, let's fix it by
quoting untracked files in the same way as tracked files.  Users parsing
this output already need to handle quoted names for tracked files (or
use -z) so this shouldn't be an incompatible change.

Note that the test for this case should be portable because all known
modern systems support spaces in file names and our test trash
directories use them already.

Reported-by: Patrick Fong <patrickf3139@gmail.com>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/t7508-status.sh | 21 +++++++++++++++++++++
 wt-status.c       |  8 +++++++-
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index e81759319f..ef8d19c151 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -814,6 +814,27 @@ test_expect_success 'status -s without relative paths' '
 
 '
 
+cat >expect <<\EOF
+ M dir1/modified
+A  dir2/added
+A  "file with spaces"
+?? dir1/untracked
+?? dir2/modified
+?? dir2/untracked
+?? "file with spaces 2"
+?? untracked
+EOF
+
+test_expect_success 'status -s without relative paths' '
+	test_when_finished "git rm --cached \"file with spaces\"; rm -f file*" &&
+	>"file with spaces" &&
+	>"file with spaces 2" &&
+	git add "file with spaces" &&
+	git status -s >output &&
+	test_cmp expect output
+
+'
+
 test_expect_success 'dry-run of partial commit excluding new file in index' '
 	cat >expect <<EOF &&
 On branch master
diff --git a/wt-status.c b/wt-status.c
index bb0f9120de..bea6cf98b1 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1909,7 +1909,13 @@ static void wt_shortstatus_other(struct string_list_item *it,
 		const char *one;
 		one = quote_path(it->string, s->prefix, &onebuf);
 		color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "%s", sign);
-		printf(" %s\n", one);
+		putchar(' ');
+		if (*one != '"' && strchr(one, ' ') != NULL) {
+			putchar('"');
+			strbuf_addch(&onebuf, '"');
+			one = onebuf.buf;
+		}
+		printf("%s\n", one);
 		strbuf_release(&onebuf);
 	}
 }

[PATCH 0/6] quote_path() clean-ups

[Junio C Hamano]

So, this is an alternative approach to tackle the same issue
<xmqq4ko8yxp9.fsf@gitster.c.googlers.com> tried to address.  It
ended up to be more involved than I would have liked, but it was
primarily because it needed some function signature changes.
 
The overall structure of the series is:

 (1) rename quote_path_relative() to quote_path().

 (2) allow quote_path() to take a flags parameter and update all the
     callers to pass 0 (all bits false)

 (3) move the "always quote a path with SP in it, even if there is
     no byte that needs quoting" logic out of wt-status.c that is
     used to show "git status --short" for tracked paths to
     quote_path() and give it the QUOTE_PATH_QUOTE_SP bit in the
     flags parameter.

 (4) using QUOTE_PATH_QUOTE_SP bit, apply the "always quote a path
     with SP" to "git status --short" output for untracked, ignored
     and unmerged paths.

Patches (5) and (6) are not strictly necessary.  They are what I
found while reading the existing code and thought it might, or might
not, be worth cleaning up and couldn't decide ;-)

Junio C Hamano (6):
  quote_path: rename quote_path_relative() to quote_path()
  quote_path: give flags parameter to quote_path()
  quote_path: optionally allow quoting a path with SP in it
  wt-status: consistently quote paths in "status --short" output
  quote: rename misnamed sq_lookup[] to cq_lookup[]
  quote: turn 'nodq' parameter into a set of flags

 builtin/clean.c   | 22 +++++++++++-----------
 builtin/grep.c    |  2 +-
 diff.c            |  8 ++++----
 quote.c           | 39 ++++++++++++++++++++++++---------------
 quote.h           | 11 +++++++----
 t/t7508-status.sh | 21 +++++++++++++++++++++
 wt-status.c       | 37 +++++++++++++------------------------
 7 files changed, 81 insertions(+), 59 deletions(-)

-- 
2.28.0-539-g66371d8995

[PATCH 1/6] quote_path: rename quote_path_relative() to quote_path()

[Junio C Hamano]

There is no quote_path_absolute() or anything that causes confusion,
and one of the two large consumers already rename the long name
locally with a preprocessor macro.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/clean.c | 22 +++++++++++-----------
 builtin/grep.c  |  2 +-
 quote.c         |  3 +--
 quote.h         |  3 +--
 wt-status.c     |  2 --
 5 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index e53ea52d89..dee44fff6e 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -162,7 +162,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 	if ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
 	    is_nonbare_repository_dir(path)) {
 		if (!quiet) {
-			quote_path_relative(path->buf, prefix, &quoted);
+			quote_path(path->buf, prefix, &quoted);
 			printf(dry_run ?  _(msg_would_skip_git_dir) : _(msg_skip_git_dir),
 					quoted.buf);
 		}
@@ -177,7 +177,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 		res = dry_run ? 0 : rmdir(path->buf);
 		if (res) {
 			int saved_errno = errno;
-			quote_path_relative(path->buf, prefix, &quoted);
+			quote_path(path->buf, prefix, &quoted);
 			errno = saved_errno;
 			warning_errno(_(msg_warn_remove_failed), quoted.buf);
 			*dir_gone = 0;
@@ -202,7 +202,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 			if (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))
 				ret = 1;
 			if (gone) {
-				quote_path_relative(path->buf, prefix, &quoted);
+				quote_path(path->buf, prefix, &quoted);
 				string_list_append(&dels, quoted.buf);
 			} else
 				*dir_gone = 0;
@@ -210,11 +210,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 		} else {
 			res = dry_run ? 0 : unlink(path->buf);
 			if (!res) {
-				quote_path_relative(path->buf, prefix, &quoted);
+				quote_path(path->buf, prefix, &quoted);
 				string_list_append(&dels, quoted.buf);
 			} else {
 				int saved_errno = errno;
-				quote_path_relative(path->buf, prefix, &quoted);
+				quote_path(path->buf, prefix, &quoted);
 				errno = saved_errno;
 				warning_errno(_(msg_warn_remove_failed), quoted.buf);
 				*dir_gone = 0;
@@ -238,7 +238,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 			*dir_gone = 1;
 		else {
 			int saved_errno = errno;
-			quote_path_relative(path->buf, prefix, &quoted);
+			quote_path(path->buf, prefix, &quoted);
 			errno = saved_errno;
 			warning_errno(_(msg_warn_remove_failed), quoted.buf);
 			*dir_gone = 0;
@@ -266,7 +266,7 @@ static void pretty_print_dels(void)
 	struct column_options copts;
 
 	for_each_string_list_item(item, &del_list) {
-		qname = quote_path_relative(item->string, NULL, &buf);
+		qname = quote_path(item->string, NULL, &buf);
 		string_list_append(&list, qname);
 	}
 
@@ -753,7 +753,7 @@ static int ask_each_cmd(void)
 	for_each_string_list_item(item, &del_list) {
 		/* Ctrl-D should stop removing files */
 		if (!eof) {
-			qname = quote_path_relative(item->string, NULL, &buf);
+			qname = quote_path(item->string, NULL, &buf);
 			/* TRANSLATORS: Make sure to keep [y/N] as is */
 			printf(_("Remove %s [y/N]? "), qname);
 			if (git_read_line_interactively(&confirm) == EOF) {
@@ -1047,19 +1047,19 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 			if (remove_dirs(&abs_path, prefix, rm_flags, dry_run, quiet, &gone))
 				errors++;
 			if (gone && !quiet) {
-				qname = quote_path_relative(item->string, NULL, &buf);
+				qname = quote_path(item->string, NULL, &buf);
 				printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
 			}
 		} else {
 			res = dry_run ? 0 : unlink(abs_path.buf);
 			if (res) {
 				int saved_errno = errno;
-				qname = quote_path_relative(item->string, NULL, &buf);
+				qname = quote_path(item->string, NULL, &buf);
 				errno = saved_errno;
 				warning_errno(_(msg_warn_remove_failed), qname);
 				errors++;
 			} else if (!quiet) {
-				qname = quote_path_relative(item->string, NULL, &buf);
+				qname = quote_path(item->string, NULL, &buf);
 				printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
 			}
 		}
diff --git a/builtin/grep.c b/builtin/grep.c
index f58979bc3f..9a91ad643a 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -319,7 +319,7 @@ static void grep_source_name(struct grep_opt *opt, const char *filename,
 	}
 
 	if (opt->relative && opt->prefix_length)
-		quote_path_relative(filename + tree_name_len, opt->prefix, out);
+		quote_path(filename + tree_name_len, opt->prefix, out);
 	else
 		quote_c_style(filename + tree_name_len, out, NULL, 0);
 
diff --git a/quote.c b/quote.c
index ced0245e80..7bb519c1a7 100644
--- a/quote.c
+++ b/quote.c
@@ -352,8 +352,7 @@ void write_name_quoted_relative(const char *name, const char *prefix,
 }
 
 /* quote path as relative to the given prefix */
-char *quote_path_relative(const char *in, const char *prefix,
-			  struct strbuf *out)
+char *quote_path(const char *in, const char *prefix, struct strbuf *out)
 {
 	struct strbuf sb = STRBUF_INIT;
 	const char *rel = relative_path(in, prefix, &sb);
diff --git a/quote.h b/quote.h
index fa09309cf6..837cb42a71 100644
--- a/quote.h
+++ b/quote.h
@@ -72,8 +72,7 @@ void write_name_quoted_relative(const char *name, const char *prefix,
 				FILE *fp, int terminator);
 
 /* quote path as relative to the given prefix */
-char *quote_path_relative(const char *in, const char *prefix,
-			  struct strbuf *out);
+char *quote_path(const char *in, const char *prefix, struct strbuf *out);
 
 /* quoting as a string literal for other languages */
 void perl_quote_buf(struct strbuf *sb, const char *src);
diff --git a/wt-status.c b/wt-status.c
index bb0f9120de..6b87592856 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -259,8 +259,6 @@ static void wt_longstatus_print_trailer(struct wt_status *s)
 	status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
 }
 
-#define quote_path quote_path_relative
-
 static const char *wt_status_unmerged_status_string(int stagemask)
 {
 	switch (stagemask) {
-- 
2.28.0-539-g66371d8995

[PATCH 2/6] quote_path: give flags parameter to quote_path()

[Junio C Hamano]

The quote_path() function computes a path (relative to its base
directory) and c-quotes the result if necessary.  Teach it to take a
flags parameter to allow its behaviour to be enriched later.

No behaviour change intended.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/clean.c | 22 +++++++++++-----------
 builtin/grep.c  |  2 +-
 quote.c         |  4 ++--
 quote.h         |  2 +-
 wt-status.c     | 24 ++++++++++++------------
 5 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index dee44fff6e..687ab473c2 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -162,7 +162,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 	if ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
 	    is_nonbare_repository_dir(path)) {
 		if (!quiet) {
-			quote_path(path->buf, prefix, &quoted);
+			quote_path(path->buf, prefix, &quoted, 0);
 			printf(dry_run ?  _(msg_would_skip_git_dir) : _(msg_skip_git_dir),
 					quoted.buf);
 		}
@@ -177,7 +177,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 		res = dry_run ? 0 : rmdir(path->buf);
 		if (res) {
 			int saved_errno = errno;
-			quote_path(path->buf, prefix, &quoted);
+			quote_path(path->buf, prefix, &quoted, 0);
 			errno = saved_errno;
 			warning_errno(_(msg_warn_remove_failed), quoted.buf);
 			*dir_gone = 0;
@@ -202,7 +202,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 			if (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))
 				ret = 1;
 			if (gone) {
-				quote_path(path->buf, prefix, &quoted);
+				quote_path(path->buf, prefix, &quoted, 0);
 				string_list_append(&dels, quoted.buf);
 			} else
 				*dir_gone = 0;
@@ -210,11 +210,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 		} else {
 			res = dry_run ? 0 : unlink(path->buf);
 			if (!res) {
-				quote_path(path->buf, prefix, &quoted);
+				quote_path(path->buf, prefix, &quoted, 0);
 				string_list_append(&dels, quoted.buf);
 			} else {
 				int saved_errno = errno;
-				quote_path(path->buf, prefix, &quoted);
+				quote_path(path->buf, prefix, &quoted, 0);
 				errno = saved_errno;
 				warning_errno(_(msg_warn_remove_failed), quoted.buf);
 				*dir_gone = 0;
@@ -238,7 +238,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 			*dir_gone = 1;
 		else {
 			int saved_errno = errno;
-			quote_path(path->buf, prefix, &quoted);
+			quote_path(path->buf, prefix, &quoted, 0);
 			errno = saved_errno;
 			warning_errno(_(msg_warn_remove_failed), quoted.buf);
 			*dir_gone = 0;
@@ -266,7 +266,7 @@ static void pretty_print_dels(void)
 	struct column_options copts;
 
 	for_each_string_list_item(item, &del_list) {
-		qname = quote_path(item->string, NULL, &buf);
+		qname = quote_path(item->string, NULL, &buf, 0);
 		string_list_append(&list, qname);
 	}
 
@@ -753,7 +753,7 @@ static int ask_each_cmd(void)
 	for_each_string_list_item(item, &del_list) {
 		/* Ctrl-D should stop removing files */
 		if (!eof) {
-			qname = quote_path(item->string, NULL, &buf);
+			qname = quote_path(item->string, NULL, &buf, 0);
 			/* TRANSLATORS: Make sure to keep [y/N] as is */
 			printf(_("Remove %s [y/N]? "), qname);
 			if (git_read_line_interactively(&confirm) == EOF) {
@@ -1047,19 +1047,19 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 			if (remove_dirs(&abs_path, prefix, rm_flags, dry_run, quiet, &gone))
 				errors++;
 			if (gone && !quiet) {
-				qname = quote_path(item->string, NULL, &buf);
+				qname = quote_path(item->string, NULL, &buf, 0);
 				printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
 			}
 		} else {
 			res = dry_run ? 0 : unlink(abs_path.buf);
 			if (res) {
 				int saved_errno = errno;
-				qname = quote_path(item->string, NULL, &buf);
+				qname = quote_path(item->string, NULL, &buf, 0);
 				errno = saved_errno;
 				warning_errno(_(msg_warn_remove_failed), qname);
 				errors++;
 			} else if (!quiet) {
-				qname = quote_path(item->string, NULL, &buf);
+				qname = quote_path(item->string, NULL, &buf, 0);
 				printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
 			}
 		}
diff --git a/builtin/grep.c b/builtin/grep.c
index 9a91ad643a..c8037388c6 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -319,7 +319,7 @@ static void grep_source_name(struct grep_opt *opt, const char *filename,
 	}
 
 	if (opt->relative && opt->prefix_length)
-		quote_path(filename + tree_name_len, opt->prefix, out);
+		quote_path(filename + tree_name_len, opt->prefix, out, 0);
 	else
 		quote_c_style(filename + tree_name_len, out, NULL, 0);
 
diff --git a/quote.c b/quote.c
index 7bb519c1a7..03ca85afb0 100644
--- a/quote.c
+++ b/quote.c
@@ -352,12 +352,12 @@ void write_name_quoted_relative(const char *name, const char *prefix,
 }
 
 /* quote path as relative to the given prefix */
-char *quote_path(const char *in, const char *prefix, struct strbuf *out)
+char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigned flags)
 {
 	struct strbuf sb = STRBUF_INIT;
 	const char *rel = relative_path(in, prefix, &sb);
 	strbuf_reset(out);
-	quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
+	quote_c_style_counted(rel, strlen(rel), out, NULL, flags);
 	strbuf_release(&sb);
 
 	return out->buf;
diff --git a/quote.h b/quote.h
index 837cb42a71..db7140b3c7 100644
--- a/quote.h
+++ b/quote.h
@@ -72,7 +72,7 @@ void write_name_quoted_relative(const char *name, const char *prefix,
 				FILE *fp, int terminator);
 
 /* quote path as relative to the given prefix */
-char *quote_path(const char *in, const char *prefix, struct strbuf *out);
+char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigned);
 
 /* quoting as a string literal for other languages */
 void perl_quote_buf(struct strbuf *sb, const char *src);
diff --git a/wt-status.c b/wt-status.c
index 6b87592856..d6ca7bd52c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -336,7 +336,7 @@ static void wt_longstatus_print_unmerged_data(struct wt_status *s,
 		memset(padding, ' ', label_width);
 	}
 
-	one = quote_path(it->string, s->prefix, &onebuf);
+	one = quote_path(it->string, s->prefix, &onebuf, 0);
 	status_printf(s, color(WT_STATUS_HEADER, s), "\t");
 
 	how = wt_status_unmerged_status_string(d->stagemask);
@@ -402,8 +402,8 @@ static void wt_longstatus_print_change_data(struct wt_status *s,
 	if (d->rename_status == status)
 		one_name = d->rename_source;
 
-	one = quote_path(one_name, s->prefix, &onebuf);
-	two = quote_path(two_name, s->prefix, &twobuf);
+	one = quote_path(one_name, s->prefix, &onebuf, 0);
+	two = quote_path(two_name, s->prefix, &twobuf, 0);
 
 	status_printf(s, color(WT_STATUS_HEADER, s), "\t");
 	what = wt_status_diff_status_string(status);
@@ -964,7 +964,7 @@ static void wt_longstatus_print_other(struct wt_status *s,
 		struct string_list_item *it;
 		const char *path;
 		it = &(l->items[i]);
-		path = quote_path(it->string, s->prefix, &buf);
+		path = quote_path(it->string, s->prefix, &buf, 0);
 		if (column_active(s->colopts)) {
 			string_list_append(&output, path);
 			continue;
@@ -1848,7 +1848,7 @@ static void wt_shortstatus_unmerged(struct string_list_item *it,
 	} else {
 		struct strbuf onebuf = STRBUF_INIT;
 		const char *one;
-		one = quote_path(it->string, s->prefix, &onebuf);
+		one = quote_path(it->string, s->prefix, &onebuf, 0);
 		printf(" %s\n", one);
 		strbuf_release(&onebuf);
 	}
@@ -1877,7 +1877,7 @@ static void wt_shortstatus_status(struct string_list_item *it,
 		const char *one;
 
 		if (d->rename_source) {
-			one = quote_path(d->rename_source, s->prefix, &onebuf);
+			one = quote_path(d->rename_source, s->prefix, &onebuf, 0);
 			if (*one != '"' && strchr(one, ' ') != NULL) {
 				putchar('"');
 				strbuf_addch(&onebuf, '"');
@@ -1886,7 +1886,7 @@ static void wt_shortstatus_status(struct string_list_item *it,
 			printf("%s -> ", one);
 			strbuf_release(&onebuf);
 		}
-		one = quote_path(it->string, s->prefix, &onebuf);
+		one = quote_path(it->string, s->prefix, &onebuf, 0);
 		if (*one != '"' && strchr(one, ' ') != NULL) {
 			putchar('"');
 			strbuf_addch(&onebuf, '"');
@@ -1905,7 +1905,7 @@ static void wt_shortstatus_other(struct string_list_item *it,
 	} else {
 		struct strbuf onebuf = STRBUF_INIT;
 		const char *one;
-		one = quote_path(it->string, s->prefix, &onebuf);
+		one = quote_path(it->string, s->prefix, &onebuf, 0);
 		color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "%s", sign);
 		printf(" %s\n", one);
 		strbuf_release(&onebuf);
@@ -2222,9 +2222,9 @@ static void wt_porcelain_v2_print_changed_entry(
 		 */
 		sep_char = '\t';
 		eol_char = '\n';
-		path = quote_path(it->string, s->prefix, &buf);
+		path = quote_path(it->string, s->prefix, &buf, 0);
 		if (d->rename_source)
-			path_from = quote_path(d->rename_source, s->prefix, &buf_from);
+			path_from = quote_path(d->rename_source, s->prefix, &buf_from, 0);
 	}
 
 	if (path_from)
@@ -2310,7 +2310,7 @@ static void wt_porcelain_v2_print_unmerged_entry(
 	if (s->null_termination)
 		path_index = it->string;
 	else
-		path_index = quote_path(it->string, s->prefix, &buf_index);
+		path_index = quote_path(it->string, s->prefix, &buf_index, 0);
 
 	fprintf(s->fp, "%c %s %s %06o %06o %06o %06o %s %s %s %s%c",
 			unmerged_prefix, key, submodule_token,
@@ -2343,7 +2343,7 @@ static void wt_porcelain_v2_print_other(
 		path = it->string;
 		eol_char = '\0';
 	} else {
-		path = quote_path(it->string, s->prefix, &buf);
+		path = quote_path(it->string, s->prefix, &buf, 0);
 		eol_char = '\n';
 	}
 
-- 
2.28.0-539-g66371d8995

Re: [PATCH 2/6] quote_path: give flags parameter to quote_path()

[Jeff King]

On Tue, Sep 08, 2020 at 01:52:20PM -0700, Junio C Hamano wrote:

> -char *quote_path(const char *in, const char *prefix, struct strbuf *out)
> +char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigned flags)
>  {
>  	struct strbuf sb = STRBUF_INIT;
>  	const char *rel = relative_path(in, prefix, &sb);
>  	strbuf_reset(out);
> -	quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
> +	quote_c_style_counted(rel, strlen(rel), out, NULL, flags);
>  	strbuf_release(&sb);

Here we take "flags", but then we pass it along in place of
quote_c_style_counted()'s "no_dq" parameter. That seems unlikely to be
the right thing (and indeed, it's reverted in the next commit).

> --- a/quote.h
> +++ b/quote.h
> @@ -72,7 +72,7 @@ void write_name_quoted_relative(const char *name, const char *prefix,
>  				FILE *fp, int terminator);
>  
>  /* quote path as relative to the given prefix */
> -char *quote_path(const char *in, const char *prefix, struct strbuf *out);
> +char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigned);

The meaning of the last parameter is left a bit vague. :) Maybe worth
calling it "unsigned flags" (I don't mind omitting parameter names when
they are obvious, but I don't think this counts).

-Peff

Re: [PATCH 2/6] quote_path: give flags parameter to quote_path()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Sep 08, 2020 at 01:52:20PM -0700, Junio C Hamano wrote:
>
>> -char *quote_path(const char *in, const char *prefix, struct strbuf *out)
>> +char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigned flags)
>>  {
>>  	struct strbuf sb = STRBUF_INIT;
>>  	const char *rel = relative_path(in, prefix, &sb);
>>  	strbuf_reset(out);
>> -	quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
>> +	quote_c_style_counted(rel, strlen(rel), out, NULL, flags);
>>  	strbuf_release(&sb);
>
> Here we take "flags", but then we pass it along in place of
> quote_c_style_counted()'s "no_dq" parameter. That seems unlikely to be
> the right thing (and indeed, it's reverted in the next commit).

You are seeing a rebase artifact.  It needs to be corrected.  Thanks
for noticing.

As you seem to have guessed correctly, the initial plan was to have
the new logic in quote-c-style-counted and make anything at higher
level of the callchain just relay the "SP must be quoted" bit, and
the final two patches that are no longer necessary in the current
series were placed much earlier in an earlier draft of the series
as preparatory steps for that endgame.

But it turned out that the main loop of quote-c-style-counted needs
a surgery that is a bit larger than I would have liked, so the final
series consumes the "SP must be quoted" bit in quote_path() without
telling quote-c-style-counted about it.

The problem with the main loop of quote-style-counted I saw was that
the next_quote_pos() is designed to return the position of the byte
that must be prefixed with a backslash, and the machinery to help it
(namely, cq_must_quote() and cq_lookup[]) are written with that in
mind quite firmly.  The handling of SP we would be optionally adding
to the mix is somewhat different---it forces the end result to be
enclosed within a dq-pair, but it byitself does not need backslash
quoting.  Which means cq_lookup[] table needs to somehow encode a
bit per byte that says "byte with this value itself does not need to
be backslash-quoted, but the entire thing needs to be placed in a
dq-pair", and/or next_quote_pos() needs be able to say "here is a
byte to cause us to enclose the whole thing in a dq-pair, but the
byte itself need not be backslash-quoted".

Of course none of the above becomes unnecessary if we scan the whole
string for SP before the main loop in quote-c-style-counted, but the
function was written to process the input in a single pass and such
a change defeats its design.  If we need to do it in two passes, we
can have the caller do so anyway, at least for now.  That thinking
lead to the final organization of the series, with two steps that
used to be preparatory for passing the flag down thru to the bottom
layer rebased out as a discardable appendix at the end.

Thanks.

Re: [PATCH 2/6] quote_path: give flags parameter to quote_path()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Of course none of the above becomes unnecessary if we scan the whole
> string for SP before the main loop in quote-c-style-counted, but the
> function was written to process the input in a single pass and such
> a change defeats its design.  If we need to do it in two passes, we
> can have the caller do so anyway, at least for now.  That thinking
> lead to the final organization of the series, with two steps that
> used to be preparatory for passing the flag down thru to the bottom
> layer rebased out as a discardable appendix at the end.

Actually, this made me realize that another variant is possible.
It might be easier to read, or it might not.  Since I cannot tell
without actually writing one, let's see ...

Instead of scanning the output from quote-c-style-counted for SP in
the caller, the caller can do something like this.

/* quote path as relative to the given prefix */
char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigned flags)
{
	struct strbuf sb = STRBUF_INIT;
	const char *rel = relative_path(in, prefix, &sb);
	int add_dq_ourselves = !!(flags & QUOTE_PATH_QUOTE_SP) && !!strchr(rel, ' ');

	strbuf_reset(out);
        if (add_dq_ourselves)
        	strbuf_addch(out, '"');
	quote_c_style_counted(rel, strlen(rel), out, NULL,
        		      add_dq_ourselves ? CQUOTE_NODQ : 0);
        if (add_dq_ourselves)
        	strbuf_addch(out, '"');
	strbuf_release(&sb);

	return out->buf;
}

That is, we tell quote-c-style-counted not to put dq-pair around its
output whether it needs to do the backslash quoting, if we know we
want the result inside dq-pair anyway (iow, when the caller wants us
to treat SP specially and we do see SP in the input).

I don't know if this is easier to follow or not.  I do think so
right now but that is only because it is still fresh in my brain.

Re: [PATCH 2/6] quote_path: give flags parameter to quote_path()

[Jeff King]

On Thu, Sep 10, 2020 at 08:17:32AM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Of course none of the above becomes unnecessary if we scan the whole
> > string for SP before the main loop in quote-c-style-counted, but the
> > function was written to process the input in a single pass and such
> > a change defeats its design.  If we need to do it in two passes, we
> > can have the caller do so anyway, at least for now.  That thinking
> > lead to the final organization of the series, with two steps that
> > used to be preparatory for passing the flag down thru to the bottom
> > layer rebased out as a discardable appendix at the end.
> 
> Actually, this made me realize that another variant is possible.
> It might be easier to read, or it might not.  Since I cannot tell
> without actually writing one, let's see ...

Vger seems to be delivering slowly and out-of-order the last day or two,
so I got rather confused to receive this after seeing your v2. :)

> I don't know if this is easier to follow or not.  I do think so
> right now but that is only because it is still fresh in my brain.

I do think it is easier to read than the original.

One minor nit with your analysis, though: the current code is actually
two-pass already. One pass finds the next quoted character, but then we
have to take another pass to copy it into place. That second pass can be
done with a memcpy(), which helps.

If you know you are quoting, you can do a true single-pass
character-by-character. But of course part of our task is to find out
_if_ we are quoting. And even if that were not the case, on modern
processors it is not always true that single-pass is going to be faster.
This code is definitely not such a hot-spot that it's worth doing that
kind of micro-optimization.

  Aside: We _do_ have spots where that is not true. When I looked at
  replacing xdiff's hash the sticking point was that we compute the
  newlines _and_ hash in a single pass. Most "fast" hash functions are
  optimized to take bigger sequences of data, but splitting out the
  newline-finding eliminated any gains.

-Peff

[PATCH 3/6] quote_path: optionally allow quoting a path with SP in it

[Junio C Hamano]

Some code in wt-status.c special case a path with SP in it, which
usually does not have to be c-quoted, and ensure that such a path
does get quoted.  Move the logic to quote_path() and give it a bit
in the flags word, QUOTE_PATH_QUOTE_SP.

No behaviour change intended.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 quote.c     |  9 ++++++++-
 quote.h     |  1 +
 wt-status.c | 15 +++------------
 3 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/quote.c b/quote.c
index 03ca85afb0..aa9a37b1b1 100644
--- a/quote.c
+++ b/quote.c
@@ -357,9 +357,16 @@ char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigne
 	struct strbuf sb = STRBUF_INIT;
 	const char *rel = relative_path(in, prefix, &sb);
 	strbuf_reset(out);
-	quote_c_style_counted(rel, strlen(rel), out, NULL, flags);
+	quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
 	strbuf_release(&sb);
 
+	if ((flags & QUOTE_PATH_QUOTE_SP) &&
+	    (out->buf[0] != '"' && strchr(out->buf, ' '))) {
+		/* Ensure the whole thing is quoted if the path has SP in it */
+		strbuf_insertstr(out, 0, "\"");
+		strbuf_addch(out, '"');
+	}
+
 	return out->buf;
 }
 
diff --git a/quote.h b/quote.h
index db7140b3c7..823f56cb94 100644
--- a/quote.h
+++ b/quote.h
@@ -73,6 +73,7 @@ void write_name_quoted_relative(const char *name, const char *prefix,
 
 /* quote path as relative to the given prefix */
 char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigned);
+#define QUOTE_PATH_QUOTE_SP 01
 
 /* quoting as a string literal for other languages */
 void perl_quote_buf(struct strbuf *sb, const char *src);
diff --git a/wt-status.c b/wt-status.c
index d6ca7bd52c..adbf6958bd 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1877,21 +1877,12 @@ static void wt_shortstatus_status(struct string_list_item *it,
 		const char *one;
 
 		if (d->rename_source) {
-			one = quote_path(d->rename_source, s->prefix, &onebuf, 0);
-			if (*one != '"' && strchr(one, ' ') != NULL) {
-				putchar('"');
-				strbuf_addch(&onebuf, '"');
-				one = onebuf.buf;
-			}
+			one = quote_path(d->rename_source, s->prefix, &onebuf,
+					 QUOTE_PATH_QUOTE_SP);
 			printf("%s -> ", one);
 			strbuf_release(&onebuf);
 		}
-		one = quote_path(it->string, s->prefix, &onebuf, 0);
-		if (*one != '"' && strchr(one, ' ') != NULL) {
-			putchar('"');
-			strbuf_addch(&onebuf, '"');
-			one = onebuf.buf;
-		}
+		one = quote_path(it->string, s->prefix, &onebuf, QUOTE_PATH_QUOTE_SP);
 		printf("%s\n", one);
 		strbuf_release(&onebuf);
 	}
-- 
2.28.0-539-g66371d8995

Re: [PATCH 3/6] quote_path: optionally allow quoting a path with SP in it

[Jeff King]

On Tue, Sep 08, 2020 at 01:52:21PM -0700, Junio C Hamano wrote:

> Some code in wt-status.c special case a path with SP in it, which
> usually does not have to be c-quoted, and ensure that such a path
> does get quoted.  Move the logic to quote_path() and give it a bit
> in the flags word, QUOTE_PATH_QUOTE_SP.
> 
> No behaviour change intended.

Sounds like a good direction.

> @@ -357,9 +357,16 @@ char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigne
>  	struct strbuf sb = STRBUF_INIT;
>  	const char *rel = relative_path(in, prefix, &sb);
>  	strbuf_reset(out);
> -	quote_c_style_counted(rel, strlen(rel), out, NULL, flags);
> +	quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
>  	strbuf_release(&sb);
>  
> +	if ((flags & QUOTE_PATH_QUOTE_SP) &&
> +	    (out->buf[0] != '"' && strchr(out->buf, ' '))) {
> +		/* Ensure the whole thing is quoted if the path has SP in it */
> +		strbuf_insertstr(out, 0, "\"");
> +		strbuf_addch(out, '"');
> +	}

This might be premature optimization, but using insertstr() means we
have to recopy the entire string. As a general rule these kind of
"splice into an array" operations tickle my accidentally-quadratic
spider sense. But I don't think that's the case here (because we'd do at
most one insertstr() per string; the real sin would be inserting
characters like that throughout the string).

So it may not be worth addressing. I do think it would be conceptually
simpler if we could just tell quote_c_style_counted() to look for space
characters, but it looks like doing that ends up pretty invasive.

-Peff

[PATCH 4/6] wt-status: consistently quote paths in "status --short" output

[Junio C Hamano]

Tracked paths with SP in them were c-quoted in "git status --short"
output, but untracked, ignored, and unmerged paths weren't.  Treat
them the same way for consistency.

The test was stolen from a patch to fix output for the 'untracked'
paths by brian m. carlson.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7508-status.sh | 21 +++++++++++++++++++++
 wt-status.c       |  4 ++--
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index e81759319f..ef8d19c151 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -814,6 +814,27 @@ test_expect_success 'status -s without relative paths' '
 
 '
 
+cat >expect <<\EOF
+ M dir1/modified
+A  dir2/added
+A  "file with spaces"
+?? dir1/untracked
+?? dir2/modified
+?? dir2/untracked
+?? "file with spaces 2"
+?? untracked
+EOF
+
+test_expect_success 'status -s without relative paths' '
+	test_when_finished "git rm --cached \"file with spaces\"; rm -f file*" &&
+	>"file with spaces" &&
+	>"file with spaces 2" &&
+	git add "file with spaces" &&
+	git status -s >output &&
+	test_cmp expect output
+
+'
+
 test_expect_success 'dry-run of partial commit excluding new file in index' '
 	cat >expect <<EOF &&
 On branch master
diff --git a/wt-status.c b/wt-status.c
index adbf6958bd..7139623025 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1848,7 +1848,7 @@ static void wt_shortstatus_unmerged(struct string_list_item *it,
 	} else {
 		struct strbuf onebuf = STRBUF_INIT;
 		const char *one;
-		one = quote_path(it->string, s->prefix, &onebuf, 0);
+		one = quote_path(it->string, s->prefix, &onebuf, QUOTE_PATH_QUOTE_SP);
 		printf(" %s\n", one);
 		strbuf_release(&onebuf);
 	}
@@ -1896,7 +1896,7 @@ static void wt_shortstatus_other(struct string_list_item *it,
 	} else {
 		struct strbuf onebuf = STRBUF_INIT;
 		const char *one;
-		one = quote_path(it->string, s->prefix, &onebuf, 0);
+		one = quote_path(it->string, s->prefix, &onebuf, QUOTE_PATH_QUOTE_SP);
 		color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "%s", sign);
 		printf(" %s\n", one);
 		strbuf_release(&onebuf);
-- 
2.28.0-539-g66371d8995

[PATCH 5/6] quote: rename misnamed sq_lookup[] to cq_lookup[]

[Junio C Hamano]

This table is used to see if each byte needs quoting when responding
to a request to C-quote the string, not quoting with single-quote in
the shell style.  Similarly, sq_must_quote() is fed each byte from
the string being C-quoted.

No behaviour change intended.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 quote.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/quote.c b/quote.c
index aa9a37b1b1..016ebf8873 100644
--- a/quote.c
+++ b/quote.c
@@ -210,7 +210,7 @@ int sq_dequote_to_strvec(char *arg, struct strvec *array)
  */
 #define X8(x)   x, x, x, x, x, x, x, x
 #define X16(x)  X8(x), X8(x)
-static signed char const sq_lookup[256] = {
+static signed char const cq_lookup[256] = {
 	/*           0    1    2    3    4    5    6    7 */
 	/* 0x00 */   1,   1,   1,   1,   1,   1,   1, 'a',
 	/* 0x08 */ 'b', 't', 'n', 'v', 'f', 'r',   1,   1,
@@ -223,9 +223,9 @@ static signed char const sq_lookup[256] = {
 	/* 0x80 */ /* set to 0 */
 };
 
-static inline int sq_must_quote(char c)
+static inline int cq_must_quote(char c)
 {
-	return sq_lookup[(unsigned char)c] + quote_path_fully > 0;
+	return cq_lookup[(unsigned char)c] + quote_path_fully > 0;
 }
 
 /* returns the longest prefix not needing a quote up to maxlen if positive.
@@ -235,9 +235,9 @@ static size_t next_quote_pos(const char *s, ssize_t maxlen)
 {
 	size_t len;
 	if (maxlen < 0) {
-		for (len = 0; !sq_must_quote(s[len]); len++);
+		for (len = 0; !cq_must_quote(s[len]); len++);
 	} else {
-		for (len = 0; len < maxlen && !sq_must_quote(s[len]); len++);
+		for (len = 0; len < maxlen && !cq_must_quote(s[len]); len++);
 	}
 	return len;
 }
@@ -291,8 +291,8 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
 		ch = (unsigned char)*p++;
 		if (maxlen >= 0)
 			maxlen -= len + 1;
-		if (sq_lookup[ch] >= ' ') {
-			EMIT(sq_lookup[ch]);
+		if (cq_lookup[ch] >= ' ') {
+			EMIT(cq_lookup[ch]);
 		} else {
 			EMIT(((ch >> 6) & 03) + '0');
 			EMIT(((ch >> 3) & 07) + '0');
-- 
2.28.0-539-g66371d8995

[PATCH 6/6] quote: turn 'nodq' parameter into a set of flags

[Junio C Hamano]

quote_c_style() and its friend quote_two_c_style() both take an
optional "please omit the double quotes around the quoted body"
parameter.  Turn it into a flag word, assign one bit out of it,
and call it CQUOTE_NODQ bit.

No behaviour change intended.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c  |  8 ++++----
 quote.c | 15 +++++++++------
 quote.h |  7 +++++--
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/diff.c b/diff.c
index 0299a73079..e7d6e60b23 100644
--- a/diff.c
+++ b/diff.c
@@ -482,14 +482,14 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 
 static char *quote_two(const char *one, const char *two)
 {
-	int need_one = quote_c_style(one, NULL, NULL, 1);
-	int need_two = quote_c_style(two, NULL, NULL, 1);
+	int need_one = quote_c_style(one, NULL, NULL, CQUOTE_NODQ);
+	int need_two = quote_c_style(two, NULL, NULL, CQUOTE_NODQ);
 	struct strbuf res = STRBUF_INIT;
 
 	if (need_one + need_two) {
 		strbuf_addch(&res, '"');
-		quote_c_style(one, &res, NULL, 1);
-		quote_c_style(two, &res, NULL, 1);
+		quote_c_style(one, &res, NULL, CQUOTE_NODQ);
+		quote_c_style(two, &res, NULL, CQUOTE_NODQ);
 		strbuf_addch(&res, '"');
 	} else {
 		strbuf_addstr(&res, one);
diff --git a/quote.c b/quote.c
index 016ebf8873..b2f38d0e57 100644
--- a/quote.c
+++ b/quote.c
@@ -256,7 +256,7 @@ static size_t next_quote_pos(const char *s, ssize_t maxlen)
  *     Return value is the same as in (1).
  */
 static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
-				    struct strbuf *sb, FILE *fp, int no_dq)
+				    struct strbuf *sb, FILE *fp, unsigned flags)
 {
 #undef EMIT
 #define EMIT(c)                                 \
@@ -272,6 +272,7 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
 		count += (l);                           \
 	} while (0)
 
+	int no_dq = !!(flags & CQUOTE_NODQ);
 	size_t len, count = 0;
 	const char *p = name;
 
@@ -309,19 +310,21 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
 	return count;
 }
 
-size_t quote_c_style(const char *name, struct strbuf *sb, FILE *fp, int nodq)
+size_t quote_c_style(const char *name, struct strbuf *sb, FILE *fp, unsigned flags)
 {
-	return quote_c_style_counted(name, -1, sb, fp, nodq);
+	return quote_c_style_counted(name, -1, sb, fp, flags);
 }
 
-void quote_two_c_style(struct strbuf *sb, const char *prefix, const char *path, int nodq)
+void quote_two_c_style(struct strbuf *sb, const char *prefix, const char *path,
+		       unsigned flags)
 {
+	int nodq = !!(flags & CQUOTE_NODQ);
 	if (quote_c_style(prefix, NULL, NULL, 0) ||
 	    quote_c_style(path, NULL, NULL, 0)) {
 		if (!nodq)
 			strbuf_addch(sb, '"');
-		quote_c_style(prefix, sb, NULL, 1);
-		quote_c_style(path, sb, NULL, 1);
+		quote_c_style(prefix, sb, NULL, CQUOTE_NODQ);
+		quote_c_style(path, sb, NULL, CQUOTE_NODQ);
 		if (!nodq)
 			strbuf_addch(sb, '"');
 	} else {
diff --git a/quote.h b/quote.h
index 823f56cb94..0e49c61144 100644
--- a/quote.h
+++ b/quote.h
@@ -64,8 +64,11 @@ struct strvec;
 int sq_dequote_to_strvec(char *arg, struct strvec *);
 
 int unquote_c_style(struct strbuf *, const char *quoted, const char **endp);
-size_t quote_c_style(const char *name, struct strbuf *, FILE *, int no_dq);
-void quote_two_c_style(struct strbuf *, const char *, const char *, int);
+
+/* Bits in the flags parameter to quote_c_style() */
+#define CQUOTE_NODQ 01
+size_t quote_c_style(const char *name, struct strbuf *, FILE *, unsigned);
+void quote_two_c_style(struct strbuf *, const char *, const char *, unsigned);
 
 void write_name_quoted(const char *name, FILE *, int terminator);
 void write_name_quoted_relative(const char *name, const char *prefix,
-- 
2.28.0-539-g66371d8995

Re: [PATCH 6/6] quote: turn 'nodq' parameter into a set of flags

[Jeff King]

On Tue, Sep 08, 2020 at 01:52:24PM -0700, Junio C Hamano wrote:

> quote_c_style() and its friend quote_two_c_style() both take an
> optional "please omit the double quotes around the quoted body"
> parameter.  Turn it into a flag word, assign one bit out of it,
> and call it CQUOTE_NODQ bit.

Sounds reasonable.

If this shared the same "flags" namespace as quote_path(), then we
really could pass quote_path() flags along. And in fact, that could be
the first step to just teaching quote_c_style() the "if it has spaces,
then quote it" rule. Maybe not worth spending more time on, but just
thinking out loud after my earlier comments.

> @@ -272,6 +272,7 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
>  		count += (l);                           \
>  	} while (0)
>  
> +	int no_dq = !!(flags & CQUOTE_NODQ);
>  	size_t len, count = 0;
>  	const char *p = name;

Looking at the context, I wondered how this was not adding a
decl-after-statement (and how we were not already complaining about the
existing ones). But the reason is that the code above it is part of a
macro.

-Peff

Re: [PATCH 0/6] quote_path() clean-ups

[Chris Torek]

On Tue, Sep 8, 2020 at 1:56 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> So, this is an alternative approach to tackle the same issue
> <xmqq4ko8yxp9.fsf@gitster.c.googlers.com> tried to address.  It
> ended up to be more involved than I would have liked, but it was
> primarily because it needed some function signature changes.


Not that it necessarily means much, but I gave the whole series
a quick eyeball scan and it looks good. I like the idea of using one
consistent "quote the path" routine.

Chris

Re: [PATCH 0/6] quote_path() clean-ups

[Jeff King]

On Tue, Sep 08, 2020 at 01:52:18PM -0700, Junio C Hamano wrote:

> So, this is an alternative approach to tackle the same issue
> <xmqq4ko8yxp9.fsf@gitster.c.googlers.com> tried to address.  It
> ended up to be more involved than I would have liked, but it was
> primarily because it needed some function signature changes.

This looks like an improvement to me. I left a few minor comments that
might be worth addressing, and a few whose resolution is probably "well,
we _could_ do that but it's not worth spending more time on this".

-Peff

[PATCH v2 0/7] quote_path() clean-ups

[Junio C Hamano]

Here is an update, after seeing Peff's review.

The overall structure of the series stays the same.

 * The second patch lost an incorrect use of 'flags' parameter down
   to quote_c_style_counted() from quote_path().  In the function
   declaration of quote_path(), the flags parameter is now named.

 * The third patch that moves the "optionally quote path with SP in
   it" logic to quote_path() is essentialy unchanged.

 * But the fourth patch rewrites the implementation to avoid
   shifting the output bytes with insertstr().

 * The fifth patch (formerly the fourth) that teachs "status --short"
   to consistently quote hasn't changed, except that we test also
   for the ignored paths.

 * The last two patches are unchanged.  They are optional clean-ups
   that are not required.

We might want to add a bit more tests for:

 - how conflicted "funny" paths are shown.

 - how doubly funny paths (those that quote_c_style_counted() needs
   to use backslash quoting *and* that contain SP) are shown.

but I'd say that is outside the scope of this round.  Seeing what is
done in t3300, the latter test cannot be written portably as far as
I can tell.


Junio C Hamano (7):
  quote_path: rename quote_path_relative() to quote_path()
  quote_path: give flags parameter to quote_path()
  quote_path: optionally allow quoting a path with SP in it
  quote_path: code clarification
  wt-status: consistently quote paths in "status --short" output
  quote: rename misnamed sq_lookup[] to cq_lookup[]
  quote: turn 'nodq' parameter into a set of flags

 builtin/clean.c   | 22 +++++++++++-----------
 builtin/grep.c    |  2 +-
 diff.c            |  8 ++++----
 quote.c           | 47 +++++++++++++++++++++++++++++++----------------
 quote.h           | 11 +++++++----
 t/t7508-status.sh | 27 +++++++++++++++++++++++++++
 wt-status.c       | 37 +++++++++++++------------------------
 7 files changed, 94 insertions(+), 60 deletions(-)

Range-diff against v1:
1:  a9306429f4 = 1:  a9306429f4 quote_path: rename quote_path_relative() to quote_path()
2:  c2784628a4 ! 2:  179d12d16f quote_path: give flags parameter to quote_path()
    @@ quote.c: void write_name_quoted_relative(const char *name, const char *prefix,
      {
      	struct strbuf sb = STRBUF_INIT;
      	const char *rel = relative_path(in, prefix, &sb);
    - 	strbuf_reset(out);
    --	quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
    -+	quote_c_style_counted(rel, strlen(rel), out, NULL, flags);
    - 	strbuf_release(&sb);
    - 
    - 	return out->buf;
     
      ## quote.h ##
     @@ quote.h: void write_name_quoted_relative(const char *name, const char *prefix,
    @@ quote.h: void write_name_quoted_relative(const char *name, const char *prefix,
      
      /* quote path as relative to the given prefix */
     -char *quote_path(const char *in, const char *prefix, struct strbuf *out);
    -+char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigned);
    ++char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigned flags);
      
      /* quoting as a string literal for other languages */
      void perl_quote_buf(struct strbuf *sb, const char *src);
3:  640673148e ! 3:  d566c38d2f quote_path: optionally allow quoting a path with SP in it
    @@ Commit message
     
      ## quote.c ##
     @@ quote.c: char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigne
    - 	struct strbuf sb = STRBUF_INIT;
    - 	const char *rel = relative_path(in, prefix, &sb);
    - 	strbuf_reset(out);
    --	quote_c_style_counted(rel, strlen(rel), out, NULL, flags);
    -+	quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
    + 	quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
      	strbuf_release(&sb);
      
     +	if ((flags & QUOTE_PATH_QUOTE_SP) &&
    @@ quote.h
     @@ quote.h: void write_name_quoted_relative(const char *name, const char *prefix,
      
      /* quote path as relative to the given prefix */
    - char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigned);
    + char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigned flags);
     +#define QUOTE_PATH_QUOTE_SP 01
      
      /* quoting as a string literal for other languages */
-:  ---------- > 4:  f3769b7cf4 quote_path: code clarification
4:  ac9a0c4a33 ! 5:  498bed71e4 wt-status: consistently quote paths in "status --short" output
    @@ Commit message
         output, but untracked, ignored, and unmerged paths weren't.
     
         The test was stolen from a patch to fix output for the 'untracked'
    -    paths by brian m. carlson.
    +    paths by brian m. carlson, with similar tests added for 'ignored'
    +    ones.
     
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
    @@ t/t7508-status.sh: test_expect_success 'status -s without relative paths' '
     +	test_when_finished "git rm --cached \"file with spaces\"; rm -f file*" &&
     +	>"file with spaces" &&
     +	>"file with spaces 2" &&
    ++	>"expect with spaces" &&
     +	git add "file with spaces" &&
    ++
     +	git status -s >output &&
    -+	test_cmp expect output
    ++	test_cmp expect output &&
     +
    ++	git status -s --ignored >output &&
    ++	grep "^!! \"expect with spaces\"$" output &&
    ++	grep -v "^!! " output >output-wo-ignored &&
    ++	test_cmp expect output-wo-ignored
     +'
     +
      test_expect_success 'dry-run of partial commit excluding new file in index' '
5:  57c6294695 = 6:  e74186bbd7 quote: rename misnamed sq_lookup[] to cq_lookup[]
6:  480152cfe4 ! 7:  d87d7dd561 quote: turn 'nodq' parameter into a set of flags
    @@ quote.c: static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
      		if (!nodq)
      			strbuf_addch(sb, '"');
      	} else {
    +@@ quote.c: char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigne
    + 	 */
    + 	if (force_dq)
    + 		strbuf_addch(out, '"');
    +-	quote_c_style_counted(rel, strlen(rel), out, NULL, !!force_dq);
    ++	quote_c_style_counted(rel, strlen(rel), out, NULL,
    ++			      force_dq ? CQUOTE_NODQ : 0);
    + 	if (force_dq)
    + 		strbuf_addch(out, '"');
    + 	strbuf_release(&sb);
     
      ## quote.h ##
     @@ quote.h: struct strvec;
-- 
2.28.0-603-ga98dad7d4d

[PATCH v2 1/7] quote_path: rename quote_path_relative() to quote_path()

[Junio C Hamano]

There is no quote_path_absolute() or anything that causes confusion,
and one of the two large consumers already rename the long name
locally with a preprocessor macro.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/clean.c | 22 +++++++++++-----------
 builtin/grep.c  |  2 +-
 quote.c         |  3 +--
 quote.h         |  3 +--
 wt-status.c     |  2 --
 5 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index e53ea52d89..dee44fff6e 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -162,7 +162,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 	if ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
 	    is_nonbare_repository_dir(path)) {
 		if (!quiet) {
-			quote_path_relative(path->buf, prefix, &quoted);
+			quote_path(path->buf, prefix, &quoted);
 			printf(dry_run ?  _(msg_would_skip_git_dir) : _(msg_skip_git_dir),
 					quoted.buf);
 		}
@@ -177,7 +177,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 		res = dry_run ? 0 : rmdir(path->buf);
 		if (res) {
 			int saved_errno = errno;
-			quote_path_relative(path->buf, prefix, &quoted);
+			quote_path(path->buf, prefix, &quoted);
 			errno = saved_errno;
 			warning_errno(_(msg_warn_remove_failed), quoted.buf);
 			*dir_gone = 0;
@@ -202,7 +202,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 			if (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))
 				ret = 1;
 			if (gone) {
-				quote_path_relative(path->buf, prefix, &quoted);
+				quote_path(path->buf, prefix, &quoted);
 				string_list_append(&dels, quoted.buf);
 			} else
 				*dir_gone = 0;
@@ -210,11 +210,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 		} else {
 			res = dry_run ? 0 : unlink(path->buf);
 			if (!res) {
-				quote_path_relative(path->buf, prefix, &quoted);
+				quote_path(path->buf, prefix, &quoted);
 				string_list_append(&dels, quoted.buf);
 			} else {
 				int saved_errno = errno;
-				quote_path_relative(path->buf, prefix, &quoted);
+				quote_path(path->buf, prefix, &quoted);
 				errno = saved_errno;
 				warning_errno(_(msg_warn_remove_failed), quoted.buf);
 				*dir_gone = 0;
@@ -238,7 +238,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 			*dir_gone = 1;
 		else {
 			int saved_errno = errno;
-			quote_path_relative(path->buf, prefix, &quoted);
+			quote_path(path->buf, prefix, &quoted);
 			errno = saved_errno;
 			warning_errno(_(msg_warn_remove_failed), quoted.buf);
 			*dir_gone = 0;
@@ -266,7 +266,7 @@ static void pretty_print_dels(void)
 	struct column_options copts;
 
 	for_each_string_list_item(item, &del_list) {
-		qname = quote_path_relative(item->string, NULL, &buf);
+		qname = quote_path(item->string, NULL, &buf);
 		string_list_append(&list, qname);
 	}
 
@@ -753,7 +753,7 @@ static int ask_each_cmd(void)
 	for_each_string_list_item(item, &del_list) {
 		/* Ctrl-D should stop removing files */
 		if (!eof) {
-			qname = quote_path_relative(item->string, NULL, &buf);
+			qname = quote_path(item->string, NULL, &buf);
 			/* TRANSLATORS: Make sure to keep [y/N] as is */
 			printf(_("Remove %s [y/N]? "), qname);
 			if (git_read_line_interactively(&confirm) == EOF) {
@@ -1047,19 +1047,19 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 			if (remove_dirs(&abs_path, prefix, rm_flags, dry_run, quiet, &gone))
 				errors++;
 			if (gone && !quiet) {
-				qname = quote_path_relative(item->string, NULL, &buf);
+				qname = quote_path(item->string, NULL, &buf);
 				printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
 			}
 		} else {
 			res = dry_run ? 0 : unlink(abs_path.buf);
 			if (res) {
 				int saved_errno = errno;
-				qname = quote_path_relative(item->string, NULL, &buf);
+				qname = quote_path(item->string, NULL, &buf);
 				errno = saved_errno;
 				warning_errno(_(msg_warn_remove_failed), qname);
 				errors++;
 			} else if (!quiet) {
-				qname = quote_path_relative(item->string, NULL, &buf);
+				qname = quote_path(item->string, NULL, &buf);
 				printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
 			}
 		}
diff --git a/builtin/grep.c b/builtin/grep.c
index f58979bc3f..9a91ad643a 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -319,7 +319,7 @@ static void grep_source_name(struct grep_opt *opt, const char *filename,
 	}
 
 	if (opt->relative && opt->prefix_length)
-		quote_path_relative(filename + tree_name_len, opt->prefix, out);
+		quote_path(filename + tree_name_len, opt->prefix, out);
 	else
 		quote_c_style(filename + tree_name_len, out, NULL, 0);
 
diff --git a/quote.c b/quote.c
index ced0245e80..7bb519c1a7 100644
--- a/quote.c
+++ b/quote.c
@@ -352,8 +352,7 @@ void write_name_quoted_relative(const char *name, const char *prefix,
 }
 
 /* quote path as relative to the given prefix */
-char *quote_path_relative(const char *in, const char *prefix,
-			  struct strbuf *out)
+char *quote_path(const char *in, const char *prefix, struct strbuf *out)
 {
 	struct strbuf sb = STRBUF_INIT;
 	const char *rel = relative_path(in, prefix, &sb);
diff --git a/quote.h b/quote.h
index fa09309cf6..837cb42a71 100644
--- a/quote.h
+++ b/quote.h
@@ -72,8 +72,7 @@ void write_name_quoted_relative(const char *name, const char *prefix,
 				FILE *fp, int terminator);
 
 /* quote path as relative to the given prefix */
-char *quote_path_relative(const char *in, const char *prefix,
-			  struct strbuf *out);
+char *quote_path(const char *in, const char *prefix, struct strbuf *out);
 
 /* quoting as a string literal for other languages */
 void perl_quote_buf(struct strbuf *sb, const char *src);
diff --git a/wt-status.c b/wt-status.c
index bb0f9120de..6b87592856 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -259,8 +259,6 @@ static void wt_longstatus_print_trailer(struct wt_status *s)
 	status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
 }
 
-#define quote_path quote_path_relative
-
 static const char *wt_status_unmerged_status_string(int stagemask)
 {
 	switch (stagemask) {
-- 
2.28.0-603-ga98dad7d4d

[PATCH v2 2/7] quote_path: give flags parameter to quote_path()

[Junio C Hamano]

The quote_path() function computes a path (relative to its base
directory) and c-quotes the result if necessary.  Teach it to take a
flags parameter to allow its behaviour to be enriched later.

No behaviour change intended.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/clean.c | 22 +++++++++++-----------
 builtin/grep.c  |  2 +-
 quote.c         |  2 +-
 quote.h         |  2 +-
 wt-status.c     | 24 ++++++++++++------------
 5 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index dee44fff6e..687ab473c2 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -162,7 +162,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 	if ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
 	    is_nonbare_repository_dir(path)) {
 		if (!quiet) {
-			quote_path(path->buf, prefix, &quoted);
+			quote_path(path->buf, prefix, &quoted, 0);
 			printf(dry_run ?  _(msg_would_skip_git_dir) : _(msg_skip_git_dir),
 					quoted.buf);
 		}
@@ -177,7 +177,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 		res = dry_run ? 0 : rmdir(path->buf);
 		if (res) {
 			int saved_errno = errno;
-			quote_path(path->buf, prefix, &quoted);
+			quote_path(path->buf, prefix, &quoted, 0);
 			errno = saved_errno;
 			warning_errno(_(msg_warn_remove_failed), quoted.buf);
 			*dir_gone = 0;
@@ -202,7 +202,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 			if (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))
 				ret = 1;
 			if (gone) {
-				quote_path(path->buf, prefix, &quoted);
+				quote_path(path->buf, prefix, &quoted, 0);
 				string_list_append(&dels, quoted.buf);
 			} else
 				*dir_gone = 0;
@@ -210,11 +210,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 		} else {
 			res = dry_run ? 0 : unlink(path->buf);
 			if (!res) {
-				quote_path(path->buf, prefix, &quoted);
+				quote_path(path->buf, prefix, &quoted, 0);
 				string_list_append(&dels, quoted.buf);
 			} else {
 				int saved_errno = errno;
-				quote_path(path->buf, prefix, &quoted);
+				quote_path(path->buf, prefix, &quoted, 0);
 				errno = saved_errno;
 				warning_errno(_(msg_warn_remove_failed), quoted.buf);
 				*dir_gone = 0;
@@ -238,7 +238,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 			*dir_gone = 1;
 		else {
 			int saved_errno = errno;
-			quote_path(path->buf, prefix, &quoted);
+			quote_path(path->buf, prefix, &quoted, 0);
 			errno = saved_errno;
 			warning_errno(_(msg_warn_remove_failed), quoted.buf);
 			*dir_gone = 0;
@@ -266,7 +266,7 @@ static void pretty_print_dels(void)
 	struct column_options copts;
 
 	for_each_string_list_item(item, &del_list) {
-		qname = quote_path(item->string, NULL, &buf);
+		qname = quote_path(item->string, NULL, &buf, 0);
 		string_list_append(&list, qname);
 	}
 
@@ -753,7 +753,7 @@ static int ask_each_cmd(void)
 	for_each_string_list_item(item, &del_list) {
 		/* Ctrl-D should stop removing files */
 		if (!eof) {
-			qname = quote_path(item->string, NULL, &buf);
+			qname = quote_path(item->string, NULL, &buf, 0);
 			/* TRANSLATORS: Make sure to keep [y/N] as is */
 			printf(_("Remove %s [y/N]? "), qname);
 			if (git_read_line_interactively(&confirm) == EOF) {
@@ -1047,19 +1047,19 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 			if (remove_dirs(&abs_path, prefix, rm_flags, dry_run, quiet, &gone))
 				errors++;
 			if (gone && !quiet) {
-				qname = quote_path(item->string, NULL, &buf);
+				qname = quote_path(item->string, NULL, &buf, 0);
 				printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
 			}
 		} else {
 			res = dry_run ? 0 : unlink(abs_path.buf);
 			if (res) {
 				int saved_errno = errno;
-				qname = quote_path(item->string, NULL, &buf);
+				qname = quote_path(item->string, NULL, &buf, 0);
 				errno = saved_errno;
 				warning_errno(_(msg_warn_remove_failed), qname);
 				errors++;
 			} else if (!quiet) {
-				qname = quote_path(item->string, NULL, &buf);
+				qname = quote_path(item->string, NULL, &buf, 0);
 				printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
 			}
 		}
diff --git a/builtin/grep.c b/builtin/grep.c
index 9a91ad643a..c8037388c6 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -319,7 +319,7 @@ static void grep_source_name(struct grep_opt *opt, const char *filename,
 	}
 
 	if (opt->relative && opt->prefix_length)
-		quote_path(filename + tree_name_len, opt->prefix, out);
+		quote_path(filename + tree_name_len, opt->prefix, out, 0);
 	else
 		quote_c_style(filename + tree_name_len, out, NULL, 0);
 
diff --git a/quote.c b/quote.c
index 7bb519c1a7..a86f9f22a2 100644
--- a/quote.c
+++ b/quote.c
@@ -352,7 +352,7 @@ void write_name_quoted_relative(const char *name, const char *prefix,
 }
 
 /* quote path as relative to the given prefix */
-char *quote_path(const char *in, const char *prefix, struct strbuf *out)
+char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigned flags)
 {
 	struct strbuf sb = STRBUF_INIT;
 	const char *rel = relative_path(in, prefix, &sb);
diff --git a/quote.h b/quote.h
index 837cb42a71..4687b5daf4 100644
--- a/quote.h
+++ b/quote.h
@@ -72,7 +72,7 @@ void write_name_quoted_relative(const char *name, const char *prefix,
 				FILE *fp, int terminator);
 
 /* quote path as relative to the given prefix */
-char *quote_path(const char *in, const char *prefix, struct strbuf *out);
+char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigned flags);
 
 /* quoting as a string literal for other languages */
 void perl_quote_buf(struct strbuf *sb, const char *src);
diff --git a/wt-status.c b/wt-status.c
index 6b87592856..d6ca7bd52c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -336,7 +336,7 @@ static void wt_longstatus_print_unmerged_data(struct wt_status *s,
 		memset(padding, ' ', label_width);
 	}
 
-	one = quote_path(it->string, s->prefix, &onebuf);
+	one = quote_path(it->string, s->prefix, &onebuf, 0);
 	status_printf(s, color(WT_STATUS_HEADER, s), "\t");
 
 	how = wt_status_unmerged_status_string(d->stagemask);
@@ -402,8 +402,8 @@ static void wt_longstatus_print_change_data(struct wt_status *s,
 	if (d->rename_status == status)
 		one_name = d->rename_source;
 
-	one = quote_path(one_name, s->prefix, &onebuf);
-	two = quote_path(two_name, s->prefix, &twobuf);
+	one = quote_path(one_name, s->prefix, &onebuf, 0);
+	two = quote_path(two_name, s->prefix, &twobuf, 0);
 
 	status_printf(s, color(WT_STATUS_HEADER, s), "\t");
 	what = wt_status_diff_status_string(status);
@@ -964,7 +964,7 @@ static void wt_longstatus_print_other(struct wt_status *s,
 		struct string_list_item *it;
 		const char *path;
 		it = &(l->items[i]);
-		path = quote_path(it->string, s->prefix, &buf);
+		path = quote_path(it->string, s->prefix, &buf, 0);
 		if (column_active(s->colopts)) {
 			string_list_append(&output, path);
 			continue;
@@ -1848,7 +1848,7 @@ static void wt_shortstatus_unmerged(struct string_list_item *it,
 	} else {
 		struct strbuf onebuf = STRBUF_INIT;
 		const char *one;
-		one = quote_path(it->string, s->prefix, &onebuf);
+		one = quote_path(it->string, s->prefix, &onebuf, 0);
 		printf(" %s\n", one);
 		strbuf_release(&onebuf);
 	}
@@ -1877,7 +1877,7 @@ static void wt_shortstatus_status(struct string_list_item *it,
 		const char *one;
 
 		if (d->rename_source) {
-			one = quote_path(d->rename_source, s->prefix, &onebuf);
+			one = quote_path(d->rename_source, s->prefix, &onebuf, 0);
 			if (*one != '"' && strchr(one, ' ') != NULL) {
 				putchar('"');
 				strbuf_addch(&onebuf, '"');
@@ -1886,7 +1886,7 @@ static void wt_shortstatus_status(struct string_list_item *it,
 			printf("%s -> ", one);
 			strbuf_release(&onebuf);
 		}
-		one = quote_path(it->string, s->prefix, &onebuf);
+		one = quote_path(it->string, s->prefix, &onebuf, 0);
 		if (*one != '"' && strchr(one, ' ') != NULL) {
 			putchar('"');
 			strbuf_addch(&onebuf, '"');
@@ -1905,7 +1905,7 @@ static void wt_shortstatus_other(struct string_list_item *it,
 	} else {
 		struct strbuf onebuf = STRBUF_INIT;
 		const char *one;
-		one = quote_path(it->string, s->prefix, &onebuf);
+		one = quote_path(it->string, s->prefix, &onebuf, 0);
 		color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "%s", sign);
 		printf(" %s\n", one);
 		strbuf_release(&onebuf);
@@ -2222,9 +2222,9 @@ static void wt_porcelain_v2_print_changed_entry(
 		 */
 		sep_char = '\t';
 		eol_char = '\n';
-		path = quote_path(it->string, s->prefix, &buf);
+		path = quote_path(it->string, s->prefix, &buf, 0);
 		if (d->rename_source)
-			path_from = quote_path(d->rename_source, s->prefix, &buf_from);
+			path_from = quote_path(d->rename_source, s->prefix, &buf_from, 0);
 	}
 
 	if (path_from)
@@ -2310,7 +2310,7 @@ static void wt_porcelain_v2_print_unmerged_entry(
 	if (s->null_termination)
 		path_index = it->string;
 	else
-		path_index = quote_path(it->string, s->prefix, &buf_index);
+		path_index = quote_path(it->string, s->prefix, &buf_index, 0);
 
 	fprintf(s->fp, "%c %s %s %06o %06o %06o %06o %s %s %s %s%c",
 			unmerged_prefix, key, submodule_token,
@@ -2343,7 +2343,7 @@ static void wt_porcelain_v2_print_other(
 		path = it->string;
 		eol_char = '\0';
 	} else {
-		path = quote_path(it->string, s->prefix, &buf);
+		path = quote_path(it->string, s->prefix, &buf, 0);
 		eol_char = '\n';
 	}
 
-- 
2.28.0-603-ga98dad7d4d

[PATCH v2 3/7] quote_path: optionally allow quoting a path with SP in it

[Junio C Hamano]

Some code in wt-status.c special case a path with SP in it, which
usually does not have to be c-quoted, and ensure that such a path
does get quoted.  Move the logic to quote_path() and give it a bit
in the flags word, QUOTE_PATH_QUOTE_SP.

No behaviour change intended.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 quote.c     |  7 +++++++
 quote.h     |  1 +
 wt-status.c | 15 +++------------
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/quote.c b/quote.c
index a86f9f22a2..aa9a37b1b1 100644
--- a/quote.c
+++ b/quote.c
@@ -360,6 +360,13 @@ char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigne
 	quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
 	strbuf_release(&sb);
 
+	if ((flags & QUOTE_PATH_QUOTE_SP) &&
+	    (out->buf[0] != '"' && strchr(out->buf, ' '))) {
+		/* Ensure the whole thing is quoted if the path has SP in it */
+		strbuf_insertstr(out, 0, "\"");
+		strbuf_addch(out, '"');
+	}
+
 	return out->buf;
 }
 
diff --git a/quote.h b/quote.h
index 4687b5daf4..1918d1e00e 100644
--- a/quote.h
+++ b/quote.h
@@ -73,6 +73,7 @@ void write_name_quoted_relative(const char *name, const char *prefix,
 
 /* quote path as relative to the given prefix */
 char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigned flags);
+#define QUOTE_PATH_QUOTE_SP 01
 
 /* quoting as a string literal for other languages */
 void perl_quote_buf(struct strbuf *sb, const char *src);
diff --git a/wt-status.c b/wt-status.c
index d6ca7bd52c..adbf6958bd 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1877,21 +1877,12 @@ static void wt_shortstatus_status(struct string_list_item *it,
 		const char *one;
 
 		if (d->rename_source) {
-			one = quote_path(d->rename_source, s->prefix, &onebuf, 0);
-			if (*one != '"' && strchr(one, ' ') != NULL) {
-				putchar('"');
-				strbuf_addch(&onebuf, '"');
-				one = onebuf.buf;
-			}
+			one = quote_path(d->rename_source, s->prefix, &onebuf,
+					 QUOTE_PATH_QUOTE_SP);
 			printf("%s -> ", one);
 			strbuf_release(&onebuf);
 		}
-		one = quote_path(it->string, s->prefix, &onebuf, 0);
-		if (*one != '"' && strchr(one, ' ') != NULL) {
-			putchar('"');
-			strbuf_addch(&onebuf, '"');
-			one = onebuf.buf;
-		}
+		one = quote_path(it->string, s->prefix, &onebuf, QUOTE_PATH_QUOTE_SP);
 		printf("%s\n", one);
 		strbuf_release(&onebuf);
 	}
-- 
2.28.0-603-ga98dad7d4d

[PATCH v2 4/7] quote_path: code clarification

[Junio C Hamano]

The implementation we moved from wt-status to enclose a pathname
that has a SP in it inside a dq-pair is a bit convoluted.  It lets
quote_c_style_counted() do its escaping and then

 (1) if the input string got escaped, which is checked by seeing if
     the result begins with a double-quote, declare that we are
     done.  If there wasn't any SP in the input, that is OK, and if
     there was, the result is quoted already so it is OK, too.

 (2) if the input string did not get escaped, and the result has SP
     in it, enclose the whole thing in a dq-pair ourselves.

Instead we can scan the path upfront to see if the input has SP in
it.  If so, we tell quote_c_style_counted() not to enclose its
output in a dq-pair, and we add a dq-pair ourselves.  Whether the
input had bytes that quote_c_style_counted() uses backslash quoting,
this would give us a desired quoted string.  If the input does not
have SP in it, we just let quote_c_style_counted() do its thing as
usual, which would enclose the result in a dq-pair only when needed.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 quote.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/quote.c b/quote.c
index aa9a37b1b1..b8107cd403 100644
--- a/quote.c
+++ b/quote.c
@@ -356,16 +356,21 @@ char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigne
 {
 	struct strbuf sb = STRBUF_INIT;
 	const char *rel = relative_path(in, prefix, &sb);
+	int force_dq = ((flags & QUOTE_PATH_QUOTE_SP) && strchr(rel, ' '));
+
 	strbuf_reset(out);
-	quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
-	strbuf_release(&sb);
 
-	if ((flags & QUOTE_PATH_QUOTE_SP) &&
-	    (out->buf[0] != '"' && strchr(out->buf, ' '))) {
-		/* Ensure the whole thing is quoted if the path has SP in it */
-		strbuf_insertstr(out, 0, "\"");
+	/*
+	 * If the caller wants us to enclose the output in a dq-pair
+	 * whether quote_c_style_counted() needs to, we do it ourselves
+	 * and tell quote_c_style_counted() not to.
+	 */
+	if (force_dq)
 		strbuf_addch(out, '"');
-	}
+	quote_c_style_counted(rel, strlen(rel), out, NULL, !!force_dq);
+	if (force_dq)
+		strbuf_addch(out, '"');
+	strbuf_release(&sb);
 
 	return out->buf;
 }
-- 
2.28.0-603-ga98dad7d4d

Re: [PATCH v2 4/7] quote_path: code clarification

[Jeff King]

On Thu, Sep 10, 2020 at 10:01:56AM -0700, Junio C Hamano wrote:

> Instead we can scan the path upfront to see if the input has SP in
> it.  If so, we tell quote_c_style_counted() not to enclose its
> output in a dq-pair, and we add a dq-pair ourselves.  Whether the
> input had bytes that quote_c_style_counted() uses backslash quoting,
> this would give us a desired quoted string.  If the input does not
> have SP in it, we just let quote_c_style_counted() do its thing as
> usual, which would enclose the result in a dq-pair only when needed.

Nice. I think the result is easier to follow than the original, not to
mention more efficient. And the fact that we didn't need to touch
quote_c_style_counted() is the cherry on top.

> +	int force_dq = ((flags & QUOTE_PATH_QUOTE_SP) && strchr(rel, ' '));
> [...]
> +	quote_c_style_counted(rel, strlen(rel), out, NULL, !!force_dq);

I think force_dq is already normalized to 0/1 by the &&, so we wouldn't
need the "!!" here.

-Peff

Re: [PATCH v2 4/7] quote_path: code clarification

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Sep 10, 2020 at 10:01:56AM -0700, Junio C Hamano wrote:
>
>> Instead we can scan the path upfront to see if the input has SP in
>> it.  If so, we tell quote_c_style_counted() not to enclose its
>> output in a dq-pair, and we add a dq-pair ourselves.  Whether the
>> input had bytes that quote_c_style_counted() uses backslash quoting,
>> this would give us a desired quoted string.  If the input does not
>> have SP in it, we just let quote_c_style_counted() do its thing as
>> usual, which would enclose the result in a dq-pair only when needed.
>
> Nice. I think the result is easier to follow than the original, not to
> mention more efficient. And the fact that we didn't need to touch
> quote_c_style_counted() is the cherry on top.
>
>> +	int force_dq = ((flags & QUOTE_PATH_QUOTE_SP) && strchr(rel, ' '));
>> [...]
>> +	quote_c_style_counted(rel, strlen(rel), out, NULL, !!force_dq);
>
> I think force_dq is already normalized to 0/1 by the &&, so we wouldn't
> need the "!!" here.

True.  CQUOTE_NODQ patch would get rid of it in the end, though ;-)

Re: [PATCH v2 4/7] quote_path: code clarification

[Jeff King]

On Thu, Sep 10, 2020 at 11:40:42AM -0700, Junio C Hamano wrote:

> >> +	int force_dq = ((flags & QUOTE_PATH_QUOTE_SP) && strchr(rel, ' '));
> >> [...]
> >> +	quote_c_style_counted(rel, strlen(rel), out, NULL, !!force_dq);
> >
> > I think force_dq is already normalized to 0/1 by the &&, so we wouldn't
> > need the "!!" here.
> 
> True.  CQUOTE_NODQ patch would get rid of it in the end, though ;-)

Oh, true. I didn't think to look ahead. Having done so, yeah, I don't
see any point in quibbling about this one. :)

-Peff

[PATCH v2 5/7] wt-status: consistently quote paths in "status --short" output

[Junio C Hamano]

Tracked paths with SP in them were cquoted in "git status --short"
output, but untracked, ignored, and unmerged paths weren't.

The test was stolen from a patch to fix output for the 'untracked'
paths by brian m. carlson, with similar tests added for 'ignored'
ones.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7508-status.sh | 27 +++++++++++++++++++++++++++
 wt-status.c       |  4 ++--
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index e81759319f..2e9c6daf1a 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -814,6 +814,33 @@ test_expect_success 'status -s without relative paths' '
 
 '
 
+cat >expect <<\EOF
+ M dir1/modified
+A  dir2/added
+A  "file with spaces"
+?? dir1/untracked
+?? dir2/modified
+?? dir2/untracked
+?? "file with spaces 2"
+?? untracked
+EOF
+
+test_expect_success 'status -s without relative paths' '
+	test_when_finished "git rm --cached \"file with spaces\"; rm -f file*" &&
+	>"file with spaces" &&
+	>"file with spaces 2" &&
+	>"expect with spaces" &&
+	git add "file with spaces" &&
+
+	git status -s >output &&
+	test_cmp expect output &&
+
+	git status -s --ignored >output &&
+	grep "^!! \"expect with spaces\"$" output &&
+	grep -v "^!! " output >output-wo-ignored &&
+	test_cmp expect output-wo-ignored
+'
+
 test_expect_success 'dry-run of partial commit excluding new file in index' '
 	cat >expect <<EOF &&
 On branch master
diff --git a/wt-status.c b/wt-status.c
index adbf6958bd..7139623025 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1848,7 +1848,7 @@ static void wt_shortstatus_unmerged(struct string_list_item *it,
 	} else {
 		struct strbuf onebuf = STRBUF_INIT;
 		const char *one;
-		one = quote_path(it->string, s->prefix, &onebuf, 0);
+		one = quote_path(it->string, s->prefix, &onebuf, QUOTE_PATH_QUOTE_SP);
 		printf(" %s\n", one);
 		strbuf_release(&onebuf);
 	}
@@ -1896,7 +1896,7 @@ static void wt_shortstatus_other(struct string_list_item *it,
 	} else {
 		struct strbuf onebuf = STRBUF_INIT;
 		const char *one;
-		one = quote_path(it->string, s->prefix, &onebuf, 0);
+		one = quote_path(it->string, s->prefix, &onebuf, QUOTE_PATH_QUOTE_SP);
 		color_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), "%s", sign);
 		printf(" %s\n", one);
 		strbuf_release(&onebuf);
-- 
2.28.0-603-ga98dad7d4d

Re: [PATCH v2 5/7] wt-status: consistently quote paths in "status --short" output

[Jeff King]

On Thu, Sep 10, 2020 at 10:01:57AM -0700, Junio C Hamano wrote:

> Tracked paths with SP in them were cquoted in "git status --short"
> output, but untracked, ignored, and unmerged paths weren't.

By the way, the long status output will quote paths as appropriate, but
_not_ apply this "we should quote spaces" rule. I'm not sure if that
came up in earlier discussion or not. Certainly we're free to do
whatever looks nicest to humans there, so maybe we prefer to avoid
quoting as much as possible.

-Peff

Re: [PATCH v2 5/7] wt-status: consistently quote paths in "status --short" output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Sep 10, 2020 at 10:01:57AM -0700, Junio C Hamano wrote:
>
>> Tracked paths with SP in them were cquoted in "git status --short"
>> output, but untracked, ignored, and unmerged paths weren't.
>
> By the way, the long status output will quote paths as appropriate, but
> _not_ apply this "we should quote spaces" rule. I'm not sure if that
> came up in earlier discussion or not. Certainly we're free to do
> whatever looks nicest to humans there, so maybe we prefer to avoid
> quoting as much as possible.

Yup, the long output format is strictly for human consumption and
one fewer case to enclose the path in a dq-pair is good ;-).

[PATCH v2 6/7] quote: rename misnamed sq_lookup[] to cq_lookup[]

[Junio C Hamano]

This table is used to see if each byte needs quoting when responding
to a request to C-quote the string, not quoting with single-quote in
the shell style.  Similarly, sq_must_quote() is fed each byte from
the string being C-quoted.

No behaviour change intended.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 quote.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/quote.c b/quote.c
index b8107cd403..9a6e0e7dea 100644
--- a/quote.c
+++ b/quote.c
@@ -210,7 +210,7 @@ int sq_dequote_to_strvec(char *arg, struct strvec *array)
  */
 #define X8(x)   x, x, x, x, x, x, x, x
 #define X16(x)  X8(x), X8(x)
-static signed char const sq_lookup[256] = {
+static signed char const cq_lookup[256] = {
 	/*           0    1    2    3    4    5    6    7 */
 	/* 0x00 */   1,   1,   1,   1,   1,   1,   1, 'a',
 	/* 0x08 */ 'b', 't', 'n', 'v', 'f', 'r',   1,   1,
@@ -223,9 +223,9 @@ static signed char const sq_lookup[256] = {
 	/* 0x80 */ /* set to 0 */
 };
 
-static inline int sq_must_quote(char c)
+static inline int cq_must_quote(char c)
 {
-	return sq_lookup[(unsigned char)c] + quote_path_fully > 0;
+	return cq_lookup[(unsigned char)c] + quote_path_fully > 0;
 }
 
 /* returns the longest prefix not needing a quote up to maxlen if positive.
@@ -235,9 +235,9 @@ static size_t next_quote_pos(const char *s, ssize_t maxlen)
 {
 	size_t len;
 	if (maxlen < 0) {
-		for (len = 0; !sq_must_quote(s[len]); len++);
+		for (len = 0; !cq_must_quote(s[len]); len++);
 	} else {
-		for (len = 0; len < maxlen && !sq_must_quote(s[len]); len++);
+		for (len = 0; len < maxlen && !cq_must_quote(s[len]); len++);
 	}
 	return len;
 }
@@ -291,8 +291,8 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
 		ch = (unsigned char)*p++;
 		if (maxlen >= 0)
 			maxlen -= len + 1;
-		if (sq_lookup[ch] >= ' ') {
-			EMIT(sq_lookup[ch]);
+		if (cq_lookup[ch] >= ' ') {
+			EMIT(cq_lookup[ch]);
 		} else {
 			EMIT(((ch >> 6) & 03) + '0');
 			EMIT(((ch >> 3) & 07) + '0');
-- 
2.28.0-603-ga98dad7d4d

[PATCH v2 7/7] quote: turn 'nodq' parameter into a set of flags

[Junio C Hamano]

quote_c_style() and its friend quote_two_c_style() both take an
optional "please omit the double quotes around the quoted body"
parameter.  Turn it into a flag word, assign one bit out of it,
and call it CQUOTE_NODQ bit.

No behaviour change intended.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c  |  8 ++++----
 quote.c | 18 +++++++++++-------
 quote.h |  7 +++++--
 3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/diff.c b/diff.c
index 0299a73079..e7d6e60b23 100644
--- a/diff.c
+++ b/diff.c
@@ -482,14 +482,14 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 
 static char *quote_two(const char *one, const char *two)
 {
-	int need_one = quote_c_style(one, NULL, NULL, 1);
-	int need_two = quote_c_style(two, NULL, NULL, 1);
+	int need_one = quote_c_style(one, NULL, NULL, CQUOTE_NODQ);
+	int need_two = quote_c_style(two, NULL, NULL, CQUOTE_NODQ);
 	struct strbuf res = STRBUF_INIT;
 
 	if (need_one + need_two) {
 		strbuf_addch(&res, '"');
-		quote_c_style(one, &res, NULL, 1);
-		quote_c_style(two, &res, NULL, 1);
+		quote_c_style(one, &res, NULL, CQUOTE_NODQ);
+		quote_c_style(two, &res, NULL, CQUOTE_NODQ);
 		strbuf_addch(&res, '"');
 	} else {
 		strbuf_addstr(&res, one);
diff --git a/quote.c b/quote.c
index 9a6e0e7dea..69f4ca45da 100644
--- a/quote.c
+++ b/quote.c
@@ -256,7 +256,7 @@ static size_t next_quote_pos(const char *s, ssize_t maxlen)
  *     Return value is the same as in (1).
  */
 static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
-				    struct strbuf *sb, FILE *fp, int no_dq)
+				    struct strbuf *sb, FILE *fp, unsigned flags)
 {
 #undef EMIT
 #define EMIT(c)                                 \
@@ -272,6 +272,7 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
 		count += (l);                           \
 	} while (0)
 
+	int no_dq = !!(flags & CQUOTE_NODQ);
 	size_t len, count = 0;
 	const char *p = name;
 
@@ -309,19 +310,21 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
 	return count;
 }
 
-size_t quote_c_style(const char *name, struct strbuf *sb, FILE *fp, int nodq)
+size_t quote_c_style(const char *name, struct strbuf *sb, FILE *fp, unsigned flags)
 {
-	return quote_c_style_counted(name, -1, sb, fp, nodq);
+	return quote_c_style_counted(name, -1, sb, fp, flags);
 }
 
-void quote_two_c_style(struct strbuf *sb, const char *prefix, const char *path, int nodq)
+void quote_two_c_style(struct strbuf *sb, const char *prefix, const char *path,
+		       unsigned flags)
 {
+	int nodq = !!(flags & CQUOTE_NODQ);
 	if (quote_c_style(prefix, NULL, NULL, 0) ||
 	    quote_c_style(path, NULL, NULL, 0)) {
 		if (!nodq)
 			strbuf_addch(sb, '"');
-		quote_c_style(prefix, sb, NULL, 1);
-		quote_c_style(path, sb, NULL, 1);
+		quote_c_style(prefix, sb, NULL, CQUOTE_NODQ);
+		quote_c_style(path, sb, NULL, CQUOTE_NODQ);
 		if (!nodq)
 			strbuf_addch(sb, '"');
 	} else {
@@ -367,7 +370,8 @@ char *quote_path(const char *in, const char *prefix, struct strbuf *out, unsigne
 	 */
 	if (force_dq)
 		strbuf_addch(out, '"');
-	quote_c_style_counted(rel, strlen(rel), out, NULL, !!force_dq);
+	quote_c_style_counted(rel, strlen(rel), out, NULL,
+			      force_dq ? CQUOTE_NODQ : 0);
 	if (force_dq)
 		strbuf_addch(out, '"');
 	strbuf_release(&sb);
diff --git a/quote.h b/quote.h
index 1918d1e00e..4b72a583cf 100644
--- a/quote.h
+++ b/quote.h
@@ -64,8 +64,11 @@ struct strvec;
 int sq_dequote_to_strvec(char *arg, struct strvec *);
 
 int unquote_c_style(struct strbuf *, const char *quoted, const char **endp);
-size_t quote_c_style(const char *name, struct strbuf *, FILE *, int no_dq);
-void quote_two_c_style(struct strbuf *, const char *, const char *, int);
+
+/* Bits in the flags parameter to quote_c_style() */
+#define CQUOTE_NODQ 01
+size_t quote_c_style(const char *name, struct strbuf *, FILE *, unsigned);
+void quote_two_c_style(struct strbuf *, const char *, const char *, unsigned);
 
 void write_name_quoted(const char *name, FILE *, int terminator);
 void write_name_quoted_relative(const char *name, const char *prefix,
-- 
2.28.0-603-ga98dad7d4d

Re: [PATCH v2 0/7] quote_path() clean-ups

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]

On 2020-09-10 at 17:01:52, Junio C Hamano wrote:
> Here is an update, after seeing Peff's review.
> 
> The overall structure of the series stays the same.
> 
>  * The second patch lost an incorrect use of 'flags' parameter down
>    to quote_c_style_counted() from quote_path().  In the function
>    declaration of quote_path(), the flags parameter is now named.
> 
>  * The third patch that moves the "optionally quote path with SP in
>    it" logic to quote_path() is essentialy unchanged.
> 
>  * But the fourth patch rewrites the implementation to avoid
>    shifting the output bytes with insertstr().
> 
>  * The fifth patch (formerly the fourth) that teachs "status --short"
>    to consistently quote hasn't changed, except that we test also
>    for the ignored paths.
> 
>  * The last two patches are unchanged.  They are optional clean-ups
>    that are not required.
> 
> We might want to add a bit more tests for:
> 
>  - how conflicted "funny" paths are shown.
> 
>  - how doubly funny paths (those that quote_c_style_counted() needs
>    to use backslash quoting *and* that contain SP) are shown.
> 
> but I'd say that is outside the scope of this round.  Seeing what is
> done in t3300, the latter test cannot be written portably as far as
> I can tell.

I took a look and this seems like a great improvement.  Thanks for
picking up where my patch left off.
-- 
brian m. carlson: Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]