Hi Junio, On Tue, 13 Nov 2018, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schinde...@gmx.de> writes: > > >> For a trivially small change/fix like this, it is OK and even > >> preferrable to make 1+2 a single step, as applying t/ part only to > >> try to see the breakage (or "am"ing everything and then "diff | > >> apply -R" the part outside t/ for the same purpose) is easy enough. > > > > I disagree. It helps both development and porting to different branches to > > be able to cherry-pick the regression test individually. Please do not ask > > me to violate this hard-learned principle. > > A trivially small change/fix like this, by definition (of "trivial" > and "small" ness), it is trivial to develop and port to different > branches a single patch, and test with just one half (either the > test part or the code-change part) of the change reversed, to ensure > that the codebase is broken without the code-change and to > demonstrate that the code-change does fix the problem revealed by > the test change. And "porting" by cherry-picking a single patch is > always easier than two patch series. > > So you may disagree all you want in your project, but do not make > reviewer's lives unnecessarily harder in this project. You misunderstand. In this case it is crucial to read the regression test first. The fix does not make much sense before one understands the condition under which the order of the code statements matters. By trying to force me to smoosh them together, you are trying to force me to combine them in one patch where you would read the (now seemingly non-sensical) fix first, and only then the test. That's just really unhelpful. If I were a reviewer, I would want it presented in the way it *was* presented. I firmly believe most reviewers would. Ciao, Dscho
