Re: [PATCH v4 1/1] sq_quote_buf_pretty: don't drop empty arguments

From: Junio C Hamano <gitster@pobox.com>
To: "Garima Singh via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, garimasigit@gmail.com,
	jeffhost@microsoft.com, stolee@gmail.com,
	Garima Singh <garima.singh@microsoft.com>
Subject: Re: [PATCH v4 1/1] sq_quote_buf_pretty: don't drop empty arguments
Date: Tue, 08 Oct 2019 12:16:10 +0900	[thread overview]
Message-ID: <xmqqd0f8nczp.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <a6a0217ce6fa2a7436724d76fc50fd6f8b925de5.1570477135.git.gitgitgadget@gmail.com> (Garima Singh via GitGitGadget's message of "Mon, 07 Oct 2019 12:38:56 -0700 (PDT)")

"Garima Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Garima Singh <garima.singh@microsoft.com>
>
> Empty arguments passed on the command line can be a represented by
> a '', however sq_quote_buf_pretty was incorrectly dropping 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>
> ---
>  quote.c          | 9 +++++++++
>  t/t0014-alias.sh | 7 +++++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/quote.c b/quote.c
> index 7f2aa6faa4..26f1848dde 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;
>  
> +	if (!src) 
> +		BUG("Cannot append a NULL token to the buffer");

Remove these two lines.

I do not want to see "if (!ptr) BUG("don't give a NULL pointer")"
sprinkled to every function that takes a pointer that must not be
NULL.  Any caller that violates the contract with the callee
deserves a segfault, so let's leave it at that.

> +	/* Avoid losing a zero-length string by adding '' */ 
> +	if (!*src) {
> +		strbuf_addstr(dst, "''");
> +		return;
> +	}
> +

Nice.

>  	for (p = src; *p; p++) {
>  		if (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {
>  			sq_quote_buf(dst, src);
> diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
> index a070e645d7..2694c81afd 100755
> --- a/t/t0014-alias.sh
> +++ b/t/t0014-alias.sh
> @@ -37,4 +37,11 @@ test_expect_success 'looping aliases - internal execution' '
>  #	test_i18ngrep "^fatal: alias loop detected: expansion of" output
>  #'
>  
> +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
> +'
> +
>  test_done