{"thread":{"id":"59535","subject":"[PATCH] repack: fix geometric repacking with gitalternates","startedAt":"2023-04-04T11:09:33Z","lastAt":"2023-04-14T17:30:47Z","messageCount":67,"participants":["Patrick Steinhardt","Taylor Blau","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"474771","messageId":"a07ed50feeec4bfc3e9736bf493b9876896bcdd2.1680606445.git.ps@pks.im","threadId":"59535","inReplyTo":null,"subject":"[PATCH] repack: fix geometric repacking with gitalternates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-04T11:08:33Z","receivedAt":"2023-04-04T11:09:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Performing geometric repacking with repositories that have alternate\nobject directories set up is causing errors in different scenarios.\n\n- Repacking fails when the repository linked to the object\n directory does not have any objects on its own.\n\n ```\n $ git init shared\n $ git -C shared commit --allow-empty --message something\n $ git clone --shared shared member\n $ git -C member repack --geometric=2 --write-midx\n Nothing new to pack.\n warning: unknown preferred pack: 'pack-3e1a94a8dc9bb4defb0d98ce2ebf325312d76362.pack'\n error: multi-pack-index died of signal 7\n ```\n\n- Repacking fails when the repository linked to the object\n directory has the exact same packfile as the linked-to object\n directory.\n\n ```\n $ git init shared\n $ git -C shared commit --allow-empty --message something\n $ git -C shared repack -Ad\n $ cp -r shared member\n $ realpath shared/.git/objects >member/.git/objects/info/alternates\n $ git -C member repack --geometric 2 --write-midx\n fatal: could not find pack 'pack-d404037a861afe456e07a1aefb3655150f1299f0.pack'\n ```\n\nBoth issues have the same underlying root cause, which is that geometric\nrepacks don't honor whether packfiles are local or not. As a result,\nthey will try to include packs part of the alternate object directory\nand then at a later point fail to locate them as they do not exist in\nthe object directory of the repository we're about to repack.\n\nSkip over packfiles that aren't local. This will cause geometric repacks\nto never include packfiles of its alternates.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/repack.c | 6 ++++\n t/t7703-repack-geometric.sh | 59 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 65 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 87f73c8923..c6d12fa4bd 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -333,6 +333,12 @@ static void init_pack_geometry(struct pack_geometry **geometry_p,\n \tgeometry = *geometry_p;\n \n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n+\t\t/*\n+\t\t * We don't want to repack packfiles which are not part of the\n+\t\t * primary object database.\n+\t\t */\n+\t\tif (!p->pack_local)\n+\t\t\tcontinue;\n \t\tif (!pack_kept_objects) {\n \t\t\t/*\n \t\t\t * Any pack that has its pack_keep bit set will appear\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 8821fbd2dd..9f8bc663e4 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -281,4 +281,63 @@ test_expect_success '--geometric with pack.packSizeLimit' '\n \t)\n '\n \n+packed_objects() {\n+\tgit verify-pack -v \"$@\" >tmp-object-list &&\n+\tsed -n -e \"s/^\\([0-9a-f][0-9a-f]*\\).*\\(commit\\|tree\\|blob\\|tag\\).*/\\1/p\" actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success '--geometric does not include shared packfiles' '\n+\tgit init shared &&\n+\ttest_when_finished \"rm -fr shared\" &&\n+\ttest_commit -C shared \"shared\" &&\n+\tgit -C shared repack -Ad &&\n+\n+\tgit clone --shared shared member &&\n+\ttest_when_finished \"rm -fr member\" &&\n+\tgit -C member commit --allow-empty --message \"not-shared\" &&\n+\tgit -C member repack --geometric 2 --write-midx &&\n+\n+\t# We expect the created packfile to only contain the new commit.\n+\tpacked_objects member/.git/objects/pack/pack-*.idx >actual &&\n+\tgit -C member rev-parse HEAD >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--geometric with same packfile in shared repository' '\n+\tgit init shared &&\n+\ttest_when_finished \"rm -fr shared\" &&\n+\ttest_commit -C shared \"shared\" &&\n+\tgit -C shared repack -Ad &&\n+\n+\t# Prepare the member repository so that it got the exact same packfile\n+\t# as the shared repository and set up gitalternates.\n+\tcp -r shared member &&\n+\ttest_when_finished \"rm -fr member\" &&\n+\ttest-tool path-utils real_path shared/.git/objects >member/.git/objects/info/alternates &&\n+\tfind shared/.git/objects -type f >expect &&\n+\n+\t# After repacking, contents of the member repository should not have\n+\t# changed.\n+\tgit -C member repack --geometric 2 --write-midx 2>error &&\n+\tfind shared/.git/objects -type f >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"474797","messageId":"ZCxytq1esQWvjIz/@nand.local","threadId":"59535","inReplyTo":"a07ed50feeec4bfc3e9736bf493b9876896bcdd2.1680606445.git.ps@pks.im","subject":"Re: [PATCH] repack: fix geometric repacking with gitalternates","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-04T18:55:50Z","receivedAt":"2023-04-04T18:55:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 04, 2023 at 01:08:33PM +0200, Patrick Steinhardt wrote:\n> Both issues have the same underlying root cause, which is that geometric\n> repacks don't honor whether packfiles are local or not. As a result,\n> they will try to include packs part of the alternate object directory\n> and then at a later point fail to locate them as they do not exist in\n> the object directory of the repository we're about to repack.\n\nInteresting. This fix does make sense, but I wonder if it is doing the\nright thing.\n\nWhen we have an alternated repository and do 'git repack -ad' in the\n\"member\" repository, we'll end up creating a new pack containing all\nobjects in that repository, including ones from the alternate.\n\nFor example, if you do something like:\n\n rm -fr shared member\n\n git init shared\n git -C shared commit --allow-empty -m \"base\" && git -C shared repack -d\n\n git clone --shared shared member\n git -C member repack -ad\n\n for dir in shared member\n do\n echo \"==> $dir\"\n find $dir/.git/objects -type f\n done\n\nThen you'll end up with the output:\n\n ==> shared\n shared/.git/objects/pack/pack-a2f0c663b287c3eeb0207397f8cafb9ae49f8277.idx\n shared/.git/objects/pack/pack-a2f0c663b287c3eeb0207397f8cafb9ae49f8277.pack\n shared/.git/objects/info/packs\n ==> member\n member/.git/objects/pack/pack-a2f0c663b287c3eeb0207397f8cafb9ae49f8277.idx\n member/.git/objects/pack/pack-a2f0c663b287c3eeb0207397f8cafb9ae49f8277.pack\n member/.git/objects/info/alternates\n member/.git/objects/info/packs\n\nIn other words, we end up creating the pack necessary in the member\nrepository necessary to complete the repack. Since we're using `-a`\nhere, that means creating an identical pack as we have in the shared\nrepository.\n\nIf we instead did something like:\n\n git -C member repack -adl # <- `-l` is new here\n\n, our output changes to instead contain the following (empty) pack\ndirectory in the member repository:\n\n ==> member\n member/.git/objects/info/alternates\n member/.git/objects/info/packs\n\n> Skip over packfiles that aren't local. This will cause geometric repacks\n> to never include packfiles of its alternates.\n\n...So I wonder if this is the right thing to do in all cases. IOW,\nshould we be creating packs in the \"member\" repository which may be\nbased off of packs from the shared repository when `-l` is not\nspecified?\n\nI think this gets tricky. For one, the geometric repack code creates at\nmost one new pack. So if some of the packs that were above the split\nline (IOW, the ones that don't need to be squashed together) were in the\nshared repository, I'm not sure how you'd write a MIDX that covers both\nwithout using the MIDX-over-alternates feature. I have no idea how that\nworks with MIDX bitmaps (IIUC, the MIDX/alternates feature is very\nniche).\n\nI think we reasonably could do something like ignoring non-local packs\nin `init_pack_geometry()` only when `-l` is given. That still runs into\nproblems when trying to write a MIDX or MIDX bitmaps, so we should\nlikely declare the combination \"-l --write-midx --write-bitmap-index\" as\nunsupported. For backwards compatibility, I think it would make sense to\nhave \"--no-local\" be the default when `--geometric` is given (which is\nalready the case, since po_args is zero-initialized).\n\nI suspect in practice that nobody cares about what happens when you run\n\"git repack --geometric= --local\", but in case they do, I think the\nabove is probably the most reasonable behavior that I can quickly come\nup with.\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/repack.c | 6 ++++\n> t/t7703-repack-geometric.sh | 59 +++++++++++++++++++++++++++++++++++++\n> 2 files changed, 65 insertions(+)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 87f73c8923..c6d12fa4bd 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -333,6 +333,12 @@ static void init_pack_geometry(struct pack_geometry **geometry_p,\n> \tgeometry = *geometry_p;\n>\n> \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n> +\t\t/*\n> +\t\t * We don't want to repack packfiles which are not part of the\n> +\t\t * primary object database.\n> +\t\t */\n> +\t\tif (!p->pack_local)\n> +\t\t\tcontinue;\n\nSo this would change to something like `if (po_args.local &&\n!p->pack_local)`.\n\n> diff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\n> index 8821fbd2dd..9f8bc663e4 100755\n> --- a/t/t7703-repack-geometric.sh\n> +++ b/t/t7703-repack-geometric.sh\n> @@ -281,4 +281,63 @@ test_expect_success '--geometric with pack.packSizeLimit' '\n> \t)\n> '\n>\n> +packed_objects() {\n> +\tgit verify-pack -v \"$@\" >tmp-object-list &&\n> +\tsed -n -e \"s/^\\([0-9a-f][0-9a-f]*\\).*\\(commit\\|tree\\|blob\\|tag\\).*/\\1/p\" +\trm -f tmp-object-list\n> +}\n\nJust a nitpick, but I've found the output from index-pack to be a little\neasier to work with here. I think that you could replace this function\nwith the following and have it work:\n\n--- 8< ---\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 9f8bc663e4..e3ac5001bd 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -282,9 +282,8 @@ test_expect_success '--geometric with pack.packSizeLimit' '\n '\n\n packed_objects() {\n-\tgit verify-pack -v \"$@\" >tmp-object-list &&\n-\tsed -n -e \"s/^\\([0-9a-f][0-9a-f]*\\).*\\(commit\\|tree\\|blob\\|tag\\).*/\\1/p\" tmp-object-list &&\n+\tcut -d' ' -f2 tmp-object-list\n }\n\n test_expect_success '--geometric with no local objects creates no pack' '\n--- >8 ---\n\nI removed the \"&& rm -f tmp-object-list\", since we overwrite it anyway\nwhenever we call packed_objects().\n\n> +test_expect_success '--geometric with no local objects creates no pack' '\n> +\tgit init shared &&\n> +\ttest_when_finished \"rm -fr shared\" &&\n> +\ttest_commit -C shared \"shared\" &&\n> +\tgit -C shared repack -Ad &&\n> +\n> +\t# Set up the member repository so that it does not have\n> +\t# any objects on its own.\n> +\tgit clone --shared shared member &&\n> +\ttest_when_finished \"rm -fr member\" &&\n> +\n> +\tgit -C member repack --geometric 2 --write-midx &&\n> +\tfind member/.git/objects/pack -type f >actual &&\n> +\ttest_must_be_empty actual\n\nAnother small nitpick, but I think these last two lines could be\nreplaced with\n\n test_dir_is_empty member/.git/objects/pack\n\nor even\n\n test_dir_is_empty member/$packdir\n\n> +test_expect_success '--geometric does not include shared packfiles' '\n> +\tgit init shared &&\n> +\ttest_when_finished \"rm -fr shared\" &&\n> +\ttest_commit -C shared \"shared\" &&\n> +\tgit -C shared repack -Ad &&\n> +\n> +\tgit clone --shared shared member &&\n> +\ttest_when_finished \"rm -fr member\" &&\n> +\tgit -C member commit --allow-empty --message \"not-shared\" &&\n\nAny reason to make this commit empty? Could we replace this with\n\n test_commit -C member not-shared\n\n?\n\n> +test_expect_success '--geometric with same packfile in shared repository' '\n> +\tgit init shared &&\n> +\ttest_when_finished \"rm -fr shared\" &&\n> +\ttest_commit -C shared \"shared\" &&\n> +\tgit -C shared repack -Ad &&\n> +\n> +\t# Prepare the member repository so that it got the exact same packfile\n> +\t# as the shared repository and set up gitalternates.\n> +\tcp -r shared member &&\n> +\ttest_when_finished \"rm -fr member\" &&\n> +\ttest-tool path-utils real_path shared/.git/objects >member/.git/objects/info/alternates &&\n> +\tfind shared/.git/objects -type f >expect &&\n\nThis works, though it may be easier to write something like:\n\n git clone -s shared member &&\n test_when_finished \"rm -fr member\" &&\n # ensure that \"shared\" and \"member\" have an identical set of packs\n git -C member repack -a &&\n find shared/.git/objects -type f >expect &&\n\n> +\t# After repacking, contents of the member repository should not have\n> +\t# changed.\n> +\tgit -C member repack --geometric 2 --write-midx 2>error &&\n\nI think that \"2>error\" is a stray change left over from debugging, and\ncould probably be removed.\n\n> +\tfind shared/.git/objects -type f >actual &&\n\nThis and above could be replaced with shared/$packdir. Whenever I'm\nasserting the contents of a directory are unchanged, I typically like to\ncall the two files under comparison \"before\" and \"after\", but \"expect\"\nand \"actual\" are fine here, too.\n\nThanks,\nTaylor\n"},{"id":"474798","messageId":"ZCxztCEq0sZygZ25@nand.local","threadId":"59535","inReplyTo":"ZCxytq1esQWvjIz/@nand.local","subject":"Re: [PATCH] repack: fix geometric repacking with gitalternates","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-04T19:00:04Z","receivedAt":"2023-04-04T19:00:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 04, 2023 at 02:55:50PM -0400, Taylor Blau wrote:\n> I think we reasonably could do something like ignoring non-local packs\n> in `init_pack_geometry()` only when `-l` is given. That still runs into\n> problems when trying to write a MIDX or MIDX bitmaps, so we should\n> likely declare the combination \"-l --write-midx --write-bitmap-index\" as\n> unsupported. For backwards compatibility, I think it would make sense to\n> have \"--no-local\" be the default when `--geometric` is given (which is\n> already the case, since po_args is zero-initialized).\n\nWhen summarizing this message to colleagues at GitHub, I came up with\nthis TL;DR, which I figured may be useful to share:\n\n TL;DR: I think that this is a (much) more complicated problem than\n originally anticipated, but the easiest thing to do is to assume that\n git repack --geometric= means git repack --geometric= --no-local\n unless otherwise specified, and declare --geometric= --local\n unsupported when used in conjunction with --write-midx or\n --write-bitmap-index.\n\nThanks,\nTaylor\n"},{"id":"474825","messageId":"ZC0eY8q6ushpfkrQ@ncase","threadId":"59535","inReplyTo":"ZCxytq1esQWvjIz/@nand.local","subject":"Re: [PATCH] repack: fix geometric repacking with gitalternates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-05T07:08:19Z","receivedAt":"2023-04-05T07:08:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 04, 2023 at 02:55:50PM -0400, Taylor Blau wrote:\n> On Tue, Apr 04, 2023 at 01:08:33PM +0200, Patrick Steinhardt wrote:\n> > Both issues have the same underlying root cause, which is that geometric\n> > repacks don't honor whether packfiles are local or not. As a result,\n> > they will try to include packs part of the alternate object directory\n> > and then at a later point fail to locate them as they do not exist in\n> > the object directory of the repository we're about to repack.\n> \n> Interesting. This fix does make sense, but I wonder if it is doing the\n> right thing.\n> \n> When we have an alternated repository and do 'git repack -ad' in the\n> \"member\" repository, we'll end up creating a new pack containing all\n> objects in that repository, including ones from the alternate.\n> \n> For example, if you do something like:\n> \n> rm -fr shared member\n> \n> git init shared\n> git -C shared commit --allow-empty -m \"base\" && git -C shared repack -d\n> \n> git clone --shared shared member\n> git -C member repack -ad\n> \n> for dir in shared member\n> do\n> echo \"==> $dir\"\n> find $dir/.git/objects -type f\n> done\n> \n> Then you'll end up with the output:\n> \n> ==> shared\n> shared/.git/objects/pack/pack-a2f0c663b287c3eeb0207397f8cafb9ae49f8277.idx\n> shared/.git/objects/pack/pack-a2f0c663b287c3eeb0207397f8cafb9ae49f8277.pack\n> shared/.git/objects/info/packs\n> ==> member\n> member/.git/objects/pack/pack-a2f0c663b287c3eeb0207397f8cafb9ae49f8277.idx\n> member/.git/objects/pack/pack-a2f0c663b287c3eeb0207397f8cafb9ae49f8277.pack\n> member/.git/objects/info/alternates\n> member/.git/objects/info/packs\n> \n> In other words, we end up creating the pack necessary in the member\n> repository necessary to complete the repack. Since we're using `-a`\n> here, that means creating an identical pack as we have in the shared\n> repository.\n> \n> If we instead did something like:\n> \n> git -C member repack -adl # <- `-l` is new here\n> \n> , our output changes to instead contain the following (empty) pack\n> directory in the member repository:\n> \n> ==> member\n> member/.git/objects/info/alternates\n> member/.git/objects/info/packs\n\nYup, that's fair.\n\n> > Skip over packfiles that aren't local. This will cause geometric repacks\n> > to never include packfiles of its alternates.\n> \n> ...So I wonder if this is the right thing to do in all cases. IOW,\n> should we be creating packs in the \"member\" repository which may be\n> based off of packs from the shared repository when `-l` is not\n> specified?\n> \n> I think this gets tricky. For one, the geometric repack code creates at\n> most one new pack. So if some of the packs that were above the split\n> line (IOW, the ones that don't need to be squashed together) were in the\n> shared repository, I'm not sure how you'd write a MIDX that covers both\n> without using the MIDX-over-alternates feature. I have no idea how that\n> works with MIDX bitmaps (IIUC, the MIDX/alternates feature is very\n> niche).\n\nI agree, things would become tricky in case geometric repacks are\nexpected to work across alternates.\n\n> I think we reasonably could do something like ignoring non-local packs\n> in `init_pack_geometry()` only when `-l` is given. That still runs into\n> problems when trying to write a MIDX or MIDX bitmaps, so we should\n> likely declare the combination \"-l --write-midx --write-bitmap-index\" as\n> unsupported. For backwards compatibility, I think it would make sense to\n> have \"--no-local\" be the default when `--geometric` is given (which is\n> already the case, since po_args is zero-initialized).\n\nOkay, I agree that it's not all that sensible to allow writing bitmaps\nin a geometric repack that spans across multiple repositories. These\nbitmaps would immediately break once the shared repository performs a\nrepack that removes a subset of packfiles that the bitmap depends on,\nwhich would make it non-obvious for how to even do repacks in such a\nshared repository at all.\n\nBut I'm not yet sure whether I understand why `-l --write-midx` should\nbe prohibited like you summarized in the follow-up mail:\n\nOn Tue, Apr 04, 2023 at 02:55:50PM -0400, Taylor Blau wrote:\n> TL;DR: I think that this is a (much) more complicated problem than\n> originally anticipated, but the easiest thing to do is to assume that\n> git repack --geometric= means git repack --geometric= --no-local\n> unless otherwise specified, and declare --geometric= --local\n> unsupported when used in conjunction with --write-midx or\n> --write-bitmap-index.\n\nThe newly written MIDX would of course only span over the local\npackfiles, but is that even a problem? Ideally, Git would know to handle\nmultiple MIDX files, and in that case it would make sense both for the\nshared and for the member repository to have one.\n\n> I suspect in practice that nobody cares about what happens when you run\n> \"git repack --geometric= --local\", but in case they do, I think the\n> above is probably the most reasonable behavior that I can quickly come\n> up with.\n\nWell, I do as we use alternates to implement fork networks at GitLab and\nwe're in the process of adopting geometric repacking. So what I want to\nhave is that I can perform geometric repacks both in the shared and in\nthe member repository that includes only the local packfiles.\n\nAnd yes, I agree that the above is the most reasonable behaviour, with\nthe exception of disallowing MIDXs when doing a local geometric repack.\nBut that raises the question: what do we do about the currently-broken\nbehaviour when executing `git repack --geometric= --no-local` in a\nalternated repository?\n\nI'd personally be fine to start honoring the `po_args.local` flag so\nthat we skip over any non-local packfiles there while ignoring the\nlarger problem of non-local geometric repacks breaking in certain\nscenarios. It would at least improve the status quo as users now have a\nway out in case they ever happen to hit that error. And it allows us to\nuse geometric repacks in alternated repositories. But are we okay with\npunting on the larger issue for the time being?\n\nThanks for your feedback and this interesting discussion!\n\nPatrick\n"},{"id":"475063","messageId":"4c7b95e1-9d3c-e253-98ca-ac6c201babb3@github.com","threadId":"59535","inReplyTo":"ZC0eY8q6ushpfkrQ@ncase","subject":"Re: [PATCH] repack: fix geometric repacking with gitalternates","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-10T15:06:49Z","receivedAt":"2023-04-10T15:06:58Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/5/2023 3:08 AM, Patrick Steinhardt wrote:\n> On Tue, Apr 04, 2023 at 02:55:50PM -0400, Taylor Blau wrote:\n>> On Tue, Apr 04, 2023 at 01:08:33PM +0200, Patrick Steinhardt wrote:\n>>> Both issues have the same underlying root cause, which is that geometric\n>>> repacks don't honor whether packfiles are local or not. As a result,\n>>> they will try to include packs part of the alternate object directory\n>>> and then at a later point fail to locate them as they do not exist in\n>>> the object directory of the repository we're about to repack.\n>>\n>> Interesting. This fix does make sense, but I wonder if it is doing the\n>> right thing.\n>>\n>> When we have an alternated repository and do 'git repack -ad' in the\n>> \"member\" repository, we'll end up creating a new pack containing all\n>> objects in that repository, including ones from the alternate.\n...\n>> I think we reasonably could do something like ignoring non-local packs\n>> in `init_pack_geometry()` only when `-l` is given. That still runs into\n>> problems when trying to write a MIDX or MIDX bitmaps, so we should\n>> likely declare the combination \"-l --write-midx --write-bitmap-index\" as\n>> unsupported. For backwards compatibility, I think it would make sense to\n>> have \"--no-local\" be the default when `--geometric` is given (which is\n>> already the case, since po_args is zero-initialized).\n> \n> Okay, I agree that it's not all that sensible to allow writing bitmaps\n> in a geometric repack that spans across multiple repositories. These\n> bitmaps would immediately break once the shared repository performs a\n> repack that removes a subset of packfiles that the bitmap depends on,\n> which would make it non-obvious for how to even do repacks in such a\n> shared repository at all.\n\nWe have mechanisms for avoiding writing bitmaps for packs that are not\nclosed under reachability. We should have some protection against writing\nthem for multi-pack-indexes that are not closed under reachability, if\nonly as a check during bitmap_writer_build().\n\n> But I'm not yet sure whether I understand why `-l --write-midx` should\n> be prohibited like you summarized in the follow-up mail:\n> \n> On Tue, Apr 04, 2023 at 02:55:50PM -0400, Taylor Blau wrote:\n>> TL;DR: I think that this is a (much) more complicated problem than\n>> originally anticipated, but the easiest thing to do is to assume that\n>> git repack --geometric= means git repack --geometric= --no-local\n>> unless otherwise specified, and declare --geometric= --local\n>> unsupported when used in conjunction with --write-midx or\n>> --write-bitmap-index.\n> \n> The newly written MIDX would of course only span over the local\n> packfiles, but is that even a problem? Ideally, Git would know to handle\n> multiple MIDX files, and in that case it would make sense both for the\n> shared and for the member repository to have one.\n\nYes, each odb is allowed a multi-pack-index, and they chain the same way\npack-files do. I agree that this restriction isn't necessary.\n\n>> I suspect in practice that nobody cares about what happens when you run\n>> \"git repack --geometric= --local\", but in case they do, I think the\n>> above is probably the most reasonable behavior that I can quickly come\n>> up with.\n> \n> Well, I do as we use alternates to implement fork networks at GitLab and\n> we're in the process of adopting geometric repacking. So what I want to\n> have is that I can perform geometric repacks both in the shared and in\n> the member repository that includes only the local packfiles.\n> \n> And yes, I agree that the above is the most reasonable behaviour, with\n> the exception of disallowing MIDXs when doing a local geometric repack.\n\nI think the recommended\n\n\tif (po_args.local && !p->local)\n\t\tcontinue;\n\napproach is a nice _performance improvement_ for the --local case, since\nit avoids adding a list of objects to be packed (and then thrown away,\nbecause those objects exist in an alternate). Of course, we are currently\nblocked on that part working because of the non-local packs being a\nproblem.\n\n> But that raises the question: what do we do about the currently-broken\n> behaviour when executing `git repack --geometric= --no-local` in a\n> alternated repository?\n\nMuch like \"git repack -a --no-local\" in an alternated repository, I\ndon't think this is something good for users to do, but I agree that it\nnot working is a problem we should fix.\n\nYour original message includes error messages like \"could not find pack\"\nand \"unknown preferred pack\" which makes me think the _real_ problem is\nthat we are not respecting the full path name of the pack-file and are\nsomehow localizing packs to the local object dir.\n\nThe basic reason is that write_midx_included_packs() takes all of the\npack-files from the geometry, but does not strip out the pack-files\nthat are not in the same object directory.\n\nPerhaps this method could include a step to create a new, \"local\"\ngeometry containing only the packs within the local object dir. We can\nthen skip the --preferred-pack option and bitmap if this is different\nthan the original geometry (perhaps with a warning message to help\nusers who did this accidentally).\n\n> I'd personally be fine to start honoring the `po_args.local` flag so\n> that we skip over any non-local packfiles there while ignoring the\n> larger problem of non-local geometric repacks breaking in certain\n> scenarios. It would at least improve the status quo as users now have a\n> way out in case they ever happen to hit that error. And it allows us to\n> use geometric repacks in alternated repositories. But are we okay with\n> punting on the larger issue for the time being?\n\nI think the real bug is isolated in write_midx_included_packs() in how\nit may specify packs that the multi-pack-index cannot track. It should\nbe worth the time exploring if there is an easy fix there, and then the\npo_args.local version can be used as a backup/performance tweak on top\nof that.\n\nThanks,\n-Stolee\n"},{"id":"475101","messageId":"ZDSb2T8v6lvaqOyL@nand.local","threadId":"59535","inReplyTo":"ZC0eY8q6ushpfkrQ@ncase","subject":"Re: [PATCH] repack: fix geometric repacking with gitalternates","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-10T23:29:29Z","receivedAt":"2023-04-10T23:29:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 05, 2023 at 09:08:19AM +0200, Patrick Steinhardt wrote:\n> But I'm not yet sure whether I understand why `-l --write-midx` should\n> be prohibited like you summarized in the follow-up mail:\n>\n> On Tue, Apr 04, 2023 at 02:55:50PM -0400, Taylor Blau wrote:\n> > TL;DR: I think that this is a (much) more complicated problem than\n> > originally anticipated, but the easiest thing to do is to assume that\n> > git repack --geometric= means git repack --geometric= --no-local\n> > unless otherwise specified, and declare --geometric= --local\n> > unsupported when used in conjunction with --write-midx or\n> > --write-bitmap-index.\n>\n> The newly written MIDX would of course only span over the local\n> packfiles, but is that even a problem? Ideally, Git would know to handle\n> multiple MIDX files, and in that case it would make sense both for the\n> shared and for the member repository to have one.\n\nRight, I don't think that it's a problem. But I don't know how well the\nMIDX-over-alternates thing works in practice. I know that the feature\nexists, but I don't think it is as battled-tested as non-alternated\nMIDXs.\n\nSo I think you'd have to ban MIDX bitmaps when the MIDX spans over\nmultiple repositories through an alternates file, but otherwise it\nshould be OK.\n\n> But that raises the question: what do we do about the currently-broken\n> behaviour when executing `git repack --geometric= --no-local` in a\n> alternated repository?\n>\n> I'd personally be fine to start honoring the `po_args.local` flag so\n> that we skip over any non-local packfiles there while ignoring the\n> larger problem of non-local geometric repacks breaking in certain\n> scenarios. It would at least improve the status quo as users now have a\n> way out in case they ever happen to hit that error. And it allows us to\n> use geometric repacks in alternated repositories. But are we okay with\n> punting on the larger issue for the time being?\n\nI wonder if the larger issue could be simplified into (a) honoring\n`po_args.local` when doing the geometric rollup, and (b) dropping the\nnon-local packs when writing a MIDX.\n\n> Thanks for your feedback and this interesting discussion!\n\nDitto ;-).\n\nThanks,\nTaylor\n"},{"id":"475105","messageId":"ZDSgbYa+j/5c5t8j@nand.local","threadId":"59535","inReplyTo":"4c7b95e1-9d3c-e253-98ca-ac6c201babb3@github.com","subject":"Re: [PATCH] repack: fix geometric repacking with gitalternates","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-10T23:49:01Z","receivedAt":"2023-04-10T23:49:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Apr 10, 2023 at 11:06:49AM -0400, Derrick Stolee wrote:\n> Your original message includes error messages like \"could not find pack\"\n> and \"unknown preferred pack\" which makes me think the _real_ problem is\n> that we are not respecting the full path name of the pack-file and are\n> somehow localizing packs to the local object dir.\n\n\"could not find pack\" comes from pack-objects's --stdin-packs mode when\nthe caller specifies a pack that doesn't exist in the repository.\n\nI'm not sure what's going on here, since read_packs_list_from_stdin()\nsearches over the result of calling get_all_packs(), which does include\npacks in alternate object stores. And indeed, pack-objects can recognize\nsuch packs:\n\n--- 8< ---\n#!/bin/sh\n\nrm -fr shared member\n\ngit init shared\ngit -C shared commit --allow-empty -m \"base\" && git -C shared repack -d\n\ngit clone --shared shared member\n\nbasename \"$(find shared/.git/objects/pack -type f -name '*.pack')\" >input\n\ngit -C member pack-objects .git/objects/pack/pack --stdin-packs $repo\"\n\tfind $repo/.git/objects/pack -type f\ndone\n--- >8 ---\n\n^^ the above results in generating the identical pack in the \"member\"\nrepository as expected:\n\n==> shared\nshared/.git/objects/pack/pack-d51c724345703411d57134d018e9643924900d39.idx\nshared/.git/objects/pack/pack-d51c724345703411d57134d018e9643924900d39.pack\n==> member\nmember/.git/objects/pack/pack-d51c724345703411d57134d018e9643924900d39.idx\nmember/.git/objects/pack/pack-d51c724345703411d57134d018e9643924900d39.pack\n\n> The basic reason is that write_midx_included_packs() takes all of the\n> pack-files from the geometry, but does not strip out the pack-files\n> that are not in the same object directory.\n\nI agree here; the pack-geometry code should be stripping out non-local\npacks when writing a MIDX regardless of whether the caller passed\n--local or not.\n\n> Perhaps this method could include a step to create a new, \"local\"\n> geometry containing only the packs within the local object dir. We can\n> then skip the --preferred-pack option and bitmap if this is different\n> than the original geometry (perhaps with a warning message to help\n> users who did this accidentally).\n\nIt would be nice to not have to juggle multiple pack geometry structs,\nsince the logic of what gets repacked, what gets thrown away, and what\ngets kept is already fairly complicated (at least to me) and pretty\nfragile.\n\nI think if I were sketching this out, I'd start out by doing something\nlike:\n\n--- 8< ---\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex df4d8e0f0b..eab5f58444 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -558,6 +558,10 @@ static void midx_included_packs(struct string_list *include,\n \t\tfor (i = geometry->split; i < geometry->pack_nr; i++) {\n \t\t\tstruct packed_git *p = geometry->pack[i];\n\n+\t\t\t/* MIDXs cannot refer to non-local packs */\n+\t\t\tif (!p->pack_local)\n+\t\t\t\tcontinue;\n+\n \t\t\tstrbuf_addstr(&buf, pack_basename(p));\n \t\t\tstrbuf_strip_suffix(&buf, \".pack\");\n \t\t\tstrbuf_addstr(&buf, \".idx\");\n--- >8 ---\n\n...and I actually think that might do it, since:\n\n\t- existing_nonkept_packs is populated by calling readdir() on the local\n\t\trepository's $GIT_DIR/objects/pack directory, so it will never contain\n\t\tany non-local packs.\n\n\t- existing_kept_packs is also OK for the same reason\n\n\t- names (which tracks the packs that we just wrote) will never contain a\n\t\tnon-local pack, since we never write packs outside of our local pack\n\t\tdirectory\n\nSo that would cause you to write a MIDX containing only local packs (as\ndesired) regardless of whether or not the caller passed --[no]-local or\nnot.\n\n> > I'd personally be fine to start honoring the `po_args.local` flag so\n> > that we skip over any non-local packfiles there while ignoring the\n> > larger problem of non-local geometric repacks breaking in certain\n> > scenarios. It would at least improve the status quo as users now have a\n> > way out in case they ever happen to hit that error. And it allows us to\n> > use geometric repacks in alternated repositories. But are we okay with\n> > punting on the larger issue for the time being?\n>\n> I think the real bug is isolated in write_midx_included_packs() in how\n> it may specify packs that the multi-pack-index cannot track. It should\n> be worth the time exploring if there is an easy fix there, and then the\n> po_args.local version can be used as a backup/performance tweak on top\n> of that.\n\nYeah, seeing \"unknown preferred pack\" makes me suspicious that we tried\nto write a MIDX that contains a pack outside of our repository. I tried\nto reproduce that case with the following script but couldn't do it (the\npack that the MIDX does track is the local one, as expected):\n\n--- 8< ---\n#!/bin/sh\n\nrm -fr shared member\n\ngit init shared\ngit -C shared commit --allow-empty -m \"base\" && git -C shared repack -d\n\ngit clone --shared shared member\n\ngit -C member commit --allow-empty -m \"base\" && git -C member repack -d\n\n{\n\tbasename \"$(find shared/.git/objects/pack -type f -name '*.idx')\"\n\tbasename \"$(find member/.git/objects/pack -type f -name '*.idx')\"\n} |\ngit.compile -C member multi-pack-index write --stdin-packs\n\nfor repo in shared member\ndo\n\techo \"==> $repo\"\n\tfind $repo/.git/objects/pack -type f\ndone\n\n( cd member && ~/src/git/t/helper/test-tool read-midx --show-objects .git/objects )\n--- >8 ---\n\nBut the MIDX code that is responsible for collecting the packs specified\nover --stdin-packs enumerates the possible packs by calling\n`for_each_file_in_pack_dir()` with our local `object_dir`, so I don't\nthink it's possible to have a MIDX that contains a non-local pack.\n\nIOW, I agree with you that the bug is in write_midx_included_packs(),\nand I think the fix that I outlined would do the trick.\n\nThanks,\nTaylor\n"},{"id":"475156","messageId":"ZDWTp2RfFTbiE9I9@ncase","threadId":"59535","inReplyTo":"4c7b95e1-9d3c-e253-98ca-ac6c201babb3@github.com","subject":"Re: [PATCH] repack: fix geometric repacking with gitalternates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-11T17:06:47Z","receivedAt":"2023-04-11T17:07:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 10, 2023 at 11:06:49AM -0400, Derrick Stolee wrote:\n> On 4/5/2023 3:08 AM, Patrick Steinhardt wrote:\n> > On Tue, Apr 04, 2023 at 02:55:50PM -0400, Taylor Blau wrote:\n> >> On Tue, Apr 04, 2023 at 01:08:33PM +0200, Patrick Steinhardt wrote:\n[snip]\n> > I'd personally be fine to start honoring the `po_args.local` flag so\n> > that we skip over any non-local packfiles there while ignoring the\n> > larger problem of non-local geometric repacks breaking in certain\n> > scenarios. It would at least improve the status quo as users now have a\n> > way out in case they ever happen to hit that error. And it allows us to\n> > use geometric repacks in alternated repositories. But are we okay with\n> > punting on the larger issue for the time being?\n> \n> I think the real bug is isolated in write_midx_included_packs() in how\n> it may specify packs that the multi-pack-index cannot track. It should\n> be worth the time exploring if there is an easy fix there, and then the\n> po_args.local version can be used as a backup/performance tweak on top\n> of that.\n\nThe problems are quite multi-faceted, but taken on their own most of the\nproblems actually seem quite easy to fix. I've got a local patch series\nthat addresses almost all of the pain points I have found until now:\n\n- A segfault in git-multi-pack-index(1) when passed no packfiles and a\n preferred packfile that it cannot find.\n\n- The issue that git-repack(1) asks git-multi-pack-index(1) to write an\n MIDX with packs that it cannot actually track because they are not\n local.\n\n- An issue with git-pack-objects(1) that keeps it from packing objects\n that are non-local due to the way `--stdin-packs` is only referring to\n the packfile basenames.\n\n- The fact that we don't honor `-l` when doing geometric repacking.\n\nI'm still busy polishing it and finding testcases for each of these to\ndemonstrate the actual bugs, but will send out the series later this\nweek.\n\nPatrick\n"},{"id":"475160","messageId":"ZDWVSuFJLU54txwc@ncase","threadId":"59535","inReplyTo":"ZDSgbYa+j/5c5t8j@nand.local","subject":"Re: [PATCH] repack: fix geometric repacking with gitalternates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-11T17:13:46Z","receivedAt":"2023-04-11T17:13:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 10, 2023 at 07:49:01PM -0400, Taylor Blau wrote:\n> On Mon, Apr 10, 2023 at 11:06:49AM -0400, Derrick Stolee wrote:\n[snip]\n> > Perhaps this method could include a step to create a new, \"local\"\n> > geometry containing only the packs within the local object dir. We can\n> > then skip the --preferred-pack option and bitmap if this is different\n> > than the original geometry (perhaps with a warning message to help\n> > users who did this accidentally).\n> \n> It would be nice to not have to juggle multiple pack geometry structs,\n> since the logic of what gets repacked, what gets thrown away, and what\n> gets kept is already fairly complicated (at least to me) and pretty\n> fragile.\n> \n> I think if I were sketching this out, I'd start out by doing something\n> like:\n> \n> --- 8< ---\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index df4d8e0f0b..eab5f58444 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -558,6 +558,10 @@ static void midx_included_packs(struct string_list *include,\n> \t\tfor (i = geometry->split; i < geometry->pack_nr; i++) {\n> \t\t\tstruct packed_git *p = geometry->pack[i];\n> \n> +\t\t\t/* MIDXs cannot refer to non-local packs */\n> +\t\t\tif (!p->pack_local)\n> +\t\t\t\tcontinue;\n> +\n> \t\t\tstrbuf_addstr(&buf, pack_basename(p));\n> \t\t\tstrbuf_strip_suffix(&buf, \".pack\");\n> \t\t\tstrbuf_addstr(&buf, \".idx\");\n> --- >8 ---\n> \n> ...and I actually think that might do it, since:\n> \n> \t- existing_nonkept_packs is populated by calling readdir() on the local\n> \t\trepository's $GIT_DIR/objects/pack directory, so it will never contain\n> \t\tany non-local packs.\n> \n> \t- existing_kept_packs is also OK for the same reason\n> \n> \t- names (which tracks the packs that we just wrote) will never contain a\n> \t\tnon-local pack, since we never write packs outside of our local pack\n> \t\tdirectory\n> \n> So that would cause you to write a MIDX containing only local packs (as\n> desired) regardless of whether or not the caller passed --[no]-local or\n> not.\n\nI think this doesn't actually do anything. The reason is that the\n`--stdin-packs` option in git-multi-pack-index(1) does not actually\ntreat the provided packs as packs that are to be included in the\nmulti-pack-index. Instead, it uses it as a filter for `get_all_packs()`.\nAnd as `get_all_packs()` returns packfiles paths prefixed with the\nalternate directory's name, but we pass in basenames only, we would\nalready be filtering out any non-local packfiles.\n\nSo ultimately, we would only ever write an MIDX containing only local\npacks already. It rather feels like this is only by chance though, so I\nthink it is good to include your patch regardless of whether it actually\ndoes something or not. Better be explicit here, also as documentation to\nthe reader.\n\nI'll include it as part of my patch series that I'm to send out later\nthis week.\n\nPatrick\n"},{"id":"475161","messageId":"ZDWYLPw4Hk0-12Fe@ncase","threadId":"59535","inReplyTo":"ZDWTp2RfFTbiE9I9@ncase","subject":"Re: [PATCH] repack: fix geometric repacking with gitalternates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-11T17:26:04Z","receivedAt":"2023-04-11T17:26:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 11, 2023 at 07:06:48PM +0200, Patrick Steinhardt wrote:\n> On Mon, Apr 10, 2023 at 11:06:49AM -0400, Derrick Stolee wrote:\n> > On 4/5/2023 3:08 AM, Patrick Steinhardt wrote:\n> > > On Tue, Apr 04, 2023 at 02:55:50PM -0400, Taylor Blau wrote:\n> > >> On Tue, Apr 04, 2023 at 01:08:33PM +0200, Patrick Steinhardt wrote:\n> [snip]\n> > > I'd personally be fine to start honoring the `po_args.local` flag so\n> > > that we skip over any non-local packfiles there while ignoring the\n> > > larger problem of non-local geometric repacks breaking in certain\n> > > scenarios. It would at least improve the status quo as users now have a\n> > > way out in case they ever happen to hit that error. And it allows us to\n> > > use geometric repacks in alternated repositories. But are we okay with\n> > > punting on the larger issue for the time being?\n> > \n> > I think the real bug is isolated in write_midx_included_packs() in how\n> > it may specify packs that the multi-pack-index cannot track. It should\n> > be worth the time exploring if there is an easy fix there, and then the\n> > po_args.local version can be used as a backup/performance tweak on top\n> > of that.\n> \n> The problems are quite multi-faceted, but taken on their own most of the\n> problems actually seem quite easy to fix. I've got a local patch series\n> that addresses almost all of the pain points I have found until now:\n> \n> - A segfault in git-multi-pack-index(1) when passed no packfiles and a\n> preferred packfile that it cannot find.\n> \n> - The issue that git-repack(1) asks git-multi-pack-index(1) to write an\n> MIDX with packs that it cannot actually track because they are not\n> local.\n> \n> - An issue with git-pack-objects(1) that keeps it from packing objects\n> that are non-local due to the way `--stdin-packs` is only referring to\n> the packfile basenames.\n> \n> - The fact that we don't honor `-l` when doing geometric repacking.\n\nAh, one more thing I forgot: add a safety mechanism that disables\nwriting bitmaps when we see non-local packs.\n\nPatrick\n"},{"id":"475170","messageId":"ZDXNaCYV+DhxaBt3@nand.local","threadId":"59535","inReplyTo":"ZDWVSuFJLU54txwc@ncase","subject":"Re: [PATCH] repack: fix geometric repacking with gitalternates","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-11T21:13:12Z","receivedAt":"2023-04-11T21:13:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 11, 2023 at 07:13:46PM +0200, Patrick Steinhardt wrote:\n> So ultimately, we would only ever write an MIDX containing only local\n> packs already. It rather feels like this is only by chance though, so I\n> think it is good to include your patch regardless of whether it actually\n> does something or not. Better be explicit here, also as documentation to\n> the reader.\n\nDoh, this is definitely right. I even wrote that code a while ago; shows\nyou how good my memory is ;-).\n\nFWIW, I agree that even though it doesn't do anything in this instance,\nit is a good safeguard to have in place, so I think including it in your\nseries is the right thing to do.\n\nThanks,\nTaylor\n"},{"id":"475171","messageId":"ZDXNtygNkdSbohTe@nand.local","threadId":"59535","inReplyTo":"ZDWYLPw4Hk0-12Fe@ncase","subject":"Re: [PATCH] repack: fix geometric repacking with gitalternates","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-11T21:14:31Z","receivedAt":"2023-04-11T21:14:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 11, 2023 at 07:26:04PM +0200, Patrick Steinhardt wrote:\n> > The problems are quite multi-faceted, but taken on their own most of the\n> > problems actually seem quite easy to fix. I've got a local patch series\n> > that addresses almost all of the pain points I have found until now:\n> >\n> > - A segfault in git-multi-pack-index(1) when passed no packfiles and a\n> > preferred packfile that it cannot find.\n> >\n> > - The issue that git-repack(1) asks git-multi-pack-index(1) to write an\n> > MIDX with packs that it cannot actually track because they are not\n> > local.\n> >\n> > - An issue with git-pack-objects(1) that keeps it from packing objects\n> > that are non-local due to the way `--stdin-packs` is only referring to\n> > the packfile basenames.\n> >\n> > - The fact that we don't honor `-l` when doing geometric repacking.\n>\n> Ah, one more thing I forgot: add a safety mechanism that disables\n> writing bitmaps when we see non-local packs.\n\nThank you very much for working on this.\n\nFor what it's worth, I think that this whole thing is pretty cool.\nHaving a couple of different forges use this feature in subtly different\nways is proving to be an extremely effective way to shake out subtle\nbugs in the implementation.\n\nThanks,\nTaylor\n"},{"id":"475203","messageId":"ZDZ7661iNaWW9Vj7@ncase","threadId":"59535","inReplyTo":"ZDXNaCYV+DhxaBt3@nand.local","subject":"Re: [PATCH] repack: fix geometric repacking with gitalternates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-12T09:37:47Z","receivedAt":"2023-04-12T09:38:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 11, 2023 at 05:13:12PM -0400, Taylor Blau wrote:\n> On Tue, Apr 11, 2023 at 07:13:46PM +0200, Patrick Steinhardt wrote:\n> > So ultimately, we would only ever write an MIDX containing only local\n> > packs already. It rather feels like this is only by chance though, so I\n> > think it is good to include your patch regardless of whether it actually\n> > does something or not. Better be explicit here, also as documentation to\n> > the reader.\n> \n> Doh, this is definitely right. I even wrote that code a while ago; shows\n> you how good my memory is ;-).\n> \n> FWIW, I agree that even though it doesn't do anything in this instance,\n> it is a good safeguard to have in place, so I think including it in your\n> series is the right thing to do.\n\nI was wrong, it actually does fix a problem, just not the one we thought\nit would fix. When all packfiles are non-local, git-multi-pack-index(1)\nwould end up returning an error because it cannot find any of them. So\nby skipping over these non-local packfiles early on we avoid spawning\nit altogether and thus avoid the error.\n\nPartick\n"},{"id":"475207","messageId":"cover.1681294715.git.ps@pks.im","threadId":"59535","inReplyTo":"a07ed50feeec4bfc3e9736bf493b9876896bcdd2.1680606445.git.ps@pks.im","subject":"[PATCH v2 0/8] repack: fix geometric repacking with gitalternates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-12T10:22:27Z","receivedAt":"2023-04-12T10:22:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch to fix geometric repacking with\nrepositories that are connected to an alternate object database.\nFollowing the discussion with Taylor and Derrick this series goes into\nmuch more detail than the first version and fixes a bunch of issues all\nrelated to geometric repacking.\n\nPatrick\n\nPatrick Steinhardt (8):\n midx: fix segfault with no packs and invalid preferred pack\n repack: fix trying to use preferred pack in alternates\n repack: fix generating multi-pack-index with only non-local packs\n pack-objects: fix error when packing same pack twice\n pack-objects: fix error when same packfile is included and excluded\n pack-objects: extend test coverage of `--stdin-packs` with alternates\n repack: honor `-l` when calculating pack geometry\n repack: disable writing bitmaps when doing a local geometric repack\n\n builtin/pack-objects.c | 10 +-\n builtin/repack.c | 65 +++++++++++--\n midx.c | 6 ++\n t/t5319-multi-pack-index.sh | 11 +++\n t/t5331-pack-objects-stdin.sh | 103 +++++++++++++++++++++\n t/t7703-repack-geometric.sh | 166 ++++++++++++++++++++++++++++++++++\n 6 files changed, 349 insertions(+), 12 deletions(-)\n create mode 100755 t/t5331-pack-objects-stdin.sh\n\n-- \n2.40.0\n\n"},{"id":"475208","messageId":"5ecad306b42441fa7d4f50bdfb9c09ccce22b6c9.1681294715.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681294715.git.ps@pks.im","subject":"[PATCH v2 1/8] midx: fix segfault with no packs and invalid preferred pack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-12T10:22:31Z","receivedAt":"2023-04-12T10:22:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When asked to write a multi-pack-index the user can specify a preferred\npack that is used as a tie breaker when multiple packs contain the same\nobjects. When this packfile cannot be found, we just pick the first pack\nthat is getting tracked by the newly written multi-pack-index as a\nfallback.\n\nPicking the fallback can fail in the case where we're asked to write a\nmulti-pack-index with no packfiles at all: picking the fallback value\nwill cause a segfault as we blindly index into the array of packfiles,\nwhich would be empty.\n\nFix this bug by exiting early in case we have determined that the MIDX\nwouldn't have any packfiles to index. While the request itself does not\nmake much sense anyway, it is still preferable to exit gracefully than\nto abort.\n\nSigned-off-by: Patrick Steinhardt \n---\n midx.c | 6 ++++++\n t/t5319-multi-pack-index.sh | 11 +++++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/midx.c b/midx.c\nindex 9af3e5de88..22ea7ffb75 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1307,6 +1307,12 @@ static int write_midx_internal(const char *object_dir,\n \tfor_each_file_in_pack_dir(object_dir, add_pack_to_midx, &ctx);\n \tstop_progress(&ctx.progress);\n \n+\tif (!ctx.nr) {\n+\t\terror(_(\"no pack files to index.\"));\n+\t\tresult = 1;\n+\t\tgoto cleanup;\n+\t}\n+\n \tif ((ctx.m && ctx.nr == ctx.m->num_packs) &&\n \t !(packs_to_include || packs_to_drop)) {\n \t\tstruct bitmap_index *bitmap_git;\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 499d5d4c78..be7f3c1e1f 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -183,6 +183,17 @@ test_expect_success 'write midx with --stdin-packs' '\n \n compare_results_with_midx \"mixed mode (one pack + extra)\"\n \n+test_expect_success 'write with no objects and preferred pack' '\n+\ttest_when_finished \"rm -rf empty\" &&\n+\tgit init empty &&\n+\ttest_must_fail git -C empty multi-pack-index write \\\n+\t\t--stdin-packs --preferred-pack=does-not-exist err &&\n+\tcat >expect <<-EOF &&\n+\terror: no pack files to index.\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n test_expect_success 'write progress off for redirected stderr' '\n \tgit multi-pack-index --object-dir=$objdir write 2>err &&\n \ttest_line_count = 0 err\n-- \n2.40.0\n\n"},{"id":"475209","messageId":"011b08f3b64f264e3abbe8b49ee5338c221badb9.1681294715.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681294715.git.ps@pks.im","subject":"[PATCH v2 2/8] repack: fix trying to use preferred pack in alternates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-12T10:22:35Z","receivedAt":"2023-04-12T10:22:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When doing a geometric repack with multi-pack-indices, then we ask\ngit-multi-pack-index(1) to use the largest packfile as the preferred\npack. It can happen though that the largest packfile is not part of the\nmain object database, but instead part of an alternate object database.\nThe result is that git-multi-pack-index(1) will not be able to find the\npreferred pack and print a warning. It then falls back to use the first\npackfile that the multi-pack-index shall reference.\n\nFix this bug by only considering packfiles as preferred pack that are\nlocal. This is the right thing to do given that a multi-pack-index\nshould never reference packfiles borrowed from an alternate.\n\nWhile at it, rename the function `get_largest_active_packfile()` to\n`get_preferred_pack()` to better document its intent.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/repack.c | 21 ++++++++++++++++-----\n t/t7703-repack-geometric.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 47 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex df4d8e0f0b..9d36dc8b84 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -448,8 +448,10 @@ static void split_pack_geometry(struct pack_geometry *geometry, int factor)\n \tgeometry->split = split;\n }\n \n-static struct packed_git *get_largest_active_pack(struct pack_geometry *geometry)\n+static struct packed_git *get_preferred_pack(struct pack_geometry *geometry)\n {\n+\tuint32_t i;\n+\n \tif (!geometry) {\n \t\t/*\n \t\t * No geometry means either an all-into-one repack (in which\n@@ -464,7 +466,16 @@ static struct packed_git *get_largest_active_pack(struct pack_geometry *geometry\n \t}\n \tif (geometry->split == geometry->pack_nr)\n \t\treturn NULL;\n-\treturn geometry->pack[geometry->pack_nr - 1];\n+\n+\tfor (i = geometry->pack_nr; i > 0; i--)\n+\t\t/*\n+\t\t * A pack that is not local would never be included in a\n+\t\t * multi-pack index. We thus skip over any non-local packs.\n+\t\t */\n+\t\tif (geometry->pack[i - 1]->pack_local)\n+\t\t\treturn geometry->pack[i - 1];\n+\n+\treturn NULL;\n }\n \n static void clear_pack_geometry(struct pack_geometry *geometry)\n@@ -591,7 +602,7 @@ static int write_midx_included_packs(struct string_list *include,\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct string_list_item *item;\n-\tstruct packed_git *largest = get_largest_active_pack(geometry);\n+\tstruct packed_git *preferred = get_preferred_pack(geometry);\n \tFILE *in;\n \tint ret;\n \n@@ -612,9 +623,9 @@ static int write_midx_included_packs(struct string_list *include,\n \tif (write_bitmaps)\n \t\tstrvec_push(&cmd.args, \"--bitmap\");\n \n-\tif (largest)\n+\tif (preferred)\n \t\tstrvec_pushf(&cmd.args, \"--preferred-pack=%s\",\n-\t\t\t pack_basename(largest));\n+\t\t\t pack_basename(preferred));\n \n \tif (refs_snapshot)\n \t\tstrvec_pushf(&cmd.args, \"--refs-snapshot=%s\", refs_snapshot);\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 8821fbd2dd..92a1aaa754 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -281,4 +281,35 @@ test_expect_success '--geometric with pack.packSizeLimit' '\n \t)\n '\n \n+test_expect_success '--geometric --write-midx with packfiles in main and alternate ODB' '\n+\ttest_when_finished \"rm -fr shared member\" &&\n+\n+\t# Create a shared repository that will serve as the alternate object\n+\t# database for the member linked to it. It has got some objects on its\n+\t# own that are packed into a single packfile.\n+\tgit init shared &&\n+\ttest_commit -C shared common-object &&\n+\tgit -C shared repack -ad &&\n+\n+\t# We create member so that its alternates file points to the shared\n+\t# repository. We then create a commit in it so that git-repack(1) has\n+\t# something to repack.\n+\t# of the shared object database.\n+\tgit clone --shared shared member &&\n+\ttest_commit -C member unique-object &&\n+\tgit -C member repack --geometric=2 --write-midx 2>err &&\n+\ttest_must_be_empty err &&\n+\n+\t# We should see that a new packfile was generated.\n+\tfind shared/.git/objects/pack -type f -name \"*.pack\" | sort >packs &&\n+\ttest $(wc -l \nSigned-off-by: Patrick Steinhardt \n---\n builtin/repack.c | 11 +++++++++++\n t/t7703-repack-geometric.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 35 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 9d36dc8b84..80d4e813c8 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -569,6 +569,17 @@ static void midx_included_packs(struct string_list *include,\n \t\tfor (i = geometry->split; i < geometry->pack_nr; i++) {\n \t\t\tstruct packed_git *p = geometry->pack[i];\n \n+\t\t\t/*\n+\t\t\t * The multi-pack index never refers to packfiles part\n+\t\t\t * of an alternate object database, so we skip these.\n+\t\t\t * While git-multi-pack-index(1) would silently ignore\n+\t\t\t * them anyway, this allows us to skip executing the\n+\t\t\t * command completely when we have only non-local\n+\t\t\t * packfiles.\n+\t\t\t */\n+\t\t\tif (!p->pack_local)\n+\t\t\t\tcontinue;\n+\n \t\t\tstrbuf_addstr(&buf, pack_basename(p));\n \t\t\tstrbuf_strip_suffix(&buf, \".pack\");\n \t\t\tstrbuf_addstr(&buf, \".idx\");\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 92a1aaa754..9e95350cbf 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -312,4 +312,28 @@ test_expect_success '--geometric --write-midx with packfiles in main and alterna\n \tgit -C member multi-pack-index verify\n '\n \n+test_expect_success '--geometric --with-midx with no local objects' '\n+\ttest_when_finished \"rm -fr shared member\" &&\n+\n+\t# Create a repository with a single packfile that acts as alternate\n+\t# object database.\n+\tgit init shared &&\n+\ttest_commit -C shared \"shared-objects\" &&\n+\tgit -C shared repack -ad &&\n+\n+\t# Create a second repository linked to the first one and perform a\n+\t# geometric repack on it.\n+\tgit clone --shared shared member &&\n+\tgit -C member repack --geometric 2 --write-midx 2>err &&\n+\ttest_must_be_empty err &&\n+\n+\t# Assert that we wrote neither a new packfile nor a multi-pack-index.\n+\t# We should not have a packfile because the single packfile in the\n+\t# alternate object database does not invalidate the geometric sequence.\n+\t# And we should not have a multi-pack-index because these only index\n+\t# local packfiles, and there are none.\n+\tfind member/.git/objects/pack -type f >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475211","messageId":"e7d30fd22c65b33defb944bd043a56d0c525f875.1681294715.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681294715.git.ps@pks.im","subject":"[PATCH v2 4/8] pack-objects: fix error when packing same pack twice","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-12T10:22:44Z","receivedAt":"2023-04-12T10:23:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When passed the same packfile twice via `--stdin-packs` we return an\nerror that the packfile supposedly was not found. This is because when\nreading packs into the list of included or excluded packfiles, we will\nhappily re-add packfiles even if they are part of the lists already. And\nwhile the list can now contain duplicates, we will only set the `util`\npointer of the first list entry to the `packed_git` structure. We notice\nthat at a later point when checking that all list entries have their\n`util` pointer set and die with an error.\n\nWhile this is kind of a nonsensical request, this scenario can be hit\nwhen doing geometric repacks. When a repository is connected to an\nalternate object directory and both have the exact same packfile then\nboth would get added to the geometric sequence. And when we then decide\nto perform the repack, we will invoke git-pack-objects(1) with the same\npackfile twice.\n\nFix this bug by removing any duplicates from both the included and\nexcluded packs.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-objects.c | 2 ++\n t/t5331-pack-objects-stdin.sh | 29 +++++++++++++++++++++++++++++\n t/t7703-repack-geometric.sh | 25 +++++++++++++++++++++++++\n 3 files changed, 56 insertions(+)\n create mode 100755 t/t5331-pack-objects-stdin.sh\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 77d88f85b0..fdf3f440be 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3359,7 +3359,9 @@ static void read_packs_list_from_stdin(void)\n \t}\n \n \tstring_list_sort(&include_packs);\n+\tstring_list_remove_duplicates(&include_packs, 0);\n \tstring_list_sort(&exclude_packs);\n+\tstring_list_remove_duplicates(&exclude_packs, 0);\n \n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tconst char *pack_name = pack_basename(p);\ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nnew file mode 100755\nindex 0000000000..ab34cfc729\n--- /dev/null\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -0,0 +1,29 @@\n+#!/bin/sh\n+\n+test_description='pack-objects --stdin'\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'pack-objects --stdin with duplicate packfile' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit \"commit\" &&\n+\t\tgit repack -ad &&\n+\n+\t\t(\n+\t\t\tbasename .git/objects/pack/pack-*.pack &&\n+\t\t\tbasename .git/objects/pack/pack-*.pack\n+\t\t) >packfiles &&\n+\n+\t\tgit pack-objects --stdin-packs generated-pack member/.git/objects/info/alternates &&\n+\tfind shared/.git/objects -type f >expected-files &&\n+\n+\t# Verify that we can repack objects as expected without observing any\n+\t# error. Having the same packfile in both ODBs used to cause an error\n+\t# in git-pack-objects(1).\n+\tgit -C member repack --geometric 2 2>err &&\n+\ttest_must_be_empty err &&\n+\t# Nothing should have changed.\n+\tfind shared/.git/objects -type f >actual-files &&\n+\ttest_cmp expected-files actual-files\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475212","messageId":"9b278a4c91a5631d1b7b11bf56ab560c6bb58645.1681294715.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681294715.git.ps@pks.im","subject":"[PATCH v2 5/8] pack-objects: fix error when same packfile is included and excluded","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-12T10:22:48Z","receivedAt":"2023-04-12T10:23:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When passing the same packfile both as included and excluded via the\n`--stdin-packs` option, then we will return an error because the\nexcluded packfile cannot be found. This is because we will only set the\n`util` pointer for the included packfile list if it was found, so that\nwe later die when we notice that it's in fact not set for the excluded\npackfile list.\n\nFix this bug by always setting the `util` pointer for both the included\nand excluded list entries.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-objects.c | 8 +++-----\n t/t5331-pack-objects-stdin.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 29 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex fdf3f440be..522eb4dd31 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3366,11 +3366,9 @@ static void read_packs_list_from_stdin(void)\n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tconst char *pack_name = pack_basename(p);\n \n-\t\titem = string_list_lookup(&include_packs, pack_name);\n-\t\tif (!item)\n-\t\t\titem = string_list_lookup(&exclude_packs, pack_name);\n-\n-\t\tif (item)\n+\t\tif ((item = string_list_lookup(&include_packs, pack_name)))\n+\t\t\titem->util = p;\n+\t\tif ((item = string_list_lookup(&exclude_packs, pack_name)))\n \t\t\titem->util = p;\n \t}\n \ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex ab34cfc729..f389a78b38 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -7,6 +7,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+packed_objects() {\n+\tgit show-index <\"$1\" >tmp-object-list &&\n+\tcut -d' ' -f2 tmp-object-list &&\n+\trm tmp-object-list\n+ }\n+\n test_expect_success 'pack-objects --stdin with duplicate packfile' '\n \ttest_when_finished \"rm -fr repo\" &&\n \n@@ -26,4 +32,24 @@ test_expect_success 'pack-objects --stdin with duplicate packfile' '\n \t)\n '\n \n+test_expect_success 'pack-objects --stdin with same packfile excluded and included' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit \"commit\" &&\n+\t\tgit repack -ad &&\n+\n+\t\t(\n+\t\t\tbasename .git/objects/pack/pack-*.pack &&\n+\t\t\tprintf \"^%s\\n\" \"$(basename .git/objects/pack/pack-*.pack)\"\n+\t\t) >packfiles &&\n+\n+\t\tgit pack-objects --stdin-packs generated-pack packed-objects &&\n+\t\ttest_must_be_empty packed-objects\n+\t)\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475213","messageId":"3a7a2015f7ced01dc738099f18b0fce6537af05c.1681294715.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681294715.git.ps@pks.im","subject":"[PATCH v2 6/8] pack-objects: extend test coverage of `--stdin-packs` with alternates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-12T10:22:52Z","receivedAt":"2023-04-12T10:23:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We don't have any tests that verify that git-pack-objects(1) works with\n`--stdin-packs` when combined with alternate object directories. Add\nsome to make sure that the basic functionality works as expected.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5331-pack-objects-stdin.sh | 48 +++++++++++++++++++++++++++++++++++\n 1 file changed, 48 insertions(+)\n\ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex f389a78b38..8bd1d26caf 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -52,4 +52,52 @@ test_expect_success 'pack-objects --stdin with same packfile excluded and includ\n \t)\n '\n \n+test_expect_success 'pack-objects --stdin with packfiles from alternate object database' '\n+\ttest_when_finished \"rm -fr shared member\" &&\n+\n+\t# Set up a shared repository with a single packfile.\n+\tgit init shared &&\n+\ttest_commit -C shared \"shared-objects\" &&\n+\tgit -C shared repack -ad &&\n+\tbasename shared/.git/objects/pack/pack-*.pack >packfile &&\n+\n+\t# Set up a repository that is connected to the shared repository. This\n+\t# repository has no objects on its own, but we still expect to be able\n+\t# to pack objects from its alternate.\n+\tgit clone --shared shared member &&\n+\tgit -C member pack-objects --stdin-packs generated-pack packfiles &&\n+\n+\t(\n+\t\tpacked_objects shared/.git/objects/pack/pack-*.idx &&\n+\t\tpacked_objects member/.git/objects/pack/pack-*.idx\n+\t) | sort >expected-objects &&\n+\n+\tgit -C member pack-objects --stdin-packs generated-pack actual-objects &&\n+\ttest_cmp expected-objects actual-objects\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475214","messageId":"608dde4ad52c28ef42845b6bfdcb168e252bd29b.1681294715.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681294715.git.ps@pks.im","subject":"[PATCH v2 7/8] repack: honor `-l` when calculating pack geometry","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-12T10:22:56Z","receivedAt":"2023-04-12T10:23:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When the user passes `-l` to git-repack(1), then they essentially ask us\nto only repack objects part of the local object database while ignoring\nany packfiles part of an alternate object database. And we in fact honor\nthis bit when doing a geometric repack as the resulting packfile will\nonly ever contain local objects.\n\nWhat we're missing though is that we don't take locality of packfiles\ninto account when computing whether the geometric sequence is intact or\nnot. So even though we would only ever roll up local packfiles anyway,\nwe could end up trying to repack because of non-local packfiles. This\ndoes not make much sense, and in the worst case it can cause us to try\nand do the geometric repack over and over again because we're never able\nto restore the geometric sequence.\n\nFix this bug by honoring whether the user has passed `-l`. If so, we\nskip adding any non-local packfiles to the pack geometry.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/repack.c | 13 ++++++--\n t/t7703-repack-geometric.sh | 59 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 70 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 80d4e813c8..f57869f14a 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -325,7 +325,8 @@ static int geometry_cmp(const void *va, const void *vb)\n }\n \n static void init_pack_geometry(struct pack_geometry **geometry_p,\n-\t\t\t struct string_list *existing_kept_packs)\n+\t\t\t struct string_list *existing_kept_packs,\n+\t\t\t const struct pack_objects_args *args)\n {\n \tstruct packed_git *p;\n \tstruct pack_geometry *geometry;\n@@ -335,6 +336,14 @@ static void init_pack_geometry(struct pack_geometry **geometry_p,\n \tgeometry = *geometry_p;\n \n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n+\t\tif (args->local && !p->pack_local)\n+\t\t\t/*\n+\t\t\t * When asked to only repack local packfiles we skip\n+\t\t\t * over any packfiles that are borrowed from alternate\n+\t\t\t * object directories.\n+\t\t\t */\n+\t\t\tcontinue;\n+\n \t\tif (!pack_kept_objects) {\n \t\t\t/*\n \t\t\t * Any pack that has its pack_keep bit set will appear\n@@ -897,7 +906,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (geometric_factor) {\n \t\tif (pack_everything)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--geometric\", \"-A/-a\");\n-\t\tinit_pack_geometry(&geometry, &existing_kept_packs);\n+\t\tinit_pack_geometry(&geometry, &existing_kept_packs, &po_args);\n \t\tsplit_pack_geometry(geometry, geometric_factor);\n \t}\n \ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 0a2f2bd46c..96c8d4cdfa 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -10,6 +10,12 @@ objdir=.git/objects\n packdir=$objdir/pack\n midx=$objdir/pack/multi-pack-index\n \n+packed_objects() {\n+\tgit show-index <\"$1\" >tmp-object-list &&\n+\tcut -d' ' -f2 tmp-object-list &&\n+\trm tmp-object-list\n+ }\n+\n test_expect_success '--geometric with no packs' '\n \tgit init geometric &&\n \ttest_when_finished \"rm -fr geometric\" &&\n@@ -361,4 +367,57 @@ test_expect_success '--geometric with same pack in main and alternate ODB' '\n \ttest_cmp expected-files actual-files\n '\n \n+test_expect_success '--geometric -l with non-intact geometric sequence across ODBs' '\n+\ttest_when_finished \"rm -fr shared member\" &&\n+\n+\tgit init shared &&\n+\ttest_commit_bulk -C shared --start=1 1 &&\n+\n+\tgit clone --shared shared member &&\n+\ttest_commit_bulk -C member --start=2 1 &&\n+\n+\t# Verify that our assumptions actually hold: both generated packfiles\n+\t# should have three objects and should be non-equal.\n+\tpacked_objects shared/.git/objects/pack/pack-*.idx >packed-objects &&\n+\ttest_line_count = 3 packed-objects &&\n+\tpacked_objects member/.git/objects/pack/pack-*.idx >packed-objetcs &&\n+\ttest_line_count = 3 packed-objects &&\n+\ttest \"$(basename member/.git/objects/pack/pack-*.pack)\" != \"$(basename shared/.git/objects/pack/pack-*.pack)\" &&\n+\n+\t# Perform the geometric repack. With `-l`, we should only see the local\n+\t# packfile and thus arrive at the conclusion that the geometric\n+\t# sequence is intact. We thus expect no changes.\n+\t#\n+\t# Note that we are using stat(1) to verify idempotence to also verify\n+\t# that the mtime did not change. This is done in order to detect the\n+\t# case where we do repack objects, but the resulting packfile is the\n+\t# same.\n+\tstat member/.git/objects/pack/* >expected-member-packs &&\n+\tgit -C member repack --geometric=2 -l -d &&\n+\tstat member/.git/objects/pack/* >actual-member-packs &&\n+\ttest_cmp expected-member-packs actual-member-packs &&\n+\n+\t(\n+\t\tpacked_objects shared/.git/objects/pack/pack-*.idx &&\n+\t\tpacked_objects member/.git/objects/pack/pack-*.idx\n+\t) | sort >expected-objects &&\n+\n+\t# On the other hand, when doing a non-local geometric repack we should\n+\t# see both packfiles and thus repack them. We expect that the shared\n+\t# object database was not changed.\n+\tstat shared/.git/objects/pack/* >expected-shared-packs &&\n+\tgit -C member repack --geometric=2 -d &&\n+\tstat shared/.git/objects/pack/* >actual-shared-packs &&\n+\ttest_cmp expected-shared-packs actual-shared-packs &&\n+\n+\t# Furthermore, we expect that the member repository now has a single\n+\t# packfile that contains the combined shared and non-shared objects.\n+\tls member/.git/objects/pack/pack-*.idx >actual &&\n+\ttest_line_count = 1 actual &&\n+\tpacked_objects member/.git/objects/pack/pack-*.idx >actual-objects &&\n+\ttest_line_count = 6 actual-objects &&\n+\tsort actual-objects.sorted &&\n+\ttest_cmp expected-objects actual-objects.sorted\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475215","messageId":"b74d0a037b07706d07fad7b438fa0cb211726575.1681294715.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681294715.git.ps@pks.im","subject":"[PATCH v2 8/8] repack: disable writing bitmaps when doing a local geometric repack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-12T10:23:01Z","receivedAt":"2023-04-12T10:23:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In order to write a bitmap, we need to have full coverage of all objects\nthat are about to be packed. In the traditional non-multi-pack-index\nworld this meant we need to do a full repack of all objects into a\nsingle packfile. But in the new multi-pack-index world we can get away\nwith writing bitmaps when we have multiple packfiles as long as the\nmulti-pack-index covers all objects.\n\nThis is not always the case though. When writing multi-pack-indices in a\nrepository that is connected to an alternate object directory we may end\nup writing a multi-pack-index that only has partial coverage of objects.\nThe end result is that writing the bitmap will fail:\n\n $ git multi-pack-index write --stdin-packs --bitmap \n---\n builtin/repack.c | 20 ++++++++++++++++++++\n t/t7703-repack-geometric.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 47 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex f57869f14a..07d92fdf87 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -881,6 +881,26 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (pack_kept_objects < 0)\n \t\tpack_kept_objects = write_bitmaps > 0 && !write_midx;\n \n+\tif (write_midx && write_bitmaps && geometric_factor && po_args.local) {\n+\t\tstruct packed_git *p;\n+\n+\t\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n+\t\t\tif (p->pack_local)\n+\t\t\t\tcontinue;\n+\n+\t\t\t/*\n+\t\t\t * When asked to do a local repack, but we have\n+\t\t\t * packfiles that are inherited from an alternate, then\n+\t\t\t * we cannot guarantee that the multi-pack-index would\n+\t\t\t * have full coverage of all objects. We thus disable\n+\t\t\t * writing bitmaps in that case.\n+\t\t\t */\n+\t\t\twarning(_(\"disabling bitmap writing, as some objects are not being packed\"));\n+\t\t\twrite_bitmaps = 0;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n \tif (write_bitmaps && !(pack_everything & ALL_INTO_ONE) && !write_midx)\n \t\tdie(_(incremental_bitmap_conflict_error));\n \ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 96c8d4cdfa..0aaec9f853 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -420,4 +420,31 @@ test_expect_success '--geometric -l with non-intact geometric sequence across OD\n \ttest_cmp expected-objects actual-objects.sorted\n '\n \n+test_expect_success '--geometric -l disables writing bitmaps with non-local packfiles' '\n+\ttest_when_finished \"rm -fr shared member\" &&\n+\n+\tgit init shared &&\n+\ttest_commit_bulk -C shared --start=1 1 &&\n+\n+\tgit clone --shared shared member &&\n+\ttest_commit_bulk -C member --start=2 1 &&\n+\n+\t# When performing a geometric repack with `-l` while connecting to an\n+\t# alternate object database that has a packfile we do not have full\n+\t# coverage of objects. As a result, we expect that writing the bitmap\n+\t# will be disabled.\n+\tgit -C member repack -l --geometric=2 --write-midx --write-bitmap-index 2>err &&\n+\tcat >expect <<-EOF &&\n+\twarning: disabling bitmap writing, as some objects are not being packed\n+\tEOF\n+\ttest_cmp expect err &&\n+\ttest ! -f member/.git/objects/pack/multi-pack-index-*.bitmap &&\n+\n+\t# On the other hand, when we repack without `-l`, we should see that\n+\t# the bitmap gets created.\n+\tgit -C member repack --geometric=2 --write-midx --write-bitmap-index 2>err &&\n+\ttest_must_be_empty err &&\n+\ttest -f member/.git/objects/pack/multi-pack-index-*.bitmap\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475223","messageId":"ZDbw6v8r89zciE3q@nand.local","threadId":"59535","inReplyTo":"5ecad306b42441fa7d4f50bdfb9c09ccce22b6c9.1681294715.git.ps@pks.im","subject":"Re: [PATCH v2 1/8] midx: fix segfault with no packs and invalid preferred pack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-12T17:56:58Z","receivedAt":"2023-04-12T17:57:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 12, 2023 at 12:22:31PM +0200, Patrick Steinhardt wrote:\n> Fix this bug by exiting early in case we have determined that the MIDX\n> wouldn't have any packfiles to index. While the request itself does not\n> make much sense anyway, it is still preferable to exit gracefully than\n> to abort.\n\nInteresting. This reminded me quite a bit of eb57277ba3 (midx: prevent\nwriting a .bitmap without any objects, 2022-02-09) which tackled a\nsimilar problem of trying to write a MIDX bitmap without any objects.\n\nWe may want to consider moving that conditional further up, since this\nmakes the conditional added in eb57277ba3 dead code AFAICT. Here's a\npatch on top of this one that I think would do the trick.\n\nIt has the added benefit of sticking a:\n\n warning: unknown preferred pack: 'does-not-exist'\n\nin the output before dying, which might be nice (though I doubt anybody\nwill ever see it ;-)). The main difference is that we unset the bitmap\nrelated bits from `flags`, which avoids us trying to compute a preferred\npack in the first place.\n\nFor it to work, though, we need to make sure that ctx.preferred_pack_idx\nis set to -1, and not zero-initialized, since we'll segfault otherwise\nwhen trying to read into an empty array.\n\n--- 8< ---\ndiff --git a/midx.c b/midx.c\nindex 22ea7ffb75..dc4821eab8 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1263,6 +1263,7 @@ static int write_midx_internal(const char *object_dir,\n \tctx.nr = 0;\n \tctx.alloc = ctx.m ? ctx.m->num_packs : 16;\n \tctx.info = NULL;\n+\tctx.preferred_pack_idx = -1;\n \tALLOC_ARRAY(ctx.info, ctx.alloc);\n\n \tif (ctx.m) {\n@@ -1307,10 +1308,10 @@ static int write_midx_internal(const char *object_dir,\n \tfor_each_file_in_pack_dir(object_dir, add_pack_to_midx, &ctx);\n \tstop_progress(&ctx.progress);\n\n-\tif (!ctx.nr) {\n-\t\terror(_(\"no pack files to index.\"));\n-\t\tresult = 1;\n-\t\tgoto cleanup;\n+\tif (!ctx.entries_nr) {\n+\t\tif (flags & MIDX_WRITE_BITMAP)\n+\t\t\twarning(_(\"refusing to write multi-pack .bitmap without any objects\"));\n+\t\tflags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP);\n \t}\n\n \tif ((ctx.m && ctx.nr == ctx.m->num_packs) &&\n@@ -1488,12 +1489,6 @@ static int write_midx_internal(const char *object_dir,\n \t\tgoto cleanup;\n \t}\n\n-\tif (!ctx.entries_nr) {\n-\t\tif (flags & MIDX_WRITE_BITMAP)\n-\t\t\twarning(_(\"refusing to write multi-pack .bitmap without any objects\"));\n-\t\tflags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP);\n-\t}\n-\n \tcf = init_chunkfile(f);\n\n \tadd_chunk(cf, MIDX_CHUNKID_PACKNAMES, pack_name_concat_len,\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex be7f3c1e1f..8a17361272 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -188,10 +188,9 @@ test_expect_success 'write with no objects and preferred pack' '\n \tgit init empty &&\n \ttest_must_fail git -C empty multi-pack-index write \\\n \t\t--stdin-packs --preferred-pack=does-not-exist err &&\n-\tcat >expect <<-EOF &&\n-\terror: no pack files to index.\n-\tEOF\n-\ttest_cmp expect err\n+\t\tcat err &&\n+\tgrep \"warning: unknown preferred pack: .does-not-exist.\" err &&\n+\tgrep \"error: no pack files to index.\" err\n '\n\n test_expect_success 'write progress off for redirected stderr' '\n--- >8 ---\n\nThanks,\nTaylor\n"},{"id":"475225","messageId":"ZDb6gZycwTdsaB6o@nand.local","threadId":"59535","inReplyTo":"011b08f3b64f264e3abbe8b49ee5338c221badb9.1681294715.git.ps@pks.im","subject":"Re: [PATCH v2 2/8] repack: fix trying to use preferred pack in alternates","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-12T18:37:53Z","receivedAt":"2023-04-12T18:39:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 12, 2023 at 12:22:35PM +0200, Patrick Steinhardt wrote:\n> When doing a geometric repack with multi-pack-indices, then we ask\n> git-multi-pack-index(1) to use the largest packfile as the preferred\n> pack. It can happen though that the largest packfile is not part of the\n> main object database, but instead part of an alternate object database.\n> The result is that git-multi-pack-index(1) will not be able to find the\n> preferred pack and print a warning. It then falls back to use the first\n> packfile that the multi-pack-index shall reference.\n>\n> Fix this bug by only considering packfiles as preferred pack that are\n> local. This is the right thing to do given that a multi-pack-index\n> should never reference packfiles borrowed from an alternate.\n>\n> While at it, rename the function `get_largest_active_packfile()` to\n> `get_preferred_pack()` to better document its intent.\n>\n> Signed-off-by: Patrick Steinhardt \n\n> @@ -464,7 +466,16 @@ static struct packed_git *get_largest_active_pack(struct pack_geometry *geometry\n> \t}\n> \tif (geometry->split == geometry->pack_nr)\n> \t\treturn NULL;\n> -\treturn geometry->pack[geometry->pack_nr - 1];\n> +\n> +\tfor (i = geometry->pack_nr; i > 0; i--)\n> +\t\t/*\n> +\t\t * A pack that is not local would never be included in a\n> +\t\t * multi-pack index. We thus skip over any non-local packs.\n> +\t\t */\n> +\t\tif (geometry->pack[i - 1]->pack_local)\n> +\t\t\treturn geometry->pack[i - 1];\n> +\n> +\treturn NULL;\n> }\n\nLooking good, we want to go through this list in reverse order, since\nthe packs are ordered largest to smallest. I think that you could\nslightly rewrite the loop condition to avoid having to always access\n`geometry->pack` at `i-1` instead of `i`, like so:\n\n--- 8< ---\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 9d36dc8b84..ba468ac44e 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -467,13 +467,15 @@ static struct packed_git *get_preferred_pack(struct pack_geometry *geometry)\n \tif (geometry->split == geometry->pack_nr)\n \t\treturn NULL;\n\n-\tfor (i = geometry->pack_nr; i > 0; i--)\n+\tfor (i = geometry->pack_nr - 1; i >= 0; i--) {\n \t\t/*\n \t\t * A pack that is not local would never be included in a\n \t\t * multi-pack index. We thus skip over any non-local packs.\n \t\t */\n-\t\tif (geometry->pack[i - 1]->pack_local)\n-\t\t\treturn geometry->pack[i - 1];\n+\t\tstruct packed_git *p = geometry->pack[i];\n+\t\tif (p->pack_local)\n+\t\t\treturn p;\n+\t}\n\n \treturn NULL;\n }\n--- >8 ---\n\nbut I'm not sure that the loop condition is quite right to begin with.\nWe don't want to iterate all the way down to the beginning of the pack\nlist, since some of those packs may be below the \"split\" line, IOW their\ncontents are going to be rolled up and the packs destroyed.\n\nSo I think the right loop condition would be:\n\n for (i = geometry->pack_nr - 1; i >= geometry->split; i--)\n\nwhich I think makes the \"if (geometry->split == geometry->pack_nr)\"\ncondition above redundant with this loop, so you could probably drop\nthat.\n\nI would definitely appreciate a second set of eye here. The pack *at*\nthe split line is considered frozen (IOW, we create a new pack\nconsisting of the packs in range [0, geometry->split), and leave the\npacks in range [geometry->split, geometry->nr) alone).\n\nSo it should be fine to enter that loop when \"i == geometry->split\",\nbecause it's OK to return that as a potential preferred pack.\n\n> +test_expect_success '--geometric --write-midx with packfiles in main and alternate ODB' '\n> +\ttest_when_finished \"rm -fr shared member\" &&\n\nLooks familiar ;-).\n\n> +\t# Create a shared repository that will serve as the alternate object\n> +\t# database for the member linked to it. It has got some objects on its\n> +\t# own that are packed into a single packfile.\n> +\tgit init shared &&\n> +\ttest_commit -C shared common-object &&\n> +\tgit -C shared repack -ad &&\n> +\n> +\t# We create member so that its alternates file points to the shared\n> +\t# repository. We then create a commit in it so that git-repack(1) has\n> +\t# something to repack.\n> +\t# of the shared object database.\n> +\tgit clone --shared shared member &&\n> +\ttest_commit -C member unique-object &&\n> +\tgit -C member repack --geometric=2 --write-midx 2>err &&\n> +\ttest_must_be_empty err &&\n> +\n> +\t# We should see that a new packfile was generated.\n> +\tfind shared/.git/objects/pack -type f -name \"*.pack\" | sort >packs &&\n\nDo we need a sort here? I don't think we rely on the order anywhere, and\nit looks like we only care that there is a single entry.\n\nIf that's the case, I think we could replace the wc invocation here\nwith:\n\n test_line_count = 1 packs\n\n> +\ttest $(wc -l +\n> +\t# We should also see a multi-pack-index. This multi-pack-index should\n> +\t# never refer to any packfiles in the alternate object database.\n> +\t# Consequentially, it should be valid even with the alternate ODB\n> +\t# deleted.\n> +\trm -r shared &&\n> +\tgit -C member multi-pack-index verify\n\nTo test this even more directly, I think that you could ensure that the\nset of packs (which should just be the single entry found in \"packs\"\nabove) matches what we expect. That should look something like:\n\n test-tool read-midx member/.git/objects >packs.midx &&\n grep \"^pack-.*\\.idx$\" packs.midx | sort >actual &&\n xargs -n 1 basename expect\n test_cmp expect actual\n\nNote that the read-midx test helper outputs packs with the \"*.idx\"\nsuffix, so you'd probably want to alter your find invocation a few lines\nabove accordingly, or rewrite it before writing into \"actual\".\n\nAlternatively, since we expect there to be just a single pack here, I\nthink we could just as easily write something like:\n\n test-tool read-midx member/.git/objects >packs.midx &&\n grep \"^pack-.*\\.idx$\" packs.midx >actual &&\n test_line_count = 1 actual &&\n grep $(cat actual) packs\n\nthough I slightly prefer the former since it is less brittle to changing\nthis test. Either way, though.\n\nThanks,\nTaylor\n"},{"id":"475236","messageId":"ZDcW7LjZkyeGjEsV@nand.local","threadId":"59535","inReplyTo":"f82e44f1da47890b7ef6ee2f5c1cdbbe20fa6684.1681294715.git.ps@pks.im","subject":"Re: [PATCH v2 3/8] repack: fix generating multi-pack-index with only non-local packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-12T20:39:08Z","receivedAt":"2023-04-12T20:39:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 12, 2023 at 12:22:39PM +0200, Patrick Steinhardt wrote:\n> When writing the multi-pack-index with geometric repacking we will add\n> all packfiles to the index that are part of the geometric sequence. This\n> can potentially also include packfiles borrowed from an alternate object\n> directory. But given that a multi-pack-index can only ever include packs\n> that are part of the main object database this does not make much sense\n> whatsoever.\n>\n> In the edge case where all packfiles are contained in the alternate\n> object database and the local repository has none itself this bug can\n> cause us to invoke git-multi-pack-index(1) with only non-local packfiles\n> that it ultimately cannot find. This causes it to return an error and\n> thus causes the geometric repack to fail.\n>\n> Fix the code to skip non-local packfiles.\n>\n> Co-authored-by: Taylor Blau \n\nThanks for listing me as a co-author. Doing so probably requires my\nS-o-b, which you are more than free to forge here.\n\n> +\t# Assert that we wrote neither a new packfile nor a multi-pack-index.\n> +\t# We should not have a packfile because the single packfile in the\n> +\t# alternate object database does not invalidate the geometric sequence.\n> +\t# And we should not have a multi-pack-index because these only index\n> +\t# local packfiles, and there are none.\n> +\tfind member/.git/objects/pack -type f >actual &&\n> +\ttest_must_be_empty actual\n\nThis test looks good, though one small opportunity for cleanup might be\nto replace this with a single:\n\n test_dir_is_empty member/$packdir\n\nThanks,\nTaylor\n"},{"id":"475242","messageId":"ZDcjshQeoQ7e8TbB@nand.local","threadId":"59535","inReplyTo":"e7d30fd22c65b33defb944bd043a56d0c525f875.1681294715.git.ps@pks.im","subject":"Re: [PATCH v2 4/8] pack-objects: fix error when packing same pack twice","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-12T21:33:38Z","receivedAt":"2023-04-12T21:33:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 12, 2023 at 12:22:44PM +0200, Patrick Steinhardt wrote:\n> When passed the same packfile twice via `--stdin-packs` we return an\n> error that the packfile supposedly was not found. This is because when\n> reading packs into the list of included or excluded packfiles, we will\n> happily re-add packfiles even if they are part of the lists already. And\n> while the list can now contain duplicates, we will only set the `util`\n> pointer of the first list entry to the `packed_git` structure. We notice\n> that at a later point when checking that all list entries have their\n> `util` pointer set and die with an error.\n\nWell explained.\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/pack-objects.c | 2 ++\n> t/t5331-pack-objects-stdin.sh | 29 +++++++++++++++++++++++++++++\n> t/t7703-repack-geometric.sh | 25 +++++++++++++++++++++++++\n> 3 files changed, 56 insertions(+)\n> create mode 100755 t/t5331-pack-objects-stdin.sh\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 77d88f85b0..fdf3f440be 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -3359,7 +3359,9 @@ static void read_packs_list_from_stdin(void)\n> \t}\n>\n> \tstring_list_sort(&include_packs);\n> +\tstring_list_remove_duplicates(&include_packs, 0);\n> \tstring_list_sort(&exclude_packs);\n> +\tstring_list_remove_duplicates(&exclude_packs, 0);\n\nI for some reason thought that string_list_remove_duplicates() sorted\nits first argument as a side-effect, but double checking on it, that\nisn't the case. So I agree that calling string_list_remove_duplicates()\nis the right thing to do here.\n\n> diff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\n> new file mode 100755\n> index 0000000000..ab34cfc729\n> --- /dev/null\n> +++ b/t/t5331-pack-objects-stdin.sh\n\nCool, I am glad that we are moving some of these tests out of t5300. We\ncould probably go a little bit further, since there are a handful of\npurely `--stdin-packs`-related tests towards the bottom of that script\nthat could or should also be moved out.\n\nWe could also always do it later, but I wouldn't be sad to see an extra\ncommit before this one that introduces t5331 and moves the last six or\nso `--stdin-packs` tests out of t5300. If nothing else, it would be\nnice to be able to run the tests for just that feature without having to\nskip a bunch of other unrelated tests.\n\nLuckily, those tests don't depend on any of the earlier ones or setup\ntests, so they should be able to move with a pure cut and paste.\n\n> @@ -0,0 +1,29 @@\n> +#!/bin/sh\n> +\n> +test_description='pack-objects --stdin'\n> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> +\n> +TEST_PASSES_SANITIZE_LEAK=true\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'pack-objects --stdin with duplicate packfile' '\n> +\ttest_when_finished \"rm -fr repo\" &&\n> +\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\ttest_commit \"commit\" &&\n> +\t\tgit repack -ad &&\n> +\n> +\t\t(\n> +\t\t\tbasename .git/objects/pack/pack-*.pack &&\n> +\t\t\tbasename .git/objects/pack/pack-*.pack\n> +\t\t) >packfiles &&\n> +\n> +\t\tgit pack-objects --stdin-packs generated-pack +\t\ttest_cmp generated-pack-*.pack .git/objects/pack/pack-*.pack\n\nI think that in practice that test_cmp'ing these two together will be\njust fine, but if you wanted to be extra careful, you could grab their\nobject lists and compare those two together. That would allow you to\ncontinue passing this test without needing to worry about if you\ngenerated the same logical content with slightly different on-disk\ncontent.\n\nThanks,\nTaylor\n"},{"id":"475245","messageId":"ZDcoKN1rEzqxZAe2@nand.local","threadId":"59535","inReplyTo":"9b278a4c91a5631d1b7b11bf56ab560c6bb58645.1681294715.git.ps@pks.im","subject":"Re: [PATCH v2 5/8] pack-objects: fix error when same packfile is included and excluded","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-12T21:52:40Z","receivedAt":"2023-04-12T21:52:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 12, 2023 at 12:22:48PM +0200, Patrick Steinhardt wrote:\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index fdf3f440be..522eb4dd31 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -3366,11 +3366,9 @@ static void read_packs_list_from_stdin(void)\n> \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n> \t\tconst char *pack_name = pack_basename(p);\n>\n> -\t\titem = string_list_lookup(&include_packs, pack_name);\n> -\t\tif (!item)\n> -\t\t\titem = string_list_lookup(&exclude_packs, pack_name);\n> -\n> -\t\tif (item)\n> +\t\tif ((item = string_list_lookup(&include_packs, pack_name)))\n> +\t\t\titem->util = p;\n> +\t\tif ((item = string_list_lookup(&exclude_packs, pack_name)))\n> \t\t\titem->util = p;\n\nOof. I was hoping that we could avoid having to look through both\nlists. But that relies on us having disjoint sets of packs in the\ninclude and exclude lists.\n\nWe probably *could* just ban this combination outright, but that would\nalso involve some work to try and detect that case. So I think that\ndoing this (and ensuring that the resulting pack is empty, that is that\nthe exclude set takes precedence here) is the right thing to do.\n\nThis (and the elided test below) look great to me.\n\nThanks,\nTaylor\n"},{"id":"475247","messageId":"ZDcqIjSUMW4sKNXE@nand.local","threadId":"59535","inReplyTo":"b74d0a037b07706d07fad7b438fa0cb211726575.1681294715.git.ps@pks.im","subject":"Re: [PATCH v2 8/8] repack: disable writing bitmaps when doing a local geometric repack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-12T22:01:06Z","receivedAt":"2023-04-12T22:01:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 12, 2023 at 12:23:01PM +0200, Patrick Steinhardt wrote:\n> Now there are two different ways to fix this. The first one would be to\n> amend git-multi-pack-index(1) to disable writing bitmaps when we notice\n> that we don't have full object coverage. But we ain't really got enough\n> information there, and seeing that it is a low-level plumbing command it\n> does not feel like the right place to fix this.\n\nI might actually advocate that we either fix this in both places, or fix\nit at the lower level only. I think that we would still be able to\ntrigger this problem by invoking `git multi-pack-index write\n--bitmap --stdin-packs` directly.\n\n> ---\n> builtin/repack.c | 20 ++++++++++++++++++++\n> t/t7703-repack-geometric.sh | 27 +++++++++++++++++++++++++++\n> 2 files changed, 47 insertions(+)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index f57869f14a..07d92fdf87 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -881,6 +881,26 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \tif (pack_kept_objects < 0)\n> \t\tpack_kept_objects = write_bitmaps > 0 && !write_midx;\n>\n> +\tif (write_midx && write_bitmaps && geometric_factor && po_args.local) {\n> +\t\tstruct packed_git *p;\n> +\n> +\t\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n> +\t\t\tif (p->pack_local)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\t/*\n> +\t\t\t * When asked to do a local repack, but we have\n> +\t\t\t * packfiles that are inherited from an alternate, then\n> +\t\t\t * we cannot guarantee that the multi-pack-index would\n> +\t\t\t * have full coverage of all objects. We thus disable\n> +\t\t\t * writing bitmaps in that case.\n> +\t\t\t */\n> +\t\t\twarning(_(\"disabling bitmap writing, as some objects are not being packed\"));\n> +\t\t\twrite_bitmaps = 0;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n\nIn terms of the higher-level fix here, though, I think that you could\nreasonably assume that the alternate repository has at least one pack,\nand that the combination of \"write_midx && write_bitmaps &&\npo.args_local\" and \"has any alternate(s)\" is banned (or, at least, emits\nthe above warning and disables writing bitmaps).\n\nBut certainly ensuring that there are indeed packs in at least one of\nthe alternate(s) doesn't hurt either, so I don't mind this approach at\nall.\n\nOne thing that I don't quite follow with this logic is why we need to\nhave geometric_factor set. You could (somewhat unreasonably) write a\nMIDX containing a single pack (git repack -[A|a] --write-midx\n--write-bitmap-index), or a MIDX containing just the new pack along with\nall of the existing (local) packs, (git repack --write-midx\n--write-bitmap-index).\n\nSo I think we'd want to drop the geometric_factor from the above\nconditional. (And in the future, I think we typically refer to whether\nor not the \"geometry\" pointer is NULL or not to indicate whether or not\nwe are doing a geometric repack, though the diff context doesn't give me\nenough to know whether we have even attempted to set up that instance\nyet, so this is fine, too).\n\nThanks,\nTaylor\n"},{"id":"475248","messageId":"ZDcqYn7urrkwzX+z@nand.local","threadId":"59535","inReplyTo":"cover.1681294715.git.ps@pks.im","subject":"Re: [PATCH v2 0/8] repack: fix geometric repacking with gitalternates","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-12T22:02:10Z","receivedAt":"2023-04-12T22:02:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Apr 12, 2023 at 12:22:27PM +0200, Patrick Steinhardt wrote:\n> Patrick Steinhardt (8):\n> midx: fix segfault with no packs and invalid preferred pack\n> repack: fix trying to use preferred pack in alternates\n> repack: fix generating multi-pack-index with only non-local packs\n> pack-objects: fix error when packing same pack twice\n> pack-objects: fix error when same packfile is included and excluded\n> pack-objects: extend test coverage of `--stdin-packs` with alternates\n> repack: honor `-l` when calculating pack geometry\n> repack: disable writing bitmaps when doing a local geometric repack\n\nThis is looking like it's on a good track. I gave this a fairly thorough\nreview, and left a handful of comments ranging from \"you could do it\nthis way, or not\", to \"we should probably adjust this before merging\".\n\nLet me know if you have any questions, and I'll be more than happy to\nreview a v2 ;-).\n\nThanks,\nTaylor\n"},{"id":"475259","messageId":"xmqqr0sozmc2.fsf@gitster.g","threadId":"59535","inReplyTo":"608dde4ad52c28ef42845b6bfdcb168e252bd29b.1681294715.git.ps@pks.im","subject":"Re: [PATCH v2 7/8] repack: honor `-l` when calculating pack geometry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-12T23:56:29Z","receivedAt":"2023-04-12T23:56:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> +\t# Note that we are using stat(1) to verify idempotence to also verify\n> +\t# that the mtime did not change. This is done in order to detect the\n> +\t# case where we do repack objects, but the resulting packfile is the\n> +\t# same.\n> +\tstat member/.git/objects/pack/* >expected-member-packs &&\n> +\tgit -C member repack --geometric=2 -l -d &&\n> +\tstat member/.git/objects/pack/* >actual-member-packs &&\n\nThese tests that use stat(1) are very likely to be non-portable.\n\nWorse yet, even when stat(1) is available, when running\n\n $ cd t && sh t7703-repack-geometric.sh --root=/dev/shm/testpen -i -v\n\nin my environment (based on Debian), the lists fail to match at\nsubseconds.\n\n--- expected-member-packs 2023-04-12 23:48:52.817035290 +0000\n+++ actual-member-packs 2023-04-12 23:48:52.833036370 +0000\n@@ -2,7 +2,7 @@\n Size: 1156 Blocks: 8 IO Block: 4096 regular file\n Device: 0,25 Inode: 2330515 Links: 1\n Access: (0400/-r--------) Uid: (1001/ gitster) Gid: (1001/gitster)\n-Access: 2023-04-12 23:48:52.801034210 +0000\n+Access: 2023-04-12 23:48:52.829036100 +0000\n Modify: 2023-04-12 23:48:52.773032320 +0000\n Change: 2023-04-12 23:48:52.773032320 +0000\n Birth: 2023-04-12 23:48:52.773032320 +0000\n@@ -10,7 +10,7 @@\n Size: 276 Blocks: 8 IO Block: 4096 regular file\n Device: 0,25 Inode: 2330514 Links: 1\n Access: (0400/-r--------) Uid: (1001/ gitster) Gid: (1001/gitster)\n-Access: 2023-04-12 23:48:52.781032860 +0000\n+Access: 2023-04-12 23:48:52.829036100 +0000\n Modify: 2023-04-12 23:48:52.773032320 +0000\n Change: 2023-04-12 23:48:52.773032320 +0000\n Birth: 2023-04-12 23:48:52.769032050 +0000\n\n"},{"id":"475264","messageId":"xmqqmt3cz7rp.fsf@gitster.g","threadId":"59535","inReplyTo":"xmqqr0sozmc2.fsf@gitster.g","subject":"Re: [PATCH v2 7/8] repack: honor `-l` when calculating pack geometry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-13T05:11:06Z","receivedAt":"2023-04-13T05:11:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Worse yet, even when stat(1) is available, when running\n>\n> $ cd t && sh t7703-repack-geometric.sh --root=/dev/shm/testpen -i -v\n>\n> in my environment (based on Debian), the lists fail to match at\n> subseconds.\n> ...\n\nA CI run of 'seen' with this topic fails on macOS:\n\n https://github.com/git/git/actions/runs/4683726291/jobs/8299125722#step:4:2090\n\nAlmost the same 'seen' without this topic is CI clean:\n\n https://github.com/git/git/actions/runs/4683470969\n\n"},{"id":"475265","messageId":"ZDekFNg8eXr66BHl@ncase","threadId":"59535","inReplyTo":"xmqqmt3cz7rp.fsf@gitster.g","subject":"Re: [PATCH v2 7/8] repack: honor `-l` when calculating pack geometry","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-13T06:41:24Z","receivedAt":"2023-04-13T06:41:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 12, 2023 at 10:11:06PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Worse yet, even when stat(1) is available, when running\n> >\n> > $ cd t && sh t7703-repack-geometric.sh --root=/dev/shm/testpen -i -v\n> >\n> > in my environment (based on Debian), the lists fail to match at\n> > subseconds.\n> > ...\n> \n> A CI run of 'seen' with this topic fails on macOS:\n> \n> https://github.com/git/git/actions/runs/4683726291/jobs/8299125722#step:4:2090\n> \n> Almost the same 'seen' without this topic is CI clean:\n> \n> https://github.com/git/git/actions/runs/4683470969\n> \n\nThanks, I'll come up with an alternative in v3 of this patch series.\n\nPatrick\n"},{"id":"475268","messageId":"ZDfLMm-AoXkt-4zj@ncase","threadId":"59535","inReplyTo":"ZDbw6v8r89zciE3q@nand.local","subject":"Re: [PATCH v2 1/8] midx: fix segfault with no packs and invalid preferred pack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-13T09:28:18Z","receivedAt":"2023-04-13T09:28:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 12, 2023 at 01:56:58PM -0400, Taylor Blau wrote:\n> On Wed, Apr 12, 2023 at 12:22:31PM +0200, Patrick Steinhardt wrote:\n> > Fix this bug by exiting early in case we have determined that the MIDX\n> > wouldn't have any packfiles to index. While the request itself does not\n> > make much sense anyway, it is still preferable to exit gracefully than\n> > to abort.\n> \n> Interesting. This reminded me quite a bit of eb57277ba3 (midx: prevent\n> writing a .bitmap without any objects, 2022-02-09) which tackled a\n> similar problem of trying to write a MIDX bitmap without any objects.\n> \n> We may want to consider moving that conditional further up, since this\n> makes the conditional added in eb57277ba3 dead code AFAICT. Here's a\n> patch on top of this one that I think would do the trick.\n> \n> It has the added benefit of sticking a:\n> \n> warning: unknown preferred pack: 'does-not-exist'\n> \n> in the output before dying, which might be nice (though I doubt anybody\n> will ever see it ;-)). The main difference is that we unset the bitmap\n> related bits from `flags`, which avoids us trying to compute a preferred\n> pack in the first place.\n> \n> For it to work, though, we need to make sure that ctx.preferred_pack_idx\n> is set to -1, and not zero-initialized, since we'll segfault otherwise\n> when trying to read into an empty array.\n\nIndeed, that is a good point. I think we can simplify your patch even\nfurther in that case:\n\ndiff --git a/midx.c b/midx.c\nindex 47989f7ea7..67eb617591 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1328,17 +1328,17 @@ static int write_midx_internal(const char *object_dir,\n }\n\n if (preferred_pack_name) {\n- int found = 0;\n+ ctx.preferred_pack_idx = -1;\n+\n for (i = 0; i < ctx.nr; i++) {\n if (!cmp_idx_or_pack_name(preferred_pack_name,\n ctx.info[i].pack_name)) {\n ctx.preferred_pack_idx = i;\n- found = 1;\n break;\n }\n }\n\n- if (!found)\n+ if (ctx.preferred_pack_idx == -1)\n warning(_(\"unknown preferred pack: '%s'\"),\n preferred_pack_name);\n } else if (ctx.nr &&\n\nThe other cases already set `preferred_pack_idx = -1`, so this is really\nall we need to do to fix the segfault.\n\nPatrick\n"},{"id":"475269","messageId":"ZDfL_6G-ySm94Vla@ncase","threadId":"59535","inReplyTo":"ZDb6gZycwTdsaB6o@nand.local","subject":"Re: [PATCH v2 2/8] repack: fix trying to use preferred pack in alternates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-13T09:31:43Z","receivedAt":"2023-04-13T09:31:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 12, 2023 at 02:37:53PM -0400, Taylor Blau wrote:\n> On Wed, Apr 12, 2023 at 12:22:35PM +0200, Patrick Steinhardt wrote:\n> > When doing a geometric repack with multi-pack-indices, then we ask\n> > git-multi-pack-index(1) to use the largest packfile as the preferred\n> > pack. It can happen though that the largest packfile is not part of the\n> > main object database, but instead part of an alternate object database.\n> > The result is that git-multi-pack-index(1) will not be able to find the\n> > preferred pack and print a warning. It then falls back to use the first\n> > packfile that the multi-pack-index shall reference.\n> >\n> > Fix this bug by only considering packfiles as preferred pack that are\n> > local. This is the right thing to do given that a multi-pack-index\n> > should never reference packfiles borrowed from an alternate.\n> >\n> > While at it, rename the function `get_largest_active_packfile()` to\n> > `get_preferred_pack()` to better document its intent.\n> >\n> > Signed-off-by: Patrick Steinhardt \n> \n> > @@ -464,7 +466,16 @@ static struct packed_git *get_largest_active_pack(struct pack_geometry *geometry\n> > \t}\n> > \tif (geometry->split == geometry->pack_nr)\n> > \t\treturn NULL;\n> > -\treturn geometry->pack[geometry->pack_nr - 1];\n> > +\n> > +\tfor (i = geometry->pack_nr; i > 0; i--)\n> > +\t\t/*\n> > +\t\t * A pack that is not local would never be included in a\n> > +\t\t * multi-pack index. We thus skip over any non-local packs.\n> > +\t\t */\n> > +\t\tif (geometry->pack[i - 1]->pack_local)\n> > +\t\t\treturn geometry->pack[i - 1];\n> > +\n> > +\treturn NULL;\n> > }\n> \n> Looking good, we want to go through this list in reverse order, since\n> the packs are ordered largest to smallest. I think that you could\n> slightly rewrite the loop condition to avoid having to always access\n> `geometry->pack` at `i-1` instead of `i`, like so:\n> \n> --- 8< ---\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 9d36dc8b84..ba468ac44e 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -467,13 +467,15 @@ static struct packed_git *get_preferred_pack(struct pack_geometry *geometry)\n> \tif (geometry->split == geometry->pack_nr)\n> \t\treturn NULL;\n> \n> -\tfor (i = geometry->pack_nr; i > 0; i--)\n> +\tfor (i = geometry->pack_nr - 1; i >= 0; i--) {\n> \t\t/*\n> \t\t * A pack that is not local would never be included in a\n> \t\t * multi-pack index. We thus skip over any non-local packs.\n> \t\t */\n> -\t\tif (geometry->pack[i - 1]->pack_local)\n> -\t\t\treturn geometry->pack[i - 1];\n> +\t\tstruct packed_git *p = geometry->pack[i];\n> +\t\tif (p->pack_local)\n> +\t\t\treturn p;\n> +\t}\n> \n> \treturn NULL;\n> }\n> --- >8 ---\n\nThere's a gotcha: `i` is an `unit32_t`, so `i >= 0` would always be true\nand thus we wrap around and would try to access the pack array at an\nout-of-bound index.\n\n> but I'm not sure that the loop condition is quite right to begin with.\n> We don't want to iterate all the way down to the beginning of the pack\n> list, since some of those packs may be below the \"split\" line, IOW their\n> contents are going to be rolled up and the packs destroyed.\n> \n> So I think the right loop condition would be:\n> \n> for (i = geometry->pack_nr - 1; i >= geometry->split; i--)\n> \n> which I think makes the \"if (geometry->split == geometry->pack_nr)\"\n> condition above redundant with this loop, so you could probably drop\n> that.\n> \n> I would definitely appreciate a second set of eye here. The pack *at*\n> the split line is considered frozen (IOW, we create a new pack\n> consisting of the packs in range [0, geometry->split), and leave the\n> packs in range [geometry->split, geometry->nr) alone).\n> \n> So it should be fine to enter that loop when \"i == geometry->split\",\n> because it's OK to return that as a potential preferred pack.\n\nThat makes sense indeed. Will amend.\n\n[snip]\n> > +\ttest $(wc -l > +\n> > +\t# We should also see a multi-pack-index. This multi-pack-index should\n> > +\t# never refer to any packfiles in the alternate object database.\n> > +\t# Consequentially, it should be valid even with the alternate ODB\n> > +\t# deleted.\n> > +\trm -r shared &&\n> > +\tgit -C member multi-pack-index verify\n> \n> To test this even more directly, I think that you could ensure that the\n> set of packs (which should just be the single entry found in \"packs\"\n> above) matches what we expect. That should look something like:\n> \n> test-tool read-midx member/.git/objects >packs.midx &&\n> grep \"^pack-.*\\.idx$\" packs.midx | sort >actual &&\n> xargs -n 1 basename expect\n> test_cmp expect actual\n> \n> Note that the read-midx test helper outputs packs with the \"*.idx\"\n> suffix, so you'd probably want to alter your find invocation a few lines\n> above accordingly, or rewrite it before writing into \"actual\".\n> \n> Alternatively, since we expect there to be just a single pack here, I\n> think we could just as easily write something like:\n> \n> test-tool read-midx member/.git/objects >packs.midx &&\n> grep \"^pack-.*\\.idx$\" packs.midx >actual &&\n> test_line_count = 1 actual &&\n> grep $(cat actual) packs\n> \n> though I slightly prefer the former since it is less brittle to changing\n> this test. Either way, though.\n\nThanks, I've adopted that approach with a slight modification.\n\nPatrick\n"},{"id":"475271","messageId":"ZDfRb2PI-QJDavzm@ncase","threadId":"59535","inReplyTo":"ZDcqIjSUMW4sKNXE@nand.local","subject":"Re: [PATCH v2 8/8] repack: disable writing bitmaps when doing a local geometric repack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-13T09:54:55Z","receivedAt":"2023-04-13T09:55:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 12, 2023 at 06:01:06PM -0400, Taylor Blau wrote:\n> On Wed, Apr 12, 2023 at 12:23:01PM +0200, Patrick Steinhardt wrote:\n> > Now there are two different ways to fix this. The first one would be to\n> > amend git-multi-pack-index(1) to disable writing bitmaps when we notice\n> > that we don't have full object coverage. But we ain't really got enough\n> > information there, and seeing that it is a low-level plumbing command it\n> > does not feel like the right place to fix this.\n> \n> I might actually advocate that we either fix this in both places, or fix\n> it at the lower level only. I think that we would still be able to\n> trigger this problem by invoking `git multi-pack-index write\n> --bitmap --stdin-packs` directly.\n\nThe problem I see with implementing the fix is that we're just not in a\ngood position to judge whether we have full coverage of objects or not.\nAll we see is a set of packfiles, and those packfiles _could_ have full\ncoverage, but they may just as well not have full coverage. And whether\nthey do is not easy to figure out in git-multi-pack-index(1).\n\nSo in order to fix this we'd likely have to use heuristics, like whether\nor not there are alternates or alternate packfiles. But unconditionally\ndisabling bitmaps when there are feels overly restrictive to me as it\nwould break perfectly-valid usecases.\n\nI'm thus still not convinced we should implement it at the lowest level\npossible. While it would be nice to deduplicate the logic around this,\nI wouldn't want to close doors we don't necessarily have to.\n\n> > ---\n> > builtin/repack.c | 20 ++++++++++++++++++++\n> > t/t7703-repack-geometric.sh | 27 +++++++++++++++++++++++++++\n> > 2 files changed, 47 insertions(+)\n> >\n> > diff --git a/builtin/repack.c b/builtin/repack.c\n> > index f57869f14a..07d92fdf87 100644\n> > --- a/builtin/repack.c\n> > +++ b/builtin/repack.c\n> > @@ -881,6 +881,26 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> > \tif (pack_kept_objects < 0)\n> > \t\tpack_kept_objects = write_bitmaps > 0 && !write_midx;\n> >\n> > +\tif (write_midx && write_bitmaps && geometric_factor && po_args.local) {\n> > +\t\tstruct packed_git *p;\n> > +\n> > +\t\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n> > +\t\t\tif (p->pack_local)\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * When asked to do a local repack, but we have\n> > +\t\t\t * packfiles that are inherited from an alternate, then\n> > +\t\t\t * we cannot guarantee that the multi-pack-index would\n> > +\t\t\t * have full coverage of all objects. We thus disable\n> > +\t\t\t * writing bitmaps in that case.\n> > +\t\t\t */\n> > +\t\t\twarning(_(\"disabling bitmap writing, as some objects are not being packed\"));\n> > +\t\t\twrite_bitmaps = 0;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> \n> In terms of the higher-level fix here, though, I think that you could\n> reasonably assume that the alternate repository has at least one pack,\n> and that the combination of \"write_midx && write_bitmaps &&\n> po.args_local\" and \"has any alternate(s)\" is banned (or, at least, emits\n> the above warning and disables writing bitmaps).\n> \n> But certainly ensuring that there are indeed packs in at least one of\n> the alternate(s) doesn't hurt either, so I don't mind this approach at\n> all.\n\nIt's an edge case for sure. I don't quite mind which way we go either.\nFor now I'll just keep the current way of doing things, but am happy to\nchange it.\n\n> One thing that I don't quite follow with this logic is why we need to\n> have geometric_factor set. You could (somewhat unreasonably) write a\n> MIDX containing a single pack (git repack -[A|a] --write-midx\n> --write-bitmap-index), or a MIDX containing just the new pack along with\n> all of the existing (local) packs, (git repack --write-midx\n> --write-bitmap-index).\n> \n> So I think we'd want to drop the geometric_factor from the above\n> conditional. (And in the future, I think we typically refer to whether\n> or not the \"geometry\" pointer is NULL or not to indicate whether or not\n> we are doing a geometric repack, though the diff context doesn't give me\n> enough to know whether we have even attempted to set up that instance\n> yet, so this is fine, too).\n\nMh. Yeah, I think you're right. I'll change it.\n\nPatrick\n"},{"id":"475272","messageId":"ZDfV9gm779EsU3A4@ncase","threadId":"59535","inReplyTo":"ZDfRb2PI-QJDavzm@ncase","subject":"Re: [PATCH v2 8/8] repack: disable writing bitmaps when doing a local geometric repack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-13T10:14:14Z","receivedAt":"2023-04-13T10:15:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Apr 13, 2023 at 11:54:55AM +0200, Patrick Steinhardt wrote:\n> On Wed, Apr 12, 2023 at 06:01:06PM -0400, Taylor Blau wrote:\n> > On Wed, Apr 12, 2023 at 12:23:01PM +0200, Patrick Steinhardt wrote:\n[snip]\n> > One thing that I don't quite follow with this logic is why we need to\n> > have geometric_factor set. You could (somewhat unreasonably) write a\n> > MIDX containing a single pack (git repack -[A|a] --write-midx\n> > --write-bitmap-index), or a MIDX containing just the new pack along with\n> > all of the existing (local) packs, (git repack --write-midx\n> > --write-bitmap-index).\n> > \n> > So I think we'd want to drop the geometric_factor from the above\n> > conditional. (And in the future, I think we typically refer to whether\n> > or not the \"geometry\" pointer is NULL or not to indicate whether or not\n> > we are doing a geometric repack, though the diff context doesn't give me\n> > enough to know whether we have even attempted to set up that instance\n> > yet, so this is fine, too).\n> \n> Mh. Yeah, I think you're right. I'll change it.\n\nActually, do we even have to care about the `write_midx` case? Right now\nwe have:\n\n```\nif (write_bitmaps && !(pack_everything & ALL_INTO_ONE) && !write_midx)\n die(_(incremental_bitmap_conflict_error));\n```\n\nThe intent is to die when not repacking all objects into a single pack\n(potentially with a cruft pack), but to allow this when writing a\nmulti-pack-index because they can have a bitmap that spans across\nmultiple packs.\n\nNow, whether or not we write a multi-pack-index, as soon as the user\npasses `-l` we cannot guarantee that we have all objects available\nlocally either in a single packfile nor in multiple packfiles when the\nrepository is connected to an alternate object directory. So in the\nspirit of the preexisting check, couldn't we do the following:\n\n```\nif (write_bitmaps && po_args.local && has_alternates(repo))\n die(_(\"Repacking local objects is incompatible with bitmap indexes.\");\n```\n\nSo in words, we die when the user asks us to write a bitmap index\nfor a repack that is supposed to only include local objects when there\nare objects that could have been inherited from an alternate object\ndirectory.\n\nI'm not sure whether we are okay with retroactively tightening checks\nthough. I'd argue it's likely fine, because it wouldn't have worked\nbefore this check either. And I'd rather fail with explicit reasons that\nare user-actionable rather than implicitly somewhere deep down in the\ncallstack.\n\nPatrick\n"},{"id":"475274","messageId":"dd8145beade440e5444130d3a3189b2c5d15a911.1681384405.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681384405.git.ps@pks.im","subject":"[PATCH v3 01/10] midx: fix segfault with no packs and invalid preferred pack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-13T11:16:06Z","receivedAt":"2023-04-13T11:16:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When asked to write a multi-pack-index the user can specify a preferred\npack that is used as a tie breaker when multiple packs contain the same\nobjects. When this packfile cannot be found, we just pick the first pack\nthat is getting tracked by the newly written multi-pack-index as a\nfallback.\n\nPicking the fallback can fail in the case where we're asked to write a\nmulti-pack-index with no packfiles at all: picking the fallback value\nwill cause a segfault as we blindly index into the array of packfiles,\nwhich would be empty.\n\nFix this bug by resetting the preferred packfile index to `-1` before\nsearching for the preferred pack. This fixes the segfault as we already\ncheck for whether the index is `> - 1`. If it is not, we simply don't\npick a preferred packfile at all.\n\nHelped-by: Taylor Blau \nSigned-off-by: Patrick Steinhardt \n---\n midx.c | 6 +++---\n t/t5319-multi-pack-index.sh | 12 ++++++++++++\n 2 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 47989f7ea7..67eb617591 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1328,17 +1328,17 @@ static int write_midx_internal(const char *object_dir,\n \t}\n \n \tif (preferred_pack_name) {\n-\t\tint found = 0;\n+\t\tctx.preferred_pack_idx = -1;\n+\n \t\tfor (i = 0; i < ctx.nr; i++) {\n \t\t\tif (!cmp_idx_or_pack_name(preferred_pack_name,\n \t\t\t\t\t\t ctx.info[i].pack_name)) {\n \t\t\t\tctx.preferred_pack_idx = i;\n-\t\t\t\tfound = 1;\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n \n-\t\tif (!found)\n+\t\tif (ctx.preferred_pack_idx == -1)\n \t\t\twarning(_(\"unknown preferred pack: '%s'\"),\n \t\t\t\tpreferred_pack_name);\n \t} else if (ctx.nr &&\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 499d5d4c78..0883c7c6bd 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -183,6 +183,18 @@ test_expect_success 'write midx with --stdin-packs' '\n \n compare_results_with_midx \"mixed mode (one pack + extra)\"\n \n+test_expect_success 'write with no objects and preferred pack' '\n+\ttest_when_finished \"rm -rf empty\" &&\n+\tgit init empty &&\n+\ttest_must_fail git -C empty multi-pack-index write \\\n+\t\t--stdin-packs --preferred-pack=does-not-exist err &&\n+\tcat >expect <<-EOF &&\n+\twarning: unknown preferred pack: ${SQ}does-not-exist${SQ}\n+\terror: no pack files to index.\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n test_expect_success 'write progress off for redirected stderr' '\n \tgit multi-pack-index --object-dir=$objdir write 2>err &&\n \ttest_line_count = 0 err\n-- \n2.40.0\n\n"},{"id":"475275","messageId":"cover.1681384405.git.ps@pks.im","threadId":"59535","inReplyTo":"a07ed50feeec4bfc3e9736bf493b9876896bcdd2.1680606445.git.ps@pks.im","subject":"[PATCH v3 00/10] repack: fix geometric repacking with gitalternates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-13T11:16:02Z","receivedAt":"2023-04-13T11:16:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the third version of my patch series to fix geometric repacking\nwith repositories that are connected to an alternate object database.\n\nChanges compared to v2:\n\n - I've simplified patch 1 to reset the preferred pack index instead\n of moving around some of the checks. This also causes us to print\n the warning for missing preferred packfiles again.\n\n - I've fixed the logic in patch 2 to find the preferred packfile to\n not return a packfile that would be rolled up in a geometric\n repack.\n\n - I've added an additional patch to split out preexisting tests for\n `--stdin-packs` into their own test file.\n\n - I've changed the unportable test added for geometric repacking\n with `-l` that used stat(1) to instead use our `test-tool chmtime`\n helper.\n\n - I've changed the logic that disables writing bitmaps in git-repack\n to cover more cases. It now always kicks in when doing a repack\n with `-l` that asks for bitmaps when connected to an alternate\n object directory.\n\n - In general, there's a bunch of small improvements left and right\n for the tests I'm adding.\n\nThanks for all the feedback so far.\n\nPatrick\n\nPatrick Steinhardt (10):\n midx: fix segfault with no packs and invalid preferred pack\n repack: fix trying to use preferred pack in alternates\n repack: fix generating multi-pack-index with only non-local packs\n pack-objects: split out `--stdin-packs` tests into separate file\n pack-objects: fix error when packing same pack twice\n pack-objects: fix error when same packfile is included and excluded\n pack-objects: extend test coverage of `--stdin-packs` with alternates\n t/helper: allow chmtime to print verbosely without modifying mtime\n repack: honor `-l` when calculating pack geometry\n repack: disable writing bitmaps when doing a local repack\n\n builtin/pack-objects.c | 10 +-\n builtin/repack.c | 62 ++++++++-\n midx.c | 6 +-\n object-file.c | 6 +\n object-store.h | 1 +\n t/helper/test-chmtime.c | 2 +-\n t/t5300-pack-object.sh | 135 -------------------\n t/t5319-multi-pack-index.sh | 12 ++\n t/t5331-pack-objects-stdin.sh | 240 ++++++++++++++++++++++++++++++++++\n t/t7700-repack.sh | 17 +++\n t/t7703-repack-geometric.sh | 165 +++++++++++++++++++++++\n 11 files changed, 505 insertions(+), 151 deletions(-)\n create mode 100755 t/t5331-pack-objects-stdin.sh\n\nRange-diff against v2:\n 1: 8c384aabde ! 1: dd8145bead midx: fix segfault with no packs and invalid preferred pack\n @@ Commit message\n will cause a segfault as we blindly index into the array of packfiles,\n which would be empty.\n \n - Fix this bug by exiting early in case we have determined that the MIDX\n - wouldn't have any packfiles to index. While the request itself does not\n - make much sense anyway, it is still preferable to exit gracefully than\n - to abort.\n + Fix this bug by resetting the preferred packfile index to `-1` before\n + searching for the preferred pack. This fixes the segfault as we already\n + check for whether the index is `> - 1`. If it is not, we simply don't\n + pick a preferred packfile at all.\n \n + Helped-by: Taylor Blau \n Signed-off-by: Patrick Steinhardt \n \n ## midx.c ##\n @@ midx.c: static int write_midx_internal(const char *object_dir,\n - \tfor_each_file_in_pack_dir(object_dir, add_pack_to_midx, &ctx);\n - \tstop_progress(&ctx.progress);\n + \t}\n \n -+\tif (!ctx.nr) {\n -+\t\terror(_(\"no pack files to index.\"));\n -+\t\tresult = 1;\n -+\t\tgoto cleanup;\n -+\t}\n + \tif (preferred_pack_name) {\n +-\t\tint found = 0;\n ++\t\tctx.preferred_pack_idx = -1;\n +\n - \tif ((ctx.m && ctx.nr == ctx.m->num_packs) &&\n - \t !(packs_to_include || packs_to_drop)) {\n - \t\tstruct bitmap_index *bitmap_git;\n + \t\tfor (i = 0; i < ctx.nr; i++) {\n + \t\t\tif (!cmp_idx_or_pack_name(preferred_pack_name,\n + \t\t\t\t\t\t ctx.info[i].pack_name)) {\n + \t\t\t\tctx.preferred_pack_idx = i;\n +-\t\t\t\tfound = 1;\n + \t\t\t\tbreak;\n + \t\t\t}\n + \t\t}\n + \n +-\t\tif (!found)\n ++\t\tif (ctx.preferred_pack_idx == -1)\n + \t\t\twarning(_(\"unknown preferred pack: '%s'\"),\n + \t\t\t\tpreferred_pack_name);\n + \t} else if (ctx.nr &&\n \n ## t/t5319-multi-pack-index.sh ##\n @@ t/t5319-multi-pack-index.sh: test_expect_success 'write midx with --stdin-packs' '\n @@ t/t5319-multi-pack-index.sh: test_expect_success 'write midx with --stdin-packs'\n +\ttest_must_fail git -C empty multi-pack-index write \\\n +\t\t--stdin-packs --preferred-pack=does-not-exist err &&\n +\tcat >expect <<-EOF &&\n ++\twarning: unknown preferred pack: ${SQ}does-not-exist${SQ}\n +\terror: no pack files to index.\n +\tEOF\n +\ttest_cmp expect err\n 2: 5a6c2ead87 ! 2: c5db1bc587 repack: fix trying to use preferred pack in alternates\n @@ Commit message\n While at it, rename the function `get_largest_active_packfile()` to\n `get_preferred_pack()` to better document its intent.\n \n + Helped-by: Taylor Blau \n Signed-off-by: Patrick Steinhardt \n \n ## builtin/repack.c ##\n @@ builtin/repack.c: static struct packed_git *get_largest_active_pack(struct pack_\n \t\treturn NULL;\n -\treturn geometry->pack[geometry->pack_nr - 1];\n +\n -+\tfor (i = geometry->pack_nr; i > 0; i--)\n ++\t/*\n ++\t * The preferred pack is the largest pack above the split line. In\n ++\t * other words, it is the largest pack that does not get rolled up in\n ++\t * the geometric repack.\n ++\t */\n ++\tfor (i = geometry->pack_nr; i > geometry->split; i--)\n +\t\t/*\n +\t\t * A pack that is not local would never be included in a\n +\t\t * multi-pack index. We thus skip over any non-local packs.\n @@ t/t7703-repack-geometric.sh: test_expect_success '--geometric with pack.packSize\n +\ttest_must_be_empty err &&\n +\n +\t# We should see that a new packfile was generated.\n -+\tfind shared/.git/objects/pack -type f -name \"*.pack\" | sort >packs &&\n -+\ttest $(wc -l packs &&\n ++\ttest_line_count = 1 packs &&\n +\n +\t# We should also see a multi-pack-index. This multi-pack-index should\n +\t# never refer to any packfiles in the alternate object database.\n -+\t# Consequentially, it should be valid even with the alternate ODB\n -+\t# deleted.\n -+\trm -r shared &&\n -+\tgit -C member multi-pack-index verify\n ++\ttest -f member/.git/objects/pack/multi-pack-index &&\n ++\ttest-tool read-midx member/.git/objects >packs.midx &&\n ++\tgrep \"^pack-.*\\.idx$\" packs.midx | sort >actual &&\n ++\tbasename member/.git/objects/pack/pack-*.idx >expect &&\n ++\ttest_cmp expect actual\n +'\n +\n test_done\n 3: f705cd6c49 ! 3: 8c3193268f repack: fix generating multi-pack-index with only non-local packs\n @@ Commit message\n Fix the code to skip non-local packfiles.\n \n Co-authored-by: Taylor Blau \n + Signed-off-by: Taylor Blau \n Signed-off-by: Patrick Steinhardt \n \n ## builtin/repack.c ##\n @@ builtin/repack.c: static void midx_included_packs(struct string_list *include,\n \n ## t/t7703-repack-geometric.sh ##\n @@ t/t7703-repack-geometric.sh: test_expect_success '--geometric --write-midx with packfiles in main and alterna\n - \tgit -C member multi-pack-index verify\n + \ttest_cmp expect actual\n '\n \n +test_expect_success '--geometric --with-midx with no local objects' '\n @@ t/t7703-repack-geometric.sh: test_expect_success '--geometric --write-midx with\n +\t# alternate object database does not invalidate the geometric sequence.\n +\t# And we should not have a multi-pack-index because these only index\n +\t# local packfiles, and there are none.\n -+\tfind member/.git/objects/pack -type f >actual &&\n -+\ttest_must_be_empty actual\n ++\ttest_dir_is_empty member/$packdir\n +'\n +\n test_done\n -: ---------- > 4: 8d47d753dc pack-objects: split out `--stdin-packs` tests into separate file\n 4: ff28829589 ! 5: ac528514e7 pack-objects: fix error when packing same pack twice\n @@ builtin/pack-objects.c: static void read_packs_list_from_stdin(void)\n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tconst char *pack_name = pack_basename(p);\n \n - ## t/t5331-pack-objects-stdin.sh (new) ##\n -@@\n -+#!/bin/sh\n -+\n -+test_description='pack-objects --stdin'\n -+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n -+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n -+\n -+TEST_PASSES_SANITIZE_LEAK=true\n -+. ./test-lib.sh\n + ## t/t5331-pack-objects-stdin.sh ##\n +@@ t/t5331-pack-objects-stdin.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n + TEST_PASSES_SANITIZE_LEAK=true\n + . ./test-lib.sh\n + \n ++packed_objects() {\n ++\tgit show-index <\"$1\" >tmp-object-list &&\n ++\tcut -d' ' -f2 tmp-object-list | sort &&\n ++\trm tmp-object-list\n ++ }\n +\n + test_expect_success 'setup for --stdin-packs tests' '\n + \tgit init stdin-packs &&\n + \t(\n +@@ t/t5331-pack-objects-stdin.sh: test_expect_success '--stdin-packs with broken links' '\n + \t)\n + '\n + \n +test_expect_success 'pack-objects --stdin with duplicate packfile' '\n +\ttest_when_finished \"rm -fr repo\" &&\n +\n @@ t/t5331-pack-objects-stdin.sh (new)\n +\t\ttest_commit \"commit\" &&\n +\t\tgit repack -ad &&\n +\n -+\t\t(\n ++\t\t{\n +\t\t\tbasename .git/objects/pack/pack-*.pack &&\n +\t\t\tbasename .git/objects/pack/pack-*.pack\n -+\t\t) >packfiles &&\n ++\t\t} >packfiles &&\n +\n +\t\tgit pack-objects --stdin-packs generated-pack expect &&\n ++\t\tpacked_objects generated-pack-*.idx >actual &&\n ++\t\ttest_cmp expect actual\n +\t)\n +'\n +\n -+test_done\n + test_done\n \n ## t/t7703-repack-geometric.sh ##\n @@ t/t7703-repack-geometric.sh: test_expect_success '--geometric --with-midx with no local objects' '\n - \ttest_must_be_empty actual\n + \ttest_dir_is_empty member/$packdir\n '\n \n +test_expect_success '--geometric with same pack in main and alternate ODB' '\n 5: 8011603d6d ! 6: afd7c7864d pack-objects: fix error when same packfile is included and excluded\n @@ builtin/pack-objects.c: static void read_packs_list_from_stdin(void)\n \n \n ## t/t5331-pack-objects-stdin.sh ##\n -@@ t/t5331-pack-objects-stdin.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n - TEST_PASSES_SANITIZE_LEAK=true\n - . ./test-lib.sh\n - \n -+packed_objects() {\n -+\tgit show-index <\"$1\" >tmp-object-list &&\n -+\tcut -d' ' -f2 tmp-object-list &&\n -+\trm tmp-object-list\n -+ }\n -+\n - test_expect_success 'pack-objects --stdin with duplicate packfile' '\n - \ttest_when_finished \"rm -fr repo\" &&\n - \n @@ t/t5331-pack-objects-stdin.sh: test_expect_success 'pack-objects --stdin with duplicate packfile' '\n \t)\n '\n @@ t/t5331-pack-objects-stdin.sh: test_expect_success 'pack-objects --stdin with du\n +\t\ttest_commit \"commit\" &&\n +\t\tgit repack -ad &&\n +\n -+\t\t(\n ++\t\t{\n +\t\t\tbasename .git/objects/pack/pack-*.pack &&\n +\t\t\tprintf \"^%s\\n\" \"$(basename .git/objects/pack/pack-*.pack)\"\n -+\t\t) >packfiles &&\n ++\t\t} >packfiles &&\n +\n +\t\tgit pack-objects --stdin-packs generated-pack packed-objects &&\n 6: 3e958bf2a4 ! 7: cd135439ae pack-objects: extend test coverage of `--stdin-packs` with alternates\n @@ t/t5331-pack-objects-stdin.sh: test_expect_success 'pack-objects --stdin with sa\n +\ttest_commit -C member \"local-commit\" &&\n +\tgit -C member repack -dl &&\n +\n -+\t(\n -+\t\tbasename shared/.git/objects/pack/pack-*.pack &&\n ++\t{\n ++\t\tbasename shared/.git/objects/pack/pack-*.pack &&\n +\t\tbasename member/.git/objects/pack/pack-*.pack\n -+\t) >packfiles &&\n ++\t} >packfiles &&\n +\n -+\t(\n ++\t{\n +\t\tpacked_objects shared/.git/objects/pack/pack-*.idx &&\n +\t\tpacked_objects member/.git/objects/pack/pack-*.idx\n -+\t) | sort >expected-objects &&\n ++\t} | sort >expected-objects &&\n +\n +\tgit -C member pack-objects --stdin-packs generated-pack actual-objects &&\n ++\tpacked_objects member/generated-pack-*.idx >actual-objects &&\n +\ttest_cmp expected-objects actual-objects\n +'\n +\n -: ---------- > 8: 18bac90289 t/helper: allow chmtime to print verbosely without modifying mtime\n 7: f014872ed4 ! 9: 285695deaf repack: honor `-l` when calculating pack geometry\n @@ t/t7703-repack-geometric.sh: test_expect_success '--geometric with same pack in\n +\t# packfile and thus arrive at the conclusion that the geometric\n +\t# sequence is intact. We thus expect no changes.\n +\t#\n -+\t# Note that we are using stat(1) to verify idempotence to also verify\n -+\t# that the mtime did not change. This is done in order to detect the\n -+\t# case where we do repack objects, but the resulting packfile is the\n -+\t# same.\n -+\tstat member/.git/objects/pack/* >expected-member-packs &&\n ++\t# Note that we are tweaking mtimes of the packfiles so that we can\n ++\t# verify they did not change. This is done in order to detect the case\n ++\t# where we do repack objects, but the resulting packfile is the same.\n ++\ttest-tool chmtime --verbose =0 member/.git/objects/pack/* >expected-member-packs &&\n +\tgit -C member repack --geometric=2 -l -d &&\n -+\tstat member/.git/objects/pack/* >actual-member-packs &&\n ++\ttest-tool chmtime --verbose member/.git/objects/pack/* >actual-member-packs &&\n +\ttest_cmp expected-member-packs actual-member-packs &&\n +\n -+\t(\n ++\t{\n +\t\tpacked_objects shared/.git/objects/pack/pack-*.idx &&\n +\t\tpacked_objects member/.git/objects/pack/pack-*.idx\n -+\t) | sort >expected-objects &&\n ++\t} | sort >expected-objects &&\n +\n +\t# On the other hand, when doing a non-local geometric repack we should\n +\t# see both packfiles and thus repack them. We expect that the shared\n +\t# object database was not changed.\n -+\tstat shared/.git/objects/pack/* >expected-shared-packs &&\n ++\ttest-tool chmtime --verbose =0 shared/.git/objects/pack/* >expected-shared-packs &&\n +\tgit -C member repack --geometric=2 -d &&\n -+\tstat shared/.git/objects/pack/* >actual-shared-packs &&\n ++\ttest-tool chmtime --verbose shared/.git/objects/pack/* >actual-shared-packs &&\n +\ttest_cmp expected-shared-packs actual-shared-packs &&\n +\n +\t# Furthermore, we expect that the member repository now has a single\n 8: b10ee7fc3d ! 10: bb0ee0eb3c repack: disable writing bitmaps when doing a local geometric repack\n @@ Metadata\n Author: Patrick Steinhardt \n \n ## Commit message ##\n - repack: disable writing bitmaps when doing a local geometric repack\n + repack: disable writing bitmaps when doing a local repack\n \n In order to write a bitmap, we need to have full coverage of all objects\n that are about to be packed. In the traditional non-multi-pack-index\n @@ Commit message\n with writing bitmaps when we have multiple packfiles as long as the\n multi-pack-index covers all objects.\n \n - This is not always the case though. When writing multi-pack-indices in a\n - repository that is connected to an alternate object directory we may end\n - up writing a multi-pack-index that only has partial coverage of objects.\n - The end result is that writing the bitmap will fail:\n + This is not always the case though. When asked to perform a repack of\n + local objects, only, then we cannot guarantee to have full coverage of\n + all objects regardless of whether we do a full repack or a repack with a\n + multi-pack-index. The end result is that writing the bitmap will fail in\n + both worlds:\n \n $ git multi-pack-index write --stdin-packs --bitmap \n \n ## builtin/repack.c ##\n @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)\n - \tif (pack_kept_objects < 0)\n - \t\tpack_kept_objects = write_bitmaps > 0 && !write_midx;\n - \n -+\tif (write_midx && write_bitmaps && geometric_factor && po_args.local) {\n -+\t\tstruct packed_git *p;\n -+\n -+\t\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n -+\t\t\tif (p->pack_local)\n -+\t\t\t\tcontinue;\n -+\n -+\t\t\t/*\n -+\t\t\t * When asked to do a local repack, but we have\n -+\t\t\t * packfiles that are inherited from an alternate, then\n -+\t\t\t * we cannot guarantee that the multi-pack-index would\n -+\t\t\t * have full coverage of all objects. We thus disable\n -+\t\t\t * writing bitmaps in that case.\n -+\t\t\t */\n -+\t\t\twarning(_(\"disabling bitmap writing, as some objects are not being packed\"));\n -+\t\t\twrite_bitmaps = 0;\n -+\t\t\tbreak;\n -+\t\t}\n -+\t}\n -+\n \tif (write_bitmaps && !(pack_everything & ALL_INTO_ONE) && !write_midx)\n \t\tdie(_(incremental_bitmap_conflict_error));\n \n ++\tif (write_bitmaps && po_args.local && has_alt_odb(the_repository)) {\n ++\t\t/*\n ++\t\t * When asked to do a local repack, but we have\n ++\t\t * packfiles that are inherited from an alternate, then\n ++\t\t * we cannot guarantee that the multi-pack-index would\n ++\t\t * have full coverage of all objects. We thus disable\n ++\t\t * writing bitmaps in that case.\n ++\t\t */\n ++\t\twarning(_(\"disabling bitmap writing, as some objects are not being packed\"));\n ++\t\twrite_bitmaps = 0;\n ++\t}\n ++\n + \tif (write_midx && write_bitmaps) {\n + \t\tstruct strbuf path = STRBUF_INIT;\n + \n +\n + ## object-file.c ##\n +@@ object-file.c: void prepare_alt_odb(struct repository *r)\n + \tr->objects->loaded_alternates = 1;\n + }\n + \n ++int has_alt_odb(struct repository *r)\n ++{\n ++\tprepare_alt_odb(r);\n ++\treturn !!r->objects->odb->next;\n ++}\n ++\n + /* Returns 1 if we have successfully freshened the file, 0 otherwise. */\n + static int freshen_file(const char *fn)\n + {\n +\n + ## object-store.h ##\n +@@ object-store.h: KHASH_INIT(odb_path_map, const char * /* key: odb_path */,\n + \tstruct object_directory *, 1, fspathhash, fspatheq)\n + \n + void prepare_alt_odb(struct repository *r);\n ++int has_alt_odb(struct repository *r);\n + char *compute_alternate_path(const char *path, struct strbuf *err);\n + struct object_directory *find_odb(struct repository *r, const char *obj_dir);\n + typedef int alt_odb_fn(struct object_directory *, void *);\n +\n + ## t/t7700-repack.sh ##\n +@@ t/t7700-repack.sh: test_expect_success SYMLINKS '--local keeps packs when alternate is objectdir '\n + \ttest_cmp expect actual\n + '\n + \n ++test_expect_success '--local disables writing bitmaps when connected to alternate ODB' '\n ++\ttest_when_finished \"rm -rf shared member\" &&\n ++\n ++\tgit init shared &&\n ++\tgit clone --shared shared member &&\n ++\t(\n ++\t\tcd member &&\n ++\t\ttest_commit \"object\" &&\n ++\t\tgit repack -Adl --write-bitmap-index 2>err &&\n ++\t\tcat >expect <<-EOF &&\n ++\t\twarning: disabling bitmap writing, as some objects are not being packed\n ++\t\tEOF\n ++\t\ttest_cmp expect err &&\n ++\t\ttest ! -f .git/objects/pack-*.bitmap\n ++\t)\n ++'\n ++\n + test_expect_success 'packed obs in alt ODB are repacked even when local repo is packless' '\n + \tmkdir alt_objects/pack &&\n + \tmv .git/objects/pack/* alt_objects/pack &&\n \n ## t/t7703-repack-geometric.sh ##\n @@ t/t7703-repack-geometric.sh: test_expect_success '--geometric -l with non-intact geometric sequence across OD\n-- \n2.40.0\n\n"},{"id":"475276","messageId":"c5db1bc587701df35f2cb32270ab734ffdec2e32.1681384405.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681384405.git.ps@pks.im","subject":"[PATCH v3 02/10] repack: fix trying to use preferred pack in alternates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-13T11:16:11Z","receivedAt":"2023-04-13T11:16:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When doing a geometric repack with multi-pack-indices, then we ask\ngit-multi-pack-index(1) to use the largest packfile as the preferred\npack. It can happen though that the largest packfile is not part of the\nmain object database, but instead part of an alternate object database.\nThe result is that git-multi-pack-index(1) will not be able to find the\npreferred pack and print a warning. It then falls back to use the first\npackfile that the multi-pack-index shall reference.\n\nFix this bug by only considering packfiles as preferred pack that are\nlocal. This is the right thing to do given that a multi-pack-index\nshould never reference packfiles borrowed from an alternate.\n\nWhile at it, rename the function `get_largest_active_packfile()` to\n`get_preferred_pack()` to better document its intent.\n\nHelped-by: Taylor Blau \nSigned-off-by: Patrick Steinhardt \n---\n builtin/repack.c | 26 +++++++++++++++++++++-----\n t/t7703-repack-geometric.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 53 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 87f73c8923..687419776d 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -446,8 +446,10 @@ static void split_pack_geometry(struct pack_geometry *geometry, int factor)\n \tgeometry->split = split;\n }\n \n-static struct packed_git *get_largest_active_pack(struct pack_geometry *geometry)\n+static struct packed_git *get_preferred_pack(struct pack_geometry *geometry)\n {\n+\tuint32_t i;\n+\n \tif (!geometry) {\n \t\t/*\n \t\t * No geometry means either an all-into-one repack (in which\n@@ -462,7 +464,21 @@ static struct packed_git *get_largest_active_pack(struct pack_geometry *geometry\n \t}\n \tif (geometry->split == geometry->pack_nr)\n \t\treturn NULL;\n-\treturn geometry->pack[geometry->pack_nr - 1];\n+\n+\t/*\n+\t * The preferred pack is the largest pack above the split line. In\n+\t * other words, it is the largest pack that does not get rolled up in\n+\t * the geometric repack.\n+\t */\n+\tfor (i = geometry->pack_nr; i > geometry->split; i--)\n+\t\t/*\n+\t\t * A pack that is not local would never be included in a\n+\t\t * multi-pack index. We thus skip over any non-local packs.\n+\t\t */\n+\t\tif (geometry->pack[i - 1]->pack_local)\n+\t\t\treturn geometry->pack[i - 1];\n+\n+\treturn NULL;\n }\n \n static void clear_pack_geometry(struct pack_geometry *geometry)\n@@ -589,7 +605,7 @@ static int write_midx_included_packs(struct string_list *include,\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct string_list_item *item;\n-\tstruct packed_git *largest = get_largest_active_pack(geometry);\n+\tstruct packed_git *preferred = get_preferred_pack(geometry);\n \tFILE *in;\n \tint ret;\n \n@@ -610,9 +626,9 @@ static int write_midx_included_packs(struct string_list *include,\n \tif (write_bitmaps)\n \t\tstrvec_push(&cmd.args, \"--bitmap\");\n \n-\tif (largest)\n+\tif (preferred)\n \t\tstrvec_pushf(&cmd.args, \"--preferred-pack=%s\",\n-\t\t\t pack_basename(largest));\n+\t\t\t pack_basename(preferred));\n \n \tif (refs_snapshot)\n \t\tstrvec_pushf(&cmd.args, \"--refs-snapshot=%s\", refs_snapshot);\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 8821fbd2dd..d6db362541 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -281,4 +281,36 @@ test_expect_success '--geometric with pack.packSizeLimit' '\n \t)\n '\n \n+test_expect_success '--geometric --write-midx with packfiles in main and alternate ODB' '\n+\ttest_when_finished \"rm -fr shared member\" &&\n+\n+\t# Create a shared repository that will serve as the alternate object\n+\t# database for the member linked to it. It has got some objects on its\n+\t# own that are packed into a single packfile.\n+\tgit init shared &&\n+\ttest_commit -C shared common-object &&\n+\tgit -C shared repack -ad &&\n+\n+\t# We create member so that its alternates file points to the shared\n+\t# repository. We then create a commit in it so that git-repack(1) has\n+\t# something to repack.\n+\t# of the shared object database.\n+\tgit clone --shared shared member &&\n+\ttest_commit -C member unique-object &&\n+\tgit -C member repack --geometric=2 --write-midx 2>err &&\n+\ttest_must_be_empty err &&\n+\n+\t# We should see that a new packfile was generated.\n+\tfind shared/.git/objects/pack -type f -name \"*.pack\" >packs &&\n+\ttest_line_count = 1 packs &&\n+\n+\t# We should also see a multi-pack-index. This multi-pack-index should\n+\t# never refer to any packfiles in the alternate object database.\n+\ttest -f member/.git/objects/pack/multi-pack-index &&\n+\ttest-tool read-midx member/.git/objects >packs.midx &&\n+\tgrep \"^pack-.*\\.idx$\" packs.midx | sort >actual &&\n+\tbasename member/.git/objects/pack/pack-*.idx >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475277","messageId":"8c3193268fd5938ebf85bd43a551d0c50d2e8336.1681384405.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681384405.git.ps@pks.im","subject":"[PATCH v3 03/10] repack: fix generating multi-pack-index with only non-local packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-13T11:16:15Z","receivedAt":"2023-04-13T11:16:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When writing the multi-pack-index with geometric repacking we will add\nall packfiles to the index that are part of the geometric sequence. This\ncan potentially also include packfiles borrowed from an alternate object\ndirectory. But given that a multi-pack-index can only ever include packs\nthat are part of the main object database this does not make much sense\nwhatsoever.\n\nIn the edge case where all packfiles are contained in the alternate\nobject database and the local repository has none itself this bug can\ncause us to invoke git-multi-pack-index(1) with only non-local packfiles\nthat it ultimately cannot find. This causes it to return an error and\nthus causes the geometric repack to fail.\n\nFix the code to skip non-local packfiles.\n\nCo-authored-by: Taylor Blau \nSigned-off-by: Taylor Blau \nSigned-off-by: Patrick Steinhardt \n---\n builtin/repack.c | 11 +++++++++++\n t/t7703-repack-geometric.sh | 23 +++++++++++++++++++++++\n 2 files changed, 34 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 687419776d..80fc860613 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -572,6 +572,17 @@ static void midx_included_packs(struct string_list *include,\n \t\tfor (i = geometry->split; i < geometry->pack_nr; i++) {\n \t\t\tstruct packed_git *p = geometry->pack[i];\n \n+\t\t\t/*\n+\t\t\t * The multi-pack index never refers to packfiles part\n+\t\t\t * of an alternate object database, so we skip these.\n+\t\t\t * While git-multi-pack-index(1) would silently ignore\n+\t\t\t * them anyway, this allows us to skip executing the\n+\t\t\t * command completely when we have only non-local\n+\t\t\t * packfiles.\n+\t\t\t */\n+\t\t\tif (!p->pack_local)\n+\t\t\t\tcontinue;\n+\n \t\t\tstrbuf_addstr(&buf, pack_basename(p));\n \t\t\tstrbuf_strip_suffix(&buf, \".pack\");\n \t\t\tstrbuf_addstr(&buf, \".idx\");\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex d6db362541..7027676977 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -313,4 +313,27 @@ test_expect_success '--geometric --write-midx with packfiles in main and alterna\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--geometric --with-midx with no local objects' '\n+\ttest_when_finished \"rm -fr shared member\" &&\n+\n+\t# Create a repository with a single packfile that acts as alternate\n+\t# object database.\n+\tgit init shared &&\n+\ttest_commit -C shared \"shared-objects\" &&\n+\tgit -C shared repack -ad &&\n+\n+\t# Create a second repository linked to the first one and perform a\n+\t# geometric repack on it.\n+\tgit clone --shared shared member &&\n+\tgit -C member repack --geometric 2 --write-midx 2>err &&\n+\ttest_must_be_empty err &&\n+\n+\t# Assert that we wrote neither a new packfile nor a multi-pack-index.\n+\t# We should not have a packfile because the single packfile in the\n+\t# alternate object database does not invalidate the geometric sequence.\n+\t# And we should not have a multi-pack-index because these only index\n+\t# local packfiles, and there are none.\n+\ttest_dir_is_empty member/$packdir\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475278","messageId":"8d47d753dc03979bd515f473a591906ba03c0c2b.1681384405.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681384405.git.ps@pks.im","subject":"[PATCH v3 04/10] pack-objects: split out `--stdin-packs` tests into separate file","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-13T11:16:19Z","receivedAt":"2023-04-13T11:16:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The test suite for git-pack-objects(1) is quite huge, and we're about to\nadd more tests that relate to the `--stdin-packs` option. Split out all\ntests related to this option into a standalone file so that it becomes\neasier to test the feature in isolation.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5300-pack-object.sh | 135 -------------------------------\n t/t5331-pack-objects-stdin.sh | 145 ++++++++++++++++++++++++++++++++++\n 2 files changed, 145 insertions(+), 135 deletions(-)\n create mode 100755 t/t5331-pack-objects-stdin.sh\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex f8a0f309e2..d2ce236d61 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -589,141 +589,6 @@ test_expect_success 'prefetch objects' '\n \ttest_line_count = 1 donelines\n '\n \n-test_expect_success 'setup for --stdin-packs tests' '\n-\tgit init stdin-packs &&\n-\t(\n-\t\tcd stdin-packs &&\n-\n-\t\ttest_commit A &&\n-\t\ttest_commit B &&\n-\t\ttest_commit C &&\n-\n-\t\tfor id in A B C\n-\t\tdo\n-\t\t\tgit pack-objects .git/objects/pack/pack-$id \\\n-\t\t\t\t--incremental --revs <<-EOF || exit 1\n-\t\t\trefs/tags/$id\n-\t\t\tEOF\n-\t\tdone &&\n-\n-\t\tls -la .git/objects/pack\n-\t)\n-'\n-\n-test_expect_success '--stdin-packs with excluded packs' '\n-\t(\n-\t\tcd stdin-packs &&\n-\n-\t\tPACK_A=\"$(basename .git/objects/pack/pack-A-*.pack)\" &&\n-\t\tPACK_B=\"$(basename .git/objects/pack/pack-B-*.pack)\" &&\n-\t\tPACK_C=\"$(basename .git/objects/pack/pack-C-*.pack)\" &&\n-\n-\t\tgit pack-objects test --stdin-packs <<-EOF &&\n-\t\t$PACK_A\n-\t\t^$PACK_B\n-\t\t$PACK_C\n-\t\tEOF\n-\n-\t\t(\n-\t\t\tgit show-index <$(ls .git/objects/pack/pack-A-*.idx) &&\n-\t\t\tgit show-index <$(ls .git/objects/pack/pack-C-*.idx)\n-\t\t) >expect.raw &&\n-\t\tgit show-index <$(ls test-*.idx) >actual.raw &&\n-\n-\t\tcut -d\" \" -f2 expect &&\n-\t\tcut -d\" \" -f2 actual &&\n-\t\ttest_cmp expect actual\n-\t)\n-'\n-\n-test_expect_success '--stdin-packs is incompatible with --filter' '\n-\t(\n-\t\tcd stdin-packs &&\n-\t\ttest_must_fail git pack-objects --stdin-packs --stdout \\\n-\t\t\t--filter=blob:none err &&\n-\t\ttest_i18ngrep \"cannot use --filter with --stdin-packs\" err\n-\t)\n-'\n-\n-test_expect_success '--stdin-packs is incompatible with --revs' '\n-\t(\n-\t\tcd stdin-packs &&\n-\t\ttest_must_fail git pack-objects --stdin-packs --revs out \\\n-\t\t\terr &&\n-\t\ttest_i18ngrep \"cannot use internal rev list with --stdin-packs\" err\n-\t)\n-'\n-\n-test_expect_success '--stdin-packs with loose objects' '\n-\t(\n-\t\tcd stdin-packs &&\n-\n-\t\tPACK_A=\"$(basename .git/objects/pack/pack-A-*.pack)\" &&\n-\t\tPACK_B=\"$(basename .git/objects/pack/pack-B-*.pack)\" &&\n-\t\tPACK_C=\"$(basename .git/objects/pack/pack-C-*.pack)\" &&\n-\n-\t\ttest_commit D && # loose\n-\n-\t\tgit pack-objects test2 --stdin-packs --unpacked <<-EOF &&\n-\t\t$PACK_A\n-\t\t^$PACK_B\n-\t\t$PACK_C\n-\t\tEOF\n-\n-\t\t(\n-\t\t\tgit show-index <$(ls .git/objects/pack/pack-A-*.idx) &&\n-\t\t\tgit show-index <$(ls .git/objects/pack/pack-C-*.idx) &&\n-\t\t\tgit rev-list --objects --no-object-names \\\n-\t\t\t\trefs/tags/C..refs/tags/D\n-\n-\t\t) >expect.raw &&\n-\t\tls -la . &&\n-\t\tgit show-index <$(ls test2-*.idx) >actual.raw &&\n-\n-\t\tcut -d\" \" -f2 expect &&\n-\t\tcut -d\" \" -f2 actual &&\n-\t\ttest_cmp expect actual\n-\t)\n-'\n-\n-test_expect_success '--stdin-packs with broken links' '\n-\t(\n-\t\tcd stdin-packs &&\n-\n-\t\t# make an unreachable object with a bogus parent\n-\t\tgit cat-file -p HEAD >commit &&\n-\t\tsed \"s/$(git rev-parse HEAD^)/$(test_oid zero)/\" in &&\n-\n-\t\tgit pack-objects .git/objects/pack/pack-D expect.raw &&\n-\t\tgit show-index <$(ls test3-*.idx) >actual.raw &&\n-\n-\t\tcut -d\" \" -f2 expect &&\n-\t\tcut -d\" \" -f2 actual &&\n-\t\ttest_cmp expect actual\n-\t)\n-'\n-\n test_expect_success 'negative window clamps to 0' '\n \tgit pack-objects --progress --window=-1 neg-window stderr &&\n \tcheck_deltas stderr = 0\ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nnew file mode 100755\nindex 0000000000..d5eece5899\n--- /dev/null\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -0,0 +1,145 @@\n+#!/bin/sh\n+\n+test_description='pack-objects --stdin'\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'setup for --stdin-packs tests' '\n+\tgit init stdin-packs &&\n+\t(\n+\t\tcd stdin-packs &&\n+\n+\t\ttest_commit A &&\n+\t\ttest_commit B &&\n+\t\ttest_commit C &&\n+\n+\t\tfor id in A B C\n+\t\tdo\n+\t\t\tgit pack-objects .git/objects/pack/pack-$id \\\n+\t\t\t\t--incremental --revs <<-EOF || exit 1\n+\t\t\trefs/tags/$id\n+\t\t\tEOF\n+\t\tdone &&\n+\n+\t\tls -la .git/objects/pack\n+\t)\n+'\n+\n+test_expect_success '--stdin-packs with excluded packs' '\n+\t(\n+\t\tcd stdin-packs &&\n+\n+\t\tPACK_A=\"$(basename .git/objects/pack/pack-A-*.pack)\" &&\n+\t\tPACK_B=\"$(basename .git/objects/pack/pack-B-*.pack)\" &&\n+\t\tPACK_C=\"$(basename .git/objects/pack/pack-C-*.pack)\" &&\n+\n+\t\tgit pack-objects test --stdin-packs <<-EOF &&\n+\t\t$PACK_A\n+\t\t^$PACK_B\n+\t\t$PACK_C\n+\t\tEOF\n+\n+\t\t(\n+\t\t\tgit show-index <$(ls .git/objects/pack/pack-A-*.idx) &&\n+\t\t\tgit show-index <$(ls .git/objects/pack/pack-C-*.idx)\n+\t\t) >expect.raw &&\n+\t\tgit show-index <$(ls test-*.idx) >actual.raw &&\n+\n+\t\tcut -d\" \" -f2 expect &&\n+\t\tcut -d\" \" -f2 actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--stdin-packs is incompatible with --filter' '\n+\t(\n+\t\tcd stdin-packs &&\n+\t\ttest_must_fail git pack-objects --stdin-packs --stdout \\\n+\t\t\t--filter=blob:none err &&\n+\t\ttest_i18ngrep \"cannot use --filter with --stdin-packs\" err\n+\t)\n+'\n+\n+test_expect_success '--stdin-packs is incompatible with --revs' '\n+\t(\n+\t\tcd stdin-packs &&\n+\t\ttest_must_fail git pack-objects --stdin-packs --revs out \\\n+\t\t\terr &&\n+\t\ttest_i18ngrep \"cannot use internal rev list with --stdin-packs\" err\n+\t)\n+'\n+\n+test_expect_success '--stdin-packs with loose objects' '\n+\t(\n+\t\tcd stdin-packs &&\n+\n+\t\tPACK_A=\"$(basename .git/objects/pack/pack-A-*.pack)\" &&\n+\t\tPACK_B=\"$(basename .git/objects/pack/pack-B-*.pack)\" &&\n+\t\tPACK_C=\"$(basename .git/objects/pack/pack-C-*.pack)\" &&\n+\n+\t\ttest_commit D && # loose\n+\n+\t\tgit pack-objects test2 --stdin-packs --unpacked <<-EOF &&\n+\t\t$PACK_A\n+\t\t^$PACK_B\n+\t\t$PACK_C\n+\t\tEOF\n+\n+\t\t(\n+\t\t\tgit show-index <$(ls .git/objects/pack/pack-A-*.idx) &&\n+\t\t\tgit show-index <$(ls .git/objects/pack/pack-C-*.idx) &&\n+\t\t\tgit rev-list --objects --no-object-names \\\n+\t\t\t\trefs/tags/C..refs/tags/D\n+\n+\t\t) >expect.raw &&\n+\t\tls -la . &&\n+\t\tgit show-index <$(ls test2-*.idx) >actual.raw &&\n+\n+\t\tcut -d\" \" -f2 expect &&\n+\t\tcut -d\" \" -f2 actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--stdin-packs with broken links' '\n+\t(\n+\t\tcd stdin-packs &&\n+\n+\t\t# make an unreachable object with a bogus parent\n+\t\tgit cat-file -p HEAD >commit &&\n+\t\tsed \"s/$(git rev-parse HEAD^)/$(test_oid zero)/\" in &&\n+\n+\t\tgit pack-objects .git/objects/pack/pack-D expect.raw &&\n+\t\tgit show-index <$(ls test3-*.idx) >actual.raw &&\n+\n+\t\tcut -d\" \" -f2 expect &&\n+\t\tcut -d\" \" -f2 actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_done\n-- \n2.40.0\n\n"},{"id":"475279","messageId":"ac528514e7e609adc00f417b30593722ef73105a.1681384405.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681384405.git.ps@pks.im","subject":"[PATCH v3 05/10] pack-objects: fix error when packing same pack twice","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-13T11:16:24Z","receivedAt":"2023-04-13T11:16:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When passed the same packfile twice via `--stdin-packs` we return an\nerror that the packfile supposedly was not found. This is because when\nreading packs into the list of included or excluded packfiles, we will\nhappily re-add packfiles even if they are part of the lists already. And\nwhile the list can now contain duplicates, we will only set the `util`\npointer of the first list entry to the `packed_git` structure. We notice\nthat at a later point when checking that all list entries have their\n`util` pointer set and die with an error.\n\nWhile this is kind of a nonsensical request, this scenario can be hit\nwhen doing geometric repacks. When a repository is connected to an\nalternate object directory and both have the exact same packfile then\nboth would get added to the geometric sequence. And when we then decide\nto perform the repack, we will invoke git-pack-objects(1) with the same\npackfile twice.\n\nFix this bug by removing any duplicates from both the included and\nexcluded packs.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-objects.c | 2 ++\n t/t5331-pack-objects-stdin.sh | 27 +++++++++++++++++++++++++++\n t/t7703-repack-geometric.sh | 25 +++++++++++++++++++++++++\n 3 files changed, 54 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 7e9e20172a..7d0e864c35 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3348,7 +3348,9 @@ static void read_packs_list_from_stdin(void)\n \t}\n \n \tstring_list_sort(&include_packs);\n+\tstring_list_remove_duplicates(&include_packs, 0);\n \tstring_list_sort(&exclude_packs);\n+\tstring_list_remove_duplicates(&exclude_packs, 0);\n \n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tconst char *pack_name = pack_basename(p);\ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex d5eece5899..def7640c91 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -7,6 +7,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+packed_objects() {\n+\tgit show-index <\"$1\" >tmp-object-list &&\n+\tcut -d' ' -f2 tmp-object-list | sort &&\n+\trm tmp-object-list\n+ }\n+\n test_expect_success 'setup for --stdin-packs tests' '\n \tgit init stdin-packs &&\n \t(\n@@ -142,4 +148,25 @@ test_expect_success '--stdin-packs with broken links' '\n \t)\n '\n \n+test_expect_success 'pack-objects --stdin with duplicate packfile' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit \"commit\" &&\n+\t\tgit repack -ad &&\n+\n+\t\t{\n+\t\t\tbasename .git/objects/pack/pack-*.pack &&\n+\t\t\tbasename .git/objects/pack/pack-*.pack\n+\t\t} >packfiles &&\n+\n+\t\tgit pack-objects --stdin-packs generated-pack expect &&\n+\t\tpacked_objects generated-pack-*.idx >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 7027676977..d0823f2eb2 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -336,4 +336,29 @@ test_expect_success '--geometric --with-midx with no local objects' '\n \ttest_dir_is_empty member/$packdir\n '\n \n+test_expect_success '--geometric with same pack in main and alternate ODB' '\n+\ttest_when_finished \"rm -fr shared member\" &&\n+\n+\t# Create a repository with a single packfile that acts as alternate\n+\t# object database.\n+\tgit init shared &&\n+\ttest_commit -C shared \"shared-objects\" &&\n+\tgit -C shared repack -ad &&\n+\n+\t# We create the member repository as an exact copy so that it has the\n+\t# same packfile.\n+\tcp -r shared member &&\n+\ttest-tool path-utils real_path shared/.git/objects >member/.git/objects/info/alternates &&\n+\tfind shared/.git/objects -type f >expected-files &&\n+\n+\t# Verify that we can repack objects as expected without observing any\n+\t# error. Having the same packfile in both ODBs used to cause an error\n+\t# in git-pack-objects(1).\n+\tgit -C member repack --geometric 2 2>err &&\n+\ttest_must_be_empty err &&\n+\t# Nothing should have changed.\n+\tfind shared/.git/objects -type f >actual-files &&\n+\ttest_cmp expected-files actual-files\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475280","messageId":"afd7c7864d61c43292889c93495a28742dfa00ab.1681384405.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681384405.git.ps@pks.im","subject":"[PATCH v3 06/10] pack-objects: fix error when same packfile is included and excluded","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-13T11:16:28Z","receivedAt":"2023-04-13T11:17:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When passing the same packfile both as included and excluded via the\n`--stdin-packs` option, then we will return an error because the\nexcluded packfile cannot be found. This is because we will only set the\n`util` pointer for the included packfile list if it was found, so that\nwe later die when we notice that it's in fact not set for the excluded\npackfile list.\n\nFix this bug by always setting the `util` pointer for both the included\nand excluded list entries.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-objects.c | 8 +++-----\n t/t5331-pack-objects-stdin.sh | 20 ++++++++++++++++++++\n 2 files changed, 23 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 7d0e864c35..c5147d392f 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3355,11 +3355,9 @@ static void read_packs_list_from_stdin(void)\n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tconst char *pack_name = pack_basename(p);\n \n-\t\titem = string_list_lookup(&include_packs, pack_name);\n-\t\tif (!item)\n-\t\t\titem = string_list_lookup(&exclude_packs, pack_name);\n-\n-\t\tif (item)\n+\t\tif ((item = string_list_lookup(&include_packs, pack_name)))\n+\t\t\titem->util = p;\n+\t\tif ((item = string_list_lookup(&exclude_packs, pack_name)))\n \t\t\titem->util = p;\n \t}\n \ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex def7640c91..1e23cc2e61 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -169,4 +169,24 @@ test_expect_success 'pack-objects --stdin with duplicate packfile' '\n \t)\n '\n \n+test_expect_success 'pack-objects --stdin with same packfile excluded and included' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit \"commit\" &&\n+\t\tgit repack -ad &&\n+\n+\t\t{\n+\t\t\tbasename .git/objects/pack/pack-*.pack &&\n+\t\t\tprintf \"^%s\\n\" \"$(basename .git/objects/pack/pack-*.pack)\"\n+\t\t} >packfiles &&\n+\n+\t\tgit pack-objects --stdin-packs generated-pack packed-objects &&\n+\t\ttest_must_be_empty packed-objects\n+\t)\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475281","messageId":"cd135439ae9708bb9d2a97ceb78d905e1b125007.1681384405.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681384405.git.ps@pks.im","subject":"[PATCH v3 07/10] pack-objects: extend test coverage of `--stdin-packs` with alternates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-13T11:16:32Z","receivedAt":"2023-04-13T11:17:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We don't have any tests that verify that git-pack-objects(1) works with\n`--stdin-packs` when combined with alternate object directories. Add\nsome to make sure that the basic functionality works as expected.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5331-pack-objects-stdin.sh | 48 +++++++++++++++++++++++++++++++++++\n 1 file changed, 48 insertions(+)\n\ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex 1e23cc2e61..45e24fa94a 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -189,4 +189,52 @@ test_expect_success 'pack-objects --stdin with same packfile excluded and includ\n \t)\n '\n \n+test_expect_success 'pack-objects --stdin with packfiles from alternate object database' '\n+\ttest_when_finished \"rm -fr shared member\" &&\n+\n+\t# Set up a shared repository with a single packfile.\n+\tgit init shared &&\n+\ttest_commit -C shared \"shared-objects\" &&\n+\tgit -C shared repack -ad &&\n+\tbasename shared/.git/objects/pack/pack-*.pack >packfile &&\n+\n+\t# Set up a repository that is connected to the shared repository. This\n+\t# repository has no objects on its own, but we still expect to be able\n+\t# to pack objects from its alternate.\n+\tgit clone --shared shared member &&\n+\tgit -C member pack-objects --stdin-packs generated-pack packfiles &&\n+\n+\t{\n+\t\tpacked_objects shared/.git/objects/pack/pack-*.idx &&\n+\t\tpacked_objects member/.git/objects/pack/pack-*.idx\n+\t} | sort >expected-objects &&\n+\n+\tgit -C member pack-objects --stdin-packs generated-pack actual-objects &&\n+\ttest_cmp expected-objects actual-objects\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475282","messageId":"18bac902896d6955a359093c4bb072166209ffc4.1681384405.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681384405.git.ps@pks.im","subject":"[PATCH v3 08/10] t/helper: allow chmtime to print verbosely without modifying mtime","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-13T11:16:37Z","receivedAt":"2023-04-13T11:17:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `test-tool chmtime` helper allows us to both read and modify the\nmodification time of files. But while it is possible to only read the\nmtimes of a file via `--get`, it is not possible to read the mtimes\nand report them together with their respective file paths via the\n`--verbose` flag without also modifying the mtime at the same time.\n\nFix this so that it is possible to call `test-tool chmtime --verbose\n...` without modifying any mtimes.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/helper/test-chmtime.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/helper/test-chmtime.c b/t/helper/test-chmtime.c\nindex dc28890a18..0e5538833a 100644\n--- a/t/helper/test-chmtime.c\n+++ b/t/helper/test-chmtime.c\n@@ -94,7 +94,7 @@ int cmd__chmtime(int argc, const char **argv)\n \tif (timespec_arg(argv[i], &set_time, &set_eq)) {\n \t\t++i;\n \t} else {\n-\t\tif (get == 0) {\n+\t\tif (get == 0 && verbose == 0) {\n \t\t\tfprintf(stderr, \"Not a base-10 integer: %s\\n\", argv[i] + 1);\n \t\t\tgoto usage;\n \t\t}\n-- \n2.40.0\n\n"},{"id":"475283","messageId":"285695deafa5a4a49f774dc484782dd8e4fd4997.1681384405.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681384405.git.ps@pks.im","subject":"[PATCH v3 09/10] repack: honor `-l` when calculating pack geometry","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-13T11:16:42Z","receivedAt":"2023-04-13T11:17:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When the user passes `-l` to git-repack(1), then they essentially ask us\nto only repack objects part of the local object database while ignoring\nany packfiles part of an alternate object database. And we in fact honor\nthis bit when doing a geometric repack as the resulting packfile will\nonly ever contain local objects.\n\nWhat we're missing though is that we don't take locality of packfiles\ninto account when computing whether the geometric sequence is intact or\nnot. So even though we would only ever roll up local packfiles anyway,\nwe could end up trying to repack because of non-local packfiles. This\ndoes not make much sense, and in the worst case it can cause us to try\nand do the geometric repack over and over again because we're never able\nto restore the geometric sequence.\n\nFix this bug by honoring whether the user has passed `-l`. If so, we\nskip adding any non-local packfiles to the pack geometry.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/repack.c | 13 +++++++--\n t/t7703-repack-geometric.sh | 58 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 69 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 80fc860613..5768aeb1f3 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -323,7 +323,8 @@ static int geometry_cmp(const void *va, const void *vb)\n }\n \n static void init_pack_geometry(struct pack_geometry **geometry_p,\n-\t\t\t struct string_list *existing_kept_packs)\n+\t\t\t struct string_list *existing_kept_packs,\n+\t\t\t const struct pack_objects_args *args)\n {\n \tstruct packed_git *p;\n \tstruct pack_geometry *geometry;\n@@ -333,6 +334,14 @@ static void init_pack_geometry(struct pack_geometry **geometry_p,\n \tgeometry = *geometry_p;\n \n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n+\t\tif (args->local && !p->pack_local)\n+\t\t\t/*\n+\t\t\t * When asked to only repack local packfiles we skip\n+\t\t\t * over any packfiles that are borrowed from alternate\n+\t\t\t * object directories.\n+\t\t\t */\n+\t\t\tcontinue;\n+\n \t\tif (!pack_kept_objects) {\n \t\t\t/*\n \t\t\t * Any pack that has its pack_keep bit set will appear\n@@ -900,7 +909,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (geometric_factor) {\n \t\tif (pack_everything)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--geometric\", \"-A/-a\");\n-\t\tinit_pack_geometry(&geometry, &existing_kept_packs);\n+\t\tinit_pack_geometry(&geometry, &existing_kept_packs, &po_args);\n \t\tsplit_pack_geometry(geometry, geometric_factor);\n \t}\n \ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex d0823f2eb2..c440956ad5 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -10,6 +10,12 @@ objdir=.git/objects\n packdir=$objdir/pack\n midx=$objdir/pack/multi-pack-index\n \n+packed_objects() {\n+\tgit show-index <\"$1\" >tmp-object-list &&\n+\tcut -d' ' -f2 tmp-object-list &&\n+\trm tmp-object-list\n+ }\n+\n test_expect_success '--geometric with no packs' '\n \tgit init geometric &&\n \ttest_when_finished \"rm -fr geometric\" &&\n@@ -361,4 +367,56 @@ test_expect_success '--geometric with same pack in main and alternate ODB' '\n \ttest_cmp expected-files actual-files\n '\n \n+test_expect_success '--geometric -l with non-intact geometric sequence across ODBs' '\n+\ttest_when_finished \"rm -fr shared member\" &&\n+\n+\tgit init shared &&\n+\ttest_commit_bulk -C shared --start=1 1 &&\n+\n+\tgit clone --shared shared member &&\n+\ttest_commit_bulk -C member --start=2 1 &&\n+\n+\t# Verify that our assumptions actually hold: both generated packfiles\n+\t# should have three objects and should be non-equal.\n+\tpacked_objects shared/.git/objects/pack/pack-*.idx >packed-objects &&\n+\ttest_line_count = 3 packed-objects &&\n+\tpacked_objects member/.git/objects/pack/pack-*.idx >packed-objetcs &&\n+\ttest_line_count = 3 packed-objects &&\n+\ttest \"$(basename member/.git/objects/pack/pack-*.pack)\" != \"$(basename shared/.git/objects/pack/pack-*.pack)\" &&\n+\n+\t# Perform the geometric repack. With `-l`, we should only see the local\n+\t# packfile and thus arrive at the conclusion that the geometric\n+\t# sequence is intact. We thus expect no changes.\n+\t#\n+\t# Note that we are tweaking mtimes of the packfiles so that we can\n+\t# verify they did not change. This is done in order to detect the case\n+\t# where we do repack objects, but the resulting packfile is the same.\n+\ttest-tool chmtime --verbose =0 member/.git/objects/pack/* >expected-member-packs &&\n+\tgit -C member repack --geometric=2 -l -d &&\n+\ttest-tool chmtime --verbose member/.git/objects/pack/* >actual-member-packs &&\n+\ttest_cmp expected-member-packs actual-member-packs &&\n+\n+\t{\n+\t\tpacked_objects shared/.git/objects/pack/pack-*.idx &&\n+\t\tpacked_objects member/.git/objects/pack/pack-*.idx\n+\t} | sort >expected-objects &&\n+\n+\t# On the other hand, when doing a non-local geometric repack we should\n+\t# see both packfiles and thus repack them. We expect that the shared\n+\t# object database was not changed.\n+\ttest-tool chmtime --verbose =0 shared/.git/objects/pack/* >expected-shared-packs &&\n+\tgit -C member repack --geometric=2 -d &&\n+\ttest-tool chmtime --verbose shared/.git/objects/pack/* >actual-shared-packs &&\n+\ttest_cmp expected-shared-packs actual-shared-packs &&\n+\n+\t# Furthermore, we expect that the member repository now has a single\n+\t# packfile that contains the combined shared and non-shared objects.\n+\tls member/.git/objects/pack/pack-*.idx >actual &&\n+\ttest_line_count = 1 actual &&\n+\tpacked_objects member/.git/objects/pack/pack-*.idx >actual-objects &&\n+\ttest_line_count = 6 actual-objects &&\n+\tsort actual-objects.sorted &&\n+\ttest_cmp expected-objects actual-objects.sorted\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475284","messageId":"bb0ee0eb3c72c0e9f4306c9072b9dd104af965cd.1681384405.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681384405.git.ps@pks.im","subject":"[PATCH v3 10/10] repack: disable writing bitmaps when doing a local repack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-13T11:16:46Z","receivedAt":"2023-04-13T11:17:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In order to write a bitmap, we need to have full coverage of all objects\nthat are about to be packed. In the traditional non-multi-pack-index\nworld this meant we need to do a full repack of all objects into a\nsingle packfile. But in the new multi-pack-index world we can get away\nwith writing bitmaps when we have multiple packfiles as long as the\nmulti-pack-index covers all objects.\n\nThis is not always the case though. When asked to perform a repack of\nlocal objects, only, then we cannot guarantee to have full coverage of\nall objects regardless of whether we do a full repack or a repack with a\nmulti-pack-index. The end result is that writing the bitmap will fail in\nboth worlds:\n\n $ git multi-pack-index write --stdin-packs --bitmap \n---\n builtin/repack.c | 12 ++++++++++++\n object-file.c | 6 ++++++\n object-store.h | 1 +\n t/t7700-repack.sh | 17 +++++++++++++++++\n t/t7703-repack-geometric.sh | 27 +++++++++++++++++++++++++++\n 5 files changed, 63 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 5768aeb1f3..e2abcea2ee 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -887,6 +887,18 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (write_bitmaps && !(pack_everything & ALL_INTO_ONE) && !write_midx)\n \t\tdie(_(incremental_bitmap_conflict_error));\n \n+\tif (write_bitmaps && po_args.local && has_alt_odb(the_repository)) {\n+\t\t/*\n+\t\t * When asked to do a local repack, but we have\n+\t\t * packfiles that are inherited from an alternate, then\n+\t\t * we cannot guarantee that the multi-pack-index would\n+\t\t * have full coverage of all objects. We thus disable\n+\t\t * writing bitmaps in that case.\n+\t\t */\n+\t\twarning(_(\"disabling bitmap writing, as some objects are not being packed\"));\n+\t\twrite_bitmaps = 0;\n+\t}\n+\n \tif (write_midx && write_bitmaps) {\n \t\tstruct strbuf path = STRBUF_INIT;\n \ndiff --git a/object-file.c b/object-file.c\nindex 8fab8dbe80..3ad11c683b 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -946,6 +946,12 @@ void prepare_alt_odb(struct repository *r)\n \tr->objects->loaded_alternates = 1;\n }\n \n+int has_alt_odb(struct repository *r)\n+{\n+\tprepare_alt_odb(r);\n+\treturn !!r->objects->odb->next;\n+}\n+\n /* Returns 1 if we have successfully freshened the file, 0 otherwise. */\n static int freshen_file(const char *fn)\n {\ndiff --git a/object-store.h b/object-store.h\nindex 82201ec3e7..574f16140f 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -56,6 +56,7 @@ KHASH_INIT(odb_path_map, const char * /* key: odb_path */,\n \tstruct object_directory *, 1, fspathhash, fspatheq)\n \n void prepare_alt_odb(struct repository *r);\n+int has_alt_odb(struct repository *r);\n char *compute_alternate_path(const char *path, struct strbuf *err);\n struct object_directory *find_odb(struct repository *r, const char *obj_dir);\n typedef int alt_odb_fn(struct object_directory *, void *);\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 4aabe98139..93c5763e7d 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -106,6 +106,23 @@ test_expect_success SYMLINKS '--local keeps packs when alternate is objectdir '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--local disables writing bitmaps when connected to alternate ODB' '\n+\ttest_when_finished \"rm -rf shared member\" &&\n+\n+\tgit init shared &&\n+\tgit clone --shared shared member &&\n+\t(\n+\t\tcd member &&\n+\t\ttest_commit \"object\" &&\n+\t\tgit repack -Adl --write-bitmap-index 2>err &&\n+\t\tcat >expect <<-EOF &&\n+\t\twarning: disabling bitmap writing, as some objects are not being packed\n+\t\tEOF\n+\t\ttest_cmp expect err &&\n+\t\ttest ! -f .git/objects/pack-*.bitmap\n+\t)\n+'\n+\n test_expect_success 'packed obs in alt ODB are repacked even when local repo is packless' '\n \tmkdir alt_objects/pack &&\n \tmv .git/objects/pack/* alt_objects/pack &&\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex c440956ad5..33d7977fca 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -419,4 +419,31 @@ test_expect_success '--geometric -l with non-intact geometric sequence across OD\n \ttest_cmp expected-objects actual-objects.sorted\n '\n \n+test_expect_success '--geometric -l disables writing bitmaps with non-local packfiles' '\n+\ttest_when_finished \"rm -fr shared member\" &&\n+\n+\tgit init shared &&\n+\ttest_commit_bulk -C shared --start=1 1 &&\n+\n+\tgit clone --shared shared member &&\n+\ttest_commit_bulk -C member --start=2 1 &&\n+\n+\t# When performing a geometric repack with `-l` while connecting to an\n+\t# alternate object database that has a packfile we do not have full\n+\t# coverage of objects. As a result, we expect that writing the bitmap\n+\t# will be disabled.\n+\tgit -C member repack -l --geometric=2 --write-midx --write-bitmap-index 2>err &&\n+\tcat >expect <<-EOF &&\n+\twarning: disabling bitmap writing, as some objects are not being packed\n+\tEOF\n+\ttest_cmp expect err &&\n+\ttest ! -f member/.git/objects/pack/multi-pack-index-*.bitmap &&\n+\n+\t# On the other hand, when we repack without `-l`, we should see that\n+\t# the bitmap gets created.\n+\tgit -C member repack --geometric=2 --write-midx --write-bitmap-index 2>err &&\n+\ttest_must_be_empty err &&\n+\ttest -f member/.git/objects/pack/multi-pack-index-*.bitmap\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475300","messageId":"1d2e1796-c305-0876-533c-2fe0fd39d722@github.com","threadId":"59535","inReplyTo":"dd8145beade440e5444130d3a3189b2c5d15a911.1681384405.git.ps@pks.im","subject":"Re: [PATCH v3 01/10] midx: fix segfault with no packs and invalid preferred pack","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-13T13:49:34Z","receivedAt":"2023-04-13T13:49:43Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/13/2023 7:16 AM, Patrick Steinhardt wrote:\n\n> Fix this bug by resetting the preferred packfile index to `-1` before\n> searching for the preferred pack. This fixes the segfault as we already\n> check for whether the index is `> - 1`. If it is not, we simply don't\n> pick a preferred packfile at all.\n\n> \tif (preferred_pack_name) {\n> -\t\tint found = 0;\n> +\t\tctx.preferred_pack_idx = -1;\n> +\n\nThis patch looks good, but I did need to think about it for a bit,\nso here are my thoughts (feel free to ignore):\n\nI briefly considered recommending that we set this as the default in\nan initializer macro, something like\n\n #define WRITE_MIDX_CONTEXT_INIT { .preferred_pack_idx = -1 }\n\nbut this struct is internal to the file and only constructed once\n(at the start of write_midx_internal).\n\nFurther, outside the context of this patch we have this code:\n\n\t} else if (ctx.nr &&\n\t\t (flags & (MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP))) {\n\t\tstruct packed_git *oldest = ctx.info[ctx.preferred_pack_idx].p;\n\t\tctx.preferred_pack_idx = 0;\n\nI don't see a way that ctx.preferred_pack_idx can be anything but zero\nhere, but it's also not going to give a segfault because of the 'ctx.nr'\ncondition.\n\nThanks,\n-Stolee\n"},{"id":"475302","messageId":"52079b9b-a55c-e7d1-930e-38105dd8a85b@github.com","threadId":"59535","inReplyTo":"285695deafa5a4a49f774dc484782dd8e4fd4997.1681384405.git.ps@pks.im","subject":"Re: [PATCH v3 09/10] repack: honor `-l` when calculating pack geometry","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-13T13:59:02Z","receivedAt":"2023-04-13T13:59:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/13/2023 7:16 AM, Patrick Steinhardt wrote:\n> When the user passes `-l` to git-repack(1), then they essentially ask us\n> to only repack objects part of the local object database while ignoring\n> any packfiles part of an alternate object database. And we in fact honor\n> this bit when doing a geometric repack as the resulting packfile will\n> only ever contain local objects.\n\n> +\t# Verify that our assumptions actually hold: both generated packfiles\n> +\t# should have three objects and should be non-equal.\n> +\tpacked_objects shared/.git/objects/pack/pack-*.idx >packed-objects &&\n> +\ttest_line_count = 3 packed-objects &&\n> +\tpacked_objects member/.git/objects/pack/pack-*.idx >packed-objetcs &&\n\nTypo: s/packed-objetcs/packed-objects/\n\n> +\ttest_line_count = 3 packed-objects &&\n> +\ttest \"$(basename member/.git/objects/pack/pack-*.pack)\" != \"$(basename shared/.git/objects/pack/pack-*.pack)\" &&\n\nnit: could we do this where we store the output of the previous two\ncommands into different files and then use \"! test_cmp\"?\n\n\tpacked_objects shared/.git/objects/pack/pack-*.idx >shared-objects &&\n\tpacked_objects member/.git/objects/pack/pack-*.idx >member-objects &&\n\ttest_line_count = 3 shared-objects &&\n\ttest_line_count = 3 member-objects &&\n\t! test_cmp shared-objects member-objects &&\n\nThanks,\n-Stolee\n"},{"id":"475305","messageId":"ZDgOHdxv5lbf49g6@ncase","threadId":"59535","inReplyTo":"52079b9b-a55c-e7d1-930e-38105dd8a85b@github.com","subject":"Re: [PATCH v3 09/10] repack: honor `-l` when calculating pack geometry","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-13T14:13:49Z","receivedAt":"2023-04-13T14:14:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Apr 13, 2023 at 09:59:02AM -0400, Derrick Stolee wrote:\n> On 4/13/2023 7:16 AM, Patrick Steinhardt wrote:\n> > When the user passes `-l` to git-repack(1), then they essentially ask us\n> > to only repack objects part of the local object database while ignoring\n> > any packfiles part of an alternate object database. And we in fact honor\n> > this bit when doing a geometric repack as the resulting packfile will\n> > only ever contain local objects.\n> \n> > +\t# Verify that our assumptions actually hold: both generated packfiles\n> > +\t# should have three objects and should be non-equal.\n> > +\tpacked_objects shared/.git/objects/pack/pack-*.idx >packed-objects &&\n> > +\ttest_line_count = 3 packed-objects &&\n> > +\tpacked_objects member/.git/objects/pack/pack-*.idx >packed-objetcs &&\n> \n> Typo: s/packed-objetcs/packed-objects/\n> \n> > +\ttest_line_count = 3 packed-objects &&\n> > +\ttest \"$(basename member/.git/objects/pack/pack-*.pack)\" != \"$(basename shared/.git/objects/pack/pack-*.pack)\" &&\n> \n> nit: could we do this where we store the output of the previous two\n> commands into different files and then use \"! test_cmp\"?\n> \n> \tpacked_objects shared/.git/objects/pack/pack-*.idx >shared-objects &&\n> \tpacked_objects member/.git/objects/pack/pack-*.idx >member-objects &&\n> \ttest_line_count = 3 shared-objects &&\n> \ttest_line_count = 3 member-objects &&\n> \t! test_cmp shared-objects member-objects &&\n\nYeah, this is definitely easier to read.\n\nPatrick\n"},{"id":"475306","messageId":"cbcc6022-1f13-1fa7-aca3-5f87a9cc8eb5@github.com","threadId":"59535","inReplyTo":"cover.1681384405.git.ps@pks.im","subject":"Re: [PATCH v3 00/10] repack: fix geometric repacking with gitalternates","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-13T14:54:53Z","receivedAt":"2023-04-13T14:54:58Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/13/2023 7:16 AM, Patrick Steinhardt wrote:\n> Hi,\n> \n> this is the third version of my patch series to fix geometric repacking\n> with repositories that are connected to an alternate object database.\n> \n> Changes compared to v2:\n> \n> - I've simplified patch 1 to reset the preferred pack index instead\n> of moving around some of the checks. This also causes us to print\n> the warning for missing preferred packfiles again.\n> \n> - I've fixed the logic in patch 2 to find the preferred packfile to\n> not return a packfile that would be rolled up in a geometric\n> repack.\n> \n> - I've added an additional patch to split out preexisting tests for\n> `--stdin-packs` into their own test file.\n> \n> - I've changed the unportable test added for geometric repacking\n> with `-l` that used stat(1) to instead use our `test-tool chmtime`\n> helper.\n> \n> - I've changed the logic that disables writing bitmaps in git-repack\n> to cover more cases. It now always kicks in when doing a repack\n> with `-l` that asks for bitmaps when connected to an alternate\n> object directory.\n> \n> - In general, there's a bunch of small improvements left and right\n> for the tests I'm adding.\n\nI didn't get feedback on your v2 in time, but this v3 is very close.\n\nThere's just the one test change in patch 9 that I think deserves a\nre-roll, but otherwise this looks good.\n\nThanks,\n-Stolee\n"},{"id":"475309","messageId":"xmqq5y9zzt7j.fsf@gitster.g","threadId":"59535","inReplyTo":"52079b9b-a55c-e7d1-930e-38105dd8a85b@github.com","subject":"Re: [PATCH v3 09/10] repack: honor `-l` when calculating pack geometry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-13T15:40:16Z","receivedAt":"2023-04-13T15:40:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n>> +\ttest_line_count = 3 packed-objects &&\n>> +\tpacked_objects member/.git/objects/pack/pack-*.idx >packed-objetcs &&\n>\n> Typo: s/packed-objetcs/packed-objects/\n>\n>> +\ttest_line_count = 3 packed-objects &&\n\nThat's a good one. Because the result file created by the previous\nstep happens to also have three lines, the typo does not cause the\ntest to fail, but it is not testing what it designed to test.\n\nHere is \"git diff @{1}\" after my local \"rebase -i\" (\"diff\" of the\nresult is easier to read than \"range-diff\" in this case, as I did\nnot touch any log message).\n\nThanks for a very good set of eyes.\n\ndiff --git c/t/t5331-pack-objects-stdin.sh w/t/t5331-pack-objects-stdin.sh\nindex 45e24fa94a..acab31667a 100755\n--- c/t/t5331-pack-objects-stdin.sh\n+++ w/t/t5331-pack-objects-stdin.sh\n@@ -7,7 +7,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n-packed_objects() {\n+packed_objects () {\n \tgit show-index <\"$1\" >tmp-object-list &&\n \tcut -d' ' -f2 tmp-object-list | sort &&\n \trm tmp-object-list\ndiff --git c/t/t7703-repack-geometric.sh w/t/t7703-repack-geometric.sh\nindex 33d7977fca..57796f3437 100755\n--- c/t/t7703-repack-geometric.sh\n+++ w/t/t7703-repack-geometric.sh\n@@ -10,7 +10,7 @@ objdir=.git/objects\n packdir=$objdir/pack\n midx=$objdir/pack/multi-pack-index\n \n-packed_objects() {\n+packed_objects () {\n \tgit show-index <\"$1\" >tmp-object-list &&\n \tcut -d' ' -f2 tmp-object-list &&\n \trm tmp-object-list\n@@ -380,7 +380,7 @@ test_expect_success '--geometric -l with non-intact geometric sequence across OD\n \t# should have three objects and should be non-equal.\n \tpacked_objects shared/.git/objects/pack/pack-*.idx >packed-objects &&\n \ttest_line_count = 3 packed-objects &&\n-\tpacked_objects member/.git/objects/pack/pack-*.idx >packed-objetcs &&\n+\tpacked_objects member/.git/objects/pack/pack-*.idx >packed-objects &&\n \ttest_line_count = 3 packed-objects &&\n \ttest \"$(basename member/.git/objects/pack/pack-*.pack)\" != \"$(basename shared/.git/objects/pack/pack-*.pack)\" &&\n \n"},{"id":"475330","messageId":"xmqqv8hzusmz.fsf@gitster.g","threadId":"59535","inReplyTo":"cover.1681384405.git.ps@pks.im","subject":"Re: [PATCH v3 00/10] repack: fix geometric repacking with gitalternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-14T02:03:48Z","receivedAt":"2023-04-14T02:03:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Changes compared to v2:\n>\n> - I've simplified patch 1 to reset the preferred pack index instead\n> of moving around some of the checks. This also causes us to print\n> the warning for missing preferred packfiles again.\n>\n> - I've fixed the logic in patch 2 to find the preferred packfile to\n> not return a packfile that would be rolled up in a geometric\n> repack.\n>\n> - I've added an additional patch to split out preexisting tests for\n> `--stdin-packs` into their own test file.\n>\n> - I've changed the unportable test added for geometric repacking\n> with `-l` that used stat(1) to instead use our `test-tool chmtime`\n> helper.\n>\n> - I've changed the logic that disables writing bitmaps in git-repack\n> to cover more cases. It now always kicks in when doing a repack\n> with `-l` that asks for bitmaps when connected to an alternate\n> object directory.\n>\n> - In general, there's a bunch of small improvements left and right\n> for the tests I'm adding.\n>\n> Thanks for all the feedback so far.\n\nLoss of \"stat\" is very much appreciated, as the ones added\nin the previous round were the only hits from\n\n $ git grep '^[ ]*stat ' t/ ;# leading SP/HT plus \"stat \"\n\nand I suspect among platforms that are not Windows, not Linux, and\nnot macOS, there may be ones that lack the tool.\n\nWith a few fix-ups to the test script I sent to the thread for the\nreview of [09/10] squashed in, this topic however seems to cause the\nlinux-TEST-vars job fail when queued at the tip of 'seen' (bbfd3bf)\n[*1*]. When 'seen' is rewound by one merge to exclude this topic,\nthe CI runs OK (c35f3cd)[*2*]. I dug only far enough to identify\nwhich topic among 40+ ones was causing the CI breakage. Perhaps the\ncode is operating correctly but the expectation in the test needs\nupdating? I dunno.\n\nThanks.\n\n\n[References]\n*1* https://github.com/git/git/actions/runs/4694670520/jobs/8323047888#step:5:313\n*2* https://github.com/git/git/actions/runs/4694155668\n\n"},{"id":"475332","messageId":"ZDjnvgFyDZb9VZ8u@ncase","threadId":"59535","inReplyTo":"xmqqv8hzusmz.fsf@gitster.g","subject":"Re: [PATCH v3 00/10] repack: fix geometric repacking with gitalternates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-14T05:42:22Z","receivedAt":"2023-04-14T05:42:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Apr 13, 2023 at 07:03:48PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n[snip]\n> With a few fix-ups to the test script I sent to the thread for the\n> review of [09/10] squashed in, this topic however seems to cause the\n> linux-TEST-vars job fail when queued at the tip of 'seen' (bbfd3bf)\n> [*1*]. When 'seen' is rewound by one merge to exclude this topic,\n> the CI runs OK (c35f3cd)[*2*]. I dug only far enough to identify\n> which topic among 40+ ones was causing the CI breakage. Perhaps the\n> code is operating correctly but the expectation in the test needs\n> updating? I dunno.\n\nThe issue is that linux-TEST-vars sets a bunch of environment variables\nthat change the way git-repack(1) operates. Important in this context\nare GIT_TEST_MULTI_PACK_INDEX and GIT_TEST_MULTI_PACK_INDEX_GENERATE_BITMAP\nas they change the assumptions that we're doing in our test. What is\ninteresting is that these cause us to write a multi-pack-index and in\nfact a bitmap, but the warning that we're testing for is not printed.\nAnd that's because of the following code snippet:\n\n```\nif (write_bitmaps < 0) {\n if (!write_midx &&\n (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))\n write_bitmaps = 0;\n} else if (write_bitmaps &&\n git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0) &&\n git_env_bool(GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP, 0)) {\n write_bitmaps = 0;\n}\n```\n\nSo if either of those environment variables is set we'll disable\ngeneration of bitmaps in git-repack(1) in favor of enabling them in\ngit-multi-pack-index(1). And consequentially we don't see the expected\nerror message.\n\nAnyway. I think what we should do here is to simply override the\nmechanism with GIT_TEST_MULTI_PACK_INDEX=0. Other tests in this file do\nthe same already, and it does make sense as we explicitly want to test\nthe non-multi-pack-index scenario. And given that we already added a\ntest for the with-multi-pack-index scenario in the same commit we don't\nreally loose any test coverage there.\n\nPatrick\n"},{"id":"475333","messageId":"cover.1681452028.git.ps@pks.im","threadId":"59535","inReplyTo":"a07ed50feeec4bfc3e9736bf493b9876896bcdd2.1680606445.git.ps@pks.im","subject":"[PATCH v4 00/10] repack: fix geometric repacking with gitalternates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-14T06:01:27Z","receivedAt":"2023-04-14T06:01:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the fourth version of my patch series to fix geometric repacking\nwith repositories connected to an alternate object database.\n\nThis version only addresses some issues with the tests, the actual logic\nremains untouched:\n\n - The test added in t7700-repack.sh that verifies that `--local`\n causes us to disable generation of the bitmap index was failing in\n the linux-TEST-vars CI job. This was because it sets\n GIT_TEST_MULTI_PACK_INDEX=1, which causes us to disable the bitmap\n logic in git-repack(1). I've fixed this failure by explicitly\n overriding the environment variable like other tests in the same\n file do.\n\n - I've converted path checks to use `test_path_is_missing` and\n `test_path_is_file` instead of `test ! -f` and `test -f`.\n\n - I've fixed a typo in t7703-repack-geometric.sh and shifted code\n around a bit to make the test more readable, following Derrick's\n suggestion.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (10):\n midx: fix segfault with no packs and invalid preferred pack\n repack: fix trying to use preferred pack in alternates\n repack: fix generating multi-pack-index with only non-local packs\n pack-objects: split out `--stdin-packs` tests into separate file\n pack-objects: fix error when packing same pack twice\n pack-objects: fix error when same packfile is included and excluded\n pack-objects: extend test coverage of `--stdin-packs` with alternates\n t/helper: allow chmtime to print verbosely without modifying mtime\n repack: honor `-l` when calculating pack geometry\n repack: disable writing bitmaps when doing a local repack\n\n builtin/pack-objects.c | 10 +-\n builtin/repack.c | 62 ++++++++-\n midx.c | 6 +-\n object-file.c | 6 +\n object-store.h | 1 +\n t/helper/test-chmtime.c | 2 +-\n t/t5300-pack-object.sh | 135 -------------------\n t/t5319-multi-pack-index.sh | 12 ++\n t/t5331-pack-objects-stdin.sh | 240 ++++++++++++++++++++++++++++++++++\n t/t7700-repack.sh | 17 +++\n t/t7703-repack-geometric.sh | 164 +++++++++++++++++++++++\n 11 files changed, 504 insertions(+), 151 deletions(-)\n create mode 100755 t/t5331-pack-objects-stdin.sh\n\nDiff against v3:\ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex 45e24fa94a..acab31667a 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -7,7 +7,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n-packed_objects() {\n+packed_objects () {\n \tgit show-index <\"$1\" >tmp-object-list &&\n \tcut -d' ' -f2 tmp-object-list | sort &&\n \trm tmp-object-list\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 93c5763e7d..faa739eeb9 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -114,12 +114,12 @@ test_expect_success '--local disables writing bitmaps when connected to alternat\n \t(\n \t\tcd member &&\n \t\ttest_commit \"object\" &&\n-\t\tgit repack -Adl --write-bitmap-index 2>err &&\n+\t\tGIT_TEST_MULTI_PACK_INDEX=0 git repack -Adl --write-bitmap-index 2>err &&\n \t\tcat >expect <<-EOF &&\n \t\twarning: disabling bitmap writing, as some objects are not being packed\n \t\tEOF\n \t\ttest_cmp expect err &&\n-\t\ttest ! -f .git/objects/pack-*.bitmap\n+\t\ttest_path_is_missing .git/objects/pack-*.bitmap\n \t)\n '\n \ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 33d7977fca..00f28fb558 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -10,9 +10,9 @@ objdir=.git/objects\n packdir=$objdir/pack\n midx=$objdir/pack/multi-pack-index\n \n-packed_objects() {\n+packed_objects () {\n \tgit show-index <\"$1\" >tmp-object-list &&\n-\tcut -d' ' -f2 tmp-object-list &&\n+\tcut -d' ' -f2 tmp-object-list | sort &&\n \trm tmp-object-list\n }\n \n@@ -312,7 +312,7 @@ test_expect_success '--geometric --write-midx with packfiles in main and alterna\n \n \t# We should also see a multi-pack-index. This multi-pack-index should\n \t# never refer to any packfiles in the alternate object database.\n-\ttest -f member/.git/objects/pack/multi-pack-index &&\n+\ttest_path_is_file member/.git/objects/pack/multi-pack-index &&\n \ttest-tool read-midx member/.git/objects >packs.midx &&\n \tgrep \"^pack-.*\\.idx$\" packs.midx | sort >actual &&\n \tbasename member/.git/objects/pack/pack-*.idx >expect &&\n@@ -378,11 +378,11 @@ test_expect_success '--geometric -l with non-intact geometric sequence across OD\n \n \t# Verify that our assumptions actually hold: both generated packfiles\n \t# should have three objects and should be non-equal.\n-\tpacked_objects shared/.git/objects/pack/pack-*.idx >packed-objects &&\n-\ttest_line_count = 3 packed-objects &&\n-\tpacked_objects member/.git/objects/pack/pack-*.idx >packed-objetcs &&\n-\ttest_line_count = 3 packed-objects &&\n-\ttest \"$(basename member/.git/objects/pack/pack-*.pack)\" != \"$(basename shared/.git/objects/pack/pack-*.pack)\" &&\n+\tpacked_objects shared/.git/objects/pack/pack-*.idx >shared-objects &&\n+\tpacked_objects member/.git/objects/pack/pack-*.idx >member-objects &&\n+\ttest_line_count = 3 shared-objects &&\n+\ttest_line_count = 3 member-objects &&\n+\t! test_cmp shared-objects member-objects &&\n \n \t# Perform the geometric repack. With `-l`, we should only see the local\n \t# packfile and thus arrive at the conclusion that the geometric\n@@ -415,8 +415,7 @@ test_expect_success '--geometric -l with non-intact geometric sequence across OD\n \ttest_line_count = 1 actual &&\n \tpacked_objects member/.git/objects/pack/pack-*.idx >actual-objects &&\n \ttest_line_count = 6 actual-objects &&\n-\tsort actual-objects.sorted &&\n-\ttest_cmp expected-objects actual-objects.sorted\n+\ttest_cmp expected-objects actual-objects\n '\n \n test_expect_success '--geometric -l disables writing bitmaps with non-local packfiles' '\n@@ -428,7 +427,7 @@ test_expect_success '--geometric -l disables writing bitmaps with non-local pack\n \tgit clone --shared shared member &&\n \ttest_commit_bulk -C member --start=2 1 &&\n \n-\t# When performing a geometric repack with `-l` while connecting to an\n+\t# When performing a geometric repack with `-l` while connected to an\n \t# alternate object database that has a packfile we do not have full\n \t# coverage of objects. As a result, we expect that writing the bitmap\n \t# will be disabled.\n@@ -437,13 +436,13 @@ test_expect_success '--geometric -l disables writing bitmaps with non-local pack\n \twarning: disabling bitmap writing, as some objects are not being packed\n \tEOF\n \ttest_cmp expect err &&\n-\ttest ! -f member/.git/objects/pack/multi-pack-index-*.bitmap &&\n+\ttest_path_is_missing member/.git/objects/pack/multi-pack-index-*.bitmap &&\n \n \t# On the other hand, when we repack without `-l`, we should see that\n \t# the bitmap gets created.\n \tgit -C member repack --geometric=2 --write-midx --write-bitmap-index 2>err &&\n \ttest_must_be_empty err &&\n-\ttest -f member/.git/objects/pack/multi-pack-index-*.bitmap\n+\ttest_path_is_file member/.git/objects/pack/multi-pack-index-*.bitmap\n '\n \n test_done\n-- \n2.40.0\n\n"},{"id":"475334","messageId":"a91e795bcb330b1d24c9f30b439552f786a6b4e3.1681452028.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681452028.git.ps@pks.im","subject":"[PATCH v4 01/10] midx: fix segfault with no packs and invalid preferred pack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-14T06:01:31Z","receivedAt":"2023-04-14T06:01:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When asked to write a multi-pack-index the user can specify a preferred\npack that is used as a tie breaker when multiple packs contain the same\nobjects. When this packfile cannot be found, we just pick the first pack\nthat is getting tracked by the newly written multi-pack-index as a\nfallback.\n\nPicking the fallback can fail in the case where we're asked to write a\nmulti-pack-index with no packfiles at all: picking the fallback value\nwill cause a segfault as we blindly index into the array of packfiles,\nwhich would be empty.\n\nFix this bug by resetting the preferred packfile index to `-1` before\nsearching for the preferred pack. This fixes the segfault as we already\ncheck for whether the index is `> - 1`. If it is not, we simply don't\npick a preferred packfile at all.\n\nHelped-by: Taylor Blau \nSigned-off-by: Patrick Steinhardt \n---\n midx.c | 6 +++---\n t/t5319-multi-pack-index.sh | 12 ++++++++++++\n 2 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 47989f7ea7..67eb617591 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1328,17 +1328,17 @@ static int write_midx_internal(const char *object_dir,\n \t}\n \n \tif (preferred_pack_name) {\n-\t\tint found = 0;\n+\t\tctx.preferred_pack_idx = -1;\n+\n \t\tfor (i = 0; i < ctx.nr; i++) {\n \t\t\tif (!cmp_idx_or_pack_name(preferred_pack_name,\n \t\t\t\t\t\t ctx.info[i].pack_name)) {\n \t\t\t\tctx.preferred_pack_idx = i;\n-\t\t\t\tfound = 1;\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n \n-\t\tif (!found)\n+\t\tif (ctx.preferred_pack_idx == -1)\n \t\t\twarning(_(\"unknown preferred pack: '%s'\"),\n \t\t\t\tpreferred_pack_name);\n \t} else if (ctx.nr &&\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 499d5d4c78..0883c7c6bd 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -183,6 +183,18 @@ test_expect_success 'write midx with --stdin-packs' '\n \n compare_results_with_midx \"mixed mode (one pack + extra)\"\n \n+test_expect_success 'write with no objects and preferred pack' '\n+\ttest_when_finished \"rm -rf empty\" &&\n+\tgit init empty &&\n+\ttest_must_fail git -C empty multi-pack-index write \\\n+\t\t--stdin-packs --preferred-pack=does-not-exist err &&\n+\tcat >expect <<-EOF &&\n+\twarning: unknown preferred pack: ${SQ}does-not-exist${SQ}\n+\terror: no pack files to index.\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n test_expect_success 'write progress off for redirected stderr' '\n \tgit multi-pack-index --object-dir=$objdir write 2>err &&\n \ttest_line_count = 0 err\n-- \n2.40.0\n\n"},{"id":"475335","messageId":"975ee38fc49d3a814c17bbe9f5b68f02511783aa.1681452028.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681452028.git.ps@pks.im","subject":"[PATCH v4 03/10] repack: fix generating multi-pack-index with only non-local packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-14T06:01:40Z","receivedAt":"2023-04-14T06:01:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When writing the multi-pack-index with geometric repacking we will add\nall packfiles to the index that are part of the geometric sequence. This\ncan potentially also include packfiles borrowed from an alternate object\ndirectory. But given that a multi-pack-index can only ever include packs\nthat are part of the main object database this does not make much sense\nwhatsoever.\n\nIn the edge case where all packfiles are contained in the alternate\nobject database and the local repository has none itself this bug can\ncause us to invoke git-multi-pack-index(1) with only non-local packfiles\nthat it ultimately cannot find. This causes it to return an error and\nthus causes the geometric repack to fail.\n\nFix the code to skip non-local packfiles.\n\nCo-authored-by: Taylor Blau \nSigned-off-by: Taylor Blau \nSigned-off-by: Patrick Steinhardt \n---\n builtin/repack.c | 11 +++++++++++\n t/t7703-repack-geometric.sh | 23 +++++++++++++++++++++++\n 2 files changed, 34 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 687419776d..80fc860613 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -572,6 +572,17 @@ static void midx_included_packs(struct string_list *include,\n \t\tfor (i = geometry->split; i < geometry->pack_nr; i++) {\n \t\t\tstruct packed_git *p = geometry->pack[i];\n \n+\t\t\t/*\n+\t\t\t * The multi-pack index never refers to packfiles part\n+\t\t\t * of an alternate object database, so we skip these.\n+\t\t\t * While git-multi-pack-index(1) would silently ignore\n+\t\t\t * them anyway, this allows us to skip executing the\n+\t\t\t * command completely when we have only non-local\n+\t\t\t * packfiles.\n+\t\t\t */\n+\t\t\tif (!p->pack_local)\n+\t\t\t\tcontinue;\n+\n \t\t\tstrbuf_addstr(&buf, pack_basename(p));\n \t\t\tstrbuf_strip_suffix(&buf, \".pack\");\n \t\t\tstrbuf_addstr(&buf, \".idx\");\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 4abc7d4c55..9dd002437f 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -313,4 +313,27 @@ test_expect_success '--geometric --write-midx with packfiles in main and alterna\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--geometric --with-midx with no local objects' '\n+\ttest_when_finished \"rm -fr shared member\" &&\n+\n+\t# Create a repository with a single packfile that acts as alternate\n+\t# object database.\n+\tgit init shared &&\n+\ttest_commit -C shared \"shared-objects\" &&\n+\tgit -C shared repack -ad &&\n+\n+\t# Create a second repository linked to the first one and perform a\n+\t# geometric repack on it.\n+\tgit clone --shared shared member &&\n+\tgit -C member repack --geometric 2 --write-midx 2>err &&\n+\ttest_must_be_empty err &&\n+\n+\t# Assert that we wrote neither a new packfile nor a multi-pack-index.\n+\t# We should not have a packfile because the single packfile in the\n+\t# alternate object database does not invalidate the geometric sequence.\n+\t# And we should not have a multi-pack-index because these only index\n+\t# local packfiles, and there are none.\n+\ttest_dir_is_empty member/$packdir\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475336","messageId":"36c3eb65ca1590d0c77dff3e49d6d6f1515d1bc5.1681452028.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681452028.git.ps@pks.im","subject":"[PATCH v4 02/10] repack: fix trying to use preferred pack in alternates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-14T06:01:36Z","receivedAt":"2023-04-14T06:01:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When doing a geometric repack with multi-pack-indices, then we ask\ngit-multi-pack-index(1) to use the largest packfile as the preferred\npack. It can happen though that the largest packfile is not part of the\nmain object database, but instead part of an alternate object database.\nThe result is that git-multi-pack-index(1) will not be able to find the\npreferred pack and print a warning. It then falls back to use the first\npackfile that the multi-pack-index shall reference.\n\nFix this bug by only considering packfiles as preferred pack that are\nlocal. This is the right thing to do given that a multi-pack-index\nshould never reference packfiles borrowed from an alternate.\n\nWhile at it, rename the function `get_largest_active_packfile()` to\n`get_preferred_pack()` to better document its intent.\n\nHelped-by: Taylor Blau \nSigned-off-by: Patrick Steinhardt \n---\n builtin/repack.c | 26 +++++++++++++++++++++-----\n t/t7703-repack-geometric.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 53 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 87f73c8923..687419776d 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -446,8 +446,10 @@ static void split_pack_geometry(struct pack_geometry *geometry, int factor)\n \tgeometry->split = split;\n }\n \n-static struct packed_git *get_largest_active_pack(struct pack_geometry *geometry)\n+static struct packed_git *get_preferred_pack(struct pack_geometry *geometry)\n {\n+\tuint32_t i;\n+\n \tif (!geometry) {\n \t\t/*\n \t\t * No geometry means either an all-into-one repack (in which\n@@ -462,7 +464,21 @@ static struct packed_git *get_largest_active_pack(struct pack_geometry *geometry\n \t}\n \tif (geometry->split == geometry->pack_nr)\n \t\treturn NULL;\n-\treturn geometry->pack[geometry->pack_nr - 1];\n+\n+\t/*\n+\t * The preferred pack is the largest pack above the split line. In\n+\t * other words, it is the largest pack that does not get rolled up in\n+\t * the geometric repack.\n+\t */\n+\tfor (i = geometry->pack_nr; i > geometry->split; i--)\n+\t\t/*\n+\t\t * A pack that is not local would never be included in a\n+\t\t * multi-pack index. We thus skip over any non-local packs.\n+\t\t */\n+\t\tif (geometry->pack[i - 1]->pack_local)\n+\t\t\treturn geometry->pack[i - 1];\n+\n+\treturn NULL;\n }\n \n static void clear_pack_geometry(struct pack_geometry *geometry)\n@@ -589,7 +605,7 @@ static int write_midx_included_packs(struct string_list *include,\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct string_list_item *item;\n-\tstruct packed_git *largest = get_largest_active_pack(geometry);\n+\tstruct packed_git *preferred = get_preferred_pack(geometry);\n \tFILE *in;\n \tint ret;\n \n@@ -610,9 +626,9 @@ static int write_midx_included_packs(struct string_list *include,\n \tif (write_bitmaps)\n \t\tstrvec_push(&cmd.args, \"--bitmap\");\n \n-\tif (largest)\n+\tif (preferred)\n \t\tstrvec_pushf(&cmd.args, \"--preferred-pack=%s\",\n-\t\t\t pack_basename(largest));\n+\t\t\t pack_basename(preferred));\n \n \tif (refs_snapshot)\n \t\tstrvec_pushf(&cmd.args, \"--refs-snapshot=%s\", refs_snapshot);\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 8821fbd2dd..4abc7d4c55 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -281,4 +281,36 @@ test_expect_success '--geometric with pack.packSizeLimit' '\n \t)\n '\n \n+test_expect_success '--geometric --write-midx with packfiles in main and alternate ODB' '\n+\ttest_when_finished \"rm -fr shared member\" &&\n+\n+\t# Create a shared repository that will serve as the alternate object\n+\t# database for the member linked to it. It has got some objects on its\n+\t# own that are packed into a single packfile.\n+\tgit init shared &&\n+\ttest_commit -C shared common-object &&\n+\tgit -C shared repack -ad &&\n+\n+\t# We create member so that its alternates file points to the shared\n+\t# repository. We then create a commit in it so that git-repack(1) has\n+\t# something to repack.\n+\t# of the shared object database.\n+\tgit clone --shared shared member &&\n+\ttest_commit -C member unique-object &&\n+\tgit -C member repack --geometric=2 --write-midx 2>err &&\n+\ttest_must_be_empty err &&\n+\n+\t# We should see that a new packfile was generated.\n+\tfind shared/.git/objects/pack -type f -name \"*.pack\" >packs &&\n+\ttest_line_count = 1 packs &&\n+\n+\t# We should also see a multi-pack-index. This multi-pack-index should\n+\t# never refer to any packfiles in the alternate object database.\n+\ttest_path_is_file member/.git/objects/pack/multi-pack-index &&\n+\ttest-tool read-midx member/.git/objects >packs.midx &&\n+\tgrep \"^pack-.*\\.idx$\" packs.midx | sort >actual &&\n+\tbasename member/.git/objects/pack/pack-*.idx >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475337","messageId":"bdd11864c220a66120704462a41d16935735d0b0.1681452028.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681452028.git.ps@pks.im","subject":"[PATCH v4 04/10] pack-objects: split out `--stdin-packs` tests into separate file","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-14T06:01:45Z","receivedAt":"2023-04-14T06:02:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The test suite for git-pack-objects(1) is quite huge, and we're about to\nadd more tests that relate to the `--stdin-packs` option. Split out all\ntests related to this option into a standalone file so that it becomes\neasier to test the feature in isolation.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5300-pack-object.sh | 135 -------------------------------\n t/t5331-pack-objects-stdin.sh | 145 ++++++++++++++++++++++++++++++++++\n 2 files changed, 145 insertions(+), 135 deletions(-)\n create mode 100755 t/t5331-pack-objects-stdin.sh\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex f8a0f309e2..d2ce236d61 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -589,141 +589,6 @@ test_expect_success 'prefetch objects' '\n \ttest_line_count = 1 donelines\n '\n \n-test_expect_success 'setup for --stdin-packs tests' '\n-\tgit init stdin-packs &&\n-\t(\n-\t\tcd stdin-packs &&\n-\n-\t\ttest_commit A &&\n-\t\ttest_commit B &&\n-\t\ttest_commit C &&\n-\n-\t\tfor id in A B C\n-\t\tdo\n-\t\t\tgit pack-objects .git/objects/pack/pack-$id \\\n-\t\t\t\t--incremental --revs <<-EOF || exit 1\n-\t\t\trefs/tags/$id\n-\t\t\tEOF\n-\t\tdone &&\n-\n-\t\tls -la .git/objects/pack\n-\t)\n-'\n-\n-test_expect_success '--stdin-packs with excluded packs' '\n-\t(\n-\t\tcd stdin-packs &&\n-\n-\t\tPACK_A=\"$(basename .git/objects/pack/pack-A-*.pack)\" &&\n-\t\tPACK_B=\"$(basename .git/objects/pack/pack-B-*.pack)\" &&\n-\t\tPACK_C=\"$(basename .git/objects/pack/pack-C-*.pack)\" &&\n-\n-\t\tgit pack-objects test --stdin-packs <<-EOF &&\n-\t\t$PACK_A\n-\t\t^$PACK_B\n-\t\t$PACK_C\n-\t\tEOF\n-\n-\t\t(\n-\t\t\tgit show-index <$(ls .git/objects/pack/pack-A-*.idx) &&\n-\t\t\tgit show-index <$(ls .git/objects/pack/pack-C-*.idx)\n-\t\t) >expect.raw &&\n-\t\tgit show-index <$(ls test-*.idx) >actual.raw &&\n-\n-\t\tcut -d\" \" -f2 expect &&\n-\t\tcut -d\" \" -f2 actual &&\n-\t\ttest_cmp expect actual\n-\t)\n-'\n-\n-test_expect_success '--stdin-packs is incompatible with --filter' '\n-\t(\n-\t\tcd stdin-packs &&\n-\t\ttest_must_fail git pack-objects --stdin-packs --stdout \\\n-\t\t\t--filter=blob:none err &&\n-\t\ttest_i18ngrep \"cannot use --filter with --stdin-packs\" err\n-\t)\n-'\n-\n-test_expect_success '--stdin-packs is incompatible with --revs' '\n-\t(\n-\t\tcd stdin-packs &&\n-\t\ttest_must_fail git pack-objects --stdin-packs --revs out \\\n-\t\t\terr &&\n-\t\ttest_i18ngrep \"cannot use internal rev list with --stdin-packs\" err\n-\t)\n-'\n-\n-test_expect_success '--stdin-packs with loose objects' '\n-\t(\n-\t\tcd stdin-packs &&\n-\n-\t\tPACK_A=\"$(basename .git/objects/pack/pack-A-*.pack)\" &&\n-\t\tPACK_B=\"$(basename .git/objects/pack/pack-B-*.pack)\" &&\n-\t\tPACK_C=\"$(basename .git/objects/pack/pack-C-*.pack)\" &&\n-\n-\t\ttest_commit D && # loose\n-\n-\t\tgit pack-objects test2 --stdin-packs --unpacked <<-EOF &&\n-\t\t$PACK_A\n-\t\t^$PACK_B\n-\t\t$PACK_C\n-\t\tEOF\n-\n-\t\t(\n-\t\t\tgit show-index <$(ls .git/objects/pack/pack-A-*.idx) &&\n-\t\t\tgit show-index <$(ls .git/objects/pack/pack-C-*.idx) &&\n-\t\t\tgit rev-list --objects --no-object-names \\\n-\t\t\t\trefs/tags/C..refs/tags/D\n-\n-\t\t) >expect.raw &&\n-\t\tls -la . &&\n-\t\tgit show-index <$(ls test2-*.idx) >actual.raw &&\n-\n-\t\tcut -d\" \" -f2 expect &&\n-\t\tcut -d\" \" -f2 actual &&\n-\t\ttest_cmp expect actual\n-\t)\n-'\n-\n-test_expect_success '--stdin-packs with broken links' '\n-\t(\n-\t\tcd stdin-packs &&\n-\n-\t\t# make an unreachable object with a bogus parent\n-\t\tgit cat-file -p HEAD >commit &&\n-\t\tsed \"s/$(git rev-parse HEAD^)/$(test_oid zero)/\" in &&\n-\n-\t\tgit pack-objects .git/objects/pack/pack-D expect.raw &&\n-\t\tgit show-index <$(ls test3-*.idx) >actual.raw &&\n-\n-\t\tcut -d\" \" -f2 expect &&\n-\t\tcut -d\" \" -f2 actual &&\n-\t\ttest_cmp expect actual\n-\t)\n-'\n-\n test_expect_success 'negative window clamps to 0' '\n \tgit pack-objects --progress --window=-1 neg-window stderr &&\n \tcheck_deltas stderr = 0\ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nnew file mode 100755\nindex 0000000000..d5eece5899\n--- /dev/null\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -0,0 +1,145 @@\n+#!/bin/sh\n+\n+test_description='pack-objects --stdin'\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'setup for --stdin-packs tests' '\n+\tgit init stdin-packs &&\n+\t(\n+\t\tcd stdin-packs &&\n+\n+\t\ttest_commit A &&\n+\t\ttest_commit B &&\n+\t\ttest_commit C &&\n+\n+\t\tfor id in A B C\n+\t\tdo\n+\t\t\tgit pack-objects .git/objects/pack/pack-$id \\\n+\t\t\t\t--incremental --revs <<-EOF || exit 1\n+\t\t\trefs/tags/$id\n+\t\t\tEOF\n+\t\tdone &&\n+\n+\t\tls -la .git/objects/pack\n+\t)\n+'\n+\n+test_expect_success '--stdin-packs with excluded packs' '\n+\t(\n+\t\tcd stdin-packs &&\n+\n+\t\tPACK_A=\"$(basename .git/objects/pack/pack-A-*.pack)\" &&\n+\t\tPACK_B=\"$(basename .git/objects/pack/pack-B-*.pack)\" &&\n+\t\tPACK_C=\"$(basename .git/objects/pack/pack-C-*.pack)\" &&\n+\n+\t\tgit pack-objects test --stdin-packs <<-EOF &&\n+\t\t$PACK_A\n+\t\t^$PACK_B\n+\t\t$PACK_C\n+\t\tEOF\n+\n+\t\t(\n+\t\t\tgit show-index <$(ls .git/objects/pack/pack-A-*.idx) &&\n+\t\t\tgit show-index <$(ls .git/objects/pack/pack-C-*.idx)\n+\t\t) >expect.raw &&\n+\t\tgit show-index <$(ls test-*.idx) >actual.raw &&\n+\n+\t\tcut -d\" \" -f2 expect &&\n+\t\tcut -d\" \" -f2 actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--stdin-packs is incompatible with --filter' '\n+\t(\n+\t\tcd stdin-packs &&\n+\t\ttest_must_fail git pack-objects --stdin-packs --stdout \\\n+\t\t\t--filter=blob:none err &&\n+\t\ttest_i18ngrep \"cannot use --filter with --stdin-packs\" err\n+\t)\n+'\n+\n+test_expect_success '--stdin-packs is incompatible with --revs' '\n+\t(\n+\t\tcd stdin-packs &&\n+\t\ttest_must_fail git pack-objects --stdin-packs --revs out \\\n+\t\t\terr &&\n+\t\ttest_i18ngrep \"cannot use internal rev list with --stdin-packs\" err\n+\t)\n+'\n+\n+test_expect_success '--stdin-packs with loose objects' '\n+\t(\n+\t\tcd stdin-packs &&\n+\n+\t\tPACK_A=\"$(basename .git/objects/pack/pack-A-*.pack)\" &&\n+\t\tPACK_B=\"$(basename .git/objects/pack/pack-B-*.pack)\" &&\n+\t\tPACK_C=\"$(basename .git/objects/pack/pack-C-*.pack)\" &&\n+\n+\t\ttest_commit D && # loose\n+\n+\t\tgit pack-objects test2 --stdin-packs --unpacked <<-EOF &&\n+\t\t$PACK_A\n+\t\t^$PACK_B\n+\t\t$PACK_C\n+\t\tEOF\n+\n+\t\t(\n+\t\t\tgit show-index <$(ls .git/objects/pack/pack-A-*.idx) &&\n+\t\t\tgit show-index <$(ls .git/objects/pack/pack-C-*.idx) &&\n+\t\t\tgit rev-list --objects --no-object-names \\\n+\t\t\t\trefs/tags/C..refs/tags/D\n+\n+\t\t) >expect.raw &&\n+\t\tls -la . &&\n+\t\tgit show-index <$(ls test2-*.idx) >actual.raw &&\n+\n+\t\tcut -d\" \" -f2 expect &&\n+\t\tcut -d\" \" -f2 actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success '--stdin-packs with broken links' '\n+\t(\n+\t\tcd stdin-packs &&\n+\n+\t\t# make an unreachable object with a bogus parent\n+\t\tgit cat-file -p HEAD >commit &&\n+\t\tsed \"s/$(git rev-parse HEAD^)/$(test_oid zero)/\" in &&\n+\n+\t\tgit pack-objects .git/objects/pack/pack-D expect.raw &&\n+\t\tgit show-index <$(ls test3-*.idx) >actual.raw &&\n+\n+\t\tcut -d\" \" -f2 expect &&\n+\t\tcut -d\" \" -f2 actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_done\n-- \n2.40.0\n\n"},{"id":"475338","messageId":"7881828de7fa86a7d7e169b1ed67765334a814f4.1681452028.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681452028.git.ps@pks.im","subject":"[PATCH v4 05/10] pack-objects: fix error when packing same pack twice","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-14T06:01:49Z","receivedAt":"2023-04-14T06:02:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When passed the same packfile twice via `--stdin-packs` we return an\nerror that the packfile supposedly was not found. This is because when\nreading packs into the list of included or excluded packfiles, we will\nhappily re-add packfiles even if they are part of the lists already. And\nwhile the list can now contain duplicates, we will only set the `util`\npointer of the first list entry to the `packed_git` structure. We notice\nthat at a later point when checking that all list entries have their\n`util` pointer set and die with an error.\n\nWhile this is kind of a nonsensical request, this scenario can be hit\nwhen doing geometric repacks. When a repository is connected to an\nalternate object directory and both have the exact same packfile then\nboth would get added to the geometric sequence. And when we then decide\nto perform the repack, we will invoke git-pack-objects(1) with the same\npackfile twice.\n\nFix this bug by removing any duplicates from both the included and\nexcluded packs.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-objects.c | 2 ++\n t/t5331-pack-objects-stdin.sh | 27 +++++++++++++++++++++++++++\n t/t7703-repack-geometric.sh | 25 +++++++++++++++++++++++++\n 3 files changed, 54 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 7e9e20172a..7d0e864c35 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3348,7 +3348,9 @@ static void read_packs_list_from_stdin(void)\n \t}\n \n \tstring_list_sort(&include_packs);\n+\tstring_list_remove_duplicates(&include_packs, 0);\n \tstring_list_sort(&exclude_packs);\n+\tstring_list_remove_duplicates(&exclude_packs, 0);\n \n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tconst char *pack_name = pack_basename(p);\ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex d5eece5899..71c8a4a635 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -7,6 +7,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+packed_objects () {\n+\tgit show-index <\"$1\" >tmp-object-list &&\n+\tcut -d' ' -f2 tmp-object-list | sort &&\n+\trm tmp-object-list\n+ }\n+\n test_expect_success 'setup for --stdin-packs tests' '\n \tgit init stdin-packs &&\n \t(\n@@ -142,4 +148,25 @@ test_expect_success '--stdin-packs with broken links' '\n \t)\n '\n \n+test_expect_success 'pack-objects --stdin with duplicate packfile' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit \"commit\" &&\n+\t\tgit repack -ad &&\n+\n+\t\t{\n+\t\t\tbasename .git/objects/pack/pack-*.pack &&\n+\t\t\tbasename .git/objects/pack/pack-*.pack\n+\t\t} >packfiles &&\n+\n+\t\tgit pack-objects --stdin-packs generated-pack expect &&\n+\t\tpacked_objects generated-pack-*.idx >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 9dd002437f..a358dfb7bd 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -336,4 +336,29 @@ test_expect_success '--geometric --with-midx with no local objects' '\n \ttest_dir_is_empty member/$packdir\n '\n \n+test_expect_success '--geometric with same pack in main and alternate ODB' '\n+\ttest_when_finished \"rm -fr shared member\" &&\n+\n+\t# Create a repository with a single packfile that acts as alternate\n+\t# object database.\n+\tgit init shared &&\n+\ttest_commit -C shared \"shared-objects\" &&\n+\tgit -C shared repack -ad &&\n+\n+\t# We create the member repository as an exact copy so that it has the\n+\t# same packfile.\n+\tcp -r shared member &&\n+\ttest-tool path-utils real_path shared/.git/objects >member/.git/objects/info/alternates &&\n+\tfind shared/.git/objects -type f >expected-files &&\n+\n+\t# Verify that we can repack objects as expected without observing any\n+\t# error. Having the same packfile in both ODBs used to cause an error\n+\t# in git-pack-objects(1).\n+\tgit -C member repack --geometric 2 2>err &&\n+\ttest_must_be_empty err &&\n+\t# Nothing should have changed.\n+\tfind shared/.git/objects -type f >actual-files &&\n+\ttest_cmp expected-files actual-files\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475339","messageId":"33aea4c71edf60094e01ef245429befe1142c0ea.1681452028.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681452028.git.ps@pks.im","subject":"[PATCH v4 06/10] pack-objects: fix error when same packfile is included and excluded","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-14T06:01:54Z","receivedAt":"2023-04-14T06:02:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When passing the same packfile both as included and excluded via the\n`--stdin-packs` option, then we will return an error because the\nexcluded packfile cannot be found. This is because we will only set the\n`util` pointer for the included packfile list if it was found, so that\nwe later die when we notice that it's in fact not set for the excluded\npackfile list.\n\nFix this bug by always setting the `util` pointer for both the included\nand excluded list entries.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/pack-objects.c | 8 +++-----\n t/t5331-pack-objects-stdin.sh | 20 ++++++++++++++++++++\n 2 files changed, 23 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 7d0e864c35..c5147d392f 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3355,11 +3355,9 @@ static void read_packs_list_from_stdin(void)\n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tconst char *pack_name = pack_basename(p);\n \n-\t\titem = string_list_lookup(&include_packs, pack_name);\n-\t\tif (!item)\n-\t\t\titem = string_list_lookup(&exclude_packs, pack_name);\n-\n-\t\tif (item)\n+\t\tif ((item = string_list_lookup(&include_packs, pack_name)))\n+\t\t\titem->util = p;\n+\t\tif ((item = string_list_lookup(&exclude_packs, pack_name)))\n \t\t\titem->util = p;\n \t}\n \ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex 71c8a4a635..3ef736ec05 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -169,4 +169,24 @@ test_expect_success 'pack-objects --stdin with duplicate packfile' '\n \t)\n '\n \n+test_expect_success 'pack-objects --stdin with same packfile excluded and included' '\n+\ttest_when_finished \"rm -fr repo\" &&\n+\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit \"commit\" &&\n+\t\tgit repack -ad &&\n+\n+\t\t{\n+\t\t\tbasename .git/objects/pack/pack-*.pack &&\n+\t\t\tprintf \"^%s\\n\" \"$(basename .git/objects/pack/pack-*.pack)\"\n+\t\t} >packfiles &&\n+\n+\t\tgit pack-objects --stdin-packs generated-pack packed-objects &&\n+\t\ttest_must_be_empty packed-objects\n+\t)\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475340","messageId":"f9d490f5b8bb41b93543bff46ea3d446fa07c2ee.1681452028.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681452028.git.ps@pks.im","subject":"[PATCH v4 07/10] pack-objects: extend test coverage of `--stdin-packs` with alternates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-14T06:01:59Z","receivedAt":"2023-04-14T06:02:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We don't have any tests that verify that git-pack-objects(1) works with\n`--stdin-packs` when combined with alternate object directories. Add\nsome to make sure that the basic functionality works as expected.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5331-pack-objects-stdin.sh | 48 +++++++++++++++++++++++++++++++++++\n 1 file changed, 48 insertions(+)\n\ndiff --git a/t/t5331-pack-objects-stdin.sh b/t/t5331-pack-objects-stdin.sh\nindex 3ef736ec05..acab31667a 100755\n--- a/t/t5331-pack-objects-stdin.sh\n+++ b/t/t5331-pack-objects-stdin.sh\n@@ -189,4 +189,52 @@ test_expect_success 'pack-objects --stdin with same packfile excluded and includ\n \t)\n '\n \n+test_expect_success 'pack-objects --stdin with packfiles from alternate object database' '\n+\ttest_when_finished \"rm -fr shared member\" &&\n+\n+\t# Set up a shared repository with a single packfile.\n+\tgit init shared &&\n+\ttest_commit -C shared \"shared-objects\" &&\n+\tgit -C shared repack -ad &&\n+\tbasename shared/.git/objects/pack/pack-*.pack >packfile &&\n+\n+\t# Set up a repository that is connected to the shared repository. This\n+\t# repository has no objects on its own, but we still expect to be able\n+\t# to pack objects from its alternate.\n+\tgit clone --shared shared member &&\n+\tgit -C member pack-objects --stdin-packs generated-pack packfiles &&\n+\n+\t{\n+\t\tpacked_objects shared/.git/objects/pack/pack-*.idx &&\n+\t\tpacked_objects member/.git/objects/pack/pack-*.idx\n+\t} | sort >expected-objects &&\n+\n+\tgit -C member pack-objects --stdin-packs generated-pack actual-objects &&\n+\ttest_cmp expected-objects actual-objects\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475341","messageId":"548ac777a9dd317c1def68a9b5569bbf889b20fe.1681452028.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681452028.git.ps@pks.im","subject":"[PATCH v4 08/10] t/helper: allow chmtime to print verbosely without modifying mtime","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-14T06:02:04Z","receivedAt":"2023-04-14T06:02:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `test-tool chmtime` helper allows us to both read and modify the\nmodification time of files. But while it is possible to only read the\nmtimes of a file via `--get`, it is not possible to read the mtimes\nand report them together with their respective file paths via the\n`--verbose` flag without also modifying the mtime at the same time.\n\nFix this so that it is possible to call `test-tool chmtime --verbose\n...` without modifying any mtimes.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/helper/test-chmtime.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/helper/test-chmtime.c b/t/helper/test-chmtime.c\nindex dc28890a18..0e5538833a 100644\n--- a/t/helper/test-chmtime.c\n+++ b/t/helper/test-chmtime.c\n@@ -94,7 +94,7 @@ int cmd__chmtime(int argc, const char **argv)\n \tif (timespec_arg(argv[i], &set_time, &set_eq)) {\n \t\t++i;\n \t} else {\n-\t\tif (get == 0) {\n+\t\tif (get == 0 && verbose == 0) {\n \t\t\tfprintf(stderr, \"Not a base-10 integer: %s\\n\", argv[i] + 1);\n \t\t\tgoto usage;\n \t\t}\n-- \n2.40.0\n\n"},{"id":"475342","messageId":"c512b58ae02832b1962ab165065fa20b7ca9c790.1681452028.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681452028.git.ps@pks.im","subject":"[PATCH v4 09/10] repack: honor `-l` when calculating pack geometry","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-14T06:02:08Z","receivedAt":"2023-04-14T06:02:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When the user passes `-l` to git-repack(1), then they essentially ask us\nto only repack objects part of the local object database while ignoring\nany packfiles part of an alternate object database. And we in fact honor\nthis bit when doing a geometric repack as the resulting packfile will\nonly ever contain local objects.\n\nWhat we're missing though is that we don't take locality of packfiles\ninto account when computing whether the geometric sequence is intact or\nnot. So even though we would only ever roll up local packfiles anyway,\nwe could end up trying to repack because of non-local packfiles. This\ndoes not make much sense, and in the worst case it can cause us to try\nand do the geometric repack over and over again because we're never able\nto restore the geometric sequence.\n\nFix this bug by honoring whether the user has passed `-l`. If so, we\nskip adding any non-local packfiles to the pack geometry.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/repack.c | 13 +++++++--\n t/t7703-repack-geometric.sh | 57 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 68 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 80fc860613..5768aeb1f3 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -323,7 +323,8 @@ static int geometry_cmp(const void *va, const void *vb)\n }\n \n static void init_pack_geometry(struct pack_geometry **geometry_p,\n-\t\t\t struct string_list *existing_kept_packs)\n+\t\t\t struct string_list *existing_kept_packs,\n+\t\t\t const struct pack_objects_args *args)\n {\n \tstruct packed_git *p;\n \tstruct pack_geometry *geometry;\n@@ -333,6 +334,14 @@ static void init_pack_geometry(struct pack_geometry **geometry_p,\n \tgeometry = *geometry_p;\n \n \tfor (p = get_all_packs(the_repository); p; p = p->next) {\n+\t\tif (args->local && !p->pack_local)\n+\t\t\t/*\n+\t\t\t * When asked to only repack local packfiles we skip\n+\t\t\t * over any packfiles that are borrowed from alternate\n+\t\t\t * object directories.\n+\t\t\t */\n+\t\t\tcontinue;\n+\n \t\tif (!pack_kept_objects) {\n \t\t\t/*\n \t\t\t * Any pack that has its pack_keep bit set will appear\n@@ -900,7 +909,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (geometric_factor) {\n \t\tif (pack_everything)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--geometric\", \"-A/-a\");\n-\t\tinit_pack_geometry(&geometry, &existing_kept_packs);\n+\t\tinit_pack_geometry(&geometry, &existing_kept_packs, &po_args);\n \t\tsplit_pack_geometry(geometry, geometric_factor);\n \t}\n \ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex a358dfb7bd..be8fe78448 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -10,6 +10,12 @@ objdir=.git/objects\n packdir=$objdir/pack\n midx=$objdir/pack/multi-pack-index\n \n+packed_objects () {\n+\tgit show-index <\"$1\" >tmp-object-list &&\n+\tcut -d' ' -f2 tmp-object-list | sort &&\n+\trm tmp-object-list\n+ }\n+\n test_expect_success '--geometric with no packs' '\n \tgit init geometric &&\n \ttest_when_finished \"rm -fr geometric\" &&\n@@ -361,4 +367,55 @@ test_expect_success '--geometric with same pack in main and alternate ODB' '\n \ttest_cmp expected-files actual-files\n '\n \n+test_expect_success '--geometric -l with non-intact geometric sequence across ODBs' '\n+\ttest_when_finished \"rm -fr shared member\" &&\n+\n+\tgit init shared &&\n+\ttest_commit_bulk -C shared --start=1 1 &&\n+\n+\tgit clone --shared shared member &&\n+\ttest_commit_bulk -C member --start=2 1 &&\n+\n+\t# Verify that our assumptions actually hold: both generated packfiles\n+\t# should have three objects and should be non-equal.\n+\tpacked_objects shared/.git/objects/pack/pack-*.idx >shared-objects &&\n+\tpacked_objects member/.git/objects/pack/pack-*.idx >member-objects &&\n+\ttest_line_count = 3 shared-objects &&\n+\ttest_line_count = 3 member-objects &&\n+\t! test_cmp shared-objects member-objects &&\n+\n+\t# Perform the geometric repack. With `-l`, we should only see the local\n+\t# packfile and thus arrive at the conclusion that the geometric\n+\t# sequence is intact. We thus expect no changes.\n+\t#\n+\t# Note that we are tweaking mtimes of the packfiles so that we can\n+\t# verify they did not change. This is done in order to detect the case\n+\t# where we do repack objects, but the resulting packfile is the same.\n+\ttest-tool chmtime --verbose =0 member/.git/objects/pack/* >expected-member-packs &&\n+\tgit -C member repack --geometric=2 -l -d &&\n+\ttest-tool chmtime --verbose member/.git/objects/pack/* >actual-member-packs &&\n+\ttest_cmp expected-member-packs actual-member-packs &&\n+\n+\t{\n+\t\tpacked_objects shared/.git/objects/pack/pack-*.idx &&\n+\t\tpacked_objects member/.git/objects/pack/pack-*.idx\n+\t} | sort >expected-objects &&\n+\n+\t# On the other hand, when doing a non-local geometric repack we should\n+\t# see both packfiles and thus repack them. We expect that the shared\n+\t# object database was not changed.\n+\ttest-tool chmtime --verbose =0 shared/.git/objects/pack/* >expected-shared-packs &&\n+\tgit -C member repack --geometric=2 -d &&\n+\ttest-tool chmtime --verbose shared/.git/objects/pack/* >actual-shared-packs &&\n+\ttest_cmp expected-shared-packs actual-shared-packs &&\n+\n+\t# Furthermore, we expect that the member repository now has a single\n+\t# packfile that contains the combined shared and non-shared objects.\n+\tls member/.git/objects/pack/pack-*.idx >actual &&\n+\ttest_line_count = 1 actual &&\n+\tpacked_objects member/.git/objects/pack/pack-*.idx >actual-objects &&\n+\ttest_line_count = 6 actual-objects &&\n+\ttest_cmp expected-objects actual-objects\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475343","messageId":"03503f9fc19523ce999f2baa20120c3f178f8d1a.1681452028.git.ps@pks.im","threadId":"59535","inReplyTo":"cover.1681452028.git.ps@pks.im","subject":"[PATCH v4 10/10] repack: disable writing bitmaps when doing a local repack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-14T06:02:12Z","receivedAt":"2023-04-14T06:02:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In order to write a bitmap, we need to have full coverage of all objects\nthat are about to be packed. In the traditional non-multi-pack-index\nworld this meant we need to do a full repack of all objects into a\nsingle packfile. But in the new multi-pack-index world we can get away\nwith writing bitmaps when we have multiple packfiles as long as the\nmulti-pack-index covers all objects.\n\nThis is not always the case though. When asked to perform a repack of\nlocal objects, only, then we cannot guarantee to have full coverage of\nall objects regardless of whether we do a full repack or a repack with a\nmulti-pack-index. The end result is that writing the bitmap will fail in\nboth worlds:\n\n $ git multi-pack-index write --stdin-packs --bitmap \n---\n builtin/repack.c | 12 ++++++++++++\n object-file.c | 6 ++++++\n object-store.h | 1 +\n t/t7700-repack.sh | 17 +++++++++++++++++\n t/t7703-repack-geometric.sh | 27 +++++++++++++++++++++++++++\n 5 files changed, 63 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 5768aeb1f3..e2abcea2ee 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -887,6 +887,18 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (write_bitmaps && !(pack_everything & ALL_INTO_ONE) && !write_midx)\n \t\tdie(_(incremental_bitmap_conflict_error));\n \n+\tif (write_bitmaps && po_args.local && has_alt_odb(the_repository)) {\n+\t\t/*\n+\t\t * When asked to do a local repack, but we have\n+\t\t * packfiles that are inherited from an alternate, then\n+\t\t * we cannot guarantee that the multi-pack-index would\n+\t\t * have full coverage of all objects. We thus disable\n+\t\t * writing bitmaps in that case.\n+\t\t */\n+\t\twarning(_(\"disabling bitmap writing, as some objects are not being packed\"));\n+\t\twrite_bitmaps = 0;\n+\t}\n+\n \tif (write_midx && write_bitmaps) {\n \t\tstruct strbuf path = STRBUF_INIT;\n \ndiff --git a/object-file.c b/object-file.c\nindex 8fab8dbe80..3ad11c683b 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -946,6 +946,12 @@ void prepare_alt_odb(struct repository *r)\n \tr->objects->loaded_alternates = 1;\n }\n \n+int has_alt_odb(struct repository *r)\n+{\n+\tprepare_alt_odb(r);\n+\treturn !!r->objects->odb->next;\n+}\n+\n /* Returns 1 if we have successfully freshened the file, 0 otherwise. */\n static int freshen_file(const char *fn)\n {\ndiff --git a/object-store.h b/object-store.h\nindex 82201ec3e7..574f16140f 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -56,6 +56,7 @@ KHASH_INIT(odb_path_map, const char * /* key: odb_path */,\n \tstruct object_directory *, 1, fspathhash, fspatheq)\n \n void prepare_alt_odb(struct repository *r);\n+int has_alt_odb(struct repository *r);\n char *compute_alternate_path(const char *path, struct strbuf *err);\n struct object_directory *find_odb(struct repository *r, const char *obj_dir);\n typedef int alt_odb_fn(struct object_directory *, void *);\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 4aabe98139..faa739eeb9 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -106,6 +106,23 @@ test_expect_success SYMLINKS '--local keeps packs when alternate is objectdir '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--local disables writing bitmaps when connected to alternate ODB' '\n+\ttest_when_finished \"rm -rf shared member\" &&\n+\n+\tgit init shared &&\n+\tgit clone --shared shared member &&\n+\t(\n+\t\tcd member &&\n+\t\ttest_commit \"object\" &&\n+\t\tGIT_TEST_MULTI_PACK_INDEX=0 git repack -Adl --write-bitmap-index 2>err &&\n+\t\tcat >expect <<-EOF &&\n+\t\twarning: disabling bitmap writing, as some objects are not being packed\n+\t\tEOF\n+\t\ttest_cmp expect err &&\n+\t\ttest_path_is_missing .git/objects/pack-*.bitmap\n+\t)\n+'\n+\n test_expect_success 'packed obs in alt ODB are repacked even when local repo is packless' '\n \tmkdir alt_objects/pack &&\n \tmv .git/objects/pack/* alt_objects/pack &&\ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex be8fe78448..00f28fb558 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -418,4 +418,31 @@ test_expect_success '--geometric -l with non-intact geometric sequence across OD\n \ttest_cmp expected-objects actual-objects\n '\n \n+test_expect_success '--geometric -l disables writing bitmaps with non-local packfiles' '\n+\ttest_when_finished \"rm -fr shared member\" &&\n+\n+\tgit init shared &&\n+\ttest_commit_bulk -C shared --start=1 1 &&\n+\n+\tgit clone --shared shared member &&\n+\ttest_commit_bulk -C member --start=2 1 &&\n+\n+\t# When performing a geometric repack with `-l` while connected to an\n+\t# alternate object database that has a packfile we do not have full\n+\t# coverage of objects. As a result, we expect that writing the bitmap\n+\t# will be disabled.\n+\tgit -C member repack -l --geometric=2 --write-midx --write-bitmap-index 2>err &&\n+\tcat >expect <<-EOF &&\n+\twarning: disabling bitmap writing, as some objects are not being packed\n+\tEOF\n+\ttest_cmp expect err &&\n+\ttest_path_is_missing member/.git/objects/pack/multi-pack-index-*.bitmap &&\n+\n+\t# On the other hand, when we repack without `-l`, we should see that\n+\t# the bitmap gets created.\n+\tgit -C member repack --geometric=2 --write-midx --write-bitmap-index 2>err &&\n+\ttest_must_be_empty err &&\n+\ttest_path_is_file member/.git/objects/pack/multi-pack-index-*.bitmap\n+'\n+\n test_done\n-- \n2.40.0\n\n"},{"id":"475377","messageId":"15437c50-d3a1-1e34-b854-dc553e835cce@github.com","threadId":"59535","inReplyTo":"cover.1681452028.git.ps@pks.im","subject":"Re: [PATCH v4 00/10] repack: fix geometric repacking with gitalternates","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-14T13:23:46Z","receivedAt":"2023-04-14T13:23:55Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/14/2023 2:01 AM, Patrick Steinhardt wrote:\n> Hi,\n> \n> this is the fourth version of my patch series to fix geometric repacking\n> with repositories connected to an alternate object database.\n> \n> This version only addresses some issues with the tests, the actual logic\n> remains untouched:\n> \n> - The test added in t7700-repack.sh that verifies that `--local`\n> causes us to disable generation of the bitmap index was failing in\n> the linux-TEST-vars CI job. This was because it sets\n> GIT_TEST_MULTI_PACK_INDEX=1, which causes us to disable the bitmap\n> logic in git-repack(1). I've fixed this failure by explicitly\n> overriding the environment variable like other tests in the same\n> file do.\n> \n> - I've converted path checks to use `test_path_is_missing` and\n> `test_path_is_file` instead of `test ! -f` and `test -f`.\n> \n> - I've fixed a typo in t7703-repack-geometric.sh and shifted code\n> around a bit to make the test more readable, following Derrick's\n> suggestion.\n\nThanks for these updates. I'm happy with v4.\n\n-Stolee\n"},{"id":"475393","messageId":"xmqqzg7aqsmr.fsf@gitster.g","threadId":"59535","inReplyTo":"15437c50-d3a1-1e34-b854-dc553e835cce@github.com","subject":"Re: [PATCH v4 00/10] repack: fix geometric repacking with gitalternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-14T17:29:48Z","receivedAt":"2023-04-14T17:30:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 4/14/2023 2:01 AM, Patrick Steinhardt wrote:\n>> Hi,\n>> \n>> this is the fourth version of my patch series to fix geometric repacking\n>> with repositories connected to an alternate object database.\n>> \n>> This version only addresses some issues with the tests, the actual logic\n>> remains untouched:\n>> \n>> - The test added in t7700-repack.sh that verifies that `--local`\n>> causes us to disable generation of the bitmap index was failing in\n>> the linux-TEST-vars CI job. This was because it sets\n>> GIT_TEST_MULTI_PACK_INDEX=1, which causes us to disable the bitmap\n>> logic in git-repack(1). I've fixed this failure by explicitly\n>> overriding the environment variable like other tests in the same\n>> file do.\n>> \n>> - I've converted path checks to use `test_path_is_missing` and\n>> `test_path_is_file` instead of `test ! -f` and `test -f`.\n>> \n>> - I've fixed a typo in t7703-repack-geometric.sh and shifted code\n>> around a bit to make the test more readable, following Derrick's\n>> suggestion.\n>\n> Thanks for these updates. I'm happy with v4.\n\nYup, the changes since the previous round all look definite\nimprovement, and GIT_TEST_MULTI_PACK_INDEX=1 workaround looks fine,\ntoo.\n\nThanks, all. Will queue. Hopefully we can merge it down to 'next'\nsoonish.\n\n"}]}