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

[Elijah Newren <newren@gmail.com>]

On Fri, Jun 14, 2024 at 4:12 AM Jeff King <peff@peff.net> wrote:
>
> Now that the previous commit removed the possibility that a "struct
> remote" will ever have zero url fields, we can drop a number of
> redundant checks and untriggerable code paths.

Ooh, nice.

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

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

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

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