transports: buffer the git requests before sending them

Trying to send every single line immediately won't give us any speed
improvement and duplicates the code we need for other transports. Make
the git transport use the same buffer functions as HTTP.

include/git2/indexer.h

@@ -41,7 +41,7 @@ GIT_EXTERN(int) git_indexer_stream_new(git_indexer_stream **out, const char *git
  * @param size the size of the data
  * @param stats stat storage
  */
-GIT_EXTERN(int) git_indexer_stream_add(git_indexer_stream *idx, void *data, size_t size, git_indexer_stats *stats);
+GIT_EXTERN(int) git_indexer_stream_add(git_indexer_stream *idx, const void *data, size_t size, git_indexer_stats *stats);
 
 /**
  * Finalize the pack and index

include/git2/remote.h

@@ -11,6 +11,7 @@
 #include "repository.h"
 #include "refspec.h"
 #include "net.h"
+#include "indexer.h"
 
 /**
  * @file git2/remote.h

src/fetch.c

@@ -161,3 +161,44 @@ int git_fetch__download_pack(
 	git_indexer_stream_free(idx);
 	return -1;
 }
+
+int git_fetch_setup_walk(git_revwalk **out, git_repository *repo)
+{
+	git_revwalk *walk;
+	git_strarray refs;
+	unsigned int i;
+	git_reference *ref;
+
+	if (git_reference_listall(&refs, repo, GIT_REF_LISTALL) < 0)
+		return -1;
+
+	if (git_revwalk_new(&walk, repo) < 0)
+		return -1;
+
+	git_revwalk_sorting(walk, GIT_SORT_TIME);
+
+	for (i = 0; i < refs.count; ++i) {
+		/* No tags */
+		if (!git__prefixcmp(refs.strings[i], GIT_REFS_TAGS_DIR))
+			continue;
+
+		if (git_reference_lookup(&ref, repo, refs.strings[i]) < 0)
+			goto on_error;
+
+		if (git_reference_type(ref) == GIT_REF_SYMBOLIC)
+			continue;
+		if (git_revwalk_push(walk, git_reference_oid(ref)) < 0)
+			goto on_error;
+
+		git_reference_free(ref);
+	}
+
+	git_strarray_free(&refs);
+	*out = walk;
+	return 0;
+
+on_error:
+	git_reference_free(ref);
+	git_strarray_free(&refs);
+	return -1;
+}

src/fetch.h

