mbox series

[0/1] Fix bug in revision walk if --exclude-promisor-objects is set

Message ID 20240719093435.69238-1-hanyang.tony@bytedance.com (mailing list archive)
Headers show
Series Fix bug in revision walk if --exclude-promisor-objects is set | expand

Message

Han Young July 19, 2024, 9:34 a.m. UTC
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".

Here are the minimal steps to recreate issue.
  # create a normal git repo, add one file and push to remote
  mkdir full && cd full && git init && touch foo && git add foo && git commit -m "commit 1" && git push

  # partial clone a copy of the repo we just created
  cd .. && git clone git@example.org:example/foo.git --filter=blob:none partial

  # create a commit in partial cloned repo and push it to remote
  cd partial && echo 'hello' > foo && git commit -a -m "commit 2" && git push

  # run gc in partial repo
  git gc --prune=now

  # in normal git repo, create another commit on top of the commit we created in partial repo
  cd ../full && git pull && echo ' world' >> foo && git commit -a -m "commit 3" && git push

  # pull from remote in partial repo, and run gc again
  cd ../partial && git pull && git gc --prune=now

The last `git gc` will error out on fsck with error message like this:
  error: Could not read d3fbfea9e448461c2b72a79a95a220ae10defd94
  error: Could not read d3fbfea9e448461c2b72a79a95a220ae10defd94

Note 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.

The 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.

Han Young (1):
  revision: don't set parents as uninteresting if exclude promisor

 revision.c               |  2 +-
 t/t0410-partial-clone.sh | 22 +++++++++++++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)

Comments

Junio C Hamano July 19, 2024, 3:07 p.m. UTC | #1
Han Young <hanyang.tony@bytedance.com> writes:

> 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".

Please wrap overly long lines.  Unless they are wrapped down to
something that can be quoted in e-mail responses a few times and
still comfortably fit on a terminal line, e.g. 68-72 columns, they
are just unreadable mess.

Thanks.
Han Young July 20, 2024, 5:28 a.m. UTC | #2
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: xxx".

Here are the minimal steps to recreate issue.
  # create a normal git repo, add one file and push to remote
  mkdir full && cd full && git init && touch foo
  git add foo && git commit -m "commit 1" && git push

  # partial clone a copy of the repo we just created
  cd ..
  git clone git@example.org:example/foo.git --filter=blob:none partial

  # create a commit in partial cloned repo and push it to remote
  cd partial && echo 'hello' > foo && git commit -a -m "commit 2"
  git push

  # run gc in partial repo
  git gc --prune=now

  # in normal git repo, create another commit on top of the
  # commit we created in partial repo
  cd ../full && git pull && echo ' world' >> foo
  git commit -a -m "commit 3" && git push

  # pull from remote in partial repo, and run gc again
  cd ../partial && git pull && git gc --prune=now

The last `git gc` will error out on fsck with error message like this:
  error: Could not read d3fbfea9e448461c2b72a79a95a220ae10defd94
  error: Could not read d3fbfea9e448461c2b72a79a95a220ae10defd94

Note 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.

The 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.

Han Young (1):
  revision: don't set parents as uninteresting if exclude promisor

 revision.c               |  2 +-
 t/t0410-partial-clone.sh | 22 +++++++++++++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)