Re: [PATCH v3 4/6] t7500: add tests for --fixup=[amend|reword] options

[Junio C Hamano <gitster@pobox.com>]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Mar 1, 2021 at 3:50 AM Charvi Mendiratta <charvi077@gmail.com> wrote:
>> t7500: add tests for --fixup=[amend|reword] options
>
> It's usually preferable for tests and documentation updates to be
> bundled along with the patch which makes a particular change[1] rather
> than waiting until the very end of the series and adding tests and
> documentation covering all the changes made by patches earlier in the
> series. As a reviewer, it is much harder to tell if the late-added
> tests and documentation updates are comprehensive since it's difficult
> to keep in mind all the changes made by earlier patches.
>
> When reading earlier patches in this series, I questioned whether or
> not certain features of each patch were going to be covered by tests
> or documentation updates, but I couldn't tell because those updates
> weren't made at the same time as the change about which I was reading.
> For instance, when reading the implementation of `--fixup:reword`, I
> was wondering if the documentation was going to be updated to mention
> that it would ignore changes staged in the index and leave the index
> untouched, and I wondered if and hoped that tests would be added to
> verify that the index was indeed left untouched. Over the course of
> many patches, it can be difficult to keep track of all the accumulated
> questions, which makes it onerous to review the final patches adding
> the tests and documentation updates enmasse.
>
> I'm not necessarily suggesting that you re-roll merely to incorporate
> the tests and documentation updates into the patches to which they
> belong, but it's something to keep in mind for future submissions.
>
> FOOTNOTES
>
> [1]: Once in a while a patch introducing a change is so large on its
> own that it may make sense to split tests and documentation updates
> out to their own patches which immediately follow the patch to which
> they apply, but that's different from delaying _all_ tests and
> documentation updates and plopping them at the end of the series all
> crammed together.

A good piece of advice.  Thanks.