Re: [PATCH v15 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

From: Pranit Bauva <pranit.bauva@gmail.com>
Date: Dec 6, 2016, 19:33 UTC
Message-ID: <CAFZEwPOZhO=sXLVwh03C8QN0uVXBUfb=xZ-JS003tgCNLgVOjg@mail.gmail.com>
In-Reply-To: <a4c7fec8-0e84-eb53-ca22-c369ce3facfa@gmx.net>

Hey Stephan,

On Thu, Nov 17, 2016 at 5:17 AM, Stephan Beyer <s-beyer@gmx.net> wrote:

Show 34 quoted lines

> Hi,
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index d84ba86..c542e8b 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -123,13 +123,40 @@ static int bisect_reset(const char *commit)
>>       return bisect_clean_state();
>>  }
>>
>> +static int is_expected_rev(const char *expected_hex)
>> +{
>> +     struct strbuf actual_hex = STRBUF_INIT;
>> +     int res = 0;
>> +     if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 40) {
>> +             strbuf_trim(&actual_hex);
>> +             res = !strcmp(actual_hex.buf, expected_hex);
>> +     }
>> +     strbuf_release(&actual_hex);
>> +     return res;
>> +}
>
> I am not sure it does what it should.
>
> I would expect the following behavior from this function:
>  - file does not exist (or is "broken") => return 0
>  - actual_hex != expected_hex => return 0
>  - otherwise return 1
>
> If I am not wrong, the code does the following instead:
>  - file does not exist (or is "broken") => return 0
>  - actual_hex != expected_hex => return 1
>  - otherwise => return 0

Yeah, you are right. I should update this. Thanks for pointing it out.

Show 20 quoted lines

>> +static int check_expected_revs(const char **revs, int rev_nr)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < rev_nr; i++) {
>> +             if (!is_expected_rev(revs[i])) {
>> +                     unlink_or_warn(git_path_bisect_ancestors_ok());
>> +                     unlink_or_warn(git_path_bisect_expected_rev());
>> +                     return 0;
>> +             }
>> +     }
>> +     return 0;
>> +}
>
> Here I am not sure what the function *should* do. However, I see that it
> basically mimics the behavior of the shell function (assuming
> is_expected_rev() is implemented correctly).
>
> I don't understand why the return value is int and not void. To avoid a
> "return 0;" line when calling this function?

Initially I thought I would be using the return value but now I realize that it is meaningless to do so. Using void seems better. :)

Show 10 quoted lines

>> @@ -167,6 +196,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>               if (argc > 1)
>>                       die(_("--bisect-reset requires either zero or one arguments"));
>>               return bisect_reset(argc ? argv[0] : NULL);
>> +     case CHECK_EXPECTED_REVS:
>> +             return check_expected_revs(argv, argc);
>
> I note that you check the correct number of arguments for some
> subcommands and you do not check it for some other subcommands like this
> one. (I don't care, I just want to mention it.)

Here we should be able to accept any number of arguments. I think it would be good to add a non-zero check though just to maintain the uniformity. Though this is something programmer needs to be careful about rather than the user.

Show 5 quoted lines

>>       default:
>>               die("BUG: unknown subcommand '%d'", cmdmode);
>>       }
>
> ~Stephan

Regards, Pranit Bauva