* [PATCH 00/20] ref transactions part 2 @ 2014-07-15 23:33 Ronnie Sahlberg 2014-07-15 23:33 ` [PATCH 01/20] refs.c: change ref_transaction_create to do error checking and return status Ronnie Sahlberg ` (20 more replies) 0 siblings, 21 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:33 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg This is the next 20 patches from my originally big patch series and follow the previous 19 patches that is now in juns tree. These patches were numbered 20-39 in the original 48-patch series. Changes since these patches were in the original series: - Addressing concerns from mhagger's review Ronnie Sahlberg (20): refs.c: change ref_transaction_create to do error checking and return status refs.c: update ref_transaction_delete to check for error and return status refs.c: make ref_transaction_begin take an err argument refs.c: add transaction.status and track OPEN/CLOSED/ERROR tag.c: use ref transactions when doing updates replace.c: use the ref transaction functions for updates commit.c: use ref transactions for updates sequencer.c: use ref transactions for all ref updates fast-import.c: change update_branch to use ref transactions branch.c: use ref transaction for all ref updates refs.c: change update_ref to use a transaction receive-pack.c: use a reference transaction for updating the refs fast-import.c: use a ref transaction when dumping tags walker.c: use ref transaction for ref updates refs.c: make lock_ref_sha1 static refs.c: remove the update_ref_lock function refs.c: remove the update_ref_write function refs.c: remove lock_ref_sha1 refs.c: make prune_ref use a transaction to delete the ref refs.c: make delete_ref use a transaction branch.c | 30 +++--- builtin/commit.c | 24 +++-- builtin/receive-pack.c | 96 +++++++++++++------- builtin/replace.c | 15 +-- builtin/tag.c | 15 +-- builtin/update-ref.c | 11 ++- fast-import.c | 53 +++++++---- refs.c | 242 ++++++++++++++++++++++++++++--------------------- refs.h | 78 ++++++++++++---- sequencer.c | 27 ++++-- walker.c | 59 +++++++----- 11 files changed, 403 insertions(+), 247 deletions(-) -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 01/20] refs.c: change ref_transaction_create to do error checking and return status 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg @ 2014-07-15 23:33 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 02/20] refs.c: update ref_transaction_delete to check for error " Ronnie Sahlberg ` (19 subsequent siblings) 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:33 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg 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. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- builtin/update-ref.c | 4 +++- refs.c | 18 ++++++++++++------ refs.h | 48 +++++++++++++++++++++++++++++++++++++++++------- 3 files changed, 56 insertions(+), 14 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 3067b11..41121fa 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -226,7 +226,9 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next) if (*next != line_termination) die("create %s: extra input: %s", refname, next); - ref_transaction_create(transaction, refname, new_sha1, update_flags); + if (ref_transaction_create(transaction, refname, new_sha1, + update_flags, &err)) + die("%s", err.buf); update_flags = 0; free(refname); diff --git a/refs.c b/refs.c index 3f05e88..c49f1c6 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, diff --git a/refs.h b/refs.h index c5376ce..b648819 100644 --- a/refs.h +++ b/refs.h @@ -10,6 +10,38 @@ struct ref_lock { int force_write; }; +/* + * A ref_transaction represents a collection of ref updates + * that should succeed or fail together. + * + * Calling sequence + * ---------------- + * - Allocate and initialize a `struct ref_transaction` by calling + * `ref_transaction_begin()`. + * + * - List intended ref updates by calling functions like + * `ref_transaction_update()` and `ref_transaction_create()`. + * + * - Call `ref_transaction_commit()` to execute the transaction. + * If this succeeds, the ref updates will have taken place and + * the transaction cannot be rolled back. + * + * - At any time call `ref_transaction_free()` to discard the + * transaction and free associated resources. In particular, + * this rolls back the transaction if it has not been + * successfully committed. + * + * Error handling + * -------------- + * + * On error, transaction functions append a message about what + * went wrong to the 'err' argument. The message mentions what + * ref was being updated (if any) when the error occurred so it + * can be passed to 'die' or 'error' as-is. + * + * The message is appended to err without first clearing err. + * err will not be '\n' terminated. + */ struct ref_transaction; /* @@ -248,7 +280,7 @@ struct ref_transaction *ref_transaction_begin(void); * it must not have existed beforehand. * Function returns 0 on success and non-zero on failure. A failure to update * means that the transaction as a whole has failed and will need to be - * rolled back. On failure the err buffer will be updated. + * rolled back. */ int ref_transaction_update(struct ref_transaction *transaction, const char *refname, @@ -262,11 +294,15 @@ int ref_transaction_update(struct ref_transaction *transaction, * that the reference should have after the update; it must not be the * null SHA-1. It is verified that the reference does not exist * already. + * Function returns 0 on success and non-zero on failure. A failure to create + * means that the transaction as a whole has failed and will need to be + * rolled back. */ -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); /* * Add a reference deletion to transaction. If have_old is true, then @@ -282,8 +318,6 @@ void ref_transaction_delete(struct ref_transaction *transaction, * Commit all of the changes that have been queued in transaction, as * atomically as possible. Return a nonzero value if there is a * problem. - * If err is non-NULL we will add an error string to it to explain why - * the transaction failed. The string does not end in newline. */ int ref_transaction_commit(struct ref_transaction *transaction, const char *msg, struct strbuf *err); -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 02/20] refs.c: update ref_transaction_delete to check for error and return status 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg 2014-07-15 23:33 ` [PATCH 01/20] refs.c: change ref_transaction_create to do error checking and return status Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 03/20] refs.c: make ref_transaction_begin take an err argument Ronnie Sahlberg ` (18 subsequent siblings) 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg Change ref_transaction_delete() to do basic error checking and return non-zero of 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. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- builtin/update-ref.c | 5 +++-- refs.c | 16 +++++++++++----- refs.h | 12 ++++++++---- 3 files changed, 22 insertions(+), 11 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 41121fa..7c9c248 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -258,8 +258,9 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next) if (*next != line_termination) die("delete %s: extra input: %s", refname, next); - ref_transaction_delete(transaction, refname, old_sha1, - update_flags, have_old); + if (ref_transaction_delete(transaction, refname, old_sha1, + update_flags, have_old, &err)) + die("%s", err.buf); update_flags = 0; free(refname); diff --git a/refs.c b/refs.c index c49f1c6..40f04f4 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, diff --git a/refs.h b/refs.h index b648819..71389a1 100644 --- a/refs.h +++ b/refs.h @@ -308,11 +308,15 @@ int ref_transaction_create(struct ref_transaction *transaction, * Add a reference deletion to transaction. If have_old is true, then * old_sha1 holds the value that the reference should have had before * the update (which must not be the null SHA-1). + * Function returns 0 on success and non-zero on failure. A failure to delete + * means that the transaction as a whole has failed and will need to be + * rolled back. */ -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); /* * Commit all of the changes that have been queued in transaction, as -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 03/20] refs.c: make ref_transaction_begin take an err argument 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg 2014-07-15 23:33 ` [PATCH 01/20] refs.c: change ref_transaction_create to do error checking and return status Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 02/20] refs.c: update ref_transaction_delete to check for error " Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 04/20] refs.c: add transaction.status and track OPEN/CLOSED/ERROR Ronnie Sahlberg ` (17 subsequent siblings) 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg 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". Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- builtin/update-ref.c | 2 +- refs.c | 2 +- refs.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 7c9c248..c6ad0be 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -365,7 +365,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) die("Refusing to perform update with empty message."); if (read_stdin) { - transaction = ref_transaction_begin(); + transaction = ref_transaction_begin(&err); if (delete || no_deref || argc > 0) usage_with_options(git_update_ref_usage, options); if (end_null) diff --git a/refs.c b/refs.c index 40f04f4..9cb7908 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)); } diff --git a/refs.h b/refs.h index 71389a1..3f37c65 100644 --- a/refs.h +++ b/refs.h @@ -262,7 +262,7 @@ enum action_on_err { * Begin a reference transaction. The reference transaction must * be freed by calling ref_transaction_free(). */ -struct ref_transaction *ref_transaction_begin(void); +struct ref_transaction *ref_transaction_begin(struct strbuf *err); /* * The following functions add a reference check or update to a -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 04/20] refs.c: add transaction.status and track OPEN/CLOSED/ERROR 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg ` (2 preceding siblings ...) 2014-07-15 23:34 ` [PATCH 03/20] refs.c: make ref_transaction_begin take an err argument Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 05/20] tag.c: use ref transactions when doing updates Ronnie Sahlberg ` (16 subsequent siblings) 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg 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 <sahlberg@google.com> --- refs.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/refs.c b/refs.c index 9cb7908..d015285 100644 --- a/refs.c +++ b/refs.c @@ -3387,6 +3387,25 @@ struct ref_update { }; /* + * Transaction states. + * OPEN: The transaction is in a valid state and can accept new updates. + * An OPEN transaction can be committed. + * CLOSED: If an open transaction is successfully committed the state will + * change to CLOSED. No further changes can be made to a CLOSED + * transaction. + * CLOSED means that all updates have been successfully committed and + * the only thing that remains is to free the completed transaction. + * ERROR: The transaction has failed and is no longer committable. + * No further changes can be made to a CLOSED transaction and it must + * be rolled back using transaction_free. + */ +enum ref_transaction_state { + REF_TRANSACTION_OPEN = 0, + REF_TRANSACTION_CLOSED = 1, + REF_TRANSACTION_ERROR = 2, +}; + +/* * Data structure for holding a reference transaction, which can * consist of checks and updates to multiple references, carried out * as atomically as possible. This structure is opaque to callers. @@ -3395,6 +3414,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 +3457,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 +3480,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 +3503,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 +3561,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 +3629,9 @@ int ref_transaction_commit(struct ref_transaction *transaction, clear_loose_ref_cache(&ref_cache); cleanup: + transaction->state = ret ? REF_TRANSACTION_ERROR + : REF_TRANSACTION_CLOSED; + for (i = 0; i < n; i++) if (updates[i]->lock) unlock_ref(updates[i]->lock); -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 05/20] tag.c: use ref transactions when doing updates 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg ` (3 preceding siblings ...) 2014-07-15 23:34 ` [PATCH 04/20] refs.c: add transaction.status and track OPEN/CLOSED/ERROR Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 06/20] replace.c: use the ref transaction functions for updates Ronnie Sahlberg ` (15 subsequent siblings) 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg Change tag.c to use ref transactions for all ref updates. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- builtin/tag.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/builtin/tag.c b/builtin/tag.c index c6e8a71..1aa88a2 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -548,7 +548,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix) struct strbuf ref = STRBUF_INIT; unsigned char object[20], prev[20]; const char *object_ref, *tag; - struct ref_lock *lock; struct create_tag_options opt; char *cleanup_arg = NULL; int annotate = 0, force = 0, lines = -1; @@ -556,6 +555,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix) const char *msgfile = NULL, *keyid = NULL; struct msg_arg msg = { 0, STRBUF_INIT }; struct commit_list *with_commit = NULL; + struct ref_transaction *transaction; + struct strbuf err = STRBUF_INIT; struct option options[] = { OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'), { OPTION_INTEGER, 'n', NULL, &lines, N_("n"), @@ -701,11 +702,13 @@ int cmd_tag(int argc, const char **argv, const char *prefix) if (annotate) create_tag(object, tag, &buf, &opt, prev, object); - lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL); - if (!lock) - die(_("%s: cannot lock the ref"), ref.buf); - if (write_ref_sha1(lock, object, NULL) < 0) - die(_("%s: cannot update the ref"), ref.buf); + transaction = ref_transaction_begin(&err); + if (!transaction || + ref_transaction_update(transaction, ref.buf, object, prev, + 0, 1, &err) || + ref_transaction_commit(transaction, NULL, &err)) + die("%s", err.buf); + ref_transaction_free(transaction); if (force && !is_null_sha1(prev) && hashcmp(prev, object)) printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV)); -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 06/20] replace.c: use the ref transaction functions for updates 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg ` (4 preceding siblings ...) 2014-07-15 23:34 ` [PATCH 05/20] tag.c: use ref transactions when doing updates Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 07/20] commit.c: use ref transactions " Ronnie Sahlberg ` (14 subsequent siblings) 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg Update replace.c to use ref transactions for updates. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- builtin/replace.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/builtin/replace.c b/builtin/replace.c index 1bb491d..7528f3d 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -153,7 +153,8 @@ static int replace_object_sha1(const char *object_ref, unsigned char prev[20]; enum object_type obj_type, repl_type; char ref[PATH_MAX]; - struct ref_lock *lock; + struct ref_transaction *transaction; + struct strbuf err = STRBUF_INIT; obj_type = sha1_object_info(object, NULL); repl_type = sha1_object_info(repl, NULL); @@ -166,12 +167,14 @@ static int replace_object_sha1(const char *object_ref, check_ref_valid(object, prev, ref, sizeof(ref), force); - lock = lock_any_ref_for_update(ref, prev, 0, NULL); - if (!lock) - die("%s: cannot lock the ref", ref); - if (write_ref_sha1(lock, repl, NULL) < 0) - die("%s: cannot update the ref", ref); + transaction = ref_transaction_begin(&err); + if (!transaction || + ref_transaction_update(transaction, ref, repl, prev, + 0, !is_null_sha1(prev), &err) || + ref_transaction_commit(transaction, NULL, &err)) + die("%s", err.buf); + ref_transaction_free(transaction); return 0; } -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 07/20] commit.c: use ref transactions for updates 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg ` (5 preceding siblings ...) 2014-07-15 23:34 ` [PATCH 06/20] replace.c: use the ref transaction functions for updates Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 08/20] sequencer.c: use ref transactions for all ref updates Ronnie Sahlberg ` (13 subsequent siblings) 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg Change commit.c to use ref transactions for all ref updates. Make sure we pass a NULL pointer to ref_transaction_update if have_old is false. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- builtin/commit.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 5e2221c..668fa6a 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1627,11 +1627,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix) const char *index_file, *reflog_msg; char *nl; unsigned char sha1[20]; - struct ref_lock *ref_lock; struct commit_list *parents = NULL, **pptr = &parents; struct stat statbuf; struct commit *current_head = NULL; struct commit_extra_header *extra = NULL; + struct ref_transaction *transaction; + struct strbuf err = STRBUF_INIT; if (argc == 2 && !strcmp(argv[1], "-h")) usage_with_options(builtin_commit_usage, builtin_commit_options); @@ -1753,16 +1754,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix) strbuf_release(&author_ident); free_commit_extra_headers(extra); - ref_lock = lock_any_ref_for_update("HEAD", - !current_head - ? NULL - : current_head->object.sha1, - 0, NULL); - if (!ref_lock) { - rollback_index_files(); - die(_("cannot lock HEAD ref")); - } - nl = strchr(sb.buf, '\n'); if (nl) strbuf_setlen(&sb, nl + 1 - sb.buf); @@ -1771,10 +1762,17 @@ int cmd_commit(int argc, const char **argv, const char *prefix) strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg)); strbuf_insert(&sb, strlen(reflog_msg), ": ", 2); - if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) { + transaction = ref_transaction_begin(&err); + if (!transaction || + ref_transaction_update(transaction, "HEAD", sha1, + current_head ? + current_head->object.sha1 : NULL, + 0, !!current_head, &err) || + ref_transaction_commit(transaction, sb.buf, &err)) { rollback_index_files(); - die(_("cannot update HEAD ref")); + die("%s", err.buf); } + ref_transaction_free(transaction); unlink(git_path("CHERRY_PICK_HEAD")); unlink(git_path("REVERT_HEAD")); -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 08/20] sequencer.c: use ref transactions for all ref updates 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg ` (6 preceding siblings ...) 2014-07-15 23:34 ` [PATCH 07/20] commit.c: use ref transactions " Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 09/20] fast-import.c: change update_branch to use ref transactions Ronnie Sahlberg ` (12 subsequent siblings) 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg Change to use ref transactions for all updates to refs. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- sequencer.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/sequencer.c b/sequencer.c index 9230474..cf17c69 100644 --- a/sequencer.c +++ b/sequencer.c @@ -272,23 +272,32 @@ static int error_dirty_index(struct replay_opts *opts) static int fast_forward_to(const unsigned char *to, const unsigned char *from, int unborn, struct replay_opts *opts) { - struct ref_lock *ref_lock; + struct ref_transaction *transaction; struct strbuf sb = STRBUF_INIT; - int ret; + struct strbuf err = STRBUF_INIT; read_cache(); if (checkout_fast_forward(from, to, 1)) - exit(128); /* the callee should have complained already */ - ref_lock = lock_any_ref_for_update("HEAD", unborn ? null_sha1 : from, - 0, NULL); - if (!ref_lock) - return error(_("Failed to lock HEAD during fast_forward_to")); + exit(1); /* the callee should have complained already */ strbuf_addf(&sb, "%s: fast-forward", action_name(opts)); - ret = write_ref_sha1(ref_lock, to, sb.buf); + + transaction = ref_transaction_begin(&err); + if (!transaction || + ref_transaction_update(transaction, "HEAD", + to, unborn ? null_sha1 : from, + 0, 1, &err) || + ref_transaction_commit(transaction, sb.buf, &err)) { + ref_transaction_free(transaction); + error("%s", err.buf); + strbuf_release(&sb); + strbuf_release(&err); + return -1; + } strbuf_release(&sb); - return ret; + ref_transaction_free(transaction); + return 0; } static int do_recursive_merge(struct commit *base, struct commit *next, -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 09/20] fast-import.c: change update_branch to use ref transactions 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg ` (7 preceding siblings ...) 2014-07-15 23:34 ` [PATCH 08/20] sequencer.c: use ref transactions for all ref updates Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 10/20] branch.c: use ref transaction for all ref updates Ronnie Sahlberg ` (11 subsequent siblings) 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg Change update_branch() to use ref transactions for updates. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- fast-import.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/fast-import.c b/fast-import.c index 6707a66..d5206ee 100644 --- a/fast-import.c +++ b/fast-import.c @@ -1679,8 +1679,9 @@ found_entry: static int update_branch(struct branch *b) { static const char *msg = "fast-import"; - struct ref_lock *lock; + struct ref_transaction *transaction; unsigned char old_sha1[20]; + struct strbuf err = STRBUF_INIT; if (read_ref(b->name, old_sha1)) hashclr(old_sha1); @@ -1689,29 +1690,32 @@ static int update_branch(struct branch *b) delete_ref(b->name, old_sha1, 0); return 0; } - lock = lock_any_ref_for_update(b->name, old_sha1, 0, NULL); - if (!lock) - return error("Unable to lock %s", b->name); if (!force_update && !is_null_sha1(old_sha1)) { struct commit *old_cmit, *new_cmit; old_cmit = lookup_commit_reference_gently(old_sha1, 0); new_cmit = lookup_commit_reference_gently(b->sha1, 0); - if (!old_cmit || !new_cmit) { - unlock_ref(lock); + if (!old_cmit || !new_cmit) return error("Branch %s is missing commits.", b->name); - } if (!in_merge_bases(old_cmit, new_cmit)) { - unlock_ref(lock); warning("Not updating %s" " (new tip %s does not contain %s)", b->name, sha1_to_hex(b->sha1), sha1_to_hex(old_sha1)); return -1; } } - if (write_ref_sha1(lock, b->sha1, msg) < 0) - return error("Unable to update %s", b->name); + transaction = ref_transaction_begin(&err); + if (!transaction || + ref_transaction_update(transaction, b->name, b->sha1, old_sha1, + 0, 1, &err) || + ref_transaction_commit(transaction, msg, &err)) { + ref_transaction_free(transaction); + error("%s", err.buf); + strbuf_release(&err); + return -1; + } + ref_transaction_free(transaction); return 0; } -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 10/20] branch.c: use ref transaction for all ref updates 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg ` (8 preceding siblings ...) 2014-07-15 23:34 ` [PATCH 09/20] fast-import.c: change update_branch to use ref transactions Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 11/20] refs.c: change update_ref to use a transaction Ronnie Sahlberg ` (10 subsequent siblings) 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg Change create_branch to use a ref transaction when creating the new branch. This also fixes a race condition in the old code where two concurrent create_branch could race since the lock_any_ref_for_update/write_ref_sha1 did not protect against the ref already existing. I.e. one thread could end up overwriting a branch even if the forcing flag is false. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- branch.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/branch.c b/branch.c index 660097b..c1eae00 100644 --- a/branch.c +++ b/branch.c @@ -226,7 +226,6 @@ void create_branch(const char *head, int force, int reflog, int clobber_head, int quiet, enum branch_track track) { - struct ref_lock *lock = NULL; struct commit *commit; unsigned char sha1[20]; char *real_ref, msg[PATH_MAX + 20]; @@ -285,15 +284,6 @@ void create_branch(const char *head, die(_("Not a valid branch point: '%s'."), start_name); hashcpy(sha1, commit->object.sha1); - if (!dont_change_ref) { - lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL); - if (!lock) - die_errno(_("Failed to lock ref for update")); - } - - if (reflog) - log_all_ref_updates = 1; - if (forcing) snprintf(msg, sizeof msg, "branch: Reset to %s", start_name); @@ -301,13 +291,25 @@ void create_branch(const char *head, snprintf(msg, sizeof msg, "branch: Created from %s", start_name); + if (reflog) + log_all_ref_updates = 1; + + if (!dont_change_ref) { + struct ref_transaction *transaction; + struct strbuf err = STRBUF_INIT; + + transaction = ref_transaction_begin(&err); + if (!transaction || + ref_transaction_update(transaction, ref.buf, sha1, + null_sha1, 0, !forcing, &err) || + ref_transaction_commit(transaction, msg, &err)) + die("%s", err.buf); + ref_transaction_free(transaction); + } + if (real_ref && track) setup_tracking(ref.buf + 11, real_ref, track, quiet); - if (!dont_change_ref) - if (write_ref_sha1(lock, sha1, msg) < 0) - die_errno(_("Failed to write ref")); - strbuf_release(&ref); free(real_ref); } -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 11/20] refs.c: change update_ref to use a transaction 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg ` (9 preceding siblings ...) 2014-07-15 23:34 ` [PATCH 10/20] branch.c: use ref transaction for all ref updates Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 12/20] receive-pack.c: use a reference transaction for updating the refs Ronnie Sahlberg ` (9 subsequent siblings) 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg Change the update_ref helper function to use a ref transaction internally. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- refs.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/refs.c b/refs.c index d015285..ff4e799 100644 --- a/refs.c +++ b/refs.c @@ -3523,11 +3523,31 @@ 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); + } + return 0; } static int ref_update_compare(const void *r1, const void *r2) -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 12/20] receive-pack.c: use a reference transaction for updating the refs 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg ` (10 preceding siblings ...) 2014-07-15 23:34 ` [PATCH 11/20] refs.c: change update_ref to use a transaction Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 13/20] fast-import.c: use a ref transaction when dumping tags Ronnie Sahlberg ` (8 subsequent siblings) 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg Wrap all the ref updates inside a transaction. Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- builtin/receive-pack.c | 96 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 63 insertions(+), 33 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index c323081..91099ad 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -194,7 +194,7 @@ static void write_head_info(void) struct command { struct command *next; - const char *error_string; + char *error_string; unsigned int skip_update:1, did_not_exist:1; int index; @@ -468,19 +468,18 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si) return 0; } -static const char *update(struct command *cmd, struct shallow_info *si) +static char *update(struct command *cmd, struct shallow_info *si) { const char *name = cmd->ref_name; struct strbuf namespaced_name_buf = STRBUF_INIT; const char *namespaced_name; unsigned char *old_sha1 = cmd->old_sha1; unsigned char *new_sha1 = cmd->new_sha1; - struct ref_lock *lock; /* only refs/... are allowed */ if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) { rp_error("refusing to create funny ref '%s' remotely", name); - return "funny refname"; + return xstrdup("funny refname"); } strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name); @@ -498,20 +497,20 @@ static const char *update(struct command *cmd, struct shallow_info *si) rp_error("refusing to update checked out branch: %s", name); if (deny_current_branch == DENY_UNCONFIGURED) refuse_unconfigured_deny(); - return "branch is currently checked out"; + return xstrdup("branch is currently checked out"); } } if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) { error("unpack should have generated %s, " "but I can't find it!", sha1_to_hex(new_sha1)); - return "bad pack"; + return xstrdup("bad pack"); } if (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) { if (deny_deletes && starts_with(name, "refs/heads/")) { rp_error("denying ref deletion for %s", name); - return "deletion prohibited"; + return xstrdup("deletion prohibited"); } if (!strcmp(namespaced_name, head_name)) { @@ -526,7 +525,7 @@ static const char *update(struct command *cmd, struct shallow_info *si) if (deny_delete_current == DENY_UNCONFIGURED) refuse_unconfigured_deny_delete_current(); rp_error("refusing to delete the current branch: %s", name); - return "deletion of the current branch prohibited"; + return xstrdup("deletion of the current branch prohibited"); } } } @@ -544,19 +543,19 @@ static const char *update(struct command *cmd, struct shallow_info *si) old_object->type != OBJ_COMMIT || new_object->type != OBJ_COMMIT) { error("bad sha1 objects for %s", name); - return "bad ref"; + return xstrdup("bad ref"); } old_commit = (struct commit *)old_object; new_commit = (struct commit *)new_object; if (!in_merge_bases(old_commit, new_commit)) { rp_error("denying non-fast-forward %s" " (you should pull first)", name); - return "non-fast-forward"; + return xstrdup("non-fast-forward"); } } if (run_update_hook(cmd)) { rp_error("hook declined to update %s", name); - return "hook declined"; + return xstrdup("hook declined"); } if (is_null_sha1(new_sha1)) { @@ -571,24 +570,32 @@ static const char *update(struct command *cmd, struct shallow_info *si) } if (delete_ref(namespaced_name, old_sha1, 0)) { rp_error("failed to delete %s", name); - return "failed to delete"; + return xstrdup("failed to delete"); } return NULL; /* good */ } else { + struct strbuf err = STRBUF_INIT; + struct ref_transaction *transaction; + if (shallow_update && si->shallow_ref[cmd->index] && update_shallow_ref(cmd, si)) - return "shallow error"; - - lock = lock_any_ref_for_update(namespaced_name, old_sha1, - 0, NULL); - if (!lock) { - rp_error("failed to lock %s", name); - return "failed to lock"; - } - if (write_ref_sha1(lock, new_sha1, "push")) { - return "failed to write"; /* error() already called */ + return xstrdup("shallow error"); + + transaction = ref_transaction_begin(&err); + if (!transaction || + ref_transaction_update(transaction, namespaced_name, + new_sha1, old_sha1, 0, 1, &err) || + ref_transaction_commit(transaction, "push", &err)) { + char *str = strbuf_detach(&err, NULL); + ref_transaction_free(transaction); + + rp_error("%s", str); + return str; } + + ref_transaction_free(transaction); + strbuf_release(&err); return NULL; /* good */ } } @@ -647,6 +654,9 @@ static void check_aliased_update(struct command *cmd, struct string_list *list) char cmd_oldh[41], cmd_newh[41], dst_oldh[41], dst_newh[41]; int flag; + if (cmd->error_string) + die("BUG: check_aliased_update called with failed cmd"); + strbuf_addf(&buf, "%s%s", get_git_namespace(), cmd->ref_name); dst_name = resolve_ref_unsafe(buf.buf, sha1, 0, &flag); strbuf_release(&buf); @@ -658,7 +668,7 @@ static void check_aliased_update(struct command *cmd, struct string_list *list) if (!dst_name) { rp_error("refusing update to broken symref '%s'", cmd->ref_name); cmd->skip_update = 1; - cmd->error_string = "broken symref"; + cmd->error_string = xstrdup("broken symref"); return; } @@ -684,8 +694,9 @@ static void check_aliased_update(struct command *cmd, struct string_list *list) cmd->ref_name, cmd_oldh, cmd_newh, dst_cmd->ref_name, dst_oldh, dst_newh); - cmd->error_string = dst_cmd->error_string = - "inconsistent aliased update"; + cmd->error_string = xstrdup("inconsistent aliased update"); + free(dst_cmd->error_string); + dst_cmd->error_string = xstrdup("inconsistent aliased update"); } static void check_aliased_updates(struct command *commands) @@ -733,7 +744,9 @@ static void set_connectivity_errors(struct command *commands, if (!check_everything_connected(command_singleton_iterator, 0, &singleton)) continue; - cmd->error_string = "missing necessary objects"; + if (cmd->error_string) /* can't happen */ + continue; + cmd->error_string = xstrdup("missing necessary objects"); } } @@ -770,9 +783,9 @@ static void reject_updates_to_hidden(struct command *commands) if (cmd->error_string || !ref_is_hidden(cmd->ref_name)) continue; if (is_null_sha1(cmd->new_sha1)) - cmd->error_string = "deny deleting a hidden ref"; + cmd->error_string = xstrdup("deny deleting a hidden ref"); else - cmd->error_string = "deny updating a hidden ref"; + cmd->error_string = xstrdup("deny updating a hidden ref"); } } @@ -786,8 +799,11 @@ static void execute_commands(struct command *commands, struct iterate_data data; if (unpacker_error) { - for (cmd = commands; cmd; cmd = cmd->next) - cmd->error_string = "unpacker error"; + for (cmd = commands; cmd; cmd = cmd->next) { + if (cmd->error_string) /* can't happen */ + continue; + cmd->error_string = xstrdup("unpacker error"); + } return; } @@ -800,8 +816,9 @@ static void execute_commands(struct command *commands, if (run_receive_hook(commands, "pre-receive", 0)) { for (cmd = commands; cmd; cmd = cmd->next) { - if (!cmd->error_string) - cmd->error_string = "pre-receive hook declined"; + if (cmd->error_string) + continue; + cmd->error_string = xstrdup("pre-receive hook declined"); } return; } @@ -1079,7 +1096,8 @@ static void update_shallow_info(struct command *commands, if (is_null_sha1(cmd->new_sha1)) continue; if (ref_status[cmd->index]) { - cmd->error_string = "shallow update not allowed"; + free(cmd->error_string); + cmd->error_string = xstrdup("shallow update not allowed"); cmd->skip_update = 1; } } @@ -1120,6 +1138,17 @@ static int delete_only(struct command *commands) return 1; } +static void free_commands(struct command *commands) +{ + while (commands) { + struct command *next = commands->next; + + free(commands->error_string); + free(commands); + commands = next; + } +} + int cmd_receive_pack(int argc, const char **argv, const char *prefix) { int advertise_refs = 0; @@ -1215,5 +1244,6 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) packet_flush(1); sha1_array_clear(&shallow); sha1_array_clear(&ref); + free_commands(commands); return 0; } -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 13/20] fast-import.c: use a ref transaction when dumping tags 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg ` (11 preceding siblings ...) 2014-07-15 23:34 ` [PATCH 12/20] receive-pack.c: use a reference transaction for updating the refs Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 14/20] walker.c: use ref transaction for ref updates Ronnie Sahlberg ` (7 subsequent siblings) 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- fast-import.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/fast-import.c b/fast-import.c index d5206ee..a95e1be 100644 --- a/fast-import.c +++ b/fast-import.c @@ -1734,15 +1734,32 @@ static void dump_tags(void) { static const char *msg = "fast-import"; struct tag *t; - struct ref_lock *lock; - char ref_name[PATH_MAX]; + struct strbuf ref_name = STRBUF_INIT; + struct strbuf err = STRBUF_INIT; + struct ref_transaction *transaction; + transaction = ref_transaction_begin(&err); + if (!transaction) { + failure |= error("%s", err.buf); + goto cleanup; + } for (t = first_tag; t; t = t->next_tag) { - sprintf(ref_name, "tags/%s", t->name); - lock = lock_ref_sha1(ref_name, NULL); - if (!lock || write_ref_sha1(lock, t->sha1, msg) < 0) - failure |= error("Unable to update %s", ref_name); + strbuf_reset(&ref_name); + strbuf_addf(&ref_name, "refs/tags/%s", t->name); + + if (ref_transaction_update(transaction, ref_name.buf, t->sha1, + NULL, 0, 0, &err)) { + failure |= error("%s", err.buf); + goto cleanup; + } } + if (ref_transaction_commit(transaction, msg, &err)) + failure |= error("%s", err.buf); + + cleanup: + ref_transaction_free(transaction); + strbuf_release(&ref_name); + strbuf_release(&err); } static void dump_marks_helper(FILE *f, -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 14/20] walker.c: use ref transaction for ref updates 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg ` (12 preceding siblings ...) 2014-07-15 23:34 ` [PATCH 13/20] fast-import.c: use a ref transaction when dumping tags Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 15/20] refs.c: make lock_ref_sha1 static Ronnie Sahlberg ` (6 subsequent siblings) 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg Switch to using ref transactions in walker_fetch(). As part of the refactoring to use ref transactions we also fix a potential memory leak where in the original code if write_ref_sha1() would fail we would end up returning from the function without free()ing the msg string. Note that this function is only called when fetching from a remote HTTP repository onto the local (most of the time single-user) repository which likely means that the type of collisions that the previous locking would protect against and cause the fetch to fail for are even more rare. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- walker.c | 59 +++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 35 insertions(+), 24 deletions(-) diff --git a/walker.c b/walker.c index 1dd86b8..60d9f9e 100644 --- a/walker.c +++ b/walker.c @@ -251,39 +251,36 @@ void walker_targets_free(int targets, char **target, const char **write_ref) int walker_fetch(struct walker *walker, int targets, char **target, const char **write_ref, const char *write_ref_log_details) { - struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *)); + struct strbuf ref_name = STRBUF_INIT; + struct strbuf err = STRBUF_INIT; + struct ref_transaction *transaction = NULL; unsigned char *sha1 = xmalloc(targets * 20); - char *msg; - int ret; + char *msg = NULL; int i; save_commit_buffer = 0; - for (i = 0; i < targets; i++) { - if (!write_ref || !write_ref[i]) - continue; - - lock[i] = lock_ref_sha1(write_ref[i], NULL); - if (!lock[i]) { - error("Can't lock ref %s", write_ref[i]); - goto unlock_and_fail; + if (write_ref) { + transaction = ref_transaction_begin(&err); + if (!transaction) { + error("%s", err.buf); + goto rollback_and_fail; } } - if (!walker->get_recover) for_each_ref(mark_complete, NULL); for (i = 0; i < targets; i++) { if (interpret_target(walker, target[i], &sha1[20 * i])) { error("Could not interpret response from server '%s' as something to pull", target[i]); - goto unlock_and_fail; + goto rollback_and_fail; } if (process(walker, lookup_unknown_object(&sha1[20 * i]))) - goto unlock_and_fail; + goto rollback_and_fail; } if (loop(walker)) - goto unlock_and_fail; + goto rollback_and_fail; if (write_ref_log_details) { msg = xmalloc(strlen(write_ref_log_details) + 12); @@ -294,19 +291,33 @@ int walker_fetch(struct walker *walker, int targets, char **target, for (i = 0; i < targets; i++) { if (!write_ref || !write_ref[i]) continue; - ret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : "fetch (unknown)"); - lock[i] = NULL; - if (ret) - goto unlock_and_fail; + strbuf_reset(&ref_name); + strbuf_addf(&ref_name, "refs/%s", write_ref[i]); + if (ref_transaction_update(transaction, ref_name.buf, + &sha1[20 * i], NULL, 0, 0, + &err)) { + error("%s", err.buf); + goto rollback_and_fail; + } + } + if (write_ref) { + if (ref_transaction_commit(transaction, + msg ? msg : "fetch (unknown)", + &err)) { + error("%s", err.buf); + goto rollback_and_fail; + } + ref_transaction_free(transaction); } - free(msg); + free(msg); return 0; -unlock_and_fail: - for (i = 0; i < targets; i++) - if (lock[i]) - unlock_ref(lock[i]); +rollback_and_fail: + ref_transaction_free(transaction); + free(msg); + strbuf_release(&err); + strbuf_release(&ref_name); return -1; } -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 15/20] refs.c: make lock_ref_sha1 static 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg ` (13 preceding siblings ...) 2014-07-15 23:34 ` [PATCH 14/20] walker.c: use ref transaction for ref updates Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 16/20] refs.c: remove the update_ref_lock function Ronnie Sahlberg ` (5 subsequent siblings) 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg No external callers reference lock_ref_sha1 any more so lets declare it static. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- refs.c | 2 +- refs.h | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/refs.c b/refs.c index ff4e799..10cea87 100644 --- a/refs.c +++ b/refs.c @@ -2170,7 +2170,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)) diff --git a/refs.h b/refs.h index 3f37c65..65dd593 100644 --- a/refs.h +++ b/refs.h @@ -170,12 +170,6 @@ extern int ref_exists(const char *); */ extern int peel_ref(const char *refname, unsigned char *sha1); -/* - * Locks a "refs/" ref returning the lock on success and NULL on failure. - * On failure errno is set to something meaningful. - */ -extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1); - /** Locks any ref (for 'HEAD' type refs). */ #define REF_NODEREF 0x01 /* errno is set to something meaningful on failure */ -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 16/20] refs.c: remove the update_ref_lock function 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg ` (14 preceding siblings ...) 2014-07-15 23:34 ` [PATCH 15/20] refs.c: make lock_ref_sha1 static Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 17/20] refs.c: remove the update_ref_write function Ronnie Sahlberg ` (4 subsequent siblings) 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg 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. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- refs.c | 30 ++++++------------------------ 1 file changed, 6 insertions(+), 24 deletions(-) diff --git a/refs.c b/refs.c index 10cea87..091c343 100644 --- a/refs.c +++ b/refs.c @@ -3333,24 +3333,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) @@ -3602,12 +3584,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'.", -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 17/20] refs.c: remove the update_ref_write function 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg ` (15 preceding siblings ...) 2014-07-15 23:34 ` [PATCH 16/20] refs.c: remove the update_ref_lock function Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 18/20] refs.c: remove lock_ref_sha1 Ronnie Sahlberg ` (3 subsequent siblings) 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg 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. For example if the commit failed due to name conflicts etc. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- refs.c | 35 +++++++++-------------------------- 1 file changed, 9 insertions(+), 26 deletions(-) diff --git a/refs.c b/refs.c index 091c343..27c629f 100644 --- a/refs.c +++ b/refs.c @@ -3333,25 +3333,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 @@ -3604,14 +3585,16 @@ 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) { + const char *str = "Cannot update the ref '%s'."; + + if (err) + strbuf_addf(err, str, update->refname); goto cleanup; + } } } -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 18/20] refs.c: remove lock_ref_sha1 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg ` (16 preceding siblings ...) 2014-07-15 23:34 ` [PATCH 17/20] refs.c: remove the update_ref_write function Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 19/20] refs.c: make prune_ref use a transaction to delete the ref Ronnie Sahlberg ` (2 subsequent siblings) 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg lock_ref_sha1 was only called from one place in refc.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. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- refs.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/refs.c b/refs.c index 27c629f..dbf6af9 100644 --- a/refs.c +++ b/refs.c @@ -2170,15 +2170,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) @@ -2388,8 +2379,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); -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 19/20] refs.c: make prune_ref use a transaction to delete the ref 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg ` (17 preceding siblings ...) 2014-07-15 23:34 ` [PATCH 18/20] refs.c: remove lock_ref_sha1 Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 20/20] refs.c: make delete_ref use a transaction Ronnie Sahlberg 2014-07-15 23:37 ` [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg 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. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- refs.c | 27 ++++++++++++++++++++------- refs.h | 14 ++++++++++++-- 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/refs.c b/refs.c index dbf6af9..186df37 100644 --- a/refs.c +++ b/refs.c @@ -25,6 +25,11 @@ static unsigned char refname_disposition[256] = { }; /* + * 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 * not legal. It is legal if it is something reasonable to have under @@ -2379,17 +2384,24 @@ 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); + try_remove_empty_parents(r->name); } static void prune_refs(struct ref_to_prune *r) @@ -3598,8 +3610,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; } } diff --git a/refs.h b/refs.h index 65dd593..aad846c 100644 --- a/refs.h +++ b/refs.h @@ -170,9 +170,19 @@ extern int ref_exists(const char *); */ extern int peel_ref(const char *refname, unsigned char *sha1); -/** Locks any ref (for 'HEAD' type refs). */ +/* + * Flags controlling lock_any_ref_for_update(), ref_transaction_update(), + * ref_transaction_create(), etc. + * REF_NODEREF: act on the ref directly, instead of dereferencing + * symbolic references. + * + * Flags >= 0x100 are reserved for internal use. + */ #define REF_NODEREF 0x01 -/* errno is set to something meaningful on failure */ +/* + * Locks any ref (for 'HEAD' type refs) and sets errno to something + * meaningful on failure. + */ extern struct ref_lock *lock_any_ref_for_update(const char *refname, const unsigned char *old_sha1, int flags, int *type_p); -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 20/20] refs.c: make delete_ref use a transaction 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg ` (18 preceding siblings ...) 2014-07-15 23:34 ` [PATCH 19/20] refs.c: make prune_ref use a transaction to delete the ref Ronnie Sahlberg @ 2014-07-15 23:34 ` Ronnie Sahlberg 2014-07-15 23:37 ` [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg 20 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:34 UTC (permalink / raw) To: git; +Cc: mhagger, Ronnie Sahlberg 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. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com> --- refs.c | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/refs.c b/refs.c index 186df37..0017d9c 100644 --- a/refs.c +++ b/refs.c @@ -2544,11 +2544,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) { @@ -2566,24 +2561,21 @@ 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); + return 0; } /* -- 2.0.1.442.g7fe6834.dirty ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 00/20] ref transactions part 2 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg ` (19 preceding siblings ...) 2014-07-15 23:34 ` [PATCH 20/20] refs.c: make delete_ref use a transaction Ronnie Sahlberg @ 2014-07-15 23:37 ` Ronnie Sahlberg 2014-07-16 22:16 ` Junio C Hamano 20 siblings, 1 reply; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-15 23:37 UTC (permalink / raw) To: git, Michael Haggerty; +Cc: Ronnie Sahlberg Hi Michael, Here is the next set of 20 patches of those you already reviewed. I cut this patch off just before patch #40 in the previous series. These are the patches numbered 20-39 in the original series. I think I have addressed your concerns here so If you could take a quick look and hopefully bless them that would be awesome! Thanks ronnie sahlberg On Tue, Jul 15, 2014 at 4:33 PM, Ronnie Sahlberg <sahlberg@google.com> wrote: > This is the next 20 patches from my originally big patch series and follow > the previous 19 patches that is now in juns tree. > These patches were numbered 20-39 in the original 48-patch series. > > Changes since these patches were in the original series: > > - Addressing concerns from mhagger's review > > > Ronnie Sahlberg (20): > refs.c: change ref_transaction_create to do error checking and return > status > refs.c: update ref_transaction_delete to check for error and return > status > refs.c: make ref_transaction_begin take an err argument > refs.c: add transaction.status and track OPEN/CLOSED/ERROR > tag.c: use ref transactions when doing updates > replace.c: use the ref transaction functions for updates > commit.c: use ref transactions for updates > sequencer.c: use ref transactions for all ref updates > fast-import.c: change update_branch to use ref transactions > branch.c: use ref transaction for all ref updates > refs.c: change update_ref to use a transaction > receive-pack.c: use a reference transaction for updating the refs > fast-import.c: use a ref transaction when dumping tags > walker.c: use ref transaction for ref updates > refs.c: make lock_ref_sha1 static > refs.c: remove the update_ref_lock function > refs.c: remove the update_ref_write function > refs.c: remove lock_ref_sha1 > refs.c: make prune_ref use a transaction to delete the ref > refs.c: make delete_ref use a transaction > > branch.c | 30 +++--- > builtin/commit.c | 24 +++-- > builtin/receive-pack.c | 96 +++++++++++++------- > builtin/replace.c | 15 +-- > builtin/tag.c | 15 +-- > builtin/update-ref.c | 11 ++- > fast-import.c | 53 +++++++---- > refs.c | 242 ++++++++++++++++++++++++++++--------------------- > refs.h | 78 ++++++++++++---- > sequencer.c | 27 ++++-- > walker.c | 59 +++++++----- > 11 files changed, 403 insertions(+), 247 deletions(-) > > -- > 2.0.1.442.g7fe6834.dirty > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 00/20] ref transactions part 2 2014-07-15 23:37 ` [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg @ 2014-07-16 22:16 ` Junio C Hamano 2014-07-16 22:52 ` Ronnie Sahlberg 0 siblings, 1 reply; 25+ messages in thread From: Junio C Hamano @ 2014-07-16 22:16 UTC (permalink / raw) To: Ronnie Sahlberg; +Cc: git, Michael Haggerty Ronnie Sahlberg <sahlberg@google.com> writes: > On Tue, Jul 15, 2014 at 4:33 PM, Ronnie Sahlberg <sahlberg@google.com> wrote: >> This is the next 20 patches from my originally big patch series and follow >> the previous 19 patches that is now in juns tree. >> These patches were numbered 20-39 in the original 48-patch series. >> >> Changes since these patches were in the original series: >> >> - Addressing concerns from mhagger's review One patch in the series did not apply cleanly on top of the tip of the previous series (now queued as rs/ref-transaction-0) and I had to wiggle it. Please check the result (queued as three topics, this one is rs/ref-transaction-1 which is built on the abovementioned "-0", and the remainder from the previous round is rebased on "-1" as rs/ref-transaction), all of which are queued on 'jch' (which is part of 'pu'). Thanks. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 00/20] ref transactions part 2 2014-07-16 22:16 ` Junio C Hamano @ 2014-07-16 22:52 ` Ronnie Sahlberg 0 siblings, 0 replies; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-16 22:52 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Michael Haggerty I had a look at the changes in origin/pu and they look sane to me. make test passes all tests too. regards ronnie sahlberg On Wed, Jul 16, 2014 at 3:16 PM, Junio C Hamano <gitster@pobox.com> wrote: > Ronnie Sahlberg <sahlberg@google.com> writes: > >> On Tue, Jul 15, 2014 at 4:33 PM, Ronnie Sahlberg <sahlberg@google.com> wrote: >>> This is the next 20 patches from my originally big patch series and follow >>> the previous 19 patches that is now in juns tree. >>> These patches were numbered 20-39 in the original 48-patch series. >>> >>> Changes since these patches were in the original series: >>> >>> - Addressing concerns from mhagger's review > > One patch in the series did not apply cleanly on top of the tip of > the previous series (now queued as rs/ref-transaction-0) and I had > to wiggle it. Please check the result (queued as three topics, this > one is rs/ref-transaction-1 which is built on the abovementioned > "-0", and the remainder from the previous round is rebased on "-1" > as rs/ref-transaction), all of which are queued on 'jch' (which is > part of 'pu'). > > Thanks. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Transaction patch series overview @ 2014-07-30 17:10 Ronnie Sahlberg 2014-07-31 21:41 ` Ronnie Sahlberg 0 siblings, 1 reply; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-30 17:10 UTC (permalink / raw) To: git List, please see here an overview and ordering of the ref transaction patch series. These series build on each other and needs to be applied in the order listed below. rs/ref-transaction-0 --------------------------- Early part of the "ref transaction" topic. * rs/ref-transaction-0: refs.c: change ref_transaction_update() to do error checking and return status refs.c: remove the onerr argument to ref_transaction_commit update-ref: use err argument to get error from ref_transaction_commit refs.c: make update_ref_write update a strbuf on failure refs.c: make ref_update_reject_duplicates take a strbuf argument for errors refs.c: log_ref_write should try to return meaningful errno refs.c: make resolve_ref_unsafe set errno to something meaningful on error refs.c: commit_packed_refs to return a meaningful errno on failure refs.c: make remove_empty_directories always set errno to something sane refs.c: verify_lock should set errno to something meaningful refs.c: make sure log_ref_setup returns a meaningful errno refs.c: add an err argument to repack_without_refs lockfile.c: make lock_file return a meaningful errno on failurei lockfile.c: add a new public function unable_to_lock_message refs.c: add a strbuf argument to ref_transaction_commit for error logging refs.c: allow passing NULL to ref_transaction_free refs.c: constify the sha arguments for ref_transaction_create|delete|update refs.c: ref_transaction_commit should not free the transaction refs.c: remove ref_transaction_rollback Has been merged into next. ref-transaction-1 (2014-07-16) 20 commits ------------------------------------------------------------- Second batch of ref transactions - refs.c: make delete_ref use a transaction - refs.c: make prune_ref use a transaction to delete the ref - refs.c: remove lock_ref_sha1 - refs.c: remove the update_ref_write function - refs.c: remove the update_ref_lock function - refs.c: make lock_ref_sha1 static - walker.c: use ref transaction for ref updates - fast-import.c: use a ref transaction when dumping tags - receive-pack.c: use a reference transaction for updating the refs - refs.c: change update_ref to use a transaction - branch.c: use ref transaction for all ref updates - fast-import.c: change update_branch to use ref transactions - sequencer.c: use ref transactions for all ref updates - commit.c: use ref transactions for updates - replace.c: use the ref transaction functions for updates - tag.c: use ref transactions when doing updates - refs.c: add transaction.status and track OPEN/CLOSED/ERROR - refs.c: make ref_transaction_begin take an err argument - refs.c: update ref_transaction_delete to check for error and return status - refs.c: change ref_transaction_create to do error checking and return status (this branch is used by rs/ref-transaction, rs/ref-transaction-multi, rs/ref-transaction-reflog and rs/ref-transaction-rename.) The second batch of the transactional ref update series. Has been merged into pu rs/ref-transaction (2014-07-17) 12 commits ----------------------------------------------------------------- - refs.c: fix handling of badly named refs - refs.c: make write_ref_sha1 static - fetch.c: change s_update_ref to use a ref transaction - refs.c: propagate any errno==ENOTDIR from _commit back to the callers - refs.c: pass a skip list to name_conflict_fn - refs.c: call lock_ref_sha1_basic directly from commit - refs.c: move the check for valid refname to lock_ref_sha1_basic - refs.c: pass NULL as *flags to read_ref_full - refs.c: pass the ref log message to _create/delete/update instead of _commit - refs.c: add an err argument to delete_ref_loose - wrapper.c: add a new function unlink_or_msg - wrapper.c: simplify warn_if_unremovable (this branch is used by rs/ref-transaction-multi, rs/ref-transaction-reflog and rs/ref-transaction-rename; uses rs/ref-transaction-1.) The third and final part of the basic ref-transaction work. Has been merged into pu. rs/ref-transaction-reflog (2014-07-23) 15 commits ----------------------------------------------------------------------- - refs.c: allow deleting refs with a broken sha1 - refs.c: remove lock_any_ref_for_update - refs.c: make unlock_ref/close_ref/commit_ref static - refs.c: rename log_ref_setup to create_reflog - reflog.c: use a reflog transaction when writing during expire - refs.c: allow multiple reflog updates during a single transaction - refs.c: only write reflog update if msg is non-NULL - refs.c: add a flag to allow reflog updates to truncate the log - refs.c: add a transaction function to append a reflog entry - lockfile.c: make hold_lock_file_for_append preserve meaningful errno - refs.c: add a function to append a reflog entry to a fd - refs.c: add a new update_type field to ref_update - refs.c: rename the transaction functions - refs.c: make ref_transaction_delete a wrapper for ref_transaction_update - refs.c: make ref_transaction_create a wrapper to ref_transaction_update (this branch is used by rs/ref-transaction-multi and rs/ref-transaction-rename; uses rs/ref-transaction and rs/ref-transaction-1.) This patch series adds support for reflog updates to the transaction subsystem. Once it has refactored the builtin/reflog.c to use a transaction instead of accessing the refs and reflogs directly it allows us to remove unlock_ref/close_ref/commit_ref/lock_any_ref_for_update from the public API. As part of the reflog work I also refactor how reflog creation works renaming log_ref_setup to create_reflog. These changes allow us then to reduce the number of places where we expose the global log_all_ref_updates to the callers. This leads to a much cleaner api for reflog creation and avoids completely the "manipulate the global log_all_ref_updates before calling log_ref_setup" thing that was done in checkout.c. Now checkout.c can just call create_reflog() and the right thing will happen. At the end of the patch series is an unrelated patch for repairing the "allow deletion of a broken ref" functionality that was lost a while back. This patch is independent of the reflog work but is placed here just to avoid creating additional conflicts with this series and series that follow. Has been merged to pu. Following are the patch series that has not yet made its way into pu. ref-transactions-rename ----------------------------------- refs.c: allow passing raw git_committer_info as email to _update_reflog refs.c: return error instead of dying when locking fails during transaction refs.c: use packed refs when deleting refs during a transaction refs.c: update rename_ref to use a transaction refs.c: rollback the lockfile before we die() in repack_without_refs This series focuses on reworking how rename_ref works and to make it use an atomic transaction for the rename. The first three patches lead up to a point where we can now perform the "delete the old ref and add the new ref" as a single atomic operation to the packed refs file followed by the fourth patch where we rework rename_ref() completely to perform the rename and the reflog updates as a single atomic transaction. As part of this the rename_ref logic is greatly simplified and we reach several nice goals: * no need to disallow rename_ref() if the reflog is a symlink * no need to rename the reflog via a third filename * make the rename become completely atomic both for the caller but also for any external observers. The fifth patch is independent to the rename work itself but was discovered during the work for this patch. It could be applied separately but I did not see any urgency to push it separetely and left it as a patch in this series. ref-transactions-req-packed-refs ---------------------------------------------- refs.c: move reflog updates into its own function refs.c: write updates to packed refs when a transaction has more than one ref remote.c: use a transaction for deleting refs refs.c: make repack_without_refs static refs.c: make the *_packed_refs functions static These patches expands on the "perform ref operations as a packed refs file commit" and changes the transaction subsystem to always perform multi-ref updates as a packed ref commit instead of discreete operations on loose refs. The new logic is basically: IF the transaction only touches a single ref, then just do the normal loose ref operation as before. This is to make sure that the very common "git commit" command will still always be fast. IF the transaction touches multiple refs, then * first copy/move all affected refs to the packed refs file and commit, then re-lock the packed refs file until the transaction has completed. * then delete any affected loose refs files (they are already in the packed refs file) * perform the operation by adding/removing/updating packed refs entries * finally, commit the packed refs file This leads to several new nice properties. First, the transaction subsystem will now do the "right thing" and automatically convert any multi-ref updates to use the packed refs file. This then means that we no longer need to have special casing for clone.c to use a special packed_refs api. Clone.c can now just use a transaction for its "add all refs" and the right thing will happen. Similar optimizations can be done for remote.c which now also can just use a transaction and know that the right thing will happen. As we now no longer need to expose the special purpose packed-refs api to external callers we can now make all the packed refs functions private to refs.c and shrink the size of the public api. Note: we still do have packed refs and they still work the same way as before. It is just that the packed refs are now an internal optimization for the refs code and no longer part of the public api. Second, all transaction that affects multiple refs will now also become atomic for any external observer regardless of whether they used the packed refs api or not. The following functions are now static to refs.c and no longer part of the public api: add_packed_ref/lock_packed_refs/commit_packed_refs/rollback_packed_refs/repack_without_refs ref-transactions-req-strbuf-err ------------------------------------------- This series just contains various changes internally to refs.c to expand on the use of a strbuf *err as argument for passing error messages back to the caller. Only thing by note here is tgat by doing this to update_ref() we can finally get rid of the action_on_err enum: -enum action_on_err { - UPDATE_REFS_MSG_ON_ERR, - UPDATE_REFS_DIE_ON_ERR, - UPDATE_REFS_QUIET_ON_ERR -}; wooohooo As part of this patch series I noticed that there were two places in transport*.c where we were calling update_ref() and passing '0' as argument instead of UPDATE_REFS_MSG_ON_ERR. Not a bug and not a change in functionality but it would be nice if the compiler could warn for this. ref-transactions-send-pack --------------------------------------- Various changes to allow a new argument --atomic-push, and protocol changes, to allow a client to request/negotiate that a multi-ref push should be atomic. I.e. apply all changes or none. Not sent to the list yet. ref-transactions-send-pack-post ---------------------------------------------- Various other changes to add a strbuf *err argument to error handling. Not sent to the list yet. And that concludes the basic ref-transaction stuff. Once this is in I have a very large patch series that is mainly rearranging code and to add support for pluggable ref backends. But that is for when the ref transaction patches are in. regards ronnie sahlberg ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Transaction patch series overview 2014-07-30 17:10 Transaction patch series overview Ronnie Sahlberg @ 2014-07-31 21:41 ` Ronnie Sahlberg 2014-08-08 16:50 ` Ronnie Sahlberg 0 siblings, 1 reply; 25+ messages in thread From: Ronnie Sahlberg @ 2014-07-31 21:41 UTC (permalink / raw) To: git List, please see here an overview and ordering of the ref transaction patch series. These series build on each other and needs to be applied in the order listed below. This is an update. rs/ref-transaction-0 --------------------------- Early part of the "ref transaction" topic. * rs/ref-transaction-0: refs.c: change ref_transaction_update() to do error checking and return status refs.c: remove the onerr argument to ref_transaction_commit update-ref: use err argument to get error from ref_transaction_commit refs.c: make update_ref_write update a strbuf on failure refs.c: make ref_update_reject_duplicates take a strbuf argument for errors refs.c: log_ref_write should try to return meaningful errno refs.c: make resolve_ref_unsafe set errno to something meaningful on error refs.c: commit_packed_refs to return a meaningful errno on failure refs.c: make remove_empty_directories always set errno to something sane refs.c: verify_lock should set errno to something meaningful refs.c: make sure log_ref_setup returns a meaningful errno refs.c: add an err argument to repack_without_refs lockfile.c: make lock_file return a meaningful errno on failurei lockfile.c: add a new public function unable_to_lock_message refs.c: add a strbuf argument to ref_transaction_commit for error logging refs.c: allow passing NULL to ref_transaction_free refs.c: constify the sha arguments for ref_transaction_create|delete|update refs.c: ref_transaction_commit should not free the transaction refs.c: remove ref_transaction_rollback Has been merged into next. ref-transaction-1 (2014-07-16) 20 commits ------------------------------------------------------------- Second batch of ref transactions - refs.c: make delete_ref use a transaction - refs.c: make prune_ref use a transaction to delete the ref - refs.c: remove lock_ref_sha1 - refs.c: remove the update_ref_write function - refs.c: remove the update_ref_lock function - refs.c: make lock_ref_sha1 static - walker.c: use ref transaction for ref updates - fast-import.c: use a ref transaction when dumping tags - receive-pack.c: use a reference transaction for updating the refs - refs.c: change update_ref to use a transaction - branch.c: use ref transaction for all ref updates - fast-import.c: change update_branch to use ref transactions - sequencer.c: use ref transactions for all ref updates - commit.c: use ref transactions for updates - replace.c: use the ref transaction functions for updates - tag.c: use ref transactions when doing updates - refs.c: add transaction.status and track OPEN/CLOSED/ERROR - refs.c: make ref_transaction_begin take an err argument - refs.c: update ref_transaction_delete to check for error and return status - refs.c: change ref_transaction_create to do error checking and return status (this branch is used by rs/ref-transaction, rs/ref-transaction-multi, rs/ref-transaction-reflog and rs/ref-transaction-rename.) The second batch of the transactional ref update series. Has been merged into pu rs/ref-transaction (2014-07-17) 12 commits ----------------------------------------------------------------- - refs.c: fix handling of badly named refs - refs.c: make write_ref_sha1 static - fetch.c: change s_update_ref to use a ref transaction - refs.c: propagate any errno==ENOTDIR from _commit back to the callers - refs.c: pass a skip list to name_conflict_fn - refs.c: call lock_ref_sha1_basic directly from commit - refs.c: move the check for valid refname to lock_ref_sha1_basic - refs.c: pass NULL as *flags to read_ref_full - refs.c: pass the ref log message to _create/delete/update instead of _commit - refs.c: add an err argument to delete_ref_loose - wrapper.c: add a new function unlink_or_msg - wrapper.c: simplify warn_if_unremovable (this branch is used by rs/ref-transaction-multi, rs/ref-transaction-reflog and rs/ref-transaction-rename; uses rs/ref-transaction-1.) The third and final part of the basic ref-transaction work. Has been merged into pu. rs/ref-transaction-reflog (2014-07-23) 15 commits ----------------------------------------------------------------------- - refs.c: allow deleting refs with a broken sha1 - refs.c: remove lock_any_ref_for_update - refs.c: make unlock_ref/close_ref/commit_ref static - refs.c: rename log_ref_setup to create_reflog - reflog.c: use a reflog transaction when writing during expire - refs.c: allow multiple reflog updates during a single transaction - refs.c: only write reflog update if msg is non-NULL - refs.c: add a flag to allow reflog updates to truncate the log - refs.c: add a transaction function to append a reflog entry - lockfile.c: make hold_lock_file_for_append preserve meaningful errno - refs.c: add a function to append a reflog entry to a fd - refs.c: add a new update_type field to ref_update - refs.c: rename the transaction functions - refs.c: make ref_transaction_delete a wrapper for ref_transaction_update - refs.c: make ref_transaction_create a wrapper to ref_transaction_update (this branch is used by rs/ref-transaction-multi and rs/ref-transaction-rename; uses rs/ref-transaction and rs/ref-transaction-1.) This patch series adds support for reflog updates to the transaction subsystem. Once it has refactored the builtin/reflog.c to use a transaction instead of accessing the refs and reflogs directly it allows us to remove unlock_ref/close_ref/commit_ref/lock_any_ref_for_update from the public API. As part of the reflog work I also refactor how reflog creation works renaming log_ref_setup to create_reflog. These changes allow us then to reduce the number of places where we expose the global log_all_ref_updates to the callers. This leads to a much cleaner api for reflog creation and avoids completely the "manipulate the global log_all_ref_updates before calling log_ref_setup" thing that was done in checkout.c. Now checkout.c can just call create_reflog() and the right thing will happen. At the end of the patch series is an unrelated patch for repairing the "allow deletion of a broken ref" functionality that was lost a while back. This patch is independent of the reflog work but is placed here just to avoid creating additional conflicts with this series and series that follow. Has been merged to pu. ref-transactions-rename ----------------------------------- refs.c: allow passing raw git_committer_info as email to _update_reflog refs.c: return error instead of dying when locking fails during transaction refs.c: use packed refs when deleting refs during a transaction refs.c: update rename_ref to use a transaction refs.c: rollback the lockfile before we die() in repack_without_refs This series focuses on reworking how rename_ref works and to make it use an atomic transaction for the rename. The first three patches lead up to a point where we can now perform the "delete the old ref and add the new ref" as a single atomic operation to the packed refs file followed by the fourth patch where we rework rename_ref() completely to perform the rename and the reflog updates as a single atomic transaction. As part of this the rename_ref logic is greatly simplified and we reach several nice goals: * no need to disallow rename_ref() if the reflog is a symlink * no need to rename the reflog via a third filename * make the rename become completely atomic both for the caller but also for any external observers. The fifth patch is independent to the rename work itself but was discovered during the work for this patch. It could be applied separately but I did not see any urgency to push it separetely and left it as a patch in this series. Has been merged into pu. ref-transactions-req-packed-refs ---------------------------------------------- refs.c: move reflog updates into its own function refs.c: write updates to packed refs when a transaction has more than one ref remote.c: use a transaction for deleting refs refs.c: make repack_without_refs static refs.c: make the *_packed_refs functions static These patches expands on the "perform ref operations as a packed refs file commit" and changes the transaction subsystem to always perform multi-ref updates as a packed ref commit instead of discreete operations on loose refs. The new logic is basically: IF the transaction only touches a single ref, then just do the normal loose ref operation as before. This is to make sure that the very common "git commit" command will still always be fast. IF the transaction touches multiple refs, then * first copy/move all affected refs to the packed refs file and commit, then re-lock the packed refs file until the transaction has completed. * then delete any affected loose refs files (they are already in the packed refs file) * perform the operation by adding/removing/updating packed refs entries * finally, commit the packed refs file This leads to several new nice properties. First, the transaction subsystem will now do the "right thing" and automatically convert any multi-ref updates to use the packed refs file. This then means that we no longer need to have special casing for clone.c to use a special packed_refs api. Clone.c can now just use a transaction for its "add all refs" and the right thing will happen. Similar optimizations can be done for remote.c which now also can just use a transaction and know that the right thing will happen. As we now no longer need to expose the special purpose packed-refs api to external callers we can now make all the packed refs functions private to refs.c and shrink the size of the public api. Note: we still do have packed refs and they still work the same way as before. It is just that the packed refs are now an internal optimization for the refs code and no longer part of the public api. Second, all transaction that affects multiple refs will now also become atomic for any external observer regardless of whether they used the packed refs api or not. The following functions are now static to refs.c and no longer part of the public api: add_packed_ref/lock_packed_refs/commit_packed_refs/rollback_packed_refs/repack_without_refs Has been merged into pu. Following are the patch series that has not yet made its way into pu. ref-transactions-req-strbuf-err ------------------------------------------- This series just contains various changes internally to refs.c to expand on the use of a strbuf *err as argument for passing error messages back to the caller. Only thing by note here is tgat by doing this to update_ref() we can finally get rid of the action_on_err enum: -enum action_on_err { - UPDATE_REFS_MSG_ON_ERR, - UPDATE_REFS_DIE_ON_ERR, - UPDATE_REFS_QUIET_ON_ERR -}; wooohooo As part of this patch series I noticed that there were two places in transport*.c where we were calling update_ref() and passing '0' as argument instead of UPDATE_REFS_MSG_ON_ERR. Not a bug and not a change in functionality but it would be nice if the compiler could warn for this. ref-transactions-send-pack --------------------------------------- Various changes to allow a new argument --atomic-push, and protocol changes, to allow a client to request/negotiate that a multi-ref push should be atomic. I.e. apply all changes or none. ref-transactions-send-pack-post ---------------------------------------------- Various other changes to add a strbuf *err argument to error handling. Not sent to the list yet. And that concludes the basic ref-transaction stuff. Once this is in I have a very large patch series that is mainly rearranging code and to add support for pluggable ref backends. But that is for when the ref transaction patches are in. regards ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Transaction patch series overview 2014-07-31 21:41 ` Ronnie Sahlberg @ 2014-08-08 16:50 ` Ronnie Sahlberg 2014-08-19 19:54 ` Ronnie Sahlberg 0 siblings, 1 reply; 25+ messages in thread From: Ronnie Sahlberg @ 2014-08-08 16:50 UTC (permalink / raw) To: git List, please see here an overview and ordering of the ref transaction patch series. These series build on each other and needs to be applied in the order listed below. This is an update. rs/ref-transaction-0 --------------------------- Early part of the "ref transaction" topic. * rs/ref-transaction-0: refs.c: change ref_transaction_update() to do error checking and return status refs.c: remove the onerr argument to ref_transaction_commit update-ref: use err argument to get error from ref_transaction_commit refs.c: make update_ref_write update a strbuf on failure refs.c: make ref_update_reject_duplicates take a strbuf argument for errors refs.c: log_ref_write should try to return meaningful errno refs.c: make resolve_ref_unsafe set errno to something meaningful on error refs.c: commit_packed_refs to return a meaningful errno on failure refs.c: make remove_empty_directories always set errno to something sane refs.c: verify_lock should set errno to something meaningful refs.c: make sure log_ref_setup returns a meaningful errno refs.c: add an err argument to repack_without_refs lockfile.c: make lock_file return a meaningful errno on failurei lockfile.c: add a new public function unable_to_lock_message refs.c: add a strbuf argument to ref_transaction_commit for error logging refs.c: allow passing NULL to ref_transaction_free refs.c: constify the sha arguments for ref_transaction_create|delete|update refs.c: ref_transaction_commit should not free the transaction refs.c: remove ref_transaction_rollback Has been merged into next. ref-transaction-1 (2014-07-16) 20 commits ------------------------------------------------------------- Second batch of ref transactions - refs.c: make delete_ref use a transaction - refs.c: make prune_ref use a transaction to delete the ref - refs.c: remove lock_ref_sha1 - refs.c: remove the update_ref_write function - refs.c: remove the update_ref_lock function - refs.c: make lock_ref_sha1 static - walker.c: use ref transaction for ref updates - fast-import.c: use a ref transaction when dumping tags - receive-pack.c: use a reference transaction for updating the refs - refs.c: change update_ref to use a transaction - branch.c: use ref transaction for all ref updates - fast-import.c: change update_branch to use ref transactions - sequencer.c: use ref transactions for all ref updates - commit.c: use ref transactions for updates - replace.c: use the ref transaction functions for updates - tag.c: use ref transactions when doing updates - refs.c: add transaction.status and track OPEN/CLOSED/ERROR - refs.c: make ref_transaction_begin take an err argument - refs.c: update ref_transaction_delete to check for error and return status - refs.c: change ref_transaction_create to do error checking and return status (this branch is used by rs/ref-transaction, rs/ref-transaction-multi, rs/ref-transaction-reflog and rs/ref-transaction-rename.) The second batch of the transactional ref update series. Has been merged into pu rs/ref-transaction (2014-07-17) 12 commits ----------------------------------------------------------------- - refs.c: fix handling of badly named refs - refs.c: make write_ref_sha1 static - fetch.c: change s_update_ref to use a ref transaction - refs.c: propagate any errno==ENOTDIR from _commit back to the callers - refs.c: pass a skip list to name_conflict_fn - refs.c: call lock_ref_sha1_basic directly from commit - refs.c: move the check for valid refname to lock_ref_sha1_basic - refs.c: pass NULL as *flags to read_ref_full - refs.c: pass the ref log message to _create/delete/update instead of _commit - refs.c: add an err argument to delete_ref_loose - wrapper.c: add a new function unlink_or_msg - wrapper.c: simplify warn_if_unremovable (this branch is used by rs/ref-transaction-multi, rs/ref-transaction-reflog and rs/ref-transaction-rename; uses rs/ref-transaction-1.) The third and final part of the basic ref-transaction work. Has been merged into pu. rs/ref-transaction-reflog (2014-07-23) 15 commits ----------------------------------------------------------------------- - refs.c: allow deleting refs with a broken sha1 - refs.c: remove lock_any_ref_for_update - refs.c: make unlock_ref/close_ref/commit_ref static - refs.c: rename log_ref_setup to create_reflog - reflog.c: use a reflog transaction when writing during expire - refs.c: allow multiple reflog updates during a single transaction - refs.c: only write reflog update if msg is non-NULL - refs.c: add a flag to allow reflog updates to truncate the log - refs.c: add a transaction function to append a reflog entry - lockfile.c: make hold_lock_file_for_append preserve meaningful errno - refs.c: add a function to append a reflog entry to a fd - refs.c: add a new update_type field to ref_update - refs.c: rename the transaction functions - refs.c: make ref_transaction_delete a wrapper for ref_transaction_update - refs.c: make ref_transaction_create a wrapper to ref_transaction_update (this branch is used by rs/ref-transaction-multi and rs/ref-transaction-rename; uses rs/ref-transaction and rs/ref-transaction-1.) This patch series adds support for reflog updates to the transaction subsystem. Once it has refactored the builtin/reflog.c to use a transaction instead of accessing the refs and reflogs directly it allows us to remove unlock_ref/close_ref/commit_ref/lock_any_ref_for_update from the public API. As part of the reflog work I also refactor how reflog creation works renaming log_ref_setup to create_reflog. These changes allow us then to reduce the number of places where we expose the global log_all_ref_updates to the callers. This leads to a much cleaner api for reflog creation and avoids completely the "manipulate the global log_all_ref_updates before calling log_ref_setup" thing that was done in checkout.c. Now checkout.c can just call create_reflog() and the right thing will happen. At the end of the patch series is an unrelated patch for repairing the "allow deletion of a broken ref" functionality that was lost a while back. This patch is independent of the reflog work but is placed here just to avoid creating additional conflicts with this series and series that follow. Has been merged to pu. ref-transactions-rename ----------------------------------- refs.c: allow passing raw git_committer_info as email to _update_reflog refs.c: return error instead of dying when locking fails during transaction refs.c: use packed refs when deleting refs during a transaction refs.c: update rename_ref to use a transaction refs.c: rollback the lockfile before we die() in repack_without_refs This series focuses on reworking how rename_ref works and to make it use an atomic transaction for the rename. The first three patches lead up to a point where we can now perform the "delete the old ref and add the new ref" as a single atomic operation to the packed refs file followed by the fourth patch where we rework rename_ref() completely to perform the rename and the reflog updates as a single atomic transaction. As part of this the rename_ref logic is greatly simplified and we reach several nice goals: * no need to disallow rename_ref() if the reflog is a symlink * no need to rename the reflog via a third filename * make the rename become completely atomic both for the caller but also for any external observers. The fifth patch is independent to the rename work itself but was discovered during the work for this patch. It could be applied separately but I did not see any urgency to push it separetely and left it as a patch in this series. Has been merged into pu. ref-transactions-req-packed-refs ---------------------------------------------- refs.c: move reflog updates into its own function refs.c: write updates to packed refs when a transaction has more than one ref remote.c: use a transaction for deleting refs refs.c: make repack_without_refs static refs.c: make the *_packed_refs functions static These patches expands on the "perform ref operations as a packed refs file commit" and changes the transaction subsystem to always perform multi-ref updates as a packed ref commit instead of discreete operations on loose refs. The new logic is basically: IF the transaction only touches a single ref, then just do the normal loose ref operation as before. This is to make sure that the very common "git commit" command will still always be fast. IF the transaction touches multiple refs, then * first copy/move all affected refs to the packed refs file and commit, then re-lock the packed refs file until the transaction has completed. * then delete any affected loose refs files (they are already in the packed refs file) * perform the operation by adding/removing/updating packed refs entries * finally, commit the packed refs file This leads to several new nice properties. First, the transaction subsystem will now do the "right thing" and automatically convert any multi-ref updates to use the packed refs file. This then means that we no longer need to have special casing for clone.c to use a special packed_refs api. Clone.c can now just use a transaction for its "add all refs" and the right thing will happen. Similar optimizations can be done for remote.c which now also can just use a transaction and know that the right thing will happen. As we now no longer need to expose the special purpose packed-refs api to external callers we can now make all the packed refs functions private to refs.c and shrink the size of the public api. Note: we still do have packed refs and they still work the same way as before. It is just that the packed refs are now an internal optimization for the refs code and no longer part of the public api. Second, all transaction that affects multiple refs will now also become atomic for any external observer regardless of whether they used the packed refs api or not. The following functions are now static to refs.c and no longer part of the public api: add_packed_ref/lock_packed_refs/commit_packed_refs/rollback_packed_refs/repack_without_refs Has been merged into pu. Following are the patch series that has not yet made its way into pu. ref-transactions-req-strbuf-err ------------------------------------------- This series just contains various changes internally to refs.c to expand on the use of a strbuf *err as argument for passing error messages back to the caller. Only thing by note here is tgat by doing this to update_ref() we can finally get rid of the action_on_err enum: -enum action_on_err { - UPDATE_REFS_MSG_ON_ERR, - UPDATE_REFS_DIE_ON_ERR, - UPDATE_REFS_QUIET_ON_ERR -}; wooohooo As part of this patch series I noticed that there were two places in transport*.c where we were calling update_ref() and passing '0' as argument instead of UPDATE_REFS_MSG_ON_ERR. Not a bug and not a change in functionality but it would be nice if the compiler could warn for this. Sent to the list. ref-transactions-send-pack --------------------------------------- Various changes to allow a new argument --atomic-push, and protocol changes, to allow a client to request/negotiate that a multi-ref push should be atomic. I.e. apply all changes or none. Sent to the list. backend-struct-db -------------------------- Various changes that mainly moves functions between files to split the current refs.c file into functions that are backend agnostic and the functions that implement the files based ref backend. It finally adds a backend structure and the methods that define a backend datastore for refs. This series concludes the work to define pluggable backends for refs. At this stage it will now be possible to start building and adding new types of ref backends to git. Sent to the list regards ronnie sahlberg ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Transaction patch series overview 2014-08-08 16:50 ` Ronnie Sahlberg @ 2014-08-19 19:54 ` Ronnie Sahlberg 2014-08-20 23:17 ` Jonathan Nieder 0 siblings, 1 reply; 25+ messages in thread From: Ronnie Sahlberg @ 2014-08-19 19:54 UTC (permalink / raw) To: git List, please see here an overview and ordering of the ref transaction patch series. These series build on each other and needs to be applied in the order listed below. This is an update. rs/ref-transaction-0 --------------------------- Early part of the "ref transaction" topic. * rs/ref-transaction-0: refs.c: change ref_transaction_update() to do error checking and return status refs.c: remove the onerr argument to ref_transaction_commit update-ref: use err argument to get error from ref_transaction_commit refs.c: make update_ref_write update a strbuf on failure refs.c: make ref_update_reject_duplicates take a strbuf argument for errors refs.c: log_ref_write should try to return meaningful errno refs.c: make resolve_ref_unsafe set errno to something meaningful on error refs.c: commit_packed_refs to return a meaningful errno on failure refs.c: make remove_empty_directories always set errno to something sane refs.c: verify_lock should set errno to something meaningful refs.c: make sure log_ref_setup returns a meaningful errno refs.c: add an err argument to repack_without_refs lockfile.c: make lock_file return a meaningful errno on failurei lockfile.c: add a new public function unable_to_lock_message refs.c: add a strbuf argument to ref_transaction_commit for error logging refs.c: allow passing NULL to ref_transaction_free refs.c: constify the sha arguments for ref_transaction_create|delete|update refs.c: ref_transaction_commit should not free the transaction refs.c: remove ref_transaction_rollback Has been merged into next. ref-transaction-1 (2014-07-16) 20 commits ------------------------------------------------------------- Second batch of ref transactions - refs.c: make delete_ref use a transaction - refs.c: make prune_ref use a transaction to delete the ref - refs.c: remove lock_ref_sha1 - refs.c: remove the update_ref_write function - refs.c: remove the update_ref_lock function - refs.c: make lock_ref_sha1 static - walker.c: use ref transaction for ref updates - fast-import.c: use a ref transaction when dumping tags - receive-pack.c: use a reference transaction for updating the refs - refs.c: change update_ref to use a transaction - branch.c: use ref transaction for all ref updates - fast-import.c: change update_branch to use ref transactions - sequencer.c: use ref transactions for all ref updates - commit.c: use ref transactions for updates - replace.c: use the ref transaction functions for updates - tag.c: use ref transactions when doing updates - refs.c: add transaction.status and track OPEN/CLOSED/ERROR - refs.c: make ref_transaction_begin take an err argument - refs.c: update ref_transaction_delete to check for error and return status - refs.c: change ref_transaction_create to do error checking and return status (this branch is used by rs/ref-transaction, rs/ref-transaction-multi, rs/ref-transaction-reflog and rs/ref-transaction-rename.) The second batch of the transactional ref update series. Has been merged into pu rs/ref-transaction (2014-07-17) 12 commits ----------------------------------------------------------------- - refs.c: fix handling of badly named refs - refs.c: make write_ref_sha1 static - fetch.c: change s_update_ref to use a ref transaction - refs.c: propagate any errno==ENOTDIR from _commit back to the callers - refs.c: pass a skip list to name_conflict_fn - refs.c: call lock_ref_sha1_basic directly from commit - refs.c: move the check for valid refname to lock_ref_sha1_basic - refs.c: pass NULL as *flags to read_ref_full - refs.c: pass the ref log message to _create/delete/update instead of _commit - refs.c: add an err argument to delete_ref_loose - wrapper.c: add a new function unlink_or_msg - wrapper.c: simplify warn_if_unremovable (this branch is used by rs/ref-transaction-multi, rs/ref-transaction-reflog and rs/ref-transaction-rename; uses rs/ref-transaction-1.) The third and final part of the basic ref-transaction work. Has been merged into pu. rs/ref-transaction-reflog (2014-07-23) 15 commits ----------------------------------------------------------------------- - refs.c: allow deleting refs with a broken sha1 - refs.c: remove lock_any_ref_for_update - refs.c: make unlock_ref/close_ref/commit_ref static - refs.c: rename log_ref_setup to create_reflog - reflog.c: use a reflog transaction when writing during expire - refs.c: allow multiple reflog updates during a single transaction - refs.c: only write reflog update if msg is non-NULL - refs.c: add a flag to allow reflog updates to truncate the log - refs.c: add a transaction function to append a reflog entry - lockfile.c: make hold_lock_file_for_append preserve meaningful errno - refs.c: add a function to append a reflog entry to a fd - refs.c: add a new update_type field to ref_update - refs.c: rename the transaction functions - refs.c: make ref_transaction_delete a wrapper for ref_transaction_update - refs.c: make ref_transaction_create a wrapper to ref_transaction_update (this branch is used by rs/ref-transaction-multi and rs/ref-transaction-rename; uses rs/ref-transaction and rs/ref-transaction-1.) This patch series adds support for reflog updates to the transaction subsystem. Once it has refactored the builtin/reflog.c to use a transaction instead of accessing the refs and reflogs directly it allows us to remove unlock_ref/close_ref/commit_ref/lock_any_ref_for_update from the public API. As part of the reflog work I also refactor how reflog creation works renaming log_ref_setup to create_reflog. These changes allow us then to reduce the number of places where we expose the global log_all_ref_updates to the callers. This leads to a much cleaner api for reflog creation and avoids completely the "manipulate the global log_all_ref_updates before calling log_ref_setup" thing that was done in checkout.c. Now checkout.c can just call create_reflog() and the right thing will happen. At the end of the patch series is an unrelated patch for repairing the "allow deletion of a broken ref" functionality that was lost a while back. This patch is independent of the reflog work but is placed here just to avoid creating additional conflicts with this series and series that follow. Has been merged to pu. ref-transactions-rename ----------------------------------- refs.c: allow passing raw git_committer_info as email to _update_reflog refs.c: return error instead of dying when locking fails during transaction refs.c: use packed refs when deleting refs during a transaction refs.c: update rename_ref to use a transaction refs.c: rollback the lockfile before we die() in repack_without_refs This series focuses on reworking how rename_ref works and to make it use an atomic transaction for the rename. The first three patches lead up to a point where we can now perform the "delete the old ref and add the new ref" as a single atomic operation to the packed refs file followed by the fourth patch where we rework rename_ref() completely to perform the rename and the reflog updates as a single atomic transaction. As part of this the rename_ref logic is greatly simplified and we reach several nice goals: * no need to disallow rename_ref() if the reflog is a symlink * no need to rename the reflog via a third filename * make the rename become completely atomic both for the caller but also for any external observers. The fifth patch is independent to the rename work itself but was discovered during the work for this patch. It could be applied separately but I did not see any urgency to push it separetely and left it as a patch in this series. Has been merged into pu. ref-transactions-req-packed-refs ---------------------------------------------- refs.c: move reflog updates into its own function refs.c: write updates to packed refs when a transaction has more than one ref remote.c: use a transaction for deleting refs refs.c: make repack_without_refs static refs.c: make the *_packed_refs functions static These patches expands on the "perform ref operations as a packed refs file commit" and changes the transaction subsystem to always perform multi-ref updates as a packed ref commit instead of discreete operations on loose refs. The new logic is basically: IF the transaction only touches a single ref, then just do the normal loose ref operation as before. This is to make sure that the very common "git commit" command will still always be fast. IF the transaction touches multiple refs, then * first copy/move all affected refs to the packed refs file and commit, then re-lock the packed refs file until the transaction has completed. * then delete any affected loose refs files (they are already in the packed refs file) * perform the operation by adding/removing/updating packed refs entries * finally, commit the packed refs file This leads to several new nice properties. First, the transaction subsystem will now do the "right thing" and automatically convert any multi-ref updates to use the packed refs file. This then means that we no longer need to have special casing for clone.c to use a special packed_refs api. Clone.c can now just use a transaction for its "add all refs" and the right thing will happen. Similar optimizations can be done for remote.c which now also can just use a transaction and know that the right thing will happen. As we now no longer need to expose the special purpose packed-refs api to external callers we can now make all the packed refs functions private to refs.c and shrink the size of the public api. Note: we still do have packed refs and they still work the same way as before. It is just that the packed refs are now an internal optimization for the refs code and no longer part of the public api. Second, all transaction that affects multiple refs will now also become atomic for any external observer regardless of whether they used the packed refs api or not. The following functions are now static to refs.c and no longer part of the public api: add_packed_ref/lock_packed_refs/commit_packed_refs/rollback_packed_refs/repack_without_refs Has been merged into pu. Following are the patch series that has not yet made its way into pu. ref-transactions-req-strbuf-err ------------------------------------------- This series just contains various changes internally to refs.c to expand on the use of a strbuf *err as argument for passing error messages back to the caller. Only thing by note here is tgat by doing this to update_ref() we can finally get rid of the action_on_err enum: -enum action_on_err { - UPDATE_REFS_MSG_ON_ERR, - UPDATE_REFS_DIE_ON_ERR, - UPDATE_REFS_QUIET_ON_ERR -}; wooohooo As part of this patch series I noticed that there were two places in transport*.c where we were calling update_ref() and passing '0' as argument instead of UPDATE_REFS_MSG_ON_ERR. Not a bug and not a change in functionality but it would be nice if the compiler could warn for this. Sent to the list. ref-transactions-send-pack --------------------------------------- Various changes to allow a new argument --atomic-push, and protocol changes, to allow a client to request/negotiate that a multi-ref push should be atomic. I.e. apply all changes or none. Sent to the list. backend-struct-db -------------------------- Various changes that mainly moves functions between files to split the current refs.c file into functions that are backend agnostic and the functions that implement the files based ref backend. It finally adds a backend structure and the methods that define a backend datastore for refs. This series concludes the work to define pluggable backends for refs. At this stage it will now be possible to start building and adding new types of ref backends to git. Sent to the list In addition to these patches, there is one more patchseries with 4 patches at : https://github.com/rsahlberg/git/tree/backend-struct-db-2 This series is for once the previous transaction changes are in and this series will add a new backend for refs: refs-be-db.c which offloads all refs access to an external database daemon. An example reference implementation for an external daemon is also provided and can be used as basis for creating a daemon to plug into, say, a SQL database or similar. regards ronnie sahlberg ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Transaction patch series overview 2014-08-19 19:54 ` Ronnie Sahlberg @ 2014-08-20 23:17 ` Jonathan Nieder 2014-08-26 0:03 ` Jonathan Nieder 0 siblings, 1 reply; 25+ messages in thread From: Jonathan Nieder @ 2014-08-20 23:17 UTC (permalink / raw) To: Ronnie Sahlberg; +Cc: git, Michael Haggerty Hi, Ronnie Sahlberg wrote: > List, please see here an overview and ordering of the ref transaction > patch series. Thanks much for this. [...] > rs/ref-transaction-0 [...] > Has been merged into next. This is even part of "master" now, so if people have review comments then they can make them most easily by submitting new patches on top. > ref-transaction-1 (2014-07-16) 20 commits > ------------------------------------------------------------- > Second batch of ref transactions > > - refs.c: make delete_ref use a transaction > - refs.c: make prune_ref use a transaction to delete the ref > - refs.c: remove lock_ref_sha1 > - refs.c: remove the update_ref_write function > - refs.c: remove the update_ref_lock function > - refs.c: make lock_ref_sha1 static > - walker.c: use ref transaction for ref updates > - fast-import.c: use a ref transaction when dumping tags > - receive-pack.c: use a reference transaction for updating the refs > - refs.c: change update_ref to use a transaction > - branch.c: use ref transaction for all ref updates > - fast-import.c: change update_branch to use ref transactions > - sequencer.c: use ref transactions for all ref updates > - commit.c: use ref transactions for updates > - replace.c: use the ref transaction functions for updates > - tag.c: use ref transactions when doing updates > - refs.c: add transaction.status and track OPEN/CLOSED/ERROR > - refs.c: make ref_transaction_begin take an err argument > - refs.c: update ref_transaction_delete to check for error and return status > - refs.c: change ref_transaction_create to do error checking and return status > (this branch is used by rs/ref-transaction, rs/ref-transaction-multi, > rs/ref-transaction-reflog and rs/ref-transaction-rename.) > > The second batch of the transactional ref update series. > > Has been merged into pu Last reviewed at http://thread.gmane.org/gmane.comp.version-control.git/252226, if I am using gmane search correctly. Are there any pending questions for this part? I've having trouble keeping track of how patches change, which patches have been reviewed and which haven't, unaddressed comments, and so on, so as an experiment I've pushed this part of the series to the Gerrit server at https://code-review.googlesource.com/#/q/topic:ref-transaction-1 It's running a copy of gerrit code review (https://code.google.com/p/gerrit). Maybe this can be useful as a way of keeping track of the state of long and long-lived series like this one. It works something like this: Preparation ----------- Get a password from https://code-review.googlesource.com/new-password and put it in netrc. Install the hook from https://code-review.googlesource.com/tools/hooks/commit-msg to .git/hooks/commit-msg. This automatically populates a Change-Id line in the commit message if none is present so gerrit can tell when you are uploading a new version of an existing patch. (The commit-msg hook can be annoying when not working with gerrit code review. To turn it off, use 'git config gerrit.createChangeId false'.) Uploading patches ----------------- Write a patch series against 'maint' or 'master' as usual, then push: git push https://code.googlesource.com/git \ HEAD:refs/for/master; # or refs/for/maint There can be a parameter '%topic=<name>' after the refs/for/<branch> (e.g., refs/for/master%topic=ref-transaction-1) if the series should be labelled with a topic name, or that can be set in the web UI or using the HTTP API after the fact. Commenting on patches --------------------- Visit https://code-review.googlesource.com. Patches can be found under "All" -> "Open". Patches you're involved with somehow are at "My" -> "Changes". To prepare inline comments, choose a file, highlight the text to comment on or click a line number, and comment. To publish comments, go back up to the change overview screen (using the up arrow button or by pressing "u"), click "Reply", and say something. '?' brings up keyboard shortcuts. Downloading patches ------------------- In the change overview screen, there's a 'Download' menu in the top-right corner with commands to download the patch. Revising patches ---------------- After modifying a patch locally using the usual tools such as rebase --interactive, push again: git push https://code.googlesource.com/git \ HEAD:refs/for/master; # or refs/for/maint When a patch seems polished --------------------------- Get rid of the Change-Id lines --- they shouldn't be needed any more. Send the patches or a request to pull from a public git repository, as usual. A link (in the top-left corner where it says 'Change 1000', the "1000" is a link to the change) can be helpful for letting people on-list see how the patch got into the form it's in today. Maybe it can be useful. Thanks, Jonathan ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Transaction patch series overview 2014-08-20 23:17 ` Jonathan Nieder @ 2014-08-26 0:03 ` Jonathan Nieder 2014-08-26 21:01 ` Junio C Hamano 0 siblings, 1 reply; 25+ messages in thread From: Jonathan Nieder @ 2014-08-26 0:03 UTC (permalink / raw) To: Ronnie Sahlberg; +Cc: git, Michael Haggerty Jonathan Nieder wrote: > Ronnie Sahlberg wrote: >> ref-transaction-1 (2014-07-16) 20 commits >> ------------------------------------------------------------- >> Second batch of ref transactions >> >> - refs.c: make delete_ref use a transaction >> - refs.c: make prune_ref use a transaction to delete the ref >> - refs.c: remove lock_ref_sha1 >> - refs.c: remove the update_ref_write function >> - refs.c: remove the update_ref_lock function >> - refs.c: make lock_ref_sha1 static >> - walker.c: use ref transaction for ref updates >> - fast-import.c: use a ref transaction when dumping tags >> - receive-pack.c: use a reference transaction for updating the refs >> - refs.c: change update_ref to use a transaction >> - branch.c: use ref transaction for all ref updates >> - fast-import.c: change update_branch to use ref transactions >> - sequencer.c: use ref transactions for all ref updates >> - commit.c: use ref transactions for updates >> - replace.c: use the ref transaction functions for updates >> - tag.c: use ref transactions when doing updates >> - refs.c: add transaction.status and track OPEN/CLOSED/ERROR >> - refs.c: make ref_transaction_begin take an err argument >> - refs.c: update ref_transaction_delete to check for error and return status >> - refs.c: change ref_transaction_create to do error checking and return status >> (this branch is used by rs/ref-transaction, rs/ref-transaction-multi, >> rs/ref-transaction-reflog and rs/ref-transaction-rename.) [...] > I've having trouble keeping track of how patches change, which patches > have been reviewed and which haven't, unaddressed comments, and so on, > so as an experiment I've pushed this part of the series to the Gerrit > server at > > https://code-review.googlesource.com/#/q/topic:ref-transaction-1 Outcome of the experiment: patches gained some minor changes and then 1-12 Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> 13 Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> 14 Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> 15-16 Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> 17 Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu> 18 Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> 19 Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> 20 Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> I found the web UI helpful in seeing how each patch evolved since I last looked at it. Interdiff relative to the version in pu is below. I'm still hoping for a tweak in response to a minor comment and then I can put up a copy of the updated series to pull. The next series from Ronnie's collection is available at https://code-review.googlesource.com/#/q/topic:ref-transaction in case someone wants a fresh series to look at. diff --git a/branch.c b/branch.c index c1eae00..37ac555 100644 --- a/branch.c +++ b/branch.c @@ -305,6 +305,7 @@ void create_branch(const char *head, ref_transaction_commit(transaction, msg, &err)) die("%s", err.buf); ref_transaction_free(transaction); + strbuf_release(&err); } if (real_ref && track) diff --git a/builtin/commit.c b/builtin/commit.c index 668fa6a..9bf1003 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1765,8 +1765,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix) transaction = ref_transaction_begin(&err); if (!transaction || ref_transaction_update(transaction, "HEAD", sha1, - current_head ? - current_head->object.sha1 : NULL, + current_head + ? current_head->object.sha1 : NULL, 0, !!current_head, &err) || ref_transaction_commit(transaction, sb.buf, &err)) { rollback_index_files(); @@ -1801,5 +1801,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix) if (!quiet) print_summary(prefix, sha1, !current_head); + strbuf_release(&err); return 0; } diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 91099ad..224fadc 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -194,7 +194,7 @@ static void write_head_info(void) struct command { struct command *next; - char *error_string; + const char *error_string; unsigned int skip_update:1, did_not_exist:1; int index; @@ -468,7 +468,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si) return 0; } -static char *update(struct command *cmd, struct shallow_info *si) +static const char *update(struct command *cmd, struct shallow_info *si) { const char *name = cmd->ref_name; struct strbuf namespaced_name_buf = STRBUF_INIT; @@ -479,7 +479,7 @@ static char *update(struct command *cmd, struct shallow_info *si) /* only refs/... are allowed */ if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) { rp_error("refusing to create funny ref '%s' remotely", name); - return xstrdup("funny refname"); + return "funny refname"; } strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name); @@ -497,20 +497,20 @@ static char *update(struct command *cmd, struct shallow_info *si) rp_error("refusing to update checked out branch: %s", name); if (deny_current_branch == DENY_UNCONFIGURED) refuse_unconfigured_deny(); - return xstrdup("branch is currently checked out"); + return "branch is currently checked out"; } } if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) { error("unpack should have generated %s, " "but I can't find it!", sha1_to_hex(new_sha1)); - return xstrdup("bad pack"); + return "bad pack"; } if (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) { if (deny_deletes && starts_with(name, "refs/heads/")) { rp_error("denying ref deletion for %s", name); - return xstrdup("deletion prohibited"); + return "deletion prohibited"; } if (!strcmp(namespaced_name, head_name)) { @@ -525,7 +525,7 @@ static char *update(struct command *cmd, struct shallow_info *si) if (deny_delete_current == DENY_UNCONFIGURED) refuse_unconfigured_deny_delete_current(); rp_error("refusing to delete the current branch: %s", name); - return xstrdup("deletion of the current branch prohibited"); + return "deletion of the current branch prohibited"; } } } @@ -543,19 +543,19 @@ static char *update(struct command *cmd, struct shallow_info *si) old_object->type != OBJ_COMMIT || new_object->type != OBJ_COMMIT) { error("bad sha1 objects for %s", name); - return xstrdup("bad ref"); + return "bad ref"; } old_commit = (struct commit *)old_object; new_commit = (struct commit *)new_object; if (!in_merge_bases(old_commit, new_commit)) { rp_error("denying non-fast-forward %s" " (you should pull first)", name); - return xstrdup("non-fast-forward"); + return "non-fast-forward"; } } if (run_update_hook(cmd)) { rp_error("hook declined to update %s", name); - return xstrdup("hook declined"); + return "hook declined"; } if (is_null_sha1(new_sha1)) { @@ -570,7 +570,7 @@ static char *update(struct command *cmd, struct shallow_info *si) } if (delete_ref(namespaced_name, old_sha1, 0)) { rp_error("failed to delete %s", name); - return xstrdup("failed to delete"); + return "failed to delete"; } return NULL; /* good */ } @@ -580,18 +580,18 @@ static char *update(struct command *cmd, struct shallow_info *si) if (shallow_update && si->shallow_ref[cmd->index] && update_shallow_ref(cmd, si)) - return xstrdup("shallow error"); + return "shallow error"; transaction = ref_transaction_begin(&err); if (!transaction || ref_transaction_update(transaction, namespaced_name, new_sha1, old_sha1, 0, 1, &err) || ref_transaction_commit(transaction, "push", &err)) { - char *str = strbuf_detach(&err, NULL); ref_transaction_free(transaction); - rp_error("%s", str); - return str; + rp_error("%s", err.buf); + strbuf_release(&err); + return "failed to update ref"; } ref_transaction_free(transaction); @@ -654,9 +654,6 @@ static void check_aliased_update(struct command *cmd, struct string_list *list) char cmd_oldh[41], cmd_newh[41], dst_oldh[41], dst_newh[41]; int flag; - if (cmd->error_string) - die("BUG: check_aliased_update called with failed cmd"); - strbuf_addf(&buf, "%s%s", get_git_namespace(), cmd->ref_name); dst_name = resolve_ref_unsafe(buf.buf, sha1, 0, &flag); strbuf_release(&buf); @@ -668,7 +665,7 @@ static void check_aliased_update(struct command *cmd, struct string_list *list) if (!dst_name) { rp_error("refusing update to broken symref '%s'", cmd->ref_name); cmd->skip_update = 1; - cmd->error_string = xstrdup("broken symref"); + cmd->error_string = "broken symref"; return; } @@ -694,9 +691,8 @@ static void check_aliased_update(struct command *cmd, struct string_list *list) cmd->ref_name, cmd_oldh, cmd_newh, dst_cmd->ref_name, dst_oldh, dst_newh); - cmd->error_string = xstrdup("inconsistent aliased update"); - free(dst_cmd->error_string); - dst_cmd->error_string = xstrdup("inconsistent aliased update"); + cmd->error_string = dst_cmd->error_string = + "inconsistent aliased update"; } static void check_aliased_updates(struct command *commands) @@ -744,9 +740,7 @@ static void set_connectivity_errors(struct command *commands, if (!check_everything_connected(command_singleton_iterator, 0, &singleton)) continue; - if (cmd->error_string) /* can't happen */ - continue; - cmd->error_string = xstrdup("missing necessary objects"); + cmd->error_string = "missing necessary objects"; } } @@ -783,9 +777,9 @@ static void reject_updates_to_hidden(struct command *commands) if (cmd->error_string || !ref_is_hidden(cmd->ref_name)) continue; if (is_null_sha1(cmd->new_sha1)) - cmd->error_string = xstrdup("deny deleting a hidden ref"); + cmd->error_string = "deny deleting a hidden ref"; else - cmd->error_string = xstrdup("deny updating a hidden ref"); + cmd->error_string = "deny updating a hidden ref"; } } @@ -799,11 +793,8 @@ static void execute_commands(struct command *commands, struct iterate_data data; if (unpacker_error) { - for (cmd = commands; cmd; cmd = cmd->next) { - if (cmd->error_string) /* can't happen */ - continue; - cmd->error_string = xstrdup("unpacker error"); - } + for (cmd = commands; cmd; cmd = cmd->next) + cmd->error_string = "unpacker error"; return; } @@ -816,9 +807,8 @@ static void execute_commands(struct command *commands, if (run_receive_hook(commands, "pre-receive", 0)) { for (cmd = commands; cmd; cmd = cmd->next) { - if (cmd->error_string) - continue; - cmd->error_string = xstrdup("pre-receive hook declined"); + if (!cmd->error_string) + cmd->error_string = "pre-receive hook declined"; } return; } @@ -1096,8 +1086,7 @@ static void update_shallow_info(struct command *commands, if (is_null_sha1(cmd->new_sha1)) continue; if (ref_status[cmd->index]) { - free(cmd->error_string); - cmd->error_string = xstrdup("shallow update not allowed"); + cmd->error_string = "shallow update not allowed"; cmd->skip_update = 1; } } @@ -1138,17 +1127,6 @@ static int delete_only(struct command *commands) return 1; } -static void free_commands(struct command *commands) -{ - while (commands) { - struct command *next = commands->next; - - free(commands->error_string); - free(commands); - commands = next; - } -} - int cmd_receive_pack(int argc, const char **argv, const char *prefix) { int advertise_refs = 0; @@ -1244,6 +1222,5 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) packet_flush(1); sha1_array_clear(&shallow); sha1_array_clear(&ref); - free_commands(commands); return 0; } diff --git a/builtin/replace.c b/builtin/replace.c index 7528f3d..1fcd06d 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -169,8 +169,7 @@ static int replace_object_sha1(const char *object_ref, transaction = ref_transaction_begin(&err); if (!transaction || - ref_transaction_update(transaction, ref, repl, prev, - 0, !is_null_sha1(prev), &err) || + ref_transaction_update(transaction, ref, repl, prev, 0, 1, &err) || ref_transaction_commit(transaction, NULL, &err)) die("%s", err.buf); diff --git a/builtin/tag.c b/builtin/tag.c index 1aa88a2..f3f172f 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -712,6 +712,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) if (force && !is_null_sha1(prev) && hashcmp(prev, object)) printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV)); + strbuf_release(&err); strbuf_release(&buf); strbuf_release(&ref); return 0; diff --git a/builtin/update-ref.c b/builtin/update-ref.c index c6ad0be..96a53b9 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -366,6 +366,8 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) if (read_stdin) { transaction = ref_transaction_begin(&err); + if (!transaction) + die("%s", err.buf); if (delete || no_deref || argc > 0) usage_with_options(git_update_ref_usage, options); if (end_null) @@ -374,6 +376,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) if (ref_transaction_commit(transaction, msg, &err)) die("%s", err.buf); ref_transaction_free(transaction); + strbuf_release(&err); return 0; } diff --git a/fast-import.c b/fast-import.c index a95e1be..e7f6e37 100644 --- a/fast-import.c +++ b/fast-import.c @@ -1716,6 +1716,7 @@ static int update_branch(struct branch *b) return -1; } ref_transaction_free(transaction); + strbuf_release(&err); return 0; } diff --git a/refs.c b/refs.c index 0017d9c..819e25f 100644 --- a/refs.c +++ b/refs.c @@ -2074,7 +2074,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) @@ -2401,6 +2404,7 @@ static void prune_ref(struct ref_to_prune *r) return; } ref_transaction_free(transaction); + strbuf_release(&err); try_remove_empty_parents(r->name); } @@ -2575,6 +2579,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt) return 1; } ref_transaction_free(transaction); + strbuf_release(&err); return 0; } @@ -3352,19 +3357,15 @@ struct ref_update { * Transaction states. * OPEN: The transaction is in a valid state and can accept new updates. * An OPEN transaction can be committed. - * CLOSED: If an open transaction is successfully committed the state will - * change to CLOSED. No further changes can be made to a CLOSED - * transaction. - * CLOSED means that all updates have been successfully committed and - * the only thing that remains is to free the completed transaction. - * ERROR: The transaction has failed and is no longer committable. - * No further changes can be made to a CLOSED transaction and it must - * be rolled back using transaction_free. + * 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, - REF_TRANSACTION_ERROR = 2, + REF_TRANSACTION_CLOSED = 1 }; /* @@ -3509,6 +3510,7 @@ int update_ref(const char *action, const char *refname, strbuf_release(&err); return 1; } + strbuf_release(&err); return 0; } @@ -3588,10 +3590,10 @@ int ref_transaction_commit(struct ref_transaction *transaction, msg); update->lock = NULL; /* freed by write_ref_sha1 */ if (ret) { - const char *str = "Cannot update the ref '%s'."; - if (err) - strbuf_addf(err, str, update->refname); + strbuf_addf(err, "Cannot update the " + "ref '%s'.", + update->refname); goto cleanup; } } @@ -3614,8 +3616,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, clear_loose_ref_cache(&ref_cache); cleanup: - transaction->state = ret ? REF_TRANSACTION_ERROR - : REF_TRANSACTION_CLOSED; + transaction->state = REF_TRANSACTION_CLOSED; for (i = 0; i < n; i++) if (updates[i]->lock) diff --git a/refs.h b/refs.h index aad846c..69ef28c 100644 --- a/refs.h +++ b/refs.h @@ -180,8 +180,7 @@ extern int peel_ref(const char *refname, unsigned char *sha1); */ #define REF_NODEREF 0x01 /* - * Locks any ref (for 'HEAD' type refs) and sets errno to something - * meaningful on failure. + * This function sets errno to something meaningful on failure. */ extern struct ref_lock *lock_any_ref_for_update(const char *refname, const unsigned char *old_sha1, diff --git a/sequencer.c b/sequencer.c index cf17c69..5e93b6a 100644 --- a/sequencer.c +++ b/sequencer.c @@ -296,6 +296,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from, } strbuf_release(&sb); + strbuf_release(&err); ref_transaction_free(transaction); return 0; } diff --git a/walker.c b/walker.c index 60d9f9e..b8a5441 100644 --- a/walker.c +++ b/walker.c @@ -251,12 +251,12 @@ void walker_targets_free(int targets, char **target, const char **write_ref) int walker_fetch(struct walker *walker, int targets, char **target, const char **write_ref, const char *write_ref_log_details) { - struct strbuf ref_name = STRBUF_INIT; + struct strbuf refname = STRBUF_INIT; struct strbuf err = STRBUF_INIT; struct ref_transaction *transaction = NULL; unsigned char *sha1 = xmalloc(targets * 20); char *msg = NULL; - int i; + int i, ret = -1; save_commit_buffer = 0; @@ -264,7 +264,7 @@ int walker_fetch(struct walker *walker, int targets, char **target, transaction = ref_transaction_begin(&err); if (!transaction) { error("%s", err.buf); - goto rollback_and_fail; + goto done; } } if (!walker->get_recover) @@ -273,15 +273,18 @@ int walker_fetch(struct walker *walker, int targets, char **target, for (i = 0; i < targets; i++) { if (interpret_target(walker, target[i], &sha1[20 * i])) { error("Could not interpret response from server '%s' as something to pull", target[i]); - goto rollback_and_fail; + goto done; } if (process(walker, lookup_unknown_object(&sha1[20 * i]))) - goto rollback_and_fail; + goto done; } if (loop(walker)) - goto rollback_and_fail; - + goto done; + if (!write_ref) { + ret = 0; + goto done; + } if (write_ref_log_details) { msg = xmalloc(strlen(write_ref_log_details) + 12); sprintf(msg, "fetch from %s", write_ref_log_details); @@ -289,37 +292,33 @@ int walker_fetch(struct walker *walker, int targets, char **target, msg = NULL; } for (i = 0; i < targets; i++) { - if (!write_ref || !write_ref[i]) + if (!write_ref[i]) continue; - strbuf_reset(&ref_name); - strbuf_addf(&ref_name, "refs/%s", write_ref[i]); - if (ref_transaction_update(transaction, ref_name.buf, + strbuf_reset(&refname); + strbuf_addf(&refname, "refs/%s", write_ref[i]); + if (ref_transaction_update(transaction, refname.buf, &sha1[20 * i], NULL, 0, 0, &err)) { error("%s", err.buf); - goto rollback_and_fail; + goto done; } } - if (write_ref) { - if (ref_transaction_commit(transaction, - msg ? msg : "fetch (unknown)", - &err)) { - error("%s", err.buf); - goto rollback_and_fail; - } - ref_transaction_free(transaction); + if (ref_transaction_commit(transaction, + msg ? msg : "fetch (unknown)", + &err)) { + error("%s", err.buf); + goto done; } - free(msg); - return 0; + ret = 0; -rollback_and_fail: +done: ref_transaction_free(transaction); free(msg); + free(sha1); strbuf_release(&err); - strbuf_release(&ref_name); - - return -1; + strbuf_release(&refname); + return ret; } void walker_free(struct walker *walker) ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: Transaction patch series overview 2014-08-26 0:03 ` Jonathan Nieder @ 2014-08-26 21:01 ` Junio C Hamano 2014-08-26 22:14 ` Jonathan Nieder 0 siblings, 1 reply; 25+ messages in thread From: Junio C Hamano @ 2014-08-26 21:01 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Ronnie Sahlberg, git, Michael Haggerty Jonathan Nieder <jrnieder@gmail.com> writes: >> https://code-review.googlesource.com/#/q/topic:ref-transaction-1 > > Outcome of the experiment: patches gained some minor changes and then > > 1-12 > Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> > > 13 > Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu> > Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> > > 14 > Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> > > 15-16 > Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu> > Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> > > 17 > Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu> > > 18 > Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu> > Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> > > 19 > Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> > > 20 > Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu> > Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> > > I found the web UI helpful in seeing how each patch evolved since I > last looked at it. Interdiff relative to the version in pu is below. Thanks for the interdiff, it really helps to be able to take a glance without having to click around. It seems that I can hold the topic in 'pu' a bit longer and expect a reroll from this effort before merging it to 'next'? > The next series from Ronnie's collection is available at > https://code-review.googlesource.com/#/q/topic:ref-transaction in case > someone wants a fresh series to look at. Thanks. > diff --git a/builtin/commit.c b/builtin/commit.c > index 668fa6a..9bf1003 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -1765,8 +1765,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > transaction = ref_transaction_begin(&err); > if (!transaction || > ref_transaction_update(transaction, "HEAD", sha1, > - current_head ? > - current_head->object.sha1 : NULL, > + current_head > + ? current_head->object.sha1 : NULL, > 0, !!current_head, &err) || > ref_transaction_commit(transaction, sb.buf, &err)) { > rollback_index_files(); Perhaps this is nicer, but probably most people would not care. > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c > index 91099ad..224fadc 100644 > --- a/builtin/receive-pack.c > +++ b/builtin/receive-pack.c > @@ -580,18 +580,18 @@ static char *update(struct command *cmd, struct shallow_info *si) > > if (shallow_update && si->shallow_ref[cmd->index] && > update_shallow_ref(cmd, si)) > - return xstrdup("shallow error"); > + return "shallow error"; > > transaction = ref_transaction_begin(&err); > if (!transaction || > ref_transaction_update(transaction, namespaced_name, > new_sha1, old_sha1, 0, 1, &err) || > ref_transaction_commit(transaction, "push", &err)) { > - char *str = strbuf_detach(&err, NULL); > ref_transaction_free(transaction); > > - rp_error("%s", str); > - return str; > + rp_error("%s", err.buf); > + strbuf_release(&err); > + return "failed to update ref"; > } I am guessing that the purpose of this change is to make sure that cmd->error_string is taken from a fixed set of strings, not an arbitrary collection of errors from the transaction subsystem, but what is the significance of doing so? Do the other side i18n while running print_ref_status() or something? > diff --git a/refs.h b/refs.h > index aad846c..69ef28c 100644 > --- a/refs.h > +++ b/refs.h > @@ -180,8 +180,7 @@ extern int peel_ref(const char *refname, unsigned char *sha1); > */ > #define REF_NODEREF 0x01 > /* > - * Locks any ref (for 'HEAD' type refs) and sets errno to something > - * meaningful on failure. > + * This function sets errno to something meaningful on failure. > */ > extern struct ref_lock *lock_any_ref_for_update(const char *refname, > const unsigned char *old_sha1, To contrast this function with lock_ref_sha1() that only allowed refs under refs/ hierarchy, the comment may have made sense, but now that other function is gone, losing the comment makes sense. I removed from the above the interdiff hunks I did not have any comments or questions on. Thanks again. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Transaction patch series overview 2014-08-26 21:01 ` Junio C Hamano @ 2014-08-26 22:14 ` Jonathan Nieder 2014-08-27 0:28 ` [PATCH 0/20] rs/ref-transaction-1 (Re: Transaction patch series overview) Jonathan Nieder 0 siblings, 1 reply; 25+ messages in thread From: Jonathan Nieder @ 2014-08-26 22:14 UTC (permalink / raw) To: Junio C Hamano; +Cc: Ronnie Sahlberg, git, Michael Haggerty Hi again, Junio C Hamano wrote: > It seems that I can hold the topic in 'pu' a bit longer and expect a > reroll from this effort before merging it to 'next'? Sorry for the delay. The following changes on top of "master" (refs.c: change ref_transaction_update() to do error checking and return status, 2014-07-14) are available in the git repository at git://repo.or.cz/git/jrn.git tags/rs/ref-transaction-1 for you to fetch changes up to 432ad1f39fd773b37757dd8f10b3075dc3d0d0a2: refs.c: make delete_ref use a transaction (2014-08-26 14:22:53 -0700) ---------------------------------------------------------------- "Use ref transactions", early part Ronnie explains: This patch series expands on the transaction API. It converts ref updates, inside refs.c as well as external, to use the transaction API for updates. This makes most ref updates atomic when there are failures locking or writing to a ref. This series teaches the tag, replace, commit, cherry-pick, fast-import, checkout -b, branch, receive-pack, and http-fetch commands and all update_ref and delete_ref callers to use the ref transaction API instead of lock_ref_sha1. The main user-visible change should be some cosmetic changes to error messages. The series also combines multiple ref updates into a single transaction in 'git http-fetch -w' and when writing tags in fast-import but otherwise doesn't change the granularity of transactions. Reviewed at https://code-review.googlesource.com/#/q/topic:ref-transaction-1 ---------------------------------------------------------------- Ronnie Sahlberg (20): refs.c: change ref_transaction_create to do error checking and return status refs.c: update ref_transaction_delete to check for error and return status refs.c: make ref_transaction_begin take an err argument refs.c: add transaction.status and track OPEN/CLOSED tag.c: use ref transactions when doing updates replace.c: use the ref transaction functions for updates commit.c: use ref transactions for updates sequencer.c: use ref transactions for all ref updates fast-import.c: change update_branch to use ref transactions branch.c: use ref transaction for all ref updates refs.c: change update_ref to use a transaction receive-pack.c: use a reference transaction for updating the refs fast-import.c: use a ref transaction when dumping tags walker.c: use ref transaction for ref updates refs.c: make lock_ref_sha1 static refs.c: remove the update_ref_lock function refs.c: remove the update_ref_write function refs.c: remove lock_ref_sha1 refs.c: make prune_ref use a transaction to delete the ref refs.c: make delete_ref use a transaction branch.c | 31 ++++--- builtin/commit.c | 25 +++-- builtin/receive-pack.c | 25 +++-- builtin/replace.c | 14 +-- builtin/tag.c | 16 ++-- builtin/update-ref.c | 14 ++- fast-import.c | 54 +++++++---- refs.c | 244 ++++++++++++++++++++++++++++--------------------- refs.h | 77 ++++++++++++---- sequencer.c | 26 ++++-- walker.c | 70 ++++++++------ 11 files changed, 367 insertions(+), 229 deletions(-) [...] > Jonathan Nieder <jrnieder@gmail.com> writes: >> --- a/builtin/receive-pack.c >> +++ b/builtin/receive-pack.c >> @@ -580,18 +580,18 @@ static char *update(struct command *cmd, struct shallow_info *si) [...] >> if (!transaction || >> ref_transaction_update(transaction, namespaced_name, >> new_sha1, old_sha1, 0, 1, &err) || >> ref_transaction_commit(transaction, "push", &err)) { >> - char *str = strbuf_detach(&err, NULL); >> ref_transaction_free(transaction); >> >> - rp_error("%s", str); >> - return str; >> + rp_error("%s", err.buf); >> + strbuf_release(&err); >> + return "failed to update ref"; >> } > > I am guessing that the purpose of this change is to make sure that > cmd->error_string is taken from a fixed set of strings, not an > arbitrary collection of errors from the transaction subsystem, but > what is the significance of doing so? Do the other side i18n while > running print_ref_status() or something? It's about keeping the message in parentheses on the "! rejected master -> master" line short and simple, since the more detailed diagnosis is redundant next to the full message that is sent over sideband earlier and gets a line to itself. Side effects: * a less invasive patch --- no more need to change the calling convention to return a dynamically allocated string, with assertions throughout the file that check for leaks * using the same "all lowercase and brief" convention makes the message less jarring next to other statuses for "! rejected" lines, like "non-fast-forward" * no more O(n) stall from traversing the linked list to free messages when receive-pack is about to exit [...] >> diff --git a/refs.h b/refs.h >> index aad846c..69ef28c 100644 >> --- a/refs.h >> +++ b/refs.h >> @@ -180,8 +180,7 @@ extern int peel_ref(const char *refname, unsigned char *sha1); >> */ >> #define REF_NODEREF 0x01 >> /* >> - * Locks any ref (for 'HEAD' type refs) and sets errno to something >> - * meaningful on failure. >> + * This function sets errno to something meaningful on failure. >> */ >> extern struct ref_lock *lock_any_ref_for_update(const char *refname, >> const unsigned char *old_sha1, > > To contrast this function with lock_ref_sha1() that only allowed > refs under refs/ hierarchy, the comment may have made sense, but now > that other function is gone, losing the comment makes sense. Ahhh. Thanks for explaining. Jonathan ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 0/20] rs/ref-transaction-1 (Re: Transaction patch series overview) 2014-08-26 22:14 ` Jonathan Nieder @ 2014-08-27 0:28 ` Jonathan Nieder 2014-08-27 0:35 ` [PATCH 18/20] refs.c: remove lock_ref_sha1 Jonathan Nieder 0 siblings, 1 reply; 25+ messages in thread From: Jonathan Nieder @ 2014-08-27 0:28 UTC (permalink / raw) To: Junio C Hamano; +Cc: Ronnie Sahlberg, git, Michael Haggerty Jonathan Nieder wrote: > This series teaches the tag, replace, commit, cherry-pick, > fast-import, checkout -b, branch, receive-pack, and http-fetch > commands and all update_ref and delete_ref callers to use the ref > transaction API instead of lock_ref_sha1. > > The main user-visible change should be some cosmetic changes to error > messages. The series also combines multiple ref updates into a single > transaction in 'git http-fetch -w' and when writing tags in > fast-import but otherwise doesn't change the granularity of > transactions. > > Reviewed at https://code-review.googlesource.com/#/q/topic:ref-transaction-1 > > ---------------------------------------------------------------- > Ronnie Sahlberg (20): > refs.c: change ref_transaction_create to do error checking and return status > refs.c: update ref_transaction_delete to check for error and return status > refs.c: make ref_transaction_begin take an err argument > refs.c: add transaction.status and track OPEN/CLOSED > tag.c: use ref transactions when doing updates > replace.c: use the ref transaction functions for updates > commit.c: use ref transactions for updates > sequencer.c: use ref transactions for all ref updates > fast-import.c: change update_branch to use ref transactions > branch.c: use ref transaction for all ref updates > refs.c: change update_ref to use a transaction > receive-pack.c: use a reference transaction for updating the refs > fast-import.c: use a ref transaction when dumping tags > walker.c: use ref transaction for ref updates > refs.c: make lock_ref_sha1 static > refs.c: remove the update_ref_lock function > refs.c: remove the update_ref_write function > refs.c: remove lock_ref_sha1 > refs.c: make prune_ref use a transaction to delete the ref > refs.c: make delete_ref use a transaction And here are the patches. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 18/20] refs.c: remove lock_ref_sha1 2014-08-27 0:28 ` [PATCH 0/20] rs/ref-transaction-1 (Re: Transaction patch series overview) Jonathan Nieder @ 2014-08-27 0:35 ` Jonathan Nieder 0 siblings, 0 replies; 25+ messages in thread From: Jonathan Nieder @ 2014-08-27 0:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: Ronnie Sahlberg, git, Michael Haggerty From: Ronnie Sahlberg <sahlberg@google.com> Date: Tue, 29 Apr 2014 15:45:52 -0700 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 <sahlberg@google.com> Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- refs.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/refs.c b/refs.c index ef7660a..f0883d0 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); -- 2.1.0.rc2.206.gedb03e5 ^ permalink raw reply related [flat|nested] 25+ messages in thread
end of thread, other threads:[~2014-08-27 0:35 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-07-15 23:33 [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg 2014-07-15 23:33 ` [PATCH 01/20] refs.c: change ref_transaction_create to do error checking and return status Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 02/20] refs.c: update ref_transaction_delete to check for error " Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 03/20] refs.c: make ref_transaction_begin take an err argument Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 04/20] refs.c: add transaction.status and track OPEN/CLOSED/ERROR Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 05/20] tag.c: use ref transactions when doing updates Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 06/20] replace.c: use the ref transaction functions for updates Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 07/20] commit.c: use ref transactions " Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 08/20] sequencer.c: use ref transactions for all ref updates Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 09/20] fast-import.c: change update_branch to use ref transactions Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 10/20] branch.c: use ref transaction for all ref updates Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 11/20] refs.c: change update_ref to use a transaction Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 12/20] receive-pack.c: use a reference transaction for updating the refs Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 13/20] fast-import.c: use a ref transaction when dumping tags Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 14/20] walker.c: use ref transaction for ref updates Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 15/20] refs.c: make lock_ref_sha1 static Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 16/20] refs.c: remove the update_ref_lock function Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 17/20] refs.c: remove the update_ref_write function Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 18/20] refs.c: remove lock_ref_sha1 Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 19/20] refs.c: make prune_ref use a transaction to delete the ref Ronnie Sahlberg 2014-07-15 23:34 ` [PATCH 20/20] refs.c: make delete_ref use a transaction Ronnie Sahlberg 2014-07-15 23:37 ` [PATCH 00/20] ref transactions part 2 Ronnie Sahlberg 2014-07-16 22:16 ` Junio C Hamano 2014-07-16 22:52 ` Ronnie Sahlberg 2014-07-30 17:10 Transaction patch series overview Ronnie Sahlberg 2014-07-31 21:41 ` Ronnie Sahlberg 2014-08-08 16:50 ` Ronnie Sahlberg 2014-08-19 19:54 ` Ronnie Sahlberg 2014-08-20 23:17 ` Jonathan Nieder 2014-08-26 0:03 ` Jonathan Nieder 2014-08-26 21:01 ` Junio C Hamano 2014-08-26 22:14 ` Jonathan Nieder 2014-08-27 0:28 ` [PATCH 0/20] rs/ref-transaction-1 (Re: Transaction patch series overview) Jonathan Nieder 2014-08-27 0:35 ` [PATCH 18/20] refs.c: remove lock_ref_sha1 Jonathan Nieder
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.