Skip to content

repack: don't lose objects to a ".keep" that appears mid-run #2219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
qeesung wants to merge 5 commits into
base: master
Choose a base branch
from
Open

repack: don't lose objects to a ".keep" that appears mid-run #2219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8cf7231
pack-objects: keep --keep-pack open when following
qeesung Sep 8, 2026
77aec89
pack-objects: reset kept-pack cache for cruft walk
qeesung Sep 8, 2026
b76e06a
pack-objects: sort --keep-pack list for lookup
qeesung Sep 8, 2026
20a051c
pack-objects: add --keep-pack-from-file
qeesung Sep 8, 2026
4684fd8
repack: tell pack-objects which packs are kept
qeesung Sep 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Documentation/git-pack-objects.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ SYNOPSIS
[--no-reuse-delta] [--delta-base-offset] [--non-empty]
[--local] [--incremental] [--window=<n>] [--depth=<n>]
[--revs [--unpacked | --all]] [--keep-pack=<pack-name>]
[--keep-pack-from-file=<file>]
[--cruft] [--cruft-expiration=<time>]
[--stdout [--filter=<filter-spec>] | <base-name>]
[--shallow] [--keep-true-parents] [--[no-]sparse]
Expand Down Expand Up @@ -193,6 +194,13 @@ depth is 4095.
leading directory (e.g. `pack-123.pack`). The option could be
specified multiple times to keep multiple packs.

--keep-pack-from-file=<file>::
Read names of packs to keep from `<file>`, one per line, and
treat each of them as if it had been given with `--keep-pack`.
Empty lines are ignored. This is meant for callers such as
linkgit:git-repack[1] that may have to name more packs than fit
on a command line.

--incremental::
This flag causes an object already in a pack to be ignored
even if it would have otherwise been packed.
Expand Down
71 changes: 58 additions & 13 deletions builtin/pack-objects.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ static const char *const pack_usage[] = {
" [--no-reuse-delta] [--delta-base-offset] [--non-empty]\n"
" [--local] [--incremental] [--window=<n>] [--depth=<n>]\n"
" [--revs [--unpacked | --all]] [--keep-pack=<pack-name>]\n"
" [--keep-pack-from-file=<file>]\n"
" [--cruft] [--cruft-expiration=<time>]\n"
" [--stdout [--filter=<filter-spec>] | <base-name>]\n"
" [--shallow] [--keep-true-parents] [--[no-]sparse]\n"
Expand Down Expand Up @@ -4274,6 +4275,7 @@ static void enumerate_cruft_objects(void)
static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs)

@gitgitgadget gitgitgadget Bot Sep 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Qin ShiCheng via GitGitGadget" <gitgitgadget@gmail.com> writes:

> @@ -4301,10 +4302,17 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs
>  	/*
>  	 * Re-mark only the fresh packs as kept so that objects in
>  	 * unknown packs do not halt the reachability traversal early.
> +	 * The kept-pack cache was built while those packs were still
> +	 * marked, so drop it too.
>  	 */
>  	repo_for_each_pack(the_repository, p)
>  		p->pack_keep_in_core = 0;
>  	mark_pack_kept_in_core(fresh_packs, 1);
> +	for (source = the_repository->objects->sources; source;
> +	     source = source->next) {
> +		struct odb_source_files *files = odb_source_files_downcast(source);
> +		packfile_store_invalidate_kept_pack_cache(files->packed);
> +	}

This question is primarily meant for folks who are pushing different
ODB backends, but I am not sure this is safe in the long term.

When downcasting finds that 'source' is not from the files backend,
we immediately hit BUG().  Is checking the type of 'source' first
and calling packfile_store_invalidate_kept_pack_cache() only when
it is from the files backend a sensible workaround?  That sounds
like a blatant layering violation.

One of the recent design decisions, unrelated to this, was to make
the concept of "alternate object store" an implementation detail of
the files backend, if I recall correctly.  Do we need a similar
rearchitecting of the code here, pushing details like packfile
management down to the files backend layer, before we can properly
fix this?

Of course, until an ODB backend other than files materializes, all
of the above is merely academic and the proposed change might be
sufficient.  However, relying on an unchecked downcast feels like
laying mines for our future selves.

