[PATCH v3 0/4] Refactoring: remove duplicated code

[PATCH v3 0/4] Refactoring: remove duplicated code

[Michael Lukashov]

Hi,

Here is 3rd iteration of my refactoring patches. These patches take into
account suggestions made by Tay Ray Chuan, Johannes Sixt, Jeff King and Junio C Hamano

Michael Lukashov (4):
  Refactoring: remove duplicated code from builtin-send-pack.c and
    transport.c
  Refactoring: connect.c: move duplicated code to a new function
    'get_host_and_port'
  Refactoring: move duplicated code from builtin-pack-objects.c and
    fast-import.c to sha1_file.c
  Refactoring: move duplicated code from builtin-checkout.c and
    merge-recursive.c to xdiff-interface.c

 builtin-checkout.c     |   24 +-----
 builtin-fetch.c        |   20 +++---
 builtin-pack-objects.c |   31 +-------
 builtin-send-pack.c    |  191 ++----------------------------------------------
 cache.h                |    8 ++
 connect.c              |   83 +++++++--------------
 fast-import.c          |   29 +-------
 merge-recursive.c      |   23 +-----
 sha1_file.c            |   20 +++++
 transport.c            |   22 +++---
 transport.h            |   11 +++
 xdiff-interface.c      |   17 ++++
 xdiff-interface.h      |    1 +
 13 files changed, 123 insertions(+), 357 deletions(-)

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

[Michael Lukashov]

The following functions are (almost) identical:

  verify_remote_names
  update_tracking_ref
  refs_pushed
  print_push_status

Move common versions of these functions to transport.c and rename them,
as suggested by Jeff King and Junio C Hamano.

The following functions have been removed entirely from builtin-send-pack.c,
since they are only used internally by print_push_status():

  print_ref_status
  status_abbrev
  print_ok_ref_status
  print_one_push_status

Also, move #define SUMMARY_WIDTH to transport.h and rename it TRANSPORT_SUMMARY_WIDTH
as it is used in builtin-fetch.c and transport.c

Signed-off-by: Michael Lukashov <michael.lukashov@gmail.com>
---
 builtin-fetch.c     |   20 +++---
 builtin-send-pack.c |  191 ++-------------------------------------------------
 transport.c         |   22 +++---
 transport.h         |   11 +++
 4 files changed, 37 insertions(+), 207 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index 8654fa7..d3b9d8a 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -11,6 +11,7 @@
 #include "run-command.h"
 #include "parse-options.h"
 #include "sigchain.h"
