Re: [PATCH v3 1/1] quote: handle numm and empty strings in sq_quote_buf_pretty
On Mon, Oct 7, 2019 at 12:17 PM Garima Singh via GitGitGadget <gitgitgadget@gmail.com> 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
Show 6 quoted lines
> 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.Show 8 quoted lines
> Reported by: Junio Hamano <gitster@pobox.com>
> Signed-off-by: Garima Singh <garima.singh@microsoft.com>
> ---
> 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.
Show 5 quoted lines
> + /* 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' '
Show 5 quoted lines
> + 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
> +'
