git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Jan 30, 2020, 15:12 UTC
Message-ID
<nycvar.QRO.7.76.6.2001301605110.46@tvgsbejvaqbjf.bet>
In-Reply-To
<CAP8UFD0W=kBs1zne56OjPbFzeX7BtEazPh2MW1My3qAsByS=nA@mail.gmail.com>
Hi Chris,
On Tue, 21 Jan 2020, Christian Couder wrote:
Show 23 quoted lines
> 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.

Show 6 quoted lines
> > 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

Previous: Christian CouderNext: Junio C Hamano
Message 51 of 54 in “Finish converting git bisect to C part 1”
  1. Miriam RubioJan 20, 2020
  2. 03/29 bisect: use the standard 'if (!var)' way to check for 0Miriam Rubio, Jan 20, 2020
  3. 01/29 bisect--helper: convert `vocab_*` char pointers to char arraysMiriam Rubio, Jan 20, 2020
  4. 02/29 bisect--helper: change `retval` to `res`Miriam Rubio, Jan 20, 2020
  5. 07/29 bisect: libify `bisect_checkout`Miriam Rubio, Jan 20, 2020
  6. 08/29 bisect: libify `check_merge_bases` and its dependentsMiriam Rubio, Jan 20, 2020
  7. Johannes SchindelinJan 20, 2020
  8. Miriam R.Jan 21, 2020
  9. 04/29 run-command: make `exists_in_PATH()` non-staticMiriam Rubio, Jan 20, 2020
  10. 09/29 bisect: libify `check_good_are_ancestors_of_bad` and its dependentsMiriam Rubio, Jan 20, 2020
  11. Johannes SchindelinJan 20, 2020
  12. Christian CouderJan 21, 2020
  13. Miriam R.Jan 21, 2020
  14. 05/29 bisect--helper: introduce new `decide_next()` functionMiriam Rubio, Jan 20, 2020
  15. 10/29 bisect: libify `handle_bad_merge_base` and its dependentsMiriam Rubio, Jan 20, 2020
  16. Johannes SchindelinJan 20, 2020
  17. Christian CouderJan 21, 2020
  18. Miriam R.Jan 21, 2020
  19. 11/29 bisect: libify `bisect_next_all`Miriam Rubio, Jan 20, 2020
  20. Johannes SchindelinJan 20, 2020
  21. Christian CouderJan 21, 2020
  22. Johannes SchindelinJan 30, 2020
  23. 12/29 bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in CMiriam Rubio, Jan 20, 2020
  24. Johannes SchindelinJan 30, 2020
  25. Miriam R.Jan 31, 2020
  26. Christian CouderFeb 17, 2020
  27. Johannes SchindelinFeb 17, 2020
  28. 13/29 bisect--helper: finish porting `bisect_start()` to CMiriam Rubio, Jan 20, 2020
  29. 06/29 bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependentsMiriam Rubio, Jan 20, 2020
  30. Johannes SchindelinJan 20, 2020
  31. Christian CouderJan 21, 2020
  32. Miriam R.Jan 21, 2020
  33. 14/29 bisect--helper: retire `--bisect-clean-state` subcommandMiriam Rubio, Jan 20, 2020
  34. 15/29 bisect--helper: retire `--next-all` subcommandMiriam Rubio, Jan 20, 2020
  35. 17/29 bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in CMiriam Rubio, Jan 20, 2020
  36. 16/29 bisect--helper: reimplement `bisect_autostart` shell function in CMiriam Rubio, Jan 20, 2020
  37. 19/29 bisect--helper: retire `--write-terms` subcommandMiriam Rubio, Jan 20, 2020
  38. 18/29 bisect--helper: retire `--check-expected-revs` subcommandMiriam Rubio, Jan 20, 2020
  39. 20/29 bisect--helper: reimplement `bisect_log` shell function in CMiriam Rubio, Jan 20, 2020
  40. 21/29 bisect--helper: reimplement `bisect_replay` shell function in CMiriam Rubio, Jan 20, 2020
  41. 22/29 bisect--helper: retire `--bisect-write` subcommandMiriam Rubio, Jan 20, 2020
  42. 24/29 bisect--helper: retire `--bisect-autostart` subcommandMiriam Rubio, Jan 20, 2020
  43. 25/29 bisect--helper: retire `--bisect-auto-next` subcommandMiriam Rubio, Jan 20, 2020
  44. 23/29 bisect--helper: use `res` instead of return in BISECT_RESET case optionMiriam Rubio, Jan 20, 2020
  45. 26/29 bisect--helper: reimplement `bisect_skip` shell function in CMiriam Rubio, Jan 20, 2020
  46. 27/29 bisect--helper: retire `--check-and-set-terms` subcommandMiriam Rubio, Jan 20, 2020
  47. 28/29 bisect--helper: reimplement `bisect_visualize()`shell function in CMiriam Rubio, Jan 20, 2020
  48. 29/29 bisect--helper: reimplement `bisect_run` shell function in CMiriam Rubio, Jan 20, 2020
  49. Johannes SchindelinJan 20, 2020
  50. Christian CouderJan 20, 2020
  51. Johannes SchindelinJan 30, 2020
  52. Junio C HamanoJan 30, 2020
  53. Miriam R.Jan 21, 2020
  54. Johannes SchindelinJan 30, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.