Re: [PATCH v3 31/39] bundle: add new version for use with SHA-256

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 23, 2020, 05:31 UTC
Message-ID: <CAPig+cRWptYRJUpT71jW6_O9UZ1RK=FCbZj-+OkR_5kGqSWScg@mail.gmail.com>
In-Reply-To: <20200723010943.2329634-32-sandals@crustytoothpaste.net>

On Wed, Jul 22, 2020 at 9:10 PM brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 16 quoted lines

> [...]
> Since we cannot extend the v2 format in a backward-compatible way, let's
> add a v3 format, which is identical, except for the addition of
> capabilities, which are prefixed by an at sign.  We add "object-format"
> as the only capability and reject unknown capabilities, since we do not
> have a network connection and therefore cannot negotiate with the other
> side.
> [...]
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> diff --git a/Documentation/technical/bundle-format.txt b/Documentation/technical/bundle-format.txt
> @@ -7,6 +7,8 @@ The Git bundle format is a format that represents both refs and Git objects.
>  == Semantics
>
> -A Git bundle consists of three parts.
> +A Git bundle consists of four parts.

Rather than having to worry about updating this each time the format changes, perhaps just say:

    A Git bundle consists of several parts.

Show 14 quoted lines

> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> @@ -281,15 +281,16 @@ test_expect_success 'create bundle 1' '
>  test_expect_success 'header of bundle looks right' '
>         head -n 4 "$D"/bundle1 &&
> -       head -n 1 "$D"/bundle1 | grep "^#" &&
> -       head -n 2 "$D"/bundle1 | grep "^-$OID_REGEX " &&
> -       head -n 3 "$D"/bundle1 | grep "^$OID_REGEX " &&
> -       head -n 4 "$D"/bundle1 | grep "^$"
> +       sed -n -e 1p "$D"/bundle1 | grep "^#" &&
> +       sed -n -e 2p "$D"/bundle1 | grep "^@object-format=" &&
> +       sed -n -e 3p "$D"/bundle1 | grep "^-$OID_REGEX " &&
> +       sed -n -e 4p "$D"/bundle1 | grep "^$OID_REGEX " &&
> +       sed -n -e 5p "$D"/bundle1 | grep "^$"
>  '

I wonder if it would be simpler and cleaner to do something like this instead:

    cat >expect <<\-EOF &&
    # v3 git bundle
    @object-format=sha256
    -[OID] message
    [OID] refs/head/master

    EOF
    head -n 5 "$D"/bundle1 | sanitize_oid >actual &&
    test_cmp expect actual

where sanitize_oid() is a function which converts a hex OID into the literal string "[OID]" (or whatever). I believe I've seen you employ such sanitization functions already in these series in cases when you want to verify that an OID is present in some output but don't care about the actual value.

Or perhaps this approach is overkill?

Reading further in this patch, I see that you actually do employ this technique in a new test you add to t5607, though you don't bother with OID sanitation in that test.

Show 11 quoted lines

> diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
> @@ -94,4 +98,29 @@ test_expect_success 'fetch SHA-1 from bundle' '
> +test_expect_success 'git bundle v3 has expected contents' '
> +       git branch side HEAD &&
> +       git bundle create --version=3 bundle HEAD^..side &&
> +       head -n2 bundle >actual &&
> +       cat >expect <<-EOF &&
> +       # v3 git bundle
> +       @object-format=$(test_oid algo)
> +       EOF
> +       test_cmp actual expect &&

I think you want to swap the 'actual' and 'expect' arguments.

Show 10 quoted lines

> +       git bundle verify bundle
> +'
> +
> +test_expect_success 'git bundle v3 rejects unknown extensions' '
> +       head -n2 bundle >new &&
> +       echo "@unknown=silly" >>new &&
> +       sed "1,2d" bundle >>new &&
> +       test_must_fail git bundle verify new 2>output &&
> +       test_i18ngrep "unknown capability .unknown=silly." output
> +'

I worry about passing binary bundle data through 'sed' like this. Historically, some 'sed' implementations would drop the last part of a file if it didn't end with a newline. Even today, not all 'sed' implementations necessarily pass the binary data through unmolested. For instance, on Mac OS, 'sed' adds a newline at the end of the file if the binary bundle blob didn't end with a newline. Perhaps a more reliable approach would be to use Perl to read the entire bundle in as a blob, use s/// to munge the @object-format= line into the @unknown= line, and write it out.