From 581d4e0cdbe9526c122b9d0835f951e478f82448 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Thu, 12 Feb 2015 12:12:12 +0100 Subject: [PATCH 01/13] refs: move REF_DELETING to refs.c It is only used internally now. Document it a little bit better, too. Signed-off-by: Michael Haggerty Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- refs.c | 6 ++++++ refs.h | 4 +--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/refs.c b/refs.c index ab2f2a92cd..5e6355c930 100644 --- a/refs.c +++ b/refs.c @@ -34,6 +34,12 @@ static unsigned char refname_disposition[256] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 4, 4 }; +/* + * Flag passed to lock_ref_sha1_basic() telling it to tolerate broken + * refs (i.e., because the reference is about to be deleted anyway). + */ +#define REF_DELETING 0x02 + /* * Used as a flag to ref_transaction_delete when a loose ref is being * pruned. diff --git a/refs.h b/refs.h index afa3c4decd..9bf214880f 100644 --- a/refs.h +++ b/refs.h @@ -183,12 +183,10 @@ extern int peel_ref(const char *refname, unsigned char *sha1); * Flags controlling ref_transaction_update(), ref_transaction_create(), etc. * REF_NODEREF: act on the ref directly, instead of dereferencing * symbolic references. - * REF_DELETING: tolerate broken refs * - * Flags >= 0x100 are reserved for internal use. + * Other flags are reserved for internal use. */ #define REF_NODEREF 0x01 -#define REF_DELETING 0x02 /* * Setup reflog before using. Set errno to something meaningful on failure. From 31e79f0a54e57454a9677eeb8b1108e4f907b8b9 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Thu, 12 Feb 2015 12:12:13 +0100 Subject: [PATCH 02/13] refs: remove the gap in the REF_* constant values There is no reason to "reserve" a gap between the public and private flags values. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/refs.c b/refs.c index 5e6355c930..4de13833c8 100644 --- a/refs.c +++ b/refs.c @@ -44,7 +44,8 @@ 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 +#define REF_ISPRUNING 0x04 + /* * Try to read one refname component from the front of refname. * Return the length of the component found, or -1 if the component is From fec14ec38ca65b13f9e0fcdb60f27674c6f9af70 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Tue, 17 Feb 2015 18:00:13 +0100 Subject: [PATCH 03/13] refs.c: change some "flags" to "unsigned int" Change the following functions' "flags" arguments from "int" to "unsigned int": * ref_transaction_update() * ref_transaction_create() * ref_transaction_delete() * update_ref() * delete_ref() * lock_ref_sha1_basic() Also change the "flags" member in "struct ref_update" to unsigned. Suggested-by: Junio C Hamano Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/update-ref.c | 3 ++- cache.h | 2 +- refs.c | 18 +++++++++--------- refs.h | 10 +++++----- 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 2497ba4303..9a1659e11e 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -353,7 +353,8 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) { const char *refname, *oldval; unsigned char sha1[20], oldsha1[20]; - int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0; + int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0; + unsigned int flags = 0; struct option options[] = { OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the update")), OPT_BOOL('d', NULL, &delete, N_("delete the reference")), diff --git a/cache.h b/cache.h index f704af5df0..ab265be1cd 100644 --- a/cache.h +++ b/cache.h @@ -568,7 +568,7 @@ extern void update_index_if_able(struct index_state *, struct lock_file *); extern int hold_locked_index(struct lock_file *, int); extern void set_alternate_index_output(const char *); -extern int delete_ref(const char *, const unsigned char *sha1, int delopt); +extern int delete_ref(const char *, const unsigned char *sha1, unsigned int flags); /* Environment bits from configuration mechanism */ extern int trust_executable_bit; diff --git a/refs.c b/refs.c index 4de13833c8..a203e44559 100644 --- a/refs.c +++ b/refs.c @@ -2256,7 +2256,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log) static struct ref_lock *lock_ref_sha1_basic(const char *refname, const unsigned char *old_sha1, const struct string_list *skip, - int flags, int *type_p) + unsigned int flags, int *type_p) { char *ref_file; const char *orig_refname = refname; @@ -2740,14 +2740,14 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err) return 0; } -int delete_ref(const char *refname, const unsigned char *sha1, int delopt) +int delete_ref(const char *refname, const unsigned char *sha1, unsigned int flags) { struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; transaction = ref_transaction_begin(&err); if (!transaction || - ref_transaction_delete(transaction, refname, sha1, delopt, + ref_transaction_delete(transaction, refname, sha1, flags, sha1 && !is_null_sha1(sha1), NULL, &err) || ref_transaction_commit(transaction, &err)) { error("%s", err.buf); @@ -3571,7 +3571,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data) struct ref_update { unsigned char new_sha1[20]; unsigned char old_sha1[20]; - int flags; /* REF_NODEREF? */ + unsigned int flags; /* REF_NODEREF? */ int have_old; /* 1 if old_sha1 is valid, 0 otherwise */ struct ref_lock *lock; int type; @@ -3644,7 +3644,7 @@ int ref_transaction_update(struct ref_transaction *transaction, const char *refname, const unsigned char *new_sha1, const unsigned char *old_sha1, - int flags, int have_old, const char *msg, + unsigned int flags, int have_old, const char *msg, struct strbuf *err) { struct ref_update *update; @@ -3678,7 +3678,7 @@ int ref_transaction_update(struct ref_transaction *transaction, int ref_transaction_create(struct ref_transaction *transaction, const char *refname, const unsigned char *new_sha1, - int flags, const char *msg, + unsigned int flags, const char *msg, struct strbuf *err) { return ref_transaction_update(transaction, refname, new_sha1, @@ -3688,7 +3688,7 @@ int ref_transaction_create(struct ref_transaction *transaction, int ref_transaction_delete(struct ref_transaction *transaction, const char *refname, const unsigned char *old_sha1, - int flags, int have_old, const char *msg, + unsigned int flags, int have_old, const char *msg, struct strbuf *err) { return ref_transaction_update(transaction, refname, null_sha1, @@ -3697,7 +3697,7 @@ int ref_transaction_delete(struct ref_transaction *transaction, int update_ref(const char *action, const char *refname, const unsigned char *sha1, const unsigned char *oldval, - int flags, enum action_on_err onerr) + unsigned int flags, enum action_on_err onerr) { struct ref_transaction *t; struct strbuf err = STRBUF_INIT; @@ -3781,7 +3781,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, /* Acquire all locks while verifying old values */ for (i = 0; i < n; i++) { struct ref_update *update = updates[i]; - int flags = update->flags; + unsigned int flags = update->flags; if (is_null_sha1(update->new_sha1)) flags |= REF_DELETING; diff --git a/refs.h b/refs.h index 9bf214880f..92b85979c1 100644 --- a/refs.h +++ b/refs.h @@ -276,7 +276,7 @@ int ref_transaction_update(struct ref_transaction *transaction, const char *refname, const unsigned char *new_sha1, const unsigned char *old_sha1, - int flags, int have_old, const char *msg, + unsigned int flags, int have_old, const char *msg, struct strbuf *err); /* @@ -291,7 +291,7 @@ int ref_transaction_update(struct ref_transaction *transaction, int ref_transaction_create(struct ref_transaction *transaction, const char *refname, const unsigned char *new_sha1, - int flags, const char *msg, + unsigned int flags, const char *msg, struct strbuf *err); /* @@ -305,7 +305,7 @@ int ref_transaction_create(struct ref_transaction *transaction, int ref_transaction_delete(struct ref_transaction *transaction, const char *refname, const unsigned char *old_sha1, - int flags, int have_old, const char *msg, + unsigned int flags, int have_old, const char *msg, struct strbuf *err); /* @@ -328,8 +328,8 @@ void ref_transaction_free(struct ref_transaction *transaction); /** Lock a ref and then write its file */ int update_ref(const char *action, const char *refname, - const unsigned char *sha1, const unsigned char *oldval, - int flags, enum action_on_err onerr); + const unsigned char *sha1, const unsigned char *oldval, + unsigned int flags, 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 *); From 8df4e51138781927962438819d79ae3221b527b5 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Tue, 17 Feb 2015 18:00:14 +0100 Subject: [PATCH 04/13] struct ref_update: move "have_old" into "flags" Instead of having a separate have_old field, record this boolean value as a bit in the "flags" field. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 45 ++++++++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/refs.c b/refs.c index a203e44559..e3a2ba8bd7 100644 --- a/refs.c +++ b/refs.c @@ -41,11 +41,17 @@ static unsigned char refname_disposition[256] = { #define REF_DELETING 0x02 /* - * Used as a flag to ref_transaction_delete when a loose ref is being + * Used as a flag in ref_update::flags when a loose ref is being * pruned. */ #define REF_ISPRUNING 0x04 +/* + * Used as a flag in ref_update::flags when old_sha1 should be + * checked. + */ +#define REF_HAVE_OLD 0x08 + /* * Try to read one refname component from the front of refname. * Return the length of the component found, or -1 if the component is @@ -3563,16 +3569,20 @@ int for_each_reflog(each_ref_fn fn, void *cb_data) } /** - * 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 - * while locking the ref, set have_old to 1 and set old_sha1 to the - * value or to zero to ensure the ref does not exist before update. + * Information needed for a single ref update. Set new_sha1 to the new + * value or to null_sha1 to delete the ref. To check the old value + * while the ref is locked, set (flags & REF_HAVE_OLD) and set + * old_sha1 to the old value, or to null_sha1 to ensure the ref does + * not exist before update. */ struct ref_update { unsigned char new_sha1[20]; unsigned char old_sha1[20]; - unsigned int flags; /* REF_NODEREF? */ - int have_old; /* 1 if old_sha1 is valid, 0 otherwise */ + /* + * One or more of REF_HAVE_OLD, REF_NODEREF, + * REF_DELETING, and REF_ISPRUNING: + */ + unsigned int flags; struct ref_lock *lock; int type; char *msg; @@ -3666,10 +3676,11 @@ int ref_transaction_update(struct ref_transaction *transaction, update = add_update(transaction, refname); hashcpy(update->new_sha1, new_sha1); - update->flags = flags; - update->have_old = have_old; - if (have_old) + if (have_old) { hashcpy(update->old_sha1, old_sha1); + flags |= REF_HAVE_OLD; + } + update->flags = flags; if (msg) update->msg = xstrdup(msg); return 0; @@ -3785,13 +3796,13 @@ int ref_transaction_commit(struct ref_transaction *transaction, if (is_null_sha1(update->new_sha1)) flags |= REF_DELETING; - update->lock = lock_ref_sha1_basic(update->refname, - (update->have_old ? - update->old_sha1 : - NULL), - NULL, - flags, - &update->type); + update->lock = lock_ref_sha1_basic( + update->refname, + ((update->flags & REF_HAVE_OLD) ? + update->old_sha1 : NULL), + NULL, + flags, + &update->type); if (!update->lock) { ret = (errno == ENOTDIR) ? TRANSACTION_NAME_CONFLICT From 1d147bdff0b8132d3aa53a46df8dbab7b673b796 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Tue, 17 Feb 2015 18:00:15 +0100 Subject: [PATCH 05/13] ref_transaction_update(): remove "have_old" parameter Instead, verify the reference's old value if and only if old_sha1 is non-NULL. ref_transaction_delete() will get the same treatment in a moment. Signed-off-by: Michael Haggerty Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- branch.c | 5 +++-- builtin/commit.c | 2 +- builtin/fetch.c | 6 ++++-- builtin/receive-pack.c | 2 +- builtin/replace.c | 2 +- builtin/tag.c | 2 +- builtin/update-ref.c | 7 ++++--- fast-import.c | 6 +++--- refs.c | 18 ++++++++---------- refs.h | 6 +++--- sequencer.c | 2 +- walker.c | 2 +- 12 files changed, 31 insertions(+), 29 deletions(-) diff --git a/branch.c b/branch.c index 4bab55a9a8..b0024353f4 100644 --- a/branch.c +++ b/branch.c @@ -284,8 +284,9 @@ void create_branch(const char *head, transaction = ref_transaction_begin(&err); if (!transaction || - ref_transaction_update(transaction, ref.buf, sha1, - null_sha1, 0, !forcing, msg, &err) || + ref_transaction_update(transaction, ref.buf, + sha1, forcing ? NULL : null_sha1, + 0, msg, &err) || ref_transaction_commit(transaction, &err)) die("%s", err.buf); ref_transaction_free(transaction); diff --git a/builtin/commit.c b/builtin/commit.c index 7f467133b8..8afb0ff5e0 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1767,7 +1767,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) ref_transaction_update(transaction, "HEAD", sha1, current_head ? current_head->object.sha1 : NULL, - 0, !!current_head, sb.buf, &err) || + 0, sb.buf, &err) || ref_transaction_commit(transaction, &err)) { rollback_index_files(); die("%s", err.buf); diff --git a/builtin/fetch.c b/builtin/fetch.c index 7b84d35d83..719bf4f244 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -416,8 +416,10 @@ static int s_update_ref(const char *action, transaction = ref_transaction_begin(&err); if (!transaction || - ref_transaction_update(transaction, ref->name, ref->new_sha1, - ref->old_sha1, 0, check_old, msg, &err)) + ref_transaction_update(transaction, ref->name, + ref->new_sha1, + check_old ? ref->old_sha1 : NULL, + 0, msg, &err)) goto fail; ret = ref_transaction_commit(transaction, &err); diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index e0ce78e5a0..0be50e9540 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -971,7 +971,7 @@ static const char *update(struct command *cmd, struct shallow_info *si) if (ref_transaction_update(transaction, namespaced_name, new_sha1, old_sha1, - 0, 1, "push", + 0, "push", &err)) { rp_error("%s", err.buf); strbuf_release(&err); diff --git a/builtin/replace.c b/builtin/replace.c index 85d39b58d8..54bf01acb4 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -172,7 +172,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, 1, NULL, &err) || + 0, NULL, &err) || ref_transaction_commit(transaction, &err)) die("%s", err.buf); diff --git a/builtin/tag.c b/builtin/tag.c index 6dc85a9d5e..4194b9a711 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -733,7 +733,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) transaction = ref_transaction_begin(&err); if (!transaction || ref_transaction_update(transaction, ref.buf, object, prev, - 0, 1, NULL, &err) || + 0, NULL, &err) || ref_transaction_commit(transaction, &err)) die("%s", err.buf); ref_transaction_free(transaction); diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 9a1659e11e..1ad6ce1877 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -198,8 +198,9 @@ static const char *parse_cmd_update(struct ref_transaction *transaction, if (*next != line_termination) die("update %s: extra input: %s", refname, next); - if (ref_transaction_update(transaction, refname, new_sha1, old_sha1, - update_flags, have_old, msg, &err)) + if (ref_transaction_update(transaction, refname, + new_sha1, have_old ? old_sha1 : NULL, + update_flags, msg, &err)) die("%s", err.buf); update_flags = 0; @@ -297,7 +298,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, die("verify %s: extra input: %s", refname, next); if (ref_transaction_update(transaction, refname, new_sha1, old_sha1, - update_flags, 1, msg, &err)) + update_flags, msg, &err)) die("%s", err.buf); update_flags = 0; diff --git a/fast-import.c b/fast-import.c index d0bd285a16..1e72bfbd4c 100644 --- a/fast-import.c +++ b/fast-import.c @@ -1716,7 +1716,7 @@ static int update_branch(struct branch *b) transaction = ref_transaction_begin(&err); if (!transaction || ref_transaction_update(transaction, b->name, b->sha1, old_sha1, - 0, 1, msg, &err) || + 0, msg, &err) || ref_transaction_commit(transaction, &err)) { ref_transaction_free(transaction); error("%s", err.buf); @@ -1756,8 +1756,8 @@ static void dump_tags(void) 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, msg, &err)) { + if (ref_transaction_update(transaction, ref_name.buf, + t->sha1, NULL, 0, msg, &err)) { failure |= error("%s", err.buf); goto cleanup; } diff --git a/refs.c b/refs.c index e3a2ba8bd7..e88817c623 100644 --- a/refs.c +++ b/refs.c @@ -3654,7 +3654,7 @@ int ref_transaction_update(struct ref_transaction *transaction, const char *refname, const unsigned char *new_sha1, const unsigned char *old_sha1, - unsigned int flags, int have_old, const char *msg, + unsigned int flags, const char *msg, struct strbuf *err) { struct ref_update *update; @@ -3664,9 +3664,6 @@ int ref_transaction_update(struct ref_transaction *transaction, 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"); - if (!is_null_sha1(new_sha1) && check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { strbuf_addf(err, "refusing to update ref with bad name %s", @@ -3676,7 +3673,7 @@ int ref_transaction_update(struct ref_transaction *transaction, update = add_update(transaction, refname); hashcpy(update->new_sha1, new_sha1); - if (have_old) { + if (old_sha1) { hashcpy(update->old_sha1, old_sha1); flags |= REF_HAVE_OLD; } @@ -3693,7 +3690,7 @@ int ref_transaction_create(struct ref_transaction *transaction, struct strbuf *err) { return ref_transaction_update(transaction, refname, new_sha1, - null_sha1, flags, 1, msg, err); + null_sha1, flags, msg, err); } int ref_transaction_delete(struct ref_transaction *transaction, @@ -3702,8 +3699,9 @@ int ref_transaction_delete(struct ref_transaction *transaction, unsigned int flags, int have_old, const char *msg, struct strbuf *err) { - return ref_transaction_update(transaction, refname, null_sha1, - old_sha1, flags, have_old, msg, err); + return ref_transaction_update(transaction, refname, + null_sha1, have_old ? old_sha1 : NULL, + flags, msg, err); } int update_ref(const char *action, const char *refname, @@ -3715,8 +3713,8 @@ int update_ref(const char *action, const char *refname, t = ref_transaction_begin(&err); if (!t || - ref_transaction_update(t, refname, sha1, oldval, flags, - !!oldval, action, &err) || + ref_transaction_update(t, refname, sha1, oldval, + flags, action, &err) || ref_transaction_commit(t, &err)) { const char *str = "update_ref failed for ref '%s': %s"; diff --git a/refs.h b/refs.h index 92b85979c1..dced4c9464 100644 --- a/refs.h +++ b/refs.h @@ -265,8 +265,8 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err); /* * Add a reference update to transaction. new_sha1 is the value that * the reference should have after the update, or null_sha1 if it should - * be deleted. If have_old is true, then old_sha1 holds the value - * that the reference should have had before the update, or zeros if + * be deleted. If old_sha1 is non-NULL, then it is the value + * that the reference should have had before the update, or null_sha1 if * 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 @@ -276,7 +276,7 @@ int ref_transaction_update(struct ref_transaction *transaction, const char *refname, const unsigned char *new_sha1, const unsigned char *old_sha1, - unsigned int flags, int have_old, const char *msg, + unsigned int flags, const char *msg, struct strbuf *err); /* diff --git a/sequencer.c b/sequencer.c index 77a1266760..32aa05ed82 100644 --- a/sequencer.c +++ b/sequencer.c @@ -252,7 +252,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from, if (!transaction || ref_transaction_update(transaction, "HEAD", to, unborn ? null_sha1 : from, - 0, 1, sb.buf, &err) || + 0, sb.buf, &err) || ref_transaction_commit(transaction, &err)) { ref_transaction_free(transaction); error("%s", err.buf); diff --git a/walker.c b/walker.c index 483da4e0fb..58ffeca264 100644 --- a/walker.c +++ b/walker.c @@ -299,7 +299,7 @@ int walker_fetch(struct walker *walker, int targets, char **target, strbuf_reset(&refname); strbuf_addf(&refname, "refs/%s", write_ref[i]); if (ref_transaction_update(transaction, refname.buf, - &sha1[20 * i], NULL, 0, 0, + &sha1[20 * i], NULL, 0, msg ? msg : "fetch (unknown)", &err)) { error("%s", err.buf); From fb5a6bb61c215546b94156bbb54d43225424f7d0 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Tue, 17 Feb 2015 18:00:16 +0100 Subject: [PATCH 06/13] ref_transaction_delete(): remove "have_old" parameter Instead, verify the reference's old value if and only if old_sha1 is non-NULL. Signed-off-by: Michael Haggerty Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 3 +-- builtin/update-ref.c | 5 +++-- refs.c | 11 ++++++----- refs.h | 6 +++--- 4 files changed, 13 insertions(+), 12 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 0be50e9540..70e9ce5f96 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -953,8 +953,7 @@ static const char *update(struct command *cmd, struct shallow_info *si) if (ref_transaction_delete(transaction, namespaced_name, old_sha1, - 0, old_sha1 != NULL, - "push", &err)) { + 0, "push", &err)) { rp_error("%s", err.buf); strbuf_release(&err); return "failed to delete"; diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 1ad6ce1877..226995f029 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -265,8 +265,9 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction, if (*next != line_termination) die("delete %s: extra input: %s", refname, next); - if (ref_transaction_delete(transaction, refname, old_sha1, - update_flags, have_old, msg, &err)) + if (ref_transaction_delete(transaction, refname, + have_old ? old_sha1 : NULL, + update_flags, msg, &err)) die("%s", err.buf); update_flags = 0; diff --git a/refs.c b/refs.c index e88817c623..e3c4ab5fd5 100644 --- a/refs.c +++ b/refs.c @@ -2576,7 +2576,7 @@ static void prune_ref(struct ref_to_prune *r) transaction = ref_transaction_begin(&err); if (!transaction || ref_transaction_delete(transaction, r->name, r->sha1, - REF_ISPRUNING, 1, NULL, &err) || + REF_ISPRUNING, NULL, &err) || ref_transaction_commit(transaction, &err)) { ref_transaction_free(transaction); error("%s", err.buf); @@ -2753,8 +2753,9 @@ int delete_ref(const char *refname, const unsigned char *sha1, unsigned int flag transaction = ref_transaction_begin(&err); if (!transaction || - ref_transaction_delete(transaction, refname, sha1, flags, - sha1 && !is_null_sha1(sha1), NULL, &err) || + ref_transaction_delete(transaction, refname, + (sha1 && !is_null_sha1(sha1)) ? sha1 : NULL, + flags, NULL, &err) || ref_transaction_commit(transaction, &err)) { error("%s", err.buf); ref_transaction_free(transaction); @@ -3696,11 +3697,11 @@ int ref_transaction_create(struct ref_transaction *transaction, int ref_transaction_delete(struct ref_transaction *transaction, const char *refname, const unsigned char *old_sha1, - unsigned int flags, int have_old, const char *msg, + unsigned int flags, const char *msg, struct strbuf *err) { return ref_transaction_update(transaction, refname, - null_sha1, have_old ? old_sha1 : NULL, + null_sha1, old_sha1, flags, msg, err); } diff --git a/refs.h b/refs.h index dced4c9464..100731d1c8 100644 --- a/refs.h +++ b/refs.h @@ -295,8 +295,8 @@ int ref_transaction_create(struct ref_transaction *transaction, struct strbuf *err); /* - * Add a reference deletion to transaction. If have_old is true, then - * old_sha1 holds the value that the reference should have had before + * Add a reference deletion to transaction. If old_sha1 is non-NULL, then + * it 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 @@ -305,7 +305,7 @@ int ref_transaction_create(struct ref_transaction *transaction, int ref_transaction_delete(struct ref_transaction *transaction, const char *refname, const unsigned char *old_sha1, - unsigned int flags, int have_old, const char *msg, + unsigned int flags, const char *msg, struct strbuf *err); /* From a908a31c344cdcce2bf1a9fc163f3bccd2349e6a Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Tue, 17 Feb 2015 18:00:17 +0100 Subject: [PATCH 07/13] commit: add tests of commit races Committing involves the following steps: 1. Determine the current value of HEAD (if any). 2. Create the new commit object. 3. Update HEAD. Please note that step 2 can take arbitrarily long, because it might involve the user editing a commit message. If a second process sneaks in a commit during step 2, then the first commit process should fail. This is usually done correctly, because step 3 verifies that HEAD still points at the same commit that it pointed to during step 1. However, if there is a race when creating an *orphan* commit, then the test in step 3 is skipped. Add tests for proper handling of such races. One of the new tests fails. It will be fixed in a moment. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- t/t7516-commit-races.sh | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100755 t/t7516-commit-races.sh diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh new file mode 100755 index 0000000000..ed04d1c425 --- /dev/null +++ b/t/t7516-commit-races.sh @@ -0,0 +1,30 @@ +#!/bin/sh + +test_description='git commit races' +. ./test-lib.sh + +test_expect_failure 'race to create orphan commit' ' + write_script hare-editor <<-\EOF && + git commit --allow-empty -m hare + EOF + test_must_fail env EDITOR=./hare-editor git commit --allow-empty -m tortoise -e && + git show -s --pretty=format:%s >subject && + grep hare subject && + test -z "$(git show -s --pretty=format:%P)" +' + +test_expect_success 'race to create non-orphan commit' ' + write_script airplane-editor <<-\EOF && + git commit --allow-empty -m airplane + EOF + git checkout --orphan branch && + git commit --allow-empty -m base && + git rev-parse HEAD >base && + test_must_fail env EDITOR=./airplane-editor git commit --allow-empty -m ship -e && + git show -s --pretty=format:%s >subject && + grep airplane subject && + git rev-parse HEAD^ >parent && + test_cmp base parent +' + +test_done From a933c23e66265eedf822535ac56fcdb4ecb07a8c Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Tue, 17 Feb 2015 18:00:18 +0100 Subject: [PATCH 08/13] commit: avoid race when creating orphan commits If HEAD doesn't point at anything during the initial check, then we should make sure that it *still* doesn't point at anything when we are ready to update the reference. Otherwise, another process might commit while we are working (e.g., while we are waiting for the user to edit the commit message) and we will silently overwrite it. This fixes a failing test in t7516. Signed-off-by: Michael Haggerty Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- builtin/commit.c | 2 +- t/t7516-commit-races.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 8afb0ff5e0..682f922c73 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1766,7 +1766,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) if (!transaction || ref_transaction_update(transaction, "HEAD", sha1, current_head - ? current_head->object.sha1 : NULL, + ? current_head->object.sha1 : null_sha1, 0, sb.buf, &err) || ref_transaction_commit(transaction, &err)) { rollback_index_files(); diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh index ed04d1c425..f2ce14e907 100755 --- a/t/t7516-commit-races.sh +++ b/t/t7516-commit-races.sh @@ -3,7 +3,7 @@ test_description='git commit races' . ./test-lib.sh -test_expect_failure 'race to create orphan commit' ' +test_expect_success 'race to create orphan commit' ' write_script hare-editor <<-\EOF && git commit --allow-empty -m hare EOF From f04c5b5522214cd48b39399d1d26b07848fd0e52 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Tue, 17 Feb 2015 18:00:19 +0100 Subject: [PATCH 09/13] ref_transaction_create(): check that new_sha1 is valid Creating a reference requires a new_sha1 that is not NULL and not null_sha1. Signed-off-by: Michael Haggerty Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- refs.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/refs.c b/refs.c index e3c4ab5fd5..b9cf284f31 100644 --- a/refs.c +++ b/refs.c @@ -3690,6 +3690,8 @@ int ref_transaction_create(struct ref_transaction *transaction, unsigned int flags, const char *msg, struct strbuf *err) { + if (!new_sha1 || is_null_sha1(new_sha1)) + die("BUG: create called without valid new_sha1"); return ref_transaction_update(transaction, refname, new_sha1, null_sha1, flags, msg, err); } From 60294596ba6bf2f63506ffad60b9a94fc04612a1 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Tue, 17 Feb 2015 18:00:20 +0100 Subject: [PATCH 10/13] ref_transaction_delete(): check that old_sha1 is not null_sha1 It makes no sense to delete a reference that is already known not to exist. Signed-off-by: Michael Haggerty Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- refs.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/refs.c b/refs.c index b9cf284f31..d5bfd11790 100644 --- a/refs.c +++ b/refs.c @@ -3702,6 +3702,8 @@ int ref_transaction_delete(struct ref_transaction *transaction, unsigned int flags, const char *msg, struct strbuf *err) { + if (old_sha1 && is_null_sha1(old_sha1)) + die("BUG: delete called with old_sha1 set to zeros"); return ref_transaction_update(transaction, refname, null_sha1, old_sha1, flags, msg, err); From 16180334015ab44b0310b9d896e554a66c36a1a4 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Tue, 17 Feb 2015 18:00:21 +0100 Subject: [PATCH 11/13] ref_transaction_verify(): new function to check a reference's value If NULL is passed to ref_transaction_update()'s new_sha1 parameter, then just verify old_sha1 (under lock) without trying to change the new value of the reference. Use this functionality to add a new function ref_transaction_verify(), which checks the current value of the reference under lock but doesn't change it. Use ref_transaction_verify() in the implementation of "git update-ref --stdin"'s "verify" command to avoid the awkward need to "update" the reference to its existing value. Signed-off-by: Michael Haggerty Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- builtin/update-ref.c | 7 ++----- refs.c | 47 ++++++++++++++++++++++++++++++++++++-------- refs.h | 34 ++++++++++++++++++++++++-------- 3 files changed, 67 insertions(+), 21 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 226995f029..3d79a46b03 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -282,7 +282,6 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, { struct strbuf err = STRBUF_INIT; char *refname; - unsigned char new_sha1[20]; unsigned char old_sha1[20]; refname = parse_refname(input, &next); @@ -293,13 +292,11 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, PARSE_SHA1_OLD)) hashclr(old_sha1); - hashcpy(new_sha1, old_sha1); - if (*next != line_termination) die("verify %s: extra input: %s", refname, next); - if (ref_transaction_update(transaction, refname, new_sha1, old_sha1, - update_flags, msg, &err)) + if (ref_transaction_verify(transaction, refname, old_sha1, + update_flags, &err)) die("%s", err.buf); update_flags = 0; diff --git a/refs.c b/refs.c index d5bfd11790..1aaba3f7f7 100644 --- a/refs.c +++ b/refs.c @@ -46,11 +46,17 @@ static unsigned char refname_disposition[256] = { */ #define REF_ISPRUNING 0x04 +/* + * Used as a flag in ref_update::flags when the reference should be + * updated to new_sha1. + */ +#define REF_HAVE_NEW 0x08 + /* * Used as a flag in ref_update::flags when old_sha1 should be * checked. */ -#define REF_HAVE_OLD 0x08 +#define REF_HAVE_OLD 0x10 /* * Try to read one refname component from the front of refname. @@ -3577,10 +3583,17 @@ int for_each_reflog(each_ref_fn fn, void *cb_data) * not exist before update. */ struct ref_update { + /* + * If (flags & REF_HAVE_NEW), set the reference to this value: + */ unsigned char new_sha1[20]; + /* + * If (flags & REF_HAVE_OLD), check that the reference + * previously had this value: + */ unsigned char old_sha1[20]; /* - * One or more of REF_HAVE_OLD, REF_NODEREF, + * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF, * REF_DELETING, and REF_ISPRUNING: */ unsigned int flags; @@ -3665,7 +3678,7 @@ int ref_transaction_update(struct ref_transaction *transaction, if (transaction->state != REF_TRANSACTION_OPEN) die("BUG: update called for transaction that is not open"); - if (!is_null_sha1(new_sha1) && + if (new_sha1 && !is_null_sha1(new_sha1) && check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { strbuf_addf(err, "refusing to update ref with bad name %s", refname); @@ -3673,7 +3686,10 @@ int ref_transaction_update(struct ref_transaction *transaction, } update = add_update(transaction, refname); - hashcpy(update->new_sha1, new_sha1); + if (new_sha1) { + hashcpy(update->new_sha1, new_sha1); + flags |= REF_HAVE_NEW; + } if (old_sha1) { hashcpy(update->old_sha1, old_sha1); flags |= REF_HAVE_OLD; @@ -3709,6 +3725,19 @@ int ref_transaction_delete(struct ref_transaction *transaction, flags, msg, err); } +int ref_transaction_verify(struct ref_transaction *transaction, + const char *refname, + const unsigned char *old_sha1, + unsigned int flags, + struct strbuf *err) +{ + if (!old_sha1) + die("BUG: verify called with old_sha1 set to NULL"); + return ref_transaction_update(transaction, refname, + NULL, old_sha1, + flags, NULL, err); +} + int update_ref(const char *action, const char *refname, const unsigned char *sha1, const unsigned char *oldval, unsigned int flags, enum action_on_err onerr) @@ -3797,7 +3826,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, struct ref_update *update = updates[i]; unsigned int flags = update->flags; - if (is_null_sha1(update->new_sha1)) + if ((flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1)) flags |= REF_DELETING; update->lock = lock_ref_sha1_basic( update->refname, @@ -3819,8 +3848,9 @@ int ref_transaction_commit(struct ref_transaction *transaction, /* Perform updates first so live commits remain referenced */ for (i = 0; i < n; i++) { struct ref_update *update = updates[i]; + int flags = update->flags; - if (!is_null_sha1(update->new_sha1)) { + if ((flags & REF_HAVE_NEW) && !is_null_sha1(update->new_sha1)) { if (write_ref_sha1(update->lock, update->new_sha1, update->msg)) { update->lock = NULL; /* freed by write_ref_sha1 */ @@ -3836,14 +3866,15 @@ int ref_transaction_commit(struct ref_transaction *transaction, /* Perform deletes now that updates are safely completed */ for (i = 0; i < n; i++) { struct ref_update *update = updates[i]; + int flags = update->flags; - if (update->lock) { + if ((flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1)) { if (delete_ref_loose(update->lock, update->type, err)) { ret = TRANSACTION_GENERIC_ERROR; goto cleanup; } - if (!(update->flags & REF_ISPRUNING)) + if (!(flags & REF_ISPRUNING)) string_list_append(&refs_to_delete, update->lock->ref_name); } diff --git a/refs.h b/refs.h index 100731d1c8..5e139d5f8c 100644 --- a/refs.h +++ b/refs.h @@ -263,14 +263,19 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err); */ /* - * Add a reference update to transaction. new_sha1 is the value that - * the reference should have after the update, or null_sha1 if it should - * be deleted. If old_sha1 is non-NULL, then it is the value - * that the reference should have had before the update, or null_sha1 if - * 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. + * Add a reference update to transaction. new_sha1 is the value that + * the reference should have after the update, or null_sha1 if it + * should be deleted. If new_sha1 is NULL, then the reference is not + * changed at all. old_sha1 is the value that the reference must have + * before the update, or null_sha1 if it must not have existed + * beforehand. The old value is checked after the lock is taken to + * prevent races. If the old value doesn't agree with old_sha1, the + * whole transaction fails. If old_sha1 is NULL, then the previous + * value is not checked. + * + * Return 0 on success and non-zero on failure. Any failure in the + * transaction means that the transaction as a whole has failed and + * will need to be rolled back. */ int ref_transaction_update(struct ref_transaction *transaction, const char *refname, @@ -308,6 +313,19 @@ int ref_transaction_delete(struct ref_transaction *transaction, unsigned int flags, const char *msg, struct strbuf *err); +/* + * Verify, within a transaction, that refname has the value old_sha1, + * or, if old_sha1 is null_sha1, then verify that the reference + * doesn't exist. old_sha1 must be non-NULL. Function returns 0 on + * success and non-zero on failure. A failure to verify means that the + * transaction as a whole has failed and will need to be rolled back. + */ +int ref_transaction_verify(struct ref_transaction *transaction, + const char *refname, + const unsigned char *old_sha1, + unsigned int flags, + struct strbuf *err); + /* * Commit all of the changes that have been queued in transaction, as * atomically as possible. From 4b7b520b9f761704400a82285d0812fd9e50957f Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Tue, 17 Feb 2015 18:00:22 +0100 Subject: [PATCH 12/13] update_ref(): improve documentation Add a docstring for update_ref(), emphasizing its similarity to ref_transaction_update(). Rename its parameters to match those of ref_transaction_update(). Signed-off-by: Michael Haggerty Reviewed-by: Stefan Beller Signed-off-by: Junio C Hamano --- refs.c | 8 ++++---- refs.h | 13 ++++++++++--- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/refs.c b/refs.c index 1aaba3f7f7..8d46b08055 100644 --- a/refs.c +++ b/refs.c @@ -3738,8 +3738,8 @@ int ref_transaction_verify(struct ref_transaction *transaction, flags, NULL, err); } -int update_ref(const char *action, const char *refname, - const unsigned char *sha1, const unsigned char *oldval, +int update_ref(const char *msg, const char *refname, + const unsigned char *new_sha1, const unsigned char *old_sha1, unsigned int flags, enum action_on_err onerr) { struct ref_transaction *t; @@ -3747,8 +3747,8 @@ int update_ref(const char *action, const char *refname, t = ref_transaction_begin(&err); if (!t || - ref_transaction_update(t, refname, sha1, oldval, - flags, action, &err) || + ref_transaction_update(t, refname, new_sha1, old_sha1, + flags, msg, &err) || ref_transaction_commit(t, &err)) { const char *str = "update_ref failed for ref '%s': %s"; diff --git a/refs.h b/refs.h index 5e139d5f8c..bb9d7b5a02 100644 --- a/refs.h +++ b/refs.h @@ -344,9 +344,16 @@ int ref_transaction_commit(struct ref_transaction *transaction, */ void ref_transaction_free(struct ref_transaction *transaction); -/** Lock a ref and then write its file */ -int update_ref(const char *action, const char *refname, - const unsigned char *sha1, const unsigned char *oldval, +/** + * Lock, update, and unlock a single reference. This function + * basically does a transaction containing a single call to + * ref_transaction_update(). The parameters to this function have the + * same meaning as the corresponding parameters to + * ref_transaction_update(). Handle errors as requested by the `onerr` + * argument. + */ +int update_ref(const char *msg, const char *refname, + const unsigned char *new_sha1, const unsigned char *old_sha1, unsigned int flags, enum action_on_err onerr); extern int parse_hide_refs_config(const char *var, const char *value, const char *); From d1dd721f11b7b124f35e347876e5d7204a3df664 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Tue, 17 Feb 2015 18:00:23 +0100 Subject: [PATCH 13/13] refs.h: remove duplication in function docstrings Add more information to the comment introducing the four reference transaction update functions, so that each function's docstring doesn't have to repeat it. Add a pointer from the individual functions' docstrings to the introductory comment. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.h | 66 ++++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 43 insertions(+), 23 deletions(-) diff --git a/refs.h b/refs.h index bb9d7b5a02..cf642e6ddc 100644 --- a/refs.h +++ b/refs.h @@ -255,11 +255,31 @@ enum action_on_err { struct ref_transaction *ref_transaction_begin(struct strbuf *err); /* - * The following functions add a reference check or update to a - * ref_transaction. In all of them, refname is the name of the - * reference to be affected. The functions make internal copies of - * refname and msg, so the caller retains ownership of these parameters. - * flags can be REF_NODEREF; it is passed to update_ref_lock(). + * Reference transaction updates + * + * The following four functions add a reference check or update to a + * ref_transaction. They have some common similar parameters: + * + * transaction -- a pointer to an open ref_transaction, obtained + * from ref_transaction_begin(). + * + * refname -- the name of the reference to be affected. + * + * flags -- flags affecting the update, passed to + * update_ref_lock(). Can be REF_NODEREF, which means that + * symbolic references should not be followed. + * + * msg -- a message describing the change (for the reflog). + * + * err -- a strbuf for receiving a description of any error that + * might have occured. + * + * The functions make internal copies of refname and msg, so the + * caller retains ownership of these parameters. + * + * The functions return 0 on success and non-zero on failure. A + * failure means that the transaction as a whole has failed and needs + * to be rolled back. */ /* @@ -273,9 +293,8 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err); * whole transaction fails. If old_sha1 is NULL, then the previous * value is not checked. * - * Return 0 on success and non-zero on failure. Any failure in the - * transaction means that the transaction as a whole has failed and - * will need to be rolled back. + * See the above comment "Reference transaction updates" for more + * information. */ int ref_transaction_update(struct ref_transaction *transaction, const char *refname, @@ -285,13 +304,13 @@ int ref_transaction_update(struct ref_transaction *transaction, struct strbuf *err); /* - * Add a reference creation to transaction. new_sha1 is the value - * 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 + * Add a reference creation to transaction. new_sha1 is the value that + * the reference should have after the update; it must not be + * null_sha1. 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. + * + * See the above comment "Reference transaction updates" for more + * information. */ int ref_transaction_create(struct ref_transaction *transaction, const char *refname, @@ -300,12 +319,12 @@ int ref_transaction_create(struct ref_transaction *transaction, struct strbuf *err); /* - * Add a reference deletion to transaction. If old_sha1 is non-NULL, then - * it 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. + * Add a reference deletion to transaction. If old_sha1 is non-NULL, + * then it holds the value that the reference should have had before + * the update (which must not be null_sha1). + * + * See the above comment "Reference transaction updates" for more + * information. */ int ref_transaction_delete(struct ref_transaction *transaction, const char *refname, @@ -316,9 +335,10 @@ int ref_transaction_delete(struct ref_transaction *transaction, /* * Verify, within a transaction, that refname has the value old_sha1, * or, if old_sha1 is null_sha1, then verify that the reference - * doesn't exist. old_sha1 must be non-NULL. Function returns 0 on - * success and non-zero on failure. A failure to verify means that the - * transaction as a whole has failed and will need to be rolled back. + * doesn't exist. old_sha1 must be non-NULL. + * + * See the above comment "Reference transaction updates" for more + * information. */ int ref_transaction_verify(struct ref_transaction *transaction, const char *refname,