Hi,
Comments from http://marc.info/?l=git&m=140079653930751&w=2:
Ronnie Sahlberg wrote:
[...]
> --- 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.
> +
> +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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
