{"thread":{"id":"61890","subject":"[PATCH 0/1] revision: fix reachable objects being gc'ed in no blob clone repo","startedAt":"2024-08-02T07:31:58Z","lastAt":"2024-10-23T17:03:55Z","messageCount":65,"participants":["Han Young","Junio C Hamano","韩仰","Jonathan Tan","Calvin Wan","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"499936","messageId":"20240802073143.56731-1-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":null,"subject":"[PATCH 0/1] revision: fix reachable objects being gc'ed in no blob clone repo","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-08-02T07:31:42Z","receivedAt":"2024-08-02T07:31:58Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"We use --filter=blob:none to clone our large monorepo.\nAfter a while we started getting reports from engineers complaining \nthat their local repository was broken. Upon further investigation, \nwe found that broken repositories are missing objects that created \nin that particular local repository. git fsck reports \"bad object: xxx\".\n\nHere are the minimal steps to recreate issue.\n # create a normal git repo, add one file and push to remote\n $ mkdir full && cd full && git init && touch foo\n $ git add foo && git commit -m \"commit 1\" && git push\n\n # partial clone a copy of the repo we just created\n $ cd ..\n $ git clone git@example.org:example/foo.git --filter=blob:none partial\n\n # create a commit in partial cloned repo and push it to remote\n $ cd partial && echo 'hello' > foo && git commit -a -m \"commit 2\"\n $ git push\n\n # run gc in partial repo\n $ git gc --prune=now\n\n # in normal git repo, create another commit on top of the\n # commit we created in partial repo\n $ cd ../full && git pull && echo ' world' >> foo\n $ git commit -a -m \"commit 3\" && git push\n\n # pull from remote in partial repo, and run gc again\n $ cd ../partial && git pull && git gc --prune=now\n\nThe last `git gc` will error out on fsck with error message like this:\n\n error: Could not read d3fbfea9e448461c2b72a79a95a220ae10defd94\n error: Could not read d3fbfea9e448461c2b72a79a95a220ae10defd94\n\nNote that disabling commit graph on the partial repo will cause \n`git gc` to exit normally, but will still not solve the \nunderlying problem. And in more complex situations, \ndisabling commit graph will not avoid the error.\n\nThe problem is caused by the wrong result returned by setup_revision\nwith `--exclude-promisor-objects` enabled.\n`git gc` will call `git repack`, which will call `git pack-objects`\ntwice on a partially cloned repo. The first call to pack-objects \ncombines all the promisor packfiles, and the second pack-objects \ncommand packs all reachable non-promisor objects into a normal packfile.\nHowever, a bug in setup_revision caused some non-promisor objects \nto be mistakenly marked as in promisor packfiles in the second call \nto pack-objects. These incorrectly marked objects are never repacked, \nand were deleted from the object store as a result. In revision.c, \n`process_parents()` recursively marks commit parents as UNINTERESTING \nif the commit itself is UNINTERESTING. `--exclude-promisor-objects` \nis implemented as \"iterate all objects in promisor packfiles, \nmark them as UNINTERESTING\". So when we find a commit object in \na promisor packfile, we also set its ancestors as UNINTERESTING, \nwhether the ancestor is a promisor object or not. In the example above, \n\"commit 2\" is a normal commit object, living in a normal packfile, \nbut marked as a promisor object and gc'ed from the object store.\n\nHan Young (1):\n revision: don't set parents as uninteresting if exclude promisor\n\n revision.c | 2 +-\n t/t0410-partial-clone.sh | 22 +++++++++++++++++++++-\n 2 files changed, 22 insertions(+), 2 deletions(-)\n\n-- \n2.46.0.rc0.107.gae139121ac.dirty\n\n"},{"id":"499937","messageId":"20240802073143.56731-2-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20240802073143.56731-1-hanyang.tony@bytedance.com","subject":"[PATCH 1/1] revision: don't set parents as uninteresting if exclude promisor objects","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-08-02T07:31:43Z","receivedAt":"2024-08-02T07:32:02Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"In revision.c, `process_parents()` recursively marks commit parents \nas UNINTERESTING if the commit itself is UNINTERESTING.\n`--exclude-promisor-objects` is implemented as \n\"iterate all objects in promisor packfiles, mark them as UNINTERESTING\".\nSo when we find a commit object in a promisor packfile, we also set its ancestors \nas UNINTERESTING, whether the ancestor is a promisor object or not.\nThis causes normal objects to be falsely marked as promisor objects \nand removed by `git repack`.\n\nStop setting the parents of uninteresting commits' to UNINTERESTING \nwhen we exclude promisor objects, and add a test to prevent regression.\n\nNote that this change would cause rev-list to report incorrect results if \n`--exclude-promisor-objects` is used with other revision walk filters. But \n`--exclude-promisor-objects` is for internal use only, so we don't have to worry\nabout users using other filters with `--exclude-promisor-objects`.\n\nSigned-off-by: Han Young \nHelped-by: C O Xing Xin \n---\n revision.c | 2 +-\n t/t0410-partial-clone.sh | 22 +++++++++++++++++++++-\n 2 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 1c0192f522..eacb0c909d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1164,7 +1164,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \t * wasn't uninteresting), in which case we need\n \t * to mark its parents recursively too..\n \t */\n-\tif (commit->object.flags & UNINTERESTING) {\n+\tif (!revs->exclude_promisor_objects && commit->object.flags & UNINTERESTING) {\n \t\twhile (parent) {\n \t\t\tstruct commit *p = parent->item;\n \t\t\tparent = parent->next;\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 2c30c86e7b..4ee3437685 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -22,6 +22,17 @@ pack_as_from_promisor () {\n \techo $HASH\n }\n \n+pack_commit() {\n+\tHASH=$(echo $1 | git -C repo pack-objects .git/objects/pack/pack --missing=allow-any) &&\n+\tdelete_object repo $1 &&\n+\techo $HASH\n+}\n+\n+pack_commit_as_promisor() {\n+\tHASH=$(pack_commit $1) &&\n+\t>repo/.git/objects/pack/pack-$HASH.promisor\n+}\n+\n promise_and_delete () {\n \tHASH=$(git -C repo rev-parse \"$1\") &&\n \tgit -C repo tag -a -m message my_annotated_tag \"$HASH\" &&\n@@ -369,7 +380,16 @@ test_expect_success 'missing tree objects with --missing=allow-promisor and --ex\n \tgit -C repo rev-list --exclude-promisor-objects --objects HEAD >objs 2>rev_list_err &&\n \ttest_must_be_empty rev_list_err &&\n \t# 3 commits, no blobs or trees\n-\ttest_line_count = 3 objs\n+\ttest_line_count = 3 objs &&\n+\n+\t# Pack all three commits into individual packs, and mark the last commit pack as promisor\n+\tpack_commit_as_promisor $(git -C repo rev-parse baz) &&\n+\tpack_commit $(git -C repo rev-parse bar) &&\n+\tpack_commit $(git -C repo rev-parse foo) &&\n+\tgit -C repo rev-list --exclude-promisor-objects --objects HEAD >objs 2>rev_list_err &&\n+\ttest_must_be_empty rev_list_err &&\n+\t# commits foo and bar should remain\n+\ttest_line_count = 2 objs\n '\n \n test_expect_success 'missing non-root tree object and rev-list' '\n-- \n2.46.0.rc0.107.gae139121ac.dirty\n\n"},{"id":"499960","messageId":"xmqq4j82euvr.fsf@gitster.g","threadId":"61890","inReplyTo":"20240802073143.56731-2-hanyang.tony@bytedance.com","subject":"Re: [PATCH 1/1] revision: don't set parents as uninteresting if exclude promisor objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-02T16:45:28Z","receivedAt":"2024-08-02T16:45:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Young writes:\n\n> In revision.c, `process_parents()` recursively marks commit parents \n> as UNINTERESTING if the commit itself is UNINTERESTING.\n\nMakes sense.\n\n> `--exclude-promisor-objects` is implemented as \n> \"iterate all objects in promisor packfiles, mark them as UNINTERESTING\".\n\nAlso makes sense.\n\n> So when we find a commit object in a promisor packfile, we also set its ancestors \n> as UNINTERESTING, whether the ancestor is a promisor object or not.\n> This causes normal objects to be falsely marked as promisor objects \n> and removed by `git repack`.\n\nAhh, that is not desirable. So the need to do something to fix it\nis well established here.\n\n> Signed-off-by: Han Young \n> Helped-by: C O Xing Xin \n> ---\n\nPlease order these trailer lines logically in chronological order,\ni.e. you get helped by others to complete the change and then seal\nit by signing it off at the end. I'll swap these two while queuing.\n\n> diff --git a/revision.c b/revision.c\n> index 1c0192f522..eacb0c909d 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1164,7 +1164,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n> \t * wasn't uninteresting), in which case we need\n> \t * to mark its parents recursively too..\n> \t */\n> -\tif (commit->object.flags & UNINTERESTING) {\n> +\tif (!revs->exclude_promisor_objects && commit->object.flags & UNINTERESTING) {\n> \t\twhile (parent) {\n> \t\t\tstruct commit *p = parent->item;\n> \t\t\tparent = parent->next;\n\nBut if the iteration is over all objects in certain packfiles to\nmark them all uninteresting, shouldn't the caller avoid the call to\nprocess_parents() in the first place? Letting process_parents() to\ndo other things and only refrain from doing the \"this commit is\nmarked uninteresting\" part does not quite match what you are trying\nto do, at least to me.\n\nPlease check \"git blame\" to find those who are likely to know better\nabout the code around the area and ask for help from them. I think\nthe bulk of the logic related came from the series that led to\nf3d618d2 (Merge branch 'jh/fsck-promisors', 2018-02-13), so I added\nthe authors of the series.\n\nIt apepars to me that its approach to exclude the objects that\nappear in the promisor packs may be sound, but the design and\nimplementation of it is dubious. Shouldn't it be getting the list\nof objects with get_object_list() WITHOUT paying any attention to\n--exclude-promisor-objects flag, and then filtering objects that\nappear in the promisor packs out of that list, without mucking with\nthe object and commit traversal in revision.c at all?\n\nThanks.\n\n"},{"id":"500629","messageId":"CAG1j3zEQh3xujrU3tGOftwvCZ+d9RjvMHw8v4W3dqd3DsiGCUQ@mail.gmail.com","threadId":"61890","inReplyTo":"xmqq4j82euvr.fsf@gitster.g","subject":"Re: [External] Re: [PATCH 1/1] revision: don't set parents as uninteresting if exclude promisor objects","fromName":"韩仰","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-08-12T12:34:27Z","receivedAt":"2024-08-12T12:34:39Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"On Sat, Aug 3, 2024 at 12:45 AM Junio C Hamano wrote:\n> > diff --git a/revision.c b/revision.c\n> > index 1c0192f522..eacb0c909d 100644\n> > --- a/revision.c\n> > +++ b/revision.c\n> > @@ -1164,7 +1164,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n> > * wasn't uninteresting), in which case we need\n> > * to mark its parents recursively too..\n> > */\n> > - if (commit->object.flags & UNINTERESTING) {\n> > + if (!revs->exclude_promisor_objects && commit->object.flags & UNINTERESTING) {\n> > while (parent) {\n> > struct commit *p = parent->item;\n> > parent = parent->next;\n>\n> But if the iteration is over all objects in certain packfiles to\n> mark them all uninteresting, shouldn't the caller avoid the call to\n> process_parents() in the first place? Letting process_parents() to\n> do other things and only refrain from doing the \"this commit is\n> marked uninteresting\" part does not quite match what you are trying\n> to do, at least to me.\n\nThanks, I agree process_parents() isn't the right place to fix the issue.\n\n> It apepars to me that its approach to exclude the objects that\n> appear in the promisor packs may be sound, but the design and\n> implementation of it is dubious. Shouldn't it be getting the list\n> of objects with get_object_list() WITHOUT paying any attention to\n> --exclude-promisor-objects flag, and then filtering objects that\n> appear in the promisor packs out of that list, without mucking with\n> the object and commit traversal in revision.c at all?\n\nThe problem is --exclude-promisor-objects is an option in revision.c,\nand this option is used by pack-objects, prune, midx-write and rev-list.\nI see there are two ways to fix this issue, one is to remove the\n--exclude-promisor-objects from revision.c, and filter objects in show_commit\nor show_objects functions. The other place to filter objects is probably\nin revision walk, maybe in traverse_commit_list?\n\nThanks.\n"},{"id":"500647","messageId":"xmqqo75x67v7.fsf@gitster.g","threadId":"61890","inReplyTo":"CAG1j3zEQh3xujrU3tGOftwvCZ+d9RjvMHw8v4W3dqd3DsiGCUQ@mail.gmail.com","subject":"Re: [External] Re: [PATCH 1/1] revision: don't set parents as uninteresting if exclude promisor objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-12T16:09:16Z","receivedAt":"2024-08-12T16:09:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"韩仰 writes:\n\n> Thanks, I agree process_parents() isn't the right place to fix the issue.\n>\n>> It apepars to me that its approach to exclude the objects that\n>> appear in the promisor packs may be sound, but the design and\n>> implementation of it is dubious. Shouldn't it be getting the list\n>> of objects with get_object_list() WITHOUT paying any attention to\n>> --exclude-promisor-objects flag, and then filtering objects that\n>> appear in the promisor packs out of that list, without mucking with\n>> the object and commit traversal in revision.c at all?\n>\n> The problem is --exclude-promisor-objects is an option in revision.c,\n> and this option is used by pack-objects, prune, midx-write and rev-list.\n> I see there are two ways to fix this issue, one is to remove the\n> --exclude-promisor-objects from revision.c, and filter objects in show_commit\n> or show_objects functions. The other place to filter objects is probably\n> in revision walk, maybe in traverse_commit_list?\n\nPerhaps another simpler approach may be to use is_promisor_object()\nfunction and get rid of this initial marking of these objects in\nprepare_revision_walk() with the for_each_packed_object() loop,\nwhich abuses the UNINTERESTING bit. The feature wants to exclude\nobjects contained in these packs, but does not want to exclude\nobjects that are referred to and outside of these packs, so\nUNINTERESTING bit whose natural behaviour is to propagate down the\nhistory is a very bad fit for it. We may be able to lose a lot of\nexisting code paths that say \"if exclude_promisor_objects then do\nthis\", and filter objects out with \"is_promisor_object()\" at the\noutput phase near get_revision().\n\nJonathan, if I am not mistaken, this is almost all your code. Any\ninsights to lend us, even though you may not be very active around\nhere lately?\n\nThanks.\n"},{"id":"500674","messageId":"20240813004508.2768102-1-jonathantanmy@google.com","threadId":"61890","inReplyTo":"20240802073143.56731-1-hanyang.tony@bytedance.com","subject":"Re: [PATCH 0/1] revision: fix reachable objects being gc'ed in no blob clone repo","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-08-13T00:45:08Z","receivedAt":"2024-08-13T00:45:12Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Han Young writes:\n> Here are the minimal steps to recreate issue.\n[snip]\n\nI think the following is what is happening. Before the final gc, the\nrepo looks as follows:\n\n commit tree blob\n C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n |\n C2 ---- T2 -- B2 (created locally, in non-promisor pack)\n |\n C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n\nAfter the final gc, {C,T,B}3 and {C,T,B}1 are in a promisor pack, but\nall of {C,T,B}2 are deleted because they are thought to be promisor\nobjects.\n\n> The last `git gc` will error out on fsck with error message like this:\n> \n> error: Could not read d3fbfea9e448461c2b72a79a95a220ae10defd94\n> error: Could not read d3fbfea9e448461c2b72a79a95a220ae10defd94\n\nI'm not sure how `git gc` (or `git fsck`) knows the name of this\nobject (what is the type of this object, and which object refers to\nthis object?) but I think that if we implement one of the solutions I\ndescribe below, this problem will go away.\n\n> `git gc` will call `git repack`, which will call `git pack-objects`\n> twice on a partially cloned repo. The first call to pack-objects \n> combines all the promisor packfiles, and the second pack-objects \n> command packs all reachable non-promisor objects into a normal packfile.\n\nYes, this is what I remember.\n\n> However, a bug in setup_revision caused some non-promisor objects \n> to be mistakenly marked as in promisor packfiles in the second call \n> to pack-objects.\n\nI think they ({C,T,B}2 in the example above) should be considered as\npromisor objects, actually. From the partial clone doc, it says of a\npromisor object: \"the local repository has that object in one of its\npromisor packfiles, or because another promisor object refers to it\".\nC3 (in a promisor packfile) refers to C2, so C2 is a promisor object. It\nrefers to T2, which refers to B2, so all of them are promisor objects.\nHowever, in the Git code, I don't think this definition is applied\nrecursively - if I remember correctly, we made the assumption (e.g.\nin is_promisor_object() in packfile.c) that we only need to care about\nobjects in promisor packfiles and the objects they directly reference,\nbecause how would we know what objects they indirectly reference?\nBut this does not cover the case in which we know what objects they\nindirectly reference because we pushed them to the server in the first\nplace.\n\nSolutions I can think of:\n\n - When fetching from a promisor remote, never declare commits that are\n not in promisor packfiles as HAVE. This means that we would refetch\n C2 and T2 as being in promisor packfiles. But it's wasteful in\n network bandwidth, and does not repair problems in Git repos created\n by Git versions that do not have this solution.\n\n - When fetching from a promisor remote, parse every object and repack\n any local objects referenced (directly or indirectly) into a promisor\n packfile. Also does not repair problems.\n\n - When repacking all objects in promisor packfiles, if any object they\n refer to is present in a non-promisor packfile, do a revwalk on that\n object and pack those objects too. The repack will probably be slower\n because each object now has to be parsed. The revwalks themselves\n probably will not take too long, since they can stop at known promisor\n objects.\n\n(One other thing that might be considered is, whenever pushing to a\npromisor remote, to write the pack that's pushed as a promisor packfile.\nBut I don't think this is a good idea - the server may not retain any\npacks that were pushed.)\n"},{"id":"500807","messageId":"20240813171808.504427-1-jonathantanmy@google.com","threadId":"61890","inReplyTo":"20240813004508.2768102-1-jonathantanmy@google.com","subject":"Re: [PATCH 0/1] revision: fix reachable objects being gc'ed in no blob clone repo","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-08-13T17:18:08Z","receivedAt":"2024-08-13T17:18:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jonathan Tan writes:\n> Solutions I can think of:\n\nOne more thing that I just thought of regarding the solution in this\npatch. It seems to be to have a different separation of packs: all\nobjects currently in promisor packs and all objects currently not\nin promisor packs. And the way it is done is to only exclude (in\nthis patch, mark UNINTERESTING, although it might be better to have\na separate flag for it) objects in promisor packs, but not their\nancestors. There are two ways we can go from here:\n\n - Do not iterate past this object, just like for UNINTERESTING. This\n would end up not packing objects that we need to pack (e.g. {C,T,B}2\n below, if we only have a ref pointing to C3).\n\n commit tree blob\n C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n |\n C2 ---- T2 -- B2 (created locally, in non-promisor pack)\n |\n C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n\n - Iterate past this object (I think this is the path this patch took,\n but I didn't look at it closely). This works, but seems to be very\n slow. We would need to walk through all reachable objects (promisor\n object or not), unlike currently in which we stop once we have\n reached a promisor object.\n\n"},{"id":"500835","messageId":"xmqqsev73ftc.fsf@gitster.g","threadId":"61890","inReplyTo":"20240813171808.504427-1-jonathantanmy@google.com","subject":"Re: [PATCH 0/1] revision: fix reachable objects being gc'ed in no blob clone repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-14T04:10:23Z","receivedAt":"2024-08-14T04:10:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Jonathan Tan writes:\n>> Solutions I can think of:\n>\n> One more thing that I just thought of regarding the solution in this\n> patch. It seems to be to have a different separation of packs: all\n> objects currently in promisor packs and all objects currently not\n> in promisor packs. And the way it is done is to only exclude (in\n> this patch, mark UNINTERESTING, although it might be better to have\n> a separate flag for it) objects in promisor packs, but not their\n> ancestors.\n\nYou're right to mention two separate bits, especially because you do\nnot want the \"I am in a promisor pack\" bit to propagate down to the\nancestry chain like UNINTERESTING bit does. But isn't the approach\nto enumerate all objects in promisor packs in an oidset and give a\nquick way for is_promisor_object() to answer if an object is or is\nnot in promisor pack sufficient to replace the need to use _any_\nobject flag bits to manage objects in promisor packs?\n\n> There are two ways we can go from here:\n>\n> - Do not iterate past this object, just like for UNINTERESTING. This\n> would end up not packing objects that we need to pack (e.g. {C,T,B}2\n> below, if we only have a ref pointing to C3).\n>\n> commit tree blob\n> C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n> |\n> C2 ---- T2 -- B2 (created locally, in non-promisor pack)\n> |\n> C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n>\n> - Iterate past this object (I think this is the path this patch took,\n> but I didn't look at it closely). This works, but seems to be very\n> slow. We would need to walk through all reachable objects (promisor\n> object or not), unlike currently in which we stop once we have\n> reached a promisor object.\n\nThanks for helping Han & Xinxin.\n"},{"id":"500935","messageId":"20240814193036.3918771-1-jonathantanmy@google.com","threadId":"61890","inReplyTo":"xmqqsev73ftc.fsf@gitster.g","subject":"Re: [PATCH 0/1] revision: fix reachable objects being gc'ed in no blob clone repo","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-08-14T19:30:36Z","receivedAt":"2024-08-14T19:30:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano writes:\n> Jonathan Tan writes:\n> \n> > Jonathan Tan writes:\n> >> Solutions I can think of:\n> >\n> > One more thing that I just thought of regarding the solution in this\n> > patch. It seems to be to have a different separation of packs: all\n> > objects currently in promisor packs and all objects currently not\n> > in promisor packs. And the way it is done is to only exclude (in\n> > this patch, mark UNINTERESTING, although it might be better to have\n> > a separate flag for it) objects in promisor packs, but not their\n> > ancestors.\n> \n> You're right to mention two separate bits, especially because you do\n> not want the \"I am in a promisor pack\" bit to propagate down to the\n> ancestry chain like UNINTERESTING bit does. But isn't the approach\n> to enumerate all objects in promisor packs in an oidset and give a\n> quick way for is_promisor_object() to answer if an object is or is\n> not in promisor pack sufficient to replace the need to use _any_\n> object flag bits to manage objects in promisor packs?\n\nAh...yes, you're right. But if someone is intending to go this route,\nnote that you can't use the oidset in is_promisor_object() directly,\nas it contains both objects in promisor packs and objects that they\ndirectly reference. You'll need to adapt it.\n"},{"id":"501500","messageId":"CAG1j3zHKic1DQr-M2nS6Qjp=DV5B90guNbP-PgQkxY2e3XtK8g@mail.gmail.com","threadId":"61890","inReplyTo":"xmqqo75x67v7.fsf@gitster.g","subject":"Re: [External] Re: [PATCH 1/1] revision: don't set parents as uninteresting if exclude promisor objects","fromName":"韩仰","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-08-22T08:28:02Z","receivedAt":"2024-08-22T08:28:18Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"On Tue, Aug 13, 2024 at 12:09 AM Junio C Hamano wrote:\n> Perhaps another simpler approach may be to use is_promisor_object()\n> function and get rid of this initial marking of these objects in\n> prepare_revision_walk() with the for_each_packed_object() loop,\n> which abuses the UNINTERESTING bit. The feature wants to exclude\n> objects contained in these packs, but does not want to exclude\n> objects that are referred to and outside of these packs, so\n> UNINTERESTING bit whose natural behaviour is to propagate down the\n> history is a very bad fit for it. We may be able to lose a lot of\n> existing code paths that say \"if exclude_promisor_objects then do\n> this\", and filter objects out with \"is_promisor_object()\" at the\n> output phase near get_revision().\n\nI tried to go down this route. I removed the for_each_packed_object()\nloop and filter promisor commits in get_revision_1() instead.\nHowever, this only filtered promisor commits, not promisor trees and\nobjects. A combined approach would be keeping the\nfor_each_packed_object() loop, but only mark non-commit objects\nas UNINTERESTING there, and filter promisor commits in\nget_revision()?\n"},{"id":"501590","messageId":"20240823124354.12982-1-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20240802073143.56731-1-hanyang.tony@bytedance.com","subject":"[WIP v2 0/4] revision: fix reachable objects being gc'ed in no blob clone repo","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-08-23T12:43:50Z","receivedAt":"2024-08-23T12:44:03Z","isPatch":false,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"Following Jonathan and Junio's suggestion, I tried to filter promisor\nobjects in get_revision(). Initially I got ride of the\nfor_each_packed_object() loop, but that way promisor trees and blobs are\nnot filtered.\n\nI kept the for_each_packed_object() loop, but only mark non-commit objects\nas UNINTERESTING, that way we ensure all the promisor objects are filtered,\nand UNINTERESTING bit is not passed down in process_parent() call.\n\nBut this isn't enough solve the 'reachable objects being gc'ed' problem,\nas promisor projects is defined as \"objects in promisor pack or referenced\".\ngit repack only packs objects in promisor pack and non promisor objects.\nMeaning objects who are promisor objects but not in promisor pack are discarded.\nSo I added a new option to list-objects, '--exclude-promisor-pack-objects'.\nWhich only exclude objects in promisor packs, this way when we run git repack,\nno reachable objects will be discarded. This seems to fix the problem, but I\nstill don't feel the approach is elegant.\n\nAnother way to fix this problem I come up with is to pack everything into\na promisor packfile, if it is a partial clone repo. This would make pruning\nunreachable objects impossible, but that is already the case with promisor\nobjects. Packing everything into one packfile will simplify code by a lot.\n\nHan Young (4):\n packfile: split promisor objects oidset into two\n revision: add exclude-promisor-pack-objects option\n revision: don't mark commit as UNINTERESTING if\n --exclude-promisor-objects is set\n repack: use new exclude promisor pack objects option\n\n builtin/pack-objects.c | 8 ++++----\n builtin/repack.c | 2 +-\n list-objects.c | 3 ++-\n packfile.c | 25 ++++++++++++++++---------\n packfile.h | 7 ++++++-\n revision.c | 17 +++++++++++++++--\n revision.h | 3 ++-\n 7 files changed, 46 insertions(+), 19 deletions(-)\n\n-- \n2.45.2\n\n"},{"id":"501591","messageId":"20240823124354.12982-2-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20240823124354.12982-1-hanyang.tony@bytedance.com","subject":"[WIP v2 1/4] packfile: split promisor objects oidset into two","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-08-23T12:43:51Z","receivedAt":"2024-08-23T12:44:06Z","isPatch":false,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"split promisor objects oidset into two, one is objects in promisor packfile,\nand other set is objects referenced in promisor packfile. This enable us to\ncheck if an object is in promisor packfile.\n\n---\n packfile.c | 25 ++++++++++++++++---------\n packfile.h | 7 ++++++-\n 2 files changed, 22 insertions(+), 10 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex cf12a539ea..1cf69a17be 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2234,12 +2234,17 @@ int for_each_packed_object(each_packed_object_fn cb, void *data,\n \treturn r ? r : pack_errors;\n }\n \n+struct promisor_objects {\n+\tstruct oidset promisor_pack_objects;\n+\tstruct oidset promisor_pack_referenced_objects;\n+};\n+\n static int add_promisor_object(const struct object_id *oid,\n \t\t\t struct packed_git *pack UNUSED,\n \t\t\t uint32_t pos UNUSED,\n \t\t\t void *set_)\n {\n-\tstruct oidset *set = set_;\n+\tstruct promisor_objects *set = set_;\n \tstruct object *obj;\n \tint we_parsed_object;\n \n@@ -2254,7 +2259,7 @@ static int add_promisor_object(const struct object_id *oid,\n \tif (!obj)\n \t\treturn 1;\n \n-\toidset_insert(set, oid);\n+\toidset_insert(&set->promisor_pack_objects, oid);\n \n \t/*\n \t * If this is a tree, commit, or tag, the objects it refers\n@@ -2272,26 +2277,26 @@ static int add_promisor_object(const struct object_id *oid,\n \t\t\t */\n \t\t\treturn 0;\n \t\twhile (tree_entry_gently(&desc, &entry))\n-\t\t\toidset_insert(set, &entry.oid);\n+\t\t\toidset_insert(&set->promisor_pack_referenced_objects, &entry.oid);\n \t\tif (we_parsed_object)\n \t\t\tfree_tree_buffer(tree);\n \t} else if (obj->type == OBJ_COMMIT) {\n \t\tstruct commit *commit = (struct commit *) obj;\n \t\tstruct commit_list *parents = commit->parents;\n \n-\t\toidset_insert(set, get_commit_tree_oid(commit));\n+\t\toidset_insert(&set->promisor_pack_referenced_objects, get_commit_tree_oid(commit));\n \t\tfor (; parents; parents = parents->next)\n-\t\t\toidset_insert(set, &parents->item->object.oid);\n+\t\t\toidset_insert(&set->promisor_pack_referenced_objects, &parents->item->object.oid);\n \t} else if (obj->type == OBJ_TAG) {\n \t\tstruct tag *tag = (struct tag *) obj;\n-\t\toidset_insert(set, get_tagged_oid(tag));\n+\t\toidset_insert(&set->promisor_pack_referenced_objects, get_tagged_oid(tag));\n \t}\n \treturn 0;\n }\n \n-int is_promisor_object(const struct object_id *oid)\n+int is_in_promisor_pack(const struct object_id *oid, int referenced)\n {\n-\tstatic struct oidset promisor_objects;\n+\tstatic struct promisor_objects promisor_objects;\n \tstatic int promisor_objects_prepared;\n \n \tif (!promisor_objects_prepared) {\n@@ -2303,5 +2308,7 @@ int is_promisor_object(const struct object_id *oid)\n \t\t}\n \t\tpromisor_objects_prepared = 1;\n \t}\n-\treturn oidset_contains(&promisor_objects, oid);\n+\t\n+\treturn oidset_contains(&promisor_objects.promisor_pack_objects, oid) ||\n+\t\t(referenced && oidset_contains(&promisor_objects.promisor_pack_referenced_objects, oid));\n }\ndiff --git a/packfile.h b/packfile.h\nindex 0f78658229..13a349e223 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -195,11 +195,16 @@ int has_object_kept_pack(const struct object_id *oid, unsigned flags);\n \n int has_pack_index(const unsigned char *sha1);\n \n+int is_in_promisor_pack(const struct object_id *oid, int referenced);\n+\n /*\n * Return 1 if an object in a promisor packfile is or refers to the given\n * object, 0 otherwise.\n */\n-int is_promisor_object(const struct object_id *oid);\n+static inline int is_promisor_object(const struct object_id *oid)\n+{\n+\treturn is_in_promisor_pack(oid, 1);\n+}\n \n /*\n * Expose a function for fuzz testing.\n-- \n2.45.2\n\n"},{"id":"501592","messageId":"20240823124354.12982-3-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20240823124354.12982-1-hanyang.tony@bytedance.com","subject":"[WIP v2 2/4] revision: add exclude-promisor-pack-objects option","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-08-23T12:43:52Z","receivedAt":"2024-08-23T12:44:09Z","isPatch":false,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"add --exclude-promisor-pack-objects option to revision walk, this option will\nbe used by git repack in following commits. Unlike --exclude-promisor-objects,\nwhich exclude promisor objects, --exclude-promisor-pack-objects only excludes\nobjects in promisor packfile, objects referenced by objects in promisor pack\nare not excluded.\n\n---\n revision.c | 13 ++++++++++++-\n revision.h | 3 ++-\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 6b33bd814f..7bb03a84c2 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2701,6 +2701,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\tif (fetch_if_missing)\n \t\t\tBUG(\"exclude_promisor_objects can only be used when fetch_if_missing is 0\");\n \t\trevs->exclude_promisor_objects = 1;\n+\t} else if (opt && opt->allow_exclude_promisor_objects &&\n+\t\t !strcmp(arg, \"--exclude-promisor-pack-objects\")) {\n+\t\tif (fetch_if_missing)\n+\t\t\tBUG(\"exclude_promisor_pack_objects can only be used when fetch_if_missing is 0\");\n+\t\trevs->exclude_promisor_pack_objects = 1;\n \t} else {\n \t\tint opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);\n \t\tif (!opts)\n@@ -3908,7 +3913,7 @@ int prepare_revision_walk(struct rev_info *revs)\n \t (revs->limited && limiting_can_increase_treesame(revs)))\n \t\trevs->treesame.name = \"treesame\";\n \n-\tif (revs->exclude_promisor_objects) {\n+\tif (revs->exclude_promisor_objects || revs->exclude_promisor_pack_objects) {\n \t\tfor_each_packed_object(mark_uninteresting, revs,\n \t\t\t\t FOR_EACH_OBJECT_PROMISOR_ONLY);\n \t}\n@@ -4275,6 +4280,12 @@ static struct commit *get_revision_1(struct rev_info *revs)\n \t\tif (!commit)\n \t\t\treturn NULL;\n \n+\t\tif (revs->exclude_promisor_objects && is_promisor_object(&commit->object.oid))\n+\t\t\tcontinue;\n+\n+\t\tif (revs->exclude_promisor_pack_objects && is_in_promisor_pack(&commit->object.oid, 0))\n+\t\t\tcontinue;\n+\n \t\tif (revs->reflog_info)\n \t\t\tcommit->object.flags &= ~(ADDED | SEEN | SHOWN);\n \ndiff --git a/revision.h b/revision.h\nindex 0e470d1df1..6219c35c45 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -229,7 +229,8 @@ struct rev_info {\n \t\t\tdo_not_die_on_missing_objects:1,\n \n \t\t\t/* for internal use only */\n-\t\t\texclude_promisor_objects:1;\n+\t\t\texclude_promisor_objects:1,\n+\t\t\texclude_promisor_pack_objects:1;\n \n \t/* Diff flags */\n \tunsigned int\tdiff:1,\n-- \n2.45.2\n\n"},{"id":"501593","messageId":"20240823124354.12982-4-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20240823124354.12982-1-hanyang.tony@bytedance.com","subject":"[WIP v2 3/4] revision: don't mark commit as UNINTERESTING if --exclude-promisor-objects is set","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-08-23T12:43:53Z","receivedAt":"2024-08-23T12:44:11Z","isPatch":false,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"if commit is marked as UNINTERESTING, the bit will propagate down to its\nparents. This is undesirable in --exclude-promisor-objects, since a promisor\nobjects' parents can be a normal object.\n---\n revision.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex 7bb03a84c2..02227e6a0a 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -3609,7 +3609,9 @@ static int mark_uninteresting(const struct object_id *oid,\n {\n \tstruct rev_info *revs = cb;\n \tstruct object *o = lookup_unknown_object(revs->repo, oid);\n-\to->flags |= UNINTERESTING | SEEN;\n+\tif (o->type != OBJ_COMMIT)\n+\t\to->flags |= UNINTERESTING | SEEN;\n+\n \treturn 0;\n }\n \n-- \n2.45.2\n\n"},{"id":"501594","messageId":"20240823124354.12982-5-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20240823124354.12982-1-hanyang.tony@bytedance.com","subject":"[WIP v2 4/4] repack: use new exclude promisor pack objects option","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-08-23T12:43:54Z","receivedAt":"2024-08-23T12:44:14Z","isPatch":false,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"use --exclude-promisor-pack-objects to pack objects in partial clone repo.\ngit repack will call git pack-objects twice on a partially cloned repo.\nThe first call to pack-objects combines all the objects in promisor packfiles,\nand the second pack-objects command packs all reachable non-promisor objects\ninto a normal packfile. However 'objects in promisor packfiles' plus\n'non-promisor objects' does not equal 'all the reachable objects in repo',\nSince promisor objects also include objects referenced in promisor packfile.\n\n--exclude-promisor-pack-objects only excludes objects in promisor packfiles,\nthis way we don't discard any reachable objects in git repack.\n---\n builtin/pack-objects.c | 8 ++++----\n builtin/repack.c | 2 +-\n list-objects.c | 3 ++-\n 3 files changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c481feadbf..a2b1aaa2e0 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -238,7 +238,7 @@ static enum {\n } write_bitmap_index;\n static uint16_t write_bitmap_options = BITMAP_OPT_HASH_CACHE;\n \n-static int exclude_promisor_objects;\n+static int exclude_promisor_pack_objects;\n \n static int use_delta_islands;\n \n@@ -4391,7 +4391,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tOPT_CALLBACK_F(0, \"missing\", NULL, N_(\"action\"),\n \t\t N_(\"handling for missing objects\"), PARSE_OPT_NONEG,\n \t\t option_parse_missing_action),\n-\t\tOPT_BOOL(0, \"exclude-promisor-objects\", &exclude_promisor_objects,\n+\t\tOPT_BOOL(0, \"exclude-promisor-pack-objects\", &exclude_promisor_pack_objects,\n \t\t\t N_(\"do not pack objects in promisor packfiles\")),\n \t\tOPT_BOOL(0, \"delta-islands\", &use_delta_islands,\n \t\t\t N_(\"respect islands during delta compression\")),\n@@ -4473,10 +4473,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tstrvec_push(&rp, \"--unpacked\");\n \t}\n \n-\tif (exclude_promisor_objects) {\n+\tif (exclude_promisor_pack_objects) {\n \t\tuse_internal_rev_list = 1;\n \t\tfetch_if_missing = 0;\n-\t\tstrvec_push(&rp, \"--exclude-promisor-objects\");\n+\t\tstrvec_push(&rp, \"--exclude-promisor-pack-objects\");\n \t}\n \tif (unpack_unreachable || keep_unreachable || pack_loose_unreachable)\n \t\tuse_internal_rev_list = 1;\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 62cfa50c50..aafe7d30ce 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -1289,7 +1289,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tstrvec_push(&cmd.args, \"--indexed-objects\");\n \t}\n \tif (repo_has_promisor_remote(the_repository))\n-\t\tstrvec_push(&cmd.args, \"--exclude-promisor-objects\");\n+\t\tstrvec_push(&cmd.args, \"--exclude-promisor-pack-objects\");\n \tif (!write_midx) {\n \t\tif (write_bitmaps > 0)\n \t\t\tstrvec_push(&cmd.args, \"--write-bitmap-index\");\ndiff --git a/list-objects.c b/list-objects.c\nindex 985d008799..9b3ff0fe1d 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -178,7 +178,8 @@ static void process_tree(struct traversal_context *ctx,\n \t\t * requested. This may cause the actual filter to report\n \t\t * an incomplete list of missing objects.\n \t\t */\n-\t\tif (revs->exclude_promisor_objects &&\n+\t\tif ((revs->exclude_promisor_objects ||\n+\t\t revs->exclude_promisor_pack_objects) &&\n \t\t is_promisor_object(&obj->oid))\n \t\t\treturn;\n \n-- \n2.45.2\n\n"},{"id":"503105","messageId":"20240919234741.1317946-1-calvinwan@google.com","threadId":"61890","inReplyTo":"20240802073143.56731-1-hanyang.tony@bytedance.com","subject":"[PATCH 0/2] revision: fix reachable commits being gc'ed in partial repo","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-09-19T23:47:39Z","receivedAt":"2024-09-19T23:48:09Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"I took the first patch of Han Young's original series and implemented\nthe fix suggested by Jonathan[1]. While all of his ideas would've\nworked, the first one ends up being the simplest to implement at the\ncost of minor network resources but saves on CPU compared to the other\nideas. This series does not fix repositories that were previously broken\nby this issue, but broken repositories can easily fix themselves by\nrefetching the missing commit, so there isn't much benefit adding\npreventative measures to gc.\n\n[1] https://lore.kernel.org/git/20240813004508.2768102-1-jonathantanmy@google.com/\n\nCalvin Wan (1):\n fetch-pack.c: do not declare local commits as \"have\" in partial repos\n\nHan Young (1):\n packfile: split promisor objects oidset into two\n\n fetch-pack.c | 17 ++++++++++++++---\n packfile.c | 24 +++++++++++++++---------\n packfile.h | 7 ++++++-\n t/t5616-partial-clone.sh | 29 +++++++++++++++++++++++++++++\n 4 files changed, 64 insertions(+), 13 deletions(-)\n\n-- \n2.46.0.792.g87dc391469-goog\n\n"},{"id":"503106","messageId":"20240919234741.1317946-2-calvinwan@google.com","threadId":"61890","inReplyTo":"20240802073143.56731-1-hanyang.tony@bytedance.com","subject":"[PATCH 1/2] packfile: split promisor objects oidset into two","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-09-19T23:47:40Z","receivedAt":"2024-09-19T23:48:11Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"From: Han Young \n\nsplit promisor objects oidset into two, one is objects in promisor packfile,\nand other set is objects referenced in promisor packfile. This enable us to\ncheck if an object is in promisor packfile.\n\nSigned-off-by: Han Young \n---\n packfile.c | 24 +++++++++++++++---------\n packfile.h | 7 ++++++-\n 2 files changed, 21 insertions(+), 10 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex cf12a539ea..3ff191b2e7 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2234,12 +2234,17 @@ int for_each_packed_object(each_packed_object_fn cb, void *data,\n \treturn r ? r : pack_errors;\n }\n \n+struct promisor_objects {\n+\tstruct oidset promisor_pack_objects;\n+\tstruct oidset promisor_pack_referenced_objects;\n+};\n+\n static int add_promisor_object(const struct object_id *oid,\n \t\t\t struct packed_git *pack UNUSED,\n \t\t\t uint32_t pos UNUSED,\n \t\t\t void *set_)\n {\n-\tstruct oidset *set = set_;\n+\tstruct promisor_objects *set = set_;\n \tstruct object *obj;\n \tint we_parsed_object;\n \n@@ -2254,7 +2259,7 @@ static int add_promisor_object(const struct object_id *oid,\n \tif (!obj)\n \t\treturn 1;\n \n-\toidset_insert(set, oid);\n+\toidset_insert(&set->promisor_pack_objects, oid);\n \n \t/*\n \t * If this is a tree, commit, or tag, the objects it refers\n@@ -2272,26 +2277,26 @@ static int add_promisor_object(const struct object_id *oid,\n \t\t\t */\n \t\t\treturn 0;\n \t\twhile (tree_entry_gently(&desc, &entry))\n-\t\t\toidset_insert(set, &entry.oid);\n+\t\t\toidset_insert(&set->promisor_pack_referenced_objects, &entry.oid);\n \t\tif (we_parsed_object)\n \t\t\tfree_tree_buffer(tree);\n \t} else if (obj->type == OBJ_COMMIT) {\n \t\tstruct commit *commit = (struct commit *) obj;\n \t\tstruct commit_list *parents = commit->parents;\n \n-\t\toidset_insert(set, get_commit_tree_oid(commit));\n+\t\toidset_insert(&set->promisor_pack_referenced_objects, get_commit_tree_oid(commit));\n \t\tfor (; parents; parents = parents->next)\n-\t\t\toidset_insert(set, &parents->item->object.oid);\n+\t\t\toidset_insert(&set->promisor_pack_referenced_objects, &parents->item->object.oid);\n \t} else if (obj->type == OBJ_TAG) {\n \t\tstruct tag *tag = (struct tag *) obj;\n-\t\toidset_insert(set, get_tagged_oid(tag));\n+\t\toidset_insert(&set->promisor_pack_referenced_objects, get_tagged_oid(tag));\n \t}\n \treturn 0;\n }\n \n-int is_promisor_object(const struct object_id *oid)\n+int is_in_promisor_pack(const struct object_id *oid, int referenced)\n {\n-\tstatic struct oidset promisor_objects;\n+\tstatic struct promisor_objects promisor_objects;\n \tstatic int promisor_objects_prepared;\n \n \tif (!promisor_objects_prepared) {\n@@ -2303,5 +2308,6 @@ int is_promisor_object(const struct object_id *oid)\n \t\t}\n \t\tpromisor_objects_prepared = 1;\n \t}\n-\treturn oidset_contains(&promisor_objects, oid);\n+\treturn oidset_contains(&promisor_objects.promisor_pack_objects, oid) ||\n+\t\t(referenced && oidset_contains(&promisor_objects.promisor_pack_referenced_objects, oid));\n }\ndiff --git a/packfile.h b/packfile.h\nindex 0f78658229..13a349e223 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -195,11 +195,16 @@ int has_object_kept_pack(const struct object_id *oid, unsigned flags);\n \n int has_pack_index(const unsigned char *sha1);\n \n+int is_in_promisor_pack(const struct object_id *oid, int referenced);\n+\n /*\n * Return 1 if an object in a promisor packfile is or refers to the given\n * object, 0 otherwise.\n */\n-int is_promisor_object(const struct object_id *oid);\n+static inline int is_promisor_object(const struct object_id *oid)\n+{\n+\treturn is_in_promisor_pack(oid, 1);\n+}\n \n /*\n * Expose a function for fuzz testing.\n-- \n2.46.0.792.g87dc391469-goog\n\n"},{"id":"503107","messageId":"20240919234741.1317946-3-calvinwan@google.com","threadId":"61890","inReplyTo":"20240802073143.56731-1-hanyang.tony@bytedance.com","subject":"[PATCH 2/2] fetch-pack.c: do not declare local commits as \"have\" in partial repos","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-09-19T23:47:41Z","receivedAt":"2024-09-19T23:48:13Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"In a partial repository, creating a local commit and then fetching\ncauses the following state to occur:\n\ncommit tree blob\n C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n |\n C2 ---- T2 -- B2 (created locally, in non-promisor pack)\n |\n C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n\nDuring garbage collection, parents of promisor objects are marked as\nUNINTERESTING and are subsequently garbage collected. In this case, C2\nwould be deleted and attempts to access that commit would result in \"bad\nobject\" errors (originally reported here[1]).\n\nThis is not a bug in gc since it should be the case that parents of\npromisor objects are also promisor objects (fsck assumes this as\nwell). When promisor objects are fetched, the state of the repository\nshould ensure that the above holds true. Therefore, do not declare local\ncommits as \"have\" in partial repositores so they can be fetched into a\npromisor pack.\n\n[1] https://lore.kernel.org/git/20240802073143.56731-1-hanyang.tony@bytedance.com/\n\nHelped-by: Jonathan Tan \nSigned-off-by: Calvin Wan \n---\n fetch-pack.c | 17 ++++++++++++++---\n t/t5616-partial-clone.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 3 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 58b4581ad8..c39b0f6ad4 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1297,12 +1297,23 @@ static void add_common(struct strbuf *req_buf, struct oidset *common)\n \n static int add_haves(struct fetch_negotiator *negotiator,\n \t\t struct strbuf *req_buf,\n-\t\t int *haves_to_send)\n+\t\t int *haves_to_send,\n+\t\t int from_promisor)\n {\n \tint haves_added = 0;\n \tconst struct object_id *oid;\n \n \twhile ((oid = negotiator->next(negotiator))) {\n+\t\t/* \n+\t\t * In partial repos, do not declare local objects as \"have\"\n+\t\t * so that they can be fetched into a promisor pack. Certain\n+\t\t * operations mark parent commits of promisor objects as\n+\t\t * UNINTERESTING and are subsequently garbage collected so\n+\t\t * this ensures local commits are still available in promisor\n+\t\t * packs after a fetch + gc.\n+\t\t */\n+\t\tif (from_promisor && !is_in_promisor_pack(oid, 0))\n+\t\t\tcontinue;\n \t\tpacket_buf_write(req_buf, \"have %s\\n\", oid_to_hex(oid));\n \t\tif (++haves_added >= *haves_to_send)\n \t\t\tbreak;\n@@ -1405,7 +1416,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t/* Add all of the common commits we've found in previous rounds */\n \tadd_common(&req_buf, common);\n \n-\thaves_added = add_haves(negotiator, &req_buf, haves_to_send);\n+\thaves_added = add_haves(negotiator, &req_buf, haves_to_send, args->from_promisor);\n \t*in_vain += haves_added;\n \ttrace2_data_intmax(\"negotiation_v2\", the_repository, \"haves_added\", haves_added);\n \ttrace2_data_intmax(\"negotiation_v2\", the_repository, \"in_vain\", *in_vain);\n@@ -2178,7 +2189,7 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,\n \n \t\tpacket_buf_write(&req_buf, \"wait-for-done\");\n \n-\t\thaves_added = add_haves(&negotiator, &req_buf, &haves_to_send);\n+\t\thaves_added = add_haves(&negotiator, &req_buf, &haves_to_send, 0);\n \t\tin_vain += haves_added;\n \t\tif (!haves_added || (seen_ack && in_vain >= MAX_IN_VAIN))\n \t\t\tlast_iteration = 1;\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 8415884754..cba9f7ed9b 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -693,6 +693,35 @@ test_expect_success 'lazy-fetch in submodule succeeds' '\n \tgit -C client restore --recurse-submodules --source=HEAD^ :/\n '\n \n+test_expect_success 'fetching from promisor remote fetches previously local commits' '\n+\t# Setup\n+\tgit init full &&\n+\tgit -C full config uploadpack.allowfilter 1 &&\n+ \tgit -C full config uploadpack.allowanysha1inwant 1 &&\n+\ttouch full/foo &&\n+\tgit -C full add foo &&\n+\tgit -C full commit -m \"commit 1\" &&\n+\tgit -C full checkout --detach &&\n+\n+\t# Partial clone and push commit to remote\n+\tgit clone \"file://$(pwd)/full\" --filter=blob:none partial &&\n+\techo \"hello\" > partial/foo &&\n+\tgit -C partial commit -a -m \"commit 2\" &&\n+\tgit -C partial push &&\n+\n+\t# gc in partial repo\n+\tgit -C partial gc --prune=now &&\n+\n+\t# Create another commit in normal repo\n+\tgit -C full checkout main &&\n+\techo \" world\" >> full/foo &&\n+\tgit -C full commit -a -m \"commit 3\" &&\n+\n+\t# Pull from remote in partial repo, and run gc again\n+\tgit -C partial pull &&\n+\tgit -C partial gc --prune=now\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.46.0.792.g87dc391469-goog\n\n"},{"id":"503210","messageId":"xmqqzfo08a99.fsf@gitster.g","threadId":"61890","inReplyTo":"20240919234741.1317946-2-calvinwan@google.com","subject":"Re: [PATCH 1/2] packfile: split promisor objects oidset into two","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-22T06:37:06Z","receivedAt":"2024-09-22T06:37:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calvin Wan writes:\n\n> From: Han Young \n>\n> split promisor objects oidset into two, one is objects in promisor packfile,\n> and other set is objects referenced in promisor packfile. This enable us to\n> check if an object is in promisor packfile.\n\nOK, so the idea is that we can discard the objects that are _in_ a\npromisor packfile and assume that we can fetch them back?\n\nObjects that are referenced by objects in the promisor packfile may\nor may not be in the same packfile, and we obviously cannot expect\nthat we can refetch those that are not in the promisor packfile from\nthe promisor. So what is the other list for?\n\nWhat I am wondering is what good the existing helper function\nis_promisor_object() is for. It will say \"yes\" for objects that we\nmay have obtained from the promisor remote (hence we can lazily\nfetch them again even if we lost them) and in promisor packs, but it\nmay also say \"yes\" for any object that an object that is in a\npromisor pack (e.g., a tree object that represents a subdirectory\nthat was not modified by a commit in a promisor pack, a parent\ncommit of a commit in a promisor pack, etc.). In other words, are\nthe callers getting any useful answer to their question to the\nhelper function, or are they all buggy for not asking \"is this\nobject in a promisor pack\" and allowing the helper to say \"yes\" for\nobjects that are merely referenced by an object in promisor packs?\n\nThanks.\n\n\n> -int is_promisor_object(const struct object_id *oid)\n> +int is_in_promisor_pack(const struct object_id *oid, int referenced)\n> {\n> -\tstatic struct oidset promisor_objects;\n> +\tstatic struct promisor_objects promisor_objects;\n> \tstatic int promisor_objects_prepared;\n> \n> \tif (!promisor_objects_prepared) {\n> @@ -2303,5 +2308,6 @@ int is_promisor_object(const struct object_id *oid)\n> \t\t}\n> \t\tpromisor_objects_prepared = 1;\n> \t}\n> -\treturn oidset_contains(&promisor_objects, oid);\n> +\treturn oidset_contains(&promisor_objects.promisor_pack_objects, oid) ||\n> +\t\t(referenced && oidset_contains(&promisor_objects.promisor_pack_referenced_objects, oid));\n> }\n> diff --git a/packfile.h b/packfile.h\n> index 0f78658229..13a349e223 100644\n> --- a/packfile.h\n> +++ b/packfile.h\n> @@ -195,11 +195,16 @@ int has_object_kept_pack(const struct object_id *oid, unsigned flags);\n> \n> int has_pack_index(const unsigned char *sha1);\n> \n> +int is_in_promisor_pack(const struct object_id *oid, int referenced);\n> +\n> /*\n> * Return 1 if an object in a promisor packfile is or refers to the given\n> * object, 0 otherwise.\n> */\n> -int is_promisor_object(const struct object_id *oid);\n> +static inline int is_promisor_object(const struct object_id *oid)\n> +{\n> +\treturn is_in_promisor_pack(oid, 1);\n> +}\n> \n> /*\n> * Expose a function for fuzz testing.\n"},{"id":"503212","messageId":"xmqqr09c89id.fsf@gitster.g","threadId":"61890","inReplyTo":"20240919234741.1317946-3-calvinwan@google.com","subject":"Re: [PATCH 2/2] fetch-pack.c: do not declare local commits as \"have\" in partial repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-22T06:53:14Z","receivedAt":"2024-09-22T06:53:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calvin Wan writes:\n\n> In a partial repository, creating a local commit and then fetching\n> causes the following state to occur:\n>\n> commit tree blob\n> C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n> |\n> C2 ---- T2 -- B2 (created locally, in non-promisor pack)\n> |\n> C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n>\n> During garbage collection, parents of promisor objects are marked as\n> UNINTERESTING and are subsequently garbage collected. In this case, C2\n> would be deleted and attempts to access that commit would result in \"bad\n> object\" errors (originally reported here[1]).\n\nUnderstandable.\n\n> This is not a bug in gc since it should be the case that parents of\n> promisor objects are also promisor objects (fsck assumes this as\n> well).\n\nI am not sure where this \"not a bug\" claim comes from. Here, the\ndefinition of \"promisor objects\" seems to be anything that are\nreachable from objects in promisor packs, but isn't the source of\nthe bug that collects C2 exactly that \"gc\" uses such a definition\nfor discardable objects that can be refetchd from promisor remotes?\n\n> When promisor objects are fetched, the state of the repository\n> should ensure that the above holds true. Therefore, do not declare local\n> commits as \"have\" in partial repositores so they can be fetched into a\n> promisor pack.\n\nCould you clarify what it means in the context of the above example\nyou gave in an updated version of the proposed log message?\n\nWe pretend that C2 and anything it reaches do not exist locally, to\nforce them to be fetched from the remote? We'd end up having two\ncopies of C2 (one that we created locally and had before starting\nthis fetch, the other we fetched when we fetched C3 from them)?\nThis sounds like it is awfully inefficient both network bandwidth-\nand local disk-wise.\n\nI was hoping to see that the issue can be fixed on the \"gc\" side,\nregardless of how the objects enter our repository, but perhaps I am\nmissing something. Isn't it just the matter of collecting C1, C3\nbut not C2? Or to put it another way, if we first create a list of\nobjects to be packed (regardless of whether they are in promisor\npacks), and then remove the objects that are in promisor packs from\nthe list, and pack the objects still remaining in the list?\n\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 58b4581ad8..c39b0f6ad4 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1297,12 +1297,23 @@ static void add_common(struct strbuf *req_buf, struct oidset *common)\n> \n> static int add_haves(struct fetch_negotiator *negotiator,\n> \t\t struct strbuf *req_buf,\n> -\t\t int *haves_to_send)\n> +\t\t int *haves_to_send,\n> +\t\t int from_promisor)\n> {\n> \tint haves_added = 0;\n> \tconst struct object_id *oid;\n> \n> \twhile ((oid = negotiator->next(negotiator))) {\n> +\t\t/* \n> +\t\t * In partial repos, do not declare local objects as \"have\"\n> +\t\t * so that they can be fetched into a promisor pack. Certain\n> +\t\t * operations mark parent commits of promisor objects as\n> +\t\t * UNINTERESTING and are subsequently garbage collected so\n> +\t\t * this ensures local commits are still available in promisor\n> +\t\t * packs after a fetch + gc.\n> +\t\t */\n> +\t\tif (from_promisor && !is_in_promisor_pack(oid, 0))\n> +\t\t\tcontinue;\n> \t\tpacket_buf_write(req_buf, \"have %s\\n\", oid_to_hex(oid));\n> \t\tif (++haves_added >= *haves_to_send)\n> \t\t\tbreak;\n> @@ -1405,7 +1416,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n> \t/* Add all of the common commits we've found in previous rounds */\n> \tadd_common(&req_buf, common);\n> \n> -\thaves_added = add_haves(negotiator, &req_buf, haves_to_send);\n> +\thaves_added = add_haves(negotiator, &req_buf, haves_to_send, args->from_promisor);\n> \t*in_vain += haves_added;\n> \ttrace2_data_intmax(\"negotiation_v2\", the_repository, \"haves_added\", haves_added);\n> \ttrace2_data_intmax(\"negotiation_v2\", the_repository, \"in_vain\", *in_vain);\n> @@ -2178,7 +2189,7 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,\n> \n> \t\tpacket_buf_write(&req_buf, \"wait-for-done\");\n> \n> -\t\thaves_added = add_haves(&negotiator, &req_buf, &haves_to_send);\n> +\t\thaves_added = add_haves(&negotiator, &req_buf, &haves_to_send, 0);\n> \t\tin_vain += haves_added;\n> \t\tif (!haves_added || (seen_ack && in_vain >= MAX_IN_VAIN))\n> \t\t\tlast_iteration = 1;\n> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n> index 8415884754..cba9f7ed9b 100755\n> --- a/t/t5616-partial-clone.sh\n> +++ b/t/t5616-partial-clone.sh\n> @@ -693,6 +693,35 @@ test_expect_success 'lazy-fetch in submodule succeeds' '\n> \tgit -C client restore --recurse-submodules --source=HEAD^ :/\n> '\n> \n> +test_expect_success 'fetching from promisor remote fetches previously local commits' '\n> +\t# Setup\n> +\tgit init full &&\n> +\tgit -C full config uploadpack.allowfilter 1 &&\n> + \tgit -C full config uploadpack.allowanysha1inwant 1 &&\n> +\ttouch full/foo &&\n> +\tgit -C full add foo &&\n> +\tgit -C full commit -m \"commit 1\" &&\n> +\tgit -C full checkout --detach &&\n> +\n> +\t# Partial clone and push commit to remote\n> +\tgit clone \"file://$(pwd)/full\" --filter=blob:none partial &&\n> +\techo \"hello\" > partial/foo &&\n> +\tgit -C partial commit -a -m \"commit 2\" &&\n> +\tgit -C partial push &&\n> +\n> +\t# gc in partial repo\n> +\tgit -C partial gc --prune=now &&\n> +\n> +\t# Create another commit in normal repo\n> +\tgit -C full checkout main &&\n> +\techo \" world\" >> full/foo &&\n> +\tgit -C full commit -a -m \"commit 3\" &&\n> +\n> +\t# Pull from remote in partial repo, and run gc again\n> +\tgit -C partial pull &&\n> +\tgit -C partial gc --prune=now\n> +'\n> +\n> . \"$TEST_DIRECTORY\"/lib-httpd.sh\n> start_httpd\n"},{"id":"503219","messageId":"xmqqh6a78wv6.fsf@gitster.g","threadId":"61890","inReplyTo":"xmqqr09c89id.fsf@gitster.g","subject":"Re: [PATCH 2/2] fetch-pack.c: do not declare local commits as \"have\" in partial repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-22T16:41:01Z","receivedAt":"2024-09-22T16:41:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Calvin Wan writes:\n>\n>> In a partial repository, creating a local commit and then fetching\n>> causes the following state to occur:\n>>\n>> commit tree blob\n>> C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n>> |\n>> C2 ---- T2 -- B2 (created locally, in non-promisor pack)\n>> |\n>> C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n>>\n>> During garbage collection, parents of promisor objects are marked as\n>> UNINTERESTING and are subsequently garbage collected. In this case, C2\n>> would be deleted and attempts to access that commit would result in \"bad\n>> object\" errors (originally reported here[1]).\n>\n> Understandable.\n> ...\n>> When promisor objects are fetched, the state of the repository\n>> should ensure that the above holds true. Therefore, do not declare local\n>> commits as \"have\" in partial repositores so they can be fetched into a\n>> promisor pack.\n> ...\n> We pretend that C2 and anything it reaches do not exist locally, to\n> force them to be fetched from the remote? We'd end up having two\n> copies of C2 (one that we created locally and had before starting\n> this fetch, the other we fetched when we fetched C3 from them)?\n> This sounds like it is awfully inefficient both network bandwidth-\n> and local disk-wise.\n\nOne related thing that worries me is what happens after we make a\nlarge push, either directly to the remote, or what we pushed\nelsewhere were fetched by the remote, and then we need to fetch what\nthey created on top. The history may look like this:\n\n1. we fetch from promisor remote. C is in promisor packs\n\n ---C\n\n2. we build on top. 'x' are local.\n\n ---C---x---x---x---x---x---x---x---x\n\n3. 'x' we created above ends up to be a the promisor side,\n and others build a few commits on top.\n\n ---C---x---x---x---x---x---x---x---x---o---o\n\n4. Now we try to fetch from them. I.e. a repository that has\n history illustrated in 2. fetches the history illustrated in 3.\n\nBecause this change forbids the fetching side to tell the other side\nthat it has 'x', the first \"have\" we are allowed to send is 'C',\neven though we only need to fetch two commits 'o' from them.\n\nAnd 'x' could be numerous in distributed development workflows, as\nthese \"local\" commits do not have to be ones you created locally\nyourself. You may have fetched and merged these commits from\nelsewhere where the active development is happening. The only\ncriterion that qualifies a commit to be \"local\" (and causes us to\nomit them from \"have\" communication) is that we didn't obtain it\ndirectly from our promisor remote, so you may end up fetching\na large portion of the history you already have from the promisor\nremote, just to have them into a promisor pack.\n\nIf we cannot change the definition of \"is-promisor-object\" for the\npurpose of \"gc\" (and it is probably I am missing what you, JTan, and\nHanYang thought about that I do not see he reason why), I wonder if\nthere is a way to somehow avoid the refetching but still \"move\"\nthese 'x' objects purely locally into a promisor pack?\n\nThat is, the current \"git fetch\" without this patch would only fetch\ntwo 'o' commits (and its associated trees and blobs) into a new\npromisor pack, but because we know that commits 'x' have now become\nre-fetchable from the promisor, we can make them promisor objects by\nrepacking locally them and mark the resulting pack a promisor pack,\nwithout incurring the cost to the remote to prepare and send 'x'\nagain to us. That would give us the same protection the patch under\ndiscussion offers, wouldn't it?\n\nI however still think fixing \"gc\" would give us a lot more intuitive\nbehaviour, though.\n\nThanks.\n\n"},{"id":"503239","messageId":"CAG1j3zHJVrpK5JZtUXFwkZgWY1-CxqET+ygpaMqo5aM-KeWaxg@mail.gmail.com","threadId":"61890","inReplyTo":"xmqqr09c89id.fsf@gitster.g","subject":"Re: [External] Re: [PATCH 2/2] fetch-pack.c: do not declare local commits as \"have\" in partial repos","fromName":"韩仰","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-09-23T03:44:57Z","receivedAt":"2024-09-23T03:45:09Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"On Sun, Sep 22, 2024 at 2:53 PM Junio C Hamano wrote:\n\n> I was hoping to see that the issue can be fixed on the \"gc\" side,\n> regardless of how the objects enter our repository, but perhaps I am\n> missing something. Isn't it just the matter of collecting C1, C3\n> but not C2? Or to put it another way, if we first create a list of\n> objects to be packed (regardless of whether they are in promisor\n> packs), and then remove the objects that are in promisor packs from\n> the list, and pack the objects still remaining in the list?\n\nI tried to fix the issue on the \"gc\" side following JTan's suggestion,\nby packing local objects referenced by promisor objects into promisor\npacks. But it turns out the cost for \"for each promisor object,\nparse them and try to decide the objects they reference is in local repo\"\nis too great. In a test blob:none partial clone repo, the gc would take more\nthan one hour in the 2019 MacBook, despite the repo only\nhaving 17071073 objects. Normally it would take about 30 minutes.\n\n> if we first create a list of\n> objects to be packed (regardless of whether they are in promisor\n> packs), and then remove the objects that are in promisor packs from\n> the list, and pack the objects still remaining in the list?\n\nThis would work, though the remaining objects in the list would be\nsuboptimally packed, due to the delta heuristic. Because we feed\nobject id directly into git-pack-objects, instead of using rev-list. But\nthat's how we pack promisor objects anyway.\n\nIn $JOB, we modified git-repack to pack everything into a giant promisor\npack if the repo is partially cloned. This basically does the same thing as\nyou suggested, but without the cost of constructing the object list and\nremoving the objects in the promisor packs.\n\nThanks.\n"},{"id":"503261","messageId":"xmqqy13i49za.fsf@gitster.g","threadId":"61890","inReplyTo":"CAG1j3zHJVrpK5JZtUXFwkZgWY1-CxqET+ygpaMqo5aM-KeWaxg@mail.gmail.com","subject":"Re: [External] Re: [PATCH 2/2] fetch-pack.c: do not declare local commits as \"have\" in partial repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-23T16:21:13Z","receivedAt":"2024-09-23T16:21:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"韩仰 writes:\n\n> In $JOB, we modified git-repack to pack everything into a giant promisor\n> pack if the repo is partially cloned.\n\nI would imagine that would give you pretty much similar results as\nthe posted patch without incurring the cost of transfering the same\nobjects from the promisor remote.\n\n> This basically does the same thing as\n> you suggested, but without the cost of constructing the object list and\n> removing the objects in the promisor packs.\n\nYup, repacking, instead of creating a new pack only to hold the\nobjects in the gap, would be much simpler.\n"},{"id":"503457","messageId":"20240925072021.77078-1-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20240802073143.56731-1-hanyang.tony@bytedance.com","subject":"[PATCH 0/2] repack: pack everything into promisor packfile in partial repos","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-09-25T07:20:19Z","receivedAt":"2024-09-25T07:20:31Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"As suggested by Jonathan[1], there are number of ways to fix this issue.\nWe have already explored some of them in this thread, and so far none of them\nis satisfiable. Calvin and I tried to address the problem from fetch-pack side\nand rev-list side. But the fix either consumes too much CPU power or results\nin inefficient bandwidth use.\n\nSo let's attack the problem from repack side. The goal is to prevent repack\nfrom discarding local objects, previously it is done by carefully\nseparating promisor objects and normal objects in rev-list.\nThe implementation is flawed and no solution have been found so far.\nInstead, we can get ride of rev-list and just pack everything into promisor\nfiles. This way, no objects would be lost.\n\nBy using 'repack everything', repacking requires less work and we are not\nusing more bandwidth. The only downside is normal objects packing does not\nbenefiting from the history and path based delta calculation. Majority of\nobjects in a partial repo is promisor objects, so the impact of worse normal\nobjects repacking is negligible.\n\n[1] https://lore.kernel.org/git/20240813004508.2768102-1-jonathantanmy@google.com/\n\nHan Young (2):\n repack: pack everything into promisor packfile in partial repos\n t0410: adapt tests to repack changes\n\n builtin/repack.c | 258 ++++++++++++++++++++++-----------------\n t/t0410-partial-clone.sh | 68 +----------\n 2 files changed, 145 insertions(+), 181 deletions(-)\n\n-- \n2.46.0\n\n"},{"id":"503458","messageId":"20240925072021.77078-2-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20240925072021.77078-1-hanyang.tony@bytedance.com","subject":"[PATCH 1/2] repack: pack everything into packfile","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-09-25T07:20:20Z","receivedAt":"2024-09-25T07:20:40Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"In a partial repository, creating a local commit and then fetching\ncauses the following state to occur:\n\ncommit tree blob\n C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n |\n C2 ---- T2 -- B2 (created locally, in non-promisor pack)\n |\n C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n\nDuring garbage collection, parents of promisor objects are marked as\nUNINTERESTING and are subsequently garbage collected. In this case, C2\nwould be deleted and attempts to access that commit would result in \"bad\nobject\" errors (originally reported here[1]).\n\nFor partial repos, repacking is done in two steps. We first repack all the\nobjects in promisor packfile, then repack all the non-promisor objects.\nIt turns out C2, T2 and B2 are not repacked in either steps, ended up deleted.\nWe can avoid this by packing everything into a promisor packfile, if the repo\nis partial cloned.\n\n[1] https://lore.kernel.org/git/20240802073143.56731-1-hanyang.tony@bytedance.com/\n\nHelped-by: Calvin Wan \nSigned-off-by: Han Young \n---\n builtin/repack.c | 257 ++++++++++++++++++++++++++---------------------\n 1 file changed, 143 insertions(+), 114 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex cb4420f085..e9e18a31fe 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -321,6 +321,23 @@ static int write_oid(const struct object_id *oid,\n \treturn 0;\n }\n \n+static int write_loose_oid(const struct object_id *oid,\n+\t\t\t\t const char *path UNUSED,\n+\t\t\t\t void *data)\n+{\n+\tstruct child_process *cmd = data;\n+\n+\tif (cmd->in == -1) {\n+\t\tif (start_command(cmd))\n+\t\t\tdie(_(\"could not start pack-objects to repack promisor objects\"));\n+\t}\n+\n+\tif (write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n+\t write_in_full(cmd->in, \"\\n\", 1) < 0)\n+\t\tdie(_(\"failed to feed promisor objects to pack-objects\"));\n+\treturn 0;\n+}\n+\n static struct {\n \tconst char *name;\n \tunsigned optional:1;\n@@ -370,12 +387,15 @@ static int has_pack_ext(const struct generated_pack_data *data,\n \tBUG(\"unknown pack extension: '%s'\", ext);\n }\n \n-static void repack_promisor_objects(const struct pack_objects_args *args,\n-\t\t\t\t struct string_list *names)\n+static int repack_promisor_objects(const struct pack_objects_args *args,\n+\t\t\t\t struct string_list *names,\n+\t\t\t\t struct string_list *list,\n+\t\t\t\t int pack_all)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tFILE *out;\n \tstruct strbuf line = STRBUF_INIT;\n+\tstruct string_list_item *item;\n \n \tprepare_pack_objects(&cmd, args, packtmp);\n \tcmd.in = -1;\n@@ -387,13 +407,19 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \t * {type -> existing pack order} ordering when computing deltas instead\n \t * of a {type -> size} ordering, which may produce better deltas.\n \t */\n-\tfor_each_packed_object(write_oid, &cmd,\n-\t\t\t FOR_EACH_OBJECT_PROMISOR_ONLY);\n+\tif (pack_all)\n+\t\tfor_each_packed_object(write_oid, &cmd, 0);\n+\telse\n+\t\tfor_each_string_list_item(item, list) {\n+\t\t\tpack_mark_retained(item);\n+\t\t}\n+\n+\tfor_each_loose_object(write_loose_oid, &cmd, 0);\n \n \tif (cmd.in == -1) {\n \t\t/* No packed objects; cmd was never started */\n \t\tchild_process_clear(&cmd);\n-\t\treturn;\n+\t\treturn 0;\n \t}\n \n \tclose(cmd.in);\n@@ -431,6 +457,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \tif (finish_command(&cmd))\n \t\tdie(_(\"could not finish pack-objects to repack promisor objects\"));\n \tstrbuf_release(&line);\n+\treturn 0;\n }\n \n struct pack_geometry {\n@@ -1312,8 +1339,7 @@ int cmd_repack(int argc,\n \t\tstrvec_push(&cmd.args, \"--reflog\");\n \t\tstrvec_push(&cmd.args, \"--indexed-objects\");\n \t}\n-\tif (repo_has_promisor_remote(the_repository))\n-\t\tstrvec_push(&cmd.args, \"--exclude-promisor-objects\");\n+\n \tif (!write_midx) {\n \t\tif (write_bitmaps > 0)\n \t\t\tstrvec_push(&cmd.args, \"--write-bitmap-index\");\n@@ -1323,125 +1349,128 @@ int cmd_repack(int argc,\n \tif (use_delta_islands)\n \t\tstrvec_push(&cmd.args, \"--delta-islands\");\n \n-\tif (pack_everything & ALL_INTO_ONE) {\n-\t\trepack_promisor_objects(&po_args, &names);\n-\n-\t\tif (has_existing_non_kept_packs(&existing) &&\n-\t\t delete_redundant &&\n-\t\t !(pack_everything & PACK_CRUFT)) {\n-\t\t\tfor_each_string_list_item(item, &names) {\n-\t\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n-\t\t\t\t\t packtmp_name, item->string);\n-\t\t\t}\n-\t\t\tif (unpack_unreachable) {\n-\t\t\t\tstrvec_pushf(&cmd.args,\n-\t\t\t\t\t \"--unpack-unreachable=%s\",\n-\t\t\t\t\t unpack_unreachable);\n-\t\t\t} else if (pack_everything & LOOSEN_UNREACHABLE) {\n-\t\t\t\tstrvec_push(&cmd.args,\n-\t\t\t\t\t \"--unpack-unreachable\");\n-\t\t\t} else if (keep_unreachable) {\n-\t\t\t\tstrvec_push(&cmd.args, \"--keep-unreachable\");\n-\t\t\t\tstrvec_push(&cmd.args, \"--pack-loose-unreachable\");\n+\tif (repo_has_promisor_remote(the_repository)) {\n+\t\tret = repack_promisor_objects(&po_args, &names,\n+\t\t\t&existing.non_kept_packs, pack_everything & ALL_INTO_ONE);\n+\t} else {\n+\t\tif (pack_everything & ALL_INTO_ONE) {\n+\t\t\tif (has_existing_non_kept_packs(&existing) &&\n+\t\t\tdelete_redundant &&\n+\t\t\t!(pack_everything & PACK_CRUFT)) {\n+\t\t\t\tfor_each_string_list_item(item, &names) {\n+\t\t\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n+\t\t\t\t\t\tpacktmp_name, item->string);\n+\t\t\t\t}\n+\t\t\t\tif (unpack_unreachable) {\n+\t\t\t\t\tstrvec_pushf(&cmd.args,\n+\t\t\t\t\t\t\"--unpack-unreachable=%s\",\n+\t\t\t\t\t\tunpack_unreachable);\n+\t\t\t\t} else if (pack_everything & LOOSEN_UNREACHABLE) {\n+\t\t\t\t\tstrvec_push(&cmd.args,\n+\t\t\t\t\t\t\"--unpack-unreachable\");\n+\t\t\t\t} else if (keep_unreachable) {\n+\t\t\t\t\tstrvec_push(&cmd.args, \"--keep-unreachable\");\n+\t\t\t\t\tstrvec_push(&cmd.args, \"--pack-loose-unreachable\");\n+\t\t\t\t}\n \t\t\t}\n+\t\t} else if (geometry.split_factor) {\n+\t\t\tstrvec_push(&cmd.args, \"--stdin-packs\");\n+\t\t\tstrvec_push(&cmd.args, \"--unpacked\");\n+\t\t} else {\n+\t\t\tstrvec_push(&cmd.args, \"--unpacked\");\n+\t\t\tstrvec_push(&cmd.args, \"--incremental\");\n \t\t}\n-\t} else if (geometry.split_factor) {\n-\t\tstrvec_push(&cmd.args, \"--stdin-packs\");\n-\t\tstrvec_push(&cmd.args, \"--unpacked\");\n-\t} else {\n-\t\tstrvec_push(&cmd.args, \"--unpacked\");\n-\t\tstrvec_push(&cmd.args, \"--incremental\");\n-\t}\n \n-\tif (po_args.filter_options.choice)\n-\t\tstrvec_pushf(&cmd.args, \"--filter=%s\",\n-\t\t\t expand_list_objects_filter_spec(&po_args.filter_options));\n-\telse if (filter_to)\n-\t\tdie(_(\"option '%s' can only be used along with '%s'\"), \"--filter-to\", \"--filter\");\n+\t\tif (po_args.filter_options.choice)\n+\t\t\tstrvec_pushf(&cmd.args, \"--filter=%s\",\n+\t\t\t\texpand_list_objects_filter_spec(&po_args.filter_options));\n+\t\telse if (filter_to)\n+\t\t\tdie(_(\"option '%s' can only be used along with '%s'\"), \"--filter-to\", \"--filter\");\n \n-\tif (geometry.split_factor)\n-\t\tcmd.in = -1;\n-\telse\n-\t\tcmd.no_stdin = 1;\n+\t\tif (geometry.split_factor)\n+\t\t\tcmd.in = -1;\n+\t\telse\n+\t\t\tcmd.no_stdin = 1;\n \n-\tret = start_command(&cmd);\n-\tif (ret)\n-\t\tgoto cleanup;\n+\t\tret = start_command(&cmd);\n+\t\tif (ret)\n+\t\t\tgoto cleanup;\n \n-\tif (geometry.split_factor) {\n-\t\tFILE *in = xfdopen(cmd.in, \"w\");\n-\t\t/*\n-\t\t * The resulting pack should contain all objects in packs that\n-\t\t * are going to be rolled up, but exclude objects in packs which\n-\t\t * are being left alone.\n-\t\t */\n-\t\tfor (i = 0; i < geometry.split; i++)\n-\t\t\tfprintf(in, \"%s\\n\", pack_basename(geometry.pack[i]));\n-\t\tfor (i = geometry.split; i < geometry.pack_nr; i++)\n-\t\t\tfprintf(in, \"^%s\\n\", pack_basename(geometry.pack[i]));\n-\t\tfclose(in);\n-\t}\n+\t\tif (geometry.split_factor) {\n+\t\t\tFILE *in = xfdopen(cmd.in, \"w\");\n+\t\t\t/*\n+\t\t\t* The resulting pack should contain all objects in packs that\n+\t\t\t* are going to be rolled up, but exclude objects in packs which\n+\t\t\t* are being left alone.\n+\t\t\t*/\n+\t\t\tfor (i = 0; i < geometry.split; i++)\n+\t\t\t\tfprintf(in, \"%s\\n\", pack_basename(geometry.pack[i]));\n+\t\t\tfor (i = geometry.split; i < geometry.pack_nr; i++)\n+\t\t\t\tfprintf(in, \"^%s\\n\", pack_basename(geometry.pack[i]));\n+\t\t\tfclose(in);\n+\t\t}\n \n-\tret = finish_pack_objects_cmd(&cmd, &names, 1);\n-\tif (ret)\n-\t\tgoto cleanup;\n-\n-\tif (!names.nr && !po_args.quiet)\n-\t\tprintf_ln(_(\"Nothing new to pack.\"));\n-\n-\tif (pack_everything & PACK_CRUFT) {\n-\t\tconst char *pack_prefix = find_pack_prefix(packdir, packtmp);\n-\n-\t\tif (!cruft_po_args.window)\n-\t\t\tcruft_po_args.window = po_args.window;\n-\t\tif (!cruft_po_args.window_memory)\n-\t\t\tcruft_po_args.window_memory = po_args.window_memory;\n-\t\tif (!cruft_po_args.depth)\n-\t\t\tcruft_po_args.depth = po_args.depth;\n-\t\tif (!cruft_po_args.threads)\n-\t\t\tcruft_po_args.threads = po_args.threads;\n-\t\tif (!cruft_po_args.max_pack_size)\n-\t\t\tcruft_po_args.max_pack_size = po_args.max_pack_size;\n-\n-\t\tcruft_po_args.local = po_args.local;\n-\t\tcruft_po_args.quiet = po_args.quiet;\n-\n-\t\tret = write_cruft_pack(&cruft_po_args, packtmp, pack_prefix,\n-\t\t\t\t cruft_expiration, &names,\n-\t\t\t\t &existing);\n+\t\tret = finish_pack_objects_cmd(&cmd, &names, 1);\n \t\tif (ret)\n \t\t\tgoto cleanup;\n \n-\t\tif (delete_redundant && expire_to) {\n-\t\t\t/*\n-\t\t\t * If `--expire-to` is given with `-d`, it's possible\n-\t\t\t * that we're about to prune some objects. With cruft\n-\t\t\t * packs, pruning is implicit: any objects from existing\n-\t\t\t * packs that weren't picked up by new packs are removed\n-\t\t\t * when their packs are deleted.\n-\t\t\t *\n-\t\t\t * Generate an additional cruft pack, with one twist:\n-\t\t\t * `names` now includes the name of the cruft pack\n-\t\t\t * written in the previous step. So the contents of\n-\t\t\t * _this_ cruft pack exclude everything contained in the\n-\t\t\t * existing cruft pack (that is, all of the unreachable\n-\t\t\t * objects which are no older than\n-\t\t\t * `--cruft-expiration`).\n-\t\t\t *\n-\t\t\t * To make this work, cruft_expiration must become NULL\n-\t\t\t * so that this cruft pack doesn't actually prune any\n-\t\t\t * objects. If it were non-NULL, this call would always\n-\t\t\t * generate an empty pack (since every object not in the\n-\t\t\t * cruft pack generated above will have an mtime older\n-\t\t\t * than the expiration).\n-\t\t\t */\n-\t\t\tret = write_cruft_pack(&cruft_po_args, expire_to,\n-\t\t\t\t\t pack_prefix,\n-\t\t\t\t\t NULL,\n-\t\t\t\t\t &names,\n-\t\t\t\t\t &existing);\n+\t\tif (!names.nr && !po_args.quiet)\n+\t\t\tprintf_ln(_(\"Nothing new to pack.\"));\n+\t\t\t\n+\t\tif (pack_everything & PACK_CRUFT) {\n+\t\t\tconst char *pack_prefix = find_pack_prefix(packdir, packtmp);\n+\n+\t\t\tif (!cruft_po_args.window)\n+\t\t\t\tcruft_po_args.window = po_args.window;\n+\t\t\tif (!cruft_po_args.window_memory)\n+\t\t\t\tcruft_po_args.window_memory = po_args.window_memory;\n+\t\t\tif (!cruft_po_args.depth)\n+\t\t\t\tcruft_po_args.depth = po_args.depth;\n+\t\t\tif (!cruft_po_args.threads)\n+\t\t\t\tcruft_po_args.threads = po_args.threads;\n+\t\t\tif (!cruft_po_args.max_pack_size)\n+\t\t\t\tcruft_po_args.max_pack_size = po_args.max_pack_size;\n+\n+\t\t\tcruft_po_args.local = po_args.local;\n+\t\t\tcruft_po_args.quiet = po_args.quiet;\n+\n+\t\t\tret = write_cruft_pack(&cruft_po_args, packtmp, pack_prefix,\n+\t\t\t\t\tcruft_expiration, &names,\n+\t\t\t\t\t&existing);\n \t\t\tif (ret)\n \t\t\t\tgoto cleanup;\n+\n+\t\t\tif (delete_redundant && expire_to) {\n+\t\t\t\t/*\n+\t\t\t\t* If `--expire-to` is given with `-d`, it's possible\n+\t\t\t\t* that we're about to prune some objects. With cruft\n+\t\t\t\t* packs, pruning is implicit: any objects from existing\n+\t\t\t\t* packs that weren't picked up by new packs are removed\n+\t\t\t\t* when their packs are deleted.\n+\t\t\t\t*\n+\t\t\t\t* Generate an additional cruft pack, with one twist:\n+\t\t\t\t* `names` now includes the name of the cruft pack\n+\t\t\t\t* written in the previous step. So the contents of\n+\t\t\t\t* _this_ cruft pack exclude everything contained in the\n+\t\t\t\t* existing cruft pack (that is, all of the unreachable\n+\t\t\t\t* objects which are no older than\n+\t\t\t\t* `--cruft-expiration`).\n+\t\t\t\t*\n+\t\t\t\t* To make this work, cruft_expiration must become NULL\n+\t\t\t\t* so that this cruft pack doesn't actually prune any\n+\t\t\t\t* objects. If it were non-NULL, this call would always\n+\t\t\t\t* generate an empty pack (since every object not in the\n+\t\t\t\t* cruft pack generated above will have an mtime older\n+\t\t\t\t* than the expiration).\n+\t\t\t\t*/\n+\t\t\t\tret = write_cruft_pack(&cruft_po_args, expire_to,\n+\t\t\t\t\t\tpack_prefix,\n+\t\t\t\t\t\tNULL,\n+\t\t\t\t\t\t&names,\n+\t\t\t\t\t\t&existing);\n+\t\t\t\tif (ret)\n+\t\t\t\t\tgoto cleanup;\n+\t\t\t}\n \t\t}\n \t}\n \n-- \n2.46.0\n\n"},{"id":"503459","messageId":"20240925072021.77078-3-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20240925072021.77078-1-hanyang.tony@bytedance.com","subject":"[PATCH 2/2] t0410: adapt tests to repack changes","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-09-25T07:20:21Z","receivedAt":"2024-09-25T07:20:44Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"In the previous commit, we changed how partial repo is cloned.\nAdapt tests to these changes. Also check gc does not delete normal\nobjects too.\n\nSigned-off-by: Han Young \n---\n t/t0410-partial-clone.sh | 68 +---------------------------------------\n 1 file changed, 1 insertion(+), 67 deletions(-)\n\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 34bdb3ab1f..c169b47160 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -499,46 +499,6 @@ test_expect_success 'single promisor remote can be re-initialized gracefully' '\n \tgit -C repo fetch --filter=blob:none foo\n '\n \n-test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '\n-\trm -rf repo &&\n-\ttest_create_repo repo &&\n-\ttest_commit -C repo one &&\n-\ttest_commit -C repo two &&\n-\n-\tTREE_ONE=$(git -C repo rev-parse one^{tree}) &&\n-\tprintf \"$TREE_ONE\\n\" | pack_as_from_promisor &&\n-\tTREE_TWO=$(git -C repo rev-parse two^{tree}) &&\n-\tprintf \"$TREE_TWO\\n\" | pack_as_from_promisor &&\n-\n-\tgit -C repo config core.repositoryformatversion 1 &&\n-\tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n-\tgit -C repo gc &&\n-\n-\t# Ensure that exactly one promisor packfile exists, and that it\n-\t# contains the trees but not the commits\n-\tls repo/.git/objects/pack/pack-*.promisor >promisorlist &&\n-\ttest_line_count = 1 promisorlist &&\n-\tPROMISOR_PACKFILE=$(sed \"s/.promisor/.pack/\" out &&\n-\tgrep \"$TREE_ONE\" out &&\n-\tgrep \"$TREE_TWO\" out &&\n-\t! grep \"$(git -C repo rev-parse one)\" out &&\n-\t! grep \"$(git -C repo rev-parse two)\" out &&\n-\n-\t# Remove the promisor packfile and associated files\n-\trm $(sed \"s/.promisor//\" packlist &&\n-\ttest_line_count = 1 packlist &&\n-\tgit verify-pack repo/.git/objects/pack/pack-*.pack -v >out &&\n-\tgrep \"$(git -C repo rev-parse one)\" out &&\n-\tgrep \"$(git -C repo rev-parse two)\" out &&\n-\t! grep \"$TREE_ONE\" out &&\n-\t! grep \"$TREE_TWO\" out\n-'\n-\n test_expect_success 'gc does not repack promisor objects if there are none' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n@@ -569,7 +529,7 @@ repack_and_check () {\n \tgit -C repo2 cat-file -e $3\n }\n \n-test_expect_success 'repack -d does not irreversibly delete promisor objects' '\n+test_expect_success 'repack -d does not irreversibly delete objects' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n \tgit -C repo config core.repositoryformatversion 1 &&\n@@ -583,40 +543,14 @@ test_expect_success 'repack -d does not irreversibly delete promisor objects' '\n \tTWO=$(git -C repo rev-parse HEAD^^) &&\n \tTHREE=$(git -C repo rev-parse HEAD^) &&\n \n-\tprintf \"$TWO\\n\" | pack_as_from_promisor &&\n \tprintf \"$THREE\\n\" | pack_as_from_promisor &&\n \tdelete_object repo \"$ONE\" &&\n \n-\trepack_and_check --must-fail -ab \"$TWO\" \"$THREE\" &&\n \trepack_and_check -a \"$TWO\" \"$THREE\" &&\n \trepack_and_check -A \"$TWO\" \"$THREE\" &&\n \trepack_and_check -l \"$TWO\" \"$THREE\"\n '\n \n-test_expect_success 'gc stops traversal when a missing but promised object is reached' '\n-\trm -rf repo &&\n-\ttest_create_repo repo &&\n-\ttest_commit -C repo my_commit &&\n-\n-\tTREE_HASH=$(git -C repo rev-parse HEAD^{tree}) &&\n-\tHASH=$(promise_and_delete $TREE_HASH) &&\n-\n-\tgit -C repo config core.repositoryformatversion 1 &&\n-\tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n-\tgit -C repo gc &&\n-\n-\t# Ensure that the promisor packfile still exists, and remove it\n-\ttest -e repo/.git/objects/pack/pack-$HASH.pack &&\n-\trm repo/.git/objects/pack/pack-$HASH.* &&\n-\n-\t# Ensure that the single other pack contains the commit, but not the tree\n-\tls repo/.git/objects/pack/pack-*.pack >packlist &&\n-\ttest_line_count = 1 packlist &&\n-\tgit verify-pack repo/.git/objects/pack/pack-*.pack -v >out &&\n-\tgrep \"$(git -C repo rev-parse HEAD)\" out &&\n-\t! grep \"$TREE_HASH\" out\n-'\n-\n test_expect_success 'do not fetch when checking existence of tree we construct ourselves' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n-- \n2.46.0\n\n"},{"id":"503470","messageId":"a5e3322d-4e63-4b8c-84af-6578fe257cad@gmail.com","threadId":"61890","inReplyTo":"20240925072021.77078-1-hanyang.tony@bytedance.com","subject":"Re: [PATCH 0/2] repack: pack everything into promisor packfile in partial repos","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-09-25T15:20:55Z","receivedAt":"2024-09-25T15:21:00Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Han\n\nOn 25/09/2024 08:20, Han Young wrote:\n> As suggested by Jonathan[1], there are number of ways to fix this issue.\n> We have already explored some of them in this thread, and so far none of them\n> is satisfiable. Calvin and I tried to address the problem from fetch-pack side\n> and rev-list side. But the fix either consumes too much CPU power or results\n> in inefficient bandwidth use.\n\nI was wondering if it would be possible to cache the tip commits in \npromisor packs when repacking so that a subsequent repack only has to \nwalk the commits added since the last repack when it is trying to figure \nout if a local object should be moved into a promisor pack.\n\n> So let's attack the problem from repack side. The goal is to prevent repack\n> from discarding local objects, previously it is done by carefully\n> separating promisor objects and normal objects in rev-list.\n> The implementation is flawed and no solution have been found so far.\n> Instead, we can get ride of rev-list and just pack everything into promisor\n> files. This way, no objects would be lost.\n> \n> By using 'repack everything', repacking requires less work and we are not\n> using more bandwidth. The only downside is normal objects packing does not\n> benefiting from the history and path based delta calculation.\n\nI've just been looking at Documentation/technical/partial-clone.txt and \nI think there are a couple of other implications of this change\n\n > An object may be missing due to a partial clone or fetch, or missing\n > due to repository corruption. To differentiate these cases, the\n > local repository specially indicates such filtered packfiles\n > obtained from promisor remotes as \"promisor packfiles\".\n\nPacking local objects into promisor packfiles means that it is no longer \npossible to detect if an object is missing due to repository corruption \nor because we need to fetch it from a promisor remote.\n\n > `repack` in GC has been updated to not touch promisor packfiles at\n > all, and to only repack other objects.\n\nPacking local objects into promisor packfiles means that GC will \nno-longer remove unreachable local objects.\n\nIt would be helpful if the cover letter or commit messages discussed the \ntradeoffs of these changes and updated that document accordingly.\n\nBest Wishes\n\nPhillip\n\n> Majority of\n> objects in a partial repo is promisor objects, so the impact of worse normal\n> objects repacking is negligible.\n> \n> [1] https://lore.kernel.org/git/20240813004508.2768102-1-jonathantanmy@google.com/\n> \n> Han Young (2):\n> repack: pack everything into promisor packfile in partial repos\n> t0410: adapt tests to repack changes\n> \n> builtin/repack.c | 258 ++++++++++++++++++++++-----------------\n> t/t0410-partial-clone.sh | 68 +----------\n> 2 files changed, 145 insertions(+), 181 deletions(-)\n> \n"},{"id":"503476","messageId":"xmqqfrpnptl5.fsf@gitster.g","threadId":"61890","inReplyTo":"a5e3322d-4e63-4b8c-84af-6578fe257cad@gmail.com","subject":"Re: [PATCH 0/2] repack: pack everything into promisor packfile in partial repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-25T16:48:54Z","receivedAt":"2024-09-25T16:48:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> I was wondering if it would be possible to cache the tip commits in\n> promisor packs when repacking so that a subsequent repack only has to\n> walk the commits added since the last repack when it is trying to\n> figure out if a local object should be moved into a promisor pack.\n\nI was wondering the same thing. If packfiles (and bundles) record\nthe entry points and the exit points of the DAG, it would help quite\na bit.\n\n> It would be helpful if the cover letter or commit messages discussed\n> the tradeoffs of these changes and updated that document accordingly.\n\nI like the suggestion very much.\n\nThanks for a review.\n"},{"id":"503480","messageId":"xmqqwmizoeco.fsf@gitster.g","threadId":"61890","inReplyTo":"20240925072021.77078-1-hanyang.tony@bytedance.com","subject":"Re: [PATCH 0/2] repack: pack everything into promisor packfile in partial repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-25T17:03:19Z","receivedAt":"2024-09-25T17:03:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Young writes:\n\n> By using 'repack everything', repacking requires less work and we are not\n> using more bandwidth. The only downside is normal objects packing does not\n> benefiting from the history and path based delta calculation. Majority of\n> objects in a partial repo is promisor objects, so the impact of worse normal\n> objects repacking is negligible.\n\nThere is an important assumption that any objects in promisor packs\n*and* any objects that are (directly or indirectly) referenced by\nthese objects in promisor packs can safely be expunged from the\nlocal object store because they can be later fetched again from the\npromisor remote. In that (in)famous failure case topology of the\nhistory:\n\n commit tree blob\n C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n |\n C2 ---- T2 -- B2 (created locally, in non-promisor pack)\n |\n C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n\neven though the objects associated with the commit C2 are created\nlocally, the fact that C3 in promisor pack references it alone is\nsufficient for us to also assume that these \"locally created\" are\nnow refetchable from the promisor remote that gave us C3, hence it\nis safe to repack the history leading to C3 and all objects involved\nin the history and mark the resulting pack(s) promisor packs.\n\nOK. That sounds workable, but aren't there downsides?\n\nThanks for working on this topic.\n\n"},{"id":"503863","messageId":"20241001191811.1934900-1-calvinwan@google.com","threadId":"61890","inReplyTo":"20240802073143.56731-1-hanyang.tony@bytedance.com","subject":"Missing Promisor Objects in Partial Repo Design Doc","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-10-01T19:17:51Z","receivedAt":"2024-10-01T19:18:25Z","isPatch":false,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"It seems that we're at a standstill for the various possible designs\nthat can solve this problem, so I decided to write up a design document\nto discuss the ideas we've come up with so far and new ones. Hopefully\nthis will get us closer to a viable implementation we can agree on.\n\nMissing Promisor Objects in Partial Repo Design Doc\n===================================================\n\nBasic Reproduction Steps\n------------------------\n\n - Partial clone repository\n - Create local commit and push\n - Fetch new changes\n - Garbage collection\n\nState After Reproduction\n------------------------\n\ncommit tree blob\n C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n |\n C2b ---- T2b -- B2b (created locally, in non-promisor pack)\n |\n C2a ---- T2a -- B2a (created locally, in non-promisor pack)\n |\n C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n\nExplanation of the Problem\n--------------------------\n\nIn a partial clone repository, non-promisor commits are locally\ncommitted as children of promisor commits and then pushed up to the\nserver. Fetches of new history can result in promisor commits that have\nnon-promisor commits as ancestors. During garbage collection, objects\nare repacked in 2 steps. In the first step, if there is more than one\npromisor packfile, all objects in promisor packfiles are repacked into a\nsingle promisor packfile. In the second step, a revision walk is made\nfrom all refs (and some other things like HEAD and reflog entries) that\nstops whenever it encounters a promisor object. In the example above, if\na ref pointed directly to C2a, it would be returned by the walk (as an\nobject to be packed). But if we only had a ref pointing to C3, the\nrevision walk immediately sees that it is a promisor object, does not\nreturn it, and does not iterate through its parents.\n\n(C2b is a bit of a special case. Despite not being in a promisor pack,\nit is still considered to be a promisor object since C3 directly\nreferences it.)\n\nIf we think this is a bad state, we should propagate the “promisor-ness”\nof C3 to its ancestors. Git commands should either prevent this state\nfrom occurring or tolerate it and fix it when we can. If we did run into\nthis state unexpectedly, then it would be considered a BUG.\n\nIf we think it is a valid state, we should NOT propagate the\n“promisor-ness” of C3 to its ancestors. Git commands should respect that\nthis is a possible state and be able to work around it. Therefore, this\nbug would then be strictly caused by garbage collection\n\n\nBad State Solutions\n===================\n\nFetch negotiation\n-----------------\nImplemented at\nhttps://lore.kernel.org/git/20240919234741.1317946-1-calvinwan@google.com/\n\nDuring fetch negotiation, if a commit is not in a promisor pack and\ntherefore local, do not declare it as \"have\" so they can be fetched into\na promisor pack.\n\nCost:\n- Creation of set of promisor pack objects (by iterating through every\n .idx of promisor packs)\n- Refetch number of local commits\n\nPros: Implementation is simple, client doesn’t have to repack, prevents\nstate from ever occurring in the repository.\n\nCons: Network cost of refetching could be high if many local commits\nneed to be refetched.\n\ncommit tree blob\n C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n |\n C2 ---- T2 -- B2 (created locally, refetched into promisor pack)\n |\n C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n\nFetch repack\n------------\nNot yet implemented.\n\nEnumerate the objects in the freshly fetched promisor packs, checking\nevery outgoing link to see if they reference a non-promisor object that\nwe have, to get a list of tips where local objects are parents of\npromisor objects (\"bad history\"). After collecting these \"tips of bad\nhistory\", you then start another traversal from them until you hit an\nobject in a promisor pack and stop traversal there. You have\nsuccessfully enumerated the local objects to be repacked into a promisor\npack.\n\nCost:\n- Traversal through newly fetched promisor trees and commits\n- Creation of set of promisor pack objects (for tips of bad history\n traversal to stop at a promisor object)\n- Traversal through all local commits and check existence in promisor\n pack set\n- Repack all pushed local commits\n\nPros: Prevents state from ever occurring in the repository, no network\ncost.\n\nCons: Additional cost of repacking is incurred during fetch, more\ncomplex implementation.\n\ncommit tree blob\n C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n |\n C2 ---- T2 -- B2 (created locally, packed into promisor pack)\n |\n C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n\nGarbage Collection repack\n-------------------------\nNot yet implemented.\n\nSame concept at “fetch repack”, but happens during garbage collection\ninstead. The traversal is more expensive since we no longer have access\nto what was recently fetched so we have to traverse through all promisor\npacks to collect tips of “bad” history.\n\nCost:\n- Creation of set of promisor pack objects\n- Traversal through all promisor commits\n- Traversal through all local commits and check existence in promisor\n object set\n- Repack all pushed local commits\n\nPros: Can be run in the background as part of maintenance, no network\ncost.\n\nCons: More expensive than “fetch repack”, state isn’t fixed until\ngarbage collection, more complex implementation\n\ncommit tree blob\n C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n |\n C2 ---- T2 -- B2 (created locally, packed into promisor pack)\n |\n C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n\nGarbage Collection repack all\n-----------------------------\nImplemented at\nhttps://lore.kernel.org/git/20240925072021.77078-1-hanyang.tony@bytedance.com/ \n\nRepack all local commits into promisor packs during garbage collection.\n\nBoth valid scenarios\ncommit tree blob\n C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n |\n C2 ---- T2 -- B2 (created locally, packed into promisor pack)\n |\n C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n\ncommit tree blob\n C3 ---- T3 -- B3 (created locally, packed into promisor pack)\n |\n C2 ---- T2 -- B2 (created locally, packed into promisor pack)\n |\n C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n\nCost:\nRepack all local commits\n\nPros: Can be run in the background as part of maintenance, no network\ncost, less complex implementation, and less expensive than “garbage\ncollection repack”.\n\nCons: Packing local objects into promisor packs means that it is no\nlonger possible to detect if an object is missing due to repository\ncorruption or because we need to fetch it from a promisor remote.\nPacking local objects into promisor packs means that garbage collection\nwill no longer remove unreachable local objects.\n\nValid State Solutions\n=====================\nGarbage Collection check\n------------------------\nNot yet implemented.\n\nCurrently during the garbage collection rev walk, whenever a promisor\ncommit is reached, it is marked UNINTERESTING, and then subsequently all\nancestors of the promisor commit are traversed and also marked\nUNINTERESTING. Therefore, add a check for whether a commit is local or\nnot during promisor commit ancestor traversal and do not mark local\ncommits as UNINTERESTING.\n\ncommit tree blob\n C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n |\n C2 ---- T2 -- B2 (created locally, in non-promisor pack, gc does not delete)\n |\n C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n\nCost:\n- Adds an additional check to every ancestor of a promisor commit.\n\nThis is practically the only solution if the state is valid. Fsck would\nalso have to start checking for validity of ancestors of promisor\ncommits instead of ignoring them as it currently does.\n\nOptimizations\n=============\n\nThe “creation of set of promisor pack objects” can be replaced with\n“creation of set of non-promisor objects” since the latter is almost\nalways cheaper and we can check for non-existence rather than existence.\nThis does not work for “fetch negotiation” since if we have a commit\nthat's in both a promisor pack and a non-promisor pack, the algorithm's\ncorrectness relies on the fact that we report it as a promisor object\n(because we really need the server to re-send it).\n"},{"id":"503869","messageId":"xmqq34lfvco6.fsf@gitster.g","threadId":"61890","inReplyTo":"20241001191811.1934900-1-calvinwan@google.com","subject":"Re: Missing Promisor Objects in Partial Repo Design Doc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-01T19:35:53Z","receivedAt":"2024-10-01T19:35:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calvin Wan writes:\n\n> It seems that we're at a standstill for the various possible designs\n> that can solve this problem, so I decided to write up a design document\n> to discuss the ideas we've come up with so far and new ones. Hopefully\n> this will get us closer to a viable implementation we can agree on.\n\nThanks for writing this up. Very much appreciated.\n\nI'll hold my thoughts before others have chance to speak up, though.\n\nThanks.\n"},{"id":"503883","messageId":"xmqqo743qkn9.fsf@gitster.g","threadId":"61890","inReplyTo":"20241001191811.1934900-1-calvinwan@google.com","subject":"Re: Missing Promisor Objects in Partial Repo Design Doc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-02T02:54:50Z","receivedAt":"2024-10-02T02:54:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\n> Missing Promisor Objects in Partial Repo Design Doc\n> ===================================================\n>\n> Basic Reproduction Steps\n> ------------------------\n>\n> - Partial clone repository\n> - Create local commit and push\n> - Fetch new changes\n> - Garbage collection\n>\n> State After Reproduction\n> ------------------------\n>\n> commit tree blob\n> C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n> |\n> C2b ---- T2b -- B2b (created locally, in non-promisor pack)\n> |\n> C2a ---- T2a -- B2a (created locally, in non-promisor pack)\n> |\n> C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n>\n> Explanation of the Problem\n> --------------------------\n>\n> In a partial clone repository, non-promisor commits are locally\n> committed as children of promisor commits and then pushed up to the\n> server. Fetches of new history can result in promisor commits that have\n> non-promisor commits as ancestors. During garbage collection, objects\n> are repacked in 2 steps. In the first step, if there is more than one\n> promisor packfile, all objects in promisor packfiles are repacked into a\n> single promisor packfile. In the second step, a revision walk is made\n> from all refs (and some other things like HEAD and reflog entries) that\n> stops whenever it encounters a promisor object. In the example above, if\n> a ref pointed directly to C2a, it would be returned by the walk (as an\n> object to be packed). But if we only had a ref pointing to C3, the\n> revision walk immediately sees that it is a promisor object, does not\n> return it, and does not iterate through its parents.\n\nTrue. Will it become even worse, if a protocol extension Christian\nproposes starts suggesting a repository that is not lazy to add a\npromisor remote? In such a set-up, perhaps all history leading to\nC2b down to the root are local, but C3 may have come from a promisor\nremote (hence in a promisor pack).\n\n> (C2b is a bit of a special case. Despite not being in a promisor pack,\n> it is still considered to be a promisor object since C3 directly\n> references it.)\n\nYes, and I suspect the root cause of this confusion is because\n\"promisor object\", as defined today, is a flawed concept. If C2b\nwere pointed by a local ref, just like the case the ref points at\nC2a, they should be treated the same way, as both of them are\nlocally created. To put it another way, presumably the local have\nalready been pushed out to elsewhere and the promisor remote got\nhold of them, and that is why C3 can build on top of them. And the\nfact C2b is directly reachable from C3 and C2a is not should not\nhave any relevance if C2a or C2b are not _included_ in promisor\npacks (hence both of them need to be included in the local pack).\n\nTwo concepts that would have been useful are (1) objects that are in\npromisor packs and (2) objects that are reachable from an object\nthat is in a promisor pack. I do not see how the current definition\nof \"promisor objects\" (i.e. in a promisor pack, or one hop from an\nobject in a promisor pack) is useful in any context.\n\n> If we think this is a bad state, we should propagate the “promisor-ness”\n> of C3 to its ancestors. Git commands should either prevent this state\n> from occurring or tolerate it and fix it when we can. If we did run into\n> this state unexpectedly, then it would be considered a BUG.\n\nYup, that is the basis of the solutions we saw proposed so far.\n\n> If we think it is a valid state, we should NOT propagate the\n> “promisor-ness” of C3 to its ancestors. Git commands should respect that\n> this is a possible state and be able to work around it. Therefore, this\n> bug would then be strictly caused by garbage collection\n\nYes, that is possibly an alternative.\n\n> Bad State Solutions\n> ===================\n>\n> Fetch negotiation\n> -----------------\n> Implemented at\n> https://lore.kernel.org/git/20240919234741.1317946-1-calvinwan@google.com/\n>\n> During fetch negotiation, if a commit is not in a promisor pack and\n> therefore local, do not declare it as \"have\" so they can be fetched into\n> a promisor pack.\n>\n> Cost:\n> - Creation of set of promisor pack objects (by iterating through every\n> .idx of promisor packs)\n\nWhat is \"promisor PACK objects\"? Is it different from the \"promisor\nobjects\" (i.e. what I called the useless definition above)?\n\n> - Refetch number of local commits\n>\n> Pros: Implementation is simple, client doesn’t have to repack, prevents\n> state from ever occurring in the repository.\n>\n> Cons: Network cost of refetching could be high if many local commits\n> need to be refetched.\n\nWhat if we get into the same state by creating local C4, which gets\nto outside and on top of which C5 is built, which is now sitting at\nthe tip of the remote history and we fetch from them? In order to\ninclude C4 in the \"promisor pack\", we refrain from saying C4 is a\n\"have\" for us and refetch. Would C2 be fetched again?\n\nI do not think C2 would be, because we made it an object in a\npromisor pack when we \"fixed\" the history for C3.\n\nSo the cost will not grow proportionally to the depth of the\nhistory, which makes it OK from my point of view.\n\n> Garbage Collection repack\n> -------------------------\n> Not yet implemented.\n>\n> Same concept at “fetch repack”, but happens during garbage collection\n> instead. The traversal is more expensive since we no longer have access\n> to what was recently fetched so we have to traverse through all promisor\n> packs to collect tips of “bad” history.\n\nIn other words, with the status quo, \"git gc\" that attempts to\nrepack \"objects in promisor packs\" and \"other objects that did not\nget repacked in the step that repack objects in promisor packs\"\nseparately, it implements the latter in a buggy way and discards\nsome objects. And fixing that bug by doing the right thing is\nexpensive.\n\nStepping back a bit, why is the loss of C2a/C2b/C2 a problem after\n\"git gc\"? Wouldn't these \"missing\" objects be lazily fetchable, now\nC3 is known to the remote and the remote promises everything\nreachable from what they offer are (re)fetchable from them? IOW, is\nthis a correctness issue, or only performance issue (of having to\nre-fetch what we once locally had)?\n\n> Cons: Packing local objects into promisor packs means that it is no\n> longer possible to detect if an object is missing due to repository\n> corruption or because we need to fetch it from a promisor remote.\n\nIs this true? Can we tell, when trying to access C2a/C2b/C2 after\nthe current version of \"git gc\" removes them from the local object\nstore, that they are missing due to repository corruption? After\nall, C3 can reach them so wouldn't it be possible for us to fetch\nthem from the promisor remote?\n\nAfter a lazy clone that omits a lot of objects acquires many objects\nover time by fetching missing objects on demand, wouldn't we want to\nhave an option to \"slim\" the local repository by discarding some of\nthese objects (the ones that are least frequently used), relying on\nthe promise by the promisor remote that even if we did so, they can\nbe fetched again? Can we treat loss of C2a/C2b/C2 as if such a\nfeature prematurely kicked in? Or are we failing to refetch them\nfor some reason?\n\n> Packing local objects into promisor packs means that garbage collection\n> will no longer remove unreachable local objects.\n>\n> Valid State Solutions\n> =====================\n> Garbage Collection check\n> ------------------------\n> Not yet implemented.\n>\n> Currently during the garbage collection rev walk, whenever a promisor\n> commit is reached, it is marked UNINTERESTING, and then subsequently all\n> ancestors of the promisor commit are traversed and also marked\n> UNINTERESTING. Therefore, add a check for whether a commit is local or\n> not during promisor commit ancestor traversal and do not mark local\n> commits as UNINTERESTING.\n>\n> commit tree blob\n> C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n> |\n> C2 ---- T2 -- B2 (created locally, in non-promisor pack, gc does not delete)\n> |\n> C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n>\n> Cost:\n> - Adds an additional check to every ancestor of a promisor commit.\n>\n> This is practically the only solution if the state is valid. Fsck would\n> also have to start checking for validity of ancestors of promisor\n> commits instead of ignoring them as it currently does.\n\nIn the longer term, this looks like the most straight-forward and\neasy to explain solution to me.\n\n> Optimizations\n> =============\n>\n> The “creation of set of promisor pack objects” can be replaced with\n> “creation of set of non-promisor objects” since the latter is almost\n> always cheaper and we can check for non-existence rather than existence.\n> This does not work for “fetch negotiation” since if we have a commit\n> that's in both a promisor pack and a non-promisor pack, the algorithm's\n> correctness relies on the fact that we report it as a promisor object\n> (because we really need the server to re-send it).\n"},{"id":"503892","messageId":"CAG1j3zFUGPev2voyEocv=G8=NSUDqH2Bbr5_dpfuS9ua8tTs6Q@mail.gmail.com","threadId":"61890","inReplyTo":"xmqqo743qkn9.fsf@gitster.g","subject":"Re: [External] Re: Missing Promisor Objects in Partial Repo Design Doc","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-10-02T07:57:57Z","receivedAt":"2024-10-02T07:58:09Z","isPatch":false,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"On Wed, Oct 2, 2024 at 10:55 AM Junio C Hamano wrote:\n\n> Stepping back a bit, why is the loss of C2a/C2b/C2 a problem after\n> \"git gc\"? Wouldn't these \"missing\" objects be lazily fetchable, now\n> C3 is known to the remote and the remote promises everything\n> reachable from what they offer are (re)fetchable from them? IOW, is\n> this a correctness issue, or only performance issue (of having to\n> re-fetch what we once locally had)?\n>\n> Is this true? Can we tell, when trying to access C2a/C2b/C2 after\n> the current version of \"git gc\" removes them from the local object\n> store, that they are missing due to repository corruption? After\n> all, C3 can reach them so wouldn't it be possible for us to fetch\n> them from the promisor remote?\n>\n> After a lazy clone that omits a lot of objects acquires many objects\n> over time by fetching missing objects on demand, wouldn't we want to\n> have an option to \"slim\" the local repository by discarding some of\n> these objects (the ones that are least frequently used), relying on\n> the promise by the promisor remote that even if we did so, they can\n> be fetched again? Can we treat loss of C2a/C2b/C2 as if such a\n> feature prematurely kicked in? Or are we failing to refetch them\n> for some reason?\n\nIn a blobless clone, we expect commits and trees to be present in repo.\nIf C2/T2 is missing, commands like \"git merge\" will complain\n\"cannot merge unrelated history\" and fail. Or commands like \"git log\" will\ntry to lazily fetch the commit, but without 'have' negotiation, end up\npulling all the trees and blobs reachable from that commit.\n\nIt's possible to minimize the impact of missing commits by adding negotiation\nto lazy fetching, but we probably need to adapt code in many places where\nwe don't do lazy fetching. \"git log\", \"git merge\" commit graph etc. it's\nno trivia amount of work.\n"},{"id":"503988","messageId":"20241002223533.1408491-1-calvinwan@google.com","threadId":"61890","inReplyTo":"CAG1j3zHJVrpK5JZtUXFwkZgWY1-CxqET+ygpaMqo5aM-KeWaxg@mail.gmail.com","subject":"Re: [External] Re: [PATCH 2/2] fetch-pack.c: do not declare local commits as \"have\" in partial repos","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-10-02T22:35:21Z","receivedAt":"2024-10-02T22:35:37Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"韩仰 writes:\n> On Sun, Sep 22, 2024 at 2:53 PM Junio C Hamano wrote:\n> \n> > I was hoping to see that the issue can be fixed on the \"gc\" side,\n> > regardless of how the objects enter our repository, but perhaps I am\n> > missing something. Isn't it just the matter of collecting C1, C3\n> > but not C2? Or to put it another way, if we first create a list of\n> > objects to be packed (regardless of whether they are in promisor\n> > packs), and then remove the objects that are in promisor packs from\n> > the list, and pack the objects still remaining in the list?\n> \n> I tried to fix the issue on the \"gc\" side following JTan's suggestion,\n> by packing local objects referenced by promisor objects into promisor\n> packs. But it turns out the cost for \"for each promisor object,\n> parse them and try to decide the objects they reference is in local repo\"\n> is too great. In a test blob:none partial clone repo, the gc would take more\n> than one hour in the 2019 MacBook, despite the repo only\n> having 17071073 objects. Normally it would take about 30 minutes.\n\nI found that running `time git submodule foreach git ` on Android takes 25 minutes on my machine. Granted this is\nsingle threaded but it's still quite an expensive operation to be doing\non every recursive fetch. If this operation is so expensive, then unless\nwe can figure out some method that doesn't involve creating a set of\npromisor pack objects, solving this during fetch is infeasible.\n"},{"id":"504398","messageId":"20241008081350.8950-1-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20240802073143.56731-1-hanyang.tony@bytedance.com","subject":"[PATCH v2 0/3] repack: pack everything into promisor packfile in partial repos","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-10-08T08:13:47Z","receivedAt":"2024-10-08T08:14:01Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"As suggested by Jonathan[1], there are number of ways to fix this issue.\nWe have already explored some of them in this thread, and so far none of them\nis satisfiable. Calvin and I tried to address the problem from fetch-pack side\nand rev-list side. But the fix either consumes too much CPU power or results\nin inefficient bandwidth use.\n\nSo let's attack the problem from repack side. The goal is to prevent repack\nfrom discarding local objects, previously it is done by carefully\nseparating promisor objects and normal objects in rev-list.\nThe implementation is flawed and no solution have been found so far.\nInstead, we can get ride of rev-list and just pack everything into promisor\nfiles. This way, no objects would be lost.\n\nBy using 'repack everything', repacking requires less work and we are not\nusing more bandwidth.\n\nPacking local objects into promisor packfiles means that it is no longer\npossible to detect if an object is missing due to repository corruption\nor because we need to fetch it from a promisor remote.\n\nPromisor objects packing does not benefiting from the history and\npath based delta calculation, and GC does not remove unreachable promisor\nobjects. By packing locally created normal objects into promisor packfile,\nnormal objects are converted into promisor objects. However, in partial cloned\nrepos, the number of locally created objects are small compared to promisor\nobjects. The impact should be negligible.\n\n[1] https://lore.kernel.org/git/20240813004508.2768102-1-jonathantanmy@google.com/\n\n*** Changes since v1 ***\nAdded tradeoffs in cover letter.\nFixed some partial clone test cases.\nUpdated partial clone documentation.\n\nHan Young (3):\n repack: pack everything into packfile\n t0410: adapt tests to repack changes\n partial-clone: update doc\n\n Documentation/technical/partial-clone.txt | 16 +-\n builtin/repack.c | 257 ++++++++++++----------\n t/t0410-partial-clone.sh | 68 +-----\n t/t5616-partial-clone.sh | 9 +-\n 4 files changed, 157 insertions(+), 193 deletions(-)\n\n-- \n2.46.0\n\n"},{"id":"504399","messageId":"20241008081350.8950-2-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20241008081350.8950-1-hanyang.tony@bytedance.com","subject":"[PATCH v2 1/3] repack: pack everything into packfile","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-10-08T08:13:48Z","receivedAt":"2024-10-08T08:14:05Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"In a partial repository, creating a local commit and then fetching\ncauses the following state to occur:\n\ncommit tree blob\n C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n |\n C2 ---- T2 -- B2 (created locally, in non-promisor pack)\n |\n C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n\nDuring garbage collection, parents of promisor objects are marked as\nUNINTERESTING and are subsequently garbage collected. In this case, C2\nwould be deleted and attempts to access that commit would result in \"bad\nobject\" errors (originally reported here[1]).\n\nFor partial repos, repacking is done in two steps. We first repack all the\nobjects in promisor packfile, then repack all the non-promisor objects.\nIt turns out C2, T2 and B2 are not repacked in either steps, ended up deleted.\nWe can avoid this by packing everything into a promisor packfile, if the repo\nis partial cloned.\n\n[1] https://lore.kernel.org/git/20240802073143.56731-1-hanyang.tony@bytedance.com/\n\nHelped-by: Calvin Wan \nSigned-off-by: Han Young \n---\n builtin/repack.c | 257 ++++++++++++++++++++++++++---------------------\n 1 file changed, 143 insertions(+), 114 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex cb4420f085..e9e18a31fe 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -321,6 +321,23 @@ static int write_oid(const struct object_id *oid,\n \treturn 0;\n }\n \n+static int write_loose_oid(const struct object_id *oid,\n+\t\t\t\t const char *path UNUSED,\n+\t\t\t\t void *data)\n+{\n+\tstruct child_process *cmd = data;\n+\n+\tif (cmd->in == -1) {\n+\t\tif (start_command(cmd))\n+\t\t\tdie(_(\"could not start pack-objects to repack promisor objects\"));\n+\t}\n+\n+\tif (write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n+\t write_in_full(cmd->in, \"\\n\", 1) < 0)\n+\t\tdie(_(\"failed to feed promisor objects to pack-objects\"));\n+\treturn 0;\n+}\n+\n static struct {\n \tconst char *name;\n \tunsigned optional:1;\n@@ -370,12 +387,15 @@ static int has_pack_ext(const struct generated_pack_data *data,\n \tBUG(\"unknown pack extension: '%s'\", ext);\n }\n \n-static void repack_promisor_objects(const struct pack_objects_args *args,\n-\t\t\t\t struct string_list *names)\n+static int repack_promisor_objects(const struct pack_objects_args *args,\n+\t\t\t\t struct string_list *names,\n+\t\t\t\t struct string_list *list,\n+\t\t\t\t int pack_all)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tFILE *out;\n \tstruct strbuf line = STRBUF_INIT;\n+\tstruct string_list_item *item;\n \n \tprepare_pack_objects(&cmd, args, packtmp);\n \tcmd.in = -1;\n@@ -387,13 +407,19 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \t * {type -> existing pack order} ordering when computing deltas instead\n \t * of a {type -> size} ordering, which may produce better deltas.\n \t */\n-\tfor_each_packed_object(write_oid, &cmd,\n-\t\t\t FOR_EACH_OBJECT_PROMISOR_ONLY);\n+\tif (pack_all)\n+\t\tfor_each_packed_object(write_oid, &cmd, 0);\n+\telse\n+\t\tfor_each_string_list_item(item, list) {\n+\t\t\tpack_mark_retained(item);\n+\t\t}\n+\n+\tfor_each_loose_object(write_loose_oid, &cmd, 0);\n \n \tif (cmd.in == -1) {\n \t\t/* No packed objects; cmd was never started */\n \t\tchild_process_clear(&cmd);\n-\t\treturn;\n+\t\treturn 0;\n \t}\n \n \tclose(cmd.in);\n@@ -431,6 +457,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \tif (finish_command(&cmd))\n \t\tdie(_(\"could not finish pack-objects to repack promisor objects\"));\n \tstrbuf_release(&line);\n+\treturn 0;\n }\n \n struct pack_geometry {\n@@ -1312,8 +1339,7 @@ int cmd_repack(int argc,\n \t\tstrvec_push(&cmd.args, \"--reflog\");\n \t\tstrvec_push(&cmd.args, \"--indexed-objects\");\n \t}\n-\tif (repo_has_promisor_remote(the_repository))\n-\t\tstrvec_push(&cmd.args, \"--exclude-promisor-objects\");\n+\n \tif (!write_midx) {\n \t\tif (write_bitmaps > 0)\n \t\t\tstrvec_push(&cmd.args, \"--write-bitmap-index\");\n@@ -1323,125 +1349,128 @@ int cmd_repack(int argc,\n \tif (use_delta_islands)\n \t\tstrvec_push(&cmd.args, \"--delta-islands\");\n \n-\tif (pack_everything & ALL_INTO_ONE) {\n-\t\trepack_promisor_objects(&po_args, &names);\n-\n-\t\tif (has_existing_non_kept_packs(&existing) &&\n-\t\t delete_redundant &&\n-\t\t !(pack_everything & PACK_CRUFT)) {\n-\t\t\tfor_each_string_list_item(item, &names) {\n-\t\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n-\t\t\t\t\t packtmp_name, item->string);\n-\t\t\t}\n-\t\t\tif (unpack_unreachable) {\n-\t\t\t\tstrvec_pushf(&cmd.args,\n-\t\t\t\t\t \"--unpack-unreachable=%s\",\n-\t\t\t\t\t unpack_unreachable);\n-\t\t\t} else if (pack_everything & LOOSEN_UNREACHABLE) {\n-\t\t\t\tstrvec_push(&cmd.args,\n-\t\t\t\t\t \"--unpack-unreachable\");\n-\t\t\t} else if (keep_unreachable) {\n-\t\t\t\tstrvec_push(&cmd.args, \"--keep-unreachable\");\n-\t\t\t\tstrvec_push(&cmd.args, \"--pack-loose-unreachable\");\n+\tif (repo_has_promisor_remote(the_repository)) {\n+\t\tret = repack_promisor_objects(&po_args, &names,\n+\t\t\t&existing.non_kept_packs, pack_everything & ALL_INTO_ONE);\n+\t} else {\n+\t\tif (pack_everything & ALL_INTO_ONE) {\n+\t\t\tif (has_existing_non_kept_packs(&existing) &&\n+\t\t\tdelete_redundant &&\n+\t\t\t!(pack_everything & PACK_CRUFT)) {\n+\t\t\t\tfor_each_string_list_item(item, &names) {\n+\t\t\t\t\tstrvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n+\t\t\t\t\t\tpacktmp_name, item->string);\n+\t\t\t\t}\n+\t\t\t\tif (unpack_unreachable) {\n+\t\t\t\t\tstrvec_pushf(&cmd.args,\n+\t\t\t\t\t\t\"--unpack-unreachable=%s\",\n+\t\t\t\t\t\tunpack_unreachable);\n+\t\t\t\t} else if (pack_everything & LOOSEN_UNREACHABLE) {\n+\t\t\t\t\tstrvec_push(&cmd.args,\n+\t\t\t\t\t\t\"--unpack-unreachable\");\n+\t\t\t\t} else if (keep_unreachable) {\n+\t\t\t\t\tstrvec_push(&cmd.args, \"--keep-unreachable\");\n+\t\t\t\t\tstrvec_push(&cmd.args, \"--pack-loose-unreachable\");\n+\t\t\t\t}\n \t\t\t}\n+\t\t} else if (geometry.split_factor) {\n+\t\t\tstrvec_push(&cmd.args, \"--stdin-packs\");\n+\t\t\tstrvec_push(&cmd.args, \"--unpacked\");\n+\t\t} else {\n+\t\t\tstrvec_push(&cmd.args, \"--unpacked\");\n+\t\t\tstrvec_push(&cmd.args, \"--incremental\");\n \t\t}\n-\t} else if (geometry.split_factor) {\n-\t\tstrvec_push(&cmd.args, \"--stdin-packs\");\n-\t\tstrvec_push(&cmd.args, \"--unpacked\");\n-\t} else {\n-\t\tstrvec_push(&cmd.args, \"--unpacked\");\n-\t\tstrvec_push(&cmd.args, \"--incremental\");\n-\t}\n \n-\tif (po_args.filter_options.choice)\n-\t\tstrvec_pushf(&cmd.args, \"--filter=%s\",\n-\t\t\t expand_list_objects_filter_spec(&po_args.filter_options));\n-\telse if (filter_to)\n-\t\tdie(_(\"option '%s' can only be used along with '%s'\"), \"--filter-to\", \"--filter\");\n+\t\tif (po_args.filter_options.choice)\n+\t\t\tstrvec_pushf(&cmd.args, \"--filter=%s\",\n+\t\t\t\texpand_list_objects_filter_spec(&po_args.filter_options));\n+\t\telse if (filter_to)\n+\t\t\tdie(_(\"option '%s' can only be used along with '%s'\"), \"--filter-to\", \"--filter\");\n \n-\tif (geometry.split_factor)\n-\t\tcmd.in = -1;\n-\telse\n-\t\tcmd.no_stdin = 1;\n+\t\tif (geometry.split_factor)\n+\t\t\tcmd.in = -1;\n+\t\telse\n+\t\t\tcmd.no_stdin = 1;\n \n-\tret = start_command(&cmd);\n-\tif (ret)\n-\t\tgoto cleanup;\n+\t\tret = start_command(&cmd);\n+\t\tif (ret)\n+\t\t\tgoto cleanup;\n \n-\tif (geometry.split_factor) {\n-\t\tFILE *in = xfdopen(cmd.in, \"w\");\n-\t\t/*\n-\t\t * The resulting pack should contain all objects in packs that\n-\t\t * are going to be rolled up, but exclude objects in packs which\n-\t\t * are being left alone.\n-\t\t */\n-\t\tfor (i = 0; i < geometry.split; i++)\n-\t\t\tfprintf(in, \"%s\\n\", pack_basename(geometry.pack[i]));\n-\t\tfor (i = geometry.split; i < geometry.pack_nr; i++)\n-\t\t\tfprintf(in, \"^%s\\n\", pack_basename(geometry.pack[i]));\n-\t\tfclose(in);\n-\t}\n+\t\tif (geometry.split_factor) {\n+\t\t\tFILE *in = xfdopen(cmd.in, \"w\");\n+\t\t\t/*\n+\t\t\t* The resulting pack should contain all objects in packs that\n+\t\t\t* are going to be rolled up, but exclude objects in packs which\n+\t\t\t* are being left alone.\n+\t\t\t*/\n+\t\t\tfor (i = 0; i < geometry.split; i++)\n+\t\t\t\tfprintf(in, \"%s\\n\", pack_basename(geometry.pack[i]));\n+\t\t\tfor (i = geometry.split; i < geometry.pack_nr; i++)\n+\t\t\t\tfprintf(in, \"^%s\\n\", pack_basename(geometry.pack[i]));\n+\t\t\tfclose(in);\n+\t\t}\n \n-\tret = finish_pack_objects_cmd(&cmd, &names, 1);\n-\tif (ret)\n-\t\tgoto cleanup;\n-\n-\tif (!names.nr && !po_args.quiet)\n-\t\tprintf_ln(_(\"Nothing new to pack.\"));\n-\n-\tif (pack_everything & PACK_CRUFT) {\n-\t\tconst char *pack_prefix = find_pack_prefix(packdir, packtmp);\n-\n-\t\tif (!cruft_po_args.window)\n-\t\t\tcruft_po_args.window = po_args.window;\n-\t\tif (!cruft_po_args.window_memory)\n-\t\t\tcruft_po_args.window_memory = po_args.window_memory;\n-\t\tif (!cruft_po_args.depth)\n-\t\t\tcruft_po_args.depth = po_args.depth;\n-\t\tif (!cruft_po_args.threads)\n-\t\t\tcruft_po_args.threads = po_args.threads;\n-\t\tif (!cruft_po_args.max_pack_size)\n-\t\t\tcruft_po_args.max_pack_size = po_args.max_pack_size;\n-\n-\t\tcruft_po_args.local = po_args.local;\n-\t\tcruft_po_args.quiet = po_args.quiet;\n-\n-\t\tret = write_cruft_pack(&cruft_po_args, packtmp, pack_prefix,\n-\t\t\t\t cruft_expiration, &names,\n-\t\t\t\t &existing);\n+\t\tret = finish_pack_objects_cmd(&cmd, &names, 1);\n \t\tif (ret)\n \t\t\tgoto cleanup;\n \n-\t\tif (delete_redundant && expire_to) {\n-\t\t\t/*\n-\t\t\t * If `--expire-to` is given with `-d`, it's possible\n-\t\t\t * that we're about to prune some objects. With cruft\n-\t\t\t * packs, pruning is implicit: any objects from existing\n-\t\t\t * packs that weren't picked up by new packs are removed\n-\t\t\t * when their packs are deleted.\n-\t\t\t *\n-\t\t\t * Generate an additional cruft pack, with one twist:\n-\t\t\t * `names` now includes the name of the cruft pack\n-\t\t\t * written in the previous step. So the contents of\n-\t\t\t * _this_ cruft pack exclude everything contained in the\n-\t\t\t * existing cruft pack (that is, all of the unreachable\n-\t\t\t * objects which are no older than\n-\t\t\t * `--cruft-expiration`).\n-\t\t\t *\n-\t\t\t * To make this work, cruft_expiration must become NULL\n-\t\t\t * so that this cruft pack doesn't actually prune any\n-\t\t\t * objects. If it were non-NULL, this call would always\n-\t\t\t * generate an empty pack (since every object not in the\n-\t\t\t * cruft pack generated above will have an mtime older\n-\t\t\t * than the expiration).\n-\t\t\t */\n-\t\t\tret = write_cruft_pack(&cruft_po_args, expire_to,\n-\t\t\t\t\t pack_prefix,\n-\t\t\t\t\t NULL,\n-\t\t\t\t\t &names,\n-\t\t\t\t\t &existing);\n+\t\tif (!names.nr && !po_args.quiet)\n+\t\t\tprintf_ln(_(\"Nothing new to pack.\"));\n+\t\t\t\n+\t\tif (pack_everything & PACK_CRUFT) {\n+\t\t\tconst char *pack_prefix = find_pack_prefix(packdir, packtmp);\n+\n+\t\t\tif (!cruft_po_args.window)\n+\t\t\t\tcruft_po_args.window = po_args.window;\n+\t\t\tif (!cruft_po_args.window_memory)\n+\t\t\t\tcruft_po_args.window_memory = po_args.window_memory;\n+\t\t\tif (!cruft_po_args.depth)\n+\t\t\t\tcruft_po_args.depth = po_args.depth;\n+\t\t\tif (!cruft_po_args.threads)\n+\t\t\t\tcruft_po_args.threads = po_args.threads;\n+\t\t\tif (!cruft_po_args.max_pack_size)\n+\t\t\t\tcruft_po_args.max_pack_size = po_args.max_pack_size;\n+\n+\t\t\tcruft_po_args.local = po_args.local;\n+\t\t\tcruft_po_args.quiet = po_args.quiet;\n+\n+\t\t\tret = write_cruft_pack(&cruft_po_args, packtmp, pack_prefix,\n+\t\t\t\t\tcruft_expiration, &names,\n+\t\t\t\t\t&existing);\n \t\t\tif (ret)\n \t\t\t\tgoto cleanup;\n+\n+\t\t\tif (delete_redundant && expire_to) {\n+\t\t\t\t/*\n+\t\t\t\t* If `--expire-to` is given with `-d`, it's possible\n+\t\t\t\t* that we're about to prune some objects. With cruft\n+\t\t\t\t* packs, pruning is implicit: any objects from existing\n+\t\t\t\t* packs that weren't picked up by new packs are removed\n+\t\t\t\t* when their packs are deleted.\n+\t\t\t\t*\n+\t\t\t\t* Generate an additional cruft pack, with one twist:\n+\t\t\t\t* `names` now includes the name of the cruft pack\n+\t\t\t\t* written in the previous step. So the contents of\n+\t\t\t\t* _this_ cruft pack exclude everything contained in the\n+\t\t\t\t* existing cruft pack (that is, all of the unreachable\n+\t\t\t\t* objects which are no older than\n+\t\t\t\t* `--cruft-expiration`).\n+\t\t\t\t*\n+\t\t\t\t* To make this work, cruft_expiration must become NULL\n+\t\t\t\t* so that this cruft pack doesn't actually prune any\n+\t\t\t\t* objects. If it were non-NULL, this call would always\n+\t\t\t\t* generate an empty pack (since every object not in the\n+\t\t\t\t* cruft pack generated above will have an mtime older\n+\t\t\t\t* than the expiration).\n+\t\t\t\t*/\n+\t\t\t\tret = write_cruft_pack(&cruft_po_args, expire_to,\n+\t\t\t\t\t\tpack_prefix,\n+\t\t\t\t\t\tNULL,\n+\t\t\t\t\t\t&names,\n+\t\t\t\t\t\t&existing);\n+\t\t\t\tif (ret)\n+\t\t\t\t\tgoto cleanup;\n+\t\t\t}\n \t\t}\n \t}\n \n-- \n2.46.0\n\n"},{"id":"504400","messageId":"20241008081350.8950-3-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20241008081350.8950-1-hanyang.tony@bytedance.com","subject":"[PATCH v2 2/3] t0410: adapt tests to repack changes","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-10-08T08:13:49Z","receivedAt":"2024-10-08T08:14:08Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"In the previous commit, we changed how partial repo is cloned.\nAdapt tests to these changes. Also check gc does not delete normal\nobjects too.\n\nSigned-off-by: Han Young \n---\n t/t0410-partial-clone.sh | 68 +---------------------------------------\n t/t5616-partial-clone.sh | 9 +-----\n 2 files changed, 2 insertions(+), 75 deletions(-)\n\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 34bdb3ab1f..c169b47160 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -499,46 +499,6 @@ test_expect_success 'single promisor remote can be re-initialized gracefully' '\n \tgit -C repo fetch --filter=blob:none foo\n '\n \n-test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '\n-\trm -rf repo &&\n-\ttest_create_repo repo &&\n-\ttest_commit -C repo one &&\n-\ttest_commit -C repo two &&\n-\n-\tTREE_ONE=$(git -C repo rev-parse one^{tree}) &&\n-\tprintf \"$TREE_ONE\\n\" | pack_as_from_promisor &&\n-\tTREE_TWO=$(git -C repo rev-parse two^{tree}) &&\n-\tprintf \"$TREE_TWO\\n\" | pack_as_from_promisor &&\n-\n-\tgit -C repo config core.repositoryformatversion 1 &&\n-\tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n-\tgit -C repo gc &&\n-\n-\t# Ensure that exactly one promisor packfile exists, and that it\n-\t# contains the trees but not the commits\n-\tls repo/.git/objects/pack/pack-*.promisor >promisorlist &&\n-\ttest_line_count = 1 promisorlist &&\n-\tPROMISOR_PACKFILE=$(sed \"s/.promisor/.pack/\" out &&\n-\tgrep \"$TREE_ONE\" out &&\n-\tgrep \"$TREE_TWO\" out &&\n-\t! grep \"$(git -C repo rev-parse one)\" out &&\n-\t! grep \"$(git -C repo rev-parse two)\" out &&\n-\n-\t# Remove the promisor packfile and associated files\n-\trm $(sed \"s/.promisor//\" packlist &&\n-\ttest_line_count = 1 packlist &&\n-\tgit verify-pack repo/.git/objects/pack/pack-*.pack -v >out &&\n-\tgrep \"$(git -C repo rev-parse one)\" out &&\n-\tgrep \"$(git -C repo rev-parse two)\" out &&\n-\t! grep \"$TREE_ONE\" out &&\n-\t! grep \"$TREE_TWO\" out\n-'\n-\n test_expect_success 'gc does not repack promisor objects if there are none' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n@@ -569,7 +529,7 @@ repack_and_check () {\n \tgit -C repo2 cat-file -e $3\n }\n \n-test_expect_success 'repack -d does not irreversibly delete promisor objects' '\n+test_expect_success 'repack -d does not irreversibly delete objects' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n \tgit -C repo config core.repositoryformatversion 1 &&\n@@ -583,40 +543,14 @@ test_expect_success 'repack -d does not irreversibly delete promisor objects' '\n \tTWO=$(git -C repo rev-parse HEAD^^) &&\n \tTHREE=$(git -C repo rev-parse HEAD^) &&\n \n-\tprintf \"$TWO\\n\" | pack_as_from_promisor &&\n \tprintf \"$THREE\\n\" | pack_as_from_promisor &&\n \tdelete_object repo \"$ONE\" &&\n \n-\trepack_and_check --must-fail -ab \"$TWO\" \"$THREE\" &&\n \trepack_and_check -a \"$TWO\" \"$THREE\" &&\n \trepack_and_check -A \"$TWO\" \"$THREE\" &&\n \trepack_and_check -l \"$TWO\" \"$THREE\"\n '\n \n-test_expect_success 'gc stops traversal when a missing but promised object is reached' '\n-\trm -rf repo &&\n-\ttest_create_repo repo &&\n-\ttest_commit -C repo my_commit &&\n-\n-\tTREE_HASH=$(git -C repo rev-parse HEAD^{tree}) &&\n-\tHASH=$(promise_and_delete $TREE_HASH) &&\n-\n-\tgit -C repo config core.repositoryformatversion 1 &&\n-\tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n-\tgit -C repo gc &&\n-\n-\t# Ensure that the promisor packfile still exists, and remove it\n-\ttest -e repo/.git/objects/pack/pack-$HASH.pack &&\n-\trm repo/.git/objects/pack/pack-$HASH.* &&\n-\n-\t# Ensure that the single other pack contains the commit, but not the tree\n-\tls repo/.git/objects/pack/pack-*.pack >packlist &&\n-\ttest_line_count = 1 packlist &&\n-\tgit verify-pack repo/.git/objects/pack/pack-*.pack -v >out &&\n-\tgrep \"$(git -C repo rev-parse HEAD)\" out &&\n-\t! grep \"$TREE_HASH\" out\n-'\n-\n test_expect_success 'do not fetch when checking existence of tree we construct ourselves' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex c53e93be2f..2c6f10026f 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -643,16 +643,9 @@ test_expect_success 'fetch from a partial clone, protocol v2' '\n \tgrep \"version 2\" trace\n '\n \n-test_expect_success 'repack does not loosen promisor objects' '\n-\trm -rf client trace &&\n-\tgit clone --bare --filter=blob:none \"file://$(pwd)/srv.bare\" client &&\n-\ttest_when_finished \"rm -rf client trace\" &&\n-\tGIT_TRACE2_PERF=\"$(pwd)/trace\" git -C client repack -A -d &&\n-\tgrep \"loosen_unused_packed_objects/loosened:0\" trace\n-'\n-\n test_expect_success 'lazy-fetch in submodule succeeds' '\n \t# setup\n+\trm -rf client &&\n \ttest_config_global protocol.file.allow always &&\n \n \ttest_when_finished \"rm -rf src-sub\" &&\n-- \n2.46.0\n\n"},{"id":"504401","messageId":"20241008081350.8950-4-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20241008081350.8950-1-hanyang.tony@bytedance.com","subject":"[PATCH v2 3/3] partial-clone: update doc","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-10-08T08:13:50Z","receivedAt":"2024-10-08T08:14:12Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"Document new repack behavior for partial repo\n\nSigned-off-by: Han Young \n---\n Documentation/technical/partial-clone.txt | 16 ++++++++++++----\n 1 file changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/technical/partial-clone.txt b/Documentation/technical/partial-clone.txt\nindex cd948b0072..9791c9ac24 100644\n--- a/Documentation/technical/partial-clone.txt\n+++ b/Documentation/technical/partial-clone.txt\n@@ -124,6 +124,10 @@ their \".pack\" and \".idx\" files.\n +\n When Git encounters a missing object, Git can see if it is a promisor object\n and handle it appropriately. If not, Git can report a corruption.\n+\n+To prevent `repack` from removing locally created objects, `repack` packs all\n+the objects into one promisor packfile. It's no longer possible to determine\n+the cause of missing objects after `gc`.[7]\n +\n This means that there is no need for the client to explicitly maintain an\n expensive-to-modify list of missing objects.[a]\n@@ -156,8 +160,9 @@ and prefetch those objects in bulk.\n \n - `fsck` has been updated to be fully aware of promisor objects.\n \n-- `repack` in GC has been updated to not touch promisor packfiles at all,\n- and to only repack other objects.\n+- `repack` in GC has been taught to handle partial clone repo differently.\n+ `repack` will pack every objects into one promisor packfile for partial\n+ repos.\n \n - The global variable \"fetch_if_missing\" is used to control whether an\n object lookup will attempt to dynamically fetch a missing object or\n@@ -244,8 +249,7 @@ remote in a specific order.\n objects. We assume that promisor remotes have a complete view of the\n repository and can satisfy all such requests.\n \n-- Repack essentially treats promisor and non-promisor packfiles as 2\n- distinct partitions and does not mix them.\n+- It's not possible to discard dangling objects in repack.\n \n - Dynamic object fetching invokes fetch-pack once *for each item*\n because most algorithms stumble upon a missing object and need to have\n@@ -365,3 +369,7 @@ Related Links\n [6] https://lore.kernel.org/git/20170714132651.170708-1-benpeart@microsoft.com/ +\n Subject: [RFC/PATCH v2 0/1] Add support for downloading blobs on demand +\n Date: Fri, 14 Jul 2017 09:26:50 -0400\n+\n+[7] https://lore.kernel.org/git/20240802073143.56731-1-hanyang.tony@bytedance.com/ +\n+ Subject: [PATCH 0/1] revision: fix reachable objects being gc'ed in no blob clone repo +\n+ Date: Fri, 2 Aug 2024 15:31:42 +0800\n-- \n2.46.0\n\n"},{"id":"504491","messageId":"CAFySSZCyoaKCGycYgJjCJGJ2mV1yfg+gVFb7RytGKmkjupkNkQ@mail.gmail.com","threadId":"61890","inReplyTo":"xmqqo743qkn9.fsf@gitster.g","subject":"Re: Missing Promisor Objects in Partial Repo Design Doc","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-10-08T21:35:56Z","receivedAt":"2024-10-08T21:36:09Z","isPatch":false,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"On Tue, Oct 1, 2024 at 7:54 PM Junio C Hamano wrote:\n>\n> True. Will it become even worse, if a protocol extension Christian\n> proposes starts suggesting a repository that is not lazy to add a\n> promisor remote? In such a set-up, perhaps all history leading to\n> C2b down to the root are local, but C3 may have come from a promisor\n> remote (hence in a promisor pack).\n\nYes if we and consequently Git considers this state to be problematic.\n\n> > Bad State Solutions\n> > ===================\n> >\n> > Fetch negotiation\n> > -----------------\n> > Implemented at\n> > https://lore.kernel.org/git/20240919234741.1317946-1-calvinwan@google.com/\n> >\n> > During fetch negotiation, if a commit is not in a promisor pack and\n> > therefore local, do not declare it as \"have\" so they can be fetched into\n> > a promisor pack.\n> >\n> > Cost:\n> > - Creation of set of promisor pack objects (by iterating through every\n> > .idx of promisor packs)\n>\n> What is \"promisor PACK objects\"? Is it different from the \"promisor\n> objects\" (i.e. what I called the useless definition above)?\n\nObjects that are in promisor packs, specifically the ones that have the\nflag, packed_git::pack_promisor, set. However, since this design doc\nwas sent out, it turns out the creation of a set of promisor pack objects\nin a large repository (such as Android or Chrome) is very expensive, so\nthis design is infeasible in my opinion.\n\n>\n> > - Refetch number of local commits\n> >\n> > Pros: Implementation is simple, client doesn’t have to repack, prevents\n> > state from ever occurring in the repository.\n> >\n> > Cons: Network cost of refetching could be high if many local commits\n> > need to be refetched.\n>\n> What if we get into the same state by creating local C4, which gets\n> to outside and on top of which C5 is built, which is now sitting at\n> the tip of the remote history and we fetch from them? In order to\n> include C4 in the \"promisor pack\", we refrain from saying C4 is a\n> \"have\" for us and refetch. Would C2 be fetched again?\n>\n> I do not think C2 would be, because we made it an object in a\n> promisor pack when we \"fixed\" the history for C3.\n>\n> So the cost will not grow proportionally to the depth of the\n> history, which makes it OK from my point of view.\n\nCorrect, the cost of refetching is only a one time cost, but\nunfortunately creation of a set of promisor pack objects isn't.\n\n>\n> > Garbage Collection repack\n> > -------------------------\n> > Not yet implemented.\n> >\n> > Same concept at “fetch repack”, but happens during garbage collection\n> > instead. The traversal is more expensive since we no longer have access\n> > to what was recently fetched so we have to traverse through all promisor\n> > packs to collect tips of “bad” history.\n>\n> In other words, with the status quo, \"git gc\" that attempts to\n> repack \"objects in promisor packs\" and \"other objects that did not\n> get repacked in the step that repack objects in promisor packs\"\n> separately, it implements the latter in a buggy way and discards\n> some objects. And fixing that bug by doing the right thing is\n> expensive.\n>\n> Stepping back a bit, why is the loss of C2a/C2b/C2 a problem after\n> \"git gc\"? Wouldn't these \"missing\" objects be lazily fetchable, now\n> C3 is known to the remote and the remote promises everything\n> reachable from what they offer are (re)fetchable from them? IOW, is\n> this a correctness issue, or only performance issue (of having to\n> re-fetch what we once locally had)?\n\nMy first thought is that from both the user and developer perspective,\nwe don't expect our reachable objects to be gc'ed. So all of the \"bad\nstate\" solutions work to ensure that that isn't the case in some way or\nform. However, if it turns out that all of these solutions are much more\nexpensive and disruptive to the user than accepting that local objects\ncan be gc'ed and JIT refetching, then the latter seems much more\npalatable. It is inevitable that we take some performance hit to fix this\nproblem and we may just have to accept this as one of the costs of\nhaving partial clones to begin with.\n\n>\n> > Cons: Packing local objects into promisor packs means that it is no\n> > longer possible to detect if an object is missing due to repository\n> > corruption or because we need to fetch it from a promisor remote.\n>\n> Is this true? Can we tell, when trying to access C2a/C2b/C2 after\n> the current version of \"git gc\" removes them from the local object\n> store, that they are missing due to repository corruption? After\n> all, C3 can reach them so wouldn't it be possible for us to fetch\n> them from the promisor remote?\n\nI should be more clear that \"detecting if an object is missing due to\nrepository corruption\" refers to fsck currently not having the\nfunctionality to do that. We are \"accidentally\" discovering the\ncorruption when we try to access the missing object, but we can\nstill fetch them from the promisor remote afterwards.\n\n> After a lazy clone that omits a lot of objects acquires many objects\n> over time by fetching missing objects on demand, wouldn't we want to\n> have an option to \"slim\" the local repository by discarding some of\n> these objects (the ones that are least frequently used), relying on\n> the promise by the promisor remote that even if we did so, they can\n> be fetched again? Can we treat loss of C2a/C2b/C2 as if such a\n> feature prematurely kicked in? Or are we failing to refetch them\n> for some reason?\n\nYes if such a feature existed, then it would be feasible and a possible\nsolution for this issue (I'm leaning quite towards this now after testing\nout some of the other designs).\n"},{"id":"504492","messageId":"CAFySSZDjbWdYKRSdSkpd8XzxwOskZ-8tp0tZWxrYBgC6eCED4Q@mail.gmail.com","threadId":"61890","inReplyTo":"20241008081350.8950-2-hanyang.tony@bytedance.com","subject":"Re: [PATCH v2 1/3] repack: pack everything into packfile","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-10-08T21:41:04Z","receivedAt":"2024-10-08T21:41:17Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"On Tue, Oct 8, 2024 at 1:14 AM Han Young wrote:\n>\n> In a partial repository, creating a local commit and then fetching\n> causes the following state to occur:\n>\n> commit tree blob\n> C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n> |\n> C2 ---- T2 -- B2 (created locally, in non-promisor pack)\n> |\n> C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n>\n> During garbage collection, parents of promisor objects are marked as\n> UNINTERESTING and are subsequently garbage collected. In this case, C2\n> would be deleted and attempts to access that commit would result in \"bad\n> object\" errors (originally reported here[1]).\n>\n> For partial repos, repacking is done in two steps. We first repack all the\n> objects in promisor packfile, then repack all the non-promisor objects.\n> It turns out C2, T2 and B2 are not repacked in either steps, ended up deleted.\n> We can avoid this by packing everything into a promisor packfile, if the repo\n> is partial cloned.\n>\n> [1] https://lore.kernel.org/git/20240802073143.56731-1-hanyang.tony@bytedance.com/\n>\n> Helped-by: Calvin Wan \n> Signed-off-by: Han Young \n> ---\n> builtin/repack.c | 257 ++++++++++++++++++++++++++---------------------\n> 1 file changed, 143 insertions(+), 114 deletions(-)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index cb4420f085..e9e18a31fe 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -321,6 +321,23 @@ static int write_oid(const struct object_id *oid,\n> return 0;\n> }\n>\n> +static int write_loose_oid(const struct object_id *oid,\n> + const char *path UNUSED,\n> + void *data)\n> +{\n> + struct child_process *cmd = data;\n> +\n> + if (cmd->in == -1) {\n> + if (start_command(cmd))\n> + die(_(\"could not start pack-objects to repack promisor objects\"));\n> + }\n> +\n> + if (write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n> + write_in_full(cmd->in, \"\\n\", 1) < 0)\n> + die(_(\"failed to feed promisor objects to pack-objects\"));\n> + return 0;\n> +}\n> +\n> static struct {\n> const char *name;\n> unsigned optional:1;\n> @@ -370,12 +387,15 @@ static int has_pack_ext(const struct generated_pack_data *data,\n> BUG(\"unknown pack extension: '%s'\", ext);\n> }\n>\n> -static void repack_promisor_objects(const struct pack_objects_args *args,\n> - struct string_list *names)\n> +static int repack_promisor_objects(const struct pack_objects_args *args,\n> + struct string_list *names,\n> + struct string_list *list,\n> + int pack_all)\n> {\n> struct child_process cmd = CHILD_PROCESS_INIT;\n> FILE *out;\n> struct strbuf line = STRBUF_INIT;\n> + struct string_list_item *item;\n>\n> prepare_pack_objects(&cmd, args, packtmp);\n> cmd.in = -1;\n> @@ -387,13 +407,19 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n> * {type -> existing pack order} ordering when computing deltas instead\n> * of a {type -> size} ordering, which may produce better deltas.\n> */\n> - for_each_packed_object(write_oid, &cmd,\n> - FOR_EACH_OBJECT_PROMISOR_ONLY);\n> + if (pack_all)\n> + for_each_packed_object(write_oid, &cmd, 0);\n> + else\n> + for_each_string_list_item(item, list) {\n> + pack_mark_retained(item);\n> + }\n> +\n> + for_each_loose_object(write_loose_oid, &cmd, 0);\n>\n> if (cmd.in == -1) {\n> /* No packed objects; cmd was never started */\n> child_process_clear(&cmd);\n> - return;\n> + return 0;\n> }\n>\n> close(cmd.in);\n> @@ -431,6 +457,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n> if (finish_command(&cmd))\n> die(_(\"could not finish pack-objects to repack promisor objects\"));\n> strbuf_release(&line);\n> + return 0;\n> }\n>\n> struct pack_geometry {\n> @@ -1312,8 +1339,7 @@ int cmd_repack(int argc,\n> strvec_push(&cmd.args, \"--reflog\");\n> strvec_push(&cmd.args, \"--indexed-objects\");\n> }\n> - if (repo_has_promisor_remote(the_repository))\n> - strvec_push(&cmd.args, \"--exclude-promisor-objects\");\n> +\n> if (!write_midx) {\n> if (write_bitmaps > 0)\n> strvec_push(&cmd.args, \"--write-bitmap-index\");\n> @@ -1323,125 +1349,128 @@ int cmd_repack(int argc,\n> if (use_delta_islands)\n> strvec_push(&cmd.args, \"--delta-islands\");\n>\n> - if (pack_everything & ALL_INTO_ONE) {\n> - repack_promisor_objects(&po_args, &names);\n> -\n> - if (has_existing_non_kept_packs(&existing) &&\n> - delete_redundant &&\n> - !(pack_everything & PACK_CRUFT)) {\n> - for_each_string_list_item(item, &names) {\n> - strvec_pushf(&cmd.args, \"--keep-pack=%s-%s.pack\",\n> - packtmp_name, item->string);\n> - }\n> - if (unpack_unreachable) {\n> - strvec_pushf(&cmd.args,\n> - \"--unpack-unreachable=%s\",\n> - unpack_unreachable);\n> - } else if (pack_everything & LOOSEN_UNREACHABLE) {\n> - strvec_push(&cmd.args,\n> - \"--unpack-unreachable\");\n> - } else if (keep_unreachable) {\n> - strvec_push(&cmd.args, \"--keep-unreachable\");\n> - strvec_push(&cmd.args, \"--pack-loose-unreachable\");\n> + if (repo_has_promisor_remote(the_repository)) {\n> + ret = repack_promisor_objects(&po_args, &names,\n> + &existing.non_kept_packs, pack_everything & ALL_INTO_ONE);\n\nUsing a goto jump would be easier to both read your patch and\nremove the need to indent this entire code block.\n"},{"id":"504494","messageId":"xmqq4j5mz295.fsf@gitster.g","threadId":"61890","inReplyTo":"20241008081350.8950-1-hanyang.tony@bytedance.com","subject":"Re: [PATCH v2 0/3] repack: pack everything into promisor packfile in partial repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-08T21:57:42Z","receivedAt":"2024-10-08T21:57:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Young writes:\n\n> As suggested by Jonathan[1], there are number of ways to fix this issue.\n> We have already explored some of them in this thread, and so far none of them\n> is satisfiable. Calvin and I tried to address the problem from fetch-pack side\n> and rev-list side. But the fix either consumes too much CPU power or results\n> in inefficient bandwidth use.\n>\n> So let's attack the problem from repack side. The goal is to prevent repack\n> from discarding local objects, previously it is done by carefully\n> separating promisor objects and normal objects in rev-list.\n> The implementation is flawed and no solution have been found so far.\n> Instead, we can get ride of rev-list and just pack everything into promisor\n> files. This way, no objects would be lost.\n>\n> By using 'repack everything', repacking requires less work and we are not\n> using more bandwidth.\n\nOK, perhaps.\n\n> Packing local objects into promisor packfiles means that it is no longer\n> possible to detect if an object is missing due to repository corruption\n> or because we need to fetch it from a promisor remote.\n\nIs it true that without doing this, we can tell between these two\ncases, though? More importantly, even if it is true, would there be\na practical difference?\n\nIn the sample scenario used in [1/3] where you created C2/T2/B2 on\ntop of C1/T1/B1 (which came from a promisor remote), somebody else\nbuilt C3/T3/B3 on top, and it came back from the promisor remote,\nyou could lose 3's objects and 1's objects and they can be refetched\nbut even if you lose 2's objects, since 3's objects are building on\ntop of them, you should be able to fetch them from the promisor\nremote just like objects from 1 and 3, no? So strictly speaking,\nmissing 2's objects may be \"repository corruption\" while missing 1's\nand 3's objects may not be, would there be a practical use for that\ninformation?\n\n> Promisor objects packing does not benefiting from the history and\n> path based delta calculation, and GC does not remove unreachable promisor\n> objects. By packing locally created normal objects into promisor packfile,\n> normal objects are converted into promisor objects. However, in partial cloned\n> repos, the number of locally created objects are small compared to promisor\n> objects. The impact should be negligible.\n\n> [1] https://lore.kernel.org/git/20240813004508.2768102-1-jonathantanmy@google.com/\n>\n> *** Changes since v1 ***\n> Added tradeoffs in cover letter.\n> Fixed some partial clone test cases.\n> Updated partial clone documentation.\n\nThese patches are based on the tip of master before 365529e1 (Merge\nbranch 'ps/leakfixes-part-7', 2024-10-02), which will give mildly\nannoying conflicts when merged to 'seen'.\n\nI've managed to apply and then merge, so unless review discussions\nfind needs for updates, there is no need for immediate reroll, but\nif you end up having to update these patches, it is a good idea to\nrebase the topic on top of v2.47.0 that was released early this\nweek, as we are now entering a new development cycle.\n\nThanks.\n\n\n>\n> Han Young (3):\n> repack: pack everything into packfile\n> t0410: adapt tests to repack changes\n> partial-clone: update doc\n>\n> Documentation/technical/partial-clone.txt | 16 +-\n> builtin/repack.c | 257 ++++++++++++----------\n> t/t0410-partial-clone.sh | 68 +-----\n> t/t5616-partial-clone.sh | 9 +-\n> 4 files changed, 157 insertions(+), 193 deletions(-)\n"},{"id":"504496","messageId":"xmqqzfnexlku.fsf@gitster.g","threadId":"61890","inReplyTo":"xmqq4j5mz295.fsf@gitster.g","subject":"Re: [PATCH v2 0/3] repack: pack everything into promisor packfile in partial repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-08T22:43:13Z","receivedAt":"2024-10-08T22:43:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I've managed to apply and then merge, so unless review discussions\n> find needs for updates, there is no need for immediate reroll, but\n> if you end up having to update these patches, it is a good idea to\n> rebase the topic on top of v2.47.0 that was released early this\n> week, as we are now entering a new development cycle.\n\nWhen merged to the tip of 'seen', it seems to break t5710. It might\nbe due to mismerge, but can you check on your end?\n\nThanks.\n"},{"id":"504519","messageId":"CAG1j3zFOMz-C=6xq_+mN2PrfyVcDrrTpMEHpLrrP_crS9J+rUg@mail.gmail.com","threadId":"61890","inReplyTo":"xmqq4j5mz295.fsf@gitster.g","subject":"Re: [External] Re: [PATCH v2 0/3] repack: pack everything into promisor packfile in partial repos","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-10-09T06:31:55Z","receivedAt":"2024-10-09T06:32:07Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"On Wed, Oct 9, 2024 at 5:57 AM Junio C Hamano wrote:\n\n> > Packing local objects into promisor packfiles means that it is no longer\n> > possible to detect if an object is missing due to repository corruption\n> > or because we need to fetch it from a promisor remote.\n>\n> Is it true that without doing this, we can tell between these two\n> cases, though? More importantly, even if it is true, would there be\n> a practical difference?\n>\n> In the sample scenario used in [1/3] where you created C2/T2/B2 on\n> top of C1/T1/B1 (which came from a promisor remote), somebody else\n> built C3/T3/B3 on top, and it came back from the promisor remote,\n> you could lose 3's objects and 1's objects and they can be refetched\n> but even if you lose 2's objects, since 3's objects are building on\n> top of them, you should be able to fetch them from the promisor\n> remote just like objects from 1 and 3, no? So strictly speaking,\n> missing 2's objects may be \"repository corruption\" while missing 1's\n> and 3's objects may not be, would there be a practical use for that\n> information?\n\n Some code path does check if the missing object is promisor object before\n lazy fetching, `--missing=` does this check.\nBut in that case, C2 is also a promisor object, the check would pass.\nThere are no partial clone filter that omits commits, missing commit will\nalways result in error. And even if we do report \"repository corruption\",\nthe best course of action is still try to fetching them.\nSo, no. I don't think there are practical uses for that information\n\n\n> These patches are based on the tip of master before 365529e1 (Merge\n> branch 'ps/leakfixes-part-7', 2024-10-02), which will give mildly\n> annoying conflicts when merged to 'seen'.\n>\n> I've managed to apply and then merge, so unless review discussions\n> find needs for updates, there is no need for immediate reroll, but\n> if you end up having to update these patches, it is a good idea to\n> rebase the topic on top of v2.47.0 that was released early this\n> week, as we are now entering a new development cycle.\n\nThanks, I will rebase to master and see if any other tests break.\n"},{"id":"504520","messageId":"CAG1j3zGcdfd6YRb=Fb1Aqt5kLajueagV+6upt6vwsGW9RxaR7Q@mail.gmail.com","threadId":"61890","inReplyTo":"CAFySSZCyoaKCGycYgJjCJGJ2mV1yfg+gVFb7RytGKmkjupkNkQ@mail.gmail.com","subject":"Re: [External] Re: Missing Promisor Objects in Partial Repo Design Doc","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-10-09T06:46:57Z","receivedAt":"2024-10-09T06:47:09Z","isPatch":false,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"On Wed, Oct 9, 2024 at 5:36 AM Calvin Wan wrote:\n\n> Objects that are in promisor packs, specifically the ones that have the\n> flag, packed_git::pack_promisor, set. However, since this design doc\n> was sent out, it turns out the creation of a set of promisor pack objects\n> in a large repository (such as Android or Chrome) is very expensive, so\n> this design is infeasible in my opinion.\n\nI wonder if a set of local loose/pack objects will be cheaper to construct?\nNormally loose objects are always non-promisor objects, unless the user\nrunning something like `unpack-objects`.\n\n> > After a lazy clone that omits a lot of objects acquires many objects\n> > over time by fetching missing objects on demand, wouldn't we want to\n> > have an option to \"slim\" the local repository by discarding some of\n> > these objects (the ones that are least frequently used), relying on\n> > the promise by the promisor remote that even if we did so, they can\n> > be fetched again? Can we treat loss of C2a/C2b/C2 as if such a\n> > feature prematurely kicked in? Or are we failing to refetch them\n> > for some reason?\n>\n> Yes if such a feature existed, then it would be feasible and a possible\n> solution for this issue (I'm leaning quite towards this now after testing\n> out some of the other designs).\n\nSince no partial clone filter omits commit objects, we always assume\ncommits are available in the codebase. `merge` reports \"cannot merge\nunrelated history\" if one of the commits is missing, instead of trying to\nfetch it.\nAnother problem is current lazy fetching code does not report \"haves\"\nto remote, so a lazy fetching of commit ended up pulling all the trees,\nblobs associated with that commit.\nI also prefer the \"fetching the missing objects\" approach, making sure\nthe repo has all the \"correct\" objects is difficult to get right.\n"},{"id":"504625","messageId":"20241009183455.164222-1-jonathantanmy@google.com","threadId":"61890","inReplyTo":"CAG1j3zGcdfd6YRb=Fb1Aqt5kLajueagV+6upt6vwsGW9RxaR7Q@mail.gmail.com","subject":"Re: [External] Re: Missing Promisor Objects in Partial Repo Design Doc","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-09T18:34:53Z","receivedAt":"2024-10-09T18:34:58Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Han Young writes:\n> On Wed, Oct 9, 2024 at 5:36 AM Calvin Wan wrote:\n> \n> > Objects that are in promisor packs, specifically the ones that have the\n> > flag, packed_git::pack_promisor, set. However, since this design doc\n> > was sent out, it turns out the creation of a set of promisor pack objects\n> > in a large repository (such as Android or Chrome) is very expensive, so\n> > this design is infeasible in my opinion.\n> \n> I wonder if a set of local loose/pack objects will be cheaper to construct?\n> Normally loose objects are always non-promisor objects, unless the user\n> running something like `unpack-objects`.\n\nWe had a similar idea at $JOB. Note that you don't actually\nneed to create the set - when looking up an object using\noid_object_info_extended(), we know if it's a loose object and if not,\nwhich pack it is in. The pack has a promisor bit that we can check.\n\nNote that there is a possibility of a false positive. If the same object\nis in two packs - one promisor and one non-promisor - I believe there's\nno guarantee that one pack will be preferred. So we can see that the\nobject is in a non-promisor pack, but there's no guarantee that it's not\nalso in a promisor pack. For the omit-local-commits-in-\"have\" solution,\nthis is a fatal flaw (we absolutely must guarantee that we don't send\nany promisor commits) but for the repack-on-fetch solution, this is no\nbig deal (we are looking for objects to repack into a promisor pack, and\nrepacking a promisor object into a promisor pack is perfectly file). For\nthis reason, I think the repack-on-fetch solution is the most promising\none so far.\n\nLoose objects are always non-promisor objects, yes. (I don't think the\nuser running `unpack-objects` counts - the user running a command on a\npackfile in the .git directory is out of scope, I think.)\n\n> > > After a lazy clone that omits a lot of objects acquires many objects\n> > > over time by fetching missing objects on demand, wouldn't we want to\n> > > have an option to \"slim\" the local repository by discarding some of\n> > > these objects (the ones that are least frequently used), relying on\n> > > the promise by the promisor remote that even if we did so, they can\n> > > be fetched again? Can we treat loss of C2a/C2b/C2 as if such a\n> > > feature prematurely kicked in? Or are we failing to refetch them\n> > > for some reason?\n> >\n> > Yes if such a feature existed, then it would be feasible and a possible\n> > solution for this issue (I'm leaning quite towards this now after testing\n> > out some of the other designs).\n> \n> Since no partial clone filter omits commit objects, we always assume\n> commits are available in the codebase. `merge` reports \"cannot merge\n> unrelated history\" if one of the commits is missing, instead of trying to\n> fetch it.\n> Another problem is current lazy fetching code does not report \"haves\"\n> to remote, so a lazy fetching of commit ended up pulling all the trees,\n> blobs associated with that commit.\n> I also prefer the \"fetching the missing objects\" approach, making sure\n> the repo has all the \"correct\" objects is difficult to get right.\n\nIf I remember correctly, our intention (or, at least, my intention)\nof not treating missing commits differently was so that we don't limit\nthe solutions that we can implement. For example, we had the idea of\nserver-assisted merge base computation - this and other features would\nmake it feasible to omit commits locally. It has not been implemented,\nthough.\n"},{"id":"504630","messageId":"20241009185312.200629-1-jonathantanmy@google.com","threadId":"61890","inReplyTo":"xmqqo743qkn9.fsf@gitster.g","subject":"Re: Missing Promisor Objects in Partial Repo Design Doc","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-09T18:53:11Z","receivedAt":"2024-10-09T18:53:15Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano writes:\n> > (C2b is a bit of a special case. Despite not being in a promisor pack,\n> > it is still considered to be a promisor object since C3 directly\n> > references it.)\n> \n> Yes, and I suspect the root cause of this confusion is because\n> \"promisor object\", as defined today, is a flawed concept. If C2b\n> were pointed by a local ref, just like the case the ref points at\n> C2a, they should be treated the same way, as both of them are\n> locally created. To put it another way, presumably the local have\n> already been pushed out to elsewhere and the promisor remote got\n> hold of them, and that is why C3 can build on top of them. And the\n> fact C2b is directly reachable from C3 and C2a is not should not\n> have any relevance if C2a or C2b are not _included_ in promisor\n> packs (hence both of them need to be included in the local pack).\n> \n> Two concepts that would have been useful are (1) objects that are in\n> promisor packs and (2) objects that are reachable from an object\n> that is in a promisor pack. I do not see how the current definition\n> of \"promisor objects\" (i.e. in a promisor pack, or one hop from an\n> object in a promisor pack) is useful in any context.\n\nThe one-hop part in the current definition is meant to (a) explain what\nobjects the client knows the remote has (in theory the client has no\nknowledge of objects beyond the first hop, but we now know this theory\nto not be true) and (b) explain what objects a non-promisor object can\nreference (in particular, a non-promisor tree can reference promisor\nblobs, even when our knowledge of that promisor blob only comes from a\ntree in a promisor pack).\n\nIf we think that a promisor commit being a child of a non-promisor\ncommit as a \"bad state\" that needs to be fixed [1], then the one-hop\ncurrent definition seems to be equivalent to (2).\n\nAs for (1), we do use that concept in Git, although it's limited to the\nrepack during GC (or maybe there are others that I don't recall), so the\nconcept doesn't have a widely-used name like \"promisor object\".\n\n[1] https://lore.kernel.org/git/20241001191811.1934900-1-calvinwan@google.com/\n\n> > Garbage Collection repack\n> > -------------------------\n> > Not yet implemented.\n> >\n> > Same concept at “fetch repack”, but happens during garbage collection\n> > instead. The traversal is more expensive since we no longer have access\n> > to what was recently fetched so we have to traverse through all promisor\n> > packs to collect tips of “bad” history.\n> \n> In other words, with the status quo, \"git gc\" that attempts to\n> repack \"objects in promisor packs\" and \"other objects that did not\n> get repacked in the step that repack objects in promisor packs\"\n> separately, it implements the latter in a buggy way and discards\n> some objects. And fixing that bug by doing the right thing is\n> expensive.\n> \n> Stepping back a bit, why is the loss of C2a/C2b/C2 a problem after\n> \"git gc\"? Wouldn't these \"missing\" objects be lazily fetchable, now\n> C3 is known to the remote and the remote promises everything\n> reachable from what they offer are (re)fetchable from them? IOW, is\n> this a correctness issue, or only performance issue (of having to\n> re-fetch what we once locally had)?\n\nI believe the re-fetch didn't happen because it was run from a command\nwith fetch_if_missing=0. (But even if we decide that we shouldn't use\nfetch_if_missing, and then change all commands to not use it, there\nstill remains the performance issue, so we should still fix it.)\n\n> > Cons: Packing local objects into promisor packs means that it is no\n> > longer possible to detect if an object is missing due to repository\n> > corruption or because we need to fetch it from a promisor remote.\n> \n> Is this true? Can we tell, when trying to access C2a/C2b/C2 after\n> the current version of \"git gc\" removes them from the local object\n> store, that they are missing due to repository corruption? After\n> all, C3 can reach them so wouldn't it be possible for us to fetch\n> them from the promisor remote?\n> \n> After a lazy clone that omits a lot of objects acquires many objects\n> over time by fetching missing objects on demand, wouldn't we want to\n> have an option to \"slim\" the local repository by discarding some of\n> these objects (the ones that are least frequently used), relying on\n> the promise by the promisor remote that even if we did so, they can\n> be fetched again? Can we treat loss of C2a/C2b/C2 as if such a\n> feature prematurely kicked in? Or are we failing to refetch them\n> for some reason?\n\nThis is under the \"repack all\" option, which states that we repack all\nobjects (wherever they came from) into promisor packs. If we locally\ncreated commit A and then its child commit B, and the repo got corrupted\nso that we lost A, repacking all objects would mean that we could never\ndetect that the loss of A is problematic.\n\n"},{"id":"504821","messageId":"20241011082404.88939-1-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20240802073143.56731-1-hanyang.tony@bytedance.com","subject":"[PATCH v3 0/3] repack: pack everything into promisor packfile in partial repos","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-10-11T08:24:01Z","receivedAt":"2024-10-11T08:24:15Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"Changes since v2:\n- rebased to seen: 89afaf27d3 (Merge branch 'ak/typofixes' into seen, 2024-10-10)\n- fixed t5710, \"repack all\" affects how the test repo is initialized\n- use goto to skip normal repack\n\nThis series doesn't address the underlying problem with promisor objects,\nbut rather mitigates the \"repack removes local objects\" problem.\nUntil a satisfiable solution can be found[1], this would at least prevent\nmore promisor repos from becoming corrupted.\n\n[1] https://lore.kernel.org/git/20241001191811.1934900-1-calvinwan@google.com/\n\nHan Young (3):\n repack: pack everything into packfile\n repack: adapt tests to repack changes\n partial-clone: update doc\n\n Documentation/technical/partial-clone.txt | 16 ++++--\n builtin/repack.c | 46 ++++++++++++---\n t/t0410-partial-clone.sh | 68 +----------------------\n t/t5616-partial-clone.sh | 9 +--\n t/t5710-promisor-remote-capability.sh | 15 ++++-\n 5 files changed, 65 insertions(+), 89 deletions(-)\n\n-- \n2.47.0.266.g0b04b6b485.dirty\n\n"},{"id":"504822","messageId":"20241011082404.88939-2-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20241011082404.88939-1-hanyang.tony@bytedance.com","subject":"[PATCH v3 1/3] repack: pack everything into packfile","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-10-11T08:24:02Z","receivedAt":"2024-10-11T08:24:19Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"In a partial repository, creating a local commit and then fetching\ncauses the following state to occur:\n\ncommit tree blob\n C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n |\n C2 ---- T2 -- B2 (created locally, in non-promisor pack)\n |\n C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n\nDuring garbage collection, parents of promisor objects are marked as\nUNINTERESTING and are subsequently garbage collected. In this case, C2\nwould be deleted and attempts to access that commit would result in \"bad\nobject\" errors (originally reported here[1]).\n\nFor partial repos, repacking is done in two steps. We first repack all the\nobjects in promisor packfile, then repack all the non-promisor objects.\nIt turns out C2, T2 and B2 are not repacked in either steps, ended up deleted.\nWe can avoid this by packing everything into a promisor packfile, if the repo\nis partial cloned.\n\n[1] https://lore.kernel.org/git/20240802073143.56731-1-hanyang.tony@bytedance.com/\n\nHelped-by: Calvin Wan \nSigned-off-by: Han Young \n---\n builtin/repack.c | 46 +++++++++++++++++++++++++++++++++++++++-------\n 1 file changed, 39 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 79f407ca04..50e14ccfc4 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -343,6 +343,23 @@ static int write_oid(const struct object_id *oid,\n \treturn 0;\n }\n \n+static int write_loose_oid(const struct object_id *oid,\n+\t\t\t const char *path UNUSED,\n+\t\t\t void *data)\n+{\n+\tstruct child_process *cmd = data;\n+\n+\tif (cmd->in == -1) {\n+\t\tif (start_command(cmd))\n+\t\t\tdie(_(\"could not start pack-objects to repack promisor objects\"));\n+\t}\n+\n+\tif (write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n+\t write_in_full(cmd->in, \"\\n\", 1) < 0)\n+\t\tdie(_(\"failed to feed promisor objects to pack-objects\"));\n+\treturn 0;\n+}\n+\n static struct {\n \tconst char *name;\n \tunsigned optional:1;\n@@ -392,12 +409,15 @@ static int has_pack_ext(const struct generated_pack_data *data,\n \tBUG(\"unknown pack extension: '%s'\", ext);\n }\n \n-static void repack_promisor_objects(const struct pack_objects_args *args,\n-\t\t\t\t struct string_list *names)\n+static int repack_promisor_objects(const struct pack_objects_args *args,\n+\t\t\t\t struct string_list *names,\n+\t\t\t\t struct string_list *list,\n+\t\t\t\t int pack_all)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tFILE *out;\n \tstruct strbuf line = STRBUF_INIT;\n+\tstruct string_list_item *item;\n \n \tprepare_pack_objects(&cmd, args, packtmp);\n \tcmd.in = -1;\n@@ -409,13 +429,19 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \t * {type -> existing pack order} ordering when computing deltas instead\n \t * of a {type -> size} ordering, which may produce better deltas.\n \t */\n-\tfor_each_packed_object(write_oid, &cmd,\n-\t\t\t FOR_EACH_OBJECT_PROMISOR_ONLY);\n+\tif (pack_all)\n+\t\tfor_each_packed_object(write_oid, &cmd, 0);\n+\telse\n+\t\tfor_each_string_list_item(item, list) {\n+\t\t\tpack_mark_retained(item);\n+\t\t}\n+\n+\tfor_each_loose_object(write_loose_oid, &cmd, 0);\n \n \tif (cmd.in == -1) {\n \t\t/* No packed objects; cmd was never started */\n \t\tchild_process_clear(&cmd);\n-\t\treturn;\n+\t\treturn 0;\n \t}\n \n \tclose(cmd.in);\n@@ -453,6 +479,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \tif (finish_command(&cmd))\n \t\tdie(_(\"could not finish pack-objects to repack promisor objects\"));\n \tstrbuf_release(&line);\n+\treturn 0;\n }\n \n struct pack_geometry {\n@@ -1356,9 +1383,13 @@ int cmd_repack(int argc,\n \tif (use_delta_islands)\n \t\tstrvec_push(&cmd.args, \"--delta-islands\");\n \n-\tif (pack_everything & ALL_INTO_ONE) {\n-\t\trepack_promisor_objects(&po_args, &names);\n+\tif (repo_has_promisor_remote(the_repository)) {\n+\t\tret = repack_promisor_objects(&po_args, &names,\n+\t\t\t&existing.non_kept_packs, pack_everything & ALL_INTO_ONE);\n+\t\tgoto pack_objects_end;\n+\t}\n \n+\tif (pack_everything & ALL_INTO_ONE) {\n \t\tif (has_existing_non_kept_packs(&existing) &&\n \t\t delete_redundant &&\n \t\t !(pack_everything & PACK_CRUFT)) {\n@@ -1478,6 +1509,7 @@ int cmd_repack(int argc,\n \t\t}\n \t}\n \n+pack_objects_end:\n \tif (po_args.filter_options.choice) {\n \t\tif (!filter_to)\n \t\t\tfilter_to = packtmp;\n-- \n2.47.0.266.g0b04b6b485.dirty\n\n"},{"id":"504823","messageId":"20241011082404.88939-3-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20241011082404.88939-1-hanyang.tony@bytedance.com","subject":"[PATCH v3 2/3] repack: adapt tests to repack changes","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-10-11T08:24:03Z","receivedAt":"2024-10-11T08:24:23Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"In the previous commit, we changed how partial repo is cloned.\nAdapt tests to these changes. Also check gc does not delete normal\nobjects too.\n\nSigned-off-by: Han Young \n---\n t/t0410-partial-clone.sh | 68 +--------------------------\n t/t5616-partial-clone.sh | 9 +---\n t/t5710-promisor-remote-capability.sh | 15 ++++--\n 3 files changed, 14 insertions(+), 78 deletions(-)\n\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 818700fbec..c87102fcb7 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -500,46 +500,6 @@ test_expect_success 'single promisor remote can be re-initialized gracefully' '\n \tgit -C repo fetch --filter=blob:none foo\n '\n \n-test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '\n-\trm -rf repo &&\n-\ttest_create_repo repo &&\n-\ttest_commit -C repo one &&\n-\ttest_commit -C repo two &&\n-\n-\tTREE_ONE=$(git -C repo rev-parse one^{tree}) &&\n-\tprintf \"$TREE_ONE\\n\" | pack_as_from_promisor &&\n-\tTREE_TWO=$(git -C repo rev-parse two^{tree}) &&\n-\tprintf \"$TREE_TWO\\n\" | pack_as_from_promisor &&\n-\n-\tgit -C repo config core.repositoryformatversion 1 &&\n-\tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n-\tgit -C repo gc &&\n-\n-\t# Ensure that exactly one promisor packfile exists, and that it\n-\t# contains the trees but not the commits\n-\tls repo/.git/objects/pack/pack-*.promisor >promisorlist &&\n-\ttest_line_count = 1 promisorlist &&\n-\tPROMISOR_PACKFILE=$(sed \"s/.promisor/.pack/\" out &&\n-\tgrep \"$TREE_ONE\" out &&\n-\tgrep \"$TREE_TWO\" out &&\n-\t! grep \"$(git -C repo rev-parse one)\" out &&\n-\t! grep \"$(git -C repo rev-parse two)\" out &&\n-\n-\t# Remove the promisor packfile and associated files\n-\trm $(sed \"s/.promisor//\" packlist &&\n-\ttest_line_count = 1 packlist &&\n-\tgit verify-pack repo/.git/objects/pack/pack-*.pack -v >out &&\n-\tgrep \"$(git -C repo rev-parse one)\" out &&\n-\tgrep \"$(git -C repo rev-parse two)\" out &&\n-\t! grep \"$TREE_ONE\" out &&\n-\t! grep \"$TREE_TWO\" out\n-'\n-\n test_expect_success 'gc does not repack promisor objects if there are none' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n@@ -570,7 +530,7 @@ repack_and_check () {\n \tgit -C repo2 cat-file -e $3\n }\n \n-test_expect_success 'repack -d does not irreversibly delete promisor objects' '\n+test_expect_success 'repack -d does not irreversibly delete objects' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n \tgit -C repo config core.repositoryformatversion 1 &&\n@@ -584,40 +544,14 @@ test_expect_success 'repack -d does not irreversibly delete promisor objects' '\n \tTWO=$(git -C repo rev-parse HEAD^^) &&\n \tTHREE=$(git -C repo rev-parse HEAD^) &&\n \n-\tprintf \"$TWO\\n\" | pack_as_from_promisor &&\n \tprintf \"$THREE\\n\" | pack_as_from_promisor &&\n \tdelete_object repo \"$ONE\" &&\n \n-\trepack_and_check --must-fail -ab \"$TWO\" \"$THREE\" &&\n \trepack_and_check -a \"$TWO\" \"$THREE\" &&\n \trepack_and_check -A \"$TWO\" \"$THREE\" &&\n \trepack_and_check -l \"$TWO\" \"$THREE\"\n '\n \n-test_expect_success 'gc stops traversal when a missing but promised object is reached' '\n-\trm -rf repo &&\n-\ttest_create_repo repo &&\n-\ttest_commit -C repo my_commit &&\n-\n-\tTREE_HASH=$(git -C repo rev-parse HEAD^{tree}) &&\n-\tHASH=$(promise_and_delete $TREE_HASH) &&\n-\n-\tgit -C repo config core.repositoryformatversion 1 &&\n-\tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n-\tgit -C repo gc &&\n-\n-\t# Ensure that the promisor packfile still exists, and remove it\n-\ttest -e repo/.git/objects/pack/pack-$HASH.pack &&\n-\trm repo/.git/objects/pack/pack-$HASH.* &&\n-\n-\t# Ensure that the single other pack contains the commit, but not the tree\n-\tls repo/.git/objects/pack/pack-*.pack >packlist &&\n-\ttest_line_count = 1 packlist &&\n-\tgit verify-pack repo/.git/objects/pack/pack-*.pack -v >out &&\n-\tgrep \"$(git -C repo rev-parse HEAD)\" out &&\n-\t! grep \"$TREE_HASH\" out\n-'\n-\n test_expect_success 'do not fetch when checking existence of tree we construct ourselves' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 2c2c50e3ff..19166cd4ce 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -643,16 +643,9 @@ test_expect_success 'fetch from a partial clone, protocol v2' '\n \tgrep \"version 2\" trace\n '\n \n-test_expect_success 'repack does not loosen promisor objects' '\n-\trm -rf client trace &&\n-\tgit clone --bare --filter=blob:none \"file://$(pwd)/srv.bare\" client &&\n-\ttest_when_finished \"rm -rf client trace\" &&\n-\tGIT_TRACE2_PERF=\"$(pwd)/trace\" git -C client repack -A -d &&\n-\tgrep \"loosen_unused_packed_objects/loosened:0\" trace\n-'\n-\n test_expect_success 'lazy-fetch in submodule succeeds' '\n \t# setup\n+\trm -rf client &&\n \ttest_config_global protocol.file.allow always &&\n \n \ttest_when_finished \"rm -rf src-sub\" &&\ndiff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh\nindex c2c83a5914..0912acae44 100755\n--- a/t/t5710-promisor-remote-capability.sh\n+++ b/t/t5710-promisor-remote-capability.sh\n@@ -32,17 +32,26 @@ check_missing_objects () {\n }\n \n initialize_server () {\n-\t# Repack everything first\n-\tgit -C server -c repack.writebitmaps=false repack -a -d &&\n+\tgit -C server remote remove server2\n+\thas_promisor_remote=$?\n \n \t# Remove promisor file in case they exist, useful when reinitializing\n \trm -rf server/objects/pack/*.promisor &&\n \n+\t# Repack everything first\n+\tgit -C server -c repack.writebitmaps=false repack -a -d &&\n+\n \t# Repack without the largest object and create a promisor pack on server\n \tgit -C server -c repack.writebitmaps=false repack -a -d \\\n \t --filter=blob:limit=5k --filter-to=\"$(pwd)\" &&\n \tpromisor_file=$(ls server/objects/pack/*.pack | sed \"s/\\.pack/.promisor/\") &&\n-\ttouch \"$promisor_file\" &&\n+\ttouch \"$promisor_file\"\n+\n+\t# Configure server2 as promisor remote for server\n+\tif [[ $has_promisor_remote -eq 0 ]]; then\n+\t \tgit -C server remote add server2 \"file://$(pwd)/server2\" &&\n+\t \tgit -C server config remote.server2.promisor true\n+\tfi\n \n \t# Check that only one object is missing on the server\n \tcheck_missing_objects server 1 \"$oid\"\n-- \n2.47.0.266.g0b04b6b485.dirty\n\n"},{"id":"504824","messageId":"20241011082404.88939-4-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20241011082404.88939-1-hanyang.tony@bytedance.com","subject":"[PATCH v3 3/3] partial-clone: update doc","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-10-11T08:24:04Z","receivedAt":"2024-10-11T08:24:26Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"Document new repack behavior for partial repo\n\nSigned-off-by: Han Young \n---\n Documentation/technical/partial-clone.txt | 16 ++++++++++++----\n 1 file changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/technical/partial-clone.txt b/Documentation/technical/partial-clone.txt\nindex cd948b0072..9791c9ac24 100644\n--- a/Documentation/technical/partial-clone.txt\n+++ b/Documentation/technical/partial-clone.txt\n@@ -124,6 +124,10 @@ their \".pack\" and \".idx\" files.\n +\n When Git encounters a missing object, Git can see if it is a promisor object\n and handle it appropriately. If not, Git can report a corruption.\n+\n+To prevent `repack` from removing locally created objects, `repack` packs all\n+the objects into one promisor packfile. It's no longer possible to determine\n+the cause of missing objects after `gc`.[7]\n +\n This means that there is no need for the client to explicitly maintain an\n expensive-to-modify list of missing objects.[a]\n@@ -156,8 +160,9 @@ and prefetch those objects in bulk.\n \n - `fsck` has been updated to be fully aware of promisor objects.\n \n-- `repack` in GC has been updated to not touch promisor packfiles at all,\n- and to only repack other objects.\n+- `repack` in GC has been taught to handle partial clone repo differently.\n+ `repack` will pack every objects into one promisor packfile for partial\n+ repos.\n \n - The global variable \"fetch_if_missing\" is used to control whether an\n object lookup will attempt to dynamically fetch a missing object or\n@@ -244,8 +249,7 @@ remote in a specific order.\n objects. We assume that promisor remotes have a complete view of the\n repository and can satisfy all such requests.\n \n-- Repack essentially treats promisor and non-promisor packfiles as 2\n- distinct partitions and does not mix them.\n+- It's not possible to discard dangling objects in repack.\n \n - Dynamic object fetching invokes fetch-pack once *for each item*\n because most algorithms stumble upon a missing object and need to have\n@@ -365,3 +369,7 @@ Related Links\n [6] https://lore.kernel.org/git/20170714132651.170708-1-benpeart@microsoft.com/ +\n Subject: [RFC/PATCH v2 0/1] Add support for downloading blobs on demand +\n Date: Fri, 14 Jul 2017 09:26:50 -0400\n+\n+[7] https://lore.kernel.org/git/20240802073143.56731-1-hanyang.tony@bytedance.com/ +\n+ Subject: [PATCH 0/1] revision: fix reachable objects being gc'ed in no blob clone repo +\n+ Date: Fri, 2 Aug 2024 15:31:42 +0800\n-- \n2.47.0.266.g0b04b6b485.dirty\n\n"},{"id":"504866","messageId":"xmqqa5faec4x.fsf@gitster.g","threadId":"61890","inReplyTo":"20241011082404.88939-1-hanyang.tony@bytedance.com","subject":"Re: [PATCH v3 0/3] repack: pack everything into promisor packfile in partial repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-11T18:18:54Z","receivedAt":"2024-10-11T18:18:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Young writes:\n\n> Changes since v2:\n> - rebased to seen: 89afaf27d3 (Merge branch 'ak/typofixes' into seen, 2024-10-10)\n\nPlease NEVER do this. 'seen' is as unstable and fluid as you can get.\n\nInstead, base it on something that is well known and (supposedly)\nstable, like v2.47.0 (or an updated tip of 'master'), and then\ntest (1) the topic by itself, (2) the result of trial merge of the\ntopic into 'next', and optionally (3) the same for 'seen'.\n\nThanks.\n"},{"id":"504867","messageId":"xmqq5xpyebxl.fsf@gitster.g","threadId":"61890","inReplyTo":"xmqqa5faec4x.fsf@gitster.g","subject":"Re: [PATCH v3 0/3] repack: pack everything into promisor packfile in partial repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-11T18:23:18Z","receivedAt":"2024-10-11T18:23:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Han Young writes:\n>\n>> Changes since v2:\n>> - rebased to seen: 89afaf27d3 (Merge branch 'ak/typofixes' into seen, 2024-10-10)\n>\n> Please NEVER do this. 'seen' is as unstable and fluid as you can get.\n>\n> Instead, base it on something that is well known and (supposedly)\n> stable, like v2.47.0 (or an updated tip of 'master'), and then\n> test (1) the topic by itself, (2) the result of trial merge of the\n> topic into 'next', and optionally (3) the same for 'seen'.\n\nIf your topic really depends on what is done by other topics\nin-flight, either in 'next' or 'seen', then prepare the base\nby\n\n - picking a well known and stable base, e.g. v2.47.0\n\n - merge these ohter topics in-flight you depend on into the base\n you chose above\n\nand then build your series on top. Remember to describe what you\ndid to prepare the base in your cover letter.\n\nDon't directly base your changes to 'next', which would mean your\ntopic will never graduate to 'master', as it is taken hostage by all\nthe topics in 'next' (and the merge commits that merge these topics\ninto 'next', which will never be merged to 'master').\n\nThanks.\n"},{"id":"504894","messageId":"20241012020512.217383-1-jonathantanmy@google.com","threadId":"61890","inReplyTo":"20241009183455.164222-1-jonathantanmy@google.com","subject":"Re: [External] Re: Missing Promisor Objects in Partial Repo Design Doc","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-12T02:05:12Z","receivedAt":"2024-10-12T02:05:15Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jonathan Tan writes:\n> For\n> this reason, I think the repack-on-fetch solution is the most promising\n> one so far.\n\nI had time to take a closer look at this solution. One problem that\nI've noticed is that the \"best effort\" promisor object check cannot\nnaively replace is_promisor_object(), because a lot of the time (e.g.\nin revision.c's get_reference()) is_promisor_object() is used when an\nobject is missing to check whether we need to error out or not. Our\n\"best effort\" promisor object check cannot replace this because it needs\nus to have looked up the object in the first place to check whether it's\nloose or packed (and if packed, which packfile it's in), so it can't\nwork with an object that's missing.\n\nSo I think we'll need to use do_not_die_on_missing_objects. It does have\nthe weakness that if the object is not supposed to be missing, we don't\ninform the user, but perhaps this is OK here because we know that all\nobjects we encounter on this object walk are promisor objects, so if\nit's missing, it's OK.\n\nIn addition to do_not_die_on_missing_objects, we'll also need the actual\ncode that stops iteration through objects that pass our \"best effort\"\npromisor object check. Probably the best place is in get_revision_1()\nafter the NULL check, but I haven't fully thought through what happens\nif this option is used when some commits are UNINTERESTING. (For the\nrepack-on-fetch, no commits are UNINTERESTING, but it's probably best\nto make sure our feature is as useful in as many cases as possible,\nespecially since we're going to further complicate revision walking\ncode, which is complicated enough.\n"},{"id":"504896","messageId":"CAG1j3zFFYcwYg7b9_xGRGHAOHm+qTHY=WpngqtJCrmDznhD+HA@mail.gmail.com","threadId":"61890","inReplyTo":"20241012020512.217383-1-jonathantanmy@google.com","subject":"Re: [External] Re: Missing Promisor Objects in Partial Repo Design Doc","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-10-12T03:30:06Z","receivedAt":"2024-10-12T03:30:18Z","isPatch":false,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"On Sat, Oct 12, 2024 at 10:05 AM Jonathan Tan wrote:\n> So I think we'll need to use do_not_die_on_missing_objects. It does have\n> the weakness that if the object is not supposed to be missing, we don't\n> inform the user, but perhaps this is OK here because we know that all\n> objects we encounter on this object walk are promisor objects, so if\n> it's missing, it's OK.\n\nAnd I think users would prefer the git command to succeed if possible,\nrather than die on the first (noncritical) error. Maybe show a warning\nand swallow the error?\n\n> In addition to do_not_die_on_missing_objects, we'll also need the actual\n> code that stops iteration through objects that pass our \"best effort\"\n> promisor object check. Probably the best place is in get_revision_1()\n> after the NULL check\n\nget_revision_1() only does commit limiting though. Some callers of rev-list\nalso do tree walking on commits, in a (corrupted) partial repo, tree could\nalso be missing. There isn't a central place we can stop tree walking,\ncallers using this feature would have to implement \"tree walking early\ntermination\" themself.\n"},{"id":"504961","messageId":"20241014032546.68427-1-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20240802073143.56731-1-hanyang.tony@bytedance.com","subject":"[PATCH v4 0/3] repack: pack everything into promisor packfile in partial repos","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-10-14T03:25:42Z","receivedAt":"2024-10-14T03:25:55Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"Changes from v3:\n- rebased to master: ef8ce8f3d4 (Start the 2.48 cycle, 2024-10-10)\n\nNote that this series breaks tests on branch seen, the test introduced by\nbc0c4e1637 (promisor-remote: check advertised name or URL, 2024-09-10)\nrelies on the current repack behavior. I will provide an additional patch\nif both land in master.\n\nThis series doesn't address the underlying problem with promisor objects,\nbut rather mitigates the \"repack removes local objects\" problem.\nUntil a satisfiable solution can be found[1], this would at least prevent\nmore promisor repos from becoming corrupted.\n\n[1] https://lore.kernel.org/git/20241001191811.1934900-1-calvinwan@google.com/\n\nHan Young (3):\n repack: pack everything into packfile\n t0410: adapt tests to repack changes\n partial-clone: update doc\n\n Documentation/technical/partial-clone.txt | 16 ++++--\n builtin/repack.c | 46 ++++++++++++---\n t/t0410-partial-clone.sh | 68 +----------------------\n t/t5616-partial-clone.sh | 9 +--\n 4 files changed, 53 insertions(+), 86 deletions(-)\n\n-- \n2.46.0\n\n"},{"id":"504962","messageId":"20241014032546.68427-2-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20241014032546.68427-1-hanyang.tony@bytedance.com","subject":"[PATCH v4 1/3] repack: pack everything into packfile","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-10-14T03:25:43Z","receivedAt":"2024-10-14T03:25:59Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"In a partial repository, creating a local commit and then fetching\ncauses the following state to occur:\n\ncommit tree blob\n C3 ---- T3 -- B3 (fetched from remote, in promisor pack)\n |\n C2 ---- T2 -- B2 (created locally, in non-promisor pack)\n |\n C1 ---- T1 -- B1 (fetched from remote, in promisor pack)\n\nDuring garbage collection, parents of promisor objects are marked as\nUNINTERESTING and are subsequently garbage collected. In this case, C2\nwould be deleted and attempts to access that commit would result in \"bad\nobject\" errors (originally reported here[1]).\n\nFor partial repos, repacking is done in two steps. We first repack all the\nobjects in promisor packfile, then repack all the non-promisor objects.\nIt turns out C2, T2 and B2 are not repacked in either steps, ended up deleted.\nWe can avoid this by packing everything into a promisor packfile, if the repo\nis partial cloned.\n\n[1] https://lore.kernel.org/git/20240802073143.56731-1-hanyang.tony@bytedance.com/\n\nHelped-by: Calvin Wan \nSigned-off-by: Han Young \n---\n builtin/repack.c | 46 +++++++++++++++++++++++++++++++++++++++-------\n 1 file changed, 39 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex d6bb37e84a..071d2171da 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -338,6 +338,23 @@ static int write_oid(const struct object_id *oid,\n \treturn 0;\n }\n \n+static int write_loose_oid(const struct object_id *oid,\n+\t\t\t const char *path UNUSED,\n+\t\t\t void *data)\n+{\n+\tstruct child_process *cmd = data;\n+\n+\tif (cmd->in == -1) {\n+\t\tif (start_command(cmd))\n+\t\t\tdie(_(\"could not start pack-objects to repack promisor objects\"));\n+\t}\n+\n+\tif (write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n+\t write_in_full(cmd->in, \"\\n\", 1) < 0)\n+\t\tdie(_(\"failed to feed promisor objects to pack-objects\"));\n+\treturn 0;\n+}\n+\n static struct {\n \tconst char *name;\n \tunsigned optional:1;\n@@ -387,12 +404,15 @@ static int has_pack_ext(const struct generated_pack_data *data,\n \tBUG(\"unknown pack extension: '%s'\", ext);\n }\n \n-static void repack_promisor_objects(const struct pack_objects_args *args,\n-\t\t\t\t struct string_list *names)\n+static int repack_promisor_objects(const struct pack_objects_args *args,\n+\t\t\t\t struct string_list *names,\n+\t\t\t\t struct string_list *list,\n+\t\t\t\t int pack_all)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tFILE *out;\n \tstruct strbuf line = STRBUF_INIT;\n+\tstruct string_list_item *item;\n \n \tprepare_pack_objects(&cmd, args, packtmp);\n \tcmd.in = -1;\n@@ -404,13 +424,19 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \t * {type -> existing pack order} ordering when computing deltas instead\n \t * of a {type -> size} ordering, which may produce better deltas.\n \t */\n-\tfor_each_packed_object(write_oid, &cmd,\n-\t\t\t FOR_EACH_OBJECT_PROMISOR_ONLY);\n+\tif (pack_all)\n+\t\tfor_each_packed_object(write_oid, &cmd, 0);\n+\telse\n+\t\tfor_each_string_list_item(item, list) {\n+\t\t\tpack_mark_retained(item);\n+\t\t}\n+\n+\tfor_each_loose_object(write_loose_oid, &cmd, 0);\n \n \tif (cmd.in == -1) {\n \t\t/* No packed objects; cmd was never started */\n \t\tchild_process_clear(&cmd);\n-\t\treturn;\n+\t\treturn 0;\n \t}\n \n \tclose(cmd.in);\n@@ -448,6 +474,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \tif (finish_command(&cmd))\n \t\tdie(_(\"could not finish pack-objects to repack promisor objects\"));\n \tstrbuf_release(&line);\n+\treturn 0;\n }\n \n struct pack_geometry {\n@@ -1349,9 +1376,13 @@ int cmd_repack(int argc,\n \tif (use_delta_islands)\n \t\tstrvec_push(&cmd.args, \"--delta-islands\");\n \n-\tif (pack_everything & ALL_INTO_ONE) {\n-\t\trepack_promisor_objects(&po_args, &names);\n+\tif (repo_has_promisor_remote(the_repository)) {\n+\t\tret = repack_promisor_objects(&po_args, &names,\n+\t\t\t&existing.non_kept_packs, pack_everything & ALL_INTO_ONE);\n+\t\tgoto pack_objects_end;\n+\t}\n \n+\tif (pack_everything & ALL_INTO_ONE) {\n \t\tif (has_existing_non_kept_packs(&existing) &&\n \t\t delete_redundant &&\n \t\t !(pack_everything & PACK_CRUFT)) {\n@@ -1471,6 +1502,7 @@ int cmd_repack(int argc,\n \t\t}\n \t}\n \n+pack_objects_end:\n \tif (po_args.filter_options.choice) {\n \t\tif (!filter_to)\n \t\t\tfilter_to = packtmp;\n-- \n2.46.0\n\n"},{"id":"504963","messageId":"20241014032546.68427-3-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20241014032546.68427-1-hanyang.tony@bytedance.com","subject":"[PATCH v4 2/3] t0410: adapt tests to repack changes","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-10-14T03:25:44Z","receivedAt":"2024-10-14T03:26:03Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"In the previous commit, we changed how partial repo is cloned.\nAdapt tests to these changes. Also check gc does not delete normal\nobjects too.\n\nSigned-off-by: Han Young \n---\n t/t0410-partial-clone.sh | 68 +---------------------------------------\n t/t5616-partial-clone.sh | 9 +-----\n 2 files changed, 2 insertions(+), 75 deletions(-)\n\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 818700fbec..c87102fcb7 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -500,46 +500,6 @@ test_expect_success 'single promisor remote can be re-initialized gracefully' '\n \tgit -C repo fetch --filter=blob:none foo\n '\n \n-test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '\n-\trm -rf repo &&\n-\ttest_create_repo repo &&\n-\ttest_commit -C repo one &&\n-\ttest_commit -C repo two &&\n-\n-\tTREE_ONE=$(git -C repo rev-parse one^{tree}) &&\n-\tprintf \"$TREE_ONE\\n\" | pack_as_from_promisor &&\n-\tTREE_TWO=$(git -C repo rev-parse two^{tree}) &&\n-\tprintf \"$TREE_TWO\\n\" | pack_as_from_promisor &&\n-\n-\tgit -C repo config core.repositoryformatversion 1 &&\n-\tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n-\tgit -C repo gc &&\n-\n-\t# Ensure that exactly one promisor packfile exists, and that it\n-\t# contains the trees but not the commits\n-\tls repo/.git/objects/pack/pack-*.promisor >promisorlist &&\n-\ttest_line_count = 1 promisorlist &&\n-\tPROMISOR_PACKFILE=$(sed \"s/.promisor/.pack/\" out &&\n-\tgrep \"$TREE_ONE\" out &&\n-\tgrep \"$TREE_TWO\" out &&\n-\t! grep \"$(git -C repo rev-parse one)\" out &&\n-\t! grep \"$(git -C repo rev-parse two)\" out &&\n-\n-\t# Remove the promisor packfile and associated files\n-\trm $(sed \"s/.promisor//\" packlist &&\n-\ttest_line_count = 1 packlist &&\n-\tgit verify-pack repo/.git/objects/pack/pack-*.pack -v >out &&\n-\tgrep \"$(git -C repo rev-parse one)\" out &&\n-\tgrep \"$(git -C repo rev-parse two)\" out &&\n-\t! grep \"$TREE_ONE\" out &&\n-\t! grep \"$TREE_TWO\" out\n-'\n-\n test_expect_success 'gc does not repack promisor objects if there are none' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n@@ -570,7 +530,7 @@ repack_and_check () {\n \tgit -C repo2 cat-file -e $3\n }\n \n-test_expect_success 'repack -d does not irreversibly delete promisor objects' '\n+test_expect_success 'repack -d does not irreversibly delete objects' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n \tgit -C repo config core.repositoryformatversion 1 &&\n@@ -584,40 +544,14 @@ test_expect_success 'repack -d does not irreversibly delete promisor objects' '\n \tTWO=$(git -C repo rev-parse HEAD^^) &&\n \tTHREE=$(git -C repo rev-parse HEAD^) &&\n \n-\tprintf \"$TWO\\n\" | pack_as_from_promisor &&\n \tprintf \"$THREE\\n\" | pack_as_from_promisor &&\n \tdelete_object repo \"$ONE\" &&\n \n-\trepack_and_check --must-fail -ab \"$TWO\" \"$THREE\" &&\n \trepack_and_check -a \"$TWO\" \"$THREE\" &&\n \trepack_and_check -A \"$TWO\" \"$THREE\" &&\n \trepack_and_check -l \"$TWO\" \"$THREE\"\n '\n \n-test_expect_success 'gc stops traversal when a missing but promised object is reached' '\n-\trm -rf repo &&\n-\ttest_create_repo repo &&\n-\ttest_commit -C repo my_commit &&\n-\n-\tTREE_HASH=$(git -C repo rev-parse HEAD^{tree}) &&\n-\tHASH=$(promise_and_delete $TREE_HASH) &&\n-\n-\tgit -C repo config core.repositoryformatversion 1 &&\n-\tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n-\tgit -C repo gc &&\n-\n-\t# Ensure that the promisor packfile still exists, and remove it\n-\ttest -e repo/.git/objects/pack/pack-$HASH.pack &&\n-\trm repo/.git/objects/pack/pack-$HASH.* &&\n-\n-\t# Ensure that the single other pack contains the commit, but not the tree\n-\tls repo/.git/objects/pack/pack-*.pack >packlist &&\n-\ttest_line_count = 1 packlist &&\n-\tgit verify-pack repo/.git/objects/pack/pack-*.pack -v >out &&\n-\tgrep \"$(git -C repo rev-parse HEAD)\" out &&\n-\t! grep \"$TREE_HASH\" out\n-'\n-\n test_expect_success 'do not fetch when checking existence of tree we construct ourselves' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex c53e93be2f..2c6f10026f 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -643,16 +643,9 @@ test_expect_success 'fetch from a partial clone, protocol v2' '\n \tgrep \"version 2\" trace\n '\n \n-test_expect_success 'repack does not loosen promisor objects' '\n-\trm -rf client trace &&\n-\tgit clone --bare --filter=blob:none \"file://$(pwd)/srv.bare\" client &&\n-\ttest_when_finished \"rm -rf client trace\" &&\n-\tGIT_TRACE2_PERF=\"$(pwd)/trace\" git -C client repack -A -d &&\n-\tgrep \"loosen_unused_packed_objects/loosened:0\" trace\n-'\n-\n test_expect_success 'lazy-fetch in submodule succeeds' '\n \t# setup\n+\trm -rf client &&\n \ttest_config_global protocol.file.allow always &&\n \n \ttest_when_finished \"rm -rf src-sub\" &&\n-- \n2.46.0\n\n"},{"id":"504964","messageId":"20241014032546.68427-4-hanyang.tony@bytedance.com","threadId":"61890","inReplyTo":"20241014032546.68427-1-hanyang.tony@bytedance.com","subject":"[PATCH v4 3/3] partial-clone: update doc","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-10-14T03:25:45Z","receivedAt":"2024-10-14T03:26:06Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"Document new repack behavior for partial repo\n\nSigned-off-by: Han Young \n---\n Documentation/technical/partial-clone.txt | 16 ++++++++++++----\n 1 file changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/technical/partial-clone.txt b/Documentation/technical/partial-clone.txt\nindex cd948b0072..9791c9ac24 100644\n--- a/Documentation/technical/partial-clone.txt\n+++ b/Documentation/technical/partial-clone.txt\n@@ -124,6 +124,10 @@ their \".pack\" and \".idx\" files.\n +\n When Git encounters a missing object, Git can see if it is a promisor object\n and handle it appropriately. If not, Git can report a corruption.\n+\n+To prevent `repack` from removing locally created objects, `repack` packs all\n+the objects into one promisor packfile. It's no longer possible to determine\n+the cause of missing objects after `gc`.[7]\n +\n This means that there is no need for the client to explicitly maintain an\n expensive-to-modify list of missing objects.[a]\n@@ -156,8 +160,9 @@ and prefetch those objects in bulk.\n \n - `fsck` has been updated to be fully aware of promisor objects.\n \n-- `repack` in GC has been updated to not touch promisor packfiles at all,\n- and to only repack other objects.\n+- `repack` in GC has been taught to handle partial clone repo differently.\n+ `repack` will pack every objects into one promisor packfile for partial\n+ repos.\n \n - The global variable \"fetch_if_missing\" is used to control whether an\n object lookup will attempt to dynamically fetch a missing object or\n@@ -244,8 +249,7 @@ remote in a specific order.\n objects. We assume that promisor remotes have a complete view of the\n repository and can satisfy all such requests.\n \n-- Repack essentially treats promisor and non-promisor packfiles as 2\n- distinct partitions and does not mix them.\n+- It's not possible to discard dangling objects in repack.\n \n - Dynamic object fetching invokes fetch-pack once *for each item*\n because most algorithms stumble upon a missing object and need to have\n@@ -365,3 +369,7 @@ Related Links\n [6] https://lore.kernel.org/git/20170714132651.170708-1-benpeart@microsoft.com/ +\n Subject: [RFC/PATCH v2 0/1] Add support for downloading blobs on demand +\n Date: Fri, 14 Jul 2017 09:26:50 -0400\n+\n+[7] https://lore.kernel.org/git/20240802073143.56731-1-hanyang.tony@bytedance.com/ +\n+ Subject: [PATCH 0/1] revision: fix reachable objects being gc'ed in no blob clone repo +\n+ Date: Fri, 2 Aug 2024 15:31:42 +0800\n-- \n2.46.0\n\n"},{"id":"505041","messageId":"20241014175203.740046-1-jonathantanmy@google.com","threadId":"61890","inReplyTo":"CAG1j3zFFYcwYg7b9_xGRGHAOHm+qTHY=WpngqtJCrmDznhD+HA@mail.gmail.com","subject":"Re: [External] Re: Missing Promisor Objects in Partial Repo Design Doc","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-14T17:52:03Z","receivedAt":"2024-10-14T17:52:07Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Han Young writes:\n> On Sat, Oct 12, 2024 at 10:05 AM Jonathan Tan wrote:\n> > So I think we'll need to use do_not_die_on_missing_objects. It does have\n> > the weakness that if the object is not supposed to be missing, we don't\n> > inform the user, but perhaps this is OK here because we know that all\n> > objects we encounter on this object walk are promisor objects, so if\n> > it's missing, it's OK.\n> \n> And I think users would prefer the git command to succeed if possible,\n> rather than die on the first (noncritical) error. Maybe show a warning\n> and swallow the error?\n\nJust to be clear, this is not an error condition so we shouldn't show an\nerror or warning. Whenever we repack non-promisor objects in a partial\nclone we will almost always encounter missing objects. In the gc repack,\nthis is mitigated by --exclude-promisor-objects, which checks the\npromisor object set whenever a missing object is encountered; it does\nnot show an error if the missing object is in that set.\n\nMy proposal is to use do_not_die_on_missing_objects instead, which\nalways tolerates missing objects (without showing any error or warning).\nThis is probably not safe enough for the gc repack, but should be OK\nfor the fetch repack, since we are only repacking ancestors of known\npromisor objects (so we can deduce that the missing objects are promisor\nobjects).\n\n> > In addition to do_not_die_on_missing_objects, we'll also need the actual\n> > code that stops iteration through objects that pass our \"best effort\"\n> > promisor object check. Probably the best place is in get_revision_1()\n> > after the NULL check\n> \n> get_revision_1() only does commit limiting though. Some callers of rev-list\n> also do tree walking on commits,\n\nAh, yes, you're right. The repack on fetch is one such caller (that will\nneed tree walking).\n\n> in a (corrupted) partial repo, tree could\n> also be missing. There isn't a central place we can stop tree walking,\n> callers using this feature would have to implement \"tree walking early\n> termination\" themself.\n\nThe repo could have been cloned with a tree filter (e.g.\n\"--filter=tree:0\") too, in which case trees would be missing even if the\nrepo is not corrupted. But even in a non-corrupted --filter=blob:none\npartial clone, we still don't want to iterate through promisor trees, so\nthat we don't repack them unnecessarily. So yes, get_revision_1() is not\nthe only place that needs to be changed.\n\nI think that there is a central place to stop tree walking - in\nlist-objects.c.\n\n\n"},{"id":"505788","messageId":"cover.1729549127.git.jonathantanmy@google.com","threadId":"61890","inReplyTo":"20241014032546.68427-1-hanyang.tony@bytedance.com","subject":"[WIP 0/3] Repack on fetch","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-21T22:29:42Z","receivedAt":"2024-10-21T22:29:50Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"I think that ultimately we want something like repack on fetch, so I\nmade some effort to implement it. There were some details that needed\nto be ironed out, but here's a WIP of the repack-on-fetch solution. In\nparticular, note that we do not need to create the expensive set used by\nis_promisor_object().\n\nAs you can see from the patches, some polishing still needs to be\ndone, but I'm sending them out now to check if other people have\nopinions about the solution. In particular, Han Young reported that an\nalternative solution (repack on GC) takes too long [1], so I would be\ninterested to see if the time taken by this solution is good enough for\nHan Young's use case.\n\n[1] https://lore.kernel.org/git/CAG1j3zHJVrpK5JZtUXFwkZgWY1-CxqET+ygpaMqo5aM-KeWaxg@mail.gmail.com/\n\nJonathan Tan (3):\n move variable\n pack-objects\n record local links and call pack-objects\n\n builtin/index-pack.c | 116 ++++++++++++++++++++++++++++++++++++++-\n builtin/pack-objects.c | 31 ++++++++++-\n t/t0410-partial-clone.sh | 11 ++--\n t/t5300-pack-object.sh | 8 +--\n t/t5616-partial-clone.sh | 30 ++++++++++\n 5 files changed, 183 insertions(+), 13 deletions(-)\n\n-- \n2.47.0.105.g07ac214952-goog\n\n"},{"id":"505789","messageId":"c66937e675ee2e6e4dac067f180da949f0bc14bc.1729549127.git.jonathantanmy@google.com","threadId":"61890","inReplyTo":"cover.1729549127.git.jonathantanmy@google.com","subject":"[WIP 1/3] move variable","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-21T22:29:43Z","receivedAt":"2024-10-21T22:29:52Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"---\n builtin/pack-objects.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 0fc0680b40..e15fbaeb21 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -238,8 +238,6 @@ static enum {\n } write_bitmap_index;\n static uint16_t write_bitmap_options = BITMAP_OPT_HASH_CACHE;\n \n-static int exclude_promisor_objects;\n-\n static int use_delta_islands;\n \n static unsigned long delta_cache_size = 0;\n@@ -4327,6 +4325,7 @@ int cmd_pack_objects(int argc,\n \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n \tstruct list_objects_filter_options filter_options =\n \t\tLIST_OBJECTS_FILTER_INIT;\n+\tint exclude_promisor_objects = 0;\n \n \tstruct option pack_objects_options[] = {\n \t\tOPT_CALLBACK_F('q', \"quiet\", &progress, NULL,\n-- \n2.47.0.105.g07ac214952-goog\n\n"},{"id":"505790","messageId":"fb2c202591b466eea33b4585e47b70e9086603bb.1729549127.git.jonathantanmy@google.com","threadId":"61890","inReplyTo":"cover.1729549127.git.jonathantanmy@google.com","subject":"[WIP 2/3] pack-objects","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-21T22:29:44Z","receivedAt":"2024-10-21T22:29:53Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"---\n builtin/pack-objects.c | 28 ++++++++++++++++++++++++++++\n 1 file changed, 28 insertions(+)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex e15fbaeb21..a565ab9b40 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4310,6 +4310,18 @@ static int option_parse_cruft_expiration(const struct option *opt UNUSED,\n \treturn 0;\n }\n \n+static int should_include_obj(struct object *obj, void *data UNUSED)\n+{\n+\tstruct object_info info = OBJECT_INFO_INIT;\n+\tif (oid_object_info_extended(the_repository, &obj->oid, &info, 0))\n+\t\tBUG(\"should_include_obj should only be called on existing objects\");\n+\treturn info.whence != OI_PACKED || !info.u.packed.pack->pack_promisor;\n+}\n+\n+static int should_include(struct commit *commit, void *data) {\n+\treturn should_include_obj((struct object *) commit, data);\n+}\n+\n int cmd_pack_objects(int argc,\n \t\t const char **argv,\n \t\t const char *prefix,\n@@ -4326,6 +4338,7 @@ int cmd_pack_objects(int argc,\n \tstruct list_objects_filter_options filter_options =\n \t\tLIST_OBJECTS_FILTER_INIT;\n \tint exclude_promisor_objects = 0;\n+\tint exclude_promisor_objects_best_effort = 0;\n \n \tstruct option pack_objects_options[] = {\n \t\tOPT_CALLBACK_F('q', \"quiet\", &progress, NULL,\n@@ -4423,6 +4436,9 @@ int cmd_pack_objects(int argc,\n \t\t option_parse_missing_action),\n \t\tOPT_BOOL(0, \"exclude-promisor-objects\", &exclude_promisor_objects,\n \t\t\t N_(\"do not pack objects in promisor packfiles\")),\n+\t\tOPT_BOOL(0, \"exclude-promisor-objects-best-effort\",\n+\t\t\t &exclude_promisor_objects_best_effort,\n+\t\t\t N_(\"implies --missing=allow-any\")),\n \t\tOPT_BOOL(0, \"delta-islands\", &use_delta_islands,\n \t\t\t N_(\"respect islands during delta compression\")),\n \t\tOPT_STRING_LIST(0, \"uri-protocol\", &uri_protocols,\n@@ -4503,10 +4519,18 @@ int cmd_pack_objects(int argc,\n \t\tstrvec_push(&rp, \"--unpacked\");\n \t}\n \n+\tif (exclude_promisor_objects && exclude_promisor_objects_best_effort)\n+\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t \"--exclude-promisor-objects\", \"--exclude-promisor-objects-best-effort\");\n \tif (exclude_promisor_objects) {\n \t\tuse_internal_rev_list = 1;\n \t\tfetch_if_missing = 0;\n \t\tstrvec_push(&rp, \"--exclude-promisor-objects\");\n+\t} else if (exclude_promisor_objects_best_effort) {\n+\t\tuse_internal_rev_list = 1;\n+\t\tfetch_if_missing = 0;\n+\t\toption_parse_missing_action(NULL, \"allow-any\", 0);\n+\t\t/* revs configured below */\n \t}\n \tif (unpack_unreachable || keep_unreachable || pack_loose_unreachable)\n \t\tuse_internal_rev_list = 1;\n@@ -4626,6 +4650,10 @@ int cmd_pack_objects(int argc,\n \n \t\trepo_init_revisions(the_repository, &revs, NULL);\n \t\tlist_objects_filter_copy(&revs.filter, &filter_options);\n+\t\tif (exclude_promisor_objects_best_effort) {\n+\t\t\trevs.include_check = should_include;\n+\t\t\trevs.include_check_obj = should_include_obj;\n+\t\t}\n \t\tget_object_list(&revs, rp.nr, rp.v);\n \t\trelease_revisions(&revs);\n \t}\n-- \n2.47.0.105.g07ac214952-goog\n\n"},{"id":"505791","messageId":"de081ce80eb7358a347ea2435f5fee23d3557e75.1729549127.git.jonathantanmy@google.com","threadId":"61890","inReplyTo":"cover.1729549127.git.jonathantanmy@google.com","subject":"[WIP 3/3] record local links and call pack-objects","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-21T22:29:45Z","receivedAt":"2024-10-21T22:29:55Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"---\n builtin/index-pack.c | 116 ++++++++++++++++++++++++++++++++++++++-\n t/t0410-partial-clone.sh | 11 ++--\n t/t5300-pack-object.sh | 8 +--\n t/t5616-partial-clone.sh | 30 ++++++++++\n 4 files changed, 154 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex e228c56ff2..77e9abc3b0 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -9,6 +9,7 @@\n #include \"csum-file.h\"\n #include \"blob.h\"\n #include \"commit.h\"\n+#include \"tag.h\"\n #include \"tree.h\"\n #include \"progress.h\"\n #include \"fsck.h\"\n@@ -20,9 +21,14 @@\n #include \"object-file.h\"\n #include \"object-store-ll.h\"\n #include \"oid-array.h\"\n+#include \"oidset.h\"\n+#include \"path.h\"\n #include \"replace-object.h\"\n+#include \"tree-walk.h\"\n #include \"promisor-remote.h\"\n+#include \"run-command.h\"\n #include \"setup.h\"\n+#include \"strvec.h\"\n \n static const char index_pack_usage[] =\n \"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[==...]] [--fsck-objects[==...]] ( | --stdin [--fix-thin] [])\";\n@@ -148,6 +154,13 @@ static uint32_t input_crc32;\n static int input_fd, output_fd;\n static const char *curr_pack;\n \n+/*\n+ * local_links is guarded by work_mutex, and record_local_links is read-only in\n+ * a thread.\n+ */\n+static struct oidset local_links = OIDSET_INIT;\n+static int record_local_links;\n+\n static struct thread_local *thread_data;\n static int nr_dispatched;\n static int threads_active;\n@@ -168,6 +181,10 @@ static pthread_mutex_t deepest_delta_mutex;\n #define deepest_delta_lock()\tlock_mutex(&deepest_delta_mutex)\n #define deepest_delta_unlock()\tunlock_mutex(&deepest_delta_mutex)\n \n+static pthread_mutex_t local_links_mutex;\n+#define local_links_lock()\tlock_mutex(&local_links_mutex)\n+#define local_links_unlock()\tunlock_mutex(&local_links_mutex)\n+\n static pthread_key_t key;\n \n static inline void lock_mutex(pthread_mutex_t *mutex)\n@@ -799,6 +816,46 @@ static int check_collison(struct object_entry *entry)\n \treturn 0;\n }\n \n+static void record_if_local_object(const struct object_id *oid)\n+{\n+\tstruct object_info info = OBJECT_INFO_INIT;\n+\tif (oid_object_info_extended(the_repository, oid, &info, 0))\n+\t\t/* Missing; assume it is a promisor object */\n+\t\treturn;\n+\tif (info.whence == OI_PACKED && info.u.packed.pack->pack_promisor)\n+\t\treturn;\n+\tlocal_links_lock();\n+\toidset_insert(&local_links, oid);\n+\tlocal_links_unlock();\n+}\n+\n+static void do_record_local_links(struct object *obj)\n+{\n+\tif (obj->type == OBJ_TREE) {\n+\t\tstruct tree *tree = (struct tree *)obj;\n+\t\tstruct tree_desc desc;\n+\t\tstruct name_entry entry;\n+\t\tif (init_tree_desc_gently(&desc, &tree->object.oid,\n+\t\t\t\t\t tree->buffer, tree->size, 0))\n+\t\t\t/*\n+\t\t\t * Error messages are given when packs are\n+\t\t\t * verified, so do not print any here.\n+\t\t\t */\n+\t\t\treturn;\n+\t\twhile (tree_entry_gently(&desc, &entry))\n+\t\t\trecord_if_local_object(&entry.oid);\n+\t} else if (obj->type == OBJ_COMMIT) {\n+\t\tstruct commit *commit = (struct commit *) obj;\n+\t\tstruct commit_list *parents = commit->parents;\n+\n+\t\tfor (; parents; parents = parents->next)\n+\t\t\trecord_if_local_object(&parents->item->object.oid);\n+\t} else if (obj->type == OBJ_TAG) {\n+\t\tstruct tag *tag = (struct tag *) obj;\n+\t\trecord_if_local_object(get_tagged_oid(tag));\n+\t}\n+}\n+\n static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tunsigned long size, enum object_type type,\n \t\t\tconst struct object_id *oid)\n@@ -845,7 +902,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\tfree(has_data);\n \t}\n \n-\tif (strict || do_fsck_object) {\n+\tif (strict || do_fsck_object || record_local_links) {\n \t\tread_lock();\n \t\tif (type == OBJ_BLOB) {\n \t\t\tstruct blob *blob = lookup_blob(the_repository, oid);\n@@ -877,6 +934,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\t\tdie(_(\"fsck error in packed object\"));\n \t\t\tif (strict && fsck_walk(obj, NULL, &fsck_options))\n \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), oid_to_hex(&obj->oid));\n+\t\t\tif (record_local_links)\n+\t\t\t\tdo_record_local_links(obj);\n \n \t\t\tif (obj->type == OBJ_TREE) {\n \t\t\t\tstruct tree *item = (struct tree *) obj;\n@@ -1719,6 +1778,57 @@ static void show_pack_info(int stat_only)\n \tfree(chain_histogram);\n }\n \n+static void repack_local_links(void)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tFILE *out;\n+\tstruct strbuf line = STRBUF_INIT;\n+\tstruct oidset_iter iter;\n+\tstruct object_id *oid;\n+\tchar *base_name;\n+\n+\tif (!oidset_size(&local_links))\n+\t\treturn;\n+\n+\tbase_name = mkpathdup(\"%s/pack/pack\", repo_get_object_directory(the_repository));\n+\n+\tstrvec_push(&cmd.args, \"pack-objects\");\n+\tstrvec_push(&cmd.args, \"--exclude-promisor-objects-best-effort\");\n+\tstrvec_push(&cmd.args, base_name);\n+\tcmd.git_cmd = 1;\n+\tcmd.in = -1;\n+\tcmd.out = -1;\n+\tif (start_command(&cmd))\n+\t\tdie(_(\"could not start pack-objects to repack local links\"));\n+\n+\toidset_iter_init(&local_links, &iter);\n+\twhile ((oid = oidset_iter_next(&iter))) {\n+\t\tif (write_in_full(cmd.in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n+\t\t write_in_full(cmd.in, \"\\n\", 1) < 0)\n+\t\t\tdie(_(\"failed to feed local object to pack-objects\"));\n+\t}\n+\tclose(cmd.in);\n+\n+\tout = xfdopen(cmd.out, \"r\");\n+\twhile (strbuf_getline_lf(&line, out) != EOF) {\n+\t\tunsigned char binary[GIT_MAX_RAWSZ];\n+\t\tif (line.len != the_hash_algo->hexsz ||\n+\t\t !hex_to_bytes(binary, line.buf, line.len))\n+\t\t\tdie(_(\"index-pack: Expecting full hex object ID lines only from pack-objects.\"));\n+\n+\t\t/*\n+\t\t * pack-objects creates the .pack and .idx files, but not the\n+\t\t * .promisor file. Create the .promisor file, which is empty.\n+\t\t */\n+\t\twrite_special_file(\"promisor\", \"\", NULL, binary, NULL);\n+\t}\n+\n+\tfclose(out);\n+\tif (finish_command(&cmd))\n+\t\tdie(_(\"could not finish pack-objects to repack local links\"));\n+\tstrbuf_release(&line);\n+}\n+\n int cmd_index_pack(int argc,\n \t\t const char **argv,\n \t\t const char *prefix,\n@@ -1794,7 +1904,7 @@ int cmd_index_pack(int argc,\n \t\t\t} else if (skip_to_optional_arg(arg, \"--keep\", &keep_msg)) {\n \t\t\t\t; /* nothing to do */\n \t\t\t} else if (skip_to_optional_arg(arg, \"--promisor\", &promisor_msg)) {\n-\t\t\t\t; /* already parsed */\n+\t\t\t\trecord_local_links = 1;\n \t\t\t} else if (starts_with(arg, \"--threads=\")) {\n \t\t\t\tchar *end;\n \t\t\t\tnr_threads = strtoul(arg+10, &end, 0);\n@@ -1971,6 +2081,8 @@ int cmd_index_pack(int argc,\n \tif (!rev_index_name)\n \t\tfree((void *) curr_rev_index);\n \n+\trepack_local_links();\n+\n \t/*\n \t * Let the caller know this pack is not self contained\n \t */\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 34bdb3ab1f..4c3d93c3db 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -279,11 +279,12 @@ test_expect_success 'fetching of missing objects configures a promisor remote' '\n \n \t# Ensure that the .promisor file is written, and check that its\n \t# associated packfile contains the object\n-\tls repo/.git/objects/pack/pack-*.promisor >promisorlist &&\n-\ttest_line_count = 1 promisorlist &&\n-\tIDX=$(sed \"s/promisor$/idx/\" promisorlist) &&\n-\tgit verify-pack --verbose \"$IDX\" >out &&\n-\tgrep \"$HASH3\" out\n+\t#ls repo/.git/objects/pack/pack-*.promisor >promisorlist &&\n+\t#test_line_count = 1 promisorlist &&\n+\t#IDX=$(sed \"s/promisor$/idx/\" promisorlist) &&\n+\t#git verify-pack --verbose \"$IDX\" >out &&\n+\t#grep \"$HASH3\" out\n+\ttrue\n '\n \n test_expect_success 'fetching of missing blobs works' '\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 3b9dae331a..514ac9a832 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -630,10 +630,10 @@ test_expect_success 'prefetch objects' '\n \ttest_line_count = 1 donelines\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\n-'\n+#test_expect_success 'negative window clamps to 0' '\n+\t#git pack-objects --progress --window=-1 neg-window stderr &&\n+\t#check_deltas stderr = 0\n+#'\n \n for hash in sha1 sha256\n do\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex c53e93be2f..c2541010bf 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -694,6 +694,36 @@ test_expect_success 'lazy-fetch in submodule succeeds' '\n \tgit -C client restore --recurse-submodules --source=HEAD^ :/\n '\n \n+test_expect_success 'the test from calvins patch' '\n+\t# Setup\n+\tgit init full &&\n+\tgit -C full config uploadpack.allowfilter 1 &&\n+ \tgit -C full config uploadpack.allowanysha1inwant 1 &&\n+\ttouch full/foo &&\n+\tgit -C full add foo &&\n+\tgit -C full commit -m \"commit 1\" &&\n+\tgit -C full checkout --detach &&\n+\n+\t# Partial clone and push commit to remote\n+\tgit clone \"file://$(pwd)/full\" --filter=blob:none partial &&\n+\techo \"hello\" > partial/foo &&\n+\tgit -C partial commit -a -m \"commit 2\" &&\n+\tgit -C partial push &&\n+\n+\t# gc in partial repo\n+\tgit -C partial gc --prune=now &&\n+\n+\t# Create another commit in normal repo\n+\tgit -C full checkout main &&\n+\techo \" world\" >> full/foo &&\n+\tgit -C full commit -a -m \"commit 3\" &&\n+\n+\t# Pull from remote in partial repo, and run gc again\n+\tgit -C partial pull &&\n+\tgit -C partial gc --prune=now\n+'\n+\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.47.0.105.g07ac214952-goog\n\n"},{"id":"505897","messageId":"CAG1j3zGiNMbri8rZNaF0w+yP+6OdMz0T8+8_Wgd1R_p1HzVasg@mail.gmail.com","threadId":"61890","inReplyTo":"cover.1729549127.git.jonathantanmy@google.com","subject":"Re: [External] [WIP 0/3] Repack on fetch","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-10-23T07:00:31Z","receivedAt":"2024-10-23T07:00:43Z","isPatch":false,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"On Tue, Oct 22, 2024 at 6:29 AM Jonathan Tan wrote:\n> As you can see from the patches, some polishing still needs to be\n> done, but I'm sending them out now to check if other people have\n> opinions about the solution. In particular, Han Young reported that an\n> alternative solution (repack on GC) takes too long [1], so I would be\n> interested to see if the time taken by this solution is good enough for\n> Han Young's use case.\n\nThanks, I've tested the patches on our internal repos, the fetching time\nincrease isn't noticeable.\n"},{"id":"505952","messageId":"20241023170351.2939502-1-jonathantanmy@google.com","threadId":"61890","inReplyTo":"CAG1j3zGiNMbri8rZNaF0w+yP+6OdMz0T8+8_Wgd1R_p1HzVasg@mail.gmail.com","subject":"Re: [External] [WIP 0/3] Repack on fetch","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-23T17:03:50Z","receivedAt":"2024-10-23T17:03:55Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Han Young writes:\n> On Tue, Oct 22, 2024 at 6:29 AM Jonathan Tan wrote:\n> > As you can see from the patches, some polishing still needs to be\n> > done, but I'm sending them out now to check if other people have\n> > opinions about the solution. In particular, Han Young reported that an\n> > alternative solution (repack on GC) takes too long [1], so I would be\n> > interested to see if the time taken by this solution is good enough for\n> > Han Young's use case.\n> \n> Thanks, I've tested the patches on our internal repos, the fetching time\n> increase isn't noticeable.\n\nAh, thanks. I'm looking into why the tests are failing now - I have a\nsolution for t0410, but am still looking into the other two. I'll send\nout non-WIP patches once I have them.\n"}]}