Re: [PATCH v12 5/5] commit: add a commit.verbose config variable

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 4, 2016, 23:29 UTC
Message-ID: <CAPig+cQdAbweFF7NWJhNjALXN5cdpE-gtvZ_LB0tVmLJ2XGLiA@mail.gmail.com>
In-Reply-To: <CAPig+cTYHFyDXJ2ZECtje2OG-ObT04gnQw+0+LbMMVO51bx2fg@mail.gmail.com>

On Sun, Apr 3, 2016 at 8:58 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 11 quoted lines

> The fact that the 32 new tests are nearly identical suggests strongly
> that the testing should instead either be table-driven or be done via
> for-loops to systematically cover all cases. Not only would either of
> these approaches be easier to digest, but they would make it easy to
> tell at a glance if any cases were missing. See [2] for an example of
> how the tests can be table-driven, and see the bottom of [3] for an
> example of using for-loops to test systematically (though you'd need
> to use nested for-loops rather than just the one in the example).
>
> I'm leaning toward systematic testing via nested for-loops as the more
> suitable of the two approach for this particular application.

By the way, while this would be a nice change, it doesn't necessarily have to be part of this series, and could be done as a follow-up by you or someone else.

(The other changes suggested in the same review, however, ought to be fixed in this series; in particular, simplifying the "setup" test and making the first test after "setup" consistent with the remaining tests.)