[PATCH v3 0/5] repack: don't lose objects to a ".keep" that appears mid-run

["qeesung via GitGitGadget" <gitgitgadget@gmail.com>]

A concurrent push or fetch can make "git repack -d" delete a pack whose
objects were never copied anywhere, and exit 0. We hit this in production: a
ref pointing at a commit that no longer exists, on git 2.43, and it
reproduces on master.

What happens:

 * repack scans for ".keep" files and decides which packs to delete, then
   spawns pack-objects with --honor-pack-keep, which scans again;
 * in between, an index-pack --keep finishes -- a push migrating its
   quarantine, or a fetch -- and installs a ".keep" next to a pack that
   repack has already decided to delete;
 * pack-objects sees that ".keep" and leaves the pack's objects out; repack
   deletes the pack by its earlier list, with force_delete.

The fix is to stop the two processes from scanning separately: hand
pack-objects the snapshot repack took at startup (5/5). Patches 1-4 are what
5/5 needs to be safe:

 * 1/5: under --stdin-packs=follow, a --keep-pack pack stops the traversal
   like a "^" pack; on-disk ".keep" packs never did.
 * 2/5: the cruft walk goes by a stale kept-pack cache, which
   --honor-pack-keep happened to mask. Pre-existing, reproducible today.
 * 3/5: look --keep-pack names up in a sorted list; it gets long.
 * 4/5: --keep-pack-from-file, since a repository can have more kept packs
   than fit on a command line (32K characters on Windows).

Every fix comes with a test that fails without it; the race itself is
reproduced in t7703 by having a ".keep" appear as pack-objects starts. The
full suite passes.

Interaction with topics in seen:

 * ps/odb-files-alternates turns the list of object sources into a single
   files source with a list of object directories, so
   repo_invalidate_kept_pack_caches() from 2/5 needs to walk those instead.
   The textual merge is clean, but the build breaks; this resolution follows
   has_object_kept_pack() on that topic:
   
   void repo_invalidate_kept_pack_caches(struct repository *r) { struct
   odb_source_files *files = odb_source_files_downcast(r->objects->source);
   for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next)
   invalidate_kept_pack_cache(dir->packed); }

 * tb/repack-cruft-less-midx-corner-cases appends tests to the end of t5331,
   as does 4/5; keep both. That topic also hands kept packs to the
   "--stdin-packs=follow" walk as '^' or '!', so with this series such a
   pack is named twice, once on stdin and once in the file from 5/5. The two
   compose: a '^' pack still stops the walk, and a '!' pack is open either
   way.

With that resolution, seen with this series merged passes the full suite.

Changes since v2:

 * 2/5: the loop that drops the kept-pack cache of every object source moved
   from pack-objects into packfile.c, as repo_invalidate_kept_pack_caches(),
   next to has_object_kept_pack() which reads that cache; the per-store
   helper is static again. This keeps pack-objects from gaining a direct use
   of the files backend, but the loop still assumes every source is one,
   like its neighbours, so the layering issue Junio raised is not solved
   here: https://lore.kernel.org/git/xmqqcxu3c15i.fsf@gitster.g/ (I first
   misread his question as asking whether the fix itself had to wait for
   that rework; it was about the layering.)
 * The rest is unchanged.

Changes since v1:

 * Dropped the receive-pack patch; Justin Tobler is fixing that side by
   having the ODB transaction create the ".keep" itself:
   https://lore.kernel.org/git/aql8Wt2q9RnQpjEC@jtobler--20250820-SHC54/

Qin ShiCheng (5):
  pack-objects: keep --keep-pack open when following
  pack-objects: reset kept-pack cache for cruft walk
  pack-objects: sort --keep-pack list for lookup
  pack-objects: add --keep-pack-from-file
  repack: tell pack-objects which packs are kept

 Documentation/git-pack-objects.adoc |  8 +++
 builtin/pack-objects.c              | 66 +++++++++++++++++-----
 builtin/repack.c                    | 15 +++++
 odb/source-packed.h                 |  3 +-
 packfile.c                          | 19 ++++++-
 packfile.h                          |  7 +++
 repack-filtered.c                   |  3 -
 repack.c                            | 34 ++++++++++-
 repack.h                            | 17 +++++-
 t/t5329-pack-objects-cruft.sh       | 40 +++++++++++++
 t/t5331-pack-objects-stdin.sh       | 87 +++++++++++++++++++++++++++++
 t/t7700-repack.sh                   | 43 ++++++++++++++
 t/t7703-repack-geometric.sh         | 72 ++++++++++++++++++++++++
 13 files changed, 391 insertions(+), 23 deletions(-)