+#include "transport.h"
 
 static const char * const builtin_fetch_usage[] = {
 	"git fetch [options] [<repository> <refspec>...]",
@@ -205,7 +206,6 @@ static int s_update_ref(const char *action,
 	return 0;
 }
 
-#define SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
 #define REFCOL_WIDTH  10
 
 static int update_local_ref(struct ref *ref,
@@ -224,7 +224,7 @@ static int update_local_ref(struct ref *ref,
 
 	if (!hashcmp(ref->old_sha1, ref->new_sha1)) {
 		if (verbosity > 0)
-			sprintf(display, "= %-*s %-*s -> %s", SUMMARY_WIDTH,
+			sprintf(display, "= %-*s %-*s -> %s", TRANSPORT_SUMMARY_WIDTH,
 				"[up to date]", REFCOL_WIDTH, remote,
 				pretty_ref);
 		return 0;
@@ -239,7 +239,7 @@ static int update_local_ref(struct ref *ref,
 		 * the head, and the old value of the head isn't empty...
 		 */
 		sprintf(display, "! %-*s %-*s -> %s  (can't fetch in current branch)",
-			SUMMARY_WIDTH, "[rejected]", REFCOL_WIDTH, remote,
+			TRANSPORT_SUMMARY_WIDTH, "[rejected]", REFCOL_WIDTH, remote,
 			pretty_ref);
 		return 1;
 	}
@@ -249,7 +249,7 @@ static int update_local_ref(struct ref *ref,
 		int r;
 		r = s_update_ref("updating tag", ref, 0);
 		sprintf(display, "%c %-*s %-*s -> %s%s", r ? '!' : '-',
-			SUMMARY_WIDTH, "[tag update]", REFCOL_WIDTH, remote,
+			TRANSPORT_SUMMARY_WIDTH, "[tag update]", REFCOL_WIDTH, remote,
 			pretty_ref, r ? "  (unable to update local ref)" : "");
 		return r;
 	}
@@ -271,7 +271,7 @@ static int update_local_ref(struct ref *ref,
 
 		r = s_update_ref(msg, ref, 0);
 		sprintf(display, "%c %-*s %-*s -> %s%s", r ? '!' : '*',
-			SUMMARY_WIDTH, what, REFCOL_WIDTH, remote, pretty_ref,
+			TRANSPORT_SUMMARY_WIDTH, what, REFCOL_WIDTH, remote, pretty_ref,
 			r ? "  (unable to update local ref)" : "");
 		return r;
 	}
@@ -284,7 +284,7 @@ static int update_local_ref(struct ref *ref,
 		strcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));
 		r = s_update_ref("fast-forward", ref, 1);
 		sprintf(display, "%c %-*s %-*s -> %s%s", r ? '!' : ' ',
-			SUMMARY_WIDTH, quickref, REFCOL_WIDTH, remote,
+			TRANSPORT_SUMMARY_WIDTH, quickref, REFCOL_WIDTH, remote,
 			pretty_ref, r ? "  (unable to update local ref)" : "");
 		return r;
 	} else if (force || ref->force) {
@@ -295,13 +295,13 @@ static int update_local_ref(struct ref *ref,
 		strcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));
 		r = s_update_ref("forced-update", ref, 1);
 		sprintf(display, "%c %-*s %-*s -> %s  (%s)", r ? '!' : '+',
-			SUMMARY_WIDTH, quickref, REFCOL_WIDTH, remote,
+			TRANSPORT_SUMMARY_WIDTH, quickref, REFCOL_WIDTH, remote,
 			pretty_ref,
 			r ? "unable to update local ref" : "forced update");
 		return r;
 	} else {
 		sprintf(display, "! %-*s %-*s -> %s  (non-fast-forward)",
-			SUMMARY_WIDTH, "[rejected]", REFCOL_WIDTH, remote,
+			TRANSPORT_SUMMARY_WIDTH, "[rejected]", REFCOL_WIDTH, remote,
 			pretty_ref);
 		return 1;
 	}
@@ -393,7 +393,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 			rc |= update_local_ref(ref, what, note);
 		else
 			sprintf(note, "* %-*s %-*s -> FETCH_HEAD",
-				SUMMARY_WIDTH, *kind ? kind : "branch",
+				TRANSPORT_SUMMARY_WIDTH, *kind ? kind : "branch",
 				 REFCOL_WIDTH, *what ? what : "HEAD");
 		if (*note) {
 			if (verbosity >= 0 && !shown_url) {
@@ -514,7 +514,7 @@ static int prune_refs(struct transport *transport, struct ref *ref_map)
 			result |= delete_ref(ref->name, NULL, 0);
 		if (verbosity >= 0) {
 			fprintf(stderr, " x %-*s %-*s -> %s\n",
-				SUMMARY_WIDTH, "[deleted]",
+				TRANSPORT_SUMMARY_WIDTH, "[deleted]",
 				REFCOL_WIDTH, "(none)", prettify_refname(ref->name));
 			warn_dangling_symref(stderr, dangling_msg, ref->name);
 		}
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 76c7206..cbda311 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -7,6 +7,7 @@
 #include "remote.h"
 #include "send-pack.h"
 #include "quote.h"
+#include "transport.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"
@@ -169,156 +170,6 @@ static int receive_status(int in, struct ref *refs)
 	return ret;
 }
 
-static void update_tracking_ref(struct remote *remote, struct ref *ref)
-{
-	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 (args.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)
-{
-	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)
-{
-	if (ref->deletion)
-		print_ref_status('-', "[deleted]", ref, NULL, NULL);
-	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);
-	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);
-	}
-}
-
-static int print_one_push_status(struct ref *ref, const char *dest, int count)
-{
-	if (!count)
-		fprintf(stderr, "To %s\n", dest);
-
-	switch(ref->status) {
-	case REF_STATUS_NONE:
-		print_ref_status('X', "[no match]", ref, NULL, NULL);
-		break;
-	case REF_STATUS_REJECT_NODELETE:
-		print_ref_status('!', "[rejected]", ref, NULL,
-				"remote does not support deleting refs");
-		break;
-	case REF_STATUS_UPTODATE:
-		print_ref_status('=', "[up to date]", ref,
-				ref->peer_ref, NULL);
-		break;
-	case REF_STATUS_REJECT_NONFASTFORWARD:
-		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
-				"non-fast-forward");
-		break;
-	case REF_STATUS_REMOTE_REJECT:
-		print_ref_status('!', "[remote rejected]", ref,
-				ref->deletion ? NULL : ref->peer_ref,
-				ref->remote_status);
-		break;
-	case REF_STATUS_EXPECTING_REPORT:
-		print_ref_status('!', "[remote failure]", ref,
-				ref->deletion ? NULL : ref->peer_ref,
-				"remote failed to report status");
-		break;
-	case REF_STATUS_OK:
-		print_ok_ref_status(ref);
-		break;
-	}
-
-	return 1;
-}
-
-static void print_push_status(const char *dest, struct ref *refs)
-{
-	struct ref *ref;
-	int n = 0;
-
-	if (args.verbose) {
-		for (ref = refs; ref; ref = ref->next)
-			if (ref->status == REF_STATUS_UPTODATE)
-				n += print_one_push_status(ref, dest, n);
-	}
-
-	for (ref = refs; ref; ref = ref->next)
-		if (ref->status == REF_STATUS_OK)
-			n += print_one_push_status(ref, dest, n);
-
-	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);
-	}
-}
-
-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 print_helper_status(struct ref *ref)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -489,37 +340,6 @@ int send_pack(struct send_pack_args *args,
 	return 0;
 }
 
