[PATCH 1/4] Refactoring: remove duplicated code from transport.c and builtin-send-pack.c

[PATCH 1/4] Refactoring: remove duplicated code from transport.c and builtin-send-pack.c

[Michael Lukashov]

The following functions are duplicated:

  verify_remote_names
  update_tracking_ref
  print_ref_status
  status_abbrev
  print_ok_ref_status
  print_one_push_status
  refs_pushed
  print_push_status

Signed-off-by: Michael Lukashov <michael.lukashov@gmail.com>
---
 builtin-send-pack.c |   89 ++++++++++++++----------
 send-pack.h         |   20 +++++
 transport.c         |  196 ---------------------------------------------------
 3 files changed, 72 insertions(+), 233 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 76c7206..616811a 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -169,7 +169,7 @@ static int receive_status(int in, struct ref *refs)
 	return ret;
 }
 
-static void update_tracking_ref(struct remote *remote, struct ref *ref)
+void update_tracking_ref(struct remote *remote, struct ref *ref, int verbose)
 {
 	struct refspec rs;
 
@@ -180,7 +180,7 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
 	rs.dst = NULL;
 
 	if (!remote_find_tracking(remote, &rs)) {
-		if (args.verbose)
+		if (verbose)
 			fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
 		if (ref->deletion) {
 			delete_ref(rs.dst, NULL, 0);
@@ -191,37 +191,47 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
 	}
 }
 
-#define SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
-
-static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg)
+void print_ref_status(char flag, const char *summary, struct ref *to,
+		  struct ref *from, const char *msg, int porcelain)
 {
-	fprintf(stderr, " %c %-*s ", flag, SUMMARY_WIDTH, summary);
-	if (from)
-		fprintf(stderr, "%s -> %s", prettify_refname(from->name), prettify_refname(to->name));
-	else
-		fputs(prettify_refname(to->name), stderr);
-	if (msg) {
-		fputs(" (", stderr);
-		fputs(msg, stderr);
-		fputc(')', stderr);
+	if (porcelain) {
+		if (from)
+			fprintf(stdout, "%c\t%s:%s\t", flag, from->name, to->name);
+		else
+			fprintf(stdout, "%c\t:%s\t", flag, to->name);
+		if (msg)
+			fprintf(stdout, "%s (%s)\n", summary, msg);
+		else
+			fprintf(stdout, "%s\n", summary);
+	} else {
+		fprintf(stderr, " %c %-*s ", flag, SUMMARY_WIDTH, summary);
+		if (from)
+			fprintf(stderr, "%s -> %s", prettify_refname(from->name), prettify_refname(to->name));
+		else
+			fputs(prettify_refname(to->name), stderr);
+		if (msg) {
+			fputs(" (", stderr);
+			fputs(msg, stderr);
+			fputc(')', stderr);
+		}
+		fputc('\n', stderr);
 	}
-	fputc('\n', stderr);
 }
 
-static const char *status_abbrev(unsigned char sha1[20])
+const char *status_abbrev(unsigned char sha1[20])
 {
 	return find_unique_abbrev(sha1, DEFAULT_ABBREV);
 }
 
-static void print_ok_ref_status(struct ref *ref)
+void print_ok_ref_status(struct ref *ref, int porcelain)
 {
 	if (ref->deletion)
-		print_ref_status('-', "[deleted]", ref, NULL, NULL);
+		print_ref_status('-', "[deleted]", ref, NULL, NULL, porcelain);
 	else if (is_null_sha1(ref->old_sha1))
 		print_ref_status('*',
 			(!prefixcmp(ref->name, "refs/tags/") ? "[new tag]" :
 			  "[new branch]"),
-			ref, ref->peer_ref, NULL);
+			ref, ref->peer_ref, NULL, porcelain);
 	else {
 		char quickref[84];
 		char type;
@@ -239,73 +249,77 @@ static void print_ok_ref_status(struct ref *ref)
 		}
 		strcat(quickref, status_abbrev(ref->new_sha1));
 
-		print_ref_status(type, quickref, ref, ref->peer_ref, msg);
+		print_ref_status(type, quickref, ref, ref->peer_ref, msg, porcelain);
 	}
 }
 
