{"thread":{"id":"38528","subject":"[PATCH 05/11] ref_transaction_delete(): remove \"have_old\" parameter","startedAt":"2015-02-08T16:13:54Z","lastAt":"2015-02-11T18:54:38Z","messageCount":35,"participants":["Michael Haggerty","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"255775","messageId":"1423412045-15616-1-git-send-email-mhagger@alum.mit.edu","threadId":"38528","inReplyTo":null,"subject":"[PATCH 00/11] Allow reference values to be checked in a transaction","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-08T16:13:54Z","receivedAt":"2015-02-08T16:13:54Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The main purpose of this series is to simplify the interface to\nreference transactions as follows:\n\n* Remove the need to supply an explicit have_old parameter to\n ref_transaction_update() and ref_transaction_delete(). Instead,\n check the old_sha1 if and only if it is non-NULL.\n\n* Allow NULL to be supplied to ref_transaction_update() as new_sha1,\n in which case old_sha1 will be verified under lock, but the\n reference's value will not be altered.\n\n* Add a function ref_transaction_verify(), which verifies the current\n value of a reference without changing it.\n\n* Make the similarity between ref_transaction_update() and\n update_ref() more obvious.\n\nAlong the way, it fixes a race that could happen if two processes try\nto create an orphan commit at the same time.\n\nThis patch series applies on top of master merged together with\nsb/atomic-push, which in turn depends on mh/reflog-expire. It is also\navailable from my GitHub account [1] as branch \"refs-have-new\":\n\nIt's nothing earth-shattering, but I think it is a worthwhile cleanup.\n\nMichael\n\n[1] https://github.com/mhagger/git\n\nMichael Haggerty (11):\n refs: move REF_DELETING to refs.c\n refs: remove the gap in the REF_* constant values\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\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 | 17 +++----\n fast-import.c | 6 +--\n refs.c | 130 +++++++++++++++++++++++++++++++++---------------\n refs.h | 61 ++++++++++++++++-------\n sequencer.c | 2 +-\n t/t7516-commit-races.sh | 38 ++++++++++++++\n walker.c | 2 +-\n 13 files changed, 197 insertions(+), 83 deletions(-)\n create mode 100755 t/t7516-commit-races.sh\n\n-- \n2.1.4\n"},{"id":"255774","messageId":"1423412045-15616-2-git-send-email-mhagger@alum.mit.edu","threadId":"38528","inReplyTo":"1423412045-15616-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 01/11] refs: move REF_DELETING to refs.c","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-08T16:13:55Z","receivedAt":"2015-02-08T16:13:55Z","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 \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 c5fa709..cd5208b 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":"255776","messageId":"1423412045-15616-3-git-send-email-mhagger@alum.mit.edu","threadId":"38528","inReplyTo":"1423412045-15616-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 02/11] refs: remove the gap in the REF_* constant values","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-08T16:13:56Z","receivedAt":"2015-02-08T16:13:56Z","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 \n---\n refs.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex cd5208b..477a5b3 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":"255777","messageId":"1423412045-15616-4-git-send-email-mhagger@alum.mit.edu","threadId":"38528","inReplyTo":"1423412045-15616-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 03/11] struct ref_update: move \"have_old\" into \"flags\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-08T16:13:57Z","receivedAt":"2015-02-08T16:13:57Z","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 \n---\n refs.c | 45 ++++++++++++++++++++++++++++-----------------\n 1 file changed, 28 insertions(+), 17 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 477a5b3..851c9f8 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@@ -3564,16 +3570,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-\tint 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_IS_PRUNING:\n+\t */\n+\tint flags;\n \tstruct ref_lock *lock;\n \tint type;\n \tchar *msg;\n@@ -3667,10 +3677,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@@ -3786,13 +3797,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":"255778","messageId":"1423412045-15616-5-git-send-email-mhagger@alum.mit.edu","threadId":"38528","inReplyTo":"1423412045-15616-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 04/11] ref_transaction_update(): remove \"have_old\" parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-08T16:13:58Z","receivedAt":"2015-02-08T16:13:58Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead, if old_sha1 is non-NULL, verify it; otherwise, don't.\n\nref_transaction_delete() will get the same treatment in a moment.\n\nSigned-off-by: Michael Haggerty \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 7d90c35..5654abd 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1773,7 +1773,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 4e85e25..71be82e 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -951,7 +951,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 e633f4e..232c28c 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 1993529..f0db7a3 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 851c9f8..6beafcf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3655,7 +3655,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 int flags, const char *msg,\n \t\t\t struct strbuf *err)\n {\n \tstruct ref_update *update;\n@@ -3665,9 +3665,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@@ -3677,7 +3674,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@@ -3694,7 +3691,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@@ -3703,8 +3700,9 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t 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@@ -3716,8 +3714,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 9bf2148..f05ac89 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 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 int flags, int have_old, const char *msg,\n+\t\t\t 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 f149371..34881b0 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":"255768","messageId":"1423412045-15616-6-git-send-email-mhagger@alum.mit.edu","threadId":"38528","inReplyTo":"1423412045-15616-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 05/11] ref_transaction_delete(): remove \"have_old\" parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-08T16:13:59Z","receivedAt":"2015-02-08T16:13:59Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead, if old_sha1 is non-NULL, verify it; otherwise, don't.\n\nSigned-off-by: Michael Haggerty \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 71be82e..b08806d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -933,8 +933,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 f0db7a3..76b8aef 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 6beafcf..6bd6581 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2577,7 +2577,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@@ -2754,8 +2754,9 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction ||\n-\t ref_transaction_delete(transaction, refname, sha1, delopt,\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 delopt, NULL, &err) ||\n \t ref_transaction_commit(transaction, &err)) {\n \t\terror(\"%s\", err.buf);\n \t\tref_transaction_free(transaction);\n@@ -3697,11 +3698,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 int flags, int have_old, const char *msg,\n+\t\t\t 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 f05ac89..ee612da 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 int flags, int have_old, const char *msg,\n+\t\t\t int flags, const char *msg,\n \t\t\t struct strbuf *err);\n \n /*\n-- \n2.1.4\n"},{"id":"255769","messageId":"1423412045-15616-7-git-send-email-mhagger@alum.mit.edu","threadId":"38528","inReplyTo":"1423412045-15616-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 06/11] commit: add tests of commit races","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-08T16:14:00Z","receivedAt":"2015-02-08T16:14:00Z","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 \n---\n t/t7516-commit-races.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 38 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..5efa351\n--- /dev/null\n+++ b/t/t7516-commit-races.sh\n@@ -0,0 +1,38 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2014 Michael Haggerty \n+#\n+\n+test_description='git commit races'\n+. ./test-lib.sh\n+\n+test_tick\n+\n+test_expect_success 'set up editor' '\n+\tcat >editor <<-\\EOF &&\n+\t#!/bin/sh\n+\tgit commit --allow-empty -m hare\n+\techo tortoise >\"$1\"\n+\tEOF\n+\tchmod +x editor\n+'\n+\n+test_expect_failure 'race to create orphan commit' '\n+\ttest_must_fail env EDITOR=./editor git commit --allow-empty &&\n+\tgit show -s --pretty=format:%s >subject &&\n+\tgrep -q hare subject &&\n+\ttest -z \"$(git show -s --pretty=format:%P)\"\n+'\n+\n+test_expect_success 'race to create non-orphan commit' '\n+\tgit checkout --orphan branch &&\n+\tgit commit --allow-empty -m base &&\n+\tgit rev-parse HEAD >base &&\n+\ttest_must_fail env EDITOR=./editor git commit --allow-empty &&\n+\tgit show -s --pretty=format:%s >subject &&\n+\tgrep -q hare subject &&\n+\tgit rev-parse HEAD^ >parent &&\n+\ttest_cmp base parent\n+'\n+\n+test_done\n-- \n2.1.4\n"},{"id":"255773","messageId":"1423412045-15616-8-git-send-email-mhagger@alum.mit.edu","threadId":"38528","inReplyTo":"1423412045-15616-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 07/11] commit: avoid race when creating orphan commits","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-08T16:14:01Z","receivedAt":"2015-02-08T16:14:01Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If, during the initial check, HEAD doesn't point at anything, 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 \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 5654abd..c566e32 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1772,7 +1772,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 5efa351..17a139e 100755\n--- a/t/t7516-commit-races.sh\n+++ b/t/t7516-commit-races.sh\n@@ -17,7 +17,7 @@ test_expect_success 'set up editor' '\n \tchmod +x editor\n '\n \n-test_expect_failure 'race to create orphan commit' '\n+test_expect_success 'race to create orphan commit' '\n \ttest_must_fail env EDITOR=./editor git commit --allow-empty &&\n \tgit show -s --pretty=format:%s >subject &&\n \tgrep -q hare subject &&\n-- \n2.1.4\n"},{"id":"255770","messageId":"1423412045-15616-9-git-send-email-mhagger@alum.mit.edu","threadId":"38528","inReplyTo":"1423412045-15616-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 08/11] ref_transaction_create(): check that new_sha1 is valid","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-08T16:14:02Z","receivedAt":"2015-02-08T16:14:02Z","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 \n---\n refs.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex 6bd6581..8ab1f8e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3691,6 +3691,8 @@ int ref_transaction_create(struct ref_transaction *transaction,\n \t\t\t 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":"255771","messageId":"1423412045-15616-10-git-send-email-mhagger@alum.mit.edu","threadId":"38528","inReplyTo":"1423412045-15616-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 09/11] ref_transaction_delete(): check that old_sha1 is not null_sha1","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-08T16:14:03Z","receivedAt":"2015-02-08T16:14:03Z","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 \n---\n refs.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex 8ab1f8e..85815d7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3703,6 +3703,8 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n \t\t\t 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":"255772","messageId":"1423412045-15616-11-git-send-email-mhagger@alum.mit.edu","threadId":"38528","inReplyTo":"1423412045-15616-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 10/11] ref_transaction_verify(): new function to check a reference's value","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-08T16:14:04Z","receivedAt":"2015-02-08T16:14:04Z","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 \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 76b8aef..a2eedba 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 85815d7..0da680e 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@@ -3578,10 +3584,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_IS_PRUNING:\n \t */\n \tint flags;\n@@ -3666,7 +3679,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@@ -3674,7 +3687,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@@ -3710,6 +3726,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 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 int flags, enum action_on_err onerr)\n@@ -3798,7 +3827,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\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\tflags |= REF_DELETING;\n \t\tupdate->lock = lock_ref_sha1_basic(\n \t\t\t\tupdate->refname,\n@@ -3820,8 +3849,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@@ -3837,14 +3867,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 ee612da..02d6e39 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 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 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":"255779","messageId":"1423412045-15616-12-git-send-email-mhagger@alum.mit.edu","threadId":"38528","inReplyTo":"1423412045-15616-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 11/11] update_ref(): improve documentation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-08T16:14:05Z","receivedAt":"2015-02-08T16:14:05Z","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 \n---\n refs.c | 8 ++++----\n refs.h | 15 +++++++++++----\n 2 files changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 0da680e..2651583 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3739,8 +3739,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 int flags, enum action_on_err onerr)\n {\n \tstruct ref_transaction *t;\n@@ -3748,8 +3748,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 02d6e39..08abd2c 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -344,10 +344,17 @@ 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\tconst unsigned char *sha1, const unsigned char *oldval,\n-\t\tint flags, enum action_on_err onerr);\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 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":"255821","messageId":"CAGZ79kb1xYBCGALM99f2L1pVKYyrpstDfgaVneHmW25=9_ZVhA@mail.gmail.com","threadId":"38528","inReplyTo":"1423412045-15616-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 01/11] refs: move REF_DELETING to refs.c","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-09T18:09:01Z","receivedAt":"2015-02-09T18:09:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Feb 8, 2015 at 8:13 AM, Michael Haggerty wrote:\n> It is only used internally now. Document it a little bit better, too.\n>\n> Signed-off-by: Michael Haggerty \nReviewed-by: Stefan Beller \n"},{"id":"255822","messageId":"CAGZ79kbcgv236MxzNpQ3H6E9y-v_SYaYettzXUjr9311DyF-ig@mail.gmail.com","threadId":"38528","inReplyTo":"1423412045-15616-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 02/11] refs: remove the gap in the REF_* constant values","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-09T18:14:17Z","receivedAt":"2015-02-09T18:14:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Feb 8, 2015 at 8:13 AM, Michael Haggerty wrote:\n> There is no reason to \"reserve\" a gap between the public and private\n> flags values.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> refs.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/refs.c b/refs.c\n> index cd5208b..477a5b3 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 0x0100\n> +#define REF_ISPRUNING 0x04\n> +\n\nI'm ok with that change, though I remember Duy had once started to\ncreate a table in a header file documenting all positions of flags\n(e.g foo.c would use 1<<{1,2,3,7}, and bar.c had 1<<{1,2,5,26} in\nuse).\nI wonder if we should start a discussion on that topic. I think it\nmakes sense to have a central place documenting all the flags\n(specially their bit allocation), but this one is only in one c file,\nso we're really unlikely to\nbreak something by having different defines pointing at the same bit.\n"},{"id":"255823","messageId":"CAGZ79kbbRdqd+EnJD3Opfc5Luf-1FnEarUpTnXAiKoxh-pqc1Q@mail.gmail.com","threadId":"38528","inReplyTo":"1423412045-15616-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 04/11] ref_transaction_update(): remove \"have_old\" parameter","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-09T18:20:16Z","receivedAt":"2015-02-09T18:20:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Feb 8, 2015 at 8:13 AM, Michael Haggerty wrote:\n> Instead, if old_sha1 is non-NULL, verify it; otherwise, don't.\n\nparsing error on that commit message. I needed to read the code to understand\nwhat you want to say here.\nThe code itself is\n\nReviewed-by: Stefan Beller \n\n>\n> ref_transaction_delete() will get the same treatment in a moment.\n>\n> Signed-off-by: Michael Haggerty \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>\n> diff --git a/branch.c b/branch.c\n> index 4bab55a..b002435 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -284,8 +284,9 @@ void create_branch(const char *head,\n>\n> transaction = ref_transaction_begin(&err);\n> if (!transaction ||\n> - ref_transaction_update(transaction, ref.buf, sha1,\n> - null_sha1, 0, !forcing, msg, &err) ||\n> + ref_transaction_update(transaction, ref.buf,\n> + sha1, forcing ? NULL : null_sha1,\n> + 0, msg, &err) ||\n> ref_transaction_commit(transaction, &err))\n> die(\"%s\", err.buf);\n> ref_transaction_free(transaction);\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 7d90c35..5654abd 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1773,7 +1773,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> ref_transaction_update(transaction, \"HEAD\", sha1,\n> current_head\n> ? current_head->object.sha1 : NULL,\n> - 0, !!current_head, sb.buf, &err) ||\n> + 0, sb.buf, &err) ||\n> ref_transaction_commit(transaction, &err)) {\n> rollback_index_files();\n> die(\"%s\", err.buf);\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 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> transaction = ref_transaction_begin(&err);\n> if (!transaction ||\n> - ref_transaction_update(transaction, ref->name, ref->new_sha1,\n> - ref->old_sha1, 0, check_old, msg, &err))\n> + ref_transaction_update(transaction, ref->name,\n> + ref->new_sha1,\n> + check_old ? ref->old_sha1 : NULL,\n> + 0, msg, &err))\n> goto fail;\n>\n> ret = ref_transaction_commit(transaction, &err);\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 4e85e25..71be82e 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -951,7 +951,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n> if (ref_transaction_update(transaction,\n> namespaced_name,\n> new_sha1, old_sha1,\n> - 0, 1, \"push\",\n> + 0, \"push\",\n> &err)) {\n> rp_error(\"%s\", err.buf);\n> strbuf_release(&err);\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index 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> transaction = ref_transaction_begin(&err);\n> if (!transaction ||\n> ref_transaction_update(transaction, ref, repl, prev,\n> - 0, 1, NULL, &err) ||\n> + 0, NULL, &err) ||\n> ref_transaction_commit(transaction, &err))\n> die(\"%s\", err.buf);\n>\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index e633f4e..232c28c 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> transaction = ref_transaction_begin(&err);\n> if (!transaction ||\n> ref_transaction_update(transaction, ref.buf, object, prev,\n> - 0, 1, NULL, &err) ||\n> + 0, NULL, &err) ||\n> ref_transaction_commit(transaction, &err))\n> die(\"%s\", err.buf);\n> ref_transaction_free(transaction);\n> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n> index 1993529..f0db7a3 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> if (*next != line_termination)\n> die(\"update %s: extra input: %s\", refname, next);\n>\n> - if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n> - update_flags, have_old, msg, &err))\n> + if (ref_transaction_update(transaction, refname,\n> + new_sha1, have_old ? old_sha1 : NULL,\n> + update_flags, msg, &err))\n> die(\"%s\", err.buf);\n>\n> update_flags = 0;\n> @@ -297,7 +298,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n> die(\"verify %s: extra input: %s\", refname, next);\n>\n> if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,\n> - update_flags, 1, msg, &err))\n> + update_flags, msg, &err))\n> die(\"%s\", err.buf);\n>\n> update_flags = 0;\n> diff --git a/fast-import.c b/fast-import.c\n> index 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> transaction = ref_transaction_begin(&err);\n> if (!transaction ||\n> ref_transaction_update(transaction, b->name, b->sha1, old_sha1,\n> - 0, 1, msg, &err) ||\n> + 0, msg, &err) ||\n> ref_transaction_commit(transaction, &err)) {\n> ref_transaction_free(transaction);\n> error(\"%s\", err.buf);\n> @@ -1756,8 +1756,8 @@ static void dump_tags(void)\n> strbuf_reset(&ref_name);\n> strbuf_addf(&ref_name, \"refs/tags/%s\", t->name);\n>\n> - if (ref_transaction_update(transaction, ref_name.buf, t->sha1,\n> - NULL, 0, 0, msg, &err)) {\n> + if (ref_transaction_update(transaction, ref_name.buf,\n> + t->sha1, NULL, 0, msg, &err)) {\n> failure |= error(\"%s\", err.buf);\n> goto cleanup;\n> }\n> diff --git a/refs.c b/refs.c\n> index 851c9f8..6beafcf 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3655,7 +3655,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n> const char *refname,\n> const unsigned char *new_sha1,\n> const unsigned char *old_sha1,\n> - int flags, int have_old, const char *msg,\n> + int flags, const char *msg,\n> struct strbuf *err)\n> {\n> struct ref_update *update;\n> @@ -3665,9 +3665,6 @@ int ref_transaction_update(struct ref_transaction *transaction,\n> if (transaction->state != REF_TRANSACTION_OPEN)\n> die(\"BUG: update called for transaction that is not open\");\n>\n> - if (have_old && !old_sha1)\n> - die(\"BUG: have_old is true but old_sha1 is NULL\");\n> -\n> if (!is_null_sha1(new_sha1) &&\n> check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n> strbuf_addf(err, \"refusing to update ref with bad name %s\",\n> @@ -3677,7 +3674,7 @@ int ref_transaction_update(struct ref_transaction *transaction,\n>\n> update = add_update(transaction, refname);\n> hashcpy(update->new_sha1, new_sha1);\n> - if (have_old) {\n> + if (old_sha1) {\n> hashcpy(update->old_sha1, old_sha1);\n> flags |= REF_HAVE_OLD;\n> }\n> @@ -3694,7 +3691,7 @@ int ref_transaction_create(struct ref_transaction *transaction,\n> struct strbuf *err)\n> {\n> return ref_transaction_update(transaction, refname, new_sha1,\n> - null_sha1, flags, 1, msg, err);\n> + null_sha1, flags, msg, err);\n> }\n>\n> int ref_transaction_delete(struct ref_transaction *transaction,\n> @@ -3703,8 +3700,9 @@ int ref_transaction_delete(struct ref_transaction *transaction,\n> int flags, int have_old, const char *msg,\n> struct strbuf *err)\n> {\n> - return ref_transaction_update(transaction, refname, null_sha1,\n> - old_sha1, flags, have_old, msg, err);\n> + return ref_transaction_update(transaction, refname,\n> + null_sha1, have_old ? old_sha1 : NULL,\n> + flags, msg, err);\n> }\n>\n> int update_ref(const char *action, const char *refname,\n> @@ -3716,8 +3714,8 @@ int update_ref(const char *action, const char *refname,\n>\n> t = ref_transaction_begin(&err);\n> if (!t ||\n> - ref_transaction_update(t, refname, sha1, oldval, flags,\n> - !!oldval, action, &err) ||\n> + ref_transaction_update(t, refname, sha1, oldval,\n> + flags, action, &err) ||\n> ref_transaction_commit(t, &err)) {\n> const char *str = \"update_ref failed for ref '%s': %s\";\n>\n> diff --git a/refs.h b/refs.h\n> index 9bf2148..f05ac89 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 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> const char *refname,\n> const unsigned char *new_sha1,\n> const unsigned char *old_sha1,\n> - int flags, int have_old, const char *msg,\n> + int flags, const char *msg,\n> struct strbuf *err);\n>\n> /*\n> diff --git a/sequencer.c b/sequencer.c\n> index 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> if (!transaction ||\n> ref_transaction_update(transaction, \"HEAD\",\n> to, unborn ? null_sha1 : from,\n> - 0, 1, sb.buf, &err) ||\n> + 0, sb.buf, &err) ||\n> ref_transaction_commit(transaction, &err)) {\n> ref_transaction_free(transaction);\n> error(\"%s\", err.buf);\n> diff --git a/walker.c b/walker.c\n> index f149371..34881b0 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> strbuf_reset(&refname);\n> strbuf_addf(&refname, \"refs/%s\", write_ref[i]);\n> if (ref_transaction_update(transaction, refname.buf,\n> - &sha1[20 * i], NULL, 0, 0,\n> + &sha1[20 * i], NULL, 0,\n> msg ? msg : \"fetch (unknown)\",\n> &err)) {\n> error(\"%s\", err.buf);\n> --\n> 2.1.4\n>\n"},{"id":"255824","messageId":"CAGZ79kZNrFWvNJU_hmHmaMpYW4AZs3vEhKtzHEH+DMKgww7dTQ@mail.gmail.com","threadId":"38528","inReplyTo":"1423412045-15616-6-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 05/11] ref_transaction_delete(): remove \"have_old\" parameter","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-09T18:22:23Z","receivedAt":"2015-02-09T18:22:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Feb 8, 2015 at 8:13 AM, Michael Haggerty wrote:\n> Instead, if old_sha1 is non-NULL, verify it; otherwise, don't.\n>\n> Signed-off-by: Michael Haggerty \n\nReviewed-by: Stefan Beller apart from the commit message.\n"},{"id":"255826","messageId":"CAGZ79kYmgeXt1k22h3fbDR04BTHOQRxryNVSJDOL2DC5yLLHpw@mail.gmail.com","threadId":"38528","inReplyTo":"1423412045-15616-7-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 06/11] commit: add tests of commit races","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-09T18:31:59Z","receivedAt":"2015-02-09T18:31:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Feb 8, 2015 at 8:14 AM, Michael Haggerty wrote:\n\n> diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh\n> new file mode 100755\n> index 0000000..5efa351\n> --- /dev/null\n> +++ b/t/t7516-commit-races.sh\n> @@ -0,0 +1,38 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2014 Michael Haggerty \n\nWhat is the projects stance on copyright lines?\nI've seen files (most of them from the beginning) having some copyright lines,\nother files (often introduced way later) not having them, \"because\nwe're git and have\nhistory, so we know who did it\".\n\nThe tests themselves look fine.\n\nIs there a reason you did not append the tests in 7509 ?\n\n\n>\n"},{"id":"255827","messageId":"CAGZ79kYF2WWqSq71d5FZvP7PCKra-cmYHPAB0RVFed8Ag14ZWA@mail.gmail.com","threadId":"38528","inReplyTo":"1423412045-15616-8-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 07/11] commit: avoid race when creating orphan commits","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-09T18:35:17Z","receivedAt":"2015-02-09T18:35:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Feb 8, 2015 at 8:14 AM, Michael Haggerty wrote:\n> If, during the initial check, HEAD doesn't point at anything, then we\n\nMaybe\n\"If HEAD doesn't point at anything during the initial check, then...\" ?\n(Being a non native speaker is hard. These commas look so confusing,\nso the version without commas makes it way easier to read for me.)\n\nOtherwise\nReviewed-by: Stefan Beller \n"},{"id":"255828","messageId":"CAGZ79kbC-HB7nFcLUBGddkXKT__gYdigSHs=bg8CQW9SieUPWg@mail.gmail.com","threadId":"38528","inReplyTo":"1423412045-15616-9-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 08/11] ref_transaction_create(): check that new_sha1 is valid","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-09T18:35:58Z","receivedAt":"2015-02-09T18:35:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Feb 8, 2015 at 8:14 AM, Michael Haggerty wrote:\n> Creating a reference requires a new_sha1 that is not NULL and not\n> null_sha1.\n>\n> Signed-off-by: Michael Haggerty \n\nReviewed-by: Stefan Beller \n"},{"id":"255829","messageId":"CAGZ79kZ-MrCjxzwOqsfP7x72tSJiwawCb3e=XiK6+4F1B8z7gQ@mail.gmail.com","threadId":"38528","inReplyTo":"1423412045-15616-10-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 09/11] ref_transaction_delete(): check that old_sha1 is not null_sha1","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-09T18:37:39Z","receivedAt":"2015-02-09T18:37:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Feb 8, 2015 at 8:14 AM, Michael Haggerty wrote:\n> It makes no sense to delete a reference that is already known not to\n> exist.\n>\n> Signed-off-by: Michael Haggerty \n\nReviewed-by: Stefan Beller \n"},{"id":"255830","messageId":"xmqqtwyvapx7.fsf@gitster.dls.corp.google.com","threadId":"38528","inReplyTo":"1423412045-15616-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 00/11] Allow reference values to be checked in a transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-09T18:41:08Z","receivedAt":"2015-02-09T18:41:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> The main purpose of this series is to simplify the interface to\n> reference transactions as follows:\n>\n> * Remove the need to supply an explicit have_old parameter to\n> ref_transaction_update() and ref_transaction_delete(). Instead,\n> check the old_sha1 if and only if it is non-NULL.\n>\n> * Allow NULL to be supplied to ref_transaction_update() as new_sha1,\n> in which case old_sha1 will be verified under lock, but the\n> reference's value will not be altered.\n>\n> * Add a function ref_transaction_verify(), which verifies the current\n> value of a reference without changing it.\n>\n> * Make the similarity between ref_transaction_update() and\n> update_ref() more obvious.\n>\n> Along the way, it fixes a race that could happen if two processes try\n> to create an orphan commit at the same time.\n>\n> This patch series applies on top of master merged together with\n> sb/atomic-push, which in turn depends on mh/reflog-expire.\n\nI am a bit puzzled by your intentions, so help me out.\n\nI see that your understanding is that Stefan will be rerolling the\npush atomicity thing; wouldn't we then want to have a \"fix and\nclean\" topic like this one first and build the push atomicity thing\non top instead?\n\nIn other words, would it make sense to extend mh/reflog-expire (in\n'next') topic with commits from \"Fix some problems with reflog\nexpiration (8 patches)\" series and this series to fix and clean it?\n\nWe may even want to rebase/reroll mh/reflog-expire on top of v2.3\nwhile doing so to adjust to the transaction stuff, if that makes\nsome of the changes in the two new series unnecessary (if these \"fix\nand clean up\" changes made in mh/reflog-expire in 'next', that is).\n"},{"id":"255833","messageId":"CAGZ79kZqsCmhzw9mW4Bxyzd-+XRuzh5aYFnp5GLZwjGj9SW_LQ@mail.gmail.com","threadId":"38528","inReplyTo":"1423412045-15616-11-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 10/11] ref_transaction_verify(): new function to check a reference's value","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-09T18:50:47Z","receivedAt":"2015-02-09T18:50:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Feb 8, 2015 at 8:14 AM, Michael Haggerty wrote:\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 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\nDo we need to be explicit about having to rollback everything in each\nref_transaction_* function documentation?\n\nI like the new wording (first paragraph) of this function documentation.\n"},{"id":"255834","messageId":"CAGZ79kZA-z6OsSE7hb9N1taDh2K6zaK_=P67zNo0G9OSric=8g@mail.gmail.com","threadId":"38528","inReplyTo":"1423412045-15616-12-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 11/11] update_ref(): improve documentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-09T18:51:48Z","receivedAt":"2015-02-09T18:51:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Feb 8, 2015 at 8:14 AM, Michael Haggerty wrote:\n> Add a docstring for update_ref(), emphasizing its similarity to\n> ref_transaction_update(). Rename its parameters to match those of\n> ref_transaction_update().\n>\n> Signed-off-by: Michael Haggerty \n\nReviewed-by: Stefan Beller \n"},{"id":"255837","messageId":"CAGZ79kZmwk5=ViPdRVd+0ncMyAhGAbid6TJUznvMuK6pQ_VSRw@mail.gmail.com","threadId":"38528","inReplyTo":"xmqqtwyvapx7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 00/11] Allow reference values to be checked in a transaction","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-09T19:05:15Z","receivedAt":"2015-02-09T19:05:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 9, 2015 at 10:41 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n>\n>> The main purpose of this series is to simplify the interface to\n>> reference transactions as follows:\n>>\n>> * Remove the need to supply an explicit have_old parameter to\n>> ref_transaction_update() and ref_transaction_delete(). Instead,\n>> check the old_sha1 if and only if it is non-NULL.\n>>\n>> * Allow NULL to be supplied to ref_transaction_update() as new_sha1,\n>> in which case old_sha1 will be verified under lock, but the\n>> reference's value will not be altered.\n>>\n>> * Add a function ref_transaction_verify(), which verifies the current\n>> value of a reference without changing it.\n>>\n>> * Make the similarity between ref_transaction_update() and\n>> update_ref() more obvious.\n>>\n>> Along the way, it fixes a race that could happen if two processes try\n>> to create an orphan commit at the same time.\n>>\n>> This patch series applies on top of master merged together with\n>> sb/atomic-push, which in turn depends on mh/reflog-expire.\n>\n> I am a bit puzzled by your intentions, so help me out.\n>\n> I see that your understanding is that Stefan will be rerolling the\n> push atomicity thing; wouldn't we then want to have a \"fix and\n> clean\" topic like this one first and build the push atomicity thing\n> on top instead?\n\nMy understanding is to not reroll origin/sb/atomic-push, but\norigin/sb/atomic-push-fix (which is worded misleading. It is not about\natomic pushes, but about enabling large transactions in my understanding)\n\n>\n> In other words, would it make sense to extend mh/reflog-expire (in\n> 'next') topic with commits from \"Fix some problems with reflog\n> expiration (8 patches)\" series and this series to fix and clean it?\n\nI am not sure what advantages this would bring. A better history\nfor bisection? I cannot speak for Michael, but my understanding was\nto have mh/reflog-expire and sb/atomic-push-fix merged now that 2.3\nis out and everything else on top is unclear/rerolled/discussed as needed.\n\n>\n> We may even want to rebase/reroll mh/reflog-expire on top of v2.3\n> while doing so to adjust to the transaction stuff, if that makes\n> some of the changes in the two new series unnecessary (if these \"fix\n> and clean up\" changes made in mh/reflog-expire in 'next', that is).\n>\n"},{"id":"255842","messageId":"54D91B24.5050808@alum.mit.edu","threadId":"38528","inReplyTo":"CAGZ79kZmwk5=ViPdRVd+0ncMyAhGAbid6TJUznvMuK6pQ_VSRw@mail.gmail.com","subject":"Re: [PATCH 00/11] Allow reference values to be checked in a transaction","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-09T20:40:04Z","receivedAt":"2015-02-09T20:40:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/09/2015 08:05 PM, Stefan Beller wrote:\n> On Mon, Feb 9, 2015 at 10:41 AM, Junio C Hamano wrote:\n>> Michael Haggerty writes:\n>>> [...]\n>>> This patch series applies on top of master merged together with\n>>> sb/atomic-push, which in turn depends on mh/reflog-expire.\n>>\n>> I am a bit puzzled by your intentions, so help me out.\n>>\n>> I see that your understanding is that Stefan will be rerolling the\n>> push atomicity thing; wouldn't we then want to have a \"fix and\n>> clean\" topic like this one first and build the push atomicity thing\n>> on top instead?\n> \n> My understanding is to not reroll origin/sb/atomic-push, but\n> origin/sb/atomic-push-fix (which is worded misleading. It is not about\n> atomic pushes, but about enabling large transactions in my understanding)\n\nYes, that is what I thought.\n\n>> In other words, would it make sense to extend mh/reflog-expire (in\n>> 'next') topic with commits from \"Fix some problems with reflog\n>> expiration (8 patches)\" series and this series to fix and clean it?\n\nBoth series have to do with reflogs, but they are logically pretty\nindependent. In particular, \"Fix some problems with reflog expiration\"\nfixes problems that existed before mh/reflog-expire. And considering\nthat one topic is quite mature whereas the the other is just making its\ndebut, it seemed like yoking them together would slow down the first\ntopic for no good reason.\n\n> I am not sure what advantages this would bring. A better history\n> for bisection? I cannot speak for Michael, but my understanding was\n> to have mh/reflog-expire and sb/atomic-push-fix merged now that 2.3\n> is out and everything else on top is unclear/rerolled/discussed as needed.\n\nStefan, I think you mean sb/atomic-push, not sb/atomic-push-fix, right?\n\n>> We may even want to rebase/reroll mh/reflog-expire on top of v2.3\n>> while doing so to adjust to the transaction stuff, if that makes\n>> some of the changes in the two new series unnecessary (if these \"fix\n>> and clean up\" changes made in mh/reflog-expire in 'next', that is).\n\nI see all of these topics as pretty independent, though given that they\ntouch similar areas of the code they often have annoying (but small)\nconflicts with each other.\n\nI expected that mh/reflog-expire and sb/atomic-push would be merged\npretty early in the 2.4 cycle (they are both in next already). Junio, is\nthat not your plan?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"255845","messageId":"CAGZ79kZ3ShkW0tu=1uskXw-R93pnkw0faxut-5kA=UrsJiv+Ng@mail.gmail.com","threadId":"38528","inReplyTo":"54D91B24.5050808@alum.mit.edu","subject":"Re: [PATCH 00/11] Allow reference values to be checked in a transaction","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-09T20:41:19Z","receivedAt":"2015-02-09T20:41:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 9, 2015 at 12:40 PM, Michael Haggerty wrote:\n\n>\n>> I am not sure what advantages this would bring. A better history\n>> for bisection? I cannot speak for Michael, but my understanding was\n>> to have mh/reflog-expire and sb/atomic-push-fix merged now that 2.3\n>> is out and everything else on top is unclear/rerolled/discussed as needed.\n>\n> Stefan, I think you mean sb/atomic-push, not sb/atomic-push-fix, right?\n\nOf course, sorry for adding more confusion here.\n"},{"id":"255846","messageId":"xmqqbnl2bypl.fsf@gitster.dls.corp.google.com","threadId":"38528","inReplyTo":"54D91B24.5050808@alum.mit.edu","subject":"Re: [PATCH 00/11] Allow reference values to be checked in a transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-09T20:45:58Z","receivedAt":"2015-02-09T20:45:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 02/09/2015 08:05 PM, Stefan Beller wrote:\n>> On Mon, Feb 9, 2015 at 10:41 AM, Junio C Hamano wrote:\n>>> Michael Haggerty writes:\n>>>> [...]\n>>>> This patch series applies on top of master merged together with\n>>>> sb/atomic-push, which in turn depends on mh/reflog-expire.\n>>>\n>>> I am a bit puzzled by your intentions, so help me out.\n>>>\n>>> I see that your understanding is that Stefan will be rerolling the\n>>> push atomicity thing; wouldn't we then want to have a \"fix and\n>>> clean\" topic like this one first and build the push atomicity thing\n>>> on top instead?\n>> \n>> My understanding is to not reroll origin/sb/atomic-push, but\n>> origin/sb/atomic-push-fix (which is worded misleading. It is not about\n>> atomic pushes, but about enabling large transactions in my understanding)\n>\n> Yes, that is what I thought.\n> ...\n> Both series have to do with reflogs, but they are logically pretty\n> independent. In particular, \"Fix some problems with reflog expiration\"\n> fixes problems that existed before mh/reflog-expire. And considering\n> that one topic is quite mature whereas the the other is just making its\n> debut, it seemed like yoking them together would slow down the first\n> topic for no good reason.\n> ...\n> I expected that mh/reflog-expire and sb/atomic-push would be merged\n> pretty early in the 2.4 cycle (they are both in next already). Junio, is\n> that not your plan?\n\nOK, I am glad I asked for clarifications. It was that I felt uneasy\nto see many new \"this cleans and fixes\" while there are in-flight\ntopics in the same area.\n\nThanks.\n"},{"id":"255895","messageId":"xmqq1tlxwpgi.fsf@gitster.dls.corp.google.com","threadId":"38528","inReplyTo":"CAGZ79kYmgeXt1k22h3fbDR04BTHOQRxryNVSJDOL2DC5yLLHpw@mail.gmail.com","subject":"Re: [PATCH 06/11] commit: add tests of commit races","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-10T19:12:29Z","receivedAt":"2015-02-10T19:12:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Sun, Feb 8, 2015 at 8:14 AM, Michael Haggerty wrote:\n>\n>> +# Copyright (c) 2014 Michael Haggerty \n>\n> What is the projects stance on copyright lines?\n\nI do not think we have a strong one.\n\n> I've seen files (most of them from the beginning) having some copyright lines,\n> other files (often introduced way later) not having them, \"because\n> we're git and have\n> history, so we know who did it\".\n\nI personally agree with that statement. Also, a copyright notice\nper file is often added when a new file is added, but that ends up\ngiving false sense of \"ownership\" to everybody else down the line\neven after the file has been extensively modified. It's not like\nMichael solely owns all lines in this file in later versions. And\neven if people added their name at the top every time they make any\nchange, their names tend to stay even when their contributions are\nlater completely rewritten or removed.\n\nIn a sense, my agreement with your statement is stronger than \"Yes,\nGit can tell us who did what anyway\". What we can find in the\nhistory is the sole source of truth, and in-file copyright notice is\nmisleading. You do not even have to have one in the Berne signatory\nnations anyway.\n\n> The tests themselves look fine.\n>\n> Is there a reason you did not append the tests in 7509 ?\n\nHmph.\n"},{"id":"255929","messageId":"54DB6F9C.7060600@alum.mit.edu","threadId":"38528","inReplyTo":"xmqq1tlxwpgi.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 06/11] commit: add tests of commit races","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-11T15:05:00Z","receivedAt":"2015-02-11T15:05:00Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/10/2015 08:12 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n> \n>> On Sun, Feb 8, 2015 at 8:14 AM, Michael Haggerty wrote:\n>>\n>>> +# Copyright (c) 2014 Michael Haggerty \n>>\n>> What is the projects stance on copyright lines?\n> \n> I do not think we have a strong one.\n> \n>> I've seen files (most of them from the beginning) having some copyright lines,\n>> other files (often introduced way later) not having them, \"because\n>> we're git and have\n>> history, so we know who did it\".\n> \n> I personally agree with that statement. Also, a copyright notice\n> per file is often added when a new file is added, but that ends up\n> giving false sense of \"ownership\" to everybody else down the line\n> even after the file has been extensively modified. It's not like\n> Michael solely owns all lines in this file in later versions. And\n> even if people added their name at the top every time they make any\n> change, their names tend to stay even when their contributions are\n> later completely rewritten or removed.\n> \n> In a sense, my agreement with your statement is stronger than \"Yes,\n> Git can tell us who did what anyway\". What we can find in the\n> history is the sole source of truth, and in-file copyright notice is\n> misleading. You do not even have to have one in the Berne signatory\n> nations anyway.\n\nI only put a copyright notice there because I thought it was standard\npractice. I think it is ugly and would rather do without it, even aside\nfrom the practical problems that Junio mentioned.\n\nOn the other hand, there's this [1] and this [2] from the FSF, which\nrecommend a copyright blurb at the beginning of every source file.\nThough actually the recommendation is to include a GPL blurb too, not\njust a naked copyright line like I used. But I get the feeling that the\nFSF's recommendation is more for ideological than for legal reasons.\n\nIf I don't hear anything else, I'll delete the copyright line in the reroll.\n\n>> The tests themselves look fine.\n>>\n>> Is there a reason you did not append the tests in 7509 ?\n> \n> Hmph.\n\nI don't know what \"Hmph\" means in this context.\n\nThe description for t7509 is \"git commit --reset-author\", which doesn't\nseem to describe the new tests.\n\nThere are also\n\n t7500 \"git commit / Tests for selected commit options\"\n t7501 \"git commit\"\n t7502 \"git commit porcelain-ish\"\n\nI suppose the new tests could go in any of these. But since the tests\nare thematically a bit unusual (dealing with races rather than testing\ncommand-line options) and they start with an orphan commit, I thought it\nwould be just as easy to put them in their own file to make it clear\nthat they are independent.\n\nI really don't care either way.\n\nMichael\n\n[1] http://www.gnu.org/licenses/gpl-howto.html\n[2] http://www.gnu.org/licenses/gpl-faq.html#NoticeInSourceFile\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"255930","messageId":"54DB761D.6080300@alum.mit.edu","threadId":"38528","inReplyTo":"CAGZ79kbbRdqd+EnJD3Opfc5Luf-1FnEarUpTnXAiKoxh-pqc1Q@mail.gmail.com","subject":"Re: [PATCH 04/11] ref_transaction_update(): remove \"have_old\" parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-11T15:32:45Z","receivedAt":"2015-02-11T15:32:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/09/2015 07:20 PM, Stefan Beller wrote:\n> On Sun, Feb 8, 2015 at 8:13 AM, Michael Haggerty wrote:\n>> Instead, if old_sha1 is non-NULL, verify it; otherwise, don't.\n> \n> parsing error on that commit message. I needed to read the code to understand\n> what you want to say here.\n\nThanks for the comment. I will change that sentence to\n\n Instead, verify the reference's old value if and only if old_sha1 is\n non-NULL.\n\n(in this commit and its successor).\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"255931","messageId":"54DB79A8.9020409@alum.mit.edu","threadId":"38528","inReplyTo":"CAGZ79kYF2WWqSq71d5FZvP7PCKra-cmYHPAB0RVFed8Ag14ZWA@mail.gmail.com","subject":"Re: [PATCH 07/11] commit: avoid race when creating orphan commits","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-11T15:47:52Z","receivedAt":"2015-02-11T15:47:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/09/2015 07:35 PM, Stefan Beller wrote:\n> On Sun, Feb 8, 2015 at 8:14 AM, Michael Haggerty wrote:\n>> If, during the initial check, HEAD doesn't point at anything, then we\n> \n> Maybe\n> \"If HEAD doesn't point at anything during the initial check, then...\" ?\n> (Being a non native speaker is hard. These commas look so confusing,\n> so the version without commas makes it way easier to read for me.)\n\nWill change. Thanks!\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"255934","messageId":"54DB7F37.7030206@alum.mit.edu","threadId":"38528","inReplyTo":"CAGZ79kZqsCmhzw9mW4Bxyzd-+XRuzh5aYFnp5GLZwjGj9SW_LQ@mail.gmail.com","subject":"Re: [PATCH 10/11] ref_transaction_verify(): new function to check a reference's value","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-11T16:11:35Z","receivedAt":"2015-02-11T16:11:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/09/2015 07:50 PM, Stefan Beller wrote:\n> On Sun, Feb 8, 2015 at 8:14 AM, Michael Haggerty wrote:\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 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> Do we need to be explicit about having to rollback everything in each\n> ref_transaction_* function documentation?\n\nThanks for the suggestion.\n\nI just added a new commit that moves this (and more) information to the\nintroductory comment above these four functions' declarations, so that\nit doesn't have to be repeated for each function. It will be in the re-roll.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"255936","messageId":"xmqqk2zouxnx.fsf@gitster.dls.corp.google.com","threadId":"38528","inReplyTo":"54DB6F9C.7060600@alum.mit.edu","subject":"Re: [PATCH 06/11] commit: add tests of commit races","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-11T18:10:26Z","receivedAt":"2015-02-11T18:10:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On the other hand, there's this [1] and this [2] from the FSF, which\n> recommend a copyright blurb at the beginning of every source file.\n> Though actually the recommendation is to include a GPL blurb too, not\n> just a naked copyright line like I used. But I get the feeling that the\n> FSF's recommendation is more for ideological than for legal reasons.\n\nIt is relatively recent (late 1980s) that US became part of Berne\nConvention (1886). Before that you had to write Copyright and All\nRights Reserved (or Todos Derechos Reserrvados) in Buenos Aires\ndays.\n\nIt is not surprising to see the more cautious practice from the\nolder days in recommendations by an old organization like FSF.\n\n>>> Is there a reason you did not append the tests in 7509 ?\n>> \n>> Hmph.\n>\n> I don't know what \"Hmph\" means in this context.\n\n\"Hmph, it might deserve more thought, but I do not have opinion\nright now\".\n"},{"id":"255938","messageId":"CAGZ79kYFvLWsasWsEevLqugY_Yts5K4e3WRJtKmN4S67F9W6+Q@mail.gmail.com","threadId":"38528","inReplyTo":"xmqqk2zouxnx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 06/11] commit: add tests of commit races","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-11T18:24:35Z","receivedAt":"2015-02-11T18:24:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Feb 11, 2015 at 10:10 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n>\n>> On the other hand, there's this [1] and this [2] from the FSF, which\n>> recommend a copyright blurb at the beginning of every source file.\n>> Though actually the recommendation is to include a GPL blurb too, not\n>> just a naked copyright line like I used. But I get the feeling that the\n>> FSF's recommendation is more for ideological than for legal reasons.\n>\n> It is relatively recent (late 1980s) that US became part of Berne\n> Convention (1886). Before that you had to write Copyright and All\n> Rights Reserved (or Todos Derechos Reserrvados) in Buenos Aires\n> days.\n\nQuoting from wikipedia[1]\n\n(note however that when the United States joined the Convention\nin 1988, it continued to make statutory damages and attorney's fees\nonly available for registered works).\n\nDoes that mean if somebody would infringe the GPL on git (e.g. selling\na modified git version without giving sources), it would be harder to\ntell him to stop because of the missing attorney's fees in case we drop\nout the copyright notices? (I have no deep understanding of legal\nprocesses in the US).\n\n>\n> It is not surprising to see the more cautious practice from the\n> older days in recommendations by an old organization like FSF.\n>\n>>>> Is there a reason you did not append the tests in 7509 ?\n\nYou convinced me that having to start with an orphan commit justifies\na new test file as well as the nature of the test.\n\n>>>\n>>> Hmph.\n>>\n>> I don't know what \"Hmph\" means in this context.\n>\n> \"Hmph, it might deserve more thought, but I do not have opinion\n> right now\".\n\n[1] http://en.wikipedia.org/wiki/Berne_Convention\n"},{"id":"255940","messageId":"xmqqy4o4th1t.fsf@gitster.dls.corp.google.com","threadId":"38528","inReplyTo":"CAGZ79kYFvLWsasWsEevLqugY_Yts5K4e3WRJtKmN4S67F9W6+Q@mail.gmail.com","subject":"Re: [PATCH 06/11] commit: add tests of commit races","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-11T18:54:38Z","receivedAt":"2015-02-11T18:54:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Quoting from wikipedia[1]\n>\n> (note however that when the United States joined the Convention\n> in 1988, it continued to make statutory damages and attorney's fees\n> only available for registered works).\n>\n> Does that mean if somebody would infringe the GPL on git (e.g. selling\n> a modified git version without giving sources), it would be harder to\n> tell him to stop because of the missing attorney's fees in case we drop\n> out the copyright notices? (I have no deep understanding of legal\n> processes in the US).\n\nNo. \"registered works\" in that sentence is about registering\ncopyright with U.S. Copyright Office. In-file Copyright circle-c\nline does not have much to do with that.\n"}]}