On Sun, May 8, 2016 at 1:28 AM, Alex R <[email protected]> wrote: > I agree that "atomic patches" (those that do one thing per patch) are a > good thing because they simplify navigating history, do blame and bisect. > But how to define that "one thing"? Some people would say, that a new
If "one thing" can't be defined with regarding to a specific patch, then people don't understand the code, period. > feature is one thing, and if introducing a feature requires some > refactoring, it should be done in the same patch, so that motivation for > the refactoring is clear. On the other hand there will be folks who would That doesn't make sense, refactoring can almost always be done in a separated patch, otherwise it would hurt readability. If they are _logically_ separated, they should be separated. > say that putting all loosely-coupled changes like refactoring, tests, > protobuf update, implementation into the same patch makes it hardly > reviewable and complicates finding problems via blame / bisect. > > Let's take a step back and think, why we need to read commit history in the > first place. Based on my experience, I see several cases: > * Searching which patch introduced a bug, a regression, or some peculiar > behaviour. > * Learning how a feature was implemented and why. > > That's why it is important to separate functional changes from cleanups. > > However, do we still want to separate different sorts of cleanups as well > or squash them together to avoid the churn? If I search for a bug and see a > style fix patch, I simply skip. I'd rather prefer to have one single patch > for all style fixes than a tiny patch for each type of cleanup. This is what Linux kernel has been doing for years. I know people here don't like to be compared with Linux kernel, but you need to realize that it is the best practice. > > Regarding r/46827/ <https://reviews.apache.org/r/46827/>, you are right > that doing audit of include and using sections was not the primary > intention, but I considered it fine to include those changes since they > were not making the code "worse" (modulo removing <stout/check.hpp> which > was a mistake). No one intends to make things worse. It is not a personal taste or something, you have to make it a rule, otherwise same mistake would repeat.
