{"thread":{"id":"54830","subject":"bug report: \"git pack-redundant --all\" crash in minimize()","startedAt":"2020-12-15T17:45:05Z","lastAt":"2020-12-17T06:08:58Z","messageCount":11,"participants":["Daniel C. Klauer","Jeff King","Junio C Hamano","Jiang Xin","Daniel Klauer"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"412271","messageId":"dc410ef1-fc05-162e-ed4e-c843e0bc285d@web.de","threadId":"54830","inReplyTo":null,"subject":"bug report: \"git pack-redundant --all\" crash in minimize()","fromName":"Daniel C. Klauer","fromEmail":"daniel.c.klauer@web.de","sentAt":"2020-12-15T17:34:53Z","receivedAt":"2020-12-15T17:45:05Z","isPatch":false,"sender":{"key":"daniel.c.klauer@web.de","avatar":null},"body":"Hello,\n\nI'm getting the following crash from \"git pack-redundant --all\" (output\nfrom valgrind):\n\n==14070== Invalid read of size 8\n==14070==    at 0x18F165: minimize (pack-redundant.c:399)\n==14070==    by 0x18F165: cmd_pack_redundant (pack-redundant.c:622)\n==14070==    by 0x1242D3: run_builtin (git.c:444)\n==14070==    by 0x1242D3: handle_builtin (git.c:674)\n==14070==    by 0x125393: run_argv (git.c:741)\n==14070==    by 0x125393: cmd_main (git.c:872)\n==14070==    by 0x123E7D: main (common-main.c:52)\n==14070==  Address 0x10 is not stack'd, malloc'd or (recently) free'd\n\nCommands to reproduce:\n\nmkdir new\ncd new\ngit init\ntouch foo.txt\ngit add foo.txt\ngit commit -m \"first commit\"\ngit gc\ngit pack-redundant --all\n\nThe same happens with Ubuntu's git 2.25.1 as well as manually compiled\ngit 2.30.0.rc0 from current master.\n\nThe reason this showed up is that Yocto's build tool, bitbake, sometimes\ninvokes this git command on some of the repositories it downloaded. This\nissue was apparently also reported here:\nhttps://bugzilla.redhat.com/show_bug.cgi?id=1803506\n\nKind Regards,\nDaniel\n"},{"id":"412277","messageId":"X9j+nuzjiYyw/vQx@coredump.intra.peff.net","threadId":"54830","inReplyTo":"dc410ef1-fc05-162e-ed4e-c843e0bc285d@web.de","subject":"Re: bug report: \"git pack-redundant --all\" crash in minimize()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-12-15T18:21:18Z","receivedAt":"2020-12-15T18:22:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 15, 2020 at 06:34:53PM +0100, Daniel C. Klauer wrote:\n\n> I'm getting the following crash from \"git pack-redundant --all\" (output\n> from valgrind):\n> \n> ==14070== Invalid read of size 8\n> ==14070==    at 0x18F165: minimize (pack-redundant.c:399)\n> ==14070==    by 0x18F165: cmd_pack_redundant (pack-redundant.c:622)\n> ==14070==    by 0x1242D3: run_builtin (git.c:444)\n> ==14070==    by 0x1242D3: handle_builtin (git.c:674)\n> ==14070==    by 0x125393: run_argv (git.c:741)\n> ==14070==    by 0x125393: cmd_main (git.c:872)\n> ==14070==    by 0x123E7D: main (common-main.c:52)\n> ==14070==  Address 0x10 is not stack'd, malloc'd or (recently) free'd\n> \n> Commands to reproduce:\n> \n> mkdir new\n> cd new\n> git init\n> touch foo.txt\n> git add foo.txt\n> git commit -m \"first commit\"\n> git gc\n> git pack-redundant --all\n\nThanks for an easy reproduction. This bisects to 3011177640\n(pack-redundant: delay creation of unique_objects, 2019-02-02).\n\nI suspect the fix is just:\n\ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex 3e70f2a4c1..68afcfeb7b 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -396,7 +396,7 @@ static void minimize(struct pack_list **min)\n \n \tpl = local_packs;\n \twhile (pl) {\n-\t\tif (pl->unique_objects->size)\n+\t\tif (pl->unique_objects && pl->unique_objects->size)\n \t\t\tpack_list_insert(&unique, pl);\n \t\telse\n \t\t\tpack_list_insert(&non_unique, pl);\n\nbut I didn't look closely (author cc'd).\n\n> The reason this showed up is that Yocto's build tool, bitbake, sometimes\n> invokes this git command on some of the repositories it downloaded. This\n> issue was apparently also reported here:\n> https://bugzilla.redhat.com/show_bug.cgi?id=1803506\n\nWe had a discussion not too long ago[1] about whether we should\ndeprecate and remove pack-redundant, as it's not generally useful. I'm\ncurious if you have any more context on why it's used in that tool.\n\n-Peff\n\n[1] https://lore.kernel.org/git/20200825172214.GC1414394@coredump.intra.peff.net/\n"},{"id":"412306","messageId":"xmqq8s9y7la5.fsf@gitster.c.googlers.com","threadId":"54830","inReplyTo":"X9j+nuzjiYyw/vQx@coredump.intra.peff.net","subject":"Re: bug report: \"git pack-redundant --all\" crash in minimize()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-15T22:32:34Z","receivedAt":"2020-12-15T22:34:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> We had a discussion not too long ago[1] about whether we should\n> deprecate and remove pack-redundant, as it's not generally useful. I'm\n> curious if you have any more context on why it's used in that tool.\n>\n> -Peff\n>\n> [1] https://lore.kernel.org/git/20200825172214.GC1414394@coredump.intra.peff.net/\n\nHmm, it seems that we didn't follow through.\n\nI do want to learn from the answer you just asked above, but in the\nmeantime, here is to save future work of digging the archive for me ;-)\n\nThanks.\n\n-- >8 --\nSubject: [PATCH] pack-redundant: gauge the usage before proposing its removal\n\nThe subcommand is unusably slow and the reason why nobody reports it\nas a performance bug is suspected to be the absense of users.  Let's\nshow a big message that asks the user to tell us that they still\ncare about the command when an attempt is made to run the command,\nwith an escape hatch to override it with a command line option.\n\nIn a few releases, we may turn it into an error and keep it for a\nfew more releases before finally removing it (during the whole time,\nthe plan to remove it would be interrupted by end user raising hand).\n\nReviewed-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/pack-redundant.c  | 13 +++++++++++++\n t/t5323-pack-redundant.sh | 28 +++++++++++++++-------------\n 2 files changed, 28 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex 3e70f2a4c1..d3b5ee8364 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -554,6 +554,7 @@ static void load_all(void)\n int cmd_pack_redundant(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n+\tint i_still_use_this = 0;\n \tstruct pack_list *min = NULL, *red, *pl;\n \tstruct llist *ignore;\n \tstruct object_id *oid;\n@@ -580,12 +581,24 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)\n \t\t\talt_odb = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"--i-still-use-this\")) {\n+\t\t\ti_still_use_this = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (*arg == '-')\n \t\t\tusage(pack_redundant_usage);\n \t\telse\n \t\t\tbreak;\n \t}\n \n+\tif (!i_still_use_this) {\n+\t\tfputs(_(\"'git pack-redundant' is nominated for removal.\\n\"\n+\t\t\t\"If you still use this command, please add an extra\\n\"\n+\t\t\t\"option, '--i-still-use-this', on the command line\\n\"\n+\t\t\t\"and let us know you still use it by sending an e-mail\\n\"\n+\t\t\t\"to <git@vger.kernel.org>.  Thanks.\\n\"), stderr);\n+\t}\n+\n \tif (load_all_packs)\n \t\tload_all();\n \telse\ndiff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh\nindex 6b4d1ca353..2dd2d67b9e 100755\n--- a/t/t5323-pack-redundant.sh\n+++ b/t/t5323-pack-redundant.sh\n@@ -39,6 +39,8 @@ relationship between packs and objects is as follows:\n master_repo=master.git\n shared_repo=shared.git\n \n+git_pack_redundant='git pack-redundant --i-still-use-this'\n+\n # Create commits in <repo> and assign each commit's oid to shell variables\n # given in the arguments (A, B, and C). E.g.:\n #\n@@ -154,7 +156,7 @@ test_expect_success 'master: no redundant for pack 1, 2, 3' '\n \t\tEOF\n \t(\n \t\tcd \"$master_repo\" &&\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\ttest_must_be_empty out\n \t)\n '\n@@ -192,7 +194,7 @@ test_expect_success 'master: one of pack-2/pack-3 is redundant' '\n \t\tcat >expect <<-EOF &&\n \t\t\tP3:$P3\n \t\t\tEOF\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\tformat_packfiles <out >actual &&\n \t\ttest_cmp expect actual\n \t)\n@@ -231,7 +233,7 @@ test_expect_success 'master: pack 2, 4, and 6 are redundant' '\n \t\t\tP4:$P4\n \t\t\tP6:$P6\n \t\t\tEOF\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\tformat_packfiles <out >actual &&\n \t\ttest_cmp expect actual\n \t)\n@@ -266,7 +268,7 @@ test_expect_success 'master: pack-8 (subset of pack-1) is also redundant' '\n \t\t\tP6:$P6\n \t\t\tP8:$P8\n \t\t\tEOF\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\tformat_packfiles <out >actual &&\n \t\ttest_cmp expect actual\n \t)\n@@ -284,9 +286,9 @@ test_expect_success 'master: clean loose objects' '\n test_expect_success 'master: remove redundant packs and pass fsck' '\n \t(\n \t\tcd \"$master_repo\" &&\n-\t\tgit pack-redundant --all | xargs rm &&\n+\t\t$git_pack_redundant --all | xargs rm &&\n \t\tgit fsck &&\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\ttest_must_be_empty out\n \t)\n '\n@@ -304,7 +306,7 @@ test_expect_success 'setup shared.git' '\n test_expect_success 'shared: all packs are redundant, but no output without --alt-odb' '\n \t(\n \t\tcd \"$shared_repo\" &&\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\ttest_must_be_empty out\n \t)\n '\n@@ -343,7 +345,7 @@ test_expect_success 'shared: show redundant packs in stderr for verbose mode' '\n \t\t\tP5:$P5\n \t\t\tP7:$P7\n \t\t\tEOF\n-\t\tgit pack-redundant --all --verbose >out 2>out.err &&\n+\t\t$git_pack_redundant --all --verbose >out 2>out.err &&\n \t\ttest_must_be_empty out &&\n \t\tgrep \"pack$\" out.err | format_packfiles >actual &&\n \t\ttest_cmp expect actual\n@@ -356,9 +358,9 @@ test_expect_success 'shared: remove redundant packs, no packs left' '\n \t\tcat >expect <<-EOF &&\n \t\t\tfatal: Zero packs found!\n \t\t\tEOF\n-\t\tgit pack-redundant --all --alt-odb | xargs rm &&\n+\t\t$git_pack_redundant --all --alt-odb | xargs rm &&\n \t\tgit fsck &&\n-\t\ttest_must_fail git pack-redundant --all --alt-odb >actual 2>&1 &&\n+\t\ttest_must_fail $git_pack_redundant --all --alt-odb >actual 2>&1 &&\n \t\ttest_cmp expect actual\n \t)\n '\n@@ -386,7 +388,7 @@ test_expect_success 'shared: create new objects and packs' '\n test_expect_success 'shared: no redundant without --alt-odb' '\n \t(\n \t\tcd \"$shared_repo\" &&\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\ttest_must_be_empty out\n \t)\n '\n@@ -417,7 +419,7 @@ test_expect_success 'shared: no redundant without --alt-odb' '\n test_expect_success 'shared: one pack is redundant with --alt-odb' '\n \t(\n \t\tcd \"$shared_repo\" &&\n-\t\tgit pack-redundant --all --alt-odb >out &&\n+\t\t$git_pack_redundant --all --alt-odb >out &&\n \t\tformat_packfiles <out >actual &&\n \t\ttest_line_count = 1 actual\n \t)\n@@ -454,7 +456,7 @@ test_expect_success 'shared: ignore unique objects and all two packs are redunda\n \t\t\tPx1:$Px1\n \t\t\tPx2:$Px2\n \t\t\tEOF\n-\t\tgit pack-redundant --all --alt-odb >out <<-EOF &&\n+\t\t$git_pack_redundant --all --alt-odb >out <<-EOF &&\n \t\t\t$X\n \t\t\t$Y\n \t\t\t$Z\n-- \n2.30.0-rc0-186-g20447144ec\n\n"},{"id":"412351","messageId":"CANYiYbHOczhRoJkOofFNQ6VA3BiyOF=QjffFBDvLn43Ts8B67w@mail.gmail.com","threadId":"54830","inReplyTo":"X9j+nuzjiYyw/vQx@coredump.intra.peff.net","subject":"Re: bug report: \"git pack-redundant --all\" crash in minimize()","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2020-12-16T09:21:46Z","receivedAt":"2020-12-16T09:22:39Z","isPatch":false,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"Jeff King <peff@peff.net> 于2020年12月16日周三 上午2:24写道：\n>\n> On Tue, Dec 15, 2020 at 06:34:53PM +0100, Daniel C. Klauer wrote:\n>\n> > I'm getting the following crash from \"git pack-redundant --all\" (output\n> > from valgrind):\n> >\n> > ==14070== Invalid read of size 8\n> > ==14070==    at 0x18F165: minimize (pack-redundant.c:399)\n> > ==14070==    by 0x18F165: cmd_pack_redundant (pack-redundant.c:622)\n> > ==14070==    by 0x1242D3: run_builtin (git.c:444)\n> > ==14070==    by 0x1242D3: handle_builtin (git.c:674)\n> > ==14070==    by 0x125393: run_argv (git.c:741)\n> > ==14070==    by 0x125393: cmd_main (git.c:872)\n> > ==14070==    by 0x123E7D: main (common-main.c:52)\n> > ==14070==  Address 0x10 is not stack'd, malloc'd or (recently) free'd\n> >\n> > Commands to reproduce:\n> >\n> > mkdir new\n> > cd new\n> > git init\n> > touch foo.txt\n> > git add foo.txt\n> > git commit -m \"first commit\"\n> > git gc\n> > git pack-redundant --all\n>\n> Thanks for an easy reproduction. This bisects to 3011177640\n> (pack-redundant: delay creation of unique_objects, 2019-02-02).\n>\n> I suspect the fix is just:\n>\n> diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\n> index 3e70f2a4c1..68afcfeb7b 100644\n> --- a/builtin/pack-redundant.c\n> +++ b/builtin/pack-redundant.c\n> @@ -396,7 +396,7 @@ static void minimize(struct pack_list **min)\n>\n>         pl = local_packs;\n>         while (pl) {\n> -               if (pl->unique_objects->size)\n> +               if (pl->unique_objects && pl->unique_objects->size)\n>                         pack_list_insert(&unique, pl);\n>                 else\n>                         pack_list_insert(&non_unique, pl);\n>\n> but I didn't look closely (author cc'd).\n>\n\nWill fix like this:\n\n    pack-redundant: fix crash when one packfile in repo\n\n    Command `git pack-redundant --all` will crash if there is only one\n    packfile in the repository.  This is because, if there is only one\n    packfile in local_packs, `cmp_local_packs` will do nothing and will\n    leave `pl->unique_objects` as uninitialized.\n\n    ... ...\n\ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex 178e3409b7..9b0646a5e2 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -473,6 +473,12 @@ static void cmp_local_packs(void)\n {\n        struct pack_list *subset, *pl = local_packs;\n\n+       /* only one packfile */\n+       if (!pl->next && !pl->unique_objects) {\n+               llist_init(&pl->unique_objects);\n+               return;\n+       }\n+\n        while ((subset = pl)) {\n                while ((subset = subset->next))\n                        cmp_two_packs(pl, subset);\n"},{"id":"412352","messageId":"20201216100952.16890-1-worldhello.net@gmail.com","threadId":"54830","inReplyTo":"CANYiYbHOczhRoJkOofFNQ6VA3BiyOF=QjffFBDvLn43Ts8B67w@mail.gmail.com","subject":"[PATCH] pack-redundant: fix crash when one packfile in repo","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2020-12-16T10:09:52Z","receivedAt":"2020-12-16T10:10:53Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n\nCommand `git pack-redundant --all` will crash if there is only one\npackfile in the repository.  This is because, if there is only one\npackfile in local_packs, `cmp_local_packs` will do nothing and will\nleave `pl->unique_objects` as uninitialized.\n\nJunio will add additional option for pack-redundant for preparing\nits removal.  So replace `git pack-redundant` command with variable\n`$git_pack_redundant` in t5323.\n\nReported-by: Daniel C. Klauer <daniel.c.klauer@web.de>\nSigned-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n---\n builtin/pack-redundant.c  |  6 ++++\n t/t5323-pack-redundant.sh | 65 +++++++++++++++++++++++++++++----------\n 2 files changed, 54 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex 178e3409b7..690775fa82 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -473,6 +473,12 @@ static void cmp_local_packs(void)\n {\n \tstruct pack_list *subset, *pl = local_packs;\n \n+\t/* only one packfile */\n+\tif (!pl->next) {\n+\t\tllist_init(&pl->unique_objects);\n+\t\treturn;\n+\t}\n+\n \twhile ((subset = pl)) {\n \t\twhile ((subset = subset->next))\n \t\t\tcmp_two_packs(pl, subset);\ndiff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh\nindex 6b4d1ca353..45436bd6e1 100755\n--- a/t/t5323-pack-redundant.sh\n+++ b/t/t5323-pack-redundant.sh\n@@ -39,6 +39,8 @@ relationship between packs and objects is as follows:\n master_repo=master.git\n shared_repo=shared.git\n \n+git_pack_redundant='git pack-redundant'\n+\n # Create commits in <repo> and assign each commit's oid to shell variables\n # given in the arguments (A, B, and C). E.g.:\n #\n@@ -112,19 +114,28 @@ test_expect_success 'setup master repo' '\n \tcreate_commits_in \"$master_repo\" A B C D E F G H I J K L M N O P Q R\n '\n \n+test_expect_success 'master: pack-redundant works with no packfile' '\n+\t(\n+\t\tcd \"$master_repo\" &&\n+\t\tcat >expect <<-EOF &&\n+\t\t\tfatal: Zero packs found!\n+\t\t\tEOF\n+\t\ttest_must_fail $git_pack_redundant --all >actual 2>&1 &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n #############################################################################\n # Chart of packs and objects for this test case\n #\n #         | T A B C D E F G H I J K L M N O P Q R\n #     ----+--------------------------------------\n #     P1  | x x x x x x x                       x\n-#     P2  |     x x x x   x x x\n-#     P3  |             x     x x x x x\n #     ----+--------------------------------------\n-#     ALL | x x x x x x x x x x x x x x         x\n+#     ALL | x x x x x x x                       x\n #\n #############################################################################\n-test_expect_success 'master: no redundant for pack 1, 2, 3' '\n+test_expect_success 'master: pack-redundant works with one packfile' '\n \tcreate_pack_in \"$master_repo\" P1 <<-EOF &&\n \t\t$T\n \t\t$A\n@@ -135,6 +146,26 @@ test_expect_success 'master: no redundant for pack 1, 2, 3' '\n \t\t$F\n \t\t$R\n \t\tEOF\n+\t(\n+\t\tcd \"$master_repo\" &&\n+\t\t$git_pack_redundant --all >out &&\n+\t\ttest_must_be_empty out\n+\t)\n+'\n+\n+#############################################################################\n+# Chart of packs and objects for this test case\n+#\n+#         | T A B C D E F G H I J K L M N O P Q R\n+#     ----+--------------------------------------\n+#     P1  | x x x x x x x                       x\n+#     P2  |     x x x x   x x x\n+#     P3  |             x     x x x x x\n+#     ----+--------------------------------------\n+#     ALL | x x x x x x x x x x x x x x         x\n+#\n+#############################################################################\n+test_expect_success 'master: no redundant for pack 1, 2, 3' '\n \tcreate_pack_in \"$master_repo\" P2 <<-EOF &&\n \t\t$B\n \t\t$C\n@@ -154,7 +185,7 @@ test_expect_success 'master: no redundant for pack 1, 2, 3' '\n \t\tEOF\n \t(\n \t\tcd \"$master_repo\" &&\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\ttest_must_be_empty out\n \t)\n '\n@@ -192,7 +223,7 @@ test_expect_success 'master: one of pack-2/pack-3 is redundant' '\n \t\tcat >expect <<-EOF &&\n \t\t\tP3:$P3\n \t\t\tEOF\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\tformat_packfiles <out >actual &&\n \t\ttest_cmp expect actual\n \t)\n@@ -231,7 +262,7 @@ test_expect_success 'master: pack 2, 4, and 6 are redundant' '\n \t\t\tP4:$P4\n \t\t\tP6:$P6\n \t\t\tEOF\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\tformat_packfiles <out >actual &&\n \t\ttest_cmp expect actual\n \t)\n@@ -266,7 +297,7 @@ test_expect_success 'master: pack-8 (subset of pack-1) is also redundant' '\n \t\t\tP6:$P6\n \t\t\tP8:$P8\n \t\t\tEOF\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\tformat_packfiles <out >actual &&\n \t\ttest_cmp expect actual\n \t)\n@@ -284,9 +315,9 @@ test_expect_success 'master: clean loose objects' '\n test_expect_success 'master: remove redundant packs and pass fsck' '\n \t(\n \t\tcd \"$master_repo\" &&\n-\t\tgit pack-redundant --all | xargs rm &&\n+\t\t$git_pack_redundant --all | xargs rm &&\n \t\tgit fsck &&\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\ttest_must_be_empty out\n \t)\n '\n@@ -304,7 +335,7 @@ test_expect_success 'setup shared.git' '\n test_expect_success 'shared: all packs are redundant, but no output without --alt-odb' '\n \t(\n \t\tcd \"$shared_repo\" &&\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\ttest_must_be_empty out\n \t)\n '\n@@ -343,7 +374,7 @@ test_expect_success 'shared: show redundant packs in stderr for verbose mode' '\n \t\t\tP5:$P5\n \t\t\tP7:$P7\n \t\t\tEOF\n-\t\tgit pack-redundant --all --verbose >out 2>out.err &&\n+\t\t$git_pack_redundant --all --verbose >out 2>out.err &&\n \t\ttest_must_be_empty out &&\n \t\tgrep \"pack$\" out.err | format_packfiles >actual &&\n \t\ttest_cmp expect actual\n@@ -356,9 +387,9 @@ test_expect_success 'shared: remove redundant packs, no packs left' '\n \t\tcat >expect <<-EOF &&\n \t\t\tfatal: Zero packs found!\n \t\t\tEOF\n-\t\tgit pack-redundant --all --alt-odb | xargs rm &&\n+\t\t$git_pack_redundant --all --alt-odb | xargs rm &&\n \t\tgit fsck &&\n-\t\ttest_must_fail git pack-redundant --all --alt-odb >actual 2>&1 &&\n+\t\ttest_must_fail $git_pack_redundant --all --alt-odb >actual 2>&1 &&\n \t\ttest_cmp expect actual\n \t)\n '\n@@ -386,7 +417,7 @@ test_expect_success 'shared: create new objects and packs' '\n test_expect_success 'shared: no redundant without --alt-odb' '\n \t(\n \t\tcd \"$shared_repo\" &&\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\ttest_must_be_empty out\n \t)\n '\n@@ -417,7 +448,7 @@ test_expect_success 'shared: no redundant without --alt-odb' '\n test_expect_success 'shared: one pack is redundant with --alt-odb' '\n \t(\n \t\tcd \"$shared_repo\" &&\n-\t\tgit pack-redundant --all --alt-odb >out &&\n+\t\t$git_pack_redundant --all --alt-odb >out &&\n \t\tformat_packfiles <out >actual &&\n \t\ttest_line_count = 1 actual\n \t)\n@@ -454,7 +485,7 @@ test_expect_success 'shared: ignore unique objects and all two packs are redunda\n \t\t\tPx1:$Px1\n \t\t\tPx2:$Px2\n \t\t\tEOF\n-\t\tgit pack-redundant --all --alt-odb >out <<-EOF &&\n+\t\t$git_pack_redundant --all --alt-odb >out <<-EOF &&\n \t\t\t$X\n \t\t\t$Y\n \t\t\t$Z\n-- \n2.30.0.rc0\n\n"},{"id":"412355","messageId":"a46660c3-630c-5573-9ef4-75d273d37767@web.de","threadId":"54830","inReplyTo":"X9j+nuzjiYyw/vQx@coredump.intra.peff.net","subject":"Re: bug report: \"git pack-redundant --all\" crash in minimize()","fromName":"Daniel Klauer","fromEmail":"daniel.c.klauer@web.de","sentAt":"2020-12-16T13:22:52Z","receivedAt":"2020-12-16T13:33:09Z","isPatch":false,"sender":{"key":"daniel.c.klauer@web.de","avatar":null},"body":"On 15.12.20 19:21, Jeff King wrote:\n> On Tue, Dec 15, 2020 at 06:34:53PM +0100, Daniel C. Klauer wrote:\n>> The reason this showed up is that Yocto's build tool, bitbake, sometimes\n>> invokes this git command on some of the repositories it downloaded. This\n>> issue was apparently also reported here:\n>> https://bugzilla.redhat.com/show_bug.cgi?id=1803506\n>\n> We had a discussion not too long ago[1] about whether we should\n> deprecate and remove pack-redundant, as it's not generally useful. I'm\n> curious if you have any more context on why it's used in that tool.\n\nBackground: bitbake downloads git repositories during a build process\nand supports caching them locally (in form of bare repos in some\nuser-defined directory). This prevents having to re-download them during\nthe next build, and also it is a convenient mirroring/backup system in\ncase the original URLs stop working.\n\nAs far as I can tell (since I'm not a bitbake developer) the git\npack-redundant invocation is one of multiple calls meant to improve\nstorage (probably minimize disk usage) of the locally cached git repos.\nFor reference, please take a look at the other git commands it's\ninvoking [1], and at the commit messages of the commits that added these\ninvocations [2] [3] [4].\n\nIf doing it that way seems wrong, I'll report the issue to bitbake\nupstream too. Maybe there is a better way to do whatever bitbake wants\nto do here?\n\nThanks!\n\n[1]\nhttp://git.openembedded.org/bitbake/tree/lib/bb/fetch2/git.py?h=1.48.1#n364\n[2]\nhttp://git.openembedded.org/bitbake/commit?id=d5d958744fff66cf5286022d78cbec1839fca2e2\n[3]\nhttp://git.openembedded.org/bitbake/commit?id=f8126aaf774186a6eaf0bd4067b89c074594886c\n[4]\nhttp://git.openembedded.org/bitbake/commit?id=7d55491f88ff90a4c16ad87b1483d55b1824acfc\n"},{"id":"412376","messageId":"xmqqo8it4pxt.fsf@gitster.c.googlers.com","threadId":"54830","inReplyTo":"20201216100952.16890-1-worldhello.net@gmail.com","subject":"Re: [PATCH] pack-redundant: fix crash when one packfile in repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-16T17:32:30Z","receivedAt":"2020-12-16T17:33:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin <worldhello.net@gmail.com> writes:\n\n> Junio will add additional option for pack-redundant for preparing\n> its removal.  So replace `git pack-redundant` command with variable\n> `$git_pack_redundant` in t5323.\n\nCould you please revert the part about $git_pack_redundant,\npretending as if the other patch never existed, so that this fix can\nbe applied without having to wait until the discussion to deprecate\nand remove comes to conclusion?\n\nThanks.\n"},{"id":"412383","messageId":"X9pWDRAn7GmJgt7Z@coredump.intra.peff.net","threadId":"54830","inReplyTo":"20201216100952.16890-1-worldhello.net@gmail.com","subject":"Re: [PATCH] pack-redundant: fix crash when one packfile in repo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-12-16T18:46:37Z","receivedAt":"2020-12-16T18:47:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 16, 2020 at 05:09:52AM -0500, Jiang Xin wrote:\n\n> Command `git pack-redundant --all` will crash if there is only one\n> packfile in the repository.  This is because, if there is only one\n> packfile in local_packs, `cmp_local_packs` will do nothing and will\n> leave `pl->unique_objects` as uninitialized.\n\nThanks, that makes sense. And I'm glad I waited to get the opinion of\nsomebody much more clueful. Mine was a band-aid, but yours is hitting\nthe root cause and will help any other downstream code that assumes\nunique_objects isn't NULL.\n\n>  builtin/pack-redundant.c  |  6 ++++\n>  t/t5323-pack-redundant.sh | 65 +++++++++++++++++++++++++++++----------\n\nThe new test coverage makes sense, I think, though as Junio noted, it\nshould be done independent of the $git_pack_redundant change (which will\nalso make the diff a lot easier to review).\n\n-Peff\n"},{"id":"412420","messageId":"X9pZUELTD9i5TrvQ@coredump.intra.peff.net","threadId":"54830","inReplyTo":"a46660c3-630c-5573-9ef4-75d273d37767@web.de","subject":"Re: bug report: \"git pack-redundant --all\" crash in minimize()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-12-16T19:00:32Z","receivedAt":"2020-12-16T19:01:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 16, 2020 at 02:22:52PM +0100, Daniel Klauer wrote:\n\n> Background: bitbake downloads git repositories during a build process\n> and supports caching them locally (in form of bare repos in some\n> user-defined directory). This prevents having to re-download them during\n> the next build, and also it is a convenient mirroring/backup system in\n> case the original URLs stop working.\n> \n> As far as I can tell (since I'm not a bitbake developer) the git\n> pack-redundant invocation is one of multiple calls meant to improve\n> storage (probably minimize disk usage) of the locally cached git repos.\n> For reference, please take a look at the other git commands it's\n> invoking [1], and at the commit messages of the commits that added these\n> invocations [2] [3] [4].\n> \n> If doing it that way seems wrong, I'll report the issue to bitbake\n> upstream too. Maybe there is a better way to do whatever bitbake wants\n> to do here?\n\nThanks for that context.\n\nI don't think it's _wrong_, in the sense that what they want to do\n(remove redundant packs) is a reasonable thing to want. But in practice\nI suspect that it rarely helps. It only makes sense if a pack is fully\nmade redundant by other packs. But that is unlikely to happen after a\nfetch, because Git tries not to send objects that already exist. So\nwhile there could be overlap, it's unlikely that full packs are\ncandidates for deletion. And if any are, then that is probably a sign\nthat fetch is not being given enough information (e.g., if there are\npacks being copied into the repo behind the scenes, make sure that there\nare matching refs pointing to their objects, so Git knows it has that\npart of the object graph).\n\nFor saving space, \"git repack -ad\" is a much better option. It puts\neverything reachable into a single pack, which means:\n\n  - if two packs contain duplicates of an object, we'll end up with only\n    a single copy, even if those packs also contained some unique\n    objects\n\n  - by putting all objects in the same pack, we have more opportunities\n    for delta compression between similar objects\n\n  - we'll drop any unreachable objects completely (presumably this is\n    desirable here, but if they're trying to keep objects that don't\n    have refs pointing at them as part of some caching scheme, they\n    might not; passing \"-k\" will keep the unreachable objects, too)\n\nSince they're doing other maintenance like \"pack-refs\", then running\n\"git gc\" may be preferable, as it would cover that, too. Use\n\"--prune=now\" to drop the unreachable objects immediately (as opposed to\ngiving them a 2-week grace period). Note that there's no equivalent to\nrepack's \"-k\" from git-gc\", so if they need that, they'll have to invoke\ngit-repack directly.\n\n-Peff\n"},{"id":"412472","messageId":"20201217015709.28827-1-worldhello.net@gmail.com","threadId":"54830","inReplyTo":"xmqqo8it4pxt.fsf@gitster.c.googlers.com","subject":"[PATCH v2] pack-redundant: fix crash when one packfile in repo","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2020-12-17T01:57:09Z","receivedAt":"2020-12-17T01:58:09Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"From: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n\nCommand `git pack-redundant --all` will crash if there is only one\npackfile in the repository.  This is because, if there is only one\npackfile in local_packs, `cmp_local_packs` will do nothing and will\nleave `pl->unique_objects` as uninitialized.\n\nAlso add testcases for repository with no packfile and one packfile\nin t5323.\n\nReported-by: Daniel C. Klauer <daniel.c.klauer@web.de>\nSigned-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n---\n builtin/pack-redundant.c  |  6 ++++++\n t/t5323-pack-redundant.sh | 37 +++++++++++++++++++++++++++++++++----\n 2 files changed, 39 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex 178e3409b7..690775fa82 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -473,6 +473,12 @@ static void cmp_local_packs(void)\n {\n \tstruct pack_list *subset, *pl = local_packs;\n \n+\t/* only one packfile */\n+\tif (!pl->next) {\n+\t\tllist_init(&pl->unique_objects);\n+\t\treturn;\n+\t}\n+\n \twhile ((subset = pl)) {\n \t\twhile ((subset = subset->next))\n \t\t\tcmp_two_packs(pl, subset);\ndiff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh\nindex 6b4d1ca353..7e3340843f 100755\n--- a/t/t5323-pack-redundant.sh\n+++ b/t/t5323-pack-redundant.sh\n@@ -112,19 +112,28 @@ test_expect_success 'setup master repo' '\n \tcreate_commits_in \"$master_repo\" A B C D E F G H I J K L M N O P Q R\n '\n \n+test_expect_success 'master: pack-redundant works with no packfile' '\n+\t(\n+\t\tcd \"$master_repo\" &&\n+\t\tcat >expect <<-EOF &&\n+\t\t\tfatal: Zero packs found!\n+\t\t\tEOF\n+\t\ttest_must_fail git pack-redundant --all >actual 2>&1 &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n #############################################################################\n # Chart of packs and objects for this test case\n #\n #         | T A B C D E F G H I J K L M N O P Q R\n #     ----+--------------------------------------\n #     P1  | x x x x x x x                       x\n-#     P2  |     x x x x   x x x\n-#     P3  |             x     x x x x x\n #     ----+--------------------------------------\n-#     ALL | x x x x x x x x x x x x x x         x\n+#     ALL | x x x x x x x                       x\n #\n #############################################################################\n-test_expect_success 'master: no redundant for pack 1, 2, 3' '\n+test_expect_success 'master: pack-redundant works with one packfile' '\n \tcreate_pack_in \"$master_repo\" P1 <<-EOF &&\n \t\t$T\n \t\t$A\n@@ -135,6 +144,26 @@ test_expect_success 'master: no redundant for pack 1, 2, 3' '\n \t\t$F\n \t\t$R\n \t\tEOF\n+\t(\n+\t\tcd \"$master_repo\" &&\n+\t\tgit pack-redundant --all >out &&\n+\t\ttest_must_be_empty out\n+\t)\n+'\n+\n+#############################################################################\n+# Chart of packs and objects for this test case\n+#\n+#         | T A B C D E F G H I J K L M N O P Q R\n+#     ----+--------------------------------------\n+#     P1  | x x x x x x x                       x\n+#     P2  |     x x x x   x x x\n+#     P3  |             x     x x x x x\n+#     ----+--------------------------------------\n+#     ALL | x x x x x x x x x x x x x x         x\n+#\n+#############################################################################\n+test_expect_success 'master: no redundant for pack 1, 2, 3' '\n \tcreate_pack_in \"$master_repo\" P2 <<-EOF &&\n \t\t$B\n \t\t$C\n-- \n2.30.0.rc0\n\n"},{"id":"412479","messageId":"xmqqczz9yng4.fsf@gitster.c.googlers.com","threadId":"54830","inReplyTo":"20201217015709.28827-1-worldhello.net@gmail.com","subject":"Re: [PATCH v2] pack-redundant: fix crash when one packfile in repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-17T06:08:11Z","receivedAt":"2020-12-17T06:08:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin <worldhello.net@gmail.com> writes:\n\n> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n>\n> Command `git pack-redundant --all` will crash if there is only one\n> packfile in the repository.  This is because, if there is only one\n> packfile in local_packs, `cmp_local_packs` will do nothing and will\n> leave `pl->unique_objects` as uninitialized.\n>\n> Also add testcases for repository with no packfile and one packfile\n> in t5323.\n>\n> Reported-by: Daniel C. Klauer <daniel.c.klauer@web.de>\n> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>\n> ---\n>  builtin/pack-redundant.c  |  6 ++++++\n>  t/t5323-pack-redundant.sh | 37 +++++++++++++++++++++++++++++++++----\n>  2 files changed, 39 insertions(+), 4 deletions(-)\n\nThanks.\n\n>\n> diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\n> index 178e3409b7..690775fa82 100644\n> --- a/builtin/pack-redundant.c\n> +++ b/builtin/pack-redundant.c\n> @@ -473,6 +473,12 @@ static void cmp_local_packs(void)\n>  {\n>  \tstruct pack_list *subset, *pl = local_packs;\n>  \n> +\t/* only one packfile */\n> +\tif (!pl->next) {\n> +\t\tllist_init(&pl->unique_objects);\n> +\t\treturn;\n> +\t}\n> +\n>  \twhile ((subset = pl)) {\n>  \t\twhile ((subset = subset->next))\n>  \t\t\tcmp_two_packs(pl, subset);\n> diff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh\n> index 6b4d1ca353..7e3340843f 100755\n> --- a/t/t5323-pack-redundant.sh\n> +++ b/t/t5323-pack-redundant.sh\n> @@ -112,19 +112,28 @@ test_expect_success 'setup master repo' '\n>  \tcreate_commits_in \"$master_repo\" A B C D E F G H I J K L M N O P Q R\n>  '\n>  \n> +test_expect_success 'master: pack-redundant works with no packfile' '\n> +\t(\n> +\t\tcd \"$master_repo\" &&\n> +\t\tcat >expect <<-EOF &&\n> +\t\t\tfatal: Zero packs found!\n> +\t\t\tEOF\n> +\t\ttest_must_fail git pack-redundant --all >actual 2>&1 &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n>  #############################################################################\n>  # Chart of packs and objects for this test case\n>  #\n>  #         | T A B C D E F G H I J K L M N O P Q R\n>  #     ----+--------------------------------------\n>  #     P1  | x x x x x x x                       x\n> -#     P2  |     x x x x   x x x\n> -#     P3  |             x     x x x x x\n>  #     ----+--------------------------------------\n> -#     ALL | x x x x x x x x x x x x x x         x\n> +#     ALL | x x x x x x x                       x\n>  #\n>  #############################################################################\n> -test_expect_success 'master: no redundant for pack 1, 2, 3' '\n> +test_expect_success 'master: pack-redundant works with one packfile' '\n>  \tcreate_pack_in \"$master_repo\" P1 <<-EOF &&\n>  \t\t$T\n>  \t\t$A\n> @@ -135,6 +144,26 @@ test_expect_success 'master: no redundant for pack 1, 2, 3' '\n>  \t\t$F\n>  \t\t$R\n>  \t\tEOF\n> +\t(\n> +\t\tcd \"$master_repo\" &&\n> +\t\tgit pack-redundant --all >out &&\n> +\t\ttest_must_be_empty out\n> +\t)\n> +'\n> +\n> +#############################################################################\n> +# Chart of packs and objects for this test case\n> +#\n> +#         | T A B C D E F G H I J K L M N O P Q R\n> +#     ----+--------------------------------------\n> +#     P1  | x x x x x x x                       x\n> +#     P2  |     x x x x   x x x\n> +#     P3  |             x     x x x x x\n> +#     ----+--------------------------------------\n> +#     ALL | x x x x x x x x x x x x x x         x\n> +#\n> +#############################################################################\n> +test_expect_success 'master: no redundant for pack 1, 2, 3' '\n>  \tcreate_pack_in \"$master_repo\" P2 <<-EOF &&\n>  \t\t$B\n>  \t\t$C\n"}]}