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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html