On Mon, Jun 06 2022, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>
>> Yes, but I think in this case most of these are either 100% legitimate
>> issues, or things where we'd like to e.g. add a BUG() assertion anyway,
>> e.g. in the diff parsing case.
>
> Where does that "BUG() assertion anyway" comes from? What are we
> protecting against?
>
> I do not think such a BUG() added only for placating -fanalyzer
> falls into "legitimate" category at all.
No, but e.g. in 09/15[1] we *would* have a segfault or NULL dereference if
that function was fed input that it wasn't expecting, which is output
from another git command (or the same output created in-core).
In that case we can reason about it being impossible in the current
state, but annotating that with the equivalent of a BUG("unreachable")
seems prudent, i.e. it helps human readers to understand the code,
whatever -fanalyzer says.
Ditto the pack-related code in [2], where it's noting an aspect I'd
noted in an earlier code review (before I'd ever used
-fanalyzer). I.e. the combination of certain flags could land us in a
NULL dereference, even if we can carefully reason that we won't combine
the two having the code be less easily misunderstood seems better.
1. https://lore.kernel.org/git/RFC-patch-09.15-de0f7722608-20220603T183608Z-avarab@xxxxxxxxx/
2. https://lore.kernel.org/git/RFC-patch-13.15-63eeb66185a-20220603T183608Z-avarab@xxxxxxxxx/
