Re: [PATCH v15 18/27] bisect--helper: `bisect_autostart` shell function in C

Hey Stephan,

On Mon, Nov 21, 2016 at 1:45 AM, Stephan Beyer [off-list ref] wrote:

Hi,

On 10/14/2016 04:14 PM, Pranit Bauva wrote:

quoted

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 502bf18..1767916 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c

@@ -422,6 +425,7 @@ static int bisect_next(...)
 {
      int res, no_checkout;

+     bisect_autostart(terms);

You are not checking for return values here. (The shell code simply
exited if there is no tty, but you don't.)

True. I didn't notice it carefully. Thanks for pointing it out.

quoted

@@ -754,6 +758,32 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
      return retval || bisect_auto_next(terms, NULL);
 }

+static int bisect_autostart(struct bisect_terms *terms)
+{
+     if (is_empty_or_missing_file(git_path_bisect_start())) {
+             const char *yesno;
+             const char *argv[] = {NULL};
+             fprintf(stderr, _("You need to start by \"git bisect "
+                               "start\"\n"));
+
+             if (!isatty(0))

isatty(STDIN_FILENO)?

Seems better.

quoted

+                     return 1;
+
+             /*
+              * TRANSLATORS: Make sure to include [Y] and [n] in your
+              * translation. THe program will only accept English input

Typo "THe"

Sure.

quoted

+              * at this point.
+              */

Taking "at this point" into consideration, I think the Y and n can be
easily translated now that it is in C. I guess, by using...

quoted

+             yesno = git_prompt(_("Do you want me to do it for you "
+                                  "[Y/n]? "), PROMPT_ECHO);
+             if (starts_with(yesno, "n") || starts_with(yesno, "N"))

... starts_with(yesno, _("n")) || starts_with(yesno, _("N"))
here (but not sure). However, this would be an extra patch on top of
this series.

Can add it as an extra patch. Thanks for informing.

quoted

+                     exit(0);

Shouldn't this also be "return 1;"? Saying "no" is the same outcome as
not having a tty to ask for yes or no.

Yes.

quoted

 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
      enum {

@@ -790,6 +821,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
                       N_("find the next bisection commit"), BISECT_NEXT),
              OPT_CMDMODE(0, "bisect-auto-next", &cmdmode,
                       N_("verify the next bisection state then find the next bisection state"), BISECT_AUTO_NEXT),
+             OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
+                      N_("start the bisection if BISECT_START empty or missing"), BISECT_AUTOSTART),

The word "is" is missing.

Sure. Thanks for going through these patches very carefully.

Regards,
Pranit Bauva