[PATCH 00/13] New output style for git remote show

[PATCH 00/13] New output style for git remote show

[Jay Soffian]

Junio, this is based on pu, as it builds on my and Jeff's git remote
changes. Patches 1-7 are about new output style for "git remote show"
for everything but the push refspecs. Patches 8-13 then culiminate in
teaching it a new output style for push refspecs as well.

1-4 were previously a single patch which I broke up per your feedback
(gmane 110806).

Jay Soffian (13):
  remote: rename variable and eliminate redundant function call
  remote: remove unused code in get_ref_states
  remote: fix two inconsistencies in the output of "show <remote>"
  remote: make get_remote_ref_states() always populate states.tracked
  remote: name remote_refs consistently
  string-list: new for_each_string_list() function
  remote: new show output style
  refactor duplicated get_local_heads() to remote.c
  refactor duplicated ref_newer() to remote.c
  remote.c: make match_refs() copy src ref before assigning to peer_ref
  remote.c: don't short-circuit match_refs() when error in
    match_explicit_refs()
  remote.c: refactor get_remote_ref_states()
  remote: new show output style for push refspecs

 builtin-remote.c    |  469 ++++++++++++++++++++++++++++++++++++++++-----------
 builtin-send-pack.c |   78 +---------
 http-push.c         |   72 +--------
 remote.c            |   88 +++++++++-
 remote.h            |    2 +
 string-list.c       |   10 +
 string-list.h       |    5 +
 t/t5505-remote.sh   |   66 +++++---
 8 files changed, 516 insertions(+), 274 deletions(-)

[PATCH 01/13] remote: rename variable and eliminate redundant function call

[Jay Soffian]

The variable name "remote" is used as both a "char *" and as a "struct
remote *"; this is confusing, so rename the former to remote_name.

There is no need to call "sort_string_list(&branch_list)" as branch_list
is populated via string_list_insert(), which maintains its order.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin-remote.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index d6958d4..1603219 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -144,7 +144,7 @@ static int add(int argc, const char **argv)
 }
 
 struct branch_info {
-	char *remote;
+	char *remote_name;
 	struct string_list merge;
 };
 
