git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
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
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.

  reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-20 22:00 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

  List information: http://vger.kernel.org/majordomo-info.html

* 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

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror http://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git