[PATCH] repack: do not rebuild packs on --dry-run

[PATCH] repack: do not rebuild packs on --dry-run

[Siddharth Shrimali]

"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 command
still exits successfully, so the user is not told that the repository
was modified.

The existing guard only skips the implied "delete_redundant = 1", so
it does not stop an explicit -d, nor the new pack from being written.

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>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
Bug report:
https://lore.kernel.org/git/CACgTecOm+=vbf50tZNXhcYvRi1ZTsQwbjVoJAbQqs2CmXdJCxg@mail.gmail.com/

 builtin/repack.c                |  8 ++++++++
 t/t7706-repack-drop-filtered.sh | 19 +++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/builtin/repack.c b/builtin/repack.c
index c4360382c1..c048053912 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -391,6 +391,14 @@ int cmd_repack(int argc,
 			oidset_iter_init(&drop_oids, &iter);
 			while ((oid = oidset_iter_next(&iter)))
 				printf("%s\n", oid_to_hex(oid));
+
+			/*
+			 * add an exit here, so that dry run does not
+			 * go on to rebuild any pack or delete anything, even
+			 * if the user explicitly asked for -d
+			 */
+			ret = 0;
+			goto cleanup;
 		}
 	}
 
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
@@ -135,6 +135,25 @@ test_expect_success '--dry-run does not remove the filtered objects' '
 	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) &&
-- 
2.56.0

Re: [PATCH] repack: do not rebuild packs on --dry-run

[D. Ben Knoble]

Hi Siddharth,

On Thu, Oct 8, 2026 at 2:25 AM Siddharth Shrimali
<r.siddharth.shrimali@gmail.com> wrote:
>
> "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 command
> still exits successfully, so the user is not told that the repository
> was modified.
>
> The existing guard only skips the implied "delete_redundant = 1", so
> it does not stop an explicit -d, nor the new pack from being written.
>
> 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.

Makes sense

> Reported-by: Coy Geek <coygeek@gmail.com>
> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
> ---
> Bug report:
> https://lore.kernel.org/git/CACgTecOm+=vbf50tZNXhcYvRi1ZTsQwbjVoJAbQqs2CmXdJCxg@mail.gmail.com/
>
>  builtin/repack.c                |  8 ++++++++
>  t/t7706-repack-drop-filtered.sh | 19 +++++++++++++++++++
>  2 files changed, 27 insertions(+)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index c4360382c1..c048053912 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -391,6 +391,14 @@ int cmd_repack(int argc,
>                         oidset_iter_init(&drop_oids, &iter);
>                         while ((oid = oidset_iter_next(&iter)))
>                                 printf("%s\n", oid_to_hex(oid));

Just outside the patch context is the "if (dry_run)" conditional, so
this is the right place. (In this case, formatting with a larger "-U"
value might help.)

> +
> +                       /*
> +                        * add an exit here, so that dry run does not
> +                        * go on to rebuild any pack or delete anything, even
> +                        * if the user explicitly asked for -d
> +                        */

2 notes:

1. "add an exit" will stop making sense as soon as the patch becomes a
commit; that is, it only makes sense in the context of proposed
changes. Once those changes are part of the code base, the comment and
code are not adding anything. They simply are. So, if we need a
comment (see 2), it might be best phrased as "exit here so that […]",
keeping some of your original wording. Or we could be more terse:
"skip non-dry-run operations" or something.
2. Do we need such a comment, I wonder? git-blame will point folks
towards this commit :)

> +                       ret = 0;
> +                       goto cleanup;

This matches the pattern elsewhere in this procedure, so that looks good.

>                 }
>         }
>
> 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
> @@ -135,6 +135,25 @@ test_expect_success '--dry-run does not remove the filtered objects' '
>         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) &&
> --
> 2.56.0

Thanks for adding a test to cover this. It looks reasonable to me, but
I didn't study the surrounding tests to think very critically about
it.

-- 
D. Ben Knoble

Re: [PATCH] repack: do not rebuild packs on --dry-run

[Junio C Hamano]

Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:

> "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 command
> still exits successfully, so the user is not told that the repository
> was modified.

Very interesting finding.  I am curious if this was the case from
the beginning, or we broke --dry-run unknowingly as a side effect of
some unrelated changes.  If it is not too much, can you bisect and
document where we broke it in the log message?

> The existing guard only skips the implied "delete_redundant = 1", so
> it does not stop an explicit -d, nor the new pack from being written.
>
> 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>
> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
> ---
> Bug report:
> https://lore.kernel.org/git/CACgTecOm+=vbf50tZNXhcYvRi1ZTsQwbjVoJAbQqs2CmXdJCxg@mail.gmail.com/
>
>  builtin/repack.c                |  8 ++++++++
>  t/t7706-repack-drop-filtered.sh | 19 +++++++++++++++++++
>  2 files changed, 27 insertions(+)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index c4360382c1..c048053912 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -391,6 +391,14 @@ int cmd_repack(int argc,
>  			oidset_iter_init(&drop_oids, &iter);
>  			while ((oid = oidset_iter_next(&iter)))
>  				printf("%s\n", oid_to_hex(oid));
> +
> +			/*
> +			 * add an exit here, so that dry run does not
> +			 * go on to rebuild any pack or delete anything, even
> +			 * if the user explicitly asked for -d
> +			 */
> +			ret = 0;
> +			goto cleanup;
>  		}
>  	}
>  
> 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
> @@ -135,6 +135,25 @@ test_expect_success '--dry-run does not remove the filtered objects' '
>  	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) &&

