From: Josh Steadmon <steadmon@google.com>
To: Emily Shaffer <emilyshaffer@google.com>
Cc: Jonathan Tan <jonathantanmy@google.com>, git@vger.kernel.org
Subject: Re: [PATCH] connected: always use partial clone optimization
Date: Thu, 26 Mar 2020 16:14:16 -0700 [thread overview]
Message-ID: <20200326231416.GC12694@google.com> (raw)
In-Reply-To: <20200326211156.GA37946@google.com>
On 2020.03.26 14:11, Emily Shaffer wrote:
> On Fri, Mar 20, 2020 at 03:00:45PM -0700, Jonathan Tan wrote:
> > With 50033772d5 ("connected: verify promisor-ness of partial clone",
> > 2020-01-30), the fast path (checking promisor packs) in
> > check_connected() now passes a subset of the slow path (rev-list) - if
> > all objects to be checked are found in promisor packs, both the fast
> > path and the slow path will pass; otherwise, the fast path will
> > definitely not pass. This means that we can always attempt the fast path
> > whenever we need to do the slow path.
> >
> > The fast path is currently guarded by a flag; therefore, remove that
> > flag. Also, make the fast path fallback to the slow path - if the fast
> > path fails, the failing OID and all remaining OIDs will be passed to
> > rev-list.
>
> It looks like a pretty simple change. I had one probably-biased
> complaint about gotos below, otherwise it looks reasonable to me.
[snip]
> > diff --git a/connected.c b/connected.c
> > index 7e9bd1bc62..846f2e4eef 100644
> > --- a/connected.c
> > +++ b/connected.c
> > @@ -52,7 +52,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
> > strbuf_release(&idx_file);
> > }
> >
> > - if (opt->check_refs_are_promisor_objects_only) {
> > + if (has_promisor_remote()) {
> > /*
> > * For partial clones, we don't want to have to do a regular
> > * connectivity check because we have to enumerate and exclude
> > @@ -71,13 +71,18 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
> > if (find_pack_entry_one(oid.hash, p))
> > goto promisor_pack_found;
> > }
> > - return 1;
> > + /*
> > + * Fallback to rev-list with oid and the rest of the
> > + * object IDs provided by fn.
> > + */
> > + goto no_promisor_pack_found;
> > promisor_pack_found:
> > ;
> > } while (!fn(cb_data, &oid));
> > return 0;
> > }
> >
> > +no_promisor_pack_found:
>
> Having a look at the final structure of the loop with these gotos, I'm a
> little confused. Could be this isn't C-idiomatic but I think the code
> could be easier to read with helpers instead of gotos. I realize it's
> longer but I have a hard time understanding that your gotos are used to
> double-continue or double-break; nested loops tend to make me want to
> use helpers. But - I'm a lowly barely-reformed C++ developer, so what do
> I know ;)
>
> int oid_in_promisor(oid) {
> for (p = get_all_packs(the_repository); p; p = p->next) {
> if (!p->pack_promisor)
> continue;
> if (find_pack_entry_one(oid.hash, p)
> return 1;
> }
> }
>
> int all_oids_in_promisors(oid, fn, cb_data)
> {
> do {
> if (! oid_in_promisor(oid))
> return 0;
> } while (!fn(cb_data, &oid));
> return 1;
> }
>
> int check_connected(...)
> {
> ...
> if (has_promisor_remote()) {
> if (all_oids_in_promisors(oid, fn, cb_data))
> return 0;
> if (opt->shallow_file) {
> ...
> }
I like this version better as well.
next prev parent reply other threads:[~2020-03-26 23:14 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-20 22:00 [PATCH] connected: always use partial clone optimization Jonathan Tan
2020-03-20 22:54 ` Junio C Hamano
2020-03-26 19:01 ` Josh Steadmon
2020-03-26 21:11 ` Emily Shaffer
2020-03-26 23:14 ` Josh Steadmon [this message]
2020-03-29 17:39 ` Junio C Hamano
2020-03-30 3:32 ` Jonathan Tan
2020-03-30 5:12 ` Junio C Hamano
2020-03-30 16:04 ` Jonathan Tan
2020-03-30 18:09 ` Junio C Hamano
2020-03-30 13:37 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200326231416.GC12694@google.com \
--to=steadmon@google.com \
--cc=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
--cc=jonathantanmy@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).