patchclean: use warning_errno() when appropriate

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/clean.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/clean.c b/builtin/clean.c
> index d6bc3aaaea..dc1168747e 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -175,7 +175,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
>  		res = dry_run ? 0 : rmdir(path->buf);
>  		if (res) {
>  			quote_path_relative(path->buf, prefix, &quoted);
> -			warning(_(msg_warn_remove_failed), quoted.buf);
> +			warning_errno(_(msg_warn_remove_failed), quoted.buf);
>  			*dir_gone = 0;
>  		}
>  		return res;
> @@ -209,7 +209,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
>  				string_list_append(&dels, quoted.buf);
>  			} else {
>  				quote_path_relative(path->buf, prefix, &quoted);
> -				warning(_(msg_warn_remove_failed), quoted.buf);
> +				warning_errno(_(msg_warn_remove_failed), quoted.buf);
>  				*dir_gone = 0;
>  				ret = 1;
>  			}
> @@ -231,7 +231,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
>  			*dir_gone = 1;
>  		else {
>  			quote_path_relative(path->buf, prefix, &quoted);
> -			warning(_(msg_warn_remove_failed), quoted.buf);
> +			warning_errno(_(msg_warn_remove_failed), quoted.buf);
>  			*dir_gone = 0;
>  			ret = 1;
>  		}
> @@ -982,7 +982,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  			res = dry_run ? 0 : unlink(abs_path.buf);
>  			if (res) {
>  				qname = quote_path_relative(item->string, NULL, &buf);
> -				warning(_(msg_warn_remove_failed), qname);
> +				warning_errno(_(msg_warn_remove_failed), qname);
>  				errors++;
>  			} else if (!quiet) {
>  				qname = quote_path_relative(item->string, NULL, &buf);

> 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.

> Jeff King <peff@peff.net> writes:
> 
> > 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().
> 
> I would be lying if I said I did not considered it when I wrote the
> message you are responding to, but I rejected it because that would
> be optimizing for a wrong case, in that most callers of xmalloc()
> and friends do not do so in the error codepath, and we would be
> penalizing them by doing the saved_errno dance unconditionally.

> Jeff King <peff@peff.net> writes:
> 
> > IOW, I think this may be a case where we should be optimizing for
> > programmer time (fewer lines of code, and one less thing to worry about
> > in the callers) versus squeezing out every instruction.
> 
> Fair enough.
> 
> Unless we do the save_errno dance in all the helper functions we
> commonly use to safely stash away errno as necessary and tell
> developers that they can depend on it, the code in the patch that
> began this discussion still needs its own saved_errno dance to be
> safe, though.  I do not have a feeling that we are not there yet,
> even after we teach xmalloc() and its family to do so.

> It would be nice if there was a way to annotate a function as
> errno-safe, and then transitively compute which other functions were
> errno-safe when they do not call any errno-unsafe function. I don't know
> if any static analyzers allow that kind of custom annotation, though
> (and also I wonder whether the cost/benefit of maintaining those
> annotations would be worth it).

> 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.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  v2 dances with errno

>
>  builtin/clean.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/clean.c b/builtin/clean.c
> index d6bc3aaae..3569736f6 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -154,6 +154,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
>  	struct strbuf quoted = STRBUF_INIT;
>  	struct dirent *e;
>  	int res = 0, ret = 0, gone = 1, original_len = path->len, len;
> +	int saved_errno;
>  	struct string_list dels = STRING_LIST_INIT_DUP;
>  
>  	*dir_gone = 1;
> @@ -173,9 +174,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
>  	if (!dir) {
>  		/* an empty dir could be removed even if it is unreadble */
>  		res = dry_run ? 0 : rmdir(path->buf);
> +		saved_errno = errno;
>  		if (res) {
>  			quote_path_relative(path->buf, prefix, &quoted);

> 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.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  v2 dances with errno
>
> Thanks.
>
>>
>>  builtin/clean.c | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/builtin/clean.c b/builtin/clean.c
>> index d6bc3aaae..3569736f6 100644
>> --- a/builtin/clean.c
>> +++ b/builtin/clean.c
>> @@ -154,6 +154,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
>>       struct strbuf quoted = STRBUF_INIT;
>>       struct dirent *e;
>>       int res = 0, ret = 0, gone = 1, original_len = path->len, len;
>> +     int saved_errno;
>>       struct string_list dels = STRING_LIST_INIT_DUP;
>>
>>       *dir_gone = 1;
>> @@ -173,9 +174,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
>>       if (!dir) {
>>               /* an empty dir could be removed even if it is unreadble */
>>               res = dry_run ? 0 : rmdir(path->buf);
>> +             saved_errno = errno;
>>               if (res) {
>>                       quote_path_relative(path->buf, prefix, &quoted);
>
> I think this part should be more like
>
>                 res = ... : rmdir(...);
>                 if (res) {
>                         int saved_errno = errno;
>                         ... do other things that can touch errno ...
>                         errno = saved_errno;
>                         ... now we know what the original error was ...
>
> The reason to store the errno in saved_errno here is not because we
> want to help code after "if (res) {...}", but the patch sent as-is
> gives that impression and is confusing to the readers.
>
> Perhaps all hunks of this patch share the same issue?  I could
> locally amend, of course, but I'd like to double check before doing
> so myself---perhaps you did it this way for a good reason that I am
> missing?

> On Wed, Feb 15, 2017 at 1:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> The reason to store the errno in saved_errno here is not because we
>> want to help code after "if (res) {...}", but the patch sent as-is
>> gives that impression and is confusing to the readers.
>>
>> Perhaps all hunks of this patch share the same issue?  I could
>> locally amend, of course, but I'd like to double check before doing
>> so myself---perhaps you did it this way for a good reason that I am
>> missing?
>
> One thing I like about putting saved_errno right next to the related
> syscall is, the syscall is visible from the diff (previously some are
> out of context). This is really minor though.

> I care more about looking at errno ONLY after we saw our call to a
> system library function indicated an error, and I wanted to avoid
> doing:
> 
> 	res = dry_run ? 0 : rmdir(path);
>         saved_errno = errno;
> 	if (res) {
> 		... do something else ...
> 		errno = saved_errno;
>                 call_something_that_uses_errno();
> 
> When our call to rmdir() did not fail, or when we didn't even call
> rmdir() at all, what is in errno has nothing to do with what we are
> doing, and making a copy of it makes the code doubly confusing.
> 
> Rather, I'd prefer to see:
> 
> 	res = dry_run ? 0 : rmdir(path);
> 	if (res) {
>                 int saved_errno = errno;
> 		... do something else ...
> 		errno = saved_errno;
>                 call_something_that_uses_errno();
> 
> which makes it clear what is going on when reading the resulting
> code.
> 
> For now, I'll queue a separate SQUASH??? and wait for others to
> comment.