Re: [PATCH] repack: do not rebuild packs on --dry-run

[Siddharth Shrimali]

Hi Ben,

On Thu, 8 Oct 2026 at 18:26, D. Ben Knoble <ben.knoble@gmail.com> wrote:
> Just outside the patch context is the "if (dry_run)" conditional, so
> this is the right place. (In this case, formatting with a larger "-U"
> value might help.)
Thanks, Ben! I will do that in v2

> > +
> > +                       /*
> > +                        * add an exit here, so that dry run does not
> > +                        * go on to rebuild any pack or delete anything, even
> > +                        * if the user explicitly asked for -d
> > +                        */
>
> 2 notes:
>
> 1. "add an exit" will stop making sense as soon as the patch becomes a
> commit; that is, it only makes sense in the context of proposed
> changes. Once those changes are part of the code base, the comment and
> code are not adding anything. They simply are. So, if we need a
> comment (see 2), it might be best phrased as "exit here so that […]",
> keeping some of your original wording. Or we could be more terse:
> "skip non-dry-run operations" or something.
> 2. Do we need such a comment, I wonder? git-blame will point folks
> towards this commit :)
>
Oh yeah right, it will stop making sense without any context.
Yeah git blame does the thing, but I feel like adding a short comment for that

Will send a v2 soon

Thanks,
Siddharth

Re: [PATCH] repack: do not rebuild packs on --dry-run

[Siddharth Shrimali]

Hi Junio,

On Thu, 8 Oct 2026 at 19:53, Junio C Hamano <gitster@pobox.com> wrote:
> Very interesting finding.  I am curious if this was the case from
> the beginning, or we broke --dry-run unknowingly as a side effect of
> some unrelated changes.  If it is not too much, can you bisect and
> document where we broke it in the log message?

unfortunately, this was a mistake on my end and it has been broken
since --dry-run was introduced in 1e746b00aa. the --dry-run
block never returned early, so it always went on to do a regular
repack, including the deletion when -d is given explicitly.

I will document this in the log message in v2

Thanks,
Siddharth

[PATCH v2] repack: do not rebuild packs on --dry-run

[Siddharth Shrimali]

"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

 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

Re: [PATCH v2] repack: do not rebuild packs on --dry-run

[Ben Knoble]

> 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
> 

Re: [PATCH v2] repack: do not rebuild packs on --dry-run

[Junio C Hamano]

Ben Knoble <ben.knoble@gmail.com> writes:

>>  ...
>> 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

Thanks for writing and reviewing.  Let me mark the topic for 'next'.

Re: [PATCH v2] repack: do not rebuild packs on --dry-run

[Patrick Steinhardt]

On Sun, Oct 11, 2026 at 02:30:50AM +0530, Siddharth Shrimali wrote:
>  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;
>  		}
>  	}

