Hey Stephan,
On Wed, Dec 7, 2016 at 5:24 AM, Stephan Beyer <s-beyer@xxxxxxx> wrote:
> 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@xxxxxxx> 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 **) &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?
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