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

[Stefan Beller <sbeller@google.com>]

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)

Thanks,
Stefan