@gitgitgadget gitgitgadget Bot Sep 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Qin ShiCheng wrote on the Git mailing list (how to reply to this email):

Junio C Hamano <gitster@pobox.com> writes:

> When downcasting finds that 'source' is not from the files backend,
> we immediately hit BUG().  Is checking the type of 'source' first
> and calling packfile_store_invalidate_kept_pack_cache() only when
> it is from the files backend a sensible workaround?  That sounds
> like a blatant layering violation.

Agreed, and I would rather not have pack-objects look at the type of
a source at all.

The assumption is already made two lines above the new loop, though:
repo_for_each_pack() downcasts every source in the same way, and so
does has_object_kept_pack(), which is what reads this cache in the
first place. So the loop is not wrong so much as in the wrong place.
It belongs next to its reader in packfile.c, not in the builtin.

For v3 I have this instead:

	void repo_invalidate_kept_pack_caches(struct repository *r)
	{
		struct odb_source *source;

		for (source = r->objects->sources; source; source = source->next) {
			struct odb_source_files *files = odb_source_files_downcast(source);
			invalidate_kept_pack_cache(files->packed);
		}
	}

with the per-store function made static again, and the caller in
pack-objects reduced to

	mark_pack_kept_in_core(fresh_packs, 1);
	repo_invalidate_kept_pack_caches(the_repository);

This does not make the code work with another backend -- nothing
around it would either -- but pack-objects no longer gains a new
dependency on the files backend, and the downcast sits with the
others that will have to move together.

> Do we need a similar
> rearchitecting of the code here, pushing details like packfile
> management down to the files backend layer, before we can properly
> fix this?

I hope not. Without this patch, a cruft repack with an expiration
drops objects that are only reachable through a pack pack-objects was
not told about; the new test in t5329 shows it happening today. When
packfile management does move down to the files backend, this
function should go along with has_object_kept_pack(), and nothing in
the fix depends on where they end up. Patrick may well know better
how that is meant to look.

Thanks,
Qin

@gitgitgadget gitgitgadget Bot Sep 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

Qin ShiCheng <qeesung@live.com> writes:

> This does not make the code work with another backend -- nothing
> around it would either -- but pack-objects no longer gains a new
> dependency on the files backend, and the downcast sits with the
> others that will have to move together.

OK.

>> Do we need a similar
>> rearchitecting of the code here, pushing details like packfile
>> management down to the files backend layer, before we can properly
>> fix this?
>
> I hope not. Without this patch, a cruft repack with an expiration
> drops objects ...

Ah, I think you misunderstood.

By fix "this" I meant fixing "the layering violation" and not what
your topic originally wanted to achieve.  And as we agreed above,
these downcasts that sit together with existing ones need to move in
order to avoid layering violation, which is what I meant by
"rearchitecting".  Until that happens, layering violation is left
unfixed, but addressing the kept pack cache issue with layering
violation can be better than not addressing the issue at all.

In any case, my original question to experts

>> This question is primarily meant for folks who are pushing different
>> ODB backends, but I am not sure this is safe in the long term.

still stands.  I think we between two of us agreed the answer is "no
it is not safe in the long term", but others may have ideas to solve
it more cleanly, hopefully.

Thanks.

{
struct packed_git *p;
struct odb_source *source;
struct rev_info revs;
int ret;

Expand Down Expand Up @@ -4301,10 +4303,17 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs
/*
* Re-mark only the fresh packs as kept so that objects in
* unknown packs do not halt the reachability traversal early.
* The kept-pack cache was built while those packs were still
* marked, so drop it too.
*/
repo_for_each_pack(the_repository, p)
p->pack_keep_in_core = 0;
mark_pack_kept_in_core(fresh_packs, 1);
for (source = the_repository->objects->sources; source;
source = source->next) {
struct odb_source_files *files = odb_source_files_downcast(source);
packfile_store_invalidate_kept_pack_cache(files->packed);
}

if (prepare_revision_walk(&revs))
die(_("revision walk setup failed"));
Expand Down Expand Up @@ -4999,27 +5008,54 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)
oid_array_clear(&recent_objects);
}

