Re: [PATCH v11 08/41] refs.c: add an err argument to delete_ref_loose
From: Jonathan Nieder <hidden>
Date: 2016-06-15 23:01:24
Hi, Comments from http://marc.info/?l=git&m=140079653930751&w=2: Ronnie Sahlberg wrote: [...]
quoted hunk ↗ jump to hunk
--- 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