Re: [PATCH v2 00/15] Assorted fixes for `git config` (including the "empty sections" bug)

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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