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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 27, 2017, 17:40 UTC
Message-ID: <xmqqshe4bib0.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <0102015f5e5ee171-f30f4868-886f-47a1-a4e4-b4936afc545d-000000@eu-west-1.amazonses.com>

Pranit Bauva <pranit.bauva@gmail.com> writes:

Show 14 quoted lines

> +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?

Show 12 quoted lines

> +	}
> +
> +	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.

Show 7 quoted lines

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