Re: [PATCH v2 2/2] grep: stop looking at random places for .gitattributes

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

Johannes Sixt <j.sixt@viscovery.net> writes:

> For fflush() (as in write_or_die.c), EINVAL is not even listed
> as possible error code. Therefore, catching EINVAL wholesale should not be
> a problem, IMO, at least not "on other systems".

I see a disturbing gap between "EINVAL is not supposed to be even
possible" and "therefore it is safe to ignore".  Our usual attitude
towards catching errors is to ignore only specific things that are
expected to happen and known to be safe, e.g. the original code
before that patch which special cased to ignore EPIPE with

	if (fflush(f)) {
		if (errno == EPIPE)
                	exit(0);
		die_errno("write failure on '%s'", desc);
	}

where we know we are often downstream of something else and
diagnosing EPIPE as an error is actively wrong.

Unless you assume that *no* other platform has the same glitch as
Windows has with respect to fflush(f) returning EINVAL, or any other
platform that may return EINVAL from fflush(f) has the exactly same
glitch as Windows, ignoring EINVAL unconditionally everywhere is
wrong.  Perhaps the next broken platform may return EINVAL when it
should return EIO or something.  Ideally, that earlier workaround
should have done a logica equivalent of:


	if (fflush(f)) {
#ifdef WINDOWS		
		/*
		 * On Windows, EPIPE is returned only by the first write()
		 * after the reading end has closed its handle; subsequent
		 * write()s return EINVAL.
		 */
                if (errno == EINVAL && this is after a second write)
			errno = EPIPE;
#endif
		if (errno == EPIPE)
                	exit(0);
		die_errno("write failure on '%s'", desc);
	}

and did so not in-line at the calling site but in a compat/ wrapper
for fflush() to eliminate the need for the ifdef.

> But reverting the EINVAL check from write_or_die.c is out of question,
> because that handles a real case.

I am not saying we should not deal with EINVAL on Windows at all.  I
am just saying ignoring EINVAL on other platforms is wrong, and
these two shouldn't have been mutually incompatible goals.