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

From: Jeff King <peff@peff.net>
Date: Apr 6, 2018, 21:33 UTC
Message-ID: <20180406213358.GD7870@sigill.intra.peff.net>
In-Reply-To: <cover.1522772789.git.johannes.schindelin@gmx.de>

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.

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.

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.

Thanks for reworking this, and if it's still not merged when I get back, I promise to review it more carefully then. :)

-Peff