Skip to contentSkip to conversation

protocol version 2

PATCH Superseded Brandon Williams 13 participants 362 messages Jan 3, 2018 – Mar 14, 2018 Page 3 of 3

Subsystems (2)

v1 of 6 Superseded Latest is v6 →

lore
Share & tools
lore
#275 lore raw

Re: [PATCH v3 35/35] remote-curl: don't request v2 when pushing

In reply to Jonathan Tan
Quoted 17 lines · Jonathan Tan

t/t5702-protocol-v2.sh test_expect_success 'fetch with http:// using protocol v2' ' · line 224

Quoted 11 lines · On Tue, 6 Feb 2018 17:13:12 -0800 Brandon Williams <[email protected]> wrote:
t/t5702-protocol-v2.sh +11 -0
+test_expect_success 'push with http:// and a config of v2 does not request v2' '
+	# Till v2 for push is designed, make sure that if a client has
+	# protocol.version configured to use v2, that the client instead falls
+	# back and uses v0.
+
+	test_commit -C http_child three &&
+
+	# Push to another branch, as the target repository has the
+	# master branch checked out and we cannot push into it.
+	GIT_TRACE_PACKET=1 git -C http_child -c protocol.version=1 \
+		push origin HEAD:client_branch && 2>log &&

Should it be protocol.version=2? Also, two double ampersands?

Quoted 3 lines

Also, optionally, it might be better to do GIT_TRACE_PACKET="$(pwd)/log", so that it does not get mixed with other stderr output.

Wow thanks for catching that, let me fix that.

Signature · 1 line
Brandon Williams
#276 lore raw

Re: [PATCH v3 35/35] remote-curl: don't request v2 when pushing

Quoted 18 lines · Brandon Williams
Quoted 17 lines · Jonathan Tan

t/t5702-protocol-v2.sh test_expect_success 'fetch with http:// using protocol v2' ' · line 224

Quoted 11 lines · On Tue, 6 Feb 2018 17:13:12 -0800 Brandon Williams <[email protected]> wrote:
t/t5702-protocol-v2.sh +11 -0
+test_expect_success 'push with http:// and a config of v2 does not request v2' '
+	# Till v2 for push is designed, make sure that if a client has
+	# protocol.version configured to use v2, that the client instead falls
+	# back and uses v0.
+
+	test_commit -C http_child three &&
+
+	# Push to another branch, as the target repository has the
+	# master branch checked out and we cannot push into it.
+	GIT_TRACE_PACKET=1 git -C http_child -c protocol.version=1 \
+		push origin HEAD:client_branch && 2>log &&

Should it be protocol.version=2? Also, two double ampersands?

Quoted 5 lines
Quoted 3 lines

Also, optionally, it might be better to do GIT_TRACE_PACKET="$(pwd)/log", so that it does not get mixed with other stderr output.

Wow thanks for catching that, let me fix that.

I like setting the log via "$(pwd)/log" but it turns out that this appends to the file if it already exists, which means the previous tests need to do some cleanup. This is actually probably preferable anyway.

Quoted 3 lines
Signature · 2 lines
-- 
Brandon Williams
Signature · 1 line
Brandon Williams
Reviewed-by on the v3 series
#277 lore raw
In reply to Brandon Williams
Quoted 105 lines · Brandon Williams

Changes in v3:
* There were some comments about how the protocol should be designed stateless first. I've made this change and instead of having to supply the stateless-rpc=true capability to force stateless behavior, the protocol just requires all commands to be stateless.

* Added some patches towards the end of the series to force the client to not request to use protocol v2 when pushing (even if configured to use v2). This is to ease the roll-out process of a push command in protocol v2. This way when servers gain the ability to accept pushing in v2 (and they start responding using v2 when requests are sent to the git-receive-pack endpoint) that clients who still don't understand how to push using v2 won't request to use v2 and then die when they recognize that the server does indeed know how to accept a push under v2.

* I implemented the shallow feature for fetch. This feature encapsulates the existing functionality of all the shallow/deepen capabilities in v0. So now a server can process shallow requests.

* Various other small tweaks that I can't remember :)

After all of that I think the series is in a pretty good state, baring any more critical reviewing feedback.

Thanks!

Brandon Williams (35):
pkt-line: introduce packet_read_with_status
pkt-line: introduce struct packet_reader
pkt-line: add delim packet support
upload-pack: convert to a builtin
upload-pack: factor out processing lines
transport: use get_refs_via_connect to get refs
connect: convert get_remote_heads to use struct packet_reader
connect: discover protocol version outside of get_remote_heads
transport: store protocol version
protocol: introduce enum protocol_version value protocol_v2
test-pkt-line: introduce a packet-line test helper
serve: introduce git-serve
ls-refs: introduce ls-refs server command
connect: request remote refs using v2
transport: convert get_refs_list to take a list of ref patterns
transport: convert transport_get_remote_refs to take a list of ref
patterns
ls-remote: pass ref patterns when requesting a remote's refs
fetch: pass ref patterns when fetching
push: pass ref patterns when pushing
upload-pack: introduce fetch server command
fetch-pack: perform a fetch using v2
upload-pack: support shallow requests
fetch-pack: support shallow requests
connect: refactor git_connect to only get the protocol version once
connect: don't request v2 when pushing
transport-helper: remove name parameter
transport-helper: refactor process_connect_service
transport-helper: introduce stateless-connect
pkt-line: add packet_buf_write_len function
remote-curl: create copy of the service name
remote-curl: store the protocol version the server responded with
http: allow providing extra headers for http requests
http: don't always add Git-Protocol header
remote-curl: implement stateless-connect command
remote-curl: don't request v2 when pushing

  .gitignore                              |   1 +
  Documentation/technical/protocol-v2.txt | 338 +++++++++++++++++
  Makefile                                |   7 +-
  builtin.h                               |   2 +
  builtin/clone.c                         |   2 +-
  builtin/fetch-pack.c                    |  21 +-
  builtin/fetch.c                         |  14 +-
  builtin/ls-remote.c                     |   7 +-
  builtin/receive-pack.c                  |   6 +
  builtin/remote.c                        |   2 +-
  builtin/send-pack.c                     |  20 +-
  builtin/serve.c                         |  30 ++
  builtin/upload-pack.c                   |  74 ++++
  connect.c                               | 352 +++++++++++++-----
  connect.h                               |   7 +
  fetch-pack.c                            | 319 +++++++++++++++-
  fetch-pack.h                            |   4 +-
  git.c                                   |   2 +
  http.c                                  |  25 +-
  http.h                                  |   2 +
  ls-refs.c                               |  96 +++++
  ls-refs.h                               |   9 +
  pkt-line.c                              | 149 +++++++-
  pkt-line.h                              |  77 ++++
  protocol.c                              |   2 +
  protocol.h                              |   1 +
  remote-curl.c                           | 257 ++++++++++++-
  remote.h                                |   9 +-
  serve.c                                 | 260 +++++++++++++
  serve.h                                 |  15 +
  t/helper/test-pkt-line.c                |  64 ++++
  t/t5701-git-serve.sh                    | 176 +++++++++
  t/t5702-protocol-v2.sh                  | 239 ++++++++++++
  transport-helper.c                      |  84 +++--
  transport-internal.h                    |   4 +-
  transport.c                             | 116 ++++--
  transport.h                             |   9 +-
  upload-pack.c                           | 625 ++++++++++++++++++++++++--------
  upload-pack.h                           |  21 ++
  39 files changed, 3088 insertions(+), 360 deletions(-)
Quoted 12 lines

create mode 100644 Documentation/technical/protocol-v2.txt create mode 100644 builtin/serve.c
create mode 100644 builtin/upload-pack.c
create mode 100644 ls-refs.c
create mode 100644 ls-refs.h
create mode 100644 serve.c
create mode 100644 serve.h
create mode 100644 t/helper/test-pkt-line.c create mode 100755 t/t5701-git-serve.sh
create mode 100755 t/t5702-protocol-v2.sh
create mode 100644 upload-pack.h

I inspected the diff between v2 and v3 and found the changes to be good.

Thanks,
-Stolee

#278 lore raw
In reply to Brandon Williams
Quoted 20 lines · Brandon Williams

Changes in v3:
* There were some comments about how the protocol should be designed stateless first. I've made this change and instead of having to supply the stateless-rpc=true capability to force stateless behavior, the protocol just requires all commands to be stateless.

* Added some patches towards the end of the series to force the client to not request to use protocol v2 when pushing (even if configured to use v2). This is to ease the roll-out process of a push command in protocol v2. This way when servers gain the ability to accept pushing in v2 (and they start responding using v2 when requests are sent to the git-receive-pack endpoint) that clients who still don't understand how to push using v2 won't request to use v2 and then die when they recognize that the server does indeed know how to accept a push under v2.

* I implemented the shallow feature for fetch. This feature encapsulates the existing functionality of all the shallow/deepen capabilities in v0. So now a server can process shallow requests.

Quoted 6 lines

* Various other small tweaks that I can't remember :)

After all of that I think the series is in a pretty good state, baring any more critical reviewing feedback.

Thanks!

I'm hoping to get some more in depth review before I do any more re-rolls, but for those interested I will need to do a re-roll to eliminate the prelude from the http transport. This is the prelude which includes the service line followed by any number of packet lines culminating in a flush-pkt like so:

# service=git-upload-pack
some
other
optional
lines
0000

With this eliminated all transports will be exactly the same, the only difference will be how the protocol is tunneled.

Quoted 94 lines

Brandon Williams (35):
pkt-line: introduce packet_read_with_status
pkt-line: introduce struct packet_reader
pkt-line: add delim packet support
upload-pack: convert to a builtin
upload-pack: factor out processing lines
transport: use get_refs_via_connect to get refs
connect: convert get_remote_heads to use struct packet_reader
connect: discover protocol version outside of get_remote_heads
transport: store protocol version
protocol: introduce enum protocol_version value protocol_v2
test-pkt-line: introduce a packet-line test helper
serve: introduce git-serve
ls-refs: introduce ls-refs server command
connect: request remote refs using v2
transport: convert get_refs_list to take a list of ref patterns
transport: convert transport_get_remote_refs to take a list of ref
patterns
ls-remote: pass ref patterns when requesting a remote's refs
fetch: pass ref patterns when fetching
push: pass ref patterns when pushing
upload-pack: introduce fetch server command
fetch-pack: perform a fetch using v2
upload-pack: support shallow requests
fetch-pack: support shallow requests
connect: refactor git_connect to only get the protocol version once
connect: don't request v2 when pushing
transport-helper: remove name parameter
transport-helper: refactor process_connect_service
transport-helper: introduce stateless-connect
pkt-line: add packet_buf_write_len function
remote-curl: create copy of the service name
remote-curl: store the protocol version the server responded with
http: allow providing extra headers for http requests
http: don't always add Git-Protocol header
remote-curl: implement stateless-connect command
remote-curl: don't request v2 when pushing

 .gitignore                              |   1 +
 Documentation/technical/protocol-v2.txt | 338 +++++++++++++++++
 Makefile                                |   7 +-
 builtin.h                               |   2 +
 builtin/clone.c                         |   2 +-
 builtin/fetch-pack.c                    |  21 +-
 builtin/fetch.c                         |  14 +-
 builtin/ls-remote.c                     |   7 +-
 builtin/receive-pack.c                  |   6 +
 builtin/remote.c                        |   2 +-
 builtin/send-pack.c                     |  20 +-
 builtin/serve.c                         |  30 ++
 builtin/upload-pack.c                   |  74 ++++
 connect.c                               | 352 +++++++++++++-----
 connect.h                               |   7 +
 fetch-pack.c                            | 319 +++++++++++++++-
 fetch-pack.h                            |   4 +-
 git.c                                   |   2 +
 http.c                                  |  25 +-
 http.h                                  |   2 +
 ls-refs.c                               |  96 +++++
 ls-refs.h                               |   9 +
 pkt-line.c                              | 149 +++++++-
 pkt-line.h                              |  77 ++++
 protocol.c                              |   2 +
 protocol.h                              |   1 +
 remote-curl.c                           | 257 ++++++++++++-
 remote.h                                |   9 +-
 serve.c                                 | 260 +++++++++++++
 serve.h                                 |  15 +
 t/helper/test-pkt-line.c                |  64 ++++
 t/t5701-git-serve.sh                    | 176 +++++++++
 t/t5702-protocol-v2.sh                  | 239 ++++++++++++
 transport-helper.c                      |  84 +++--
 transport-internal.h                    |   4 +-
 transport.c                             | 116 ++++--
 transport.h                             |   9 +-
 upload-pack.c                           | 625 ++++++++++++++++++++++++--------
 upload-pack.h                           |  21 ++
 39 files changed, 3088 insertions(+), 360 deletions(-)
 create mode 100644 Documentation/technical/protocol-v2.txt
 create mode 100644 builtin/serve.c
 create mode 100644 builtin/upload-pack.c
 create mode 100644 ls-refs.c
 create mode 100644 ls-refs.h
 create mode 100644 serve.c
 create mode 100644 serve.h
 create mode 100644 t/helper/test-pkt-line.c
 create mode 100755 t/t5701-git-serve.sh
 create mode 100755 t/t5702-protocol-v2.sh
 create mode 100644 upload-pack.h
Signature · 1 line
2.16.0.rc1.238.g530d649a79-goog
Signature · 1 line
Brandon Williams
#279 lore raw

v4 cover protocol version 2

In reply to Brandon Williams

Lots of changes since v3 (well more than between v2 and v3). Thanks for all of the reviews on the last round, the series is getting more polished.

* Eliminated the "# service" line from the response from an HTTP server. This means that the response to a v2 request is exactly the same regardless of which transport you use! Docs for this have been added as well.
* Changed how ref-patterns work with the ls-refs command. Instead of using wildmatch all patterns must either match exactly or they can contain a single '*' character at the end to mean that the prefix must match. Docs for this have also been added.
* Lots of updates to the docs. Including documenting the stateless-connect remote-helper command used by remote-curl to handle the http transport.
* Fixed a number of bugs with the fetch command, one of which didn't use objects from configured alternates.

Brandon Williams (35):
pkt-line: introduce packet_read_with_status
pkt-line: allow peeking a packet line without consuming it
pkt-line: add delim packet support
upload-pack: convert to a builtin
upload-pack: factor out processing lines
transport: use get_refs_via_connect to get refs
connect: convert get_remote_heads to use struct packet_reader
connect: discover protocol version outside of get_remote_heads
transport: store protocol version
protocol: introduce enum protocol_version value protocol_v2
test-pkt-line: introduce a packet-line test helper
serve: introduce git-serve
ls-refs: introduce ls-refs server command
connect: request remote refs using v2
transport: convert get_refs_list to take a list of ref patterns
transport: convert transport_get_remote_refs to take a list of ref
patterns
ls-remote: pass ref patterns when requesting a remote's refs
fetch: pass ref patterns when fetching
push: pass ref patterns when pushing
upload-pack: introduce fetch server command
fetch-pack: perform a fetch using v2
fetch-pack: support shallow requests
connect: refactor git_connect to only get the protocol version once
connect: don't request v2 when pushing
transport-helper: remove name parameter
transport-helper: refactor process_connect_service
transport-helper: introduce stateless-connect
pkt-line: add packet_buf_write_len function
remote-curl: create copy of the service name
remote-curl: store the protocol version the server responded with
http: allow providing extra headers for http requests
http: don't always add Git-Protocol header
http: eliminate "# service" line when using protocol v2
remote-curl: implement stateless-connect command
remote-curl: don't request v2 when pushing

 .gitignore                              |   1 +
 Documentation/gitremote-helpers.txt     |  32 ++
 Documentation/technical/protocol-v2.txt | 401 +++++++++++++++
 Makefile                                |   7 +-
 builtin.h                               |   2 +
 builtin/clone.c                         |   2 +-
 builtin/fetch-pack.c                    |  20 +-
 builtin/fetch.c                         |  18 +-
 builtin/ls-remote.c                     |  12 +-
 builtin/receive-pack.c                  |   6 +
 builtin/remote.c                        |   2 +-
 builtin/send-pack.c                     |  20 +-
 builtin/serve.c                         |  30 ++
 builtin/upload-pack.c                   |  74 +++
 connect.c                               | 364 ++++++++++----
 connect.h                               |   7 +
 fetch-pack.c                            | 339 ++++++++++++-
 fetch-pack.h                            |   4 +-
 git.c                                   |   2 +
 http-backend.c                          |   8 +-
 http.c                                  |  25 +-
 http.h                                  |   7 +
 ls-refs.c                               | 144 ++++++
 ls-refs.h                               |   9 +
 pkt-line.c                              | 147 +++++-
 pkt-line.h                              |  78 +++
 protocol.c                              |   2 +
 protocol.h                              |   1 +
 refs.c                                  |  14 +
 refs.h                                  |   7 +
 remote-curl.c                           | 278 ++++++++++-
 remote.h                                |  11 +-
 serve.c                                 | 260 ++++++++++
 serve.h                                 |  15 +
 t/helper/test-pkt-line.c                |  64 +++
 t/t5701-git-serve.sh                    | 176 +++++++
 t/t5702-protocol-v2.sh                  | 273 +++++++++++
 transport-helper.c                      |  87 ++--
 transport-internal.h                    |   9 +-
 transport.c                             | 125 +++--
 transport.h                             |  18 +-
 upload-pack.c                           | 616 ++++++++++++++++++------
 upload-pack.h                           |  21 +
 43 files changed, 3370 insertions(+), 368 deletions(-)
 create mode 100644 Documentation/technical/protocol-v2.txt
 create mode 100644 builtin/serve.c
 create mode 100644 builtin/upload-pack.c
 create mode 100644 ls-refs.c
 create mode 100644 ls-refs.h
 create mode 100644 serve.c
 create mode 100644 serve.h
 create mode 100644 t/helper/test-pkt-line.c
 create mode 100755 t/t5701-git-serve.sh
 create mode 100755 t/t5702-protocol-v2.sh
 create mode 100644 upload-pack.h
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#280 lore raw

PATCH v4 1/35 pkt-line: introduce packet_read_with_status

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.

---
 pkt-line.c | 55 ++++++++++++++++++++++++++++++++++++++++--------------
 pkt-line.h | 16 ++++++++++++++++
 2 files changed, 57 insertions(+), 14 deletions(-)
pkt-line.c +41 -14
index 2827ca772..08e5ba44c 100644
@@ -280,28 +280,34 @@ 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);
-	if (len < 0)
+
+	if (len < 0) {
 		die("protocol error: bad line length character: %.4s", linelen);
-	if (!len) {
+	} else if (!len) {
 		packet_trace("0000", 4, 0);
-		return 0;
+		return PACKET_READ_FLUSH;
+	} else if (len < 4) {
+		die("protocol error: bad line length %d", len);
 	}
+
 	len -= 4;
-	if (len >= size)
+	if ((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 +315,28 @@ 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)
+{
+	int pktlen;
+
+	switch (packet_read_with_status(fd, src_buffer, src_len, buffer, size,
+					&pktlen, options)) {
+	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 +16 -0
index 3dad583e2..0be691116 100644
@@ -65,6 +65,22 @@ 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,
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#281 lore raw

Re: [PATCH v4 01/35] pkt-line: introduce packet_read_with_status

Quoted 5 lines · On Wed, 28 Feb 2018 15:22:18 -0800 Brandon Williams <[email protected]> wrote:
pkt-line.c +2 -1
+	if (len < 0) {
 		die("protocol error: bad line length character: %.4s", linelen);
-	if (!len) {
+	} else if (!len) {
 		packet_trace("0000", 4, 0);

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

Quoted 10 lines
pkt-line.c +5 -2
-		return 0;
+		return PACKET_READ_FLUSH;
+	} else if (len < 4) {
+		die("protocol error: bad line length %d", len);
 	}
+
 	len -= 4;
-	if (len >= size)
+	if ((unsigned)len >= size)
 		die("protocol error: bad line length %d", len);

The cast to unsigned is safe, since len was at least 4 before "len -= 4". I can't think of a better way to write this to make that more obvious, though.

Quoted 5 lines
pkt-line.h +5 -0
+/*
+ * 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'.

pkt-line.h int write_packetized_from_buf(const char *src_in, size_t len, int fd_out); · line 82

Quoted 10 lines
pkt-line.h +10 -0
+ */
+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);

jrnieder said in [1], referring to the definition of enum packet_read_status:

Quoted 2 lines

nit: do any callers treat the return value as a number? It would be less magical if the numbering were left to the compiler (0, 1, 2).

I checked the result of the entire patch set and the only callers seem to be packet_read() (modified in this patch) and the soon-to-be-introduced packet_reader_read(). So not only can the numbering be left to the compiler, this function can (and should) be marked static as well (and the enum definition moved to .c), since I think that future development should be encouraged to use packet_reader.

The commit message would also thus need to be rewritten, since this becomes more of a refactoring into a function with a more precisely specified return type, to be used both by the existing packet_read() and a soon-to-be-introduced packet_reader_read().

[1] https://public-inbox.org/git/[email protected]/

#282 lore raw

Re: [PATCH v4 01/35] pkt-line: introduce packet_read_with_status

Quoted 33 lines · Jonathan Tan
Quoted 5 lines · On Wed, 28 Feb 2018 15:22:18 -0800 Brandon Williams <[email protected]> wrote:
pkt-line.c +2 -1
+	if (len < 0) {
 		die("protocol error: bad line length character: %.4s", linelen);
-	if (!len) {
+	} else if (!len) {
 		packet_trace("0000", 4, 0);

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

Quoted 10 lines
pkt-line.c +5 -2
-		return 0;
+		return PACKET_READ_FLUSH;
+	} else if (len < 4) {
+		die("protocol error: bad line length %d", len);
 	}
+
 	len -= 4;
-	if (len >= size)
+	if ((unsigned)len >= size)
 		die("protocol error: bad line length %d", len);

The cast to unsigned is safe, since len was at least 4 before "len -= 4". I can't think of a better way to write this to make that more obvious, though.

Quoted 10 lines
pkt-line.h +10 -0
+/*
+ * 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,
Quoted 11 lines

pkt-line.h int write_packetized_from_buf(const char *src_in, size_t len, int fd_out); · line 82

Quoted 5 lines
pkt-line.h +5 -0
+};
+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);

jrnieder said in [1], referring to the definition of enum packet_read_status:

Quoted 2 lines

nit: do any callers treat the return value as a number? It would be less magical if the numbering were left to the compiler (0, 1, 2).

yeah i'll do this.

Quoted 7 lines

I checked the result of the entire patch set and the only callers seem to be packet_read() (modified in this patch) and the soon-to-be-introduced packet_reader_read(). So not only can the numbering be left to the compiler, this function can (and should) be marked static as well (and the enum definition moved to .c), since I think that future development should be encouraged to use packet_reader.

The enum definition can't be moved as its needed outside this file.

Quoted 7 lines

The commit message would also thus need to be rewritten, since this becomes more of a refactoring into a function with a more precisely specified return type, to be used both by the existing packet_read() and a soon-to-be-introduced packet_reader_read().

[1] https://public-inbox.org/git/[email protected]/

Signature · 1 line
Brandon Williams
#283 lore raw

PATCH v4 2/35 pkt-line: allow peeking a packet line without consuming it

In reply to Brandon Williams

Sometimes it is advantageous to be able to peek the next packet line without consuming it (e.g. to be able to determine the protocol version a server is speaking). In order to do that introduce 'struct packet_reader' which is an abstraction around the normal packet reading logic. This enables a caller to be able to peek a single line at a time using 'packet_reader_peek()' and having a caller consume a line by calling 'packet_reader_read()'.

---
 pkt-line.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 pkt-line.h | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+)
pkt-line.c +59 -0
index 08e5ba44c..6307fa4a3 100644
@@ -404,3 +404,62 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)
 	}
 	return sb_out->len - orig_len;
 }
+
+/* Packet Reader Functions */
+void packet_reader_init(struct packet_reader *reader, int fd,
+			char *src_buffer, size_t src_len,
+			int options)
+{
+	memset(reader, 0, sizeof(*reader));
+
+	reader->fd = fd;
+	reader->src_buffer = src_buffer;
+	reader->src_len = src_len;
+	reader->buffer = packet_buffer;
+	reader->buffer_size = sizeof(packet_buffer);
+	reader->options = options;
+}
+
+enum packet_read_status packet_reader_read(struct packet_reader *reader)
+{
+	if (reader->line_peeked) {
+		reader->line_peeked = 0;
+		return reader->status;
+	}
+
+	reader->status = packet_read_with_status(reader->fd,
+						 &reader->src_buffer,
+						 &reader->src_len,
+						 reader->buffer,
+						 reader->buffer_size,
+						 &reader->pktlen,
+						 reader->options);
+
+	switch (reader->status) {
+	case PACKET_READ_EOF:
+		reader->pktlen = -1;
+		reader->line = NULL;
+		break;
+	case PACKET_READ_NORMAL:
+		reader->line = reader->buffer;
+		break;
+	case PACKET_READ_FLUSH:
+		reader->pktlen = 0;
+		reader->line = NULL;
+		break;
+	}
+
+	return reader->status;
+}
+
+enum packet_read_status packet_reader_peek(struct packet_reader *reader)
+{
+	/* Only allow peeking a single line */
+	if (reader->line_peeked)
+		return reader->status;
+
+	/* Peek a line by reading it and setting peeked flag */
+	packet_reader_read(reader);
+	reader->line_peeked = 1;
+	return reader->status;
+}
pkt-line.h +58 -0

64 lines not on this page · raw message

Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#284 lore raw

Re: [PATCH v4 02/35] pkt-line: allow peeking a packet line without consuming it

Quoted 21 lines · Brandon Williams
pkt-line.c +21 -0
+enum packet_read_status packet_reader_read(struct packet_reader *reader)
+{
+	if (reader->line_peeked) {
+		reader->line_peeked = 0;
+		return reader->status;
+	}
+
+	reader->status = packet_read_with_status(reader->fd,
+						 &reader->src_buffer,
+						 &reader->src_len,
+						 reader->buffer,
+						 reader->buffer_size,
+						 &reader->pktlen,
+						 reader->options);
+
+	switch (reader->status) {
+	case PACKET_READ_EOF:
+		reader->pktlen = -1;
+		reader->line = NULL;
+		break;
+	case PACKET_READ_NORMAL:

pkt-line.c ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out) · line 453

Quoted 10 lines
pkt-line.c +10 -0
+		reader->line = reader->buffer;
+		break;
+	case PACKET_READ_FLUSH:
+		reader->pktlen = 0;
+		reader->line = NULL;
+		break;
+	}
+
+	return reader->status;
+}

With the way _peek() interface interacts with the reader instance (which by the way I find is well designed), it is understandable that we want almost everything available in reader's fields, but having to manually clear pktlen field upon non NORMAL status feels a bit strange.

Perhaps that is because the underlying packet_read_with_status() does not set *pktlen in these cases? Shouldn't it be doing that so the caller does not have to?

A similar comment applies for reader's line field. In priniciple, as the status field is part of a reader, it does not have to exist as a separate field, i.e.

	#define line_of(reader) \
		((reader).status == PACKET_READ_NORMAL ? \
		(reader).buffer : NULL)

can be used to as substitute for it. I guess it depends on how the actual callers wants to use this interface.

#285 lore raw

Re: [PATCH v4 02/35] pkt-line: allow peeking a packet line without consuming it

Quoted 40 lines · Junio C Hamano
Quoted 21 lines · Brandon Williams
pkt-line.c +21 -0
+enum packet_read_status packet_reader_read(struct packet_reader *reader)
+{
+	if (reader->line_peeked) {
+		reader->line_peeked = 0;
+		return reader->status;
+	}
+
+	reader->status = packet_read_with_status(reader->fd,
+						 &reader->src_buffer,
+						 &reader->src_len,
+						 reader->buffer,
+						 reader->buffer_size,
+						 &reader->pktlen,
+						 reader->options);
+
+	switch (reader->status) {
+	case PACKET_READ_EOF:
+		reader->pktlen = -1;
+		reader->line = NULL;
+		break;
+	case PACKET_READ_NORMAL:

pkt-line.c ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out) · line 453

Quoted 10 lines
pkt-line.c +10 -0
+		reader->line = reader->buffer;
+		break;
+	case PACKET_READ_FLUSH:
+		reader->pktlen = 0;
+		reader->line = NULL;
+		break;
+	}
+
+	return reader->status;
+}

With the way _peek() interface interacts with the reader instance (which by the way I find is well designed), it is understandable that we want almost everything available in reader's fields, but having to manually clear pktlen field upon non NORMAL status feels a bit strange.

Quoted 3 lines

Perhaps that is because the underlying packet_read_with_status() does not set *pktlen in these cases? Shouldn't it be doing that so the caller does not have to?

That's true, I'll fix that.

Signature · 1 line
Brandon Williams
#286 lore raw

PATCH v4 3/35 pkt-line: add delim packet support

In reply to Brandon Williams

One of the design goals of protocol-v2 is to improve the semantics of flush packets. Currently in protocol-v1, flush packets are used both to indicate a break in a list of packet lines as well as an indication that one side has finished speaking. This makes it particularly difficult to implement proxies as a proxy would need to completely understand git protocol instead of simply looking for a flush packet.

To do this, introduce the special deliminator packet '0001'. A delim packet can then be used as a deliminator between lists of packet lines while flush packets can be reserved to indicate the end of a response.

Documentation for how this packet will be used in protocol v2 will included in a future patch.

---
 pkt-line.c | 17 +++++++++++++++++
 pkt-line.h |  3 +++
 2 files changed, 20 insertions(+)
pkt-line.c +17 -0
index 6307fa4a3..87a24bd17 100644
@@ -91,6 +91,12 @@ void packet_flush(int fd)
 	write_or_die(fd, "0000", 4);
 }
 
+void packet_delim(int fd)
+{
+	packet_trace("0001", 4, 1);
+	write_or_die(fd, "0001", 4);
+}
+
 int packet_flush_gently(int fd)
 {
 	packet_trace("0000", 4, 1);
@@ -105,6 +111,12 @@ void packet_buf_flush(struct strbuf *buf)
 	strbuf_add(buf, "0000", 4);
 }
 
+void packet_buf_delim(struct strbuf *buf)
+{
+	packet_trace("0001", 4, 1);
+	strbuf_add(buf, "0001", 4);
+}
+
 static void set_packet_header(char *buf, const int size)
 {
 	static char hexchar[] = "0123456789abcdef";
@@ -298,6 +310,9 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 	} else if (!len) {
 		packet_trace("0000", 4, 0);
 		return PACKET_READ_FLUSH;
+	} else if (len == 1) {
+		packet_trace("0001", 4, 0);
+		return PACKET_READ_DELIM;
 	} else if (len < 4) {
 		die("protocol error: bad line length %d", len);
 	}
@@ -331,6 +346,7 @@ int packet_read(int fd, char **src_buffer, size_t *src_len,
 		break;
 	case PACKET_READ_NORMAL:
 		break;
+	case PACKET_READ_DELIM:
 	case PACKET_READ_FLUSH:
 		pktlen = 0;
 		break;
@@ -443,6 +459,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
 	case PACKET_READ_NORMAL:
 		reader->line = reader->buffer;
 		break;
+	case PACKET_READ_DELIM:
 	case PACKET_READ_FLUSH:
 		reader->pktlen = 0;
 		reader->line = NULL;
pkt-line.h +3 -0
index f2edfae9a..3f836f01a 100644
@@ -20,8 +20,10 @@
  * side can't, we stay with pure read/write interfaces.
  */
 void packet_flush(int fd);
+void packet_delim(int fd);
 void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
 void packet_buf_flush(struct strbuf *buf);
+void packet_buf_delim(struct strbuf *buf);
 void packet_write(int fd_out, const char *buf, size_t size);
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
 int packet_flush_gently(int fd);
@@ -75,6 +77,7 @@ enum packet_read_status {
 	PACKET_READ_EOF = -1,
 	PACKET_READ_NORMAL,
 	PACKET_READ_FLUSH,
+	PACKET_READ_DELIM,
 };
 enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 						size_t *src_len, char *buffer,
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#287 lore raw

Re: [PATCH v4 03/35] pkt-line: add delim packet support

Quoted 6 lines · Brandon Williams

One of the design goals of protocol-v2 is to improve the semantics of flush packets. Currently in protocol-v1, flush packets are used both to indicate a break in a list of packet lines as well as an indication that one side has finished speaking. This makes it particularly difficult to implement proxies as a proxy would need to completely understand git protocol instead of simply looking for a flush packet.

Good ;-) Yes, this has been one of the largest gripe about the smart-http support code we have.

#288 lore raw

Re: [PATCH v4 03/35] pkt-line: add delim packet support

Quoted 11 lines · Junio C Hamano
Quoted 6 lines · Brandon Williams

One of the design goals of protocol-v2 is to improve the semantics of flush packets. Currently in protocol-v1, flush packets are used both to indicate a break in a list of packet lines as well as an indication that one side has finished speaking. This makes it particularly difficult to implement proxies as a proxy would need to completely understand git protocol instead of simply looking for a flush packet.

Good ;-) Yes, this has been one of the largest gripe about the smart-http support code we have.

Hmph, strictly speaking, the "delim" does not have to be a part of how packetized stream is defined. As long as we stop abusing flush as "This is merely an end of one segment of what I say." and make it always mean "I am done speaking, it is your turn.", the application payload can define its own syntax to separate groups of packets.

I do not mind having this "delim" thing defined at the protocol level too much, though.

#289 lore raw

Re: [PATCH v4 03/35] pkt-line: add delim packet support

Quoted 11 lines · Junio C Hamano
Quoted 9 lines · Junio C Hamano
Quoted 6 lines · Brandon Williams

One of the design goals of protocol-v2 is to improve the semantics of flush packets. Currently in protocol-v1, flush packets are used both to indicate a break in a list of packet lines as well as an indication that one side has finished speaking. This makes it particularly difficult to implement proxies as a proxy would need to completely understand git protocol instead of simply looking for a flush packet.

Quoted 8 lines
Quoted 2 lines

Good ;-) Yes, this has been one of the largest gripe about the smart-http support code we have.