-static int print_one_push_status(struct ref *ref, const char *dest, int count)
+int print_one_push_status(struct ref *ref, const char *dest, int count, int porcelain)
 {
 	if (!count)
 		fprintf(stderr, "To %s\n", dest);
 
 	switch(ref->status) {
 	case REF_STATUS_NONE:
-		print_ref_status('X', "[no match]", ref, NULL, NULL);
+		print_ref_status('X', "[no match]", ref, NULL, NULL, porcelain);
 		break;
 	case REF_STATUS_REJECT_NODELETE:
 		print_ref_status('!', "[rejected]", ref, NULL,
-				"remote does not support deleting refs");
+				 "remote does not support deleting refs", porcelain);
 		break;
 	case REF_STATUS_UPTODATE:
 		print_ref_status('=', "[up to date]", ref,
-				ref->peer_ref, NULL);
+				 ref->peer_ref, NULL, porcelain);
 		break;
 	case REF_STATUS_REJECT_NONFASTFORWARD:
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
-				"non-fast-forward");
+				 "non-fast-forward", porcelain);
 		break;
 	case REF_STATUS_REMOTE_REJECT:
 		print_ref_status('!', "[remote rejected]", ref,
 				ref->deletion ? NULL : ref->peer_ref,
-				ref->remote_status);
+						 ref->remote_status, porcelain);
 		break;
 	case REF_STATUS_EXPECTING_REPORT:
 		print_ref_status('!', "[remote failure]", ref,
 				ref->deletion ? NULL : ref->peer_ref,
-				"remote failed to report status");
+				"remote failed to report status", porcelain);
 		break;
 	case REF_STATUS_OK:
-		print_ok_ref_status(ref);
+		print_ok_ref_status(ref, porcelain);
 		break;
 	}
 
 	return 1;
 }
 
-static void print_push_status(const char *dest, struct ref *refs)
+void print_push_status(const char *dest, struct ref *refs,
+		  int verbose, int porcelain, int *nonfastforward)
 {
 	struct ref *ref;
 	int n = 0;
 
-	if (args.verbose) {
+	if (verbose) {
 		for (ref = refs; ref; ref = ref->next)
 			if (ref->status == REF_STATUS_UPTODATE)
-				n += print_one_push_status(ref, dest, n);
+				n += print_one_push_status(ref, dest, n, porcelain);
 	}
 
 	for (ref = refs; ref; ref = ref->next)
 		if (ref->status == REF_STATUS_OK)
-			n += print_one_push_status(ref, dest, n);
+			n += print_one_push_status(ref, dest, n, porcelain);
 
+	*nonfastforward = 0;
 	for (ref = refs; ref; ref = ref->next) {
 		if (ref->status != REF_STATUS_NONE &&
 		    ref->status != REF_STATUS_UPTODATE &&
 		    ref->status != REF_STATUS_OK)
-			n += print_one_push_status(ref, dest, n);
+			n += print_one_push_status(ref, dest, n, porcelain);
+		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD)
+			*nonfastforward = 1;
 	}
 }
 
