diff mbox series

[v3,1/3] bisect--helper: remove unused options

Message ID 6b80fd93980ec5171fe0637cbd1a8173a5337da4.1668097286.git.congdanhqx@gmail.com (mailing list archive)
State Accepted
Commit b63e7e99649653509e3e0a4cb53d58289f1a8fcc
Headers show
Series Convert git-bisect--helper to OPT_SUBCOMMAND | expand

Commit Message

Đoàn Trần Công Danh Nov. 10, 2022, 4:36 p.m. UTC 
'git-bisect.sh' used to have a 'bisect_next_check' to check if we have
both good/bad, old/new terms set or not.  In commit 129a6cf344
(bisect--helper: `bisect_next_check` shell function in C, 2019-01-02),
a subcommand for bisect--helper was introduced to port the check to C.
Since d1bbbe45df (bisect--helper: reimplement `bisect_run` shell
function in C, 2021-09-13), all users of 'bisect_next_check' was
re-implemented in C, this subcommand was no longer used but we forgot
to remove '--bisect-next-check'.

'git-bisect.sh' also used to have a 'bisect_write' function, whose
third positional parameter was a "nolog" flag.  This flag was only used
when 'bisect_start' invoked 'bisect_write' to write the starting good
and bad revisions.  Then 0f30233a11 (bisect--helper: `bisect_write`
shell function in C, 2019-01-02) ported it to C as a command mode of
'bisect--helper', which (incorrectly) added the '--no-log' option,
and convert the only place ('bisect_start') that call 'bisect_write'
with 'nolog' to 'git bisect--helper --bisect-write' with 'nolog'
instead of '--no-log', since 'bisect--helper' has command modes not
subcommands, all other command modes see and handle that option as well.
This bogus state didn't last long, however, because in the same patch
series 06f5608c14 (bisect--helper: `bisect_start` shell function
partially in C, 2019-01-02) the C reimplementation of bisect_start()
started calling the bisect_write() C function, this time with the
right 'nolog' function parameter. From then on there was no need for
the '--no-log' option in 'bisect--helper'. Eventually all bisect
subcommands were ported to C as 'bisect--helper' command modes, each
calling the bisect_write() C function instead, but when the
'--bisect-write' command mode was removed in 68efed8c8a
(bisect--helper: retire `--bisect-write` subcommand, 2021-02-03) it
forgot to remove that '--no-log' option.
'--no-log' option had never been used and it's unused now.

Let's remove --bisect-next-check and --no-log from option parsing.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 builtin/bisect--helper.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Nov. 11, 2022, 12:42 p.m. UTC | #1 
On Thu, Nov 10 2022, Đoàn Trần Công Danh wrote:

> 'git-bisect.sh' used to have a 'bisect_next_check' to check if we have
> both good/bad, old/new terms set or not.  In commit 129a6cf344
> (bisect--helper: `bisect_next_check` shell function in C, 2019-01-02),
> a subcommand for bisect--helper was introduced to port the check to C.
> Since d1bbbe45df (bisect--helper: reimplement `bisect_run` shell
> function in C, 2021-09-13), all users of 'bisect_next_check' was
> re-implemented in C, this subcommand was no longer used but we forgot
> to remove '--bisect-next-check'.



> 'git-bisect.sh' also used to have a 'bisect_write' function, whose
> third positional parameter was a "nolog" flag.  This flag was only used
> when 'bisect_start' invoked 'bisect_write' to write the starting good
> and bad revisions.  Then 0f30233a11 (bisect--helper: `bisect_write`
> shell function in C, 2019-01-02) ported it to C as a command mode of
> 'bisect--helper', which (incorrectly) added the '--no-log' option,
> and convert the only place ('bisect_start') that call 'bisect_write'
> with 'nolog' to 'git bisect--helper --bisect-write' with 'nolog'
> instead of '--no-log', since 'bisect--helper' has command modes not
> subcommands, all other command modes see and handle that option as well.
> This bogus state didn't last long, however, because in the same patch
> series 06f5608c14 (bisect--helper: `bisect_start` shell function
> partially in C, 2019-01-02) the C reimplementation of bisect_start()
> started calling the bisect_write() C function, this time with the
> right 'nolog' function parameter. From then on there was no need for
> the '--no-log' option in 'bisect--helper'. Eventually all bisect
> subcommands were ported to C as 'bisect--helper' command modes, each
> calling the bisect_write() C function instead, but when the
> '--bisect-write' command mode was removed in 68efed8c8a
> (bisect--helper: retire `--bisect-write` subcommand, 2021-02-03) it
> forgot to remove that '--no-log' option.
> '--no-log' option had never been used and it's unused now.

