{"thread":{"id":"36358","subject":"[PATCH v3 00/27] Clean up update-refs --stdin and implement ref_transaction","startedAt":"2014-04-07T13:47:51Z","lastAt":"2014-04-15T07:40:56Z","messageCount":34,"participants":["Michael Haggerty","Junio C Hamano","Ronnie Sahlberg"],"isPatch":true,"patchVersion":3,"patchTotal":27},"messages":[{"id":"238442","messageId":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":null,"subject":"[PATCH v3 00/27] Clean up update-refs --stdin and implement ref_transaction","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:47:51Z","receivedAt":"2014-04-07T13:47:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Here is v3. It is also available on GitHub [1].\n\nThanks to Junio and Brad for their comments about v2. I think I have\naddressed all of your comments (except for Junio's regrets that the\nseries didn't include a transactional receive-pack).\n\nSee the mailing list threads about v1 [2] and v2 [3] and the\nlonger-term goals of this campaign [4].\n\nChanges since v2:\n\n* Rebased to current master (there were no conflicts)\n\n* Don't allow ref_transation_create() with new_sha1 set to zeros.\n\n* Don't allow ref_transation_delete() with old_sha1 set to zeros.\n\n* Fixed subject lines to use lower-case after the colon.\n\n* Expanded a few commit messages, fixed a comment, and removed some\n \"squash\" detritus in a commit message.\n\nCheers,\nMichael\n\n[1] https://github.com/mhagger/git, branch ref-transactions\n[2] http://thread.gmane.org/gmane.comp.version-control.git/243731\n[3] http://thread.gmane.org/gmane.comp.version-control.git/244857\n[4] http://article.gmane.org/gmane.comp.version-control.git/243726\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(): simplify code using temporary variables\n struct ref_update: add a lock field\n struct ref_update: add a 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 | 195 +++++++++++----\n refs.h | 94 ++++++--\n t/t1400-update-ref.sh | 100 +++++---\n 13 files changed, 585 insertions(+), 284 deletions(-)\n\n-- \n1.9.1\n"},{"id":"238443","messageId":"1396878498-19887-2-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 01/27] t1400: fix name and expected result of one test","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:47:52Z","receivedAt":"2014-04-07T13:47:52Z","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, because although \"git\nupdate-ref\" tries to distinguish between these two errors, it does not\nsucceed in this situation. Fixing it is more than a one-liner, so\nmark the test test_expect_failure for now. The failure will be fixed\nlater in this patch series.\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.1\n"},{"id":"238445","messageId":"1396878498-19887-3-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 02/27] t1400: provide more usual input to the command","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:47:53Z","receivedAt":"2014-04-07T13:47:53Z","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.1\n"},{"id":"238444","messageId":"1396878498-19887-4-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 03/27] parse_arg(): really test that argument is properly terminated","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:47:54Z","receivedAt":"2014-04-07T13:47:54Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The 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\nthe *next* argument that a problem was noticed. But in fact, the\ndefinition of the input format requires arguments to be terminated by\nSP or NUL, so *this* argument is already erroneous and parse_arg()\nshould diagnose the problem.\n\nSo teach parse_arg() to verify that C-quoted arguments are terminated\ncorrectly. If not, emit a more specific error message.\n\nThere is no corresponding error case of a non-C-quoted argument that\nis not terminated correctly, because the end of a non-quoted argument\nis *by definition* a space or NUL, so there is no way to insert other\njunk between the \"end\" of the argument and the argument terminator.\n\nAdjust the tests to expect the new error message. Add a docstring to\nthe function, incorporating the comments that were formerly within the\nfunction 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.1\n"},{"id":"238446","messageId":"1396878498-19887-5-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 04/27] t1400: add some more tests involving quoted arguments","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:47:55Z","receivedAt":"2014-04-07T13:47:55Z","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 1b86d9c..6bf2318 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 9b3c04d..b12989d 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 e15d0e1..7d1d83e 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.1\n"},{"id":"238449","messageId":"1396878498-19887-7-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 06/27] update_refs(): fix constness","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:47:57Z","receivedAt":"2014-04-07T13:47:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The old signature of update_refs() required a\n(const struct ref_update **) for its updates_orig argument. The\n\"const\" is presumably there to promise that the function will not\nmodify the contents of the structures.\n\nBut this declaration does not permit the function to be called with a\n(struct ref_update **), which is perfectly legitimate. C's type\nsystem is not powerful enough to express what we'd like. So remove\nthe first \"const\" from the declaration.\n\nOn the other hand, the function *can* promise not to modify the\npointers within the array that is passed to it without inconveniencing\nits callers. So add a \"const\" that has that effect, making the final\ndeclaration\n(struct ref_update * const *).\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.1\n"},{"id":"238450","messageId":"1396878498-19887-8-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 07/27] update-ref --stdin: read the whole input at once","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:47:58Z","receivedAt":"2014-04-07T13:47:58Z","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.1\n"},{"id":"238448","messageId":"1396878498-19887-9-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 08/27] parse_cmd_verify(): copy old_sha1 instead of evaluating twice","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:47:59Z","receivedAt":"2014-04-07T13:47:59Z","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.1\n"},{"id":"238451","messageId":"1396878498-19887-10-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 09/27] update-ref.c: extract a new function, parse_refname()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:00Z","receivedAt":"2014-04-07T13:48:00Z","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.1\n"},{"id":"238469","messageId":"1396878498-19887-11-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 10/27] update-ref --stdin: improve error messages for invalid values","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:01Z","receivedAt":"2014-04-07T13:48:01Z","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.1\n"},{"id":"238468","messageId":"1396878498-19887-12-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 11/27] update-ref --stdin: make error messages more consistent","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:02Z","receivedAt":"2014-04-07T13:48:02Z","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.1\n"},{"id":"238452","messageId":"1396878498-19887-13-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 12/27] update-ref --stdin: simplify error messages for missing oldvalues","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:03Z","receivedAt":"2014-04-07T13:48:03Z","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.1\n"},{"id":"238467","messageId":"1396878498-19887-14-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 13/27] t1400: test that stdin -z update treats empty as zeros","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:04Z","receivedAt":"2014-04-07T13:48:04Z","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.1\n"},{"id":"238454","messageId":"1396878498-19887-15-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 14/27] update-ref.c: extract a new function, parse_next_sha1()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:05Z","receivedAt":"2014-04-07T13:48:05Z","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..c61120f 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 update'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.1\n"},{"id":"238466","messageId":"1396878498-19887-16-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 15/27] update-ref --stdin -z: deprecate interpreting the empty string as zeros","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:06Z","receivedAt":"2014-04-07T13:48:06Z","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 c61120f..6f3b909 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.1\n"},{"id":"238453","messageId":"1396878498-19887-17-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 16/27] t1400: test one mistake at a time","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:07Z","receivedAt":"2014-04-07T13:48:07Z","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---\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.1\n"},{"id":"238455","messageId":"1396878498-19887-18-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 17/27] update-ref --stdin: improve the error message for unexpected EOF","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:08Z","receivedAt":"2014-04-07T13:48:08Z","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 6f3b909..0d5f1d0 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.1\n"},{"id":"238465","messageId":"1396878498-19887-19-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 18/27] update-ref --stdin: harmonize error messages","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:09Z","receivedAt":"2014-04-07T13:48:09Z","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 0d5f1d0..423c5c3 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.1\n"},{"id":"238456","messageId":"1396878498-19887-20-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 19/27] refs: add a concept of a reference transaction","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:10Z","receivedAt":"2014-04-07T13:48:10Z","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 | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n refs.h | 65 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 164 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex 1305eb1..f0b5764 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3267,6 +3267,96 @@ 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+\tassert(!is_null_sha1(new_sha1));\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\tassert(!is_null_sha1(old_sha1));\n+\t\thashcpy(update->old_sha1, old_sha1);\n+\t}\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 +3468,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..0518dd5 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; it must not be the\n+ * null SHA-1. It is verified that the reference does not exist\n+ * 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 (which must not be the null SHA-1).\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.1\n"},{"id":"238464","messageId":"1396878498-19887-21-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 20/27] update-ref --stdin: reimplement using reference transactions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:11Z","receivedAt":"2014-04-07T13:48:11Z","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 423c5c3..405267f 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.1\n"},{"id":"238462","messageId":"1396878498-19887-22-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 21/27] refs: remove API function update_refs()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:12Z","receivedAt":"2014-04-07T13:48:12Z","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 f0b5764..6984ff0 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@@ -3396,16 +3410,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@@ -3415,7 +3430,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@@ -3437,7 +3452,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@@ -3465,14 +3480,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 0518dd5..cb799a3 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.1\n"},{"id":"238463","messageId":"1396878498-19887-23-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 22/27] struct ref_update: rename field \"ref_name\" to \"refname\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:13Z","receivedAt":"2014-04-07T13:48:13Z","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 6984ff0..b6778aa 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@@ -3386,7 +3386,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@@ -3394,14 +3394,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@@ -3438,7 +3438,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@@ -3453,7 +3453,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 cb799a3..0f08def 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.1\n"},{"id":"238458","messageId":"1396878498-19887-24-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 23/27] struct ref_update: store refname as a FLEX_ARRAY","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:14Z","receivedAt":"2014-04-07T13:48:14Z","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 b6778aa..2ff195f 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.1\n"},{"id":"238457","messageId":"1396878498-19887-25-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 24/27] ref_transaction_commit(): simplify code using temporary variables","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:15Z","receivedAt":"2014-04-07T13:48:15Z","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 2ff195f..33c34df 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3435,10 +3435,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@@ -3447,16 +3449,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.1\n"},{"id":"238461","messageId":"1396878498-19887-26-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 25/27] struct ref_update: add a lock field","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:16Z","receivedAt":"2014-04-07T13:48:16Z","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 33c34df..6fe4bfe8 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@@ -3413,7 +3414,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@@ -3423,7 +3423,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@@ -3437,12 +3436,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@@ -3456,19 +3455,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@@ -3476,11 +3479,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.1\n"},{"id":"238460","messageId":"1396878498-19887-27-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 26/27] struct ref_update: add a type field","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:17Z","receivedAt":"2014-04-07T13:48:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It used to be that ref_transaction_commit() allocated a temporary\narray to hold the types of references while it is working. Instead,\nadd a type field to ref_update that ref_transaction_commit() can use\nas its scratch space.\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 6fe4bfe8..c058f30 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@@ -3413,7 +3414,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@@ -3422,7 +3422,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@@ -3440,7 +3439,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@@ -3468,7 +3467,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@@ -3482,7 +3481,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.1\n"},{"id":"238459","messageId":"1396878498-19887-28-git-send-email-mhagger@alum.mit.edu","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 27/27] ref_transaction_commit(): work with transaction->updates in place","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-07T13:48:18Z","receivedAt":"2014-04-07T13:48:18Z","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 c058f30..728a761 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3413,19 +3413,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@@ -3480,7 +3478,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.1\n"},{"id":"238499","messageId":"xmqq8urhlzr1.fsf@gitster.dls.corp.google.com","threadId":"36358","inReplyTo":"1396878498-19887-20-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v3 19/27] refs: add a concept of a reference transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-07T19:13:06Z","receivedAt":"2014-04-07T19:13:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\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> +\tassert(!is_null_sha1(new_sha1));\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\tassert(!is_null_sha1(old_sha1));\n> +\t\thashcpy(update->old_sha1, old_sha1);\n> +\t}\n> +}\n\nThese assert()s will often turn into no-op in production builds. If\nit is a bug in the code (i.e. the callers are responsible for\ncatching these conditions and issuing errors, and there are actually\nsuch checks implemented in the callers), it is fine to have these as\nassert()s, but otherwise these should be 'if (...) die(\"BUG:\")', I\nthink.\n\nOther than that, I did not spot anything questionable in this round.\n\nThanks; will replace the series (but on the same base as I needed to\napply the series there to compare what got changed with the old\nversion of corresponding change for each patches).\n"},{"id":"238740","messageId":"CAL=YDWkjNm-pt2dUsg3Dbtbb1Xi2v3XAP8231g1bxsWF6LHRhQ@mail.gmail.com","threadId":"36358","inReplyTo":"1396878498-19887-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v3 00/27] Clean up update-refs --stdin and implement ref_transaction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-11T19:57:42Z","receivedAt":"2014-04-11T19:57:42Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Nice.\n\nOnce this is in I can add transactional support to receive-pack.\n\nOn Mon, Apr 7, 2014 at 6:47 AM, Michael Haggerty wrote:\n> Here is v3. It is also available on GitHub [1].\n>\n> Thanks to Junio and Brad for their comments about v2. I think I have\n> addressed all of your comments (except for Junio's regrets that the\n> series didn't include a transactional receive-pack).\n>\n> See the mailing list threads about v1 [2] and v2 [3] and the\n> longer-term goals of this campaign [4].\n>\n> Changes since v2:\n>\n> * Rebased to current master (there were no conflicts)\n>\n> * Don't allow ref_transation_create() with new_sha1 set to zeros.\n>\n> * Don't allow ref_transation_delete() with old_sha1 set to zeros.\n>\n> * Fixed subject lines to use lower-case after the colon.\n>\n> * Expanded a few commit messages, fixed a comment, and removed some\n> \"squash\" detritus in a commit message.\n>\n> Cheers,\n> Michael\n>\n> [1] https://github.com/mhagger/git, branch ref-transactions\n> [2] http://thread.gmane.org/gmane.comp.version-control.git/243731\n> [3] http://thread.gmane.org/gmane.comp.version-control.git/244857\n> [4] http://article.gmane.org/gmane.comp.version-control.git/243726\n>\n> Michael 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(): simplify code using temporary variables\n> struct ref_update: add a lock field\n> struct ref_update: add a 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 | 195 +++++++++++----\n> refs.h | 94 ++++++--\n> t/t1400-update-ref.sh | 100 +++++---\n> 13 files changed, 585 insertions(+), 284 deletions(-)\n>\n> --\n> 1.9.1\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"238821","messageId":"534BBE67.3040100@alum.mit.edu","threadId":"36358","inReplyTo":"xmqq8urhlzr1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 19/27] refs: add a concept of a reference transaction","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T10:54:31Z","receivedAt":"2014-04-14T10:54:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/07/2014 09:13 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\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>> +\tassert(!is_null_sha1(new_sha1));\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\tassert(!is_null_sha1(old_sha1));\n>> +\t\thashcpy(update->old_sha1, old_sha1);\n>> +\t}\n>> +}\n> \n> These assert()s will often turn into no-op in production builds. If\n> it is a bug in the code (i.e. the callers are responsible for\n> catching these conditions and issuing errors, and there are actually\n> such checks implemented in the callers), it is fine to have these as\n> assert()s, but otherwise these should be 'if (...) die(\"BUG:\")', I\n> think.\n\nI forgot to confirm that the callers *do* verify that they don't pass\nincorrect values to ref_transaction_create() and\nref_transaction_delete(). But if they wouldn't, then die(\"BUG:\") would\nnot be appropriate either, would it? It would have to be die(\"you silly\nuser...\").\n\nAnother reason I'm comfortable with only having assert()s in this case\nis that even if the preconditions are not met, nothing really crazy\nhappens. If I were guarding against something nastier, like a buffer\noverflow, then I would more likely have used die(\"BUG:\") instead.\n\n\nIt is not material to this discussion, but I wonder how often production\nbuilds use NDEBUG. I just checked that Debian does not; i.e.,\nassertions are enabled for git there. Personally I wouldn't run code\nbuilt with NDEBUG unless building for a severely resource-constrained\ndevice, and even then I'd be pretty nervous about it.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238868","messageId":"xmqqa9bnmwnk.fsf@gitster.dls.corp.google.com","threadId":"36358","inReplyTo":"534BBE67.3040100@alum.mit.edu","subject":"Re: [PATCH v3 19/27] refs: add a concept of a reference transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-14T21:25:03Z","receivedAt":"2014-04-14T21:25:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> I forgot to confirm that the callers *do* verify that they don't pass\n> incorrect values to ref_transaction_create() and\n> ref_transaction_delete(). But if they wouldn't, then die(\"BUG:\") would\n> not be appropriate either, would it? It would have to be die(\"you silly\n> user...\").\n\nHaving the assert() there gives a confused message to the readers\n(at least it did to this reader).\n\nassert() implies that callers that are not buggy should not give\ninput that triggers the condition, which would mean it is the\ncallers' responsibility to sanity check the user-input to reject\ncreating a ref at 0{40} or deleting a ref whose old value is 0{40},\nwhich in turn means these input are errors that need to be diagnosed\nand documented.\n\nBut as you said below...\n\n> ... even if the preconditions are not met, nothing really crazy\n> happens.\n\nI agree that it also is perfectly fine to treat such input as\nnot-an-input-error.\n\nIt is a signal that these checks are not 'if (...) die()' that the\ncode may take that stance.\n\nI cannot tell which interpretation the code is trying to implement.\n\nAny one of the following I can understand:\n\n (1) drop the assert(), declaring that the user input is perfectly\n fine;\n\n (2) keep the assert(), reject such user input at the callers, and\n document that these are invalid inputs;\n\n (3) replace the assert() with 'if (...) die(\"you silly user...\")',\n and document that these are invalid inputs; or\n\n (4) same as (3) but replace with warn(), declaring such input as\n suspicious.\n\nbut the current assert() makes the code look \"cannot decide\" ;-).\n\nI would consider the first two more sensible than the other two, and\nam leaning slightly towards (1) over (2).\n\nThanks.\n"},{"id":"238874","messageId":"534CC69C.1020503@alum.mit.edu","threadId":"36358","inReplyTo":"xmqqa9bnmwnk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 19/27] refs: add a concept of a reference transaction","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-15T05:41:48Z","receivedAt":"2014-04-15T05:41:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/14/2014 11:25 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> I forgot to confirm that the callers *do* verify that they don't pass\n>> incorrect values to ref_transaction_create() and\n>> ref_transaction_delete(). But if they wouldn't, then die(\"BUG:\") would\n>> not be appropriate either, would it? It would have to be die(\"you silly\n>> user...\").\n> \n> Having the assert() there gives a confused message to the readers\n> (at least it did to this reader).\n> \n> assert() implies that callers that are not buggy should not give\n> input that triggers the condition, which would mean it is the\n> callers' responsibility to sanity check the user-input to reject\n> creating a ref at 0{40} or deleting a ref whose old value is 0{40},\n> which in turn means these input are errors that need to be diagnosed\n> and documented.\n\nIn v2 of this patch series, I didn't forbid calling create with new_sha\n== 0{40}, because, even though it's not what the user would think of as\ncreating a reference, the code didn't care--it would just verify that\nthe reference didn't exist before and then leave it undefined.\n\nThen in [1] you commented:\n\n> Sounds a bit crazy that you can ask \"create\", which verifies the\n> absense of the thing, to delete a thing.\n\nI reacted to your comment by changing the documentation to forbid\ncalling \"create\" with new_sha1 == 0{40}, and enforced the rule using an\nassert(). At the same time I added an analogous restriction that if\n\"delete\" is called with have_old set, then old_sha1 must not be 0{40}.\n\nIn retrospect, you might have been objecting more to the misleading\ndocstring than to the behavior as implemented at the time. The\ndocstring implied that create could actually be used to delete a\nreference, but that was not true: it always checked that the reference\ndidn't exist beforehand. So at worst it could leave a non-existent\nreference un-created. Sorry for the confusion.\n\n> But as you said below...\n> \n>> ... even if the preconditions are not met, nothing really crazy\n>> happens.\n> \n> I agree that it also is perfectly fine to treat such input as\n> not-an-input-error.\n\nThat was the case in v2.\n\n> It is a signal that these checks are not 'if (...) die()' that the\n> code may take that stance.\n> \n> I cannot tell which interpretation the code is trying to implement.\n> \n> Any one of the following I can understand:\n> \n> (1) drop the assert(), declaring that the user input is perfectly\n> fine;\n> \n> (2) keep the assert(), reject such user input at the callers, and\n> document that these are invalid inputs;\n> \n> (3) replace the assert() with 'if (...) die(\"you silly user...\")',\n> and document that these are invalid inputs; or\n> \n> (4) same as (3) but replace with warn(), declaring such input as\n> suspicious.\n> \n> but the current assert() makes the code look \"cannot decide\" ;-).\n> \n> I would consider the first two more sensible than the other two, and\n> am leaning slightly towards (1) over (2).\n\nThe current status in v3 is that (2) is implemented.\n\nObviously I don't feel strongly about it, given that I implemented (1)\nin v2. But I think that this restriction makes code at the calling site\neasier to understand: \"create\" now always means \"create\"; i.e., if the\ntransaction goes through, then the reference exists afterwards. And\n\"delete\" always means delete; i.e., afterwards, there is one less\nreference in the world.\n\nMichael\n\n[1] http://mid.gmane.org/xmqqtxaczvod.fsf@gitster.dls.corp.google.com\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238879","messageId":"xmqqk3arkpkn.fsf@gitster.dls.corp.google.com","threadId":"36358","inReplyTo":"534CC69C.1020503@alum.mit.edu","subject":"Re: [PATCH v3 19/27] refs: add a concept of a reference transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-15T07:40:56Z","receivedAt":"2014-04-15T07:40:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> In retrospect, you might have been objecting more to the misleading\n> docstring than to the behavior as implemented at the time. \n\nYeah, I was reacting to the comment that said create can delete ;-)\n\n> The\n> docstring implied that create could actually be used to delete a\n> reference, but that was not true: it always checked that the reference\n> didn't exist beforehand. So at worst it could leave a non-existent\n> reference un-created.\n\nOK, all is clear then. \n\n>> (2) keep the assert(), reject such user input at the callers, and\n>> document that these are invalid inputs;\n>\n> The current status in v3 is that (2) is implemented.\n\nGood. Thanks.\n"}]}