git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: Garima Singh via GitGitGadget <gitgitgadget@gmail.com>
Cc: Git List <git@vger.kernel.org>,
	garimasigit@gmail.com, Jeff Hostetler <jeffhost@microsoft.com>,
	Derrick Stolee <stolee@gmail.com>,
	Junio C Hamano <gitster@pobox.com>,
	Garima Singh <garima.singh@microsoft.com>
Subject: Re: [PATCH v3 1/1] quote: handle numm and empty strings in sq_quote_buf_pretty
Date: Mon, 7 Oct 2019 13:27:01 -0400	[thread overview]
Message-ID: <CAPig+cRtL1YPxTHfZ+uYek6hBbRmKJgSNiPNX_zM-Tc_7LnhWA@mail.gmail.com> (raw)
In-Reply-To: <399fe02cb155770fc2d937607014677874075458.1570465059.git.gitgitgadget@gmail.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
> +'

  parent reply	other threads:[~2019-10-07 17:27 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-20 19:35 [PATCH 0/1] quote: handle null and empty strings in sq_quote_buf_pretty() Garima Singh via GitGitGadget
2019-08-20 19:35 ` [PATCH 1/1] " Garima Singh via GitGitGadget
2019-08-20 20:29   ` Junio C Hamano
2019-08-21 15:22     ` Junio C Hamano
2019-08-20 20:32   ` Junio C Hamano
2019-08-26 14:44 ` [PATCH v2 0/1] " Garima Singh via GitGitGadget
2019-08-26 14:44   ` [PATCH v2 1/1] " Garima Singh via GitGitGadget
2019-08-26 15:24     ` Garima Singh
2019-08-26 16:20       ` Junio C Hamano
2019-10-07 16:17   ` [PATCH v3 0/1] " Garima Singh via GitGitGadget
2019-10-07 16:17     ` [PATCH v3 1/1] quote: handle numm and empty strings in sq_quote_buf_pretty Garima Singh via GitGitGadget
2019-10-07 17:08       ` Garima Singh
2019-10-07 17:27       ` Eric Sunshine [this message]
2019-10-07 17:47         ` Garima Singh
2019-10-07 19:38     ` [PATCH v4 0/1] quote: handle null and empty strings in sq_quote_buf_pretty() Garima Singh via GitGitGadget
2019-10-07 19:38       ` [PATCH v4 1/1] sq_quote_buf_pretty: don't drop empty arguments Garima Singh via GitGitGadget
2019-10-08  3:16         ` Junio C Hamano
2019-10-08 16:40       ` [PATCH v5 0/1] quote: handle null and empty strings in sq_quote_buf_pretty() Garima Singh via GitGitGadget
2019-10-08 16:40         ` [PATCH v5 1/1] sq_quote_buf_pretty: don't drop empty arguments Garima Singh via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPig+cRtL1YPxTHfZ+uYek6hBbRmKJgSNiPNX_zM-Tc_7LnhWA@mail.gmail.com \
    --to=sunshine@sunshineco.com \
    --cc=garima.singh@microsoft.com \
    --cc=garimasigit@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=jeffhost@microsoft.com \
    --cc=stolee@gmail.com \
    --subject='Re: [PATCH v3 1/1] quote: handle numm and empty strings in sq_quote_buf_pretty' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).