[v2,1/1] quote: handle null and empty strings in sq_quote_buf_pretty()
diff mbox series

Message ID b9a68598d79724849995283e6967f1c52843c048.1566830682.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • quote: handle null and empty strings in sq_quote_buf_pretty()
Related show

Commit Message

Johannes Schindelin via GitGitGadget Aug. 26, 2019, 2:44 p.m. UTC
From: Garima Singh <garima.singh@microsoft.com>

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 <gitster@pobox.com>
Signed-off-by: Garima Singh <garima.singh@microsoft.com>
---
 quote.c          | 12 ++++++++++++
 t/t0014-alias.sh |  8 ++++++++
 2 files changed, 20 insertions(+)

Comments

Garima Singh Aug. 26, 2019, 3:24 p.m. UTC | #1
Thanks for the review Junio! I really appreciate it and look forward
to hear what you think of the updated patch.

Cheers!
Garima Singh


On Mon, Aug 26, 2019 at 10:44 AM Garima Singh via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Garima Singh <garima.singh@microsoft.com>
>
> 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 <gitster@pobox.com>
> Signed-off-by: Garima Singh <garima.singh@microsoft.com>
> ---
>  quote.c          | 12 ++++++++++++
>  t/t0014-alias.sh |  8 ++++++++
>  2 files changed, 20 insertions(+)
>
> diff --git a/quote.c b/quote.c
> index 7f2aa6faa4..6d0f8a22a9 100644
> --- a/quote.c
> +++ b/quote.c
> @@ -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);
> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
> index a070e645d7..9c176c7cbb 100755
> --- a/t/t0014-alias.sh
> +++ b/t/t0014-alias.sh
> @@ -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
Junio C Hamano Aug. 26, 2019, 4:20 p.m. UTC | #2
Garima Singh <garimasigit@gmail.com> writes:

>> diff --git a/quote.c b/quote.c
>> index 7f2aa6faa4..6d0f8a22a9 100644
>> --- a/quote.c
>> +++ b/quote.c
>> @@ -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");

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.

>> +       /* In case of empty tokens, add a '' to ensure they
>> +        * don't get inadvertently dropped.
>> +        */

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?

>> +       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);
>> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
>> index a070e645d7..9c176c7cbb 100755
>> --- a/t/t0014-alias.sh
>> +++ b/t/t0014-alias.sh
>> @@ -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
>> +'

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

>> +
>>  test_done
>> --
>> gitgitgadget

Patch
diff mbox series

diff --git a/quote.c b/quote.c
index 7f2aa6faa4..6d0f8a22a9 100644
--- a/quote.c
+++ b/quote.c
@@ -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);
diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
index a070e645d7..9c176c7cbb 100755
--- a/t/t0014-alias.sh
+++ b/t/t0014-alias.sh
@@ -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