Re: [PATCH v11 08/41] refs.c: add an err argument to delete_ref_loose

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 28, 2014, 00:25 UTC
Message-ID: <20140528002538.GI12314@google.com>
In-Reply-To: <1401222360-21175-9-git-send-email-sahlberg@google.com>

Hi,

Comments from http://marc.info/?l=git&m=140079653930751&w=2:

Ronnie Sahlberg wrote:

[...]

Show 27 quoted lines

> --- a/refs.c
> +++ b/refs.c
> @@ -2491,17 +2491,43 @@ static int repack_without_ref(const char *refname)
>  	return repack_without_refs(&refname, 1, NULL);
>  }
>  
> -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)
> +{
> +	int err = errno;
> +	if (rc < 0 && err != ENOENT) {
> +		strbuf_addf(e, "unable to %s %s: %s",
> +			    op, file, strerror(errno));
> +		errno = err;
> +	}
> +	return rc;
> +}
> +
> +static int unlink_or_err(const char *file, struct strbuf *err)
> +{
> +	if (err)
> +		return add_err_if_unremovable("unlink", file, err,
> +					      unlink(file));
> +	else
> +		return unlink_or_warn(file);
> +}

The new unlink_or_err has an odd contract when the err argument is passed. On error:

 * if errno != ENOENT, it will append a message to err and return -1 (good)
 * if errno == ENOENT, it will not append a message to err but will
   still return -1.

This means the caller has to check errno to figure out whether err is meaningful when it returns an error.

Perhaps it should return 0 when errno == ENOENT. After all, in that case the file does not exist any more, which is all we wanted.

Show 18 quoted lines

> +
> +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 && errno != ENOENT) {
> +			if (err)
> +				strbuf_addf(err,
> +					    "failed to delete loose ref '%s'",
> +					    lock->lk->filename);

On failure we seem to add our own message to err, too, so the resulting message would be something like

	fatal: unable to unlink .git/refs/heads/master: \
	Permission deniedfailed to delete loose ref '.git/refs/heads/master.lock'

Is the second strbuf_addf a typo?

Thanks, Jonathan