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

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

[Michael Lukashov]

Hi,

Here is the 2nd iteration of my refactoring patches. 

Michael Lukashov (4):
  Refactoring: remove duplicated code from builtin-send-pack.c and
    transport.c
  Refactoring: connect.c: move duplicated code to get_host_and_port
  Refactoring: move duplicated code from builtin-pack-objects.c and
    fast-import.c to object.c
  Refactoring: remove duplicated code from builtin-checkout.c and
    merge-recursive.c

 builtin-checkout.c     |   18 -----
 builtin-fetch.c        |   20 +++---
 builtin-pack-objects.c |   27 -------
 builtin-send-pack.c    |  190 +----------------------------------------------
 connect.c              |   83 +++++++--------------
 fast-import.c          |   23 ------
 merge-recursive.c      |    2 +-
 merge-recursive.h      |    3 +
 object.c               |   20 +++++
 object.h               |    9 ++
 transport.c            |   27 +++----
 transport.h            |   11 +++
 12 files changed, 101 insertions(+), 332 deletions(-)

[PATCH v2 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
  print_ref_status
  status_abbrev
  print_ok_ref_status
  print_one_push_status
  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

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 |  190 ++-------------------------------------------------
 transport.c         |   27 ++++----
 transport.h         |   11 +++
 4 files changed, 39 insertions(+), 209 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..b6e8948 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -169,156 +169,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 +339,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 +355,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 +448,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 +477,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..0924288 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,9 +609,8 @@ 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)
+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)
@@ -623,7 +622,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
@@ -687,7 +686,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 		break;
 	case REF_STATUS_UPTODATE:
 		print_ref_status('=', "[up to date]", ref,
-						 ref->peer_ref, NULL, porcelain);
+				 ref->peer_ref, NULL, porcelain);
 		break;
 	case REF_STATUS_REJECT_NONFASTFORWARD:
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
@@ -711,8 +710,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 +737,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 +1017,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 +1056,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 +1066,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

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

[Tay Ray Chuan]

Hi,

On Mon, 15 Feb 2010 23:26:47 +0000
Michael Lukashov <michael.lukashov@gmail.com> wrote:

> The following functions are (almost) identical:
> 
>   verify_remote_names
>   update_tracking_ref
>   print_ref_status
>   status_abbrev
>   print_ok_ref_status
>   print_one_push_status
>   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

this is misleading. This list should the 4 functions added to
transport.h. Some of the functions have been removed entirely from
builtin-send-pack.c and aren't renamed at all (eg. print_ref_status,
print_one_push_status).

For these, you could put them in another list, and say that "they have
been removed entirely and will not be made public, since they are only
used internally by print_push_status()."

> diff --git a/transport.c b/transport.c
> index 3846aac..0924288 100644
> --- a/transport.c
> +++ b/transport.c
> [snip]
> @@ -609,9 +609,8 @@ 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)
> +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)

Unrelated whitespace change in the method signature.

> @@ -687,7 +686,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
>  		break;
>  	case REF_STATUS_UPTODATE:
>  		print_ref_status('=', "[up to date]", ref,
> -						 ref->peer_ref, NULL, porcelain);
> +				 ref->peer_ref, NULL, porcelain);
>  		break;
>  	case REF_STATUS_REJECT_NONFASTFORWARD:
>  		print_ref_status('!', "[rejected]", ref, ref->peer_ref,

Unrelated whitespace change.

> @@ -711,8 +710,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;

Unrelated whitespace change for the second line of the signature.

-- 
Cheers,
Ray Chuan

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

[Jeff King]

On Mon, Feb 15, 2010 at 11:26:47PM +0000, Michael Lukashov wrote:

> diff --git a/builtin-fetch.c b/builtin-fetch.c
> index 8654fa7..d3b9d8a 100644
> --- a/builtin-fetch.c
> +++ b/builtin-fetch.c
> [...]
> @@ -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);

If you are refactoring, can all of these fetch lines just call
print_ref_status, which handles the summary width stuff itself? The push
and fetch formats are meant to be quite similar.

-Peff

[PATCH v2 2/4] Refactoring: connect.c: move duplicated code to 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

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

[Larry D'Anna]

* Michael Lukashov (michael.lukashov@gmail.com) [100215 18:33]:
> 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>";

shouldn't that be 'port = "none";'?

          --larry

[PATCH v2 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

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

[Junio C Hamano]

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

> The following functions are duplicated:
>
>   encode_header

what are the other duplicated ones ;-)?

> Signed-off-by: Michael Lukashov <michael.lukashov@gmail.com>
> ---

Two comments:

 - encode_header() was a perfectly good name for a static function in
   these two contexts, but when lifted into public namespace, it is not
   clear enough anymore.  It is not clear "header" in what context you are
   talking about.  At least it should be encode_in_pack_object_header();

 - Look at what are in object.[ch]; they are all about "object" layer,
   that sits one level higher in the abstraction on top of the raw object
   data layer (e.g. read_sha1_file() and friends).  This function belongs
   to a layer that is even lower level than the raw object data (i.e. one
   particular implementation of the raw object data representations among
   others).

   It looks very out of place.  I would say that cache.h and sha1_file.c
   would probably be a better place, if nobody else finds a better
   alternative.

Other than that, I agree with the patch, including its choice of types
involved.

[PATCH v2 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 v2 4/4] Refactoring: remove duplicated code from builtin-checkout.c and merge-recursive.c

[Junio C Hamano]

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

> 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;

Isn't a much better home for this function next to read_mmfile() in
xdiff-interface.c?

Perhaps it would make sense to morph it into something like this

	int read_mmblob(mmfile_t *ptr, const unsigned char *sha1);

for consistency.