git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Elijah Newren <newren@gmail.com>
Date
Jun 25, 2024, 17:37 UTC
Message-ID
<CABPp-BHdVp+4s7QN0xiXFzoimtDwMPbb4Gw=Kb5MsBoY+SNLvA@mail.gmail.com>
In-Reply-To
<20240614104203.GK222445@coredump.intra.peff.net>
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.
Show 23 quoted lines
> 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.
Show 157 quoted lines
>  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. ;-)

Previous: Jeff KingNext: Elijah Newren
Message 27 of 29 in “Cannot override `remote.origin.url` with `-c` option”
  1. Mathew GeorgeJun 9, 2024
  2. Jeff KingJun 11, 2024
  3. Junio C HamanoJun 11, 2024
  4. Jeff KingJun 13, 2024
  5. 0/11 allow overriding remote.*.urlJeff King, Jun 14, 2024
  6. 01/11 archive: fix check for missing urlJeff King, Jun 14, 2024
  7. 02/11 remote: refactor alias_url() memory ownershipJeff King, Jun 14, 2024
  8. Junio C HamanoJun 14, 2024
  9. 03/11 remote: transfer ownership of memory in add_url(), etcJeff King, Jun 14, 2024
  10. Junio C HamanoJun 14, 2024
  11. Jeff KingJun 16, 2024
  12. Junio C HamanoJun 17, 2024
  13. Elijah NewrenJun 25, 2024
  14. 04/11 remote: use strvecs to store remote url/pushurlJeff King, Jun 14, 2024
  15. Elijah NewrenJun 25, 2024
  16. 05/11 remote: simplify url/pushurl selectionJeff King, Jun 14, 2024
  17. Elijah NewrenJun 25, 2024
  18. 06/11 config: document remote.*.url/pushurl interactionJeff King, Jun 14, 2024
  19. Elijah NewrenJun 25, 2024
  20. 07/11 remote: allow resetting url listJeff King, Jun 14, 2024
  21. Elijah NewrenJun 25, 2024
  22. 08/11 t5801: make remote-testgit GIT_DIR setup more robustJeff King, Jun 14, 2024
  23. Elijah NewrenJun 25, 2024
  24. 09/11 t5801: test remote.*.vcs configJeff King, Jun 14, 2024
  25. 10/11 remote: always require at least one url in a remoteJeff King, Jun 14, 2024
  26. 11/11 remote: drop checks for zero-url caseJeff King, Jun 14, 2024
  27. Elijah NewrenJun 25, 2024
  28. Elijah NewrenJun 25, 2024
  29. Jeff KingJun 26, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.