From: Jacob Keller Date: Thu, 05 Mar 2026 23:18:12 GMT Subject: Re: [PATCH 1/4] check_connected(): delay opening new_pack Message-ID: In-Reply-To: <20260305230854.GA2901305@coredump.intra.peff.net> On Thu, Mar 5, 2026 at 3:08 PM Jeff King wrote: > > In check_connected(), if the transport tells us we got a single packfile > that has already been verified as self-contained and connected, then we > can skip checking connectivity for any tips that are mentioned in that > pack. This goes back to c6807a40dc (clone: open a shortcut for > connectivity check, 2013-05-26). > > We don't need to open that pack until we are about to start sending oids > to our child rev-list process, since that's when we check whether they > are in the self-contained pack. Let's push the opening of that pack > further down in the function. That saves us from having to clean it up > when we leave the function early (and by the time have opened the > rev-list process, we never leave the function early, since we have to > clean up the child process). > > Signed-off-by: Jeff King > --- > One thing I noticed here is that for a clone with a single > self-contained pack, we could probably skip running rev-list entirely. I > don't know if it matters much, though, as a noop rev-list process is not > that expensive compared to the cost of a clone. And in the worst case, > it would involve calling find_pack_entry() on each proposed ref tip an > extra time only to find that at least one does need to be sent. Though > that is also not very expensive. > > I left it out of this series, though it would involve moving the > new_pack opening up above the start_command() invocation again. > > I also wondered if this whole thing out to be written to avoid a one-off > packed_git in the first place, like: > > - call reprepare_packed_git() to re-scan objects/pack > > - find the pack by name in the packed_git list > > - don't clean it up; it's owned by the repository struct now > > But that's a somewhat bigger change, and I'm not sure it really buys us > that much. I agree, this seems like the best low hanging fruit improvement to avoid the unnecessary cleanup. Reviewed-by: Jacob Keller > > connected.c | 33 +++++++++++++++------------------ > 1 file changed, 15 insertions(+), 18 deletions(-) > > diff --git a/connected.c b/connected.c > index 79403108dd..530357de54 100644 > --- a/connected.c > +++ b/connected.c > @@ -45,20 +45,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data, > return err; > } > > - if (transport && transport->smart_options && > - transport->smart_options->self_contained_and_connected && > - transport->pack_lockfiles.nr == 1 && > - strip_suffix(transport->pack_lockfiles.items[0].string, > - ".keep", &base_len)) { > - struct strbuf idx_file = STRBUF_INIT; > - strbuf_add(&idx_file, transport->pack_lockfiles.items[0].string, > - base_len); > - strbuf_addstr(&idx_file, ".idx"); > - new_pack = add_packed_git(the_repository, idx_file.buf, > - idx_file.len, 1); > - strbuf_release(&idx_file); > - } > - > if (repo_has_promisor_remote(the_repository)) { > /* > * For partial clones, we don't want to have to do a regular > @@ -90,7 +76,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data, > promisor_pack_found: > ; > } while ((oid = fn(cb_data)) != NULL); > - free(new_pack); > return 0; > } > > @@ -127,15 +112,27 @@ int check_connected(oid_iterate_fn fn, void *cb_data, > else > rev_list.no_stderr = opt->quiet; > > - if (start_command(&rev_list)) { > - free(new_pack); > + if (start_command(&rev_list)) > return error(_("Could not run 'git rev-list'")); > - } > > sigchain_push(SIGPIPE, SIG_IGN); > > rev_list_in = xfdopen(rev_list.in, "w"); > > + if (transport && transport->smart_options && > + transport->smart_options->self_contained_and_connected && > + transport->pack_lockfiles.nr == 1 && > + strip_suffix(transport->pack_lockfiles.items[0].string, > + ".keep", &base_len)) { > + struct strbuf idx_file = STRBUF_INIT; > + strbuf_add(&idx_file, transport->pack_lockfiles.items[0].string, > + base_len); > + strbuf_addstr(&idx_file, ".idx"); > + new_pack = add_packed_git(the_repository, idx_file.buf, > + idx_file.len, 1); > + strbuf_release(&idx_file); > + } > + > do { > /* > * If index-pack already checked that: > -- > 2.53.0.786.g466665faa3 > >