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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 30, 2022, 21:11 UTC
Message-ID: <xmqqmtblighr.fsf@gitster.g>
In-Reply-To: <92b3b116ef8f879192d9deb94d68b73e29d5dcd6.1661885419.git.gitgitgadget@gmail.com>

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

Show 12 quoted lines

> 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".

Show 8 quoted lines

> -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.