From b416af5bcdca3ac8426220f09efbfca2f1bec6e0 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Wed, 16 Apr 2014 15:26:44 -0700 Subject: refs.c: change ref_transaction_create to do error checking and return status Do basic error checking in ref_transaction_create() and make it return non-zero on error. Update all callers to check the result of ref_transaction_create(). There are currently no conditions in _create that will return error but there will be in the future. Add an err argument that will be updated on failure. Signed-off-by: Ronnie Sahlberg Reviewed-by: Michael Haggerty Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- refs.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 3f05e88329..c49f1c609a 100644 --- a/refs.c +++ b/refs.c @@ -3449,18 +3449,24 @@ int ref_transaction_update(struct ref_transaction *transaction, return 0; } -void ref_transaction_create(struct ref_transaction *transaction, - const char *refname, - const unsigned char *new_sha1, - int flags) +int ref_transaction_create(struct ref_transaction *transaction, + const char *refname, + const unsigned char *new_sha1, + int flags, + struct strbuf *err) { - struct ref_update *update = add_update(transaction, refname); + struct ref_update *update; + + if (!new_sha1 || is_null_sha1(new_sha1)) + die("BUG: create ref with null new_sha1"); + + update = add_update(transaction, refname); - assert(!is_null_sha1(new_sha1)); hashcpy(update->new_sha1, new_sha1); hashclr(update->old_sha1); update->flags = flags; update->have_old = 1; + return 0; } void ref_transaction_delete(struct ref_transaction *transaction, -- cgit v1.2.1 From 8c8bdc0d3582e42c13815a191344b1aa3e06c792 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Wed, 16 Apr 2014 15:27:45 -0700 Subject: refs.c: update ref_transaction_delete to check for error and return status Change ref_transaction_delete() to do basic error checking and return non-zero on error. Update all callers to check the return for ref_transaction_delete(). There are currently no conditions in _delete that will return error but there will be in the future. Add an err argument that will be updated on failure. Signed-off-by: Ronnie Sahlberg Reviewed-by: Michael Haggerty Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- refs.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index c49f1c609a..40f04f4a6f 100644 --- a/refs.c +++ b/refs.c @@ -3469,19 +3469,25 @@ int ref_transaction_create(struct ref_transaction *transaction, return 0; } -void ref_transaction_delete(struct ref_transaction *transaction, - const char *refname, - const unsigned char *old_sha1, - int flags, int have_old) +int ref_transaction_delete(struct ref_transaction *transaction, + const char *refname, + 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); update->flags = flags; update->have_old = have_old; if (have_old) { assert(!is_null_sha1(old_sha1)); hashcpy(update->old_sha1, old_sha1); } + return 0; } int update_ref(const char *action, const char *refname, -- cgit v1.2.1 From 93a644ea9d3702cc1cc62c0d413f81f8e46fabe7 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Mon, 19 May 2014 10:42:34 -0700 Subject: refs.c: make ref_transaction_begin take an err argument Add an err argument to _begin so that on non-fatal failures in future ref backends we can report a nice error back to the caller. While _begin can currently never fail for other reasons than OOM, in which case we die() anyway, we may add other types of backends in the future. For example, a hypothetical MySQL backend could fail in _begin with "Can not connect to MySQL server. No route to host". Signed-off-by: Ronnie Sahlberg Reviewed-by: Michael Haggerty Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- refs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 40f04f4a6f..9cb79081b5 100644 --- a/refs.c +++ b/refs.c @@ -3397,7 +3397,7 @@ struct ref_transaction { size_t nr; }; -struct ref_transaction *ref_transaction_begin(void) +struct ref_transaction *ref_transaction_begin(struct strbuf *err) { return xcalloc(1, sizeof(struct ref_transaction)); } -- cgit v1.2.1 From 2bdc785fd7f699669a582f7a83f5b81192d24d88 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Tue, 29 Apr 2014 12:06:19 -0700 Subject: refs.c: add transaction.status and track OPEN/CLOSED Track the state of a transaction in a new state field. Check the field for sanity, i.e. that state must be OPEN when _commit/_create/_delete or _update is called or else die(BUG:...) Signed-off-by: Ronnie Sahlberg Reviewed-by: Michael Haggerty Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- refs.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 9cb79081b5..cc6305630c 100644 --- a/refs.c +++ b/refs.c @@ -3386,6 +3386,21 @@ struct ref_update { const char refname[FLEX_ARRAY]; }; +/* + * Transaction states. + * OPEN: The transaction is in a valid state and can accept new updates. + * An OPEN transaction can be committed. + * CLOSED: A closed transaction is no longer active and no other operations + * than free can be used on it in this state. + * A transaction can either become closed by successfully committing + * an active transaction or if there is a failure while building + * the transaction thus rendering it failed/inactive. + */ +enum ref_transaction_state { + REF_TRANSACTION_OPEN = 0, + REF_TRANSACTION_CLOSED = 1 +}; + /* * Data structure for holding a reference transaction, which can * consist of checks and updates to multiple references, carried out @@ -3395,6 +3410,7 @@ struct ref_transaction { struct ref_update **updates; size_t alloc; size_t nr; + enum ref_transaction_state state; }; struct ref_transaction *ref_transaction_begin(struct strbuf *err) @@ -3437,6 +3453,9 @@ int ref_transaction_update(struct ref_transaction *transaction, { struct ref_update *update; + if (transaction->state != REF_TRANSACTION_OPEN) + die("BUG: update called for transaction that is not open"); + if (have_old && !old_sha1) die("BUG: have_old is true but old_sha1 is NULL"); @@ -3457,6 +3476,9 @@ int ref_transaction_create(struct ref_transaction *transaction, { struct ref_update *update; + if (transaction->state != REF_TRANSACTION_OPEN) + die("BUG: create called for transaction that is not open"); + if (!new_sha1 || is_null_sha1(new_sha1)) die("BUG: create ref with null new_sha1"); @@ -3477,6 +3499,9 @@ int ref_transaction_delete(struct ref_transaction *transaction, { struct ref_update *update; + if (transaction->state != REF_TRANSACTION_OPEN) + die("BUG: delete called for transaction that is not open"); + if (have_old && !old_sha1) die("BUG: have_old is true but old_sha1 is NULL"); @@ -3532,8 +3557,13 @@ int ref_transaction_commit(struct ref_transaction *transaction, int n = transaction->nr; struct ref_update **updates = transaction->updates; - if (!n) + if (transaction->state != REF_TRANSACTION_OPEN) + die("BUG: commit called for transaction that is not open"); + + if (!n) { + transaction->state = REF_TRANSACTION_CLOSED; return 0; + } /* Allocate work space */ delnames = xmalloc(sizeof(*delnames) * n); @@ -3595,6 +3625,8 @@ int ref_transaction_commit(struct ref_transaction *transaction, clear_loose_ref_cache(&ref_cache); cleanup: + transaction->state = REF_TRANSACTION_CLOSED; + for (i = 0; i < n; i++) if (updates[i]->lock) unlock_ref(updates[i]->lock); -- cgit v1.2.1 From b4d75ac1d152bbab44b0777a4cc0c48db75f6024 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Thu, 24 Apr 2014 16:36:55 -0700 Subject: refs.c: change update_ref to use a transaction Change the update_ref helper function to use a ref transaction internally. Signed-off-by: Ronnie Sahlberg Reviewed-by: Michael Haggerty Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- refs.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index cc6305630c..a3532ab186 100644 --- a/refs.c +++ b/refs.c @@ -3519,11 +3519,33 @@ int update_ref(const char *action, const char *refname, const unsigned char *sha1, const unsigned char *oldval, int flags, enum action_on_err onerr) { - struct ref_lock *lock; - lock = update_ref_lock(refname, oldval, flags, NULL, onerr); - if (!lock) + struct ref_transaction *t; + struct strbuf err = STRBUF_INIT; + + t = ref_transaction_begin(&err); + if (!t || + ref_transaction_update(t, refname, sha1, oldval, flags, + !!oldval, &err) || + ref_transaction_commit(t, action, &err)) { + const char *str = "update_ref failed for ref '%s': %s"; + + ref_transaction_free(t); + switch (onerr) { + case UPDATE_REFS_MSG_ON_ERR: + error(str, refname, err.buf); + break; + case UPDATE_REFS_DIE_ON_ERR: + die(str, refname, err.buf); + break; + case UPDATE_REFS_QUIET_ON_ERR: + break; + } + strbuf_release(&err); return 1; - return update_ref_write(action, refname, sha1, lock, NULL, onerr); + } + strbuf_release(&err); + ref_transaction_free(t); + return 0; } static int ref_update_compare(const void *r1, const void *r2) -- cgit v1.2.1 From 88b680ae8d7a8f88409eff0c4cd2f12acf07155f Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Mon, 28 Apr 2014 15:38:47 -0700 Subject: refs.c: make lock_ref_sha1 static No external callers reference lock_ref_sha1 any more so let's declare it static. Signed-off-by: Ronnie Sahlberg Reviewed-by: Michael Haggerty Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- refs.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index a3532ab186..d4cd44b05c 100644 --- a/refs.c +++ b/refs.c @@ -2069,7 +2069,10 @@ 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 */ +/* + * Locks a "refs/" ref returning the lock on success and NULL on failure. + * On failure errno is set to something meaningful. + */ static struct ref_lock *lock_ref_sha1_basic(const char *refname, const unsigned char *old_sha1, int flags, int *type_p) @@ -2170,7 +2173,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, return NULL; } -struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1) +static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1) { char refpath[PATH_MAX]; if (check_refname_format(refname, 0)) -- cgit v1.2.1 From 45421e24e8322184a35a32a9072f04afabe33880 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Tue, 29 Apr 2014 12:14:47 -0700 Subject: refs.c: remove the update_ref_lock function Since we now only call update_ref_lock with onerr==QUIET_ON_ERR we no longer need this function and can replace it with just calling lock_any_ref_for_update directly. Signed-off-by: Ronnie Sahlberg Reviewed-by: Michael Haggerty Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- refs.c | 30 ++++++------------------------ 1 file changed, 6 insertions(+), 24 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index d4cd44b05c..a4445a15a6 100644 --- a/refs.c +++ b/refs.c @@ -3336,24 +3336,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data) return retval; } -static struct ref_lock *update_ref_lock(const char *refname, - const unsigned char *oldval, - int flags, int *type_p, - enum action_on_err onerr) -{ - struct ref_lock *lock; - lock = lock_any_ref_for_update(refname, oldval, flags, type_p); - if (!lock) { - const char *str = "Cannot lock the ref '%s'."; - switch (onerr) { - case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break; - case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break; - case UPDATE_REFS_QUIET_ON_ERR: break; - } - } - return lock; -} - static int update_ref_write(const char *action, const char *refname, const unsigned char *sha1, struct ref_lock *lock, struct strbuf *err, enum action_on_err onerr) @@ -3603,12 +3585,12 @@ int ref_transaction_commit(struct ref_transaction *transaction, for (i = 0; i < n; i++) { struct ref_update *update = updates[i]; - update->lock = update_ref_lock(update->refname, - (update->have_old ? - update->old_sha1 : NULL), - update->flags, - &update->type, - UPDATE_REFS_QUIET_ON_ERR); + update->lock = lock_any_ref_for_update(update->refname, + (update->have_old ? + update->old_sha1 : + NULL), + update->flags, + &update->type); if (!update->lock) { if (err) strbuf_addf(err, "Cannot lock the ref '%s'.", -- cgit v1.2.1 From 04ad6223ec1163163c4f39308149bee852ee245f Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Tue, 29 Apr 2014 13:42:07 -0700 Subject: refs.c: remove the update_ref_write function Since we only call update_ref_write from a single place and we only call it with onerr==QUIET_ON_ERR we can just as well get rid of it and just call write_ref_sha1 directly. This changes the return status for _commit from 1 to -1 on failures when writing to the ref. Eventually we will want _commit to start returning more detailed error conditions than the current simple success/failure. Signed-off-by: Ronnie Sahlberg Reviewed-by: Michael Haggerty Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- refs.c | 34 ++++++++-------------------------- 1 file changed, 8 insertions(+), 26 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index a4445a15a6..a6b39ec2d7 100644 --- a/refs.c +++ b/refs.c @@ -3336,25 +3336,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data) return retval; } -static int update_ref_write(const char *action, const char *refname, - const unsigned char *sha1, struct ref_lock *lock, - 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; - case UPDATE_REFS_QUIET_ON_ERR: break; - } - return 1; - } - return 0; -} - /** * Information needed for a single ref update. Set new_sha1 to the * new value or to zero to delete the ref. To check the old value @@ -3605,14 +3586,15 @@ int ref_transaction_commit(struct ref_transaction *transaction, struct ref_update *update = updates[i]; if (!is_null_sha1(update->new_sha1)) { - ret = update_ref_write(msg, - update->refname, - update->new_sha1, - update->lock, err, - UPDATE_REFS_QUIET_ON_ERR); - update->lock = NULL; /* freed by update_ref_write */ - if (ret) + ret = write_ref_sha1(update->lock, update->new_sha1, + msg); + update->lock = NULL; /* freed by write_ref_sha1 */ + if (ret) { + if (err) + strbuf_addf(err, "Cannot update the ref '%s'.", + update->refname); goto cleanup; + } } } -- cgit v1.2.1 From cba12021c3c932e42de838d0fc05d60b93790599 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Tue, 29 Apr 2014 15:45:52 -0700 Subject: refs.c: remove lock_ref_sha1 lock_ref_sha1 was only called from one place in refs.c and only provided a check that the refname was sane before adding back the initial "refs/" part of the ref path name, the initial "refs/" that this caller had already stripped off before calling lock_ref_sha1. Signed-off-by: Ronnie Sahlberg Reviewed-by: Michael Haggerty Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- refs.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index a6b39ec2d7..fd6768429d 100644 --- a/refs.c +++ b/refs.c @@ -2173,15 +2173,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, return NULL; } -static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1) -{ - char refpath[PATH_MAX]; - if (check_refname_format(refname, 0)) - return NULL; - strcpy(refpath, mkpath("refs/%s", refname)); - return lock_ref_sha1_basic(refpath, old_sha1, 0, NULL); -} - struct ref_lock *lock_any_ref_for_update(const char *refname, const unsigned char *old_sha1, int flags, int *type_p) @@ -2391,8 +2382,12 @@ static void try_remove_empty_parents(char *name) /* make sure nobody touched the ref, and unlink */ static void prune_ref(struct ref_to_prune *r) { - struct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1); + struct ref_lock *lock; + + if (check_refname_format(r->name + 5, 0)) + return; + lock = lock_ref_sha1_basic(r->name, r->sha1, 0, NULL); if (lock) { unlink_or_warn(git_path("%s", r->name)); unlock_ref(lock); -- cgit v1.2.1 From 029cdb4ab21c49a916efd68eaf2d2431c7fab7c7 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Wed, 30 Apr 2014 09:03:36 -0700 Subject: refs.c: make prune_ref use a transaction to delete the ref Change prune_ref to delete the ref using a ref transaction. To do this we also need to add a new flag REF_ISPRUNING that will tell the transaction that we do not want to delete this ref from the packed refs. This flag is private to refs.c and not exposed to external callers. Signed-off-by: Ronnie Sahlberg Reviewed-by: Michael Haggerty Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- refs.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index fd6768429d..22eb3dda7d 100644 --- a/refs.c +++ b/refs.c @@ -24,6 +24,11 @@ static unsigned char refname_disposition[256] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 4, 4 }; +/* + * Used as a flag to ref_transaction_delete when a loose ref is being + * pruned. + */ +#define REF_ISPRUNING 0x0100 /* * Try to read one refname component from the front of refname. * Return the length of the component found, or -1 if the component is @@ -2382,17 +2387,25 @@ static void try_remove_empty_parents(char *name) /* make sure nobody touched the ref, and unlink */ static void prune_ref(struct ref_to_prune *r) { - struct ref_lock *lock; + struct ref_transaction *transaction; + struct strbuf err = STRBUF_INIT; if (check_refname_format(r->name + 5, 0)) return; - lock = lock_ref_sha1_basic(r->name, r->sha1, 0, NULL); - if (lock) { - unlink_or_warn(git_path("%s", r->name)); - unlock_ref(lock); - try_remove_empty_parents(r->name); + transaction = ref_transaction_begin(&err); + if (!transaction || + ref_transaction_delete(transaction, r->name, r->sha1, + REF_ISPRUNING, 1, &err) || + ref_transaction_commit(transaction, NULL, &err)) { + ref_transaction_free(transaction); + error("%s", err.buf); + strbuf_release(&err); + return; } + ref_transaction_free(transaction); + strbuf_release(&err); + try_remove_empty_parents(r->name); } static void prune_refs(struct ref_to_prune *r) @@ -3598,8 +3611,9 @@ int ref_transaction_commit(struct ref_transaction *transaction, struct ref_update *update = updates[i]; if (update->lock) { - delnames[delnum++] = update->lock->ref_name; ret |= delete_ref_loose(update->lock, update->type); + if (!(update->flags & REF_ISPRUNING)) + delnames[delnum++] = update->lock->ref_name; } } -- cgit v1.2.1 From 7521cc4611a783f4a8174bd0fcec5f4a47357ac1 Mon Sep 17 00:00:00 2001 From: Ronnie Sahlberg Date: Wed, 30 Apr 2014 09:22:45 -0700 Subject: refs.c: make delete_ref use a transaction Change delete_ref to use a ref transaction for the deletion. At the same time since we no longer have any callers of repack_without_ref we can now delete this function. Change delete_ref to return 0 on success and 1 on failure instead of the previous 0 on success either 1 or -1 on failure. Signed-off-by: Ronnie Sahlberg Reviewed-by: Michael Haggerty Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- refs.c | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-) (limited to 'refs.c') diff --git a/refs.c b/refs.c index 22eb3dda7d..723557485d 100644 --- a/refs.c +++ b/refs.c @@ -2548,11 +2548,6 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err) return ret; } -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) { if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) { @@ -2570,24 +2565,22 @@ static int delete_ref_loose(struct ref_lock *lock, int flag) int delete_ref(const char *refname, const unsigned char *sha1, int delopt) { - struct ref_lock *lock; - int ret = 0, flag = 0; + struct ref_transaction *transaction; + struct strbuf err = STRBUF_INIT; - lock = lock_ref_sha1_basic(refname, sha1, delopt, &flag); - if (!lock) + transaction = ref_transaction_begin(&err); + if (!transaction || + ref_transaction_delete(transaction, refname, sha1, delopt, + sha1 && !is_null_sha1(sha1), &err) || + ref_transaction_commit(transaction, NULL, &err)) { + error("%s", err.buf); + ref_transaction_free(transaction); + strbuf_release(&err); return 1; - ret |= delete_ref_loose(lock, flag); - - /* removing the loose one could have resurrected an earlier - * packed one. Also, if it was not loose we need to repack - * without it. - */ - ret |= repack_without_ref(lock->ref_name); - - unlink_or_warn(git_path("logs/%s", lock->ref_name)); - clear_loose_ref_cache(&ref_cache); - unlock_ref(lock); - return ret; + } + ref_transaction_free(transaction); + strbuf_release(&err); + return 0; } /* -- cgit v1.2.1