mbox series

[v7,00/13] Finish converting git bisect to C part 2

Message ID 20200831105043.97665-1-mirucam@gmail.com (mailing list archive)
Headers show
Series Finish converting git bisect to C part 2 | expand

Message

Miriam R. Aug. 31, 2020, 10:50 a.m. UTC
These patches correspond to a second part of patch series 
of Outreachy project "Finish converting `git bisect` from shell to C" 
started by Pranit Bauva and Tanushree Tumane
(https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and
continued by me.

These patch series emails were generated from:
https://gitlab.com/mirucam/git/commits/git-bisect-work-part2-v7.

I would like to thank Junio Hamano for reviewing this patch series.

General changes
---------------

* Rebased on e9b77c84a0 (Tenth batch, 2020-08-24), to build on the
  strvec API (instead of argv_array) and adjust to the codebase
  after the "--first-parent" feature was added.


Specific changes
----------------

[1/13] bisect--helper: BUG() in cmd_*() on invalid subcommand

* Amend commit message.
* Remove casting to int.

---

[2/13] bisect--helper: use '-res' in 'cmd_bisect__helper' return

* Amend commit message.

---

[3/13] bisect--helper: introduce new `write_in_file()` function

* Use saved_errno variable.

---

[5/13] bisect--helper: reimplement `bisect_autostart` shell function in C

* Fix bug using empty_strvec instead of NULL in a `bisect_start()` call.

---


[6/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
 functions in C
 
* Remove unused `no-checkout` variable.
* Move a comment to more appropriate place.
 
---

.iriam Rubio (4):
  bisect--helper: BUG() in cmd_*() on invalid subcommand
  bisect--helper: use '-res' in 'cmd_bisect__helper' return
  bisect--helper: introduce new `write_in_file()` function
  bisect: call 'clear_commit_marks_all()' in 'bisect_next_all()'

Pranit Bauva (9):
  bisect--helper: reimplement `bisect_autostart` shell function in C
  bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
    functions in C
  bisect--helper: finish porting `bisect_start()` to C
  bisect--helper: retire `--bisect-clean-state` subcommand
  bisect--helper: retire `--next-all` subcommand
  bisect--helper: reimplement `bisect_state` & `bisect_head` shell
    functions in C
  bisect--helper: retire `--check-expected-revs` subcommand
  bisect--helper: retire `--write-terms` subcommand
  bisect--helper: retire `--bisect-autostart` subcommand

 bisect.c                 |  13 +-
 builtin/bisect--helper.c | 442 ++++++++++++++++++++++++++++++++-------
 git-bisect.sh            | 145 +------------
 3 files changed, 380 insertions(+), 220 deletions(-)

Comments

Johannes Schindelin Sept. 3, 2020, 12:04 p.m. UTC | #1
Hi Miriam,


On Mon, 31 Aug 2020, Miriam Rubio wrote:

> These patches correspond to a second part of patch series
> of Outreachy project "Finish converting `git bisect` from shell to C"
> started by Pranit Bauva and Tanushree Tumane
> (https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and
> continued by me.
>
> These patch series emails were generated from:
> https://gitlab.com/mirucam/git/commits/git-bisect-work-part2-v7.

Excellent progress, and thank you for your patience and diligence working
on this patch series. I think we are _really_ close to being ready for
`next`.

Thanks!
Johannes

>
> I would like to thank Junio Hamano for reviewing this patch series.
>
> General changes
> ---------------
>
> * Rebased on e9b77c84a0 (Tenth batch, 2020-08-24), to build on the
>   strvec API (instead of argv_array) and adjust to the codebase
>   after the "--first-parent" feature was added.
>
>
> Specific changes
> ----------------
>
> [1/13] bisect--helper: BUG() in cmd_*() on invalid subcommand
>
> * Amend commit message.
> * Remove casting to int.
>
> ---
>
> [2/13] bisect--helper: use '-res' in 'cmd_bisect__helper' return
>
> * Amend commit message.
>
> ---
>
> [3/13] bisect--helper: introduce new `write_in_file()` function
>
> * Use saved_errno variable.
>
> ---
>
> [5/13] bisect--helper: reimplement `bisect_autostart` shell function in C
>
> * Fix bug using empty_strvec instead of NULL in a `bisect_start()` call.
>
> ---
>
>
> [6/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
>  functions in C
>
> * Remove unused `no-checkout` variable.
> * Move a comment to more appropriate place.
>
> ---
>
> .iriam Rubio (4):
>   bisect--helper: BUG() in cmd_*() on invalid subcommand
>   bisect--helper: use '-res' in 'cmd_bisect__helper' return
>   bisect--helper: introduce new `write_in_file()` function
>   bisect: call 'clear_commit_marks_all()' in 'bisect_next_all()'
>
> Pranit Bauva (9):
>   bisect--helper: reimplement `bisect_autostart` shell function in C
>   bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
>     functions in C
>   bisect--helper: finish porting `bisect_start()` to C
>   bisect--helper: retire `--bisect-clean-state` subcommand
>   bisect--helper: retire `--next-all` subcommand
>   bisect--helper: reimplement `bisect_state` & `bisect_head` shell
>     functions in C
>   bisect--helper: retire `--check-expected-revs` subcommand
>   bisect--helper: retire `--write-terms` subcommand
>   bisect--helper: retire `--bisect-autostart` subcommand
>
>  bisect.c                 |  13 +-
>  builtin/bisect--helper.c | 442 ++++++++++++++++++++++++++++++++-------
>  git-bisect.sh            | 145 +------------
>  3 files changed, 380 insertions(+), 220 deletions(-)
>
> --
> 2.25.0
>
>
Miriam R. Sept. 23, 2020, 7:26 a.m. UTC | #2
Hi Johannes,

El jue., 3 sept. 2020 a las 22:29, Johannes Schindelin
(<Johannes.Schindelin@gmx.de>) escribió:
>
> Hi Miriam,
>
>
> On Mon, 31 Aug 2020, Miriam Rubio wrote:
>
> > These patches correspond to a second part of patch series
> > of Outreachy project "Finish converting `git bisect` from shell to C"
> > started by Pranit Bauva and Tanushree Tumane
> > (https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and
> > continued by me.
> >
> > These patch series emails were generated from:
> > https://gitlab.com/mirucam/git/commits/git-bisect-work-part2-v7.
>
> Excellent progress, and thank you for your patience and diligence working
> on this patch series. I think we are _really_ close to being ready for
> `next`.
>
Thank you for your comments and encouragement.
Applying some of your suggestions related to removing some 'eval' in
git-bisect shell script, a bug has appeared. It seems it is related to
a previous code merged before my internship.
Christian Couder will take over this fix, but in the meantime I will
send a subset of this part2, with the first six patches of this patch
series (and that are not affected by the problem) in order to move
them forward and be accepted, hopefully .
Best,
Miriam




> Thanks!
> Johannes
>
> >
> > I would like to thank Junio Hamano for reviewing this patch series.
> >
> > General changes
> > ---------------
> >
> > * Rebased on e9b77c84a0 (Tenth batch, 2020-08-24), to build on the
> >   strvec API (instead of argv_array) and adjust to the codebase
> >   after the "--first-parent" feature was added.
> >
> >
> > Specific changes
> > ----------------
> >
> > [1/13] bisect--helper: BUG() in cmd_*() on invalid subcommand
> >
> > * Amend commit message.
> > * Remove casting to int.
> >
> > ---
> >
> > [2/13] bisect--helper: use '-res' in 'cmd_bisect__helper' return
> >
> > * Amend commit message.
> >
> > ---
> >
> > [3/13] bisect--helper: introduce new `write_in_file()` function
> >
> > * Use saved_errno variable.
> >
> > ---
> >
> > [5/13] bisect--helper: reimplement `bisect_autostart` shell function in C
> >
> > * Fix bug using empty_strvec instead of NULL in a `bisect_start()` call.
> >
> > ---
> >
> >
> > [6/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
> >  functions in C
> >
> > * Remove unused `no-checkout` variable.
> > * Move a comment to more appropriate place.
> >
> > ---
> >
> > .iriam Rubio (4):
> >   bisect--helper: BUG() in cmd_*() on invalid subcommand
> >   bisect--helper: use '-res' in 'cmd_bisect__helper' return
> >   bisect--helper: introduce new `write_in_file()` function
> >   bisect: call 'clear_commit_marks_all()' in 'bisect_next_all()'
> >
> > Pranit Bauva (9):
> >   bisect--helper: reimplement `bisect_autostart` shell function in C
> >   bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
> >     functions in C
> >   bisect--helper: finish porting `bisect_start()` to C
> >   bisect--helper: retire `--bisect-clean-state` subcommand
> >   bisect--helper: retire `--next-all` subcommand
> >   bisect--helper: reimplement `bisect_state` & `bisect_head` shell
> >     functions in C
> >   bisect--helper: retire `--check-expected-revs` subcommand
> >   bisect--helper: retire `--write-terms` subcommand
> >   bisect--helper: retire `--bisect-autostart` subcommand
> >
> >  bisect.c                 |  13 +-
> >  builtin/bisect--helper.c | 442 ++++++++++++++++++++++++++++++++-------
> >  git-bisect.sh            | 145 +------------
> >  3 files changed, 380 insertions(+), 220 deletions(-)
> >
> > --
> > 2.25.0
> >
> >
Johannes Schindelin Sept. 23, 2020, 2:48 p.m. UTC | #3
Hi Miriam,

On Wed, 23 Sep 2020, Miriam R. wrote:

> Applying some of your suggestions related to removing some 'eval' in
> git-bisect shell script, a bug has appeared. It seems it is related to
> a previous code merged before my internship.

Now you got me curious: what bug did you see?

> Christian Couder will take over this fix, but in the meantime I will
> send a subset of this part2, with the first six patches of this patch
> series (and that are not affected by the problem) in order to move
> them forward and be accepted, hopefully .

Okay, good. Let me know if I can help!

Thanks,
Dscho
Johannes Schindelin Sept. 23, 2020, 9:23 p.m. UTC | #4
Hi Miriam,

On Wed, 23 Sep 2020, Johannes Schindelin wrote:

> On Wed, 23 Sep 2020, Miriam R. wrote:
>
> > Applying some of your suggestions related to removing some 'eval' in
> > git-bisect shell script, a bug has appeared. It seems it is related to
> > a previous code merged before my internship.
>
> Now you got me curious: what bug did you see?

I found your fork and ran the test, and this is the first symptom:

-- snip --
[...]
++ git bisect skip
Bisecting: 1 revision left to test after this (roughly 1 step)
[32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into <hello>.
++ git bisect good
++ grep '3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit' my_bisect_log.txt
3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit
++ git bisect log
++ git bisect reset
Previous HEAD position was 32a594a Add <4: Ciao for now> into <hello>.
Switched to branch 'other'
ok 22 - bisect skip: add line and then a new test

expecting success of 6030.23 'bisect skip and bisect replay':
        git bisect replay log_to_replay.txt > my_bisect_log.txt &&
        grep "$HASH5 is the first bad commit" my_bisect_log.txt &&
        git bisect reset

++ git bisect replay log_to_replay.txt
error: update_ref failed for ref 'refs/bisect/bad': cannot update ref 'refs/bisect/bad': trying to write ref 'refs/bisect/bad' with nonexistent object 10006d020000000068986d020000000054f65f00
error: last command exited with $?=1
not ok 23 - bisect skip and bisect replay
#
#               git bisect replay log_to_replay.txt > my_bisect_log.txt &&
#               grep "$HASH5 is the first bad commit" my_bisect_log.txt &&
#               git bisect reset
-- snap --

So I dug a little bit further (and applied Christian's patch in the
meantime), and it turns out that the `eval` has nothing to do with what I
originally thought it would be required for: I thought that it wanted to
prevent `exit` calls from actually exiting the script.

Instead, those `eval` calls are required because the arguments are
provided in quoted form. For example, during the execution of t6030.68,
the `eval` would expand the call

	eval "git bisect--helper --bisect-start $rev $tail"

to

	git bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'

Therefore, the `eval` really needs to stay in place (also the other `eval`
I had originally suggested to remove, for the same reason).

I would still recommend appending `|| exit`, even if it just so happens
that we will eventually abort when the `bisect--helper` command failed
anyway, because the next command will then fail, and abort. But it's
cleaner to abort already when this invocation failed rather than relying
on that side effect.

Ciao,
Dscho
Christian Couder Sept. 24, 2020, 5:33 a.m. UTC | #5
HI Dscho,

On Wed, Sep 23, 2020 at 11:26 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:

> On Wed, 23 Sep 2020, Johannes Schindelin wrote:
>
> > On Wed, 23 Sep 2020, Miriam R. wrote:
> >
> > > Applying some of your suggestions related to removing some 'eval' in
> > > git-bisect shell script, a bug has appeared. It seems it is related to
> > > a previous code merged before my internship.
> >
> > Now you got me curious: what bug did you see?
>
> I found your fork and ran the test, and this is the first symptom:
>
> -- snip --
> [...]
> ++ git bisect skip
> Bisecting: 1 revision left to test after this (roughly 1 step)
> [32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into <hello>.
> ++ git bisect good
> ++ grep '3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit' my_bisect_log.txt
> 3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit
> ++ git bisect log
> ++ git bisect reset
> Previous HEAD position was 32a594a Add <4: Ciao for now> into <hello>.
> Switched to branch 'other'
> ok 22 - bisect skip: add line and then a new test
>
> expecting success of 6030.23 'bisect skip and bisect replay':
>         git bisect replay log_to_replay.txt > my_bisect_log.txt &&
>         grep "$HASH5 is the first bad commit" my_bisect_log.txt &&
>         git bisect reset
>
> ++ git bisect replay log_to_replay.txt
> error: update_ref failed for ref 'refs/bisect/bad': cannot update ref 'refs/bisect/bad': trying to write ref 'refs/bisect/bad' with nonexistent object 10006d020000000068986d020000000054f65f00
> error: last command exited with $?=1
> not ok 23 - bisect skip and bisect replay
> #
> #               git bisect replay log_to_replay.txt > my_bisect_log.txt &&
> #               grep "$HASH5 is the first bad commit" my_bisect_log.txt &&
> #               git bisect reset
> -- snap --
>
> So I dug a little bit further (and applied Christian's patch in the
> meantime), and it turns out that the `eval` has nothing to do with what I
> originally thought it would be required for: I thought that it wanted to
> prevent `exit` calls from actually exiting the script.
>
> Instead, those `eval` calls are required because the arguments are
> provided in quoted form. For example, during the execution of t6030.68,
> the `eval` would expand the call
>
>         eval "git bisect--helper --bisect-start $rev $tail"
>
> to
>
>         git bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'

Yeah, that was also what I found (along with the bug I sent a patch for).

> Therefore, the `eval` really needs to stay in place (also the other `eval`
> I had originally suggested to remove, for the same reason).
>
> I would still recommend appending `|| exit`, even if it just so happens
> that we will eventually abort when the `bisect--helper` command failed
> anyway, because the next command will then fail, and abort. But it's
> cleaner to abort already when this invocation failed rather than relying
> on that side effect.

Yeah, I think it's a good solution.

Thanks for taking a look,
Christian.
Johannes Schindelin Sept. 24, 2020, 7:56 a.m. UTC | #6
Hi Christian,

On Thu, 24 Sep 2020, Christian Couder wrote:

> On Wed, Sep 23, 2020 at 11:26 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > On Wed, 23 Sep 2020, Johannes Schindelin wrote:
> >
> > > On Wed, 23 Sep 2020, Miriam R. wrote:
> > >
> > > > Applying some of your suggestions related to removing some 'eval' in
> > > > git-bisect shell script, a bug has appeared. It seems it is related to
> > > > a previous code merged before my internship.
> > >
> > > Now you got me curious: what bug did you see?
> >
> > I found your fork and ran the test, and this is the first symptom:
> >
> > -- snip --
> > [...]
> > ++ git bisect skip
> > Bisecting: 1 revision left to test after this (roughly 1 step)
> > [32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into <hello>.
> > ++ git bisect good
> > ++ grep '3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit' my_bisect_log.txt
> > 3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit
> > ++ git bisect log
> > ++ git bisect reset
> > Previous HEAD position was 32a594a Add <4: Ciao for now> into <hello>.
> > Switched to branch 'other'
> > ok 22 - bisect skip: add line and then a new test
> >
> > expecting success of 6030.23 'bisect skip and bisect replay':
> >         git bisect replay log_to_replay.txt > my_bisect_log.txt &&
> >         grep "$HASH5 is the first bad commit" my_bisect_log.txt &&
> >         git bisect reset
> >
> > ++ git bisect replay log_to_replay.txt
> > error: update_ref failed for ref 'refs/bisect/bad': cannot update ref 'refs/bisect/bad': trying to write ref 'refs/bisect/bad' with nonexistent object 10006d020000000068986d020000000054f65f00
> > error: last command exited with $?=1
> > not ok 23 - bisect skip and bisect replay
> > #
> > #               git bisect replay log_to_replay.txt > my_bisect_log.txt &&
> > #               grep "$HASH5 is the first bad commit" my_bisect_log.txt &&
> > #               git bisect reset
> > -- snap --
> >
> > So I dug a little bit further (and applied Christian's patch in the
> > meantime), and it turns out that the `eval` has nothing to do with what I
> > originally thought it would be required for: I thought that it wanted to
> > prevent `exit` calls from actually exiting the script.
> >
> > Instead, those `eval` calls are required because the arguments are
> > provided in quoted form. For example, during the execution of t6030.68,
> > the `eval` would expand the call
> >
> >         eval "git bisect--helper --bisect-start $rev $tail"
> >
> > to
> >
> >         git bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'
>
> Yeah, that was also what I found (along with the bug I sent a patch for).

I suspected that you had found that out, but I really wanted a record on
the Git mailing list about our findings.

It might be a good idea to add a paragraph to the respective patches,
along these lines:

	Note that the `eval` in the changed line of `git-bisect.sh` cannot be
	dropped: it is necessary because the `rev` and the `tail`
	variables may contain multiple, quoted arguments that need to be
	passed to `bisect--helper` (without the quotes, naturally).

> > Therefore, the `eval` really needs to stay in place (also the other `eval`
> > I had originally suggested to remove, for the same reason).
> >
> > I would still recommend appending `|| exit`, even if it just so happens
> > that we will eventually abort when the `bisect--helper` command failed
> > anyway, because the next command will then fail, and abort. But it's
> > cleaner to abort already when this invocation failed rather than relying
> > on that side effect.
>
> Yeah, I think it's a good solution.

Excellent. I think we can actually move forward with the entire patch
series now, not just the first subset, right?

Ciao,
Dscho
Christian Couder Sept. 24, 2020, 10:46 a.m. UTC | #7
Hi Dscho,

On Thu, Sep 24, 2020 at 12:06 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:

> On Thu, 24 Sep 2020, Christian Couder wrote:
>
> > On Wed, Sep 23, 2020 at 11:26 PM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:

> > > Instead, those `eval` calls are required because the arguments are
> > > provided in quoted form. For example, during the execution of t6030.68,
> > > the `eval` would expand the call
> > >
> > >         eval "git bisect--helper --bisect-start $rev $tail"
> > >
> > > to
> > >
> > >         git bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'
> >
> > Yeah, that was also what I found (along with the bug I sent a patch for).
>
> I suspected that you had found that out, but I really wanted a record on
> the Git mailing list about our findings.
>
> It might be a good idea to add a paragraph to the respective patches,
> along these lines:
>
>         Note that the `eval` in the changed line of `git-bisect.sh` cannot be
>         dropped: it is necessary because the `rev` and the `tail`
>         variables may contain multiple, quoted arguments that need to be
>         passed to `bisect--helper` (without the quotes, naturally).

Yeah, sure. Hopefully Miriam will send this in the commit message of
the right patch which is in the subset of the patch series she hasn't
sent.

> > > Therefore, the `eval` really needs to stay in place (also the other `eval`
> > > I had originally suggested to remove, for the same reason).
> > >
> > > I would still recommend appending `|| exit`, even if it just so happens
> > > that we will eventually abort when the `bisect--helper` command failed
> > > anyway, because the next command will then fail, and abort. But it's
> > > cleaner to abort already when this invocation failed rather than relying
> > > on that side effect.
> >
> > Yeah, I think it's a good solution.
>
> Excellent. I think we can actually move forward with the entire patch
> series now, not just the first subset, right?

Yeah, I think so too.

Thanks,
Christian.
Miriam R. Sept. 24, 2020, 12:53 p.m. UTC | #8
Hi,

El jue., 24 sept. 2020 a las 12:46, Christian Couder
(<christian.couder@gmail.com>) escribió:
>
> Hi Dscho,
>
> On Thu, Sep 24, 2020 at 12:06 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > On Thu, 24 Sep 2020, Christian Couder wrote:
> >
> > > On Wed, Sep 23, 2020 at 11:26 PM Johannes Schindelin
> > > <Johannes.Schindelin@gmx.de> wrote:
>
> > > > Instead, those `eval` calls are required because the arguments are
> > > > provided in quoted form. For example, during the execution of t6030.68,
> > > > the `eval` would expand the call
> > > >
> > > >         eval "git bisect--helper --bisect-start $rev $tail"
> > > >
> > > > to
> > > >
> > > >         git bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'
> > >
> > > Yeah, that was also what I found (along with the bug I sent a patch for).
> >
> > I suspected that you had found that out, but I really wanted a record on
> > the Git mailing list about our findings.
> >
> > It might be a good idea to add a paragraph to the respective patches,
> > along these lines:
> >
> >         Note that the `eval` in the changed line of `git-bisect.sh` cannot be
> >         dropped: it is necessary because the `rev` and the `tail`
> >         variables may contain multiple, quoted arguments that need to be
> >         passed to `bisect--helper` (without the quotes, naturally).
>
> Yeah, sure. Hopefully Miriam will send this in the commit message of
> the right patch which is in the subset of the patch series she hasn't
> sent.
Ok. Noted!!
>
> > > > Therefore, the `eval` really needs to stay in place (also the other `eval`
> > > > I had originally suggested to remove, for the same reason).
> > > >
> > > > I would still recommend appending `|| exit`, even if it just so happens
> > > > that we will eventually abort when the `bisect--helper` command failed
> > > > anyway, because the next command will then fail, and abort. But it's
> > > > cleaner to abort already when this invocation failed rather than relying
> > > > on that side effect.
> > >
> > > Yeah, I think it's a good solution.
> >
> > Excellent. I think we can actually move forward with the entire patch
> > series now, not just the first subset, right?
>
> Yeah, I think so too.
Oops, I have already sent the first subset.

Best,
Miriam
>
> Thanks,
> Christian.
Miriam R. Sept. 24, 2020, 12:56 p.m. UTC | #9
Hi Johannes,

El mié., 23 sept. 2020 a las 23:23, Johannes Schindelin
(<Johannes.Schindelin@gmx.de>) escribió:
>
> Hi Miriam,
>
> On Wed, 23 Sep 2020, Johannes Schindelin wrote:
>
> > On Wed, 23 Sep 2020, Miriam R. wrote:
> >
> > > Applying some of your suggestions related to removing some 'eval' in
> > > git-bisect shell script, a bug has appeared. It seems it is related to
> > > a previous code merged before my internship.
> >
> > Now you got me curious: what bug did you see?
>
> I found your fork and ran the test, and this is the first symptom:
>
> -- snip --
> [...]
> ++ git bisect skip
> Bisecting: 1 revision left to test after this (roughly 1 step)
> [32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into <hello>.
> ++ git bisect good
> ++ grep '3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit' my_bisect_log.txt
> 3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit
> ++ git bisect log
> ++ git bisect reset
> Previous HEAD position was 32a594a Add <4: Ciao for now> into <hello>.
> Switched to branch 'other'
> ok 22 - bisect skip: add line and then a new test
>
> expecting success of 6030.23 'bisect skip and bisect replay':
>         git bisect replay log_to_replay.txt > my_bisect_log.txt &&
>         grep "$HASH5 is the first bad commit" my_bisect_log.txt &&
>         git bisect reset
>
> ++ git bisect replay log_to_replay.txt
> error: update_ref failed for ref 'refs/bisect/bad': cannot update ref 'refs/bisect/bad': trying to write ref 'refs/bisect/bad' with nonexistent object 10006d020000000068986d020000000054f65f00
> error: last command exited with $?=1
> not ok 23 - bisect skip and bisect replay
> #
> #               git bisect replay log_to_replay.txt > my_bisect_log.txt &&
> #               grep "$HASH5 is the first bad commit" my_bisect_log.txt &&
> #               git bisect reset
> -- snap --
>
> So I dug a little bit further (and applied Christian's patch in the
> meantime), and it turns out that the `eval` has nothing to do with what I
> originally thought it would be required for: I thought that it wanted to
> prevent `exit` calls from actually exiting the script.
>
> Instead, those `eval` calls are required because the arguments are
> provided in quoted form. For example, during the execution of t6030.68,
> the `eval` would expand the call
>
>         eval "git bisect--helper --bisect-start $rev $tail"
>
> to
>
>         git bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'
>
> Therefore, the `eval` really needs to stay in place (also the other `eval`
> I had originally suggested to remove, for the same reason).
Ok, I will recover your other suggestions too.
>
> I would still recommend appending `|| exit`, even if it just so happens
> that we will eventually abort when the `bisect--helper` command failed
> anyway, because the next command will then fail, and abort. But it's
> cleaner to abort already when this invocation failed rather than relying
> on that side effect.

Aha.Ok
Best,
Miriam
>
> Ciao,
> Dscho
>