git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/2] builtin/repack: fix `--keep-unreachable` when there are no packs

From
Junio C Hamano <gitster@pobox.com>
Date
Feb 4, 2025, 13:28 UTC
Message-ID
<xmqq8qqlx2ja.fsf@gitster.g>
In-Reply-To
<20250203-b4-pks-repack-unreachable-objects-wo-packfiles-v1-2-7c4d69c5072c@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 5 quoted lines
> The "--keep-unreachable" flag is supposed to append any unreachable
> objects to the newly written pack. This flag is explicitly documented as
> appending both packed and loose unreachable objects to the new packfile.
> And while this works alright when repacking with preexisting packfiles,
> it stops working when the repository does not have any packfiles at all.

Chuckle. That's a cute corner case the developers never considered, it seems ;-).

Show 41 quoted lines
> The root cause are the conditions used to decide whether or not we want
> to append "--pack-loose-unreachable" to git-pack-objects(1). There are
> a couple of conditions here:
>
>   - `has_existing_non_kept_packs()` checks whether there are existing
>     packfiles. This condition makes sense to guard "--keep-pack=",
>     "--unpack-unreachable" and "--keep-unreachable", because all of
>     these flags only make sense in combination with existing packfiles.
>     But it does not make sense to disable `--pack-loose-unreachable`
>     when there aren't any preexisting packfiles, as loose objects can be
>     packed into the new packfile regardless of that.
>
>   - `delete_redundant` checks whether we want to delete any objects or
>     packs that are about to become redundant. The documentation of
>     `--keep-unreachable` explicitly says that `git repack -ad` needs to
>     be executed for the flag to have an effect.
>
>     It is not immediately obvious why such redundant objects need to be
>     deleted in order for "--pack-unreachable-objects" to be effective.
>     But as things are working as documented this is nothing we'll change
>     for now.
>
>   - `pack_everything & PACK_CRUFT` checks that we're not creating a
>     cruft pack. This condition makes sense in the context of
>     "--pack-loose-unreachable", as unreachable objects would end up in
>     the cruft pack anyway.
>
> So while the second and third condition are sensible, it does not make
> any sense to condition `--pack-loose-unreachable` on the existence of
> packfiles.
>
> Fix the bug by splitting out the "--pack-loose-unreachable" and only
> making it depend on the second and third condition. Like this, loose
> unreachable objects will be packed regardless of any preexisting
> packfiles.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/repack.c  | 5 ++++-
>  t/t7700-repack.sh | 2 +-
>  2 files changed, 5 insertions(+), 2 deletions(-)
Nicely analized and described.  Thanks.
Previous: Jeff KingNext: Patrick Steinhardt
Message 6 of 15 in “builtin/repack: fix `--keep-unreachable` when there are no packs”
  1. 0/2 builtin/repack: fix `--keep-unreachable` when there are no packsPatrick Steinhardt, Feb 3, 2025
  2. 2/2 builtin/repack: fix `--keep-unreachable` when there are no packsPatrick Steinhardt, Feb 3, 2025
  3. Jeff KingFeb 4, 2025
  4. Patrick SteinhardtFeb 4, 2025
  5. Jeff KingFeb 4, 2025
  6. Junio C HamanoFeb 4, 2025
  7. 1/2 t7700: add tests for `--keep-unreachable`Patrick Steinhardt, Feb 3, 2025
  8. Junio C HamanoFeb 3, 2025
  9. Jeff KingFeb 3, 2025
  10. Junio C HamanoFeb 3, 2025
  11. Jeff KingFeb 4, 2025
  12. Patrick SteinhardtFeb 4, 2025
  13. builtin/repack: fix `--keep-unreachable` when there are no packsPatrick Steinhardt, Feb 4, 2025
  14. Jeff KingFeb 4, 2025
  15. Patrick SteinhardtFeb 5, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.