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

> Hi Pranit,
>
> On 12/06/2016 11:40 PM, Pranit Bauva wrote:
>> On Tue, Nov 22, 2016 at 5:42 AM, Stephan Beyer <s-beyer@gmx.net> 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 **) &hex.items[i].string;
>>>> +                     if(bisect_write(state, *hex_string, terms, 0)) {
>>>> +                             string_list_clear(&hex, 0);
>>>> +                             return -1;
>>>> +                     }
>>>> +                     if (check_expected_revs(hex_string, 1)) {
>>>> +                             string_list_clear(&hex, 0);
>>>> +                             return -1;
>>>> +                     }
>>>
>>> by:
>>>
>>>                         const char *hex_str = hex.items[i].string;
>>>                         if (set_state(terms, state, hex_string)) {
>>>                                 string_list_clear(&hex, 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")