@@ -14,5 +14,6 @@ int git_fetch_download_pack(git_remote *remote, git_off_t *bytes, git_indexer_st
 
 int git_fetch__download_pack(const char *buffered, size_t buffered_size, GIT_SOCKET fd,
 			     git_repository *repo, git_off_t *bytes, git_indexer_stats *stats);
+int git_fetch_setup_walk(git_revwalk **out, git_repository *repo);
 
 #endif

src/indexer.c

@@ -288,7 +288,7 @@ static int hash_and_save(git_indexer_stream *idx, git_rawobj *obj, git_off_t ent
 	return -1;
 }
 
-int git_indexer_stream_add(git_indexer_stream *idx, void *data, size_t size, git_indexer_stats *stats)
+int git_indexer_stream_add(git_indexer_stream *idx, const void *data, size_t size, git_indexer_stats *stats)
 {
 	int error;
 	struct git_pack_header hdr;

src/pkt.c

@@ -287,19 +287,6 @@ static int buffer_want_with_caps(git_remote_head *head, git_transport_caps *caps
 	return git_buf_printf(buf, "%04xwant %s%c%s\n", len, oid, 0, capstr);
 }
 
-static int send_want_with_caps(git_remote_head *head, git_transport_caps *caps, GIT_SOCKET fd)
-{
-	git_buf buf = GIT_BUF_INIT;
-	int ret;
-
-	if (buffer_want_with_caps(head, caps, &buf) < 0)
-		return -1;
-
-	ret = gitno_send(fd, buf.ptr, buf.size, 0);
-	git_buf_free(&buf);
-	return ret;
-}
-
 /*
  * All "want" packets have the same length and format, so what we do
  * is overwrite the OID each time.
@@ -341,47 +328,6 @@ int git_pkt_buffer_wants(const git_vector *refs, git_transport_caps *caps, git_b
 	return git_pkt_buffer_flush(buf);
 }
 
-int git_pkt_send_wants(const git_vector *refs, git_transport_caps *caps, GIT_SOCKET fd)
-{
-	unsigned int i = 0;
-	char buf[sizeof(pkt_want_prefix) + GIT_OID_HEXSZ + 1];
-	git_remote_head *head;
-
-	memcpy(buf, pkt_want_prefix, strlen(pkt_want_prefix));
-	buf[sizeof(buf) - 2] = '\n';
-	buf[sizeof(buf) - 1] = '\0';
-
-	/* If there are common caps, find the first one */
-	if (caps->common) {
-		for (; i < refs->length; ++i) {
-			head = refs->contents[i];
-			if (head->local)
-				continue;
-			else
-				break;
-		}
-
-		if (send_want_with_caps(refs->contents[i], caps, fd) < 0)
-			return -1;
-
-		/* Increase it here so it's correct whether we run this or not */
-		i++;
-	}
-
-	/* Continue from where we left off */
-	for (; i < refs->length; ++i) {
-		head = refs->contents[i];
-		if (head->local)
-			continue;
-
-		git_oid_fmt(buf + strlen(pkt_want_prefix), &head->oid);
-		if (gitno_send(fd, buf, strlen(buf), 0) < 0)
-			return -1;
-	}
-
-	return git_pkt_send_flush(fd);
-}
-
 int git_pkt_buffer_have(git_oid *oid, git_buf *buf)
 {
 	char oidhex[GIT_OID_HEXSZ + 1];
@@ -391,21 +337,7 @@ int git_pkt_buffer_have(git_oid *oid, git_buf *buf)
 	return git_buf_printf(buf, "%s%s\n", pkt_have_prefix, oidhex);
 }
 
-int git_pkt_send_have(git_oid *oid, GIT_SOCKET fd)
-{
-	char buf[] = "0032have 0000000000000000000000000000000000000000\n";
-
-	git_oid_fmt(buf + strlen(pkt_have_prefix), oid);
-	return gitno_send(fd, buf, strlen(buf), 0);
-}
-
-
 int git_pkt_buffer_done(git_buf *buf)
 {
 	return git_buf_puts(buf, pkt_done_str);
 }
-
-int git_pkt_send_done(GIT_SOCKET fd)
-{
-	return gitno_send(fd, pkt_done_str, strlen(pkt_done_str), 0);
-}

src/pkt.h

@@ -68,11 +68,8 @@ int git_pkt_parse_line(git_pkt **head, const char *line, const char **out, size_
 int git_pkt_buffer_flush(git_buf *buf);
 int git_pkt_send_flush(GIT_SOCKET s);
 int git_pkt_buffer_done(git_buf *buf);
-int git_pkt_send_done(GIT_SOCKET s);
 int git_pkt_buffer_wants(const git_vector *refs, git_transport_caps *caps, git_buf *buf);
-int git_pkt_send_wants(const git_vector *refs, git_transport_caps *caps, GIT_SOCKET fd);
 int git_pkt_buffer_have(git_oid *oid, git_buf *buf);
-int git_pkt_send_have(git_oid *oid, GIT_SOCKET fd);
 void git_pkt_free(git_pkt *pkt);
 
 #endif

src/transport.h

@@ -8,6 +8,7 @@
 #define INCLUDE_transport_h__
 
 #include "git2/net.h"
+#include "git2/indexer.h"
 #include "vector.h"
 
 #define GIT_CAP_OFS_DELTA "ofs-delta"
@@ -65,19 +66,11 @@ struct git_transport {
 	 * Push the changes over
 	 */
 	int (*push)(struct git_transport *transport);
-	/**
-	 * Send a 'done' message
-	 */
-	int (*send_done)(struct git_transport *transport);
 	/**
 	 * Negotiate the minimal amount of objects that need to be
 	 * retrieved
 	 */
 	int (*negotiate_fetch)(struct git_transport *transport, git_repository *repo, const git_vector *wants);
-	/**
-	 * Send a flush
-	 */
-	int (*send_flush)(struct git_transport *transport);
 	/**
 	 * Download the packfile
 	 */

src/transports/git.c

@@ -293,54 +293,41 @@ static int git_negotiate_fetch(git_transport *transport, git_repository *repo, c
 {
 	transport_git *t = (transport_git *) transport;
 	git_revwalk *walk;
-	git_reference *ref;
-	git_strarray refs;
 	git_oid oid;
 	int error;
 	unsigned int i;
+	git_buf data = GIT_BUF_INIT;
 	gitno_buffer *buf = &t->buf;
 
-	if (git_pkt_send_wants(wants, &t->caps, t->socket) < 0)
+	if (git_pkt_buffer_wants(wants, &t->caps, &data) < 0)
 		return -1;
 
-	if (git_reference_listall(&refs, repo, GIT_REF_LISTALL) < 0)
-		return -1;
-
-	if (git_revwalk_new(&walk, repo) < 0)
-		return -1;
-
-	git_revwalk_sorting(walk, GIT_SORT_TIME);
-
-	for (i = 0; i < refs.count; ++i) {
-		/* No tags */
-		if (!git__prefixcmp(refs.strings[i], GIT_REFS_TAGS_DIR))
-			continue;
-
-		if (git_reference_lookup(&ref, repo, refs.strings[i]) < 0)
-			goto on_error;
-
-		if (git_reference_type(ref) == GIT_REF_SYMBOLIC)
-			continue;
-
-		if ((error = git_revwalk_push(walk, git_reference_oid(ref))) < 0)
-			goto on_error;
+	if (git_fetch_setup_walk(&walk, repo) < 0)
+		goto on_error;
 
-	}
-	git_strarray_free(&refs);
+	if (gitno_send(t->socket, data.ptr, data.size, 0) < 0)
+		goto on_error;
 
+	git_buf_clear(&data);
 	/*
 	 * We don't support any kind of ACK extensions, so the negotiation
 	 * boils down to sending what we have and listening for an ACK
 	 * every once in a while.
 	 */
 	i = 0;
 	while ((error = git_revwalk_next(&oid, walk)) == 0) {
-		error = git_pkt_send_have(&oid, t->socket);
+		git_pkt_buffer_have(&oid, &data);
 		i++;
 		if (i % 20 == 0) {
 			int pkt_type;
 
-			git_pkt_send_flush(t->socket);
+			git_pkt_buffer_flush(&data);
+			if (git_buf_oom(&data))
+				goto on_error;
+
+			if (gitno_send(t->socket, data.ptr, data.size, 0) < 0)
+				goto on_error;
+
 			pkt_type = recv_pkt(buf);
 
 			if (pkt_type == GIT_PKT_ACK) {
@@ -354,34 +341,26 @@ static int git_negotiate_fetch(git_transport *transport, git_repository *repo, c
 
 		}
 	}
-	if (error != GIT_EREVWALKOVER)
+	if (error < 0 && error != GIT_EREVWALKOVER)
 		goto on_error;
 
-	git_pkt_send_flush(t->socket);
-	git_pkt_send_done(t->socket);
+	/* Tell the other end that we're done negotiating */
+	git_buf_clear(&data);
+	git_pkt_buffer_flush(&data);
+	git_pkt_buffer_done(&data);
+	if (gitno_send(t->socket, data.ptr, data.size, 0) < 0)
+		goto on_error;
 
+	git_buf_free(&data);
 	git_revwalk_free(walk);
 	return 0;
 
 on_error:
+	git_buf_free(&data);
 	git_revwalk_free(walk);
 	return -1;
 }
 
-static int git_send_flush(git_transport *transport)
-{
-	transport_git *t = (transport_git *) transport;
-
-	return git_pkt_send_flush(t->socket);
-}
-
-static int git_send_done(git_transport *transport)
-{
-	transport_git *t = (transport_git *) transport;
-
-	return git_pkt_send_done(t->socket);
-}
-
 static int git_download_pack(git_transport *transport, git_repository *repo, git_off_t *bytes, git_indexer_stats *stats)
 {
 	transport_git *t = (transport_git *) transport;
@@ -424,7 +403,6 @@ static int git_download_pack(git_transport *transport, git_repository *repo, git
 	return read_bytes;
 }
 
-
 static int git_close(git_transport *transport)
 {
 	transport_git *t = (transport_git*) transport;
@@ -476,8 +454,6 @@ int git_transport_git(git_transport **out)
 	t->parent.connect = git_connect;
 	t->parent.ls = git_ls;
 	t->parent.negotiate_fetch = git_negotiate_fetch;
-	t->parent.send_flush = git_send_flush;
-	t->parent.send_done = git_send_done;
 	t->parent.download_pack = git_download_pack;
 	t->parent.close = git_close;
 	t->parent.free = git_free;

src/transports/http.c

@@ -410,44 +410,6 @@ static int parse_response(transport_http *t)
 	return ret;
 }
 
-static int setup_walk(git_revwalk **out, git_repository *repo)
-{
-	git_revwalk *walk;
-	git_strarray refs;
-	unsigned int i;
-	git_reference *ref;
-
-	if (git_reference_listall(&refs, repo, GIT_REF_LISTALL) < 0)
-		return -1;
-
-	if (git_revwalk_new(&walk, repo) < 0)
-		return -1;
-
-	git_revwalk_sorting(walk, GIT_SORT_TIME);
-
-	for (i = 0; i < refs.count; ++i) {
-		/* No tags */
-		if (!git__prefixcmp(refs.strings[i], GIT_REFS_TAGS_DIR))
-			continue;
-
-		if (git_reference_lookup(&ref, repo, refs.strings[i]) < 0)
-			goto on_error;
-
-		if (git_reference_type(ref) == GIT_REF_SYMBOLIC)
-			continue;
-		if (git_revwalk_push(walk, git_reference_oid(ref)) < 0)
-			goto on_error;
-	}
-
-	git_strarray_free(&refs);
-	*out = walk;
-	return 0;
-
-on_error:
-	git_strarray_free(&refs);
-	return -1;
-}
-
 static int http_negotiate_fetch(git_transport *transport, git_repository *repo, const git_vector *wants)
 {
 	transport_http *t = (transport_http *) transport;
@@ -470,7 +432,7 @@ static int http_negotiate_fetch(git_transport *transport, git_repository *repo,
 	if (git_vector_init(common, 16, NULL) < 0)
 		return -1;
 
-	if (setup_walk(&walk, repo) < 0)
+	if (git_fetch_setup_walk(&walk, repo) < 0)
 		return -1;
 
 	do {