Re: [PATCH 1/2] t1450: add tests for NUL in headers of commits and tags

[Jeff King]

On Thu, Nov 19, 2015 at 09:54:54PM +0100, René Scharfe wrote:

> >>diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> >>@@ -176,6 +176,18 @@ test_expect_success 'integer overflow in timestamps is 
> >>reported' '
> >>+test_expect_success 'commit with NUL in header' '
> >>+   git cat-file commit HEAD >basis &&
> >>+   sed "s/author ./author Q/" commit-NUL-header &&
> >>+   new=$(git hash-object -t commit -w --stdin  >>+   test_when_finished "remove_object $new" &&
> >>+   git update-ref refs/heads/bogus "$new" &&
> >>+   test_when_finished "git update-ref -d refs/heads/bogus" &&
> >>+   test_must_fail git fsck 2>out &&
> >>+   cat out &&
> >
> >What is the purpose of this 'cat'?
> 
> It shows the full error message when the test is run with --debug, which is
> convenient when the following grep doesn't match.  The same is done in most
> tests in that file.

I'm slightly negative on such a construct, just because it wastes a
process in the case where we are not in --verbose mode. I don't mind it
in this patch in the spirit of consistency within t1450, but I think we
should probably avoid spreading it.

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

Re: [PATCH 1/2] t1450: add tests for NUL in headers of commits and tags

[René Scharfe]

Am 20.11.2015 um 12:14 schrieb Jeff King:

On Thu, Nov 19, 2015 at 09:54:54PM +0100, René Scharfe wrote:


diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
@@ -176,6 +176,18 @@ test_expect_success 'integer overflow in timestamps is 
reported' '
+test_expect_success 'commit with NUL in header' '
+   git cat-file commit HEAD >basis &&
+   sed "s/author ./author Q/" commit-NUL-header &&
+   new=$(git hash-object -t commit -w --stdin out &&
+   cat out &&


What is the purpose of this 'cat'?


It shows the full error message when the test is run with --debug, which is
convenient when the following grep doesn't match.  The same is done in most
tests in that file.


I'm slightly negative on such a construct, just because it wastes a
process in the case where we are not in --verbose mode. I don't mind it
in this patch in the spirit of consistency within t1450, but I think we
should probably avoid spreading it.


This practice is not used that much (yet).  We can contain it by
providing an alternative.  I'll send patches for adding a helper
function similar to test_must_be_empty for that.

René

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

Re: [PATCH 1/2] t1450: add tests for NUL in headers of commits and tags

[Eric Sunshine]

On Thu, Nov 19, 2015 at 11:20 AM, René Scharfe  wrote:
> Signed-off-by: Rene Scharfe 
> ---
> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> @@ -176,6 +176,18 @@ test_expect_success 'integer overflow in timestamps is 
> reported' '
> +test_expect_success 'commit with NUL in header' '
> +   git cat-file commit HEAD >basis &&
> +   sed "s/author ./author Q/" commit-NUL-header &&
> +   new=$(git hash-object -t commit -w --stdin  +   test_when_finished "remove_object $new" &&
> +   git update-ref refs/heads/bogus "$new" &&
> +   test_when_finished "git update-ref -d refs/heads/bogus" &&
> +   test_must_fail git fsck 2>out &&
> +   cat out &&

What is the purpose of this 'cat'?

> +   grep "error in commit $new.*unterminated header: NUL at offset" out
> +'
> @@ -276,6 +288,26 @@ test_expect_success 'tag with bad tagger' '
> +test_expect_failure 'tag with NUL in header' '
> +   sha=$(git rev-parse HEAD) &&
> +   q_to_nul >tag-NUL-header <<-EOF &&
> +   object $sha
> +   type commit
> +   tag contains-Q-in-header
> +   tagger T A Gger  1234567890 -
> +
> +   This is an invalid tag.
> +   EOF
> +
> +   tag=$(git hash-object --literally -t tag -w --stdin  &&
> +   test_when_finished "remove_object $tag" &&
> +   echo $tag >.git/refs/tags/wrong &&
> +   test_when_finished "git update-ref -d refs/tags/wrong" &&
> +   test_must_fail git fsck --tags 2>out &&
> +   cat out &&

Ditto.

> +   grep "error in tag $tag.*unterminated header: NUL at offset" out
> +'
> +
> --
> 2.6.3
--
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

Re: [PATCH 1/2] t1450: add tests for NUL in headers of commits and tags

[René Scharfe]

Am 19.11.2015 um 21:33 schrieb Eric Sunshine:

On Thu, Nov 19, 2015 at 11:20 AM, René Scharfe  wrote:

Signed-off-by: Rene Scharfe 
---
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
@@ -176,6 +176,18 @@ test_expect_success 'integer overflow in timestamps is 
reported' '
+test_expect_success 'commit with NUL in header' '
+   git cat-file commit HEAD >basis &&
+   sed "s/author ./author Q/" commit-NUL-header &&
+   new=$(git hash-object -t commit -w --stdin out &&
+   cat out &&


What is the purpose of this 'cat'?


It shows the full error message when the test is run with --debug, which 
is convenient when the following grep doesn't match.  The same is done 
in most tests in that file.





+   grep "error in commit $new.*unterminated header: NUL at offset" out
+'
@@ -276,6 +288,26 @@ test_expect_success 'tag with bad tagger' '
+test_expect_failure 'tag with NUL in header' '
+   sha=$(git rev-parse HEAD) &&
+   q_to_nul >tag-NUL-header <<-EOF &&
+   object $sha
+   type commit
+   tag contains-Q-in-header
+   tagger T A Gger  1234567890 -
+
+   This is an invalid tag.
+   EOF
+
+   tag=$(git hash-object --literally -t tag -w --stdin .git/refs/tags/wrong &&
+   test_when_finished "git update-ref -d refs/tags/wrong" &&
+   test_must_fail git fsck --tags 2>out &&
+   cat out &&


Ditto.


+   grep "error in tag $tag.*unterminated header: NUL at offset" out
+'
+
--
2.6.3

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