From b984d333a1367078c3631b8dfdd0d0ec7b332715 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:47:52 +0200 Subject: [PATCH 01/27] t1400: fix name and expected result of one test The test stdin -z create ref fails with zero new value actually passes an empty new value, not a zero new value. So rename the test s/zero/empty/, and change the expected error from fatal: create $c given zero new value to fatal: create $c missing Of course, this makes the test fail now, because although "git update-ref" tries to distinguish between these two errors, it does not succeed in this situation. Fixing it is more than a one-liner, so mark the test test_expect_failure for now. The failure will be fixed later in this patch series. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- t/t1400-update-ref.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 6ffd82fe32..fa927d2184 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -827,10 +827,10 @@ test_expect_success 'stdin -z create ref fails with bad new value' ' test_must_fail git rev-parse --verify -q $c ' -test_expect_success 'stdin -z create ref fails with zero new value' ' +test_expect_failure 'stdin -z create ref fails with empty new value' ' printf $F "create $c" "" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: create $c given zero new value" err && + grep "fatal: create $c missing " err && test_must_fail git rev-parse --verify -q $c ' From c13291108880f9adc26a2ab5c09535869afecb22 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:47:53 +0200 Subject: [PATCH 02/27] t1400: provide more usual input to the command The old version was passing (among other things) update SP refs/heads/c NUL NUL 0{40} NUL to "git update-ref -z --stdin" to test whether the old-value check for c is working. But the is empty, which is a bit off the beaten track. So, to be sure that we are testing what we want to test, provide an actual on the "update" line. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- t/t1400-update-ref.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index fa927d2184..29391c67fc 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -912,7 +912,7 @@ test_expect_success 'stdin -z update refs works with identity updates' ' test_expect_success 'stdin -z update refs fails with wrong old value' ' git update-ref $c $m && - printf $F "update $a" "$m" "$m" "update $b" "$m" "$m" "update $c" "" "$Z" >stdin && + printf $F "update $a" "$m" "$m" "update $b" "$m" "$m" "update $c" "$m" "$Z" >stdin && test_must_fail git update-ref -z --stdin err && grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err && git rev-parse $m >expect && From 697a41519b0d41c1b2c5714b5558f68814d78885 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:47:54 +0200 Subject: [PATCH 03/27] parse_arg(): really test that argument is properly terminated The old parse_arg(), when fed an argument "refs/heads/a"master parsed 'refs/heads/a' off of the front of the argument and considered itself successful. It was only when parse_next_arg() tried to parse the *next* argument that a problem was noticed. But in fact, the definition of the input format requires arguments to be terminated by SP or NUL, so *this* argument is already erroneous and parse_arg() should diagnose the problem. So teach parse_arg() to verify that C-quoted arguments are terminated correctly. If not, emit a more specific error message. There is no corresponding error case of a non-C-quoted argument that is not terminated correctly, because the end of a non-quoted argument is *by definition* a space or NUL, so there is no way to insert other junk between the "end" of the argument and the argument terminator. Adjust the tests to expect the new error message. Add a docstring to the function, incorporating the comments that were formerly within the function plus some added information. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/update-ref.c | 20 +++++++++++++++----- t/t1400-update-ref.sh | 4 ++-- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 1292cfea11..02b5f950e3 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -62,16 +62,26 @@ static void update_store_old_sha1(struct ref_update *update, update->have_old = *oldvalue || line_termination; } +/* + * Parse one whitespace- or NUL-terminated, possibly C-quoted argument + * and append the result to arg. Return a pointer to the terminator. + * Die if there is an error in how the argument is C-quoted. This + * function is only used if not -z. + */ static const char *parse_arg(const char *next, struct strbuf *arg) { - /* Parse SP-terminated, possibly C-quoted argument */ - if (*next != '"') + if (*next == '"') { + const char *orig = next; + + if (unquote_c_style(arg, next, &next)) + die("badly quoted argument: %s", orig); + if (*next && !isspace(*next)) + die("unexpected character after quoted argument: %s", orig); + } else { while (*next && !isspace(*next)) strbuf_addch(arg, *next++); - else if (unquote_c_style(arg, next, &next)) - die("badly quoted argument: %s", next); + } - /* Return position after the argument */ return next; } diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 29391c67fc..774f8c5bf1 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -356,10 +356,10 @@ test_expect_success 'stdin fails on badly quoted input' ' grep "fatal: badly quoted argument: \\\"master" err ' -test_expect_success 'stdin fails on arguments not separated by space' ' +test_expect_success 'stdin fails on junk after quoted argument' ' echo "create \"$a\"master" >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: expected SP but got: master" err + grep "fatal: unexpected character after quoted argument: \\\"$a\\\"master" err ' test_expect_success 'stdin fails create with no ref' ' From 20fcffcc8de9fcfba15fce916ff38c98ca20323d Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:47:55 +0200 Subject: [PATCH 04/27] t1400: add some more tests involving quoted arguments Previously there were no good tests of C-quoted arguments. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- t/t1400-update-ref.sh | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 774f8c5bf1..00862bc28e 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -350,12 +350,18 @@ test_expect_success 'stdin fails on unknown command' ' grep "fatal: unknown command: unknown $a" err ' -test_expect_success 'stdin fails on badly quoted input' ' +test_expect_success 'stdin fails on unbalanced quotes' ' echo "create $a \"master" >stdin && test_must_fail git update-ref --stdin err && grep "fatal: badly quoted argument: \\\"master" err ' +test_expect_success 'stdin fails on invalid escape' ' + echo "create $a \"ma\zter\"" >stdin && + test_must_fail git update-ref --stdin err && + grep "fatal: badly quoted argument: \\\"ma\\\\zter\\\"" err +' + test_expect_success 'stdin fails on junk after quoted argument' ' echo "create \"$a\"master" >stdin && test_must_fail git update-ref --stdin err && @@ -458,6 +464,24 @@ test_expect_success 'stdin create ref works' ' test_cmp expect actual ' +test_expect_success 'stdin succeeds with quoted argument' ' + git update-ref -d $a && + echo "create $a \"$m\"" >stdin && + git update-ref --stdin expect && + git rev-parse $a >actual && + test_cmp expect actual +' + +test_expect_success 'stdin succeeds with escaped character' ' + git update-ref -d $a && + echo "create $a \"ma\\163ter\"" >stdin && + git update-ref --stdin expect && + git rev-parse $a >actual && + test_cmp expect actual +' + test_expect_success 'stdin update ref creates with zero old value' ' echo "update $b $m $Z" >stdin && git update-ref --stdin Date: Mon, 7 Apr 2014 15:47:56 +0200 Subject: [PATCH 05/27] refs.h: rename the action_on_err constants Given that these constants are only being used when updating references, it is inappropriate to give them such generic names as "DIE_ON_ERR". So prefix their names with "UPDATE_REFS_". Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/checkout.c | 2 +- builtin/clone.c | 9 +++++---- builtin/merge.c | 6 +++--- builtin/notes.c | 6 +++--- builtin/reset.c | 6 ++++-- builtin/update-ref.c | 5 +++-- contrib/examples/builtin-fetch--tool.c | 3 ++- notes-cache.c | 2 +- notes-utils.c | 3 ++- refs.c | 18 +++++++++--------- refs.h | 9 +++++++-- 11 files changed, 40 insertions(+), 29 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 1b86d9c868..6bf23188c9 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -624,7 +624,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts, /* Nothing to do. */ } else if (opts->force_detach || !new->path) { /* No longer on any branch. */ update_ref(msg.buf, "HEAD", new->commit->object.sha1, NULL, - REF_NODEREF, DIE_ON_ERR); + REF_NODEREF, UPDATE_REFS_DIE_ON_ERR); if (!opts->quiet) { if (old->path && advice_detached_head) detach_advice(new->name); diff --git a/builtin/clone.c b/builtin/clone.c index 9b3c04d914..b12989d1ca 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -521,7 +521,7 @@ static void write_followtags(const struct ref *refs, const char *msg) if (!has_sha1_file(ref->old_sha1)) continue; update_ref(msg, ref->name, ref->old_sha1, - NULL, 0, DIE_ON_ERR); + NULL, 0, UPDATE_REFS_DIE_ON_ERR); } } @@ -589,14 +589,15 @@ static void update_head(const struct ref *our, const struct ref *remote, create_symref("HEAD", our->name, NULL); if (!option_bare) { const char *head = skip_prefix(our->name, "refs/heads/"); - update_ref(msg, "HEAD", our->old_sha1, NULL, 0, DIE_ON_ERR); + update_ref(msg, "HEAD", our->old_sha1, NULL, 0, + UPDATE_REFS_DIE_ON_ERR); install_branch_config(0, head, option_origin, our->name); } } else if (our) { struct commit *c = lookup_commit_reference(our->old_sha1); /* --branch specifies a non-branch (i.e. tags), detach HEAD */ update_ref(msg, "HEAD", c->object.sha1, - NULL, REF_NODEREF, DIE_ON_ERR); + NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR); } else if (remote) { /* * We know remote HEAD points to a non-branch, or @@ -604,7 +605,7 @@ static void update_head(const struct ref *our, const struct ref *remote, * Detach HEAD in all these cases. */ update_ref(msg, "HEAD", remote->old_sha1, - NULL, REF_NODEREF, DIE_ON_ERR); + NULL, REF_NODEREF, UPDATE_REFS_DIE_ON_ERR); } } diff --git a/builtin/merge.c b/builtin/merge.c index e15d0e145a..7d1d83e8a5 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -398,7 +398,7 @@ static void finish(struct commit *head_commit, const char *argv_gc_auto[] = { "gc", "--auto", NULL }; update_ref(reflog_message.buf, "HEAD", new_head, head, 0, - DIE_ON_ERR); + UPDATE_REFS_DIE_ON_ERR); /* * We ignore errors in 'gc --auto', since the * user should see them. @@ -1222,7 +1222,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) die(_("%s - not something we can merge"), argv[0]); read_empty(remote_head->object.sha1, 0); update_ref("initial pull", "HEAD", remote_head->object.sha1, - NULL, 0, DIE_ON_ERR); + NULL, 0, UPDATE_REFS_DIE_ON_ERR); goto done; } else { struct strbuf merge_names = STRBUF_INIT; @@ -1339,7 +1339,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) } update_ref("updating ORIG_HEAD", "ORIG_HEAD", head_commit->object.sha1, - NULL, 0, DIE_ON_ERR); + NULL, 0, UPDATE_REFS_DIE_ON_ERR); if (remoteheads && !common) ; /* No common ancestors found. We need a real merge. */ diff --git a/builtin/notes.c b/builtin/notes.c index bb89930373..66147b6739 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -717,7 +717,7 @@ static int merge_commit(struct notes_merge_options *o) strbuf_insert(&msg, 0, "notes: ", 7); update_ref(msg.buf, o->local_ref, sha1, is_null_sha1(parent_sha1) ? NULL : parent_sha1, - 0, DIE_ON_ERR); + 0, UPDATE_REFS_DIE_ON_ERR); free_notes(t); strbuf_release(&msg); @@ -812,11 +812,11 @@ static int merge(int argc, const char **argv, const char *prefix) if (result >= 0) /* Merge resulted (trivially) in result_sha1 */ /* Update default notes ref with new commit */ update_ref(msg.buf, default_notes_ref(), result_sha1, NULL, - 0, DIE_ON_ERR); + 0, UPDATE_REFS_DIE_ON_ERR); else { /* Merge has unresolved conflicts */ /* Update .git/NOTES_MERGE_PARTIAL with partial merge result */ update_ref(msg.buf, "NOTES_MERGE_PARTIAL", result_sha1, NULL, - 0, DIE_ON_ERR); + 0, UPDATE_REFS_DIE_ON_ERR); /* Store ref-to-be-updated into .git/NOTES_MERGE_REF */ if (create_symref("NOTES_MERGE_REF", default_notes_ref(), NULL)) die("Failed to store link to current notes ref (%s)", diff --git a/builtin/reset.c b/builtin/reset.c index f4e087596b..f368266762 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -252,11 +252,13 @@ static int reset_refs(const char *rev, const unsigned char *sha1) if (!get_sha1("HEAD", sha1_orig)) { orig = sha1_orig; set_reflog_message(&msg, "updating ORIG_HEAD", NULL); - update_ref(msg.buf, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR); + update_ref(msg.buf, "ORIG_HEAD", orig, old_orig, 0, + UPDATE_REFS_MSG_ON_ERR); } else if (old_orig) delete_ref("ORIG_HEAD", old_orig, 0); set_reflog_message(&msg, "updating HEAD", rev); - update_ref_status = update_ref(msg.buf, "HEAD", sha1, orig, 0, MSG_ON_ERR); + update_ref_status = update_ref(msg.buf, "HEAD", sha1, orig, 0, + UPDATE_REFS_MSG_ON_ERR); strbuf_release(&msg); return update_ref_status; } diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 02b5f950e3..f6345e5251 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -282,7 +282,8 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) if (end_null) line_termination = '\0'; update_refs_stdin(); - return update_refs(msg, updates, updates_count, DIE_ON_ERR); + return update_refs(msg, updates, updates_count, + UPDATE_REFS_DIE_ON_ERR); } if (end_null) @@ -314,5 +315,5 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) return delete_ref(refname, oldval ? oldsha1 : NULL, flags); else return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL, - flags, DIE_ON_ERR); + flags, UPDATE_REFS_DIE_ON_ERR); } diff --git a/contrib/examples/builtin-fetch--tool.c b/contrib/examples/builtin-fetch--tool.c index 8bc8c7533a..ee1916641e 100644 --- a/contrib/examples/builtin-fetch--tool.c +++ b/contrib/examples/builtin-fetch--tool.c @@ -31,7 +31,8 @@ static int update_ref_env(const char *action, rla = "(reflog update)"; if (snprintf(msg, sizeof(msg), "%s: %s", rla, action) >= sizeof(msg)) warning("reflog message too long: %.*s...", 50, msg); - return update_ref(msg, refname, sha1, oldval, 0, QUIET_ON_ERR); + return update_ref(msg, refname, sha1, oldval, 0, + UPDATE_REFS_QUIET_ON_ERR); } static int update_local_ref(const char *name, diff --git a/notes-cache.c b/notes-cache.c index eabe4a0d9b..97dfd63c9b 100644 --- a/notes-cache.c +++ b/notes-cache.c @@ -62,7 +62,7 @@ int notes_cache_write(struct notes_cache *c) if (commit_tree(&msg, tree_sha1, NULL, commit_sha1, NULL, NULL) < 0) return -1; if (update_ref("update notes cache", c->tree.ref, commit_sha1, NULL, - 0, QUIET_ON_ERR) < 0) + 0, UPDATE_REFS_QUIET_ON_ERR) < 0) return -1; return 0; diff --git a/notes-utils.c b/notes-utils.c index 4aa7023903..a0b1d7be98 100644 --- a/notes-utils.c +++ b/notes-utils.c @@ -48,7 +48,8 @@ void commit_notes(struct notes_tree *t, const char *msg) create_notes_commit(t, NULL, &buf, commit_sha1); strbuf_insert(&buf, 0, "notes: ", 7); /* commit message starts at index 7 */ - update_ref(buf.buf, t->ref, commit_sha1, NULL, 0, DIE_ON_ERR); + update_ref(buf.buf, t->ref, commit_sha1, NULL, 0, + UPDATE_REFS_DIE_ON_ERR); strbuf_release(&buf); } diff --git a/refs.c b/refs.c index 28d5eca8ea..196984e12e 100644 --- a/refs.c +++ b/refs.c @@ -3243,9 +3243,9 @@ static struct ref_lock *update_ref_lock(const char *refname, if (!lock) { const char *str = "Cannot lock the ref '%s'."; switch (onerr) { - case MSG_ON_ERR: error(str, refname); break; - case DIE_ON_ERR: die(str, refname); break; - case QUIET_ON_ERR: break; + case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break; + case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break; + case UPDATE_REFS_QUIET_ON_ERR: break; } } return lock; @@ -3258,9 +3258,9 @@ static int update_ref_write(const char *action, const char *refname, if (write_ref_sha1(lock, sha1, action) < 0) { const char *str = "Cannot update the ref '%s'."; switch (onerr) { - case MSG_ON_ERR: error(str, refname); break; - case DIE_ON_ERR: die(str, refname); break; - case QUIET_ON_ERR: break; + case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break; + case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break; + case UPDATE_REFS_QUIET_ON_ERR: break; } return 1; } @@ -3294,11 +3294,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n, const char *str = "Multiple updates for ref '%s' not allowed."; switch (onerr) { - case MSG_ON_ERR: + case UPDATE_REFS_MSG_ON_ERR: error(str, updates[i]->ref_name); break; - case DIE_ON_ERR: + case UPDATE_REFS_DIE_ON_ERR: die(str, updates[i]->ref_name); break; - case QUIET_ON_ERR: + case UPDATE_REFS_QUIET_ON_ERR: break; } return 1; diff --git a/refs.h b/refs.h index 87a1a79ad6..a713b34ad8 100644 --- a/refs.h +++ b/refs.h @@ -214,8 +214,13 @@ extern int rename_ref(const char *oldref, const char *newref, const char *logmsg */ extern int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *sha1); -/** lock a ref and then write its file */ -enum action_on_err { MSG_ON_ERR, DIE_ON_ERR, QUIET_ON_ERR }; +enum action_on_err { + UPDATE_REFS_MSG_ON_ERR, + UPDATE_REFS_DIE_ON_ERR, + UPDATE_REFS_QUIET_ON_ERR +}; + +/** Lock a ref and then write its file */ int update_ref(const char *action, const char *refname, const unsigned char *sha1, const unsigned char *oldval, int flags, enum action_on_err onerr); From 595deb8da69b4f816ff0c8e669b49f7527ff609b Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:47:57 +0200 Subject: [PATCH 06/27] update_refs(): fix constness The old signature of update_refs() required a (const struct ref_update **) for its updates_orig argument. The "const" is presumably there to promise that the function will not modify the contents of the structures. But this declaration does not permit the function to be called with a (struct ref_update **), which is perfectly legitimate. C's type system is not powerful enough to express what we'd like. So remove the first "const" from the declaration. On the other hand, the function *can* promise not to modify the pointers within the array that is passed to it without inconveniencing its callers. So add a "const" that has that effect, making the final declaration (struct ref_update * const *). Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/update-ref.c | 2 +- refs.c | 2 +- refs.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index f6345e5251..a8a68e8a50 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -14,7 +14,7 @@ static const char * const git_update_ref_usage[] = { static int updates_alloc; static int updates_count; -static const struct ref_update **updates; +static struct ref_update **updates; static char line_termination = '\n'; static int update_flags; diff --git a/refs.c b/refs.c index 196984e12e..1305eb1fcf 100644 --- a/refs.c +++ b/refs.c @@ -3306,7 +3306,7 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n, return 0; } -int update_refs(const char *action, const struct ref_update **updates_orig, +int update_refs(const char *action, struct ref_update * const *updates_orig, int n, enum action_on_err onerr) { int ret = 0, delnum = 0, i; diff --git a/refs.h b/refs.h index a713b34ad8..08e60ac07f 100644 --- a/refs.h +++ b/refs.h @@ -228,7 +228,7 @@ int update_ref(const char *action, const char *refname, /** * Lock all refs and then perform all modifications. */ -int update_refs(const char *action, const struct ref_update **updates, +int update_refs(const char *action, struct ref_update * const *updates, int n, enum action_on_err onerr); extern int parse_hide_refs_config(const char *var, const char *value, const char *); From e23d84350a3f4a3bfb86037eb1e6c4b28240324e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:47:58 +0200 Subject: [PATCH 07/27] update-ref --stdin: read the whole input at once Read the whole input into a strbuf at once, and then parse it from there. This might also be a tad faster, but that is not the point. The point is to decouple the parsing code from the input source (the old parsing code had to read new data even in the middle of commands). Add docstrings for the parsing functions. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/update-ref.c | 170 +++++++++++++++++++++++++++---------------- 1 file changed, 108 insertions(+), 62 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index a8a68e8a50..5f197fe212 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -85,44 +85,70 @@ static const char *parse_arg(const char *next, struct strbuf *arg) return next; } -static const char *parse_first_arg(const char *next, struct strbuf *arg) +/* + * Parse the argument immediately after "command SP". If not -z, then + * handle C-quoting. Write the argument to arg. Set *next to point + * at the character that terminates the argument. Die if C-quoting is + * malformed. + */ +static void parse_first_arg(struct strbuf *input, const char **next, + struct strbuf *arg) { - /* Parse argument immediately after "command SP" */ strbuf_reset(arg); if (line_termination) { /* Without -z, use the next argument */ - next = parse_arg(next, arg); + *next = parse_arg(*next, arg); } else { - /* With -z, use rest of first NUL-terminated line */ - strbuf_addstr(arg, next); - next = next + arg->len; + /* With -z, use everything up to the next NUL */ + strbuf_addstr(arg, *next); + *next += arg->len; } - return next; } -static const char *parse_next_arg(const char *next, struct strbuf *arg) +/* + * Parse a SP/NUL separator followed by the next SP- or NUL-terminated + * argument, if any. If there is an argument, write it to arg, set + * *next to point at the character terminating the argument, and + * return 0. If there is no argument at all (not even the empty + * string), return a non-zero result and leave *next unchanged. + */ +static int parse_next_arg(struct strbuf *input, const char **next, + struct strbuf *arg) { - /* Parse next SP-terminated or NUL-terminated argument, if any */ strbuf_reset(arg); if (line_termination) { /* Without -z, consume SP and use next argument */ - if (!*next) - return NULL; - if (*next != ' ') - die("expected SP but got: %s", next); - next = parse_arg(next + 1, arg); + if (!**next || **next == line_termination) + return -1; + if (**next != ' ') + die("expected SP but got: %s", *next); + (*next)++; + *next = parse_arg(*next, arg); } else { /* With -z, read the next NUL-terminated line */ - if (*next) - die("expected NUL but got: %s", next); - if (strbuf_getline(arg, stdin, '\0') == EOF) - return NULL; - next = arg->buf + arg->len; + if (**next) + die("expected NUL but got: %s", *next); + (*next)++; + if (*next == input->buf + input->len) + return -1; + strbuf_addstr(arg, *next); + *next += arg->len; } - return next; + return 0; } -static void parse_cmd_update(const char *next) + +/* + * The following five parse_cmd_*() functions parse the corresponding + * command. In each case, next points at the character following the + * command name and the following space. They each return a pointer + * to the character terminating the command, and die with an + * explanatory message if there are any parsing problems. All of + * these functions handle either text or binary format input, + * depending on how line_termination is set. + */ + +static const char *parse_cmd_update(struct strbuf *input, const char *next) { struct strbuf ref = STRBUF_INIT; struct strbuf newvalue = STRBUF_INIT; @@ -131,26 +157,28 @@ static void parse_cmd_update(const char *next) update = update_alloc(); - if ((next = parse_first_arg(next, &ref)) != NULL && ref.buf[0]) + parse_first_arg(input, &next, &ref); + if (ref.buf[0]) update_store_ref_name(update, ref.buf); else die("update line missing "); - if ((next = parse_next_arg(next, &newvalue)) != NULL) + if (!parse_next_arg(input, &next, &newvalue)) update_store_new_sha1(update, newvalue.buf); else die("update %s missing ", ref.buf); - if ((next = parse_next_arg(next, &oldvalue)) != NULL) + if (!parse_next_arg(input, &next, &oldvalue)) { update_store_old_sha1(update, oldvalue.buf); - else if(!line_termination) + if (*next != line_termination) + die("update %s has extra input: %s", ref.buf, next); + } else if (!line_termination) die("update %s missing [] NUL", ref.buf); - if (next && *next) - die("update %s has extra input: %s", ref.buf, next); + return next; } -static void parse_cmd_create(const char *next) +static const char *parse_cmd_create(struct strbuf *input, const char *next) { struct strbuf ref = STRBUF_INIT; struct strbuf newvalue = STRBUF_INIT; @@ -158,23 +186,27 @@ static void parse_cmd_create(const char *next) update = update_alloc(); - if ((next = parse_first_arg(next, &ref)) != NULL && ref.buf[0]) + parse_first_arg(input, &next, &ref); + if (ref.buf[0]) update_store_ref_name(update, ref.buf); else die("create line missing "); - if ((next = parse_next_arg(next, &newvalue)) != NULL) + if (!parse_next_arg(input, &next, &newvalue)) update_store_new_sha1(update, newvalue.buf); else die("create %s missing ", ref.buf); + if (is_null_sha1(update->new_sha1)) die("create %s given zero new value", ref.buf); - if (next && *next) + if (*next != line_termination) die("create %s has extra input: %s", ref.buf, next); + + return next; } -static void parse_cmd_delete(const char *next) +static const char *parse_cmd_delete(struct strbuf *input, const char *next) { struct strbuf ref = STRBUF_INIT; struct strbuf oldvalue = STRBUF_INIT; @@ -182,23 +214,26 @@ static void parse_cmd_delete(const char *next) update = update_alloc(); - if ((next = parse_first_arg(next, &ref)) != NULL && ref.buf[0]) + parse_first_arg(input, &next, &ref); + if (ref.buf[0]) update_store_ref_name(update, ref.buf); else die("delete line missing "); - if ((next = parse_next_arg(next, &oldvalue)) != NULL) + if (!parse_next_arg(input, &next, &oldvalue)) { update_store_old_sha1(update, oldvalue.buf); - else if(!line_termination) + if (update->have_old && is_null_sha1(update->old_sha1)) + die("delete %s given zero old value", ref.buf); + } else if (!line_termination) die("delete %s missing [] NUL", ref.buf); - if (update->have_old && is_null_sha1(update->old_sha1)) - die("delete %s given zero old value", ref.buf); - if (next && *next) + if (*next != line_termination) die("delete %s has extra input: %s", ref.buf, next); + + return next; } -static void parse_cmd_verify(const char *next) +static const char *parse_cmd_verify(struct strbuf *input, const char *next) { struct strbuf ref = STRBUF_INIT; struct strbuf value = STRBUF_INIT; @@ -206,53 +241,64 @@ static void parse_cmd_verify(const char *next) update = update_alloc(); - if ((next = parse_first_arg(next, &ref)) != NULL && ref.buf[0]) + parse_first_arg(input, &next, &ref); + if (ref.buf[0]) update_store_ref_name(update, ref.buf); else die("verify line missing "); - if ((next = parse_next_arg(next, &value)) != NULL) { + if (!parse_next_arg(input, &next, &value)) { update_store_old_sha1(update, value.buf); update_store_new_sha1(update, value.buf); - } else if(!line_termination) + } else if (!line_termination) die("verify %s missing [] NUL", ref.buf); - if (next && *next) + if (*next != line_termination) die("verify %s has extra input: %s", ref.buf, next); + + return next; } -static void parse_cmd_option(const char *next) +static const char *parse_cmd_option(struct strbuf *input, const char *next) { - if (!strcmp(next, "no-deref")) + if (!strncmp(next, "no-deref", 8) && next[8] == line_termination) update_flags |= REF_NODEREF; else die("option unknown: %s", next); + return next + 8; } static void update_refs_stdin(void) { - struct strbuf cmd = STRBUF_INIT; + struct strbuf input = STRBUF_INIT; + const char *next; + if (strbuf_read(&input, 0, 1000) < 0) + die_errno("could not read from stdin"); + next = input.buf; /* Read each line dispatch its command */ - while (strbuf_getline(&cmd, stdin, line_termination) != EOF) - if (!cmd.buf[0]) + while (next < input.buf + input.len) { + if (*next == line_termination) die("empty command in input"); - else if (isspace(*cmd.buf)) - die("whitespace before command: %s", cmd.buf); - else if (starts_with(cmd.buf, "update ")) - parse_cmd_update(cmd.buf + 7); - else if (starts_with(cmd.buf, "create ")) - parse_cmd_create(cmd.buf + 7); - else if (starts_with(cmd.buf, "delete ")) - parse_cmd_delete(cmd.buf + 7); - else if (starts_with(cmd.buf, "verify ")) - parse_cmd_verify(cmd.buf + 7); - else if (starts_with(cmd.buf, "option ")) - parse_cmd_option(cmd.buf + 7); + else if (isspace(*next)) + die("whitespace before command: %s", next); + else if (starts_with(next, "update ")) + next = parse_cmd_update(&input, next + 7); + else if (starts_with(next, "create ")) + next = parse_cmd_create(&input, next + 7); + else if (starts_with(next, "delete ")) + next = parse_cmd_delete(&input, next + 7); + else if (starts_with(next, "verify ")) + next = parse_cmd_verify(&input, next + 7); + else if (starts_with(next, "option ")) + next = parse_cmd_option(&input, next + 7); else - die("unknown command: %s", cmd.buf); + die("unknown command: %s", next); + + next++; + } - strbuf_release(&cmd); + strbuf_release(&input); } int cmd_update_ref(int argc, const char **argv, const char *prefix) From 2f57736002e6a774ce5ab53a15a631da8299f8b4 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:47:59 +0200 Subject: [PATCH 08/27] parse_cmd_verify(): copy old_sha1 instead of evaluating twice Aside from avoiding a tiny bit of work, this makes it transparently obvious that old_sha1 and new_sha1 are identical. It is arguably a bit silly to have to set new_sha1 in order to verify old_sha1, but that is a problem for another day. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/update-ref.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 5f197fe212..51adf2dafa 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -249,7 +249,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next) if (!parse_next_arg(input, &next, &value)) { update_store_old_sha1(update, value.buf); - update_store_new_sha1(update, value.buf); + hashcpy(update->new_sha1, update->old_sha1); } else if (!line_termination) die("verify %s missing [] NUL", ref.buf); From ed410e611dc39dc7e845f27a660b76b7d0ecbab6 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:00 +0200 Subject: [PATCH 09/27] update-ref.c: extract a new function, parse_refname() There is no reason to obscure the fact that parse_first_arg() always parses refnames. Form the new function by combining parse_first_arg() and update_store_ref_name(). Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/update-ref.c | 90 ++++++++++++++++++++------------------------ 1 file changed, 41 insertions(+), 49 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 51adf2dafa..0dc2061c7e 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -35,14 +35,6 @@ static struct ref_update *update_alloc(void) return update; } -static void update_store_ref_name(struct ref_update *update, - const char *ref_name) -{ - if (check_refname_format(ref_name, REFNAME_ALLOW_ONELEVEL)) - die("invalid ref format: %s", ref_name); - update->ref_name = xstrdup(ref_name); -} - static void update_store_new_sha1(struct ref_update *update, const char *newvalue) { @@ -86,23 +78,35 @@ static const char *parse_arg(const char *next, struct strbuf *arg) } /* - * Parse the argument immediately after "command SP". If not -z, then - * handle C-quoting. Write the argument to arg. Set *next to point - * at the character that terminates the argument. Die if C-quoting is - * malformed. + * Parse the reference name immediately after "command SP". If not + * -z, then handle C-quoting. Return a pointer to a newly allocated + * string containing the name of the reference, or NULL if there was + * an error. Update *next to point at the character that terminates + * the argument. Die if C-quoting is malformed or the reference name + * is invalid. */ -static void parse_first_arg(struct strbuf *input, const char **next, - struct strbuf *arg) +static char *parse_refname(struct strbuf *input, const char **next) { - strbuf_reset(arg); + struct strbuf ref = STRBUF_INIT; + if (line_termination) { /* Without -z, use the next argument */ - *next = parse_arg(*next, arg); + *next = parse_arg(*next, &ref); } else { /* With -z, use everything up to the next NUL */ - strbuf_addstr(arg, *next); - *next += arg->len; + strbuf_addstr(&ref, *next); + *next += ref.len; + } + + if (!ref.len) { + strbuf_release(&ref); + return NULL; } + + if (check_refname_format(ref.buf, REFNAME_ALLOW_ONELEVEL)) + die("invalid ref format: %s", ref.buf); + + return strbuf_detach(&ref, NULL); } /* @@ -150,111 +154,99 @@ static int parse_next_arg(struct strbuf *input, const char **next, static const char *parse_cmd_update(struct strbuf *input, const char *next) { - struct strbuf ref = STRBUF_INIT; struct strbuf newvalue = STRBUF_INIT; struct strbuf oldvalue = STRBUF_INIT; struct ref_update *update; update = update_alloc(); - parse_first_arg(input, &next, &ref); - if (ref.buf[0]) - update_store_ref_name(update, ref.buf); - else + update->ref_name = parse_refname(input, &next); + if (!update->ref_name) die("update line missing "); if (!parse_next_arg(input, &next, &newvalue)) update_store_new_sha1(update, newvalue.buf); else - die("update %s missing ", ref.buf); + die("update %s missing ", update->ref_name); if (!parse_next_arg(input, &next, &oldvalue)) { update_store_old_sha1(update, oldvalue.buf); if (*next != line_termination) - die("update %s has extra input: %s", ref.buf, next); + die("update %s has extra input: %s", update->ref_name, next); } else if (!line_termination) - die("update %s missing [] NUL", ref.buf); + die("update %s missing [] NUL", update->ref_name); return next; } static const char *parse_cmd_create(struct strbuf *input, const char *next) { - struct strbuf ref = STRBUF_INIT; struct strbuf newvalue = STRBUF_INIT; struct ref_update *update; update = update_alloc(); - parse_first_arg(input, &next, &ref); - if (ref.buf[0]) - update_store_ref_name(update, ref.buf); - else + update->ref_name = parse_refname(input, &next); + if (!update->ref_name) die("create line missing "); if (!parse_next_arg(input, &next, &newvalue)) update_store_new_sha1(update, newvalue.buf); else - die("create %s missing ", ref.buf); + die("create %s missing ", update->ref_name); if (is_null_sha1(update->new_sha1)) - die("create %s given zero new value", ref.buf); + die("create %s given zero new value", update->ref_name); if (*next != line_termination) - die("create %s has extra input: %s", ref.buf, next); + die("create %s has extra input: %s", update->ref_name, next); return next; } static const char *parse_cmd_delete(struct strbuf *input, const char *next) { - struct strbuf ref = STRBUF_INIT; struct strbuf oldvalue = STRBUF_INIT; struct ref_update *update; update = update_alloc(); - parse_first_arg(input, &next, &ref); - if (ref.buf[0]) - update_store_ref_name(update, ref.buf); - else + update->ref_name = parse_refname(input, &next); + if (!update->ref_name) die("delete line missing "); if (!parse_next_arg(input, &next, &oldvalue)) { update_store_old_sha1(update, oldvalue.buf); if (update->have_old && is_null_sha1(update->old_sha1)) - die("delete %s given zero old value", ref.buf); + die("delete %s given zero old value", update->ref_name); } else if (!line_termination) - die("delete %s missing [] NUL", ref.buf); + die("delete %s missing [] NUL", update->ref_name); if (*next != line_termination) - die("delete %s has extra input: %s", ref.buf, next); + die("delete %s has extra input: %s", update->ref_name, next); return next; } static const char *parse_cmd_verify(struct strbuf *input, const char *next) { - struct strbuf ref = STRBUF_INIT; struct strbuf value = STRBUF_INIT; struct ref_update *update; update = update_alloc(); - parse_first_arg(input, &next, &ref); - if (ref.buf[0]) - update_store_ref_name(update, ref.buf); - else + update->ref_name = parse_refname(input, &next); + if (!update->ref_name) die("verify line missing "); if (!parse_next_arg(input, &next, &value)) { update_store_old_sha1(update, value.buf); hashcpy(update->new_sha1, update->old_sha1); } else if (!line_termination) - die("verify %s missing [] NUL", ref.buf); + die("verify %s missing [] NUL", update->ref_name); if (*next != line_termination) - die("verify %s has extra input: %s", ref.buf, next); + die("verify %s has extra input: %s", update->ref_name, next); return next; } From 1746ef4e9d869fb0f54194bb225604eb61a77501 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:01 +0200 Subject: [PATCH 10/27] update-ref --stdin: improve error messages for invalid values If an invalid value is passed to "update-ref --stdin" as or , include the command and the name of the reference at the beginning of the error message. Update the tests accordingly. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/update-ref.c | 24 +++++++++++++----------- t/t1400-update-ref.sh | 8 ++++---- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 0dc2061c7e..13a884a45e 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -35,20 +35,22 @@ static struct ref_update *update_alloc(void) return update; } -static void update_store_new_sha1(struct ref_update *update, +static void update_store_new_sha1(const char *command, + struct ref_update *update, const char *newvalue) { if (*newvalue && get_sha1(newvalue, update->new_sha1)) - die("invalid new value for ref %s: %s", - update->ref_name, newvalue); + die("%s %s: invalid new value: %s", + command, update->ref_name, newvalue); } -static void update_store_old_sha1(struct ref_update *update, +static void update_store_old_sha1(const char *command, + struct ref_update *update, const char *oldvalue) { if (*oldvalue && get_sha1(oldvalue, update->old_sha1)) - die("invalid old value for ref %s: %s", - update->ref_name, oldvalue); + die("%s %s: invalid old value: %s", + command, update->ref_name, oldvalue); /* We have an old value if non-empty, or if empty without -z */ update->have_old = *oldvalue || line_termination; @@ -165,12 +167,12 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next) die("update line missing "); if (!parse_next_arg(input, &next, &newvalue)) - update_store_new_sha1(update, newvalue.buf); + update_store_new_sha1("update", update, newvalue.buf); else die("update %s missing ", update->ref_name); if (!parse_next_arg(input, &next, &oldvalue)) { - update_store_old_sha1(update, oldvalue.buf); + update_store_old_sha1("update", update, oldvalue.buf); if (*next != line_termination) die("update %s has extra input: %s", update->ref_name, next); } else if (!line_termination) @@ -191,7 +193,7 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next) die("create line missing "); if (!parse_next_arg(input, &next, &newvalue)) - update_store_new_sha1(update, newvalue.buf); + update_store_new_sha1("create", update, newvalue.buf); else die("create %s missing ", update->ref_name); @@ -216,7 +218,7 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next) die("delete line missing "); if (!parse_next_arg(input, &next, &oldvalue)) { - update_store_old_sha1(update, oldvalue.buf); + update_store_old_sha1("delete", update, oldvalue.buf); if (update->have_old && is_null_sha1(update->old_sha1)) die("delete %s given zero old value", update->ref_name); } else if (!line_termination) @@ -240,7 +242,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next) die("verify line missing "); if (!parse_next_arg(input, &next, &value)) { - update_store_old_sha1(update, value.buf); + update_store_old_sha1("verify", update, value.buf); hashcpy(update->new_sha1, update->old_sha1); } else if (!line_termination) die("verify %s missing [] NUL", update->ref_name); diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 00862bc28e..f6c6e960da 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -518,14 +518,14 @@ test_expect_success 'stdin update ref fails with wrong old value' ' test_expect_success 'stdin update ref fails with bad old value' ' echo "update $c $m does-not-exist" >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: invalid old value for ref $c: does-not-exist" err && + grep "fatal: update $c: invalid old value: does-not-exist" err && test_must_fail git rev-parse --verify -q $c ' test_expect_success 'stdin create ref fails with bad new value' ' echo "create $c does-not-exist" >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: invalid new value for ref $c: does-not-exist" err && + grep "fatal: create $c: invalid new value: does-not-exist" err && test_must_fail git rev-parse --verify -q $c ' @@ -840,14 +840,14 @@ test_expect_success 'stdin -z update ref fails with wrong old value' ' test_expect_success 'stdin -z update ref fails with bad old value' ' printf $F "update $c" "$m" "does-not-exist" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: invalid old value for ref $c: does-not-exist" err && + grep "fatal: update $c: invalid old value: does-not-exist" err && test_must_fail git rev-parse --verify -q $c ' test_expect_success 'stdin -z create ref fails with bad new value' ' printf $F "create $c" "does-not-exist" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: invalid new value for ref $c: does-not-exist" err && + grep "fatal: create $c: invalid new value: does-not-exist" err && test_must_fail git rev-parse --verify -q $c ' From 9255f059ff0bfce7605748f62f58d6ba9055a1f3 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:02 +0200 Subject: [PATCH 11/27] update-ref --stdin: make error messages more consistent The old error messages emitted for invalid input sometimes said ""/"" and sometimes said "old value"/"new value". Convert them all to the former. Update the tests accordingly. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/update-ref.c | 8 ++++---- t/t1400-update-ref.sh | 14 +++++++------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 13a884a45e..e4c0854e9f 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -40,7 +40,7 @@ static void update_store_new_sha1(const char *command, const char *newvalue) { if (*newvalue && get_sha1(newvalue, update->new_sha1)) - die("%s %s: invalid new value: %s", + die("%s %s: invalid : %s", command, update->ref_name, newvalue); } @@ -49,7 +49,7 @@ static void update_store_old_sha1(const char *command, const char *oldvalue) { if (*oldvalue && get_sha1(oldvalue, update->old_sha1)) - die("%s %s: invalid old value: %s", + die("%s %s: invalid : %s", command, update->ref_name, oldvalue); /* We have an old value if non-empty, or if empty without -z */ @@ -198,7 +198,7 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next) die("create %s missing ", update->ref_name); if (is_null_sha1(update->new_sha1)) - die("create %s given zero new value", update->ref_name); + die("create %s given zero ", update->ref_name); if (*next != line_termination) die("create %s has extra input: %s", update->ref_name, next); @@ -220,7 +220,7 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next) if (!parse_next_arg(input, &next, &oldvalue)) { update_store_old_sha1("delete", update, oldvalue.buf); if (update->have_old && is_null_sha1(update->old_sha1)) - die("delete %s given zero old value", update->ref_name); + die("delete %s given zero ", update->ref_name); } else if (!line_termination) die("delete %s missing [] NUL", update->ref_name); diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index f6c6e960da..ef61fe324b 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -518,21 +518,21 @@ test_expect_success 'stdin update ref fails with wrong old value' ' test_expect_success 'stdin update ref fails with bad old value' ' echo "update $c $m does-not-exist" >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: update $c: invalid old value: does-not-exist" err && + grep "fatal: update $c: invalid : does-not-exist" err && test_must_fail git rev-parse --verify -q $c ' test_expect_success 'stdin create ref fails with bad new value' ' echo "create $c does-not-exist" >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: create $c: invalid new value: does-not-exist" err && + grep "fatal: create $c: invalid : does-not-exist" err && test_must_fail git rev-parse --verify -q $c ' test_expect_success 'stdin create ref fails with zero new value' ' echo "create $c " >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: create $c given zero new value" err && + grep "fatal: create $c given zero " err && test_must_fail git rev-parse --verify -q $c ' @@ -556,7 +556,7 @@ test_expect_success 'stdin delete ref fails with wrong old value' ' test_expect_success 'stdin delete ref fails with zero old value' ' echo "delete $a " >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: delete $a given zero old value" err && + grep "fatal: delete $a given zero " err && git rev-parse $m >expect && git rev-parse $a >actual && test_cmp expect actual @@ -840,14 +840,14 @@ test_expect_success 'stdin -z update ref fails with wrong old value' ' test_expect_success 'stdin -z update ref fails with bad old value' ' printf $F "update $c" "$m" "does-not-exist" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: update $c: invalid old value: does-not-exist" err && + grep "fatal: update $c: invalid : does-not-exist" err && test_must_fail git rev-parse --verify -q $c ' test_expect_success 'stdin -z create ref fails with bad new value' ' printf $F "create $c" "does-not-exist" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: create $c: invalid new value: does-not-exist" err && + grep "fatal: create $c: invalid : does-not-exist" err && test_must_fail git rev-parse --verify -q $c ' @@ -878,7 +878,7 @@ test_expect_success 'stdin -z delete ref fails with wrong old value' ' test_expect_success 'stdin -z delete ref fails with zero old value' ' printf $F "delete $a" "$Z" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: delete $a given zero old value" err && + grep "fatal: delete $a given zero " err && git rev-parse $m >expect && git rev-parse $a >actual && test_cmp expect actual From ac1177553d8c632e93c507f8efc80b80e6c7d3d8 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:03 +0200 Subject: [PATCH 12/27] update-ref --stdin: simplify error messages for missing oldvalues Instead of, for example, fatal: update refs/heads/master missing [] NUL emit fatal: update refs/heads/master missing Update the tests accordingly. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/update-ref.c | 6 +++--- t/t1400-update-ref.sh | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index e4c0854e9f..a9eb5fe24d 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -176,7 +176,7 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next) if (*next != line_termination) die("update %s has extra input: %s", update->ref_name, next); } else if (!line_termination) - die("update %s missing [] NUL", update->ref_name); + die("update %s missing ", update->ref_name); return next; } @@ -222,7 +222,7 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next) if (update->have_old && is_null_sha1(update->old_sha1)) die("delete %s given zero ", update->ref_name); } else if (!line_termination) - die("delete %s missing [] NUL", update->ref_name); + die("delete %s missing ", update->ref_name); if (*next != line_termination) die("delete %s has extra input: %s", update->ref_name, next); @@ -245,7 +245,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next) update_store_old_sha1("verify", update, value.buf); hashcpy(update->new_sha1, update->old_sha1); } else if (!line_termination) - die("verify %s missing [] NUL", update->ref_name); + die("verify %s missing ", update->ref_name); if (*next != line_termination) die("verify %s has extra input: %s", update->ref_name, next); diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index ef61fe324b..a2015d0977 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -739,7 +739,7 @@ test_expect_success 'stdin -z fails update with no new value' ' test_expect_success 'stdin -z fails update with no old value' ' printf $F "update $a" "$m" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: update $a missing \\[\\] NUL" err + grep "fatal: update $a missing " err ' test_expect_success 'stdin -z fails update with too many arguments' ' @@ -763,7 +763,7 @@ test_expect_success 'stdin -z fails delete with bad ref name' ' test_expect_success 'stdin -z fails delete with no old value' ' printf $F "delete $a" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: delete $a missing \\[\\] NUL" err + grep "fatal: delete $a missing " err ' test_expect_success 'stdin -z fails delete with too many arguments' ' @@ -781,7 +781,7 @@ test_expect_success 'stdin -z fails verify with too many arguments' ' test_expect_success 'stdin -z fails verify with no old value' ' printf $F "verify $a" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: verify $a missing \\[\\] NUL" err + grep "fatal: verify $a missing " err ' test_expect_success 'stdin -z fails option with unknown name' ' From 191f241b528c10e242d045bde2cef70fb013a6e5 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:04 +0200 Subject: [PATCH 13/27] t1400: test that stdin -z update treats empty as zeros This is the (slightly inconsistent) status quo; make sure it doesn't change by accident. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- t/t1400-update-ref.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index a2015d0977..208f56e518 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -730,6 +730,13 @@ test_expect_success 'stdin -z fails update with bad ref name' ' grep "fatal: invalid ref format: ~a" err ' +test_expect_success 'stdin -z treats empty new value as zeros' ' + git update-ref $a $m && + printf $F "update $a" "" "" >stdin && + git update-ref -z --stdin stdin && test_must_fail git update-ref -z --stdin err && From 3afcc4637452100c68b469de7757dd2b45b4d29c Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:05 +0200 Subject: [PATCH 14/27] update-ref.c: extract a new function, parse_next_sha1() Replace three functions, update_store_new_sha1(), update_store_old_sha1(), and parse_next_arg(), with a single function, parse_next_sha1(). The new function takes care of a whole argument, including checking whether it is there, converting it to an SHA-1, and emitting errors on EOF or for invalid values. The return value indicates whether the argument was present or absent, which requires a bit of intelligence because absent values are represented differently depending on whether "-z" was used. The new interface means that the calling functions, parse_cmd_*(), don't have to interpret the result differently based on the line_termination mode that is in effect. It also means that parse_cmd_create() can distinguish unambiguously between an empty new value and a zeros new value, which fixes a failure in t1400. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/update-ref.c | 160 ++++++++++++++++++++++++++---------------- t/t1400-update-ref.sh | 2 +- 2 files changed, 99 insertions(+), 63 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index a9eb5fe24d..c61120faf7 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -35,27 +35,6 @@ static struct ref_update *update_alloc(void) return update; } -static void update_store_new_sha1(const char *command, - struct ref_update *update, - const char *newvalue) -{ - if (*newvalue && get_sha1(newvalue, update->new_sha1)) - die("%s %s: invalid : %s", - command, update->ref_name, newvalue); -} - -static void update_store_old_sha1(const char *command, - struct ref_update *update, - const char *oldvalue) -{ - if (*oldvalue && get_sha1(oldvalue, update->old_sha1)) - die("%s %s: invalid : %s", - command, update->ref_name, oldvalue); - - /* We have an old value if non-empty, or if empty without -z */ - update->have_old = *oldvalue || line_termination; -} - /* * Parse one whitespace- or NUL-terminated, possibly C-quoted argument * and append the result to arg. Return a pointer to the terminator. @@ -112,35 +91,94 @@ static char *parse_refname(struct strbuf *input, const char **next) } /* - * Parse a SP/NUL separator followed by the next SP- or NUL-terminated - * argument, if any. If there is an argument, write it to arg, set - * *next to point at the character terminating the argument, and + * The value being parsed is (as opposed to ; the + * difference affects which error messages are generated): + */ +#define PARSE_SHA1_OLD 0x01 + +/* + * For backwards compatibility, accept an empty string for update's + * in binary mode to be equivalent to specifying zeros. + */ +#define PARSE_SHA1_ALLOW_EMPTY 0x02 + +/* + * Parse an argument separator followed by the next argument, if any. + * If there is an argument, convert it to a SHA-1, write it to sha1, + * set *next to point at the character terminating the argument, and * return 0. If there is no argument at all (not even the empty - * string), return a non-zero result and leave *next unchanged. + * string), return 1 and leave *next unchanged. If the value is + * provided but cannot be converted to a SHA-1, die. flags can + * include PARSE_SHA1_OLD and/or PARSE_SHA1_ALLOW_EMPTY. */ -static int parse_next_arg(struct strbuf *input, const char **next, - struct strbuf *arg) +static int parse_next_sha1(struct strbuf *input, const char **next, + unsigned char *sha1, + const char *command, const char *refname, + int flags) { - strbuf_reset(arg); + struct strbuf arg = STRBUF_INIT; + int ret = 0; + + if (*next == input->buf + input->len) + goto eof; + if (line_termination) { /* Without -z, consume SP and use next argument */ if (!**next || **next == line_termination) - return -1; + return 1; if (**next != ' ') - die("expected SP but got: %s", *next); + die("%s %s: expected SP but got: %s", + command, refname, *next); (*next)++; - *next = parse_arg(*next, arg); + *next = parse_arg(*next, &arg); + if (arg.len) { + if (get_sha1(arg.buf, sha1)) + goto invalid; + } else { + /* Without -z, an empty value means all zeros: */ + hashclr(sha1); + } } else { /* With -z, read the next NUL-terminated line */ if (**next) - die("expected NUL but got: %s", *next); + die("%s %s: expected NUL but got: %s", + command, refname, *next); (*next)++; if (*next == input->buf + input->len) - return -1; - strbuf_addstr(arg, *next); - *next += arg->len; + goto eof; + strbuf_addstr(&arg, *next); + *next += arg.len; + + if (arg.len) { + if (get_sha1(arg.buf, sha1)) + goto invalid; + } else if (flags & PARSE_SHA1_ALLOW_EMPTY) { + /* With -z, treat an empty value as all zeros: */ + hashclr(sha1); + } else { + /* + * With -z, an empty non-required value means + * unspecified: + */ + ret = 1; + } } - return 0; + + strbuf_release(&arg); + + return ret; + + invalid: + die(flags & PARSE_SHA1_OLD ? + "%s %s: invalid : %s" : + "%s %s: invalid : %s", + command, refname, arg.buf); + + eof: + die(flags & PARSE_SHA1_OLD ? + "%s %s missing " : + "%s %s missing ", + command, refname); } @@ -156,8 +194,6 @@ static int parse_next_arg(struct strbuf *input, const char **next, static const char *parse_cmd_update(struct strbuf *input, const char *next) { - struct strbuf newvalue = STRBUF_INIT; - struct strbuf oldvalue = STRBUF_INIT; struct ref_update *update; update = update_alloc(); @@ -166,24 +202,23 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next) if (!update->ref_name) die("update line missing "); - if (!parse_next_arg(input, &next, &newvalue)) - update_store_new_sha1("update", update, newvalue.buf); - else + if (parse_next_sha1(input, &next, update->new_sha1, + "update", update->ref_name, + PARSE_SHA1_ALLOW_EMPTY)) die("update %s missing ", update->ref_name); - if (!parse_next_arg(input, &next, &oldvalue)) { - update_store_old_sha1("update", update, oldvalue.buf); - if (*next != line_termination) - die("update %s has extra input: %s", update->ref_name, next); - } else if (!line_termination) - die("update %s missing ", update->ref_name); + update->have_old = !parse_next_sha1(input, &next, update->old_sha1, + "update", update->ref_name, + PARSE_SHA1_OLD); + + if (*next != line_termination) + die("update %s has extra input: %s", update->ref_name, next); return next; } static const char *parse_cmd_create(struct strbuf *input, const char *next) { - struct strbuf newvalue = STRBUF_INIT; struct ref_update *update; update = update_alloc(); @@ -192,9 +227,8 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next) if (!update->ref_name) die("create line missing "); - if (!parse_next_arg(input, &next, &newvalue)) - update_store_new_sha1("create", update, newvalue.buf); - else + if (parse_next_sha1(input, &next, update->new_sha1, + "create", update->ref_name, 0)) die("create %s missing ", update->ref_name); if (is_null_sha1(update->new_sha1)) @@ -208,7 +242,6 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next) static const char *parse_cmd_delete(struct strbuf *input, const char *next) { - struct strbuf oldvalue = STRBUF_INIT; struct ref_update *update; update = update_alloc(); @@ -217,12 +250,14 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next) if (!update->ref_name) die("delete line missing "); - if (!parse_next_arg(input, &next, &oldvalue)) { - update_store_old_sha1("delete", update, oldvalue.buf); - if (update->have_old && is_null_sha1(update->old_sha1)) + if (parse_next_sha1(input, &next, update->old_sha1, + "delete", update->ref_name, PARSE_SHA1_OLD)) { + update->have_old = 0; + } else { + if (is_null_sha1(update->old_sha1)) die("delete %s given zero ", update->ref_name); - } else if (!line_termination) - die("delete %s missing ", update->ref_name); + update->have_old = 1; + } if (*next != line_termination) die("delete %s has extra input: %s", update->ref_name, next); @@ -232,7 +267,6 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next) static const char *parse_cmd_verify(struct strbuf *input, const char *next) { - struct strbuf value = STRBUF_INIT; struct ref_update *update; update = update_alloc(); @@ -241,11 +275,13 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next) if (!update->ref_name) die("verify line missing "); - if (!parse_next_arg(input, &next, &value)) { - update_store_old_sha1("verify", update, value.buf); + if (parse_next_sha1(input, &next, update->old_sha1, + "verify", update->ref_name, PARSE_SHA1_OLD)) { + update->have_old = 0; + } else { hashcpy(update->new_sha1, update->old_sha1); - } else if (!line_termination) - die("verify %s missing ", update->ref_name); + update->have_old = 1; + } if (*next != line_termination) die("verify %s has extra input: %s", update->ref_name, next); diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 208f56e518..15f5bfd168 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -858,7 +858,7 @@ test_expect_success 'stdin -z create ref fails with bad new value' ' test_must_fail git rev-parse --verify -q $c ' -test_expect_failure 'stdin -z create ref fails with empty new value' ' +test_expect_success 'stdin -z create ref fails with empty new value' ' printf $F "create $c" "" >stdin && test_must_fail git update-ref -z --stdin err && grep "fatal: create $c missing " err && From 1fbd504942b20a541ba4fcbe90d3ea21b03717e4 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:06 +0200 Subject: [PATCH 15/27] update-ref --stdin -z: deprecate interpreting the empty string as zeros In the original version of this command, for the single case of the "update" command's , the empty string was interpreted as being equivalent to 40 "0"s. This shorthand is unnecessary (binary input will usually be generated programmatically anyway), and it complicates the parser and the documentation. So gently deprecate this usage: remove its description from the documentation and emit a warning if it is found. But for reasons of backwards compatibility, continue to accept it. Helped-by: Brad King Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- Documentation/git-update-ref.txt | 18 ++++++++++++------ builtin/update-ref.c | 2 ++ t/t1400-update-ref.sh | 5 +++-- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt index 0a0a5512b3..c8f5ae5cb3 100644 --- a/Documentation/git-update-ref.txt +++ b/Documentation/git-update-ref.txt @@ -68,7 +68,12 @@ performs all modifications together. Specify commands of the form: option SP LF Quote fields containing whitespace as if they were strings in C source -code. Alternatively, use `-z` to specify commands without quoting: +code; i.e., surrounded by double-quotes and with backslash escapes. +Use 40 "0" characters or the empty string to specify a zero value. To +specify a missing value, omit the value and its preceding SP entirely. + +Alternatively, use `-z` to specify in NUL-terminated format, without +quoting: update SP NUL NUL [] NUL create SP NUL NUL @@ -76,8 +81,12 @@ code. Alternatively, use `-z` to specify commands without quoting: verify SP NUL [] NUL option SP NUL -Lines of any other format or a repeated produce an error. -Command meanings are: +In this format, use 40 "0" to specify a zero value, and use the empty +string to specify a missing value. + +In either format, values can be specified in any form that Git +recognizes as an object name. Commands in any other format or a +repeated produce an error. Command meanings are: update:: Set to after verifying , if given. @@ -102,9 +111,6 @@ option:: The only valid option is `no-deref` to avoid dereferencing a symbolic ref. -Use 40 "0" or the empty string to specify a zero value, except that -with `-z` an empty is considered missing. - If all s can be locked with matching s simultaneously, all modifications are performed. Otherwise, no modifications are performed. Note that while each individual diff --git a/builtin/update-ref.c b/builtin/update-ref.c index c61120faf7..6f3b9095cf 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -154,6 +154,8 @@ static int parse_next_sha1(struct strbuf *input, const char **next, goto invalid; } else if (flags & PARSE_SHA1_ALLOW_EMPTY) { /* With -z, treat an empty value as all zeros: */ + warning("%s %s: missing , treating as zero", + command, refname); hashclr(sha1); } else { /* diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 15f5bfd168..2d61cceb4c 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -730,10 +730,11 @@ test_expect_success 'stdin -z fails update with bad ref name' ' grep "fatal: invalid ref format: ~a" err ' -test_expect_success 'stdin -z treats empty new value as zeros' ' +test_expect_success 'stdin -z emits warning with empty new value' ' git update-ref $a $m && printf $F "update $a" "" "" >stdin && - git update-ref -z --stdin err && + grep "warning: update $a: missing , treating as zero" err && test_must_fail git rev-parse --verify -q $a ' From ff6ee39525244b7c1bca1953743fdcc14605a031 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:07 +0200 Subject: [PATCH 16/27] t1400: test one mistake at a time This case wants to test passing a bad refname to the "update" command. But it also passes too few arguments to "update", which muddles the situation: which error should be diagnosed? So split this test into two: * One that passes too few arguments to update * One that passes all three arguments to "update", but with a bad refname. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- t/t1400-update-ref.sh | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 2d61cceb4c..6b21e459e6 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -724,8 +724,14 @@ test_expect_success 'stdin -z fails update with no ref' ' grep "fatal: update line missing " err ' +test_expect_success 'stdin -z fails update with too few args' ' + printf $F "update $a" "$m" >stdin && + test_must_fail git update-ref -z --stdin err && + grep "fatal: update $a missing " err +' + test_expect_success 'stdin -z fails update with bad ref name' ' - printf $F "update ~a" "$m" >stdin && + printf $F "update ~a" "$m" "" >stdin && test_must_fail git update-ref -z --stdin err && grep "fatal: invalid ref format: ~a" err ' From 726f69166ff0ac3fef0a9a0241f09a7d9f0fa48f Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:08 +0200 Subject: [PATCH 17/27] update-ref --stdin: improve the error message for unexpected EOF Distinguish this error from the error that an argument is missing for another reason. Update the tests accordingly. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/update-ref.c | 4 ++-- t/t1400-update-ref.sh | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 6f3b9095cf..0d5f1d076a 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -178,8 +178,8 @@ static int parse_next_sha1(struct strbuf *input, const char **next, eof: die(flags & PARSE_SHA1_OLD ? - "%s %s missing " : - "%s %s missing ", + "%s %s: unexpected end of input when reading " : + "%s %s: unexpected end of input when reading ", command, refname); } diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 6b21e459e6..1db0689548 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -709,7 +709,7 @@ test_expect_success 'stdin -z fails create with bad ref name' ' test_expect_success 'stdin -z fails create with no new value' ' printf $F "create $a" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: create $a missing " err + grep "fatal: create $a: unexpected end of input when reading " err ' test_expect_success 'stdin -z fails create with too many arguments' ' @@ -727,7 +727,7 @@ test_expect_success 'stdin -z fails update with no ref' ' test_expect_success 'stdin -z fails update with too few args' ' printf $F "update $a" "$m" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: update $a missing " err + grep "fatal: update $a: unexpected end of input when reading " err ' test_expect_success 'stdin -z fails update with bad ref name' ' @@ -747,13 +747,13 @@ test_expect_success 'stdin -z emits warning with empty new value' ' test_expect_success 'stdin -z fails update with no new value' ' printf $F "update $a" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: update $a missing " err + grep "fatal: update $a: unexpected end of input when reading " err ' test_expect_success 'stdin -z fails update with no old value' ' printf $F "update $a" "$m" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: update $a missing " err + grep "fatal: update $a: unexpected end of input when reading " err ' test_expect_success 'stdin -z fails update with too many arguments' ' @@ -777,7 +777,7 @@ test_expect_success 'stdin -z fails delete with bad ref name' ' test_expect_success 'stdin -z fails delete with no old value' ' printf $F "delete $a" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: delete $a missing " err + grep "fatal: delete $a: unexpected end of input when reading " err ' test_expect_success 'stdin -z fails delete with too many arguments' ' @@ -795,7 +795,7 @@ test_expect_success 'stdin -z fails verify with too many arguments' ' test_expect_success 'stdin -z fails verify with no old value' ' printf $F "verify $a" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: verify $a missing " err + grep "fatal: verify $a: unexpected end of input when reading " err ' test_expect_success 'stdin -z fails option with unknown name' ' From f11b09fb60556954c6a222f4809631470c81cae6 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:09 +0200 Subject: [PATCH 18/27] update-ref --stdin: harmonize error messages Make (most of) the error messages for invalid input have the same format [1]: $COMMAND [SP $REFNAME]: $MESSAGE Update the tests accordingly. [1] A few error messages are left with their old form, because $COMMAND and $REFNAME aren't passed all the way down the call stack. Maybe those sites should be changed some day, too. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/update-ref.c | 24 ++++++++++++------------ t/t1400-update-ref.sh | 32 ++++++++++++++++---------------- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 0d5f1d076a..423c5c30c7 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -202,19 +202,19 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next) update->ref_name = parse_refname(input, &next); if (!update->ref_name) - die("update line missing "); + die("update: missing "); if (parse_next_sha1(input, &next, update->new_sha1, "update", update->ref_name, PARSE_SHA1_ALLOW_EMPTY)) - die("update %s missing ", update->ref_name); + die("update %s: missing ", update->ref_name); update->have_old = !parse_next_sha1(input, &next, update->old_sha1, "update", update->ref_name, PARSE_SHA1_OLD); if (*next != line_termination) - die("update %s has extra input: %s", update->ref_name, next); + die("update %s: extra input: %s", update->ref_name, next); return next; } @@ -227,17 +227,17 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next) update->ref_name = parse_refname(input, &next); if (!update->ref_name) - die("create line missing "); + die("create: missing "); if (parse_next_sha1(input, &next, update->new_sha1, "create", update->ref_name, 0)) - die("create %s missing ", update->ref_name); + die("create %s: missing ", update->ref_name); if (is_null_sha1(update->new_sha1)) - die("create %s given zero ", update->ref_name); + die("create %s: zero ", update->ref_name); if (*next != line_termination) - die("create %s has extra input: %s", update->ref_name, next); + die("create %s: extra input: %s", update->ref_name, next); return next; } @@ -250,19 +250,19 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next) update->ref_name = parse_refname(input, &next); if (!update->ref_name) - die("delete line missing "); + die("delete: missing "); if (parse_next_sha1(input, &next, update->old_sha1, "delete", update->ref_name, PARSE_SHA1_OLD)) { update->have_old = 0; } else { if (is_null_sha1(update->old_sha1)) - die("delete %s given zero ", update->ref_name); + die("delete %s: zero ", update->ref_name); update->have_old = 1; } if (*next != line_termination) - die("delete %s has extra input: %s", update->ref_name, next); + die("delete %s: extra input: %s", update->ref_name, next); return next; } @@ -275,7 +275,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next) update->ref_name = parse_refname(input, &next); if (!update->ref_name) - die("verify line missing "); + die("verify: missing "); if (parse_next_sha1(input, &next, update->old_sha1, "verify", update->ref_name, PARSE_SHA1_OLD)) { @@ -286,7 +286,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next) } if (*next != line_termination) - die("verify %s has extra input: %s", update->ref_name, next); + die("verify %s: extra input: %s", update->ref_name, next); return next; } diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 1db0689548..48ccc4d635 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -371,7 +371,7 @@ test_expect_success 'stdin fails on junk after quoted argument' ' test_expect_success 'stdin fails create with no ref' ' echo "create " >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: create line missing " err + grep "fatal: create: missing " err ' test_expect_success 'stdin fails create with bad ref name' ' @@ -383,19 +383,19 @@ test_expect_success 'stdin fails create with bad ref name' ' test_expect_success 'stdin fails create with no new value' ' echo "create $a" >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: create $a missing " err + grep "fatal: create $a: missing " err ' test_expect_success 'stdin fails create with too many arguments' ' echo "create $a $m $m" >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: create $a has extra input: $m" err + grep "fatal: create $a: extra input: $m" err ' test_expect_success 'stdin fails update with no ref' ' echo "update " >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: update line missing " err + grep "fatal: update: missing " err ' test_expect_success 'stdin fails update with bad ref name' ' @@ -407,19 +407,19 @@ test_expect_success 'stdin fails update with bad ref name' ' test_expect_success 'stdin fails update with no new value' ' echo "update $a" >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: update $a missing " err + grep "fatal: update $a: missing " err ' test_expect_success 'stdin fails update with too many arguments' ' echo "update $a $m $m $m" >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: update $a has extra input: $m" err + grep "fatal: update $a: extra input: $m" err ' test_expect_success 'stdin fails delete with no ref' ' echo "delete " >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: delete line missing " err + grep "fatal: delete: missing " err ' test_expect_success 'stdin fails delete with bad ref name' ' @@ -431,13 +431,13 @@ test_expect_success 'stdin fails delete with bad ref name' ' test_expect_success 'stdin fails delete with too many arguments' ' echo "delete $a $m $m" >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: delete $a has extra input: $m" err + grep "fatal: delete $a: extra input: $m" err ' test_expect_success 'stdin fails verify with too many arguments' ' echo "verify $a $m $m" >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: verify $a has extra input: $m" err + grep "fatal: verify $a: extra input: $m" err ' test_expect_success 'stdin fails option with unknown name' ' @@ -532,7 +532,7 @@ test_expect_success 'stdin create ref fails with bad new value' ' test_expect_success 'stdin create ref fails with zero new value' ' echo "create $c " >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: create $c given zero " err && + grep "fatal: create $c: zero " err && test_must_fail git rev-parse --verify -q $c ' @@ -556,7 +556,7 @@ test_expect_success 'stdin delete ref fails with wrong old value' ' test_expect_success 'stdin delete ref fails with zero old value' ' echo "delete $a " >stdin && test_must_fail git update-ref --stdin err && - grep "fatal: delete $a given zero " err && + grep "fatal: delete $a: zero " err && git rev-parse $m >expect && git rev-parse $a >actual && test_cmp expect actual @@ -697,7 +697,7 @@ test_expect_success 'stdin -z fails on unknown command' ' test_expect_success 'stdin -z fails create with no ref' ' printf $F "create " >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: create line missing " err + grep "fatal: create: missing " err ' test_expect_success 'stdin -z fails create with bad ref name' ' @@ -721,7 +721,7 @@ test_expect_success 'stdin -z fails create with too many arguments' ' test_expect_success 'stdin -z fails update with no ref' ' printf $F "update " >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: update line missing " err + grep "fatal: update: missing " err ' test_expect_success 'stdin -z fails update with too few args' ' @@ -765,7 +765,7 @@ test_expect_success 'stdin -z fails update with too many arguments' ' test_expect_success 'stdin -z fails delete with no ref' ' printf $F "delete " >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: delete line missing " err + grep "fatal: delete: missing " err ' test_expect_success 'stdin -z fails delete with bad ref name' ' @@ -868,7 +868,7 @@ test_expect_success 'stdin -z create ref fails with bad new value' ' test_expect_success 'stdin -z create ref fails with empty new value' ' printf $F "create $c" "" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: create $c missing " err && + grep "fatal: create $c: missing " err && test_must_fail git rev-parse --verify -q $c ' @@ -892,7 +892,7 @@ test_expect_success 'stdin -z delete ref fails with wrong old value' ' test_expect_success 'stdin -z delete ref fails with zero old value' ' printf $F "delete $a" "$Z" >stdin && test_must_fail git update-ref -z --stdin err && - grep "fatal: delete $a given zero " err && + grep "fatal: delete $a: zero " err && git rev-parse $m >expect && git rev-parse $a >actual && test_cmp expect actual From caa4046c4f480ceae5afb20e3172a437865cc51f Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:10 +0200 Subject: [PATCH 19/27] refs: add a concept of a reference transaction Build out the API for dealing with a bunch of reference checks and changes within a transaction. Define an opaque ref_transaction type that is managed entirely within refs.c. Introduce functions for beginning a transaction, adding updates to a transaction, and committing/rolling back a transaction. This API will soon replace update_refs(). Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ refs.h | 65 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 164 insertions(+) diff --git a/refs.c b/refs.c index 1305eb1fcf..f0b5764b34 100644 --- a/refs.c +++ b/refs.c @@ -3267,6 +3267,96 @@ static int update_ref_write(const char *action, const char *refname, return 0; } +/* + * Data structure for holding a reference transaction, which can + * consist of checks and updates to multiple references, carried out + * as atomically as possible. This structure is opaque to callers. + */ +struct ref_transaction { + struct ref_update **updates; + size_t alloc; + size_t nr; +}; + +struct ref_transaction *ref_transaction_begin(void) +{ + return xcalloc(1, sizeof(struct ref_transaction)); +} + +static void ref_transaction_free(struct ref_transaction *transaction) +{ + int i; + + for (i = 0; i < transaction->nr; i++) { + struct ref_update *update = transaction->updates[i]; + + free((char *)update->ref_name); + free(update); + } + + free(transaction->updates); + free(transaction); +} + +void ref_transaction_rollback(struct ref_transaction *transaction) +{ + ref_transaction_free(transaction); +} + +static struct ref_update *add_update(struct ref_transaction *transaction, + const char *refname) +{ + struct ref_update *update = xcalloc(1, sizeof(*update)); + + update->ref_name = xstrdup(refname); + ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc); + transaction->updates[transaction->nr++] = update; + return update; +} + +void ref_transaction_update(struct ref_transaction *transaction, + const char *refname, + unsigned char *new_sha1, unsigned char *old_sha1, + int flags, int have_old) +{ + struct ref_update *update = add_update(transaction, refname); + + hashcpy(update->new_sha1, new_sha1); + update->flags = flags; + update->have_old = have_old; + if (have_old) + hashcpy(update->old_sha1, old_sha1); +} + +void ref_transaction_create(struct ref_transaction *transaction, + const char *refname, + unsigned char *new_sha1, + int flags) +{ + struct ref_update *update = add_update(transaction, refname); + + assert(!is_null_sha1(new_sha1)); + hashcpy(update->new_sha1, new_sha1); + hashclr(update->old_sha1); + update->flags = flags; + update->have_old = 1; +} + +void ref_transaction_delete(struct ref_transaction *transaction, + const char *refname, + unsigned char *old_sha1, + int flags, int have_old) +{ + struct ref_update *update = add_update(transaction, refname); + + update->flags = flags; + update->have_old = have_old; + if (have_old) { + assert(!is_null_sha1(old_sha1)); + hashcpy(update->old_sha1, old_sha1); + } +} + int update_ref(const char *action, const char *refname, const unsigned char *sha1, const unsigned char *oldval, int flags, enum action_on_err onerr) @@ -3378,6 +3468,15 @@ cleanup: return ret; } +int ref_transaction_commit(struct ref_transaction *transaction, + const char *msg, enum action_on_err onerr) +{ + int ret = update_refs(msg, transaction->updates, transaction->nr, + onerr); + ref_transaction_free(transaction); + return ret; +} + char *shorten_unambiguous_ref(const char *refname, int strict) { int i; diff --git a/refs.h b/refs.h index 08e60ac07f..0518dd5203 100644 --- a/refs.h +++ b/refs.h @@ -24,6 +24,8 @@ struct ref_update { int have_old; /* 1 if old_sha1 is valid, 0 otherwise */ }; +struct ref_transaction; + /* * Bit values set in the flags argument passed to each_ref_fn(): */ @@ -220,6 +222,69 @@ enum action_on_err { UPDATE_REFS_QUIET_ON_ERR }; +/* + * Begin a reference transaction. The reference transaction must + * eventually be commited using ref_transaction_commit() or rolled + * back using ref_transaction_rollback(). + */ +struct ref_transaction *ref_transaction_begin(void); + +/* + * Roll back a ref_transaction and free all associated data. + */ +void ref_transaction_rollback(struct ref_transaction *transaction); + + +/* + * The following functions add a reference check or update to a + * ref_transaction. In all of them, refname is the name of the + * reference to be affected. The functions make internal copies of + * refname, so the caller retains ownership of the parameter. flags + * can be REF_NODEREF; it is passed to update_ref_lock(). + */ + + +/* + * Add a reference update to transaction. new_sha1 is the value that + * the reference should have after the update, or zeros if it should + * be deleted. If have_old is true, then old_sha1 holds the value + * that the reference should have had before the update, or zeros if + * it must not have existed beforehand. + */ +void ref_transaction_update(struct ref_transaction *transaction, + const char *refname, + unsigned char *new_sha1, unsigned char *old_sha1, + int flags, int have_old); + +/* + * Add a reference creation to transaction. new_sha1 is the value + * that the reference should have after the update; it must not be the + * null SHA-1. It is verified that the reference does not exist + * already. + */ +void ref_transaction_create(struct ref_transaction *transaction, + const char *refname, + unsigned char *new_sha1, + int flags); + +/* + * Add a reference deletion to transaction. If have_old is true, then + * old_sha1 holds the value that the reference should have had before + * the update (which must not be the null SHA-1). + */ +void ref_transaction_delete(struct ref_transaction *transaction, + const char *refname, + unsigned char *old_sha1, + int flags, int have_old); + +/* + * Commit all of the changes that have been queued in transaction, as + * atomically as possible. Return a nonzero value if there is a + * problem. The ref_transaction is freed by this function. + */ +int ref_transaction_commit(struct ref_transaction *transaction, + const char *msg, enum action_on_err onerr); + /** Lock a ref and then write its file */ int update_ref(const char *action, const char *refname, const unsigned char *sha1, const unsigned char *oldval, From aebfc13337e503b5a7d064cb1e9c9916f24c2baf Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:11 +0200 Subject: [PATCH 20/27] update-ref --stdin: reimplement using reference transactions This change is mostly clerical: the parse_cmd_*() functions need to use local variables rather than a struct ref_update to collect the arguments needed for each update, and then call ref_transaction_*() to queue the change rather than building up the list of changes at the caller side. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/update-ref.c | 142 +++++++++++++++++++++++-------------------- 1 file changed, 75 insertions(+), 67 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 423c5c30c7..405267f6e2 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -12,29 +12,11 @@ static const char * const git_update_ref_usage[] = { NULL }; -static int updates_alloc; -static int updates_count; -static struct ref_update **updates; +static struct ref_transaction *transaction; static char line_termination = '\n'; static int update_flags; -static struct ref_update *update_alloc(void) -{ - struct ref_update *update; - - /* Allocate and zero-init a struct ref_update */ - update = xcalloc(1, sizeof(*update)); - ALLOC_GROW(updates, updates_count + 1, updates_alloc); - updates[updates_count++] = update; - - /* Store and reset accumulated options */ - update->flags = update_flags; - update_flags = 0; - - return update; -} - /* * Parse one whitespace- or NUL-terminated, possibly C-quoted argument * and append the result to arg. Return a pointer to the terminator. @@ -196,97 +178,119 @@ static int parse_next_sha1(struct strbuf *input, const char **next, static const char *parse_cmd_update(struct strbuf *input, const char *next) { - struct ref_update *update; - - update = update_alloc(); + char *refname; + unsigned char new_sha1[20]; + unsigned char old_sha1[20]; + int have_old; - update->ref_name = parse_refname(input, &next); - if (!update->ref_name) + refname = parse_refname(input, &next); + if (!refname) die("update: missing "); - if (parse_next_sha1(input, &next, update->new_sha1, - "update", update->ref_name, + if (parse_next_sha1(input, &next, new_sha1, "update", refname, PARSE_SHA1_ALLOW_EMPTY)) - die("update %s: missing ", update->ref_name); + die("update %s: missing ", refname); - update->have_old = !parse_next_sha1(input, &next, update->old_sha1, - "update", update->ref_name, - PARSE_SHA1_OLD); + have_old = !parse_next_sha1(input, &next, old_sha1, "update", refname, + PARSE_SHA1_OLD); if (*next != line_termination) - die("update %s: extra input: %s", update->ref_name, next); + die("update %s: extra input: %s", refname, next); + + ref_transaction_update(transaction, refname, new_sha1, old_sha1, + update_flags, have_old); + + update_flags = 0; + free(refname); return next; } static const char *parse_cmd_create(struct strbuf *input, const char *next) { - struct ref_update *update; - - update = update_alloc(); + char *refname; + unsigned char new_sha1[20]; - update->ref_name = parse_refname(input, &next); - if (!update->ref_name) + refname = parse_refname(input, &next); + if (!refname) die("create: missing "); - if (parse_next_sha1(input, &next, update->new_sha1, - "create", update->ref_name, 0)) - die("create %s: missing ", update->ref_name); + if (parse_next_sha1(input, &next, new_sha1, "create", refname, 0)) + die("create %s: missing ", refname); - if (is_null_sha1(update->new_sha1)) - die("create %s: zero ", update->ref_name); + if (is_null_sha1(new_sha1)) + die("create %s: zero ", refname); if (*next != line_termination) - die("create %s: extra input: %s", update->ref_name, next); + die("create %s: extra input: %s", refname, next); + + ref_transaction_create(transaction, refname, new_sha1, update_flags); + + update_flags = 0; + free(refname); return next; } static const char *parse_cmd_delete(struct strbuf *input, const char *next) { - struct ref_update *update; + char *refname; + unsigned char old_sha1[20]; + int have_old; - update = update_alloc(); - - update->ref_name = parse_refname(input, &next); - if (!update->ref_name) + refname = parse_refname(input, &next); + if (!refname) die("delete: missing "); - if (parse_next_sha1(input, &next, update->old_sha1, - "delete", update->ref_name, PARSE_SHA1_OLD)) { - update->have_old = 0; + if (parse_next_sha1(input, &next, old_sha1, "delete", refname, + PARSE_SHA1_OLD)) { + have_old = 0; } else { - if (is_null_sha1(update->old_sha1)) - die("delete %s: zero ", update->ref_name); - update->have_old = 1; + if (is_null_sha1(old_sha1)) + die("delete %s: zero ", refname); + have_old = 1; } if (*next != line_termination) - die("delete %s: extra input: %s", update->ref_name, next); + die("delete %s: extra input: %s", refname, next); + + ref_transaction_delete(transaction, refname, old_sha1, + update_flags, have_old); + + update_flags = 0; + free(refname); return next; } static const char *parse_cmd_verify(struct strbuf *input, const char *next) { - struct ref_update *update; - - update = update_alloc(); + char *refname; + unsigned char new_sha1[20]; + unsigned char old_sha1[20]; + int have_old; - update->ref_name = parse_refname(input, &next); - if (!update->ref_name) + refname = parse_refname(input, &next); + if (!refname) die("verify: missing "); - if (parse_next_sha1(input, &next, update->old_sha1, - "verify", update->ref_name, PARSE_SHA1_OLD)) { - update->have_old = 0; + if (parse_next_sha1(input, &next, old_sha1, "verify", refname, + PARSE_SHA1_OLD)) { + hashclr(new_sha1); + have_old = 0; } else { - hashcpy(update->new_sha1, update->old_sha1); - update->have_old = 1; + hashcpy(new_sha1, old_sha1); + have_old = 1; } if (*next != line_termination) - die("verify %s: extra input: %s", update->ref_name, next); + die("verify %s: extra input: %s", refname, next); + + ref_transaction_update(transaction, refname, new_sha1, old_sha1, + update_flags, have_old); + + update_flags = 0; + free(refname); return next; } @@ -355,13 +359,17 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) die("Refusing to perform update with empty message."); if (read_stdin) { + int ret; + transaction = ref_transaction_begin(); + if (delete || no_deref || argc > 0) usage_with_options(git_update_ref_usage, options); if (end_null) line_termination = '\0'; update_refs_stdin(); - return update_refs(msg, updates, updates_count, - UPDATE_REFS_DIE_ON_ERR); + ret = ref_transaction_commit(transaction, msg, + UPDATE_REFS_DIE_ON_ERR); + return ret; } if (end_null) From b5c8ea2afb9bea910f0db5f9a4dfe58471184b3d Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:12 +0200 Subject: [PATCH 21/27] refs: remove API function update_refs() It has been superseded by reference transactions. This also means that struct ref_update can become private. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 33 ++++++++++++++++++++------------- refs.h | 20 -------------------- 2 files changed, 20 insertions(+), 33 deletions(-) diff --git a/refs.c b/refs.c index f0b5764b34..6984ff0aff 100644 --- a/refs.c +++ b/refs.c @@ -3267,6 +3267,20 @@ static int update_ref_write(const char *action, const char *refname, return 0; } +/** + * Information needed for a single ref update. Set new_sha1 to the + * new value or to zero to delete the ref. To check the old value + * while locking the ref, set have_old to 1 and set old_sha1 to the + * value or to zero to ensure the ref does not exist before update. + */ +struct ref_update { + const char *ref_name; + unsigned char new_sha1[20]; + unsigned char old_sha1[20]; + int flags; /* REF_NODEREF? */ + int have_old; /* 1 if old_sha1 is valid, 0 otherwise */ +}; + /* * Data structure for holding a reference transaction, which can * consist of checks and updates to multiple references, carried out @@ -3396,16 +3410,17 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n, return 0; } -int update_refs(const char *action, struct ref_update * const *updates_orig, - int n, enum action_on_err onerr) +int ref_transaction_commit(struct ref_transaction *transaction, + const char *msg, enum action_on_err onerr) { int ret = 0, delnum = 0, i; struct ref_update **updates; int *types; struct ref_lock **locks; const char **delnames; + int n = transaction->nr; - if (!updates_orig || !n) + if (!n) return 0; /* Allocate work space */ @@ -3415,7 +3430,7 @@ int update_refs(const char *action, struct ref_update * const *updates_orig, delnames = xmalloc(sizeof(*delnames) * n); /* Copy, sort, and reject duplicate refs */ - memcpy(updates, updates_orig, sizeof(*updates) * n); + memcpy(updates, transaction->updates, sizeof(*updates) * n); qsort(updates, n, sizeof(*updates), ref_update_compare); ret = ref_update_reject_duplicates(updates, n, onerr); if (ret) @@ -3437,7 +3452,7 @@ int update_refs(const char *action, struct ref_update * const *updates_orig, /* Perform updates first so live commits remain referenced */ for (i = 0; i < n; i++) if (!is_null_sha1(updates[i]->new_sha1)) { - ret = update_ref_write(action, + ret = update_ref_write(msg, updates[i]->ref_name, updates[i]->new_sha1, locks[i], onerr); @@ -3465,14 +3480,6 @@ cleanup: free(types); free(locks); free(delnames); - return ret; -} - -int ref_transaction_commit(struct ref_transaction *transaction, - const char *msg, enum action_on_err onerr) -{ - int ret = update_refs(msg, transaction->updates, transaction->nr, - onerr); ref_transaction_free(transaction); return ret; } diff --git a/refs.h b/refs.h index 0518dd5203..cb799a39f7 100644 --- a/refs.h +++ b/refs.h @@ -10,20 +10,6 @@ struct ref_lock { int force_write; }; -/** - * Information needed for a single ref update. Set new_sha1 to the - * new value or to zero to delete the ref. To check the old value - * while locking the ref, set have_old to 1 and set old_sha1 to the - * value or to zero to ensure the ref does not exist before update. - */ -struct ref_update { - const char *ref_name; - unsigned char new_sha1[20]; - unsigned char old_sha1[20]; - int flags; /* REF_NODEREF? */ - int have_old; /* 1 if old_sha1 is valid, 0 otherwise */ -}; - struct ref_transaction; /* @@ -290,12 +276,6 @@ int update_ref(const char *action, const char *refname, const unsigned char *sha1, const unsigned char *oldval, int flags, enum action_on_err onerr); -/** - * Lock all refs and then perform all modifications. - */ -int update_refs(const char *action, struct ref_update * const *updates, - int n, enum action_on_err onerr); - extern int parse_hide_refs_config(const char *var, const char *value, const char *); extern int ref_is_hidden(const char *); From 5524e2416ec97f8c6d1a2fc12ee857efa9641175 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:13 +0200 Subject: [PATCH 22/27] struct ref_update: rename field "ref_name" to "refname" This is consistent with the usual nomenclature. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 18 +++++++++--------- refs.h | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/refs.c b/refs.c index 6984ff0aff..b6778aaa12 100644 --- a/refs.c +++ b/refs.c @@ -3274,7 +3274,7 @@ static int update_ref_write(const char *action, const char *refname, * value or to zero to ensure the ref does not exist before update. */ struct ref_update { - const char *ref_name; + const char *refname; unsigned char new_sha1[20]; unsigned char old_sha1[20]; int flags; /* REF_NODEREF? */ @@ -3304,7 +3304,7 @@ static void ref_transaction_free(struct ref_transaction *transaction) for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; - free((char *)update->ref_name); + free((char *)update->refname); free(update); } @@ -3322,7 +3322,7 @@ static struct ref_update *add_update(struct ref_transaction *transaction, { struct ref_update *update = xcalloc(1, sizeof(*update)); - update->ref_name = xstrdup(refname); + update->refname = xstrdup(refname); ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc); transaction->updates[transaction->nr++] = update; return update; @@ -3386,7 +3386,7 @@ static int ref_update_compare(const void *r1, const void *r2) { const struct ref_update * const *u1 = r1; const struct ref_update * const *u2 = r2; - return strcmp((*u1)->ref_name, (*u2)->ref_name); + return strcmp((*u1)->refname, (*u2)->refname); } static int ref_update_reject_duplicates(struct ref_update **updates, int n, @@ -3394,14 +3394,14 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n, { int i; for (i = 1; i < n; i++) - if (!strcmp(updates[i - 1]->ref_name, updates[i]->ref_name)) { + if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) { const char *str = "Multiple updates for ref '%s' not allowed."; switch (onerr) { case UPDATE_REFS_MSG_ON_ERR: - error(str, updates[i]->ref_name); break; + error(str, updates[i]->refname); break; case UPDATE_REFS_DIE_ON_ERR: - die(str, updates[i]->ref_name); break; + die(str, updates[i]->refname); break; case UPDATE_REFS_QUIET_ON_ERR: break; } @@ -3438,7 +3438,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, /* Acquire all locks while verifying old values */ for (i = 0; i < n; i++) { - locks[i] = update_ref_lock(updates[i]->ref_name, + locks[i] = update_ref_lock(updates[i]->refname, (updates[i]->have_old ? updates[i]->old_sha1 : NULL), updates[i]->flags, @@ -3453,7 +3453,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, for (i = 0; i < n; i++) if (!is_null_sha1(updates[i]->new_sha1)) { ret = update_ref_write(msg, - updates[i]->ref_name, + updates[i]->refname, updates[i]->new_sha1, locks[i], onerr); locks[i] = NULL; /* freed by update_ref_write */ diff --git a/refs.h b/refs.h index cb799a39f7..0f08def210 100644 --- a/refs.h +++ b/refs.h @@ -154,7 +154,7 @@ extern void unlock_ref(struct ref_lock *lock); extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg); /** Setup reflog before using. **/ -int log_ref_setup(const char *ref_name, char *logfile, int bufsize); +int log_ref_setup(const char *refname, char *logfile, int bufsize); /** Reads log for the value of ref during at_time. **/ extern int read_ref_at(const char *refname, unsigned long at_time, int cnt, From 88615910db7f700ae437318308a8631888bd28cd Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:14 +0200 Subject: [PATCH 23/27] struct ref_update: store refname as a FLEX_ARRAY Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/refs.c b/refs.c index b6778aaa12..2ff195f471 100644 --- a/refs.c +++ b/refs.c @@ -3274,11 +3274,11 @@ static int update_ref_write(const char *action, const char *refname, * value or to zero to ensure the ref does not exist before update. */ struct ref_update { - const char *refname; unsigned char new_sha1[20]; unsigned char old_sha1[20]; int flags; /* REF_NODEREF? */ int have_old; /* 1 if old_sha1 is valid, 0 otherwise */ + const char refname[FLEX_ARRAY]; }; /* @@ -3301,12 +3301,8 @@ static void ref_transaction_free(struct ref_transaction *transaction) { int i; - for (i = 0; i < transaction->nr; i++) { - struct ref_update *update = transaction->updates[i]; - - free((char *)update->refname); - free(update); - } + for (i = 0; i < transaction->nr; i++) + free(transaction->updates[i]); free(transaction->updates); free(transaction); @@ -3320,9 +3316,10 @@ void ref_transaction_rollback(struct ref_transaction *transaction) static struct ref_update *add_update(struct ref_transaction *transaction, const char *refname) { - struct ref_update *update = xcalloc(1, sizeof(*update)); + size_t len = strlen(refname); + struct ref_update *update = xcalloc(1, sizeof(*update) + len + 1); - update->refname = xstrdup(refname); + strcpy((char *)update->refname, refname); ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc); transaction->updates[transaction->nr++] = update; return update; From cb198d21d3848f0c5f3d85a471a6a6793e540ca4 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:15 +0200 Subject: [PATCH 24/27] ref_transaction_commit(): simplify code using temporary variables Use temporary variables in the for-loop blocks to simplify expressions in the rest of the loop. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/refs.c b/refs.c index 2ff195f471..33c34dfeff 100644 --- a/refs.c +++ b/refs.c @@ -3435,10 +3435,12 @@ int ref_transaction_commit(struct ref_transaction *transaction, /* Acquire all locks while verifying old values */ for (i = 0; i < n; i++) { - locks[i] = update_ref_lock(updates[i]->refname, - (updates[i]->have_old ? - updates[i]->old_sha1 : NULL), - updates[i]->flags, + struct ref_update *update = updates[i]; + + locks[i] = update_ref_lock(update->refname, + (update->have_old ? + update->old_sha1 : NULL), + update->flags, &types[i], onerr); if (!locks[i]) { ret = 1; @@ -3447,16 +3449,19 @@ int ref_transaction_commit(struct ref_transaction *transaction, } /* Perform updates first so live commits remain referenced */ - for (i = 0; i < n; i++) - if (!is_null_sha1(updates[i]->new_sha1)) { + for (i = 0; i < n; i++) { + struct ref_update *update = updates[i]; + + if (!is_null_sha1(update->new_sha1)) { ret = update_ref_write(msg, - updates[i]->refname, - updates[i]->new_sha1, + update->refname, + update->new_sha1, locks[i], onerr); locks[i] = NULL; /* freed by update_ref_write */ if (ret) goto cleanup; } + } /* Perform deletes now that updates are safely completed */ for (i = 0; i < n; i++) From 81c960e4dcbd69e28b031cbe370100cb28acb911 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:16 +0200 Subject: [PATCH 25/27] struct ref_update: add a lock field Now that we manage ref_update objects internally, we can use them to hold some of the scratch space we need when actually carrying out the updates. Store the (struct ref_lock *) there. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/refs.c b/refs.c index 33c34dfeff..6fe4bfe8d9 100644 --- a/refs.c +++ b/refs.c @@ -3278,6 +3278,7 @@ struct ref_update { unsigned char old_sha1[20]; int flags; /* REF_NODEREF? */ int have_old; /* 1 if old_sha1 is valid, 0 otherwise */ + struct ref_lock *lock; const char refname[FLEX_ARRAY]; }; @@ -3413,7 +3414,6 @@ int ref_transaction_commit(struct ref_transaction *transaction, int ret = 0, delnum = 0, i; struct ref_update **updates; int *types; - struct ref_lock **locks; const char **delnames; int n = transaction->nr; @@ -3423,7 +3423,6 @@ int ref_transaction_commit(struct ref_transaction *transaction, /* Allocate work space */ updates = xmalloc(sizeof(*updates) * n); types = xmalloc(sizeof(*types) * n); - locks = xcalloc(n, sizeof(*locks)); delnames = xmalloc(sizeof(*delnames) * n); /* Copy, sort, and reject duplicate refs */ @@ -3437,12 +3436,12 @@ int ref_transaction_commit(struct ref_transaction *transaction, for (i = 0; i < n; i++) { struct ref_update *update = updates[i]; - locks[i] = update_ref_lock(update->refname, - (update->have_old ? - update->old_sha1 : NULL), - update->flags, - &types[i], onerr); - if (!locks[i]) { + update->lock = update_ref_lock(update->refname, + (update->have_old ? + update->old_sha1 : NULL), + update->flags, + &types[i], onerr); + if (!update->lock) { ret = 1; goto cleanup; } @@ -3456,19 +3455,23 @@ int ref_transaction_commit(struct ref_transaction *transaction, ret = update_ref_write(msg, update->refname, update->new_sha1, - locks[i], onerr); - locks[i] = NULL; /* freed by update_ref_write */ + update->lock, onerr); + update->lock = NULL; /* freed by update_ref_write */ if (ret) goto cleanup; } } /* Perform deletes now that updates are safely completed */ - for (i = 0; i < n; i++) - if (locks[i]) { - delnames[delnum++] = locks[i]->ref_name; - ret |= delete_ref_loose(locks[i], types[i]); + for (i = 0; i < n; i++) { + struct ref_update *update = updates[i]; + + if (update->lock) { + delnames[delnum++] = update->lock->ref_name; + ret |= delete_ref_loose(update->lock, types[i]); } + } + ret |= repack_without_refs(delnames, delnum); for (i = 0; i < delnum; i++) unlink_or_warn(git_path("logs/%s", delnames[i])); @@ -3476,11 +3479,10 @@ int ref_transaction_commit(struct ref_transaction *transaction, cleanup: for (i = 0; i < n; i++) - if (locks[i]) - unlock_ref(locks[i]); + if (updates[i]->lock) + unlock_ref(updates[i]->lock); free(updates); free(types); - free(locks); free(delnames); ref_transaction_free(transaction); return ret; From 84178db76f34d0b8d363545f6e18c99f6e37df4e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:17 +0200 Subject: [PATCH 26/27] struct ref_update: add a type field It used to be that ref_transaction_commit() allocated a temporary array to hold the types of references while it is working. Instead, add a type field to ref_update that ref_transaction_commit() can use as its scratch space. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/refs.c b/refs.c index 6fe4bfe8d9..c058f304de 100644 --- a/refs.c +++ b/refs.c @@ -3279,6 +3279,7 @@ struct ref_update { int flags; /* REF_NODEREF? */ int have_old; /* 1 if old_sha1 is valid, 0 otherwise */ struct ref_lock *lock; + int type; const char refname[FLEX_ARRAY]; }; @@ -3413,7 +3414,6 @@ int ref_transaction_commit(struct ref_transaction *transaction, { int ret = 0, delnum = 0, i; struct ref_update **updates; - int *types; const char **delnames; int n = transaction->nr; @@ -3422,7 +3422,6 @@ int ref_transaction_commit(struct ref_transaction *transaction, /* Allocate work space */ updates = xmalloc(sizeof(*updates) * n); - types = xmalloc(sizeof(*types) * n); delnames = xmalloc(sizeof(*delnames) * n); /* Copy, sort, and reject duplicate refs */ @@ -3440,7 +3439,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, (update->have_old ? update->old_sha1 : NULL), update->flags, - &types[i], onerr); + &update->type, onerr); if (!update->lock) { ret = 1; goto cleanup; @@ -3468,7 +3467,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, if (update->lock) { delnames[delnum++] = update->lock->ref_name; - ret |= delete_ref_loose(update->lock, types[i]); + ret |= delete_ref_loose(update->lock, update->type); } } @@ -3482,7 +3481,6 @@ cleanup: if (updates[i]->lock) unlock_ref(updates[i]->lock); free(updates); - free(types); free(delnames); ref_transaction_free(transaction); return ret; From 6a402338ec9ca0369e1801533dda2108689ceaaf Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:18 +0200 Subject: [PATCH 27/27] ref_transaction_commit(): work with transaction->updates in place Now that we free the transaction when we are done, there is no need to make a copy of transaction->updates before working with it. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/refs.c b/refs.c index c058f304de..728a761648 100644 --- a/refs.c +++ b/refs.c @@ -3413,19 +3413,17 @@ int ref_transaction_commit(struct ref_transaction *transaction, const char *msg, enum action_on_err onerr) { int ret = 0, delnum = 0, i; - struct ref_update **updates; const char **delnames; int n = transaction->nr; + struct ref_update **updates = transaction->updates; if (!n) return 0; /* Allocate work space */ - updates = xmalloc(sizeof(*updates) * n); delnames = xmalloc(sizeof(*delnames) * n); /* Copy, sort, and reject duplicate refs */ - memcpy(updates, transaction->updates, sizeof(*updates) * n); qsort(updates, n, sizeof(*updates), ref_update_compare); ret = ref_update_reject_duplicates(updates, n, onerr); if (ret) @@ -3480,7 +3478,6 @@ cleanup: for (i = 0; i < n; i++) if (updates[i]->lock) unlock_ref(updates[i]->lock); - free(updates); free(delnames); ref_transaction_free(transaction); return ret;