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

From: Stephan Beyer <s-beyer@gmx.net>
Date: Dec 6, 2016, 23:54 UTC
Message-ID: <14b9ba3a-4568-2e4b-0b1a-f0ee75a7c677@gmx.net>
In-Reply-To: <CAFZEwPPTnfOYsL8OB5x5n9cm2s+chMwg=xo459kVnFV9bB+5Hw@mail.gmail.com>

Hi Pranit,

On 12/06/2016 11:40 PM, Pranit Bauva wrote:

Show 20 quoted lines

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

Show 38 quoted lines

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

Show 19 quoted lines

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

Show 15 quoted lines

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