[RFC PATCH] promisor-remote: always JIT fetch with --refetch

[RFC PATCH] promisor-remote: always JIT fetch with --refetch

[Emily Shaffer]

By the time we decide we need to do a partial clone fetch, we already
know the object is missing, even if the_repository->parsed_objects
thinks it exists. But --refetch bypasses the local object check, so we
can guarantee that a JIT fetch will fix incorrect local caching.

This manifested at $DAYJOB in a repo with the following features:
 * blob-filtered partial clone enabled
 * commit graph enabled
 * ref Foo pointing to commit object 6aaaca
 * object 6aaaca missing[a]

With these prerequisites, we noticed that `git fetch` in the repo
produced an infinite loop:
1. `git fetch` tries to fetch, but thinks it has all objects, so it
   noops.
2. At the end of cmd_fetch(), we try to write_commit_graph_reachable().
3. write_commit_graph_reachable() does a reachability walk, including
   starting from Foo
4. The reachability walk tries to peel Foo, and notices it's missing
   6aaaca.
5. The partial clone machinery asks for a per-object JIT fetch of
   6aaaca.
6. `git fetch` (child process) is asked to fetch 6aaaca.
7. We put together the_repository->parsed_objects, adding all commit IDs
   reachable from local refs to it
   (fetch-pack.c:mark_complete_and_common_refs(), trace region
   mark_complete_local_refs). We see Foo, so we add 6aaaca to
   the_repository->parsed_objects and mark it as COMPLETE.
8. cmd_fetch notices that the object ID it was asked for is already
   known, so does not fetch anything new.
9. GOTO 2.

The culprit is that we're assuming all local refs already must have
objects in place. Using --refetch means we ignore that assumption during
JIT fetch.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>

---

There are a few alternative approaches for this issue that I talked
about with some folks at $DAYJOB:

i. Just disabling the commit graph rewrite allows this to fall
eventually into a path where the fetch actually succeeds. I didn't like
this solution - it's just whack-a-mole - so I didn't look too hard into
why it succeeds that way. It *could* make sense to disable commit graph
rewrite when we do a JIT fetch with blob or tree filter provided - but
if later we want to implement commit filter (something we've talked
about at Google) then I'd worry about this situation coming up again.

ii. We could decide not to mark local refs (and commits reachable from
them) as COMPLETE in the_repository->parsed_objects. I didn't try this
solution out, and I'm not sure what the performance implications are,
but Jonathan Tan likes this solution, so I may try it out and see what
breaks shortly.

iii. We could do all the JIT fetches with --refetch. In my opinion, this
is the safest/most self-healing solution; the JIT fetch only happens
when we really know we're missing the object, so it doesn't make sense
for that fetch to be canceled by any cache. It doesn't have performance
implications as far as I can guess (except that I think we still build
the parsed_objects hash even though we are going to ignore it, but we
already were doing that anyway). Of course, that's what this patch does.

iv. We could do nothing; when cmd_fetch gets a fetch-by-object-id but
decides there is nothing more to do, it could terminate with an error.
That should stop the infinite recursion, and the error could suggest the
user to run `git fsck` and discover what the problem is. Depending on
the remediation we suggest, though, I think a direct fetch to fix this
particular loop would not work.

I'm curious to hear thoughts from people who are more expert than me on
partial clone and fetching in general, though.

This change is also still in RFC, for two reasons:

First, it's intermittently failing tests for me locally, in weirdly
flaky ways:

- t0410-partial-clone.sh fails when I run it from prove, but passes when
  I run it manually, every time.
- t5601-clone.sh and t5505-remote.sh fail nonsensically on `rm -rf` that
  should succeed (and does succeed if I stop the test with test_pause),
  which makes me think there's something else borked in my setup, but
  I'm not sure what.
- t5616-partial-clone.sh actually does fail in a way that I could see
  having to do with this change (since I guess we might download more
  packs than usual), but I was so confused by the other two errors I
  haven't looked closely yet.

And secondly, I didn't write tests verifying the breakage and that this
change fixes it yet, either.

I'm going to work on both those things in the background, but I wanted
to get the description and RFC out early so that folks could take a look
and we could decide which approach is best.

Thanks,
 - Emily

a: That commit object went missing as a byproduct of this partial clone
   gc issue that Calvin, Jonathan, Han Young, and others have been
   investigating:
   https://lore.kernel.org/git/20241001191811.1934900-1-calvinwan@google.com/
---
 promisor-remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/promisor-remote.c b/promisor-remote.c
index 9345ae3db2..cf00e31d3b 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -43,7 +43,7 @@ static int fetch_objects(struct repository *repo,
 	strvec_pushl(&child.args, "-c", "fetch.negotiationAlgorithm=noop",
 		     "fetch", remote_name, "--no-tags",
 		     "--no-write-fetch-head", "--recurse-submodules=no",
-		     "--filter=blob:none", "--stdin", NULL);
+		     "--filter=blob:none", "--refetch", "--stdin", NULL);
 	if (!git_config_get_bool("promisor.quiet", &quiet) && quiet)
 		strvec_push(&child.args, "--quiet");
 	if (start_command(&child))
-- 
2.47.0.rc0.187.ge670bccf7e-goog

Re: [RFC PATCH] promisor-remote: always JIT fetch with --refetch

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> By the time we decide we need to do a partial clone fetch, we already
> know the object is missing, even if the_repository->parsed_objects
> thinks it exists. But --refetch bypasses the local object check, so we
> can guarantee that a JIT fetch will fix incorrect local caching.
> ...
> The culprit is that we're assuming all local refs already must have
> objects in place. Using --refetch means we ignore that assumption during
> JIT fetch.

Hmph.  The whole lazy fetch business looks more and more broken X-<.
There is a comment in the refetch code path that tells us to "perform
a full refetch ignoring existing objects", but if an object truly
exists, there should be no need to refetch, and it starts to smell
more like "ignoring somebody who gives us an incorrect information
that these objects exist".

But a ref that points at a missing commit is "somebody giving a
false information" and an option to ignore such misinformation would
be a perfect tool fit to sweep such a breakage under the rug.

But is this sufficient?  Looking at how check_exist_and_connected()
does its work, I am not sure how it would cope with a case where an
object that is pointed by a ref does happen to exist, but the commit
that is referred to by the commit is missing, as it only checks the
existence of the tips.

> diff --git a/promisor-remote.c b/promisor-remote.c
> index 9345ae3db2..cf00e31d3b 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -43,7 +43,7 @@ static int fetch_objects(struct repository *repo,
>  	strvec_pushl(&child.args, "-c", "fetch.negotiationAlgorithm=noop",
>  		     "fetch", remote_name, "--no-tags",
>  		     "--no-write-fetch-head", "--recurse-submodules=no",
> -		     "--filter=blob:none", "--stdin", NULL);
> +		     "--filter=blob:none", "--refetch", "--stdin", NULL);
>  	if (!git_config_get_bool("promisor.quiet", &quiet) && quiet)
>  		strvec_push(&child.args, "--quiet");
>  	if (start_command(&child))

The documentation for "git fetch --refetch" says that this grabs
everything as if we are making a fresh clone, ignoring everything we
already have.  Which makes the change in this patch prohibitively
expensive for asking each single object lazily from the promisor
remote, but is that really the case?  If there is a reasonable
safety that prevents us from doing something silly like transferring
one clone worth of data for every single object we lazily fetch,
perhaps this would be a workable solution (but if that is the case,
perhaps "git fetch --refetch" documentation needs to be rephrased,
to avoid such an impression).

Thanks.

Re: [RFC PATCH] promisor-remote: always JIT fetch with --refetch

[Robert Coup]

Hi Emily,

I was the one who originally implemented --refetch in [1][2]

[1] https://lore.kernel.org/git/pull.1138.v4.git.1648476131.gitgitgadget@gmail.com/
[2] https://github.com/gitgitgadget/git/pull/1138

On Sun, 6 Oct 2024 at 23:43, Junio C Hamano <gitster@pobox.com> wrote:
>
> Hmph.  The whole lazy fetch business looks more and more broken X-<.
> There is a comment in the refetch code path that tells us to "perform
> a full refetch ignoring existing objects", but if an object truly
> exists, there should be no need to refetch, and it starts to smell
> more like "ignoring somebody who gives us an incorrect information
> that these objects exist".

Basically --refetch was originally designed to send no 'have's during a fetch,
the original motivation being changing a partial clone filter and fetching
all the newly-applicable trees & blobs in a single transfer.

> The documentation for "git fetch --refetch" says that this grabs
> everything as if we are making a fresh clone, ignoring everything we
> already have.  Which makes the change in this patch prohibitively
> expensive for asking each single object lazily from the promisor
> remote, but is that really the case?

From a very quick re-review this is correct that it's expensive: refetch sends
no 'have's, so if you pass a single commit oid then it'll fetch all ancestors
and all the dependent trees & blobs, duplicating what's in the object store and
relying on a repack to clean up. If a commit is missing that's one way to fix
it, but it's a pretty nuclear option: feels like your option iv (terminate with
an error) leading to fsck invoking/suggesting --refetch might avoid
unintentionally recloning the entire repo.

In my original RFC [3], Jonathan Tan suggested that --refetch could be useful
to repair missing objects like this, but it was out of scope for me at the time.
But maybe there's a way to improve it for this sort of case?

[3] https://lore.kernel.org/git/20220202185957.1928631-1-jonathantanmy@google.com/

> Emily Shaffer <emilyshaffer@google.com> writes:
>
> This manifested at $DAYJOB in a repo with the following features:
> * blob-filtered partial clone enabled
> * commit graph enabled
> * ref Foo pointing to commit object 6aaaca
> * object 6aaaca missing[a]

I presume there wasn't an obvious/related cause for commit 6aaaca to go missing
in the first place?

Thanks,

Rob :)

Re: [RFC PATCH] promisor-remote: always JIT fetch with --refetch

[Junio C Hamano]

Robert Coup <robert.coup@koordinates.com> writes:

> Basically --refetch was originally designed to send no 'have's during a fetch,
> the original motivation being changing a partial clone filter and fetching
> all the newly-applicable trees & blobs in a single transfer.
> ...
> If a commit is missing that's one way to fix
> it, but it's a pretty nuclear option: feels like your option iv (terminate with
> an error) leading to fsck invoking/suggesting --refetch might avoid
> unintentionally recloning the entire repo.
> ...
> In my original RFC [3], Jonathan Tan suggested that --refetch could be useful
> to repair missing objects like this, but it was out of scope for me at the time.
> But maybe there's a way to improve it for this sort of case?
>
> [3] https://lore.kernel.org/git/20220202185957.1928631-1-jonathantanmy@google.com/

Thanks for your comments on the original story behind that option.

> I presume there wasn't an obvious/related cause for commit 6aaaca to go missing
> in the first place?

Emily had this after the three-dash line


a: That commit object went missing as a byproduct of this partial clone
   gc issue that Calvin, Jonathan, Han Young, and others have been
   investigating:
   https://lore.kernel.org/git/20241001191811.1934900-1-calvinwan@google.com/

IOW, I think how the lossage was caused is well understood by now.

Thanks.

Re: [RFC PATCH] promisor-remote: always JIT fetch with --refetch

[Emily Shaffer]

Sorry for the slow response/page-context-back-in. I'll be working on
this today and try to send a different approach, but after that point,
I'm not sure when the next time I'll get a chance to work on it may
be. If I don't come up with something suitable today, it's likely that
Jonathan Tan will take over the effort from me, but I'm not sure
around when he'll be able to prioritize it.

On Sun, Oct 6, 2024 at 3:43 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Emily Shaffer <emilyshaffer@google.com> writes:
>
> > By the time we decide we need to do a partial clone fetch, we already
> > know the object is missing, even if the_repository->parsed_objects
> > thinks it exists. But --refetch bypasses the local object check, so we
> > can guarantee that a JIT fetch will fix incorrect local caching.
> > ...
> > The culprit is that we're assuming all local refs already must have
> > objects in place. Using --refetch means we ignore that assumption during
> > JIT fetch.
>
> Hmph.  The whole lazy fetch business looks more and more broken X-<.

By "lazy fetch", are you referring to the partial clone fetch, or are
you referring to the mark_complete stuff? (I know we have been having
lots of issues with the partial clone fetch at Google in the last
month or so, so excuse me disambiguating :) )

> There is a comment in the refetch code path that tells us to "perform
> a full refetch ignoring existing objects", but if an object truly
> exists, there should be no need to refetch, and it starts to smell
> more like "ignoring somebody who gives us an incorrect information
> that these objects exist".
>
> But a ref that points at a missing commit is "somebody giving a
> false information" and an option to ignore such misinformation would
> be a perfect tool fit to sweep such a breakage under the rug.
>
> But is this sufficient?  Looking at how check_exist_and_connected()
> does its work, I am not sure how it would cope with a case where an
> object that is pointed by a ref does happen to exist, but the commit
> that is referred to by the commit is missing, as it only checks the
> existence of the tips.

Is that so? mark_complete_and_common claims that it recurses through
all parents of all local refs and marks them existing, too. Looks like
it does that in fetch-pack.c:mark_recent_complete_commits(), only up
to a certain date cutoff, and doesn't do that at all if there's no
cutoff provided. I don't think I see anywhere else that it's recursing
over parents, so I'm not sure why the comment says that. In fact, I
sort of wonder if the comment is wrong; it was introduced in this[1]
series much later than this code block has existed. But then, nobody
questioned it during the series, so I can also be misreading the code
:)

>
> > diff --git a/promisor-remote.c b/promisor-remote.c
> > index 9345ae3db2..cf00e31d3b 100644
> > --- a/promisor-remote.c
> > +++ b/promisor-remote.c
> > @@ -43,7 +43,7 @@ static int fetch_objects(struct repository *repo,
> >       strvec_pushl(&child.args, "-c", "fetch.negotiationAlgorithm=noop",
> >                    "fetch", remote_name, "--no-tags",
> >                    "--no-write-fetch-head", "--recurse-submodules=no",
> > -                  "--filter=blob:none", "--stdin", NULL);
> > +                  "--filter=blob:none", "--refetch", "--stdin", NULL);
> >       if (!git_config_get_bool("promisor.quiet", &quiet) && quiet)
> >               strvec_push(&child.args, "--quiet");
> >       if (start_command(&child))
>
> The documentation for "git fetch --refetch" says that this grabs
> everything as if we are making a fresh clone, ignoring everything we
> already have.  Which makes the change in this patch prohibitively
> expensive for asking each single object lazily from the promisor
> remote, but is that really the case?  If there is a reasonable
> safety that prevents us from doing something silly like transferring
> one clone worth of data for every single object we lazily fetch,
> perhaps this would be a workable solution (but if that is the case,
> perhaps "git fetch --refetch" documentation needs to be rephrased,
> to avoid such an impression).

