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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 03/31/2014 11:36 PM, Junio C Hamano wrote:
> 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.

The old parse_arg(), when fed an argument

    "refs/heads/a"master

parsed 'refs/heads/a' off of the front of the argument and considered
itself successful.  It was only when parse_next_arg() tried to parse the
*next* argument that a problem was noticed.  But in fact, the definition
of the input format requires arguments to be terminated by SP or NUL, so
*this* argument is already erroneous and parse_arg() should diagnose the
problem.

The point of this patch is to move the error detection for C-quoted
arguments that have trailing junk to the parse_arg() call for the broken
argument and to make the error message more descriptive of the situation.

There is no corresponding error case for non-C-quoted arguments, because
the end of the argument is *by definition* a space or NUL, so there is
no way to insert other junk between the "end" of the argument and the
argument terminator.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/