Re: [PATCH 08/17] builtin/pack-objects.c: --cruft without expiration

On Mon, Dec 06, 2021 at 04:44:31PM -0500, Derrick Stolee wrote:

On 11/29/2021 5:25 PM, Taylor Blau wrote:

quoted

Generating a non-expiring cruft packs works as follows:

I had trouble parsing the documentation changes below, so I came back
to this commit message to see if that helps.

quoted

  - Callers provide a list of every pack they know about, and indicate
    which packs are about to be removed.

This corresponds to the list over stdin.

quoted

  - All packs which are going to be removed (we'll call these the
    redundant ones) are marked as kept in-core, as well as any packs
    that `pack-objects` found but the caller did not specify.

Ok, so as an implementation detail we mark these as keep packs.

quoted

    These packs are presumed to have entered the repository between
    the caller collecting packs and invoking `pack-objects`. Since we
    do not want to include objects in these packs (because we don't know
    which of their objects are or aren't reachable), these are also
    marked as kept in-core.

Here, "are presumed" is doing a lot of work. Theoretically, there could
be three categories:

1. This pack was just repacked and will be removed because all of its
   objects were placed into new objects.

2. Either this pack was repacked and contains important reachable objects
   OR we did a repack of reachable objects and this pack contained some
   extra, unreachable objects.

3. This pack was added to the repository while creating those repacked
   packs from category 2, so we don't know if things are reachable or
   not.

So, the packs that we discover on-disk but are not specified over stdin
are in this third category, but these are grouped with category 1 as we
will treat them the same.

Ah, I think I caused some unintentional confusion by attaching "are
presumed" to "these packs", when it wasn't clear that "these packs"
meant "ones that aren't listed over stdin".

Since the caller is supposed to provide a complete picture of the
repository as they see it, any packs known to the pack-objects process
that aren't mentioned over stdin are assumed to have entered the
repository after the caller was spun up.

I'll clarify this section of the commit message, since I agree it is
unnecessarily confusing.

quoted

  - Then, we enumerate all objects in the repository, and add them to
    our packing list if they do not appear in an in-core kept pack.

Here, we are looking at all of the objects in category 2 as well as
loose objects.

We're enumerating any objects that aren't in packs which are marked as
kept in-core (along with loose objects which don't appear in packs that
are marked as kept in-core).

The in-core kept packs are ones that the caller (and I find it's helpful
to read "the caller" as "git repack") has marked as "will delete". So
the non in-core pack(s) that we're looking at here contain all reachable
objects (e.g., like you would get with `git repack -A`).

quoted

+	Packs unreachable objects into a separate "cruft" pack, denoted
+	by the existence of a `.mtimes` file. Pack names provided over
+	stdin indicate which packs will remain after a `git repack`.
+	Pack names prefixed with a `-` indicate those which will be
+	removed. (...)

This description is too tied to 'git repack'. Can we describe the
input using terms independent of the 'git repack' operation? I need
to keep reading.

quoted

(...) The contents of the cruft pack are all objects not
+	contained in the surviving packs specified by `--keep-pack`)

Now you use --keep-pack, which is a way of specifying a pack as
"in-core keep" which was not in your commit message. Here, we also
don't link the packs over stdin to the concept of keep packs.

The mention of `--keep-pack` is a mistake left over from a previous
version; thanks for spotting. Here's a version of the first paragraph
from this piece of documentation which is less tied to `git repack` and
hopefully a little clearer:

    --cruft::
            Packs unreachable objects into a separate "cruft" pack, denoted
            by the existence of a `.mtimes` file. Typically used by `git
            repack --cruft`. Callers provide a list of pack names and
            indicate which packs will remain in the repository, along with
            which packs will be deleted (indicated by the `-` prefix). The
            contents of the cruft pack are all objects not contained in the
            surviving packs which have not exceeded the grace period (see
            `--cruft-expiration` below), or which have exceeded the grace
            period, but are reachable from an other object which hasn't.

quoted

+	which have not exceeded the grace period (see
+	`--cruft-expiration` below), or which have exceeded the grace
+	period, but are reachable from an other object which hasn't.

And now we think about the grace period! There is so much going on
that I need to break it down to understand.

  An object is _excluded_ from the new cruft pack if

  1. It is reachable from at least one reference.
  2. It is in a pack from stdin prefixed with "-"
  3. It is in a pack specified by `--keep-pack`
  4. It is in an existing cruft pack and the .mtimes file states
     that its mtime is at least as recent as the time specified by
     the --cruft-expiration option.

Breaking it down into a list like this helps me, at least. I'm not
sure what the best way would look like.

Given some expiration T, cruft packs contain all unreachable objects
which are newer than T, along with any cruft objects (i.e., those not
directly reachable from any ref) which are older than T, but reachable
from another cruft object newer than T.

Thanks,
Taylor