Hmph, strictly speaking, the "delim" does not have to be a part of how packetized stream is defined. As long as we stop abusing flush as "This is merely an end of one segment of what I say." and make it always mean "I am done speaking, it is your turn.", the application payload can define its own syntax to separate groups of packets.

Thanks actually a good point. We could just as easily have the delim packet to be an empty packet-line "0004" or something like that.

Quoted 3 lines

I do not mind having this "delim" thing defined at the protocol level too much, though.

Signature · 1 line
Brandon Williams
#290 lore raw

Re: [PATCH v4 03/35] pkt-line: add delim packet support

Quoted 10 lines · Brandon Williams

On 03/01, Junio C Hamano wrote:
...

Quoted 5 lines

Hmph, strictly speaking, the "delim" does not have to be a part of how packetized stream is defined. As long as we stop abusing flush as "This is merely an end of one segment of what I say." and make it always mean "I am done speaking, it is your turn.", the application payload can define its own syntax to separate groups of packets.

Thanks actually a good point. We could just as easily have the delim packet to be an empty packet-line "0004" or something like that.

Yes. As long as there is an easy and obvious "cannot be a value" constant, you can use it as a delimiter defined at the application level. For example, your command-request uses delim, like so:

+    request = empty-request | command-request
+    empty-request = flush-pkt
+    command-request = command
+		      capability-list
+		      (command-args)
+		      flush-pkt

+ command = PKT-LINE("command=" key LF)
+ command-args = delim-pkt
+ *PKT-Line(arg LF)

to mark the end of cap list, but if an empty packet does not make sense as a member of a cap list and a commmand args list, then an empty packet between cap list and command arg can be used instead. A protocol-ignorant proxy can still work just fine.

Having a defined delim at the protocol level is often convenient, of course, but once the application starts calling for multi-level delimiters (i.e. maybe there are chapters and sections inside each chapter in a single request message), it would not be sufficient to define a single delim packet type. The application layer needs to define its own convention (e.g. if no "empty" section is allowed, then "two consecutive delim is a chapter break; one delim is a section break" can become a viable way to emulate multi-level delimiters).

#291 lore raw

PATCH v4 4/35 upload-pack: convert to a builtin

In reply to 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.

---
 Makefile              |   3 +-
 builtin.h             |   1 +
 builtin/upload-pack.c |  67 ++++++++++++++++++++++++++
 git.c                 |   1 +
 upload-pack.c         | 107 ++++++++++--------------------------------
 upload-pack.h         |  13 +++++
 6 files changed, 109 insertions(+), 83 deletions(-)
 create mode 100644 builtin/upload-pack.c
 create mode 100644 upload-pack.h
Makefile +2 -1
index 1a9b23b67..b7ccc05fa 100644
@@ -639,7 +639,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
@@ -909,6 +908,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
@@ -1026,6 +1026,7 @@ BUILTIN_OBJS += builtin/update-index.o
 BUILTIN_OBJS += builtin/update-ref.o
 BUILTIN_OBJS += builtin/update-server-info.o
 BUILTIN_OBJS += builtin/upload-archive.o
+BUILTIN_OBJS += builtin/upload-pack.o
 BUILTIN_OBJS += builtin/var.o
 BUILTIN_OBJS += builtin/verify-commit.o
 BUILTIN_OBJS += builtin/verify-pack.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);
builtin/upload-pack.c new file +67 -0

67 lines not on this page · raw message

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 +25 -82

145 lines not on this page · raw message

upload-pack.h new file +13 -0
new file mode 100644
index 000000000..a71e4dc7e
@@ -0,0 +1,13 @@
+#ifndef UPLOAD_PACK_H
+#define UPLOAD_PACK_H
+
+struct upload_pack_options {
+	int stateless_rpc;
+	int advertise_refs;
+	unsigned int timeout;
+	int daemon_mode;
+};
+
+void upload_pack(struct upload_pack_options *options);
+
+#endif /* UPLOAD_PACK_H */
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#292 lore raw

Re: [PATCH v4 04/35] upload-pack: convert to a builtin

Quoted 4 lines · On Wed, 28 Feb 2018 15:22:21 -0800 Brandon Williams <[email protected]> wrote:

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

I suggested updating the commit message in my previous review [1], but I understand that my comment might have been lost in the ensuing long discussion.

[1] https://public-inbox.org/git/[email protected]/

#293 lore raw

Re: [PATCH v4 04/35] upload-pack: convert to a builtin

Quoted 6 lines · Jonathan Tan
Quoted 3 lines · On Wed, 28 Feb 2018 15:22:21 -0800 Brandon Williams <[email protected]> wrote:

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

Quoted 7 lines
Quoted 1 line

I suggested updating the commit message in my previous review [1], but I understand that my comment might have been lost in the ensuing long discussion.

[1] https://public-inbox.org/git/[email protected]/

Your suggested change to the commit msg isn't quite accurate as you can already run "git-upload-pack --help" today.

Signature · 1 line
Brandon Williams
#294 lore raw

PATCH v4 5/35 upload-pack: factor out processing lines

In reply to Brandon Williams

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.

---
 upload-pack.c | 113 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 74 insertions(+), 39 deletions(-)
upload-pack.c +74 -39
index 2ad73a98b..1e8a9e1ca 100644
@@ -724,6 +724,75 @@ static void deepen_by_rev_list(int ac, const char **av,
 	packet_flush(1);
 }
 
+static int process_shallow(const char *line, struct object_array *shallows)
+{
+	const char *arg;
+	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)
+			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 = (int)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;
@@ -745,49 +814,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, "
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#295 lore raw

Re: [PATCH v4 05/35] upload-pack: factor out processing lines

Quoted 4 lines · Brandon Williams

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.

These little functions that still require their incoming data to begin with fixed prefixes feels a bit strange way to refactor the logic for later reuse (when I imagine "reuse", the first use case that comes to my mind is "this data source our new code reads from gives the same data as the old 'shallow' packet used to give, but in a different syntax"---so I'd restructure the code in such a way that the caller figures out the syntax part and the called helper just groks the "information contents" unwrapped from the surface syntax; the syntax may be different in the new codepath but once unwrapped, the "information contents" to be processed would not be different hence we can reuse the helper).

IOW, I would have expected the caller to be not like this:

Quoted 8 lines
upload-pack.c +0 -8
-		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;

upload-pack.c static void receive_needs(void) · line 820

Quoted 10 lines
upload-pack.c +2 -6
-			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 (process_deepen(line, &depth))
 			continue;
		...

but more like

		if (skip_prefix(line, "shallow ", &arg) {
			process_shallow(arg, &shallows);
			continue;
		}
		if (skip_prefix(line, "deepen ", &arg) {
			process_deepen(arg, &depth);
			continue;
		}
		...

I need to defer the final judgment until I see how they are used, though. It's not too big a deal either way---it just felt "not quite right" to me.

#296 lore raw

Re: [PATCH v4 05/35] upload-pack: factor out processing lines

Quoted 53 lines · Junio C Hamano
Quoted 4 lines · Brandon Williams

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.

These little functions that still require their incoming data to begin with fixed prefixes feels a bit strange way to refactor the logic for later reuse (when I imagine "reuse", the first use case that comes to my mind is "this data source our new code reads from gives the same data as the old 'shallow' packet used to give, but in a different syntax"---so I'd restructure the code in such a way that the caller figures out the syntax part and the called helper just groks the "information contents" unwrapped from the surface syntax; the syntax may be different in the new codepath but once unwrapped, the "information contents" to be processed would not be different hence we can reuse the helper).

IOW, I would have expected the caller to be not like this:

Quoted 8 lines
upload-pack.c +0 -8
-		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;

upload-pack.c static void receive_needs(void) · line 820

Quoted 10 lines
upload-pack.c +2 -6
-			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 (process_deepen(line, &depth))
 			continue;
		...

but more like

		if (skip_prefix(line, "shallow ", &arg) {
			process_shallow(arg, &shallows);
			continue;
		}
		if (skip_prefix(line, "deepen ", &arg) {
			process_deepen(arg, &depth);
			continue;
		}
		...
Quoted 3 lines

I need to defer the final judgment until I see how they are used, though. It's not too big a deal either way---it just felt "not quite right" to me.

This is actually a really good point (and maybe the same point stefan was trying to make on an old revision of this series). I think it makes much more sense to refactor the code to have a structure like you've outlined. I'll fix this for the next version.

Signature · 1 line
Brandon Williams
#297 lore raw

Re: [PATCH v4 05/35] upload-pack: factor out processing lines

Quoted 54 lines · Brandon Williams
Quoted 53 lines · Junio C Hamano
Quoted 4 lines · Brandon Williams

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.

These little functions that still require their incoming data to begin with fixed prefixes feels a bit strange way to refactor the logic for later reuse (when I imagine "reuse", the first use case that comes to my mind is "this data source our new code reads from gives the same data as the old 'shallow' packet used to give, but in a different syntax"---so I'd restructure the code in such a way that the caller figures out the syntax part and the called helper just groks the "information contents" unwrapped from the surface syntax; the syntax may be different in the new codepath but once unwrapped, the "information contents" to be processed would not be different hence we can reuse the helper).

IOW, I would have expected the caller to be not like this:

Quoted 8 lines
upload-pack.c +0 -8
-		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;

upload-pack.c static void receive_needs(void) · line 820

Quoted 10 lines
upload-pack.c +2 -6
-			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 (process_deepen(line, &depth))
 			continue;
		...

but more like

		if (skip_prefix(line, "shallow ", &arg) {
			process_shallow(arg, &shallows);
			continue;
		}
		if (skip_prefix(line, "deepen ", &arg) {
			process_deepen(arg, &depth);
			continue;
		}
		...
Quoted 8 lines
Quoted 3 lines

I need to defer the final judgment until I see how they are used, though. It's not too big a deal either way---it just felt "not quite right" to me.

This is actually a really good point (and maybe the same point stefan was trying to make on an old revision of this series). I think it makes much more sense to refactor the code to have a structure like you've outlined. I'll fix this for the next version.

And then I started writing the code and now I don't know which I prefer. The issue is that its for processing a line which has some well defined structure and moving the check for "shallow " away from the rest of the code which does the processing makes it a little less clear how that shallow line is to be defined.

Signature · 1 line
Brandon Williams
#298 lore raw

PATCH v4 6/35 transport: use get_refs_via_connect to get refs

In reply to Brandon Williams

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()'.

---
 transport.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)
transport.c +4 -14
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);
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#299 lore raw

Re: [PATCH v4 06/35] transport: use get_refs_via_connect to get refs

Quoted 8 lines · Brandon Williams

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()'.

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

Nice ;-)

#300 lore raw

PATCH v4 7/35 connect: convert get_remote_heads to use struct packet_reader

In reply to Brandon Williams

In order to allow for better control flow when protocol_v2 is introduced convert 'get_remote_heads()' to use 'struct packet_reader' to read packet lines. This enables a client to be able to peek the first line of a server's response (without consuming it) in order to determine the protocol version its speaking and then passing control to the appropriate handler.

This is needed because the initial response from a server speaking protocol_v0 includes the first ref, while subsequent protocol versions respond with a version line. We want to be able to read this first line without consuming the first ref sent in the protocol_v0 case so that the protocol version the server is speaking can be determined outside of 'get_remote_heads()' in a future patch.

---
 connect.c | 173 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 95 insertions(+), 78 deletions(-)
connect.c +95 -78
index c3a014c5b..c82c90b7c 100644
@@ -48,6 +48,12 @@ int check_ref_type(const struct ref *ref, int flags)
 
 static void die_initial_contact(int unexpected)
 {
+	/*
+	 * A hang-up after seeing some response from the other end
+	 * means that it is unexpected, as we know the other end is
+	 * willing to talk to us.  A hang-up before seeing any
+	 * response does not necessarily mean an ACL problem, though.
+	 */
 	if (unexpected)
 		die(_("The remote end hung up upon initial contact"));
 	else
@@ -56,6 +62,40 @@ static void die_initial_contact(int unexpected)
 		      "and the repository exists."));
 }
 
+static enum protocol_version discover_version(struct packet_reader *reader)
+{
+	enum protocol_version version = protocol_unknown_version;
+
+	/*
+	 * Peek the first line of the server's response to
+	 * determine the protocol version the server is speaking.
+	 */
+	switch (packet_reader_peek(reader)) {
+	case PACKET_READ_EOF:
+		die_initial_contact(0);
+	case PACKET_READ_FLUSH:
+	case PACKET_READ_DELIM:
+		version = protocol_v0;
+		break;
+	case PACKET_READ_NORMAL:
+		version = determine_protocol_version_client(reader->line);
+		break;
+	}
+
+	switch (version) {
+	case protocol_v1:
+		/* Read the peeked version line */
+		packet_reader_read(reader);
+		break;
+	case protocol_v0:
+		break;
+	case protocol_unknown_version:
+		BUG("unknown protocol version");
+	}
+
+	return version;
+}
+
 static void parse_one_symref_info(struct string_list *symref, const char *val, int len)
 {
 	char *sym, *target;
@@ -109,60 +149,21 @@ 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)
+static void process_capabilities(const char *line, int *len)
 {
-	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_PROTOCOL_VERSION 0
-#define EXPECTING_FIRST_REF 1
-#define EXPECTING_REF 2
-#define EXPECTING_SHALLOW 3
-
-/* Returns 1 if packet_buffer is a protocol version pkt-line, 0 otherwise. */
-static int process_protocol_version(void)
-{
-	switch (determine_protocol_version_client(packet_buffer)) {
-	case protocol_v1:
-		return 1;
-	case protocol_v0:
-		return 0;
-	default:
-		die("server is speaking an unknown protocol");
-	}
-}
-
-static void process_capabilities(int *len)
-{
-	int nul_location = strlen(packet_buffer);
+	int nul_location = strlen(line);
 	if (nul_location == *len)
 		return;
-	server_capabilities = xstrdup(packet_buffer + nul_location + 1);
+	server_capabilities = xstrdup(line + nul_location + 1);
 	*len = nul_location;
 }
 
-static int process_dummy_ref(void)
+static int process_dummy_ref(const char *line)
 {
 	struct object_id oid;
 	const char *name;
 
-	if (parse_oid_hex(packet_buffer, &oid, &name))
+	if (parse_oid_hex(line, &oid, &name))
 		return 0;
 	if (*name != ' ')
 		return 0;
@@ -171,20 +172,20 @@ static int process_dummy_ref(void)
 	return !oidcmp(&null_oid, &oid) && !strcmp(name, "capabilities^{}");
 }
 
-static void check_no_capabilities(int len)
+static void check_no_capabilities(const char *line, int len)
 {
-	if (strlen(packet_buffer) != len)
+	if (strlen(line) != len)
 		warning("Ignoring capabilities after first line '%s'",
-			packet_buffer + strlen(packet_buffer));
+			line + strlen(line));
 }
 
-static int process_ref(int len, struct ref ***list, unsigned int flags,
-		       struct oid_array *extra_have)
+static int process_ref(const char *line, int len, 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))
+	if (parse_oid_hex(line, &old_oid, &name))
 		return 0;
 	if (*name != ' ')
 		return 0;
@@ -200,16 +201,17 @@ static int process_ref(int len, struct ref ***list, unsigned int flags,
 		**list = ref;
 		*list = &ref->next;
 	}
-	check_no_capabilities(len);
+	check_no_capabilities(line, len);
 	return 1;
 }
 
-static int process_shallow(int len, struct oid_array *shallow_points)
+static int process_shallow(const char *line, int len,
+			   struct oid_array *shallow_points)
 {
 	const char *arg;
 	struct object_id old_oid;
 
-	if (!skip_prefix(packet_buffer, "shallow ", &arg))
+	if (!skip_prefix(line, "shallow ", &arg))
 		return 0;
 
 	if (get_oid_hex(arg, &old_oid))
@@ -217,10 +219,17 @@ static int process_shallow(int len, struct oid_array *shallow_points)
 	if (!shallow_points)
 		die("repository on the other end cannot be shallow");
 	oid_array_append(shallow_points, &old_oid);
-	check_no_capabilities(len);
+	check_no_capabilities(line, len);
 	return 1;
 }
 
+enum get_remote_heads_state {
+	EXPECTING_FIRST_REF = 0,
+	EXPECTING_REF,
+	EXPECTING_SHALLOW,
+	EXPECTING_DONE,
+};
+
 /*
  * Read all the refs from the other end
  */
@@ -230,47 +239,55 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 			      struct oid_array *shallow_points)
 {
 	struct ref **orig_list = list;
+	int len = 0;
+	enum get_remote_heads_state state = EXPECTING_FIRST_REF;
+	struct packet_reader reader;
+	const char *arg;
 
-	/*
-	 * A hang-up after seeing some response from the other end
-	 * means that it is unexpected, as we know the other end is
-	 * willing to talk to us.  A hang-up before seeing any
-	 * response does not necessarily mean an ACL problem, though.
-	 */
-	int responded = 0;
-	int len;
-	int state = EXPECTING_PROTOCOL_VERSION;
+	packet_reader_init(&reader, in, src_buf, src_len,
+			   PACKET_READ_CHOMP_NEWLINE |
+			   PACKET_READ_GENTLE_ON_EOF);
+
+	discover_version(&reader);
 
 	*list = NULL;
 
-	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(reader.line, "ERR ", &arg))
+				die("remote error: %s", arg);
+			break;
+		case PACKET_READ_FLUSH:
+			state = EXPECTING_DONE;
+			break;
+		case PACKET_READ_DELIM:
+			die("invalid packet");
+		}
+
 		switch (state) {
-		case EXPECTING_PROTOCOL_VERSION:
-			if (process_protocol_version()) {
-				state = EXPECTING_FIRST_REF;
-				break;
-			}
-			state = EXPECTING_FIRST_REF;
-			/* fallthrough */
 		case EXPECTING_FIRST_REF:
-			process_capabilities(&len);
-			if (process_dummy_ref()) {
+			process_capabilities(reader.line, &len);
+			if (process_dummy_ref(reader.line)) {
 				state = EXPECTING_SHALLOW;
 				break;
 			}
 			state = EXPECTING_REF;
 			/* fallthrough */
 		case EXPECTING_REF:
-			if (process_ref(len, &list, flags, extra_have))
+			if (process_ref(reader.line, len, &list, flags, extra_have))
 				break;
 			state = EXPECTING_SHALLOW;
 			/* fallthrough */
 		case EXPECTING_SHALLOW:
-			if (process_shallow(len, shallow_points))
+			if (process_shallow(reader.line, len, shallow_points))
 				break;
-			die("protocol error: unexpected '%s'", packet_buffer);
-		default:
-			die("unexpected state %d", state);
+			die("protocol error: unexpected '%s'", reader.line);
+		case EXPECTING_DONE:
+			break;
 		}
 	}
 
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#301 lore raw

PATCH v4 8/35 connect: discover protocol version outside of get_remote_heads

In reply to Brandon Williams

In order to prepare for the addition of protocol_v2 push the protocol version discovery outside of 'get_remote_heads()'. This will allow for keeping the logic for processing the reference advertisement for protocol_v1 and protocol_v0 separate from the logic for protocol_v2.

---
 builtin/fetch-pack.c | 16 +++++++++++++++-
 builtin/send-pack.c  | 17 +++++++++++++++--
 connect.c            | 27 ++++++++++-----------------
 connect.h            |  3 +++
 remote-curl.c        | 20 ++++++++++++++++++--
 remote.h             |  5 +++--
 transport.c          | 24 +++++++++++++++++++-----
 7 files changed, 83 insertions(+), 29 deletions(-)
builtin/fetch-pack.c +15 -1
index 366b9d13f..85d4faf76 100644
@@ -4,6 +4,7 @@
 #include "remote.h"
 #include "connect.h"
 #include "sha1-array.h"
+#include "protocol.h"
 
 static const char fetch_pack_usage[] =
 "git fetch-pack [--all] [--stdin] [--quiet | -q] [--keep | -k] [--thin] "
@@ -52,6 +53,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	struct fetch_pack_args args;
 	struct oid_array shallow = OID_ARRAY_INIT;
 	struct string_list deepen_not = STRING_LIST_INIT_DUP;
+	struct packet_reader reader;
 
 	packet_trace_identity("fetch-pack");
 
@@ -193,7 +195,19 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 		if (!conn)
 			return args.diag_url ? 0 : 1;
 	}
-	get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
+
+	packet_reader_init(&reader, fd[0], NULL, 0,
+			   PACKET_READ_CHOMP_NEWLINE |
+			   PACKET_READ_GENTLE_ON_EOF);
+
+	switch (discover_version(&reader)) {
+	case protocol_v1:
+	case protocol_v0:
+		get_remote_heads(&reader, &ref, 0, NULL, &shallow);
+		break;
+	case protocol_unknown_version:
+		BUG("unknown protocol version");
+	}
 
 	ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
 			 &shallow, pack_lockfile_ptr);
builtin/send-pack.c +15 -2
index fc4f0bb5f..83cb125a6 100644
@@ -14,6 +14,7 @@
 #include "sha1-array.h"
 #include "gpg-interface.h"
 #include "gettext.h"
