[RFC PATCH v3 00/17] Return of smart HTTP

[RFC PATCH v3 00/17] Return of smart HTTP

[Shawn O. Pearce]

This v3 series is a respin, and includes fixes to address reviewer
comments from v2.  The protocol specification is still omitted
from the series as I have not yet had a chance to merge reviewer
comments into the draft.

Major changes from v2:

  upload-pack:
  * multi_ack_2 renamed multi_ack_detailed

  helper:
  * options are now global
  * fetch is now a batch command (instead of fetch-multiple)
  * ftp:// no longer tries info/refs?service=git-upload-pack

  http-backend:
  * manual page
  * alternates files are now served as-is
  * uses proper QUERY_STRING parsing
  * GIT_COMMITTER_EMAIL set to REMOTE_USER, for receive-pack reflog
  * receive-pack defaults to *ON* if REMOTE_USER is set

This series is still lacking:

  * The HTTP protocol documentation
  * Tests for the smart http transport code (existing tests pass)

I don't want to rush reviews, but I'm fast running of time to work on
this series right now.  Next week I am offline for vacation... and
then its time for GitTogether '09.  I plan to work on tests for
this series and the HTTP spec document draft, but that's it until
after GitTogether '09.


These commits are also available at:

  git://repo.or.cz/git/fastimport.git v3-smart-http


Shawn O. Pearce (17):
  pkt-line: Add strbuf based functions
  pkt-line: Make packet_read_line easier to debug
  fetch-pack: Use a strbuf to compose the want list
  Move "get_ack()" back to fetch-pack
  Add multi_ack_detailed capability to fetch-pack/upload-pack
  remote-curl: Refactor walker initialization
  fetch: Allow transport -v -v -v to set verbosity to 3
  remote-helpers: Fetch more than one ref in a batch
  remote-helpers: Support custom transport options
  Move WebDAV HTTP push under remote-curl
  Git-aware CGI to provide dumb HTTP transport
  Add stateless RPC options to upload-pack, receive-pack
  Smart fetch and push over HTTP: server side
  Discover refs via smart HTTP server when available
  Smart push over HTTP: client side
  Smart fetch over HTTP: client side
  Smart HTTP fetch: gzip requests

 .gitignore                           |    1 +
 Documentation/config.txt             |    8 +
 Documentation/git-http-backend.txt   |  142 +++++++
 Documentation/git-remote-helpers.txt |   85 ++++-
 Makefile                             |    1 +
 builtin-fetch-pack.c                 |  210 ++++++++--
 builtin-fetch.c                      |    2 +-
 builtin-receive-pack.c               |   26 +-
 builtin-send-pack.c                  |  116 +++++-
 cache.h                              |    1 -
 commit.c                             |   10 +-
 commit.h                             |    2 +-
 connect.c                            |   21 -
 fetch-pack.h                         |    3 +-
 http-backend.c                       |  603 +++++++++++++++++++++++++++
 http-push.c                          |   43 ++-
 http.c                               |    9 +
 http.h                               |    1 +
 pkt-line.c                           |   86 ++++-
 pkt-line.h                           |    4 +
 remote-curl.c                        |  758 ++++++++++++++++++++++++++++++++--
 send-pack.h                          |    3 +-
 sideband.c                           |   11 +-
 transport-helper.c                   |  262 +++++++++++-
 transport.c                          |   32 +--
 transport.h                          |    2 +-
 upload-pack.c                        |   71 +++-
 27 files changed, 2296 insertions(+), 217 deletions(-)
 create mode 100644 Documentation/git-http-backend.txt
 create mode 100644 http-backend.c

[RFC PATCH v3 01/17] pkt-line: Add strbuf based functions

[Shawn O. Pearce]

These routines help to work with pkt-line values inside of a strbuf,
permitting simple formatting of buffered network messages.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 pkt-line.c |   84 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
 pkt-line.h |    4 +++
 2 files changed, 76 insertions(+), 12 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index b691abe..bd603f8 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -42,17 +42,19 @@ void packet_flush(int fd)
 	safe_write(fd, "0000", 4);
 }
 
+void packet_buf_flush(struct strbuf *buf)
+{
+	strbuf_add(buf, "0000", 4);
+}
+
 #define hex(a) (hexchar[(a) & 15])
-void packet_write(int fd, const char *fmt, ...)
+static char buffer[1000];
+static unsigned format_packet(const char *fmt, va_list args)
 {
-	static char buffer[1000];
 	static char hexchar[] = "0123456789abcdef";
-	va_list args;
 	unsigned n;
 
-	va_start(args, fmt);
 	n = vsnprintf(buffer + 4, sizeof(buffer) - 4, fmt, args);
-	va_end(args);
 	if (n >= sizeof(buffer)-4)
 		die("protocol error: impossibly long line");
 	n += 4;
@@ -60,9 +62,31 @@ void packet_write(int fd, const char *fmt, ...)
 	buffer[1] = hex(n >> 8);
 	buffer[2] = hex(n >> 4);
 	buffer[3] = hex(n);
+	return n;
+}
+
+void packet_write(int fd, const char *fmt, ...)
+{
+	va_list args;
+	unsigned n;
+
+	va_start(args, fmt);
+	n = format_packet(fmt, args);
+	va_end(args);
 	safe_write(fd, buffer, n);
 }
 
+void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
+{
+	va_list args;
+	unsigned n;
+
+	va_start(args, fmt);
+	n = format_packet(fmt, args);
+	va_end(args);
+	strbuf_add(buf, buffer, n);
+}
+
 static void safe_read(int fd, void *buffer, unsigned size)
 {
 	ssize_t ret = read_in_full(fd, buffer, size);
@@ -72,15 +96,11 @@ static void safe_read(int fd, void *buffer, unsigned size)
 		die("The remote end hung up unexpectedly");
 }
 
-int packet_read_line(int fd, char *buffer, unsigned size)
+static int packet_length(const char *linelen)
 {
 	int n;
-	unsigned len;
-	char linelen[4];
-
-	safe_read(fd, linelen, 4);
+	int len = 0;
 
-	len = 0;
 	for (n = 0; n < 4; n++) {
 		unsigned char c = linelen[n];
 		len <<= 4;
@@ -96,8 +116,20 @@ int packet_read_line(int fd, char *buffer, unsigned size)
 			len += c - 'A' + 10;
 			continue;
 		}
-		die("protocol error: bad line length character");
+		return -1;
 	}
+	return len;
+}
+
+int packet_read_line(int fd, char *buffer, unsigned size)
+{
+	int len;
+	char linelen[4];
+
+	safe_read(fd, linelen, 4);
+	len = packet_length(linelen);
+	if (len < 0)
+		die("protocol error: bad line length character");
 	if (!len)
 		return 0;
 	len -= 4;
@@ -107,3 +139,31 @@ int packet_read_line(int fd, char *buffer, unsigned size)
 	buffer[len] = 0;
 	return len;
 }
+
+int packet_get_line(struct strbuf *out,
+	char **src_buf, size_t *src_len)
+{
+	int len;
+
+	if (*src_len < 4)
+		return -1;
+	len = packet_length(*src_buf);
+	if (len < 0)
+		return -1;
+	if (!len) {
+		*src_buf += 4;
+		*src_len -= 4;
+		return 0;
+	}
+	if (*src_len < len)
+		return -2;
+
+	*src_buf += 4;
+	*src_len -= 4;
+	len -= 4;
+
+	strbuf_add(out, *src_buf, len);
+	*src_buf += len;
+	*src_len -= len;
+	return len;
+}
diff --git a/pkt-line.h b/pkt-line.h
index 9df653f..1e5dcfe 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -2,14 +2,18 @@
 #define PKTLINE_H
 
 #include "git-compat-util.h"