Yeah, this is on me for not reading the entire documentation, just
noticing in code that it disabled this COMPLETE cache thingie. You're
right that it would be too expensive to use this way. As I said at the
top, I'll try to send one of the other alternative approaches today.

 - Emily

1: https://lore.kernel.org/git/pull.451.git.1572981981.gitgitgadget@gmail.com/

Re: [RFC PATCH] promisor-remote: always JIT fetch with --refetch

[Junio C Hamano]

Emily Shaffer <nasamuffin@google.com> writes:

> By "lazy fetch", are you referring to the partial clone fetch, or are

Anything we do to compensate for the fact that that initial clone or
fetch can be told to deliberately omit objects, in the hope that we
can grab missing objects on demand.

> Yeah, this is on me for not reading the entire documentation, just
> noticing in code that it disabled this COMPLETE cache thingie. You're
> right that it would be too expensive to use this way. As I said at the
> top, I'll try to send one of the other alternative approaches today.

I thought you were already on your vacation ;-).  I'll go offline
end of this week and won't be back until the end of the month.

[PATCH v2] fetch-pack: don't mark COMPLETE unless we have the full object

[Emily Shaffer]

When fetching, we decide which objects to skip asking for marking
certain commit IDs with the COMPLETE flag. This flag is set in
fetch-pack.c:mark_complete(), which is called from a few different
functions who decide what to mark or not mark. mark_complete() is
insulated against null pointer deref and repeatedly writing the same
commit to the list of objects to skip; because it's the central function
which decides that an object is COMPLETE and doesn't need to be fetched,
let's also insulate it against corruption where the object is not
present (even though we think it is).

Without this check, it's possible to reach a corrupted state where
fetches can infinitely loop because we decide to skip fetching, but we
don't actually have the skipped commit in our object store.

This manifested at $DAYJOB in a repo with the following features:
 * blob-filtered partial clone enabled
 * commit graph enabled
 * ref Foo pointing to commit object 6aaaca
 * object 6aaaca missing[1]

With these prerequisites, we noticed that `git fetch` in the repo
produced an infinite loop:
1. `git fetch` tries to fetch, but thinks it has all objects, so it
   noops.
2. At the end of cmd_fetch(), we try to write_commit_graph_reachable().
3. write_commit_graph_reachable() does a reachability walk, including
   starting from Foo
4. The reachability walk tries to peel Foo, and notices it's missing
   6aaaca.
5. The partial clone machinery asks for a per-object JIT fetch of
   6aaaca.
6. `git fetch` (child process) is asked to fetch 6aaaca.
7. We put together the_repository->parsed_objects, adding all commit IDs
   reachable from local refs to it
   (fetch-pack.c:mark_complete_and_common_refs(), trace region
   mark_complete_local_refs). We see Foo, so we add 6aaaca to
   the_repository->parsed_objects and mark it as COMPLETE.
8. cmd_fetch notices that the object ID it was asked for is already
   known, so does not fetch anything new.
9. GOTO 2.

The culprit is that we're assuming all local refs already must have
objects in place. Let's not assume that, and explicitly check
has_object() before marking objects as COMPLETE.

NEEDSWORK: It could be valuable to emit a trace event when we try to
mark_complete() an object where we don't actually has_object() (to
understand how often we're having to heal from corruption).

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Helped-by: Jonathan Tan <jonathantanmy@google.com>

1: That commit object went missing as a byproduct of this partial clone
   gc issue:
   https://lore.kernel.org/git/20241001191811.1934900-1-calvinwan@google.com/

---


On the list, Junio and Robert Coup suggested that we should notice cases
when the list of object IDs we are trying to fetch gets filtered to
nothing during a lazy fetch. However, this turned out to be quite tricky
to implement - fetch-by-OID stores the OID in a ref object, so it's
challenging to guess that the ref we care about is an OID instead of a
normal ref. It also required some heuristic approach to catch the exact
moment after we removed COMPLETE objects from the list we wanted to
fetch, and to notice that the wrong objects were missing from that list.

(From the list of alternatives I included with v1, v1 was approach iii;
Junio and Robert suggested iv; this patch holds something close to ii.)

Jonathan Tan suggested that instead, we could be more careful about what
we mark COMPLETE or not; it seems like this is pretty straightforward to
do.

I do wonder if it's a layering violation to do the has_object() check in
mark_complete(), which is otherwise a pretty stupid function; I included
it there because all of the other flavors of mark_complete_* eventually
boil down to mark_complete(), so we wouldn't need to remember to check
for object existence any other time we're trying to do this COMPLETE
marking thing.

