Re: [PATCH] bisect--helper: use BISECT_TERMS in 'bisect skip' command

From: Bagas Sanjaya <bagasdotme@gmail.com>
Date: Apr 25, 2021, 08:16 UTC
Message-ID: <473a11db-cbb5-58b9-b05d-cab2072d5d2f@gmail.com>
In-Reply-To: <fd40e12f-9649-1327-4bdb-dce5b5eed619@ramsayjones.plus.com>

On 25/04/21 07.18, Ramsay Jones wrote:

Show 6 quoted lines

> 
> This patch was created directly on top of commit e4c7b33747 and tested
> with the test from Bagas Sanjaya [1] (ie the second version of the
> stand-alone test file t6031-*.sh, rather than the newer patch that
> adds the test to t6030-*.sh). I applied this patch to the current
> master branch (@311531c9de55) and it also passed the test in [1].

I have just sent v2 of t6030 version of my test [1]. Please test this patch against that v2 test. And if the test passes (breakage vanished), please update the test by do instructions in the FIXME comment lines of [1].

Show 5 quoted lines

> @@ -1129,6 +1129,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>   		break;
>   	case BISECT_SKIP:
>   		set_terms(&terms, "bad", "good");
> +		get_terms(&terms);

I'm not fluent in C, but I read these lines above as "ok, we set terms from &terms, bad and good as fallback in case the reference is empty; then we get these terms from the reference". Wouldn't it makes sense if we can say "get the terms from &terms" first, then "set the terms from the reference, if empty use bad and good as fallback"?

[1]: https://lore.kernel.org/git/20210425080508.154159-1-bagasdotme@gmail.com/

-- 
An old man doll... just what I always wanted! - Clara