+#include "strbuf.h"
 
 /*
  * Silly packetized line writing interface
  */
 void packet_flush(int fd);
 void packet_write(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
+void packet_buf_flush(struct strbuf *buf);
+void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
 
 int packet_read_line(int fd, char *buffer, unsigned size);
+int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
 ssize_t safe_write(int, const void *, ssize_t);
 
 #endif
-- 
1.6.5.52.g0ff2e

[RFC PATCH v3 02/17] pkt-line: Make packet_read_line easier to debug

[Shawn O. Pearce]

When there is an error parsing the 4 byte length component we now
NUL terminate the string and display it as part of the die message,
this may hint as to what data was misunderstood by the application.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 pkt-line.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index bd603f8..893dd3c 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -124,12 +124,14 @@ static int packet_length(const char *linelen)
 int packet_read_line(int fd, char *buffer, unsigned size)
 {
 	int len;
-	char linelen[4];
+	char linelen[5];
 
 	safe_read(fd, linelen, 4);
 	len = packet_length(linelen);
-	if (len < 0)
-		die("protocol error: bad line length character");
+	if (len < 0) {
+		linelen[4] = '\0';
+		die("protocol error: bad line length character: %s", linelen);
+	}
 	if (!len)
 		return 0;
 	len -= 4;
-- 
1.6.5.52.g0ff2e

[RFC PATCH v3 03/17] fetch-pack: Use a strbuf to compose the want list

[Shawn O. Pearce]

This change is being offered as a refactoring to make later
commits in the smart HTTP series easier.

By changing the enabled capabilities to be formatted in a strbuf
it is easier to add a new capability to the set of supported
capabilities.

By formatting the want portion of the request into a strbuf and
writing it as a whole block we can later decide to hold onto
the req_buf (instead of releasing it) to recycle in stateless
communications.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 builtin-fetch-pack.c |   52 ++++++++++++++++++++++++++++++++-----------------
 commit.c             |   10 +++-----
 commit.h             |    2 +-
 3 files changed, 39 insertions(+), 25 deletions(-)

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 629735f..783c2b0 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -165,6 +165,7 @@ static int find_common(int fd[2], unsigned char *result_sha1,
 	const unsigned char *sha1;
 	unsigned in_vain = 0;
 	int got_continue = 0;
+	struct strbuf req_buf = STRBUF_INIT;
 
 	if (marked)
 		for_each_ref(clear_marks, NULL);
@@ -175,6 +176,7 @@ static int find_common(int fd[2], unsigned char *result_sha1,
 	fetching = 0;
 	for ( ; refs ; refs = refs->next) {
 		unsigned char *remote = refs->old_sha1;
+		const char *remote_hex;
 		struct object *o;
 
 		/*
@@ -192,27 +194,36 @@ static int find_common(int fd[2], unsigned char *result_sha1,
 			continue;
 		}
 
-		if (!fetching)
-			packet_write(fd[1], "want %s%s%s%s%s%s%s%s\n",
-				     sha1_to_hex(remote),
-				     (multi_ack ? " multi_ack" : ""),
-				     (use_sideband == 2 ? " side-band-64k" : ""),
-				     (use_sideband == 1 ? " side-band" : ""),
-				     (args.use_thin_pack ? " thin-pack" : ""),
-				     (args.no_progress ? " no-progress" : ""),
-				     (args.include_tag ? " include-tag" : ""),
-				     (prefer_ofs_delta ? " ofs-delta" : ""));
-		else
-			packet_write(fd[1], "want %s\n", sha1_to_hex(remote));
+		remote_hex = sha1_to_hex(remote);
+		if (!fetching) {
+			struct strbuf c = STRBUF_INIT;
+			if (multi_ack)          strbuf_addstr(&c, " multi_ack");
+			if (use_sideband == 2)  strbuf_addstr(&c, " side-band-64k");
+			if (use_sideband == 1)  strbuf_addstr(&c, " side-band");
+			if (args.use_thin_pack) strbuf_addstr(&c, " thin-pack");
+			if (args.no_progress)   strbuf_addstr(&c, " no-progress");
+			if (args.include_tag)   strbuf_addstr(&c, " include-tag");
+			if (prefer_ofs_delta)   strbuf_addstr(&c, " ofs-delta");
+			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);
+			strbuf_release(&c);
+		} else
+			packet_buf_write(&req_buf, "want %s\n", remote_hex);
 		fetching++;
 	}
+
+	if (!fetching) {
+		strbuf_release(&req_buf);
+		packet_flush(fd[1]);
+		return 1;
+	}
+
 	if (is_repository_shallow())
-		write_shallow_commits(fd[1], 1);
+		write_shallow_commits(&req_buf, 1);
 	if (args.depth > 0)
-		packet_write(fd[1], "deepen %d", args.depth);
-	packet_flush(fd[1]);
-	if (!fetching)
-		return 1;
+		packet_buf_write(&req_buf, "deepen %d", args.depth);
+	packet_buf_flush(&req_buf);
+
+	safe_write(fd[1], req_buf.buf, req_buf.len);
 
 	if (args.depth > 0) {
 		char line[1024];
@@ -296,6 +307,8 @@ done:
 		multi_ack = 0;
 		flushes++;
 	}
+	strbuf_release(&req_buf);
+
 	while (flushes || multi_ack) {
 		int ack = get_ack(fd[0], result_sha1);
 		if (ack) {
@@ -809,6 +822,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 
 	if (args.depth > 0) {
 		struct cache_time mtime;
+		struct strbuf sb = STRBUF_INIT;
 		char *shallow = git_path("shallow");
 		int fd;
 
@@ -826,12 +840,14 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 
 		fd = hold_lock_file_for_update(&lock, shallow,
 					       LOCK_DIE_ON_ERROR);
-		if (!write_shallow_commits(fd, 0)) {
+		if (!write_shallow_commits(&sb, 0)
+		 || write_in_full(fd, sb.buf, sb.len) != sb.len) {
 			unlink_or_warn(shallow);
 			rollback_lock_file(&lock);
 		} else {
 			commit_lock_file(&lock);
 		}
+		strbuf_release(&sb);
 	}
 
 	reprepare_packed_git();
diff --git a/commit.c b/commit.c
index fedbd5e..471efb0 100644
--- a/commit.c
+++ b/commit.c
@@ -199,7 +199,7 @@ struct commit_graft *lookup_commit_graft(const unsigned char *sha1)
 	return commit_graft[pos];
 }
 
-int write_shallow_commits(int fd, int use_pack_protocol)
+int write_shallow_commits(struct strbuf *out, int use_pack_protocol)
 {
 	int i, count = 0;
 	for (i = 0; i < commit_graft_nr; i++)
@@ -208,12 +208,10 @@ int write_shallow_commits(int fd, int use_pack_protocol)
 				sha1_to_hex(commit_graft[i]->sha1);
 			count++;
 			if (use_pack_protocol)
-				packet_write(fd, "shallow %s", hex);
+				packet_buf_write(out, "shallow %s", hex);
 			else {
-				if (write_in_full(fd, hex,  40) != 40)
-					break;
-				if (write_str_in_full(fd, "\n") != 1)
-					break;
+				strbuf_addstr(out, hex);
+				strbuf_addch(out, '\n');
 			}
 		}
 	return count;
diff --git a/commit.h b/commit.h
index f4fc5c5..817c75c 100644
--- a/commit.h
+++ b/commit.h
@@ -131,7 +131,7 @@ extern struct commit_list *get_octopus_merge_bases(struct commit_list *in);
 
 extern int register_shallow(const unsigned char *sha1);
 extern int unregister_shallow(const unsigned char *sha1);
-extern int write_shallow_commits(int fd, int use_pack_protocol);
+extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol);
 extern int is_repository_shallow(void);
 extern struct commit_list *get_shallow_commits(struct object_array *heads,
 		int depth, int shallow_flag, int not_shallow_flag);
-- 
1.6.5.52.g0ff2e

[RFC PATCH v3 04/17] Move "get_ack()" back to fetch-pack

[Shawn O. Pearce]

In 41cb7488 Linus moved this function to connect.c for reuse inside
of the git-clone-pack command.  That was 2005, but in 2006 Junio
retired git-clone-pack in commit efc7fa53.  Since then the only
caller has been fetch-pack.  Since this ACK/NAK exchange is only
used by the fetch-pack/upload-pack protocol we should keep move
it back to a private detail of fetch-pack.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 builtin-fetch-pack.c |   21 +++++++++++++++++++++
 cache.h              |    1 -
 connect.c            |   21 ---------------------
 3 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 783c2b0..7c09d46 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -157,6 +157,27 @@ static const unsigned char *get_rev(void)
 	return commit->object.sha1;
 }
 
+static int get_ack(int fd, unsigned char *result_sha1)
+{
+	static char line[1000];
+	int len = packet_read_line(fd, line, sizeof(line));
+
+	if (!len)
+		die("git fetch-pack: expected ACK/NAK, got EOF");
+	if (line[len-1] == '\n')
+		line[--len] = 0;
+	if (!strcmp(line, "NAK"))
+		return 0;
+	if (!prefixcmp(line, "ACK ")) {
+		if (!get_sha1_hex(line+4, result_sha1)) {
+			if (strstr(line+45, "continue"))
+				return 2;
+			return 1;
+		}
+	}
+	die("git fetch_pack: expected ACK/NAK, got '%s'", line);
+}
+
 static int find_common(int fd[2], unsigned char *result_sha1,
 		       struct ref *refs)
 {
diff --git a/cache.h b/cache.h
index a5eeead..4e283be 100644
--- a/cache.h
+++ b/cache.h
@@ -856,7 +856,6 @@ extern struct ref *find_ref_by_name(const struct ref *list, const char *name);
 extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);
 extern int finish_connect(struct child_process *conn);
 extern int path_match(const char *path, int nr, char **match);
-extern int get_ack(int fd, unsigned char *result_sha1);
 struct extra_have_objects {
 	int nr, alloc;
 	unsigned char (*array)[20];
diff --git a/connect.c b/connect.c
index 7945e38..839a103 100644
--- a/connect.c
+++ b/connect.c
@@ -107,27 +107,6 @@ int server_supports(const char *feature)
 		strstr(server_capabilities, feature) != NULL;
 }
 
-int get_ack(int fd, unsigned char *result_sha1)
-{
-	static char line[1000];
-	int len = packet_read_line(fd, line, sizeof(line));
-
-	if (!len)
-		die("git fetch-pack: expected ACK/NAK, got EOF");
-	if (line[len-1] == '\n')
-		line[--len] = 0;
-	if (!strcmp(line, "NAK"))
-		return 0;
-	if (!prefixcmp(line, "ACK ")) {
-		if (!get_sha1_hex(line+4, result_sha1)) {
-			if (strstr(line+45, "continue"))
-				return 2;
-			return 1;
-		}
-	}
-	die("git fetch_pack: expected ACK/NAK, got '%s'", line);
-}
-
 int path_match(const char *path, int nr, char **match)
 {
 	int i;
-- 
1.6.5.52.g0ff2e

[RFC PATCH v3 05/17] Add multi_ack_detailed capability to fetch-pack/upload-pack

[Shawn O. Pearce]

When multi_ack_detailed is enabled the ACK continue messages returned
by the remote upload-pack are broken out to describe the different
states within the peer.  This permits the client to better understand
the server's in-memory state.

The fetch-pack/upload-pack protocol now looks like:

NAK
---------------------------------
  Always sent in response to "done" if there was no common base
  selected from the "have" lines (or no have lines were sent).

  * no multi_ack or multi_ack_detailed:

    Sent when the client has sent a pkt-line flush ("0000") and
    the server has not yet found a common base object.

  * either multi_ack or multi_ack_detailed:

    Always sent in response to a pkt-line flush.

ACK %s
-----------------------------------
  * no multi_ack or multi_ack_detailed:

    Sent in response to "have" when the object exists on the remote
    side and is therefore an object in common between the peers.
    The argument is the SHA-1 of the common object.

  * either multi_ack or multi_ack_detailed:

    Sent in response to "done" if there are common objects.
    The argument is the last SHA-1 determined to be common.

ACK %s continue
-----------------------------------
  * multi_ack only:

    Sent in response to "have".

    The remote side wants the client to consider this object as
    common, and immediately stop transmitting additional "have"
    lines for objects that are reachable from it.  The reason
    the client should stop is not given, but is one of the two
    cases below available under multi_ack_detailed.

ACK %s common
-----------------------------------
  * multi_ack_detailed only:

    Sent in response to "have".  Both sides have this object.
    Like with "ACK %s continue" above the client should stop
    sending have lines reachable for objects from the argument.

ACK %s ready
-----------------------------------
  * multi_ack_detailed only:

    Sent in response to "have".

    The client should stop transmitting objects which are reachable
    from the argument, and send "done" soon to get the objects.

    If the remote side has the specified object, it should
    first send an "ACK %s common" message prior to sending
    "ACK %s ready".

    Clients may still submit additional "have" lines if there are
    more side branches for the client to explore that might be added
    to the common set and reduce the number of objects to transfer.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 builtin-fetch-pack.c |   41 ++++++++++++++++++++++++++++++++---------
 upload-pack.c        |   31 ++++++++++++++++++-------------
 2 files changed, 50 insertions(+), 22 deletions(-)

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 7c09d46..615f549 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -157,7 +157,15 @@ static const unsigned char *get_rev(void)
 	return commit->object.sha1;
 }
 
-static int get_ack(int fd, unsigned char *result_sha1)
+enum ack_type {
+	NAK = 0,
+	ACK,
+	ACK_continue,
+	ACK_common,
+	ACK_ready
+};
+
+static enum ack_type get_ack(int fd, unsigned char *result_sha1)
 {
 	static char line[1000];
 	int len = packet_read_line(fd, line, sizeof(line));
@@ -167,12 +175,16 @@ static int get_ack(int fd, unsigned char *result_sha1)
 	if (line[len-1] == '\n')
 		line[--len] = 0;
 	if (!strcmp(line, "NAK"))
-		return 0;
+		return NAK;
 	if (!prefixcmp(line, "ACK ")) {
 		if (!get_sha1_hex(line+4, result_sha1)) {
 			if (strstr(line+45, "continue"))
-				return 2;
-			return 1;
+				return ACK_continue;
+			if (strstr(line+45, "common"))
+				return ACK_common;
+			if (strstr(line+45, "ready"))
+				return ACK_ready;
+			return ACK;
 		}
 	}
 	die("git fetch_pack: expected ACK/NAK, got '%s'", line);
@@ -218,7 +230,8 @@ static int find_common(int fd[2], unsigned char *result_sha1,
 		remote_hex = sha1_to_hex(remote);
 		if (!fetching) {
 			struct strbuf c = STRBUF_INIT;
-			if (multi_ack)          strbuf_addstr(&c, " multi_ack");
+			if (multi_ack == 2)     strbuf_addstr(&c, " multi_ack_detailed");
+			if (multi_ack == 1)     strbuf_addstr(&c, " multi_ack");
 			if (use_sideband == 2)  strbuf_addstr(&c, " side-band-64k");
 			if (use_sideband == 1)  strbuf_addstr(&c, " side-band");
 			if (args.use_thin_pack) strbuf_addstr(&c, " thin-pack");
@@ -298,18 +311,23 @@ static int find_common(int fd[2], unsigned char *result_sha1,
 				if (args.verbose && ack)
 					fprintf(stderr, "got ack %d %s\n", ack,
 							sha1_to_hex(result_sha1));
-				if (ack == 1) {
+				switch (ack) {
+				case ACK:
 					flushes = 0;
 					multi_ack = 0;
 					retval = 0;
 					goto done;
-				} else if (ack == 2) {
+				case ACK_common:
+				case ACK_ready:
+				case ACK_continue: {
 					struct commit *commit =
 						lookup_commit(result_sha1);
 					mark_common(commit, 0, 1);
 					retval = 0;
 					in_vain = 0;
 					got_continue = 1;
+					break;
+					}
 				}
 			} while (ack);
 			flushes--;
@@ -336,7 +354,7 @@ done:
 			if (args.verbose)
 				fprintf(stderr, "got ack (%d) %s\n", ack,
 					sha1_to_hex(result_sha1));
-			if (ack == 1)
+			if (ack == ACK)
 				return 0;
 			multi_ack = 1;
 			continue;
@@ -618,7 +636,12 @@ static struct ref *do_fetch_pack(int fd[2],
 
 	if (is_repository_shallow() && !server_supports("shallow"))
 		die("Server does not support shallow clients");
-	if (server_supports("multi_ack")) {
+	if (server_supports("multi_ack_detailed")) {
+		if (args.verbose)
+			fprintf(stderr, "Server supports multi_ack_detailed\n");
+		multi_ack = 2;
+	}
+	else if (server_supports("multi_ack")) {
 		if (args.verbose)
 			fprintf(stderr, "Server supports multi_ack\n");
 		multi_ack = 1;
diff --git a/upload-pack.c b/upload-pack.c
index 38ddac2..f1dc3a3 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -498,7 +498,7 @@ static int get_common_commits(void)
 {
 	static char line[1000];
 	unsigned char sha1[20];
-	char hex[41], last_hex[41];
+	char last_hex[41];
 
 	save_commit_buffer = 0;
 
@@ -515,19 +515,22 @@ static int get_common_commits(void)
 		if (!prefixcmp(line, "have ")) {
 			switch (got_sha1(line+5, sha1)) {
 			case -1: /* they have what we do not */
-				if (multi_ack && ok_to_give_up())
-					packet_write(1, "ACK %s continue\n",
-						     sha1_to_hex(sha1));
+				if (multi_ack && ok_to_give_up()) {
+					const char *hex = sha1_to_hex(sha1);
+					if (multi_ack == 2)
+						packet_write(1, "ACK %s ready\n", hex);
+					else
+						packet_write(1, "ACK %s continue\n", hex);
+				}
 				break;
 			default:
-				memcpy(hex, sha1_to_hex(sha1), 41);
-				if (multi_ack) {
-					const char *msg = "ACK %s continue\n";
-					packet_write(1, msg, hex);
-					memcpy(last_hex, hex, 41);
-				}
+				memcpy(last_hex, sha1_to_hex(sha1), 41);
+				if (multi_ack == 2)
+					packet_write(1, "ACK %s common\n", last_hex);
+				else if (multi_ack)
+					packet_write(1, "ACK %s continue\n", last_hex);
 				else if (have_obj.nr == 1)
-					packet_write(1, "ACK %s\n", hex);
+					packet_write(1, "ACK %s\n", last_hex);
 				break;
 			}
 			continue;
@@ -587,7 +590,9 @@ static void receive_needs(void)
 		    get_sha1_hex(line+5, sha1_buf))
 			die("git upload-pack: protocol error, "
 			    "expected to get sha, not '%s'", line);
-		if (strstr(line+45, "multi_ack"))
+		if (strstr(line+45, "multi_ack_detailed"))
+			multi_ack = 2;
+		else if (strstr(line+45, "multi_ack"))
 			multi_ack = 1;
 		if (strstr(line+45, "thin-pack"))
 			use_thin_pack = 1;
@@ -681,7 +686,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 {
 	static const char *capabilities = "multi_ack thin-pack side-band"
 		" side-band-64k ofs-delta shallow no-progress"
-		" include-tag";
+		" include-tag multi_ack_detailed";
 	struct object *o = parse_object(sha1);
 
 	if (!o)
-- 
1.6.5.52.g0ff2e

[RFC PATCH v3 06/17] remote-curl: Refactor walker initialization

[Shawn O. Pearce]

We will need the walker, url and remote in other functions as the
code grows larger to support smart HTTP.  Extract this out into a
set of globals we can easily reference once configured.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
CC: Daniel Barkalow <barkalow@iabervon.org>
---
 remote-curl.c |   24 ++++++++++++++----------
 1 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 2faf1c6..478f3ea 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -5,7 +5,17 @@
 #include "http.h"
 #include "exec_cmd.h"
 
-static struct ref *get_refs(struct walker *walker, const char *url)
+static struct remote *remote;
+static const char *url;
+static struct walker *walker;
+
+static void init_walker(void)
+{
+	if (!walker)
+		walker = get_http_walker(url, remote);
+}
+
+static struct ref *get_refs(void)
 {
 	struct strbuf buffer = STRBUF_INIT;
 	char *data, *start, *mid;
@@ -21,6 +31,7 @@ static struct ref *get_refs(struct walker *walker, const char *url)
 	refs_url = xmalloc(strlen(url) + 11);
 	sprintf(refs_url, "%s/info/refs", url);
 
+	init_walker();
 	http_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);
 	switch (http_ret) {
 	case HTTP_OK:
@@ -78,10 +89,7 @@ static struct ref *get_refs(struct walker *walker, const char *url)
 
 int main(int argc, const char **argv)
 {
-	struct remote *remote;
 	struct strbuf buf = STRBUF_INIT;
-	const char *url;
-	struct walker *walker = NULL;
 
 	git_extract_argv0_path(argv[0]);
 	setup_git_directory();
@@ -103,8 +111,7 @@ int main(int argc, const char **argv)
 			break;
 		if (!prefixcmp(buf.buf, "fetch ")) {
 			char *obj = buf.buf + strlen("fetch ");
-			if (!walker)
-				walker = get_http_walker(url, remote);
+			init_walker();
 			walker->get_all = 1;
 			walker->get_tree = 1;
 			walker->get_history = 1;
@@ -115,11 +122,8 @@ int main(int argc, const char **argv)
 			printf("\n");
 			fflush(stdout);
 		} else if (!strcmp(buf.buf, "list")) {
-			struct ref *refs;
+			struct ref *refs = get_refs();
 			struct ref *posn;
-			if (!walker)
-				walker = get_http_walker(url, remote);
-			refs = get_refs(walker, url);
 			for (posn = refs; posn; posn = posn->next) {
 				if (posn->symref)
 					printf("@%s %s\n", posn->symref, posn->name);
-- 
1.6.5.52.g0ff2e

[RFC PATCH v3 07/17] fetch: Allow transport -v -v -v to set verbosity to 3

[Shawn O. Pearce]

Helpers might want a higher level of verbosity than just +1 (the
porcelain default setting) and +2 (-v -v).  Expand the field to
allow verbosity in the range -1..3.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
CC: Daniel Barkalow <barkalow@iabervon.org>
---
 builtin-fetch.c |    2 +-
 transport.h     |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index cb48c57..52a9a42 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -665,7 +665,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 
 	transport = transport_get(remote, remote->url[0]);
 	if (verbosity >= 2)
-		transport->verbose = 1;
+		transport->verbose = verbosity <= 3 ? verbosity : 3;
 	if (verbosity < 0)
 		transport->verbose = -1;
 	if (upload_pack)
diff --git a/transport.h b/transport.h
index c14da6f..e4e6177 100644
--- a/transport.h
+++ b/transport.h
@@ -25,7 +25,7 @@ struct transport {
 
 	int (*disconnect)(struct transport *connection);
 	char *pack_lockfile;
-	signed verbose : 2;
+	signed verbose : 3;
 	/* Force progress even if the output is not a tty */
 	unsigned progress : 1;
 };
-- 
1.6.5.52.g0ff2e

[RFC PATCH v3 08/17] remote-helpers: Fetch more than one ref in a batch

[Shawn O. Pearce]

Some network protocols (e.g. native git://) are able to fetch more
than one ref at a time and reduce the overall transfer cost by
combining the requests into a single exchange.  Instead of feeding
each fetch request one at a time to the helper, feed all of them
at once so the helper can decide whether or not it should batch them.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
CC: Daniel Barkalow <barkalow@iabervon.org>
---
 Documentation/git-remote-helpers.txt |   14 ++++--
 remote-curl.c                        |   88 +++++++++++++++++++++++++++++----
 transport-helper.c                   |   39 +++++++++++----
 3 files changed, 115 insertions(+), 26 deletions(-)

diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
index 173ee23..e44d821 100644
--- a/Documentation/git-remote-helpers.txt
+++ b/Documentation/git-remote-helpers.txt
@@ -36,10 +36,16 @@ Commands are given by the caller on the helper's standard input, one per line.
 	complete list, outputs a blank line.
 
 'fetch' <sha1> <name>::
-	Fetches the given object, writing the necessary objects to the
-	database. Outputs a blank line when the fetch is
-	complete. Only objects which were reported in the ref list
-	with a sha1 may be fetched this way.
+	Fetches the given object, writing the necessary objects
+	to the database.  Fetch commands are sent in a batch, one
+	per line, and the batch is terminated with a blank line.
+	Outputs a single blank line when all fetch commands in the
+	same batch are complete. Only objects which were reported
+	in the ref list with a sha1 may be fetched this way.
++
+Optionally may output a 'lock <file>' line indicating a file under
+GIT_DIR/objects/pack which is keeping a pack until refs can be
+suitably updated.
 +
 Supported if the helper has the "fetch" capability.
 
diff --git a/remote-curl.c b/remote-curl.c
index 478f3ea..22cd5c5 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -87,6 +87,81 @@ static struct ref *get_refs(void)
 	return refs;
 }
 
+static int fetch_dumb(int nr_heads, struct ref **to_fetch)
+{
+	char **targets = xmalloc(nr_heads * sizeof(char*));
+	int ret, i;
+
+	for (i = 0; i < nr_heads; i++)
+		targets[i] = xstrdup(sha1_to_hex(to_fetch[i]->old_sha1));
+
+	init_walker();
+	walker->get_all = 1;
+	walker->get_tree = 1;
+	walker->get_history = 1;
+	walker->get_verbosely = 0;
+	walker->get_recover = 0;
+	ret = walker_fetch(walker, nr_heads, targets, NULL, NULL);
+
+	for (i = 0; i < nr_heads; i++)
+		free(targets[i]);
+	free(targets);
+
+	return ret ? error("Fetch failed.") : 0;
+}
+
+static void parse_fetch(struct strbuf *buf)
+{
+	struct ref **to_fetch = NULL;
+	struct ref *list_head = NULL;
+	struct ref **list = &list_head;
+	int alloc_heads = 0, nr_heads = 0;
+
+	do {
+		if (!prefixcmp(buf->buf, "fetch ")) {
+			char *p = buf->buf + strlen("fetch ");
+			char *name;
+			struct ref *ref;
+			unsigned char old_sha1[20];
+
+			if (strlen(p) < 40 || get_sha1_hex(p, old_sha1))
+				die("protocol error: expected sha/ref, got %s'", p);
+			if (p[40] == ' ')
+				name = p + 41;
+			else if (!p[40])
+				name = "";
+			else
+				die("protocol error: expected sha/ref, got %s'", p);
+
+			ref = alloc_ref(name);
+			hashcpy(ref->old_sha1, old_sha1);
+
+			*list = ref;
+			list = &ref->next;
+
+			ALLOC_GROW(to_fetch, nr_heads + 1, alloc_heads);
+			to_fetch[nr_heads++] = ref;
+		}
+		else
+			die("http transport does not support %s", buf->buf);
+
+		strbuf_reset(buf);
+		if (strbuf_getline(buf, stdin, '\n') == EOF)
+			return;
+		if (!*buf->buf)
+			break;
+	} while (1);
+
+	if (fetch_dumb(nr_heads, to_fetch))
+		exit(128); /* error already reported */
+	free_refs(list_head);
+	free(to_fetch);
+
+	printf("\n");
+	fflush(stdout);
+	strbuf_reset(buf);
+}
+
 int main(int argc, const char **argv)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -110,17 +185,8 @@ int main(int argc, const char **argv)
 		if (strbuf_getline(&buf, stdin, '\n') == EOF)
 			break;
 		if (!prefixcmp(buf.buf, "fetch ")) {
-			char *obj = buf.buf + strlen("fetch ");
-			init_walker();
-			walker->get_all = 1;
-			walker->get_tree = 1;
-			walker->get_history = 1;
-			walker->get_verbosely = 0;
-			walker->get_recover = 0;
-			if (walker_fetch(walker, 1, &obj, NULL, NULL))
-				die("Fetch failed.");
-			printf("\n");
-			fflush(stdout);
+			parse_fetch(&buf);
+
 		} else if (!strcmp(buf.buf, "list")) {
 			struct ref *refs = get_refs();
 			struct ref *posn;
diff --git a/transport-helper.c b/transport-helper.c
index f57e84c..9de3408 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -10,6 +10,7 @@ struct helper_data
 {
 	const char *name;
 	struct child_process *helper;
+	FILE *out;
 	unsigned fetch : 1;
 };
 
@@ -18,7 +19,6 @@ static struct child_process *get_helper(struct transport *transport)
 	struct helper_data *data = transport->data;
 	struct strbuf buf = STRBUF_INIT;
 	struct child_process *helper;
-	FILE *file;
 
 	if (data->helper)
 		return data->helper;
@@ -39,9 +39,9 @@ static struct child_process *get_helper(struct transport *transport)
 
 	write_str_in_full(helper->in, "capabilities\n");
 
-	file = xfdopen(helper->out, "r");
+	data->out = xfdopen(helper->out, "r");
 	while (1) {
-		if (strbuf_getline(&buf, file, '\n') == EOF)
+		if (strbuf_getline(&buf, data->out, '\n') == EOF)
 			exit(128); /* child died, message supplied already */
 
 		if (!*buf.buf)
@@ -58,6 +58,7 @@ static int disconnect_helper(struct transport *transport)
 	if (data->helper) {
 		write_str_in_full(data->helper->in, "\n");
 		close(data->helper->in);
+		fclose(data->out);
 		finish_command(data->helper);
 		free((char *)data->helper->argv[0]);
 		free(data->helper->argv);
@@ -70,8 +71,7 @@ static int disconnect_helper(struct transport *transport)
 static int fetch_with_fetch(struct transport *transport,
 			    int nr_heads, const struct ref **to_fetch)
 {
-	struct child_process *helper = get_helper(transport);
-	FILE *file = xfdopen(helper->out, "r");
+	struct helper_data *data = transport->data;
 	int i;
 	struct strbuf buf = STRBUF_INIT;
 
@@ -82,12 +82,30 @@ static int fetch_with_fetch(struct transport *transport,
 
 		strbuf_addf(&buf, "fetch %s %s\n",
 			    sha1_to_hex(posn->old_sha1), posn->name);
-		write_in_full(helper->in, buf.buf, buf.len);
-		strbuf_reset(&buf);
+	}
 
-		if (strbuf_getline(&buf, file, '\n') == EOF)
+	strbuf_addch(&buf, '\n');
+	if (write_in_full(data->helper->in, buf.buf, buf.len) != buf.len)
+		die_errno("cannot send fetch to %s", data->name);
+
+	while (1) {
+		strbuf_reset(&buf);
+		if (strbuf_getline(&buf, data->out, '\n') == EOF)
 			exit(128); /* child died, message supplied already */
+
+		if (!prefixcmp(buf.buf, "lock ")) {
+			const char *name = buf.buf + 5;
+			if (transport->pack_lockfile)
+				warning("%s also locked %s", data->name, name);
+			else
+				transport->pack_lockfile = xstrdup(name);
+		}
+		else if (!buf.len)
+			break;
+		else
+			warning("%s unexpectedly said: '%s'", data->name, buf.buf);
 	}
+	strbuf_release(&buf);
 	return 0;
 }
 
@@ -113,21 +131,20 @@ static int fetch(struct transport *transport,
 
 static struct ref *get_refs_list(struct transport *transport, int for_push)
 {
+	struct helper_data *data = transport->data;
 	struct child_process *helper;
 	struct ref *ret = NULL;
 	struct ref **tail = &ret;
 	struct ref *posn;
 	struct strbuf buf = STRBUF_INIT;
-	FILE *file;
 
 	helper = get_helper(transport);
 
 	write_str_in_full(helper->in, "list\n");
 
-	file = xfdopen(helper->out, "r");
 	while (1) {
 		char *eov, *eon;
-		if (strbuf_getline(&buf, file, '\n') == EOF)
+		if (strbuf_getline(&buf, data->out, '\n') == EOF)
 			exit(128); /* child died, message supplied already */
 
 		if (!*buf.buf)
-- 
1.6.5.52.g0ff2e

[RFC PATCH v3 09/17] remote-helpers: Support custom transport options

[Shawn O. Pearce]

Some transports, like the native pack transport implemented by
fetch-pack, support useful features like depth or include tags.
These should be exposed if the underlying helper knows how to
use them.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
CC: Daniel Barkalow <barkalow@iabervon.org>
---
 Documentation/git-remote-helpers.txt |   38 +++++++++++++++
 remote-curl.c                        |   74 ++++++++++++++++++++++++++++-
 transport-helper.c                   |   88 +++++++++++++++++++++++++++++++++-
 3 files changed, 198 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
index e44d821..1133f04 100644
--- a/Documentation/git-remote-helpers.txt
+++ b/Documentation/git-remote-helpers.txt
@@ -35,6 +35,16 @@ Commands are given by the caller on the helper's standard input, one per line.
 	the name; unrecognized attributes are ignored. After the
 	complete list, outputs a blank line.
 
+'option' <name> <value>::
+	Set the transport helper option <name> to <value>.  Outputs a
+	single line containing one of 'ok' (option successfully set),
+	'unsupported' (option not recognized) or 'error <msg>'
+	(option <name> is supported but <value> is not correct
+	for it).  Options should be set before other commands,
+	and may how those commands behave.
++
+Supported if the helper has the "option" capability.
+
 'fetch' <sha1> <name>::
 	Fetches the given object, writing the necessary objects
 	to the database.  Fetch commands are sent in a batch, one
@@ -63,11 +73,39 @@ CAPABILITIES
 'fetch'::
 	This helper supports the 'fetch' command.
 
+'option'::
+	This helper supports the option command.
+
 REF LIST ATTRIBUTES
 -------------------
 
 None are defined yet, but the caller must accept any which are supplied.
 
+OPTIONS
+-------
+'option verbosity' <N>::
+	Change the level of messages displayed by the helper.
+	When N is 0 the end-user has asked the process to be
+	quiet, and the helper should produce only error output.
+	N of 1 is the default level of verbosity, higher values
+	of N correspond to the number of -v flags passed on the
+	command line.
+
+'option progress' \{'true'|'false'\}::
+	Enable (or disable) progress messages displayed by the
+	transport helper during a command.
+
+'option depth' <depth>::
+	Deepen the history of a shallow repository.
+
+'option followtags' \{'true'|'false'\}::
+	If enabled the helper should automatically fetch annotated
+	tag objects if the object the tag points at was transferred
+	during the fetch command.  If the tag is not fetched by
+	the helper a second fetch command will usually be sent to
+	ask for the tag specifically.  Some helpers may be able to
+	use this option to avoid a second network connection.
+
 Documentation
 -------------
 Documentation by Daniel Barkalow.
diff --git a/remote-curl.c b/remote-curl.c
index 22cd5c5..0951f11 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -9,12 +9,61 @@ static struct remote *remote;
 static const char *url;
 static struct walker *walker;
 
+struct options {
+	int verbosity;
+	unsigned long depth;
+	unsigned progress : 1,
+		followtags : 1;
+};
+static struct options options;
+
 static void init_walker(void)
 {
 	if (!walker)
 		walker = get_http_walker(url, remote);
 }
 
+static int set_option(const char *name, const char *value)
+{
+	if (!strcmp(name, "verbosity")) {
+		char *end;
+		int v = strtol(value, &end, 10);
+		if (value == end || *end)
+			return -1;
+		options.verbosity = v;
+		return 0;
+	}
+	else if (!strcmp(name, "progress")) {
+		if (!strcmp(value, "true"))
+			options.progress = 1;
+		else if (!strcmp(value, "false"))
+			options.progress = 0;
+		else
+			return -1;
+		return 1 /* TODO implement later */;
+	}
+	else if (!strcmp(name, "depth")) {
+		char *end;
+		unsigned long v = strtoul(value, &end, 10);
+		if (value == end || *end)
+			return -1;
+		options.depth = v;
+		return 1 /* TODO implement later */;
+	}
+	else if (!strcmp(name, "followtags")) {
+		if (!strcmp(value, "true"))
+			options.followtags = 1;
+		else if (!strcmp(value, "false"))
+			options.followtags = 0;
+		else
+			return -1;
+		return 1 /* TODO implement later */;
+	}
+	else {
+		return 1 /* unsupported */;
+	}
+}
+
 static struct ref *get_refs(void)
 {
 	struct strbuf buffer = STRBUF_INIT;
@@ -99,7 +148,7 @@ static int fetch_dumb(int nr_heads, struct ref **to_fetch)
 	walker->get_all = 1;
 	walker->get_tree = 1;
 	walker->get_history = 1;
-	walker->get_verbosely = 0;
+	walker->get_verbosely = options.verbosity >= 3;
 	walker->get_recover = 0;
 	ret = walker_fetch(walker, nr_heads, targets, NULL, NULL);
 
@@ -173,6 +222,9 @@ int main(int argc, const char **argv)
 		return 1;
 	}
 
+	options.verbosity = 1;
+	options.progress = !!isatty(2);
+
 	remote = remote_get(argv[1]);
 
 	if (argc > 2) {
@@ -198,8 +250,28 @@ int main(int argc, const char **argv)
 			}
 			printf("\n");
 			fflush(stdout);
+		} else if (!prefixcmp(buf.buf, "option ")) {
+			char *name = buf.buf + strlen("option ");
+			char *value = strchr(name, ' ');
+			int result;
+
+			if (value)
+				*value++ = '\0';
+			else
+				value = "true";
+
+			result = set_option(name, value);
+			if (!result)
+				printf("ok\n");
+			else if (result < 0)
+				printf("error invalid value\n");
+			else
+				printf("unsupported\n");
+			fflush(stdout);
+
 		} else if (!strcmp(buf.buf, "capabilities")) {
 			printf("fetch\n");
+			printf("option\n");
 			printf("\n");
 			fflush(stdout);
 		} else {
diff --git a/transport-helper.c b/transport-helper.c
index 9de3408..577abc6 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -5,13 +5,15 @@
 #include "commit.h"
 #include "diff.h"
 #include "revision.h"
+#include "quote.h"
 
 struct helper_data
 {
 	const char *name;
 	struct child_process *helper;
 	FILE *out;
-	unsigned fetch : 1;
+	unsigned fetch : 1,
+		option : 1;
 };
 
 static struct child_process *get_helper(struct transport *transport)
@@ -48,6 +50,8 @@ static struct child_process *get_helper(struct transport *transport)
 			break;
 		if (!strcmp(buf.buf, "fetch"))
 			data->fetch = 1;
+		if (!strcmp(buf.buf, "option"))
+			data->option = 1;
 	}
 	return data->helper;
 }
@@ -65,9 +69,88 @@ static int disconnect_helper(struct transport *transport)
 		free(data->helper);
 		data->helper = NULL;
 	}
+	free(data);
 	return 0;
 }
 
+static const char *unsupported_options[] = {
+	TRANS_OPT_UPLOADPACK,
+	TRANS_OPT_RECEIVEPACK,
+	TRANS_OPT_THIN,
+	TRANS_OPT_KEEP
+	};
+static const char *boolean_options[] = {
+	TRANS_OPT_THIN,
+	TRANS_OPT_KEEP,
+	TRANS_OPT_FOLLOWTAGS
+	};
+
+static int set_helper_option(struct transport *transport,
+			  const char *name, const char *value)
+{
+	struct helper_data *data = transport->data;
+	struct child_process *helper = get_helper(transport);
+	struct strbuf buf = STRBUF_INIT;
+	int i, ret, is_bool = 0;
+
+	if (!data->option)
+		return 1;
+
+	for (i = 0; i < ARRAY_SIZE(unsupported_options); i++) {
+		if (!strcmp(name, unsupported_options[i]))
+			return 1;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(boolean_options); i++) {
+		if (!strcmp(name, boolean_options[i])) {
+			is_bool = 1;
+			break;
+		}
+	}
+
+	strbuf_addf(&buf, "option %s ", name);
+	if (is_bool)
+		strbuf_addstr(&buf, value ? "true" : "false");
+	else
+		quote_c_style(value, &buf, NULL, 0);
+	strbuf_addch(&buf, '\n');
+
+	if (write_in_full(helper->in, buf.buf, buf.len) != buf.len)
+		die_errno("cannot send option to %s", data->name);
+
+	strbuf_reset(&buf);
+	if (strbuf_getline(&buf, data->out, '\n') == EOF)
+		exit(128); /* child died, message supplied already */
+
+	if (!strcmp(buf.buf, "ok"))
+		ret = 0;
+	else if (!prefixcmp(buf.buf, "error")) {
+		ret = -1;
+	} else if (!strcmp(buf.buf, "unsupported"))
+		ret = 1;
+	else {
+		warning("%s unexpectedly said: '%s'", data->name, buf.buf);
+		ret = 1;
+	}
+	strbuf_release(&buf);
+	return ret;
+}
+
+static void standard_options(struct transport *t)
+{
+	char buf[16];
+	int n;
+	int v = t->verbose;
+	int no_progress = v < 0 || (!t->progress && !isatty(1));
+
+	set_helper_option(t, "progress", !no_progress ? "true" : "false");
+
+	n = snprintf(buf, sizeof(buf), "%d", v + 1);
+	if (n >= sizeof(buf))
+		die("impossibly large verbosity value");
+	set_helper_option(t, "verbosity", buf);
+}
+
 static int fetch_with_fetch(struct transport *transport,
 			    int nr_heads, const struct ref **to_fetch)
 {
@@ -75,6 +158,8 @@ static int fetch_with_fetch(struct transport *transport,
 	int i;
 	struct strbuf buf = STRBUF_INIT;
 
+	standard_options(transport);
+
 	for (i = 0; i < nr_heads; i++) {
 		const struct ref *posn = to_fetch[i];
 		if (posn->status & REF_STATUS_UPTODATE)
@@ -178,6 +263,7 @@ int transport_helper_init(struct transport *transport, const char *name)
 	data->name = name;
 
 	transport->data = data;
+	transport->set_option = set_helper_option;
 	transport->get_refs_list = get_refs_list;
 	transport->fetch = fetch;
 	transport->disconnect = disconnect_helper;
-- 
1.6.5.52.g0ff2e

[RFC PATCH v3 10/17] Move WebDAV HTTP push under remote-curl

[Shawn O. Pearce]

The remote helper interface now supports the push capability,
which can be used to ask the implementation to push one or more
specs to the remote repository.  For remote-curl we implement this
by calling the existing WebDAV based git-http-push executable.

Internally the helper interface uses the push_refs transport hook
so that the complexity of the refspec parsing and matching can be
reused between remote implementations.  When possible however the
helper protocol uses source ref name rather than the source SHA-1,
thereby allowing the helper to access this name if it is useful.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
CC: Daniel Barkalow <barkalow@iabervon.org>
CC: Tay Ray Chuan <rctay89@gmail.com>
CC: Mike Hommey <mh@glandium.org>
---
 Documentation/git-remote-helpers.txt |   33 ++++++++-
 http-push.c                          |   43 ++++++++---
 remote-curl.c                        |   98 +++++++++++++++++++++---
 transport-helper.c                   |  137 +++++++++++++++++++++++++++++++++-
 transport.c                          |   31 --------
 5 files changed, 286 insertions(+), 56 deletions(-)

diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
index 1133f04..3751b12 100644
--- a/Documentation/git-remote-helpers.txt
+++ b/Documentation/git-remote-helpers.txt
@@ -34,6 +34,10 @@ Commands are given by the caller on the helper's standard input, one per line.
 	value of the ref. A space-separated list of attributes follows
 	the name; unrecognized attributes are ignored. After the
 	complete list, outputs a blank line.
++
+If 'push' is supported this may be called as 'list for-push'
+to obtain the current refs prior to sending one or more 'push'
+commands to the helper.
 
 'option' <name> <value>::
 	Set the transport helper option <name> to <value>.  Outputs a
@@ -59,6 +63,22 @@ suitably updated.
 +
 Supported if the helper has the "fetch" capability.
 
+'push' +<src>:<dst>::
+	Pushes the given <src> commit or branch locally to the
+	remote branch described by <dst>.  A batch sequence of
+	one or more push commands is terminated with a blank line.
++
+Zero or more protocol options may be entered after the last 'push'
+command, before the batch's terminating blank line.
++
+When the push is complete, outputs one or more 'ok <dst>' or
+'error <dst> <why>?' lines to indicate success or failure of
+each pushed ref.  The status report output is terminated by
+a blank line.  The option field <why> may be quoted in a C
+style string if it contains an LF.
++
+Supported if the helper has the "push" 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
@@ -76,10 +96,16 @@ CAPABILITIES
 'option'::
 	This helper supports the option command.
 
+'push'::
+	This helper supports the 'push' command.
+
 REF LIST ATTRIBUTES
 -------------------
 
-None are defined yet, but the caller must accept any which are supplied.
+'for-push'::
+	The caller wants to use the ref list to prepare push
+	commands.  A helper might chose to acquire the ref list by
+	opening a different type of connection to the destination.
 
 OPTIONS
 -------
@@ -106,6 +132,11 @@ OPTIONS
 	ask for the tag specifically.  Some helpers may be able to
 	use this option to avoid a second network connection.
 
+'option dry-run' \{'true'|'false'\}:
+	If true, pretend like the operation completed successfully,
+	but don't actually change any repository data.	For most
+	helpers this only applies to the 'push', if supported.
+
 Documentation
 -------------
 Documentation by Daniel Barkalow.
diff --git a/http-push.c b/http-push.c
index 00e83dc..9010ccc 100644
--- a/http-push.c
+++ b/http-push.c
@@ -78,6 +78,7 @@ static int push_verbosely;
 static int push_all = MATCH_REFS_NONE;
 static int force_all;
 static int dry_run;
+static int helper_status;
 
 static struct object_list *objects;
 
@@ -1813,6 +1814,10 @@ int main(int argc, char **argv)
 				dry_run = 1;
 				continue;
 			}
+			if (!strcmp(arg, "--helper-status")) {
+				helper_status = 1;
+				continue;
+			}
 			if (!strcmp(arg, "--verbose")) {
 				push_verbosely = 1;
 				http_is_verbose = 1;
@@ -1941,9 +1946,14 @@ int main(int argc, char **argv)
 
 		if (is_null_sha1(ref->peer_ref->new_sha1)) {
 			if (delete_remote_branch(ref->name, 1) == -1) {
-				error("Could not remove %s", ref->name);
+				if (helper_status)
+					printf("error %s cannot remove\n", ref->name);
+				else
+					error("Could not remove %s", ref->name);
 				rc = -4;
 			}
+			else if (helper_status)
+				printf("ok %s\n", ref->name);
 			new_refs++;
 			continue;
 		}
@@ -1951,6 +1961,8 @@ int main(int argc, char **argv)
 		if (!hashcmp(ref->old_sha1, ref->peer_ref->new_sha1)) {
 			if (push_verbosely || 1)
 				fprintf(stderr, "'%s': up-to-date\n", ref->name);
+			if (helper_status)
+				printf("ok %s up to date\n", ref->name);
 			continue;
 		}
 
@@ -1968,12 +1980,15 @@ int main(int argc, char **argv)
 				 * commits at the remote end and likely
 				 * we were not up to date to begin with.
 				 */
-				error("remote '%s' is not an ancestor of\n"
-				      "local '%s'.\n"
-				      "Maybe you are not up-to-date and "
-				      "need to pull first?",
-				      ref->name,
-				      ref->peer_ref->name);
+				if (helper_status)
+					printf("error %s non-fast forward\n", ref->name);
+				else
+					error("remote '%s' is not an ancestor of\n"
+						  "local '%s'.\n"
+						  "Maybe you are not up-to-date and "
+						  "need to pull first?",
+						  ref->name,
+						  ref->peer_ref->name);
 				rc = -2;
 				continue;
 			}
@@ -1987,14 +2002,20 @@ int main(int argc, char **argv)
 		if (strcmp(ref->name, ref->peer_ref->name))
 			fprintf(stderr, " using '%s'", ref->peer_ref->name);
 		fprintf(stderr, "\n  from %s\n  to   %s\n", old_hex, new_hex);
-		if (dry_run)
+		if (dry_run) {
+			if (helper_status)
+				printf("ok %s\n", ref->name);
 			continue;
+		}
 
 		/* Lock remote branch ref */
 		ref_lock = lock_remote(ref->name, LOCK_TIME);
 		if (ref_lock == NULL) {
-			fprintf(stderr, "Unable to lock remote branch %s\n",
-				ref->name);
+			if (helper_status)
+				printf("error %s lock error\n", ref->name);
+			else
+				fprintf(stderr, "Unable to lock remote branch %s\n",
+					ref->name);
 			rc = 1;
 			continue;
 		}
@@ -2045,6 +2066,8 @@ int main(int argc, char **argv)
 
 		if (!rc)
 			fprintf(stderr, "    done\n");
+		if (helper_status)
+			printf("%s %s\n", !rc ? "ok" : "error", ref->name);
 		unlock_remote(ref_lock);
 		check_locks();
 	}
diff --git a/remote-curl.c b/remote-curl.c
index 0951f11..af2fddf 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -4,6 +4,7 @@
 #include "walker.h"
 #include "http.h"
 #include "exec_cmd.h"
+#include "run-command.h"
 
 static struct remote *remote;
 static const char *url;
@@ -13,7 +14,8 @@ struct options {
 	int verbosity;
 	unsigned long depth;
 	unsigned progress : 1,
-		followtags : 1;
+		followtags : 1,
+		dry_run : 1;
 };
 static struct options options;
 
@@ -59,6 +61,15 @@ static int set_option(const char *name, const char *value)
 			return -1;
 		return 1 /* TODO implement later */;
 	}
+	else if (!strcmp(name, "dry-run")) {
+		if (!strcmp(value, "true"))
+			options.dry_run = 1;
+		else if (!strcmp(value, "false"))
+			options.dry_run = 0;
+		else
+			return -1;
+		return 0;
+	}
 	else {
 		return 1 /* unsupported */;
 	}
@@ -136,6 +147,20 @@ static struct ref *get_refs(void)
 	return refs;
 }
 
+static void output_refs(struct ref *refs)
+{
+	struct ref *posn;
+	for (posn = refs; posn; posn = posn->next) {
+		if (posn->symref)
+			printf("@%s %s\n", posn->symref, posn->name);
+		else
+			printf("%s %s\n", sha1_to_hex(posn->old_sha1), posn->name);
+	}
+	printf("\n");
+	fflush(stdout);
+	free_refs(refs);
+}
+
 static int fetch_dumb(int nr_heads, struct ref **to_fetch)
 {
 	char **targets = xmalloc(nr_heads * sizeof(char*));
@@ -211,6 +236,58 @@ static void parse_fetch(struct strbuf *buf)
 	strbuf_reset(buf);
 }
 
+static int push_dav(int nr_spec, char **specs)
+{
+	const char **argv = xmalloc((10 + nr_spec) * sizeof(char*));
+	int argc = 0, i;
+
+	argv[argc++] = "http-push";
+	argv[argc++] = "--helper-status";
+	if (options.dry_run)
+		argv[argc++] = "--dry-run";
+	if (options.verbosity > 1)
+		argv[argc++] = "--verbose";
+	argv[argc++] = url;
+	for (i = 0; i < nr_spec; i++)
+		argv[argc++] = specs[i];
+	argv[argc++] = NULL;
+
+	if (run_command_v_opt(argv, RUN_GIT_CMD))
+		die("git-%s failed", argv[0]);
+	free(argv);
+	return 0;
+}
+
+static void parse_push(struct strbuf *buf)
+{
+	char **specs = NULL;
+	int alloc_spec = 0, nr_spec = 0, i;
+
+	do {
+		if (!prefixcmp(buf->buf, "push ")) {
+			ALLOC_GROW(specs, nr_spec + 1, alloc_spec);
+			specs[nr_spec++] = xstrdup(buf->buf + 5);
+		}
+		else
+			die("http transport does not support %s", buf->buf);
+
+		strbuf_reset(buf);
+		if (strbuf_getline(buf, stdin, '\n') == EOF)
+			return;
+		if (!*buf->buf)
+			break;
+	} while (1);
+
+	if (push_dav(nr_spec, specs))
+		exit(128); /* error already reported */
+	for (i = 0; i < nr_spec; i++)
+		free(specs[i]);
+	free(specs);
+
+	printf("\n");
+	fflush(stdout);
+}
+
 int main(int argc, const char **argv)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -240,16 +317,14 @@ int main(int argc, const char **argv)
 			parse_fetch(&buf);
 
 		} else if (!strcmp(buf.buf, "list")) {
-			struct ref *refs = get_refs();
-			struct ref *posn;
-			for (posn = refs; posn; posn = posn->next) {
-				if (posn->symref)
-					printf("@%s %s\n", posn->symref, posn->name);
-				else
-					printf("%s %s\n", sha1_to_hex(posn->old_sha1), posn->name);
-			}
-			printf("\n");
-			fflush(stdout);
+			output_refs(get_refs());
+
+		} else if (!strcmp(buf.buf, "list for-push")) {
+			output_refs(get_refs());
+
+		} else if (!prefixcmp(buf.buf, "push ")) {
+			parse_push(&buf);
+
 		} else if (!prefixcmp(buf.buf, "option ")) {
 			char *name = buf.buf + strlen("option ");
 			char *value = strchr(name, ' ');
@@ -272,6 +347,7 @@ int main(int argc, const char **argv)
 		} else if (!strcmp(buf.buf, "capabilities")) {
 			printf("fetch\n");
 			printf("option\n");
+			printf("push\n");
 			printf("\n");
 			fflush(stdout);
 		} else {
diff --git a/transport-helper.c b/transport-helper.c
index 577abc6..16c6641 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1,6 +1,6 @@
 #include "cache.h"
 #include "transport.h"
-
+#include "quote.h"
 #include "run-command.h"
 #include "commit.h"
 #include "diff.h"
@@ -13,7 +13,8 @@ struct helper_data
 	struct child_process *helper;
 	FILE *out;
 	unsigned fetch : 1,
-		option : 1;
+		option : 1,
+		push : 1;
 };
 
 static struct child_process *get_helper(struct transport *transport)
@@ -52,6 +53,8 @@ static struct child_process *get_helper(struct transport *transport)
 			data->fetch = 1;
 		if (!strcmp(buf.buf, "option"))
 			data->option = 1;
+		if (!strcmp(buf.buf, "push"))
+			data->push = 1;
 	}
 	return data->helper;
 }
@@ -214,6 +217,130 @@ static int fetch(struct transport *transport,
 	return -1;
 }
 
+static int push_refs(struct transport *transport,
+		struct ref *remote_refs, int flags)
+{
+	int force_all = flags & TRANSPORT_PUSH_FORCE;
+	int mirror = flags & TRANSPORT_PUSH_MIRROR;
+	struct helper_data *data = transport->data;
+	struct strbuf buf = STRBUF_INIT;
+	struct child_process *helper;
+	struct ref *ref;
+
+	if (!remote_refs)
+		return 0;
+
+	helper = get_helper(transport);
+	if (!data->push)
+		return 1;
+
+	for (ref = remote_refs; ref; ref = ref->next) {
+		if (ref->peer_ref)
+			hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
+		else if (!mirror)
+			continue;
+
+		ref->deletion = is_null_sha1(ref->new_sha1);
+		if (!ref->deletion &&
+			!hashcmp(ref->old_sha1, ref->new_sha1)) {
+			ref->status = REF_STATUS_UPTODATE;
+			continue;
+		}
+
+		if (force_all)
+			ref->force = 1;
+
+		strbuf_addstr(&buf, "push ");
+		if (!ref->deletion) {
+			if (ref->force)
+				strbuf_addch(&buf, '+');
+			if (ref->peer_ref)
+				strbuf_addstr(&buf, ref->peer_ref->name);
+			else
+				strbuf_addstr(&buf, sha1_to_hex(ref->new_sha1));
+		}
+		strbuf_addch(&buf, ':');
+		strbuf_addstr(&buf, ref->name);
+		strbuf_addch(&buf, '\n');
+	}
+
+	transport->verbose = flags & TRANSPORT_PUSH_VERBOSE ? 1 : 0;
+	standard_options(transport);
+
+	if (flags & TRANSPORT_PUSH_DRY_RUN) {
+		if (set_helper_option(transport, "dry-run", "true") != 0)
+			die("helper %s does not support dry-run", data->name);
+	}
+
+	strbuf_addch(&buf, '\n');
+	if (write_in_full(helper->in, buf.buf, buf.len) != buf.len)
+		exit(128);
+
+	ref = remote_refs;
+	while (1) {
+		char *refname, *msg;
+		int status;
+
+		strbuf_reset(&buf);
+		if (strbuf_getline(&buf, data->out, '\n') == EOF)
+			exit(128); /* child died, message supplied already */
+		if (!buf.len)
+			break;
+
+		if (!prefixcmp(buf.buf, "ok ")) {
+			status = REF_STATUS_OK;
+			refname = buf.buf + 3;
+		} else if (!prefixcmp(buf.buf, "error ")) {
+			status = REF_STATUS_REMOTE_REJECT;
+			refname = buf.buf + 6;
+		} else
+			die("expected ok/error, helper said '%s'\n", buf.buf);
+
+		msg = strchr(refname, ' ');
+		if (msg) {
+			struct strbuf msg_buf = STRBUF_INIT;
+			const char *end;
+
+			*msg++ = '\0';
+			if (!unquote_c_style(&msg_buf, msg, &end))
+				msg = strbuf_detach(&msg_buf, NULL);
+			else
+				msg = xstrdup(msg);
+			strbuf_release(&msg_buf);
+
+			if (!strcmp(msg, "no match")) {
+				status = REF_STATUS_NONE;
+				free(msg);
+				msg = NULL;
+			}
+			else if (!strcmp(msg, "up to date")) {
+				status = REF_STATUS_UPTODATE;
+				free(msg);
+				msg = NULL;
+			}
+			else if (!strcmp(msg, "non-fast forward")) {
+				status = REF_STATUS_REJECT_NONFASTFORWARD;
+				free(msg);
+				msg = NULL;
+			}
+		}
+
+		if (ref)
+			ref = find_ref_by_name(ref, refname);
+		if (!ref)
+			ref = find_ref_by_name(remote_refs, refname);
+		if (!ref) {
+			warning("helper reported unexpected status of %s", refname);
+			continue;
+		}
+
+		ref->status = status;
+		ref->remote_status = msg;
+	}
+	strbuf_release(&buf);
+	return 0;
+}
+
 static struct ref *get_refs_list(struct transport *transport, int for_push)
 {
 	struct helper_data *data = transport->data;
@@ -225,7 +352,10 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 
 	helper = get_helper(transport);
 
-	write_str_in_full(helper->in, "list\n");
+	if (data->push && for_push)
+		write_str_in_full(helper->in, "list for-push\n");
+	else
+		write_str_in_full(helper->in, "list\n");
 
 	while (1) {
 		char *eov, *eon;
@@ -266,6 +396,7 @@ int transport_helper_init(struct transport *transport, const char *name)
 	transport->set_option = set_helper_option;
 	transport->get_refs_list = get_refs_list;
 	transport->fetch = fetch;
+	transport->push_refs = push_refs;
 	transport->disconnect = disconnect_helper;
 	return 0;
 }
diff --git a/transport.c b/transport.c
index 644a30a..6d9652d 100644
--- a/transport.c
+++ b/transport.c
@@ -349,35 +349,6 @@ static int rsync_transport_push(struct transport *transport,
 	return result;
 }
 
-#ifndef NO_CURL
-static int curl_transport_push(struct transport *transport, int refspec_nr, const char **refspec, int flags)
-{
-	const char **argv;
-	int argc;
-
-	if (flags & TRANSPORT_PUSH_MIRROR)
-		return error("http transport does not support mirror mode");
-
-	argv = xmalloc((refspec_nr + 12) * sizeof(char *));
-	argv[0] = "http-push";
-	argc = 1;
-	if (flags & TRANSPORT_PUSH_ALL)
-		argv[argc++] = "--all";
-	if (flags & TRANSPORT_PUSH_FORCE)
-		argv[argc++] = "--force";
-	if (flags & TRANSPORT_PUSH_DRY_RUN)
-		argv[argc++] = "--dry-run";
-	if (flags & TRANSPORT_PUSH_VERBOSE)
-		argv[argc++] = "--verbose";
-	argv[argc++] = transport->url;
-	while (refspec_nr--)
-		argv[argc++] = *refspec++;
-	argv[argc] = NULL;
-	return !!run_command_v_opt(argv, RUN_GIT_CMD);
-}
-
-#endif
-
 struct bundle_transport_data {
 	int fd;
 	struct bundle_header header;
@@ -826,8 +797,6 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		transport_helper_init(ret, "curl");
 #ifdef NO_CURL
 		error("git was compiled without libcurl support.");
-#else
-		ret->push = curl_transport_push;
 #endif
 
 	} else if (is_local(url) && is_file(url)) {
-- 
1.6.5.52.g0ff2e

[RFC PATCH v3 11/17] Git-aware CGI to provide dumb HTTP transport

[Shawn O. Pearce]

The git-http-backend CGI can be configured into any Apache server
using ScriptAlias, such as with the following configuration:

  LoadModule cgi_module /usr/libexec/apache2/mod_cgi.so
  LoadModule alias_module /usr/libexec/apache2/mod_alias.so
  ScriptAlias /git/ /usr/libexec/git-core/git-http-backend/

Repositories are accessed via the translated PATH_INFO.

The CGI is backwards compatible with the dumb client, allowing all
older HTTP clients to continue to download repositories which are
managed by the CGI.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .gitignore                         |    1 +
 Documentation/git-http-backend.txt |  105 +++++++++++++
 Makefile                           |    1 +
 http-backend.c                     |  290 ++++++++++++++++++++++++++++++++++++
 4 files changed, 397 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/git-http-backend.txt
 create mode 100644 http-backend.c

diff --git a/.gitignore b/.gitignore
index 51a37b1..353d22f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -55,6 +55,7 @@ git-get-tar-commit-id
 git-grep
 git-hash-object
 git-help
+git-http-backend
 git-http-fetch
 git-http-push
 git-imap-send
diff --git a/Documentation/git-http-backend.txt b/Documentation/git-http-backend.txt
new file mode 100644
index 0000000..867675f
--- /dev/null
+++ b/Documentation/git-http-backend.txt
@@ -0,0 +1,105 @@
+git-http-backend(1)
+===================
+
+NAME
+----
+git-http-backend - Server side implementation of Git over HTTP
+
+SYNOPSIS
+--------
+[verse]
+'git-http-backend'
+
+DESCRIPTION
+-----------
+A simple CGI program to serve the contents of a Git repository to Git
+clients accessing the repository over http:// and https:// protocols.
+
+By default, only the `upload-pack` service is enabled, which serves
+'git-fetch-pack' and 'git-ls-remote' clients, which are invoked from
+'git-fetch', 'git-pull', and 'git-clone'.
+
+This is ideally suited for read-only updates, i.e., pulling from
+git repositories.
+
+URL TRANSLATION
+---------------
+'git-http-backend' relies on the invoking web server to perform
+URL to path translation, and store the repository path into the
+PATH_TRANSLATED environment variable.  Most web servers will do
+this translation automatically, resolving the suffix after the
+CGI name relative to the server's document root.
+
+EXAMPLES
+--------
+
+Apache 2.x::
+	To serve all Git repositories contained within the '/git/'
+	subdirectory of the DocumentRoot, ensure mod_cgi and
+	mod_alias are enabled, and create a ScriptAlias to the CGI:
++
+----------------------------------------------------------------
+ScriptAlias /git/ /usr/libexec/git-core/git-http-backend/git/
+
+<Directory /usr/libexec/git-core>
+	Options None
+</Directory>
+<Files /usr/libexec/git-core/git-http-backend>
+	Options ExecCGI
+</Files>
+----------------------------------------------------------------
++
+To require authentication for reads, use a Directory
+directive around the repository, or one of its parent directories:
++
+----------------------------------------------------------------
+<Directory /var/www/git/private>
+	AuthType Basic
+	AuthName "Private Git Access"
+	Require group committers
+	...
+</Directory>
+----------------------------------------------------------------
+
+Accelerated static Apache 2.x::
+	Similar to the above, but Apache can be used to return static
+	files that are stored on disk.	On many systems this may
+	be more efficient as Apache can ask the kernel to copy the
+	file contents from the file system directly to the network:
++
+----------------------------------------------------------------
+DocumentRoot /var/www
+
+ScriptAlias /git/        /usr/libexec/git-core/git-http-backend/git/
+Alias       /git_static/ /var/www/git/
+
+RewriteEngine on
+RewriteRule ^/git/(.*/objects/[0-9a-f]{2}/[0-9a-f]{38})$    /git_static/$1 [PT]
+RewriteRule ^/git/(.*/objects/pack/pack-[0-9a-f]{40}.pack)$ /git_static/$1 [PT]
+RewriteRule ^/git/(.*/objects/pack/pack-[0-9a-f]{40}.idx)$  /git_static/$1 [PT]
+----------------------------------------------------------------
+
+
+ENVIRONMENT
+-----------
+'git-http-backend' relies upon the CGI environment variables set
+by the invoking web server, including:
+
+* PATH_TRANSLATED
+* REMOTE_USER
+* REMOTE_ADDR
+* CONTENT_TYPE
+* QUERY_STRING
+* REQUEST_METHOD
+
+Author
+------
+Written by Shawn O. Pearce <spearce@spearce.org>.
+
+Documentation
+--------------
+Documentation by Shawn O. Pearce <spearce@spearce.org>.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index fea237b..271c290 100644
--- a/Makefile
+++ b/Makefile
@@ -365,6 +365,7 @@ PROGRAMS += git-show-index$X
 PROGRAMS += git-unpack-file$X
 PROGRAMS += git-upload-pack$X
 PROGRAMS += git-var$X
+PROGRAMS += git-http-backend$X
 
 # List built-in command $C whose implementation cmd_$C() is not in
 # builtin-$C.o but is linked in as part of some other command.
diff --git a/http-backend.c b/http-backend.c
new file mode 100644
index 0000000..374f60d
--- /dev/null
+++ b/http-backend.c
@@ -0,0 +1,290 @@
+#include "cache.h"
+#include "refs.h"
+#include "pkt-line.h"
+#include "object.h"
+#include "tag.h"
+#include "exec_cmd.h"
+
+static const char content_type[] = "Content-Type";
+static const char content_length[] = "Content-Length";
+static const char last_modified[] = "Last-Modified";
+
+static void format_write(int fd, const char *fmt, ...)
+{
+	static char buffer[1024];
+
+	va_list args;
+	unsigned n;
+
+	va_start(args, fmt);
+	n = vsnprintf(buffer, sizeof(buffer), fmt, args);
+	va_end(args);
+	if (n >= sizeof(buffer))
+		die("protocol error: impossibly long line");
+
+	safe_write(fd, buffer, n);
+}
+
+static void http_status(unsigned code, const char *msg)
+{
+	format_write(1, "Status: %u %s\r\n", code, msg);
+}
+
+static void hdr_str(const char *name, const char *value)
+{
+	format_write(1, "%s: %s\r\n", name, value);
+}
+
+static void hdr_int(const char *name, size_t value)
+{
+	format_write(1, "%s: %" PRIuMAX "\r\n", name, value);
+}
+
+static void hdr_date(const char *name, unsigned long when)
+{
+	const char *value = show_date(when, 0, DATE_RFC2822);
+	hdr_str(name, value);
+}
+
+static void hdr_nocache(void)
+{
+	hdr_str("Expires", "Fri, 01 Jan 1980 00:00:00 GMT");
+	hdr_str("Pragma", "no-cache");
+	hdr_str("Cache-Control", "no-cache, max-age=0, must-revalidate");
+}
+
+static void hdr_cache_forever(void)
+{
+	unsigned long now = time(NULL);
+	hdr_date("Date", now);
+	hdr_date("Expires", now + 31536000);
+	hdr_str("Cache-Control", "public, max-age=31536000");
+}
+
+static void end_headers(void)
+{
+	safe_write(1, "\r\n", 2);
+}
+
+static NORETURN void not_found(const char *err, ...)
+{
+	va_list params;
+
+	http_status(404, "Not Found");
+	hdr_nocache();
+	end_headers();
+
+	va_start(params, err);
+	if (err && *err)
+		vfprintf(stderr, err, params);
+	va_end(params);
+	exit(0);
+}
+
+static void send_strbuf(const char *type, struct strbuf *buf)
+{
+	hdr_int(content_length, buf->len);
+	hdr_str(content_type, type);
+	end_headers();
+	safe_write(1, buf->buf, buf->len);
+}
+
+static void send_file(const char *the_type, const char *name)
+{
+	const char *p = git_path("%s", name);
+	size_t buf_alloc = 8192;
+	char *buf = xmalloc(buf_alloc);
+	int fd;
+	struct stat sb;
+	size_t size;
+
+	fd = open(p, O_RDONLY);
+	if (fd < 0)
+		not_found("Cannot open '%s': %s", p, strerror(errno));
+	if (fstat(fd, &sb) < 0)
+		die_errno("Cannot stat '%s'", p);
+
+	size = xsize_t(sb.st_size);
+
+	hdr_int(content_length, size);
+	hdr_str(content_type, the_type);
+	hdr_date(last_modified, sb.st_mtime);
+	end_headers();
+
+	while (size) {
+		ssize_t n = xread(fd, buf, buf_alloc);
+		if (n < 0)
+			die_errno("Cannot read '%s'", p);
+		if (!n)
+			break;
+		safe_write(1, buf, n);
+	}
+	close(fd);
+	free(buf);
+}
+
+static void get_text_file(char *name)
+{
+	hdr_nocache();
+	send_file("text/plain", name);
+}
+
+static void get_loose_object(char *name)
+{
+	hdr_cache_forever();
+	send_file("application/x-git-loose-object", name);
+}
+
+static void get_pack_file(char *name)
+{
+	hdr_cache_forever();
+	send_file("application/x-git-packed-objects", name);
+}
+
+static void get_idx_file(char *name)
+{
+	hdr_cache_forever();
+	send_file("application/x-git-packed-objects-toc", name);
+}
+
+static int show_text_ref(const char *name, const unsigned char *sha1,
+	int flag, void *cb_data)
+{
+	struct strbuf *buf = cb_data;
+	struct object *o = parse_object(sha1);
+	if (!o)
+		return 0;
+
+	strbuf_addf(buf, "%s\t%s\n", sha1_to_hex(sha1), name);
+	if (o->type == OBJ_TAG) {
+		o = deref_tag(o, name, 0);
+		if (!o)
+			return 0;
+		strbuf_addf(buf, "%s\t%s^{}\n", sha1_to_hex(o->sha1), name);
+	}
+	return 0;
+}
+
+static void get_info_refs(char *arg)
+{
+	struct strbuf buf = STRBUF_INIT;
+
+	for_each_ref(show_text_ref, &buf);
+	hdr_nocache();
+	send_strbuf("text/plain", &buf);
+	strbuf_release(&buf);
+}
+
+static void get_info_packs(char *arg)
+{
+	size_t objdirlen = strlen(get_object_directory());
+	struct strbuf buf = STRBUF_INIT;
+	struct packed_git *p;
+	size_t cnt = 0;
+
+	prepare_packed_git();
+	for (p = packed_git; p; p = p->next) {
+		if (p->pack_local)
+			cnt++;
+	}
+
+	strbuf_grow(&buf, cnt * 53 + 2);
+	for (p = packed_git; p; p = p->next) {
+		if (p->pack_local)
+			strbuf_addf(&buf, "P %s\n", p->pack_name + objdirlen + 6);
+	}
+	strbuf_addch(&buf, '\n');
+
+	hdr_nocache();
+	send_strbuf("text/plain; charset=utf-8", &buf);
+	strbuf_release(&buf);
+}
+
+static NORETURN void die_webcgi(const char *err, va_list params)
+{
+	char buffer[1000];
+
+	http_status(500, "Internal Server Error");
+	hdr_nocache();
+	end_headers();
+
+	vsnprintf(buffer, sizeof(buffer), err, params);
+	fprintf(stderr, "fatal: %s\n", buffer);
+	exit(0);
+}
+
+static struct service_cmd {
+	const char *method;
+	const char *pattern;
+	void (*imp)(char *);
+} services[] = {
+	{"GET", "/HEAD$", get_text_file},
+	{"GET", "/info/refs$", get_info_refs},
+	{"GET", "/objects/info/alternates$", get_text_file},
+	{"GET", "/objects/info/http-alternates$", get_text_file},
+	{"GET", "/objects/info/packs$", get_info_packs},
+	{"GET", "/objects/info/[^/]*$", get_text_file},
+	{"GET", "/objects/[0-9a-f]{2}/[0-9a-f]{38}$", get_loose_object},
+	{"GET", "/objects/pack/pack-[0-9a-f]{40}\\.pack$", get_pack_file},
+	{"GET", "/objects/pack/pack-[0-9a-f]{40}\\.idx$", get_idx_file}
+};
+
+int main(int argc, char **argv)
+{
+	char *method = getenv("REQUEST_METHOD");
+	char *dir = getenv("PATH_TRANSLATED");
+	struct service_cmd *cmd = NULL;
+	char *cmd_arg = NULL;
+	int i;
+
+	git_extract_argv0_path(argv[0]);
+	set_die_routine(die_webcgi);
+
+	if (!method)
+		die("No REQUEST_METHOD from server");
+	if (!strcmp(method, "HEAD"))
+		method = "GET";
+	if (!dir)
+		die("No PATH_TRANSLATED from server");
+
+	for (i = 0; i < ARRAY_SIZE(services); i++) {
+		struct service_cmd *c = &services[i];
+		regex_t re;
+		regmatch_t out[1];
+
+		if (regcomp(&re, c->pattern, REG_EXTENDED))
+			die("Bogus regex in service table: %s", c->pattern);
+		if (!regexec(&re, dir, 1, out, 0)) {
+			size_t n = out[0].rm_eo - out[0].rm_so;
+
+			if (strcmp(method, c->method)) {
+				const char *proto = getenv("SERVER_PROTOCOL");
+				if (proto && !strcmp(proto, "HTTP/1.1"))
+					http_status(405, "Method Not Allowed");
+				else
+					http_status(400, "Bad Request");
+				hdr_nocache();
+				end_headers();
+				return 0;
+			}
+
+			cmd = c;
+			cmd_arg = xmalloc(n);
+			strncpy(cmd_arg, dir + out[0].rm_so + 1, n);
+			cmd_arg[n] = '\0';
+			dir[out[0].rm_so] = 0;
+			break;
+		}
+		regfree(&re);
+	}
+
+	if (!cmd)
+		not_found("Request not supported: '%s'", dir);
+
+	setup_path();
+	if (!enter_repo(dir, 0))
+		not_found("Not a git repository: '%s'", dir);
+
+	cmd->imp(cmd_arg);
+	return 0;
+}
-- 
1.6.5.52.g0ff2e

[RFC PATCH v3 12/17] Add stateless RPC options to upload-pack, receive-pack

[Shawn O. Pearce]

When --stateless-rpc is passed as a command line parameter to
upload-pack or receive-pack the programs now assume they may
perform only a single read-write cycle with stdin and stdout.
This fits with the HTTP POST request processing model where a
program may read the request, write a response, and must exit.

When --advertise-refs is passed as a command line parameter only
the initial ref advertisement is output, and the program exits
immediately.  This fits with the HTTP GET request model, where
no request content is received but a response must be produced.

HTTP headers and/or environment are not processed here, but
instead are assumed to be handled by the program invoking
either service backend.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 builtin-receive-pack.c |   26 ++++++++++++++++++++------
 upload-pack.c          |   40 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 56 insertions(+), 10 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index b771fe9..70ff8c5 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -615,6 +615,8 @@ static void add_alternate_refs(void)
 
 int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 {
+	int advertise_refs = 0;
+	int stateless_rpc = 0;
 	int i;
 	char *dir = NULL;
 
@@ -623,7 +625,15 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		const char *arg = *argv++;
 
 		if (*arg == '-') {
-			/* Do flag handling here */
+			if (!strcmp(arg, "--advertise-refs")) {
+				advertise_refs = 1;
+				continue;
+			}
+			if (!strcmp(arg, "--stateless-rpc")) {
+				stateless_rpc = 1;
+				continue;
+			}
+
 			usage(receive_pack_usage);
 		}
 		if (dir)
@@ -652,12 +662,16 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		" report-status delete-refs ofs-delta " :
 		" report-status delete-refs ";
 
-	add_alternate_refs();
-	write_head_info();
-	clear_extra_refs();
+	if (advertise_refs || !stateless_rpc) {
+		add_alternate_refs();
+		write_head_info();
+		clear_extra_refs();
 
-	/* EOF */
-	packet_flush(1);
+		/* EOF */
+		packet_flush(1);
+	}
+	if (advertise_refs)
+		return 0;
 
 	read_head_info();
 	if (commands) {
diff --git a/upload-pack.c b/upload-pack.c
index f1dc3a3..70badcf 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -39,6 +39,8 @@ static unsigned int timeout;
  */
 static int use_sideband;
 static int debug_fd;
+static int advertise_refs;
+static int stateless_rpc;
 
 static void reset_timeout(void)
 {
@@ -509,6 +511,8 @@ static int get_common_commits(void)
 		if (!len) {
 			if (have_obj.nr == 0 || multi_ack)
 				packet_write(1, "NAK\n");
+			if (stateless_rpc)
+				exit(0);
 			continue;
 		}
 		strip(line, len);
@@ -710,12 +714,32 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 	return 0;
 }
 
+static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
+{
+	struct object *o = parse_object(sha1);
+	if (!o)
+		die("git upload-pack: cannot find object %s:", sha1_to_hex(sha1));
+	if (!(o->flags & OUR_REF)) {
+		o->flags |= OUR_REF;
+		nr_our_refs++;
+	}
+	return 0;
+}
+
 static void upload_pack(void)
 {
-	reset_timeout();
-	head_ref(send_ref, NULL);
-	for_each_ref(send_ref, NULL);
-	packet_flush(1);
+	if (advertise_refs || !stateless_rpc) {
+		reset_timeout();
+		head_ref(send_ref, NULL);
+		for_each_ref(send_ref, NULL);
+		packet_flush(1);
+	} else {
+		head_ref(mark_our_ref, NULL);
+		for_each_ref(mark_our_ref, NULL);
+	}
+	if (advertise_refs)
+		return;
+
 	receive_needs();
 	if (want_obj.nr) {
 		get_common_commits();
@@ -737,6 +761,14 @@ int main(int argc, char **argv)
 
 		if (arg[0] != '-')
 			break;
+		if (!strcmp(arg, "--advertise-refs")) {
+			advertise_refs = 1;
+			continue;
+		}
+		if (!strcmp(arg, "--stateless-rpc")) {
+			stateless_rpc = 1;
+			continue;
+		}
 		if (!strcmp(arg, "--strict")) {
 			strict = 1;
 			continue;
-- 
1.6.5.52.g0ff2e

[RFC PATCH v3 13/17] Smart fetch and push over HTTP: server side

[Shawn O. Pearce]

Requests for $GIT_URL/git-receive-pack and $GIT_URL/git-upload-pack
are forwarded to the corresponding backend process by directly
executing it and leaving stdin and stdout connected to the invoking
web server.  Prior to starting the backend process the HTTP response
headers are sent, thereby freeing the backend from needing to know
about the HTTP protocol.

Requests that are encoded with Content-Encoding: gzip are
automatically inflated before being streamed into the backend.
This is primarily useful for the git-upload-pack backend, which
receives highly repetitive text data from clients that easily
compresses to 50% of its original size.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 Documentation/git-http-backend.txt |   39 +++++-
 http-backend.c                     |  324 +++++++++++++++++++++++++++++++++++-
 2 files changed, 359 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-http-backend.txt b/Documentation/git-http-backend.txt
index 867675f..022a243 100644
--- a/Documentation/git-http-backend.txt
+++ b/Documentation/git-http-backend.txt
@@ -22,6 +22,23 @@ By default, only the `upload-pack` service is enabled, which serves
 This is ideally suited for read-only updates, i.e., pulling from
 git repositories.
 
+SERVICES
+--------
+These services can be enabled/disabled using the per-repository
+configuration file:
+
+http.uploadpack::
+	This serves 'git-fetch-pack' and 'git-ls-remote' clients.
+	It is enabled by default, but a repository can disable it
+	by setting this configuration item to `false`.
+
+http.receivepack::
+	This serves 'git-send-pack' clients, allowing push.  It is
+	disabled by default for anonymous users, and enabled by
+	default for users authenticated by the web server.  It can be
+	disabled by setting this item to `false`, or enabled for all
+	users, including anonymous users, by setting it to `true`.
+
 URL TRANSLATION
 ---------------
 'git-http-backend' relies on the invoking web server to perform
@@ -49,7 +66,19 @@ ScriptAlias /git/ /usr/libexec/git-core/git-http-backend/git/
 </Files>
 ----------------------------------------------------------------
 +
-To require authentication for reads, use a Directory
+To enable anonymous read access but authenticated write access,
+require authorization with a LocationMatch directive:
++
+----------------------------------------------------------------
+<LocationMatch ".*/git-receive-pack$">
+	AuthType Basic
+	AuthName "Git Access"
+	Require group committers
+	...
+</LocationMatch>
+----------------------------------------------------------------
++
+To require authentication for both reads and writes, use a Directory
 directive around the repository, or one of its parent directories:
 +
 ----------------------------------------------------------------
@@ -92,6 +121,14 @@ by the invoking web server, including:
 * QUERY_STRING
 * REQUEST_METHOD
 
+The backend process sets GIT_COMMITTER_NAME to '$REMOTE_USER' and
+GIT_COMMITTER_EMAIL to '$\{REMOTE_USER}@http.$\{REMOTE_ADDR\}',
+ensuring that any reflogs created by 'git-receive-pack' contain some
+identifying information of the remote user who performed the push.
+
+All CGI environment variables are available to each of the hooks
+invoked by the 'git-receive-pack'.
+
 Author
 ------
 Written by Shawn O. Pearce <spearce@spearce.org>.
diff --git a/http-backend.c b/http-backend.c
index 374f60d..67030b5 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -4,11 +4,109 @@
 #include "object.h"
 #include "tag.h"
 #include "exec_cmd.h"
+#include "run-command.h"
+#include "string-list.h"
 
 static const char content_type[] = "Content-Type";
 static const char content_length[] = "Content-Length";
 static const char last_modified[] = "Last-Modified";
 
+static struct string_list *query_params;
+
+struct rpc_service {
+	const char *name;
+	const char *config_name;
+	signed enabled : 2;
+};
+
+static struct rpc_service rpc_service[] = {
+	{ "upload-pack", "uploadpack", 1 },
+	{ "receive-pack", "receivepack", -1 },
+};
+
+static int decode_char(const char *q)
+{
+	int i;
+	unsigned char val = 0;
+	for (i = 0; i < 2; i++) {
+		unsigned char c = *q++;
+		val <<= 4;
+		if (c >= '0' && c <= '9')
+			val += c - '0';
+		else if (c >= 'a' && c <= 'f')
+			val += c - 'a' + 10;
+		else if (c >= 'A' && c <= 'F')
+			val += c - 'A' + 10;
+		else
+			return -1;
+	}
+	return val;
+}
+
+static char *decode_parameter(const char **query, int is_name)
+{
+	const char *q = *query;
+	struct strbuf out;
+
+	strbuf_init(&out, 16);
+	do {
+		unsigned char c = *q;
+
+		if (!c)
+			break;
+		if (c == '&' || (is_name && c == '=')) {
+			q++;
+			break;
+		}
+
+		if (c == '%') {
+			int val = decode_char(q + 1);
+			if (0 <= val) {
+				strbuf_addch(&out, val);
+				q += 3;
+				continue;
+			}
+		}
+
+		if (c == '+')
+			strbuf_addch(&out, ' ');
+		else
+			strbuf_addch(&out, c);
+		q++;
+	} while (1);
+	*query = q;
+	return strbuf_detach(&out, NULL);
+}
+
+static struct string_list *get_parameters(void)
+{
+	if (!query_params) {
+		const char *query = getenv("QUERY_STRING");
+
+		query_params = xcalloc(1, sizeof(*query_params));
+		while (query && *query) {
+			char *name = decode_parameter(&query, 1);
+			char *value = decode_parameter(&query, 0);
+			struct string_list_item *i;
+
+			i = string_list_lookup(name, query_params);
+			if (!i)
+				i = string_list_insert(name, query_params);
+			else
+				free(i->util);
+			i->util = value;
+		}
+	}
+	return query_params;
+}
+
+static const char *get_parameter(const char *name)
+{
+	struct string_list_item *i;
+	i = string_list_lookup(name, get_parameters());
+	return i ? i->util : NULL;
+}
+
 static void format_write(int fd, const char *fmt, ...)
 {
 	static char buffer[1024];
@@ -81,6 +179,21 @@ static NORETURN void not_found(const char *err, ...)
 	exit(0);
 }
 
+static NORETURN void forbidden(const char *err, ...)
+{
+	va_list params;
+
+	http_status(403, "Forbidden");
+	hdr_nocache();
+	end_headers();
+
+	va_start(params, err);
+	if (err && *err)
+		vfprintf(stderr, err, params);
+	va_end(params);
+	exit(0);
+}
+
 static void send_strbuf(const char *type, struct strbuf *buf)
 {
 	hdr_int(content_length, buf->len);
@@ -147,6 +260,145 @@ static void get_idx_file(char *name)
 	send_file("application/x-git-packed-objects-toc", name);
 }
 
+static int http_config(const char *var, const char *value, void *cb)
+{
+	struct rpc_service *svc = cb;
+
+	if (!prefixcmp(var, "http.") &&
+	    !strcmp(var + 5, svc->config_name)) {
+		svc->enabled = git_config_bool(var, value);
+		return 0;
+	}
+
+	/* we are not interested in parsing any other configuration here */
+	return 0;
+}
+
+static struct rpc_service *select_service(const char *name)
+{
+	struct rpc_service *svc = NULL;
+	int i;
+
+	if (prefixcmp(name, "git-"))
+		forbidden("Unsupported service: '%s'", name);
+
+	for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {
+		struct rpc_service *s = &rpc_service[i];
+		if (!strcmp(s->name, name + 4)) {
+			svc = s;
+			break;
+		}
+	}
+
+	if (!svc)
+		forbidden("Unsupported service: '%s'", name);
+
+	git_config(http_config, svc);
+	if (svc->enabled < 0) {
+		const char *user = getenv("REMOTE_USER");
+		svc->enabled = (user && *user) ? 1 : 0;
+	}
+	if (!svc->enabled)
+		forbidden("Service not enabled: '%s'", svc->name);
+	return svc;
+}
+
+static void inflate_request(const char *prog_name, int out)
+{
+	z_stream stream;
+	unsigned char in_buf[8192];
+	unsigned char out_buf[8192];
+	unsigned long cnt = 0;
+	int ret;
+
+	memset(&stream, 0, sizeof(stream));
+	ret = inflateInit2(&stream, (15 + 16));
+	if (ret != Z_OK)
+		die("cannot start zlib inflater, zlib err %d", ret);
+
+	while (1) {
+		ssize_t n = xread(0, in_buf, sizeof(in_buf));
+		if (n <= 0)
+			die("request ended in the middle of the gzip stream");
+
+		stream.next_in = in_buf;
+		stream.avail_in = n;
+
+		while (0 < stream.avail_in) {
+			int ret;
+
+			stream.next_out = out_buf;
+			stream.avail_out = sizeof(out_buf);
+
+			ret = inflate(&stream, Z_NO_FLUSH);
+			if (ret != Z_OK && ret != Z_STREAM_END)
+				die("zlib error inflating request, result %d", ret);
+
+			n = stream.total_out - cnt;
+			if (write_in_full(out, out_buf, n) != n)
+				die("%s aborted reading request", prog_name);
+			cnt += n;
+
+			if (ret == Z_STREAM_END)
+				goto done;
+		}
+	}
+
+done:
+	inflateEnd(&stream);
+	close(out);
+}
+
+static void run_service(const char **argv)
+{
+	const char *encoding = getenv("HTTP_CONTENT_ENCODING");
+	const char *user = getenv("REMOTE_USER");
+	const char *host = getenv("REMOTE_ADDR");
+	char *env[3];
+	struct strbuf buf = STRBUF_INIT;
+	int gzipped_request = 0;
+	struct child_process cld;
+
+	if (encoding && !strcmp(encoding, "gzip"))
+		gzipped_request = 1;
+	else if (encoding && !strcmp(encoding, "x-gzip"))
+		gzipped_request = 1;
+
+	if (!user || !*user)
+		user = "anonymous";
+	if (!host || !*host)
+		host = "(none)";
+
+	memset(&env, 0, sizeof(env));
+	strbuf_addf(&buf, "GIT_COMMITTER_NAME=%s", user);
+	env[0] = strbuf_detach(&buf, NULL);
+
+	strbuf_addf(&buf, "GIT_COMMITTER_EMAIL=%s@http.%s", user, host);
+	env[1] = strbuf_detach(&buf, NULL);
+	env[2] = NULL;
+
+	memset(&cld, 0, sizeof(cld));
+	cld.argv = argv;
+	cld.env = (const char *const *)env;
+	if (gzipped_request)
+		cld.in = -1;
+	cld.git_cmd = 1;
+	if (start_command(&cld))
+		exit(1);
+
+	close(1);
+	if (gzipped_request)
+		inflate_request(argv[0], cld.in);
+	else
+		close(0);
+
+	if (finish_command(&cld))
+		exit(1);
+	free(env[0]);
+	free(env[1]);
+	strbuf_release(&buf);
+}
+
 static int show_text_ref(const char *name, const unsigned char *sha1,
 	int flag, void *cb_data)
 {
@@ -167,11 +419,32 @@ static int show_text_ref(const char *name, const unsigned char *sha1,
 
 static void get_info_refs(char *arg)
 {
+	const char *service_name = get_parameter("service");
 	struct strbuf buf = STRBUF_INIT;
 
-	for_each_ref(show_text_ref, &buf);
 	hdr_nocache();
-	send_strbuf("text/plain", &buf);
+
+	if (service_name) {
+		const char *argv[] = {NULL /* service name */,
+			"--stateless-rpc", "--advertise-refs",
+			".", NULL};
+		struct rpc_service *svc = select_service(service_name);
+
+		strbuf_addf(&buf, "application/x-git-%s-advertisement",
+			svc->name);
+		hdr_str(content_type, buf.buf);
+		end_headers();
+
+		packet_write(1, "# service=git-%s\n", svc->name);
+		packet_flush(1);
+
+		argv[0] = svc->name;
+		run_service(argv);
+
+	} else {
+		for_each_ref(show_text_ref, &buf);
+		send_strbuf("text/plain", &buf);
+	}
 	strbuf_release(&buf);
 }
 
@@ -200,6 +473,48 @@ static void get_info_packs(char *arg)
 	strbuf_release(&buf);
 }
 
+static void check_content_type(const char *accepted_type)
+{
+	const char *actual_type = getenv("CONTENT_TYPE");
+
+	if (!actual_type)
+		actual_type = "";
+
+	if (strcmp(actual_type, accepted_type)) {
+		http_status(415, "Unsupported Media Type");
+		hdr_nocache();
+		end_headers();
+		format_write(1,
+			"Expected POST with Content-Type '%s',"
+			" but received '%s' instead.\n",
+			accepted_type, actual_type);
+		exit(0);
+	}
+}
+
+static void service_rpc(char *service_name)
+{
+	const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
+	struct rpc_service *svc = select_service(service_name);
+	struct strbuf buf = STRBUF_INIT;
+
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, "application/x-git-%s-request", svc->name);
+	check_content_type(buf.buf);
+
+	hdr_nocache();
+
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, "application/x-git-%s-result", svc->name);
+	hdr_str(content_type, buf.buf);
+
+	end_headers();
+
+	argv[0] = svc->name;
+	run_service(argv);
+	strbuf_release(&buf);
+}
+
 static NORETURN void die_webcgi(const char *err, va_list params)
 {
 	char buffer[1000];
@@ -226,7 +541,10 @@ static struct service_cmd {
 	{"GET", "/objects/info/[^/]*$", get_text_file},
 	{"GET", "/objects/[0-9a-f]{2}/[0-9a-f]{38}$", get_loose_object},
 	{"GET", "/objects/pack/pack-[0-9a-f]{40}\\.pack$", get_pack_file},
-	{"GET", "/objects/pack/pack-[0-9a-f]{40}\\.idx$", get_idx_file}
+	{"GET", "/objects/pack/pack-[0-9a-f]{40}\\.idx$", get_idx_file},
+
+	{"POST", "/git-upload-pack$", service_rpc},
+	{"POST", "/git-receive-pack$", service_rpc}
 };
 
 int main(int argc, char **argv)
-- 
1.6.5.52.g0ff2e

[RFC PATCH v3 14/17] Discover refs via smart HTTP server when available

[Shawn O. Pearce]

Instead of loading the cached info/refs, try to use the smart HTTP
version when the server supports it.  Since the smart variant is
actually the pkt-line stream from the start of either upload-pack
or receive-pack we need to parse these through get_remote_heads,
which requires a background thread to feed its pipe.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
CC: Daniel Barkalow <barkalow@iabervon.org>
---
 remote-curl.c |  149 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 131 insertions(+), 18 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index af2fddf..c6e3172 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -5,6 +5,7 @@
 #include "http.h"
 #include "exec_cmd.h"
 #include "run-command.h"
+#include "pkt-line.h"
 
 static struct remote *remote;
 static const char *url;
@@ -75,21 +76,46 @@ static int set_option(const char *name, const char *value)
 	}
 }
 
-static struct ref *get_refs(void)
+struct discovery {
+	const char *service;
+	char *buf_alloc;
+	char *buf;
+	size_t len;
+	unsigned proto_git : 1;
+};
+static struct discovery *last_discovery;
+
+static void free_discovery(struct discovery *d)
+{
+	if (d) {
+		if (d == last_discovery)
+			last_discovery = NULL;
+		free(d->buf_alloc);
+		free(d);
+	}
+}
+
+static struct discovery* discover_refs(const char *service)
 {
 	struct strbuf buffer = STRBUF_INIT;
-	char *data, *start, *mid;
-	char *ref_name;
+	struct discovery *last = last_discovery;
 	char *refs_url;
-	int i = 0;
-	int http_ret;
+	int http_ret, is_http = 0;
 
-	struct ref *refs = NULL;
-	struct ref *ref = NULL;
-	struct ref *last_ref = NULL;
+	if (last && !strcmp(service, last->service))
+		return last;
+	free_discovery(last);
 
-	refs_url = xmalloc(strlen(url) + 11);
-	sprintf(refs_url, "%s/info/refs", url);
+	strbuf_addf(&buffer, "%s/info/refs", url);
+	if (!prefixcmp(url, "http://") || !prefixcmp(url, "https://")) {
+		is_http = 1;
+		if (!strchr(url, '?'))
+			strbuf_addch(&buffer, '?');
+		else
+			strbuf_addch(&buffer, '&');
+		strbuf_addf(&buffer, "service=%s", service);
+	}
+	refs_url = strbuf_detach(&buffer, NULL);
 
 	init_walker();
 	http_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);
@@ -104,10 +130,86 @@ static struct ref *get_refs(void)
 		die("HTTP request failed");
 	}
 
-	data = buffer.buf;
+	last= xcalloc(1, sizeof(*last_discovery));
+	last->service = service;
+	last->buf_alloc = strbuf_detach(&buffer, &last->len);
+	last->buf = last->buf_alloc;
+
+	if (is_http && 5 <= last->len && last->buf[4] == '#') {
+		/* smart HTTP response; validate that the service
+		 * pkt-line matches our request.
+		 */
+		struct strbuf exp = STRBUF_INIT;
+
+		if (packet_get_line(&buffer, &last->buf, &last->len) <= 0)
+			die("%s has invalid packet header", refs_url);
+		if (buffer.len && buffer.buf[buffer.len - 1] == '\n')
+			strbuf_setlen(&buffer, buffer.len - 1);
+
+		strbuf_addf(&exp, "# service=%s", service);
+		if (strbuf_cmp(&exp, &buffer))
+			die("invalid server response; got '%s'", buffer.buf);
+		strbuf_release(&exp);
+
+		/* The header can include additional metadata lines, up
+		 * until a packet flush marker.  Ignore these now, but
+		 * in the future we might start to scan them.
+		 */
+		strbuf_reset(&buffer);
+		while (packet_get_line(&buffer, &last->buf, &last->len) > 0)
+			strbuf_reset(&buffer);
+
+		last->proto_git = 1;
+	}
+
+	free(refs_url);
+	strbuf_release(&buffer);
+	last_discovery = last;
+	return last;
+}
+
+static int write_discovery(int fd, void *data)
+{
+	struct discovery *heads = data;
+	int err = 0;
+	if (write_in_full(fd, heads->buf, heads->len) != heads->len)
+		err = 1;
+	close(fd);
+	return err;
+}
+
+static struct ref *parse_git_refs(struct discovery *heads)
+{
+	struct ref *list = NULL;
+	struct async async;
+
+	memset(&async, 0, sizeof(async));
+	async.proc = write_discovery;
+	async.data = heads;
+
+	if (start_async(&async))
+		die("cannot start thread to parse advertised refs");
+	get_remote_heads(async.out, &list, 0, NULL, 0, NULL);
+	close(async.out);
+	if (finish_async(&async))
+		die("ref parsing thread failed");
+	return list;
+}
+
+static struct ref *parse_info_refs(struct discovery *heads)
+{
+	char *data, *start, *mid;
+	char *ref_name;
+	int i = 0;
+
+	struct ref *refs = NULL;
+	struct ref *ref = NULL;
+	struct ref *last_ref = NULL;
+
+	data = heads->buf;
 	start = NULL;
 	mid = data;
-	while (i < buffer.len) {
+	while (i < heads->len) {
 		if (!start) {
 			start = &data[i];
 		}
@@ -131,8 +233,7 @@ static struct ref *get_refs(void)
 		i++;
 	}
 
-	strbuf_release(&buffer);
-
+	init_walker();
 	ref = alloc_ref("HEAD");
 	if (!walker->fetch_ref(walker, ref) &&
 	    !resolve_remote_symref(ref, refs)) {
@@ -142,11 +243,23 @@ static struct ref *get_refs(void)
 		free(ref);
 	}
 
-	strbuf_release(&buffer);
-	free(refs_url);
 	return refs;
 }
 
+static struct ref *get_refs(int for_push)
+{
+	struct discovery *heads;
+
+	if (for_push)
+		heads = discover_refs("git-receive-pack");
+	else
+		heads = discover_refs("git-upload-pack");
+
+	if (heads->proto_git)
+		return parse_git_refs(heads);
+	return parse_info_refs(heads);
+}
+
 static void output_refs(struct ref *refs)
 {
 	struct ref *posn;
@@ -317,10 +430,10 @@ int main(int argc, const char **argv)
 			parse_fetch(&buf);
 
 		} else if (!strcmp(buf.buf, "list")) {
-			output_refs(get_refs());
+			output_refs(get_refs(0));
 
 		} else if (!strcmp(buf.buf, "list for-push")) {
-			output_refs(get_refs());
+			output_refs(get_refs(1));
 
 		} else if (!prefixcmp(buf.buf, "push ")) {
 			parse_push(&buf);
-- 
1.6.5.52.g0ff2e

[RFC PATCH v3 15/17] Smart push over HTTP: client side

[Shawn O. Pearce]

The git-remote-curl backend detects if the remote server supports
the git-receive-pack service, and if so, runs git-send-pack in a
pipe to dump the command and pack data as a single POST request.

The advertisements from the server that were obtained during the
discovery are passed into git-send-pack before the POST request
starts.  This permits git-send-pack to operate largely unmodified.

For smaller packs (those under 1 MiB) a HTTP/1.0 POST with a
Content-Length is used, permitting interaction with any server.
The 1 MiB limit is arbitrary, but is sufficent to fit most deltas
created by human authors against text sources with the occasional
small binary file (e.g. few KiB icon image).  The configuration
option http.postBuffer can be used to increase (or shink) this
buffer if the default is not sufficient.

For larger packs which cannot be spooled entirely into the helper's
memory space (due to http.postBuffer being too small), the POST
request requires HTTP/1.1 and sets "Transfer-Encoding: chunked".
This permits the client to upload an unknown amount of data in one
HTTP transaction without needing to pregenerate the entire pack
file locally.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
CC: Daniel Barkalow <barkalow@iabervon.org>
---
 Documentation/config.txt |    8 ++
 builtin-send-pack.c      |  116 ++++++++++++++++++++++-
 http.c                   |    9 ++
 http.h                   |    1 +
 remote-curl.c            |  234 +++++++++++++++++++++++++++++++++++++++++++++-
 send-pack.h              |    3 +-
 sideband.c               |   11 ++-
 transport.c              |    1 +
 8 files changed, 371 insertions(+), 12 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index cd17814..7130d07 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1089,6 +1089,14 @@ http.maxRequests::
 	How many HTTP requests to launch in parallel. Can be overridden
 	by the 'GIT_HTTP_MAX_REQUESTS' environment variable. Default is 5.
 
+http.postBuffer::
+	Maximum size in bytes of the buffer used by smart HTTP
+	transports when POSTing data to the remote system.
+	For requests larger than this buffer size, HTTP/1.1 and
+	Transfer-Encoding: chunked is used to avoid creating a
+	massive pack file locally.  Default is 1 MiB, which is
+	sufficient for most requests.
+
 http.lowSpeedLimit, http.lowSpeedTime::
 	If the HTTP transfer speed is less than 'http.lowSpeedLimit'
 	for longer than 'http.lowSpeedTime' seconds, the transfer is aborted.
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 37e528e..a0fbad0 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -2,9 +2,11 @@
 #include "commit.h"
 #include "refs.h"
 #include "pkt-line.h"
+#include "sideband.h"
 #include "run-command.h"
 #include "remote.h"
 #include "send-pack.h"
+#include "quote.h"
 
 static const char send_pack_usage[] =
 "git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [<host>:]<directory> [<ref>...]\n"
@@ -59,7 +61,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 	memset(&po, 0, sizeof(po));
 	po.argv = argv;
 	po.in = -1;
-	po.out = fd;
+	po.out = args->stateless_rpc ? -1 : fd;
 	po.git_cmd = 1;
 	if (start_command(&po))
 		die_errno("git pack-objects failed");
@@ -83,6 +85,20 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 	}
 
 	close(po.in);
+
+	if (args->stateless_rpc) {
+		char *buf = xmalloc(LARGE_PACKET_MAX);
+		while (1) {
+			ssize_t n = xread(po.out, buf, LARGE_PACKET_MAX);
+			if (n <= 0)
+				break;
+			send_sideband(fd, -1, buf, n, LARGE_PACKET_MAX);
+		}
+		free(buf);
+		close(po.out);
+		po.out = -1;
+	}
+
 	if (finish_command(&po))
 		return error("pack-objects died with strange error");
 	return 0;
@@ -303,6 +319,59 @@ static int refs_pushed(struct ref *ref)
 	return 0;
 }
 
+static void print_helper_status(struct ref *ref)
+{
+	struct strbuf buf = STRBUF_INIT;
+
+	for (; ref; ref = ref->next) {
+		const char *msg = NULL;
+		const char *res;
+
+		switch(ref->status) {
+		case REF_STATUS_NONE:
+			res = "error";
+			msg = "no match";
+			break;
+
+		case REF_STATUS_OK:
+			res = "ok";
+			break;
+
+		case REF_STATUS_UPTODATE:
+			res = "ok";
+			msg = "up to date";
+			break;
+
+		case REF_STATUS_REJECT_NONFASTFORWARD:
+			res = "error";
+			msg = "non-fast forward";
+			break;
+
+		case REF_STATUS_REJECT_NODELETE:
+		case REF_STATUS_REMOTE_REJECT:
+			res = "error";
+			break;
+
+		case REF_STATUS_EXPECTING_REPORT:
+		default:
+			continue;
+		}
+
+		strbuf_reset(&buf);
+		strbuf_addf(&buf, "%s %s", res, ref->name);
+		if (ref->remote_status)
+			msg = ref->remote_status;
+		if (msg) {
+			strbuf_addch(&buf, ' ');
+			quote_two_c_style(&buf, "", msg, 0);
+		}
+		strbuf_addch(&buf, '\n');
+
+		safe_write(1, buf.buf, buf.len);
+	}
+	strbuf_release(&buf);
+}
+
 int send_pack(struct send_pack_args *args,
 	      int fd[], struct child_process *conn,
 	      struct ref *remote_refs,
@@ -310,6 +379,7 @@ int send_pack(struct send_pack_args *args,
 {
 	int in = fd[0];
 	int out = fd[1];
+	struct strbuf req_buf = STRBUF_INIT;
 	struct ref *ref;
 	int new_refs;
 	int ask_for_status_report = 0;
@@ -391,14 +461,14 @@ int send_pack(struct send_pack_args *args,
 			char *new_hex = sha1_to_hex(ref->new_sha1);
 
 			if (ask_for_status_report) {
-				packet_write(out, "%s %s %s%c%s",
+				packet_buf_write(&req_buf, "%s %s %s%c%s",
 					old_hex, new_hex, ref->name, 0,
 					"report-status");
 				ask_for_status_report = 0;
 				expect_status_report = 1;
 			}
 			else
-				packet_write(out, "%s %s %s",
+				packet_buf_write(&req_buf, "%s %s %s",
 					old_hex, new_hex, ref->name);
 		}
 		ref->status = expect_status_report ?
@@ -406,7 +476,17 @@ int send_pack(struct send_pack_args *args,
 			REF_STATUS_OK;
 	}
 
-	packet_flush(out);
+	if (args->stateless_rpc) {
+		if (!args->dry_run) {
+			packet_buf_flush(&req_buf);
+			send_sideband(out, -1, req_buf.buf, req_buf.len, LARGE_PACKET_MAX);
+		}
+	} else {
+		safe_write(out, req_buf.buf, req_buf.len);
+		packet_flush(out);
+	}
+	strbuf_release(&req_buf);
+
 	if (new_refs && !args->dry_run) {
 		if (pack_objects(out, remote_refs, extra_have, args) < 0) {
 			for (ref = remote_refs; ref; ref = ref->next)
@@ -414,11 +494,15 @@ int send_pack(struct send_pack_args *args,
 			return -1;
 		}
 	}
+	if (args->stateless_rpc && !args->dry_run)
+		packet_flush(out);
 
 	if (expect_status_report)
 		ret = receive_status(in, remote_refs);
 	else
 		ret = 0;
+	if (args->stateless_rpc)
+		packet_flush(out);
 
 	if (ret < 0)
 		return ret;
@@ -478,6 +562,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	struct extra_have_objects extra_have;
 	struct ref *remote_refs, *local_refs;
 	int ret;
+	int helper_status = 0;
 	int send_all = 0;
 	const char *receivepack = "git-receive-pack";
 	int flags;
@@ -523,6 +608,14 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 				args.use_thin_pack = 1;
 				continue;
 			}
+			if (!strcmp(arg, "--stateless-rpc")) {
+				args.stateless_rpc = 1;
+				continue;
+			}
+			if (!strcmp(arg, "--helper-status")) {
+				helper_status = 1;
+				continue;
+			}
 			usage(send_pack_usage);
 		}
 		if (!dest) {
@@ -551,7 +644,14 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	conn = git_connect(fd, dest, receivepack, args.verbose ? CONNECT_VERBOSE : 0);
+	if (args.stateless_rpc) {
+		conn = NULL;
+		fd[0] = 0;
+		fd[1] = 1;
+	} else {
+		conn = git_connect(fd, dest, receivepack,
+			args.verbose ? CONNECT_VERBOSE : 0);
+	}
 
 	memset(&extra_have, 0, sizeof(extra_have));
 
@@ -575,12 +675,16 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 
 	ret = send_pack(&args, fd, conn, remote_refs, &extra_have);
 
+	if (helper_status)
+		print_helper_status(remote_refs);
+
 	close(fd[1]);
 	close(fd[0]);
 
 	ret |= finish_connect(conn);
 
-	print_push_status(dest, remote_refs);
+	if (!helper_status)
+		print_push_status(dest, remote_refs);
 
 	if (!args.dry_run && remote) {
 		struct ref *ref;
diff --git a/http.c b/http.c
index 23b2a19..07e188a 100644
--- a/http.c
+++ b/http.c
@@ -1,9 +1,11 @@
 #include "http.h"
 #include "pack.h"
+#include "sideband.h"
 
 int data_received;
 int active_requests;
 int http_is_verbose;
+size_t http_post_buffer = 16 * LARGE_PACKET_MAX;
 
 #ifdef USE_CURL_MULTI
 static int max_requests = -1;
@@ -174,6 +176,13 @@ static int http_options(const char *var, const char *value, void *cb)
 	if (!strcmp("http.proxy", var))
 		return git_config_string(&curl_http_proxy, var, value);
 
+	if (!strcmp("http.postbuffer", var)) {
+		http_post_buffer = git_config_int(var, value);
+		if (http_post_buffer < LARGE_PACKET_MAX)
+			http_post_buffer = LARGE_PACKET_MAX;
+		return 0;
+	}
+
 	/* Fall back on the default ones */
 	return git_default_config(var, value, cb);
 }
diff --git a/http.h b/http.h
index 4c4e99c..1b0562f 100644
--- a/http.h
+++ b/http.h
@@ -94,6 +94,7 @@ extern void http_cleanup(void);
 extern int data_received;
 extern int active_requests;
 extern int http_is_verbose;
+extern size_t http_post_buffer;
 
 extern char curl_errorstr[CURL_ERROR_SIZE];
 
diff --git a/remote-curl.c b/remote-curl.c
index c6e3172..45436d1 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -6,6 +6,7 @@
 #include "exec_cmd.h"
 #include "run-command.h"
 #include "pkt-line.h"
+#include "sideband.h"
 
 static struct remote *remote;
 static const char *url;
@@ -16,7 +17,8 @@ struct options {
 	unsigned long depth;
 	unsigned progress : 1,
 		followtags : 1,
-		dry_run : 1;
+		dry_run : 1,
+		thin : 1;
 };
 static struct options options;
 
@@ -274,6 +276,187 @@ static void output_refs(struct ref *refs)
 	free_refs(refs);
 }
 
+struct rpc_state {
+	const char *service_name;
+	const char **argv;
+	char *service_url;
+	char *hdr_content_type;
+	char *hdr_accept;
+	char *buf;
+	size_t alloc;
+	size_t len;
+	size_t pos;
+	int in;
+	int out;
+	struct strbuf result;
+};
+
+static size_t rpc_out(void *ptr, size_t eltsize,
+		size_t nmemb, void *buffer_)
+{
+	size_t max = eltsize * nmemb;
+	struct rpc_state *rpc = buffer_;
+	size_t avail = rpc->len - rpc->pos;
+
+	if (!avail) {
+		avail = packet_read_line(rpc->out, rpc->buf, rpc->alloc);
+		if (!avail)
+			return 0;
+		rpc->pos = 0;
+		rpc->len = avail;
+	}
+
+	if (max < avail);
+		avail = max;
+	memcpy(ptr, rpc->buf + rpc->pos, avail);
+	rpc->pos += avail;
+	return avail;
+}
+
+static size_t rpc_in(const void *ptr, size_t eltsize,
+		size_t nmemb, void *buffer_)
+{
+	size_t size = eltsize * nmemb;
+	struct rpc_state *rpc = buffer_;
+	write_or_die(rpc->in, ptr, size);
+	return size;
+}
+
+static int post_rpc(struct rpc_state *rpc)
+{
+	struct active_request_slot *slot;
+	struct slot_results results;
+	struct curl_slist *headers = NULL;
+	int err = 0, large_request = 0;
+
+	/* Try to load the entire request, if we can fit it into the
+	 * allocated buffer space we can use HTTP/1.0 and avoid the
+	 * chunked encoding mess.
+	 */
+	while (1) {
+		size_t left = rpc->alloc - rpc->len;
+		char *buf = rpc->buf + rpc->len;
+		int n;
+
+		if (left < LARGE_PACKET_MAX) {
+			large_request = 1;
+			break;
+		}
+
+		n = packet_read_line(rpc->out, buf, left);
+		if (!n)
+			break;
+		rpc->len += n;
+	}
+
+	slot = get_active_slot();
+	slot->results = &results;
+
+	curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
+	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
+	curl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);
+
+	headers = curl_slist_append(headers, rpc->hdr_content_type);
+	headers = curl_slist_append(headers, rpc->hdr_accept);
+
+	if (large_request) {
+		/* The request body is large and the size cannot be predicted.
+		 * We must use chunked encoding to send it.
+		 */
+		headers = curl_slist_append(headers, "Expect: 100-continue");
+		headers = curl_slist_append(headers, "Transfer-Encoding: chunked");
+		curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
+		curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);
+		if (options.verbosity > 1) {
+			fprintf(stderr, "POST %s (chunked)\n", rpc->service_name);
+			fflush(stderr);
+		}
+
+	} else {
+		/* We know the complete request size in advance, use the
+		 * more normal Content-Length approach.
+		 */
+		curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, rpc->buf);
+		curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, rpc->len);
+		if (options.verbosity > 1) {
+			fprintf(stderr, "POST %s (%lu bytes)\n",
+				rpc->service_name, (unsigned long)rpc->len);
+			fflush(stderr);
+		}
+	}
+
+	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
+	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
+	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
+
+	if (start_active_slot(slot)) {
+		run_active_slot(slot);
+		if (results.curl_result != CURLE_OK) {
+			err |= error("RPC failed; result=%d, HTTP code = %ld",
+				results.curl_result, results.http_code);
+		}
+	}
+	curl_slist_free_all(headers);
+	return err;
+}
+
+static int rpc_service(struct rpc_state *rpc, struct discovery *heads)
+{
+	const char *svc = rpc->service_name;
+	struct strbuf buf = STRBUF_INIT;
+	struct child_process client;
+	int err = 0;
+
+	init_walker();
+	memset(&client, 0, sizeof(client));
+	client.in = -1;
+	client.out = -1;
+	client.git_cmd = 1;
+	client.argv = rpc->argv;
+	if (start_command(&client))
+		exit(1);
+	if (heads)
+		write_or_die(client.in, heads->buf, heads->len);
+
+	rpc->alloc = http_post_buffer;
+	rpc->buf = xmalloc(rpc->alloc);
+	rpc->in = client.in;
+	rpc->out = client.out;
+	strbuf_init(&rpc->result, 0);
+
+	strbuf_addf(&buf, "%s/%s", url, svc);
+	rpc->service_url = strbuf_detach(&buf, NULL);
+
+	strbuf_addf(&buf, "Content-Type: application/x-%s-request", svc);
+	rpc->hdr_content_type = strbuf_detach(&buf, NULL);
+
+	strbuf_addf(&buf, "Accept: application/x-%s-response", svc);
+	rpc->hdr_accept = strbuf_detach(&buf, NULL);
+
+	while (!err) {
+		int n = packet_read_line(rpc->out, rpc->buf, rpc->alloc);
+		if (!n)
+			break;
+		rpc->pos = 0;
+		rpc->len = n;
+		err |= post_rpc(rpc);
+	}
+	strbuf_read(&rpc->result, client.out, 0);
+
+	close(client.in);
+	close(client.out);
+	client.in = -1;
+	client.out = -1;
+
+	err |= finish_command(&client);
+	free(rpc->service_url);
+	free(rpc->hdr_content_type);
+	free(rpc->hdr_accept);
+	free(rpc->buf);
+	strbuf_release(&buf);
+	return err;
+}
+
 static int fetch_dumb(int nr_heads, struct ref **to_fetch)
 {
 	char **targets = xmalloc(nr_heads * sizeof(char*));
@@ -371,6 +554,52 @@ static int push_dav(int nr_spec, char **specs)
 	return 0;
 }
 
+static int push_git(struct discovery *heads, int nr_spec, char **specs)
+{
+	struct rpc_state rpc;
+	const char **argv;
+	int argc = 0, i, err;
+
+	argv = xmalloc((10 + nr_spec) * sizeof(char*));
+	argv[argc++] = "send-pack";
+	argv[argc++] = "--stateless-rpc";
+	argv[argc++] = "--helper-status";
+	if (options.thin)
+		argv[argc++] = "--thin";
+	if (options.dry_run)
+		argv[argc++] = "--dry-run";
+	if (options.verbosity > 1)
+		argv[argc++] = "--verbose";
+	argv[argc++] = url;
+	for (i = 0; i < nr_spec; i++)
+		argv[argc++] = specs[i];
+	argv[argc++] = NULL;
+
+	memset(&rpc, 0, sizeof(rpc));
+	rpc.service_name = "git-receive-pack",
+	rpc.argv = argv;
+
+	err = rpc_service(&rpc, heads);
+	if (rpc.result.len)
+		safe_write(1, rpc.result.buf, rpc.result.len);
+	strbuf_release(&rpc.result);
+	free(argv);
+	return err;
+}
+
+static int push(int nr_spec, char **specs)
+{
+	struct discovery *heads = discover_refs("git-receive-pack");
+	int ret;
+
+	if (heads->proto_git)
+		ret = push_git(heads, nr_spec, specs);
+	else
+		ret = push_dav(nr_spec, specs);
+	free_discovery(heads);
+	return ret;
+}
+
 static void parse_push(struct strbuf *buf)
 {
 	char **specs = NULL;
@@ -391,7 +620,7 @@ static void parse_push(struct strbuf *buf)
 			break;
 	} while (1);
 
-	if (push_dav(nr_spec, specs))
+	if (push(nr_spec, specs))
 		exit(128); /* error already reported */
 	for (i = 0; i < nr_spec; i++)
 		free(specs[i]);
@@ -414,6 +643,7 @@ int main(int argc, const char **argv)
 
 	options.verbosity = 1;
 	options.progress = !!isatty(2);
+	options.thin = 1;
 
 	remote = remote_get(argv[1]);
 
diff --git a/send-pack.h b/send-pack.h
index 8b3cf02..28141ac 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -8,7 +8,8 @@ struct send_pack_args {
 		force_update:1,
 		use_thin_pack:1,
 		use_ofs_delta:1,
-		dry_run:1;
+		dry_run:1,
+		stateless_rpc:1;
 };
 
 int send_pack(struct send_pack_args *args,
diff --git a/sideband.c b/sideband.c
index 899b1ff..d5ffa1c 100644
--- a/sideband.c
+++ b/sideband.c
@@ -135,9 +135,14 @@ ssize_t send_sideband(int fd, int band, const char *data, ssize_t sz, int packet
 		n = sz;
 		if (packet_max - 5 < n)
 			n = packet_max - 5;
-		sprintf(hdr, "%04x", n + 5);
-		hdr[4] = band;
-		safe_write(fd, hdr, 5);
+		if (0 <= band) {
+			sprintf(hdr, "%04x", n + 5);
+			hdr[4] = band;
+			safe_write(fd, hdr, 5);
+		} else {
+			sprintf(hdr, "%04x", n + 4);
+			safe_write(fd, hdr, 4);
+		}
 		safe_write(fd, p, n);
 		p += n;
 		sz -= n;
diff --git a/transport.c b/transport.c
index 6d9652d..2ff1650 100644
--- a/transport.c
+++ b/transport.c
@@ -731,6 +731,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 				 NULL);
 	}
 
+	memset(&args, 0, sizeof(args));
 	args.send_mirror = !!(flags & TRANSPORT_PUSH_MIRROR);
 	args.force_update = !!(flags & TRANSPORT_PUSH_FORCE);
 	args.use_thin_pack = data->thin;
-- 
1.6.5.52.g0ff2e

[RFC PATCH v3 16/17] Smart fetch over HTTP: client side

[Shawn O. Pearce]

The git-remote-curl backend detects if the remote server supports
the git-upload-pack service, and if so, runs git-fetch-pack locally
in a pipe to generate the want/have commands.

The advertisements from the server that were obtained during the
discovery are passed into git-fetch-pack before the POST request
starts, permitting server capability discovery and enablement.

Common objects that are discovered are appended onto the request as
have lines and are sent again on the next request.  This allows the
remote side to reinitialize its in-memory list of common objects
during the next request.

Because all requests are relatively short, below git-remote-curl's
1 MiB buffer limit, requests will use the standard Content-Length
header and be valid HTTP/1.0 POST requests.  This makes the fetch
client more tolerant of proxy servers which don't support HTTP/1.1
or the chunked transfer encoding.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
CC: Daniel Barkalow <barkalow@iabervon.org>
---
 builtin-fetch-pack.c |  110 ++++++++++++++++++++++++++++++++++++++++++--------
 fetch-pack.h         |    3 +-
 remote-curl.c        |   69 +++++++++++++++++++++++++++++--
 3 files changed, 160 insertions(+), 22 deletions(-)

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 615f549..8ed4a6f 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -165,6 +165,24 @@ enum ack_type {
 	ACK_ready
 };
 
+static void consume_shallow_list(int fd)
+{
+	if (args.stateless_rpc && args.depth > 0) {
+		/* If we sent a depth we will get back "duplicate"
+		 * shallow and unshallow commands every time there
+		 * is a block of have lines exchanged.
+		 */
+		char line[1000];
+		while (packet_read_line(fd, line, sizeof(line))) {
+			if (!prefixcmp(line, "shallow "))
+				continue;
+			if (!prefixcmp(line, "unshallow "))
+				continue;
+			die("git fetch-pack: expected shallow list");
+		}
+	}
+}
+
 static enum ack_type get_ack(int fd, unsigned char *result_sha1)
 {
 	static char line[1000];
@@ -190,6 +208,15 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)
 	die("git fetch_pack: expected ACK/NAK, got '%s'", line);
 }
 
+static void send_request(int fd, struct strbuf *buf)
+{
+	if (args.stateless_rpc) {
+		send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);
+		packet_flush(fd);
+	} else
+		safe_write(fd, buf->buf, buf->len);
+}
+
 static int find_common(int fd[2], unsigned char *result_sha1,
 		       struct ref *refs)
 {
@@ -199,7 +226,10 @@ static int find_common(int fd[2], unsigned char *result_sha1,
 	unsigned in_vain = 0;
 	int got_continue = 0;
 	struct strbuf req_buf = STRBUF_INIT;
+	size_t state_len = 0;
 
+	if (args.stateless_rpc && multi_ack == 1)
+		die("--stateless-rpc requires multi_ack_detailed");
 	if (marked)
 		for_each_ref(clear_marks, NULL);
 	marked = 1;
@@ -256,13 +286,13 @@ static int find_common(int fd[2], unsigned char *result_sha1,
 	if (args.depth > 0)
 		packet_buf_write(&req_buf, "deepen %d", args.depth);
 	packet_buf_flush(&req_buf);
-
-	safe_write(fd[1], req_buf.buf, req_buf.len);
+	state_len = req_buf.len;
 
 	if (args.depth > 0) {
 		char line[1024];
 		unsigned char sha1[20];
 
+		send_request(fd[1], &req_buf);
 		while (packet_read_line(fd[0], line, sizeof(line))) {
 			if (!prefixcmp(line, "shallow ")) {
 				if (get_sha1_hex(line + 8, sha1))
@@ -284,28 +314,40 @@ static int find_common(int fd[2], unsigned char *result_sha1,
 			}
 			die("expected shallow/unshallow, got %s", line);
 		}
+	} else if (!args.stateless_rpc)
+		send_request(fd[1], &req_buf);
+
+	if (!args.stateless_rpc) {
+		/* If we aren't using the stateless-rpc interface
+		 * we don't need to retain the headers.
+		 */
+		strbuf_setlen(&req_buf, 0);
+		state_len = 0;
 	}
 
 	flushes = 0;
 	retval = -1;
 	while ((sha1 = get_rev())) {
-		packet_write(fd[1], "have %s\n", sha1_to_hex(sha1));
+		packet_buf_write(&req_buf, "have %s\n", sha1_to_hex(sha1));
 		if (args.verbose)
 			fprintf(stderr, "have %s\n", sha1_to_hex(sha1));
 		in_vain++;
 		if (!(31 & ++count)) {
 			int ack;
 
-			packet_flush(fd[1]);
+			packet_buf_flush(&req_buf);
+			send_request(fd[1], &req_buf);
+			strbuf_setlen(&req_buf, state_len);
 			flushes++;
 
 			/*
 			 * We keep one window "ahead" of the other side, and
 			 * will wait for an ACK only on the next one
 			 */
-			if (count == 32)
+			if (!args.stateless_rpc && count == 32)
 				continue;
 
+			consume_shallow_list(fd[0]);
 			do {
 				ack = get_ack(fd[0], result_sha1);
 				if (args.verbose && ack)
@@ -322,6 +364,17 @@ static int find_common(int fd[2], unsigned char *result_sha1,
 				case ACK_continue: {
 					struct commit *commit =
 						lookup_commit(result_sha1);
+					if (args.stateless_rpc
+					 && ack == ACK_common
+					 && !(commit->object.flags & COMMON)) {
+						/* We need to replay the have for this object
+						 * on the next RPC request so the peer knows
+						 * it is in common with us.
+						 */
+						const char *hex = sha1_to_hex(result_sha1);
+						packet_buf_write(&req_buf, "have %s\n", hex);
+						state_len = req_buf.len;
+					}
 					mark_common(commit, 0, 1);
 					retval = 0;
 					in_vain = 0;
@@ -339,7 +392,8 @@ static int find_common(int fd[2], unsigned char *result_sha1,
 		}
 	}
 done:
-	packet_write(fd[1], "done\n");
+	packet_buf_write(&req_buf, "done\n");
+	send_request(fd[1], &req_buf);
 	if (args.verbose)
 		fprintf(stderr, "done\n");
 	if (retval != 0) {
@@ -348,6 +402,7 @@ done:
 	}
 	strbuf_release(&req_buf);
 
+	consume_shallow_list(fd[0]);
 	while (flushes || multi_ack) {
 		int ack = get_ack(fd[0], result_sha1);
 		if (ack) {
@@ -672,6 +727,8 @@ static struct ref *do_fetch_pack(int fd[2],
 			 */
 			warning("no common commits");
 
+	if (args.stateless_rpc)
+		packet_flush(fd[1]);
 	if (get_pack(fd, pack_lockfile))
 		die("git fetch-pack: fetch failed.");
 
@@ -742,6 +799,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	struct ref *ref = NULL;
 	char *dest = NULL, **heads;
 	int fd[2];
+	char *pack_lockfile = NULL;
+	char **pack_lockfile_ptr = NULL;
 	struct child_process *conn;
 
 	nr_heads = 0;
@@ -791,6 +850,15 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 				args.no_progress = 1;
 				continue;
 			}
+			if (!strcmp("--stateless-rpc", arg)) {
+				args.stateless_rpc = 1;
+				continue;
+			}
+			if (!strcmp("--lock-pack", arg)) {
+				args.lock_pack = 1;
+				pack_lockfile_ptr = &pack_lockfile;
+				continue;
+			}
 			usage(fetch_pack_usage);
 		}
 		dest = (char *)arg;
@@ -801,19 +869,27 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	if (!dest)
 		usage(fetch_pack_usage);
 
-	conn = git_connect(fd, (char *)dest, args.uploadpack,
-			   args.verbose ? CONNECT_VERBOSE : 0);
-	if (conn) {
-		get_remote_heads(fd[0], &ref, 0, NULL, 0, NULL);
-
-		ref = fetch_pack(&args, fd, conn, ref, dest, nr_heads, heads, NULL);
-		close(fd[0]);
-		close(fd[1]);
-		if (finish_connect(conn))
-			ref = NULL;
+	if (args.stateless_rpc) {
+		conn = NULL;
+		fd[0] = 0;
+		fd[1] = 1;
 	} else {
-		ref = NULL;
+		conn = git_connect(fd, (char *)dest, args.uploadpack,
+				   args.verbose ? CONNECT_VERBOSE : 0);
+	}
+
+	get_remote_heads(fd[0], &ref, 0, NULL, 0, NULL);
+
+	ref = fetch_pack(&args, fd, conn, ref, dest,
+		nr_heads, heads, pack_lockfile_ptr);
+	if (pack_lockfile) {
+		printf("lock %s\n", pack_lockfile);
+		fflush(stdout);
 	}
+	close(fd[0]);
+	close(fd[1]);
+	if (finish_connect(conn))
+		ref = NULL;
 	ret = !ref;
 
 	if (!ret && nr_heads) {
diff --git a/fetch-pack.h b/fetch-pack.h
index 8bd9c32..fbe85ac 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -13,7 +13,8 @@ struct fetch_pack_args
 		fetch_all:1,
 		verbose:1,
 		no_progress:1,
-		include_tag:1;
+		include_tag:1,
+		stateless_rpc:1;
 };
 
 struct ref *fetch_pack(struct fetch_pack_args *args,
diff --git a/remote-curl.c b/remote-curl.c
index 45436d1..eb71ffd 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -45,7 +45,7 @@ static int set_option(const char *name, const char *value)
 			options.progress = 0;
 		else
 			return -1;
-		return 1 /* TODO implement later */;
+		return 0;
 	}
 	else if (!strcmp(name, "depth")) {
 		char *end;
@@ -53,7 +53,7 @@ static int set_option(const char *name, const char *value)
 		if (value == end || *end)
 			return -1;
 		options.depth = v;
-		return 1 /* TODO implement later */;
+		return 0;
 	}
 	else if (!strcmp(name, "followtags")) {
 		if (!strcmp(value, "true"))
@@ -62,7 +62,7 @@ static int set_option(const char *name, const char *value)
 			options.followtags = 0;
 		else
 			return -1;
-		return 1 /* TODO implement later */;
+		return 0;
 	}
 	else if (!strcmp(name, "dry-run")) {
 		if (!strcmp(value, "true"))
@@ -462,6 +462,8 @@ static int fetch_dumb(int nr_heads, struct ref **to_fetch)
 	char **targets = xmalloc(nr_heads * sizeof(char*));
 	int ret, i;
 
+	if (options.depth)
+		die("dumb http transport does not support --depth");
 	for (i = 0; i < nr_heads; i++)
 		targets[i] = xstrdup(sha1_to_hex(to_fetch[i]->old_sha1));
 
@@ -480,6 +482,65 @@ static int fetch_dumb(int nr_heads, struct ref **to_fetch)
 	return ret ? error("Fetch failed.") : 0;
 }
 
+static int fetch_git(struct discovery *heads,
+	int nr_heads, struct ref **to_fetch)
+{
+	struct rpc_state rpc;
+	char *depth_arg = NULL;
+	const char **argv;
+	int argc = 0, i, err;
+
+	argv = xmalloc((15 + nr_heads) * sizeof(char*));
+	argv[argc++] = "fetch-pack";
+	argv[argc++] = "--stateless-rpc";
+	argv[argc++] = "--lock-pack";
+	if (options.followtags)
+		argv[argc++] = "--include-tag";
+	if (options.thin)
+		argv[argc++] = "--thin";
+	if (options.verbosity >= 3) {
+		argv[argc++] = "-v";
+		argv[argc++] = "-v";
+	}
+	if (!options.progress)
+		argv[argc++] = "--no-progress";
+	if (options.depth) {
+		struct strbuf buf = STRBUF_INIT;
+		strbuf_addf(&buf, "--depth=%lu", options.depth);
+		depth_arg = strbuf_detach(&buf, NULL);
+		argv[argc++] = depth_arg;
+	}
+	argv[argc++] = url;
+	for (i = 0; i < nr_heads; i++) {
+		struct ref *ref = to_fetch[i];
+		if (!ref->name || !*ref->name)
+			die("cannot fetch by sha1 over smart http");
+		argv[argc++] = ref->name;
+	}
+	argv[argc++] = NULL;
+
+	memset(&rpc, 0, sizeof(rpc));
+	rpc.service_name = "git-upload-pack",
+	rpc.argv = argv;
+
+	err = rpc_service(&rpc, heads);
+	if (rpc.result.len)
+		safe_write(1, rpc.result.buf, rpc.result.len);
+	strbuf_release(&rpc.result);
+	free(argv);
+	free(depth_arg);
+	return err;
+}
+
+static int fetch(int nr_heads, struct ref **to_fetch)
+{
+	struct discovery *d = discover_refs("git-upload-pack");
+	if (d->proto_git)
+		return fetch_git(d, nr_heads, to_fetch);
+	else
+		return fetch_dumb(nr_heads, to_fetch);
+}
+
 static void parse_fetch(struct strbuf *buf)
 {
 	struct ref **to_fetch = NULL;
@@ -522,7 +583,7 @@ static void parse_fetch(struct strbuf *buf)
 			break;
 	} while (1);
 
-	if (fetch_dumb(nr_heads, to_fetch))
+	if (fetch(nr_heads, to_fetch))
 		exit(128); /* error already reported */
 	free_refs(list_head);
 	free(to_fetch);
-- 
1.6.5.52.g0ff2e

[RFC PATCH v3 17/17] Smart HTTP fetch: gzip requests

[Shawn O. Pearce]

The upload-pack requests are mostly plain text and they compress
rather well.  Deflating them with Content-Encoding: gzip can easily
drop the size of the request by 50%, reducing the amount of data
to transfer as we negotiate the common commits.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
CC: Daniel Barkalow <barkalow@iabervon.org>
---
 remote-curl.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index eb71ffd..b6420fe 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -289,6 +289,7 @@ struct rpc_state {
 	int in;
 	int out;
 	struct strbuf result;
+	unsigned gzip_request : 1;
 };
 
 static size_t rpc_out(void *ptr, size_t eltsize,
@@ -327,6 +328,8 @@ static int post_rpc(struct rpc_state *rpc)
 	struct active_request_slot *slot;
 	struct slot_results results;
 	struct curl_slist *headers = NULL;
+	int use_gzip = rpc->gzip_request;
+	char *gzip_body = NULL;
 	int err = 0, large_request = 0;
 
 	/* Try to load the entire request, if we can fit it into the
@@ -340,6 +343,7 @@ static int post_rpc(struct rpc_state *rpc)
 
 		if (left < LARGE_PACKET_MAX) {
 			large_request = 1;
+			use_gzip = 0;
 			break;
 		}
 
@@ -355,6 +359,7 @@ static int post_rpc(struct rpc_state *rpc)
 	curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
 	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
 	curl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);
+	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
 
 	headers = curl_slist_append(headers, rpc->hdr_content_type);
 	headers = curl_slist_append(headers, rpc->hdr_accept);
@@ -372,6 +377,49 @@ static int post_rpc(struct rpc_state *rpc)
 			fflush(stderr);
 		}
 
+	} else if (use_gzip && 1024 < rpc->len) {
+		/* The client backend isn't giving us compressed data so
+		 * we can try to deflate it ourselves, this may save on.
+		 * the transfer time.
+		 */
+		size_t size;
+		z_stream stream;
+		int ret;
+
+		memset(&stream, 0, sizeof(stream));
+		ret = deflateInit2(&stream, Z_BEST_COMPRESSION,
+				Z_DEFLATED, (15 + 16),
+				8, Z_DEFAULT_STRATEGY);
+		if (ret != Z_OK)
+			die("cannot deflate request; zlib init error %d", ret);
+		size = deflateBound(&stream, rpc->len);
+		gzip_body = xmalloc(size);
+
+		stream.next_in = (unsigned char *)rpc->buf;
+		stream.avail_in = rpc->len;
+		stream.next_out = (unsigned char *)gzip_body;
+		stream.avail_out = size;
+
+		ret = deflate(&stream, Z_FINISH);
+		if (ret != Z_STREAM_END)
+			die("cannot deflate request; zlib deflate error %d", ret);
+
+		ret = deflateEnd(&stream);
+		if (ret != Z_OK)
+			die("cannot deflate request; zlib end error %d", ret);
+
+		size = stream.total_out;
+
+		headers = curl_slist_append(headers, "Content-Encoding: gzip");
+		curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, gzip_body);
+		curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, size);
+
+		if (options.verbosity > 1) {
+			fprintf(stderr, "POST %s (gzip %lu to %lu bytes)\n",
+				rpc->service_name,
+				(unsigned long)rpc->len, (unsigned long)size);
+			fflush(stderr);
+		}
 	} else {
 		/* We know the complete request size in advance, use the
 		 * more normal Content-Length approach.
@@ -397,6 +445,7 @@ static int post_rpc(struct rpc_state *rpc)
 		}
 	}
 	curl_slist_free_all(headers);
+	free(gzip_body);
 	return err;
 }
 
@@ -522,6 +571,7 @@ static int fetch_git(struct discovery *heads,
 	memset(&rpc, 0, sizeof(rpc));
 	rpc.service_name = "git-upload-pack",
 	rpc.argv = argv;
+	rpc.gzip_request = 1;
 
 	err = rpc_service(&rpc, heads);
 	if (rpc.result.len)
-- 
1.6.5.52.g0ff2e

Re: [RFC PATCH v3 00/17] Return of smart HTTP

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> This v3 series is a respin, and includes fixes to address reviewer
> comments from v2.

Thanks.

Re: [RFC PATCH v3 00/17] Return of smart HTTP

[Nanako Shiraishi]

Quoting Junio C Hamano <gitster@pobox.com> writes:

> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
>> This v3 series is a respin, and includes fixes to address reviewer
>> comments from v2.
>
> Thanks.

Junio, you merged this series to your "pu" branch, but it breaks "make test" (t9801). Shawn's "sp/smart-http" topic doesn't have t9801 and passes all the tests.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [RFC PATCH v3 00/17] Return of smart HTTP

[Shawn O. Pearce]

Nanako Shiraishi <nanako3@lavabit.com> wrote:
> > "Shawn O. Pearce" <spearce@spearce.org> writes:
> >
> >> This v3 series is a respin, and includes fixes to address reviewer
> >> comments from v2.
> 
> Junio, you merged this series to your "pu" branch, but it breaks
> "make test" (t9801). Shawn's "sp/smart-http" topic doesn't have
> t9801 and passes all the tests.

Actually, pu contains v2 right now, not v3.

And pu doesn't always pass the tests... because of issues like this.

It looks like there might be a semantic clash between John Herland's
jh/cvs-helper branch and my sp/smart-http branch... but digging
around at the code I haven't quite identified what that might be.

-- 
Shawn.

Re: [RFC PATCH v3 00/17] Return of smart HTTP

[Johan Herland]

On Thursday 15 October 2009, Shawn O. Pearce wrote:
> Nanako Shiraishi <nanako3@lavabit.com> wrote:
> > > "Shawn O. Pearce" <spearce@spearce.org> writes:
> > >> This v3 series is a respin, and includes fixes to address
> > >> reviewer comments from v2.
> >
> > Junio, you merged this series to your "pu" branch, but it breaks
> > "make test" (t9801). Shawn's "sp/smart-http" topic doesn't have
> > t9801 and passes all the tests.
>
> Actually, pu contains v2 right now, not v3.
>
> And pu doesn't always pass the tests... because of issues like this.
>
> It looks like there might be a semantic clash between John Herland's
> jh/cvs-helper branch and my sp/smart-http branch... but digging
> around at the code I haven't quite identified what that might be.

Don't have time to look into this at the moment, but a cursory gdb
shows that the "git fetch" in test #4 in t9801 segfaults with the
following stacktrace:

#0  0x00007f8dd67e8a47 in fclose () from /lib/libc.so.6
#1  0x00000000004a05b5 in disconnect_helper (transport=<value optimized out>) at transport-helper.c:81
#2  0x000000000049de1e in transport_disconnect (transport=0x1955490) at transport.c:952
#3  0x0000000000423477 in cmd_fetch (argc=26566704, argv=0x0, prefix=<value optimized out>) at builtin-fetch.c:748
#4  0x0000000000404233 in handle_internal_command (argc=2, argv=0x7fffdf293d20) at git.c:251
#5  0x0000000000404426 in main (argc=2, argv=0x7fffdf293d20) at git.c:438

Don't know if this helps...


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [RFC PATCH v3 00/17] Return of smart HTTP

[Shawn O. Pearce]

Johan Herland <johan@herland.net> wrote:
> Don't have time to look into this at the moment, but a cursory gdb
> shows that the "git fetch" in test #4 in t9801 segfaults with the
> following stacktrace:
> 
> #0  0x00007f8dd67e8a47 in fclose () from /lib/libc.so.6
> #1  0x00000000004a05b5 in disconnect_helper (transport=<value optimized out>) at transport-helper.c:81
> #2  0x000000000049de1e in transport_disconnect (transport=0x1955490) at transport.c:952
> #3  0x0000000000423477 in cmd_fetch (argc=26566704, argv=0x0, prefix=<value optimized out>) at builtin-fetch.c:748
> #4  0x0000000000404233 in handle_internal_command (argc=2, argv=0x7fffdf293d20) at git.c:251
> #5  0x0000000000404426 in main (argc=2, argv=0x7fffdf293d20) at git.c:438

It does.  It is caused by the disconnect_helper call inside of
fetch_with_import.  You can't disconnect inside of the fetch method
of a transport, the caller is going to disconnect you a second time.

During that second disconnect the transport->data field is now
pointing at a garbage area of memory.  We're passing a garbage
pointer from data->out to fclose, and fclose is rightly upset.

This bug isn't due to the merge, its a bug in Johan's series that
needs to be fixed before it could merge down to next/master.

-- 
Shawn.

Re: [RFC PATCH v3 00/17] Return of smart HTTP

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> It does.  It is caused by the disconnect_helper call inside of
> fetch_with_import.  You can't disconnect inside of the fetch method
> of a transport, the caller is going to disconnect you a second time.
> ...
> This bug isn't due to the merge, its a bug in Johan's series that
> needs to be fixed before it could merge down to next/master.

Thanks; I pushed out 'pu' with your v3 this time.

Last night I did a trial fetch merge with FETCH_HEAD into 'pu', but then
after I queued some fixes to 'maint' and 'master' to prepare for 1.6.5.1,
I rebuilt 'pu' with the still-old sp/smart-http topic, and that is what
was sitting at k.org til this morning.

I am a bit confused about your diagnosis, though.  As far as I recall,
Johan's topic itself nor 'pu' with Johan's topic but without v2 of
sp/smart-http did not have the issue.  Does that indicate that the
behaviour expected from the fetch method is different between the two
topics, and this indeed is a semantic conflict as you suspected initially?
In other words, if Johan's series is updated not to disconnect inside the
fetch method of the transport, and if it is not merged with your series,
does the caller still disconnect it properly?

Re: [RFC PATCH v3 00/17] Return of smart HTTP

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > It does.  It is caused by the disconnect_helper call inside of
> > fetch_with_import.  You can't disconnect inside of the fetch method
> > of a transport, the caller is going to disconnect you a second time.
> > ...
> > This bug isn't due to the merge, its a bug in Johan's series that
> > needs to be fixed before it could merge down to next/master.
...
> I am a bit confused about your diagnosis, though.  As far as I recall,
> Johan's topic itself nor 'pu' with Johan's topic but without v2 of
> sp/smart-http did not have the issue.

Sadly, sometimes double frees do not result in segfaults, other
times they do.  The reason you are not seeing a problem with these
other variants is because of luck, not code correctness.

Actually, after some further research, the bug is not Johan's but is
actually Daniel's.  Johan, I apologize for claiming it was your bug.

In:

  commit 23a3380ee9c2d5164712c40f8821cb0fba24e80c
  Author: Daniel Barkalow <barkalow@iabervon.org>
  Date:   Thu Sep 3 22:14:01 2009 -0400

    Add support for "import" helper command

Daniel introduces the fetch_with_import() function to
transport-helper.c.  This method calls disconnect_helper():

+static int fetch_with_import(struct transport *transport,
+                            int nr_heads, struct ref **to_fetch)
+{
...
+       disconnect_helper(transport);
+       finish_command(&fastimport);

Unfortunately this is in the middle of the transport_fetch() call
stack; transport_fetch() called the static fetch() function in
transport-helper.c, which in turn called fetch_with_import().

Callers (e.g. builtin-fetch.c) invoke transport_close() when
they are done with the handle (see line 704).  That in turn calls
disconnect_helper() a second time.

The disconnect_helper function is not prepared to be called twice:

static int disconnect_helper(struct transport *transport)
{
	struct helper_data *data = transport->data;
	if (data->helper) {
	...
	}
	free(data);
	return 0;
}

Because of that unexpected invocation inside of fetch_with_import
we have already free'd the memory block used by transport->data,
and the second invocation attempts to free it again.  Worse, if the
block was reused by a subsequent malloc, data->helper might not be
NULL, and we'd enter into the if block and do its work again.

Long story short, transport_close() is what is supposed to perform
the work that disconnect_helper does, as its the final thing right
before we free the struct transport block.  Free'ing the data block
inside of the fetch or push functions is wrong.

Its fine to close the helper and restart it within the single
lifespan of a struct transport, but dammit, don't free the
struct helper_data until transport_close().

-- 
Shawn.

Re: [RFC PATCH v3 00/17] Return of smart HTTP

[Mark Lodato]

On Wed, Oct 14, 2009 at 11:36 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
> This series is still lacking:
>
>  * The HTTP protocol documentation
>  * Tests for the smart http transport code (existing tests pass)

* Dumb HTTP push support

It would be really nice if git-http-backend supported dumb pushing
over WebDAV.  Currently, to support both smart and dumb pushing, one
has to configure Apache in a very awkward and confusing way (if it is
even possible - I'm still trying to figure it out).  Without some way
to support older clients, it will be very hard to transition to the
new protocol.


Also, your examples use "DocumentRoot /pub/git", but I think most
people would want to have their main website as the DocumentRoot, have
the URL "/git" serve the repositories through gitweb, and have that
same "/git" URL be `git clone'-able.  The Apache configuration for
this is complicated and non-intuitive, so I think an example of this
in the documentation is warranted.  The following accomplishes what I
describe, except it does not work with dump HTTP push, and does not
allow anonymous read-only access.  (I am currently trying to figure
out how to do both of these things.)

-- 8< --
DocumentRoot /var/www/htdocs
# Rest of httpd config...

<Directory /pub/git>
   SetHandler git-http-backend
   Action git-http-backend /git-http-backend virtual
</Directory>

# To allow anonymous access (but to disallow pushing), comment out the
following Location block.
<Location /git-http-backend>
   AuthName "git"
   AuthType Basic
   AuthUserFile /etc/apache2/passwd/git.passwd
   Require valid-user
</Location>

# Each of the following Aliases should be one line:
AliasMatch ^/git/(.*/(HEAD|info/refs|objects/(info/[^/]*|[0-9a-f]{2}/[0-9a-f]{38}|pack/pack-[0-9a-f]{40}\.(pack|idx))|git-(upload|receive)-pack))$
"/pub/git/$1"
ScriptAlias /git "/var/www/cgi-bin/gitweb.cgi"
ScriptAlias /git-http-backend "/usr/local/libexec/git-core/git-http-backend"
-- >8 --

Re: [RFC PATCH v3 00/17] Return of smart HTTP

[Shawn O. Pearce]

Mark Lodato <lodatom@gmail.com> wrote:
> On Wed, Oct 14, 2009 at 11:36 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
> > This series is still lacking:
> >
> > ??* The HTTP protocol documentation
> > ??* Tests for the smart http transport code (existing tests pass)
> 
> * Dumb HTTP push support

I'm not interested in implementing a WebDAV server.  13 years
ago when WebDAV was the new shiny I considered it.  Today, bleh,
no desire.
 
> It would be really nice if git-http-backend supported dumb pushing
> over WebDAV.  Currently, to support both smart and dumb pushing, one
> has to configure Apache in a very awkward and confusing way (if it is
> even possible - I'm still trying to figure it out).

Sure, its confusing, its one reason nobody uses it.  Another is
that pushing over WebDAV is slow and error prone, locks get taken
and have to get released, its a general mess.

> Without some way
> to support older clients, it will be very hard to transition to the
> new protocol.

Why?

Smart HTTP is primarily about improving the situation for a client
fetching from the server.  If the server happens to allow writes,
users will just have to upgrade to a new enough version of Git that
understands the push variant of the protocol.

If users don't want to upgrade, or can't upgrade, then you can't
push over HTTP.  Simple.

Really, what it comes down to is, I don't think it matters that
we don't have backwards compatiblity for pushing through WebDAV.
If you think it matters, you are free to write a patch series on
top of mine which adds the functionality.  But don't wait for me
to do it, it won't happen.
 
> Also, your examples use "DocumentRoot /pub/git", but I think most
> people would want to have their main website as the DocumentRoot, have
> the URL "/git" serve the repositories through gitweb, and have that
> same "/git" URL be `git clone'-able.

Why not have git-http-backend exec gitweb when it gets a request
for the repository itself?  Why do you have to go through such
contortions in Apache for this?  The two CGIs are shipped in the
same software package, surely one could actually invoke the other.

> The Apache configuration for
> this is complicated and non-intuitive, so I think an example of this
> in the documentation is warranted.  The following accomplishes what I
> describe, except it does not work with dump HTTP push, and does not
> allow anonymous read-only access.  (I am currently trying to figure
> out how to do both of these things.)

Yes, I'd like to have examples in the git-http-backend manpage.
I put a couple in there already, but they don't consider gitweb
because I assumed we'd find a way to have gitweb be invoked out
of git-http-backend.  Unfortunately that hasn't happened yet.
 
-- 
Shawn.

Re: [RFC PATCH v3 00/17] Return of smart HTTP

[Mark Lodato]

I just realized I forgot to say something in my last email: THANK
YOU!!!  I have been looking forward to this for a long time.  I was
planning to one day to sit down and start thinking about how to
implement a smart protocol, and then I see a post saying that not only
has someone figured out the protocol, but he has implemented it!
Amazing!  This is really crucial for corporate adoption, at least at
my job.  We need to have strong authentication for many users, and the
SSH key management is just a nightmare, not to mention that not all
users can SSH due to firewalls.  This will save me so much time and
frustration.  Thanks!

On Fri, Oct 16, 2009 at 10:31 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> If users don't want to upgrade, or can't upgrade, then you can't
> push over HTTP.  Simple.

Yeah, I realized after I wrote my previous email that it probably
doesn't matter - the current state of HTTP push is so awful that no
one probably uses it!

> Really, what it comes down to is, I don't think it matters that
> we don't have backwards compatiblity for pushing through WebDAV.
> If you think it matters, you are free to write a patch series on
> top of mine which adds the functionality.  But don't wait for me
> to do it, it won't happen.

Agreed.  I was just wondering if it was on the to-do list for the far
off future.  Either way, it should probably be noted in the
documentation that dumb push is not supported.

>> Also, your examples use "DocumentRoot /pub/git", but I think most
>> people would want to have their main website as the DocumentRoot, have
>> the URL "/git" serve the repositories through gitweb, and have that
>> same "/git" URL be `git clone'-able.
>
> Why not have git-http-backend exec gitweb when it gets a request
> for the repository itself?  Why do you have to go through such
> contortions in Apache for this?  The two CGIs are shipped in the
> same software package, surely one could actually invoke the other.
>
> [snip]
>
> Yes, I'd like to have examples in the git-http-backend manpage.
> I put a couple in there already, but they don't consider gitweb
> because I assumed we'd find a way to have gitweb be invoked out
> of git-http-backend.  Unfortunately that hasn't happened yet.

The gitweb part is just bonus.  (The only thing for gitweb is the one
ScriptAlias line.)  The real challenge is getting git-http-backend to
serve repositories out of something other than DocumentRoot - say
DocumentRoot is /var/www/htdocs but your git repositories are in
/pub/git - which is why I posted the configuration.  If you'd like, I
can send you a patch to add this to the documentation.

One idea to improve the situation is to first try
$GIT_PROJECT_ROOT$PATH_INFO, falling back to $PATH_TRANSLATED if
$GIT_PROJECT_ROOT is empty.  This would make the configuration simple:

-- 8< --
SetEnv GIT_PROJECT_ROOT /pub/git
ScriptAlias /git /usr/libexec/git-core/git-http-backend
-- >8 --

As far as having git-http-backend launch an external process such as
gitweb, I personally don't think this is important enough to block
this from continuing.  One could configure the webserver as I have in
my previous email until such a feature is implemented.  For cgit, the
solution is simple - just integrate git-http-backend into the
executable.

Regards,
Mark

Re: [RFC PATCH v3 00/17] Return of smart HTTP

[Shawn O. Pearce]

Mark Lodato <lodatom@gmail.com> wrote:
> The gitweb part is just bonus.  (The only thing for gitweb is the one
> ScriptAlias line.)  The real challenge is getting git-http-backend to
> serve repositories out of something other than DocumentRoot - say
> DocumentRoot is /var/www/htdocs but your git repositories are in
> /pub/git - which is why I posted the configuration.  If you'd like, I
> can send you a patch to add this to the documentation.

Yes, a patch would be wonderful.
 
> One idea to improve the situation is to first try
> $GIT_PROJECT_ROOT$PATH_INFO, falling back to $PATH_TRANSLATED if
> $GIT_PROJECT_ROOT is empty.  This would make the configuration simple:
> 
> -- 8< --
> SetEnv GIT_PROJECT_ROOT /pub/git
> ScriptAlias /git /usr/libexec/git-core/git-http-backend
> -- >8 --

Oooh.  Perhaps a good idea.  :-)

-- 
Shawn.

Re: [RFC PATCH v3 10/17] Move WebDAV HTTP push under remote-curl

[Tay Ray Chuan]

Hi,

On Thu, Oct 15, 2009 at 11:36 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
> @@ -34,6 +34,10 @@ Commands are given by the caller on the helper's standard input, one per line.
>        value of the ref. A space-separated list of attributes follows
>        the name; unrecognized attributes are ignored. After the
>        complete list, outputs a blank line.
> ++
> +If 'push' is supported this may be called as 'list for-push'
> +to obtain the current refs prior to sending one or more 'push'
> +commands to the helper.

The new paragraph should have the same indentation as 'list'.

It would have been great if you implemented this as a filter, such that
'list <attr>' lists the refs with the specified attribute <attr>,
rather than hardcoding it.

> @@ -59,6 +63,22 @@ suitably updated.
> +When the push is complete, outputs one or more 'ok <dst>' or
> +'error <dst> <why>?' lines to indicate success or failure of
> +each pushed ref.  The status report output is terminated by
> +a blank line.  The option field <why> may be quoted in a C
> +style string if it contains an LF.

You should mention that this behaviour only occurs when the
--helper-status option is used.

> @@ -106,6 +132,11 @@ OPTIONS
> +'option dry-run' \{'true'|'false'\}:
> +       If true, pretend like the operation completed successfully,
> +       but don't actually change any repository data.  For most
> +       helpers this only applies to the 'push', if supported.
> +

The 'like' after 'pretend' can be, like, removed. :)

> diff --git a/http-push.c b/http-push.c
> @@ -1941,9 +1946,14 @@ int main(int argc, char **argv)
>
>                if (is_null_sha1(ref->peer_ref->new_sha1)) {
>                        if (delete_remote_branch(ref->name, 1) == -1) {
> -                               error("Could not remove %s", ref->name);
> +                               if (helper_status)
> +                                       printf("error %s cannot remove\n", ref->name);
> +                               else
> +                                       error("Could not remove %s", ref->name);
>                                rc = -4;
>                        }

I think error() calls should be left intact (as indicators to the
user), even when --helper-status is specified. It wouldn't affect
transport-helper.c's parsing of '(ok|error) <ref>' lines, since it
reads stdout only.

In other words, the above would read:

>                                error("Could not remove %s", ref->name);
> +                               if (helper_status)
> +                                       printf("error %s cannot remove\n", ref->name);
>                                rc = -4;
>                        }

> @@ -1968,12 +1980,15 @@ int main(int argc, char **argv)
>                                 * commits at the remote end and likely
>                                 * we were not up to date to begin with.
>                                 */
> -                               error("remote '%s' is not an ancestor of\n"
> -                                     "local '%s'.\n"
> -                                     "Maybe you are not up-to-date and "
> -                                     "need to pull first?",
> -                                     ref->name,
> -                                     ref->peer_ref->name);
> +                               if (helper_status)
> +                                       printf("error %s non-fast forward\n", ref->name);
> +                               else
> +                                       error("remote '%s' is not an ancestor of\n"
> +                                                 "local '%s'.\n"
> +                                                 "Maybe you are not up-to-date and "
> +                                                 "need to pull first?",
> +                                                 ref->name,
> +                                                 ref->peer_ref->name);
>                                rc = -2;
>                                continue;
>                        }

Same here.

> @@ -1987,14 +2002,20 @@ int main(int argc, char **argv)
>                /* Lock remote branch ref */
>                ref_lock = lock_remote(ref->name, LOCK_TIME);
>                if (ref_lock == NULL) {
> -                       fprintf(stderr, "Unable to lock remote branch %s\n",
> -                               ref->name);
> +                       if (helper_status)
> +                               printf("error %s lock error\n", ref->name);
> +                       else
> +                               fprintf(stderr, "Unable to lock remote branch %s\n",
> +                                       ref->name);
>                        rc = 1;
>                        continue;
>                }

Same here.

Two more areas in http-push.c that should have status messages
(generated on top of pu):

(I'm not sure what ref should read when there's no match, but
transport-helper.c should have no problems parsing it with 'null'.)

-->8--

diff --git a/http-push.c b/http-push.c
index 9010ccc..e979feb 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1916,9 +1916,12 @@ int main(int argc, char **argv)

 	/* Remove a remote branch if -d or -D was specified */
 	if (delete_branch) {
-		if (delete_remote_branch(refspec[0], force_delete) == -1)
+		if (delete_remote_branch(refspec[0], force_delete) == -1) {
 			fprintf(stderr, "Unable to delete remote branch %s\n",
 				refspec[0]);
+			if (helper_status)
+				printf("error %s cannot remove\n", refspec[0]);
+		}
 		goto cleanup;
 	}

@@ -1930,6 +1933,8 @@ int main(int argc, char **argv)
 	}
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n");
+		if (helper_status)
+			printf("error null no match\n");
 		rc = 0;
 		goto cleanup;
 	}

--
Cheers,
Ray Chuan

Re: [RFC PATCH v3 00/17] Return of smart HTTP

[Nanako Shiraishi]

Quoting "Shawn O. Pearce" <spearce@spearce.org>

> Actually, after some further research, the bug is not Johan's but is
> actually Daniel's.  Johan, I apologize for claiming it was your bug.
> ...
> Long story short, transport_close() is what is supposed to perform
> the work that disconnect_helper does, as its the final thing right
> before we free the struct transport block.  Free'ing the data block
> inside of the fetch or push functions is wrong.
>
> Its fine to close the helper and restart it within the single
> lifespan of a struct transport, but dammit, don't free the
> struct helper_data until transport_close().

Ping? Are there any progress on this issue?

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [RFC PATCH v3 00/17] Return of smart HTTP

[Daniel Barkalow]

On Thu, 22 Oct 2009, Nanako Shiraishi wrote:

> Quoting "Shawn O. Pearce" <spearce@spearce.org>
> 
> > Actually, after some further research, the bug is not Johan's but is
> > actually Daniel's.  Johan, I apologize for claiming it was your bug.
> > ...
> > Long story short, transport_close() is what is supposed to perform
> > the work that disconnect_helper does, as its the final thing right
> > before we free the struct transport block.  Free'ing the data block
> > inside of the fetch or push functions is wrong.
> >
> > Its fine to close the helper and restart it within the single
> > lifespan of a struct transport, but dammit, don't free the
> > struct helper_data until transport_close().
> 
> Ping? Are there any progress on this issue?

Ah, right. Shawn's analysis is correct, and I should have a different 
function to just finish the helper, but leave the rest of the data alone. 
(when I wrote it originally, I didn't have anything other than the 
connection in there, so it was right to clear it, but now there's a real 
helper_data and it needs to do the right things).

	-Daniel
*This .sig left intentionally blank*

Re: [RFC PATCH v3 00/17] Return of smart HTTP

[Marcus Camen]

On Samstag 17 Oktober 2009, Mark Lodato wrote:
> I just realized I forgot to say something in my last email: THANK
> YOU!!!  I have been looking forward to this for a long time.  I was
> planning to one day to sit down and start thinking about how to
> implement a smart protocol, and then I see a post saying that not only
> has someone figured out the protocol, but he has implemented it!
> Amazing!  This is really crucial for corporate adoption, at least at
> my job.  We need to have strong authentication for many users, and the
> SSH key management is just a nightmare, not to mention that not all
> users can SSH due to firewalls.  This will save me so much time and
> frustration.  Thanks!

As I am using git in a corporate environment I couldn't agree more. smart-
http definitely is the most import addition to git in the last months.

So far I didn't carry out extensive production runs but my first tests 
already showed that the smart-http backend solves many problems we have 
with the dumb webdav approach (no hooks, insane amount of http requests, 
slow performance, ...).

As I have a systems engineer (not a C coder) perspective on git I cannot 
comment an any possible glitches of this implementation. I just want to 
make sure that everyone recognizes just how import smart-http is for 
corporate environments and that the current patch already solves a lot of 
problems.


Regards,
Marcus

[PATCH 0/6] http: push and test fixes

[Tay Ray Chuan]

This series applies on top of pu, and should apply cleanly on top of
Shawn's v3-smart-http branch.

Patch 1: fixes a bug in http-push.c when fetching packs. This issue is
  minor and is unlikely to be triggered by normal users (for a possible
  trigger, see the "http-push fetches packed objects" test in
  t5540-http-push).

Patch 2: here, I collect my comments to Shawn's "Move WebDAV HTTP push
  under remote-curl" on the disabling of messages to stderr when
  --helper-status is used with http-push. I hope this will make it
  easier for Shawn to squash into his series, in the event he decides
  to.

Patch 3: also in response to Shawn's "Move WebDAV HTTP push under
  remote-curl", I collect here my mini-patch to add more reports for
  transport-helper. Again, I hope this will make it easier for Shawn to
  squash into his series.

Patch 4-7: update tests, as Shawn's patches changed the behaviour of
  the http push mechanism (dumb).

Tay Ray Chuan (7):
  http-push: fix check condition on http.c::finish_http_pack_request()
  http-push: allow stderr messages to appear alongside helper_status
    ones
  http-push: add more 'error <dst> <why>' status reports
  t5540-http-push: expect success when pushing without arguments
  t5540-http-push: check existence of fetched files
  t5540-http-push: when deleting remote refs, don't need to branch -d
    -r
  t5540-http-push: remove redundant fetches

 http-push.c          |   30 ++++++++++++++++--------------
 t/t5540-http-push.sh |   38 +++++++++++++++++++-------------------
 2 files changed, 35 insertions(+), 33 deletions(-)

[PATCH 1/7] http-push: fix check condition on http.c::finish_http_pack_request()

[Tay Ray Chuan]

Check that http.c::finish_http_pack_request() returns 0 (for success).

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 http-push.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/http-push.c b/http-push.c
index 9010ccc..24eec73 100644
--- a/http-push.c
+++ b/http-push.c
@@ -605,7 +605,7 @@ static void finish_request(struct transfer_request *request)
 			preq = (struct http_pack_request *)request->userData;

 			if (preq) {
-				if (finish_http_pack_request(preq) > 0)
+				if (finish_http_pack_request(preq) == 0)
 					fail = 0;
 				release_http_pack_request(preq);
 			}
--
1.6.4.4

[PATCH 2/7] http-push: allow stderr messages to appear alongside helper_status ones

[Tay Ray Chuan]

These messages notifies user over pushing progress (or the lack
thereof). It is safe for them to appear alongside status reports
('ok|error <dst>'), since the reports go to stdout, while the
notifications go to stderr.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 http-push.c |   21 +++++++++------------
 1 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/http-push.c b/http-push.c
index 24eec73..b97ea1f 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1946,10 +1946,9 @@ int main(int argc, char **argv)

 		if (is_null_sha1(ref->peer_ref->new_sha1)) {
 			if (delete_remote_branch(ref->name, 1) == -1) {
+				error("Could not remove %s", ref->name);
 				if (helper_status)
 					printf("error %s cannot remove\n", ref->name);
-				else
-					error("Could not remove %s", ref->name);
 				rc = -4;
 			}
 			else if (helper_status)
@@ -1980,15 +1979,14 @@ int main(int argc, char **argv)
 				 * commits at the remote end and likely
 				 * we were not up to date to begin with.
 				 */
+				error("remote '%s' is not an ancestor of\n"
+				      "local '%s'.\n"
+				      "Maybe you are not up-to-date and "
+				      "need to pull first?",
+				      ref->name,
+				      ref->peer_ref->name);
 				if (helper_status)
 					printf("error %s non-fast forward\n", ref->name);
-				else
-					error("remote '%s' is not an ancestor of\n"
-						  "local '%s'.\n"
-						  "Maybe you are not up-to-date and "
-						  "need to pull first?",
-						  ref->name,
-						  ref->peer_ref->name);
 				rc = -2;
 				continue;
 			}
@@ -2011,11 +2009,10 @@ int main(int argc, char **argv)
 		/* Lock remote branch ref */
 		ref_lock = lock_remote(ref->name, LOCK_TIME);
 		if (ref_lock == NULL) {
+			fprintf(stderr, "Unable to lock remote branch %s\n",
+				ref->name);
 			if (helper_status)
 				printf("error %s lock error\n", ref->name);
-			else
-				fprintf(stderr, "Unable to lock remote branch %s\n",
-					ref->name);
 			rc = 1;
 			continue;
 		}
--
1.6.4.4

[PATCH 3/7] http-push: add more 'error <dst> <why>' status reports

[Tay Ray Chuan]

Complement c6aa27e (Move WebDAV HTTP push under remote-curl, Wed Oct
14) by adding error reports for 'cannot remove' (failed to delete
branch) and 'no match'.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 http-push.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/http-push.c b/http-push.c
index b97ea1f..f10803a 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1916,9 +1916,12 @@ int main(int argc, char **argv)

 	/* Remove a remote branch if -d or -D was specified */
 	if (delete_branch) {
-		if (delete_remote_branch(refspec[0], force_delete) == -1)
+		if (delete_remote_branch(refspec[0], force_delete) == -1) {
 			fprintf(stderr, "Unable to delete remote branch %s\n",
 				refspec[0]);
+			if (helper_status)
+				printf("error %s cannot remove\n", refspec[0]);
+		}
 		goto cleanup;
 	}

@@ -1930,6 +1933,8 @@ int main(int argc, char **argv)
 	}
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n");
+		if (helper_status)
+			printf("error null no match\n");
 		rc = 0;
 		goto cleanup;
 	}
--
1.6.4.4

[PATCH 4/7] t5540-http-push: expect success when pushing without arguments

[Tay Ray Chuan]

Remove mention of packed refs from the test description, and expect
success.

The dumb http push mechanism used to learn about the refs on the remote
repository by recursing through the /refs directory in the repository.
This meant that it was unaware of packed refs, since it did not read
/packed-refs. Thus the push failed, as no remote refs were found.

But after c6aa27e (Move WebDAV HTTP push under remote-curl, Wed Oct
14), the dumb http mechanism additionally learns about the refs through
/info/refs (via remote-curl.c::get_refs), so it is aware of packed
refs, even though it still doesn't read /packed-refs (assuming /info/
refs is up-to-date). Thus the push now succeeds.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 t/t5540-http-push.sh |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index f4a2cf6..c7b8a40 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -45,7 +45,7 @@ test_expect_success 'clone remote repository' '
 	git clone $HTTPD_URL/test_repo.git test_repo_clone
 '

-test_expect_failure 'push to remote repository with packed refs' '
+test_expect_success 'push to remote repository' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	: >path2 &&
 	git add path2 &&
@@ -57,16 +57,6 @@ test_expect_failure 'push to remote repository with packed refs' '
 	 test $HEAD = $(git rev-parse --verify HEAD))
 '

-test_expect_success ' push to remote repository with unpacked refs' '
-	(cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
-	 rm packed-refs &&
-	 git update-ref refs/heads/master \
-		0c973ae9bd51902a28466f3850b543fa66a6aaf4) &&
-	git push &&
-	(cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
-	 test $HEAD = $(git rev-parse --verify HEAD))
-'
-
 test_expect_success 'http-push fetches unpacked objects' '
 	cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \
 		"$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_unpacked.git &&
--
1.6.4.4

[PATCH 5/7] t5540-http-push: check existence of fetched files

[Tay Ray Chuan]

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 t/t5540-http-push.sh |   23 ++++++++++++++++++-----
 1 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index c7b8a40..ee7f84a 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -64,13 +64,18 @@ test_expect_success 'http-push fetches unpacked objects' '
 	git clone $HTTPD_URL/test_repo_unpacked.git \
 		"$ROOT_PATH"/fetch_unpacked &&

+	COMMIT_PATH=$(git rev-parse --verify HEAD |
+		sed -e "s/^\([0-9a-f]\{2\}\)\([0-9a-f]\{38\}\)/\1\/\2/") &&
+
 	# By reset, we force git to retrieve the object
 	(cd "$ROOT_PATH"/fetch_unpacked &&
 	 git reset --hard HEAD^ &&
 	 git remote rm origin &&
 	 git reflog expire --expire=0 --all &&
 	 git prune &&
-	 git push -f -v $HTTPD_URL/test_repo_unpacked.git master)
+	 test ! -e ".git/objects/$COMMIT_PATH" &&
+	 git push -f -v $HTTPD_URL/test_repo_unpacked.git master &&
+	 test -e ".git/objects/$COMMIT_PATH")
 '

 test_expect_success 'http-push fetches packed objects' '
@@ -80,9 +85,14 @@ test_expect_success 'http-push fetches packed objects' '
 	git clone $HTTPD_URL/test_repo_packed.git \
 		"$ROOT_PATH"/test_repo_clone_packed &&

-	(cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_packed.git &&
-	 git --bare repack &&
-	 git --bare prune-packed) &&
+	cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_packed.git &&
+	git --bare repack &&
+	git --bare prune-packed &&
+
+	PACK_PATH=$(cat "objects/info/packs" |
+		sed -e "s/^P \(pack-[0-9a-f]\{40\}\.pack\)/\1/") &&
+	COMMIT_PATH=$(git rev-parse --verify HEAD |
+		sed -e "s/^\([0-9a-f]\{2\}\)\([0-9a-f]\{38\}\)/\1\/\2/") &&

 	# By reset, we force git to retrieve the packed object
 	(cd "$ROOT_PATH"/test_repo_clone_packed &&
@@ -90,7 +100,10 @@ test_expect_success 'http-push fetches packed objects' '
 	 git remote rm origin &&
 	 git reflog expire --expire=0 --all &&
 	 git prune &&
-	 git push -f -v $HTTPD_URL/test_repo_packed.git master)
+	 test ! -e ".git/objects/$COMMIT_PATH" &&
+	 test ! -e ".git/objects/pack/$PACK_PATH" &&
+	 git push -f -v $HTTPD_URL/test_repo_packed.git master &&
+	 test -e ".git/objects/pack/$PACK_PATH")
 '

 test_expect_success 'create and delete remote branch' '
--
1.6.4.4

[PATCH 6/7] t5540-http-push: when deleting remote refs, don't need to branch -d -r

[Tay Ray Chuan]

In c6aa27e (Move WebDAV HTTP push under remote-curl, Wed Oct 14),
transport->push_refs became defined for http pushing. This enabled the
code to update local tracking refs (transport.c::update_tracking_ref()
via transport.c::transport_push()), allowing these refs to be deleted
together with the remote ref, thus doing away with a manual branch -d
-r.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 t/t5540-http-push.sh |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index ee7f84a..6bb5afa 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -116,7 +116,6 @@ test_expect_success 'create and delete remote branch' '
 	git push origin dev &&
 	git fetch &&
 	git push origin :dev &&
-	git branch -d -r origin/dev &&
 	git fetch &&
 	test_must_fail git show-ref --verify refs/remotes/origin/dev
 '
--
1.6.4.4

[PATCH 7/7] t5540-http-push: remove redundant fetches

[Tay Ray Chuan]

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 t/t5540-http-push.sh |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index 6bb5afa..3852dcb 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -114,9 +114,7 @@ test_expect_success 'create and delete remote branch' '
 	test_tick &&
 	git commit -m dev &&
 	git push origin dev &&
-	git fetch &&
 	git push origin :dev &&
-	git fetch &&
 	test_must_fail git show-ref --verify refs/remotes/origin/dev
 '

--
1.6.4.4

Re: [PATCH 4/7] t5540-http-push: expect success when pushing without arguments

[Clemens Buchacher]

On Sun, Oct 25, 2009 at 11:21:42PM +0800, Tay Ray Chuan wrote:

> But after c6aa27e (Move WebDAV HTTP push under remote-curl, Wed Oct
> 14), the dumb http mechanism additionally learns about the refs through
> /info/refs (via remote-curl.c::get_refs), so it is aware of packed
> refs, even though it still doesn't read /packed-refs (assuming /info/
> refs is up-to-date). Thus the push now succeeds.

If left as is, it should actually fail, and for two reasons.

1. push aborts immediately because info/refs is not updated by "git
update-ref" so that it thinks everything is up-to-date from the previous
test. The old http-push code did not check for up-to-date refs which is why
this problem did not trigger so far. This is fixed in "update http tests
according to remote-curl capabilities".

2. Even if info/refs is updated, a bug in the new remote-helper causes push
to exit with error status 128. This is fixed in "remote-helpers: return
successfully if everything up-to-date".

This is unrelated to the original intention of the test, but I think it
doesn't hurt to keep it around, at least for the time being.

It's unfortunate that we both worked on t5540 at the same time. But except
for this point I think our changes are compatible.

Clemens

Re: [PATCH 5/7] t5540-http-push: check existence of fetched files

[Clemens Buchacher]

On Sun, Oct 25, 2009 at 11:22:27PM +0800, Tay Ray Chuan wrote:

>  	# By reset, we force git to retrieve the object
>  	(cd "$ROOT_PATH"/fetch_unpacked &&
>  	 git reset --hard HEAD^ &&
>  	 git remote rm origin &&
>  	 git reflog expire --expire=0 --all &&
>  	 git prune &&
> -	 git push -f -v $HTTPD_URL/test_repo_unpacked.git master)
> +	 test ! -e ".git/objects/$COMMIT_PATH" &&
> +	 git push -f -v $HTTPD_URL/test_repo_unpacked.git master &&
> +	 test -e ".git/objects/$COMMIT_PATH")
>  '

This fails with smart HTTP. First of all, the objects are packed,
so I substituted the above with

 ! git rev-list -1 $HEAD &&
 git push -f -v $HTTPD_GIT_URL/test_repo_unpacked.git master &&
 git rev-list -1 $HEAD > rev-list.out &&
 test -n rev-list.out

That should be an equivalent test, right? But still, why should push
re-fetch the commit we just pruned? Smart HTTP does not do that and
therefore fails the last rev-list test:

 ++ git rev-list -1 9d498b0bbc2a25438e2fbd19081948da86028c23
 fatal: bad object 9d498b0bbc2a25438e2fbd19081948da86028c23

Clemens

[PATCH] Fix memory leak in transport-helper

[Daniel Barkalow]

On Thu, 15 Oct 2009, Shawn O. Pearce wrote:

> The disconnect_helper function is not prepared to be called twice:
> 
> static int disconnect_helper(struct transport *transport)
> {
> 	struct helper_data *data = transport->data;
> 	if (data->helper) {
> 	...
> 	}
> 	free(data);
> 	return 0;
> }

Actually, my version just leaks transport->data; it looks like the 
"free(data);" line comes from your patch "remote-helpers: Support custom 
transport options". Here's a version (against origin/master) that neither 
leaks memory nor frees too much for disconnecting temporarily. 

commit 8731d804c20828d20130e286f088613b5d33d57a
Author: Daniel Barkalow <barkalow@iabervon.org>
Date:   Tue Oct 27 00:42:16 2009 -0400

    Fix memory leak in helper method for disconnect.
    
    Since some cases may need to disconnect from the helper and reconnect,
    wrap the function that just disconnects in a function that also frees
    transport->data.
    
    Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>

diff --git a/transport-helper.c b/transport-helper.c
index f57e84c..479539d 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -67,6 +67,13 @@ static int disconnect_helper(struct transport *transport)
 	return 0;
 }
 
+static int close_helper(struct transport *transport)
+{
+	disconnect_helper(transport);
+	free(transport->data);
+	return 0;
+}
+
 static int fetch_with_fetch(struct transport *transport,
 			    int nr_heads, const struct ref **to_fetch)
 {
@@ -163,6 +170,6 @@ int transport_helper_init(struct transport *transport, const char *name)
 	transport->data = data;
 	transport->get_refs_list = get_refs_list;
 	transport->fetch = fetch;
-	transport->disconnect = disconnect_helper;
+	transport->disconnect = close_helper;
 	return 0;
 }

Re: [PATCH] Fix memory leak in transport-helper

[Johannes Schindelin]

Hi,

On Tue, 27 Oct 2009, Daniel Barkalow wrote:

> diff --git a/transport-helper.c b/transport-helper.c
> index f57e84c..479539d 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -67,6 +67,13 @@ static int disconnect_helper(struct transport *transport)
>  	return 0;
>  }
>  
> +static int close_helper(struct transport *transport)
> +{
> +	disconnect_helper(transport);
> +	free(transport->data);
> +	return 0;
> +}

Why did you not leech the transport->data = NULL; part from Peff/Sverre's 
patch?

Ciao,
Dscho

Re: [PATCH] Fix memory leak in transport-helper

[Daniel Barkalow]

On Tue, 27 Oct 2009, Johannes Schindelin wrote:

> Hi,
> 
> On Tue, 27 Oct 2009, Daniel Barkalow wrote:
> 
> > diff --git a/transport-helper.c b/transport-helper.c
> > index f57e84c..479539d 100644
> > --- a/transport-helper.c
> > +++ b/transport-helper.c
> > @@ -67,6 +67,13 @@ static int disconnect_helper(struct transport *transport)
> >  	return 0;
> >  }
> >  
> > +static int close_helper(struct transport *transport)
> > +{
> > +	disconnect_helper(transport);
> > +	free(transport->data);
> > +	return 0;
> > +}
> 
> Why did you not leech the transport->data = NULL; part from Peff/Sverre's 
> patch?

