quote: handle null and empty strings in sq_quote_buf_pretty()

Hey,

In [1], Junio described a potential bug in sq_quote_buf_pretty() when the arg is a zero length string and how he believes the method should behave. This commit teaches sq_quote_buf_pretty to emit '' for null and empty strings.

Looking forward to your review. Cheers! Garima Singh

[1]
https://public-inbox.org/git/[email protected]/T/#m9e33936067ec2066f675aa63133a2486efd415fd

Garima Singh (1):
quote: handle null and empty strings in sq_quote_buf_pretty()

 Makefile              |  1 +
 quote.c               |  9 +++++++
 t/helper/test-quote.c | 28 +++++++++++++++++++++
 t/helper/test-tool.c  |  1 +
 t/helper/test-tool.h  |  1 +
 t/t0091-quote.sh      | 58 +++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 98 insertions(+)
 create mode 100644 t/helper/test-quote.c
 create mode 100755 t/t0091-quote.sh

base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-314%2Fgarimasi514%2FcoreGit-fixQuote-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-314/garimasi514/coreGit-fixQuote-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/314

gitgitgadget

From: Garima Singh <[email protected]>

In [1], Junio described a potential bug in sq_quote_buf_pretty() when the arg is a zero length string. It should emit quote-quote rather than nothing. This commit teaches sq_quote_buf_pretty to emit '' for null and empty strings.

[1] https://public-inbox.org/git/[email protected]/T/#m9e33936067ec2066f675aa63133a2486efd415fd

---
 Makefile              |  1 +
 quote.c               |  9 +++++++
 t/helper/test-quote.c | 28 +++++++++++++++++++++
 t/helper/test-tool.c  |  1 +
 t/helper/test-tool.h  |  1 +
 t/t0091-quote.sh      | 58 +++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 98 insertions(+)
 create mode 100644 t/helper/test-quote.c
 create mode 100755 t/t0091-quote.sh

index f9255344ae..2d6a12db57 100644
@@ -728,6 +728,7 @@ TEST_BUILTINS_OBJS += test-parse-options.o
 TEST_BUILTINS_OBJS += test-path-utils.o
 TEST_BUILTINS_OBJS += test-pkt-line.o
 TEST_BUILTINS_OBJS += test-prio-queue.o
+TEST_BUILTINS_OBJS += test-quote.o
 TEST_BUILTINS_OBJS += test-reach.o
 TEST_BUILTINS_OBJS += test-read-cache.o
 TEST_BUILTINS_OBJS += test-read-midx.o

index 7f2aa6faa4..84f61380fc 100644
@@ -48,6 +48,15 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)
 	static const char ok_punct[] = "+,-./:=@_^";
 	const char *p;
 
