| Message ID | 20240614104203.GK222445@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | aecd794fca275b42e271b80236e95f0d288bd709 |
| Headers | show |
| Series | allow overriding remote.*.url | expand |
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. ;-)
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);
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. 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. 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(-)