[PATCH 0/4] Improve "remote show" output

[PATCH 0/4] Improve "remote show" output

[Jay Soffian]

This patch series was inspired by
http://thread.gmane.org/gmane.comp.version-control.git/109301/

I've based it on pu.

Jay Soffian (4):
  remote: minor code cleanups in preparation for changing "show" output
  remote: move append_ref_to_tracked_list to get rid of prototype
  string-list: add for_each_string_list()
  remote: new show output style

 builtin-remote.c  |  232 +++++++++++++++++++++++++++++++++++------------------
 string-list.c     |   10 +++
 string-list.h     |    5 +
 t/t5505-remote.sh |   32 ++++----
 4 files changed, 184 insertions(+), 95 deletions(-)

[PATCH 1/4] remote: minor code cleanups in preparation for changing "show" output

[Jay Soffian]

* Rename char *remote to remote_name to distinguish it clearly from the
  struct remote pointer, also named remote.

* There is no need to call sort_string_list() on branch_list, as its
  items are added to it via string_list_insert() which maintains its
  order.

* Sort states->new and states->tracked so that we can use binary search
  string_list_has_string() on them instead of less efficient linear
  unsorted_string_list_has_string. This alters the output of "remote
  show" slightly, so update the tests to match.

* Simplify get_ref_states(); nothing is using the pointer to states that
  is being copied into util.

* Have get_remote_ref_states() populate states->tracked even when it is
  not querying the remote so that this need not be done by the caller.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
I added a function prototype for get_remote_ref_states() so I didn't
need to move its location in this diff, which kept the diff cleaner.
The next patch then moves the function and gets rid of the prototype.

 builtin-remote.c  |   45 ++++++++++++++++++++++-----------------------
 t/t5505-remote.sh |    2 +-
 2 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index d6958d4..ea5e808 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -23,6 +23,9 @@ static int verbose;
 
 static int show_all(void);
 
+static int append_ref_to_tracked_list(const char *refname,
+	const unsigned char *sha1, int flags, void *cb_data);
+
 static inline int postfixcmp(const char *string, const char *postfix)
 {
 	int len1 = strlen(string), len2 = strlen(postfix);
@@ -144,7 +147,7 @@ static int add(int argc, const char **argv)
 }
 
 struct branch_info {
-	char *remote;
+	char *remote_name;
 	struct string_list merge;
 };
 
@@ -183,9 +186,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 +210,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 {
@@ -228,10 +230,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);
@@ -251,21 +251,16 @@ 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);
 
+	sort_string_list(&states->new);
+	sort_string_list(&states->tracked);
 	for_each_ref(handle_one_branch, states);
 	sort_string_list(&states->stale);
 
@@ -490,7 +485,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 +595,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);
@@ -685,6 +680,9 @@ 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;
@@ -696,6 +694,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))
@@ -743,7 +744,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   ",
@@ -762,8 +763,6 @@ static int show(int argc, const char **argv)
 				"prune')", &states.stale, "");
 		}
 
-		if (no_query)
-			for_each_ref(append_ref_to_tracked_list, &states);
 		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.218.g1b4fab

[PATCH 2/4] remote: move append_ref_to_tracked_list to get rid of prototype

[Jay Soffian]

Relocate the append_ref_to_tracked_list() function above all its callers
so that we can get rid of the prototype.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
Doing this as a separate patch makes the prior patch easier to review,
but I think it could be squashed when you apply the series.

 builtin-remote.c |   37 +++++++++++++++++--------------------
 1 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index ea5e808..b61f754 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -23,9 +23,6 @@ static int verbose;
 
 static int show_all(void);
 
-static int append_ref_to_tracked_list(const char *refname,
-	const unsigned char *sha1, int flags, void *cb_data);
-
 static inline int postfixcmp(const char *string, const char *postfix)
 {
 	int len1 = strlen(string), len2 = strlen(postfix);
@@ -659,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)
@@ -688,23 +702,6 @@ static int get_remote_ref_states(const char *name,
 	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;
-- 
1.6.2.rc1.218.g1b4fab

[PATCH 3/4] string-list: add for_each_string_list()

[Jay Soffian]

Give string-list a convenience function for iterating over each of the
items in a string_list.

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.218.g1b4fab

[PATCH 4/4] 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.

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 w/remote foo, bar, baz, 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 w/remote foo, bar, baz, frotz

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
Dscho, using --patience made this more readable. So thank you for
adding that.

 builtin-remote.c  |  172 ++++++++++++++++++++++++++++++++++++++--------------
 t/t5505-remote.sh |   32 +++++-----
 2 files changed, 141 insertions(+), 63 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index b61f754..6f0e40f 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.")) {
+		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,
@@ -634,20 +641,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,10 +691,93 @@ 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 maxwidth;
+};
+
+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->maxwidth)
+		info->maxwidth = 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->maxwidth, name);
+		printf(fmt, arg);
+		printf("\n");
+	} else
+		printf("    %s\n", name);
+
+	return 0;
+}
+
+int add_local_to_show_info(struct string_list_item *branch, void *cb_data)
+{
+	struct show_info *show_info = cb_data;
+	struct ref_states *states = show_info->states;
+	struct string_list_item *item;
+	struct branch_info *branch_info = branch->util;
+	struct strbuf buf = STRBUF_INIT;
+	int i;
+
+	if (!branch_info->merge.nr ||
+	    strcmp(states->remote->name, branch_info->remote_name))
+		return 0;
+	if ((i = strlen(branch->string)) > show_info->maxwidth)
+		show_info->maxwidth = i;
+
+	item = string_list_insert(branch->string, show_info->list);
+
+	if (branch_info->rebase)
+		strbuf_addf(&buf, "rebases onto remote");
+	else
+		strbuf_addf(&buf, "merges w/remote");
+
+	for (i = 0; i < branch_info->merge.nr; i++)
+		strbuf_addf(&buf, " %s,", branch_info->merge.items[i].string);
+
+	if (buf.len)
+		buf.buf[buf.len - 1] = '\0'; /* trailing comma */
+
+	item->util = strbuf_detach(&buf, NULL);
+	return 0;
+}
+
+int show_local_info_item(struct string_list_item *item, void *cb_data)
+{
+	struct show_info *info = cb_data;
+	printf("    %-*s %s\n", info->maxwidth, item->string, (char *) item->util);
+	return 0;
+}
+
 static int show(int argc, const char **argv)
 {
 	int no_query = 0, result = 0;
@@ -711,6 +787,10 @@ 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;
+	info.states = &states;
+	info.list = &info_list;
 
 	argc = parse_options(argc, argv, options, builtin_remote_usage, 0);
 
@@ -730,40 +810,38 @@ static int show(int argc, const char **argv)
 			printf("  HEAD branch: (not queried)\n");
 		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");
+		else {
+			printf("  HEAD branch%s: ", states.heads.nr > 1 ? "es" : "");
+			for (i = 0; i < states.heads.nr; i++)
+				printf("%s%s", states.heads.items[i].string,
+					i < states.heads.nr - 1 ? ", " : "\n");
 		}
 
-		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 */
+		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);
+		info.maxwidth = 0;
 
-		show_list("  Tracked remote branch%s", &states.tracked, "");
+		/* git pull info */
+		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, 1);
+		info.maxwidth = 0;
 
+		/* git push info */
 		if (states.remote->push_refspec_nr) {
-			printf("  Local branch%s pushed with 'git push'\n",
+			printf("  Local branch%s configured for 'git push':\n",
 				states.remote->push_refspec_nr > 1 ?
 					"es" : "");
 			for (i = 0; i < states.remote->push_refspec_nr; i++) {
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index fdc4a29..d3dea3a 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,21 +137,18 @@ 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
-  Local branches pushed with 'git push'
+  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 w/remote master
+  Local branches configured for 'git push':
     master:upstream
     +refs/tags/lastbackup
 * remote two
   URL: ../two
-  HEAD branches:
-    another
-    master
+  HEAD branches: another, master
 EOF
 
 test_expect_success 'show' '
@@ -159,8 +156,10 @@ test_expect_success 'show' '
 	 git config --add remote.origin.fetch \
 		refs/heads/master:refs/heads/upstream &&
 	 git fetch &&
+	 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 &&
 	 (cd ../one &&
 	  echo 1 > file &&
 	  test_tick &&
@@ -170,6 +169,7 @@ test_expect_success 'show' '
 	 git config --add remote.origin.push \
 		+refs/tags/lastbackup &&
 	 git remote show origin two > output &&
+	 git branch -d foo &&
 	 test_cmp expect output)
 '
 
@@ -177,12 +177,12 @@ 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 branches pushed with 'git push'
+  Local branch configured for 'git pull':
+    master merges w/remote master
+  Local branches configured for 'git push':
     master:upstream
     +refs/tags/lastbackup
 EOF
-- 
1.6.2.rc1.218.g1b4fab

Re: [PATCH 4/4] remote: new show output style

[Marc Branchaud]

Jay, thanks a ton!  I'd been poking at this myself off and on over the last week -- I'm very glad to see you've done the work!

A couple of suggestions on the new format:

Jay Soffian wrote:
> 
> @@ -137,21 +137,18 @@ 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
> -  Local branches pushed with 'git push'
> +  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 w/remote master
> +  Local branches configured for 'git push':
>      master:upstream
>      +refs/tags/lastbackup
>  * remote two
>    URL: ../two
> -  HEAD branches:
> -    another
> -    master
> +  HEAD branches: another, master
>  EOF

First, a nit: I don't know if the "w/remote" notation makes sense to non-English speakers.  I also like the alignment achieved by "merges with remote " (note the trailing space).

Second, I think it would be good to also change the format of the 'git push' list, for consistency:

	Local branches configured for 'git push':
	  master               fast-forwards remote upstream
	  refs/tags/lastbackup updates remote       refs/tags/lastbackup

Though I'm not really happy using "updates" when + is specified in the push refspec.  What, precisely, is a "non-fast forward update" anyway?  Is it essentially a rebase?  If so, maybe "rebases onto remote " would be better (again with a trailing space to get nice alignment).

Thanks again!

		M.

Re: [PATCH 4/4] remote: new show output style

[Sverre Rabbelier]

On Thu, Feb 19, 2009 at 17:03, Marc Branchaud <marcnarc@xiplink.com> wrote:
> Though I'm not really happy using "updates" when + is specified in the push
> refspec.  What, precisely, is a "non-fast forward update" anyway?  Is it
> essentially a rebase?  If so, maybe "rebases onto remote " would be better
> (again with a trailing space to get nice alignment).

A non-fast forward update in terms of pushing means you overwrite
whatever the remote currently has set as that branch's head. This can
be desirable in a private repository or branch, but is usually not
desired on shared branches.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 4/4] remote: new show output style

[Rostislav Svoboda]

On Thu, Feb 19, 2009 at 17:03, Marc Branchaud <marcnarc@xiplink.com> wrote:

> First, a nit: I don't know if the "w/remote" notation makes sense to
> non-English speakers.

No it doesn't :)

Bost

Re: [PATCH 4/4] remote: new show output style

[Marc Branchaud]

Sverre Rabbelier wrote:
> 
> A non-fast forward update in terms of pushing means you overwrite
> whatever the remote currently has set as that branch's head. This can
> be desirable in a private repository or branch, but is usually not
> desired on shared branches.

Let me see if I understand what you're saying.  If my local repo has

	o--o--o--A--B  <-- origin/thing
	       \
	        X--Y--Z  <-- mystuff

and I do 'git push origin +mystuff:thing', does the origin end up with

	        A--B  <-- (branch with no symbolic reference)
	       /
	o--o--o--X--Y--Z  <-- origin/thing

So commits A and B are basically left dangling?

If that's the case, then I'd say "replaces" or "overwrites" is the right word to use in the 'remote show' output.  But more importantly, I think the 'git push' man page needs to explain this!

		M.

Re: [PATCH 4/4] remote: new show output style

[Sverre Rabbelier]

On Thu, Feb 19, 2009 at 17:31, Marc Branchaud <marcnarc@xiplink.com> wrote:
> So commits A and B are basically left dangling?

Correct, that's why it's not desirable to do so when it's not your
private repo/branch ;).

> If that's the case, then I'd say "replaces" or "overwrites" is the right
> word to use in the 'remote show' output.

Yes, that would be a better wording.

> But more importantly, I think the 'git push' man page needs to explain this!

>From my cursory glance the term 'non-fast forward' is indeed not
explained in the man-page, feel like writing a patch? ;)

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 4/4] remote: new show output style

[Jay Soffian]

On Thu, Feb 19, 2009 at 11:03 AM, Marc Branchaud <marcnarc@xiplink.com> wrote:
> First, a nit: I don't know if the "w/remote" notation makes sense to
> non-English speakers.  I also like the alignment achieved by "merges with
> remote " (note the trailing space).

I tried out a few different option and none was very satisfactory to me.

1)
    master            rebases onto remote master
    another-branch    merges with remote  next
    some-other-branch rebases onto remote master

Here, the unaligned "with" and "onto" is ugly.

2)

    master            rebases onto remote master
    another-branch    merges  with remote next
    some-other-branch rebases onto remote master

This looks better to me. However, if none of your branches rebase,
then the extra space looks like it is a mistake. e.g.:

    master            merges  with remote master
    another-branch    merges  with remote next
    some-other-branch merges  with remote master

I could add code to detect whether all the branches merge and then not
output the space, but, sigh. And I couldn't think of any other
combination of words that had the same character spacing.

So that's how I ended up with "merges w/remote". This is also slightly
less wide. I always try to have output fit into 80 columns (how
quaint, I know) and a merging branch might have multiple upstreams.
e.g.

   another-branch merges w/foo, bar, baz

IOW, the output in the patch wasn't arbitrary. I did think about it
quite a bit. Which isn't to say it's right, just it's the best I came
up with.

I'm somewhat confused by "w/remote" making sense to non-English
speakers as it's English output.

> Second, I think it would be good to also change the format of the 'git push'
> list, for consistency:

I left that out on purpose. The only folks with push refspecs put them
their manually, and the raw refspec is clearer and more concise than
any English words can convey. That was my reasoning anyway.

Thanks for the feedback.

j.

Re: [PATCH 4/4] remote: new show output style

[Jay Soffian]

On Thu, Feb 19, 2009 at 12:57 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> I tried out a few different option and none was very satisfactory to me.
>
> 1)
>    master            rebases onto remote master
>    another-branch    merges with remote  next
>    some-other-branch rebases onto remote master
>
> Here, the unaligned "with" and "onto" is ugly.
>
> 2)
>
>    master            rebases onto remote master
>    another-branch    merges  with remote next
>    some-other-branch rebases onto remote master

I also had a version that used the word "upstream" instead of remote,
and another that put the actual remote name in place. e.g.

 master merges with upstream master
 master merges with origin master  (assuming remote is "origin" of course.)

j.

Re: [PATCH 4/4] remote: new show output style

[Julian Phillips]

On Thu, 19 Feb 2009, Jay Soffian wrote:

> On Thu, Feb 19, 2009 at 11:03 AM, Marc Branchaud <marcnarc@xiplink.com> wrote:
>> First, a nit: I don't know if the "w/remote" notation makes sense to
>> non-English speakers.  I also like the alignment achieved by "merges with
>> remote " (note the trailing space).
>
> I tried out a few different option and none was very satisfactory to me.
>
> 1)
>    master            rebases onto remote master
>    another-branch    merges with remote  next
>    some-other-branch rebases onto remote master
>
> Here, the unaligned "with" and "onto" is ugly.
>
> 2)
>
>    master            rebases onto remote master
>    another-branch    merges  with remote next
>    some-other-branch rebases onto remote master
>
> This looks better to me. However, if none of your branches rebase,
> then the extra space looks like it is a mistake. e.g.:
>
>    master            merges  with remote master
>    another-branch    merges  with remote next
>    some-other-branch merges  with remote master
>
> I could add code to detect whether all the branches merge and then not
> output the space, but, sigh. And I couldn't think of any other
> combination of words that had the same character spacing.

or

  3)

     master            rebases onto remote master
     another-branch     merges with remote next
     some-other-branch rebases onto remote master

perhaps, which doesn't look odd without the rebase lines?

> So that's how I ended up with "merges w/remote". This is also slightly
> less wide. I always try to have output fit into 80 columns (how
> quaint, I know) and a merging branch might have multiple upstreams.
> e.g.
>
>   another-branch merges w/foo, bar, baz

or

    another-branch merges with foo
                      and with bar
                      and with baz

perhaps?

> IOW, the output in the patch wasn't arbitrary. I did think about it
> quite a bit. Which isn't to say it's right, just it's the best I came
> up with.
>
> I'm somewhat confused by "w/remote" making sense to non-English
> speakers as it's English output.

But generally the output is not as idomatic as w/foo.  If English is a 
non-native language then "with foo" is going to be much clearer - and if 
nothing else, much easier to lookup in a dictionary.

>> Second, I think it would be good to also change the format of the 'git push'
>> list, for consistency:
>
> I left that out on purpose. The only folks with push refspecs put them
> their manually, and the raw refspec is clearer and more concise than
> any English words can convey. That was my reasoning anyway.
>
> Thanks for the feedback.
>
> j.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Julian

  ---
"You're just the sort of person I imagined marrying, when I was little...
except, y'know, not green... and without all the patches of fungus."
 		-- Swamp Thing

Re: [PATCH 4/4] remote: new show output style

[Johannes Sixt]

Jay Soffian schrieb:
>  * remote two
>    URL: ../two
> -  HEAD branches:
> -    another
> -    master
> +  HEAD branches: another, master

Any reason why this list of branches is now horizontal instead of 
vertical? I must admit that I don't know what is meant by "HEAD 
branches". Can this list grow large?

I'm asking because I changed horizontal branch lists of 'remote show' 
output to vertical lists some time ago because I found that vertical 
lists are much easier to read if they grow large.

-- Hannes

Re: [PATCH 4/4] remote: new show output style

[Jay Soffian]

On Thu, Feb 19, 2009 at 2:29 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Jay Soffian schrieb:
>>
>>  * remote two
>>   URL: ../two
>> -  HEAD branches:
>> -    another
>> -    master
>> +  HEAD branches: another, master
>
> Any reason why this list of branches is now horizontal instead of vertical?

Not a good one, but I thought it fit better w/how an octopus branch
listing was displayed.

> I must admit that I don't know what is meant by "HEAD branches".
> Can this list grow large?

The git:// protocol does not currently have a way to determine what a
remote sym ref is pointed to, so it guesses by matching SHA1s. In
theory this list is unbounded, in practice I don't think it is likely
to be large.

I like the suggestion above about how to display the octopus merge
tho, so I will put this back how it was.

j.

Re: [PATCH 1/4] remote: minor code cleanups in preparation for changing "show" output

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> * Rename char *remote to remote_name to distinguish it clearly from the
>   struct remote pointer, also named remote.
>
> * There is no need to call sort_string_list() on branch_list, as its
>   items are added to it via string_list_insert() which maintains its
>   order.
>
> * Sort states->new and states->tracked so that we can use binary search
>   string_list_has_string() on them instead of less efficient linear
>   unsorted_string_list_has_string. This alters the output of "remote
>   show" slightly, so update the tests to match.
>
> * Simplify get_ref_states(); nothing is using the pointer to states that
>   is being copied into util.
>
> * Have get_remote_ref_states() populate states->tracked even when it is
>   not querying the remote so that this need not be done by the caller.

This does too many things in a single patch.

Ideally this would have been four patches for reviewability:

 - one "trivial and obviously correct bits" (s/remote/remote_name/ and
   removal of sort_string_list(&branch_list)) patch;

 - the change for states->{new,tracked}, should stand on its own; I think
   the reordering of the output should be described much better and
   defended independently.  "Earlier it was sorted by this order, which
   did not make sense for such and such reasons; this fixes the logic to
   sort the list by the name of the tracked branch, which makes it easier
   to read", or something like that.

 - change to the states->tracked population rule; and

 - get_ref_states() to lose the util bit.

It probably is Ok to squash the last two, though.

Re: [PATCH 1/4] remote: minor code cleanups in preparation for changing "show" output

[Jay Soffian]

On Fri, Feb 20, 2009 at 2:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> This does too many things in a single patch.
>
> Ideally this would have been four patches for reviewability:

Okay. I'm re-doing 4/4 anyway, so I'll just re-do the series.

j.

Re: [PATCH 4/4] remote: new show output style

[Marc Branchaud]

Jay Soffian wrote:
> 
> 
>     master            merges  with remote master
>     another-branch    merges  with remote next
>     some-other-branch merges  with remote master
> 
> I could add code to detect whether all the branches merge and then not
> output the space, but, sigh. And I couldn't think of any other
> combination of words that had the same character spacing.

All valid points.  But, dang it, I really think intelligently adding that extra space the right thing to do.

It wouldn't be all that tedious -- you could detect the need for the extra space in config_read_branches() the first time type is set to REBASE, no?

> I'm somewhat confused by "w/remote" making sense to non-English
> speakers as it's English output.

Quite right.  Julian made the point better than I: it's fairly idiomatic.  I think it's reasonable to assume that many users of the English version of git aren't native English speakers.

Plus it throws the alignment with "rebases" lines way off...

>> Second, I think it would be good to also change the format of the 'git push'
>> list, for consistency:
> 
> I left that out on purpose. The only folks with push refspecs put them
> their manually, and the raw refspec is clearer and more concise than
> any English words can convey. That was my reasoning anyway.

Fair enough.

Thanks again!

		M.

Re: [PATCH 4/4] remote: new show output style

[Jay Soffian]

On Fri, Feb 20, 2009 at 5:34 PM, Marc Branchaud <marcnarc@xiplink.com> wrote:
> All valid points.  But, dang it, I really think intelligently adding that
> extra space the right thing to do.

Already coded. :-)

> It wouldn't be all that tedious -- you could detect the need for the extra
> space in config_read_branches() the first time type is set to REBASE, no?

What is that about everything being easy for the man who doesn't have
to do it? :-)

(It was no big deal, I just needed to rework a few things cause I'd
painted myself into a corner.)

>> I left that out on purpose. The only folks with push refspecs put them
>> their manually, and the raw refspec is clearer and more concise than
>> any English words can convey. That was my reasoning anyway.
>
> Fair enough.

Yeah, I'm fixing this too by properly expanded out the refspecs the
same way that it's done by {builtin-send-pack, http-push}.c.

Hopefully I can have a patch out later today.

j.