From: Jeff King <peff@peff•net>
To: Jacob Keller <jacob.e.keller@intel•com>
Cc: git@vger•kernel.org
Subject: [PATCH 1/4] check_connected(): delay opening new_pack
Date: Thu, 5 Mar 2026 18:08:54 -0500 [thread overview]
Message-ID: <20260305230854.GA2901305@coredump.intra.peff.net> (raw)
In-Reply-To: <20260305230315.GA2354983@coredump.intra.peff.net>
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 <peff@peff•net>
---
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.
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
next prev parent reply other threads:[~2026-03-05 23:08 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-05 20:51 memory leak when cloning a repository Jacob Keller
2026-03-05 22:02 ` Jeff King
2026-03-05 23:03 ` [PATCH 0/4] plugging some mmap() leaks Jeff King
2026-03-05 23:08 ` Jeff King [this message]
2026-03-05 23:18 ` [PATCH 1/4] check_connected(): delay opening new_pack Jacob Keller
2026-03-05 23:09 ` [PATCH 2/4] check_connected(): fix leak of pack-index mmap Jeff King
2026-03-05 23:20 ` Jacob Keller
2026-03-05 23:12 ` [PATCH 3/4] pack-revindex: avoid double-loading .rev files Jeff King
2026-03-05 23:13 ` [PATCH 4/4] Makefile: turn on NO_MMAP when building with LSan Jeff King
2026-03-06 9:17 ` Jacob Keller
2026-03-06 16:25 ` [PATCH 5/4] meson: " Jeff King
2026-03-06 18:00 ` Ramsay Jones
2026-03-07 1:14 ` [PATCH 4/4] Makefile: " Junio C Hamano
2026-03-07 2:24 ` [PATCH 3.5/4] object-file: fix mmap() leak in odb_source_loose_read_object_stream() Jeff King
2026-03-07 5:35 ` Junio C Hamano
2026-03-10 12:23 ` Patrick Steinhardt
2026-03-06 4:37 ` [PATCH 0/4] plugging some mmap() leaks Ramsay Jones
2026-03-06 16:21 ` Jeff King
2026-03-06 17:49 ` Ramsay Jones
2026-03-06 18:37 ` Junio C Hamano
2026-03-06 18:55 ` Ramsay Jones
2026-03-06 22:05 ` Junio C Hamano
2026-03-06 23:25 ` Ramsay Jones
2026-03-07 1:15 ` Junio C Hamano
2026-03-05 23:16 ` memory leak when cloning a repository Jacob Keller
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=20260305230854.GA2901305@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=jacob.e.keller@intel$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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