Re: [PATCH] clean: use warning_errno() when appropriate

From: Jeff King <peff@peff.net>
Date: Feb 13, 2017, 19:14 UTC
Message-ID: <20170213191433.muwhz7zem64p3rxr@sigill.intra.peff.net>
In-Reply-To: <xmqqwpcudjoh.fsf@gitster.mtv.corp.google.com>

On Mon, Feb 13, 2017 at 10:34:06AM -0800, Junio C Hamano wrote:

Show 24 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> 
> > All these warning() calls are preceded by a system call. Report the
> > actual error to help the user understand why we fail to remove
> > something.
> 
> I think this patch is probably correct in the current code, but I
> say this only after following what quote_path_relative() and
> relative_path() that is called from it.  These warnings are preceded
> by a call to a system library function, but it is not apparent that
> they are immediately preceded without anything else that could have
> failed in between.
> 
>     Side note.  There are many calls into strbuf API in these two
>     functions.  Any calls to xmalloc() and friends made by strbuf
>     functions may see ENOMEM from underlying malloc() and recover by
>     releasing cached resources, by which time the original errno is
>     unrecoverable.  So the above "probably correct" is not strictly
>     true.
> 
> If we care deeply enough that we want to reliably show the errno we
> got from the preceding call to a system library function even after
> whatever comes in between, I think you'd need the usual saved_errno
> trick.  Is that worth it?---I do not offhand have an opinion.

I wonder if xmalloc() should be the one doing the saved_errno trick. After all, it has only two outcomes: we successfully allocated the memory, or we called die().

And that would transitively make most of the strbuf calls errno-safe (except for obvious syscall-related ones like strbuf_read_file). And in turn that makes quote_path_relative() pretty safe (at least when writing to a strbuf).

-Peff