[PATCH 0/5] Allow push and fetch urls to be different

[PATCH 0/5] Allow push and fetch urls to be different

[Michael J Gruber]

This series implements a new config setting remote.${remotename}.pushurl
which allows the urls for fetch and push to be different. 

In http://permalink.gmane.org/gmane.comp.version-control.git/120726 the
feature was suggested, and an initital WIP/RFC was discussed. The patch
1/5 contains exactly that version amended with documentation.

If there is no pushurl then all values of remote.${remotename}.url are
used for pushes just like before. Otherwise, all values of
remote.${remotename}.pushurl are used for pushes. In any case, the
first value of remote.${remotename}.url is used for fetches.

1/5 implements the config setting, makes push obey it and documents it.
2/5 adds tests for the new setting.
3/5 updates the api doc.
4/5 makes "remote show ${remotename}" show the new push URLs, and makes
clear which ones are used for fetch and which ones for push.
5/5 brings the output of "remote -v" more in line with the output of the
above (it was out of sync even before 4/5).

1 through 3 apply on yesterday's next.

4 through 5 require tomorrow's next ;) More specifically, 4 requires
'builtin-remote: Make "remote show" display all urls' which I suggested
for applying to maint in 
http://permalink.gmane.org/gmane.comp.version-control.git/120924
because I think the current output is incomplete.
If that patch is not considered for separate inclusion in maint or
master I would suggest squashing it together with 4/5 of the above.

Michael J Gruber (5):
  Allow push and fetch urls to be different
  t5516: Check pushurl config setting
  technical/api-remote: Describe new struct remote member pushurl
  builtin-remote: Show push urls as well
  builtin-remote: Make "remote -v" display push urls

[PATCH 1/5] Allow push and fetch urls to be different

[Michael J Gruber]

This introduces a config setting remote.$remotename.pushurl which is
used for pushes only. If absent remote.$remotename.url is used for
pushes and fetches as before.
This is useful, for example, in order to do passwordless fetches
(remote update) over the git transport but pushes over ssh.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/config.txt       |    3 +++
 Documentation/urls-remotes.txt |    3 +++
 builtin-push.c                 |   17 +++++++++++++----
 remote.c                       |   14 ++++++++++++++
 remote.h                       |    4 ++++
 5 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3a86d1f..2fecbe3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1319,6 +1319,9 @@ remote.<name>.url::
 	The URL of a remote repository.  See linkgit:git-fetch[1] or
 	linkgit:git-push[1].
 
+remote.<name>.pushurl::
+	The push URL of a remote repository.  See linkgit:git-push[1].
+
 remote.<name>.proxy::
 	For remotes that require curl (http, https and ftp), the URL to
 	the proxy to use for that remote.  Set to the empty string to
diff --git a/Documentation/urls-remotes.txt b/Documentation/urls-remotes.txt
index 41ec777..2a0e7b8 100644
--- a/Documentation/urls-remotes.txt
+++ b/Documentation/urls-remotes.txt
@@ -27,10 +27,13 @@ config file would appear like this:
 ------------
 	[remote "<name>"]
 		url = <url>
+		pushurl = <pushurl>
 		push = <refspec>
 		fetch = <refspec>
 ------------
 
+The `<pushurl>` is used for pushes only. It is optional and defaults
+to `<url>`.
 
 Named file in `$GIT_DIR/remotes`
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/builtin-push.c b/builtin-push.c
index c869974..7be1239 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -117,6 +117,8 @@ static int do_push(const char *repo, int flags)
 {
 	int i, errs;
 	struct remote *remote = remote_get(repo);
+	const char **url;
+	int url_nr;
 
 	if (!remote) {
 		if (repo)
@@ -152,9 +154,16 @@ static int do_push(const char *repo, int flags)
 			setup_default_push_refspecs();
 	}
 	errs = 0;
-	for (i = 0; i < remote->url_nr; i++) {
+	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++) {
 		struct transport *transport =
-			transport_get(remote, remote->url[i]);
+			transport_get(remote, url[i]);
 		int err;
 		if (receivepack)
 			transport_set_option(transport,
@@ -163,14 +172,14 @@ static int do_push(const char *repo, int flags)
 			transport_set_option(transport, TRANS_OPT_THIN, "yes");
 
 		if (flags & TRANSPORT_PUSH_VERBOSE)
-			fprintf(stderr, "Pushing to %s\n", remote->url[i]);
+			fprintf(stderr, "Pushing to %s\n", url[i]);
 		err = transport_push(transport, refspec_nr, refspec, flags);
 		err |= transport_disconnect(transport);
 
 		if (!err)
 			continue;
 
-		error("failed to push some refs to '%s'", remote->url[i]);
+		error("failed to push some refs to '%s'", url[i]);
 		errs++;
 	}
 	return !!errs;
diff --git a/remote.c b/remote.c
index 08a5964..9a0397e 100644
--- a/remote.c
+++ b/remote.c
@@ -106,6 +106,12 @@ static void add_url_alias(struct remote *remote, const char *url)
 	add_url(remote, alias_url(url));
 }
 
+static void add_pushurl(struct remote *remote, const char *pushurl)
+{
+	ALLOC_GROW(remote->pushurl, remote->pushurl_nr + 1, remote->pushurl_alloc);
+	remote->pushurl[remote->pushurl_nr++] = pushurl;
+}
+
 static struct remote *make_remote(const char *name, int len)
 {
 	struct remote *ret;
@@ -379,6 +385,11 @@ static int handle_config(const char *key, const char *value, void *cb)
 		if (git_config_string(&v, key, value))
 			return -1;
 		add_url(remote, v);
+	} else if (!strcmp(subkey, ".pushurl")) {
+		const char *v;
+		if (git_config_string(&v, key, value))
+			return -1;
+		add_pushurl(remote, v);
 	} else if (!strcmp(subkey, ".push")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
@@ -424,6 +435,9 @@ static void alias_all_urls(void)
 		for (j = 0; j < remotes[i]->url_nr; j++) {
 			remotes[i]->url[j] = alias_url(remotes[i]->url[j]);
 		}
+		for (j = 0; j < remotes[i]->pushurl_nr; j++) {
+			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j]);
+		}
 	}
 }
 
diff --git a/remote.h b/remote.h
index 257a555..5db8420 100644
--- a/remote.h
+++ b/remote.h
@@ -15,6 +15,10 @@ struct remote {
 	int url_nr;
 	int url_alloc;
 
+	const char **pushurl;
+	int pushurl_nr;
+	int pushurl_alloc;
+
 	const char **push_refspec;
 	struct refspec *push;
 	int push_refspec_nr;
-- 
1.6.3.2.278.gb6431.dirty

[PATCH 2/5] t5516: Check pushurl config setting

[Michael J Gruber]

Check whether the new remote.${remotename}.pushurl setting is obeyed
and whether it overrides remote.${remotename}.url.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t5516-fetch-push.sh |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 89649e7..2d2633f 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -419,6 +419,19 @@ test_expect_success 'push with config remote.*.push = HEAD' '
 git config --remove-section remote.there
 git config --remove-section branch.master
 
+test_expect_success 'push with config remote.*.pushurl' '
+
+	mk_test heads/master &&
+	git checkout master &&
+	git config remote.there.url test2repo &&
+	git config remote.there.pushurl testrepo &&
+	git push there &&
+	check_push_result $the_commit heads/master
+'
+
+# clean up the cruft left with the previous one
+git config --remove-section remote.there
+
 test_expect_success 'push with dry-run' '
 
 	mk_test heads/master &&
-- 
1.6.3.2.278.gb6431.dirty

[PATCH 3/5] technical/api-remote: Describe new struct remote member pushurl

[Michael J Gruber]

...and pushurl_nr

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/technical/api-remote.txt |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/Documentation/technical/api-remote.txt b/Documentation/technical/api-remote.txt
index 073b22b..c54b17d 100644
--- a/Documentation/technical/api-remote.txt
+++ b/Documentation/technical/api-remote.txt
@@ -18,6 +18,10 @@ struct remote
 
 	An array of all of the url_nr URLs configured for the remote
 
+`pushurl`::
+
+	An array of all of the pushurl_nr push URLs configured for the remote
+
 `push`::
 
 	 An array of refspecs configured for pushing, with
-- 
1.6.3.2.278.gb6431.dirty

[PATCH 4/5] builtin-remote: Show push urls as well

[Michael J Gruber]

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".

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

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 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.278.gb6431.dirty

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

[Michael J Gruber]

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 "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)

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 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.278.gb6431.dirty

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

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> 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://".

I am debating myself if the last two should be just one line,
without "(fetch)" nor "(push)" tacked at the end, like this:

        sb     git://repo.or.cz/git/sbeyer.git

If we change the rule in your patch to format a remote.*.url used for both
push and fetch as a single line to achieve this, however, it would make
your "origin" example come out like this instead:

	origin git://repo.or.cz/git.git
        origin git://git.kernel.org/pub/scm/git/git.git (push)
        origin git://repo.or.cz/alt-git.git (push)

which is arguably better (one less line) and worse (it is unclear if the
top one is only for fetching) at the same time.

Or perhaps we could go with something like this.

	origin git://repo.or.cz/git.git (fetch/push)
        origin git://git.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

i.e. make the rule such that a URL used for both are shown with (fetch/push)
only if there are other lines for the same remote.

Hmm?

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

[Bert Wesarg]

Hi,

On Tue, Jun 9, 2009 at 18:01, Michael J Gruber<git@drmicha.warpmail.net> wrote:
> 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 "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)

Wouldn't it be more readable if push|fetch comes first?

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

And how about to print only one line for (url_nr == 1 && pushurl_nr == 0):

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

>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  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)");
How about using struct strbuf?

