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

[Pranit Bauva]

Hey Stephan,

On Mon, Nov 21, 2016 at 1:45 AM, Stephan Beyer  wrote:
> Hi,
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> 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.

>> @@ -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.

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

Sure.

>> +  * 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...
>
>> + 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.

>> + 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.

>>  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", ,
>>N_("verify the next bisection state then find the 
>> next bisection state"), BISECT_AUTO_NEXT),
>> + OPT_CMDMODE(0, "bisect-autostart", ,
>> +  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

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

[Stephan Beyer]

Hi,

On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> 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.)

> @@ -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)?

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

Typo "THe"

> +  * 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...

> + 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.

> + 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.

>  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", ,
>N_("verify the next bisection state then find the next 
> bisection state"), BISECT_AUTO_NEXT),
> + OPT_CMDMODE(0, "bisect-autostart", ,
> +  N_("start the bisection if BISECT_START empty or 
> missing"), BISECT_AUTOSTART),

The word "is" is missing.

~Stephan

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

[Pranit Bauva]

Reimplement the `bisect_autostart` shell function in C and add the
C implementation from `bisect_next()` which was previously left
uncovered. Also add a subcommand `--bisect-autostart` to
`git bisect--helper` be called from `bisect_state()` from
git-bisect.sh .

Using `--bisect-autostart` subcommand is a temporary measure to port
shell function to C so as to use the existing test suite. As more
functions are ported, this subcommand will be retired and will be called
by `bisect_state()`.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 40 
 git-bisect.sh| 23 +--
 2 files changed, 41 insertions(+), 22 deletions(-)

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
@@ -30,6 +30,7 @@ static const char * const git_bisect_helper_usage[] = {
  "[--no-checkout] [ 
[...]] [--] [...]"),
N_("git bisect--helper --bisect-next"),
N_("git bisect--helper --bisect-auto-next"),
+   N_("git bisect--helper --bisect-autostart"),
NULL
 };
 
@@ -38,6 +39,8 @@ struct bisect_terms {
const char *term_bad;
 };
 
+static int bisect_autostart(struct bisect_terms *terms);
+
 /*
  * Check whether the string `term` belongs to the set of strings
  * included in the variable arguments.
@@ -422,6 +425,7 @@ static int bisect_next(struct bisect_terms *terms, const 
char *prefix)
 {
int res, no_checkout;
 
+   bisect_autostart(terms);
/*
 * In case of mistaken revs or checkout error, or signals received,
 * "bisect_auto_next" below may exit or misbehave.
@@ -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))
+   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);
+   if (starts_with(yesno, "n") || starts_with(yesno, "N"))
+   exit(0);
+
+   return bisect_start(terms, 0, argv, 0);
+   }
+   return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -767,6 +797,7 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
BISECT_START,
BISECT_NEXT,
BISECT_AUTO_NEXT,
+   BISECT_AUTOSTART,
} cmdmode = 0;
int no_checkout = 0, res = 0;
struct option options[] = {
@@ -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", ,
 N_("verify the next bisection state then find the next 
bisection state"), BISECT_AUTO_NEXT),
+   OPT_CMDMODE(0, "bisect-autostart", ,
+N_("start the bisection if BISECT_START empty or 
missing"), BISECT_AUTOSTART),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -862,6 +895,13 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
get_terms();
res = bisect_auto_next(, prefix);
break;
+   case BISECT_AUTOSTART:
+   if (argc)
+   die(_("--bisect-autostart requires 0 arguments"));
+   terms.term_good = "good";
+   terms.term_bad = "bad";
+   res = bisect_autostart();
+   break;
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index d574c44..cd56551 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
-