| Message ID | 9d2685bdb2e193986bec8cad88795963977d41fe.1566329700.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | quote: handle null and empty strings in sq_quote_buf_pretty() | expand |
"Garima Singh via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Garima Singh <garima.singh@microsoft.com> > > 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/pull.298.git.gitgitgadget@gmail.com/T/#m9e33936067ec2066f675aa63133a2486efd415fd 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. > create mode 100644 t/helper/test-quote.c > create mode 100755 t/t0091-quote.sh 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.
"Garima Singh via GitGitGadget" <gitgitgadget@gmail.com> writes: > + /* > + * In case of null or empty tokens, add a '' to ensure we > + * don't inadvertently drop those tokens > + */ A good comment. > + if (!src || !*src) { I think a caller that passes src==NULL deserves a BUG, or just a normal segfault. The condition here should just be "if (!*src)" instead. > + strbuf_addstr(dst, "''"); > + return; > + } Otherwise, the fix itself is good. Thanks. > for (p = src; *p; p++) { > if (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) { > sq_quote_buf(dst, src);
Junio C Hamano <gitster@pobox.com> writes: > "Garima Singh via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> create mode 100644 t/helper/test-quote.c >> create mode 100755 t/t0091-quote.sh > > I do not appreciate these two new files only to test this corner > case. ... 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.
diff --git a/Makefile b/Makefile index f9255344ae..2d6a12db57 100644 --- a/Makefile +++ b/Makefile @@ -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 diff --git a/quote.c b/quote.c index 7f2aa6faa4..84f61380fc 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; + /* + * 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); diff --git a/t/helper/test-quote.c b/t/helper/test-quote.c new file mode 100644 index 0000000000..0266cc4fec --- /dev/null +++ b/t/helper/test-quote.c @@ -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; +} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index ce7e89028c..55ee1402dd 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -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 }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index f805bb39ae..8c0affe89c 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -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); diff --git a/t/t0091-quote.sh b/t/t0091-quote.sh new file mode 100755 index 0000000000..a5515973c7 --- /dev/null +++ b/t/t0091-quote.sh @@ -0,0 +1,58 @@ +#!/bin/sh + +test_description='Testing the sq_quote_buf_pretty method in quote.c' +. ./test-lib.sh + +test_expect_success 'test method without input string' ' + test_must_fail test-tool quote-buf-pretty +' + +test_expect_success 'test null string' ' + cat >expect <<-EOF && + '[\'\']' + EOF + test-tool quote-buf-pretty nullString >actual && + test_cmp expect actual +' + +test_expect_success 'test empty string' ' + cat >expect <<-EOF && + '[\'\']' + EOF + test-tool quote-buf-pretty "" >actual && + test_cmp expect actual +' + +test_expect_success 'string without any punctuation' ' + cat >expect <<-EOF && + [testString] + EOF + test-tool quote-buf-pretty testString >actual && + test_cmp expect actual +' + +test_expect_success 'string with punctuation that do not require special quotes' ' + cat >expect <<-EOF && + [test+String] + EOF + test-tool quote-buf-pretty test+String >actual && + test_cmp expect actual +' + +test_expect_success 'string with punctuation that requires special quotes' ' + cat >expect <<-EOF && + '[\'test~String\']' + EOF + test-tool quote-buf-pretty test~String >actual && + test_cmp expect actual +' + +test_expect_success 'string with punctuation that requires special quotes' ' + cat >expect <<-EOF && + '[\'test\'\\!\'String\']' + EOF + test-tool quote-buf-pretty test!String >actual && + test_cmp expect actual +' + +test_done