* bug report: "git pack-redundant --all" crash in minimize() @ 2020-12-15 17:34 Daniel C. Klauer 2020-12-15 18:21 ` Jeff King 0 siblings, 1 reply; 11+ messages in thread From: Daniel C. Klauer @ 2020-12-15 17:34 UTC (permalink / raw) To: git Hello, I'm getting the following crash from "git pack-redundant --all" (output from valgrind): ==14070== Invalid read of size 8 ==14070== at 0x18F165: minimize (pack-redundant.c:399) ==14070== by 0x18F165: cmd_pack_redundant (pack-redundant.c:622) ==14070== by 0x1242D3: run_builtin (git.c:444) ==14070== by 0x1242D3: handle_builtin (git.c:674) ==14070== by 0x125393: run_argv (git.c:741) ==14070== by 0x125393: cmd_main (git.c:872) ==14070== by 0x123E7D: main (common-main.c:52) ==14070== Address 0x10 is not stack'd, malloc'd or (recently) free'd Commands to reproduce: mkdir new cd new git init touch foo.txt git add foo.txt git commit -m "first commit" git gc git pack-redundant --all The same happens with Ubuntu's git 2.25.1 as well as manually compiled git 2.30.0.rc0 from current master. The reason this showed up is that Yocto's build tool, bitbake, sometimes invokes this git command on some of the repositories it downloaded. This issue was apparently also reported here: https://bugzilla.redhat.com/show_bug.cgi?id=1803506 Kind Regards, Daniel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: bug report: "git pack-redundant --all" crash in minimize() 2020-12-15 17:34 bug report: "git pack-redundant --all" crash in minimize() Daniel C. Klauer @ 2020-12-15 18:21 ` Jeff King 2020-12-15 22:32 ` Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Jeff King @ 2020-12-15 18:21 UTC (permalink / raw) To: Daniel C. Klauer; +Cc: Jiang Xin, git On Tue, Dec 15, 2020 at 06:34:53PM +0100, Daniel C. Klauer wrote: > I'm getting the following crash from "git pack-redundant --all" (output > from valgrind): > > ==14070== Invalid read of size 8 > ==14070== at 0x18F165: minimize (pack-redundant.c:399) > ==14070== by 0x18F165: cmd_pack_redundant (pack-redundant.c:622) > ==14070== by 0x1242D3: run_builtin (git.c:444) > ==14070== by 0x1242D3: handle_builtin (git.c:674) > ==14070== by 0x125393: run_argv (git.c:741) > ==14070== by 0x125393: cmd_main (git.c:872) > ==14070== by 0x123E7D: main (common-main.c:52) > ==14070== Address 0x10 is not stack'd, malloc'd or (recently) free'd > > Commands to reproduce: > > mkdir new > cd new > git init > touch foo.txt > git add foo.txt > git commit -m "first commit" > git gc > git pack-redundant --all Thanks for an easy reproduction. This bisects to 3011177640 (pack-redundant: delay creation of unique_objects, 2019-02-02). I suspect the fix is just: diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c index 3e70f2a4c1..68afcfeb7b 100644 --- a/builtin/pack-redundant.c +++ b/builtin/pack-redundant.c @@ -396,7 +396,7 @@ static void minimize(struct pack_list **min) pl = local_packs; while (pl) { - if (pl->unique_objects->size) + if (pl->unique_objects && pl->unique_objects->size) pack_list_insert(&unique, pl); else pack_list_insert(&non_unique, pl); but I didn't look closely (author cc'd). > The reason this showed up is that Yocto's build tool, bitbake, sometimes > invokes this git command on some of the repositories it downloaded. This > issue was apparently also reported here: > https://bugzilla.redhat.com/show_bug.cgi?id=1803506 We had a discussion not too long ago[1] about whether we should deprecate and remove pack-redundant, as it's not generally useful. I'm curious if you have any more context on why it's used in that tool. -Peff [1] https://lore.kernel.org/git/20200825172214.GC1414394@coredump.intra.peff.net/ ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: bug report: "git pack-redundant --all" crash in minimize() 2020-12-15 18:21 ` Jeff King @ 2020-12-15 22:32 ` Junio C Hamano 2020-12-16 9:21 ` Jiang Xin 2020-12-16 13:22 ` bug report: "git pack-redundant --all" crash in minimize() Daniel Klauer 2 siblings, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2020-12-15 22:32 UTC (permalink / raw) To: Jeff King; +Cc: Daniel C. Klauer, Jiang Xin, git Jeff King <peff@peff.net> writes: > We had a discussion not too long ago[1] about whether we should > deprecate and remove pack-redundant, as it's not generally useful. I'm > curious if you have any more context on why it's used in that tool. > > -Peff > > [1] https://lore.kernel.org/git/20200825172214.GC1414394@coredump.intra.peff.net/ Hmm, it seems that we didn't follow through. I do want to learn from the answer you just asked above, but in the meantime, here is to save future work of digging the archive for me ;-) Thanks. -- >8 -- Subject: [PATCH] pack-redundant: gauge the usage before proposing its removal The subcommand is unusably slow and the reason why nobody reports it as a performance bug is suspected to be the absense of users. Let's show a big message that asks the user to tell us that they still care about the command when an attempt is made to run the command, with an escape hatch to override it with a command line option. In a few releases, we may turn it into an error and keep it for a few more releases before finally removing it (during the whole time, the plan to remove it would be interrupted by end user raising hand). Reviewed-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/pack-redundant.c | 13 +++++++++++++ t/t5323-pack-redundant.sh | 28 +++++++++++++++------------- 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c index 3e70f2a4c1..d3b5ee8364 100644 --- a/builtin/pack-redundant.c +++ b/builtin/pack-redundant.c @@ -554,6 +554,7 @@ static void load_all(void) int cmd_pack_redundant(int argc, const char **argv, const char *prefix) { int i; + int i_still_use_this = 0; struct pack_list *min = NULL, *red, *pl; struct llist *ignore; struct object_id *oid; @@ -580,12 +581,24 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix) alt_odb = 1; continue; } + if (!strcmp(arg, "--i-still-use-this")) { + i_still_use_this = 1; + continue; + } if (*arg == '-') usage(pack_redundant_usage); else break; } + if (!i_still_use_this) { + fputs(_("'git pack-redundant' is nominated for removal.\n" + "If you still use this command, please add an extra\n" + "option, '--i-still-use-this', on the command line\n" + "and let us know you still use it by sending an e-mail\n" + "to <git@vger.kernel.org>. Thanks.\n"), stderr); + } + if (load_all_packs) load_all(); else diff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh index 6b4d1ca353..2dd2d67b9e 100755 --- a/t/t5323-pack-redundant.sh +++ b/t/t5323-pack-redundant.sh @@ -39,6 +39,8 @@ relationship between packs and objects is as follows: master_repo=master.git shared_repo=shared.git +git_pack_redundant='git pack-redundant --i-still-use-this' + # Create commits in <repo> and assign each commit's oid to shell variables # given in the arguments (A, B, and C). E.g.: # @@ -154,7 +156,7 @@ test_expect_success 'master: no redundant for pack 1, 2, 3' ' EOF ( cd "$master_repo" && - git pack-redundant --all >out && + $git_pack_redundant --all >out && test_must_be_empty out ) ' @@ -192,7 +194,7 @@ test_expect_success 'master: one of pack-2/pack-3 is redundant' ' cat >expect <<-EOF && P3:$P3 EOF - git pack-redundant --all >out && + $git_pack_redundant --all >out && format_packfiles <out >actual && test_cmp expect actual ) @@ -231,7 +233,7 @@ test_expect_success 'master: pack 2, 4, and 6 are redundant' ' P4:$P4 P6:$P6 EOF - git pack-redundant --all >out && + $git_pack_redundant --all >out && format_packfiles <out >actual && test_cmp expect actual ) @@ -266,7 +268,7 @@ test_expect_success 'master: pack-8 (subset of pack-1) is also redundant' ' P6:$P6 P8:$P8 EOF - git pack-redundant --all >out && + $git_pack_redundant --all >out && format_packfiles <out >actual && test_cmp expect actual ) @@ -284,9 +286,9 @@ test_expect_success 'master: clean loose objects' ' test_expect_success 'master: remove redundant packs and pass fsck' ' ( cd "$master_repo" && - git pack-redundant --all | xargs rm && + $git_pack_redundant --all | xargs rm && git fsck && - git pack-redundant --all >out && + $git_pack_redundant --all >out && test_must_be_empty out ) ' @@ -304,7 +306,7 @@ test_expect_success 'setup shared.git' ' test_expect_success 'shared: all packs are redundant, but no output without --alt-odb' ' ( cd "$shared_repo" && - git pack-redundant --all >out && + $git_pack_redundant --all >out && test_must_be_empty out ) ' @@ -343,7 +345,7 @@ test_expect_success 'shared: show redundant packs in stderr for verbose mode' ' P5:$P5 P7:$P7 EOF - git pack-redundant --all --verbose >out 2>out.err && + $git_pack_redundant --all --verbose >out 2>out.err && test_must_be_empty out && grep "pack$" out.err | format_packfiles >actual && test_cmp expect actual @@ -356,9 +358,9 @@ test_expect_success 'shared: remove redundant packs, no packs left' ' cat >expect <<-EOF && fatal: Zero packs found! EOF - git pack-redundant --all --alt-odb | xargs rm && + $git_pack_redundant --all --alt-odb | xargs rm && git fsck && - test_must_fail git pack-redundant --all --alt-odb >actual 2>&1 && + test_must_fail $git_pack_redundant --all --alt-odb >actual 2>&1 && test_cmp expect actual ) ' @@ -386,7 +388,7 @@ test_expect_success 'shared: create new objects and packs' ' test_expect_success 'shared: no redundant without --alt-odb' ' ( cd "$shared_repo" && - git pack-redundant --all >out && + $git_pack_redundant --all >out && test_must_be_empty out ) ' @@ -417,7 +419,7 @@ test_expect_success 'shared: no redundant without --alt-odb' ' test_expect_success 'shared: one pack is redundant with --alt-odb' ' ( cd "$shared_repo" && - git pack-redundant --all --alt-odb >out && + $git_pack_redundant --all --alt-odb >out && format_packfiles <out >actual && test_line_count = 1 actual ) @@ -454,7 +456,7 @@ test_expect_success 'shared: ignore unique objects and all two packs are redunda Px1:$Px1 Px2:$Px2 EOF - git pack-redundant --all --alt-odb >out <<-EOF && + $git_pack_redundant --all --alt-odb >out <<-EOF && $X $Y $Z -- 2.30.0-rc0-186-g20447144ec ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: bug report: "git pack-redundant --all" crash in minimize() 2020-12-15 18:21 ` Jeff King 2020-12-15 22:32 ` Junio C Hamano @ 2020-12-16 9:21 ` Jiang Xin 2020-12-16 10:09 ` [PATCH] pack-redundant: fix crash when one packfile in repo Jiang Xin 2020-12-16 13:22 ` bug report: "git pack-redundant --all" crash in minimize() Daniel Klauer 2 siblings, 1 reply; 11+ messages in thread From: Jiang Xin @ 2020-12-16 9:21 UTC (permalink / raw) To: Jeff King; +Cc: Daniel C. Klauer, Jiang Xin, Git List Jeff King <peff@peff.net> 于2020年12月16日周三 上午2:24写道: > > On Tue, Dec 15, 2020 at 06:34:53PM +0100, Daniel C. Klauer wrote: > > > I'm getting the following crash from "git pack-redundant --all" (output > > from valgrind): > > > > ==14070== Invalid read of size 8 > > ==14070== at 0x18F165: minimize (pack-redundant.c:399) > > ==14070== by 0x18F165: cmd_pack_redundant (pack-redundant.c:622) > > ==14070== by 0x1242D3: run_builtin (git.c:444) > > ==14070== by 0x1242D3: handle_builtin (git.c:674) > > ==14070== by 0x125393: run_argv (git.c:741) > > ==14070== by 0x125393: cmd_main (git.c:872) > > ==14070== by 0x123E7D: main (common-main.c:52) > > ==14070== Address 0x10 is not stack'd, malloc'd or (recently) free'd > > > > Commands to reproduce: > > > > mkdir new > > cd new > > git init > > touch foo.txt > > git add foo.txt > > git commit -m "first commit" > > git gc > > git pack-redundant --all > > Thanks for an easy reproduction. This bisects to 3011177640 > (pack-redundant: delay creation of unique_objects, 2019-02-02). > > I suspect the fix is just: > > diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c > index 3e70f2a4c1..68afcfeb7b 100644 > --- a/builtin/pack-redundant.c > +++ b/builtin/pack-redundant.c > @@ -396,7 +396,7 @@ static void minimize(struct pack_list **min) > > pl = local_packs; > while (pl) { > - if (pl->unique_objects->size) > + if (pl->unique_objects && pl->unique_objects->size) > pack_list_insert(&unique, pl); > else > pack_list_insert(&non_unique, pl); > > but I didn't look closely (author cc'd). > Will fix like this: pack-redundant: fix crash when one packfile in repo Command `git pack-redundant --all` will crash if there is only one packfile in the repository. This is because, if there is only one packfile in local_packs, `cmp_local_packs` will do nothing and will leave `pl->unique_objects` as uninitialized. ... ... diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c index 178e3409b7..9b0646a5e2 100644 --- a/builtin/pack-redundant.c +++ b/builtin/pack-redundant.c @@ -473,6 +473,12 @@ static void cmp_local_packs(void) { struct pack_list *subset, *pl = local_packs; + /* only one packfile */ + if (!pl->next && !pl->unique_objects) { + llist_init(&pl->unique_objects); + return; + } + while ((subset = pl)) { while ((subset = subset->next)) cmp_two_packs(pl, subset); ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH] pack-redundant: fix crash when one packfile in repo 2020-12-16 9:21 ` Jiang Xin @ 2020-12-16 10:09 ` Jiang Xin 2020-12-16 17:32 ` Junio C Hamano 2020-12-16 18:46 ` [PATCH] " Jeff King 0 siblings, 2 replies; 11+ messages in thread From: Jiang Xin @ 2020-12-16 10:09 UTC (permalink / raw) To: Junio C Hamano, Git List, Jeff King, Daniel C . Klauer; +Cc: Jiang Xin From: Jiang Xin <zhiyou.jx@alibaba-inc.com> Command `git pack-redundant --all` will crash if there is only one packfile in the repository. This is because, if there is only one packfile in local_packs, `cmp_local_packs` will do nothing and will leave `pl->unique_objects` as uninitialized. Junio will add additional option for pack-redundant for preparing its removal. So replace `git pack-redundant` command with variable `$git_pack_redundant` in t5323. Reported-by: Daniel C. Klauer <daniel.c.klauer@web.de> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> --- builtin/pack-redundant.c | 6 ++++ t/t5323-pack-redundant.sh | 65 +++++++++++++++++++++++++++++---------- 2 files changed, 54 insertions(+), 17 deletions(-) diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c index 178e3409b7..690775fa82 100644 --- a/builtin/pack-redundant.c +++ b/builtin/pack-redundant.c @@ -473,6 +473,12 @@ static void cmp_local_packs(void) { struct pack_list *subset, *pl = local_packs; + /* only one packfile */ + if (!pl->next) { + llist_init(&pl->unique_objects); + return; + } + while ((subset = pl)) { while ((subset = subset->next)) cmp_two_packs(pl, subset); diff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh index 6b4d1ca353..45436bd6e1 100755 --- a/t/t5323-pack-redundant.sh +++ b/t/t5323-pack-redundant.sh @@ -39,6 +39,8 @@ relationship between packs and objects is as follows: master_repo=master.git shared_repo=shared.git +git_pack_redundant='git pack-redundant' + # Create commits in <repo> and assign each commit's oid to shell variables # given in the arguments (A, B, and C). E.g.: # @@ -112,19 +114,28 @@ test_expect_success 'setup master repo' ' create_commits_in "$master_repo" A B C D E F G H I J K L M N O P Q R ' +test_expect_success 'master: pack-redundant works with no packfile' ' + ( + cd "$master_repo" && + cat >expect <<-EOF && + fatal: Zero packs found! + EOF + test_must_fail $git_pack_redundant --all >actual 2>&1 && + test_cmp expect actual + ) +' + ############################################################################# # Chart of packs and objects for this test case # # | T A B C D E F G H I J K L M N O P Q R # ----+-------------------------------------- # P1 | x x x x x x x x -# P2 | x x x x x x x -# P3 | x x x x x x # ----+-------------------------------------- -# ALL | x x x x x x x x x x x x x x x +# ALL | x x x x x x x x # ############################################################################# -test_expect_success 'master: no redundant for pack 1, 2, 3' ' +test_expect_success 'master: pack-redundant works with one packfile' ' create_pack_in "$master_repo" P1 <<-EOF && $T $A @@ -135,6 +146,26 @@ test_expect_success 'master: no redundant for pack 1, 2, 3' ' $F $R EOF + ( + cd "$master_repo" && + $git_pack_redundant --all >out && + test_must_be_empty out + ) +' + +############################################################################# +# Chart of packs and objects for this test case +# +# | T A B C D E F G H I J K L M N O P Q R +# ----+-------------------------------------- +# P1 | x x x x x x x x +# P2 | x x x x x x x +# P3 | x x x x x x +# ----+-------------------------------------- +# ALL | x x x x x x x x x x x x x x x +# +############################################################################# +test_expect_success 'master: no redundant for pack 1, 2, 3' ' create_pack_in "$master_repo" P2 <<-EOF && $B $C @@ -154,7 +185,7 @@ test_expect_success 'master: no redundant for pack 1, 2, 3' ' EOF ( cd "$master_repo" && - git pack-redundant --all >out && + $git_pack_redundant --all >out && test_must_be_empty out ) ' @@ -192,7 +223,7 @@ test_expect_success 'master: one of pack-2/pack-3 is redundant' ' cat >expect <<-EOF && P3:$P3 EOF - git pack-redundant --all >out && + $git_pack_redundant --all >out && format_packfiles <out >actual && test_cmp expect actual ) @@ -231,7 +262,7 @@ test_expect_success 'master: pack 2, 4, and 6 are redundant' ' P4:$P4 P6:$P6 EOF - git pack-redundant --all >out && + $git_pack_redundant --all >out && format_packfiles <out >actual && test_cmp expect actual ) @@ -266,7 +297,7 @@ test_expect_success 'master: pack-8 (subset of pack-1) is also redundant' ' P6:$P6 P8:$P8 EOF - git pack-redundant --all >out && + $git_pack_redundant --all >out && format_packfiles <out >actual && test_cmp expect actual ) @@ -284,9 +315,9 @@ test_expect_success 'master: clean loose objects' ' test_expect_success 'master: remove redundant packs and pass fsck' ' ( cd "$master_repo" && - git pack-redundant --all | xargs rm && + $git_pack_redundant --all | xargs rm && git fsck && - git pack-redundant --all >out && + $git_pack_redundant --all >out && test_must_be_empty out ) ' @@ -304,7 +335,7 @@ test_expect_success 'setup shared.git' ' test_expect_success 'shared: all packs are redundant, but no output without --alt-odb' ' ( cd "$shared_repo" && - git pack-redundant --all >out && + $git_pack_redundant --all >out && test_must_be_empty out ) ' @@ -343,7 +374,7 @@ test_expect_success 'shared: show redundant packs in stderr for verbose mode' ' P5:$P5 P7:$P7 EOF - git pack-redundant --all --verbose >out 2>out.err && + $git_pack_redundant --all --verbose >out 2>out.err && test_must_be_empty out && grep "pack$" out.err | format_packfiles >actual && test_cmp expect actual @@ -356,9 +387,9 @@ test_expect_success 'shared: remove redundant packs, no packs left' ' cat >expect <<-EOF && fatal: Zero packs found! EOF - git pack-redundant --all --alt-odb | xargs rm && + $git_pack_redundant --all --alt-odb | xargs rm && git fsck && - test_must_fail git pack-redundant --all --alt-odb >actual 2>&1 && + test_must_fail $git_pack_redundant --all --alt-odb >actual 2>&1 && test_cmp expect actual ) ' @@ -386,7 +417,7 @@ test_expect_success 'shared: create new objects and packs' ' test_expect_success 'shared: no redundant without --alt-odb' ' ( cd "$shared_repo" && - git pack-redundant --all >out && + $git_pack_redundant --all >out && test_must_be_empty out ) ' @@ -417,7 +448,7 @@ test_expect_success 'shared: no redundant without --alt-odb' ' test_expect_success 'shared: one pack is redundant with --alt-odb' ' ( cd "$shared_repo" && - git pack-redundant --all --alt-odb >out && + $git_pack_redundant --all --alt-odb >out && format_packfiles <out >actual && test_line_count = 1 actual ) @@ -454,7 +485,7 @@ test_expect_success 'shared: ignore unique objects and all two packs are redunda Px1:$Px1 Px2:$Px2 EOF - git pack-redundant --all --alt-odb >out <<-EOF && + $git_pack_redundant --all --alt-odb >out <<-EOF && $X $Y $Z -- 2.30.0.rc0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] pack-redundant: fix crash when one packfile in repo 2020-12-16 10:09 ` [PATCH] pack-redundant: fix crash when one packfile in repo Jiang Xin @ 2020-12-16 17:32 ` Junio C Hamano 2020-12-17 1:57 ` [PATCH v2] " Jiang Xin 2020-12-16 18:46 ` [PATCH] " Jeff King 1 sibling, 1 reply; 11+ messages in thread From: Junio C Hamano @ 2020-12-16 17:32 UTC (permalink / raw) To: Jiang Xin; +Cc: Git List, Jeff King, Daniel C . Klauer, Jiang Xin Jiang Xin <worldhello.net@gmail.com> writes: > Junio will add additional option for pack-redundant for preparing > its removal. So replace `git pack-redundant` command with variable > `$git_pack_redundant` in t5323. Could you please revert the part about $git_pack_redundant, pretending as if the other patch never existed, so that this fix can be applied without having to wait until the discussion to deprecate and remove comes to conclusion? Thanks. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] pack-redundant: fix crash when one packfile in repo 2020-12-16 17:32 ` Junio C Hamano @ 2020-12-17 1:57 ` Jiang Xin 2020-12-17 6:08 ` Junio C Hamano 0 siblings, 1 reply; 11+ messages in thread From: Jiang Xin @ 2020-12-17 1:57 UTC (permalink / raw) To: Junio C Hamano, Git List, Jeff King, Daniel C . Klauer; +Cc: Jiang Xin From: Jiang Xin <zhiyou.jx@alibaba-inc.com> Command `git pack-redundant --all` will crash if there is only one packfile in the repository. This is because, if there is only one packfile in local_packs, `cmp_local_packs` will do nothing and will leave `pl->unique_objects` as uninitialized. Also add testcases for repository with no packfile and one packfile in t5323. Reported-by: Daniel C. Klauer <daniel.c.klauer@web.de> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> --- builtin/pack-redundant.c | 6 ++++++ t/t5323-pack-redundant.sh | 37 +++++++++++++++++++++++++++++++++---- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c index 178e3409b7..690775fa82 100644 --- a/builtin/pack-redundant.c +++ b/builtin/pack-redundant.c @@ -473,6 +473,12 @@ static void cmp_local_packs(void) { struct pack_list *subset, *pl = local_packs; + /* only one packfile */ + if (!pl->next) { + llist_init(&pl->unique_objects); + return; + } + while ((subset = pl)) { while ((subset = subset->next)) cmp_two_packs(pl, subset); diff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh index 6b4d1ca353..7e3340843f 100755 --- a/t/t5323-pack-redundant.sh +++ b/t/t5323-pack-redundant.sh @@ -112,19 +112,28 @@ test_expect_success 'setup master repo' ' create_commits_in "$master_repo" A B C D E F G H I J K L M N O P Q R ' +test_expect_success 'master: pack-redundant works with no packfile' ' + ( + cd "$master_repo" && + cat >expect <<-EOF && + fatal: Zero packs found! + EOF + test_must_fail git pack-redundant --all >actual 2>&1 && + test_cmp expect actual + ) +' + ############################################################################# # Chart of packs and objects for this test case # # | T A B C D E F G H I J K L M N O P Q R # ----+-------------------------------------- # P1 | x x x x x x x x -# P2 | x x x x x x x -# P3 | x x x x x x # ----+-------------------------------------- -# ALL | x x x x x x x x x x x x x x x +# ALL | x x x x x x x x # ############################################################################# -test_expect_success 'master: no redundant for pack 1, 2, 3' ' +test_expect_success 'master: pack-redundant works with one packfile' ' create_pack_in "$master_repo" P1 <<-EOF && $T $A @@ -135,6 +144,26 @@ test_expect_success 'master: no redundant for pack 1, 2, 3' ' $F $R EOF + ( + cd "$master_repo" && + git pack-redundant --all >out && + test_must_be_empty out + ) +' + +############################################################################# +# Chart of packs and objects for this test case +# +# | T A B C D E F G H I J K L M N O P Q R +# ----+-------------------------------------- +# P1 | x x x x x x x x +# P2 | x x x x x x x +# P3 | x x x x x x +# ----+-------------------------------------- +# ALL | x x x x x x x x x x x x x x x +# +############################################################################# +test_expect_success 'master: no redundant for pack 1, 2, 3' ' create_pack_in "$master_repo" P2 <<-EOF && $B $C -- 2.30.0.rc0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] pack-redundant: fix crash when one packfile in repo 2020-12-17 1:57 ` [PATCH v2] " Jiang Xin @ 2020-12-17 6:08 ` Junio C Hamano 0 siblings, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2020-12-17 6:08 UTC (permalink / raw) To: Jiang Xin; +Cc: Git List, Jeff King, Daniel C . Klauer, Jiang Xin Jiang Xin <worldhello.net@gmail.com> writes: > From: Jiang Xin <zhiyou.jx@alibaba-inc.com> > > Command `git pack-redundant --all` will crash if there is only one > packfile in the repository. This is because, if there is only one > packfile in local_packs, `cmp_local_packs` will do nothing and will > leave `pl->unique_objects` as uninitialized. > > Also add testcases for repository with no packfile and one packfile > in t5323. > > Reported-by: Daniel C. Klauer <daniel.c.klauer@web.de> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> > --- > builtin/pack-redundant.c | 6 ++++++ > t/t5323-pack-redundant.sh | 37 +++++++++++++++++++++++++++++++++---- > 2 files changed, 39 insertions(+), 4 deletions(-) Thanks. > > diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c > index 178e3409b7..690775fa82 100644 > --- a/builtin/pack-redundant.c > +++ b/builtin/pack-redundant.c > @@ -473,6 +473,12 @@ static void cmp_local_packs(void) > { > struct pack_list *subset, *pl = local_packs; > > + /* only one packfile */ > + if (!pl->next) { > + llist_init(&pl->unique_objects); > + return; > + } > + > while ((subset = pl)) { > while ((subset = subset->next)) > cmp_two_packs(pl, subset); > diff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh > index 6b4d1ca353..7e3340843f 100755 > --- a/t/t5323-pack-redundant.sh > +++ b/t/t5323-pack-redundant.sh > @@ -112,19 +112,28 @@ test_expect_success 'setup master repo' ' > create_commits_in "$master_repo" A B C D E F G H I J K L M N O P Q R > ' > > +test_expect_success 'master: pack-redundant works with no packfile' ' > + ( > + cd "$master_repo" && > + cat >expect <<-EOF && > + fatal: Zero packs found! > + EOF > + test_must_fail git pack-redundant --all >actual 2>&1 && > + test_cmp expect actual > + ) > +' > + > ############################################################################# > # Chart of packs and objects for this test case > # > # | T A B C D E F G H I J K L M N O P Q R > # ----+-------------------------------------- > # P1 | x x x x x x x x > -# P2 | x x x x x x x > -# P3 | x x x x x x > # ----+-------------------------------------- > -# ALL | x x x x x x x x x x x x x x x > +# ALL | x x x x x x x x > # > ############################################################################# > -test_expect_success 'master: no redundant for pack 1, 2, 3' ' > +test_expect_success 'master: pack-redundant works with one packfile' ' > create_pack_in "$master_repo" P1 <<-EOF && > $T > $A > @@ -135,6 +144,26 @@ test_expect_success 'master: no redundant for pack 1, 2, 3' ' > $F > $R > EOF > + ( > + cd "$master_repo" && > + git pack-redundant --all >out && > + test_must_be_empty out > + ) > +' > + > +############################################################################# > +# Chart of packs and objects for this test case > +# > +# | T A B C D E F G H I J K L M N O P Q R > +# ----+-------------------------------------- > +# P1 | x x x x x x x x > +# P2 | x x x x x x x > +# P3 | x x x x x x > +# ----+-------------------------------------- > +# ALL | x x x x x x x x x x x x x x x > +# > +############################################################################# > +test_expect_success 'master: no redundant for pack 1, 2, 3' ' > create_pack_in "$master_repo" P2 <<-EOF && > $B > $C ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] pack-redundant: fix crash when one packfile in repo 2020-12-16 10:09 ` [PATCH] pack-redundant: fix crash when one packfile in repo Jiang Xin 2020-12-16 17:32 ` Junio C Hamano @ 2020-12-16 18:46 ` Jeff King 1 sibling, 0 replies; 11+ messages in thread From: Jeff King @ 2020-12-16 18:46 UTC (permalink / raw) To: Jiang Xin; +Cc: Junio C Hamano, Git List, Daniel C . Klauer, Jiang Xin On Wed, Dec 16, 2020 at 05:09:52AM -0500, Jiang Xin wrote: > Command `git pack-redundant --all` will crash if there is only one > packfile in the repository. This is because, if there is only one > packfile in local_packs, `cmp_local_packs` will do nothing and will > leave `pl->unique_objects` as uninitialized. Thanks, that makes sense. And I'm glad I waited to get the opinion of somebody much more clueful. Mine was a band-aid, but yours is hitting the root cause and will help any other downstream code that assumes unique_objects isn't NULL. > builtin/pack-redundant.c | 6 ++++ > t/t5323-pack-redundant.sh | 65 +++++++++++++++++++++++++++++---------- The new test coverage makes sense, I think, though as Junio noted, it should be done independent of the $git_pack_redundant change (which will also make the diff a lot easier to review). -Peff ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: bug report: "git pack-redundant --all" crash in minimize() 2020-12-15 18:21 ` Jeff King 2020-12-15 22:32 ` Junio C Hamano 2020-12-16 9:21 ` Jiang Xin @ 2020-12-16 13:22 ` Daniel Klauer 2020-12-16 19:00 ` Jeff King 2 siblings, 1 reply; 11+ messages in thread From: Daniel Klauer @ 2020-12-16 13:22 UTC (permalink / raw) To: Jeff King; +Cc: Jiang Xin, git On 15.12.20 19:21, Jeff King wrote: > On Tue, Dec 15, 2020 at 06:34:53PM +0100, Daniel C. Klauer wrote: >> The reason this showed up is that Yocto's build tool, bitbake, sometimes >> invokes this git command on some of the repositories it downloaded. This >> issue was apparently also reported here: >> https://bugzilla.redhat.com/show_bug.cgi?id=1803506 > > We had a discussion not too long ago[1] about whether we should > deprecate and remove pack-redundant, as it's not generally useful. I'm > curious if you have any more context on why it's used in that tool. Background: bitbake downloads git repositories during a build process and supports caching them locally (in form of bare repos in some user-defined directory). This prevents having to re-download them during the next build, and also it is a convenient mirroring/backup system in case the original URLs stop working. As far as I can tell (since I'm not a bitbake developer) the git pack-redundant invocation is one of multiple calls meant to improve storage (probably minimize disk usage) of the locally cached git repos. For reference, please take a look at the other git commands it's invoking [1], and at the commit messages of the commits that added these invocations [2] [3] [4]. If doing it that way seems wrong, I'll report the issue to bitbake upstream too. Maybe there is a better way to do whatever bitbake wants to do here? Thanks! [1] http://git.openembedded.org/bitbake/tree/lib/bb/fetch2/git.py?h=1.48.1#n364 [2] http://git.openembedded.org/bitbake/commit?id=d5d958744fff66cf5286022d78cbec1839fca2e2 [3] http://git.openembedded.org/bitbake/commit?id=f8126aaf774186a6eaf0bd4067b89c074594886c [4] http://git.openembedded.org/bitbake/commit?id=7d55491f88ff90a4c16ad87b1483d55b1824acfc ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: bug report: "git pack-redundant --all" crash in minimize() 2020-12-16 13:22 ` bug report: "git pack-redundant --all" crash in minimize() Daniel Klauer @ 2020-12-16 19:00 ` Jeff King 0 siblings, 0 replies; 11+ messages in thread From: Jeff King @ 2020-12-16 19:00 UTC (permalink / raw) To: Daniel Klauer; +Cc: Jiang Xin, git On Wed, Dec 16, 2020 at 02:22:52PM +0100, Daniel Klauer wrote: > Background: bitbake downloads git repositories during a build process > and supports caching them locally (in form of bare repos in some > user-defined directory). This prevents having to re-download them during > the next build, and also it is a convenient mirroring/backup system in > case the original URLs stop working. > > As far as I can tell (since I'm not a bitbake developer) the git > pack-redundant invocation is one of multiple calls meant to improve > storage (probably minimize disk usage) of the locally cached git repos. > For reference, please take a look at the other git commands it's > invoking [1], and at the commit messages of the commits that added these > invocations [2] [3] [4]. > > If doing it that way seems wrong, I'll report the issue to bitbake > upstream too. Maybe there is a better way to do whatever bitbake wants > to do here? Thanks for that context. I don't think it's _wrong_, in the sense that what they want to do (remove redundant packs) is a reasonable thing to want. But in practice I suspect that it rarely helps. It only makes sense if a pack is fully made redundant by other packs. But that is unlikely to happen after a fetch, because Git tries not to send objects that already exist. So while there could be overlap, it's unlikely that full packs are candidates for deletion. And if any are, then that is probably a sign that fetch is not being given enough information (e.g., if there are packs being copied into the repo behind the scenes, make sure that there are matching refs pointing to their objects, so Git knows it has that part of the object graph). For saving space, "git repack -ad" is a much better option. It puts everything reachable into a single pack, which means: - if two packs contain duplicates of an object, we'll end up with only a single copy, even if those packs also contained some unique objects - by putting all objects in the same pack, we have more opportunities for delta compression between similar objects - we'll drop any unreachable objects completely (presumably this is desirable here, but if they're trying to keep objects that don't have refs pointing at them as part of some caching scheme, they might not; passing "-k" will keep the unreachable objects, too) Since they're doing other maintenance like "pack-refs", then running "git gc" may be preferable, as it would cover that, too. Use "--prune=now" to drop the unreachable objects immediately (as opposed to giving them a 2-week grace period). Note that there's no equivalent to repack's "-k" from git-gc", so if they need that, they'll have to invoke git-repack directly. -Peff ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-12-17 6:09 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-15 17:34 bug report: "git pack-redundant --all" crash in minimize() Daniel C. Klauer 2020-12-15 18:21 ` Jeff King 2020-12-15 22:32 ` Junio C Hamano 2020-12-16 9:21 ` Jiang Xin 2020-12-16 10:09 ` [PATCH] pack-redundant: fix crash when one packfile in repo Jiang Xin 2020-12-16 17:32 ` Junio C Hamano 2020-12-17 1:57 ` [PATCH v2] " Jiang Xin 2020-12-17 6:08 ` Junio C Hamano 2020-12-16 18:46 ` [PATCH] " Jeff King 2020-12-16 13:22 ` bug report: "git pack-redundant --all" crash in minimize() Daniel Klauer 2020-12-16 19:00 ` Jeff King
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).