Re: [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 4, 2017, 05:35 UTC
- Message-ID
- <xmqqh9119obh.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170503101706.9223-5-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 18 quoted lines
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> dir.c | 3 +--
> git-compat-util.h | 2 ++
> wrapper.c | 10 ++++++++++
> 3 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index f451bfa48c..8218a24962 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -745,8 +745,7 @@ static int add_excludes(const char *fname, const char *base, int baselen,
>
> fd = open(fname, O_RDONLY);
> if (fd < 0 || fstat(fd, &st) < 0) {
> - if (errno != ENOENT)
> - warn_on_inaccessible(fname);
> + warn_on_fopen_errors(fname);At least in the original, a reader can guess that, because errno cannot be NOENT if open(2) succeeded and fstat(2) failed, we call warn_on_inaccessible() only when we fail to open.
This change makes it harder to see why this is OK when the failure is not in open(2) but in fstat(2).
fd = open(fname, O_RDONLY);
if (fd < 0 || fstat(fd, &st) < 0) {
- if (errno != ENOENT);
- warn_on_inaccessible(fname);
- if (0 <= fd)
+ if (fd < 0)
+ warn_on_fopen_errors(fname);
+ else
close(fd);
... and the remainder of the original ...or something like that, perhaps?
Show 8 quoted lines
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -418,6 +418,16 @@ FILE *fopen_for_writing(const char *path)
> return ret;
> }
>
> +int warn_on_fopen_errors(const char *path)
> +{Does this need to be returning "int", or should it be "void", because it always warns when there is an issue, the caller does not get to choose to give its own warning?
Show 11 quoted lines
> + if (errno != ENOENT && errno != ENOTDIR) {
> + warn_on_inaccessible(path);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> int xmkstemp(char *template)
> {
> int fd;