Re: [PATCH 5/5] builtin-remote: Make "remote -v" display push urls

Michael J Gruber [off-list ref] writes:

Junio C Hamano venit, vidit, dixit 09.06.2009 18:25:

quoted

Michael J Gruber [off-list ref] writes:

quoted

Example with "mjg" having 1 url and 1 pushurl, "origin" having 3 urls,
sb having 1 url:

mjg     git://repo.or.cz/git/mjg.git (fetch)
mjg     repoor:/srv/git/git/mjg.git (push)
origin  git://repo.or.cz/git.git (fetch)
origin  git://repo.or.cz/git.git (push)
origin  git://git2.kernel.org/pub/scm/git/git.git (push)
origin  git://repo.or.cz/alt-git.git (push)
sb      git://repo.or.cz/git/sbeyer.git (fetch)
sb      git://repo.or.cz/git/sbeyer.git (push)

The readers will get distracted, saying "eh, git:// can be used for push?"
(and the answer is "yes, sometimes, but not for repo.or.cz") even though
that is not the point of these illustrations.  For these examles, I think
it is better to use "repo.or.cz:foo.git" style, instead of "git://".

Uhm, isn't host:foo.git equivalent to ssh://host/foo.git?

The primary point is git:// is usually considered read-only and not for
push.  I personally am more used to host:repo and that is why I wrote it
that way; besides, the second line in your example already uses that
notation, not the ssh:// one.

Teach builtin remote to show push urls also when asked to
"show" a specific remote.

This improves upon the standard display mode: multiple specified "url"s
mean that the first one is for fetching, all are used for pushing. We
make this clearer now by displaying the first one prefixed with "Fetch
URL", and all "url"s (or, if present, all "pushurl"s) prefixed with
"Push  URL".

Example with "one" having one url, "two" two urls, "three" one url and
one pushurl (URL part only):

* remote one
  Fetch URL: hostone.com:/somepath/repoone.git
  Push  URL: hostone.com:/somepath/repoone.git
* remote two
  Fetch URL: hosttwo.com:/somepath/repotwo.git
  Push  URL: hosttwo.com:/somepath/repotwo.git
  Push  URL: hosttwobackup.com:/somewheresafe/repotwo.git
* remote three
  Fetch URL: http://hostthree.com/otherpath/repothree.git
  Push  URL: hostthree.com:/pathforpushes/repothree.git

Also, adjust t5505 accordingly and make it test for the new output.

Signed-off-by: Michael J Gruber <redacted>
---
 builtin-remote.c  |   20 +++++++++++++++-----
 t/t5505-remote.sh |   10 +++++++---
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index dfc0b9e..b350b18 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c

@@ -999,15 +999,25 @@ static int show(int argc, const char **argv)
 	info.list = &info_list;
 	for (; argc; argc--, argv++) {
 		int i;
+		const char **url;
+		int url_nr;
 
 		get_remote_ref_states(*argv, &states, query_flag);
 
 		printf("* remote %s\n", *argv);
-		if (states.remote->url_nr) {
-			for (i=0; i < states.remote->url_nr; i++)
-				printf("  URL: %s\n", states.remote->url[i]);
-		} else
-			printf("  URL: %s\n", "(no URL)");
+		printf("  Fetch URL: %s\n", states.remote->url_nr > 0 ?
+			states.remote->url[0] : "(no URL)");
+		if (states.remote->pushurl_nr) {
+			url = states.remote->pushurl;
+			url_nr = states.remote->pushurl_nr;
+		} else {
+			url = states.remote->url;
+			url_nr = states.remote->url_nr;
+		}
+		for (i=0; i < url_nr; i++)
+			printf("  Push  URL: %s\n", url[i]);
+		if (!i)
+			printf("  Push  URL: %s\n", "(no URL)");
 		if (no_query)
 			printf("  HEAD branch: (not queried)\n");
 		else if (!states.heads.nr)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index e70246b..852ccb5 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh

@@ -135,7 +135,8 @@ EOF
 
 cat > test/expect << EOF
 * remote origin
-  URL: $(pwd)/one
+  Fetch URL: $(pwd)/one
+  Push  URL: $(pwd)/one
   HEAD branch: master
   Remote branches:
     master new (next fetch will store in remotes/origin)

@@ -151,7 +152,8 @@ cat > test/expect << EOF
     master pushes to master   (local out of date)
     master pushes to upstream (create)
 * remote two
-  URL: ../two
+  Fetch URL: ../two
+  Push  URL: ../three
   HEAD branch (remote HEAD is ambiguous, may be one of the following):
     another
     master

@@ -173,6 +175,7 @@ test_expect_success 'show' '
 	 git branch --track rebase origin/master &&
 	 git branch -d -r origin/master &&
 	 git config --add remote.two.url ../two &&
+	 git config --add remote.two.pushurl ../three &&
 	 git config branch.rebase.rebase true &&
 	 git config branch.octopus.merge "topic-a topic-b topic-c" &&
 	 (cd ../one &&

@@ -191,7 +194,8 @@ test_expect_success 'show' '
 
 cat > test/expect << EOF
 * remote origin
-  URL: $(pwd)/one
+  Fetch URL: $(pwd)/one
+  Push  URL: $(pwd)/one
   HEAD branch: (not queried)
   Remote branches: (status not queried)
     master

-- 
1.6.3.2.367.gf0de

So here are v2 of 4/5 and 5/5: Only the commit messages are revised
according to the discussion. The one of 5/5 uses a better example, and
the one of 4/5 shows the output for the same example now (before it had
no example). Patch-ids unchanged. 1/5, 2/5 and 3/5 are completely
unchanged and, thus, unsent :)

Cheers,
Michael

Michael J Gruber (2):
  builtin-remote: Show push urls as well
  builtin-remote: Make "remote -v" display push urls

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

Currently, "remote -v" simply lists all urls so that one has to remember
that only the first one is used for fetches, and all are used for
pushes.

Change this so that the role of an url is displayed in parentheses, and
also display push urls.

Example with "one" having one url, "two" two urls, "three" one url and
one pushurl:

one     hostone.com:/somepath/repoone.git (fetch)
one     hostone.com:/somepath/repoone.git (push)
three   http://hostthree.com/otherpath/repothree.git (fetch)
three   hostthree.com:/pathforpushes/repothree.git (push)
two     hosttwo.com:/somepath/repotwo.git (fetch)
two     hosttwo.com:/somepath/repotwo.git (push)
two     hosttwobackup.com:/somewheresafe/repotwo.git (push)

Signed-off-by: Michael J Gruber <redacted>
---
 builtin-remote.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index b350b18..80b2536 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c

@@ -1276,14 +1276,31 @@ static int update(int argc, const char **argv)
 static int get_one_entry(struct remote *remote, void *priv)
 {
 	struct string_list *list = priv;
+	const char **url;
+	int i, url_nr;
+	void **utilp;
 
 	if (remote->url_nr > 0) {
-		int i;
-
-		for (i = 0; i < remote->url_nr; i++)
-			string_list_append(remote->name, list)->util = (void *)remote->url[i];
+		utilp = &(string_list_append(remote->name, list)->util);
+		*utilp = malloc(strlen(remote->url[0])+strlen(" (fetch)")+1);
+		strcpy((char *) *utilp, remote->url[0]);
+		strcat((char *) *utilp, " (fetch)");
 	} else
 		string_list_append(remote->name, list)->util = NULL;
+	if (remote->pushurl_nr) {
+		url = remote->pushurl;
+		url_nr = remote->pushurl_nr;
+	} else {
+		url = remote->url;
+		url_nr = remote->url_nr;
+	}
+	for (i = 0; i < url_nr; i++)
+	{
+		utilp = &(string_list_append(remote->name, list)->util);
+		*utilp = malloc(strlen(url[i])+strlen(" (push)")+1);
+		strcpy((char *) *utilp, url[i]);
+		strcat((char *) *utilp, " (push)");
+	}
 
 	return 0;
 }

@@ -1291,6 +1308,7 @@ static int get_one_entry(struct remote *remote, void *priv)
 static int show_all(void)
 {
 	struct string_list list = { NULL, 0, 0 };
+	list.strdup_strings = 1;
 	int result = for_each_remote(get_one_entry, &list);
 
 	if (!result) {

@@ -1309,6 +1327,7 @@ static int show_all(void)
 			}
 		}
 	}
+	string_list_clear(&list, 1);
 	return result;
 }
 

-- 
1.6.3.2.367.gf0de

Thanks, will replace with these two and merge to 'next'.

I'll squash this in to [5/5], which is the same fix-up as I queued the
previous round to 'pu', to avoid decl-after-statement, by the way.

 builtin-remote.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index f377722..3f6f5c2 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c

@@ -1311,8 +1311,10 @@ static int get_one_entry(struct remote *remote, void *priv)
 static int show_all(void)
 {
 	struct string_list list = { NULL, 0, 0 };
+	int result;
+
 	list.strdup_strings = 1;
-	int result = for_each_remote(get_one_entry, &list);
+	result = for_each_remote(get_one_entry, &list);
 
 	if (!result) {
 		int i;