-
Notifications
You must be signed in to change notification settings - Fork 141
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.
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
Conversation
Welcome to GitGitGadgetHi @garimasi514, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that this Pull Request has a good description, as it will be used as cover letter. Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a PR comment of the form Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions. If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox ("raw") file corresponding to the mail you want to reply to from the Git mailing list. If you use GMail, you can upload that raw mbox file via: curl -g --user "<EMailAddress>:<Password>" --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt |
f995ddc
to
6392059
Compare
9a20289
to
ca4d4dd
Compare
ca4d4dd
to
9d2685b
Compare
/submit |
Submitted as [email protected] |
9d2685b
to
0f2152e
Compare
655c001
to
b9a6859
Compare
/submit |
Submitted as [email protected] |
quote.c
Outdated
if (!src) | ||
BUG("BUG can't append a NULL token to the buffer"); | ||
|
||
/* In case of empty tokens, add a '' to ensure they |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a multi-line comment, the style is
/*
* start words on line 2
* and not on the line with the slash star.
*/
@@ -48,6 +48,18 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src) | |||
static const char ok_punct[] = "+,-./:=@_^"; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Don't include braces for single-line statements.
Very confusingly, Git no longer insists on the rule to skip braces for single-liners. I don't like them, personally, but you _can_, now. I'd much rather prefer a coding style that can be formatted by a tool, and does not require brain cycles... It is what it is.
|
b9a6859
to
399fe02
Compare
/submit |
Submitted as [email protected] WARNING: garimasi514 has no public email address set on GitHub |
@@ -48,6 +48,16 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src) | |||
static const char ok_punct[] = "+,-./:=@_^"; |
There was a problem hiding this comment.
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
>
@@ -48,6 +48,16 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src) | |||
static const char ok_punct[] = "+,-./:=@_^"; |
There was a problem hiding this comment.
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
> +'
There was a problem hiding this comment.
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.
b89f6a4
to
a6a0217
Compare
/submit |
Submitted as [email protected] WARNING: garimasi514 has no public email address set on GitHub |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
@@ -48,6 +48,15 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src) | |||
static const char ok_punct[] = "+,-./:=@_^"; |
There was a problem hiding this comment.
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
a6a0217
to
b33a402
Compare
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]>
b33a402
to
412626c
Compare
/submit |
Submitted as [email protected] WARNING: garimasi514 has no public email address set on GitHub |
This branch is now known as |
This patch series was integrated into pu via git@6dee36b. |
This patch series was integrated into pu via git@df0b039. |
This patch series was integrated into pu via git@85ba7b7. |
This patch series was integrated into next via git@2f7c006. |
This patch series was integrated into pu via git@7745887. |
This patch series was integrated into next via git@7745887. |
This patch series was integrated into master via git@7745887. |
Closed via 7745887. |
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] in
https://public-inbox.org/git/[email protected]/T/#m9e33936067ec2066f675aa63133a2486efd415fd
Cc: [email protected], [email protected], [email protected]