Hi Peff,

On Fri, 28 Apr 2017, Jeff King wrote:

On Fri, Apr 28, 2017 at 12:41:02PM +0200, Johannes Schindelin wrote:

quoted

But then, I guess I misunderstood what Coverity complained about:
maybe the problem was not so much the isspace() call but that EOF is
not being handled correctly. We pass it, unchecked, to ungetc().

It appears that I (or Coverity, if you will), missed another instance
where we simply passed EOF unchecked to ungetc().

I think that is also fine according to the standard.

Do you happen to have the exact error from Coverity?

Wow, that was unnecessarily hard. It is a major hassle to get to any
scan other than the latest one.

But I did it. Call me tenatious.

The report says this:

233        do {
   2. negative_return_fn: Function mingw_fgetc(f) returns a negative number.
   3. var_assign: Assigning: signed variable peek = mingw_fgetc.
234                peek = fgetc(f);
   CID 1049734: Negative array index read (NEGATIVE_RETURNS)
   4.  negative_returns: Using variable peek as an index to array sane_ctype.
235        } while (isspace(peek));
236        ungetc(peek, f);

So part of the thing is that we use mingw_fgetc() instead of fgetc().
However, the return value is *still* the one from the "real" fgetc(), even
if we intercept what appears to be a Ctrl+C from an interactive console.

I'm wondering if it is complaining about some aspect of our custom
isspace() when used with EOF.

That would appear to be the real issue, yes, and I should have
double-checked the claim that POSIX isspace() handles EOF properly: we
override isspace() with our own version, after all:

	#define isspace(x) sane_istest(x,GIT_SPACE)

where

	#define sane_istest(x,mask) \
		((sane_ctype[(unsigned char)(x)] & (mask)) != 0)

(rewrapped for readability)

As usual, EOF is defined as -1 in Git for Windows' context, meaning that
we look at the last entry of the sane_ctype array, which returns 0 for any
sane_istest(x,mask) test for x >= 0x80:

        /* Nothing in the 128.. range */

So it would appear that it happens to work, but I doubt that it was
intentional.

Having said that, it is really curious why Coverity should get confused by
the code and not realize that casting a negative number to (unsigned char)
will make it valid as an index for the sane_ctype array.

I double-checked, and there is no override for the isspace() function in
what Coverity calls a "model file" (i.e. pseudo code intended to helping
Coverity realize where it can stop reporting false positives).

quoted

The next iteration will have it completely reworked: I no longer guard
the isspace() behind an `!= EOF` check, but rather handle an early EOF
as I think it should be handled. Extra eyes very welcome (this is the
fixup!  patch):

I do think handling EOF explicitly is probably a better strategy anyway,
as it lets us tell when we have an empty patch.

I agree, I came to the same conclusion independently.

Ciao,
Dscho