Re: [PATCH] config: read the bytes of a blob as unsigned characters

[Junio C Hamano <gitster@pobox.com>]

Olof Fairbrook <olof.fairbrook@gmail.com> writes:

> "git config --blob" and the other readers of config from memory get
> each byte from config_buf_fgetc(), which returns it as a plain "char".
> Where "char" is signed, as on x86, a byte 0xff comes back as -1, which
> is EOF, and any byte from 0x80 up as a negative number.  So the parser
> stops at a byte 0xff and silently ignores the rest of the blob:
>
>   $ printf '# caf\377\n[some]\n\tkey = value\n' >config
>   $ git config --blob=$(git hash-object -w config) --list
>   $
>
> and a blob that starts with a UTF-8 byte order mark is a "bad config
> line 1", because the code that skips the mark compares with unsigned
> bytes.  A config file has neither problem, as getc() returns unsigned
> bytes.  ".gitmodules" read from a commit goes through the same reader.

Nicely explained, even though it is a tad overly verbose side
(especially given that the example code is something we can see in
the added test easily).

> This has been so since config_buf_fgetc() was added in 1bc888193e
> (teach config --blob option to parse config from database,
> 2013-07-12).
>
> Return the byte as an unsigned char, as getc() does.
> config_buf_ungetc() checks that it is given back the byte it read; it
> now compares with the byte as an unsigned char too, or a CR followed
> by such a byte would hit its BUG().

Very nice.  Will queue as-is.

> diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh
> index 5cb546dd00..e2c0883350 100755
> --- a/t/t1307-config-blob.sh
> +++ b/t/t1307-config-blob.sh
> @@ -72,6 +72,27 @@ test_expect_success 'can parse blob ending with CR' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'a byte 0xff does not end a blob' '
> +	test_commit --printf FF config "# caf\\377\\n[some]\\n\\tkey = value\\n" &&
> +	echo value >expect &&
> +	git config --blob=HEAD:config some.key >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'a byte 0xff after a CR in a blob' '
> +	test_commit --printf CRFF config "# a\\r\\377\\n[some]\\n\\tkey = value\\n" &&
> +	echo value >expect &&
> +	git config --blob=HEAD:config some.key >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'can parse blob starting with a byte order mark' '
> +	test_commit --printf BOM config "\\357\\273\\277[some]\\n\\tkey = value\\n" &&
> +	echo value >expect &&
> +	git config --blob=HEAD:config some.key >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'config --blob outside of a repository is an error' '
>  	nongit test_must_fail git config --blob=foo --list
>  '
>
> base-commit: a018953688f1b10bddf91bff8747068f5f4746a4