diff mbox series

[32/44] serve: advertise object-format capability for protocol v2

Message ID 20200513005424.81369-33-sandals@crustytoothpaste.net (mailing list archive)
State New, archived
Headers show
Series SHA-256 part 2/3: protocol functionality | expand

Commit Message

brian m. carlson May 13, 2020, 12:54 a.m. UTC
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.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 serve.c              | 27 +++++++++++++++++++++++++++
 t/t5701-git-serve.sh | 28 ++++++++++++++++++++--------
 2 files changed, 47 insertions(+), 8 deletions(-)

Comments

Martin Ă…gren May 16, 2020, 11:15 a.m. UTC | #1
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
diff mbox series

Patch

diff --git a/serve.c b/serve.c
index 317256c1a4..7ab7807fef 100644
--- a/serve.c
+++ b/serve.c
@@ -22,6 +22,14 @@  static int agent_advertise(struct repository *r,
 	return 1;
 }
 
+static int object_format_advertise(struct repository *r,
+				   struct strbuf *value)
+{
+	if (value)
+		strbuf_addstr(value, r->hash_algo->name);
+	return 1;
+}
+
 struct protocol_capability {
 	/*
 	 * The name of the capability.  The server uses this name when
@@ -57,6 +65,7 @@  static struct protocol_capability capabilities[] = {
 	{ "ls-refs", always_advertise, ls_refs },
 	{ "fetch", upload_pack_advertise, upload_pack_v2 },
 	{ "server-option", always_advertise, NULL },
+	{ "object-format", object_format_advertise, NULL },
 };
 
 static void advertise_capabilities(void)
@@ -153,6 +162,22 @@  int has_capability(const struct argv_array *keys, const char *capability,
 	return 0;
 }
 
+static void check_algorithm(struct repository *r, struct argv_array *keys)
+{
+	int client = GIT_HASH_SHA1, server = hash_algo_by_ptr(r->hash_algo);
+	const char *algo_name;
+
+	if (has_capability(keys, "object-format", &algo_name)) {
+		client = hash_algo_by_name(algo_name);
+		if (client == GIT_HASH_UNKNOWN)
+			die("unknown object format '%s'", algo_name);
+	}
+
+	if (client != server)
+		die("mismatched object format: server %s; client %s\n",
+		    r->hash_algo->name, hash_algos[client].name);
+}
+
 enum request_state {
 	PROCESS_REQUEST_KEYS,
 	PROCESS_REQUEST_DONE,
@@ -223,6 +248,8 @@  static int process_request(void)
 	if (!command)
 		die("no command requested");
 
+	check_algorithm(the_repository, &keys);
+
 	command->command(the_repository, &keys, &reader);
 
 	argv_array_clear(&keys);
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index ffb9613885..bcb6453ae3 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -4,13 +4,24 @@  test_description='test protocol v2 server commands'
 
 . ./test-lib.sh
 
+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 &&
@@ -53,7 +65,7 @@  test_expect_success 'request with no command' '
 
 test_expect_success 'request invalid command' '
 	test-tool pkt-line pack >in <<-EOF &&
-	command=foo
+	$(write_command foo)
 	agent=git/test
 	0000
 	EOF
@@ -73,7 +85,7 @@  test_expect_success 'setup some refs and tags' '
 
 test_expect_success 'basics of ls-refs' '
 	test-tool pkt-line pack >in <<-EOF &&
-	command=ls-refs
+	$(write_command ls-refs)
 	0000
 	EOF
 
@@ -95,7 +107,7 @@  test_expect_success 'basics of ls-refs' '
 
 test_expect_success 'basic ref-prefixes' '
 	test-tool pkt-line pack >in <<-EOF &&
-	command=ls-refs
+	$(write_command ls-refs)
 	0001
 	ref-prefix refs/heads/master
 	ref-prefix refs/tags/one
@@ -115,7 +127,7 @@  test_expect_success 'basic ref-prefixes' '
 
 test_expect_success 'refs/heads prefix' '
 	test-tool pkt-line pack >in <<-EOF &&
-	command=ls-refs
+	$(write_command ls-refs)
 	0001
 	ref-prefix refs/heads/
 	0000
@@ -135,7 +147,7 @@  test_expect_success 'refs/heads prefix' '
 
 test_expect_success 'peel parameter' '
 	test-tool pkt-line pack >in <<-EOF &&
-	command=ls-refs
+	$(write_command ls-refs)
 	0001
 	peel
 	ref-prefix refs/tags/
@@ -156,7 +168,7 @@  test_expect_success 'peel parameter' '
 
 test_expect_success 'symrefs parameter' '
 	test-tool pkt-line pack >in <<-EOF &&
-	command=ls-refs
+	$(write_command ls-refs)
 	0001
 	symrefs
 	ref-prefix refs/heads/
@@ -177,7 +189,7 @@  test_expect_success 'symrefs parameter' '
 
 test_expect_success 'sending server-options' '
 	test-tool pkt-line pack >in <<-EOF &&
-	command=ls-refs
+	$(write_command ls-refs)
 	server-option=hello
 	server-option=world
 	0001
@@ -199,7 +211,7 @@  test_expect_success 'unexpected lines are not allowed in fetch request' '
 	git init server &&
 
 	test-tool pkt-line pack >in <<-EOF &&
-	command=fetch
+	$(write_command fetch)
 	0001
 	this-is-not-a-command
 	0000