Re: [PATCH v3] connect: in ref advertisement, shallows are last

Junio C Hamano [off-list ref] writes:

Jonathan Tan [off-list ref] writes:

quoted

Currently, get_remote_heads() parses the ref advertisement in one loop,
allowing refs and shallow lines to intersperse, despite this not being
allowed by the specification. Refactor get_remote_heads() to use two
loops instead, enforcing that refs come first, and then shallows.

This also makes it easier to teach get_remote_heads() to interpret other
lines in the ref advertisement, which will be done in a subsequent
patch.

Sounds sensible.  This still replaces the earlier 1.5?

Well, it does, but it also invalidates how the new "pick the version
offered and used" feature is integrated to this callchain.  I guess
we'd need a new "we are now expecting the version info" state in a
patch to replace "connect: teach client to recognize v1 server
response".

quoted

+static int process_ref(int *state, int len, struct ref ***list,
+		       unsigned int flags, struct oid_array *extra_have)
+{
+	struct object_id old_oid;
+	char *name;
+	int name_len;
+
+	if (len < GIT_SHA1_HEXSZ + 2 ||
+	    get_oid_hex(packet_buffer, &old_oid) ||
+	    packet_buffer[GIT_SHA1_HEXSZ] != ' ') {
+		*state = EXPECTING_SHALLOW;
+		return 0;
+	}
+
+	name = packet_buffer + GIT_SHA1_HEXSZ + 1;
+	name_len = strlen(name);
+	if (*state == EXPECTING_REF_WITH_CAPABILITIES &&
+	    len != name_len + GIT_SHA1_HEXSZ + 1) {
+		free(server_capabilities);

Is this free() still needed?  After hitting this block, you'd set
*state to EXPECTING_REF before you return, so nobody would set
server_capabilities by hitting this block twice, and an attempt to
do so will hit the die("unexpected cap") below, no?

Or it may be a signal that this patch tightens it too much and
breaks older or third-party implementations of the other side that
can emit more than one refs with capability advertisement?

quoted

+		server_capabilities = xstrdup(name + name_len + 1);
+	} else if (*state == EXPECTING_REF) {
+		if (len != name_len + GIT_SHA1_HEXSZ + 1)
+			die("unexpected capabilities after ref name");
+	}
+	...
+	}
+	*state = EXPECTING_REF;
+	return 1;
+}

quoted

@@ -123,76 +208,26 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	 * willing to talk to us.  A hang-up before seeing any
 	 * response does not necessarily mean an ACL problem, though.
 	 */
-	int saw_response;
-	int got_dummy_ref_with_capabilities_declaration = 0;
+	int responded = 0;
+	int len;
+	int state = EXPECTING_REF_WITH_CAPABILITIES;
 
 	*list = NULL;

quoted

