On 05/16/2013 05:47 AM, Jeff King wrote: >> I probably would have separated the rest of the patch, which is a pure >> refactoring, from this last chunk, which is a functional change. But >> that's just me. > > Yeah, I go back and forth on whether it is better to have strict > refactors followed by changes or not. Sometimes it is hard to understand > the motivation for the refactor without seeing the change, and you end > up explaining it twice.
A pure refactoring doesn't need much justification. Something like "extract function foo()" plus maybe "this function will soon have multiple callers" is IMO usually adequate, especially if the function is well-named and documented in the patch itself. > My usual rule of thumb is: > > - If you are factoring out some code, and then are going to change > that code, make it two separate changes. That keeps the diffs > readable (the first one is pure movement and you do not need to look > closely, and the second shows a sane diff of the change). > > - If you are factoring out some code, and then adding more callers, > it's OK to do it together. The new caller provides the motivation > for the refactor. > > This is the latter case. But I'm open to arguments that the rule is not > a good one. :) Yes, I see how keeping the changes together makes the justification of the refactoring more obvious. On the other hand, splitting has the following benefits: 1. Reviewers have a single thing to check in each patch: "Did he transcribe the code correctly into a function and choose a good API?" vs. "Does it make sense to call the function from this new place?" The threads of feedback emails about each patch are similarly separated. On the other hand, of course these two changes are not completely independent, because having an idea what new callers want to do with the function affects what its API should be. 2. If the patch series needs to be revised, it is quite possible that the revisions affect only one patch or the other. Therefore, the unaffected patch can be carried along through interactive rebases etc. intact, or might serve as a building block for an alternative solution. 3. If there's a problem, bisect can figure out which half of the change was to blame. That being said, this case is very much in the gray area where it is a matter of personal preference and I don't mind at all if you leave it as a single patch. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
