| Message ID | eddbdde222a01113d8facdcb17d6ec85676edbe7.1643328752.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Finish converting git bisect into a built-in | expand |
On Fri, Jan 28, 2022 at 3:52 PM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > In preparation for making `git bisect` a real built-in, let's prepare > the `bisect--helper` built-in to handle `git bisect--helper good` and > `git bisect--helper bad`, i.e. do not require the `--bisect-state` > option to be passed explicitly. > > To prepare for converting `bisect--helper` to a full built-in > implementation of `git bisect` (which must not require the > `--bisect-state` option to be specified at all), let's move the handling > toward the end of the `switch (cmdmode)` block. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > builtin/bisect--helper.c | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index cc5a9ca41b9..219fa99cd0b 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -26,8 +26,8 @@ static const char * const git_bisect_helper_usage[] = { > N_("git bisect--helper --bisect-start [--term-{new,bad}=<term> --term-{old,good}=<term>]" > " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"), > N_("git bisect--helper --bisect-next"), > - N_("git bisect--helper --bisect-state (bad|new) [<rev>]"), > - N_("git bisect--helper --bisect-state (good|old) [<rev>...]"), > + N_("git bisect--helper [--bisect-state] (bad|new) [<rev>]"), > + N_("git bisect--helper [--bisect-state] (good|old) [<rev>...]"), > N_("git bisect--helper --bisect-replay <filename>"), > N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"), > N_("git bisect--helper --bisect-visualize"), > @@ -1210,6 +1210,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > git_bisect_helper_usage, > PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN); > > + if (!cmdmode && argc > 0) { > + set_terms(&terms, "bad", "good"); > + get_terms(&terms); > + if (!check_and_set_terms(&terms, argv[0])) > + cmdmode = BISECT_STATE; > + } > + > if (!cmdmode) > usage_with_options(git_bisect_helper_usage, options); > > @@ -1218,11 +1225,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > set_terms(&terms, "bad", "good"); > res = bisect_start(&terms, argv, argc); > break; > - case BISECT_STATE: > - set_terms(&terms, "bad", "good"); > - get_terms(&terms); > - res = bisect_state(&terms, argv, argc); > - break; > case BISECT_TERMS: > if (argc > 1) > return error(_("--bisect-terms requires 0 or 1 argument")); > @@ -1265,6 +1267,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > get_terms(&terms); > res = bisect_run(&terms, argv, argc); > break; > + case BISECT_STATE: > + if (!terms.term_good) { > + set_terms(&terms, "bad", "good"); > + get_terms(&terms); > + } > + res = bisect_state(&terms, argv, argc); > + break; > default: > BUG("unknown subcommand %d", cmdmode); > } > -- > gitgitgadget Does it make sense to also include something like this: diff --git a/git-bisect.sh b/git-bisect.sh index 405cf76f2a..fbf56649d7 100755 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -60,7 +60,7 @@ case "$#" in start) git bisect--helper --bisect-start "$@" ;; bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD") - git bisect--helper --bisect-state "$cmd" "$@" ;; + git bisect--helper "$cmd" "$@" ;; skip) git bisect--helper --bisect-skip "$@" || exit;; next) to prove that you've made it optional? (Well, assuming one has run the tests with that change, but I did...)
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > @@ -1210,6 +1210,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > git_bisect_helper_usage, > PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN); > > + if (!cmdmode && argc > 0) { > + set_terms(&terms, "bad", "good"); > + get_terms(&terms); > + if (!check_and_set_terms(&terms, argv[0])) > + cmdmode = BISECT_STATE; > + } > + I do agree with "if we want to reuse this function without changing it much, --bisect-state must become an optional and the default mode of operation" as a goal, but I do not quite get this change. From the rephased description of the goal, what I would expect is more like if (!cmdmode && possibly-other-conditions-like-argc-check) cmdmode = BISECT_STATE; and nothing else. Between the case where --bisect-state was and was not given explicitly, check-and-set-terms is or is not called. I can see that checking would be _nice_ when we try to decide if the first token makes sense as good/bad and the user wanted to do the "state" thing impolicitly, but if it is worth checking in implicit case, shouldn't we be checking when the --bisect-state is explicitly given as well? And the actual execution of the BISECT_STATE command is even more puzzling, below. > if (!cmdmode) > usage_with_options(git_bisect_helper_usage, options); > > @@ -1218,11 +1225,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > set_terms(&terms, "bad", "good"); > res = bisect_start(&terms, argv, argc); > break; > - case BISECT_STATE: > - set_terms(&terms, "bad", "good"); > - get_terms(&terms); > - res = bisect_state(&terms, argv, argc); > - break; > case BISECT_TERMS: > if (argc > 1) > return error(_("--bisect-terms requires 0 or 1 argument")); > @@ -1265,6 +1267,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > get_terms(&terms); > res = bisect_run(&terms, argv, argc); > break; > + case BISECT_STATE: > + if (!terms.term_good) { > + set_terms(&terms, "bad", "good"); > + get_terms(&terms); > + } > + res = bisect_state(&terms, argv, argc); > + break; This case arm has been moved but because there is no fall-through in this switch statement, the movement is a no-op. But the code has also changed with this patch. We used to do set_terms/get_terms unconditionally, but we do not even do so when terms_good (but not terms_bad) is already set. Is this an unrelated bugfix of some kind? This does not look related to "making --bisect-state optional and implicit default" at all. At least the proposed log message does not explain why. Thanks. > default: > BUG("unknown subcommand %d", cmdmode); > }
Hi Elijah, On Tue, 8 Feb 2022, Elijah Newren wrote: > On Fri, Jan 28, 2022 at 3:52 PM Johannes Schindelin via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > In preparation for making `git bisect` a real built-in, let's prepare > > the `bisect--helper` built-in to handle `git bisect--helper good` and > > `git bisect--helper bad`, i.e. do not require the `--bisect-state` > > option to be passed explicitly. > > > > To prepare for converting `bisect--helper` to a full built-in > > implementation of `git bisect` (which must not require the > > `--bisect-state` option to be specified at all), let's move the handling > > toward the end of the `switch (cmdmode)` block. > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > builtin/bisect--helper.c | 23 ++++++++++++++++------- > > 1 file changed, 16 insertions(+), 7 deletions(-) > > > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > > index cc5a9ca41b9..219fa99cd0b 100644 > > --- a/builtin/bisect--helper.c > > +++ b/builtin/bisect--helper.c > > @@ -26,8 +26,8 @@ static const char * const git_bisect_helper_usage[] = { > > N_("git bisect--helper --bisect-start [--term-{new,bad}=<term> --term-{old,good}=<term>]" > > " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"), > > N_("git bisect--helper --bisect-next"), > > - N_("git bisect--helper --bisect-state (bad|new) [<rev>]"), > > - N_("git bisect--helper --bisect-state (good|old) [<rev>...]"), > > + N_("git bisect--helper [--bisect-state] (bad|new) [<rev>]"), > > + N_("git bisect--helper [--bisect-state] (good|old) [<rev>...]"), > > N_("git bisect--helper --bisect-replay <filename>"), > > N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"), > > N_("git bisect--helper --bisect-visualize"), > > @@ -1210,6 +1210,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > > git_bisect_helper_usage, > > PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN); > > > > + if (!cmdmode && argc > 0) { > > + set_terms(&terms, "bad", "good"); > > + get_terms(&terms); > > + if (!check_and_set_terms(&terms, argv[0])) > > + cmdmode = BISECT_STATE; > > + } > > + > > if (!cmdmode) > > usage_with_options(git_bisect_helper_usage, options); > > > > @@ -1218,11 +1225,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > > set_terms(&terms, "bad", "good"); > > res = bisect_start(&terms, argv, argc); > > break; > > - case BISECT_STATE: > > - set_terms(&terms, "bad", "good"); > > - get_terms(&terms); > > - res = bisect_state(&terms, argv, argc); > > - break; > > case BISECT_TERMS: > > if (argc > 1) > > return error(_("--bisect-terms requires 0 or 1 argument")); > > @@ -1265,6 +1267,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > > get_terms(&terms); > > res = bisect_run(&terms, argv, argc); > > break; > > + case BISECT_STATE: > > + if (!terms.term_good) { > > + set_terms(&terms, "bad", "good"); > > + get_terms(&terms); > > + } > > + res = bisect_state(&terms, argv, argc); > > + break; > > default: > > BUG("unknown subcommand %d", cmdmode); > > } > > -- > > gitgitgadget > > Does it make sense to also include something like this: > > diff --git a/git-bisect.sh b/git-bisect.sh > index 405cf76f2a..fbf56649d7 100755 > --- a/git-bisect.sh > +++ b/git-bisect.sh > @@ -60,7 +60,7 @@ case "$#" in > start) > git bisect--helper --bisect-start "$@" ;; > bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD") > - git bisect--helper --bisect-state "$cmd" "$@" ;; > + git bisect--helper "$cmd" "$@" ;; > skip) > git bisect--helper --bisect-skip "$@" || exit;; > next) > > to prove that you've made it optional? (Well, assuming one has run > the tests with that change, but I did...) Good point! That code will go away within the same patch series, of course, but it is good to keep this in an intermediate commit. Thanks, Dscho
Hi Junio, On Wed, 9 Feb 2022, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > @@ -1210,6 +1210,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > > git_bisect_helper_usage, > > PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN); > > > > + if (!cmdmode && argc > 0) { > > + set_terms(&terms, "bad", "good"); > > + get_terms(&terms); > > + if (!check_and_set_terms(&terms, argv[0])) > > + cmdmode = BISECT_STATE; > > + } > > + > > I do agree with "if we want to reuse this function without changing > it much, --bisect-state must become an optional and the default mode > of operation" as a goal, but I do not quite get this change. From > the rephased description of the goal, what I would expect is more > like > > if (!cmdmode && possibly-other-conditions-like-argc-check) > cmdmode = BISECT_STATE; > > and nothing else. That would not work, though: we must ensure that the `argv[0]` is a bad/good term (hence the need to call `set_terms()` and `get_terms()` to retrieve potentially overridden terms and then the call to `check_and_set_terms()` to verify that it was one of those terms). > Between the case where --bisect-state was and was not given explicitly, > check-and-set-terms is or is not called. I > can see that checking would be _nice_ when we try to decide if the > first token makes sense as good/bad and the user wanted to do the > "state" thing impolicitly, but if it is worth checking in implicit > case, shouldn't we be checking when the --bisect-state is explicitly > given as well? I tried to keep the `if (!cmdmode) usage...` in place, and that's probably what caused your confusion. I replaced it with: -- snip -- @@ -1210,10 +1210,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) git_bisect_helper_usage, PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN); - if (!cmdmode) - usage_with_options(git_bisect_helper_usage, options); - - switch (cmdmode) { + switch (cmdmode ? cmdmode : BISECT_STATE) { case BISECT_START: set_terms(&terms, "bad", "good"); res = bisect_start(&terms, argv, argc); @@ -1221,6 +1218,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) case BISECT_STATE: set_terms(&terms, "bad", "good"); get_terms(&terms); + if (!cmdmode && + (!argc || check_and_set_terms(&terms, argv[0]))) { + char *msg = xstrfmt(_("unknown command: '%s'"), argv[0]); + usage_msg_opt(msg, git_bisect_helper_usage, options); + } res = bisect_state(&terms, argv, argc); break; case BISECT_TERMS: -- snap -- I am not the best judge whether this is more obvious because I am so familiar with this code, but I hope that it is at least more concise. > And the actual execution of the BISECT_STATE command is even more > puzzling, below. > > > if (!cmdmode) > > usage_with_options(git_bisect_helper_usage, options); > > > > @@ -1218,11 +1225,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > > set_terms(&terms, "bad", "good"); > > res = bisect_start(&terms, argv, argc); > > break; > > - case BISECT_STATE: > > - set_terms(&terms, "bad", "good"); > > - get_terms(&terms); > > - res = bisect_state(&terms, argv, argc); > > - break; > > case BISECT_TERMS: > > if (argc > 1) > > return error(_("--bisect-terms requires 0 or 1 argument")); > > @@ -1265,6 +1267,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > > get_terms(&terms); > > res = bisect_run(&terms, argv, argc); > > break; > > + case BISECT_STATE: > > + if (!terms.term_good) { > > + set_terms(&terms, "bad", "good"); > > + get_terms(&terms); > > + } > > + res = bisect_state(&terms, argv, argc); > > + break; > > This case arm has been moved but because there is no fall-through in > this switch statement, the movement is a no-op. But the code has > also changed with this patch. We used to do set_terms/get_terms > unconditionally, but we do not even do so when terms_good (but not > terms_bad) is already set. > > Is this an unrelated bugfix of some kind? This does not look > related to "making --bisect-state optional and implicit default" at > all. At least the proposed log message does not explain why. I had hoped that this part of the commit message would be clear enough: To prepare for converting `bisect--helper` to a full built-in implementation of `git bisect` (which must not require the `--bisect-state` option to be specified at all), let's move the handling toward the end of the `switch (cmdmode)` block. Since my hope did not turn into reality, I split out that move into its own commit (making the patch series even longer, but hopefully at least making it more readable, too). Ciao, Dscho
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index cc5a9ca41b9..219fa99cd0b 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -26,8 +26,8 @@ static const char * const git_bisect_helper_usage[] = { N_("git bisect--helper --bisect-start [--term-{new,bad}=<term> --term-{old,good}=<term>]" " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"), N_("git bisect--helper --bisect-next"), - N_("git bisect--helper --bisect-state (bad|new) [<rev>]"), - N_("git bisect--helper --bisect-state (good|old) [<rev>...]"), + N_("git bisect--helper [--bisect-state] (bad|new) [<rev>]"), + N_("git bisect--helper [--bisect-state] (good|old) [<rev>...]"), N_("git bisect--helper --bisect-replay <filename>"), N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"), N_("git bisect--helper --bisect-visualize"), @@ -1210,6 +1210,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) git_bisect_helper_usage, PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN); + if (!cmdmode && argc > 0) { + set_terms(&terms, "bad", "good"); + get_terms(&terms); + if (!check_and_set_terms(&terms, argv[0])) + cmdmode = BISECT_STATE; + } + if (!cmdmode) usage_with_options(git_bisect_helper_usage, options); @@ -1218,11 +1225,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) set_terms(&terms, "bad", "good"); res = bisect_start(&terms, argv, argc); break; - case BISECT_STATE: - set_terms(&terms, "bad", "good"); - get_terms(&terms); - res = bisect_state(&terms, argv, argc); - break; case BISECT_TERMS: if (argc > 1) return error(_("--bisect-terms requires 0 or 1 argument")); @@ -1265,6 +1267,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) get_terms(&terms); res = bisect_run(&terms, argv, argc); break; + case BISECT_STATE: + if (!terms.term_good) { + set_terms(&terms, "bad", "good"); + get_terms(&terms); + } + res = bisect_state(&terms, argv, argc); + break; default: BUG("unknown subcommand %d", cmdmode); }