{"thread":{"id":"61810","subject":"[PATCH 0/1] Fix bug in revision walk if --exclude-promisor-objects is set","startedAt":"2024-07-19T09:34:43Z","lastAt":"2024-07-20T05:28:56Z","messageCount":5,"participants":["Han Young","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"498958","messageId":"20240719093435.69238-1-hanyang.tony@bytedance.com","threadId":"61810","inReplyTo":null,"subject":"[PATCH 0/1] Fix bug in revision walk if --exclude-promisor-objects is set","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-07-19T09:34:34Z","receivedAt":"2024-07-19T09:34:43Z","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. After a while we started getting reports from engineers complaining that their local repository was broken. Upon further investigation, we found that broken repositories are missing objects that created in that particular local repository. `git fsck` reports \"bad object: xxxx\".\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 && git add foo && git commit -m \"commit 1\" && git push\n\n  # partial clone a copy of the repo we just created\n  cd .. && 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\" && 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 commit we created in partial repo\n  cd ../full && git pull && echo ' world' >> foo && 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  error: Could not read d3fbfea9e448461c2b72a79a95a220ae10defd94\n  error: Could not read d3fbfea9e448461c2b72a79a95a220ae10defd94\n\nNote that disabling commit graph on the partial repo will cause`git gc` to exit normally, but will still not solve the underlying problem. And in more complex situations, disabling commit graph will not avoid the error.\n\nThe problem is caused by the wrong result returned by setup_revision with `--exclude-promisor-objects` enabled. `git gc` will call `git repack`, which will call `git pack-objects` twice on a partially cloned repo. The first call to pack-objects combines all the promisor packfiles, and the second pack-objects command packs all reachable non-promisor objects into a normal packfile. However, a bug in setup_revision caused some non-promisor objects to be mistakenly marked as in promisor packfiles in the second call to pack-objects. These incorrectly marked objects are never repacked, and were deleted from the object store as a result. In revision.c, `process_parents()` recursively marks commit parents as UNINTERESTING if the commit itself is UNINTERESTING. `--exclude-promisor-objects` is implemented as \"iterate all objects in promisor packfiles, mark them as UNINTERESTING\". So when we find a commit object in a promisor packfile, we also set its ancestors as UNINTERESTING, whether the ancestor is a promisor object or not. In the example above, \"commit 2\" is a normal commit object, living in a normal packfile, but 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":"498959","messageId":"20240719093435.69238-2-hanyang.tony@bytedance.com","threadId":"61810","inReplyTo":"20240719093435.69238-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-07-19T09:34:35Z","receivedAt":"2024-07-19T09:34:46Z","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 <hanyang.tony@bytedance.com>\nHelped-by: C O Xing Xin <xingxin.xx@bytedance.com>\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":"498967","messageId":"xmqq7cdhl8tk.fsf@gitster.g","threadId":"61810","inReplyTo":"20240719093435.69238-1-hanyang.tony@bytedance.com","subject":"Re: [PATCH 0/1] Fix bug in revision walk if --exclude-promisor-objects is set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-19T15:07:03Z","receivedAt":"2024-07-19T15:07:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Young <hanyang.tony@bytedance.com> writes:\n\n> We use --filter=blob:none to clone our large monorepo. After a while we started getting reports from engineers complaining that their local repository was broken. Upon further investigation, we found that broken repositories are missing objects that created in that particular local repository. `git fsck` reports \"bad object: xxxx\".\n\nPlease wrap overly long lines.  Unless they are wrapped down to\nsomething that can be quoted in e-mail responses a few times and\nstill comfortably fit on a terminal line, e.g. 68-72 columns, they\nare just unreadable mess.\n\nThanks.\n"},{"id":"498988","messageId":"20240720052845.89111-1-hanyang.tony@bytedance.com","threadId":"61810","inReplyTo":"20240719093435.69238-1-hanyang.tony@bytedance.com","subject":"[PATCH 0/1] Fix bug in revision walk if --exclude-promisor-objects is set","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-07-20T05:28:44Z","receivedAt":"2024-07-20T05:28:53Z","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  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":"498989","messageId":"20240720052845.89111-2-hanyang.tony@bytedance.com","threadId":"61810","inReplyTo":"20240720052845.89111-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-07-20T05:28:45Z","receivedAt":"2024-07-20T05:28:56Z","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 <hanyang.tony@bytedance.com>\nHelped-by: C O Xing Xin <xingxin.xx@bytedance.com>\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"}]}