{"thread":{"id":"36283","subject":"[PATCH v2 02/27] t1400: Provide more usual input to the command","startedAt":"2014-03-24T17:56:33Z","lastAt":"2014-04-04T05:02:48Z","messageCount":65,"participants":["Michael Haggerty","Brad King","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":27},"messages":[{"id":"237479","messageId":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":null,"subject":"[PATCH v2 00/27] Clean up update-refs --stdin and implement ref_transaction","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:33Z","receivedAt":"2014-03-24T17:56:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is v2 of this patch series. See also [1] for more context.\n\nThanks to Brad, Junio, and Johan for their feedback on v1 [2]. I\nthink I have addressed all of your points.\n\nChanges relative to v1:\n\n* Rename the functions associated with ref_transactions to be more\n reminiscent of database transactions:\n\n * create_ref_transaction() -> ref_transaction_begin()\n * free_ref_transaction() -> ref_transaction_rollback()\n * queue_update_ref() -> ref_transaction_update()\n * queue_create_ref() -> ref_transaction_create()\n * queue_delete_ref() -> ref_transaction_delete()\n * commit_ref_transaction() -> ref_transaction_commit()\n\n* Change ref_transaction_commit() to also free the transaction, so the\n user doesn't have to think about memory resources at all.\n\n* Fix backwards compatibility of \"git update-ref --stdin -z\"'s\n handling of the \"create\" command: allow to be the empty\n string, treating it the same zeros. But deprecate this usage.\n\n* Rebased to current master (there were no conflicts).\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/243726\n[2] http://thread.gmane.org/gmane.comp.version-control.git/243731\n\nMichael Haggerty (27):\n t1400: Fix name and expected result of one test\n t1400: Provide more usual input to the command\n parse_arg(): Really test that argument is properly terminated\n t1400: Add some more tests involving quoted arguments\n refs.h: Rename the action_on_err constants\n update_refs(): Fix constness\n update-ref --stdin: Read the whole input at once\n parse_cmd_verify(): Copy old_sha1 instead of evaluating \n twice\n update-ref.c: Extract a new function, parse_refname()\n update-ref --stdin: Improve error messages for invalid values\n update-ref --stdin: Make error messages more consistent\n update-ref --stdin: Simplify error messages for missing oldvalues\n t1400: Test that stdin -z update treats empty as zeros\n update-ref.c: Extract a new function, parse_next_sha1()\n update-ref --stdin -z: Deprecate interpreting the empty string as\n zeros\n t1400: Test one mistake at a time\n update-ref --stdin: Improve the error message for unexpected EOF\n update-ref --stdin: Harmonize error messages\n refs: Add a concept of a reference transaction\n update-ref --stdin: Reimplement using reference transactions\n refs: Remove API function update_refs()\n struct ref_update: Rename field \"ref_name\" to \"refname\"\n struct ref_update: Store refname as a FLEX_ARRAY.\n ref_transaction_commit(): Introduce temporary variables\n struct ref_update: Add a lock member\n struct ref_update: Add type field\n ref_transaction_commit(): Work with transaction->updates in place\n\n Documentation/git-update-ref.txt | 18 +-\n builtin/checkout.c | 2 +-\n builtin/clone.c | 9 +-\n builtin/merge.c | 6 +-\n builtin/notes.c | 6 +-\n builtin/reset.c | 6 +-\n builtin/update-ref.c | 425 ++++++++++++++++++++-------------\n contrib/examples/builtin-fetch--tool.c | 3 +-\n notes-cache.c | 2 +-\n notes-utils.c | 3 +-\n refs.c | 192 +++++++++++----\n refs.h | 94 ++++++--\n t/t1400-update-ref.sh | 100 +++++---\n 13 files changed, 582 insertions(+), 284 deletions(-)\n\n-- \n1.9.0\n"},{"id":"237478","messageId":"1395683820-17304-2-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 01/27] t1400: Fix name and expected result of one test","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:34Z","receivedAt":"2014-03-24T17:56:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The test\n\n stdin -z create ref fails with zero new value\n\nactually passes an empty new value, not a zero new value. So rename\nthe test s/zero/empty/, and change the expected error from\n\n fatal: create $c given zero new value\n\nto\n\n fatal: create $c missing \n\nOf course, this makes the test fail now, so mark it\ntest_expect_failure. The failure will be fixed later in this patch\nseries.\n\nSigned-off-by: Michael Haggerty \n---\n t/t1400-update-ref.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 6ffd82f..fa927d2 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -827,10 +827,10 @@ test_expect_success 'stdin -z create ref fails with bad new value' '\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n-test_expect_success 'stdin -z create ref fails with zero new value' '\n+test_expect_failure 'stdin -z create ref fails with empty new value' '\n \tprintf $F \"create $c\" \"\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: create $c given zero new value\" err &&\n+\tgrep \"fatal: create $c missing \" err &&\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n-- \n1.9.0\n"},{"id":"237452","messageId":"1395683820-17304-3-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 02/27] t1400: Provide more usual input to the command","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:35Z","receivedAt":"2014-03-24T17:56:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The old version was passing (among other things)\n\n update SP refs/heads/c NUL NUL 0{40} NUL\n\nto \"git update-ref -z --stdin\" to test whether the old-value check for\nc is working. But the is empty, which is a bit off the\nbeaten track.\n\nSo, to be sure that we are testing what we want to test, provide an\nactual on the \"update\" line.\n\nSigned-off-by: Michael Haggerty \n---\n t/t1400-update-ref.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex fa927d2..29391c6 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -912,7 +912,7 @@ test_expect_success 'stdin -z update refs works with identity updates' '\n \n test_expect_success 'stdin -z update refs fails with wrong old value' '\n \tgit update-ref $c $m &&\n-\tprintf $F \"update $a\" \"$m\" \"$m\" \"update $b\" \"$m\" \"$m\" \"update $c\" \"\" \"$Z\" >stdin &&\n+\tprintf $F \"update $a\" \"$m\" \"$m\" \"update $b\" \"$m\" \"$m\" \"update $c\" \"$m\" \"$Z\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n \tgrep \"fatal: Cannot lock the ref '\"'\"'$c'\"'\"'\" err &&\n \tgit rev-parse $m >expect &&\n-- \n1.9.0\n"},{"id":"237454","messageId":"1395683820-17304-4-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 03/27] parse_arg(): Really test that argument is properly terminated","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:36Z","receivedAt":"2014-03-24T17:56:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Test that the argument is properly terminated by either whitespace or\na NUL character, even if it is quoted, to be consistent with the\nnon-quoted case. Adjust the tests to expect the new error message.\nAdd a docstring to the function, incorporating the comments that were\nformerly within the function plus some added information.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/update-ref.c | 20 +++++++++++++++-----\n t/t1400-update-ref.sh | 4 ++--\n 2 files changed, 17 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 1292cfe..02b5f95 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -62,16 +62,26 @@ static void update_store_old_sha1(struct ref_update *update,\n \tupdate->have_old = *oldvalue || line_termination;\n }\n \n+/*\n+ * Parse one whitespace- or NUL-terminated, possibly C-quoted argument\n+ * and append the result to arg. Return a pointer to the terminator.\n+ * Die if there is an error in how the argument is C-quoted. This\n+ * function is only used if not -z.\n+ */\n static const char *parse_arg(const char *next, struct strbuf *arg)\n {\n-\t/* Parse SP-terminated, possibly C-quoted argument */\n-\tif (*next != '\"')\n+\tif (*next == '\"') {\n+\t\tconst char *orig = next;\n+\n+\t\tif (unquote_c_style(arg, next, &next))\n+\t\t\tdie(\"badly quoted argument: %s\", orig);\n+\t\tif (*next && !isspace(*next))\n+\t\t\tdie(\"unexpected character after quoted argument: %s\", orig);\n+\t} else {\n \t\twhile (*next && !isspace(*next))\n \t\t\tstrbuf_addch(arg, *next++);\n-\telse if (unquote_c_style(arg, next, &next))\n-\t\tdie(\"badly quoted argument: %s\", next);\n+\t}\n \n-\t/* Return position after the argument */\n \treturn next;\n }\n \ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 29391c6..774f8c5 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -356,10 +356,10 @@ test_expect_success 'stdin fails on badly quoted input' '\n \tgrep \"fatal: badly quoted argument: \\\\\\\"master\" err\n '\n \n-test_expect_success 'stdin fails on arguments not separated by space' '\n+test_expect_success 'stdin fails on junk after quoted argument' '\n \techo \"create \\\"$a\\\"master\" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: expected SP but got: master\" err\n+\tgrep \"fatal: unexpected character after quoted argument: \\\\\\\"$a\\\\\\\"master\" err\n '\n \n test_expect_success 'stdin fails create with no ref' '\n-- \n1.9.0\n"},{"id":"237458","messageId":"1395683820-17304-5-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 04/27] t1400: Add some more tests involving quoted arguments","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:37Z","receivedAt":"2014-03-24T17:56:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Previously there were no good tests of C-quoted arguments.\n\nSigned-off-by: Michael Haggerty \n---\n t/t1400-update-ref.sh | 26 +++++++++++++++++++++++++-\n 1 file changed, 25 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 774f8c5..00862bc 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -350,12 +350,18 @@ test_expect_success 'stdin fails on unknown command' '\n \tgrep \"fatal: unknown command: unknown $a\" err\n '\n \n-test_expect_success 'stdin fails on badly quoted input' '\n+test_expect_success 'stdin fails on unbalanced quotes' '\n \techo \"create $a \\\"master\" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n \tgrep \"fatal: badly quoted argument: \\\\\\\"master\" err\n '\n \n+test_expect_success 'stdin fails on invalid escape' '\n+\techo \"create $a \\\"ma\\zter\\\"\" >stdin &&\n+\ttest_must_fail git update-ref --stdin err &&\n+\tgrep \"fatal: badly quoted argument: \\\\\\\"ma\\\\\\\\zter\\\\\\\"\" err\n+'\n+\n test_expect_success 'stdin fails on junk after quoted argument' '\n \techo \"create \\\"$a\\\"master\" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n@@ -458,6 +464,24 @@ test_expect_success 'stdin create ref works' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'stdin succeeds with quoted argument' '\n+\tgit update-ref -d $a &&\n+\techo \"create $a \\\"$m\\\"\" >stdin &&\n+\tgit update-ref --stdin expect &&\n+\tgit rev-parse $a >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'stdin succeeds with escaped character' '\n+\tgit update-ref -d $a &&\n+\techo \"create $a \\\"ma\\\\163ter\\\"\" >stdin &&\n+\tgit update-ref --stdin expect &&\n+\tgit rev-parse $a >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'stdin update ref creates with zero old value' '\n \techo \"update $b $m $Z\" >stdin &&\n \tgit update-ref --stdin \n---\n builtin/checkout.c | 2 +-\n builtin/clone.c | 9 +++++----\n builtin/merge.c | 6 +++---\n builtin/notes.c | 6 +++---\n builtin/reset.c | 6 ++++--\n builtin/update-ref.c | 5 +++--\n contrib/examples/builtin-fetch--tool.c | 3 ++-\n notes-cache.c | 2 +-\n notes-utils.c | 3 ++-\n refs.c | 18 +++++++++---------\n refs.h | 9 +++++++--\n 11 files changed, 40 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex ada51fa..f79b222 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -624,7 +624,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t/* Nothing to do. */\n \t} else if (opts->force_detach || !new->path) {\t/* No longer on any branch. */\n \t\tupdate_ref(msg.buf, \"HEAD\", new->commit->object.sha1, NULL,\n-\t\t\t REF_NODEREF, DIE_ON_ERR);\n+\t\t\t REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);\n \t\tif (!opts->quiet) {\n \t\t\tif (old->path && advice_detached_head)\n \t\t\t\tdetach_advice(new->name);\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 43e772c..af3b86f 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -521,7 +521,7 @@ static void write_followtags(const struct ref *refs, const char *msg)\n \t\tif (!has_sha1_file(ref->old_sha1))\n \t\t\tcontinue;\n \t\tupdate_ref(msg, ref->name, ref->old_sha1,\n-\t\t\t NULL, 0, DIE_ON_ERR);\n+\t\t\t NULL, 0, UPDATE_REFS_DIE_ON_ERR);\n \t}\n }\n \n@@ -589,14 +589,15 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t\tcreate_symref(\"HEAD\", our->name, NULL);\n \t\tif (!option_bare) {\n \t\t\tconst char *head = skip_prefix(our->name, \"refs/heads/\");\n-\t\t\tupdate_ref(msg, \"HEAD\", our->old_sha1, NULL, 0, DIE_ON_ERR);\n+\t\t\tupdate_ref(msg, \"HEAD\", our->old_sha1, NULL, 0,\n+\t\t\t\t UPDATE_REFS_DIE_ON_ERR);\n \t\t\tinstall_branch_config(0, head, option_origin, our->name);\n \t\t}\n \t} else if (our) {\n \t\tstruct commit *c = lookup_commit_reference(our->old_sha1);\n \t\t/* --branch specifies a non-branch (i.e. tags), detach HEAD */\n \t\tupdate_ref(msg, \"HEAD\", c->object.sha1,\n-\t\t\t NULL, REF_NODEREF, DIE_ON_ERR);\n+\t\t\t NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);\n \t} else if (remote) {\n \t\t/*\n \t\t * We know remote HEAD points to a non-branch, or\n@@ -604,7 +605,7 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t\t * Detach HEAD in all these cases.\n \t\t */\n \t\tupdate_ref(msg, \"HEAD\", remote->old_sha1,\n-\t\t\t NULL, REF_NODEREF, DIE_ON_ERR);\n+\t\t\t NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);\n \t}\n }\n \ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex f0cf120..a4c3b17 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -398,7 +398,7 @@ static void finish(struct commit *head_commit,\n \t\t\tconst char *argv_gc_auto[] = { \"gc\", \"--auto\", NULL };\n \t\t\tupdate_ref(reflog_message.buf, \"HEAD\",\n \t\t\t\tnew_head, head, 0,\n-\t\t\t\tDIE_ON_ERR);\n+\t\t\t\tUPDATE_REFS_DIE_ON_ERR);\n \t\t\t/*\n \t\t\t * We ignore errors in 'gc --auto', since the\n \t\t\t * user should see them.\n@@ -1222,7 +1222,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"%s - not something we can merge\"), argv[0]);\n \t\tread_empty(remote_head->object.sha1, 0);\n \t\tupdate_ref(\"initial pull\", \"HEAD\", remote_head->object.sha1,\n-\t\t\t NULL, 0, DIE_ON_ERR);\n+\t\t\t NULL, 0, UPDATE_REFS_DIE_ON_ERR);\n \t\tgoto done;\n \t} else {\n \t\tstruct strbuf merge_names = STRBUF_INIT;\n@@ -1339,7 +1339,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t}\n \n \tupdate_ref(\"updating ORIG_HEAD\", \"ORIG_HEAD\", head_commit->object.sha1,\n-\t\t NULL, 0, DIE_ON_ERR);\n+\t\t NULL, 0, UPDATE_REFS_DIE_ON_ERR);\n \n \tif (remoteheads && !common)\n \t\t; /* No common ancestors found. We need a real merge. */\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex bb89930..66147b6 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -717,7 +717,7 @@ static int merge_commit(struct notes_merge_options *o)\n \tstrbuf_insert(&msg, 0, \"notes: \", 7);\n \tupdate_ref(msg.buf, o->local_ref, sha1,\n \t\t is_null_sha1(parent_sha1) ? NULL : parent_sha1,\n-\t\t 0, DIE_ON_ERR);\n+\t\t 0, UPDATE_REFS_DIE_ON_ERR);\n \n \tfree_notes(t);\n \tstrbuf_release(&msg);\n@@ -812,11 +812,11 @@ static int merge(int argc, const char **argv, const char *prefix)\n \tif (result >= 0) /* Merge resulted (trivially) in result_sha1 */\n \t\t/* Update default notes ref with new commit */\n \t\tupdate_ref(msg.buf, default_notes_ref(), result_sha1, NULL,\n-\t\t\t 0, DIE_ON_ERR);\n+\t\t\t 0, UPDATE_REFS_DIE_ON_ERR);\n \telse { /* Merge has unresolved conflicts */\n \t\t/* Update .git/NOTES_MERGE_PARTIAL with partial merge result */\n \t\tupdate_ref(msg.buf, \"NOTES_MERGE_PARTIAL\", result_sha1, NULL,\n-\t\t\t 0, DIE_ON_ERR);\n+\t\t\t 0, UPDATE_REFS_DIE_ON_ERR);\n \t\t/* Store ref-to-be-updated into .git/NOTES_MERGE_REF */\n \t\tif (create_symref(\"NOTES_MERGE_REF\", default_notes_ref(), NULL))\n \t\t\tdie(\"Failed to store link to current notes ref (%s)\",\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex f4e0875..f368266 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -252,11 +252,13 @@ static int reset_refs(const char *rev, const unsigned char *sha1)\n \tif (!get_sha1(\"HEAD\", sha1_orig)) {\n \t\torig = sha1_orig;\n \t\tset_reflog_message(&msg, \"updating ORIG_HEAD\", NULL);\n-\t\tupdate_ref(msg.buf, \"ORIG_HEAD\", orig, old_orig, 0, MSG_ON_ERR);\n+\t\tupdate_ref(msg.buf, \"ORIG_HEAD\", orig, old_orig, 0,\n+\t\t\t UPDATE_REFS_MSG_ON_ERR);\n \t} else if (old_orig)\n \t\tdelete_ref(\"ORIG_HEAD\", old_orig, 0);\n \tset_reflog_message(&msg, \"updating HEAD\", rev);\n-\tupdate_ref_status = update_ref(msg.buf, \"HEAD\", sha1, orig, 0, MSG_ON_ERR);\n+\tupdate_ref_status = update_ref(msg.buf, \"HEAD\", sha1, orig, 0,\n+\t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n \tstrbuf_release(&msg);\n \treturn update_ref_status;\n }\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 02b5f95..f6345e5 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -282,7 +282,8 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t\tif (end_null)\n \t\t\tline_termination = '\\0';\n \t\tupdate_refs_stdin();\n-\t\treturn update_refs(msg, updates, updates_count, DIE_ON_ERR);\n+\t\treturn update_refs(msg, updates, updates_count,\n+\t\t\t\t UPDATE_REFS_DIE_ON_ERR);\n \t}\n \n \tif (end_null)\n@@ -314,5 +315,5 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t\treturn delete_ref(refname, oldval ? oldsha1 : NULL, flags);\n \telse\n \t\treturn update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,\n-\t\t\t\t flags, DIE_ON_ERR);\n+\t\t\t\t flags, UPDATE_REFS_DIE_ON_ERR);\n }\ndiff --git a/contrib/examples/builtin-fetch--tool.c b/contrib/examples/builtin-fetch--tool.c\nindex 8bc8c75..ee19166 100644\n--- a/contrib/examples/builtin-fetch--tool.c\n+++ b/contrib/examples/builtin-fetch--tool.c\n@@ -31,7 +31,8 @@ static int update_ref_env(const char *action,\n \t\trla = \"(reflog update)\";\n \tif (snprintf(msg, sizeof(msg), \"%s: %s\", rla, action) >= sizeof(msg))\n \t\twarning(\"reflog message too long: %.*s...\", 50, msg);\n-\treturn update_ref(msg, refname, sha1, oldval, 0, QUIET_ON_ERR);\n+\treturn update_ref(msg, refname, sha1, oldval, 0,\n+\t\t\t UPDATE_REFS_QUIET_ON_ERR);\n }\n \n static int update_local_ref(const char *name,\ndiff --git a/notes-cache.c b/notes-cache.c\nindex eabe4a0..97dfd63 100644\n--- a/notes-cache.c\n+++ b/notes-cache.c\n@@ -62,7 +62,7 @@ int notes_cache_write(struct notes_cache *c)\n \tif (commit_tree(&msg, tree_sha1, NULL, commit_sha1, NULL, NULL) < 0)\n \t\treturn -1;\n \tif (update_ref(\"update notes cache\", c->tree.ref, commit_sha1, NULL,\n-\t\t 0, QUIET_ON_ERR) < 0)\n+\t\t 0, UPDATE_REFS_QUIET_ON_ERR) < 0)\n \t\treturn -1;\n \n \treturn 0;\ndiff --git a/notes-utils.c b/notes-utils.c\nindex 4aa7023..a0b1d7b 100644\n--- a/notes-utils.c\n+++ b/notes-utils.c\n@@ -48,7 +48,8 @@ void commit_notes(struct notes_tree *t, const char *msg)\n \n \tcreate_notes_commit(t, NULL, &buf, commit_sha1);\n \tstrbuf_insert(&buf, 0, \"notes: \", 7); /* commit message starts at index 7 */\n-\tupdate_ref(buf.buf, t->ref, commit_sha1, NULL, 0, DIE_ON_ERR);\n+\tupdate_ref(buf.buf, t->ref, commit_sha1, NULL, 0,\n+\t\t UPDATE_REFS_DIE_ON_ERR);\n \n \tstrbuf_release(&buf);\n }\ndiff --git a/refs.c b/refs.c\nindex 28d5eca..196984e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3243,9 +3243,9 @@ static struct ref_lock *update_ref_lock(const char *refname,\n \tif (!lock) {\n \t\tconst char *str = \"Cannot lock the ref '%s'.\";\n \t\tswitch (onerr) {\n-\t\tcase MSG_ON_ERR: error(str, refname); break;\n-\t\tcase DIE_ON_ERR: die(str, refname); break;\n-\t\tcase QUIET_ON_ERR: break;\n+\t\tcase UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;\n+\t\tcase UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;\n+\t\tcase UPDATE_REFS_QUIET_ON_ERR: break;\n \t\t}\n \t}\n \treturn lock;\n@@ -3258,9 +3258,9 @@ static int update_ref_write(const char *action, const char *refname,\n \tif (write_ref_sha1(lock, sha1, action) < 0) {\n \t\tconst char *str = \"Cannot update the ref '%s'.\";\n \t\tswitch (onerr) {\n-\t\tcase MSG_ON_ERR: error(str, refname); break;\n-\t\tcase DIE_ON_ERR: die(str, refname); break;\n-\t\tcase QUIET_ON_ERR: break;\n+\t\tcase UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;\n+\t\tcase UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;\n+\t\tcase UPDATE_REFS_QUIET_ON_ERR: break;\n \t\t}\n \t\treturn 1;\n \t}\n@@ -3294,11 +3294,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n \t\t\tconst char *str =\n \t\t\t\t\"Multiple updates for ref '%s' not allowed.\";\n \t\t\tswitch (onerr) {\n-\t\t\tcase MSG_ON_ERR:\n+\t\t\tcase UPDATE_REFS_MSG_ON_ERR:\n \t\t\t\terror(str, updates[i]->ref_name); break;\n-\t\t\tcase DIE_ON_ERR:\n+\t\t\tcase UPDATE_REFS_DIE_ON_ERR:\n \t\t\t\tdie(str, updates[i]->ref_name); break;\n-\t\t\tcase QUIET_ON_ERR:\n+\t\t\tcase UPDATE_REFS_QUIET_ON_ERR:\n \t\t\t\tbreak;\n \t\t\t}\n \t\t\treturn 1;\ndiff --git a/refs.h b/refs.h\nindex 87a1a79..a713b34 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -214,8 +214,13 @@ extern int rename_ref(const char *oldref, const char *newref, const char *logmsg\n */\n extern int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sha1);\n \n-/** lock a ref and then write its file */\n-enum action_on_err { MSG_ON_ERR, DIE_ON_ERR, QUIET_ON_ERR };\n+enum action_on_err {\n+\tUPDATE_REFS_MSG_ON_ERR,\n+\tUPDATE_REFS_DIE_ON_ERR,\n+\tUPDATE_REFS_QUIET_ON_ERR\n+};\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-- \n1.9.0\n"},{"id":"237460","messageId":"1395683820-17304-7-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 06/27] update_refs(): Fix constness","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:39Z","receivedAt":"2014-03-24T17:56:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Since full const correctness is beyond the ability of C's type system,\njust put the const where it doesn't do any harm. A (struct ref_update\n**) can be passed to a (struct ref_update * const *) argument, but not\nto a (const struct ref_update **) argument.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/update-ref.c | 2 +-\n refs.c | 2 +-\n refs.h | 2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex f6345e5..a8a68e8 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -14,7 +14,7 @@ static const char * const git_update_ref_usage[] = {\n \n static int updates_alloc;\n static int updates_count;\n-static const struct ref_update **updates;\n+static struct ref_update **updates;\n \n static char line_termination = '\\n';\n static int update_flags;\ndiff --git a/refs.c b/refs.c\nindex 196984e..1305eb1 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3306,7 +3306,7 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n \treturn 0;\n }\n \n-int update_refs(const char *action, const struct ref_update **updates_orig,\n+int update_refs(const char *action, struct ref_update * const *updates_orig,\n \t\tint n, enum action_on_err onerr)\n {\n \tint ret = 0, delnum = 0, i;\ndiff --git a/refs.h b/refs.h\nindex a713b34..08e60ac 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -228,7 +228,7 @@ int update_ref(const char *action, const char *refname,\n /**\n * Lock all refs and then perform all modifications.\n */\n-int update_refs(const char *action, const struct ref_update **updates,\n+int update_refs(const char *action, struct ref_update * const *updates,\n \t\tint n, enum action_on_err onerr);\n \n extern int parse_hide_refs_config(const char *var, const char *value, const char *);\n-- \n1.9.0\n"},{"id":"237456","messageId":"1395683820-17304-8-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 07/27] update-ref --stdin: Read the whole input at once","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:40Z","receivedAt":"2014-03-24T17:56:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Read the whole input into a strbuf at once, and then parse it from\nthere. This might also be a tad faster, but that is not the point.\nThe point is to decouple the parsing code from the input source (the\nold parsing code had to read new data even in the middle of commands).\nAdd docstrings for the parsing functions.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/update-ref.c | 170 ++++++++++++++++++++++++++++++++-------------------\n 1 file changed, 108 insertions(+), 62 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex a8a68e8..5f197fe 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -85,44 +85,70 @@ static const char *parse_arg(const char *next, struct strbuf *arg)\n \treturn next;\n }\n \n-static const char *parse_first_arg(const char *next, struct strbuf *arg)\n+/*\n+ * Parse the argument immediately after \"command SP\". If not -z, then\n+ * handle C-quoting. Write the argument to arg. Set *next to point\n+ * at the character that terminates the argument. Die if C-quoting is\n+ * malformed.\n+ */\n+static void parse_first_arg(struct strbuf *input, const char **next,\n+\t\t\t struct strbuf *arg)\n {\n-\t/* Parse argument immediately after \"command SP\" */\n \tstrbuf_reset(arg);\n \tif (line_termination) {\n \t\t/* Without -z, use the next argument */\n-\t\tnext = parse_arg(next, arg);\n+\t\t*next = parse_arg(*next, arg);\n \t} else {\n-\t\t/* With -z, use rest of first NUL-terminated line */\n-\t\tstrbuf_addstr(arg, next);\n-\t\tnext = next + arg->len;\n+\t\t/* With -z, use everything up to the next NUL */\n+\t\tstrbuf_addstr(arg, *next);\n+\t\t*next += arg->len;\n \t}\n-\treturn next;\n }\n \n-static const char *parse_next_arg(const char *next, struct strbuf *arg)\n+/*\n+ * Parse a SP/NUL separator followed by the next SP- or NUL-terminated\n+ * argument, if any. If there is an argument, write it to arg, set\n+ * *next to point at the character terminating the argument, and\n+ * return 0. If there is no argument at all (not even the empty\n+ * string), return a non-zero result and leave *next unchanged.\n+ */\n+static int parse_next_arg(struct strbuf *input, const char **next,\n+\t\t\t struct strbuf *arg)\n {\n-\t/* Parse next SP-terminated or NUL-terminated argument, if any */\n \tstrbuf_reset(arg);\n \tif (line_termination) {\n \t\t/* Without -z, consume SP and use next argument */\n-\t\tif (!*next)\n-\t\t\treturn NULL;\n-\t\tif (*next != ' ')\n-\t\t\tdie(\"expected SP but got: %s\", next);\n-\t\tnext = parse_arg(next + 1, arg);\n+\t\tif (!**next || **next == line_termination)\n+\t\t\treturn -1;\n+\t\tif (**next != ' ')\n+\t\t\tdie(\"expected SP but got: %s\", *next);\n+\t\t(*next)++;\n+\t\t*next = parse_arg(*next, arg);\n \t} else {\n \t\t/* With -z, read the next NUL-terminated line */\n-\t\tif (*next)\n-\t\t\tdie(\"expected NUL but got: %s\", next);\n-\t\tif (strbuf_getline(arg, stdin, '\\0') == EOF)\n-\t\t\treturn NULL;\n-\t\tnext = arg->buf + arg->len;\n+\t\tif (**next)\n+\t\t\tdie(\"expected NUL but got: %s\", *next);\n+\t\t(*next)++;\n+\t\tif (*next == input->buf + input->len)\n+\t\t\treturn -1;\n+\t\tstrbuf_addstr(arg, *next);\n+\t\t*next += arg->len;\n \t}\n-\treturn next;\n+\treturn 0;\n }\n \n-static void parse_cmd_update(const char *next)\n+\n+/*\n+ * The following five parse_cmd_*() functions parse the corresponding\n+ * command. In each case, next points at the character following the\n+ * command name and the following space. They each return a pointer\n+ * to the character terminating the command, and die with an\n+ * explanatory message if there are any parsing problems. All of\n+ * these functions handle either text or binary format input,\n+ * depending on how line_termination is set.\n+ */\n+\n+static const char *parse_cmd_update(struct strbuf *input, const char *next)\n {\n \tstruct strbuf ref = STRBUF_INIT;\n \tstruct strbuf newvalue = STRBUF_INIT;\n@@ -131,26 +157,28 @@ static void parse_cmd_update(const char *next)\n \n \tupdate = update_alloc();\n \n-\tif ((next = parse_first_arg(next, &ref)) != NULL && ref.buf[0])\n+\tparse_first_arg(input, &next, &ref);\n+\tif (ref.buf[0])\n \t\tupdate_store_ref_name(update, ref.buf);\n \telse\n \t\tdie(\"update line missing \");\n \n-\tif ((next = parse_next_arg(next, &newvalue)) != NULL)\n+\tif (!parse_next_arg(input, &next, &newvalue))\n \t\tupdate_store_new_sha1(update, newvalue.buf);\n \telse\n \t\tdie(\"update %s missing \", ref.buf);\n \n-\tif ((next = parse_next_arg(next, &oldvalue)) != NULL)\n+\tif (!parse_next_arg(input, &next, &oldvalue)) {\n \t\tupdate_store_old_sha1(update, oldvalue.buf);\n-\telse if(!line_termination)\n+\t\tif (*next != line_termination)\n+\t\t\tdie(\"update %s has extra input: %s\", ref.buf, next);\n+\t} else if (!line_termination)\n \t\tdie(\"update %s missing [] NUL\", ref.buf);\n \n-\tif (next && *next)\n-\t\tdie(\"update %s has extra input: %s\", ref.buf, next);\n+\treturn next;\n }\n \n-static void parse_cmd_create(const char *next)\n+static const char *parse_cmd_create(struct strbuf *input, const char *next)\n {\n \tstruct strbuf ref = STRBUF_INIT;\n \tstruct strbuf newvalue = STRBUF_INIT;\n@@ -158,23 +186,27 @@ static void parse_cmd_create(const char *next)\n \n \tupdate = update_alloc();\n \n-\tif ((next = parse_first_arg(next, &ref)) != NULL && ref.buf[0])\n+\tparse_first_arg(input, &next, &ref);\n+\tif (ref.buf[0])\n \t\tupdate_store_ref_name(update, ref.buf);\n \telse\n \t\tdie(\"create line missing \");\n \n-\tif ((next = parse_next_arg(next, &newvalue)) != NULL)\n+\tif (!parse_next_arg(input, &next, &newvalue))\n \t\tupdate_store_new_sha1(update, newvalue.buf);\n \telse\n \t\tdie(\"create %s missing \", ref.buf);\n+\n \tif (is_null_sha1(update->new_sha1))\n \t\tdie(\"create %s given zero new value\", ref.buf);\n \n-\tif (next && *next)\n+\tif (*next != line_termination)\n \t\tdie(\"create %s has extra input: %s\", ref.buf, next);\n+\n+\treturn next;\n }\n \n-static void parse_cmd_delete(const char *next)\n+static const char *parse_cmd_delete(struct strbuf *input, const char *next)\n {\n \tstruct strbuf ref = STRBUF_INIT;\n \tstruct strbuf oldvalue = STRBUF_INIT;\n@@ -182,23 +214,26 @@ static void parse_cmd_delete(const char *next)\n \n \tupdate = update_alloc();\n \n-\tif ((next = parse_first_arg(next, &ref)) != NULL && ref.buf[0])\n+\tparse_first_arg(input, &next, &ref);\n+\tif (ref.buf[0])\n \t\tupdate_store_ref_name(update, ref.buf);\n \telse\n \t\tdie(\"delete line missing \");\n \n-\tif ((next = parse_next_arg(next, &oldvalue)) != NULL)\n+\tif (!parse_next_arg(input, &next, &oldvalue)) {\n \t\tupdate_store_old_sha1(update, oldvalue.buf);\n-\telse if(!line_termination)\n+\t\tif (update->have_old && is_null_sha1(update->old_sha1))\n+\t\t\tdie(\"delete %s given zero old value\", ref.buf);\n+\t} else if (!line_termination)\n \t\tdie(\"delete %s missing [] NUL\", ref.buf);\n-\tif (update->have_old && is_null_sha1(update->old_sha1))\n-\t\tdie(\"delete %s given zero old value\", ref.buf);\n \n-\tif (next && *next)\n+\tif (*next != line_termination)\n \t\tdie(\"delete %s has extra input: %s\", ref.buf, next);\n+\n+\treturn next;\n }\n \n-static void parse_cmd_verify(const char *next)\n+static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n {\n \tstruct strbuf ref = STRBUF_INIT;\n \tstruct strbuf value = STRBUF_INIT;\n@@ -206,53 +241,64 @@ static void parse_cmd_verify(const char *next)\n \n \tupdate = update_alloc();\n \n-\tif ((next = parse_first_arg(next, &ref)) != NULL && ref.buf[0])\n+\tparse_first_arg(input, &next, &ref);\n+\tif (ref.buf[0])\n \t\tupdate_store_ref_name(update, ref.buf);\n \telse\n \t\tdie(\"verify line missing \");\n \n-\tif ((next = parse_next_arg(next, &value)) != NULL) {\n+\tif (!parse_next_arg(input, &next, &value)) {\n \t\tupdate_store_old_sha1(update, value.buf);\n \t\tupdate_store_new_sha1(update, value.buf);\n-\t} else if(!line_termination)\n+\t} else if (!line_termination)\n \t\tdie(\"verify %s missing [] NUL\", ref.buf);\n \n-\tif (next && *next)\n+\tif (*next != line_termination)\n \t\tdie(\"verify %s has extra input: %s\", ref.buf, next);\n+\n+\treturn next;\n }\n \n-static void parse_cmd_option(const char *next)\n+static const char *parse_cmd_option(struct strbuf *input, const char *next)\n {\n-\tif (!strcmp(next, \"no-deref\"))\n+\tif (!strncmp(next, \"no-deref\", 8) && next[8] == line_termination)\n \t\tupdate_flags |= REF_NODEREF;\n \telse\n \t\tdie(\"option unknown: %s\", next);\n+\treturn next + 8;\n }\n \n static void update_refs_stdin(void)\n {\n-\tstruct strbuf cmd = STRBUF_INIT;\n+\tstruct strbuf input = STRBUF_INIT;\n+\tconst char *next;\n \n+\tif (strbuf_read(&input, 0, 1000) < 0)\n+\t\tdie_errno(\"could not read from stdin\");\n+\tnext = input.buf;\n \t/* Read each line dispatch its command */\n-\twhile (strbuf_getline(&cmd, stdin, line_termination) != EOF)\n-\t\tif (!cmd.buf[0])\n+\twhile (next < input.buf + input.len) {\n+\t\tif (*next == line_termination)\n \t\t\tdie(\"empty command in input\");\n-\t\telse if (isspace(*cmd.buf))\n-\t\t\tdie(\"whitespace before command: %s\", cmd.buf);\n-\t\telse if (starts_with(cmd.buf, \"update \"))\n-\t\t\tparse_cmd_update(cmd.buf + 7);\n-\t\telse if (starts_with(cmd.buf, \"create \"))\n-\t\t\tparse_cmd_create(cmd.buf + 7);\n-\t\telse if (starts_with(cmd.buf, \"delete \"))\n-\t\t\tparse_cmd_delete(cmd.buf + 7);\n-\t\telse if (starts_with(cmd.buf, \"verify \"))\n-\t\t\tparse_cmd_verify(cmd.buf + 7);\n-\t\telse if (starts_with(cmd.buf, \"option \"))\n-\t\t\tparse_cmd_option(cmd.buf + 7);\n+\t\telse if (isspace(*next))\n+\t\t\tdie(\"whitespace before command: %s\", next);\n+\t\telse if (starts_with(next, \"update \"))\n+\t\t\tnext = parse_cmd_update(&input, next + 7);\n+\t\telse if (starts_with(next, \"create \"))\n+\t\t\tnext = parse_cmd_create(&input, next + 7);\n+\t\telse if (starts_with(next, \"delete \"))\n+\t\t\tnext = parse_cmd_delete(&input, next + 7);\n+\t\telse if (starts_with(next, \"verify \"))\n+\t\t\tnext = parse_cmd_verify(&input, next + 7);\n+\t\telse if (starts_with(next, \"option \"))\n+\t\t\tnext = parse_cmd_option(&input, next + 7);\n \t\telse\n-\t\t\tdie(\"unknown command: %s\", cmd.buf);\n+\t\t\tdie(\"unknown command: %s\", next);\n+\n+\t\tnext++;\n+\t}\n \n-\tstrbuf_release(&cmd);\n+\tstrbuf_release(&input);\n }\n \n int cmd_update_ref(int argc, const char **argv, const char *prefix)\n-- \n1.9.0\n"},{"id":"237470","messageId":"1395683820-17304-9-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 08/27] parse_cmd_verify(): Copy old_sha1 instead of evaluating twice","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:41Z","receivedAt":"2014-03-24T17:56:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Aside from avoiding a tiny bit of work, this makes it transparently\nobvious that old_sha1 and new_sha1 are identical. It is arguably a\nbit silly to have to set new_sha1 in order to verify old_sha1, but\nthat is a problem for another day.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/update-ref.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 5f197fe..51adf2d 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -249,7 +249,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n \n \tif (!parse_next_arg(input, &next, &value)) {\n \t\tupdate_store_old_sha1(update, value.buf);\n-\t\tupdate_store_new_sha1(update, value.buf);\n+\t\thashcpy(update->new_sha1, update->old_sha1);\n \t} else if (!line_termination)\n \t\tdie(\"verify %s missing [] NUL\", ref.buf);\n \n-- \n1.9.0\n"},{"id":"237455","messageId":"1395683820-17304-10-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 09/27] update-ref.c: Extract a new function, parse_refname()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:42Z","receivedAt":"2014-03-24T17:56:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"There is no reason to obscure the fact that parse_first_arg() always\nparses refnames. Form the new function by combining parse_first_arg()\nand update_store_ref_name().\n\nSigned-off-by: Michael Haggerty \n---\n builtin/update-ref.c | 90 ++++++++++++++++++++++++----------------------------\n 1 file changed, 41 insertions(+), 49 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 51adf2d..0dc2061 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -35,14 +35,6 @@ static struct ref_update *update_alloc(void)\n \treturn update;\n }\n \n-static void update_store_ref_name(struct ref_update *update,\n-\t\t\t\t const char *ref_name)\n-{\n-\tif (check_refname_format(ref_name, REFNAME_ALLOW_ONELEVEL))\n-\t\tdie(\"invalid ref format: %s\", ref_name);\n-\tupdate->ref_name = xstrdup(ref_name);\n-}\n-\n static void update_store_new_sha1(struct ref_update *update,\n \t\t\t\t const char *newvalue)\n {\n@@ -86,23 +78,35 @@ static const char *parse_arg(const char *next, struct strbuf *arg)\n }\n \n /*\n- * Parse the argument immediately after \"command SP\". If not -z, then\n- * handle C-quoting. Write the argument to arg. Set *next to point\n- * at the character that terminates the argument. Die if C-quoting is\n- * malformed.\n+ * Parse the reference name immediately after \"command SP\". If not\n+ * -z, then handle C-quoting. Return a pointer to a newly allocated\n+ * string containing the name of the reference, or NULL if there was\n+ * an error. Update *next to point at the character that terminates\n+ * the argument. Die if C-quoting is malformed or the reference name\n+ * is invalid.\n */\n-static void parse_first_arg(struct strbuf *input, const char **next,\n-\t\t\t struct strbuf *arg)\n+static char *parse_refname(struct strbuf *input, const char **next)\n {\n-\tstrbuf_reset(arg);\n+\tstruct strbuf ref = STRBUF_INIT;\n+\n \tif (line_termination) {\n \t\t/* Without -z, use the next argument */\n-\t\t*next = parse_arg(*next, arg);\n+\t\t*next = parse_arg(*next, &ref);\n \t} else {\n \t\t/* With -z, use everything up to the next NUL */\n-\t\tstrbuf_addstr(arg, *next);\n-\t\t*next += arg->len;\n+\t\tstrbuf_addstr(&ref, *next);\n+\t\t*next += ref.len;\n+\t}\n+\n+\tif (!ref.len) {\n+\t\tstrbuf_release(&ref);\n+\t\treturn NULL;\n \t}\n+\n+\tif (check_refname_format(ref.buf, REFNAME_ALLOW_ONELEVEL))\n+\t\tdie(\"invalid ref format: %s\", ref.buf);\n+\n+\treturn strbuf_detach(&ref, NULL);\n }\n \n /*\n@@ -150,111 +154,99 @@ static int parse_next_arg(struct strbuf *input, const char **next,\n \n static const char *parse_cmd_update(struct strbuf *input, const char *next)\n {\n-\tstruct strbuf ref = STRBUF_INIT;\n \tstruct strbuf newvalue = STRBUF_INIT;\n \tstruct strbuf oldvalue = STRBUF_INIT;\n \tstruct ref_update *update;\n \n \tupdate = update_alloc();\n \n-\tparse_first_arg(input, &next, &ref);\n-\tif (ref.buf[0])\n-\t\tupdate_store_ref_name(update, ref.buf);\n-\telse\n+\tupdate->ref_name = parse_refname(input, &next);\n+\tif (!update->ref_name)\n \t\tdie(\"update line missing \");\n \n \tif (!parse_next_arg(input, &next, &newvalue))\n \t\tupdate_store_new_sha1(update, newvalue.buf);\n \telse\n-\t\tdie(\"update %s missing \", ref.buf);\n+\t\tdie(\"update %s missing \", update->ref_name);\n \n \tif (!parse_next_arg(input, &next, &oldvalue)) {\n \t\tupdate_store_old_sha1(update, oldvalue.buf);\n \t\tif (*next != line_termination)\n-\t\t\tdie(\"update %s has extra input: %s\", ref.buf, next);\n+\t\t\tdie(\"update %s has extra input: %s\", update->ref_name, next);\n \t} else if (!line_termination)\n-\t\tdie(\"update %s missing [] NUL\", ref.buf);\n+\t\tdie(\"update %s missing [] NUL\", update->ref_name);\n \n \treturn next;\n }\n \n static const char *parse_cmd_create(struct strbuf *input, const char *next)\n {\n-\tstruct strbuf ref = STRBUF_INIT;\n \tstruct strbuf newvalue = STRBUF_INIT;\n \tstruct ref_update *update;\n \n \tupdate = update_alloc();\n \n-\tparse_first_arg(input, &next, &ref);\n-\tif (ref.buf[0])\n-\t\tupdate_store_ref_name(update, ref.buf);\n-\telse\n+\tupdate->ref_name = parse_refname(input, &next);\n+\tif (!update->ref_name)\n \t\tdie(\"create line missing \");\n \n \tif (!parse_next_arg(input, &next, &newvalue))\n \t\tupdate_store_new_sha1(update, newvalue.buf);\n \telse\n-\t\tdie(\"create %s missing \", ref.buf);\n+\t\tdie(\"create %s missing \", update->ref_name);\n \n \tif (is_null_sha1(update->new_sha1))\n-\t\tdie(\"create %s given zero new value\", ref.buf);\n+\t\tdie(\"create %s given zero new value\", update->ref_name);\n \n \tif (*next != line_termination)\n-\t\tdie(\"create %s has extra input: %s\", ref.buf, next);\n+\t\tdie(\"create %s has extra input: %s\", update->ref_name, next);\n \n \treturn next;\n }\n \n static const char *parse_cmd_delete(struct strbuf *input, const char *next)\n {\n-\tstruct strbuf ref = STRBUF_INIT;\n \tstruct strbuf oldvalue = STRBUF_INIT;\n \tstruct ref_update *update;\n \n \tupdate = update_alloc();\n \n-\tparse_first_arg(input, &next, &ref);\n-\tif (ref.buf[0])\n-\t\tupdate_store_ref_name(update, ref.buf);\n-\telse\n+\tupdate->ref_name = parse_refname(input, &next);\n+\tif (!update->ref_name)\n \t\tdie(\"delete line missing \");\n \n \tif (!parse_next_arg(input, &next, &oldvalue)) {\n \t\tupdate_store_old_sha1(update, oldvalue.buf);\n \t\tif (update->have_old && is_null_sha1(update->old_sha1))\n-\t\t\tdie(\"delete %s given zero old value\", ref.buf);\n+\t\t\tdie(\"delete %s given zero old value\", update->ref_name);\n \t} else if (!line_termination)\n-\t\tdie(\"delete %s missing [] NUL\", ref.buf);\n+\t\tdie(\"delete %s missing [] NUL\", update->ref_name);\n \n \tif (*next != line_termination)\n-\t\tdie(\"delete %s has extra input: %s\", ref.buf, next);\n+\t\tdie(\"delete %s has extra input: %s\", update->ref_name, next);\n \n \treturn next;\n }\n \n static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n {\n-\tstruct strbuf ref = STRBUF_INIT;\n \tstruct strbuf value = STRBUF_INIT;\n \tstruct ref_update *update;\n \n \tupdate = update_alloc();\n \n-\tparse_first_arg(input, &next, &ref);\n-\tif (ref.buf[0])\n-\t\tupdate_store_ref_name(update, ref.buf);\n-\telse\n+\tupdate->ref_name = parse_refname(input, &next);\n+\tif (!update->ref_name)\n \t\tdie(\"verify line missing \");\n \n \tif (!parse_next_arg(input, &next, &value)) {\n \t\tupdate_store_old_sha1(update, value.buf);\n \t\thashcpy(update->new_sha1, update->old_sha1);\n \t} else if (!line_termination)\n-\t\tdie(\"verify %s missing [] NUL\", ref.buf);\n+\t\tdie(\"verify %s missing [] NUL\", update->ref_name);\n \n \tif (*next != line_termination)\n-\t\tdie(\"verify %s has extra input: %s\", ref.buf, next);\n+\t\tdie(\"verify %s has extra input: %s\", update->ref_name, next);\n \n \treturn next;\n }\n-- \n1.9.0\n"},{"id":"237475","messageId":"1395683820-17304-11-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 10/27] update-ref --stdin: Improve error messages for invalid values","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:43Z","receivedAt":"2014-03-24T17:56:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If an invalid value is passed to \"update-ref --stdin\" as or\n, include the command and the name of the reference at the\nbeginning of the error message. Update the tests accordingly.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/update-ref.c | 24 +++++++++++++-----------\n t/t1400-update-ref.sh | 8 ++++----\n 2 files changed, 17 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 0dc2061..13a884a 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -35,20 +35,22 @@ static struct ref_update *update_alloc(void)\n \treturn update;\n }\n \n-static void update_store_new_sha1(struct ref_update *update,\n+static void update_store_new_sha1(const char *command,\n+\t\t\t\t struct ref_update *update,\n \t\t\t\t const char *newvalue)\n {\n \tif (*newvalue && get_sha1(newvalue, update->new_sha1))\n-\t\tdie(\"invalid new value for ref %s: %s\",\n-\t\t update->ref_name, newvalue);\n+\t\tdie(\"%s %s: invalid new value: %s\",\n+\t\t command, update->ref_name, newvalue);\n }\n \n-static void update_store_old_sha1(struct ref_update *update,\n+static void update_store_old_sha1(const char *command,\n+\t\t\t\t struct ref_update *update,\n \t\t\t\t const char *oldvalue)\n {\n \tif (*oldvalue && get_sha1(oldvalue, update->old_sha1))\n-\t\tdie(\"invalid old value for ref %s: %s\",\n-\t\t update->ref_name, oldvalue);\n+\t\tdie(\"%s %s: invalid old value: %s\",\n+\t\t command, update->ref_name, oldvalue);\n \n \t/* We have an old value if non-empty, or if empty without -z */\n \tupdate->have_old = *oldvalue || line_termination;\n@@ -165,12 +167,12 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)\n \t\tdie(\"update line missing \");\n \n \tif (!parse_next_arg(input, &next, &newvalue))\n-\t\tupdate_store_new_sha1(update, newvalue.buf);\n+\t\tupdate_store_new_sha1(\"update\", update, newvalue.buf);\n \telse\n \t\tdie(\"update %s missing \", update->ref_name);\n \n \tif (!parse_next_arg(input, &next, &oldvalue)) {\n-\t\tupdate_store_old_sha1(update, oldvalue.buf);\n+\t\tupdate_store_old_sha1(\"update\", update, oldvalue.buf);\n \t\tif (*next != line_termination)\n \t\t\tdie(\"update %s has extra input: %s\", update->ref_name, next);\n \t} else if (!line_termination)\n@@ -191,7 +193,7 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)\n \t\tdie(\"create line missing \");\n \n \tif (!parse_next_arg(input, &next, &newvalue))\n-\t\tupdate_store_new_sha1(update, newvalue.buf);\n+\t\tupdate_store_new_sha1(\"create\", update, newvalue.buf);\n \telse\n \t\tdie(\"create %s missing \", update->ref_name);\n \n@@ -216,7 +218,7 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)\n \t\tdie(\"delete line missing \");\n \n \tif (!parse_next_arg(input, &next, &oldvalue)) {\n-\t\tupdate_store_old_sha1(update, oldvalue.buf);\n+\t\tupdate_store_old_sha1(\"delete\", update, oldvalue.buf);\n \t\tif (update->have_old && is_null_sha1(update->old_sha1))\n \t\t\tdie(\"delete %s given zero old value\", update->ref_name);\n \t} else if (!line_termination)\n@@ -240,7 +242,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n \t\tdie(\"verify line missing \");\n \n \tif (!parse_next_arg(input, &next, &value)) {\n-\t\tupdate_store_old_sha1(update, value.buf);\n+\t\tupdate_store_old_sha1(\"verify\", update, value.buf);\n \t\thashcpy(update->new_sha1, update->old_sha1);\n \t} else if (!line_termination)\n \t\tdie(\"verify %s missing [] NUL\", update->ref_name);\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 00862bc..f6c6e96 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -518,14 +518,14 @@ test_expect_success 'stdin update ref fails with wrong old value' '\n test_expect_success 'stdin update ref fails with bad old value' '\n \techo \"update $c $m does-not-exist\" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: invalid old value for ref $c: does-not-exist\" err &&\n+\tgrep \"fatal: update $c: invalid old value: does-not-exist\" err &&\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n test_expect_success 'stdin create ref fails with bad new value' '\n \techo \"create $c does-not-exist\" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: invalid new value for ref $c: does-not-exist\" err &&\n+\tgrep \"fatal: create $c: invalid new value: does-not-exist\" err &&\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n@@ -840,14 +840,14 @@ test_expect_success 'stdin -z update ref fails with wrong old value' '\n test_expect_success 'stdin -z update ref fails with bad old value' '\n \tprintf $F \"update $c\" \"$m\" \"does-not-exist\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: invalid old value for ref $c: does-not-exist\" err &&\n+\tgrep \"fatal: update $c: invalid old value: does-not-exist\" err &&\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n test_expect_success 'stdin -z create ref fails with bad new value' '\n \tprintf $F \"create $c\" \"does-not-exist\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: invalid new value for ref $c: does-not-exist\" err &&\n+\tgrep \"fatal: create $c: invalid new value: does-not-exist\" err &&\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n-- \n1.9.0\n"},{"id":"237474","messageId":"1395683820-17304-12-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 11/27] update-ref --stdin: Make error messages more consistent","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:44Z","receivedAt":"2014-03-24T17:56:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The old error messages emitted for invalid input sometimes said\n\"\"/\"\" and sometimes said \"old value\"/\"new value\".\nConvert them all to the former. Update the tests accordingly.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/update-ref.c | 8 ++++----\n t/t1400-update-ref.sh | 14 +++++++-------\n 2 files changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 13a884a..e4c0854 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -40,7 +40,7 @@ static void update_store_new_sha1(const char *command,\n \t\t\t\t const char *newvalue)\n {\n \tif (*newvalue && get_sha1(newvalue, update->new_sha1))\n-\t\tdie(\"%s %s: invalid new value: %s\",\n+\t\tdie(\"%s %s: invalid : %s\",\n \t\t command, update->ref_name, newvalue);\n }\n \n@@ -49,7 +49,7 @@ static void update_store_old_sha1(const char *command,\n \t\t\t\t const char *oldvalue)\n {\n \tif (*oldvalue && get_sha1(oldvalue, update->old_sha1))\n-\t\tdie(\"%s %s: invalid old value: %s\",\n+\t\tdie(\"%s %s: invalid : %s\",\n \t\t command, update->ref_name, oldvalue);\n \n \t/* We have an old value if non-empty, or if empty without -z */\n@@ -198,7 +198,7 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)\n \t\tdie(\"create %s missing \", update->ref_name);\n \n \tif (is_null_sha1(update->new_sha1))\n-\t\tdie(\"create %s given zero new value\", update->ref_name);\n+\t\tdie(\"create %s given zero \", update->ref_name);\n \n \tif (*next != line_termination)\n \t\tdie(\"create %s has extra input: %s\", update->ref_name, next);\n@@ -220,7 +220,7 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)\n \tif (!parse_next_arg(input, &next, &oldvalue)) {\n \t\tupdate_store_old_sha1(\"delete\", update, oldvalue.buf);\n \t\tif (update->have_old && is_null_sha1(update->old_sha1))\n-\t\t\tdie(\"delete %s given zero old value\", update->ref_name);\n+\t\t\tdie(\"delete %s given zero \", update->ref_name);\n \t} else if (!line_termination)\n \t\tdie(\"delete %s missing [] NUL\", update->ref_name);\n \ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex f6c6e96..ef61fe3 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -518,21 +518,21 @@ test_expect_success 'stdin update ref fails with wrong old value' '\n test_expect_success 'stdin update ref fails with bad old value' '\n \techo \"update $c $m does-not-exist\" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: update $c: invalid old value: does-not-exist\" err &&\n+\tgrep \"fatal: update $c: invalid : does-not-exist\" err &&\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n test_expect_success 'stdin create ref fails with bad new value' '\n \techo \"create $c does-not-exist\" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: create $c: invalid new value: does-not-exist\" err &&\n+\tgrep \"fatal: create $c: invalid : does-not-exist\" err &&\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n test_expect_success 'stdin create ref fails with zero new value' '\n \techo \"create $c \" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: create $c given zero new value\" err &&\n+\tgrep \"fatal: create $c given zero \" err &&\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n@@ -556,7 +556,7 @@ test_expect_success 'stdin delete ref fails with wrong old value' '\n test_expect_success 'stdin delete ref fails with zero old value' '\n \techo \"delete $a \" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: delete $a given zero old value\" err &&\n+\tgrep \"fatal: delete $a given zero \" err &&\n \tgit rev-parse $m >expect &&\n \tgit rev-parse $a >actual &&\n \ttest_cmp expect actual\n@@ -840,14 +840,14 @@ test_expect_success 'stdin -z update ref fails with wrong old value' '\n test_expect_success 'stdin -z update ref fails with bad old value' '\n \tprintf $F \"update $c\" \"$m\" \"does-not-exist\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: update $c: invalid old value: does-not-exist\" err &&\n+\tgrep \"fatal: update $c: invalid : does-not-exist\" err &&\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n test_expect_success 'stdin -z create ref fails with bad new value' '\n \tprintf $F \"create $c\" \"does-not-exist\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: create $c: invalid new value: does-not-exist\" err &&\n+\tgrep \"fatal: create $c: invalid : does-not-exist\" err &&\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n@@ -878,7 +878,7 @@ test_expect_success 'stdin -z delete ref fails with wrong old value' '\n test_expect_success 'stdin -z delete ref fails with zero old value' '\n \tprintf $F \"delete $a\" \"$Z\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: delete $a given zero old value\" err &&\n+\tgrep \"fatal: delete $a given zero \" err &&\n \tgit rev-parse $m >expect &&\n \tgit rev-parse $a >actual &&\n \ttest_cmp expect actual\n-- \n1.9.0\n"},{"id":"237457","messageId":"1395683820-17304-13-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 12/27] update-ref --stdin: Simplify error messages for missing oldvalues","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:45Z","receivedAt":"2014-03-24T17:56:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of, for example,\n\n fatal: update refs/heads/master missing [] NUL\n\nemit\n\n fatal: update refs/heads/master missing \n\nUpdate the tests accordingly.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/update-ref.c | 6 +++---\n t/t1400-update-ref.sh | 6 +++---\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex e4c0854..a9eb5fe 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -176,7 +176,7 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)\n \t\tif (*next != line_termination)\n \t\t\tdie(\"update %s has extra input: %s\", update->ref_name, next);\n \t} else if (!line_termination)\n-\t\tdie(\"update %s missing [] NUL\", update->ref_name);\n+\t\tdie(\"update %s missing \", update->ref_name);\n \n \treturn next;\n }\n@@ -222,7 +222,7 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)\n \t\tif (update->have_old && is_null_sha1(update->old_sha1))\n \t\t\tdie(\"delete %s given zero \", update->ref_name);\n \t} else if (!line_termination)\n-\t\tdie(\"delete %s missing [] NUL\", update->ref_name);\n+\t\tdie(\"delete %s missing \", update->ref_name);\n \n \tif (*next != line_termination)\n \t\tdie(\"delete %s has extra input: %s\", update->ref_name, next);\n@@ -245,7 +245,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n \t\tupdate_store_old_sha1(\"verify\", update, value.buf);\n \t\thashcpy(update->new_sha1, update->old_sha1);\n \t} else if (!line_termination)\n-\t\tdie(\"verify %s missing [] NUL\", update->ref_name);\n+\t\tdie(\"verify %s missing \", update->ref_name);\n \n \tif (*next != line_termination)\n \t\tdie(\"verify %s has extra input: %s\", update->ref_name, next);\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex ef61fe3..a2015d0 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -739,7 +739,7 @@ test_expect_success 'stdin -z fails update with no new value' '\n test_expect_success 'stdin -z fails update with no old value' '\n \tprintf $F \"update $a\" \"$m\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: update $a missing \\\\[\\\\] NUL\" err\n+\tgrep \"fatal: update $a missing \" err\n '\n \n test_expect_success 'stdin -z fails update with too many arguments' '\n@@ -763,7 +763,7 @@ test_expect_success 'stdin -z fails delete with bad ref name' '\n test_expect_success 'stdin -z fails delete with no old value' '\n \tprintf $F \"delete $a\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: delete $a missing \\\\[\\\\] NUL\" err\n+\tgrep \"fatal: delete $a missing \" err\n '\n \n test_expect_success 'stdin -z fails delete with too many arguments' '\n@@ -781,7 +781,7 @@ test_expect_success 'stdin -z fails verify with too many arguments' '\n test_expect_success 'stdin -z fails verify with no old value' '\n \tprintf $F \"verify $a\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: verify $a missing \\\\[\\\\] NUL\" err\n+\tgrep \"fatal: verify $a missing \" err\n '\n \n test_expect_success 'stdin -z fails option with unknown name' '\n-- \n1.9.0\n"},{"id":"237463","messageId":"1395683820-17304-14-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 13/27] t1400: Test that stdin -z update treats empty as zeros","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:46Z","receivedAt":"2014-03-24T17:56:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is the (slightly inconsistent) status quo; make sure it doesn't\nchange by accident.\n\nSigned-off-by: Michael Haggerty \n---\n t/t1400-update-ref.sh | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex a2015d0..208f56e 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -730,6 +730,13 @@ test_expect_success 'stdin -z fails update with bad ref name' '\n \tgrep \"fatal: invalid ref format: ~a\" err\n '\n \n+test_expect_success 'stdin -z treats empty new value as zeros' '\n+\tgit update-ref $a $m &&\n+\tprintf $F \"update $a\" \"\" \"\" >stdin &&\n+\tgit update-ref -z --stdin stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-- \n1.9.0\n"},{"id":"237476","messageId":"1395683820-17304-15-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 14/27] update-ref.c: Extract a new function, parse_next_sha1()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:47Z","receivedAt":"2014-03-24T17:56:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Replace three functions, update_store_new_sha1(),\nupdate_store_old_sha1(), and parse_next_arg(), with a single function,\nparse_next_sha1(). The new function takes care of a whole argument,\nincluding checking whether it is there, converting it to an SHA-1, and\nemitting errors on EOF or for invalid values. The return value\nindicates whether the argument was present or absent, which requires\na bit of intelligence because absent values are represented\ndifferently depending on whether \"-z\" was used.\n\nThe new interface means that the calling functions, parse_cmd_*(),\ndon't have to interpret the result differently based on the\nline_termination mode that is in effect. It also means that\nparse_cmd_create() can distinguish unambiguously between an empty new\nvalue and a zeros new value, which fixes a failure in t1400.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/update-ref.c | 160 +++++++++++++++++++++++++++++++-------------------\n t/t1400-update-ref.sh | 2 +-\n 2 files changed, 99 insertions(+), 63 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex a9eb5fe..6462b2f 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -35,27 +35,6 @@ static struct ref_update *update_alloc(void)\n \treturn update;\n }\n \n-static void update_store_new_sha1(const char *command,\n-\t\t\t\t struct ref_update *update,\n-\t\t\t\t const char *newvalue)\n-{\n-\tif (*newvalue && get_sha1(newvalue, update->new_sha1))\n-\t\tdie(\"%s %s: invalid : %s\",\n-\t\t command, update->ref_name, newvalue);\n-}\n-\n-static void update_store_old_sha1(const char *command,\n-\t\t\t\t struct ref_update *update,\n-\t\t\t\t const char *oldvalue)\n-{\n-\tif (*oldvalue && get_sha1(oldvalue, update->old_sha1))\n-\t\tdie(\"%s %s: invalid : %s\",\n-\t\t command, update->ref_name, oldvalue);\n-\n-\t/* We have an old value if non-empty, or if empty without -z */\n-\tupdate->have_old = *oldvalue || line_termination;\n-}\n-\n /*\n * Parse one whitespace- or NUL-terminated, possibly C-quoted argument\n * and append the result to arg. Return a pointer to the terminator.\n@@ -112,35 +91,94 @@ static char *parse_refname(struct strbuf *input, const char **next)\n }\n \n /*\n- * Parse a SP/NUL separator followed by the next SP- or NUL-terminated\n- * argument, if any. If there is an argument, write it to arg, set\n- * *next to point at the character terminating the argument, and\n+ * The value being parsed is (as opposed to ; the\n+ * difference affects which error messages are generated):\n+ */\n+#define PARSE_SHA1_OLD 0x01\n+\n+/*\n+ * For backwards compatibility, accept an empty string for create's\n+ * in binary mode to be equivalent to specifying zeros.\n+ */\n+#define PARSE_SHA1_ALLOW_EMPTY 0x02\n+\n+/*\n+ * Parse an argument separator followed by the next argument, if any.\n+ * If there is an argument, convert it to a SHA-1, write it to sha1,\n+ * set *next to point at the character terminating the argument, and\n * return 0. If there is no argument at all (not even the empty\n- * string), return a non-zero result and leave *next unchanged.\n+ * string), return 1 and leave *next unchanged. If the value is\n+ * provided but cannot be converted to a SHA-1, die. flags can\n+ * include PARSE_SHA1_OLD and/or PARSE_SHA1_ALLOW_EMPTY.\n */\n-static int parse_next_arg(struct strbuf *input, const char **next,\n-\t\t\t struct strbuf *arg)\n+static int parse_next_sha1(struct strbuf *input, const char **next,\n+\t\t\t unsigned char *sha1,\n+\t\t\t const char *command, const char *refname,\n+\t\t\t int flags)\n {\n-\tstrbuf_reset(arg);\n+\tstruct strbuf arg = STRBUF_INIT;\n+\tint ret = 0;\n+\n+\tif (*next == input->buf + input->len)\n+\t\tgoto eof;\n+\n \tif (line_termination) {\n \t\t/* Without -z, consume SP and use next argument */\n \t\tif (!**next || **next == line_termination)\n-\t\t\treturn -1;\n+\t\t\treturn 1;\n \t\tif (**next != ' ')\n-\t\t\tdie(\"expected SP but got: %s\", *next);\n+\t\t\tdie(\"%s %s: expected SP but got: %s\",\n+\t\t\t command, refname, *next);\n \t\t(*next)++;\n-\t\t*next = parse_arg(*next, arg);\n+\t\t*next = parse_arg(*next, &arg);\n+\t\tif (arg.len) {\n+\t\t\tif (get_sha1(arg.buf, sha1))\n+\t\t\t\tgoto invalid;\n+\t\t} else {\n+\t\t\t/* Without -z, an empty value means all zeros: */\n+\t\t\thashclr(sha1);\n+\t\t}\n \t} else {\n \t\t/* With -z, read the next NUL-terminated line */\n \t\tif (**next)\n-\t\t\tdie(\"expected NUL but got: %s\", *next);\n+\t\t\tdie(\"%s %s: expected NUL but got: %s\",\n+\t\t\t command, refname, *next);\n \t\t(*next)++;\n \t\tif (*next == input->buf + input->len)\n-\t\t\treturn -1;\n-\t\tstrbuf_addstr(arg, *next);\n-\t\t*next += arg->len;\n+\t\t\tgoto eof;\n+\t\tstrbuf_addstr(&arg, *next);\n+\t\t*next += arg.len;\n+\n+\t\tif (arg.len) {\n+\t\t\tif (get_sha1(arg.buf, sha1))\n+\t\t\t\tgoto invalid;\n+\t\t} else if (flags & PARSE_SHA1_ALLOW_EMPTY) {\n+\t\t\t/* With -z, treat an empty value as all zeros: */\n+\t\t\thashclr(sha1);\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * With -z, an empty non-required value means\n+\t\t\t * unspecified:\n+\t\t\t */\n+\t\t\tret = 1;\n+\t\t}\n \t}\n-\treturn 0;\n+\n+\tstrbuf_release(&arg);\n+\n+\treturn ret;\n+\n+ invalid:\n+\tdie(flags & PARSE_SHA1_OLD ?\n+\t \"%s %s: invalid : %s\" :\n+\t \"%s %s: invalid : %s\",\n+\t command, refname, arg.buf);\n+\n+ eof:\n+\tdie(flags & PARSE_SHA1_OLD ?\n+\t \"%s %s missing \" :\n+\t \"%s %s missing \",\n+\t command, refname);\n }\n \n \n@@ -156,8 +194,6 @@ static int parse_next_arg(struct strbuf *input, const char **next,\n \n static const char *parse_cmd_update(struct strbuf *input, const char *next)\n {\n-\tstruct strbuf newvalue = STRBUF_INIT;\n-\tstruct strbuf oldvalue = STRBUF_INIT;\n \tstruct ref_update *update;\n \n \tupdate = update_alloc();\n@@ -166,24 +202,23 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)\n \tif (!update->ref_name)\n \t\tdie(\"update line missing \");\n \n-\tif (!parse_next_arg(input, &next, &newvalue))\n-\t\tupdate_store_new_sha1(\"update\", update, newvalue.buf);\n-\telse\n+\tif (parse_next_sha1(input, &next, update->new_sha1,\n+\t\t\t \"update\", update->ref_name,\n+\t\t\t PARSE_SHA1_ALLOW_EMPTY))\n \t\tdie(\"update %s missing \", update->ref_name);\n \n-\tif (!parse_next_arg(input, &next, &oldvalue)) {\n-\t\tupdate_store_old_sha1(\"update\", update, oldvalue.buf);\n-\t\tif (*next != line_termination)\n-\t\t\tdie(\"update %s has extra input: %s\", update->ref_name, next);\n-\t} else if (!line_termination)\n-\t\tdie(\"update %s missing \", update->ref_name);\n+\tupdate->have_old = !parse_next_sha1(input, &next, update->old_sha1,\n+\t\t\t\t\t \"update\", update->ref_name,\n+\t\t\t\t\t PARSE_SHA1_OLD);\n+\n+\tif (*next != line_termination)\n+\t\tdie(\"update %s has extra input: %s\", update->ref_name, next);\n \n \treturn next;\n }\n \n static const char *parse_cmd_create(struct strbuf *input, const char *next)\n {\n-\tstruct strbuf newvalue = STRBUF_INIT;\n \tstruct ref_update *update;\n \n \tupdate = update_alloc();\n@@ -192,9 +227,8 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)\n \tif (!update->ref_name)\n \t\tdie(\"create line missing \");\n \n-\tif (!parse_next_arg(input, &next, &newvalue))\n-\t\tupdate_store_new_sha1(\"create\", update, newvalue.buf);\n-\telse\n+\tif (parse_next_sha1(input, &next, update->new_sha1,\n+\t\t\t \"create\", update->ref_name, 0))\n \t\tdie(\"create %s missing \", update->ref_name);\n \n \tif (is_null_sha1(update->new_sha1))\n@@ -208,7 +242,6 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)\n \n static const char *parse_cmd_delete(struct strbuf *input, const char *next)\n {\n-\tstruct strbuf oldvalue = STRBUF_INIT;\n \tstruct ref_update *update;\n \n \tupdate = update_alloc();\n@@ -217,12 +250,14 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)\n \tif (!update->ref_name)\n \t\tdie(\"delete line missing \");\n \n-\tif (!parse_next_arg(input, &next, &oldvalue)) {\n-\t\tupdate_store_old_sha1(\"delete\", update, oldvalue.buf);\n-\t\tif (update->have_old && is_null_sha1(update->old_sha1))\n+\tif (parse_next_sha1(input, &next, update->old_sha1,\n+\t\t\t \"delete\", update->ref_name, PARSE_SHA1_OLD)) {\n+\t\tupdate->have_old = 0;\n+\t} else {\n+\t\tif (is_null_sha1(update->old_sha1))\n \t\t\tdie(\"delete %s given zero \", update->ref_name);\n-\t} else if (!line_termination)\n-\t\tdie(\"delete %s missing \", update->ref_name);\n+\t\tupdate->have_old = 1;\n+\t}\n \n \tif (*next != line_termination)\n \t\tdie(\"delete %s has extra input: %s\", update->ref_name, next);\n@@ -232,7 +267,6 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)\n \n static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n {\n-\tstruct strbuf value = STRBUF_INIT;\n \tstruct ref_update *update;\n \n \tupdate = update_alloc();\n@@ -241,11 +275,13 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n \tif (!update->ref_name)\n \t\tdie(\"verify line missing \");\n \n-\tif (!parse_next_arg(input, &next, &value)) {\n-\t\tupdate_store_old_sha1(\"verify\", update, value.buf);\n+\tif (parse_next_sha1(input, &next, update->old_sha1,\n+\t\t\t \"verify\", update->ref_name, PARSE_SHA1_OLD)) {\n+\t\tupdate->have_old = 0;\n+\t} else {\n \t\thashcpy(update->new_sha1, update->old_sha1);\n-\t} else if (!line_termination)\n-\t\tdie(\"verify %s missing \", update->ref_name);\n+\t\tupdate->have_old = 1;\n+\t}\n \n \tif (*next != line_termination)\n \t\tdie(\"verify %s has extra input: %s\", update->ref_name, next);\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 208f56e..15f5bfd 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -858,7 +858,7 @@ test_expect_success 'stdin -z create ref fails with bad new value' '\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n-test_expect_failure 'stdin -z create ref fails with empty new value' '\n+test_expect_success 'stdin -z create ref fails with empty new value' '\n \tprintf $F \"create $c\" \"\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n \tgrep \"fatal: create $c missing \" err &&\n-- \n1.9.0\n"},{"id":"237462","messageId":"1395683820-17304-16-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 15/27] update-ref --stdin -z: Deprecate interpreting the empty string as zeros","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:48Z","receivedAt":"2014-03-24T17:56:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"In the original version of this command, for the single case of the\n\"update\" command's , the empty string was interpreted as\nbeing equivalent to 40 \"0\"s. This shorthand is unnecessary (binary\ninput will usually be generated programmatically anyway), and it\ncomplicates the parser and the documentation.\n\nSo gently deprecate this usage: remove its description from the\ndocumentation and emit a warning if it is found. But for reasons of\nbackwards compatibility, continue to accept it.\n\nHelped-by: Brad King \nSigned-off-by: Michael Haggerty \n---\n Documentation/git-update-ref.txt | 18 ++++++++++++------\n builtin/update-ref.c | 2 ++\n t/t1400-update-ref.sh | 5 +++--\n 3 files changed, 17 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt\nindex 0a0a551..c8f5ae5 100644\n--- a/Documentation/git-update-ref.txt\n+++ b/Documentation/git-update-ref.txt\n@@ -68,7 +68,12 @@ performs all modifications together. Specify commands of the form:\n \toption SP LF\n \n Quote fields containing whitespace as if they were strings in C source\n-code. Alternatively, use `-z` to specify commands without quoting:\n+code; i.e., surrounded by double-quotes and with backslash escapes.\n+Use 40 \"0\" characters or the empty string to specify a zero value. To\n+specify a missing value, omit the value and its preceding SP entirely.\n+\n+Alternatively, use `-z` to specify in NUL-terminated format, without\n+quoting:\n \n \tupdate SP NUL NUL [] NUL\n \tcreate SP NUL NUL\n@@ -76,8 +81,12 @@ code. Alternatively, use `-z` to specify commands without quoting:\n \tverify SP NUL [] NUL\n \toption SP NUL\n \n-Lines of any other format or a repeated produce an error.\n-Command meanings are:\n+In this format, use 40 \"0\" to specify a zero value, and use the empty\n+string to specify a missing value.\n+\n+In either format, values can be specified in any form that Git\n+recognizes as an object name. Commands in any other format or a\n+repeated produce an error. Command meanings are:\n \n update::\n \tSet to after verifying , if given.\n@@ -102,9 +111,6 @@ option::\n \tThe only valid option is `no-deref` to avoid dereferencing\n \ta symbolic ref.\n \n-Use 40 \"0\" or the empty string to specify a zero value, except that\n-with `-z` an empty is considered missing.\n-\n If all s can be locked with matching s\n simultaneously, all modifications are performed. Otherwise, no\n modifications are performed. Note that while each individual\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 6462b2f..eef7537 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -154,6 +154,8 @@ static int parse_next_sha1(struct strbuf *input, const char **next,\n \t\t\t\tgoto invalid;\n \t\t} else if (flags & PARSE_SHA1_ALLOW_EMPTY) {\n \t\t\t/* With -z, treat an empty value as all zeros: */\n+\t\t\twarning(\"%s %s: missing , treating as zero\",\n+\t\t\t\tcommand, refname);\n \t\t\thashclr(sha1);\n \t\t} else {\n \t\t\t/*\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 15f5bfd..2d61cce 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -730,10 +730,11 @@ test_expect_success 'stdin -z fails update with bad ref name' '\n \tgrep \"fatal: invalid ref format: ~a\" err\n '\n \n-test_expect_success 'stdin -z treats empty new value as zeros' '\n+test_expect_success 'stdin -z emits warning with empty new value' '\n \tgit update-ref $a $m &&\n \tprintf $F \"update $a\" \"\" \"\" >stdin &&\n-\tgit update-ref -z --stdin err &&\n+\tgrep \"warning: update $a: missing , treating as zero\" err &&\n \ttest_must_fail git rev-parse --verify -q $a\n '\n \n-- \n1.9.0\n"},{"id":"237459","messageId":"1395683820-17304-17-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 16/27] t1400: Test one mistake at a time","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:49Z","receivedAt":"2014-03-24T17:56:49Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This case wants to test passing a bad refname to the \"update\" command.\nBut it also passes too few arguments to \"update\", which muddles the\nsituation: which error should be diagnosed? So split this test into\ntwo:\n\n* One that passes too few arguments to update\n\n* One that passes all three arguments to \"update\", but with a bad\n refname.\n\nSigned-off-by: Michael Haggerty \n\nt1400: Add a test of \"update\" with too few arguments\n\nSigned-off-by: Michael Haggerty \n---\n t/t1400-update-ref.sh | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 2d61cce..6b21e45 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -724,8 +724,14 @@ test_expect_success 'stdin -z fails update with no ref' '\n \tgrep \"fatal: update line missing \" err\n '\n \n+test_expect_success 'stdin -z fails update with too few args' '\n+\tprintf $F \"update $a\" \"$m\" >stdin &&\n+\ttest_must_fail git update-ref -z --stdin err &&\n+\tgrep \"fatal: update $a missing \" err\n+'\n+\n test_expect_success 'stdin -z fails update with bad ref name' '\n-\tprintf $F \"update ~a\" \"$m\" >stdin &&\n+\tprintf $F \"update ~a\" \"$m\" \"\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n \tgrep \"fatal: invalid ref format: ~a\" err\n '\n-- \n1.9.0\n"},{"id":"237461","messageId":"1395683820-17304-18-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 17/27] update-ref --stdin: Improve the error message for unexpected EOF","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:50Z","receivedAt":"2014-03-24T17:56:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Distinguish this error from the error that an argument is missing for\nanother reason. Update the tests accordingly.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/update-ref.c | 4 ++--\n t/t1400-update-ref.sh | 12 ++++++------\n 2 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex eef7537..b49a5b0 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -178,8 +178,8 @@ static int parse_next_sha1(struct strbuf *input, const char **next,\n \n eof:\n \tdie(flags & PARSE_SHA1_OLD ?\n-\t \"%s %s missing \" :\n-\t \"%s %s missing \",\n+\t \"%s %s: unexpected end of input when reading \" :\n+\t \"%s %s: unexpected end of input when reading \",\n \t command, refname);\n }\n \ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 6b21e45..1db0689 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -709,7 +709,7 @@ test_expect_success 'stdin -z fails create with bad ref name' '\n test_expect_success 'stdin -z fails create with no new value' '\n \tprintf $F \"create $a\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: create $a missing \" err\n+\tgrep \"fatal: create $a: unexpected end of input when reading \" err\n '\n \n test_expect_success 'stdin -z fails create with too many arguments' '\n@@ -727,7 +727,7 @@ test_expect_success 'stdin -z fails update with no ref' '\n test_expect_success 'stdin -z fails update with too few args' '\n \tprintf $F \"update $a\" \"$m\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: update $a missing \" err\n+\tgrep \"fatal: update $a: unexpected end of input when reading \" err\n '\n \n test_expect_success 'stdin -z fails update with bad ref name' '\n@@ -747,13 +747,13 @@ test_expect_success 'stdin -z emits warning with empty new value' '\n test_expect_success 'stdin -z fails update with no new value' '\n \tprintf $F \"update $a\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: update $a missing \" err\n+\tgrep \"fatal: update $a: unexpected end of input when reading \" err\n '\n \n test_expect_success 'stdin -z fails update with no old value' '\n \tprintf $F \"update $a\" \"$m\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: update $a missing \" err\n+\tgrep \"fatal: update $a: unexpected end of input when reading \" err\n '\n \n test_expect_success 'stdin -z fails update with too many arguments' '\n@@ -777,7 +777,7 @@ test_expect_success 'stdin -z fails delete with bad ref name' '\n test_expect_success 'stdin -z fails delete with no old value' '\n \tprintf $F \"delete $a\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: delete $a missing \" err\n+\tgrep \"fatal: delete $a: unexpected end of input when reading \" err\n '\n \n test_expect_success 'stdin -z fails delete with too many arguments' '\n@@ -795,7 +795,7 @@ test_expect_success 'stdin -z fails verify with too many arguments' '\n test_expect_success 'stdin -z fails verify with no old value' '\n \tprintf $F \"verify $a\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: verify $a missing \" err\n+\tgrep \"fatal: verify $a: unexpected end of input when reading \" err\n '\n \n test_expect_success 'stdin -z fails option with unknown name' '\n-- \n1.9.0\n"},{"id":"237466","messageId":"1395683820-17304-19-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 18/27] update-ref --stdin: Harmonize error messages","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:51Z","receivedAt":"2014-03-24T17:56:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Make (most of) the error messages for invalid input have the same\nformat [1]:\n\n $COMMAND [SP $REFNAME]: $MESSAGE\n\nUpdate the tests accordingly.\n\n[1] A few error messages are left with their old form, because\n $COMMAND and $REFNAME aren't passed all the way down the call\n stack. Maybe those sites should be changed some day, too.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/update-ref.c | 24 ++++++++++++------------\n t/t1400-update-ref.sh | 32 ++++++++++++++++----------------\n 2 files changed, 28 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex b49a5b0..bbc04b2 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -202,19 +202,19 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)\n \n \tupdate->ref_name = parse_refname(input, &next);\n \tif (!update->ref_name)\n-\t\tdie(\"update line missing \");\n+\t\tdie(\"update: missing \");\n \n \tif (parse_next_sha1(input, &next, update->new_sha1,\n \t\t\t \"update\", update->ref_name,\n \t\t\t PARSE_SHA1_ALLOW_EMPTY))\n-\t\tdie(\"update %s missing \", update->ref_name);\n+\t\tdie(\"update %s: missing \", update->ref_name);\n \n \tupdate->have_old = !parse_next_sha1(input, &next, update->old_sha1,\n \t\t\t\t\t \"update\", update->ref_name,\n \t\t\t\t\t PARSE_SHA1_OLD);\n \n \tif (*next != line_termination)\n-\t\tdie(\"update %s has extra input: %s\", update->ref_name, next);\n+\t\tdie(\"update %s: extra input: %s\", update->ref_name, next);\n \n \treturn next;\n }\n@@ -227,17 +227,17 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)\n \n \tupdate->ref_name = parse_refname(input, &next);\n \tif (!update->ref_name)\n-\t\tdie(\"create line missing \");\n+\t\tdie(\"create: missing \");\n \n \tif (parse_next_sha1(input, &next, update->new_sha1,\n \t\t\t \"create\", update->ref_name, 0))\n-\t\tdie(\"create %s missing \", update->ref_name);\n+\t\tdie(\"create %s: missing \", update->ref_name);\n \n \tif (is_null_sha1(update->new_sha1))\n-\t\tdie(\"create %s given zero \", update->ref_name);\n+\t\tdie(\"create %s: zero \", update->ref_name);\n \n \tif (*next != line_termination)\n-\t\tdie(\"create %s has extra input: %s\", update->ref_name, next);\n+\t\tdie(\"create %s: extra input: %s\", update->ref_name, next);\n \n \treturn next;\n }\n@@ -250,19 +250,19 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)\n \n \tupdate->ref_name = parse_refname(input, &next);\n \tif (!update->ref_name)\n-\t\tdie(\"delete line missing \");\n+\t\tdie(\"delete: missing \");\n \n \tif (parse_next_sha1(input, &next, update->old_sha1,\n \t\t\t \"delete\", update->ref_name, PARSE_SHA1_OLD)) {\n \t\tupdate->have_old = 0;\n \t} else {\n \t\tif (is_null_sha1(update->old_sha1))\n-\t\t\tdie(\"delete %s given zero \", update->ref_name);\n+\t\t\tdie(\"delete %s: zero \", update->ref_name);\n \t\tupdate->have_old = 1;\n \t}\n \n \tif (*next != line_termination)\n-\t\tdie(\"delete %s has extra input: %s\", update->ref_name, next);\n+\t\tdie(\"delete %s: extra input: %s\", update->ref_name, next);\n \n \treturn next;\n }\n@@ -275,7 +275,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n \n \tupdate->ref_name = parse_refname(input, &next);\n \tif (!update->ref_name)\n-\t\tdie(\"verify line missing \");\n+\t\tdie(\"verify: missing \");\n \n \tif (parse_next_sha1(input, &next, update->old_sha1,\n \t\t\t \"verify\", update->ref_name, PARSE_SHA1_OLD)) {\n@@ -286,7 +286,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n \t}\n \n \tif (*next != line_termination)\n-\t\tdie(\"verify %s has extra input: %s\", update->ref_name, next);\n+\t\tdie(\"verify %s: extra input: %s\", update->ref_name, next);\n \n \treturn next;\n }\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 1db0689..48ccc4d 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -371,7 +371,7 @@ test_expect_success 'stdin fails on junk after quoted argument' '\n test_expect_success 'stdin fails create with no ref' '\n \techo \"create \" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: create line missing \" err\n+\tgrep \"fatal: create: missing \" err\n '\n \n test_expect_success 'stdin fails create with bad ref name' '\n@@ -383,19 +383,19 @@ test_expect_success 'stdin fails create with bad ref name' '\n test_expect_success 'stdin fails create with no new value' '\n \techo \"create $a\" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: create $a missing \" err\n+\tgrep \"fatal: create $a: missing \" err\n '\n \n test_expect_success 'stdin fails create with too many arguments' '\n \techo \"create $a $m $m\" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: create $a has extra input: $m\" err\n+\tgrep \"fatal: create $a: extra input: $m\" err\n '\n \n test_expect_success 'stdin fails update with no ref' '\n \techo \"update \" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: update line missing \" err\n+\tgrep \"fatal: update: missing \" err\n '\n \n test_expect_success 'stdin fails update with bad ref name' '\n@@ -407,19 +407,19 @@ test_expect_success 'stdin fails update with bad ref name' '\n test_expect_success 'stdin fails update with no new value' '\n \techo \"update $a\" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: update $a missing \" err\n+\tgrep \"fatal: update $a: missing \" err\n '\n \n test_expect_success 'stdin fails update with too many arguments' '\n \techo \"update $a $m $m $m\" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: update $a has extra input: $m\" err\n+\tgrep \"fatal: update $a: extra input: $m\" err\n '\n \n test_expect_success 'stdin fails delete with no ref' '\n \techo \"delete \" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: delete line missing \" err\n+\tgrep \"fatal: delete: missing \" err\n '\n \n test_expect_success 'stdin fails delete with bad ref name' '\n@@ -431,13 +431,13 @@ test_expect_success 'stdin fails delete with bad ref name' '\n test_expect_success 'stdin fails delete with too many arguments' '\n \techo \"delete $a $m $m\" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: delete $a has extra input: $m\" err\n+\tgrep \"fatal: delete $a: extra input: $m\" err\n '\n \n test_expect_success 'stdin fails verify with too many arguments' '\n \techo \"verify $a $m $m\" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: verify $a has extra input: $m\" err\n+\tgrep \"fatal: verify $a: extra input: $m\" err\n '\n \n test_expect_success 'stdin fails option with unknown name' '\n@@ -532,7 +532,7 @@ test_expect_success 'stdin create ref fails with bad new value' '\n test_expect_success 'stdin create ref fails with zero new value' '\n \techo \"create $c \" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: create $c given zero \" err &&\n+\tgrep \"fatal: create $c: zero \" err &&\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n@@ -556,7 +556,7 @@ test_expect_success 'stdin delete ref fails with wrong old value' '\n test_expect_success 'stdin delete ref fails with zero old value' '\n \techo \"delete $a \" >stdin &&\n \ttest_must_fail git update-ref --stdin err &&\n-\tgrep \"fatal: delete $a given zero \" err &&\n+\tgrep \"fatal: delete $a: zero \" err &&\n \tgit rev-parse $m >expect &&\n \tgit rev-parse $a >actual &&\n \ttest_cmp expect actual\n@@ -697,7 +697,7 @@ test_expect_success 'stdin -z fails on unknown command' '\n test_expect_success 'stdin -z fails create with no ref' '\n \tprintf $F \"create \" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: create line missing \" err\n+\tgrep \"fatal: create: missing \" err\n '\n \n test_expect_success 'stdin -z fails create with bad ref name' '\n@@ -721,7 +721,7 @@ test_expect_success 'stdin -z fails create with too many arguments' '\n test_expect_success 'stdin -z fails update with no ref' '\n \tprintf $F \"update \" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: update line missing \" err\n+\tgrep \"fatal: update: missing \" err\n '\n \n test_expect_success 'stdin -z fails update with too few args' '\n@@ -765,7 +765,7 @@ test_expect_success 'stdin -z fails update with too many arguments' '\n test_expect_success 'stdin -z fails delete with no ref' '\n \tprintf $F \"delete \" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: delete line missing \" err\n+\tgrep \"fatal: delete: missing \" err\n '\n \n test_expect_success 'stdin -z fails delete with bad ref name' '\n@@ -868,7 +868,7 @@ test_expect_success 'stdin -z create ref fails with bad new value' '\n test_expect_success 'stdin -z create ref fails with empty new value' '\n \tprintf $F \"create $c\" \"\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: create $c missing \" err &&\n+\tgrep \"fatal: create $c: missing \" err &&\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n@@ -892,7 +892,7 @@ test_expect_success 'stdin -z delete ref fails with wrong old value' '\n test_expect_success 'stdin -z delete ref fails with zero old value' '\n \tprintf $F \"delete $a\" \"$Z\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin err &&\n-\tgrep \"fatal: delete $a given zero \" err &&\n+\tgrep \"fatal: delete $a: zero \" err &&\n \tgit rev-parse $m >expect &&\n \tgit rev-parse $a >actual &&\n \ttest_cmp expect actual\n-- \n1.9.0\n"},{"id":"237465","messageId":"1395683820-17304-20-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 19/27] refs: Add a concept of a reference transaction","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:52Z","receivedAt":"2014-03-24T17:56:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Build out the API for dealing with a bunch of reference checks and\nchanges within a transaction. Define an opaque ref_transaction type\nthat is managed entirely within refs.c. Introduce functions for\nbeginning a transaction, adding updates to a transaction, and\ncommitting/rolling back a transaction.\n\nThis API will soon replace update_refs().\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n refs.h | 65 +++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 161 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex 1305eb1..e788c27 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3267,6 +3267,93 @@ static int update_ref_write(const char *action, const char *refname,\n \treturn 0;\n }\n \n+/*\n+ * Data structure for holding a reference transaction, which can\n+ * consist of checks and updates to multiple references, carried out\n+ * as atomically as possible. This structure is opaque to callers.\n+ */\n+struct ref_transaction {\n+\tstruct ref_update **updates;\n+\tsize_t alloc;\n+\tsize_t nr;\n+};\n+\n+struct ref_transaction *ref_transaction_begin(void)\n+{\n+\treturn xcalloc(1, sizeof(struct ref_transaction));\n+}\n+\n+static void ref_transaction_free(struct ref_transaction *transaction)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < transaction->nr; i++) {\n+\t\tstruct ref_update *update = transaction->updates[i];\n+\n+\t\tfree((char *)update->ref_name);\n+\t\tfree(update);\n+\t}\n+\n+\tfree(transaction->updates);\n+\tfree(transaction);\n+}\n+\n+void ref_transaction_rollback(struct ref_transaction *transaction)\n+{\n+\tref_transaction_free(transaction);\n+}\n+\n+static struct ref_update *add_update(struct ref_transaction *transaction,\n+\t\t\t\t const char *refname)\n+{\n+\tstruct ref_update *update = xcalloc(1, sizeof(*update));\n+\n+\tupdate->ref_name = xstrdup(refname);\n+\tALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);\n+\ttransaction->updates[transaction->nr++] = update;\n+\treturn update;\n+}\n+\n+void ref_transaction_update(struct ref_transaction *transaction,\n+\t\t\t const char *refname,\n+\t\t\t unsigned char *new_sha1, unsigned char *old_sha1,\n+\t\t\t int flags, int have_old)\n+{\n+\tstruct ref_update *update = add_update(transaction, refname);\n+\n+\thashcpy(update->new_sha1, new_sha1);\n+\tupdate->flags = flags;\n+\tupdate->have_old = have_old;\n+\tif (have_old)\n+\t\thashcpy(update->old_sha1, old_sha1);\n+}\n+\n+void ref_transaction_create(struct ref_transaction *transaction,\n+\t\t\t const char *refname,\n+\t\t\t unsigned char *new_sha1,\n+\t\t\t int flags)\n+{\n+\tstruct ref_update *update = add_update(transaction, refname);\n+\n+\thashcpy(update->new_sha1, new_sha1);\n+\thashclr(update->old_sha1);\n+\tupdate->flags = flags;\n+\tupdate->have_old = 1;\n+}\n+\n+void ref_transaction_delete(struct ref_transaction *transaction,\n+\t\t\t const char *refname,\n+\t\t\t unsigned char *old_sha1,\n+\t\t\t int flags, int have_old)\n+{\n+\tstruct ref_update *update = add_update(transaction, refname);\n+\n+\tupdate->flags = flags;\n+\tupdate->have_old = have_old;\n+\tif (have_old)\n+\t\thashcpy(update->old_sha1, old_sha1);\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@@ -3378,6 +3465,15 @@ cleanup:\n \treturn ret;\n }\n \n+int ref_transaction_commit(struct ref_transaction *transaction,\n+\t\t\t const char *msg, enum action_on_err onerr)\n+{\n+\tint ret = update_refs(msg, transaction->updates, transaction->nr,\n+\t\t\t onerr);\n+\tref_transaction_free(transaction);\n+\treturn ret;\n+}\n+\n char *shorten_unambiguous_ref(const char *refname, int strict)\n {\n \tint i;\ndiff --git a/refs.h b/refs.h\nindex 08e60ac..476a923 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -24,6 +24,8 @@ struct ref_update {\n \tint have_old; /* 1 if old_sha1 is valid, 0 otherwise */\n };\n \n+struct ref_transaction;\n+\n /*\n * Bit values set in the flags argument passed to each_ref_fn():\n */\n@@ -220,6 +222,69 @@ enum action_on_err {\n \tUPDATE_REFS_QUIET_ON_ERR\n };\n \n+/*\n+ * Begin a reference transaction. The reference transaction must\n+ * eventually be commited using ref_transaction_commit() or rolled\n+ * back using ref_transaction_rollback().\n+ */\n+struct ref_transaction *ref_transaction_begin(void);\n+\n+/*\n+ * Roll back a ref_transaction and free all associated data.\n+ */\n+void ref_transaction_rollback(struct ref_transaction *transaction);\n+\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, so the caller retains ownership of the parameter. flags\n+ * can be REF_NODEREF; it is passed to update_ref_lock().\n+ */\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 zeros 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+ * it must not have existed beforehand.\n+ */\n+void ref_transaction_update(struct ref_transaction *transaction,\n+\t\t\t const char *refname,\n+\t\t\t unsigned char *new_sha1, unsigned char *old_sha1,\n+\t\t\t int flags, int have_old);\n+\n+/*\n+ * Add a reference creation to transaction. new_sha1 is the value\n+ * that the reference should have after the update, or zeros if it\n+ * should be deleted. It is verified that the reference does not\n+ * exist already.\n+ */\n+void ref_transaction_create(struct ref_transaction *transaction,\n+\t\t\t const char *refname,\n+\t\t\t unsigned char *new_sha1,\n+\t\t\t int flags);\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+ * the update.\n+ */\n+void ref_transaction_delete(struct ref_transaction *transaction,\n+\t\t\t const char *refname,\n+\t\t\t unsigned char *old_sha1,\n+\t\t\t int flags, int have_old);\n+\n+/*\n+ * Commit all of the changes that have been queued in transaction, as\n+ * atomically as possible. Return a nonzero value if there is a\n+ * problem. The ref_transaction is freed by this function.\n+ */\n+int ref_transaction_commit(struct ref_transaction *transaction,\n+\t\t\t const char *msg, enum action_on_err onerr);\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-- \n1.9.0\n"},{"id":"237472","messageId":"1395683820-17304-21-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:53Z","receivedAt":"2014-03-24T17:56:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This change is mostly clerical: the parse_cmd_*() functions need to\nuse local variables rather than a struct ref_update to collect the\narguments needed for each update, and then call ref_transaction_*() to\nqueue the change rather than building up the list of changes at the\ncaller side.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/update-ref.c | 142 +++++++++++++++++++++++++++------------------------\n 1 file changed, 75 insertions(+), 67 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex bbc04b2..2c8678b 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -12,29 +12,11 @@ static const char * const git_update_ref_usage[] = {\n \tNULL\n };\n \n-static int updates_alloc;\n-static int updates_count;\n-static struct ref_update **updates;\n+static struct ref_transaction *transaction;\n \n static char line_termination = '\\n';\n static int update_flags;\n \n-static struct ref_update *update_alloc(void)\n-{\n-\tstruct ref_update *update;\n-\n-\t/* Allocate and zero-init a struct ref_update */\n-\tupdate = xcalloc(1, sizeof(*update));\n-\tALLOC_GROW(updates, updates_count + 1, updates_alloc);\n-\tupdates[updates_count++] = update;\n-\n-\t/* Store and reset accumulated options */\n-\tupdate->flags = update_flags;\n-\tupdate_flags = 0;\n-\n-\treturn update;\n-}\n-\n /*\n * Parse one whitespace- or NUL-terminated, possibly C-quoted argument\n * and append the result to arg. Return a pointer to the terminator.\n@@ -196,97 +178,119 @@ static int parse_next_sha1(struct strbuf *input, const char **next,\n \n static const char *parse_cmd_update(struct strbuf *input, const char *next)\n {\n-\tstruct ref_update *update;\n-\n-\tupdate = update_alloc();\n+\tchar *refname;\n+\tunsigned char new_sha1[20];\n+\tunsigned char old_sha1[20];\n+\tint have_old;\n \n-\tupdate->ref_name = parse_refname(input, &next);\n-\tif (!update->ref_name)\n+\trefname = parse_refname(input, &next);\n+\tif (!refname)\n \t\tdie(\"update: missing \");\n \n-\tif (parse_next_sha1(input, &next, update->new_sha1,\n-\t\t\t \"update\", update->ref_name,\n+\tif (parse_next_sha1(input, &next, new_sha1, \"update\", refname,\n \t\t\t PARSE_SHA1_ALLOW_EMPTY))\n-\t\tdie(\"update %s: missing \", update->ref_name);\n+\t\tdie(\"update %s: missing \", refname);\n \n-\tupdate->have_old = !parse_next_sha1(input, &next, update->old_sha1,\n-\t\t\t\t\t \"update\", update->ref_name,\n-\t\t\t\t\t PARSE_SHA1_OLD);\n+\thave_old = !parse_next_sha1(input, &next, old_sha1, \"update\", refname,\n+\t\t\t\t PARSE_SHA1_OLD);\n \n \tif (*next != line_termination)\n-\t\tdie(\"update %s: extra input: %s\", update->ref_name, next);\n+\t\tdie(\"update %s: extra input: %s\", refname, next);\n+\n+\tref_transaction_update(transaction, refname, new_sha1, old_sha1,\n+\t\t\t update_flags, have_old);\n+\n+\tupdate_flags = 0;\n+\tfree(refname);\n \n \treturn next;\n }\n \n static const char *parse_cmd_create(struct strbuf *input, const char *next)\n {\n-\tstruct ref_update *update;\n-\n-\tupdate = update_alloc();\n+\tchar *refname;\n+\tunsigned char new_sha1[20];\n \n-\tupdate->ref_name = parse_refname(input, &next);\n-\tif (!update->ref_name)\n+\trefname = parse_refname(input, &next);\n+\tif (!refname)\n \t\tdie(\"create: missing \");\n \n-\tif (parse_next_sha1(input, &next, update->new_sha1,\n-\t\t\t \"create\", update->ref_name, 0))\n-\t\tdie(\"create %s: missing \", update->ref_name);\n+\tif (parse_next_sha1(input, &next, new_sha1, \"create\", refname, 0))\n+\t\tdie(\"create %s: missing \", refname);\n \n-\tif (is_null_sha1(update->new_sha1))\n-\t\tdie(\"create %s: zero \", update->ref_name);\n+\tif (is_null_sha1(new_sha1))\n+\t\tdie(\"create %s: zero \", refname);\n \n \tif (*next != line_termination)\n-\t\tdie(\"create %s: extra input: %s\", update->ref_name, next);\n+\t\tdie(\"create %s: extra input: %s\", refname, next);\n+\n+\tref_transaction_create(transaction, refname, new_sha1, update_flags);\n+\n+\tupdate_flags = 0;\n+\tfree(refname);\n \n \treturn next;\n }\n \n static const char *parse_cmd_delete(struct strbuf *input, const char *next)\n {\n-\tstruct ref_update *update;\n+\tchar *refname;\n+\tunsigned char old_sha1[20];\n+\tint have_old;\n \n-\tupdate = update_alloc();\n-\n-\tupdate->ref_name = parse_refname(input, &next);\n-\tif (!update->ref_name)\n+\trefname = parse_refname(input, &next);\n+\tif (!refname)\n \t\tdie(\"delete: missing \");\n \n-\tif (parse_next_sha1(input, &next, update->old_sha1,\n-\t\t\t \"delete\", update->ref_name, PARSE_SHA1_OLD)) {\n-\t\tupdate->have_old = 0;\n+\tif (parse_next_sha1(input, &next, old_sha1, \"delete\", refname,\n+\t\t\t PARSE_SHA1_OLD)) {\n+\t\thave_old = 0;\n \t} else {\n-\t\tif (is_null_sha1(update->old_sha1))\n-\t\t\tdie(\"delete %s: zero \", update->ref_name);\n-\t\tupdate->have_old = 1;\n+\t\tif (is_null_sha1(old_sha1))\n+\t\t\tdie(\"delete %s: zero \", refname);\n+\t\thave_old = 1;\n \t}\n \n \tif (*next != line_termination)\n-\t\tdie(\"delete %s: extra input: %s\", update->ref_name, next);\n+\t\tdie(\"delete %s: extra input: %s\", refname, next);\n+\n+\tref_transaction_delete(transaction, refname, old_sha1,\n+\t\t\t update_flags, have_old);\n+\n+\tupdate_flags = 0;\n+\tfree(refname);\n \n \treturn next;\n }\n \n static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n {\n-\tstruct ref_update *update;\n-\n-\tupdate = update_alloc();\n+\tchar *refname;\n+\tunsigned char new_sha1[20];\n+\tunsigned char old_sha1[20];\n+\tint have_old;\n \n-\tupdate->ref_name = parse_refname(input, &next);\n-\tif (!update->ref_name)\n+\trefname = parse_refname(input, &next);\n+\tif (!refname)\n \t\tdie(\"verify: missing \");\n \n-\tif (parse_next_sha1(input, &next, update->old_sha1,\n-\t\t\t \"verify\", update->ref_name, PARSE_SHA1_OLD)) {\n-\t\tupdate->have_old = 0;\n+\tif (parse_next_sha1(input, &next, old_sha1, \"verify\", refname,\n+\t\t\t PARSE_SHA1_OLD)) {\n+\t\thashclr(new_sha1);\n+\t\thave_old = 0;\n \t} else {\n-\t\thashcpy(update->new_sha1, update->old_sha1);\n-\t\tupdate->have_old = 1;\n+\t\thashcpy(new_sha1, old_sha1);\n+\t\thave_old = 1;\n \t}\n \n \tif (*next != line_termination)\n-\t\tdie(\"verify %s: extra input: %s\", update->ref_name, next);\n+\t\tdie(\"verify %s: extra input: %s\", refname, next);\n+\n+\tref_transaction_update(transaction, refname, new_sha1, old_sha1,\n+\t\t\t update_flags, have_old);\n+\n+\tupdate_flags = 0;\n+\tfree(refname);\n \n \treturn next;\n }\n@@ -355,13 +359,17 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t\tdie(\"Refusing to perform update with empty message.\");\n \n \tif (read_stdin) {\n+\t\tint ret;\n+\t\ttransaction = ref_transaction_begin();\n+\n \t\tif (delete || no_deref || argc > 0)\n \t\t\tusage_with_options(git_update_ref_usage, options);\n \t\tif (end_null)\n \t\t\tline_termination = '\\0';\n \t\tupdate_refs_stdin();\n-\t\treturn update_refs(msg, updates, updates_count,\n-\t\t\t\t UPDATE_REFS_DIE_ON_ERR);\n+\t\tret = ref_transaction_commit(transaction, msg,\n+\t\t\t\t\t UPDATE_REFS_DIE_ON_ERR);\n+\t\treturn ret;\n \t}\n \n \tif (end_null)\n-- \n1.9.0\n"},{"id":"237469","messageId":"1395683820-17304-22-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 21/27] refs: Remove API function update_refs()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:54Z","receivedAt":"2014-03-24T17:56:54Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It has been superseded by reference transactions. This also means\nthat struct ref_update can become private.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 33 ++++++++++++++++++++-------------\n refs.h | 20 --------------------\n 2 files changed, 20 insertions(+), 33 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex e788c27..dfff117 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3267,6 +3267,20 @@ static int update_ref_write(const char *action, const char *refname,\n \treturn 0;\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+ */\n+struct ref_update {\n+\tconst char *ref_name;\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+};\n+\n /*\n * Data structure for holding a reference transaction, which can\n * consist of checks and updates to multiple references, carried out\n@@ -3393,16 +3407,17 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n \treturn 0;\n }\n \n-int update_refs(const char *action, struct ref_update * const *updates_orig,\n-\t\tint n, enum action_on_err onerr)\n+int ref_transaction_commit(struct ref_transaction *transaction,\n+\t\t\t const char *msg, enum action_on_err onerr)\n {\n \tint ret = 0, delnum = 0, i;\n \tstruct ref_update **updates;\n \tint *types;\n \tstruct ref_lock **locks;\n \tconst char **delnames;\n+\tint n = transaction->nr;\n \n-\tif (!updates_orig || !n)\n+\tif (!n)\n \t\treturn 0;\n \n \t/* Allocate work space */\n@@ -3412,7 +3427,7 @@ int update_refs(const char *action, struct ref_update * const *updates_orig,\n \tdelnames = xmalloc(sizeof(*delnames) * n);\n \n \t/* Copy, sort, and reject duplicate refs */\n-\tmemcpy(updates, updates_orig, sizeof(*updates) * n);\n+\tmemcpy(updates, transaction->updates, sizeof(*updates) * n);\n \tqsort(updates, n, sizeof(*updates), ref_update_compare);\n \tret = ref_update_reject_duplicates(updates, n, onerr);\n \tif (ret)\n@@ -3434,7 +3449,7 @@ int update_refs(const char *action, struct ref_update * const *updates_orig,\n \t/* Perform updates first so live commits remain referenced */\n \tfor (i = 0; i < n; i++)\n \t\tif (!is_null_sha1(updates[i]->new_sha1)) {\n-\t\t\tret = update_ref_write(action,\n+\t\t\tret = update_ref_write(msg,\n \t\t\t\t\t updates[i]->ref_name,\n \t\t\t\t\t updates[i]->new_sha1,\n \t\t\t\t\t locks[i], onerr);\n@@ -3462,14 +3477,6 @@ cleanup:\n \tfree(types);\n \tfree(locks);\n \tfree(delnames);\n-\treturn ret;\n-}\n-\n-int ref_transaction_commit(struct ref_transaction *transaction,\n-\t\t\t const char *msg, enum action_on_err onerr)\n-{\n-\tint ret = update_refs(msg, transaction->updates, transaction->nr,\n-\t\t\t onerr);\n \tref_transaction_free(transaction);\n \treturn ret;\n }\ndiff --git a/refs.h b/refs.h\nindex 476a923..99c194b 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -10,20 +10,6 @@ struct ref_lock {\n \tint force_write;\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- */\n-struct ref_update {\n-\tconst char *ref_name;\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-};\n-\n struct ref_transaction;\n \n /*\n@@ -290,12 +276,6 @@ 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-/**\n- * Lock all refs and then perform all modifications.\n- */\n-int update_refs(const char *action, struct ref_update * const *updates,\n-\t\tint n, 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 \n-- \n1.9.0\n"},{"id":"237464","messageId":"1395683820-17304-23-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 22/27] struct ref_update: Rename field \"ref_name\" to \"refname\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:55Z","receivedAt":"2014-03-24T17:56:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is consistent with the usual nomenclature.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 18 +++++++++---------\n refs.h | 2 +-\n 2 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex dfff117..d72d0ab 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3274,7 +3274,7 @@ static int update_ref_write(const char *action, const char *refname,\n * value or to zero to ensure the ref does not exist before update.\n */\n struct ref_update {\n-\tconst char *ref_name;\n+\tconst char *refname;\n \tunsigned char new_sha1[20];\n \tunsigned char old_sha1[20];\n \tint flags; /* REF_NODEREF? */\n@@ -3304,7 +3304,7 @@ static void ref_transaction_free(struct ref_transaction *transaction)\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n \n-\t\tfree((char *)update->ref_name);\n+\t\tfree((char *)update->refname);\n \t\tfree(update);\n \t}\n \n@@ -3322,7 +3322,7 @@ static struct ref_update *add_update(struct ref_transaction *transaction,\n {\n \tstruct ref_update *update = xcalloc(1, sizeof(*update));\n \n-\tupdate->ref_name = xstrdup(refname);\n+\tupdate->refname = xstrdup(refname);\n \tALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);\n \ttransaction->updates[transaction->nr++] = update;\n \treturn update;\n@@ -3383,7 +3383,7 @@ static int ref_update_compare(const void *r1, const void *r2)\n {\n \tconst struct ref_update * const *u1 = r1;\n \tconst struct ref_update * const *u2 = r2;\n-\treturn strcmp((*u1)->ref_name, (*u2)->ref_name);\n+\treturn strcmp((*u1)->refname, (*u2)->refname);\n }\n \n static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n@@ -3391,14 +3391,14 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n {\n \tint i;\n \tfor (i = 1; i < n; i++)\n-\t\tif (!strcmp(updates[i - 1]->ref_name, updates[i]->ref_name)) {\n+\t\tif (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {\n \t\t\tconst char *str =\n \t\t\t\t\"Multiple updates for ref '%s' not allowed.\";\n \t\t\tswitch (onerr) {\n \t\t\tcase UPDATE_REFS_MSG_ON_ERR:\n-\t\t\t\terror(str, updates[i]->ref_name); break;\n+\t\t\t\terror(str, updates[i]->refname); break;\n \t\t\tcase UPDATE_REFS_DIE_ON_ERR:\n-\t\t\t\tdie(str, updates[i]->ref_name); break;\n+\t\t\t\tdie(str, updates[i]->refname); break;\n \t\t\tcase UPDATE_REFS_QUIET_ON_ERR:\n \t\t\t\tbreak;\n \t\t\t}\n@@ -3435,7 +3435,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t/* Acquire all locks while verifying old values */\n \tfor (i = 0; i < n; i++) {\n-\t\tlocks[i] = update_ref_lock(updates[i]->ref_name,\n+\t\tlocks[i] = update_ref_lock(updates[i]->refname,\n \t\t\t\t\t (updates[i]->have_old ?\n \t\t\t\t\t updates[i]->old_sha1 : NULL),\n \t\t\t\t\t updates[i]->flags,\n@@ -3450,7 +3450,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tfor (i = 0; i < n; i++)\n \t\tif (!is_null_sha1(updates[i]->new_sha1)) {\n \t\t\tret = update_ref_write(msg,\n-\t\t\t\t\t updates[i]->ref_name,\n+\t\t\t\t\t updates[i]->refname,\n \t\t\t\t\t updates[i]->new_sha1,\n \t\t\t\t\t locks[i], onerr);\n \t\t\tlocks[i] = NULL; /* freed by update_ref_write */\ndiff --git a/refs.h b/refs.h\nindex 99c194b..30ee721 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -154,7 +154,7 @@ extern void unlock_ref(struct ref_lock *lock);\n extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);\n \n /** Setup reflog before using. **/\n-int log_ref_setup(const char *ref_name, char *logfile, int bufsize);\n+int log_ref_setup(const char *refname, char *logfile, int bufsize);\n \n /** Reads log for the value of ref during at_time. **/\n extern int read_ref_at(const char *refname, unsigned long at_time, int cnt,\n-- \n1.9.0\n"},{"id":"237477","messageId":"1395683820-17304-24-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 23/27] struct ref_update: Store refname as a FLEX_ARRAY.","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:56Z","receivedAt":"2014-03-24T17:56:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty \n---\n refs.c | 15 ++++++---------\n 1 file changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex d72d0ab..2b80f6d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3274,11 +3274,11 @@ static int update_ref_write(const char *action, const char *refname,\n * value or to zero to ensure the ref does not exist before update.\n */\n struct ref_update {\n-\tconst char *refname;\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+\tconst char refname[FLEX_ARRAY];\n };\n \n /*\n@@ -3301,12 +3301,8 @@ static void ref_transaction_free(struct ref_transaction *transaction)\n {\n \tint i;\n \n-\tfor (i = 0; i < transaction->nr; i++) {\n-\t\tstruct ref_update *update = transaction->updates[i];\n-\n-\t\tfree((char *)update->refname);\n-\t\tfree(update);\n-\t}\n+\tfor (i = 0; i < transaction->nr; i++)\n+\t\tfree(transaction->updates[i]);\n \n \tfree(transaction->updates);\n \tfree(transaction);\n@@ -3320,9 +3316,10 @@ void ref_transaction_rollback(struct ref_transaction *transaction)\n static struct ref_update *add_update(struct ref_transaction *transaction,\n \t\t\t\t const char *refname)\n {\n-\tstruct ref_update *update = xcalloc(1, sizeof(*update));\n+\tsize_t len = strlen(refname);\n+\tstruct ref_update *update = xcalloc(1, sizeof(*update) + len + 1);\n \n-\tupdate->refname = xstrdup(refname);\n+\tstrcpy((char *)update->refname, refname);\n \tALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);\n \ttransaction->updates[transaction->nr++] = update;\n \treturn update;\n-- \n1.9.0\n"},{"id":"237467","messageId":"1395683820-17304-25-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 24/27] ref_transaction_commit(): Introduce temporary variables","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:57Z","receivedAt":"2014-03-24T17:56:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Use temporary variables in the for-loop blocks to simplify expressions\nin the rest of the loop.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 21 +++++++++++++--------\n 1 file changed, 13 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 2b80f6d..d51566c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3432,10 +3432,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t/* Acquire all locks while verifying old values */\n \tfor (i = 0; i < n; i++) {\n-\t\tlocks[i] = update_ref_lock(updates[i]->refname,\n-\t\t\t\t\t (updates[i]->have_old ?\n-\t\t\t\t\t updates[i]->old_sha1 : NULL),\n-\t\t\t\t\t updates[i]->flags,\n+\t\tstruct ref_update *update = updates[i];\n+\n+\t\tlocks[i] = update_ref_lock(update->refname,\n+\t\t\t\t\t (update->have_old ?\n+\t\t\t\t\t update->old_sha1 : NULL),\n+\t\t\t\t\t update->flags,\n \t\t\t\t\t &types[i], onerr);\n \t\tif (!locks[i]) {\n \t\t\tret = 1;\n@@ -3444,16 +3446,19 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t}\n \n \t/* Perform updates first so live commits remain referenced */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (!is_null_sha1(updates[i]->new_sha1)) {\n+\tfor (i = 0; i < n; i++) {\n+\t\tstruct ref_update *update = updates[i];\n+\n+\t\tif (!is_null_sha1(update->new_sha1)) {\n \t\t\tret = update_ref_write(msg,\n-\t\t\t\t\t updates[i]->refname,\n-\t\t\t\t\t updates[i]->new_sha1,\n+\t\t\t\t\t update->refname,\n+\t\t\t\t\t update->new_sha1,\n \t\t\t\t\t locks[i], onerr);\n \t\t\tlocks[i] = NULL; /* freed by update_ref_write */\n \t\t\tif (ret)\n \t\t\t\tgoto cleanup;\n \t\t}\n+\t}\n \n \t/* Perform deletes now that updates are safely completed */\n \tfor (i = 0; i < n; i++)\n-- \n1.9.0\n"},{"id":"237471","messageId":"1395683820-17304-26-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 25/27] struct ref_update: Add a lock member","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:58Z","receivedAt":"2014-03-24T17:56:58Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Now that we manage ref_update objects internally, we can use them to\nhold some of the scratch space we need when actually carrying out the\nupdates. Store the (struct ref_lock *) there.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 36 +++++++++++++++++++-----------------\n 1 file changed, 19 insertions(+), 17 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex d51566c..d1edd57 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3278,6 +3278,7 @@ struct ref_update {\n \tunsigned char old_sha1[20];\n \tint flags; /* REF_NODEREF? */\n \tint have_old; /* 1 if old_sha1 is valid, 0 otherwise */\n+\tstruct ref_lock *lock;\n \tconst char refname[FLEX_ARRAY];\n };\n \n@@ -3410,7 +3411,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tint ret = 0, delnum = 0, i;\n \tstruct ref_update **updates;\n \tint *types;\n-\tstruct ref_lock **locks;\n \tconst char **delnames;\n \tint n = transaction->nr;\n \n@@ -3420,7 +3420,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t/* Allocate work space */\n \tupdates = xmalloc(sizeof(*updates) * n);\n \ttypes = xmalloc(sizeof(*types) * n);\n-\tlocks = xcalloc(n, sizeof(*locks));\n \tdelnames = xmalloc(sizeof(*delnames) * n);\n \n \t/* Copy, sort, and reject duplicate refs */\n@@ -3434,12 +3433,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \tfor (i = 0; i < n; i++) {\n \t\tstruct ref_update *update = updates[i];\n \n-\t\tlocks[i] = update_ref_lock(update->refname,\n-\t\t\t\t\t (update->have_old ?\n-\t\t\t\t\t update->old_sha1 : NULL),\n-\t\t\t\t\t update->flags,\n-\t\t\t\t\t &types[i], onerr);\n-\t\tif (!locks[i]) {\n+\t\tupdate->lock = update_ref_lock(update->refname,\n+\t\t\t\t\t (update->have_old ?\n+\t\t\t\t\t\tupdate->old_sha1 : NULL),\n+\t\t\t\t\t update->flags,\n+\t\t\t\t\t &types[i], onerr);\n+\t\tif (!update->lock) {\n \t\t\tret = 1;\n \t\t\tgoto cleanup;\n \t\t}\n@@ -3453,19 +3452,23 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\tret = update_ref_write(msg,\n \t\t\t\t\t update->refname,\n \t\t\t\t\t update->new_sha1,\n-\t\t\t\t\t locks[i], onerr);\n-\t\t\tlocks[i] = NULL; /* freed by update_ref_write */\n+\t\t\t\t\t update->lock, onerr);\n+\t\t\tupdate->lock = NULL; /* freed by update_ref_write */\n \t\t\tif (ret)\n \t\t\t\tgoto cleanup;\n \t\t}\n \t}\n \n \t/* Perform deletes now that updates are safely completed */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (locks[i]) {\n-\t\t\tdelnames[delnum++] = locks[i]->ref_name;\n-\t\t\tret |= delete_ref_loose(locks[i], types[i]);\n+\tfor (i = 0; i < n; i++) {\n+\t\tstruct ref_update *update = updates[i];\n+\n+\t\tif (update->lock) {\n+\t\t\tdelnames[delnum++] = update->lock->ref_name;\n+\t\t\tret |= delete_ref_loose(update->lock, types[i]);\n \t\t}\n+\t}\n+\n \tret |= repack_without_refs(delnames, delnum);\n \tfor (i = 0; i < delnum; i++)\n \t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n@@ -3473,11 +3476,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n cleanup:\n \tfor (i = 0; i < n; i++)\n-\t\tif (locks[i])\n-\t\t\tunlock_ref(locks[i]);\n+\t\tif (updates[i]->lock)\n+\t\t\tunlock_ref(updates[i]->lock);\n \tfree(updates);\n \tfree(types);\n-\tfree(locks);\n \tfree(delnames);\n \tref_transaction_free(transaction);\n \treturn ret;\n-- \n1.9.0\n"},{"id":"237468","messageId":"1395683820-17304-27-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 26/27] struct ref_update: Add type field","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:56:59Z","receivedAt":"2014-03-24T17:56:59Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is temporary space for ref_transaction_commit().\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex d1edd57..07f900a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3279,6 +3279,7 @@ struct ref_update {\n \tint flags; /* REF_NODEREF? */\n \tint have_old; /* 1 if old_sha1 is valid, 0 otherwise */\n \tstruct ref_lock *lock;\n+\tint type;\n \tconst char refname[FLEX_ARRAY];\n };\n \n@@ -3410,7 +3411,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n {\n \tint ret = 0, delnum = 0, i;\n \tstruct ref_update **updates;\n-\tint *types;\n \tconst char **delnames;\n \tint n = transaction->nr;\n \n@@ -3419,7 +3419,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t/* Allocate work space */\n \tupdates = xmalloc(sizeof(*updates) * n);\n-\ttypes = xmalloc(sizeof(*types) * n);\n \tdelnames = xmalloc(sizeof(*delnames) * n);\n \n \t/* Copy, sort, and reject duplicate refs */\n@@ -3437,7 +3436,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t\t (update->have_old ?\n \t\t\t\t\t\tupdate->old_sha1 : NULL),\n \t\t\t\t\t update->flags,\n-\t\t\t\t\t &types[i], onerr);\n+\t\t\t\t\t &update->type, onerr);\n \t\tif (!update->lock) {\n \t\t\tret = 1;\n \t\t\tgoto cleanup;\n@@ -3465,7 +3464,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \n \t\tif (update->lock) {\n \t\t\tdelnames[delnum++] = update->lock->ref_name;\n-\t\t\tret |= delete_ref_loose(update->lock, types[i]);\n+\t\t\tret |= delete_ref_loose(update->lock, update->type);\n \t\t}\n \t}\n \n@@ -3479,7 +3478,6 @@ cleanup:\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n \tfree(updates);\n-\tfree(types);\n \tfree(delnames);\n \tref_transaction_free(transaction);\n \treturn ret;\n-- \n1.9.0\n"},{"id":"237473","messageId":"1395683820-17304-28-git-send-email-mhagger@alum.mit.edu","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 27/27] ref_transaction_commit(): Work with transaction->updates in place","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-24T17:57:00Z","receivedAt":"2014-03-24T17:57:00Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Now that we free the transaction when we are done, there is no need to\nmake a copy of transaction->updates before working with it.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 07f900a..aaf75f6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3410,19 +3410,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t const char *msg, enum action_on_err onerr)\n {\n \tint ret = 0, delnum = 0, i;\n-\tstruct ref_update **updates;\n \tconst char **delnames;\n \tint n = transaction->nr;\n+\tstruct ref_update **updates = transaction->updates;\n \n \tif (!n)\n \t\treturn 0;\n \n \t/* Allocate work space */\n-\tupdates = xmalloc(sizeof(*updates) * n);\n \tdelnames = xmalloc(sizeof(*delnames) * n);\n \n \t/* Copy, sort, and reject duplicate refs */\n-\tmemcpy(updates, transaction->updates, sizeof(*updates) * n);\n \tqsort(updates, n, sizeof(*updates), ref_update_compare);\n \tret = ref_update_reject_duplicates(updates, n, onerr);\n \tif (ret)\n@@ -3477,7 +3475,6 @@ cleanup:\n \tfor (i = 0; i < n; i++)\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n-\tfree(updates);\n \tfree(delnames);\n \tref_transaction_free(transaction);\n \treturn ret;\n-- \n1.9.0\n"},{"id":"237825","messageId":"53331ECB.5040901@kitware.com","threadId":"36283","inReplyTo":"1395683820-17304-15-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 14/27] update-ref.c: Extract a new function, parse_next_sha1()","fromName":"Brad King","fromEmail":"brad.king@kitware.com","sentAt":"2014-03-26T18:39:07Z","receivedAt":"2014-03-26T18:39:07Z","isPatch":true,"sender":{"key":"brad.king@kitware.com","avatar":"https://avatars.githubusercontent.com/u/87268?v=4"},"body":"On 03/24/2014 01:56 PM, Michael Haggerty wrote:\n> +/*\n> + * For backwards compatibility, accept an empty string for create's\n> + * in binary mode to be equivalent to specifying zeros.\n> + */\n> +#define PARSE_SHA1_ALLOW_EMPTY 0x02\n\nThe comment should say \"update's\", not \"create's\".\n\n-Brad\n"},{"id":"237826","messageId":"53331ED1.7060804@kitware.com","threadId":"36283","inReplyTo":"1395683820-17304-17-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 16/27] t1400: Test one mistake at a time","fromName":"Brad King","fromEmail":"brad.king@kitware.com","sentAt":"2014-03-26T18:39:13Z","receivedAt":"2014-03-26T18:39:13Z","isPatch":true,"sender":{"key":"brad.king@kitware.com","avatar":"https://avatars.githubusercontent.com/u/87268?v=4"},"body":"On 03/24/2014 01:56 PM, Michael Haggerty wrote:\n> Signed-off-by: Michael Haggerty \n> \n> t1400: Add a test of \"update\" with too few arguments\n> \n> Signed-off-by: Michael Haggerty \n\nThis looks like a stray squash message.\n\n-Brad\n"},{"id":"237828","messageId":"53331ED7.9020004@kitware.com","threadId":"36283","inReplyTo":"1395683820-17304-20-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 19/27] refs: Add a concept of a reference transaction","fromName":"Brad King","fromEmail":"brad.king@kitware.com","sentAt":"2014-03-26T18:39:19Z","receivedAt":"2014-03-26T18:39:19Z","isPatch":true,"sender":{"key":"brad.king@kitware.com","avatar":"https://avatars.githubusercontent.com/u/87268?v=4"},"body":"On 03/24/2014 01:56 PM, Michael Haggerty wrote:\n> +void ref_transaction_update(struct ref_transaction *transaction,\n> +\t\t\t const char *refname,\n> +\t\t\t unsigned char *new_sha1, unsigned char *old_sha1,\n> +\t\t\t int flags, int have_old);\n[snip]\n> +void ref_transaction_create(struct ref_transaction *transaction,\n> +\t\t\t const char *refname,\n> +\t\t\t unsigned char *new_sha1,\n> +\t\t\t int flags);\n[snip]\n> +void ref_transaction_delete(struct ref_transaction *transaction,\n> +\t\t\t const char *refname,\n> +\t\t\t unsigned char *old_sha1,\n> +\t\t\t int flags, int have_old);\n\nPerhaps we also need:\n\nvoid ref_transaction_verify(struct ref_transaction *transaction,\n\t\t\t const char *refname,\n\t\t\t unsigned char *old_sha1,\n\t\t\t int flags, int have_old);\n\nas equivalent to the \"verify\" command in \"update-ref --stdin\"?\n\n-Brad\n"},{"id":"237827","messageId":"53331EE6.2010100@kitware.com","threadId":"36283","inReplyTo":"1395683820-17304-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 00/27] Clean up update-refs --stdin and implement ref_transaction","fromName":"Brad King","fromEmail":"brad.king@kitware.com","sentAt":"2014-03-26T18:39:34Z","receivedAt":"2014-03-26T18:39:34Z","isPatch":true,"sender":{"key":"brad.king@kitware.com","avatar":"https://avatars.githubusercontent.com/u/87268?v=4"},"body":"On 03/24/2014 01:56 PM, Michael Haggerty wrote:\n> Changes relative to v1:\n> \n> * Rename the functions associated with ref_transactions to be more\n> reminiscent of database transactions:\n> \n> * create_ref_transaction() -> ref_transaction_begin()\n> * free_ref_transaction() -> ref_transaction_rollback()\n> * queue_update_ref() -> ref_transaction_update()\n> * queue_create_ref() -> ref_transaction_create()\n> * queue_delete_ref() -> ref_transaction_delete()\n> * commit_ref_transaction() -> ref_transaction_commit()\n\nThose new names look better.\n\n> * Fix backwards compatibility of \"git update-ref --stdin -z\"'s\n> handling of the \"create\" command: allow to be the empty\n> string, treating it the same zeros. But deprecate this usage.\n\nThe changes related to that look good. The new documentation is\nmuch clearer than my old wording.\n\nSeries v2 looks good to me except for my responses to individual\ncommits.\n\nThanks,\n-Brad\n"},{"id":"237853","messageId":"533349DF.8090004@alum.mit.edu","threadId":"36283","inReplyTo":"53331ED7.9020004@kitware.com","subject":"Re: [PATCH v2 19/27] refs: Add a concept of a reference transaction","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-26T21:42:55Z","receivedAt":"2014-03-26T21:42:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/26/2014 07:39 PM, Brad King wrote:\n> On 03/24/2014 01:56 PM, Michael Haggerty wrote:\n>> +void ref_transaction_update(struct ref_transaction *transaction,\n>> +\t\t\t const char *refname,\n>> +\t\t\t unsigned char *new_sha1, unsigned char *old_sha1,\n>> +\t\t\t int flags, int have_old);\n> [snip]\n>> +void ref_transaction_create(struct ref_transaction *transaction,\n>> +\t\t\t const char *refname,\n>> +\t\t\t unsigned char *new_sha1,\n>> +\t\t\t int flags);\n> [snip]\n>> +void ref_transaction_delete(struct ref_transaction *transaction,\n>> +\t\t\t const char *refname,\n>> +\t\t\t unsigned char *old_sha1,\n>> +\t\t\t int flags, int have_old);\n> \n> Perhaps we also need:\n> \n> void ref_transaction_verify(struct ref_transaction *transaction,\n> \t\t\t const char *refname,\n> \t\t\t unsigned char *old_sha1,\n> \t\t\t int flags, int have_old);\n> \n> as equivalent to the \"verify\" command in \"update-ref --stdin\"?\n\nYes. That's already on my todo list for a future batch of patches. But\nfirst I was going to beef up the ref_update structure to handle verify\nactions directly rather than as updates with oldvalue==newvalue,\nprobably by turning has_old into a flag with HAS_OLD and HAS_NEW bits.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"237854","messageId":"53334ADD.8030806@alum.mit.edu","threadId":"36283","inReplyTo":"53331EE6.2010100@kitware.com","subject":"Re: [PATCH v2 00/27] Clean up update-refs --stdin and implement ref_transaction","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-26T21:47:09Z","receivedAt":"2014-03-26T21:47:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/26/2014 07:39 PM, Brad King wrote:\n> On 03/24/2014 01:56 PM, Michael Haggerty wrote:\n>> Changes relative to v1:\n>>\n>> * Rename the functions associated with ref_transactions to be more\n>> reminiscent of database transactions:\n>>\n>> * create_ref_transaction() -> ref_transaction_begin()\n>> * free_ref_transaction() -> ref_transaction_rollback()\n>> * queue_update_ref() -> ref_transaction_update()\n>> * queue_create_ref() -> ref_transaction_create()\n>> * queue_delete_ref() -> ref_transaction_delete()\n>> * commit_ref_transaction() -> ref_transaction_commit()\n> \n> Those new names look better.\n> \n>> * Fix backwards compatibility of \"git update-ref --stdin -z\"'s\n>> handling of the \"create\" command: allow to be the empty\n>> string, treating it the same zeros. But deprecate this usage.\n> \n> The changes related to that look good. The new documentation is\n> much clearer than my old wording.\n> \n> Series v2 looks good to me except for my responses to individual\n> commits.\n\nThanks a lot for the review. Your other two comments are correct, of\ncourse, and I will fix them if there needs to be a re-roll.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238151","messageId":"xmqqtxae3voc.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"1395683820-17304-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 02/27] t1400: Provide more usual input to the command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-31T21:28:03Z","receivedAt":"2014-03-31T21:28:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Subject: Re: [PATCH v2 02/27] t1400: Provide more usual input to the command\n\nThis applies to the patches throughout the series, but during the\nmicroproject reviews, Eric pointed out that we seem to start the\nsummary after area: on the subject with lowercase and omit the full\nstop at the end, so I'll try to tweak the subjects while queueing to\nread patches in the series.\n\n> The old version was passing (among other things)\n>\n> update SP refs/heads/c NUL NUL 0{40} NUL\n>\n> to \"git update-ref -z --stdin\" to test whether the old-value check for\n> c is working. But the is empty, which is a bit off the\n> beaten track.\n>\n> So, to be sure that we are testing what we want to test, provide an\n> actual on the \"update\" line.\n\nInteresting. So the test used to expect failure, but we couldn't\ntell if that was due to giving a \"Please update to this value\" which\nis malformed, or \"I am giving 0{40} as the old value, telling you\nthat you have to make sure the ref does not exist\" which does not\nhold because we already have that ref?\n\nThat would mean that the test may not have been testing the right\nthing. A good change.\n\n> Signed-off-by: Michael Haggerty \n> ---\n> t/t1400-update-ref.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index fa927d2..29391c6 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -912,7 +912,7 @@ test_expect_success 'stdin -z update refs works with identity updates' '\n> \n> test_expect_success 'stdin -z update refs fails with wrong old value' '\n> \tgit update-ref $c $m &&\n> -\tprintf $F \"update $a\" \"$m\" \"$m\" \"update $b\" \"$m\" \"$m\" \"update $c\" \"\" \"$Z\" >stdin &&\n> +\tprintf $F \"update $a\" \"$m\" \"$m\" \"update $b\" \"$m\" \"$m\" \"update $c\" \"$m\" \"$Z\" >stdin &&\n> \ttest_must_fail git update-ref -z --stdin err &&\n> \tgrep \"fatal: Cannot lock the ref '\"'\"'$c'\"'\"'\" err &&\n> \tgit rev-parse $m >expect &&\n"},{"id":"238152","messageId":"xmqqppl23vjl.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"1395683820-17304-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 01/27] t1400: Fix name and expected result of one test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-31T21:30:54Z","receivedAt":"2014-03-31T21:30:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> The test\n>\n> stdin -z create ref fails with zero new value\n>\n> actually passes an empty new value, not a zero new value. So rename\n> the test s/zero/empty/, and change the expected error from\n>\n> fatal: create $c given zero new value\n>\n> to\n>\n> fatal: create $c missing \n\nI have a feeling that \"zero new value\" might have been done by a\nnon-native (like me) to say \"no new value\"; \"missing newvalue\"\nsounds like a good phrasing to use.\n\n> Of course, this makes the test fail now, so mark it\n> test_expect_failure. The failure will be fixed later in this patch\n> series.\n\nThat sounds somewhat strange. Why not just give a single-liner to\nupdate-ref.c instead?\n\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> t/t1400-update-ref.sh | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index 6ffd82f..fa927d2 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -827,10 +827,10 @@ test_expect_success 'stdin -z create ref fails with bad new value' '\n> \ttest_must_fail git rev-parse --verify -q $c\n> '\n> \n> -test_expect_success 'stdin -z create ref fails with zero new value' '\n> +test_expect_failure 'stdin -z create ref fails with empty new value' '\n> \tprintf $F \"create $c\" \"\" >stdin &&\n> \ttest_must_fail git update-ref -z --stdin err &&\n> -\tgrep \"fatal: create $c given zero new value\" err &&\n> +\tgrep \"fatal: create $c missing \" err &&\n> \ttest_must_fail git rev-parse --verify -q $c\n> '\n"},{"id":"238153","messageId":"xmqqlhvq3vaj.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"1395683820-17304-4-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 03/27] parse_arg(): Really test that argument is properly terminated","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-31T21:36:20Z","receivedAt":"2014-03-31T21:36:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Test that the argument is properly terminated by either whitespace or\n> a NUL character, even if it is quoted, to be consistent with the\n> non-quoted case. Adjust the tests to expect the new error message.\n> Add a docstring to the function, incorporating the comments that were\n> formerly within the function plus some added information.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> builtin/update-ref.c | 20 +++++++++++++++-----\n> t/t1400-update-ref.sh | 4 ++--\n> 2 files changed, 17 insertions(+), 7 deletions(-)\n>\n> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n> index 1292cfe..02b5f95 100644\n> --- a/builtin/update-ref.c\n> +++ b/builtin/update-ref.c\n> @@ -62,16 +62,26 @@ static void update_store_old_sha1(struct ref_update *update,\n> \tupdate->have_old = *oldvalue || line_termination;\n> }\n> \n> +/*\n> + * Parse one whitespace- or NUL-terminated, possibly C-quoted argument\n> + * and append the result to arg. Return a pointer to the terminator.\n> + * Die if there is an error in how the argument is C-quoted. This\n> + * function is only used if not -z.\n> + */\n> static const char *parse_arg(const char *next, struct strbuf *arg)\n> {\n> -\t/* Parse SP-terminated, possibly C-quoted argument */\n> -\tif (*next != '\"')\n> +\tif (*next == '\"') {\n> +\t\tconst char *orig = next;\n> +\n> +\t\tif (unquote_c_style(arg, next, &next))\n> +\t\t\tdie(\"badly quoted argument: %s\", orig);\n> +\t\tif (*next && !isspace(*next))\n> +\t\t\tdie(\"unexpected character after quoted argument: %s\", orig);\n> +\t} else {\n> \t\twhile (*next && !isspace(*next))\n> \t\t\tstrbuf_addch(arg, *next++);\n> -\telse if (unquote_c_style(arg, next, &next))\n> -\t\tdie(\"badly quoted argument: %s\", next);\n> +\t}\n> \n> -\t/* Return position after the argument */\n> \treturn next;\n> }\n> \n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index 29391c6..774f8c5 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -356,10 +356,10 @@ test_expect_success 'stdin fails on badly quoted input' '\n> \tgrep \"fatal: badly quoted argument: \\\\\\\"master\" err\n> '\n> \n> -test_expect_success 'stdin fails on arguments not separated by space' '\n> +test_expect_success 'stdin fails on junk after quoted argument' '\n> \techo \"create \\\"$a\\\"master\" >stdin &&\n> \ttest_must_fail git update-ref --stdin err &&\n> -\tgrep \"fatal: expected SP but got: master\" err\n> +\tgrep \"fatal: unexpected character after quoted argument: \\\\\\\"$a\\\\\\\"master\" err\n> '\n\nInteresting.\n\nI would have expected that \"We used to check only one of the two\ncodepaths and the other was loose, fix it\" to be accompanied by \"So\nhere is an _addition_ to the test suite to validate the other case\nthat used to be loose, now tightened\", not \"We update one existing\ncase\". The test before and after the patch is about a c-quoted\nstring, so I am not sure if we are still testing the right thing.\n\nThe code in update-ref.c after the patch does look reasonable,\nthough.\n"},{"id":"238155","messageId":"xmqqha6e3v44.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"1395683820-17304-7-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 06/27] update_refs(): Fix constness","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-31T21:40:11Z","receivedAt":"2014-03-31T21:40:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Since full const correctness is beyond the ability of C's type system,\n> just put the const where it doesn't do any harm. A (struct ref_update\n> **) can be passed to a (struct ref_update * const *) argument, but not\n> to a (const struct ref_update **) argument.\n\nSounds good, but next time please try not to break lines inside a\nsingle typename, which is somewhat unreadable ;-)\n\nI'd suggest rewording \"s/Fix/tighten/\". Because a patch that\nchanges constness can loosen constness to make things more correct,\n\"git shortlog\" output that says if it is tightening or loosening\nwould be more informative than the one that says that it is \"fixing\".\n\n> Signed-off-by: Michael Haggerty \n> ---\n> builtin/update-ref.c | 2 +-\n> refs.c | 2 +-\n> refs.h | 2 +-\n> 3 files changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n> index f6345e5..a8a68e8 100644\n> --- a/builtin/update-ref.c\n> +++ b/builtin/update-ref.c\n> @@ -14,7 +14,7 @@ static const char * const git_update_ref_usage[] = {\n> \n> static int updates_alloc;\n> static int updates_count;\n> -static const struct ref_update **updates;\n> +static struct ref_update **updates;\n> \n> static char line_termination = '\\n';\n> static int update_flags;\n> diff --git a/refs.c b/refs.c\n> index 196984e..1305eb1 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3306,7 +3306,7 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n> \treturn 0;\n> }\n> \n> -int update_refs(const char *action, const struct ref_update **updates_orig,\n> +int update_refs(const char *action, struct ref_update * const *updates_orig,\n> \t\tint n, enum action_on_err onerr)\n> {\n> \tint ret = 0, delnum = 0, i;\n> diff --git a/refs.h b/refs.h\n> index a713b34..08e60ac 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -228,7 +228,7 @@ int update_ref(const char *action, const char *refname,\n> /**\n> * Lock all refs and then perform all modifications.\n> */\n> -int update_refs(const char *action, const struct ref_update **updates,\n> +int update_refs(const char *action, struct ref_update * const *updates,\n> \t\tint n, enum action_on_err onerr);\n> \n> extern int parse_hide_refs_config(const char *var, const char *value, const char *);\n"},{"id":"238157","messageId":"xmqqbnwm3uqw.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"1395683820-17304-14-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 13/27] t1400: Test that stdin -z update treats empty as zeros","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-31T21:48:07Z","receivedAt":"2014-03-31T21:48:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> This is the (slightly inconsistent) status quo; make sure it doesn't\n> change by accident.\n\nInteresting. So \"oldvalue\" being empty is \"we do not care what it\nis\" (as opposed to \"we know it must not exist yet\" aka 0{40}), and\n\"newvalue\" being empty is the same as \"delete it\" aka 0{40}.\n\nThat is unfortunate, but I agree it is a good idea to add a test for\nit, so that we will notice when we decide to fix it.\n\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> t/t1400-update-ref.sh | 7 +++++++\n> 1 file changed, 7 insertions(+)\n>\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index a2015d0..208f56e 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -730,6 +730,13 @@ test_expect_success 'stdin -z fails update with bad ref name' '\n> \tgrep \"fatal: invalid ref format: ~a\" err\n> '\n> \n> +test_expect_success 'stdin -z treats empty new value as zeros' '\n> +\tgit update-ref $a $m &&\n> +\tprintf $F \"update $a\" \"\" \"\" >stdin &&\n> +\tgit update-ref -z --stdin +\ttest_must_fail git rev-parse --verify -q $a\n> +'\n> +\n> test_expect_success 'stdin -z fails update with no new value' '\n> \tprintf $F \"update $a\" >stdin &&\n> \ttest_must_fail git update-ref -z --stdin err &&\n"},{"id":"238159","messageId":"xmqq7g7a3uo9.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"1395683820-17304-16-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 15/27] update-ref --stdin -z: Deprecate interpreting the empty string as zeros","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-31T21:49:42Z","receivedAt":"2014-03-31T21:49:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> In the original version of this command, for the single case of the\n> \"update\" command's , the empty string was interpreted as\n> being equivalent to 40 \"0\"s. This shorthand is unnecessary (binary\n> input will usually be generated programmatically anyway), and it\n> complicates the parser and the documentation.\n\nNice.\n\n>\n> So gently deprecate this usage: remove its description from the\n> documentation and emit a warning if it is found. But for reasons of\n> backwards compatibility, continue to accept it.\n>\n> Helped-by: Brad King \n> Signed-off-by: Michael Haggerty \n> ---\n> Documentation/git-update-ref.txt | 18 ++++++++++++------\n> builtin/update-ref.c | 2 ++\n> t/t1400-update-ref.sh | 5 +++--\n> 3 files changed, 17 insertions(+), 8 deletions(-)\n>\n> diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt\n> index 0a0a551..c8f5ae5 100644\n> --- a/Documentation/git-update-ref.txt\n> +++ b/Documentation/git-update-ref.txt\n> @@ -68,7 +68,12 @@ performs all modifications together. Specify commands of the form:\n> \toption SP LF\n> \n> Quote fields containing whitespace as if they were strings in C source\n> -code. Alternatively, use `-z` to specify commands without quoting:\n> +code; i.e., surrounded by double-quotes and with backslash escapes.\n> +Use 40 \"0\" characters or the empty string to specify a zero value. To\n> +specify a missing value, omit the value and its preceding SP entirely.\n> +\n> +Alternatively, use `-z` to specify in NUL-terminated format, without\n> +quoting:\n> \n> \tupdate SP NUL NUL [] NUL\n> \tcreate SP NUL NUL\n> @@ -76,8 +81,12 @@ code. Alternatively, use `-z` to specify commands without quoting:\n> \tverify SP NUL [] NUL\n> \toption SP NUL\n> \n> -Lines of any other format or a repeated produce an error.\n> -Command meanings are:\n> +In this format, use 40 \"0\" to specify a zero value, and use the empty\n> +string to specify a missing value.\n> +\n> +In either format, values can be specified in any form that Git\n> +recognizes as an object name. Commands in any other format or a\n> +repeated produce an error. Command meanings are:\n> \n> update::\n> \tSet to after verifying , if given.\n> @@ -102,9 +111,6 @@ option::\n> \tThe only valid option is `no-deref` to avoid dereferencing\n> \ta symbolic ref.\n> \n> -Use 40 \"0\" or the empty string to specify a zero value, except that\n> -with `-z` an empty is considered missing.\n> -\n> If all s can be locked with matching s\n> simultaneously, all modifications are performed. Otherwise, no\n> modifications are performed. Note that while each individual\n> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n> index 6462b2f..eef7537 100644\n> --- a/builtin/update-ref.c\n> +++ b/builtin/update-ref.c\n> @@ -154,6 +154,8 @@ static int parse_next_sha1(struct strbuf *input, const char **next,\n> \t\t\t\tgoto invalid;\n> \t\t} else if (flags & PARSE_SHA1_ALLOW_EMPTY) {\n> \t\t\t/* With -z, treat an empty value as all zeros: */\n> +\t\t\twarning(\"%s %s: missing , treating as zero\",\n> +\t\t\t\tcommand, refname);\n> \t\t\thashclr(sha1);\n> \t\t} else {\n> \t\t\t/*\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index 15f5bfd..2d61cce 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -730,10 +730,11 @@ test_expect_success 'stdin -z fails update with bad ref name' '\n> \tgrep \"fatal: invalid ref format: ~a\" err\n> '\n> \n> -test_expect_success 'stdin -z treats empty new value as zeros' '\n> +test_expect_success 'stdin -z emits warning with empty new value' '\n> \tgit update-ref $a $m &&\n> \tprintf $F \"update $a\" \"\" \"\" >stdin &&\n> -\tgit update-ref -z --stdin +\tgit update-ref -z --stdin err &&\n> +\tgrep \"warning: update $a: missing , treating as zero\" err &&\n> \ttest_must_fail git rev-parse --verify -q $a\n> '\n"},{"id":"238160","messageId":"5339E2FC.8080403@alum.mit.edu","threadId":"36283","inReplyTo":"xmqqppl23vjl.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 01/27] t1400: Fix name and expected result of one test","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-31T21:49:48Z","receivedAt":"2014-03-31T21:49:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/31/2014 11:30 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> The test\n>>\n>> stdin -z create ref fails with zero new value\n>>\n>> actually passes an empty new value, not a zero new value. So rename\n>> the test s/zero/empty/, and change the expected error from\n>>\n>> fatal: create $c given zero new value\n>>\n>> to\n>>\n>> fatal: create $c missing \n> \n> I have a feeling that \"zero new value\" might have been done by a\n> non-native (like me) to say \"no new value\"; \"missing newvalue\"\n> sounds like a good phrasing to use.\n> \n>> Of course, this makes the test fail now, so mark it\n>> test_expect_failure. The failure will be fixed later in this patch\n>> series.\n> \n> That sounds somewhat strange. Why not just give a single-liner to\n> update-ref.c instead?\n\nThis is because there really is a difference between the two errors, and\n\"git update-ref\" tries to emit distinct error messages for them:\n\n* \"zero new value\" means that the new value was 0{40}\n* \"missing \" means that the new value was absent\n\nThe problem is that it is not distinguishing between these two cases\ncorrectly, and fixing *that* is more than a one-liner.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238161","messageId":"xmqq38hy3umr.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"1395683820-17304-17-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 16/27] t1400: Test one mistake at a time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-31T21:50:36Z","receivedAt":"2014-03-31T21:50:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> This case wants to test passing a bad refname to the \"update\" command.\n> But it also passes too few arguments to \"update\", which muddles the\n> situation: which error should be diagnosed? So split this test into\n> two:\n>\n> * One that passes too few arguments to update\n>\n> * One that passes all three arguments to \"update\", but with a bad\n> refname.\n>\n> Signed-off-by: Michael Haggerty \n>\n> t1400: Add a test of \"update\" with too few arguments\n>\n> Signed-off-by: Michael Haggerty \n\nWhat's happening here?\n\n> ---\n> t/t1400-update-ref.sh | 8 +++++++-\n> 1 file changed, 7 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index 2d61cce..6b21e45 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -724,8 +724,14 @@ test_expect_success 'stdin -z fails update with no ref' '\n> \tgrep \"fatal: update line missing \" err\n> '\n> \n> +test_expect_success 'stdin -z fails update with too few args' '\n> +\tprintf $F \"update $a\" \"$m\" >stdin &&\n> +\ttest_must_fail git update-ref -z --stdin err &&\n> +\tgrep \"fatal: update $a missing \" err\n> +'\n> +\n> test_expect_success 'stdin -z fails update with bad ref name' '\n> -\tprintf $F \"update ~a\" \"$m\" >stdin &&\n> +\tprintf $F \"update ~a\" \"$m\" \"\" >stdin &&\n> \ttest_must_fail git update-ref -z --stdin err &&\n> \tgrep \"fatal: invalid ref format: ~a\" err\n> '\n"},{"id":"238163","messageId":"xmqqy4zq2g0b.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"1395683820-17304-19-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 18/27] update-ref --stdin: Harmonize error messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-31T21:51:48Z","receivedAt":"2014-03-31T21:51:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Make (most of) the error messages for invalid input have the same\n> format [1]:\n>\n> $COMMAND [SP $REFNAME]: $MESSAGE\n>\n> Update the tests accordingly.\n>\n> [1] A few error messages are left with their old form, because\n> $COMMAND and $REFNAME aren't passed all the way down the call\n> stack. Maybe those sites should be changed some day, too.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n\nUp to this point, modulo nits that have been pointed out separately,\nthe series looked reasonably well done.\n\nThanks.\n\n> builtin/update-ref.c | 24 ++++++++++++------------\n> t/t1400-update-ref.sh | 32 ++++++++++++++++----------------\n> 2 files changed, 28 insertions(+), 28 deletions(-)\n>\n> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n> index b49a5b0..bbc04b2 100644\n> --- a/builtin/update-ref.c\n> +++ b/builtin/update-ref.c\n> @@ -202,19 +202,19 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)\n> \n> \tupdate->ref_name = parse_refname(input, &next);\n> \tif (!update->ref_name)\n> -\t\tdie(\"update line missing \");\n> +\t\tdie(\"update: missing \");\n> \n> \tif (parse_next_sha1(input, &next, update->new_sha1,\n> \t\t\t \"update\", update->ref_name,\n> \t\t\t PARSE_SHA1_ALLOW_EMPTY))\n> -\t\tdie(\"update %s missing \", update->ref_name);\n> +\t\tdie(\"update %s: missing \", update->ref_name);\n> \n> \tupdate->have_old = !parse_next_sha1(input, &next, update->old_sha1,\n> \t\t\t\t\t \"update\", update->ref_name,\n> \t\t\t\t\t PARSE_SHA1_OLD);\n> \n> \tif (*next != line_termination)\n> -\t\tdie(\"update %s has extra input: %s\", update->ref_name, next);\n> +\t\tdie(\"update %s: extra input: %s\", update->ref_name, next);\n> \n> \treturn next;\n> }\n> @@ -227,17 +227,17 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)\n> \n> \tupdate->ref_name = parse_refname(input, &next);\n> \tif (!update->ref_name)\n> -\t\tdie(\"create line missing \");\n> +\t\tdie(\"create: missing \");\n> \n> \tif (parse_next_sha1(input, &next, update->new_sha1,\n> \t\t\t \"create\", update->ref_name, 0))\n> -\t\tdie(\"create %s missing \", update->ref_name);\n> +\t\tdie(\"create %s: missing \", update->ref_name);\n> \n> \tif (is_null_sha1(update->new_sha1))\n> -\t\tdie(\"create %s given zero \", update->ref_name);\n> +\t\tdie(\"create %s: zero \", update->ref_name);\n> \n> \tif (*next != line_termination)\n> -\t\tdie(\"create %s has extra input: %s\", update->ref_name, next);\n> +\t\tdie(\"create %s: extra input: %s\", update->ref_name, next);\n> \n> \treturn next;\n> }\n> @@ -250,19 +250,19 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)\n> \n> \tupdate->ref_name = parse_refname(input, &next);\n> \tif (!update->ref_name)\n> -\t\tdie(\"delete line missing \");\n> +\t\tdie(\"delete: missing \");\n> \n> \tif (parse_next_sha1(input, &next, update->old_sha1,\n> \t\t\t \"delete\", update->ref_name, PARSE_SHA1_OLD)) {\n> \t\tupdate->have_old = 0;\n> \t} else {\n> \t\tif (is_null_sha1(update->old_sha1))\n> -\t\t\tdie(\"delete %s given zero \", update->ref_name);\n> +\t\t\tdie(\"delete %s: zero \", update->ref_name);\n> \t\tupdate->have_old = 1;\n> \t}\n> \n> \tif (*next != line_termination)\n> -\t\tdie(\"delete %s has extra input: %s\", update->ref_name, next);\n> +\t\tdie(\"delete %s: extra input: %s\", update->ref_name, next);\n> \n> \treturn next;\n> }\n> @@ -275,7 +275,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n> \n> \tupdate->ref_name = parse_refname(input, &next);\n> \tif (!update->ref_name)\n> -\t\tdie(\"verify line missing \");\n> +\t\tdie(\"verify: missing \");\n> \n> \tif (parse_next_sha1(input, &next, update->old_sha1,\n> \t\t\t \"verify\", update->ref_name, PARSE_SHA1_OLD)) {\n> @@ -286,7 +286,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n> \t}\n> \n> \tif (*next != line_termination)\n> -\t\tdie(\"verify %s has extra input: %s\", update->ref_name, next);\n> +\t\tdie(\"verify %s: extra input: %s\", update->ref_name, next);\n> \n> \treturn next;\n> }\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index 1db0689..48ccc4d 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -371,7 +371,7 @@ test_expect_success 'stdin fails on junk after quoted argument' '\n> test_expect_success 'stdin fails create with no ref' '\n> \techo \"create \" >stdin &&\n> \ttest_must_fail git update-ref --stdin err &&\n> -\tgrep \"fatal: create line missing \" err\n> +\tgrep \"fatal: create: missing \" err\n> '\n> \n> test_expect_success 'stdin fails create with bad ref name' '\n> @@ -383,19 +383,19 @@ test_expect_success 'stdin fails create with bad ref name' '\n> test_expect_success 'stdin fails create with no new value' '\n> \techo \"create $a\" >stdin &&\n> \ttest_must_fail git update-ref --stdin err &&\n> -\tgrep \"fatal: create $a missing \" err\n> +\tgrep \"fatal: create $a: missing \" err\n> '\n> \n> test_expect_success 'stdin fails create with too many arguments' '\n> \techo \"create $a $m $m\" >stdin &&\n> \ttest_must_fail git update-ref --stdin err &&\n> -\tgrep \"fatal: create $a has extra input: $m\" err\n> +\tgrep \"fatal: create $a: extra input: $m\" err\n> '\n> \n> test_expect_success 'stdin fails update with no ref' '\n> \techo \"update \" >stdin &&\n> \ttest_must_fail git update-ref --stdin err &&\n> -\tgrep \"fatal: update line missing \" err\n> +\tgrep \"fatal: update: missing \" err\n> '\n> \n> test_expect_success 'stdin fails update with bad ref name' '\n> @@ -407,19 +407,19 @@ test_expect_success 'stdin fails update with bad ref name' '\n> test_expect_success 'stdin fails update with no new value' '\n> \techo \"update $a\" >stdin &&\n> \ttest_must_fail git update-ref --stdin err &&\n> -\tgrep \"fatal: update $a missing \" err\n> +\tgrep \"fatal: update $a: missing \" err\n> '\n> \n> test_expect_success 'stdin fails update with too many arguments' '\n> \techo \"update $a $m $m $m\" >stdin &&\n> \ttest_must_fail git update-ref --stdin err &&\n> -\tgrep \"fatal: update $a has extra input: $m\" err\n> +\tgrep \"fatal: update $a: extra input: $m\" err\n> '\n> \n> test_expect_success 'stdin fails delete with no ref' '\n> \techo \"delete \" >stdin &&\n> \ttest_must_fail git update-ref --stdin err &&\n> -\tgrep \"fatal: delete line missing \" err\n> +\tgrep \"fatal: delete: missing \" err\n> '\n> \n> test_expect_success 'stdin fails delete with bad ref name' '\n> @@ -431,13 +431,13 @@ test_expect_success 'stdin fails delete with bad ref name' '\n> test_expect_success 'stdin fails delete with too many arguments' '\n> \techo \"delete $a $m $m\" >stdin &&\n> \ttest_must_fail git update-ref --stdin err &&\n> -\tgrep \"fatal: delete $a has extra input: $m\" err\n> +\tgrep \"fatal: delete $a: extra input: $m\" err\n> '\n> \n> test_expect_success 'stdin fails verify with too many arguments' '\n> \techo \"verify $a $m $m\" >stdin &&\n> \ttest_must_fail git update-ref --stdin err &&\n> -\tgrep \"fatal: verify $a has extra input: $m\" err\n> +\tgrep \"fatal: verify $a: extra input: $m\" err\n> '\n> \n> test_expect_success 'stdin fails option with unknown name' '\n> @@ -532,7 +532,7 @@ test_expect_success 'stdin create ref fails with bad new value' '\n> test_expect_success 'stdin create ref fails with zero new value' '\n> \techo \"create $c \" >stdin &&\n> \ttest_must_fail git update-ref --stdin err &&\n> -\tgrep \"fatal: create $c given zero \" err &&\n> +\tgrep \"fatal: create $c: zero \" err &&\n> \ttest_must_fail git rev-parse --verify -q $c\n> '\n> \n> @@ -556,7 +556,7 @@ test_expect_success 'stdin delete ref fails with wrong old value' '\n> test_expect_success 'stdin delete ref fails with zero old value' '\n> \techo \"delete $a \" >stdin &&\n> \ttest_must_fail git update-ref --stdin err &&\n> -\tgrep \"fatal: delete $a given zero \" err &&\n> +\tgrep \"fatal: delete $a: zero \" err &&\n> \tgit rev-parse $m >expect &&\n> \tgit rev-parse $a >actual &&\n> \ttest_cmp expect actual\n> @@ -697,7 +697,7 @@ test_expect_success 'stdin -z fails on unknown command' '\n> test_expect_success 'stdin -z fails create with no ref' '\n> \tprintf $F \"create \" >stdin &&\n> \ttest_must_fail git update-ref -z --stdin err &&\n> -\tgrep \"fatal: create line missing \" err\n> +\tgrep \"fatal: create: missing \" err\n> '\n> \n> test_expect_success 'stdin -z fails create with bad ref name' '\n> @@ -721,7 +721,7 @@ test_expect_success 'stdin -z fails create with too many arguments' '\n> test_expect_success 'stdin -z fails update with no ref' '\n> \tprintf $F \"update \" >stdin &&\n> \ttest_must_fail git update-ref -z --stdin err &&\n> -\tgrep \"fatal: update line missing \" err\n> +\tgrep \"fatal: update: missing \" err\n> '\n> \n> test_expect_success 'stdin -z fails update with too few args' '\n> @@ -765,7 +765,7 @@ test_expect_success 'stdin -z fails update with too many arguments' '\n> test_expect_success 'stdin -z fails delete with no ref' '\n> \tprintf $F \"delete \" >stdin &&\n> \ttest_must_fail git update-ref -z --stdin err &&\n> -\tgrep \"fatal: delete line missing \" err\n> +\tgrep \"fatal: delete: missing \" err\n> '\n> \n> test_expect_success 'stdin -z fails delete with bad ref name' '\n> @@ -868,7 +868,7 @@ test_expect_success 'stdin -z create ref fails with bad new value' '\n> test_expect_success 'stdin -z create ref fails with empty new value' '\n> \tprintf $F \"create $c\" \"\" >stdin &&\n> \ttest_must_fail git update-ref -z --stdin err &&\n> -\tgrep \"fatal: create $c missing \" err &&\n> +\tgrep \"fatal: create $c: missing \" err &&\n> \ttest_must_fail git rev-parse --verify -q $c\n> '\n> \n> @@ -892,7 +892,7 @@ test_expect_success 'stdin -z delete ref fails with wrong old value' '\n> test_expect_success 'stdin -z delete ref fails with zero old value' '\n> \tprintf $F \"delete $a\" \"$Z\" >stdin &&\n> \ttest_must_fail git update-ref -z --stdin err &&\n> -\tgrep \"fatal: delete $a given zero \" err &&\n> +\tgrep \"fatal: delete $a: zero \" err &&\n> \tgit rev-parse $m >expect &&\n> \tgit rev-parse $a >actual &&\n> \ttest_cmp expect actual\n"},{"id":"238169","messageId":"5339E7F4.1090805@alum.mit.edu","threadId":"36283","inReplyTo":"xmqqlhvq3vaj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 03/27] parse_arg(): Really test that argument is properly terminated","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-31T22:11:00Z","receivedAt":"2014-03-31T22:11:00Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/31/2014 11:36 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> Test that the argument is properly terminated by either whitespace or\n>> a NUL character, even if it is quoted, to be consistent with the\n>> non-quoted case. Adjust the tests to expect the new error message.\n>> Add a docstring to the function, incorporating the comments that were\n>> formerly within the function plus some added information.\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n>> builtin/update-ref.c | 20 +++++++++++++++-----\n>> t/t1400-update-ref.sh | 4 ++--\n>> 2 files changed, 17 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n>> index 1292cfe..02b5f95 100644\n>> --- a/builtin/update-ref.c\n>> +++ b/builtin/update-ref.c\n>> @@ -62,16 +62,26 @@ static void update_store_old_sha1(struct ref_update *update,\n>> \tupdate->have_old = *oldvalue || line_termination;\n>> }\n>> \n>> +/*\n>> + * Parse one whitespace- or NUL-terminated, possibly C-quoted argument\n>> + * and append the result to arg. Return a pointer to the terminator.\n>> + * Die if there is an error in how the argument is C-quoted. This\n>> + * function is only used if not -z.\n>> + */\n>> static const char *parse_arg(const char *next, struct strbuf *arg)\n>> {\n>> -\t/* Parse SP-terminated, possibly C-quoted argument */\n>> -\tif (*next != '\"')\n>> +\tif (*next == '\"') {\n>> +\t\tconst char *orig = next;\n>> +\n>> +\t\tif (unquote_c_style(arg, next, &next))\n>> +\t\t\tdie(\"badly quoted argument: %s\", orig);\n>> +\t\tif (*next && !isspace(*next))\n>> +\t\t\tdie(\"unexpected character after quoted argument: %s\", orig);\n>> +\t} else {\n>> \t\twhile (*next && !isspace(*next))\n>> \t\t\tstrbuf_addch(arg, *next++);\n>> -\telse if (unquote_c_style(arg, next, &next))\n>> -\t\tdie(\"badly quoted argument: %s\", next);\n>> +\t}\n>> \n>> -\t/* Return position after the argument */\n>> \treturn next;\n>> }\n>> \n>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n>> index 29391c6..774f8c5 100755\n>> --- a/t/t1400-update-ref.sh\n>> +++ b/t/t1400-update-ref.sh\n>> @@ -356,10 +356,10 @@ test_expect_success 'stdin fails on badly quoted input' '\n>> \tgrep \"fatal: badly quoted argument: \\\\\\\"master\" err\n>> '\n>> \n>> -test_expect_success 'stdin fails on arguments not separated by space' '\n>> +test_expect_success 'stdin fails on junk after quoted argument' '\n>> \techo \"create \\\"$a\\\"master\" >stdin &&\n>> \ttest_must_fail git update-ref --stdin err &&\n>> -\tgrep \"fatal: expected SP but got: master\" err\n>> +\tgrep \"fatal: unexpected character after quoted argument: \\\\\\\"$a\\\\\\\"master\" err\n>> '\n> \n> Interesting.\n> \n> I would have expected that \"We used to check only one of the two\n> codepaths and the other was loose, fix it\" to be accompanied by \"So\n> here is an _addition_ to the test suite to validate the other case\n> that used to be loose, now tightened\", not \"We update one existing\n> case\". The test before and after the patch is about a c-quoted\n> string, so I am not sure if we are still testing the right thing.\n> \n> The code in update-ref.c after the patch does look reasonable,\n> though.\n\nThe old parse_arg(), when fed an argument\n\n \"refs/heads/a\"master\n\nparsed 'refs/heads/a' off of the front of the argument and considered\nitself successful. It was only when parse_next_arg() tried to parse the\n*next* argument that a problem was noticed. But in fact, the definition\nof the input format requires arguments to be terminated by SP or NUL, so\n*this* argument is already erroneous and parse_arg() should diagnose the\nproblem.\n\nThe point of this patch is to move the error detection for C-quoted\narguments that have trailing junk to the parse_arg() call for the broken\nargument and to make the error message more descriptive of the situation.\n\nThere is no corresponding error case for non-C-quoted arguments, because\nthe end of the argument is *by definition* a space or NUL, so there is\nno way to insert other junk between the \"end\" of the argument and the\nargument terminator.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238174","messageId":"5339E948.4090109@alum.mit.edu","threadId":"36283","inReplyTo":"xmqqha6e3v44.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 06/27] update_refs(): Fix constness","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-31T22:16:40Z","receivedAt":"2014-03-31T22:16:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/31/2014 11:40 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> Since full const correctness is beyond the ability of C's type system,\n>> just put the const where it doesn't do any harm. A (struct ref_update\n>> **) can be passed to a (struct ref_update * const *) argument, but not\n>> to a (const struct ref_update **) argument.\n> \n> Sounds good, but next time please try not to break lines inside a\n> single typename, which is somewhat unreadable ;-)\n> \n> I'd suggest rewording \"s/Fix/tighten/\". Because a patch that\n> changes constness can loosen constness to make things more correct,\n> \"git shortlog\" output that says if it is tightening or loosening\n> would be more informative than the one that says that it is \"fixing\".\n\nIt is not a strict tightening, because I add a \"const\" in one place but\nremove it from another:\n\n const struct ref_update **\n\nbecomes\n\n struct ref_update * const *\n\nin the update_refs() signature. In fact, the old declaration was too\nstrict for some changes later in the patch series, which is why I needed\nto loosen (one aspect) of it.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238175","messageId":"5339EA44.1050006@alum.mit.edu","threadId":"36283","inReplyTo":"xmqqbnwm3uqw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 13/27] t1400: Test that stdin -z update treats empty as zeros","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-31T22:20:52Z","receivedAt":"2014-03-31T22:20:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/31/2014 11:48 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> This is the (slightly inconsistent) status quo; make sure it doesn't\n>> change by accident.\n> \n> Interesting. So \"oldvalue\" being empty is \"we do not care what it\n> is\" (as opposed to \"we know it must not exist yet\" aka 0{40}), and\n> \"newvalue\" being empty is the same as \"delete it\" aka 0{40}.\n> \n> That is unfortunate, but I agree it is a good idea to add a test for\n> it, so that we will notice when we decide to fix it.\n\nCorrect. This was discussed at some more length here [1]. In v1 of\nthis patch series I incorrectly changed this behavior, thinking it to\nhave been an accident.\n\nMichael\n\n[1]\nhttp://thread.gmane.org/gmane.comp.version-control.git/243731/focus=243773\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238177","messageId":"5339ED15.6040201@alum.mit.edu","threadId":"36283","inReplyTo":"xmqq38hy3umr.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 16/27] t1400: Test one mistake at a time","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-31T22:32:53Z","receivedAt":"2014-03-31T22:32:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/31/2014 11:50 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> This case wants to test passing a bad refname to the \"update\" command.\n>> But it also passes too few arguments to \"update\", which muddles the\n>> situation: which error should be diagnosed? So split this test into\n>> two:\n>>\n>> * One that passes too few arguments to update\n>>\n>> * One that passes all three arguments to \"update\", but with a bad\n>> refname.\n>>\n>> Signed-off-by: Michael Haggerty \n>>\n>> t1400: Add a test of \"update\" with too few arguments\n>>\n>> Signed-off-by: Michael Haggerty \n> \n> What's happening here?\n\nThat was a squashing accident, also pointed out by Brad [1]. The last\nthree lines should be deleted. This and an error in a comment in patch\n14/27 that was also pointed out by Brad are both fixed in my GitHub repo\n[2]. I haven't sent the fixed version to the list, though; let me know\nif I should.\n\nMichael\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/245205\n[2] Branch \"ref-transactions\" at https://github.com/mhagger/git\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238178","messageId":"5339EE33.7050708@alum.mit.edu","threadId":"36283","inReplyTo":"xmqqy4zq2g0b.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 18/27] update-ref --stdin: Harmonize error messages","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-31T22:37:39Z","receivedAt":"2014-03-31T22:37:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/31/2014 11:51 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> Make (most of) the error messages for invalid input have the same\n>> format [1]:\n>>\n>> $COMMAND [SP $REFNAME]: $MESSAGE\n>>\n>> Update the tests accordingly.\n>>\n>> [1] A few error messages are left with their old form, because\n>> $COMMAND and $REFNAME aren't passed all the way down the call\n>> stack. Maybe those sites should be changed some day, too.\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n> \n> Up to this point, modulo nits that have been pointed out separately,\n> the series looked reasonably well done.\n\nThanks for the feedback! Would you like me to expand the commit\nmessages to answer the questions that you asked about the previous\npatches? And if so, do you want a v3 sent to the list already or should\nI wait for you to review patches 19-27 first?\n\nCheers,\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238179","messageId":"xmqq38hy2dtx.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"5339E948.4090109@alum.mit.edu","subject":"Re: [PATCH v2 06/27] update_refs(): Fix constness","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-31T22:38:50Z","receivedAt":"2014-03-31T22:38:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 03/31/2014 11:40 PM, Junio C Hamano wrote:\n>> Michael Haggerty writes:\n>> \n>>> Since full const correctness is beyond the ability of C's type system,\n>>> just put the const where it doesn't do any harm. A (struct ref_update\n>>> **) can be passed to a (struct ref_update * const *) argument, but not\n>>> to a (const struct ref_update **) argument.\n>> \n>> Sounds good, but next time please try not to break lines inside a\n>> single typename, which is somewhat unreadable ;-)\n>> \n>> I'd suggest rewording \"s/Fix/tighten/\". Because a patch that\n>> changes constness can loosen constness to make things more correct,\n>> \"git shortlog\" output that says if it is tightening or loosening\n>> would be more informative than the one that says that it is \"fixing\".\n>\n> It is not a strict tightening, because I add a \"const\" in one place but\n> remove it from another:\n>\n> const struct ref_update **\n>\n> becomes\n>\n> struct ref_update * const *\n>\n> in the update_refs() signature. In fact, the old declaration was too\n> strict for some changes later in the patch series, which is why I needed\n> to loosen (one aspect) of it.\n\nInteresting. Then that _is_ a fix. Thanks for explaining it to\nme. As always, I would prefer it be explained to the proposed\ncommit log, not to me over an e-mail ;-)\n"},{"id":"238192","messageId":"533A86F2.90508@alum.mit.edu","threadId":"36283","inReplyTo":"5339EE33.7050708@alum.mit.edu","subject":"Re: [PATCH v2 18/27] update-ref --stdin: Harmonize error messages","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-01T09:29:22Z","receivedAt":"2014-04-01T09:29:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/01/2014 12:37 AM, Michael Haggerty wrote:\n> On 03/31/2014 11:51 PM, Junio C Hamano wrote:\n>> Michael Haggerty writes:\n>>\n>>> Make (most of) the error messages for invalid input have the same\n>>> format [1]:\n>>>\n>>> $COMMAND [SP $REFNAME]: $MESSAGE\n>>>\n>>> Update the tests accordingly.\n>>>\n>>> [1] A few error messages are left with their old form, because\n>>> $COMMAND and $REFNAME aren't passed all the way down the call\n>>> stack. Maybe those sites should be changed some day, too.\n>>>\n>>> Signed-off-by: Michael Haggerty \n>>> ---\n>>\n>> Up to this point, modulo nits that have been pointed out separately,\n>> the series looked reasonably well done.\n> \n> Thanks for the feedback! Would you like me to expand the commit\n> messages to answer the questions that you asked about the previous\n> patches? And if so, do you want a v3 sent to the list already or should\n> I wait for you to review patches 19-27 first?\n\nJunio, I incorporated your feedback (which so far has only affected\ncommit messages). I also rebased the patch series to the current\nmaster. I pushed the result to GitHub [1]. I'll refrain from spamming\nthe list with v3 yet.\n\nMichael\n\n[1] Branch \"ref-transactions\" at https://github.com/mhagger/git\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238250","messageId":"xmqqy4zozw9q.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"1395683820-17304-25-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 24/27] ref_transaction_commit(): Introduce temporary variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-01T19:26:25Z","receivedAt":"2014-04-01T19:26:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Use temporary variables in the for-loop blocks to simplify expressions\n> in the rest of the loop.\n\nShouldn't the summary of the change \"simplify expressions\"? Use of\ntemporary variables is a means to the end. If you have enough room\nto say \"achieve X by doing Y\", please do so; otherwise \"achieve X\"\nis more important part than \"do Y\".\n\nOther than that, this looks good.\n\n> Signed-off-by: Michael Haggerty \n> ---\n> refs.c | 21 +++++++++++++--------\n> 1 file changed, 13 insertions(+), 8 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 2b80f6d..d51566c 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3432,10 +3432,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n> \n> \t/* Acquire all locks while verifying old values */\n> \tfor (i = 0; i < n; i++) {\n> -\t\tlocks[i] = update_ref_lock(updates[i]->refname,\n> -\t\t\t\t\t (updates[i]->have_old ?\n> -\t\t\t\t\t updates[i]->old_sha1 : NULL),\n> -\t\t\t\t\t updates[i]->flags,\n> +\t\tstruct ref_update *update = updates[i];\n> +\n> +\t\tlocks[i] = update_ref_lock(update->refname,\n> +\t\t\t\t\t (update->have_old ?\n> +\t\t\t\t\t update->old_sha1 : NULL),\n> +\t\t\t\t\t update->flags,\n> \t\t\t\t\t &types[i], onerr);\n> \t\tif (!locks[i]) {\n> \t\t\tret = 1;\n> @@ -3444,16 +3446,19 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n> \t}\n> \n> \t/* Perform updates first so live commits remain referenced */\n> -\tfor (i = 0; i < n; i++)\n> -\t\tif (!is_null_sha1(updates[i]->new_sha1)) {\n> +\tfor (i = 0; i < n; i++) {\n> +\t\tstruct ref_update *update = updates[i];\n> +\n> +\t\tif (!is_null_sha1(update->new_sha1)) {\n> \t\t\tret = update_ref_write(msg,\n> -\t\t\t\t\t updates[i]->refname,\n> -\t\t\t\t\t updates[i]->new_sha1,\n> +\t\t\t\t\t update->refname,\n> +\t\t\t\t\t update->new_sha1,\n> \t\t\t\t\t locks[i], onerr);\n> \t\t\tlocks[i] = NULL; /* freed by update_ref_write */\n> \t\t\tif (ret)\n> \t\t\t\tgoto cleanup;\n> \t\t}\n> +\t}\n> \n> \t/* Perform deletes now that updates are safely completed */\n> \tfor (i = 0; i < n; i++)\n"},{"id":"238252","messageId":"xmqqtxaczvod.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"1395683820-17304-20-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 19/27] refs: Add a concept of a reference transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-01T19:39:14Z","receivedAt":"2014-04-01T19:39:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Build out the API for dealing with a bunch of reference checks and\n> changes within a transaction. Define an opaque ref_transaction type\n> that is managed entirely within refs.c. Introduce functions for\n> beginning a transaction, adding updates to a transaction, and\n> committing/rolling back a transaction.\n>\n> This API will soon replace update_refs().\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> refs.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n> refs.h | 65 +++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 161 insertions(+)\n>\n> diff --git a/refs.c b/refs.c\n> index 1305eb1..e788c27 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3267,6 +3267,93 @@ static int update_ref_write(const char *action, const char *refname,\n> \treturn 0;\n> }\n> \n> +/*\n> + * Data structure for holding a reference transaction, which can\n> + * consist of checks and updates to multiple references, carried out\n> + * as atomically as possible. This structure is opaque to callers.\n> + */\n> +struct ref_transaction {\n> +\tstruct ref_update **updates;\n\nDon't we try to name an array update[] (not plural updates[]) so\nthat we can say update[7] to mean the seventh update?\n\n> +\tsize_t alloc;\n> +\tsize_t nr;\n> +};\n> +\n> +struct ref_transaction *ref_transaction_begin(void)\n> +{\n> +\treturn xcalloc(1, sizeof(struct ref_transaction));\n> +}\n> +\n> +static void ref_transaction_free(struct ref_transaction *transaction)\n> +{\n> +\tint i;\n> +\n> +\tfor (i = 0; i < transaction->nr; i++) {\n> +\t\tstruct ref_update *update = transaction->updates[i];\n> +\n> +\t\tfree((char *)update->ref_name);\n> +\t\tfree(update);\n> +\t}\n> +\n> +\tfree(transaction->updates);\n> +\tfree(transaction);\n> +}\n\nOK.\n\n> +void ref_transaction_rollback(struct ref_transaction *transaction)\n> +{\n> +\tref_transaction_free(transaction);\n> +}\n\nOK.\n\n> +static struct ref_update *add_update(struct ref_transaction *transaction,\n> +\t\t\t\t const char *refname)\n> +{\n> +\tstruct ref_update *update = xcalloc(1, sizeof(*update));\n> +\n> +\tupdate->ref_name = xstrdup(refname);\n> +\tALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);\n> +\ttransaction->updates[transaction->nr++] = update;\n> +\treturn update;\n> +}\n> +\n> +void ref_transaction_update(struct ref_transaction *transaction,\n> +\t\t\t const char *refname,\n> +\t\t\t unsigned char *new_sha1, unsigned char *old_sha1,\n> +\t\t\t int flags, int have_old)\n> +{\n> +\tstruct ref_update *update = add_update(transaction, refname);\n> +\n> +\thashcpy(update->new_sha1, new_sha1);\n> +\tupdate->flags = flags;\n> +\tupdate->have_old = have_old;\n> +\tif (have_old)\n> +\t\thashcpy(update->old_sha1, old_sha1);\n> +}\n> +\n> +void ref_transaction_create(struct ref_transaction *transaction,\n> +\t\t\t const char *refname,\n> +\t\t\t unsigned char *new_sha1,\n> +\t\t\t int flags)\n> +{\n> +\tstruct ref_update *update = add_update(transaction, refname);\n> +\n> +\thashcpy(update->new_sha1, new_sha1);\n> +\thashclr(update->old_sha1);\n> +\tupdate->flags = flags;\n> +\tupdate->have_old = 1;\n> +}\n> +\n> +void ref_transaction_delete(struct ref_transaction *transaction,\n> +\t\t\t const char *refname,\n> +\t\t\t unsigned char *old_sha1,\n> +\t\t\t int flags, int have_old)\n> +{\n> +\tstruct ref_update *update = add_update(transaction, refname);\n> +\n> +\tupdate->flags = flags;\n> +\tupdate->have_old = have_old;\n> +\tif (have_old)\n> +\t\thashcpy(update->old_sha1, old_sha1);\n> +}\n> +\n\nI can see that the chosen set of primitives update/create/delete\nmirrors what update-ref allows us to do, but given the explanation\nof \"update\" in refs.h, wouldn't it make more sense to implement the\nothers in terms of it?\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> @@ -3378,6 +3465,15 @@ cleanup:\n> \treturn ret;\n> }\n> \n> +int ref_transaction_commit(struct ref_transaction *transaction,\n> +\t\t\t const char *msg, enum action_on_err onerr)\n> +{\n> +\tint ret = update_refs(msg, transaction->updates, transaction->nr,\n> +\t\t\t onerr);\n> +\tref_transaction_free(transaction);\n> +\treturn ret;\n> +}\n\nOK.\n\n> char *shorten_unambiguous_ref(const char *refname, int strict)\n> {\n> \tint i;\n> diff --git a/refs.h b/refs.h\n> index 08e60ac..476a923 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -24,6 +24,8 @@ struct ref_update {\n> \tint have_old; /* 1 if old_sha1 is valid, 0 otherwise */\n> };\n> \n> +struct ref_transaction;\n> +\n> /*\n> * Bit values set in the flags argument passed to each_ref_fn():\n> */\n> @@ -220,6 +222,69 @@ enum action_on_err {\n> \tUPDATE_REFS_QUIET_ON_ERR\n> };\n> \n> +/*\n> + * Begin a reference transaction. The reference transaction must\n> + * eventually be commited using ref_transaction_commit() or rolled\n> + * back using ref_transaction_rollback().\n> + */\n> +struct ref_transaction *ref_transaction_begin(void);\n> +\n> +/*\n> + * Roll back a ref_transaction and free all associated data.\n> + */\n> +void ref_transaction_rollback(struct ref_transaction *transaction);\n> +\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, so the caller retains ownership of the parameter. flags\n\nGood to see the ownership rules described.\n\n> + * can be REF_NODEREF; it is passed to update_ref_lock().\n> + */\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 zeros 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> + * it must not have existed beforehand.\n> + */\n> +void ref_transaction_update(struct ref_transaction *transaction,\n> +\t\t\t const char *refname,\n> +\t\t\t unsigned char *new_sha1, unsigned char *old_sha1,\n> +\t\t\t int flags, int have_old);\n> +\n> +/*\n> + * Add a reference creation to transaction. new_sha1 is the value\n> + * that the reference should have after the update, or zeros if it\n> + * should be deleted. It is verified that the reference does not\n> + * exist already.\n> + */\n\nSounds a bit crazy that you can ask \"create\", which verifies the\nabsense of the thing, to delete a thing.\n\n> +void ref_transaction_create(struct ref_transaction *transaction,\n> +\t\t\t const char *refname,\n> +\t\t\t unsigned char *new_sha1,\n> +\t\t\t int flags);\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> + * the update.\n> + */\n> +void ref_transaction_delete(struct ref_transaction *transaction,\n> +\t\t\t const char *refname,\n> +\t\t\t unsigned char *old_sha1,\n> +\t\t\t int flags, int have_old);\n> +\n> +/*\n> + * Commit all of the changes that have been queued in transaction, as\n> + * atomically as possible. Return a nonzero value if there is a\n> + * problem. The ref_transaction is freed by this function.\n> + */\n> +int ref_transaction_commit(struct ref_transaction *transaction,\n> +\t\t\t const char *msg, enum action_on_err onerr);\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"},{"id":"238254","messageId":"xmqqppl0zvcs.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"1395683820-17304-21-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-01T19:46:11Z","receivedAt":"2014-04-01T19:46:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> This change is mostly clerical: the parse_cmd_*() functions need to\n> use local variables rather than a struct ref_update to collect the\n> arguments needed for each update, and then call ref_transaction_*() to\n> queue the change rather than building up the list of changes at the\n> caller side.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n\nWith the implementation of ref_transaction at this point in the\nseries it does not matter, but the updated code in this patch means\nthat it is perfectly acceptable to do this sequence:\n\n ref_transaction_begin();\n ref_transaction_update();\n ...\n ref_transaction_update();\n die();\n\nwithout ever calling ref_transaction_rollback() API function.\nDepending on the future backends, we may want to ensure rollback is\ncalled, no? And if that is the case, we would want to prepare\ncallers of the API with some at-exit facility to call rollback, no?\n\nOther than that, the code looks straight-forward.\n\nThanks.\n\n> builtin/update-ref.c | 142 +++++++++++++++++++++++++++------------------------\n> 1 file changed, 75 insertions(+), 67 deletions(-)\n>\n> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n> index bbc04b2..2c8678b 100644\n> --- a/builtin/update-ref.c\n> +++ b/builtin/update-ref.c\n> @@ -12,29 +12,11 @@ static const char * const git_update_ref_usage[] = {\n> \tNULL\n> };\n> \n> -static int updates_alloc;\n> -static int updates_count;\n> -static struct ref_update **updates;\n> +static struct ref_transaction *transaction;\n> \n> static char line_termination = '\\n';\n> static int update_flags;\n> \n> -static struct ref_update *update_alloc(void)\n> -{\n> -\tstruct ref_update *update;\n> -\n> -\t/* Allocate and zero-init a struct ref_update */\n> -\tupdate = xcalloc(1, sizeof(*update));\n> -\tALLOC_GROW(updates, updates_count + 1, updates_alloc);\n> -\tupdates[updates_count++] = update;\n> -\n> -\t/* Store and reset accumulated options */\n> -\tupdate->flags = update_flags;\n> -\tupdate_flags = 0;\n> -\n> -\treturn update;\n> -}\n> -\n> /*\n> * Parse one whitespace- or NUL-terminated, possibly C-quoted argument\n> * and append the result to arg. Return a pointer to the terminator.\n> @@ -196,97 +178,119 @@ static int parse_next_sha1(struct strbuf *input, const char **next,\n> \n> static const char *parse_cmd_update(struct strbuf *input, const char *next)\n> {\n> -\tstruct ref_update *update;\n> -\n> -\tupdate = update_alloc();\n> +\tchar *refname;\n> +\tunsigned char new_sha1[20];\n> +\tunsigned char old_sha1[20];\n> +\tint have_old;\n> \n> -\tupdate->ref_name = parse_refname(input, &next);\n> -\tif (!update->ref_name)\n> +\trefname = parse_refname(input, &next);\n> +\tif (!refname)\n> \t\tdie(\"update: missing \");\n> \n> -\tif (parse_next_sha1(input, &next, update->new_sha1,\n> -\t\t\t \"update\", update->ref_name,\n> +\tif (parse_next_sha1(input, &next, new_sha1, \"update\", refname,\n> \t\t\t PARSE_SHA1_ALLOW_EMPTY))\n> -\t\tdie(\"update %s: missing \", update->ref_name);\n> +\t\tdie(\"update %s: missing \", refname);\n> \n> -\tupdate->have_old = !parse_next_sha1(input, &next, update->old_sha1,\n> -\t\t\t\t\t \"update\", update->ref_name,\n> -\t\t\t\t\t PARSE_SHA1_OLD);\n> +\thave_old = !parse_next_sha1(input, &next, old_sha1, \"update\", refname,\n> +\t\t\t\t PARSE_SHA1_OLD);\n> \n> \tif (*next != line_termination)\n> -\t\tdie(\"update %s: extra input: %s\", update->ref_name, next);\n> +\t\tdie(\"update %s: extra input: %s\", refname, next);\n> +\n> +\tref_transaction_update(transaction, refname, new_sha1, old_sha1,\n> +\t\t\t update_flags, have_old);\n> +\n> +\tupdate_flags = 0;\n> +\tfree(refname);\n> \n> \treturn next;\n> }\n> \n> static const char *parse_cmd_create(struct strbuf *input, const char *next)\n> {\n> -\tstruct ref_update *update;\n> -\n> -\tupdate = update_alloc();\n> +\tchar *refname;\n> +\tunsigned char new_sha1[20];\n> \n> -\tupdate->ref_name = parse_refname(input, &next);\n> -\tif (!update->ref_name)\n> +\trefname = parse_refname(input, &next);\n> +\tif (!refname)\n> \t\tdie(\"create: missing \");\n> \n> -\tif (parse_next_sha1(input, &next, update->new_sha1,\n> -\t\t\t \"create\", update->ref_name, 0))\n> -\t\tdie(\"create %s: missing \", update->ref_name);\n> +\tif (parse_next_sha1(input, &next, new_sha1, \"create\", refname, 0))\n> +\t\tdie(\"create %s: missing \", refname);\n> \n> -\tif (is_null_sha1(update->new_sha1))\n> -\t\tdie(\"create %s: zero \", update->ref_name);\n> +\tif (is_null_sha1(new_sha1))\n> +\t\tdie(\"create %s: zero \", refname);\n> \n> \tif (*next != line_termination)\n> -\t\tdie(\"create %s: extra input: %s\", update->ref_name, next);\n> +\t\tdie(\"create %s: extra input: %s\", refname, next);\n> +\n> +\tref_transaction_create(transaction, refname, new_sha1, update_flags);\n> +\n> +\tupdate_flags = 0;\n> +\tfree(refname);\n> \n> \treturn next;\n> }\n> \n> static const char *parse_cmd_delete(struct strbuf *input, const char *next)\n> {\n> -\tstruct ref_update *update;\n> +\tchar *refname;\n> +\tunsigned char old_sha1[20];\n> +\tint have_old;\n> \n> -\tupdate = update_alloc();\n> -\n> -\tupdate->ref_name = parse_refname(input, &next);\n> -\tif (!update->ref_name)\n> +\trefname = parse_refname(input, &next);\n> +\tif (!refname)\n> \t\tdie(\"delete: missing \");\n> \n> -\tif (parse_next_sha1(input, &next, update->old_sha1,\n> -\t\t\t \"delete\", update->ref_name, PARSE_SHA1_OLD)) {\n> -\t\tupdate->have_old = 0;\n> +\tif (parse_next_sha1(input, &next, old_sha1, \"delete\", refname,\n> +\t\t\t PARSE_SHA1_OLD)) {\n> +\t\thave_old = 0;\n> \t} else {\n> -\t\tif (is_null_sha1(update->old_sha1))\n> -\t\t\tdie(\"delete %s: zero \", update->ref_name);\n> -\t\tupdate->have_old = 1;\n> +\t\tif (is_null_sha1(old_sha1))\n> +\t\t\tdie(\"delete %s: zero \", refname);\n> +\t\thave_old = 1;\n> \t}\n> \n> \tif (*next != line_termination)\n> -\t\tdie(\"delete %s: extra input: %s\", update->ref_name, next);\n> +\t\tdie(\"delete %s: extra input: %s\", refname, next);\n> +\n> +\tref_transaction_delete(transaction, refname, old_sha1,\n> +\t\t\t update_flags, have_old);\n> +\n> +\tupdate_flags = 0;\n> +\tfree(refname);\n> \n> \treturn next;\n> }\n> \n> static const char *parse_cmd_verify(struct strbuf *input, const char *next)\n> {\n> -\tstruct ref_update *update;\n> -\n> -\tupdate = update_alloc();\n> +\tchar *refname;\n> +\tunsigned char new_sha1[20];\n> +\tunsigned char old_sha1[20];\n> +\tint have_old;\n> \n> -\tupdate->ref_name = parse_refname(input, &next);\n> -\tif (!update->ref_name)\n> +\trefname = parse_refname(input, &next);\n> +\tif (!refname)\n> \t\tdie(\"verify: missing \");\n> \n> -\tif (parse_next_sha1(input, &next, update->old_sha1,\n> -\t\t\t \"verify\", update->ref_name, PARSE_SHA1_OLD)) {\n> -\t\tupdate->have_old = 0;\n> +\tif (parse_next_sha1(input, &next, old_sha1, \"verify\", refname,\n> +\t\t\t PARSE_SHA1_OLD)) {\n> +\t\thashclr(new_sha1);\n> +\t\thave_old = 0;\n> \t} else {\n> -\t\thashcpy(update->new_sha1, update->old_sha1);\n> -\t\tupdate->have_old = 1;\n> +\t\thashcpy(new_sha1, old_sha1);\n> +\t\thave_old = 1;\n> \t}\n> \n> \tif (*next != line_termination)\n> -\t\tdie(\"verify %s: extra input: %s\", update->ref_name, next);\n> +\t\tdie(\"verify %s: extra input: %s\", refname, next);\n> +\n> +\tref_transaction_update(transaction, refname, new_sha1, old_sha1,\n> +\t\t\t update_flags, have_old);\n> +\n> +\tupdate_flags = 0;\n> +\tfree(refname);\n> \n> \treturn next;\n> }\n> @@ -355,13 +359,17 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n> \t\tdie(\"Refusing to perform update with empty message.\");\n> \n> \tif (read_stdin) {\n> +\t\tint ret;\n> +\t\ttransaction = ref_transaction_begin();\n> +\n> \t\tif (delete || no_deref || argc > 0)\n> \t\t\tusage_with_options(git_update_ref_usage, options);\n> \t\tif (end_null)\n> \t\t\tline_termination = '\\0';\n> \t\tupdate_refs_stdin();\n> -\t\treturn update_refs(msg, updates, updates_count,\n> -\t\t\t\t UPDATE_REFS_DIE_ON_ERR);\n> +\t\tret = ref_transaction_commit(transaction, msg,\n> +\t\t\t\t\t UPDATE_REFS_DIE_ON_ERR);\n> +\t\treturn ret;\n> \t}\n> \n> \tif (end_null)\n"},{"id":"238255","messageId":"xmqqlhvozvbu.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"1395683820-17304-22-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 21/27] refs: Remove API function update_refs()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-01T19:46:45Z","receivedAt":"2014-04-01T19:46:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> It has been superseded by reference transactions. This also means\n> that struct ref_update can become private.\n\nGood.\n"},{"id":"238257","messageId":"xmqqha6czuzu.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"1395683820-17304-23-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 22/27] struct ref_update: Rename field \"ref_name\" to \"refname\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-01T19:53:57Z","receivedAt":"2014-04-01T19:53:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> This is consistent with the usual nomenclature.\n\nI am of two minds.\n\nLooking for \"\\(\\.\\|->\\)ref_name\" used to ignore refname fields of\nother structures and let us focus on the ref_update structure. Yes,\nthere is the ref_lock structure that shares ref_name to contaminate\nsuch a grep output already, but this change makes the output even\nmore noisy, as you have to now look for \"\\(\\.\\|->\\)refname\" which\nwould give you more hits from other unrelated structures.\n\nOn the other hand, I do not like to name this to \"update_refname\" or\nsome nonsense like that, of course. A reference name field in a\n\"ref_update\" structure shouldn't have to say that it is about\nupdating in its name; it should be known from the name of the\nstructure it appears in.\n\nSo I dunno.\n\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> refs.c | 18 +++++++++---------\n> refs.h | 2 +-\n> 2 files changed, 10 insertions(+), 10 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index dfff117..d72d0ab 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3274,7 +3274,7 @@ static int update_ref_write(const char *action, const char *refname,\n> * value or to zero to ensure the ref does not exist before update.\n> */\n> struct ref_update {\n> -\tconst char *ref_name;\n> +\tconst char *refname;\n> \tunsigned char new_sha1[20];\n> \tunsigned char old_sha1[20];\n> \tint flags; /* REF_NODEREF? */\n> @@ -3304,7 +3304,7 @@ static void ref_transaction_free(struct ref_transaction *transaction)\n> \tfor (i = 0; i < transaction->nr; i++) {\n> \t\tstruct ref_update *update = transaction->updates[i];\n> \n> -\t\tfree((char *)update->ref_name);\n> +\t\tfree((char *)update->refname);\n> \t\tfree(update);\n> \t}\n> \n> @@ -3322,7 +3322,7 @@ static struct ref_update *add_update(struct ref_transaction *transaction,\n> {\n> \tstruct ref_update *update = xcalloc(1, sizeof(*update));\n> \n> -\tupdate->ref_name = xstrdup(refname);\n> +\tupdate->refname = xstrdup(refname);\n> \tALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);\n> \ttransaction->updates[transaction->nr++] = update;\n> \treturn update;\n> @@ -3383,7 +3383,7 @@ static int ref_update_compare(const void *r1, const void *r2)\n> {\n> \tconst struct ref_update * const *u1 = r1;\n> \tconst struct ref_update * const *u2 = r2;\n> -\treturn strcmp((*u1)->ref_name, (*u2)->ref_name);\n> +\treturn strcmp((*u1)->refname, (*u2)->refname);\n> }\n> \n> static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n> @@ -3391,14 +3391,14 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n> {\n> \tint i;\n> \tfor (i = 1; i < n; i++)\n> -\t\tif (!strcmp(updates[i - 1]->ref_name, updates[i]->ref_name)) {\n> +\t\tif (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {\n> \t\t\tconst char *str =\n> \t\t\t\t\"Multiple updates for ref '%s' not allowed.\";\n> \t\t\tswitch (onerr) {\n> \t\t\tcase UPDATE_REFS_MSG_ON_ERR:\n> -\t\t\t\terror(str, updates[i]->ref_name); break;\n> +\t\t\t\terror(str, updates[i]->refname); break;\n> \t\t\tcase UPDATE_REFS_DIE_ON_ERR:\n> -\t\t\t\tdie(str, updates[i]->ref_name); break;\n> +\t\t\t\tdie(str, updates[i]->refname); break;\n> \t\t\tcase UPDATE_REFS_QUIET_ON_ERR:\n> \t\t\t\tbreak;\n> \t\t\t}\n> @@ -3435,7 +3435,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n> \n> \t/* Acquire all locks while verifying old values */\n> \tfor (i = 0; i < n; i++) {\n> -\t\tlocks[i] = update_ref_lock(updates[i]->ref_name,\n> +\t\tlocks[i] = update_ref_lock(updates[i]->refname,\n> \t\t\t\t\t (updates[i]->have_old ?\n> \t\t\t\t\t updates[i]->old_sha1 : NULL),\n> \t\t\t\t\t updates[i]->flags,\n> @@ -3450,7 +3450,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n> \tfor (i = 0; i < n; i++)\n> \t\tif (!is_null_sha1(updates[i]->new_sha1)) {\n> \t\t\tret = update_ref_write(msg,\n> -\t\t\t\t\t updates[i]->ref_name,\n> +\t\t\t\t\t updates[i]->refname,\n> \t\t\t\t\t updates[i]->new_sha1,\n> \t\t\t\t\t locks[i], onerr);\n> \t\t\tlocks[i] = NULL; /* freed by update_ref_write */\n> diff --git a/refs.h b/refs.h\n> index 99c194b..30ee721 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -154,7 +154,7 @@ extern void unlock_ref(struct ref_lock *lock);\n> extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);\n> \n> /** Setup reflog before using. **/\n> -int log_ref_setup(const char *ref_name, char *logfile, int bufsize);\n> +int log_ref_setup(const char *refname, char *logfile, int bufsize);\n> \n> /** Reads log for the value of ref during at_time. **/\n> extern int read_ref_at(const char *refname, unsigned long at_time, int cnt,\n"},{"id":"238258","messageId":"xmqqd2h0zuyb.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"1395683820-17304-24-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 23/27] struct ref_update: Store refname as a FLEX_ARRAY.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-01T19:54:52Z","receivedAt":"2014-04-01T19:54:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Signed-off-by: Michael Haggerty \n> ---\n> refs.c | 15 ++++++---------\n> 1 file changed, 6 insertions(+), 9 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index d72d0ab..2b80f6d 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3274,11 +3274,11 @@ static int update_ref_write(const char *action, const char *refname,\n> * value or to zero to ensure the ref does not exist before update.\n> */\n> struct ref_update {\n> -\tconst char *refname;\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> +\tconst char refname[FLEX_ARRAY];\n> };\n\nYeah, as we no longer borrow a pointer but make our own copy since\nthe earlier patch in the series, this perfectly makes sense.\n\n> \n> /*\n> @@ -3301,12 +3301,8 @@ static void ref_transaction_free(struct ref_transaction *transaction)\n> {\n> \tint i;\n> \n> -\tfor (i = 0; i < transaction->nr; i++) {\n> -\t\tstruct ref_update *update = transaction->updates[i];\n> -\n> -\t\tfree((char *)update->refname);\n> -\t\tfree(update);\n> -\t}\n> +\tfor (i = 0; i < transaction->nr; i++)\n> +\t\tfree(transaction->updates[i]);\n> \n> \tfree(transaction->updates);\n> \tfree(transaction);\n> @@ -3320,9 +3316,10 @@ void ref_transaction_rollback(struct ref_transaction *transaction)\n> static struct ref_update *add_update(struct ref_transaction *transaction,\n> \t\t\t\t const char *refname)\n> {\n> -\tstruct ref_update *update = xcalloc(1, sizeof(*update));\n> +\tsize_t len = strlen(refname);\n> +\tstruct ref_update *update = xcalloc(1, sizeof(*update) + len + 1);\n> \n> -\tupdate->refname = xstrdup(refname);\n> +\tstrcpy((char *)update->refname, refname);\n> \tALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);\n> \ttransaction->updates[transaction->nr++] = update;\n> \treturn update;\n"},{"id":"238263","messageId":"xmqq8urozuk0.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"1395683820-17304-27-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 26/27] struct ref_update: Add type field","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-01T20:03:27Z","receivedAt":"2014-04-01T20:03:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> This is temporary space for ref_transaction_commit().\n>\n> Signed-off-by: Michael Haggerty \n> ---\n\nI was about to complain to \"*Add* type\" that does not say what it is\nused for at all, with \"Please do not add something for unknown purpose\nonly to utilise it in a later patch\".\n\nBut that was before I noticed that these are already used and\nrealized that the change is about \"moving what is recorded in the\ntype array, which is used to receive the existing reftype discovered\nby calling resolve_ref_unsafe() in ref_transaction_commit() and not\nused anywhere else, to a field of individual ref_update structure\".\n\nSo it was somewhat of a \"Huh?\", but perhaps it is OK.\n\nI wonder if ref-transaction-commit can shrink its parameter list by\naccepting a single pointer to one ref_update?\n\n> refs.c | 8 +++-----\n> 1 file changed, 3 insertions(+), 5 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index d1edd57..07f900a 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3279,6 +3279,7 @@ struct ref_update {\n> \tint flags; /* REF_NODEREF? */\n> \tint have_old; /* 1 if old_sha1 is valid, 0 otherwise */\n> \tstruct ref_lock *lock;\n> +\tint type;\n> \tconst char refname[FLEX_ARRAY];\n> };\n> \n> @@ -3410,7 +3411,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n> {\n> \tint ret = 0, delnum = 0, i;\n> \tstruct ref_update **updates;\n> -\tint *types;\n> \tconst char **delnames;\n> \tint n = transaction->nr;\n> \n> @@ -3419,7 +3419,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n> \n> \t/* Allocate work space */\n> \tupdates = xmalloc(sizeof(*updates) * n);\n> -\ttypes = xmalloc(sizeof(*types) * n);\n> \tdelnames = xmalloc(sizeof(*delnames) * n);\n> \n> \t/* Copy, sort, and reject duplicate refs */\n> @@ -3437,7 +3436,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n> \t\t\t\t\t (update->have_old ?\n> \t\t\t\t\t\tupdate->old_sha1 : NULL),\n> \t\t\t\t\t update->flags,\n> -\t\t\t\t\t &types[i], onerr);\n> +\t\t\t\t\t &update->type, onerr);\n> \t\tif (!update->lock) {\n> \t\t\tret = 1;\n> \t\t\tgoto cleanup;\n> @@ -3465,7 +3464,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n> \n> \t\tif (update->lock) {\n> \t\t\tdelnames[delnum++] = update->lock->ref_name;\n> -\t\t\tret |= delete_ref_loose(update->lock, types[i]);\n> +\t\t\tret |= delete_ref_loose(update->lock, update->type);\n> \t\t}\n> \t}\n> \n> @@ -3479,7 +3478,6 @@ cleanup:\n> \t\tif (updates[i]->lock)\n> \t\t\tunlock_ref(updates[i]->lock);\n> \tfree(updates);\n> -\tfree(types);\n> \tfree(delnames);\n> \tref_transaction_free(transaction);\n> \treturn ret;\n"},{"id":"238298","messageId":"533B98A0.7030305@alum.mit.edu","threadId":"36283","inReplyTo":"xmqqtxaczvod.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 19/27] refs: Add a concept of a reference transaction","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-02T04:57:04Z","receivedAt":"2014-04-02T04:57:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/01/2014 09:39 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> Build out the API for dealing with a bunch of reference checks and\n>> changes within a transaction. Define an opaque ref_transaction type\n>> that is managed entirely within refs.c. Introduce functions for\n>> beginning a transaction, adding updates to a transaction, and\n>> committing/rolling back a transaction.\n>>\n>> This API will soon replace update_refs().\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n>> refs.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n>> refs.h | 65 +++++++++++++++++++++++++++++++++++++++++++++\n>> 2 files changed, 161 insertions(+)\n>>\n>> diff --git a/refs.c b/refs.c\n>> index 1305eb1..e788c27 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -3267,6 +3267,93 @@ static int update_ref_write(const char *action, const char *refname,\n>> \treturn 0;\n>> }\n>> \n>> +/*\n>> + * Data structure for holding a reference transaction, which can\n>> + * consist of checks and updates to multiple references, carried out\n>> + * as atomically as possible. This structure is opaque to callers.\n>> + */\n>> +struct ref_transaction {\n>> +\tstruct ref_update **updates;\n> \n> Don't we try to name an array update[] (not plural updates[]) so\n> that we can say update[7] to mean the seventh update?\n\nI know that some people prefer to name their arrays using singular, but\nI wasn't aware that this is a project rule. If it is, I think it is a\nbad rule. If followed, it would basically rule out the use of plural\nnouns as identifiers, and why would we want to deprive ourselves of the\nability to use the singular/plural distinction to help clarify our code?\n\nI like to use plural names to make it clear that an identifier refers to\na collection of objects. This leaves the singular noun available to\nrefer to single objects taken out of the aggregate like\n\n for (i = 0; i < nr; i++) {\n struct ref_update *update = updates[i];\n /* ... work with update... */\n }\n\nThe singular/plural distinction can also be used to give a good hint\nabout a pointer: does it point at a single object, or does it point at\nthe start of an array, linked list, etc? This convention is especially\nuseful in C, given that C declarations mostly don't distinguish between\npointers and arrays.\n\nIn SQL, the \"name aggregates using singular noun\" convention makes\nsense. SQL table names appear directly in expressions, and there is no\nneed to \"assign\" a record to an iteration variable. I suspect that this\nconvention, sensible in SQL, has been carried over to traditional\nprogramming languages where it is not (IMHO) sensible.\n\nBut...you're the maintainer. If you would like to make this a\nCodingGuidelines-level policy, I will of course conform to it.\n\n>> +\tsize_t alloc;\n>> +\tsize_t nr;\n>> +};\n>> +\n>> +struct ref_transaction *ref_transaction_begin(void)\n>> +{\n>> +\treturn xcalloc(1, sizeof(struct ref_transaction));\n>> +}\n>> +\n>> +static void ref_transaction_free(struct ref_transaction *transaction)\n>> +{\n>> +\tint i;\n>> +\n>> +\tfor (i = 0; i < transaction->nr; i++) {\n>> +\t\tstruct ref_update *update = transaction->updates[i];\n>> +\n>> +\t\tfree((char *)update->ref_name);\n>> +\t\tfree(update);\n>> +\t}\n>> +\n>> +\tfree(transaction->updates);\n>> +\tfree(transaction);\n>> +}\n> \n> OK.\n> \n>> +void ref_transaction_rollback(struct ref_transaction *transaction)\n>> +{\n>> +\tref_transaction_free(transaction);\n>> +}\n> \n> OK.\n> \n>> +static struct ref_update *add_update(struct ref_transaction *transaction,\n>> +\t\t\t\t const char *refname)\n>> +{\n>> +\tstruct ref_update *update = xcalloc(1, sizeof(*update));\n>> +\n>> +\tupdate->ref_name = xstrdup(refname);\n>> +\tALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);\n>> +\ttransaction->updates[transaction->nr++] = update;\n>> +\treturn update;\n>> +}\n>> +\n>> +void ref_transaction_update(struct ref_transaction *transaction,\n>> +\t\t\t const char *refname,\n>> +\t\t\t unsigned char *new_sha1, unsigned char *old_sha1,\n>> +\t\t\t int flags, int have_old)\n>> +{\n>> +\tstruct ref_update *update = add_update(transaction, refname);\n>> +\n>> +\thashcpy(update->new_sha1, new_sha1);\n>> +\tupdate->flags = flags;\n>> +\tupdate->have_old = have_old;\n>> +\tif (have_old)\n>> +\t\thashcpy(update->old_sha1, old_sha1);\n>> +}\n>> +\n>> +void ref_transaction_create(struct ref_transaction *transaction,\n>> +\t\t\t const char *refname,\n>> +\t\t\t unsigned char *new_sha1,\n>> +\t\t\t int flags)\n>> +{\n>> +\tstruct ref_update *update = add_update(transaction, refname);\n>> +\n>> +\thashcpy(update->new_sha1, new_sha1);\n>> +\thashclr(update->old_sha1);\n>> +\tupdate->flags = flags;\n>> +\tupdate->have_old = 1;\n>> +}\n>> +\n>> +void ref_transaction_delete(struct ref_transaction *transaction,\n>> +\t\t\t const char *refname,\n>> +\t\t\t unsigned char *old_sha1,\n>> +\t\t\t int flags, int have_old)\n>> +{\n>> +\tstruct ref_update *update = add_update(transaction, refname);\n>> +\n>> +\tupdate->flags = flags;\n>> +\tupdate->have_old = have_old;\n>> +\tif (have_old)\n>> +\t\thashcpy(update->old_sha1, old_sha1);\n>> +}\n>> +\n> \n> I can see that the chosen set of primitives update/create/delete\n> mirrors what update-ref allows us to do, but given the explanation\n> of \"update\" in refs.h, wouldn't it make more sense to implement the\n> others in terms of it?\n\nI didn't want to put a lot of thought and refactoring into the\nimplementation yet, because I still have plans to change ref_update but\nI haven't yet had time to decide how. The two possibilities that I am\nconsidering are:\n\n1. Change ref_update to have not just have_old but also a have_new\nboolean attribute (perhaps inserting both into the flags field). This\nwould allow a neater coding of all of the elementary operations,\nincluding \"verify\", which currently is implemented as \"update\n$EXPECTED_VALUE to $EXPECTED_VALUE\", which is a bit silly.\n\n2. Simplify each ref_update object to do only one operation: either\nverify an old value, or set a new value. It would then only need one\nSHA-1 field. A new flag bit would tell whether the old or new value is\nbeing operated on, and no \"have_old\" attribute would be needed at all.\nThis approach would require a general \"update \" to\nbe decomposed into two ref_update records, but many other operations\nwould only require one. If I go this route, then it would make more\nsense to implement ref_transaction_update() and the others in terms of a\n\"verify\" and/or a \"set\" operation.\n\nSo this patch series has mostly focused on putting a new API between\nusers and refs.c and I'd rather put off this decision.\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>> @@ -3378,6 +3465,15 @@ cleanup:\n>> \treturn ret;\n>> }\n>> \n>> +int ref_transaction_commit(struct ref_transaction *transaction,\n>> +\t\t\t const char *msg, enum action_on_err onerr)\n>> +{\n>> +\tint ret = update_refs(msg, transaction->updates, transaction->nr,\n>> +\t\t\t onerr);\n>> +\tref_transaction_free(transaction);\n>> +\treturn ret;\n>> +}\n> \n> OK.\n> \n>> char *shorten_unambiguous_ref(const char *refname, int strict)\n>> {\n>> \tint i;\n>> diff --git a/refs.h b/refs.h\n>> index 08e60ac..476a923 100644\n>> --- a/refs.h\n>> +++ b/refs.h\n>> @@ -24,6 +24,8 @@ struct ref_update {\n>> \tint have_old; /* 1 if old_sha1 is valid, 0 otherwise */\n>> };\n>> \n>> +struct ref_transaction;\n>> +\n>> /*\n>> * Bit values set in the flags argument passed to each_ref_fn():\n>> */\n>> @@ -220,6 +222,69 @@ enum action_on_err {\n>> \tUPDATE_REFS_QUIET_ON_ERR\n>> };\n>> \n>> +/*\n>> + * Begin a reference transaction. The reference transaction must\n>> + * eventually be commited using ref_transaction_commit() or rolled\n>> + * back using ref_transaction_rollback().\n>> + */\n>> +struct ref_transaction *ref_transaction_begin(void);\n>> +\n>> +/*\n>> + * Roll back a ref_transaction and free all associated data.\n>> + */\n>> +void ref_transaction_rollback(struct ref_transaction *transaction);\n>> +\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, so the caller retains ownership of the parameter. flags\n> \n> Good to see the ownership rules described.\n> \n>> + * can be REF_NODEREF; it is passed to update_ref_lock().\n>> + */\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 zeros 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>> + * it must not have existed beforehand.\n>> + */\n>> +void ref_transaction_update(struct ref_transaction *transaction,\n>> +\t\t\t const char *refname,\n>> +\t\t\t unsigned char *new_sha1, unsigned char *old_sha1,\n>> +\t\t\t int flags, int have_old);\n>> +\n>> +/*\n>> + * Add a reference creation to transaction. new_sha1 is the value\n>> + * that the reference should have after the update, or zeros if it\n>> + * should be deleted. It is verified that the reference does not\n>> + * exist already.\n>> + */\n> \n> Sounds a bit crazy that you can ask \"create\", which verifies the\n> absense of the thing, to delete a thing.\n\nYes, you are right. I can't remember why I defined it this way. Maybe\nI just wanted to avoid the extra call to is_null_sha1() as a consistency\ncheck? In any case, I'll change \"create\" to require a nonzero new\nvalue. I think an assert() will be adequate, since it would be a\nprogramming error to call it with a zero hash.\n\nBy analogy, ref_transaction_delete() should insist that if have_old is\nset, then old_sha1 is not zeros. I will make that change as well.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238330","messageId":"533B9A26.8050303@alum.mit.edu","threadId":"36283","inReplyTo":"xmqqppl0zvcs.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-02T05:03:34Z","receivedAt":"2014-04-02T05:03:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/01/2014 09:46 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> This change is mostly clerical: the parse_cmd_*() functions need to\n>> use local variables rather than a struct ref_update to collect the\n>> arguments needed for each update, and then call ref_transaction_*() to\n>> queue the change rather than building up the list of changes at the\n>> caller side.\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n> \n> With the implementation of ref_transaction at this point in the\n> series it does not matter, but the updated code in this patch means\n> that it is perfectly acceptable to do this sequence:\n> \n> ref_transaction_begin();\n> ref_transaction_update();\n> ...\n> ref_transaction_update();\n> die();\n> \n> without ever calling ref_transaction_rollback() API function.\n> Depending on the future backends, we may want to ensure rollback is\n> called, no? And if that is the case, we would want to prepare\n> callers of the API with some at-exit facility to call rollback, no?\n\nI assumed that rolling back a non-consummated transaction in the case of\nearly program death should be the responsibility of the library, not of\nthe caller. If I'm correct, the caller(s) won't have to be modified\nwhen the atexit facility is added, so I don't see a reason to add it\nbefore it is needed by a concrete backend.\n\nBut you suggest that the caller should be involved. Do you have an idea\nfor something that a caller might want to do besides roll back the\ntransaction?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238337","messageId":"533B9BED.4050509@alum.mit.edu","threadId":"36283","inReplyTo":"xmqqha6czuzu.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 22/27] struct ref_update: Rename field \"ref_name\" to \"refname\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-02T05:11:09Z","receivedAt":"2014-04-02T05:11:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/01/2014 09:53 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> This is consistent with the usual nomenclature.\n> \n> I am of two minds.\n> \n> Looking for \"\\(\\.\\|->\\)ref_name\" used to ignore refname fields of\n> other structures and let us focus on the ref_update structure. Yes,\n> there is the ref_lock structure that shares ref_name to contaminate\n> such a grep output already, but this change makes the output even\n> more noisy, as you have to now look for \"\\(\\.\\|->\\)refname\" which\n> would give you more hits from other unrelated structures.\n> \n> On the other hand, I do not like to name this to \"update_refname\" or\n> some nonsense like that, of course. A reference name field in a\n> \"ref_update\" structure shouldn't have to say that it is about\n> updating in its name; it should be known from the name of the\n> structure it appears in.\n> \n> So I dunno.\n\nI prefer naming consistency but whatever.\n\nWhen I want to find all users of a common identifier, I usually rename\nthe identifier at its declaration (e.g., to \"refnameXXX\") and see where\ngcc flags errors. Or if I'm doing a lot of this sort of thing, I might\neven fire up Eclipse, which does a pretty good job of finding instances\nof a particular identifier throughout the code.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238301","messageId":"533BE2C5.7050700@alum.mit.edu","threadId":"36283","inReplyTo":"xmqq8urozuk0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 26/27] struct ref_update: Add type field","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-02T10:13:25Z","receivedAt":"2014-04-02T10:13:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/01/2014 10:03 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> This is temporary space for ref_transaction_commit().\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n> \n> I was about to complain to \"*Add* type\" that does not say what it is\n> used for at all, with \"Please do not add something for unknown purpose\n> only to utilise it in a later patch\".\n> \n> But that was before I noticed that these are already used and\n> realized that the change is about \"moving what is recorded in the\n> type array, which is used to receive the existing reftype discovered\n> by calling resolve_ref_unsafe() in ref_transaction_commit() and not\n> used anywhere else, to a field of individual ref_update structure\".\n> \n> So it was somewhat of a \"Huh?\", but perhaps it is OK.\n\nI will expand the comment in v3.\n\n> I wonder if ref-transaction-commit can shrink its parameter list by\n> accepting a single pointer to one ref_update?\n\nI don't understand this last point. ref_transaction_commit() has the\nfollowing signature:\n\nint ref_transaction_commit(struct ref_transaction *transaction,\n\t\t\t const char *msg, enum action_on_err onerr)\n\nWhat change are you proposing?\n\nBy the way, longer-term, I wonder if msg and maybe action_on_err should\nbe set for each ref_update, rather than for a whole transaction.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238313","messageId":"xmqq61mry9ee.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"533A86F2.90508@alum.mit.edu","subject":"Re: [PATCH v2 18/27] update-ref --stdin: Harmonize error messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-02T16:38:01Z","receivedAt":"2014-04-02T16:38:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Junio, I incorporated your feedback (which so far has only affected\n> commit messages). I also rebased the patch series to the current\n> master. I pushed the result to GitHub [1]. I'll refrain from spamming\n> the list with v3 yet.\n\nThanks; let us know when you are ready ;-) I finished reading the\nremainder of the v2, and I think I sent out what I found worth\ncommenting on (either positive or negative).\n\nI think the next thing to convert to the transaction API would be\nthe \"ok we know the set of updates from the pusher; let's update all\nof them\" in receive-pack? In a sense that is of a lot more\nreal-world impact than the update-ref plumbing. \n\n Side note: honestly speaking, I was dissapointed to see\n that the ref updates by the receive-pack process was not\n included in the series when I saw the cover letter that\n said this was a series about transactional updates to\n refs. Anyway...\n\nThere are a few things that need to be thought through.\n\nMaking the update in receive-pack all-or-none is a behaviour change,\neven though it may be a good one. We may want to allow the user a\nway to ask for the traditional \"reject only the ones that cannot be\nupdated\". It probably goes like this:\n\n - On the wire, a new \"ref-update-aon\" capability is\n advertised from receive-pack to send-pack and can be requested in\n the opposite direction.\n\n - On the \"git push\" side, a new \"--all-or-none\" option, and\n optionally a new \"push.allOrNone\" configuration, is used to\n request the \"ref-update-aon\" capability over the wire.\n\n - On the receive-pack side, a new \"receive.allOrNone\" configuration \n can be used to always update refs in all-or-none fashion, no\n matter what the pusher says.\n\n - The receive-pack uses the ref transaction to update the refs in\n all-or-none fashion if it has receive.allOrNone, or both sides\n agree to use ref-update-aon in the capability exchange. If not,\n it updates the refs in some-may-succeed-some-may-fail fashion,\n one by one.\n\nOr something like that.\n"},{"id":"238318","messageId":"xmqqbnwjvd68.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"xmqq8urozuk0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 26/27] struct ref_update: Add type field","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-02T17:44:47Z","receivedAt":"2014-04-02T17:44:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I wonder if ref-transaction-commit can shrink its parameter list by\n> accepting a single pointer to one ref_update?\n\nDisregard this one. I was fooled into thinking that the function is\ncalled with parameters such as update->old_sha1, update_flags,\nupdate->type when looking at the hunk starting at l.3437; the called\nfunction there is not ref-transaction-commit.\n\nSorry, and thanks.\n\n>> @@ -3437,7 +3436,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n>> \t\t\t\t\t (update->have_old ?\n>> \t\t\t\t\t\tupdate->old_sha1 : NULL),\n>> \t\t\t\t\t update->flags,\n>> -\t\t\t\t\t &types[i], onerr);\n>> +\t\t\t\t\t &update->type, onerr);\n>> \t\tif (!update->lock) {\n>> \t\t\tret = 1;\n>> \t\t\tgoto cleanup;\n"},{"id":"238353","messageId":"xmqq61mqs8wl.fsf@gitster.dls.corp.google.com","threadId":"36283","inReplyTo":"533B9A26.8050303@alum.mit.edu","subject":"Re: [PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-03T15:57:30Z","receivedAt":"2014-04-03T15:57:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> I assumed that rolling back a non-consummated transaction in the case of\n> early program death should be the responsibility of the library, not of\n> the caller. If I'm correct, the caller(s) won't have to be modified\n> when the atexit facility is added, so I don't see a reason to add it\n> before it is needed by a concrete backend.\n>\n> But you suggest that the caller should be involved.\n\nI didn't say \"should\". If the library can automatically rollback\nwithout being called upon die() anywhere in the system, that is\nbetter. The suggestion was because I didn't think you were shooting\nfor such a completeness in the library part, and a possible way out\nis for the caller to help.\n"},{"id":"238368","messageId":"533E3CF8.5080506@alum.mit.edu","threadId":"36283","inReplyTo":"xmqq61mqs8wl.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-04T05:02:48Z","receivedAt":"2014-04-04T05:02:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/03/2014 05:57 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> I assumed that rolling back a non-consummated transaction in the case of\n>> early program death should be the responsibility of the library, not of\n>> the caller. If I'm correct, the caller(s) won't have to be modified\n>> when the atexit facility is added, so I don't see a reason to add it\n>> before it is needed by a concrete backend.\n>>\n>> But you suggest that the caller should be involved.\n> \n> I didn't say \"should\". If the library can automatically rollback\n> without being called upon die() anywhere in the system, that is\n> better. The suggestion was because I didn't think you were shooting\n> for such a completeness in the library part, and a possible way out\n> is for the caller to help.\n\nI was assuming that any ref backends that required rollback-on-fail\nwould register an atexit handler and a signal handler, similar to how\nlock_file rollbacks are done.\n\nI admit that I haven't thought through all the details; for example, are\nthere restrictions on the things that a signal handler is allowed to do\nthat would preclude its being able to rollback the types of transactions\nthat back ends might want to implement? (Though if so, what hope do we\nhave that the caller can do better?)\n\nSo, if somebody can think of a reason that we would need to involve the\ncaller in cleanup, please speak up. Otherwise I think it would be less\nerror-prone to leave this responsibility with the individual back ends.\n (And if something unexpected comes up, we can make this change later.)\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}