Skip to content

quote: handle null and empty strings in sq_quote_buf_pretty() #314

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

quote: handle null and empty strings in sq_quote_buf_pretty() #314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions quote.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)
static const char ok_punct[] = "+,-./:=@_^";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Garima Singh wrote (reply to this):

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
<[email protected]> wrote:
>
> 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]>
> Signed-off-by: Garima Singh <[email protected]>
> ---
>  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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Garima Singh <[email protected]> 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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Garima Singh wrote (reply to this):

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 <[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]>
> Signed-off-by: Garima Singh <[email protected]>
> ---
>  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
>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Mon, Oct 7, 2019 at 12:17 PM Garima Singh via GitGitGadget
<[email protected]> 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 <[email protected]>
> Signed-off-by: Garima Singh <[email protected]>
> ---
> 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
> +'

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Garima Singh wrote (reply to this):

On 10/7/2019 1:27 PM, Eric Sunshine wrote:
> On Mon, Oct 7, 2019 at 12:17 PM Garima Singh via GitGitGadget
> <[email protected]> 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 <[email protected]>
>> Signed-off-by: Garima Singh <[email protected]>
>> ---
>> 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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Garima Singh via GitGitGadget" <[email protected]> writes:

> 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]>
> Signed-off-by: Garima Singh <[email protected]>
> ---
>  quote.c          | 9 +++++++++
>  t/t0014-alias.sh | 7 +++++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/quote.c b/quote.c
> index 7f2aa6faa4..26f1848dde 100644
> --- a/quote.c
> +++ b/quote.c
> @@ -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");

Remove these two lines.

I do not want to see "if (!ptr) BUG("don't give a NULL pointer")"
sprinkled to every function that takes a pointer that must not be
NULL.  Any caller that violates the contract with the callee
deserves a segfault, so let's leave it at that.

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

Nice.

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

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);
Expand Down
7 changes: 7 additions & 0 deletions t/t0014-alias.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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