Because this code is only called just before transport itself is freed by 
the caller, and, in general, a transport with these methods is invalid 
without a valid transport->data. I expect that's also why Peff called it a 
hack not appropriate for actual application.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Fix memory leak in transport-helper

[Jeff King]

On Tue, Oct 27, 2009 at 01:37:19PM -0400, Daniel Barkalow wrote:

> > Why did you not leech the transport->data = NULL; part from Peff/Sverre's 
> > patch?
> 
> Because this code is only called just before transport itself is freed by 
> the caller, and, in general, a transport with these methods is invalid 
> without a valid transport->data. I expect that's also why Peff called it a 
> hack not appropriate for actual application.

More or less. I called it a hack because I didn't even look far enough
to know whether or not that was the case. I was just trying to get it to
stop segfaulting so Sverre could continue his work.

-Peff

Re: [PATCH] Fix memory leak in transport-helper

[Johannes Schindelin]

Hi,

On Tue, 27 Oct 2009, Daniel Barkalow wrote:

> On Tue, 27 Oct 2009, Johannes Schindelin wrote:
> 
> > Hi,
> > 
> > On Tue, 27 Oct 2009, Daniel Barkalow wrote:
> > 
> > > diff --git a/transport-helper.c b/transport-helper.c
> > > index f57e84c..479539d 100644
> > > --- a/transport-helper.c
> > > +++ b/transport-helper.c
> > > @@ -67,6 +67,13 @@ static int disconnect_helper(struct transport *transport)
> > >  	return 0;
> > >  }
> > >  
> > > +static int close_helper(struct transport *transport)
> > > +{
> > > +	disconnect_helper(transport);
> > > +	free(transport->data);
> > > +	return 0;
> > > +}
> > 
> > Why did you not leech the transport->data = NULL; part from Peff/Sverre's 
> > patch?
> 
> Because this code is only called just before transport itself is freed by 
> the caller, and, in general, a transport with these methods is invalid 
> without a valid transport->data. I expect that's also why Peff called it a 
> hack not appropriate for actual application.

