From b984d333a1367078c3631b8dfdd0d0ec7b332715 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:47:52 +0200 Subject: 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 diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 6ffd82f..fa927d2 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 ' -- cgit v0.10.2-6-g49f6 From c13291108880f9adc26a2ab5c09535869afecb22 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:47:53 +0200 Subject: 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 diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index fa927d2..29391c6 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 && -- cgit v0.10.2-6-g49f6 From 697a41519b0d41c1b2c5714b5558f68814d78885 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:47:54 +0200 Subject: 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 diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 1292cfe..02b5f95 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 29391c6..774f8c5 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' ' -- cgit v0.10.2-6-g49f6 From 20fcffcc8de9fcfba15fce916ff38c98ca20323d Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:47:55 +0200 Subject: 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 diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 774f8c5..00862bc 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: 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 diff --git a/builtin/checkout.c b/builtin/checkout.c index 1b86d9c..6bf2318 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 9b3c04d..b12989d 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 e15d0e1..7d1d83e 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 bb89930..66147b6 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 f4e0875..f368266 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 02b5f95..f6345e5 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 8bc8c75..ee19166 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 eabe4a0..97dfd63 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 4aa7023..a0b1d7b 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 28d5eca..196984e 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 87a1a79..a713b34 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); -- cgit v0.10.2-6-g49f6 From 595deb8da69b4f816ff0c8e669b49f7527ff609b Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:47:57 +0200 Subject: 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 diff --git a/builtin/update-ref.c b/builtin/update-ref.c index f6345e5..a8a68e8 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 196984e..1305eb1 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 a713b34..08e60ac 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 *); -- cgit v0.10.2-6-g49f6 From e23d84350a3f4a3bfb86037eb1e6c4b28240324e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:47:58 +0200 Subject: 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 diff --git a/builtin/update-ref.c b/builtin/update-ref.c index a8a68e8..5f197fe 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) -- cgit v0.10.2-6-g49f6 From 2f57736002e6a774ce5ab53a15a631da8299f8b4 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:47:59 +0200 Subject: 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 diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 5f197fe..51adf2d 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); -- cgit v0.10.2-6-g49f6 From ed410e611dc39dc7e845f27a660b76b7d0ecbab6 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:00 +0200 Subject: 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 diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 51adf2d..0dc2061 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; } -- cgit v0.10.2-6-g49f6 From 1746ef4e9d869fb0f54194bb225604eb61a77501 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:01 +0200 Subject: 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 diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 0dc2061..13a884a 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 00862bc..f6c6e96 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 ' -- cgit v0.10.2-6-g49f6 From 9255f059ff0bfce7605748f62f58d6ba9055a1f3 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:02 +0200 Subject: 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 diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 13a884a..e4c0854 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 f6c6e96..ef61fe3 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 -- cgit v0.10.2-6-g49f6 From ac1177553d8c632e93c507f8efc80b80e6c7d3d8 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:03 +0200 Subject: 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 diff --git a/builtin/update-ref.c b/builtin/update-ref.c index e4c0854..a9eb5fe 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 ef61fe3..a2015d0 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' ' -- cgit v0.10.2-6-g49f6 From 191f241b528c10e242d045bde2cef70fb013a6e5 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:04 +0200 Subject: 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 diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index a2015d0..208f56e 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 && -- cgit v0.10.2-6-g49f6 From 3afcc4637452100c68b469de7757dd2b45b4d29c Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:05 +0200 Subject: 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 diff --git a/builtin/update-ref.c b/builtin/update-ref.c index a9eb5fe..c61120f 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 208f56e..15f5bfd 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 && -- cgit v0.10.2-6-g49f6 From 1fbd504942b20a541ba4fcbe90d3ea21b03717e4 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:06 +0200 Subject: 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 diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt index 0a0a551..c8f5ae5 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 c61120f..6f3b909 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 15f5bfd..2d61cce 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 ' -- cgit v0.10.2-6-g49f6 From ff6ee39525244b7c1bca1953743fdcc14605a031 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:07 +0200 Subject: 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 diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 2d61cce..6b21e45 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 ' -- cgit v0.10.2-6-g49f6 From 726f69166ff0ac3fef0a9a0241f09a7d9f0fa48f Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:08 +0200 Subject: 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 diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 6f3b909..0d5f1d0 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 6b21e45..1db0689 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' ' -- cgit v0.10.2-6-g49f6 From f11b09fb60556954c6a222f4809631470c81cae6 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:09 +0200 Subject: 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 diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 0d5f1d0..423c5c3 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 1db0689..48ccc4d 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 -- cgit v0.10.2-6-g49f6 From caa4046c4f480ceae5afb20e3172a437865cc51f Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:10 +0200 Subject: 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 diff --git a/refs.c b/refs.c index 1305eb1..f0b5764 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 08e60ac..0518dd5 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, -- cgit v0.10.2-6-g49f6 From aebfc13337e503b5a7d064cb1e9c9916f24c2baf Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:11 +0200 Subject: 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 diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 423c5c3..405267f 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) -- cgit v0.10.2-6-g49f6 From b5c8ea2afb9bea910f0db5f9a4dfe58471184b3d Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:12 +0200 Subject: 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 diff --git a/refs.c b/refs.c index f0b5764..6984ff0 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 0518dd5..cb799a3 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 *); -- cgit v0.10.2-6-g49f6 From 5524e2416ec97f8c6d1a2fc12ee857efa9641175 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:13 +0200 Subject: 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 diff --git a/refs.c b/refs.c index 6984ff0..b6778aa 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 cb799a3..0f08def 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, -- cgit v0.10.2-6-g49f6 From 88615910db7f700ae437318308a8631888bd28cd Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:14 +0200 Subject: struct ref_update: store refname as a FLEX_ARRAY Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano diff --git a/refs.c b/refs.c index b6778aa..2ff195f 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; -- cgit v0.10.2-6-g49f6 From cb198d21d3848f0c5f3d85a471a6a6793e540ca4 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:15 +0200 Subject: 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 diff --git a/refs.c b/refs.c index 2ff195f..33c34df 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++) -- cgit v0.10.2-6-g49f6 From 81c960e4dcbd69e28b031cbe370100cb28acb911 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:16 +0200 Subject: 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 diff --git a/refs.c b/refs.c index 33c34df..6fe4bfe8 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; -- cgit v0.10.2-6-g49f6 From 84178db76f34d0b8d363545f6e18c99f6e37df4e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:17 +0200 Subject: 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 diff --git a/refs.c b/refs.c index 6fe4bfe8..c058f30 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; -- cgit v0.10.2-6-g49f6 From 6a402338ec9ca0369e1801533dda2108689ceaaf Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Mon, 7 Apr 2014 15:48:18 +0200 Subject: 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 diff --git a/refs.c b/refs.c index c058f30..728a761 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; -- cgit v0.10.2-6-g49f6