Re: [PATCH 2/2] fetch-pack.c: do not declare local commits as "have" in partial repos

[Junio C Hamano <gitster@pobox.com>]

Calvin Wan <calvinwan@google.com> writes:

> In a partial repository, creating a local commit and then fetching
> causes the following state to occur:
>
> commit  tree  blob
>  C3 ---- T3 -- B3 (fetched from remote, in promisor pack)
>  |
>  C2 ---- T2 -- B2 (created locally, in non-promisor pack)
>  |
>  C1 ---- T1 -- B1 (fetched from remote, in promisor pack)
>
> During garbage collection, parents of promisor objects are marked as
> UNINTERESTING and are subsequently garbage collected. In this case, C2
> would be deleted and attempts to access that commit would result in "bad
> object" errors (originally reported here[1]).

Understandable.

> This is not a bug in gc since it should be the case that parents of
> promisor objects are also promisor objects (fsck assumes this as
> well).

I am not sure where this "not a bug" claim comes from.  Here, the
definition of "promisor objects" seems to be anything that are
reachable from objects in promisor packs, but isn't the source of
the bug that collects C2 exactly that "gc" uses such a definition
for discardable objects that can be refetchd from promisor remotes?

> When promisor objects are fetched, the state of the repository
> should ensure that the above holds true. Therefore, do not declare local
> commits as "have" in partial repositores so they can be fetched into a
> promisor pack.

Could you clarify what it means in the context of the above example
you gave in an updated version of the proposed log message?

We pretend that C2 and anything it reaches do not exist locally, to
force them to be fetched from the remote?  We'd end up having two
copies of C2 (one that we created locally and had before starting
this fetch, the other we fetched when we fetched C3 from them)?
This sounds like it is awfully inefficient both network bandwidth-
and local disk-wise.

I was hoping to see that the issue can be fixed on the "gc" side,
regardless of how the objects enter our repository, but perhaps I am
missing something.  Isn't it just the matter of collecting C1, C3
but not C2?  Or to put it another way, if we first create a list of
objects to be packed (regardless of whether they are in promisor
packs), and then remove the objects that are in promisor packs from
the list, and pack the objects still remaining in the list?

> diff --git a/fetch-pack.c b/fetch-pack.c
> index 58b4581ad8..c39b0f6ad4 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1297,12 +1297,23 @@ static void add_common(struct strbuf *req_buf, struct oidset *common)
>  
>  static int add_haves(struct fetch_negotiator *negotiator,
>  		     struct strbuf *req_buf,
> -		     int *haves_to_send)
> +		     int *haves_to_send,
> +		     int from_promisor)
>  {
>  	int haves_added = 0;
>  	const struct object_id *oid;
>  
>  	while ((oid = negotiator->next(negotiator))) {
> +		/* 
> +		 * In partial repos, do not declare local objects as "have"
> +		 * so that they can be fetched into a promisor pack. Certain
> +		 * operations mark parent commits of promisor objects as
> +		 * UNINTERESTING and are subsequently garbage collected so
> +		 * this ensures local commits are still available in promisor
> +		 * packs after a fetch + gc.
> +		 */
> +		if (from_promisor && !is_in_promisor_pack(oid, 0))
> +			continue;
>  		packet_buf_write(req_buf, "have %s\n", oid_to_hex(oid));
>  		if (++haves_added >= *haves_to_send)
>  			break;
> @@ -1405,7 +1416,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
>  	/* Add all of the common commits we've found in previous rounds */
>  	add_common(&req_buf, common);
>  
> -	haves_added = add_haves(negotiator, &req_buf, haves_to_send);
> +	haves_added = add_haves(negotiator, &req_buf, haves_to_send, args->from_promisor);
>  	*in_vain += haves_added;
>  	trace2_data_intmax("negotiation_v2", the_repository, "haves_added", haves_added);
>  	trace2_data_intmax("negotiation_v2", the_repository, "in_vain", *in_vain);
> @@ -2178,7 +2189,7 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
>  
>  		packet_buf_write(&req_buf, "wait-for-done");
>  
> -		haves_added = add_haves(&negotiator, &req_buf, &haves_to_send);
> +		haves_added = add_haves(&negotiator, &req_buf, &haves_to_send, 0);
>  		in_vain += haves_added;
>  		if (!haves_added || (seen_ack && in_vain >= MAX_IN_VAIN))
>  			last_iteration = 1;
> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> index 8415884754..cba9f7ed9b 100755
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -693,6 +693,35 @@ test_expect_success 'lazy-fetch in submodule succeeds' '
>  	git -C client restore --recurse-submodules --source=HEAD^ :/
>  '
>  
> +test_expect_success 'fetching from promisor remote fetches previously local commits' '
> +	# Setup
> +	git init full &&
> +	git -C full config uploadpack.allowfilter 1 &&
> + 	git -C full config uploadpack.allowanysha1inwant 1 &&
> +	touch full/foo &&
> +	git -C full add foo &&
> +	git -C full commit -m "commit 1" &&
> +	git -C full checkout --detach &&
> +
> +	# Partial clone and push commit to remote
> +	git clone "file://$(pwd)/full" --filter=blob:none partial &&
> +	echo "hello" > partial/foo &&
> +	git -C partial commit -a -m "commit 2" &&
> +	git -C partial push &&
> +
> +	# gc in partial repo
> +	git -C partial gc --prune=now &&
> +
> +	# Create another commit in normal repo
> +	git -C full checkout main &&
> +	echo " world" >> full/foo &&
> +	git -C full commit -a -m "commit 3" &&
> +
> +	# Pull from remote in partial repo, and run gc again
> +	git -C partial pull &&
> +	git -C partial gc --prune=now
> +'
> +
>  . "$TEST_DIRECTORY"/lib-httpd.sh
>  start_httpd