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

Junio C Hamano· Sep 8, 2020, 01:13 UTC · re: Patrick Fong · lore

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

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.

Thanks.  

brian m. carlson· Sep 8, 2020, 01:17 UTC · re: Patrick Fong · lore

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

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.

Patch incoming once the testsuite finishes running on this commit.

-- 
brian m. carlson: Houston, Texas, US

Junio C Hamano· Sep 8, 2020, 01:30 UTC · re: brian m. carlson · lore

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

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

Thanks.

Junio C Hamano· Sep 8, 2020, 04:41 UTC · re: Junio C Hamano · lore

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

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.

Junio C Hamano· Sep 8, 2020, 17:39 UTC · re: Junio C Hamano · lore

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

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.

 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

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.

Martin

René Scharfe· Sep 8, 2020, 21:06 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Sep 9, 2020, 22:22 UTC · re: René Scharfe · lore

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

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.

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

René Scharfe· Sep 10, 2020, 14:23 UTC · re: Junio C Hamano · lore

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

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

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

Junio C Hamano· Sep 10, 2020, 15:28 UTC · re: René Scharfe · lore

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

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

brian m. carlson· Sep 8, 2020, 01:30 UTC · re: Patrick Fong · lore

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

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.

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

Junio C Hamano· Sep 8, 2020, 20:52 UTC · re: brian m. carlson · lore

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

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.

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

Junio C Hamano· Sep 8, 2020, 20:52 UTC · re: Junio C Hamano · lore

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

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

Jeff King· Sep 10, 2020, 12:21 UTC · re: Junio C Hamano · lore

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

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

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

-Peff

Junio C Hamano· Sep 10, 2020, 15:04 UTC · re: Jeff King · lore

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

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

Thanks.

Junio C Hamano· Sep 10, 2020, 15:17 UTC · re: Junio C Hamano · lore

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

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.

/* 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;
}

Jeff King· Sep 10, 2020, 20:26 UTC · re: Junio C Hamano · lore

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

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

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

  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

Junio C Hamano· Sep 8, 2020, 20:52 UTC · re: Junio C Hamano · lore

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

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

Jeff King· Sep 10, 2020, 12:35 UTC · re: Junio C Hamano · lore

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

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, '"');
> +	}

-Peff

Junio C Hamano· Sep 8, 2020, 20:52 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Sep 8, 2020, 20:52 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Sep 8, 2020, 20:52 UTC · re: Junio C Hamano · lore

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

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

Jeff King· Sep 10, 2020, 12:38 UTC · re: Junio C Hamano · lore

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

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.

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

-Peff

Junio C Hamano· Sep 8, 2020, 20:52 UTC · re: Junio C Hamano · lore

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

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

Chris Torek· Sep 8, 2020, 22:56 UTC · re: Junio C Hamano · lore

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

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.

Chris

Jeff King· Sep 10, 2020, 12:39 UTC · re: Junio C Hamano · lore

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

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.

-Peff

Junio C Hamano· Sep 10, 2020, 17:01 UTC · re: Junio C Hamano · lore

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

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.

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

Junio C Hamano· Sep 10, 2020, 17:01 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Sep 10, 2020, 17:01 UTC · re: Junio C Hamano · lore

[PATCH v2 4/7] quote_path: code clarification

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

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

Jeff King· Sep 10, 2020, 18:08 UTC · re: Junio C Hamano · lore

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

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.

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

-Peff

Junio C Hamano· Sep 10, 2020, 18:40 UTC · re: Jeff King · lore

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

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

Jeff King· Sep 10, 2020, 19:29 UTC · re: Junio C Hamano · lore

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

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

-Peff

Junio C Hamano· Sep 10, 2020, 17:01 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Sep 10, 2020, 17:01 UTC · re: Junio C Hamano · lore

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

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

Jeff King· Sep 10, 2020, 18:13 UTC · re: Junio C Hamano · lore

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

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.

-Peff

Junio C Hamano· Sep 10, 2020, 18:38 UTC · re: Jeff King · lore

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

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.

Junio C Hamano· Sep 10, 2020, 17:01 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Sep 10, 2020, 17:01 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Sep 10, 2020, 17:01 UTC · re: Junio C Hamano · lore

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

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

brian m. carlson· Sep 10, 2020, 23:03 UTC · re: Junio C Hamano · lore

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

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.

-- 
brian m. carlson: Houston, Texas, US