From: Derrick Stolee <stolee@gmail.com>
Date: Tue, 12 May 2026 17:55:02 GMT
Subject: Re: [PATCH v3 6/7] remote: add remote.*.negotiationInclude config
Message-ID: <019912f6-0dd7-4224-8c4d-3dc26b1b85c6@gmail.com>
In-Reply-To: <VI0PR03MB11634F3D6B345482BD992FDC9C0392@VI0PR03MB11634.eurprd03.prod.outlook.com>

On 5/12/26 10:54 AM, Matthew John Cheetham wrote:
> On 2026-04-22 16:25, Derrick Stolee via GitGitGadget wrote:

> This patch is a mirror of patch 4 that added the remote config for
> negotiateRestrict. Some of the same comments apply here too:
> 
> - reusing `parse_transport_option()` vs inline resetting the list
> 
> - values could be commit SHAs as well as refs/globs

Will do. Thanks.

>> diff --git a/Documentation/config/remote.adoc b/Documentation/config/remote.adoc
>> index f1d889d03e..44de6d3c1f 100644
>> --- a/Documentation/config/remote.adoc
>> +++ b/Documentation/config/remote.adoc
>> @@ -126,6 +126,33 @@ values are not used.
>>   Blank values signal to ignore all previous values, allowing a reset of
>>   the list from broader config scenarios.
>> +remote.<name>.negotiationInclude::
>> +    When negotiating with this remote during `git fetch` and `git push`,
>> +    the client advertises a list of commits that exist locally.  In
>> +    repos with many references, this list of "haves" can be truncated.
>> +    Depending on data shape, dropping certain references may be
>> +    expensive.  This multi-valued config option specifies ref patterns
>> +    whose tips should always be sent as "have" commits during fetch
>> +    negotiation with this remote.
>> ++
>> +Each value is either an exact ref name (e.g. `refs/heads/release`) or a
>> +glob pattern (e.g. `refs/heads/release/*`).  The pattern syntax is the same
>> +as for `--negotiation-restrict`.
> 
> Should this say "..same as for `--negotiation-include`"?
> 
> This way each `remote.<name>.negotiationX` doc cross-references the
> corresponding `--negotiation-X` command line option.

Good find. The rest of the description uses *-include.

>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>> index 4316f8d4ea..db73ed5379 100755
>> --- a/t/t5510-fetch.sh
>> +++ b/t/t5510-fetch.sh
>> @@ -1577,6 +1577,55 @@ test_expect_success '--negotiation-include avoids 
>> duplicates with negotiator' '
>>       test_line_count = 1 matches
>>   '
>> +test_expect_success 'remote.<name>.negotiationInclude used as default for -- 
>> negotiation-include' '
>> +    test_when_finished rm -f trace &&
>> +    setup_negotiation_tip server server 0 &&
>> +
>> +    # test the reset of the list on an empty value
>> +    git -C client config --add remote.origin.negotiationInclude refs/tags/ 
>> alpha_1 &&
>> +    git -C client config --add remote.origin.negotiationInclude "" &&
>> +    git -C client config --add remote.origin.negotiationInclude refs/tags/ 
>> beta_1 &&
>> +    GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch \
>> +        --negotiation-restrict=alpha_1 \
>> +        origin alpha_s beta_s &&
>> +
>> +    ALPHA_1=$(git -C client rev-parse alpha_1) &&
>> +    test_grep "fetch> have $ALPHA_1" trace &&
>> +    BETA_1=$(git -C client rev-parse beta_1) &&
>> +    test_grep "fetch> have $BETA_1" trace
>> +'
> 
> This test sets up the include list as [alpha_1, "", beta_1] which after
> the reset should become [beta_1], but the assertions in the test only
> check that alpha_1 (sent via the --negotiation-restrict option) and
> beta_1 (sent via the include) appear. If the reset of the list didn't
> work then the test still passes because alpha_1 is sent via the CLI
> option.

Good point. the negotiation-restrict option is making this less clear.
If I point the restrict option at alpha_2, then I think it exercises
things correctly.

Thanks,
-Stolee