Hi Chris, On Tue, 21 Jan 2020, Christian Couder wrote: > On Mon, Jan 20, 2020 at 10:43 PM Johannes Schindelin > <Johannes.Schindelin@xxxxxx> 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
