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

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

[Mathew George]

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

```
# Create a test repo to pull into
git init test
cd $_
git remote add origin wrong-url
# Create a test repo to pull from
git init ../test2
cd $_
touch a
git add .
git commit -m 'commit to pull'
# Attempt to pull, setting remote.origin.url
# The following git commands will fail:
cd ../test
git -c remote.origin.url=../test2 pull
git -c remote.origin.url=../test2 fetch
# However, this succeeds:
git pull ../test2
```


> 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
```

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)

The following errors are produced for both 'fetch' and 'pull'
operations:

```
fatal: 'wrong-url' does not appear to be a git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
```

Regardless of the -c option, the commands above use the url
'wrong-url' from .git/config.


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

The -c option is not respected.


> Anything else you want to add:

It may be that this is not supported, but if that's the case, I
missed any documentation saying so.


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


[System Info]
git version:
git version 2.45.2
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /usr/bin/sh
compiler info: clang: 17.0.2 (https://android.googlesource.com/toolchain/llvm-project d9f89f4d16663d5012e5c09495f3b30ece3d2362)
libc info: no libc information available
$SHELL (typically, interactive shell): /usr/bin/bash


[Enabled Hooks]
not run from a git repository - no hooks to show

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

[Jeff King]

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

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

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

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

[Junio C Hamano]

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

It came from days back when there weren't a separate push URL IIRC.
You may pull from a central place like everybody else, but you may
have other publishing points of your work.

Place to fetch must be a single known stable place for obvious
reasons.  "I have N, so I'll pull from one chosen at random among
these N" would lead to madness ;-).

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

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

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.

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

[Jeff King]

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.

No, you're right. I was just misremembering (maybe because "--no-foo" is
the command-line way to reset the "foo" list). The three instances of
config I found (merge.suppressdest, http.extraheaders, and
credential.helper) all treat the empty string that way.

The multi-valued boolean thing would cover config keys where an empty
string was semantically meaningful, but I wouldn't be surprised if we
never run across such a case. So probably using "empty string is reset"
as the convention is fine, and matches existing cases.

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

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

-Peff

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

[Jeff King]

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

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

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

[Jeff King]

Running "git archive --remote" checks that we have at least one url for
the remote. It does so by looking at remote.url[0], but that won't work;
if we have no url at all, then remote.url will be NULL, and we'll
segfault.

Check url_nr instead, which is a more direct way of asking what we
want.

You can trigger the segfault like this:

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

but I didn't bother adding a test. This is the tip of the iceberg for
no-url remotes, and a later patch will improve that situation. I just
wanted to clean up this bug so it didn't make further refactoring of
this code more confusing.

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

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

[Jeff King]

The alias_url() function may return either a newly allocated string
(which the caller must take ownership of), or the original const "url"
parameter that was passed in.

This often works OK because callers are generally passing in a "url"
that they expect to retain ownership of anyway. So whether we got back
the original or a new string, we're always interested in storing it
forever. But I suspect there are some possible leaks here (e.g.,
add_url_alias() may end up discarding the original "url").

Whether there are active leaks or not, this is a confusing setup that
makes further refactoring of memory ownership harder. 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.

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

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

[Junio C Hamano]

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.

Looking at how remote_clear() deals with the .url[] and .pushurl[]
elements, add_url() makes the remote structure take ownership, which
is perfectly fine when we got a non-NULL alias back (i.e. it is a
new piece of string allocated just for us).  Depending on who owns
the incoming url parameter, we might need strdup(url) here, but
since we haven't heard crashes due to freeing remote->url[] elements
that should not be freed, presumably url is a piece memory the
caller is giving us the ownership?  In any case, I imagine that
untangling that ownership mess is left to the later steps of the
series.

> @@ -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;

Does this change behaviour?  No.  We used to (1) grab the current
value, (2) replace it if it is an alias, or (3) otherwise replace it
with itself.  We just do not do the obvious noop anymore.

The story is the same with the last remaing hunk of this patch.
Looking good.

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;
>  		}
>  	}
>  }

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

[Jeff King]

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

Instead, let's have remote->url and remote->pushurl own their string
memory. They'll copy the const strings that are passed in, and callers
can stop making their own copies. Likewise, when we overwrite an entry,
we can free the memory it points to, fixing the leak mentioned above.

We'll leave the struct members as "const" since they are visible to the
outside world, and shouldn't usually be touched. This requires casting
on free() for now, but we'll clean that further in a future patch.

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

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

[Junio C Hamano]

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);
>  }

OK.  This makes it easier to reason about why these elements are
freed in remote_clear().

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

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

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.

> @@ -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).

> @@ -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;
> +			}
>  		}

OK, this is the replacement codepath we saw earlier.  Makes sense
for this and the .url[] side on the other hunk (it is curious that
this has .pushurl[] before .url[], which is the opposite order of
how everybody else deals with them, as pushurl came later).

Thanks.

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

[Jeff King]

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. :)

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

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.

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

Yep. If we followed through on the "bool means reset" approach, then these
extra NULL checks would go away in favor of one in add_url(). I didn't
pursue that here. And if we do, I think we should do it in all the other
spots, too, for consistency.

-Peff

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

[Junio C Hamano]

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. :)

No squashing is needed.  It's just that [02/11] could go in either
direction and the reader was held in suspense until [03/11] that
picked one direction to go ;-).

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

Yeah, either way it is correct, and I think the "cleanup at the end,
where the single label is there for any new error code paths to jump
to" pattern is a good approach going forward.

Looking good.

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

[Elijah Newren]

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

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

[Jeff King]

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

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

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

[Elijah Newren]

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(-)
>
[...]

I really like the simplifications this provides in remote.[hc]; the
rest all looks like straightforward translations.

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

[Jeff King]

When we want to know the push urls for a remote, there is some simple
logic:

  - 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

Many spots implement this with a level of indirection, assigning to a
local url/url_nr pair. But since both arrays are now strvecs, we can
just use a pointer to select the appropriate strvec, shortening the code
a bit.

Even though this is now a one-liner, since it is application logic that
is present in so many places, it's worth abstracting a helper function.
In fact, we already have such a function, but it's local to
builtin/push.c. So we'll just make it available everywhere via remote.h.

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

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

[Elijah Newren]

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.

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

[Jeff King]

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

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

[Elijah Newren]

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

I was unaware of these facts prior to reading this series, and I've
read the documentation multiple times and occasionally even glanced at
nearby code.  So, definitely a welcome documentation addition.

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

[Jeff King]

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.

 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

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

[Elijah Newren]

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.

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

[Jeff King]

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)
+GIT_DIR=$(git -C "$url" rev-parse --absolute-git-dir)
 export GIT_DIR
 
 force=
-- 
2.45.2.937.g0bcb3c087a

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

[Elijah Newren]

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.

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

[Jeff King]

The usual way to trigger a remote helper is to use the "::" syntax from:
87422439d1 (Allow specifying the remote helper in the url, 2009-11-18).
Doing:

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

will run "git-remote-hg origin https://example.com/repo". Or you can
use the fallback handling from 25d5cc488a (Pass unknown protocols to
external protocol handlers, 2009-12-09):

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

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

But there's a third way, from c578f51d52 (Add a config option for
remotes to specify a foreign vcs, 2009-11-18):

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

which will run "git-remote-foo origin bar". This is mostly redundant
with the other methods, except that it is supposed to allow you to run
without a URL at all. So:

  git config remote.origin.vcs foo

would run "git-remote-foo origin" with no extra URL parameter (under the
assumption that the helper somehow knows how to access the remote repo).
However, this mode has been broken since 25d5cc488a, shortly after it
was added! That commit taught the transport code to always look at the
URL string to parse off the "foo::" bits, meaning it would always
segfault in the no-url case. You can see that with:

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

Nobody seems to have noticed in the almost 15 years since, so presumably
it's not a well-used feature. And without that, arguably the whole
remote.*.vcs feature could be removed entirely, as it isn't offering
anything you couldn't do with the "helper::" syntax. But it _does_ work
if you have a URL, and it has been advertised in the documentation for
all that time. So we shouldn't just remove it without warning.

Likewise, even if we were going to deprecate it, we should avoid
breaking it in the meantime. Since there are no tests for it at all,
let's add a few basic ones:

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

In the first two cases, I took the extra step of checking GIT_TRACE
output to confirm that we actually ran the helper (since the URL is a
valid Git repo, the clone/fetch would appear to work even if we
didn't use the helper at all!).

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

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

[Jeff King]

When we return a struct from remote_get(), the result _almost_ always
has at least one url. In remotes_remote_get_1(), we do this:

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

So if the remote doesn't have a url, we give it one based on the name
(this is how unconfigured urls are used as remotes). And if that doesn't
work, we return NULL.

But there's a catch: valid_remote() checks that we have at least one url
_unless_ the remote.*.vcs field is set. This comes from c578f51d52 (Add
a config option for remotes to specify a foreign vcs, 2009-11-18), and
the whole idea was to support remote helpers that don't have their own
url.

However, that mode has been broken since 25d5cc488a (Pass unknown
protocols to external protocol handlers, 2009-12-09)! That commit
unconditionally looks at the url in get_helper(), causing a segfault
with something like:

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

We could fix that now, of course. But given that it has been broken for
almost 15 years and nobody noticed, there's a better option. This weird
"there might not be a url" special case requires checks all over the
code base, and it's not clear if there are other similar segfaults
lurking. It would be nice if we could drop that special case.

So instead, let's let the "the remote name is the url" code kick in. If
you have "remote.foo.vcs", then your url (unless otherwise configured)
is "foo". This does have a visible effect compared to what 25d5cc488a
was trying to do. The idea back then is that for a remote without a url,
we'd run:

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

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

  git-remote-bar foo foo

Again, in practice nobody can be relying on this because it has been
segfaulting for 15 years. We should consider just removing this "vcs"
config option entirely, but that would be a user-visible breakage. So by
fixing it this way, we can keep things working that have been working,
and simplify away one special case inside our code.

This fixes the segfault from 25d5cc488a (demonstrated by the test), and
we can build further cleanups on top.

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

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

[Jeff King]

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.

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.

 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

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

[Elijah Newren]

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

So, after reading the first 8 patches, I only kinda skimmed 9 and 10,
thinking "I don't really care about these remote helper things and
don't have an opinion on them -- besides, I'm sure Peff is picking
something reasonable", but I'm always a fan of code simplifications
like this patch.  It makes it tempting to go back and read those last
two patches a little closer.  Almost tempting enough, in fact.  ;-)

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

[Elijah Newren]

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.

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.

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

[Jeff King]

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.

Thanks for taking a look. I do think patches 9 and 10 are the most
controversial in their goals. But for the reasons given there, I don't
think anybody will care about the direction much either way. And
certainly they are not making anything _worse_, since the thing they
disallow is already broken. They are merely shutting off the option of
"fixing" it to match the original intent.

So I stand by the direction I took in the patches, but I wanted to point
out that if anybody wants to be extra careful, those are the ones to
look at.

-Peff