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