Hey Stephan,
On Thu, Nov 17, 2016 at 5:17 AM, Stephan Beyer <s-beyer@xxxxxxx> wrote:
> 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.
>> +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. :)
>> @@ -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.
>> default:
>> die("BUG: unknown subcommand '%d'", cmdmode);
>> }
>
> ~Stephan
Regards,
Pranit Bauva