+#include "protocol.h"
 
 static const char * const send_pack_usage[] = {
 	N_("git send-pack [--all | --mirror] [--dry-run] [--force] "
@@ -154,6 +155,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	int progress = -1;
 	int from_stdin = 0;
 	struct push_cas_option cas = {0};
+	struct packet_reader reader;
 
 	struct option options[] = {
 		OPT__VERBOSITY(&verbose),
@@ -256,8 +258,19 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 			args.verbose ? CONNECT_VERBOSE : 0);
 	}
 
-	get_remote_heads(fd[0], NULL, 0, &remote_refs, REF_NORMAL,
-			 &extra_have, &shallow);
+	packet_reader_init(&reader, fd[0], NULL, 0,
+			   PACKET_READ_CHOMP_NEWLINE |
+			   PACKET_READ_GENTLE_ON_EOF);
+
+	switch (discover_version(&reader)) {
+	case protocol_v1:
+	case protocol_v0:
+		get_remote_heads(&reader, &remote_refs, REF_NORMAL,
+				 &extra_have, &shallow);
+		break;
+	case protocol_unknown_version:
+		BUG("unknown protocol version");
+	}
 
 	transport_verify_remote_names(nr_refspecs, refspecs);
 
connect.c +10 -17
index c82c90b7c..0b111e62d 100644
@@ -62,7 +62,7 @@ static void die_initial_contact(int unexpected)
 		      "and the repository exists."));
 }
 
-static enum protocol_version discover_version(struct packet_reader *reader)
+enum protocol_version discover_version(struct packet_reader *reader)
 {
 	enum protocol_version version = protocol_unknown_version;
 
@@ -233,7 +233,7 @@ enum get_remote_heads_state {
 /*
  * Read all the refs from the other end
  */
-struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
+struct ref **get_remote_heads(struct packet_reader *reader,
 			      struct ref **list, unsigned int flags,
 			      struct oid_array *extra_have,
 			      struct oid_array *shallow_points)
@@ -241,24 +241,17 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	struct ref **orig_list = list;
 	int len = 0;
 	enum get_remote_heads_state state = EXPECTING_FIRST_REF;
-	struct packet_reader reader;
 	const char *arg;
 
-	packet_reader_init(&reader, in, src_buf, src_len,
-			   PACKET_READ_CHOMP_NEWLINE |
-			   PACKET_READ_GENTLE_ON_EOF);
-
-	discover_version(&reader);
-
 	*list = NULL;
 
 	while (state != EXPECTING_DONE) {
-		switch (packet_reader_read(&reader)) {
+		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(reader.line, "ERR ", &arg))
+			len = reader->pktlen;
+			if (len > 4 && skip_prefix(reader->line, "ERR ", &arg))
 				die("remote error: %s", arg);
 			break;
 		case PACKET_READ_FLUSH:
@@ -270,22 +263,22 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 
 		switch (state) {
 		case EXPECTING_FIRST_REF:
-			process_capabilities(reader.line, &len);
-			if (process_dummy_ref(reader.line)) {
+			process_capabilities(reader->line, &len);
+			if (process_dummy_ref(reader->line)) {
 				state = EXPECTING_SHALLOW;
 				break;
 			}
 			state = EXPECTING_REF;
 			/* fallthrough */
 		case EXPECTING_REF:
-			if (process_ref(reader.line, len, &list, flags, extra_have))
+			if (process_ref(reader->line, len, &list, flags, extra_have))
 				break;
 			state = EXPECTING_SHALLOW;
 			/* fallthrough */
 		case EXPECTING_SHALLOW:
-			if (process_shallow(reader.line, len, shallow_points))
+			if (process_shallow(reader->line, len, shallow_points))
 				break;
-			die("protocol error: unexpected '%s'", reader.line);
+			die("protocol error: unexpected '%s'", reader->line);
 		case EXPECTING_DONE:
 			break;
 		}
connect.h +3 -0
index 01f14cdf3..cdb8979dc 100644
@@ -13,4 +13,7 @@ extern int parse_feature_request(const char *features, const char *feature);
 extern const char *server_feature_value(const char *feature, int *len_ret);
 extern int url_is_local_not_ssh(const char *url);
 
+struct packet_reader;
+extern enum protocol_version discover_version(struct packet_reader *reader);
+
 #endif
remote-curl.c +18 -2
index 0053b0954..9f6d07683 100644
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "config.h"
 #include "remote.h"
+#include "connect.h"
 #include "strbuf.h"
 #include "walker.h"
 #include "http.h"
@@ -13,6 +14,7 @@
 #include "credential.h"
 #include "sha1-array.h"
 #include "send-pack.h"
+#include "protocol.h"
 
 static struct remote *remote;
 /* always ends with a trailing slash */
@@ -176,8 +178,22 @@ static struct discovery *last_discovery;
 static struct ref *parse_git_refs(struct discovery *heads, int for_push)
 {
 	struct ref *list = NULL;
-	get_remote_heads(-1, heads->buf, heads->len, &list,
-			 for_push ? REF_NORMAL : 0, NULL, &heads->shallow);
+	struct packet_reader reader;
+
+	packet_reader_init(&reader, -1, heads->buf, heads->len,
+			   PACKET_READ_CHOMP_NEWLINE |
+			   PACKET_READ_GENTLE_ON_EOF);
+
+	switch (discover_version(&reader)) {
+	case protocol_v1:
+	case protocol_v0:
+		get_remote_heads(&reader, &list, for_push ? REF_NORMAL : 0,
+				 NULL, &heads->shallow);
+		break;
+	case protocol_unknown_version:
+		BUG("unknown protocol version");
+	}
+
 	return list;
 }
 
remote.h +3 -2
index 1f6611be2..2016461df 100644
@@ -150,10 +150,11 @@ int check_ref_type(const struct ref *ref, int flags);
 void free_refs(struct ref *ref);
 
 struct oid_array;
-extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
+struct packet_reader;
+extern struct ref **get_remote_heads(struct packet_reader *reader,
 				     struct ref **list, unsigned int flags,
 				     struct oid_array *extra_have,
-				     struct oid_array *shallow);
+				     struct oid_array *shallow_points);
 
 int resolve_remote_symref(struct ref *ref, struct ref *list);
 int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid);
transport.c +19 -5
index 8e8779096..63c3dbab9 100644
@@ -18,6 +18,7 @@
 #include "sha1-array.h"
 #include "sigchain.h"
 #include "transport-internal.h"
+#include "protocol.h"
 
 static void set_upstreams(struct transport *transport, struct ref *refs,
 	int pretend)
@@ -190,13 +191,26 @@ static int connect_setup(struct transport *transport, int for_push)
 static struct ref *get_refs_via_connect(struct transport *transport, int for_push)
 {
 	struct git_transport_data *data = transport->data;
-	struct ref *refs;
+	struct ref *refs = NULL;
+	struct packet_reader reader;
 
 	connect_setup(transport, for_push);
-	get_remote_heads(data->fd[0], NULL, 0, &refs,
-			 for_push ? REF_NORMAL : 0,
-			 &data->extra_have,
-			 &data->shallow);
+
+	packet_reader_init(&reader, data->fd[0], NULL, 0,
+			   PACKET_READ_CHOMP_NEWLINE |
+			   PACKET_READ_GENTLE_ON_EOF);
+
+	switch (discover_version(&reader)) {
+	case protocol_v1:
+	case protocol_v0:
+		get_remote_heads(&reader, &refs,
+				 for_push ? REF_NORMAL : 0,
+				 &data->extra_have,
+				 &data->shallow);
+		break;
+	case protocol_unknown_version:
+		BUG("unknown protocol version");
+	}
 	data->got_remote_heads = 1;
 
 	return refs;
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
Reviewed-by on v4 8/35
#302 lore raw

Re: [PATCH v4 08/35] connect: discover protocol version outside of get_remote_heads

Quoted 6 lines · On Wed, 28 Feb 2018 15:22:25 -0800 Brandon Williams <[email protected]> wrote:

In order to prepare for the addition of protocol_v2 push the protocol version discovery outside of 'get_remote_heads()'. This will allow for keeping the logic for processing the reference advertisement for protocol_v1 and protocol_v0 separate from the logic for protocol_v2.

I had one issue in version 3 which turns out to not be one, and this patch is unchanged from version 3, so:

Quoted 293 lines
---
 builtin/fetch-pack.c | 16 +++++++++++++++-
 builtin/send-pack.c  | 17 +++++++++++++++--
 connect.c            | 27 ++++++++++-----------------
 connect.h            |  3 +++
 remote-curl.c        | 20 ++++++++++++++++++--
 remote.h             |  5 +++--
 transport.c          | 24 +++++++++++++++++++-----
 7 files changed, 83 insertions(+), 29 deletions(-)
builtin/fetch-pack.c +15 -1
index 366b9d13f..85d4faf76 100644
@@ -4,6 +4,7 @@
 #include "remote.h"
 #include "connect.h"
 #include "sha1-array.h"
+#include "protocol.h"
 
 static const char fetch_pack_usage[] =
 "git fetch-pack [--all] [--stdin] [--quiet | -q] [--keep | -k] [--thin] "
@@ -52,6 +53,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	struct fetch_pack_args args;
 	struct oid_array shallow = OID_ARRAY_INIT;
 	struct string_list deepen_not = STRING_LIST_INIT_DUP;
+	struct packet_reader reader;
 
 	packet_trace_identity("fetch-pack");
 
@@ -193,7 +195,19 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 		if (!conn)
 			return args.diag_url ? 0 : 1;
 	}
-	get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
+
+	packet_reader_init(&reader, fd[0], NULL, 0,
+			   PACKET_READ_CHOMP_NEWLINE |
+			   PACKET_READ_GENTLE_ON_EOF);
+
+	switch (discover_version(&reader)) {
+	case protocol_v1:
+	case protocol_v0:
+		get_remote_heads(&reader, &ref, 0, NULL, &shallow);
+		break;
+	case protocol_unknown_version:
+		BUG("unknown protocol version");
+	}
 
 	ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
 			 &shallow, pack_lockfile_ptr);
builtin/send-pack.c +15 -2
index fc4f0bb5f..83cb125a6 100644
@@ -14,6 +14,7 @@
 #include "sha1-array.h"
 #include "gpg-interface.h"
 #include "gettext.h"
+#include "protocol.h"
 
 static const char * const send_pack_usage[] = {
 	N_("git send-pack [--all | --mirror] [--dry-run] [--force] "
@@ -154,6 +155,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	int progress = -1;
 	int from_stdin = 0;
 	struct push_cas_option cas = {0};
+	struct packet_reader reader;
 
 	struct option options[] = {
 		OPT__VERBOSITY(&verbose),
@@ -256,8 +258,19 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 			args.verbose ? CONNECT_VERBOSE : 0);
 	}
 
-	get_remote_heads(fd[0], NULL, 0, &remote_refs, REF_NORMAL,
-			 &extra_have, &shallow);
+	packet_reader_init(&reader, fd[0], NULL, 0,
+			   PACKET_READ_CHOMP_NEWLINE |
+			   PACKET_READ_GENTLE_ON_EOF);
+
+	switch (discover_version(&reader)) {
+	case protocol_v1:
+	case protocol_v0:
+		get_remote_heads(&reader, &remote_refs, REF_NORMAL,
+				 &extra_have, &shallow);
+		break;
+	case protocol_unknown_version:
+		BUG("unknown protocol version");
+	}
 
 	transport_verify_remote_names(nr_refspecs, refspecs);
 
connect.c +10 -17
index c82c90b7c..0b111e62d 100644
@@ -62,7 +62,7 @@ static void die_initial_contact(int unexpected)
 		      "and the repository exists."));
 }
 
-static enum protocol_version discover_version(struct packet_reader *reader)
+enum protocol_version discover_version(struct packet_reader *reader)
 {
 	enum protocol_version version = protocol_unknown_version;
 
@@ -233,7 +233,7 @@ enum get_remote_heads_state {
 /*
  * Read all the refs from the other end
  */
-struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
+struct ref **get_remote_heads(struct packet_reader *reader,
 			      struct ref **list, unsigned int flags,
 			      struct oid_array *extra_have,
 			      struct oid_array *shallow_points)
@@ -241,24 +241,17 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	struct ref **orig_list = list;
 	int len = 0;
 	enum get_remote_heads_state state = EXPECTING_FIRST_REF;
-	struct packet_reader reader;
 	const char *arg;
 
-	packet_reader_init(&reader, in, src_buf, src_len,
-			   PACKET_READ_CHOMP_NEWLINE |
-			   PACKET_READ_GENTLE_ON_EOF);
-
-	discover_version(&reader);
-
 	*list = NULL;
 
 	while (state != EXPECTING_DONE) {
-		switch (packet_reader_read(&reader)) {
+		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(reader.line, "ERR ", &arg))
+			len = reader->pktlen;
+			if (len > 4 && skip_prefix(reader->line, "ERR ", &arg))
 				die("remote error: %s", arg);
 			break;
 		case PACKET_READ_FLUSH:
@@ -270,22 +263,22 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 
 		switch (state) {
 		case EXPECTING_FIRST_REF:
-			process_capabilities(reader.line, &len);
-			if (process_dummy_ref(reader.line)) {
+			process_capabilities(reader->line, &len);
+			if (process_dummy_ref(reader->line)) {
 				state = EXPECTING_SHALLOW;
 				break;
 			}
 			state = EXPECTING_REF;
 			/* fallthrough */
 		case EXPECTING_REF:
-			if (process_ref(reader.line, len, &list, flags, extra_have))
+			if (process_ref(reader->line, len, &list, flags, extra_have))
 				break;
 			state = EXPECTING_SHALLOW;
 			/* fallthrough */
 		case EXPECTING_SHALLOW:
-			if (process_shallow(reader.line, len, shallow_points))
+			if (process_shallow(reader->line, len, shallow_points))
 				break;
-			die("protocol error: unexpected '%s'", reader.line);
+			die("protocol error: unexpected '%s'", reader->line);
 		case EXPECTING_DONE:
 			break;
 		}
connect.h +3 -0
index 01f14cdf3..cdb8979dc 100644
@@ -13,4 +13,7 @@ extern int parse_feature_request(const char *features, const char *feature);
 extern const char *server_feature_value(const char *feature, int *len_ret);
 extern int url_is_local_not_ssh(const char *url);
 
+struct packet_reader;
+extern enum protocol_version discover_version(struct packet_reader *reader);
+
 #endif
remote-curl.c +18 -2
index 0053b0954..9f6d07683 100644
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "config.h"
 #include "remote.h"
+#include "connect.h"
 #include "strbuf.h"
 #include "walker.h"
 #include "http.h"
@@ -13,6 +14,7 @@
 #include "credential.h"
 #include "sha1-array.h"
 #include "send-pack.h"
+#include "protocol.h"
 
 static struct remote *remote;
 /* always ends with a trailing slash */
@@ -176,8 +178,22 @@ static struct discovery *last_discovery;
 static struct ref *parse_git_refs(struct discovery *heads, int for_push)
 {
 	struct ref *list = NULL;
-	get_remote_heads(-1, heads->buf, heads->len, &list,
-			 for_push ? REF_NORMAL : 0, NULL, &heads->shallow);
+	struct packet_reader reader;
+
+	packet_reader_init(&reader, -1, heads->buf, heads->len,
+			   PACKET_READ_CHOMP_NEWLINE |
+			   PACKET_READ_GENTLE_ON_EOF);
+
+	switch (discover_version(&reader)) {
+	case protocol_v1:
+	case protocol_v0:
+		get_remote_heads(&reader, &list, for_push ? REF_NORMAL : 0,
+				 NULL, &heads->shallow);
+		break;
+	case protocol_unknown_version:
+		BUG("unknown protocol version");
+	}
+
 	return list;
 }
 
remote.h +3 -2
index 1f6611be2..2016461df 100644
@@ -150,10 +150,11 @@ int check_ref_type(const struct ref *ref, int flags);
 void free_refs(struct ref *ref);
 
 struct oid_array;
-extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
+struct packet_reader;
+extern struct ref **get_remote_heads(struct packet_reader *reader,
 				     struct ref **list, unsigned int flags,
 				     struct oid_array *extra_have,
-				     struct oid_array *shallow);
+				     struct oid_array *shallow_points);
 
 int resolve_remote_symref(struct ref *ref, struct ref *list);
 int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid);
transport.c +19 -5
index 8e8779096..63c3dbab9 100644
@@ -18,6 +18,7 @@
 #include "sha1-array.h"
 #include "sigchain.h"
 #include "transport-internal.h"
+#include "protocol.h"
 
 static void set_upstreams(struct transport *transport, struct ref *refs,
 	int pretend)
@@ -190,13 +191,26 @@ static int connect_setup(struct transport *transport, int for_push)
 static struct ref *get_refs_via_connect(struct transport *transport, int for_push)
 {
 	struct git_transport_data *data = transport->data;
-	struct ref *refs;
+	struct ref *refs = NULL;
+	struct packet_reader reader;
 
 	connect_setup(transport, for_push);
-	get_remote_heads(data->fd[0], NULL, 0, &refs,
-			 for_push ? REF_NORMAL : 0,
-			 &data->extra_have,
-			 &data->shallow);
+
+	packet_reader_init(&reader, data->fd[0], NULL, 0,
+			   PACKET_READ_CHOMP_NEWLINE |
+			   PACKET_READ_GENTLE_ON_EOF);
+
+	switch (discover_version(&reader)) {
+	case protocol_v1:
+	case protocol_v0:
+		get_remote_heads(&reader, &refs,
+				 for_push ? REF_NORMAL : 0,
+				 &data->extra_have,
+				 &data->shallow);
+		break;
+	case protocol_unknown_version:
+		BUG("unknown protocol version");
+	}
 	data->got_remote_heads = 1;
 
 	return refs;
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#303 lore raw

PATCH v4 9/35 transport: store protocol version

In reply to Brandon Williams

Once protocol_v2 is introduced requesting a fetch or a push will need to be handled differently depending on the protocol version. Store the protocol version the server is speaking in 'struct git_transport_data' and use it to determine what to do in the case of a fetch or a push.

---
 transport.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)
transport.c +26 -9
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;
 	struct oid_array extra_have;
 	struct oid_array shallow;
 };
@@ -200,7 +201,8 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus
 			   PACKET_READ_CHOMP_NEWLINE |
 			   PACKET_READ_GENTLE_ON_EOF);
 
