From: Elijah Newren <newren@gmail.com>
Date: Thu, 01 Oct 2026 23:22:11 GMT
Subject: Re: [PATCH 2/4] pack-objects: ensure tree/tag closure with '--stdin-packs=follow'
Message-ID: <CABPp-BHE662t9aaNcZ4DZ+2AU_C7jR7_VyHZwt2Tm8JSPE3JZw@mail.gmail.com>
In-Reply-To: <a78a38ca-b08a-4194-b17c-b8802e4d43a7@gmail.com>

On Wed, Sep 30, 2026 at 3:23 PM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 9/29/2026 9:28 PM, Taylor Blau wrote:
>
> > Add trees and tags from included and '!' packs (and loose ones with
> > '--unpacked') as roots in '--stdin-packs=follow' mode. This rescues
> > their descendants even when no input commit reaches them. Walk these
> > roots after the existing traversal, preserving the `SEEN` bit to avoid
> > redundant traversals. Ensure that the walk takes place *after* the
> > existing traversal so that we don't lose the path prefix used for trees
> > and blobs wherever possible.
>
> > @@ -3807,6 +3807,7 @@ static int stdin_packs_hints_nr;
> >  struct stdin_packs_context {
> >       struct rev_info *revs;
> >       enum stdin_packs_mode mode;
> > +     struct oid_array extra_roots;
>
> I believe this should be an oidset to avoid adding duplicate objects
> that appear multiple times. The order of these extra roots doesn't
> matter (such as in a --topo-order walk). We only care about the
> binary "reachable or not?" question.

Is that true? After iterating with copilot for a while, it says:

The walk supplies paths to pack-objects, and those paths affect more
than reachability. In particular, they affect both namehash-based
delta selection and path-based attributes.

First, it is possible to demonstrate a pack-quality regression with a
vanilla repository configuration:

test_expect_success '--stdin-packs=follow preserves namehash ordering' '
    test_when_finished "rm -rf repo" &&
    git init repo &&
    (
        cd repo &&

        mkdir sub &&
        test-tool genrandom similar 8192 >sub/a &&
        cp sub/a sub/c &&
        printf x >>sub/c &&

        for name in \
            b yzz9 zzz9 aazz9 abzz9 aczz9 \
            adyz9 adzz9 aeyz9 aezz9 afyz9
        do
            test-tool genrandom "unrelated-9-$name" 8192 >"$name" ||
            return 1
        done &&
        git add . &&
        git commit -m base &&

        git rev-parse HEAD^{tree} HEAD:sub >in &&
        P=$(git pack-objects --window=0 $packdir/pack <in) &&
        echo "pack-$P.pack" >in &&

        git pack-objects --stdin-packs=follow \
            --no-reuse-delta $packdir/pack <in &&
        rm "$packdir/pack-$P.pack" "$packdir/pack-$P.idx" &&
        git prune-packed &&

        printf "%s\n" HEAD:sub/a HEAD:sub/c |
            git cat-file --batch-check="%(deltabase)" >actual &&
        printf "%s\n" "$(git rev-parse HEAD:sub/c)" \
            "$ZERO_OID" >expect &&
        test_cmp expect actual
    )
'
The funny-looking names make the test deterministic: their namehashes
fall between the hashes for a and c, but not between those for sub/a
and sub/c. There are enough of them to fill the normal default delta
window. --window=0 on the input pack and --no-reuse-delta on the
output merely ensure that the test observes the new delta search; the
output pack uses the normal default window.

With the submitted oid_array, the parent-first input-pack order is
preserved. sub/a and sub/c remain adjacent in the namehash sort, and
sub/a is written as a six-byte delta against sub/c.

With the straightforward oidset conversion, hash iteration visits the
subtree first. The blobs are named a and c, the unrelated blobs
separate them in the namehash sort, and both are written in full. Both
packs are valid, so this is a pack-quality regression rather than
repository corruption.

There is also a shorter, but admittedly more contrived, example using
path-based attributes:

test_expect_success '--stdin-packs=follow preserves paths for attributes' '
    test_when_finished "rm -rf repo" &&
    git init repo &&
    (
        cd repo &&

        echo "sub/* -delta" >.gitattributes &&
        mkdir sub &&
        test-tool genrandom seed-2 8192 >sub/a &&
        cp sub/a sub/b &&
        echo modified >>sub/b &&
        git add . &&
        git commit -m base &&

        git rev-parse HEAD^{tree} HEAD:sub >in &&
        P=$(git pack-objects $packdir/pack <in) &&
        echo "pack-$P.pack" >in &&

        git pack-objects --stdin-packs=follow $packdir/pack <in &&
        git prune-packed &&

        printf "%s\n" HEAD:sub/a HEAD:sub/b |
            git cat-file --batch-check="%(deltabase)" >actual &&
        printf "%s\n" "$ZERO_OID" "$ZERO_OID" >expect &&
        test_cmp expect actual
    )
'
With the oid_array and parent-first pack order, the blobs are visited
as sub/a and sub/b, so sub/* -delta applies. With the oidset, the
subtree is visited first and the blobs are seen as a and b, so one is
delta-compressed. When the root is processed later, the subtree is
already marked SEEN and is not revisited with the sub/ prefix.

The oid_array does not manufacture parent-before-child ordering if the
input pack itself has the subtree first; this path information is
explicitly best-effort. But it preserves a useful order when one
exists, whereas an oidset discards it.