On Tue, Sep 26, 2017 at 01:11:59PM +0900, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> >> #ifndef EUNDERFLOW
> >> # ifdef ENODATA
> >> # define EUNDERFLOW ENODATA
> >> # else
> >> # define EUNDERFLOW ESPIPE
> >> # endif
> >> #endif
> >
> > Right, I think our mails just crossed but I'm leaning in this direction.
>
> Hmph, I may be slow (or may be skimming the exchanges too fast), but
> what exactly is wrong with "0"? As long as we do not have to tell
> two or more "not exactly an error from syscall in errno" cases, I
> would think "0" is the best value to use.
The main reason to avoid "0" is just that the "read error: Success"
message is a bit funny.
> If the syserror message _is_ the issue, then we'd need to either
> pick an existing errno that is available everywhere (with possibly
> suboptimal message), or pick something and prepare a fallback for
> platforms that lack the errno, so picking "0" as the value and use
> whatever logic we would have used for the "fallback" would not sound
> too bad. I.e.
>
> if (read_in_full(..., size) != size)
> if (errno)
> die_errno("oops");
> else
> die("short read");
>
> If the callsite were too numerous,
You actually don't need errno for that. You can write:
ret = read_in_full(..., size);
if (ret < 0)
die_errno("oops");
else if (ret != size)
die("short read");
So I think using errno as a sentinel value to tell between the two cases
doesn't have much value.
> #define die_errno_or(msg1, msg2) if (errno) die_errno(msg1); else die(msg2)
>
> perhaps?
If you just care about dying, you can wrap the whole read_in_full() in a
helper (which is what my original patch 7 did). But I found there were
cases that didn't want to die. Of course _most_ of those don't bother
looking at errno in the first place. I think the only one that does is
index_core(), which calls error_errno().
So with two helpers (one for die and one for error), I think we could
cover the existing cases. Or we could convert the single error() case to
just handle the logic inline.
One of the reasons I dislike the helper I showed earlier is that it
involves assembling a lego sentence. But the alternative is requiring
the caller to write both sentences from scratch (and writing a good
sentence for the short read really is long: you expected X bytes but got
Y).
I guess just tweaking the errno value does not really give a good "short
read" error either. You get "unable to read foo: No data available",
without the X/Y information.
Sometimes a custom "short read" message can be better, if it's "could
not read enough bytes for packfile header" or similar. You don't need to
know the exact number then.
So I dunno. Maybe I have just talked myself into actually just fixing
all of the read_in_full() call sites manually.
-Peff
