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

[Junio C Hamano <gitster@pobox.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.

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

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