FWIW I think this very long backstory can be condensed to just (mainly
trying to get some paragraph breaks in there):

	The "--no-log" option has never been used. It was added in
	0f30233a11f (bisect--helper: `bisect_write` shell function in C,
	2019-01-02).

        In that commit bisect_write() was also changed to take a
	"no_log" function parameter, with nothing providing the
	"--no-log" option, and "bisect--helper --bisect-write" being
	given an unused "nolog" parameter.

        Then, later in the same series 06f5608c14e (bisect--helper:
        `bisect_start` shell function partially in C, 2019-01-02) the
        code passing the unused "nolog" parameter went away, while
        leaving us with this unused "--no-log" code.

> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>  builtin/bisect--helper.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 1d2ce8a0e1..5ec2e67f59 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -1283,7 +1283,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>  	enum {
>  		BISECT_RESET = 1,
> -		BISECT_NEXT_CHECK,
>  		BISECT_TERMS,
>  		BISECT_START,
>  		BISECT_AUTOSTART,
> @@ -1295,12 +1294,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		BISECT_VISUALIZE,
>  		BISECT_RUN,
>  	} cmdmode = 0;
> -	int res = 0, nolog = 0;
> +	int res = 0;
>  	struct option options[] = {
>  		OPT_CMDMODE(0, "bisect-reset", &cmdmode,
>  			 N_("reset the bisection state"), BISECT_RESET),
> -		OPT_CMDMODE(0, "bisect-next-check", &cmdmode,
> -			 N_("check whether bad or good terms exist"), BISECT_NEXT_CHECK),
>  		OPT_CMDMODE(0, "bisect-terms", &cmdmode,
>  			 N_("print out the bisect terms"), BISECT_TERMS),
>  		OPT_CMDMODE(0, "bisect-start", &cmdmode,
> @@ -1319,8 +1316,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  			 N_("visualize the bisection"), BISECT_VISUALIZE),
>  		OPT_CMDMODE(0, "bisect-run", &cmdmode,
>  			 N_("use <cmd>... to automatically bisect"), BISECT_RUN),
> -		OPT_BOOL(0, "no-log", &nolog,
> -			 N_("no log for BISECT_WRITE")),
>  		OPT_END()
>  	};
>  	struct bisect_terms terms = { .term_good = NULL, .term_bad = NULL };

This looks good. Is it your original work, or did the signed-off-by's go
missing along the way? I.e. in the greater history of this topic this
comes from Johannes's:

	https://lore.kernel.org/git/1e43148864a52ffe05b5075bd0e449c0e056f078.1661885419.git.gitgitgadget@gmail.com/
	https://lore.kernel.org/git/05262b6a7d1b20a0d2f2ca2090be284ffb8c679c.1661885419.git.gitgitgadget@gmail.com/

Which, when I re-rolled it I carried forward as:

	https://lore.kernel.org/git/patch-04.13-b10deee4827-20221104T132117Z-avarab@gmail.com/

So I assumed you went back and looked at the original topic...

In any case, if you are doing a "misc little cleanups" series, I think
lifting some more from Johannes's original topic might be useful> I
dropped some in my 13 patch re-roll to keep it more focused on just the
"bisect built-in" goal, e.g. there's:

	https://lore.kernel.org/git/f2132b61ff7d7959fd8efcd9d416736b154718f0.1661885419.git.gitgitgadget@gmail.com/
	https://lore.kernel.org/git/4f93692e071cf316fd391344b5dbbc995c162232.1661885419.git.gitgitgadget@gmail.com/

But maybe those are better left out/or submitted as *another* topic,
just pointing them out in case you missed them...
diff mbox series

Patch

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 1d2ce8a0e1..5ec2e67f59 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -1283,7 +1283,6 @@  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		BISECT_RESET = 1,
-		BISECT_NEXT_CHECK,
 		BISECT_TERMS,
 		BISECT_START,
 		BISECT_AUTOSTART,
@@ -1295,12 +1294,10 @@  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		BISECT_VISUALIZE,
 		BISECT_RUN,
 	} cmdmode = 0;
-	int res = 0, nolog = 0;
+	int res = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "bisect-reset", &cmdmode,
 			 N_("reset the bisection state"), BISECT_RESET),
-		OPT_CMDMODE(0, "bisect-next-check", &cmdmode,
-			 N_("check whether bad or good terms exist"), BISECT_NEXT_CHECK),
 		OPT_CMDMODE(0, "bisect-terms", &cmdmode,
 			 N_("print out the bisect terms"), BISECT_TERMS),
 		OPT_CMDMODE(0, "bisect-start", &cmdmode,
@@ -1319,8 +1316,6 @@  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("visualize the bisection"), BISECT_VISUALIZE),
 		OPT_CMDMODE(0, "bisect-run", &cmdmode,
 			 N_("use <cmd>... to automatically bisect"), BISECT_RUN),
-		OPT_BOOL(0, "no-log", &nolog,
-			 N_("no log for BISECT_WRITE")),
 		OPT_END()
 	};
 	struct bisect_terms terms = { .term_good = NULL, .term_bad = NULL };