Re: [PATCH 02/12] wrapper.c: add a new function unlink_or_msg

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

Ronnie Sahlberg <sahlberg@google.com> writes:

> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  git-compat-util.h |  6 ++++++
>  wrapper.c         | 18 ++++++++++++++++++
>  2 files changed, 24 insertions(+)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index b6f03b3..426bc98 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -704,12 +704,18 @@ void git_qsort(void *base, size_t nmemb, size_t size,
>  #endif
>  #endif
>  
> +#include "strbuf.h"
> +
>  /*
>   * Preserves errno, prints a message, but gives no warning for ENOENT.
>   * Always returns the return value of unlink(2).
>   */
>  int unlink_or_warn(const char *path);
>  /*
> + * Like unlink_or_warn but populates a strbuf
> + */
> +int unlink_or_msg(const char *file, struct strbuf *err);
> +/*
>   * Likewise for rmdir(2).
>   */
>  int rmdir_or_warn(const char *path);
> diff --git a/wrapper.c b/wrapper.c
> index 740e193..74a0cc0 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -438,6 +438,24 @@ static int warn_if_unremovable(const char *op, const char *file, int rc)
>  	return rc;
>  }
>  
> +int unlink_or_msg(const char *file, struct strbuf *err)
> +{
> +	if (err) {
> +		int rc = unlink(file);
> +		int save_errno = errno;
> +
> +		if (rc < 0 && errno != ENOENT) {
> +			strbuf_addf(err, "unable to unlink %s: %s",
> +				    file, strerror(errno));
> +			errno = save_errno;
> +			return -1;
> +		}
> +		return 0;
> +	}
> +
> +	return unlink_or_warn(file);
> +}

In general, I do not generally like to see messages propagated
upwards from deeper levels of the callchain to the callers to be
used later, primarily because that will easily make it harder to
localize the message-lego.

For this partcular one, shouldn't the caller be doing

	if (unlink(file) && errno != ENOENT) {
        	... do its own error message ...
	}

instead of calling any of the unlink_or_whatever() helper?


>  int unlink_or_warn(const char *file)
>  {
>  	return warn_if_unremovable("unlink", file, unlink(file));