Re: [PATCH v20 40/48] refs.c: add an err argument to delete_ref_loose

[Michael Haggerty <mhagger@alum.mit.edu>]

On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> Add an err argument to delete_loose_ref so that we can pass a descriptive
> error string back to the caller. Pass the err argument from transaction
> commit to this function so that transaction users will have a nice error
> string if the transaction failed due to delete_loose_ref.
> 
> Add a new function unlink_or_err that we can call from delete_ref_loose. This
> function is similar to unlink_or_warn except that we can pass it an err
> argument. If err is non-NULL the function will populate err instead of
> printing a warning().
> 
> Simplify warn_if_unremovable.

The change to warn_if_unremovable() is orthogonal to the rest of the
commit and should be a separate commit.

> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  refs.c    | 33 ++++++++++++++++++++++++++++-----
>  wrapper.c | 14 ++++++--------
>  2 files changed, 34 insertions(+), 13 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 92a06d4..c7d1f3e 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2544,16 +2544,38 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)
>  	return ret;
>  }
>  
> -static int delete_ref_loose(struct ref_lock *lock, int flag)
> +static int add_err_if_unremovable(const char *op, const char *file,
> +				  struct strbuf *e, int rc)

This function is only used once.  Given also that its purpose is not
that obvious from its signature, it seems to me that the code would be
easier to read if it were inlined.

> +{
> +	int err = errno;
> +	if (rc < 0 && errno != ENOENT) {
> +		strbuf_addf(e, "unable to %s %s: %s",
> +			    op, file, strerror(errno));
> +		errno = err;
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +static int unlink_or_err(const char *file, struct strbuf *err)

The name of this function is misleading; it sounds like it will try to
unlink the file and if not possible call error().  Maybe a name like
"unlink_or_report" would be less prejudicial.

It might also make sense to move this function to wrapper.c and
implement unlink_or_warn() in terms of it rather than vice versa.

> +{
> +	if (err)
> +		return add_err_if_unremovable("unlink", file, err,
> +					      unlink(file));
> +	else
> +		return unlink_or_warn(file);
> +}
> +
> +static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
>  {
>  	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
>  		/* loose */
> -		int err, i = strlen(lock->lk->filename) - 5; /* .lock */
> +		int res, i = strlen(lock->lk->filename) - 5; /* .lock */
>  
>  		lock->lk->filename[i] = 0;
> -		err = unlink_or_warn(lock->lk->filename);
> +		res = unlink_or_err(lock->lk->filename, err);
>  		lock->lk->filename[i] = '.';
> -		if (err && errno != ENOENT)
> +		if (res)
>  			return 1;
>  	}
>  	return 0;
> @@ -3603,7 +3625,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  		struct ref_update *update = updates[i];
>  
>  		if (update->lock) {
> -			ret |= delete_ref_loose(update->lock, update->type);
> +			ret |= delete_ref_loose(update->lock, update->type,
> +						err);
>  			if (!(update->flags & REF_ISPRUNING))
>  				delnames[delnum++] = update->lock->ref_name;
>  		}
> diff --git a/wrapper.c b/wrapper.c
> index bc1bfb8..740e193 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -429,14 +429,12 @@ int xmkstemp_mode(char *template, int mode)
>  
>  static int warn_if_unremovable(const char *op, const char *file, int rc)
>  {
> -	if (rc < 0) {
> -		int err = errno;
> -		if (ENOENT != err) {
> -			warning("unable to %s %s: %s",
> -				op, file, strerror(errno));
> -			errno = err;
> -		}
> -	}
> +	int err;
> +	if (rc >= 0 || errno == ENOENT)
> +		return rc;
> +	err = errno;
> +	warning("unable to %s %s: %s", op, file, strerror(errno));
> +	errno = err;
>  	return rc;
>  }
>  
> 

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu