From: Junio C Hamano <gitster@pobox•com>
To: Brad King <brad.king@kitware•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH/RFC 6/7] refs: add update_refs for multiple simultaneous updates
Date: Thu, 29 Aug 2013 10:39:58 -0700 [thread overview]
Message-ID: <xmqqhae85rbl.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <518ba77096664a679e4c7212e4cc4d496c6b38d3.1377784597.git.brad.king@kitware.com> (Brad King's message of "Thu, 29 Aug 2013 10:11:54 -0400")
Brad King <brad.king@kitware•com> writes:
> Add 'struct ref_update' to encode the information needed to update or
> delete a ref (name, new sha1, optional old sha1, no-deref flag). Add
> function 'update_refs' accepting an array of updates to perform. First
> acquire locks on all refs with verified old values. Then update or
> delete all refs accordingly. Fail if any one lock cannot be obtained or
> any one old value does not match.
>
> Though the refs themeselves cannot be modified together in a single
> atomic transaction, this function does enable some useful semantics.
> For example, a caller may create a new branch starting from the head of
> another branch and rewind the original branch at the same time. This
> transfers ownership of commits between branches without risk of losing
> commits added to the original branch by a concurrent process, or risk of
> a concurrent process creating the new branch first.
>
> Signed-off-by: Brad King <brad.king@kitware•com>
> ---
> refs.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> refs.h | 11 +++++++++++
> 2 files changed, 77 insertions(+)
>
> diff --git a/refs.c b/refs.c
> index 5a6c14e..0a0c19e 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3238,6 +3238,72 @@ int update_ref(const char *action, const char *refname,
> return update_ref_write(action, refname, sha1, lock, onerr);
> }
>
> +int update_refs(const char *action, struct ref_update *updates,
> + int n, enum action_on_err onerr)
> +{
> + int ret = 0, delnum = 0, i;
> + int *types;
> + struct ref_lock **locks;
> + const char **delnames;
> +
> + if (!updates || !n)
> + return 0;
> +
> + /* Allocate work space: */
> + types = xmalloc(sizeof(int)*n);
> + locks = xmalloc(sizeof(struct ref_lock*)*n);
> + delnames = xmalloc(sizeof(const char*)*n);
> +
> + /* Acquire all locks while verifying old values: */
> + for (i=0; i < n; ++i) {
Style:
for (i = 0; i < n; i++) {
> + locks[i] = update_ref_lock(updates[i].ref_name,
> + updates[i].old_sha1,
> + updates[i].flags,
> + &types[i], onerr);
> + if (!locks[i])
> + break;
> + }
Is it asking for AB-BA deadlock? If so, is the caller responsible
for avoiding it?
> + /* Abort if we did not get all locks: */
> + if (i < n) {
> + while (--i >= 0)
> + unlock_ref(locks[i]);
> + free(types);
> + free(locks);
> + free(delnames);
> + return 1;
> + }
> +
> + /* Perform updates first so live commits remain referenced: */
> + for (i=0; i < n; ++i)
> + if (!is_null_sha1(updates[i].new_sha1)) {
> + ret |= update_ref_write(action,
> + updates[i].ref_name,
> + updates[i].new_sha1,
> + locks[i], onerr);
> + locks[i] = 0; /* freed by update_ref_write */
> + }
> +
> + /* Perform deletes now that updates are safely completed: */
> + for (i=0; i < n; ++i)
> + if (locks[i]) {
> + delnames[delnum++] = locks[i]->ref_name;
> + ret |= delete_ref_loose(locks[i], types[i]);
> + }
> + ret |= repack_without_refs(delnames, delnum);
> + for (i=0; i < delnum; ++i)
> + unlink_or_warn(git_path("logs/%s", delnames[i]));
> + clear_loose_ref_cache(&ref_cache);
> + for (i=0; i < n; ++i)
> + if (locks[i])
> + unlock_ref(locks[i]);
> +
> + free(types);
> + free(locks);
> + free(delnames);
> + return ret;
> +}
> +
> struct ref *find_ref_by_name(const struct ref *list, const char *name)
> {
> for ( ; list; list = list->next)
> diff --git a/refs.h b/refs.h
> index 2cd307a..5763f3a 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -214,6 +214,17 @@ 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_update {
> + const char *ref_name;
> + unsigned char new_sha1[20];
> + unsigned char *old_sha1;
This asymmetry is rather curious and will later become problematic
when we have more users of this API. Maybe your callers happen have
static storage to keep old_sha1[] outside this structure but do not
have it for new_sha1[] for some unknown reason, which may not apply
to later callers we may want to add.
> + int flags;
> +};
> +
> +/** lock all refs and then write all of them */
> +int update_refs(const char *action, struct ref_update *updates,
> + int n, enum action_on_err onerr);
> +
> extern int parse_hide_refs_config(const char *var, const char *value, const char *);
> extern int ref_is_hidden(const char *);
next prev parent reply other threads:[~2013-08-29 17:40 UTC|newest]
Thread overview: 106+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-29 14:11 [PATCH/RFC 0/7] Multiple simultaneously locked ref updates Brad King
2013-08-29 14:11 ` [PATCH/RFC 1/7] reset: rename update_refs to reset_refs Brad King
2013-08-29 17:17 ` Junio C Hamano
2013-08-29 18:07 ` Brad King
2013-08-29 14:11 ` [PATCH/RFC 2/7] refs: report ref type from lock_any_ref_for_update Brad King
2013-08-29 17:22 ` Junio C Hamano
2013-08-29 18:08 ` Brad King
2013-08-29 14:11 ` [PATCH/RFC 3/7] refs: factor update_ref steps into helpers Brad King
2013-08-29 14:11 ` [PATCH/RFC 4/7] refs: factor delete_ref loose ref step into a helper Brad King
2013-08-29 17:28 ` Junio C Hamano
2013-08-29 18:08 ` Brad King
2013-08-29 14:11 ` [PATCH/RFC 5/7] refs: add function to repack without multiple refs Brad King
2013-08-29 17:34 ` Junio C Hamano
2013-08-29 18:09 ` Brad King
2013-08-29 14:11 ` [PATCH/RFC 6/7] refs: add update_refs for multiple simultaneous updates Brad King
2013-08-29 17:39 ` Junio C Hamano [this message]
2013-08-29 18:20 ` Brad King
2013-08-29 18:32 ` Junio C Hamano
2013-08-29 18:38 ` Brad King
2013-08-29 19:30 ` Brad King
2013-08-29 14:11 ` [PATCH/RFC 7/7] update-ref: support " Brad King
2013-08-29 18:34 ` Junio C Hamano
2013-08-29 18:42 ` Brad King
2013-08-29 15:32 ` [PATCH/RFC 0/7] Multiple simultaneously locked ref updates Martin Fick
2013-08-29 15:46 ` Brad King
2013-08-29 16:21 ` Junio C Hamano
2013-08-29 17:09 ` Brad King
2013-08-29 18:07 ` Junio C Hamano
2013-08-29 18:23 ` Brad King
2013-08-30 18:11 ` [PATCH v2 0/8] " Brad King
2013-08-30 18:11 ` [PATCH v2 1/8] reset: rename update_refs to reset_refs Brad King
2013-08-30 18:12 ` [PATCH v2 2/8] refs: report ref type from lock_any_ref_for_update Brad King
2013-08-30 18:12 ` [PATCH v2 3/8] refs: factor update_ref steps into helpers Brad King
2013-09-01 6:08 ` Junio C Hamano
2013-09-02 17:19 ` Brad King
2013-08-30 18:12 ` [PATCH v2 4/8] refs: factor delete_ref loose ref step into a helper Brad King
2013-08-31 16:30 ` Michael Haggerty
2013-09-02 17:19 ` Brad King
2013-08-30 18:12 ` [PATCH v2 5/8] refs: add function to repack without multiple refs Brad King
2013-08-30 18:12 ` [PATCH v2 6/8] refs: add update_refs for multiple simultaneous updates Brad King
2013-08-31 18:19 ` Michael Haggerty
2013-09-02 17:20 ` Brad King
2013-09-01 6:08 ` Junio C Hamano
2013-09-02 17:20 ` Brad King
2013-09-03 4:43 ` Michael Haggerty
2013-09-03 11:59 ` Brad King
2013-08-30 18:12 ` [PATCH v2 7/8] update-ref: support " Brad King
2013-08-30 22:51 ` Junio C Hamano
2013-09-02 17:23 ` Brad King
2013-08-31 18:42 ` Michael Haggerty
2013-09-02 17:21 ` Brad King
2013-08-30 18:12 ` [PATCH v2 8/8] update-ref: add test cases covering --stdin signature Brad King
2013-09-01 3:41 ` Eric Sunshine
2013-09-02 17:23 ` Brad King
2013-08-31 19:02 ` [PATCH v2 0/8] Multiple simultaneously locked ref updates Michael Haggerty
2013-09-02 17:48 ` [PATCH v3 " Brad King
2013-09-02 17:48 ` [PATCH v3 1/8] reset: rename update_refs to reset_refs Brad King
2013-09-02 17:48 ` [PATCH v3 2/8] refs: report ref type from lock_any_ref_for_update Brad King
2013-09-02 17:48 ` [PATCH v3 3/8] refs: factor update_ref steps into helpers Brad King
2013-09-02 17:48 ` [PATCH v3 4/8] refs: factor delete_ref loose ref step into a helper Brad King
2013-09-02 17:48 ` [PATCH v3 5/8] refs: add function to repack without multiple refs Brad King
2013-09-02 17:48 ` [PATCH v3 6/8] refs: add update_refs for multiple simultaneous updates Brad King
2013-09-02 17:48 ` [PATCH v3 7/8] update-ref: support " Brad King
2013-09-02 18:37 ` Brad King
2013-09-02 17:48 ` [PATCH v3 8/8] update-ref: add test cases covering --stdin signature Brad King
2013-09-03 8:16 ` Eric Sunshine
2013-09-03 12:15 ` Brad King
2013-09-04 15:22 ` [PATCH v4 0/8] Multiple simultaneously locked ref updates Brad King
2013-09-04 15:22 ` [PATCH v4 1/8] reset: rename update_refs to reset_refs Brad King
2013-09-04 15:22 ` [PATCH v4 2/8] refs: report ref type from lock_any_ref_for_update Brad King
2013-09-04 15:22 ` [PATCH v4 3/8] refs: factor update_ref steps into helpers Brad King
2013-09-04 15:22 ` [PATCH v4 4/8] refs: factor delete_ref loose ref step into a helper Brad King
2013-09-04 15:22 ` [PATCH v4 5/8] refs: add function to repack without multiple refs Brad King
2013-09-04 15:22 ` [PATCH v4 6/8] refs: add update_refs for multiple simultaneous updates Brad King
2013-09-04 15:22 ` [PATCH v4 7/8] update-ref: support " Brad King
2013-09-04 18:23 ` Junio C Hamano
2013-09-04 19:59 ` Brad King
2013-09-04 21:27 ` Junio C Hamano
2013-09-05 20:32 ` Brad King
2013-09-05 21:23 ` Junio C Hamano
2013-09-05 23:44 ` Brad King
2013-09-04 19:17 ` Junio C Hamano
2013-09-04 19:16 ` Brad King
2013-09-04 15:22 ` [PATCH v4 8/8] update-ref: add test cases covering --stdin signature Brad King
2013-09-09 13:22 ` [PATCH v5 0/8] Multiple simultaneously locked ref updates Brad King
2013-09-09 13:22 ` [PATCH v5 7/8] update-ref: support multiple simultaneous updates Brad King
2013-09-09 13:22 ` [PATCH v5 8/8] update-ref: add test cases covering --stdin signature Brad King
2013-09-10 0:57 ` [PATCH v6 0/8] Multiple simultaneously locked ref updates Brad King
2013-09-10 0:57 ` [PATCH v6 1/8] reset: rename update_refs to reset_refs Brad King
2013-09-10 3:43 ` Ramkumar Ramachandra
2013-09-10 0:57 ` [PATCH v6 2/8] refs: report ref type from lock_any_ref_for_update Brad King
2013-09-10 0:57 ` [PATCH v6 3/8] refs: factor update_ref steps into helpers Brad King
2013-09-10 0:57 ` [PATCH v6 4/8] refs: factor delete_ref loose ref step into a helper Brad King
2013-09-10 0:57 ` [PATCH v6 5/8] refs: add function to repack without multiple refs Brad King
2013-09-10 0:57 ` [PATCH v6 6/8] refs: add update_refs for multiple simultaneous updates Brad King
2013-09-10 0:57 ` [PATCH v6 7/8] update-ref: support " Brad King
2013-09-10 22:51 ` Eric Sunshine
2013-09-11 12:36 ` Brad King
2013-09-11 16:07 ` Eric Sunshine
2013-09-10 0:57 ` [PATCH v6 8/8] update-ref: add test cases covering --stdin signature Brad King
2013-09-10 22:46 ` Eric Sunshine
2013-09-10 22:54 ` Junio C Hamano
2013-09-11 12:46 ` [PATCH v6.1 " Brad King
2013-09-10 16:30 ` [PATCH v6 0/8] Multiple simultaneously locked ref updates Junio C Hamano
2013-09-10 16:54 ` Brad King
2013-09-10 20:18 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqhae85rbl.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=brad.king@kitware$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox