Cannot override `remote.origin.url` with `-c` option

> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.

> What did you do before the bug happened? (Steps to reproduce your issue)

> What did you expect to happen? (Expected behavior)

```
[remote "origin"]
	url = wrong-url
```

From git(1): 
```
-c <name>=<value>
    Pass a configuration parameter to the command. The
    value given will override values from configuration
    files.
```

> What happened instead? (Actual behavior)

> What's different between what you expected and what actually
> happened?

The -c option is not respected.

> Anything else you want to add:

> Please review the rest of the bug report below.
> You can delete any lines you don't wish to share.

Jeff King· Jun 11, 2024, 07:51 UTC · re: Mathew George · lore

Re: Cannot override `remote.origin.url` with `-c` option

On Sun, Jun 09, 2024 at 12:21:47PM +0530, Mathew George wrote:

> > What did you expect to happen? (Expected behavior)
> 
> The '-c' option should allow me to override the remote URL,
> which, as a result of running `git remote add`, is configured as
> follows in test/.git/config:
> 
> ```
> [remote "origin"]
> 	url = wrong-url
> ```

I don't think "-c" is the culprit here, but rather that remote.*.url is
a multi-valued config option. Rather than having a single value with
"last one wins" semantics, multiple instances of the config variable
form a list. So you'd see the same thing with:
  
  [remote "origin"]
  url = wrong-url
  url = right-url

in the config file.

Of course that leaves two questions:

  1. What are multiple URLs actually good for? I have no idea. I
     couldn't find any useful documentation in the manpages for
     git-config, git-remote, or git-fetch. It looks like any URLs after
     the first are used as push-only URLs:

       $ git remote show -n origin
       * remote origin
       Fetch URL: wrong-url
       Push  URL: wrong-url
       Push  URL: right-url
       [...]

     There's also a "pushurl" config option, which _replaces_ the
     original URL for pushing, rather than including it.

     As far as I can tell there's no way to actually have multiple fetch
     URLs (and the corner cases would be weird anyway). I suspect we'd
     come up with different semantics if we were designing it these
     days, but it has been this way since at least 2007. So any changes
     would probably need a deprecation period.

  2. Is there a way to override the list?

     Sadly, no. For some config keys, we allow a value-less boolean
     entry to reset the list. But since the config code doesn't know
     about the semantics of each key, this has to be implemented
     individually for each key, and nobody has bothered to do so for
     remote.*.url.

     The value-less key is something like:

       [remote "origin"]
       url = wrong-url
       # this one resets the list to empty!
       url
       # and this one adds a new value
       url = right-url

     Or in "-c" terms, it would be:

       git -c remote.origin.url -c remote.origin.url=right-url ...

     The syntax is ugly, but it is backwards compatible (that value-less
     entry is currently an error). So we could do that now (I'll show a
     messy patch below).

  git -c url.right-url.insteadOf=wrong-url ...

-Peff

---

diff --git a/remote.c b/remote.c
index dcb5492c85..69b0f28637 100644
--- a/remote.c
+++ b/remote.c
@@ -63,6 +63,10 @@ static const char *alias_url(const char *url, struct rewrites *r)
 
 static void add_url(struct remote *remote, const char *url)
 {
+	if (!url) {
+		remote->url_nr = 0;
+		return;
+	}
 	ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);
 	remote->url[remote->url_nr++] = url;
 }
@@ -430,10 +434,7 @@ static int handle_config(const char *key, const char *value,
 	else if (!strcmp(subkey, "prunetags"))
 		remote->prune_tags = git_config_bool(key, value);
 	else if (!strcmp(subkey, "url")) {
-		char *v;
-		if (git_config_string(&v, key, value))
-			return -1;
-		add_url(remote, v);
+		add_url(remote, xstrdup_or_null(value));
 	} else if (!strcmp(subkey, "pushurl")) {
 		char *v;
 		if (git_config_string(&v, key, value))

Junio C Hamano· Jun 11, 2024, 15:28 UTC · re: Jeff King · lore

Re: Cannot override `remote.origin.url` with `-c` option

Jeff King <peff@peff.net> writes:

> Of course that leaves two questions:
>
>   1. What are multiple URLs actually good for? I have no idea. I

>   2. Is there a way to override the list?
>
>      Sadly, no. For some config keys, we allow a value-less boolean
>      entry to reset the list.

> Of course none of that helps your immediate case. I did think of one
> workaround, though, which is to use the "insteadOf" config to rewrite
> the URL. So:
>
>   git -c url.right-url.insteadOf=wrong-url ...
>
> will rewrite all instances of "wrong-url" to use "right-url" instead (in
> origin and elsewhere).

Yup, that is an excellent idea.

> diff --git a/remote.c b/remote.c
> index dcb5492c85..69b0f28637 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -63,6 +63,10 @@ static const char *alias_url(const char *url, struct rewrites *r)
>  
>  static void add_url(struct remote *remote, const char *url)
>  {
> +	if (!url) {
> +		remote->url_nr = 0;
> +		return;
> +	}
>  	ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);
>  	remote->url[remote->url_nr++] = url;
>  }
> @@ -430,10 +434,7 @@ static int handle_config(const char *key, const char *value,
>  	else if (!strcmp(subkey, "prunetags"))
>  		remote->prune_tags = git_config_bool(key, value);
>  	else if (!strcmp(subkey, "url")) {
> -		char *v;
> -		if (git_config_string(&v, key, value))
> -			return -1;
> -		add_url(remote, v);
> +		add_url(remote, xstrdup_or_null(value));
>  	} else if (!strcmp(subkey, "pushurl")) {
>  		char *v;
>  		if (git_config_string(&v, key, value))

Jeff King· Jun 13, 2024, 10:24 UTC · re: Junio C Hamano · lore

Re: Cannot override `remote.origin.url` with `-c` option

On Tue, Jun 11, 2024 at 08:28:41AM -0700, Junio C Hamano wrote:

> >   2. Is there a way to override the list?
> >
> >      Sadly, no. For some config keys, we allow a value-less boolean
> >      entry to reset the list.
> 
> Hmph, I somehow thought that a more widely used "clear" was an empty
> string, but if we can make the convention around the "I exist and I
> mean true" entries, that would be great.  It would not make much
> sense to have multi-valued Boolean variable to begin with.

> > diff --git a/remote.c b/remote.c
> [...]
> 
> I was expecting (with excitement) a mess, but the above is as clean
> as we can make the idea, I would say.  Lack of documentation and
> tests do count as incompleteness though of course.

-Peff

Jeff King· Jun 14, 2024, 10:24 UTC · re: Jeff King · lore

[PATCH 0/11] allow overriding remote.*.url

On Thu, Jun 13, 2024 at 06:24:09AM -0400, Jeff King wrote:

> > I was expecting (with excitement) a mess, but the above is as clean
> > as we can make the idea, I would say.  Lack of documentation and
> > tests do count as incompleteness though of course.
> 
> Yeah, and we should probably do the same for pushurl. And I think there
> could be some cleanup of the memory ownership handling of add_url().

We can split the series into segments:

  [01/11]: archive: fix check for missing url

    A nearby trivial bugfix.

  [02/11]: remote: refactor alias_url() memory ownership
  [03/11]: remote: transfer ownership of memory in add_url(), etc
  [04/11]: remote: use strvecs to store remote url/pushurl
  [05/11]: remote: simplify url/pushurl selection

    Fixing memory handling weirdness, which is a necessary prereq for
    the "reset" operation to avoid leaking. The switch to using a strvec
    isn't strictly necessary, but it does make the code (including the
    later patch 7) simpler.

  [06/11]: config: document remote.*.url/pushurl interaction
  [07/11]: remote: allow resetting url list

    The actual change is in patch 7 here, but it was hard to add new
    docs to the rather anemic existing ones. Hence patch 6.

  [08/11]: t5801: make remote-testgit GIT_DIR setup more robust
  [09/11]: t5801: test remote.*.vcs config
  [10/11]: remote: always require at least one url in a remote
  [11/11]: remote: drop checks for zero-url case

    This is a related cleanup I found while working in the area.
    Arguably it could be a separate topic, though it does depend
    textually on what came before.

 Documentation/config/remote.txt | 11 +++-
 builtin/archive.c               |  4 +-
 builtin/clone.c                 |  4 +-
 builtin/ls-remote.c             |  6 +--
 builtin/push.c                  | 28 ++--------
 builtin/remote.c                | 88 +++++++++---------------------
 remote-curl.c                   |  2 +-
 remote.c                        | 94 ++++++++++++++++++---------------
 remote.h                        | 13 ++---
 t/helper/test-bundle-uri.c      |  2 -
 t/t5505-remote.sh               | 36 +++++++++++++
 t/t5801-remote-helpers.sh       | 23 ++++++++
 t/t5801/git-remote-nourl        |  3 ++
 t/t5801/git-remote-testgit      |  3 +-
 transport.c                     | 19 +++----
 15 files changed, 174 insertions(+), 162 deletions(-)
 create mode 100755 t/t5801/git-remote-nourl

-Peff

Jeff King· Jun 14, 2024, 10:25 UTC · re: Jeff King · lore

[PATCH 01/11] archive: fix check for missing url

You can trigger the segfault like this:

  git -c remote.foo.vcs=bar archive --remote=foo

Signed-off-by: Jeff King <peff@peff.net>
---
This code actually goes away in patch 10, but it's possible we'd want to
take a different approach there. So I preferred to fix this up front
anyway.

 builtin/archive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index 15ee1ec7bb..f35560042e 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -31,7 +31,7 @@ static int run_remote_archiver(int argc, const char **argv,
 	struct packet_reader reader;
 
 	_remote = remote_get(remote);
-	if (!_remote->url[0])
+	if (!_remote->url_nr)
 		die(_("git archive: Remote with no URL"));
 	transport = transport_get(_remote, _remote->url[0]);
 	transport_connect(transport, "git-upload-archive", exec, fd);

-- 
2.45.2.937.g0bcb3c087a

Jeff King· Jun 14, 2024, 10:26 UTC · re: Jeff King · lore

[PATCH 02/11] remote: refactor alias_url() memory ownership

Signed-off-by: Jeff King <peff@peff.net>
---
Just to be clear, I think there's probably still a leak in
add_url_alias() even after this patch. It's just a step on the way to
fixing.

 remote.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/remote.c b/remote.c
index dcb5492c85..fd9d58f820 100644
--- a/remote.c
+++ b/remote.c
@@ -35,7 +35,7 @@ static int valid_remote(const struct remote *remote)
 	return (!!remote->url) || (!!remote->foreign_vcs);
 }
 
-static const char *alias_url(const char *url, struct rewrites *r)
+static char *alias_url(const char *url, struct rewrites *r)
 {
 	int i, j;
 	struct counted_string *longest;
@@ -56,7 +56,7 @@ static const char *alias_url(const char *url, struct rewrites *r)
 		}
 	}
 	if (!longest)
-		return url;
+		return NULL;
 
 	return xstrfmt("%s%s", r->rewrite[longest_i]->base, url + longest->len);
 }
@@ -76,15 +76,16 @@ static void add_pushurl(struct remote *remote, const char *pushurl)
 static void add_pushurl_alias(struct remote_state *remote_state,
 			      struct remote *remote, const char *url)
 {
-	const char *pushurl = alias_url(url, &remote_state->rewrites_push);
-	if (pushurl != url)
-		add_pushurl(remote, pushurl);
+	char *alias = alias_url(url, &remote_state->rewrites_push);
+	if (alias)
+		add_pushurl(remote, alias);
 }
 
 static void add_url_alias(struct remote_state *remote_state,
 			  struct remote *remote, const char *url)
 {
-	add_url(remote, alias_url(url, &remote_state->rewrites));
+	char *alias = alias_url(url, &remote_state->rewrites);
+	add_url(remote, alias ? alias : url);
 	add_pushurl_alias(remote_state, remote, url);
 }
 
@@ -492,19 +493,22 @@ static void alias_all_urls(struct remote_state *remote_state)
 		if (!remote_state->remotes[i])
 			continue;
 		for (j = 0; j < remote_state->remotes[i]->pushurl_nr; j++) {
-			remote_state->remotes[i]->pushurl[j] =
-				alias_url(remote_state->remotes[i]->pushurl[j],
-					  &remote_state->rewrites);
+			char *alias = alias_url(remote_state->remotes[i]->pushurl[j],
+						&remote_state->rewrites);
+			if (alias)
+				remote_state->remotes[i]->pushurl[j] = alias;
 		}
 		add_pushurl_aliases = remote_state->remotes[i]->pushurl_nr == 0;
 		for (j = 0; j < remote_state->remotes[i]->url_nr; j++) {
+			char *alias;
 			if (add_pushurl_aliases)
 				add_pushurl_alias(
 					remote_state, remote_state->remotes[i],
 					remote_state->remotes[i]->url[j]);
-			remote_state->remotes[i]->url[j] =
-				alias_url(remote_state->remotes[i]->url[j],
+			alias = alias_url(remote_state->remotes[i]->url[j],
 					  &remote_state->rewrites);
+			if (alias)
+				remote_state->remotes[i]->url[j] = alias;
 		}
 	}
 }

-- 
2.45.2.937.g0bcb3c087a

Junio C Hamano· Jun 14, 2024, 17:05 UTC · re: Jeff King · lore

Re: [PATCH 02/11] remote: refactor alias_url() memory ownership

Jeff King <peff@peff.net> writes:

> ... So instead of
> returning the original string, return NULL, forcing callers to decide
> what to do with it explicitly. We can then build further cleanups on top
> of that.

OK.

> @@ -76,15 +76,16 @@ static void add_pushurl(struct remote *remote, const char *pushurl)
>  static void add_pushurl_alias(struct remote_state *remote_state,
>  			      struct remote *remote, const char *url)
>  {
> -	const char *pushurl = alias_url(url, &remote_state->rewrites_push);
> -	if (pushurl != url)
> -		add_pushurl(remote, pushurl);
> +	char *alias = alias_url(url, &remote_state->rewrites_push);
> +	if (alias)
> +		add_pushurl(remote, alias);

OK, that's an obviously equivalent rewrite.

>  static void add_url_alias(struct remote_state *remote_state,
>  			  struct remote *remote, const char *url)
>  {
> -	add_url(remote, alias_url(url, &remote_state->rewrites));
> +	char *alias = alias_url(url, &remote_state->rewrites);
> +	add_url(remote, alias ? alias : url);
>  	add_pushurl_alias(remote_state, remote, url);
>  }

This is also an obviously equivalent rewrite.

> @@ -492,19 +493,22 @@ static void alias_all_urls(struct remote_state *remote_state)
>  		if (!remote_state->remotes[i])
>  			continue;
>  		for (j = 0; j < remote_state->remotes[i]->pushurl_nr; j++) {
> -			remote_state->remotes[i]->pushurl[j] =
> -				alias_url(remote_state->remotes[i]->pushurl[j],
> -					  &remote_state->rewrites);
> +			char *alias = alias_url(remote_state->remotes[i]->pushurl[j],
> +						&remote_state->rewrites);
> +			if (alias)
> +				remote_state->remotes[i]->pushurl[j] = alias;

Thanks.

>  		}
>  		add_pushurl_aliases = remote_state->remotes[i]->pushurl_nr == 0;
>  		for (j = 0; j < remote_state->remotes[i]->url_nr; j++) {
> +			char *alias;
>  			if (add_pushurl_aliases)
>  				add_pushurl_alias(
>  					remote_state, remote_state->remotes[i],
>  					remote_state->remotes[i]->url[j]);
> -			remote_state->remotes[i]->url[j] =
> -				alias_url(remote_state->remotes[i]->url[j],
> +			alias = alias_url(remote_state->remotes[i]->url[j],
>  					  &remote_state->rewrites);
> +			if (alias)
> +				remote_state->remotes[i]->url[j] = alias;
>  		}
>  	}
>  }

Jeff King· Jun 14, 2024, 10:27 UTC · re: Jeff King · lore

[PATCH 03/11] remote: transfer ownership of memory in add_url(), etc

  - it's impossible to clear any of the arrays, because the data
    dependencies between them are too muddled (if you free() a string,
    it might also be referenced from another array, causing a
    user-after-free; but if you don't, that might be the last reference,
    causing a leak).

    This is mostly of interest for further refactoring and features, but
    there's at least one spot that's already a problem. In alias_all_urls(),
    we replace elements of remote->url and remote->pushurl with their
    aliased forms, dropping references to the original.

  - sometimes strings from outside callers make their way in. For
    example, calling remote_get("foo") when there is no configured "foo"
    remote will create a remote struct with the single url "foo". But
    we'll do so by holding on to the string passed to remote_get()
    forever.

    In practice I think this works out because we'd usually pass in a
    string that lasts the length of the program (a string literal, or
    argv reference, or other data structure allocated in the main
    function). But it's a rather subtle requirement.

Signed-off-by: Jeff King <peff@peff.net>
---
Since now we'll always allocate, I don't think it's possible for this to
introduce any memory corruption issues. It _might_ introduce a leak, but
I think I fixed all of the callers.

 remote.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/remote.c b/remote.c
index fd9d58f820..f7c846865f 100644
--- a/remote.c
+++ b/remote.c
@@ -64,13 +64,13 @@ static char *alias_url(const char *url, struct rewrites *r)
 static void add_url(struct remote *remote, const char *url)
 {
 	ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);
