Re: [PATCH v20 40/48] refs.c: add an err argument to delete_ref_loose
From: Michael Haggerty <hidden>
Date: 2016-06-15 23:01:50
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.
quoted hunk ↗ jump to hunk
Signed-off-by: Ronnie Sahlberg <redacted> --- 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.
quoted hunk ↗ jump to hunk
+{ + 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