[-- Attachment #1: Type: text/plain, Size: 2910 bytes --] Hi, inspired by recent discussions about handling transactions in git-update-refs(1), this series implements proper transaction support in `git update-refs --stdin`. The goal is to have an all-or-nothing transaction where a user can queue multiple updates, verify that they can be committed, and then finally either commits or aborts the transaction. A typical session would look like the following: # Start the transaction > start < start: ok # Queue updates > delete refs/heads/branch > create refs/heads/another $OID1 # Prepare the transaction. git-update-ref will now try to allocate # all locks and verify that references are at their expected values. > prepare < prepare: ok # Commit the transaction. The user could also have said "abort" to # roll back everything. > commit < commit: ok The series builds on the already existing transaction support in refs.c and exposes it to the user. The most important change that was required to support this was to convert `git-update-ref --stdin` to handle input linewise instead of trying to read it in full and only acting after stdin was closed. The series is structured as follows: Patches 1-2: Preparatory patches which make sense as standalone patches. Patches 3-7: Preparatory patches that make it easier to convert to reading commands in a line-wise fashion. No functional changes are expected. Patch 8: Conversion to read commands line-wise. No functional changes are expected, except that Git builds up the transaction while reading stdin instead of waiting for stdin to be closed first. Patch 9: Implementation of transactional commands. All in all, the new transactional support will only be enabled if the user invokes any the new commands "start", "prepare", "commit" or "abort". In case he doesn't, no functional changes are expected. Patrick Patrick Steinhardt (9): refs: fix segfault when aborting empty transaction git-update-ref.txt: add missing word strbuf: provide function to append whole lines update-ref: organize commands in an array update-ref: drop unused argument for `parse_refname` update-ref: pass end pointer instead of strbuf update-ref: move transaction handling into `update_refs_stdin()` update-ref: read commands in a line-wise fashion update-ref: implement interactive transaction handling Documentation/git-update-ref.txt | 28 +++- builtin/update-ref.c | 255 ++++++++++++++++++++++--------- refs/files-backend.c | 20 +-- strbuf.c | 10 ++ strbuf.h | 6 + t/t1400-update-ref.sh | 131 ++++++++++++++++ 6 files changed, 370 insertions(+), 80 deletions(-) -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1707 bytes --] When cleaning up a transaction that has no updates queued, then the transaction's backend data will not have been allocated. We correctly handle this for the packed backend, where the cleanup function checks whether the backend data has been allocated at all -- if not, then there is nothing to clean up. For the files backend we do not check this and as a result will hit a segfault due to dereferencing a `NULL` pointer when cleaning up such a transaction. Fix the issue by checking whether `backend_data` is set in the files backend, too. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- refs/files-backend.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 561c33ac8a..6516c7bc8c 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2565,17 +2565,19 @@ static void files_transaction_cleanup(struct files_ref_store *refs, } } - if (backend_data->packed_transaction && - ref_transaction_abort(backend_data->packed_transaction, &err)) { - error("error aborting transaction: %s", err.buf); - strbuf_release(&err); + if (backend_data) { + if (backend_data->packed_transaction && + ref_transaction_abort(backend_data->packed_transaction, &err)) { + error("error aborting transaction: %s", err.buf); + strbuf_release(&err); + } + + if (backend_data->packed_refs_locked) + packed_refs_unlock(refs->packed_ref_store); + + free(backend_data); } - if (backend_data->packed_refs_locked) - packed_refs_unlock(refs->packed_ref_store); - - free(backend_data); - transaction->state = REF_TRANSACTION_CLOSED; } -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 749 bytes --] The description for the "verify" command is lacking a single word "is", which this commit corrects. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Documentation/git-update-ref.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt index 9671423117..9bd039ce08 100644 --- a/Documentation/git-update-ref.txt +++ b/Documentation/git-update-ref.txt @@ -107,7 +107,7 @@ delete:: verify:: Verify <ref> against <oldvalue> but do not change it. If - <oldvalue> zero or missing, the ref must not exist. + <oldvalue> is zero or missing, the ref must not exist. option:: Modify behavior of the next command naming a <ref>. -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1765 bytes --] While the strbuf interface already provides functions to read a line into it that completely replaces its current contents, we do not have an interface that allows for appending lines without discarding current contents. Add a new function `strbuf_appendwholeline` that reads a line including its terminating character into a strbuf non-destructively. This is a preparatory step for git-update-ref(1) reading standard input line-wise instead of as a block. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- strbuf.c | 10 ++++++++++ strbuf.h | 6 ++++++ 2 files changed, 16 insertions(+) diff --git a/strbuf.c b/strbuf.c index bb0065ccaf..6e74901bfa 100644 --- a/strbuf.c +++ b/strbuf.c @@ -690,6 +690,16 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term) } #endif +int strbuf_appendwholeline(struct strbuf *sb, FILE *fp, int term) +{ + struct strbuf line = STRBUF_INIT; + if (strbuf_getwholeline(&line, fp, term)) + return EOF; + strbuf_addbuf(sb, &line); + strbuf_release(&line); + return 0; +} + static int strbuf_getdelim(struct strbuf *sb, FILE *fp, int term) { if (strbuf_getwholeline(sb, fp, term)) diff --git a/strbuf.h b/strbuf.h index ce8e49c0b2..411063ca76 100644 --- a/strbuf.h +++ b/strbuf.h @@ -502,6 +502,12 @@ int strbuf_getline(struct strbuf *sb, FILE *file); */ int strbuf_getwholeline(struct strbuf *sb, FILE *file, int term); +/** + * Like `strbuf_getwholeline`, but appends the line instead of + * resetting the buffer first. + */ +int strbuf_appendwholeline(struct strbuf *sb, FILE *file, int term); + /** * Like `strbuf_getwholeline`, but operates on a file descriptor. * It reads one character at a time, so it is very slow. Do not -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 5687 bytes --] We currently manually wire up all commands known to `git-update-ref --stdin`, making it harder than necessary to preprocess arguments after the command is determined. To make this more extensible, let's refactor the code to use an array of known commands instead. While this doesn't add a lot of value now, it is a preparatory step to implement line-wise reading of commands. As we're going to introduce commands without trailing spaces, this commit also moves whitespace parsing into the respective commands. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/update-ref.c | 66 ++++++++++++++++++++++++++++++++------------ 1 file changed, 49 insertions(+), 17 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 2d8f7f0578..d2b917a47c 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -171,11 +171,11 @@ static int parse_next_oid(struct strbuf *input, 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. + * command name. 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 ref_transaction *transaction, @@ -186,6 +186,9 @@ static const char *parse_cmd_update(struct ref_transaction *transaction, struct object_id new_oid, old_oid; int have_old; + if (!skip_prefix(next, " ", &next)) + die("update: missing space after command"); + refname = parse_refname(input, &next); if (!refname) die("update: missing <ref>"); @@ -220,6 +223,9 @@ static const char *parse_cmd_create(struct ref_transaction *transaction, char *refname; struct object_id new_oid; + if (!skip_prefix(next, " ", &next)) + die("create: missing space after command"); + refname = parse_refname(input, &next); if (!refname) die("create: missing <ref>"); @@ -253,6 +259,9 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction, struct object_id old_oid; int have_old; + if (!skip_prefix(next, " ", &next)) + die("delete: missing space after command"); + refname = parse_refname(input, &next); if (!refname) die("delete: missing <ref>"); @@ -288,6 +297,9 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, char *refname; struct object_id old_oid; + if (!skip_prefix(next, " ", &next)) + die("verify: missing space after command"); + refname = parse_refname(input, &next); if (!refname) die("verify: missing <ref>"); @@ -310,9 +322,12 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, return next; } -static const char *parse_cmd_option(struct strbuf *input, const char *next) +static const char *parse_cmd_option(struct ref_transaction *transaction, + struct strbuf *input, const char *next) { const char *rest; + if (!skip_prefix(next, " ", &next)) + die("option: missing space after command"); if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination) update_flags |= REF_NO_DEREF; else @@ -320,33 +335,50 @@ static const char *parse_cmd_option(struct strbuf *input, const char *next) return rest; } +static const struct parse_cmd { + const char *prefix; + const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *); +} commands[] = { + { "update", parse_cmd_update }, + { "create", parse_cmd_create }, + { "delete", parse_cmd_delete }, + { "verify", parse_cmd_verify }, + { "option", parse_cmd_option }, +}; + static void update_refs_stdin(struct ref_transaction *transaction) { struct strbuf input = STRBUF_INIT; const char *next; + int i; if (strbuf_read(&input, 0, 1000) < 0) die_errno("could not read from stdin"); next = input.buf; /* Read each line dispatch its command */ while (next < input.buf + input.len) { + const struct parse_cmd *cmd = NULL; + if (*next == line_termination) die("empty command in input"); else if (isspace(*next)) die("whitespace before command: %s", next); - else if (skip_prefix(next, "update ", &next)) - next = parse_cmd_update(transaction, &input, next); - else if (skip_prefix(next, "create ", &next)) - next = parse_cmd_create(transaction, &input, next); - else if (skip_prefix(next, "delete ", &next)) - next = parse_cmd_delete(transaction, &input, next); - else if (skip_prefix(next, "verify ", &next)) - next = parse_cmd_verify(transaction, &input, next); - else if (skip_prefix(next, "option ", &next)) - next = parse_cmd_option(&input, next); - else + + for (i = 0; i < ARRAY_SIZE(commands); i++) { + if (!skip_prefix(next, commands[i].prefix , &next)) + continue; + cmd = &commands[i]; + break; + } + if (!cmd) die("unknown command: %s", next); + if (input.buf[strlen(cmd->prefix)] != line_termination && + input.buf[strlen(cmd->prefix)] != '\0' && + input.buf[strlen(cmd->prefix)] != ' ') + die("%s: no separator after command", cmd->prefix); + + next = cmd->fn(transaction, &input, next); next++; } -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 2180 bytes --] The `parse_refname` function accepts a `struct strbuf *input` argument that isn't used at all. As we're about to convert commands to not use a strbuf anymore but instead an end pointer, let's drop this argument now to make the converting commit easier to review. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/update-ref.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index d2b917a47c..6f4e8e88c1 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -50,7 +50,7 @@ static const char *parse_arg(const char *next, struct strbuf *arg) * the argument. Die if C-quoting is malformed or the reference name * is invalid. */ -static char *parse_refname(struct strbuf *input, const char **next) +static char *parse_refname(const char **next) { struct strbuf ref = STRBUF_INIT; @@ -189,7 +189,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction, if (!skip_prefix(next, " ", &next)) die("update: missing space after command"); - refname = parse_refname(input, &next); + refname = parse_refname(&next); if (!refname) die("update: missing <ref>"); @@ -226,7 +226,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction, if (!skip_prefix(next, " ", &next)) die("create: missing space after command"); - refname = parse_refname(input, &next); + refname = parse_refname(&next); if (!refname) die("create: missing <ref>"); @@ -262,7 +262,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction, if (!skip_prefix(next, " ", &next)) die("delete: missing space after command"); - refname = parse_refname(input, &next); + refname = parse_refname(&next); if (!refname) die("delete: missing <ref>"); @@ -300,7 +300,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, if (!skip_prefix(next, " ", &next)) die("verify: missing space after command"); - refname = parse_refname(input, &next); + refname = parse_refname(&next); if (!refname) die("verify: missing <ref>"); -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 5642 bytes --] We currently pass both an `strbuf` containing the current command line as well as the `next` pointer pointing to the first argument to commands. This is both confusing and makes code more intertwined. Convert this to use a simple pointer as well as a pointer pointing to the end of the input as a preparatory step to line-wise reading of stdin. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/update-ref.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 6f4e8e88c1..6f036ae765 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -95,7 +95,7 @@ static char *parse_refname(const char **next) * 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_oid(struct strbuf *input, const char **next, +static int parse_next_oid(const char **next, const char *end, struct object_id *oid, const char *command, const char *refname, int flags) @@ -103,7 +103,7 @@ static int parse_next_oid(struct strbuf *input, const char **next, struct strbuf arg = STRBUF_INIT; int ret = 0; - if (*next == input->buf + input->len) + if (*next == end) goto eof; if (line_termination) { @@ -128,7 +128,7 @@ static int parse_next_oid(struct strbuf *input, const char **next, die("%s %s: expected NUL but got: %s", command, refname, *next); (*next)++; - if (*next == input->buf + input->len) + if (*next == end) goto eof; strbuf_addstr(&arg, *next); *next += arg.len; @@ -179,7 +179,7 @@ static int parse_next_oid(struct strbuf *input, const char **next, */ static const char *parse_cmd_update(struct ref_transaction *transaction, - struct strbuf *input, const char *next) + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -193,11 +193,11 @@ static const char *parse_cmd_update(struct ref_transaction *transaction, if (!refname) die("update: missing <ref>"); - if (parse_next_oid(input, &next, &new_oid, "update", refname, + if (parse_next_oid(&next, end, &new_oid, "update", refname, PARSE_SHA1_ALLOW_EMPTY)) die("update %s: missing <newvalue>", refname); - have_old = !parse_next_oid(input, &next, &old_oid, "update", refname, + have_old = !parse_next_oid(&next, end, &old_oid, "update", refname, PARSE_SHA1_OLD); if (*next != line_termination) @@ -217,7 +217,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction, } static const char *parse_cmd_create(struct ref_transaction *transaction, - struct strbuf *input, const char *next) + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -230,7 +230,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction, if (!refname) die("create: missing <ref>"); - if (parse_next_oid(input, &next, &new_oid, "create", refname, 0)) + if (parse_next_oid(&next, end, &new_oid, "create", refname, 0)) die("create %s: missing <newvalue>", refname); if (is_null_oid(&new_oid)) @@ -252,7 +252,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction, } static const char *parse_cmd_delete(struct ref_transaction *transaction, - struct strbuf *input, const char *next) + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -266,7 +266,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction, if (!refname) die("delete: missing <ref>"); - if (parse_next_oid(input, &next, &old_oid, "delete", refname, + if (parse_next_oid(&next, end, &old_oid, "delete", refname, PARSE_SHA1_OLD)) { have_old = 0; } else { @@ -291,7 +291,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction, } static const char *parse_cmd_verify(struct ref_transaction *transaction, - struct strbuf *input, const char *next) + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -304,7 +304,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, if (!refname) die("verify: missing <ref>"); - if (parse_next_oid(input, &next, &old_oid, "verify", refname, + if (parse_next_oid(&next, end, &old_oid, "verify", refname, PARSE_SHA1_OLD)) oidclr(&old_oid); @@ -323,7 +323,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, } static const char *parse_cmd_option(struct ref_transaction *transaction, - struct strbuf *input, const char *next) + const char *next, const char *end) { const char *rest; if (!skip_prefix(next, " ", &next)) @@ -337,7 +337,7 @@ static const char *parse_cmd_option(struct ref_transaction *transaction, static const struct parse_cmd { const char *prefix; - const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *); + const char *(*fn)(struct ref_transaction *, const char *, const char *); } commands[] = { { "update", parse_cmd_update }, { "create", parse_cmd_create }, @@ -378,7 +378,7 @@ static void update_refs_stdin(struct ref_transaction *transaction) input.buf[strlen(cmd->prefix)] != ' ') die("%s: no separator after command", cmd->prefix); - next = cmd->fn(transaction, &input, next); + next = cmd->fn(transaction, next, input.buf + input.len); next++; } -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 2473 bytes --] While the actual logic to update the transaction is handled in `update_refs_stdin()`, the transaction itself is started and committed in `cmd_update_ref()` itself. This makes it hard to handle transaction abortion and commits as part of `update_refs_stdin()` itself, which is required in order to introduce transaction handling features to `git update-refs --stdin`. Refactor the code to move all transaction handling into `update_refs_stdin()` to prepare for transaction handling features. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/update-ref.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 6f036ae765..5f97bb0b75 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -346,15 +346,21 @@ static const struct parse_cmd { { "option", parse_cmd_option }, }; -static void update_refs_stdin(struct ref_transaction *transaction) +static void update_refs_stdin(void) { - struct strbuf input = STRBUF_INIT; + struct strbuf input = STRBUF_INIT, err = STRBUF_INIT; + struct ref_transaction *transaction; const char *next; int i; if (strbuf_read(&input, 0, 1000) < 0) die_errno("could not read from stdin"); next = input.buf; + + transaction = ref_transaction_begin(&err); + if (!transaction) + die("%s", err.buf); + /* Read each line dispatch its command */ while (next < input.buf + input.len) { const struct parse_cmd *cmd = NULL; @@ -382,6 +388,11 @@ static void update_refs_stdin(struct ref_transaction *transaction) next++; } + if (ref_transaction_commit(transaction, &err)) + die("%s", err.buf); + + ref_transaction_free(transaction); + strbuf_release(&err); strbuf_release(&input); } @@ -416,21 +427,11 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) } if (read_stdin) { - struct strbuf err = STRBUF_INIT; - struct ref_transaction *transaction; - - transaction = ref_transaction_begin(&err); - if (!transaction) - die("%s", err.buf); if (delete || argc > 0) usage_with_options(git_update_ref_usage, options); if (end_null) line_termination = '\0'; - update_refs_stdin(transaction); - if (ref_transaction_commit(transaction, &err)) - die("%s", err.buf); - ref_transaction_free(transaction); - strbuf_release(&err); + update_refs_stdin(); return 0; } -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 6626 bytes --] The git-update-ref(1) supports a `--stdin` mode that allows it to read all reference updates from standard input. This is mainly used to allow for atomic reference updates that are all or nothing, so that either all references will get updated or none. Currently, git-update-ref(1) reads all commands as a single block of up to 1000 characters and only starts processing after stdin gets closed. This is less flexible than one might wish for, as it doesn't really allow for longer-lived transactions and doesn't allow any verification without committing everything. E.g. one may imagine the following exchange: > start < start: ok > update refs/heads/master $NEWOID1 $OLDOID1 > update refs/heads/branch $NEWOID2 $OLDOID2 > prepare < prepare: ok > commit < commit: ok When reading all input as a whole block, the above interactive protocol is obviously impossible to achieve. But by converting the command to read commands linewise, we can make it more interactive than before. Obviously, the linewise interface is only a first step in making git-update-ref(1) work in a more transaction-oriented way. Missing is most importantly support for transactional commands that manage the current transaction. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/update-ref.c | 70 ++++++++++++++++++++------------------------ 1 file changed, 31 insertions(+), 39 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 5f97bb0b75..f35e3ca5ae 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -178,8 +178,8 @@ static int parse_next_oid(const char **next, const char *end, * line_termination is set. */ -static const char *parse_cmd_update(struct ref_transaction *transaction, - const char *next, const char *end) +static void parse_cmd_update(struct ref_transaction *transaction, + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -212,12 +212,10 @@ static const char *parse_cmd_update(struct ref_transaction *transaction, update_flags = default_flags; free(refname); strbuf_release(&err); - - return next; } -static const char *parse_cmd_create(struct ref_transaction *transaction, - const char *next, const char *end) +static void parse_cmd_create(struct ref_transaction *transaction, + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -247,12 +245,10 @@ static const char *parse_cmd_create(struct ref_transaction *transaction, update_flags = default_flags; free(refname); strbuf_release(&err); - - return next; } -static const char *parse_cmd_delete(struct ref_transaction *transaction, - const char *next, const char *end) +static void parse_cmd_delete(struct ref_transaction *transaction, + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -286,12 +282,10 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction, update_flags = default_flags; free(refname); strbuf_release(&err); - - return next; } -static const char *parse_cmd_verify(struct ref_transaction *transaction, - const char *next, const char *end) +static void parse_cmd_verify(struct ref_transaction *transaction, + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -318,12 +312,10 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, update_flags = default_flags; free(refname); strbuf_release(&err); - - return next; } -static const char *parse_cmd_option(struct ref_transaction *transaction, - const char *next, const char *end) +static void parse_cmd_option(struct ref_transaction *transaction, + const char *next, const char *end) { const char *rest; if (!skip_prefix(next, " ", &next)) @@ -332,60 +324,60 @@ static const char *parse_cmd_option(struct ref_transaction *transaction, update_flags |= REF_NO_DEREF; else die("option unknown: %s", next); - return rest; } static const struct parse_cmd { const char *prefix; - const char *(*fn)(struct ref_transaction *, const char *, const char *); + void (*fn)(struct ref_transaction *, const char *, const char *); + unsigned extra_lines; } commands[] = { - { "update", parse_cmd_update }, - { "create", parse_cmd_create }, - { "delete", parse_cmd_delete }, - { "verify", parse_cmd_verify }, - { "option", parse_cmd_option }, + { "update", parse_cmd_update, 2 }, + { "create", parse_cmd_create, 1 }, + { "delete", parse_cmd_delete, 1 }, + { "verify", parse_cmd_verify, 1 }, + { "option", parse_cmd_option, 0 }, }; static void update_refs_stdin(void) { struct strbuf input = STRBUF_INIT, err = STRBUF_INIT; struct ref_transaction *transaction; - const char *next; - int i; - - if (strbuf_read(&input, 0, 1000) < 0) - die_errno("could not read from stdin"); - next = input.buf; + int i, j; transaction = ref_transaction_begin(&err); if (!transaction) die("%s", err.buf); /* Read each line dispatch its command */ - while (next < input.buf + input.len) { + while (!strbuf_getwholeline(&input, stdin, line_termination)) { const struct parse_cmd *cmd = NULL; - if (*next == line_termination) + if (*input.buf == line_termination) die("empty command in input"); - else if (isspace(*next)) - die("whitespace before command: %s", next); + else if (isspace(*input.buf)) + die("whitespace before command: %s", input.buf); for (i = 0; i < ARRAY_SIZE(commands); i++) { - if (!skip_prefix(next, commands[i].prefix , &next)) + if (!starts_with(input.buf, commands[i].prefix)) continue; cmd = &commands[i]; break; } if (!cmd) - die("unknown command: %s", next); + die("unknown command: %s", input.buf); if (input.buf[strlen(cmd->prefix)] != line_termination && input.buf[strlen(cmd->prefix)] != '\0' && input.buf[strlen(cmd->prefix)] != ' ') die("%s: no separator after command", cmd->prefix); - next = cmd->fn(transaction, next, input.buf + input.len); - next++; + /* Read extra lines if NUL-terminated */ + for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++) + if (strbuf_appendwholeline(&input, stdin, line_termination)) + break; + + cmd->fn(transaction, input.buf + strlen(cmd->prefix), + input.buf + input.len); } if (ref_transaction_commit(transaction, &err)) -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 12314 bytes --] The git-update-ref(1) command can only handle queueing transactions right now via its "--stdin" parameter, but there is no way for users to handle the transaction itself in a more explicit way. E.g. in a replicated scenario, one may imagine a coordinator that spawns git-update-ref(1) for multiple repositories and only if all agree that an update is possible will the coordinator send a commit. Such a transactional session could look like > start < start: ok > update refs/heads/master $OLD $NEW > prepare < prepare: ok # All nodes have returned "ok" > commit < commit: ok or > start < start: ok > create refs/heads/master $OLD $NEW > prepare < fatal: cannot lock ref 'refs/heads/master': reference already exists # On all other nodes: > abort < abort: ok In order to allow for such transactional sessions, this commit introduces four new commands for git-update-ref(1), which matches those we have internally already with the exception of "start": - start: start a new transaction - prepare: prepare the transaction, that is try to lock all references and verify their current value matches the expected one - commit: explicitly commit a session, that is update references to match their new expected state - abort: abort a session and roll back all changes By design, git-update-ref(1) will commit as soon as standard input is being closed. While fine in a non-transactional world, it is definitely unexpected in a transactional world. Because of this, as soon as any of the new transactional commands is used, the default will change to aborting without an explicit "commit". To avoid a race between queueing updates and the first "prepare" that starts a transaction, the "start" command has been added to start an explicit transaction. Add some tests to exercise this new functionality. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Documentation/git-update-ref.txt | 26 ++++++ builtin/update-ref.c | 108 ++++++++++++++++++++++--- t/t1400-update-ref.sh | 131 +++++++++++++++++++++++++++++++ 3 files changed, 256 insertions(+), 9 deletions(-) diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt index 9bd039ce08..3e737c2360 100644 --- a/Documentation/git-update-ref.txt +++ b/Documentation/git-update-ref.txt @@ -66,6 +66,10 @@ performs all modifications together. Specify commands of the form: delete SP <ref> [SP <oldvalue>] LF verify SP <ref> [SP <oldvalue>] LF option SP <opt> LF + start LF + prepare LF + commit LF + abort LF With `--create-reflog`, update-ref will create a reflog for each ref even if one would not ordinarily be created. @@ -83,6 +87,10 @@ quoting: delete SP <ref> NUL [<oldvalue>] NUL verify SP <ref> NUL [<oldvalue>] NUL option SP <opt> NUL + start NUL + prepare NUL + commit NUL + abort NUL In this format, use 40 "0" to specify a zero value, and use the empty string to specify a missing value. @@ -114,6 +122,24 @@ option:: The only valid option is `no-deref` to avoid dereferencing a symbolic ref. +start:: + Start a transaction. In contrast to a non-transactional session, a + transaction will automatically abort if the session ends without an + explicit commit. + +prepare:: + Prepare to commit the transaction. This will create lock files for all + queued reference updates. If one reference could not be locked, the + transaction will be aborted. + +commit:: + Commit all reference updates queued for the transaction, ending the + transaction. + +abort:: + Abort the transaction, releasing all locks if the transaction is in + prepared state. + If all <ref>s can be locked with matching <oldvalue>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 f35e3ca5ae..6b870507e0 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -326,21 +326,80 @@ static void parse_cmd_option(struct ref_transaction *transaction, die("option unknown: %s", next); } +static void parse_cmd_start(struct ref_transaction *transaction, + const char *next, const char *end) +{ + if (*next != line_termination) + die("start: extra input: %s", next); + puts("start: ok"); +} + +static void parse_cmd_prepare(struct ref_transaction *transaction, + const char *next, const char *end) +{ + struct strbuf error = STRBUF_INIT; + if (*next != line_termination) + die("prepare: extra input: %s", next); + if (ref_transaction_prepare(transaction, &error)) + die("prepare: %s", error.buf); + puts("prepare: ok"); +} + +static void parse_cmd_abort(struct ref_transaction *transaction, + const char *next, const char *end) +{ + struct strbuf error = STRBUF_INIT; + if (*next != line_termination) + die("abort: extra input: %s", next); + if (ref_transaction_abort(transaction, &error)) + die("abort: %s", error.buf); + puts("abort: ok"); +} + +static void parse_cmd_commit(struct ref_transaction *transaction, + const char *next, const char *end) +{ + struct strbuf error = STRBUF_INIT; + if (*next != line_termination) + die("commit: extra input: %s", next); + if (ref_transaction_commit(transaction, &error)) + die("commit: %s", error.buf); + puts("commit: ok"); + ref_transaction_free(transaction); +} + +enum update_refs_state { + /* Non-transactional state open for updates. */ + UPDATE_REFS_OPEN, + /* A transaction has been started. */ + UPDATE_REFS_STARTED, + /* References are locked and ready for commit */ + UPDATE_REFS_PREPARED, + /* Transaction has been committed or closed. */ + UPDATE_REFS_CLOSED, +}; + static const struct parse_cmd { const char *prefix; void (*fn)(struct ref_transaction *, const char *, const char *); unsigned extra_lines; + enum update_refs_state state; } commands[] = { - { "update", parse_cmd_update, 2 }, - { "create", parse_cmd_create, 1 }, - { "delete", parse_cmd_delete, 1 }, - { "verify", parse_cmd_verify, 1 }, - { "option", parse_cmd_option, 0 }, + { "update", parse_cmd_update, 2, UPDATE_REFS_OPEN }, + { "create", parse_cmd_create, 1, UPDATE_REFS_OPEN }, + { "delete", parse_cmd_delete, 1, UPDATE_REFS_OPEN }, + { "verify", parse_cmd_verify, 1, UPDATE_REFS_OPEN }, + { "option", parse_cmd_option, 0, UPDATE_REFS_OPEN }, + { "start", parse_cmd_start, 0, UPDATE_REFS_STARTED }, + { "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED }, + { "abort", parse_cmd_abort, 0, UPDATE_REFS_CLOSED }, + { "commit", parse_cmd_commit, 0, UPDATE_REFS_CLOSED }, }; static void update_refs_stdin(void) { struct strbuf input = STRBUF_INIT, err = STRBUF_INIT; + enum update_refs_state state = UPDATE_REFS_OPEN; struct ref_transaction *transaction; int i, j; @@ -371,19 +430,50 @@ static void update_refs_stdin(void) input.buf[strlen(cmd->prefix)] != ' ') die("%s: no separator after command", cmd->prefix); - /* Read extra lines if NUL-terminated */ + /* Read extra lines if NUL-terminated, but let commands handle missing ones. */ for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++) if (strbuf_appendwholeline(&input, stdin, line_termination)) break; + switch (state) { + case UPDATE_REFS_OPEN: + case UPDATE_REFS_STARTED: + /* Do not downgrade a transaction to a non-transaction. */ + if (cmd->state >= state) + state = cmd->state; + break; + case UPDATE_REFS_PREPARED: + if (cmd->state != UPDATE_REFS_CLOSED) + die("prepared transactions can only be closed"); + state = cmd->state; + break; + case UPDATE_REFS_CLOSED: + die("transaction is closed"); + break; + } + cmd->fn(transaction, input.buf + strlen(cmd->prefix), input.buf + input.len); } - if (ref_transaction_commit(transaction, &err)) - die("%s", err.buf); + switch (state) { + case UPDATE_REFS_OPEN: + /* Commit by default if no transaction was requested. */ + if (ref_transaction_commit(transaction, &err)) + die("%s", err.buf); + ref_transaction_free(transaction); + break; + case UPDATE_REFS_STARTED: + case UPDATE_REFS_PREPARED: + /* If using a transaction, we want to abort it. */ + if (ref_transaction_abort(transaction, &err)) + die("%s", err.buf); + break; + case UPDATE_REFS_CLOSED: + /* Otherwise no need to do anything, the transaction was closed already. */ + break; + } - ref_transaction_free(transaction); strbuf_release(&err); strbuf_release(&input); } diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index a6224ef65f..48d0d42afd 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -1404,4 +1404,135 @@ test_expect_success 'handle per-worktree refs in refs/bisect' ' ! test_cmp main-head worktree-head ' +test_expect_success 'transaction handles empty commit' ' + cat >stdin <<-EOF && + start + prepare + commit + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start prepare commit >expect && + test_cmp expect actual +' + +test_expect_success 'transaction handles empty commit with missing prepare' ' + cat >stdin <<-EOF && + start + commit + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start commit >expect && + test_cmp expect actual +' + +test_expect_success 'transaction handles sole commit' ' + cat >stdin <<-EOF && + commit + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" commit >expect && + test_cmp expect actual +' + +test_expect_success 'transaction handles empty abort' ' + cat >stdin <<-EOF && + start + prepare + abort + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start prepare abort >expect && + test_cmp expect actual +' + +test_expect_success 'transaction exits on multiple aborts' ' + cat >stdin <<-EOF && + abort + abort + EOF + test_must_fail git update-ref --stdin <stdin >actual 2>err && + printf "%s: ok\n" abort >expect && + test_cmp expect actual && + grep "fatal: transaction is closed" err +' + +test_expect_success 'transaction exits on start after prepare' ' + cat >stdin <<-EOF && + prepare + start + EOF + test_must_fail git update-ref --stdin <stdin 2>err >actual && + printf "%s: ok\n" prepare >expect && + test_cmp expect actual && + grep "fatal: prepared transactions can only be closed" err +' + +test_expect_success 'transaction handles empty abort with missing prepare' ' + cat >stdin <<-EOF && + start + abort + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start abort >expect && + test_cmp expect actual +' + +test_expect_success 'transaction handles sole abort' ' + cat >stdin <<-EOF && + abort + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" abort >expect && + test_cmp expect actual +' + +test_expect_success 'transaction can handle commit' ' + cat >stdin <<-EOF && + start + create $a HEAD + commit + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start commit >expect && + test_cmp expect actual && + git rev-parse HEAD >expect && + git rev-parse $a >actual && + test_cmp expect actual +' + +test_expect_success 'transaction can handle abort' ' + cat >stdin <<-EOF && + start + create $b HEAD + abort + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start abort >expect && + test_cmp expect actual && + test_path_is_missing .git/$b +' + +test_expect_success 'transaction aborts by default' ' + cat >stdin <<-EOF && + start + create $b HEAD + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start >expect && + test_cmp expect actual && + test_path_is_missing .git/$b +' + +test_expect_success 'transaction with prepare aborts by default' ' + cat >stdin <<-EOF && + start + create $b HEAD + prepare + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start prepare >expect && + test_cmp expect actual && + test_path_is_missing .git/$b +' + test_done -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
Patrick Steinhardt <ps@pks.im> writes: > When cleaning up a transaction that has no updates queued, then the > transaction's backend data will not have been allocated. We correctly > handle this for the packed backend, where the cleanup function checks > whether the backend data has been allocated at all -- if not, then there > is nothing to clean up. For the files backend we do not check this and > as a result will hit a segfault due to dereferencing a `NULL` pointer > when cleaning up such a transaction. > > Fix the issue by checking whether `backend_data` is set in the files > backend, too. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > refs/files-backend.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) Makes sense. Will queue. Thanks. > > diff --git a/refs/files-backend.c b/refs/files-backend.c > index 561c33ac8a..6516c7bc8c 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -2565,17 +2565,19 @@ static void files_transaction_cleanup(struct files_ref_store *refs, > } > } > > - if (backend_data->packed_transaction && > - ref_transaction_abort(backend_data->packed_transaction, &err)) { > - error("error aborting transaction: %s", err.buf); > - strbuf_release(&err); > + if (backend_data) { > + if (backend_data->packed_transaction && > + ref_transaction_abort(backend_data->packed_transaction, &err)) { > + error("error aborting transaction: %s", err.buf); > + strbuf_release(&err); > + } > + > + if (backend_data->packed_refs_locked) > + packed_refs_unlock(refs->packed_ref_store); > + > + free(backend_data); > } > > - if (backend_data->packed_refs_locked) > - packed_refs_unlock(refs->packed_ref_store); > - > - free(backend_data); > - > transaction->state = REF_TRANSACTION_CLOSED; > }
Patrick Steinhardt <ps@pks.im> writes: > While the strbuf interface already provides functions to read a line > into it that completely replaces its current contents, we do not have an > interface that allows for appending lines without discarding current > contents. > > Add a new function `strbuf_appendwholeline` that reads a line including > its terminating character into a strbuf non-destructively. This is a > preparatory step for git-update-ref(1) reading standard input line-wise > instead of as a block. Hmph. If we were to gain more uses of this function over time, the necessity for an extra strbuf_addbuf() becomes annoying. I wonder if we should be doing this patch the other way around, i.e. move the whole implementation, except for the early if (feof(fp)) return EOF; strbuf_reset(sb); part, and call it strbuf_addwholeline(), and strbuf_getwholeline() becomes a thin wrapper around it that resets the incoming buffer before calling strbuf_addwholeline(). The logic to determine EOF would need to be updated if we go that route (i.e. instead of checking sb->len is zero in the !HAVE_GETDELIM side of the implementation, we need to see if the number is the same as before) but it should be minor. There is a stale comment in the HAVE_GETDELIM side of the curent implementation, by the way. I think our xrealloc no longer tries to "recover" from ENOMEM. Having said all that, I am fine with this version, at least for now. > +int strbuf_appendwholeline(struct strbuf *sb, FILE *fp, int term) > +{ > + struct strbuf line = STRBUF_INIT; > + if (strbuf_getwholeline(&line, fp, term)) > + return EOF; So, if caller wants to read the stream until it runs out, it can do strbuf_init(&sb); while (strbuf_appendwholeline(&sb, fp, '\n') != EOF) ; /* keep going */ If the caller knows how many lines to read, EOF-return can be used only for error checking, e.g. strbuf_init(&sb); for (count = 0; count < 5; count++) if (strbuf_appendwholeline(&sb, fp, '\n') == EOF) die("cannot grab 5 lines"); It becomes cumbersome if the input lines are terminated, though. The caller wouldn't be using strbuf_appendwholeline() interface, e.g. strbuf_init(&accum); while ((strbuf_getwholeline(&sb, fp, '\n') != EOF) && strcmp(sb.buf, "done\n")) strbuf_addbuf(&accum, &sb); Anyway, I was primarily wondering if returning EOF, which perfectly makes sense for getwholeline(), still makes sense for addwholeline(), and it seems that it is still useful. > + strbuf_addbuf(sb, &line); > + strbuf_release(&line); > + return 0; > +} > + > static int strbuf_getdelim(struct strbuf *sb, FILE *fp, int term) > { > if (strbuf_getwholeline(sb, fp, term)) > diff --git a/strbuf.h b/strbuf.h > index ce8e49c0b2..411063ca76 100644 > --- a/strbuf.h > +++ b/strbuf.h > @@ -502,6 +502,12 @@ int strbuf_getline(struct strbuf *sb, FILE *file); > */ > int strbuf_getwholeline(struct strbuf *sb, FILE *file, int term); > > +/** > + * Like `strbuf_getwholeline`, but appends the line instead of > + * resetting the buffer first. > + */ > +int strbuf_appendwholeline(struct strbuf *sb, FILE *file, int term); > + > /** > * Like `strbuf_getwholeline`, but operates on a file descriptor. > * It reads one character at a time, so it is very slow. Do not
Patrick Steinhardt <ps@pks.im> writes: > +static const struct parse_cmd { > + const char *prefix; > + const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *); > +} commands[] = { Do not call an array the represents a set of THINGs "type things[]"; instead call it "type thing[]", so that the 0th thing can be referred to as thing[0], not things[0]. One exception is when the set as a whole is referred to more often than individual element of an array, in which case "things" (without the [index]) becomes a sensible way to refer to the set. > + { "update", parse_cmd_update }, > + { "create", parse_cmd_create }, > + { "delete", parse_cmd_delete }, > + { "verify", parse_cmd_verify }, > + { "option", parse_cmd_option }, > +}; > + > static void update_refs_stdin(struct ref_transaction *transaction) > { > struct strbuf input = STRBUF_INIT; > const char *next; > + int i; > > if (strbuf_read(&input, 0, 1000) < 0) > die_errno("could not read from stdin"); > next = input.buf; > /* Read each line dispatch its command */ > while (next < input.buf + input.len) { > + const struct parse_cmd *cmd = NULL; > + > if (*next == line_termination) > die("empty command in input"); > else if (isspace(*next)) > die("whitespace before command: %s", next); > - else if (skip_prefix(next, "update ", &next)) > - next = parse_cmd_update(transaction, &input, next); > - else if (skip_prefix(next, "create ", &next)) > - next = parse_cmd_create(transaction, &input, next); > - else if (skip_prefix(next, "delete ", &next)) > - next = parse_cmd_delete(transaction, &input, next); > - else if (skip_prefix(next, "verify ", &next)) > - next = parse_cmd_verify(transaction, &input, next); > - else if (skip_prefix(next, "option ", &next)) > - next = parse_cmd_option(&input, next); > - else > + > + for (i = 0; i < ARRAY_SIZE(commands); i++) { > + if (!skip_prefix(next, commands[i].prefix , &next)) > + continue; > + cmd = &commands[i]; > + break; > + } The only reason why you had to sprinkle if (!skip_prefix(next, " ", &next)) die(_("%s: missing space after command"), cmd); all over the place is because the table lacks the trailing SP (which makes sense---after all, you are making a table of commands). In other words, it's not like some command called from this dispatcher would require " " after the command name and some others would not. So why not avoid touching the parse_cmd_<cmd>() at all (except for the "option" thing that now needs to take the transaction object for uniformity), and then verify the presence of " " here, perhaps like this: for (i = 0; i < ARRAY_SIZE(command); i++) { const char *eoc; if (!skip_prefix(next, commands[i].prefix, &eoc) || *eoc != ' ') continue; cmd = &command[i]; next = eoc; break; } Note that you cannot reuse &next here to future-proof the code; otherwise, you wouldn't be able to add a new command, e.g. "options", that sits next to the existing command "option", in the future. > + if (!cmd) > die("unknown command: %s", next); > > + if (input.buf[strlen(cmd->prefix)] != line_termination && > + input.buf[strlen(cmd->prefix)] != '\0' && > + input.buf[strlen(cmd->prefix)] != ' ') > + die("%s: no separator after command", cmd->prefix); This part of your version does not make sense to me. If the input line began with "update" with some separator that is not a SP, the original would not have matched. But with this code, "update\0" would pass this code and cause cmd->fn() to be called, only to get the input rejected, because next is not pointing to SP. So why do you even need this if statement? > + > + next = cmd->fn(transaction, &input, next); > next++; > } Thanks.
Patrick Steinhardt <ps@pks.im> writes:
> While the actual logic to update the transaction is handled in
> `update_refs_stdin()`, the transaction itself is started and committed
> in `cmd_update_ref()` itself. This makes it hard to handle transaction
> abortion and commits as part of `update_refs_stdin()` itself, ...
It is not offhand clear why it makes any difference, especially
because there is only a single caller of update_refs_stdin()
function, but let's see how this move makes things easier in [8/9]
and [9/9].
The patch itself looks like a bug-free no-op change.
Thanks.
Patrick Steinhardt <ps@pks.im> writes: > static const struct parse_cmd { > const char *prefix; > - const char *(*fn)(struct ref_transaction *, const char *, const char *); > + void (*fn)(struct ref_transaction *, const char *, const char *); > + unsigned extra_lines; Define and explain the meaning of extra_lines in a comment. Without it, the most natural way to infer what the variable means by readers would be that "update" command sometimes receives up to two more lines but it also is perfectly normal if there is no extra line. But I am not sure if that is what is going on. "update" _always_ needs exactly two more input "lines" when the input is NUL delimited, perhaps, and it is an _error_ if there are not these two lines, no? The name of the field should make such details clear. > } commands[] = { > - { "update", parse_cmd_update }, > - { "create", parse_cmd_create }, > - { "delete", parse_cmd_delete }, > - { "verify", parse_cmd_verify }, > - { "option", parse_cmd_option }, > + { "update", parse_cmd_update, 2 }, > + { "create", parse_cmd_create, 1 }, > + { "delete", parse_cmd_delete, 1 }, > + { "verify", parse_cmd_verify, 1 }, > + { "option", parse_cmd_option, 0 }, > }; > + /* Read extra lines if NUL-terminated */ > + for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++) > + if (strbuf_appendwholeline(&input, stdin, line_termination)) > + break; And this code does not barf if you get a premature EOF and let the call to cmd->fn() go through, which sounds buggy. > + cmd->fn(transaction, input.buf + strlen(cmd->prefix), > + input.buf + input.len); > } > > if (ref_transaction_commit(transaction, &err)) Thanks.
Patrick Steinhardt <ps@pks.im> writes:
> The git-update-ref(1) command can only handle queueing transactions
> right now via its "--stdin" parameter, but there is no way for users to
> handle the transaction itself in a more explicit way. E.g. in a
> replicated scenario, one may imagine a coordinator that spawns
> git-update-ref(1) for multiple repositories and only if all agree that
> an update is possible will the coordinator send a commit. Such a
> transactional session could look like
>
> > start
> < start: ok
> > update refs/heads/master $OLD $NEW
> > prepare
> < prepare: ok
> # All nodes have returned "ok"
> > commit
> < commit: ok
>
> or
>
> > start
> < start: ok
> > create refs/heads/master $OLD $NEW
> > prepare
> < fatal: cannot lock ref 'refs/heads/master': reference already exists
> # On all other nodes:
> > abort
> < abort: ok
>
> In order to allow for such transactional sessions, this commit
> introduces four new commands for git-update-ref(1), which matches those
> we have internally already with the exception of "start":
>
> - start: start a new transaction
>
> - prepare: prepare the transaction, that is try to lock all
> references and verify their current value matches the
> expected one
>
> - commit: explicitly commit a session, that is update references to
> match their new expected state
>
> - abort: abort a session and roll back all changes
OK, this is a fun stuff, and explains why we wanted to do [7/9].
[-- Attachment #1: Type: text/plain, Size: 4976 bytes --] On Fri, Mar 27, 2020 at 02:25:34PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > +static const struct parse_cmd { > > + const char *prefix; > > + const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *); > > +} commands[] = { > > Do not call an array the represents a set of THINGs "type things[]"; > instead call it "type thing[]", so that the 0th thing can be > referred to as thing[0], not things[0]. > > One exception is when the set as a whole is referred to more often > than individual element of an array, in which case "things" (without > the [index]) becomes a sensible way to refer to the set. > > > + { "update", parse_cmd_update }, > > + { "create", parse_cmd_create }, > > + { "delete", parse_cmd_delete }, > > + { "verify", parse_cmd_verify }, > > + { "option", parse_cmd_option }, > > +}; > > + > > static void update_refs_stdin(struct ref_transaction *transaction) > > { > > struct strbuf input = STRBUF_INIT; > > const char *next; > > + int i; > > > > if (strbuf_read(&input, 0, 1000) < 0) > > die_errno("could not read from stdin"); > > next = input.buf; > > /* Read each line dispatch its command */ > > while (next < input.buf + input.len) { > > + const struct parse_cmd *cmd = NULL; > > + > > if (*next == line_termination) > > die("empty command in input"); > > else if (isspace(*next)) > > die("whitespace before command: %s", next); > > - else if (skip_prefix(next, "update ", &next)) > > - next = parse_cmd_update(transaction, &input, next); > > - else if (skip_prefix(next, "create ", &next)) > > - next = parse_cmd_create(transaction, &input, next); > > - else if (skip_prefix(next, "delete ", &next)) > > - next = parse_cmd_delete(transaction, &input, next); > > - else if (skip_prefix(next, "verify ", &next)) > > - next = parse_cmd_verify(transaction, &input, next); > > - else if (skip_prefix(next, "option ", &next)) > > - next = parse_cmd_option(&input, next); > > - else > > + > > + for (i = 0; i < ARRAY_SIZE(commands); i++) { > > + if (!skip_prefix(next, commands[i].prefix , &next)) > > + continue; > > + cmd = &commands[i]; > > + break; > > + } > > The only reason why you had to sprinkle > > if (!skip_prefix(next, " ", &next)) > die(_("%s: missing space after command"), cmd); > > all over the place is because the table lacks the trailing SP (which > makes sense---after all, you are making a table of commands). In > other words, it's not like some command called from this dispatcher > would require " " after the command name and some others would not. > So why not avoid touching the parse_cmd_<cmd>() at all (except for > the "option" thing that now needs to take the transaction object for > uniformity), and then verify the presence of " " here, perhaps like > this: > > for (i = 0; i < ARRAY_SIZE(command); i++) { > const char *eoc; > if (!skip_prefix(next, commands[i].prefix, &eoc) || > *eoc != ' ') > continue; > cmd = &command[i]; > next = eoc; > break; > } The reason why I moved those `skip_prefix` calls into each of the respective commands is that this patch series introduces calls that do not accept a trailing space at all. Thus we cannot handle the space generically here, as that would was soon as we introduce the set of new commands. Initially I just had a trailing " " in the `command` array as you hint at, but it felt a bit magical to me and might make readers wonder why some commands are spelt "update " and why some are spelt "commit", without a trailing space. > Note that you cannot reuse &next here to future-proof the code; > otherwise, you wouldn't be able to add a new command, e.g. "options", > that sits next to the existing command "option", in the future. > > > + if (!cmd) > > die("unknown command: %s", next); > > > > + if (input.buf[strlen(cmd->prefix)] != line_termination && > > + input.buf[strlen(cmd->prefix)] != '\0' && > > + input.buf[strlen(cmd->prefix)] != ' ') > > + die("%s: no separator after command", cmd->prefix); > > This part of your version does not make sense to me. If the input > line began with "update" with some separator that is not a SP, the > original would not have matched. But with this code, "update\0" > would pass this code and cause cmd->fn() to be called, only to get > the input rejected, because next is not pointing to SP. So why do > you even need this if statement? I hope this makes more sense with the above background of commands without trailing space. It would probably make sense to move this into the command-matching loop though to be able to discern "option" and a potentially new "options" command in the future. Instead of dying, we'd then have to `continue` the loop and check whether any of the remaining commands matches. Patrick [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 2235 bytes --] On Fri, Mar 27, 2020 at 02:58:38PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > static const struct parse_cmd { > > const char *prefix; > > - const char *(*fn)(struct ref_transaction *, const char *, const char *); > > + void (*fn)(struct ref_transaction *, const char *, const char *); > > + unsigned extra_lines; > > Define and explain the meaning of extra_lines in a comment. Without > it, the most natural way to infer what the variable means by readers > would be that "update" command sometimes receives up to two more > lines but it also is perfectly normal if there is no extra line. > > But I am not sure if that is what is going on. > > "update" _always_ needs exactly two more input "lines" when the > input is NUL delimited, perhaps, and it is an _error_ if there are > not these two lines, no? That's extactly right. I pondered on a good name, but I wasn't yet able to come up with any one that brings across its meaning. I originally had `extra_args` but changed it after some internal discussion with Chris. > The name of the field should make such details clear. > > > } commands[] = { > > - { "update", parse_cmd_update }, > > - { "create", parse_cmd_create }, > > - { "delete", parse_cmd_delete }, > > - { "verify", parse_cmd_verify }, > > - { "option", parse_cmd_option }, > > + { "update", parse_cmd_update, 2 }, > > + { "create", parse_cmd_create, 1 }, > > + { "delete", parse_cmd_delete, 1 }, > > + { "verify", parse_cmd_verify, 1 }, > > + { "option", parse_cmd_option, 0 }, > > }; > > + /* Read extra lines if NUL-terminated */ > > + for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++) > > + if (strbuf_appendwholeline(&input, stdin, line_termination)) > > + break; > > And this code does not barf if you get a premature EOF and let the > call to cmd->fn() go through, which sounds buggy. This is in fact by design, but I only change the comment in a later commit. The reasoning is that by passing even incomplete buffers to a command, the command is going to be able to print a much better error message than printing a generic one here. I'll make sure to move over the comment. Patrick [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1746 bytes --] On Fri, Mar 27, 2020 at 02:04:18PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > While the strbuf interface already provides functions to read a line > > into it that completely replaces its current contents, we do not have an > > interface that allows for appending lines without discarding current > > contents. > > > > Add a new function `strbuf_appendwholeline` that reads a line including > > its terminating character into a strbuf non-destructively. This is a > > preparatory step for git-update-ref(1) reading standard input line-wise > > instead of as a block. > > Hmph. If we were to gain more uses of this function over time, the > necessity for an extra strbuf_addbuf() becomes annoying. I wonder > if we should be doing this patch the other way around, i.e. move the > whole implementation, except for the early > > if (feof(fp)) > return EOF; > strbuf_reset(sb); > > part, and call it strbuf_addwholeline(), and strbuf_getwholeline() > becomes a thin wrapper around it that resets the incoming buffer > before calling strbuf_addwholeline(). The logic to determine EOF > would need to be updated if we go that route (i.e. instead of > checking sb->len is zero in the !HAVE_GETDELIM side of the > implementation, we need to see if the number is the same as before) > but it should be minor. Unfortunately it's not that easy for the HAVE_GETDELIM case, though, as we cannot call getdelim(3P) to append to an already populated buffer. So we'd have to call getdelim(3P) either on a NULL line and append manually or on an empty line in case the strbuf doesn't have any contents. While doable, I wanted to keep out this additional complexity for now. Patrick [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 8630 bytes --] Hi, this is the second version of this patch series to introduce transaction support in `git update-ref --stdin`. The goal is to make commands available to the user of that command so that he can explicitly control the given transaction by starting, preparing, and finally committing or aborting the transaction. The second version addresses the feedback of Junio: - Renamed the `commands` array to `command`. - Adds documentation to the `extra_lines` field to hopefully make its intent clearer. - Clarify why we ignore EOF when reading `extra_lines`. - When reading commands, the code now verifies that a line is not only a prefix to the command, but in fact the whole command. This allows implementation of multiple commands that have the same prefix, e.g. "option" and "options". Thanks Junio for your review! Patrick Patrick Steinhardt (9): refs: fix segfault when aborting empty transaction git-update-ref.txt: add missing word strbuf: provide function to append whole lines update-ref: organize commands in an array update-ref: drop unused argument for `parse_refname` update-ref: pass end pointer instead of strbuf update-ref: move transaction handling into `update_refs_stdin()` update-ref: read commands in a line-wise fashion update-ref: implement interactive transaction handling Documentation/git-update-ref.txt | 28 +++- builtin/update-ref.c | 274 +++++++++++++++++++++++-------- refs/files-backend.c | 20 ++- strbuf.c | 10 ++ strbuf.h | 6 + t/t1400-update-ref.sh | 131 +++++++++++++++ 6 files changed, 388 insertions(+), 81 deletions(-) Range-diff against v1: 1: 3c7f8c47f3 = 1: 7a297db4da refs: fix segfault when aborting empty transaction 2: dd7798abb7 = 2: 15857e1b8c git-update-ref.txt: add missing word 3: 4b0a963ea5 = 3: b6546ae44e strbuf: provide function to append whole lines 4: 50ffc26329 ! 4: bd8c059fbc update-ref: organize commands in an array @@ builtin/update-ref.c: static const char *parse_cmd_option(struct strbuf *input, +static const struct parse_cmd { + const char *prefix; + const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *); -+} commands[] = { ++} command[] = { + { "update", parse_cmd_update }, + { "create", parse_cmd_create }, + { "delete", parse_cmd_delete }, @@ builtin/update-ref.c: static const char *parse_cmd_option(struct strbuf *input, - next = parse_cmd_option(&input, next); - else + -+ for (i = 0; i < ARRAY_SIZE(commands); i++) { -+ if (!skip_prefix(next, commands[i].prefix , &next)) ++ for (i = 0; i < ARRAY_SIZE(command); i++) { ++ const char *prefix = command[i].prefix; ++ ++ if (!skip_prefix(next, prefix, &next)) + continue; -+ cmd = &commands[i]; ++ ++ /* ++ * Check that the command is terminated by an argument ++ * or line terminator and not only a matching prefix. ++ */ ++ if (input.buf[strlen(prefix)] != line_termination && ++ input.buf[strlen(prefix)] != '\0' && ++ input.buf[strlen(prefix)] != ' ') ++ continue; ++ ++ cmd = &command[i]; + break; + } + if (!cmd) die("unknown command: %s", next); -+ if (input.buf[strlen(cmd->prefix)] != line_termination && -+ input.buf[strlen(cmd->prefix)] != '\0' && -+ input.buf[strlen(cmd->prefix)] != ' ') -+ die("%s: no separator after command", cmd->prefix); -+ + next = cmd->fn(transaction, &input, next); next++; } 5: a78043b188 = 5: 49a14d2046 update-ref: drop unused argument for `parse_refname` 6: 51ebb2f849 ! 6: cbe430029d update-ref: pass end pointer instead of strbuf @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction const char *prefix; - const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *); + const char *(*fn)(struct ref_transaction *, const char *, const char *); - } commands[] = { + } command[] = { { "update", parse_cmd_update }, { "create", parse_cmd_create }, @@ builtin/update-ref.c: static void update_refs_stdin(struct ref_transaction *transaction) - input.buf[strlen(cmd->prefix)] != ' ') - die("%s: no separator after command", cmd->prefix); + if (!cmd) + die("unknown command: %s", next); - next = cmd->fn(transaction, &input, next); + next = cmd->fn(transaction, next, input.buf + input.len); 7: bd92a649d7 = 7: d2f68f59a7 update-ref: move transaction handling into `update_refs_stdin()` 8: 1db63f97ec ! 8: f8786fdeb3 update-ref: read commands in a line-wise fashion @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction const char *prefix; - const char *(*fn)(struct ref_transaction *, const char *, const char *); + void (*fn)(struct ref_transaction *, const char *, const char *); ++ /* ++ * If using NUL-terminated format, only the first argument will be ++ * available in the first line. In case a command expects more than one ++ * argument, we thus have to fetch an additional `extra_lines` number ++ * of lines. ++ */ + unsigned extra_lines; - } commands[] = { + } command[] = { - { "update", parse_cmd_update }, - { "create", parse_cmd_create }, - { "delete", parse_cmd_delete }, @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction + else if (isspace(*input.buf)) + die("whitespace before command: %s", input.buf); - for (i = 0; i < ARRAY_SIZE(commands); i++) { -- if (!skip_prefix(next, commands[i].prefix , &next)) -+ if (!starts_with(input.buf, commands[i].prefix)) + for (i = 0; i < ARRAY_SIZE(command); i++) { + const char *prefix = command[i].prefix; + +- if (!skip_prefix(next, prefix, &next)) ++ if (!starts_with(input.buf, prefix)) continue; - cmd = &commands[i]; + + /* +@@ builtin/update-ref.c: static void update_refs_stdin(void) break; } if (!cmd) - die("unknown command: %s", next); + die("unknown command: %s", input.buf); - if (input.buf[strlen(cmd->prefix)] != line_termination && - input.buf[strlen(cmd->prefix)] != '\0' && - input.buf[strlen(cmd->prefix)] != ' ') - die("%s: no separator after command", cmd->prefix); - - next = cmd->fn(transaction, next, input.buf + input.len); - next++; -+ /* Read extra lines if NUL-terminated */ ++ /* ++ * Read extra lines if NUL-terminated. Do not raise an error in ++ * case there is an early EOF to let the command handle missing ++ * arguments with a proper error message. ++ */ + for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++) + if (strbuf_appendwholeline(&input, stdin, line_termination)) + break; 9: 88c0089bb5 ! 9: c3fffdf9fa update-ref: implement interactive transaction handling @@ builtin/update-ref.c: static void parse_cmd_option(struct ref_transaction *trans static const struct parse_cmd { const char *prefix; void (*fn)(struct ref_transaction *, const char *, const char *); +@@ builtin/update-ref.c: static const struct parse_cmd { + * of lines. + */ unsigned extra_lines; + enum update_refs_state state; - } commands[] = { + } command[] = { - { "update", parse_cmd_update, 2 }, - { "create", parse_cmd_create, 1 }, - { "delete", parse_cmd_delete, 1 }, @@ builtin/update-ref.c: static void parse_cmd_option(struct ref_transaction *trans int i, j; @@ builtin/update-ref.c: static void update_refs_stdin(void) - input.buf[strlen(cmd->prefix)] != ' ') - die("%s: no separator after command", cmd->prefix); - -- /* Read extra lines if NUL-terminated */ -+ /* Read extra lines if NUL-terminated, but let commands handle missing ones. */ - for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++) if (strbuf_appendwholeline(&input, stdin, line_termination)) break; -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1707 bytes --] When cleaning up a transaction that has no updates queued, then the transaction's backend data will not have been allocated. We correctly handle this for the packed backend, where the cleanup function checks whether the backend data has been allocated at all -- if not, then there is nothing to clean up. For the files backend we do not check this and as a result will hit a segfault due to dereferencing a `NULL` pointer when cleaning up such a transaction. Fix the issue by checking whether `backend_data` is set in the files backend, too. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- refs/files-backend.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 561c33ac8a..6516c7bc8c 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2565,17 +2565,19 @@ static void files_transaction_cleanup(struct files_ref_store *refs, } } - if (backend_data->packed_transaction && - ref_transaction_abort(backend_data->packed_transaction, &err)) { - error("error aborting transaction: %s", err.buf); - strbuf_release(&err); + if (backend_data) { + if (backend_data->packed_transaction && + ref_transaction_abort(backend_data->packed_transaction, &err)) { + error("error aborting transaction: %s", err.buf); + strbuf_release(&err); + } + + if (backend_data->packed_refs_locked) + packed_refs_unlock(refs->packed_ref_store); + + free(backend_data); } - if (backend_data->packed_refs_locked) - packed_refs_unlock(refs->packed_ref_store); - - free(backend_data); - transaction->state = REF_TRANSACTION_CLOSED; } -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 749 bytes --] The description for the "verify" command is lacking a single word "is", which this commit corrects. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Documentation/git-update-ref.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt index 9671423117..9bd039ce08 100644 --- a/Documentation/git-update-ref.txt +++ b/Documentation/git-update-ref.txt @@ -107,7 +107,7 @@ delete:: verify:: Verify <ref> against <oldvalue> but do not change it. If - <oldvalue> zero or missing, the ref must not exist. + <oldvalue> is zero or missing, the ref must not exist. option:: Modify behavior of the next command naming a <ref>. -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1765 bytes --] While the strbuf interface already provides functions to read a line into it that completely replaces its current contents, we do not have an interface that allows for appending lines without discarding current contents. Add a new function `strbuf_appendwholeline` that reads a line including its terminating character into a strbuf non-destructively. This is a preparatory step for git-update-ref(1) reading standard input line-wise instead of as a block. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- strbuf.c | 10 ++++++++++ strbuf.h | 6 ++++++ 2 files changed, 16 insertions(+) diff --git a/strbuf.c b/strbuf.c index bb0065ccaf..6e74901bfa 100644 --- a/strbuf.c +++ b/strbuf.c @@ -690,6 +690,16 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term) } #endif +int strbuf_appendwholeline(struct strbuf *sb, FILE *fp, int term) +{ + struct strbuf line = STRBUF_INIT; + if (strbuf_getwholeline(&line, fp, term)) + return EOF; + strbuf_addbuf(sb, &line); + strbuf_release(&line); + return 0; +} + static int strbuf_getdelim(struct strbuf *sb, FILE *fp, int term) { if (strbuf_getwholeline(sb, fp, term)) diff --git a/strbuf.h b/strbuf.h index ce8e49c0b2..411063ca76 100644 --- a/strbuf.h +++ b/strbuf.h @@ -502,6 +502,12 @@ int strbuf_getline(struct strbuf *sb, FILE *file); */ int strbuf_getwholeline(struct strbuf *sb, FILE *file, int term); +/** + * Like `strbuf_getwholeline`, but appends the line instead of + * resetting the buffer first. + */ +int strbuf_appendwholeline(struct strbuf *sb, FILE *file, int term); + /** * Like `strbuf_getwholeline`, but operates on a file descriptor. * It reads one character at a time, so it is very slow. Do not -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 5805 bytes --] We currently manually wire up all commands known to `git-update-ref --stdin`, making it harder than necessary to preprocess arguments after the command is determined. To make this more extensible, let's refactor the code to use an array of known commands instead. While this doesn't add a lot of value now, it is a preparatory step to implement line-wise reading of commands. As we're going to introduce commands without trailing spaces, this commit also moves whitespace parsing into the respective commands. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/update-ref.c | 73 +++++++++++++++++++++++++++++++++----------- 1 file changed, 56 insertions(+), 17 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 2d8f7f0578..559475681e 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -171,11 +171,11 @@ static int parse_next_oid(struct strbuf *input, 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. + * command name. 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 ref_transaction *transaction, @@ -186,6 +186,9 @@ static const char *parse_cmd_update(struct ref_transaction *transaction, struct object_id new_oid, old_oid; int have_old; + if (!skip_prefix(next, " ", &next)) + die("update: missing space after command"); + refname = parse_refname(input, &next); if (!refname) die("update: missing <ref>"); @@ -220,6 +223,9 @@ static const char *parse_cmd_create(struct ref_transaction *transaction, char *refname; struct object_id new_oid; + if (!skip_prefix(next, " ", &next)) + die("create: missing space after command"); + refname = parse_refname(input, &next); if (!refname) die("create: missing <ref>"); @@ -253,6 +259,9 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction, struct object_id old_oid; int have_old; + if (!skip_prefix(next, " ", &next)) + die("delete: missing space after command"); + refname = parse_refname(input, &next); if (!refname) die("delete: missing <ref>"); @@ -288,6 +297,9 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, char *refname; struct object_id old_oid; + if (!skip_prefix(next, " ", &next)) + die("verify: missing space after command"); + refname = parse_refname(input, &next); if (!refname) die("verify: missing <ref>"); @@ -310,9 +322,12 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, return next; } -static const char *parse_cmd_option(struct strbuf *input, const char *next) +static const char *parse_cmd_option(struct ref_transaction *transaction, + struct strbuf *input, const char *next) { const char *rest; + if (!skip_prefix(next, " ", &next)) + die("option: missing space after command"); if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination) update_flags |= REF_NO_DEREF; else @@ -320,33 +335,57 @@ static const char *parse_cmd_option(struct strbuf *input, const char *next) return rest; } +static const struct parse_cmd { + const char *prefix; + const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *); +} command[] = { + { "update", parse_cmd_update }, + { "create", parse_cmd_create }, + { "delete", parse_cmd_delete }, + { "verify", parse_cmd_verify }, + { "option", parse_cmd_option }, +}; + static void update_refs_stdin(struct ref_transaction *transaction) { struct strbuf input = STRBUF_INIT; const char *next; + int i; if (strbuf_read(&input, 0, 1000) < 0) die_errno("could not read from stdin"); next = input.buf; /* Read each line dispatch its command */ while (next < input.buf + input.len) { + const struct parse_cmd *cmd = NULL; + if (*next == line_termination) die("empty command in input"); else if (isspace(*next)) die("whitespace before command: %s", next); - else if (skip_prefix(next, "update ", &next)) - next = parse_cmd_update(transaction, &input, next); - else if (skip_prefix(next, "create ", &next)) - next = parse_cmd_create(transaction, &input, next); - else if (skip_prefix(next, "delete ", &next)) - next = parse_cmd_delete(transaction, &input, next); - else if (skip_prefix(next, "verify ", &next)) - next = parse_cmd_verify(transaction, &input, next); - else if (skip_prefix(next, "option ", &next)) - next = parse_cmd_option(&input, next); - else + + for (i = 0; i < ARRAY_SIZE(command); i++) { + const char *prefix = command[i].prefix; + + if (!skip_prefix(next, prefix, &next)) + continue; + + /* + * Check that the command is terminated by an argument + * or line terminator and not only a matching prefix. + */ + if (input.buf[strlen(prefix)] != line_termination && + input.buf[strlen(prefix)] != '\0' && + input.buf[strlen(prefix)] != ' ') + continue; + + cmd = &command[i]; + break; + } + if (!cmd) die("unknown command: %s", next); + next = cmd->fn(transaction, &input, next); next++; } -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 2180 bytes --] The `parse_refname` function accepts a `struct strbuf *input` argument that isn't used at all. As we're about to convert commands to not use a strbuf anymore but instead an end pointer, let's drop this argument now to make the converting commit easier to review. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/update-ref.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 559475681e..6f2aba916b 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -50,7 +50,7 @@ static const char *parse_arg(const char *next, struct strbuf *arg) * the argument. Die if C-quoting is malformed or the reference name * is invalid. */ -static char *parse_refname(struct strbuf *input, const char **next) +static char *parse_refname(const char **next) { struct strbuf ref = STRBUF_INIT; @@ -189,7 +189,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction, if (!skip_prefix(next, " ", &next)) die("update: missing space after command"); - refname = parse_refname(input, &next); + refname = parse_refname(&next); if (!refname) die("update: missing <ref>"); @@ -226,7 +226,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction, if (!skip_prefix(next, " ", &next)) die("create: missing space after command"); - refname = parse_refname(input, &next); + refname = parse_refname(&next); if (!refname) die("create: missing <ref>"); @@ -262,7 +262,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction, if (!skip_prefix(next, " ", &next)) die("delete: missing space after command"); - refname = parse_refname(input, &next); + refname = parse_refname(&next); if (!refname) die("delete: missing <ref>"); @@ -300,7 +300,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, if (!skip_prefix(next, " ", &next)) die("verify: missing space after command"); - refname = parse_refname(input, &next); + refname = parse_refname(&next); if (!refname) die("verify: missing <ref>"); -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 5590 bytes --] We currently pass both an `strbuf` containing the current command line as well as the `next` pointer pointing to the first argument to commands. This is both confusing and makes code more intertwined. Convert this to use a simple pointer as well as a pointer pointing to the end of the input as a preparatory step to line-wise reading of stdin. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/update-ref.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 6f2aba916b..139c3b11fa 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -95,7 +95,7 @@ static char *parse_refname(const char **next) * 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_oid(struct strbuf *input, const char **next, +static int parse_next_oid(const char **next, const char *end, struct object_id *oid, const char *command, const char *refname, int flags) @@ -103,7 +103,7 @@ static int parse_next_oid(struct strbuf *input, const char **next, struct strbuf arg = STRBUF_INIT; int ret = 0; - if (*next == input->buf + input->len) + if (*next == end) goto eof; if (line_termination) { @@ -128,7 +128,7 @@ static int parse_next_oid(struct strbuf *input, const char **next, die("%s %s: expected NUL but got: %s", command, refname, *next); (*next)++; - if (*next == input->buf + input->len) + if (*next == end) goto eof; strbuf_addstr(&arg, *next); *next += arg.len; @@ -179,7 +179,7 @@ static int parse_next_oid(struct strbuf *input, const char **next, */ static const char *parse_cmd_update(struct ref_transaction *transaction, - struct strbuf *input, const char *next) + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -193,11 +193,11 @@ static const char *parse_cmd_update(struct ref_transaction *transaction, if (!refname) die("update: missing <ref>"); - if (parse_next_oid(input, &next, &new_oid, "update", refname, + if (parse_next_oid(&next, end, &new_oid, "update", refname, PARSE_SHA1_ALLOW_EMPTY)) die("update %s: missing <newvalue>", refname); - have_old = !parse_next_oid(input, &next, &old_oid, "update", refname, + have_old = !parse_next_oid(&next, end, &old_oid, "update", refname, PARSE_SHA1_OLD); if (*next != line_termination) @@ -217,7 +217,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction, } static const char *parse_cmd_create(struct ref_transaction *transaction, - struct strbuf *input, const char *next) + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -230,7 +230,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction, if (!refname) die("create: missing <ref>"); - if (parse_next_oid(input, &next, &new_oid, "create", refname, 0)) + if (parse_next_oid(&next, end, &new_oid, "create", refname, 0)) die("create %s: missing <newvalue>", refname); if (is_null_oid(&new_oid)) @@ -252,7 +252,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction, } static const char *parse_cmd_delete(struct ref_transaction *transaction, - struct strbuf *input, const char *next) + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -266,7 +266,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction, if (!refname) die("delete: missing <ref>"); - if (parse_next_oid(input, &next, &old_oid, "delete", refname, + if (parse_next_oid(&next, end, &old_oid, "delete", refname, PARSE_SHA1_OLD)) { have_old = 0; } else { @@ -291,7 +291,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction, } static const char *parse_cmd_verify(struct ref_transaction *transaction, - struct strbuf *input, const char *next) + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -304,7 +304,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, if (!refname) die("verify: missing <ref>"); - if (parse_next_oid(input, &next, &old_oid, "verify", refname, + if (parse_next_oid(&next, end, &old_oid, "verify", refname, PARSE_SHA1_OLD)) oidclr(&old_oid); @@ -323,7 +323,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, } static const char *parse_cmd_option(struct ref_transaction *transaction, - struct strbuf *input, const char *next) + const char *next, const char *end) { const char *rest; if (!skip_prefix(next, " ", &next)) @@ -337,7 +337,7 @@ static const char *parse_cmd_option(struct ref_transaction *transaction, static const struct parse_cmd { const char *prefix; - const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *); + const char *(*fn)(struct ref_transaction *, const char *, const char *); } command[] = { { "update", parse_cmd_update }, { "create", parse_cmd_create }, @@ -385,7 +385,7 @@ static void update_refs_stdin(struct ref_transaction *transaction) if (!cmd) die("unknown command: %s", next); - next = cmd->fn(transaction, &input, next); + next = cmd->fn(transaction, next, input.buf + input.len); next++; } -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 2473 bytes --] While the actual logic to update the transaction is handled in `update_refs_stdin()`, the transaction itself is started and committed in `cmd_update_ref()` itself. This makes it hard to handle transaction abortion and commits as part of `update_refs_stdin()` itself, which is required in order to introduce transaction handling features to `git update-refs --stdin`. Refactor the code to move all transaction handling into `update_refs_stdin()` to prepare for transaction handling features. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/update-ref.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 139c3b11fa..1a7906545d 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -346,15 +346,21 @@ static const struct parse_cmd { { "option", parse_cmd_option }, }; -static void update_refs_stdin(struct ref_transaction *transaction) +static void update_refs_stdin(void) { - struct strbuf input = STRBUF_INIT; + struct strbuf input = STRBUF_INIT, err = STRBUF_INIT; + struct ref_transaction *transaction; const char *next; int i; if (strbuf_read(&input, 0, 1000) < 0) die_errno("could not read from stdin"); next = input.buf; + + transaction = ref_transaction_begin(&err); + if (!transaction) + die("%s", err.buf); + /* Read each line dispatch its command */ while (next < input.buf + input.len) { const struct parse_cmd *cmd = NULL; @@ -389,6 +395,11 @@ static void update_refs_stdin(struct ref_transaction *transaction) next++; } + if (ref_transaction_commit(transaction, &err)) + die("%s", err.buf); + + ref_transaction_free(transaction); + strbuf_release(&err); strbuf_release(&input); } @@ -423,21 +434,11 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) } if (read_stdin) { - struct strbuf err = STRBUF_INIT; - struct ref_transaction *transaction; - - transaction = ref_transaction_begin(&err); - if (!transaction) - die("%s", err.buf); if (delete || argc > 0) usage_with_options(git_update_ref_usage, options); if (end_null) line_termination = '\0'; - update_refs_stdin(transaction); - if (ref_transaction_commit(transaction, &err)) - die("%s", err.buf); - ref_transaction_free(transaction); - strbuf_release(&err); + update_refs_stdin(); return 0; } -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 6872 bytes --] The git-update-ref(1) supports a `--stdin` mode that allows it to read all reference updates from standard input. This is mainly used to allow for atomic reference updates that are all or nothing, so that either all references will get updated or none. Currently, git-update-ref(1) reads all commands as a single block of up to 1000 characters and only starts processing after stdin gets closed. This is less flexible than one might wish for, as it doesn't really allow for longer-lived transactions and doesn't allow any verification without committing everything. E.g. one may imagine the following exchange: > start < start: ok > update refs/heads/master $NEWOID1 $OLDOID1 > update refs/heads/branch $NEWOID2 $OLDOID2 > prepare < prepare: ok > commit < commit: ok When reading all input as a whole block, the above interactive protocol is obviously impossible to achieve. But by converting the command to read commands linewise, we can make it more interactive than before. Obviously, the linewise interface is only a first step in making git-update-ref(1) work in a more transaction-oriented way. Missing is most importantly support for transactional commands that manage the current transaction. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/update-ref.c | 80 +++++++++++++++++++++++--------------------- 1 file changed, 41 insertions(+), 39 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 1a7906545d..77cd235dfc 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -178,8 +178,8 @@ static int parse_next_oid(const char **next, const char *end, * line_termination is set. */ -static const char *parse_cmd_update(struct ref_transaction *transaction, - const char *next, const char *end) +static void parse_cmd_update(struct ref_transaction *transaction, + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -212,12 +212,10 @@ static const char *parse_cmd_update(struct ref_transaction *transaction, update_flags = default_flags; free(refname); strbuf_release(&err); - - return next; } -static const char *parse_cmd_create(struct ref_transaction *transaction, - const char *next, const char *end) +static void parse_cmd_create(struct ref_transaction *transaction, + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -247,12 +245,10 @@ static const char *parse_cmd_create(struct ref_transaction *transaction, update_flags = default_flags; free(refname); strbuf_release(&err); - - return next; } -static const char *parse_cmd_delete(struct ref_transaction *transaction, - const char *next, const char *end) +static void parse_cmd_delete(struct ref_transaction *transaction, + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -286,12 +282,10 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction, update_flags = default_flags; free(refname); strbuf_release(&err); - - return next; } -static const char *parse_cmd_verify(struct ref_transaction *transaction, - const char *next, const char *end) +static void parse_cmd_verify(struct ref_transaction *transaction, + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -318,12 +312,10 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, update_flags = default_flags; free(refname); strbuf_release(&err); - - return next; } -static const char *parse_cmd_option(struct ref_transaction *transaction, - const char *next, const char *end) +static void parse_cmd_option(struct ref_transaction *transaction, + const char *next, const char *end) { const char *rest; if (!skip_prefix(next, " ", &next)) @@ -332,48 +324,49 @@ static const char *parse_cmd_option(struct ref_transaction *transaction, update_flags |= REF_NO_DEREF; else die("option unknown: %s", next); - return rest; } static const struct parse_cmd { const char *prefix; - const char *(*fn)(struct ref_transaction *, const char *, const char *); + void (*fn)(struct ref_transaction *, const char *, const char *); + /* + * If using NUL-terminated format, only the first argument will be + * available in the first line. In case a command expects more than one + * argument, we thus have to fetch an additional `extra_lines` number + * of lines. + */ + unsigned extra_lines; } command[] = { - { "update", parse_cmd_update }, - { "create", parse_cmd_create }, - { "delete", parse_cmd_delete }, - { "verify", parse_cmd_verify }, - { "option", parse_cmd_option }, + { "update", parse_cmd_update, 2 }, + { "create", parse_cmd_create, 1 }, + { "delete", parse_cmd_delete, 1 }, + { "verify", parse_cmd_verify, 1 }, + { "option", parse_cmd_option, 0 }, }; static void update_refs_stdin(void) { struct strbuf input = STRBUF_INIT, err = STRBUF_INIT; struct ref_transaction *transaction; - const char *next; - int i; - - if (strbuf_read(&input, 0, 1000) < 0) - die_errno("could not read from stdin"); - next = input.buf; + int i, j; transaction = ref_transaction_begin(&err); if (!transaction) die("%s", err.buf); /* Read each line dispatch its command */ - while (next < input.buf + input.len) { + while (!strbuf_getwholeline(&input, stdin, line_termination)) { const struct parse_cmd *cmd = NULL; - if (*next == line_termination) + if (*input.buf == line_termination) die("empty command in input"); - else if (isspace(*next)) - die("whitespace before command: %s", next); + else if (isspace(*input.buf)) + die("whitespace before command: %s", input.buf); for (i = 0; i < ARRAY_SIZE(command); i++) { const char *prefix = command[i].prefix; - if (!skip_prefix(next, prefix, &next)) + if (!starts_with(input.buf, prefix)) continue; /* @@ -389,10 +382,19 @@ static void update_refs_stdin(void) break; } if (!cmd) - die("unknown command: %s", next); + die("unknown command: %s", input.buf); - next = cmd->fn(transaction, next, input.buf + input.len); - next++; + /* + * Read extra lines if NUL-terminated. Do not raise an error in + * case there is an early EOF to let the command handle missing + * arguments with a proper error message. + */ + for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++) + if (strbuf_appendwholeline(&input, stdin, line_termination)) + break; + + cmd->fn(transaction, input.buf + strlen(cmd->prefix), + input.buf + input.len); } if (ref_transaction_commit(transaction, &err)) -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 12083 bytes --] The git-update-ref(1) command can only handle queueing transactions right now via its "--stdin" parameter, but there is no way for users to handle the transaction itself in a more explicit way. E.g. in a replicated scenario, one may imagine a coordinator that spawns git-update-ref(1) for multiple repositories and only if all agree that an update is possible will the coordinator send a commit. Such a transactional session could look like > start < start: ok > update refs/heads/master $OLD $NEW > prepare < prepare: ok # All nodes have returned "ok" > commit < commit: ok or > start < start: ok > create refs/heads/master $OLD $NEW > prepare < fatal: cannot lock ref 'refs/heads/master': reference already exists # On all other nodes: > abort < abort: ok In order to allow for such transactional sessions, this commit introduces four new commands for git-update-ref(1), which matches those we have internally already with the exception of "start": - start: start a new transaction - prepare: prepare the transaction, that is try to lock all references and verify their current value matches the expected one - commit: explicitly commit a session, that is update references to match their new expected state - abort: abort a session and roll back all changes By design, git-update-ref(1) will commit as soon as standard input is being closed. While fine in a non-transactional world, it is definitely unexpected in a transactional world. Because of this, as soon as any of the new transactional commands is used, the default will change to aborting without an explicit "commit". To avoid a race between queueing updates and the first "prepare" that starts a transaction, the "start" command has been added to start an explicit transaction. Add some tests to exercise this new functionality. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Documentation/git-update-ref.txt | 26 ++++++ builtin/update-ref.c | 106 +++++++++++++++++++++++-- t/t1400-update-ref.sh | 131 +++++++++++++++++++++++++++++++ 3 files changed, 255 insertions(+), 8 deletions(-) diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt index 9bd039ce08..3e737c2360 100644 --- a/Documentation/git-update-ref.txt +++ b/Documentation/git-update-ref.txt @@ -66,6 +66,10 @@ performs all modifications together. Specify commands of the form: delete SP <ref> [SP <oldvalue>] LF verify SP <ref> [SP <oldvalue>] LF option SP <opt> LF + start LF + prepare LF + commit LF + abort LF With `--create-reflog`, update-ref will create a reflog for each ref even if one would not ordinarily be created. @@ -83,6 +87,10 @@ quoting: delete SP <ref> NUL [<oldvalue>] NUL verify SP <ref> NUL [<oldvalue>] NUL option SP <opt> NUL + start NUL + prepare NUL + commit NUL + abort NUL In this format, use 40 "0" to specify a zero value, and use the empty string to specify a missing value. @@ -114,6 +122,24 @@ option:: The only valid option is `no-deref` to avoid dereferencing a symbolic ref. +start:: + Start a transaction. In contrast to a non-transactional session, a + transaction will automatically abort if the session ends without an + explicit commit. + +prepare:: + Prepare to commit the transaction. This will create lock files for all + queued reference updates. If one reference could not be locked, the + transaction will be aborted. + +commit:: + Commit all reference updates queued for the transaction, ending the + transaction. + +abort:: + Abort the transaction, releasing all locks if the transaction is in + prepared state. + If all <ref>s can be locked with matching <oldvalue>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 77cd235dfc..120c0a5fc0 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -326,6 +326,59 @@ static void parse_cmd_option(struct ref_transaction *transaction, die("option unknown: %s", next); } +static void parse_cmd_start(struct ref_transaction *transaction, + const char *next, const char *end) +{ + if (*next != line_termination) + die("start: extra input: %s", next); + puts("start: ok"); +} + +static void parse_cmd_prepare(struct ref_transaction *transaction, + const char *next, const char *end) +{ + struct strbuf error = STRBUF_INIT; + if (*next != line_termination) + die("prepare: extra input: %s", next); + if (ref_transaction_prepare(transaction, &error)) + die("prepare: %s", error.buf); + puts("prepare: ok"); +} + +static void parse_cmd_abort(struct ref_transaction *transaction, + const char *next, const char *end) +{ + struct strbuf error = STRBUF_INIT; + if (*next != line_termination) + die("abort: extra input: %s", next); + if (ref_transaction_abort(transaction, &error)) + die("abort: %s", error.buf); + puts("abort: ok"); +} + +static void parse_cmd_commit(struct ref_transaction *transaction, + const char *next, const char *end) +{ + struct strbuf error = STRBUF_INIT; + if (*next != line_termination) + die("commit: extra input: %s", next); + if (ref_transaction_commit(transaction, &error)) + die("commit: %s", error.buf); + puts("commit: ok"); + ref_transaction_free(transaction); +} + +enum update_refs_state { + /* Non-transactional state open for updates. */ + UPDATE_REFS_OPEN, + /* A transaction has been started. */ + UPDATE_REFS_STARTED, + /* References are locked and ready for commit */ + UPDATE_REFS_PREPARED, + /* Transaction has been committed or closed. */ + UPDATE_REFS_CLOSED, +}; + static const struct parse_cmd { const char *prefix; void (*fn)(struct ref_transaction *, const char *, const char *); @@ -336,17 +389,23 @@ static const struct parse_cmd { * of lines. */ unsigned extra_lines; + enum update_refs_state state; } command[] = { - { "update", parse_cmd_update, 2 }, - { "create", parse_cmd_create, 1 }, - { "delete", parse_cmd_delete, 1 }, - { "verify", parse_cmd_verify, 1 }, - { "option", parse_cmd_option, 0 }, + { "update", parse_cmd_update, 2, UPDATE_REFS_OPEN }, + { "create", parse_cmd_create, 1, UPDATE_REFS_OPEN }, + { "delete", parse_cmd_delete, 1, UPDATE_REFS_OPEN }, + { "verify", parse_cmd_verify, 1, UPDATE_REFS_OPEN }, + { "option", parse_cmd_option, 0, UPDATE_REFS_OPEN }, + { "start", parse_cmd_start, 0, UPDATE_REFS_STARTED }, + { "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED }, + { "abort", parse_cmd_abort, 0, UPDATE_REFS_CLOSED }, + { "commit", parse_cmd_commit, 0, UPDATE_REFS_CLOSED }, }; static void update_refs_stdin(void) { struct strbuf input = STRBUF_INIT, err = STRBUF_INIT; + enum update_refs_state state = UPDATE_REFS_OPEN; struct ref_transaction *transaction; int i, j; @@ -393,14 +452,45 @@ static void update_refs_stdin(void) if (strbuf_appendwholeline(&input, stdin, line_termination)) break; + switch (state) { + case UPDATE_REFS_OPEN: + case UPDATE_REFS_STARTED: + /* Do not downgrade a transaction to a non-transaction. */ + if (cmd->state >= state) + state = cmd->state; + break; + case UPDATE_REFS_PREPARED: + if (cmd->state != UPDATE_REFS_CLOSED) + die("prepared transactions can only be closed"); + state = cmd->state; + break; + case UPDATE_REFS_CLOSED: + die("transaction is closed"); + break; + } + cmd->fn(transaction, input.buf + strlen(cmd->prefix), input.buf + input.len); } - if (ref_transaction_commit(transaction, &err)) - die("%s", err.buf); + switch (state) { + case UPDATE_REFS_OPEN: + /* Commit by default if no transaction was requested. */ + if (ref_transaction_commit(transaction, &err)) + die("%s", err.buf); + ref_transaction_free(transaction); + break; + case UPDATE_REFS_STARTED: + case UPDATE_REFS_PREPARED: + /* If using a transaction, we want to abort it. */ + if (ref_transaction_abort(transaction, &err)) + die("%s", err.buf); + break; + case UPDATE_REFS_CLOSED: + /* Otherwise no need to do anything, the transaction was closed already. */ + break; + } - ref_transaction_free(transaction); strbuf_release(&err); strbuf_release(&input); } diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index a6224ef65f..48d0d42afd 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -1404,4 +1404,135 @@ test_expect_success 'handle per-worktree refs in refs/bisect' ' ! test_cmp main-head worktree-head ' +test_expect_success 'transaction handles empty commit' ' + cat >stdin <<-EOF && + start + prepare + commit + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start prepare commit >expect && + test_cmp expect actual +' + +test_expect_success 'transaction handles empty commit with missing prepare' ' + cat >stdin <<-EOF && + start + commit + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start commit >expect && + test_cmp expect actual +' + +test_expect_success 'transaction handles sole commit' ' + cat >stdin <<-EOF && + commit + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" commit >expect && + test_cmp expect actual +' + +test_expect_success 'transaction handles empty abort' ' + cat >stdin <<-EOF && + start + prepare + abort + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start prepare abort >expect && + test_cmp expect actual +' + +test_expect_success 'transaction exits on multiple aborts' ' + cat >stdin <<-EOF && + abort + abort + EOF + test_must_fail git update-ref --stdin <stdin >actual 2>err && + printf "%s: ok\n" abort >expect && + test_cmp expect actual && + grep "fatal: transaction is closed" err +' + +test_expect_success 'transaction exits on start after prepare' ' + cat >stdin <<-EOF && + prepare + start + EOF + test_must_fail git update-ref --stdin <stdin 2>err >actual && + printf "%s: ok\n" prepare >expect && + test_cmp expect actual && + grep "fatal: prepared transactions can only be closed" err +' + +test_expect_success 'transaction handles empty abort with missing prepare' ' + cat >stdin <<-EOF && + start + abort + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start abort >expect && + test_cmp expect actual +' + +test_expect_success 'transaction handles sole abort' ' + cat >stdin <<-EOF && + abort + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" abort >expect && + test_cmp expect actual +' + +test_expect_success 'transaction can handle commit' ' + cat >stdin <<-EOF && + start + create $a HEAD + commit + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start commit >expect && + test_cmp expect actual && + git rev-parse HEAD >expect && + git rev-parse $a >actual && + test_cmp expect actual +' + +test_expect_success 'transaction can handle abort' ' + cat >stdin <<-EOF && + start + create $b HEAD + abort + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start abort >expect && + test_cmp expect actual && + test_path_is_missing .git/$b +' + +test_expect_success 'transaction aborts by default' ' + cat >stdin <<-EOF && + start + create $b HEAD + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start >expect && + test_cmp expect actual && + test_path_is_missing .git/$b +' + +test_expect_success 'transaction with prepare aborts by default' ' + cat >stdin <<-EOF && + start + create $b HEAD + prepare + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start prepare >expect && + test_cmp expect actual && + test_path_is_missing .git/$b +' + test_done -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
>> for (i = 0; i < ARRAY_SIZE(command); i++) {
>> const char *eoc;
>> if (!skip_prefix(next, commands[i].prefix, &eoc) ||
>> *eoc != ' ')
>> continue;
>> cmd = &command[i];
>> next = eoc;
>> break;
>> }
>
> The reason why I moved those `skip_prefix` calls into each of the
> respective commands is that this patch series introduces calls that do
> not accept a trailing space at all. Thus we cannot handle the space
> generically here, as that would was soon as we introduce the set of new
> commands.
That's not a good excuse, though, is it? The command[] structure
can say "this takes parameters" or even "this takes N parameters",
and the field being zero (i.e. "does not take parameters" or "takes
zero parameters") would mean you do not want a trailing SP, no?
I also suspect that the "extra lines" thing we'd see in a later step
is correlated with this, but we'll see.
Thanks.
Patrick Steinhardt <ps@pks.im> writes:
> Unfortunately it's not that easy for the HAVE_GETDELIM case, though, as
> we cannot call getdelim(3P) to append to an already populated buffer. So
> we'd have to call getdelim(3P) either on a NULL line and append manually
> or on an empty line in case the strbuf doesn't have any contents. While
> doable, I wanted to keep out this additional complexity for now.
Ahh, I see. Thanks for a clarification.
[-- Attachment #1: Type: text/plain, Size: 1197 bytes --] On Mon, Mar 30, 2020 at 09:55:44AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > >> for (i = 0; i < ARRAY_SIZE(command); i++) { > >> const char *eoc; > >> if (!skip_prefix(next, commands[i].prefix, &eoc) || > >> *eoc != ' ') > >> continue; > >> cmd = &command[i]; > >> next = eoc; > >> break; > >> } > > > > The reason why I moved those `skip_prefix` calls into each of the > > respective commands is that this patch series introduces calls that do > > not accept a trailing space at all. Thus we cannot handle the space > > generically here, as that would was soon as we introduce the set of new > > commands. > > That's not a good excuse, though, is it? The command[] structure > can say "this takes parameters" or even "this takes N parameters", > and the field being zero (i.e. "does not take parameters" or "takes > zero parameters") would mean you do not want a trailing SP, no? > > I also suspect that the "extra lines" thing we'd see in a later step > is correlated with this, but we'll see. > > Thanks. You've got a point there. I'll convert this for the next version, thanks! Patrick [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Fri, Mar 27, 2020 at 02:58:38PM -0700, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>> > static const struct parse_cmd {
>> > const char *prefix;
>> > - const char *(*fn)(struct ref_transaction *, const char *, const char *);
>> > + void (*fn)(struct ref_transaction *, const char *, const char *);
>> > + unsigned extra_lines;
>>
>> Define and explain the meaning of extra_lines in a comment. Without
>> it, the most natural way to infer what the variable means by readers
>> would be that "update" command sometimes receives up to two more
>> lines but it also is perfectly normal if there is no extra line.
>>
>> But I am not sure if that is what is going on.
>>
>> "update" _always_ needs exactly two more input "lines" when the
>> input is NUL delimited, perhaps, and it is an _error_ if there are
>> not these two lines, no?
>
> That's extactly right. I pondered on a good name, but I wasn't yet able
> to come up with any one that brings across its meaning. I originally had
> `extra_args` but changed it after some internal discussion with Chris.
Wouldn't it make more sense to store the number of args expected
here, not "extra"? The one that does not take any then can say "I
am subcommand X and I do not want a SP after my name".
The "sometimes after the command there is line.termination, and
sometimes there is NUL, and yet some other times there is SP", which
is a sloppy way to check malformed input (i.e. you do not want SP
for a command that does not take parameters), that you have after
the loop can then go, if you did the parsing that way.
[-- Attachment #1: Type: text/plain, Size: 15162 bytes --] Hi, this is the second version of this patch series to introduce transaction support in `git update-ref --stdin`. The goal is to make commands available to the user of that command so that he can explicitly control the given transaction by starting, preparing, and finally committing or aborting the transaction. This version improves the argument handling for commands as proposed by Junio. Instead of the hard-to-understand `extra_lines` field, commands now have a `args` field that specifies how many arguments a given command has, including the first one separated by a space. As a result, the initial space for the first argument is now handled in the generic part and not moved into each of the command functions anymore. I've also added a comment that clarifies why we ignore EOF when fetching additional command lines. The range-diff against v2 is attached below. Patrick Patrick Steinhardt (9): refs: fix segfault when aborting empty transaction git-update-ref.txt: add missing word strbuf: provide function to append whole lines update-ref: organize commands in an array update-ref: drop unused argument for `parse_refname` update-ref: pass end pointer instead of strbuf update-ref: move transaction handling into `update_refs_stdin()` update-ref: read commands in a line-wise fashion update-ref: implement interactive transaction handling Documentation/git-update-ref.txt | 28 +++- builtin/update-ref.c | 245 ++++++++++++++++++++++--------- refs/files-backend.c | 20 +-- strbuf.c | 10 ++ strbuf.h | 6 + t/t1400-update-ref.sh | 131 +++++++++++++++++ 6 files changed, 364 insertions(+), 76 deletions(-) Range-diff against v2: 1: 7a297db4da = 1: 7a297db4da refs: fix segfault when aborting empty transaction 2: 15857e1b8c = 2: 15857e1b8c git-update-ref.txt: add missing word 3: b6546ae44e = 3: b6546ae44e strbuf: provide function to append whole lines 4: bd8c059fbc ! 4: 0f881d4936 update-ref: organize commands in an array @@ Commit message Signed-off-by: Patrick Steinhardt <ps@pks.im> ## builtin/update-ref.c ## -@@ builtin/update-ref.c: static int parse_next_oid(struct strbuf *input, 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. -+ * command name. 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 ref_transaction *transaction, -@@ builtin/update-ref.c: static const char *parse_cmd_update(struct ref_transaction *transaction, - struct object_id new_oid, old_oid; - int have_old; - -+ if (!skip_prefix(next, " ", &next)) -+ die("update: missing space after command"); -+ - refname = parse_refname(input, &next); - if (!refname) - die("update: missing <ref>"); -@@ builtin/update-ref.c: static const char *parse_cmd_create(struct ref_transaction *transaction, - char *refname; - struct object_id new_oid; - -+ if (!skip_prefix(next, " ", &next)) -+ die("create: missing space after command"); -+ - refname = parse_refname(input, &next); - if (!refname) - die("create: missing <ref>"); -@@ builtin/update-ref.c: static const char *parse_cmd_delete(struct ref_transaction *transaction, - struct object_id old_oid; - int have_old; - -+ if (!skip_prefix(next, " ", &next)) -+ die("delete: missing space after command"); -+ - refname = parse_refname(input, &next); - if (!refname) - die("delete: missing <ref>"); -@@ builtin/update-ref.c: static const char *parse_cmd_verify(struct ref_transaction *transaction, - char *refname; - struct object_id old_oid; - -+ if (!skip_prefix(next, " ", &next)) -+ die("verify: missing space after command"); -+ - refname = parse_refname(input, &next); - if (!refname) - die("verify: missing <ref>"); @@ builtin/update-ref.c: static const char *parse_cmd_verify(struct ref_transaction *transaction, return next; } @@ builtin/update-ref.c: static const char *parse_cmd_verify(struct ref_transaction + struct strbuf *input, const char *next) { const char *rest; -+ if (!skip_prefix(next, " ", &next)) -+ die("option: missing space after command"); if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination) - update_flags |= REF_NO_DEREF; - else @@ builtin/update-ref.c: static const char *parse_cmd_option(struct strbuf *input, const char *next) return rest; } @@ builtin/update-ref.c: static const char *parse_cmd_option(struct strbuf *input, + for (i = 0; i < ARRAY_SIZE(command); i++) { + const char *prefix = command[i].prefix; + -+ if (!skip_prefix(next, prefix, &next)) -+ continue; -+ -+ /* -+ * Check that the command is terminated by an argument -+ * or line terminator and not only a matching prefix. -+ */ -+ if (input.buf[strlen(prefix)] != line_termination && -+ input.buf[strlen(prefix)] != '\0' && -+ input.buf[strlen(prefix)] != ' ') ++ if (!skip_prefix(next, prefix, &next) || ++ !skip_prefix(next, " ", &next)) + continue; + + cmd = &command[i]; 5: 49a14d2046 ! 5: c96395a14d update-ref: drop unused argument for `parse_refname` @@ builtin/update-ref.c: static const char *parse_arg(const char *next, struct strb struct strbuf ref = STRBUF_INIT; @@ builtin/update-ref.c: static const char *parse_cmd_update(struct ref_transaction *transaction, - if (!skip_prefix(next, " ", &next)) - die("update: missing space after command"); + struct object_id new_oid, old_oid; + int have_old; - refname = parse_refname(input, &next); + refname = parse_refname(&next); @@ builtin/update-ref.c: static const char *parse_cmd_update(struct ref_transaction die("update: missing <ref>"); @@ builtin/update-ref.c: static const char *parse_cmd_create(struct ref_transaction *transaction, - if (!skip_prefix(next, " ", &next)) - die("create: missing space after command"); + char *refname; + struct object_id new_oid; - refname = parse_refname(input, &next); + refname = parse_refname(&next); @@ builtin/update-ref.c: static const char *parse_cmd_create(struct ref_transaction die("create: missing <ref>"); @@ builtin/update-ref.c: static const char *parse_cmd_delete(struct ref_transaction *transaction, - if (!skip_prefix(next, " ", &next)) - die("delete: missing space after command"); + struct object_id old_oid; + int have_old; - refname = parse_refname(input, &next); + refname = parse_refname(&next); @@ builtin/update-ref.c: static const char *parse_cmd_delete(struct ref_transaction die("delete: missing <ref>"); @@ builtin/update-ref.c: static const char *parse_cmd_verify(struct ref_transaction *transaction, - if (!skip_prefix(next, " ", &next)) - die("verify: missing space after command"); + char *refname; + struct object_id old_oid; - refname = parse_refname(input, &next); + refname = parse_refname(&next); 6: cbe430029d ! 6: 192cbf5944 update-ref: pass end pointer instead of strbuf @@ builtin/update-ref.c: static const char *parse_cmd_verify(struct ref_transaction + const char *next, const char *end) { const char *rest; - if (!skip_prefix(next, " ", &next)) + if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination) @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction *transaction, static const struct parse_cmd { 7: d2f68f59a7 = 7: be7bcf3dbd update-ref: move transaction handling into `update_refs_stdin()` 8: f8786fdeb3 ! 8: 02ff6b7337 update-ref: read commands in a line-wise fashion @@ Commit message ## builtin/update-ref.c ## @@ builtin/update-ref.c: static int parse_next_oid(const char **next, const char *end, - * line_termination is set. + * depending on how line_termination is set. */ -static const char *parse_cmd_update(struct ref_transaction *transaction, @@ builtin/update-ref.c: static const char *parse_cmd_verify(struct ref_transaction + const char *next, const char *end) { const char *rest; - if (!skip_prefix(next, " ", &next)) -@@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction *transaction, + if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination) update_flags |= REF_NO_DEREF; else die("option unknown: %s", next); @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction const char *prefix; - const char *(*fn)(struct ref_transaction *, const char *, const char *); + void (*fn)(struct ref_transaction *, const char *, const char *); -+ /* -+ * If using NUL-terminated format, only the first argument will be -+ * available in the first line. In case a command expects more than one -+ * argument, we thus have to fetch an additional `extra_lines` number -+ * of lines. -+ */ -+ unsigned extra_lines; ++ unsigned args; } command[] = { - { "update", parse_cmd_update }, - { "create", parse_cmd_create }, - { "delete", parse_cmd_delete }, - { "verify", parse_cmd_verify }, - { "option", parse_cmd_option }, -+ { "update", parse_cmd_update, 2 }, -+ { "create", parse_cmd_create, 1 }, -+ { "delete", parse_cmd_delete, 1 }, -+ { "verify", parse_cmd_verify, 1 }, -+ { "option", parse_cmd_option, 0 }, ++ { "update", parse_cmd_update, 3 }, ++ { "create", parse_cmd_create, 2 }, ++ { "delete", parse_cmd_delete, 2 }, ++ { "verify", parse_cmd_verify, 2 }, ++ { "option", parse_cmd_option, 1 }, }; static void update_refs_stdin(void) @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction for (i = 0; i < ARRAY_SIZE(command); i++) { const char *prefix = command[i].prefix; ++ char c; -- if (!skip_prefix(next, prefix, &next)) +- if (!skip_prefix(next, prefix, &next) || +- !skip_prefix(next, " ", &next)) + if (!starts_with(input.buf, prefix)) ++ continue; ++ ++ /* ++ * If the command has arguments, verify that it's ++ * followed by a space. Otherwise, it shall be followed ++ * by a line terminator. ++ */ ++ c = command[i].args ? ' ' : line_termination; ++ if (input.buf[strlen(prefix)] != c) continue; - /* -@@ builtin/update-ref.c: static void update_refs_stdin(void) + cmd = &command[i]; break; } if (!cmd) @@ builtin/update-ref.c: static void update_refs_stdin(void) - next = cmd->fn(transaction, next, input.buf + input.len); - next++; + /* -+ * Read extra lines if NUL-terminated. Do not raise an error in -+ * case there is an early EOF to let the command handle missing -+ * arguments with a proper error message. ++ * Read additional arguments if NUL-terminated. Do not raise an ++ * error in case there is an early EOF to let the command ++ * handle missing arguments with a proper error message. + */ -+ for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++) ++ for (j = 1; line_termination == '\0' && j < cmd->args; j++) + if (strbuf_appendwholeline(&input, stdin, line_termination)) + break; + -+ cmd->fn(transaction, input.buf + strlen(cmd->prefix), ++ cmd->fn(transaction, input.buf + strlen(cmd->prefix) + !!cmd->args, + input.buf + input.len); } 9: c3fffdf9fa ! 9: 5670bea2b1 update-ref: implement interactive transaction handling @@ builtin/update-ref.c: static void parse_cmd_option(struct ref_transaction *trans static const struct parse_cmd { const char *prefix; void (*fn)(struct ref_transaction *, const char *, const char *); -@@ builtin/update-ref.c: static const struct parse_cmd { - * of lines. - */ - unsigned extra_lines; + unsigned args; + enum update_refs_state state; } command[] = { -- { "update", parse_cmd_update, 2 }, -- { "create", parse_cmd_create, 1 }, -- { "delete", parse_cmd_delete, 1 }, -- { "verify", parse_cmd_verify, 1 }, -- { "option", parse_cmd_option, 0 }, -+ { "update", parse_cmd_update, 2, UPDATE_REFS_OPEN }, -+ { "create", parse_cmd_create, 1, UPDATE_REFS_OPEN }, -+ { "delete", parse_cmd_delete, 1, UPDATE_REFS_OPEN }, -+ { "verify", parse_cmd_verify, 1, UPDATE_REFS_OPEN }, -+ { "option", parse_cmd_option, 0, UPDATE_REFS_OPEN }, +- { "update", parse_cmd_update, 3 }, +- { "create", parse_cmd_create, 2 }, +- { "delete", parse_cmd_delete, 2 }, +- { "verify", parse_cmd_verify, 2 }, +- { "option", parse_cmd_option, 1 }, ++ { "update", parse_cmd_update, 3, UPDATE_REFS_OPEN }, ++ { "create", parse_cmd_create, 2, UPDATE_REFS_OPEN }, ++ { "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN }, ++ { "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN }, ++ { "option", parse_cmd_option, 1, UPDATE_REFS_OPEN }, + { "start", parse_cmd_start, 0, UPDATE_REFS_STARTED }, + { "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED }, + { "abort", parse_cmd_abort, 0, UPDATE_REFS_CLOSED }, @@ builtin/update-ref.c: static void update_refs_stdin(void) + break; + } + - cmd->fn(transaction, input.buf + strlen(cmd->prefix), + cmd->fn(transaction, input.buf + strlen(cmd->prefix) + !!cmd->args, input.buf + input.len); } -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1707 bytes --] When cleaning up a transaction that has no updates queued, then the transaction's backend data will not have been allocated. We correctly handle this for the packed backend, where the cleanup function checks whether the backend data has been allocated at all -- if not, then there is nothing to clean up. For the files backend we do not check this and as a result will hit a segfault due to dereferencing a `NULL` pointer when cleaning up such a transaction. Fix the issue by checking whether `backend_data` is set in the files backend, too. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- refs/files-backend.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 561c33ac8a..6516c7bc8c 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2565,17 +2565,19 @@ static void files_transaction_cleanup(struct files_ref_store *refs, } } - if (backend_data->packed_transaction && - ref_transaction_abort(backend_data->packed_transaction, &err)) { - error("error aborting transaction: %s", err.buf); - strbuf_release(&err); + if (backend_data) { + if (backend_data->packed_transaction && + ref_transaction_abort(backend_data->packed_transaction, &err)) { + error("error aborting transaction: %s", err.buf); + strbuf_release(&err); + } + + if (backend_data->packed_refs_locked) + packed_refs_unlock(refs->packed_ref_store); + + free(backend_data); } - if (backend_data->packed_refs_locked) - packed_refs_unlock(refs->packed_ref_store); - - free(backend_data); - transaction->state = REF_TRANSACTION_CLOSED; } -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 749 bytes --] The description for the "verify" command is lacking a single word "is", which this commit corrects. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Documentation/git-update-ref.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt index 9671423117..9bd039ce08 100644 --- a/Documentation/git-update-ref.txt +++ b/Documentation/git-update-ref.txt @@ -107,7 +107,7 @@ delete:: verify:: Verify <ref> against <oldvalue> but do not change it. If - <oldvalue> zero or missing, the ref must not exist. + <oldvalue> is zero or missing, the ref must not exist. option:: Modify behavior of the next command naming a <ref>. -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1765 bytes --] While the strbuf interface already provides functions to read a line into it that completely replaces its current contents, we do not have an interface that allows for appending lines without discarding current contents. Add a new function `strbuf_appendwholeline` that reads a line including its terminating character into a strbuf non-destructively. This is a preparatory step for git-update-ref(1) reading standard input line-wise instead of as a block. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- strbuf.c | 10 ++++++++++ strbuf.h | 6 ++++++ 2 files changed, 16 insertions(+) diff --git a/strbuf.c b/strbuf.c index bb0065ccaf..6e74901bfa 100644 --- a/strbuf.c +++ b/strbuf.c @@ -690,6 +690,16 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term) } #endif +int strbuf_appendwholeline(struct strbuf *sb, FILE *fp, int term) +{ + struct strbuf line = STRBUF_INIT; + if (strbuf_getwholeline(&line, fp, term)) + return EOF; + strbuf_addbuf(sb, &line); + strbuf_release(&line); + return 0; +} + static int strbuf_getdelim(struct strbuf *sb, FILE *fp, int term) { if (strbuf_getwholeline(sb, fp, term)) diff --git a/strbuf.h b/strbuf.h index ce8e49c0b2..411063ca76 100644 --- a/strbuf.h +++ b/strbuf.h @@ -502,6 +502,12 @@ int strbuf_getline(struct strbuf *sb, FILE *file); */ int strbuf_getwholeline(struct strbuf *sb, FILE *file, int term); +/** + * Like `strbuf_getwholeline`, but appends the line instead of + * resetting the buffer first. + */ +int strbuf_appendwholeline(struct strbuf *sb, FILE *file, int term); + /** * Like `strbuf_getwholeline`, but operates on a file descriptor. * It reads one character at a time, so it is very slow. Do not -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 3173 bytes --] We currently manually wire up all commands known to `git-update-ref --stdin`, making it harder than necessary to preprocess arguments after the command is determined. To make this more extensible, let's refactor the code to use an array of known commands instead. While this doesn't add a lot of value now, it is a preparatory step to implement line-wise reading of commands. As we're going to introduce commands without trailing spaces, this commit also moves whitespace parsing into the respective commands. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/update-ref.c | 41 +++++++++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 2d8f7f0578..a6ff88b95b 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -310,7 +310,8 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, return next; } -static const char *parse_cmd_option(struct strbuf *input, const char *next) +static const char *parse_cmd_option(struct ref_transaction *transaction, + struct strbuf *input, const char *next) { const char *rest; if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination) @@ -320,33 +321,49 @@ static const char *parse_cmd_option(struct strbuf *input, const char *next) return rest; } +static const struct parse_cmd { + const char *prefix; + const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *); +} command[] = { + { "update", parse_cmd_update }, + { "create", parse_cmd_create }, + { "delete", parse_cmd_delete }, + { "verify", parse_cmd_verify }, + { "option", parse_cmd_option }, +}; + static void update_refs_stdin(struct ref_transaction *transaction) { struct strbuf input = STRBUF_INIT; const char *next; + int i; if (strbuf_read(&input, 0, 1000) < 0) die_errno("could not read from stdin"); next = input.buf; /* Read each line dispatch its command */ while (next < input.buf + input.len) { + const struct parse_cmd *cmd = NULL; + if (*next == line_termination) die("empty command in input"); else if (isspace(*next)) die("whitespace before command: %s", next); - else if (skip_prefix(next, "update ", &next)) - next = parse_cmd_update(transaction, &input, next); - else if (skip_prefix(next, "create ", &next)) - next = parse_cmd_create(transaction, &input, next); - else if (skip_prefix(next, "delete ", &next)) - next = parse_cmd_delete(transaction, &input, next); - else if (skip_prefix(next, "verify ", &next)) - next = parse_cmd_verify(transaction, &input, next); - else if (skip_prefix(next, "option ", &next)) - next = parse_cmd_option(&input, next); - else + + for (i = 0; i < ARRAY_SIZE(command); i++) { + const char *prefix = command[i].prefix; + + if (!skip_prefix(next, prefix, &next) || + !skip_prefix(next, " ", &next)) + continue; + + cmd = &command[i]; + break; + } + if (!cmd) die("unknown command: %s", next); + next = cmd->fn(transaction, &input, next); next++; } -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 2027 bytes --] The `parse_refname` function accepts a `struct strbuf *input` argument that isn't used at all. As we're about to convert commands to not use a strbuf anymore but instead an end pointer, let's drop this argument now to make the converting commit easier to review. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/update-ref.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index a6ff88b95b..1bba5ea6c2 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -50,7 +50,7 @@ static const char *parse_arg(const char *next, struct strbuf *arg) * the argument. Die if C-quoting is malformed or the reference name * is invalid. */ -static char *parse_refname(struct strbuf *input, const char **next) +static char *parse_refname(const char **next) { struct strbuf ref = STRBUF_INIT; @@ -186,7 +186,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction, struct object_id new_oid, old_oid; int have_old; - refname = parse_refname(input, &next); + refname = parse_refname(&next); if (!refname) die("update: missing <ref>"); @@ -220,7 +220,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction, char *refname; struct object_id new_oid; - refname = parse_refname(input, &next); + refname = parse_refname(&next); if (!refname) die("create: missing <ref>"); @@ -253,7 +253,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction, struct object_id old_oid; int have_old; - refname = parse_refname(input, &next); + refname = parse_refname(&next); if (!refname) die("delete: missing <ref>"); @@ -288,7 +288,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, char *refname; struct object_id old_oid; - refname = parse_refname(input, &next); + refname = parse_refname(&next); if (!refname) die("verify: missing <ref>"); -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 5625 bytes --] We currently pass both an `strbuf` containing the current command line as well as the `next` pointer pointing to the first argument to commands. This is both confusing and makes code more intertwined. Convert this to use a simple pointer as well as a pointer pointing to the end of the input as a preparatory step to line-wise reading of stdin. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/update-ref.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 1bba5ea6c2..381d347fb4 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -95,7 +95,7 @@ static char *parse_refname(const char **next) * 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_oid(struct strbuf *input, const char **next, +static int parse_next_oid(const char **next, const char *end, struct object_id *oid, const char *command, const char *refname, int flags) @@ -103,7 +103,7 @@ static int parse_next_oid(struct strbuf *input, const char **next, struct strbuf arg = STRBUF_INIT; int ret = 0; - if (*next == input->buf + input->len) + if (*next == end) goto eof; if (line_termination) { @@ -128,7 +128,7 @@ static int parse_next_oid(struct strbuf *input, const char **next, die("%s %s: expected NUL but got: %s", command, refname, *next); (*next)++; - if (*next == input->buf + input->len) + if (*next == end) goto eof; strbuf_addstr(&arg, *next); *next += arg.len; @@ -179,7 +179,7 @@ static int parse_next_oid(struct strbuf *input, const char **next, */ static const char *parse_cmd_update(struct ref_transaction *transaction, - struct strbuf *input, const char *next) + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -190,11 +190,11 @@ static const char *parse_cmd_update(struct ref_transaction *transaction, if (!refname) die("update: missing <ref>"); - if (parse_next_oid(input, &next, &new_oid, "update", refname, + if (parse_next_oid(&next, end, &new_oid, "update", refname, PARSE_SHA1_ALLOW_EMPTY)) die("update %s: missing <newvalue>", refname); - have_old = !parse_next_oid(input, &next, &old_oid, "update", refname, + have_old = !parse_next_oid(&next, end, &old_oid, "update", refname, PARSE_SHA1_OLD); if (*next != line_termination) @@ -214,7 +214,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction, } static const char *parse_cmd_create(struct ref_transaction *transaction, - struct strbuf *input, const char *next) + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -224,7 +224,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction, if (!refname) die("create: missing <ref>"); - if (parse_next_oid(input, &next, &new_oid, "create", refname, 0)) + if (parse_next_oid(&next, end, &new_oid, "create", refname, 0)) die("create %s: missing <newvalue>", refname); if (is_null_oid(&new_oid)) @@ -246,7 +246,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction, } static const char *parse_cmd_delete(struct ref_transaction *transaction, - struct strbuf *input, const char *next) + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -257,7 +257,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction, if (!refname) die("delete: missing <ref>"); - if (parse_next_oid(input, &next, &old_oid, "delete", refname, + if (parse_next_oid(&next, end, &old_oid, "delete", refname, PARSE_SHA1_OLD)) { have_old = 0; } else { @@ -282,7 +282,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction, } static const char *parse_cmd_verify(struct ref_transaction *transaction, - struct strbuf *input, const char *next) + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -292,7 +292,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, if (!refname) die("verify: missing <ref>"); - if (parse_next_oid(input, &next, &old_oid, "verify", refname, + if (parse_next_oid(&next, end, &old_oid, "verify", refname, PARSE_SHA1_OLD)) oidclr(&old_oid); @@ -311,7 +311,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, } static const char *parse_cmd_option(struct ref_transaction *transaction, - struct strbuf *input, const char *next) + const char *next, const char *end) { const char *rest; if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination) @@ -323,7 +323,7 @@ static const char *parse_cmd_option(struct ref_transaction *transaction, static const struct parse_cmd { const char *prefix; - const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *); + const char *(*fn)(struct ref_transaction *, const char *, const char *); } command[] = { { "update", parse_cmd_update }, { "create", parse_cmd_create }, @@ -363,7 +363,7 @@ static void update_refs_stdin(struct ref_transaction *transaction) if (!cmd) die("unknown command: %s", next); - next = cmd->fn(transaction, &input, next); + next = cmd->fn(transaction, next, input.buf + input.len); next++; } -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 2473 bytes --] While the actual logic to update the transaction is handled in `update_refs_stdin()`, the transaction itself is started and committed in `cmd_update_ref()` itself. This makes it hard to handle transaction abortion and commits as part of `update_refs_stdin()` itself, which is required in order to introduce transaction handling features to `git update-refs --stdin`. Refactor the code to move all transaction handling into `update_refs_stdin()` to prepare for transaction handling features. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/update-ref.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 381d347fb4..0f34b68904 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -332,15 +332,21 @@ static const struct parse_cmd { { "option", parse_cmd_option }, }; -static void update_refs_stdin(struct ref_transaction *transaction) +static void update_refs_stdin(void) { - struct strbuf input = STRBUF_INIT; + struct strbuf input = STRBUF_INIT, err = STRBUF_INIT; + struct ref_transaction *transaction; const char *next; int i; if (strbuf_read(&input, 0, 1000) < 0) die_errno("could not read from stdin"); next = input.buf; + + transaction = ref_transaction_begin(&err); + if (!transaction) + die("%s", err.buf); + /* Read each line dispatch its command */ while (next < input.buf + input.len) { const struct parse_cmd *cmd = NULL; @@ -367,6 +373,11 @@ static void update_refs_stdin(struct ref_transaction *transaction) next++; } + if (ref_transaction_commit(transaction, &err)) + die("%s", err.buf); + + ref_transaction_free(transaction); + strbuf_release(&err); strbuf_release(&input); } @@ -401,21 +412,11 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix) } if (read_stdin) { - struct strbuf err = STRBUF_INIT; - struct ref_transaction *transaction; - - transaction = ref_transaction_begin(&err); - if (!transaction) - die("%s", err.buf); if (delete || argc > 0) usage_with_options(git_update_ref_usage, options); if (end_null) line_termination = '\0'; - update_refs_stdin(transaction); - if (ref_transaction_commit(transaction, &err)) - die("%s", err.buf); - ref_transaction_free(transaction); - strbuf_release(&err); + update_refs_stdin(); return 0; } -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 6877 bytes --] The git-update-ref(1) supports a `--stdin` mode that allows it to read all reference updates from standard input. This is mainly used to allow for atomic reference updates that are all or nothing, so that either all references will get updated or none. Currently, git-update-ref(1) reads all commands as a single block of up to 1000 characters and only starts processing after stdin gets closed. This is less flexible than one might wish for, as it doesn't really allow for longer-lived transactions and doesn't allow any verification without committing everything. E.g. one may imagine the following exchange: > start < start: ok > update refs/heads/master $NEWOID1 $OLDOID1 > update refs/heads/branch $NEWOID2 $OLDOID2 > prepare < prepare: ok > commit < commit: ok When reading all input as a whole block, the above interactive protocol is obviously impossible to achieve. But by converting the command to read commands linewise, we can make it more interactive than before. Obviously, the linewise interface is only a first step in making git-update-ref(1) work in a more transaction-oriented way. Missing is most importantly support for transactional commands that manage the current transaction. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/update-ref.c | 85 +++++++++++++++++++++++--------------------- 1 file changed, 45 insertions(+), 40 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 0f34b68904..348407b896 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -178,8 +178,8 @@ static int parse_next_oid(const char **next, const char *end, * depending on how line_termination is set. */ -static const char *parse_cmd_update(struct ref_transaction *transaction, - const char *next, const char *end) +static void parse_cmd_update(struct ref_transaction *transaction, + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -209,12 +209,10 @@ static const char *parse_cmd_update(struct ref_transaction *transaction, update_flags = default_flags; free(refname); strbuf_release(&err); - - return next; } -static const char *parse_cmd_create(struct ref_transaction *transaction, - const char *next, const char *end) +static void parse_cmd_create(struct ref_transaction *transaction, + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -241,12 +239,10 @@ static const char *parse_cmd_create(struct ref_transaction *transaction, update_flags = default_flags; free(refname); strbuf_release(&err); - - return next; } -static const char *parse_cmd_delete(struct ref_transaction *transaction, - const char *next, const char *end) +static void parse_cmd_delete(struct ref_transaction *transaction, + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -277,12 +273,10 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction, update_flags = default_flags; free(refname); strbuf_release(&err); - - return next; } -static const char *parse_cmd_verify(struct ref_transaction *transaction, - const char *next, const char *end) +static void parse_cmd_verify(struct ref_transaction *transaction, + const char *next, const char *end) { struct strbuf err = STRBUF_INIT; char *refname; @@ -306,71 +300,82 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction, update_flags = default_flags; free(refname); strbuf_release(&err); - - return next; } -static const char *parse_cmd_option(struct ref_transaction *transaction, - const char *next, const char *end) +static void parse_cmd_option(struct ref_transaction *transaction, + const char *next, const char *end) { const char *rest; if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination) update_flags |= REF_NO_DEREF; else die("option unknown: %s", next); - return rest; } static const struct parse_cmd { const char *prefix; - const char *(*fn)(struct ref_transaction *, const char *, const char *); + void (*fn)(struct ref_transaction *, const char *, const char *); + unsigned args; } command[] = { - { "update", parse_cmd_update }, - { "create", parse_cmd_create }, - { "delete", parse_cmd_delete }, - { "verify", parse_cmd_verify }, - { "option", parse_cmd_option }, + { "update", parse_cmd_update, 3 }, + { "create", parse_cmd_create, 2 }, + { "delete", parse_cmd_delete, 2 }, + { "verify", parse_cmd_verify, 2 }, + { "option", parse_cmd_option, 1 }, }; static void update_refs_stdin(void) { struct strbuf input = STRBUF_INIT, err = STRBUF_INIT; struct ref_transaction *transaction; - const char *next; - int i; - - if (strbuf_read(&input, 0, 1000) < 0) - die_errno("could not read from stdin"); - next = input.buf; + int i, j; transaction = ref_transaction_begin(&err); if (!transaction) die("%s", err.buf); /* Read each line dispatch its command */ - while (next < input.buf + input.len) { + while (!strbuf_getwholeline(&input, stdin, line_termination)) { const struct parse_cmd *cmd = NULL; - if (*next == line_termination) + if (*input.buf == line_termination) die("empty command in input"); - else if (isspace(*next)) - die("whitespace before command: %s", next); + else if (isspace(*input.buf)) + die("whitespace before command: %s", input.buf); for (i = 0; i < ARRAY_SIZE(command); i++) { const char *prefix = command[i].prefix; + char c; - if (!skip_prefix(next, prefix, &next) || - !skip_prefix(next, " ", &next)) + if (!starts_with(input.buf, prefix)) + continue; + + /* + * If the command has arguments, verify that it's + * followed by a space. Otherwise, it shall be followed + * by a line terminator. + */ + c = command[i].args ? ' ' : line_termination; + if (input.buf[strlen(prefix)] != c) continue; cmd = &command[i]; break; } if (!cmd) - die("unknown command: %s", next); + die("unknown command: %s", input.buf); - next = cmd->fn(transaction, next, input.buf + input.len); - next++; + /* + * Read additional arguments if NUL-terminated. Do not raise an + * error in case there is an early EOF to let the command + * handle missing arguments with a proper error message. + */ + for (j = 1; line_termination == '\0' && j < cmd->args; j++) + if (strbuf_appendwholeline(&input, stdin, line_termination)) + break; + + cmd->fn(transaction, input.buf + strlen(cmd->prefix) + !!cmd->args, + input.buf + input.len); } if (ref_transaction_commit(transaction, &err)) -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 12013 bytes --] The git-update-ref(1) command can only handle queueing transactions right now via its "--stdin" parameter, but there is no way for users to handle the transaction itself in a more explicit way. E.g. in a replicated scenario, one may imagine a coordinator that spawns git-update-ref(1) for multiple repositories and only if all agree that an update is possible will the coordinator send a commit. Such a transactional session could look like > start < start: ok > update refs/heads/master $OLD $NEW > prepare < prepare: ok # All nodes have returned "ok" > commit < commit: ok or > start < start: ok > create refs/heads/master $OLD $NEW > prepare < fatal: cannot lock ref 'refs/heads/master': reference already exists # On all other nodes: > abort < abort: ok In order to allow for such transactional sessions, this commit introduces four new commands for git-update-ref(1), which matches those we have internally already with the exception of "start": - start: start a new transaction - prepare: prepare the transaction, that is try to lock all references and verify their current value matches the expected one - commit: explicitly commit a session, that is update references to match their new expected state - abort: abort a session and roll back all changes By design, git-update-ref(1) will commit as soon as standard input is being closed. While fine in a non-transactional world, it is definitely unexpected in a transactional world. Because of this, as soon as any of the new transactional commands is used, the default will change to aborting without an explicit "commit". To avoid a race between queueing updates and the first "prepare" that starts a transaction, the "start" command has been added to start an explicit transaction. Add some tests to exercise this new functionality. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Documentation/git-update-ref.txt | 26 ++++++ builtin/update-ref.c | 106 +++++++++++++++++++++++-- t/t1400-update-ref.sh | 131 +++++++++++++++++++++++++++++++ 3 files changed, 255 insertions(+), 8 deletions(-) diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt index 9bd039ce08..3e737c2360 100644 --- a/Documentation/git-update-ref.txt +++ b/Documentation/git-update-ref.txt @@ -66,6 +66,10 @@ performs all modifications together. Specify commands of the form: delete SP <ref> [SP <oldvalue>] LF verify SP <ref> [SP <oldvalue>] LF option SP <opt> LF + start LF + prepare LF + commit LF + abort LF With `--create-reflog`, update-ref will create a reflog for each ref even if one would not ordinarily be created. @@ -83,6 +87,10 @@ quoting: delete SP <ref> NUL [<oldvalue>] NUL verify SP <ref> NUL [<oldvalue>] NUL option SP <opt> NUL + start NUL + prepare NUL + commit NUL + abort NUL In this format, use 40 "0" to specify a zero value, and use the empty string to specify a missing value. @@ -114,6 +122,24 @@ option:: The only valid option is `no-deref` to avoid dereferencing a symbolic ref. +start:: + Start a transaction. In contrast to a non-transactional session, a + transaction will automatically abort if the session ends without an + explicit commit. + +prepare:: + Prepare to commit the transaction. This will create lock files for all + queued reference updates. If one reference could not be locked, the + transaction will be aborted. + +commit:: + Commit all reference updates queued for the transaction, ending the + transaction. + +abort:: + Abort the transaction, releasing all locks if the transaction is in + prepared state. + If all <ref>s can be locked with matching <oldvalue>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 348407b896..b74dd9a69d 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -312,21 +312,80 @@ static void parse_cmd_option(struct ref_transaction *transaction, die("option unknown: %s", next); } +static void parse_cmd_start(struct ref_transaction *transaction, + const char *next, const char *end) +{ + if (*next != line_termination) + die("start: extra input: %s", next); + puts("start: ok"); +} + +static void parse_cmd_prepare(struct ref_transaction *transaction, + const char *next, const char *end) +{ + struct strbuf error = STRBUF_INIT; + if (*next != line_termination) + die("prepare: extra input: %s", next); + if (ref_transaction_prepare(transaction, &error)) + die("prepare: %s", error.buf); + puts("prepare: ok"); +} + +static void parse_cmd_abort(struct ref_transaction *transaction, + const char *next, const char *end) +{ + struct strbuf error = STRBUF_INIT; + if (*next != line_termination) + die("abort: extra input: %s", next); + if (ref_transaction_abort(transaction, &error)) + die("abort: %s", error.buf); + puts("abort: ok"); +} + +static void parse_cmd_commit(struct ref_transaction *transaction, + const char *next, const char *end) +{ + struct strbuf error = STRBUF_INIT; + if (*next != line_termination) + die("commit: extra input: %s", next); + if (ref_transaction_commit(transaction, &error)) + die("commit: %s", error.buf); + puts("commit: ok"); + ref_transaction_free(transaction); +} + +enum update_refs_state { + /* Non-transactional state open for updates. */ + UPDATE_REFS_OPEN, + /* A transaction has been started. */ + UPDATE_REFS_STARTED, + /* References are locked and ready for commit */ + UPDATE_REFS_PREPARED, + /* Transaction has been committed or closed. */ + UPDATE_REFS_CLOSED, +}; + static const struct parse_cmd { const char *prefix; void (*fn)(struct ref_transaction *, const char *, const char *); unsigned args; + enum update_refs_state state; } command[] = { - { "update", parse_cmd_update, 3 }, - { "create", parse_cmd_create, 2 }, - { "delete", parse_cmd_delete, 2 }, - { "verify", parse_cmd_verify, 2 }, - { "option", parse_cmd_option, 1 }, + { "update", parse_cmd_update, 3, UPDATE_REFS_OPEN }, + { "create", parse_cmd_create, 2, UPDATE_REFS_OPEN }, + { "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN }, + { "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN }, + { "option", parse_cmd_option, 1, UPDATE_REFS_OPEN }, + { "start", parse_cmd_start, 0, UPDATE_REFS_STARTED }, + { "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED }, + { "abort", parse_cmd_abort, 0, UPDATE_REFS_CLOSED }, + { "commit", parse_cmd_commit, 0, UPDATE_REFS_CLOSED }, }; static void update_refs_stdin(void) { struct strbuf input = STRBUF_INIT, err = STRBUF_INIT; + enum update_refs_state state = UPDATE_REFS_OPEN; struct ref_transaction *transaction; int i, j; @@ -374,14 +433,45 @@ static void update_refs_stdin(void) if (strbuf_appendwholeline(&input, stdin, line_termination)) break; + switch (state) { + case UPDATE_REFS_OPEN: + case UPDATE_REFS_STARTED: + /* Do not downgrade a transaction to a non-transaction. */ + if (cmd->state >= state) + state = cmd->state; + break; + case UPDATE_REFS_PREPARED: + if (cmd->state != UPDATE_REFS_CLOSED) + die("prepared transactions can only be closed"); + state = cmd->state; + break; + case UPDATE_REFS_CLOSED: + die("transaction is closed"); + break; + } + cmd->fn(transaction, input.buf + strlen(cmd->prefix) + !!cmd->args, input.buf + input.len); } - if (ref_transaction_commit(transaction, &err)) - die("%s", err.buf); + switch (state) { + case UPDATE_REFS_OPEN: + /* Commit by default if no transaction was requested. */ + if (ref_transaction_commit(transaction, &err)) + die("%s", err.buf); + ref_transaction_free(transaction); + break; + case UPDATE_REFS_STARTED: + case UPDATE_REFS_PREPARED: + /* If using a transaction, we want to abort it. */ + if (ref_transaction_abort(transaction, &err)) + die("%s", err.buf); + break; + case UPDATE_REFS_CLOSED: + /* Otherwise no need to do anything, the transaction was closed already. */ + break; + } - ref_transaction_free(transaction); strbuf_release(&err); strbuf_release(&input); } diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index a6224ef65f..48d0d42afd 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -1404,4 +1404,135 @@ test_expect_success 'handle per-worktree refs in refs/bisect' ' ! test_cmp main-head worktree-head ' +test_expect_success 'transaction handles empty commit' ' + cat >stdin <<-EOF && + start + prepare + commit + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start prepare commit >expect && + test_cmp expect actual +' + +test_expect_success 'transaction handles empty commit with missing prepare' ' + cat >stdin <<-EOF && + start + commit + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start commit >expect && + test_cmp expect actual +' + +test_expect_success 'transaction handles sole commit' ' + cat >stdin <<-EOF && + commit + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" commit >expect && + test_cmp expect actual +' + +test_expect_success 'transaction handles empty abort' ' + cat >stdin <<-EOF && + start + prepare + abort + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start prepare abort >expect && + test_cmp expect actual +' + +test_expect_success 'transaction exits on multiple aborts' ' + cat >stdin <<-EOF && + abort + abort + EOF + test_must_fail git update-ref --stdin <stdin >actual 2>err && + printf "%s: ok\n" abort >expect && + test_cmp expect actual && + grep "fatal: transaction is closed" err +' + +test_expect_success 'transaction exits on start after prepare' ' + cat >stdin <<-EOF && + prepare + start + EOF + test_must_fail git update-ref --stdin <stdin 2>err >actual && + printf "%s: ok\n" prepare >expect && + test_cmp expect actual && + grep "fatal: prepared transactions can only be closed" err +' + +test_expect_success 'transaction handles empty abort with missing prepare' ' + cat >stdin <<-EOF && + start + abort + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start abort >expect && + test_cmp expect actual +' + +test_expect_success 'transaction handles sole abort' ' + cat >stdin <<-EOF && + abort + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" abort >expect && + test_cmp expect actual +' + +test_expect_success 'transaction can handle commit' ' + cat >stdin <<-EOF && + start + create $a HEAD + commit + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start commit >expect && + test_cmp expect actual && + git rev-parse HEAD >expect && + git rev-parse $a >actual && + test_cmp expect actual +' + +test_expect_success 'transaction can handle abort' ' + cat >stdin <<-EOF && + start + create $b HEAD + abort + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start abort >expect && + test_cmp expect actual && + test_path_is_missing .git/$b +' + +test_expect_success 'transaction aborts by default' ' + cat >stdin <<-EOF && + start + create $b HEAD + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start >expect && + test_cmp expect actual && + test_path_is_missing .git/$b +' + +test_expect_success 'transaction with prepare aborts by default' ' + cat >stdin <<-EOF && + start + create $b HEAD + prepare + EOF + git update-ref --stdin <stdin >actual && + printf "%s: ok\n" start prepare >expect && + test_cmp expect actual && + test_path_is_missing .git/$b +' + test_done -- 2.26.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
Hi Patrick On 02/04/2020 08:10, Patrick Steinhardt wrote: > The git-update-ref(1) command can only handle queueing transactions > right now via its "--stdin" parameter, but there is no way for users to > handle the transaction itself in a more explicit way. E.g. in a > replicated scenario, one may imagine a coordinator that spawns > git-update-ref(1) for multiple repositories and only if all agree that > an update is possible will the coordinator send a commit. Such a > transactional session could look like > > > start > < start: ok > > update refs/heads/master $OLD $NEW > > prepare > < prepare: ok > # All nodes have returned "ok" > > commit > < commit: ok > > or > > > start > < start: ok > > create refs/heads/master $OLD $NEW > > prepare > < fatal: cannot lock ref 'refs/heads/master': reference already exists > # On all other nodes: > > abort > < abort: ok > > In order to allow for such transactional sessions, this commit > introduces four new commands for git-update-ref(1), which matches those > we have internally already with the exception of "start": > > - start: start a new transaction > > - prepare: prepare the transaction, that is try to lock all > references and verify their current value matches the > expected one > > - commit: explicitly commit a session, that is update references to > match their new expected state > > - abort: abort a session and roll back all changes > > By design, git-update-ref(1) will commit as soon as standard input is > being closed. While fine in a non-transactional world, it is definitely > unexpected in a transactional world. Because of this, as soon as any of > the new transactional commands is used, the default will change to > aborting without an explicit "commit". To avoid a race between queueing > updates and the first "prepare" that starts a transaction, the "start" > command has been added to start an explicit transaction. > > Add some tests to exercise this new functionality. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > Documentation/git-update-ref.txt | 26 ++++++ > builtin/update-ref.c | 106 +++++++++++++++++++++++-- > t/t1400-update-ref.sh | 131 +++++++++++++++++++++++++++++++ > 3 files changed, 255 insertions(+), 8 deletions(-) > > diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt > index 9bd039ce08..3e737c2360 100644 > --- a/Documentation/git-update-ref.txt > +++ b/Documentation/git-update-ref.txt > @@ -66,6 +66,10 @@ performs all modifications together. Specify commands of the form: > delete SP <ref> [SP <oldvalue>] LF > verify SP <ref> [SP <oldvalue>] LF > option SP <opt> LF > + start LF > + prepare LF > + commit LF > + abort LF > > With `--create-reflog`, update-ref will create a reflog for each ref > even if one would not ordinarily be created. > @@ -83,6 +87,10 @@ quoting: > delete SP <ref> NUL [<oldvalue>] NUL > verify SP <ref> NUL [<oldvalue>] NUL > option SP <opt> NUL > + start NUL > + prepare NUL > + commit NUL > + abort NUL > > In this format, use 40 "0" to specify a zero value, and use the empty > string to specify a missing value. > @@ -114,6 +122,24 @@ option:: > The only valid option is `no-deref` to avoid dereferencing > a symbolic ref. > > +start:: > + Start a transaction. In contrast to a non-transactional session, I found the talk of "non-transactional" sessions a bit confusing because the normal --stdin does update all the refs it is given as a single transaction, so that if it cannot update one ref none of them are updated. If I've understood correctly these changes are about coordinating transactions across several repositories. I'm not sure how best to convey that in the man page - perhaps we could call them single repository transactions and multi repository transaction or something. Best Wishes Phillip > a > + transaction will automatically abort if the session ends without an > + explicit commit. > + > +prepare:: > + Prepare to commit the transaction. This will create lock files for all > + queued reference updates. If one reference could not be locked, the > + transaction will be aborted. > + > +commit:: > + Commit all reference updates queued for the transaction, ending the > + transaction. > + > +abort:: > + Abort the transaction, releasing all locks if the transaction is in > + prepared state. > + > If all <ref>s can be locked with matching <oldvalue>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 348407b896..b74dd9a69d 100644 > --- a/builtin/update-ref.c > +++ b/builtin/update-ref.c > @@ -312,21 +312,80 @@ static void parse_cmd_option(struct ref_transaction *transaction, > die("option unknown: %s", next); > } > > +static void parse_cmd_start(struct ref_transaction *transaction, > + const char *next, const char *end) > +{ > + if (*next != line_termination) > + die("start: extra input: %s", next); > + puts("start: ok"); > +} > + > +static void parse_cmd_prepare(struct ref_transaction *transaction, > + const char *next, const char *end) > +{ > + struct strbuf error = STRBUF_INIT; > + if (*next != line_termination) > + die("prepare: extra input: %s", next); > + if (ref_transaction_prepare(transaction, &error)) > + die("prepare: %s", error.buf); > + puts("prepare: ok"); > +} > + > +static void parse_cmd_abort(struct ref_transaction *transaction, > + const char *next, const char *end) > +{ > + struct strbuf error = STRBUF_INIT; > + if (*next != line_termination) > + die("abort: extra input: %s", next); > + if (ref_transaction_abort(transaction, &error)) > + die("abort: %s", error.buf); > + puts("abort: ok"); > +} > + > +static void parse_cmd_commit(struct ref_transaction *transaction, > + const char *next, const char *end) > +{ > + struct strbuf error = STRBUF_INIT; > + if (*next != line_termination) > + die("commit: extra input: %s", next); > + if (ref_transaction_commit(transaction, &error)) > + die("commit: %s", error.buf); > + puts("commit: ok"); > + ref_transaction_free(transaction); > +} > + > +enum update_refs_state { > + /* Non-transactional state open for updates. */ > + UPDATE_REFS_OPEN, > + /* A transaction has been started. */ > + UPDATE_REFS_STARTED, > + /* References are locked and ready for commit */ > + UPDATE_REFS_PREPARED, > + /* Transaction has been committed or closed. */ > + UPDATE_REFS_CLOSED, > +}; > + > static const struct parse_cmd { > const char *prefix; > void (*fn)(struct ref_transaction *, const char *, const char *); > unsigned args; > + enum update_refs_state state; > } command[] = { > - { "update", parse_cmd_update, 3 }, > - { "create", parse_cmd_create, 2 }, > - { "delete", parse_cmd_delete, 2 }, > - { "verify", parse_cmd_verify, 2 }, > - { "option", parse_cmd_option, 1 }, > + { "update", parse_cmd_update, 3, UPDATE_REFS_OPEN }, > + { "create", parse_cmd_create, 2, UPDATE_REFS_OPEN }, > + { "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN }, > + { "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN }, > + { "option", parse_cmd_option, 1, UPDATE_REFS_OPEN }, > + { "start", parse_cmd_start, 0, UPDATE_REFS_STARTED }, > + { "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED }, > + { "abort", parse_cmd_abort, 0, UPDATE_REFS_CLOSED }, > + { "commit", parse_cmd_commit, 0, UPDATE_REFS_CLOSED }, > }; > > static void update_refs_stdin(void) > { > struct strbuf input = STRBUF_INIT, err = STRBUF_INIT; > + enum update_refs_state state = UPDATE_REFS_OPEN; > struct ref_transaction *transaction; > int i, j; > > @@ -374,14 +433,45 @@ static void update_refs_stdin(void) > if (strbuf_appendwholeline(&input, stdin, line_termination)) > break; > > + switch (state) { > + case UPDATE_REFS_OPEN: > + case UPDATE_REFS_STARTED: > + /* Do not downgrade a transaction to a non-transaction. */ > + if (cmd->state >= state) > + state = cmd->state; > + break; > + case UPDATE_REFS_PREPARED: > + if (cmd->state != UPDATE_REFS_CLOSED) > + die("prepared transactions can only be closed"); > + state = cmd->state; > + break; > + case UPDATE_REFS_CLOSED: > + die("transaction is closed"); > + break; > + } > + > cmd->fn(transaction, input.buf + strlen(cmd->prefix) + !!cmd->args, > input.buf + input.len); > } > > - if (ref_transaction_commit(transaction, &err)) > - die("%s", err.buf); > + switch (state) { > + case UPDATE_REFS_OPEN: > + /* Commit by default if no transaction was requested. */ > + if (ref_transaction_commit(transaction, &err)) > + die("%s", err.buf); > + ref_transaction_free(transaction); > + break; > + case UPDATE_REFS_STARTED: > + case UPDATE_REFS_PREPARED: > + /* If using a transaction, we want to abort it. */ > + if (ref_transaction_abort(transaction, &err)) > + die("%s", err.buf); > + break; > + case UPDATE_REFS_CLOSED: > + /* Otherwise no need to do anything, the transaction was closed already. */ > + break; > + } > > - ref_transaction_free(transaction); > strbuf_release(&err); > strbuf_release(&input); > } > diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh > index a6224ef65f..48d0d42afd 100755 > --- a/t/t1400-update-ref.sh > +++ b/t/t1400-update-ref.sh > @@ -1404,4 +1404,135 @@ test_expect_success 'handle per-worktree refs in refs/bisect' ' > ! test_cmp main-head worktree-head > ' > > +test_expect_success 'transaction handles empty commit' ' > + cat >stdin <<-EOF && > + start > + prepare > + commit > + EOF > + git update-ref --stdin <stdin >actual && > + printf "%s: ok\n" start prepare commit >expect && > + test_cmp expect actual > +' > + > +test_expect_success 'transaction handles empty commit with missing prepare' ' > + cat >stdin <<-EOF && > + start > + commit > + EOF > + git update-ref --stdin <stdin >actual && > + printf "%s: ok\n" start commit >expect && > + test_cmp expect actual > +' > + > +test_expect_success 'transaction handles sole commit' ' > + cat >stdin <<-EOF && > + commit > + EOF > + git update-ref --stdin <stdin >actual && > + printf "%s: ok\n" commit >expect && > + test_cmp expect actual > +' > + > +test_expect_success 'transaction handles empty abort' ' > + cat >stdin <<-EOF && > + start > + prepare > + abort > + EOF > + git update-ref --stdin <stdin >actual && > + printf "%s: ok\n" start prepare abort >expect && > + test_cmp expect actual > +' > + > +test_expect_success 'transaction exits on multiple aborts' ' > + cat >stdin <<-EOF && > + abort > + abort > + EOF > + test_must_fail git update-ref --stdin <stdin >actual 2>err && > + printf "%s: ok\n" abort >expect && > + test_cmp expect actual && > + grep "fatal: transaction is closed" err > +' > + > +test_expect_success 'transaction exits on start after prepare' ' > + cat >stdin <<-EOF && > + prepare > + start > + EOF > + test_must_fail git update-ref --stdin <stdin 2>err >actual && > + printf "%s: ok\n" prepare >expect && > + test_cmp expect actual && > + grep "fatal: prepared transactions can only be closed" err > +' > + > +test_expect_success 'transaction handles empty abort with missing prepare' ' > + cat >stdin <<-EOF && > + start > + abort > + EOF > + git update-ref --stdin <stdin >actual && > + printf "%s: ok\n" start abort >expect && > + test_cmp expect actual > +' > + > +test_expect_success 'transaction handles sole abort' ' > + cat >stdin <<-EOF && > + abort > + EOF > + git update-ref --stdin <stdin >actual && > + printf "%s: ok\n" abort >expect && > + test_cmp expect actual > +' > + > +test_expect_success 'transaction can handle commit' ' > + cat >stdin <<-EOF && > + start > + create $a HEAD > + commit > + EOF > + git update-ref --stdin <stdin >actual && > + printf "%s: ok\n" start commit >expect && > + test_cmp expect actual && > + git rev-parse HEAD >expect && > + git rev-parse $a >actual && > + test_cmp expect actual > +' > + > +test_expect_success 'transaction can handle abort' ' > + cat >stdin <<-EOF && > + start > + create $b HEAD > + abort > + EOF > + git update-ref --stdin <stdin >actual && > + printf "%s: ok\n" start abort >expect && > + test_cmp expect actual && > + test_path_is_missing .git/$b > +' > + > +test_expect_success 'transaction aborts by default' ' > + cat >stdin <<-EOF && > + start > + create $b HEAD > + EOF > + git update-ref --stdin <stdin >actual && > + printf "%s: ok\n" start >expect && > + test_cmp expect actual && > + test_path_is_missing .git/$b > +' > + > +test_expect_success 'transaction with prepare aborts by default' ' > + cat >stdin <<-EOF && > + start > + create $b HEAD > + prepare > + EOF > + git update-ref --stdin <stdin >actual && > + printf "%s: ok\n" start prepare >expect && > + test_cmp expect actual && > + test_path_is_missing .git/$b > +' > + > test_done >
[-- Attachment #1: Type: text/plain, Size: 5285 bytes --] On Fri, Apr 03, 2020 at 02:40:26PM +0100, Phillip Wood wrote: > Hi Patrick > > On 02/04/2020 08:10, Patrick Steinhardt wrote: > > The git-update-ref(1) command can only handle queueing transactions > > right now via its "--stdin" parameter, but there is no way for users to > > handle the transaction itself in a more explicit way. E.g. in a > > replicated scenario, one may imagine a coordinator that spawns > > git-update-ref(1) for multiple repositories and only if all agree that > > an update is possible will the coordinator send a commit. Such a > > transactional session could look like > > > > > start > > < start: ok > > > update refs/heads/master $OLD $NEW > > > prepare > > < prepare: ok > > # All nodes have returned "ok" > > > commit > > < commit: ok > > > > or > > > > > start > > < start: ok > > > create refs/heads/master $OLD $NEW > > > prepare > > < fatal: cannot lock ref 'refs/heads/master': reference already exists > > # On all other nodes: > > > abort > > < abort: ok > > > > In order to allow for such transactional sessions, this commit > > introduces four new commands for git-update-ref(1), which matches those > > we have internally already with the exception of "start": > > > > - start: start a new transaction > > > > - prepare: prepare the transaction, that is try to lock all > > references and verify their current value matches the > > expected one > > > > - commit: explicitly commit a session, that is update references to > > match their new expected state > > > > - abort: abort a session and roll back all changes > > > > By design, git-update-ref(1) will commit as soon as standard input is > > being closed. While fine in a non-transactional world, it is definitely > > unexpected in a transactional world. Because of this, as soon as any of > > the new transactional commands is used, the default will change to > > aborting without an explicit "commit". To avoid a race between queueing > > updates and the first "prepare" that starts a transaction, the "start" > > command has been added to start an explicit transaction. > > > > Add some tests to exercise this new functionality. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > Documentation/git-update-ref.txt | 26 ++++++ > > builtin/update-ref.c | 106 +++++++++++++++++++++++-- > > t/t1400-update-ref.sh | 131 +++++++++++++++++++++++++++++++ > > 3 files changed, 255 insertions(+), 8 deletions(-) > > > > diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt > > index 9bd039ce08..3e737c2360 100644 > > --- a/Documentation/git-update-ref.txt > > +++ b/Documentation/git-update-ref.txt > > @@ -66,6 +66,10 @@ performs all modifications together. Specify commands of the form: > > delete SP <ref> [SP <oldvalue>] LF > > verify SP <ref> [SP <oldvalue>] LF > > option SP <opt> LF > > + start LF > > + prepare LF > > + commit LF > > + abort LF > > > > With `--create-reflog`, update-ref will create a reflog for each ref > > even if one would not ordinarily be created. > > @@ -83,6 +87,10 @@ quoting: > > delete SP <ref> NUL [<oldvalue>] NUL > > verify SP <ref> NUL [<oldvalue>] NUL > > option SP <opt> NUL > > + start NUL > > + prepare NUL > > + commit NUL > > + abort NUL > > > > In this format, use 40 "0" to specify a zero value, and use the empty > > string to specify a missing value. > > @@ -114,6 +122,24 @@ option:: > > The only valid option is `no-deref` to avoid dereferencing > > a symbolic ref. > > > > +start:: > > + Start a transaction. In contrast to a non-transactional session, > > I found the talk of "non-transactional" sessions a bit confusing because > the normal --stdin does update all the refs it is given as a single > transaction, so that if it cannot update one ref none of them are > updated. If I've understood correctly these changes are about > coordinating transactions across several repositories. I'm not sure how > best to convey that in the man page - perhaps we could call them single > repository transactions and multi repository transaction or something. The ultimate goal is to be able to create something that sits atop a set of repos that's able to coordinate multiple reference transactions at the same time and then do an all or nothing commit across all repos or abort in case any of the repos will not be able to perform the update. The proposed change is still about a single repository, only, and have the aim of actually enabling transaction semantics in the first place. Until now, it's only possible to start a transaction via git-update-refs(1), but you can't really control it except for the fnial commit. So with that being said I'm not quite sure whether it makes sense to document the potential for handling transactions across multiple repos. It does show that I could improve the documentation, though, and make the intent and scope clearer. I'll try to do that for the next version, thanks for your feedback! Patrick [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
Phillip Wood <phillip.wood123@gmail.com> writes:
> I found the talk of "non-transactional" sessions a bit confusing
> because the normal --stdin does update all the refs it is given as a
> single transaction, so that if it cannot update one ref none of them
> are updated. If I've understood correctly these changes are about
> coordinating transactions across several repositories. I'm not sure
> how best to convey that in the man page - perhaps we could call them
> single repository transactions and multi repository transaction or
> something.
FWIW, I described the topic in my "What's cooking" report as adding
an ingredient to implement two-phase commit-style atomic updates
across multiple repositories. These "prepare to commit, now if you
say 'yes I am ready', there is no taking it back", "now do the
commit as you promised", and "Earlier I made you promise that you'd
commit all when I tell you, but you no longer should commit" are
good enough ingredients, but I am not so sure about the actual
implementation in this topic (or it is really possible to make the
promise "ok, now I can commit everything without lock conflicts if
you tell me" with our ref API, which does not have "now freeze all
refs in the repository---from now on until I say otherwise, no refs
can be manipulated", in the first place).
Junio C Hamano <gitster@pobox.com> writes:
> ... but I am not so sure about the actual
> implementation in this topic ...
I take this part back---given that this is merely building on top of
the existing atomic update, we should be OK.
[-- Attachment #1: Type: text/plain, Size: 1164 bytes --] On Fri, Apr 03, 2020 at 10:33:58AM -0700, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > > > I found the talk of "non-transactional" sessions a bit confusing > > because the normal --stdin does update all the refs it is given as a > > single transaction, so that if it cannot update one ref none of them > > are updated. If I've understood correctly these changes are about > > coordinating transactions across several repositories. I'm not sure > > how best to convey that in the man page - perhaps we could call them > > single repository transactions and multi repository transaction or > > something. > > FWIW, I described the topic in my "What's cooking" report as adding > an ingredient to implement two-phase commit-style atomic updates > across multiple repositories. Fair enough. It definitely may serve as a building block for cross-repo transactions, which also is the original intent of this patch series. I'm not sure whether it makes sense to document it as such, as I can imagine usecases acting on a single repository that still want to be in the business of doing two-phase commits. Patrick [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]