From: Martin Ågren <martin.agren@gmail.com>
Date: Sat, 16 May 2020 11:15:25 GMT
Subject: Re: [PATCH 32/44] serve: advertise object-format capability for protocol v2
Message-ID: <CAN0heSpNp1f=3p_ef-13547Sf6qkDXG1-i+0tDDX3VBhP72dgQ@mail.gmail.com>
In-Reply-To: <20200513005424.81369-33-sandals@crustytoothpaste.net>

On Wed, 13 May 2020 at 02:56, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> In order to communicate the protocol supported by the server side, add
> support for advertising the object-format capability.  We check that the
> client side sends us an identical algorithm if it sends us its own
> object-format capability, and assume it speaks SHA-1 if not.
>
> In the test, when we're using an algorithm other than SHA-1, we need to
> specify the algorithm in use so we don't get a failure with an "unknown
> format" message. Add a wrapper function that specifies this header if
> required.  Skip specifying this header for SHA-1 to test that it works
> both with and without this header.

This last sentence sort of answers an earlier question I made: should we
stop special-casing in the test and just always write the capability? I
can see your point here, but it only applies if you actually go to the
trouble of running the tests both with SHA-1 and SHA-256, right?

That is, I wonder if we shouldn't always pass the "object-format"
capability in the tests and, if we have the SHA-1 prereq, execute a
dedicated test where we do not pass it and verify that we default
correctly. Hmm?

> +write_command () {
> +       echo "command=$1"
> +
> +       if test "$(test_oid algo)" != sha1
> +       then
> +               echo "object-format=$(test_oid algo)"
> +       fi
> +}
> +
>  test_expect_success 'test capability advertisement' '
> +       test_oid_init &&
>         cat >expect <<-EOF &&
>         version 2
>         agent=git/$(git version | cut -d" " -f3)
>         ls-refs
>         fetch=shallow
>         server-option
> +       object-format=$(test_oid algo)
>         0000
>         EOF
>
> @@ -45,6 +56,7 @@ test_expect_success 'request invalid capability' '
>  test_expect_success 'request with no command' '
>         test-tool pkt-line pack >in <<-EOF &&
>         agent=git/test
> +       object-format=$(test_oid algo)
>         0000
>         EOF
>         test_must_fail test-tool serve-v2 --stateless-rpc 2>err <in &&

In these two tests, we give "object-format" unconditionally, meaning
that in a SHA-1 run, we don't *always* skip passing in the capability.
So that's good. Should we verify that the implementation acts on the
"object-format=sha1" capability? Can we? The server should behave as
if it wasn't passed in at all, so I'm not sure how we could do that.

But that brings me to another point: Shouldn't we try to test the whole
"mismatched object format" detection by passing in "sha1" in a SHA-256
build and "sha256" with SHA-1. I suppose a `test_oid wrong_algo` could
come in handy in lots of negative tests that we'll want to add
throughout. Or maybe that doesn't quite fit the long-term goal.


Martin