Okay, makes sense. I wonder though, we later have a check like this:

	if (pack_everything & ALL_INTO_ONE) {
		repack_promisor_objects(repo, &po_args, &names, packtmp,
			(drop_filtered && !dry_run) ? &drop_oids : NULL);

Is the `!dry_run` check redundant now because of the above early exit?

Patrick

Re: [PATCH v2] repack: do not rebuild packs on --dry-run

[Siddharth Shrimali]

Hi Patrick,
Sorry for the late reply, I was a bit occupied with my university work.

On Tue, 13 Oct 2026 at 17:00, Patrick Steinhardt <ps@pks.im> wrote:
> Okay, makes sense. I wonder though, we later have a check like this:
>
>         if (pack_everything & ALL_INTO_ONE) {
>                 repack_promisor_objects(repo, &po_args, &names, packtmp,
>                         (drop_filtered && !dry_run) ? &drop_oids : NULL);
>
> Is the `!dry_run` check redundant now because of the above early exit?

oh yes, now that we have an early exit, a dry run never gets past
the candidate listing, so dry_run is always false by the time we
reach repack_promisor_objects(). the same applies to the earlier

    if (!dry_run)
        delete_redundant = 1;

since nothing reads delete_redundant before the early exit

I will drop both checks in v3.

Thanks,
Siddharth

[PATCH v3] repack: do not rebuild packs on --dry-run

[Siddharth Shrimali]

"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.

A dry run can no longer get past that point, so the two "!dry_run"
checks that tried to keep it from deleting or dropping anything are
now redundant.

Reported-by: Coy Geek <coygeek@gmail.com>
Helped-by: D. Ben Knoble <ben.knoble@gmail.com>
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
Bug report:
https://lore.kernel.org/git/CACgTecOm+=vbf50tZNXhcYvRi1ZTsQwbjVoJAbQqs2CmXdJCxg@mail.gmail.com/

Changes since v2:
 - dropped the "!dry_run" checks around the implied -d and the
   repack_promisor_objects() call, which the early return made
   redundant

 builtin/repack.c                | 14 ++++++++++----
 t/t7706-repack-drop-filtered.sh | 19 +++++++++++++++++++
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index c4360382c1..19894804f0 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -347,18 +347,17 @@ int cmd_repack(int argc,
 		}
 
 		write_bitmaps = 0;
 
 		/*
 		 * Dropping objects means rebuilding the promisor packs
 		 * without them and then removing the old packs, so the
-		 * redundant packs must be deleted. Imply -d on a real run.
+		 * redundant packs must be deleted. Imply -d.
 		 */
-		if (!dry_run)
-			delete_redundant = 1;
+		delete_redundant = 1;
 
 		ret = enumerate_promisor_blobs(repo, &po_args.filter_options, &drop_oids);
 
 		if (ret)
 			goto cleanup;
 
 		/*
@@ -387,14 +386,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",
@@ -501,15 +507,15 @@ int cmd_repack(int argc,
 			strvec_push(&cmd.args, "--write-bitmap-index-quiet");
 	}
 	if (use_delta_islands)
 		strvec_push(&cmd.args, "--delta-islands");
 
 	if (pack_everything & ALL_INTO_ONE) {
 		repack_promisor_objects(repo, &po_args, &names, packtmp,
-			(drop_filtered && !dry_run) ? &drop_oids : NULL);
+			drop_filtered ? &drop_oids : NULL);
 
 		if (existing_packs_has_non_kept(&existing) &&
 		    delete_redundant &&
 		    !(pack_everything & PACK_CRUFT)) {
 			for_each_string_list_item(item, &names) {
 				strvec_pushf(&cmd.args, "--keep-pack=%s-%s.pack",
 					     packtmp_name, item->string);
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

Re: [PATCH v3] repack: do not rebuild packs on --dry-run

[Junio C Hamano]

Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:

> "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.

As a part of an answer to a question "was this broken from the
beginning, or is it a regression?", the last sentence may have
value, but as part of a standalone proposed log message, it sounds
weird, don't you think?  You already clearly said this was broken
since the beginning.

> 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.

Makes sense.

> A dry run can no longer get past that point, so the two "!dry_run"
> checks that tried to keep it from deleting or dropping anything are
> now redundant.

This makes sense as an explanation on the differences between v2 and
v3, but for those who are reading v3 standalone (which are our
primary target audience, i.e., those who read "git log -p" in the
future), it is on the borderline redundant, as it is a natural part
of what you already described, i.e., return right after giving a
list without doing anything.

Will queue as-is.  I think it is good enough, even though I did
nitpick a couple of points in the above.

Thanks.

> Reported-by: Coy Geek <coygeek@gmail.com>
> Helped-by: D. Ben Knoble <ben.knoble@gmail.com>
> Helped-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
> ---
> Bug report:
> https://lore.kernel.org/git/CACgTecOm+=vbf50tZNXhcYvRi1ZTsQwbjVoJAbQqs2CmXdJCxg@mail.gmail.com/
>
> Changes since v2:
>  - dropped the "!dry_run" checks around the implied -d and the
>    repack_promisor_objects() call, which the early return made
>    redundant

>  builtin/repack.c                | 14 ++++++++++----
>  t/t7706-repack-drop-filtered.sh | 19 +++++++++++++++++++
>  2 files changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index c4360382c1..19894804f0 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -347,18 +347,17 @@ int cmd_repack(int argc,
>  		}
>  
>  		write_bitmaps = 0;
>  
>  		/*
>  		 * Dropping objects means rebuilding the promisor packs
>  		 * without them and then removing the old packs, so the
> -		 * redundant packs must be deleted. Imply -d on a real run.
> +		 * redundant packs must be deleted. Imply -d.
>  		 */
> -		if (!dry_run)
> -			delete_redundant = 1;
> +		delete_redundant = 1;
>  
>  		ret = enumerate_promisor_blobs(repo, &po_args.filter_options, &drop_oids);
>  
>  		if (ret)
>  			goto cleanup;
>  
>  		/*
> @@ -387,14 +386,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",
> @@ -501,15 +507,15 @@ int cmd_repack(int argc,
>  			strvec_push(&cmd.args, "--write-bitmap-index-quiet");
>  	}
>  	if (use_delta_islands)
>  		strvec_push(&cmd.args, "--delta-islands");
>  
>  	if (pack_everything & ALL_INTO_ONE) {
>  		repack_promisor_objects(repo, &po_args, &names, packtmp,
> -			(drop_filtered && !dry_run) ? &drop_oids : NULL);
> +			drop_filtered ? &drop_oids : NULL);
>  
>  		if (existing_packs_has_non_kept(&existing) &&
>  		    delete_redundant &&
>  		    !(pack_everything & PACK_CRUFT)) {
>  			for_each_string_list_item(item, &names) {
>  				strvec_pushf(&cmd.args, "--keep-pack=%s-%s.pack",
>  					     packtmp_name, item->string);
> 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 &&