Re: [PATCH v3 1/1] quote: handle numm and empty strings in sq_quote_buf_pretty

[Eric Sunshine <sunshine@sunshineco.com>]

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

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

> +       /* 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
> +'