Hi Miriam,
On Mon, 31 Aug 2020, Miriam Rubio wrote:
> From: Pranit Bauva <pranit.bauva@xxxxxxxxx>
>
> Reimplement the `bisect_autostart()` shell function in C and add the
> C implementation from `bisect_next()` which was previously left
> uncovered.
>
> Add `--bisect-autostart` subcommand to be called from git-bisect.sh.
> Using `--bisect-autostart` subcommand is a temporary measure to port
> the shell function to C so as to use the existing test suite. As more
> functions are ported, this subcommand will be retired and
> bisect_autostart() will be called directly by `bisect_state()`.
>
> Mentored-by: Lars Schneider <larsxschneider@xxxxxxxxx>
> Mentored-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
> Mentored-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
> Signed-off-by: Pranit Bauva <pranit.bauva@xxxxxxxxx>
> Signed-off-by: Tanushree Tumane <tanushreetumane@xxxxxxxxx>
> Signed-off-by: Miriam Rubio <mirucam@xxxxxxxxx>
> ---
> builtin/bisect--helper.c | 44 +++++++++++++++++++++++++++++++++++++++-
> git-bisect.sh | 25 ++---------------------
> 2 files changed, 45 insertions(+), 24 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index bae09ce65d..f71e8e54d2 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -29,6 +29,7 @@ static const char * const git_bisect_helper_usage[] = {
> N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
> N_("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]"
> " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"),
> + N_("git bisect--helper --bisect-autostart"),
> NULL
> };
>
> @@ -653,6 +654,38 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
> return res;
> }
>
> +static inline int file_is_not_empty(const char *path)
> +{
> + return !is_empty_or_missing_file(path);
> +}
> +
> +static int bisect_autostart(struct bisect_terms *terms)
> +{
> + int res;
> + const char *yesno;
> +
> + if (file_is_not_empty(git_path_bisect_start()))
> + return 0;
> +
> + fprintf_ln(stderr, _("You need to start by \"git bisect "
> + "start\"\n"));
> +
> + if (!isatty(STDIN_FILENO))
Not a big deal, but we have only two callers to `isatty()` that use the
`_FILENO` constants, and neither of them is about stdin. But that is not a
big deal.
> + return 0;
However, when we cannot auto-start, the shell script version calls `exit
1` to cause a failure. I think we need to do the same here, i.e. `return
-1;`.
> +
> + /*
> + * TRANSLATORS: Make sure to include [Y] and [n] in your
> + * translation. The program will only accept English input
> + * at this point.
> + */
> + yesno = git_prompt(_("Do you want me to do it for you "
> + "[Y/n]? "), PROMPT_ECHO);
> + res = tolower(*yesno) == 'n' ?
> + -1 : bisect_start(terms, empty_strvec, 0);
The corresponding shell script code reads like this:
read yesno
case "$yesno" in
[Nn]*)
exit ;;
esac
That is, if the user does not want to start, the command exits. With exit
code 0, i.e. success.
However, we do not do that here.
Now, you could argue (in the commit message) that this actually fixes a
bug (because if the bisection was aborted, that does not count for
success), and I'd be fine with that.
> +
> + return res;
> +}
> +
> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> {
> enum {
> @@ -665,7 +698,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> CHECK_AND_SET_TERMS,
> BISECT_NEXT_CHECK,
> BISECT_TERMS,
> - BISECT_START
> + BISECT_START,
> + BISECT_AUTOSTART,
> } cmdmode = 0;
> int res = 0, nolog = 0;
> struct option options[] = {
> @@ -689,6 +723,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> N_("print out the bisect terms"), BISECT_TERMS),
> OPT_CMDMODE(0, "bisect-start", &cmdmode,
> N_("start the bisect session"), BISECT_START),
> + OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
> + N_("start the bisection if it has not yet been started"), BISECT_AUTOSTART),
> OPT_BOOL(0, "no-log", &nolog,
> N_("no log for BISECT_WRITE")),
> OPT_END()
> @@ -748,6 +784,12 @@ 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_AUTOSTART:
> + if (argc)
> + return error(_("--bisect-autostart does not accept arguments"));
> + set_terms(&terms, "bad", "good");
> + res = bisect_autostart(&terms);
> + break;
> default:
> BUG("unknown subcommand %d", cmdmode);
> }
> diff --git a/git-bisect.sh b/git-bisect.sh
> index c7580e51a0..9ca583d964 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -49,27 +49,6 @@ bisect_head()
> fi
> }
>
> -bisect_autostart() {
> - test -s "$GIT_DIR/BISECT_START" || {
> - gettextln "You need to start by \"git bisect start\"" >&2
> - if test -t 0
> - then
> - # TRANSLATORS: Make sure to include [Y] and [n] in your
> - # translation. The program will only accept English input
> - # at this point.
> - gettext "Do you want me to do it for you [Y/n]? " >&2
> - read yesno
> - case "$yesno" in
> - [Nn]*)
> - exit ;;
> - esac
> - bisect_start
> - else
> - exit 1
> - fi
> - }
> -}
> -
> bisect_start() {
> git bisect--helper --bisect-start $@ || exit
>
> @@ -108,7 +87,7 @@ bisect_skip() {
> }
>
> bisect_state() {
> - bisect_autostart
> + git bisect--helper --bisect-autostart
This (and the change below as well) is insufficient, as it won't `exit` in
case of an error (or in case the user aborted). I think you need to append
`|| exit` as is done two lines below this line.
Ciao,
Dscho
> state=$1
> git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
> get_terms
> @@ -149,7 +128,7 @@ bisect_auto_next() {
>
> bisect_next() {
> case "$#" in 0) ;; *) usage ;; esac
> - bisect_autostart
> + git bisect--helper --bisect-autostart
> git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit
>
> # Perform all bisection computation, display and checkout
> --
> 2.25.0
>
>
