Re: [PATCH 2/8] bisect--helper: factor out do_bisect_run()

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 27, 2022, 22:26 UTC
Message-ID: <221028.86v8o4aoiq.gmgdl@evledraar.gmail.com>
In-Reply-To: <033ebd37-40c7-01bc-e9bb-29d738532125@web.de>

On Thu, Oct 27 2022, René Scharfe wrote:

Show 41 quoted lines

> Deduplicate the code for reporting and starting the bisect run command
> by moving it to a short helper function.  Use a string array instead of
> a strvec to prepare the arguments, for simplicity.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  builtin/bisect--helper.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 28ef7ec2a4..70d1e9d1ad 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -1142,8 +1142,14 @@ static int get_first_good(const char *refname UNUSED,
>  	return 1;
>  }
>
> -static int verify_good(const struct bisect_terms *terms,
> -		       const char **quoted_argv)
> +static int do_bisect_run(const char *command)
> +{
> +	const char *argv[] = { command, NULL };
> +	printf(_("running %s\n"), command);
> +	return run_command_v_opt(argv, RUN_USING_SHELL);
> +}
> +
> +static int verify_good(const struct bisect_terms *terms, const char *command)
>  {
>  	int rc;
>  	enum bisect_error res;
> @@ -1163,8 +1169,7 @@ static int verify_good(const struct bisect_terms *terms,
>  	if (res != BISECT_OK)
>  		return -1;
>
> -	printf(_("running %s\n"), quoted_argv[0]);
> -	rc = run_command_v_opt(quoted_argv, RUN_USING_SHELL);
> +	rc = do_bisect_run(command);
>
>  	res = bisect_checkout(&current_rev, no_checkout);
>  	if (res != BISECT_OK)
> [...]

Okey, so we end up with a helper just for the convenience of doing that printf at the start, that's fine.

But given the line count of some of the other changes, and e.g. including the free(), oid_to_hex_r() to oid_to_hex() etc. in later commits I don't see why we can't just make it use run_command() directly.

I.e. I think it makes sense as one commit, but the conversion is easy enough, easier than looking at the same code again later in the series...