lock_ref_for_update(): make error handling more uniform

diff --git a/refs.c b/refs.c
index 87dc82f1d87d6c0969b97f94035233bec0bc3729..842c5c7b0543ea21da3b46e9ad588cf587cb91e6 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -120,25 +120,33 @@ int check_refname_format(const char *refname, int flags)
 
 int refname_is_safe(const char *refname)
 {
-       if (starts_with(refname, "refs/")) {
+       const char *rest;
+
+       if (skip_prefix(refname, "refs/", &rest)) {
                char *buf;
                int result;
+               size_t restlen = strlen(rest);
+
+               /* rest must not be empty, or start or end with "/" */
+               if (!restlen || *rest == '/' || rest[restlen - 1] == '/')
+                       return 0;
 
-               buf = xmallocz(strlen(refname));
                /*
                 * Does the refname try to escape refs/?
                 * For example: refs/foo/../bar is safe but refs/foo/../../bar
                 * is not.
                 */
-               result = !normalize_path_copy(buf, refname + strlen("refs/"));
+               buf = xmallocz(restlen);
+               result = !normalize_path_copy(buf, rest) && !strcmp(buf, rest);
                free(buf);
                return result;
        }
-       while (*refname) {
+
+       do {
                if (!isupper(*refname) && *refname != '_')
                        return 0;
                refname++;
-       }
+       } while (*refname);
        return 1;
 }
 
@@ -496,7 +504,7 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
        filename = git_path("%s", pseudoref);
        fd = hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR);
        if (fd < 0) {
-               strbuf_addf(err, "Could not open '%s' for writing: %s",
+               strbuf_addf(err, "could not open '%s' for writing: %s",
                            filename, strerror(errno));
                return -1;
        }
@@ -507,14 +515,14 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
                if (read_ref(pseudoref, actual_old_sha1))
                        die("could not read ref '%s'", pseudoref);
                if (hashcmp(actual_old_sha1, old_sha1)) {
-                       strbuf_addf(err, "Unexpected sha1 when writing %s", pseudoref);
+                       strbuf_addf(err, "unexpected sha1 when writing '%s'", pseudoref);
                        rollback_lock_file(&lock);
                        goto done;
                }
        }
 
        if (write_in_full(fd, buf.buf, buf.len) != buf.len) {
-               strbuf_addf(err, "Could not write to '%s'", filename);
+               strbuf_addf(err, "could not write to '%s'", filename);
                rollback_lock_file(&lock);
                goto done;
        }
@@ -758,13 +766,33 @@ void ref_transaction_free(struct ref_transaction *transaction)
        free(transaction);
 }
 
-static struct ref_update *add_update(struct ref_transaction *transaction,
-                                    const char *refname)
+struct ref_update *ref_transaction_add_update(
+               struct ref_transaction *transaction,
+               const char *refname, unsigned int flags,
+               const unsigned char *new_sha1,
+               const unsigned char *old_sha1,
+               const char *msg)
 {
        struct ref_update *update;
+
+       if (transaction->state != REF_TRANSACTION_OPEN)
+               die("BUG: update called for transaction that is not open");
+
+       if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
+               die("BUG: REF_ISPRUNING set without REF_NODEREF");
+
        FLEX_ALLOC_STR(update, refname, refname);
        ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
        transaction->updates[transaction->nr++] = update;
+
+       update->flags = flags;
+
+       if (flags & REF_HAVE_NEW)
+               hashcpy(update->new_sha1, new_sha1);
+       if (flags & REF_HAVE_OLD)
+               hashcpy(update->old_sha1, old_sha1);
+       if (msg)
+               update->msg = xstrdup(msg);
        return update;
 }
 
@@ -775,32 +803,20 @@ int ref_transaction_update(struct ref_transaction *transaction,
                           unsigned int flags, const char *msg,
                           struct strbuf *err)
 {
-       struct ref_update *update;
-
        assert(err);
 
-       if (transaction->state != REF_TRANSACTION_OPEN)
-               die("BUG: update called for transaction that is not open");
-
-       if (new_sha1 && !is_null_sha1(new_sha1) &&
-           check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-               strbuf_addf(err, "refusing to update ref with bad name %s",
+       if ((new_sha1 && !is_null_sha1(new_sha1)) ?
+           check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
+           !refname_is_safe(refname)) {
+               strbuf_addf(err, "refusing to update ref with bad name '%s'",
                            refname);
                return -1;
        }
 
-       update = add_update(transaction, refname);
-       if (new_sha1) {
-               hashcpy(update->new_sha1, new_sha1);
-               flags |= REF_HAVE_NEW;
-       }
-       if (old_sha1) {
-               hashcpy(update->old_sha1, old_sha1);
-               flags |= REF_HAVE_OLD;
-       }
-       update->flags = flags;
-       if (msg)
-               update->msg = xstrdup(msg);
+       flags |= (new_sha1 ? REF_HAVE_NEW : 0) | (old_sha1 ? REF_HAVE_OLD : 0);
+
+       ref_transaction_add_update(transaction, refname, flags,
+                                  new_sha1, old_sha1, msg);
        return 0;
 }