From: Taylor Blau <me@ttaylorr.com>
Date: Fri, 09 Jan 2026 23:32:12 GMT
Subject: Re: [PATCH 1/5] builtin/pack-objects: exclude promisor objects with "--stdin-packs"
Message-ID: <aWGP/Lp2Eo03F7vN@nand.local>
In-Reply-To: <20260105-pks-geometric-repack-with-promisors-v1-1-c4660573437e@pks.im>

On Mon, Jan 05, 2026 at 02:16:41PM +0100, Patrick Steinhardt wrote:
> It is currently not possible to combine "--exclude-promisor-objects"
> with "--stdin-packs" because both flags want to set up a revision walk
> to enumerate the objects to pack. In a subsequent commit though we want
> to extend geometric repacks to support promisor objects, and for that we
> need to handle the combination of both flags.
>
> There are two cases we have to think about here:
>
>   - "--stdin-packs" asks us to pack exactly the objects part of the
>     specified packfiles. It is somewhat questionable what to do in the
>     case where the user asks us to exclude promisor objects, but at the
>     same time explicitly passes a promisor pack to us. For now, we
>     simply abort the request as it is self-contradicting. As we have
>     also been dying before this commit there is no regression here.

I was wondering whether or not this is the case, because we don't have
an explicit `die()` here or a incompatible pair of options declared. But
it does die(), although the message is somewhat confusing:

    $ git.compile pack-objects --stdin-packs --exclude-promisor-objects --stdout >/dev/null
    fatal: cannot use internal rev list with --stdin-packs

;-).

>   - "--stdin-packs=follow" does the same as the first flag, but it also
>     asks us to include all objects transitively reachable from any
>     object in the packs we are about to repack. This is done by doing
>     the revision walk mentioned further up. Luckily, fixing this case is
>     trivial: we only need to modify the revision walk to also set the
>     `exclude_promisor_objects` field.

Hmm. I'm not totally sure if I'm following why we handle this case
separately. Could you elaborate?

> Note that we do not support the "--exclude-promisor-objects-best-effort"
> flag for now as we don't need it to support geometric repacking with
> promisor objects.

I didn't know we had such an option in the first place, but it looks
like it behaves similarly wrt. its incompatibility with `--stdin-packs`.

> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c

I'll hold off on commenting on the code to give us a chance to discuss
the above.

> diff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh
> index 4a8df5a389..cd949025b9 100755
> --- a/t/t5331-pack-objects-stdin.sh
> +++ b/t/t5331-pack-objects-stdin.sh
> @@ -319,6 +319,45 @@ test_expect_success '--stdin-packs=follow walks into unknown packs' '
>  	)
>  '
>
> +test_expect_success '--stdin-packs with promisors' '
> +	test_when_finished "rm -fr repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		git config set maintenance.auto false &&
> +		git remote add promisor garbage &&
> +		git config set remote.promisor.promisor true &&
> +
> +		for c in A B C D
> +		do
> +			echo "$c" >file &&
> +			git add file &&
> +			git commit --message "$c" &&
> +			git tag "$c" || return 1

Unless these changes all have to live in the same file, could this
instead be written as:

    for c in A B C D
    do
        test_commit "$c" || return 1
    done &&
    # ...

?

Thanks,
Taylor