From: Junio C Hamano <gitster@pobox.com>
To: Miriam Rubio <mirucam@gmail.com>
Cc: git@vger.kernel.org, Tanushree Tumane <tanushreetumane@gmail.com>,
Christian Couder <chriscool@tuxfamily.org>
Subject: Re: [PATCH v3 3/4] bisect--helper: reimplement `bisect_run` shell function in C
Date: Sun, 11 Apr 2021 13:31:12 -0700 [thread overview]
Message-ID: <xmqqy2doftrj.fsf@gitster.g> (raw)
In-Reply-To: <20210411095538.34129-4-mirucam@gmail.com> (Miriam Rubio's message of "Sun, 11 Apr 2021 11:55:37 +0200")
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;
> + }
next prev parent reply other threads:[~2021-04-11 20:31 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-11 9:55 [PATCH v3 0/4] Finish converting git bisect to C part 4 Miriam Rubio
2021-04-11 9:55 ` [PATCH v3 1/4] run-command: make `exists_in_PATH()` non-static Miriam Rubio
2021-04-11 9:55 ` [PATCH v3 2/4] bisect--helper: reimplement `bisect_visualize()`shell function in C Miriam Rubio
2021-04-11 20:22 ` Junio C Hamano
2021-04-11 9:55 ` [PATCH v3 3/4] bisect--helper: reimplement `bisect_run` shell " Miriam Rubio
2021-04-11 20:31 ` Junio C Hamano [this message]
2021-04-11 20:33 ` Junio C Hamano
2021-05-05 9:04 ` Christian Couder
2021-05-04 17:26 ` Andrzej Hunt
2021-05-05 2:04 ` Junio C Hamano
2021-04-11 9:55 ` [PATCH v3 4/4] bisect--helper: retire `--bisect-next-check` subcommand Miriam Rubio
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqy2doftrj.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=chriscool@tuxfamily.org \
--cc=git@vger.kernel.org \
--cc=mirucam@gmail.com \
--cc=tanushreetumane@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).