-	remote->url[remote->url_nr++] = url;
+	remote->url[remote->url_nr++] = xstrdup(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;
+	remote->pushurl[remote->pushurl_nr++] = xstrdup(pushurl);
 }
 
 static void add_pushurl_alias(struct remote_state *remote_state,
@@ -79,6 +79,7 @@ static void add_pushurl_alias(struct remote_state *remote_state,
 	char *alias = alias_url(url, &remote_state->rewrites_push);
 	if (alias)
 		add_pushurl(remote, alias);
+	free(alias);
 }
 
 static void add_url_alias(struct remote_state *remote_state,
@@ -87,6 +88,7 @@ static void add_url_alias(struct remote_state *remote_state,
 	char *alias = alias_url(url, &remote_state->rewrites);
 	add_url(remote, alias ? alias : url);
 	add_pushurl_alias(remote_state, remote, url);
+	free(alias);
 }
 
 struct remotes_hash_key {
@@ -293,7 +295,7 @@ static void read_remotes_file(struct remote_state *remote_state,
 
 		if (skip_prefix(buf.buf, "URL:", &v))
 			add_url_alias(remote_state, remote,
-				      xstrdup(skip_spaces(v)));
+				      skip_spaces(v));
 		else if (skip_prefix(buf.buf, "Push:", &v))
 			refspec_append(&remote->push, skip_spaces(v));
 		else if (skip_prefix(buf.buf, "Pull:", &v))
@@ -336,7 +338,7 @@ static void read_branches_file(struct remote_state *remote_state,
 	else
 		frag = to_free = repo_default_branch_name(the_repository, 0);
 
-	add_url_alias(remote_state, remote, strbuf_detach(&buf, NULL));
+	add_url_alias(remote_state, remote, buf.buf);
 	refspec_appendf(&remote->fetch, "refs/heads/%s:refs/heads/%s",
 			frag, remote->name);
 
@@ -347,6 +349,7 @@ static void read_branches_file(struct remote_state *remote_state,
 	refspec_appendf(&remote->push, "HEAD:refs/heads/%s", frag);
 	remote->fetch_tags = 1; /* always auto-follow */
 
+	strbuf_release(&buf);
 	free(to_free);
 }
 
@@ -431,15 +434,13 @@ static int handle_config(const char *key, const char *value,
 	else if (!strcmp(subkey, "prunetags"))
 		remote->prune_tags = git_config_bool(key, value);
 	else if (!strcmp(subkey, "url")) {
-		char *v;
-		if (git_config_string(&v, key, value))
-			return -1;
-		add_url(remote, v);
+		if (!value)
+			return config_error_nonbool(key);
+		add_url(remote, value);
 	} else if (!strcmp(subkey, "pushurl")) {
-		char *v;
-		if (git_config_string(&v, key, value))
-			return -1;
-		add_pushurl(remote, v);
+		if (!value)
+			return config_error_nonbool(key);
+		add_pushurl(remote, value);
 	} else if (!strcmp(subkey, "push")) {
 		char *v;
 		if (git_config_string(&v, key, value))
@@ -495,8 +496,10 @@ static void alias_all_urls(struct remote_state *remote_state)
 		for (j = 0; j < remote_state->remotes[i]->pushurl_nr; j++) {
 			char *alias = alias_url(remote_state->remotes[i]->pushurl[j],
 						&remote_state->rewrites);
-			if (alias)
+			if (alias) {
+				free((char *)remote_state->remotes[i]->pushurl[j]);
 				remote_state->remotes[i]->pushurl[j] = alias;
+			}
 		}
 		add_pushurl_aliases = remote_state->remotes[i]->pushurl_nr == 0;
 		for (j = 0; j < remote_state->remotes[i]->url_nr; j++) {
@@ -507,8 +510,10 @@ static void alias_all_urls(struct remote_state *remote_state)
 					remote_state->remotes[i]->url[j]);
 			alias = alias_url(remote_state->remotes[i]->url[j],
 					  &remote_state->rewrites);
-			if (alias)
+			if (alias) {
+				free((char *)remote_state->remotes[i]->url[j]);
 				remote_state->remotes[i]->url[j] = alias;
+			}
 		}
 	}
 }

-- 
2.45.2.937.g0bcb3c087a

Junio C Hamano· Jun 14, 2024, 17:04 UTC · re: Jeff King · lore

Re: [PATCH 03/11] remote: transfer ownership of memory in add_url(), etc

Jeff King <peff@peff.net> writes:

> diff --git a/remote.c b/remote.c
> index fd9d58f820..f7c846865f 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -64,13 +64,13 @@ static char *alias_url(const char *url, struct rewrites *r)
>  static void add_url(struct remote *remote, const char *url)
>  {
>  	ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);
> -	remote->url[remote->url_nr++] = url;
> +	remote->url[remote->url_nr++] = xstrdup(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;
> +	remote->pushurl[remote->pushurl_nr++] = xstrdup(pushurl);
>  }

>  static void add_pushurl_alias(struct remote_state *remote_state,
> @@ -79,6 +79,7 @@ static void add_pushurl_alias(struct remote_state *remote_state,
>  	char *alias = alias_url(url, &remote_state->rewrites_push);
>  	if (alias)
>  		add_pushurl(remote, alias);
> +	free(alias);
>  }

>  static void add_url_alias(struct remote_state *remote_state,
> @@ -87,6 +88,7 @@ static void add_url_alias(struct remote_state *remote_state,
>  	char *alias = alias_url(url, &remote_state->rewrites);
>  	add_url(remote, alias ? alias : url);
>  	add_pushurl_alias(remote_state, remote, url);
> +	free(alias);
>  }

Likewise.

> @@ -336,7 +338,7 @@ static void read_branches_file(struct remote_state *remote_state,
>  	else
>  		frag = to_free = repo_default_branch_name(the_repository, 0);
>  
> -	add_url_alias(remote_state, remote, strbuf_detach(&buf, NULL));
> +	add_url_alias(remote_state, remote, buf.buf);

It is curious that you delay ...

> @@ -347,6 +349,7 @@ static void read_branches_file(struct remote_state *remote_state,
>  	refspec_appendf(&remote->push, "HEAD:refs/heads/%s", frag);
>  	remote->fetch_tags = 1; /* always auto-follow */
>  
> +	strbuf_release(&buf);
>  	free(to_free);
>  }

... strbuf_release() of the buf to the very end of the function.  

> @@ -431,15 +434,13 @@ static int handle_config(const char *key, const char *value,
>  	else if (!strcmp(subkey, "prunetags"))
>  		remote->prune_tags = git_config_bool(key, value);
>  	else if (!strcmp(subkey, "url")) {
> -		char *v;
> -		if (git_config_string(&v, key, value))
> -			return -1;
> -		add_url(remote, v);
> +		if (!value)
> +			return config_error_nonbool(key);
> +		add_url(remote, value);

> @@ -495,8 +496,10 @@ static void alias_all_urls(struct remote_state *remote_state)
>  		for (j = 0; j < remote_state->remotes[i]->pushurl_nr; j++) {
>  			char *alias = alias_url(remote_state->remotes[i]->pushurl[j],
>  						&remote_state->rewrites);
> -			if (alias)
> +			if (alias) {
> +				free((char *)remote_state->remotes[i]->pushurl[j]);
>  				remote_state->remotes[i]->pushurl[j] = alias;
> +			}
>  		}

Thanks.

Jeff King· Jun 16, 2024, 04:59 UTC · re: Junio C Hamano · lore

Re: [PATCH 03/11] remote: transfer ownership of memory in add_url(), etc

On Fri, Jun 14, 2024 at 10:04:50AM -0700, Junio C Hamano wrote:

> >  static void add_pushurl_alias(struct remote_state *remote_state,
> > @@ -79,6 +79,7 @@ static void add_pushurl_alias(struct remote_state *remote_state,
> >  	char *alias = alias_url(url, &remote_state->rewrites_push);
> >  	if (alias)
> >  		add_pushurl(remote, alias);
> > +	free(alias);
> >  }
> 
> OK.  I wondered if we want to strdup(url) in my review on the
> previous step, but now we are making the add_url() responsible
> for making a copy, we instead do the opposite, i.e. free alias
> that was allocated for us because we no longer need it.

> > @@ -336,7 +338,7 @@ static void read_branches_file(struct remote_state *remote_state,
> >  	else
> >  		frag = to_free = repo_default_branch_name(the_repository, 0);
> >  
> > -	add_url_alias(remote_state, remote, strbuf_detach(&buf, NULL));
> > +	add_url_alias(remote_state, remote, buf.buf);
> 
> It is curious that you delay ...
> 
> > @@ -347,6 +349,7 @@ static void read_branches_file(struct remote_state *remote_state,
> >  	refspec_appendf(&remote->push, "HEAD:refs/heads/%s", frag);
> >  	remote->fetch_tags = 1; /* always auto-follow */
> >  
> > +	strbuf_release(&buf);
> >  	free(to_free);
> >  }
> 
> ... strbuf_release() of the buf to the very end of the function.  
> 
> In the original, buf became invalid by doing strbuf_detach(), so we
> could do strbuf_release() immediately after add_url_alias() returns
> to us if we wanted to.

Bumping to --inter-hunk-context=4 does it a little more obvious.

> > @@ -431,15 +434,13 @@ static int handle_config(const char *key, const char *value,
> >  	else if (!strcmp(subkey, "prunetags"))
> >  		remote->prune_tags = git_config_bool(key, value);
> >  	else if (!strcmp(subkey, "url")) {
> > -		char *v;
> > -		if (git_config_string(&v, key, value))
> > -			return -1;
> > -		add_url(remote, v);
> > +		if (!value)
> > +			return config_error_nonbool(key);
> > +		add_url(remote, value);
> 
> OK.  config_string() does (1) check for "I exist hence I am true"
> boolean, and (2) give us a duplicate of the value.  We do not want
> the latter, so we do the former ourselves here.  The same story
> repeats for pushurl below (ellided).

-Peff

Junio C Hamano· Jun 17, 2024, 17:42 UTC · re: Jeff King · lore

Re: [PATCH 03/11] remote: transfer ownership of memory in add_url(), etc

Jeff King <peff@peff.net> writes:

> On Fri, Jun 14, 2024 at 10:04:50AM -0700, Junio C Hamano wrote:
>
>> >  static void add_pushurl_alias(struct remote_state *remote_state,
>> > @@ -79,6 +79,7 @@ static void add_pushurl_alias(struct remote_state *remote_state,
>> >  	char *alias = alias_url(url, &remote_state->rewrites_push);
>> >  	if (alias)
>> >  		add_pushurl(remote, alias);
>> > +	free(alias);
>> >  }
>> 
>> OK.  I wondered if we want to strdup(url) in my review on the
>> previous step, but now we are making the add_url() responsible
>> for making a copy, we instead do the opposite, i.e. free alias
>> that was allocated for us because we no longer need it.
>
> Yeah. Possibly the two should be squashed. I was trying to make this
> patch a little less long/confusing, but maybe breaking things up just
> posed new questions. :)

> Right. I had originally written it that way, since that would be the
> mechanical conversion. But since there was already cleanup at the bottom
> of the function, it felt more natural to shuffle it there. Which is
> correct as long as there are no other references to buf nor early
> returns. You can't see that from the context, but it is true in this
> case.

Looking good.

Elijah Newren· Jun 25, 2024, 17:30 UTC · re: Jeff King · lore

Re: [PATCH 03/11] remote: transfer ownership of memory in add_url(), etc

On Fri, Jun 14, 2024 at 4:08 AM Jeff King <peff@peff.net> wrote:

>
> Many of the internal functions in remote.c take const strings and store
> them forever in instances of "struct remote". Since the functions are
> internal and callers are aware of the convention, this seems to mostly
> work and not cause leaks. But there are some issues:
>
>   - it's impossible to clear any of the arrays, because the data
>     dependencies between them are too muddled (if you free() a string,
>     it might also be referenced from another array, causing a
>     user-after-free; but if you don't, that might be the last reference,

s/user-after-free/use-after-free/

(Read the rest of the patch and it all looks good.)

Jeff King· Jun 14, 2024, 10:28 UTC · re: Jeff King · lore

[PATCH 04/11] remote: use strvecs to store remote url/pushurl

  - push/clear are now one-liners

  - likewise the free+assigns in alias_all_urls() can use
    strvec_replace()

  - we now use size_t for storage, avoiding possible overflow

  - this will enable some further cleanups in future patches

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/archive.c          |  4 +--
 builtin/clone.c            |  4 +--
 builtin/ls-remote.c        |  6 ++--
 builtin/push.c             | 10 +++----
 builtin/remote.c           | 56 +++++++++++++++++++-------------------
 remote-curl.c              |  2 +-
 remote.c                   | 52 ++++++++++++++---------------------
 remote.h                   | 12 ++------
 t/helper/test-bundle-uri.c |  2 +-
 transport.c                |  4 +--
 10 files changed, 68 insertions(+), 84 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index f35560042e..0d9aff7e6f 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -31,9 +31,9 @@ static int run_remote_archiver(int argc, const char **argv,
 	struct packet_reader reader;
 
 	_remote = remote_get(remote);
-	if (!_remote->url_nr)
+	if (!_remote->url.nr)
 		die(_("git archive: Remote with no URL"));
-	transport = transport_get(_remote, _remote->url[0]);
+	transport = transport_get(_remote, _remote->url.v[0]);
 	transport_connect(transport, "git-upload-archive", exec, fd);
 
 	/*
diff --git a/builtin/clone.c b/builtin/clone.c
index 730b3efae6..d3b70b49b0 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1290,7 +1290,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	refspec_appendf(&remote->fetch, "+%s*:%s*", src_ref_prefix,
 			branch_top.buf);
 
-	path = get_repo_path(remote->url[0], &is_bundle);
+	path = get_repo_path(remote->url.v[0], &is_bundle);
 	is_local = option_local != 0 && path && !is_bundle;
 	if (is_local) {
 		if (option_depth)
@@ -1312,7 +1312,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (option_local > 0 && !is_local)
 		warning(_("--local is ignored"));
 
-	transport = transport_get(remote, path ? path : remote->url[0]);
+	transport = transport_get(remote, path ? path : remote->url.v[0]);
 	transport_set_verbosity(transport, option_verbosity, option_progress);
 	transport->family = family;
 	transport->cloning = 1;
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index e8d65ebbdc..4c04dbbf19 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -109,11 +109,11 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 			die("bad repository '%s'", dest);
 		die("No remote configured to list refs from.");
 	}
-	if (!remote->url_nr)
+	if (!remote->url.nr)
 		die("remote %s has no configured URL", dest);
 
 	if (get_url) {
-		printf("%s\n", *remote->url);
+		printf("%s\n", remote->url.v[0]);
 		return 0;
 	}
 
@@ -130,7 +130,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	}
 
 	if (!dest && !quiet)
-		fprintf(stderr, "From %s\n", *remote->url);
+		fprintf(stderr, "From %s\n", remote->url.v[0]);
 	for ( ; ref; ref = ref->next) {
 		struct ref_array_item *item;
 		if (!check_ref_type(ref, flags))
diff --git a/builtin/push.c b/builtin/push.c
index 2fbb31c3ad..61b5d3afdd 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -143,12 +143,12 @@ static void set_refspecs(const char **refs, int nr, const char *repo)
 
 static int push_url_of_remote(struct remote *remote, const char ***url_p)
 {
-	if (remote->pushurl_nr) {
-		*url_p = remote->pushurl;
-		return remote->pushurl_nr;
+	if (remote->pushurl.nr) {
+		*url_p = remote->pushurl.v;
+		return remote->pushurl.nr;
 	}
-	*url_p = remote->url;
-	return remote->url_nr;
+	*url_p = remote->url.v;
+	return remote->url.nr;
 }
 
 static NORETURN void die_push_simple(struct branch *branch,
diff --git a/builtin/remote.c b/builtin/remote.c
index 447ef1d3c9..ee6a33ff11 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -619,8 +619,8 @@ static int migrate_file(struct remote *remote)
 	int i;
 
 	strbuf_addf(&buf, "remote.%s.url", remote->name);
-	for (i = 0; i < remote->url_nr; i++)
-		git_config_set_multivar(buf.buf, remote->url[i], "^$", 0);
+	for (i = 0; i < remote->url.nr; i++)
+		git_config_set_multivar(buf.buf, remote->url.v[i], "^$", 0);
 	strbuf_reset(&buf);
 	strbuf_addf(&buf, "remote.%s.push", remote->name);
 	for (i = 0; i < remote->push.raw_nr; i++)
@@ -1002,8 +1002,8 @@ static int get_remote_ref_states(const char *name,
 		struct transport *transport;
 		const struct ref *remote_refs;
 
-		transport = transport_get(states->remote, states->remote->url_nr > 0 ?
-			states->remote->url[0] : NULL);
+		transport = transport_get(states->remote, states->remote->url.nr > 0 ?
+			states->remote->url.v[0] : NULL);
 		remote_refs = transport_get_remote_refs(transport, NULL);
 
 		states->queried = 1;
@@ -1216,12 +1216,12 @@ static int get_one_entry(struct remote *remote, void *priv)
 	const char **url;
 	int i, url_nr;
 
-	if (remote->url_nr > 0) {
+	if (remote->url.nr > 0) {
 		struct strbuf promisor_config = STRBUF_INIT;
 		const char *partial_clone_filter = NULL;
 
 		strbuf_addf(&promisor_config, "remote.%s.partialclonefilter", remote->name);
-		strbuf_addf(&remote_info_buf, "%s (fetch)", remote->url[0]);
+		strbuf_addf(&remote_info_buf, "%s (fetch)", remote->url.v[0]);
 		if (!git_config_get_string_tmp(promisor_config.buf, &partial_clone_filter))
 			strbuf_addf(&remote_info_buf, " [%s]", partial_clone_filter);
 
@@ -1230,12 +1230,12 @@ static int get_one_entry(struct remote *remote, void *priv)
 				strbuf_detach(&remote_info_buf, NULL);
 	} else
 		string_list_append(list, remote->name)->util = NULL;
-	if (remote->pushurl_nr) {
-		url = remote->pushurl;
-		url_nr = remote->pushurl_nr;
+	if (remote->pushurl.nr) {
+		url = remote->pushurl.v;
+		url_nr = remote->pushurl.nr;
 	} else {
-		url = remote->url;
-		url_nr = remote->url_nr;
+		url = remote->url.v;
+		url_nr = remote->url.nr;
 	}
 	for (i = 0; i < url_nr; i++)
 	{
@@ -1301,14 +1301,14 @@ static int show(int argc, const char **argv, const char *prefix)
 		get_remote_ref_states(*argv, &info.states, query_flag);
 
 		printf_ln(_("* remote %s"), *argv);
-		printf_ln(_("  Fetch URL: %s"), info.states.remote->url_nr > 0 ?
-		       info.states.remote->url[0] : _("(no URL)"));
-		if (info.states.remote->pushurl_nr) {
-			url = info.states.remote->pushurl;
-			url_nr = info.states.remote->pushurl_nr;
+		printf_ln(_("  Fetch URL: %s"), info.states.remote->url.nr > 0 ?
+		       info.states.remote->url.v[0] : _("(no URL)"));
+		if (info.states.remote->pushurl.nr) {
+			url = info.states.remote->pushurl.v;
+			url_nr = info.states.remote->pushurl.nr;
 		} else {
-			url = info.states.remote->url;
-			url_nr = info.states.remote->url_nr;
+			url = info.states.remote->url.v;
+			url_nr = info.states.remote->url.nr;
 		}
 		for (i = 0; i < url_nr; i++)
 			/*
@@ -1454,8 +1454,8 @@ static int prune_remote(const char *remote, int dry_run)
 
 	printf_ln(_("Pruning %s"), remote);
 	printf_ln(_("URL: %s"),
-		  states.remote->url_nr
-		  ? states.remote->url[0]
+		  states.remote->url.nr
+		  ? states.remote->url.v[0]
 		  : _("(no URL)"));
 
 	for_each_string_list_item(item, &states.stale)
@@ -1647,15 +1647,15 @@ static int get_url(int argc, const char **argv, const char *prefix)
 
 	url_nr = 0;
 	if (push_mode) {
-		url = remote->pushurl;
-		url_nr = remote->pushurl_nr;
+		url = remote->pushurl.v;
+		url_nr = remote->pushurl.nr;
 	}
 	/* else fetch mode */
 
 	/* Use the fetch URL when no push URLs were found or requested. */
 	if (!url_nr) {
-		url = remote->url;
-		url_nr = remote->url_nr;
+		url = remote->url.v;
+		url_nr = remote->url.nr;
 	}
 
 	if (!url_nr)
@@ -1718,12 +1718,12 @@ static int set_url(int argc, const char **argv, const char *prefix)
 
 	if (push_mode) {
 		strbuf_addf(&name_buf, "remote.%s.pushurl", remotename);
-		urlset = remote->pushurl;
-		urlset_nr = remote->pushurl_nr;
+		urlset = remote->pushurl.v;
+		urlset_nr = remote->pushurl.nr;
 	} else {
 		strbuf_addf(&name_buf, "remote.%s.url", remotename);
-		urlset = remote->url;
-		urlset_nr = remote->url_nr;
+		urlset = remote->url.v;
+		urlset_nr = remote->url.nr;
 	}
 
 	/* Special cases that add new entry. */
diff --git a/remote-curl.c b/remote-curl.c
index 6008d7e87c..22957c16db 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1574,7 +1574,7 @@ int cmd_main(int argc, const char **argv)
 	if (argc > 2) {
 		end_url_with_slash(&url, argv[2]);
 	} else {
-		end_url_with_slash(&url, remote->url[0]);
+		end_url_with_slash(&url, remote->url.v[0]);
 	}
 
 	http_init(remote, url.buf, 0);
diff --git a/remote.c b/remote.c
index f7c846865f..76a3e41c73 100644
--- a/remote.c
+++ b/remote.c
@@ -32,7 +32,7 @@ struct counted_string {
 
 static int valid_remote(const struct remote *remote)
 {
-	return (!!remote->url) || (!!remote->foreign_vcs);
+	return (!!remote->url.nr) || (!!remote->foreign_vcs);
 }
 
 static char *alias_url(const char *url, struct rewrites *r)
@@ -63,14 +63,12 @@ static char *alias_url(const char *url, struct rewrites *r)
 
 static void add_url(struct remote *remote, const char *url)
 {
-	ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);
-	remote->url[remote->url_nr++] = xstrdup(url);
+	strvec_push(&remote->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++] = xstrdup(pushurl);
+	strvec_push(&remote->pushurl, pushurl);
 }
 
 static void add_pushurl_alias(struct remote_state *remote_state,
@@ -150,18 +148,12 @@ static struct remote *make_remote(struct remote_state *remote_state,
 
 static void remote_clear(struct remote *remote)
 {
-	int i;
-
 	free((char *)remote->name);
 	free((char *)remote->foreign_vcs);
 
-	for (i = 0; i < remote->url_nr; i++)
-		free((char *)remote->url[i]);
-	FREE_AND_NULL(remote->url);
+	strvec_clear(&remote->url);
+	strvec_clear(&remote->pushurl);
 
-	for (i = 0; i < remote->pushurl_nr; i++)
-		free((char *)remote->pushurl[i]);
-	FREE_AND_NULL(remote->pushurl);
 	free((char *)remote->receivepack);
 	free((char *)remote->uploadpack);
 	FREE_AND_NULL(remote->http_proxy);
@@ -493,27 +485,25 @@ static void alias_all_urls(struct remote_state *remote_state)
 		int add_pushurl_aliases;
 		if (!remote_state->remotes[i])
 			continue;
-		for (j = 0; j < remote_state->remotes[i]->pushurl_nr; j++) {
-			char *alias = alias_url(remote_state->remotes[i]->pushurl[j],
+		for (j = 0; j < remote_state->remotes[i]->pushurl.nr; j++) {
+			char *alias = alias_url(remote_state->remotes[i]->pushurl.v[j],
 						&remote_state->rewrites);
-			if (alias) {
-				free((char *)remote_state->remotes[i]->pushurl[j]);
-				remote_state->remotes[i]->pushurl[j] = alias;
-			}
+			if (alias)
+				strvec_replace(&remote_state->remotes[i]->pushurl,
+					       j, alias);
 		}
-		add_pushurl_aliases = remote_state->remotes[i]->pushurl_nr == 0;
-		for (j = 0; j < remote_state->remotes[i]->url_nr; j++) {
+		add_pushurl_aliases = remote_state->remotes[i]->pushurl.nr == 0;
+		for (j = 0; j < remote_state->remotes[i]->url.nr; j++) {
 			char *alias;
 			if (add_pushurl_aliases)
 				add_pushurl_alias(
 					remote_state, remote_state->remotes[i],
-					remote_state->remotes[i]->url[j]);
-			alias = alias_url(remote_state->remotes[i]->url[j],
+					remote_state->remotes[i]->url.v[j]);
+			alias = alias_url(remote_state->remotes[i]->url.v[j],
 					  &remote_state->rewrites);
-			if (alias) {
-				free((char *)remote_state->remotes[i]->url[j]);
-				remote_state->remotes[i]->url[j] = alias;
-			}
+			if (alias)
+				strvec_replace(&remote_state->remotes[i]->url,
+					       j, alias);
 		}
 	}
 }
@@ -653,10 +643,10 @@ static void validate_remote_url(struct remote *remote)
 	else
 		die(_("unrecognized value transfer.credentialsInUrl: '%s'"), value);
 
-	for (i = 0; i < remote->url_nr; i++) {
+	for (i = 0; i < remote->url.nr; i++) {
 		struct url_info url_info = { 0 };
 
-		if (!url_normalize(remote->url[i], &url_info) ||
+		if (!url_normalize(remote->url.v[i], &url_info) ||
 		    !url_info.passwd_off)
 			goto loop_cleanup;
 
@@ -830,8 +820,8 @@ struct ref *ref_remove_duplicates(struct ref *ref_map)
 int remote_has_url(struct remote *remote, const char *url)
 {
 	int i;
-	for (i = 0; i < remote->url_nr; i++) {
-		if (!strcmp(remote->url[i], url))
+	for (i = 0; i < remote->url.nr; i++) {
+		if (!strcmp(remote->url.v[i], url))
 			return 1;
 	}
 	return 0;
diff --git a/remote.h b/remote.h
index e8c6655e42..84dc91cca0 100644
--- a/remote.h
+++ b/remote.h
@@ -4,6 +4,7 @@
 #include "hash-ll.h"
 #include "hashmap.h"
 #include "refspec.h"
+#include "strvec.h"
 
 struct option;
 struct transport_ls_refs_options;
@@ -68,16 +69,9 @@ struct remote {
 	char *foreign_vcs;
 
 	/* An array of all of the url_nr URLs configured for the remote */
-	const char **url;
-
-	int url_nr;
-	int url_alloc;
-
+	struct strvec url;
 	/* An array of all of the pushurl_nr push URLs configured for the remote */
-	const char **pushurl;
-
-	int pushurl_nr;
-	int pushurl_alloc;
+	struct strvec pushurl;
 
 	struct refspec push;
 
diff --git a/t/helper/test-bundle-uri.c b/t/helper/test-bundle-uri.c
index 09dc78733c..3285dd962e 100644
--- a/t/helper/test-bundle-uri.c
+++ b/t/helper/test-bundle-uri.c
@@ -88,7 +88,7 @@ static int cmd_ls_remote(int argc, const char **argv)
 			die(_("bad repository '%s'"), dest);
 		die(_("no remote configured to get bundle URIs from"));
 	}
-	if (!remote->url_nr)
+	if (!remote->url.nr)
 		die(_("remote '%s' has no configured URL"), dest);
 
 	transport = transport_get(remote, NULL);
diff --git a/transport.c b/transport.c
index 83ddea8fbc..eba92eb7e0 100644
--- a/transport.c
+++ b/transport.c
@@ -1127,8 +1127,8 @@ struct transport *transport_get(struct remote *remote, const char *url)
 	ret->remote = remote;
 	helper = remote->foreign_vcs;
 
-	if (!url && remote->url)
-		url = remote->url[0];
+	if (!url && remote->url.nr)
+		url = remote->url.v[0];
 	ret->url = url;
 
 	/* maybe it is a foreign URL? */

-- 
2.45.2.937.g0bcb3c087a

Elijah Newren· Jun 25, 2024, 17:32 UTC · re: Jeff King · lore

Re: [PATCH 04/11] remote: use strvecs to store remote url/pushurl

On Fri, Jun 14, 2024 at 3:32 AM Jeff King <peff@peff.net> wrote:

>
> Now that the url/pushurl fields of "struct remote" own their strings, we
> can switch from bare arrays to strvecs. This has a few advantages:
>
>   - push/clear are now one-liners
>
>   - likewise the free+assigns in alias_all_urls() can use
>     strvec_replace()
>
>   - we now use size_t for storage, avoiding possible overflow
>
>   - this will enable some further cleanups in future patches
>
> There's quite a bit of fallout in the code that reads these fields, as
> it tends to access these arrays directly. But it's mostly a mechanical
> replacement of "url_nr" with "url.nr", and "url[i]" with "url.v[i]",
> with a few variations (e.g. "*url" could become "*url.v", but I used
> "url.v[0]" for consistency).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/archive.c          |  4 +--
>  builtin/clone.c            |  4 +--
>  builtin/ls-remote.c        |  6 ++--
>  builtin/push.c             | 10 +++----
>  builtin/remote.c           | 56 +++++++++++++++++++-------------------
>  remote-curl.c              |  2 +-
>  remote.c                   | 52 ++++++++++++++---------------------
>  remote.h                   | 12 ++------
>  t/helper/test-bundle-uri.c |  2 +-
>  transport.c                |  4 +--
>  10 files changed, 68 insertions(+), 84 deletions(-)
>

[...]

Jeff King· Jun 14, 2024, 10:29 UTC · re: Jeff King · lore

[PATCH 05/11] remote: simplify url/pushurl selection

  - if the user configured any remote.*.pushurl keys, then those make
    the complete set of push urls

  - otherwise we push to all urls in remote.*.url

There are two spots to pay special attention to here:

  1. in builtin/remote.c's get_url(), we are selecting first based on
     push_mode and then falling back to "url" when we're in push_mode
     but no pushurl is defined. The updated code makes that much more
     clear, compared to the original which had an "else" fall-through.

  2. likewise in that file's set_url(), we _only_ respect push_mode,
     sine the point is that we are adding to pushurl in that case
     (whether it is empty or not). And thus it does not use our helper
     function.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/push.c   | 21 ++++-----------
 builtin/remote.c | 69 ++++++++++++++----------------------------------
 remote.c         |  5 ++++
 remote.h         |  1 +
 4 files changed, 31 insertions(+), 65 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 61b5d3afdd..00d99af1a8 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -141,16 +141,6 @@ static void set_refspecs(const char **refs, int nr, const char *repo)
 	free_refs(local_refs);
 }
 
-static int push_url_of_remote(struct remote *remote, const char ***url_p)
-{
-	if (remote->pushurl.nr) {
-		*url_p = remote->pushurl.v;
-		return remote->pushurl.nr;
-	}
-	*url_p = remote->url.v;
-	return remote->url.nr;
-}
-
 static NORETURN void die_push_simple(struct branch *branch,
 				     struct remote *remote)
 {
@@ -434,8 +424,7 @@ static int do_push(int flags,
 		   struct remote *remote)
 {
 	int i, errs;
-	const char **url;
-	int url_nr;
+	struct strvec *url;
 	struct refspec *push_refspec = &rs;
 
 	if (push_options->nr)
@@ -448,11 +437,11 @@ static int do_push(int flags,
 			setup_default_push_refspecs(&flags, remote);
 	}
 	errs = 0;
-	url_nr = push_url_of_remote(remote, &url);
-	if (url_nr) {
-		for (i = 0; i < url_nr; i++) {
+	url = push_url_of_remote(remote);
+	if (url->nr) {
+		for (i = 0; i < url->nr; i++) {
 			struct transport *transport =
-				transport_get(remote, url[i]);
+				transport_get(remote, url->v[i]);
 			if (flags & TRANSPORT_PUSH_OPTIONS)
 				transport->push_options = push_options;
 			if (push_with_options(transport, push_refspec, flags))
diff --git a/builtin/remote.c b/builtin/remote.c
index ee6a33ff11..06c09ed060 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1213,8 +1213,8 @@ static int get_one_entry(struct remote *remote, void *priv)
 {
 	struct string_list *list = priv;
 	struct strbuf remote_info_buf = STRBUF_INIT;
-	const char **url;
-	int i, url_nr;
+	struct strvec *url;
+	int i;
 
 	if (remote->url.nr > 0) {
 		struct strbuf promisor_config = STRBUF_INIT;
@@ -1230,16 +1230,10 @@ static int get_one_entry(struct remote *remote, void *priv)
 				strbuf_detach(&remote_info_buf, NULL);
 	} else
 		string_list_append(list, remote->name)->util = NULL;
-	if (remote->pushurl.nr) {
-		url = remote->pushurl.v;
-		url_nr = remote->pushurl.nr;
-	} else {
-		url = remote->url.v;
-		url_nr = remote->url.nr;
-	}
-	for (i = 0; i < url_nr; i++)
+	url = push_url_of_remote(remote);
+	for (i = 0; i < url->nr; i++)
 	{
-		strbuf_addf(&remote_info_buf, "%s (push)", url[i]);
+		strbuf_addf(&remote_info_buf, "%s (push)", url->v[i]);
 		string_list_append(list, remote->name)->util =
 				strbuf_detach(&remote_info_buf, NULL);
 	}
@@ -1295,28 +1289,21 @@ static int show(int argc, const char **argv, const char *prefix)
 
 	for (; argc; argc--, argv++) {
 		int i;
-		const char **url;
-		int url_nr;
+		struct strvec *url;
 
 		get_remote_ref_states(*argv, &info.states, query_flag);
 
 		printf_ln(_("* remote %s"), *argv);
 		printf_ln(_("  Fetch URL: %s"), info.states.remote->url.nr > 0 ?
 		       info.states.remote->url.v[0] : _("(no URL)"));
-		if (info.states.remote->pushurl.nr) {
-			url = info.states.remote->pushurl.v;
-			url_nr = info.states.remote->pushurl.nr;
-		} else {
-			url = info.states.remote->url.v;
-			url_nr = info.states.remote->url.nr;
-		}
-		for (i = 0; i < url_nr; i++)
+		url = push_url_of_remote(info.states.remote);
+		for (i = 0; i < url->nr; i++)
 			/*
 			 * TRANSLATORS: the colon ':' should align
 			 * with the one in " Fetch URL: %s"
 			 * translation.
 			 */
-			printf_ln(_("  Push  URL: %s"), url[i]);
+			printf_ln(_("  Push  URL: %s"), url->v[i]);
 		if (!i)
 			printf_ln(_("  Push  URL: %s"), _("(no URL)"));
 		if (no_query)
@@ -1622,8 +1609,7 @@ static int get_url(int argc, const char **argv, const char *prefix)
 	int i, push_mode = 0, all_mode = 0;
 	const char *remotename = NULL;
 	struct remote *remote;
-	const char **url;
-	int url_nr;
+	struct strvec *url;
 	struct option options[] = {
 		OPT_BOOL('\0', "push", &push_mode,
 			 N_("query push URLs rather than fetch URLs")),
@@ -1645,27 +1631,15 @@ static int get_url(int argc, const char **argv, const char *prefix)
 		exit(2);
 	}
 
-	url_nr = 0;
-	if (push_mode) {
-		url = remote->pushurl.v;
-		url_nr = remote->pushurl.nr;
-	}
-	/* else fetch mode */
-
-	/* Use the fetch URL when no push URLs were found or requested. */
-	if (!url_nr) {
-		url = remote->url.v;
-		url_nr = remote->url.nr;
-	}
-
-	if (!url_nr)
+	url = push_mode ? push_url_of_remote(remote) : &remote->url;
+	if (!url->nr)
 		die(_("no URLs configured for remote '%s'"), remotename);
 
 	if (all_mode) {
-		for (i = 0; i < url_nr; i++)
-			printf_ln("%s", url[i]);
+		for (i = 0; i < url->nr; i++)
+			printf_ln("%s", url->v[i]);
 	} else {
-		printf_ln("%s", *url);
+		printf_ln("%s", url->v[0]);
 	}
 
 	return 0;
@@ -1680,8 +1654,7 @@ static int set_url(int argc, const char **argv, const char *prefix)
 	const char *oldurl = NULL;
 	struct remote *remote;
 	regex_t old_regex;
-	const char **urlset;
-	int urlset_nr;
+	struct strvec *urlset;
 	struct strbuf name_buf = STRBUF_INIT;
 	struct option options[] = {
 		OPT_BOOL('\0', "push", &push_mode,
@@ -1718,12 +1691,10 @@ static int set_url(int argc, const char **argv, const char *prefix)
 
 	if (push_mode) {
 		strbuf_addf(&name_buf, "remote.%s.pushurl", remotename);
-		urlset = remote->pushurl.v;
-		urlset_nr = remote->pushurl.nr;
+		urlset = &remote->pushurl;
 	} else {
 		strbuf_addf(&name_buf, "remote.%s.url", remotename);
-		urlset = remote->url.v;
-		urlset_nr = remote->url.nr;
+		urlset = &remote->url;
 	}
 
 	/* Special cases that add new entry. */
@@ -1740,8 +1711,8 @@ static int set_url(int argc, const char **argv, const char *prefix)
 	if (regcomp(&old_regex, oldurl, REG_EXTENDED))
 		die(_("Invalid old URL pattern: %s"), oldurl);
 
-	for (i = 0; i < urlset_nr; i++)
-		if (!regexec(&old_regex, urlset[i], 0, NULL, 0))
+	for (i = 0; i < urlset->nr; i++)
+		if (!regexec(&old_regex, urlset->v[i], 0, NULL, 0))
 			matches++;
 		else
 			negative_matches++;
diff --git a/remote.c b/remote.c
index 76a3e41c73..9417d83e51 100644
--- a/remote.c
+++ b/remote.c
@@ -827,6 +827,11 @@ int remote_has_url(struct remote *remote, const char *url)
 	return 0;
 }
 
+struct strvec *push_url_of_remote(struct remote *remote)
+{
+	return remote->pushurl.nr ? &remote->pushurl : &remote->url;
+}
+
 static int match_name_with_pattern(const char *key, const char *name,
 				   const char *value, char **result)
 {
diff --git a/remote.h b/remote.h
index 84dc91cca0..034f9d6660 100644
--- a/remote.h
+++ b/remote.h
@@ -123,6 +123,7 @@ typedef int each_remote_fn(struct remote *remote, void *priv);
 int for_each_remote(each_remote_fn fn, void *priv);
 
 int remote_has_url(struct remote *remote, const char *url);
+struct strvec *push_url_of_remote(struct remote *remote);
 
 struct ref_push_report {
 	const char *ref_name;

-- 
2.45.2.937.g0bcb3c087a

Elijah Newren· Jun 25, 2024, 17:33 UTC · re: Jeff King · lore

Re: [PATCH 05/11] remote: simplify url/pushurl selection

On Fri, Jun 14, 2024 at 3:32 AM Jeff King <peff@peff.net> wrote:

>

[...]

> There are two spots to pay special attention to here:
>
>   1. in builtin/remote.c's get_url(), we are selecting first based on
>      push_mode and then falling back to "url" when we're in push_mode
>      but no pushurl is defined. The updated code makes that much more
>      clear, compared to the original which had an "else" fall-through.
>
>   2. likewise in that file's set_url(), we _only_ respect push_mode,
>      sine the point is that we are adding to pushurl in that case

s/sine/since/

>      (whether it is empty or not). And thus it does not use our helper
>      function.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/push.c   | 21 ++++-----------
>  builtin/remote.c | 69 ++++++++++++++----------------------------------
>  remote.c         |  5 ++++
>  remote.h         |  1 +
>  4 files changed, 31 insertions(+), 65 deletions(-)
>

[...]

I like the simplifications in this patch; it all looks good to me.

Jeff King· Jun 14, 2024, 10:30 UTC · re: Jeff King · lore

[PATCH 06/11] config: document remote.*.url/pushurl interaction

  - these are keys that can be defined multiple times with meaningful
    behavior (especially remote.*.url)

  - the way that pushurl overrides url (the git-push page does mention
    that "pushurl defaults to url", but it is not immediately clear what
    a multi-valued url would do in that situation).

Let's try to summarize the current behavior.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/config/remote.txt | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt
index 0678b4bcfe..eef0bf4f62 100644
--- a/Documentation/config/remote.txt
+++ b/Documentation/config/remote.txt
@@ -5,10 +5,16 @@ remote.pushDefault::
 
 remote.<name>.url::
 	The URL of a remote repository.  See linkgit:git-fetch[1] or
-	linkgit:git-push[1].
+	linkgit:git-push[1]. A configured remote can have multiple URLs;
+	in this case the first is used for fetching, and all are used
+	for pushing (assuming no `remote.<name>.pushurl` is defined).
 
 remote.<name>.pushurl::
 	The push URL of a remote repository.  See linkgit:git-push[1].
+	If a `pushurl` option is present in a configured remote, it
+	is used for pushing instead of `remote.<name>.url`. A configured
+	remote can have multiple push URLs; in this case a push goes to
+	all of them.
 
 remote.<name>.proxy::
 	For remotes that require curl (http, https and ftp), the URL to

-- 
2.45.2.937.g0bcb3c087a

Elijah Newren· Jun 25, 2024, 17:34 UTC · re: Jeff King · lore

Re: [PATCH 06/11] config: document remote.*.url/pushurl interaction

On Fri, Jun 14, 2024 at 3:43 AM Jeff King <peff@peff.net> wrote:

>
> The documentation for these keys gives a very terse definition and
> points you to the fetch/push manpages. But from reading those pages it
> was not at all obvious to me that:
>
>   - these are keys that can be defined multiple times with meaningful
>     behavior (especially remote.*.url)
>
>   - the way that pushurl overrides url (the git-push page does mention
>     that "pushurl defaults to url", but it is not immediately clear what
>     a multi-valued url would do in that situation).
>
> Let's try to summarize the current behavior.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Documentation/config/remote.txt | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt
> index 0678b4bcfe..eef0bf4f62 100644
> --- a/Documentation/config/remote.txt
> +++ b/Documentation/config/remote.txt
> @@ -5,10 +5,16 @@ remote.pushDefault::
>
>  remote.<name>.url::
>         The URL of a remote repository.  See linkgit:git-fetch[1] or
> -       linkgit:git-push[1].
> +       linkgit:git-push[1]. A configured remote can have multiple URLs;
> +       in this case the first is used for fetching, and all are used
> +       for pushing (assuming no `remote.<name>.pushurl` is defined).
>
>  remote.<name>.pushurl::
>         The push URL of a remote repository.  See linkgit:git-push[1].
> +       If a `pushurl` option is present in a configured remote, it
> +       is used for pushing instead of `remote.<name>.url`. A configured
> +       remote can have multiple push URLs; in this case a push goes to
> +       all of them.
>
>  remote.<name>.proxy::
>         For remotes that require curl (http, https and ftp), the URL to
> --
> 2.45.2.937.g0bcb3c087a

Jeff King· Jun 14, 2024, 10:31 UTC · re: Jeff King · lore

[PATCH 07/11] remote: allow resetting url list

  git -c remote.origin.url=right fetch

Reported-by: Mathew George <mathewegeorge@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
By the way, I think the nearby remote.*.fetch and remote.*.push could
learn the same trick. I left that out of this series, mostly because it
was getting long. But also because I had trouble imagining how a one-off
refspec change would be useful. We can revisit it on top if we want.

 Documentation/config/remote.txt |  5 ++++-
 remote.c                        | 10 +++++++--
 t/t5505-remote.sh               | 36 +++++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt
index eef0bf4f62..8efc53e836 100644
--- a/Documentation/config/remote.txt
+++ b/Documentation/config/remote.txt
@@ -8,13 +8,16 @@ remote.<name>.url::
 	linkgit:git-push[1]. A configured remote can have multiple URLs;
 	in this case the first is used for fetching, and all are used
 	for pushing (assuming no `remote.<name>.pushurl` is defined).
+	Setting this key to the empty string clears the list of urls,
+	allowing you to override earlier config.
 
 remote.<name>.pushurl::
 	The push URL of a remote repository.  See linkgit:git-push[1].
 	If a `pushurl` option is present in a configured remote, it
 	is used for pushing instead of `remote.<name>.url`. A configured
 	remote can have multiple push URLs; in this case a push goes to
-	all of them.
+	all of them. Setting this key to the empty string clears the
+	list of urls, allowing you to override earlier config.
 
 remote.<name>.proxy::
 	For remotes that require curl (http, https and ftp), the URL to
diff --git a/remote.c b/remote.c
index 9417d83e51..b7262964fb 100644
--- a/remote.c
+++ b/remote.c
@@ -63,12 +63,18 @@ static char *alias_url(const char *url, struct rewrites *r)
 
 static void add_url(struct remote *remote, const char *url)
 {
-	strvec_push(&remote->url, url);
+	if (*url)
+		strvec_push(&remote->url, url);
+	else
+		strvec_clear(&remote->url);
 }
 
 static void add_pushurl(struct remote *remote, const char *pushurl)
 {
-	strvec_push(&remote->pushurl, pushurl);
+	if (*pushurl)
+		strvec_push(&remote->pushurl, pushurl);
+	else
+		strvec_clear(&remote->pushurl);
 }
 
 static void add_pushurl_alias(struct remote_state *remote_state,
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 7789ff12c4..08424e878e 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -1492,4 +1492,40 @@ test_expect_success 'refs/remotes/* <src> refspec and unqualified <dst> DWIM and
 	)
 '
 
+test_expect_success 'empty config clears remote.*.url list' '
+	test_when_finished "git config --remove-section remote.multi" &&
+	git config --add remote.multi.url wrong-one &&
+	git config --add remote.multi.url wrong-two &&
+	git -c remote.multi.url= \
+	    -c remote.multi.url=right-one \
+	    -c remote.multi.url=right-two \
+	    remote show -n multi >actual.raw &&
+	grep URL actual.raw >actual &&
+	cat >expect <<-\EOF &&
+	  Fetch URL: right-one
+	  Push  URL: right-one
+	  Push  URL: right-two
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'empty config clears remote.*.pushurl list' '
+	test_when_finished "git config --remove-section remote.multi" &&
+	git config --add remote.multi.url right &&
+	git config --add remote.multi.url will-be-ignored &&
+	git config --add remote.multi.pushurl wrong-push-one &&
+	git config --add remote.multi.pushurl wrong-push-two &&
+	git -c remote.multi.pushurl= \
+	    -c remote.multi.pushurl=right-push-one \
+	    -c remote.multi.pushurl=right-push-two \
+	    remote show -n multi >actual.raw &&
+	grep URL actual.raw >actual &&
+	cat >expect <<-\EOF &&
+	  Fetch URL: right
+	  Push  URL: right-push-one
+	  Push  URL: right-push-two
+	EOF
+	test_cmp expect actual
+'
+
 test_done

-- 
2.45.2.937.g0bcb3c087a

Elijah Newren· Jun 25, 2024, 17:35 UTC · re: Jeff King · lore

Re: [PATCH 07/11] remote: allow resetting url list

On Fri, Jun 14, 2024 at 3:32 AM Jeff King <peff@peff.net> wrote:

>
> Because remote.*.url is treated as a multi-valued key, there is no way
> to override previous config. So for example if you have
> remote.origin.url set to some wrong value, doing:
>
>   git -c remote.origin.url=right fetch
>
> would not work. It would append "right" to the list, which means we'd
> still fetch from "wrong" (since subsequent values are used only as push
> urls).
>
> Let's provide a mechanism to reset the list, like we do for other
> multi-valued keys (e.g., credential.helper, http.extraheaders, and
> merge.suppressDest all use this "empty string means reset" pattern).
>
> Reported-by: Mathew George <mathewegeorge@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> By the way, I think the nearby remote.*.fetch and remote.*.push could
> learn the same trick. I left that out of this series, mostly because it
> was getting long. But also because I had trouble imagining how a one-off
> refspec change would be useful. We can revisit it on top if we want.

Yeah, deferring makes sense to me.

>  Documentation/config/remote.txt |  5 ++++-
>  remote.c                        | 10 +++++++--
>  t/t5505-remote.sh               | 36 +++++++++++++++++++++++++++++++++
>  3 files changed, 48 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt
> index eef0bf4f62..8efc53e836 100644
> --- a/Documentation/config/remote.txt
> +++ b/Documentation/config/remote.txt
> @@ -8,13 +8,16 @@ remote.<name>.url::
>         linkgit:git-push[1]. A configured remote can have multiple URLs;
>         in this case the first is used for fetching, and all are used
>         for pushing (assuming no `remote.<name>.pushurl` is defined).
> +       Setting this key to the empty string clears the list of urls,
> +       allowing you to override earlier config.
>
>  remote.<name>.pushurl::
>         The push URL of a remote repository.  See linkgit:git-push[1].
>         If a `pushurl` option is present in a configured remote, it
>         is used for pushing instead of `remote.<name>.url`. A configured
>         remote can have multiple push URLs; in this case a push goes to
> -       all of them.
> +       all of them. Setting this key to the empty string clears the
> +       list of urls, allowing you to override earlier config.
>
>  remote.<name>.proxy::
>         For remotes that require curl (http, https and ftp), the URL to

Thanks for documenting this too.

> diff --git a/remote.c b/remote.c
> index 9417d83e51..b7262964fb 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -63,12 +63,18 @@ static char *alias_url(const char *url, struct rewrites *r)
>
>  static void add_url(struct remote *remote, const char *url)
>  {
> -       strvec_push(&remote->url, url);
> +       if (*url)
> +               strvec_push(&remote->url, url);
> +       else
> +               strvec_clear(&remote->url);
>  }
>
>  static void add_pushurl(struct remote *remote, const char *pushurl)
>  {
> -       strvec_push(&remote->pushurl, pushurl);
> +       if (*pushurl)
> +               strvec_push(&remote->pushurl, pushurl);
> +       else
> +               strvec_clear(&remote->pushurl);
>  }

Nice that after the preparation patches the fix is nice and simple.

>  static void add_pushurl_alias(struct remote_state *remote_state,
> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index 7789ff12c4..08424e878e 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -1492,4 +1492,40 @@ test_expect_success 'refs/remotes/* <src> refspec and unqualified <dst> DWIM and
>         )
>  '
>
> +test_expect_success 'empty config clears remote.*.url list' '
> +       test_when_finished "git config --remove-section remote.multi" &&
> +       git config --add remote.multi.url wrong-one &&
> +       git config --add remote.multi.url wrong-two &&
> +       git -c remote.multi.url= \
> +           -c remote.multi.url=right-one \
> +           -c remote.multi.url=right-two \
> +           remote show -n multi >actual.raw &&
> +       grep URL actual.raw >actual &&
> +       cat >expect <<-\EOF &&
> +         Fetch URL: right-one
> +         Push  URL: right-one
> +         Push  URL: right-two
> +       EOF
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'empty config clears remote.*.pushurl list' '
> +       test_when_finished "git config --remove-section remote.multi" &&
> +       git config --add remote.multi.url right &&
> +       git config --add remote.multi.url will-be-ignored &&
> +       git config --add remote.multi.pushurl wrong-push-one &&
> +       git config --add remote.multi.pushurl wrong-push-two &&
> +       git -c remote.multi.pushurl= \
> +           -c remote.multi.pushurl=right-push-one \
> +           -c remote.multi.pushurl=right-push-two \
> +           remote show -n multi >actual.raw &&
> +       grep URL actual.raw >actual &&
> +       cat >expect <<-\EOF &&
> +         Fetch URL: right
> +         Push  URL: right-push-one
> +         Push  URL: right-push-two
> +       EOF
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.45.2.937.g0bcb3c087a

Make sense.

Jeff King· Jun 14, 2024, 10:31 UTC · re: Jeff King · lore

[PATCH 08/11] t5801: make remote-testgit GIT_DIR setup more robust

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5801/git-remote-testgit | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit
index c5b10f5775..f8b476499f 100755
--- a/t/t5801/git-remote-testgit
+++ b/t/t5801/git-remote-testgit
@@ -26,7 +26,8 @@ then
 	t_refspec=""
 fi
 
-GIT_DIR="$url/.git"
+unset $(git rev-parse --local-env-vars)
+GIT_DIR=$(git -C "$url" rev-parse --absolute-git-dir)
 export GIT_DIR
 
 force=

-- 
2.45.2.937.g0bcb3c087a

Elijah Newren· Jun 25, 2024, 17:36 UTC · re: Jeff King · lore

Re: [PATCH 08/11] t5801: make remote-testgit GIT_DIR setup more robust

On Fri, Jun 14, 2024 at 3:32 AM Jeff King <peff@peff.net> wrote:

>
> Our tests use a fake helper that just imports from an existing Git
> repository. We're fed the path to that repo on the command line, and
> derive the GIT_DIR by tacking on "/.git".
>
> This is wrong if the path is a bare repository, but that's OK since this
> is just a limited test. But it's also wrong if the transport code feeds
> us the actual .git directory itself (i.e., we expect "/path/to/repo" but
> it gives us "/path/to/repo/.git"). None of the current tests do that,
> but let's future-proof ourselves against adding a test that does.
>
> We can instead ask "rev-parse" to set our GIT_DIR. Note that we have to
> first unset other git variables from our environment. Coming into this
> script, we'll have GIT_DIR set to the fetching repository, and we need
> to "switch" to the remote one.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t5801/git-remote-testgit | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit
> index c5b10f5775..f8b476499f 100755
> --- a/t/t5801/git-remote-testgit
> +++ b/t/t5801/git-remote-testgit
> @@ -26,7 +26,8 @@ then
>         t_refspec=""
>  fi
>
> -GIT_DIR="$url/.git"
> +unset $(git rev-parse --local-env-vars)

Ooh, I somehow didn't know about that flag before.  TIL.

> +GIT_DIR=$(git -C "$url" rev-parse --absolute-git-dir)
>  export GIT_DIR
>
>  force=
> --
> 2.45.2.937.g0bcb3c087a

Makes sense.

Jeff King· Jun 14, 2024, 10:34 UTC · re: Jeff King · lore

[PATCH 09/11] t5801: test remote.*.vcs config

  git config remote.origin.url hg::https://example.com/repo

  git config remote.origin.url "foo://bar"

which will run "git-remote-foo origin foo://bar".

  git config remote.origin.vcs foo
  git config remote.origin.url bar

  git config remote.origin.vcs foo

  git -c remote.foo.vcs=bar fetch foo

  - this syntax doesn't work well with "git clone" (another point
    against it versus "helper::"). But we can use "clone -c" to set up
    the config manually, passing the URL as usual to clone. This does
    work, though note that I had to use --no-local in the test to avoid
    broken interactions between the local code and the helper. In the
    real world this would be a non-issue, since the remote URL would
    generally not also be a local Git repo!

  - likewise, we should be able to set up the config manually and fetch
    into a repository. This also works.

  - we can simulate a vcs that has no URL support by stuffing the remote
    path into another environment variable. This should work, but
    doesn't (it hits the segfault mentioned above).

Signed-off-by: Jeff King <peff@peff.net>
---
I have no real problem with deprecating this, and it might be a nice
thing to clean up in the long run. But it seemed like less work to just
fix it in the next patch, so I did that. ;)

 t/t5801-remote-helpers.sh | 23 +++++++++++++++++++++++
 t/t5801/git-remote-nourl  |  3 +++
 2 files changed, 26 insertions(+)
 create mode 100755 t/t5801/git-remote-nourl

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 4e0a77f985..7c8c4359aa 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -38,6 +38,29 @@ test_expect_success 'cloning from local repo' '
 	test_cmp server/file local/file
 '
 
+test_expect_success 'clone with remote.*.vcs config' '
+	GIT_TRACE=$PWD/vcs-clone.trace \
+	git clone --no-local -c remote.origin.vcs=testgit "$PWD/server" vcs-clone &&
+	test_grep remote-testgit vcs-clone.trace
+'
+
+test_expect_success 'fetch with configured remote.*.vcs' '
+	git init vcs-fetch &&
+	git -C vcs-fetch config remote.origin.vcs testgit &&
+	git -C vcs-fetch config remote.origin.url "$PWD/server" &&
+	GIT_TRACE=$PWD/vcs-fetch.trace \
+	git -C vcs-fetch fetch origin &&
+	test_grep remote-testgit vcs-fetch.trace
+'
+
+test_expect_failure 'vcs remote with no url' '
+	NOURL_UPSTREAM=$PWD/server &&
+	export NOURL_UPSTREAM &&
+	git init vcs-nourl &&
+	git -C vcs-nourl config remote.origin.vcs nourl &&
+	git -C vcs-nourl fetch origin
+'
+
 test_expect_success 'create new commit on remote' '
 	(cd server &&
 	 echo content >>file &&
diff --git a/t/t5801/git-remote-nourl b/t/t5801/git-remote-nourl
new file mode 100755
index 0000000000..09be6013c5
--- /dev/null
+++ b/t/t5801/git-remote-nourl
@@ -0,0 +1,3 @@
+#!/bin/sh
+
+exec git-remote-testgit "$1" "$NOURL_UPSTREAM"

-- 
2.45.2.937.g0bcb3c087a

Jeff King· Jun 14, 2024, 10:37 UTC · re: Jeff King · lore

[PATCH 10/11] remote: always require at least one url in a remote

        if (name_given && !valid_remote(ret))
                add_url_alias(remote_state, ret, name);
        if (!valid_remote(ret))
                return NULL;

  git -c remote.foo.vcs=bar fetch foo

   # only one command-line option!
   git-remote-bar foo

whereas with our default url, now we'll run:

  git-remote-bar foo foo

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c                  | 2 +-
 t/t5801-remote-helpers.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index b7262964fb..5fa046c8f8 100644
--- a/remote.c
+++ b/remote.c
@@ -32,7 +32,7 @@ struct counted_string {
 
 static int valid_remote(const struct remote *remote)
 {
-	return (!!remote->url.nr) || (!!remote->foreign_vcs);
+	return !!remote->url.nr;
 }
 
 static char *alias_url(const char *url, struct rewrites *r)
diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 7c8c4359aa..20f43f7b7d 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -53,7 +53,7 @@ test_expect_success 'fetch with configured remote.*.vcs' '
 	test_grep remote-testgit vcs-fetch.trace
 '
 
-test_expect_failure 'vcs remote with no url' '
+test_expect_success 'vcs remote with no url' '
 	NOURL_UPSTREAM=$PWD/server &&
 	export NOURL_UPSTREAM &&
 	git init vcs-nourl &&

-- 
2.45.2.937.g0bcb3c087a

Jeff King· Jun 14, 2024, 10:42 UTC · re: Jeff King · lore

[PATCH 11/11] remote: drop checks for zero-url case

Signed-off-by: Jeff King <peff@peff.net>
---
Note for reviewers: the hunk in builtin/push.c is funny. The original
code was:

  if (url->nr) {
	for (i = 0; i < url->nr; i++) {
		do this
	}
  } else {
    do that
  }

And I removed the "do that" part along with the if/else to become:

  for (i = 0; i < url->nr; i++) {
	do this
  }

 builtin/archive.c          |  2 --
 builtin/ls-remote.c        |  2 --
 builtin/push.c             | 13 ++-----------
 builtin/remote.c           | 13 +++----------
 t/helper/test-bundle-uri.c |  2 --
 transport.c                | 17 +++++++----------
 6 files changed, 12 insertions(+), 37 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index 0d9aff7e6f..7234607aaa 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -31,8 +31,6 @@ static int run_remote_archiver(int argc, const char **argv,
 	struct packet_reader reader;
 
 	_remote = remote_get(remote);
-	if (!_remote->url.nr)
-		die(_("git archive: Remote with no URL"));
 	transport = transport_get(_remote, _remote->url.v[0]);
 	transport_connect(transport, "git-upload-archive", exec, fd);
 
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 4c04dbbf19..8f3a64d838 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -109,8 +109,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 			die("bad repository '%s'", dest);
 		die("No remote configured to list refs from.");
 	}
-	if (!remote->url.nr)
-		die("remote %s has no configured URL", dest);
 
 	if (get_url) {
 		printf("%s\n", remote->url.v[0]);
diff --git a/builtin/push.c b/builtin/push.c
index 00d99af1a8..8260c6e46a 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -438,18 +438,9 @@ static int do_push(int flags,
 	}
 	errs = 0;
 	url = push_url_of_remote(remote);
-	if (url->nr) {
-		for (i = 0; i < url->nr; i++) {
-			struct transport *transport =
-				transport_get(remote, url->v[i]);
-			if (flags & TRANSPORT_PUSH_OPTIONS)
-				transport->push_options = push_options;
-			if (push_with_options(transport, push_refspec, flags))
-				errs++;
-		}
-	} else {
+	for (i = 0; i < url->nr; i++) {
 		struct transport *transport =
-			transport_get(remote, NULL);
+			transport_get(remote, url->v[i]);
 		if (flags & TRANSPORT_PUSH_OPTIONS)
 			transport->push_options = push_options;
 		if (push_with_options(transport, push_refspec, flags))
diff --git a/builtin/remote.c b/builtin/remote.c
index 06c09ed060..c5c94d4dbd 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1002,8 +1002,7 @@ static int get_remote_ref_states(const char *name,
 		struct transport *transport;
 		const struct ref *remote_refs;
 
-		transport = transport_get(states->remote, states->remote->url.nr > 0 ?
-			states->remote->url.v[0] : NULL);
+		transport = transport_get(states->remote, states->remote->url.v[0]);
 		remote_refs = transport_get_remote_refs(transport, NULL);
 
 		states->queried = 1;
@@ -1294,8 +1293,7 @@ static int show(int argc, const char **argv, const char *prefix)
 		get_remote_ref_states(*argv, &info.states, query_flag);
 
 		printf_ln(_("* remote %s"), *argv);
-		printf_ln(_("  Fetch URL: %s"), info.states.remote->url.nr > 0 ?
-		       info.states.remote->url.v[0] : _("(no URL)"));
+		printf_ln(_("  Fetch URL: %s"), info.states.remote->url.v[0]);
 		url = push_url_of_remote(info.states.remote);
 		for (i = 0; i < url->nr; i++)
 			/*
@@ -1440,10 +1438,7 @@ static int prune_remote(const char *remote, int dry_run)
 	}
 
 	printf_ln(_("Pruning %s"), remote);
-	printf_ln(_("URL: %s"),
-		  states.remote->url.nr
-		  ? states.remote->url.v[0]
-		  : _("(no URL)"));
+	printf_ln(_("URL: %s"), states.remote->url.v[0]);
 
 	for_each_string_list_item(item, &states.stale)
 		string_list_append(&refs_to_prune, item->util);
@@ -1632,8 +1627,6 @@ static int get_url(int argc, const char **argv, const char *prefix)
 	}
 
 	url = push_mode ? push_url_of_remote(remote) : &remote->url;
-	if (!url->nr)
-		die(_("no URLs configured for remote '%s'"), remotename);
 
 	if (all_mode) {
 		for (i = 0; i < url->nr; i++)
diff --git a/t/helper/test-bundle-uri.c b/t/helper/test-bundle-uri.c
index 3285dd962e..0c5fa723d8 100644
--- a/t/helper/test-bundle-uri.c
+++ b/t/helper/test-bundle-uri.c
@@ -88,8 +88,6 @@ static int cmd_ls_remote(int argc, const char **argv)
 			die(_("bad repository '%s'"), dest);
 		die(_("no remote configured to get bundle URIs from"));
 	}
-	if (!remote->url.nr)
-		die(_("remote '%s' has no configured URL"), dest);
 
 	transport = transport_get(remote, NULL);
 	if (transport_get_remote_bundle_uri(transport) < 0) {
diff --git a/transport.c b/transport.c
index eba92eb7e0..a324045240 100644
--- a/transport.c
+++ b/transport.c
@@ -1112,6 +1112,7 @@ static struct transport_vtable builtin_smart_vtable = {
 struct transport *transport_get(struct remote *remote, const char *url)
 {
 	const char *helper;
+	const char *p;
 	struct transport *ret = xcalloc(1, sizeof(*ret));
 
 	ret->progress = isatty(2);
@@ -1127,19 +1128,15 @@ struct transport *transport_get(struct remote *remote, const char *url)
 	ret->remote = remote;
 	helper = remote->foreign_vcs;
 
-	if (!url && remote->url.nr)
+	if (!url)
 		url = remote->url.v[0];
 	ret->url = url;
 
-	/* maybe it is a foreign URL? */
-	if (url) {
-		const char *p = url;
-
-		while (is_urlschemechar(p == url, *p))
-			p++;
-		if (starts_with(p, "::"))
-			helper = xstrndup(url, p - url);
-	}
+	p = url;
+	while (is_urlschemechar(p == url, *p))
+		p++;
+	if (starts_with(p, "::"))
+		helper = xstrndup(url, p - url);
 
 	if (helper) {
 		transport_helper_init(ret, helper);

-- 
2.45.2.937.g0bcb3c087a

Elijah Newren· Jun 25, 2024, 17:37 UTC · re: Jeff King · lore

Re: [PATCH 11/11] remote: drop checks for zero-url case

On Fri, Jun 14, 2024 at 4:12 AM Jeff King <peff@peff.net> wrote:

>
> Now that the previous commit removed the possibility that a "struct
> remote" will ever have zero url fields, we can drop a number of
> redundant checks and untriggerable code paths.

Ooh, nice.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Note for reviewers: the hunk in builtin/push.c is funny. The original
> code was:
>
>   if (url->nr) {
>         for (i = 0; i < url->nr; i++) {
>                 do this
>         }
>   } else {
>     do that
>   }
>
> And I removed the "do that" part along with the if/else to become:
>
>   for (i = 0; i < url->nr; i++) {
>         do this
>   }
>
>
> But because "this" and "that" were so similar, and because the
> indentation of "this" in the loop was now the same of the old "that",
> the diff makes it look like I dropped the first half of the conditional.

Thanks for adding this comment; was very helpful while reviewing.

>  builtin/archive.c          |  2 --
>  builtin/ls-remote.c        |  2 --
>  builtin/push.c             | 13 ++-----------
>  builtin/remote.c           | 13 +++----------
>  t/helper/test-bundle-uri.c |  2 --
>  transport.c                | 17 +++++++----------
>  6 files changed, 12 insertions(+), 37 deletions(-)
>
> diff --git a/builtin/archive.c b/builtin/archive.c
> index 0d9aff7e6f..7234607aaa 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -31,8 +31,6 @@ static int run_remote_archiver(int argc, const char **argv,
>         struct packet_reader reader;
>
>         _remote = remote_get(remote);
> -       if (!_remote->url.nr)
> -               die(_("git archive: Remote with no URL"));
>         transport = transport_get(_remote, _remote->url.v[0]);
>         transport_connect(transport, "git-upload-archive", exec, fd);
>
> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
> index 4c04dbbf19..8f3a64d838 100644
> --- a/builtin/ls-remote.c
> +++ b/builtin/ls-remote.c
> @@ -109,8 +109,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>                         die("bad repository '%s'", dest);
>                 die("No remote configured to list refs from.");
>         }
> -       if (!remote->url.nr)
> -               die("remote %s has no configured URL", dest);
>
>         if (get_url) {
>                 printf("%s\n", remote->url.v[0]);
> diff --git a/builtin/push.c b/builtin/push.c
> index 00d99af1a8..8260c6e46a 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -438,18 +438,9 @@ static int do_push(int flags,
>         }
>         errs = 0;
>         url = push_url_of_remote(remote);
> -       if (url->nr) {
> -               for (i = 0; i < url->nr; i++) {
> -                       struct transport *transport =
> -                               transport_get(remote, url->v[i]);
> -                       if (flags & TRANSPORT_PUSH_OPTIONS)
> -                               transport->push_options = push_options;
> -                       if (push_with_options(transport, push_refspec, flags))
> -                               errs++;
> -               }
> -       } else {
> +       for (i = 0; i < url->nr; i++) {
>                 struct transport *transport =
> -                       transport_get(remote, NULL);
> +                       transport_get(remote, url->v[i]);
>                 if (flags & TRANSPORT_PUSH_OPTIONS)
>                         transport->push_options = push_options;
>                 if (push_with_options(transport, push_refspec, flags))
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 06c09ed060..c5c94d4dbd 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -1002,8 +1002,7 @@ static int get_remote_ref_states(const char *name,
>                 struct transport *transport;
>                 const struct ref *remote_refs;
>
> -               transport = transport_get(states->remote, states->remote->url.nr > 0 ?
> -                       states->remote->url.v[0] : NULL);
> +               transport = transport_get(states->remote, states->remote->url.v[0]);
>                 remote_refs = transport_get_remote_refs(transport, NULL);
>
>                 states->queried = 1;
> @@ -1294,8 +1293,7 @@ static int show(int argc, const char **argv, const char *prefix)
>                 get_remote_ref_states(*argv, &info.states, query_flag);
>
>                 printf_ln(_("* remote %s"), *argv);
> -               printf_ln(_("  Fetch URL: %s"), info.states.remote->url.nr > 0 ?
> -                      info.states.remote->url.v[0] : _("(no URL)"));
> +               printf_ln(_("  Fetch URL: %s"), info.states.remote->url.v[0]);
>                 url = push_url_of_remote(info.states.remote);
>                 for (i = 0; i < url->nr; i++)
>                         /*
> @@ -1440,10 +1438,7 @@ static int prune_remote(const char *remote, int dry_run)
>         }
>
>         printf_ln(_("Pruning %s"), remote);
> -       printf_ln(_("URL: %s"),
> -                 states.remote->url.nr
> -                 ? states.remote->url.v[0]
> -                 : _("(no URL)"));
> +       printf_ln(_("URL: %s"), states.remote->url.v[0]);
>
>         for_each_string_list_item(item, &states.stale)
>                 string_list_append(&refs_to_prune, item->util);
> @@ -1632,8 +1627,6 @@ static int get_url(int argc, const char **argv, const char *prefix)
>         }
>
>         url = push_mode ? push_url_of_remote(remote) : &remote->url;
> -       if (!url->nr)
> -               die(_("no URLs configured for remote '%s'"), remotename);
>
>         if (all_mode) {
>                 for (i = 0; i < url->nr; i++)
> diff --git a/t/helper/test-bundle-uri.c b/t/helper/test-bundle-uri.c
> index 3285dd962e..0c5fa723d8 100644
> --- a/t/helper/test-bundle-uri.c
> +++ b/t/helper/test-bundle-uri.c
> @@ -88,8 +88,6 @@ static int cmd_ls_remote(int argc, const char **argv)
>                         die(_("bad repository '%s'"), dest);
>                 die(_("no remote configured to get bundle URIs from"));
>         }
> -       if (!remote->url.nr)
> -               die(_("remote '%s' has no configured URL"), dest);
>
>         transport = transport_get(remote, NULL);
>         if (transport_get_remote_bundle_uri(transport) < 0) {
> diff --git a/transport.c b/transport.c
> index eba92eb7e0..a324045240 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1112,6 +1112,7 @@ static struct transport_vtable builtin_smart_vtable = {
>  struct transport *transport_get(struct remote *remote, const char *url)
>  {
>         const char *helper;
> +       const char *p;
>         struct transport *ret = xcalloc(1, sizeof(*ret));
>
>         ret->progress = isatty(2);
> @@ -1127,19 +1128,15 @@ struct transport *transport_get(struct remote *remote, const char *url)
>         ret->remote = remote;
>         helper = remote->foreign_vcs;
>
> -       if (!url && remote->url.nr)
> +       if (!url)
>                 url = remote->url.v[0];
>         ret->url = url;
>
> -       /* maybe it is a foreign URL? */
> -       if (url) {
> -               const char *p = url;
> -
> -               while (is_urlschemechar(p == url, *p))
> -                       p++;
> -               if (starts_with(p, "::"))
> -                       helper = xstrndup(url, p - url);
> -       }
> +       p = url;
> +       while (is_urlschemechar(p == url, *p))
> +               p++;
> +       if (starts_with(p, "::"))
> +               helper = xstrndup(url, p - url);
>
>         if (helper) {
>                 transport_helper_init(ret, helper);
> --
> 2.45.2.937.g0bcb3c087a

Elijah Newren· Jun 25, 2024, 17:44 UTC · re: Jeff King · lore

Re: [PATCH 0/11] allow overriding remote.*.url

On Fri, Jun 14, 2024 at 3:25 AM Jeff King <peff@peff.net> wrote:

>
> On Thu, Jun 13, 2024 at 06:24:09AM -0400, Jeff King wrote:
>
> > > I was expecting (with excitement) a mess, but the above is as clean
> > > as we can make the idea, I would say.  Lack of documentation and
> > > tests do count as incompleteness though of course.
> >
> > Yeah, and we should probably do the same for pushurl. And I think there
> > could be some cleanup of the memory ownership handling of add_url().
>
> So as always with this crufty 2009-era code, there turned out to be some
> subtleties. ;)
>
> The good news is that I think dealing with them left the code in a
> better place. It's easier to reason about, and a few possible leaks have
> been plugged (I don't know if they were triggered in the test suite or
> not; if so they weren't enough to tip any scripts over to being
> leak-free).

I agree with this good news after reviewing the series.

> We can split the series into segments:
>
>   [01/11]: archive: fix check for missing url
>
>     A nearby trivial bugfix.
>
>   [02/11]: remote: refactor alias_url() memory ownership
>   [03/11]: remote: transfer ownership of memory in add_url(), etc
>   [04/11]: remote: use strvecs to store remote url/pushurl
>   [05/11]: remote: simplify url/pushurl selection
>
>     Fixing memory handling weirdness, which is a necessary prereq for
>     the "reset" operation to avoid leaking. The switch to using a strvec
>     isn't strictly necessary, but it does make the code (including the
>     later patch 7) simpler.
>
>   [06/11]: config: document remote.*.url/pushurl interaction
>   [07/11]: remote: allow resetting url list
>
>     The actual change is in patch 7 here, but it was hard to add new
>     docs to the rather anemic existing ones. Hence patch 6.
>
>   [08/11]: t5801: make remote-testgit GIT_DIR setup more robust
>   [09/11]: t5801: test remote.*.vcs config
>   [10/11]: remote: always require at least one url in a remote
>   [11/11]: remote: drop checks for zero-url case
>
>     This is a related cleanup I found while working in the area.
>     Arguably it could be a separate topic, though it does depend
>     textually on what came before.

Jeff King· Jun 26, 2024, 20:40 UTC · re: Elijah Newren · lore

Re: [PATCH 0/11] allow overriding remote.*.url

On Tue, Jun 25, 2024 at 10:44:03AM -0700, Elijah Newren wrote:

> I only managed to find a few typos in commit messages, but I looked
> through patches 1-8 pretty closely.  I only skimmed 9 & 10 -- I don't
> really have an opinion on the remote helpers.  I agree that the issue
> you bring up in the patches makes sense to discuss, and the route you
> picked looks reasonable to me, but I don't feel motivated to try to
> use or understand the remote helpers enough to form an opinion.
> However, I'm a fan of the cleanup in patch 11 that your changes in 9 &
> 10 enabled, so if everyone's as ambivalent as me (and 15 years of
> things being broken suggests everyone is likely to be as ambivalent as
> me) then I'd say just go with your changes in 9 & 10 and call it a
> day.

-Peff