+	/*
+	 * In case of null or empty tokens, add a '' to ensure we 
+	 * don't inadvertently drop those tokens
+	 */
+	if (!src || !*src) {
+		strbuf_addstr(dst, "''");
+		return;
+	}
+
 	for (p = src; *p; p++) {
 		if (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {
 			sq_quote_buf(dst, src);

new file mode 100644
index 0000000000..0266cc4fec
@@ -0,0 +1,28 @@
+#include "test-tool.h"
+#include "quote.h"
+#include "strbuf.h"
+#include "string.h"
+
+int cmd__quote_buf_pretty(int argc, const char **argv)
+{
+	struct strbuf buf_payload = STRBUF_INIT;
+
+	if (!argv[1]) {
+		strbuf_release(&buf_payload);
+		die("missing input string");
+	}
+
+	if (!strcmp(argv[1], "nullString"))
+		sq_quote_buf_pretty(&buf_payload, NULL);
+
+	else if (!*argv[1])
+		sq_quote_buf_pretty(&buf_payload, "");
+
+	else
+		sq_quote_buf_pretty(&buf_payload, argv[1]);
+	
+	/* Wrap the results in [] to make the test script more readable */
+	printf("[%s]\n", buf_payload.buf);
+	strbuf_release(&buf_payload);
+	return 0;
+}

index ce7e89028c..55ee1402dd 100644
@@ -56,6 +56,7 @@ static struct test_cmd cmds[] = {
 	{ "sha1-array", cmd__sha1_array },
 	{ "sha256", cmd__sha256 },
 	{ "sigchain", cmd__sigchain },
+	{ "quote-buf-pretty", cmd__quote_buf_pretty },
 	{ "strcmp-offset", cmd__strcmp_offset },
 	{ "string-list", cmd__string_list },
 	{ "submodule-config", cmd__submodule_config },

index f805bb39ae..8c0affe89c 100644
@@ -46,6 +46,7 @@ int cmd__sha1(int argc, const char **argv);
 int cmd__sha1_array(int argc, const char **argv);
 int cmd__sha256(int argc, const char **argv);
 int cmd__sigchain(int argc, const char **argv);
+int cmd__quote_buf_pretty(int argc, const char **argv);
 int cmd__strcmp_offset(int argc, const char **argv);
 int cmd__string_list(int argc, const char **argv);
 int cmd__submodule_config(int argc, const char **argv);

gitgitgadget

It would be more helpful to omit "Junio described a bug" and say what the bug is. As written, people still need to go back the list archive to read what I said in order to understand what bug was noticed by me, but you can save their time by describing the bug directly in the log message. For example:

The sq_quote_buf_pretty() function does not emit anything when the incoming string is empty, but the function is to accumulate command line arguments, properly quoted as necessary, and the right way to add an argument that is an empty string is to show it quoted, i.e. ''.

or something like that. The credit to discoverer, if you must, can be given with

    Reported-by: ...

before your sign-off, but I do not think it is worth the trouble this time.

I do not appreciate these two new files only to test this corner case. That feels overly inefficient and unwieldy. It also hides the potential impact of the bug from readers to run *only* a unit test by using the function directly from an invented, non-real-world caller that is a program in t/helper/. It sometimes cannot be helped as some codepath is harder to trigger from the actual codepath in Git that matters in the real-world and is OK to resort to t/helper/ program, but in this particular case, with a little effort, we can find a codepath that can be used to feed an empty string to the function quite easily.

Here is what I did for example.

 $ git grep sq_quote_buf_pretty

tells me that sq_quote_quote_argv_pretty() calls it.

 $ git grep sq_quote_argv_pretty

then tells me that trace_run_command() makes a call to it. This is perfect, as we can have "git" run a command with arbitrary command line args and have trace print what it did.

So...

 $ GIT_TRACE=1 git -c "alias.foo=frotz foo '' bar" foo
 13:19:51.999614 git.c:703               trace: exec: git-foo
 13:19:51.999695 run-command.c:663       trace: run_command: git-foo
 13:19:51.999963 git.c:384               trace: alias expansion: foo => frotz foo  bar
 13:19:52.000327 git.c:703               trace: exec: git-frotz foo  bar
 13:19:52.000348 run-command.c:663       trace: run_command: git-frotz foo  bar
 expansion of alias 'foo' failed; 'frotz' is not a git command

With the bug fixed,

 $ GIT_TRACE=1 ./git -c "alias.foo=frotz foo '' bar" foo
 13:22:16.777692 git.c:670               trace: exec: git-foo
 13:22:16.777806 run-command.c:643       trace: run_command: git-foo
 13:22:16.778084 git.c:366               trace: alias expansion: foo => frotz foo '' bar
 13:22:16.778315 git.c:670               trace: exec: git-frotz foo '' bar
 13:22:16.778329 run-command.c:643       trace: run_command: git-frotz foo '' bar
 expansion of alias 'foo' failed; 'frotz' is not a git command

we can see that the second arg to git-frotz is prettily shown.

To avoid misunderstanding, I am not against having unit tests when they are appropriate. What I am against is to have only unit tests, especially when the effect of a bug (and its fix) can be tested with externally observable behaviour. The latter gives us a better sense of the real-world impact (e.g. if run_command would spawn the given command via shell using the 'sh -c "... stringified command and its arguments ..."' idiom, it may be done with the function we fixed here, which would mean that the user cannot pass '' as an argument to that codepath), while a unit test gives readers "ok, the function behaves that way now" alone, without answering "then what? What difference does this fix make to my use of Git as a whole?".

In any case, thanks for an attempt to fix.

Hey,

The sq_quote_buf_pretty() function does not emit anything when the incoming string is empty, but the function is to accumulate command line arguments, properly quoted as necessary, and the right way to add an argument that is an empty string is to show it quoted, i.e. ''.

Looking forward to your review. Cheers! Garima Singh

Reported by: Junio Hamano [email protected] [[email protected]] in
https://public-inbox.org/git/[email protected]/T/#m9e33936067ec2066f675aa63133a2486efd415fd

Garima Singh (1):
quote: handle null and empty strings in sq_quote_buf_pretty()

 quote.c          | 12 ++++++++++++
 t/t0014-alias.sh |  8 ++++++++
 2 files changed, 20 insertions(+)

base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-314%2Fgarimasi514%2FcoreGit-fixQuote-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-314/garimasi514/coreGit-fixQuote-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/314

Range-diff vs v1:

 1:  9d2685bdb2 < -:  ---------- quote: handle null and empty strings in sq_quote_buf_pretty()
 -:  ---------- > 1:  b9a68598d7 quote: handle null and empty strings in sq_quote_buf_pretty()

gitgitgadget

From: Garima Singh <[email protected]>

The sq_quote_buf_pretty() function does not emit anything when the incoming string is empty, but the function is to accumulate command line arguments, properly quoted as necessary, and the right way to add an argument that is an empty string is to show it quoted, i.e. ''. We warn the caller with the BUG macro if they pass in a NULL.

Reported by: Junio Hamano <[email protected]>

---
 quote.c          | 12 ++++++++++++
 t/t0014-alias.sh |  8 ++++++++
 2 files changed, 20 insertions(+)

index 7f2aa6faa4..6d0f8a22a9 100644
@@ -48,6 +48,18 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)
 	static const char ok_punct[] = "+,-./:=@_^";
 	const char *p;
 
+	/* In case of null tokens, warn the user of the BUG in their call. */
+	if (!src) 
+		BUG("BUG can't append a NULL token to the buffer");
+	
+	/* In case of empty tokens, add a '' to ensure they 
+	 * don't get inadvertently dropped. 
+	 */
+	if (!*src) {
+		strbuf_addstr(dst, "''");
+		return;
+	}
+
 	for (p = src; *p; p++) {
 		if (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {
 			sq_quote_buf(dst, src);

index a070e645d7..9c176c7cbb 100755
@@ -37,4 +37,12 @@ test_expect_success 'looping aliases - internal execution' '
 #	test_i18ngrep "^fatal: alias loop detected: expansion of" output
 #'
 
+test_expect_success 'run-command parses empty args properly, using sq_quote_buf_pretty' '
+	cat >expect <<-EOF &&
+	fatal: cannot change to '\''alias.foo=frotz foo '\'''\'' bar'\'': No such file or directory
+	EOF
+	test_expect_code 128 git -C "alias.foo=frotz foo '\'''\'' bar" foo 2>actual &&
+	test_cmp expect actual
+'
+
 test_done

gitgitgadget

Thanks for the review Junio! I really appreciate it and look forward to hear what you think of the updated patch.

Cheers!
Garima Singh

I thought that the BUG() macro already says "BUG" upfront, no?

Dereferencing to see if we have an empty string below will immediately give us segfault, so I would omit this check if I were writing this code, though.

Our multi-line comments have the opening slash-asterisk and the closing asterisk-slash on their own lines.

But more importantly, "In case of empty tokens, add a ''" in this comment has zero information contents---you can read that from the code. Why we do that is what we cannot express in the code, and deserves a comment.

	/* avoid losing a zero-length string by giving nothing */

or something like that, perhaps?

I think it was my mistake, but we do not ahe to use "alias" for something like this, perhaps like:

# 'git frotz' will fail with "no such command", but we are # not interested in its exit status. We just want to see # how sq_quote_argv_pretty() shows arguments in the trace. GIT_TRACE=1 git frotz a "" b " " c 2>&1 |
sed -ne "/run_command:/s/.*trace: run_command: //p" >actual && echo "git-frotz a '' b ' ' c" >expect &&
test_cmp expect actual

Hey,

The sq_quote_buf_pretty() function does not emit anything when the incoming string is empty, but the function is to accumulate command line arguments, properly quoted as necessary, and the right way to add an argument that is an empty string is to show it quoted, i.e. ''.

Looking forward to your review. Cheers! Garima Singh

Reported by: Junio Hamano [email protected] [[email protected]] in
https://public-inbox.org/git/[email protected]/T/#m9e33936067ec2066f675aa63133a2486efd415fd

Garima Singh (1):
quote: handle numm and empty strings in sq_quote_buf_pretty

 quote.c          | 10 ++++++++++
 t/t0014-alias.sh |  7 +++++++
 2 files changed, 17 insertions(+)

base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-314%2Fgarimasi514%2FcoreGit-fixQuote-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-314/garimasi514/coreGit-fixQuote-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/314

Range-diff vs v2:

1: b9a68598d7 ! 1: 399fe02cb1 quote: handle null and empty strings in sq_quote_buf_pretty()
@@ -1,13 +1,13 @@
Author: Garima Singh <[email protected]>

- quote: handle null and empty strings in sq_quote_buf_pretty()
+ quote: handle numm and empty strings in sq_quote_buf_pretty

- The sq_quote_buf_pretty() function does not emit anything when
- the incoming string is empty, but the function is to accumulate
- command line arguments, properly quoted as necessary, and the
- right way to add an argument that is an empty string is to show
- it quoted, i.e. ''. We warn the caller with the BUG macro if they
- pass in a NULL.
+ The sq_quote_buf_pretty() function does not emit anything
+ when the incoming string is empty, but the function is to
+ accumulate command line arguments, properly quoted as
+ necessary, and the right way to add an argument that is an
+ empty string is to show it quoted, i.e. ''. We warn the caller
+ with the BUG macro is they pass in a NULL.

Reported by: Junio Hamano <[email protected]>

@@ -21,11 +21,9 @@

+ /* In case of null tokens, warn the user of the BUG in their call. */
+ if (!src)
-+ BUG("BUG can't append a NULL token to the buffer");
++ BUG("Cannot append a NULL token to the buffer");
+
-+ /* In case of empty tokens, add a '' to ensure they
-+ * don't get inadvertently dropped.
-+ */
++ /* Avoid dropping a zero-length token by adding '' */
+ if (!*src) {
+ strbuf_addstr(dst, "''");
+ return;
@@ -43,11 +41,10 @@
#'

+test_expect_success 'run-command parses empty args properly, using sq_quote_buf_pretty' '
-+ cat >expect <<-EOF &&
-+ fatal: cannot change to '\''alias.foo=frotz foo '\'''\'' bar'\'': No such file or directory
-+ EOF
-+ test_expect_code 128 git -C "alias.foo=frotz foo '\'''\'' bar" foo 2>actual &&
-+ test_cmp expect actual
++ GIT_TRACE=1 git frotz a "" b " " c 2>&1 |
++ sed -ne "/run_command:/s/.*trace: run_command: //p" >actual &&
++ echo "git-frotz a '\'''\'' b '\'' '\'' c" >expect &&
++ test_cmp expect actual
+'
+
test_done

gitgitgadget

From: Garima Singh <[email protected]>

The sq_quote_buf_pretty() function does not emit anything when the incoming string is empty, but the function is to accumulate command line arguments, properly quoted as necessary, and the right way to add an argument that is an empty string is to show it quoted, i.e. ''. We warn the caller with the BUG macro is they pass in a NULL.

Reported by: Junio Hamano <[email protected]>

---
 quote.c          | 10 ++++++++++
 t/t0014-alias.sh |  7 +++++++
 2 files changed, 17 insertions(+)

index 7f2aa6faa4..f31ebf6c43 100644
@@ -48,6 +48,16 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)
 	static const char ok_punct[] = "+,-./:=@_^";
 	const char *p;
 
+	/* In case of null tokens, warn the user of the BUG in their call. */
+	if (!src) 
+		BUG("Cannot append a NULL token to the buffer");
+	
+	/* Avoid dropping a zero-length token by adding '' */
+	if (!*src) {
+		strbuf_addstr(dst, "''");
+		return;
+	}
+
 	for (p = src; *p; p++) {
 		if (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {
 			sq_quote_buf(dst, src);

index a070e645d7..ae316aa6fd 100755
@@ -37,4 +37,11 @@ test_expect_success 'looping aliases - internal execution' '
 #	test_i18ngrep "^fatal: alias loop detected: expansion of" output
 #'
 
+test_expect_success 'run-command parses empty args properly, using sq_quote_buf_pretty' '
+    GIT_TRACE=1 git frotz a "" b " " c 2>&1 |
+    sed -ne "/run_command:/s/.*trace: run_command: //p" >actual &&
+    echo "git-frotz a '\'''\'' b '\'' '\'' c" >expect &&
+    test_cmp expect actual
+'
+
 test_done

gitgitgadget

I just noticed the typo in the commit message in my latest update. Sorry about that.
Junio, would you be willing to fix it up whenever you queue the patch? Or would you like me to send another update.

Thanks
Garima G Singh

What is "numm"?

What does it mean to "handle" these things? A possible rewrite of the subject to explain the problem more precisely rather than using generalizations might be:

sq_quote_buf_pretty: don't drop empty arguments

s/is they/if they/

By including the final sentence in this paragraph, the reader is confused into thinking that warning the caller with BUG() is the overall purpose of this patch and is the "fix" for the stated problem. At minimum, the final sentence should be yanked out to its own paragraph or, better yet, dropped altogether since it's of little importance in the overall scheme of the patch.

As a reader of this commit message, I find it difficult to understand what problem it's trying to solve since the problem and solution and existing behavior are presented in a circuitous way which doesn't make any of them stand out clearly. Here's a possible rewrite:

sq_quote_buf_pretty: don't drop empty arguments

Empty arguments passed on a command-line should be represented by a zero-length quoted string, however, sq_quote_buf_pretty() incorrectly drops these arguments altogether. Fix this problem by ensuring that such arguments are emitted as '' instead.

Typo. Fixing in next update.

Typo. Fixing in next update.

Works for me. Thanks!

Fair enough. Removing the comment. Leaving the check. I would rather the caller of the function know what went wrong instead of a segfault.

Sure.

Sure.

Hey,

Empty arguments passed on the command line can be a represented by a '', however sq_quote_buf_pretty was incorrectly dropping these arguments altogether. Fix this problem by ensuring that such arguments are emitted as '' instead.

Looking forward to your review. Cheers! Garima Singh

Reported by: Junio Hamano [email protected] [[email protected]] in
https://public-inbox.org/git/[email protected]/T/#m9e33936067ec2066f675aa63133a2486efd415fd

Garima Singh (1):
sq_quote_buf_pretty: don't drop empty arguments

 quote.c          | 9 +++++++++
 t/t0014-alias.sh | 7 +++++++
 2 files changed, 16 insertions(+)

base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-314%2Fgarimasi514%2FcoreGit-fixQuote-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-314/garimasi514/coreGit-fixQuote-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/314

Range-diff vs v3:

1: 399fe02cb1 ! 1: a6a0217ce6 quote: handle numm and empty strings in sq_quote_buf_pretty
@@ -1,13 +1,11 @@
Author: Garima Singh <[email protected]>

- quote: handle numm and empty strings in sq_quote_buf_pretty
+ sq_quote_buf_pretty: don't drop empty arguments

- The sq_quote_buf_pretty() function does not emit anything
- when the incoming string is empty, but the function is to
- accumulate command line arguments, properly quoted as
- necessary, and the right way to add an argument that is an
- empty string is to show it quoted, i.e. ''. We warn the caller
- with the BUG macro is they pass in a NULL.
+ Empty arguments passed on the command line can be a represented by
+ a '', however sq_quote_buf_pretty was incorrectly dropping these
+ arguments altogether. Fix this problem by ensuring that such
+ arguments are emitted as '' instead.

Reported by: Junio Hamano <[email protected]>

@@ -19,11 +17,10 @@
static const char ok_punct[] = "+,-./:=@_^"; const char *p;

-+ /* In case of null tokens, warn the user of the BUG in their call. */
+ if (!src)
+ BUG("Cannot append a NULL token to the buffer");
+
-+ /* Avoid dropping a zero-length token by adding '' */
++ /* Avoid losing a zero-length string by adding '' */
+ if (!*src) {
+ strbuf_addstr(dst, "''");
+ return;
@@ -40,7 +37,7 @@
# test_i18ngrep "^fatal: alias loop detected: expansion of" output
#'

-+test_expect_success 'run-command parses empty args properly, using sq_quote_buf_pretty' ' ++test_expect_success 'run-command formats empty args properly' '
+ GIT_TRACE=1 git frotz a "" b " " c 2>&1 |
+ sed -ne "/run_command:/s/.*trace: run_command: //p" >actual &&
+ echo "git-frotz a '\'''\'' b '\'' '\'' c" >expect &&

gitgitgadget

From: Garima Singh <[email protected]>

Empty arguments passed on the command line can be a represented by a '', however sq_quote_buf_pretty was incorrectly dropping these arguments altogether. Fix this problem by ensuring that such arguments are emitted as '' instead.

Reported by: Junio Hamano <[email protected]>

---
 quote.c          | 9 +++++++++
 t/t0014-alias.sh | 7 +++++++
 2 files changed, 16 insertions(+)

index 7f2aa6faa4..26f1848dde 100644
@@ -48,6 +48,15 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)
 	static const char ok_punct[] = "+,-./:=@_^";
 	const char *p;
 
+	if (!src) 
+		BUG("Cannot append a NULL token to the buffer");
+	
+	/* Avoid losing a zero-length string by adding '' */ 
+	if (!*src) {
+		strbuf_addstr(dst, "''");
+		return;
+	}
+
 	for (p = src; *p; p++) {
 		if (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {
 			sq_quote_buf(dst, src);

index a070e645d7..2694c81afd 100755
@@ -37,4 +37,11 @@ test_expect_success 'looping aliases - internal execution' '
 #	test_i18ngrep "^fatal: alias loop detected: expansion of" output
 #'
 
+test_expect_success 'run-command formats empty args properly' '
+    GIT_TRACE=1 git frotz a "" b " " c 2>&1 |
+    sed -ne "/run_command:/s/.*trace: run_command: //p" >actual &&
+    echo "git-frotz a '\'''\'' b '\'' '\'' c" >expect &&
+    test_cmp expect actual
+'
+
 test_done

gitgitgadget

Hey,

Empty arguments passed on the command line can be a represented by a '', however sq_quote_buf_pretty was incorrectly dropping these arguments altogether. Fix this problem by ensuring that such arguments are emitted as '' instead.

Looking forward to your review. Cheers! Garima Singh

Reported by: Junio Hamano [email protected] [[email protected]] in
https://public-inbox.org/git/[email protected]/T/#m9e33936067ec2066f675aa63133a2486efd415fd

Garima Singh (1):
sq_quote_buf_pretty: don't drop empty arguments

 quote.c          | 6 ++++++
 t/t0014-alias.sh | 7 +++++++
 2 files changed, 13 insertions(+)

base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-314%2Fgarimasi514%2FcoreGit-fixQuote-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-314/garimasi514/coreGit-fixQuote-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/314

Range-diff vs v4:

1: a6a0217ce6 ! 1: 412626ccf9 sq_quote_buf_pretty: don't drop empty arguments

     @@ -17,9 +17,6 @@
       	static const char ok_punct[] = "+,-./:=@_^";
       	const char *p;
       
     -+	if (!src) 
     -+		BUG("Cannot append a NULL token to the buffer");
     -+	
      +	/* Avoid losing a zero-length string by adding '' */ 
      +	if (!*src) {
      +		strbuf_addstr(dst, "''");

gitgitgadget

From: Garima Singh <[email protected]>

Empty arguments passed on the command line can be a represented by a '', however sq_quote_buf_pretty was incorrectly dropping these arguments altogether. Fix this problem by ensuring that such arguments are emitted as '' instead.

Reported by: Junio Hamano <[email protected]>

---
 quote.c          | 6 ++++++
 t/t0014-alias.sh | 7 +++++++
 2 files changed, 13 insertions(+)

index 7f2aa6faa4..2d5d1c4360 100644
@@ -48,6 +48,12 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)
 	static const char ok_punct[] = "+,-./:=@_^";
 	const char *p;
 
+	/* Avoid losing a zero-length string by adding '' */ 
+	if (!*src) {
+		strbuf_addstr(dst, "''");
+		return;
+	}
+
 	for (p = src; *p; p++) {
 		if (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {
 			sq_quote_buf(dst, src);

index a070e645d7..2694c81afd 100755
@@ -37,4 +37,11 @@ test_expect_success 'looping aliases - internal execution' '
 #	test_i18ngrep "^fatal: alias loop detected: expansion of" output
 #'
 
+test_expect_success 'run-command formats empty args properly' '
+    GIT_TRACE=1 git frotz a "" b " " c 2>&1 |
+    sed -ne "/run_command:/s/.*trace: run_command: //p" >actual &&
+    echo "git-frotz a '\'''\'' b '\'' '\'' c" >expect &&
+    test_cmp expect actual
+'
+
 test_done

gitgitgadget