Re: [PATCH v2 03/27] parse_arg(): Really test that argument is properly terminated

[Junio C Hamano <gitster@pobox.com>]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Test that the argument is properly terminated by either whitespace or
> a NUL character, even if it is quoted, to be consistent with the
> non-quoted case.  Adjust the tests to expect the new error message.
> Add a docstring to the function, incorporating the comments that were
> formerly within the function plus some added information.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  builtin/update-ref.c  | 20 +++++++++++++++-----
>  t/t1400-update-ref.sh |  4 ++--
>  2 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 1292cfe..02b5f95 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -62,16 +62,26 @@ static void update_store_old_sha1(struct ref_update *update,
>  	update->have_old = *oldvalue || line_termination;
>  }
>  
> +/*
> + * Parse one whitespace- or NUL-terminated, possibly C-quoted argument
> + * and append the result to arg.  Return a pointer to the terminator.
> + * Die if there is an error in how the argument is C-quoted.  This
> + * function is only used if not -z.
> + */
>  static const char *parse_arg(const char *next, struct strbuf *arg)
>  {
> -	/* Parse SP-terminated, possibly C-quoted argument */
> -	if (*next != '"')
> +	if (*next == '"') {
> +		const char *orig = next;
> +
> +		if (unquote_c_style(arg, next, &next))
> +			die("badly quoted argument: %s", orig);
> +		if (*next && !isspace(*next))
> +			die("unexpected character after quoted argument: %s", orig);
> +	} else {
>  		while (*next && !isspace(*next))
>  			strbuf_addch(arg, *next++);
> -	else if (unquote_c_style(arg, next, &next))
> -		die("badly quoted argument: %s", next);
> +	}
>  
> -	/* Return position after the argument */
>  	return next;
>  }
>  
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 29391c6..774f8c5 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -356,10 +356,10 @@ test_expect_success 'stdin fails on badly quoted input' '
>  	grep "fatal: badly quoted argument: \\\"master" err
>  '
>  
> -test_expect_success 'stdin fails on arguments not separated by space' '
> +test_expect_success 'stdin fails on junk after quoted argument' '
>  	echo "create \"$a\"master" >stdin &&
>  	test_must_fail git update-ref --stdin <stdin 2>err &&
> -	grep "fatal: expected SP but got: master" err
> +	grep "fatal: unexpected character after quoted argument: \\\"$a\\\"master" err
>  '

Interesting.

I would have expected that "We used to check only one of the two
codepaths and the other was loose, fix it" to be accompanied by "So
here is an _addition_ to the test suite to validate the other case
that used to be loose, now tightened", not "We update one existing
case".  The test before and after the patch is about a c-quoted
string, so I am not sure if we are still testing the right thing.

The code in update-ref.c after the patch does look reasonable,
though.