| Message ID | 20230209122857.M669733@dcvr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] fetch: support hideRefs to speed up connectivity checks | expand |
Eric Wong <e@80x24.org> writes: > git -c transfer.hideRefs=refs \ > -c transfer.hideRefs='!refs/remotes/$REMOTE/' \ > fetch $REMOTE > > I initially considered passing --negotiation-tip OIDs, but this seems > like an easier solution as I'm not yet familiar with this code > and prefer to avoid writing too much C. --negotiation-tip supports ref name globs too. Would that be sufficient for your purposes?
Jonathan Tan <jonathantanmy@google.com> wrote: > Eric Wong <e@80x24.org> writes: > > git -c transfer.hideRefs=refs \ > > -c transfer.hideRefs='!refs/remotes/$REMOTE/' \ > > fetch $REMOTE > > > > I initially considered passing --negotiation-tip OIDs, but this seems > > like an easier solution as I'm not yet familiar with this code > > and prefer to avoid writing too much C. > > --negotiation-tip supports ref name globs too. Would that be sufficient > for your purposes? Yes, I tried using globs but didn't want to figure out how to pass the resulting OIDs to rev-list.
Eric Wong <e@80x24.org> writes: > Not sure if this is the right way to go about this... > If it's close, maybe --exclude-hidden=fetch can be supported. Yeah, why not. I however notice error handling in the codepath that deals with "--exclude-hidden" is a bit sloppy. refs.c::parse_hide_refs_config() is nice enough to diagnose a malformed transfer.hiderefs configuration as an error by returning -1, and revision.c::hide_refs_config() propagates such an error up, but revision.c::exclude_hidden_refs() ignores the error from git_config(), and revision.c::handle_revision_pseudo_opt() ignores any error from exclude_hidden_refs() anyway. We may want to tighten it a bit before (ab)using the option in more contexts. Thanks.
Junio C Hamano <gitster@pobox.com> wrote: > I however notice error handling in the codepath that deals with > "--exclude-hidden" is a bit sloppy. > > refs.c::parse_hide_refs_config() is nice enough to diagnose a > malformed transfer.hiderefs configuration as an error by returning > -1, and revision.c::hide_refs_config() propagates such an error up, > but revision.c::exclude_hidden_refs() ignores the error from > git_config(), and revision.c::handle_revision_pseudo_opt() ignores > any error from exclude_hidden_refs() anyway. Not sure I follow. exclude_hidden_refs() either dies or calls git_config(). git_config() calls repo_config(), then configset_iter(). configset_iter() will git_die_config_linenr() if `fn' (hide_refs_config() in this case) returns < 0. > We may want to tighten it a bit before (ab)using the option in more > contexts. > > Thanks.
Eric Wong <e@80x24.org> writes: > Junio C Hamano <gitster@pobox.com> wrote: >> I however notice error handling in the codepath that deals with >> "--exclude-hidden" is a bit sloppy. >> >> refs.c::parse_hide_refs_config() is nice enough to diagnose a >> malformed transfer.hiderefs configuration as an error by returning >> -1, and revision.c::hide_refs_config() propagates such an error up, >> but revision.c::exclude_hidden_refs() ignores the error from >> git_config(), and revision.c::handle_revision_pseudo_opt() ignores >> any error from exclude_hidden_refs() anyway. > > Not sure I follow. exclude_hidden_refs() either dies or calls > git_config(). git_config() calls repo_config(), then > configset_iter(). configset_iter() will git_die_config_linenr() > if `fn' (hide_refs_config() in this case) returns < 0. Somehow I had this wishful thinking that the return value from git_config() can be checked and the caller can handle the error more gracefully, but its return type is void. We'll die when we see a bad configuration but only when we see "--exclude-hidden", which is when we need a valid value from there. That is how it should work, so I am now happier. Thanks.
diff --git a/builtin/fetch.c b/builtin/fetch.c index 12978622d5..473d99fd26 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1131,6 +1131,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, if (!connectivity_checked) { struct check_connected_options opt = CHECK_CONNECTED_INIT; + opt.exclude_hidden_refs_section = "receive"; rm = ref_map; if (check_connected(iterate_ref_map, &rm, &opt)) { rc = error(_("%s did not send all necessary objects\n"), url); @@ -1324,6 +1325,7 @@ static int check_exist_and_connected(struct ref *ref_map) } opt.quiet = 1; + opt.exclude_hidden_refs_section = "receive"; return check_connected(iterate_ref_map, &rm, &opt); }
Not sure if this is the right way to go about this... If it's close, maybe --exclude-hidden=fetch can be supported. I'm using `receive' for now to minimize the change. With roughly 800 remotes all fetching to their own refs/remotes/$REMOTE/* island, the connectivity check[1] gets expensive for each fetch. To do a no-op fetch on one $REMOTE out of hundreds, hideRefs now allows the no-op fetch to take ~30 seconds instead of ~20 minutes on a noisy, RAM-constrained machine (localhost, so no network latency): git -c transfer.hideRefs=refs \ -c transfer.hideRefs='!refs/remotes/$REMOTE/' \ fetch $REMOTE I initially considered passing --negotiation-tip OIDs, but this seems like an easier solution as I'm not yet familiar with this code and prefer to avoid writing too much C. [1] `git rev-list --objects --stdin --not --all --quiet --alternate-refs' gets painful w/o enough RAM to cache the repo, even on a SATA-2 SSD. --- builtin/fetch.c | 2 ++ 1 file changed, 2 insertions(+)