So you mean to imply that this method is not about closing, but about 
releasing the structure.  Right?

Ciao,
Dscho

Re: [PATCH] Fix memory leak in transport-helper

[Daniel Barkalow]

On Tue, 27 Oct 2009, Johannes Schindelin wrote:

> Hi,
> 
> On Tue, 27 Oct 2009, Daniel Barkalow wrote:
> 
> > On Tue, 27 Oct 2009, Johannes Schindelin wrote:
> > 
> > > Hi,
> > > 
> > > On Tue, 27 Oct 2009, Daniel Barkalow wrote:
> > > 
> > > > diff --git a/transport-helper.c b/transport-helper.c
> > > > index f57e84c..479539d 100644
> > > > --- a/transport-helper.c
> > > > +++ b/transport-helper.c
> > > > @@ -67,6 +67,13 @@ static int disconnect_helper(struct transport *transport)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static int close_helper(struct transport *transport)
> > > > +{
> > > > +	disconnect_helper(transport);
> > > > +	free(transport->data);
> > > > +	return 0;
> > > > +}
> > > 
> > > Why did you not leech the transport->data = NULL; part from Peff/Sverre's 
> > > patch?
> > 
> > Because this code is only called just before transport itself is freed by 
> > the caller, and, in general, a transport with these methods is invalid 
> > without a valid transport->data. I expect that's also why Peff called it a 
> > hack not appropriate for actual application.
> 
> So you mean to imply that this method is not about closing, but about 
> releasing the structure.  Right?

