{"thread":{"id":"66486","subject":"[PATCH] repack: do not rebuild packs on --dry-run","startedAt":"2026-10-08T06:25:21Z","lastAt":"2026-10-08T14:23:35Z","messageCount":3,"participants":["Siddharth Shrimali","D. Ben Knoble","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"554443","messageId":"20261008062521.25505-1-r.siddharth.shrimali@gmail.com","threadId":"66486","inReplyTo":null,"subject":"[PATCH] repack: do not rebuild packs on --dry-run","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-10-08T06:25:21Z","receivedAt":"2026-10-08T06:25:21Z","isPatch":true,"sender":{"key":"r.siddharth.shrimali@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183274193?v=4"},"body":"\"git repack --drop-filtered --dry-run\" is documented to list the\nobjects that would be dropped \"without rebuilding any pack or\ndeleting anything\", but it does both.\n\nThis is a bug in cmd_repack(): after printing the candidates, the\n--dry-run block falls through into the regular repack code.\nrepack_promisor_objects() writes a new promisor pack, and with -d,\nexisting_packs_remove_redundant() deletes the old packs. The command\nstill exits successfully, so the user is not told that the repository\nwas modified.\n\nThe existing guard only skips the implied \"delete_redundant = 1\", so\nit does not stop an explicit -d, nor the new pack from being written.\n\nFix it by returning right after the candidates are listed, and add a\ntest that checks the pack directory is unchanged with and without -d.\n\nReported-by: Coy Geek <coygeek@gmail.com>\nSigned-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>\n---\nBug report:\nhttps://lore.kernel.org/git/CACgTecOm+=vbf50tZNXhcYvRi1ZTsQwbjVoJAbQqs2CmXdJCxg@mail.gmail.com/\n\n builtin/repack.c                |  8 ++++++++\n t/t7706-repack-drop-filtered.sh | 19 +++++++++++++++++++\n 2 files changed, 27 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex c4360382c1..c048053912 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -391,6 +391,14 @@ int cmd_repack(int argc,\n \t\t\toidset_iter_init(&drop_oids, &iter);\n \t\t\twhile ((oid = oidset_iter_next(&iter)))\n \t\t\t\tprintf(\"%s\\n\", oid_to_hex(oid));\n+\n+\t\t\t/*\n+\t\t\t * add an exit here, so that dry run does not\n+\t\t\t * go on to rebuild any pack or delete anything, even\n+\t\t\t * if the user explicitly asked for -d\n+\t\t\t */\n+\t\t\tret = 0;\n+\t\t\tgoto cleanup;\n \t\t}\n \t}\n \ndiff --git a/t/t7706-repack-drop-filtered.sh b/t/t7706-repack-drop-filtered.sh\nindex cb36115834..a1c475e4ec 100755\n--- a/t/t7706-repack-drop-filtered.sh\n+++ b/t/t7706-repack-drop-filtered.sh\n@@ -135,6 +135,25 @@ test_expect_success '--dry-run does not remove the filtered objects' '\n \tgit -C repo cat-file -e \"$BIG\"\n '\n \n+test_expect_success '--dry-run leaves the pack directory untouched' '\n+\tBIG=$(cat big_oid) &&\n+\tpackdir=repo/.git/objects/pack &&\n+\n+\tfor opt in \"\" -d\n+\tdo\n+\t\tls $packdir >before &&\n+\n+\t\tgit -C repo -c repack.writeBitmaps=false \\\n+\t\t\trepack --drop-filtered --filter=blob:limit=1k \\\n+\t\t\t--dry-run -a $opt >out &&\n+\n+\t\tls $packdir >after &&\n+\t\ttest_cmp before after &&\n+\t\ttest_grep \"$BIG\" out &&\n+\t\tgit -C repo cat-file -e \"$BIG\" || return 1\n+\tdone\n+'\n+\n test_expect_success '--drop-filtered removes the promisor blob locally' '\n \tBIG=$(cat big_oid) &&\n \tSMALL=$(cat small_oid) &&\n-- \n2.56.0\n\n\n"},{"id":"554495","messageId":"CALnO6CD9roPhKsYRVXRuZXQckTmbCskKmvxiUyxp6vkJ03S8rQ@mail.gmail.com","threadId":"66486","inReplyTo":"20261008062521.25505-1-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH] repack: do not rebuild packs on --dry-run","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-10-08T12:56:12Z","receivedAt":"2026-10-08T12:56:12Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Hi Siddharth,\n\nOn Thu, Oct 8, 2026 at 2:25 AM Siddharth Shrimali\n<r.siddharth.shrimali@gmail.com> wrote:\n>\n> \"git repack --drop-filtered --dry-run\" is documented to list the\n> objects that would be dropped \"without rebuilding any pack or\n> deleting anything\", but it does both.\n>\n> This is a bug in cmd_repack(): after printing the candidates, the\n> --dry-run block falls through into the regular repack code.\n> repack_promisor_objects() writes a new promisor pack, and with -d,\n> existing_packs_remove_redundant() deletes the old packs. The command\n> still exits successfully, so the user is not told that the repository\n> was modified.\n>\n> The existing guard only skips the implied \"delete_redundant = 1\", so\n> it does not stop an explicit -d, nor the new pack from being written.\n>\n> Fix it by returning right after the candidates are listed, and add a\n> test that checks the pack directory is unchanged with and without -d.\n\nMakes sense\n\n> Reported-by: Coy Geek <coygeek@gmail.com>\n> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>\n> ---\n> Bug report:\n> https://lore.kernel.org/git/CACgTecOm+=vbf50tZNXhcYvRi1ZTsQwbjVoJAbQqs2CmXdJCxg@mail.gmail.com/\n>\n>  builtin/repack.c                |  8 ++++++++\n>  t/t7706-repack-drop-filtered.sh | 19 +++++++++++++++++++\n>  2 files changed, 27 insertions(+)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index c4360382c1..c048053912 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -391,6 +391,14 @@ int cmd_repack(int argc,\n>                         oidset_iter_init(&drop_oids, &iter);\n>                         while ((oid = oidset_iter_next(&iter)))\n>                                 printf(\"%s\\n\", oid_to_hex(oid));\n\nJust outside the patch context is the \"if (dry_run)\" conditional, so\nthis is the right place. (In this case, formatting with a larger \"-U\"\nvalue might help.)\n\n> +\n> +                       /*\n> +                        * add an exit here, so that dry run does not\n> +                        * go on to rebuild any pack or delete anything, even\n> +                        * if the user explicitly asked for -d\n> +                        */\n\n2 notes:\n\n1. \"add an exit\" will stop making sense as soon as the patch becomes a\ncommit; that is, it only makes sense in the context of proposed\nchanges. Once those changes are part of the code base, the comment and\ncode are not adding anything. They simply are. So, if we need a\ncomment (see 2), it might be best phrased as \"exit here so that […]\",\nkeeping some of your original wording. Or we could be more terse:\n\"skip non-dry-run operations\" or something.\n2. Do we need such a comment, I wonder? git-blame will point folks\ntowards this commit :)\n\n> +                       ret = 0;\n> +                       goto cleanup;\n\nThis matches the pattern elsewhere in this procedure, so that looks good.\n\n>                 }\n>         }\n>\n> diff --git a/t/t7706-repack-drop-filtered.sh b/t/t7706-repack-drop-filtered.sh\n> index cb36115834..a1c475e4ec 100755\n> --- a/t/t7706-repack-drop-filtered.sh\n> +++ b/t/t7706-repack-drop-filtered.sh\n> @@ -135,6 +135,25 @@ test_expect_success '--dry-run does not remove the filtered objects' '\n>         git -C repo cat-file -e \"$BIG\"\n>  '\n>\n> +test_expect_success '--dry-run leaves the pack directory untouched' '\n> +       BIG=$(cat big_oid) &&\n> +       packdir=repo/.git/objects/pack &&\n> +\n> +       for opt in \"\" -d\n> +       do\n> +               ls $packdir >before &&\n> +\n> +               git -C repo -c repack.writeBitmaps=false \\\n> +                       repack --drop-filtered --filter=blob:limit=1k \\\n> +                       --dry-run -a $opt >out &&\n> +\n> +               ls $packdir >after &&\n> +               test_cmp before after &&\n> +               test_grep \"$BIG\" out &&\n> +               git -C repo cat-file -e \"$BIG\" || return 1\n> +       done\n> +'\n> +\n>  test_expect_success '--drop-filtered removes the promisor blob locally' '\n>         BIG=$(cat big_oid) &&\n>         SMALL=$(cat small_oid) &&\n> --\n> 2.56.0\n\nThanks for adding a test to cover this. It looks reasonable to me, but\nI didn't study the surrounding tests to think very critically about\nit.\n\n-- \nD. Ben Knoble\n\n"},{"id":"554505","messageId":"xmqqwlrs2rvc.fsf@gitster.g","threadId":"66486","inReplyTo":"20261008062521.25505-1-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH] repack: do not rebuild packs on --dry-run","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-08T14:23:35Z","receivedAt":"2026-10-08T14:23:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:\n\n> \"git repack --drop-filtered --dry-run\" is documented to list the\n> objects that would be dropped \"without rebuilding any pack or\n> deleting anything\", but it does both.\n>\n> This is a bug in cmd_repack(): after printing the candidates, the\n> --dry-run block falls through into the regular repack code.\n> repack_promisor_objects() writes a new promisor pack, and with -d,\n> existing_packs_remove_redundant() deletes the old packs. The command\n> still exits successfully, so the user is not told that the repository\n> was modified.\n\nVery interesting finding.  I am curious if this was the case from\nthe beginning, or we broke --dry-run unknowingly as a side effect of\nsome unrelated changes.  If it is not too much, can you bisect and\ndocument where we broke it in the log message?\n\n> The existing guard only skips the implied \"delete_redundant = 1\", so\n> it does not stop an explicit -d, nor the new pack from being written.\n>\n> Fix it by returning right after the candidates are listed, and add a\n> test that checks the pack directory is unchanged with and without -d.\n>\n> Reported-by: Coy Geek <coygeek@gmail.com>\n> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>\n> ---\n> Bug report:\n> https://lore.kernel.org/git/CACgTecOm+=vbf50tZNXhcYvRi1ZTsQwbjVoJAbQqs2CmXdJCxg@mail.gmail.com/\n>\n>  builtin/repack.c                |  8 ++++++++\n>  t/t7706-repack-drop-filtered.sh | 19 +++++++++++++++++++\n>  2 files changed, 27 insertions(+)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index c4360382c1..c048053912 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -391,6 +391,14 @@ int cmd_repack(int argc,\n>  \t\t\toidset_iter_init(&drop_oids, &iter);\n>  \t\t\twhile ((oid = oidset_iter_next(&iter)))\n>  \t\t\t\tprintf(\"%s\\n\", oid_to_hex(oid));\n> +\n> +\t\t\t/*\n> +\t\t\t * add an exit here, so that dry run does not\n> +\t\t\t * go on to rebuild any pack or delete anything, even\n> +\t\t\t * if the user explicitly asked for -d\n> +\t\t\t */\n> +\t\t\tret = 0;\n> +\t\t\tgoto cleanup;\n>  \t\t}\n>  \t}\n>  \n> diff --git a/t/t7706-repack-drop-filtered.sh b/t/t7706-repack-drop-filtered.sh\n> index cb36115834..a1c475e4ec 100755\n> --- a/t/t7706-repack-drop-filtered.sh\n> +++ b/t/t7706-repack-drop-filtered.sh\n> @@ -135,6 +135,25 @@ test_expect_success '--dry-run does not remove the filtered objects' '\n>  \tgit -C repo cat-file -e \"$BIG\"\n>  '\n>  \n> +test_expect_success '--dry-run leaves the pack directory untouched' '\n> +\tBIG=$(cat big_oid) &&\n> +\tpackdir=repo/.git/objects/pack &&\n> +\n> +\tfor opt in \"\" -d\n> +\tdo\n> +\t\tls $packdir >before &&\n> +\n> +\t\tgit -C repo -c repack.writeBitmaps=false \\\n> +\t\t\trepack --drop-filtered --filter=blob:limit=1k \\\n> +\t\t\t--dry-run -a $opt >out &&\n> +\n> +\t\tls $packdir >after &&\n> +\t\ttest_cmp before after &&\n> +\t\ttest_grep \"$BIG\" out &&\n> +\t\tgit -C repo cat-file -e \"$BIG\" || return 1\n> +\tdone\n> +'\n> +\n>  test_expect_success '--drop-filtered removes the promisor blob locally' '\n>  \tBIG=$(cat big_oid) &&\n>  \tSMALL=$(cat small_oid) &&\n\n"}]}