Re: [PATCH v5 10/11] setup_git_directory_gently_1(): avoid die()ing

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@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.