On Fri, Jun 14, 2024 at 4:12 AM Jeff King <peff@xxxxxxxx> 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@xxxxxxxx>
> ---
> 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. ;-)