Bert

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 09.06.2009 18:25:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> 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?

In any case, if you clone from a fetch-only url (which is the typical
head start) then the default push url does not work (like sb above),
before as well as after this series.

I noticed that I left an URL alias ("repoor" = "repoor:/srv/git/" plus
ssh config for "repoor") in that example which isn't great either...
Maybe I should just use generic domains (example.com).

I'll answer to the other points in a different reply.

Michael

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

[Michael J Gruber]

> On Tue, Jun 9, 2009 at 18:01, Michael J Gruber<git@drmicha.warpmail.net> wrote:
>> 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 "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)

Junio wrote:
> I am debating myself if the last two should be just one line,
> without "(fetch)" nor "(push)" tacked at the end, like this:
> 
>         sb     git://repo.or.cz/git/sbeyer.git
> 
> If we change the rule in your patch to format a remote.*.url used for both
> push and fetch as a single line to achieve this, however, it would make
> your "origin" example come out like this instead:
> 
> 	origin git://repo.or.cz/git.git
>         origin git://git.kernel.org/pub/scm/git/git.git (push)
>         origin git://repo.or.cz/alt-git.git (push)
> 
> which is arguably better (one less line) and worse (it is unclear if the
> top one is only for fetching) at the same time.
> 
> Or perhaps we could go with something like this.
> 
> 	origin git://repo.or.cz/git.git (fetch/push)
>         origin git://git.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
> 
> i.e. make the rule such that a URL used for both are shown with (fetch/push)
> only if there are other lines for the same remote.

Bert wrote:
> Wouldn't it be more readable if push|fetch comes first?
> 
> mjg     (fetch) git://repo.or.cz/git/mjg.git
> mjg     (push)  repoor:/srv/git/git/mjg.git
> origin  (fetch) git://repo.or.cz/git.git
> origin  (push)  git://repo.or.cz/git.git
> origin  (push)  git://git2.kernel.org/pub/scm/git/git.git
> origin  (push)  git://repo.or.cz/alt-git.git
> sb      (fetch) git://repo.or.cz/git/sbeyer.git
> sb      (push)  git://repo.or.cz/git/sbeyer.git
> 
> And how about to print only one line for (url_nr == 1 && pushurl_nr == 0):
> 
> mjg     (fetch) git://repo.or.cz/git/mjg.git
> mjg     (push)  repoor:/srv/git/git/mjg.git
> origin  (fetch) git://repo.or.cz/git.git
> origin  (push)  git://repo.or.cz/git.git
> origin  (push)  git://git2.kernel.org/pub/scm/git/git.git
> origin  (push)  git://repo.or.cz/alt-git.git
> sb              git://repo.or.cz/git/sbeyer.git
> 

All this shows why this one is 5/5 and separate ;) I was thinking hard
about the best display also. From my point of view, there are two
arguments against the seemingly more user friendly variants (collapsing
fetch and push entries with the same url):

- It introduces 3 different types of lines, rather than 2.
- The code would have to go through all (push) urls and check whether it
matches url[0].

While the latter one is technical, the first makes it more difficult to
parse the output, both visually and programmatically (I know, porc...).

Note that at least implicitely we always had 2 different types of lines
in the output of "remote -v": the first one (fetch+push) and the others
(push). There is no way to keep only those 2 types (when there is a
"pushurl" setting), my suggestion was to use 2 types now for the 2 purposes.

>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> ---
>>  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)");
> How about using struct strbuf?

I thought it's complicated enough for the little change in output... But
what do strbufs bring us here?
Before the patch, builtin-remote would leak the string_list. They way I
use malloc enables me to free the string list (using string_list_clear)
later on, including the strings and utils. strcpy and strcat look a bit
low level but I see no point in using the sprintf shotgun here.

More or less I have zero clue about strbuf (OK, probably not less...)
but I think that if util is a strbuf then freeing everything would be
more complicated because a strbuf may or may not be allocated.

Michael

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

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Junio C Hamano venit, vidit, dixit 09.06.2009 18:25:
>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>> 
>>> 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.

[PATCHv2 0/5] builtin-remote: Make "remote -v" display push urls

[Michael J Gruber]

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(-)

[PATCHv2 4/5] builtin-remote: Show push urls as well

[Michael J Gruber]

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 <git@drmicha.warpmail.net>
---
 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

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

[Michael J Gruber]

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 <git@drmicha.warpmail.net>
---
 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

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

[Junio C Hamano]

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;