Re: [PATCH 5/8] clone: set fetch.bundleURI if appropriate

[Derrick Stolee <derrickstolee@github.com>]

On 1/19/2023 2:42 PM, Victoria Dye wrote:
> Derrick Stolee via GitGitGadget wrote:
>> diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
>> index cd65d236b43..4f796218aab 100644
>> --- a/Documentation/config/fetch.txt
>> +++ b/Documentation/config/fetch.txt
>> @@ -96,3 +96,11 @@ fetch.writeCommitGraph::
>>  	merge and the write may take longer. Having an updated commit-graph
>>  	file helps performance of many Git commands, including `git merge-base`,
>>  	`git push -f`, and `git log --graph`. Defaults to false.
>> +
>> +fetch.bundleURI::
>> +	This value stores a URI for fetching Git object data from a bundle URI
>> +	before performing an incremental fetch from the origin Git server. If
>> +	the value is `<uri>` then running `git fetch <args>` is equivalent to
>> +	first running `git fetch --bundle-uri=<uri>` immediately before
>> +	`git fetch <args>`. See details of the `--bundle-uri` option in
>> +	linkgit:git-fetch[1].
>
> Since it's not mentioned from this or any other user-facing documentation
> (AFAICT), could you note that this value is set automatically by 'git clone'
> iff '--bundle-uri' is specified *and* 'bundle.heuristic' is set for the
> initially downloaded bundle list?

Can do.

> It would also be nice to make note of that behavior in the documentation of
> the '--bundle-uri' option in 'Documentation/git-clone.txt', since command
> documentation in general seems to be more popular/visible to users than
> config docs.

Yes. I also thought that I had updated this documentation to not refer
to 'git fetch --bundle-uri', which doesn't exist anymore since an earlier
RFC version. I'll be sure to update that, too.

>>  	strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD");
>> diff --git a/bundle-uri.c b/bundle-uri.c
>> index b30c85ba6f2..1dbbbb980eb 100644
>> --- a/bundle-uri.c
>> +++ b/bundle-uri.c
>> @@ -594,9 +594,10 @@ static int fetch_bundle_list_in_config_format(struct repository *r,
>>  	 * it advertises are expected to be bundles, not nested lists.
>>  	 * We can drop 'global_list' and 'depth'.
>>  	 */
>> -	if (list_from_bundle.heuristic == BUNDLE_HEURISTIC_CREATIONTOKEN)
>> +	if (list_from_bundle.heuristic == BUNDLE_HEURISTIC_CREATIONTOKEN) {
>>  		result = fetch_bundles_by_token(r, &list_from_bundle);
>> -	else if ((result = download_bundle_list(r, &list_from_bundle,
>> +		global_list->heuristic = BUNDLE_HEURISTIC_CREATIONTOKEN;
>
> If the 'heuristic' field already existed and was being used to apply
> bundles, why wasn't 'global_list->heuristic' already being set? Before this
> patch, was the 'global_list->heuristic' field not accurately reflecting the
> heuristic type of a given bundle list?
>
> If so, I think it'd make sense to move this section to patch 4 [1], since
> that's when the heuristic is first applied to the bundle list.
>
> [1] https://lore.kernel.org/git/57c0174d3752fb61a05e0653de9d3057616ed16a.1673037405.git.gitgitgadget@gmail.com/

Can do.

Thanks,
-Stolee