base-commit: 3cb9185f65410273787f74333cc027d2ea5daada
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2219%2Fqeesung%2Frepack-kept-packs-snapshot-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2219/qeesung/repack-kept-packs-snapshot-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2219

Range-diff vs v2:

 1:  8cf72312c5 = 1:  8cf72312c5 pack-objects: keep --keep-pack open when following
 2:  77aec8941f ! 2:  58019e4983 pack-objects: reset kept-pack cache for cruft walk
     @@ Commit message
          dropped when asked about a different kind of kept pack. Collecting
          builds it while the unlisted pack is still marked, the walk asks the
          same kind of question, and so the unlisted pack stays in it: the walk
     -    stops there, and whatever lies beyond it in an expired pack is lost.
     +    stops there, and whatever lies beyond it in an expired pack is left
     +    out of the cruft pack, to go when that pack is deleted.
      
          This went unnoticed because of "--honor-pack-keep". repack passes it,
          and when there is a ".keep" file it makes the collecting side ask
     @@ Commit message
          ".keep" file away and the objects are lost today. A later commit stops
          repack from passing "--honor-pack-keep" at all, so fix this first.
      
     -    Expose the invalidation packfile.c already has and call it after
     -    re-marking. The test builds an unreachable chain whose middle commit
     -    sits in a pack pack-objects is not told about and whose oldest objects
     -    have expired; without the fix the cruft pack holds only the recent tip.
     +    Drop the cache after re-marking. The loop over the object sources
     +    that does so lives in packfile.c, as repo_invalidate_kept_pack_caches(),
     +    next to has_object_kept_pack() which reads the cache. Like it, the
     +    loop assumes every source is a files backend; keeping that assumption
     +    in packfile.c rather than adding it to pack-objects means the two can
     +    move together once packfile management is pushed down into that
     +    backend.
     +
     +    The test builds an unreachable chain whose middle commit sits in a
     +    pack pack-objects is not told about and whose oldest objects have
     +    expired; without the fix the cruft pack holds only the recent tip.
      
          Signed-off-by: Qin ShiCheng <qeesung@live.com>
      
       ## builtin/pack-objects.c ##
     -@@ builtin/pack-objects.c: static void enumerate_cruft_objects(void)
     - static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs)
     - {
     - 	struct packed_git *p;
     -+	struct odb_source *source;
     - 	struct rev_info revs;
     - 	int ret;
     - 
      @@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs
       	/*
       	 * Re-mark only the fresh packs as kept so that objects in
     @@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct
       	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);
     -+	}
     ++	repo_invalidate_kept_pack_caches(the_repository);
       
       	if (prepare_revision_walk(&revs))
       		die(_("revision walk setup failed"));
     @@ odb/source-packed.h: struct odb_source_packed {
       	 * 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()`.
     ++	 * `repo_invalidate_kept_pack_caches()`.
       	 */
       	struct {
       		struct packed_git **packs;
     @@ packfile.c: int packfile_fill_entry(struct packed_git *p,
       	return 1;
       }
       
     -+void packfile_store_invalidate_kept_pack_cache(struct odb_source_packed *store)
     ++static void invalidate_kept_pack_cache(struct odb_source_packed *store)
      +{
      +	FREE_AND_NULL(store->kept_cache.packs);
      +	store->kept_cache.flags = 0;
      +}
     ++
     ++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);
     ++	}
     ++}
      +
       static void maybe_invalidate_kept_pack_cache(struct odb_source_packed *store,
       					     unsigned flags)
     @@ packfile.c: static void maybe_invalidate_kept_pack_cache(struct odb_source_packe
       		return;
      -	FREE_AND_NULL(store->kept_cache.packs);
      -	store->kept_cache.flags = 0;
     -+	packfile_store_invalidate_kept_pack_cache(store);
     ++	invalidate_kept_pack_cache(store);
       }
       
       struct packed_git **packfile_store_get_kept_pack_cache(struct odb_source_packed *store,
     @@ packfile.h: enum kept_pack_type {
       						       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.
     ++ * Drop every packfile store's 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);
     ++void repo_invalidate_kept_pack_caches(struct repository *r);
      +
       struct pack_window {
       	struct pack_window *next;
 3:  b76e06a467 = 3:  ff2146d002 pack-objects: sort --keep-pack list for lookup
 4:  20a051cfb6 = 4:  148175cfa0 pack-objects: add --keep-pack-from-file
 5:  4684fd8552 = 5:  8f3b9d9ee0 repack: tell pack-objects which packs are kept

-- 
gitgitgadget