bug report: "git pack-redundant --all" crash in minimize()

bug report: "git pack-redundant --all" crash in minimize()

[Daniel C. Klauer]

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

Re: bug report: "git pack-redundant --all" crash in minimize()

[Jeff King]

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/

Re: bug report: "git pack-redundant --all" crash in minimize()

[Junio C Hamano]

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

Re: bug report: "git pack-redundant --all" crash in minimize()

[Jiang Xin]

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

[PATCH] pack-redundant: fix crash when one packfile in repo

[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

Re: bug report: "git pack-redundant --all" crash in minimize()

[Daniel Klauer]

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

Re: [PATCH] pack-redundant: fix crash when one packfile in repo

[Junio C Hamano]

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.

Re: [PATCH] pack-redundant: fix crash when one packfile in repo

[Jeff King]

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

Re: bug report: "git pack-redundant --all" crash in minimize()

[Jeff King]

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

[PATCH v2] pack-redundant: fix crash when one packfile in repo

[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

Re: [PATCH v2] pack-redundant: fix crash when one packfile in repo

[Junio C Hamano]

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