-static int refs_pushed(struct ref *ref)
+int refs_pushed(struct ref *ref)
 {
 	for (; ref; ref = ref->next) {
 		switch(ref->status) {
@@ -489,7 +503,7 @@ int send_pack(struct send_pack_args *args,
 	return 0;
 }
 
-static void verify_remote_names(int nr_heads, const char **heads)
+void verify_remote_names(int nr_heads, const char **heads)
 {
 	int i;
 
@@ -536,6 +550,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	int send_all = 0;
 	const char *receivepack = "git-receive-pack";
 	int flags;
+	int nonfastforward = 0;
 
 	argv++;
 	for (i = 1; i < argc; i++, argv++) {
@@ -657,12 +672,12 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	ret |= finish_connect(conn);
 
 	if (!helper_status)
-		print_push_status(dest, remote_refs);
+		print_push_status(dest, remote_refs, args.verbose, 0, &nonfastforward);
 
 	if (!args.dry_run && remote) {
 		struct ref *ref;
 		for (ref = remote_refs; ref; ref = ref->next)
-			update_tracking_ref(remote, ref);
+			update_tracking_ref(remote, ref, args.verbose);
 	}
 
 	if (!ret && !refs_pushed(remote_refs))
diff --git a/send-pack.h b/send-pack.h
index 28141ac..decc23c 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -16,4 +16,24 @@ int send_pack(struct send_pack_args *args,
 	      int fd[], struct child_process *conn,
 	      struct ref *remote_refs, struct extra_have_objects *extra_have);
 
+void verify_remote_names(int nr_heads, const char **heads);
+
+void update_tracking_ref(struct remote *remote, struct ref *ref, int verbose);
+
+#define SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
+
+void print_ref_status(char flag, const char *summary, struct ref *to,
+		  struct ref *from, const char *msg, int porcelain);
+
+const char *status_abbrev(unsigned char sha1[20]);
+
+void print_ok_ref_status(struct ref *ref, int porcelain);
+
+int print_one_push_status(struct ref *ref, const char *dest, int count, int porcelain);
+
+int refs_pushed(struct ref *ref);
+
+void print_push_status(const char *dest, struct ref *refs,
+		  int verbose, int porcelain, int *nonfastforward);
+
 #endif
diff --git a/transport.c b/transport.c
index 3846aac..aace286 100644
--- a/transport.c
+++ b/transport.c
@@ -573,202 +573,6 @@ static int push_had_errors(struct ref *ref)
 	return 0;
 }
 
-static int refs_pushed(struct ref *ref)
-{
-	for (; ref; ref = ref->next) {
-		switch(ref->status) {
-		case REF_STATUS_NONE:
-		case REF_STATUS_UPTODATE:
-			break;
-		default:
-			return 1;
-		}
-	}
-	return 0;
-}
-
-static void update_tracking_ref(struct remote *remote, struct ref *ref, int verbose)
-{
-	struct refspec rs;
-
-	if (ref->status != REF_STATUS_OK && ref->status != REF_STATUS_UPTODATE)
-		return;
-
-	rs.src = ref->name;
-	rs.dst = NULL;
-
-	if (!remote_find_tracking(remote, &rs)) {
-		if (verbose)
-			fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
-		if (ref->deletion) {
-			delete_ref(rs.dst, NULL, 0);
-		} else
-			update_ref("update by push", rs.dst,
-					ref->new_sha1, NULL, 0, 0);
-		free(rs.dst);
-	}
-}
-
-#define SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
-
-static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg, int porcelain)
-{
-	if (porcelain) {
-		if (from)
-			fprintf(stdout, "%c\t%s:%s\t", flag, from->name, to->name);
-		else
-			fprintf(stdout, "%c\t:%s\t", flag, to->name);
-		if (msg)
-			fprintf(stdout, "%s (%s)\n", summary, msg);
-		else
-			fprintf(stdout, "%s\n", summary);
-	} else {
-		fprintf(stderr, " %c %-*s ", flag, SUMMARY_WIDTH, summary);
-		if (from)
-			fprintf(stderr, "%s -> %s", prettify_refname(from->name), prettify_refname(to->name));
-		else
-			fputs(prettify_refname(to->name), stderr);
-		if (msg) {
-			fputs(" (", stderr);
-			fputs(msg, stderr);
-			fputc(')', stderr);
-		}
-		fputc('\n', stderr);
-	}
-}
-
-static const char *status_abbrev(unsigned char sha1[20])
-{
-	return find_unique_abbrev(sha1, DEFAULT_ABBREV);
-}
-
-static void print_ok_ref_status(struct ref *ref, int porcelain)
-{
-	if (ref->deletion)
-		print_ref_status('-', "[deleted]", ref, NULL, NULL, porcelain);
-	else if (is_null_sha1(ref->old_sha1))
-		print_ref_status('*',
-			(!prefixcmp(ref->name, "refs/tags/") ? "[new tag]" :
-			"[new branch]"),
-			ref, ref->peer_ref, NULL, porcelain);
-	else {
-		char quickref[84];
-		char type;
-		const char *msg;
-
-		strcpy(quickref, status_abbrev(ref->old_sha1));
-		if (ref->nonfastforward) {
-			strcat(quickref, "...");
-			type = '+';
-			msg = "forced update";
-		} else {
-			strcat(quickref, "..");
-			type = ' ';
-			msg = NULL;
-		}
-		strcat(quickref, status_abbrev(ref->new_sha1));
-
-		print_ref_status(type, quickref, ref, ref->peer_ref, msg, porcelain);
-	}
-}
-
-static int print_one_push_status(struct ref *ref, const char *dest, int count, int porcelain)
-{
-	if (!count)
-		fprintf(stderr, "To %s\n", dest);
-
-	switch(ref->status) {
-	case REF_STATUS_NONE:
-		print_ref_status('X', "[no match]", ref, NULL, NULL, porcelain);
-		break;
-	case REF_STATUS_REJECT_NODELETE:
-		print_ref_status('!', "[rejected]", ref, NULL,
-						 "remote does not support deleting refs", porcelain);
-		break;
-	case REF_STATUS_UPTODATE:
-		print_ref_status('=', "[up to date]", ref,
-						 ref->peer_ref, NULL, porcelain);
-		break;
-	case REF_STATUS_REJECT_NONFASTFORWARD:
-		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
-						 "non-fast-forward", porcelain);
-		break;
-	case REF_STATUS_REMOTE_REJECT:
-		print_ref_status('!', "[remote rejected]", ref,
-						 ref->deletion ? NULL : ref->peer_ref,
-						 ref->remote_status, porcelain);
-		break;
-	case REF_STATUS_EXPECTING_REPORT:
-		print_ref_status('!', "[remote failure]", ref,
-						 ref->deletion ? NULL : ref->peer_ref,
-						 "remote failed to report status", porcelain);
-		break;
-	case REF_STATUS_OK:
-		print_ok_ref_status(ref, porcelain);
-		break;
-	}
-
-	return 1;
-}
-
-static void print_push_status(const char *dest, struct ref *refs,
-			      int verbose, int porcelain, int * nonfastforward)
-{
-	struct ref *ref;
-	int n = 0;
-
-	if (verbose) {
-		for (ref = refs; ref; ref = ref->next)
-			if (ref->status == REF_STATUS_UPTODATE)
-				n += print_one_push_status(ref, dest, n, porcelain);
-	}
-
-	for (ref = refs; ref; ref = ref->next)
-		if (ref->status == REF_STATUS_OK)
-			n += print_one_push_status(ref, dest, n, porcelain);
-
-	*nonfastforward = 0;
-	for (ref = refs; ref; ref = ref->next) {
-		if (ref->status != REF_STATUS_NONE &&
-		    ref->status != REF_STATUS_UPTODATE &&
-		    ref->status != REF_STATUS_OK)
-			n += print_one_push_status(ref, dest, n, porcelain);
-		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD)
-			*nonfastforward = 1;
-	}
-}
-
-static void verify_remote_names(int nr_heads, const char **heads)
-{
-	int i;
-
-	for (i = 0; i < nr_heads; i++) {
-		const char *local = heads[i];
-		const char *remote = strrchr(heads[i], ':');
-
-		if (*local == '+')
-			local++;
-
-		/* A matching refspec is okay.  */
-		if (remote == local && remote[1] == '\0')
-			continue;
-
-		remote = remote ? (remote + 1) : local;
-		switch (check_ref_format(remote)) {
-		case 0: /* ok */
-		case CHECK_REF_FORMAT_ONELEVEL:
-			/* ok but a single level -- that is fine for
-			 * a match pattern.
-			 */
-		case CHECK_REF_FORMAT_WILDCARD:
-			/* ok but ends with a pattern-match character */
-			continue;
-		}
-		die("remote part of refspec is not a valid name in %s",
-		    heads[i]);
-	}
-}
-
 static int git_transport_push(struct transport *transport, struct ref *remote_refs, int flags)
 {
 	struct git_transport_data *data = transport->data;
-- 
1.7.0.1571.g856c2

[PATCH 2/4] Refactoring: connect.c: move duplicated code to get_host_and_port

[Michael Lukashov]

The following functions:

  git_tcp_connect_sock
  git_tcp_connect_sock,
  git_proxy_connect

have common block of code, which is moved to get_host_and_port

Signed-off-by: Michael Lukashov <michael.lukashov@gmail.com>
---
 connect.c |   83 +++++++++++++++++++++---------------------------------------
 1 files changed, 29 insertions(+), 54 deletions(-)

diff --git a/connect.c b/connect.c
index 20054e4..616b312 100644
--- a/connect.c
+++ b/connect.c
@@ -152,6 +152,30 @@ static enum protocol get_protocol(const char *name)
 #define STR_(s)	# s
 #define STR(s)	STR_(s)
 
+static void get_host_and_port(char **host, const char **port, int set_port_none)
+{
+	char *colon, *end;
+
+	if (*host[0] == '[') {
+		end = strchr(*host + 1, ']');
+		if (end) {
+			*end = 0;
+			end++;
+			(*host)++;
+		} else
+			end = *host;
+	} else
+		end = *host;
+	colon = strchr(end, ':');
+
+	if (colon) {
+		*colon = 0;
+		*port = colon + 1;
+		if (set_port_none && !**port)
+			*port = "<none>";
+	}
+}
+
 #ifndef NO_IPV6
 
 static const char *ai_name(const struct addrinfo *ai)
@@ -170,30 +194,12 @@ static const char *ai_name(const struct addrinfo *ai)
 static int git_tcp_connect_sock(char *host, int flags)
 {
 	int sockfd = -1, saved_errno = 0;
-	char *colon, *end;
 	const char *port = STR(DEFAULT_GIT_PORT);
 	struct addrinfo hints, *ai0, *ai;
 	int gai;
 	int cnt = 0;
 
-	if (host[0] == '[') {
-		end = strchr(host + 1, ']');
-		if (end) {
-			*end = 0;
-			end++;
-			host++;
-		} else
-			end = host;
-	} else
-		end = host;
-	colon = strchr(end, ':');
-
-	if (colon) {
-		*colon = 0;
-		port = colon + 1;
-		if (!*port)
-			port = "<none>";
-	}
+	get_host_and_port(&host, &port, 1);
 
 	memset(&hints, 0, sizeof(hints));
 	hints.ai_socktype = SOCK_STREAM;
@@ -251,30 +257,15 @@ static int git_tcp_connect_sock(char *host, int flags)
 static int git_tcp_connect_sock(char *host, int flags)
 {
 	int sockfd = -1, saved_errno = 0;
-	char *colon, *end;
-	char *port = STR(DEFAULT_GIT_PORT), *ep;
+	const char *port = STR(DEFAULT_GIT_PORT);
+	char *ep;
 	struct hostent *he;
 	struct sockaddr_in sa;
 	char **ap;
 	unsigned int nport;
 	int cnt;
 
-	if (host[0] == '[') {
-		end = strchr(host + 1, ']');
-		if (end) {
-			*end = 0;
-			end++;
-			host++;
-		} else
-			end = host;
-	} else
-		end = host;
-	colon = strchr(end, ':');
-
-	if (colon) {
-		*colon = 0;
-		port = colon + 1;
-	}
+	get_host_and_port(&host, &port, 0);
 
 	if (flags & CONNECT_VERBOSE)
 		fprintf(stderr, "Looking up %s ... ", host);
@@ -406,26 +397,10 @@ static int git_use_proxy(const char *host)
 static void git_proxy_connect(int fd[2], char *host)
 {
 	const char *port = STR(DEFAULT_GIT_PORT);
-	char *colon, *end;
 	const char *argv[4];
 	struct child_process proxy;
 
-	if (host[0] == '[') {
-		end = strchr(host + 1, ']');
-		if (end) {
-			*end = 0;
-			end++;
-			host++;
-		} else
-			end = host;
-	} else
-		end = host;
-	colon = strchr(end, ':');
-
-	if (colon) {
-		*colon = 0;
-		port = colon + 1;
-	}
+	get_host_and_port(&host, &port, 0);
 
 	argv[0] = git_proxy_command;
 	argv[1] = host;
-- 
1.7.0.1571.g856c2

[PATCH 3/4] Refactoring: move duplicated code from builtin-pack-objects.c and fast-import.c to object.c

[Michael Lukashov]

The following functions are duplicated:

  encode_header

Signed-off-by: Michael Lukashov <michael.lukashov@gmail.com>
---
 builtin-pack-objects.c |   27 ---------------------------
 fast-import.c          |   23 -----------------------
 object.c               |   20 ++++++++++++++++++++
 object.h               |    9 +++++++++
 4 files changed, 29 insertions(+), 50 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index e1d3adf..80bbcd2 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -155,33 +155,6 @@ static unsigned long do_compress(void **pptr, unsigned long size)
 }
 
 /*
- * The per-object header is a pretty dense thing, which is
- *  - first byte: low four bits are "size", then three bits of "type",
- *    and the high bit is "size continues".
- *  - each byte afterwards: low seven bits are size continuation,
- *    with the high bit being "size continues"
- */
-static int encode_header(enum object_type type, unsigned long size, unsigned char *hdr)
-{
-	int n = 1;
-	unsigned char c;
-
-	if (type < OBJ_COMMIT || type > OBJ_REF_DELTA)
-		die("bad type %d", type);
-
-	c = (type << 4) | (size & 15);
-	size >>= 4;
-	while (size) {
-		*hdr++ = c | 0x80;
-		c = size & 0x7f;
-		size >>= 7;
-		n++;
-	}
-	*hdr = c;
-	return n;
-}
-
-/*
  * we are going to reuse the existing object data as is.  make
  * sure it is not corrupt.
  */
diff --git a/fast-import.c b/fast-import.c
index b477dc6..f983338 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1013,29 +1013,6 @@ static void cycle_packfile(void)
 	start_packfile();
 }
 
-static size_t encode_header(
-	enum object_type type,
-	uintmax_t size,
-	unsigned char *hdr)
-{
-	int n = 1;
-	unsigned char c;
-
-	if (type < OBJ_COMMIT || type > OBJ_REF_DELTA)
-		die("bad type %d", type);
-
-	c = (type << 4) | (size & 15);
-	size >>= 4;
-	while (size) {
-		*hdr++ = c | 0x80;
-		c = size & 0x7f;
-		size >>= 7;
-		n++;
-	}
-	*hdr = c;
-	return n;
-}
-
 static int store_object(
 	enum object_type type,
 	struct strbuf *dat,
diff --git a/object.c b/object.c
index 3ca92c4..a06ad01 100644
--- a/object.c
+++ b/object.c
@@ -268,3 +268,23 @@ void object_array_remove_duplicates(struct object_array *array)
 		array->nr = dst;
 	}
 }
+
+int encode_header(enum object_type type, uintmax_t size, unsigned char *hdr)
+{
+	int n = 1;
+	unsigned char c;
+
+	if (type < OBJ_COMMIT || type > OBJ_REF_DELTA)
+		die("bad type %d", type);
+
+	c = (type << 4) | (size & 15);
+	size >>= 4;
+	while (size) {
+		*hdr++ = c | 0x80;
+		c = size & 0x7f;
+		size >>= 7;
+		n++;
+	}
+	*hdr = c;
+	return n;
+}
diff --git a/object.h b/object.h
index 82877c8..f5a5c77 100644
--- a/object.h
+++ b/object.h
@@ -79,4 +79,13 @@ void add_object_array(struct object *obj, const char *name, struct object_array
 void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode);
 void object_array_remove_duplicates(struct object_array *);
 
+/*
+ * The per-object header is a pretty dense thing, which is
+ *  - first byte: low four bits are "size", then three bits of "type",
+ *    and the high bit is "size continues".
+ *  - each byte afterwards: low seven bits are size continuation,
+ *    with the high bit being "size continues"
+ */
+int encode_header(enum object_type type, uintmax_t size, unsigned char *hdr);
+
 #endif /* OBJECT_H */
-- 
1.7.0.1571.g856c2

[PATCH 4/4] Refactoring: remove duplicated code from builtin-checkout.c and merge-recursive.c

[Michael Lukashov]

The following functions are duplicated:

  fill_mm

Signed-off-by: Michael Lukashov <michael.lukashov@gmail.com>
---
 builtin-checkout.c |   18 ------------------
 merge-recursive.c  |    2 +-
 merge-recursive.h  |    3 +++
 3 files changed, 4 insertions(+), 19 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 5277817..e53e857 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -128,24 +128,6 @@ static int checkout_stage(int stage, struct cache_entry *ce, int pos,
 		     (stage == 2) ? "our" : "their");
 }
 
-/* NEEDSWORK: share with merge-recursive */
-static void fill_mm(const unsigned char *sha1, mmfile_t *mm)
-{
-	unsigned long size;
-	enum object_type type;
-
-	if (!hashcmp(sha1, null_sha1)) {
-		mm->ptr = xstrdup("");
-		mm->size = 0;
-		return;
-	}
-
-	mm->ptr = read_sha1_file(sha1, &type, &size);
-	if (!mm->ptr || type != OBJ_BLOB)
-		die("unable to read blob object %s", sha1_to_hex(sha1));
-	mm->size = size;
-}
-
 static int checkout_merged(int pos, struct checkout *state)
 {
 	struct cache_entry *ce = active_cache[pos];
diff --git a/merge-recursive.c b/merge-recursive.c
index cb53b01..5999ae2 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -599,7 +599,7 @@ struct merge_file_info
 		 merge:1;
 };
 
-static void fill_mm(const unsigned char *sha1, mmfile_t *mm)
+void fill_mm(const unsigned char *sha1, mmfile_t *mm)
 {
 	unsigned long size;
 	enum object_type type;
diff --git a/merge-recursive.h b/merge-recursive.h
index be8410a..ccc4002 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -2,6 +2,7 @@
 #define MERGE_RECURSIVE_H
 
 #include "string-list.h"
+#include "xdiff/xdiff.h"
 
 struct merge_options {
 	const char *branch1;
@@ -53,4 +54,6 @@ int merge_recursive_generic(struct merge_options *o,
 void init_merge_options(struct merge_options *o);
 struct tree *write_tree_from_memory(struct merge_options *o);
 
+void fill_mm(const unsigned char *sha1, mmfile_t *mm);
+
 #endif
-- 
1.7.0.1571.g856c2

Re: [PATCH 1/4] Refactoring: remove duplicated code from transport.c and builtin-send-pack.c

[Tay Ray Chuan]

Hi,

On Mon, Feb 15, 2010 at 5:27 AM, Michael Lukashov
<michael.lukashov@gmail.com> wrote:
> The following functions are duplicated:
>
>  verify_remote_names
>  update_tracking_ref
>  print_ref_status
>  status_abbrev
>  print_ok_ref_status
>  print_one_push_status
>  refs_pushed
>  print_push_status
>
> Signed-off-by: Michael Lukashov <michael.lukashov@gmail.com>

strictly speaking, the implementation for these functions are
different. Perhaps you could advertise in the commit message that some
of the functions from builtin-send-pack.c learnt porcelain, even
though it's always off (0).

> diff --git a/builtin-send-pack.c b/builtin-send-pack.c
> index 76c7206..616811a 100644
> --- a/builtin-send-pack.c
> +++ b/builtin-send-pack.c
> [snip]
> @@ -191,37 +191,47 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
>        }
>  }
>
> -#define SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)

hmm, since this is only used internally by print_ref_status, can't
this stay here rather than being made public in send-pack.h?

-- 
Cheers,
Ray Chuan

Re: [PATCH 1/4] Refactoring: remove duplicated code from transport.c and builtin-send-pack.c

[Jeff King]

On Sun, Feb 14, 2010 at 09:27:40PM +0000, Michael Lukashov wrote:

> The following functions are duplicated:
> 
>   verify_remote_names
>   update_tracking_ref
>   print_ref_status
>   status_abbrev
>   print_ok_ref_status
>   print_one_push_status
>   refs_pushed
>   print_push_status
> 
> Signed-off-by: Michael Lukashov <michael.lukashov@gmail.com>
> ---
>  builtin-send-pack.c |   89 ++++++++++++++----------
>  send-pack.h         |   20 +++++
>  transport.c         |  196 ---------------------------------------------------

I think this is backwards. The versions in send-pack were there first,
and then were ported to transport.c so that other transports could
benefit from them. And that is where they should ultimately be.

I can't remember the exact details of why the originals were not
removed, though (I think I complained about it once before, and there
was some technical reason, but I don't recall now). Daniel (cc'd) might
remember more.

-Peff

Re: [PATCH 1/4] Refactoring: remove duplicated code from transport.c and builtin-send-pack.c

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Feb 14, 2010 at 09:27:40PM +0000, Michael Lukashov wrote:
>
>> The following functions are duplicated:
>> 
>>   verify_remote_names
>>   update_tracking_ref
>>   print_ref_status
>>   status_abbrev
>>   print_ok_ref_status
>>   print_one_push_status
>>   refs_pushed
>>   print_push_status
>> 
>> Signed-off-by: Michael Lukashov <michael.lukashov@gmail.com>
>> ---
>>  builtin-send-pack.c |   89 ++++++++++++++----------
>>  send-pack.h         |   20 +++++
>>  transport.c         |  196 ---------------------------------------------------
>
> I think this is backwards. The versions in send-pack were there first,
> and then were ported to transport.c so that other transports could
> benefit from them. And that is where they should ultimately be.
>
> I can't remember the exact details of why the originals were not
> removed, though (I think I complained about it once before, and there
> was some technical reason, but I don't recall now). Daniel (cc'd) might
> remember more.

Also the names of these functions probably need to be made more specific
so that people not so familiar with the transport code can tell that they
are from "transport" family.  The names didn't matter much while they were
file scope static, but this series changes that.

Re: [PATCH 1/4] Refactoring: remove duplicated code from transport.c and builtin-send-pack.c

[Jeff King]

On Sun, Feb 14, 2010 at 10:34:20PM -0800, Junio C Hamano wrote:

> > I can't remember the exact details of why the originals were not
> > removed, though (I think I complained about it once before, and there
> > was some technical reason, but I don't recall now). Daniel (cc'd) might
> > remember more.
> 
> Also the names of these functions probably need to be made more specific
> so that people not so familiar with the transport code can tell that they
> are from "transport" family.  The names didn't matter much while they were
> file scope static, but this series changes that.

Actually, I wonder if we can simply get rid of some of the calls in
send-pack. I think that the code in send-pack isn't even called anymore
via "git push"; it only gets called when you call send-pack directly.
And arguably send-pack as plumbing shouldn't be generating all sorts of
user-facing output. But it is a behavior change. I wonder if anybody
actually calls send-pack directly anymore. It seems like even scripts
use "git push" because of the transport agnosticism.

-Peff

Re: [PATCH 1/4] Refactoring: remove duplicated code from transport.c and builtin-send-pack.c

[Ilari Liusvaara]

On Mon, Feb 15, 2010 at 02:55:15AM -0500, Jeff King wrote:
> On Sun, Feb 14, 2010 at 10:34:20PM -0800, Junio C Hamano wrote:
> 
> Actually, I wonder if we can simply get rid of some of the calls in
> send-pack. I think that the code in send-pack isn't even called anymore
> via "git push"; it only gets called when you call send-pack directly.

Actually, its also seemingly called by git-remote-http(s) (at least it
contains references to "stateless RPC", which is related to smart HTTP).

> And arguably send-pack as plumbing shouldn't be generating all sorts of
> user-facing output. But it is a behavior change. I wonder if anybody
> actually calls send-pack directly anymore. It seems like even scripts
> use "git push" because of the transport agnosticism.

For non-stateless case, it seems that the only protocols builtin-send-pack
can deal with are ssh://, git:// and file://, it can't deal with any
sort of remote helper, not even one provoding smart transport.

-Ilari

Re: [PATCH 1/4] Refactoring: remove duplicated code from transport.c and builtin-send-pack.c

[Larry D'Anna]

Weird: I only got the Cc for this, git@vger.kernel.org didnt' sent it to me.  It
doesn't seem to be on gmane either.

* Junio C Hamano (gitster@pobox.com) [100215 01:51]:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Jeff King <peff@peff.net> writes:
> >
> >>>  builtin-send-pack.c |   89 ++++++++++++++----------
> >>>  send-pack.h         |   20 +++++
> >>>  transport.c         |  196 ---------------------------------------------------
> >>
> >> I think this is backwards. The versions in send-pack were there first,
> >> and then were ported to transport.c so that other transports could
> >> benefit from them. And that is where they should ultimately be.
> >
> > Also the names of these functions probably need to be made more specific
> > so that people not so familiar with the transport code can tell that they
> > are from "transport" family.  The names didn't matter much while they were
> > file scope static, but this series changes that.
> 
> Ah, one more thing.  I think this patch touches somewhat overlapping areas
> the ld/push-porcelain topic in 'pu' touches.
> 
> I think Peff's "backwards" observation is correct (and Daniel can
> elaborate if he wants).  Once the direction is set on that point, you and
> Larry probably would need to coordinate to decide how to proceed.  My gut
> feeling without actually looking at the conflicts is that applying your
> code consolidation first and then doing the "porcelain" rework on top
> might be a cleaner approach, but you two are in better position to decide
> on the order, as these are your codes that will be conflicting with each
> other.

That sounds good to me.  I'll rebase the porcelain stuff off the next version of
Michael's series.

          --larry

Re: [PATCH 1/4] Refactoring: remove duplicated code from transport.c and builtin-send-pack.c

[Daniel Barkalow]

On Mon, 15 Feb 2010, Jeff King wrote:

> On Sun, Feb 14, 2010 at 10:34:20PM -0800, Junio C Hamano wrote:
> 
> > > I can't remember the exact details of why the originals were not
> > > removed, though (I think I complained about it once before, and there
> > > was some technical reason, but I don't recall now). Daniel (cc'd) might
> > > remember more.
> > 
> > Also the names of these functions probably need to be made more specific
> > so that people not so familiar with the transport code can tell that they
> > are from "transport" family.  The names didn't matter much while they were
> > file scope static, but this series changes that.
> 
> Actually, I wonder if we can simply get rid of some of the calls in
> send-pack. I think that the code in send-pack isn't even called anymore
> via "git push"; it only gets called when you call send-pack directly.
> And arguably send-pack as plumbing shouldn't be generating all sorts of
> user-facing output. But it is a behavior change. I wonder if anybody
> actually calls send-pack directly anymore. It seems like even scripts
> use "git push" because of the transport agnosticism.

I think it would probably be better to get rid of send-pack as a separate 
command entirely, rather than changing any of its behavior, and make 
remote-curl use a private command that only has the desired behavior, 
which is stdio to a local proxy for the remote.

For that matter, it would likely be worthwhile abstracting the packet_line 
code such that send-pack (and fetch-pack) could be done in-process without 
the messages going over a classic packet_line connection to remote-curl 
before being sent over HTTP to the actual server.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 2/4] Refactoring: connect.c: move duplicated code to get_host_and_port

[Johannes Sixt]

Michael Lukashov schrieb:
> +static void get_host_and_port(char **host, const char **port, int set_port_none)

Minor nit: The last parameter, set_port_none, is a rather prominent sign 
that this function mixes policy and functionality. And indeed, this 
implementation:

> +	if (colon) {
> +		*colon = 0;
> +		*port = colon + 1;
> +		if (set_port_none && !**port)
> +			*port = "<none>";
> +	}

proves it. The _functionality_ is to find host and port from a string. The 
_policy_ is to set the port to "<none>" if it would otherwise be empty. 
The callers take care of the _policy_, this function should only care 
about _functionality_. There's only one call site that wants "<none>"; 
don't move this detail into this function.

Other than that: nice catch.

-- Hannes