Yes, that's the word I was failing to come up with last night, thanks.
Junio, "s/close/release/g" on that patch should improve comprehensibility 
greatly. (And changing the transport method name would probably also 
improve matters)

	-Daniel
*This .sig left intentionally blank*

Re: [RFC PATCH v3 10/17] Move WebDAV HTTP push under remote-curl

[Shawn O. Pearce]

Tay Ray Chuan <rctay89@gmail.com> wrote:
> On Thu, Oct 15, 2009 at 11:36 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> > diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
> > @@ -34,6 +34,10 @@ Commands are given by the caller on the helper's standard input, one per line.
> >        value of the ref. A space-separated list of attributes follows
> >        the name; unrecognized attributes are ignored. After the
> >        complete list, outputs a blank line.
> > ++
> > +If 'push' is supported this may be called as 'list for-push'
> > +to obtain the current refs prior to sending one or more 'push'
> > +commands to the helper.
> 
> The new paragraph should have the same indentation as 'list'.

Isn't the style I have here the standard way of adding a second
paragraph to a list item?
 
> It would have been great if you implemented this as a filter, such that
> 'list <attr>' lists the refs with the specified attribute <attr>,
> rather than hardcoding it.

Ack, will fix.
 
> > @@ -59,6 +63,22 @@ suitably updated.
> > +When the push is complete, outputs one or more 'ok <dst>' or
> > +'error <dst> <why>?' lines to indicate success or failure of
> > +each pushed ref.  The status report output is terminated by
> > +a blank line.  The option field <why> may be quoted in a C
> > +style string if it contains an LF.
> 
> You should mention that this behaviour only occurs when the
> --helper-status option is used.

