| Message ID | 20200120143800.900-1-mirucam@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Finish converting git bisect to C part 1 | expand |
Hi Miriam, On Mon, 20 Jan 2020, Miriam Rubio wrote: > --- Changes since Tanushree’s pr117 sent patch series: > https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) --- > > General changes > --------------- > > * Rebase on master branch. > * Improve commit messages. > * Amend patch series titles. > * Reorder commits: first clean-up/preparatory commits, squash or split > commits. Great! > Specific changes > ---------------- > > [1/29] bisect--helper: convert `vocab_*` char pointers to char arrays > > * New patch to convert `vocab_bad` and `vocab_good` char pointers > to char arrays 29 patches is _a lot_ to review. I would have preferred a series of smaller patch series. For example, the first three patches would have made for a fine "some cleanups" patch series, from my point of view. Also, as the mail's subject says "part 1", it would be good to have an overview how this part fits into the overall story of converting `git bisect` into a built-in. Finally, it would be nice to have a link to a public repository with the branch from which these mails were generated. I will try to review this patch series in its entirety, but it will take me a while. Ciao, Johannes > > -- > > [2/29] bisect--helper: change `retval` to `res` > > * Replace one last variable `retval` to `res`. > > -- > > [3/29] bisect: use the standard 'if (!var)' way to check for 0 > > * New patch to use '!var' and make 'bisect.c' more consistent with the > rest of the code > > -- > > [4/29] run-command: make `exists_in_PATH()` non-static > > * Add comment before function declaration. > * Move function declaration in `run-command.h`. > > -- > > [6/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` > and its dependents > > * Fix `mark_edges_uninteresting()` and `show_diff_tree()` calls after > rebase on master. > > -- > > [7/29] bisect: libify `bisect_checkout` > > * Fix `memcpy()` call after rebase on master. > * Introduce `res` variable to return `bisect_checkout()` output. > * Fix `get_commit_reference()` declaration after rebase on master. > > -- > > [8/29] bisect: libify `check_merge_bases` and its dependents > > State: Previously sent > > * Fix `check_ancestors()` declaration after rebase on master. > * Fix `get_bad_and_good_commits()` call after rebase on master. > > -- > > [9/29] bisect: libify `check_good_are_ancestors_of_bad` and its > dependents > > State: Previously sent > > * Fix `check_good_are_ancestors_of_bad()` declaration after rebase on > master. > * Fix `check_good_are_ancestors_of_bad()`, `bisect_next_all()` > and `bisect_rev_setup()` calls after rebase on master. > > -- > > [11/29] bisect: libify `bisect_next_all` > > State: Previously sent > > * Fix `show_diff_tree()` call after rebase on master. > > -- > > [12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` > shell functions in C > > * Fix `repo_init_revisions()` and `bisect_next_all()` calls after rebase > on master. > * Remove `goto` statement in `bisect_skipped_commits()` > > -- > > [13/29] bisect--helper: finish porting `bisect_start()` to C > > * Change `return` statement instead of `die` in error handling. > * Remove `goto` statements in `bisect_skipped_commits()`. > > -- > > [21/29] bisect--helper: reimplement `bisect_replay` shell function in C > > * Add blank line in `get_next_word()`. > * Remove `goto` statements in `bisect_replay()`. > > -- > > [23/29] bisect--helper: use `res` instead of return in BISECT_RESET case > option > > * New patch to split previous commit in two. > > -- > > [26/29] bisect--helper: reimplement `bisect_skip` shell function in C > > State: Previously sent > > * Add blank line. > > -- > > [28/29] bisect--helper: reimplement `bisect_visualize()`shell function > in C > > New patch: > > * Reimplement the `bisect_visualize()` shell function in C. > * Add `--bisect-visualize` subcommand. > * Fix long code line. > > -- > > [29/29] bisect--helper: reimplement `bisect_run` shell function in C > > New patch: > > * Reimplement the `bisect_run()` shell function in C. > * Add `--bisect-run` subcommand. > * Remove blank line. > > -- > > Miriam Rubio (2): > bisect--helper: convert `vocab_*` char pointers to char arrays > bisect: use the standard 'if (!var)' way to check for 0 > > Pranit Bauva (24): > run-command: make `exists_in_PATH()` non-static > bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and > its dependents > bisect: libify `bisect_checkout` > bisect: libify `check_merge_bases` and its dependents > bisect: libify `check_good_are_ancestors_of_bad` and its dependents > bisect: libify `handle_bad_merge_base` and its dependents > bisect: libify `bisect_next_all` > 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_autostart` shell function in C > 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: reimplement `bisect_log` shell function in C > bisect--helper: reimplement `bisect_replay` shell function in C > bisect--helper: retire `--bisect-write` subcommand > bisect--helper: use `res` instead of return in BISECT_RESET case > option > bisect--helper: retire `--bisect-autostart` subcommand > bisect--helper: retire `--bisect-auto-next` subcommand > bisect--helper: reimplement `bisect_skip` shell function in C > bisect--helper: retire `--check-and-set-terms` subcommand > bisect--helper: reimplement `bisect_visualize()`shell function in C > > Tanushree Tumane (3): > bisect--helper: change `retval` to `res` > bisect--helper: introduce new `decide_next()` function > bisect--helper: reimplement `bisect_run` shell function in C > > bisect.c | 146 +++++--- > builtin/bisect--helper.c | 776 +++++++++++++++++++++++++++++++++------ > git-bisect.sh | 279 +------------- > run-command.c | 2 +- > run-command.h | 11 + > 5 files changed, 793 insertions(+), 421 deletions(-) > > -- > 2.21.1 (Apple Git-122.3) > >
Hi Dscho, On Mon, Jan 20, 2020 at 10:43 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > On Mon, 20 Jan 2020, Miriam Rubio wrote: > > [1/29] bisect--helper: convert `vocab_*` char pointers to char arrays > > > > * New patch to convert `vocab_bad` and `vocab_good` char pointers > > to char arrays > > 29 patches is _a lot_ to review. I would have preferred a series of > smaller patch series. Yeah, it's possible to split it into smaller patch series. There are a many similar patches in the series so it was easier to work on everything together to make similar and consistent changes to all the patches at once. > For example, the first three patches would have made for a fine "some > cleanups" patch series, from my point of view. Yeah, but this might then be rejected by Junio as it would be only "code churn". > Also, as the mail's subject says "part 1", it would be good to have an > overview how this part fits into the overall story of converting `git > bisect` into a built-in. We don't know how the rest will be split yet. Hopefully there will be only one other smaller patch series after this one. > Finally, it would be nice to have a link to a public repository with the > branch from which these mails were generated. Yeah, I agree that would be nice. > I will try to review this patch series in its entirety, but it will take > me a while. Great, thanks! Christian.
Hi Johannes, El lun., 20 ene. 2020 a las 22:41, Johannes Schindelin (<Johannes.Schindelin@gmx.de>) escribió: > > Hi Miriam, > > On Mon, 20 Jan 2020, Miriam Rubio wrote: > > > --- Changes since Tanushree’s pr117 sent patch series: > > https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) --- > > > > General changes > > --------------- > > > > * Rebase on master branch. > > * Improve commit messages. > > * Amend patch series titles. > > * Reorder commits: first clean-up/preparatory commits, squash or split > > commits. > > Great! > > > Specific changes > > ---------------- > > > > [1/29] bisect--helper: convert `vocab_*` char pointers to char arrays > > > > * New patch to convert `vocab_bad` and `vocab_good` char pointers > > to char arrays > > 29 patches is _a lot_ to review. I would have preferred a series of > smaller patch series. Ok, I will discuss with my mentor how is the best way to split all this work. > > For example, the first three patches would have made for a fine "some > cleanups" patch series, from my point of view. > > Also, as the mail's subject says "part 1", it would be good to have an > overview how this part fits into the overall story of converting `git > bisect` into a built-in. > > Finally, it would be nice to have a link to a public repository with the > branch from which these mails were generated. > This is the link of the current branch: https://gitlab.com/mirucam/git/commits/git-bisect-work2.8.2 I will add the public repository link on next cover-letters. Thank you for the suggestion. > I will try to review this patch series in its entirety, but it will take > me a while. Thank you very much for reviewing. Best, Miriam > > Ciao, > Johannes > > > > > -- > > > > [2/29] bisect--helper: change `retval` to `res` > > > > * Replace one last variable `retval` to `res`. > > > > -- > > > > [3/29] bisect: use the standard 'if (!var)' way to check for 0 > > > > * New patch to use '!var' and make 'bisect.c' more consistent with the > > rest of the code > > > > -- > > > > [4/29] run-command: make `exists_in_PATH()` non-static > > > > * Add comment before function declaration. > > * Move function declaration in `run-command.h`. > > > > -- > > > > [6/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` > > and its dependents > > > > * Fix `mark_edges_uninteresting()` and `show_diff_tree()` calls after > > rebase on master. > > > > -- > > > > [7/29] bisect: libify `bisect_checkout` > > > > * Fix `memcpy()` call after rebase on master. > > * Introduce `res` variable to return `bisect_checkout()` output. > > * Fix `get_commit_reference()` declaration after rebase on master. > > > > -- > > > > [8/29] bisect: libify `check_merge_bases` and its dependents > > > > State: Previously sent > > > > * Fix `check_ancestors()` declaration after rebase on master. > > * Fix `get_bad_and_good_commits()` call after rebase on master. > > > > -- > > > > [9/29] bisect: libify `check_good_are_ancestors_of_bad` and its > > dependents > > > > State: Previously sent > > > > * Fix `check_good_are_ancestors_of_bad()` declaration after rebase on > > master. > > * Fix `check_good_are_ancestors_of_bad()`, `bisect_next_all()` > > and `bisect_rev_setup()` calls after rebase on master. > > > > -- > > > > [11/29] bisect: libify `bisect_next_all` > > > > State: Previously sent > > > > * Fix `show_diff_tree()` call after rebase on master. > > > > -- > > > > [12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` > > shell functions in C > > > > * Fix `repo_init_revisions()` and `bisect_next_all()` calls after rebase > > on master. > > * Remove `goto` statement in `bisect_skipped_commits()` > > > > -- > > > > [13/29] bisect--helper: finish porting `bisect_start()` to C > > > > * Change `return` statement instead of `die` in error handling. > > * Remove `goto` statements in `bisect_skipped_commits()`. > > > > -- > > > > [21/29] bisect--helper: reimplement `bisect_replay` shell function in C > > > > * Add blank line in `get_next_word()`. > > * Remove `goto` statements in `bisect_replay()`. > > > > -- > > > > [23/29] bisect--helper: use `res` instead of return in BISECT_RESET case > > option > > > > * New patch to split previous commit in two. > > > > -- > > > > [26/29] bisect--helper: reimplement `bisect_skip` shell function in C > > > > State: Previously sent > > > > * Add blank line. > > > > -- > > > > [28/29] bisect--helper: reimplement `bisect_visualize()`shell function > > in C > > > > New patch: > > > > * Reimplement the `bisect_visualize()` shell function in C. > > * Add `--bisect-visualize` subcommand. > > * Fix long code line. > > > > -- > > > > [29/29] bisect--helper: reimplement `bisect_run` shell function in C > > > > New patch: > > > > * Reimplement the `bisect_run()` shell function in C. > > * Add `--bisect-run` subcommand. > > * Remove blank line. > > > > -- > > > > Miriam Rubio (2): > > bisect--helper: convert `vocab_*` char pointers to char arrays > > bisect: use the standard 'if (!var)' way to check for 0 > > > > Pranit Bauva (24): > > run-command: make `exists_in_PATH()` non-static > > bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and > > its dependents > > bisect: libify `bisect_checkout` > > bisect: libify `check_merge_bases` and its dependents > > bisect: libify `check_good_are_ancestors_of_bad` and its dependents > > bisect: libify `handle_bad_merge_base` and its dependents > > bisect: libify `bisect_next_all` > > 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_autostart` shell function in C > > 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: reimplement `bisect_log` shell function in C > > bisect--helper: reimplement `bisect_replay` shell function in C > > bisect--helper: retire `--bisect-write` subcommand > > bisect--helper: use `res` instead of return in BISECT_RESET case > > option > > bisect--helper: retire `--bisect-autostart` subcommand > > bisect--helper: retire `--bisect-auto-next` subcommand > > bisect--helper: reimplement `bisect_skip` shell function in C > > bisect--helper: retire `--check-and-set-terms` subcommand > > bisect--helper: reimplement `bisect_visualize()`shell function in C > > > > Tanushree Tumane (3): > > bisect--helper: change `retval` to `res` > > bisect--helper: introduce new `decide_next()` function > > bisect--helper: reimplement `bisect_run` shell function in C > > > > bisect.c | 146 +++++--- > > builtin/bisect--helper.c | 776 +++++++++++++++++++++++++++++++++------ > > git-bisect.sh | 279 +------------- > > run-command.c | 2 +- > > run-command.h | 11 + > > 5 files changed, 793 insertions(+), 421 deletions(-) > > > > -- > > 2.21.1 (Apple Git-122.3) > > > >
Hi Chris, On Tue, 21 Jan 2020, Christian Couder wrote: > On Mon, Jan 20, 2020 at 10:43 PM Johannes Schindelin > <Johannes.Schindelin@gmx.de> wrote: > > > > On Mon, 20 Jan 2020, Miriam Rubio wrote: > > > > [1/29] bisect--helper: convert `vocab_*` char pointers to char arrays > > > > > > * New patch to convert `vocab_bad` and `vocab_good` char pointers > > > to char arrays > > > > 29 patches is _a lot_ to review. I would have preferred a series of > > smaller patch series. > > Yeah, it's possible to split it into smaller patch series. There are a > many similar patches in the series so it was easier to work on > everything together to make similar and consistent changes to all the > patches at once. > > > For example, the first three patches would have made for a fine "some > > cleanups" patch series, from my point of view. > > Yeah, but this might then be rejected by Junio as it would be only "code > churn". I don't think so. We have ample "prior art" where a patch series came with a wonderful cover letter that explained why such a cleanup prepares for a later patch series rather than being code churn. In general, it appears to me that cleanups are much appreciated over here rather than discouraged. I also have to admit that I can justify _a lot better_ to set aside time for reviewing a tiny patch series consisting of three or so pure cleanups on one day, and then a now-smaller patch series to libify `bisect.c` on another day. Much easier to justify than trying to find the time to read through one honking 29-strong patch series, where realistically I have to re-read all of the patches upon the next iteration because I filled my brain with so many other things in the meantime. I think that it would be good mentor advice to pass along: not only the commits are ideally so small as to be _trivial_ to review, also the patch series should be structured in such a way. The scarce resource is the reviewer time, after all. > > Also, as the mail's subject says "part 1", it would be good to have an > > overview how this part fits into the overall story of converting `git > > bisect` into a built-in. > > We don't know how the rest will be split yet. Hopefully there will be > only one other smaller patch series after this one. That is a wonderful contradiction: first you say that you cannot know, and then you state that there will be only one patch series after this ;-) Seriously again, nobody will hold you to a statement like "this is the first of three patch series" when you later explain "originally, I intended this patch series to be the final one, but decided to split it even further, to make it easier on reviewers" or some such. But done is done, v2 was a nice read. Ciao, Dscho
Hi Miriam, On Tue, 21 Jan 2020, Miriam R. wrote: > El lun., 20 ene. 2020 a las 22:41, Johannes Schindelin > (<Johannes.Schindelin@gmx.de>) escribió: > > > > Finally, it would be nice to have a link to a public repository with the > > branch from which these mails were generated. > > > > This is the link of the current branch: > https://gitlab.com/mirucam/git/commits/git-bisect-work2.8.2 > I will add the public repository link on next cover-letters. > Thank you for the suggestion. Excellent, thank you very much! Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > In general, it appears to me that cleanups are much appreciated over here > rather than discouraged. Yup.
--- Changes since Tanushree’s pr117 sent patch series: https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) --- General changes --------------- * Rebase on master branch. * Improve commit messages. * Amend patch series titles. * Reorder commits: first clean-up/preparatory commits, squash or split commits. Specific changes ---------------- [1/29] bisect--helper: convert `vocab_*` char pointers to char arrays * New patch to convert `vocab_bad` and `vocab_good` char pointers to char arrays -- [2/29] bisect--helper: change `retval` to `res` * Replace one last variable `retval` to `res`. -- [3/29] bisect: use the standard 'if (!var)' way to check for 0 * New patch to use '!var' and make 'bisect.c' more consistent with the rest of the code -- [4/29] run-command: make `exists_in_PATH()` non-static * Add comment before function declaration. * Move function declaration in `run-command.h`. -- [6/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents * Fix `mark_edges_uninteresting()` and `show_diff_tree()` calls after rebase on master. -- [7/29] bisect: libify `bisect_checkout` * Fix `memcpy()` call after rebase on master. * Introduce `res` variable to return `bisect_checkout()` output. * Fix `get_commit_reference()` declaration after rebase on master. -- [8/29] bisect: libify `check_merge_bases` and its dependents State: Previously sent * Fix `check_ancestors()` declaration after rebase on master. * Fix `get_bad_and_good_commits()` call after rebase on master. -- [9/29] bisect: libify `check_good_are_ancestors_of_bad` and its dependents State: Previously sent * Fix `check_good_are_ancestors_of_bad()` declaration after rebase on master. * Fix `check_good_are_ancestors_of_bad()`, `bisect_next_all()` and `bisect_rev_setup()` calls after rebase on master. -- [11/29] bisect: libify `bisect_next_all` State: Previously sent * Fix `show_diff_tree()` call after rebase on master. -- [12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C * Fix `repo_init_revisions()` and `bisect_next_all()` calls after rebase on master. * Remove `goto` statement in `bisect_skipped_commits()` -- [13/29] bisect--helper: finish porting `bisect_start()` to C * Change `return` statement instead of `die` in error handling. * Remove `goto` statements in `bisect_skipped_commits()`. -- [21/29] bisect--helper: reimplement `bisect_replay` shell function in C * Add blank line in `get_next_word()`. * Remove `goto` statements in `bisect_replay()`. -- [23/29] bisect--helper: use `res` instead of return in BISECT_RESET case option * New patch to split previous commit in two. -- [26/29] bisect--helper: reimplement `bisect_skip` shell function in C State: Previously sent * Add blank line. -- [28/29] bisect--helper: reimplement `bisect_visualize()`shell function in C New patch: * Reimplement the `bisect_visualize()` shell function in C. * Add `--bisect-visualize` subcommand. * Fix long code line. -- [29/29] bisect--helper: reimplement `bisect_run` shell function in C New patch: * Reimplement the `bisect_run()` shell function in C. * Add `--bisect-run` subcommand. * Remove blank line. -- Miriam Rubio (2): bisect--helper: convert `vocab_*` char pointers to char arrays bisect: use the standard 'if (!var)' way to check for 0 Pranit Bauva (24): run-command: make `exists_in_PATH()` non-static bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents bisect: libify `bisect_checkout` bisect: libify `check_merge_bases` and its dependents bisect: libify `check_good_are_ancestors_of_bad` and its dependents bisect: libify `handle_bad_merge_base` and its dependents bisect: libify `bisect_next_all` 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_autostart` shell function in C 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: reimplement `bisect_log` shell function in C bisect--helper: reimplement `bisect_replay` shell function in C bisect--helper: retire `--bisect-write` subcommand bisect--helper: use `res` instead of return in BISECT_RESET case option bisect--helper: retire `--bisect-autostart` subcommand bisect--helper: retire `--bisect-auto-next` subcommand bisect--helper: reimplement `bisect_skip` shell function in C bisect--helper: retire `--check-and-set-terms` subcommand bisect--helper: reimplement `bisect_visualize()`shell function in C Tanushree Tumane (3): bisect--helper: change `retval` to `res` bisect--helper: introduce new `decide_next()` function bisect--helper: reimplement `bisect_run` shell function in C bisect.c | 146 +++++--- builtin/bisect--helper.c | 776 +++++++++++++++++++++++++++++++++------ git-bisect.sh | 279 +------------- run-command.c | 2 +- run-command.h | 11 + 5 files changed, 793 insertions(+), 421 deletions(-)