Re: [PATCH v2 2/2] commit-graph.c: no lazy fetch in lookup_commit_in_graph()

Han Xin <hanxin.hx@bytedance.com> writes:

Hmph, is that a fundamental requirement, or is that a limitation of the current implementation? Naïvely, I do not quite see why we cannot first partially clone from a remote, access objects that locally do not exist and lazily fetch them from the promissor, and then build a reachability graph. I expect that the resulting commit graph records the lazily fetched objects at that point. And then we should be able to "lose" the lazily fetched objects that we know we can fetch from the promissor again when we need them in the future. And we would be in a situation where commits are pruned away, not locally available in our object store, but can be (re)fetched from the promisor, no?

It all depends on the reason we call lookup_commit_in_graph(), I think. Is there an easy way to remember the fact that we are checking if object X is here with repo_has_object_file(X), so that an on-demand fetch that happens when X does not locally exist would not bother checking with lookup_commit_in_graph()? IOW, temporarily disable the use of commit-graph when we are lazily fetching?

Is that a mode of "succeed"-ing? Or merely a way to exit an endless loop that does not make any progress with a failure?

I think the single-liner change in the patch is a good one, but I am having a hard time to agree with the reasoning above that explains why it is a good change.

Here is an attempt to express a reasoning I can understand, can agree with, and (I think) better describes why the change is a good one. Does my understanding of the problem and the solution totally misses the mark?

	The commit-graph is used to opportunistically optimize
	accesses to certain pieces of information on commit objects,
	and lookup_commit_in_graph() tries to say "no" when the
	requested commit does not locally exist by returning NULL,
	in which case the caller can ask for (which may result in
	on-demand fetching from a promisor remote) and parse the
	commit object itself.

	However, it uses a wrong helper, repo_has_object_file(), to
	do so.  This helper not only checks if an object is
	immediately available in the local object store, but also
	tries to fetch from a promisor remote.  But the fetch
	machinery calls lookup_commit_in_graph(), thus causing an
	infinite loop.

	We should make lookup_commit_in_graph() expect that a commit
	given to it can be legitimately missing from the local
	object store, by using the has_object_file() helper instead.
	

Hmph, does this short-test need a completely new file?

somthing? something?

Doesn't this deliberately _corrupt_ the with-commit-graph repository that depended on the object whose name is $oid in with-commit repository? Do we require a corrupt repository to trigger the "bug"?

anycommit?  another_commit?  Be consistent in naming.

So we did all of the above set-up sequences only to skip the most interesting test, if we were testing with "dash"? I suspect that it may be cleaner to put the prerequisite to the whole file with the "early test_done" trick like t0051 and t3008.

Thanks.