No.  This manual page is about what a helper program must do, so I
mean what "git-remote-curl" must do.  The fact that git-remote-curl
honors this output format via passing the --helper-status option
into a command it executes is an implementation detail.

> > @@ -106,6 +132,11 @@ OPTIONS
> > +'option dry-run' \{'true'|'false'\}:
> > +       If true, pretend like the operation completed successfully,
> > +       but don't actually change any repository data.  For most
> > +       helpers this only applies to the 'push', if supported.
> > +
> 
> The 'like' after 'pretend' can be, like, removed. :)

Thanks, fixed.
 
> Two more areas in http-push.c that should have status messages
> (generated on top of pu):

I folded your patches into my series, thanks.
 
-- 
Shawn.

Re: [PATCH] Fix memory leak in transport-helper

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Tue, 27 Oct 2009, Johannes Schindelin wrote:
>
>> So you mean to imply that this method is not about closing, but about 
>> releasing the structure.  Right?
>
> Yes, that's the word I was failing to come up with last night, thanks.
> Junio, "s/close/release/g" on that patch should improve comprehensibility 
> greatly. (And changing the transport method name would probably also 
> improve matters)

Thanks, both.

Re: [RFC PATCH v3 10/17] Move WebDAV HTTP push under remote-curl

[Tay Ray Chuan]

Hi,

On Wed, Oct 28, 2009 at 9:08 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> Isn't the style I have here the standard way of adding a second
> paragraph to a list item?

sorry, I didn't catch the '+'-line style.

> I folded your patches into my series, thanks.

You're welcome.

-- 
Cheers,
Ray Chuan