public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail•com>
Cc: git@vger•kernel.org, Jeff King <peff@peff•net>
Subject: Re: [PATCH v2] clean: use warning_errno() when appropriate
Date: Tue, 14 Feb 2017 10:13:12 -0800	[thread overview]
Message-ID: <xmqqh93wabev.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170214095449.15585-1-pclouds@gmail.com> ("Nguyễn Thái Ngọc Duy"'s message of "Tue, 14 Feb 2017 16:54:49 +0700")

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?

next prev parent reply	other threads:[~2017-02-14 18:13 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-13  9:27 [PATCH] clean: use warning_errno() when appropriate Nguyễn Thái Ngọc Duy
2017-02-13 18:34 ` Junio C Hamano
2017-02-13 19:14   ` Jeff King
2017-02-13 20:38     ` Junio C Hamano
2017-02-13 21:10       ` Jeff King
2017-02-13 21:53         ` Junio C Hamano
2017-02-13 22:22           ` Jeff King
2017-02-13 23:48             ` Junio C Hamano
2017-02-14  9:54 ` [PATCH v2] " Nguyễn Thái Ngọc Duy
2017-02-14 18:13   ` Junio C Hamano [this message]
2017-02-15  0:49     ` Duy Nguyen
2017-02-15  1:28       ` Junio C Hamano
2017-02-15  1:36         ` 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=xmqqh93wabev.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=pclouds@gmail$(echo .)com \
    --cc=peff@peff$(echo .)net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox