Re: [PATCH v15 19/27] bisect--helper: `bisect_state` & `bisect_head` shell function in C

[Pranit Bauva]

Hey Stephan,

On Wed, Dec 7, 2016 at 5:24 AM, Stephan Beyer  wrote:
> Hi Pranit,
>
> On 12/06/2016 11:40 PM, Pranit Bauva wrote:
>> On Tue, Nov 22, 2016 at 5:42 AM, Stephan Beyer  wrote:
>>> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
 +static int bisect_state(struct bisect_terms *terms, const char **argv,
 + int argc)
 +{
 + const char *state = argv[0];
 +
 + get_terms(terms);
 + if (check_and_set_terms(terms, state))
 + return -1;
 +
 + if (!argc)
 + die(_("Please call `--bisect-state` with at least one 
 argument"));
>>>
>>> I think this check should move to cmd_bisect__helper. There are also the
>>> other argument number checks.
>>
>> Not really. After the whole conversion, the cmdmode will cease to
>> exists while bisect_state will be called directly, thus it is
>> important to check it here.
>
> Okay, that's a point.
> In that case, you should probably use "return error()" again and the
> message (mentioning "--bisect-state") does not make sense when
> --bisect-state ceases to exist.

True. Will change accordingly.

 +
 + if (argc == 1 && one_of(state, terms->term_good,
 + terms->term_bad, "skip", NULL)) {
 + const char *bisected_head = xstrdup(bisect_head());
 + const char *hex[1];
>>>
>>> Maybe:
>>> const char *hex;
>>>
 + unsigned char sha1[20];
 +
 + if (get_sha1(bisected_head, sha1))
 + die(_("Bad rev input: %s"), bisected_head);
>>>
>>> And instead of...
>>>
 + if (bisect_write(state, sha1_to_hex(sha1), terms, 0))
 + return -1;
 +
 + *hex = xstrdup(sha1_to_hex(sha1));
 + if (check_expected_revs(hex, 1))
 + return -1;
>>>
>>> ... simply:
>>>
>>> hex = xstrdup(sha1_to_hex(sha1));
>>> if (set_state(terms, state, hex)) {
>>> free(hex);
>>> return -1;
>>> }
>>> free(hex);
>>>
>>> where:
>>
>> Yes I am planning to convert all places with hex rather than the sha1
>> but not yet, maybe in an another patch series because currently a lot
>> of things revolve around sha1 and changing its behaviour wouldn't
>> really be a part of a porting patch series.
>
> I was not suggesting a change of behavior, I was suggesting a simple
> helper function set_state() to get rid of code duplication above and
> some lines below:
>
>>> ... And replace this:
>>>
 + const char **hex_string = (const char **) 
 [i].string;
 + if(bisect_write(state, *hex_string, terms, 0)) {
 + string_list_clear(, 0);
 + return -1;
 + }
 + if (check_expected_revs(hex_string, 1)) {
 + string_list_clear(, 0);
 + return -1;
 + }
>>>
>>> by:
>>>
>>> const char *hex_str = hex.items[i].string;
>>> if (set_state(terms, state, hex_string)) {
>>> string_list_clear(, 0);
>>> return -1;
>>> }
>
> See?

I can do this change of using set_state() keeping aside the sha1 to
hex and such change.

 @@ -184,8 +137,8 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
   state="$TERM_GOOD"
   fi

 - # We have to use a subshell because "bisect_state" can exit.
 - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
 + # We have to use a subshell because "--bisect-state" can 
 exit.
 + ( git bisect--helper --bisect-state $state 
 >"$GIT_DIR/BISECT_RUN" )
>>>
>>> The new comment is funny, but you don't need a subshell here any
>>> longer.
>>
>> True, but right now I didn't want to modify that part of the source
>> code to remove the comment. I will remove the comment all together
>> when I port bisect_run()
> For most of the patches, I was commenting on the current state, not on
> the big picture.
>
> Still I think that it is better to remove the comment and the subshell
> instead of making the comment weird ("yes the builtin can exit, but why
> do we need a subshell?" vs "yes the shell function calls exit, so we
> need a subshell because we do not want to exit this shell script")

Sure I will remove the comment.

Regards,
Pranit Bauva

Re: [PATCH v15 19/27] bisect--helper: `bisect_state` & `bisect_head` shell function in C

[Stephan Beyer]

Hi Pranit,

On 12/06/2016 11:40 PM, Pranit Bauva wrote:
> On Tue, Nov 22, 2016 at 5:42 AM, Stephan Beyer  wrote:
>> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>>> +static int bisect_state(struct bisect_terms *terms, const char **argv,
>>> + int argc)
>>> +{
>>> + const char *state = argv[0];
>>> +
>>> + get_terms(terms);
>>> + if (check_and_set_terms(terms, state))
>>> + return -1;
>>> +
>>> + if (!argc)
>>> + die(_("Please call `--bisect-state` with at least one 
>>> argument"));
>>
>> I think this check should move to cmd_bisect__helper. There are also the
>> other argument number checks.
> 
> Not really. After the whole conversion, the cmdmode will cease to
> exists while bisect_state will be called directly, thus it is
> important to check it here.

Okay, that's a point.
In that case, you should probably use "return error()" again and the
message (mentioning "--bisect-state") does not make sense when
--bisect-state ceases to exist.

>>> +
>>> + if (argc == 1 && one_of(state, terms->term_good,
>>> + terms->term_bad, "skip", NULL)) {
>>> + const char *bisected_head = xstrdup(bisect_head());
>>> + const char *hex[1];
>>
>> Maybe:
>> const char *hex;
>>
>>> + unsigned char sha1[20];
>>> +
>>> + if (get_sha1(bisected_head, sha1))
>>> + die(_("Bad rev input: %s"), bisected_head);
>>
>> And instead of...
>>
>>> + if (bisect_write(state, sha1_to_hex(sha1), terms, 0))
>>> + return -1;
>>> +
>>> + *hex = xstrdup(sha1_to_hex(sha1));
>>> + if (check_expected_revs(hex, 1))
>>> + return -1;
>>
>> ... simply:
>>
>> hex = xstrdup(sha1_to_hex(sha1));
>> if (set_state(terms, state, hex)) {
>> free(hex);
>> return -1;
>> }
>> free(hex);
>>
>> where:
> 
> Yes I am planning to convert all places with hex rather than the sha1
> but not yet, maybe in an another patch series because currently a lot
> of things revolve around sha1 and changing its behaviour wouldn't
> really be a part of a porting patch series.

I was not suggesting a change of behavior, I was suggesting a simple
helper function set_state() to get rid of code duplication above and
some lines below:

>> ... And replace this:
>>
>>> + const char **hex_string = (const char **) 
>>> [i].string;
>>> + if(bisect_write(state, *hex_string, terms, 0)) {
>>> + string_list_clear(, 0);
>>> + return -1;
>>> + }
>>> + if (check_expected_revs(hex_string, 1)) {
>>> + string_list_clear(, 0);
>>> + return -1;
>>> + }
>>
>> by:
>>
>> const char *hex_str = hex.items[i].string;
>> if (set_state(terms, state, hex_string)) {
>> string_list_clear(, 0);
>> return -1;
>> }

See?

>>> @@ -184,8 +137,8 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
>>>   state="$TERM_GOOD"
>>>   fi
>>>
>>> - # We have to use a subshell because "bisect_state" can exit.
>>> - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
>>> + # We have to use a subshell because "--bisect-state" can exit.
>>> + ( git bisect--helper --bisect-state $state 
>>> >"$GIT_DIR/BISECT_RUN" )
>>
>> The new comment is funny, but you don't need a subshell here any
>> longer.
> 
> True, but right now I didn't want to modify that part of the source
> code to remove the comment. I will remove the comment all together
> when I port bisect_run()
For most of the patches, I was commenting on the current state, not on
the big picture.

Still I think that it is better to remove the comment and the subshell
instead of making the comment weird ("yes the builtin can exit, but why
do we need a subshell?" vs "yes the shell function calls exit, so we
need a subshell because we do not want to exit this shell script")

~Stephan

Re: [PATCH v15 19/27] bisect--helper: `bisect_state` & `bisect_head` shell function in C

[Pranit Bauva]

Hey Stephan,

On Tue, Nov 22, 2016 at 5:42 AM, Stephan Beyer  wrote:
> Hi,
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> Reimplement the `bisect_state` shell function in C and also add a
>> subcommand `--bisect-state` to `git-bisect--helper` to call it from
>> git-bisect.sh .
>>
>> Using `--bisect-state` 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 some
>> other methods.
>>
>> `bisect_head` is called from `bisect_state` thus its not required to
>> introduce another subcommand.
>
> Missing comma before "thus", and "it is" (or "it's") instead of "its" :)

Sure, will fix.

>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 1767916..1481c6d 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -784,6 +786,79 @@ static int bisect_autostart(struct bisect_terms *terms)
>>   return 0;
>>  }
>>
>> +static char *bisect_head(void)
>> +{
>> + if (is_empty_or_missing_file(git_path_bisect_head()))
>> + return "HEAD";
>> + else
>> + return "BISECT_HEAD";
>> +}
>
> This is very shellish.
> In C I'd expect something like
>
> static int bisect_head_sha1(unsigned char *sha)
> {
> int res;
> if (is_empty_or_missing_file(git_path_bisect_head()))
> res = get_sha1("HEAD", sha);
> else
> res = get_sha1("BISECT_HEAD", sha);
>
> if (res)
> return error(_("Could not find BISECT_HEAD or HEAD."));
>
> return 0;
> }
>
>> +
>> +static int bisect_state(struct bisect_terms *terms, const char **argv,
>> + int argc)
>> +{
>> + const char *state = argv[0];
>> +
>> + get_terms(terms);
>> + if (check_and_set_terms(terms, state))
>> + return -1;
>> +
>> + if (!argc)
>> + die(_("Please call `--bisect-state` with at least one 
>> argument"));
>
> I think this check should move to cmd_bisect__helper. There are also the
> other argument number checks.

Not really. After the whole conversion, the cmdmode will cease to
exists while bisect_state will be called directly, thus it is
important to check it here.

>> +
>> + if (argc == 1 && one_of(state, terms->term_good,
>> + terms->term_bad, "skip", NULL)) {
>> + const char *bisected_head = xstrdup(bisect_head());
>> + const char *hex[1];
>
> Maybe:
> const char *hex;
>
>> + unsigned char sha1[20];
>> +
>> + if (get_sha1(bisected_head, sha1))
>> + die(_("Bad rev input: %s"), bisected_head);
>
> And instead of...
>
>> + if (bisect_write(state, sha1_to_hex(sha1), terms, 0))
>> + return -1;
>> +
>> + *hex = xstrdup(sha1_to_hex(sha1));
>> + if (check_expected_revs(hex, 1))
>> + return -1;
>
> ... simply:
>
> hex = xstrdup(sha1_to_hex(sha1));
> if (set_state(terms, state, hex)) {
> free(hex);
> return -1;
> }
> free(hex);
>
> where:

Yes I am planning to convert all places with hex rather than the sha1
but not yet, maybe in an another patch series because currently a lot
of things revolve around sha1 and changing its behaviour wouldn't
really be a part of a porting patch series.

> static int set_state(struct bisect_terms *terms, const char *state,
>  const char *hex)
> {
> if (bisect_write(state, hex, terms, 0))
> return -1;
> if (check_expected_revs(, 1))
> return -1;
> return 0;
> }
>
>> + return bisect_auto_next(terms, NULL);
>> + }
>> +
>> + if ((argc == 2 && !strcmp(state, terms->term_bad)) ||
>> + one_of(state, terms->term_good, "skip", NULL)) {
>> + int i;
>> + struct string_list hex = STRING_LIST_INIT_DUP;
>> +
>> + for (i = 1; i < argc; i++) {
>> + unsigned char sha1[20];
>> +
>> + if (get_sha1(argv[i], sha1)) {
>> + string_list_clear(, 0);
>> + die(_("Bad rev input: %s"), argv[i]);
>> + }
>> + string_list_append(, sha1_to_hex(sha1));
>> + }
>> + for (i = 0; i < hex.nr; i++) {
>
> ... And replace this:
>
>> + const char **hex_string = (const char **) 
>> [i].string;
>> + if(bisect_write(state, *hex_string, terms, 0)) {
>> + string_list_clear(, 0);
>> + return -1;
>> + }
>> + if (check_expected_revs(hex_string, 1)) {
>> + 

Re: [PATCH v15 19/27] bisect--helper: `bisect_state` & `bisect_head` shell function in C

[Stephan Beyer]

Hi,

On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> Reimplement the `bisect_state` shell function in C and also add a
> subcommand `--bisect-state` to `git-bisect--helper` to call it from
> git-bisect.sh .
> 
> Using `--bisect-state` 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 some
> other methods.
> 
> `bisect_head` is called from `bisect_state` thus its not required to
> introduce another subcommand.

Missing comma before "thus", and "it is" (or "it's") instead of "its" :)

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 1767916..1481c6d 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -784,6 +786,79 @@ static int bisect_autostart(struct bisect_terms *terms)
>   return 0;
>  }
>  
> +static char *bisect_head(void)
> +{
> + if (is_empty_or_missing_file(git_path_bisect_head()))
> + return "HEAD";
> + else
> + return "BISECT_HEAD";
> +}

This is very shellish.
In C I'd expect something like

static int bisect_head_sha1(unsigned char *sha)
{
int res;
if (is_empty_or_missing_file(git_path_bisect_head()))
res = get_sha1("HEAD", sha);
else
res = get_sha1("BISECT_HEAD", sha);

if (res)
return error(_("Could not find BISECT_HEAD or HEAD."));

return 0;
}

> +
> +static int bisect_state(struct bisect_terms *terms, const char **argv,
> + int argc)
> +{
> + const char *state = argv[0];
> +
> + get_terms(terms);
> + if (check_and_set_terms(terms, state))
> + return -1;
> +
> + if (!argc)
> + die(_("Please call `--bisect-state` with at least one 
> argument"));

I think this check should move to cmd_bisect__helper. There are also the
other argument number checks.

> +
> + if (argc == 1 && one_of(state, terms->term_good,
> + terms->term_bad, "skip", NULL)) {
> + const char *bisected_head = xstrdup(bisect_head());
> + const char *hex[1];

Maybe:
const char *hex;

> + unsigned char sha1[20];
> +
> + if (get_sha1(bisected_head, sha1))
> + die(_("Bad rev input: %s"), bisected_head);

And instead of...

> + if (bisect_write(state, sha1_to_hex(sha1), terms, 0))
> + return -1;
> +
> + *hex = xstrdup(sha1_to_hex(sha1));
> + if (check_expected_revs(hex, 1))
> + return -1;

... simply:

hex = xstrdup(sha1_to_hex(sha1));
if (set_state(terms, state, hex)) {
free(hex);
return -1;
}
free(hex);

where:

static int set_state(struct bisect_terms *terms, const char *state,
 const char *hex)
{
if (bisect_write(state, hex, terms, 0))
return -1;
if (check_expected_revs(, 1))
return -1;
return 0;
}

> + return bisect_auto_next(terms, NULL);
> + }
> +
> + if ((argc == 2 && !strcmp(state, terms->term_bad)) ||
> + one_of(state, terms->term_good, "skip", NULL)) {
> + int i;
> + struct string_list hex = STRING_LIST_INIT_DUP;
> +
> + for (i = 1; i < argc; i++) {
> + unsigned char sha1[20];
> +
> + if (get_sha1(argv[i], sha1)) {
> + string_list_clear(, 0);
> + die(_("Bad rev input: %s"), argv[i]);
> + }
> + string_list_append(, sha1_to_hex(sha1));
> + }
> + for (i = 0; i < hex.nr; i++) {

... And replace this:

> + const char **hex_string = (const char **) 
> [i].string;
> + if(bisect_write(state, *hex_string, terms, 0)) {
> + string_list_clear(, 0);
> + return -1;
> + }
> + if (check_expected_revs(hex_string, 1)) {
> + string_list_clear(, 0);
> + return -1;
> + }

by:

const char *hex_str = hex.items[i].string;
if (set_state(terms, state, hex_string)) {
string_list_clear(, 0);
return -1;
}

> + }
> + string_list_clear(, 0);
> + return bisect_auto_next(terms, NULL);
> + }
> +
> + if (!strcmp(state, terms->term_bad))
> + die(_("'git bisect %s' can take only one argument."),
> +   terms->term_bad);
> +
> + return -1;
> +}
> +
>  int 

[PATCH v15 19/27] bisect--helper: `bisect_state` & `bisect_head` shell function in C

[Pranit Bauva]

Reimplement the `bisect_state` shell function in C and also add a
subcommand `--bisect-state` to `git-bisect--helper` to call it from
git-bisect.sh .

Using `--bisect-state` 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 some
other methods.

`bisect_head` is called from `bisect_state` thus its not required to
introduce another subcommand.

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 86 
 git-bisect.sh| 57 +++-
 2 files changed, 91 insertions(+), 52 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 1767916..1481c6d 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --bisect-next"),
N_("git bisect--helper --bisect-auto-next"),
N_("git bisect--helper --bisect-autostart"),
+   N_("git bisect--helper --bisect-state (bad|new) []"),
+   N_("git bisect--helper --bisect-state (good|old) [...]"),
NULL
 };
 
@@ -784,6 +786,79 @@ static int bisect_autostart(struct bisect_terms *terms)
return 0;
 }
 
+static char *bisect_head(void)
+{
+   if (is_empty_or_missing_file(git_path_bisect_head()))
+   return "HEAD";
+   else
+   return "BISECT_HEAD";
+}
+
+static int bisect_state(struct bisect_terms *terms, const char **argv,
+   int argc)
+{
+   const char *state = argv[0];
+
+   get_terms(terms);
+   if (check_and_set_terms(terms, state))
+   return -1;
+
+   if (!argc)
+   die(_("Please call `--bisect-state` with at least one 
argument"));
+
+   if (argc == 1 && one_of(state, terms->term_good,
+   terms->term_bad, "skip", NULL)) {
+   const char *bisected_head = xstrdup(bisect_head());
+   const char *hex[1];
+   unsigned char sha1[20];
+
+   if (get_sha1(bisected_head, sha1))
+   die(_("Bad rev input: %s"), bisected_head);
+   if (bisect_write(state, sha1_to_hex(sha1), terms, 0))
+   return -1;
+
+   *hex = xstrdup(sha1_to_hex(sha1));
+   if (check_expected_revs(hex, 1))
+   return -1;
+   return bisect_auto_next(terms, NULL);
+   }
+
+   if ((argc == 2 && !strcmp(state, terms->term_bad)) ||
+   one_of(state, terms->term_good, "skip", NULL)) {
+   int i;
+   struct string_list hex = STRING_LIST_INIT_DUP;
+
+   for (i = 1; i < argc; i++) {
+   unsigned char sha1[20];
+
+   if (get_sha1(argv[i], sha1)) {
+   string_list_clear(, 0);
+   die(_("Bad rev input: %s"), argv[i]);
+   }
+   string_list_append(, sha1_to_hex(sha1));
+   }
+   for (i = 0; i < hex.nr; i++) {
+   const char **hex_string = (const char **) 
[i].string;
+   if(bisect_write(state, *hex_string, terms, 0)) {
+   string_list_clear(, 0);
+   return -1;
+   }
+   if (check_expected_revs(hex_string, 1)) {
+   string_list_clear(, 0);
+   return -1;
+   }
+   }
+   string_list_clear(, 0);
+   return bisect_auto_next(terms, NULL);
+   }
+
+   if (!strcmp(state, terms->term_bad))
+   die(_("'git bisect %s' can take only one argument."),
+ terms->term_bad);
+
+   return -1;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -798,6 +873,7 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
BISECT_NEXT,
BISECT_AUTO_NEXT,
BISECT_AUTOSTART,
+   BISECT_STATE
} cmdmode = 0;
int no_checkout = 0, res = 0;
struct option options[] = {
@@ -823,6 +899,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 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_CMDMODE(0, "bisect-state", ,
+N_("mark the state of ref