+	while ((len = read_remote_ref(in, &src_buf, &src_len, &responded))) {
+		switch (state) {
+		case EXPECTING_REF_WITH_CAPABILITIES:
+		case EXPECTING_REF:
+			if (process_ref(&state, len, &list, flags, extra_have))
+				break;
+			/* fallthrough */

OK.  This fallthrough is because expecting-ref is really expecting
ref or shallow and once we see a shallow, we no longer expect ref
and expect only shallow.  So from that point of view, an assignment
to set state to EXPECTING_SHALLOW could happen here, not inside
process_ref.  I mention this because in general, passing state
around and let it be updated in helper functions would make the
state transition harder to follow, not easier, even though
refactoring the processing needed in different stages into helper
functions like this patch does ought to make it easier to see by
shrinking the outer loop (i.e. this one) that controls the whole
process.

I think if we split process_ref() further into two, then we no
longer need to pass &state to that function?  We start this loop
with "expecting the dummy ref (or other)" state, have a new
process_dummy_ref() function check if we got "capabilities^{}" thing
and do its thing if that is the case (otherwise we fall through to
the call to process_ref(), just like the above code falls through to
call process_shallow() when it realizes what it got is not a ref),
and after the first call to process_dummy_ref() we'd be in the
"expecting ref (or other)" state---and the state transition can
happen in this caller, not in process_dummy_ref() or process_ref().

Inside process_dummy_ref() and process_ref(), there would be a call
to the same helper that notices and extracts the server capability
and stores it (or barfs against the second line that advertises the
capability, by noticing that server_capabilities is not NULL).

Wouldn't that make the presentation of the state machine cleaner?

On 09/22, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

Jonathan Tan [off-list ref] writes:

quoted

Currently, get_remote_heads() parses the ref advertisement in one loop,
allowing refs and shallow lines to intersperse, despite this not being
allowed by the specification. Refactor get_remote_heads() to use two
loops instead, enforcing that refs come first, and then shallows.

This also makes it easier to teach get_remote_heads() to interpret other
lines in the ref advertisement, which will be done in a subsequent
patch.

Sounds sensible.  This still replaces the earlier 1.5?

Well, it does, but it also invalidates how the new "pick the version
offered and used" feature is integrated to this callchain.  I guess
we'd need a new "we are now expecting the version info" state in a
patch to replace "connect: teach client to recognize v1 server
response".

Yeah given we go with this patch, which is probably a better cleanup
than what I attempted, then I would need to change how a client
recognizes a v1 server.  That would probably be easily done by adding a
new state.

I do think that once a v2 protocol rolls around we'll probably have to
do even more refactoring because I don't think we'll want to keep all
the version checking logic in get_remote_heads() for different protocol
versions which may not be interested in a servers ref advertisement, but
that'll be for another time.

quoted

quoted

+static int process_ref(int *state, int len, struct ref ***list,
+		       unsigned int flags, struct oid_array *extra_have)
+{
+	struct object_id old_oid;
+	char *name;
+	int name_len;
+
+	if (len < GIT_SHA1_HEXSZ + 2 ||
+	    get_oid_hex(packet_buffer, &old_oid) ||
+	    packet_buffer[GIT_SHA1_HEXSZ] != ' ') {
+		*state = EXPECTING_SHALLOW;
+		return 0;
+	}
+
+	name = packet_buffer + GIT_SHA1_HEXSZ + 1;
+	name_len = strlen(name);
+	if (*state == EXPECTING_REF_WITH_CAPABILITIES &&
+	    len != name_len + GIT_SHA1_HEXSZ + 1) {
+		free(server_capabilities);

Is this free() still needed?  After hitting this block, you'd set
*state to EXPECTING_REF before you return, so nobody would set
server_capabilities by hitting this block twice, and an attempt to
do so will hit the die("unexpected cap") below, no?

Or it may be a signal that this patch tightens it too much and
breaks older or third-party implementations of the other side that
can emit more than one refs with capability advertisement?

quoted

quoted

+		server_capabilities = xstrdup(name + name_len + 1);
+	} else if (*state == EXPECTING_REF) {
+		if (len != name_len + GIT_SHA1_HEXSZ + 1)
+			die("unexpected capabilities after ref name");
+	}
+	...
+	}
+	*state = EXPECTING_REF;
+	return 1;
+}

quoted

quoted

@@ -123,76 +208,26 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	 * willing to talk to us.  A hang-up before seeing any
 	 * response does not necessarily mean an ACL problem, though.
 	 */
-	int saw_response;
-	int got_dummy_ref_with_capabilities_declaration = 0;
+	int responded = 0;
+	int len;
+	int state = EXPECTING_REF_WITH_CAPABILITIES;
 
 	*list = NULL;

quoted

quoted

+	while ((len = read_remote_ref(in, &src_buf, &src_len, &responded))) {
+		switch (state) {
+		case EXPECTING_REF_WITH_CAPABILITIES:
+		case EXPECTING_REF:
+			if (process_ref(&state, len, &list, flags, extra_have))
+				break;
+			/* fallthrough */

OK.  This fallthrough is because expecting-ref is really expecting
ref or shallow and once we see a shallow, we no longer expect ref
and expect only shallow.  So from that point of view, an assignment
to set state to EXPECTING_SHALLOW could happen here, not inside
process_ref.  I mention this because in general, passing state
around and let it be updated in helper functions would make the
state transition harder to follow, not easier, even though
refactoring the processing needed in different stages into helper
functions like this patch does ought to make it easier to see by
shrinking the outer loop (i.e. this one) that controls the whole
process.

I think if we split process_ref() further into two, then we no
longer need to pass &state to that function?  We start this loop
with "expecting the dummy ref (or other)" state, have a new
process_dummy_ref() function check if we got "capabilities^{}" thing
and do its thing if that is the case (otherwise we fall through to
the call to process_ref(), just like the above code falls through to
call process_shallow() when it realizes what it got is not a ref),
and after the first call to process_dummy_ref() we'd be in the
"expecting ref (or other)" state---and the state transition can
happen in this caller, not in process_dummy_ref() or process_ref().

Inside process_dummy_ref() and process_ref(), there would be a call
to the same helper that notices and extracts the server capability
and stores it (or barfs against the second line that advertises the
capability, by noticing that server_capabilities is not NULL).

Wouldn't that make the presentation of the state machine cleaner?

I mentioned this when looking at v2 of this patch, that it would
probably be cleaner to remove passing the state variable around the
place and updating it inside a helper function.  It would just make the
logic simpler to follow if 'state' is updated directly instead of
indirectly.

-- 
Brandon Williams

Currently, get_remote_heads() parses the ref advertisement in one loop,
allowing refs and shallow lines to intersperse, despite this not being
allowed by the specification. Refactor get_remote_heads() to use two
loops instead, enforcing that refs come first, and then shallows.

This also makes it easier to teach get_remote_heads() to interpret other
lines in the ref advertisement, which will be done in a subsequent
patch.

As part of this change, this patch interprets capabilities only on the
first line in the ref advertisement, ignoring all others.

Signed-off-by: Jonathan Tan <redacted>
---
I've updated state transitions to occur in get_remote_heads() instead,
as suggested. I didn't want to do that previously because each step in
the state machine needed to communicate if (i) the line is "consumed"
and (ii) the state needed to be advanced, but with Junio's suggestion to
reorganize the methods, that is no longer true.

As Junio said, the free(server_capabilities) can be removed.

As for whether how capabilities on subsequent lines are handled, I think
it's better to ignore them - they are behind NULs, after all.

Yes, "connect: teach client to recognize v1 server response" will need
to be modified.

This change does have the side effect that if the server sends a ref
advertisement with "shallow"s only (and no refs), things will still
work, and the server can even tuck capabilities on the first "shallow"
line. I think that's fine, and it does make the client code cleaner.
---
 connect.c | 171 ++++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 105 insertions(+), 66 deletions(-)

diff --git a/connect.c b/connect.c
index 49b28b83b..978d01359 100644
--- a/connect.c
+++ b/connect.c

@@ -11,6 +11,7 @@
 #include "string-list.h"
 #include "sha1-array.h"
 #include "transport.h"
+#include "strbuf.h"
 
 static char *server_capabilities;
 static const char *parse_feature_value(const char *, const char *, int *);

@@ -107,6 +108,86 @@ static void annotate_refs_with_symref_info(struct ref *ref)
 	string_list_clear(&symref, 0);
 }
 
+/*
+ * Read one line of a server's ref advertisement into packet_buffer.
+ */
+static int read_remote_ref(int in, char **src_buf, size_t *src_len,
+			   int *responded)
+{
+	int len = packet_read(in, src_buf, src_len,
+			      packet_buffer, sizeof(packet_buffer),
+			      PACKET_READ_GENTLE_ON_EOF |
+			      PACKET_READ_CHOMP_NEWLINE);
+	const char *arg;
+	if (len < 0)
+		die_initial_contact(*responded);
+	if (len > 4 && skip_prefix(packet_buffer, "ERR ", &arg))
+		die("remote error: %s", arg);
+
+	*responded = 1;
+
+	return len;
+}
+
+#define EXPECTING_FIRST_REF 0
+#define EXPECTING_REF 1
+#define EXPECTING_SHALLOW 2
+
+static void process_capabilities(int len)
+{
+	int nul_location = strlen(packet_buffer);
+	if (nul_location == len)
+		return;
+	server_capabilities = xstrdup(packet_buffer + nul_location + 1);
+}
+
+static int process_dummy_ref(void)
+{
+	static char *template;
+	if (!template)
+		template = xstrfmt("%040d capabilities^{}", 0);
+	return !strcmp(packet_buffer, template);
+}
+
+static int process_ref(struct ref ***list, unsigned int flags,
+		       struct oid_array *extra_have)
+{
+	struct object_id old_oid;
+	const char *name;
+
+	if (parse_oid_hex(packet_buffer, &old_oid, &name))
+		return 0;
+	if (*name != ' ')
+		return 0;
+	name++;
+
+	if (extra_have && !strcmp(name, ".have")) {
+		oid_array_append(extra_have, &old_oid);
+	} else if (check_ref(name, flags)) {
+		struct ref *ref = alloc_ref(name);
+		oidcpy(&ref->old_oid, &old_oid);
+		**list = ref;
+		*list = &ref->next;
+	}
+	return 1;
+}
+
+static int process_shallow(struct oid_array *shallow_points)
+{
+	const char *arg;
+	struct object_id old_oid;
+
+	if (!skip_prefix(packet_buffer, "shallow ", &arg))
+		return 0;
+
+	if (get_oid_hex(arg, &old_oid))
+		die("protocol error: expected shallow sha-1, got '%s'", arg);
+	if (!shallow_points)
+		die("repository on the other end cannot be shallow");
+	oid_array_append(shallow_points, &old_oid);
+	return 1;
+}
+
 /*
  * Read all the refs from the other end
  */

@@ -123,76 +204,34 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	 * willing to talk to us.  A hang-up before seeing any
 	 * response does not necessarily mean an ACL problem, though.
 	 */
-	int saw_response;
-	int got_dummy_ref_with_capabilities_declaration = 0;
+	int responded = 0;
+	int len;
+	int state = EXPECTING_FIRST_REF;
 
 	*list = NULL;
-	for (saw_response = 0; ; saw_response = 1) {
-		struct ref *ref;
-		struct object_id old_oid;
-		char *name;
-		int len, name_len;
-		char *buffer = packet_buffer;
-		const char *arg;
-
-		len = packet_read(in, &src_buf, &src_len,
-				  packet_buffer, sizeof(packet_buffer),
-				  PACKET_READ_GENTLE_ON_EOF |
-				  PACKET_READ_CHOMP_NEWLINE);
-		if (len < 0)
-			die_initial_contact(saw_response);
-
-		if (!len)
-			break;
 
-		if (len > 4 && skip_prefix(buffer, "ERR ", &arg))
-			die("remote error: %s", arg);
-
-		if (len == GIT_SHA1_HEXSZ + strlen("shallow ") &&
-			skip_prefix(buffer, "shallow ", &arg)) {
-			if (get_oid_hex(arg, &old_oid))
-				die("protocol error: expected shallow sha-1, got '%s'", arg);
-			if (!shallow_points)
-				die("repository on the other end cannot be shallow");
-			oid_array_append(shallow_points, &old_oid);
-			continue;
-		}
-
-		if (len < GIT_SHA1_HEXSZ + 2 || get_oid_hex(buffer, &old_oid) ||
-			buffer[GIT_SHA1_HEXSZ] != ' ')
-			die("protocol error: expected sha/ref, got '%s'", buffer);
-		name = buffer + GIT_SHA1_HEXSZ + 1;
-
-		name_len = strlen(name);
-		if (len != name_len + GIT_SHA1_HEXSZ + 1) {
-			free(server_capabilities);
-			server_capabilities = xstrdup(name + name_len + 1);
-		}
-
-		if (extra_have && !strcmp(name, ".have")) {
-			oid_array_append(extra_have, &old_oid);
-			continue;
-		}
-
-		if (!strcmp(name, "capabilities^{}")) {
-			if (saw_response)
-				die("protocol error: unexpected capabilities^{}");
-			if (got_dummy_ref_with_capabilities_declaration)
-				die("protocol error: multiple capabilities^{}");
-			got_dummy_ref_with_capabilities_declaration = 1;
-			continue;
+	while ((len = read_remote_ref(in, &src_buf, &src_len, &responded))) {
+		switch (state) {
+		case EXPECTING_FIRST_REF:
+			process_capabilities(len);
+			if (process_dummy_ref()) {
+				state = EXPECTING_SHALLOW;
+				break;
+			}
+			state = EXPECTING_REF;
+			/* fallthrough */
+		case EXPECTING_REF:
+			if (process_ref(&list, flags, extra_have))
+				break;
+			state = EXPECTING_SHALLOW;
+			/* fallthrough */
+		case EXPECTING_SHALLOW:
+			if (process_shallow(shallow_points))
+				break;
+			die("protocol error: unexpected '%s'", packet_buffer);
+		default:
+			die("unexpected state %d", state);
 		}
-
-		if (!check_ref(name, flags))
-			continue;
-
-		if (got_dummy_ref_with_capabilities_declaration)
-			die("protocol error: unexpected ref after capabilities^{}");
-
-		ref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);
-		oidcpy(&ref->old_oid, &old_oid);
-		*list = ref;
-		list = &ref->next;
 	}
 
 	annotate_refs_with_symref_info(*orig_list);

-- 
2.14.1.728.g20a5b67d5.dirty

On 09/22, Jonathan Tan wrote:

It may make more sense to not rely on accessing a global buffer here
directly and instead pass in the buff you're working on, much like your
are doing with len.

+	if (nul_location == len)
+		return;
+	server_capabilities = xstrdup(packet_buffer + nul_location + 1);
+}
+
+static int process_dummy_ref(void)
+{
+	static char *template;
+	if (!template)
+		template = xstrfmt("%040d capabilities^{}", 0);

I'm not the biggest fan of dynamically allocating this and then using it
to compare.  Maybe we can check to make sure that the oid matches the
null_oid and that the name matches the "capabilities^{}" string?  That
way you can avoid the allocation?

+	return !strcmp(packet_buffer, template);
+}
+
+static int process_ref(struct ref ***list, unsigned int flags,
+		       struct oid_array *extra_have)

So from comparing this to the current code it doesn't look like there is
a check in 'process_ref' that ensures that a 'capabilities^{}' line
doesn't show up after a normal ref, or am I missing something?

Looks much cleaner, thanks!

 		}
-
-		if (!check_ref(name, flags))
-			continue;
-
-		if (got_dummy_ref_with_capabilities_declaration)
-			die("protocol error: unexpected ref after capabilities^{}");
-
-		ref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);
-		oidcpy(&ref->old_oid, &old_oid);
-		*list = ref;
-		list = &ref->next;
 	}
 
 	annotate_refs_with_symref_info(*orig_list);
