Re: [PATCH 13/16] test-reach: test can_all_from_reach_with_flags

> On Mon, Jul 16, 2018 at 6:00 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> The can_all_from_reach_with_flags method is used by ok_to_give_up in
>> upload-pack.c to see if we have done enough negotiation during a fetch.
>> This method is intentionally created to preserve state between calls to
>> assist with stateful negotiation, such as over SSH.
>>
>> To make this method testable, add a new can_all_from_reach method that
>> does the initial setup and final tear-down. Call the method from
>> 'test-tool reach'.
>>
>> Since this is a many-to-many reachability query, add a new type of input
>> to the 'test-tool reach' input format. Lines "Y:<committish>" create a
>> list of commits to be the reachability targets from the commits in the
>> 'X' list. In the context of fetch negotiation, the 'X' commits are the
>> 'want' commits and the 'Y' commits are the 'have' commits.
> Makes sense. I shortly wondered if we want to s/Y/Z/ as I find X and Z
> more distinguishable than X/Y for reading/skimming.
>
> Thanks,
> Stefan
>
>> +++ b/commit-reach.c
>> @@ -593,3 +593,50 @@ int can_all_from_reach_with_flag(struct object_array *from,
>>          }
>>          return 1;
>>   }
>> +
>> +int can_all_from_reach(struct commit_list *from, struct commit_list *to,
>> +                      int cutoff_by_min_date)
> We'll put this method (that is only used by tests so far) here to
> not clutter the test tool code too much, or do we see more benefits
> from the code
> here? If so, docs would be nice.

>
>> +++ b/t/t6600-test-reach.sh
>> +test_expect_success 'can_all_from_reach:hit' '
>    [...]
>> +               Y:commit-7-3
>> +               Y:commit-8-1
>> +test_expect_success 'can_all_from_reach:miss' '
> [...]
>> +               Y:commit-8-5
> It would be nice if the difference in the list could be easier
> to spot as a reader. (There is a lot of repetition).
>
> Maybe we can teach "test-tool reach" to ignore input lines
> starting with '#' such that we can annotate the last line in
> the miss case?
>
> Why do we omit 7-3 in the miss case? (might be nice
> for symmetry to keep around)