From: Junio C Hamano <gitster@pobox•com>
To: Jeff King <peff@peff•net>
Cc: "Michael Haggerty" <mhagger@alum•mit.edu>,
"Andreas Schwab" <schwab@linux-m68k•org>,
"Jáchym Barvínek" <jachymb@gmail•com>,
git@vger•kernel.org
Subject: Re: [PATCH] tempfile: avoid "ferror | fclose" trick
Date: Fri, 17 Feb 2017 14:40:19 -0800 [thread overview]
Message-ID: <xmqqzihkphkc.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170217221019.wjuaxmaatqtx2olt@sigill.intra.peff.net> (Jeff King's message of "Fri, 17 Feb 2017 17:10:19 -0500")
Jeff King <peff@peff•net> writes:
>> If we are trying to make sure that the caller would not say "failed
>> to close tempfile: ERRNO" with an ERRNO that is unrelated to any
>> stdio opration, I am not sure if the patch improves things. The
>> caller did not fail to close (most likely we successfully closed
>> it), and no matter what futzing we do to errno, the message supplied
>> by such a caller will not be improved.
>
> Right. EIO is almost certainly _not_ the error we saw. But I would
> rather consistently say "I/O error" and have the user scratch their
> head, look up this thread, and say "ah, it was probably a deferred
> error", as opposed to the alternative: the user sees something
> nonsensical like ENOMEM or EBADF. Those are more misleading, and worse,
> may change from run to run based on what other code runs or fails in
> between.
My point was actually not what errno we feed to strerror(). In that
example, what is more misleading is the fixed part of the error
message the caller of close_tempfile() used after seeing the funcion
fail, i.e. "failed to close". strerror() part is used to explain
why we "failed to close", and of course any nonsensical errno that
we did not get from the failed stdio call would not explain it, but
a more misleading part is that we did not even "failed to close" it.
We just noticed an earlier error while attempting to close it.
strerror() in the message does not even have to be related to the
closing of the file handle.
>> If the caller used "noticed an earlier error while closing tempfile:
>> ERRNO", such a message would describe the situation more correctly,
>> but then ERRNO that is not about stdio is probably acceptable in the
>> context of that message (the original ERRNO might be ENOSPC that is
>> even more specific than EIO, FWIW). So I am not sure if the things
>> will improve from the status quo.
>
> Yes, that's I suggested that xfclose() is probably not a good direction.
> The _best_ thing we can do is have the caller not report errno at all
> (or even say "there was an earlier error, I have no idea what errno
> was"). And xfclose() works in the opposite direction.
I think we are in agreement on this point ;-)
> The only reason I do not think we should do so for close_tempfile() is
> that the fclose is typically far away from the code that actually calls
> error(). We'd have to pass the tristate (success, fail, fail-with-errno)
> state up through the stack (most of the calls indirectly come from
> commit_lock_file(), I would think).
We _could_ clear errno to allow caller to tell them apart, though,
if we wanted to ;-)
next prev parent reply other threads:[~2017-02-17 22:40 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-12 16:37 Confusing git messages when disk is full Jáchym Barvínek
2017-02-15 21:32 ` Jeff King
2017-02-15 21:47 ` Junio C Hamano
2017-02-15 21:51 ` Jeff King
2017-02-15 22:28 ` Junio C Hamano
2017-02-15 22:32 ` Jeff King
2017-02-15 22:50 ` Junio C Hamano
2017-02-15 23:18 ` Jeff King
2017-02-16 10:10 ` Andreas Schwab
2017-02-16 16:44 ` Jeff King
2017-02-16 21:31 ` [PATCH] tempfile: avoid "ferror | fclose" trick Jeff King
2017-02-17 8:00 ` Michael Haggerty
2017-02-17 8:07 ` Jeff King
2017-02-17 10:42 ` Michael Haggerty
2017-02-17 20:54 ` Jeff King
2017-02-17 21:07 ` Jeff King
2017-02-17 21:17 ` Junio C Hamano
2017-02-17 21:21 ` Jeff King
2017-02-17 21:42 ` Junio C Hamano
2017-02-17 22:10 ` Jeff King
2017-02-17 22:40 ` Junio C Hamano [this message]
2017-02-17 23:39 ` Jeff King
2017-02-17 23:52 ` Junio C Hamano
2017-02-17 23:54 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqzihkphkc.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jachymb@gmail$(echo .)com \
--cc=mhagger@alum$(echo .)mit.edu \
--cc=peff@peff$(echo .)net \
--cc=schwab@linux-m68k$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox