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

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

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.

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.