Re: [PATCH v6 05/16] bisect--helper: make `terms` an explicit singleton

[Junio C Hamano <gitster@pobox.com>]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> We tried very hard to keep code in `builtin/bisect--helper.c` in as
> libifyable a state as possible.
>
> However, we are about to migrate this built-in to the `OPT_SUBCOMMAND()`
> API, which does not allow for passing any context (e.g. via a `void
> *data` parameters as they are used in the config API).
>
> Therefore, we _have_ to move the `terms` variable outside of
> `cmd_bisect__helper()` and explicitly make it a singleton (as it
> currently is, anyway).

Well, I do not find the above all that convincing.

I can see that the top-level functions that are called from
OPT_SUBCOMMAND(), because the only thing they get may be argc, argv,
and prefix, may need to access a global variable to keep track of
other states.  But all the helper functions that are called by them,
which are already "kept as libifyable as possible", can be called
with a pointer to that global variable by these subcommand functions
like cmd_bisect_{terms,start,...}().  As long as the libification
boundary is left relatively clean, I do not see the need to churn
this much code to change even the lowest level of helpers to access
the global.

I was actually imagining that these functions that (used to) take a
pointer to "struct bisect_terms" can be moved to the top-level
"bisect.c", together with the code already libified that drive the
"git bisect" program, as part of the "libified part of bisect
engine".

> -static void free_terms(struct bisect_terms *terms)
> +static void free_terms(void)
>  {
> -	FREE_AND_NULL(terms->term_good);
> -	FREE_AND_NULL(terms->term_bad);
> +	FREE_AND_NULL(terms.term_good);
> +	FREE_AND_NULL(terms.term_bad);
>  }

For example, at the tip of 'seen', free_term() are called by
get_terms() and cmd_bisect() and nothing else.  get_terms(), before
this patch deliberately made it less libifiable, used to take a
pointer to "struct bisect_terms", so it is just the matter of
passing what it got from its callers down to free_terms().

get_terms() in turn are called from many places, but as far as I can
see, all the callers are either

 - functions that are OPT_SUBCOMMAND() handler, which should take
   the pointer to the global &global_terms and pass it down to the
   callchain that are "kept libifiable" until this step, or

 - functions that used to take a pointer to "struct bisect_terms"
   before this step, and can just pass it down the callchain.

That is the impression after I tentatively modified the file at the
tip of 'seen' to

    static struct bisect_terms {
            char *term_good;
            char *term_bad;
    } global_terms;

and tried resurrect the "as libified as possible" state for some
call graphs.

Is there a function I missed that MUST access the global *and* also
access pointer given as a parameter?  If there is such a codepath,
that would indeed cause a confusion for future developers and leave
a maintenance burden because developers do not have a clear guidance
which one to use, but I do not think the current (i.e. the state
before this step) "as libifiable as possible" state is not that bad.

Thanks.