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

On Sun, Apr 11, 2021 at 10:31 PM Junio C Hamano [off-list ref] wrote:

Miriam Rubio [off-list ref] writes:

quoted

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

Sorry for the late answer. I think the content of the BISECT_RUN file
should indeed be shown.

bisect_state() calls bisect_auto_next() which calls bisect_next()
which calls bisect_next_all(), and bisect_next_all() uses printf() to
show things like "XXX is the first bad commit" which should be shown
when using `git bisect run`.

Also when adding an "exit 1 &&" before "git bisect reset" at the end
of the `"git bisect run" simple case` test, with 'next' I get:

-----------------
$ ./t6030-bisect-porcelain.sh -i -v
...
expecting success of 6030.21 '"git bisect run" simple case':
       write_script test_script.sh <<-\EOF &&
       ! grep Another hello >/dev/null
       EOF
       git bisect start &&
       git bisect good $HASH1 &&
       git bisect bad $HASH4 &&
       git bisect run ./test_script.sh >my_bisect_log.txt &&
       grep "$HASH3 is the first bad commit" my_bisect_log.txt &&
       exit 1 &&
       git bisect reset

Bisecting: 0 revisions left to test after this (roughly 1 step)
[3de952f2416b6084f557ec417709eac740c6818c] Add <3: Another new day for
git> into <hello>.
3de952f2416b6084f557ec417709eac740c6818c is the first bad commit
FATAL: Unexpected exit with code 1
-----------------

and:

-----------------
$ cat trash\ directory.t6030-bisect-porcelain/.git/BISECT_RUN
3de952f2416b6084f557ec417709eac740c6818c is the first bad commit
commit 3de952f2416b6084f557ec417709eac740c6818c
Author: A U Thor [off-list ref]
Date:   Thu Apr 7 15:15:13 2005 -0700

   Add <3: Another new day for git> into <hello>.

hello | 1 +
1 file changed, 1 insertion(+)
-----------------

while with 'seen' I get:

-----------------
$ ./t6030-bisect-porcelain.sh -i -v
...
expecting success of 6030.21 '"git bisect run" simple case':
       write_script test_script.sh <<-\EOF &&
       ! grep Another hello >/dev/null
       EOF
       git bisect start &&
       git bisect good $HASH1 &&
       git bisect bad $HASH4 &&
       git bisect run ./test_script.sh >my_bisect_log.txt &&
       grep "$HASH3 is the first bad commit" my_bisect_log.txt &&
       exit 1 &&
       git bisect reset

Bisecting: 0 revisions left to test after this (roughly 1 step)
[3de952f2416b6084f557ec417709eac740c6818c] Add <3: Another new day for
git> into <hello>.
error: bisect run failed:'git bisect--helper --bisect-state good'
exited with error code -10
running  './test_script.sh'running
'./test_script.sh'3de952f2416b6084f557ec417709eac740c6818c is the
first bad commit
FATAL: Unexpected exit with code 1
-----------------

and:

-----------------
$ cat trash\ directory.t6030-bisect-porcelain/.git/BISECT_RUN
-----------------

So it looks like there might be another issue with what's in 'seen'.

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

Definitely it seems that taking a look at the tests is a good idea.
For example, comparing the verbose (with -v) output of t6030 before
and after each patch (and before and after this patch series) might
help find issues.