Re: [PATCH v2] fetch: commit references fetched before backfilling tags

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

Karthik Nayak <karthik.188@gmail.com> writes:

> When using batched updates, the references aren't yet committed to disk
> when we start backfilling tags. This means in situations such as shallow
> fetching the negotiation during backfilling tags, the client doesn't
> have any references to report in the 'have' section. Since backfilling
> doesn't use a depth limit, this can cause the server to send all the
> objects present in the repository.

Ouch.

> Fix this by committing the previous batched update and initiating a new
> one for backfilling tags. Also add a test which captures this regression.

> @@ -2076,6 +2076,27 @@ static int do_fetch(struct transport *transport,
>  		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
>  
>  		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
> +
> +		/*
> +		 * Backfilling tags has no depth limit. If we don't commit
> +		 * the fetched references, the backfill will report no refs
> +		 * in the 'have' section of the negotiation. This can cause
> +		 * the server to send all objects.
> +		 */

Does this problem only happen when the main fetch updated some ref,
or can backfilling make such a huge negotiation even when the main
fetch transaction is empty?

> +		if (tags_ref_map && !atomic_fetch) {
> +			retcode |= commit_ref_transaction(&transaction, false,
> +							  transport->remote->name,
> +							  &rejected_refs, &err);

This block is entered only when "--atomic" is not in effect, because
we want to update all refs or nothing, and these "all refs"
presumably includes the followed tags, but does it mean those who
want atomic fetch should be prepared to see their fetch degenerate
down to a full clone?

> +
> +			transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
> +								  REF_TRANSACTION_ALLOW_FAILURE, &err);
> +			if (!transaction) {
> +				free_refs(tags_ref_map);
> +				retcode = -1;
> +				goto cleanup;

Is this jumping a bit too far?  If our primary fetch have finished
successfully, and opening the next transaction only for tag backfill
task has failed, there are tons of processing happening we skip with
this new "goto" that we would have normally performed, starting with
computing summary_width used for later reporting, writing out
FETCH_HEAD, and setting upstream for the current branch, all of
which we should be able to do as long as the primary fetch
successfully finishes, no?

> +			}
> +		}

> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 300bd5396d..5ba372fbcb 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -1942,6 +1942,22 @@ test_expect_success "backfill tags when providing a refspec" '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'shallow fetch does not fetch objects again for tags' '
> +	test_when_finished rm -rf source target &&
> +
> +	git init source &&
> +	test_commit_bulk -C source 10 &&
> +	git -C source tag -a tag -m tag HEAD~2 &&
> +	root_oid=$(git -C source rev-parse HEAD~9) &&
> +
> +	git init target &&
> +	git -C target remote add origin ../source &&
> +	git -C target fetch --depth 5 origin &&
> +
> +	# Should not fetch objects beyond the pre-specified depth.
> +	test_must_fail git -C target cat-file -e "$root_oid"
> +'
> +
>  test_expect_success REFFILES "FETCH_HEAD is updated even if ref updates fail" '
>  	test_when_finished rm -rf base repo &&
>  
>
> ---
> base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
> change-id: 20261007-799-shallow-fetch-with-tags-d62b3efb29f0
>
>
> Thanks
> - Karthik