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

[Christian Couder <christian.couder@gmail.com>]

On Sun, Apr 11, 2021 at 10:31 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Miriam Rubio <mirucam@gmail.com> writes:

> > +             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 <author@example.com>
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.