Junio C Hamano <gits...@pobox.com> writes: > The former case may split into two. "Yes I agree that is bad and it > is the same badness as the version without this change", in which > case we may want to leave a "NEEDSWORK" comment in-code so that > people can notice when browsing the code (but fixing the badness > would be outside the scope of the series), and "yes that is bad and > we shouldn't introduce that badness", in which case we do want to > fix it in the patch.
We however need to be a bit careful here, though. When a patch series is refactoring an existing function to be used in different codepaths, an existing issue inherited from the original code (e.g. perhaps an existing error checking that is looser than ideal) may have been OK in the original context (e.g. perhaps it died and refused to run until the user corrected the repository), and it still is OK in the codepath that uses the refactored building blocks to replace the original code, but it may not be acceptable to leave the issue in the original code when a new caller calls the refactored building block (e.g. perhaps the refactoring made it possible for a caller to ask the function not to die and instead act on different kinds of errors in different ways). So "being bug-to-bug compatible with existing code" is not always a good thing---we need to see case by case if a problem identified in the review as inherited from the original needs to be addressed in the series. It would be better to address issues we identify even if it is an old one. After all, any change has potential to introduce new bugs, and striving to leave known bug inherited from the old code around would guarantee that the resulting code will be buggier than the original ;-) But in order to keep bisectability, such "further fixups" should be done as a follow-up to the series, not as part of it.
