[v3,1/1] quote: handle numm and empty strings in sq_quote_buf_pretty

Commit Message

John Passaro via GitGitGadget Oct. 7, 2019, 4:17 p.m. UTC

From: Garima Singh <[e-mail]>

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 <[e-mail]>
Signed-off-by: Garima Singh <[e-mail]>
---
 quote.c          | 10 ++++++++++
 t/t0014-alias.sh |  7 +++++++
 2 files changed, 17 insertions(+)

Comments

Garima Singh Oct. 7, 2019, 5:08 p.m. UTC | #1

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

On 10/7/2019 12:17 PM, Garima Singh via GitGitGadget wrote:
> From: Garima Singh <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Garima Singh <[e-mail]>
> ---
>  quote.c          | 10 ++++++++++
>  t/t0014-alias.sh |  7 +++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/quote.c b/quote.c
> index 7f2aa6faa4..f31ebf6c43 100644
> --- a/quote.c
> +++ b/quote.c
> @@ -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);
> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
> index a070e645d7..ae316aa6fd 100755
> --- a/t/t0014-alias.sh
> +++ b/t/t0014-alias.sh
> @@ -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
>

Eric Sunshine Oct. 7, 2019, 5:27 p.m. UTC | #2

On Mon, Oct 7, 2019 at 12:17 PM Garima Singh via GitGitGadget
<[e-mail]> wrote:
> quote: handle numm and empty strings in sq_quote_buf_pretty

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

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

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.

> Reported by: Junio Hamano <[e-mail]>
> Signed-off-by: Garima Singh <[e-mail]>
> ---
> diff --git a/quote.c b/quote.c
> @@ -48,6 +48,16 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)
> +       /* 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");

The comment merely repeats what the code itself already says clearly,
thus adds no value and ought to be dropped.

Moreover, this entire check seems superfluous since the program will
crash anyhow as soon as 'src' is dereferenced (just below), thus the
programmer will find out soon enough about the error. I'd suggest
dropping this check entirely since it's not adding any value.

> +       /* Avoid dropping a zero-length token by adding '' */
> +       if (!*src) {
> +               strbuf_addstr(dst, "''");
> +               return;
> +       }

Ditto regarding dropping the useless comment which merely repeats what
the code itself already says clearly.

> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
> @@ -37,4 +37,11 @@ test_expect_success 'looping aliases - internal execution' '
> +test_expect_success 'run-command parses empty args properly, using sq_quote_buf_pretty' '

Is "parses" the correct word? Should it be "formats" or something?

Also, the bit about "using sq_quote_buf_pretty" lets an implementation
detail bleed unnecessarily into the test suite, and that detail could
become outdated at some point (say, if some function ever replaces
that one, for instance). It should be sufficient for the test title
merely to mention that it is checking that empty arguments are handled
properly. So, perhaps:

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

Garima Singh Oct. 7, 2019, 5:47 p.m. UTC | #3

On 10/7/2019 1:27 PM, Eric Sunshine wrote:
> On Mon, Oct 7, 2019 at 12:17 PM Garima Singh via GitGitGadget
> <[e-mail]> wrote:
>> quote: handle numm and empty strings in sq_quote_buf_pretty
> 
> What is "numm"?

Typo. Fixing in next update. 

> 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
> 
>> 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.
> 
> s/is they/if they/

Typo. Fixing in next update. 

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

Works for me. Thanks! 

>> Reported by: Junio Hamano <[e-mail]>
>> Signed-off-by: Garima Singh <[e-mail]>
>> ---
>> diff --git a/quote.c b/quote.c
>> @@ -48,6 +48,16 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)
>> +       /* 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");
> 
> The comment merely repeats what the code itself already says clearly,
> thus adds no value and ought to be dropped.
> 
> Moreover, this entire check seems superfluous since the program will
> crash anyhow as soon as 'src' is dereferenced (just below), thus the
> programmer will find out soon enough about the error. I'd suggest
> dropping this check entirely since it's not adding any value.
> 

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

>> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
>> @@ -37,4 +37,11 @@ test_expect_success 'looping aliases - internal execution' '
>> +test_expect_success 'run-command parses empty args properly, using sq_quote_buf_pretty' '
> 
> Is "parses" the correct word? Should it be "formats" or something?
> 
Sure. 

> Also, the bit about "using sq_quote_buf_pretty" lets an implementation
> detail bleed unnecessarily into the test suite, and that detail could
> become outdated at some point (say, if some function ever replaces
> that one, for instance). It should be sufficient for the test title
> merely to mention that it is checking that empty arguments are handled
> properly. So, perhaps:
> 
>     test_expect_success 'run-command formats empty args properly' '
> 

Sure.

Patch

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