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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 9, 2018, 08:19 UTC
Message-ID: <nycvar.QRO.7.76.6.1804091017480.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
In-Reply-To: <20180406213358.GD7870@sigill.intra.peff.net>

Hi Peff,

On Fri, 6 Apr 2018, Jeff King wrote:

Show 9 quoted lines

> 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!

Show 11 quoted lines

> 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