{"thread":{"id":"53094","subject":"[PATCH 0/9] Support for transactions in `git-update-ref --stdin`","startedAt":"2020-03-25T09:53:28Z","lastAt":"2020-04-06T07:11:08Z","messageCount":48,"participants":["Patrick Steinhardt","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"393952","messageId":"cover.1585129842.git.ps@pks.im","threadId":"53094","inReplyTo":null,"subject":"[PATCH 0/9] Support for transactions in `git-update-ref --stdin`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-25T09:53:20Z","receivedAt":"2020-03-25T09:53:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\ninspired by recent discussions about handling transactions in\ngit-update-refs(1), this series implements proper transaction support in\n`git update-refs --stdin`. The goal is to have an all-or-nothing\ntransaction where a user can queue multiple updates, verify that they\ncan be committed, and then finally either commits or aborts the\ntransaction. A typical session would look like the following:\n\n # Start the transaction\n > start\n < start: ok\n # Queue updates\n > delete refs/heads/branch\n > create refs/heads/another $OID1\n # Prepare the transaction. git-update-ref will now try to allocate\n # all locks and verify that references are at their expected values.\n > prepare\n < prepare: ok\n # Commit the transaction. The user could also have said \"abort\" to\n # roll back everything.\n > commit\n < commit: ok\n\nThe series builds on the already existing transaction support in refs.c\nand exposes it to the user. The most important change that was required\nto support this was to convert `git-update-ref --stdin` to handle input\nlinewise instead of trying to read it in full and only acting after\nstdin was closed.\n\nThe series is structured as follows:\n\n Patches 1-2: Preparatory patches which make sense as standalone\n patches.\n\n Patches 3-7: Preparatory patches that make it easier to convert to\n reading commands in a line-wise fashion. No functional\n changes are expected.\n\n Patch 8: Conversion to read commands line-wise. No functional\n changes are expected, except that Git builds up the transaction\n while reading stdin instead of waiting for stdin to be closed\n first.\n\n Patch 9: Implementation of transactional commands.\n\nAll in all, the new transactional support will only be enabled if the\nuser invokes any the new commands \"start\", \"prepare\", \"commit\" or\n\"abort\". In case he doesn't, no functional changes are expected.\n\nPatrick\n\nPatrick Steinhardt (9):\n refs: fix segfault when aborting empty transaction\n git-update-ref.txt: add missing word\n strbuf: provide function to append whole lines\n update-ref: organize commands in an array\n update-ref: drop unused argument for `parse_refname`\n update-ref: pass end pointer instead of strbuf\n update-ref: move transaction handling into `update_refs_stdin()`\n update-ref: read commands in a line-wise fashion\n update-ref: implement interactive transaction handling\n\n Documentation/git-update-ref.txt | 28 +++-\n builtin/update-ref.c | 255 ++++++++++++++++++++++---------\n refs/files-backend.c | 20 +--\n strbuf.c | 10 ++\n strbuf.h | 6 +\n t/t1400-update-ref.sh | 131 ++++++++++++++++\n 6 files changed, 370 insertions(+), 80 deletions(-)\n\n-- \n2.26.0\n\n"},{"id":"393953","messageId":"3c7f8c47f361c46751629b44570657ef811d94d0.1585129842.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585129842.git.ps@pks.im","subject":"[PATCH 1/9] refs: fix segfault when aborting empty transaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-25T09:53:28Z","receivedAt":"2020-03-25T09:53:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When cleaning up a transaction that has no updates queued, then the\ntransaction's backend data will not have been allocated. We correctly\nhandle this for the packed backend, where the cleanup function checks\nwhether the backend data has been allocated at all -- if not, then there\nis nothing to clean up. For the files backend we do not check this and\nas a result will hit a segfault due to dereferencing a `NULL` pointer\nwhen cleaning up such a transaction.\n\nFix the issue by checking whether `backend_data` is set in the files\nbackend, too.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/files-backend.c | 20 +++++++++++---------\n 1 file changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 561c33ac8a..6516c7bc8c 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2565,17 +2565,19 @@ static void files_transaction_cleanup(struct files_ref_store *refs,\n \t\t}\n \t}\n \n-\tif (backend_data->packed_transaction &&\n-\t ref_transaction_abort(backend_data->packed_transaction, &err)) {\n-\t\terror(\"error aborting transaction: %s\", err.buf);\n-\t\tstrbuf_release(&err);\n+\tif (backend_data) {\n+\t\tif (backend_data->packed_transaction &&\n+\t\t ref_transaction_abort(backend_data->packed_transaction, &err)) {\n+\t\t\terror(\"error aborting transaction: %s\", err.buf);\n+\t\t\tstrbuf_release(&err);\n+\t\t}\n+\n+\t\tif (backend_data->packed_refs_locked)\n+\t\t\tpacked_refs_unlock(refs->packed_ref_store);\n+\n+\t\tfree(backend_data);\n \t}\n \n-\tif (backend_data->packed_refs_locked)\n-\t\tpacked_refs_unlock(refs->packed_ref_store);\n-\n-\tfree(backend_data);\n-\n \ttransaction->state = REF_TRANSACTION_CLOSED;\n }\n \n-- \n2.26.0\n\n"},{"id":"393954","messageId":"dd7798abb75c91a102022fb5389e2ef961629667.1585129842.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585129842.git.ps@pks.im","subject":"[PATCH 2/9] git-update-ref.txt: add missing word","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-25T09:53:38Z","receivedAt":"2020-03-25T09:53:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The description for the \"verify\" command is lacking a single word \"is\",\nwhich this commit corrects.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/git-update-ref.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt\nindex 9671423117..9bd039ce08 100644\n--- a/Documentation/git-update-ref.txt\n+++ b/Documentation/git-update-ref.txt\n@@ -107,7 +107,7 @@ delete::\n \n verify::\n \tVerify against but do not change it. If\n-\t zero or missing, the ref must not exist.\n+\t is zero or missing, the ref must not exist.\n \n option::\n \tModify behavior of the next command naming a .\n-- \n2.26.0\n\n"},{"id":"393955","messageId":"4b0a963ea5c47a951b95aa0a03966315b3e8299d.1585129842.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585129842.git.ps@pks.im","subject":"[PATCH 3/9] strbuf: provide function to append whole lines","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-25T09:53:47Z","receivedAt":"2020-03-25T09:53:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While the strbuf interface already provides functions to read a line\ninto it that completely replaces its current contents, we do not have an\ninterface that allows for appending lines without discarding current\ncontents.\n\nAdd a new function `strbuf_appendwholeline` that reads a line including\nits terminating character into a strbuf non-destructively. This is a\npreparatory step for git-update-ref(1) reading standard input line-wise\ninstead of as a block.\n\nSigned-off-by: Patrick Steinhardt \n---\n strbuf.c | 10 ++++++++++\n strbuf.h | 6 ++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex bb0065ccaf..6e74901bfa 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -690,6 +690,16 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n }\n #endif\n \n+int strbuf_appendwholeline(struct strbuf *sb, FILE *fp, int term)\n+{\n+\tstruct strbuf line = STRBUF_INIT;\n+\tif (strbuf_getwholeline(&line, fp, term))\n+\t\treturn EOF;\n+\tstrbuf_addbuf(sb, &line);\n+\tstrbuf_release(&line);\n+\treturn 0;\n+}\n+\n static int strbuf_getdelim(struct strbuf *sb, FILE *fp, int term)\n {\n \tif (strbuf_getwholeline(sb, fp, term))\ndiff --git a/strbuf.h b/strbuf.h\nindex ce8e49c0b2..411063ca76 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -502,6 +502,12 @@ int strbuf_getline(struct strbuf *sb, FILE *file);\n */\n int strbuf_getwholeline(struct strbuf *sb, FILE *file, int term);\n \n+/**\n+ * Like `strbuf_getwholeline`, but appends the line instead of\n+ * resetting the buffer first.\n+ */\n+int strbuf_appendwholeline(struct strbuf *sb, FILE *file, int term);\n+\n /**\n * Like `strbuf_getwholeline`, but operates on a file descriptor.\n * It reads one character at a time, so it is very slow. Do not\n-- \n2.26.0\n\n"},{"id":"393956","messageId":"50ffc263293571f8af71fd1d253ac238c6909229.1585129842.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585129842.git.ps@pks.im","subject":"[PATCH 4/9] update-ref: organize commands in an array","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-25T09:53:55Z","receivedAt":"2020-03-25T09:53:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We currently manually wire up all commands known to `git-update-ref\n--stdin`, making it harder than necessary to preprocess arguments after\nthe command is determined. To make this more extensible, let's refactor\nthe code to use an array of known commands instead. While this doesn't\nadd a lot of value now, it is a preparatory step to implement line-wise\nreading of commands.\n\nAs we're going to introduce commands without trailing spaces, this\ncommit also moves whitespace parsing into the respective commands.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/update-ref.c | 66 ++++++++++++++++++++++++++++++++------------\n 1 file changed, 49 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 2d8f7f0578..d2b917a47c 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -171,11 +171,11 @@ static int parse_next_oid(struct strbuf *input, const char **next,\n /*\n * The following five parse_cmd_*() functions parse the corresponding\n * command. In each case, next points at the character following the\n- * command name and the following space. They each return a pointer\n- * to the character terminating the command, and die with an\n- * explanatory message if there are any parsing problems. All of\n- * these functions handle either text or binary format input,\n- * depending on how line_termination is set.\n+ * command name. They each return a pointer to the character\n+ * terminating the command, and die with an explanatory message if\n+ * there are any parsing problems. All of these functions handle\n+ * either text or binary format input, depending on how\n+ * line_termination is set.\n */\n \n static const char *parse_cmd_update(struct ref_transaction *transaction,\n@@ -186,6 +186,9 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n \tstruct object_id new_oid, old_oid;\n \tint have_old;\n \n+\tif (!skip_prefix(next, \" \", &next))\n+\t\tdie(\"update: missing space after command\");\n+\n \trefname = parse_refname(input, &next);\n \tif (!refname)\n \t\tdie(\"update: missing \");\n@@ -220,6 +223,9 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,\n \tchar *refname;\n \tstruct object_id new_oid;\n \n+\tif (!skip_prefix(next, \" \", &next))\n+\t\tdie(\"create: missing space after command\");\n+\n \trefname = parse_refname(input, &next);\n \tif (!refname)\n \t\tdie(\"create: missing \");\n@@ -253,6 +259,9 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,\n \tstruct object_id old_oid;\n \tint have_old;\n \n+\tif (!skip_prefix(next, \" \", &next))\n+\t\tdie(\"delete: missing space after command\");\n+\n \trefname = parse_refname(input, &next);\n \tif (!refname)\n \t\tdie(\"delete: missing \");\n@@ -288,6 +297,9 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n \tchar *refname;\n \tstruct object_id old_oid;\n \n+\tif (!skip_prefix(next, \" \", &next))\n+\t\tdie(\"verify: missing space after command\");\n+\n \trefname = parse_refname(input, &next);\n \tif (!refname)\n \t\tdie(\"verify: missing \");\n@@ -310,9 +322,12 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n \treturn next;\n }\n \n-static const char *parse_cmd_option(struct strbuf *input, const char *next)\n+static const char *parse_cmd_option(struct ref_transaction *transaction,\n+\t\t\t\t struct strbuf *input, const char *next)\n {\n \tconst char *rest;\n+\tif (!skip_prefix(next, \" \", &next))\n+\t\tdie(\"option: missing space after command\");\n \tif (skip_prefix(next, \"no-deref\", &rest) && *rest == line_termination)\n \t\tupdate_flags |= REF_NO_DEREF;\n \telse\n@@ -320,33 +335,50 @@ static const char *parse_cmd_option(struct strbuf *input, const char *next)\n \treturn rest;\n }\n \n+static const struct parse_cmd {\n+\tconst char *prefix;\n+\tconst char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);\n+} commands[] = {\n+\t{ \"update\", parse_cmd_update },\n+\t{ \"create\", parse_cmd_create },\n+\t{ \"delete\", parse_cmd_delete },\n+\t{ \"verify\", parse_cmd_verify },\n+\t{ \"option\", parse_cmd_option },\n+};\n+\n static void update_refs_stdin(struct ref_transaction *transaction)\n {\n \tstruct strbuf input = STRBUF_INIT;\n \tconst char *next;\n+\tint i;\n \n \tif (strbuf_read(&input, 0, 1000) < 0)\n \t\tdie_errno(\"could not read from stdin\");\n \tnext = input.buf;\n \t/* Read each line dispatch its command */\n \twhile (next < input.buf + input.len) {\n+\t\tconst struct parse_cmd *cmd = NULL;\n+\n \t\tif (*next == line_termination)\n \t\t\tdie(\"empty command in input\");\n \t\telse if (isspace(*next))\n \t\t\tdie(\"whitespace before command: %s\", next);\n-\t\telse if (skip_prefix(next, \"update \", &next))\n-\t\t\tnext = parse_cmd_update(transaction, &input, next);\n-\t\telse if (skip_prefix(next, \"create \", &next))\n-\t\t\tnext = parse_cmd_create(transaction, &input, next);\n-\t\telse if (skip_prefix(next, \"delete \", &next))\n-\t\t\tnext = parse_cmd_delete(transaction, &input, next);\n-\t\telse if (skip_prefix(next, \"verify \", &next))\n-\t\t\tnext = parse_cmd_verify(transaction, &input, next);\n-\t\telse if (skip_prefix(next, \"option \", &next))\n-\t\t\tnext = parse_cmd_option(&input, next);\n-\t\telse\n+\n+\t\tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n+\t\t\tif (!skip_prefix(next, commands[i].prefix , &next))\n+\t\t\t\tcontinue;\n+\t\t\tcmd = &commands[i];\n+\t\t\tbreak;\n+\t\t}\n+\t\tif (!cmd)\n \t\t\tdie(\"unknown command: %s\", next);\n \n+\t\tif (input.buf[strlen(cmd->prefix)] != line_termination &&\n+\t\t input.buf[strlen(cmd->prefix)] != '\\0' &&\n+\t\t input.buf[strlen(cmd->prefix)] != ' ')\n+\t\t\tdie(\"%s: no separator after command\", cmd->prefix);\n+\n+\t\tnext = cmd->fn(transaction, &input, next);\n \t\tnext++;\n \t}\n \n-- \n2.26.0\n\n"},{"id":"393958","messageId":"a78043b188eb75652b84988c626ce14136c80433.1585129842.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585129842.git.ps@pks.im","subject":"[PATCH 5/9] update-ref: drop unused argument for `parse_refname`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-25T09:54:01Z","receivedAt":"2020-03-25T09:54:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `parse_refname` function accepts a `struct strbuf *input` argument\nthat isn't used at all. As we're about to convert commands to not use a\nstrbuf anymore but instead an end pointer, let's drop this argument now\nto make the converting commit easier to review.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/update-ref.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex d2b917a47c..6f4e8e88c1 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -50,7 +50,7 @@ static const char *parse_arg(const char *next, struct strbuf *arg)\n * the argument. Die if C-quoting is malformed or the reference name\n * is invalid.\n */\n-static char *parse_refname(struct strbuf *input, const char **next)\n+static char *parse_refname(const char **next)\n {\n \tstruct strbuf ref = STRBUF_INIT;\n \n@@ -189,7 +189,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n \tif (!skip_prefix(next, \" \", &next))\n \t\tdie(\"update: missing space after command\");\n \n-\trefname = parse_refname(input, &next);\n+\trefname = parse_refname(&next);\n \tif (!refname)\n \t\tdie(\"update: missing \");\n \n@@ -226,7 +226,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,\n \tif (!skip_prefix(next, \" \", &next))\n \t\tdie(\"create: missing space after command\");\n \n-\trefname = parse_refname(input, &next);\n+\trefname = parse_refname(&next);\n \tif (!refname)\n \t\tdie(\"create: missing \");\n \n@@ -262,7 +262,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,\n \tif (!skip_prefix(next, \" \", &next))\n \t\tdie(\"delete: missing space after command\");\n \n-\trefname = parse_refname(input, &next);\n+\trefname = parse_refname(&next);\n \tif (!refname)\n \t\tdie(\"delete: missing \");\n \n@@ -300,7 +300,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n \tif (!skip_prefix(next, \" \", &next))\n \t\tdie(\"verify: missing space after command\");\n \n-\trefname = parse_refname(input, &next);\n+\trefname = parse_refname(&next);\n \tif (!refname)\n \t\tdie(\"verify: missing \");\n \n-- \n2.26.0\n\n"},{"id":"393959","messageId":"51ebb2f8496641380aed4810fa0bb7ebc3bc0a99.1585129842.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585129842.git.ps@pks.im","subject":"[PATCH 6/9] update-ref: pass end pointer instead of strbuf","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-25T09:54:09Z","receivedAt":"2020-03-25T09:54:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We currently pass both an `strbuf` containing the current command line\nas well as the `next` pointer pointing to the first argument to\ncommands. This is both confusing and makes code more intertwined.\nConvert this to use a simple pointer as well as a pointer pointing to\nthe end of the input as a preparatory step to line-wise reading of\nstdin.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/update-ref.c | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 6f4e8e88c1..6f036ae765 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -95,7 +95,7 @@ static char *parse_refname(const char **next)\n * provided but cannot be converted to a SHA-1, die. flags can\n * include PARSE_SHA1_OLD and/or PARSE_SHA1_ALLOW_EMPTY.\n */\n-static int parse_next_oid(struct strbuf *input, const char **next,\n+static int parse_next_oid(const char **next, const char *end,\n \t\t\t struct object_id *oid,\n \t\t\t const char *command, const char *refname,\n \t\t\t int flags)\n@@ -103,7 +103,7 @@ static int parse_next_oid(struct strbuf *input, const char **next,\n \tstruct strbuf arg = STRBUF_INIT;\n \tint ret = 0;\n \n-\tif (*next == input->buf + input->len)\n+\tif (*next == end)\n \t\tgoto eof;\n \n \tif (line_termination) {\n@@ -128,7 +128,7 @@ static int parse_next_oid(struct strbuf *input, const char **next,\n \t\t\tdie(\"%s %s: expected NUL but got: %s\",\n \t\t\t command, refname, *next);\n \t\t(*next)++;\n-\t\tif (*next == input->buf + input->len)\n+\t\tif (*next == end)\n \t\t\tgoto eof;\n \t\tstrbuf_addstr(&arg, *next);\n \t\t*next += arg.len;\n@@ -179,7 +179,7 @@ static int parse_next_oid(struct strbuf *input, const char **next,\n */\n \n static const char *parse_cmd_update(struct ref_transaction *transaction,\n-\t\t\t\t struct strbuf *input, const char *next)\n+\t\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -193,11 +193,11 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n \tif (!refname)\n \t\tdie(\"update: missing \");\n \n-\tif (parse_next_oid(input, &next, &new_oid, \"update\", refname,\n+\tif (parse_next_oid(&next, end, &new_oid, \"update\", refname,\n \t\t\t PARSE_SHA1_ALLOW_EMPTY))\n \t\tdie(\"update %s: missing \", refname);\n \n-\thave_old = !parse_next_oid(input, &next, &old_oid, \"update\", refname,\n+\thave_old = !parse_next_oid(&next, end, &old_oid, \"update\", refname,\n \t\t\t\t PARSE_SHA1_OLD);\n \n \tif (*next != line_termination)\n@@ -217,7 +217,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n }\n \n static const char *parse_cmd_create(struct ref_transaction *transaction,\n-\t\t\t\t struct strbuf *input, const char *next)\n+\t\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -230,7 +230,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,\n \tif (!refname)\n \t\tdie(\"create: missing \");\n \n-\tif (parse_next_oid(input, &next, &new_oid, \"create\", refname, 0))\n+\tif (parse_next_oid(&next, end, &new_oid, \"create\", refname, 0))\n \t\tdie(\"create %s: missing \", refname);\n \n \tif (is_null_oid(&new_oid))\n@@ -252,7 +252,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,\n }\n \n static const char *parse_cmd_delete(struct ref_transaction *transaction,\n-\t\t\t\t struct strbuf *input, const char *next)\n+\t\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -266,7 +266,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,\n \tif (!refname)\n \t\tdie(\"delete: missing \");\n \n-\tif (parse_next_oid(input, &next, &old_oid, \"delete\", refname,\n+\tif (parse_next_oid(&next, end, &old_oid, \"delete\", refname,\n \t\t\t PARSE_SHA1_OLD)) {\n \t\thave_old = 0;\n \t} else {\n@@ -291,7 +291,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,\n }\n \n static const char *parse_cmd_verify(struct ref_transaction *transaction,\n-\t\t\t\t struct strbuf *input, const char *next)\n+\t\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -304,7 +304,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n \tif (!refname)\n \t\tdie(\"verify: missing \");\n \n-\tif (parse_next_oid(input, &next, &old_oid, \"verify\", refname,\n+\tif (parse_next_oid(&next, end, &old_oid, \"verify\", refname,\n \t\t\t PARSE_SHA1_OLD))\n \t\toidclr(&old_oid);\n \n@@ -323,7 +323,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n }\n \n static const char *parse_cmd_option(struct ref_transaction *transaction,\n-\t\t\t\t struct strbuf *input, const char *next)\n+\t\t\t\t const char *next, const char *end)\n {\n \tconst char *rest;\n \tif (!skip_prefix(next, \" \", &next))\n@@ -337,7 +337,7 @@ static const char *parse_cmd_option(struct ref_transaction *transaction,\n \n static const struct parse_cmd {\n \tconst char *prefix;\n-\tconst char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);\n+\tconst char *(*fn)(struct ref_transaction *, const char *, const char *);\n } commands[] = {\n \t{ \"update\", parse_cmd_update },\n \t{ \"create\", parse_cmd_create },\n@@ -378,7 +378,7 @@ static void update_refs_stdin(struct ref_transaction *transaction)\n \t\t input.buf[strlen(cmd->prefix)] != ' ')\n \t\t\tdie(\"%s: no separator after command\", cmd->prefix);\n \n-\t\tnext = cmd->fn(transaction, &input, next);\n+\t\tnext = cmd->fn(transaction, next, input.buf + input.len);\n \t\tnext++;\n \t}\n \n-- \n2.26.0\n\n"},{"id":"393960","messageId":"bd92a649d79cde43260358ca939baee0f1a2f076.1585129842.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585129842.git.ps@pks.im","subject":"[PATCH 7/9] update-ref: move transaction handling into `update_refs_stdin()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-25T09:54:16Z","receivedAt":"2020-03-25T09:54:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While the actual logic to update the transaction is handled in\n`update_refs_stdin()`, the transaction itself is started and committed\nin `cmd_update_ref()` itself. This makes it hard to handle transaction\nabortion and commits as part of `update_refs_stdin()` itself, which is\nrequired in order to introduce transaction handling features to `git\nupdate-refs --stdin`.\n\nRefactor the code to move all transaction handling into\n`update_refs_stdin()` to prepare for transaction handling features.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/update-ref.c | 27 ++++++++++++++-------------\n 1 file changed, 14 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 6f036ae765..5f97bb0b75 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -346,15 +346,21 @@ static const struct parse_cmd {\n \t{ \"option\", parse_cmd_option },\n };\n \n-static void update_refs_stdin(struct ref_transaction *transaction)\n+static void update_refs_stdin(void)\n {\n-\tstruct strbuf input = STRBUF_INIT;\n+\tstruct strbuf input = STRBUF_INIT, err = STRBUF_INIT;\n+\tstruct ref_transaction *transaction;\n \tconst char *next;\n \tint i;\n \n \tif (strbuf_read(&input, 0, 1000) < 0)\n \t\tdie_errno(\"could not read from stdin\");\n \tnext = input.buf;\n+\n+\ttransaction = ref_transaction_begin(&err);\n+\tif (!transaction)\n+\t\tdie(\"%s\", err.buf);\n+\n \t/* Read each line dispatch its command */\n \twhile (next < input.buf + input.len) {\n \t\tconst struct parse_cmd *cmd = NULL;\n@@ -382,6 +388,11 @@ static void update_refs_stdin(struct ref_transaction *transaction)\n \t\tnext++;\n \t}\n \n+\tif (ref_transaction_commit(transaction, &err))\n+\t\tdie(\"%s\", err.buf);\n+\n+\tref_transaction_free(transaction);\n+\tstrbuf_release(&err);\n \tstrbuf_release(&input);\n }\n \n@@ -416,21 +427,11 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (read_stdin) {\n-\t\tstruct strbuf err = STRBUF_INIT;\n-\t\tstruct ref_transaction *transaction;\n-\n-\t\ttransaction = ref_transaction_begin(&err);\n-\t\tif (!transaction)\n-\t\t\tdie(\"%s\", err.buf);\n \t\tif (delete || argc > 0)\n \t\t\tusage_with_options(git_update_ref_usage, options);\n \t\tif (end_null)\n \t\t\tline_termination = '\\0';\n-\t\tupdate_refs_stdin(transaction);\n-\t\tif (ref_transaction_commit(transaction, &err))\n-\t\t\tdie(\"%s\", err.buf);\n-\t\tref_transaction_free(transaction);\n-\t\tstrbuf_release(&err);\n+\t\tupdate_refs_stdin();\n \t\treturn 0;\n \t}\n \n-- \n2.26.0\n\n"},{"id":"393961","messageId":"1db63f97ec78fad794cec51c5d96b093f7cd2440.1585129843.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585129842.git.ps@pks.im","subject":"[PATCH 8/9] update-ref: read commands in a line-wise fashion","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-25T09:54:22Z","receivedAt":"2020-03-25T09:54:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The git-update-ref(1) supports a `--stdin` mode that allows it to read\nall reference updates from standard input. This is mainly used to allow\nfor atomic reference updates that are all or nothing, so that either all\nreferences will get updated or none.\n\nCurrently, git-update-ref(1) reads all commands as a single block of up\nto 1000 characters and only starts processing after stdin gets closed.\nThis is less flexible than one might wish for, as it doesn't really\nallow for longer-lived transactions and doesn't allow any verification\nwithout committing everything. E.g. one may imagine the following\nexchange:\n\n > start\n < start: ok\n > update refs/heads/master $NEWOID1 $OLDOID1\n > update refs/heads/branch $NEWOID2 $OLDOID2\n > prepare\n < prepare: ok\n > commit\n < commit: ok\n\nWhen reading all input as a whole block, the above interactive protocol\nis obviously impossible to achieve. But by converting the command to\nread commands linewise, we can make it more interactive than before.\n\nObviously, the linewise interface is only a first step in making\ngit-update-ref(1) work in a more transaction-oriented way. Missing is\nmost importantly support for transactional commands that manage the\ncurrent transaction.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/update-ref.c | 70 ++++++++++++++++++++------------------------\n 1 file changed, 31 insertions(+), 39 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 5f97bb0b75..f35e3ca5ae 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -178,8 +178,8 @@ static int parse_next_oid(const char **next, const char *end,\n * line_termination is set.\n */\n \n-static const char *parse_cmd_update(struct ref_transaction *transaction,\n-\t\t\t\t const char *next, const char *end)\n+static void parse_cmd_update(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -212,12 +212,10 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n \tupdate_flags = default_flags;\n \tfree(refname);\n \tstrbuf_release(&err);\n-\n-\treturn next;\n }\n \n-static const char *parse_cmd_create(struct ref_transaction *transaction,\n-\t\t\t\t const char *next, const char *end)\n+static void parse_cmd_create(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -247,12 +245,10 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,\n \tupdate_flags = default_flags;\n \tfree(refname);\n \tstrbuf_release(&err);\n-\n-\treturn next;\n }\n \n-static const char *parse_cmd_delete(struct ref_transaction *transaction,\n-\t\t\t\t const char *next, const char *end)\n+static void parse_cmd_delete(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -286,12 +282,10 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,\n \tupdate_flags = default_flags;\n \tfree(refname);\n \tstrbuf_release(&err);\n-\n-\treturn next;\n }\n \n-static const char *parse_cmd_verify(struct ref_transaction *transaction,\n-\t\t\t\t const char *next, const char *end)\n+static void parse_cmd_verify(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -318,12 +312,10 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n \tupdate_flags = default_flags;\n \tfree(refname);\n \tstrbuf_release(&err);\n-\n-\treturn next;\n }\n \n-static const char *parse_cmd_option(struct ref_transaction *transaction,\n-\t\t\t\t const char *next, const char *end)\n+static void parse_cmd_option(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n {\n \tconst char *rest;\n \tif (!skip_prefix(next, \" \", &next))\n@@ -332,60 +324,60 @@ static const char *parse_cmd_option(struct ref_transaction *transaction,\n \t\tupdate_flags |= REF_NO_DEREF;\n \telse\n \t\tdie(\"option unknown: %s\", next);\n-\treturn rest;\n }\n \n static const struct parse_cmd {\n \tconst char *prefix;\n-\tconst char *(*fn)(struct ref_transaction *, const char *, const char *);\n+\tvoid (*fn)(struct ref_transaction *, const char *, const char *);\n+\tunsigned extra_lines;\n } commands[] = {\n-\t{ \"update\", parse_cmd_update },\n-\t{ \"create\", parse_cmd_create },\n-\t{ \"delete\", parse_cmd_delete },\n-\t{ \"verify\", parse_cmd_verify },\n-\t{ \"option\", parse_cmd_option },\n+\t{ \"update\", parse_cmd_update, 2 },\n+\t{ \"create\", parse_cmd_create, 1 },\n+\t{ \"delete\", parse_cmd_delete, 1 },\n+\t{ \"verify\", parse_cmd_verify, 1 },\n+\t{ \"option\", parse_cmd_option, 0 },\n };\n \n static void update_refs_stdin(void)\n {\n \tstruct strbuf input = STRBUF_INIT, err = STRBUF_INIT;\n \tstruct ref_transaction *transaction;\n-\tconst char *next;\n-\tint i;\n-\n-\tif (strbuf_read(&input, 0, 1000) < 0)\n-\t\tdie_errno(\"could not read from stdin\");\n-\tnext = input.buf;\n+\tint i, j;\n \n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction)\n \t\tdie(\"%s\", err.buf);\n \n \t/* Read each line dispatch its command */\n-\twhile (next < input.buf + input.len) {\n+\twhile (!strbuf_getwholeline(&input, stdin, line_termination)) {\n \t\tconst struct parse_cmd *cmd = NULL;\n \n-\t\tif (*next == line_termination)\n+\t\tif (*input.buf == line_termination)\n \t\t\tdie(\"empty command in input\");\n-\t\telse if (isspace(*next))\n-\t\t\tdie(\"whitespace before command: %s\", next);\n+\t\telse if (isspace(*input.buf))\n+\t\t\tdie(\"whitespace before command: %s\", input.buf);\n \n \t\tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n-\t\t\tif (!skip_prefix(next, commands[i].prefix , &next))\n+\t\t\tif (!starts_with(input.buf, commands[i].prefix))\n \t\t\t\tcontinue;\n \t\t\tcmd = &commands[i];\n \t\t\tbreak;\n \t\t}\n \t\tif (!cmd)\n-\t\t\tdie(\"unknown command: %s\", next);\n+\t\t\tdie(\"unknown command: %s\", input.buf);\n \n \t\tif (input.buf[strlen(cmd->prefix)] != line_termination &&\n \t\t input.buf[strlen(cmd->prefix)] != '\\0' &&\n \t\t input.buf[strlen(cmd->prefix)] != ' ')\n \t\t\tdie(\"%s: no separator after command\", cmd->prefix);\n \n-\t\tnext = cmd->fn(transaction, next, input.buf + input.len);\n-\t\tnext++;\n+\t\t/* Read extra lines if NUL-terminated */\n+\t\tfor (j = 0; line_termination == '\\0' && j < cmd->extra_lines; j++)\n+\t\t\tif (strbuf_appendwholeline(&input, stdin, line_termination))\n+\t\t\t\tbreak;\n+\n+\t\tcmd->fn(transaction, input.buf + strlen(cmd->prefix),\n+\t\t\tinput.buf + input.len);\n \t}\n \n \tif (ref_transaction_commit(transaction, &err))\n-- \n2.26.0\n\n"},{"id":"393962","messageId":"88c0089bb50bca13efc79a1f8fd42b80286af949.1585129843.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585129842.git.ps@pks.im","subject":"[PATCH 9/9] update-ref: implement interactive transaction handling","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-25T09:54:29Z","receivedAt":"2020-03-25T09:54:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The git-update-ref(1) command can only handle queueing transactions\nright now via its \"--stdin\" parameter, but there is no way for users to\nhandle the transaction itself in a more explicit way. E.g. in a\nreplicated scenario, one may imagine a coordinator that spawns\ngit-update-ref(1) for multiple repositories and only if all agree that\nan update is possible will the coordinator send a commit. Such a\ntransactional session could look like\n\n > start\n < start: ok\n > update refs/heads/master $OLD $NEW\n > prepare\n < prepare: ok\n # All nodes have returned \"ok\"\n > commit\n < commit: ok\n\nor\n\n > start\n < start: ok\n > create refs/heads/master $OLD $NEW\n > prepare\n < fatal: cannot lock ref 'refs/heads/master': reference already exists\n # On all other nodes:\n > abort\n < abort: ok\n\nIn order to allow for such transactional sessions, this commit\nintroduces four new commands for git-update-ref(1), which matches those\nwe have internally already with the exception of \"start\":\n\n - start: start a new transaction\n\n - prepare: prepare the transaction, that is try to lock all\n references and verify their current value matches the\n expected one\n\n - commit: explicitly commit a session, that is update references to\n match their new expected state\n\n - abort: abort a session and roll back all changes\n\nBy design, git-update-ref(1) will commit as soon as standard input is\nbeing closed. While fine in a non-transactional world, it is definitely\nunexpected in a transactional world. Because of this, as soon as any of\nthe new transactional commands is used, the default will change to\naborting without an explicit \"commit\". To avoid a race between queueing\nupdates and the first \"prepare\" that starts a transaction, the \"start\"\ncommand has been added to start an explicit transaction.\n\nAdd some tests to exercise this new functionality.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/git-update-ref.txt | 26 ++++++\n builtin/update-ref.c | 108 ++++++++++++++++++++++---\n t/t1400-update-ref.sh | 131 +++++++++++++++++++++++++++++++\n 3 files changed, 256 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt\nindex 9bd039ce08..3e737c2360 100644\n--- a/Documentation/git-update-ref.txt\n+++ b/Documentation/git-update-ref.txt\n@@ -66,6 +66,10 @@ performs all modifications together. Specify commands of the form:\n \tdelete SP [SP ] LF\n \tverify SP [SP ] LF\n \toption SP LF\n+\tstart LF\n+\tprepare LF\n+\tcommit LF\n+\tabort LF\n \n With `--create-reflog`, update-ref will create a reflog for each ref\n even if one would not ordinarily be created.\n@@ -83,6 +87,10 @@ quoting:\n \tdelete SP NUL [] NUL\n \tverify SP NUL [] NUL\n \toption SP NUL\n+\tstart NUL\n+\tprepare NUL\n+\tcommit NUL\n+\tabort NUL\n \n In this format, use 40 \"0\" to specify a zero value, and use the empty\n string to specify a missing value.\n@@ -114,6 +122,24 @@ option::\n \tThe only valid option is `no-deref` to avoid dereferencing\n \ta symbolic ref.\n \n+start::\n+\tStart a transaction. In contrast to a non-transactional session, a\n+\ttransaction will automatically abort if the session ends without an\n+\texplicit commit.\n+\n+prepare::\n+\tPrepare to commit the transaction. This will create lock files for all\n+\tqueued reference updates. If one reference could not be locked, the\n+\ttransaction will be aborted.\n+\n+commit::\n+\tCommit all reference updates queued for the transaction, ending the\n+\ttransaction.\n+\n+abort::\n+\tAbort the transaction, releasing all locks if the transaction is in\n+\tprepared state.\n+\n If all s can be locked with matching s\n simultaneously, all modifications are performed. Otherwise, no\n modifications are performed. Note that while each individual\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex f35e3ca5ae..6b870507e0 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -326,21 +326,80 @@ static void parse_cmd_option(struct ref_transaction *transaction,\n \t\tdie(\"option unknown: %s\", next);\n }\n \n+static void parse_cmd_start(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n+{\n+\tif (*next != line_termination)\n+\t\tdie(\"start: extra input: %s\", next);\n+\tputs(\"start: ok\");\n+}\n+\n+static void parse_cmd_prepare(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n+{\n+\tstruct strbuf error = STRBUF_INIT;\n+\tif (*next != line_termination)\n+\t\tdie(\"prepare: extra input: %s\", next);\n+\tif (ref_transaction_prepare(transaction, &error))\n+\t\tdie(\"prepare: %s\", error.buf);\n+\tputs(\"prepare: ok\");\n+}\n+\n+static void parse_cmd_abort(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n+{\n+\tstruct strbuf error = STRBUF_INIT;\n+\tif (*next != line_termination)\n+\t\tdie(\"abort: extra input: %s\", next);\n+\tif (ref_transaction_abort(transaction, &error))\n+\t\tdie(\"abort: %s\", error.buf);\n+\tputs(\"abort: ok\");\n+}\n+\n+static void parse_cmd_commit(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n+{\n+\tstruct strbuf error = STRBUF_INIT;\n+\tif (*next != line_termination)\n+\t\tdie(\"commit: extra input: %s\", next);\n+\tif (ref_transaction_commit(transaction, &error))\n+\t\tdie(\"commit: %s\", error.buf);\n+\tputs(\"commit: ok\");\n+\tref_transaction_free(transaction);\n+}\n+\n+enum update_refs_state {\n+\t/* Non-transactional state open for updates. */\n+\tUPDATE_REFS_OPEN,\n+\t/* A transaction has been started. */\n+\tUPDATE_REFS_STARTED,\n+\t/* References are locked and ready for commit */\n+\tUPDATE_REFS_PREPARED,\n+\t/* Transaction has been committed or closed. */\n+\tUPDATE_REFS_CLOSED,\n+};\n+\n static const struct parse_cmd {\n \tconst char *prefix;\n \tvoid (*fn)(struct ref_transaction *, const char *, const char *);\n \tunsigned extra_lines;\n+\tenum update_refs_state state;\n } commands[] = {\n-\t{ \"update\", parse_cmd_update, 2 },\n-\t{ \"create\", parse_cmd_create, 1 },\n-\t{ \"delete\", parse_cmd_delete, 1 },\n-\t{ \"verify\", parse_cmd_verify, 1 },\n-\t{ \"option\", parse_cmd_option, 0 },\n+\t{ \"update\", parse_cmd_update, 2, UPDATE_REFS_OPEN },\n+\t{ \"create\", parse_cmd_create, 1, UPDATE_REFS_OPEN },\n+\t{ \"delete\", parse_cmd_delete, 1, UPDATE_REFS_OPEN },\n+\t{ \"verify\", parse_cmd_verify, 1, UPDATE_REFS_OPEN },\n+\t{ \"option\", parse_cmd_option, 0, UPDATE_REFS_OPEN },\n+\t{ \"start\", parse_cmd_start, 0, UPDATE_REFS_STARTED },\n+\t{ \"prepare\", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED },\n+\t{ \"abort\", parse_cmd_abort, 0, UPDATE_REFS_CLOSED },\n+\t{ \"commit\", parse_cmd_commit, 0, UPDATE_REFS_CLOSED },\n };\n \n static void update_refs_stdin(void)\n {\n \tstruct strbuf input = STRBUF_INIT, err = STRBUF_INIT;\n+\tenum update_refs_state state = UPDATE_REFS_OPEN;\n \tstruct ref_transaction *transaction;\n \tint i, j;\n \n@@ -371,19 +430,50 @@ static void update_refs_stdin(void)\n \t\t input.buf[strlen(cmd->prefix)] != ' ')\n \t\t\tdie(\"%s: no separator after command\", cmd->prefix);\n \n-\t\t/* Read extra lines if NUL-terminated */\n+\t\t/* Read extra lines if NUL-terminated, but let commands handle missing ones. */\n \t\tfor (j = 0; line_termination == '\\0' && j < cmd->extra_lines; j++)\n \t\t\tif (strbuf_appendwholeline(&input, stdin, line_termination))\n \t\t\t\tbreak;\n \n+\t\tswitch (state) {\n+\t\tcase UPDATE_REFS_OPEN:\n+\t\tcase UPDATE_REFS_STARTED:\n+\t\t\t/* Do not downgrade a transaction to a non-transaction. */\n+\t\t\tif (cmd->state >= state)\n+\t\t\t\tstate = cmd->state;\n+\t\t\tbreak;\n+\t\tcase UPDATE_REFS_PREPARED:\n+\t\t\tif (cmd->state != UPDATE_REFS_CLOSED)\n+\t\t\t\tdie(\"prepared transactions can only be closed\");\n+\t\t\tstate = cmd->state;\n+\t\t\tbreak;\n+\t\tcase UPDATE_REFS_CLOSED:\n+\t\t\tdie(\"transaction is closed\");\n+\t\t\tbreak;\n+\t\t}\n+\n \t\tcmd->fn(transaction, input.buf + strlen(cmd->prefix),\n \t\t\tinput.buf + input.len);\n \t}\n \n-\tif (ref_transaction_commit(transaction, &err))\n-\t\tdie(\"%s\", err.buf);\n+\tswitch (state) {\n+\tcase UPDATE_REFS_OPEN:\n+\t\t/* Commit by default if no transaction was requested. */\n+\t\tif (ref_transaction_commit(transaction, &err))\n+\t\t\tdie(\"%s\", err.buf);\n+\t\tref_transaction_free(transaction);\n+\t\tbreak;\n+\tcase UPDATE_REFS_STARTED:\n+\tcase UPDATE_REFS_PREPARED:\n+\t\t/* If using a transaction, we want to abort it. */\n+\t\tif (ref_transaction_abort(transaction, &err))\n+\t\t\tdie(\"%s\", err.buf);\n+\t\tbreak;\n+\tcase UPDATE_REFS_CLOSED:\n+\t\t/* Otherwise no need to do anything, the transaction was closed already. */\n+\t\tbreak;\n+\t}\n \n-\tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n \tstrbuf_release(&input);\n }\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex a6224ef65f..48d0d42afd 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -1404,4 +1404,135 @@ test_expect_success 'handle per-worktree refs in refs/bisect' '\n \t! test_cmp main-head worktree-head\n '\n \n+test_expect_success 'transaction handles empty commit' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tprepare\n+\tcommit\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start prepare commit >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction handles empty commit with missing prepare' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tcommit\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start commit >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction handles sole commit' '\n+\tcat >stdin <<-EOF &&\n+\tcommit\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" commit >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction handles empty abort' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tprepare\n+\tabort\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start prepare abort >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction exits on multiple aborts' '\n+\tcat >stdin <<-EOF &&\n+\tabort\n+\tabort\n+\tEOF\n+\ttest_must_fail git update-ref --stdin actual 2>err &&\n+\tprintf \"%s: ok\\n\" abort >expect &&\n+\ttest_cmp expect actual &&\n+\tgrep \"fatal: transaction is closed\" err\n+'\n+\n+test_expect_success 'transaction exits on start after prepare' '\n+\tcat >stdin <<-EOF &&\n+\tprepare\n+\tstart\n+\tEOF\n+\ttest_must_fail git update-ref --stdin err >actual &&\n+\tprintf \"%s: ok\\n\" prepare >expect &&\n+\ttest_cmp expect actual &&\n+\tgrep \"fatal: prepared transactions can only be closed\" err\n+'\n+\n+test_expect_success 'transaction handles empty abort with missing prepare' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tabort\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start abort >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction handles sole abort' '\n+\tcat >stdin <<-EOF &&\n+\tabort\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" abort >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction can handle commit' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tcreate $a HEAD\n+\tcommit\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start commit >expect &&\n+\ttest_cmp expect actual &&\n+\tgit rev-parse HEAD >expect &&\n+\tgit rev-parse $a >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction can handle abort' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tcreate $b HEAD\n+\tabort\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start abort >expect &&\n+\ttest_cmp expect actual &&\n+\ttest_path_is_missing .git/$b\n+'\n+\n+test_expect_success 'transaction aborts by default' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tcreate $b HEAD\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start >expect &&\n+\ttest_cmp expect actual &&\n+\ttest_path_is_missing .git/$b\n+'\n+\n+test_expect_success 'transaction with prepare aborts by default' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tcreate $b HEAD\n+\tprepare\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start prepare >expect &&\n+\ttest_cmp expect actual &&\n+\ttest_path_is_missing .git/$b\n+'\n+\n test_done\n-- \n2.26.0\n\n"},{"id":"394192","messageId":"xmqqwo75fu1u.fsf@gitster.c.googlers.com","threadId":"53094","inReplyTo":"3c7f8c47f361c46751629b44570657ef811d94d0.1585129842.git.ps@pks.im","subject":"Re: [PATCH 1/9] refs: fix segfault when aborting empty transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-27T19:59:25Z","receivedAt":"2020-03-27T19:59:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> When cleaning up a transaction that has no updates queued, then the\n> transaction's backend data will not have been allocated. We correctly\n> handle this for the packed backend, where the cleanup function checks\n> whether the backend data has been allocated at all -- if not, then there\n> is nothing to clean up. For the files backend we do not check this and\n> as a result will hit a segfault due to dereferencing a `NULL` pointer\n> when cleaning up such a transaction.\n>\n> Fix the issue by checking whether `backend_data` is set in the files\n> backend, too.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> refs/files-backend.c | 20 +++++++++++---------\n> 1 file changed, 11 insertions(+), 9 deletions(-)\n\nMakes sense. Will queue. Thanks.\n\n>\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 561c33ac8a..6516c7bc8c 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2565,17 +2565,19 @@ static void files_transaction_cleanup(struct files_ref_store *refs,\n> \t\t}\n> \t}\n> \n> -\tif (backend_data->packed_transaction &&\n> -\t ref_transaction_abort(backend_data->packed_transaction, &err)) {\n> -\t\terror(\"error aborting transaction: %s\", err.buf);\n> -\t\tstrbuf_release(&err);\n> +\tif (backend_data) {\n> +\t\tif (backend_data->packed_transaction &&\n> +\t\t ref_transaction_abort(backend_data->packed_transaction, &err)) {\n> +\t\t\terror(\"error aborting transaction: %s\", err.buf);\n> +\t\t\tstrbuf_release(&err);\n> +\t\t}\n> +\n> +\t\tif (backend_data->packed_refs_locked)\n> +\t\t\tpacked_refs_unlock(refs->packed_ref_store);\n> +\n> +\t\tfree(backend_data);\n> \t}\n> \n> -\tif (backend_data->packed_refs_locked)\n> -\t\tpacked_refs_unlock(refs->packed_ref_store);\n> -\n> -\tfree(backend_data);\n> -\n> \ttransaction->state = REF_TRANSACTION_CLOSED;\n> }\n"},{"id":"394196","messageId":"xmqqpncxfr1p.fsf@gitster.c.googlers.com","threadId":"53094","inReplyTo":"4b0a963ea5c47a951b95aa0a03966315b3e8299d.1585129842.git.ps@pks.im","subject":"Re: [PATCH 3/9] strbuf: provide function to append whole lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-27T21:04:18Z","receivedAt":"2020-03-27T21:04:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> While the strbuf interface already provides functions to read a line\n> into it that completely replaces its current contents, we do not have an\n> interface that allows for appending lines without discarding current\n> contents.\n>\n> Add a new function `strbuf_appendwholeline` that reads a line including\n> its terminating character into a strbuf non-destructively. This is a\n> preparatory step for git-update-ref(1) reading standard input line-wise\n> instead of as a block.\n\nHmph. If we were to gain more uses of this function over time, the\nnecessity for an extra strbuf_addbuf() becomes annoying. I wonder\nif we should be doing this patch the other way around, i.e. move the\nwhole implementation, except for the early\n\n\tif (feof(fp))\n\t\treturn EOF;\n\tstrbuf_reset(sb);\n\npart, and call it strbuf_addwholeline(), and strbuf_getwholeline()\nbecomes a thin wrapper around it that resets the incoming buffer\nbefore calling strbuf_addwholeline(). The logic to determine EOF\nwould need to be updated if we go that route (i.e. instead of\nchecking sb->len is zero in the !HAVE_GETDELIM side of the\nimplementation, we need to see if the number is the same as before)\nbut it should be minor.\n\nThere is a stale comment in the HAVE_GETDELIM side of the curent\nimplementation, by the way. I think our xrealloc no longer tries\nto \"recover\" from ENOMEM.\n\nHaving said all that, I am fine with this version, at least for now.\n\n> +int strbuf_appendwholeline(struct strbuf *sb, FILE *fp, int term)\n> +{\n> +\tstruct strbuf line = STRBUF_INIT;\n> +\tif (strbuf_getwholeline(&line, fp, term))\n> +\t\treturn EOF;\n\nSo, if caller wants to read the stream until it runs out, it can do\n\n\tstrbuf_init(&sb);\n\twhile (strbuf_appendwholeline(&sb, fp, '\\n') != EOF)\n\t\t; /* keep going */\n\nIf the caller knows how many lines to read, EOF-return can be used\nonly for error checking, e.g.\n\n\tstrbuf_init(&sb);\n\tfor (count = 0; count < 5; count++)\n\t\tif (strbuf_appendwholeline(&sb, fp, '\\n') == EOF)\n\t\t\tdie(\"cannot grab 5 lines\");\n\nIt becomes cumbersome if the input lines are terminated, though.\nThe caller wouldn't be using strbuf_appendwholeline() interface,\ne.g.\n\n\tstrbuf_init(&accum);\n\twhile ((strbuf_getwholeline(&sb, fp, '\\n') != EOF) &&\n strcmp(sb.buf, \"done\\n\"))\n\t\tstrbuf_addbuf(&accum, &sb);\n\nAnyway, I was primarily wondering if returning EOF, which perfectly\nmakes sense for getwholeline(), still makes sense for addwholeline(),\nand it seems that it is still useful.\n\n> +\tstrbuf_addbuf(sb, &line);\n> +\tstrbuf_release(&line);\n> +\treturn 0;\n> +}\n> +\n> static int strbuf_getdelim(struct strbuf *sb, FILE *fp, int term)\n> {\n> \tif (strbuf_getwholeline(sb, fp, term))\n> diff --git a/strbuf.h b/strbuf.h\n> index ce8e49c0b2..411063ca76 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -502,6 +502,12 @@ int strbuf_getline(struct strbuf *sb, FILE *file);\n> */\n> int strbuf_getwholeline(struct strbuf *sb, FILE *file, int term);\n> \n> +/**\n> + * Like `strbuf_getwholeline`, but appends the line instead of\n> + * resetting the buffer first.\n> + */\n> +int strbuf_appendwholeline(struct strbuf *sb, FILE *file, int term);\n> +\n> /**\n> * Like `strbuf_getwholeline`, but operates on a file descriptor.\n> * It reads one character at a time, so it is very slow. Do not\n"},{"id":"394198","messageId":"xmqqh7y9fq29.fsf@gitster.c.googlers.com","threadId":"53094","inReplyTo":"50ffc263293571f8af71fd1d253ac238c6909229.1585129842.git.ps@pks.im","subject":"Re: [PATCH 4/9] update-ref: organize commands in an array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-27T21:25:34Z","receivedAt":"2020-03-27T21:25:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> +static const struct parse_cmd {\n> +\tconst char *prefix;\n> +\tconst char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);\n> +} commands[] = {\n\nDo not call an array the represents a set of THINGs \"type things[]\";\ninstead call it \"type thing[]\", so that the 0th thing can be\nreferred to as thing[0], not things[0].\n\nOne exception is when the set as a whole is referred to more often\nthan individual element of an array, in which case \"things\" (without\nthe [index]) becomes a sensible way to refer to the set.\n\n> +\t{ \"update\", parse_cmd_update },\n> +\t{ \"create\", parse_cmd_create },\n> +\t{ \"delete\", parse_cmd_delete },\n> +\t{ \"verify\", parse_cmd_verify },\n> +\t{ \"option\", parse_cmd_option },\n> +};\n> +\n> static void update_refs_stdin(struct ref_transaction *transaction)\n> {\n> \tstruct strbuf input = STRBUF_INIT;\n> \tconst char *next;\n> +\tint i;\n> \n> \tif (strbuf_read(&input, 0, 1000) < 0)\n> \t\tdie_errno(\"could not read from stdin\");\n> \tnext = input.buf;\n> \t/* Read each line dispatch its command */\n> \twhile (next < input.buf + input.len) {\n> +\t\tconst struct parse_cmd *cmd = NULL;\n> +\n> \t\tif (*next == line_termination)\n> \t\t\tdie(\"empty command in input\");\n> \t\telse if (isspace(*next))\n> \t\t\tdie(\"whitespace before command: %s\", next);\n> -\t\telse if (skip_prefix(next, \"update \", &next))\n> -\t\t\tnext = parse_cmd_update(transaction, &input, next);\n> -\t\telse if (skip_prefix(next, \"create \", &next))\n> -\t\t\tnext = parse_cmd_create(transaction, &input, next);\n> -\t\telse if (skip_prefix(next, \"delete \", &next))\n> -\t\t\tnext = parse_cmd_delete(transaction, &input, next);\n> -\t\telse if (skip_prefix(next, \"verify \", &next))\n> -\t\t\tnext = parse_cmd_verify(transaction, &input, next);\n> -\t\telse if (skip_prefix(next, \"option \", &next))\n> -\t\t\tnext = parse_cmd_option(&input, next);\n> -\t\telse\n> +\n> +\t\tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n> +\t\t\tif (!skip_prefix(next, commands[i].prefix , &next))\n> +\t\t\t\tcontinue;\n> +\t\t\tcmd = &commands[i];\n> +\t\t\tbreak;\n> +\t\t}\n\nThe only reason why you had to sprinkle\n\n\tif (!skip_prefix(next, \" \", &next))\n\t\tdie(_(\"%s: missing space after command\"), cmd);\n\nall over the place is because the table lacks the trailing SP (which\nmakes sense---after all, you are making a table of commands). In\nother words, it's not like some command called from this dispatcher\nwould require \" \" after the command name and some others would not.\n\nSo why not avoid touching the parse_cmd_() at all (except for\nthe \"option\" thing that now needs to take the transaction object for\nuniformity), and then verify the presence of \" \" here, perhaps like\nthis:\n\n\tfor (i = 0; i < ARRAY_SIZE(command); i++) {\n\t\tconst char *eoc;\n\t\tif (!skip_prefix(next, commands[i].prefix, &eoc) ||\n\t\t *eoc != ' ')\n\t\t\tcontinue;\n\t\tcmd = &command[i];\n next = eoc;\n\t\tbreak;\n\t}\n\nNote that you cannot reuse &next here to future-proof the code;\notherwise, you wouldn't be able to add a new command, e.g. \"options\",\nthat sits next to the existing command \"option\", in the future.\n\n> +\t\tif (!cmd)\n> \t\t\tdie(\"unknown command: %s\", next);\n> \n> +\t\tif (input.buf[strlen(cmd->prefix)] != line_termination &&\n> +\t\t input.buf[strlen(cmd->prefix)] != '\\0' &&\n> +\t\t input.buf[strlen(cmd->prefix)] != ' ')\n> +\t\t\tdie(\"%s: no separator after command\", cmd->prefix);\n\nThis part of your version does not make sense to me. If the input\nline began with \"update\" with some separator that is not a SP, the\noriginal would not have matched. But with this code, \"update\\0\"\nwould pass this code and cause cmd->fn() to be called, only to get\nthe input rejected, because next is not pointing to SP. So why do\nyou even need this if statement?\n\n> +\n> +\t\tnext = cmd->fn(transaction, &input, next);\n> \t\tnext++;\n> \t}\n\nThanks.\n"},{"id":"394204","messageId":"xmqqsghtealz.fsf@gitster.c.googlers.com","threadId":"53094","inReplyTo":"bd92a649d79cde43260358ca939baee0f1a2f076.1585129842.git.ps@pks.im","subject":"Re: [PATCH 7/9] update-ref: move transaction handling into `update_refs_stdin()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-27T21:44:40Z","receivedAt":"2020-03-27T21:44:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> While the actual logic to update the transaction is handled in\n> `update_refs_stdin()`, the transaction itself is started and committed\n> in `cmd_update_ref()` itself. This makes it hard to handle transaction\n> abortion and commits as part of `update_refs_stdin()` itself, ...\n\nIt is not offhand clear why it makes any difference, especially\nbecause there is only a single caller of update_refs_stdin()\nfunction, but let's see how this move makes things easier in [8/9]\nand [9/9].\n\nThe patch itself looks like a bug-free no-op change.\n\nThanks.\n"},{"id":"394206","messageId":"xmqqo8she9yp.fsf@gitster.c.googlers.com","threadId":"53094","inReplyTo":"1db63f97ec78fad794cec51c5d96b093f7cd2440.1585129843.git.ps@pks.im","subject":"Re: [PATCH 8/9] update-ref: read commands in a line-wise fashion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-27T21:58:38Z","receivedAt":"2020-03-27T21:58:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> static const struct parse_cmd {\n> \tconst char *prefix;\n> -\tconst char *(*fn)(struct ref_transaction *, const char *, const char *);\n> +\tvoid (*fn)(struct ref_transaction *, const char *, const char *);\n> +\tunsigned extra_lines;\n\nDefine and explain the meaning of extra_lines in a comment. Without\nit, the most natural way to infer what the variable means by readers\nwould be that \"update\" command sometimes receives up to two more\nlines but it also is perfectly normal if there is no extra line.\n\nBut I am not sure if that is what is going on.\n\n\"update\" _always_ needs exactly two more input \"lines\" when the\ninput is NUL delimited, perhaps, and it is an _error_ if there are\nnot these two lines, no?\n\nThe name of the field should make such details clear.\n\n> } commands[] = {\n> -\t{ \"update\", parse_cmd_update },\n> -\t{ \"create\", parse_cmd_create },\n> -\t{ \"delete\", parse_cmd_delete },\n> -\t{ \"verify\", parse_cmd_verify },\n> -\t{ \"option\", parse_cmd_option },\n> +\t{ \"update\", parse_cmd_update, 2 },\n> +\t{ \"create\", parse_cmd_create, 1 },\n> +\t{ \"delete\", parse_cmd_delete, 1 },\n> +\t{ \"verify\", parse_cmd_verify, 1 },\n> +\t{ \"option\", parse_cmd_option, 0 },\n> };\n> +\t\t/* Read extra lines if NUL-terminated */\n> +\t\tfor (j = 0; line_termination == '\\0' && j < cmd->extra_lines; j++)\n> +\t\t\tif (strbuf_appendwholeline(&input, stdin, line_termination))\n> +\t\t\t\tbreak;\n\nAnd this code does not barf if you get a premature EOF and let the\ncall to cmd->fn() go through, which sounds buggy.\n\n> +\t\tcmd->fn(transaction, input.buf + strlen(cmd->prefix),\n> +\t\t\tinput.buf + input.len);\n> \t}\n> \n> \tif (ref_transaction_commit(transaction, &err))\n\nThanks.\n"},{"id":"394207","messageId":"xmqqk135e9vw.fsf@gitster.c.googlers.com","threadId":"53094","inReplyTo":"88c0089bb50bca13efc79a1f8fd42b80286af949.1585129843.git.ps@pks.im","subject":"Re: [PATCH 9/9] update-ref: implement interactive transaction handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-27T22:00:19Z","receivedAt":"2020-03-27T22:00:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> The git-update-ref(1) command can only handle queueing transactions\n> right now via its \"--stdin\" parameter, but there is no way for users to\n> handle the transaction itself in a more explicit way. E.g. in a\n> replicated scenario, one may imagine a coordinator that spawns\n> git-update-ref(1) for multiple repositories and only if all agree that\n> an update is possible will the coordinator send a commit. Such a\n> transactional session could look like\n>\n> > start\n> < start: ok\n> > update refs/heads/master $OLD $NEW\n> > prepare\n> < prepare: ok\n> # All nodes have returned \"ok\"\n> > commit\n> < commit: ok\n>\n> or\n>\n> > start\n> < start: ok\n> > create refs/heads/master $OLD $NEW\n> > prepare\n> < fatal: cannot lock ref 'refs/heads/master': reference already exists\n> # On all other nodes:\n> > abort\n> < abort: ok\n>\n> In order to allow for such transactional sessions, this commit\n> introduces four new commands for git-update-ref(1), which matches those\n> we have internally already with the exception of \"start\":\n>\n> - start: start a new transaction\n>\n> - prepare: prepare the transaction, that is try to lock all\n> references and verify their current value matches the\n> expected one\n>\n> - commit: explicitly commit a session, that is update references to\n> match their new expected state\n>\n> - abort: abort a session and roll back all changes\n\nOK, this is a fun stuff, and explains why we wanted to do [7/9].\n\n"},{"id":"394328","messageId":"20200330080551.GA186019@ncase.pks.im","threadId":"53094","inReplyTo":"xmqqh7y9fq29.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 4/9] update-ref: organize commands in an array","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-30T08:05:51Z","receivedAt":"2020-03-30T08:05:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Mar 27, 2020 at 02:25:34PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > +static const struct parse_cmd {\n> > +\tconst char *prefix;\n> > +\tconst char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);\n> > +} commands[] = {\n> \n> Do not call an array the represents a set of THINGs \"type things[]\";\n> instead call it \"type thing[]\", so that the 0th thing can be\n> referred to as thing[0], not things[0].\n> \n> One exception is when the set as a whole is referred to more often\n> than individual element of an array, in which case \"things\" (without\n> the [index]) becomes a sensible way to refer to the set.\n> \n> > +\t{ \"update\", parse_cmd_update },\n> > +\t{ \"create\", parse_cmd_create },\n> > +\t{ \"delete\", parse_cmd_delete },\n> > +\t{ \"verify\", parse_cmd_verify },\n> > +\t{ \"option\", parse_cmd_option },\n> > +};\n> > +\n> > static void update_refs_stdin(struct ref_transaction *transaction)\n> > {\n> > \tstruct strbuf input = STRBUF_INIT;\n> > \tconst char *next;\n> > +\tint i;\n> > \n> > \tif (strbuf_read(&input, 0, 1000) < 0)\n> > \t\tdie_errno(\"could not read from stdin\");\n> > \tnext = input.buf;\n> > \t/* Read each line dispatch its command */\n> > \twhile (next < input.buf + input.len) {\n> > +\t\tconst struct parse_cmd *cmd = NULL;\n> > +\n> > \t\tif (*next == line_termination)\n> > \t\t\tdie(\"empty command in input\");\n> > \t\telse if (isspace(*next))\n> > \t\t\tdie(\"whitespace before command: %s\", next);\n> > -\t\telse if (skip_prefix(next, \"update \", &next))\n> > -\t\t\tnext = parse_cmd_update(transaction, &input, next);\n> > -\t\telse if (skip_prefix(next, \"create \", &next))\n> > -\t\t\tnext = parse_cmd_create(transaction, &input, next);\n> > -\t\telse if (skip_prefix(next, \"delete \", &next))\n> > -\t\t\tnext = parse_cmd_delete(transaction, &input, next);\n> > -\t\telse if (skip_prefix(next, \"verify \", &next))\n> > -\t\t\tnext = parse_cmd_verify(transaction, &input, next);\n> > -\t\telse if (skip_prefix(next, \"option \", &next))\n> > -\t\t\tnext = parse_cmd_option(&input, next);\n> > -\t\telse\n> > +\n> > +\t\tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n> > +\t\t\tif (!skip_prefix(next, commands[i].prefix , &next))\n> > +\t\t\t\tcontinue;\n> > +\t\t\tcmd = &commands[i];\n> > +\t\t\tbreak;\n> > +\t\t}\n> \n> The only reason why you had to sprinkle\n> \n> \tif (!skip_prefix(next, \" \", &next))\n> \t\tdie(_(\"%s: missing space after command\"), cmd);\n> \n> all over the place is because the table lacks the trailing SP (which\n> makes sense---after all, you are making a table of commands). In\n> other words, it's not like some command called from this dispatcher\n> would require \" \" after the command name and some others would not.\n\n> So why not avoid touching the parse_cmd_() at all (except for\n> the \"option\" thing that now needs to take the transaction object for\n> uniformity), and then verify the presence of \" \" here, perhaps like\n> this:\n> \n> \tfor (i = 0; i < ARRAY_SIZE(command); i++) {\n> \t\tconst char *eoc;\n> \t\tif (!skip_prefix(next, commands[i].prefix, &eoc) ||\n> \t\t *eoc != ' ')\n> \t\t\tcontinue;\n> \t\tcmd = &command[i];\n> next = eoc;\n> \t\tbreak;\n> \t}\n\nThe reason why I moved those `skip_prefix` calls into each of the\nrespective commands is that this patch series introduces calls that do\nnot accept a trailing space at all. Thus we cannot handle the space\ngenerically here, as that would was soon as we introduce the set of new\ncommands.\n\nInitially I just had a trailing \" \" in the `command` array as you hint\nat, but it felt a bit magical to me and might make readers wonder why\nsome commands are spelt \"update \" and why some are spelt \"commit\",\nwithout a trailing space.\n\n> Note that you cannot reuse &next here to future-proof the code;\n> otherwise, you wouldn't be able to add a new command, e.g. \"options\",\n> that sits next to the existing command \"option\", in the future.\n> \n> > +\t\tif (!cmd)\n> > \t\t\tdie(\"unknown command: %s\", next);\n> > \n> > +\t\tif (input.buf[strlen(cmd->prefix)] != line_termination &&\n> > +\t\t input.buf[strlen(cmd->prefix)] != '\\0' &&\n> > +\t\t input.buf[strlen(cmd->prefix)] != ' ')\n> > +\t\t\tdie(\"%s: no separator after command\", cmd->prefix);\n> \n> This part of your version does not make sense to me. If the input\n> line began with \"update\" with some separator that is not a SP, the\n> original would not have matched. But with this code, \"update\\0\"\n> would pass this code and cause cmd->fn() to be called, only to get\n> the input rejected, because next is not pointing to SP. So why do\n> you even need this if statement?\n\nI hope this makes more sense with the above background of commands\nwithout trailing space. It would probably make sense to move this into\nthe command-matching loop though to be able to discern \"option\" and a\npotentially new \"options\" command in the future. Instead of dying, we'd\nthen have to `continue` the loop and check whether any of the remaining\ncommands matches.\n\nPatrick\n"},{"id":"394329","messageId":"20200330081144.GB186019@ncase.pks.im","threadId":"53094","inReplyTo":"xmqqo8she9yp.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 8/9] update-ref: read commands in a line-wise fashion","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-30T08:11:44Z","receivedAt":"2020-03-30T08:11:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Mar 27, 2020 at 02:58:38PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > static const struct parse_cmd {\n> > \tconst char *prefix;\n> > -\tconst char *(*fn)(struct ref_transaction *, const char *, const char *);\n> > +\tvoid (*fn)(struct ref_transaction *, const char *, const char *);\n> > +\tunsigned extra_lines;\n> \n> Define and explain the meaning of extra_lines in a comment. Without\n> it, the most natural way to infer what the variable means by readers\n> would be that \"update\" command sometimes receives up to two more\n> lines but it also is perfectly normal if there is no extra line.\n> \n> But I am not sure if that is what is going on.\n> \n> \"update\" _always_ needs exactly two more input \"lines\" when the\n> input is NUL delimited, perhaps, and it is an _error_ if there are\n> not these two lines, no?\n\nThat's extactly right. I pondered on a good name, but I wasn't yet able\nto come up with any one that brings across its meaning. I originally had\n`extra_args` but changed it after some internal discussion with Chris.\n\n> The name of the field should make such details clear.\n> \n> > } commands[] = {\n> > -\t{ \"update\", parse_cmd_update },\n> > -\t{ \"create\", parse_cmd_create },\n> > -\t{ \"delete\", parse_cmd_delete },\n> > -\t{ \"verify\", parse_cmd_verify },\n> > -\t{ \"option\", parse_cmd_option },\n> > +\t{ \"update\", parse_cmd_update, 2 },\n> > +\t{ \"create\", parse_cmd_create, 1 },\n> > +\t{ \"delete\", parse_cmd_delete, 1 },\n> > +\t{ \"verify\", parse_cmd_verify, 1 },\n> > +\t{ \"option\", parse_cmd_option, 0 },\n> > };\n> > +\t\t/* Read extra lines if NUL-terminated */\n> > +\t\tfor (j = 0; line_termination == '\\0' && j < cmd->extra_lines; j++)\n> > +\t\t\tif (strbuf_appendwholeline(&input, stdin, line_termination))\n> > +\t\t\t\tbreak;\n> \n> And this code does not barf if you get a premature EOF and let the\n> call to cmd->fn() go through, which sounds buggy.\n\nThis is in fact by design, but I only change the comment in a later\ncommit. The reasoning is that by passing even incomplete buffers to a\ncommand, the command is going to be able to print a much better error\nmessage than printing a generic one here.\n\nI'll make sure to move over the comment.\n\nPatrick\n"},{"id":"394342","messageId":"20200330132509.GA1344694@ncase.pks.im","threadId":"53094","inReplyTo":"xmqqpncxfr1p.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/9] strbuf: provide function to append whole lines","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-30T13:25:09Z","receivedAt":"2020-03-30T13:25:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Mar 27, 2020 at 02:04:18PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > While the strbuf interface already provides functions to read a line\n> > into it that completely replaces its current contents, we do not have an\n> > interface that allows for appending lines without discarding current\n> > contents.\n> >\n> > Add a new function `strbuf_appendwholeline` that reads a line including\n> > its terminating character into a strbuf non-destructively. This is a\n> > preparatory step for git-update-ref(1) reading standard input line-wise\n> > instead of as a block.\n> \n> Hmph. If we were to gain more uses of this function over time, the\n> necessity for an extra strbuf_addbuf() becomes annoying. I wonder\n> if we should be doing this patch the other way around, i.e. move the\n> whole implementation, except for the early\n> \n> \tif (feof(fp))\n> \t\treturn EOF;\n> \tstrbuf_reset(sb);\n> \n> part, and call it strbuf_addwholeline(), and strbuf_getwholeline()\n> becomes a thin wrapper around it that resets the incoming buffer\n> before calling strbuf_addwholeline(). The logic to determine EOF\n> would need to be updated if we go that route (i.e. instead of\n> checking sb->len is zero in the !HAVE_GETDELIM side of the\n> implementation, we need to see if the number is the same as before)\n> but it should be minor.\n\nUnfortunately it's not that easy for the HAVE_GETDELIM case, though, as\nwe cannot call getdelim(3P) to append to an already populated buffer. So\nwe'd have to call getdelim(3P) either on a NULL line and append manually\nor on an empty line in case the strbuf doesn't have any contents. While\ndoable, I wanted to keep out this additional complexity for now.\n\nPatrick\n"},{"id":"394344","messageId":"7a297db4daaee84dc90ddeaa8f77b68a39231ef1.1585575749.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585575749.git.ps@pks.im","subject":"[PATCH v2 1/9] refs: fix segfault when aborting empty transaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-30T13:46:13Z","receivedAt":"2020-03-30T13:46:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When cleaning up a transaction that has no updates queued, then the\ntransaction's backend data will not have been allocated. We correctly\nhandle this for the packed backend, where the cleanup function checks\nwhether the backend data has been allocated at all -- if not, then there\nis nothing to clean up. For the files backend we do not check this and\nas a result will hit a segfault due to dereferencing a `NULL` pointer\nwhen cleaning up such a transaction.\n\nFix the issue by checking whether `backend_data` is set in the files\nbackend, too.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/files-backend.c | 20 +++++++++++---------\n 1 file changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 561c33ac8a..6516c7bc8c 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2565,17 +2565,19 @@ static void files_transaction_cleanup(struct files_ref_store *refs,\n \t\t}\n \t}\n \n-\tif (backend_data->packed_transaction &&\n-\t ref_transaction_abort(backend_data->packed_transaction, &err)) {\n-\t\terror(\"error aborting transaction: %s\", err.buf);\n-\t\tstrbuf_release(&err);\n+\tif (backend_data) {\n+\t\tif (backend_data->packed_transaction &&\n+\t\t ref_transaction_abort(backend_data->packed_transaction, &err)) {\n+\t\t\terror(\"error aborting transaction: %s\", err.buf);\n+\t\t\tstrbuf_release(&err);\n+\t\t}\n+\n+\t\tif (backend_data->packed_refs_locked)\n+\t\t\tpacked_refs_unlock(refs->packed_ref_store);\n+\n+\t\tfree(backend_data);\n \t}\n \n-\tif (backend_data->packed_refs_locked)\n-\t\tpacked_refs_unlock(refs->packed_ref_store);\n-\n-\tfree(backend_data);\n-\n \ttransaction->state = REF_TRANSACTION_CLOSED;\n }\n \n-- \n2.26.0\n\n"},{"id":"394345","messageId":"cover.1585575749.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585129842.git.ps@pks.im","subject":"[PATCH v2 0/9] Support for transactions in `git-update-ref --stdin`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-30T13:46:06Z","receivedAt":"2020-03-30T13:46:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of this patch series to introduce transaction\nsupport in `git update-ref --stdin`. The goal is to make commands\navailable to the user of that command so that he can explicitly control\nthe given transaction by starting, preparing, and finally committing or\naborting the transaction.\n\nThe second version addresses the feedback of Junio:\n\n - Renamed the `commands` array to `command`.\n\n - Adds documentation to the `extra_lines` field to hopefully make\n its intent clearer.\n\n - Clarify why we ignore EOF when reading `extra_lines`.\n\n - When reading commands, the code now verifies that a line is not\n only a prefix to the command, but in fact the whole command. This\n allows implementation of multiple commands that have the same\n prefix, e.g. \"option\" and \"options\".\n\nThanks Junio for your review!\n\nPatrick\n\nPatrick Steinhardt (9):\n refs: fix segfault when aborting empty transaction\n git-update-ref.txt: add missing word\n strbuf: provide function to append whole lines\n update-ref: organize commands in an array\n update-ref: drop unused argument for `parse_refname`\n update-ref: pass end pointer instead of strbuf\n update-ref: move transaction handling into `update_refs_stdin()`\n update-ref: read commands in a line-wise fashion\n update-ref: implement interactive transaction handling\n\n Documentation/git-update-ref.txt | 28 +++-\n builtin/update-ref.c | 274 +++++++++++++++++++++++--------\n refs/files-backend.c | 20 ++-\n strbuf.c | 10 ++\n strbuf.h | 6 +\n t/t1400-update-ref.sh | 131 +++++++++++++++\n 6 files changed, 388 insertions(+), 81 deletions(-)\n\nRange-diff against v1:\n 1: 3c7f8c47f3 = 1: 7a297db4da refs: fix segfault when aborting empty transaction\n 2: dd7798abb7 = 2: 15857e1b8c git-update-ref.txt: add missing word\n 3: 4b0a963ea5 = 3: b6546ae44e strbuf: provide function to append whole lines\n 4: 50ffc26329 ! 4: bd8c059fbc update-ref: organize commands in an array\n @@ builtin/update-ref.c: static const char *parse_cmd_option(struct strbuf *input,\n +static const struct parse_cmd {\n +\tconst char *prefix;\n +\tconst char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);\n -+} commands[] = {\n ++} command[] = {\n +\t{ \"update\", parse_cmd_update },\n +\t{ \"create\", parse_cmd_create },\n +\t{ \"delete\", parse_cmd_delete },\n @@ builtin/update-ref.c: static const char *parse_cmd_option(struct strbuf *input,\n -\t\t\tnext = parse_cmd_option(&input, next);\n -\t\telse\n +\n -+\t\tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n -+\t\t\tif (!skip_prefix(next, commands[i].prefix , &next))\n ++\t\tfor (i = 0; i < ARRAY_SIZE(command); i++) {\n ++\t\t\tconst char *prefix = command[i].prefix;\n ++\n ++\t\t\tif (!skip_prefix(next, prefix, &next))\n +\t\t\t\tcontinue;\n -+\t\t\tcmd = &commands[i];\n ++\n ++\t\t\t/*\n ++\t\t\t * Check that the command is terminated by an argument\n ++\t\t\t * or line terminator and not only a matching prefix.\n ++\t\t\t */\n ++\t\t\tif (input.buf[strlen(prefix)] != line_termination &&\n ++\t\t\t input.buf[strlen(prefix)] != '\\0' &&\n ++\t\t\t input.buf[strlen(prefix)] != ' ')\n ++\t\t\t\tcontinue;\n ++\n ++\t\t\tcmd = &command[i];\n +\t\t\tbreak;\n +\t\t}\n +\t\tif (!cmd)\n \t\t\tdie(\"unknown command: %s\", next);\n \n -+\t\tif (input.buf[strlen(cmd->prefix)] != line_termination &&\n -+\t\t input.buf[strlen(cmd->prefix)] != '\\0' &&\n -+\t\t input.buf[strlen(cmd->prefix)] != ' ')\n -+\t\t\tdie(\"%s: no separator after command\", cmd->prefix);\n -+\n +\t\tnext = cmd->fn(transaction, &input, next);\n \t\tnext++;\n \t}\n 5: a78043b188 = 5: 49a14d2046 update-ref: drop unused argument for `parse_refname`\n 6: 51ebb2f849 ! 6: cbe430029d update-ref: pass end pointer instead of strbuf\n @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction\n \tconst char *prefix;\n -\tconst char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);\n +\tconst char *(*fn)(struct ref_transaction *, const char *, const char *);\n - } commands[] = {\n + } command[] = {\n \t{ \"update\", parse_cmd_update },\n \t{ \"create\", parse_cmd_create },\n @@ builtin/update-ref.c: static void update_refs_stdin(struct ref_transaction *transaction)\n - \t\t input.buf[strlen(cmd->prefix)] != ' ')\n - \t\t\tdie(\"%s: no separator after command\", cmd->prefix);\n + \t\tif (!cmd)\n + \t\t\tdie(\"unknown command: %s\", next);\n \n -\t\tnext = cmd->fn(transaction, &input, next);\n +\t\tnext = cmd->fn(transaction, next, input.buf + input.len);\n 7: bd92a649d7 = 7: d2f68f59a7 update-ref: move transaction handling into `update_refs_stdin()`\n 8: 1db63f97ec ! 8: f8786fdeb3 update-ref: read commands in a line-wise fashion\n @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction\n \tconst char *prefix;\n -\tconst char *(*fn)(struct ref_transaction *, const char *, const char *);\n +\tvoid (*fn)(struct ref_transaction *, const char *, const char *);\n ++\t/*\n ++\t * If using NUL-terminated format, only the first argument will be\n ++\t * available in the first line. In case a command expects more than one\n ++\t * argument, we thus have to fetch an additional `extra_lines` number\n ++\t * of lines.\n ++\t */\n +\tunsigned extra_lines;\n - } commands[] = {\n + } command[] = {\n -\t{ \"update\", parse_cmd_update },\n -\t{ \"create\", parse_cmd_create },\n -\t{ \"delete\", parse_cmd_delete },\n @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction\n +\t\telse if (isspace(*input.buf))\n +\t\t\tdie(\"whitespace before command: %s\", input.buf);\n \n - \t\tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n --\t\t\tif (!skip_prefix(next, commands[i].prefix , &next))\n -+\t\t\tif (!starts_with(input.buf, commands[i].prefix))\n + \t\tfor (i = 0; i < ARRAY_SIZE(command); i++) {\n + \t\t\tconst char *prefix = command[i].prefix;\n + \n +-\t\t\tif (!skip_prefix(next, prefix, &next))\n ++\t\t\tif (!starts_with(input.buf, prefix))\n \t\t\t\tcontinue;\n - \t\t\tcmd = &commands[i];\n + \n + \t\t\t/*\n +@@ builtin/update-ref.c: static void update_refs_stdin(void)\n \t\t\tbreak;\n \t\t}\n \t\tif (!cmd)\n -\t\t\tdie(\"unknown command: %s\", next);\n +\t\t\tdie(\"unknown command: %s\", input.buf);\n \n - \t\tif (input.buf[strlen(cmd->prefix)] != line_termination &&\n - \t\t input.buf[strlen(cmd->prefix)] != '\\0' &&\n - \t\t input.buf[strlen(cmd->prefix)] != ' ')\n - \t\t\tdie(\"%s: no separator after command\", cmd->prefix);\n - \n -\t\tnext = cmd->fn(transaction, next, input.buf + input.len);\n -\t\tnext++;\n -+\t\t/* Read extra lines if NUL-terminated */\n ++\t\t/*\n ++\t\t * Read extra lines if NUL-terminated. Do not raise an error in\n ++\t\t * case there is an early EOF to let the command handle missing\n ++\t\t * arguments with a proper error message.\n ++\t\t */\n +\t\tfor (j = 0; line_termination == '\\0' && j < cmd->extra_lines; j++)\n +\t\t\tif (strbuf_appendwholeline(&input, stdin, line_termination))\n +\t\t\t\tbreak;\n 9: 88c0089bb5 ! 9: c3fffdf9fa update-ref: implement interactive transaction handling\n @@ builtin/update-ref.c: static void parse_cmd_option(struct ref_transaction *trans\n static const struct parse_cmd {\n \tconst char *prefix;\n \tvoid (*fn)(struct ref_transaction *, const char *, const char *);\n +@@ builtin/update-ref.c: static const struct parse_cmd {\n + \t * of lines.\n + \t */\n \tunsigned extra_lines;\n +\tenum update_refs_state state;\n - } commands[] = {\n + } command[] = {\n -\t{ \"update\", parse_cmd_update, 2 },\n -\t{ \"create\", parse_cmd_create, 1 },\n -\t{ \"delete\", parse_cmd_delete, 1 },\n @@ builtin/update-ref.c: static void parse_cmd_option(struct ref_transaction *trans\n \tint i, j;\n \n @@ builtin/update-ref.c: static void update_refs_stdin(void)\n - \t\t input.buf[strlen(cmd->prefix)] != ' ')\n - \t\t\tdie(\"%s: no separator after command\", cmd->prefix);\n - \n --\t\t/* Read extra lines if NUL-terminated */\n -+\t\t/* Read extra lines if NUL-terminated, but let commands handle missing ones. */\n - \t\tfor (j = 0; line_termination == '\\0' && j < cmd->extra_lines; j++)\n \t\t\tif (strbuf_appendwholeline(&input, stdin, line_termination))\n \t\t\t\tbreak;\n \n-- \n2.26.0\n\n"},{"id":"394346","messageId":"15857e1b8ca800a92fa96fee818a54456bfbd330.1585575749.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585575749.git.ps@pks.im","subject":"[PATCH v2 2/9] git-update-ref.txt: add missing word","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-30T13:46:20Z","receivedAt":"2020-03-30T13:46:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The description for the \"verify\" command is lacking a single word \"is\",\nwhich this commit corrects.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/git-update-ref.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt\nindex 9671423117..9bd039ce08 100644\n--- a/Documentation/git-update-ref.txt\n+++ b/Documentation/git-update-ref.txt\n@@ -107,7 +107,7 @@ delete::\n \n verify::\n \tVerify against but do not change it. If\n-\t zero or missing, the ref must not exist.\n+\t is zero or missing, the ref must not exist.\n \n option::\n \tModify behavior of the next command naming a .\n-- \n2.26.0\n\n"},{"id":"394347","messageId":"b6546ae44e6a5f7b57d75d59b23a777b363cac1e.1585575749.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585575749.git.ps@pks.im","subject":"[PATCH v2 3/9] strbuf: provide function to append whole lines","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-30T13:46:27Z","receivedAt":"2020-03-30T13:46:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While the strbuf interface already provides functions to read a line\ninto it that completely replaces its current contents, we do not have an\ninterface that allows for appending lines without discarding current\ncontents.\n\nAdd a new function `strbuf_appendwholeline` that reads a line including\nits terminating character into a strbuf non-destructively. This is a\npreparatory step for git-update-ref(1) reading standard input line-wise\ninstead of as a block.\n\nSigned-off-by: Patrick Steinhardt \n---\n strbuf.c | 10 ++++++++++\n strbuf.h | 6 ++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex bb0065ccaf..6e74901bfa 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -690,6 +690,16 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n }\n #endif\n \n+int strbuf_appendwholeline(struct strbuf *sb, FILE *fp, int term)\n+{\n+\tstruct strbuf line = STRBUF_INIT;\n+\tif (strbuf_getwholeline(&line, fp, term))\n+\t\treturn EOF;\n+\tstrbuf_addbuf(sb, &line);\n+\tstrbuf_release(&line);\n+\treturn 0;\n+}\n+\n static int strbuf_getdelim(struct strbuf *sb, FILE *fp, int term)\n {\n \tif (strbuf_getwholeline(sb, fp, term))\ndiff --git a/strbuf.h b/strbuf.h\nindex ce8e49c0b2..411063ca76 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -502,6 +502,12 @@ int strbuf_getline(struct strbuf *sb, FILE *file);\n */\n int strbuf_getwholeline(struct strbuf *sb, FILE *file, int term);\n \n+/**\n+ * Like `strbuf_getwholeline`, but appends the line instead of\n+ * resetting the buffer first.\n+ */\n+int strbuf_appendwholeline(struct strbuf *sb, FILE *file, int term);\n+\n /**\n * Like `strbuf_getwholeline`, but operates on a file descriptor.\n * It reads one character at a time, so it is very slow. Do not\n-- \n2.26.0\n\n"},{"id":"394348","messageId":"bd8c059fbcd9dc21203dff61e047af66e4239caf.1585575749.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585575749.git.ps@pks.im","subject":"[PATCH v2 4/9] update-ref: organize commands in an array","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-30T13:46:33Z","receivedAt":"2020-03-30T13:46:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We currently manually wire up all commands known to `git-update-ref\n--stdin`, making it harder than necessary to preprocess arguments after\nthe command is determined. To make this more extensible, let's refactor\nthe code to use an array of known commands instead. While this doesn't\nadd a lot of value now, it is a preparatory step to implement line-wise\nreading of commands.\n\nAs we're going to introduce commands without trailing spaces, this\ncommit also moves whitespace parsing into the respective commands.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/update-ref.c | 73 +++++++++++++++++++++++++++++++++-----------\n 1 file changed, 56 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 2d8f7f0578..559475681e 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -171,11 +171,11 @@ static int parse_next_oid(struct strbuf *input, const char **next,\n /*\n * The following five parse_cmd_*() functions parse the corresponding\n * command. In each case, next points at the character following the\n- * command name and the following space. They each return a pointer\n- * to the character terminating the command, and die with an\n- * explanatory message if there are any parsing problems. All of\n- * these functions handle either text or binary format input,\n- * depending on how line_termination is set.\n+ * command name. They each return a pointer to the character\n+ * terminating the command, and die with an explanatory message if\n+ * there are any parsing problems. All of these functions handle\n+ * either text or binary format input, depending on how\n+ * line_termination is set.\n */\n \n static const char *parse_cmd_update(struct ref_transaction *transaction,\n@@ -186,6 +186,9 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n \tstruct object_id new_oid, old_oid;\n \tint have_old;\n \n+\tif (!skip_prefix(next, \" \", &next))\n+\t\tdie(\"update: missing space after command\");\n+\n \trefname = parse_refname(input, &next);\n \tif (!refname)\n \t\tdie(\"update: missing \");\n@@ -220,6 +223,9 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,\n \tchar *refname;\n \tstruct object_id new_oid;\n \n+\tif (!skip_prefix(next, \" \", &next))\n+\t\tdie(\"create: missing space after command\");\n+\n \trefname = parse_refname(input, &next);\n \tif (!refname)\n \t\tdie(\"create: missing \");\n@@ -253,6 +259,9 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,\n \tstruct object_id old_oid;\n \tint have_old;\n \n+\tif (!skip_prefix(next, \" \", &next))\n+\t\tdie(\"delete: missing space after command\");\n+\n \trefname = parse_refname(input, &next);\n \tif (!refname)\n \t\tdie(\"delete: missing \");\n@@ -288,6 +297,9 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n \tchar *refname;\n \tstruct object_id old_oid;\n \n+\tif (!skip_prefix(next, \" \", &next))\n+\t\tdie(\"verify: missing space after command\");\n+\n \trefname = parse_refname(input, &next);\n \tif (!refname)\n \t\tdie(\"verify: missing \");\n@@ -310,9 +322,12 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n \treturn next;\n }\n \n-static const char *parse_cmd_option(struct strbuf *input, const char *next)\n+static const char *parse_cmd_option(struct ref_transaction *transaction,\n+\t\t\t\t struct strbuf *input, const char *next)\n {\n \tconst char *rest;\n+\tif (!skip_prefix(next, \" \", &next))\n+\t\tdie(\"option: missing space after command\");\n \tif (skip_prefix(next, \"no-deref\", &rest) && *rest == line_termination)\n \t\tupdate_flags |= REF_NO_DEREF;\n \telse\n@@ -320,33 +335,57 @@ static const char *parse_cmd_option(struct strbuf *input, const char *next)\n \treturn rest;\n }\n \n+static const struct parse_cmd {\n+\tconst char *prefix;\n+\tconst char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);\n+} command[] = {\n+\t{ \"update\", parse_cmd_update },\n+\t{ \"create\", parse_cmd_create },\n+\t{ \"delete\", parse_cmd_delete },\n+\t{ \"verify\", parse_cmd_verify },\n+\t{ \"option\", parse_cmd_option },\n+};\n+\n static void update_refs_stdin(struct ref_transaction *transaction)\n {\n \tstruct strbuf input = STRBUF_INIT;\n \tconst char *next;\n+\tint i;\n \n \tif (strbuf_read(&input, 0, 1000) < 0)\n \t\tdie_errno(\"could not read from stdin\");\n \tnext = input.buf;\n \t/* Read each line dispatch its command */\n \twhile (next < input.buf + input.len) {\n+\t\tconst struct parse_cmd *cmd = NULL;\n+\n \t\tif (*next == line_termination)\n \t\t\tdie(\"empty command in input\");\n \t\telse if (isspace(*next))\n \t\t\tdie(\"whitespace before command: %s\", next);\n-\t\telse if (skip_prefix(next, \"update \", &next))\n-\t\t\tnext = parse_cmd_update(transaction, &input, next);\n-\t\telse if (skip_prefix(next, \"create \", &next))\n-\t\t\tnext = parse_cmd_create(transaction, &input, next);\n-\t\telse if (skip_prefix(next, \"delete \", &next))\n-\t\t\tnext = parse_cmd_delete(transaction, &input, next);\n-\t\telse if (skip_prefix(next, \"verify \", &next))\n-\t\t\tnext = parse_cmd_verify(transaction, &input, next);\n-\t\telse if (skip_prefix(next, \"option \", &next))\n-\t\t\tnext = parse_cmd_option(&input, next);\n-\t\telse\n+\n+\t\tfor (i = 0; i < ARRAY_SIZE(command); i++) {\n+\t\t\tconst char *prefix = command[i].prefix;\n+\n+\t\t\tif (!skip_prefix(next, prefix, &next))\n+\t\t\t\tcontinue;\n+\n+\t\t\t/*\n+\t\t\t * Check that the command is terminated by an argument\n+\t\t\t * or line terminator and not only a matching prefix.\n+\t\t\t */\n+\t\t\tif (input.buf[strlen(prefix)] != line_termination &&\n+\t\t\t input.buf[strlen(prefix)] != '\\0' &&\n+\t\t\t input.buf[strlen(prefix)] != ' ')\n+\t\t\t\tcontinue;\n+\n+\t\t\tcmd = &command[i];\n+\t\t\tbreak;\n+\t\t}\n+\t\tif (!cmd)\n \t\t\tdie(\"unknown command: %s\", next);\n \n+\t\tnext = cmd->fn(transaction, &input, next);\n \t\tnext++;\n \t}\n \n-- \n2.26.0\n\n"},{"id":"394349","messageId":"49a14d20462bb9d7f00e42df73fc21b050be1db0.1585575749.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585575749.git.ps@pks.im","subject":"[PATCH v2 5/9] update-ref: drop unused argument for `parse_refname`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-30T13:46:40Z","receivedAt":"2020-03-30T13:46:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `parse_refname` function accepts a `struct strbuf *input` argument\nthat isn't used at all. As we're about to convert commands to not use a\nstrbuf anymore but instead an end pointer, let's drop this argument now\nto make the converting commit easier to review.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/update-ref.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 559475681e..6f2aba916b 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -50,7 +50,7 @@ static const char *parse_arg(const char *next, struct strbuf *arg)\n * the argument. Die if C-quoting is malformed or the reference name\n * is invalid.\n */\n-static char *parse_refname(struct strbuf *input, const char **next)\n+static char *parse_refname(const char **next)\n {\n \tstruct strbuf ref = STRBUF_INIT;\n \n@@ -189,7 +189,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n \tif (!skip_prefix(next, \" \", &next))\n \t\tdie(\"update: missing space after command\");\n \n-\trefname = parse_refname(input, &next);\n+\trefname = parse_refname(&next);\n \tif (!refname)\n \t\tdie(\"update: missing \");\n \n@@ -226,7 +226,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,\n \tif (!skip_prefix(next, \" \", &next))\n \t\tdie(\"create: missing space after command\");\n \n-\trefname = parse_refname(input, &next);\n+\trefname = parse_refname(&next);\n \tif (!refname)\n \t\tdie(\"create: missing \");\n \n@@ -262,7 +262,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,\n \tif (!skip_prefix(next, \" \", &next))\n \t\tdie(\"delete: missing space after command\");\n \n-\trefname = parse_refname(input, &next);\n+\trefname = parse_refname(&next);\n \tif (!refname)\n \t\tdie(\"delete: missing \");\n \n@@ -300,7 +300,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n \tif (!skip_prefix(next, \" \", &next))\n \t\tdie(\"verify: missing space after command\");\n \n-\trefname = parse_refname(input, &next);\n+\trefname = parse_refname(&next);\n \tif (!refname)\n \t\tdie(\"verify: missing \");\n \n-- \n2.26.0\n\n"},{"id":"394350","messageId":"cbe430029df9e8e6dc88e3e0ca7380ea5b6d00ef.1585575749.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585575749.git.ps@pks.im","subject":"[PATCH v2 6/9] update-ref: pass end pointer instead of strbuf","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-30T13:46:45Z","receivedAt":"2020-03-30T13:46:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We currently pass both an `strbuf` containing the current command line\nas well as the `next` pointer pointing to the first argument to\ncommands. This is both confusing and makes code more intertwined.\nConvert this to use a simple pointer as well as a pointer pointing to\nthe end of the input as a preparatory step to line-wise reading of\nstdin.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/update-ref.c | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 6f2aba916b..139c3b11fa 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -95,7 +95,7 @@ static char *parse_refname(const char **next)\n * provided but cannot be converted to a SHA-1, die. flags can\n * include PARSE_SHA1_OLD and/or PARSE_SHA1_ALLOW_EMPTY.\n */\n-static int parse_next_oid(struct strbuf *input, const char **next,\n+static int parse_next_oid(const char **next, const char *end,\n \t\t\t struct object_id *oid,\n \t\t\t const char *command, const char *refname,\n \t\t\t int flags)\n@@ -103,7 +103,7 @@ static int parse_next_oid(struct strbuf *input, const char **next,\n \tstruct strbuf arg = STRBUF_INIT;\n \tint ret = 0;\n \n-\tif (*next == input->buf + input->len)\n+\tif (*next == end)\n \t\tgoto eof;\n \n \tif (line_termination) {\n@@ -128,7 +128,7 @@ static int parse_next_oid(struct strbuf *input, const char **next,\n \t\t\tdie(\"%s %s: expected NUL but got: %s\",\n \t\t\t command, refname, *next);\n \t\t(*next)++;\n-\t\tif (*next == input->buf + input->len)\n+\t\tif (*next == end)\n \t\t\tgoto eof;\n \t\tstrbuf_addstr(&arg, *next);\n \t\t*next += arg.len;\n@@ -179,7 +179,7 @@ static int parse_next_oid(struct strbuf *input, const char **next,\n */\n \n static const char *parse_cmd_update(struct ref_transaction *transaction,\n-\t\t\t\t struct strbuf *input, const char *next)\n+\t\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -193,11 +193,11 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n \tif (!refname)\n \t\tdie(\"update: missing \");\n \n-\tif (parse_next_oid(input, &next, &new_oid, \"update\", refname,\n+\tif (parse_next_oid(&next, end, &new_oid, \"update\", refname,\n \t\t\t PARSE_SHA1_ALLOW_EMPTY))\n \t\tdie(\"update %s: missing \", refname);\n \n-\thave_old = !parse_next_oid(input, &next, &old_oid, \"update\", refname,\n+\thave_old = !parse_next_oid(&next, end, &old_oid, \"update\", refname,\n \t\t\t\t PARSE_SHA1_OLD);\n \n \tif (*next != line_termination)\n@@ -217,7 +217,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n }\n \n static const char *parse_cmd_create(struct ref_transaction *transaction,\n-\t\t\t\t struct strbuf *input, const char *next)\n+\t\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -230,7 +230,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,\n \tif (!refname)\n \t\tdie(\"create: missing \");\n \n-\tif (parse_next_oid(input, &next, &new_oid, \"create\", refname, 0))\n+\tif (parse_next_oid(&next, end, &new_oid, \"create\", refname, 0))\n \t\tdie(\"create %s: missing \", refname);\n \n \tif (is_null_oid(&new_oid))\n@@ -252,7 +252,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,\n }\n \n static const char *parse_cmd_delete(struct ref_transaction *transaction,\n-\t\t\t\t struct strbuf *input, const char *next)\n+\t\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -266,7 +266,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,\n \tif (!refname)\n \t\tdie(\"delete: missing \");\n \n-\tif (parse_next_oid(input, &next, &old_oid, \"delete\", refname,\n+\tif (parse_next_oid(&next, end, &old_oid, \"delete\", refname,\n \t\t\t PARSE_SHA1_OLD)) {\n \t\thave_old = 0;\n \t} else {\n@@ -291,7 +291,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,\n }\n \n static const char *parse_cmd_verify(struct ref_transaction *transaction,\n-\t\t\t\t struct strbuf *input, const char *next)\n+\t\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -304,7 +304,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n \tif (!refname)\n \t\tdie(\"verify: missing \");\n \n-\tif (parse_next_oid(input, &next, &old_oid, \"verify\", refname,\n+\tif (parse_next_oid(&next, end, &old_oid, \"verify\", refname,\n \t\t\t PARSE_SHA1_OLD))\n \t\toidclr(&old_oid);\n \n@@ -323,7 +323,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n }\n \n static const char *parse_cmd_option(struct ref_transaction *transaction,\n-\t\t\t\t struct strbuf *input, const char *next)\n+\t\t\t\t const char *next, const char *end)\n {\n \tconst char *rest;\n \tif (!skip_prefix(next, \" \", &next))\n@@ -337,7 +337,7 @@ static const char *parse_cmd_option(struct ref_transaction *transaction,\n \n static const struct parse_cmd {\n \tconst char *prefix;\n-\tconst char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);\n+\tconst char *(*fn)(struct ref_transaction *, const char *, const char *);\n } command[] = {\n \t{ \"update\", parse_cmd_update },\n \t{ \"create\", parse_cmd_create },\n@@ -385,7 +385,7 @@ static void update_refs_stdin(struct ref_transaction *transaction)\n \t\tif (!cmd)\n \t\t\tdie(\"unknown command: %s\", next);\n \n-\t\tnext = cmd->fn(transaction, &input, next);\n+\t\tnext = cmd->fn(transaction, next, input.buf + input.len);\n \t\tnext++;\n \t}\n \n-- \n2.26.0\n\n"},{"id":"394351","messageId":"d2f68f59a716544c637f1c14cfc54bfccdf31181.1585575749.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585575749.git.ps@pks.im","subject":"[PATCH v2 7/9] update-ref: move transaction handling into `update_refs_stdin()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-30T13:46:52Z","receivedAt":"2020-03-30T13:46:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While the actual logic to update the transaction is handled in\n`update_refs_stdin()`, the transaction itself is started and committed\nin `cmd_update_ref()` itself. This makes it hard to handle transaction\nabortion and commits as part of `update_refs_stdin()` itself, which is\nrequired in order to introduce transaction handling features to `git\nupdate-refs --stdin`.\n\nRefactor the code to move all transaction handling into\n`update_refs_stdin()` to prepare for transaction handling features.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/update-ref.c | 27 ++++++++++++++-------------\n 1 file changed, 14 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 139c3b11fa..1a7906545d 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -346,15 +346,21 @@ static const struct parse_cmd {\n \t{ \"option\", parse_cmd_option },\n };\n \n-static void update_refs_stdin(struct ref_transaction *transaction)\n+static void update_refs_stdin(void)\n {\n-\tstruct strbuf input = STRBUF_INIT;\n+\tstruct strbuf input = STRBUF_INIT, err = STRBUF_INIT;\n+\tstruct ref_transaction *transaction;\n \tconst char *next;\n \tint i;\n \n \tif (strbuf_read(&input, 0, 1000) < 0)\n \t\tdie_errno(\"could not read from stdin\");\n \tnext = input.buf;\n+\n+\ttransaction = ref_transaction_begin(&err);\n+\tif (!transaction)\n+\t\tdie(\"%s\", err.buf);\n+\n \t/* Read each line dispatch its command */\n \twhile (next < input.buf + input.len) {\n \t\tconst struct parse_cmd *cmd = NULL;\n@@ -389,6 +395,11 @@ static void update_refs_stdin(struct ref_transaction *transaction)\n \t\tnext++;\n \t}\n \n+\tif (ref_transaction_commit(transaction, &err))\n+\t\tdie(\"%s\", err.buf);\n+\n+\tref_transaction_free(transaction);\n+\tstrbuf_release(&err);\n \tstrbuf_release(&input);\n }\n \n@@ -423,21 +434,11 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (read_stdin) {\n-\t\tstruct strbuf err = STRBUF_INIT;\n-\t\tstruct ref_transaction *transaction;\n-\n-\t\ttransaction = ref_transaction_begin(&err);\n-\t\tif (!transaction)\n-\t\t\tdie(\"%s\", err.buf);\n \t\tif (delete || argc > 0)\n \t\t\tusage_with_options(git_update_ref_usage, options);\n \t\tif (end_null)\n \t\t\tline_termination = '\\0';\n-\t\tupdate_refs_stdin(transaction);\n-\t\tif (ref_transaction_commit(transaction, &err))\n-\t\t\tdie(\"%s\", err.buf);\n-\t\tref_transaction_free(transaction);\n-\t\tstrbuf_release(&err);\n+\t\tupdate_refs_stdin();\n \t\treturn 0;\n \t}\n \n-- \n2.26.0\n\n"},{"id":"394352","messageId":"f8786fdeb351a60bb83ac64cf5a1cbc34c73e2e3.1585575749.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585575749.git.ps@pks.im","subject":"[PATCH v2 8/9] update-ref: read commands in a line-wise fashion","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-30T13:46:59Z","receivedAt":"2020-03-30T13:47:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The git-update-ref(1) supports a `--stdin` mode that allows it to read\nall reference updates from standard input. This is mainly used to allow\nfor atomic reference updates that are all or nothing, so that either all\nreferences will get updated or none.\n\nCurrently, git-update-ref(1) reads all commands as a single block of up\nto 1000 characters and only starts processing after stdin gets closed.\nThis is less flexible than one might wish for, as it doesn't really\nallow for longer-lived transactions and doesn't allow any verification\nwithout committing everything. E.g. one may imagine the following\nexchange:\n\n > start\n < start: ok\n > update refs/heads/master $NEWOID1 $OLDOID1\n > update refs/heads/branch $NEWOID2 $OLDOID2\n > prepare\n < prepare: ok\n > commit\n < commit: ok\n\nWhen reading all input as a whole block, the above interactive protocol\nis obviously impossible to achieve. But by converting the command to\nread commands linewise, we can make it more interactive than before.\n\nObviously, the linewise interface is only a first step in making\ngit-update-ref(1) work in a more transaction-oriented way. Missing is\nmost importantly support for transactional commands that manage the\ncurrent transaction.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/update-ref.c | 80 +++++++++++++++++++++++---------------------\n 1 file changed, 41 insertions(+), 39 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 1a7906545d..77cd235dfc 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -178,8 +178,8 @@ static int parse_next_oid(const char **next, const char *end,\n * line_termination is set.\n */\n \n-static const char *parse_cmd_update(struct ref_transaction *transaction,\n-\t\t\t\t const char *next, const char *end)\n+static void parse_cmd_update(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -212,12 +212,10 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n \tupdate_flags = default_flags;\n \tfree(refname);\n \tstrbuf_release(&err);\n-\n-\treturn next;\n }\n \n-static const char *parse_cmd_create(struct ref_transaction *transaction,\n-\t\t\t\t const char *next, const char *end)\n+static void parse_cmd_create(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -247,12 +245,10 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,\n \tupdate_flags = default_flags;\n \tfree(refname);\n \tstrbuf_release(&err);\n-\n-\treturn next;\n }\n \n-static const char *parse_cmd_delete(struct ref_transaction *transaction,\n-\t\t\t\t const char *next, const char *end)\n+static void parse_cmd_delete(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -286,12 +282,10 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,\n \tupdate_flags = default_flags;\n \tfree(refname);\n \tstrbuf_release(&err);\n-\n-\treturn next;\n }\n \n-static const char *parse_cmd_verify(struct ref_transaction *transaction,\n-\t\t\t\t const char *next, const char *end)\n+static void parse_cmd_verify(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -318,12 +312,10 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n \tupdate_flags = default_flags;\n \tfree(refname);\n \tstrbuf_release(&err);\n-\n-\treturn next;\n }\n \n-static const char *parse_cmd_option(struct ref_transaction *transaction,\n-\t\t\t\t const char *next, const char *end)\n+static void parse_cmd_option(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n {\n \tconst char *rest;\n \tif (!skip_prefix(next, \" \", &next))\n@@ -332,48 +324,49 @@ static const char *parse_cmd_option(struct ref_transaction *transaction,\n \t\tupdate_flags |= REF_NO_DEREF;\n \telse\n \t\tdie(\"option unknown: %s\", next);\n-\treturn rest;\n }\n \n static const struct parse_cmd {\n \tconst char *prefix;\n-\tconst char *(*fn)(struct ref_transaction *, const char *, const char *);\n+\tvoid (*fn)(struct ref_transaction *, const char *, const char *);\n+\t/*\n+\t * If using NUL-terminated format, only the first argument will be\n+\t * available in the first line. In case a command expects more than one\n+\t * argument, we thus have to fetch an additional `extra_lines` number\n+\t * of lines.\n+\t */\n+\tunsigned extra_lines;\n } command[] = {\n-\t{ \"update\", parse_cmd_update },\n-\t{ \"create\", parse_cmd_create },\n-\t{ \"delete\", parse_cmd_delete },\n-\t{ \"verify\", parse_cmd_verify },\n-\t{ \"option\", parse_cmd_option },\n+\t{ \"update\", parse_cmd_update, 2 },\n+\t{ \"create\", parse_cmd_create, 1 },\n+\t{ \"delete\", parse_cmd_delete, 1 },\n+\t{ \"verify\", parse_cmd_verify, 1 },\n+\t{ \"option\", parse_cmd_option, 0 },\n };\n \n static void update_refs_stdin(void)\n {\n \tstruct strbuf input = STRBUF_INIT, err = STRBUF_INIT;\n \tstruct ref_transaction *transaction;\n-\tconst char *next;\n-\tint i;\n-\n-\tif (strbuf_read(&input, 0, 1000) < 0)\n-\t\tdie_errno(\"could not read from stdin\");\n-\tnext = input.buf;\n+\tint i, j;\n \n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction)\n \t\tdie(\"%s\", err.buf);\n \n \t/* Read each line dispatch its command */\n-\twhile (next < input.buf + input.len) {\n+\twhile (!strbuf_getwholeline(&input, stdin, line_termination)) {\n \t\tconst struct parse_cmd *cmd = NULL;\n \n-\t\tif (*next == line_termination)\n+\t\tif (*input.buf == line_termination)\n \t\t\tdie(\"empty command in input\");\n-\t\telse if (isspace(*next))\n-\t\t\tdie(\"whitespace before command: %s\", next);\n+\t\telse if (isspace(*input.buf))\n+\t\t\tdie(\"whitespace before command: %s\", input.buf);\n \n \t\tfor (i = 0; i < ARRAY_SIZE(command); i++) {\n \t\t\tconst char *prefix = command[i].prefix;\n \n-\t\t\tif (!skip_prefix(next, prefix, &next))\n+\t\t\tif (!starts_with(input.buf, prefix))\n \t\t\t\tcontinue;\n \n \t\t\t/*\n@@ -389,10 +382,19 @@ static void update_refs_stdin(void)\n \t\t\tbreak;\n \t\t}\n \t\tif (!cmd)\n-\t\t\tdie(\"unknown command: %s\", next);\n+\t\t\tdie(\"unknown command: %s\", input.buf);\n \n-\t\tnext = cmd->fn(transaction, next, input.buf + input.len);\n-\t\tnext++;\n+\t\t/*\n+\t\t * Read extra lines if NUL-terminated. Do not raise an error in\n+\t\t * case there is an early EOF to let the command handle missing\n+\t\t * arguments with a proper error message.\n+\t\t */\n+\t\tfor (j = 0; line_termination == '\\0' && j < cmd->extra_lines; j++)\n+\t\t\tif (strbuf_appendwholeline(&input, stdin, line_termination))\n+\t\t\t\tbreak;\n+\n+\t\tcmd->fn(transaction, input.buf + strlen(cmd->prefix),\n+\t\t\tinput.buf + input.len);\n \t}\n \n \tif (ref_transaction_commit(transaction, &err))\n-- \n2.26.0\n\n"},{"id":"394353","messageId":"c3fffdf9fa5bcf123aff2d6f9b904f5cac57972a.1585575749.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585575749.git.ps@pks.im","subject":"[PATCH v2 9/9] update-ref: implement interactive transaction handling","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-30T13:47:05Z","receivedAt":"2020-03-30T13:47:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The git-update-ref(1) command can only handle queueing transactions\nright now via its \"--stdin\" parameter, but there is no way for users to\nhandle the transaction itself in a more explicit way. E.g. in a\nreplicated scenario, one may imagine a coordinator that spawns\ngit-update-ref(1) for multiple repositories and only if all agree that\nan update is possible will the coordinator send a commit. Such a\ntransactional session could look like\n\n > start\n < start: ok\n > update refs/heads/master $OLD $NEW\n > prepare\n < prepare: ok\n # All nodes have returned \"ok\"\n > commit\n < commit: ok\n\nor\n\n > start\n < start: ok\n > create refs/heads/master $OLD $NEW\n > prepare\n < fatal: cannot lock ref 'refs/heads/master': reference already exists\n # On all other nodes:\n > abort\n < abort: ok\n\nIn order to allow for such transactional sessions, this commit\nintroduces four new commands for git-update-ref(1), which matches those\nwe have internally already with the exception of \"start\":\n\n - start: start a new transaction\n\n - prepare: prepare the transaction, that is try to lock all\n references and verify their current value matches the\n expected one\n\n - commit: explicitly commit a session, that is update references to\n match their new expected state\n\n - abort: abort a session and roll back all changes\n\nBy design, git-update-ref(1) will commit as soon as standard input is\nbeing closed. While fine in a non-transactional world, it is definitely\nunexpected in a transactional world. Because of this, as soon as any of\nthe new transactional commands is used, the default will change to\naborting without an explicit \"commit\". To avoid a race between queueing\nupdates and the first \"prepare\" that starts a transaction, the \"start\"\ncommand has been added to start an explicit transaction.\n\nAdd some tests to exercise this new functionality.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/git-update-ref.txt | 26 ++++++\n builtin/update-ref.c | 106 +++++++++++++++++++++++--\n t/t1400-update-ref.sh | 131 +++++++++++++++++++++++++++++++\n 3 files changed, 255 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt\nindex 9bd039ce08..3e737c2360 100644\n--- a/Documentation/git-update-ref.txt\n+++ b/Documentation/git-update-ref.txt\n@@ -66,6 +66,10 @@ performs all modifications together. Specify commands of the form:\n \tdelete SP [SP ] LF\n \tverify SP [SP ] LF\n \toption SP LF\n+\tstart LF\n+\tprepare LF\n+\tcommit LF\n+\tabort LF\n \n With `--create-reflog`, update-ref will create a reflog for each ref\n even if one would not ordinarily be created.\n@@ -83,6 +87,10 @@ quoting:\n \tdelete SP NUL [] NUL\n \tverify SP NUL [] NUL\n \toption SP NUL\n+\tstart NUL\n+\tprepare NUL\n+\tcommit NUL\n+\tabort NUL\n \n In this format, use 40 \"0\" to specify a zero value, and use the empty\n string to specify a missing value.\n@@ -114,6 +122,24 @@ option::\n \tThe only valid option is `no-deref` to avoid dereferencing\n \ta symbolic ref.\n \n+start::\n+\tStart a transaction. In contrast to a non-transactional session, a\n+\ttransaction will automatically abort if the session ends without an\n+\texplicit commit.\n+\n+prepare::\n+\tPrepare to commit the transaction. This will create lock files for all\n+\tqueued reference updates. If one reference could not be locked, the\n+\ttransaction will be aborted.\n+\n+commit::\n+\tCommit all reference updates queued for the transaction, ending the\n+\ttransaction.\n+\n+abort::\n+\tAbort the transaction, releasing all locks if the transaction is in\n+\tprepared state.\n+\n If all s can be locked with matching s\n simultaneously, all modifications are performed. Otherwise, no\n modifications are performed. Note that while each individual\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 77cd235dfc..120c0a5fc0 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -326,6 +326,59 @@ static void parse_cmd_option(struct ref_transaction *transaction,\n \t\tdie(\"option unknown: %s\", next);\n }\n \n+static void parse_cmd_start(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n+{\n+\tif (*next != line_termination)\n+\t\tdie(\"start: extra input: %s\", next);\n+\tputs(\"start: ok\");\n+}\n+\n+static void parse_cmd_prepare(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n+{\n+\tstruct strbuf error = STRBUF_INIT;\n+\tif (*next != line_termination)\n+\t\tdie(\"prepare: extra input: %s\", next);\n+\tif (ref_transaction_prepare(transaction, &error))\n+\t\tdie(\"prepare: %s\", error.buf);\n+\tputs(\"prepare: ok\");\n+}\n+\n+static void parse_cmd_abort(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n+{\n+\tstruct strbuf error = STRBUF_INIT;\n+\tif (*next != line_termination)\n+\t\tdie(\"abort: extra input: %s\", next);\n+\tif (ref_transaction_abort(transaction, &error))\n+\t\tdie(\"abort: %s\", error.buf);\n+\tputs(\"abort: ok\");\n+}\n+\n+static void parse_cmd_commit(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n+{\n+\tstruct strbuf error = STRBUF_INIT;\n+\tif (*next != line_termination)\n+\t\tdie(\"commit: extra input: %s\", next);\n+\tif (ref_transaction_commit(transaction, &error))\n+\t\tdie(\"commit: %s\", error.buf);\n+\tputs(\"commit: ok\");\n+\tref_transaction_free(transaction);\n+}\n+\n+enum update_refs_state {\n+\t/* Non-transactional state open for updates. */\n+\tUPDATE_REFS_OPEN,\n+\t/* A transaction has been started. */\n+\tUPDATE_REFS_STARTED,\n+\t/* References are locked and ready for commit */\n+\tUPDATE_REFS_PREPARED,\n+\t/* Transaction has been committed or closed. */\n+\tUPDATE_REFS_CLOSED,\n+};\n+\n static const struct parse_cmd {\n \tconst char *prefix;\n \tvoid (*fn)(struct ref_transaction *, const char *, const char *);\n@@ -336,17 +389,23 @@ static const struct parse_cmd {\n \t * of lines.\n \t */\n \tunsigned extra_lines;\n+\tenum update_refs_state state;\n } command[] = {\n-\t{ \"update\", parse_cmd_update, 2 },\n-\t{ \"create\", parse_cmd_create, 1 },\n-\t{ \"delete\", parse_cmd_delete, 1 },\n-\t{ \"verify\", parse_cmd_verify, 1 },\n-\t{ \"option\", parse_cmd_option, 0 },\n+\t{ \"update\", parse_cmd_update, 2, UPDATE_REFS_OPEN },\n+\t{ \"create\", parse_cmd_create, 1, UPDATE_REFS_OPEN },\n+\t{ \"delete\", parse_cmd_delete, 1, UPDATE_REFS_OPEN },\n+\t{ \"verify\", parse_cmd_verify, 1, UPDATE_REFS_OPEN },\n+\t{ \"option\", parse_cmd_option, 0, UPDATE_REFS_OPEN },\n+\t{ \"start\", parse_cmd_start, 0, UPDATE_REFS_STARTED },\n+\t{ \"prepare\", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED },\n+\t{ \"abort\", parse_cmd_abort, 0, UPDATE_REFS_CLOSED },\n+\t{ \"commit\", parse_cmd_commit, 0, UPDATE_REFS_CLOSED },\n };\n \n static void update_refs_stdin(void)\n {\n \tstruct strbuf input = STRBUF_INIT, err = STRBUF_INIT;\n+\tenum update_refs_state state = UPDATE_REFS_OPEN;\n \tstruct ref_transaction *transaction;\n \tint i, j;\n \n@@ -393,14 +452,45 @@ static void update_refs_stdin(void)\n \t\t\tif (strbuf_appendwholeline(&input, stdin, line_termination))\n \t\t\t\tbreak;\n \n+\t\tswitch (state) {\n+\t\tcase UPDATE_REFS_OPEN:\n+\t\tcase UPDATE_REFS_STARTED:\n+\t\t\t/* Do not downgrade a transaction to a non-transaction. */\n+\t\t\tif (cmd->state >= state)\n+\t\t\t\tstate = cmd->state;\n+\t\t\tbreak;\n+\t\tcase UPDATE_REFS_PREPARED:\n+\t\t\tif (cmd->state != UPDATE_REFS_CLOSED)\n+\t\t\t\tdie(\"prepared transactions can only be closed\");\n+\t\t\tstate = cmd->state;\n+\t\t\tbreak;\n+\t\tcase UPDATE_REFS_CLOSED:\n+\t\t\tdie(\"transaction is closed\");\n+\t\t\tbreak;\n+\t\t}\n+\n \t\tcmd->fn(transaction, input.buf + strlen(cmd->prefix),\n \t\t\tinput.buf + input.len);\n \t}\n \n-\tif (ref_transaction_commit(transaction, &err))\n-\t\tdie(\"%s\", err.buf);\n+\tswitch (state) {\n+\tcase UPDATE_REFS_OPEN:\n+\t\t/* Commit by default if no transaction was requested. */\n+\t\tif (ref_transaction_commit(transaction, &err))\n+\t\t\tdie(\"%s\", err.buf);\n+\t\tref_transaction_free(transaction);\n+\t\tbreak;\n+\tcase UPDATE_REFS_STARTED:\n+\tcase UPDATE_REFS_PREPARED:\n+\t\t/* If using a transaction, we want to abort it. */\n+\t\tif (ref_transaction_abort(transaction, &err))\n+\t\t\tdie(\"%s\", err.buf);\n+\t\tbreak;\n+\tcase UPDATE_REFS_CLOSED:\n+\t\t/* Otherwise no need to do anything, the transaction was closed already. */\n+\t\tbreak;\n+\t}\n \n-\tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n \tstrbuf_release(&input);\n }\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex a6224ef65f..48d0d42afd 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -1404,4 +1404,135 @@ test_expect_success 'handle per-worktree refs in refs/bisect' '\n \t! test_cmp main-head worktree-head\n '\n \n+test_expect_success 'transaction handles empty commit' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tprepare\n+\tcommit\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start prepare commit >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction handles empty commit with missing prepare' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tcommit\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start commit >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction handles sole commit' '\n+\tcat >stdin <<-EOF &&\n+\tcommit\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" commit >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction handles empty abort' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tprepare\n+\tabort\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start prepare abort >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction exits on multiple aborts' '\n+\tcat >stdin <<-EOF &&\n+\tabort\n+\tabort\n+\tEOF\n+\ttest_must_fail git update-ref --stdin actual 2>err &&\n+\tprintf \"%s: ok\\n\" abort >expect &&\n+\ttest_cmp expect actual &&\n+\tgrep \"fatal: transaction is closed\" err\n+'\n+\n+test_expect_success 'transaction exits on start after prepare' '\n+\tcat >stdin <<-EOF &&\n+\tprepare\n+\tstart\n+\tEOF\n+\ttest_must_fail git update-ref --stdin err >actual &&\n+\tprintf \"%s: ok\\n\" prepare >expect &&\n+\ttest_cmp expect actual &&\n+\tgrep \"fatal: prepared transactions can only be closed\" err\n+'\n+\n+test_expect_success 'transaction handles empty abort with missing prepare' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tabort\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start abort >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction handles sole abort' '\n+\tcat >stdin <<-EOF &&\n+\tabort\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" abort >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction can handle commit' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tcreate $a HEAD\n+\tcommit\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start commit >expect &&\n+\ttest_cmp expect actual &&\n+\tgit rev-parse HEAD >expect &&\n+\tgit rev-parse $a >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction can handle abort' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tcreate $b HEAD\n+\tabort\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start abort >expect &&\n+\ttest_cmp expect actual &&\n+\ttest_path_is_missing .git/$b\n+'\n+\n+test_expect_success 'transaction aborts by default' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tcreate $b HEAD\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start >expect &&\n+\ttest_cmp expect actual &&\n+\ttest_path_is_missing .git/$b\n+'\n+\n+test_expect_success 'transaction with prepare aborts by default' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tcreate $b HEAD\n+\tprepare\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start prepare >expect &&\n+\ttest_cmp expect actual &&\n+\ttest_path_is_missing .git/$b\n+'\n+\n test_done\n-- \n2.26.0\n\n"},{"id":"394371","messageId":"xmqqwo7193zj.fsf@gitster.c.googlers.com","threadId":"53094","inReplyTo":"20200330080551.GA186019@ncase.pks.im","subject":"Re: [PATCH 4/9] update-ref: organize commands in an array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-30T16:55:44Z","receivedAt":"2020-03-30T16:55:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> \tfor (i = 0; i < ARRAY_SIZE(command); i++) {\n>> \t\tconst char *eoc;\n>> \t\tif (!skip_prefix(next, commands[i].prefix, &eoc) ||\n>> \t\t *eoc != ' ')\n>> \t\t\tcontinue;\n>> \t\tcmd = &command[i];\n>> next = eoc;\n>> \t\tbreak;\n>> \t}\n>\n> The reason why I moved those `skip_prefix` calls into each of the\n> respective commands is that this patch series introduces calls that do\n> not accept a trailing space at all. Thus we cannot handle the space\n> generically here, as that would was soon as we introduce the set of new\n> commands.\n\nThat's not a good excuse, though, is it? The command[] structure\ncan say \"this takes parameters\" or even \"this takes N parameters\",\nand the field being zero (i.e. \"does not take parameters\" or \"takes\nzero parameters\") would mean you do not want a trailing SP, no?\n\nI also suspect that the \"extra lines\" thing we'd see in a later step\nis correlated with this, but we'll see.\n\nThanks.\n"},{"id":"394381","messageId":"xmqqo8sd936v.fsf@gitster.c.googlers.com","threadId":"53094","inReplyTo":"20200330132509.GA1344694@ncase.pks.im","subject":"Re: [PATCH 3/9] strbuf: provide function to append whole lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-30T17:12:56Z","receivedAt":"2020-03-30T17:13:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Unfortunately it's not that easy for the HAVE_GETDELIM case, though, as\n> we cannot call getdelim(3P) to append to an already populated buffer. So\n> we'd have to call getdelim(3P) either on a NULL line and append manually\n> or on an empty line in case the strbuf doesn't have any contents. While\n> doable, I wanted to keep out this additional complexity for now.\n\nAhh, I see. Thanks for a clarification.\n"},{"id":"394383","messageId":"20200330173718.GA4837@ncase.pks.im","threadId":"53094","inReplyTo":"xmqqwo7193zj.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 4/9] update-ref: organize commands in an array","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-03-30T17:37:32Z","receivedAt":"2020-03-30T17:37:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Mar 30, 2020 at 09:55:44AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> >> \tfor (i = 0; i < ARRAY_SIZE(command); i++) {\n> >> \t\tconst char *eoc;\n> >> \t\tif (!skip_prefix(next, commands[i].prefix, &eoc) ||\n> >> \t\t *eoc != ' ')\n> >> \t\t\tcontinue;\n> >> \t\tcmd = &command[i];\n> >> next = eoc;\n> >> \t\tbreak;\n> >> \t}\n> >\n> > The reason why I moved those `skip_prefix` calls into each of the\n> > respective commands is that this patch series introduces calls that do\n> > not accept a trailing space at all. Thus we cannot handle the space\n> > generically here, as that would was soon as we introduce the set of new\n> > commands.\n> \n> That's not a good excuse, though, is it? The command[] structure\n> can say \"this takes parameters\" or even \"this takes N parameters\",\n> and the field being zero (i.e. \"does not take parameters\" or \"takes\n> zero parameters\") would mean you do not want a trailing SP, no?\n> \n> I also suspect that the \"extra lines\" thing we'd see in a later step\n> is correlated with this, but we'll see.\n> \n> Thanks.\n\nYou've got a point there. I'll convert this for the next version,\nthanks!\n\nPatrick\n"},{"id":"394384","messageId":"xmqqk13191yu.fsf@gitster.c.googlers.com","threadId":"53094","inReplyTo":"20200330081144.GB186019@ncase.pks.im","subject":"Re: [PATCH 8/9] update-ref: read commands in a line-wise fashion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-30T17:39:21Z","receivedAt":"2020-03-30T17:39:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Fri, Mar 27, 2020 at 02:58:38PM -0700, Junio C Hamano wrote:\n>> Patrick Steinhardt writes:\n>> \n>> > static const struct parse_cmd {\n>> > \tconst char *prefix;\n>> > -\tconst char *(*fn)(struct ref_transaction *, const char *, const char *);\n>> > +\tvoid (*fn)(struct ref_transaction *, const char *, const char *);\n>> > +\tunsigned extra_lines;\n>> \n>> Define and explain the meaning of extra_lines in a comment. Without\n>> it, the most natural way to infer what the variable means by readers\n>> would be that \"update\" command sometimes receives up to two more\n>> lines but it also is perfectly normal if there is no extra line.\n>> \n>> But I am not sure if that is what is going on.\n>> \n>> \"update\" _always_ needs exactly two more input \"lines\" when the\n>> input is NUL delimited, perhaps, and it is an _error_ if there are\n>> not these two lines, no?\n>\n> That's extactly right. I pondered on a good name, but I wasn't yet able\n> to come up with any one that brings across its meaning. I originally had\n> `extra_args` but changed it after some internal discussion with Chris.\n\nWouldn't it make more sense to store the number of args expected\nhere, not \"extra\"? The one that does not take any then can say \"I\nam subcommand X and I do not want a SP after my name\".\n\nThe \"sometimes after the command there is line.termination, and\nsometimes there is NUL, and yet some other times there is SP\", which\nis a sloppy way to check malformed input (i.e. you do not want SP\nfor a command that does not take parameters), that you have after\nthe loop can then go, if you did the parsing that way.\n\n"},{"id":"394545","messageId":"7a297db4daaee84dc90ddeaa8f77b68a39231ef1.1585811013.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585811013.git.ps@pks.im","subject":"[PATCH v3 1/9] refs: fix segfault when aborting empty transaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-04-02T07:09:22Z","receivedAt":"2020-04-02T07:09:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When cleaning up a transaction that has no updates queued, then the\ntransaction's backend data will not have been allocated. We correctly\nhandle this for the packed backend, where the cleanup function checks\nwhether the backend data has been allocated at all -- if not, then there\nis nothing to clean up. For the files backend we do not check this and\nas a result will hit a segfault due to dereferencing a `NULL` pointer\nwhen cleaning up such a transaction.\n\nFix the issue by checking whether `backend_data` is set in the files\nbackend, too.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/files-backend.c | 20 +++++++++++---------\n 1 file changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 561c33ac8a..6516c7bc8c 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2565,17 +2565,19 @@ static void files_transaction_cleanup(struct files_ref_store *refs,\n \t\t}\n \t}\n \n-\tif (backend_data->packed_transaction &&\n-\t ref_transaction_abort(backend_data->packed_transaction, &err)) {\n-\t\terror(\"error aborting transaction: %s\", err.buf);\n-\t\tstrbuf_release(&err);\n+\tif (backend_data) {\n+\t\tif (backend_data->packed_transaction &&\n+\t\t ref_transaction_abort(backend_data->packed_transaction, &err)) {\n+\t\t\terror(\"error aborting transaction: %s\", err.buf);\n+\t\t\tstrbuf_release(&err);\n+\t\t}\n+\n+\t\tif (backend_data->packed_refs_locked)\n+\t\t\tpacked_refs_unlock(refs->packed_ref_store);\n+\n+\t\tfree(backend_data);\n \t}\n \n-\tif (backend_data->packed_refs_locked)\n-\t\tpacked_refs_unlock(refs->packed_ref_store);\n-\n-\tfree(backend_data);\n-\n \ttransaction->state = REF_TRANSACTION_CLOSED;\n }\n \n-- \n2.26.0\n\n"},{"id":"394546","messageId":"15857e1b8ca800a92fa96fee818a54456bfbd330.1585811013.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585811013.git.ps@pks.im","subject":"[PATCH v3 2/9] git-update-ref.txt: add missing word","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-04-02T07:09:27Z","receivedAt":"2020-04-02T07:09:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The description for the \"verify\" command is lacking a single word \"is\",\nwhich this commit corrects.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/git-update-ref.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt\nindex 9671423117..9bd039ce08 100644\n--- a/Documentation/git-update-ref.txt\n+++ b/Documentation/git-update-ref.txt\n@@ -107,7 +107,7 @@ delete::\n \n verify::\n \tVerify against but do not change it. If\n-\t zero or missing, the ref must not exist.\n+\t is zero or missing, the ref must not exist.\n \n option::\n \tModify behavior of the next command naming a .\n-- \n2.26.0\n\n"},{"id":"394547","messageId":"b6546ae44e6a5f7b57d75d59b23a777b363cac1e.1585811013.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585811013.git.ps@pks.im","subject":"[PATCH v3 3/9] strbuf: provide function to append whole lines","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-04-02T07:09:33Z","receivedAt":"2020-04-02T07:09:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While the strbuf interface already provides functions to read a line\ninto it that completely replaces its current contents, we do not have an\ninterface that allows for appending lines without discarding current\ncontents.\n\nAdd a new function `strbuf_appendwholeline` that reads a line including\nits terminating character into a strbuf non-destructively. This is a\npreparatory step for git-update-ref(1) reading standard input line-wise\ninstead of as a block.\n\nSigned-off-by: Patrick Steinhardt \n---\n strbuf.c | 10 ++++++++++\n strbuf.h | 6 ++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex bb0065ccaf..6e74901bfa 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -690,6 +690,16 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n }\n #endif\n \n+int strbuf_appendwholeline(struct strbuf *sb, FILE *fp, int term)\n+{\n+\tstruct strbuf line = STRBUF_INIT;\n+\tif (strbuf_getwholeline(&line, fp, term))\n+\t\treturn EOF;\n+\tstrbuf_addbuf(sb, &line);\n+\tstrbuf_release(&line);\n+\treturn 0;\n+}\n+\n static int strbuf_getdelim(struct strbuf *sb, FILE *fp, int term)\n {\n \tif (strbuf_getwholeline(sb, fp, term))\ndiff --git a/strbuf.h b/strbuf.h\nindex ce8e49c0b2..411063ca76 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -502,6 +502,12 @@ int strbuf_getline(struct strbuf *sb, FILE *file);\n */\n int strbuf_getwholeline(struct strbuf *sb, FILE *file, int term);\n \n+/**\n+ * Like `strbuf_getwholeline`, but appends the line instead of\n+ * resetting the buffer first.\n+ */\n+int strbuf_appendwholeline(struct strbuf *sb, FILE *file, int term);\n+\n /**\n * Like `strbuf_getwholeline`, but operates on a file descriptor.\n * It reads one character at a time, so it is very slow. Do not\n-- \n2.26.0\n\n"},{"id":"394548","messageId":"cover.1585811013.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585129842.git.ps@pks.im","subject":"[PATCH v3 0/9] Support for transactions in `git-update-ref --stdin`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-04-02T07:09:15Z","receivedAt":"2020-04-02T07:09:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of this patch series to introduce transaction\nsupport in `git update-ref --stdin`. The goal is to make commands\navailable to the user of that command so that he can explicitly control\nthe given transaction by starting, preparing, and finally committing or\naborting the transaction.\n\nThis version improves the argument handling for commands as proposed by\nJunio. Instead of the hard-to-understand `extra_lines` field, commands\nnow have a `args` field that specifies how many arguments a given\ncommand has, including the first one separated by a space. As a result,\nthe initial space for the first argument is now handled in the generic\npart and not moved into each of the command functions anymore. I've also\nadded a comment that clarifies why we ignore EOF when fetching\nadditional command lines.\n\nThe range-diff against v2 is attached below.\n\nPatrick\n\nPatrick Steinhardt (9):\n refs: fix segfault when aborting empty transaction\n git-update-ref.txt: add missing word\n strbuf: provide function to append whole lines\n update-ref: organize commands in an array\n update-ref: drop unused argument for `parse_refname`\n update-ref: pass end pointer instead of strbuf\n update-ref: move transaction handling into `update_refs_stdin()`\n update-ref: read commands in a line-wise fashion\n update-ref: implement interactive transaction handling\n\n Documentation/git-update-ref.txt | 28 +++-\n builtin/update-ref.c | 245 ++++++++++++++++++++++---------\n refs/files-backend.c | 20 +--\n strbuf.c | 10 ++\n strbuf.h | 6 +\n t/t1400-update-ref.sh | 131 +++++++++++++++++\n 6 files changed, 364 insertions(+), 76 deletions(-)\n\nRange-diff against v2:\n 1: 7a297db4da = 1: 7a297db4da refs: fix segfault when aborting empty transaction\n 2: 15857e1b8c = 2: 15857e1b8c git-update-ref.txt: add missing word\n 3: b6546ae44e = 3: b6546ae44e strbuf: provide function to append whole lines\n 4: bd8c059fbc ! 4: 0f881d4936 update-ref: organize commands in an array\n @@ Commit message\n Signed-off-by: Patrick Steinhardt \n \n ## builtin/update-ref.c ##\n -@@ builtin/update-ref.c: static int parse_next_oid(struct strbuf *input, const char **next,\n - /*\n - * The following five parse_cmd_*() functions parse the corresponding\n - * command. In each case, next points at the character following the\n -- * command name and the following space. They each return a pointer\n -- * to the character terminating the command, and die with an\n -- * explanatory message if there are any parsing problems. All of\n -- * these functions handle either text or binary format input,\n -- * depending on how line_termination is set.\n -+ * command name. They each return a pointer to the character\n -+ * terminating the command, and die with an explanatory message if\n -+ * there are any parsing problems. All of these functions handle\n -+ * either text or binary format input, depending on how\n -+ * line_termination is set.\n - */\n - \n - static const char *parse_cmd_update(struct ref_transaction *transaction,\n -@@ builtin/update-ref.c: static const char *parse_cmd_update(struct ref_transaction *transaction,\n - \tstruct object_id new_oid, old_oid;\n - \tint have_old;\n - \n -+\tif (!skip_prefix(next, \" \", &next))\n -+\t\tdie(\"update: missing space after command\");\n -+\n - \trefname = parse_refname(input, &next);\n - \tif (!refname)\n - \t\tdie(\"update: missing \");\n -@@ builtin/update-ref.c: static const char *parse_cmd_create(struct ref_transaction *transaction,\n - \tchar *refname;\n - \tstruct object_id new_oid;\n - \n -+\tif (!skip_prefix(next, \" \", &next))\n -+\t\tdie(\"create: missing space after command\");\n -+\n - \trefname = parse_refname(input, &next);\n - \tif (!refname)\n - \t\tdie(\"create: missing \");\n -@@ builtin/update-ref.c: static const char *parse_cmd_delete(struct ref_transaction *transaction,\n - \tstruct object_id old_oid;\n - \tint have_old;\n - \n -+\tif (!skip_prefix(next, \" \", &next))\n -+\t\tdie(\"delete: missing space after command\");\n -+\n - \trefname = parse_refname(input, &next);\n - \tif (!refname)\n - \t\tdie(\"delete: missing \");\n -@@ builtin/update-ref.c: static const char *parse_cmd_verify(struct ref_transaction *transaction,\n - \tchar *refname;\n - \tstruct object_id old_oid;\n - \n -+\tif (!skip_prefix(next, \" \", &next))\n -+\t\tdie(\"verify: missing space after command\");\n -+\n - \trefname = parse_refname(input, &next);\n - \tif (!refname)\n - \t\tdie(\"verify: missing \");\n @@ builtin/update-ref.c: static const char *parse_cmd_verify(struct ref_transaction *transaction,\n \treturn next;\n }\n @@ builtin/update-ref.c: static const char *parse_cmd_verify(struct ref_transaction\n +\t\t\t\t struct strbuf *input, const char *next)\n {\n \tconst char *rest;\n -+\tif (!skip_prefix(next, \" \", &next))\n -+\t\tdie(\"option: missing space after command\");\n \tif (skip_prefix(next, \"no-deref\", &rest) && *rest == line_termination)\n - \t\tupdate_flags |= REF_NO_DEREF;\n - \telse\n @@ builtin/update-ref.c: static const char *parse_cmd_option(struct strbuf *input, const char *next)\n \treturn rest;\n }\n @@ builtin/update-ref.c: static const char *parse_cmd_option(struct strbuf *input,\n +\t\tfor (i = 0; i < ARRAY_SIZE(command); i++) {\n +\t\t\tconst char *prefix = command[i].prefix;\n +\n -+\t\t\tif (!skip_prefix(next, prefix, &next))\n -+\t\t\t\tcontinue;\n -+\n -+\t\t\t/*\n -+\t\t\t * Check that the command is terminated by an argument\n -+\t\t\t * or line terminator and not only a matching prefix.\n -+\t\t\t */\n -+\t\t\tif (input.buf[strlen(prefix)] != line_termination &&\n -+\t\t\t input.buf[strlen(prefix)] != '\\0' &&\n -+\t\t\t input.buf[strlen(prefix)] != ' ')\n ++\t\t\tif (!skip_prefix(next, prefix, &next) ||\n ++\t\t\t !skip_prefix(next, \" \", &next))\n +\t\t\t\tcontinue;\n +\n +\t\t\tcmd = &command[i];\n 5: 49a14d2046 ! 5: c96395a14d update-ref: drop unused argument for `parse_refname`\n @@ builtin/update-ref.c: static const char *parse_arg(const char *next, struct strb\n \tstruct strbuf ref = STRBUF_INIT;\n \n @@ builtin/update-ref.c: static const char *parse_cmd_update(struct ref_transaction *transaction,\n - \tif (!skip_prefix(next, \" \", &next))\n - \t\tdie(\"update: missing space after command\");\n + \tstruct object_id new_oid, old_oid;\n + \tint have_old;\n \n -\trefname = parse_refname(input, &next);\n +\trefname = parse_refname(&next);\n @@ builtin/update-ref.c: static const char *parse_cmd_update(struct ref_transaction\n \t\tdie(\"update: missing \");\n \n @@ builtin/update-ref.c: static const char *parse_cmd_create(struct ref_transaction *transaction,\n - \tif (!skip_prefix(next, \" \", &next))\n - \t\tdie(\"create: missing space after command\");\n + \tchar *refname;\n + \tstruct object_id new_oid;\n \n -\trefname = parse_refname(input, &next);\n +\trefname = parse_refname(&next);\n @@ builtin/update-ref.c: static const char *parse_cmd_create(struct ref_transaction\n \t\tdie(\"create: missing \");\n \n @@ builtin/update-ref.c: static const char *parse_cmd_delete(struct ref_transaction *transaction,\n - \tif (!skip_prefix(next, \" \", &next))\n - \t\tdie(\"delete: missing space after command\");\n + \tstruct object_id old_oid;\n + \tint have_old;\n \n -\trefname = parse_refname(input, &next);\n +\trefname = parse_refname(&next);\n @@ builtin/update-ref.c: static const char *parse_cmd_delete(struct ref_transaction\n \t\tdie(\"delete: missing \");\n \n @@ builtin/update-ref.c: static const char *parse_cmd_verify(struct ref_transaction *transaction,\n - \tif (!skip_prefix(next, \" \", &next))\n - \t\tdie(\"verify: missing space after command\");\n + \tchar *refname;\n + \tstruct object_id old_oid;\n \n -\trefname = parse_refname(input, &next);\n +\trefname = parse_refname(&next);\n 6: cbe430029d ! 6: 192cbf5944 update-ref: pass end pointer instead of strbuf\n @@ builtin/update-ref.c: static const char *parse_cmd_verify(struct ref_transaction\n +\t\t\t\t const char *next, const char *end)\n {\n \tconst char *rest;\n - \tif (!skip_prefix(next, \" \", &next))\n + \tif (skip_prefix(next, \"no-deref\", &rest) && *rest == line_termination)\n @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction *transaction,\n \n static const struct parse_cmd {\n 7: d2f68f59a7 = 7: be7bcf3dbd update-ref: move transaction handling into `update_refs_stdin()`\n 8: f8786fdeb3 ! 8: 02ff6b7337 update-ref: read commands in a line-wise fashion\n @@ Commit message\n \n ## builtin/update-ref.c ##\n @@ builtin/update-ref.c: static int parse_next_oid(const char **next, const char *end,\n - * line_termination is set.\n + * depending on how line_termination is set.\n */\n \n -static const char *parse_cmd_update(struct ref_transaction *transaction,\n @@ builtin/update-ref.c: static const char *parse_cmd_verify(struct ref_transaction\n +\t\t\t const char *next, const char *end)\n {\n \tconst char *rest;\n - \tif (!skip_prefix(next, \" \", &next))\n -@@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction *transaction,\n + \tif (skip_prefix(next, \"no-deref\", &rest) && *rest == line_termination)\n \t\tupdate_flags |= REF_NO_DEREF;\n \telse\n \t\tdie(\"option unknown: %s\", next);\n @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction\n \tconst char *prefix;\n -\tconst char *(*fn)(struct ref_transaction *, const char *, const char *);\n +\tvoid (*fn)(struct ref_transaction *, const char *, const char *);\n -+\t/*\n -+\t * If using NUL-terminated format, only the first argument will be\n -+\t * available in the first line. In case a command expects more than one\n -+\t * argument, we thus have to fetch an additional `extra_lines` number\n -+\t * of lines.\n -+\t */\n -+\tunsigned extra_lines;\n ++\tunsigned args;\n } command[] = {\n -\t{ \"update\", parse_cmd_update },\n -\t{ \"create\", parse_cmd_create },\n -\t{ \"delete\", parse_cmd_delete },\n -\t{ \"verify\", parse_cmd_verify },\n -\t{ \"option\", parse_cmd_option },\n -+\t{ \"update\", parse_cmd_update, 2 },\n -+\t{ \"create\", parse_cmd_create, 1 },\n -+\t{ \"delete\", parse_cmd_delete, 1 },\n -+\t{ \"verify\", parse_cmd_verify, 1 },\n -+\t{ \"option\", parse_cmd_option, 0 },\n ++\t{ \"update\", parse_cmd_update, 3 },\n ++\t{ \"create\", parse_cmd_create, 2 },\n ++\t{ \"delete\", parse_cmd_delete, 2 },\n ++\t{ \"verify\", parse_cmd_verify, 2 },\n ++\t{ \"option\", parse_cmd_option, 1 },\n };\n \n static void update_refs_stdin(void)\n @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction\n \n \t\tfor (i = 0; i < ARRAY_SIZE(command); i++) {\n \t\t\tconst char *prefix = command[i].prefix;\n ++\t\t\tchar c;\n \n --\t\t\tif (!skip_prefix(next, prefix, &next))\n +-\t\t\tif (!skip_prefix(next, prefix, &next) ||\n +-\t\t\t !skip_prefix(next, \" \", &next))\n +\t\t\tif (!starts_with(input.buf, prefix))\n ++\t\t\t\tcontinue;\n ++\n ++\t\t\t/*\n ++\t\t\t * If the command has arguments, verify that it's\n ++\t\t\t * followed by a space. Otherwise, it shall be followed\n ++\t\t\t * by a line terminator.\n ++\t\t\t */\n ++\t\t\tc = command[i].args ? ' ' : line_termination;\n ++\t\t\tif (input.buf[strlen(prefix)] != c)\n \t\t\t\tcontinue;\n \n - \t\t\t/*\n -@@ builtin/update-ref.c: static void update_refs_stdin(void)\n + \t\t\tcmd = &command[i];\n \t\t\tbreak;\n \t\t}\n \t\tif (!cmd)\n @@ builtin/update-ref.c: static void update_refs_stdin(void)\n -\t\tnext = cmd->fn(transaction, next, input.buf + input.len);\n -\t\tnext++;\n +\t\t/*\n -+\t\t * Read extra lines if NUL-terminated. Do not raise an error in\n -+\t\t * case there is an early EOF to let the command handle missing\n -+\t\t * arguments with a proper error message.\n ++\t\t * Read additional arguments if NUL-terminated. Do not raise an\n ++\t\t * error in case there is an early EOF to let the command\n ++\t\t * handle missing arguments with a proper error message.\n +\t\t */\n -+\t\tfor (j = 0; line_termination == '\\0' && j < cmd->extra_lines; j++)\n ++\t\tfor (j = 1; line_termination == '\\0' && j < cmd->args; j++)\n +\t\t\tif (strbuf_appendwholeline(&input, stdin, line_termination))\n +\t\t\t\tbreak;\n +\n -+\t\tcmd->fn(transaction, input.buf + strlen(cmd->prefix),\n ++\t\tcmd->fn(transaction, input.buf + strlen(cmd->prefix) + !!cmd->args,\n +\t\t\tinput.buf + input.len);\n \t}\n \n 9: c3fffdf9fa ! 9: 5670bea2b1 update-ref: implement interactive transaction handling\n @@ builtin/update-ref.c: static void parse_cmd_option(struct ref_transaction *trans\n static const struct parse_cmd {\n \tconst char *prefix;\n \tvoid (*fn)(struct ref_transaction *, const char *, const char *);\n -@@ builtin/update-ref.c: static const struct parse_cmd {\n - \t * of lines.\n - \t */\n - \tunsigned extra_lines;\n + \tunsigned args;\n +\tenum update_refs_state state;\n } command[] = {\n --\t{ \"update\", parse_cmd_update, 2 },\n --\t{ \"create\", parse_cmd_create, 1 },\n --\t{ \"delete\", parse_cmd_delete, 1 },\n --\t{ \"verify\", parse_cmd_verify, 1 },\n --\t{ \"option\", parse_cmd_option, 0 },\n -+\t{ \"update\", parse_cmd_update, 2, UPDATE_REFS_OPEN },\n -+\t{ \"create\", parse_cmd_create, 1, UPDATE_REFS_OPEN },\n -+\t{ \"delete\", parse_cmd_delete, 1, UPDATE_REFS_OPEN },\n -+\t{ \"verify\", parse_cmd_verify, 1, UPDATE_REFS_OPEN },\n -+\t{ \"option\", parse_cmd_option, 0, UPDATE_REFS_OPEN },\n +-\t{ \"update\", parse_cmd_update, 3 },\n +-\t{ \"create\", parse_cmd_create, 2 },\n +-\t{ \"delete\", parse_cmd_delete, 2 },\n +-\t{ \"verify\", parse_cmd_verify, 2 },\n +-\t{ \"option\", parse_cmd_option, 1 },\n ++\t{ \"update\", parse_cmd_update, 3, UPDATE_REFS_OPEN },\n ++\t{ \"create\", parse_cmd_create, 2, UPDATE_REFS_OPEN },\n ++\t{ \"delete\", parse_cmd_delete, 2, UPDATE_REFS_OPEN },\n ++\t{ \"verify\", parse_cmd_verify, 2, UPDATE_REFS_OPEN },\n ++\t{ \"option\", parse_cmd_option, 1, UPDATE_REFS_OPEN },\n +\t{ \"start\", parse_cmd_start, 0, UPDATE_REFS_STARTED },\n +\t{ \"prepare\", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED },\n +\t{ \"abort\", parse_cmd_abort, 0, UPDATE_REFS_CLOSED },\n @@ builtin/update-ref.c: static void update_refs_stdin(void)\n +\t\t\tbreak;\n +\t\t}\n +\n - \t\tcmd->fn(transaction, input.buf + strlen(cmd->prefix),\n + \t\tcmd->fn(transaction, input.buf + strlen(cmd->prefix) + !!cmd->args,\n \t\t\tinput.buf + input.len);\n \t}\n \n-- \n2.26.0\n\n"},{"id":"394549","messageId":"0f881d49366fe9d9806b08af35319475ca07abc3.1585811013.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585811013.git.ps@pks.im","subject":"[PATCH v3 4/9] update-ref: organize commands in an array","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-04-02T07:09:38Z","receivedAt":"2020-04-02T07:09:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We currently manually wire up all commands known to `git-update-ref\n--stdin`, making it harder than necessary to preprocess arguments after\nthe command is determined. To make this more extensible, let's refactor\nthe code to use an array of known commands instead. While this doesn't\nadd a lot of value now, it is a preparatory step to implement line-wise\nreading of commands.\n\nAs we're going to introduce commands without trailing spaces, this\ncommit also moves whitespace parsing into the respective commands.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/update-ref.c | 41 +++++++++++++++++++++++++++++------------\n 1 file changed, 29 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 2d8f7f0578..a6ff88b95b 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -310,7 +310,8 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n \treturn next;\n }\n \n-static const char *parse_cmd_option(struct strbuf *input, const char *next)\n+static const char *parse_cmd_option(struct ref_transaction *transaction,\n+\t\t\t\t struct strbuf *input, const char *next)\n {\n \tconst char *rest;\n \tif (skip_prefix(next, \"no-deref\", &rest) && *rest == line_termination)\n@@ -320,33 +321,49 @@ static const char *parse_cmd_option(struct strbuf *input, const char *next)\n \treturn rest;\n }\n \n+static const struct parse_cmd {\n+\tconst char *prefix;\n+\tconst char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);\n+} command[] = {\n+\t{ \"update\", parse_cmd_update },\n+\t{ \"create\", parse_cmd_create },\n+\t{ \"delete\", parse_cmd_delete },\n+\t{ \"verify\", parse_cmd_verify },\n+\t{ \"option\", parse_cmd_option },\n+};\n+\n static void update_refs_stdin(struct ref_transaction *transaction)\n {\n \tstruct strbuf input = STRBUF_INIT;\n \tconst char *next;\n+\tint i;\n \n \tif (strbuf_read(&input, 0, 1000) < 0)\n \t\tdie_errno(\"could not read from stdin\");\n \tnext = input.buf;\n \t/* Read each line dispatch its command */\n \twhile (next < input.buf + input.len) {\n+\t\tconst struct parse_cmd *cmd = NULL;\n+\n \t\tif (*next == line_termination)\n \t\t\tdie(\"empty command in input\");\n \t\telse if (isspace(*next))\n \t\t\tdie(\"whitespace before command: %s\", next);\n-\t\telse if (skip_prefix(next, \"update \", &next))\n-\t\t\tnext = parse_cmd_update(transaction, &input, next);\n-\t\telse if (skip_prefix(next, \"create \", &next))\n-\t\t\tnext = parse_cmd_create(transaction, &input, next);\n-\t\telse if (skip_prefix(next, \"delete \", &next))\n-\t\t\tnext = parse_cmd_delete(transaction, &input, next);\n-\t\telse if (skip_prefix(next, \"verify \", &next))\n-\t\t\tnext = parse_cmd_verify(transaction, &input, next);\n-\t\telse if (skip_prefix(next, \"option \", &next))\n-\t\t\tnext = parse_cmd_option(&input, next);\n-\t\telse\n+\n+\t\tfor (i = 0; i < ARRAY_SIZE(command); i++) {\n+\t\t\tconst char *prefix = command[i].prefix;\n+\n+\t\t\tif (!skip_prefix(next, prefix, &next) ||\n+\t\t\t !skip_prefix(next, \" \", &next))\n+\t\t\t\tcontinue;\n+\n+\t\t\tcmd = &command[i];\n+\t\t\tbreak;\n+\t\t}\n+\t\tif (!cmd)\n \t\t\tdie(\"unknown command: %s\", next);\n \n+\t\tnext = cmd->fn(transaction, &input, next);\n \t\tnext++;\n \t}\n \n-- \n2.26.0\n\n"},{"id":"394550","messageId":"c96395a14de1f7fd73e04eb035b58486f737b310.1585811013.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585811013.git.ps@pks.im","subject":"[PATCH v3 5/9] update-ref: drop unused argument for `parse_refname`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-04-02T07:09:43Z","receivedAt":"2020-04-02T07:09:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `parse_refname` function accepts a `struct strbuf *input` argument\nthat isn't used at all. As we're about to convert commands to not use a\nstrbuf anymore but instead an end pointer, let's drop this argument now\nto make the converting commit easier to review.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/update-ref.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex a6ff88b95b..1bba5ea6c2 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -50,7 +50,7 @@ static const char *parse_arg(const char *next, struct strbuf *arg)\n * the argument. Die if C-quoting is malformed or the reference name\n * is invalid.\n */\n-static char *parse_refname(struct strbuf *input, const char **next)\n+static char *parse_refname(const char **next)\n {\n \tstruct strbuf ref = STRBUF_INIT;\n \n@@ -186,7 +186,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n \tstruct object_id new_oid, old_oid;\n \tint have_old;\n \n-\trefname = parse_refname(input, &next);\n+\trefname = parse_refname(&next);\n \tif (!refname)\n \t\tdie(\"update: missing \");\n \n@@ -220,7 +220,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,\n \tchar *refname;\n \tstruct object_id new_oid;\n \n-\trefname = parse_refname(input, &next);\n+\trefname = parse_refname(&next);\n \tif (!refname)\n \t\tdie(\"create: missing \");\n \n@@ -253,7 +253,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,\n \tstruct object_id old_oid;\n \tint have_old;\n \n-\trefname = parse_refname(input, &next);\n+\trefname = parse_refname(&next);\n \tif (!refname)\n \t\tdie(\"delete: missing \");\n \n@@ -288,7 +288,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n \tchar *refname;\n \tstruct object_id old_oid;\n \n-\trefname = parse_refname(input, &next);\n+\trefname = parse_refname(&next);\n \tif (!refname)\n \t\tdie(\"verify: missing \");\n \n-- \n2.26.0\n\n"},{"id":"394551","messageId":"192cbf59446e5d0dbd842008d0d43581b85dfdbe.1585811013.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585811013.git.ps@pks.im","subject":"[PATCH v3 6/9] update-ref: pass end pointer instead of strbuf","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-04-02T07:09:48Z","receivedAt":"2020-04-02T07:09:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We currently pass both an `strbuf` containing the current command line\nas well as the `next` pointer pointing to the first argument to\ncommands. This is both confusing and makes code more intertwined.\nConvert this to use a simple pointer as well as a pointer pointing to\nthe end of the input as a preparatory step to line-wise reading of\nstdin.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/update-ref.c | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 1bba5ea6c2..381d347fb4 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -95,7 +95,7 @@ static char *parse_refname(const char **next)\n * provided but cannot be converted to a SHA-1, die. flags can\n * include PARSE_SHA1_OLD and/or PARSE_SHA1_ALLOW_EMPTY.\n */\n-static int parse_next_oid(struct strbuf *input, const char **next,\n+static int parse_next_oid(const char **next, const char *end,\n \t\t\t struct object_id *oid,\n \t\t\t const char *command, const char *refname,\n \t\t\t int flags)\n@@ -103,7 +103,7 @@ static int parse_next_oid(struct strbuf *input, const char **next,\n \tstruct strbuf arg = STRBUF_INIT;\n \tint ret = 0;\n \n-\tif (*next == input->buf + input->len)\n+\tif (*next == end)\n \t\tgoto eof;\n \n \tif (line_termination) {\n@@ -128,7 +128,7 @@ static int parse_next_oid(struct strbuf *input, const char **next,\n \t\t\tdie(\"%s %s: expected NUL but got: %s\",\n \t\t\t command, refname, *next);\n \t\t(*next)++;\n-\t\tif (*next == input->buf + input->len)\n+\t\tif (*next == end)\n \t\t\tgoto eof;\n \t\tstrbuf_addstr(&arg, *next);\n \t\t*next += arg.len;\n@@ -179,7 +179,7 @@ static int parse_next_oid(struct strbuf *input, const char **next,\n */\n \n static const char *parse_cmd_update(struct ref_transaction *transaction,\n-\t\t\t\t struct strbuf *input, const char *next)\n+\t\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -190,11 +190,11 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n \tif (!refname)\n \t\tdie(\"update: missing \");\n \n-\tif (parse_next_oid(input, &next, &new_oid, \"update\", refname,\n+\tif (parse_next_oid(&next, end, &new_oid, \"update\", refname,\n \t\t\t PARSE_SHA1_ALLOW_EMPTY))\n \t\tdie(\"update %s: missing \", refname);\n \n-\thave_old = !parse_next_oid(input, &next, &old_oid, \"update\", refname,\n+\thave_old = !parse_next_oid(&next, end, &old_oid, \"update\", refname,\n \t\t\t\t PARSE_SHA1_OLD);\n \n \tif (*next != line_termination)\n@@ -214,7 +214,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n }\n \n static const char *parse_cmd_create(struct ref_transaction *transaction,\n-\t\t\t\t struct strbuf *input, const char *next)\n+\t\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -224,7 +224,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,\n \tif (!refname)\n \t\tdie(\"create: missing \");\n \n-\tif (parse_next_oid(input, &next, &new_oid, \"create\", refname, 0))\n+\tif (parse_next_oid(&next, end, &new_oid, \"create\", refname, 0))\n \t\tdie(\"create %s: missing \", refname);\n \n \tif (is_null_oid(&new_oid))\n@@ -246,7 +246,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,\n }\n \n static const char *parse_cmd_delete(struct ref_transaction *transaction,\n-\t\t\t\t struct strbuf *input, const char *next)\n+\t\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -257,7 +257,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,\n \tif (!refname)\n \t\tdie(\"delete: missing \");\n \n-\tif (parse_next_oid(input, &next, &old_oid, \"delete\", refname,\n+\tif (parse_next_oid(&next, end, &old_oid, \"delete\", refname,\n \t\t\t PARSE_SHA1_OLD)) {\n \t\thave_old = 0;\n \t} else {\n@@ -282,7 +282,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,\n }\n \n static const char *parse_cmd_verify(struct ref_transaction *transaction,\n-\t\t\t\t struct strbuf *input, const char *next)\n+\t\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -292,7 +292,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n \tif (!refname)\n \t\tdie(\"verify: missing \");\n \n-\tif (parse_next_oid(input, &next, &old_oid, \"verify\", refname,\n+\tif (parse_next_oid(&next, end, &old_oid, \"verify\", refname,\n \t\t\t PARSE_SHA1_OLD))\n \t\toidclr(&old_oid);\n \n@@ -311,7 +311,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n }\n \n static const char *parse_cmd_option(struct ref_transaction *transaction,\n-\t\t\t\t struct strbuf *input, const char *next)\n+\t\t\t\t const char *next, const char *end)\n {\n \tconst char *rest;\n \tif (skip_prefix(next, \"no-deref\", &rest) && *rest == line_termination)\n@@ -323,7 +323,7 @@ static const char *parse_cmd_option(struct ref_transaction *transaction,\n \n static const struct parse_cmd {\n \tconst char *prefix;\n-\tconst char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);\n+\tconst char *(*fn)(struct ref_transaction *, const char *, const char *);\n } command[] = {\n \t{ \"update\", parse_cmd_update },\n \t{ \"create\", parse_cmd_create },\n@@ -363,7 +363,7 @@ static void update_refs_stdin(struct ref_transaction *transaction)\n \t\tif (!cmd)\n \t\t\tdie(\"unknown command: %s\", next);\n \n-\t\tnext = cmd->fn(transaction, &input, next);\n+\t\tnext = cmd->fn(transaction, next, input.buf + input.len);\n \t\tnext++;\n \t}\n \n-- \n2.26.0\n\n"},{"id":"394552","messageId":"be7bcf3dbd3e2c4b58c6c8d6a335a6ad7373de07.1585811013.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585811013.git.ps@pks.im","subject":"[PATCH v3 7/9] update-ref: move transaction handling into `update_refs_stdin()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-04-02T07:09:53Z","receivedAt":"2020-04-02T07:09:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While the actual logic to update the transaction is handled in\n`update_refs_stdin()`, the transaction itself is started and committed\nin `cmd_update_ref()` itself. This makes it hard to handle transaction\nabortion and commits as part of `update_refs_stdin()` itself, which is\nrequired in order to introduce transaction handling features to `git\nupdate-refs --stdin`.\n\nRefactor the code to move all transaction handling into\n`update_refs_stdin()` to prepare for transaction handling features.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/update-ref.c | 27 ++++++++++++++-------------\n 1 file changed, 14 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 381d347fb4..0f34b68904 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -332,15 +332,21 @@ static const struct parse_cmd {\n \t{ \"option\", parse_cmd_option },\n };\n \n-static void update_refs_stdin(struct ref_transaction *transaction)\n+static void update_refs_stdin(void)\n {\n-\tstruct strbuf input = STRBUF_INIT;\n+\tstruct strbuf input = STRBUF_INIT, err = STRBUF_INIT;\n+\tstruct ref_transaction *transaction;\n \tconst char *next;\n \tint i;\n \n \tif (strbuf_read(&input, 0, 1000) < 0)\n \t\tdie_errno(\"could not read from stdin\");\n \tnext = input.buf;\n+\n+\ttransaction = ref_transaction_begin(&err);\n+\tif (!transaction)\n+\t\tdie(\"%s\", err.buf);\n+\n \t/* Read each line dispatch its command */\n \twhile (next < input.buf + input.len) {\n \t\tconst struct parse_cmd *cmd = NULL;\n@@ -367,6 +373,11 @@ static void update_refs_stdin(struct ref_transaction *transaction)\n \t\tnext++;\n \t}\n \n+\tif (ref_transaction_commit(transaction, &err))\n+\t\tdie(\"%s\", err.buf);\n+\n+\tref_transaction_free(transaction);\n+\tstrbuf_release(&err);\n \tstrbuf_release(&input);\n }\n \n@@ -401,21 +412,11 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (read_stdin) {\n-\t\tstruct strbuf err = STRBUF_INIT;\n-\t\tstruct ref_transaction *transaction;\n-\n-\t\ttransaction = ref_transaction_begin(&err);\n-\t\tif (!transaction)\n-\t\t\tdie(\"%s\", err.buf);\n \t\tif (delete || argc > 0)\n \t\t\tusage_with_options(git_update_ref_usage, options);\n \t\tif (end_null)\n \t\t\tline_termination = '\\0';\n-\t\tupdate_refs_stdin(transaction);\n-\t\tif (ref_transaction_commit(transaction, &err))\n-\t\t\tdie(\"%s\", err.buf);\n-\t\tref_transaction_free(transaction);\n-\t\tstrbuf_release(&err);\n+\t\tupdate_refs_stdin();\n \t\treturn 0;\n \t}\n \n-- \n2.26.0\n\n"},{"id":"394553","messageId":"02ff6b7337cd821c74f2221c2c567e0105c01e0d.1585811014.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585811013.git.ps@pks.im","subject":"[PATCH v3 8/9] update-ref: read commands in a line-wise fashion","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-04-02T07:09:57Z","receivedAt":"2020-04-02T07:09:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The git-update-ref(1) supports a `--stdin` mode that allows it to read\nall reference updates from standard input. This is mainly used to allow\nfor atomic reference updates that are all or nothing, so that either all\nreferences will get updated or none.\n\nCurrently, git-update-ref(1) reads all commands as a single block of up\nto 1000 characters and only starts processing after stdin gets closed.\nThis is less flexible than one might wish for, as it doesn't really\nallow for longer-lived transactions and doesn't allow any verification\nwithout committing everything. E.g. one may imagine the following\nexchange:\n\n > start\n < start: ok\n > update refs/heads/master $NEWOID1 $OLDOID1\n > update refs/heads/branch $NEWOID2 $OLDOID2\n > prepare\n < prepare: ok\n > commit\n < commit: ok\n\nWhen reading all input as a whole block, the above interactive protocol\nis obviously impossible to achieve. But by converting the command to\nread commands linewise, we can make it more interactive than before.\n\nObviously, the linewise interface is only a first step in making\ngit-update-ref(1) work in a more transaction-oriented way. Missing is\nmost importantly support for transactional commands that manage the\ncurrent transaction.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/update-ref.c | 85 +++++++++++++++++++++++---------------------\n 1 file changed, 45 insertions(+), 40 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 0f34b68904..348407b896 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -178,8 +178,8 @@ static int parse_next_oid(const char **next, const char *end,\n * depending on how line_termination is set.\n */\n \n-static const char *parse_cmd_update(struct ref_transaction *transaction,\n-\t\t\t\t const char *next, const char *end)\n+static void parse_cmd_update(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -209,12 +209,10 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,\n \tupdate_flags = default_flags;\n \tfree(refname);\n \tstrbuf_release(&err);\n-\n-\treturn next;\n }\n \n-static const char *parse_cmd_create(struct ref_transaction *transaction,\n-\t\t\t\t const char *next, const char *end)\n+static void parse_cmd_create(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -241,12 +239,10 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,\n \tupdate_flags = default_flags;\n \tfree(refname);\n \tstrbuf_release(&err);\n-\n-\treturn next;\n }\n \n-static const char *parse_cmd_delete(struct ref_transaction *transaction,\n-\t\t\t\t const char *next, const char *end)\n+static void parse_cmd_delete(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -277,12 +273,10 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,\n \tupdate_flags = default_flags;\n \tfree(refname);\n \tstrbuf_release(&err);\n-\n-\treturn next;\n }\n \n-static const char *parse_cmd_verify(struct ref_transaction *transaction,\n-\t\t\t\t const char *next, const char *end)\n+static void parse_cmd_verify(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tchar *refname;\n@@ -306,71 +300,82 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,\n \tupdate_flags = default_flags;\n \tfree(refname);\n \tstrbuf_release(&err);\n-\n-\treturn next;\n }\n \n-static const char *parse_cmd_option(struct ref_transaction *transaction,\n-\t\t\t\t const char *next, const char *end)\n+static void parse_cmd_option(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n {\n \tconst char *rest;\n \tif (skip_prefix(next, \"no-deref\", &rest) && *rest == line_termination)\n \t\tupdate_flags |= REF_NO_DEREF;\n \telse\n \t\tdie(\"option unknown: %s\", next);\n-\treturn rest;\n }\n \n static const struct parse_cmd {\n \tconst char *prefix;\n-\tconst char *(*fn)(struct ref_transaction *, const char *, const char *);\n+\tvoid (*fn)(struct ref_transaction *, const char *, const char *);\n+\tunsigned args;\n } command[] = {\n-\t{ \"update\", parse_cmd_update },\n-\t{ \"create\", parse_cmd_create },\n-\t{ \"delete\", parse_cmd_delete },\n-\t{ \"verify\", parse_cmd_verify },\n-\t{ \"option\", parse_cmd_option },\n+\t{ \"update\", parse_cmd_update, 3 },\n+\t{ \"create\", parse_cmd_create, 2 },\n+\t{ \"delete\", parse_cmd_delete, 2 },\n+\t{ \"verify\", parse_cmd_verify, 2 },\n+\t{ \"option\", parse_cmd_option, 1 },\n };\n \n static void update_refs_stdin(void)\n {\n \tstruct strbuf input = STRBUF_INIT, err = STRBUF_INIT;\n \tstruct ref_transaction *transaction;\n-\tconst char *next;\n-\tint i;\n-\n-\tif (strbuf_read(&input, 0, 1000) < 0)\n-\t\tdie_errno(\"could not read from stdin\");\n-\tnext = input.buf;\n+\tint i, j;\n \n \ttransaction = ref_transaction_begin(&err);\n \tif (!transaction)\n \t\tdie(\"%s\", err.buf);\n \n \t/* Read each line dispatch its command */\n-\twhile (next < input.buf + input.len) {\n+\twhile (!strbuf_getwholeline(&input, stdin, line_termination)) {\n \t\tconst struct parse_cmd *cmd = NULL;\n \n-\t\tif (*next == line_termination)\n+\t\tif (*input.buf == line_termination)\n \t\t\tdie(\"empty command in input\");\n-\t\telse if (isspace(*next))\n-\t\t\tdie(\"whitespace before command: %s\", next);\n+\t\telse if (isspace(*input.buf))\n+\t\t\tdie(\"whitespace before command: %s\", input.buf);\n \n \t\tfor (i = 0; i < ARRAY_SIZE(command); i++) {\n \t\t\tconst char *prefix = command[i].prefix;\n+\t\t\tchar c;\n \n-\t\t\tif (!skip_prefix(next, prefix, &next) ||\n-\t\t\t !skip_prefix(next, \" \", &next))\n+\t\t\tif (!starts_with(input.buf, prefix))\n+\t\t\t\tcontinue;\n+\n+\t\t\t/*\n+\t\t\t * If the command has arguments, verify that it's\n+\t\t\t * followed by a space. Otherwise, it shall be followed\n+\t\t\t * by a line terminator.\n+\t\t\t */\n+\t\t\tc = command[i].args ? ' ' : line_termination;\n+\t\t\tif (input.buf[strlen(prefix)] != c)\n \t\t\t\tcontinue;\n \n \t\t\tcmd = &command[i];\n \t\t\tbreak;\n \t\t}\n \t\tif (!cmd)\n-\t\t\tdie(\"unknown command: %s\", next);\n+\t\t\tdie(\"unknown command: %s\", input.buf);\n \n-\t\tnext = cmd->fn(transaction, next, input.buf + input.len);\n-\t\tnext++;\n+\t\t/*\n+\t\t * Read additional arguments if NUL-terminated. Do not raise an\n+\t\t * error in case there is an early EOF to let the command\n+\t\t * handle missing arguments with a proper error message.\n+\t\t */\n+\t\tfor (j = 1; line_termination == '\\0' && j < cmd->args; j++)\n+\t\t\tif (strbuf_appendwholeline(&input, stdin, line_termination))\n+\t\t\t\tbreak;\n+\n+\t\tcmd->fn(transaction, input.buf + strlen(cmd->prefix) + !!cmd->args,\n+\t\t\tinput.buf + input.len);\n \t}\n \n \tif (ref_transaction_commit(transaction, &err))\n-- \n2.26.0\n\n"},{"id":"394554","messageId":"5670bea2b15bafbd93d7d507b35e54980ad06fb9.1585811014.git.ps@pks.im","threadId":"53094","inReplyTo":"cover.1585811013.git.ps@pks.im","subject":"[PATCH v3 9/9] update-ref: implement interactive transaction handling","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-04-02T07:10:02Z","receivedAt":"2020-04-02T07:10:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The git-update-ref(1) command can only handle queueing transactions\nright now via its \"--stdin\" parameter, but there is no way for users to\nhandle the transaction itself in a more explicit way. E.g. in a\nreplicated scenario, one may imagine a coordinator that spawns\ngit-update-ref(1) for multiple repositories and only if all agree that\nan update is possible will the coordinator send a commit. Such a\ntransactional session could look like\n\n > start\n < start: ok\n > update refs/heads/master $OLD $NEW\n > prepare\n < prepare: ok\n # All nodes have returned \"ok\"\n > commit\n < commit: ok\n\nor\n\n > start\n < start: ok\n > create refs/heads/master $OLD $NEW\n > prepare\n < fatal: cannot lock ref 'refs/heads/master': reference already exists\n # On all other nodes:\n > abort\n < abort: ok\n\nIn order to allow for such transactional sessions, this commit\nintroduces four new commands for git-update-ref(1), which matches those\nwe have internally already with the exception of \"start\":\n\n - start: start a new transaction\n\n - prepare: prepare the transaction, that is try to lock all\n references and verify their current value matches the\n expected one\n\n - commit: explicitly commit a session, that is update references to\n match their new expected state\n\n - abort: abort a session and roll back all changes\n\nBy design, git-update-ref(1) will commit as soon as standard input is\nbeing closed. While fine in a non-transactional world, it is definitely\nunexpected in a transactional world. Because of this, as soon as any of\nthe new transactional commands is used, the default will change to\naborting without an explicit \"commit\". To avoid a race between queueing\nupdates and the first \"prepare\" that starts a transaction, the \"start\"\ncommand has been added to start an explicit transaction.\n\nAdd some tests to exercise this new functionality.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/git-update-ref.txt | 26 ++++++\n builtin/update-ref.c | 106 +++++++++++++++++++++++--\n t/t1400-update-ref.sh | 131 +++++++++++++++++++++++++++++++\n 3 files changed, 255 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt\nindex 9bd039ce08..3e737c2360 100644\n--- a/Documentation/git-update-ref.txt\n+++ b/Documentation/git-update-ref.txt\n@@ -66,6 +66,10 @@ performs all modifications together. Specify commands of the form:\n \tdelete SP [SP ] LF\n \tverify SP [SP ] LF\n \toption SP LF\n+\tstart LF\n+\tprepare LF\n+\tcommit LF\n+\tabort LF\n \n With `--create-reflog`, update-ref will create a reflog for each ref\n even if one would not ordinarily be created.\n@@ -83,6 +87,10 @@ quoting:\n \tdelete SP NUL [] NUL\n \tverify SP NUL [] NUL\n \toption SP NUL\n+\tstart NUL\n+\tprepare NUL\n+\tcommit NUL\n+\tabort NUL\n \n In this format, use 40 \"0\" to specify a zero value, and use the empty\n string to specify a missing value.\n@@ -114,6 +122,24 @@ option::\n \tThe only valid option is `no-deref` to avoid dereferencing\n \ta symbolic ref.\n \n+start::\n+\tStart a transaction. In contrast to a non-transactional session, a\n+\ttransaction will automatically abort if the session ends without an\n+\texplicit commit.\n+\n+prepare::\n+\tPrepare to commit the transaction. This will create lock files for all\n+\tqueued reference updates. If one reference could not be locked, the\n+\ttransaction will be aborted.\n+\n+commit::\n+\tCommit all reference updates queued for the transaction, ending the\n+\ttransaction.\n+\n+abort::\n+\tAbort the transaction, releasing all locks if the transaction is in\n+\tprepared state.\n+\n If all s can be locked with matching s\n simultaneously, all modifications are performed. Otherwise, no\n modifications are performed. Note that while each individual\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 348407b896..b74dd9a69d 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -312,21 +312,80 @@ static void parse_cmd_option(struct ref_transaction *transaction,\n \t\tdie(\"option unknown: %s\", next);\n }\n \n+static void parse_cmd_start(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n+{\n+\tif (*next != line_termination)\n+\t\tdie(\"start: extra input: %s\", next);\n+\tputs(\"start: ok\");\n+}\n+\n+static void parse_cmd_prepare(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n+{\n+\tstruct strbuf error = STRBUF_INIT;\n+\tif (*next != line_termination)\n+\t\tdie(\"prepare: extra input: %s\", next);\n+\tif (ref_transaction_prepare(transaction, &error))\n+\t\tdie(\"prepare: %s\", error.buf);\n+\tputs(\"prepare: ok\");\n+}\n+\n+static void parse_cmd_abort(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n+{\n+\tstruct strbuf error = STRBUF_INIT;\n+\tif (*next != line_termination)\n+\t\tdie(\"abort: extra input: %s\", next);\n+\tif (ref_transaction_abort(transaction, &error))\n+\t\tdie(\"abort: %s\", error.buf);\n+\tputs(\"abort: ok\");\n+}\n+\n+static void parse_cmd_commit(struct ref_transaction *transaction,\n+\t\t\t const char *next, const char *end)\n+{\n+\tstruct strbuf error = STRBUF_INIT;\n+\tif (*next != line_termination)\n+\t\tdie(\"commit: extra input: %s\", next);\n+\tif (ref_transaction_commit(transaction, &error))\n+\t\tdie(\"commit: %s\", error.buf);\n+\tputs(\"commit: ok\");\n+\tref_transaction_free(transaction);\n+}\n+\n+enum update_refs_state {\n+\t/* Non-transactional state open for updates. */\n+\tUPDATE_REFS_OPEN,\n+\t/* A transaction has been started. */\n+\tUPDATE_REFS_STARTED,\n+\t/* References are locked and ready for commit */\n+\tUPDATE_REFS_PREPARED,\n+\t/* Transaction has been committed or closed. */\n+\tUPDATE_REFS_CLOSED,\n+};\n+\n static const struct parse_cmd {\n \tconst char *prefix;\n \tvoid (*fn)(struct ref_transaction *, const char *, const char *);\n \tunsigned args;\n+\tenum update_refs_state state;\n } command[] = {\n-\t{ \"update\", parse_cmd_update, 3 },\n-\t{ \"create\", parse_cmd_create, 2 },\n-\t{ \"delete\", parse_cmd_delete, 2 },\n-\t{ \"verify\", parse_cmd_verify, 2 },\n-\t{ \"option\", parse_cmd_option, 1 },\n+\t{ \"update\", parse_cmd_update, 3, UPDATE_REFS_OPEN },\n+\t{ \"create\", parse_cmd_create, 2, UPDATE_REFS_OPEN },\n+\t{ \"delete\", parse_cmd_delete, 2, UPDATE_REFS_OPEN },\n+\t{ \"verify\", parse_cmd_verify, 2, UPDATE_REFS_OPEN },\n+\t{ \"option\", parse_cmd_option, 1, UPDATE_REFS_OPEN },\n+\t{ \"start\", parse_cmd_start, 0, UPDATE_REFS_STARTED },\n+\t{ \"prepare\", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED },\n+\t{ \"abort\", parse_cmd_abort, 0, UPDATE_REFS_CLOSED },\n+\t{ \"commit\", parse_cmd_commit, 0, UPDATE_REFS_CLOSED },\n };\n \n static void update_refs_stdin(void)\n {\n \tstruct strbuf input = STRBUF_INIT, err = STRBUF_INIT;\n+\tenum update_refs_state state = UPDATE_REFS_OPEN;\n \tstruct ref_transaction *transaction;\n \tint i, j;\n \n@@ -374,14 +433,45 @@ static void update_refs_stdin(void)\n \t\t\tif (strbuf_appendwholeline(&input, stdin, line_termination))\n \t\t\t\tbreak;\n \n+\t\tswitch (state) {\n+\t\tcase UPDATE_REFS_OPEN:\n+\t\tcase UPDATE_REFS_STARTED:\n+\t\t\t/* Do not downgrade a transaction to a non-transaction. */\n+\t\t\tif (cmd->state >= state)\n+\t\t\t\tstate = cmd->state;\n+\t\t\tbreak;\n+\t\tcase UPDATE_REFS_PREPARED:\n+\t\t\tif (cmd->state != UPDATE_REFS_CLOSED)\n+\t\t\t\tdie(\"prepared transactions can only be closed\");\n+\t\t\tstate = cmd->state;\n+\t\t\tbreak;\n+\t\tcase UPDATE_REFS_CLOSED:\n+\t\t\tdie(\"transaction is closed\");\n+\t\t\tbreak;\n+\t\t}\n+\n \t\tcmd->fn(transaction, input.buf + strlen(cmd->prefix) + !!cmd->args,\n \t\t\tinput.buf + input.len);\n \t}\n \n-\tif (ref_transaction_commit(transaction, &err))\n-\t\tdie(\"%s\", err.buf);\n+\tswitch (state) {\n+\tcase UPDATE_REFS_OPEN:\n+\t\t/* Commit by default if no transaction was requested. */\n+\t\tif (ref_transaction_commit(transaction, &err))\n+\t\t\tdie(\"%s\", err.buf);\n+\t\tref_transaction_free(transaction);\n+\t\tbreak;\n+\tcase UPDATE_REFS_STARTED:\n+\tcase UPDATE_REFS_PREPARED:\n+\t\t/* If using a transaction, we want to abort it. */\n+\t\tif (ref_transaction_abort(transaction, &err))\n+\t\t\tdie(\"%s\", err.buf);\n+\t\tbreak;\n+\tcase UPDATE_REFS_CLOSED:\n+\t\t/* Otherwise no need to do anything, the transaction was closed already. */\n+\t\tbreak;\n+\t}\n \n-\tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n \tstrbuf_release(&input);\n }\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex a6224ef65f..48d0d42afd 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -1404,4 +1404,135 @@ test_expect_success 'handle per-worktree refs in refs/bisect' '\n \t! test_cmp main-head worktree-head\n '\n \n+test_expect_success 'transaction handles empty commit' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tprepare\n+\tcommit\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start prepare commit >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction handles empty commit with missing prepare' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tcommit\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start commit >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction handles sole commit' '\n+\tcat >stdin <<-EOF &&\n+\tcommit\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" commit >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction handles empty abort' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tprepare\n+\tabort\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start prepare abort >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction exits on multiple aborts' '\n+\tcat >stdin <<-EOF &&\n+\tabort\n+\tabort\n+\tEOF\n+\ttest_must_fail git update-ref --stdin actual 2>err &&\n+\tprintf \"%s: ok\\n\" abort >expect &&\n+\ttest_cmp expect actual &&\n+\tgrep \"fatal: transaction is closed\" err\n+'\n+\n+test_expect_success 'transaction exits on start after prepare' '\n+\tcat >stdin <<-EOF &&\n+\tprepare\n+\tstart\n+\tEOF\n+\ttest_must_fail git update-ref --stdin err >actual &&\n+\tprintf \"%s: ok\\n\" prepare >expect &&\n+\ttest_cmp expect actual &&\n+\tgrep \"fatal: prepared transactions can only be closed\" err\n+'\n+\n+test_expect_success 'transaction handles empty abort with missing prepare' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tabort\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start abort >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction handles sole abort' '\n+\tcat >stdin <<-EOF &&\n+\tabort\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" abort >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction can handle commit' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tcreate $a HEAD\n+\tcommit\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start commit >expect &&\n+\ttest_cmp expect actual &&\n+\tgit rev-parse HEAD >expect &&\n+\tgit rev-parse $a >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'transaction can handle abort' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tcreate $b HEAD\n+\tabort\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start abort >expect &&\n+\ttest_cmp expect actual &&\n+\ttest_path_is_missing .git/$b\n+'\n+\n+test_expect_success 'transaction aborts by default' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tcreate $b HEAD\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start >expect &&\n+\ttest_cmp expect actual &&\n+\ttest_path_is_missing .git/$b\n+'\n+\n+test_expect_success 'transaction with prepare aborts by default' '\n+\tcat >stdin <<-EOF &&\n+\tstart\n+\tcreate $b HEAD\n+\tprepare\n+\tEOF\n+\tgit update-ref --stdin actual &&\n+\tprintf \"%s: ok\\n\" start prepare >expect &&\n+\ttest_cmp expect actual &&\n+\ttest_path_is_missing .git/$b\n+'\n+\n test_done\n-- \n2.26.0\n\n"},{"id":"394663","messageId":"fd4b874a-478f-2139-e910-c000b8a1f396@gmail.com","threadId":"53094","inReplyTo":"5670bea2b15bafbd93d7d507b35e54980ad06fb9.1585811014.git.ps@pks.im","subject":"Re: [PATCH v3 9/9] update-ref: implement interactive transaction handling","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-04-03T13:40:26Z","receivedAt":"2020-04-03T13:40:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Patrick\n\nOn 02/04/2020 08:10, Patrick Steinhardt wrote:\n> The git-update-ref(1) command can only handle queueing transactions\n> right now via its \"--stdin\" parameter, but there is no way for users to\n> handle the transaction itself in a more explicit way. E.g. in a\n> replicated scenario, one may imagine a coordinator that spawns\n> git-update-ref(1) for multiple repositories and only if all agree that\n> an update is possible will the coordinator send a commit. Such a\n> transactional session could look like\n> \n> > start\n> < start: ok\n> > update refs/heads/master $OLD $NEW\n> > prepare\n> < prepare: ok\n> # All nodes have returned \"ok\"\n> > commit\n> < commit: ok\n> \n> or\n> \n> > start\n> < start: ok\n> > create refs/heads/master $OLD $NEW\n> > prepare\n> < fatal: cannot lock ref 'refs/heads/master': reference already exists\n> # On all other nodes:\n> > abort\n> < abort: ok\n> \n> In order to allow for such transactional sessions, this commit\n> introduces four new commands for git-update-ref(1), which matches those\n> we have internally already with the exception of \"start\":\n> \n> - start: start a new transaction\n> \n> - prepare: prepare the transaction, that is try to lock all\n> references and verify their current value matches the\n> expected one\n> \n> - commit: explicitly commit a session, that is update references to\n> match their new expected state\n> \n> - abort: abort a session and roll back all changes\n> \n> By design, git-update-ref(1) will commit as soon as standard input is\n> being closed. While fine in a non-transactional world, it is definitely\n> unexpected in a transactional world. Because of this, as soon as any of\n> the new transactional commands is used, the default will change to\n> aborting without an explicit \"commit\". To avoid a race between queueing\n> updates and the first \"prepare\" that starts a transaction, the \"start\"\n> command has been added to start an explicit transaction.\n> \n> Add some tests to exercise this new functionality.\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n> Documentation/git-update-ref.txt | 26 ++++++\n> builtin/update-ref.c | 106 +++++++++++++++++++++++--\n> t/t1400-update-ref.sh | 131 +++++++++++++++++++++++++++++++\n> 3 files changed, 255 insertions(+), 8 deletions(-)\n> \n> diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt\n> index 9bd039ce08..3e737c2360 100644\n> --- a/Documentation/git-update-ref.txt\n> +++ b/Documentation/git-update-ref.txt\n> @@ -66,6 +66,10 @@ performs all modifications together. Specify commands of the form:\n> \tdelete SP [SP ] LF\n> \tverify SP [SP ] LF\n> \toption SP LF\n> +\tstart LF\n> +\tprepare LF\n> +\tcommit LF\n> +\tabort LF\n> \n> With `--create-reflog`, update-ref will create a reflog for each ref\n> even if one would not ordinarily be created.\n> @@ -83,6 +87,10 @@ quoting:\n> \tdelete SP NUL [] NUL\n> \tverify SP NUL [] NUL\n> \toption SP NUL\n> +\tstart NUL\n> +\tprepare NUL\n> +\tcommit NUL\n> +\tabort NUL\n> \n> In this format, use 40 \"0\" to specify a zero value, and use the empty\n> string to specify a missing value.\n> @@ -114,6 +122,24 @@ option::\n> \tThe only valid option is `no-deref` to avoid dereferencing\n> \ta symbolic ref.\n> \n> +start::\n> +\tStart a transaction. In contrast to a non-transactional session, \n\nI found the talk of \"non-transactional\" sessions a bit confusing because \nthe normal --stdin does update all the refs it is given as a single \ntransaction, so that if it cannot update one ref none of them are \nupdated. If I've understood correctly these changes are about \ncoordinating transactions across several repositories. I'm not sure how \nbest to convey that in the man page - perhaps we could call them single \nrepository transactions and multi repository transaction or something.\n\nBest Wishes\n\nPhillip\n\n> a\n> +\ttransaction will automatically abort if the session ends without an\n> +\texplicit commit.\n> +\n> +prepare::\n> +\tPrepare to commit the transaction. This will create lock files for all\n> +\tqueued reference updates. If one reference could not be locked, the\n> +\ttransaction will be aborted.\n> +\n> +commit::\n> +\tCommit all reference updates queued for the transaction, ending the\n> +\ttransaction.\n> +\n> +abort::\n> +\tAbort the transaction, releasing all locks if the transaction is in\n> +\tprepared state.\n> +\n> If all s can be locked with matching s\n> simultaneously, all modifications are performed. Otherwise, no\n> modifications are performed. Note that while each individual\n> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n> index 348407b896..b74dd9a69d 100644\n> --- a/builtin/update-ref.c\n> +++ b/builtin/update-ref.c\n> @@ -312,21 +312,80 @@ static void parse_cmd_option(struct ref_transaction *transaction,\n> \t\tdie(\"option unknown: %s\", next);\n> }\n> \n> +static void parse_cmd_start(struct ref_transaction *transaction,\n> +\t\t\t const char *next, const char *end)\n> +{\n> +\tif (*next != line_termination)\n> +\t\tdie(\"start: extra input: %s\", next);\n> +\tputs(\"start: ok\");\n> +}\n> +\n> +static void parse_cmd_prepare(struct ref_transaction *transaction,\n> +\t\t\t const char *next, const char *end)\n> +{\n> +\tstruct strbuf error = STRBUF_INIT;\n> +\tif (*next != line_termination)\n> +\t\tdie(\"prepare: extra input: %s\", next);\n> +\tif (ref_transaction_prepare(transaction, &error))\n> +\t\tdie(\"prepare: %s\", error.buf);\n> +\tputs(\"prepare: ok\");\n> +}\n> +\n> +static void parse_cmd_abort(struct ref_transaction *transaction,\n> +\t\t\t const char *next, const char *end)\n> +{\n> +\tstruct strbuf error = STRBUF_INIT;\n> +\tif (*next != line_termination)\n> +\t\tdie(\"abort: extra input: %s\", next);\n> +\tif (ref_transaction_abort(transaction, &error))\n> +\t\tdie(\"abort: %s\", error.buf);\n> +\tputs(\"abort: ok\");\n> +}\n> +\n> +static void parse_cmd_commit(struct ref_transaction *transaction,\n> +\t\t\t const char *next, const char *end)\n> +{\n> +\tstruct strbuf error = STRBUF_INIT;\n> +\tif (*next != line_termination)\n> +\t\tdie(\"commit: extra input: %s\", next);\n> +\tif (ref_transaction_commit(transaction, &error))\n> +\t\tdie(\"commit: %s\", error.buf);\n> +\tputs(\"commit: ok\");\n> +\tref_transaction_free(transaction);\n> +}\n> +\n> +enum update_refs_state {\n> +\t/* Non-transactional state open for updates. */\n> +\tUPDATE_REFS_OPEN,\n> +\t/* A transaction has been started. */\n> +\tUPDATE_REFS_STARTED,\n> +\t/* References are locked and ready for commit */\n> +\tUPDATE_REFS_PREPARED,\n> +\t/* Transaction has been committed or closed. */\n> +\tUPDATE_REFS_CLOSED,\n> +};\n> +\n> static const struct parse_cmd {\n> \tconst char *prefix;\n> \tvoid (*fn)(struct ref_transaction *, const char *, const char *);\n> \tunsigned args;\n> +\tenum update_refs_state state;\n> } command[] = {\n> -\t{ \"update\", parse_cmd_update, 3 },\n> -\t{ \"create\", parse_cmd_create, 2 },\n> -\t{ \"delete\", parse_cmd_delete, 2 },\n> -\t{ \"verify\", parse_cmd_verify, 2 },\n> -\t{ \"option\", parse_cmd_option, 1 },\n> +\t{ \"update\", parse_cmd_update, 3, UPDATE_REFS_OPEN },\n> +\t{ \"create\", parse_cmd_create, 2, UPDATE_REFS_OPEN },\n> +\t{ \"delete\", parse_cmd_delete, 2, UPDATE_REFS_OPEN },\n> +\t{ \"verify\", parse_cmd_verify, 2, UPDATE_REFS_OPEN },\n> +\t{ \"option\", parse_cmd_option, 1, UPDATE_REFS_OPEN },\n> +\t{ \"start\", parse_cmd_start, 0, UPDATE_REFS_STARTED },\n> +\t{ \"prepare\", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED },\n> +\t{ \"abort\", parse_cmd_abort, 0, UPDATE_REFS_CLOSED },\n> +\t{ \"commit\", parse_cmd_commit, 0, UPDATE_REFS_CLOSED },\n> };\n> \n> static void update_refs_stdin(void)\n> {\n> \tstruct strbuf input = STRBUF_INIT, err = STRBUF_INIT;\n> +\tenum update_refs_state state = UPDATE_REFS_OPEN;\n> \tstruct ref_transaction *transaction;\n> \tint i, j;\n> \n> @@ -374,14 +433,45 @@ static void update_refs_stdin(void)\n> \t\t\tif (strbuf_appendwholeline(&input, stdin, line_termination))\n> \t\t\t\tbreak;\n> \n> +\t\tswitch (state) {\n> +\t\tcase UPDATE_REFS_OPEN:\n> +\t\tcase UPDATE_REFS_STARTED:\n> +\t\t\t/* Do not downgrade a transaction to a non-transaction. */\n> +\t\t\tif (cmd->state >= state)\n> +\t\t\t\tstate = cmd->state;\n> +\t\t\tbreak;\n> +\t\tcase UPDATE_REFS_PREPARED:\n> +\t\t\tif (cmd->state != UPDATE_REFS_CLOSED)\n> +\t\t\t\tdie(\"prepared transactions can only be closed\");\n> +\t\t\tstate = cmd->state;\n> +\t\t\tbreak;\n> +\t\tcase UPDATE_REFS_CLOSED:\n> +\t\t\tdie(\"transaction is closed\");\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> \t\tcmd->fn(transaction, input.buf + strlen(cmd->prefix) + !!cmd->args,\n> \t\t\tinput.buf + input.len);\n> \t}\n> \n> -\tif (ref_transaction_commit(transaction, &err))\n> -\t\tdie(\"%s\", err.buf);\n> +\tswitch (state) {\n> +\tcase UPDATE_REFS_OPEN:\n> +\t\t/* Commit by default if no transaction was requested. */\n> +\t\tif (ref_transaction_commit(transaction, &err))\n> +\t\t\tdie(\"%s\", err.buf);\n> +\t\tref_transaction_free(transaction);\n> +\t\tbreak;\n> +\tcase UPDATE_REFS_STARTED:\n> +\tcase UPDATE_REFS_PREPARED:\n> +\t\t/* If using a transaction, we want to abort it. */\n> +\t\tif (ref_transaction_abort(transaction, &err))\n> +\t\t\tdie(\"%s\", err.buf);\n> +\t\tbreak;\n> +\tcase UPDATE_REFS_CLOSED:\n> +\t\t/* Otherwise no need to do anything, the transaction was closed already. */\n> +\t\tbreak;\n> +\t}\n> \n> -\tref_transaction_free(transaction);\n> \tstrbuf_release(&err);\n> \tstrbuf_release(&input);\n> }\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index a6224ef65f..48d0d42afd 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -1404,4 +1404,135 @@ test_expect_success 'handle per-worktree refs in refs/bisect' '\n> \t! test_cmp main-head worktree-head\n> '\n> \n> +test_expect_success 'transaction handles empty commit' '\n> +\tcat >stdin <<-EOF &&\n> +\tstart\n> +\tprepare\n> +\tcommit\n> +\tEOF\n> +\tgit update-ref --stdin actual &&\n> +\tprintf \"%s: ok\\n\" start prepare commit >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'transaction handles empty commit with missing prepare' '\n> +\tcat >stdin <<-EOF &&\n> +\tstart\n> +\tcommit\n> +\tEOF\n> +\tgit update-ref --stdin actual &&\n> +\tprintf \"%s: ok\\n\" start commit >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'transaction handles sole commit' '\n> +\tcat >stdin <<-EOF &&\n> +\tcommit\n> +\tEOF\n> +\tgit update-ref --stdin actual &&\n> +\tprintf \"%s: ok\\n\" commit >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'transaction handles empty abort' '\n> +\tcat >stdin <<-EOF &&\n> +\tstart\n> +\tprepare\n> +\tabort\n> +\tEOF\n> +\tgit update-ref --stdin actual &&\n> +\tprintf \"%s: ok\\n\" start prepare abort >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'transaction exits on multiple aborts' '\n> +\tcat >stdin <<-EOF &&\n> +\tabort\n> +\tabort\n> +\tEOF\n> +\ttest_must_fail git update-ref --stdin actual 2>err &&\n> +\tprintf \"%s: ok\\n\" abort >expect &&\n> +\ttest_cmp expect actual &&\n> +\tgrep \"fatal: transaction is closed\" err\n> +'\n> +\n> +test_expect_success 'transaction exits on start after prepare' '\n> +\tcat >stdin <<-EOF &&\n> +\tprepare\n> +\tstart\n> +\tEOF\n> +\ttest_must_fail git update-ref --stdin err >actual &&\n> +\tprintf \"%s: ok\\n\" prepare >expect &&\n> +\ttest_cmp expect actual &&\n> +\tgrep \"fatal: prepared transactions can only be closed\" err\n> +'\n> +\n> +test_expect_success 'transaction handles empty abort with missing prepare' '\n> +\tcat >stdin <<-EOF &&\n> +\tstart\n> +\tabort\n> +\tEOF\n> +\tgit update-ref --stdin actual &&\n> +\tprintf \"%s: ok\\n\" start abort >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'transaction handles sole abort' '\n> +\tcat >stdin <<-EOF &&\n> +\tabort\n> +\tEOF\n> +\tgit update-ref --stdin actual &&\n> +\tprintf \"%s: ok\\n\" abort >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'transaction can handle commit' '\n> +\tcat >stdin <<-EOF &&\n> +\tstart\n> +\tcreate $a HEAD\n> +\tcommit\n> +\tEOF\n> +\tgit update-ref --stdin actual &&\n> +\tprintf \"%s: ok\\n\" start commit >expect &&\n> +\ttest_cmp expect actual &&\n> +\tgit rev-parse HEAD >expect &&\n> +\tgit rev-parse $a >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'transaction can handle abort' '\n> +\tcat >stdin <<-EOF &&\n> +\tstart\n> +\tcreate $b HEAD\n> +\tabort\n> +\tEOF\n> +\tgit update-ref --stdin actual &&\n> +\tprintf \"%s: ok\\n\" start abort >expect &&\n> +\ttest_cmp expect actual &&\n> +\ttest_path_is_missing .git/$b\n> +'\n> +\n> +test_expect_success 'transaction aborts by default' '\n> +\tcat >stdin <<-EOF &&\n> +\tstart\n> +\tcreate $b HEAD\n> +\tEOF\n> +\tgit update-ref --stdin actual &&\n> +\tprintf \"%s: ok\\n\" start >expect &&\n> +\ttest_cmp expect actual &&\n> +\ttest_path_is_missing .git/$b\n> +'\n> +\n> +test_expect_success 'transaction with prepare aborts by default' '\n> +\tcat >stdin <<-EOF &&\n> +\tstart\n> +\tcreate $b HEAD\n> +\tprepare\n> +\tEOF\n> +\tgit update-ref --stdin actual &&\n> +\tprintf \"%s: ok\\n\" start prepare >expect &&\n> +\ttest_cmp expect actual &&\n> +\ttest_path_is_missing .git/$b\n> +'\n> +\n> test_done\n> \n"},{"id":"394676","messageId":"20200403165152.GA4244@xps.pks.im","threadId":"53094","inReplyTo":"fd4b874a-478f-2139-e910-c000b8a1f396@gmail.com","subject":"Re: [PATCH v3 9/9] update-ref: implement interactive transaction handling","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-04-03T16:51:52Z","receivedAt":"2020-04-03T16:51:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Apr 03, 2020 at 02:40:26PM +0100, Phillip Wood wrote:\n> Hi Patrick\n> \n> On 02/04/2020 08:10, Patrick Steinhardt wrote:\n> > The git-update-ref(1) command can only handle queueing transactions\n> > right now via its \"--stdin\" parameter, but there is no way for users to\n> > handle the transaction itself in a more explicit way. E.g. in a\n> > replicated scenario, one may imagine a coordinator that spawns\n> > git-update-ref(1) for multiple repositories and only if all agree that\n> > an update is possible will the coordinator send a commit. Such a\n> > transactional session could look like\n> > \n> > > start\n> > < start: ok\n> > > update refs/heads/master $OLD $NEW\n> > > prepare\n> > < prepare: ok\n> > # All nodes have returned \"ok\"\n> > > commit\n> > < commit: ok\n> > \n> > or\n> > \n> > > start\n> > < start: ok\n> > > create refs/heads/master $OLD $NEW\n> > > prepare\n> > < fatal: cannot lock ref 'refs/heads/master': reference already exists\n> > # On all other nodes:\n> > > abort\n> > < abort: ok\n> > \n> > In order to allow for such transactional sessions, this commit\n> > introduces four new commands for git-update-ref(1), which matches those\n> > we have internally already with the exception of \"start\":\n> > \n> > - start: start a new transaction\n> > \n> > - prepare: prepare the transaction, that is try to lock all\n> > references and verify their current value matches the\n> > expected one\n> > \n> > - commit: explicitly commit a session, that is update references to\n> > match their new expected state\n> > \n> > - abort: abort a session and roll back all changes\n> > \n> > By design, git-update-ref(1) will commit as soon as standard input is\n> > being closed. While fine in a non-transactional world, it is definitely\n> > unexpected in a transactional world. Because of this, as soon as any of\n> > the new transactional commands is used, the default will change to\n> > aborting without an explicit \"commit\". To avoid a race between queueing\n> > updates and the first \"prepare\" that starts a transaction, the \"start\"\n> > command has been added to start an explicit transaction.\n> > \n> > Add some tests to exercise this new functionality.\n> > \n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > Documentation/git-update-ref.txt | 26 ++++++\n> > builtin/update-ref.c | 106 +++++++++++++++++++++++--\n> > t/t1400-update-ref.sh | 131 +++++++++++++++++++++++++++++++\n> > 3 files changed, 255 insertions(+), 8 deletions(-)\n> > \n> > diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt\n> > index 9bd039ce08..3e737c2360 100644\n> > --- a/Documentation/git-update-ref.txt\n> > +++ b/Documentation/git-update-ref.txt\n> > @@ -66,6 +66,10 @@ performs all modifications together. Specify commands of the form:\n> > \tdelete SP [SP ] LF\n> > \tverify SP [SP ] LF\n> > \toption SP LF\n> > +\tstart LF\n> > +\tprepare LF\n> > +\tcommit LF\n> > +\tabort LF\n> > \n> > With `--create-reflog`, update-ref will create a reflog for each ref\n> > even if one would not ordinarily be created.\n> > @@ -83,6 +87,10 @@ quoting:\n> > \tdelete SP NUL [] NUL\n> > \tverify SP NUL [] NUL\n> > \toption SP NUL\n> > +\tstart NUL\n> > +\tprepare NUL\n> > +\tcommit NUL\n> > +\tabort NUL\n> > \n> > In this format, use 40 \"0\" to specify a zero value, and use the empty\n> > string to specify a missing value.\n> > @@ -114,6 +122,24 @@ option::\n> > \tThe only valid option is `no-deref` to avoid dereferencing\n> > \ta symbolic ref.\n> > \n> > +start::\n> > +\tStart a transaction. In contrast to a non-transactional session, \n> \n> I found the talk of \"non-transactional\" sessions a bit confusing because \n> the normal --stdin does update all the refs it is given as a single \n> transaction, so that if it cannot update one ref none of them are \n> updated. If I've understood correctly these changes are about \n> coordinating transactions across several repositories. I'm not sure how \n> best to convey that in the man page - perhaps we could call them single \n> repository transactions and multi repository transaction or something.\n\nThe ultimate goal is to be able to create something that sits atop a set\nof repos that's able to coordinate multiple reference transactions at\nthe same time and then do an all or nothing commit across all repos or\nabort in case any of the repos will not be able to perform the update.\nThe proposed change is still about a single repository, only, and have\nthe aim of actually enabling transaction semantics in the first place.\nUntil now, it's only possible to start a transaction via\ngit-update-refs(1), but you can't really control it except for the fnial\ncommit.\n\nSo with that being said I'm not quite sure whether it makes sense to\ndocument the potential for handling transactions across multiple repos.\nIt does show that I could improve the documentation, though, and make\nthe intent and scope clearer. I'll try to do that for the next version,\nthanks for your feedback!\n\nPatrick\n"},{"id":"394677","messageId":"xmqqa73s1njt.fsf@gitster.c.googlers.com","threadId":"53094","inReplyTo":"fd4b874a-478f-2139-e910-c000b8a1f396@gmail.com","subject":"Re: [PATCH v3 9/9] update-ref: implement interactive transaction handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-03T17:33:58Z","receivedAt":"2020-04-03T17:34:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> I found the talk of \"non-transactional\" sessions a bit confusing\n> because the normal --stdin does update all the refs it is given as a\n> single transaction, so that if it cannot update one ref none of them\n> are updated. If I've understood correctly these changes are about\n> coordinating transactions across several repositories. I'm not sure\n> how best to convey that in the man page - perhaps we could call them\n> single repository transactions and multi repository transaction or\n> something.\n\nFWIW, I described the topic in my \"What's cooking\" report as adding\nan ingredient to implement two-phase commit-style atomic updates\nacross multiple repositories. These \"prepare to commit, now if you\nsay 'yes I am ready', there is no taking it back\", \"now do the\ncommit as you promised\", and \"Earlier I made you promise that you'd\ncommit all when I tell you, but you no longer should commit\" are\ngood enough ingredients, but I am not so sure about the actual\nimplementation in this topic (or it is really possible to make the\npromise \"ok, now I can commit everything without lock conflicts if\nyou tell me\" with our ref API, which does not have \"now freeze all\nrefs in the repository---from now on until I say otherwise, no refs\ncan be manipulated\", in the first place).\n\n"},{"id":"394678","messageId":"xmqq5zeg1ngu.fsf@gitster.c.googlers.com","threadId":"53094","inReplyTo":"xmqqa73s1njt.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 9/9] update-ref: implement interactive transaction handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-03T17:35:45Z","receivedAt":"2020-04-03T17:35:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> ... but I am not so sure about the actual\n> implementation in this topic ...\n\nI take this part back---given that this is merely building on top of\nthe existing atomic update, we should be OK.\n"},{"id":"394820","messageId":"20200406071057.GB5302@tanuki.pks.im","threadId":"53094","inReplyTo":"xmqqa73s1njt.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 9/9] update-ref: implement interactive transaction handling","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2020-04-06T07:10:57Z","receivedAt":"2020-04-06T07:11:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Apr 03, 2020 at 10:33:58AM -0700, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n> > I found the talk of \"non-transactional\" sessions a bit confusing\n> > because the normal --stdin does update all the refs it is given as a\n> > single transaction, so that if it cannot update one ref none of them\n> > are updated. If I've understood correctly these changes are about\n> > coordinating transactions across several repositories. I'm not sure\n> > how best to convey that in the man page - perhaps we could call them\n> > single repository transactions and multi repository transaction or\n> > something.\n> \n> FWIW, I described the topic in my \"What's cooking\" report as adding\n> an ingredient to implement two-phase commit-style atomic updates\n> across multiple repositories. \n\nFair enough. It definitely may serve as a building block for cross-repo\ntransactions, which also is the original intent of this patch series.\nI'm not sure whether it makes sense to document it as such, as I can\nimagine usecases acting on a single repository that still want to be in\nthe business of doing two-phase commits.\n\nPatrick\n"}]}