Re: [PATCH v4 1/2] usage: Introduce error_errno corresponding to die_errno
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 8, 2011, 18:10 UTC
- Message-ID
- <7v39kpt64y.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1304839851-6477-2-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 10 quoted lines
> diff --git a/git-compat-util.h b/git-compat-util.h > index 40498b3..7b82038 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -241,6 +241,7 @@ extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, > extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2))); > extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2))); > extern int error(const char *err, ...) __attribute__((format (printf, 1, 2))); > +extern int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2))); > extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
Anybody remotely familiar with die_errno() would at least expect that the new and improved error_errno() function would give the errno neatly formatted via strerror() in the message, but that is not what the function does.
Show 17 quoted lines
> diff --git a/usage.c b/usage.c
> index b5e67e3..c89efcc 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -107,6 +107,16 @@ int error(const char *err, ...)
> return -1;
> }
>
> +int error_errno(const char *err, ...)
> +{
> + va_list params;
> +
> + va_start(params, err);
> + error_routine(err, params);
> + va_end(params);
> + return -errno;
> +}Can error_routine() do something to cause errno to change?
For that matter, I suspect that the caller who would _care_ about what kind of error it got would need to save it away itself anyway. For example, it is not entirely clear if the callsite of this function in your other patch is correct. Can rollback_lock_file() cause errno to change?
git_config(git_rerere_gc_config, NULL);
dir = opendir(git_path("rr-cache"));
if (!dir) {
+ int err = errno;
- rollback_lock_file(&write_lock);
- return error_errno("Unable to open rr-cache directory");
+ error("Untable to open rr-cache directory: %s", strerror(err));
+ return -err;
}I would prefer to do without introducing a confusingly named API function whose first and only usecase does not even suggest it would be useful.