-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]);
-	}
-}
-
 int cmd_send_pack(int argc, const char **argv, const char *prefix)
 {
 	int i, nr_refspecs = 0;
@@ -536,6 +356,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++) {
@@ -628,7 +449,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	get_remote_heads(fd[0], &remote_refs, 0, NULL, REF_NORMAL,
 			 &extra_have);
 
-	verify_remote_names(nr_refspecs, refspecs);
+	transport_verify_remote_names(nr_refspecs, refspecs);
 
 	local_refs = get_local_heads();
 
@@ -657,15 +478,15 @@ 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);
+		transport_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);
+			transport_update_tracking_ref(remote, ref, args.verbose);
 	}
 
-	if (!ret && !refs_pushed(remote_refs))
+	if (!ret && !transport_refs_pushed(remote_refs))
 		fprintf(stderr, "Everything up-to-date\n");
 
 	return ret;
diff --git a/transport.c b/transport.c
index 3846aac..6d7a125 100644
--- a/transport.c
+++ b/transport.c
@@ -573,7 +573,7 @@ static int push_had_errors(struct ref *ref)
 	return 0;
 }
 
-static int refs_pushed(struct ref *ref)
+int transport_refs_pushed(struct ref *ref)
 {
 	for (; ref; ref = ref->next) {
 		switch(ref->status) {
@@ -587,7 +587,7 @@ static int refs_pushed(struct ref *ref)
 	return 0;
 }
 
-static void update_tracking_ref(struct remote *remote, struct ref *ref, int verbose)
+void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int verbose)
 {
 	struct refspec rs;
 
@@ -609,8 +609,6 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref, int verb
 	}
 }
 
