Re: [PATCH v7 02/12] preload-index: add override to enable testing preload-index

[Stefan Beller <sbeller@google.com>]

On Tue, Sep 19, 2017 at 12:27 PM, Ben Peart <benpeart@microsoft.com> wrote:
> Preload index doesn't run unless it has a minimum number of 1000 files.
> To enable running tests with fewer files, add an environment variable
> (GIT_FORCE_PRELOAD_TEST) which will override that minimum and set it to 2.

'it' being the number of threads ('it' was not mentioned before,
so reading the commit message confused me initially)

>
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
>  preload-index.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/preload-index.c b/preload-index.c
> index 70a4c80878..75564c497a 100644
> --- a/preload-index.c
> +++ b/preload-index.c
> @@ -79,6 +79,8 @@ static void preload_index(struct index_state *index,
>                 return;
>
>         threads = index->cache_nr / THREAD_COST;
> +       if ((index->cache_nr > 1) && (threads < 2) && getenv("GIT_FORCE_PRELOAD_TEST"))
> +               threads = 2;

Adding these lines is just a bandaid to trick

>         if (threads < 2)
>                 return;

to not return early as the commit message does not discuss why
we set it to 2.

Do we need threads at all for these tests, or would a patch like

-    if (threads < 2)
+    if (threads < 2 && !GIT_FORCE_PRELOAD_TEST)
         return;

work as well?

That way tests can use any number of threads, though
they currently have no way of overriding the heuristic, yet.

With this alternative patch, it sounds to me as if the
issues kept more orthogonal. (specifically "Do we use preload?"
and "How many threads?". One could imagine that we later
want to introduce GIT_PRELOAD_THREADS for $reasons
and that would go over well in combination with
GIT_FORCE_PRELOAD_TEST)

It seems to be only an internal test variable, such that we do
not need documentation. (Is that worth mentioning in the
commit message?)

The test to make use of this new variable is found
in another patch I presume?

Stefan