protocol version 2

The current pkt-line API encodes the status of a pkt-line read in the length of the read content. An error is indicated with '-1', a flush with '0' (which can be confusing since a return value of '0' can also indicate an empty pkt-line), and a positive integer for the length of the read content otherwise. This doesn't leave much room for allowing the addition of additional special packets in the future.

To solve this introduce 'packet_read_with_status()' which reads a packet and returns the status of the read encoded as an 'enum packet_status' type. This allows for easily identifying between special and normal packets as well as errors. It also enables easily adding a new special packet in the future.

Signed-off-by Brandon Williams <[email protected]>

---
 pkt-line.c | 55 ++++++++++++++++++++++++++++++++++++++++++-------------
 pkt-line.h | 15 +++++++++++++++
 2 files changed, 57 insertions(+), 13 deletions(-)

pkt-line.c +7 -6

index 2827ca772..8d7cd389f 100644
@@ -280,28 +280,33 @@ static int packet_length(const char *linelen)
        return (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);
 }
 
-int packet_read(int fd, char **src_buf, size_t *src_len,
-               char *buffer, unsigned size, int options)
+enum packet_read_status packet_read_with_status(int fd, char **src_buffer, size_t *src_len,
+                                               char *buffer, unsigned size, int *pktlen,
+                                               int options)
 {
-       int len, ret;
+       int len;
        char linelen[4];
 
-       ret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);
-       if (ret < 0)
-               return ret;
+       if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0)
+               return PACKET_READ_EOF;
+
        len = packet_length(linelen);

pkt-line.c +30 -5

        }
+
        len -= 4;
-       if (len >= size)
+       if ((len < 0) || ((unsigned)len >= size))
                die("protocol error: bad line length %d", len);
-       ret = get_packet_data(fd, src_buf, src_len, buffer, len, options);
-       if (ret < 0)
-               return ret;
+
+       if (get_packet_data(fd, src_buffer, src_len, buffer, len, options) < 0)
+               return PACKET_READ_EOF;
 
        if ((options & PACKET_READ_CHOMP_NEWLINE) &&
            len && buffer[len-1] == '\n')