-	switch (discover_version(&reader)) {
+	data->version = discover_version(&reader);
+	switch (data->version) {
 	case protocol_v1:
 	case protocol_v0:
 		get_remote_heads(&reader, &refs,
@@ -221,7 +223,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 {
 	int ret = 0;
 	struct git_transport_data *data = transport->data;
-	struct ref *refs;
+	struct ref *refs = NULL;
 	char *dest = xstrdup(transport->url);
 	struct fetch_pack_args args;
 	struct ref *refs_tmp = NULL;
@@ -247,10 +249,18 @@ static int fetch_refs_via_pack(struct transport *transport,
 	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,
-			  dest, to_fetch, nr_heads, &data->shallow,
-			  &transport->pack_lockfile);
+	switch (data->version) {
+	case protocol_v1:
+	case protocol_v0:
+		refs = fetch_pack(&args, data->fd, data->conn,
+				  refs_tmp ? refs_tmp : transport->remote_refs,
+				  dest, to_fetch, nr_heads, &data->shallow,
+				  &transport->pack_lockfile);
+		break;
+	case protocol_unknown_version:
+		BUG("unknown protocol version");
+	}
+
 	close(data->fd[0]);
 	close(data->fd[1]);
 	if (finish_connect(data->conn))
@@ -549,7 +559,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 {
 	struct git_transport_data *data = transport->data;
 	struct send_pack_args args;
-	int ret;
+	int ret = 0;
 
 	if (!data->got_remote_heads)
 		get_refs_via_connect(transport, 1);
@@ -574,8 +584,15 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	else
 		args.push_cert = SEND_PACK_PUSH_CERT_NEVER;
 
-	ret = send_pack(&args, data->fd, data->conn, remote_refs,
-			&data->extra_have);
+	switch (data->version) {
+	case protocol_v1:
+	case protocol_v0:
+		ret = send_pack(&args, data->fd, data->conn, remote_refs,
+				&data->extra_have);
+		break;
+	case protocol_unknown_version:
+		BUG("unknown protocol version");
+	}
 
 	close(data->fd[1]);
 	close(data->fd[0]);
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#304 lore raw

PATCH v4 10/35 protocol: introduce enum protocol_version value protocol_v2

In reply to Brandon Williams

Introduce protocol_v2, a new value for 'enum protocol_version'. Subsequent patches will fill in the implementation of protocol_v2.

---
 builtin/fetch-pack.c   | 2 ++
 builtin/receive-pack.c | 6 ++++++
 builtin/send-pack.c    | 3 +++
 builtin/upload-pack.c  | 7 +++++++
 connect.c              | 3 +++
 protocol.c             | 2 ++
 protocol.h             | 1 +
 remote-curl.c          | 3 +++
 transport.c            | 9 +++++++++
 9 files changed, 36 insertions(+)
builtin/fetch-pack.c +2 -0
index 85d4faf76..b2374ddbb 100644
@@ -201,6 +201,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			   PACKET_READ_GENTLE_ON_EOF);
 
 	switch (discover_version(&reader)) {
+	case protocol_v2:
+		die("support for protocol v2 not implemented yet");
 	case protocol_v1:
 	case protocol_v0:
 		get_remote_heads(&reader, &ref, 0, NULL, &shallow);
builtin/receive-pack.c +6 -0
index b7ce7c7f5..3656e94fd 100644
@@ -1963,6 +1963,12 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		unpack_limit = receive_unpack_limit;
 
 	switch (determine_protocol_version_server()) {
+	case protocol_v2:
+		/*
+		 * push support for protocol v2 has not been implemented yet,
+		 * so ignore the request to use v2 and fallback to using v0.
+		 */
+		break;
 	case protocol_v1:
 		/*
 		 * v1 is just the original protocol with a version string,
builtin/send-pack.c +3 -0
index 83cb125a6..b5427f75e 100644
@@ -263,6 +263,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 			   PACKET_READ_GENTLE_ON_EOF);
 
 	switch (discover_version(&reader)) {
+	case protocol_v2:
+		die("support for protocol v2 not implemented yet");
+		break;
 	case protocol_v1:
 	case protocol_v0:
 		get_remote_heads(&reader, &remote_refs, REF_NORMAL,
builtin/upload-pack.c +7 -0
index 2cb5cb35b..8d53e9794 100644
@@ -47,6 +47,13 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 		die("'%s' does not appear to be a git repository", dir);
 
 	switch (determine_protocol_version_server()) {
+	case protocol_v2:
+		/*
+		 * fetch support for protocol v2 has not been implemented yet,
+		 * so ignore the request to use v2 and fallback to using v0.
+		 */
+		upload_pack(&opts);
+		break;
 	case protocol_v1:
 		/*
 		 * v1 is just the original protocol with a version string,
connect.c +3 -0
index 0b111e62d..4b89b984c 100644
@@ -83,6 +83,9 @@ enum protocol_version discover_version(struct packet_reader *reader)
 	}
 
 	switch (version) {
+	case protocol_v2:
+		die("support for protocol v2 not implemented yet");
+		break;
 	case protocol_v1:
 		/* Read the peeked version line */
 		packet_reader_read(reader);
protocol.c +2 -0
index 43012b7eb..5e636785d 100644
@@ -8,6 +8,8 @@ static enum protocol_version parse_protocol_version(const char *value)
 		return protocol_v0;
 	else if (!strcmp(value, "1"))
 		return protocol_v1;
+	else if (!strcmp(value, "2"))
+		return protocol_v2;
 	else
 		return protocol_unknown_version;
 }
protocol.h +1 -0
index 1b2bc94a8..2ad35e433 100644
@@ -5,6 +5,7 @@ enum protocol_version {
 	protocol_unknown_version = -1,
 	protocol_v0 = 0,
 	protocol_v1 = 1,
+	protocol_v2 = 2,
 };
 
 /*
remote-curl.c +3 -0
index 9f6d07683..dae8a4a48 100644
@@ -185,6 +185,9 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)
 			   PACKET_READ_GENTLE_ON_EOF);
 
 	switch (discover_version(&reader)) {
+	case protocol_v2:
+		die("support for protocol v2 not implemented yet");
+		break;
 	case protocol_v1:
 	case protocol_v0:
 		get_remote_heads(&reader, &list, for_push ? REF_NORMAL : 0,
transport.c +9 -0
index 2378dcb38..83d9dd1df 100644
@@ -203,6 +203,9 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus
 
 	data->version = discover_version(&reader);
 	switch (data->version) {
+	case protocol_v2:
+		die("support for protocol v2 not implemented yet");
+		break;
 	case protocol_v1:
 	case protocol_v0:
 		get_remote_heads(&reader, &refs,
@@ -250,6 +253,9 @@ static int fetch_refs_via_pack(struct transport *transport,
 		refs_tmp = get_refs_via_connect(transport, 0);
 
 	switch (data->version) {
+	case protocol_v2:
+		die("support for protocol v2 not implemented yet");
+		break;
 	case protocol_v1:
 	case protocol_v0:
 		refs = fetch_pack(&args, data->fd, data->conn,
@@ -585,6 +591,9 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 		args.push_cert = SEND_PACK_PUSH_CERT_NEVER;
 
 	switch (data->version) {
+	case protocol_v2:
+		die("support for protocol v2 not implemented yet");
+		break;
 	case protocol_v1:
 	case protocol_v0:
 		ret = send_pack(&args, data->fd, data->conn, remote_refs,
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#305 lore raw

PATCH v4 11/35 test-pkt-line: introduce a packet-line test helper

In reply to Brandon Williams

Introduce a packet-line test helper which can either pack or unpack an input stream into packet-lines and writes out the result to stdout.

---
 Makefile                 |  1 +
 t/helper/test-pkt-line.c | 64 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)
 create mode 100644 t/helper/test-pkt-line.c
Makefile +1 -0
index b7ccc05fa..3b849c060 100644
@@ -669,6 +669,7 @@ TEST_PROGRAMS_NEED_X += test-mktemp
 TEST_PROGRAMS_NEED_X += test-online-cpus
 TEST_PROGRAMS_NEED_X += test-parse-options
 TEST_PROGRAMS_NEED_X += test-path-utils
+TEST_PROGRAMS_NEED_X += test-pkt-line
 TEST_PROGRAMS_NEED_X += test-prio-queue
 TEST_PROGRAMS_NEED_X += test-read-cache
 TEST_PROGRAMS_NEED_X += test-write-cache
t/helper/test-pkt-line.c new file +64 -0

64 lines not on this page · raw message

Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#306 lore raw

PATCH v4 12/35 serve: introduce git-serve

In reply to 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.

---
 .gitignore                              |   1 +
 Documentation/technical/protocol-v2.txt | 171 ++++++++++++++++
 Makefile                                |   2 +
 builtin.h                               |   1 +
 builtin/serve.c                         |  30 +++
 git.c                                   |   1 +
 serve.c                                 | 250 ++++++++++++++++++++++++
 serve.h                                 |  15 ++
 t/t5701-git-serve.sh                    |  60 ++++++
 9 files changed, 531 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
 create mode 100755 t/t5701-git-serve.sh
.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 +171 -0

171 lines not on this page · raw message

Makefile +2 -0
index 3b849c060..18c255428 100644
@@ -881,6 +881,7 @@ LIB_OBJS += revision.o
 LIB_OBJS += run-command.o
 LIB_OBJS += send-pack.o
 LIB_OBJS += sequencer.o
+LIB_OBJS += serve.o
 LIB_OBJS += server-info.o
 LIB_OBJS += setup.o
 LIB_OBJS += sha1-array.o
@@ -1014,6 +1015,7 @@ BUILTIN_OBJS += builtin/rev-parse.o
 BUILTIN_OBJS += builtin/revert.o
 BUILTIN_OBJS += builtin/rm.o
 BUILTIN_OBJS += builtin/send-pack.o
+BUILTIN_OBJS += builtin/serve.o
 BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-ref.o
builtin.h +1 -0
index f332a1257..3f3fdfc28 100644
@@ -215,6 +215,7 @@ extern int cmd_rev_parse(int argc, const char **argv, const char *prefix);
 extern int cmd_revert(int argc, const char **argv, const char *prefix);
 extern int cmd_rm(int argc, const char **argv, const char *prefix);
 extern int cmd_send_pack(int argc, const char **argv, const char *prefix);
+extern int cmd_serve(int argc, const char **argv, const char *prefix);
 extern int cmd_shortlog(int argc, const char **argv, const char *prefix);
 extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
builtin/serve.c new file +30 -0
new file mode 100644
index 000000000..d3fd240bb
@@ -0,0 +1,30 @@
+#include "cache.h"
+#include "builtin.h"
+#include "parse-options.h"
+#include "serve.h"
+
+static char const * const serve_usage[] = {
+	N_("git serve [<options>]"),
+	NULL
+};
+
+int cmd_serve(int argc, const char **argv, const char *prefix)
+{
+	struct serve_options opts = SERVE_OPTIONS_INIT;
+
+	struct option options[] = {
+		OPT_BOOL(0, "stateless-rpc", &opts.stateless_rpc,
+			 N_("quit after a single request/response exchange")),
+		OPT_BOOL(0, "advertise-capabilities", &opts.advertise_capabilities,
+			 N_("exit immediately after advertising capabilities")),
+		OPT_END()
+	};
+
+	/* ignore all unknown cmdline switches for now */
+	argc = parse_options(argc, argv, prefix, options, serve_usage,
+			     PARSE_OPT_KEEP_DASHDASH |
+			     PARSE_OPT_KEEP_UNKNOWN);
+	serve(&opts);
+
+	return 0;
+}
git.c +1 -0
index f71073dc8..f85d682b6 100644
@@ -461,6 +461,7 @@ static struct cmd_struct commands[] = {
 	{ "revert", cmd_revert, RUN_SETUP | NEED_WORK_TREE },
 	{ "rm", cmd_rm, RUN_SETUP },
 	{ "send-pack", cmd_send_pack, RUN_SETUP },
+	{ "serve", cmd_serve, RUN_SETUP },
 	{ "shortlog", cmd_shortlog, RUN_SETUP_GENTLY | USE_PAGER },
 	{ "show", cmd_show, RUN_SETUP },
 	{ "show-branch", cmd_show_branch, RUN_SETUP },
serve.c new file +250 -0

250 lines not on this page · raw message

serve.h new file +15 -0
new file mode 100644
index 000000000..fe65ba9f4
@@ -0,0 +1,15 @@
+#ifndef SERVE_H
+#define SERVE_H
+
+struct argv_array;
+extern int has_capability(const struct argv_array *keys, const char *capability,
+			  const char **value);
+
+struct serve_options {
+	unsigned advertise_capabilities;
+	unsigned stateless_rpc;
+};
+#define SERVE_OPTIONS_INIT { 0 }
+extern void serve(struct serve_options *options);
+
+#endif /* SERVE_H */
t/t5701-git-serve.sh new file +60 -0

60 lines not on this page · raw message

Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#307 lore raw

Re: [PATCH v4 12/35] serve: introduce git-serve

Quoted 1 line · Brandon Williams
 Documentation/technical/protocol-v2.txt | 171 ++++++++++++++++

Unlike other things in Documentation/technical/, this is not listed on TECH_DOCS list in Documentation/Makefile. Shouldn't it be?

#308 lore raw

Re: [PATCH v4 12/35] serve: introduce git-serve

Quoted 6 lines · Junio C Hamano
Quoted 1 line · Brandon Williams
 Documentation/technical/protocol-v2.txt | 171 ++++++++++++++++

Unlike other things in Documentation/technical/, this is not listed on TECH_DOCS list in Documentation/Makefile. Shouldn't it be?

Yes it should, I'll fix that.

Signature · 1 line
Brandon Williams
#309 lore raw

Re: [PATCH v4 12/35] serve: introduce git-serve

In reply to Brandon Williams
Quoted 5 lines · Brandon Williams

+ Capabilities
+~~~~~~~~~~~~~~

Documentation/technical/protocol-v2.txt new file +3 -0
+
+There are two different types of capabilities: normal capabilities,
+which can be used to to convey information or alter the behavior of a

Documentation/technical/protocol-v2.txt line 156

Quoted 10 lines
Documentation/technical/protocol-v2.txt new file +10 -0
+request, and commands, which are the core actions that a client wants to
+perform (fetch, push, etc).
+
+All commands must only last a single round and be stateless from the
+perspective of the server side.  All state MUST be retained and managed
+by the client process.  This permits simple round-robin load-balancing
+on the server side, without needing to worry about state management.
+
+Clients MUST NOT require state management on the server side in order to
+function correctly.

This somehow feels a bit too HTTP centric worldview that potentially may penalize those who do not mind stateful services.

Documentation/technical/protocol-v2.txt

Quoted 9 lines

+ agent
+-------

Documentation/technical/protocol-v2.txt new file +7 -0
+
+The server can advertise the `agent` capability with a value `X` (in the
+form `agent=X`) to notify the client that the server is running version
+`X`.  The client may optionally send its own agent string by including
+the `agent` capability with a value `Y` (in the form `agent=Y`) in its
+request to the server (but it MUST NOT do so if the server did not
+advertise the agent capability).

Are there different degrees of permissiveness between "The server CAN" and "The client MAY" above, or is the above paragraph merely being fuzzy?

I notice that, with the above "MUST NOT", it is impossible for a server to collect voluntary census information from client without revealing its own "version". Because in principle it is not sensible to allow one side to send random capabilities without first making sure that the other side understands them, unsolicited "agent" from the client over a channel where the server did not say it would accept one is quite fine, and the server can always say something silly like "agent=undisclosed" to allow the clients to volunteer their own version, but the definition of this capability smells like conflating two unrelated things (i.e. advertising your own version vs permission to announce yourself).

#310 lore raw

Re: [PATCH v4 12/35] serve: introduce git-serve

Quoted 12 lines · Junio C Hamano
Quoted 10 lines · Brandon Williams

+ Capabilities
+~~~~~~~~~~~~~~

Documentation/technical/protocol-v2.txt new file +8 -0
+
+There are two different types of capabilities: normal capabilities,
+which can be used to to convey information or alter the behavior of a
+request, and commands, which are the core actions that a client wants to
+perform (fetch, push, etc).
+
+All commands must only last a single round and be stateless from the
+perspective of the server side.  All state MUST be retained and managed
Quoted 8 lines

Documentation/technical/protocol-v2.txt line 156

Quoted 5 lines
Documentation/technical/protocol-v2.txt new file +5 -0
+by the client process.  This permits simple round-robin load-balancing
+on the server side, without needing to worry about state management.
+
+Clients MUST NOT require state management on the server side in order to
+function correctly.

This somehow feels a bit too HTTP centric worldview that potentially may penalize those who do not mind stateful services.

It's meant to be that way so that we don't run into the same issue we have with the current HTTP transport. Though I've decided to loosen this slightly by making protocol v2 stateless by default unless a capability is advertised and requested by the client indicating that state can be maintained by the server. That leaves the door open for adding state later for transports which have full-duplex connections while still requiring that stateless is designed first. I'm kind of hoping we never need to add state to the protocol because hopefully we can figure out how to improve negotiation as a whole.

Quoted 14 lines

Documentation/technical/protocol-v2.txt

Quoted 10 lines

+ agent
+-------

Documentation/technical/protocol-v2.txt new file +7 -0
+
+The server can advertise the `agent` capability with a value `X` (in the
+form `agent=X`) to notify the client that the server is running version
+`X`.  The client may optionally send its own agent string by including
+the `agent` capability with a value `Y` (in the form `agent=Y`) in its
+request to the server (but it MUST NOT do so if the server did not
+advertise the agent capability).

Are there different degrees of permissiveness between "The server CAN" and "The client MAY" above, or is the above paragraph merely being fuzzy?

I don't think so? I believe I ripped this from the existing description of the agent capability from the current protocol.

Signature · 1 line
Brandon Williams
#311 lore raw

Re: [PATCH v4 12/35] serve: introduce git-serve

In reply to Brandon Williams

serve.c line 31

Quoted 7 lines · Brandon Williams
serve.c new file +7 -0
+	/*
+	 * Function queried to see if a capability should be advertised.
+	 * Optionally a value can be specified by adding it to 'value'.
+	 * If a value is added to 'value', the server will advertise this
+	 * capability as "<name>=<value>" instead of "<name>".
+	 */
+	int (*advertise)(struct repository *r, struct strbuf *value);

So this is "do we tell them about this capability?"

serve.c

Quoted 9 lines
serve.c new file +9 -0
+static void advertise_capabilities(void)
+{
+	...
+	for (i = 0; i < ARRAY_SIZE(capabilities); i++) {
+		struct protocol_capability *c = &capabilities[i];
+
+		if (c->advertise(the_repository, &value)) {
+			strbuf_addstr(&capability, c->name);
+             ...

And used as such in this function. We tell the other side about the capability only when .advertise returns true.

serve.c line 101

Quoted 6 lines
serve.c new file +6 -0
+static int is_valid_capability(const char *key)
+{
+	const struct protocol_capability *c = get_capability(key);
+
+	return c && c->advertise(the_repository, NULL);
+}

But this is different---the other side mentioned a capability's name, and we looked it up from our table to see if we know about it (i.e. NULL-ness of c), but in addition, we ask if we would tell them about it if we were advertising. I am not sure how I should feel about it (yet).

serve.c line 113

Quoted 11 lines
serve.c new file +11 -0
+static int is_command(const char *key, struct protocol_capability **command)
+{
+	const char *out;
+
+	if (skip_prefix(key, "command=", &out)) {
+		struct protocol_capability *cmd = get_capability(out);
+
+		if (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command)
+			die("invalid command '%s'", out);
+		if (*command)
+			die("command already requested");

Shouldn't these two checks that lead to die the other way around? When they give us "command=frotz" and we already have *command, it would be an error whether we understand 'frotz' or not.

Who are the target audience of these "die"? Are they meant to be communicated back to the other side of the connection, or are they only to be sent to the "server log"?

The latter one may want to say what two conflicting commands are in the log message, perhaps?

Quoted 1 line
serve.c new file +1 -0
+		*command = cmd;
#312 lore raw

Re: [PATCH v4 12/35] serve: introduce git-serve

Quoted 17 lines · Junio C Hamano

serve.c line 113

Quoted 11 lines · Brandon Williams
serve.c new file +11 -0
+static int is_command(const char *key, struct protocol_capability **command)
+{
+	const char *out;
+
+	if (skip_prefix(key, "command=", &out)) {
+		struct protocol_capability *cmd = get_capability(out);
+
+		if (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command)
+			die("invalid command '%s'", out);
+		if (*command)
+			die("command already requested");

Shouldn't these two checks that lead to die the other way around? When they give us "command=frotz" and we already have *command, it would be an error whether we understand 'frotz' or not.

Quoted 6 lines

Who are the target audience of these "die"? Are they meant to be communicated back to the other side of the connection, or are they only to be sent to the "server log"?

The latter one may want to say what two conflicting commands are in the log message, perhaps?

Yeah I'll switch the order of these checks as well as print out the two commands requested for better logging.

Quoted 3 lines
Quoted 2 lines
serve.c new file +1 -0
+		*command = cmd;
Signature · 1 line
Brandon Williams
#313 lore raw

PATCH v4 13/35 ls-refs: introduce ls-refs server command

In reply to Brandon Williams

Introduce the ls-refs server command. In protocol v2, the ls-refs command is used to request the ref advertisement from the server. Since it is a command which can be requested (as opposed to mandatory in v1), a client can sent a number of parameters in its request to limit the ref advertisement based on provided ref-patterns.

---
 Documentation/technical/protocol-v2.txt |  36 ++++++
 Makefile                                |   1 +
 ls-refs.c                               | 144 ++++++++++++++++++++++++
 ls-refs.h                               |   9 ++
 serve.c                                 |   8 ++
 t/t5701-git-serve.sh                    | 115 +++++++++++++++++++
 6 files changed, 313 insertions(+)
 create mode 100644 ls-refs.c
 create mode 100644 ls-refs.h
Documentation/technical/protocol-v2.txt +36 -0
index b02eefc21..7f50e6462 100644
@@ -169,3 +169,39 @@ printable ASCII characters except space (i.e., the byte range 32 < x <
 "git/1.8.3.1"). The agent strings are purely informative for statistics
 and debugging purposes, and MUST NOT be used to programmatically assume
 the presence or absence of particular features.
+
+ ls-refs
+---------
+
+`ls-refs` is the command used to request a reference advertisement in v2.
+Unlike the current reference advertisement, ls-refs takes in arguments
+which can be used to limit the refs sent from the server.
+
+Additional features not supported in the base command will be advertised
+as the value of the command in the capability advertisement in the form
+of a space separated list of features, e.g.  "<command>=<feature 1>
+<feature 2>".
+
+ls-refs takes in the following arguments:
+
+    symrefs
+	In addition to the object pointed by it, show the underlying ref
+	pointed by it when showing a symbolic ref.
+    peel
+	Show peeled tags.
+    ref-pattern <pattern>
+	When specified, only references matching one of the provided
+	patterns are displayed.  A pattern is either a valid refname
+	(e.g.  refs/heads/master), in which a ref must match the pattern
+	exactly, or a prefix of a ref followed by a single '*' wildcard
+	character (e.g. refs/heads/*), in which a ref must have a prefix
+	equal to the pattern up to the wildcard character.
+
+The output of ls-refs is as follows:
+
+    output = *ref
+	     flush-pkt
+    ref = PKT-LINE(obj-id SP refname *(SP ref-attribute) LF)
+    ref-attribute = (symref | peeled)
+    symref = "symref-target:" symref-target
+    peeled = "peeled:" obj-id
Makefile +1 -0
index 18c255428..e50927cfb 100644
@@ -825,6 +825,7 @@ LIB_OBJS += list-objects-filter-options.o
 LIB_OBJS += ll-merge.o
 LIB_OBJS += lockfile.o
 LIB_OBJS += log-tree.o
+LIB_OBJS += ls-refs.o
 LIB_OBJS += mailinfo.o
 LIB_OBJS += mailmap.o
 LIB_OBJS += match-trees.o
ls-refs.c new file +144 -0

144 lines not on this page · raw message

ls-refs.h new file +9 -0
new file mode 100644
index 000000000..9e4c57bfe
@@ -0,0 +1,9 @@
+#ifndef LS_REFS_H
+#define LS_REFS_H
+
+struct repository;
+struct argv_array;
+extern int ls_refs(struct repository *r, struct argv_array *keys,
+		   struct argv_array *args);
+
+#endif /* LS_REFS_H */
serve.c +8 -0
index cf23179b9..c7925c5c7 100644
@@ -4,8 +4,15 @@
 #include "pkt-line.h"
 #include "version.h"
 #include "argv-array.h"
+#include "ls-refs.h"
 #include "serve.h"
 
+static int always_advertise(struct repository *r,
+			    struct strbuf *value)
+{
+	return 1;
+}
+
 static int agent_advertise(struct repository *r,
 			   struct strbuf *value)
 {
@@ -44,6 +51,7 @@ struct protocol_capability {
 
 static struct protocol_capability capabilities[] = {
 	{ "agent", agent_advertise, NULL },
+	{ "ls-refs", always_advertise, ls_refs },
 };
 
 static void advertise_capabilities(void)
t/t5701-git-serve.sh +115 -0

125 lines not on this page · raw message

Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#314 lore raw

Re: [PATCH v4 13/35] ls-refs: introduce ls-refs server command

Documentation/technical/protocol-v2.txt printable ASCII characters except space (i.e., the byte range 32 < x < · line 178

Quoted 6 lines · Brandon Williams

+ ls-refs
+---------

Documentation/technical/protocol-v2.txt +4 -0
+
+`ls-refs` is the command used to request a reference advertisement in v2.
+Unlike the current reference advertisement, ls-refs takes in arguments
+which can be used to limit the refs sent from the server.

OK.

Documentation/technical/protocol-v2.txt printable ASCII characters except space (i.e., the byte range 32 < x < · line 183

Quoted 4 lines
Documentation/technical/protocol-v2.txt +4 -0
+Additional features not supported in the base command will be advertised
+as the value of the command in the capability advertisement in the form
+of a space separated list of features, e.g.  "<command>=<feature 1>
+<feature 2>".

Doesn't this explain the general convention that applies to any command, not just ls-refs command? As a part of ls-refs section, <command> in the above explanation is always a constant "ls-refs", right?

It is a bit unclear how <feature N> in the above description are related to "arguments" in the following paragraph. Do the server that can show symref and peeled tags and that can limit the output with ref-pattern advertise these three as supported features, i.e.

ls-refs=symrefs peel ref-pattern

or something? Would there a case where a "feature" does not correspond 1:1 to an argument to the command, and if so how would the server and the client negotiate use of such a feature?

Documentation/technical/protocol-v2.txt printable ASCII characters except space (i.e., the byte range 32 < x < · line 198

Quoted 7 lines
Documentation/technical/protocol-v2.txt +7 -0
+    ref-pattern <pattern>
+	When specified, only references matching one of the provided
+	patterns are displayed.  A pattern is either a valid refname
+	(e.g.  refs/heads/master), in which a ref must match the pattern
+	exactly, or a prefix of a ref followed by a single '*' wildcard
+	character (e.g. refs/heads/*), in which a ref must have a prefix
+	equal to the pattern up to the wildcard character.

I thought the recent concensus was left-anchored prefix match that honors /-directory boundary, i.e. no explicit asterisk and just saying "refs/heads" is enough to match "refs/heads" itself and "refs/heads/master" but not "refs/headscarf"?

#315 lore raw

Re: [PATCH v4 13/35] ls-refs: introduce ls-refs server command

Quoted 26 lines · Junio C Hamano

Documentation/technical/protocol-v2.txt printable ASCII characters except space (i.e., the byte range 32 < x < · line 178

Quoted 6 lines · Brandon Williams

+ ls-refs
+---------

Documentation/technical/protocol-v2.txt +4 -0
+
+`ls-refs` is the command used to request a reference advertisement in v2.
+Unlike the current reference advertisement, ls-refs takes in arguments
+which can be used to limit the refs sent from the server.

OK.

Documentation/technical/protocol-v2.txt printable ASCII characters except space (i.e., the byte range 32 < x < · line 183

Quoted 4 lines
Documentation/technical/protocol-v2.txt +4 -0
+Additional features not supported in the base command will be advertised
+as the value of the command in the capability advertisement in the form
+of a space separated list of features, e.g.  "<command>=<feature 1>
+<feature 2>".

Doesn't this explain the general convention that applies to any command, not just ls-refs command? As a part of ls-refs section, <command> in the above explanation is always a constant "ls-refs", right?

It is a bit unclear how <feature N> in the above description are related to "arguments" in the following paragraph. Do the server that can show symref and peeled tags and that can limit the output with ref-pattern advertise these three as supported features, i.e.

Quoted 5 lines

ls-refs=symrefs peel ref-pattern

or something? Would there a case where a "feature" does not correspond 1:1 to an argument to the command, and if so how would the server and the client negotiate use of such a feature?

I mention earlier in the document that the values of each capability are to be defined by the capability itself, so I'm just documenting what the value advertised means. And a feature could mean a couple things and doesn't necessarily mean it affects the arguments which can be provided, and it definitely doesn't mean that each argument that can be provided must be advertised as a feature. If you look at the patch that introduces shallow, the shallow feature adds lots of arguments that a client can that use in its request.

Quoted 13 lines

Documentation/technical/protocol-v2.txt printable ASCII characters except space (i.e., the byte range 32 < x < · line 198

Quoted 8 lines
Documentation/technical/protocol-v2.txt +7 -0
+    ref-pattern <pattern>
+	When specified, only references matching one of the provided
+	patterns are displayed.  A pattern is either a valid refname
+	(e.g.  refs/heads/master), in which a ref must match the pattern
+	exactly, or a prefix of a ref followed by a single '*' wildcard
+	character (e.g. refs/heads/*), in which a ref must have a prefix
+	equal to the pattern up to the wildcard character.

I thought the recent concensus was left-anchored prefix match that honors /-directory boundary, i.e. no explicit asterisk and just saying "refs/heads" is enough to match "refs/heads" itself and "refs/heads/master" but not "refs/headscarf"?

I don't think there was a consensus at the time, but in the next revision I'll have them be prefixes instead of containing wildcards.

Signature · 1 line
Brandon Williams
#316 lore raw

Re: [PATCH v4 13/35] ls-refs: introduce ls-refs server command

In reply to Brandon Williams

ls-refs.c line 25

Quoted 6 lines · Brandon Williams
ls-refs.c new file +6 -0
+static void add_pattern(struct pattern_list *patterns, const char *pattern)
+{
+	struct ref_pattern p;
+	const char *wildcard;
+
+	p.pattern = strdup(pattern);

xstrdup?

ls-refs.c line 32

Quoted 6 lines
ls-refs.c new file +6 -0
+	wildcard = strchr(pattern, '*');
+	if (wildcard) {
+		p.wildcard_pos = wildcard - pattern;
+	} else {
+		p.wildcard_pos = -1;
+	}

Hmm, so this would accept stuff like "refs/heads/*/foo" but quietly ignore the "/foo" part.

It also accepts "refs/h*" to get "refs/heads" and "refs/hello". I think it's worth going for the most-restrictive thing to start with, since that enables a lot more server operations without worrying about breaking compatibility.

-Peff

#317 lore raw

Re: [PATCH v4 13/35] ls-refs: introduce ls-refs server command

Quoted 11 lines · Jeff King

ls-refs.c line 25

Quoted 6 lines · Brandon Williams
ls-refs.c new file +6 -0
+static void add_pattern(struct pattern_list *patterns, const char *pattern)
+{
+	struct ref_pattern p;
+	const char *wildcard;
+
+	p.pattern = strdup(pattern);

xstrdup?

Quoted 9 lines

ls-refs.c line 32

Quoted 6 lines
ls-refs.c new file +6 -0
+	wildcard = strchr(pattern, '*');
+	if (wildcard) {
+		p.wildcard_pos = wildcard - pattern;
+	} else {
+		p.wildcard_pos = -1;
+	}

Hmm, so this would accept stuff like "refs/heads/*/foo" but quietly ignore the "/foo" part.

Yeah that's true...this should probably not do that. Since "refs/heads/*/foo" violates what the spec is, really this should error out as an invalid pattern.

Quoted 5 lines

It also accepts "refs/h*" to get "refs/heads" and "refs/hello". I think it's worth going for the most-restrictive thing to start with, since that enables a lot more server operations without worrying about breaking compatibility.

And just to clarify what do you see as being the most-restrictive case of patterns that would should use?

Signature · 1 line
Brandon Williams
#318 lore raw

Re: [PATCH v4 13/35] ls-refs: introduce ls-refs server command

Hi,

Quoted 9 lines · Brandon Williams
Quoted 4 lines · Jeff King

It also accepts "refs/h*" to get "refs/heads" and "refs/hello". I think it's worth going for the most-restrictive thing to start with, since that enables a lot more server operations without worrying about breaking compatibility.

And just to clarify what do you see as being the most-restrictive case of patterns that would should use?

Peff, can you say a little more about the downsides of accepting refs/h*?

IIRC the "git push" command already accepts such refspecs, so there's a benefit to accepting them. Reftable and packed-refs support such queries about as efficiently as refs/heads/*. For loose refs, readdir doesn't provide a way to restrict which files you look at, but loose refs are always slow anyway. :)

In other words, I see real benefits and I don't see much in the way of costs, so I'm not seeing why not to support this.

Thanks,
Jonathan

#319 lore raw

Re: [PATCH v4 13/35] ls-refs: introduce ls-refs server command

Quoted 11 lines · Jonathan Nieder
Quoted 7 lines
Quoted 4 lines

It also accepts "refs/h*" to get "refs/heads" and "refs/hello". I think it's worth going for the most-restrictive thing to start with, since that enables a lot more server operations without worrying about breaking compatibility.

And just to clarify what do you see as being the most-restrictive case of patterns that would should use?

Peff, can you say a little more about the downsides of accepting refs/h*?

Quoted 8 lines

IIRC the "git push" command already accepts such refspecs, so there's a benefit to accepting them. Reftable and packed-refs support such queries about as efficiently as refs/heads/*. For loose refs, readdir doesn't provide a way to restrict which files you look at, but loose refs are always slow anyway. :)

In other words, I see real benefits and I don't see much in the way of costs, so I'm not seeing why not to support this.

"git for-each-ref" only handles "/" boundaries. I think we used to have similar problems with the internal for_each_ref(), but I just checked and I think it's more flexible these days. One could imagine a more trie-like storage, though I agree that is stretching it with a hypothetical.

Mostly my point was that I don't see any big upside, and the choice seemed rather arbitrary. And as it is generally easier to loosen the patterns later than tighten them, it makes sense to go with the tightest option at first unless there is a compelling reason not to.

-Peff

#320 lore raw

Re: [PATCH v4 13/35] ls-refs: introduce ls-refs server command

In reply to Brandon Williams
Quoted 6 lines · Brandon Williams
Quoted 2 lines

Hmm, so this would accept stuff like "refs/heads/*/foo" but quietly ignore the "/foo" part.

Yeah that's true...this should probably not do that. Since "refs/heads/*/foo" violates what the spec is, really this should error out as an invalid pattern.

Yeah, that would be better, I think.

Quoted 7 lines
Quoted 4 lines

It also accepts "refs/h*" to get "refs/heads" and "refs/hello". I think it's worth going for the most-restrictive thing to start with, since that enables a lot more server operations without worrying about breaking compatibility.

And just to clarify what do you see as being the most-restrictive case of patterns that would should use?

I mean only accepting "*" at a "/" boundary (or just allowing a trailing slash to imply recursion, like "refs/heads/", or even just always assuming recursion to allow "refs/heads").

-Peff

#321 lore raw

Re: [PATCH v4 13/35] ls-refs: introduce ls-refs server command

Quoted 16 lines · Jeff King
Quoted 6 lines · Brandon Williams
Quoted 2 lines

Hmm, so this would accept stuff like "refs/heads/*/foo" but quietly ignore the "/foo" part.

Yeah that's true...this should probably not do that. Since "refs/heads/*/foo" violates what the spec is, really this should error out as an invalid pattern.

Yeah, that would be better, I think.

Quoted 5 lines
Quoted 4 lines

It also accepts "refs/h*" to get "refs/heads" and "refs/hello". I think it's worth going for the most-restrictive thing to start with, since that enables a lot more server operations without worrying about breaking compatibility.

Quoted 6 lines
Quoted 2 lines

And just to clarify what do you see as being the most-restrictive case of patterns that would should use?

I mean only accepting "*" at a "/" boundary (or just allowing a trailing slash to imply recursion, like "refs/heads/", or even just always assuming recursion to allow "refs/heads").

For simplicity I'll change ref-patterns to be ref-prefixes where a ref must start_with() one of the provided ref-prefixes. Clients won't send '*'s either but can send everything upto but not including the '*' as a prefix.

Signature · 1 line
Brandon Williams
#322 lore raw

PATCH v4 14/35 connect: request remote refs using v2

In reply to Brandon Williams

Teach the client to be able to request a remote's refs using protocol v2. This is done by having a client issue a 'ls-refs' request to a v2 server.

---
 builtin/upload-pack.c  |  10 +--
 connect.c              | 138 +++++++++++++++++++++++++++++++++++++++--
 connect.h              |   2 +
 remote.h               |   6 ++
 t/t5702-protocol-v2.sh |  57 +++++++++++++++++
 transport.c            |   2 +-
 6 files changed, 204 insertions(+), 11 deletions(-)
 create mode 100755 t/t5702-protocol-v2.sh
builtin/upload-pack.c +5 -5
index 8d53e9794..a757df8da 100644
@@ -5,6 +5,7 @@
 #include "parse-options.h"
 #include "protocol.h"
 #include "upload-pack.h"
+#include "serve.h"
 
 static const char * const upload_pack_usage[] = {
 	N_("git upload-pack [<options>] <dir>"),
@@ -16,6 +17,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 	const char *dir;
 	int strict = 0;
 	struct upload_pack_options opts = { 0 };
+	struct serve_options serve_opts = SERVE_OPTIONS_INIT;
 	struct option options[] = {
 		OPT_BOOL(0, "stateless-rpc", &opts.stateless_rpc,
 			 N_("quit after a single request/response exchange")),
@@ -48,11 +50,9 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
 
 	switch (determine_protocol_version_server()) {
 	case protocol_v2:
-		/*
-		 * fetch support for protocol v2 has not been implemented yet,
-		 * so ignore the request to use v2 and fallback to using v0.
-		 */
-		upload_pack(&opts);
+		serve_opts.advertise_capabilities = opts.advertise_refs;
+		serve_opts.stateless_rpc = opts.stateless_rpc;
+		serve(&serve_opts);
 		break;
 	case protocol_v1:
 		/*
connect.c +133 -5

182 lines not on this page · raw message

connect.h +2 -0
index cdb8979dc..8898d4495 100644
@@ -16,4 +16,6 @@ extern int url_is_local_not_ssh(const char *url);
 struct packet_reader;
 extern enum protocol_version discover_version(struct packet_reader *reader);
 
+extern int server_supports_v2(const char *c, int die_on_error);
+
 #endif
remote.h +6 -0
index 2016461df..3a9db30cf 100644
@@ -151,11 +151,17 @@ void free_refs(struct ref *ref);
 
 struct oid_array;
 struct packet_reader;
+struct argv_array;
 extern struct ref **get_remote_heads(struct packet_reader *reader,
 				     struct ref **list, unsigned int flags,
 				     struct oid_array *extra_have,
 				     struct oid_array *shallow_points);
 
+/* Used for protocol v2 in order to retrieve refs from a remote */
+extern struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
+				    struct ref **list, int for_push,
+				    const struct argv_array *ref_patterns);
+
 int resolve_remote_symref(struct ref *ref, struct ref *list);
 int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid);
 
t/t5702-protocol-v2.sh new file +57 -0

57 lines not on this page · raw message

transport.c +1 -1
index 83d9dd1df..ffc6b2614 100644
@@ -204,7 +204,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus
 	data->version = discover_version(&reader);
 	switch (data->version) {
 	case protocol_v2:
-		die("support for protocol v2 not implemented yet");
+		get_remote_refs(data->fd[1], &reader, &refs, for_push, NULL);
 		break;
 	case protocol_v1:
 	case protocol_v0:
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#323 lore raw

PATCH v4 15/35 transport: convert get_refs_list to take a list of ref patterns

In reply to Brandon Williams

Convert the 'struct transport' virtual function 'get_refs_list()' to optionally take an argv_array of ref patterns. When communicating with a server using protocol v2 these ref patterns can be sent when requesting a listing of their refs allowing the server to filter the refs it sends based on the sent patterns.

---
 transport-helper.c   |  5 +++--
 transport-internal.h |  9 ++++++++-
 transport.c          | 16 +++++++++-------
 3 files changed, 20 insertions(+), 10 deletions(-)
transport-helper.c +3 -2
index 508015023..4c334b5ee 100644
@@ -1026,7 +1026,8 @@ static int has_attribute(const char *attrs, const char *attr) {
 	}
 }
 
-static struct ref *get_refs_list(struct transport *transport, int for_push)
+static struct ref *get_refs_list(struct transport *transport, int for_push,
+				 const struct argv_array *ref_patterns)
 {
 	struct helper_data *data = transport->data;
 	struct child_process *helper;
@@ -1039,7 +1040,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 
 	if (process_connect(transport, for_push)) {
 		do_take_over(transport);
-		return transport->vtable->get_refs_list(transport, for_push);
+		return transport->vtable->get_refs_list(transport, for_push, ref_patterns);
 	}
 
 	if (data->push && for_push)
transport-internal.h +8 -1
index 3c1a29d72..36fcee437 100644
@@ -3,6 +3,7 @@
 
 struct ref;
 struct transport;
+struct argv_array;
 
 struct transport_vtable {
 	/**
@@ -17,11 +18,17 @@ struct transport_vtable {
 	 * the transport to try to share connections, for_push is a
 	 * hint as to whether the ultimate operation is a push or a fetch.
 	 *
+	 * If communicating using protocol v2 a list of patterns can be
+	 * provided to be sent to the server to enable it to limit the ref
+	 * advertisement.  Since ref filtering is done on the server's end,
+	 * this can return refs which don't match the provided ref_patterns.
+	 *
 	 * If the transport is able to determine the remote hash for
 	 * the ref without a huge amount of effort, it should store it
 	 * in the ref's old_sha1 field; otherwise it should be all 0.
 	 **/
-	struct ref *(*get_refs_list)(struct transport *transport, int for_push);
+	struct ref *(*get_refs_list)(struct transport *transport, int for_push,
+				     const struct argv_array *ref_patterns);
 
 	/**
 	 * Fetch the objects for the given refs. Note that this gets
transport.c +9 -7
index ffc6b2614..c54a44630 100644
@@ -72,7 +72,7 @@ struct bundle_transport_data {
 	struct bundle_header header;
 };
 
-static struct ref *get_refs_from_bundle(struct transport *transport, int for_push)
+static struct ref *get_refs_from_bundle(struct transport *transport, int for_push, const struct argv_array *ref_patterns)
 {
 	struct bundle_transport_data *data = transport->data;
 	struct ref *result = NULL;
@@ -189,7 +189,8 @@ static int connect_setup(struct transport *transport, int for_push)
 	return 0;
 }
 
-static struct ref *get_refs_via_connect(struct transport *transport, int for_push)
+static struct ref *get_refs_via_connect(struct transport *transport, int for_push,
+					const struct argv_array *ref_patterns)
 {
 	struct git_transport_data *data = transport->data;
 	struct ref *refs = NULL;
@@ -204,7 +205,8 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus
 	data->version = discover_version(&reader);
 	switch (data->version) {
 	case protocol_v2:
-		get_remote_refs(data->fd[1], &reader, &refs, for_push, NULL);
+		get_remote_refs(data->fd[1], &reader, &refs, for_push,
+				ref_patterns);
 		break;
 	case protocol_v1:
 	case protocol_v0:
@@ -250,7 +252,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.update_shallow = data->options.update_shallow;
 
 	if (!data->got_remote_heads)
-		refs_tmp = get_refs_via_connect(transport, 0);
+		refs_tmp = get_refs_via_connect(transport, 0, NULL);
 
 	switch (data->version) {
 	case protocol_v2:
@@ -568,7 +570,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	int ret = 0;
 
 	if (!data->got_remote_heads)
-		get_refs_via_connect(transport, 1);
+		get_refs_via_connect(transport, 1, NULL);
 
 	memset(&args, 0, sizeof(args));
 	args.send_mirror = !!(flags & TRANSPORT_PUSH_MIRROR);
@@ -1028,7 +1030,7 @@ int transport_push(struct transport *transport,
 		if (check_push_refs(local_refs, refspec_nr, refspec) < 0)
 			return -1;
 
-		remote_refs = transport->vtable->get_refs_list(transport, 1);
+		remote_refs = transport->vtable->get_refs_list(transport, 1, NULL);
 
 		if (flags & TRANSPORT_PUSH_ALL)
 			match_flags |= MATCH_REFS_ALL;
@@ -1137,7 +1139,7 @@ int transport_push(struct transport *transport,
 const struct ref *transport_get_remote_refs(struct transport *transport)
 {
 	if (!transport->got_remote_refs) {
-		transport->remote_refs = transport->vtable->get_refs_list(transport, 0);
+		transport->remote_refs = transport->vtable->get_refs_list(transport, 0, NULL);
 		transport->got_remote_refs = 1;
 	}
 
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#324 lore raw

PATCH v4 16/35 transport: convert transport_get_remote_refs to take a list of ref patterns

In reply to Brandon Williams

Convert 'transport_get_remote_refs()' to optionally take a list of ref patterns.

---
 builtin/clone.c     |  2 +-
 builtin/fetch.c     |  4 ++--
 builtin/ls-remote.c |  2 +-
 builtin/remote.c    |  2 +-
 transport.c         |  7 +++++--
 transport.h         | 12 +++++++++++-
 6 files changed, 21 insertions(+), 8 deletions(-)
builtin/clone.c +1 -1
index 284651797..6e77d993f 100644
@@ -1121,7 +1121,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (transport->smart_options && !deepen)
 		transport->smart_options->check_self_contained_and_connected = 1;
 
-	refs = transport_get_remote_refs(transport);
+	refs = transport_get_remote_refs(transport, NULL);
 
 	if (refs) {
 		mapped_refs = wanted_peer_refs(refs, refspec);
builtin/fetch.c +2 -2
index 7bbcd26fa..850382f55 100644
@@ -250,7 +250,7 @@ static void find_non_local_tags(struct transport *transport,
 	struct string_list_item *item = NULL;
 
 	for_each_ref(add_existing, &existing_refs);
-	for (ref = transport_get_remote_refs(transport); ref; ref = ref->next) {
+	for (ref = transport_get_remote_refs(transport, NULL); ref; ref = ref->next) {
 		if (!starts_with(ref->name, "refs/tags/"))
 			continue;
 
@@ -336,7 +336,7 @@ static struct ref *get_ref_map(struct transport *transport,
 	/* opportunistically-updated references: */
 	struct ref *orefs = NULL, **oref_tail = &orefs;
 
-	const struct ref *remote_refs = transport_get_remote_refs(transport);
+	const struct ref *remote_refs = transport_get_remote_refs(transport, NULL);
 
 	if (refspec_count) {
 		struct refspec *fetch_refspec;
builtin/ls-remote.c +1 -1
index c4be98ab9..c6e9847c5 100644
@@ -96,7 +96,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	if (uploadpack != NULL)
 		transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);
 
-	ref = transport_get_remote_refs(transport);
+	ref = transport_get_remote_refs(transport, NULL);
 	if (transport_disconnect(transport))
 		return 1;
 
builtin/remote.c +1 -1
index d95bf904c..d0b6ff6e2 100644
@@ -862,7 +862,7 @@ static int get_remote_ref_states(const char *name,
 	if (query) {
 		transport = transport_get(states->remote, states->remote->url_nr > 0 ?
 			states->remote->url[0] : NULL);
-		remote_refs = transport_get_remote_refs(transport);
+		remote_refs = transport_get_remote_refs(transport, NULL);
 		transport_disconnect(transport);
 
 		states->queried = 1;
transport.c +5 -2
index c54a44630..dfc603b36 100644
@@ -1136,10 +1136,13 @@ int transport_push(struct transport *transport,
 	return 1;
 }
 
-const struct ref *transport_get_remote_refs(struct transport *transport)
+const struct ref *transport_get_remote_refs(struct transport *transport,
+					    const struct argv_array *ref_patterns)
 {
 	if (!transport->got_remote_refs) {
-		transport->remote_refs = transport->vtable->get_refs_list(transport, 0, NULL);
+		transport->remote_refs =
+			transport->vtable->get_refs_list(transport, 0,
+							 ref_patterns);
 		transport->got_remote_refs = 1;
 	}
 
transport.h +11 -1
index 731c78b67..daea4770c 100644
@@ -178,7 +178,17 @@ int transport_push(struct transport *connection,
 		   int refspec_nr, const char **refspec, int flags,
 		   unsigned int * reject_reasons);
 
-const struct ref *transport_get_remote_refs(struct transport *transport);
+/*
+ * Retrieve refs from a remote.
+ *
+ * Optionally a list of ref patterns can be provided which can be sent to the
+ * server (when communicating using protocol v2) to enable it to limit the ref
+ * advertisement.  Since ref filtering is done on the server's end (and only
+ * when using protocol v2), this can return refs which don't match the provided
+ * ref_patterns.
+ */
+const struct ref *transport_get_remote_refs(struct transport *transport,
+					    const struct argv_array *ref_patterns);
 
 int transport_fetch_refs(struct transport *transport, struct ref *refs);
 void transport_unlock_pack(struct transport *transport);
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#325 lore raw

Re: [PATCH v4 16/35] transport: convert transport_get_remote_refs to take a list of ref patterns

Quoted 4 lines · On Wed, 28 Feb 2018 15:22:33 -0800 Brandon Williams <[email protected]> wrote:

Convert 'transport_get_remote_refs()' to optionally take a list of ref patterns.

[snip]

transport.h int transport_push(struct transport *connection, · line 191

Quoted 12 lines
transport.h +11 -1
-const struct ref *transport_get_remote_refs(struct transport *transport);
+/*
+ * Retrieve refs from a remote.
+ *
+ * Optionally a list of ref patterns can be provided which can be sent to the
+ * server (when communicating using protocol v2) to enable it to limit the ref
+ * advertisement.  Since ref filtering is done on the server's end (and only
+ * when using protocol v2), this can return refs which don't match the provided
+ * ref_patterns.
+ */
+const struct ref *transport_get_remote_refs(struct transport *transport,
+					    const struct argv_array *ref_patterns);

Thanks for adding the documentation, but I think this should also go into the commit message. For example:

Teach transport_get_remote_refs() to accept a list of ref patterns, which will be sent to the server for use in filtering when using protocol v2. (This list will be ignored when not using protocol v2.)

#326 lore raw

PATCH v4 17/35 ls-remote: pass ref patterns when requesting a remote's refs

In reply to Brandon Williams

Construct an argv_array of the ref patterns supplied via the command line and pass them to 'transport_get_remote_refs()' to be used when communicating protocol v2 so that the server can limit the ref advertisement based on the supplied patterns.

---
 builtin/ls-remote.c    | 12 ++++++++++--
 refs.c                 | 14 ++++++++++++++
 refs.h                 |  7 +++++++
 t/t5702-protocol-v2.sh | 26 ++++++++++++++++++++++++++
 4 files changed, 57 insertions(+), 2 deletions(-)
builtin/ls-remote.c +10 -2
index c6e9847c5..083ba8b29 100644
@@ -2,6 +2,7 @@
 #include "cache.h"
 #include "transport.h"
 #include "remote.h"
+#include "refs.h"
 
 static const char * const ls_remote_usage[] = {
 	N_("git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>]\n"
@@ -43,6 +44,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	int show_symref_target = 0;
 	const char *uploadpack = NULL;
 	const char **pattern = NULL;
+	struct argv_array ref_patterns = ARGV_ARRAY_INIT;
 
 	struct remote *remote;
 	struct transport *transport;
@@ -74,8 +76,14 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	if (argc > 1) {
 		int i;
 		pattern = xcalloc(argc, sizeof(const char *));
-		for (i = 1; i < argc; i++)
+		for (i = 1; i < argc; i++) {
 			pattern[i - 1] = xstrfmt("*/%s", argv[i]);
+
+			if (strchr(argv[i], '*'))
+				argv_array_push(&ref_patterns, argv[i]);
+			else
+				expand_ref_pattern(&ref_patterns, argv[i]);
+		}
 	}
 
 	remote = remote_get(dest);
@@ -96,7 +104,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	if (uploadpack != NULL)
 		transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);
 
-	ref = transport_get_remote_refs(transport, NULL);
+	ref = transport_get_remote_refs(transport, &ref_patterns);
 	if (transport_disconnect(transport))
 		return 1;
 
refs.c +14 -0
index 20ba82b43..58e9f88fb 100644
@@ -13,6 +13,7 @@
 #include "tag.h"
 #include "submodule.h"
 #include "worktree.h"
+#include "argv-array.h"
 
 /*
  * List of all available backends
@@ -501,6 +502,19 @@ int refname_match(const char *abbrev_name, const char *full_name)
 	return 0;
 }
 
+/*
+ * Given a 'pattern' expand it by the rules in 'ref_rev_parse_rules' and add
+ * the results to 'patterns'
+ */
+void expand_ref_pattern(struct argv_array *patterns, const char *pattern)
+{
+	const char **p;
+	for (p = ref_rev_parse_rules; *p; p++) {
+		int len = strlen(pattern);
+		argv_array_pushf(patterns, *p, len, pattern);
+	}
+}
+
 /*
  * *string and *len will only be substituted, and *string returned (for
  * later free()ing) if the string passed in is a magic short-hand form
refs.h +7 -0
index 01be5ae32..292ca35ce 100644
@@ -139,6 +139,13 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
  */
 int refname_match(const char *abbrev_name, const char *full_name);
 
+/*
+ * Given a 'pattern' expand it by the rules in 'ref_rev_parse_rules' and add
+ * the results to 'patterns'
+ */
+struct argv_array;
+void expand_ref_pattern(struct argv_array *patterns, const char *pattern);
+
 int expand_ref(const char *str, int len, struct object_id *oid, char **ref);
 int dwim_ref(const char *str, int len, struct object_id *oid, char **ref);
 int dwim_log(const char *str, int len, struct object_id *oid, char **ref);
t/t5702-protocol-v2.sh +26 -0
index dc5f813be..562610fd2 100755
@@ -32,6 +32,19 @@ test_expect_success 'list refs with git:// using protocol v2' '
 	test_cmp actual expect
 '
 
+test_expect_success 'ref advertisment is filtered with ls-remote using protocol v2' '
+	test_when_finished "rm -f log" &&
+
+	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
+		ls-remote "$GIT_DAEMON_URL/parent" master >actual &&
+
+	cat >expect <<-EOF &&
+	$(git -C "$daemon_parent" rev-parse refs/heads/master)$(printf "\t")refs/heads/master
+	EOF
+
+	test_cmp actual expect
+'
+
 stop_git_daemon
 
 # Test protocol v2 with 'file://' transport
@@ -54,4 +67,17 @@ test_expect_success 'list refs with file:// using protocol v2' '
 	test_cmp actual expect
 '
 
+test_expect_success 'ref advertisment is filtered with ls-remote using protocol v2' '
+	test_when_finished "rm -f log" &&
+
+	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
+		ls-remote "file://$(pwd)/file_parent" master >actual &&
+
+	cat >expect <<-EOF &&
+	$(git -C file_parent rev-parse refs/heads/master)$(printf "\t")refs/heads/master
+	EOF
+
+	test_cmp actual expect
+'
+
 test_done
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#327 lore raw

Re: [PATCH v4 17/35] ls-remote: pass ref patterns when requesting a remote's refs

Quoted 50 lines · Brandon Williams

Construct an argv_array of the ref patterns supplied via the command line and pass them to 'transport_get_remote_refs()' to be used when communicating protocol v2 so that the server can limit the ref advertisement based on the supplied patterns.

---
 builtin/ls-remote.c    | 12 ++++++++++--
 refs.c                 | 14 ++++++++++++++
 refs.h                 |  7 +++++++
 t/t5702-protocol-v2.sh | 26 ++++++++++++++++++++++++++
 4 files changed, 57 insertions(+), 2 deletions(-)
builtin/ls-remote.c +9 -1
index c6e9847c5..083ba8b29 100644
@@ -2,6 +2,7 @@
 #include "cache.h"
 #include "transport.h"
 #include "remote.h"
+#include "refs.h"
 
 static const char * const ls_remote_usage[] = {
 	N_("git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>]\n"
@@ -43,6 +44,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	int show_symref_target = 0;
 	const char *uploadpack = NULL;
 	const char **pattern = NULL;
+	struct argv_array ref_patterns = ARGV_ARRAY_INIT;
 
 	struct remote *remote;
 	struct transport *transport;
@@ -74,8 +76,14 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	if (argc > 1) {
 		int i;
 		pattern = xcalloc(argc, sizeof(const char *));
-		for (i = 1; i < argc; i++)
+		for (i = 1; i < argc; i++) {
 			pattern[i - 1] = xstrfmt("*/%s", argv[i]);
+
+			if (strchr(argv[i], '*'))
+				argv_array_push(&ref_patterns, argv[i]);
+			else
+				expand_ref_pattern(&ref_patterns, argv[i]);
+		}
 	}
 
 	remote = remote_get(dest);

builtin/ls-remote.c int cmd_ls_remote(int argc, const char **argv, const char *prefix) · line 107

Quoted 5 lines
builtin/ls-remote.c +1 -1
@@ -96,7 +104,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	if (uploadpack != NULL)
 		transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);
 
-	ref = transport_get_remote_refs(transport, NULL);
+	ref = transport_get_remote_refs(transport, &ref_patterns);

Yup, this is a logical and an obvious conclusion of the past handful of steps ;-) I actually was wondering why the previous step didn't do this already, but the resulting series is easier to understand if this is kept as a separate step.

However, this also means that traditional pattern language ls-remote used to support dictates what ls-refs command over the wire can take, which may not be optimal.

#328 lore raw

PATCH v4 18/35 fetch: pass ref patterns when fetching

In reply to Brandon Williams

Construct a list of ref patterns to be passed to 'transport_get_remote_refs()' from the refspec to be used during the fetch. This list of ref patterns will be used to allow the server to filter the ref advertisement when communicating using protocol v2.

---
 builtin/fetch.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
builtin/fetch.c +15 -1
index 850382f55..695fafe06 100644
@@ -332,11 +332,25 @@ static struct ref *get_ref_map(struct transport *transport,
 	struct ref *rm;
 	struct ref *ref_map = NULL;
 	struct ref **tail = &ref_map;
+	struct argv_array ref_patterns = ARGV_ARRAY_INIT;
 
 	/* opportunistically-updated references: */
 	struct ref *orefs = NULL, **oref_tail = &orefs;
 
-	const struct ref *remote_refs = transport_get_remote_refs(transport, NULL);
+	const struct ref *remote_refs;
+
+	for (i = 0; i < refspec_count; i++) {
+		if (!refspecs[i].exact_sha1) {
+			if (refspecs[i].pattern)
+				argv_array_push(&ref_patterns, refspecs[i].src);
+			else
+				expand_ref_pattern(&ref_patterns, refspecs[i].src);
+		}
+	}
+
+	remote_refs = transport_get_remote_refs(transport, &ref_patterns);
+
+	argv_array_clear(&ref_patterns);
 
 	if (refspec_count) {
 		struct refspec *fetch_refspec;
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#329 lore raw

Re: [PATCH v4 18/35] fetch: pass ref patterns when fetching

Quoted 18 lines · Brandon Williams
builtin/fetch.c +5 -1
index 850382f55..695fafe06 100644
@@ -332,11 +332,25 @@ static struct ref *get_ref_map(struct transport *transport,
 	struct ref *rm;
 	struct ref *ref_map = NULL;
 	struct ref **tail = &ref_map;
+	struct argv_array ref_patterns = ARGV_ARRAY_INIT;
 
 	/* opportunistically-updated references: */
 	struct ref *orefs = NULL, **oref_tail = &orefs;
 
-	const struct ref *remote_refs = transport_get_remote_refs(transport, NULL);
+	const struct ref *remote_refs;
+
+	for (i = 0; i < refspec_count; i++) {
+		if (!refspecs[i].exact_sha1) {

builtin/fetch.c static struct ref *get_ref_map(struct transport *transport, · line 353

Quoted 10 lines
builtin/fetch.c +10 -0
+			if (refspecs[i].pattern)
+				argv_array_push(&ref_patterns, refspecs[i].src);
+			else
+				expand_ref_pattern(&ref_patterns, refspecs[i].src);
+		}
+	}
+
+	remote_refs = transport_get_remote_refs(transport, &ref_patterns);
+
+	argv_array_clear(&ref_patterns);

Is the idea here, which is shared with 17/35 about ls-remote, that we used to grab literally everything they have in remote_refs, but we have code in place to filter that set using refspecs given in the remote.*.fetch configuration, so it is OK as long as we grab everything that would match the remote.*.fetch pattern? That is, grabbing too much is acceptable, but if we populated ref_patterns[] with too few patterns and fail to ask refs that would match our refspec it would be a bug?

The reason behind this question is that I am wondering if/how we can take advantage of this remote-side pre-filtering while doing "fetch --prune".

Thanks.

Quoted 3 lines
builtin/fetch.c +0 -0
 	if (refspec_count) {
 		struct refspec *fetch_refspec;
#330 lore raw

Re: [PATCH v4 18/35] fetch: pass ref patterns when fetching

Quoted 31 lines · Junio C Hamano
Quoted 18 lines · Brandon Williams
builtin/fetch.c +5 -1
index 850382f55..695fafe06 100644
@@ -332,11 +332,25 @@ static struct ref *get_ref_map(struct transport *transport,
 	struct ref *rm;
 	struct ref *ref_map = NULL;
 	struct ref **tail = &ref_map;
+	struct argv_array ref_patterns = ARGV_ARRAY_INIT;
 
 	/* opportunistically-updated references: */
 	struct ref *orefs = NULL, **oref_tail = &orefs;
 
-	const struct ref *remote_refs = transport_get_remote_refs(transport, NULL);
+	const struct ref *remote_refs;
+
+	for (i = 0; i < refspec_count; i++) {
+		if (!refspecs[i].exact_sha1) {

builtin/fetch.c static struct ref *get_ref_map(struct transport *transport, · line 353

Quoted 10 lines
builtin/fetch.c +10 -0
+			if (refspecs[i].pattern)
+				argv_array_push(&ref_patterns, refspecs[i].src);
+			else
+				expand_ref_pattern(&ref_patterns, refspecs[i].src);
+		}
+	}
+
+	remote_refs = transport_get_remote_refs(transport, &ref_patterns);
+
+	argv_array_clear(&ref_patterns);
Quoted 8 lines

Is the idea here, which is shared with 17/35 about ls-remote, that we used to grab literally everything they have in remote_refs, but we have code in place to filter that set using refspecs given in the remote.*.fetch configuration, so it is OK as long as we grab everything that would match the remote.*.fetch pattern? That is, grabbing too much is acceptable, but if we populated ref_patterns[] with too few patterns and fail to ask refs that would match our refspec it would be a bug?

Yes that's the idea. Right now we're in the state where we ask for everything (since there is no server side filtering) and the client just does its own filtering after the fact using the refspec. So if we end up not sending enough ref patterns to match what the refspec is, it would be a bug.

Quoted 4 lines

The reason behind this question is that I am wondering if/how we can take advantage of this remote-side pre-filtering while doing "fetch --prune".

Hmm maybe, assuming prune then means "get rid of all remote-tracking branches that don't match the user provided refspec"

Quoted 6 lines

Thanks.

Quoted 3 lines
builtin/fetch.c +0 -0
 	if (refspec_count) {
 		struct refspec *fetch_refspec;
Signature · 1 line
Brandon Williams
#331 lore raw

PATCH v4 19/35 push: pass ref patterns when pushing

In reply to Brandon Williams

Construct a list of ref patterns to be passed to 'get_refs_list()' from the refspec to be used during the push. This list of ref patterns will be used to allow the server to filter the ref advertisement when communicating using protocol v2.

---
 transport.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)
transport.c +25 -1
index dfc603b36..bf7ba6879 100644
@@ -1026,11 +1026,35 @@ int transport_push(struct transport *transport,
 		int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
 		int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
 		int push_ret, ret, err;
+		struct refspec *tmp_rs;
+		struct argv_array ref_patterns = ARGV_ARRAY_INIT;
+		int i;
 
 		if (check_push_refs(local_refs, refspec_nr, refspec) < 0)
 			return -1;
 
-		remote_refs = transport->vtable->get_refs_list(transport, 1, NULL);
+		tmp_rs = parse_push_refspec(refspec_nr, refspec);
+		for (i = 0; i < refspec_nr; i++) {
+			const char *pattern = NULL;
+
+			if (tmp_rs[i].dst)
+				pattern = tmp_rs[i].dst;
+			else if (tmp_rs[i].src && !tmp_rs[i].exact_sha1)
+				pattern = tmp_rs[i].src;
+
+			if (pattern) {
+				if (tmp_rs[i].pattern)
+					argv_array_push(&ref_patterns, pattern);
+				else
+					expand_ref_pattern(&ref_patterns, pattern);
+			}
+		}
+
+		remote_refs = transport->vtable->get_refs_list(transport, 1,
+							       &ref_patterns);
+
+		argv_array_clear(&ref_patterns);
+		free_refspec(refspec_nr, tmp_rs);
 
 		if (flags & TRANSPORT_PUSH_ALL)
 			match_flags |= MATCH_REFS_ALL;
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#332 lore raw

Re: [PATCH v4 19/35] push: pass ref patterns when pushing

Quoted 9 lines · Brandon Williams

Construct a list of ref patterns to be passed to 'get_refs_list()' from the refspec to be used during the push. This list of ref patterns will be used to allow the server to filter the ref advertisement when communicating using protocol v2.

---
 transport.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

When you are pushing 'master', we no longer hear what the other end has at 'next', with this change, no?

In a project whose 'master' is extended primarily by merging topics that have been cooking in 'next', old way of pushing would only have transferred the merge commits and resulting trees but not bulk of the blob data because they are all available on 'next', would it make the object transfer far less efficient, I wonder?

I guess it is OK only because the push side of the current protocol does not do common ancestor discovery exchange ;-)

Quoted 42 lines
transport.c +25 -1
index dfc603b36..bf7ba6879 100644
@@ -1026,11 +1026,35 @@ int transport_push(struct transport *transport,
 		int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
 		int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
 		int push_ret, ret, err;
+		struct refspec *tmp_rs;
+		struct argv_array ref_patterns = ARGV_ARRAY_INIT;
+		int i;
 
 		if (check_push_refs(local_refs, refspec_nr, refspec) < 0)
 			return -1;
 
-		remote_refs = transport->vtable->get_refs_list(transport, 1, NULL);
+		tmp_rs = parse_push_refspec(refspec_nr, refspec);
+		for (i = 0; i < refspec_nr; i++) {
+			const char *pattern = NULL;
+
+			if (tmp_rs[i].dst)
+				pattern = tmp_rs[i].dst;
+			else if (tmp_rs[i].src && !tmp_rs[i].exact_sha1)
+				pattern = tmp_rs[i].src;
+
+			if (pattern) {
+				if (tmp_rs[i].pattern)
+					argv_array_push(&ref_patterns, pattern);
+				else
+					expand_ref_pattern(&ref_patterns, pattern);
+			}
+		}
+
+		remote_refs = transport->vtable->get_refs_list(transport, 1,
+							       &ref_patterns);
+
+		argv_array_clear(&ref_patterns);
+		free_refspec(refspec_nr, tmp_rs);
 
 		if (flags & TRANSPORT_PUSH_ALL)
 			match_flags |= MATCH_REFS_ALL;
#333 lore raw

Re: [PATCH v4 19/35] push: pass ref patterns when pushing

Quoted 15 lines · Junio C Hamano
Quoted 9 lines · Brandon Williams

Construct a list of ref patterns to be passed to 'get_refs_list()' from the refspec to be used during the push. This list of ref patterns will be used to allow the server to filter the ref advertisement when communicating using protocol v2.

---
 transport.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

When you are pushing 'master', we no longer hear what the other end has at 'next', with this change, no?

Quoted 8 lines

In a project whose 'master' is extended primarily by merging topics that have been cooking in 'next', old way of pushing would only have transferred the merge commits and resulting trees but not bulk of the blob data because they are all available on 'next', would it make the object transfer far less efficient, I wonder?

I guess it is OK only because the push side of the current protocol does not do common ancestor discovery exchange ;-)

Yep, though we've been throwing around ideas of adding that in push v2 after we figure out a good way to improve negotiation with fetch.

Quoted 43 lines
Quoted 43 lines
transport.c +25 -1
index dfc603b36..bf7ba6879 100644
@@ -1026,11 +1026,35 @@ int transport_push(struct transport *transport,
 		int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
 		int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
 		int push_ret, ret, err;
+		struct refspec *tmp_rs;
+		struct argv_array ref_patterns = ARGV_ARRAY_INIT;
+		int i;
 
 		if (check_push_refs(local_refs, refspec_nr, refspec) < 0)
 			return -1;
 
-		remote_refs = transport->vtable->get_refs_list(transport, 1, NULL);
+		tmp_rs = parse_push_refspec(refspec_nr, refspec);
+		for (i = 0; i < refspec_nr; i++) {
+			const char *pattern = NULL;
+
+			if (tmp_rs[i].dst)
+				pattern = tmp_rs[i].dst;
+			else if (tmp_rs[i].src && !tmp_rs[i].exact_sha1)
+				pattern = tmp_rs[i].src;
+
+			if (pattern) {
+				if (tmp_rs[i].pattern)
+					argv_array_push(&ref_patterns, pattern);
+				else
+					expand_ref_pattern(&ref_patterns, pattern);
+			}
+		}
+
+		remote_refs = transport->vtable->get_refs_list(transport, 1,
+							       &ref_patterns);
+
+		argv_array_clear(&ref_patterns);
+		free_refspec(refspec_nr, tmp_rs);
 
 		if (flags & TRANSPORT_PUSH_ALL)
 			match_flags |= MATCH_REFS_ALL;
Signature · 1 line
Brandon Williams
#334 lore raw

PATCH v4 20/35 upload-pack: introduce fetch server command

In reply to Brandon Williams

Introduce the 'fetch' server command.

---
 Documentation/technical/protocol-v2.txt | 128 ++++++++++++
 serve.c                                 |   2 +
 t/t5701-git-serve.sh                    |   1 +
 upload-pack.c                           | 267 ++++++++++++++++++++++++
 upload-pack.h                           |   5 +
 5 files changed, 403 insertions(+)
Documentation/technical/protocol-v2.txt +128 -0
index 7f50e6462..99c70a1e4 100644
@@ -205,3 +205,131 @@ The output of ls-refs is as follows:
     ref-attribute = (symref | peeled)
     symref = "symref-target:" symref-target
     peeled = "peeled:" obj-id
+
+ fetch
+-------
+
+`fetch` is the command used to fetch a packfile in v2.  It can be looked
+at as a modified version of the v1 fetch where the ref-advertisement is
+stripped out (since the `ls-refs` command fills that role) and the
+message format is tweaked to eliminate redundancies and permit easy
+addition of future extensions.
+
+Additional features not supported in the base command will be advertised
+as the value of the command in the capability advertisement in the form
+of a space separated list of features, e.g.  "<command>=<feature 1>
+<feature 2>".
+
+A `fetch` request can take the following arguments:
+
+    want <oid>
+	Indicates to the server an object which the client wants to
+	retrieve.  Wants can be anything and are not limited to
+	advertised objects.
+
+    have <oid>
+	Indicates to the server an object which the client has locally.
+	This allows the server to make a packfile which only contains
+	the objects that the client needs. Multiple 'have' lines can be
+	supplied.
+
+    done
+	Indicates to the server that negotiation should terminate (or
+	not even begin if performing a clone) and that the server should
+	use the information supplied in the request to construct the
+	packfile.
+
+    thin-pack
+	Request that a thin pack be sent, which is a pack with deltas
+	which reference base objects not contained within the pack (but
+	are known to exist at the receiving end). This can reduce the
+	network traffic significantly, but it requires the receiving end
+	to know how to "thicken" these packs by adding the missing bases
+	to the pack.
+
+    no-progress
+	Request that progress information that would normally be sent on
+	side-band channel 2, during the packfile transfer, should not be
+	sent.  However, the side-band channel 3 is still used for error
+	responses.
+
+    include-tag
+	Request that annotated tags should be sent if the objects they
+	point to are being sent.
+
+    ofs-delta
+	Indicate that the client understands PACKv2 with delta referring
+	to its base by position in pack rather than by an oid.  That is,
+	they can read OBJ_OFS_DELTA (ake type 6) in a packfile.
+
+The response of `fetch` is broken into a number of sections separated by
+delimiter packets (0001), with each section beginning with its section
+header.
+
+    output = *section
+    section = (acknowledgments | packfile)
+	      (flush-pkt | delim-pkt)
+
+    acknowledgments = PKT-LINE("acknowledgments" LF)
+		      (nak | *ack)
+		      (ready)
+    ready = PKT-LINE("ready" LF)
+    nak = PKT-LINE("NAK" LF)
+    ack = PKT-LINE("ACK" SP obj-id LF)
+
+    packfile = PKT-LINE("packfile" LF)
+	       [PACKFILE]
+
+----
+    acknowledgments section
+	* Always begins with the section header "acknowledgments"
+
+	* The server will respond with "NAK" if none of the object ids sent
+	  as have lines were common.
+
+	* The server will respond with "ACK obj-id" for all of the
+	  object ids sent as have lines which are common.
+
+	* A response cannot have both "ACK" lines as well as a "NAK"
+	  line.
+
+	* The server will respond with a "ready" line indicating that
+	  the server has found an acceptable common base and is ready to
+	  make and send a packfile (which will be found in the packfile
+	  section of the same response)
+
+	* If the client determines that it is finished with negotiations
+	  by sending a "done" line, the acknowledgments sections MUST be
+	  omitted from the server's response.
+
+	* If the server has found a suitable cut point and has decided
+	  to send a "ready" line, then the server can decide to (as an
+	  optimization) omit any "ACK" lines it would have sent during
+	  its response.  This is because the server will have already
+	  determined the objects it plans to send to the client and no
+	  further negotiation is needed.
+
+----
+    packfile section
+	* Always begins with the section header "packfile"
+
+	* The transmission of the packfile begins immediately after the
+	  section header
+
+	* The data transfer of the packfile is always multiplexed, using
+	  the same semantics of the 'side-band-64k' capability from
+	  protocol version 1.  This means that each packet, during the
+	  packfile data stream, is made up of a leading 4-byte pkt-line
+	  length (typical of the pkt-line format), followed by a 1-byte
+	  stream code, followed by the actual data.
+
+	  The stream code can be one of:
+		1 - pack data
+		2 - progress messages
+		3 - fatal error message just before stream aborts
+
+	* This section is only included if the client has sent 'want'
+	  lines in its request and either requested that no more
+	  negotiation be done by sending 'done' or if the server has
+	  decided it has found a sufficient cut point to produce a
+	  packfile.
serve.c +2 -0
index c7925c5c7..05cc434cf 100644
@@ -6,6 +6,7 @@
 #include "argv-array.h"
 #include "ls-refs.h"
 #include "serve.h"
+#include "upload-pack.h"
 
 static int always_advertise(struct repository *r,
 			    struct strbuf *value)
@@ -52,6 +53,7 @@ struct protocol_capability {
 static struct protocol_capability capabilities[] = {
 	{ "agent", agent_advertise, NULL },
 	{ "ls-refs", always_advertise, ls_refs },
+	{ "fetch", always_advertise, upload_pack_v2 },
 };
 
 static void advertise_capabilities(void)
t/t5701-git-serve.sh +1 -0
index 11aeb0541..cc5918a67 100755
@@ -9,6 +9,7 @@ test_expect_success 'test capability advertisement' '
 	version 2
 	agent=git/$(git version | cut -d" " -f3)
 	ls-refs
+	fetch
 	0000
 	EOF
 
upload-pack.c +267 -0

276 lines not on this page · raw message

upload-pack.h +5 -0
index a71e4dc7e..6b7890238 100644
@@ -10,4 +10,9 @@ struct upload_pack_options {
 
 void upload_pack(struct upload_pack_options *options);
 
+struct repository;
+struct argv_array;
+extern int upload_pack_v2(struct repository *r, struct argv_array *keys,
+			  struct argv_array *args);
+
 #endif /* UPLOAD_PACK_H */
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#335 lore raw

Re: [PATCH v4 20/35] upload-pack: introduce fetch server command

Documentation/technical/protocol-v2.txt The output of ls-refs is as follows: · line 281

Quoted 13 lines · On Wed, 28 Feb 2018 15:22:37 -0800 Brandon Williams <[email protected]> wrote:
Documentation/technical/protocol-v2.txt +13 -0
+    output = *section
+    section = (acknowledgments | packfile)
+	      (flush-pkt | delim-pkt)
+
+    acknowledgments = PKT-LINE("acknowledgments" LF)
+		      (nak | *ack)
+		      (ready)
+    ready = PKT-LINE("ready" LF)
+    nak = PKT-LINE("NAK" LF)
+    ack = PKT-LINE("ACK" SP obj-id LF)
+
+    packfile = PKT-LINE("packfile" LF)
+	       [PACKFILE]

I should have noticed this earlier, but "PACKFILE" is not defined anywhere - it's probably better written as:

    *PKT-LINE(%x01-03 *%x00-ff)"

or something like that.

Quoted 42 lines
Documentation/technical/protocol-v2.txt +42 -0
+    acknowledgments section
+	* Always begins with the section header "acknowledgments"
+
+	* The server will respond with "NAK" if none of the object ids sent
+	  as have lines were common.
+
+	* The server will respond with "ACK obj-id" for all of the
+	  object ids sent as have lines which are common.
+
+	* A response cannot have both "ACK" lines as well as a "NAK"
+	  line.
+
+	* The server will respond with a "ready" line indicating that
+	  the server has found an acceptable common base and is ready to
+	  make and send a packfile (which will be found in the packfile
+	  section of the same response)
+
+	* If the client determines that it is finished with negotiations
+	  by sending a "done" line, the acknowledgments sections MUST be
+	  omitted from the server's response.
+
+	* If the server has found a suitable cut point and has decided
+	  to send a "ready" line, then the server can decide to (as an
+	  optimization) omit any "ACK" lines it would have sent during
+	  its response.  This is because the server will have already
+	  determined the objects it plans to send to the client and no
+	  further negotiation is needed.
+
+----
+    packfile section
+	* Always begins with the section header "packfile"
+
+	* The transmission of the packfile begins immediately after the
+	  section header
+
+	* The data transfer of the packfile is always multiplexed, using
+	  the same semantics of the 'side-band-64k' capability from
+	  protocol version 1.  This means that each packet, during the
+	  packfile data stream, is made up of a leading 4-byte pkt-line
+	  length (typical of the pkt-line format), followed by a 1-byte
+	  stream code, followed by the actual data.
+

Documentation/technical/protocol-v2.txt The output of ls-refs is as follows: · line 335

Quoted 10 lines
Documentation/technical/protocol-v2.txt +10 -0
+	  The stream code can be one of:
+		1 - pack data
+		2 - progress messages
+		3 - fatal error message just before stream aborts
+
+	* This section is only included if the client has sent 'want'
+	  lines in its request and either requested that no more
+	  negotiation be done by sending 'done' or if the server has
+	  decided it has found a sufficient cut point to produce a
+	  packfile.

For both the sections, I think that the conditions for inclusion/non-inclusion ("This section is only included if...") should be the first point.

upload-pack.c void upload_pack(struct upload_pack_options *options) · line 1091

Quoted 9 lines
upload-pack.c +9 -0
+static void upload_pack_data_init(struct upload_pack_data *data)
+{
+	struct object_array wants = OBJECT_ARRAY_INIT;
+	struct oid_array haves = OID_ARRAY_INIT;
+
+	memset(data, 0, sizeof(*data));
+	data->wants = wants;
+	data->haves = haves;
+}

Any reason to use a initializer function instead of a static literal?

#336 lore raw

Re: [PATCH v4 20/35] upload-pack: introduce fetch server command

Quoted 20 lines · Jonathan Tan

Documentation/technical/protocol-v2.txt The output of ls-refs is as follows: · line 281

Quoted 13 lines · On Wed, 28 Feb 2018 15:22:37 -0800 Brandon Williams <[email protected]> wrote:
Documentation/technical/protocol-v2.txt +13 -0
+    output = *section
+    section = (acknowledgments | packfile)
+	      (flush-pkt | delim-pkt)
+
+    acknowledgments = PKT-LINE("acknowledgments" LF)
+		      (nak | *ack)
+		      (ready)
+    ready = PKT-LINE("ready" LF)
+    nak = PKT-LINE("NAK" LF)
+    ack = PKT-LINE("ACK" SP obj-id LF)
+
+    packfile = PKT-LINE("packfile" LF)
+	       [PACKFILE]

I should have noticed this earlier, but "PACKFILE" is not defined anywhere - it's probably better written as:

Quoted 3 lines
    *PKT-LINE(%x01-03 *%x00-ff)"

or something like that.

I'll document it as you described.

Quoted 58 lines
Quoted 43 lines
Documentation/technical/protocol-v2.txt +42 -0
+    acknowledgments section
+	* Always begins with the section header "acknowledgments"
+
+	* The server will respond with "NAK" if none of the object ids sent
+	  as have lines were common.
+
+	* The server will respond with "ACK obj-id" for all of the
+	  object ids sent as have lines which are common.
+
+	* A response cannot have both "ACK" lines as well as a "NAK"
+	  line.
+
+	* The server will respond with a "ready" line indicating that
+	  the server has found an acceptable common base and is ready to
+	  make and send a packfile (which will be found in the packfile
+	  section of the same response)
+
+	* If the client determines that it is finished with negotiations
+	  by sending a "done" line, the acknowledgments sections MUST be
+	  omitted from the server's response.
+
+	* If the server has found a suitable cut point and has decided
+	  to send a "ready" line, then the server can decide to (as an
+	  optimization) omit any "ACK" lines it would have sent during
+	  its response.  This is because the server will have already
+	  determined the objects it plans to send to the client and no
+	  further negotiation is needed.
+
+----
+    packfile section
+	* Always begins with the section header "packfile"
+
+	* The transmission of the packfile begins immediately after the
+	  section header
+
+	* The data transfer of the packfile is always multiplexed, using
+	  the same semantics of the 'side-band-64k' capability from
+	  protocol version 1.  This means that each packet, during the
+	  packfile data stream, is made up of a leading 4-byte pkt-line
+	  length (typical of the pkt-line format), followed by a 1-byte
+	  stream code, followed by the actual data.
+

Documentation/technical/protocol-v2.txt The output of ls-refs is as follows: · line 335

Quoted 10 lines
Documentation/technical/protocol-v2.txt +10 -0
+	  The stream code can be one of:
+		1 - pack data
+		2 - progress messages
+		3 - fatal error message just before stream aborts
+
+	* This section is only included if the client has sent 'want'
+	  lines in its request and either requested that no more
+	  negotiation be done by sending 'done' or if the server has
+	  decided it has found a sufficient cut point to produce a
+	  packfile.

For both the sections, I think that the conditions for inclusion/non-inclusion ("This section is only included if...") should be the first point.

Quoted 11 lines

upload-pack.c void upload_pack(struct upload_pack_options *options) · line 1091

Quoted 9 lines
upload-pack.c +9 -0
+static void upload_pack_data_init(struct upload_pack_data *data)
+{
+	struct object_array wants = OBJECT_ARRAY_INIT;
+	struct oid_array haves = OID_ARRAY_INIT;
+
+	memset(data, 0, sizeof(*data));
+	data->wants = wants;
+	data->haves = haves;
+}

Any reason to use a initializer function instead of a static literal?

Its much cleaner and easier to read than it was when i was using an initializer.

Signature · 1 line
Brandon Williams
#337 lore raw

PATCH v4 21/35 fetch-pack: perform a fetch using v2

In reply to Brandon Williams

When communicating with a v2 server, perform a fetch by requesting the 'fetch' command.

---
 Documentation/technical/protocol-v2.txt |  68 +++++-
 builtin/fetch-pack.c                    |   2 +-
 fetch-pack.c                            | 270 +++++++++++++++++++++++-
 fetch-pack.h                            |   4 +-
 serve.c                                 |   2 +-
 t/t5701-git-serve.sh                    |   2 +-
 t/t5702-protocol-v2.sh                  |  97 +++++++++
 transport.c                             |   7 +-
 upload-pack.c                           | 141 ++++++++++---
 upload-pack.h                           |   3 +
 10 files changed, 548 insertions(+), 48 deletions(-)
Documentation/technical/protocol-v2.txt +67 -1
index 99c70a1e4..0d63456fc 100644
@@ -262,12 +262,43 @@ A `fetch` request can take the following arguments:
 	to its base by position in pack rather than by an oid.  That is,
 	they can read OBJ_OFS_DELTA (ake type 6) in a packfile.
 
+    shallow <oid>
+	A client must notify the server of all commits for which it only
+	has shallow copies (meaning that it doesn't have the parents of
+	a commit) by supplying a 'shallow <oid>' line for each such
+	object so that the server is aware of the limitations of the
+	client's history.  This is so that the server is aware that the
+	client may not have all objects reachable from such commits.
+
+    deepen <depth>
+	Requests that the fetch/clone should be shallow having a commit
+	depth of <depth> relative to the remote side.
+
+    deepen-relative
+	Requests that the semantics of the "deepen" command be changed
+	to indicate that the depth requested is relative to the client's
+	current shallow boundary, instead of relative to the requested
+	commits.
+
+    deepen-since <timestamp>
+	Requests that the shallow clone/fetch should be cut at a
+	specific time, instead of depth.  Internally it's equivalent to
+	doing "git rev-list --max-age=<timestamp>". Cannot be used with
+	"deepen".
+
+    deepen-not <rev>
+	Requests that the shallow clone/fetch should be cut at a
+	specific revision specified by '<rev>', instead of a depth.
+	Internally it's equivalent of doing "git rev-list --not <rev>".
+	Cannot be used with "deepen", but can be used with
+	"deepen-since".
+
 The response of `fetch` is broken into a number of sections separated by
 delimiter packets (0001), with each section beginning with its section
 header.
 
     output = *section
-    section = (acknowledgments | packfile)
+    section = (acknowledgments | shallow-info | packfile)
 	      (flush-pkt | delim-pkt)
 
     acknowledgments = PKT-LINE("acknowledgments" LF)
@@ -277,6 +308,11 @@ header.
     nak = PKT-LINE("NAK" LF)
     ack = PKT-LINE("ACK" SP obj-id LF)
 
+    shallow-info = PKT-LINE("shallow-info" LF)
+		   *PKT-LINE((shallow | unshallow) LF)
+    shallow = "shallow" SP obj-id
+    unshallow = "unshallow" SP obj-id
+
     packfile = PKT-LINE("packfile" LF)
 	       [PACKFILE]
 
@@ -309,6 +345,36 @@ header.
 	  determined the objects it plans to send to the client and no
 	  further negotiation is needed.
 
+----
+    shallow-info section
+	If the client has requested a shallow fetch/clone, a shallow
+	client requests a fetch or the server is shallow then the
+	server's response may include a shallow-info section.  The
+	shallow-info section will be included if (due to one of the
+	above conditions) the server needs to inform the client of any
+	shallow boundaries or adjustments to the clients already
+	existing shallow boundaries.
+
+	* Always begins with the section header "shallow-info"
+
+	* If a positive depth is requested, the server will compute the
+	  set of commits which are no deeper than the desired depth.
+
+	* The server sends a "shallow obj-id" line for each commit whose
+	  parents will not be sent in the following packfile.
+
+	* The server sends an "unshallow obj-id" line for each commit
+	  which the client has indicated is shallow, but is no longer
+	  shallow as a result of the fetch (due to its parents being
+	  sent in the following packfile).
+
+	* The server MUST NOT send any "unshallow" lines for anything
+	  which the client has not indicated was shallow as a part of
+	  its request.
+
+	* This section is only included if a packfile section is also
+	  included in the response.
+
 ----
     packfile section
 	* Always begins with the section header "packfile"
builtin/fetch-pack.c +1 -1
index b2374ddbb..f9d7d0b5a 100644
@@ -212,7 +212,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	}
 
 	ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
-			 &shallow, pack_lockfile_ptr);
+			 &shallow, pack_lockfile_ptr, protocol_v0);
 	if (pack_lockfile) {
 		printf("lock %s\n", pack_lockfile);
 		fflush(stdout);
fetch-pack.c +264 -6

301 lines not on this page · raw message

fetch-pack.h +3 -1
index b6aeb43a8..7afca7305 100644
@@ -3,6 +3,7 @@
 
 #include "string-list.h"
 #include "run-command.h"
+#include "protocol.h"
 
 struct oid_array;
 
@@ -43,7 +44,8 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
 		       struct ref **sought,
 		       int nr_sought,
 		       struct oid_array *shallow,
-		       char **pack_lockfile);
+		       char **pack_lockfile,
+		       enum protocol_version version);
 
 /*
  * Print an appropriate error message for each sought ref that wasn't
serve.c +1 -1
index 05cc434cf..c3e58c1e7 100644
@@ -53,7 +53,7 @@ struct protocol_capability {
 static struct protocol_capability capabilities[] = {
 	{ "agent", agent_advertise, NULL },
 	{ "ls-refs", always_advertise, ls_refs },
-	{ "fetch", always_advertise, upload_pack_v2 },
+	{ "fetch", upload_pack_advertise, upload_pack_v2 },
 };
 
 static void advertise_capabilities(void)
t/t5701-git-serve.sh +1 -1
index cc5918a67..569922f7a 100755
@@ -9,7 +9,7 @@ test_expect_success 'test capability advertisement' '
 	version 2
 	agent=git/$(git version | cut -d" " -f3)
 	ls-refs
-	fetch
+	fetch=shallow
 	0000
 	EOF
 
t/t5702-protocol-v2.sh +97 -0

107 lines not on this page · raw message

transport.c +5 -2
index bf7ba6879..8e38352c5 100644
@@ -256,14 +256,17 @@ static int fetch_refs_via_pack(struct transport *transport,
 
 	switch (data->version) {
 	case protocol_v2:
-		die("support for protocol v2 not implemented yet");
+		refs = fetch_pack(&args, data->fd, data->conn,
+				  refs_tmp ? refs_tmp : transport->remote_refs,
+				  dest, to_fetch, nr_heads, &data->shallow,
+				  &transport->pack_lockfile, data->version);
 		break;
 	case protocol_v1:
 	case protocol_v0:
 		refs = fetch_pack(&args, data->fd, data->conn,
 				  refs_tmp ? refs_tmp : transport->remote_refs,
 				  dest, to_fetch, nr_heads, &data->shallow,
-				  &transport->pack_lockfile);
+				  &transport->pack_lockfile, data->version);
 		break;
 	case protocol_unknown_version:
 		BUG("unknown protocol version");
upload-pack.c +106 -35

203 lines not on this page · raw message

upload-pack.h +3 -0
index 6b7890238..7720f2142 100644
@@ -14,5 +14,8 @@ struct repository;
 struct argv_array;
 extern int upload_pack_v2(struct repository *r, struct argv_array *keys,
 			  struct argv_array *args);
+struct strbuf;
+extern int upload_pack_advertise(struct repository *r,
+				 struct strbuf *value);
 
 #endif /* UPLOAD_PACK_H */
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#338 lore raw

PATCH v4 22/35 fetch-pack: support shallow requests

In reply to Brandon Williams

Enable shallow clones and deepen requests using protocol version 2 if the server 'fetch' command supports the 'shallow' feature.

---
 connect.c    | 22 ++++++++++++++++
 connect.h    |  2 ++
 fetch-pack.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 94 insertions(+), 1 deletion(-)
connect.c +22 -0
index 6203ce576..66a9443c8 100644
@@ -82,6 +82,28 @@ int server_supports_v2(const char *c, int die_on_error)
 	return 0;
 }
 
+int server_supports_feature(const char *c, const char *feature,
+			    int die_on_error)
+{
+	int i;
+
+	for (i = 0; i < server_capabilities_v2.argc; i++) {
+		const char *out;
+		if (skip_prefix(server_capabilities_v2.argv[i], c, &out) &&
+		    (!*out || *(out++) == '=')) {
+			if (parse_feature_request(out, feature))
+				return 1;
+			else
+				break;
+		}
+	}
+
+	if (die_on_error)
+		die("server doesn't support feature '%s'", feature);
+
+	return 0;
+}
+
 static void process_capabilities_v2(struct packet_reader *reader)
 {
 	while (packet_reader_read(reader) == PACKET_READ_NORMAL)
connect.h +2 -0
index 8898d4495..0e69c6709 100644
@@ -17,5 +17,7 @@ struct packet_reader;
 extern enum protocol_version discover_version(struct packet_reader *reader);
 
 extern int server_supports_v2(const char *c, int die_on_error);
+extern int server_supports_feature(const char *c, const char *feature,
+				   int die_on_error);
 
 #endif
fetch-pack.c +70 -1

107 lines not on this page · raw message

Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#339 lore raw

PATCH v4 23/35 connect: refactor git_connect to only get the protocol version once

In reply to Brandon Williams

Instead of having each builtin transport asking for which protocol version the user has configured in 'protocol.version' by calling get_protocol_version_config() multiple times, factor this logic out so there is just a single call at the beginning of git_connect().

This will be helpful in the next patch where we can have centralized logic which determines if we need to request a different protocol version than what the user has configured.

---
 connect.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)
connect.c +15 -12
index 66a9443c8..a0bfcdf4f 100644
@@ -1035,6 +1035,7 @@ static enum ssh_variant determine_ssh_variant(const char *ssh_command,
  */
 static struct child_process *git_connect_git(int fd[2], char *hostandport,
 					     const char *path, const char *prog,
+					     enum protocol_version version,
 					     int flags)
 {
 	struct child_process *conn;
@@ -1073,10 +1074,10 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,
 		    target_host, 0);
 
 	/* If using a new version put that stuff here after a second null byte */
-	if (get_protocol_version_config() > 0) {
+	if (version > 0) {
 		strbuf_addch(&request, '\0');
 		strbuf_addf(&request, "version=%d%c",
-			    get_protocol_version_config(), '\0');
+			    version, '\0');
 	}
 
 	packet_write(fd[1], request.buf, request.len);
@@ -1092,14 +1093,14 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,
  */
 static void push_ssh_options(struct argv_array *args, struct argv_array *env,
 			     enum ssh_variant variant, const char *port,
-			     int flags)
+			     enum protocol_version version, int flags)
 {
 	if (variant == VARIANT_SSH &&
-	    get_protocol_version_config() > 0) {
+	    version > 0) {
 		argv_array_push(args, "-o");
 		argv_array_push(args, "SendEnv=" GIT_PROTOCOL_ENVIRONMENT);
 		argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
-				 get_protocol_version_config());
+				 version);
 	}
 
 	if (flags & CONNECT_IPV4) {
@@ -1152,7 +1153,8 @@ static void push_ssh_options(struct argv_array *args, struct argv_array *env,
 
 /* Prepare a child_process for use by Git's SSH-tunneled transport. */
 static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
-			  const char *port, int flags)
+			  const char *port, enum protocol_version version,
+			  int flags)
 {
 	const char *ssh;
 	enum ssh_variant variant;
@@ -1186,14 +1188,14 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
 		argv_array_push(&detect.args, ssh);
 		argv_array_push(&detect.args, "-G");
 		push_ssh_options(&detect.args, &detect.env_array,
-				 VARIANT_SSH, port, flags);
+				 VARIANT_SSH, port, version, flags);
 		argv_array_push(&detect.args, ssh_host);
 
 		variant = run_command(&detect) ? VARIANT_SIMPLE : VARIANT_SSH;
 	}
 
 	argv_array_push(&conn->args, ssh);
-	push_ssh_options(&conn->args, &conn->env_array, variant, port, flags);
+	push_ssh_options(&conn->args, &conn->env_array, variant, port, version, flags);
 	argv_array_push(&conn->args, ssh_host);
 }
 
@@ -1214,6 +1216,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 	char *hostandport, *path;
 	struct child_process *conn;
 	enum protocol protocol;
+	enum protocol_version version = get_protocol_version_config();
 
 	/* Without this we cannot rely on waitpid() to tell
 	 * what happened to our children.
@@ -1228,7 +1231,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 		printf("Diag: path=%s\n", path ? path : "NULL");
 		conn = NULL;
 	} else if (protocol == PROTO_GIT) {
-		conn = git_connect_git(fd, hostandport, path, prog, flags);
+		conn = git_connect_git(fd, hostandport, path, prog, version, flags);
 	} else {
 		struct strbuf cmd = STRBUF_INIT;
 		const char *const *var;
@@ -1271,12 +1274,12 @@ struct child_process *git_connect(int fd[2], const char *url,
 				strbuf_release(&cmd);
 				return NULL;
 			}
-			fill_ssh_args(conn, ssh_host, port, flags);
+			fill_ssh_args(conn, ssh_host, port, version, flags);
 		} else {
 			transport_check_allowed("file");
-			if (get_protocol_version_config() > 0) {
+			if (version > 0) {
 				argv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
-						 get_protocol_version_config());
+						 version);
 			}
 		}
 		argv_array_push(&conn->args, cmd.buf);
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#340 lore raw

PATCH v4 24/35 connect: don't request v2 when pushing

In reply to Brandon Williams

In order to be able to ship protocol v2 with only supporting fetch, we need clients to not issue a request to use protocol v2 when pushing (since the client currently doesn't know how to push using protocol v2). This allows a client to have protocol v2 configured in protocol.version and take advantage of using v2 for fetch and falling back to using v0 when pushing while v2 for push is being designed.

We could run into issues if we didn't fall back to protocol v2 when pushing right now. This is because currently a server will ignore a request to use v2 when contacting the 'receive-pack' endpoint and fall back to using v0, but when push v2 is rolled out to servers, the 'receive-pack' endpoint will start responding using v2. So we don't want to get into a state where a client is requesting to push with v2 before they actually know how to push using v2.

---
 connect.c              |  8 ++++++++
 t/t5702-protocol-v2.sh | 24 ++++++++++++++++++++++++
 2 files changed, 32 insertions(+)
connect.c +8 -0
index a0bfcdf4f..32284d050 100644
@@ -1218,6 +1218,14 @@ struct child_process *git_connect(int fd[2], const char *url,
 	enum protocol protocol;
 	enum protocol_version version = get_protocol_version_config();
 
+	/*
+	 * NEEDSWORK: If we are trying to use protocol v2 and we are planning
+	 * to perform a push, then fallback to v0 since the client doesn't know
+	 * how to push yet using v2.
+	 */
+	if (version == protocol_v2 && !strcmp("git-receive-pack", prog))
+		version = protocol_v0;
+
 	/* Without this we cannot rely on waitpid() to tell
 	 * what happened to our children.
 	 */
t/t5702-protocol-v2.sh +24 -0
index 4365ac273..e3a7c09d4 100755
@@ -95,6 +95,30 @@ test_expect_success 'pull with git:// using protocol v2' '
 	grep "fetch< version 2" log
 '
 
+test_expect_success 'push with git:// and a config of v2 does not request v2' '
+	test_when_finished "rm -f log" &&
+
+	# Till v2 for push is designed, make sure that if a client has
+	# protocol.version configured to use v2, that the client instead falls
+	# back and uses v0.
+
+	test_commit -C daemon_child three &&
+
+	# Push to another branch, as the target repository has the
+	# master branch checked out and we cannot push into it.
+	GIT_TRACE_PACKET="$(pwd)/log" git -C daemon_child -c protocol.version=2 \
+		push origin HEAD:client_branch &&
+
+	git -C daemon_child log -1 --format=%s >actual &&
+	git -C "$daemon_parent" log -1 --format=%s client_branch >expect &&
+	test_cmp expect actual &&
+
+	# Client requested to use protocol v2
+	! grep "push> .*\\\0\\\0version=2\\\0$" log &&
+	# Server responded using protocol v2
+	! grep "push< version 2" log
+'
+
 stop_git_daemon
 
 # Test protocol v2 with 'file://' transport
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#341 lore raw

PATCH v4 25/35 transport-helper: remove name parameter

In reply to Brandon Williams

Commit 266f1fdfa (transport-helper: be quiet on read errors from helpers, 2013-06-21) removed a call to 'die()' which printed the name of the remote helper passed in to the 'recvline_fh()' function using the 'name' parameter. Once the call to 'die()' was removed the parameter was no longer necessary but wasn't removed. Clean up 'recvline_fh()' parameter list by removing the 'name' parameter.

---
 transport-helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
transport-helper.c +3 -3
index 4c334b5ee..d72155768 100644
@@ -49,7 +49,7 @@ static void sendline(struct helper_data *helper, struct strbuf *buffer)
 		die_errno("Full write to remote helper failed");
 }
 
-static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)
+static int recvline_fh(FILE *helper, struct strbuf *buffer)
 {
 	strbuf_reset(buffer);
 	if (debug)
@@ -67,7 +67,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)
 
 static int recvline(struct helper_data *helper, struct strbuf *buffer)
 {
-	return recvline_fh(helper->out, buffer, helper->name);
+	return recvline_fh(helper->out, buffer);
 }
 
 static void write_constant(int fd, const char *str)
@@ -586,7 +586,7 @@ static int process_connect_service(struct transport *transport,
 		goto exit;
 
 	sendline(data, &cmdbuf);
-	if (recvline_fh(input, &cmdbuf, name))
+	if (recvline_fh(input, &cmdbuf))
 		exit(128);
 
 	if (!strcmp(cmdbuf.buf, "")) {
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#342 lore raw

PATCH v4 26/35 transport-helper: refactor process_connect_service

In reply to Brandon Williams

A future patch will need to take advantage of the logic which runs and processes the response of the connect command on a remote helper so factor out this logic from 'process_connect_service()' and place it into a helper function 'run_connect()'.

---
 transport-helper.c | 67 ++++++++++++++++++++++++++--------------------
 1 file changed, 38 insertions(+), 29 deletions(-)
transport-helper.c +38 -29
index d72155768..c032a2a87 100644
@@ -545,14 +545,13 @@ static int fetch_with_import(struct transport *transport,
 	return 0;
 }
 
-static int process_connect_service(struct transport *transport,
-				   const char *name, const char *exec)
+static int run_connect(struct transport *transport, struct strbuf *cmdbuf)
 {
 	struct helper_data *data = transport->data;
-	struct strbuf cmdbuf = STRBUF_INIT;
-	struct child_process *helper;
-	int r, duped, ret = 0;
+	int ret = 0;
+	int duped;
 	FILE *input;
+	struct child_process *helper;
 
 	helper = get_helper(transport);
 
@@ -568,44 +567,54 @@ static int process_connect_service(struct transport *transport,
 	input = xfdopen(duped, "r");
 	setvbuf(input, NULL, _IONBF, 0);
 
+	sendline(data, cmdbuf);
+	if (recvline_fh(input, cmdbuf))
+		exit(128);
+
+	if (!strcmp(cmdbuf->buf, "")) {
+		data->no_disconnect_req = 1;
+		if (debug)
+			fprintf(stderr, "Debug: Smart transport connection "
+				"ready.\n");
+		ret = 1;
+	} else if (!strcmp(cmdbuf->buf, "fallback")) {
+		if (debug)
+			fprintf(stderr, "Debug: Falling back to dumb "
+				"transport.\n");
+	} else {
+		die("Unknown response to connect: %s",
+			cmdbuf->buf);
+	}
+
+	fclose(input);
+	return ret;
+}
+
+static int process_connect_service(struct transport *transport,
+				   const char *name, const char *exec)
+{
+	struct helper_data *data = transport->data;
+	struct strbuf cmdbuf = STRBUF_INIT;
+	int ret = 0;
+
 	/*
 	 * Handle --upload-pack and friends. This is fire and forget...
 	 * just warn if it fails.
 	 */
 	if (strcmp(name, exec)) {
-		r = set_helper_option(transport, "servpath", exec);
+		int r = set_helper_option(transport, "servpath", exec);
 		if (r > 0)
 			warning("Setting remote service path not supported by protocol.");
 		else if (r < 0)
 			warning("Invalid remote service path.");
 	}
 
-	if (data->connect)
+	if (data->connect) {
 		strbuf_addf(&cmdbuf, "connect %s\n", name);
-	else
-		goto exit;
-
-	sendline(data, &cmdbuf);
-	if (recvline_fh(input, &cmdbuf))
-		exit(128);
-
-	if (!strcmp(cmdbuf.buf, "")) {
-		data->no_disconnect_req = 1;
-		if (debug)
-			fprintf(stderr, "Debug: Smart transport connection "
-				"ready.\n");
-		ret = 1;
-	} else if (!strcmp(cmdbuf.buf, "fallback")) {
-		if (debug)
-			fprintf(stderr, "Debug: Falling back to dumb "
-				"transport.\n");
-	} else
-		die("Unknown response to connect: %s",
-			cmdbuf.buf);
+		ret = run_connect(transport, &cmdbuf);
+	}
 
-exit:
 	strbuf_release(&cmdbuf);
-	fclose(input);
 	return ret;
 }
 
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#343 lore raw

PATCH v4 27/35 transport-helper: introduce stateless-connect

In reply to Brandon Williams

Introduce the transport-helper capability 'stateless-connect'. This capability indicates that the transport-helper can be requested to run the 'stateless-connect' command which should attempt to make a stateless connection with a remote end. Once established, the connection can be used by the git client to communicate with the remote end natively in a stateless-rpc manner as supported by protocol v2. This means that the client must send everything the server needs in a single request as the client must not assume any state-storing on the part of the server or transport.

If a stateless connection cannot be established then the remote-helper will respond in the same manner as the 'connect' command indicating that the client should fallback to using the dumb remote-helper commands.

A future patch will implement the 'stateless-connect' capability in our http remote-helper (remote-curl) so that protocol v2 can be used using the http transport.

---
 Documentation/gitremote-helpers.txt | 32 +++++++++++++++++++++++++++++
 transport-helper.c                  | 11 ++++++++++
 transport.c                         |  1 +
 transport.h                         |  6 ++++++
 4 files changed, 50 insertions(+)
Documentation/gitremote-helpers.txt +32 -0
index 4a584f3c5..a8361ed95 100644
@@ -102,6 +102,14 @@ Capabilities for Pushing
 +
 Supported commands: 'connect'.
 
+'stateless-connect'::
+	Experimental; for internal use only.
+	Can attempt to connect to a remote server for communication
+	using git's wire-protocol version 2.  This establishes a
+	stateless, half-duplex connection.
++
+Supported commands: 'stateless-connect'.
+
 'push'::
 	Can discover remote refs and push local commits and the
 	history leading up to them to new or existing remote refs.
@@ -136,6 +144,14 @@ Capabilities for Fetching
 +
 Supported commands: 'connect'.
 
+'stateless-connect'::
+	Experimental; for internal use only.
+	Can attempt to connect to a remote server for communication
+	using git's wire-protocol version 2.  This establishes a
+	stateless, half-duplex connection.
++
+Supported commands: 'stateless-connect'.
+
 'fetch'::
 	Can discover remote refs and transfer objects reachable from
 	them to the local object store.
@@ -375,6 +391,22 @@ Supported if the helper has the "export" capability.
 +
 Supported if the helper has the "connect" capability.
 
+'stateless-connect' <service>::
+	Experimental; for internal use only.
+	Connects to the given remote service for communication using
+	git's wire-protocol version 2.  This establishes a stateless,
+	half-duplex connection.  Valid replies to this command are empty
+	line (connection established), 'fallback' (no smart transport
+	support, fall back to dumb transports) and just exiting with
+	error message printed (can't connect, don't bother trying to
+	fall back).  After line feed terminating the positive (empty)
+	response, the output of the service starts.  Messages (both
+	request and response) must be terminated with a single flush
+	packet, allowing the remote helper to properly act as a proxy.
+	After the connection ends, the remote helper exits.
++
+Supported if the helper has the "stateless-connect" capability.
+
 If a fatal error occurs, the program writes the error message to
 stderr and exits. The caller should expect that a suitable error
 message has been printed if the child closes the connection without
transport-helper.c +11 -0
index c032a2a87..e20a5076e 100644
@@ -12,6 +12,7 @@
 #include "argv-array.h"
 #include "refs.h"
 #include "transport-internal.h"
+#include "protocol.h"
 
 static int debug;
 
@@ -26,6 +27,7 @@ struct helper_data {
 		option : 1,
 		push : 1,
 		connect : 1,
+		stateless_connect : 1,
 		signed_tags : 1,
 		check_connectivity : 1,
 		no_disconnect_req : 1,
@@ -188,6 +190,8 @@ static struct child_process *get_helper(struct transport *transport)
 			refspecs[refspec_nr++] = xstrdup(arg);
 		} else if (!strcmp(capname, "connect")) {
 			data->connect = 1;
+		} else if (!strcmp(capname, "stateless-connect")) {
+			data->stateless_connect = 1;
 		} else if (!strcmp(capname, "signed-tags")) {
 			data->signed_tags = 1;
 		} else if (skip_prefix(capname, "export-marks ", &arg)) {
@@ -612,6 +616,13 @@ static int process_connect_service(struct transport *transport,
 	if (data->connect) {
 		strbuf_addf(&cmdbuf, "connect %s\n", name);
 		ret = run_connect(transport, &cmdbuf);
+	} else if (data->stateless_connect &&
+		   (get_protocol_version_config() == protocol_v2) &&
+		   !strcmp("git-upload-pack", name)) {
+		strbuf_addf(&cmdbuf, "stateless-connect %s\n", name);
+		ret = run_connect(transport, &cmdbuf);
+		if (ret)
+			transport->stateless_rpc = 1;
 	}
 
 	strbuf_release(&cmdbuf);
transport.c +1 -0
index 8e38352c5..2e7b7a715 100644
@@ -250,6 +250,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 		data->options.check_self_contained_and_connected;
 	args.cloning = transport->cloning;
 	args.update_shallow = data->options.update_shallow;
+	args.stateless_rpc = transport->stateless_rpc;
 
 	if (!data->got_remote_heads)
 		refs_tmp = get_refs_via_connect(transport, 0, NULL);
transport.h +6 -0
index daea4770c..0ef0d1902 100644
@@ -55,6 +55,12 @@ struct transport {
 	 */
 	unsigned cloning : 1;
 
+	/*
+	 * Indicates that the transport is connected via a half-duplex
+	 * connection and should operate in stateless-rpc mode.
+	 */
+	unsigned stateless_rpc : 1;
+
 	/*
 	 * These strings will be passed to the {pre, post}-receive hook,
 	 * on the remote side, if both sides support the push options capability.
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#344 lore raw

Re: [PATCH v4 27/35] transport-helper: introduce stateless-connect

Quoted 12 lines · On Wed, 28 Feb 2018 15:22:44 -0800 Brandon Williams <[email protected]> wrote:

+'stateless-connect'::

Documentation/gitremote-helpers.txt +7 -0
+	Experimental; for internal use only.
+	Can attempt to connect to a remote server for communication
+	using git's wire-protocol version 2.  This establishes a
+	stateless, half-duplex connection.
++
+Supported commands: 'stateless-connect'.
+
 'push'::
 	Can discover remote refs and push local commits and the
 	history leading up to them to new or existing remote refs.

Documentation/gitremote-helpers.txt Capabilities for Fetching · line 153

Quoted 10 lines
Documentation/gitremote-helpers.txt +7 -0
@@ -136,6 +144,14 @@ Capabilities for Fetching
 +
 Supported commands: 'connect'.
 
+'stateless-connect'::
+	Experimental; for internal use only.
+	Can attempt to connect to a remote server for communication
+	using git's wire-protocol version 2.  This establishes a
+	stateless, half-duplex connection.
++
+Supported commands: 'stateless-connect'.

I don't think we should use the term "half-duplex" - from a search, it means that both parties can use the wire but not simultaneously, which is not strictly true. Might be better to just say "see the documentation for the stateless-connect command for more information".

Quoted 5 lines
Documentation/gitremote-helpers.txt +5 -0
+'stateless-connect' <service>::
+	Experimental; for internal use only.
+	Connects to the given remote service for communication using
+	git's wire-protocol version 2.  This establishes a stateless,
+	half-duplex connection.  Valid replies to this command are empty

Documentation/gitremote-helpers.txt Supported if the helper has the "export" capability. · line 408

Quoted 10 lines
Documentation/gitremote-helpers.txt +10 -0
+	line (connection established), 'fallback' (no smart transport
+	support, fall back to dumb transports) and just exiting with
+	error message printed (can't connect, don't bother trying to
+	fall back).  After line feed terminating the positive (empty)
+	response, the output of the service starts.  Messages (both
+	request and response) must be terminated with a single flush
+	packet, allowing the remote helper to properly act as a proxy.
+	After the connection ends, the remote helper exits.
++
+Supported if the helper has the "stateless-connect" capability.

I'm not sure of the relevance of "allowing the remote helper to properly act as a proxy" - this scheme does make it easier to implement proxies, not for any party to start acting as one instead. I would write that part as:

Messages (both request and response) must consist of zero or more PKT-LINEs, terminating in a flush packet. The client must not expect the server to store any state in between request-response pairs.

(This covers the so-called "half-duplex" part and the "stateless" part.)

#345 lore raw

Re: [PATCH v4 27/35] transport-helper: introduce stateless-connect

Quoted 52 lines · Jonathan Tan
Quoted 12 lines · On Wed, 28 Feb 2018 15:22:44 -0800 Brandon Williams <[email protected]> wrote:

+'stateless-connect'::

Documentation/gitremote-helpers.txt +7 -0
+	Experimental; for internal use only.
+	Can attempt to connect to a remote server for communication
+	using git's wire-protocol version 2.  This establishes a
+	stateless, half-duplex connection.
++
+Supported commands: 'stateless-connect'.
+
 'push'::
 	Can discover remote refs and push local commits and the
 	history leading up to them to new or existing remote refs.

Documentation/gitremote-helpers.txt Capabilities for Fetching · line 153

Quoted 10 lines
Documentation/gitremote-helpers.txt +7 -0
@@ -136,6 +144,14 @@ Capabilities for Fetching
 +
 Supported commands: 'connect'.
 
+'stateless-connect'::
+	Experimental; for internal use only.
+	Can attempt to connect to a remote server for communication
+	using git's wire-protocol version 2.  This establishes a
+	stateless, half-duplex connection.
++
+Supported commands: 'stateless-connect'.

I don't think we should use the term "half-duplex" - from a search, it means that both parties can use the wire but not simultaneously, which is not strictly true. Might be better to just say "see the documentation for the stateless-connect command for more information".

Quoted 5 lines
Documentation/gitremote-helpers.txt +5 -0
+'stateless-connect' <service>::
+	Experimental; for internal use only.
+	Connects to the given remote service for communication using
+	git's wire-protocol version 2.  This establishes a stateless,
+	half-duplex connection.  Valid replies to this command are empty

Documentation/gitremote-helpers.txt Supported if the helper has the "export" capability. · line 408

Quoted 10 lines
Documentation/gitremote-helpers.txt +10 -0
+	line (connection established), 'fallback' (no smart transport
+	support, fall back to dumb transports) and just exiting with
+	error message printed (can't connect, don't bother trying to
+	fall back).  After line feed terminating the positive (empty)
+	response, the output of the service starts.  Messages (both
+	request and response) must be terminated with a single flush
+	packet, allowing the remote helper to properly act as a proxy.
+	After the connection ends, the remote helper exits.
++
+Supported if the helper has the "stateless-connect" capability.

I'm not sure of the relevance of "allowing the remote helper to properly act as a proxy" - this scheme does make it easier to implement proxies, not for any party to start acting as one instead. I would write that part as:

Quoted 5 lines

Messages (both request and response) must consist of zero or more PKT-LINEs, terminating in a flush packet. The client must not expect the server to store any state in between request-response pairs.

(This covers the so-called "half-duplex" part and the "stateless" part.)

Thanks for helping wordsmith this, I'll update the docs based on these suggestions.

Signature · 1 line
Brandon Williams
#346 lore raw

PATCH v4 28/35 pkt-line: add packet_buf_write_len function

In reply to Brandon Williams

Add the 'packet_buf_write_len()' function which allows for writing an arbitrary length buffer into a 'struct strbuf' and formatting it in packet-line format.

---
 pkt-line.c | 16 ++++++++++++++++
 pkt-line.h |  1 +
 2 files changed, 17 insertions(+)
pkt-line.c +16 -0
index 87a24bd17..5223e24e2 100644
@@ -215,6 +215,22 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
 	va_end(args);
 }
 
+void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len)
+{
+	size_t orig_len, n;
+
+	orig_len = buf->len;
+	strbuf_addstr(buf, "0000");
+	strbuf_add(buf, data, len);
+	n = buf->len - orig_len;
+
+	if (n > LARGE_PACKET_MAX)
+		die("protocol error: impossibly long line");
+
+	set_packet_header(&buf->buf[orig_len], n);
+	packet_trace(data, len, 1);
+}
+
 int write_packetized_from_fd(int fd_in, int fd_out)
 {
 	static char buf[LARGE_PACKET_DATA_MAX];
pkt-line.h +1 -0
index 3f836f01a..4f97ae3e5 100644
@@ -26,6 +26,7 @@ void packet_buf_flush(struct strbuf *buf);
 void packet_buf_delim(struct strbuf *buf);
 void packet_write(int fd_out, const char *buf, size_t size);
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
+void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len);
 int packet_flush_gently(int fd);
 int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
 int write_packetized_from_fd(int fd_in, int fd_out);
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#347 lore raw

PATCH v4 29/35 remote-curl: create copy of the service name

In reply to Brandon Williams

Make a copy of the service name being requested instead of relying on the buffer pointed to by the passed in 'const char *' to remain unchanged.

Currently, all service names are string constants, but a subsequent patch will introduce service names from external sources.

---
 remote-curl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
remote-curl.c +3 -2
index dae8a4a48..4086aa733 100644
@@ -165,7 +165,7 @@ static int set_option(const char *name, const char *value)
 }
 
 struct discovery {
-	const char *service;
+	char *service;
 	char *buf_alloc;
 	char *buf;
 	size_t len;
@@ -257,6 +257,7 @@ static void free_discovery(struct discovery *d)
 		free(d->shallow.oid);
 		free(d->buf_alloc);
 		free_refs(d->refs);
+		free(d->service);
 		free(d);
 	}
 }
@@ -343,7 +344,7 @@ static struct discovery *discover_refs(const char *service, int for_push)
 		warning(_("redirecting to %s"), url.buf);
 
 	last= xcalloc(1, sizeof(*last_discovery));
-	last->service = service;
+	last->service = xstrdup(service);
 	last->buf_alloc = strbuf_detach(&buffer, &last->len);
 	last->buf = last->buf_alloc;
 
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
Reviewed-by on v4 29/35
#348 lore raw

Re: [PATCH v4 29/35] remote-curl: create copy of the service name

Quoted 8 lines · On Wed, 28 Feb 2018 15:22:46 -0800 Brandon Williams <[email protected]> wrote:

Make a copy of the service name being requested instead of relying on the buffer pointed to by the passed in 'const char *' to remain unchanged.

Currently, all service names are string constants, but a subsequent patch will introduce service names from external sources.

Once again,

#349 lore raw

PATCH v4 30/35 remote-curl: store the protocol version the server responded with

In reply to Brandon Williams

Store the protocol version the server responded with when performing discovery. This will be used in a future patch to either change the 'Git-Protocol' header sent in subsequent requests or to determine if a client needs to fallback to using a different protocol version.

---
 remote-curl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
remote-curl.c +3 -1
index 4086aa733..c54035843 100644
@@ -171,6 +171,7 @@ struct discovery {
 	size_t len;
 	struct ref *refs;
 	struct oid_array shallow;
+	enum protocol_version version;
 	unsigned proto_git : 1;
 };
 static struct discovery *last_discovery;
@@ -184,7 +185,8 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)
 			   PACKET_READ_CHOMP_NEWLINE |
 			   PACKET_READ_GENTLE_ON_EOF);
 
-	switch (discover_version(&reader)) {
+	heads->version = discover_version(&reader);
+	switch (heads->version) {
 	case protocol_v2:
 		die("support for protocol v2 not implemented yet");
 		break;
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#350 lore raw

PATCH v4 31/35 http: allow providing extra headers for http requests

In reply to Brandon Williams

Add a way for callers to request that extra headers be included when making http requests.

---
 http.c | 8 ++++++++
 http.h | 7 +++++++
 2 files changed, 15 insertions(+)
http.c +8 -0
index 597771271..e1757d62b 100644
@@ -1723,6 +1723,14 @@ static int http_request(const char *url,
 
 	headers = curl_slist_append(headers, buf.buf);
 
+	/* Add additional headers here */
+	if (options && options->extra_headers) {
+		const struct string_list_item *item;
+		for_each_string_list_item(item, options->extra_headers) {
+			headers = curl_slist_append(headers, item->string);
+		}
+	}
+
 	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
http.h +7 -0
index f7bd3b26b..4df4a25e1 100644
@@ -172,6 +172,13 @@ struct http_get_options {
 	 * for details.
 	 */
 	struct strbuf *base_url;
+
+	/*
+	 * If not NULL, contains additional HTTP headers to be sent with the
+	 * request. The strings in the list must not be freed until after the
+	 * request has completed.
+	 */
+	struct string_list *extra_headers;
 };
 
 /* Return values for http_get_*() */
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#352 lore raw

PATCH v4 32/35 http: don't always add Git-Protocol header

In reply to Brandon Williams

Instead of always sending the Git-Protocol header with the configured version with every http request, explicitly send it when discovering refs and then only send it on subsequent http requests if the server understood the version requested.

---
 http.c        | 17 -----------------
 remote-curl.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 17 deletions(-)
http.c +0 -17
index e1757d62b..8f1129ac7 100644
@@ -904,21 +904,6 @@ static void set_from_env(const char **var, const char *envname)
 		*var = val;
 }
 
-static void protocol_http_header(void)
-{
-	if (get_protocol_version_config() > 0) {
-		struct strbuf protocol_header = STRBUF_INIT;
-
-		strbuf_addf(&protocol_header, GIT_PROTOCOL_HEADER ": version=%d",
-			    get_protocol_version_config());
-
-
-		extra_http_headers = curl_slist_append(extra_http_headers,
-						       protocol_header.buf);
-		strbuf_release(&protocol_header);
-	}
-}
-
 void http_init(struct remote *remote, const char *url, int proactive_auth)
 {
 	char *low_speed_limit;
@@ -949,8 +934,6 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 	if (remote)
 		var_override(&http_proxy_authmethod, remote->http_proxy_authmethod);
 
-	protocol_http_header();
-
 	pragma_header = curl_slist_append(http_copy_default_headers(),
 		"Pragma: no-cache");
 	no_pragma_header = curl_slist_append(http_copy_default_headers(),
remote-curl.c +33 -0
index c54035843..b4e9db85b 100644
@@ -291,6 +291,19 @@ static int show_http_message(struct strbuf *type, struct strbuf *charset,
 	return 0;
 }
 
+static int get_protocol_http_header(enum protocol_version version,
+				    struct strbuf *header)
+{
+	if (version > 0) {
+		strbuf_addf(header, GIT_PROTOCOL_HEADER ": version=%d",
+			    version);
+
+		return 1;
+	}
+
+	return 0;
+}
+
 static struct discovery *discover_refs(const char *service, int for_push)
 {
 	struct strbuf exp = STRBUF_INIT;
@@ -299,6 +312,8 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	struct strbuf buffer = STRBUF_INIT;
 	struct strbuf refs_url = STRBUF_INIT;
 	struct strbuf effective_url = STRBUF_INIT;
+	struct strbuf protocol_header = STRBUF_INIT;
+	struct string_list extra_headers = STRING_LIST_INIT_DUP;
 	struct discovery *last = last_discovery;
 	int http_ret, maybe_smart = 0;
 	struct http_get_options http_options;
@@ -318,11 +333,16 @@ static struct discovery *discover_refs(const char *service, int for_push)
 		strbuf_addf(&refs_url, "service=%s", service);
 	}
 
+	/* Add the extra Git-Protocol header */
+	if (get_protocol_http_header(get_protocol_version_config(), &protocol_header))
+		string_list_append(&extra_headers, protocol_header.buf);
+
 	memset(&http_options, 0, sizeof(http_options));
 	http_options.content_type = &type;
 	http_options.charset = &charset;
 	http_options.effective_url = &effective_url;
 	http_options.base_url = &url;
+	http_options.extra_headers = &extra_headers;
 	http_options.initial_request = 1;
 	http_options.no_cache = 1;
 	http_options.keep_error = 1;
@@ -389,6 +409,8 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	strbuf_release(&charset);
 	strbuf_release(&effective_url);
 	strbuf_release(&buffer);
+	strbuf_release(&protocol_header);
+	string_list_clear(&extra_headers, 0);
 	last_discovery = last;
 	return last;
 }
@@ -425,6 +447,7 @@ struct rpc_state {
 	char *service_url;
 	char *hdr_content_type;
 	char *hdr_accept;
+	char *protocol_header;
 	char *buf;
 	size_t alloc;
 	size_t len;
@@ -611,6 +634,10 @@ static int post_rpc(struct rpc_state *rpc)
 	headers = curl_slist_append(headers, needs_100_continue ?
 		"Expect: 100-continue" : "Expect:");
 
+	/* Add the extra Git-Protocol header */
+	if (rpc->protocol_header)
+		headers = curl_slist_append(headers, rpc->protocol_header);
+
 retry:
 	slot = get_active_slot();
 
@@ -751,6 +778,11 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)
 	strbuf_addf(&buf, "Accept: application/x-%s-result", svc);
 	rpc->hdr_accept = strbuf_detach(&buf, NULL);
 
+	if (get_protocol_http_header(heads->version, &buf))
+		rpc->protocol_header = strbuf_detach(&buf, NULL);
+	else
+		rpc->protocol_header = NULL;
+
 	while (!err) {
 		int n = packet_read(rpc->out, NULL, NULL, rpc->buf, rpc->alloc, 0);
 		if (!n)
@@ -778,6 +810,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)
 	free(rpc->service_url);
 	free(rpc->hdr_content_type);
 	free(rpc->hdr_accept);
+	free(rpc->protocol_header);
 	free(rpc->buf);
 	strbuf_release(&buf);
 	return err;
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#353 lore raw

PATCH v4 33/35 http: eliminate "# service" line when using protocol v2

In reply to Brandon Williams

When an http info/refs request is made, requesting that protocol v2 be used, don't send a "# service" line since this line is not part of the v2 spec.

---
 http-backend.c | 8 ++++++--
 remote-curl.c  | 3 +++
 2 files changed, 9 insertions(+), 2 deletions(-)
http-backend.c +6 -2
index f3dc218b2..5d241e910 100644
@@ -10,6 +10,7 @@
 #include "url.h"
 #include "argv-array.h"
 #include "packfile.h"
+#include "protocol.h"
 
 static const char content_type[] = "Content-Type";
 static const char content_length[] = "Content-Length";
@@ -466,8 +467,11 @@ static void get_info_refs(struct strbuf *hdr, char *arg)
 		hdr_str(hdr, content_type, buf.buf);
 		end_headers(hdr);
 
-		packet_write_fmt(1, "# service=git-%s\n", svc->name);
-		packet_flush(1);
+
+		if (determine_protocol_version_server() != protocol_v2) {
+			packet_write_fmt(1, "# service=git-%s\n", svc->name);
+			packet_flush(1);
+		}
 
 		argv[0] = svc->name;
 		run_service(argv, 0);
remote-curl.c +3 -0
index b4e9db85b..66a53f74b 100644
@@ -396,6 +396,9 @@ static struct discovery *discover_refs(const char *service, int for_push)
 			;
 
 		last->proto_git = 1;
+	} else if (maybe_smart &&
+		   last->len > 5 && starts_with(last->buf + 4, "version 2")) {
+		last->proto_git = 1;
 	}
 
 	if (last->proto_git)
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#354 lore raw

PATCH v4 34/35 remote-curl: implement stateless-connect command

In reply to Brandon Williams

Teach remote-curl the 'stateless-connect' command which is used to establish a stateless connection with servers which support protocol version 2. This allows remote-curl to act as a proxy, allowing the git client to communicate natively with a remote end, simply using remote-curl as a pass through to convert requests to http.

---
 remote-curl.c          | 205 ++++++++++++++++++++++++++++++++++++++++-
 t/t5702-protocol-v2.sh |  45 +++++++++
 2 files changed, 249 insertions(+), 1 deletion(-)
remote-curl.c +204 -1
index 66a53f74b..3f882d766 100644
@@ -188,7 +188,12 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)
 	heads->version = discover_version(&reader);
 	switch (heads->version) {
 	case protocol_v2:
-		die("support for protocol v2 not implemented yet");
+		/*
+		 * Do nothing.  This isn't a list of refs but rather a
+		 * capability advertisement.  Client would have run
+		 * 'stateless-connect' so we'll dump this capability listing
+		 * and let them request the refs themselves.
+		 */
 		break;
 	case protocol_v1:
 	case protocol_v0:
@@ -1085,6 +1090,200 @@ static void parse_push(struct strbuf *buf)
 	free(specs);
 }
 
+/*
+ * Used to represent the state of a connection to an HTTP server when
+ * communicating using git's wire-protocol version 2.
+ */
+struct proxy_state {
+	char *service_name;
+	char *service_url;
+	struct curl_slist *headers;
+	struct strbuf request_buffer;
+	int in;
+	int out;
+	struct packet_reader reader;
+	size_t pos;
+	int seen_flush;
+};
+
+static void proxy_state_init(struct proxy_state *p, const char *service_name,
+			     enum protocol_version version)
+{
+	struct strbuf buf = STRBUF_INIT;
+
+	memset(p, 0, sizeof(*p));
+	p->service_name = xstrdup(service_name);
+
+	p->in = 0;
+	p->out = 1;
+	strbuf_init(&p->request_buffer, 0);
+
+	strbuf_addf(&buf, "%s%s", url.buf, p->service_name);
+	p->service_url = strbuf_detach(&buf, NULL);
+
+	p->headers = http_copy_default_headers();
+
+	strbuf_addf(&buf, "Content-Type: application/x-%s-request", p->service_name);
+	p->headers = curl_slist_append(p->headers, buf.buf);
+	strbuf_reset(&buf);
+
+	strbuf_addf(&buf, "Accept: application/x-%s-result", p->service_name);
+	p->headers = curl_slist_append(p->headers, buf.buf);
+	strbuf_reset(&buf);
+
+	p->headers = curl_slist_append(p->headers, "Transfer-Encoding: chunked");
+
+	/* Add the Git-Protocol header */
+	if (get_protocol_http_header(version, &buf))
+		p->headers = curl_slist_append(p->headers, buf.buf);
+
+	packet_reader_init(&p->reader, p->in, NULL, 0,
+			   PACKET_READ_GENTLE_ON_EOF);
+
+	strbuf_release(&buf);
+}
+
+static void proxy_state_clear(struct proxy_state *p)
+{
+	free(p->service_name);
+	free(p->service_url);
+	curl_slist_free_all(p->headers);
+	strbuf_release(&p->request_buffer);
+}
+
+/*
+ * CURLOPT_READFUNCTION callback function.
+ * Attempts to copy over a single packet-line at a time into the
+ * curl provided buffer.
+ */
+static size_t proxy_in(char *buffer, size_t eltsize,
+		       size_t nmemb, void *userdata)
+{
+	size_t max;
+	struct proxy_state *p = userdata;
+	size_t avail = p->request_buffer.len - p->pos;
+
+
+	if (eltsize != 1)
+		BUG("curl read callback called with size = %zu != 1", eltsize);
+	max = nmemb;
+
+	if (!avail) {
+		if (p->seen_flush) {
+			p->seen_flush = 0;
+			return 0;
+		}
+
+		strbuf_reset(&p->request_buffer);
+		switch (packet_reader_read(&p->reader)) {
+		case PACKET_READ_EOF:
+			die("unexpected EOF when reading from parent process");
+		case PACKET_READ_NORMAL:
+			packet_buf_write_len(&p->request_buffer, p->reader.line,
+					     p->reader.pktlen);
+			break;
+		case PACKET_READ_DELIM:
+			packet_buf_delim(&p->request_buffer);
+			break;
+		case PACKET_READ_FLUSH:
+			packet_buf_flush(&p->request_buffer);
+			p->seen_flush = 1;
+			break;
+		}
+		p->pos = 0;
+		avail = p->request_buffer.len;
+	}
+
+	if (max < avail)
+		avail = max;
+	memcpy(buffer, p->request_buffer.buf + p->pos, avail);
+	p->pos += avail;
+	return avail;
+}
+
+static size_t proxy_out(char *buffer, size_t eltsize,
+			size_t nmemb, void *userdata)
+{
+	size_t size;
+	struct proxy_state *p = userdata;
+
+	if (eltsize != 1)
+		BUG("curl read callback called with size = %zu != 1", eltsize);
+	size = nmemb;
+
+	write_or_die(p->out, buffer, size);
+	return size;
+}
+
+/* Issues a request to the HTTP server configured in `p` */
+static int proxy_request(struct proxy_state *p)
+{
+	struct active_request_slot *slot;
+
+	slot = get_active_slot();
+
+	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
+	curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
+	curl_easy_setopt(slot->curl, CURLOPT_URL, p->service_url);
+	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, p->headers);
+
+	/* Setup function to read request from client */
+	curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, proxy_in);
+	curl_easy_setopt(slot->curl, CURLOPT_READDATA, p);
+
+	/* Setup function to write server response to client */
+	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, proxy_out);
+	curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, p);
+
+	if (run_slot(slot, NULL) != HTTP_OK)
+		return -1;
+
+	return 0;
+}
+
+static int stateless_connect(const char *service_name)
+{
+	struct discovery *discover;
+	struct proxy_state p;
+
+	/*
+	 * Run the info/refs request and see if the server supports protocol
+	 * v2.  If and only if the server supports v2 can we successfully
+	 * establish a stateless connection, otherwise we need to tell the
+	 * client to fallback to using other transport helper functions to
+	 * complete their request.
+	 */
+	discover = discover_refs(service_name, 0);
+	if (discover->version != protocol_v2) {
+		printf("fallback\n");
+		fflush(stdout);
+		return -1;
+	} else {
+		/* Stateless Connection established */
+		printf("\n");
+		fflush(stdout);
+	}
+
+	proxy_state_init(&p, service_name, discover->version);
+
+	/*
+	 * Dump the capability listing that we got from the server earlier
+	 * during the info/refs request.
+	 */
+	write_or_die(p.out, discover->buf, discover->len);
+
+	/* Peek the next packet line.  Until we see EOF keep sending POSTs */
+	while (packet_reader_peek(&p.reader) != PACKET_READ_EOF) {
+		if (proxy_request(&p)) {
+			/* We would have an err here */
+			break;
+		}
+	}
+
+	proxy_state_clear(&p);
+	return 0;
+}
+
 int cmd_main(int argc, const char **argv)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -1153,12 +1352,16 @@ int cmd_main(int argc, const char **argv)
 			fflush(stdout);
 
 		} else if (!strcmp(buf.buf, "capabilities")) {
+			printf("stateless-connect\n");
 			printf("fetch\n");
 			printf("option\n");
 			printf("push\n");
 			printf("check-connectivity\n");
 			printf("\n");
 			fflush(stdout);
+		} else if (skip_prefix(buf.buf, "stateless-connect ", &arg)) {
+			if (!stateless_connect(arg))
+				break;
 		} else {
 			error("remote-curl: unknown command '%s' from git", buf.buf);
 			return 1;
t/t5702-protocol-v2.sh +45 -0

49 lines not on this page · raw message

Signature · 1 line
2.16.2.395.g2e18187dfd-goog
#355 lore raw

Re: [PATCH v4 34/35] remote-curl: implement stateless-connect command

Hi Brandon,

Quoted 6 lines · Brandon Williams
remote-curl.c +0 -0
index 66a53f74b..3f882d766 100644
@@ -188,7 +188,12 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)
[...]

remote-curl.c static struct ref *parse_git_refs(struct discovery *heads, int for_push) · line 1168

Quoted 10 lines
remote-curl.c +10 -0
+static size_t proxy_in(char *buffer, size_t eltsize,
+		       size_t nmemb, void *userdata)
+{
+	size_t max;
+	struct proxy_state *p = userdata;
+	size_t avail = p->request_buffer.len - p->pos;
+
+
+	if (eltsize != 1)
+		BUG("curl read callback called with size = %zu != 1", eltsize);

The format specified %z is not actually portable. Please use PRIuMAX and cast to (uintmax_t) instead.

This breaks the Windows build of pu (before that, there was still a test failure that I did not have the time to chase down).

Ciao,
Dscho

#356 lore raw

Re: [PATCH v4 34/35] remote-curl: implement stateless-connect command

Quoted 24 lines · Johannes Schindelin

Hi Brandon,

Quoted 6 lines · Brandon Williams
remote-curl.c +0 -0
index 66a53f74b..3f882d766 100644
@@ -188,7 +188,12 @@ static struct ref *parse_git_refs(struct discovery *heads, int for_push)
[...]

remote-curl.c static struct ref *parse_git_refs(struct discovery *heads, int for_push) · line 1168

Quoted 10 lines
remote-curl.c +10 -0
+static size_t proxy_in(char *buffer, size_t eltsize,
+		       size_t nmemb, void *userdata)
+{
+	size_t max;
+	struct proxy_state *p = userdata;
+	size_t avail = p->request_buffer.len - p->pos;
+
+
+	if (eltsize != 1)
+		BUG("curl read callback called with size = %zu != 1", eltsize);

The format specified %z is not actually portable. Please use PRIuMAX and cast to (uintmax_t) instead.

Quoted 2 lines

This breaks the Windows build of pu (before that, there was still a test failure that I did not have the time to chase down).

Oh sorry, Looks like Junio put a patch ontop in pu to fix this. I'll squash that fix into this patch.

Thanks for catching this.

Quoted 3 lines

Ciao,
Dscho

Signature · 1 line
Brandon Williams
#357 lore raw

PATCH v4 35/35 remote-curl: don't request v2 when pushing

In reply to Brandon Williams

In order to be able to ship protocol v2 with only supporting fetch, we need clients to not issue a request to use protocol v2 when pushing (since the client currently doesn't know how to push using protocol v2). This allows a client to have protocol v2 configured in protocol.version and take advantage of using v2 for fetch and falling back to using v0 when pushing while v2 for push is being designed.

We could run into issues if we didn't fall back to protocol v2 when pushing right now. This is because currently a server will ignore a request to use v2 when contacting the 'receive-pack' endpoint and fall back to using v0, but when push v2 is rolled out to servers, the 'receive-pack' endpoint will start responding using v2. So we don't want to get into a state where a client is requesting to push with v2 before they actually know how to push using v2.

---
 remote-curl.c          | 11 ++++++++++-
 t/t5702-protocol-v2.sh | 24 ++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)
remote-curl.c +10 -1
index 3f882d766..379ab9b21 100644
@@ -322,6 +322,7 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	struct discovery *last = last_discovery;
 	int http_ret, maybe_smart = 0;
 	struct http_get_options http_options;
+	enum protocol_version version = get_protocol_version_config();
 
 	if (last && !strcmp(service, last->service))
 		return last;
@@ -338,8 +339,16 @@ static struct discovery *discover_refs(const char *service, int for_push)
 		strbuf_addf(&refs_url, "service=%s", service);
 	}
 
+	/*
+	 * NEEDSWORK: If we are trying to use protocol v2 and we are planning
+	 * to perform a push, then fallback to v0 since the client doesn't know
+	 * how to push yet using v2.
+	 */
+	if (version == protocol_v2 && !strcmp("git-receive-pack", service))
+		version = protocol_v0;
+
 	/* Add the extra Git-Protocol header */
-	if (get_protocol_http_header(get_protocol_version_config(), &protocol_header))
+	if (get_protocol_http_header(version, &protocol_header))
 		string_list_append(&extra_headers, protocol_header.buf);
 
 	memset(&http_options, 0, sizeof(http_options));
t/t5702-protocol-v2.sh +24 -0
index 124063c2c..56f7c3c32 100755
@@ -244,6 +244,30 @@ test_expect_success 'fetch with http:// using protocol v2' '
 	grep "git< version 2" log
 '
 
+test_expect_success 'push with http:// and a config of v2 does not request v2' '
+	test_when_finished "rm -f log" &&
+	# Till v2 for push is designed, make sure that if a client has
+	# protocol.version configured to use v2, that the client instead falls
+	# back and uses v0.
+
+	test_commit -C http_child three &&
+
+	# Push to another branch, as the target repository has the
+	# master branch checked out and we cannot push into it.
+	GIT_TRACE_PACKET="$(pwd)/log" git -C http_child -c protocol.version=2 \
+		push origin HEAD:client_branch &&
+
+	git -C http_child log -1 --format=%s >actual &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" log -1 --format=%s client_branch >expect &&
+	test_cmp expect actual &&
+
+	# Client didnt request to use protocol v2
+	! grep "Git-Protocol: version=2" log &&
+	# Server didnt respond using protocol v2
+	! grep "git< version 2" log
+'
+
+
 stop_httpd
 
 test_done
Signature · 1 line
2.16.2.395.g2e18187dfd-goog
Reviewed-by on v4 35/35
#358 lore raw

Re: [PATCH v4 35/35] remote-curl: don't request v2 when pushing

Quoted 15 lines · On Wed, 28 Feb 2018 15:22:52 -0800 Brandon Williams <[email protected]> wrote:

In order to be able to ship protocol v2 with only supporting fetch, we need clients to not issue a request to use protocol v2 when pushing (since the client currently doesn't know how to push using protocol v2). This allows a client to have protocol v2 configured in protocol.version and take advantage of using v2 for fetch and falling back to using v0 when pushing while v2 for push is being designed.

We could run into issues if we didn't fall back to protocol v2 when pushing right now. This is because currently a server will ignore a request to use v2 when contacting the 'receive-pack' endpoint and fall back to using v0, but when push v2 is rolled out to servers, the 'receive-pack' endpoint will start responding using v2. So we don't want to get into a state where a client is requesting to push with v2 before they actually know how to push using v2.

Quoted 1 line

I noticed that my review comments have been addressed, so:

#359 lore raw
In reply to Brandon Williams
Quoted 4 lines · Brandon Williams

Lots of changes since v3 (well more than between v2 and v3). Thanks for all of the reviews on the last round, the series is getting more polished.

Quoted 13 lines

* Eliminated the "# service" line from the response from an HTTP server. This means that the response to a v2 request is exactly the same regardless of which transport you use! Docs for this have been added as well.
* Changed how ref-patterns work with the ls-refs command. Instead of using wildmatch all patterns must either match exactly or they can contain a single '*' character at the end to mean that the prefix must match. Docs for this have also been added.
* Lots of updates to the docs. Including documenting the stateless-connect remote-helper command used by remote-curl to handle the http transport.
* Fixed a number of bugs with the fetch command, one of which didn't use objects from configured alternates.

I noticed that this round is built on top of v2.16.0-rc0. It certainly makes it easier to compare against the previous round which was built on top of that old commit and it is very much appreciated that a reroll does not involve pointless rebases.

For those who are helping from sidelines, it may be ehlpful to mention where in the history this was developed on, though, as applying these on the current 'master' has a handful of small conflicts.

Thanks, will replace and will comment on individual patches as needed.

#360 lore raw
Quoted 25 lines · Junio C Hamano
Quoted 4 lines · Brandon Williams

Lots of changes since v3 (well more than between v2 and v3). Thanks for all of the reviews on the last round, the series is getting more polished.

Quoted 13 lines

* Eliminated the "# service" line from the response from an HTTP server. This means that the response to a v2 request is exactly the same regardless of which transport you use! Docs for this have been added as well.
* Changed how ref-patterns work with the ls-refs command. Instead of using wildmatch all patterns must either match exactly or they can contain a single '*' character at the end to mean that the prefix must match. Docs for this have also been added.
* Lots of updates to the docs. Including documenting the stateless-connect remote-helper command used by remote-curl to handle the http transport.
* Fixed a number of bugs with the fetch command, one of which didn't use objects from configured alternates.

I noticed that this round is built on top of v2.16.0-rc0. It certainly makes it easier to compare against the previous round which was built on top of that old commit and it is very much appreciated that a reroll does not involve pointless rebases.

Quoted 7 lines

For those who are helping from sidelines, it may be ehlpful to mention where in the history this was developed on, though, as applying these on the current 'master' has a handful of small conflicts.

Thanks, will replace and will comment on individual patches as needed.

I've tried to keep building on the same base that I started with when sending out a new version of series, mostly because I thought it was easier to see what was different between rounds.

I can, in the future, try to remember to put the commit its based on. Do we have any sort of guidance about the best practice here?

Signature · 1 line
Brandon Williams
#361 lore raw
Quoted 3 lines · Brandon Williams

I've tried to keep building on the same base that I started with when sending out a new version of series, mostly because I thought it was easier to see what was different between rounds.

Yes. It indeed is easier to see the evolution if the series does not get rebased needlessly.

Quoted 2 lines

I can, in the future, try to remember to put the commit its based on. Do we have any sort of guidance about the best practice here?

I recall we taught a new "--base" option to "format-patch" not too long ago, so one way to do so may be:

    $ git format-patch --cover-letter --base=v2.16.0-rc0 master..bw/protocol-v2
    $ tail -4 0000-cover*.txt
    base-commit: 1eaabe34fc6f486367a176207420378f587d3b48
    --
    2.16.2-345-g7e31236f65

perhaps?