static void add_extra_kept_packs(const struct string_list *names)
/*
* Read pack names from the file, one per line, as if each of them had
* been given with "--keep-pack".
*/
static void read_keep_pack_list(struct string_list *names, const char *path)
{
struct strbuf buf = STRBUF_INIT;
FILE *fp = xfopen(path, "r");

while (strbuf_getline(&buf, fp) != EOF) {
if (!buf.len)
continue;
string_list_append(names, buf.buf);
}
if (ferror(fp))
die_errno(_("could not read '%s'"), path);
fclose(fp);
strbuf_release(&buf);
}

static void add_extra_kept_packs(struct string_list *names,
enum stdin_packs_mode stdin_packs)
{
struct packed_git *p;

if (!names->nr)
return;

repo_for_each_pack(the_repository, p) {
const char *name = basename(p->pack_name);
int i;
string_list_sort(names);

repo_for_each_pack(the_repository, p) {
if (!p->pack_local)
continue;

for (i = 0; i < names->nr; i++)
if (!fspathcmp(name, names->items[i].string))
break;

if (i < names->nr) {
p->pack_keep_in_core = 1;
ignore_packed_keep_in_core = 1;
if (string_list_has_string(names, basename(p->pack_name))) {
/*
* When following, treat the pack like a "!" pack, not
* a "^" one: nobody said it is closed under
* reachability, so the traversal must be able to go
* through it.
*/
if (stdin_packs == STDIN_PACKS_MODE_FOLLOW) {
p->pack_keep_in_core_open = 1;
ignore_packed_keep_in_core_open = 1;
} else {
p->pack_keep_in_core = 1;
ignore_packed_keep_in_core = 1;
}
continue;
}
}
Expand Down Expand Up @@ -5131,7 +5167,11 @@ int cmd_pack_objects(int argc,
int rev_list_unpacked = 0, rev_list_all = 0, rev_list_reflog = 0;
int rev_list_index = 0;
enum stdin_packs_mode stdin_packs = STDIN_PACKS_MODE_NONE;
struct string_list keep_pack_list = STRING_LIST_INIT_NODUP;
struct string_list keep_pack_list = {
.strdup_strings = 1,
.cmp = fspathcmp,
};
char *keep_pack_from_file = NULL;
struct list_objects_filter_options filter_options =
LIST_OBJECTS_FILTER_INIT;
struct repo_config_values *cfg = repo_config_values(the_repository);
Expand Down Expand Up @@ -5216,6 +5256,8 @@ int cmd_pack_objects(int argc,
N_("ignore packs that have companion .keep file")),
OPT_STRING_LIST(0, "keep-pack", &keep_pack_list, N_("name"),
N_("ignore this pack")),
OPT_FILENAME(0, "keep-pack-from-file", &keep_pack_from_file,
N_("ignore the packs named in <file>")),
OPT_INTEGER(0, "compression", &cfg->pack_compression_level,
N_("pack compression level")),
OPT_BOOL(0, "keep-true-parents", &grafts_keep_true_parents,
Expand Down Expand Up @@ -5443,7 +5485,9 @@ int cmd_pack_objects(int argc,
if (progress && all_progress_implied)
progress = 2;

add_extra_kept_packs(&keep_pack_list);
if (keep_pack_from_file)
read_keep_pack_list(&keep_pack_list, keep_pack_from_file);
add_extra_kept_packs(&keep_pack_list, stdin_packs);
if (ignore_packed_keep_on_disk) {
struct packed_git *p;

Expand Down Expand Up @@ -5537,6 +5581,7 @@ int cmd_pack_objects(int argc,
clear_packing_data(&to_pack);
list_objects_filter_release(&filter_options);
string_list_clear(&keep_pack_list, 0);
free(keep_pack_from_file);
strvec_clear(&rp);

return 0;
Expand Down
15 changes: 15 additions & 0 deletions builtin/repack.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ int cmd_repack(int argc,
struct oidset drop_oids = OIDSET_INIT;
struct pack_geometry geometry = { 0 };
struct tempfile *refs_snapshot = NULL;
struct tempfile *kept_packs_snapshot = NULL;
int i, ret;
int show_progress;

Expand Down Expand Up @@ -456,6 +457,19 @@ int cmd_repack(int argc,

existing.repo = repo;
existing_packs_collect(&existing, &keep_pack_list);
if (existing.kept_packs.nr) {
struct strbuf path = STRBUF_INIT;

strbuf_addf(&path, "%s/%s_XXXXXX",
repo_get_object_directory(repo), "kept-packs");

kept_packs_snapshot = xmks_tempfile(path.buf);
existing_packs_snapshot_kept(&existing, kept_packs_snapshot);
po_args.kept_packs_snapshot =
get_tempfile_path(kept_packs_snapshot);

strbuf_release(&path);
}

if (geometry.split_factor) {
if (pack_everything)
Expand Down Expand Up @@ -644,6 +658,7 @@ int cmd_repack(int argc,
cruft_po_args.quiet = po_args.quiet;
cruft_po_args.delta_base_offset = po_args.delta_base_offset;
cruft_po_args.pack_kept_objects = 0;
cruft_po_args.kept_packs_snapshot = po_args.kept_packs_snapshot;

ret = write_cruft_pack(&opts, cruft_expiration,
combine_cruft_below_size, &names,
Expand Down
3 changes: 2 additions & 1 deletion odb/source-packed.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ struct odb_source_packed {
* Should not be accessed directly, but via
* `packfile_store_get_kept_pack_cache()`. The list of packs gets
* invalidated when the stored flags and the flags passed to
* `packfile_store_get_kept_pack_cache()` mismatch.
* `packfile_store_get_kept_pack_cache()` mismatch, or explicitly via
* `packfile_store_invalidate_kept_pack_cache()`.
*/
struct {
struct packed_git **packs;
Expand Down
9 changes: 7 additions & 2 deletions packfile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1870,15 +1870,20 @@ int packfile_fill_entry(struct packed_git *p,
return 1;
}

void packfile_store_invalidate_kept_pack_cache(struct odb_source_packed *store)
{
FREE_AND_NULL(store->kept_cache.packs);
store->kept_cache.flags = 0;
}

static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store,
unsigned flags)
{
if (!store->kept_cache.packs)
return;
if (store->kept_cache.flags == flags)
return;
FREE_AND_NULL(store->kept_cache.packs);
store->kept_cache.flags = 0;
packfile_store_invalidate_kept_pack_cache(store);
}

struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed *store,
Expand Down
7 changes: 7 additions & 0 deletions packfile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,13 @@ enum kept_pack_type {
struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed *store,
unsigned flags);

/*
* Drop the cache of kept packs so that the next call to
* `packfile_store_get_kept_pack_cache()` rebuilds it, e.g. after changing
* which packs are kept in core.
*/
void packfile_store_invalidate_kept_pack_cache(struct odb_source_packed *store);

struct pack_window {
struct pack_window *next;
unsigned char *base;
Expand Down
3 changes: 0 additions & 3 deletions repack-filtered.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,6 @@ int write_filtered_pack(const struct write_pack_opts *opts,

strvec_push(&cmd.args, "--stdin-packs");

for_each_string_list_item(item, &existing->kept_packs)
strvec_pushf(&cmd.args, "--keep-pack=%s", item->string);

cmd.in = -1;

ret = start_command(&cmd);
Expand Down
34 changes: 32 additions & 2 deletions repack.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,9 @@ void prepare_pack_objects(struct child_process *cmd,
strvec_push(&cmd->args, "--quiet");
if (args->delta_base_offset)
strvec_push(&cmd->args, "--delta-base-offset");
if (!args->pack_kept_objects)
strvec_push(&cmd->args, "--honor-pack-keep");
if (!args->pack_kept_objects && args->kept_packs_snapshot)
strvec_pushf(&cmd->args, "--keep-pack-from-file=%s",
args->kept_packs_snapshot);
strvec_push(&cmd->args, out);
cmd->git_cmd = 1;
cmd->out = -1;
Expand Down Expand Up @@ -167,6 +168,35 @@ void existing_packs_collect(struct existing_packs *existing,
strbuf_release(&buf);
}

void existing_packs_snapshot_kept(const struct existing_packs *existing,
struct tempfile *f)
{
struct string_list_item *item;
FILE *out = fdopen_tempfile(f, "w");

if (!out)
die(_("could not open tempfile %s for writing"),
get_tempfile_path(f));

for_each_string_list_item(item, &existing->kept_packs) {
/*
* A newline would split the name in two, and pack-objects
* quietly keeps whichever packs the halves happen to name.
*/
if (strchr(item->string, '\n'))
die(_("cannot keep pack '%s': its name contains a newline"),
item->string);
fprintf(out, "%s.pack\n", item->string);
}

if (close_tempfile_gently(f)) {
int save_errno = errno;
delete_tempfile(&f);
errno = save_errno;
die_errno(_("could not close kept packs snapshot tempfile"));
}
}

int existing_packs_has_non_kept(const struct existing_packs *existing)
{
return existing->non_kept_packs.nr || existing->cruft_packs.nr;
Expand Down
17 changes: 15 additions & 2 deletions repack.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@ struct pack_objects_args {
int path_walk;
int delta_base_offset;
int pack_kept_objects;
/*
* File naming the packs to leave alone, one "<name>.pack" per line;
* NULL when there are none. pack-objects reads it rather than
* looking for ".keep" files itself, so that a ".keep" created or
* removed while we run cannot make the two of us disagree over
* which packs are being repacked.
*/
const char *kept_packs_snapshot;
struct list_objects_filter_options filter_options;
};

Expand All @@ -28,6 +36,7 @@ struct pack_objects_args {
}

struct child_process;
struct tempfile;

void prepare_pack_objects(struct child_process *cmd,
const struct pack_objects_args *args,
Expand Down Expand Up @@ -79,6 +88,12 @@ struct existing_packs {
*/
void existing_packs_collect(struct existing_packs *existing,
const struct string_list *extra_keep);
/*
* Writes the names of the kept packs, one "<name>.pack" per line, into
* the given tempfile, for pack-objects to read with --keep-pack-from-file.
*/
void existing_packs_snapshot_kept(const struct existing_packs *existing,
struct tempfile *f);
int existing_packs_has_non_kept(const struct existing_packs *existing);
int existing_pack_is_marked_for_deletion(struct string_list_item *item);
void existing_packs_retain_cruft(struct existing_packs *existing,
Expand Down Expand Up @@ -138,8 +153,6 @@ void pack_geometry_remove_redundant(struct pack_geometry *geometry,
bool wrote_incremental_midx);
void pack_geometry_release(struct pack_geometry *geometry);

struct tempfile;

enum repack_write_midx_mode {
REPACK_WRITE_MIDX_NONE,
REPACK_WRITE_MIDX_DEFAULT,
Expand Down
40 changes: 40 additions & 0 deletions t/t5329-pack-objects-cruft.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,46 @@ test_expect_success 'cruft trees rescue sub-trees, blobs' '
)
'

test_expect_success 'cruft traversal rescues through a pack it was not told about' '
git init repo &&
test_when_finished "rm -fr repo" &&
(
cd repo &&

test_commit packed &&
git repack -Ad &&
keep="$(basename "$(ls $packdir/pack-*.pack)")" &&

test_commit old &&
test_commit mid &&
test_commit new &&

# "old" has expired, "new" is recent, and "mid" sits in a
# pack that pack-objects is not told about. Rescuing "old"
# from "new" means walking through that pack.
git rev-list --objects --no-object-names packed..old >old &&
while read object
do
test-tool chmtime -1000 \
"$objdir/$(test_oid_to_path $object)" || exit 1
done <old &&
git rev-list --objects --no-object-names old..mid |
git pack-objects $packdir/pack >/dev/null &&
git prune-packed &&

cruft="$(echo $keep | git pack-objects --cruft \
--cruft-expiration=750.seconds.ago \
$packdir/pack)" &&
test-tool pack-mtimes "pack-$cruft.mtimes" >actual.raw &&

cut -d" " -f1 <actual.raw | sort >actual &&
git rev-list --objects --no-object-names packed..new >expect.raw &&
sort <expect.raw >expect &&

test_cmp expect actual
)
'

test_expect_success 'expired objects are pruned' '
git init repo &&
test_when_finished "rm -fr repo" &&
Expand Down
Loading
Loading