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

[Eric Sunshine <sunshine@sunshineco.com>]

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.