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

[Elijah Newren <newren@gmail.com>]

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

Yeah, deferring makes sense to me.

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

Thanks for documenting this too.

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

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

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

Make sense.