Hey Junio,
On Fri, Oct 27, 2017 at 11:10 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Pranit Bauva <pranit.bauva@xxxxxxxxx> 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?
Yes. Thanks for mentioning this point. I don't quite remember things
right now about what made me do this change. There might have been
something which had made me do this change because this isn't just a
silly mistake. Any which ways, I couldn't recollect the reason (should
be more careful to put code comments).
>> + }
>> +
>> + 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.
I had totally missed that exit() takes only single byte value and thus
only positive integers. I think changing it to "return 1;" will do.
There are a few places in the previous series which use "return -1;"
which would need to be changed. I will resend that series.
>> + }
>> + argv_array_clear(&argv);
>> + }
>> +
>> + strbuf_release(&branch);
>> + return bisect_clean_state();
>> +}
Regards,
Pranit Bauva