-- 
2.14.1.728.g20a5b67d5.dirty

-- 
Brandon Williams

On Fri, 22 Sep 2017 14:01:04 -0700
Brandon Williams [off-list ref] wrote:

quoted

+static void process_capabilities(int len)
+{
+	int nul_location = strlen(packet_buffer);

It may make more sense to not rely on accessing a global buffer here
directly and instead pass in the buff you're working on, much like your
are doing with len.

I wanted to preserve the existing code's behavior of using the global
buffer, and it didn't make sense for me to alias it (like the existing
code does).

I pass len in because I need to read beyond NUL.

I'm not the biggest fan of dynamically allocating this and then using it
to compare.  Maybe we can check to make sure that the oid matches the
null_oid and that the name matches the "capabilities^{}" string?  That
way you can avoid the allocation?

The dynamic allocation happens only once per process, since it is
static. To check the oid matches null_oid, I would have to parse it
first, and that seemed unnecessary.

Ideally I would just check again "000...000 capabilities^{}", but
writing it in source code would be error-prone, I think.

quoted

+static int process_ref(struct ref ***list, unsigned int flags,
+		       struct oid_array *extra_have)

So from comparing this to the current code it doesn't look like there is
a check in 'process_ref' that ensures that a 'capabilities^{}' line
doesn't show up after a normal ref, or am I missing something?

Ah...yes, you're right. I'll fix this by adding a check in
process_ref().

This is getting more complicated than I thought, so I'll wait a while
for other comments before sending out an updated patch.