Re: [WIP 08/15] connect: discover protocol version outside of get_remote_heads

Brandon Williams [off-list ref] writes:

We see quite a few hunks just like this one appear in this patch.
The repetition is a bit disturbing.  I wonder if we want a wrapper
around the "reader-init && discover-version && return an error if
the protocol version is not known" dance.  That would allow us to
write this part of the code like so:

	struct packet_reader reader;

	if (connection_preamble(&reader, fd[0]))
		die("unknown protocol version");
	get_remote_heads(&reader, &ref, 0, NULL, &shallow);

or something like that.

By the way, is that really a BUG()?  Getting a connection and the
other end declaring a protocol version you do not yet understand is
not a bug in your program---it is a normal runtime error, no?

On 12/07, Junio C Hamano wrote:

Brandon Williams [off-list ref] writes:

quoted

@@ -193,7 +195,17 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 		if (!conn)
 			return args.diag_url ? 0 : 1;
 	}
-	get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
+
+	packet_reader_init(&reader, fd[0], NULL, 0);
+
+	switch (discover_version(&reader)) {
+	case protocol_v1:
+	case protocol_v0:
+		get_remote_heads(&reader, &ref, 0, NULL, &shallow);
+		break;
+	case protocol_unknown_version:
+		BUG("unknown protocol version");
+	}

We see quite a few hunks just like this one appear in this patch.
The repetition is a bit disturbing.  I wonder if we want a wrapper
around the "reader-init && discover-version && return an error if
the protocol version is not known" dance.  That would allow us to
write this part of the code like so:

	struct packet_reader reader;

	if (connection_preamble(&reader, fd[0]))
		die("unknown protocol version");
	get_remote_heads(&reader, &ref, 0, NULL, &shallow);

or something like that.

By the way, is that really a BUG()?  Getting a connection and the
other end declaring a protocol version you do not yet understand is
not a bug in your program---it is a normal runtime error, no?

While we could wrap the preamble into a function it sort of defeats the
purpose since you want to be able to call different functions based on
the protocol version you're speaking.  That way you can have hard
separations between the code paths which operate on v0/v1 and v2.

As for that case being a BUG, yes it should be a BUG.  the
discover_version function won't ever return a protocol_unknown_version
value.  Its only there to make sure the switch cases are exhaustive and
cover all the enum values.  That does bring up a good point though.
This error should be handled as a run-time error and not a BUG in
discover_version, which I'll fix.


-- 
Brandon Williams