On 03/31/2014 11:36 PM, Junio C Hamano wrote:
> Michael Haggerty <mhag...@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 <mhag...@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
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
