Re: [PATCH v3 3/4] bisect--helper: reimplement `bisect_run` shell function in C

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 11, 2021, 20:31 UTC
Message-ID: <xmqqy2doftrj.fsf@gitster.g>
In-Reply-To: <20210411095538.34129-4-mirucam@gmail.com>

Miriam Rubio <mirucam@gmail.com> writes:

> +		if (res < 0 || 128 <= res) {
> +			error(_("bisect run failed: exit code %d from"
> +				" '%s' is < 0 or >= 128"), res, command.buf);

Good now.

Show 9 quoted lines

> +		temporary_stdout_fd = open(git_path_bisect_run(), O_CREAT | O_WRONLY | O_TRUNC, 0666);
> +		saved_stdout = dup(1);
> +		dup2(temporary_stdout_fd, 1);
> +
> +		res = bisect_state(terms, args.v, args.nr);
> +
> +		dup2(saved_stdout, 1);
> +		close(saved_stdout);
> +		close(temporary_stdout_fd);

In v2, we just let bisect_state() to write to our standard output, which was the reason why the loss of "cat" in the "write to BISECT_RUN file and cat it to show to the user" in the scripted version in v2 was OK. Now, we are writing out, just like the scripted version did, to BISECT_RUN, the user does not see its contents.

Does the distinction matter?  Christian, what's your call on this?

If it does not matter, then the code and tests are good as-is, but if it does, the fact that you posted this round without noticing any broken tests means that we have a gap in the test coverage. Can we have a new test about this (i.e. at the end of each round in "bisect run", the output from bisect_state() is shown to the user)?

Show 11 quoted lines

> +		if (res == BISECT_ONLY_SKIPPED_LEFT)
> +			error(_("bisect run cannot continue any more"));
> +		else if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) {
> +			printf(_("bisect run success"));
> +			res = BISECT_OK;
> +		} else if (res) {
> +			error(_("bisect run failed:'git bisect--helper --bisect-state"
> +			" %s' exited with error code %d"), args.v[0], res);
> +		} else {
> +			exit = 0;
> +		}