{"thread":{"id":"61609","subject":"Cannot override `remote.origin.url` with `-c` option","startedAt":"2024-06-09T06:51:53Z","lastAt":"2024-06-26T20:40:04Z","messageCount":29,"participants":["Mathew George","Jeff King","Junio C Hamano","Elijah Newren"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"496728","messageId":"83D801A8-3878-43C1-B7A7-78B3B7315FD8@gmail.com","threadId":"61609","inReplyTo":null,"subject":"Cannot override `remote.origin.url` with `-c` option","fromName":"Mathew George","fromEmail":"mathewegeorge@gmail.com","sentAt":"2024-06-09T06:51:47Z","receivedAt":"2024-06-09T06:51:53Z","isPatch":false,"sender":{"key":"mathewegeorge@gmail.com","avatar":null},"body":"> Thank you for filling out a Git bug report!\n> Please answer the following questions to help us understand your issue.\n\n> What did you do before the bug happened? (Steps to reproduce your issue)\n\n```\n# Create a test repo to pull into\ngit init test\ncd $_\ngit remote add origin wrong-url\n# Create a test repo to pull from\ngit init ../test2\ncd $_\ntouch a\ngit add .\ngit commit -m 'commit to pull'\n# Attempt to pull, setting remote.origin.url\n# The following git commands will fail:\ncd ../test\ngit -c remote.origin.url=../test2 pull\ngit -c remote.origin.url=../test2 fetch\n# However, this succeeds:\ngit pull ../test2\n```\n\n\n> What did you expect to happen? (Expected behavior)\n\nThe '-c' option should allow me to override the remote URL,\nwhich, as a result of running `git remote add`, is configured as\nfollows in test/.git/config:\n\n```\n[remote \"origin\"]\n\turl = wrong-url\n```\n\nFrom git(1): \n```\n-c =\n Pass a configuration parameter to the command. The\n value given will override values from configuration\n files.\n```\n\n\n> What happened instead? (Actual behavior)\n\nThe following errors are produced for both 'fetch' and 'pull'\noperations:\n\n```\nfatal: 'wrong-url' does not appear to be a git repository\nfatal: Could not read from remote repository.\n\nPlease make sure you have the correct access rights\nand the repository exists.\n```\n\nRegardless of the -c option, the commands above use the url\n'wrong-url' from .git/config.\n\n\n> What's different between what you expected and what actually\n> happened?\n\nThe -c option is not respected.\n\n\n> Anything else you want to add:\n\nIt may be that this is not supported, but if that's the case, I\nmissed any documentation saying so.\n\n\n> Please review the rest of the bug report below.\n> You can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.45.2\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /usr/bin/sh\ncompiler info: clang: 17.0.2 (https://android.googlesource.com/toolchain/llvm-project d9f89f4d16663d5012e5c09495f3b30ece3d2362)\nlibc info: no libc information available\n$SHELL (typically, interactive shell): /usr/bin/bash\n\n\n[Enabled Hooks]\nnot run from a git repository - no hooks to show\n"},{"id":"496839","messageId":"20240611075137.GF3248245@coredump.intra.peff.net","threadId":"61609","inReplyTo":"83D801A8-3878-43C1-B7A7-78B3B7315FD8@gmail.com","subject":"Re: Cannot override `remote.origin.url` with `-c` option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-11T07:51:37Z","receivedAt":"2024-06-11T07:51:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 09, 2024 at 12:21:47PM +0530, Mathew George wrote:\n\n> > What did you expect to happen? (Expected behavior)\n> \n> The '-c' option should allow me to override the remote URL,\n> which, as a result of running `git remote add`, is configured as\n> follows in test/.git/config:\n> \n> ```\n> [remote \"origin\"]\n> \turl = wrong-url\n> ```\n\nI don't think \"-c\" is the culprit here, but rather that remote.*.url is\na multi-valued config option. Rather than having a single value with\n\"last one wins\" semantics, multiple instances of the config variable\nform a list. So you'd see the same thing with:\n \n [remote \"origin\"]\n url = wrong-url\n url = right-url\n\nin the config file.\n\nOf course that leaves two questions:\n\n 1. What are multiple URLs actually good for? I have no idea. I\n couldn't find any useful documentation in the manpages for\n git-config, git-remote, or git-fetch. It looks like any URLs after\n the first are used as push-only URLs:\n\n $ git remote show -n origin\n * remote origin\n Fetch URL: wrong-url\n Push URL: wrong-url\n Push URL: right-url\n [...]\n\n There's also a \"pushurl\" config option, which _replaces_ the\n original URL for pushing, rather than including it.\n\n As far as I can tell there's no way to actually have multiple fetch\n URLs (and the corner cases would be weird anyway). I suspect we'd\n come up with different semantics if we were designing it these\n days, but it has been this way since at least 2007. So any changes\n would probably need a deprecation period.\n\n 2. Is there a way to override the list?\n\n Sadly, no. For some config keys, we allow a value-less boolean\n entry to reset the list. But since the config code doesn't know\n about the semantics of each key, this has to be implemented\n individually for each key, and nobody has bothered to do so for\n remote.*.url.\n\n The value-less key is something like:\n\n [remote \"origin\"]\n url = wrong-url\n # this one resets the list to empty!\n url\n # and this one adds a new value\n url = right-url\n\n Or in \"-c\" terms, it would be:\n\n git -c remote.origin.url -c remote.origin.url=right-url ...\n\n The syntax is ugly, but it is backwards compatible (that value-less\n entry is currently an error). So we could do that now (I'll show a\n messy patch below).\n\nOf course none of that helps your immediate case. I did think of one\nworkaround, though, which is to use the \"insteadOf\" config to rewrite\nthe URL. So:\n\n git -c url.right-url.insteadOf=wrong-url ...\n\nwill rewrite all instances of \"wrong-url\" to use \"right-url\" instead (in\norigin and elsewhere).\n\n-Peff\n\n---\ndiff --git a/remote.c b/remote.c\nindex dcb5492c85..69b0f28637 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -63,6 +63,10 @@ static const char *alias_url(const char *url, struct rewrites *r)\n \n static void add_url(struct remote *remote, const char *url)\n {\n+\tif (!url) {\n+\t\tremote->url_nr = 0;\n+\t\treturn;\n+\t}\n \tALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);\n \tremote->url[remote->url_nr++] = url;\n }\n@@ -430,10 +434,7 @@ static int handle_config(const char *key, const char *value,\n \telse if (!strcmp(subkey, \"prunetags\"))\n \t\tremote->prune_tags = git_config_bool(key, value);\n \telse if (!strcmp(subkey, \"url\")) {\n-\t\tchar *v;\n-\t\tif (git_config_string(&v, key, value))\n-\t\t\treturn -1;\n-\t\tadd_url(remote, v);\n+\t\tadd_url(remote, xstrdup_or_null(value));\n \t} else if (!strcmp(subkey, \"pushurl\")) {\n \t\tchar *v;\n \t\tif (git_config_string(&v, key, value))\n"},{"id":"496928","messageId":"xmqq34pjxzva.fsf@gitster.g","threadId":"61609","inReplyTo":"20240611075137.GF3248245@coredump.intra.peff.net","subject":"Re: Cannot override `remote.origin.url` with `-c` option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-11T15:28:41Z","receivedAt":"2024-06-11T15:28:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Of course that leaves two questions:\n>\n> 1. What are multiple URLs actually good for? I have no idea. I\n\nIt came from days back when there weren't a separate push URL IIRC.\nYou may pull from a central place like everybody else, but you may\nhave other publishing points of your work.\n\nPlace to fetch must be a single known stable place for obvious\nreasons. \"I have N, so I'll pull from one chosen at random among\nthese N\" would lead to madness ;-).\n\n> 2. Is there a way to override the list?\n>\n> Sadly, no. For some config keys, we allow a value-less boolean\n> entry to reset the list.\n\nHmph, I somehow thought that a more widely used \"clear\" was an empty\nstring, but if we can make the convention around the \"I exist and I\nmean true\" entries, that would be great. It would not make much\nsense to have multi-valued Boolean variable to begin with.\n\n> Of course none of that helps your immediate case. I did think of one\n> workaround, though, which is to use the \"insteadOf\" config to rewrite\n> the URL. So:\n>\n> git -c url.right-url.insteadOf=wrong-url ...\n>\n> will rewrite all instances of \"wrong-url\" to use \"right-url\" instead (in\n> origin and elsewhere).\n\nYup, that is an excellent idea.\n\n> diff --git a/remote.c b/remote.c\n> index dcb5492c85..69b0f28637 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -63,6 +63,10 @@ static const char *alias_url(const char *url, struct rewrites *r)\n> \n> static void add_url(struct remote *remote, const char *url)\n> {\n> +\tif (!url) {\n> +\t\tremote->url_nr = 0;\n> +\t\treturn;\n> +\t}\n> \tALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);\n> \tremote->url[remote->url_nr++] = url;\n> }\n> @@ -430,10 +434,7 @@ static int handle_config(const char *key, const char *value,\n> \telse if (!strcmp(subkey, \"prunetags\"))\n> \t\tremote->prune_tags = git_config_bool(key, value);\n> \telse if (!strcmp(subkey, \"url\")) {\n> -\t\tchar *v;\n> -\t\tif (git_config_string(&v, key, value))\n> -\t\t\treturn -1;\n> -\t\tadd_url(remote, v);\n> +\t\tadd_url(remote, xstrdup_or_null(value));\n> \t} else if (!strcmp(subkey, \"pushurl\")) {\n> \t\tchar *v;\n> \t\tif (git_config_string(&v, key, value))\n\nI was expecting (with excitement) a mess, but the above is as clean\nas we can make the idea, I would say. Lack of documentation and\ntests do count as incompleteness though of course.\n\n"},{"id":"497089","messageId":"20240613102409.GE817573@coredump.intra.peff.net","threadId":"61609","inReplyTo":"xmqq34pjxzva.fsf@gitster.g","subject":"Re: Cannot override `remote.origin.url` with `-c` option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-13T10:24:09Z","receivedAt":"2024-06-13T10:24:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 11, 2024 at 08:28:41AM -0700, Junio C Hamano wrote:\n\n> > 2. Is there a way to override the list?\n> >\n> > Sadly, no. For some config keys, we allow a value-less boolean\n> > entry to reset the list.\n> \n> Hmph, I somehow thought that a more widely used \"clear\" was an empty\n> string, but if we can make the convention around the \"I exist and I\n> mean true\" entries, that would be great. It would not make much\n> sense to have multi-valued Boolean variable to begin with.\n\nNo, you're right. I was just misremembering (maybe because \"--no-foo\" is\nthe command-line way to reset the \"foo\" list). The three instances of\nconfig I found (merge.suppressdest, http.extraheaders, and\ncredential.helper) all treat the empty string that way.\n\nThe multi-valued boolean thing would cover config keys where an empty\nstring was semantically meaningful, but I wouldn't be surprised if we\nnever run across such a case. So probably using \"empty string is reset\"\nas the convention is fine, and matches existing cases.\n\n> > diff --git a/remote.c b/remote.c\n> [...]\n> \n> I was expecting (with excitement) a mess, but the above is as clean\n> as we can make the idea, I would say. Lack of documentation and\n> tests do count as incompleteness though of course.\n\nYeah, and we should probably do the same for pushurl. And I think there\ncould be some cleanup of the memory ownership handling of add_url().\n\n-Peff\n"},{"id":"497160","messageId":"20240614102439.GA222287@coredump.intra.peff.net","threadId":"61609","inReplyTo":"20240613102409.GE817573@coredump.intra.peff.net","subject":"[PATCH 0/11] allow overriding remote.*.url","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-14T10:24:39Z","receivedAt":"2024-06-14T10:24:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 13, 2024 at 06:24:09AM -0400, Jeff King wrote:\n\n> > I was expecting (with excitement) a mess, but the above is as clean\n> > as we can make the idea, I would say. Lack of documentation and\n> > tests do count as incompleteness though of course.\n> \n> Yeah, and we should probably do the same for pushurl. And I think there\n> could be some cleanup of the memory ownership handling of add_url().\n\nSo as always with this crufty 2009-era code, there turned out to be some\nsubtleties. ;)\n\nThe good news is that I think dealing with them left the code in a\nbetter place. It's easier to reason about, and a few possible leaks have\nbeen plugged (I don't know if they were triggered in the test suite or\nnot; if so they weren't enough to tip any scripts over to being\nleak-free).\n\nWe can split the series into segments:\n\n [01/11]: archive: fix check for missing url\n\n A nearby trivial bugfix.\n\n [02/11]: remote: refactor alias_url() memory ownership\n [03/11]: remote: transfer ownership of memory in add_url(), etc\n [04/11]: remote: use strvecs to store remote url/pushurl\n [05/11]: remote: simplify url/pushurl selection\n\n Fixing memory handling weirdness, which is a necessary prereq for\n the \"reset\" operation to avoid leaking. The switch to using a strvec\n isn't strictly necessary, but it does make the code (including the\n later patch 7) simpler.\n\n [06/11]: config: document remote.*.url/pushurl interaction\n [07/11]: remote: allow resetting url list\n\n The actual change is in patch 7 here, but it was hard to add new\n docs to the rather anemic existing ones. Hence patch 6.\n\n [08/11]: t5801: make remote-testgit GIT_DIR setup more robust\n [09/11]: t5801: test remote.*.vcs config\n [10/11]: remote: always require at least one url in a remote\n [11/11]: remote: drop checks for zero-url case\n\n This is a related cleanup I found while working in the area.\n Arguably it could be a separate topic, though it does depend\n textually on what came before.\n\n Documentation/config/remote.txt | 11 +++-\n builtin/archive.c | 4 +-\n builtin/clone.c | 4 +-\n builtin/ls-remote.c | 6 +--\n builtin/push.c | 28 ++--------\n builtin/remote.c | 88 +++++++++---------------------\n remote-curl.c | 2 +-\n remote.c | 94 ++++++++++++++++++---------------\n remote.h | 13 ++---\n t/helper/test-bundle-uri.c | 2 -\n t/t5505-remote.sh | 36 +++++++++++++\n t/t5801-remote-helpers.sh | 23 ++++++++\n t/t5801/git-remote-nourl | 3 ++\n t/t5801/git-remote-testgit | 3 +-\n transport.c | 19 +++----\n 15 files changed, 174 insertions(+), 162 deletions(-)\n create mode 100755 t/t5801/git-remote-nourl\n\n-Peff\n"},{"id":"497161","messageId":"20240614102525.GA222445@coredump.intra.peff.net","threadId":"61609","inReplyTo":"20240614102439.GA222287@coredump.intra.peff.net","subject":"[PATCH 01/11] archive: fix check for missing url","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-14T10:25:25Z","receivedAt":"2024-06-14T10:25:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Running \"git archive --remote\" checks that we have at least one url for\nthe remote. It does so by looking at remote.url[0], but that won't work;\nif we have no url at all, then remote.url will be NULL, and we'll\nsegfault.\n\nCheck url_nr instead, which is a more direct way of asking what we\nwant.\n\nYou can trigger the segfault like this:\n\n git -c remote.foo.vcs=bar archive --remote=foo\n\nbut I didn't bother adding a test. This is the tip of the iceberg for\nno-url remotes, and a later patch will improve that situation. I just\nwanted to clean up this bug so it didn't make further refactoring of\nthis code more confusing.\n\nSigned-off-by: Jeff King \n---\nThis code actually goes away in patch 10, but it's possible we'd want to\ntake a different approach there. So I preferred to fix this up front\nanyway.\n\n builtin/archive.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex 15ee1ec7bb..f35560042e 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -31,7 +31,7 @@ static int run_remote_archiver(int argc, const char **argv,\n \tstruct packet_reader reader;\n \n \t_remote = remote_get(remote);\n-\tif (!_remote->url[0])\n+\tif (!_remote->url_nr)\n \t\tdie(_(\"git archive: Remote with no URL\"));\n \ttransport = transport_get(_remote, _remote->url[0]);\n \ttransport_connect(transport, \"git-upload-archive\", exec, fd);\n-- \n2.45.2.937.g0bcb3c087a\n\n"},{"id":"497162","messageId":"20240614102616.GB222445@coredump.intra.peff.net","threadId":"61609","inReplyTo":"20240614102439.GA222287@coredump.intra.peff.net","subject":"[PATCH 02/11] remote: refactor alias_url() memory ownership","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-14T10:26:16Z","receivedAt":"2024-06-14T10:26:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The alias_url() function may return either a newly allocated string\n(which the caller must take ownership of), or the original const \"url\"\nparameter that was passed in.\n\nThis often works OK because callers are generally passing in a \"url\"\nthat they expect to retain ownership of anyway. So whether we got back\nthe original or a new string, we're always interested in storing it\nforever. But I suspect there are some possible leaks here (e.g.,\nadd_url_alias() may end up discarding the original \"url\").\n\nWhether there are active leaks or not, this is a confusing setup that\nmakes further refactoring of memory ownership harder. So instead of\nreturning the original string, return NULL, forcing callers to decide\nwhat to do with it explicitly. We can then build further cleanups on top\nof that.\n\nSigned-off-by: Jeff King \n---\nJust to be clear, I think there's probably still a leak in\nadd_url_alias() even after this patch. It's just a step on the way to\nfixing.\n\n remote.c | 26 +++++++++++++++-----------\n 1 file changed, 15 insertions(+), 11 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex dcb5492c85..fd9d58f820 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -35,7 +35,7 @@ static int valid_remote(const struct remote *remote)\n \treturn (!!remote->url) || (!!remote->foreign_vcs);\n }\n \n-static const char *alias_url(const char *url, struct rewrites *r)\n+static char *alias_url(const char *url, struct rewrites *r)\n {\n \tint i, j;\n \tstruct counted_string *longest;\n@@ -56,7 +56,7 @@ static const char *alias_url(const char *url, struct rewrites *r)\n \t\t}\n \t}\n \tif (!longest)\n-\t\treturn url;\n+\t\treturn NULL;\n \n \treturn xstrfmt(\"%s%s\", r->rewrite[longest_i]->base, url + longest->len);\n }\n@@ -76,15 +76,16 @@ static void add_pushurl(struct remote *remote, const char *pushurl)\n static void add_pushurl_alias(struct remote_state *remote_state,\n \t\t\t struct remote *remote, const char *url)\n {\n-\tconst char *pushurl = alias_url(url, &remote_state->rewrites_push);\n-\tif (pushurl != url)\n-\t\tadd_pushurl(remote, pushurl);\n+\tchar *alias = alias_url(url, &remote_state->rewrites_push);\n+\tif (alias)\n+\t\tadd_pushurl(remote, alias);\n }\n \n static void add_url_alias(struct remote_state *remote_state,\n \t\t\t struct remote *remote, const char *url)\n {\n-\tadd_url(remote, alias_url(url, &remote_state->rewrites));\n+\tchar *alias = alias_url(url, &remote_state->rewrites);\n+\tadd_url(remote, alias ? alias : url);\n \tadd_pushurl_alias(remote_state, remote, url);\n }\n \n@@ -492,19 +493,22 @@ static void alias_all_urls(struct remote_state *remote_state)\n \t\tif (!remote_state->remotes[i])\n \t\t\tcontinue;\n \t\tfor (j = 0; j < remote_state->remotes[i]->pushurl_nr; j++) {\n-\t\t\tremote_state->remotes[i]->pushurl[j] =\n-\t\t\t\talias_url(remote_state->remotes[i]->pushurl[j],\n-\t\t\t\t\t &remote_state->rewrites);\n+\t\t\tchar *alias = alias_url(remote_state->remotes[i]->pushurl[j],\n+\t\t\t\t\t\t&remote_state->rewrites);\n+\t\t\tif (alias)\n+\t\t\t\tremote_state->remotes[i]->pushurl[j] = alias;\n \t\t}\n \t\tadd_pushurl_aliases = remote_state->remotes[i]->pushurl_nr == 0;\n \t\tfor (j = 0; j < remote_state->remotes[i]->url_nr; j++) {\n+\t\t\tchar *alias;\n \t\t\tif (add_pushurl_aliases)\n \t\t\t\tadd_pushurl_alias(\n \t\t\t\t\tremote_state, remote_state->remotes[i],\n \t\t\t\t\tremote_state->remotes[i]->url[j]);\n-\t\t\tremote_state->remotes[i]->url[j] =\n-\t\t\t\talias_url(remote_state->remotes[i]->url[j],\n+\t\t\talias = alias_url(remote_state->remotes[i]->url[j],\n \t\t\t\t\t &remote_state->rewrites);\n+\t\t\tif (alias)\n+\t\t\t\tremote_state->remotes[i]->url[j] = alias;\n \t\t}\n \t}\n }\n-- \n2.45.2.937.g0bcb3c087a\n\n"},{"id":"497163","messageId":"20240614102722.GC222445@coredump.intra.peff.net","threadId":"61609","inReplyTo":"20240614102439.GA222287@coredump.intra.peff.net","subject":"[PATCH 03/11] remote: transfer ownership of memory in add_url(), etc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-14T10:27:22Z","receivedAt":"2024-06-14T10:27:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many of the internal functions in remote.c take const strings and store\nthem forever in instances of \"struct remote\". Since the functions are\ninternal and callers are aware of the convention, this seems to mostly\nwork and not cause leaks. But there are some issues:\n\n - it's impossible to clear any of the arrays, because the data\n dependencies between them are too muddled (if you free() a string,\n it might also be referenced from another array, causing a\n user-after-free; but if you don't, that might be the last reference,\n causing a leak).\n\n This is mostly of interest for further refactoring and features, but\n there's at least one spot that's already a problem. In alias_all_urls(),\n we replace elements of remote->url and remote->pushurl with their\n aliased forms, dropping references to the original.\n\n - sometimes strings from outside callers make their way in. For\n example, calling remote_get(\"foo\") when there is no configured \"foo\"\n remote will create a remote struct with the single url \"foo\". But\n we'll do so by holding on to the string passed to remote_get()\n forever.\n\n In practice I think this works out because we'd usually pass in a\n string that lasts the length of the program (a string literal, or\n argv reference, or other data structure allocated in the main\n function). But it's a rather subtle requirement.\n\nInstead, let's have remote->url and remote->pushurl own their string\nmemory. They'll copy the const strings that are passed in, and callers\ncan stop making their own copies. Likewise, when we overwrite an entry,\nwe can free the memory it points to, fixing the leak mentioned above.\n\nWe'll leave the struct members as \"const\" since they are visible to the\noutside world, and shouldn't usually be touched. This requires casting\non free() for now, but we'll clean that further in a future patch.\n\nSigned-off-by: Jeff King \n---\nSince now we'll always allocate, I don't think it's possible for this to\nintroduce any memory corruption issues. It _might_ introduce a leak, but\nI think I fixed all of the callers.\n\n remote.c | 33 +++++++++++++++++++--------------\n 1 file changed, 19 insertions(+), 14 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex fd9d58f820..f7c846865f 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -64,13 +64,13 @@ static char *alias_url(const char *url, struct rewrites *r)\n static void add_url(struct remote *remote, const char *url)\n {\n \tALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);\n-\tremote->url[remote->url_nr++] = url;\n+\tremote->url[remote->url_nr++] = xstrdup(url);\n }\n \n static void add_pushurl(struct remote *remote, const char *pushurl)\n {\n \tALLOC_GROW(remote->pushurl, remote->pushurl_nr + 1, remote->pushurl_alloc);\n-\tremote->pushurl[remote->pushurl_nr++] = pushurl;\n+\tremote->pushurl[remote->pushurl_nr++] = xstrdup(pushurl);\n }\n \n static void add_pushurl_alias(struct remote_state *remote_state,\n@@ -79,6 +79,7 @@ static void add_pushurl_alias(struct remote_state *remote_state,\n \tchar *alias = alias_url(url, &remote_state->rewrites_push);\n \tif (alias)\n \t\tadd_pushurl(remote, alias);\n+\tfree(alias);\n }\n \n static void add_url_alias(struct remote_state *remote_state,\n@@ -87,6 +88,7 @@ static void add_url_alias(struct remote_state *remote_state,\n \tchar *alias = alias_url(url, &remote_state->rewrites);\n \tadd_url(remote, alias ? alias : url);\n \tadd_pushurl_alias(remote_state, remote, url);\n+\tfree(alias);\n }\n \n struct remotes_hash_key {\n@@ -293,7 +295,7 @@ static void read_remotes_file(struct remote_state *remote_state,\n \n \t\tif (skip_prefix(buf.buf, \"URL:\", &v))\n \t\t\tadd_url_alias(remote_state, remote,\n-\t\t\t\t xstrdup(skip_spaces(v)));\n+\t\t\t\t skip_spaces(v));\n \t\telse if (skip_prefix(buf.buf, \"Push:\", &v))\n \t\t\trefspec_append(&remote->push, skip_spaces(v));\n \t\telse if (skip_prefix(buf.buf, \"Pull:\", &v))\n@@ -336,7 +338,7 @@ static void read_branches_file(struct remote_state *remote_state,\n \telse\n \t\tfrag = to_free = repo_default_branch_name(the_repository, 0);\n \n-\tadd_url_alias(remote_state, remote, strbuf_detach(&buf, NULL));\n+\tadd_url_alias(remote_state, remote, buf.buf);\n \trefspec_appendf(&remote->fetch, \"refs/heads/%s:refs/heads/%s\",\n \t\t\tfrag, remote->name);\n \n@@ -347,6 +349,7 @@ static void read_branches_file(struct remote_state *remote_state,\n \trefspec_appendf(&remote->push, \"HEAD:refs/heads/%s\", frag);\n \tremote->fetch_tags = 1; /* always auto-follow */\n \n+\tstrbuf_release(&buf);\n \tfree(to_free);\n }\n \n@@ -431,15 +434,13 @@ static int handle_config(const char *key, const char *value,\n \telse if (!strcmp(subkey, \"prunetags\"))\n \t\tremote->prune_tags = git_config_bool(key, value);\n \telse if (!strcmp(subkey, \"url\")) {\n-\t\tchar *v;\n-\t\tif (git_config_string(&v, key, value))\n-\t\t\treturn -1;\n-\t\tadd_url(remote, v);\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(key);\n+\t\tadd_url(remote, value);\n \t} else if (!strcmp(subkey, \"pushurl\")) {\n-\t\tchar *v;\n-\t\tif (git_config_string(&v, key, value))\n-\t\t\treturn -1;\n-\t\tadd_pushurl(remote, v);\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(key);\n+\t\tadd_pushurl(remote, value);\n \t} else if (!strcmp(subkey, \"push\")) {\n \t\tchar *v;\n \t\tif (git_config_string(&v, key, value))\n@@ -495,8 +496,10 @@ static void alias_all_urls(struct remote_state *remote_state)\n \t\tfor (j = 0; j < remote_state->remotes[i]->pushurl_nr; j++) {\n \t\t\tchar *alias = alias_url(remote_state->remotes[i]->pushurl[j],\n \t\t\t\t\t\t&remote_state->rewrites);\n-\t\t\tif (alias)\n+\t\t\tif (alias) {\n+\t\t\t\tfree((char *)remote_state->remotes[i]->pushurl[j]);\n \t\t\t\tremote_state->remotes[i]->pushurl[j] = alias;\n+\t\t\t}\n \t\t}\n \t\tadd_pushurl_aliases = remote_state->remotes[i]->pushurl_nr == 0;\n \t\tfor (j = 0; j < remote_state->remotes[i]->url_nr; j++) {\n@@ -507,8 +510,10 @@ static void alias_all_urls(struct remote_state *remote_state)\n \t\t\t\t\tremote_state->remotes[i]->url[j]);\n \t\t\talias = alias_url(remote_state->remotes[i]->url[j],\n \t\t\t\t\t &remote_state->rewrites);\n-\t\t\tif (alias)\n+\t\t\tif (alias) {\n+\t\t\t\tfree((char *)remote_state->remotes[i]->url[j]);\n \t\t\t\tremote_state->remotes[i]->url[j] = alias;\n+\t\t\t}\n \t\t}\n \t}\n }\n-- \n2.45.2.937.g0bcb3c087a\n\n"},{"id":"497164","messageId":"20240614102801.GD222445@coredump.intra.peff.net","threadId":"61609","inReplyTo":"20240614102439.GA222287@coredump.intra.peff.net","subject":"[PATCH 04/11] remote: use strvecs to store remote url/pushurl","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-14T10:28:01Z","receivedAt":"2024-06-14T10:28:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Now that the url/pushurl fields of \"struct remote\" own their strings, we\ncan switch from bare arrays to strvecs. This has a few advantages:\n\n - push/clear are now one-liners\n\n - likewise the free+assigns in alias_all_urls() can use\n strvec_replace()\n\n - we now use size_t for storage, avoiding possible overflow\n\n - this will enable some further cleanups in future patches\n\nThere's quite a bit of fallout in the code that reads these fields, as\nit tends to access these arrays directly. But it's mostly a mechanical\nreplacement of \"url_nr\" with \"url.nr\", and \"url[i]\" with \"url.v[i]\",\nwith a few variations (e.g. \"*url\" could become \"*url.v\", but I used\n\"url.v[0]\" for consistency).\n\nSigned-off-by: Jeff King \n---\n builtin/archive.c | 4 +--\n builtin/clone.c | 4 +--\n builtin/ls-remote.c | 6 ++--\n builtin/push.c | 10 +++----\n builtin/remote.c | 56 +++++++++++++++++++-------------------\n remote-curl.c | 2 +-\n remote.c | 52 ++++++++++++++---------------------\n remote.h | 12 ++------\n t/helper/test-bundle-uri.c | 2 +-\n transport.c | 4 +--\n 10 files changed, 68 insertions(+), 84 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex f35560042e..0d9aff7e6f 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -31,9 +31,9 @@ static int run_remote_archiver(int argc, const char **argv,\n \tstruct packet_reader reader;\n \n \t_remote = remote_get(remote);\n-\tif (!_remote->url_nr)\n+\tif (!_remote->url.nr)\n \t\tdie(_(\"git archive: Remote with no URL\"));\n-\ttransport = transport_get(_remote, _remote->url[0]);\n+\ttransport = transport_get(_remote, _remote->url.v[0]);\n \ttransport_connect(transport, \"git-upload-archive\", exec, fd);\n \n \t/*\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 730b3efae6..d3b70b49b0 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1290,7 +1290,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \trefspec_appendf(&remote->fetch, \"+%s*:%s*\", src_ref_prefix,\n \t\t\tbranch_top.buf);\n \n-\tpath = get_repo_path(remote->url[0], &is_bundle);\n+\tpath = get_repo_path(remote->url.v[0], &is_bundle);\n \tis_local = option_local != 0 && path && !is_bundle;\n \tif (is_local) {\n \t\tif (option_depth)\n@@ -1312,7 +1312,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (option_local > 0 && !is_local)\n \t\twarning(_(\"--local is ignored\"));\n \n-\ttransport = transport_get(remote, path ? path : remote->url[0]);\n+\ttransport = transport_get(remote, path ? path : remote->url.v[0]);\n \ttransport_set_verbosity(transport, option_verbosity, option_progress);\n \ttransport->family = family;\n \ttransport->cloning = 1;\ndiff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\nindex e8d65ebbdc..4c04dbbf19 100644\n--- a/builtin/ls-remote.c\n+++ b/builtin/ls-remote.c\n@@ -109,11 +109,11 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \t\t\tdie(\"bad repository '%s'\", dest);\n \t\tdie(\"No remote configured to list refs from.\");\n \t}\n-\tif (!remote->url_nr)\n+\tif (!remote->url.nr)\n \t\tdie(\"remote %s has no configured URL\", dest);\n \n \tif (get_url) {\n-\t\tprintf(\"%s\\n\", *remote->url);\n+\t\tprintf(\"%s\\n\", remote->url.v[0]);\n \t\treturn 0;\n \t}\n \n@@ -130,7 +130,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (!dest && !quiet)\n-\t\tfprintf(stderr, \"From %s\\n\", *remote->url);\n+\t\tfprintf(stderr, \"From %s\\n\", remote->url.v[0]);\n \tfor ( ; ref; ref = ref->next) {\n \t\tstruct ref_array_item *item;\n \t\tif (!check_ref_type(ref, flags))\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 2fbb31c3ad..61b5d3afdd 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -143,12 +143,12 @@ static void set_refspecs(const char **refs, int nr, const char *repo)\n \n static int push_url_of_remote(struct remote *remote, const char ***url_p)\n {\n-\tif (remote->pushurl_nr) {\n-\t\t*url_p = remote->pushurl;\n-\t\treturn remote->pushurl_nr;\n+\tif (remote->pushurl.nr) {\n+\t\t*url_p = remote->pushurl.v;\n+\t\treturn remote->pushurl.nr;\n \t}\n-\t*url_p = remote->url;\n-\treturn remote->url_nr;\n+\t*url_p = remote->url.v;\n+\treturn remote->url.nr;\n }\n \n static NORETURN void die_push_simple(struct branch *branch,\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 447ef1d3c9..ee6a33ff11 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -619,8 +619,8 @@ static int migrate_file(struct remote *remote)\n \tint i;\n \n \tstrbuf_addf(&buf, \"remote.%s.url\", remote->name);\n-\tfor (i = 0; i < remote->url_nr; i++)\n-\t\tgit_config_set_multivar(buf.buf, remote->url[i], \"^$\", 0);\n+\tfor (i = 0; i < remote->url.nr; i++)\n+\t\tgit_config_set_multivar(buf.buf, remote->url.v[i], \"^$\", 0);\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"remote.%s.push\", remote->name);\n \tfor (i = 0; i < remote->push.raw_nr; i++)\n@@ -1002,8 +1002,8 @@ static int get_remote_ref_states(const char *name,\n \t\tstruct transport *transport;\n \t\tconst struct ref *remote_refs;\n \n-\t\ttransport = transport_get(states->remote, states->remote->url_nr > 0 ?\n-\t\t\tstates->remote->url[0] : NULL);\n+\t\ttransport = transport_get(states->remote, states->remote->url.nr > 0 ?\n+\t\t\tstates->remote->url.v[0] : NULL);\n \t\tremote_refs = transport_get_remote_refs(transport, NULL);\n \n \t\tstates->queried = 1;\n@@ -1216,12 +1216,12 @@ static int get_one_entry(struct remote *remote, void *priv)\n \tconst char **url;\n \tint i, url_nr;\n \n-\tif (remote->url_nr > 0) {\n+\tif (remote->url.nr > 0) {\n \t\tstruct strbuf promisor_config = STRBUF_INIT;\n \t\tconst char *partial_clone_filter = NULL;\n \n \t\tstrbuf_addf(&promisor_config, \"remote.%s.partialclonefilter\", remote->name);\n-\t\tstrbuf_addf(&remote_info_buf, \"%s (fetch)\", remote->url[0]);\n+\t\tstrbuf_addf(&remote_info_buf, \"%s (fetch)\", remote->url.v[0]);\n \t\tif (!git_config_get_string_tmp(promisor_config.buf, &partial_clone_filter))\n \t\t\tstrbuf_addf(&remote_info_buf, \" [%s]\", partial_clone_filter);\n \n@@ -1230,12 +1230,12 @@ static int get_one_entry(struct remote *remote, void *priv)\n \t\t\t\tstrbuf_detach(&remote_info_buf, NULL);\n \t} else\n \t\tstring_list_append(list, remote->name)->util = NULL;\n-\tif (remote->pushurl_nr) {\n-\t\turl = remote->pushurl;\n-\t\turl_nr = remote->pushurl_nr;\n+\tif (remote->pushurl.nr) {\n+\t\turl = remote->pushurl.v;\n+\t\turl_nr = remote->pushurl.nr;\n \t} else {\n-\t\turl = remote->url;\n-\t\turl_nr = remote->url_nr;\n+\t\turl = remote->url.v;\n+\t\turl_nr = remote->url.nr;\n \t}\n \tfor (i = 0; i < url_nr; i++)\n \t{\n@@ -1301,14 +1301,14 @@ static int show(int argc, const char **argv, const char *prefix)\n \t\tget_remote_ref_states(*argv, &info.states, query_flag);\n \n \t\tprintf_ln(_(\"* remote %s\"), *argv);\n-\t\tprintf_ln(_(\" Fetch URL: %s\"), info.states.remote->url_nr > 0 ?\n-\t\t info.states.remote->url[0] : _(\"(no URL)\"));\n-\t\tif (info.states.remote->pushurl_nr) {\n-\t\t\turl = info.states.remote->pushurl;\n-\t\t\turl_nr = info.states.remote->pushurl_nr;\n+\t\tprintf_ln(_(\" Fetch URL: %s\"), info.states.remote->url.nr > 0 ?\n+\t\t info.states.remote->url.v[0] : _(\"(no URL)\"));\n+\t\tif (info.states.remote->pushurl.nr) {\n+\t\t\turl = info.states.remote->pushurl.v;\n+\t\t\turl_nr = info.states.remote->pushurl.nr;\n \t\t} else {\n-\t\t\turl = info.states.remote->url;\n-\t\t\turl_nr = info.states.remote->url_nr;\n+\t\t\turl = info.states.remote->url.v;\n+\t\t\turl_nr = info.states.remote->url.nr;\n \t\t}\n \t\tfor (i = 0; i < url_nr; i++)\n \t\t\t/*\n@@ -1454,8 +1454,8 @@ static int prune_remote(const char *remote, int dry_run)\n \n \tprintf_ln(_(\"Pruning %s\"), remote);\n \tprintf_ln(_(\"URL: %s\"),\n-\t\t states.remote->url_nr\n-\t\t ? states.remote->url[0]\n+\t\t states.remote->url.nr\n+\t\t ? states.remote->url.v[0]\n \t\t : _(\"(no URL)\"));\n \n \tfor_each_string_list_item(item, &states.stale)\n@@ -1647,15 +1647,15 @@ static int get_url(int argc, const char **argv, const char *prefix)\n \n \turl_nr = 0;\n \tif (push_mode) {\n-\t\turl = remote->pushurl;\n-\t\turl_nr = remote->pushurl_nr;\n+\t\turl = remote->pushurl.v;\n+\t\turl_nr = remote->pushurl.nr;\n \t}\n \t/* else fetch mode */\n \n \t/* Use the fetch URL when no push URLs were found or requested. */\n \tif (!url_nr) {\n-\t\turl = remote->url;\n-\t\turl_nr = remote->url_nr;\n+\t\turl = remote->url.v;\n+\t\turl_nr = remote->url.nr;\n \t}\n \n \tif (!url_nr)\n@@ -1718,12 +1718,12 @@ static int set_url(int argc, const char **argv, const char *prefix)\n \n \tif (push_mode) {\n \t\tstrbuf_addf(&name_buf, \"remote.%s.pushurl\", remotename);\n-\t\turlset = remote->pushurl;\n-\t\turlset_nr = remote->pushurl_nr;\n+\t\turlset = remote->pushurl.v;\n+\t\turlset_nr = remote->pushurl.nr;\n \t} else {\n \t\tstrbuf_addf(&name_buf, \"remote.%s.url\", remotename);\n-\t\turlset = remote->url;\n-\t\turlset_nr = remote->url_nr;\n+\t\turlset = remote->url.v;\n+\t\turlset_nr = remote->url.nr;\n \t}\n \n \t/* Special cases that add new entry. */\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 6008d7e87c..22957c16db 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1574,7 +1574,7 @@ int cmd_main(int argc, const char **argv)\n \tif (argc > 2) {\n \t\tend_url_with_slash(&url, argv[2]);\n \t} else {\n-\t\tend_url_with_slash(&url, remote->url[0]);\n+\t\tend_url_with_slash(&url, remote->url.v[0]);\n \t}\n \n \thttp_init(remote, url.buf, 0);\ndiff --git a/remote.c b/remote.c\nindex f7c846865f..76a3e41c73 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -32,7 +32,7 @@ struct counted_string {\n \n static int valid_remote(const struct remote *remote)\n {\n-\treturn (!!remote->url) || (!!remote->foreign_vcs);\n+\treturn (!!remote->url.nr) || (!!remote->foreign_vcs);\n }\n \n static char *alias_url(const char *url, struct rewrites *r)\n@@ -63,14 +63,12 @@ static char *alias_url(const char *url, struct rewrites *r)\n \n static void add_url(struct remote *remote, const char *url)\n {\n-\tALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);\n-\tremote->url[remote->url_nr++] = xstrdup(url);\n+\tstrvec_push(&remote->url, url);\n }\n \n static void add_pushurl(struct remote *remote, const char *pushurl)\n {\n-\tALLOC_GROW(remote->pushurl, remote->pushurl_nr + 1, remote->pushurl_alloc);\n-\tremote->pushurl[remote->pushurl_nr++] = xstrdup(pushurl);\n+\tstrvec_push(&remote->pushurl, pushurl);\n }\n \n static void add_pushurl_alias(struct remote_state *remote_state,\n@@ -150,18 +148,12 @@ static struct remote *make_remote(struct remote_state *remote_state,\n \n static void remote_clear(struct remote *remote)\n {\n-\tint i;\n-\n \tfree((char *)remote->name);\n \tfree((char *)remote->foreign_vcs);\n \n-\tfor (i = 0; i < remote->url_nr; i++)\n-\t\tfree((char *)remote->url[i]);\n-\tFREE_AND_NULL(remote->url);\n+\tstrvec_clear(&remote->url);\n+\tstrvec_clear(&remote->pushurl);\n \n-\tfor (i = 0; i < remote->pushurl_nr; i++)\n-\t\tfree((char *)remote->pushurl[i]);\n-\tFREE_AND_NULL(remote->pushurl);\n \tfree((char *)remote->receivepack);\n \tfree((char *)remote->uploadpack);\n \tFREE_AND_NULL(remote->http_proxy);\n@@ -493,27 +485,25 @@ static void alias_all_urls(struct remote_state *remote_state)\n \t\tint add_pushurl_aliases;\n \t\tif (!remote_state->remotes[i])\n \t\t\tcontinue;\n-\t\tfor (j = 0; j < remote_state->remotes[i]->pushurl_nr; j++) {\n-\t\t\tchar *alias = alias_url(remote_state->remotes[i]->pushurl[j],\n+\t\tfor (j = 0; j < remote_state->remotes[i]->pushurl.nr; j++) {\n+\t\t\tchar *alias = alias_url(remote_state->remotes[i]->pushurl.v[j],\n \t\t\t\t\t\t&remote_state->rewrites);\n-\t\t\tif (alias) {\n-\t\t\t\tfree((char *)remote_state->remotes[i]->pushurl[j]);\n-\t\t\t\tremote_state->remotes[i]->pushurl[j] = alias;\n-\t\t\t}\n+\t\t\tif (alias)\n+\t\t\t\tstrvec_replace(&remote_state->remotes[i]->pushurl,\n+\t\t\t\t\t j, alias);\n \t\t}\n-\t\tadd_pushurl_aliases = remote_state->remotes[i]->pushurl_nr == 0;\n-\t\tfor (j = 0; j < remote_state->remotes[i]->url_nr; j++) {\n+\t\tadd_pushurl_aliases = remote_state->remotes[i]->pushurl.nr == 0;\n+\t\tfor (j = 0; j < remote_state->remotes[i]->url.nr; j++) {\n \t\t\tchar *alias;\n \t\t\tif (add_pushurl_aliases)\n \t\t\t\tadd_pushurl_alias(\n \t\t\t\t\tremote_state, remote_state->remotes[i],\n-\t\t\t\t\tremote_state->remotes[i]->url[j]);\n-\t\t\talias = alias_url(remote_state->remotes[i]->url[j],\n+\t\t\t\t\tremote_state->remotes[i]->url.v[j]);\n+\t\t\talias = alias_url(remote_state->remotes[i]->url.v[j],\n \t\t\t\t\t &remote_state->rewrites);\n-\t\t\tif (alias) {\n-\t\t\t\tfree((char *)remote_state->remotes[i]->url[j]);\n-\t\t\t\tremote_state->remotes[i]->url[j] = alias;\n-\t\t\t}\n+\t\t\tif (alias)\n+\t\t\t\tstrvec_replace(&remote_state->remotes[i]->url,\n+\t\t\t\t\t j, alias);\n \t\t}\n \t}\n }\n@@ -653,10 +643,10 @@ static void validate_remote_url(struct remote *remote)\n \telse\n \t\tdie(_(\"unrecognized value transfer.credentialsInUrl: '%s'\"), value);\n \n-\tfor (i = 0; i < remote->url_nr; i++) {\n+\tfor (i = 0; i < remote->url.nr; i++) {\n \t\tstruct url_info url_info = { 0 };\n \n-\t\tif (!url_normalize(remote->url[i], &url_info) ||\n+\t\tif (!url_normalize(remote->url.v[i], &url_info) ||\n \t\t !url_info.passwd_off)\n \t\t\tgoto loop_cleanup;\n \n@@ -830,8 +820,8 @@ struct ref *ref_remove_duplicates(struct ref *ref_map)\n int remote_has_url(struct remote *remote, const char *url)\n {\n \tint i;\n-\tfor (i = 0; i < remote->url_nr; i++) {\n-\t\tif (!strcmp(remote->url[i], url))\n+\tfor (i = 0; i < remote->url.nr; i++) {\n+\t\tif (!strcmp(remote->url.v[i], url))\n \t\t\treturn 1;\n \t}\n \treturn 0;\ndiff --git a/remote.h b/remote.h\nindex e8c6655e42..84dc91cca0 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -4,6 +4,7 @@\n #include \"hash-ll.h\"\n #include \"hashmap.h\"\n #include \"refspec.h\"\n+#include \"strvec.h\"\n \n struct option;\n struct transport_ls_refs_options;\n@@ -68,16 +69,9 @@ struct remote {\n \tchar *foreign_vcs;\n \n \t/* An array of all of the url_nr URLs configured for the remote */\n-\tconst char **url;\n-\n-\tint url_nr;\n-\tint url_alloc;\n-\n+\tstruct strvec url;\n \t/* An array of all of the pushurl_nr push URLs configured for the remote */\n-\tconst char **pushurl;\n-\n-\tint pushurl_nr;\n-\tint pushurl_alloc;\n+\tstruct strvec pushurl;\n \n \tstruct refspec push;\n \ndiff --git a/t/helper/test-bundle-uri.c b/t/helper/test-bundle-uri.c\nindex 09dc78733c..3285dd962e 100644\n--- a/t/helper/test-bundle-uri.c\n+++ b/t/helper/test-bundle-uri.c\n@@ -88,7 +88,7 @@ static int cmd_ls_remote(int argc, const char **argv)\n \t\t\tdie(_(\"bad repository '%s'\"), dest);\n \t\tdie(_(\"no remote configured to get bundle URIs from\"));\n \t}\n-\tif (!remote->url_nr)\n+\tif (!remote->url.nr)\n \t\tdie(_(\"remote '%s' has no configured URL\"), dest);\n \n \ttransport = transport_get(remote, NULL);\ndiff --git a/transport.c b/transport.c\nindex 83ddea8fbc..eba92eb7e0 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1127,8 +1127,8 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \tret->remote = remote;\n \thelper = remote->foreign_vcs;\n \n-\tif (!url && remote->url)\n-\t\turl = remote->url[0];\n+\tif (!url && remote->url.nr)\n+\t\turl = remote->url.v[0];\n \tret->url = url;\n \n \t/* maybe it is a foreign URL? */\n-- \n2.45.2.937.g0bcb3c087a\n\n"},{"id":"497165","messageId":"20240614102909.GE222445@coredump.intra.peff.net","threadId":"61609","inReplyTo":"20240614102439.GA222287@coredump.intra.peff.net","subject":"[PATCH 05/11] remote: simplify url/pushurl selection","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-14T10:29:09Z","receivedAt":"2024-06-14T10:29:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we want to know the push urls for a remote, there is some simple\nlogic:\n\n - if the user configured any remote.*.pushurl keys, then those make\n the complete set of push urls\n\n - otherwise we push to all urls in remote.*.url\n\nMany spots implement this with a level of indirection, assigning to a\nlocal url/url_nr pair. But since both arrays are now strvecs, we can\njust use a pointer to select the appropriate strvec, shortening the code\na bit.\n\nEven though this is now a one-liner, since it is application logic that\nis present in so many places, it's worth abstracting a helper function.\nIn fact, we already have such a function, but it's local to\nbuiltin/push.c. So we'll just make it available everywhere via remote.h.\n\nThere are two spots to pay special attention to here:\n\n 1. in builtin/remote.c's get_url(), we are selecting first based on\n push_mode and then falling back to \"url\" when we're in push_mode\n but no pushurl is defined. The updated code makes that much more\n clear, compared to the original which had an \"else\" fall-through.\n\n 2. likewise in that file's set_url(), we _only_ respect push_mode,\n sine the point is that we are adding to pushurl in that case\n (whether it is empty or not). And thus it does not use our helper\n function.\n\nSigned-off-by: Jeff King \n---\n builtin/push.c | 21 ++++-----------\n builtin/remote.c | 69 ++++++++++++++----------------------------------\n remote.c | 5 ++++\n remote.h | 1 +\n 4 files changed, 31 insertions(+), 65 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 61b5d3afdd..00d99af1a8 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -141,16 +141,6 @@ static void set_refspecs(const char **refs, int nr, const char *repo)\n \tfree_refs(local_refs);\n }\n \n-static int push_url_of_remote(struct remote *remote, const char ***url_p)\n-{\n-\tif (remote->pushurl.nr) {\n-\t\t*url_p = remote->pushurl.v;\n-\t\treturn remote->pushurl.nr;\n-\t}\n-\t*url_p = remote->url.v;\n-\treturn remote->url.nr;\n-}\n-\n static NORETURN void die_push_simple(struct branch *branch,\n \t\t\t\t struct remote *remote)\n {\n@@ -434,8 +424,7 @@ static int do_push(int flags,\n \t\t struct remote *remote)\n {\n \tint i, errs;\n-\tconst char **url;\n-\tint url_nr;\n+\tstruct strvec *url;\n \tstruct refspec *push_refspec = &rs;\n \n \tif (push_options->nr)\n@@ -448,11 +437,11 @@ static int do_push(int flags,\n \t\t\tsetup_default_push_refspecs(&flags, remote);\n \t}\n \terrs = 0;\n-\turl_nr = push_url_of_remote(remote, &url);\n-\tif (url_nr) {\n-\t\tfor (i = 0; i < url_nr; i++) {\n+\turl = push_url_of_remote(remote);\n+\tif (url->nr) {\n+\t\tfor (i = 0; i < url->nr; i++) {\n \t\t\tstruct transport *transport =\n-\t\t\t\ttransport_get(remote, url[i]);\n+\t\t\t\ttransport_get(remote, url->v[i]);\n \t\t\tif (flags & TRANSPORT_PUSH_OPTIONS)\n \t\t\t\ttransport->push_options = push_options;\n \t\t\tif (push_with_options(transport, push_refspec, flags))\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex ee6a33ff11..06c09ed060 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1213,8 +1213,8 @@ static int get_one_entry(struct remote *remote, void *priv)\n {\n \tstruct string_list *list = priv;\n \tstruct strbuf remote_info_buf = STRBUF_INIT;\n-\tconst char **url;\n-\tint i, url_nr;\n+\tstruct strvec *url;\n+\tint i;\n \n \tif (remote->url.nr > 0) {\n \t\tstruct strbuf promisor_config = STRBUF_INIT;\n@@ -1230,16 +1230,10 @@ static int get_one_entry(struct remote *remote, void *priv)\n \t\t\t\tstrbuf_detach(&remote_info_buf, NULL);\n \t} else\n \t\tstring_list_append(list, remote->name)->util = NULL;\n-\tif (remote->pushurl.nr) {\n-\t\turl = remote->pushurl.v;\n-\t\turl_nr = remote->pushurl.nr;\n-\t} else {\n-\t\turl = remote->url.v;\n-\t\turl_nr = remote->url.nr;\n-\t}\n-\tfor (i = 0; i < url_nr; i++)\n+\turl = push_url_of_remote(remote);\n+\tfor (i = 0; i < url->nr; i++)\n \t{\n-\t\tstrbuf_addf(&remote_info_buf, \"%s (push)\", url[i]);\n+\t\tstrbuf_addf(&remote_info_buf, \"%s (push)\", url->v[i]);\n \t\tstring_list_append(list, remote->name)->util =\n \t\t\t\tstrbuf_detach(&remote_info_buf, NULL);\n \t}\n@@ -1295,28 +1289,21 @@ static int show(int argc, const char **argv, const char *prefix)\n \n \tfor (; argc; argc--, argv++) {\n \t\tint i;\n-\t\tconst char **url;\n-\t\tint url_nr;\n+\t\tstruct strvec *url;\n \n \t\tget_remote_ref_states(*argv, &info.states, query_flag);\n \n \t\tprintf_ln(_(\"* remote %s\"), *argv);\n \t\tprintf_ln(_(\" Fetch URL: %s\"), info.states.remote->url.nr > 0 ?\n \t\t info.states.remote->url.v[0] : _(\"(no URL)\"));\n-\t\tif (info.states.remote->pushurl.nr) {\n-\t\t\turl = info.states.remote->pushurl.v;\n-\t\t\turl_nr = info.states.remote->pushurl.nr;\n-\t\t} else {\n-\t\t\turl = info.states.remote->url.v;\n-\t\t\turl_nr = info.states.remote->url.nr;\n-\t\t}\n-\t\tfor (i = 0; i < url_nr; i++)\n+\t\turl = push_url_of_remote(info.states.remote);\n+\t\tfor (i = 0; i < url->nr; i++)\n \t\t\t/*\n \t\t\t * TRANSLATORS: the colon ':' should align\n \t\t\t * with the one in \" Fetch URL: %s\"\n \t\t\t * translation.\n \t\t\t */\n-\t\t\tprintf_ln(_(\" Push URL: %s\"), url[i]);\n+\t\t\tprintf_ln(_(\" Push URL: %s\"), url->v[i]);\n \t\tif (!i)\n \t\t\tprintf_ln(_(\" Push URL: %s\"), _(\"(no URL)\"));\n \t\tif (no_query)\n@@ -1622,8 +1609,7 @@ static int get_url(int argc, const char **argv, const char *prefix)\n \tint i, push_mode = 0, all_mode = 0;\n \tconst char *remotename = NULL;\n \tstruct remote *remote;\n-\tconst char **url;\n-\tint url_nr;\n+\tstruct strvec *url;\n \tstruct option options[] = {\n \t\tOPT_BOOL('\\0', \"push\", &push_mode,\n \t\t\t N_(\"query push URLs rather than fetch URLs\")),\n@@ -1645,27 +1631,15 @@ static int get_url(int argc, const char **argv, const char *prefix)\n \t\texit(2);\n \t}\n \n-\turl_nr = 0;\n-\tif (push_mode) {\n-\t\turl = remote->pushurl.v;\n-\t\turl_nr = remote->pushurl.nr;\n-\t}\n-\t/* else fetch mode */\n-\n-\t/* Use the fetch URL when no push URLs were found or requested. */\n-\tif (!url_nr) {\n-\t\turl = remote->url.v;\n-\t\turl_nr = remote->url.nr;\n-\t}\n-\n-\tif (!url_nr)\n+\turl = push_mode ? push_url_of_remote(remote) : &remote->url;\n+\tif (!url->nr)\n \t\tdie(_(\"no URLs configured for remote '%s'\"), remotename);\n \n \tif (all_mode) {\n-\t\tfor (i = 0; i < url_nr; i++)\n-\t\t\tprintf_ln(\"%s\", url[i]);\n+\t\tfor (i = 0; i < url->nr; i++)\n+\t\t\tprintf_ln(\"%s\", url->v[i]);\n \t} else {\n-\t\tprintf_ln(\"%s\", *url);\n+\t\tprintf_ln(\"%s\", url->v[0]);\n \t}\n \n \treturn 0;\n@@ -1680,8 +1654,7 @@ static int set_url(int argc, const char **argv, const char *prefix)\n \tconst char *oldurl = NULL;\n \tstruct remote *remote;\n \tregex_t old_regex;\n-\tconst char **urlset;\n-\tint urlset_nr;\n+\tstruct strvec *urlset;\n \tstruct strbuf name_buf = STRBUF_INIT;\n \tstruct option options[] = {\n \t\tOPT_BOOL('\\0', \"push\", &push_mode,\n@@ -1718,12 +1691,10 @@ static int set_url(int argc, const char **argv, const char *prefix)\n \n \tif (push_mode) {\n \t\tstrbuf_addf(&name_buf, \"remote.%s.pushurl\", remotename);\n-\t\turlset = remote->pushurl.v;\n-\t\turlset_nr = remote->pushurl.nr;\n+\t\turlset = &remote->pushurl;\n \t} else {\n \t\tstrbuf_addf(&name_buf, \"remote.%s.url\", remotename);\n-\t\turlset = remote->url.v;\n-\t\turlset_nr = remote->url.nr;\n+\t\turlset = &remote->url;\n \t}\n \n \t/* Special cases that add new entry. */\n@@ -1740,8 +1711,8 @@ static int set_url(int argc, const char **argv, const char *prefix)\n \tif (regcomp(&old_regex, oldurl, REG_EXTENDED))\n \t\tdie(_(\"Invalid old URL pattern: %s\"), oldurl);\n \n-\tfor (i = 0; i < urlset_nr; i++)\n-\t\tif (!regexec(&old_regex, urlset[i], 0, NULL, 0))\n+\tfor (i = 0; i < urlset->nr; i++)\n+\t\tif (!regexec(&old_regex, urlset->v[i], 0, NULL, 0))\n \t\t\tmatches++;\n \t\telse\n \t\t\tnegative_matches++;\ndiff --git a/remote.c b/remote.c\nindex 76a3e41c73..9417d83e51 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -827,6 +827,11 @@ int remote_has_url(struct remote *remote, const char *url)\n \treturn 0;\n }\n \n+struct strvec *push_url_of_remote(struct remote *remote)\n+{\n+\treturn remote->pushurl.nr ? &remote->pushurl : &remote->url;\n+}\n+\n static int match_name_with_pattern(const char *key, const char *name,\n \t\t\t\t const char *value, char **result)\n {\ndiff --git a/remote.h b/remote.h\nindex 84dc91cca0..034f9d6660 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -123,6 +123,7 @@ typedef int each_remote_fn(struct remote *remote, void *priv);\n int for_each_remote(each_remote_fn fn, void *priv);\n \n int remote_has_url(struct remote *remote, const char *url);\n+struct strvec *push_url_of_remote(struct remote *remote);\n \n struct ref_push_report {\n \tconst char *ref_name;\n-- \n2.45.2.937.g0bcb3c087a\n\n"},{"id":"497166","messageId":"20240614103005.GF222445@coredump.intra.peff.net","threadId":"61609","inReplyTo":"20240614102439.GA222287@coredump.intra.peff.net","subject":"[PATCH 06/11] config: document remote.*.url/pushurl interaction","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-14T10:30:05Z","receivedAt":"2024-06-14T10:30:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The documentation for these keys gives a very terse definition and\npoints you to the fetch/push manpages. But from reading those pages it\nwas not at all obvious to me that:\n\n - these are keys that can be defined multiple times with meaningful\n behavior (especially remote.*.url)\n\n - the way that pushurl overrides url (the git-push page does mention\n that \"pushurl defaults to url\", but it is not immediately clear what\n a multi-valued url would do in that situation).\n\nLet's try to summarize the current behavior.\n\nSigned-off-by: Jeff King \n---\n Documentation/config/remote.txt | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\nindex 0678b4bcfe..eef0bf4f62 100644\n--- a/Documentation/config/remote.txt\n+++ b/Documentation/config/remote.txt\n@@ -5,10 +5,16 @@ remote.pushDefault::\n \n remote..url::\n \tThe URL of a remote repository. See linkgit:git-fetch[1] or\n-\tlinkgit:git-push[1].\n+\tlinkgit:git-push[1]. A configured remote can have multiple URLs;\n+\tin this case the first is used for fetching, and all are used\n+\tfor pushing (assuming no `remote..pushurl` is defined).\n \n remote..pushurl::\n \tThe push URL of a remote repository. See linkgit:git-push[1].\n+\tIf a `pushurl` option is present in a configured remote, it\n+\tis used for pushing instead of `remote..url`. A configured\n+\tremote can have multiple push URLs; in this case a push goes to\n+\tall of them.\n \n remote..proxy::\n \tFor remotes that require curl (http, https and ftp), the URL to\n-- \n2.45.2.937.g0bcb3c087a\n\n"},{"id":"497167","messageId":"20240614103122.GG222445@coredump.intra.peff.net","threadId":"61609","inReplyTo":"20240614102439.GA222287@coredump.intra.peff.net","subject":"[PATCH 07/11] remote: allow resetting url list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-14T10:31:22Z","receivedAt":"2024-06-14T10:31:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Because remote.*.url is treated as a multi-valued key, there is no way\nto override previous config. So for example if you have\nremote.origin.url set to some wrong value, doing:\n\n git -c remote.origin.url=right fetch\n\nwould not work. It would append \"right\" to the list, which means we'd\nstill fetch from \"wrong\" (since subsequent values are used only as push\nurls).\n\nLet's provide a mechanism to reset the list, like we do for other\nmulti-valued keys (e.g., credential.helper, http.extraheaders, and\nmerge.suppressDest all use this \"empty string means reset\" pattern).\n\nReported-by: Mathew George \nSigned-off-by: Jeff King \n---\nBy the way, I think the nearby remote.*.fetch and remote.*.push could\nlearn the same trick. I left that out of this series, mostly because it\nwas getting long. But also because I had trouble imagining how a one-off\nrefspec change would be useful. We can revisit it on top if we want.\n\n Documentation/config/remote.txt | 5 ++++-\n remote.c | 10 +++++++--\n t/t5505-remote.sh | 36 +++++++++++++++++++++++++++++++++\n 3 files changed, 48 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\nindex eef0bf4f62..8efc53e836 100644\n--- a/Documentation/config/remote.txt\n+++ b/Documentation/config/remote.txt\n@@ -8,13 +8,16 @@ remote..url::\n \tlinkgit:git-push[1]. A configured remote can have multiple URLs;\n \tin this case the first is used for fetching, and all are used\n \tfor pushing (assuming no `remote..pushurl` is defined).\n+\tSetting this key to the empty string clears the list of urls,\n+\tallowing you to override earlier config.\n \n remote..pushurl::\n \tThe push URL of a remote repository. See linkgit:git-push[1].\n \tIf a `pushurl` option is present in a configured remote, it\n \tis used for pushing instead of `remote..url`. A configured\n \tremote can have multiple push URLs; in this case a push goes to\n-\tall of them.\n+\tall of them. Setting this key to the empty string clears the\n+\tlist of urls, allowing you to override earlier config.\n \n remote..proxy::\n \tFor remotes that require curl (http, https and ftp), the URL to\ndiff --git a/remote.c b/remote.c\nindex 9417d83e51..b7262964fb 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -63,12 +63,18 @@ static char *alias_url(const char *url, struct rewrites *r)\n \n static void add_url(struct remote *remote, const char *url)\n {\n-\tstrvec_push(&remote->url, url);\n+\tif (*url)\n+\t\tstrvec_push(&remote->url, url);\n+\telse\n+\t\tstrvec_clear(&remote->url);\n }\n \n static void add_pushurl(struct remote *remote, const char *pushurl)\n {\n-\tstrvec_push(&remote->pushurl, pushurl);\n+\tif (*pushurl)\n+\t\tstrvec_push(&remote->pushurl, pushurl);\n+\telse\n+\t\tstrvec_clear(&remote->pushurl);\n }\n \n static void add_pushurl_alias(struct remote_state *remote_state,\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 7789ff12c4..08424e878e 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -1492,4 +1492,40 @@ test_expect_success 'refs/remotes/* refspec and unqualified DWIM and\n \t)\n '\n \n+test_expect_success 'empty config clears remote.*.url list' '\n+\ttest_when_finished \"git config --remove-section remote.multi\" &&\n+\tgit config --add remote.multi.url wrong-one &&\n+\tgit config --add remote.multi.url wrong-two &&\n+\tgit -c remote.multi.url= \\\n+\t -c remote.multi.url=right-one \\\n+\t -c remote.multi.url=right-two \\\n+\t remote show -n multi >actual.raw &&\n+\tgrep URL actual.raw >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t Fetch URL: right-one\n+\t Push URL: right-one\n+\t Push URL: right-two\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'empty config clears remote.*.pushurl list' '\n+\ttest_when_finished \"git config --remove-section remote.multi\" &&\n+\tgit config --add remote.multi.url right &&\n+\tgit config --add remote.multi.url will-be-ignored &&\n+\tgit config --add remote.multi.pushurl wrong-push-one &&\n+\tgit config --add remote.multi.pushurl wrong-push-two &&\n+\tgit -c remote.multi.pushurl= \\\n+\t -c remote.multi.pushurl=right-push-one \\\n+\t -c remote.multi.pushurl=right-push-two \\\n+\t remote show -n multi >actual.raw &&\n+\tgrep URL actual.raw >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t Fetch URL: right\n+\t Push URL: right-push-one\n+\t Push URL: right-push-two\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.45.2.937.g0bcb3c087a\n\n"},{"id":"497168","messageId":"20240614103131.GH222445@coredump.intra.peff.net","threadId":"61609","inReplyTo":"20240614102439.GA222287@coredump.intra.peff.net","subject":"[PATCH 08/11] t5801: make remote-testgit GIT_DIR setup more robust","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-14T10:31:31Z","receivedAt":"2024-06-14T10:31:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our tests use a fake helper that just imports from an existing Git\nrepository. We're fed the path to that repo on the command line, and\nderive the GIT_DIR by tacking on \"/.git\".\n\nThis is wrong if the path is a bare repository, but that's OK since this\nis just a limited test. But it's also wrong if the transport code feeds\nus the actual .git directory itself (i.e., we expect \"/path/to/repo\" but\nit gives us \"/path/to/repo/.git\"). None of the current tests do that,\nbut let's future-proof ourselves against adding a test that does.\n\nWe can instead ask \"rev-parse\" to set our GIT_DIR. Note that we have to\nfirst unset other git variables from our environment. Coming into this\nscript, we'll have GIT_DIR set to the fetching repository, and we need\nto \"switch\" to the remote one.\n\nSigned-off-by: Jeff King \n---\n t/t5801/git-remote-testgit | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit\nindex c5b10f5775..f8b476499f 100755\n--- a/t/t5801/git-remote-testgit\n+++ b/t/t5801/git-remote-testgit\n@@ -26,7 +26,8 @@ then\n \tt_refspec=\"\"\n fi\n \n-GIT_DIR=\"$url/.git\"\n+unset $(git rev-parse --local-env-vars)\n+GIT_DIR=$(git -C \"$url\" rev-parse --absolute-git-dir)\n export GIT_DIR\n \n force=\n-- \n2.45.2.937.g0bcb3c087a\n\n"},{"id":"497169","messageId":"20240614103454.GI222445@coredump.intra.peff.net","threadId":"61609","inReplyTo":"20240614102439.GA222287@coredump.intra.peff.net","subject":"[PATCH 09/11] t5801: test remote.*.vcs config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-14T10:34:54Z","receivedAt":"2024-06-14T10:34:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The usual way to trigger a remote helper is to use the \"::\" syntax from:\n87422439d1 (Allow specifying the remote helper in the url, 2009-11-18).\nDoing:\n\n git config remote.origin.url hg::https://example.com/repo\n\nwill run \"git-remote-hg origin https://example.com/repo\". Or you can\nuse the fallback handling from 25d5cc488a (Pass unknown protocols to\nexternal protocol handlers, 2009-12-09):\n\n git config remote.origin.url \"foo://bar\"\n\nwhich will run \"git-remote-foo origin foo://bar\".\n\nBut there's a third way, from c578f51d52 (Add a config option for\nremotes to specify a foreign vcs, 2009-11-18):\n\n git config remote.origin.vcs foo\n git config remote.origin.url bar\n\nwhich will run \"git-remote-foo origin bar\". This is mostly redundant\nwith the other methods, except that it is supposed to allow you to run\nwithout a URL at all. So:\n\n git config remote.origin.vcs foo\n\nwould run \"git-remote-foo origin\" with no extra URL parameter (under the\nassumption that the helper somehow knows how to access the remote repo).\nHowever, this mode has been broken since 25d5cc488a, shortly after it\nwas added! That commit taught the transport code to always look at the\nURL string to parse off the \"foo::\" bits, meaning it would always\nsegfault in the no-url case. You can see that with:\n\n git -c remote.foo.vcs=bar fetch foo\n\nNobody seems to have noticed in the almost 15 years since, so presumably\nit's not a well-used feature. And without that, arguably the whole\nremote.*.vcs feature could be removed entirely, as it isn't offering\nanything you couldn't do with the \"helper::\" syntax. But it _does_ work\nif you have a URL, and it has been advertised in the documentation for\nall that time. So we shouldn't just remove it without warning.\n\nLikewise, even if we were going to deprecate it, we should avoid\nbreaking it in the meantime. Since there are no tests for it at all,\nlet's add a few basic ones:\n\n - this syntax doesn't work well with \"git clone\" (another point\n against it versus \"helper::\"). But we can use \"clone -c\" to set up\n the config manually, passing the URL as usual to clone. This does\n work, though note that I had to use --no-local in the test to avoid\n broken interactions between the local code and the helper. In the\n real world this would be a non-issue, since the remote URL would\n generally not also be a local Git repo!\n\n - likewise, we should be able to set up the config manually and fetch\n into a repository. This also works.\n\n - we can simulate a vcs that has no URL support by stuffing the remote\n path into another environment variable. This should work, but\n doesn't (it hits the segfault mentioned above).\n\nIn the first two cases, I took the extra step of checking GIT_TRACE\noutput to confirm that we actually ran the helper (since the URL is a\nvalid Git repo, the clone/fetch would appear to work even if we\ndidn't use the helper at all!).\n\nSigned-off-by: Jeff King \n---\nI have no real problem with deprecating this, and it might be a nice\nthing to clean up in the long run. But it seemed like less work to just\nfix it in the next patch, so I did that. ;)\n\n t/t5801-remote-helpers.sh | 23 +++++++++++++++++++++++\n t/t5801/git-remote-nourl | 3 +++\n 2 files changed, 26 insertions(+)\n create mode 100755 t/t5801/git-remote-nourl\n\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 4e0a77f985..7c8c4359aa 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -38,6 +38,29 @@ test_expect_success 'cloning from local repo' '\n \ttest_cmp server/file local/file\n '\n \n+test_expect_success 'clone with remote.*.vcs config' '\n+\tGIT_TRACE=$PWD/vcs-clone.trace \\\n+\tgit clone --no-local -c remote.origin.vcs=testgit \"$PWD/server\" vcs-clone &&\n+\ttest_grep remote-testgit vcs-clone.trace\n+'\n+\n+test_expect_success 'fetch with configured remote.*.vcs' '\n+\tgit init vcs-fetch &&\n+\tgit -C vcs-fetch config remote.origin.vcs testgit &&\n+\tgit -C vcs-fetch config remote.origin.url \"$PWD/server\" &&\n+\tGIT_TRACE=$PWD/vcs-fetch.trace \\\n+\tgit -C vcs-fetch fetch origin &&\n+\ttest_grep remote-testgit vcs-fetch.trace\n+'\n+\n+test_expect_failure 'vcs remote with no url' '\n+\tNOURL_UPSTREAM=$PWD/server &&\n+\texport NOURL_UPSTREAM &&\n+\tgit init vcs-nourl &&\n+\tgit -C vcs-nourl config remote.origin.vcs nourl &&\n+\tgit -C vcs-nourl fetch origin\n+'\n+\n test_expect_success 'create new commit on remote' '\n \t(cd server &&\n \t echo content >>file &&\ndiff --git a/t/t5801/git-remote-nourl b/t/t5801/git-remote-nourl\nnew file mode 100755\nindex 0000000000..09be6013c5\n--- /dev/null\n+++ b/t/t5801/git-remote-nourl\n@@ -0,0 +1,3 @@\n+#!/bin/sh\n+\n+exec git-remote-testgit \"$1\" \"$NOURL_UPSTREAM\"\n-- \n2.45.2.937.g0bcb3c087a\n\n"},{"id":"497170","messageId":"20240614103711.GJ222445@coredump.intra.peff.net","threadId":"61609","inReplyTo":"20240614102439.GA222287@coredump.intra.peff.net","subject":"[PATCH 10/11] remote: always require at least one url in a remote","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-14T10:37:11Z","receivedAt":"2024-06-14T10:37:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we return a struct from remote_get(), the result _almost_ always\nhas at least one url. In remotes_remote_get_1(), we do this:\n\n if (name_given && !valid_remote(ret))\n add_url_alias(remote_state, ret, name);\n if (!valid_remote(ret))\n return NULL;\n\nSo if the remote doesn't have a url, we give it one based on the name\n(this is how unconfigured urls are used as remotes). And if that doesn't\nwork, we return NULL.\n\nBut there's a catch: valid_remote() checks that we have at least one url\n_unless_ the remote.*.vcs field is set. This comes from c578f51d52 (Add\na config option for remotes to specify a foreign vcs, 2009-11-18), and\nthe whole idea was to support remote helpers that don't have their own\nurl.\n\nHowever, that mode has been broken since 25d5cc488a (Pass unknown\nprotocols to external protocol handlers, 2009-12-09)! That commit\nunconditionally looks at the url in get_helper(), causing a segfault\nwith something like:\n\n git -c remote.foo.vcs=bar fetch foo\n\nWe could fix that now, of course. But given that it has been broken for\nalmost 15 years and nobody noticed, there's a better option. This weird\n\"there might not be a url\" special case requires checks all over the\ncode base, and it's not clear if there are other similar segfaults\nlurking. It would be nice if we could drop that special case.\n\nSo instead, let's let the \"the remote name is the url\" code kick in. If\nyou have \"remote.foo.vcs\", then your url (unless otherwise configured)\nis \"foo\". This does have a visible effect compared to what 25d5cc488a\nwas trying to do. The idea back then is that for a remote without a url,\nwe'd run:\n\n # only one command-line option!\n git-remote-bar foo\n\nwhereas with our default url, now we'll run:\n\n git-remote-bar foo foo\n\nAgain, in practice nobody can be relying on this because it has been\nsegfaulting for 15 years. We should consider just removing this \"vcs\"\nconfig option entirely, but that would be a user-visible breakage. So by\nfixing it this way, we can keep things working that have been working,\nand simplify away one special case inside our code.\n\nThis fixes the segfault from 25d5cc488a (demonstrated by the test), and\nwe can build further cleanups on top.\n\nSigned-off-by: Jeff King \n---\n remote.c | 2 +-\n t/t5801-remote-helpers.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex b7262964fb..5fa046c8f8 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -32,7 +32,7 @@ struct counted_string {\n \n static int valid_remote(const struct remote *remote)\n {\n-\treturn (!!remote->url.nr) || (!!remote->foreign_vcs);\n+\treturn !!remote->url.nr;\n }\n \n static char *alias_url(const char *url, struct rewrites *r)\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 7c8c4359aa..20f43f7b7d 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -53,7 +53,7 @@ test_expect_success 'fetch with configured remote.*.vcs' '\n \ttest_grep remote-testgit vcs-fetch.trace\n '\n \n-test_expect_failure 'vcs remote with no url' '\n+test_expect_success 'vcs remote with no url' '\n \tNOURL_UPSTREAM=$PWD/server &&\n \texport NOURL_UPSTREAM &&\n \tgit init vcs-nourl &&\n-- \n2.45.2.937.g0bcb3c087a\n\n"},{"id":"497171","messageId":"20240614104203.GK222445@coredump.intra.peff.net","threadId":"61609","inReplyTo":"20240614102439.GA222287@coredump.intra.peff.net","subject":"[PATCH 11/11] remote: drop checks for zero-url case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-14T10:42:03Z","receivedAt":"2024-06-14T10:42:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Now that the previous commit removed the possibility that a \"struct\nremote\" will ever have zero url fields, we can drop a number of\nredundant checks and untriggerable code paths.\n\nSigned-off-by: Jeff King \n---\nNote for reviewers: the hunk in builtin/push.c is funny. The original\ncode was:\n\n if (url->nr) {\n\tfor (i = 0; i < url->nr; i++) {\n\t\tdo this\n\t}\n } else {\n do that\n }\n\nAnd I removed the \"do that\" part along with the if/else to become:\n\n for (i = 0; i < url->nr; i++) {\n\tdo this\n }\n\n\nBut because \"this\" and \"that\" were so similar, and because the\nindentation of \"this\" in the loop was now the same of the old \"that\",\nthe diff makes it look like I dropped the first half of the conditional.\n\n builtin/archive.c | 2 --\n builtin/ls-remote.c | 2 --\n builtin/push.c | 13 ++-----------\n builtin/remote.c | 13 +++----------\n t/helper/test-bundle-uri.c | 2 --\n transport.c | 17 +++++++----------\n 6 files changed, 12 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex 0d9aff7e6f..7234607aaa 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -31,8 +31,6 @@ static int run_remote_archiver(int argc, const char **argv,\n \tstruct packet_reader reader;\n \n \t_remote = remote_get(remote);\n-\tif (!_remote->url.nr)\n-\t\tdie(_(\"git archive: Remote with no URL\"));\n \ttransport = transport_get(_remote, _remote->url.v[0]);\n \ttransport_connect(transport, \"git-upload-archive\", exec, fd);\n \ndiff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\nindex 4c04dbbf19..8f3a64d838 100644\n--- a/builtin/ls-remote.c\n+++ b/builtin/ls-remote.c\n@@ -109,8 +109,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \t\t\tdie(\"bad repository '%s'\", dest);\n \t\tdie(\"No remote configured to list refs from.\");\n \t}\n-\tif (!remote->url.nr)\n-\t\tdie(\"remote %s has no configured URL\", dest);\n \n \tif (get_url) {\n \t\tprintf(\"%s\\n\", remote->url.v[0]);\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 00d99af1a8..8260c6e46a 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -438,18 +438,9 @@ static int do_push(int flags,\n \t}\n \terrs = 0;\n \turl = push_url_of_remote(remote);\n-\tif (url->nr) {\n-\t\tfor (i = 0; i < url->nr; i++) {\n-\t\t\tstruct transport *transport =\n-\t\t\t\ttransport_get(remote, url->v[i]);\n-\t\t\tif (flags & TRANSPORT_PUSH_OPTIONS)\n-\t\t\t\ttransport->push_options = push_options;\n-\t\t\tif (push_with_options(transport, push_refspec, flags))\n-\t\t\t\terrs++;\n-\t\t}\n-\t} else {\n+\tfor (i = 0; i < url->nr; i++) {\n \t\tstruct transport *transport =\n-\t\t\ttransport_get(remote, NULL);\n+\t\t\ttransport_get(remote, url->v[i]);\n \t\tif (flags & TRANSPORT_PUSH_OPTIONS)\n \t\t\ttransport->push_options = push_options;\n \t\tif (push_with_options(transport, push_refspec, flags))\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 06c09ed060..c5c94d4dbd 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1002,8 +1002,7 @@ static int get_remote_ref_states(const char *name,\n \t\tstruct transport *transport;\n \t\tconst struct ref *remote_refs;\n \n-\t\ttransport = transport_get(states->remote, states->remote->url.nr > 0 ?\n-\t\t\tstates->remote->url.v[0] : NULL);\n+\t\ttransport = transport_get(states->remote, states->remote->url.v[0]);\n \t\tremote_refs = transport_get_remote_refs(transport, NULL);\n \n \t\tstates->queried = 1;\n@@ -1294,8 +1293,7 @@ static int show(int argc, const char **argv, const char *prefix)\n \t\tget_remote_ref_states(*argv, &info.states, query_flag);\n \n \t\tprintf_ln(_(\"* remote %s\"), *argv);\n-\t\tprintf_ln(_(\" Fetch URL: %s\"), info.states.remote->url.nr > 0 ?\n-\t\t info.states.remote->url.v[0] : _(\"(no URL)\"));\n+\t\tprintf_ln(_(\" Fetch URL: %s\"), info.states.remote->url.v[0]);\n \t\turl = push_url_of_remote(info.states.remote);\n \t\tfor (i = 0; i < url->nr; i++)\n \t\t\t/*\n@@ -1440,10 +1438,7 @@ static int prune_remote(const char *remote, int dry_run)\n \t}\n \n \tprintf_ln(_(\"Pruning %s\"), remote);\n-\tprintf_ln(_(\"URL: %s\"),\n-\t\t states.remote->url.nr\n-\t\t ? states.remote->url.v[0]\n-\t\t : _(\"(no URL)\"));\n+\tprintf_ln(_(\"URL: %s\"), states.remote->url.v[0]);\n \n \tfor_each_string_list_item(item, &states.stale)\n \t\tstring_list_append(&refs_to_prune, item->util);\n@@ -1632,8 +1627,6 @@ static int get_url(int argc, const char **argv, const char *prefix)\n \t}\n \n \turl = push_mode ? push_url_of_remote(remote) : &remote->url;\n-\tif (!url->nr)\n-\t\tdie(_(\"no URLs configured for remote '%s'\"), remotename);\n \n \tif (all_mode) {\n \t\tfor (i = 0; i < url->nr; i++)\ndiff --git a/t/helper/test-bundle-uri.c b/t/helper/test-bundle-uri.c\nindex 3285dd962e..0c5fa723d8 100644\n--- a/t/helper/test-bundle-uri.c\n+++ b/t/helper/test-bundle-uri.c\n@@ -88,8 +88,6 @@ static int cmd_ls_remote(int argc, const char **argv)\n \t\t\tdie(_(\"bad repository '%s'\"), dest);\n \t\tdie(_(\"no remote configured to get bundle URIs from\"));\n \t}\n-\tif (!remote->url.nr)\n-\t\tdie(_(\"remote '%s' has no configured URL\"), dest);\n \n \ttransport = transport_get(remote, NULL);\n \tif (transport_get_remote_bundle_uri(transport) < 0) {\ndiff --git a/transport.c b/transport.c\nindex eba92eb7e0..a324045240 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1112,6 +1112,7 @@ static struct transport_vtable builtin_smart_vtable = {\n struct transport *transport_get(struct remote *remote, const char *url)\n {\n \tconst char *helper;\n+\tconst char *p;\n \tstruct transport *ret = xcalloc(1, sizeof(*ret));\n \n \tret->progress = isatty(2);\n@@ -1127,19 +1128,15 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \tret->remote = remote;\n \thelper = remote->foreign_vcs;\n \n-\tif (!url && remote->url.nr)\n+\tif (!url)\n \t\turl = remote->url.v[0];\n \tret->url = url;\n \n-\t/* maybe it is a foreign URL? */\n-\tif (url) {\n-\t\tconst char *p = url;\n-\n-\t\twhile (is_urlschemechar(p == url, *p))\n-\t\t\tp++;\n-\t\tif (starts_with(p, \"::\"))\n-\t\t\thelper = xstrndup(url, p - url);\n-\t}\n+\tp = url;\n+\twhile (is_urlschemechar(p == url, *p))\n+\t\tp++;\n+\tif (starts_with(p, \"::\"))\n+\t\thelper = xstrndup(url, p - url);\n \n \tif (helper) {\n \t\ttransport_helper_init(ret, helper);\n-- \n2.45.2.937.g0bcb3c087a\n"},{"id":"497178","messageId":"xmqq1q4zh2vh.fsf@gitster.g","threadId":"61609","inReplyTo":"20240614102722.GC222445@coredump.intra.peff.net","subject":"Re: [PATCH 03/11] remote: transfer ownership of memory in add_url(), etc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-14T17:04:50Z","receivedAt":"2024-06-14T17:04:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/remote.c b/remote.c\n> index fd9d58f820..f7c846865f 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -64,13 +64,13 @@ static char *alias_url(const char *url, struct rewrites *r)\n> static void add_url(struct remote *remote, const char *url)\n> {\n> \tALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);\n> -\tremote->url[remote->url_nr++] = url;\n> +\tremote->url[remote->url_nr++] = xstrdup(url);\n> }\n> \n> static void add_pushurl(struct remote *remote, const char *pushurl)\n> {\n> \tALLOC_GROW(remote->pushurl, remote->pushurl_nr + 1, remote->pushurl_alloc);\n> -\tremote->pushurl[remote->pushurl_nr++] = pushurl;\n> +\tremote->pushurl[remote->pushurl_nr++] = xstrdup(pushurl);\n> }\n\nOK. This makes it easier to reason about why these elements are\nfreed in remote_clear().\n\n> static void add_pushurl_alias(struct remote_state *remote_state,\n> @@ -79,6 +79,7 @@ static void add_pushurl_alias(struct remote_state *remote_state,\n> \tchar *alias = alias_url(url, &remote_state->rewrites_push);\n> \tif (alias)\n> \t\tadd_pushurl(remote, alias);\n> +\tfree(alias);\n> }\n\nOK. I wondered if we want to strdup(url) in my review on the\nprevious step, but now we are making the add_url() responsible\nfor making a copy, we instead do the opposite, i.e. free alias\nthat was allocated for us because we no longer need it.\n\n> static void add_url_alias(struct remote_state *remote_state,\n> @@ -87,6 +88,7 @@ static void add_url_alias(struct remote_state *remote_state,\n> \tchar *alias = alias_url(url, &remote_state->rewrites);\n> \tadd_url(remote, alias ? alias : url);\n> \tadd_pushurl_alias(remote_state, remote, url);\n> +\tfree(alias);\n> }\n\nLikewise.\n\n> @@ -336,7 +338,7 @@ static void read_branches_file(struct remote_state *remote_state,\n> \telse\n> \t\tfrag = to_free = repo_default_branch_name(the_repository, 0);\n> \n> -\tadd_url_alias(remote_state, remote, strbuf_detach(&buf, NULL));\n> +\tadd_url_alias(remote_state, remote, buf.buf);\n\nIt is curious that you delay ...\n\n> @@ -347,6 +349,7 @@ static void read_branches_file(struct remote_state *remote_state,\n> \trefspec_appendf(&remote->push, \"HEAD:refs/heads/%s\", frag);\n> \tremote->fetch_tags = 1; /* always auto-follow */\n> \n> +\tstrbuf_release(&buf);\n> \tfree(to_free);\n> }\n\n... strbuf_release() of the buf to the very end of the function. \n\nIn the original, buf became invalid by doing strbuf_detach(), so we\ncould do strbuf_release() immediately after add_url_alias() returns\nto us if we wanted to.\n\n> @@ -431,15 +434,13 @@ static int handle_config(const char *key, const char *value,\n> \telse if (!strcmp(subkey, \"prunetags\"))\n> \t\tremote->prune_tags = git_config_bool(key, value);\n> \telse if (!strcmp(subkey, \"url\")) {\n> -\t\tchar *v;\n> -\t\tif (git_config_string(&v, key, value))\n> -\t\t\treturn -1;\n> -\t\tadd_url(remote, v);\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(key);\n> +\t\tadd_url(remote, value);\n\nOK. config_string() does (1) check for \"I exist hence I am true\"\nboolean, and (2) give us a duplicate of the value. We do not want\nthe latter, so we do the former ourselves here. The same story\nrepeats for pushurl below (ellided).\n\n> @@ -495,8 +496,10 @@ static void alias_all_urls(struct remote_state *remote_state)\n> \t\tfor (j = 0; j < remote_state->remotes[i]->pushurl_nr; j++) {\n> \t\t\tchar *alias = alias_url(remote_state->remotes[i]->pushurl[j],\n> \t\t\t\t\t\t&remote_state->rewrites);\n> -\t\t\tif (alias)\n> +\t\t\tif (alias) {\n> +\t\t\t\tfree((char *)remote_state->remotes[i]->pushurl[j]);\n> \t\t\t\tremote_state->remotes[i]->pushurl[j] = alias;\n> +\t\t\t}\n> \t\t}\n\nOK, this is the replacement codepath we saw earlier. Makes sense\nfor this and the .url[] side on the other hunk (it is curious that\nthis has .pushurl[] before .url[], which is the opposite order of\nhow everybody else deals with them, as pushurl came later).\n\nThanks.\n"},{"id":"497179","messageId":"xmqqtthvfoaq.fsf@gitster.g","threadId":"61609","inReplyTo":"20240614102616.GB222445@coredump.intra.peff.net","subject":"Re: [PATCH 02/11] remote: refactor alias_url() memory ownership","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-14T17:05:01Z","receivedAt":"2024-06-14T17:05:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... So instead of\n> returning the original string, return NULL, forcing callers to decide\n> what to do with it explicitly. We can then build further cleanups on top\n> of that.\n\nOK.\n\n> @@ -76,15 +76,16 @@ static void add_pushurl(struct remote *remote, const char *pushurl)\n> static void add_pushurl_alias(struct remote_state *remote_state,\n> \t\t\t struct remote *remote, const char *url)\n> {\n> -\tconst char *pushurl = alias_url(url, &remote_state->rewrites_push);\n> -\tif (pushurl != url)\n> -\t\tadd_pushurl(remote, pushurl);\n> +\tchar *alias = alias_url(url, &remote_state->rewrites_push);\n> +\tif (alias)\n> +\t\tadd_pushurl(remote, alias);\n\nOK, that's an obviously equivalent rewrite.\n\n> static void add_url_alias(struct remote_state *remote_state,\n> \t\t\t struct remote *remote, const char *url)\n> {\n> -\tadd_url(remote, alias_url(url, &remote_state->rewrites));\n> +\tchar *alias = alias_url(url, &remote_state->rewrites);\n> +\tadd_url(remote, alias ? alias : url);\n> \tadd_pushurl_alias(remote_state, remote, url);\n> }\n\nThis is also an obviously equivalent rewrite.\n\nLooking at how remote_clear() deals with the .url[] and .pushurl[]\nelements, add_url() makes the remote structure take ownership, which\nis perfectly fine when we got a non-NULL alias back (i.e. it is a\nnew piece of string allocated just for us). Depending on who owns\nthe incoming url parameter, we might need strdup(url) here, but\nsince we haven't heard crashes due to freeing remote->url[] elements\nthat should not be freed, presumably url is a piece memory the\ncaller is giving us the ownership? In any case, I imagine that\nuntangling that ownership mess is left to the later steps of the\nseries.\n\n> @@ -492,19 +493,22 @@ static void alias_all_urls(struct remote_state *remote_state)\n> \t\tif (!remote_state->remotes[i])\n> \t\t\tcontinue;\n> \t\tfor (j = 0; j < remote_state->remotes[i]->pushurl_nr; j++) {\n> -\t\t\tremote_state->remotes[i]->pushurl[j] =\n> -\t\t\t\talias_url(remote_state->remotes[i]->pushurl[j],\n> -\t\t\t\t\t &remote_state->rewrites);\n> +\t\t\tchar *alias = alias_url(remote_state->remotes[i]->pushurl[j],\n> +\t\t\t\t\t\t&remote_state->rewrites);\n> +\t\t\tif (alias)\n> +\t\t\t\tremote_state->remotes[i]->pushurl[j] = alias;\n\nDoes this change behaviour? No. We used to (1) grab the current\nvalue, (2) replace it if it is an alias, or (3) otherwise replace it\nwith itself. We just do not do the obvious noop anymore.\n\nThe story is the same with the last remaing hunk of this patch.\nLooking good.\n\nThanks.\n\n> \t\t}\n> \t\tadd_pushurl_aliases = remote_state->remotes[i]->pushurl_nr == 0;\n> \t\tfor (j = 0; j < remote_state->remotes[i]->url_nr; j++) {\n> +\t\t\tchar *alias;\n> \t\t\tif (add_pushurl_aliases)\n> \t\t\t\tadd_pushurl_alias(\n> \t\t\t\t\tremote_state, remote_state->remotes[i],\n> \t\t\t\t\tremote_state->remotes[i]->url[j]);\n> -\t\t\tremote_state->remotes[i]->url[j] =\n> -\t\t\t\talias_url(remote_state->remotes[i]->url[j],\n> +\t\t\talias = alias_url(remote_state->remotes[i]->url[j],\n> \t\t\t\t\t &remote_state->rewrites);\n> +\t\t\tif (alias)\n> +\t\t\t\tremote_state->remotes[i]->url[j] = alias;\n> \t\t}\n> \t}\n> }\n"},{"id":"497218","messageId":"20240616045937.GB17750@coredump.intra.peff.net","threadId":"61609","inReplyTo":"xmqq1q4zh2vh.fsf@gitster.g","subject":"Re: [PATCH 03/11] remote: transfer ownership of memory in add_url(), etc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-16T04:59:37Z","receivedAt":"2024-06-16T04:59:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 14, 2024 at 10:04:50AM -0700, Junio C Hamano wrote:\n\n> > static void add_pushurl_alias(struct remote_state *remote_state,\n> > @@ -79,6 +79,7 @@ static void add_pushurl_alias(struct remote_state *remote_state,\n> > \tchar *alias = alias_url(url, &remote_state->rewrites_push);\n> > \tif (alias)\n> > \t\tadd_pushurl(remote, alias);\n> > +\tfree(alias);\n> > }\n> \n> OK. I wondered if we want to strdup(url) in my review on the\n> previous step, but now we are making the add_url() responsible\n> for making a copy, we instead do the opposite, i.e. free alias\n> that was allocated for us because we no longer need it.\n\nYeah. Possibly the two should be squashed. I was trying to make this\npatch a little less long/confusing, but maybe breaking things up just\nposed new questions. :)\n\n> > @@ -336,7 +338,7 @@ static void read_branches_file(struct remote_state *remote_state,\n> > \telse\n> > \t\tfrag = to_free = repo_default_branch_name(the_repository, 0);\n> > \n> > -\tadd_url_alias(remote_state, remote, strbuf_detach(&buf, NULL));\n> > +\tadd_url_alias(remote_state, remote, buf.buf);\n> \n> It is curious that you delay ...\n> \n> > @@ -347,6 +349,7 @@ static void read_branches_file(struct remote_state *remote_state,\n> > \trefspec_appendf(&remote->push, \"HEAD:refs/heads/%s\", frag);\n> > \tremote->fetch_tags = 1; /* always auto-follow */\n> > \n> > +\tstrbuf_release(&buf);\n> > \tfree(to_free);\n> > }\n> \n> ... strbuf_release() of the buf to the very end of the function. \n> \n> In the original, buf became invalid by doing strbuf_detach(), so we\n> could do strbuf_release() immediately after add_url_alias() returns\n> to us if we wanted to.\n\nRight. I had originally written it that way, since that would be the\nmechanical conversion. But since there was already cleanup at the bottom\nof the function, it felt more natural to shuffle it there. Which is\ncorrect as long as there are no other references to buf nor early\nreturns. You can't see that from the context, but it is true in this\ncase.\n\nBumping to --inter-hunk-context=4 does it a little more obvious.\n\n> > @@ -431,15 +434,13 @@ static int handle_config(const char *key, const char *value,\n> > \telse if (!strcmp(subkey, \"prunetags\"))\n> > \t\tremote->prune_tags = git_config_bool(key, value);\n> > \telse if (!strcmp(subkey, \"url\")) {\n> > -\t\tchar *v;\n> > -\t\tif (git_config_string(&v, key, value))\n> > -\t\t\treturn -1;\n> > -\t\tadd_url(remote, v);\n> > +\t\tif (!value)\n> > +\t\t\treturn config_error_nonbool(key);\n> > +\t\tadd_url(remote, value);\n> \n> OK. config_string() does (1) check for \"I exist hence I am true\"\n> boolean, and (2) give us a duplicate of the value. We do not want\n> the latter, so we do the former ourselves here. The same story\n> repeats for pushurl below (ellided).\n\nYep. If we followed through on the \"bool means reset\" approach, then these\nextra NULL checks would go away in favor of one in add_url(). I didn't\npursue that here. And if we do, I think we should do it in all the other\nspots, too, for consistency.\n\n-Peff\n"},{"id":"497249","messageId":"xmqqv827a2ke.fsf@gitster.g","threadId":"61609","inReplyTo":"20240616045937.GB17750@coredump.intra.peff.net","subject":"Re: [PATCH 03/11] remote: transfer ownership of memory in add_url(), etc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-17T17:42:25Z","receivedAt":"2024-06-17T17:43:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Jun 14, 2024 at 10:04:50AM -0700, Junio C Hamano wrote:\n>\n>> > static void add_pushurl_alias(struct remote_state *remote_state,\n>> > @@ -79,6 +79,7 @@ static void add_pushurl_alias(struct remote_state *remote_state,\n>> > \tchar *alias = alias_url(url, &remote_state->rewrites_push);\n>> > \tif (alias)\n>> > \t\tadd_pushurl(remote, alias);\n>> > +\tfree(alias);\n>> > }\n>> \n>> OK. I wondered if we want to strdup(url) in my review on the\n>> previous step, but now we are making the add_url() responsible\n>> for making a copy, we instead do the opposite, i.e. free alias\n>> that was allocated for us because we no longer need it.\n>\n> Yeah. Possibly the two should be squashed. I was trying to make this\n> patch a little less long/confusing, but maybe breaking things up just\n> posed new questions. :)\n\nNo squashing is needed. It's just that [02/11] could go in either\ndirection and the reader was held in suspense until [03/11] that\npicked one direction to go ;-).\n\n> Right. I had originally written it that way, since that would be the\n> mechanical conversion. But since there was already cleanup at the bottom\n> of the function, it felt more natural to shuffle it there. Which is\n> correct as long as there are no other references to buf nor early\n> returns. You can't see that from the context, but it is true in this\n> case.\n\nYeah, either way it is correct, and I think the \"cleanup at the end,\nwhere the single label is there for any new error code paths to jump\nto\" pattern is a good approach going forward.\n\nLooking good.\n"},{"id":"497629","messageId":"CABPp-BHzT0dwsV3bZ4TX74JzQyhq6zHLdFtNQCxFdgEmGidh=A@mail.gmail.com","threadId":"61609","inReplyTo":"20240614102722.GC222445@coredump.intra.peff.net","subject":"Re: [PATCH 03/11] remote: transfer ownership of memory in add_url(), etc","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-06-25T17:30:48Z","receivedAt":"2024-06-25T17:31:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jun 14, 2024 at 4:08 AM Jeff King wrote:\n>\n> Many of the internal functions in remote.c take const strings and store\n> them forever in instances of \"struct remote\". Since the functions are\n> internal and callers are aware of the convention, this seems to mostly\n> work and not cause leaks. But there are some issues:\n>\n> - it's impossible to clear any of the arrays, because the data\n> dependencies between them are too muddled (if you free() a string,\n> it might also be referenced from another array, causing a\n> user-after-free; but if you don't, that might be the last reference,\n\ns/user-after-free/use-after-free/\n\n(Read the rest of the patch and it all looks good.)\n"},{"id":"497631","messageId":"CABPp-BF0Gt5QsbfepJ-NEwdNs_YXeSwpbKXJEkjaXgO47OVx6g@mail.gmail.com","threadId":"61609","inReplyTo":"20240614102801.GD222445@coredump.intra.peff.net","subject":"Re: [PATCH 04/11] remote: use strvecs to store remote url/pushurl","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-06-25T17:32:24Z","receivedAt":"2024-06-25T17:32:36Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jun 14, 2024 at 3:32 AM Jeff King wrote:\n>\n> Now that the url/pushurl fields of \"struct remote\" own their strings, we\n> can switch from bare arrays to strvecs. This has a few advantages:\n>\n> - push/clear are now one-liners\n>\n> - likewise the free+assigns in alias_all_urls() can use\n> strvec_replace()\n>\n> - we now use size_t for storage, avoiding possible overflow\n>\n> - this will enable some further cleanups in future patches\n>\n> There's quite a bit of fallout in the code that reads these fields, as\n> it tends to access these arrays directly. But it's mostly a mechanical\n> replacement of \"url_nr\" with \"url.nr\", and \"url[i]\" with \"url.v[i]\",\n> with a few variations (e.g. \"*url\" could become \"*url.v\", but I used\n> \"url.v[0]\" for consistency).\n>\n> Signed-off-by: Jeff King \n> ---\n> builtin/archive.c | 4 +--\n> builtin/clone.c | 4 +--\n> builtin/ls-remote.c | 6 ++--\n> builtin/push.c | 10 +++----\n> builtin/remote.c | 56 +++++++++++++++++++-------------------\n> remote-curl.c | 2 +-\n> remote.c | 52 ++++++++++++++---------------------\n> remote.h | 12 ++------\n> t/helper/test-bundle-uri.c | 2 +-\n> transport.c | 4 +--\n> 10 files changed, 68 insertions(+), 84 deletions(-)\n>\n[...]\n\nI really like the simplifications this provides in remote.[hc]; the\nrest all looks like straightforward translations.\n"},{"id":"497632","messageId":"CABPp-BHFmNXyV-7S_CcQLqW=J+1fPA6xOVsYGQvY3quusbzD=g@mail.gmail.com","threadId":"61609","inReplyTo":"20240614102909.GE222445@coredump.intra.peff.net","subject":"Re: [PATCH 05/11] remote: simplify url/pushurl selection","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-06-25T17:33:42Z","receivedAt":"2024-06-25T17:33:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jun 14, 2024 at 3:32 AM Jeff King wrote:\n>\n[...]\n> There are two spots to pay special attention to here:\n>\n> 1. in builtin/remote.c's get_url(), we are selecting first based on\n> push_mode and then falling back to \"url\" when we're in push_mode\n> but no pushurl is defined. The updated code makes that much more\n> clear, compared to the original which had an \"else\" fall-through.\n>\n> 2. likewise in that file's set_url(), we _only_ respect push_mode,\n> sine the point is that we are adding to pushurl in that case\n\ns/sine/since/\n\n> (whether it is empty or not). And thus it does not use our helper\n> function.\n>\n> Signed-off-by: Jeff King \n> ---\n> builtin/push.c | 21 ++++-----------\n> builtin/remote.c | 69 ++++++++++++++----------------------------------\n> remote.c | 5 ++++\n> remote.h | 1 +\n> 4 files changed, 31 insertions(+), 65 deletions(-)\n>\n[...]\n\nI like the simplifications in this patch; it all looks good to me.\n"},{"id":"497633","messageId":"CABPp-BHeBj=vcOMaVAw61YcERyGrDSX8TVXQJn+xNTW12HpcAg@mail.gmail.com","threadId":"61609","inReplyTo":"20240614103005.GF222445@coredump.intra.peff.net","subject":"Re: [PATCH 06/11] config: document remote.*.url/pushurl interaction","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-06-25T17:34:00Z","receivedAt":"2024-06-25T17:34:12Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jun 14, 2024 at 3:43 AM Jeff King wrote:\n>\n> The documentation for these keys gives a very terse definition and\n> points you to the fetch/push manpages. But from reading those pages it\n> was not at all obvious to me that:\n>\n> - these are keys that can be defined multiple times with meaningful\n> behavior (especially remote.*.url)\n>\n> - the way that pushurl overrides url (the git-push page does mention\n> that \"pushurl defaults to url\", but it is not immediately clear what\n> a multi-valued url would do in that situation).\n>\n> Let's try to summarize the current behavior.\n>\n> Signed-off-by: Jeff King \n> ---\n> Documentation/config/remote.txt | 8 +++++++-\n> 1 file changed, 7 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\n> index 0678b4bcfe..eef0bf4f62 100644\n> --- a/Documentation/config/remote.txt\n> +++ b/Documentation/config/remote.txt\n> @@ -5,10 +5,16 @@ remote.pushDefault::\n>\n> remote..url::\n> The URL of a remote repository. See linkgit:git-fetch[1] or\n> - linkgit:git-push[1].\n> + linkgit:git-push[1]. A configured remote can have multiple URLs;\n> + in this case the first is used for fetching, and all are used\n> + for pushing (assuming no `remote..pushurl` is defined).\n>\n> remote..pushurl::\n> The push URL of a remote repository. See linkgit:git-push[1].\n> + If a `pushurl` option is present in a configured remote, it\n> + is used for pushing instead of `remote..url`. A configured\n> + remote can have multiple push URLs; in this case a push goes to\n> + all of them.\n>\n> remote..proxy::\n> For remotes that require curl (http, https and ftp), the URL to\n> --\n> 2.45.2.937.g0bcb3c087a\n\nI was unaware of these facts prior to reading this series, and I've\nread the documentation multiple times and occasionally even glanced at\nnearby code. So, definitely a welcome documentation addition.\n"},{"id":"497634","messageId":"CABPp-BHDVarb8V+zMmWYdMVgypvmbPLCecPRQ3y9DGKngZzyGQ@mail.gmail.com","threadId":"61609","inReplyTo":"20240614103122.GG222445@coredump.intra.peff.net","subject":"Re: [PATCH 07/11] remote: allow resetting url list","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-06-25T17:35:37Z","receivedAt":"2024-06-25T17:35:49Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jun 14, 2024 at 3:32 AM Jeff King wrote:\n>\n> Because remote.*.url is treated as a multi-valued key, there is no way\n> to override previous config. So for example if you have\n> remote.origin.url set to some wrong value, doing:\n>\n> git -c remote.origin.url=right fetch\n>\n> would not work. It would append \"right\" to the list, which means we'd\n> still fetch from \"wrong\" (since subsequent values are used only as push\n> urls).\n>\n> Let's provide a mechanism to reset the list, like we do for other\n> multi-valued keys (e.g., credential.helper, http.extraheaders, and\n> merge.suppressDest all use this \"empty string means reset\" pattern).\n>\n> Reported-by: Mathew George \n> Signed-off-by: Jeff King \n> ---\n> By the way, I think the nearby remote.*.fetch and remote.*.push could\n> learn the same trick. I left that out of this series, mostly because it\n> was getting long. But also because I had trouble imagining how a one-off\n> refspec change would be useful. We can revisit it on top if we want.\n\nYeah, deferring makes sense to me.\n\n> Documentation/config/remote.txt | 5 ++++-\n> remote.c | 10 +++++++--\n> t/t5505-remote.sh | 36 +++++++++++++++++++++++++++++++++\n> 3 files changed, 48 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\n> index eef0bf4f62..8efc53e836 100644\n> --- a/Documentation/config/remote.txt\n> +++ b/Documentation/config/remote.txt\n> @@ -8,13 +8,16 @@ remote..url::\n> linkgit:git-push[1]. A configured remote can have multiple URLs;\n> in this case the first is used for fetching, and all are used\n> for pushing (assuming no `remote..pushurl` is defined).\n> + Setting this key to the empty string clears the list of urls,\n> + allowing you to override earlier config.\n>\n> remote..pushurl::\n> The push URL of a remote repository. See linkgit:git-push[1].\n> If a `pushurl` option is present in a configured remote, it\n> is used for pushing instead of `remote..url`. A configured\n> remote can have multiple push URLs; in this case a push goes to\n> - all of them.\n> + all of them. Setting this key to the empty string clears the\n> + list of urls, allowing you to override earlier config.\n>\n> remote..proxy::\n> For remotes that require curl (http, https and ftp), the URL to\n\nThanks for documenting this too.\n\n> diff --git a/remote.c b/remote.c\n> index 9417d83e51..b7262964fb 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -63,12 +63,18 @@ static char *alias_url(const char *url, struct rewrites *r)\n>\n> static void add_url(struct remote *remote, const char *url)\n> {\n> - strvec_push(&remote->url, url);\n> + if (*url)\n> + strvec_push(&remote->url, url);\n> + else\n> + strvec_clear(&remote->url);\n> }\n>\n> static void add_pushurl(struct remote *remote, const char *pushurl)\n> {\n> - strvec_push(&remote->pushurl, pushurl);\n> + if (*pushurl)\n> + strvec_push(&remote->pushurl, pushurl);\n> + else\n> + strvec_clear(&remote->pushurl);\n> }\n\nNice that after the preparation patches the fix is nice and simple.\n\n> static void add_pushurl_alias(struct remote_state *remote_state,\n> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\n> index 7789ff12c4..08424e878e 100755\n> --- a/t/t5505-remote.sh\n> +++ b/t/t5505-remote.sh\n> @@ -1492,4 +1492,40 @@ test_expect_success 'refs/remotes/* refspec and unqualified DWIM and\n> )\n> '\n>\n> +test_expect_success 'empty config clears remote.*.url list' '\n> + test_when_finished \"git config --remove-section remote.multi\" &&\n> + git config --add remote.multi.url wrong-one &&\n> + git config --add remote.multi.url wrong-two &&\n> + git -c remote.multi.url= \\\n> + -c remote.multi.url=right-one \\\n> + -c remote.multi.url=right-two \\\n> + remote show -n multi >actual.raw &&\n> + grep URL actual.raw >actual &&\n> + cat >expect <<-\\EOF &&\n> + Fetch URL: right-one\n> + Push URL: right-one\n> + Push URL: right-two\n> + EOF\n> + test_cmp expect actual\n> +'\n> +\n> +test_expect_success 'empty config clears remote.*.pushurl list' '\n> + test_when_finished \"git config --remove-section remote.multi\" &&\n> + git config --add remote.multi.url right &&\n> + git config --add remote.multi.url will-be-ignored &&\n> + git config --add remote.multi.pushurl wrong-push-one &&\n> + git config --add remote.multi.pushurl wrong-push-two &&\n> + git -c remote.multi.pushurl= \\\n> + -c remote.multi.pushurl=right-push-one \\\n> + -c remote.multi.pushurl=right-push-two \\\n> + remote show -n multi >actual.raw &&\n> + grep URL actual.raw >actual &&\n> + cat >expect <<-\\EOF &&\n> + Fetch URL: right\n> + Push URL: right-push-one\n> + Push URL: right-push-two\n> + EOF\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n> --\n> 2.45.2.937.g0bcb3c087a\n\nMake sense.\n"},{"id":"497635","messageId":"CABPp-BEMQkrwvT7M=QnEzWr-n0Z49AEJU8B-UhUgNDw0HCh5Mw@mail.gmail.com","threadId":"61609","inReplyTo":"20240614103131.GH222445@coredump.intra.peff.net","subject":"Re: [PATCH 08/11] t5801: make remote-testgit GIT_DIR setup more robust","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-06-25T17:36:42Z","receivedAt":"2024-06-25T17:36:53Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jun 14, 2024 at 3:32 AM Jeff King wrote:\n>\n> Our tests use a fake helper that just imports from an existing Git\n> repository. We're fed the path to that repo on the command line, and\n> derive the GIT_DIR by tacking on \"/.git\".\n>\n> This is wrong if the path is a bare repository, but that's OK since this\n> is just a limited test. But it's also wrong if the transport code feeds\n> us the actual .git directory itself (i.e., we expect \"/path/to/repo\" but\n> it gives us \"/path/to/repo/.git\"). None of the current tests do that,\n> but let's future-proof ourselves against adding a test that does.\n>\n> We can instead ask \"rev-parse\" to set our GIT_DIR. Note that we have to\n> first unset other git variables from our environment. Coming into this\n> script, we'll have GIT_DIR set to the fetching repository, and we need\n> to \"switch\" to the remote one.\n>\n> Signed-off-by: Jeff King \n> ---\n> t/t5801/git-remote-testgit | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit\n> index c5b10f5775..f8b476499f 100755\n> --- a/t/t5801/git-remote-testgit\n> +++ b/t/t5801/git-remote-testgit\n> @@ -26,7 +26,8 @@ then\n> t_refspec=\"\"\n> fi\n>\n> -GIT_DIR=\"$url/.git\"\n> +unset $(git rev-parse --local-env-vars)\n\nOoh, I somehow didn't know about that flag before. TIL.\n\n> +GIT_DIR=$(git -C \"$url\" rev-parse --absolute-git-dir)\n> export GIT_DIR\n>\n> force=\n> --\n> 2.45.2.937.g0bcb3c087a\n\nMakes sense.\n"},{"id":"497636","messageId":"CABPp-BHdVp+4s7QN0xiXFzoimtDwMPbb4Gw=Kb5MsBoY+SNLvA@mail.gmail.com","threadId":"61609","inReplyTo":"20240614104203.GK222445@coredump.intra.peff.net","subject":"Re: [PATCH 11/11] remote: drop checks for zero-url case","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-06-25T17:37:14Z","receivedAt":"2024-06-25T17:37:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jun 14, 2024 at 4:12 AM Jeff King wrote:\n>\n> Now that the previous commit removed the possibility that a \"struct\n> remote\" will ever have zero url fields, we can drop a number of\n> redundant checks and untriggerable code paths.\n\nOoh, nice.\n\n> Signed-off-by: Jeff King \n> ---\n> Note for reviewers: the hunk in builtin/push.c is funny. The original\n> code was:\n>\n> if (url->nr) {\n> for (i = 0; i < url->nr; i++) {\n> do this\n> }\n> } else {\n> do that\n> }\n>\n> And I removed the \"do that\" part along with the if/else to become:\n>\n> for (i = 0; i < url->nr; i++) {\n> do this\n> }\n>\n>\n> But because \"this\" and \"that\" were so similar, and because the\n> indentation of \"this\" in the loop was now the same of the old \"that\",\n> the diff makes it look like I dropped the first half of the conditional.\n\nThanks for adding this comment; was very helpful while reviewing.\n\n> builtin/archive.c | 2 --\n> builtin/ls-remote.c | 2 --\n> builtin/push.c | 13 ++-----------\n> builtin/remote.c | 13 +++----------\n> t/helper/test-bundle-uri.c | 2 --\n> transport.c | 17 +++++++----------\n> 6 files changed, 12 insertions(+), 37 deletions(-)\n>\n> diff --git a/builtin/archive.c b/builtin/archive.c\n> index 0d9aff7e6f..7234607aaa 100644\n> --- a/builtin/archive.c\n> +++ b/builtin/archive.c\n> @@ -31,8 +31,6 @@ static int run_remote_archiver(int argc, const char **argv,\n> struct packet_reader reader;\n>\n> _remote = remote_get(remote);\n> - if (!_remote->url.nr)\n> - die(_(\"git archive: Remote with no URL\"));\n> transport = transport_get(_remote, _remote->url.v[0]);\n> transport_connect(transport, \"git-upload-archive\", exec, fd);\n>\n> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\n> index 4c04dbbf19..8f3a64d838 100644\n> --- a/builtin/ls-remote.c\n> +++ b/builtin/ls-remote.c\n> @@ -109,8 +109,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n> die(\"bad repository '%s'\", dest);\n> die(\"No remote configured to list refs from.\");\n> }\n> - if (!remote->url.nr)\n> - die(\"remote %s has no configured URL\", dest);\n>\n> if (get_url) {\n> printf(\"%s\\n\", remote->url.v[0]);\n> diff --git a/builtin/push.c b/builtin/push.c\n> index 00d99af1a8..8260c6e46a 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -438,18 +438,9 @@ static int do_push(int flags,\n> }\n> errs = 0;\n> url = push_url_of_remote(remote);\n> - if (url->nr) {\n> - for (i = 0; i < url->nr; i++) {\n> - struct transport *transport =\n> - transport_get(remote, url->v[i]);\n> - if (flags & TRANSPORT_PUSH_OPTIONS)\n> - transport->push_options = push_options;\n> - if (push_with_options(transport, push_refspec, flags))\n> - errs++;\n> - }\n> - } else {\n> + for (i = 0; i < url->nr; i++) {\n> struct transport *transport =\n> - transport_get(remote, NULL);\n> + transport_get(remote, url->v[i]);\n> if (flags & TRANSPORT_PUSH_OPTIONS)\n> transport->push_options = push_options;\n> if (push_with_options(transport, push_refspec, flags))\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index 06c09ed060..c5c94d4dbd 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -1002,8 +1002,7 @@ static int get_remote_ref_states(const char *name,\n> struct transport *transport;\n> const struct ref *remote_refs;\n>\n> - transport = transport_get(states->remote, states->remote->url.nr > 0 ?\n> - states->remote->url.v[0] : NULL);\n> + transport = transport_get(states->remote, states->remote->url.v[0]);\n> remote_refs = transport_get_remote_refs(transport, NULL);\n>\n> states->queried = 1;\n> @@ -1294,8 +1293,7 @@ static int show(int argc, const char **argv, const char *prefix)\n> get_remote_ref_states(*argv, &info.states, query_flag);\n>\n> printf_ln(_(\"* remote %s\"), *argv);\n> - printf_ln(_(\" Fetch URL: %s\"), info.states.remote->url.nr > 0 ?\n> - info.states.remote->url.v[0] : _(\"(no URL)\"));\n> + printf_ln(_(\" Fetch URL: %s\"), info.states.remote->url.v[0]);\n> url = push_url_of_remote(info.states.remote);\n> for (i = 0; i < url->nr; i++)\n> /*\n> @@ -1440,10 +1438,7 @@ static int prune_remote(const char *remote, int dry_run)\n> }\n>\n> printf_ln(_(\"Pruning %s\"), remote);\n> - printf_ln(_(\"URL: %s\"),\n> - states.remote->url.nr\n> - ? states.remote->url.v[0]\n> - : _(\"(no URL)\"));\n> + printf_ln(_(\"URL: %s\"), states.remote->url.v[0]);\n>\n> for_each_string_list_item(item, &states.stale)\n> string_list_append(&refs_to_prune, item->util);\n> @@ -1632,8 +1627,6 @@ static int get_url(int argc, const char **argv, const char *prefix)\n> }\n>\n> url = push_mode ? push_url_of_remote(remote) : &remote->url;\n> - if (!url->nr)\n> - die(_(\"no URLs configured for remote '%s'\"), remotename);\n>\n> if (all_mode) {\n> for (i = 0; i < url->nr; i++)\n> diff --git a/t/helper/test-bundle-uri.c b/t/helper/test-bundle-uri.c\n> index 3285dd962e..0c5fa723d8 100644\n> --- a/t/helper/test-bundle-uri.c\n> +++ b/t/helper/test-bundle-uri.c\n> @@ -88,8 +88,6 @@ static int cmd_ls_remote(int argc, const char **argv)\n> die(_(\"bad repository '%s'\"), dest);\n> die(_(\"no remote configured to get bundle URIs from\"));\n> }\n> - if (!remote->url.nr)\n> - die(_(\"remote '%s' has no configured URL\"), dest);\n>\n> transport = transport_get(remote, NULL);\n> if (transport_get_remote_bundle_uri(transport) < 0) {\n> diff --git a/transport.c b/transport.c\n> index eba92eb7e0..a324045240 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -1112,6 +1112,7 @@ static struct transport_vtable builtin_smart_vtable = {\n> struct transport *transport_get(struct remote *remote, const char *url)\n> {\n> const char *helper;\n> + const char *p;\n> struct transport *ret = xcalloc(1, sizeof(*ret));\n>\n> ret->progress = isatty(2);\n> @@ -1127,19 +1128,15 @@ struct transport *transport_get(struct remote *remote, const char *url)\n> ret->remote = remote;\n> helper = remote->foreign_vcs;\n>\n> - if (!url && remote->url.nr)\n> + if (!url)\n> url = remote->url.v[0];\n> ret->url = url;\n>\n> - /* maybe it is a foreign URL? */\n> - if (url) {\n> - const char *p = url;\n> -\n> - while (is_urlschemechar(p == url, *p))\n> - p++;\n> - if (starts_with(p, \"::\"))\n> - helper = xstrndup(url, p - url);\n> - }\n> + p = url;\n> + while (is_urlschemechar(p == url, *p))\n> + p++;\n> + if (starts_with(p, \"::\"))\n> + helper = xstrndup(url, p - url);\n>\n> if (helper) {\n> transport_helper_init(ret, helper);\n> --\n> 2.45.2.937.g0bcb3c087a\n\nSo, after reading the first 8 patches, I only kinda skimmed 9 and 10,\nthinking \"I don't really care about these remote helper things and\ndon't have an opinion on them -- besides, I'm sure Peff is picking\nsomething reasonable\", but I'm always a fan of code simplifications\nlike this patch. It makes it tempting to go back and read those last\ntwo patches a little closer. Almost tempting enough, in fact. ;-)\n"},{"id":"497654","messageId":"CABPp-BEaFUH2QaAW+9B904cCN_h8oyxTaSDMkK=v_yZua=A1kA@mail.gmail.com","threadId":"61609","inReplyTo":"20240614102439.GA222287@coredump.intra.peff.net","subject":"Re: [PATCH 0/11] allow overriding remote.*.url","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-06-25T17:44:03Z","receivedAt":"2024-06-25T17:44:15Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jun 14, 2024 at 3:25 AM Jeff King wrote:\n>\n> On Thu, Jun 13, 2024 at 06:24:09AM -0400, Jeff King wrote:\n>\n> > > I was expecting (with excitement) a mess, but the above is as clean\n> > > as we can make the idea, I would say. Lack of documentation and\n> > > tests do count as incompleteness though of course.\n> >\n> > Yeah, and we should probably do the same for pushurl. And I think there\n> > could be some cleanup of the memory ownership handling of add_url().\n>\n> So as always with this crufty 2009-era code, there turned out to be some\n> subtleties. ;)\n>\n> The good news is that I think dealing with them left the code in a\n> better place. It's easier to reason about, and a few possible leaks have\n> been plugged (I don't know if they were triggered in the test suite or\n> not; if so they weren't enough to tip any scripts over to being\n> leak-free).\n\nI agree with this good news after reviewing the series.\n\n> We can split the series into segments:\n>\n> [01/11]: archive: fix check for missing url\n>\n> A nearby trivial bugfix.\n>\n> [02/11]: remote: refactor alias_url() memory ownership\n> [03/11]: remote: transfer ownership of memory in add_url(), etc\n> [04/11]: remote: use strvecs to store remote url/pushurl\n> [05/11]: remote: simplify url/pushurl selection\n>\n> Fixing memory handling weirdness, which is a necessary prereq for\n> the \"reset\" operation to avoid leaking. The switch to using a strvec\n> isn't strictly necessary, but it does make the code (including the\n> later patch 7) simpler.\n>\n> [06/11]: config: document remote.*.url/pushurl interaction\n> [07/11]: remote: allow resetting url list\n>\n> The actual change is in patch 7 here, but it was hard to add new\n> docs to the rather anemic existing ones. Hence patch 6.\n>\n> [08/11]: t5801: make remote-testgit GIT_DIR setup more robust\n> [09/11]: t5801: test remote.*.vcs config\n> [10/11]: remote: always require at least one url in a remote\n> [11/11]: remote: drop checks for zero-url case\n>\n> This is a related cleanup I found while working in the area.\n> Arguably it could be a separate topic, though it does depend\n> textually on what came before.\n\nI only managed to find a few typos in commit messages, but I looked\nthrough patches 1-8 pretty closely. I only skimmed 9 & 10 -- I don't\nreally have an opinion on the remote helpers. I agree that the issue\nyou bring up in the patches makes sense to discuss, and the route you\npicked looks reasonable to me, but I don't feel motivated to try to\nuse or understand the remote helpers enough to form an opinion.\nHowever, I'm a fan of the cleanup in patch 11 that your changes in 9 &\n10 enabled, so if everyone's as ambivalent as me (and 15 years of\nthings being broken suggests everyone is likely to be as ambivalent as\nme) then I'd say just go with your changes in 9 & 10 and call it a\nday.\n"},{"id":"497728","messageId":"20240626204001.GC441931@coredump.intra.peff.net","threadId":"61609","inReplyTo":"CABPp-BEaFUH2QaAW+9B904cCN_h8oyxTaSDMkK=v_yZua=A1kA@mail.gmail.com","subject":"Re: [PATCH 0/11] allow overriding remote.*.url","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-26T20:40:01Z","receivedAt":"2024-06-26T20:40:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 25, 2024 at 10:44:03AM -0700, Elijah Newren wrote:\n\n> I only managed to find a few typos in commit messages, but I looked\n> through patches 1-8 pretty closely. I only skimmed 9 & 10 -- I don't\n> really have an opinion on the remote helpers. I agree that the issue\n> you bring up in the patches makes sense to discuss, and the route you\n> picked looks reasonable to me, but I don't feel motivated to try to\n> use or understand the remote helpers enough to form an opinion.\n> However, I'm a fan of the cleanup in patch 11 that your changes in 9 &\n> 10 enabled, so if everyone's as ambivalent as me (and 15 years of\n> things being broken suggests everyone is likely to be as ambivalent as\n> me) then I'd say just go with your changes in 9 & 10 and call it a\n> day.\n\nThanks for taking a look. I do think patches 9 and 10 are the most\ncontroversial in their goals. But for the reasons given there, I don't\nthink anybody will care about the direction much either way. And\ncertainly they are not making anything _worse_, since the thing they\ndisallow is already broken. They are merely shutting off the option of\n\"fixing\" it to match the original intent.\n\nSo I stand by the direction I took in the patches, but I wanted to point\nout that if anybody wants to be extra careful, those are the ones to\nlook at.\n\n-Peff\n"}]}