Re: [PATCH v4 1/2] usage: Introduce error_errno corresponding to die_errno

[Junio C Hamano <gitster@pobox.com>]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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

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