{"thread":{"id":"38581","subject":"[PATCH v3 04/13] struct ref_update: move \"have_old\" into \"flags\"","startedAt":"2015-02-17T17:00:10Z","lastAt":"2015-02-17T19:29:04Z","messageCount":15,"participants":["Michael Haggerty","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":13},"messages":[{"id":"256215","messageId":"1424192423-27979-1-git-send-email-mhagger@alum.mit.edu","threadId":"38581","inReplyTo":null,"subject":"[PATCH v3 00/13] Allow reference values to be checked in a transaction","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-17T17:00:10Z","receivedAt":"2015-02-17T17:00:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is v3 of the patch series; I think I have addressed all of the\nfeedback raised about v1 [1] and v2 [2]. Thanks to Stefan Beller and\nJunio for their feedback about v2.\n\nThere are only two significant changes since v2:\n\n* Add a new patch [03/13] that changes a lot of \"flags\" variables from\n  \"int\" to \"unsigned int\".\n\n* Rework t7516:\n  * Remove unneeded \"test_tick\".\n  * Don't pass \"-q\" to \"grep\".\n  * Use different tokens for the two tests to remove ambiguity.\n\nThis branch is also available in my GitHub account [3] as branch\n\"refs-have-new\".\n\nMichael\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/263522\n[2] http://thread.gmane.org/gmane.comp.version-control.git/263718\n[3] https://github.com/mhagger/git\n\nMichael Haggerty (13):\n  refs: move REF_DELETING to refs.c\n  refs: remove the gap in the REF_* constant values\n  refs.c: Change some \"flags\" to \"unsigned int\"\n  struct ref_update: move \"have_old\" into \"flags\"\n  ref_transaction_update(): remove \"have_old\" parameter\n  ref_transaction_delete(): remove \"have_old\" parameter\n  commit: add tests of commit races\n  commit: avoid race when creating orphan commits\n  ref_transaction_create(): check that new_sha1 is valid\n  ref_transaction_delete(): check that old_sha1 is not null_sha1\n  ref_transaction_verify(): new function to check a reference's value\n  update_ref(): improve documentation\n  refs.h: Remove duplication in function docstrings\n\n branch.c                |   5 +-\n builtin/commit.c        |   4 +-\n builtin/fetch.c         |   6 ++-\n builtin/receive-pack.c  |   5 +-\n builtin/replace.c       |   2 +-\n builtin/tag.c           |   2 +-\n builtin/update-ref.c    |  20 +++----\n cache.h                 |   2 +-\n fast-import.c           |   6 +--\n refs.c                  | 140 +++++++++++++++++++++++++++++++++---------------\n refs.h                  | 113 ++++++++++++++++++++++++++------------\n sequencer.c             |   2 +-\n t/t7516-commit-races.sh |  30 +++++++++++\n walker.c                |   2 +-\n 14 files changed, 233 insertions(+), 106 deletions(-)\n create mode 100755 t/t7516-commit-races.sh\n\n-- \n2.1.4\n"},{"id":"256208","messageId":"1424192423-27979-2-git-send-email-mhagger@alum.mit.edu","threadId":"38581","inReplyTo":"1424192423-27979-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 01/13] refs: move REF_DELETING to refs.c","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-17T17:00:11Z","receivedAt":"2015-02-17T17:00:11Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is only used internally now. Document it a little bit better, too.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\nReviewed-by: Stefan Beller <sbeller@google.com>\n---\n refs.c | 6 ++++++\n refs.h | 4 +---\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ab2f2a9..5e6355c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -35,6 +35,12 @@ static unsigned char refname_disposition[256] = {\n };\n \n /*\n+ * Flag passed to lock_ref_sha1_basic() telling it to tolerate broken\n+ * refs (i.e., because the reference is about to be deleted anyway).\n+ */\n+#define REF_DELETING\t0x02\n+\n+/*\n  * Used as a flag to ref_transaction_delete when a loose ref is being\n  * pruned.\n  */\ndiff --git a/refs.h b/refs.h\nindex afa3c4d..9bf2148 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -183,12 +183,10 @@ extern int peel_ref(const char *refname, unsigned char *sha1);\n  * Flags controlling ref_transaction_update(), ref_transaction_create(), etc.\n  * REF_NODEREF: act on the ref directly, instead of dereferencing\n  *              symbolic references.\n- * REF_DELETING: tolerate broken refs\n  *\n- * Flags >= 0x100 are reserved for internal use.\n+ * Other flags are reserved for internal use.\n  */\n #define REF_NODEREF\t0x01\n-#define REF_DELETING\t0x02\n \n /*\n  * Setup reflog before using. Set errno to something meaningful on failure.\n-- \n2.1.4\n"},{"id":"256216","messageId":"1424192423-27979-3-git-send-email-mhagger@alum.mit.edu","threadId":"38581","inReplyTo":"1424192423-27979-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 02/13] refs: remove the gap in the REF_* constant values","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-17T17:00:12Z","receivedAt":"2015-02-17T17:00:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"There is no reason to \"reserve\" a gap between the public and private\nflags values.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5e6355c..4de1383 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -44,7 +44,8 @@ static unsigned char refname_disposition[256] = {\n  * Used as a flag to ref_transaction_delete when a loose ref is being\n  * pruned.\n  */\n-#define REF_ISPRUNING\t0x0100\n+#define REF_ISPRUNING\t0x04\n+\n /*\n  * Try to read one refname component from the front of refname.\n  * Return the length of the component found, or -1 if the component is\n-- \n2.1.4\n"},{"id":"256207","messageId":"1424192423-27979-4-git-send-email-mhagger@alum.mit.edu","threadId":"38581","inReplyTo":"1424192423-27979-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 03/13] refs.c: Change some \"flags\" to \"unsigned int\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-17T17:00:13Z","receivedAt":"2015-02-17T17:00:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change the following functions' \"flags\" arguments from \"int\" to\n\"unsigned int\":\n\n* ref_transaction_update()\n* ref_transaction_create()\n* ref_transaction_delete()\n* update_ref()\n* delete_ref()\n* lock_ref_sha1_basic()\n\nAlso change the \"flags\" member in \"struct ref_update\" to unsigned.\n\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/update-ref.c |  3 ++-\n cache.h              |  2 +-\n refs.c               | 18 +++++++++---------\n refs.h               | 10 +++++-----\n 4 files changed, 17 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 2497ba4..9a1659e 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -353,7 +353,8 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n {\n \tconst char *refname, *oldval;\n \tunsigned char sha1[20], oldsha1[20];\n-\tint delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;\n+\tint delete = 0, no_deref = 0, read_stdin = 0, end_null = 0;\n+\tunsigned int flags = 0;\n \tstruct option options[] = {\n \t\tOPT_STRING( 'm', NULL, &msg, N_(\"reason\"), N_(\"reason of the update\")),\n \t\tOPT_BOOL('d', NULL, &delete, N_(\"delete the reference\")),\ndiff --git a/cache.h b/cache.h\nindex f704af5..ab265be 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -568,7 +568,7 @@ extern void update_index_if_able(struct index_state *, struct lock_file *);\n extern int hold_locked_index(struct lock_file *, int);\n extern void set_alternate_index_output(const char *);\n \n-extern int delete_ref(const char *, const unsigned char *sha1, int delopt);\n+extern int delete_ref(const char *, const unsigned char *sha1, unsigned int flags);\n \n /* Environment bits from configuration mechanism */\n extern int trust_executable_bit;\ndiff --git a/refs.c b/refs.c\nindex 4de1383..a203e44 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2256,7 +2256,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t\t\t    const unsigned char *old_sha1,\n \t\t\t\t\t    const struct string_list *skip,\n-\t\t\t\t\t    int flags, int *type_p)\n+\t\t\t\t\t    unsigned int flags, int *type_p)\n {\n \tchar *ref_file;\n \tconst char *orig_refname = refname;\n@@ -2740,14 +2740,14 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)\n \treturn 0;\n }\n \n-int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n+int delete_ref(const char *refname, const unsigned char *sha1, unsigned int flags)\n {\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n-\t    ref_transaction_delete(transaction, refname, sha1, delopt,\n+\t    ref_transaction_delete(transaction, refname, sha1, flags,\n \t\t\t\t   sha1 && !is_null_sha1(sha1), NULL, &err) ||\n \t    ref_transaction_commit(transaction, &err)) {\n \t\terror(\"%s\", err.buf);\n@@ -3571,7 +3571,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)\n struct ref_update {\n \tunsigned char new_sha1[20];\n \tunsigned char old_sha1[20];\n-\tint flags; /* REF_NODEREF? */\n+\tunsigned int flags; /* REF_NODEREF? */\n \tint have_old; /* 1 if old_sha1 is valid, 0 otherwise */\n \tstruct ref_lock *lock;\n \tint type;\n@@ -3644,7 +3644,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *new_sha1,\n \t\t\t   const unsigned char *old_sha1,\n-\t\t\t   int flags, int have_old, const char *msg,\n+\t\t\t   unsigned int flags, int have_old, const char *msg,\n \t\t\t   struct strbuf *err)\n {\n \tstruct ref_update *update;\n@@ -3678,7 +3678,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n int ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *new_sha1,\n-\t\t\t   int flags, const char *msg,\n+\t\t\t   unsigned int flags, const char *msg,\n \t\t\t   struct strbuf *err)\n {\n \treturn ref_transaction_update(transaction, refname, new_sha1,\n@@ -3688,7 +3688,7 @@ int ref_transaction_create(struct ref_transaction *transaction,\n int ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *old_sha1,\n-\t\t\t   int flags, int have_old, const char *msg,\n+\t\t\t   unsigned int flags, int have_old, const char *msg,\n \t\t\t   struct strbuf *err)\n {\n \treturn ref_transaction_update(transaction, refname, null_sha1,\n@@ -3697,7 +3697,7 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n \n int update_ref(const char *action, const char *refname,\n \t       const unsigned char *sha1, const unsigned char *oldval,\n-\t       int flags, enum action_on_err onerr)\n+\t       unsigned int flags, enum action_on_err onerr)\n {\n \tstruct ref_transaction *t;\n \tstruct strbuf err = STRBUF_INIT;\n@@ -3781,7 +3781,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t/* Acquire all locks while verifying old values */\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n-\t\tint flags = update->flags;\n+\t\tunsigned int flags = update->flags;\n \n \t\tif (is_null_sha1(update->new_sha1))\n \t\t\tflags |= REF_DELETING;\ndiff --git a/refs.h b/refs.h\nindex 9bf2148..92b8597 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -276,7 +276,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *new_sha1,\n \t\t\t   const unsigned char *old_sha1,\n-\t\t\t   int flags, int have_old, const char *msg,\n+\t\t\t   unsigned int flags, int have_old, const char *msg,\n \t\t\t   struct strbuf *err);\n \n /*\n@@ -291,7 +291,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n int ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *new_sha1,\n-\t\t\t   int flags, const char *msg,\n+\t\t\t   unsigned int flags, const char *msg,\n \t\t\t   struct strbuf *err);\n \n /*\n@@ -305,7 +305,7 @@ int ref_transaction_create(struct ref_transaction *transaction,\n int ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *old_sha1,\n-\t\t\t   int flags, int have_old, const char *msg,\n+\t\t\t   unsigned int flags, int have_old, const char *msg,\n \t\t\t   struct strbuf *err);\n \n /*\n@@ -328,8 +328,8 @@ void ref_transaction_free(struct ref_transaction *transaction);\n \n /** Lock a ref and then write its file */\n int update_ref(const char *action, const char *refname,\n-\t\tconst unsigned char *sha1, const unsigned char *oldval,\n-\t\tint flags, enum action_on_err onerr);\n+\t       const unsigned char *sha1, const unsigned char *oldval,\n+\t       unsigned int flags, enum action_on_err onerr);\n \n extern int parse_hide_refs_config(const char *var, const char *value, const char *);\n extern int ref_is_hidden(const char *);\n-- \n2.1.4\n"},{"id":"256206","messageId":"1424192423-27979-5-git-send-email-mhagger@alum.mit.edu","threadId":"38581","inReplyTo":"1424192423-27979-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 04/13] struct ref_update: move \"have_old\" into \"flags\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-17T17:00:14Z","receivedAt":"2015-02-17T17:00:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of having a separate have_old field, record this boolean value\nas a bit in the \"flags\" field.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 45 ++++++++++++++++++++++++++++-----------------\n 1 file changed, 28 insertions(+), 17 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex a203e44..e3a2ba8 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -41,12 +41,18 @@ static unsigned char refname_disposition[256] = {\n #define REF_DELETING\t0x02\n \n /*\n- * Used as a flag to ref_transaction_delete when a loose ref is being\n+ * Used as a flag in ref_update::flags when a loose ref is being\n  * pruned.\n  */\n #define REF_ISPRUNING\t0x04\n \n /*\n+ * Used as a flag in ref_update::flags when old_sha1 should be\n+ * checked.\n+ */\n+#define REF_HAVE_OLD\t0x08\n+\n+/*\n  * Try to read one refname component from the front of refname.\n  * Return the length of the component found, or -1 if the component is\n  * not legal.  It is legal if it is something reasonable to have under\n@@ -3563,16 +3569,20 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)\n }\n \n /**\n- * Information needed for a single ref update.  Set new_sha1 to the\n- * new value or to zero to delete the ref.  To check the old value\n- * while locking the ref, set have_old to 1 and set old_sha1 to the\n- * value or to zero to ensure the ref does not exist before update.\n+ * Information needed for a single ref update. Set new_sha1 to the new\n+ * value or to null_sha1 to delete the ref. To check the old value\n+ * while the ref is locked, set (flags & REF_HAVE_OLD) and set\n+ * old_sha1 to the old value, or to null_sha1 to ensure the ref does\n+ * not exist before update.\n  */\n struct ref_update {\n \tunsigned char new_sha1[20];\n \tunsigned char old_sha1[20];\n-\tunsigned int flags; /* REF_NODEREF? */\n-\tint have_old; /* 1 if old_sha1 is valid, 0 otherwise */\n+\t/*\n+\t * One or more of REF_HAVE_OLD, REF_NODEREF,\n+\t * REF_DELETING, and REF_ISPRUNING:\n+\t */\n+\tunsigned int flags;\n \tstruct ref_lock *lock;\n \tint type;\n \tchar *msg;\n@@ -3666,10 +3676,11 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \n \tupdate = add_update(transaction, refname);\n \thashcpy(update->new_sha1, new_sha1);\n-\tupdate->flags = flags;\n-\tupdate->have_old = have_old;\n-\tif (have_old)\n+\tif (have_old) {\n \t\thashcpy(update->old_sha1, old_sha1);\n+\t\tflags |= REF_HAVE_OLD;\n+\t}\n+\tupdate->flags = flags;\n \tif (msg)\n \t\tupdate->msg = xstrdup(msg);\n \treturn 0;\n@@ -3785,13 +3796,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t\tif (is_null_sha1(update->new_sha1))\n \t\t\tflags |= REF_DELETING;\n-\t\tupdate->lock = lock_ref_sha1_basic(update->refname,\n-\t\t\t\t\t\t   (update->have_old ?\n-\t\t\t\t\t\t    update->old_sha1 :\n-\t\t\t\t\t\t    NULL),\n-\t\t\t\t\t\t   NULL,\n-\t\t\t\t\t\t   flags,\n-\t\t\t\t\t\t   &update->type);\n+\t\tupdate->lock = lock_ref_sha1_basic(\n+\t\t\t\tupdate->refname,\n+\t\t\t\t((update->flags & REF_HAVE_OLD) ?\n+\t\t\t\t update->old_sha1 : NULL),\n+\t\t\t\tNULL,\n+\t\t\t\tflags,\n+\t\t\t\t&update->type);\n \t\tif (!update->lock) {\n \t\t\tret = (errno == ENOTDIR)\n \t\t\t\t? TRANSACTION_NAME_CONFLICT\n-- \n2.1.4\n"},{"id":"256218","messageId":"1424192423-27979-6-git-send-email-mhagger@alum.mit.edu","threadId":"38581","inReplyTo":"1424192423-27979-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 05/13] ref_transaction_update(): remove \"have_old\" parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-17T17:00:15Z","receivedAt":"2015-02-17T17:00:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead, verify the reference's old value if and only if old_sha1 is\nnon-NULL.\n\nref_transaction_delete() will get the same treatment in a moment.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\nReviewed-by: Stefan Beller <sbeller@google.com>\n---\n branch.c               |  5 +++--\n builtin/commit.c       |  2 +-\n builtin/fetch.c        |  6 ++++--\n builtin/receive-pack.c |  2 +-\n builtin/replace.c      |  2 +-\n builtin/tag.c          |  2 +-\n builtin/update-ref.c   |  7 ++++---\n fast-import.c          |  6 +++---\n refs.c                 | 18 ++++++++----------\n refs.h                 |  6 +++---\n sequencer.c            |  2 +-\n walker.c               |  2 +-\n 12 files changed, 31 insertions(+), 29 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 4bab55a..b002435 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -284,8 +284,9 @@ void create_branch(const char *head,\n \n \t\ttransaction = ref_transaction_begin(&err);\n \t\tif (!transaction ||\n-\t\t    ref_transaction_update(transaction, ref.buf, sha1,\n-\t\t\t\t\t   null_sha1, 0, !forcing, msg, &err) ||\n+\t\t    ref_transaction_update(transaction, ref.buf,\n+\t\t\t\t\t   sha1, forcing ? NULL : null_sha1,\n+\t\t\t\t\t   0, msg, &err) ||\n \t\t    ref_transaction_commit(transaction, &err))\n \t\t\tdie(\"%s\", err.buf);\n \t\tref_transaction_free(transaction);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 7f46713..8afb0ff 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1767,7 +1767,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t    ref_transaction_update(transaction, \"HEAD\", sha1,\n \t\t\t\t   current_head\n \t\t\t\t   ? current_head->object.sha1 : NULL,\n-\t\t\t\t   0, !!current_head, sb.buf, &err) ||\n+\t\t\t\t   0, sb.buf, &err) ||\n \t    ref_transaction_commit(transaction, &err)) {\n \t\trollback_index_files();\n \t\tdie(\"%s\", err.buf);\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 7b84d35..719bf4f 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -416,8 +416,10 @@ static int s_update_ref(const char *action,\n \n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n-\t    ref_transaction_update(transaction, ref->name, ref->new_sha1,\n-\t\t\t\t   ref->old_sha1, 0, check_old, msg, &err))\n+\t    ref_transaction_update(transaction, ref->name,\n+\t\t\t\t   ref->new_sha1,\n+\t\t\t\t   check_old ? ref->old_sha1 : NULL,\n+\t\t\t\t   0, msg, &err))\n \t\tgoto fail;\n \n \tret = ref_transaction_commit(transaction, &err);\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e0ce78e..0be50e9 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -971,7 +971,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\tif (ref_transaction_update(transaction,\n \t\t\t\t\t   namespaced_name,\n \t\t\t\t\t   new_sha1, old_sha1,\n-\t\t\t\t\t   0, 1, \"push\",\n+\t\t\t\t\t   0, \"push\",\n \t\t\t\t\t   &err)) {\n \t\t\trp_error(\"%s\", err.buf);\n \t\t\tstrbuf_release(&err);\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 85d39b5..54bf01a 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -172,7 +172,7 @@ static int replace_object_sha1(const char *object_ref,\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t    ref_transaction_update(transaction, ref, repl, prev,\n-\t\t\t\t   0, 1, NULL, &err) ||\n+\t\t\t\t   0, NULL, &err) ||\n \t    ref_transaction_commit(transaction, &err))\n \t\tdie(\"%s\", err.buf);\n \ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 6dc85a9..4194b9a 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -733,7 +733,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t    ref_transaction_update(transaction, ref.buf, object, prev,\n-\t\t\t\t   0, 1, NULL, &err) ||\n+\t\t\t\t   0, NULL, &err) ||\n \t    ref_transaction_commit(transaction, &err))\n \t\tdie(\"%s\", err.buf);\n \tref_transaction_free(transaction);\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 9a1659e..1ad6ce1 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -198,8 +198,9 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n \tif (*next != line_termination)\n \t\tdie(\"update %s: extra input: %s\", refname, next);\n \n-\tif (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n-\t\t\t\t   update_flags, have_old, msg, &err))\n+\tif (ref_transaction_update(transaction, refname,\n+\t\t\t\t   new_sha1, have_old ? old_sha1 : NULL,\n+\t\t\t\t   update_flags, msg, &err))\n \t\tdie(\"%s\", err.buf);\n \n \tupdate_flags = 0;\n@@ -297,7 +298,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n \t\tdie(\"verify %s: extra input: %s\", refname, next);\n \n \tif (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n-\t\t\t\t   update_flags, 1, msg, &err))\n+\t\t\t\t   update_flags, msg, &err))\n \t\tdie(\"%s\", err.buf);\n \n \tupdate_flags = 0;\ndiff --git a/fast-import.c b/fast-import.c\nindex d0bd285..1e72bfb 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1716,7 +1716,7 @@ static int update_branch(struct branch *b)\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t    ref_transaction_update(transaction, b->name, b->sha1, old_sha1,\n-\t\t\t\t   0, 1, msg, &err) ||\n+\t\t\t\t   0, msg, &err) ||\n \t    ref_transaction_commit(transaction, &err)) {\n \t\tref_transaction_free(transaction);\n \t\terror(\"%s\", err.buf);\n@@ -1756,8 +1756,8 @@ static void dump_tags(void)\n \t\tstrbuf_reset(&ref_name);\n \t\tstrbuf_addf(&ref_name, \"refs/tags/%s\", t->name);\n \n-\t\tif (ref_transaction_update(transaction, ref_name.buf, t->sha1,\n-\t\t\t\t\t   NULL, 0, 0, msg, &err)) {\n+\t\tif (ref_transaction_update(transaction, ref_name.buf,\n+\t\t\t\t\t   t->sha1, NULL, 0, msg, &err)) {\n \t\t\tfailure |= error(\"%s\", err.buf);\n \t\t\tgoto cleanup;\n \t\t}\ndiff --git a/refs.c b/refs.c\nindex e3a2ba8..e88817c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3654,7 +3654,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *new_sha1,\n \t\t\t   const unsigned char *old_sha1,\n-\t\t\t   unsigned int flags, int have_old, const char *msg,\n+\t\t\t   unsigned int flags, const char *msg,\n \t\t\t   struct strbuf *err)\n {\n \tstruct ref_update *update;\n@@ -3664,9 +3664,6 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \tif (transaction->state != REF_TRANSACTION_OPEN)\n \t\tdie(\"BUG: update called for transaction that is not open\");\n \n-\tif (have_old && !old_sha1)\n-\t\tdie(\"BUG: have_old is true but old_sha1 is NULL\");\n-\n \tif (!is_null_sha1(new_sha1) &&\n \t    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n \t\tstrbuf_addf(err, \"refusing to update ref with bad name %s\",\n@@ -3676,7 +3673,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \n \tupdate = add_update(transaction, refname);\n \thashcpy(update->new_sha1, new_sha1);\n-\tif (have_old) {\n+\tif (old_sha1) {\n \t\thashcpy(update->old_sha1, old_sha1);\n \t\tflags |= REF_HAVE_OLD;\n \t}\n@@ -3693,7 +3690,7 @@ int ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t   struct strbuf *err)\n {\n \treturn ref_transaction_update(transaction, refname, new_sha1,\n-\t\t\t\t      null_sha1, flags, 1, msg, err);\n+\t\t\t\t      null_sha1, flags, msg, err);\n }\n \n int ref_transaction_delete(struct ref_transaction *transaction,\n@@ -3702,8 +3699,9 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t   unsigned int flags, int have_old, const char *msg,\n \t\t\t   struct strbuf *err)\n {\n-\treturn ref_transaction_update(transaction, refname, null_sha1,\n-\t\t\t\t      old_sha1, flags, have_old, msg, err);\n+\treturn ref_transaction_update(transaction, refname,\n+\t\t\t\t      null_sha1, have_old ? old_sha1 : NULL,\n+\t\t\t\t      flags, msg, err);\n }\n \n int update_ref(const char *action, const char *refname,\n@@ -3715,8 +3713,8 @@ int update_ref(const char *action, const char *refname,\n \n \tt = ref_transaction_begin(&err);\n \tif (!t ||\n-\t    ref_transaction_update(t, refname, sha1, oldval, flags,\n-\t\t\t\t   !!oldval, action, &err) ||\n+\t    ref_transaction_update(t, refname, sha1, oldval,\n+\t\t\t\t   flags, action, &err) ||\n \t    ref_transaction_commit(t, &err)) {\n \t\tconst char *str = \"update_ref failed for ref '%s': %s\";\n \ndiff --git a/refs.h b/refs.h\nindex 92b8597..dced4c9 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -265,8 +265,8 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);\n /*\n  * Add a reference update to transaction.  new_sha1 is the value that\n  * the reference should have after the update, or null_sha1 if it should\n- * be deleted.  If have_old is true, then old_sha1 holds the value\n- * that the reference should have had before the update, or zeros if\n+ * be deleted.  If old_sha1 is non-NULL, then it is the value\n+ * that the reference should have had before the update, or null_sha1 if\n  * it must not have existed beforehand.\n  * Function returns 0 on success and non-zero on failure. A failure to update\n  * means that the transaction as a whole has failed and will need to be\n@@ -276,7 +276,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *new_sha1,\n \t\t\t   const unsigned char *old_sha1,\n-\t\t\t   unsigned int flags, int have_old, const char *msg,\n+\t\t\t   unsigned int flags, const char *msg,\n \t\t\t   struct strbuf *err);\n \n /*\ndiff --git a/sequencer.c b/sequencer.c\nindex 77a1266..32aa05e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -252,7 +252,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \tif (!transaction ||\n \t    ref_transaction_update(transaction, \"HEAD\",\n \t\t\t\t   to, unborn ? null_sha1 : from,\n-\t\t\t\t   0, 1, sb.buf, &err) ||\n+\t\t\t\t   0, sb.buf, &err) ||\n \t    ref_transaction_commit(transaction, &err)) {\n \t\tref_transaction_free(transaction);\n \t\terror(\"%s\", err.buf);\ndiff --git a/walker.c b/walker.c\nindex 483da4e..58ffeca 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -299,7 +299,7 @@ int walker_fetch(struct walker *walker, int targets, char **target,\n \t\tstrbuf_reset(&refname);\n \t\tstrbuf_addf(&refname, \"refs/%s\", write_ref[i]);\n \t\tif (ref_transaction_update(transaction, refname.buf,\n-\t\t\t\t\t   &sha1[20 * i], NULL, 0, 0,\n+\t\t\t\t\t   &sha1[20 * i], NULL, 0,\n \t\t\t\t\t   msg ? msg : \"fetch (unknown)\",\n \t\t\t\t\t   &err)) {\n \t\t\terror(\"%s\", err.buf);\n-- \n2.1.4\n"},{"id":"256209","messageId":"1424192423-27979-7-git-send-email-mhagger@alum.mit.edu","threadId":"38581","inReplyTo":"1424192423-27979-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 06/13] ref_transaction_delete(): remove \"have_old\" parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-17T17:00:16Z","receivedAt":"2015-02-17T17:00:16Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead, verify the reference's old value if and only if old_sha1 is\nnon-NULL.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\nReviewed-by: Stefan Beller <sbeller@google.com>\n---\n builtin/receive-pack.c |  3 +--\n builtin/update-ref.c   |  5 +++--\n refs.c                 | 11 ++++++-----\n refs.h                 |  6 +++---\n 4 files changed, 13 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 0be50e9..70e9ce5 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -953,8 +953,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \t\tif (ref_transaction_delete(transaction,\n \t\t\t\t\t   namespaced_name,\n \t\t\t\t\t   old_sha1,\n-\t\t\t\t\t   0, old_sha1 != NULL,\n-\t\t\t\t\t   \"push\", &err)) {\n+\t\t\t\t\t   0, \"push\", &err)) {\n \t\t\trp_error(\"%s\", err.buf);\n \t\t\tstrbuf_release(&err);\n \t\t\treturn \"failed to delete\";\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 1ad6ce1..226995f 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -265,8 +265,9 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,\n \tif (*next != line_termination)\n \t\tdie(\"delete %s: extra input: %s\", refname, next);\n \n-\tif (ref_transaction_delete(transaction, refname, old_sha1,\n-\t\t\t\t   update_flags, have_old, msg, &err))\n+\tif (ref_transaction_delete(transaction, refname,\n+\t\t\t\t   have_old ? old_sha1 : NULL,\n+\t\t\t\t   update_flags, msg, &err))\n \t\tdie(\"%s\", err.buf);\n \n \tupdate_flags = 0;\ndiff --git a/refs.c b/refs.c\nindex e88817c..e3c4ab5 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2576,7 +2576,7 @@ static void prune_ref(struct ref_to_prune *r)\n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n \t    ref_transaction_delete(transaction, r->name, r->sha1,\n-\t\t\t\t   REF_ISPRUNING, 1, NULL, &err) ||\n+\t\t\t\t   REF_ISPRUNING, NULL, &err) ||\n \t    ref_transaction_commit(transaction, &err)) {\n \t\tref_transaction_free(transaction);\n \t\terror(\"%s\", err.buf);\n@@ -2753,8 +2753,9 @@ int delete_ref(const char *refname, const unsigned char *sha1, unsigned int flag\n \n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n-\t    ref_transaction_delete(transaction, refname, sha1, flags,\n-\t\t\t\t   sha1 && !is_null_sha1(sha1), NULL, &err) ||\n+\t    ref_transaction_delete(transaction, refname,\n+\t\t\t\t   (sha1 && !is_null_sha1(sha1)) ? sha1 : NULL,\n+\t\t\t\t   flags, NULL, &err) ||\n \t    ref_transaction_commit(transaction, &err)) {\n \t\terror(\"%s\", err.buf);\n \t\tref_transaction_free(transaction);\n@@ -3696,11 +3697,11 @@ int ref_transaction_create(struct ref_transaction *transaction,\n int ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *old_sha1,\n-\t\t\t   unsigned int flags, int have_old, const char *msg,\n+\t\t\t   unsigned int flags, const char *msg,\n \t\t\t   struct strbuf *err)\n {\n \treturn ref_transaction_update(transaction, refname,\n-\t\t\t\t      null_sha1, have_old ? old_sha1 : NULL,\n+\t\t\t\t      null_sha1, old_sha1,\n \t\t\t\t      flags, msg, err);\n }\n \ndiff --git a/refs.h b/refs.h\nindex dced4c9..100731d 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -295,8 +295,8 @@ int ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t   struct strbuf *err);\n \n /*\n- * Add a reference deletion to transaction.  If have_old is true, then\n- * old_sha1 holds the value that the reference should have had before\n+ * Add a reference deletion to transaction.  If old_sha1 is non-NULL, then\n+ * it holds the value that the reference should have had before\n  * the update (which must not be the null SHA-1).\n  * Function returns 0 on success and non-zero on failure. A failure to delete\n  * means that the transaction as a whole has failed and will need to be\n@@ -305,7 +305,7 @@ int ref_transaction_create(struct ref_transaction *transaction,\n int ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t   const char *refname,\n \t\t\t   const unsigned char *old_sha1,\n-\t\t\t   unsigned int flags, int have_old, const char *msg,\n+\t\t\t   unsigned int flags, const char *msg,\n \t\t\t   struct strbuf *err);\n \n /*\n-- \n2.1.4\n"},{"id":"256219","messageId":"1424192423-27979-8-git-send-email-mhagger@alum.mit.edu","threadId":"38581","inReplyTo":"1424192423-27979-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 07/13] commit: add tests of commit races","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-17T17:00:17Z","receivedAt":"2015-02-17T17:00:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Committing involves the following steps:\n\n1. Determine the current value of HEAD (if any).\n2. Create the new commit object.\n3. Update HEAD.\n\nPlease note that step 2 can take arbitrarily long, because it might\ninvolve the user editing a commit message.\n\nIf a second process sneaks in a commit during step 2, then the first\ncommit process should fail. This is usually done correctly, because\nstep 3 verifies that HEAD still points at the same commit that it\npointed to during step 1.\n\nHowever, if there is a race when creating an *orphan* commit, then the\ntest in step 3 is skipped.\n\nAdd tests for proper handling of such races. One of the new tests\nfails. It will be fixed in a moment.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n t/t7516-commit-races.sh | 30 ++++++++++++++++++++++++++++++\n 1 file changed, 30 insertions(+)\n create mode 100755 t/t7516-commit-races.sh\n\ndiff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh\nnew file mode 100755\nindex 0000000..ed04d1c\n--- /dev/null\n+++ b/t/t7516-commit-races.sh\n@@ -0,0 +1,30 @@\n+#!/bin/sh\n+\n+test_description='git commit races'\n+. ./test-lib.sh\n+\n+test_expect_failure 'race to create orphan commit' '\n+\twrite_script hare-editor <<-\\EOF &&\n+\tgit commit --allow-empty -m hare\n+\tEOF\n+\ttest_must_fail env EDITOR=./hare-editor git commit --allow-empty -m tortoise -e &&\n+\tgit show -s --pretty=format:%s >subject &&\n+\tgrep hare subject &&\n+\ttest -z \"$(git show -s --pretty=format:%P)\"\n+'\n+\n+test_expect_success 'race to create non-orphan commit' '\n+\twrite_script airplane-editor <<-\\EOF &&\n+\tgit commit --allow-empty -m airplane\n+\tEOF\n+\tgit checkout --orphan branch &&\n+\tgit commit --allow-empty -m base &&\n+\tgit rev-parse HEAD >base &&\n+\ttest_must_fail env EDITOR=./airplane-editor git commit --allow-empty -m ship -e &&\n+\tgit show -s --pretty=format:%s >subject &&\n+\tgrep airplane subject &&\n+\tgit rev-parse HEAD^ >parent &&\n+\ttest_cmp base parent\n+'\n+\n+test_done\n-- \n2.1.4\n"},{"id":"256214","messageId":"1424192423-27979-9-git-send-email-mhagger@alum.mit.edu","threadId":"38581","inReplyTo":"1424192423-27979-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 08/13] commit: avoid race when creating orphan commits","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-17T17:00:18Z","receivedAt":"2015-02-17T17:00:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If HEAD doesn't point at anything during the initial check, then we\nshould make sure that it *still* doesn't point at anything when we are\nready to update the reference. Otherwise, another process might commit\nwhile we are working (e.g., while we are waiting for the user to edit\nthe commit message) and we will silently overwrite it.\n\nThis fixes a failing test in t7516.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\nReviewed-by: Stefan Beller <sbeller@google.com>\n---\n builtin/commit.c        | 2 +-\n t/t7516-commit-races.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 8afb0ff..682f922 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1766,7 +1766,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tif (!transaction ||\n \t    ref_transaction_update(transaction, \"HEAD\", sha1,\n \t\t\t\t   current_head\n-\t\t\t\t   ? current_head->object.sha1 : NULL,\n+\t\t\t\t   ? current_head->object.sha1 : null_sha1,\n \t\t\t\t   0, sb.buf, &err) ||\n \t    ref_transaction_commit(transaction, &err)) {\n \t\trollback_index_files();\ndiff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh\nindex ed04d1c..f2ce14e 100755\n--- a/t/t7516-commit-races.sh\n+++ b/t/t7516-commit-races.sh\n@@ -3,7 +3,7 @@\n test_description='git commit races'\n . ./test-lib.sh\n \n-test_expect_failure 'race to create orphan commit' '\n+test_expect_success 'race to create orphan commit' '\n \twrite_script hare-editor <<-\\EOF &&\n \tgit commit --allow-empty -m hare\n \tEOF\n-- \n2.1.4\n"},{"id":"256217","messageId":"1424192423-27979-10-git-send-email-mhagger@alum.mit.edu","threadId":"38581","inReplyTo":"1424192423-27979-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 09/13] ref_transaction_create(): check that new_sha1 is valid","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-17T17:00:19Z","receivedAt":"2015-02-17T17:00:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Creating a reference requires a new_sha1 that is not NULL and not\nnull_sha1.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\nReviewed-by: Stefan Beller <sbeller@google.com>\n---\n refs.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex e3c4ab5..b9cf284 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3690,6 +3690,8 @@ int ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t   unsigned int flags, const char *msg,\n \t\t\t   struct strbuf *err)\n {\n+\tif (!new_sha1 || is_null_sha1(new_sha1))\n+\t\tdie(\"BUG: create called without valid new_sha1\");\n \treturn ref_transaction_update(transaction, refname, new_sha1,\n \t\t\t\t      null_sha1, flags, msg, err);\n }\n-- \n2.1.4\n"},{"id":"256213","messageId":"1424192423-27979-11-git-send-email-mhagger@alum.mit.edu","threadId":"38581","inReplyTo":"1424192423-27979-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 10/13] ref_transaction_delete(): check that old_sha1 is not null_sha1","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-17T17:00:20Z","receivedAt":"2015-02-17T17:00:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It makes no sense to delete a reference that is already known not to\nexist.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\nReviewed-by: Stefan Beller <sbeller@google.com>\n---\n refs.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex b9cf284..d5bfd11 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3702,6 +3702,8 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t   unsigned int flags, const char *msg,\n \t\t\t   struct strbuf *err)\n {\n+\tif (old_sha1 && is_null_sha1(old_sha1))\n+\t\tdie(\"BUG: delete called with old_sha1 set to zeros\");\n \treturn ref_transaction_update(transaction, refname,\n \t\t\t\t      null_sha1, old_sha1,\n \t\t\t\t      flags, msg, err);\n-- \n2.1.4\n"},{"id":"256210","messageId":"1424192423-27979-12-git-send-email-mhagger@alum.mit.edu","threadId":"38581","inReplyTo":"1424192423-27979-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 11/13] ref_transaction_verify(): new function to check a reference's value","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-17T17:00:21Z","receivedAt":"2015-02-17T17:00:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If NULL is passed to ref_transaction_update()'s new_sha1 parameter,\nthen just verify old_sha1 (under lock) without trying to change the\nnew value of the reference.\n\nUse this functionality to add a new function ref_transaction_verify(),\nwhich checks the current value of the reference under lock but doesn't\nchange it.\n\nUse ref_transaction_verify() in the implementation of \"git update-ref\n--stdin\"'s \"verify\" command to avoid the awkward need to \"update\" the\nreference to its existing value.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\nReviewed-by: Stefan Beller <sbeller@google.com>\n---\n builtin/update-ref.c |  7 ++-----\n refs.c               | 47 +++++++++++++++++++++++++++++++++++++++--------\n refs.h               | 34 ++++++++++++++++++++++++++--------\n 3 files changed, 67 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 226995f..3d79a46 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -282,7 +282,6 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n-\tunsigned char new_sha1[20];\n \tunsigned char old_sha1[20];\n \n \trefname = parse_refname(input, &next);\n@@ -293,13 +292,11 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n \t\t\t    PARSE_SHA1_OLD))\n \t\thashclr(old_sha1);\n \n-\thashcpy(new_sha1, old_sha1);\n-\n \tif (*next != line_termination)\n \t\tdie(\"verify %s: extra input: %s\", refname, next);\n \n-\tif (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n-\t\t\t\t   update_flags, msg, &err))\n+\tif (ref_transaction_verify(transaction, refname, old_sha1,\n+\t\t\t\t   update_flags, &err))\n \t\tdie(\"%s\", err.buf);\n \n \tupdate_flags = 0;\ndiff --git a/refs.c b/refs.c\nindex d5bfd11..1aaba3f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -47,10 +47,16 @@ static unsigned char refname_disposition[256] = {\n #define REF_ISPRUNING\t0x04\n \n /*\n+ * Used as a flag in ref_update::flags when the reference should be\n+ * updated to new_sha1.\n+ */\n+#define REF_HAVE_NEW\t0x08\n+\n+/*\n  * Used as a flag in ref_update::flags when old_sha1 should be\n  * checked.\n  */\n-#define REF_HAVE_OLD\t0x08\n+#define REF_HAVE_OLD\t0x10\n \n /*\n  * Try to read one refname component from the front of refname.\n@@ -3577,10 +3583,17 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)\n  * not exist before update.\n  */\n struct ref_update {\n+\t/*\n+\t * If (flags & REF_HAVE_NEW), set the reference to this value:\n+\t */\n \tunsigned char new_sha1[20];\n+\t/*\n+\t * If (flags & REF_HAVE_OLD), check that the reference\n+\t * previously had this value:\n+\t */\n \tunsigned char old_sha1[20];\n \t/*\n-\t * One or more of REF_HAVE_OLD, REF_NODEREF,\n+\t * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,\n \t * REF_DELETING, and REF_ISPRUNING:\n \t */\n \tunsigned int flags;\n@@ -3665,7 +3678,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \tif (transaction->state != REF_TRANSACTION_OPEN)\n \t\tdie(\"BUG: update called for transaction that is not open\");\n \n-\tif (!is_null_sha1(new_sha1) &&\n+\tif (new_sha1 && !is_null_sha1(new_sha1) &&\n \t    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n \t\tstrbuf_addf(err, \"refusing to update ref with bad name %s\",\n \t\t\t    refname);\n@@ -3673,7 +3686,10 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t}\n \n \tupdate = add_update(transaction, refname);\n-\thashcpy(update->new_sha1, new_sha1);\n+\tif (new_sha1) {\n+\t\thashcpy(update->new_sha1, new_sha1);\n+\t\tflags |= REF_HAVE_NEW;\n+\t}\n \tif (old_sha1) {\n \t\thashcpy(update->old_sha1, old_sha1);\n \t\tflags |= REF_HAVE_OLD;\n@@ -3709,6 +3725,19 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t\t      flags, msg, err);\n }\n \n+int ref_transaction_verify(struct ref_transaction *transaction,\n+\t\t\t   const char *refname,\n+\t\t\t   const unsigned char *old_sha1,\n+\t\t\t   unsigned int flags,\n+\t\t\t   struct strbuf *err)\n+{\n+\tif (!old_sha1)\n+\t\tdie(\"BUG: verify called with old_sha1 set to NULL\");\n+\treturn ref_transaction_update(transaction, refname,\n+\t\t\t\t      NULL, old_sha1,\n+\t\t\t\t      flags, NULL, err);\n+}\n+\n int update_ref(const char *action, const char *refname,\n \t       const unsigned char *sha1, const unsigned char *oldval,\n \t       unsigned int flags, enum action_on_err onerr)\n@@ -3797,7 +3826,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\tstruct ref_update *update = updates[i];\n \t\tunsigned int flags = update->flags;\n \n-\t\tif (is_null_sha1(update->new_sha1))\n+\t\tif ((flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1))\n \t\t\tflags |= REF_DELETING;\n \t\tupdate->lock = lock_ref_sha1_basic(\n \t\t\t\tupdate->refname,\n@@ -3819,8 +3848,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t/* Perform updates first so live commits remain referenced */\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n+\t\tint flags = update->flags;\n \n-\t\tif (!is_null_sha1(update->new_sha1)) {\n+\t\tif ((flags & REF_HAVE_NEW) && !is_null_sha1(update->new_sha1)) {\n \t\t\tif (write_ref_sha1(update->lock, update->new_sha1,\n \t\t\t\t\t   update->msg)) {\n \t\t\t\tupdate->lock = NULL; /* freed by write_ref_sha1 */\n@@ -3836,14 +3866,15 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t/* Perform deletes now that updates are safely completed */\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n+\t\tint flags = update->flags;\n \n-\t\tif (update->lock) {\n+\t\tif ((flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1)) {\n \t\t\tif (delete_ref_loose(update->lock, update->type, err)) {\n \t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\tgoto cleanup;\n \t\t\t}\n \n-\t\t\tif (!(update->flags & REF_ISPRUNING))\n+\t\t\tif (!(flags & REF_ISPRUNING))\n \t\t\t\tstring_list_append(&refs_to_delete,\n \t\t\t\t\t\t   update->lock->ref_name);\n \t\t}\ndiff --git a/refs.h b/refs.h\nindex 100731d..5e139d5 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -263,14 +263,19 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);\n  */\n \n /*\n- * Add a reference update to transaction.  new_sha1 is the value that\n- * the reference should have after the update, or null_sha1 if it should\n- * be deleted.  If old_sha1 is non-NULL, then it is the value\n- * that the reference should have had before the update, or null_sha1 if\n- * it must not have existed beforehand.\n- * Function returns 0 on success and non-zero on failure. A failure to update\n- * means that the transaction as a whole has failed and will need to be\n- * rolled back.\n+ * Add a reference update to transaction. new_sha1 is the value that\n+ * the reference should have after the update, or null_sha1 if it\n+ * should be deleted. If new_sha1 is NULL, then the reference is not\n+ * changed at all. old_sha1 is the value that the reference must have\n+ * before the update, or null_sha1 if it must not have existed\n+ * beforehand. The old value is checked after the lock is taken to\n+ * prevent races. If the old value doesn't agree with old_sha1, the\n+ * whole transaction fails. If old_sha1 is NULL, then the previous\n+ * value is not checked.\n+ *\n+ * Return 0 on success and non-zero on failure. Any failure in the\n+ * transaction means that the transaction as a whole has failed and\n+ * will need to be rolled back.\n  */\n int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t   const char *refname,\n@@ -309,6 +314,19 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t   struct strbuf *err);\n \n /*\n+ * Verify, within a transaction, that refname has the value old_sha1,\n+ * or, if old_sha1 is null_sha1, then verify that the reference\n+ * doesn't exist. old_sha1 must be non-NULL. Function returns 0 on\n+ * success and non-zero on failure. A failure to verify means that the\n+ * transaction as a whole has failed and will need to be rolled back.\n+ */\n+int ref_transaction_verify(struct ref_transaction *transaction,\n+\t\t\t   const char *refname,\n+\t\t\t   const unsigned char *old_sha1,\n+\t\t\t   unsigned int flags,\n+\t\t\t   struct strbuf *err);\n+\n+/*\n  * Commit all of the changes that have been queued in transaction, as\n  * atomically as possible.\n  *\n-- \n2.1.4\n"},{"id":"256211","messageId":"1424192423-27979-13-git-send-email-mhagger@alum.mit.edu","threadId":"38581","inReplyTo":"1424192423-27979-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 12/13] update_ref(): improve documentation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-17T17:00:22Z","receivedAt":"2015-02-17T17:00:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add a docstring for update_ref(), emphasizing its similarity to\nref_transaction_update(). Rename its parameters to match those of\nref_transaction_update().\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\nReviewed-by: Stefan Beller <sbeller@google.com>\n---\n refs.c |  8 ++++----\n refs.h | 13 ++++++++++---\n 2 files changed, 14 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 1aaba3f..8d46b08 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3738,8 +3738,8 @@ int ref_transaction_verify(struct ref_transaction *transaction,\n \t\t\t\t      flags, NULL, err);\n }\n \n-int update_ref(const char *action, const char *refname,\n-\t       const unsigned char *sha1, const unsigned char *oldval,\n+int update_ref(const char *msg, const char *refname,\n+\t       const unsigned char *new_sha1, const unsigned char *old_sha1,\n \t       unsigned int flags, enum action_on_err onerr)\n {\n \tstruct ref_transaction *t;\n@@ -3747,8 +3747,8 @@ int update_ref(const char *action, const char *refname,\n \n \tt = ref_transaction_begin(&err);\n \tif (!t ||\n-\t    ref_transaction_update(t, refname, sha1, oldval,\n-\t\t\t\t   flags, action, &err) ||\n+\t    ref_transaction_update(t, refname, new_sha1, old_sha1,\n+\t\t\t\t   flags, msg, &err) ||\n \t    ref_transaction_commit(t, &err)) {\n \t\tconst char *str = \"update_ref failed for ref '%s': %s\";\n \ndiff --git a/refs.h b/refs.h\nindex 5e139d5..bb9d7b5 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -344,9 +344,16 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n  */\n void ref_transaction_free(struct ref_transaction *transaction);\n \n-/** Lock a ref and then write its file */\n-int update_ref(const char *action, const char *refname,\n-\t       const unsigned char *sha1, const unsigned char *oldval,\n+/**\n+ * Lock, update, and unlock a single reference. This function\n+ * basically does a transaction containing a single call to\n+ * ref_transaction_update(). The parameters to this function have the\n+ * same meaning as the corresponding parameters to\n+ * ref_transaction_update(). Handle errors as requested by the `onerr`\n+ * argument.\n+ */\n+int update_ref(const char *msg, const char *refname,\n+\t       const unsigned char *new_sha1, const unsigned char *old_sha1,\n \t       unsigned int flags, enum action_on_err onerr);\n \n extern int parse_hide_refs_config(const char *var, const char *value, const char *);\n-- \n2.1.4\n"},{"id":"256212","messageId":"1424192423-27979-14-git-send-email-mhagger@alum.mit.edu","threadId":"38581","inReplyTo":"1424192423-27979-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 13/13] refs.h: Remove duplication in function docstrings","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-17T17:00:23Z","receivedAt":"2015-02-17T17:00:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add more information to the comment introducing the four reference\ntransaction update functions, so that each function's docstring\ndoesn't have to repeat it. Add a pointer from the individual\nfunctions' docstrings to the introductory comment.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.h | 66 +++++++++++++++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 43 insertions(+), 23 deletions(-)\n\ndiff --git a/refs.h b/refs.h\nindex bb9d7b5..cf642e6 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -255,11 +255,31 @@ enum action_on_err {\n struct ref_transaction *ref_transaction_begin(struct strbuf *err);\n \n /*\n- * The following functions add a reference check or update to a\n- * ref_transaction.  In all of them, refname is the name of the\n- * reference to be affected.  The functions make internal copies of\n- * refname and msg, so the caller retains ownership of these parameters.\n- * flags can be REF_NODEREF; it is passed to update_ref_lock().\n+ * Reference transaction updates\n+ *\n+ * The following four functions add a reference check or update to a\n+ * ref_transaction.  They have some common similar parameters:\n+ *\n+ *     transaction -- a pointer to an open ref_transaction, obtained\n+ *         from ref_transaction_begin().\n+ *\n+ *     refname -- the name of the reference to be affected.\n+ *\n+ *     flags -- flags affecting the update, passed to\n+ *         update_ref_lock(). Can be REF_NODEREF, which means that\n+ *         symbolic references should not be followed.\n+ *\n+ *     msg -- a message describing the change (for the reflog).\n+ *\n+ *     err -- a strbuf for receiving a description of any error that\n+ *         might have occured.\n+ *\n+ * The functions make internal copies of refname and msg, so the\n+ * caller retains ownership of these parameters.\n+ *\n+ * The functions return 0 on success and non-zero on failure. A\n+ * failure means that the transaction as a whole has failed and needs\n+ * to be rolled back.\n  */\n \n /*\n@@ -273,9 +293,8 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);\n  * whole transaction fails. If old_sha1 is NULL, then the previous\n  * value is not checked.\n  *\n- * Return 0 on success and non-zero on failure. Any failure in the\n- * transaction means that the transaction as a whole has failed and\n- * will need to be rolled back.\n+ * See the above comment \"Reference transaction updates\" for more\n+ * information.\n  */\n int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t   const char *refname,\n@@ -285,13 +304,13 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t   struct strbuf *err);\n \n /*\n- * Add a reference creation to transaction.  new_sha1 is the value\n- * that the reference should have after the update; it must not be the\n- * null SHA-1.  It is verified that the reference does not exist\n+ * Add a reference creation to transaction. new_sha1 is the value that\n+ * the reference should have after the update; it must not be\n+ * null_sha1. It is verified that the reference does not exist\n  * already.\n- * Function returns 0 on success and non-zero on failure. A failure to create\n- * means that the transaction as a whole has failed and will need to be\n- * rolled back.\n+ *\n+ * See the above comment \"Reference transaction updates\" for more\n+ * information.\n  */\n int ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t   const char *refname,\n@@ -300,12 +319,12 @@ int ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t   struct strbuf *err);\n \n /*\n- * Add a reference deletion to transaction.  If old_sha1 is non-NULL, then\n- * it holds the value that the reference should have had before\n- * the update (which must not be the null SHA-1).\n- * Function returns 0 on success and non-zero on failure. A failure to delete\n- * means that the transaction as a whole has failed and will need to be\n- * rolled back.\n+ * Add a reference deletion to transaction. If old_sha1 is non-NULL,\n+ * then it holds the value that the reference should have had before\n+ * the update (which must not be null_sha1).\n+ *\n+ * See the above comment \"Reference transaction updates\" for more\n+ * information.\n  */\n int ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t   const char *refname,\n@@ -316,9 +335,10 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n /*\n  * Verify, within a transaction, that refname has the value old_sha1,\n  * or, if old_sha1 is null_sha1, then verify that the reference\n- * doesn't exist. old_sha1 must be non-NULL. Function returns 0 on\n- * success and non-zero on failure. A failure to verify means that the\n- * transaction as a whole has failed and will need to be rolled back.\n+ * doesn't exist. old_sha1 must be non-NULL.\n+ *\n+ * See the above comment \"Reference transaction updates\" for more\n+ * information.\n  */\n int ref_transaction_verify(struct ref_transaction *transaction,\n \t\t\t   const char *refname,\n-- \n2.1.4\n"},{"id":"256228","messageId":"xmqqbnks2v7j.fsf@gitster.dls.corp.google.com","threadId":"38581","inReplyTo":"1424192423-27979-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v3 00/13] Allow reference values to be checked in a transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-17T19:29:04Z","receivedAt":"2015-02-17T19:29:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"All looked sensible from a cursory read.\n\nWill replace, wait for a few days and will merge to 'next' unless I\nhear otherwise from others.\n\nThanks.\n"}]}