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

2014-03-31 Thread Junio C Hamano
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 2err 
 - 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.
--
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


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

2014-03-24 Thread Michael Haggerty
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 2err 
-   grep fatal: expected SP but got: master err
+   grep fatal: unexpected character after quoted argument: 
\\\$a\\\master err
 '
 
 test_expect_success 'stdin fails create with no ref' '
-- 
1.9.0

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