Re: [PATCHv3 1/9] t7408: modernize style

Stefan Beller [off-list ref] writes:

On Tue, Aug 9, 2016 at 8:51 AM, Junio C Hamano [off-list ref] wrote:

quoted

Junio C Hamano [off-list ref] writes:

quoted

Eric Sunshine [off-list ref] writes:

I originally thought that it may be easier to have 2 patches.
This first one is very gentle and "obviously correct" as it only changes
formatting and drops the directory changes.

The second patch goes for renaming ans subtle style issues,
combining some tests, so it is more likely to break.

After this review, I consider using just one patch and do it all
at once to not confuse the readers as otherwise I should reword
the commit message with the rationale of doing it in two patches.

FWIW, I would think your split between 1/ and 2/ were very sensible,
and have a slight preference for keeping them separate.

If you already have squashed, I do not insist you to split it again;
it is not a big deal either way.

On Tue, Aug 9, 2016 at 1:39 PM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

I originally thought that it may be easier to have 2 patches.
This first one is very gentle and "obviously correct" as it only changes
formatting and drops the directory changes.

The second patch goes for renaming ans subtle style issues,
combining some tests, so it is more likely to break.

After this review, I consider using just one patch and do it all
at once to not confuse the readers as otherwise I should reword
the commit message with the rationale of doing it in two patches.

FWIW, I would think your split between 1/ and 2/ were very sensible,
and have a slight preference for keeping them separate.

The review comment about renaming "current" to "actual" was made
without having looked yet at patch 2. Having now seen patch 2, I agree
with Junio that the existing split is preferable.

So, only the review comment about dropping space after '>' remains relevant.