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

Commit Message

René Scharfe Oct. 27, 2022, 4:35 p.m. UTC

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 <[e-mail]>
---
 builtin/bisect--helper.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

--
2.38.1

Comments

Ævar Arnfjörð Bjarmason Oct. 27, 2022, 10:26 p.m. UTC | #1

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

> 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 <[e-mail]>
> ---
>  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...

Taylor Blau Oct. 29, 2022, 6:16 p.m. UTC | #2

On Fri, Oct 28, 2022 at 12:26:51AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 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...

Hmmph. I don't think that it matters much, either way. I was initially
confused not having read the whole series, but I see what you're saying
now.

We see the same part of the diff twice here: the post-image of this hunk
to use run_command_v_opt() becomes the pre-image of a hunk in one of the
later patches to change it to use run_command() directly.

Arguably the two could be combined into a single patch, yes. Though I
find the version as written here to be clearer, since it's a
straightforward code movement within bisect--helper.

Thanks,
Taylor

Patch

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)
@@ -1177,7 +1182,6 @@  static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 {
 	int res = BISECT_OK;
 	struct strbuf command = STRBUF_INIT;
-	struct strvec run_args = STRVEC_INIT;
 	const char *new_state;
 	int temporary_stdout_fd, saved_stdout;
 	int is_first_run = 1;
@@ -1192,11 +1196,8 @@  static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 		return BISECT_FAILED;
 	}

-	strvec_push(&run_args, command.buf);
-
 	while (1) {
-		printf(_("running %s\n"), command.buf);
-		res = run_command_v_opt(run_args.v, RUN_USING_SHELL);
+		res = do_bisect_run(command.buf);

 		/*
 		 * Exit code 126 and 127 can either come from the shell
@@ -1206,7 +1207,7 @@  static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 		 * missing or non-executable script.
 		 */
 		if (is_first_run && (res == 126 || res == 127)) {
-			int rc = verify_good(terms, run_args.v);
+			int rc = verify_good(terms, command.buf);
 			is_first_run = 0;
 			if (rc < 0) {
 				error(_("unable to verify '%s' on good"
@@ -1273,7 +1274,6 @@  static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 	}

 	strbuf_release(&command);
-	strvec_clear(&run_args);
 	return res;
 }