Hi Peff, On Fri, 6 Apr 2018, Jeff King wrote: > On Tue, Apr 03, 2018 at 06:27:55PM +0200, Johannes Schindelin wrote: > > > I am very, very grateful for the time Peff spent on reviewing the > > previous iteration, and hope that he realizes just how much the > > elegance of the event-stream-based version is due to his excellent > > review. > > Unfortunately I ran out of time this week to give this version an > equally careful review, and I'm about to go on vacation for a few weeks. No worries, and thank you for your review. I know I am adding more stuff to review these days than I review other stuff, but I promise that I will try to get more reviews in once I am done with this patch series (and with the --rebase-merges one). > I did give a cursory look over it, and the new maybe_remove_section() is > much more pleasant. So aside from a few minor nits I pointed out, this > generally looks good. Thanks! > One thing I'd like to have seen is a few more tests covering exotic > cases that I turned up in my earlier review. Some of the weird multiline > cases I care less about, but we should probably cover at least: > > 1. Comment behavior when removing a section that isn't at the > beginning of the file. > > 2. Removing the final key from a section with a subsection. > > Those should both be natural fallouts of the new method, but it would be > good to have test coverage. I added this, in a new commit I call "t1300: add a few more hairy examples of sections becoming empty". > Thanks for reworking this, and if it's still not merged when I get back, > I promise to review it more carefully then. :) :-) Have a good vacation! Dscho
