Re: [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities
On Fri, May 29, 2015 at 12:39:35PM -0700, Stefan Beller wrote: I think this is the reverse case of next_capabilities in the upload-pack side, so I'll make the reverse suggestion. :) Would it make things nicer if both v1 and v2 parsed the capabilities into a string_list? Ok, I'll do that. Though this makes future enhancements a bit uneasy. Say we want to transport a message by the server admins, this might be the right place to do. if (starts_with(message)) fprintf(stderr, Of course we can later add this in the future, but it would break older clients (clients as of this patch series). That's why I like the idea of adding a prefix here. Maybe just a c: as an abbreviation for capability. I don't understand how that breaks existing clients. Under your scheme, the older client says message? That does not start with capability:, so I must ignore it. Without the capability: flag, it becomes message? I do not know that type, so I must ignore it. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities
On Tue, May 26, 2015 at 11:45 PM, Jeff King p...@peff.net wrote:
On Tue, May 26, 2015 at 03:01:10PM -0700, Stefan Beller wrote:
+void get_remote_capabilities(int in, char *src_buf, size_t src_len)
+{
+ struct strbuf capabilities_string = STRBUF_INIT;
+ for (;;) {
+ int len;
+ char *line = 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(0);
+
+ if (!len)
+ break;
+
+ if (len 4 skip_prefix(line, ERR , arg))
+ die(remote error: %s, arg);
+
+ if (starts_with(line, capability:)) {
+ strbuf_addstr(capabilities_string, line +
strlen(capability:));
+ strbuf_addch(capabilities_string, ' ');
+ }
+ }
I think this is the reverse case of next_capabilities in the upload-pack
side, so I'll make the reverse suggestion. :) Would it make things nicer
if both v1 and v2 parsed the capabilities into a string_list?
Ok, I'll do that. Though this makes future enhancements a bit uneasy.
Say we want to transport a message by the server admins, this might be
the right place to do.
if (starts_with(message))
fprintf(stderr,
Of course we can later add this in the future, but it would break older
clients (clients as of this patch series). That's why I like the idea of
adding a prefix here. Maybe just a c: as an abbreviation for capability.
But now making it short and concise makes it painful in the future when
we want to transport anything else apart from capabilities in this phase
of the protocol exchange. Of course we could have a capability server-message
indicating that after the capabilities we'll have a dedicated message to be
displayed to the user.
Yeah well that should do.
I'll just parse in a string_list for now.
+ free(server_capabilities);
+ server_capabilities = xmalloc(capabilities_string.len + 1);
+ server_capabilities = strbuf_detach(capabilities_string, NULL);
Is this xmalloc line left over? The strbuf_detach will write over it.
No, I wasn't reading the fine documentation and assuming things I
should not.
+ strbuf_release(capabilities_string);
No need to release if we just detached.
+int request_capabilities(int out)
+{
+ fprintf(stderr, request_capabilities\n);
Debug cruft, I presume.
+ // todo: send our capabilities
+ packet_write(out, capability:foo);
No C99 comments. But I think this is just a placeholder. :)
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities
On Wed, May 27, 2015 at 2:50 AM, Jeff King p...@peff.net wrote: On Tue, May 26, 2015 at 11:25:05PM -0400, Eric Sunshine wrote: + 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(0); + + if (!len) + break; + + if (len 4 skip_prefix(line, ERR , arg)) The 'len 4' check is needed because there's no guarantee that 'line' is NUL-terminated. Correct? I think this was just blindly copied from get_remote_heads(). And I think that code was being overly paranoid. Ever since f3a3214 (Make send/receive-pack be closer to doing something interesting, 2005-06-29), the pkt-line reader will add an extra NUL to the buffer to ease cases like this. Thanks. I had started digging into packet_read() to determine whether it guaranteed NUL-termination, but didn't get far enough to decide. I agree that if NUL-termination is guaranteed, then the 'len 4' check is superfluous (and confusing, which is why it caught my attention in the first place). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities
On Wed, May 27, 2015 at 01:19:39PM -0400, Eric Sunshine wrote: The 'len 4' check is needed because there's no guarantee that 'line' is NUL-terminated. Correct? I think this was just blindly copied from get_remote_heads(). And I think that code was being overly paranoid. Ever since f3a3214 (Make send/receive-pack be closer to doing something interesting, 2005-06-29), the pkt-line reader will add an extra NUL to the buffer to ease cases like this. Thanks. I had started digging into packet_read() to determine whether it guaranteed NUL-termination, but didn't get far enough to decide. I agree that if NUL-termination is guaranteed, then the 'len 4' check is superfluous (and confusing, which is why it caught my attention in the first place). Yeah, agreed that it should be cleaned up. Interestingly, if you dig on that line, I've touched it several times myself and never noticed this. :) -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities
On Tue, May 26, 2015 at 03:01:10PM -0700, Stefan Beller wrote:
+void get_remote_capabilities(int in, char *src_buf, size_t src_len)
+{
+ struct strbuf capabilities_string = STRBUF_INIT;
+ for (;;) {
+ int len;
+ char *line = 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(0);
+
+ if (!len)
+ break;
+
+ if (len 4 skip_prefix(line, ERR , arg))
+ die(remote error: %s, arg);
+
+ if (starts_with(line, capability:)) {
+ strbuf_addstr(capabilities_string, line +
strlen(capability:));
+ strbuf_addch(capabilities_string, ' ');
+ }
+ }
I think this is the reverse case of next_capabilities in the upload-pack
side, so I'll make the reverse suggestion. :) Would it make things nicer
if both v1 and v2 parsed the capabilities into a string_list?
+ free(server_capabilities);
+ server_capabilities = xmalloc(capabilities_string.len + 1);
+ server_capabilities = strbuf_detach(capabilities_string, NULL);
Is this xmalloc line left over? The strbuf_detach will write over it.
+ strbuf_release(capabilities_string);
No need to release if we just detached.
+int request_capabilities(int out)
+{
+ fprintf(stderr, request_capabilities\n);
Debug cruft, I presume.
+ // todo: send our capabilities
+ packet_write(out, capability:foo);
No C99 comments. But I think this is just a placeholder. :)
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities
On Tue, May 26, 2015 at 11:25:05PM -0400, Eric Sunshine wrote: + 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(0); + + if (!len) + break; + + if (len 4 skip_prefix(line, ERR , arg)) The 'len 4' check is needed because there's no guarantee that 'line' is NUL-terminated. Correct? I think this was just blindly copied from get_remote_heads(). And I think that code was being overly paranoid. Ever since f3a3214 (Make send/receive-pack be closer to doing something interesting, 2005-06-29), the pkt-line reader will add an extra NUL to the buffer to ease cases like this. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities
On Tue, May 26, 2015 at 6:01 PM, Stefan Beller sbel...@google.com wrote:
Instead of calling get_remote_heads as a first command during the
protocol exchange, we need to have fine grained control over the
capability negotiation in version 2 of the protocol.
Introduce get_remote_capabilities, which will just listen to
capabilities of the remote and request_capabilities which will
tell the selection of capabilities to the remote.
Signed-off-by: Stefan Beller sbel...@google.com
---
diff --git a/connect.c b/connect.c
index c0144d8..bb17618 100644
--- a/connect.c
+++ b/connect.c
@@ -105,8 +105,54 @@ static void annotate_refs_with_symref_info(struct ref
*ref)
string_list_clear(symref, 0);
}
+void get_remote_capabilities(int in, char *src_buf, size_t src_len)
+{
+ struct strbuf capabilities_string = STRBUF_INIT;
+ for (;;) {
+ int len;
+ char *line = 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(0);
+
+ if (!len)
+ break;
+
+ if (len 4 skip_prefix(line, ERR , arg))
The 'len 4' check is needed because there's no guarantee that 'line'
is NUL-terminated. Correct?
+ die(remote error: %s, arg);
+
+ if (starts_with(line, capability:)) {
+ strbuf_addstr(capabilities_string, line +
strlen(capability:));
skip_prefix() maybe?
+ strbuf_addch(capabilities_string, ' ');
+ }
+ }
+ free(server_capabilities);
+ server_capabilities = xmalloc(capabilities_string.len + 1);
+ server_capabilities = strbuf_detach(capabilities_string, NULL);
So, you're allocating a buffer for server_capabilities and then
immediately throwing away (leaking) that buffer. Seems fishy.
+ strbuf_release(capabilities_string);
Not outright incorrect, but you've just detached capabilities_string's
buffer, so releasing it doesn't buy anything.
+}
+
+int request_capabilities(int out)
+{
+ fprintf(stderr, request_capabilities\n);
+ // todo: send our capabilities
+ packet_write(out, capability:foo);
+ packet_flush(out);
+}
+
/*
- * Read all the refs from the other end
+ * Read all the refs from the other end,
+ * in is a file descriptor input stream
+ * src_buf and src_len may be an alternative way to specify the input.
The bit about src_buf and src_len conveys little or nothing. After
reading it, I'm as clueless to their purpose as I would be if they
were undocumented.
+ * list is the output of refs
+ * extra_have is a list to store information learned about sha1 the other
side has.
+ * shallow_points
'shallow_points' what?
*/
struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
struct ref **list, unsigned int flags,
diff --git a/remote.h b/remote.h
index 04e2310..7381ddf 100644
--- a/remote.h
+++ b/remote.h
@@ -146,6 +146,9 @@ int check_ref_type(const struct ref *ref, int flags);
void free_refs(struct ref *ref);
struct sha1_array;
+
+extern void get_remote_capabilities(int in, char *src_buf, size_t src_len);
+extern int request_capabilities(int out);
extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
struct ref **list, unsigned int flags,
struct sha1_array *extra_have,
--
2.4.1.345.gab207b6.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities
Instead of calling get_remote_heads as a first command during the
protocol exchange, we need to have fine grained control over the
capability negotiation in version 2 of the protocol.
Introduce get_remote_capabilities, which will just listen to
capabilities of the remote and request_capabilities which will
tell the selection of capabilities to the remote.
Signed-off-by: Stefan Beller sbel...@google.com
---
connect.c | 48 +++-
remote.h | 3 +++
2 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/connect.c b/connect.c
index c0144d8..bb17618 100644
--- a/connect.c
+++ b/connect.c
@@ -105,8 +105,54 @@ static void annotate_refs_with_symref_info(struct ref *ref)
string_list_clear(symref, 0);
}
+void get_remote_capabilities(int in, char *src_buf, size_t src_len)
+{
+ struct strbuf capabilities_string = STRBUF_INIT;
+ for (;;) {
+ int len;
+ char *line = 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(0);
+
+ if (!len)
+ break;
+
+ if (len 4 skip_prefix(line, ERR , arg))
+ die(remote error: %s, arg);
+
+ if (starts_with(line, capability:)) {
+ strbuf_addstr(capabilities_string, line +
strlen(capability:));
+ strbuf_addch(capabilities_string, ' ');
+ }
+ }
+ free(server_capabilities);
+ server_capabilities = xmalloc(capabilities_string.len + 1);
+ server_capabilities = strbuf_detach(capabilities_string, NULL);
+
+ strbuf_release(capabilities_string);
+}
+
+int request_capabilities(int out)
+{
+ fprintf(stderr, request_capabilities\n);
+ // todo: send our capabilities
+ packet_write(out, capability:foo);
+ packet_flush(out);
+}
+
/*
- * Read all the refs from the other end
+ * Read all the refs from the other end,
+ * in is a file descriptor input stream
+ * src_buf and src_len may be an alternative way to specify the input.
+ * list is the output of refs
+ * extra_have is a list to store information learned about sha1 the other side
has.
+ * shallow_points
*/
struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
struct ref **list, unsigned int flags,
diff --git a/remote.h b/remote.h
index 04e2310..7381ddf 100644
--- a/remote.h
+++ b/remote.h
@@ -146,6 +146,9 @@ int check_ref_type(const struct ref *ref, int flags);
void free_refs(struct ref *ref);
struct sha1_array;
+
+extern void get_remote_capabilities(int in, char *src_buf, size_t src_len);
+extern int request_capabilities(int out);
extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
struct ref **list, unsigned int flags,
struct sha1_array *extra_have,
--
2.4.1.345.gab207b6.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
