Re: [PATCH 1.5/8] connect: die when a capability line comes after a ref

On 09/20, Jeff King wrote:

On Wed, Sep 20, 2017 at 11:48:32AM -0700, Brandon Williams wrote:

quoted

Commit eb398797c (connect: advertized capability is not a ref,
2016-09-09) taught 'get_remote_heads()' to recognize that the
'capabilities^{}' line isn't a ref but required that the
'capabilities^{}' line came during the first response from the server.
A future patch will introduce a version string sent by the server during
its first response which can then cause a client to unnecessarily die if
a 'capabilities^{}' line sent as the first ref.

Teach 'get_remote_heads()' to instead die if a 'capabilities^{}' line is
sent after a ref.

Hmm. I think I understand why you'd want this loosening. But why are we
sending a version line to a client that we don't know is speaking v2?
IOW, shouldn't we be reporting the version to the client in the normal
capabilities when we don't know for sure that they can handle the new
field? Otherwise we're breaking existing clients.

The client requested the version, this is the servers response.  So
older clients shouldn't be broken because they wouldn't be requesting
the newer versions.

Or is this only for v2 clients, and we've changed the protocol but
get_remote_heads() just needs to be updated, too?

A client which didn't request protocol v1 (I'm calling the current
protocol v0, and v1 is just v0 with the initial response from the server
containing a version string) should not receive a version string in the
initial response.  The problem is that when introducing the version
string to protocol version 1, I didn't want to have to do a huge
refactoring of ALL of the current transport code so I stuck the version
check in get_remote_heads() since v1 is exactly the same as v0, except
for the first line from the server.

When we introduce v2, I'm sure we'll have to do more refactoring to
separate out the logic for the different versions.

quoted

diff --git a/connect.c b/connect.c
index df56c0cbf..af5096ec6 100644
--- a/connect.c
+++ b/connect.c

@@ -124,10 +124,11 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	 * response does not necessarily mean an ACL problem, though.
 	 */
 	int saw_response;
+	int seen_ref;
 	int got_dummy_ref_with_capabilities_declaration = 0;

 	*list = NULL;
-	for (saw_response = 0; ; saw_response = 1) {
+	for (saw_response = 0, seen_ref = 0; ; saw_response = 1) {

If we're not going to update it in the right-hand side of the for-loop,
should we perhaps not be initializing it in the left-hand side? I.e.,
can we just do:

  seen_ref = 0;

above the loop, like we initialize "list"?

(For that matter, could we just be checking whether *list is NULL?)

True, that would probably be the better way to do this.

quoted

@@ -165,6 +166,8 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,

 		name_len = strlen(name);
 		if (len != name_len + GIT_SHA1_HEXSZ + 1) {
+			if (seen_ref)
+				; /* NEEDSWORK: Error out for multiple capabilities lines? */
 			free(server_capabilities);
 			server_capabilities = xstrdup(name + name_len + 1);
 		}

Interesting question. Probably it would be fine to. Coincidentally I ran
across a similar case. It seems that upload-pack will read multiple
capabilities lines back from the client. I.e., if it gets:

  want 1234abcd... foo
  want 5678abcd... bar

then it will turn on both the "foo" and "bar" capabilities. I'm pretty
sure this is unintended, and is somewhat counter to the way that clients
handle multiple lines (which is to forget the old line and respect only
the new one, as shown in the quoted hunk).

I wonder if we should be outlawing extra capabilities in both
directions. I don't _think_ we've ever relied on that working, and I
don't have much sympathy for any 3rd-party implementation that does
(though I doubt that any exists).

That tangent aside, I do this hunk is kind of orthogonal to the point of
your patch. We're talking about potential _tightening_ here, whereas the
point of your patch is loosening. And it's not clear to me what we want
to tighten:

  - should capabilities come as part of the first response, even if we
    have no refs? In which case we really want "if (saw_response)" here.

  - should they came as part of the first ref (or pseudo-ref), in which
    case "if (seen_ref)" is the right thing.

  - should we loosen it to complaining when there are multiple
    capabilities sent. In which case "if (server_capabilities)" is the
    right thing.

I'm not sure which we'd want, but it really seems like a separate topic
that should be explored on top.

I wasn't sure either, which is why I added the comment to prod
discussion.  I agree that is is orthogonal to this series so I'll most
likely drop it, as it doesn't help with the protocol transition
discussion.

-- 
Brandon Williams