"Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
writes:
> From: Johannes Schindelin <johannes.schindelin@xxxxxx>
>
> 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.