@@ -183,9 +183,9 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 			item->util = xcalloc(sizeof(struct branch_info), 1);
 		info = item->util;
 		if (type == REMOTE) {
-			if (info->remote)
+			if (info->remote_name)
 				warning("more than one branch.%s", key);
-			info->remote = xstrdup(value);
+			info->remote_name = xstrdup(value);
 		} else {
 			char *space = strchr(value, ' ');
 			value = abbrev_branch(value);
@@ -207,7 +207,6 @@ static void read_branches(void)
 	if (branch_list.nr)
 		return;
 	git_config(config_read_branches, NULL);
-	sort_string_list(&branch_list);
 }
 
 struct ref_states {
@@ -490,7 +489,7 @@ static int mv(int argc, const char **argv)
 	for (i = 0; i < branch_list.nr; i++) {
 		struct string_list_item *item = branch_list.items + i;
 		struct branch_info *info = item->util;
-		if (info->remote && !strcmp(info->remote, rename.old)) {
+		if (info->remote_name && !strcmp(info->remote_name, rename.old)) {
 			strbuf_reset(&buf);
 			strbuf_addf(&buf, "branch.%s.remote", item->string);
 			if (git_config_set(buf.buf, rename.new)) {
@@ -600,7 +599,7 @@ static int rm(int argc, const char **argv)
 	for (i = 0; i < branch_list.nr; i++) {
 		struct string_list_item *item = branch_list.items + i;
 		struct branch_info *info = item->util;
-		if (info->remote && !strcmp(info->remote, remote->name)) {
+		if (info->remote_name && !strcmp(info->remote_name, remote->name)) {
 			const char *keys[] = { "remote", "merge", NULL }, **k;
 			for (k = keys; *k; k++) {
 				strbuf_reset(&buf);
@@ -743,7 +742,7 @@ static int show(int argc, const char **argv)
 			struct branch_info *info = branch->util;
 			int j;
 
-			if (!info->merge.nr || strcmp(*argv, info->remote))
+			if (!info->merge.nr || strcmp(*argv, info->remote_name))
 				continue;
 			printf("  Remote branch%s merged with 'git pull' "
 				"while on branch %s\n   ",
-- 
1.6.2.rc1.223.gfed32

[PATCH 02/13] remote: remove unused code in get_ref_states

[Jay Soffian]

get_ref_states() populates the util pointer of the string_list_item's
that it adds to states->new and states->tracked, but nothing ever uses
the pointer, so we can get rid of the extra code.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin-remote.c |   13 +++----------
 1 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index 1603219..f2ef08a 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -250,18 +250,11 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states)
 
 	states->new.strdup_strings = states->tracked.strdup_strings = 1;
 	for (ref = fetch_map; ref; ref = ref->next) {
-		struct string_list *target = &states->tracked;
 		unsigned char sha1[20];
-		void *util = NULL;
-
 		if (!ref->peer_ref || read_ref(ref->peer_ref->name, sha1))
-			target = &states->new;
-		else {
-			target = &states->tracked;
-			if (hashcmp(sha1, ref->new_sha1))
-				util = &states;
-		}
-		string_list_append(abbrev_branch(ref->name), target)->util = util;
+			string_list_append(abbrev_branch(ref->name), &states->new);
+		else
+			string_list_append(abbrev_branch(ref->name), &states->tracked);
 	}
 	free_refs(fetch_map);
 
-- 
1.6.2.rc1.223.gfed32

[PATCH 03/13] remote: fix two inconsistencies in the output of "show <remote>"

[Jay Soffian]

Remote and stale branches are emitted in alphabetical order, but new and
tracked branches are not. So sort the latter to be consistent with the
former. This also lets us use more efficient string_list_has_string()
instead of unsorted_string_list_has_string().

"show <remote>" prunes symrefs, but "show <remote> -n" does not. Fix the
latter to match the former.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin-remote.c  |   15 ++++++++++-----
 t/t5505-remote.sh |    2 +-
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index f2ef08a..31fbd1e 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -227,10 +227,8 @@ static int handle_one_branch(const char *refname,
 		const char *name = abbrev_branch(refspec.src);
 		/* symbolic refs pointing nowhere were handled already */
 		if ((flags & REF_ISSYMREF) ||
-				unsorted_string_list_has_string(&states->tracked,
-					name) ||
-				unsorted_string_list_has_string(&states->new,
-					name))
+		    string_list_has_string(&states->tracked, name) ||
+		    string_list_has_string(&states->new, name))
 			return 0;
 		item = string_list_append(name, &states->stale);
 		item->util = xstrdup(refname);
@@ -258,6 +256,8 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states)
 	}
 	free_refs(fetch_map);
 
+	sort_string_list(&states->new);
+	sort_string_list(&states->tracked);
 	for_each_ref(handle_one_branch, states);
 	sort_string_list(&states->stale);
 
@@ -688,6 +688,9 @@ static int append_ref_to_tracked_list(const char *refname,
 	struct ref_states *states = cb_data;
 	struct refspec refspec;
 
+	if (flags & REF_ISSYMREF)
+		return 0;
+
 	memset(&refspec, 0, sizeof(refspec));
 	refspec.dst = (char *)refname;
 	if (!remote_find_tracking(states->remote, &refspec))
@@ -754,8 +757,10 @@ static int show(int argc, const char **argv)
 				"prune')", &states.stale, "");
 		}
 
-		if (no_query)
+		if (no_query) {
 			for_each_ref(append_ref_to_tracked_list, &states);
+			sort_string_list(&states.tracked);
+		}
 		show_list("  Tracked remote branch%s", &states.tracked, "");
 
 		if (states.remote->push_refspec_nr) {
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 104433d..fdc4a29 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -142,8 +142,8 @@ cat > test/expect << EOF
   New remote branch (next fetch will store in remotes/origin)
     master
   Tracked remote branches
-    side
     master
+    side
   Local branches pushed with 'git push'
     master:upstream
     +refs/tags/lastbackup
-- 
1.6.2.rc1.223.gfed32

[PATCH 04/13] remote: make get_remote_ref_states() always populate states.tracked

[Jay Soffian]

When not querying the remote, show() was having to populate
states.tracked itself. It makes more sense for get_remote_ref_states()
to do this consistently. Since show() is the only caller of
get_remote_ref_states() with query=0, this change does not affect the
other call sites.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin-remote.c |   41 ++++++++++++++++++++---------------------
 1 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index 31fbd1e..b61f754 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -656,6 +656,23 @@ static void free_remote_ref_states(struct ref_states *states)
 	string_list_clear(&states->heads, 0);
 }
 
+static int append_ref_to_tracked_list(const char *refname,
+	const unsigned char *sha1, int flags, void *cb_data)
+{
+	struct ref_states *states = cb_data;
+	struct refspec refspec;
+
+	if (flags & REF_ISSYMREF)
+		return 0;
+
+	memset(&refspec, 0, sizeof(refspec));
+	refspec.dst = (char *)refname;
+	if (!remote_find_tracking(states->remote, &refspec))
+		string_list_append(abbrev_branch(refspec.src), &states->tracked);
+
+	return 0;
+}
+
 static int get_remote_ref_states(const char *name,
 				 struct ref_states *states,
 				 int query)
@@ -677,28 +694,14 @@ static int get_remote_ref_states(const char *name,
 
 		get_head_names(ref, name, states);
 		get_ref_states(ref, states);
+	} else {
+		for_each_ref(append_ref_to_tracked_list, states);
+		sort_string_list(&states->tracked);
 	}
 
 	return 0;
 }
 
-static int append_ref_to_tracked_list(const char *refname,
-	const unsigned char *sha1, int flags, void *cb_data)
-{
-	struct ref_states *states = cb_data;
-	struct refspec refspec;
-
-	if (flags & REF_ISSYMREF)
-		return 0;
-
-	memset(&refspec, 0, sizeof(refspec));
-	refspec.dst = (char *)refname;
-	if (!remote_find_tracking(states->remote, &refspec))
-		string_list_append(abbrev_branch(refspec.src), &states->tracked);
-
-	return 0;
-}
-
 static int show(int argc, const char **argv)
 {
 	int no_query = 0, result = 0;
@@ -757,10 +760,6 @@ static int show(int argc, const char **argv)
 				"prune')", &states.stale, "");
 		}
 
-		if (no_query) {
-			for_each_ref(append_ref_to_tracked_list, &states);
-			sort_string_list(&states.tracked);
-		}
 		show_list("  Tracked remote branch%s", &states.tracked, "");
 
 		if (states.remote->push_refspec_nr) {
-- 
1.6.2.rc1.223.gfed32

[PATCH 05/13] remote: name remote_refs consistently

[Jay Soffian]

Clarify the code a bit by consistently referring to the remote refs
returned by transport_get_remote_refs() as remote_refs.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin-remote.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index b61f754..df6b525 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -236,13 +236,14 @@ static int handle_one_branch(const char *refname,
 	return 0;
 }
 
-static int get_ref_states(const struct ref *ref, struct ref_states *states)
+static int get_ref_states(const struct ref *remote_refs, struct ref_states *states)
 {
 	struct ref *fetch_map = NULL, **tail = &fetch_map;
+	struct ref *ref;
 	int i;
 
 	for (i = 0; i < states->remote->fetch_refspec_nr; i++)
-		if (get_fetch_map(ref, states->remote->fetch + i, &tail, 1))
+		if (get_fetch_map(remote_refs, states->remote->fetch + i, &tail, 1))
 			die("Could not get fetch map for refspec %s",
 				states->remote->fetch_refspec[i]);
 
@@ -264,7 +265,7 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states)
 	return 0;
 }
 
-static int get_head_names(const struct ref *refs,
+static int get_head_names(const struct ref *remote_refs,
 	const char *remote_name, struct ref_states *states)
 {
 	struct ref *ref, *matches;
@@ -275,9 +276,9 @@ static int get_head_names(const struct ref *refs,
 	refspec.pattern = 1;
 	refspec.src = refspec.dst = "refs/heads/";
 	states->heads.strdup_strings = 1;
-	get_fetch_map(refs, &refspec, &fetch_map_tail, 0);
-	matches = guess_remote_head(find_ref_by_name(refs, "HEAD"),
-					fetch_map, 1);
+	get_fetch_map(remote_refs, &refspec, &fetch_map_tail, 0);
+	matches = guess_remote_head(find_ref_by_name(remote_refs, "HEAD"),
+				    fetch_map, 1);
 	for(ref = matches; ref; ref = ref->next)
 		string_list_append(abbrev_branch(ref->name), &states->heads);
 
@@ -678,7 +679,7 @@ static int get_remote_ref_states(const char *name,
 				 int query)
 {
 	struct transport *transport;
-	const struct ref *ref;
+	const struct ref *remote_refs;
 
 	states->remote = remote_get(name);
 	if (!states->remote)
@@ -689,11 +690,10 @@ static int get_remote_ref_states(const char *name,
 	if (query) {
 		transport = transport_get(NULL, states->remote->url_nr > 0 ?
 			states->remote->url[0] : NULL);
-		ref = transport_get_remote_refs(transport);
+		remote_refs = transport_get_remote_refs(transport);
 		transport_disconnect(transport);
-
-		get_head_names(ref, name, states);
-		get_ref_states(ref, states);
+		get_head_names(remote_refs, name, states);
+		get_ref_states(remote_refs, states);
 	} else {
 		for_each_ref(append_ref_to_tracked_list, states);
 		sort_string_list(&states->tracked);
-- 
1.6.2.rc1.223.gfed32

[PATCH 06/13] string-list: new for_each_string_list() function

[Jay Soffian]

Add a convenience function for iterating over a string_list's items via
a callback.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 string-list.c |   10 ++++++++++
 string-list.h |    5 +++++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/string-list.c b/string-list.c
index 15e14cf..1ac536e 100644
--- a/string-list.c
+++ b/string-list.c
@@ -92,6 +92,16 @@ struct string_list_item *string_list_lookup(const char *string, struct string_li
 	return list->items + i;
 }
 
+int for_each_string_list(string_list_each_func_t fn,
+			 struct string_list *list, void *cb_data)
+{
+	int i, ret = 0;
+	for (i = 0; i < list->nr; i++)
+		if ((ret = fn(&list->items[i], cb_data)))
+			break;
+	return ret;
+}
+
 void string_list_clear(struct string_list *list, int free_util)
 {
 	if (list->items) {
diff --git a/string-list.h b/string-list.h
index d32ba05..14bbc47 100644
--- a/string-list.h
+++ b/string-list.h
@@ -20,6 +20,11 @@ void string_list_clear(struct string_list *list, int free_util);
 typedef void (*string_list_clear_func_t)(void *p, const char *str);
 void string_list_clear_func(struct string_list *list, string_list_clear_func_t clearfunc);
 
+/* Use this function to iterate over each item */
+typedef int (*string_list_each_func_t)(struct string_list_item *, void *);
+int for_each_string_list(string_list_each_func_t,
+			 struct string_list *list, void *cb_data);
+
 /* Use these functions only on sorted lists: */
 int string_list_has_string(const struct string_list *list, const char *string);
 int string_list_find_insert_index(const struct string_list *list, const char *string,
-- 
1.6.2.rc1.223.gfed32

[PATCH 07/13] remote: new show output style

[Jay Soffian]

The existing output of "git remote show <remote>" is too verbose for the
information it provides. This patch teaches it to provide more
information in less space.

The output for push refspecs is addressed in followup patch.

Before the patch:

$ git remote show origin
* remote origin
  URL: git://git.kernel.org/pub/scm/git/git.git
  HEAD branch: master
  Remote branch merged with 'git pull' while on branch master
    master
  Remote branch merged with 'git pull' while on branch next
    next
  Remote branches merged with 'git pull' while on branch octopus
    foo bar baz frotz
  New remote branch (next fetch will store in remotes/origin)
    html
  Stale tracking branch (use 'git remote prune')
    bogus
  Tracked remote branches
    maint
    man
    master
    next
    pu
    todo

After this patch:

$ git remote show origin
* remote origin
  URL: git://git.kernel.org/pub/scm/git/git.git
  HEAD branch: master
  Remote branches:
    bogus  stale (use 'git remote prune' to remove)
    html   new (next fetch will store in remotes/origin)
    maint  tracked
    man    tracked
    master tracked
    next   tracked
    pu     tracked
    todo   tracked
  Local branches configured for 'git pull':
    master  rebases onto remote master
    next    rebases onto remote next
    octopus  merges with remote foo
                and with remote bar
                and with remote baz
                and with remote frotz

$ git remote show origin -n
* remote origin
  URL: git://git.kernel.org/pub/scm/git/git.git
  HEAD branch: (not queried)
  Remote branches: (status not queried)
    bogus
    maint
    man
    master
    next
    pu
    todo
  Local branches configured for 'git pull':
    master  rebases onto remote master
    next    rebases onto remote next
    octopus  merges with remote foo
                and with remote bar
                and with remote baz
                and with remote frotz

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
I incorporated the feedback I received to gmane 110626. As the commit
message mentions, the push refspec output is addressed in a later patch
in this series.

 builtin-remote.c  |  187 ++++++++++++++++++++++++++++++++++++++++-------------
 t/t5505-remote.sh |   31 ++++++----
 2 files changed, 161 insertions(+), 57 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index df6b525..4975a0e 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -146,6 +146,7 @@ static int add(int argc, const char **argv)
 struct branch_info {
 	char *remote_name;
 	struct string_list merge;
+	int rebase;
 };
 
 static struct string_list branch_list;
@@ -162,10 +163,11 @@ static const char *abbrev_ref(const char *name, const char *prefix)
 static int config_read_branches(const char *key, const char *value, void *cb)
 {
 	if (!prefixcmp(key, "branch.")) {
+		const char *orig_key = key;
 		char *name;
 		struct string_list_item *item;
 		struct branch_info *info;
-		enum { REMOTE, MERGE } type;
+		enum { REMOTE, MERGE, REBASE } type;
 
 		key += 7;
 		if (!postfixcmp(key, ".remote")) {
@@ -174,6 +176,9 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 		} else if (!postfixcmp(key, ".merge")) {
 			name = xstrndup(key, strlen(key) - 6);
 			type = MERGE;
+		} else if (!postfixcmp(key, ".rebase")) {
+			name = xstrndup(key, strlen(key) - 7);
+			type = REBASE;
 		} else
 			return 0;
 
@@ -184,9 +189,9 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 		info = item->util;
 		if (type == REMOTE) {
 			if (info->remote_name)
-				warning("more than one branch.%s", key);
+				warning("more than one %s", orig_key);
 			info->remote_name = xstrdup(value);
-		} else {
+		} else if (type == MERGE) {
 			char *space = strchr(value, ' ');
 			value = abbrev_branch(value);
 			while (space) {
@@ -197,7 +202,8 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 				space = strchr(value, ' ');
 			}
 			string_list_append(xstrdup(value), &info->merge);
-		}
+		} else
+			info->rebase = git_config_bool(orig_key, value);
 	}
 	return 0;
 }
@@ -212,6 +218,7 @@ static void read_branches(void)
 struct ref_states {
 	struct remote *remote;
 	struct string_list new, stale, tracked, heads;
+	int queried;
 };
 
 static int handle_one_branch(const char *refname,
@@ -635,20 +642,6 @@ static int rm(int argc, const char **argv)
 	return result;
 }
 
-static void show_list(const char *title, struct string_list *list,
-		      const char *extra_arg)
-{
-	int i;
-
-	if (!list->nr)
-		return;
-
-	printf(title, list->nr > 1 ? "es" : "", extra_arg);
-	printf("\n");
-	for (i = 0; i < list->nr; i++)
-		printf("    %s\n", list->items[i].string);
-}
-
 static void free_remote_ref_states(struct ref_states *states)
 {
 	string_list_clear(&states->new, 0);
@@ -698,6 +691,105 @@ static int get_remote_ref_states(const char *name,
 		for_each_ref(append_ref_to_tracked_list, states);
 		sort_string_list(&states->tracked);
 	}
+	states->queried = query;
+
+	return 0;
+}
+
+struct show_info {
+	struct string_list *list;
+	struct ref_states *states;
+	int width;
+	int any_rebase;
+};
+
+int add_remote_to_show_info(struct string_list_item *item, void *cb_data)
+{
+	struct show_info *info = cb_data;
+	int n = strlen(item->string);
+	if (n > info->width)
+		info->width = n;
+	string_list_insert(item->string, info->list);
+	return 0;
+}
+
+int show_remote_info_item(struct string_list_item *item, void *cb_data)
+{
+	struct show_info *info = cb_data;
+	struct ref_states *states = info->states;
+	const char *name = item->string;
+
+	if (states->queried) {
+		const char *fmt = "%s";
+		const char *arg = "";
+		if (string_list_has_string(&states->new, name)) {
+			fmt = " new (next fetch will store in remotes/%s)";
+			arg = states->remote->name;
+		} else if (string_list_has_string(&states->tracked, name))
+			arg = " tracked";
+		else if (string_list_has_string(&states->stale, name))
+			arg = " stale (use 'git remote prune' to remove)";
+		else
+			arg = " ???";
+		printf("    %-*s", info->width, name);
+		printf(fmt, arg);
+		printf("\n");
+	} else
+		printf("    %s\n", name);
+
+	return 0;
+}
+
+int add_local_to_show_info(struct string_list_item *branch_item, void *cb_data)
+{
+	struct show_info *show_info = cb_data;
+	struct ref_states *states = show_info->states;
+	struct branch_info *branch_info = branch_item->util;
+	struct string_list_item *item;
+	int n;
+
+	if (!branch_info->merge.nr || !branch_info->remote_name ||
+	    strcmp(states->remote->name, branch_info->remote_name))
+		return 0;
+	if ((n = strlen(branch_item->string)) > show_info->width)
+		show_info->width = n;
+	if (branch_info->rebase)
+		show_info->any_rebase = 1;
+
+	item = string_list_insert(branch_item->string, show_info->list);
+	item->util = branch_info;
+
+	return 0;
+}
+
+int show_local_info_item(struct string_list_item *item, void *cb_data)
+{
+	struct show_info *show_info = cb_data;
+	struct branch_info *branch_info = item->util;
+	struct string_list *merge = &branch_info->merge;
+	const char *also;
+	int i;
+
+	if (branch_info->rebase && branch_info->merge.nr > 1) {
+		error("invalid branch.%s.merge; cannot rebase onto > 1 branch",
+			item->string);
+		return 0;
+	}
+
+	printf("    %-*s ", show_info->width, item->string);
+	if (branch_info->rebase) {
+		printf("rebases onto remote %s\n", merge->items[0].string);
+		return 0;
+	} else if (show_info->any_rebase) {
+		printf(" merges with remote %s\n", merge->items[0].string);
+		also = "    and with remote";
+	} else {
+		printf("merges with remote %s\n", merge->items[0].string);
+		also = "   and with remote";
+	}
+	for (i = 1; i < merge->nr; i++)
+		printf("    %-*s %s %s\n", show_info->width, "", also,
+		       merge->items[i].string);
 
 	return 0;
 }
@@ -711,6 +803,8 @@ static int show(int argc, const char **argv)
 		OPT_END()
 	};
 	struct ref_states states;
+	struct string_list info_list = { NULL, 0, 0, 0 };
+	struct show_info info;
 
 	argc = parse_options(argc, argv, options, builtin_remote_usage, 0);
 
@@ -718,6 +812,9 @@ static int show(int argc, const char **argv)
 		return show_all();
 
 	memset(&states, 0, sizeof(states));
+	memset(&info, 0, sizeof(info));
+	info.states = &states;
+	info.list = &info_list;
 	for (; argc; argc--, argv++) {
 		int i;
 
@@ -731,37 +828,37 @@ static int show(int argc, const char **argv)
 		else if (!states.heads.nr)
 			printf("  HEAD branch: (unknown)\n");
 		else if (states.heads.nr == 1)
-			printf("  HEAD branch: %s\n",
-				states.heads.items[0].string);
-		else
-			show_list("  HEAD branch%s:", &states.heads, "");
-
-		for (i = 0; i < branch_list.nr; i++) {
-			struct string_list_item *branch = branch_list.items + i;
-			struct branch_info *info = branch->util;
-			int j;
-
-			if (!info->merge.nr || strcmp(*argv, info->remote_name))
-				continue;
-			printf("  Remote branch%s merged with 'git pull' "
-				"while on branch %s\n   ",
-				info->merge.nr > 1 ? "es" : "",
-				branch->string);
-			for (j = 0; j < info->merge.nr; j++)
-				printf(" %s", info->merge.items[j].string);
-			printf("\n");
+			printf("  HEAD branch: %s\n", states.heads.items[0].string);
+		else {
+			printf("  HEAD branch (remote HEAD is ambiguous,"
+			       " may be one of the following):\n");
+			for (i = 0; i < states.heads.nr; i++)
+				printf("    %s\n", states.heads.items[i].string);
 		}
 
-		if (!no_query) {
-			show_list("  New remote branch%s (next fetch "
-				"will store in remotes/%s)",
-				&states.new, states.remote->name);
-			show_list("  Stale tracking branch%s (use 'git remote "
-				"prune')", &states.stale, "");
-		}
+		/* remote branch info */
+		info.width = 0;
+		for_each_string_list(add_remote_to_show_info, &states.new, &info);
+		for_each_string_list(add_remote_to_show_info, &states.tracked, &info);
+		for_each_string_list(add_remote_to_show_info, &states.stale, &info);
+		if (info.list->nr)
+			printf("  Remote branch%s:%s\n",
+			       info.list->nr > 1 ? "es" : "",
+				no_query ? " (status not queried)" : "");
+		for_each_string_list(show_remote_info_item, info.list, &info);
+		string_list_clear(info.list, 0);
 
-		show_list("  Tracked remote branch%s", &states.tracked, "");
+		/* git pull info */
+		info.width = 0;
+		info.any_rebase = 0;
+		for_each_string_list(add_local_to_show_info, &branch_list, &info);
+		if (info.list->nr)
+			printf("  Local branch%s configured for 'git pull':\n",
+			       info.list->nr > 1 ? "es" : "");
+		for_each_string_list(show_local_info_item, info.list, &info);
+		string_list_clear(info.list, 0);
 
+		/* git push info */
 		if (states.remote->push_refspec_nr) {
 			printf("  Local branch%s pushed with 'git push'\n",
 				states.remote->push_refspec_nr > 1 ?
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index fdc4a29..6dd971e 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -28,7 +28,7 @@ tokens_match () {
 }
 
 check_remote_track () {
-	actual=$(git remote show "$1" | sed -e '1,/Tracked/d') &&
+	actual=$(git remote show "$1" | sed -ne 's|^    \(.*\) tracked$|\1|p')
 	shift &&
 	tokens_match "$*" "$actual"
 }
@@ -137,19 +137,21 @@ cat > test/expect << EOF
 * remote origin
   URL: $(pwd)/one
   HEAD branch: master
-  Remote branch merged with 'git pull' while on branch master
-    master
-  New remote branch (next fetch will store in remotes/origin)
-    master
-  Tracked remote branches
-    master
-    side
+  Remote branches:
+    master new (next fetch will store in remotes/origin)
+    side   tracked
+  Local branches configured for 'git pull':
+    foo    rebases onto remote master
+    master  merges with remote master
+    oct     merges with remote topic-a
+               and with remote topic-b
+               and with remote topic-c
   Local branches pushed with 'git push'
     master:upstream
     +refs/tags/lastbackup
 * remote two
   URL: ../two
-  HEAD branches:
+  HEAD branch (remote HEAD is ambiguous, may be one of the following):
     another
     master
 EOF
@@ -159,8 +161,12 @@ test_expect_success 'show' '
 	 git config --add remote.origin.fetch \
 		refs/heads/master:refs/heads/upstream &&
 	 git fetch &&
+	 git branch --track oct origin/master &&
+	 git branch --track foo origin/master &&
 	 git branch -d -r origin/master &&
 	 git config --add remote.two.url ../two &&
+	 git config branch.foo.rebase true &&
+	 git config branch.oct.merge "topic-a topic-b topic-c" &&
 	 (cd ../one &&
 	  echo 1 > file &&
 	  test_tick &&
@@ -170,6 +176,7 @@ test_expect_success 'show' '
 	 git config --add remote.origin.push \
 		+refs/tags/lastbackup &&
 	 git remote show origin two > output &&
+	 git branch -d foo oct &&
 	 test_cmp expect output)
 '
 
@@ -177,11 +184,11 @@ cat > test/expect << EOF
 * remote origin
   URL: $(pwd)/one
   HEAD branch: (not queried)
-  Remote branch merged with 'git pull' while on branch master
-    master
-  Tracked remote branches
+  Remote branches: (status not queried)
     master
     side
+  Local branch configured for 'git pull':
+    master merges with remote master
   Local branches pushed with 'git push'
     master:upstream
     +refs/tags/lastbackup
-- 
1.6.2.rc1.223.gfed32

[PATCH 08/13] refactor duplicated get_local_heads() to remote.c

[Jay Soffian]

get_local_heads() appears to have been copied from builtin-send-pack.c
to http-push.c via cut and paste. This patch moves the function and its
helper one_local_ref() to remote.c.

The two copies of one_local_ref() were not identical. I used the more
recent version from builtin-send-pack.c after confirming with Jeff King
that it was an oversight that commit 30affa1e did not update both
copies.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin-send-pack.c |   29 ++---------------------------
 http-push.c         |   23 ++---------------------
 remote.c            |   26 ++++++++++++++++++++++++++
 remote.h            |    1 +
 4 files changed, 31 insertions(+), 48 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index d65d019..2fbfc29 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -133,33 +133,8 @@ static int ref_newer(const unsigned char *new_sha1,
 	return found;
 }
 
-static struct ref *local_refs, **local_tail;
 static struct ref *remote_refs, **remote_tail;
 
-static int one_local_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
-{
-	struct ref *ref;
-	int len;
-
-	/* we already know it starts with refs/ to get here */
-	if (check_ref_format(refname + 5))
-		return 0;
-
-	len = strlen(refname) + 1;
-	ref = xcalloc(1, sizeof(*ref) + len);
-	hashcpy(ref->new_sha1, sha1);
-	memcpy(ref->name, refname, len);
-	*local_tail = ref;
-	local_tail = &ref->next;
-	return 0;
-}
-
-static void get_local_heads(void)
-{
-	local_tail = &local_refs;
-	for_each_ref(one_local_ref, NULL);
-}
-
 static int receive_status(int in, struct ref *refs)
 {
 	struct ref *hint;
@@ -387,7 +362,7 @@ static int refs_pushed(struct ref *ref)
 
 static int do_send_pack(int in, int out, struct remote *remote, const char *dest, int nr_refspec, const char **refspec)
 {
-	struct ref *ref;
+	struct ref *ref, *local_refs;
 	int new_refs;
 	int ask_for_status_report = 0;
 	int allow_deleting_refs = 0;
@@ -405,7 +380,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 	/* No funny business with the matcher */
 	remote_tail = get_remote_heads(in, &remote_refs, 0, NULL, REF_NORMAL,
 				       &extra_have);
-	get_local_heads();
+	local_refs = get_local_heads();
 
 	/* Does the other end support the reporting? */
 	if (server_supports("report-status"))
diff --git a/http-push.c b/http-push.c
index 30d2d34..cfeed81 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1792,21 +1792,8 @@ static int update_remote(unsigned char *sha1, struct remote_lock *lock)
 	return 1;
 }
 
-static struct ref *local_refs, **local_tail;
 static struct ref *remote_refs, **remote_tail;
 
-static int one_local_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
-{
-	struct ref *ref;
-	int len = strlen(refname) + 1;
-	ref = xcalloc(1, sizeof(*ref) + len);
-	hashcpy(ref->new_sha1, sha1);
-	memcpy(ref->name, refname, len);
-	*local_tail = ref;
-	local_tail = &ref->next;
-	return 0;
-}
-
 static void one_remote_ref(char *refname)
 {
 	struct ref *ref;
@@ -1839,12 +1826,6 @@ static void one_remote_ref(char *refname)
 	remote_tail = &ref->next;
 }
 
-static void get_local_heads(void)
-{
-	local_tail = &local_refs;
-	for_each_ref(one_local_ref, NULL);
-}
-
 static void get_dav_remote_heads(void)
 {
 	remote_tail = &remote_refs;
@@ -2195,7 +2176,7 @@ int main(int argc, char **argv)
 	int rc = 0;
 	int i;
 	int new_refs;
-	struct ref *ref;
+	struct ref *ref, *local_refs;
 	char *rewritten_url = NULL;
 
 	git_extract_argv0_path(argv[0]);
@@ -2302,7 +2283,7 @@ int main(int argc, char **argv)
 		fetch_indices();
 
 	/* Get a list of all local and remote heads to validate refspecs */
-	get_local_heads();
+	local_refs = get_local_heads();
 	fprintf(stderr, "Fetching remote heads...\n");
 	get_dav_remote_heads();
 
diff --git a/remote.c b/remote.c
index 9f4821c..2c010a1 100644
--- a/remote.c
+++ b/remote.c
@@ -1427,3 +1427,29 @@ struct ref *guess_remote_head(const struct ref *head,
 
 	return list;
 }
+
+static int one_local_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
+{
+	struct ref ***local_tail = cb_data;
+	struct ref *ref;
+	int len;
+
+	/* we already know it starts with refs/ to get here */
+	if (check_ref_format(refname + 5))
+		return 0;
+
+	len = strlen(refname) + 1;
+	ref = xcalloc(1, sizeof(*ref) + len);
+	hashcpy(ref->new_sha1, sha1);
+	memcpy(ref->name, refname, len);
+	**local_tail = ref;
+	*local_tail = &ref->next;
+	return 0;
+}
+
+struct ref *get_local_heads(void)
+{
+	struct ref *local_refs, **local_tail = &local_refs;
+	for_each_ref(one_local_ref, &local_tail);
+	return local_refs;
+}
diff --git a/remote.h b/remote.h
index 98aeeb8..41b7ac2 100644
--- a/remote.h
+++ b/remote.h
@@ -149,4 +149,5 @@ struct ref *guess_remote_head(const struct ref *head,
 			      const struct ref *refs,
 			      int all);
 
+struct ref *get_local_heads(void);
 #endif
-- 
1.6.2.rc1.223.gfed32

[PATCH 09/13] refactor duplicated ref_newer() to remote.c

[Jay Soffian]

ref_newer() appears to have been copied from builtin-send-pack.c to
http-push.c via cut and paste. This patch moves the function and its
helper unmark_and_free() to remote.c.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
Hmm, I just noticed that the version in builtin-send-pack.c uses 1
whereas http-push.c uses TMP_MARK as the mark argument. I used the
version from builtin-send-pack.c. I don't know if this is signfificant.

 builtin-send-pack.c |   49 -------------------------------------------------
 http-push.c         |   49 -------------------------------------------------
 remote.c            |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 remote.h            |    1 +
 4 files changed, 50 insertions(+), 98 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 2fbfc29..186cecb 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -84,55 +84,6 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 	return 0;
 }
 
-static void unmark_and_free(struct commit_list *list, unsigned int mark)
-{
-	while (list) {
-		struct commit_list *temp = list;
-		temp->item->object.flags &= ~mark;
-		list = temp->next;
-		free(temp);
-	}
-}
-
-static int ref_newer(const unsigned char *new_sha1,
-		     const unsigned char *old_sha1)
-{
-	struct object *o;
-	struct commit *old, *new;
-	struct commit_list *list, *used;
-	int found = 0;
-
-	/* Both new and old must be commit-ish and new is descendant of
-	 * old.  Otherwise we require --force.
-	 */
-	o = deref_tag(parse_object(old_sha1), NULL, 0);
-	if (!o || o->type != OBJ_COMMIT)
-		return 0;
-	old = (struct commit *) o;
-
-	o = deref_tag(parse_object(new_sha1), NULL, 0);
-	if (!o || o->type != OBJ_COMMIT)
-		return 0;
-	new = (struct commit *) o;
-
-	if (parse_commit(new) < 0)
-		return 0;
-
-	used = list = NULL;
-	commit_list_insert(new, &list);
-	while (list) {
-		new = pop_most_recent_commit(&list, 1);
-		commit_list_insert(new, &used);
-		if (new == old) {
-			found = 1;
-			break;
-		}
-	}
-	unmark_and_free(list, 1);
-	unmark_and_free(used, 1);
-	return found;
-}
-
 static struct ref *remote_refs, **remote_tail;
 
 static int receive_status(int in, struct ref *refs)
diff --git a/http-push.c b/http-push.c
index cfeed81..392533a 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1843,55 +1843,6 @@ static int is_zero_sha1(const unsigned char *sha1)
 	return 1;
 }
 
-static void unmark_and_free(struct commit_list *list, unsigned int mark)
-{
-	while (list) {
-		struct commit_list *temp = list;
-		temp->item->object.flags &= ~mark;
-		list = temp->next;
-		free(temp);
-	}
-}
-
-static int ref_newer(const unsigned char *new_sha1,
-		     const unsigned char *old_sha1)
-{
-	struct object *o;
-	struct commit *old, *new;
-	struct commit_list *list, *used;
-	int found = 0;
-
-	/* Both new and old must be commit-ish and new is descendant of
-	 * old.  Otherwise we require --force.
-	 */
-	o = deref_tag(parse_object(old_sha1), NULL, 0);
-	if (!o || o->type != OBJ_COMMIT)
-		return 0;
-	old = (struct commit *) o;
-
-	o = deref_tag(parse_object(new_sha1), NULL, 0);
-	if (!o || o->type != OBJ_COMMIT)
-		return 0;
-	new = (struct commit *) o;
-
-	if (parse_commit(new) < 0)
-		return 0;
-
-	used = list = NULL;
-	commit_list_insert(new, &list);
-	while (list) {
-		new = pop_most_recent_commit(&list, TMP_MARK);
-		commit_list_insert(new, &used);
-		if (new == old) {
-			found = 1;
-			break;
-		}
-	}
-	unmark_and_free(list, TMP_MARK);
-	unmark_and_free(used, TMP_MARK);
-	return found;
-}
-
 static void add_remote_info_ref(struct remote_ls_ctx *ls)
 {
 	struct strbuf *buf = (struct strbuf *)ls->userData;
diff --git a/remote.c b/remote.c
index 2c010a1..5cb443f 100644
--- a/remote.c
+++ b/remote.c
@@ -5,6 +5,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "dir.h"
+#include "tag.h"
 
 static struct refspec s_tag_refspec = {
 	0,
@@ -1271,6 +1272,54 @@ int resolve_remote_symref(struct ref *ref, struct ref *list)
 	return 1;
 }
 
+static void unmark_and_free(struct commit_list *list, unsigned int mark)
+{
+	while (list) {
+		struct commit_list *temp = list;
+		temp->item->object.flags &= ~mark;
+		list = temp->next;
+		free(temp);
+	}
+}
+
+int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)
+{
+	struct object *o;
+	struct commit *old, *new;
+	struct commit_list *list, *used;
+	int found = 0;
+
+	/* Both new and old must be commit-ish and new is descendant of
+	 * old.  Otherwise we require --force.
+	 */
+	o = deref_tag(parse_object(old_sha1), NULL, 0);
+	if (!o || o->type != OBJ_COMMIT)
+		return 0;
+	old = (struct commit *) o;
+
+	o = deref_tag(parse_object(new_sha1), NULL, 0);
+	if (!o || o->type != OBJ_COMMIT)
+		return 0;
+	new = (struct commit *) o;
+
+	if (parse_commit(new) < 0)
+		return 0;
+
+	used = list = NULL;
+	commit_list_insert(new, &list);
+	while (list) {
+		new = pop_most_recent_commit(&list, 1);
+		commit_list_insert(new, &used);
+		if (new == old) {
+			found = 1;
+			break;
+		}
+	}
+	unmark_and_free(list, 1);
+	unmark_and_free(used, 1);
+	return found;
+}
+
 /*
  * Return true if there is anything to report, otherwise false.
  */
diff --git a/remote.h b/remote.h
index 41b7ac2..e14d041 100644
--- a/remote.h
+++ b/remote.h
@@ -76,6 +76,7 @@ int check_ref_type(const struct ref *ref, int flags);
 void free_refs(struct ref *ref);
 
 int resolve_remote_symref(struct ref *ref, struct ref *list);
+int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1);
 
 /*
  * Removes and frees any duplicate refs in the map.
-- 
1.6.2.rc1.223.gfed32

[PATCH 10/13] remote.c: make match_refs() copy src ref before assigning to peer_ref

[Jay Soffian]

In some instances, match_refs() was setting the peer_ref field in the
dst ref list such that it pointed to a ref in the src list, instead of
to a copy. This would cause a double-free when both the src and dst ref
lists were freed, as free_refs() frees the peer_ref. A double-free would
also occur if the same branch was pushed to two different destinations
such as:

  push = refs/heads/master:refs/heads/backup
  push = refs/heads/master:refs/heads/master

This patch corrects the problem by copying the ref when it has been
plucked from the src list.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 remote.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index 5cb443f..ab387f2 100644
--- a/remote.c
+++ b/remote.c
@@ -930,6 +930,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 			  struct refspec *rs)
 {
 	struct ref *matched_src, *matched_dst;
+	int copy_src;
 
 	const char *dst_value = rs->dst;
 	char *dst_guess;
@@ -940,6 +941,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 	matched_src = matched_dst = NULL;
 	switch (count_refspec_match(rs->src, src, &matched_src)) {
 	case 1:
+		copy_src = 1;
 		break;
 	case 0:
 		/* The source could be in the get_sha1() format
@@ -949,6 +951,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 		matched_src = try_explicit_object_name(rs->src);
 		if (!matched_src)
 			return error("src refspec %s does not match any.", rs->src);
+		copy_src = 0;
 		break;
 	default:
 		return error("src refspec %s matches more than one.", rs->src);
@@ -994,7 +997,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 		return error("dst ref %s receives from more than one src.",
 		      matched_dst->name);
 	else {
-		matched_dst->peer_ref = matched_src;
+		matched_dst->peer_ref = copy_src ? copy_ref(matched_src) : matched_src;
 		matched_dst->force = rs->force;
 	}
 	return 0;
@@ -1102,7 +1105,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
 			dst_peer = make_linked_ref(dst_name, dst_tail);
 			hashcpy(dst_peer->new_sha1, src->new_sha1);
 		}
-		dst_peer->peer_ref = src;
+		dst_peer->peer_ref = copy_ref(src);
 		dst_peer->force = pat->force;
 	free_name:
 		free(dst_name);
-- 
1.6.2.rc1.223.gfed32

[PATCH 11/13] remote.c: don't short-circuit match_refs() when error in match_explicit_refs()

[Jay Soffian]

match_refs() returns non-zero if there is an error in
match_explicit_refs(), without handling any remaining pattern ref specs.

Its existing callers exit upon receiving non-zero, so a partial result
is of no consequence to them; however it is about to have a new caller
that is interested in the complete result even if there are errors in
match_explicit_refs().

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 remote.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index ab387f2..5c14343 100644
--- a/remote.c
+++ b/remote.c
@@ -1046,6 +1046,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
 	struct refspec *rs;
 	int send_all = flags & MATCH_REFS_ALL;
 	int send_mirror = flags & MATCH_REFS_MIRROR;
+	int errs;
 	static const char *default_refspec[] = { ":", 0 };
 
 	if (!nr_refspec) {
@@ -1053,8 +1054,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
 		refspec = default_refspec;
 	}
 	rs = parse_push_refspec(nr_refspec, (const char **) refspec);
-	if (match_explicit_refs(src, dst, dst_tail, rs, nr_refspec))
-		return -1;
+	errs = match_explicit_refs(src, dst, dst_tail, rs, nr_refspec);
 
 	/* pick the remainder */
 	for ( ; src; src = src->next) {
@@ -1110,6 +1110,8 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
 	free_name:
 		free(dst_name);
 	}
+	if (errs)
+		return -1;
 	return 0;
 }
 
-- 
1.6.2.rc1.223.gfed32

[PATCH 12/13] remote.c: refactor get_remote_ref_states()

[Jay Soffian]

get_remote_ref_states() has three callers, but each is interested in
slightly different information. Give it a bit-field flag so that callers
can specify which pieces of information they need.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin-remote.c |   23 +++++++++++++++--------
 1 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index 4975a0e..327667a 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -19,6 +19,9 @@ static const char * const builtin_remote_usage[] = {
 	NULL
 };
 
+#define GET_REF_STATES (1<<0)
+#define GET_HEAD_NAMES (1<<1)
+
 static int verbose;
 
 static int show_all(void);
@@ -685,14 +688,15 @@ static int get_remote_ref_states(const char *name,
 			states->remote->url[0] : NULL);
 		remote_refs = transport_get_remote_refs(transport);
 		transport_disconnect(transport);
-		get_head_names(remote_refs, name, states);
-		get_ref_states(remote_refs, states);
+		states->queried = 1;
+		if (query & GET_REF_STATES)
+			get_ref_states(remote_refs, states);
+		if (query & GET_HEAD_NAMES)
+			get_head_names(remote_refs, name, states);
 	} else {
 		for_each_ref(append_ref_to_tracked_list, states);
 		sort_string_list(&states->tracked);
 	}
-	states->queried = query;
-
 	return 0;
 }
 
@@ -796,7 +800,7 @@ int show_local_info_item(struct string_list_item *item, void *cb_data)
 
 static int show(int argc, const char **argv)
 {
-	int no_query = 0, result = 0;
+	int no_query = 0, result = 0, query_flag = 0;
 	struct option options[] = {
 		OPT_GROUP("show specific options"),
 		OPT_BOOLEAN('n', NULL, &no_query, "do not query remotes"),
@@ -811,6 +815,9 @@ static int show(int argc, const char **argv)
 	if (argc < 1)
 		return show_all();
 
+	if (!no_query)
+		query_flag = (GET_REF_STATES | GET_HEAD_NAMES);
+
 	memset(&states, 0, sizeof(states));
 	memset(&info, 0, sizeof(info));
 	info.states = &states;
@@ -818,7 +825,7 @@ static int show(int argc, const char **argv)
 	for (; argc; argc--, argv++) {
 		int i;
 
-		get_remote_ref_states(*argv, &states, !no_query);
+		get_remote_ref_states(*argv, &states, query_flag);
 
 		printf("* remote %s\n  URL: %s\n", *argv,
 			states.remote->url_nr > 0 ?
@@ -902,7 +909,7 @@ static int set_head(int argc, const char **argv)
 	} else if (opt_a && !opt_d && argc == 1) {
 		struct ref_states states;
 		memset(&states, 0, sizeof(states));
-		get_remote_ref_states(argv[0], &states, 1);
+		get_remote_ref_states(argv[0], &states, GET_HEAD_NAMES);
 		if (!states.heads.nr)
 			result |= error("Cannot determine remote HEAD");
 		else if (states.heads.nr > 1) {
@@ -962,7 +969,7 @@ static int prune(int argc, const char **argv)
 	for (; argc; argc--, argv++) {
 		int i;
 
-		get_remote_ref_states(*argv, &states, 1);
+		get_remote_ref_states(*argv, &states, GET_REF_STATES);
 
 		if (states.stale.nr) {
 			printf("Pruning %s\n", *argv);
-- 
1.6.2.rc1.223.gfed32

[PATCH 13/13] remote: new show output style for push refspecs

[Jay Soffian]

The existing output of "git remote show <remote>" with respect to push
ref specs is basically just to show the raw refspec. This patch teaches
the command to interpret the refspecs and show how each branch will be
pushed to the destination. The output gives the user an idea of what
"git push" should do if it is run w/o any arguments.

Example new output:

1a. Typical output with no push refspec (i.e. matching branches only)

$ git remote show origin
* remote origin
  [...]
  Local refs configured for 'git push':
    master pushes to master (up to date)
    next   pushes to next   (local out of date)

1b. Same as above, w/o querying the remote:

$ git remote show origin -n
* remote origin
  [...]
  Local ref configured for 'git push' (status not queried):
    (matching) pushes to (matching)

2a. With a forcing refspec (+), and a new topic
    (something like push = refs/heads/*:refs/heads/*):

$ git remote show origin
* remote origin
  [...]
  Local refs configured for 'git push':
    master     pushes to master    (fast forwardable)
    new-topic  pushes to new-topic (create)
    next       pushes to next      (local out of date)
    pu         forces to pu        (up to date)

2b. Same as above, w/o querying the remote

$ git remote show origin -n
* remote origin
  [...]
  Local refs configured for 'git push' (status not queried):
    master     pushes to master
    new-topic  pushes to new-topic
    next       pushes to next
    pu         forces to pu

3. With a remote configured as a mirror:

* remote backup
  [...]
  Local refs will be mirrored by 'git push'

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
After spending quite a long time standing in the paint aisle looking
at all the different colors, this is my favorite.

You might like a different color paint. For example, an obvious choice
might seem to be single character status fields a-la ls-files. But that
would sacrifice clarity (IMO) as the user then has to go lookup what the
values mean in a man page, and in any case wouldn't be' consistent with
the rest of the new "show" output.

Also, "forces to" may sound a little funny, but I think it is obvious
what it means and lines up more nicely than "force pushes" or any other
choice I considered.

Anyway, I built this new shed, so I get to pick the color. (And
hopefully no one blows it up w/a stick of dynamite) :-)

 builtin-remote.c  |  201 ++++++++++++++++++++++++++++++++++++++++++++++++----
 t/t5505-remote.sh |   57 +++++++++------
 2 files changed, 219 insertions(+), 39 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index 327667a..ac49219 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -21,6 +21,7 @@ static const char * const builtin_remote_usage[] = {
 
 #define GET_REF_STATES (1<<0)
 #define GET_HEAD_NAMES (1<<1)
+#define GET_PUSH_REF_STATES (1<<2)
 
 static int verbose;
 
@@ -220,7 +221,7 @@ static void read_branches(void)
 
 struct ref_states {
 	struct remote *remote;
-	struct string_list new, stale, tracked, heads;
+	struct string_list new, stale, tracked, push, heads;
 	int queried;
 };
 
@@ -275,6 +276,112 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat
 	return 0;
 }
 
+struct push_info {
+	char *dest;
+	int forced;
+	enum {
+		PUSH_STATUS_CREATE = 0,
+		PUSH_STATUS_DELETE,
+		PUSH_STATUS_UPTODATE,
+		PUSH_STATUS_FASTFORWARD,
+		PUSH_STATUS_OUTOFDATE,
+		PUSH_STATUS_NOTQUERIED,
+	} status;
+};
+
+static int get_push_ref_states(const struct ref *remote_refs,
+	struct ref_states *states)
+{
+	struct remote *remote = states->remote;
+	struct ref *ref, *local_refs, *push_map, **push_tail;
+	if (remote->mirror)
+		return 0;
+
+	local_refs = get_local_heads();
+	ref = push_map = copy_ref_list(remote_refs);
+	while (ref->next)
+		ref = ref->next;
+	push_tail = &ref->next;
+
+	match_refs(local_refs, push_map, &push_tail, remote->push_refspec_nr,
+		   remote->push_refspec, MATCH_REFS_NONE);
+
+	states->push.strdup_strings = 1;
+	for (ref = push_map; ref; ref = ref->next) {
+		struct string_list_item *item;
+		struct push_info *info;
+
+		if (!ref->peer_ref)
+			continue;
+		hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
+
+		item = string_list_append(abbrev_branch(ref->peer_ref->name),
+					  &states->push);
+		item->util = xcalloc(sizeof(struct push_info), 1);
+		info = item->util;
+		info->forced = ref->force;
+		info->dest = xstrdup(abbrev_branch(ref->name));
+
+		if (is_null_sha1(ref->new_sha1)) {
+			info->status = PUSH_STATUS_DELETE;
+		} else if (!hashcmp(ref->old_sha1, ref->new_sha1))
+			info->status = PUSH_STATUS_UPTODATE;
+		else if (is_null_sha1(ref->old_sha1))
+			info->status = PUSH_STATUS_CREATE;
+		else if (has_sha1_file(ref->old_sha1) &&
+			 ref_newer(ref->new_sha1, ref->old_sha1))
+			info->status = PUSH_STATUS_FASTFORWARD;
+		else
+			info->status = PUSH_STATUS_OUTOFDATE;
+		// ref->peer_ref = NULL; /* local ref which is freed below */
+	}
+	free_refs(local_refs);
+	free_refs(push_map);
+	return 0;
+}
+
+static int get_push_ref_states_noquery(struct ref_states *states)
+{
+	int i;
+	struct remote *remote = states->remote;
+	struct string_list_item *item;
+	struct push_info *info;
+
+	if (remote->mirror)
+		return 0;
+
+	states->push.strdup_strings = 1;
+	if (!remote->push_refspec_nr) {
+		item = string_list_append("(matching)", &states->push);
+		info = item->util = xcalloc(sizeof(struct push_info), 1);
+		info->status = PUSH_STATUS_NOTQUERIED;
+		info->dest = xstrdup(item->string);
+	}
+	for (i = 0; i < remote->push_refspec_nr; i++) {
+		struct refspec *spec = remote->push + i;
+		char buf[PATH_MAX];
+		if (spec->matching)
+			item = string_list_append("(matching)", &states->push);
+		else if (spec->pattern) {
+			snprintf(buf, (sizeof(buf)), "%s*", spec->src);
+			item = string_list_append(buf, &states->push);
+			snprintf(buf, (sizeof(buf)), "%s*", spec->dst);
+		} else if (strlen(spec->src))
+			item = string_list_append(spec->src, &states->push);
+		else
+			item = string_list_append("(delete)", &states->push);
+
+		info = item->util = xcalloc(sizeof(struct push_info), 1);
+		info->forced = spec->force;
+		info->status = PUSH_STATUS_NOTQUERIED;
+		if (spec->pattern)
+			info->dest = xstrdup(buf);
+		else
+			info->dest = xstrdup(spec->dst ? spec->dst : item->string);
+	}
+	return 0;
+}
+
 static int get_head_names(const struct ref *remote_refs,
 	const char *remote_name, struct ref_states *states)
 {
@@ -645,12 +752,20 @@ static int rm(int argc, const char **argv)
 	return result;
 }
 
+void clear_push_info(void *util, const char *string)
+{
+	struct push_info *info = util;
+	free(info->dest);
+	free(info);
+}
+
 static void free_remote_ref_states(struct ref_states *states)
 {
 	string_list_clear(&states->new, 0);
 	string_list_clear(&states->stale, 0);
 	string_list_clear(&states->tracked, 0);
 	string_list_clear(&states->heads, 0);
+	string_list_clear_func(&states->push, clear_push_info);
 }
 
 static int append_ref_to_tracked_list(const char *refname,
@@ -693,9 +808,12 @@ static int get_remote_ref_states(const char *name,
 			get_ref_states(remote_refs, states);
 		if (query & GET_HEAD_NAMES)
 			get_head_names(remote_refs, name, states);
+		if (query & GET_PUSH_REF_STATES)
+			get_push_ref_states(remote_refs, states);
 	} else {
 		for_each_ref(append_ref_to_tracked_list, states);
 		sort_string_list(&states->tracked);
+		get_push_ref_states_noquery(states);
 	}
 	return 0;
 }
@@ -703,7 +821,7 @@ static int get_remote_ref_states(const char *name,
 struct show_info {
 	struct string_list *list;
 	struct ref_states *states;
-	int width;
+	int width, width2;
 	int any_rebase;
 };
 
@@ -798,6 +916,58 @@ int show_local_info_item(struct string_list_item *item, void *cb_data)
 	return 0;
 }
 
+int add_push_to_show_info(struct string_list_item *push_item, void *cb_data)
+{
+	struct show_info *show_info = cb_data;
+	struct push_info *push_info = push_item->util;
+	struct string_list_item *item;
+	int n;
+	if ((n = strlen(push_item->string)) > show_info->width)
+		show_info->width = n;
+	if ((n = strlen(push_info->dest)) > show_info->width2)
+		show_info->width2 = n;
+	item = string_list_append(push_item->string, show_info->list);
+	item->util = push_item->util;
+	return 0;
+}
+
+int show_push_info_item(struct string_list_item *item, void *cb_data)
+{
+	struct show_info *show_info = cb_data;
+	struct push_info *push_info = item->util;
+	char *src = item->string, *status = NULL;
+
+	switch (push_info->status) {
+	case PUSH_STATUS_CREATE:
+		status = "create";
+		break;
+	case PUSH_STATUS_DELETE:
+		status = "delete";
+		src = "(none)";
+		break;
+	case PUSH_STATUS_UPTODATE:
+		status = "up to date";
+		break;
+	case PUSH_STATUS_FASTFORWARD:
+		status = "fast forwardable";
+		break;
+	case PUSH_STATUS_OUTOFDATE:
+		status = "local out of date";
+		break;
+	case PUSH_STATUS_NOTQUERIED:
+		break;
+	}
+	if (status)
+		printf("    %-*s %s to %-*s (%s)\n", show_info->width, src,
+			push_info->forced ? "forces" : "pushes",
+			show_info->width2, push_info->dest, status);
+	else
+		printf("    %-*s %s to %s\n", show_info->width, src,
+			push_info->forced ? "forces" : "pushes",
+			push_info->dest);
+	return 0;
+}
+
 static int show(int argc, const char **argv)
 {
 	int no_query = 0, result = 0, query_flag = 0;
@@ -816,7 +986,7 @@ static int show(int argc, const char **argv)
 		return show_all();
 
 	if (!no_query)
-		query_flag = (GET_REF_STATES | GET_HEAD_NAMES);
+		query_flag = (GET_REF_STATES | GET_HEAD_NAMES | GET_PUSH_REF_STATES);
 
 	memset(&states, 0, sizeof(states));
 	memset(&info, 0, sizeof(info));
@@ -866,19 +1036,18 @@ static int show(int argc, const char **argv)
 		string_list_clear(info.list, 0);
 
 		/* git push info */
-		if (states.remote->push_refspec_nr) {
-			printf("  Local branch%s pushed with 'git push'\n",
-				states.remote->push_refspec_nr > 1 ?
-					"es" : "");
-			for (i = 0; i < states.remote->push_refspec_nr; i++) {
-				struct refspec *spec = states.remote->push + i;
-				printf("    %s%s%s%s\n",
-				       spec->force ? "+" : "",
-				       abbrev_branch(spec->src),
-				       spec->dst ? ":" : "",
-				       spec->dst ? abbrev_branch(spec->dst) : "");
-			}
-		}
+		if (states.remote->mirror)
+			printf("  Local refs will be mirrored by 'git push'\n");
+
+		info.width = info.width2 = 0;
+		for_each_string_list(add_push_to_show_info, &states.push, &info);
+		sort_string_list(info.list);
+		if (info.list->nr)
+			printf("  Local ref%s configured for 'git push'%s:\n",
+				info.list->nr > 1 ? "s" : "",
+				no_query ? " (status not queried)" : "");
+		for_each_string_list(show_push_info_item, info.list, &info);
+		string_list_clear(info.list, 0);
 
 		free_remote_ref_states(&states);
 	}
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 6dd971e..95012e7 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -141,42 +141,51 @@ cat > test/expect << EOF
     master new (next fetch will store in remotes/origin)
     side   tracked
   Local branches configured for 'git pull':
-    foo    rebases onto remote master
-    master  merges with remote master
-    oct     merges with remote topic-a
-               and with remote topic-b
-               and with remote topic-c
-  Local branches pushed with 'git push'
-    master:upstream
-    +refs/tags/lastbackup
+    ahead    merges with remote master
+    master   merges with remote master
+    octopus  merges with remote topic-a
+                and with remote topic-b
+                and with remote topic-c
+    rebase  rebases onto remote master
+  Local refs configured for 'git push':
+    master pushes to master   (local out of date)
+    master pushes to upstream (create)
 * remote two
   URL: ../two
   HEAD branch (remote HEAD is ambiguous, may be one of the following):
     another
     master
+  Local refs configured for 'git push':
+    ahead  forces to master  (fast forwardable)
+    master pushes to another (up to date)
 EOF
 
 test_expect_success 'show' '
 	(cd test &&
-	 git config --add remote.origin.fetch \
-		refs/heads/master:refs/heads/upstream &&
+	 git config --add remote.origin.fetch refs/heads/master:refs/heads/upstream &&
 	 git fetch &&
-	 git branch --track oct origin/master &&
-	 git branch --track foo origin/master &&
+	 git checkout -b ahead origin/master &&
+	 echo 1 >> file &&
+	 test_tick &&
+	 git commit -m update file &&
+	 git checkout master &&
+	 git branch --track octopus origin/master &&
+	 git branch --track rebase origin/master &&
 	 git branch -d -r origin/master &&
 	 git config --add remote.two.url ../two &&
-	 git config branch.foo.rebase true &&
-	 git config branch.oct.merge "topic-a topic-b topic-c" &&
+	 git config branch.rebase.rebase true &&
+	 git config branch.octopus.merge "topic-a topic-b topic-c" &&
 	 (cd ../one &&
 	  echo 1 > file &&
 	  test_tick &&
 	  git commit -m update file) &&
-	 git config remote.origin.push \
-		refs/heads/master:refs/heads/upstream &&
-	 git config --add remote.origin.push \
-		+refs/tags/lastbackup &&
+	 git config --add remote.origin.push : &&
+	 git config --add remote.origin.push refs/heads/master:refs/heads/upstream &&
+	 git config --add remote.origin.push +refs/tags/lastbackup &&
+	 git config --add remote.two.push +refs/heads/ahead:refs/heads/master &&
+	 git config --add remote.two.push refs/heads/master:refs/heads/another &&
 	 git remote show origin two > output &&
-	 git branch -d foo oct &&
+	 git branch -d rebase octopus &&
 	 test_cmp expect output)
 '
 
@@ -187,11 +196,13 @@ cat > test/expect << EOF
   Remote branches: (status not queried)
     master
     side
-  Local branch configured for 'git pull':
+  Local branches configured for 'git pull':
+    ahead  merges with remote master
     master merges with remote master
-  Local branches pushed with 'git push'
-    master:upstream
-    +refs/tags/lastbackup
+  Local refs configured for 'git push' (status not queried):
+    (matching)           pushes to (matching)
+    refs/heads/master    pushes to refs/heads/upstream
+    refs/tags/lastbackup forces to refs/tags/lastbackup
 EOF
 
 test_expect_success 'show -n' '
-- 
1.6.2.rc1.223.gfed32

Re: [PATCH 09/13] refactor duplicated ref_newer() to remote.c

[Jeff King]

On Mon, Feb 23, 2009 at 01:28:57AM -0500, Jay Soffian wrote:

> ref_newer() appears to have been copied from builtin-send-pack.c to
> http-push.c via cut and paste. This patch moves the function and its
> helper unmark_and_free() to remote.c.
> 
> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
> ---
> Hmm, I just noticed that the version in builtin-send-pack.c uses 1
> whereas http-push.c uses TMP_MARK as the mark argument. I used the
> version from builtin-send-pack.c. I don't know if this is signfificant.

I think the TMP_MARK one is probably better. At the very least, the
send-pack one should be using SEEN instead of a bare '1'. But given that
it is not part of the usual revision machinery, it seems like that was
what TMP_MARK was designed for.

-Peff

Re: [PATCH 07/13] remote: new show output style

[Jeff King]

On Mon, Feb 23, 2009 at 01:28:55AM -0500, Jay Soffian wrote:

> The existing output of "git remote show <remote>" is too verbose for the
> information it provides. This patch teaches it to provide more
> information in less space.
>
> Before the patch:
> [...]
> After this patch:
> [...]

Wow. IMHO, the new one is _way_ easier to read.

-Peff

Re: [PATCH 12/13] remote.c: refactor get_remote_ref_states()

[Jeff King]

On Mon, Feb 23, 2009 at 01:29:00AM -0500, Jay Soffian wrote:

> get_remote_ref_states() has three callers, but each is interested in
> slightly different information. Give it a bit-field flag so that callers
> can specify which pieces of information they need.

Hmph. I think this is probably an indication that
get_remote_ref_states() should really be 3 functions:

  common_stuff();
  query_one();
  query_two();

and then callers can choose the subset they are interested in.

Which is really more or less equivalent; it just seems like extra
obfuscation to have a single function with a bit-field.

-Peff

Re: [PATCH 00/13] New output style for git remote show

[Jeff King]

On Mon, Feb 23, 2009 at 01:28:48AM -0500, Jay Soffian wrote:

> Jay Soffian (13):
>   remote: rename variable and eliminate redundant function call
>   remote: remove unused code in get_ref_states
>   remote: fix two inconsistencies in the output of "show <remote>"
>   remote: make get_remote_ref_states() always populate states.tracked
>   remote: name remote_refs consistently
>   string-list: new for_each_string_list() function
>   remote: new show output style
>   refactor duplicated get_local_heads() to remote.c
>   refactor duplicated ref_newer() to remote.c
>   remote.c: make match_refs() copy src ref before assigning to peer_ref
>   remote.c: don't short-circuit match_refs() when error in
>     match_explicit_refs()
>   remote.c: refactor get_remote_ref_states()
>   remote: new show output style for push refspecs

I gave these a quick read (but not any extensive apply+build+experiment
cycle), and they all look good to me. I don't know when I'll have time
to try breaking them, though. ;)

Thank you for splitting them. It made reading a lot more pleasant.

-Peff

[PATCH v2 09/13] refactor duplicated ref_newer() to remote.c

[Jay Soffian]

ref_newer() appears to have been copied from builtin-send-pack.c to
http-push.c via cut and paste. This patch moves the function and its
helper unmark_and_free() to remote.c. There was a slight difference
between the two implementations, one used TMP_MARK for the mark, the
other used 1. Per Jeff King, I went with TMP_MARK as more correct.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
On Mon, Feb 23, 2009 at 1:45 AM, Jeff King <peff@peff.net> wrote:
 
> I think the TMP_MARK one is probably better. At the very least, the
> send-pack one should be using SEEN instead of a bare '1'. But given that
> it is not part of the usual revision machinery, it seems like that was
> what TMP_MARK was designed for.

Okay then. This replaces the previous in the series using TMP_MARK
instead, otherwise identical.

j.

 builtin-send-pack.c |   49 -------------------------------------------------
 http-push.c         |   49 -------------------------------------------------
 remote.c            |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 remote.h            |    1 +
 4 files changed, 50 insertions(+), 98 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 2fbfc29..186cecb 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -84,55 +84,6 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 	return 0;
 }
 
-static void unmark_and_free(struct commit_list *list, unsigned int mark)
-{
-	while (list) {
-		struct commit_list *temp = list;
-		temp->item->object.flags &= ~mark;
-		list = temp->next;
-		free(temp);
-	}
-}
-
-static int ref_newer(const unsigned char *new_sha1,
-		     const unsigned char *old_sha1)
-{
-	struct object *o;
-	struct commit *old, *new;
-	struct commit_list *list, *used;
-	int found = 0;
-
-	/* Both new and old must be commit-ish and new is descendant of
-	 * old.  Otherwise we require --force.
-	 */
-	o = deref_tag(parse_object(old_sha1), NULL, 0);
-	if (!o || o->type != OBJ_COMMIT)
-		return 0;
-	old = (struct commit *) o;
-
-	o = deref_tag(parse_object(new_sha1), NULL, 0);
-	if (!o || o->type != OBJ_COMMIT)
-		return 0;
-	new = (struct commit *) o;
-
-	if (parse_commit(new) < 0)
-		return 0;
-
-	used = list = NULL;
-	commit_list_insert(new, &list);
-	while (list) {
-		new = pop_most_recent_commit(&list, 1);
-		commit_list_insert(new, &used);
-		if (new == old) {
-			found = 1;
-			break;
-		}
-	}
-	unmark_and_free(list, 1);
-	unmark_and_free(used, 1);
-	return found;
-}
-
 static struct ref *remote_refs, **remote_tail;
 
 static int receive_status(int in, struct ref *refs)
diff --git a/http-push.c b/http-push.c
index cfeed81..392533a 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1843,55 +1843,6 @@ static int is_zero_sha1(const unsigned char *sha1)
 	return 1;
 }
 
-static void unmark_and_free(struct commit_list *list, unsigned int mark)
-{
-	while (list) {
-		struct commit_list *temp = list;
-		temp->item->object.flags &= ~mark;
-		list = temp->next;
-		free(temp);
-	}
-}
-
-static int ref_newer(const unsigned char *new_sha1,
-		     const unsigned char *old_sha1)
-{
-	struct object *o;
-	struct commit *old, *new;
-	struct commit_list *list, *used;
-	int found = 0;
-
-	/* Both new and old must be commit-ish and new is descendant of
-	 * old.  Otherwise we require --force.
-	 */
-	o = deref_tag(parse_object(old_sha1), NULL, 0);
-	if (!o || o->type != OBJ_COMMIT)
-		return 0;
-	old = (struct commit *) o;
-
-	o = deref_tag(parse_object(new_sha1), NULL, 0);
-	if (!o || o->type != OBJ_COMMIT)
-		return 0;
-	new = (struct commit *) o;
-
-	if (parse_commit(new) < 0)
-		return 0;
-
-	used = list = NULL;
-	commit_list_insert(new, &list);
-	while (list) {
-		new = pop_most_recent_commit(&list, TMP_MARK);
-		commit_list_insert(new, &used);
-		if (new == old) {
-			found = 1;
-			break;
-		}
-	}
-	unmark_and_free(list, TMP_MARK);
-	unmark_and_free(used, TMP_MARK);
-	return found;
-}
-
 static void add_remote_info_ref(struct remote_ls_ctx *ls)
 {
 	struct strbuf *buf = (struct strbuf *)ls->userData;
diff --git a/remote.c b/remote.c
index 2c010a1..d0053d5 100644
--- a/remote.c
+++ b/remote.c
@@ -5,6 +5,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "dir.h"
+#include "tag.h"
 
 static struct refspec s_tag_refspec = {
 	0,
@@ -1271,6 +1272,54 @@ int resolve_remote_symref(struct ref *ref, struct ref *list)
 	return 1;
 }
 
+static void unmark_and_free(struct commit_list *list, unsigned int mark)
+{
+	while (list) {
+		struct commit_list *temp = list;
+		temp->item->object.flags &= ~mark;
+		list = temp->next;
+		free(temp);
+	}
+}
+
+int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)
+{
+	struct object *o;
+	struct commit *old, *new;
+	struct commit_list *list, *used;
+	int found = 0;
+
+	/* Both new and old must be commit-ish and new is descendant of
+	 * old.  Otherwise we require --force.
+	 */
+	o = deref_tag(parse_object(old_sha1), NULL, 0);
+	if (!o || o->type != OBJ_COMMIT)
+		return 0;
+	old = (struct commit *) o;
+
+	o = deref_tag(parse_object(new_sha1), NULL, 0);
+	if (!o || o->type != OBJ_COMMIT)
+		return 0;
+	new = (struct commit *) o;
+
+	if (parse_commit(new) < 0)
+		return 0;
+
+	used = list = NULL;
+	commit_list_insert(new, &list);
+	while (list) {
+		new = pop_most_recent_commit(&list, TMP_MARK);
+		commit_list_insert(new, &used);
+		if (new == old) {
+			found = 1;
+			break;
+		}
+	}
+	unmark_and_free(list, TMP_MARK);
+	unmark_and_free(used, TMP_MARK);
+	return found;
+}
+
 /*
  * Return true if there is anything to report, otherwise false.
  */
diff --git a/remote.h b/remote.h
index 41b7ac2..e14d041 100644
--- a/remote.h
+++ b/remote.h
@@ -76,6 +76,7 @@ int check_ref_type(const struct ref *ref, int flags);
 void free_refs(struct ref *ref);
 
 int resolve_remote_symref(struct ref *ref, struct ref *list);
+int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1);
 
 /*
  * Removes and frees any duplicate refs in the map.
-- 
1.6.2.rc1.223.gfed32

Re: [PATCH 12/13] remote.c: refactor get_remote_ref_states()

[Jay Soffian]

On Mon, Feb 23, 2009 at 1:50 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Feb 23, 2009 at 01:29:00AM -0500, Jay Soffian wrote:
>
>> get_remote_ref_states() has three callers, but each is interested in
>> slightly different information. Give it a bit-field flag so that callers
>> can specify which pieces of information they need.
>
> Hmph. I think this is probably an indication that
> get_remote_ref_states() should really be 3 functions:
>
>  common_stuff();
>  query_one();
>  query_two();
>
> and then callers can choose the subset they are interested in.
>
> Which is really more or less equivalent; it just seems like extra
> obfuscation to have a single function with a bit-field.

I see your hmph and raise you a hmph. :-)

Well, I _had_ tried as you suggested first, and thought it yuckier. It
would actually be more like:

caller1() {
  setup_for_get();
  get_thing_one();
}

caller2() {
  setup_for_get();
  get_thing_two();
}

caller3() {
  if (query) {
     setup_for_get();
     get_thing_one();
     get_thing_two();
     get_thing_three();
} else {
     get_thing_one_noquery()
     get_thing_two_noquery();
}

As opposed to:

caller1() {
  get_things(ONE)
}

caller2() {
  get_things(TWO)
}

caller3() {
  things = 0;
  if (query)
     things = (ONE|TWO|THREE)
  get_things(things)
}

I'm not sure why passing a flag saying what you want is obfuscating.
Also, I did find other places in git that do this sort of thing (e.g.,
get_remote_heads).

(Awaiting Junio's lucid reply explaining why my justification is bogus.) :-)

j.

Re: [PATCH 00/13] New output style for git remote show

[Jay Soffian]

On Mon, Feb 23, 2009 at 1:59 AM, Jeff King <peff@peff.net> wrote:
> I gave these a quick read (but not any extensive apply+build+experiment
> cycle), and they all look good to me. I don't know when I'll have time
> to try breaking them, though. ;)
>
> Thank you for splitting them. It made reading a lot more pleasant.

Thank you for the review. I humbly await your breakage attempts.

j.

Re: [PATCH 12/13] remote.c: refactor get_remote_ref_states()

[Jay Soffian]

---
So here's the inter-diff doing it the way you suggest. I'm surprised it
didn't add any LOC. If you really think this is better I'll re-do the
end of the series to do it this way.

 builtin-remote.c |   81 ++++++++++++++++++++++++++---------------------------
 1 files changed, 40 insertions(+), 41 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index ac49219..5651b41 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -19,10 +19,6 @@ static const char * const builtin_remote_usage[] = {
 	NULL
 };
 
-#define GET_REF_STATES (1<<0)
-#define GET_HEAD_NAMES (1<<1)
-#define GET_PUSH_REF_STATES (1<<2)
-
 static int verbose;
 
 static int show_all(void);
@@ -383,7 +379,7 @@ static int get_push_ref_states_noquery(struct ref_states *states)
 }
 
 static int get_head_names(const struct ref *remote_refs,
-	const char *remote_name, struct ref_states *states)
+			  struct ref_states *states)
 {
 	struct ref *ref, *matches;
 	struct ref *fetch_map = NULL, **fetch_map_tail = &fetch_map;
@@ -785,37 +781,15 @@ static int append_ref_to_tracked_list(const char *refname,
 	return 0;
 }
 
-static int get_remote_ref_states(const char *name,
-				 struct ref_states *states,
-				 int query)
+static const struct ref *get_remote_refs(struct ref_states *states)
 {
 	struct transport *transport;
 	const struct ref *remote_refs;
-
-	states->remote = remote_get(name);
-	if (!states->remote)
-		return error("No such remote: %s", name);
-
-	read_branches();
-
-	if (query) {
-		transport = transport_get(NULL, states->remote->url_nr > 0 ?
-			states->remote->url[0] : NULL);
-		remote_refs = transport_get_remote_refs(transport);
-		transport_disconnect(transport);
-		states->queried = 1;
-		if (query & GET_REF_STATES)
-			get_ref_states(remote_refs, states);
-		if (query & GET_HEAD_NAMES)
-			get_head_names(remote_refs, name, states);
-		if (query & GET_PUSH_REF_STATES)
-			get_push_ref_states(remote_refs, states);
-	} else {
-		for_each_ref(append_ref_to_tracked_list, states);
-		sort_string_list(&states->tracked);
-		get_push_ref_states_noquery(states);
-	}
-	return 0;
+	transport = transport_get(NULL, states->remote->url_nr > 0 ?
+				  states->remote->url[0] : NULL);
+	remote_refs = transport_get_remote_refs(transport);
+	transport_disconnect(transport);
+	return remote_refs;
 }
 
 struct show_info {
@@ -970,7 +944,7 @@ int show_push_info_item(struct string_list_item *item, void *cb_data)
 
 static int show(int argc, const char **argv)
 {
-	int no_query = 0, result = 0, query_flag = 0;
+	int no_query = 0, result = 0;
 	struct option options[] = {
 		OPT_GROUP("show specific options"),
 		OPT_BOOLEAN('n', NULL, &no_query, "do not query remotes"),
@@ -985,17 +959,31 @@ static int show(int argc, const char **argv)
 	if (argc < 1)
 		return show_all();
 
-	if (!no_query)
-		query_flag = (GET_REF_STATES | GET_HEAD_NAMES | GET_PUSH_REF_STATES);
-
+	read_branches();
 	memset(&states, 0, sizeof(states));
 	memset(&info, 0, sizeof(info));
+	states.queried = !no_query;
 	info.states = &states;
 	info.list = &info_list;
+
 	for (; argc; argc--, argv++) {
 		int i;
 
-		get_remote_ref_states(*argv, &states, query_flag);
+		if (!(states.remote = remote_get(*argv))) {
+			error("No such remote: %s", *argv);
+			continue;
+		}
+		if (no_query) {
+			for_each_ref(append_ref_to_tracked_list, &states);
+			sort_string_list(&states.tracked);
+			get_push_ref_states_noquery(&states);
+		} else {
+			const struct ref *remote_refs;
+			remote_refs = get_remote_refs(&states);
+			get_ref_states(remote_refs, &states);
+			get_head_names(remote_refs, &states);
+			get_push_ref_states(remote_refs, &states);
+		}
 
 		printf("* remote %s\n  URL: %s\n", *argv,
 			states.remote->url_nr > 0 ?
@@ -1077,8 +1065,13 @@ static int set_head(int argc, const char **argv)
 		head_name = xstrdup(argv[1]);
 	} else if (opt_a && !opt_d && argc == 1) {
 		struct ref_states states;
+		const struct ref *remote_refs;
 		memset(&states, 0, sizeof(states));
-		get_remote_ref_states(argv[0], &states, GET_HEAD_NAMES);
+		read_branches();
+		if (!(states.remote = remote_get(argv[0])))
+			return error("No such remote: %s", argv[0]);
+		remote_refs = get_remote_refs(&states);
+		get_head_names(remote_refs, &states);
 		if (!states.heads.nr)
 			result |= error("Cannot determine remote HEAD");
 		else if (states.heads.nr > 1) {
@@ -1134,11 +1127,17 @@ static int prune(int argc, const char **argv)
 			? " %s will become dangling!\n"
 			: " %s has become dangling!\n");
 
+	read_branches();
 	memset(&states, 0, sizeof(states));
 	for (; argc; argc--, argv++) {
 		int i;
-
-		get_remote_ref_states(*argv, &states, GET_REF_STATES);
+		const struct ref *remote_refs;
+		if (!(states.remote = remote_get(*argv))) {
+			error("No such remote: %s", *argv);
+			continue;
+		}
+		remote_refs = get_remote_refs(&states);
+		get_ref_states(remote_refs, &states);
 
 		if (states.stale.nr) {
 			printf("Pruning %s\n", *argv);
-- 
1.6.2.rc1.223.gfed32

Re: [PATCH v2 09/13] refactor duplicated ref_newer() to remote.c

[Johannes Sixt]

Jay Soffian schrieb:
> diff --git a/remote.c b/remote.c
> index 2c010a1..d0053d5 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -5,6 +5,7 @@
>  #include "diff.h"
>  #include "revision.h"
>  #include "dir.h"
> +#include "tag.h"

You are only moving code from http-push.c and builtin-send-pack.c to
remote.c, but you have to add this '#include "tag.h"' here. How high are
the chances that you can remove the #include lines from http-push.c and
builtin-send-pack.c?

-- Hannes

Re: [PATCH v2 09/13] refactor duplicated ref_newer() to remote.c

[Jay Soffian]

On Mon, Feb 23, 2009 at 3:53 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Jay Soffian schrieb:
>> diff --git a/remote.c b/remote.c
>> index 2c010a1..d0053d5 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -5,6 +5,7 @@
>>  #include "diff.h"
>>  #include "revision.h"
>>  #include "dir.h"
>> +#include "tag.h"
>
> You are only moving code from http-push.c and builtin-send-pack.c to
> remote.c, but you have to add this '#include "tag.h"' here. How high are
> the chances that you can remove the #include lines from http-push.c and
> builtin-send-pack.c?

50/50 as it turns out. Junio, please squash this in if you don't mind:

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 186cecb..9072905 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -1,6 +1,5 @@
 #include "cache.h"
 #include "commit.h"
-#include "tag.h"
 #include "refs.h"
 #include "pkt-line.h"
 #include "run-command.h"


j.

Re: [PATCH 07/13] remote: new show output style

[Marc Branchaud]

Jeff King wrote:
> 
> Wow. IMHO, the new one is _way_ easier to read.

Agreed.  Great work, Jay!  Thanks!

		M.

Re: [PATCH 12/13] remote.c: refactor get_remote_ref_states()

[Jeff King]

On Mon, Feb 23, 2009 at 02:55:32AM -0500, Jay Soffian wrote:

> I see your hmph and raise you a hmph. :-)
> 
> Well, I _had_ tried as you suggested first, and thought it yuckier. It
> would actually be more like:

OK. My comment was along the lines of "why didn't you think of doing it
this way first?" but apparently you did. ;)

> I'm not sure why passing a flag saying what you want is obfuscating.

It doesn't have to be. But from your description and my cursory look
over the code, it seemed like get_remote_ref_states was really
conflating several unrelated things. So breaking it apart would make
sense for the same reason we have "strlen" and "strchr" as separate
functions and not "string_ops(s, WANT_STRLEN)": the function is our
basic unit of reusable work.

But from your description:

> caller1() {
>   setup_for_get();
>   get_thing_one();
> }

I didn't realize the commonality was actual setup; I thought it was "do
a list of 5 things, but the first 2 are common, and then callers may
want to some mix of the other 3".

So I think your original patch is fine.

-Peff

Re: [PATCH 04/13] remote: make get_remote_ref_states() always populate states.tracked

[Junio C Hamano]

Does this series _semantically_ depend on topics other than your other
"set HEAD" topic that is still not in 'master'?

I see you made it cleanly apply on top of 'pu', but frankly, I really hate
to see a series based on 'pu' as a matter of principle and discipline.

Other topics in 'pu' may not be ready for a long time, and you do not want
this series to be taken hostage to them.  Having to fork your topic off of
many other topics not even in 'next' means it would be more cumbersome for
me to handle your series when any of the other topics that you base your
work on need to be discarded or replaced.

In other words, you make your series unworkable with if you introduce too
many dependencies.

I've rebased it on top of Peff's jk/head-lookup topic (which in turn
depends on your js/remote-set-head topic), but I suspect that you would
want this series fork from and only depend on js/remote-set-head topic.
In any case, please check the result after I push the results out to make
sure that I did not make any silly mistakes while rebasing the series.

I also suspect that you would want to reroll the entire series anyway; see
separate comments on 10/13 and 11/13.

Thanks.

Re: [PATCH 10/13] remote.c: make match_refs() copy src ref before assigning to peer_ref

[Junio C Hamano]

If this issue was present before you started touching the affected code,
it should come much earlier in the series than [10/13].  If on the other
hand this was introduced by earlier one in the series, the patch that
introduced the bug should be corrected in place in the sequence without
this patch.

Re: [PATCH 11/13] remote.c: don't short-circuit match_refs() when error in match_explicit_refs()

[Junio C Hamano]

The same comment as 10/13 applies here.

Re: [PATCH 10/13] remote.c: make match_refs() copy src ref before assigning to peer_ref

[Jay Soffian]

On Mon, Feb 23, 2009 at 8:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> If this issue was present before you started touching the affected code,
> it should come much earlier in the series than [10/13].  If on the other
> hand this was introduced by earlier one in the series, the patch that
> introduced the bug should be corrected in place in the sequence without
> this patch.

This issue existed before, but the other callers of match_heads() only
call it once before the process terminates, and don't bother to free
what it returns. So those callers didn't notice. This topic introduces
the first caller that calls match_heads() repeatedly, so it frees the
returned ref list after it has copied out what it needs.

The change is independent and can be moved anywhere in the series, or
even introduced as a patch independent of the series itself. Which
would you prefer? (Actually, would you mind just applying it how you
see fit?)

j.

Re: [PATCH 11/13] remote.c: don't short-circuit match_refs() when error in match_explicit_refs()

[Jay Soffian]

On Mon, Feb 23, 2009 at 8:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> The same comment as 10/13 applies here.

As does the reply. :-)

j.

Re: [PATCH 04/13] remote: make get_remote_ref_states() always populate states.tracked

[Jay Soffian]

On Mon, Feb 23, 2009 at 8:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Does this series _semantically_ depend on topics other than your other
> "set HEAD" topic that is still not in 'master'?

No, but I wanted Jeff's changes. I have checked that I can apply the
"set HEAD" series, then Jeff's changes, then this latest series all to
next w/o conflict.

I've not tried it w/o Jeff's changes in the middle.

> I see you made it cleanly apply on top of 'pu', but frankly, I really hate
> to see a series based on 'pu' as a matter of principle and discipline.
>
> Other topics in 'pu' may not be ready for a long time, and you do not want
> this series to be taken hostage to them.  Having to fork your topic off of
> many other topics not even in 'next' means it would be more cumbersome for
> me to handle your series when any of the other topics that you base your
> work on need to be discarded or replaced.

Understood. I thought this situation was a little different though due
to Jeff's changes in the middle which really build on my set HEAD
topic.

> In other words, you make your series unworkable with if you introduce too
> many dependencies.
>
> I've rebased it on top of Peff's jk/head-lookup topic (which in turn
> depends on your js/remote-set-head topic), but I suspect that you would
> want this series fork from and only depend on js/remote-set-head topic.
> In any case, please check the result after I push the results out to make
> sure that I did not make any silly mistakes while rebasing the series.

Will do, thanks.

> I also suspect that you would want to reroll the entire series anyway; see
> separate comments on 10/13 and 11/13.

I've replied to those. Would you mind applying 10/13 and 11/13
independently of this series? This topic depends on them since
otherwise there will be double-frees, but they can go anywhere before
the topic.

j.

Re: [PATCH 04/13] remote: make get_remote_ref_states() always populate states.tracked

[Jay Soffian]

On Mon, Feb 23, 2009 at 10:09 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> No, but I wanted Jeff's changes. I have checked that I can apply the
> "set HEAD" series, then Jeff's changes, then this latest series all to
> next w/o conflict.

Put another way, IMO, the set HEAD series, Jeff's changes, and this
series might very well be a single topic.

>> In other words, you make your series unworkable with if you introduce too
>> many dependencies.

For future reference, should I build on top of master or next?

j.

Re: [PATCH 10/13] remote.c: make match_refs() copy src ref before assigning to peer_ref

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> This issue existed before, but the other callers of match_heads() only
> call it once before the process terminates, and don't bother to free
> what it returns. So those callers didn't notice. This topic introduces
> the first caller that calls match_heads() repeatedly, so it frees the
> returned ref list after it has copied out what it needs.
>
> The change is independent and can be moved anywhere in the series, or
> even introduced as a patch independent of the series itself.

Thanks for explanation.

If I am reading the above correctly, a sequence that does not have 10/13
before the patch that makes match_heads() called repeatedly would break
bisection, so it cannot be "can be moved anywhere".

And it sounds like keeping this as a separate patch would make more
sense.  Your proposed commit log message for the patch would contain the
first paragraph I quoted above, I'd think.

[PATCH] remote.c: make match_refs() copy src ref before assigning to peer_ref

[Jay Soffian]

In some instances, match_refs() sets the peer_ref field of refs in the
dst list such that it points to a ref in the src list. Were a caller to
free both the src and dst lists, this would cause a double-free as
free_refs() frees the peer_ref.

A double-free would also occur freeing just the dst list under the
following configuration:

  push = refs/heads/master:refs/heads/backup
  push = refs/heads/master:refs/heads/master

Since such a configuration would cause two refs in the dst list to have
the same peer_ref.

This patch corrects the problem by copying the ref when it has been
plucked from the src list.

This issue has gone undetected as the existing callers of match_heads()
call it only once and then terminate, w/o ever bothering to free the src
or dst lists.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 remote.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index d7079c6..98575be 100644
--- a/remote.c
+++ b/remote.c
@@ -927,6 +927,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 			  struct refspec *rs)
 {
 	struct ref *matched_src, *matched_dst;
+	int copy_src;
 
 	const char *dst_value = rs->dst;
 	char *dst_guess;
@@ -937,6 +938,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 	matched_src = matched_dst = NULL;
 	switch (count_refspec_match(rs->src, src, &matched_src)) {
 	case 1:
+		copy_src = 1;
 		break;
 	case 0:
 		/* The source could be in the get_sha1() format
@@ -946,6 +948,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 		matched_src = try_explicit_object_name(rs->src);
 		if (!matched_src)
 			return error("src refspec %s does not match any.", rs->src);
+		copy_src = 0;
 		break;
 	default:
 		return error("src refspec %s matches more than one.", rs->src);
@@ -991,7 +994,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 		return error("dst ref %s receives from more than one src.",
 		      matched_dst->name);
 	else {
-		matched_dst->peer_ref = matched_src;
+		matched_dst->peer_ref = copy_src ? copy_ref(matched_src) : matched_src;
 		matched_dst->force = rs->force;
 	}
 	return 0;
@@ -1099,7 +1102,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
 			dst_peer = make_linked_ref(dst_name, dst_tail);
 			hashcpy(dst_peer->new_sha1, src->new_sha1);
 		}
-		dst_peer->peer_ref = src;
+		dst_peer->peer_ref = copy_ref(src);
 		dst_peer->force = pat->force;
 	free_name:
 		free(dst_name);
-- 
1.6.2.rc1.291.g83eb

Re: [PATCH] remote.c: make match_refs() copy src ref before assigning to peer_ref

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> This issue has gone undetected as the existing callers of match_heads()
> call it only once and then terminate, w/o ever bothering to free the src
> or dst lists.

Thanks.

It sounds more like existing function and usage was alright but your new
caller made it into an issue.

So I'd queue this as the first patch in the 13-patch series, dropping 10/13?
What should happen to 11/13?

Re: [PATCH] remote.c: make match_refs() copy src ref before assigning to peer_ref

[Jay Soffian]

On Tue, Feb 24, 2009 at 1:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> This issue has gone undetected as the existing callers of match_heads()
>> call it only once and then terminate, w/o ever bothering to free the src
>> or dst lists.
>
> Thanks.
>
> It sounds more like existing function and usage was alright but your new
> caller made it into an issue.

Well, maybe. The existing function alters the passed in dst argument
(a ref list) in two ways:

1) It potentially adds new refs to the tail of the list.
2) For the existing and new refs, it populates their peer_ref field.
When it does this, it sometimes just points it at an existing ref in
the src list, but sometimes it allocates a new ref and points to that.

So there is no sane way to free the result. As there is no indication
that the caller shouldn't be able to free the src and dst lists after
it is done, it was IMO a latent bug.

Initially I tried to fix this by modifying free_refs() to make two
passes over the list passed to it, once to mark each peer_ref, then a
second time to clear only those peer_refs it had not already seen. I
did this because when I first saw the double free I was only freeing
the dst list. It took me a while to realize the cause was two refs in
the dst list having the same peer_ref.

But then I realized my caller needed to clear the src list as well,
and saw that the problem was really in match_heads(), not in free'ing
the result.

> So I'd queue this as the first patch in the 13-patch series, dropping 10/13?
> What should happen to 11/13?

I sent you a message off-list you may not have read yet. I'm
re-rolling the entire js/remote-set-head for you off of the current
master to get rid of any dependencies on pu, with jk/head-lookup on
the end of the series instead of in the middle. I needed to rebase off
master since I have a dependency on cfa1ee6 which is now in master,
but wasn't when you spawned js/remote-set-head initially.

j.

Re: [PATCH] remote.c: make match_refs() copy src ref before assigning to peer_ref

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> So there is no sane way to free the result.

Yeah, I am not disagreeing that this is a good change to make the function
easier to use.  There is no question about it.

I just found your description a bit unfair to the original author of the
codepath who was probably fully aware of this.

> I sent you a message off-list you may not have read yet.

Yes I have, but I was deep into tonight's integration cycle (I earlier
asked you to check the rebased result when I push it out --- that is what
I've been preparing).  I am almost done, and it will be pushed out
shortly, but it won't have your re-roll.  That will be after 1.6.2-rc2.