From: Ben Knoble <ben.knoble@gmail.com>
Date: Sat, 10 Oct 2026 21:06:18 GMT
Subject: Re: [PATCH v2] repack: do not rebuild packs on --dry-run
Message-ID: <BBCEC2F7-CA8A-4460-8A4F-D8305D3E9134@gmail.com>
In-Reply-To: <20261010210050.76064-1-r.siddharth.shrimali@gmail.com>


> Le 10 oct. 2026 à 17:01, Siddharth Shrimali <r.siddharth.shrimali@gmail.com> a écrit :
> 
> "git repack --drop-filtered --dry-run" is documented to list the
> objects that would be dropped "without rebuilding any pack or
> deleting anything", but it does both.
> 
> This is a bug in cmd_repack(): after printing the candidates, the
> --dry-run block falls through into the regular repack code.
> repack_promisor_objects() writes a new promisor pack, and with -d,
> existing_packs_remove_redundant() deletes the old packs.
> 
> The existing guard only skips the implied "delete_redundant = 1", so
> it does not stop an explicit -d, and it does not stop the new pack
> from being written either.
> 
> This has been broken since --dry-run was introduced in 1e746b00aa
> (builtin/repack: add --drop-filtered and --dry-run options). The
> --dry-run block never returned early, so a dry run has always gone
> on to do a regular repack. It was not broken by a later change.
> 
> Fix it by returning right after the candidates are listed, and add a
> test that checks the pack directory is unchanged with and without -d.
> 
> Reported-by: Coy Geek <coygeek@gmail.com>
> Helped-by: D. Ben Knoble <ben.knoble@gmail.com>
> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
> ---
> Bug report:
> https://lore.kernel.org/git/CACgTecOm+=vbf50tZNXhcYvRi1ZTsQwbjVoJAbQqs2CmXdJCxg@mail.gmail.com/
> 
> Changes since v1:
> - reworded the comment
> - documented the bug in the log message

Thanks, I’m happy with this version. You may add my Reviewed-by, if you like

> builtin/repack.c                |  7 +++++++
> t/t7706-repack-drop-filtered.sh | 19 +++++++++++++++++++
> 2 files changed, 26 insertions(+)
> 
> diff --git a/builtin/repack.c b/builtin/repack.c
> index c4360382c1..7797972642 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -387,14 +387,21 @@ int cmd_repack(int argc,
>        if (dry_run) {
>            struct oidset_iter iter;
>            const struct object_id *oid;
> 
>            oidset_iter_init(&drop_oids, &iter);
>            while ((oid = oidset_iter_next(&iter)))
>                printf("%s\n", oid_to_hex(oid));
> +
> +            /*
> +             * Only list the candidates to be dropped; skip
> +             * the actual repack
> +             */
> +            ret = 0;
> +            goto cleanup;
>        }
>    }
> 
>    if (delete_redundant && repo->repository_format_precious_objects)
>        die(_("cannot delete packs in a precious-objects repo"));
> 
>    die_for_incompatible_opt3(unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE), "-A",
> diff --git a/t/t7706-repack-drop-filtered.sh b/t/t7706-repack-drop-filtered.sh
> index cb36115834..a1c475e4ec 100755
> --- a/t/t7706-repack-drop-filtered.sh
> +++ b/t/t7706-repack-drop-filtered.sh
> @@ -131,14 +131,33 @@ test_expect_success '--dry-run does not remove the filtered objects' '
>    git -C repo -c repack.writeBitmaps=false \
>        repack --drop-filtered --filter=blob:limit=1k --dry-run -a >out &&
> 
>    # Candidate blob must still be present after a dry run.
>    git -C repo cat-file -e "$BIG"
> '
> 
> +test_expect_success '--dry-run leaves the pack directory untouched' '
> +    BIG=$(cat big_oid) &&
> +    packdir=repo/.git/objects/pack &&
> +
> +    for opt in "" -d
> +    do
> +        ls $packdir >before &&
> +
> +        git -C repo -c repack.writeBitmaps=false \
> +            repack --drop-filtered --filter=blob:limit=1k \
> +            --dry-run -a $opt >out &&
> +
> +        ls $packdir >after &&
> +        test_cmp before after &&
> +        test_grep "$BIG" out &&
> +        git -C repo cat-file -e "$BIG" || return 1
> +    done
> +'
> +
> test_expect_success '--drop-filtered removes the promisor blob locally' '
>    BIG=$(cat big_oid) &&
>    SMALL=$(cat small_oid) &&
> 
>    git -C repo -c repack.writeBitmaps=false \
>        repack --drop-filtered --filter=blob:limit=1k -a &&
> 
> --
> 2.56.0
>