Re: Nitpicking: unnecessary NULL-pointer check in pg_upgrade's controldata.c

On Fri, Jun 26, 2015 at 10:21 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Fri, Jun 26, 2015 at 9:49 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>> It takes about three seconds to mark it as ignored which will hide it
>>> going forward.
>
>> So what? That doesn't help if someone *else* sets up a Coverity run
>> on this code base, or if say Salesforce sets up such a run on their
>> fork of the code base. It's much better to fix the problem at the
>> root.
>
> The problem with that is allowing Coverity, which in the end is not magic
> but just another piece of software with many faults, to define what is a
> "problem". In this particular case, the only effect of the change that
> I can see is to make the code less flexible, and less robust against a
> fairly obvious type of future change. So I'm not on board with removing
> if-guards just because Coverity thinks they are unnecessary.
>
> I agree that the correct handling of this particular case is to mark it
> as not-a-bug. We have better things to do.

Well, I find that a disappointing conclusion, but I'm not going to
spend a lot of time arguing against both of you. But, for what it's
worth: it's not as if somebody is going to modify the code in that
function to make output == NULL a plausible option, so I think the
change could easily be justified on code clean-up grounds if nothing
else. There's not much point calling fgets on a FILE unconditionally
and then immediately thereafter allowing for the possibility that
output might be NULL. That's not easing the work of anyone who might
want to modify that code in the future; it just makes the code more
confusing.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company