Re: [PATCH] tempfile: avoid "ferror | fclose" trick
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 17, 2017, 23:52 UTC
- Message-ID
- <xmqqr32wnznf.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170217233952.56h2z3l76orn4zht@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> I'm also not sure that it's all that useful to distinguish errors from > fwrite() versus fclose(). In practice, errors will come from write() in > either case, and the caller does not have much control over when the > flushing happens. So any error saying "error closing file" is probably > assuming too much anyway. It should be "error writing file".
Yes.
Show 13 quoted lines
>> We _could_ clear errno to allow caller to tell them apart, though,
>> if we wanted to ;-)
>
> Hmm. So basically "errno = 0" instead of EIO? That at least has the
> advantage that you can tell it apart from a real EIO, and a caller
> _could_ if they chose do:
>
> if (commit_lock_file(&lock)) {
> if (!errno)
> error("error writing to file");
> else
> error_errno("error closing file");
> }Exactly.
Show 12 quoted lines
> But I am not sure I would want to annotate all such callers that way. It > would probably be less bad to just pass down a "quiet" flag or a strbuf > and have the low-level code fill it in. And that solves this problem > _and_ the rename() thing above. > > But TBH, I am not sure if it is worth it. Nobody is complaining. This is > just a thing we noticed. I think setting errno to EIO or to "0" is a > strict improvement over what is there (where the errno is essentially > random) and the complexity doesn't escape the function. > > So I think that "easy" thing falls far short of a solution, but it's at > least easy. I could take it or leave it at this point.
Well, I already said that earlier in this thread, and ended up queuing your patch on 'pu' ;-).