-#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) {
@@ -623,7 +621,7 @@ static void print_ref_status(char flag, const char *summary, struct ref *to, str
 		else
 			fprintf(stdout, "%s\n", summary);
 	} else {
-		fprintf(stderr, " %c %-*s ", flag, SUMMARY_WIDTH, summary);
+		fprintf(stderr, " %c %-*s ", flag, TRANSPORT_SUMMARY_WIDTH, summary);
 		if (from)
 			fprintf(stderr, "%s -> %s", prettify_refname(from->name), prettify_refname(to->name));
 		else
@@ -711,8 +709,8 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 	return 1;
 }
 
-static void print_push_status(const char *dest, struct ref *refs,
-			      int verbose, int porcelain, int * nonfastforward)
+void transport_print_push_status(const char *dest, struct ref *refs,
+				  int verbose, int porcelain, int *nonfastforward)
 {
 	struct ref *ref;
 	int n = 0;
@@ -738,7 +736,7 @@ static void print_push_status(const char *dest, struct ref *refs,
 	}
 }
 
-static void verify_remote_names(int nr_heads, const char **heads)
+void transport_verify_remote_names(int nr_heads, const char **heads)
 {
 	int i;
 
@@ -1018,7 +1016,7 @@ int transport_push(struct transport *transport,
 		   int *nonfastforward)
 {
 	*nonfastforward = 0;
-	verify_remote_names(refspec_nr, refspec);
+	transport_verify_remote_names(refspec_nr, refspec);
 
 	if (transport->push) {
 		/* Maybe FIXME. But no important transport uses this case. */
@@ -1057,7 +1055,7 @@ int transport_push(struct transport *transport,
 		ret |= err;
 
 		if (!quiet || err)
-			print_push_status(transport->url, remote_refs,
+			transport_print_push_status(transport->url, remote_refs,
 					verbose | porcelain, porcelain,
 					nonfastforward);
 
@@ -1067,10 +1065,10 @@ int transport_push(struct transport *transport,
 		if (!(flags & TRANSPORT_PUSH_DRY_RUN)) {
 			struct ref *ref;
 			for (ref = remote_refs; ref; ref = ref->next)
-				update_tracking_ref(transport->remote, ref, verbose);
+				transport_update_tracking_ref(transport->remote, ref, verbose);
 		}
 
-		if (!quiet && !ret && !refs_pushed(remote_refs))
+		if (!quiet && !ret && !transport_refs_pushed(remote_refs))
 			fprintf(stderr, "Everything up-to-date\n");
 		return ret;
 	}
diff --git a/transport.h b/transport.h
index 7cea5cc..7a9bb57 100644
--- a/transport.h
+++ b/transport.h
@@ -92,6 +92,7 @@ struct transport {
 #define TRANSPORT_PUSH_PORCELAIN 32
 #define TRANSPORT_PUSH_QUIET 64
 #define TRANSPORT_PUSH_SET_UPSTREAM 128
+#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
 
 /* Returns a transport suitable for the url */
 struct transport *transport_get(struct remote *, const char *);
@@ -142,4 +143,14 @@ int transport_connect(struct transport *transport, const char *name,
 /* Transport methods defined outside transport.c */
 int transport_helper_init(struct transport *transport, const char *name);
 
+/* common methods used by transport.c and builtin-send-pack.c */
+void transport_verify_remote_names(int nr_heads, const char **heads);
+
+void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int verbose);
+
+int transport_refs_pushed(struct ref *ref);
+
+void transport_print_push_status(const char *dest, struct ref *refs,
+		  int verbose, int porcelain, int *nonfastforward);
+
 #endif
-- 
1.7.0.1571.g856c2

[PATCH v3 2/4] Refactoring: connect.c: move duplicated code to a new function 'get_host_and_port'

[Michael Lukashov]

The following functions:

  git_tcp_connect_sock (IPV6 version)
  git_tcp_connect_sock (no IPV6 version),
  git_proxy_connect

have common block of code. Move it to a new function '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..cd399f4 100644
--- a/connect.c
+++ b/connect.c
@@ -152,6 +152,28 @@ 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)
+{
+	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;
+	}
+}
+
 #ifndef NO_IPV6
 
 static const char *ai_name(const struct addrinfo *ai)
@@ -170,30 +192,14 @@ 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);
+	if (!*port)
+		*port = "<none>";
 
 	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);
 
 	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);
 
 	argv[0] = git_proxy_command;
 	argv[1] = host;
-- 
1.7.0.1571.g856c2

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

[Michael Lukashov]

The following function is duplicated:

  encode_header

Move this function to sha1_file.c and rename it 'encode_in_pack_object_header',
as suggested by Junio C Hamano

Signed-off-by: Michael Lukashov <michael.lukashov@gmail.com>
---
 builtin-pack-objects.c |   31 ++-----------------------------
 cache.h                |    8 ++++++++
 fast-import.c          |   29 +++--------------------------
 sha1_file.c            |   20 ++++++++++++++++++++
 4 files changed, 33 insertions(+), 55 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index e1d3adf..6b2f65c 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.
  */
@@ -321,7 +294,7 @@ static unsigned long write_object(struct sha1file *f,
 		 * The object header is a byte of 'type' followed by zero or
 		 * more bytes of length.
 		 */
-		hdrlen = encode_header(type, size, header);
+		hdrlen = encode_in_pack_object_header(type, size, header);
 
 		if (type == OBJ_OFS_DELTA) {
 			/*
@@ -372,7 +345,7 @@ static unsigned long write_object(struct sha1file *f,
 		if (entry->delta)
 			type = (allow_ofs_delta && entry->delta->idx.offset) ?
 				OBJ_OFS_DELTA : OBJ_REF_DELTA;
-		hdrlen = encode_header(type, entry->size, header);
+		hdrlen = encode_in_pack_object_header(type, entry->size, header);
 
 		offset = entry->in_pack_offset;
 		revidx = find_pack_revindex(p, offset);
diff --git a/cache.h b/cache.h
index d478eff..2654b60 100644
--- a/cache.h
+++ b/cache.h
@@ -911,6 +911,14 @@ extern void *unpack_entry(struct packed_git *, off_t, enum object_type *, unsign
 extern unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);
 extern unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);
 extern const char *packed_object_info_detail(struct packed_git *, off_t, unsigned long *, unsigned long *, unsigned int *, unsigned char *);
+/*
+ * 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_in_pack_object_header(enum object_type type, uintmax_t size, unsigned char *hdr);
 
 /* Dumb servers support */
 extern int update_server_info(int);
diff --git a/fast-import.c b/fast-import.c
index b477dc6..776782a 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,
@@ -1138,7 +1115,7 @@ static int store_object(
 		delta_count_by_type[type]++;
 		e->depth = last->depth + 1;
 
-		hdrlen = encode_header(OBJ_OFS_DELTA, deltalen, hdr);
+		hdrlen = encode_in_pack_object_header(OBJ_OFS_DELTA, deltalen, hdr);
 		write_or_die(pack_data->pack_fd, hdr, hdrlen);
 		pack_size += hdrlen;
 
@@ -1149,7 +1126,7 @@ static int store_object(
 		pack_size += sizeof(hdr) - pos;
 	} else {
 		e->depth = 0;
-		hdrlen = encode_header(type, dat->len, hdr);
+		hdrlen = encode_in_pack_object_header(type, dat->len, hdr);
 		write_or_die(pack_data->pack_fd, hdr, hdrlen);
 		pack_size += hdrlen;
 	}
@@ -1209,7 +1186,7 @@ static void stream_blob(uintmax_t len, unsigned char *sha1out, uintmax_t mark)
 	memset(&s, 0, sizeof(s));
 	deflateInit(&s, pack_compression_level);
 
-	hdrlen = encode_header(OBJ_BLOB, len, out_buf);
+	hdrlen = encode_in_pack_object_header(OBJ_BLOB, len, out_buf);
 	if (out_sz <= hdrlen)
 		die("impossibly large object header");
 
diff --git a/sha1_file.c b/sha1_file.c
index 657825e..f3c9823 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1475,6 +1475,26 @@ const char *packed_object_info_detail(struct packed_git *p,
 	}
 }
 
+int encode_in_pack_object_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 packed_object_info(struct packed_git *p, off_t obj_offset,
 			      unsigned long *sizep)
 {
-- 
1.7.0.1571.g856c2

[PATCH v3 4/4] Refactoring: move duplicated code from builtin-checkout.c and merge-recursive.c to xdiff-interface.c

[Michael Lukashov]

The following function is duplicated:

  fill_mm

Move it to xdiff-interface.c and rename it 'read_mmblob',
as suggested by Junio C Hamano.

Also, change parameters order for consistency.

Signed-off-by: Michael Lukashov <michael.lukashov@gmail.com>
---
 builtin-checkout.c |   24 +++---------------------
 merge-recursive.c  |   23 +++--------------------
 xdiff-interface.c  |   17 +++++++++++++++++
 xdiff-interface.h  |    1 +
 4 files changed, 24 insertions(+), 41 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 5277817..22ae92f 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];
@@ -163,9 +145,9 @@ static int checkout_merged(int pos, struct checkout *state)
 	    ce_stage(active_cache[pos+2]) != 3)
 		return error("path '%s' does not have all 3 versions", path);
 
-	fill_mm(active_cache[pos]->sha1, &ancestor);
-	fill_mm(active_cache[pos+1]->sha1, &ours);
-	fill_mm(active_cache[pos+2]->sha1, &theirs);
+	read_mmblob(&ancestor, active_cache[pos]->sha1);
+	read_mmblob(&ours, active_cache[pos+1]->sha1);
+	read_mmblob(&theirs, active_cache[pos+2]->sha1);
 
 	status = ll_merge(&result_buf, path, &ancestor,
 			  &ours, "ours", &theirs, "theirs", 0);
diff --git a/merge-recursive.c b/merge-recursive.c
index cb53b01..195ebf9 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -599,23 +599,6 @@ struct merge_file_info
 		 merge:1;
 };
 
-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 merge_3way(struct merge_options *o,
 		      mmbuffer_t *result_buf,
 		      struct diff_filespec *one,
@@ -653,9 +636,9 @@ static int merge_3way(struct merge_options *o,
 		name2 = xstrdup(mkpath("%s", branch2));
 	}
 
-	fill_mm(one->sha1, &orig);
-	fill_mm(a->sha1, &src1);
-	fill_mm(b->sha1, &src2);
+	read_mmblob(&orig, one->sha1);
+	read_mmblob(&src1, a->sha1);
+	read_mmblob(&src2, b->sha1);
 
 	merge_status = ll_merge(result_buf, a->path, &orig,
 				&src1, name1, &src2, name2,
diff --git a/xdiff-interface.c b/xdiff-interface.c
index 01f14fb..ca5e3fb 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -218,6 +218,23 @@ int read_mmfile(mmfile_t *ptr, const char *filename)
 	return 0;
 }
 
+void read_mmblob(mmfile_t *ptr, const unsigned char *sha1)
+{
+	unsigned long size;
+	enum object_type type;
+
+	if (!hashcmp(sha1, null_sha1)) {
+		ptr->ptr = xstrdup("");
+		ptr->size = 0;
+		return;
+	}
+
+	ptr->ptr = read_sha1_file(sha1, &type, &size);
+	if (!ptr->ptr || type != OBJ_BLOB)
+		die("unable to read blob object %s", sha1_to_hex(sha1));
+	ptr->size = size;
+}
+
 #define FIRST_FEW_BYTES 8000
 int buffer_is_binary(const char *ptr, unsigned long size)
 {
diff --git a/xdiff-interface.h b/xdiff-interface.h
index 55572c3..abba70c 100644
--- a/xdiff-interface.h
+++ b/xdiff-interface.h
@@ -18,6 +18,7 @@ int parse_hunk_header(char *line, int len,
 		      int *ob, int *on,
 		      int *nb, int *nn);
 int read_mmfile(mmfile_t *ptr, const char *filename);
+void read_mmblob(mmfile_t *ptr, const unsigned char *sha1);
 int buffer_is_binary(const char *ptr, unsigned long size);
 
 extern void xdiff_set_find_func(xdemitconf_t *xecfg, const char *line, int cflags);
-- 
1.7.0.1571.g856c2

Re: [PATCH v3 2/4] Refactoring: connect.c: move duplicated code to a new function 'get_host_and_port'

[Larry D'Anna]

* Michael Lukashov (michael.lukashov@gmail.com) [100216 18:43]:

> @@ -170,30 +192,14 @@ 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);
> +	if (!*port)
> +		*port = "<none>";

Again, isn't this wrong?  This doesn't even compile for me.  Shouldn't it be

port = "<none>";

??

What am I missing?

     --larry

Re: [PATCH v3 2/4] Refactoring: connect.c: move duplicated code to a new function 'get_host_and_port'

[Junio C Hamano]

Michael Lukashov <michael.lukashov@gmail.com> writes:

> The following functions:
>
>   git_tcp_connect_sock (IPV6 version)
>   git_tcp_connect_sock (no IPV6 version),
>   git_proxy_connect
>
> have common block of code. Move it to a new function 'get_host_and_port'
>
> Signed-off-by: Michael Lukashov <michael.lukashov@gmail.com>

> @@ -170,30 +192,14 @@ static const char *ai_name(const struct addrinfo *ai)
>  static int git_tcp_connect_sock(char *host, int flags)
> ...
> -	if (colon) {
> -		*colon = 0;
> -		port = colon + 1;
> -		if (!*port)
> -			port = "<none>";
> -	}
> +	get_host_and_port(&host, &port);
> +	if (!*port)
> +		*port = "<none>";

Is this version any different from v2?

I expected that at least this one would have been fixed from the older
version.

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

[Tay Ray Chuan]

Hi,

On Wed, Feb 17, 2010 at 7:42 AM, Michael Lukashov
<michael.lukashov@gmail.com> wrote:
> [snip]
> Also, move #define SUMMARY_WIDTH to transport.h and rename it TRANSPORT_SUMMARY_WIDTH
> as it is used in builtin-fetch.c and transport.c
>
> Signed-off-by: Michael Lukashov <michael.lukashov@gmail.com>

Apart from the long line in the patch message, looks ok to me.

Acked-by: Tay Ray Chuan <rctay89@gmail.com>

-- 
Cheers,
Ray Chuan

Re: [PATCH v3 2/4] Refactoring: connect.c: move duplicated code to a new function 'get_host_and_port'

[Michael Lukashov]

On Wed, Feb 17, 2010 at 3:04 AM, Larry D'Anna <larry@elder-gods.org> wrote:
> * Michael Lukashov (michael.lukashov@gmail.com) [100216 18:43]:
>
>> @@ -170,30 +192,14 @@ 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);
>> +     if (!*port)
>> +             *port = "<none>";
>
> Again, isn't this wrong?  This doesn't even compile for me.  Shouldn't it be
>
> port = "<none>";
>
> ??
>
> What am I missing?
>
>     --larry
>

Oops, my bad, sorry.
Strange, my mingw-gcc 4.4.0 compiler didn't issue no errors or
warnings. Weird indeed.
I'll send repaired patch later.

[PATCH v4 2/4] Refactoring: connect.c: move duplicated code to a new function 'get_host_and_port'

[Michael Lukashov]

The following functions:

  git_tcp_connect_sock (IPV6 version)
  git_tcp_connect_sock (no IPV6 version),
  git_proxy_connect

have common block of code. Move it to a new function '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..f4563f9 100644
--- a/connect.c
+++ b/connect.c
@@ -152,6 +152,28 @@ 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)
+{
+	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;
+	}
+}
+
 #ifndef NO_IPV6
 
 static const char *ai_name(const struct addrinfo *ai)
@@ -170,30 +192,14 @@ 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);
+	if (!*port)
+		port = "<none>";
 
 	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);
 
 	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);
 
 	argv[0] = git_proxy_command;
 	argv[1] = host;
-- 
1.7.0.1571.g856c2