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

Michael Haggerty [off-list ref] writes:

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.

On 03/31/2014 11:36 PM, Junio C Hamano wrote:

Michael Haggerty [off-list ref] writes:

quoted

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 <redacted>
---
 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/