@@ -309,7 +314,31 @@ int packet_read(int fd, char **src_buf, size_t *src_len,
 
        buffer[len] = 0;
        packet_trace(buffer, len, 0);
-       return len;
+       *pktlen = len;
+       return PACKET_READ_NORMAL;
+}
+
+int packet_read(int fd, char **src_buffer, size_t *src_len,
+               char *buffer, unsigned size, int options)
+{
+       enum packet_read_status status;
+       int pktlen;
+
+       status = packet_read_with_status(fd, src_buffer, src_len,
+                                        buffer, size, &pktlen,
+                                        options);
+       switch (status) {
+       case PACKET_READ_EOF:
+               pktlen = -1;
+               break;
+       case PACKET_READ_NORMAL:
+               break;
+       case PACKET_READ_FLUSH:
+               pktlen = 0;
+               break;
+       }
+
+       return pktlen;
 }
 
 static char *packet_read_line_generic(int fd,

pkt-line.h +15 -1

index 3dad583e2..06c468927 100644
@@ -65,6 +65,21 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
 int packet_read(int fd, char **src_buffer, size_t *src_len, char
                *buffer, unsigned size, int options);
 
+/*
+ * Read a packetized line into a buffer like the 'packet_read()' function but
+ * returns an 'enum packet_read_status' which indicates the status of the read.
+ * The number of bytes read will be assigined to *pktlen if the status of the
+ * read was 'PACKET_READ_NORMAL'.
+ */
+enum packet_read_status {
+       PACKET_READ_EOF = -1,
+       PACKET_READ_NORMAL,
+       PACKET_READ_FLUSH,
+};
+enum packet_read_status packet_read_with_status(int fd, char **src_buffer, size_t *src_len,
+                                               char *buffer, unsigned size, int *pktlen,
+                                               int options);
+
 /*
  * Convenience wrapper for packet_read that is not gentle, and sets the
  * CHOMP_NEWLINE option. The return value is NULL for a flush packet,
--

2.15.1.620.gb9897f4670-goog

Quoted 44 lines · Brandon Williams

The current pkt-line API encodes the status of a pkt-line read in the length of the read content. An error is indicated with '-1', a flush with '0' (which can be confusing since a return value of '0' can also indicate an empty pkt-line), and a positive integer for the length of the read content otherwise. This doesn't leave much room for allowing the addition of additional special packets in the future.

To solve this introduce 'packet_read_with_status()' which reads a packet and returns the status of the read encoded as an 'enum packet_status' type. This allows for easily identifying between special and normal packets as well as errors. It also enables easily adding a new special packet in the future.

Signed-off-by Brandon Williams <[email protected]>

---
 pkt-line.c | 55 ++++++++++++++++++++++++++++++++++++++++++-------------
 pkt-line.h | 15 +++++++++++++++
 2 files changed, 57 insertions(+), 13 deletions(-)

pkt-line.c +7 -6

index 2827ca772..8d7cd389f 100644
@@ -280,28 +280,33 @@ static int packet_length(const char *linelen)
        return (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);
 }
 
-int packet_read(int fd, char **src_buf, size_t *src_len,
-               char *buffer, unsigned size, int options)
+enum packet_read_status packet_read_with_status(int fd, char **src_buffer, size_t *src_len,
+                                               char *buffer, unsigned size, int *pktlen,
+                                               int options)
 {
-       int len, ret;
+       int len;
        char linelen[4];
 
-       ret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);
-       if (ret < 0)
-               return ret;
+       if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0)
+               return PACKET_READ_EOF;
+
        len = packet_length(linelen);

pkt-line.c static int packet_length(const char *linelen) · line 301

Quoted 10 lines

pkt-line.c +5 -2

        if (len < 0)
                die("protocol error: bad line length character: %.4s", linelen);
-       if (!len) {
+
+       if (len == 0) {
                packet_trace("0000", 4, 0);
-               return 0;
+               return PACKET_READ_FLUSH;
+       } else if (len >= 1 && len <= 3) {
+               die("protocol error: bad line length character: %.4s", linelen);

I wonder how much libified code we want here already, maybe we could have PACKET_READ_ERROR as a return value here instead of die()ing. There could also be an option that tells this code to die on error, this reminds me of the repository discovery as well as the refs code, both of which have this pattern.

Currently this series is only upgrading commands that use the network anyway, so I guess die()ing in an ls-remote or fetch is no big deal, but it could
be interesting to keep going once we have more of the partial clone stuff working
(e.g. remote assisted log/blame would want to gracefully fall back instead of die()ing without any useful output, I would think.)

Quoted 78 lines

pkt-line.c +30 -5

        }
+
        len -= 4;
-       if (len >= size)
+       if ((len < 0) || ((unsigned)len >= size))
                die("protocol error: bad line length %d", len);
-       ret = get_packet_data(fd, src_buf, src_len, buffer, len, options);
-       if (ret < 0)
-               return ret;
+
+       if (get_packet_data(fd, src_buffer, src_len, buffer, len, options) < 0)
+               return PACKET_READ_EOF;
 
        if ((options & PACKET_READ_CHOMP_NEWLINE) &&
            len && buffer[len-1] == '\n')

@@ -309,7 +314,31 @@ int packet_read(int fd, char **src_buf, size_t *src_len,
 
        buffer[len] = 0;
        packet_trace(buffer, len, 0);
-       return len;
+       *pktlen = len;
+       return PACKET_READ_NORMAL;
+}
+
+int packet_read(int fd, char **src_buffer, size_t *src_len,
+               char *buffer, unsigned size, int options)
+{
+       enum packet_read_status status;
+       int pktlen;
+
+       status = packet_read_with_status(fd, src_buffer, src_len,
+                                        buffer, size, &pktlen,
+                                        options);
+       switch (status) {
+       case PACKET_READ_EOF:
+               pktlen = -1;
+               break;
+       case PACKET_READ_NORMAL:
+               break;
+       case PACKET_READ_FLUSH:
+               pktlen = 0;
+               break;
+       }
+
+       return pktlen;
 }
 
 static char *packet_read_line_generic(int fd,

pkt-line.h +15 -1

index 3dad583e2..06c468927 100644
@@ -65,6 +65,21 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
 int packet_read(int fd, char **src_buffer, size_t *src_len, char
                *buffer, unsigned size, int options);
 
+/*
+ * Read a packetized line into a buffer like the 'packet_read()' function but
+ * returns an 'enum packet_read_status' which indicates the status of the read.
+ * The number of bytes read will be assigined to *pktlen if the status of the
+ * read was 'PACKET_READ_NORMAL'.
+ */
+enum packet_read_status {
+       PACKET_READ_EOF = -1,
+       PACKET_READ_NORMAL,
+       PACKET_READ_FLUSH,
+};
+enum packet_read_status packet_read_with_status(int fd, char **src_buffer, size_t *src_len,
+                                               char *buffer, unsigned size, int *pktlen,
+                                               int options);
+
 /*
  * Convenience wrapper for packet_read that is not gentle, and sets the
  * CHOMP_NEWLINE option. The return value is NULL for a flush packet,
--

2.15.1.620.gb9897f4670-goog

pkt-line.c +4 -5

-int packet_read(int fd, char **src_buf, size_t *src_len,
-		char *buffer, unsigned size, int options)
+enum packet_read_status packet_read_with_status(int fd, char **src_buffer, size_t *src_len,
+						char *buffer, unsigned size, int *pktlen,
+						int options)
 {
-	int len, ret;
+	int len;
 	char linelen[4];
 
-	ret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);
-	if (ret < 0)

pkt-line.c +3 -3

-	ret = get_packet_data(fd, src_buf, src_len, buffer, len, options);
-	if (ret < 0)
-		return ret;
+
+	if (get_packet_data(fd, src_buffer, src_len, buffer, len, options) < 0)
+		return PACKET_READ_EOF;
 
 	if ((options & PACKET_READ_CHOMP_NEWLINE) &&
 	    len && buffer[len-1] == '\n')

Quoted 12 lines · On Tue, 2 Jan 2018 16:18:03 -0800 Brandon Williams <[email protected]> wrote:

pkt-line.c +4 -5

-int packet_read(int fd, char **src_buf, size_t *src_len,
-		char *buffer, unsigned size, int options)
+enum packet_read_status packet_read_with_status(int fd, char **src_buffer, size_t *src_len,
+						char *buffer, unsigned size, int *pktlen,
+						int options)
 {
-	int len, ret;
+	int len;
 	char linelen[4];
 
-	ret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);
-	if (ret < 0)

pkt-line.c static int packet_length(const char *linelen) · line 297

Quoted 10 lines

pkt-line.c +5 -2

-		return ret;
+	if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0)
+		return PACKET_READ_EOF;
+
 	len = packet_length(linelen);
 	if (len < 0)
 		die("protocol error: bad line length character: %.4s", linelen);
-	if (!len) {
+
+	if (len == 0) {

This change (replacing "!len" with "len == 0") is unnecessary, I think.

pkt-line.c static int packet_length(const char *linelen) · line 302

Quoted 6 lines

pkt-line.c +3 -1

 		packet_trace("0000", 4, 0);
-		return 0;
+		return PACKET_READ_FLUSH;
+	} else if (len >= 1 && len <= 3) {
+		die("protocol error: bad line length character: %.4s", linelen);
 	}

This seems to be more of a "bad line length" than a "bad line length character".

Also, some of the checks are redundant. Above, it is probably better to delete "len >= 1", and optionally write "len < 4" instead of "len <= 3" (to emphasize that the subtraction of 4 below does not result in a negative value).

pkt-line.c static int packet_length(const char *linelen) · line 306

Quoted 5 lines

pkt-line.c +2 -1

+
 	len -= 4;
-	if (len >= size)
+	if ((len < 0) || ((unsigned)len >= size))
 		die("protocol error: bad line length %d", len);

The "len < 0" check is redundant.

Quoted 9 lines

pkt-line.c +3 -3

-	ret = get_packet_data(fd, src_buf, src_len, buffer, len, options);
-	if (ret < 0)
-		return ret;
+
+	if (get_packet_data(fd, src_buffer, src_len, buffer, len, options) < 0)
+		return PACKET_READ_EOF;
 
 	if ((options & PACKET_READ_CHOMP_NEWLINE) &&
 	    len && buffer[len-1] == '\n')

pkt-line.h +1 -0

index 06c468927..c446e886a 100644
@@ -111,6 +111,63 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
  */
 ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
 
+struct packet_reader {

pkt-line.h +3 -0

+ */
+extern enum packet_read_status packet_reader_peek(struct packet_reader *reader);
+
 #define DEFAULT_PACKET_MAX 1000
 #define LARGE_PACKET_MAX 65520
 #define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)

Quoted 8 lines · On Tue, 2 Jan 2018 16:18:04 -0800 Brandon Williams <[email protected]> wrote:

pkt-line.h +0 -0

index 06c468927..c446e886a 100644
@@ -111,6 +111,63 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
  */
 ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
 

pkt-line.h char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size); · line 124

Quoted 11 lines

pkt-line.h +11 -0

+struct packet_reader {
+	/* source file descriptor */
+	int fd;
+
+	/* source buffer and its size */
+	char *src_buffer;
+	size_t src_len;
+
+	/* buffer that pkt-lines are read into and its size */
+	char *buffer;
+	unsigned buffer_size;

Is the intention to support different buffers in the future?

[snip]

Quoted 6 lines

+/*
+ * Peek the next packet line without consuming it and return the status.
+ * The next call to 'packet_reader_read()' will perform a read of the same line
+ * that was peeked, consuming the line.
+ *
+ * Only a single line can be peeked at a time.

It is logical to me that if you peeked at a line, and then peeked at it again, you will get the same line - I would phrase this not as a restriction ("only a single line") but just as a statement of fact (e.g. "Peeking at the same line multiple times without an intervening packet_reader_read will return the same result").

Quoted 7 lines

+ */

pkt-line.h +2 -0

+extern enum packet_read_status packet_reader_peek(struct packet_reader *reader);
+
 #define DEFAULT_PACKET_MAX 1000
 #define LARGE_PACKET_MAX 65520
 #define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)

In order to allow for code sharing with the server-side of fetch in protocol-v2 convert upload-pack to be a builtin.

Signed-off-by Brandon Williams <[email protected]>

---
 Makefile      | 3 ++-
 builtin.h     | 1 +
 git.c         | 1 +
 upload-pack.c | 2 +-
 4 files changed, 5 insertions(+), 2 deletions(-)

Makefile +2 -1

index 2a81ae22e..e0740b452 100644
@@ -636,7 +636,6 @@ PROGRAM_OBJS += imap-send.o
 PROGRAM_OBJS += sh-i18n--envsubst.o
 PROGRAM_OBJS += shell.o
 PROGRAM_OBJS += show-index.o
-PROGRAM_OBJS += upload-pack.o
 PROGRAM_OBJS += remote-testsvn.o
 
 # Binary suffix, set to .exe for Windows builds

@@ -701,6 +700,7 @@ BUILT_INS += git-merge-subtree$X
 BUILT_INS += git-show$X
 BUILT_INS += git-stage$X
 BUILT_INS += git-status$X
+BUILT_INS += git-upload-pack$X
 BUILT_INS += git-whatchanged$X
 
 # what 'all' will build and 'install' will install in gitexecdir,

@@ -904,6 +904,7 @@ LIB_OBJS += tree-diff.o
 LIB_OBJS += tree.o
 LIB_OBJS += tree-walk.o
 LIB_OBJS += unpack-trees.o
+LIB_OBJS += upload-pack.o
 LIB_OBJS += url.o
 LIB_OBJS += urlmatch.o
 LIB_OBJS += usage.o

builtin.h +1 -0

index 42378f3aa..f332a1257 100644
@@ -231,6 +231,7 @@ extern int cmd_update_ref(int argc, const char **argv, const char *prefix);
 extern int cmd_update_server_info(int argc, const char **argv, const char *prefix);
 extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);
 extern int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix);