Note that I added a test to guarantee this behavior works, but without
commit graph enabled, the test passes both before and after; I guess
maybe it's better to add an explicit regression test for the
combination? But, this test fails with reftable - because we don't have
a way (that I know of) to force-create a ref with a bad ID, to force
this error condition. In the test as written I'm writing to
.git/refs/heads/bar directly; that doesn't work for reftable. But `git
update-ref` is too smart to let me set a ref to garbage. Any tips there
are welcome.

The CI run is at
https://github.com/nasamuffin/git/actions/runs/11470039702 - it seems
the reftable tests are the only things failing.

Also, I am sending this version, but if there are any additional
comments or it requires more changes, please expect Jonathan Tan to take
over driving this patch the rest of the way for me. As previously
stated[2], I'll be OOO after this Friday for most of the rest of the
year; the rest of this week I'm trying to get the rest of my
non-upstream loose ends tied up, so I won't have time to do another
iteration. See folks around Christmastime :)

 - Emily

2: https://lore.kernel.org/git/CAJoAoZnovapqMcu72DGR40jRRqRn57uJVTJg82kZ_rohtGDSfQ@mail.gmail.com/

Cover letter from v1 follows:

There are a few alternative approaches for this issue that I talked
about with some folks at $DAYJOB:

i. Just disabling the commit graph rewrite allows this to fall
eventually into a path where the fetch actually succeeds. I didn't like
this solution - it's just whack-a-mole - so I didn't look too hard into
why it succeeds that way. It *could* make sense to disable commit graph
rewrite when we do a JIT fetch with blob or tree filter provided - but
if later we want to implement commit filter (something we've talked
about at Google) then I'd worry about this situation coming up again.

ii. We could decide not to mark local refs (and commits reachable from
them) as COMPLETE in the_repository->parsed_objects. I didn't try this
solution out, and I'm not sure what the performance implications are,
but Jonathan Tan likes this solution, so I may try it out and see what
breaks shortly.

iii. We could do all the JIT fetches with --refetch. In my opinion, this
is the safest/most self-healing solution; the JIT fetch only happens
when we really know we're missing the object, so it doesn't make sense
for that fetch to be canceled by any cache. It doesn't have performance
implications as far as I can guess (except that I think we still build
the parsed_objects hash even though we are going to ignore it, but we
already were doing that anyway). Of course, that's what this patch does.

iv. We could do nothing; when cmd_fetch gets a fetch-by-object-id but
decides there is nothing more to do, it could terminate with an error.
That should stop the infinite recursion, and the error could suggest the
user to run `git fsck` and discover what the problem is. Depending on
the remediation we suggest, though, I think a direct fetch to fix this
particular loop would not work.

I'm curious to hear thoughts from people who are more expert than me on
partial clone and fetching in general, though.

This change is also still in RFC, for two reasons:

First, it's intermittently failing tests for me locally, in weirdly
flaky ways:

- t0410-partial-clone.sh fails when I run it from prove, but passes when
  I run it manually, every time.
- t5601-clone.sh and t5505-remote.sh fail nonsensically on `rm -rf` that
  should succeed (and does succeed if I stop the test with test_pause),
  which makes me think there's something else borked in my setup, but
  I'm not sure what.
- t5616-partial-clone.sh actually does fail in a way that I could see
  having to do with this change (since I guess we might download more
  packs than usual), but I was so confused by the other two errors I
  haven't looked closely yet.

And secondly, I didn't write tests verifying the breakage and that this
change fixes it yet, either.

I'm going to work on both those things in the background, but I wanted
to get the description and RFC out early so that folks could take a look
and we could decide which approach is best.

Thanks,
 - Emily
---
 fetch-pack.c             |  4 +++-
 t/t0410-partial-clone.sh | 30 ++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index f752da93a8..8cb2ce4c54 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -603,7 +603,9 @@ static int mark_complete(const struct object_id *oid)
 {
 	struct commit *commit = deref_without_lazy_fetch(oid, 1);
 
-	if (commit && !(commit->object.flags & COMPLETE)) {
+	if (commit &&
+	    !(commit->object.flags & COMPLETE) &&
+	    has_object(the_repository, oid, 0)) {
 		commit->object.flags |= COMPLETE;
 		commit_list_insert(commit, &complete);
 	}
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 818700fbec..95de18ec40 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -241,6 +241,36 @@ test_expect_success 'fetching of missing objects works with ref-in-want enabled'
 	grep "fetch< fetch=.*ref-in-want" trace
 '
 
+test_expect_success 'fetching missing objects pointed to by a local ref' '
+	rm -rf reliable-server unreliable-client &&
+	test_when_finished rm -rf reliable-server unreliable-client &&
+	test_create_repo reliable-server &&
+	git -C reliable-server config uploadpack.allowanysha1inwant 1 &&
+	git -C reliable-server config uploadpack.allowfilter 1 &&
+	test_commit -C reliable-server foo &&
+
+	git clone --filter=blob:none "file://$(pwd)/reliable-server" unreliable-client &&
+
+	# to simulate the unreliable client losing a referenced object by
+	# corruption, create the object on the server side, then create only a
+	# reference to that object on the client side (without providing the
+	# object itself).
+	test_commit -C reliable-server bar &&
+	HASH=$(git -C reliable-server rev-parse HEAD) &&
+	echo "$HASH" >unreliable-client/.git/refs/heads/bar &&
+
+	# the object is really missing
+	# check if we can rev-parse a partial SHA. partial so we do not fetch it,
+	# but barely partial (trim only the last char) so that we do not collide
+	test_must_fail git -C unreliable-client rev-parse ${HASH%%?} &&
+
+	# trigger a remote fetch by checking out `bar`
+	git -C unreliable-client switch bar &&
+
+	# and now we have the missing object
+	git -C unreliable-client rev-parse ${HASH%%?}
+'
+
 test_expect_success 'fetching of missing objects from another promisor remote' '
 	git clone "file://$(pwd)/server" server2 &&
 	test_commit -C server2 bar &&

Range-diff against v1:
1:  092be0a655 ! 1:  4db6bbb4cd promisor-remote: always JIT fetch with --refetch
    - ## promisor-remote.c ##
    -@@ promisor-remote.c: static int fetch_objects(struct repository *repo,
    - 	strvec_pushl(&child.args, "-c", "fetch.negotiationAlgorithm=noop",
    - 		     "fetch", remote_name, "--no-tags",
    - 		     "--no-write-fetch-head", "--recurse-submodules=no",
    --		     "--filter=blob:none", "--stdin", NULL);
    -+		     "--filter=blob:none", "--refetch", "--stdin", NULL);
    - 	if (!git_config_get_bool("promisor.quiet", &quiet) && quiet)
    - 		strvec_push(&child.args, "--quiet");
    - 	if (start_command(&child))
    + ## fetch-pack.c ##
    +@@ fetch-pack.c: static int mark_complete(const struct object_id *oid)
    + {
    + 	struct commit *commit = deref_without_lazy_fetch(oid, 1);
    + 
    +-	if (commit && !(commit->object.flags & COMPLETE)) {
    ++	if (commit &&
    ++	    !(commit->object.flags & COMPLETE) &&
    ++	    has_object(the_repository, oid, 0)) {
    + 		commit->object.flags |= COMPLETE;
    + 		commit_list_insert(commit, &complete);
    + 	}
    +
    + ## t/t0410-partial-clone.sh ##
    +@@ t/t0410-partial-clone.sh: test_expect_success 'fetching of missing objects works with ref-in-want enabled'
    + 	grep "fetch< fetch=.*ref-in-want" trace
    + '
    + 
    ++test_expect_success 'fetching missing objects pointed to by a local ref' '
    ++	rm -rf reliable-server unreliable-client &&
    ++	test_when_finished rm -rf reliable-server unreliable-client &&
    ++	test_create_repo reliable-server &&
    ++	git -C reliable-server config uploadpack.allowanysha1inwant 1 &&
    ++	git -C reliable-server config uploadpack.allowfilter 1 &&
    ++	test_commit -C reliable-server foo &&
    ++
    ++	git clone --filter=blob:none "file://$(pwd)/reliable-server" unreliable-client &&
    ++
    ++	# to simulate the unreliable client losing a referenced object by
    ++	# corruption, create the object on the server side, then create only a
    ++	# reference to that object on the client side (without providing the
    ++	# object itself).
    ++	test_commit -C reliable-server bar &&
    ++	HASH=$(git -C reliable-server rev-parse HEAD) &&
    ++	echo "$HASH" >unreliable-client/.git/refs/heads/bar &&
    ++
    ++	# the object is really missing
    ++	# check if we can rev-parse a partial SHA. partial so we do not fetch it,
    ++	# but barely partial (trim only the last char) so that we do not collide
    ++	test_must_fail git -C unreliable-client rev-parse ${HASH%%?} &&
    ++
    ++	# trigger a remote fetch by checking out `bar`
    ++	git -C unreliable-client switch bar &&
    ++
    ++	# and now we have the missing object
    ++	git -C unreliable-client rev-parse ${HASH%%?}
    ++'
    ++
    + test_expect_success 'fetching of missing objects from another promisor remote' '
    + 	git clone "file://$(pwd)/server" server2 &&
    + 	test_commit -C server2 bar &&
-- 
2.47.0.105.g07ac214952-goog

Re: [PATCH v2] fetch-pack: don't mark COMPLETE unless we have the full object

[Emily Shaffer]

(I missed ccing reviewers from last round; adding now, although I know
Junio is vacationing. Sorry about that.)

On Tue, Oct 22, 2024 at 5:28 PM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> When fetching, we decide which objects to skip asking for marking
> certain commit IDs with the COMPLETE flag. This flag is set in
> fetch-pack.c:mark_complete(), which is called from a few different
> functions who decide what to mark or not mark. mark_complete() is
> insulated against null pointer deref and repeatedly writing the same
> commit to the list of objects to skip; because it's the central function
> which decides that an object is COMPLETE and doesn't need to be fetched,
> let's also insulate it against corruption where the object is not
> present (even though we think it is).
>
> Without this check, it's possible to reach a corrupted state where
> fetches can infinitely loop because we decide to skip fetching, but we
> don't actually have the skipped commit in our object store.
>
> This manifested at $DAYJOB in a repo with the following features:
>  * blob-filtered partial clone enabled
>  * commit graph enabled
>  * ref Foo pointing to commit object 6aaaca
>  * object 6aaaca missing[1]
>
> With these prerequisites, we noticed that `git fetch` in the repo
> produced an infinite loop:
> 1. `git fetch` tries to fetch, but thinks it has all objects, so it
>    noops.
> 2. At the end of cmd_fetch(), we try to write_commit_graph_reachable().
> 3. write_commit_graph_reachable() does a reachability walk, including
>    starting from Foo
> 4. The reachability walk tries to peel Foo, and notices it's missing
>    6aaaca.
> 5. The partial clone machinery asks for a per-object JIT fetch of
>    6aaaca.
> 6. `git fetch` (child process) is asked to fetch 6aaaca.
> 7. We put together the_repository->parsed_objects, adding all commit IDs
>    reachable from local refs to it
>    (fetch-pack.c:mark_complete_and_common_refs(), trace region
>    mark_complete_local_refs). We see Foo, so we add 6aaaca to
>    the_repository->parsed_objects and mark it as COMPLETE.
> 8. cmd_fetch notices that the object ID it was asked for is already
>    known, so does not fetch anything new.
> 9. GOTO 2.
>
> The culprit is that we're assuming all local refs already must have
> objects in place. Let's not assume that, and explicitly check
> has_object() before marking objects as COMPLETE.
>
> NEEDSWORK: It could be valuable to emit a trace event when we try to
> mark_complete() an object where we don't actually has_object() (to
> understand how often we're having to heal from corruption).
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> Helped-by: Jonathan Tan <jonathantanmy@google.com>
>
> 1: That commit object went missing as a byproduct of this partial clone
>    gc issue:
>    https://lore.kernel.org/git/20241001191811.1934900-1-calvinwan@google.com/
>
> ---
>
>
> On the list, Junio and Robert Coup suggested that we should notice cases
> when the list of object IDs we are trying to fetch gets filtered to
> nothing during a lazy fetch. However, this turned out to be quite tricky
> to implement - fetch-by-OID stores the OID in a ref object, so it's
> challenging to guess that the ref we care about is an OID instead of a
> normal ref. It also required some heuristic approach to catch the exact
> moment after we removed COMPLETE objects from the list we wanted to
> fetch, and to notice that the wrong objects were missing from that list.
>
> (From the list of alternatives I included with v1, v1 was approach iii;
> Junio and Robert suggested iv; this patch holds something close to ii.)
>
> Jonathan Tan suggested that instead, we could be more careful about what
> we mark COMPLETE or not; it seems like this is pretty straightforward to
> do.
>
> I do wonder if it's a layering violation to do the has_object() check in
> mark_complete(), which is otherwise a pretty stupid function; I included
> it there because all of the other flavors of mark_complete_* eventually
> boil down to mark_complete(), so we wouldn't need to remember to check
> for object existence any other time we're trying to do this COMPLETE
> marking thing.
>
> Note that I added a test to guarantee this behavior works, but without
> commit graph enabled, the test passes both before and after; I guess
> maybe it's better to add an explicit regression test for the
> combination? But, this test fails with reftable - because we don't have
> a way (that I know of) to force-create a ref with a bad ID, to force
> this error condition. In the test as written I'm writing to
> .git/refs/heads/bar directly; that doesn't work for reftable. But `git
> update-ref` is too smart to let me set a ref to garbage. Any tips there
> are welcome.

I keep thinking about this test, and the more I think, the less
valuable I believe it is.

I think we aren't super in the habit of writing regression tests, but
would it be that valuable to write a regression test in this case
instead? On the other hand, I think the code diff is quite obviously a
good idea, and we can see from the test suite that there isn't really
a performance hit from it. Is it necessary to add a test at all?

Or, I guess that we could try to inspect how many fetch attempts were
needed to do the JIT fetch in this test. I suspect the number will be
too high without this patch - I know it recurses at least more than
once. I dunno. Anybody have stronger opinions than me?

>
> The CI run is at
> https://github.com/nasamuffin/git/actions/runs/11470039702 - it seems
> the reftable tests are the only things failing.
>
> Also, I am sending this version, but if there are any additional
> comments or it requires more changes, please expect Jonathan Tan to take
> over driving this patch the rest of the way for me. As previously
> stated[2], I'll be OOO after this Friday for most of the rest of the
> year; the rest of this week I'm trying to get the rest of my
> non-upstream loose ends tied up, so I won't have time to do another
> iteration. See folks around Christmastime :)
>
>  - Emily
>
> 2: https://lore.kernel.org/git/CAJoAoZnovapqMcu72DGR40jRRqRn57uJVTJg82kZ_rohtGDSfQ@mail.gmail.com/
>
> Cover letter from v1 follows:
>
> There are a few alternative approaches for this issue that I talked
> about with some folks at $DAYJOB:
>
> i. Just disabling the commit graph rewrite allows this to fall
> eventually into a path where the fetch actually succeeds. I didn't like
> this solution - it's just whack-a-mole - so I didn't look too hard into
> why it succeeds that way. It *could* make sense to disable commit graph
> rewrite when we do a JIT fetch with blob or tree filter provided - but
> if later we want to implement commit filter (something we've talked
> about at Google) then I'd worry about this situation coming up again.
>
> ii. We could decide not to mark local refs (and commits reachable from
> them) as COMPLETE in the_repository->parsed_objects. I didn't try this
> solution out, and I'm not sure what the performance implications are,
> but Jonathan Tan likes this solution, so I may try it out and see what
> breaks shortly.
>
> iii. We could do all the JIT fetches with --refetch. In my opinion, this
> is the safest/most self-healing solution; the JIT fetch only happens
> when we really know we're missing the object, so it doesn't make sense
> for that fetch to be canceled by any cache. It doesn't have performance
> implications as far as I can guess (except that I think we still build
> the parsed_objects hash even though we are going to ignore it, but we
> already were doing that anyway). Of course, that's what this patch does.
>
> iv. We could do nothing; when cmd_fetch gets a fetch-by-object-id but
> decides there is nothing more to do, it could terminate with an error.
> That should stop the infinite recursion, and the error could suggest the
> user to run `git fsck` and discover what the problem is. Depending on
> the remediation we suggest, though, I think a direct fetch to fix this
> particular loop would not work.
>
> I'm curious to hear thoughts from people who are more expert than me on
> partial clone and fetching in general, though.
>
> This change is also still in RFC, for two reasons:
>
> First, it's intermittently failing tests for me locally, in weirdly
> flaky ways:
>
> - t0410-partial-clone.sh fails when I run it from prove, but passes when
>   I run it manually, every time.
> - t5601-clone.sh and t5505-remote.sh fail nonsensically on `rm -rf` that
>   should succeed (and does succeed if I stop the test with test_pause),
>   which makes me think there's something else borked in my setup, but
>   I'm not sure what.
> - t5616-partial-clone.sh actually does fail in a way that I could see
>   having to do with this change (since I guess we might download more
>   packs than usual), but I was so confused by the other two errors I
>   haven't looked closely yet.
>
> And secondly, I didn't write tests verifying the breakage and that this
> change fixes it yet, either.
>
> I'm going to work on both those things in the background, but I wanted
> to get the description and RFC out early so that folks could take a look
> and we could decide which approach is best.
>
> Thanks,
>  - Emily
> ---
>  fetch-pack.c             |  4 +++-
>  t/t0410-partial-clone.sh | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index f752da93a8..8cb2ce4c54 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -603,7 +603,9 @@ static int mark_complete(const struct object_id *oid)
>  {
>         struct commit *commit = deref_without_lazy_fetch(oid, 1);
>
> -       if (commit && !(commit->object.flags & COMPLETE)) {
> +       if (commit &&
> +           !(commit->object.flags & COMPLETE) &&
> +           has_object(the_repository, oid, 0)) {
>                 commit->object.flags |= COMPLETE;
>                 commit_list_insert(commit, &complete);
>         }
> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index 818700fbec..95de18ec40 100755
> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -241,6 +241,36 @@ test_expect_success 'fetching of missing objects works with ref-in-want enabled'
>         grep "fetch< fetch=.*ref-in-want" trace
>  '
>
> +test_expect_success 'fetching missing objects pointed to by a local ref' '
> +       rm -rf reliable-server unreliable-client &&
> +       test_when_finished rm -rf reliable-server unreliable-client &&
> +       test_create_repo reliable-server &&
> +       git -C reliable-server config uploadpack.allowanysha1inwant 1 &&
> +       git -C reliable-server config uploadpack.allowfilter 1 &&
> +       test_commit -C reliable-server foo &&
> +
> +       git clone --filter=blob:none "file://$(pwd)/reliable-server" unreliable-client &&
> +
> +       # to simulate the unreliable client losing a referenced object by
> +       # corruption, create the object on the server side, then create only a
> +       # reference to that object on the client side (without providing the
> +       # object itself).
> +       test_commit -C reliable-server bar &&
> +       HASH=$(git -C reliable-server rev-parse HEAD) &&
> +       echo "$HASH" >unreliable-client/.git/refs/heads/bar &&
> +
> +       # the object is really missing
> +       # check if we can rev-parse a partial SHA. partial so we do not fetch it,
> +       # but barely partial (trim only the last char) so that we do not collide
> +       test_must_fail git -C unreliable-client rev-parse ${HASH%%?} &&
> +
> +       # trigger a remote fetch by checking out `bar`
> +       git -C unreliable-client switch bar &&
> +
> +       # and now we have the missing object
> +       git -C unreliable-client rev-parse ${HASH%%?}
> +'
> +
>  test_expect_success 'fetching of missing objects from another promisor remote' '
>         git clone "file://$(pwd)/server" server2 &&
>         test_commit -C server2 bar &&
>
> Range-diff against v1:
> 1:  092be0a655 ! 1:  4db6bbb4cd promisor-remote: always JIT fetch with --refetch
>     - ## promisor-remote.c ##
>     -@@ promisor-remote.c: static int fetch_objects(struct repository *repo,
>     -   strvec_pushl(&child.args, "-c", "fetch.negotiationAlgorithm=noop",
>     -                "fetch", remote_name, "--no-tags",
>     -                "--no-write-fetch-head", "--recurse-submodules=no",
>     --               "--filter=blob:none", "--stdin", NULL);
>     -+               "--filter=blob:none", "--refetch", "--stdin", NULL);
>     -   if (!git_config_get_bool("promisor.quiet", &quiet) && quiet)
>     -           strvec_push(&child.args, "--quiet");
>     -   if (start_command(&child))
>     + ## fetch-pack.c ##
>     +@@ fetch-pack.c: static int mark_complete(const struct object_id *oid)
>     + {
>     +   struct commit *commit = deref_without_lazy_fetch(oid, 1);
>     +
>     +-  if (commit && !(commit->object.flags & COMPLETE)) {
>     ++  if (commit &&
>     ++      !(commit->object.flags & COMPLETE) &&
>     ++      has_object(the_repository, oid, 0)) {
>     +           commit->object.flags |= COMPLETE;
>     +           commit_list_insert(commit, &complete);
>     +   }
>     +
>     + ## t/t0410-partial-clone.sh ##
>     +@@ t/t0410-partial-clone.sh: test_expect_success 'fetching of missing objects works with ref-in-want enabled'
>     +   grep "fetch< fetch=.*ref-in-want" trace
>     + '
>     +
>     ++test_expect_success 'fetching missing objects pointed to by a local ref' '
>     ++  rm -rf reliable-server unreliable-client &&
>     ++  test_when_finished rm -rf reliable-server unreliable-client &&
>     ++  test_create_repo reliable-server &&
>     ++  git -C reliable-server config uploadpack.allowanysha1inwant 1 &&
>     ++  git -C reliable-server config uploadpack.allowfilter 1 &&
>     ++  test_commit -C reliable-server foo &&
>     ++
>     ++  git clone --filter=blob:none "file://$(pwd)/reliable-server" unreliable-client &&
>     ++
>     ++  # to simulate the unreliable client losing a referenced object by
>     ++  # corruption, create the object on the server side, then create only a
>     ++  # reference to that object on the client side (without providing the
>     ++  # object itself).
>     ++  test_commit -C reliable-server bar &&
>     ++  HASH=$(git -C reliable-server rev-parse HEAD) &&
>     ++  echo "$HASH" >unreliable-client/.git/refs/heads/bar &&
>     ++
>     ++  # the object is really missing
>     ++  # check if we can rev-parse a partial SHA. partial so we do not fetch it,
>     ++  # but barely partial (trim only the last char) so that we do not collide
>     ++  test_must_fail git -C unreliable-client rev-parse ${HASH%%?} &&
>     ++
>     ++  # trigger a remote fetch by checking out `bar`
>     ++  git -C unreliable-client switch bar &&
>     ++
>     ++  # and now we have the missing object
>     ++  git -C unreliable-client rev-parse ${HASH%%?}
>     ++'
>     ++
>     + test_expect_success 'fetching of missing objects from another promisor remote' '
>     +   git clone "file://$(pwd)/server" server2 &&
>     +   test_commit -C server2 bar &&
> --
> 2.47.0.105.g07ac214952-goog
>

Re: [PATCH v2] fetch-pack: don't mark COMPLETE unless we have the full object

[Taylor Blau]

On Tue, Oct 22, 2024 at 05:28:05PM -0700, Emily Shaffer wrote:
> This change is also still in RFC, for two reasons:
>
> First, it's intermittently failing tests for me locally, in weirdly
> flaky ways:
>
> - t0410-partial-clone.sh fails when I run it from prove, but passes when
>   I run it manually, every time.
> - t5601-clone.sh and t5505-remote.sh fail nonsensically on `rm -rf` that
>   should succeed (and does succeed if I stop the test with test_pause),
>   which makes me think there's something else borked in my setup, but
>   I'm not sure what.
> - t5616-partial-clone.sh actually does fail in a way that I could see
>   having to do with this change (since I guess we might download more
>   packs than usual), but I was so confused by the other two errors I
>   haven't looked closely yet.
>
> And secondly, I didn't write tests verifying the breakage and that this
> change fixes it yet, either.
>
> I'm going to work on both those things in the background, but I wanted
> to get the description and RFC out early so that folks could take a look
> and we could decide which approach is best.

I am a little confused. Here you say that this patch is still in RFC,
but the subject line dropped the RFC present in the first round. What is
the state of this patch's readiness?

Thanks,
Taylor

Re: [PATCH v2] fetch-pack: don't mark COMPLETE unless we have the full object

[Jonathan Tan]

Taylor Blau <me@ttaylorr.com> writes:
> > I'm going to work on both those things in the background, but I wanted
> > to get the description and RFC out early so that folks could take a look
> > and we could decide which approach is best.
> 
> I am a little confused. Here you say that this patch is still in RFC,
> but the subject line dropped the RFC present in the first round. What is
> the state of this patch's readiness?
> 
> Thanks,
> Taylor

As Emily said [1], I'll be taking over driving this patch.

The tl;dr is this patch is not ready, so I think you (the interim
maintainer) can drop it.

This patch strives to avoid marking missing objects as COMPLETE by doing
a check in mark_complete(), but deref_without_lazy_fetch() already makes
such a check (note how it can return NULL; also its name implies that
it knows something about missing objects, namely that it says it won't
lazy fetch them) so the question should be: why doesn't it return NULL
when an object is missing? It turns out that it first checks the commit
graph file and if it's there, then it's considered to be present, so
it does not fetch at all. However there are code paths during commit
graph writing (executed after every fetch, in builtin/fetch.c) that
access the object store directly without going through the commit graph
file (search for "object_info" in commit-graph.c), and those functions
perform lazy fetches when the object is missing. So there's an infinite
loop of "commit graph writer reads X" -> "fetch X" (nothing gets
fetched) -> "write new commit graph, as we always do after a fetch" ->
"commit graph writer reads X" -> ...

So my initial proposal to not mark objects as COMPLETE if they are
missing does not work, because they are already not marked as COMPLETE
if they are missing. One could say that we should check both the commit
graph file and the object store (or, perhaps even better, only the
object store) before stating that an object is present, but I think
that Git already assumes in some places that a commit is present merely
by its presence in the commit graph file, and it's not worth changing
this design.

One solution to fix this is to make the commit graph writer never
lazy-fetch. This closes us off to being able to have missing commits
in a partial clone (at least, if we also want to use the commit graph
file). This might be a reasonable thing to do - at least, partial clone
has been around for a few years and we've not made many concrete steps
towards that - but I feel that we shouldn't close ourselves off if
there's an alternative.

The alternative I'm currently thinking of is to detect if we didn't
fetch any packfiles, and if we didn't, don't write a commit graph
(and don't GC), much like we do when we have --negotiate-only. (A
packfile-less fetch still can cause refs to be rewritten and thus reduce
the number of reachable objects, thus enabling a smaller commit graph to
be written and some objects to be GC-ed, but I think that this situation
still doesn't warrant commit graph writing and/or GC - we can just do
those next time.) The main issue is that we don't always know whether
a pack is written or not - in particular, if we use something other
than "connect" or "stateless-connect" on a remote helper, we won't know
if a packfile was sent. We can solve this by (1) only write the commit
graph and GC if we know for sure that a packfile was sent, or (2) write
the commit graph and GC unless we know for sure that a packfile was
not sent. I'm leaning towards (1) because it seems more conceptually
coherent even though it is a change of behavior (from auto commit graph
and GC to no), because I think that the repos that need scalability
the most already use protocol v2 during fetching (which does require
"connect" or "stateless-connect" from remote helpers, so we're covered
here), but am OK with (2) as well.

Feel free to let me know if you have any ideas. In the meantime I'll
look at (1).

[1] https://lore.kernel.org/git/20241023002806.367082-1-emilyshaffer@google.com/

[PATCH 0/2] When fetching, warn if in commit graph but not obj db

[Jonathan Tan]

I mentioned previously [1] the possibility of not running maintenance
steps (commit graph writing and "git maintenance") if no packs were
fetched, but looking at things again, I think that we shouldn't do
that - in particular, if I ran "git fetch --refetch", I would fully
expect the objects to be repacked, even if Git wasn't able to detect
conclusively whether a pack was transmitted.

So I went back to my original idea of detecting when an object is
missing. In trying to balance the concerns of both doing something as
reasonable as possible in such a repo corruption case, and not slowing
down and/or unnecessarily complicating the main code flow, I decided
to detect when an object is present in the commit graph but not in the
object DB, and to limit this detection for objects specified in the
fetch refspec.

Upon detection, we can't fix it due to reasons mentioned in the commit
message, so I decided to print a warning. An alternate option is to make
it a fatal error (instead of a warning) if an object is detected to be
in the commit graph but not the object DB. I haven't thought through the
ramifications of that, though.

[1] https://lore.kernel.org/git/20241028225504.4151804-1-jonathantanmy@google.com/

Jonathan Tan (2):
  Revert "fetch-pack: add a deref_without_lazy_fetch_extended()"
  fetch-pack: warn if in commit graph but not obj db

 fetch-pack.c | 45 +++++++++++++++++++++++++--------------------
 object.h     |  2 +-
 2 files changed, 26 insertions(+), 21 deletions(-)

-- 
2.47.0.163.g1226f6d8fa-goog

[PATCH 1/2] Revert "fetch-pack: add a deref_without_lazy_fetch_extended()"

[Jonathan Tan]

This reverts commit a6e65fb39caf18259c660c1c7910d5bf80bc15cb.

The commit message of that commit mentions that the new function "will
be used for the bundle-uri client in a subsequent commit", but it seems
that eventually it wasn't used.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 fetch-pack.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index f752da93a8..6728a0d2f5 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -122,12 +122,11 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,
 		cb(negotiator, cache.items[i]);
 }
 
-static struct commit *deref_without_lazy_fetch_extended(const struct object_id *oid,
-							int mark_tags_complete,
-							enum object_type *type,
-							unsigned int oi_flags)
+static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
+					       int mark_tags_complete)
 {
-	struct object_info info = { .typep = type };
+	enum object_type type;
+	struct object_info info = { .typep = &type };
 	struct commit *commit;
 
 	commit = lookup_commit_in_graph(the_repository, oid);
@@ -136,9 +135,9 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *
 
 	while (1) {
 		if (oid_object_info_extended(the_repository, oid, &info,
-					     oi_flags))
+					     OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK))
 			return NULL;
-		if (*type == OBJ_TAG) {
+		if (type == OBJ_TAG) {
 			struct tag *tag = (struct tag *)
 				parse_object(the_repository, oid);
 
@@ -152,7 +151,7 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *
 		}
 	}
 
-	if (*type == OBJ_COMMIT) {
+	if (type == OBJ_COMMIT) {
 		struct commit *commit = lookup_commit(the_repository, oid);
 		if (!commit || repo_parse_commit(the_repository, commit))
 			return NULL;
@@ -162,16 +161,6 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *
 	return NULL;
 }
 
-
-static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
-					       int mark_tags_complete)
-{
-	enum object_type type;
-	unsigned flags = OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK;
-	return deref_without_lazy_fetch_extended(oid, mark_tags_complete,
-						 &type, flags);
-}
-
 static int rev_list_insert_ref(struct fetch_negotiator *negotiator,
 			       const struct object_id *oid)
 {
-- 
2.47.0.163.g1226f6d8fa-goog

[PATCH 2/2] fetch-pack: warn if in commit graph but not obj db

[Jonathan Tan]

When fetching, there is a step in which sought objects are first checked
against the local repository; only objects that are not in the local
repository are then fetched. This check first looks up the commit graph
file, and returns "present" if the object is in there.

However, the action of first looking up the commit graph file is not
done everywhere in Git, especially if the type of the object at the time
of lookup is not known. This means that in a repo corruption situation,
a user may encounter an "object missing" error, attempt to fetch it, and
still encounter the same error later when they reattempt their original
action, because the object is present in the commit graph file but not in
the object DB.

Therefore, detect when this occurs and print a warning. (Note that
we cannot proceed to include this object in the list of objects to
be fetched without changing at least the fetch negotiation code:
what would happen is that the client will send "want X" and "have X"
and when I tested at $DAYJOB with a work server that uses JGit, the
server reasonably returned an empty packfile. And changing the fetch
negotiation code to only use the object DB when deciding what to report
as "have" would be an unnecessary slowdown, I think.)

This was discovered when a lazy fetch of a missing commit completed with
nothing actually fetched, and the writing of the commit graph file after
every fetch then attempted to read said missing commit, triggering a
lazy fetch of said missing commit, resulting in an infinite loop with no
user-visible indication (until they check the list of processes running
on their computer). With this fix, at least a warning message will be
printed. Note that although the repo corruption we discovered was caused
by a bug in GC in a partial clone, the behavior that this patch teaches
Git to warn about applies to any repo with commit graph enabled and with
a missing commit, whether it is a partial clone or not.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 fetch-pack.c | 22 +++++++++++++++++++---
 object.h     |  2 +-
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 6728a0d2f5..5a0020366b 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -57,6 +57,7 @@ static struct string_list uri_protocols = STRING_LIST_INIT_DUP;
 #define ALTERNATE	(1U << 1)
 #define COMMON		(1U << 6)
 #define REACH_SCRATCH	(1U << 7)
+#define COMPLETE_FROM_COMMIT_GRAPH	(1U << 8)
 
 /*
  * After sending this many "have"s if we do not get any new ACK , we
@@ -123,15 +124,18 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,
 }
 
 static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
-					       int mark_tags_complete)
+					       int mark_additional_complete_information)
 {
 	enum object_type type;
 	struct object_info info = { .typep = &type };
 	struct commit *commit;
 
 	commit = lookup_commit_in_graph(the_repository, oid);
-	if (commit)
+	if (commit) {
+		if (mark_additional_complete_information)
+			commit->object.flags |= COMPLETE_FROM_COMMIT_GRAPH;
 		return commit;
+	}
 
 	while (1) {
 		if (oid_object_info_extended(the_repository, oid, &info,
@@ -143,7 +147,7 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
 
 			if (!tag->tagged)
 				return NULL;
-			if (mark_tags_complete)
+			if (mark_additional_complete_information)
 				tag->object.flags |= COMPLETE;
 			oid = &tag->tagged->oid;
 		} else {
@@ -809,6 +813,14 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,
 	save_commit_buffer = old_save_commit_buffer;
 }
 
+static void warn_in_commit_graph_only(const struct object_id *oid)
+{
+	warning(_("You are attempting to fetch %s, which is in the commit graph file but not in the object database."),
+		oid_to_hex(oid));
+	warning(_("This is probably due to repo corruption."));
+	warning(_("If you are attempting to repair this repo corruption by refetching the missing object, use 'git fetch --refetch' with the missing object."));
+}
+
 /*
  * Returns 1 if every object pointed to by the given remote refs is available
  * locally and reachable from a local ref, and 0 otherwise.
@@ -830,6 +842,10 @@ static int everything_local(struct fetch_pack_args *args,
 				      ref->name);
 			continue;
 		}
+		if (o->flags & COMPLETE_FROM_COMMIT_GRAPH) {
+			if (!has_object(the_repository, remote, 0))
+				warn_in_commit_graph_only(remote);
+		}
 		print_verbose(args, _("already have %s (%s)"), oid_to_hex(remote),
 			      ref->name);
 	}
diff --git a/object.h b/object.h
index 17f32f1103..196e489253 100644
--- a/object.h
+++ b/object.h
@@ -65,7 +65,7 @@ void object_array_init(struct object_array *array);
 /*
  * object flag allocation:
  * revision.h:               0---------10         15               23------27
- * fetch-pack.c:             01    67
+ * fetch-pack.c:             01    6-8
  * negotiator/default.c:       2--5
  * walker.c:                 0-2
  * upload-pack.c:                4       11-----14  16-----19
-- 
2.47.0.163.g1226f6d8fa-goog

Re: [PATCH 0/2] When fetching, warn if in commit graph but not obj db

[Josh Steadmon]

On 2024.10.29 14:11, Jonathan Tan wrote:
> I mentioned previously [1] the possibility of not running maintenance
> steps (commit graph writing and "git maintenance") if no packs were
> fetched, but looking at things again, I think that we shouldn't do
> that - in particular, if I ran "git fetch --refetch", I would fully
> expect the objects to be repacked, even if Git wasn't able to detect
> conclusively whether a pack was transmitted.
> 
> [1] https://lore.kernel.org/git/20241028225504.4151804-1-jonathantanmy@google.com/

A note for upstream, because I'm not sure it was ever explicitly
mentioned: at $DAYJOB, we saw this fetch recursion error as a
side-effect of the erroneous GC of local commits discussed at [2].

[2] https://lore.kernel.org/git/cover.1729792911.git.jonathantanmy@google.com/

> So I went back to my original idea of detecting when an object is
> missing. In trying to balance the concerns of both doing something as
> reasonable as possible in such a repo corruption case, and not slowing
> down and/or unnecessarily complicating the main code flow, I decided
> to detect when an object is present in the commit graph but not in the
> object DB, and to limit this detection for objects specified in the
> fetch refspec.
> 
> Upon detection, we can't fix it due to reasons mentioned in the commit
> message, so I decided to print a warning. An alternate option is to make
> it a fatal error (instead of a warning) if an object is detected to be
> in the commit graph but not the object DB. I haven't thought through the
> ramifications of that, though.

At first glance, I lean towards making this a fatal error, but I'll try
thinking out loud a bit:

First, we believe that [2] above should fix the root cause of the
particular case we saw at $DAYJOB (hopefully this type of error doesn't
have multiple root causes). So we expect to basically never encounter
this error again after [2] is merged and rolled out, and all existing
cases of repo corruption have been repaired. However, interacting with a
broken repo even with a client that includes [2] would still hit this
condition and issue a warning.

With the current implementation, fetching in a corrupt repo would still
cause git-fetch to infinitely recurse, and therefore would repeatedly
print the same error message to the console, until either the user
noticed, or we fail to launch a new git-fetch process due to resource
exhaustion.

I don't see any reason why the above situation is more friendly or
desirable than exiting (with the same error message) as soon as we
detect this type of corruption. However, I don't feel super strongly
about it. If the rest of the list is OK with repeated error messages,
then I can live with it.

> Jonathan Tan (2):
>   Revert "fetch-pack: add a deref_without_lazy_fetch_extended()"
>   fetch-pack: warn if in commit graph but not obj db
> 
>  fetch-pack.c | 45 +++++++++++++++++++++++++--------------------
>  object.h     |  2 +-
>  2 files changed, 26 insertions(+), 21 deletions(-)
> 
> -- 
> 2.47.0.163.g1226f6d8fa-goog
> 
> 

Re: [PATCH 1/2] Revert "fetch-pack: add a deref_without_lazy_fetch_extended()"

[Josh Steadmon]

On 2024.10.29 14:11, Jonathan Tan wrote:
> This reverts commit a6e65fb39caf18259c660c1c7910d5bf80bc15cb.
> 
> The commit message of that commit mentions that the new function "will
> be used for the bundle-uri client in a subsequent commit", but it seems
> that eventually it wasn't used.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  fetch-pack.c | 25 +++++++------------------
>  1 file changed, 7 insertions(+), 18 deletions(-)

Nit: can you mention in the commit description that this cleanup
simplifies a later patch to detect a case of repo corruption?

Re: [PATCH 2/2] fetch-pack: warn if in commit graph but not obj db

[Josh Steadmon]

On 2024.10.29 14:11, Jonathan Tan wrote:
> When fetching, there is a step in which sought objects are first checked
> against the local repository; only objects that are not in the local
> repository are then fetched. This check first looks up the commit graph
> file, and returns "present" if the object is in there.
> 
> However, the action of first looking up the commit graph file is not
> done everywhere in Git, especially if the type of the object at the time
> of lookup is not known. This means that in a repo corruption situation,
> a user may encounter an "object missing" error, attempt to fetch it, and
> still encounter the same error later when they reattempt their original
> action, because the object is present in the commit graph file but not in
> the object DB.
> 
> Therefore, detect when this occurs and print a warning. (Note that
> we cannot proceed to include this object in the list of objects to
> be fetched without changing at least the fetch negotiation code:
> what would happen is that the client will send "want X" and "have X"
> and when I tested at $DAYJOB with a work server that uses JGit, the
> server reasonably returned an empty packfile. And changing the fetch
> negotiation code to only use the object DB when deciding what to report
> as "have" would be an unnecessary slowdown, I think.)
> 
> This was discovered when a lazy fetch of a missing commit completed with
> nothing actually fetched, and the writing of the commit graph file after
> every fetch then attempted to read said missing commit, triggering a
> lazy fetch of said missing commit, resulting in an infinite loop with no
> user-visible indication (until they check the list of processes running
> on their computer). With this fix, at least a warning message will be
> printed. Note that although the repo corruption we discovered was caused
> by a bug in GC in a partial clone, the behavior that this patch teaches
> Git to warn about applies to any repo with commit graph enabled and with
> a missing commit, whether it is a partial clone or not.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  fetch-pack.c | 22 +++++++++++++++++++---
>  object.h     |  2 +-
>  2 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 6728a0d2f5..5a0020366b 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -57,6 +57,7 @@ static struct string_list uri_protocols = STRING_LIST_INIT_DUP;
>  #define ALTERNATE	(1U << 1)
>  #define COMMON		(1U << 6)
>  #define REACH_SCRATCH	(1U << 7)
> +#define COMPLETE_FROM_COMMIT_GRAPH	(1U << 8)

We're defining a new flag, and we note it in object.h as well below, so
looks good so far.


>  /*
>   * After sending this many "have"s if we do not get any new ACK , we
> @@ -123,15 +124,18 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,
>  }
>  
>  static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
> -					       int mark_tags_complete)
> +					       int mark_additional_complete_information)

We're already marking some completion flags here, so we're just making
the parameter name more descriptive, OK.


>  {
>  	enum object_type type;
>  	struct object_info info = { .typep = &type };
>  	struct commit *commit;
>  
>  	commit = lookup_commit_in_graph(the_repository, oid);
> -	if (commit)
> +	if (commit) {
> +		if (mark_additional_complete_information)
> +			commit->object.flags |= COMPLETE_FROM_COMMIT_GRAPH;
>  		return commit;
> +	}

We already have a case where we're checking the commit graph, so we can
also mark the commit complete here... well, not the original "COMPLETE"
flag since we don't want to change behavior, but our new
COMPLETE_FROM_COMMIT_GRAPH flag. Sounds good.


>  
>  	while (1) {
>  		if (oid_object_info_extended(the_repository, oid, &info,
> @@ -143,7 +147,7 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
>  
>  			if (!tag->tagged)
>  				return NULL;
> -			if (mark_tags_complete)
> +			if (mark_additional_complete_information)
>  				tag->object.flags |= COMPLETE;
>  			oid = &tag->tagged->oid;
>  		} else {
> @@ -809,6 +813,14 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,
>  	save_commit_buffer = old_save_commit_buffer;
>  }
>  
> +static void warn_in_commit_graph_only(const struct object_id *oid)
> +{
> +	warning(_("You are attempting to fetch %s, which is in the commit graph file but not in the object database."),
> +		oid_to_hex(oid));
> +	warning(_("This is probably due to repo corruption."));
> +	warning(_("If you are attempting to repair this repo corruption by refetching the missing object, use 'git fetch --refetch' with the missing object."));
> +}
> +

Here's the new warning. As mentioned in my reply to the cover letter, I
feel like it makes more sense to die(), but I don't feel too strongly
about it.


>  /*
>   * Returns 1 if every object pointed to by the given remote refs is available
>   * locally and reachable from a local ref, and 0 otherwise.
> @@ -830,6 +842,10 @@ static int everything_local(struct fetch_pack_args *args,
>  				      ref->name);
>  			continue;
>  		}
> +		if (o->flags & COMPLETE_FROM_COMMIT_GRAPH) {
> +			if (!has_object(the_repository, remote, 0))
> +				warn_in_commit_graph_only(remote);
> +		}

And now that we're checking what's local, we issue our warning if we
have an object missing from the DB but mentioned in the commit graph.
Seems fine, although I wonder if it makes more sense to fail earlier. It
looks like the only place we do the
`mark_additional_complete_information` checks is in `mark_complete()`,
so should we just check this condition there? No strong feelings either
way, just curious.


>  		print_verbose(args, _("already have %s (%s)"), oid_to_hex(remote),
>  			      ref->name);
>  	}
> diff --git a/object.h b/object.h
> index 17f32f1103..196e489253 100644
> --- a/object.h
> +++ b/object.h
> @@ -65,7 +65,7 @@ void object_array_init(struct object_array *array);
>  /*
>   * object flag allocation:
>   * revision.h:               0---------10         15               23------27
> - * fetch-pack.c:             01    67
> + * fetch-pack.c:             01    6-8
>   * negotiator/default.c:       2--5
>   * walker.c:                 0-2
>   * upload-pack.c:                4       11-----14  16-----19
> -- 
> 2.47.0.163.g1226f6d8fa-goog
> 
> 

Re: [PATCH 2/2] fetch-pack: warn if in commit graph but not obj db

[Taylor Blau]

On Tue, Oct 29, 2024 at 02:11:05PM -0700, Jonathan Tan wrote:
> When fetching, there is a step in which sought objects are first checked
> against the local repository; only objects that are not in the local
> repository are then fetched. This check first looks up the commit graph
> file, and returns "present" if the object is in there.

OK.

> However, the action of first looking up the commit graph file is not
> done everywhere in Git, especially if the type of the object at the time
> of lookup is not known. This means that in a repo corruption situation,
> a user may encounter an "object missing" error, attempt to fetch it, and
> still encounter the same error later when they reattempt their original
> action, because the object is present in the commit graph file but not in
> the object DB.

I think the type of repository corruption here may be underspecified.

You say that we have some object, say X, whose type is not known. So we
don't load the commit-graph, realize that X is missing, and then try and
fetch it. In this scenario, is X actually in the commit-graph, but not
in the object database? Further, if X is in the commit-graph, I assume
we do not look it up there because we first try and find its type, which
fails, so we assume we don't have it (despite it appearing corruptly in
the commit-graph)?

I think that matches the behavior you're describing, but I want to make
sure that I'm not thinking of something else.

> This was discovered when a lazy fetch of a missing commit completed with
> nothing actually fetched, and the writing of the commit graph file after
> every fetch then attempted to read said missing commit, triggering a
> lazy fetch of said missing commit, resulting in an infinite loop with no
> user-visible indication (until they check the list of processes running
> on their computer).

Yuck :-).

> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  fetch-pack.c | 22 +++++++++++++++++++---
>  object.h     |  2 +-
>  2 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 6728a0d2f5..5a0020366b 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -57,6 +57,7 @@ static struct string_list uri_protocols = STRING_LIST_INIT_DUP;
>  #define ALTERNATE	(1U << 1)
>  #define COMMON		(1U << 6)
>  #define REACH_SCRATCH	(1U << 7)
> +#define COMPLETE_FROM_COMMIT_GRAPH	(1U << 8)
>
>  /*
>   * After sending this many "have"s if we do not get any new ACK , we
> @@ -123,15 +124,18 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,
>  }
>
>  static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
> -					       int mark_tags_complete)
> +					       int mark_additional_complete_information)
>  {
>  	enum object_type type;
>  	struct object_info info = { .typep = &type };
>  	struct commit *commit;
>
>  	commit = lookup_commit_in_graph(the_repository, oid);
> -	if (commit)
> +	if (commit) {
> +		if (mark_additional_complete_information)
> +			commit->object.flags |= COMPLETE_FROM_COMMIT_GRAPH;
>  		return commit;
> +	}
>
>  	while (1) {
>  		if (oid_object_info_extended(the_repository, oid, &info,
> @@ -143,7 +147,7 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
>
>  			if (!tag->tagged)
>  				return NULL;
> -			if (mark_tags_complete)
> +			if (mark_additional_complete_information)
>  				tag->object.flags |= COMPLETE;
>  			oid = &tag->tagged->oid;
>  		} else {
> @@ -809,6 +813,14 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,
>  	save_commit_buffer = old_save_commit_buffer;
>  }
>
> +static void warn_in_commit_graph_only(const struct object_id *oid)
> +{
> +	warning(_("You are attempting to fetch %s, which is in the commit graph file but not in the object database."),
> +		oid_to_hex(oid));
> +	warning(_("This is probably due to repo corruption."));
> +	warning(_("If you are attempting to repair this repo corruption by refetching the missing object, use 'git fetch --refetch' with the missing object."));
> +}
> +
>  /*
>   * Returns 1 if every object pointed to by the given remote refs is available
>   * locally and reachable from a local ref, and 0 otherwise.
> @@ -830,6 +842,10 @@ static int everything_local(struct fetch_pack_args *args,
>  				      ref->name);
>  			continue;
>  		}
> +		if (o->flags & COMPLETE_FROM_COMMIT_GRAPH) {
> +			if (!has_object(the_repository, remote, 0))
> +				warn_in_commit_graph_only(remote);

You discuss this a little bit in your commit message, but I wonder if we
should just die() here. I feel like we're trying to work around a
situation where the commit-graph is obviously broken because it refers
to commit objects that don't actually exist in the object store.

A few thoughts in this area:

  - What situation provokes this to be true? I could imagine there is
    some bug that we don't fully have a grasp of. But I wonder if it is
    even easier to provoke than that, say by pruning some objects out of
    the object store, then not rewriting the commit-graph, leaving some
    of the references dangling.

  - Does 'git fsck' catch this case within the commit-graph?

  - Are the other areas of the code that rely on the assumption that all
    entries in the commit-graph actually exist on disk? If so, are they
    similarly broken?

Another thought about this whole thing is that we essentially have a
code path that says: "I found this object from the commit-graph, but
don't know if I actually have it on disk, so mark it to be checked later
via has_object()".

I wonder if it would be more straightforward to replace the call to
lookup_commit_in_graph() with a direct call to has_object() in the
deref_without_lazy_fetch() function, which I think would both (a)
eliminate the need for a new flag bit to be allocated, and (b) prevent
looking up the object twice.

Thoughts?

Thanks,
Taylor

[PATCH v2 0/2] When fetching, die if in commit graph but not obj db

[Jonathan Tan]

Thanks everyone for your review comments. I've updated the patch 1
commit message as Josh requested. I'll reply individually to comments on
patch 2.

Jonathan Tan (2):
  Revert "fetch-pack: add a deref_without_lazy_fetch_extended()"
  fetch-pack: warn if in commit graph but not obj db

 fetch-pack.c                               | 42 +++++++++++-----------
 t/t5330-no-lazy-fetch-with-commit-graph.sh |  2 +-
 2 files changed, 23 insertions(+), 21 deletions(-)

Range-diff against v1:
1:  4dea8933cf ! 1:  34e87b8388 Revert "fetch-pack: add a deref_without_lazy_fetch_extended()"
    @@ Commit message
     
         This reverts commit a6e65fb39caf18259c660c1c7910d5bf80bc15cb.
     
    +    This revert simplifies the next patch in this patch set.
    +
         The commit message of that commit mentions that the new function "will
         be used for the bundle-uri client in a subsequent commit", but it seems
         that eventually it wasn't used.
2:  1027ff2cb7 ! 2:  631b9a8677 fetch-pack: warn if in commit graph but not obj db
    @@ Commit message
         action, because the object is present in the commit graph file but not in
         the object DB.
     
    -    Therefore, detect when this occurs and print a warning. (Note that
    -    we cannot proceed to include this object in the list of objects to
    -    be fetched without changing at least the fetch negotiation code:
    -    what would happen is that the client will send "want X" and "have X"
    -    and when I tested at $DAYJOB with a work server that uses JGit, the
    -    server reasonably returned an empty packfile. And changing the fetch
    -    negotiation code to only use the object DB when deciding what to report
    -    as "have" would be an unnecessary slowdown, I think.)
    +    Therefore, make it a fatal error when this occurs. (Note that we cannot
    +    proceed to include this object in the list of objects to be fetched
    +    without changing at least the fetch negotiation code: what would happen
    +    is that the client will send "want X" and "have X" and when I tested
    +    at $DAYJOB with a work server that uses JGit, the server reasonably
    +    returned an empty packfile. And changing the fetch negotiation code to
    +    only use the object DB when deciding what to report as "have" would be
    +    an unnecessary slowdown, I think.)
     
         This was discovered when a lazy fetch of a missing commit completed with
         nothing actually fetched, and the writing of the commit graph file after
         every fetch then attempted to read said missing commit, triggering a
         lazy fetch of said missing commit, resulting in an infinite loop with no
         user-visible indication (until they check the list of processes running
    -    on their computer). With this fix, at least a warning message will be
    -    printed. Note that although the repo corruption we discovered was caused
    -    by a bug in GC in a partial clone, the behavior that this patch teaches
    -    Git to warn about applies to any repo with commit graph enabled and with
    -    a missing commit, whether it is a partial clone or not.
    +    on their computer). With this fix, there is no infinite loop. Note that
    +    although the repo corruption we discovered was caused by a bug in GC in
    +    a partial clone, the behavior that this patch teaches Git to warn about
    +    applies to any repo with commit graph enabled and with a missing commit,
    +    whether it is a partial clone or not.
    +
    +    t5330, introduced in 3a1ea94a49 (commit-graph.c: no lazy fetch in
    +    lookup_commit_in_graph(), 2022-07-01), tests that an interaction between
    +    fetch and the commit graph does not cause an infinite loop. This patch
    +    changes the exit code in that situation, so that test had to be changed.
     
         Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
     
      ## fetch-pack.c ##
    -@@ fetch-pack.c: static struct string_list uri_protocols = STRING_LIST_INIT_DUP;
    - #define ALTERNATE	(1U << 1)
    - #define COMMON		(1U << 6)
    - #define REACH_SCRATCH	(1U << 7)
    -+#define COMPLETE_FROM_COMMIT_GRAPH	(1U << 8)
    - 
    - /*
    -  * After sending this many "have"s if we do not get any new ACK , we
     @@ fetch-pack.c: static void for_each_cached_alternate(struct fetch_negotiator *negotiator,
    + 		cb(negotiator, cache.items[i]);
      }
      
    ++static void die_in_commit_graph_only(const struct object_id *oid)
    ++{
    ++	die(_("You are attempting to fetch %s, which is in the commit graph file but not in the object database.\n"
    ++	      "This is probably due to repo corruption.\n"
    ++	      "If you are attempting to repair this repo corruption by refetching the missing object, use 'git fetch --refetch' with the missing object."),
    ++	      oid_to_hex(oid));
    ++}
    ++
      static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
     -					       int mark_tags_complete)
    -+					       int mark_additional_complete_information)
    ++					       int mark_tags_complete_and_check_obj_db)
      {
      	enum object_type type;
      	struct object_info info = { .typep = &type };
    @@ fetch-pack.c: static void for_each_cached_alternate(struct fetch_negotiator *neg
      	commit = lookup_commit_in_graph(the_repository, oid);
     -	if (commit)
     +	if (commit) {
    -+		if (mark_additional_complete_information)
    -+			commit->object.flags |= COMPLETE_FROM_COMMIT_GRAPH;
    ++		if (mark_tags_complete_and_check_obj_db) {
    ++			if (!has_object(the_repository, oid, 0))
    ++				die_in_commit_graph_only(oid);
    ++		}
      		return commit;
     +	}
      
    @@ fetch-pack.c: static struct commit *deref_without_lazy_fetch(const struct object
      			if (!tag->tagged)
      				return NULL;
     -			if (mark_tags_complete)
    -+			if (mark_additional_complete_information)
    ++			if (mark_tags_complete_and_check_obj_db)
      				tag->object.flags |= COMPLETE;
      			oid = &tag->tagged->oid;
      		} else {
    -@@ fetch-pack.c: static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,
    - 	save_commit_buffer = old_save_commit_buffer;
    - }
    - 
    -+static void warn_in_commit_graph_only(const struct object_id *oid)
    -+{
    -+	warning(_("You are attempting to fetch %s, which is in the commit graph file but not in the object database."),
    -+		oid_to_hex(oid));
    -+	warning(_("This is probably due to repo corruption."));
    -+	warning(_("If you are attempting to repair this repo corruption by refetching the missing object, use 'git fetch --refetch' with the missing object."));
    -+}
    -+
    - /*
    -  * Returns 1 if every object pointed to by the given remote refs is available
    -  * locally and reachable from a local ref, and 0 otherwise.
    -@@ fetch-pack.c: static int everything_local(struct fetch_pack_args *args,
    - 				      ref->name);
    - 			continue;
    - 		}
    -+		if (o->flags & COMPLETE_FROM_COMMIT_GRAPH) {
    -+			if (!has_object(the_repository, remote, 0))
    -+				warn_in_commit_graph_only(remote);
    -+		}
    - 		print_verbose(args, _("already have %s (%s)"), oid_to_hex(remote),
    - 			      ref->name);
    - 	}
     
    - ## object.h ##
    -@@ object.h: void object_array_init(struct object_array *array);
    - /*
    -  * object flag allocation:
    -  * revision.h:               0---------10         15               23------27
    -- * fetch-pack.c:             01    67
    -+ * fetch-pack.c:             01    6-8
    -  * negotiator/default.c:       2--5
    -  * walker.c:                 0-2
    -  * upload-pack.c:                4       11-----14  16-----19
    + ## t/t5330-no-lazy-fetch-with-commit-graph.sh ##
    +@@ t/t5330-no-lazy-fetch-with-commit-graph.sh: test_expect_success 'fetch any commit from promisor with the usage of the commit
    + 	test_commit -C with-commit any-commit &&
    + 	anycommit=$(git -C with-commit rev-parse HEAD) &&
    + 	GIT_TRACE="$(pwd)/trace.txt" \
    +-		git -C with-commit-graph fetch origin $anycommit 2>err &&
    ++		test_must_fail git -C with-commit-graph fetch origin $anycommit 2>err &&
    + 	! grep "fatal: promisor-remote: unable to fork off fetch subprocess" err &&
    + 	grep "git fetch origin" trace.txt >actual &&
    + 	test_line_count = 1 actual
-- 
2.47.0.163.g1226f6d8fa-goog

[PATCH v2 1/2] Revert "fetch-pack: add a deref_without_lazy_fetch_extended()"

[Jonathan Tan]

This reverts commit a6e65fb39caf18259c660c1c7910d5bf80bc15cb.

This revert simplifies the next patch in this patch set.

The commit message of that commit mentions that the new function "will
be used for the bundle-uri client in a subsequent commit", but it seems
that eventually it wasn't used.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 fetch-pack.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index f752da93a8..6728a0d2f5 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -122,12 +122,11 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,
 		cb(negotiator, cache.items[i]);
 }
 
-static struct commit *deref_without_lazy_fetch_extended(const struct object_id *oid,
-							int mark_tags_complete,
-							enum object_type *type,
-							unsigned int oi_flags)
+static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
+					       int mark_tags_complete)
 {
-	struct object_info info = { .typep = type };
+	enum object_type type;
+	struct object_info info = { .typep = &type };
 	struct commit *commit;
 
 	commit = lookup_commit_in_graph(the_repository, oid);
@@ -136,9 +135,9 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *
 
 	while (1) {
 		if (oid_object_info_extended(the_repository, oid, &info,
-					     oi_flags))
+					     OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK))
 			return NULL;
-		if (*type == OBJ_TAG) {
+		if (type == OBJ_TAG) {
 			struct tag *tag = (struct tag *)
 				parse_object(the_repository, oid);
 
@@ -152,7 +151,7 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *
 		}
 	}
 
-	if (*type == OBJ_COMMIT) {
+	if (type == OBJ_COMMIT) {
 		struct commit *commit = lookup_commit(the_repository, oid);
 		if (!commit || repo_parse_commit(the_repository, commit))
 			return NULL;
@@ -162,16 +161,6 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *
 	return NULL;
 }
 
-
-static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
-					       int mark_tags_complete)
-{
-	enum object_type type;
-	unsigned flags = OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK;
-	return deref_without_lazy_fetch_extended(oid, mark_tags_complete,
-						 &type, flags);
-}
-
 static int rev_list_insert_ref(struct fetch_negotiator *negotiator,
 			       const struct object_id *oid)
 {
-- 
2.47.0.163.g1226f6d8fa-goog

[PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db

[Jonathan Tan]

When fetching, there is a step in which sought objects are first checked
against the local repository; only objects that are not in the local
repository are then fetched. This check first looks up the commit graph
file, and returns "present" if the object is in there.

However, the action of first looking up the commit graph file is not
done everywhere in Git, especially if the type of the object at the time
of lookup is not known. This means that in a repo corruption situation,
a user may encounter an "object missing" error, attempt to fetch it, and
still encounter the same error later when they reattempt their original
action, because the object is present in the commit graph file but not in
the object DB.

Therefore, make it a fatal error when this occurs. (Note that we cannot
proceed to include this object in the list of objects to be fetched
without changing at least the fetch negotiation code: what would happen
is that the client will send "want X" and "have X" and when I tested
at $DAYJOB with a work server that uses JGit, the server reasonably
returned an empty packfile. And changing the fetch negotiation code to
only use the object DB when deciding what to report as "have" would be
an unnecessary slowdown, I think.)

This was discovered when a lazy fetch of a missing commit completed with
nothing actually fetched, and the writing of the commit graph file after
every fetch then attempted to read said missing commit, triggering a
lazy fetch of said missing commit, resulting in an infinite loop with no
user-visible indication (until they check the list of processes running
on their computer). With this fix, there is no infinite loop. Note that
although the repo corruption we discovered was caused by a bug in GC in
a partial clone, the behavior that this patch teaches Git to warn about
applies to any repo with commit graph enabled and with a missing commit,
whether it is a partial clone or not.

t5330, introduced in 3a1ea94a49 (commit-graph.c: no lazy fetch in
lookup_commit_in_graph(), 2022-07-01), tests that an interaction between
fetch and the commit graph does not cause an infinite loop. This patch
changes the exit code in that situation, so that test had to be changed.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 fetch-pack.c                               | 19 ++++++++++++++++---
 t/t5330-no-lazy-fetch-with-commit-graph.sh |  2 +-
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 6728a0d2f5..fe1fb3c1b7 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -122,16 +122,29 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,
 		cb(negotiator, cache.items[i]);
 }
 
+static void die_in_commit_graph_only(const struct object_id *oid)
+{
+	die(_("You are attempting to fetch %s, which is in the commit graph file but not in the object database.\n"
+	      "This is probably due to repo corruption.\n"
+	      "If you are attempting to repair this repo corruption by refetching the missing object, use 'git fetch --refetch' with the missing object."),
+	      oid_to_hex(oid));
+}
+
 static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
-					       int mark_tags_complete)
+					       int mark_tags_complete_and_check_obj_db)
 {
 	enum object_type type;
 	struct object_info info = { .typep = &type };
 	struct commit *commit;
 
 	commit = lookup_commit_in_graph(the_repository, oid);
-	if (commit)
+	if (commit) {
+		if (mark_tags_complete_and_check_obj_db) {
+			if (!has_object(the_repository, oid, 0))
+				die_in_commit_graph_only(oid);
+		}
 		return commit;
+	}
 
 	while (1) {
 		if (oid_object_info_extended(the_repository, oid, &info,
@@ -143,7 +156,7 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
 
 			if (!tag->tagged)
 				return NULL;
-			if (mark_tags_complete)
+			if (mark_tags_complete_and_check_obj_db)
 				tag->object.flags |= COMPLETE;
 			oid = &tag->tagged->oid;
 		} else {
diff --git a/t/t5330-no-lazy-fetch-with-commit-graph.sh b/t/t5330-no-lazy-fetch-with-commit-graph.sh
index 5eb28f0512..feccd58324 100755
--- a/t/t5330-no-lazy-fetch-with-commit-graph.sh
+++ b/t/t5330-no-lazy-fetch-with-commit-graph.sh
@@ -39,7 +39,7 @@ test_expect_success 'fetch any commit from promisor with the usage of the commit
 	test_commit -C with-commit any-commit &&
 	anycommit=$(git -C with-commit rev-parse HEAD) &&
 	GIT_TRACE="$(pwd)/trace.txt" \
-		git -C with-commit-graph fetch origin $anycommit 2>err &&
+		test_must_fail git -C with-commit-graph fetch origin $anycommit 2>err &&
 	! grep "fatal: promisor-remote: unable to fork off fetch subprocess" err &&
 	grep "git fetch origin" trace.txt >actual &&
 	test_line_count = 1 actual
-- 
2.47.0.163.g1226f6d8fa-goog

Re: [PATCH 2/2] fetch-pack: warn if in commit graph but not obj db

[Jonathan Tan]

Josh Steadmon <steadmon@google.com> writes:
> >  /*
> >   * Returns 1 if every object pointed to by the given remote refs is available
> >   * locally and reachable from a local ref, and 0 otherwise.
> > @@ -830,6 +842,10 @@ static int everything_local(struct fetch_pack_args *args,
> >  				      ref->name);
> >  			continue;
> >  		}
> > +		if (o->flags & COMPLETE_FROM_COMMIT_GRAPH) {
> > +			if (!has_object(the_repository, remote, 0))
> > +				warn_in_commit_graph_only(remote);
> > +		}
> 
> And now that we're checking what's local, we issue our warning if we
> have an object missing from the DB but mentioned in the commit graph.
> Seems fine, although I wonder if it makes more sense to fail earlier. It
> looks like the only place we do the
> `mark_additional_complete_information` checks is in `mark_complete()`,
> so should we just check this condition there? No strong feelings either
> way, just curious.

When we were merely warning, it was useful to mark everything then
check later, so that a warning message would be printed once per
object, instead of potentially multiple times. (In the infinite case
that we discovered at $DAYJOB, it doesn't really matter since the
message is going to be printed an infinite number of times anyway,
but in the "plain" case in which the user is missing a commit and does
not have automatic commit graph writing enabled, the fetch will indeed
terminate.)

But since we're making this a fatal error, yes, it makes sense to fail
earlier. I've made the change.

Re: [PATCH 2/2] fetch-pack: warn if in commit graph but not obj db

[Jonathan Tan]

Taylor Blau <me@ttaylorr.com> writes:
> > However, the action of first looking up the commit graph file is not
> > done everywhere in Git, especially if the type of the object at the time
> > of lookup is not known. This means that in a repo corruption situation,
> > a user may encounter an "object missing" error, attempt to fetch it, and
> > still encounter the same error later when they reattempt their original
> > action, because the object is present in the commit graph file but not in
> > the object DB.
> 
> I think the type of repository corruption here may be underspecified.

Hmm...if you have any specific points you'd like me to elaborate on (or
better yet, wording suggestions), please let me know.

> You say that we have some object, say X, whose type is not known. So we
> don't load the commit-graph, realize that X is missing, and then try and
> fetch it.

Yes.

> In this scenario, is X actually in the commit-graph, but not
> in the object database?

Yes.

> Further, if X is in the commit-graph, I assume
> we do not look it up there because we first try and find its type, which
> fails, so we assume we don't have it (despite it appearing corruptly in
> the commit-graph)?
> 
> I think that matches the behavior you're describing, but I want to make
> sure that I'm not thinking of something else.

Strictly speaking, we are not trying to find its type. We are trying
to find the object itself. (One could argue that if we find out that
an object is a commit, we can then ignore the packfile and go look up
the commit graph file. I'm not so sure this is a good idea, but this is
moot, I think - as far as I know, we currently don't do this.)

But yes, if the object is not in the object DB, we assume we don't have
it.

> You discuss this a little bit in your commit message, but I wonder if we
> should just die() here. I feel like we're trying to work around a
> situation where the commit-graph is obviously broken because it refers
> to commit objects that don't actually exist in the object store.

Yeah, that seems to be the consensus. I've switched it to a fatal error.

> A few thoughts in this area:
> 
>   - What situation provokes this to be true? I could imagine there is
>     some bug that we don't fully have a grasp of. But I wonder if it is
>     even easier to provoke than that, say by pruning some objects out of
>     the object store, then not rewriting the commit-graph, leaving some
>     of the references dangling.

The fetching of promisor objects that are descendants of non-promisor
objects. [1]

I think that the rewriting of the commit graph happens on every repack,
thus avoiding the situation you describe (unless there is a bug there).

[1] https://lore.kernel.org/git/20241001191811.1934900-1-calvinwan@google.com/

>   - Does 'git fsck' catch this case within the commit-graph?

Honestly, I haven't checked - I've been concentrating on fixing the
fetch part for now (and also the bug that caused the missing commits
[2]).

[2] https://lore.kernel.org/git/cover.1729792911.git.jonathantanmy@google.com/

>   - Are the other areas of the code that rely on the assumption that all
>     entries in the commit-graph actually exist on disk? If so, are they
>     similarly broken?

Yes, the fetch negotiation code. It is not "broken" in that it solely
uses repo_parse_commit() which always checks the commit graph, so as
long as the commit graph has everything we need, there will be no error.

There might be other systems that rely both on the commit graph and the
object DB, and thus have an inconsistent view (so, "similarly broken" as
you describe it) but at least in the partial clone case, the severity of
the issue is not as high as in "fetch", because these other systems can
lazily fetch the missing commit and then proceed.

> Another thought about this whole thing is that we essentially have a
> code path that says: "I found this object from the commit-graph, but
> don't know if I actually have it on disk, so mark it to be checked later
> via has_object()".
> 
> I wonder if it would be more straightforward to replace the call to
> lookup_commit_in_graph() with a direct call to has_object() in the
> deref_without_lazy_fetch() function, which I think would both (a)
> eliminate the need for a new flag bit to be allocated, and (b) prevent
> looking up the object twice.
> 
> Thoughts?
> 
> Thanks,
> Taylor

This would undo the optimization in 62b5a35a33 (fetch-pack: optimize
loading of refs via commit graph, 2021-09-01), and also would not work
without changes to the fetch negotiation code - I tried to describe it
in the commit message, perhaps not very clearly, but the issue is that
even if we emit "want X", the fetch negotiation code would emit "have
X" (the X is the same in both), and at least for our JGit server at
$DAYJOB, the combination of "want X" and "have X" results in the server
sending an empty packfile (reasonable behavior, I think). (And I don't
think the changes to the fetch negotiation code are worth it.)

Re: [PATCH v2 0/2] When fetching, die if in commit graph but not obj db

[Josh Steadmon]

On 2024.10.31 14:18, Jonathan Tan wrote:
> Thanks everyone for your review comments. I've updated the patch 1
> commit message as Josh requested. I'll reply individually to comments on
> patch 2.
> 
> Jonathan Tan (2):
>   Revert "fetch-pack: add a deref_without_lazy_fetch_extended()"
>   fetch-pack: warn if in commit graph but not obj db
> 
>  fetch-pack.c                               | 42 +++++++++++-----------
>  t/t5330-no-lazy-fetch-with-commit-graph.sh |  2 +-
>  2 files changed, 23 insertions(+), 21 deletions(-)

This version looks good to me, thanks!

Reviewed-by: Josh Steadmon <steadmon@google.com>

Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

>  static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
> -					       int mark_tags_complete)
> +					       int mark_tags_complete_and_check_obj_db)
>  {
>  	enum object_type type;
>  	struct object_info info = { .typep = &type };
>  	struct commit *commit;
>  
>  	commit = lookup_commit_in_graph(the_repository, oid);
> -	if (commit)
> +	if (commit) {
> +		if (mark_tags_complete_and_check_obj_db) {
> +			if (!has_object(the_repository, oid, 0))
> +				die_in_commit_graph_only(oid);
> +		}
>  		return commit;
> +	}

Hmph, even when we are not doing the mark-tags-complete thing,
wouldn't it be a fatal error if the commit graph claims a commit
exists but we are missing it?

It also makes me wonder if it would be sufficient to prevent us from
saying "have X" if we just pretend as if lookup_commit_in_graph()
returned NULL in this case.

In any case, infinitely recursing to lazily fetch a single commit is
definitely worth fixing.  Thanks for digging to the bottom of the
problem and fixing it.

Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>>  	commit = lookup_commit_in_graph(the_repository, oid);
>> -	if (commit)
>> +	if (commit) {
>> +		if (mark_tags_complete_and_check_obj_db) {
>> +			if (!has_object(the_repository, oid, 0))
>> +				die_in_commit_graph_only(oid);
>> +		}
>>  		return commit;
>> +	}
>
> Hmph, even when we are not doing the mark-tags-complete thing,
> wouldn't it be a fatal error if the commit graph claims a commit
> exists but we are missing it?
>
> It also makes me wonder if it would be sufficient to prevent us from
> saying "have X" if we just pretend as if lookup_commit_in_graph()
> returned NULL in this case.

Again, sorry for the noise.

I think the posted patch is better without either of these two,
simply because the "commit graph lies" case is a repository
corruption, and "git fsck" should catch such a corruption (and if
not, we should make sure it does).

The normal codepaths should assume a healthy working repository.

As has_object() is not without cost, an extra check is warranted
only because not checking will go into infinite recursion.  If it
does not make us fail in such an unpleasant way if we return such a
commit when we are not doing the mark-tags-complete thing (but makes
us fail in some other controlled way), not paying cost for an extra
check is the right thing.

Thanks.

Re: [External] Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db

[Han Xin]

On Fri, Nov 1, 2024 at 12:25 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> >>      commit = lookup_commit_in_graph(the_repository, oid);
> >> -    if (commit)
> >> +    if (commit) {
> >> +            if (mark_tags_complete_and_check_obj_db) {
> >> +                    if (!has_object(the_repository, oid, 0))
> >> +                            die_in_commit_graph_only(oid);
> >> +            }
> >>              return commit;
> >> +    }
> >
> > Hmph, even when we are not doing the mark-tags-complete thing,
> > wouldn't it be a fatal error if the commit graph claims a commit
> > exists but we are missing it?
> >
> > It also makes me wonder if it would be sufficient to prevent us from
> > saying "have X" if we just pretend as if lookup_commit_in_graph()
> > returned NULL in this case.
>
> Again, sorry for the noise.
>
> I think the posted patch is better without either of these two,
> simply because the "commit graph lies" case is a repository
> corruption, and "git fsck" should catch such a corruption (and if
> not, we should make sure it does).
>
> The normal codepaths should assume a healthy working repository.
>
> As has_object() is not without cost, an extra check is warranted
> only because not checking will go into infinite recursion.  If it
> does not make us fail in such an unpleasant way if we return such a
> commit when we are not doing the mark-tags-complete thing (but makes
> us fail in some other controlled way), not paying cost for an extra
> check is the right thing.
>
> Thanks.

Although the scenario I faked in t/t5330-no-lazy-fetch-with-commit-graph.sh
usually does not occur, if we are unfortunate enough to encounter this issue,
I hope it can automatically fix the problem as much as possible without
relying on me to take an extra action.

Thanks.

Re: [PATCH 2/2] fetch-pack: warn if in commit graph but not obj db

[Taylor Blau]

On Thu, Oct 31, 2024 at 02:43:19PM -0700, Jonathan Tan wrote:
> > Another thought about this whole thing is that we essentially have a
> > code path that says: "I found this object from the commit-graph, but
> > don't know if I actually have it on disk, so mark it to be checked later
> > via has_object()".
> >
> > I wonder if it would be more straightforward to replace the call to
> > lookup_commit_in_graph() with a direct call to has_object() in the
> > deref_without_lazy_fetch() function, which I think would both (a)
> > eliminate the need for a new flag bit to be allocated, and (b) prevent
> > looking up the object twice.
> >
> > Thoughts?
>
> This would undo the optimization in 62b5a35a33 (fetch-pack: optimize
> loading of refs via commit graph, 2021-09-01), and also would not work
> without changes to the fetch negotiation code - I tried to describe it
> in the commit message, perhaps not very clearly, but the issue is that
> even if we emit "want X", the fetch negotiation code would emit "have
> X" (the X is the same in both), and at least for our JGit server at
> $DAYJOB, the combination of "want X" and "have X" results in the server
> sending an empty packfile (reasonable behavior, I think). (And I don't
> think the changes to the fetch negotiation code are worth it.)

Thanks for the clarifications above. What I was trying to poke at here
was... doesn't the change as presented undo that optimization, just in a
different way?

In 62b5a35a33 we taught deref_without_lazy_fetch() to lookup commits
through the commit-graph. But in this patch, we now call has_object()
on top of that existing check. Am I missing something obvious?

Thanks,
Taylor

Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db

[Taylor Blau]

On Thu, Oct 31, 2024 at 02:19:01PM -0700, Jonathan Tan wrote:
> diff --git a/t/t5330-no-lazy-fetch-with-commit-graph.sh b/t/t5330-no-lazy-fetch-with-commit-graph.sh
> index 5eb28f0512..feccd58324 100755
> --- a/t/t5330-no-lazy-fetch-with-commit-graph.sh
> +++ b/t/t5330-no-lazy-fetch-with-commit-graph.sh
> @@ -39,7 +39,7 @@ test_expect_success 'fetch any commit from promisor with the usage of the commit
>  	test_commit -C with-commit any-commit &&
>  	anycommit=$(git -C with-commit rev-parse HEAD) &&
>  	GIT_TRACE="$(pwd)/trace.txt" \
> -		git -C with-commit-graph fetch origin $anycommit 2>err &&
> +		test_must_fail git -C with-commit-graph fetch origin $anycommit 2>err &&

It appears that this line breaks CI:

    https://github.com/ttaylorr/git/actions/runs/11631453312/job/32392591229

because you're using a one-shot environment variable assignment before
calling a shell function.

This should instead be:

    test_must_fail env GIT_TRACE="$(pwd)/trace.txt" \
      git -C with-commit-graph fetch origin $anycommit 2>err &&

Thanks,
Taylor

Re: [PATCH 2/2] fetch-pack: warn if in commit graph but not obj db

[Jonathan Tan]

Taylor Blau <me@ttaylorr.com> writes:
> On Thu, Oct 31, 2024 at 02:43:19PM -0700, Jonathan Tan wrote:
> > > Another thought about this whole thing is that we essentially have a
> > > code path that says: "I found this object from the commit-graph, but
> > > don't know if I actually have it on disk, so mark it to be checked later
> > > via has_object()".
> > >
> > > I wonder if it would be more straightforward to replace the call to
> > > lookup_commit_in_graph() with a direct call to has_object() in the
> > > deref_without_lazy_fetch() function, which I think would both (a)
> > > eliminate the need for a new flag bit to be allocated, and (b) prevent
> > > looking up the object twice.
> > >
> > > Thoughts?
> >
> > This would undo the optimization in 62b5a35a33 (fetch-pack: optimize
> > loading of refs via commit graph, 2021-09-01), and also would not work
> > without changes to the fetch negotiation code - I tried to describe it
> > in the commit message, perhaps not very clearly, but the issue is that
> > even if we emit "want X", the fetch negotiation code would emit "have
> > X" (the X is the same in both), and at least for our JGit server at
> > $DAYJOB, the combination of "want X" and "have X" results in the server
> > sending an empty packfile (reasonable behavior, I think). (And I don't
> > think the changes to the fetch negotiation code are worth it.)
> 
> Thanks for the clarifications above. What I was trying to poke at here
> was... doesn't the change as presented undo that optimization, just in a
> different way?
> 
> In 62b5a35a33 we taught deref_without_lazy_fetch() to lookup commits
> through the commit-graph. But in this patch, we now call has_object()
> on top of that existing check. Am I missing something obvious?
> 
> Thanks,
> Taylor

deref_without_lazy_fetch() is used in these situations:
 (1) to mark things COMPLETE (the 2nd argument is set to 1)
 (2) all other situations (the 2nd argument is set to 0)

62b5a35a33 teaches deref_without_lazy_fetch() to use the commit-graph in
all situations.

The change I have presented in this patch set teaches
deref_without_lazy_fetch() to read both the commit-graph and the object
DB in (1) but not (2). So I'm undoing the optimization, but not for
all situations.

My understanding of your suggestion was to undo the optimization in
all situations.

Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db

[Jonathan Tan]

Junio C Hamano <gitster@pobox.com> writes:
> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> >  static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
> > -					       int mark_tags_complete)
> > +					       int mark_tags_complete_and_check_obj_db)
> >  {
> >  	enum object_type type;
> >  	struct object_info info = { .typep = &type };
> >  	struct commit *commit;
> >  
> >  	commit = lookup_commit_in_graph(the_repository, oid);
> > -	if (commit)
> > +	if (commit) {
> > +		if (mark_tags_complete_and_check_obj_db) {
> > +			if (!has_object(the_repository, oid, 0))
> > +				die_in_commit_graph_only(oid);
> > +		}
> >  		return commit;
> > +	}
> 
> Hmph, even when we are not doing the mark-tags-complete thing,
> wouldn't it be a fatal error if the commit graph claims a commit
> exists but we are missing it?

If we can detect this cheaply, yes it would be ideal if every time
we read a commit from the commit graph, we also check that the commit
exists in the object DB. I don't think we can do it cheaply, though.

> It also makes me wonder if it would be sufficient to prevent us from
> saying "have X" if we just pretend as if lookup_commit_in_graph()
> returned NULL in this case.

"have X" is controlled by the packfile negotiation part, so we would
have to change that. (This part controls whether we send "want X" for a
specific OID or not.)

> In any case, infinitely recursing to lazily fetch a single commit is
> definitely worth fixing.  Thanks for digging to the bottom of the
> problem and fixing it.

Thanks.

Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db

[Jonathan Tan]

Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >>  	commit = lookup_commit_in_graph(the_repository, oid);
> >> -	if (commit)
> >> +	if (commit) {
> >> +		if (mark_tags_complete_and_check_obj_db) {
> >> +			if (!has_object(the_repository, oid, 0))
> >> +				die_in_commit_graph_only(oid);
> >> +		}
> >>  		return commit;
> >> +	}
> >
> > Hmph, even when we are not doing the mark-tags-complete thing,
> > wouldn't it be a fatal error if the commit graph claims a commit
> > exists but we are missing it?
> >
> > It also makes me wonder if it would be sufficient to prevent us from
> > saying "have X" if we just pretend as if lookup_commit_in_graph()
> > returned NULL in this case.
> 
> Again, sorry for the noise.
> 
> I think the posted patch is better without either of these two,
> simply because the "commit graph lies" case is a repository
> corruption, and "git fsck" should catch such a corruption (and if
> not, we should make sure it does).
> 
> The normal codepaths should assume a healthy working repository.
> 
> As has_object() is not without cost, an extra check is warranted
> only because not checking will go into infinite recursion.  If it
> does not make us fail in such an unpleasant way if we return such a
> commit when we are not doing the mark-tags-complete thing (but makes
> us fail in some other controlled way), not paying cost for an extra
> check is the right thing.
> 
> Thanks.

Just checking...by "the posted patch is better without either
of these two", do you mean that we should not use has_object()
here? I included it here in as narrow a scope as possible (when
"mark_tags_complete_and_check_obj_db" is true) precisely because not
checking will go into infinite recursion, as you said. (And indeed I did
not expand the scope because I agree that the normal codepaths should
assume a healthy working repository.)

Re: [External] Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db

[Jonathan Tan]

Han Xin <hanxin.hx@bytedance.com> writes:
> Although the scenario I faked in t/t5330-no-lazy-fetch-with-commit-graph.sh
> usually does not occur, if we are unfortunate enough to encounter this issue,
> I hope it can automatically fix the problem as much as possible without
> relying on me to take an extra action.
> 
> Thanks.

Note that unfortunately the user will still need to take an extra
action.

My goal at first was to try to teach Git to fetch the commit (in the
commit graph file but not in the object DB) anyway, so that (as you
said) the problem will fix itself, but that requires not only a change
in the part of the code that emits "want", but also the part that emits
"have" (the fetch negotiation code). At that point, I decided that it's
better to stop early and warn the user (it was not a fatal error in the
original version of the code, but I have changed it).

Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db

[Jonathan Tan]

Taylor Blau <me@ttaylorr.com> writes:
> On Thu, Oct 31, 2024 at 02:19:01PM -0700, Jonathan Tan wrote:
> > diff --git a/t/t5330-no-lazy-fetch-with-commit-graph.sh b/t/t5330-no-lazy-fetch-with-commit-graph.sh
> > index 5eb28f0512..feccd58324 100755
> > --- a/t/t5330-no-lazy-fetch-with-commit-graph.sh
> > +++ b/t/t5330-no-lazy-fetch-with-commit-graph.sh
> > @@ -39,7 +39,7 @@ test_expect_success 'fetch any commit from promisor with the usage of the commit
> >  	test_commit -C with-commit any-commit &&
> >  	anycommit=$(git -C with-commit rev-parse HEAD) &&
> >  	GIT_TRACE="$(pwd)/trace.txt" \
> > -		git -C with-commit-graph fetch origin $anycommit 2>err &&
> > +		test_must_fail git -C with-commit-graph fetch origin $anycommit 2>err &&
> 
> It appears that this line breaks CI:
> 
>     https://github.com/ttaylorr/git/actions/runs/11631453312/job/32392591229
> 
> because you're using a one-shot environment variable assignment before
> calling a shell function.
> 
> This should instead be:
> 
>     test_must_fail env GIT_TRACE="$(pwd)/trace.txt" \
>       git -C with-commit-graph fetch origin $anycommit 2>err &&
> 
> Thanks,
> Taylor

Ah, thanks. I've made the change locally. There are a few ongoing
conversations about this patch set (thanks everyone for participating)
so I'll wait a while before sending out the next set.

Re: [PATCH v2 2/2] fetch-pack: warn if in commit graph but not obj db

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> >>  	commit = lookup_commit_in_graph(the_repository, oid);
>> >> -	if (commit)
>> >> +	if (commit) {
>> >> +		if (mark_tags_complete_and_check_obj_db) {
>> >> +			if (!has_object(the_repository, oid, 0))
>> >> +				die_in_commit_graph_only(oid);
>> >> +		}
>> >>  		return commit;
>> >> +	}
>> >
>> > Hmph, even when we are not doing the mark-tags-complete thing,
>> > wouldn't it be a fatal error if the commit graph claims a commit
>> > exists but we are missing it?
>> >
>> > It also makes me wonder if it would be sufficient to prevent us from
>> > saying "have X" if we just pretend as if lookup_commit_in_graph()
>> > returned NULL in this case.
>> 
>> Again, sorry for the noise.
>> 
>> I think the posted patch is better without either of these two,
>> simply because the "commit graph lies" case is a repository
>> corruption, and "git fsck" should catch such a corruption (and if
>> not, we should make sure it does).
>> 
>> The normal codepaths should assume a healthy working repository.
>> 
>> As has_object() is not without cost, an extra check is warranted
>> only because not checking will go into infinite recursion.  If it
>> does not make us fail in such an unpleasant way if we return such a
>> commit when we are not doing the mark-tags-complete thing (but makes
>> us fail in some other controlled way), not paying cost for an extra
>> check is the right thing.
>> 
>> Thanks.
>
> Just checking...by "the posted patch is better without either
> of these two", do you mean that we should not use has_object()
> here?

No, "these two" refers to two changes I hinted at in my message,
i.e. (1) regardless of mark_tags_complete_and_check_obj_db shouldn't
we check with has_object() and die? and (2) if we commit=NULL and
keep going, would it be sufficient to fix it?

[PATCH v3 0/2] When fetching, die if in commit graph but not obj db

[Jonathan Tan]

Changes: the commit message title of the second patch, and a change from
grep to test_grep.

Jonathan Tan (2):
  Revert "fetch-pack: add a deref_without_lazy_fetch_extended()"
  fetch-pack: die if in commit graph but not obj db

 fetch-pack.c                               | 42 +++++++++++-----------
 t/t5330-no-lazy-fetch-with-commit-graph.sh |  4 +--
 2 files changed, 24 insertions(+), 22 deletions(-)

Range-diff against v2:
1:  34e87b8388 = 1:  34e87b8388 Revert "fetch-pack: add a deref_without_lazy_fetch_extended()"
2:  a35e386a0e ! 2:  c92b2c9e50 fetch-pack: warn if in commit graph but not obj db
    @@ Metadata
     Author: Jonathan Tan <jonathantanmy@google.com>
     
      ## Commit message ##
    -    fetch-pack: warn if in commit graph but not obj db
    +    fetch-pack: die if in commit graph but not obj db
     
         When fetching, there is a step in which sought objects are first checked
         against the local repository; only objects that are not in the local
    @@ t/t5330-no-lazy-fetch-with-commit-graph.sh: test_expect_success 'fetch any commi
     -	GIT_TRACE="$(pwd)/trace.txt" \
     +	test_must_fail env GIT_TRACE="$(pwd)/trace.txt" \
      		git -C with-commit-graph fetch origin $anycommit 2>err &&
    - 	! grep "fatal: promisor-remote: unable to fork off fetch subprocess" err &&
    +-	! grep "fatal: promisor-remote: unable to fork off fetch subprocess" err &&
    ++	test_grep ! "fatal: promisor-remote: unable to fork off fetch subprocess" err &&
      	grep "git fetch origin" trace.txt >actual &&
    + 	test_line_count = 1 actual
    + '
-- 
2.47.0.277.g8800431eea-goog

[PATCH v3 1/2] Revert "fetch-pack: add a deref_without_lazy_fetch_extended()"

[Jonathan Tan]

This reverts commit a6e65fb39caf18259c660c1c7910d5bf80bc15cb.

This revert simplifies the next patch in this patch set.

The commit message of that commit mentions that the new function "will
be used for the bundle-uri client in a subsequent commit", but it seems
that eventually it wasn't used.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 fetch-pack.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index f752da93a8..6728a0d2f5 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -122,12 +122,11 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,
 		cb(negotiator, cache.items[i]);
 }
 
-static struct commit *deref_without_lazy_fetch_extended(const struct object_id *oid,
-							int mark_tags_complete,
-							enum object_type *type,
-							unsigned int oi_flags)
+static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
+					       int mark_tags_complete)
 {
-	struct object_info info = { .typep = type };
+	enum object_type type;
+	struct object_info info = { .typep = &type };
 	struct commit *commit;
 
 	commit = lookup_commit_in_graph(the_repository, oid);
@@ -136,9 +135,9 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *
 
 	while (1) {
 		if (oid_object_info_extended(the_repository, oid, &info,
-					     oi_flags))
+					     OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK))
 			return NULL;
-		if (*type == OBJ_TAG) {
+		if (type == OBJ_TAG) {
 			struct tag *tag = (struct tag *)
 				parse_object(the_repository, oid);
 
@@ -152,7 +151,7 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *
 		}
 	}
 
-	if (*type == OBJ_COMMIT) {
+	if (type == OBJ_COMMIT) {
 		struct commit *commit = lookup_commit(the_repository, oid);
 		if (!commit || repo_parse_commit(the_repository, commit))
 			return NULL;
@@ -162,16 +161,6 @@ static struct commit *deref_without_lazy_fetch_extended(const struct object_id *
 	return NULL;
 }
 
-
-static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
-					       int mark_tags_complete)
-{
-	enum object_type type;
-	unsigned flags = OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK;
-	return deref_without_lazy_fetch_extended(oid, mark_tags_complete,
-						 &type, flags);
-}
-
 static int rev_list_insert_ref(struct fetch_negotiator *negotiator,
 			       const struct object_id *oid)
 {
-- 
2.47.0.277.g8800431eea-goog

[PATCH v3 2/2] fetch-pack: die if in commit graph but not obj db

[Jonathan Tan]

When fetching, there is a step in which sought objects are first checked
against the local repository; only objects that are not in the local
repository are then fetched. This check first looks up the commit graph
file, and returns "present" if the object is in there.

However, the action of first looking up the commit graph file is not
done everywhere in Git, especially if the type of the object at the time
of lookup is not known. This means that in a repo corruption situation,
a user may encounter an "object missing" error, attempt to fetch it, and
still encounter the same error later when they reattempt their original
action, because the object is present in the commit graph file but not in
the object DB.

Therefore, make it a fatal error when this occurs. (Note that we cannot
proceed to include this object in the list of objects to be fetched
without changing at least the fetch negotiation code: what would happen
is that the client will send "want X" and "have X" and when I tested
at $DAYJOB with a work server that uses JGit, the server reasonably
returned an empty packfile. And changing the fetch negotiation code to
only use the object DB when deciding what to report as "have" would be
an unnecessary slowdown, I think.)

This was discovered when a lazy fetch of a missing commit completed with
nothing actually fetched, and the writing of the commit graph file after
every fetch then attempted to read said missing commit, triggering a
lazy fetch of said missing commit, resulting in an infinite loop with no
user-visible indication (until they check the list of processes running
on their computer). With this fix, there is no infinite loop. Note that
although the repo corruption we discovered was caused by a bug in GC in
a partial clone, the behavior that this patch teaches Git to warn about
applies to any repo with commit graph enabled and with a missing commit,
whether it is a partial clone or not.

t5330, introduced in 3a1ea94a49 (commit-graph.c: no lazy fetch in
lookup_commit_in_graph(), 2022-07-01), tests that an interaction between
fetch and the commit graph does not cause an infinite loop. This patch
changes the exit code in that situation, so that test had to be changed.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 fetch-pack.c                               | 19 ++++++++++++++++---
 t/t5330-no-lazy-fetch-with-commit-graph.sh |  4 ++--
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 6728a0d2f5..fe1fb3c1b7 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -122,16 +122,29 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,
 		cb(negotiator, cache.items[i]);
 }
 
+static void die_in_commit_graph_only(const struct object_id *oid)
+{
+	die(_("You are attempting to fetch %s, which is in the commit graph file but not in the object database.\n"
+	      "This is probably due to repo corruption.\n"
+	      "If you are attempting to repair this repo corruption by refetching the missing object, use 'git fetch --refetch' with the missing object."),
+	      oid_to_hex(oid));
+}
+
 static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
-					       int mark_tags_complete)
+					       int mark_tags_complete_and_check_obj_db)
 {
 	enum object_type type;
 	struct object_info info = { .typep = &type };
 	struct commit *commit;
 
 	commit = lookup_commit_in_graph(the_repository, oid);
-	if (commit)
+	if (commit) {
+		if (mark_tags_complete_and_check_obj_db) {
+			if (!has_object(the_repository, oid, 0))
+				die_in_commit_graph_only(oid);
+		}
 		return commit;
+	}
 
 	while (1) {
 		if (oid_object_info_extended(the_repository, oid, &info,
@@ -143,7 +156,7 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
 
 			if (!tag->tagged)
 				return NULL;
-			if (mark_tags_complete)
+			if (mark_tags_complete_and_check_obj_db)
 				tag->object.flags |= COMPLETE;
 			oid = &tag->tagged->oid;
 		} else {
diff --git a/t/t5330-no-lazy-fetch-with-commit-graph.sh b/t/t5330-no-lazy-fetch-with-commit-graph.sh
index 5eb28f0512..21f36eb8c3 100755
--- a/t/t5330-no-lazy-fetch-with-commit-graph.sh
+++ b/t/t5330-no-lazy-fetch-with-commit-graph.sh
@@ -38,9 +38,9 @@ test_expect_success 'fetch any commit from promisor with the usage of the commit
 	git -C with-commit-graph config remote.origin.partialclonefilter blob:none &&
 	test_commit -C with-commit any-commit &&
 	anycommit=$(git -C with-commit rev-parse HEAD) &&
-	GIT_TRACE="$(pwd)/trace.txt" \
+	test_must_fail env GIT_TRACE="$(pwd)/trace.txt" \
 		git -C with-commit-graph fetch origin $anycommit 2>err &&
-	! grep "fatal: promisor-remote: unable to fork off fetch subprocess" err &&
+	test_grep ! "fatal: promisor-remote: unable to fork off fetch subprocess" err &&
 	grep "git fetch origin" trace.txt >actual &&
 	test_line_count = 1 actual
 '
-- 
2.47.0.277.g8800431eea-goog

Re: [PATCH v3 0/2] When fetching, die if in commit graph but not obj db

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Changes: the commit message title of the second patch, and a change from
> grep to test_grep.
>
> Jonathan Tan (2):
>   Revert "fetch-pack: add a deref_without_lazy_fetch_extended()"
>   fetch-pack: die if in commit graph but not obj db

I presume that with this the topic should be ready for 'next'.

Thanks.