{"thread":{"id":"51696","subject":"[PATCH 0/1] quote: handle null and empty strings in sq_quote_buf_pretty()","startedAt":"2019-08-20T19:35:04Z","lastAt":"2019-10-08T16:40:43Z","messageCount":19,"participants":["Garima Singh via GitGitGadget","Junio C Hamano","Garima Singh","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"380846","messageId":"pull.314.git.gitgitgadget@gmail.com","threadId":"51696","inReplyTo":null,"subject":"[PATCH 0/1] quote: handle null and empty strings in sq_quote_buf_pretty()","fromName":"Garima Singh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-08-20T19:35:00Z","receivedAt":"2019-08-20T19:35:04Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"Hey,\n\nIn [1], Junio described a potential bug in sq_quote_buf_pretty() when the\narg is a zero length string and how he believes the method should behave.\nThis commit teaches sq_quote_buf_pretty to emit '' for null and empty\nstrings.\n\nLooking forward to your review. Cheers! Garima Singh\n\n[1] \nhttps://public-inbox.org/git/pull.298.git.gitgitgadget@gmail.com/T/#m9e33936067ec2066f675aa63133a2486efd415fd\n\nGarima Singh (1):\n quote: handle null and empty strings in sq_quote_buf_pretty()\n\n Makefile | 1 +\n quote.c | 9 +++++++\n t/helper/test-quote.c | 28 +++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t0091-quote.sh | 58 +++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 98 insertions(+)\n create mode 100644 t/helper/test-quote.c\n create mode 100755 t/t0091-quote.sh\n\n\nbase-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-314%2Fgarimasi514%2FcoreGit-fixQuote-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-314/garimasi514/coreGit-fixQuote-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/314\n-- \ngitgitgadget\n"},{"id":"380847","messageId":"9d2685bdb2e193986bec8cad88795963977d41fe.1566329700.git.gitgitgadget@gmail.com","threadId":"51696","inReplyTo":"pull.314.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] quote: handle null and empty strings in sq_quote_buf_pretty()","fromName":"Garima Singh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-08-20T19:35:01Z","receivedAt":"2019-08-20T19:35:07Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"From: Garima Singh \n\nIn [1], Junio described a potential bug in sq_quote_buf_pretty() when the arg\nis a zero length string. It should emit quote-quote rather than nothing.\nThis commit teaches sq_quote_buf_pretty to emit '' for null and empty strings.\n\n[1] https://public-inbox.org/git/pull.298.git.gitgitgadget@gmail.com/T/#m9e33936067ec2066f675aa63133a2486efd415fd\n\nSigned-off-by: Garima Singh \n---\n Makefile | 1 +\n quote.c | 9 +++++++\n t/helper/test-quote.c | 28 +++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t0091-quote.sh | 58 +++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 98 insertions(+)\n create mode 100644 t/helper/test-quote.c\n create mode 100755 t/t0091-quote.sh\n\ndiff --git a/Makefile b/Makefile\nindex f9255344ae..2d6a12db57 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -728,6 +728,7 @@ TEST_BUILTINS_OBJS += test-parse-options.o\n TEST_BUILTINS_OBJS += test-path-utils.o\n TEST_BUILTINS_OBJS += test-pkt-line.o\n TEST_BUILTINS_OBJS += test-prio-queue.o\n+TEST_BUILTINS_OBJS += test-quote.o\n TEST_BUILTINS_OBJS += test-reach.o\n TEST_BUILTINS_OBJS += test-read-cache.o\n TEST_BUILTINS_OBJS += test-read-midx.o\ndiff --git a/quote.c b/quote.c\nindex 7f2aa6faa4..84f61380fc 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -48,6 +48,15 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)\n \tstatic const char ok_punct[] = \"+,-./:=@_^\";\n \tconst char *p;\n \n+\t/*\n+\t * In case of null or empty tokens, add a '' to ensure we \n+\t * don't inadvertently drop those tokens\n+\t */\n+\tif (!src || !*src) {\n+\t\tstrbuf_addstr(dst, \"''\");\n+\t\treturn;\n+\t}\n+\n \tfor (p = src; *p; p++) {\n \t\tif (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {\n \t\t\tsq_quote_buf(dst, src);\ndiff --git a/t/helper/test-quote.c b/t/helper/test-quote.c\nnew file mode 100644\nindex 0000000000..0266cc4fec\n--- /dev/null\n+++ b/t/helper/test-quote.c\n@@ -0,0 +1,28 @@\n+#include \"test-tool.h\"\n+#include \"quote.h\"\n+#include \"strbuf.h\"\n+#include \"string.h\"\n+\n+int cmd__quote_buf_pretty(int argc, const char **argv)\n+{\n+\tstruct strbuf buf_payload = STRBUF_INIT;\n+\n+\tif (!argv[1]) {\n+\t\tstrbuf_release(&buf_payload);\n+\t\tdie(\"missing input string\");\n+\t}\n+\n+\tif (!strcmp(argv[1], \"nullString\"))\n+\t\tsq_quote_buf_pretty(&buf_payload, NULL);\n+\n+\telse if (!*argv[1])\n+\t\tsq_quote_buf_pretty(&buf_payload, \"\");\n+\n+\telse\n+\t\tsq_quote_buf_pretty(&buf_payload, argv[1]);\n+\t\n+\t/* Wrap the results in [] to make the test script more readable */\n+\tprintf(\"[%s]\\n\", buf_payload.buf);\n+\tstrbuf_release(&buf_payload);\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex ce7e89028c..55ee1402dd 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -56,6 +56,7 @@ static struct test_cmd cmds[] = {\n \t{ \"sha1-array\", cmd__sha1_array },\n \t{ \"sha256\", cmd__sha256 },\n \t{ \"sigchain\", cmd__sigchain },\n+\t{ \"quote-buf-pretty\", cmd__quote_buf_pretty },\n \t{ \"strcmp-offset\", cmd__strcmp_offset },\n \t{ \"string-list\", cmd__string_list },\n \t{ \"submodule-config\", cmd__submodule_config },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex f805bb39ae..8c0affe89c 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -46,6 +46,7 @@ int cmd__sha1(int argc, const char **argv);\n int cmd__sha1_array(int argc, const char **argv);\n int cmd__sha256(int argc, const char **argv);\n int cmd__sigchain(int argc, const char **argv);\n+int cmd__quote_buf_pretty(int argc, const char **argv);\n int cmd__strcmp_offset(int argc, const char **argv);\n int cmd__string_list(int argc, const char **argv);\n int cmd__submodule_config(int argc, const char **argv);\ndiff --git a/t/t0091-quote.sh b/t/t0091-quote.sh\nnew file mode 100755\nindex 0000000000..a5515973c7\n--- /dev/null\n+++ b/t/t0091-quote.sh\n@@ -0,0 +1,58 @@\n+#!/bin/sh\n+\n+test_description='Testing the sq_quote_buf_pretty method in quote.c'\n+. ./test-lib.sh\n+\n+test_expect_success 'test method without input string' '\n+\ttest_must_fail test-tool quote-buf-pretty\n+'\n+\n+test_expect_success 'test null string' '\n+\tcat >expect <<-EOF &&\n+\t'[\\'\\']'\n+\tEOF\n+\ttest-tool quote-buf-pretty nullString >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'test empty string' '\n+\tcat >expect <<-EOF &&\n+\t'[\\'\\']'\n+\tEOF\n+\ttest-tool quote-buf-pretty \"\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'string without any punctuation' '\n+\tcat >expect <<-EOF &&\n+\t[testString]\n+\tEOF\n+\ttest-tool quote-buf-pretty testString >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'string with punctuation that do not require special quotes' '\n+\tcat >expect <<-EOF &&\n+\t[test+String]\n+\tEOF\n+\ttest-tool quote-buf-pretty test+String >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'string with punctuation that requires special quotes' '\n+\tcat >expect <<-EOF &&\n+\t'[\\'test~String\\']'\n+\tEOF\n+\ttest-tool quote-buf-pretty test~String >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'string with punctuation that requires special quotes' '\n+\tcat >expect <<-EOF &&\n+\t'[\\'test\\'\\\\!\\'String\\']'\n+\tEOF\n+\ttest-tool quote-buf-pretty test!String >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \ngitgitgadget\n"},{"id":"380855","messageId":"xmqqtvabtwai.fsf@gitster-ct.c.googlers.com","threadId":"51696","inReplyTo":"9d2685bdb2e193986bec8cad88795963977d41fe.1566329700.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] quote: handle null and empty strings in sq_quote_buf_pretty()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-20T20:29:57Z","receivedAt":"2019-08-20T20:30:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Garima Singh via GitGitGadget\" writes:\n\n> From: Garima Singh \n>\n> In [1], Junio described a potential bug in sq_quote_buf_pretty() when the arg\n> is a zero length string. It should emit quote-quote rather than nothing.\n> This commit teaches sq_quote_buf_pretty to emit '' for null and empty strings.\n>\n> [1] https://public-inbox.org/git/pull.298.git.gitgitgadget@gmail.com/T/#m9e33936067ec2066f675aa63133a2486efd415fd\n\nIt would be more helpful to omit \"Junio described a bug\" and say\nwhat the bug is. As written, people still need to go back the list\narchive to read what I said in order to understand what bug was\nnoticed by me, but you can save their time by describing the bug\ndirectly in the log message. For example:\n\n The sq_quote_buf_pretty() function does not emit anything when\n the incoming string is empty, but the function is to accumulate\n command line arguments, properly quoted as necessary, and the\n right way to add an argument that is an empty string is to show\n it quoted, i.e. ''.\n\nor something like that. The credit to discoverer, if you must, can\nbe given with\n\n Reported-by: ...\n\nbefore your sign-off, but I do not think it is worth the trouble\nthis time.\n\n> create mode 100644 t/helper/test-quote.c\n> create mode 100755 t/t0091-quote.sh\n\nI do not appreciate these two new files only to test this corner\ncase. That feels overly inefficient and unwieldy. It also hides\nthe potential impact of the bug from readers to run *only* a unit\ntest by using the function directly from an invented, non-real-world\ncaller that is a program in t/helper/. It sometimes cannot be\nhelped as some codepath is harder to trigger from the actual\ncodepath in Git that matters in the real-world and is OK to resort\nto t/helper/ program, but in this particular case, with a little\neffort, we can find a codepath that can be used to feed an empty\nstring to the function quite easily.\n\nHere is what I did for example.\n\n $ git grep sq_quote_buf_pretty\n\ntells me that sq_quote_quote_argv_pretty() calls it.\n\n $ git grep sq_quote_argv_pretty\n\nthen tells me that trace_run_command() makes a call to it. This is\nperfect, as we can have \"git\" run a command with arbitrary command\nline args and have trace print what it did.\n\nSo...\n\n $ GIT_TRACE=1 git -c \"alias.foo=frotz foo '' bar\" foo\n 13:19:51.999614 git.c:703 trace: exec: git-foo\n 13:19:51.999695 run-command.c:663 trace: run_command: git-foo\n 13:19:51.999963 git.c:384 trace: alias expansion: foo => frotz foo bar\n 13:19:52.000327 git.c:703 trace: exec: git-frotz foo bar\n 13:19:52.000348 run-command.c:663 trace: run_command: git-frotz foo bar\n expansion of alias 'foo' failed; 'frotz' is not a git command\n\nWith the bug fixed, \n\n $ GIT_TRACE=1 ./git -c \"alias.foo=frotz foo '' bar\" foo\n 13:22:16.777692 git.c:670 trace: exec: git-foo\n 13:22:16.777806 run-command.c:643 trace: run_command: git-foo\n 13:22:16.778084 git.c:366 trace: alias expansion: foo => frotz foo '' bar\n 13:22:16.778315 git.c:670 trace: exec: git-frotz foo '' bar\n 13:22:16.778329 run-command.c:643 trace: run_command: git-frotz foo '' bar\n expansion of alias 'foo' failed; 'frotz' is not a git command\n\nwe can see that the second arg to git-frotz is prettily shown.\n"},{"id":"380856","messageId":"xmqqpnkztw6y.fsf@gitster-ct.c.googlers.com","threadId":"51696","inReplyTo":"9d2685bdb2e193986bec8cad88795963977d41fe.1566329700.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] quote: handle null and empty strings in sq_quote_buf_pretty()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-20T20:32:05Z","receivedAt":"2019-08-20T20:32:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Garima Singh via GitGitGadget\" writes:\n\n> +\t/*\n> +\t * In case of null or empty tokens, add a '' to ensure we \n> +\t * don't inadvertently drop those tokens\n> +\t */\n\nA good comment.\n\n> +\tif (!src || !*src) {\n\nI think a caller that passes src==NULL deserves a BUG, or just a\nnormal segfault. The condition here should just be \"if (!*src)\"\ninstead.\n\n> +\t\tstrbuf_addstr(dst, \"''\");\n> +\t\treturn;\n> +\t}\n\nOtherwise, the fix itself is good.\n\nThanks.\n\n\n> \tfor (p = src; *p; p++) {\n> \t\tif (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {\n> \t\t\tsq_quote_buf(dst, src);\n"},{"id":"380889","messageId":"xmqq7e76tufs.fsf@gitster-ct.c.googlers.com","threadId":"51696","inReplyTo":"xmqqtvabtwai.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] quote: handle null and empty strings in sq_quote_buf_pretty()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-21T15:22:15Z","receivedAt":"2019-08-21T15:22:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Garima Singh via GitGitGadget\" writes:\n>\n>> create mode 100644 t/helper/test-quote.c\n>> create mode 100755 t/t0091-quote.sh\n>\n> I do not appreciate these two new files only to test this corner\n> case. ...\n\nTo avoid misunderstanding, I am not against having unit tests when\nthey are appropriate. What I am against is to have only unit tests,\nespecially when the effect of a bug (and its fix) can be tested with\nexternally observable behaviour. The latter gives us a better sense\nof the real-world impact (e.g. if run_command would spawn the given\ncommand via shell using the 'sh -c \"... stringified command and its\narguments ...\"' idiom, it may be done with the function we fixed\nhere, which would mean that the user cannot pass '' as an argument\nto that codepath), while a unit test gives readers \"ok, the function\nbehaves that way now\" alone, without answering \"then what? What\ndifference does this fix make to my use of Git as a whole?\".\n\nIn any case, thanks for an attempt to fix.\n"},{"id":"381182","messageId":"pull.314.v2.git.gitgitgadget@gmail.com","threadId":"51696","inReplyTo":"pull.314.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] quote: handle null and empty strings in sq_quote_buf_pretty()","fromName":"Garima Singh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-08-26T14:44:43Z","receivedAt":"2019-08-26T14:44:46Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"Hey,\n\nThe sq_quote_buf_pretty() function does not emit anything when the incoming\nstring is empty, but the function is to accumulate command line arguments,\nproperly quoted as necessary, and the right way to add an argument that is\nan empty string is to show it quoted, i.e. ''.\n\nLooking forward to your review. Cheers! Garima Singh\n\nReported by: Junio Hamano gitster@pobox.com [gitster@pobox.com] in\nhttps://public-inbox.org/git/pull.298.git.gitgitgadget@gmail.com/T/#m9e33936067ec2066f675aa63133a2486efd415fd\n\nGarima Singh (1):\n quote: handle null and empty strings in sq_quote_buf_pretty()\n\n quote.c | 12 ++++++++++++\n t/t0014-alias.sh | 8 ++++++++\n 2 files changed, 20 insertions(+)\n\n\nbase-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-314%2Fgarimasi514%2FcoreGit-fixQuote-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-314/garimasi514/coreGit-fixQuote-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/314\n\nRange-diff vs v1:\n\n 1: 9d2685bdb2 < -: ---------- quote: handle null and empty strings in sq_quote_buf_pretty()\n -: ---------- > 1: b9a68598d7 quote: handle null and empty strings in sq_quote_buf_pretty()\n\n-- \ngitgitgadget\n"},{"id":"381183","messageId":"b9a68598d79724849995283e6967f1c52843c048.1566830682.git.gitgitgadget@gmail.com","threadId":"51696","inReplyTo":"pull.314.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] quote: handle null and empty strings in sq_quote_buf_pretty()","fromName":"Garima Singh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-08-26T14:44:44Z","receivedAt":"2019-08-26T14:44:48Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"From: Garima Singh \n\nThe sq_quote_buf_pretty() function does not emit anything when\nthe incoming string is empty, but the function is to accumulate\ncommand line arguments, properly quoted as necessary, and the\nright way to add an argument that is an empty string is to show\nit quoted, i.e. ''. We warn the caller with the BUG macro if they\npass in a NULL.\n\nReported by: Junio Hamano \nSigned-off-by: Garima Singh \n---\n quote.c | 12 ++++++++++++\n t/t0014-alias.sh | 8 ++++++++\n 2 files changed, 20 insertions(+)\n\ndiff --git a/quote.c b/quote.c\nindex 7f2aa6faa4..6d0f8a22a9 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -48,6 +48,18 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)\n \tstatic const char ok_punct[] = \"+,-./:=@_^\";\n \tconst char *p;\n \n+\t/* In case of null tokens, warn the user of the BUG in their call. */\n+\tif (!src) \n+\t\tBUG(\"BUG can't append a NULL token to the buffer\");\n+\t\n+\t/* In case of empty tokens, add a '' to ensure they \n+\t * don't get inadvertently dropped. \n+\t */\n+\tif (!*src) {\n+\t\tstrbuf_addstr(dst, \"''\");\n+\t\treturn;\n+\t}\n+\n \tfor (p = src; *p; p++) {\n \t\tif (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {\n \t\t\tsq_quote_buf(dst, src);\ndiff --git a/t/t0014-alias.sh b/t/t0014-alias.sh\nindex a070e645d7..9c176c7cbb 100755\n--- a/t/t0014-alias.sh\n+++ b/t/t0014-alias.sh\n@@ -37,4 +37,12 @@ test_expect_success 'looping aliases - internal execution' '\n #\ttest_i18ngrep \"^fatal: alias loop detected: expansion of\" output\n #'\n \n+test_expect_success 'run-command parses empty args properly, using sq_quote_buf_pretty' '\n+\tcat >expect <<-EOF &&\n+\tfatal: cannot change to '\\''alias.foo=frotz foo '\\'''\\'' bar'\\'': No such file or directory\n+\tEOF\n+\ttest_expect_code 128 git -C \"alias.foo=frotz foo '\\'''\\'' bar\" foo 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"381193","messageId":"CAN+QWEZU2FqkH1jWnv==owKLsMk8XNXJh6PpF6njvB6MmKt+Dw@mail.gmail.com","threadId":"51696","inReplyTo":"b9a68598d79724849995283e6967f1c52843c048.1566830682.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] quote: handle null and empty strings in sq_quote_buf_pretty()","fromName":"Garima Singh","fromEmail":"garimasigit@gmail.com","sentAt":"2019-08-26T15:24:45Z","receivedAt":"2019-08-26T15:24:56Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"Thanks for the review Junio! I really appreciate it and look forward\nto hear what you think of the updated patch.\n\nCheers!\nGarima Singh\n\n\nOn Mon, Aug 26, 2019 at 10:44 AM Garima Singh via GitGitGadget\n wrote:\n>\n> From: Garima Singh \n>\n> The sq_quote_buf_pretty() function does not emit anything when\n> the incoming string is empty, but the function is to accumulate\n> command line arguments, properly quoted as necessary, and the\n> right way to add an argument that is an empty string is to show\n> it quoted, i.e. ''. We warn the caller with the BUG macro if they\n> pass in a NULL.\n>\n> Reported by: Junio Hamano \n> Signed-off-by: Garima Singh \n> ---\n> quote.c | 12 ++++++++++++\n> t/t0014-alias.sh | 8 ++++++++\n> 2 files changed, 20 insertions(+)\n>\n> diff --git a/quote.c b/quote.c\n> index 7f2aa6faa4..6d0f8a22a9 100644\n> --- a/quote.c\n> +++ b/quote.c\n> @@ -48,6 +48,18 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)\n> static const char ok_punct[] = \"+,-./:=@_^\";\n> const char *p;\n>\n> + /* In case of null tokens, warn the user of the BUG in their call. */\n> + if (!src)\n> + BUG(\"BUG can't append a NULL token to the buffer\");\n> +\n> + /* In case of empty tokens, add a '' to ensure they\n> + * don't get inadvertently dropped.\n> + */\n> + if (!*src) {\n> + strbuf_addstr(dst, \"''\");\n> + return;\n> + }\n> +\n> for (p = src; *p; p++) {\n> if (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {\n> sq_quote_buf(dst, src);\n> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh\n> index a070e645d7..9c176c7cbb 100755\n> --- a/t/t0014-alias.sh\n> +++ b/t/t0014-alias.sh\n> @@ -37,4 +37,12 @@ test_expect_success 'looping aliases - internal execution' '\n> # test_i18ngrep \"^fatal: alias loop detected: expansion of\" output\n> #'\n>\n> +test_expect_success 'run-command parses empty args properly, using sq_quote_buf_pretty' '\n> + cat >expect <<-EOF &&\n> + fatal: cannot change to '\\''alias.foo=frotz foo '\\'''\\'' bar'\\'': No such file or directory\n> + EOF\n> + test_expect_code 128 git -C \"alias.foo=frotz foo '\\'''\\'' bar\" foo 2>actual &&\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n> --\n> gitgitgadget\n"},{"id":"381202","messageId":"xmqq1rx7kief.fsf@gitster-ct.c.googlers.com","threadId":"51696","inReplyTo":"CAN+QWEZU2FqkH1jWnv==owKLsMk8XNXJh6PpF6njvB6MmKt+Dw@mail.gmail.com","subject":"Re: [PATCH v2 1/1] quote: handle null and empty strings in sq_quote_buf_pretty()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-26T16:20:40Z","receivedAt":"2019-08-26T16:20:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Garima Singh writes:\n\n>> diff --git a/quote.c b/quote.c\n>> index 7f2aa6faa4..6d0f8a22a9 100644\n>> --- a/quote.c\n>> +++ b/quote.c\n>> @@ -48,6 +48,18 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)\n>> static const char ok_punct[] = \"+,-./:=@_^\";\n>> const char *p;\n>>\n>> + /* In case of null tokens, warn the user of the BUG in their call. */\n>> + if (!src)\n>> + BUG(\"BUG can't append a NULL token to the buffer\");\n\nI thought that the BUG() macro already says \"BUG\" upfront, no?\n\nDereferencing to see if we have an empty string below will\nimmediately give us segfault, so I would omit this check if I were\nwriting this code, though.\n\n>> + /* In case of empty tokens, add a '' to ensure they\n>> + * don't get inadvertently dropped.\n>> + */\n\nOur multi-line comments have the opening slash-asterisk and the\nclosing asterisk-slash on their own lines.\n\nBut more importantly, \"In case of empty tokens, add a ''\" in this\ncomment has zero information contents---you can read that from the\ncode. Why we do that is what we cannot express in the code, and\ndeserves a comment.\n\n\t/* avoid losing a zero-length string by giving nothing */\n\nor something like that, perhaps?\n\n>> + if (!*src) {\n>> + strbuf_addstr(dst, \"''\");\n>> + return;\n>> + }\n>> +\n>> for (p = src; *p; p++) {\n>> if (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {\n>> sq_quote_buf(dst, src);\n>> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh\n>> index a070e645d7..9c176c7cbb 100755\n>> --- a/t/t0014-alias.sh\n>> +++ b/t/t0014-alias.sh\n>> @@ -37,4 +37,12 @@ test_expect_success 'looping aliases - internal execution' '\n>> # test_i18ngrep \"^fatal: alias loop detected: expansion of\" output\n>> #'\n>>\n>> +test_expect_success 'run-command parses empty args properly, using sq_quote_buf_pretty' '\n>> + cat >expect <<-EOF &&\n>> + fatal: cannot change to '\\''alias.foo=frotz foo '\\'''\\'' bar'\\'': No such file or directory\n>> + EOF\n>> + test_expect_code 128 git -C \"alias.foo=frotz foo '\\'''\\'' bar\" foo 2>actual &&\n>> + test_cmp expect actual\n>> +'\n\nI think it was my mistake, but we do not ahe to use \"alias\" for\nsomething like this, perhaps like:\n\n # 'git frotz' will fail with \"no such command\", but we are\n # not interested in its exit status. We just want to see\n # how sq_quote_argv_pretty() shows arguments in the trace.\n GIT_TRACE=1 git frotz a \"\" b \" \" c 2>&1 |\n sed -ne \"/run_command:/s/.*trace: run_command: //p\" >actual &&\n echo \"git-frotz a '' b ' ' c\" >expect &&\n test_cmp expect actual\n\n>> +\n>> test_done\n>> --\n>> gitgitgadget\n"},{"id":"383582","messageId":"pull.314.v3.git.gitgitgadget@gmail.com","threadId":"51696","inReplyTo":"pull.314.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v3 0/1] quote: handle null and empty strings in sq_quote_buf_pretty()","fromName":"Garima Singh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-07T16:17:40Z","receivedAt":"2019-10-07T16:17:43Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"Hey,\n\nThe sq_quote_buf_pretty() function does not emit anything when the incoming\nstring is empty, but the function is to accumulate command line arguments,\nproperly quoted as necessary, and the right way to add an argument that is\nan empty string is to show it quoted, i.e. ''.\n\nLooking forward to your review. Cheers! Garima Singh\n\nReported by: Junio Hamano gitster@pobox.com [gitster@pobox.com] in\nhttps://public-inbox.org/git/pull.298.git.gitgitgadget@gmail.com/T/#m9e33936067ec2066f675aa63133a2486efd415fd\n\nGarima Singh (1):\n quote: handle numm and empty strings in sq_quote_buf_pretty\n\n quote.c | 10 ++++++++++\n t/t0014-alias.sh | 7 +++++++\n 2 files changed, 17 insertions(+)\n\n\nbase-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-314%2Fgarimasi514%2FcoreGit-fixQuote-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-314/garimasi514/coreGit-fixQuote-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/314\n\nRange-diff vs v2:\n\n 1: b9a68598d7 ! 1: 399fe02cb1 quote: handle null and empty strings in sq_quote_buf_pretty()\n @@ -1,13 +1,13 @@\n Author: Garima Singh \n \n - quote: handle null and empty strings in sq_quote_buf_pretty()\n + quote: handle numm and empty strings in sq_quote_buf_pretty\n \n - The sq_quote_buf_pretty() function does not emit anything when\n - the incoming string is empty, but the function is to accumulate\n - command line arguments, properly quoted as necessary, and the\n - right way to add an argument that is an empty string is to show\n - it quoted, i.e. ''. We warn the caller with the BUG macro if they\n - pass in a NULL.\n + The sq_quote_buf_pretty() function does not emit anything\n + when the incoming string is empty, but the function is to\n + accumulate command line arguments, properly quoted as\n + necessary, and the right way to add an argument that is an\n + empty string is to show it quoted, i.e. ''. We warn the caller\n + with the BUG macro is they pass in a NULL.\n \n Reported by: Junio Hamano \n Signed-off-by: Garima Singh \n @@ -21,11 +21,9 @@\n \n +\t/* In case of null tokens, warn the user of the BUG in their call. */\n +\tif (!src) \n -+\t\tBUG(\"BUG can't append a NULL token to the buffer\");\n ++\t\tBUG(\"Cannot append a NULL token to the buffer\");\n +\t\n -+\t/* In case of empty tokens, add a '' to ensure they \n -+\t * don't get inadvertently dropped. \n -+\t */\n ++\t/* Avoid dropping a zero-length token by adding '' */\n +\tif (!*src) {\n +\t\tstrbuf_addstr(dst, \"''\");\n +\t\treturn;\n @@ -43,11 +41,10 @@\n #'\n \n +test_expect_success 'run-command parses empty args properly, using sq_quote_buf_pretty' '\n -+\tcat >expect <<-EOF &&\n -+\tfatal: cannot change to '\\''alias.foo=frotz foo '\\'''\\'' bar'\\'': No such file or directory\n -+\tEOF\n -+\ttest_expect_code 128 git -C \"alias.foo=frotz foo '\\'''\\'' bar\" foo 2>actual &&\n -+\ttest_cmp expect actual\n ++ GIT_TRACE=1 git frotz a \"\" b \" \" c 2>&1 |\n ++ sed -ne \"/run_command:/s/.*trace: run_command: //p\" >actual &&\n ++ echo \"git-frotz a '\\'''\\'' b '\\'' '\\'' c\" >expect &&\n ++ test_cmp expect actual\n +'\n +\n test_done\n\n-- \ngitgitgadget\n"},{"id":"383583","messageId":"399fe02cb155770fc2d937607014677874075458.1570465059.git.gitgitgadget@gmail.com","threadId":"51696","inReplyTo":"pull.314.v3.git.gitgitgadget@gmail.com","subject":"[PATCH v3 1/1] quote: handle numm and empty strings in sq_quote_buf_pretty","fromName":"Garima Singh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-07T16:17:41Z","receivedAt":"2019-10-07T16:17:44Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"From: Garima Singh \n\nThe sq_quote_buf_pretty() function does not emit anything\nwhen the incoming string is empty, but the function is to\naccumulate command line arguments, properly quoted as\nnecessary, and the right way to add an argument that is an\nempty string is to show it quoted, i.e. ''. We warn the caller\nwith the BUG macro is they pass in a NULL.\n\nReported by: Junio Hamano \nSigned-off-by: Garima Singh \n---\n quote.c | 10 ++++++++++\n t/t0014-alias.sh | 7 +++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/quote.c b/quote.c\nindex 7f2aa6faa4..f31ebf6c43 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -48,6 +48,16 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)\n \tstatic const char ok_punct[] = \"+,-./:=@_^\";\n \tconst char *p;\n \n+\t/* In case of null tokens, warn the user of the BUG in their call. */\n+\tif (!src) \n+\t\tBUG(\"Cannot append a NULL token to the buffer\");\n+\t\n+\t/* Avoid dropping a zero-length token by adding '' */\n+\tif (!*src) {\n+\t\tstrbuf_addstr(dst, \"''\");\n+\t\treturn;\n+\t}\n+\n \tfor (p = src; *p; p++) {\n \t\tif (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {\n \t\t\tsq_quote_buf(dst, src);\ndiff --git a/t/t0014-alias.sh b/t/t0014-alias.sh\nindex a070e645d7..ae316aa6fd 100755\n--- a/t/t0014-alias.sh\n+++ b/t/t0014-alias.sh\n@@ -37,4 +37,11 @@ test_expect_success 'looping aliases - internal execution' '\n #\ttest_i18ngrep \"^fatal: alias loop detected: expansion of\" output\n #'\n \n+test_expect_success 'run-command parses empty args properly, using sq_quote_buf_pretty' '\n+ GIT_TRACE=1 git frotz a \"\" b \" \" c 2>&1 |\n+ sed -ne \"/run_command:/s/.*trace: run_command: //p\" >actual &&\n+ echo \"git-frotz a '\\'''\\'' b '\\'' '\\'' c\" >expect &&\n+ test_cmp expect actual\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"383587","messageId":"4bebb9e2-2afa-2c4a-78b4-3c4a9f399a0e@gmail.com","threadId":"51696","inReplyTo":"399fe02cb155770fc2d937607014677874075458.1570465059.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/1] quote: handle numm and empty strings in sq_quote_buf_pretty","fromName":"Garima Singh","fromEmail":"garimasigit@gmail.com","sentAt":"2019-10-07T17:08:44Z","receivedAt":"2019-10-07T17:08:48Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"I just noticed the typo in the commit message in my latest update.\nSorry about that. \nJunio, would you be willing to fix it up whenever you queue the patch?\nOr would you like me to send another update. \n\nThanks\nGarima G Singh\n\nOn 10/7/2019 12:17 PM, Garima Singh via GitGitGadget wrote:\n> From: Garima Singh \n> \n> The sq_quote_buf_pretty() function does not emit anything\n> when the incoming string is empty, but the function is to\n> accumulate command line arguments, properly quoted as\n> necessary, and the right way to add an argument that is an\n> empty string is to show it quoted, i.e. ''. We warn the caller\n> with the BUG macro is they pass in a NULL.\n> \n> Reported by: Junio Hamano \n> Signed-off-by: Garima Singh \n> ---\n> quote.c | 10 ++++++++++\n> t/t0014-alias.sh | 7 +++++++\n> 2 files changed, 17 insertions(+)\n> \n> diff --git a/quote.c b/quote.c\n> index 7f2aa6faa4..f31ebf6c43 100644\n> --- a/quote.c\n> +++ b/quote.c\n> @@ -48,6 +48,16 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)\n> \tstatic const char ok_punct[] = \"+,-./:=@_^\";\n> \tconst char *p;\n> \n> +\t/* In case of null tokens, warn the user of the BUG in their call. */\n> +\tif (!src) \n> +\t\tBUG(\"Cannot append a NULL token to the buffer\");\n> +\t\n> +\t/* Avoid dropping a zero-length token by adding '' */\n> +\tif (!*src) {\n> +\t\tstrbuf_addstr(dst, \"''\");\n> +\t\treturn;\n> +\t}\n> +\n> \tfor (p = src; *p; p++) {\n> \t\tif (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {\n> \t\t\tsq_quote_buf(dst, src);\n> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh\n> index a070e645d7..ae316aa6fd 100755\n> --- a/t/t0014-alias.sh\n> +++ b/t/t0014-alias.sh\n> @@ -37,4 +37,11 @@ test_expect_success 'looping aliases - internal execution' '\n> #\ttest_i18ngrep \"^fatal: alias loop detected: expansion of\" output\n> #'\n> \n> +test_expect_success 'run-command parses empty args properly, using sq_quote_buf_pretty' '\n> + GIT_TRACE=1 git frotz a \"\" b \" \" c 2>&1 |\n> + sed -ne \"/run_command:/s/.*trace: run_command: //p\" >actual &&\n> + echo \"git-frotz a '\\'''\\'' b '\\'' '\\'' c\" >expect &&\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n> \n"},{"id":"383592","messageId":"CAPig+cRtL1YPxTHfZ+uYek6hBbRmKJgSNiPNX_zM-Tc_7LnhWA@mail.gmail.com","threadId":"51696","inReplyTo":"399fe02cb155770fc2d937607014677874075458.1570465059.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/1] quote: handle numm and empty strings in sq_quote_buf_pretty","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-10-07T17:27:01Z","receivedAt":"2019-10-07T17:27:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Oct 7, 2019 at 12:17 PM Garima Singh via GitGitGadget\n wrote:\n> quote: handle numm and empty strings in sq_quote_buf_pretty\n\nWhat is \"numm\"?\n\nWhat does it mean to \"handle\" these things? A possible rewrite of the\nsubject to explain the problem more precisely rather than using\ngeneralizations might be:\n\n sq_quote_buf_pretty: don't drop empty arguments\n\n> The sq_quote_buf_pretty() function does not emit anything\n> when the incoming string is empty, but the function is to\n> accumulate command line arguments, properly quoted as\n> necessary, and the right way to add an argument that is an\n> empty string is to show it quoted, i.e. ''. We warn the caller\n> with the BUG macro is they pass in a NULL.\n\ns/is they/if they/\n\nBy including the final sentence in this paragraph, the reader is\nconfused into thinking that warning the caller with BUG() is the\noverall purpose of this patch and is the \"fix\" for the stated problem.\nAt minimum, the final sentence should be yanked out to its own\nparagraph or, better yet, dropped altogether since it's of little\nimportance in the overall scheme of the patch.\n\nAs a reader of this commit message, I find it difficult to understand\nwhat problem it's trying to solve since the problem and solution and\nexisting behavior are presented in a circuitous way which doesn't make\nany of them stand out clearly. Here's a possible rewrite:\n\n sq_quote_buf_pretty: don't drop empty arguments\n\n Empty arguments passed on a command-line should be represented by\n a zero-length quoted string, however, sq_quote_buf_pretty()\n incorrectly drops these arguments altogether. Fix this problem by\n ensuring that such arguments are emitted as '' instead.\n\n> Reported by: Junio Hamano \n> Signed-off-by: Garima Singh \n> ---\n> diff --git a/quote.c b/quote.c\n> @@ -48,6 +48,16 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)\n> + /* In case of null tokens, warn the user of the BUG in their call. */\n> + if (!src)\n> + BUG(\"Cannot append a NULL token to the buffer\");\n\nThe comment merely repeats what the code itself already says clearly,\nthus adds no value and ought to be dropped.\n\nMoreover, this entire check seems superfluous since the program will\ncrash anyhow as soon as 'src' is dereferenced (just below), thus the\nprogrammer will find out soon enough about the error. I'd suggest\ndropping this check entirely since it's not adding any value.\n\n> + /* Avoid dropping a zero-length token by adding '' */\n> + if (!*src) {\n> + strbuf_addstr(dst, \"''\");\n> + return;\n> + }\n\nDitto regarding dropping the useless comment which merely repeats what\nthe code itself already says clearly.\n\n> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh\n> @@ -37,4 +37,11 @@ test_expect_success 'looping aliases - internal execution' '\n> +test_expect_success 'run-command parses empty args properly, using sq_quote_buf_pretty' '\n\nIs \"parses\" the correct word? Should it be \"formats\" or something?\n\nAlso, the bit about \"using sq_quote_buf_pretty\" lets an implementation\ndetail bleed unnecessarily into the test suite, and that detail could\nbecome outdated at some point (say, if some function ever replaces\nthat one, for instance). It should be sufficient for the test title\nmerely to mention that it is checking that empty arguments are handled\nproperly. So, perhaps:\n\n test_expect_success 'run-command formats empty args properly' '\n\n> + GIT_TRACE=1 git frotz a \"\" b \" \" c 2>&1 |\n> + sed -ne \"/run_command:/s/.*trace: run_command: //p\" >actual &&\n> + echo \"git-frotz a '\\'''\\'' b '\\'' '\\'' c\" >expect &&\n> + test_cmp expect actual\n> +'\n"},{"id":"383594","messageId":"74a65796-ee00-66b5-a2e9-3173b057d7eb@gmail.com","threadId":"51696","inReplyTo":"CAPig+cRtL1YPxTHfZ+uYek6hBbRmKJgSNiPNX_zM-Tc_7LnhWA@mail.gmail.com","subject":"Re: [PATCH v3 1/1] quote: handle numm and empty strings in sq_quote_buf_pretty","fromName":"Garima Singh","fromEmail":"garimasigit@gmail.com","sentAt":"2019-10-07T17:47:15Z","receivedAt":"2019-10-07T17:47:18Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"On 10/7/2019 1:27 PM, Eric Sunshine wrote:\n> On Mon, Oct 7, 2019 at 12:17 PM Garima Singh via GitGitGadget\n> wrote:\n>> quote: handle numm and empty strings in sq_quote_buf_pretty\n> \n> What is \"numm\"?\n\nTypo. Fixing in next update. \n\n> What does it mean to \"handle\" these things? A possible rewrite of the\n> subject to explain the problem more precisely rather than using\n> generalizations might be:\n> \n> sq_quote_buf_pretty: don't drop empty arguments\n> \n>> The sq_quote_buf_pretty() function does not emit anything\n>> when the incoming string is empty, but the function is to\n>> accumulate command line arguments, properly quoted as\n>> necessary, and the right way to add an argument that is an\n>> empty string is to show it quoted, i.e. ''. We warn the caller\n>> with the BUG macro is they pass in a NULL.\n> \n> s/is they/if they/\n\nTypo. Fixing in next update. \n\n> By including the final sentence in this paragraph, the reader is\n> confused into thinking that warning the caller with BUG() is the\n> overall purpose of this patch and is the \"fix\" for the stated problem.\n> At minimum, the final sentence should be yanked out to its own\n> paragraph or, better yet, dropped altogether since it's of little\n> importance in the overall scheme of the patch.\n> \n> As a reader of this commit message, I find it difficult to understand\n> what problem it's trying to solve since the problem and solution and\n> existing behavior are presented in a circuitous way which doesn't make\n> any of them stand out clearly. Here's a possible rewrite:\n> \n> sq_quote_buf_pretty: don't drop empty arguments\n> \n> Empty arguments passed on a command-line should be represented by\n> a zero-length quoted string, however, sq_quote_buf_pretty()\n> incorrectly drops these arguments altogether. Fix this problem by\n> ensuring that such arguments are emitted as '' instead.\n\nWorks for me. Thanks! \n\n>> Reported by: Junio Hamano \n>> Signed-off-by: Garima Singh \n>> ---\n>> diff --git a/quote.c b/quote.c\n>> @@ -48,6 +48,16 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)\n>> + /* In case of null tokens, warn the user of the BUG in their call. */\n>> + if (!src)\n>> + BUG(\"Cannot append a NULL token to the buffer\");\n> \n> The comment merely repeats what the code itself already says clearly,\n> thus adds no value and ought to be dropped.\n> \n> Moreover, this entire check seems superfluous since the program will\n> crash anyhow as soon as 'src' is dereferenced (just below), thus the\n> programmer will find out soon enough about the error. I'd suggest\n> dropping this check entirely since it's not adding any value.\n> \n\nFair enough. Removing the comment. Leaving the check. I would\nrather the caller of the function know what went wrong instead\nof a segfault. \n\n>> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh\n>> @@ -37,4 +37,11 @@ test_expect_success 'looping aliases - internal execution' '\n>> +test_expect_success 'run-command parses empty args properly, using sq_quote_buf_pretty' '\n> \n> Is \"parses\" the correct word? Should it be \"formats\" or something?\n> \nSure. \n\n> Also, the bit about \"using sq_quote_buf_pretty\" lets an implementation\n> detail bleed unnecessarily into the test suite, and that detail could\n> become outdated at some point (say, if some function ever replaces\n> that one, for instance). It should be sufficient for the test title\n> merely to mention that it is checking that empty arguments are handled\n> properly. So, perhaps:\n> \n> test_expect_success 'run-command formats empty args properly' '\n> \n\nSure. \n"},{"id":"383605","messageId":"pull.314.v4.git.gitgitgadget@gmail.com","threadId":"51696","inReplyTo":"pull.314.v3.git.gitgitgadget@gmail.com","subject":"[PATCH v4 0/1] quote: handle null and empty strings in sq_quote_buf_pretty()","fromName":"Garima Singh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-07T19:38:56Z","receivedAt":"2019-10-07T19:39:00Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"Hey,\n\nEmpty arguments passed on the command line can be a represented by a '',\nhowever sq_quote_buf_pretty was incorrectly dropping these arguments\naltogether. Fix this problem by ensuring that such arguments are emitted as\n'' instead.\n\nLooking forward to your review. Cheers! Garima Singh\n\nReported by: Junio Hamano gitster@pobox.com [gitster@pobox.com] in\nhttps://public-inbox.org/git/pull.298.git.gitgitgadget@gmail.com/T/#m9e33936067ec2066f675aa63133a2486efd415fd\n\nGarima Singh (1):\n sq_quote_buf_pretty: don't drop empty arguments\n\n quote.c | 9 +++++++++\n t/t0014-alias.sh | 7 +++++++\n 2 files changed, 16 insertions(+)\n\n\nbase-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-314%2Fgarimasi514%2FcoreGit-fixQuote-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-314/garimasi514/coreGit-fixQuote-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/314\n\nRange-diff vs v3:\n\n 1: 399fe02cb1 ! 1: a6a0217ce6 quote: handle numm and empty strings in sq_quote_buf_pretty\n @@ -1,13 +1,11 @@\n Author: Garima Singh \n \n - quote: handle numm and empty strings in sq_quote_buf_pretty\n + sq_quote_buf_pretty: don't drop empty arguments\n \n - The sq_quote_buf_pretty() function does not emit anything\n - when the incoming string is empty, but the function is to\n - accumulate command line arguments, properly quoted as\n - necessary, and the right way to add an argument that is an\n - empty string is to show it quoted, i.e. ''. We warn the caller\n - with the BUG macro is they pass in a NULL.\n + Empty arguments passed on the command line can be a represented by\n + a '', however sq_quote_buf_pretty was incorrectly dropping these\n + arguments altogether. Fix this problem by ensuring that such\n + arguments are emitted as '' instead.\n \n Reported by: Junio Hamano \n Signed-off-by: Garima Singh \n @@ -19,11 +17,10 @@\n \tstatic const char ok_punct[] = \"+,-./:=@_^\";\n \tconst char *p;\n \n -+\t/* In case of null tokens, warn the user of the BUG in their call. */\n +\tif (!src) \n +\t\tBUG(\"Cannot append a NULL token to the buffer\");\n +\t\n -+\t/* Avoid dropping a zero-length token by adding '' */\n ++\t/* Avoid losing a zero-length string by adding '' */ \n +\tif (!*src) {\n +\t\tstrbuf_addstr(dst, \"''\");\n +\t\treturn;\n @@ -40,7 +37,7 @@\n #\ttest_i18ngrep \"^fatal: alias loop detected: expansion of\" output\n #'\n \n -+test_expect_success 'run-command parses empty args properly, using sq_quote_buf_pretty' '\n ++test_expect_success 'run-command formats empty args properly' '\n + GIT_TRACE=1 git frotz a \"\" b \" \" c 2>&1 |\n + sed -ne \"/run_command:/s/.*trace: run_command: //p\" >actual &&\n + echo \"git-frotz a '\\'''\\'' b '\\'' '\\'' c\" >expect &&\n\n-- \ngitgitgadget\n"},{"id":"383606","messageId":"a6a0217ce6fa2a7436724d76fc50fd6f8b925de5.1570477135.git.gitgitgadget@gmail.com","threadId":"51696","inReplyTo":"pull.314.v4.git.gitgitgadget@gmail.com","subject":"[PATCH v4 1/1] sq_quote_buf_pretty: don't drop empty arguments","fromName":"Garima Singh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-07T19:38:56Z","receivedAt":"2019-10-07T19:39:00Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"From: Garima Singh \n\nEmpty arguments passed on the command line can be a represented by\na '', however sq_quote_buf_pretty was incorrectly dropping these\narguments altogether. Fix this problem by ensuring that such\narguments are emitted as '' instead.\n\nReported by: Junio Hamano \nSigned-off-by: Garima Singh \n---\n quote.c | 9 +++++++++\n t/t0014-alias.sh | 7 +++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/quote.c b/quote.c\nindex 7f2aa6faa4..26f1848dde 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -48,6 +48,15 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)\n \tstatic const char ok_punct[] = \"+,-./:=@_^\";\n \tconst char *p;\n \n+\tif (!src) \n+\t\tBUG(\"Cannot append a NULL token to the buffer\");\n+\t\n+\t/* Avoid losing a zero-length string by adding '' */ \n+\tif (!*src) {\n+\t\tstrbuf_addstr(dst, \"''\");\n+\t\treturn;\n+\t}\n+\n \tfor (p = src; *p; p++) {\n \t\tif (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {\n \t\t\tsq_quote_buf(dst, src);\ndiff --git a/t/t0014-alias.sh b/t/t0014-alias.sh\nindex a070e645d7..2694c81afd 100755\n--- a/t/t0014-alias.sh\n+++ b/t/t0014-alias.sh\n@@ -37,4 +37,11 @@ test_expect_success 'looping aliases - internal execution' '\n #\ttest_i18ngrep \"^fatal: alias loop detected: expansion of\" output\n #'\n \n+test_expect_success 'run-command formats empty args properly' '\n+ GIT_TRACE=1 git frotz a \"\" b \" \" c 2>&1 |\n+ sed -ne \"/run_command:/s/.*trace: run_command: //p\" >actual &&\n+ echo \"git-frotz a '\\'''\\'' b '\\'' '\\'' c\" >expect &&\n+ test_cmp expect actual\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"383647","messageId":"xmqqd0f8nczp.fsf@gitster-ct.c.googlers.com","threadId":"51696","inReplyTo":"a6a0217ce6fa2a7436724d76fc50fd6f8b925de5.1570477135.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 1/1] sq_quote_buf_pretty: don't drop empty arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-08T03:16:10Z","receivedAt":"2019-10-08T03:16:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Garima Singh via GitGitGadget\" writes:\n\n> From: Garima Singh \n>\n> Empty arguments passed on the command line can be a represented by\n> a '', however sq_quote_buf_pretty was incorrectly dropping these\n> arguments altogether. Fix this problem by ensuring that such\n> arguments are emitted as '' instead.\n>\n> Reported by: Junio Hamano \n> Signed-off-by: Garima Singh \n> ---\n> quote.c | 9 +++++++++\n> t/t0014-alias.sh | 7 +++++++\n> 2 files changed, 16 insertions(+)\n>\n> diff --git a/quote.c b/quote.c\n> index 7f2aa6faa4..26f1848dde 100644\n> --- a/quote.c\n> +++ b/quote.c\n> @@ -48,6 +48,15 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)\n> \tstatic const char ok_punct[] = \"+,-./:=@_^\";\n> \tconst char *p;\n> \n> +\tif (!src) \n> +\t\tBUG(\"Cannot append a NULL token to the buffer\");\n\nRemove these two lines.\n\nI do not want to see \"if (!ptr) BUG(\"don't give a NULL pointer\")\"\nsprinkled to every function that takes a pointer that must not be\nNULL. Any caller that violates the contract with the callee\ndeserves a segfault, so let's leave it at that.\n\n> +\t/* Avoid losing a zero-length string by adding '' */ \n> +\tif (!*src) {\n> +\t\tstrbuf_addstr(dst, \"''\");\n> +\t\treturn;\n> +\t}\n> +\n\nNice.\n\n> \tfor (p = src; *p; p++) {\n> \t\tif (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {\n> \t\t\tsq_quote_buf(dst, src);\n> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh\n> index a070e645d7..2694c81afd 100755\n> --- a/t/t0014-alias.sh\n> +++ b/t/t0014-alias.sh\n> @@ -37,4 +37,11 @@ test_expect_success 'looping aliases - internal execution' '\n> #\ttest_i18ngrep \"^fatal: alias loop detected: expansion of\" output\n> #'\n> \n> +test_expect_success 'run-command formats empty args properly' '\n> + GIT_TRACE=1 git frotz a \"\" b \" \" c 2>&1 |\n> + sed -ne \"/run_command:/s/.*trace: run_command: //p\" >actual &&\n> + echo \"git-frotz a '\\'''\\'' b '\\'' '\\'' c\" >expect &&\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"383690","messageId":"412626ccf98e687b26e26d935a2fc23154a9f465.1570552838.git.gitgitgadget@gmail.com","threadId":"51696","inReplyTo":"pull.314.v5.git.gitgitgadget@gmail.com","subject":"[PATCH v5 1/1] sq_quote_buf_pretty: don't drop empty arguments","fromName":"Garima Singh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-08T16:40:40Z","receivedAt":"2019-10-08T16:40:43Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"From: Garima Singh \n\nEmpty arguments passed on the command line can be a represented by\na '', however sq_quote_buf_pretty was incorrectly dropping these\narguments altogether. Fix this problem by ensuring that such\narguments are emitted as '' instead.\n\nReported by: Junio Hamano \nSigned-off-by: Garima Singh \n---\n quote.c | 6 ++++++\n t/t0014-alias.sh | 7 +++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/quote.c b/quote.c\nindex 7f2aa6faa4..2d5d1c4360 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -48,6 +48,12 @@ void sq_quote_buf_pretty(struct strbuf *dst, const char *src)\n \tstatic const char ok_punct[] = \"+,-./:=@_^\";\n \tconst char *p;\n \n+\t/* Avoid losing a zero-length string by adding '' */ \n+\tif (!*src) {\n+\t\tstrbuf_addstr(dst, \"''\");\n+\t\treturn;\n+\t}\n+\n \tfor (p = src; *p; p++) {\n \t\tif (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {\n \t\t\tsq_quote_buf(dst, src);\ndiff --git a/t/t0014-alias.sh b/t/t0014-alias.sh\nindex a070e645d7..2694c81afd 100755\n--- a/t/t0014-alias.sh\n+++ b/t/t0014-alias.sh\n@@ -37,4 +37,11 @@ test_expect_success 'looping aliases - internal execution' '\n #\ttest_i18ngrep \"^fatal: alias loop detected: expansion of\" output\n #'\n \n+test_expect_success 'run-command formats empty args properly' '\n+ GIT_TRACE=1 git frotz a \"\" b \" \" c 2>&1 |\n+ sed -ne \"/run_command:/s/.*trace: run_command: //p\" >actual &&\n+ echo \"git-frotz a '\\'''\\'' b '\\'' '\\'' c\" >expect &&\n+ test_cmp expect actual\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"383691","messageId":"pull.314.v5.git.gitgitgadget@gmail.com","threadId":"51696","inReplyTo":"pull.314.v4.git.gitgitgadget@gmail.com","subject":"[PATCH v5 0/1] quote: handle null and empty strings in sq_quote_buf_pretty()","fromName":"Garima Singh via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-08T16:40:39Z","receivedAt":"2019-10-08T16:40:43Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"Hey,\n\nEmpty arguments passed on the command line can be a represented by a '',\nhowever sq_quote_buf_pretty was incorrectly dropping these arguments\naltogether. Fix this problem by ensuring that such arguments are emitted as\n'' instead.\n\nLooking forward to your review. Cheers! Garima Singh\n\nReported by: Junio Hamano gitster@pobox.com [gitster@pobox.com] in\nhttps://public-inbox.org/git/pull.298.git.gitgitgadget@gmail.com/T/#m9e33936067ec2066f675aa63133a2486efd415fd\n\nGarima Singh (1):\n sq_quote_buf_pretty: don't drop empty arguments\n\n quote.c | 6 ++++++\n t/t0014-alias.sh | 7 +++++++\n 2 files changed, 13 insertions(+)\n\n\nbase-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-314%2Fgarimasi514%2FcoreGit-fixQuote-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-314/garimasi514/coreGit-fixQuote-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/314\n\nRange-diff vs v4:\n\n 1: a6a0217ce6 ! 1: 412626ccf9 sq_quote_buf_pretty: don't drop empty arguments\n @@ -17,9 +17,6 @@\n \tstatic const char ok_punct[] = \"+,-./:=@_^\";\n \tconst char *p;\n \n -+\tif (!src) \n -+\t\tBUG(\"Cannot append a NULL token to the buffer\");\n -+\t\n +\t/* Avoid losing a zero-length string by adding '' */ \n +\tif (!*src) {\n +\t\tstrbuf_addstr(dst, \"''\");\n\n-- \ngitgitgadget\n"}]}