[PATCH] t1450: fix quoting of NUL byte when corrupting pack

We use

  printf '\0'

to generate a NUL byte which we then `dd` into the packfile to ensure
that we modify the first byte of the first object, thereby
(probabilistically) invalidating the checksum. Except the single quotes
we're using are interpreted to match with the ones we enclose the whole
test in. So we actually execute

  printf \0

and end up injecting the ASCII code for "0", 0x30, instead.

The comment right above this `printf` invocation says that "at least one
of [the type bits] is not zero, so setting the first byte to 0 is
sufficient". Substituting "0x30" for "0" in that comment won't do: we'd
need to reason about which bits go where and just what the packfile
looks like that we're modifying in this test.

Let's avoid all of that by actually executing

  printf "\0"

to generate a NUL byte, as intended.

Signed-off-by: Martin Ågren <redacted>
---
 If my reading is correct, when we substitute 0x30, the type will be 3
 (blob) and the size will be zero. So there might actually exist
 formally valid packfiles where this byte that we're modifying is
 already zero. What matters in the end is whether we might be using such
 a packfile in this exact test and from what I can tell, no, we won't be
 doing that.

 t/t1450-fsck.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 344a2aad82..af2a2c4682 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh

@@ -714,7 +714,7 @@ test_expect_success 'fsck fails on corrupt packfile' '
 	# at least one of which is not zero, so setting the first byte to 0 is
 	# sufficient.)
 	chmod a+w .git/objects/pack/pack-$pack.pack &&
-	printf '\0' | dd of=.git/objects/pack/pack-$pack.pack bs=1 conv=notrunc seek=12 &&
+	printf "\0" | dd of=.git/objects/pack/pack-$pack.pack bs=1 conv=notrunc seek=12 &&
 
 	test_when_finished "rm -f .git/objects/pack/pack-$pack.*" &&
 	remove_object $hsh &&

-- 
2.28.0.81.ge8ab941b67

On Sat, Aug 1, 2020 at 3:08 PM Martin Ågren [off-list ref] wrote:

  printf '\0'

to generate a NUL byte

[but]

end up injecting the ASCII code for "0", 0x30, instead.

-       printf '\0' | dd of=.git/objects/pack/pack-$pack.pack bs=1 conv=notrunc seek=12 &&
+       printf "\0" | dd of=.git/objects/pack/pack-$pack.pack bs=1 conv=notrunc seek=12 &&

In sh/bash, this should make no difference, and that's what I get here.
Am I missing something?

$ printf '\0' | hexdump
0000000 0000
0000001
$ printf "\0" | hexdump
0000000 0000
0000001

Chris

Oh, wait, I am indeed missing something -- the script itself is
all in single quotes, so the single quotes on the line in question
are wrong.  Never mind!

Chris

On Sun, 2 Aug 2020 at 03:02, Chris Torek [off-list ref] wrote:

Oh, wait, I am indeed missing something -- the script itself is
all in single quotes, so the single quotes on the line in question
are wrong.  Never mind!

No worries! Thanks for having a look at the patch. Is there anything
that could be done to make this clearer in the commit message? (I find it
quite awkward to discuss quoting: will the reader understand which
quoting is part of my own formatting of the message vs which is part of
the quoting issue I want to get across!?)

Martin

On Sun, Aug 2, 2020 at 7:35 AM Martin Ågren [off-list ref] wrote:

No worries! Thanks for having a look at the patch. Is there anything
that could be done to make this clearer in the commit message? (I find it
quite awkward to discuss quoting: will the reader understand which
quoting is part of my own formatting of the message vs which is part of
the quoting issue I want to get across!?)

This is indeed a problem...

Perhaps something along these lines (generic boilerplate
for any single-quote fixes, that should be adjusted for the
actual fix):

    In the test scripts, the recommended style is, e.g.:

        test_expect_success 'name' '
            multi-line test
            goes here
        '

    When using this style, any single quote in the multi-line
    test section is actually closing the lone single quotes
    that surround it.  To avoid confusion, minimize and/or
    eliminate the use of single quotes here.

Chris