From 026bd1d3e27a9e9b8f6b0098644c5753a2a1e355 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:42 -0700 Subject: refs.c: remove ref_transaction_rollback We do not yet need both a rollback and a free function for transactions. Remove ref_transaction_rollback and use ref_transaction_free instead. At a later stage we may reintroduce a rollback function if we want to start adding reusable transactions and similar. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index dc457742ea..6d841a0645 100644 --- a/refs.c +++ b/refs.c @@ -3334,7 +3334,7 @@ struct ref_transaction *ref_transaction_begin(void) return xcalloc(1, sizeof(struct ref_transaction)); } -static void ref_transaction_free(struct ref_transaction *transaction) +void ref_transaction_free(struct ref_transaction *transaction) { int i; @@ -3345,11 +3345,6 @@ static void ref_transaction_free(struct ref_transaction *transaction) free(transaction); } -void ref_transaction_rollback(struct ref_transaction *transaction) -{ - ref_transaction_free(transaction); -} - static struct ref_update *add_update(struct ref_transaction *transaction, const char *refname) { -- cgit v1.2.1 From 33f9fc593247322ddea5515f548a06cf34d12c2f Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:43 -0700 Subject: refs.c: ref_transaction_commit should not free the transaction Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 1 - 1 file changed, 1 deletion(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 6d841a0645..d9cac6db18 100644 --- a/refs.c +++ b/refs.c @@ -3509,7 +3509,6 @@ cleanup: if (updates[i]->lock) unlock_ref(updates[i]->lock); free(delnames); - ref_transaction_free(transaction); return ret; } -- cgit v1.2.1 From f1c9350ad784eca97184b73973fad353baa99a1d Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:44 -0700 Subject: refs.c: constify the sha arguments for ref_transaction_create|delete|update ref_transaction_create|delete|update has no need to modify the sha1 arguments passed to it so it should use const unsigned char* instead of unsigned char*. Some functions, such as fast_forward_to(), already have its old/new sha1 arguments as consts. This function will at some point need to use ref_transaction_update() in which case this change is required. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index d9cac6db18..21ed46534a 100644 --- a/refs.c +++ b/refs.c @@ -3359,7 +3359,8 @@ static struct ref_update *add_update(struct ref_transaction *transaction, void ref_transaction_update(struct ref_transaction *transaction, const char *refname, - unsigned char *new_sha1, unsigned char *old_sha1, + const unsigned char *new_sha1, + const unsigned char *old_sha1, int flags, int have_old) { struct ref_update *update = add_update(transaction, refname); @@ -3373,7 +3374,7 @@ void ref_transaction_update(struct ref_transaction *transaction, void ref_transaction_create(struct ref_transaction *transaction, const char *refname, - unsigned char *new_sha1, + const unsigned char *new_sha1, int flags) { struct ref_update *update = add_update(transaction, refname); @@ -3387,7 +3388,7 @@ void ref_transaction_create(struct ref_transaction *transaction, void ref_transaction_delete(struct ref_transaction *transaction, const char *refname, - unsigned char *old_sha1, + const unsigned char *old_sha1, int flags, int have_old) { struct ref_update *update = add_update(transaction, refname); -- cgit v1.2.1 From 1b07255c95cdf2f7dbe7989a734248f60799f506 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:45 -0700 Subject: refs.c: allow passing NULL to ref_transaction_free Allow ref_transaction_free(NULL) as a no-op. This makes ref_transaction_free easier to use and more similar to plain 'free'. In particular, it lets us rollback unconditionally as part of cleanup code after setting 'transaction = NULL' if a transaction has been committed or rolled back already. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 21ed46534a..1d6dece426 100644 --- a/refs.c +++ b/refs.c @@ -3338,6 +3338,9 @@ void ref_transaction_free(struct ref_transaction *transaction) { int i; + if (!transaction) + return; + for (i = 0; i < transaction->nr; i++) free(transaction->updates[i]); -- cgit v1.2.1 From 995f8746bc421a537e12622770a90be2205cd26f Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:46 -0700 Subject: refs.c: add a strbuf argument to ref_transaction_commit for error logging Add a strbuf argument to _commit so that we can pass an error string back to the caller. So that we can do error logging from the caller instead of from _commit. Longer term plan is to first convert all callers to use onerr==QUIET_ON_ERR and craft any log messages from the callers themselves and finally remove the onerr argument completely. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 1d6dece426..db05602590 100644 --- a/refs.c +++ b/refs.c @@ -3444,7 +3444,8 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n, } int ref_transaction_commit(struct ref_transaction *transaction, - const char *msg, enum action_on_err onerr) + const char *msg, struct strbuf *err, + enum action_on_err onerr) { int ret = 0, delnum = 0, i; const char **delnames; @@ -3473,6 +3474,9 @@ int ref_transaction_commit(struct ref_transaction *transaction, update->flags, &update->type, onerr); if (!update->lock) { + if (err) + strbuf_addf(err, "Cannot lock the ref '%s'.", + update->refname); ret = 1; goto cleanup; } -- cgit v1.2.1 From 447ff1bf0acf9a1d7d2dc3aed032c209f105fb8a Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:48 -0700 Subject: lockfile.c: make lock_file return a meaningful errno on failurei Making errno when returning from lock_file() meaningful, which should fix * an existing almost-bug in lock_ref_sha1_basic where it assumes errno==ENOENT is meaningful and could waste some work on retries * an existing bug in repack_without_refs where it prints strerror(errno) and picks advice based on errno, despite errno potentially being zero and potentially having been clobbered by that point Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 1 + 1 file changed, 1 insertion(+) (limited to 'refs.c') diff --git a/refs.c b/refs.c index db05602590..e9d53e4a57 100644 --- a/refs.c +++ b/refs.c @@ -2212,6 +2212,7 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data) return 0; } +/* This should return a meaningful errno on failure */ int lock_packed_refs(int flags) { struct packed_ref_cache *packed_ref_cache; -- cgit v1.2.1 From 60bca085c893f1866829b55bfee63888943cfe14 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:49 -0700 Subject: refs.c: add an err argument to repack_without_refs Update repack_without_refs to take an err argument and update it if there is a failure. Pass the err variable from ref_transaction_commit to this function so that callers can print a meaningful error message if _commit fails due to this function. Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index e9d53e4a57..67a0217a8d 100644 --- a/refs.c +++ b/refs.c @@ -2456,12 +2456,12 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data) return 0; } -int repack_without_refs(const char **refnames, int n) +int repack_without_refs(const char **refnames, int n, struct strbuf *err) { struct ref_dir *packed; struct string_list refs_to_delete = STRING_LIST_INIT_DUP; struct string_list_item *ref_to_delete; - int i, removed = 0; + int i, ret, removed = 0; /* Look for a packed ref */ for (i = 0; i < n; i++) @@ -2473,6 +2473,11 @@ int repack_without_refs(const char **refnames, int n) return 0; /* no refname exists in packed refs */ if (lock_packed_refs(0)) { + if (err) { + unable_to_lock_message(git_path("packed-refs"), errno, + err); + return -1; + } unable_to_lock_error(git_path("packed-refs"), errno); return error("cannot delete '%s' from packed refs", refnames[i]); } @@ -2499,12 +2504,16 @@ int repack_without_refs(const char **refnames, int n) } /* Write what remains */ - return commit_packed_refs(); + ret = commit_packed_refs(); + if (ret && err) + strbuf_addf(err, "unable to overwrite old ref-pack file: %s", + strerror(errno)); + return ret; } static int repack_without_ref(const char *refname) { - return repack_without_refs(&refname, 1); + return repack_without_refs(&refname, 1, NULL); } static int delete_ref_loose(struct ref_lock *lock, int flag) @@ -3508,7 +3517,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, } } - ret |= repack_without_refs(delnames, delnum); + ret |= repack_without_refs(delnames, delnum, err); for (i = 0; i < delnum; i++) unlink_or_warn(git_path("logs/%s", delnames[i])); clear_loose_ref_cache(&ref_cache); -- cgit v1.2.1 From bd3b02daec241651231f63b3fd8ee2a8ea1dac68 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:50 -0700 Subject: refs.c: make sure log_ref_setup returns a meaningful errno Making errno when returning from log_ref_setup() meaningful, Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 67a0217a8d..9ea519c3fd 100644 --- a/refs.c +++ b/refs.c @@ -2751,6 +2751,7 @@ static int copy_msg(char *buf, const char *msg) return cp - buf; } +/* This function must set a meaningful errno on failure */ int log_ref_setup(const char *refname, char *logfile, int bufsize) { int logfd, oflags = O_APPEND | O_WRONLY; @@ -2761,9 +2762,12 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize) starts_with(refname, "refs/remotes/") || starts_with(refname, "refs/notes/") || !strcmp(refname, "HEAD"))) { - if (safe_create_leading_directories(logfile) < 0) - return error("unable to create directory for %s", - logfile); + if (safe_create_leading_directories(logfile) < 0) { + int save_errno = errno; + error("unable to create directory for %s", logfile); + errno = save_errno; + return -1; + } oflags |= O_CREAT; } @@ -2774,15 +2778,22 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize) if ((oflags & O_CREAT) && errno == EISDIR) { if (remove_empty_directories(logfile)) { - return error("There are still logs under '%s'", - logfile); + int save_errno = errno; + error("There are still logs under '%s'", + logfile); + errno = save_errno; + return -1; } logfd = open(logfile, oflags, 0666); } - if (logfd < 0) - return error("Unable to append to %s: %s", - logfile, strerror(errno)); + if (logfd < 0) { + int save_errno = errno; + error("Unable to append to %s: %s", logfile, + strerror(errno)); + errno = save_errno; + return -1; + } } adjust_shared_perm(logfile); -- cgit v1.2.1 From 835e3c992fd453d172466be29283b55a792cac76 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:51 -0700 Subject: refs.c: verify_lock should set errno to something meaningful Making errno when returning from verify_lock() meaningful, which should almost but not completely fix * a bug in "git fetch"'s s_update_ref, which trusts the result of an errno == ENOTDIR check to detect D/F conflicts ENOTDIR makes sense as a sign that a file was in the way of a directory we wanted to create. Should "git fetch" also look for ENOTEMPTY or EEXIST to catch cases where a directory was in the way of a file to be created? Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 9ea519c3fd..a48f805128 100644 --- a/refs.c +++ b/refs.c @@ -1932,18 +1932,22 @@ int refname_match(const char *abbrev_name, const char *full_name) return 0; } +/* This function should make sure errno is meaningful on error */ static struct ref_lock *verify_lock(struct ref_lock *lock, const unsigned char *old_sha1, int mustexist) { if (read_ref_full(lock->ref_name, lock->old_sha1, mustexist, NULL)) { + int save_errno = errno; error("Can't verify ref %s", lock->ref_name); unlock_ref(lock); + errno = save_errno; return NULL; } if (hashcmp(lock->old_sha1, old_sha1)) { error("Ref %s is at %s but expected %s", lock->ref_name, sha1_to_hex(lock->old_sha1), sha1_to_hex(old_sha1)); unlock_ref(lock); + errno = EBUSY; return NULL; } return lock; -- cgit v1.2.1 From 470a91ef75d61d102c9cb655a7b8ea1555337d2d Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:52 -0700 Subject: refs.c: make remove_empty_directories always set errno to something sane Making errno when returning from remove_empty_directories() more obviously meaningful, which should provide some peace of mind for people auditing lock_ref_sha1_basic. Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index a48f805128..cc6958141c 100644 --- a/refs.c +++ b/refs.c @@ -1960,14 +1960,16 @@ static int remove_empty_directories(const char *file) * only empty directories), remove them. */ struct strbuf path; - int result; + int result, save_errno; strbuf_init(&path, 20); strbuf_addstr(&path, file); result = remove_dir_recursively(&path, REMOVE_DIR_EMPTY_ONLY); + save_errno = errno; strbuf_release(&path); + errno = save_errno; return result; } @@ -2056,6 +2058,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log) return logs_found; } +/* This function should make sure errno is meaningful on error */ static struct ref_lock *lock_ref_sha1_basic(const char *refname, const unsigned char *old_sha1, int flags, int *type_p) -- cgit v1.2.1 From d3f665550588d19d6d6c9f9064aa3d4685afdf4d Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:53 -0700 Subject: refs.c: commit_packed_refs to return a meaningful errno on failure Making errno when returning from commit_packed_refs() meaningful, which should fix * a bug in "git clone" where it prints strerror(errno) based on errno, despite errno possibly being zero and potentially having been clobbered by that point * the same kind of bug in "git pack-refs" and prepares for repack_without_refs() to get a meaningful error message when commit_packed_refs() fails without falling into the same bug. Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index cc6958141c..7a815be38e 100644 --- a/refs.c +++ b/refs.c @@ -2239,11 +2239,16 @@ int lock_packed_refs(int flags) return 0; } +/* + * Commit the packed refs changes. + * On error we must make sure that errno contains a meaningful value. + */ int commit_packed_refs(void) { struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(&ref_cache); int error = 0; + int save_errno = 0; if (!packed_ref_cache->lock) die("internal error: packed-refs not locked"); @@ -2253,10 +2258,13 @@ int commit_packed_refs(void) do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache), 0, write_packed_entry_fn, &packed_ref_cache->lock->fd); - if (commit_lock_file(packed_ref_cache->lock)) + if (commit_lock_file(packed_ref_cache->lock)) { + save_errno = errno; error = -1; + } packed_ref_cache->lock = NULL; release_packed_ref_cache(packed_ref_cache); + errno = save_errno; return error; } -- cgit v1.2.1 From 76d70dc0c63fe89958308f249136864aec5cdf03 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:54 -0700 Subject: refs.c: make resolve_ref_unsafe set errno to something meaningful on error Making errno when returning from resolve_ref_unsafe() meaningful, which should fix * a bug in lock_ref_sha1_basic, where it assumes EISDIR means it failed due to a directory being in the way Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 7a815be38e..211429df06 100644 --- a/refs.c +++ b/refs.c @@ -1334,6 +1334,7 @@ static const char *handle_missing_loose_ref(const char *refname, } } +/* This function needs to return a meaningful errno on failure */ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag) { int depth = MAXDEPTH; @@ -1344,8 +1345,10 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea if (flag) *flag = 0; - if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) + if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { + errno = EINVAL; return NULL; + } for (;;) { char path[PATH_MAX]; @@ -1353,8 +1356,10 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea char *buf; int fd; - if (--depth < 0) + if (--depth < 0) { + errno = ELOOP; return NULL; + } git_snpath(path, sizeof(path), "%s", refname); @@ -1416,9 +1421,13 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea return NULL; } len = read_in_full(fd, buffer, sizeof(buffer)-1); - close(fd); - if (len < 0) + if (len < 0) { + int save_errno = errno; + close(fd); + errno = save_errno; return NULL; + } + close(fd); while (len && isspace(buffer[len-1])) len--; buffer[len] = '\0'; @@ -1435,6 +1444,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea (buffer[40] != '\0' && !isspace(buffer[40]))) { if (flag) *flag |= REF_ISBROKEN; + errno = EINVAL; return NULL; } return refname; @@ -1447,6 +1457,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) { if (flag) *flag |= REF_ISBROKEN; + errno = EINVAL; return NULL; } refname = strcpy(refname_buffer, buf); -- cgit v1.2.1 From dc615de8610c807633c63313937056bf45df0b8f Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:55 -0700 Subject: refs.c: log_ref_write should try to return meaningful errno MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Making errno from write_ref_sha1() meaningful, which should fix * a bug in "git checkout -b" where it prints strerror(errno)  despite errno possibly being zero or clobbered * a bug in "git fetch"'s s_update_ref, which trusts the result of an  errno == ENOTDIR check to detect D/F conflicts Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 211429df06..61570c9036 100644 --- a/refs.c +++ b/refs.c @@ -2859,8 +2859,19 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1, len += copy_msg(logrec + len - 1, msg) - 1; written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1; free(logrec); - if (close(logfd) != 0 || written != len) - return error("Unable to append to %s", log_file); + if (written != len) { + int save_errno = errno; + close(logfd); + error("Unable to append to %s", log_file); + errno = save_errno; + return -1; + } + if (close(logfd)) { + int save_errno = errno; + error("Unable to append to %s", log_file); + errno = save_errno; + return -1; + } return 0; } @@ -2869,14 +2880,17 @@ static int is_branch(const char *refname) return !strcmp(refname, "HEAD") || starts_with(refname, "refs/heads/"); } +/* This function must return a meaningful errno */ int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *logmsg) { static char term = '\n'; struct object *o; - if (!lock) + if (!lock) { + errno = EINVAL; return -1; + } if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) { unlock_ref(lock); return 0; @@ -2886,19 +2900,23 @@ int write_ref_sha1(struct ref_lock *lock, error("Trying to write ref %s with nonexistent object %s", lock->ref_name, sha1_to_hex(sha1)); unlock_ref(lock); + errno = EINVAL; return -1; } if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) { error("Trying to write non-commit object %s to branch %s", sha1_to_hex(sha1), lock->ref_name); unlock_ref(lock); + errno = EINVAL; return -1; } if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 || - write_in_full(lock->lock_fd, &term, 1) != 1 - || close_ref(lock) < 0) { + write_in_full(lock->lock_fd, &term, 1) != 1 || + close_ref(lock) < 0) { + int save_errno = errno; error("Couldn't write %s", lock->lk->filename); unlock_ref(lock); + errno = save_errno; return -1; } clear_loose_ref_cache(&ref_cache); -- cgit v1.2.1 From 038d005129467a514189e8cc1321022f71f6a51e Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:56 -0700 Subject: refs.c: make ref_update_reject_duplicates take a strbuf argument for errors Make ref_update_reject_duplicates return any error that occurs through a new strbuf argument. This means that when a transaction commit fails in this function we will now be able to pass a helpful error message back to the caller. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 61570c9036..115f1431f7 100644 --- a/refs.c +++ b/refs.c @@ -3488,6 +3488,7 @@ static int ref_update_compare(const void *r1, const void *r2) } static int ref_update_reject_duplicates(struct ref_update **updates, int n, + struct strbuf *err, enum action_on_err onerr) { int i; @@ -3495,6 +3496,9 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n, if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) { const char *str = "Multiple updates for ref '%s' not allowed."; + if (err) + strbuf_addf(err, str, updates[i]->refname); + switch (onerr) { case UPDATE_REFS_MSG_ON_ERR: error(str, updates[i]->refname); break; @@ -3525,7 +3529,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, /* Copy, sort, and reject duplicate refs */ qsort(updates, n, sizeof(*updates), ref_update_compare); - ret = ref_update_reject_duplicates(updates, n, onerr); + ret = ref_update_reject_duplicates(updates, n, err, onerr); if (ret) goto cleanup; -- cgit v1.2.1 From c1703d7634a61b4d08d77fa0d9ca7e7ad92c3cb8 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:57 -0700 Subject: refs.c: make update_ref_write update a strbuf on failure Change update_ref_write to also update an error strbuf on failure. This makes the error available to ref_transaction_commit callers if the transaction failed due to update_ref_sha1/write_ref_sha1 failures. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 115f1431f7..003b313ade 100644 --- a/refs.c +++ b/refs.c @@ -3353,10 +3353,13 @@ static struct ref_lock *update_ref_lock(const char *refname, static int update_ref_write(const char *action, const char *refname, const unsigned char *sha1, struct ref_lock *lock, - enum action_on_err onerr) + struct strbuf *err, enum action_on_err onerr) { if (write_ref_sha1(lock, sha1, action) < 0) { const char *str = "Cannot update the ref '%s'."; + if (err) + strbuf_addf(err, str, refname); + switch (onerr) { case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break; case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break; @@ -3477,7 +3480,7 @@ int update_ref(const char *action, const char *refname, lock = update_ref_lock(refname, oldval, flags, NULL, onerr); if (!lock) return 1; - return update_ref_write(action, refname, sha1, lock, onerr); + return update_ref_write(action, refname, sha1, lock, NULL, onerr); } static int ref_update_compare(const void *r1, const void *r2) @@ -3559,7 +3562,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, ret = update_ref_write(msg, update->refname, update->new_sha1, - update->lock, onerr); + update->lock, err, onerr); update->lock = NULL; /* freed by update_ref_write */ if (ret) goto cleanup; -- cgit v1.2.1 From 01319837c53050109c60e6740dfa9462327161f0 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:42:59 -0700 Subject: refs.c: remove the onerr argument to ref_transaction_commit Since all callers now use QUIET_ON_ERR we no longer need to provide an onerr argument any more. Remove the onerr argument from the ref_transaction_commit signature. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 003b313ade..4f78bd9250 100644 --- a/refs.c +++ b/refs.c @@ -3491,8 +3491,7 @@ static int ref_update_compare(const void *r1, const void *r2) } static int ref_update_reject_duplicates(struct ref_update **updates, int n, - struct strbuf *err, - enum action_on_err onerr) + struct strbuf *err) { int i; for (i = 1; i < n; i++) @@ -3502,22 +3501,13 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n, if (err) strbuf_addf(err, str, updates[i]->refname); - switch (onerr) { - case UPDATE_REFS_MSG_ON_ERR: - error(str, updates[i]->refname); break; - case UPDATE_REFS_DIE_ON_ERR: - die(str, updates[i]->refname); break; - case UPDATE_REFS_QUIET_ON_ERR: - break; - } return 1; } return 0; } int ref_transaction_commit(struct ref_transaction *transaction, - const char *msg, struct strbuf *err, - enum action_on_err onerr) + const char *msg, struct strbuf *err) { int ret = 0, delnum = 0, i; const char **delnames; @@ -3532,7 +3522,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, /* Copy, sort, and reject duplicate refs */ qsort(updates, n, sizeof(*updates), ref_update_compare); - ret = ref_update_reject_duplicates(updates, n, err, onerr); + ret = ref_update_reject_duplicates(updates, n, err); if (ret) goto cleanup; @@ -3544,7 +3534,8 @@ int ref_transaction_commit(struct ref_transaction *transaction, (update->have_old ? update->old_sha1 : NULL), update->flags, - &update->type, onerr); + &update->type, + UPDATE_REFS_QUIET_ON_ERR); if (!update->lock) { if (err) strbuf_addf(err, "Cannot lock the ref '%s'.", @@ -3562,7 +3553,8 @@ int ref_transaction_commit(struct ref_transaction *transaction, ret = update_ref_write(msg, update->refname, update->new_sha1, - update->lock, err, onerr); + update->lock, err, + UPDATE_REFS_QUIET_ON_ERR); update->lock = NULL; /* freed by update_ref_write */ if (ret) goto cleanup; -- cgit v1.2.1 From 8e34800e5bce1dc6f1ab5520a4c866a2a73639de Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Fri, 20 Jun 2014 07:43:00 -0700 Subject: refs.c: change ref_transaction_update() to do error checking and return status Update ref_transaction_update() do some basic error checking and return non-zero on error. Update all callers to check ref_transaction_update() for error. There are currently no conditions in _update that will return error but there will be in the future. Add an err argument that will be updated on failure. In future patches we will start doing both locking and checking for name conflicts in _update instead of _commit at which time this function will start returning errors for these conditions. Also check for BUGs during update and die(BUG:...) if we are calling _update with have_old but the old_sha1 pointer is NULL. Reviewed-by: Jonathan Nieder Signed-off-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano Acked-by: Michael Haggerty --- refs.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 4f78bd9250..3f05e88329 100644 --- a/refs.c +++ b/refs.c @@ -3428,19 +3428,25 @@ static struct ref_update *add_update(struct ref_transaction *transaction, return update; } -void ref_transaction_update(struct ref_transaction *transaction, - const char *refname, - const unsigned char *new_sha1, - const unsigned char *old_sha1, - int flags, int have_old) +int ref_transaction_update(struct ref_transaction *transaction, + const char *refname, + const unsigned char *new_sha1, + const unsigned char *old_sha1, + int flags, int have_old, + struct strbuf *err) { - struct ref_update *update = add_update(transaction, refname); + struct ref_update *update; + + if (have_old && !old_sha1) + die("BUG: have_old is true but old_sha1 is NULL"); + update = add_update(transaction, refname); hashcpy(update->new_sha1, new_sha1); update->flags = flags; update->have_old = have_old; if (have_old) hashcpy(update->old_sha1, old_sha1); + return 0; } void ref_transaction_create(struct ref_transaction *transaction, -- cgit v1.2.1