Re: [PATCH v16 Part II 1/8] bisect--helper: `bisect_reset` shell function in C

> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> +static int bisect_reset(const char *commit)
>> +{
>> +     struct strbuf branch = STRBUF_INIT;
>> +
>> +     if (!commit) {
>> +             if (strbuf_read_file(&branch, git_path_bisect_start(), 0) < 1)
>> +                     return !printf(_("We are not bisecting.\n"));
>> +             strbuf_rtrim(&branch);
>> +     } else {
>> +             struct object_id oid;
>> +
>> +             if (get_oid_commit(commit, &oid))
>> +                     return error(_("'%s' is not a valid commit"), commit);
>> +             strbuf_addstr(&branch, commit);
>
> The original checks "test -s BISECT_START" and complains, even when
> an explicit commit is given.  With this change, when the user is not
> bisecting, giving "git bisect reset master" goes ahead---it is
> likely that BISECT_HEAD does not exist and we may hit "Could not
> check out" error, but if BISECT_HEAD is left behind from a previous
> run (which is likely completely unrelated to whatever the user
> currently is doing), we'd end up doing quite a random thing, no?

>> +     }
>> +
>> +     if (!file_exists(git_path_bisect_head())) {
>> +             struct argv_array argv = ARGV_ARRAY_INIT;
>> +
>> +             argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
>> +             if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
>> +                     error(_("Could not check out original HEAD '%s'. Try "
>> +                             "'git bisect reset <commit>'."), branch.buf);
>> +                     strbuf_release(&branch);
>> +                     argv_array_clear(&argv);
>> +                     return -1;
>
> How does this return value affect the value eventually given to
> exit(3), called by somewhere in git.c that called this function?
>
> The call graph would be
>
>     common-main.c::main()
>     -> git.c::cmd_main()
>        -> handle_builtin()
>           . exit(run_builtin())
>           -> run_builtin()
>              . status = p->fn()
>              -> cmd_bisect__helper()
>                 . return bisect_reset()
>                 -> bisect_reset()
>                    . return -1
>              . if (status) return status;
>
> So the -1 is returned throughout the callchain and exit(3) ends up
> getting it---which is not quite right.  We shouldn't be giving
> negative value to exit(3).  bisect_clean_state() and other helper
> functions may already share the same issue.

>> +             }
>> +             argv_array_clear(&argv);
>> +     }
>> +
>> +     strbuf_release(&branch);
>> +     return bisect_clean_state();
>> +}