+extern int cmd_upload_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_var(int argc, const char **argv, const char *prefix);
 extern int cmd_verify_commit(int argc, const char **argv, const char *prefix);
 extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);

git.c +1 -0

index c870b9719..f71073dc8 100644
@@ -478,6 +478,7 @@ static struct cmd_struct commands[] = {
        { "update-server-info", cmd_update_server_info, RUN_SETUP },
        { "upload-archive", cmd_upload_archive },
        { "upload-archive--writer", cmd_upload_archive_writer },
+       { "upload-pack", cmd_upload_pack },
        { "var", cmd_var, RUN_SETUP_GENTLY },
        { "verify-commit", cmd_verify_commit, RUN_SETUP },
        { "verify-pack", cmd_verify_pack },

upload-pack.c +1 -2

index d5de18127..20acaa49d 100644
@@ -1032,7 +1032,7 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
        return parse_hide_refs_config(var, value, "uploadpack");
 }
 
-int cmd_main(int argc, const char **argv)
+int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 {
        const char *dir;
        int strict = 0;
--

2.15.1.620.gb9897f4670-goog

Quoted 2 lines · Brandon Williams

In order to allow for code sharing with the server-side of fetch in protocol-v2 convert upload-pack to be a builtin.

What is the security aspect of this patch?

By making upload-pack builtin, it gains additional abilities, such as answers to '-h' or '--help' (which would start a pager). Is there an easy way to sooth my concerns? (best put into the commit message)

Thanks,
Stefan

Quoted 5 lines · Stefan Beller

Quoted 2 lines · Brandon Williams

In order to allow for code sharing with the server-side of fetch in protocol-v2 convert upload-pack to be a builtin.

Quoted 6 lines

What is the security aspect of this patch?

By making upload-pack builtin, it gains additional abilities, such as answers to '-h' or '--help' (which would start a pager). Is there an easy way to sooth my concerns? (best put into the commit message)

receive-pack is already a builtin, so theres that.

For defense in depth, it would be comforting if the git wrapper had some understanding of "don't support --help in handle_builtin when invoked as a dashed command". That is, I don't expect that anyone has been relying on

	git-add --help

acting like

git help add

instead of printing the usage message from

git add -h

It's a little fussy because today we rewrite "git add --help" to "git-add --help" before rewriting it to "git help add"; we'd have to skip that middle hop for this to work.

I don't think that has to block this patch or series, though --- it's just a separate thought about hardening.

Factor out the logic for processing shallow, deepen, deepen_since, and deepen_not lines into their own functions to simplify the 'receive_needs()' function in addition to making it easier to reuse some of this logic when implementing protocol_v2.

Signed-off-by Brandon Williams <[email protected]>

---
 upload-pack.c | 113 ++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 74 insertions(+), 39 deletions(-)

upload-pack.c +0 -0

index 20acaa49d..9a507ae53 100644

upload-pack.c +70 -40

+               struct object_id oid;
+               struct object *object;
+               if (get_oid_hex(arg, &oid))
+                       die("invalid shallow line: %s", line);
+               object = parse_object(&oid);
+               if (!object)
+                       return 1;
+               if (object->type != OBJ_COMMIT)
+                       die("invalid shallow object %s", oid_to_hex(&oid));
+               if (!(object->flags & CLIENT_SHALLOW)) {
+                       object->flags |= CLIENT_SHALLOW;
+                       add_object_array(object, NULL, shallows);
+               }
+               return 1;
+       }
+
+       return 0;
+}
+
+static int process_deepen(const char *line, int *depth)
+{
+       const char *arg;
+       if (skip_prefix(line, "deepen ", &arg)) {
+               char *end = NULL;
+               *depth = strtol(arg, &end, 0);
+               if (!end || *end || depth <= 0)
+                       die("Invalid deepen: %s", line);
+               return 1;
+       }
+
+       return 0;
+}
+
+static int process_deepen_since(const char *line, timestamp_t *deepen_since, int *deepen_rev_list)
+{
+       const char *arg;
+       if (skip_prefix(line, "deepen-since ", &arg)) {
+               char *end = NULL;
+               *deepen_since = parse_timestamp(arg, &end, 0);
+               if (!end || *end || !deepen_since ||
+                   /* revisions.c's max_age -1 is special */
+                   *deepen_since == -1)
+                       die("Invalid deepen-since: %s", line);
+               *deepen_rev_list = 1;
+               return 1;
+       }
+       return 0;
+}
+
+static int process_deepen_not(const char *line, struct string_list *deepen_not, int *deepen_rev_list)
+{
+       const char *arg;
+       if (skip_prefix(line, "deepen-not ", &arg)) {
+               char *ref = NULL;
+               struct object_id oid;
+               if (expand_ref(arg, strlen(arg), &oid, &ref) != 1)
+                       die("git upload-pack: ambiguous deepen-not: %s", line);
+               string_list_append(deepen_not, ref);
+               free(ref);
+               *deepen_rev_list = 1;
+               return 1;
+       }
+       return 0;
+}
+
 static void receive_needs(void)
 {
        struct object_array shallows = OBJECT_ARRAY_INIT;

@@ -752,49 +821,15 @@ static void receive_needs(void)
                if (!line)
                        break;
 
-               if (skip_prefix(line, "shallow ", &arg)) {
-                       struct object_id oid;
-                       struct object *object;
-                       if (get_oid_hex(arg, &oid))
-                               die("invalid shallow line: %s", line);
-                       object = parse_object(&oid);
-                       if (!object)
-                               continue;
-                       if (object->type != OBJ_COMMIT)
-                               die("invalid shallow object %s", oid_to_hex(&oid));
-                       if (!(object->flags & CLIENT_SHALLOW)) {
-                               object->flags |= CLIENT_SHALLOW;
-                               add_object_array(object, NULL, &shallows);
-                       }
+               if (process_shallow(line, &shallows))
                        continue;
-               }
-               if (skip_prefix(line, "deepen ", &arg)) {
-                       char *end = NULL;
-                       depth = strtol(arg, &end, 0);
-                       if (!end || *end || depth <= 0)
-                               die("Invalid deepen: %s", line);
+               if (process_deepen(line, &depth))
                        continue;
-               }
-               if (skip_prefix(line, "deepen-since ", &arg)) {
-                       char *end = NULL;
-                       deepen_since = parse_timestamp(arg, &end, 0);
-                       if (!end || *end || !deepen_since ||
-                           /* revisions.c's max_age -1 is special */
-                           deepen_since == -1)
-                               die("Invalid deepen-since: %s", line);
-                       deepen_rev_list = 1;
+               if (process_deepen_since(line, &deepen_since, &deepen_rev_list))
                        continue;
-               }
-               if (skip_prefix(line, "deepen-not ", &arg)) {
-                       char *ref = NULL;
-                       struct object_id oid;
-                       if (expand_ref(arg, strlen(arg), &oid, &ref) != 1)
-                               die("git upload-pack: ambiguous deepen-not: %s", line);
-                       string_list_append(&deepen_not, ref);
-                       free(ref);
-                       deepen_rev_list = 1;
+               if (process_deepen_not(line, &deepen_not, &deepen_rev_list))
                        continue;
-               }
+
                if (!skip_prefix(line, "want ", &arg) ||
                    get_oid_hex(arg, &oid_buf))
                        die("git upload-pack: protocol error, "
--

2.15.1.620.gb9897f4670-goog

Remove code duplication and use the existing 'get_refs_via_connect()' function to retrieve a remote's heads in 'fetch_refs_via_pack()' and 'git_transport_push()'.

Signed-off-by Brandon Williams <[email protected]>

---
 transport.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

transport.c +4 -15

index fc802260f..8e8779096 100644
@@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport *transport,
        args.cloning = transport->cloning;
        args.update_shallow = data->options.update_shallow;
 
-       if (!data->got_remote_heads) {
-               connect_setup(transport, 0);
-               get_remote_heads(data->fd[0], NULL, 0, &refs_tmp, 0,
-                                NULL, &data->shallow);
-               data->got_remote_heads = 1;
-       }
+       if (!data->got_remote_heads)
+               refs_tmp = get_refs_via_connect(transport, 0);
 
        refs = fetch_pack(&args, data->fd, data->conn,
                          refs_tmp ? refs_tmp : transport->remote_refs,

@@ -541,14 +537,8 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
        struct send_pack_args args;
        int ret;
 
-       if (!data->got_remote_heads) {
-               struct ref *tmp_refs;
-               connect_setup(transport, 1);
-
-               get_remote_heads(data->fd[0], NULL, 0, &tmp_refs, REF_NORMAL,
-                                NULL, &data->shallow);
-               data->got_remote_heads = 1;
-       }
+       if (!data->got_remote_heads)
+               get_refs_via_connect(transport, 1);
 
        memset(&args, 0, sizeof(args));
        args.send_mirror = !!(flags & TRANSPORT_PUSH_MIRROR);
--

2.15.1.620.gb9897f4670-goog

connect.c struct ref **get_remote_heads(int in, char *src_buf, size_t src_len, · line 259

Quoted 8 lines · On Tue, 2 Jan 2018 16:18:09 -0800 Brandon Williams <[email protected]> wrote:

connect.c +7 -1

-	while ((len = read_remote_ref(in, &src_buf, &src_len, &responded))) {
+	while (state != EXPECTING_DONE) {
+		switch (packet_reader_read(&reader)) {
+		case PACKET_READ_EOF:
+			die_initial_contact(1);
+		case PACKET_READ_NORMAL:
+			len = reader.pktlen;
+			if (len > 4 && skip_prefix(packet_buffer, "ERR ", &arg))

This should be a field in reader, not the global packet_buffer, I think.

Also, I did a search of usages of packet_buffer, and there are just a few of them - it might be worthwhile to eliminate it, and have each component using it allocate its own buffer. But this can be done in a separate patch set.

connect.c struct ref **get_remote_heads(int in, char *src_buf, size_t src_len, · line 286

Quoted 5 lines

connect.c +0 -0

@@ -269,6 +284,8 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 			if (process_shallow(len, shallow_points))
 				break;
 			die("protocol error: unexpected '%s'", packet_buffer);

Here too.

transport.c struct git_transport_data { · line 121

Quoted 9 lines · On Tue, 2 Jan 2018 16:18:11 -0800 Brandon Williams <[email protected]> wrote:

transport.c +1 -0

index 63c3dbab9..2378dcb38 100644
@@ -118,6 +118,7 @@ struct git_transport_data {
 	struct child_process *conn;
 	int fd[2];
 	unsigned got_remote_heads : 1;
+	enum protocol_version version;

Should this be initialized to protocol_unknown_version? Right now, as far as I can tell, it is zero-initialized, which means protocol_v0.

serve.c new file +0 -0

new file mode 100644
index 000000000..da8127775

Documentation/technical/protocol-v2.txt

Quoted 5 lines · On Tue, 2 Jan 2018 16:18:13 -0800 Brandon Williams <[email protected]> wrote:

Documentation/technical/protocol-v2.txt new file +0 -0

new file mode 100644
index 000000000..b87ba3816

I'll review the documentation later, once there is some consensus that the overall design is OK. (Or maybe there already is consensus?)

builtin/serve.c line 6

Quoted 12 lines

builtin/serve.c new file +6 -0

new file mode 100644
index 000000000..bb726786a
@@ -0,0 +1,30 @@
+#include "cache.h"
+#include "builtin.h"
+#include "parse-options.h"
+#include "serve.h"
+
+static char const * const grep_usage[] = {

Should be serve_usage.

Quoted 5 lines

serve.c new file +0 -0

new file mode 100644
index 000000000..da8127775

[snip]

serve.c line 24

Quoted 2 lines

serve.c new file +2 -0

+struct protocol_capability {
+	const char *name; /* capability name */

Maybe document as:

The name of the capability. The server uses this name when advertising this capability, and the client uses this name to invoke the command corresponding to this capability.

serve.c line 30

Quoted 5 lines

serve.c new file +5 -0

+	/*
+	 * Function queried to see if a capability should be advertised.
+	 * Optionally a value can be specified by adding it to 'value'.
+	 */
+	int (*advertise)(struct repository *r, struct strbuf *value);

Document what happens when value is appended to. For example:

... If value is appended to, the server will advertise this capability as <name>=<value> instead of <name>.

Quoted 9 lines

serve.c new file +9 -0

+	/*
+	 * Function called when a client requests the capability as a command.
+	 * The command request will be provided to the function via 'keys', the
+	 * capabilities requested, and 'args', the command specific parameters.
+	 *
+	 * This field should be NULL for capabilities which are not commands.
+	 */
+	int (*command)(struct repository *r,
+		       struct argv_array *keys,

serve.c line 41

Quoted 1 line

serve.c new file +1 -0

+		       struct argv_array *args);

Looking at the code below, I see that the command is not executed unless advertise returns true - this means that a command cannot be both supported and unadvertised. Would this be too restrictive? For example, this would disallow a gradual across-multiple-servers rollout in which we allow but not advertise a capability, and then after some time, advertise the capability.

If we change this, then the value parameter of advertise can be mandatory instead of optional.

All good documentation changes.

Quoted 9 lines

Quoted 9 lines

serve.c new file +9 -0

+	/*
+	 * Function called when a client requests the capability as a command.
+	 * The command request will be provided to the function via 'keys', the
+	 * capabilities requested, and 'args', the command specific parameters.
+	 *
+	 * This field should be NULL for capabilities which are not commands.
+	 */
+	int (*command)(struct repository *r,
+		       struct argv_array *keys,

Quoted 8 lines

serve.c line 41

Quoted 1 line

serve.c new file +1 -0

+		       struct argv_array *args);

Looking at the code below, I see that the command is not executed unless advertise returns true - this means that a command cannot be both supported and unadvertised. Would this be too restrictive? For example, this would disallow a gradual across-multiple-servers rollout in which we allow but not advertise a capability, and then after some time, advertise the capability.

One way to change this would be to just add another function to the struct which is called to check if the command is allowed, instead of relying on the same function to do that for both advertise and allow...though I don't see a big win for allowing a command but not advertising it.

Quoted 2 lines

If we change this, then the value parameter of advertise can be mandatory instead of optional.

I don't see how this fixes the issue you bring up.

Quoted 1 line · On Tue, 9 Jan 2018 14:16:42 -0800 Brandon Williams <[email protected]> wrote:

All good documentation changes.

Thanks!

Quoted 18 lines

Quoted 9 lines

Quoted 9 lines

serve.c new file +9 -0

+	/*
+	 * Function called when a client requests the capability as a command.
+	 * The command request will be provided to the function via 'keys', the
+	 * capabilities requested, and 'args', the command specific parameters.
+	 *
+	 * This field should be NULL for capabilities which are not commands.
+	 */
+	int (*command)(struct repository *r,
+		       struct argv_array *keys,

Quoted 8 lines

serve.c line 41

Quoted 1 line

serve.c new file +1 -0

+		       struct argv_array *args);

Looking at the code below, I see that the command is not executed unless advertise returns true - this means that a command cannot be both supported and unadvertised. Would this be too restrictive? For example, this would disallow a gradual across-multiple-servers rollout in which we allow but not advertise a capability, and then after some time, advertise the capability.

Quoted 5 lines

One way to change this would be to just add another function to the struct which is called to check if the command is allowed, instead of relying on the same function to do that for both advertise and allow...though I don't see a big win for allowing a command but not advertising it.

My rationale for allowing a command but not advertising it is in the paragraph above (that you quoted), but if that is insufficient rationale, then I agree that we don't need to do this.

Quoted 5 lines

Quoted 3 lines

If we change this, then the value parameter of advertise can be mandatory instead of optional.

I don't see how this fixes the issue you bring up.

This is a consequence, not a fix - if we were to do as I suggested, then we no longer need to invoke advertise to check whether something is advertised except when we are advertising them, in which case "value" never needs to be NULL.

Introduce git-serve, the base server for protocol version 2.

Protocol version 2 is intended to be a replacement for Git's current wire protocol. The intention is that it will be a simpler, less wasteful protocol which can evolve over time.

Protocol version 2 improves upon version 1 by eliminating the initial ref advertisement. In its place a server will export a list of capabilities and commands which it supports in a capability advertisement. A client can then request that a particular command be executed by providing a number of capabilities and command specific parameters. At the completion of a command, a client can request that another command be executed or can terminate the connection by sending a flush packet.

Signed-off-by Brandon Williams <[email protected]>

---
  .gitignore                              |   1 +
  Documentation/technical/protocol-v2.txt |  91 ++++++++++++
  Makefile                                |   2 +
  builtin.h                               |   1 +
  builtin/serve.c                         |  30 ++++
  git.c                                   |   1 +
  serve.c                                 | 239 ++++++++++++++++++++++++++++++++
  serve.h                                 |  15 ++
  8 files changed, 380 insertions(+)

create mode 100644 Documentation/technical/protocol-v2.txt create mode 100644 builtin/serve.c
create mode 100644 serve.c
create mode 100644 serve.h

.gitignore +1 -0

index 833ef3b0b..2d0450c26 100644
@@ -140,6 +140,7 @@
  /git-rm
  /git-send-email
  /git-send-pack
+/git-serve
  /git-sh-i18n
  /git-sh-i18n--envsubst
  /git-sh-setup

Documentation/technical/protocol-v2.txt new file +26 -0

new file mode 100644
index 000000000..b87ba3816
@@ -0,0 +1,91 @@
+ Git Wire Protocol, Version 2
+==============================
+
+This document presents a specification for a version 2 of Git's wire
+protocol.  Protocol v2 will improve upon v1 in the following ways:
+
+  * Instead of multiple service names, multiple commands will be
+    supported by a single service.
+  * Easily extendable as capabilities are moved into their own section
+    of the protocol, no longer being hidden behind a NUL byte and
+    limited by the size of a pkt-line (as there will be a single
+    capability per pkt-line).
+  * Separate out other information hidden behind NUL bytes (e.g. agent
+    string as a capability and symrefs can be requested using 'ls-refs')
+  * Reference advertisement will be omitted unless explicitly requested
+  * ls-refs command to explicitly request some refs
+
+ Detailed Design
+=================
+
+A client can request to speak protocol v2 by sending `version=2` in the
+side-channel `GIT_PROTOCOL` in the initial request to the server.
+
+In protocol v2 communication is command oriented.  When first contacting a
+server a list of capabilities will advertised.  Some of these capabilities
+will be commands which a client can request be executed.  Once a command

Documentation/technical/protocol-v2.txt new file +55 -0

+
+ Capability Advertisement
+--------------------------
+
+A server which decides to communicate (based on a request from a client)
+using protocol version 2, notifies the client by sending a version string
+in its initial response followed by an advertisement of its capabilities.
+Each capability is a key with an optional value.  Clients must ignore all
+unknown keys.  Semantics of unknown values are left to the definition of
+each key.  Some capabilities will describe commands which can be requested
+to be executed by the client.
+
+    capability-advertisement = protocol-version
+			       capability-list
+			       flush-pkt
+
+    protocol-version = PKT-LINE("version 2" LF)
+    capability-list = *capability
+    capability = PKT-LINE(key[=value] LF)
+
+    key = 1*CHAR
+    value = 1*CHAR
+    CHAR = 1*(ALPHA / DIGIT / "-" / "_")
+
+A client then responds to select the command it wants with any particular
+capabilities or arguments.  There is then an optional section where the
+client can provide any command specific parameters or queries.
+
+    command-request = command
+		      capability-list
+		      (command-args)
+		      flush-pkt
+    command = PKT-LINE("command=" key LF)
+    command-args = delim-pkt
+		   *arg
+    arg = 1*CHAR
+
+The server will then check to ensure that the client's request is
+comprised of a valid command as well as valid capabilities which were
+advertised.  If the request is valid the server will then execute the
+command.
+
+A particular command can last for as many rounds as are required to
+complete the service (multiple for negotiation during fetch or no
+additional trips in the case of ls-refs).
+
+When finished a client should send an empty request of just a flush-pkt to
+terminate the connection.
+
+ Commands in v2
+~~~~~~~~~~~~~~~~
+
+Commands are the core actions that a client wants to perform (fetch, push,
+etc).  Each command will be provided with a list capabilities and
+arguments as requested by a client.

Quoted 86 lines · Brandon Williams

Introduce git-serve, the base server for protocol version 2.

Protocol version 2 is intended to be a replacement for Git's current wire protocol. The intention is that it will be a simpler, less wasteful protocol which can evolve over time.

Protocol version 2 improves upon version 1 by eliminating the initial ref advertisement. In its place a server will export a list of capabilities and commands which it supports in a capability advertisement. A client can then request that a particular command be executed by providing a number of capabilities and command specific parameters. At the completion of a command, a client can request that another command be executed or can terminate the connection by sending a flush packet.

Signed-off-by Brandon Williams <[email protected]>

---
  .gitignore                              |   1 +
  Documentation/technical/protocol-v2.txt |  91 ++++++++++++
  Makefile                                |   2 +
  builtin.h                               |   1 +
  builtin/serve.c                         |  30 ++++
  git.c                                   |   1 +
  serve.c                                 | 239
++++++++++++++++++++++++++++++++
  serve.h                                 |  15 ++
  8 files changed, 380 insertions(+)

create mode 100644 Documentation/technical/protocol-v2.txt create mode 100644 builtin/serve.c
create mode 100644 serve.c
create mode 100644 serve.h

.gitignore +1 -0

index 833ef3b0b..2d0450c26 100644
@@ -140,6 +140,7 @@
  /git-rm
  /git-send-email
  /git-send-pack
+/git-serve
  /git-sh-i18n
  /git-sh-i18n--envsubst
  /git-sh-setup

Documentation/technical/protocol-v2.txt new file +24 -0

new file mode 100644
index 000000000..b87ba3816
@@ -0,0 +1,91 @@
+ Git Wire Protocol, Version 2
+==============================
+
+This document presents a specification for a version 2 of Git's wire
+protocol.  Protocol v2 will improve upon v1 in the following ways:
+
+  * Instead of multiple service names, multiple commands will be
+    supported by a single service.
+  * Easily extendable as capabilities are moved into their own section
+    of the protocol, no longer being hidden behind a NUL byte and
+    limited by the size of a pkt-line (as there will be a single
+    capability per pkt-line).
+  * Separate out other information hidden behind NUL bytes (e.g. agent
+    string as a capability and symrefs can be requested using 'ls-refs')
+  * Reference advertisement will be omitted unless explicitly requested
+  * ls-refs command to explicitly request some refs
+
+ Detailed Design
+=================
+
+A client can request to speak protocol v2 by sending `version=2` in the
+side-channel `GIT_PROTOCOL` in the initial request to the server.
+
+In protocol v2 communication is command oriented.  When first contacting

a
+server a list of capabilities will advertised. Some of these capabilities

Documentation/technical/protocol-v2.txt new file +7 -0

+will be commands which a client can request be executed.  Once a command
+has completed, a client can reuse the connection and request that other
+commands be executed.
+
+ Special Packets
+-----------------
+

Documentation/technical/protocol-v2.txt line 36

Quoted 4 lines

Documentation/technical/protocol-v2.txt new file +4 -0

+In protocol v2 these special packets will have the following semantics:
+
+  * '0000' Flush Packet (flush-pkt) - indicates the end of a message
+  * '0001' Delimiter Packet (delim-pkt) - separates sections of a message

Previously, a 0001 pkt-line meant that there was 1 byte of data following, right?

Does this change that and/or prevent 1 byte
packets? (Not sure if it is likely, but the odd-tail of a packfile might get sent in a 0001 line, right?) Or is it that 0001 is only special during the V2 negotiation stuff, but not during the packfile transmission?

(I'm not against having this delimiter -- I think it is useful, but just curious if will cause problems elsewhere.)

Should we also consider increasing the pkt-line limit to 5 hex-digits
while we're at it ? That would let us have 1MB buffers if that would
help with large packfiles.

Granted, we're throttled by the network,
so it might not matter. Would it be interesting to have a 5 digit prefix with parts of the high bits of first digit being flags ? Or is this too radical of a change?

Quoted 5 lines · Jeff Hostetler

Quoted 2 lines · Brandon Williams

Introduce git-serve, the base server for protocol version 2.

Quoted 11 lines

Documentation/technical/protocol-v2.txt line 36

Quoted 7 lines

+ Special Packets
+-----------------

Documentation/technical/protocol-v2.txt new file +5 -0

+
+In protocol v2 these special packets will have the following semantics:
+
+  * '0000' Flush Packet (flush-pkt) - indicates the end of a message
+  * '0001' Delimiter Packet (delim-pkt) - separates sections of a message

Previously, a 0001 pkt-line meant that there was 1 byte of data following, right?

No, the length was including the length field, so 0005 would indicate that there is one byte following, (+4 bytes of "0005" included)

Quoted 3 lines

Should we also consider increasing the pkt-line limit to 5 hex-digits
while we're at it ? That would let us have 1MB buffers if that would
help with large packfiles.

AFAICT there is a static allocation of one pkt-line (of maximum size), such that the code can read in a full packet and then process it. If we'd increase the packet size we'd need the static buffer to be 1MB, which sounds good for my developer machine. But I suspect it may be too much for people using git on embedded devices?

pack files larger than 64k are put into multiple pkt-lines, which is not a big deal, as the overhead of 4bytes per 64k is negligible. (also there is progress information in the side channel, which would come in as a special packet in between real packets, such that every 64k transmitted you can update your progress meter; Not sure I feel strongly on fewer progress updates)

Quoted 4 lines

Granted, we're throttled by the network,
so it might not matter. Would it be interesting to have a 5 digit prefix with parts of the high bits of first digit being flags ? Or is this too radical of a change?

What would the flags be for?

As an alternative we could put the channel number in one byte, such that we can have a side channel not just while streaming the pack but all the time. (Again, not sure if that buys a lot for us)

Quoted 6 lines · Stefan Beller

Quoted 5 lines · Jeff Hostetler

Quoted 2 lines · Brandon Williams

Introduce git-serve, the base server for protocol version 2.

[...]

Quoted 12 lines

Quoted 11 lines

Documentation/technical/protocol-v2.txt line 36

Quoted 7 lines

+ Special Packets
+-----------------

Documentation/technical/protocol-v2.txt new file +5 -0

+
+In protocol v2 these special packets will have the following semantics:
+
+  * '0000' Flush Packet (flush-pkt) - indicates the end of a message
+  * '0001' Delimiter Packet (delim-pkt) - separates sections of a message

Previously, a 0001 pkt-line meant that there was 1 byte of data following, right?

Quoted 2 lines

No, the length was including the length field, so 0005 would indicate that there is one byte following, (+4 bytes of "0005" included)

d'oh. right. thanks!

Quoted 9 lines

Quoted 3 lines

Should we also consider increasing the pkt-line limit to 5 hex-digits
while we're at it ? That would let us have 1MB buffers if that would
help with large packfiles.

AFAICT there is a static allocation of one pkt-line (of maximum size), such that the code can read in a full packet and then process it. If we'd increase the packet size we'd need the static buffer to be 1MB, which sounds good for my developer machine. But I suspect it may be too much for people using git on embedded devices?

I got burned by that static buffer once upon a time when I wanted to have 2 streams going at the same time. Hopefully, we can move that into the new reader structure at some point (if it isn't already).

Quoted 14 lines

pack files larger than 64k are put into multiple pkt-lines, which is not a big deal, as the overhead of 4bytes per 64k is negligible. (also there is progress information in the side channel, which would come in as a special packet in between real packets, such that every 64k transmitted you can update your progress meter; Not sure I feel strongly on fewer progress updates)

Quoted 4 lines

Granted, we're throttled by the network,
so it might not matter. Would it be interesting to have a 5 digit prefix with parts of the high bits of first digit being flags ? Or is this too radical of a change?

Quoted 6 lines

What would the flags be for?

As an alternative we could put the channel number in one byte, such that we can have a side channel not just while streaming the pack but all the time. (Again, not sure if that buys a lot for us)

Delimiters like the 0001 and the side channel are a couple of ideas, but I was just thinking out loud. And right, I'm not sure it gets us much right now.

Jeff

Quoted 93 lines · Jeff Hostetler

Quoted 90 lines · Brandon Williams

Introduce git-serve, the base server for protocol version 2.

Protocol version 2 is intended to be a replacement for Git's current wire protocol. The intention is that it will be a simpler, less wasteful protocol which can evolve over time.

Protocol version 2 improves upon version 1 by eliminating the initial ref advertisement. In its place a server will export a list of capabilities and commands which it supports in a capability advertisement. A client can then request that a particular command be executed by providing a number of capabilities and command specific parameters. At the completion of a command, a client can request that another command be executed or can terminate the connection by sending a flush packet.

Signed-off-by Brandon Williams <[email protected]>

---
  .gitignore                              |   1 +
  Documentation/technical/protocol-v2.txt |  91 ++++++++++++
  Makefile                                |   2 +
  builtin.h                               |   1 +
  builtin/serve.c                         |  30 ++++
  git.c                                   |   1 +
  serve.c                                 | 239
++++++++++++++++++++++++++++++++
  serve.h                                 |  15 ++
  8 files changed, 380 insertions(+)

create mode 100644 Documentation/technical/protocol-v2.txt create mode 100644 builtin/serve.c
create mode 100644 serve.c
create mode 100644 serve.h

.gitignore +1 -0

index 833ef3b0b..2d0450c26 100644
@@ -140,6 +140,7 @@
  /git-rm
  /git-send-email
  /git-send-pack
+/git-serve
  /git-sh-i18n
  /git-sh-i18n--envsubst
  /git-sh-setup

Documentation/technical/protocol-v2.txt new file +27 -0

new file mode 100644
index 000000000..b87ba3816
@@ -0,0 +1,91 @@
+ Git Wire Protocol, Version 2
+==============================
+
+This document presents a specification for a version 2 of Git's wire
+protocol.  Protocol v2 will improve upon v1 in the following ways:
+
+  * Instead of multiple service names, multiple commands will be
+    supported by a single service.
+  * Easily extendable as capabilities are moved into their own section
+    of the protocol, no longer being hidden behind a NUL byte and
+    limited by the size of a pkt-line (as there will be a single
+    capability per pkt-line).
+  * Separate out other information hidden behind NUL bytes (e.g. agent
+    string as a capability and symrefs can be requested using
+ 'ls-refs')
+  * Reference advertisement will be omitted unless explicitly
+ requested
+  * ls-refs command to explicitly request some refs
+
+ Detailed Design
+=================
+
+A client can request to speak protocol v2 by sending `version=2` in
+the side-channel `GIT_PROTOCOL` in the initial request to the server.
+
+In protocol v2 communication is command oriented.  When first
+contacting

a
+server a list of capabilities will advertised. Some of these capabilities

Documentation/technical/protocol-v2.txt new file +8 -0

+will be commands which a client can request be executed.  Once a
+command has completed, a client can reuse the connection and request
+that other commands be executed.
+
+ Special Packets
+-----------------
+
+In protocol v2 these special packets will have the following semantics:

Quoted 8 lines

Documentation/technical/protocol-v2.txt

Quoted 4 lines

Documentation/technical/protocol-v2.txt new file +4 -0

+
+  * '0000' Flush Packet (flush-pkt) - indicates the end of a message
+  * '0001' Delimiter Packet (delim-pkt) - separates sections of a
+ message

Previously, a 0001 pkt-line meant that there was 1 byte of data following, right?

No, the length was including the length field, so 0005 would indicate that there is one byte following, (+4 bytes of "0005" included)

Quoted 4 lines

Does this change that and/or prevent 1 byte packets? (Not sure if it is likely, but the odd-tail of a packfile might get sent in a 0001 line, right?) Or is it that 0001 is only special during the V2 negotiation stuff, but not during the packfile transmission?

0001 is invalid in the current protocol v0.

Quoted 7 lines

(I'm not against having this delimiter -- I think it is useful, but just curious if will cause problems elsewhere.)

Should we also consider increasing the pkt-line limit to 5 hex-digits
while we're at it ? That would let us have 1MB buffers if that would
help with large packfiles.

AFAICT there is a static allocation of one pkt-line (of maximum size), such that the code can read in a full packet and then process it.
If we'd increase the packet size we'd need the static buffer to be 1MB, which sounds good for my developer machine. But I suspect it may be too much for people using git on embedded devices?

pack files larger than 64k are put into multiple pkt-lines, which is not a big deal, as the overhead of 4bytes per 64k is negligible.
(also there is progress information in the side channel, which would come in as a special packet in between real packets, such that every 64k transmitted you can update your progress meter; Not sure I feel strongly on fewer progress updates)

Quoted 4 lines

Granted, we're throttled by the network, so it might not matter.
Would it be interesting to have a 5 digit prefix with parts of the high bits of first digit being flags ?
Or is this too radical of a change?

What would the flags be for?

As an alternative we could put the channel number in one byte, such that we can have a side channel not just while streaming the pack but all the time. (Again, not sure if that buys a lot for us)

Quoted 128 lines · Stefan Beller

Quoted 93 lines · Jeff Hostetler

Quoted 90 lines · Brandon Williams

Introduce git-serve, the base server for protocol version 2.

Protocol version 2 is intended to be a replacement for Git's current wire protocol. The intention is that it will be a simpler, less wasteful protocol which can evolve over time.

Protocol version 2 improves upon version 1 by eliminating the initial ref advertisement. In its place a server will export a list of capabilities and commands which it supports in a capability advertisement. A client can then request that a particular command be executed by providing a number of capabilities and command specific parameters. At the completion of a command, a client can request that another command be executed or can terminate the connection by sending a flush packet.

Signed-off-by Brandon Williams <[email protected]>

---
  .gitignore                              |   1 +
  Documentation/technical/protocol-v2.txt |  91 ++++++++++++
  Makefile                                |   2 +
  builtin.h                               |   1 +
  builtin/serve.c                         |  30 ++++
  git.c                                   |   1 +
  serve.c                                 | 239
++++++++++++++++++++++++++++++++
  serve.h                                 |  15 ++
  8 files changed, 380 insertions(+)

create mode 100644 Documentation/technical/protocol-v2.txt create mode 100644 builtin/serve.c
create mode 100644 serve.c
create mode 100644 serve.h

.gitignore +1 -0

index 833ef3b0b..2d0450c26 100644
@@ -140,6 +140,7 @@
  /git-rm
  /git-send-email
  /git-send-pack
+/git-serve
  /git-sh-i18n
  /git-sh-i18n--envsubst
  /git-sh-setup

Documentation/technical/protocol-v2.txt new file +27 -0

new file mode 100644
index 000000000..b87ba3816
@@ -0,0 +1,91 @@
+ Git Wire Protocol, Version 2
+==============================
+
+This document presents a specification for a version 2 of Git's wire
+protocol.  Protocol v2 will improve upon v1 in the following ways:
+
+  * Instead of multiple service names, multiple commands will be
+    supported by a single service.
+  * Easily extendable as capabilities are moved into their own section
+    of the protocol, no longer being hidden behind a NUL byte and
+    limited by the size of a pkt-line (as there will be a single
+    capability per pkt-line).
+  * Separate out other information hidden behind NUL bytes (e.g. agent
+    string as a capability and symrefs can be requested using
+ 'ls-refs')
+  * Reference advertisement will be omitted unless explicitly
+ requested
+  * ls-refs command to explicitly request some refs
+
+ Detailed Design
+=================
+
+A client can request to speak protocol v2 by sending `version=2` in
+the side-channel `GIT_PROTOCOL` in the initial request to the server.
+
+In protocol v2 communication is command oriented.  When first
+contacting

a
+server a list of capabilities will advertised. Some of these capabilities

Documentation/technical/protocol-v2.txt new file +8 -0

+will be commands which a client can request be executed.  Once a
+command has completed, a client can reuse the connection and request
+that other commands be executed.
+
+ Special Packets
+-----------------
+
+In protocol v2 these special packets will have the following semantics:

Quoted 8 lines

Documentation/technical/protocol-v2.txt

Quoted 4 lines

Documentation/technical/protocol-v2.txt new file +4 -0

+
+  * '0000' Flush Packet (flush-pkt) - indicates the end of a message
+  * '0001' Delimiter Packet (delim-pkt) - separates sections of a
+ message

Previously, a 0001 pkt-line meant that there was 1 byte of data following, right?

No, the length was including the length field, so 0005 would indicate that there is one byte following, (+4 bytes of "0005" included)

Quoted 4 lines

Does this change that and/or prevent 1 byte packets? (Not sure if it is likely, but the odd-tail of a packfile might get sent in a 0001 line, right?) Or is it that 0001 is only special during the V2 negotiation stuff, but not during the packfile transmission?

0001 is invalid in the current protocol v0.

Quoted 7 lines

(I'm not against having this delimiter -- I think it is useful, but just curious if will cause problems elsewhere.)

Should we also consider increasing the pkt-line limit to 5 hex-digits
while we're at it ? That would let us have 1MB buffers if that would
help with large packfiles.

AFAICT there is a static allocation of one pkt-line (of maximum size), such that the code can read in a full packet and then process it.
If we'd increase the packet size we'd need the static buffer to be 1MB, which sounds good for my developer machine. But I suspect it may be too much for people using git on embedded devices?

Quoted 6 lines

pack files larger than 64k are put into multiple pkt-lines, which is not a big deal, as the overhead of 4bytes per 64k is negligible.
(also there is progress information in the side channel, which would come in as a special packet in between real packets, such that every 64k transmitted you can update your progress meter; Not sure I feel strongly on fewer progress updates)

Can I request, selfishly from my own platform's (NonStop) performance heartache, that we don't require 1Mb? We're not embedded on this platform, but there is an optimized message system packet size down at 50Kb that I would like to stay under. Although above that is no problem, there is a significant cost incurred above that size point. And please make sure xread/xwrite are used in any event.

Quoted 11 lines

Quoted 5 lines

Granted, we're throttled by the network, so it might not matter.
Would it be interesting to have a 5 digit prefix with parts of the high bits of first digit being flags ?
Or is this too radical of a change?

What would the flags be for?

As an alternative we could put the channel number in one byte, such that we can have a side channel not just while streaming the pack but all the time. (Again, not sure if that buys a lot for us)

Cheers,
Randall

-- Brief whoami:
NonStop developer since approximately 211288444200000000 UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Quoted 57 lines · Randall S. Becker

Quoted 56 lines · Stefan Beller

Quoted 53 lines · Jeff Hostetler

Quoted 50 lines · Brandon Williams

Introduce git-serve, the base server for protocol version 2.

Protocol version 2 is intended to be a replacement for Git's current wire protocol. The intention is that it will be a simpler, less wasteful protocol which can evolve over time.

Protocol version 2 improves upon version 1 by eliminating the initial ref advertisement. In its place a server will export a list of capabilities and commands which it supports in a capability advertisement. A client can then request that a particular command be executed by providing a number of capabilities and command specific parameters. At the completion of a command, a client can request that another command be executed or can terminate the connection by sending a flush packet.

Signed-off-by Brandon Williams <[email protected]>

---
  .gitignore                              |   1 +
  Documentation/technical/protocol-v2.txt |  91 ++++++++++++
  Makefile                                |   2 +
  builtin.h                               |   1 +
  builtin/serve.c                         |  30 ++++
  git.c                                   |   1 +
  serve.c                                 | 239
++++++++++++++++++++++++++++++++
  serve.h                                 |  15 ++
  8 files changed, 380 insertions(+)

create mode 100644 Documentation/technical/protocol-v2.txt create mode 100644 builtin/serve.c
create mode 100644 serve.c
create mode 100644 serve.h

.gitignore index 833ef3b0b..2d0450c26 renamed from .gitignore +0 -0

100644

.gitignore +1 -0

@@ -140,6 +140,7 @@
  /git-rm
  /git-send-email
  /git-send-pack
+/git-serve
  /git-sh-i18n
  /git-sh-i18n--envsubst
  /git-sh-setup

Documentation/technical/protocol-v2.txt new file +0 -1

new file mode 100644
index 000000000..b87ba3816
--- /dev/null

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Documentation/technical/protocol-v2.txt line 5

Quoted 7 lines

+++ b/Documentation/technical/protocol-v2.txt

Documentation/technical/protocol-v2.txt new file +5 -0

@@ -0,0 +1,91 @@
+ Git Wire Protocol, Version 2
+==============================
+
+This document presents a specification for a version 2 of Git's
+wire protocol.  Protocol v2 will improve upon v1 in the following

ways:

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Documentation/technical/protocol-v2.txt new file +4 -0

+
+  * Instead of multiple service names, multiple commands will be
+    supported by a single service.
+  * Easily extendable as capabilities are moved into their own

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Documentation/technical/protocol-v2.txt

Quoted 4 lines

Documentation/technical/protocol-v2.txt new file +4 -0

+    of the protocol, no longer being hidden behind a NUL byte and
+    limited by the size of a pkt-line (as there will be a single
+    capability per pkt-line).
+  * Separate out other information hidden behind NUL bytes (e.g.

agent

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Documentation/technical/protocol-v2.txt line 24

Quoted 11 lines

Documentation/technical/protocol-v2.txt new file +11 -0

+    string as a capability and symrefs can be requested using
+ 'ls-refs')
+  * Reference advertisement will be omitted unless explicitly
+ requested
+  * ls-refs command to explicitly request some refs
+
+ Detailed Design
+=================
+
+A client can request to speak protocol v2 by sending `version=2`
+in the side-channel `GIT_PROTOCOL` in the initial request to the

server.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Documentation/technical/protocol-v2.txt new file +3 -0

+
+In protocol v2 communication is command oriented.  When first
+contacting

a
+server a list of capabilities will advertised. Some of these capabilities

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Documentation/technical/protocol-v2.txt

Quoted 8 lines

Documentation/technical/protocol-v2.txt new file +8 -0

+will be commands which a client can request be executed.  Once a
+command has completed, a client can reuse the connection and
+request that other commands be executed.
+
+ Special Packets
+-----------------
+
+In protocol v2 these special packets will have the following

semantics:

Quoted 26 lines

Quoted 26 lines

Quoted 9 lines

Documentation/technical/protocol-v2.txt

Quoted 5 lines

Documentation/technical/protocol-v2.txt new file +5 -0

+
+  * '0000' Flush Packet (flush-pkt) - indicates the end of a
+ message
+  * '0001' Delimiter Packet (delim-pkt) - separates sections of
+ a message

Previously, a 0001 pkt-line meant that there was 1 byte of data following, right?

No, the length was including the length field, so 0005 would indicate that there is one byte following, (+4 bytes of "0005" included)

Quoted 4 lines

Does this change that and/or prevent 1 byte packets? (Not sure if it is likely, but the odd-tail of a packfile might get sent in a 0001 line, right?) Or is it that 0001 is only special during the V2 negotiation stuff, but not during the packfile transmission?

0001 is invalid in the current protocol v0.

Quoted 5 lines

(I'm not against having this delimiter -- I think it is useful, but just curious if will cause problems elsewhere.)

Should we also consider increasing the pkt-line limit to 5

Quoted 1 line

Quoted 1 line

Quoted 1 line

while we're at it ? That would let us have 1MB buffers if that

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

help with large packfiles.

AFAICT there is a static allocation of one pkt-line (of maximum size), such that the code can read in a full packet and then process

Quoted 11 lines

Quoted 10 lines

If we'd increase the packet size we'd need the static buffer to be 1MB, which sounds good for my developer machine. But I suspect it may be too much for people using git on embedded devices?

pack files larger than 64k are put into multiple pkt-lines, which is not a big deal, as the overhead of 4bytes per 64k is negligible. (also there is progress information in the side channel, which would come in as a special packet in between real packets, such that every 64k transmitted you can update your progress meter; Not sure I feel strongly on fewer progress updates)

Quoted 1 line

Can I request, selfishly from my own platform's (NonStop) performance

heartache, that we don't require 1Mb? We're not embedded on this platform, but there is an optimized message system packet size down at 50Kb that I would like to stay under. Although above that is no problem, there is a significant cost incurred above that size point. And please

sure xread/xwrite are used in any event.

I think that it would be too much of a change to up to 1MB lines at the moment so I'm planning on leaving it right where it is :)

Quoted 9 lines

Quoted 9 lines

Quoted 4 lines

Granted, we're throttled by the network, so it might not matter.
Would it be interesting to have a 5 digit prefix with parts of the high bits of first digit being flags ?
Or is this too radical of a change?

What would the flags be for?

As an alternative we could put the channel number in one byte, such that we can have a side channel not just while streaming the pack but

the time.

Quoted 1 line

Quoted 1 line

(Again, not sure if that buys a lot for us)

Quoted 2 lines

I think that it would be too much of a change to up to 1MB lines at the moment so I'm planning on leaving it right where it is :)

In for a kilo, in for a tonne. Once we're way up there, it's not a problem or much of a difference. :)