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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jun 21, 2022, 18:23 UTC
Message-ID: <20220621182322.3444926-1-jonathantanmy@google.com>
In-Reply-To: <20220618030130.36419-1-hanxin.hx@bytedance.com>

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

Show 8 quoted lines

> If a commit is in the commit graph, we would expect the commit to also
> be present. So we should use has_object() instead of
> repo_has_object_file(), which will help us avoid getting into an endless
> loop of lazy fetch.
> 
> We can see the endless loop issue via this[1].
> 
> 1. https://lore.kernel.org/git/20220612161707.21807-1-chiyutianyi@gmail.com/

As described in SubmittingPatches:

  Try to make sure your explanation can be understood   
  without external resources. Instead of giving a URL to a mailing list
  archive, summarize the relevant points of the discussion.

Show 7 quoted lines

> +test_expect_success 'setup' '
> +	git init --bare dest.git &&
> +	test_commit one &&
> +	git checkout -b tmp &&
> +	test_commit two &&
> +	git push dest.git --all
> +'

You can commit directly to the repo by using "test_commit -C dest.git". Also, can the repositories be better named? I see a "dest.git" (which seems to contain all the objects), "alternates" (which seems to contain everything except refs/heads/tmp), "source" (which only contains the commit graph), and the current directory. It would probably be better to name them e.g. "with-commit", "without-commit", "only-commit-graph", and omit using the current directory altogether.

Show 9 quoted lines

> +test_expect_success 'prepare a alternates repository without commit two' '
> +	git clone --bare dest.git alternates &&
> +	oid=$(git -C alternates rev-parse refs/heads/tmp) &&
> +	git -C alternates update-ref -d refs/heads/tmp &&
> +	git -C alternates gc --prune=now &&
> +	pack=$(echo alternates/objects/pack/*.pack) &&
> +	git verify-pack -v "$pack" >have &&
> +	! grep "$oid" have
> +'

OK, except refs/heads/tmp could probably have a better name.

Show 14 quoted lines

> +test_expect_success 'prepare a repository with a commit-graph contains commit two' '
> +	git init source &&
> +	echo "$(pwd)/dest.git/objects" >source/.git/objects/info/alternates &&
> +	git -C source remote add origin "$(pwd)/dest.git" &&
> +	git -C source config remote.origin.promisor true &&
> +	git -C source config remote.origin.partialclonefilter blob:none &&
> +	# the source repository has the whole refs contains refs/heads/tmp
> +	git -C source fetch origin &&
> +	(
> +		cd source &&
> +		test_commit three &&
> +		git -c gc.writeCommitGraph=true gc
> +	)
> +'

Is the purpose of the fetch only to add a ref? If yes, it's clearer just to create that branch instead of fetching.

> +test_expect_success 'change the alternates of source to that without commit two' '
> +	# now we have a commit-graph in the source repository but without the commit two
> +	echo "$(pwd)/alternates/objects" >source/.git/objects/info/alternates
> +'

OK.

> +test_expect_success 'fetch the missing commit' '
> +	git -C source fetch origin $oid 2>fetch.out &&
> +	grep "$oid" fetch.out
> +'

Is the bug triggered by fetching the missing commit or by fetching any commit (which triggers the usage of the commit graph)? If any commit, then it's clearer to create an arbitrary commit and then fetch it.

Also, I thought that the issue was an infinite loop, and the thing being tested here looks different from that. If you want to ensure that nothing is being fetched, you can use GIT_TRACE="$(pwd)/trace" to observe a fetch-pack command being invoked or GIT_TRACE_PACKET="$(pwd)/trace" to observe the packet being sent. If you're worried about an infinite loop, you can set origin to a directory that does not exist (so that the fetch immediately fails).