threads / patch / 53094

patch, 9 partsSupport for transactions in `git-update-ref --stdin`

Subject: [PATCH 0/9] Support for transactions in `git-update-ref --stdin`

## tl;dr

48 messages between Mar 25, 2020 and Apr 6, 2020. Diffs are folded; open one to read it.

replies: 47people: 3as markdown or json

Patrick Steinhardt· Mar 25, 2020, 09:53 UTC · lore
Hi,

inspired by recent discussions about handling transactions in git-update-refs(1), this series implements proper transaction support in `git update-refs --stdin`. The goal is to have an all-or-nothing transaction where a user can queue multiple updates, verify that they can be committed, and then finally either commits or aborts the transaction. A typical session would look like the following:

    # Start the transaction
    > start
    < start: ok
    # Queue updates
    > delete refs/heads/branch
    > create refs/heads/another $OID1
    # Prepare the transaction. git-update-ref will now try to allocate
    # all locks and verify that references are at their expected values.
    > prepare
    < prepare: ok
    # Commit the transaction. The user could also have said "abort" to
    # roll back everything.
    > commit
    < commit: ok

The series builds on the already existing transaction support in refs.c and exposes it to the user. The most important change that was required to support this was to convert `git-update-ref --stdin` to handle input linewise instead of trying to read it in full and only acting after stdin was closed.

The series is structured as follows:
    Patches 1-2: Preparatory patches which make sense as standalone
                 patches.
    Patches 3-7: Preparatory patches that make it easier to convert to
                 reading commands in a line-wise fashion. No functional
                 changes are expected.
    Patch 8: Conversion to read commands line-wise. No functional
             changes are expected, except that Git builds up the transaction
             while reading stdin instead of waiting for stdin to be closed
             first.
    Patch 9: Implementation of transactional commands.

All in all, the new transactional support will only be enabled if the user invokes any the new commands "start", "prepare", "commit" or "abort". In case he doesn't, no functional changes are expected.

Patrick
Patrick Steinhardt (9):
  refs: fix segfault when aborting empty transaction
  git-update-ref.txt: add missing word
  strbuf: provide function to append whole lines
  update-ref: organize commands in an array
  update-ref: drop unused argument for `parse_refname`
  update-ref: pass end pointer instead of strbuf
  update-ref: move transaction handling into `update_refs_stdin()`
  update-ref: read commands in a line-wise fashion
  update-ref: implement interactive transaction handling
 Documentation/git-update-ref.txt |  28 +++-
 builtin/update-ref.c             | 255 ++++++++++++++++++++++---------
 refs/files-backend.c             |  20 +--
 strbuf.c                         |  10 ++
 strbuf.h                         |   6 +
 t/t1400-update-ref.sh            | 131 ++++++++++++++++
 6 files changed, 370 insertions(+), 80 deletions(-)
-- 
2.26.0
Patrick Steinhardt· Mar 25, 2020, 09:53 UTC · re: Patrick Steinhardt · lore

[PATCH 1/9] refs: fix segfault when aborting empty transaction

When cleaning up a transaction that has no updates queued, then the transaction's backend data will not have been allocated. We correctly handle this for the packed backend, where the cleanup function checks whether the backend data has been allocated at all -- if not, then there is nothing to clean up. For the files backend we do not check this and as a result will hit a segfault due to dereferencing a `NULL` pointer when cleaning up such a transaction.

Fix the issue by checking whether `backend_data` is set in the files backend, too.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)
Show changes to refs/files-backend.c +11 −9
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 561c33ac8a..6516c7bc8c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2565,17 +2565,19 @@ static void files_transaction_cleanup(struct files_ref_store *refs,
 		}
 	}
 
-	if (backend_data->packed_transaction &&
-	    ref_transaction_abort(backend_data->packed_transaction, &err)) {
-		error("error aborting transaction: %s", err.buf);
-		strbuf_release(&err);
+	if (backend_data) {
+		if (backend_data->packed_transaction &&
+		    ref_transaction_abort(backend_data->packed_transaction, &err)) {
+			error("error aborting transaction: %s", err.buf);
+			strbuf_release(&err);
+		}
+
+		if (backend_data->packed_refs_locked)
+			packed_refs_unlock(refs->packed_ref_store);
+
+		free(backend_data);
 	}
 
-	if (backend_data->packed_refs_locked)
-		packed_refs_unlock(refs->packed_ref_store);
-
-	free(backend_data);
-
 	transaction->state = REF_TRANSACTION_CLOSED;
 }
 

-- 
2.26.0
Junio C Hamano· Mar 27, 2020, 19:59 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 1/9] refs: fix segfault when aborting empty transaction

Patrick Steinhardt <ps@pks.im> writes:
Show 15 quoted lines
> When cleaning up a transaction that has no updates queued, then the
> transaction's backend data will not have been allocated. We correctly
> handle this for the packed backend, where the cleanup function checks
> whether the backend data has been allocated at all -- if not, then there
> is nothing to clean up. For the files backend we do not check this and
> as a result will hit a segfault due to dereferencing a `NULL` pointer
> when cleaning up such a transaction.
>
> Fix the issue by checking whether `backend_data` is set in the files
> backend, too.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs/files-backend.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
Makes sense.  Will queue.  Thanks.
Show 33 quoted lines
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 561c33ac8a..6516c7bc8c 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2565,17 +2565,19 @@ static void files_transaction_cleanup(struct files_ref_store *refs,
>  		}
>  	}
>  
> -	if (backend_data->packed_transaction &&
> -	    ref_transaction_abort(backend_data->packed_transaction, &err)) {
> -		error("error aborting transaction: %s", err.buf);
> -		strbuf_release(&err);
> +	if (backend_data) {
> +		if (backend_data->packed_transaction &&
> +		    ref_transaction_abort(backend_data->packed_transaction, &err)) {
> +			error("error aborting transaction: %s", err.buf);
> +			strbuf_release(&err);
> +		}
> +
> +		if (backend_data->packed_refs_locked)
> +			packed_refs_unlock(refs->packed_ref_store);
> +
> +		free(backend_data);
>  	}
>  
> -	if (backend_data->packed_refs_locked)
> -		packed_refs_unlock(refs->packed_ref_store);
> -
> -	free(backend_data);
> -
>  	transaction->state = REF_TRANSACTION_CLOSED;
>  }
Patrick Steinhardt· Mar 25, 2020, 09:53 UTC · re: Patrick Steinhardt · lore

[PATCH 2/9] git-update-ref.txt: add missing word

The description for the "verify" command is lacking a single word "is", which this commit corrects.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git-update-ref.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Documentation/git-update-ref.txt +1 −1
diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 9671423117..9bd039ce08 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -107,7 +107,7 @@ delete::
 
 verify::
 	Verify <ref> against <oldvalue> but do not change it.  If
-	<oldvalue> zero or missing, the ref must not exist.
+	<oldvalue> is zero or missing, the ref must not exist.
 
 option::
 	Modify behavior of the next command naming a <ref>.

-- 
2.26.0
Patrick Steinhardt· Mar 25, 2020, 09:53 UTC · re: Patrick Steinhardt · lore

[PATCH 3/9] strbuf: provide function to append whole lines

While the strbuf interface already provides functions to read a line into it that completely replaces its current contents, we do not have an interface that allows for appending lines without discarding current contents.

Add a new function `strbuf_appendwholeline` that reads a line including its terminating character into a strbuf non-destructively. This is a preparatory step for git-update-ref(1) reading standard input line-wise instead of as a block.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 strbuf.c | 10 ++++++++++
 strbuf.h |  6 ++++++
 2 files changed, 16 insertions(+)
Show changes to 2 files +16 −0

strbuf.c, strbuf.h

diff --git a/strbuf.c b/strbuf.c
index bb0065ccaf..6e74901bfa 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -690,6 +690,16 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 }
 #endif
 
+int strbuf_appendwholeline(struct strbuf *sb, FILE *fp, int term)
+{
+	struct strbuf line = STRBUF_INIT;
+	if (strbuf_getwholeline(&line, fp, term))
+		return EOF;
+	strbuf_addbuf(sb, &line);
+	strbuf_release(&line);
+	return 0;
+}
+
 static int strbuf_getdelim(struct strbuf *sb, FILE *fp, int term)
 {
 	if (strbuf_getwholeline(sb, fp, term))
diff --git a/strbuf.h b/strbuf.h
index ce8e49c0b2..411063ca76 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -502,6 +502,12 @@ int strbuf_getline(struct strbuf *sb, FILE *file);
  */
 int strbuf_getwholeline(struct strbuf *sb, FILE *file, int term);
 
+/**
+ * Like `strbuf_getwholeline`, but appends the line instead of
+ * resetting the buffer first.
+ */
+int strbuf_appendwholeline(struct strbuf *sb, FILE *file, int term);
+
 /**
  * Like `strbuf_getwholeline`, but operates on a file descriptor.
  * It reads one character at a time, so it is very slow.  Do not

-- 
2.26.0
Junio C Hamano· Mar 27, 2020, 21:04 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 3/9] strbuf: provide function to append whole lines

Patrick Steinhardt <ps@pks.im> writes:
Show 9 quoted lines
> While the strbuf interface already provides functions to read a line
> into it that completely replaces its current contents, we do not have an
> interface that allows for appending lines without discarding current
> contents.
>
> Add a new function `strbuf_appendwholeline` that reads a line including
> its terminating character into a strbuf non-destructively. This is a
> preparatory step for git-update-ref(1) reading standard input line-wise
> instead of as a block.

Hmph. If we were to gain more uses of this function over time, the necessity for an extra strbuf_addbuf() becomes annoying. I wonder if we should be doing this patch the other way around, i.e. move the whole implementation, except for the early

	if (feof(fp))
		return EOF;
	strbuf_reset(sb);

part, and call it strbuf_addwholeline(), and strbuf_getwholeline() becomes a thin wrapper around it that resets the incoming buffer before calling strbuf_addwholeline(). The logic to determine EOF would need to be updated if we go that route (i.e. instead of checking sb->len is zero in the !HAVE_GETDELIM side of the implementation, we need to see if the number is the same as before) but it should be minor.

There is a stale comment in the HAVE_GETDELIM side of the curent implementation, by the way. I think our xrealloc no longer tries to "recover" from ENOMEM.

Having said all that, I am fine with this version, at least for now.
Show 5 quoted lines
> +int strbuf_appendwholeline(struct strbuf *sb, FILE *fp, int term)
> +{
> +	struct strbuf line = STRBUF_INIT;
> +	if (strbuf_getwholeline(&line, fp, term))
> +		return EOF;
So, if caller wants to read the stream until it runs out, it can do
	strbuf_init(&sb);
	while (strbuf_appendwholeline(&sb, fp, '\n') != EOF)
		; /* keep going */

If the caller knows how many lines to read, EOF-return can be used only for error checking, e.g.

	strbuf_init(&sb);
	for (count = 0; count < 5; count++)
		if (strbuf_appendwholeline(&sb, fp, '\n') == EOF)
			die("cannot grab 5 lines");

It becomes cumbersome if the input lines are terminated, though. The caller wouldn't be using strbuf_appendwholeline() interface, e.g.

	strbuf_init(&accum);
	while ((strbuf_getwholeline(&sb, fp, '\n') != EOF) &&
               strcmp(sb.buf, "done\n"))
		strbuf_addbuf(&accum, &sb);

Anyway, I was primarily wondering if returning EOF, which perfectly makes sense for getwholeline(), still makes sense for addwholeline(), and it seems that it is still useful.

Show 25 quoted lines
> +	strbuf_addbuf(sb, &line);
> +	strbuf_release(&line);
> +	return 0;
> +}
> +
>  static int strbuf_getdelim(struct strbuf *sb, FILE *fp, int term)
>  {
>  	if (strbuf_getwholeline(sb, fp, term))
> diff --git a/strbuf.h b/strbuf.h
> index ce8e49c0b2..411063ca76 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -502,6 +502,12 @@ int strbuf_getline(struct strbuf *sb, FILE *file);
>   */
>  int strbuf_getwholeline(struct strbuf *sb, FILE *file, int term);
>  
> +/**
> + * Like `strbuf_getwholeline`, but appends the line instead of
> + * resetting the buffer first.
> + */
> +int strbuf_appendwholeline(struct strbuf *sb, FILE *file, int term);
> +
>  /**
>   * Like `strbuf_getwholeline`, but operates on a file descriptor.
>   * It reads one character at a time, so it is very slow.  Do not
Patrick Steinhardt· Mar 30, 2020, 13:25 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/9] strbuf: provide function to append whole lines

On Fri, Mar 27, 2020 at 02:04:18PM -0700, Junio C Hamano wrote:
Show 28 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > While the strbuf interface already provides functions to read a line
> > into it that completely replaces its current contents, we do not have an
> > interface that allows for appending lines without discarding current
> > contents.
> >
> > Add a new function `strbuf_appendwholeline` that reads a line including
> > its terminating character into a strbuf non-destructively. This is a
> > preparatory step for git-update-ref(1) reading standard input line-wise
> > instead of as a block.
> 
> Hmph.  If we were to gain more uses of this function over time, the
> necessity for an extra strbuf_addbuf() becomes annoying.  I wonder
> if we should be doing this patch the other way around, i.e. move the
> whole implementation, except for the early
> 
> 	if (feof(fp))
> 		return EOF;
> 	strbuf_reset(sb);
> 
> part, and call it strbuf_addwholeline(), and strbuf_getwholeline()
> becomes a thin wrapper around it that resets the incoming buffer
> before calling strbuf_addwholeline().  The logic to determine EOF
> would need to be updated if we go that route (i.e. instead of
> checking sb->len is zero in the !HAVE_GETDELIM side of the
> implementation, we need to see if the number is the same as before)
> but it should be minor.

Unfortunately it's not that easy for the HAVE_GETDELIM case, though, as we cannot call getdelim(3P) to append to an already populated buffer. So we'd have to call getdelim(3P) either on a NULL line and append manually or on an empty line in case the strbuf doesn't have any contents. While doable, I wanted to keep out this additional complexity for now.

Patrick
Junio C Hamano· Mar 30, 2020, 17:12 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 3/9] strbuf: provide function to append whole lines

Patrick Steinhardt <ps@pks.im> writes:
Show 5 quoted lines
> Unfortunately it's not that easy for the HAVE_GETDELIM case, though, as
> we cannot call getdelim(3P) to append to an already populated buffer. So
> we'd have to call getdelim(3P) either on a NULL line and append manually
> or on an empty line in case the strbuf doesn't have any contents. While
> doable, I wanted to keep out this additional complexity for now.
Ahh, I see.  Thanks for a clarification.
Patrick Steinhardt· Mar 25, 2020, 09:53 UTC · re: Patrick Steinhardt · lore

[PATCH 4/9] update-ref: organize commands in an array

We currently manually wire up all commands known to `git-update-ref --stdin`, making it harder than necessary to preprocess arguments after the command is determined. To make this more extensible, let's refactor the code to use an array of known commands instead. While this doesn't add a lot of value now, it is a preparatory step to implement line-wise reading of commands.

As we're going to introduce commands without trailing spaces, this commit also moves whitespace parsing into the respective commands.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 66 ++++++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 17 deletions(-)
Show changes to builtin/update-ref.c +49 −17
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 2d8f7f0578..d2b917a47c 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -171,11 +171,11 @@ static int parse_next_oid(struct strbuf *input, const char **next,
 /*
  * The following five parse_cmd_*() functions parse the corresponding
  * command.  In each case, next points at the character following the
- * command name and the following space.  They each return a pointer
- * to the character terminating the command, and die with an
- * explanatory message if there are any parsing problems.  All of
- * these functions handle either text or binary format input,
- * depending on how line_termination is set.
+ * command name.  They each return a pointer to the character
+ * terminating the command, and die with an explanatory message if
+ * there are any parsing problems.  All of these functions handle
+ * either text or binary format input, depending on how
+ * line_termination is set.
  */
 
 static const char *parse_cmd_update(struct ref_transaction *transaction,
@@ -186,6 +186,9 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 	struct object_id new_oid, old_oid;
 	int have_old;
 
+	if (!skip_prefix(next, " ", &next))
+		die("update: missing space after command");
+
 	refname = parse_refname(input, &next);
 	if (!refname)
 		die("update: missing <ref>");
@@ -220,6 +223,9 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 	char *refname;
 	struct object_id new_oid;
 
+	if (!skip_prefix(next, " ", &next))
+		die("create: missing space after command");
+
 	refname = parse_refname(input, &next);
 	if (!refname)
 		die("create: missing <ref>");
@@ -253,6 +259,9 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 	struct object_id old_oid;
 	int have_old;
 
+	if (!skip_prefix(next, " ", &next))
+		die("delete: missing space after command");
+
 	refname = parse_refname(input, &next);
 	if (!refname)
 		die("delete: missing <ref>");
@@ -288,6 +297,9 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 	char *refname;
 	struct object_id old_oid;
 
+	if (!skip_prefix(next, " ", &next))
+		die("verify: missing space after command");
+
 	refname = parse_refname(input, &next);
 	if (!refname)
 		die("verify: missing <ref>");
@@ -310,9 +322,12 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 	return next;
 }
 
-static const char *parse_cmd_option(struct strbuf *input, const char *next)
+static const char *parse_cmd_option(struct ref_transaction *transaction,
+				    struct strbuf *input, const char *next)
 {
 	const char *rest;
+	if (!skip_prefix(next, " ", &next))
+		die("option: missing space after command");
 	if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination)
 		update_flags |= REF_NO_DEREF;
 	else
@@ -320,33 +335,50 @@ static const char *parse_cmd_option(struct strbuf *input, const char *next)
 	return rest;
 }
 
+static const struct parse_cmd {
+	const char *prefix;
+	const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);
+} commands[] = {
+	{ "update", parse_cmd_update },
+	{ "create", parse_cmd_create },
+	{ "delete", parse_cmd_delete },
+	{ "verify", parse_cmd_verify },
+	{ "option", parse_cmd_option },
+};
+
 static void update_refs_stdin(struct ref_transaction *transaction)
 {
 	struct strbuf input = STRBUF_INIT;
 	const char *next;
+	int i;
 
 	if (strbuf_read(&input, 0, 1000) < 0)
 		die_errno("could not read from stdin");
 	next = input.buf;
 	/* Read each line dispatch its command */
 	while (next < input.buf + input.len) {
+		const struct parse_cmd *cmd = NULL;
+
 		if (*next == line_termination)
 			die("empty command in input");
 		else if (isspace(*next))
 			die("whitespace before command: %s", next);
-		else if (skip_prefix(next, "update ", &next))
-			next = parse_cmd_update(transaction, &input, next);
-		else if (skip_prefix(next, "create ", &next))
-			next = parse_cmd_create(transaction, &input, next);
-		else if (skip_prefix(next, "delete ", &next))
-			next = parse_cmd_delete(transaction, &input, next);
-		else if (skip_prefix(next, "verify ", &next))
-			next = parse_cmd_verify(transaction, &input, next);
-		else if (skip_prefix(next, "option ", &next))
-			next = parse_cmd_option(&input, next);
-		else
+
+		for (i = 0; i < ARRAY_SIZE(commands); i++) {
+			if (!skip_prefix(next, commands[i].prefix , &next))
+				continue;
+			cmd = &commands[i];
+			break;
+		}
+		if (!cmd)
 			die("unknown command: %s", next);
 
+		if (input.buf[strlen(cmd->prefix)] != line_termination &&
+		    input.buf[strlen(cmd->prefix)] != '\0' &&
+		    input.buf[strlen(cmd->prefix)] != ' ')
+			die("%s: no separator after command", cmd->prefix);
+
+		next = cmd->fn(transaction, &input, next);
 		next++;
 	}
 

-- 
2.26.0
Junio C Hamano· Mar 27, 2020, 21:25 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 4/9] update-ref: organize commands in an array

Patrick Steinhardt <ps@pks.im> writes:
> +static const struct parse_cmd {
> +	const char *prefix;
> +	const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);
> +} commands[] = {

Do not call an array the represents a set of THINGs "type things[]"; instead call it "type thing[]", so that the 0th thing can be referred to as thing[0], not things[0].

One exception is when the set as a whole is referred to more often than individual element of an array, in which case "things" (without the [index]) becomes a sensible way to refer to the set.

Show 42 quoted lines
> +	{ "update", parse_cmd_update },
> +	{ "create", parse_cmd_create },
> +	{ "delete", parse_cmd_delete },
> +	{ "verify", parse_cmd_verify },
> +	{ "option", parse_cmd_option },
> +};
> +
>  static void update_refs_stdin(struct ref_transaction *transaction)
>  {
>  	struct strbuf input = STRBUF_INIT;
>  	const char *next;
> +	int i;
>  
>  	if (strbuf_read(&input, 0, 1000) < 0)
>  		die_errno("could not read from stdin");
>  	next = input.buf;
>  	/* Read each line dispatch its command */
>  	while (next < input.buf + input.len) {
> +		const struct parse_cmd *cmd = NULL;
> +
>  		if (*next == line_termination)
>  			die("empty command in input");
>  		else if (isspace(*next))
>  			die("whitespace before command: %s", next);
> -		else if (skip_prefix(next, "update ", &next))
> -			next = parse_cmd_update(transaction, &input, next);
> -		else if (skip_prefix(next, "create ", &next))
> -			next = parse_cmd_create(transaction, &input, next);
> -		else if (skip_prefix(next, "delete ", &next))
> -			next = parse_cmd_delete(transaction, &input, next);
> -		else if (skip_prefix(next, "verify ", &next))
> -			next = parse_cmd_verify(transaction, &input, next);
> -		else if (skip_prefix(next, "option ", &next))
> -			next = parse_cmd_option(&input, next);
> -		else
> +
> +		for (i = 0; i < ARRAY_SIZE(commands); i++) {
> +			if (!skip_prefix(next, commands[i].prefix , &next))
> +				continue;
> +			cmd = &commands[i];
> +			break;
> +		}
The only reason why you had to sprinkle
	if (!skip_prefix(next, " ", &next))
		die(_("%s: missing space after command"), cmd);

all over the place is because the table lacks the trailing SP (which makes sense---after all, you are making a table of commands). In other words, it's not like some command called from this dispatcher would require " " after the command name and some others would not.

So why not avoid touching the parse_cmd_<cmd>() at all (except for the "option" thing that now needs to take the transaction object for uniformity), and then verify the presence of " " here, perhaps like this:

	for (i = 0; i < ARRAY_SIZE(command); i++) {
		const char *eoc;
		if (!skip_prefix(next, commands[i].prefix, &eoc) ||
		    *eoc != ' ')
			continue;
		cmd = &command[i];
                next = eoc;
		break;
	}

Note that you cannot reuse &next here to future-proof the code; otherwise, you wouldn't be able to add a new command, e.g. "options", that sits next to the existing command "option", in the future.

Show 7 quoted lines
> +		if (!cmd)
>  			die("unknown command: %s", next);
>  
> +		if (input.buf[strlen(cmd->prefix)] != line_termination &&
> +		    input.buf[strlen(cmd->prefix)] != '\0' &&
> +		    input.buf[strlen(cmd->prefix)] != ' ')
> +			die("%s: no separator after command", cmd->prefix);

This part of your version does not make sense to me. If the input line began with "update" with some separator that is not a SP, the original would not have matched. But with this code, "update\0" would pass this code and cause cmd->fn() to be called, only to get the input rejected, because next is not pointing to SP. So why do you even need this if statement?

> +
> +		next = cmd->fn(transaction, &input, next);
>  		next++;
>  	}
Thanks.
Patrick Steinhardt· Mar 30, 2020, 08:05 UTC · re: Junio C Hamano · lore

Re: [PATCH 4/9] update-ref: organize commands in an array

On Fri, Mar 27, 2020 at 02:25:34PM -0700, Junio C Hamano wrote:
Show 67 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > +static const struct parse_cmd {
> > +	const char *prefix;
> > +	const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);
> > +} commands[] = {
> 
> Do not call an array the represents a set of THINGs "type things[]";
> instead call it "type thing[]", so that the 0th thing can be
> referred to as thing[0], not things[0].
> 
> One exception is when the set as a whole is referred to more often
> than individual element of an array, in which case "things" (without
> the [index]) becomes a sensible way to refer to the set.
> 
> > +	{ "update", parse_cmd_update },
> > +	{ "create", parse_cmd_create },
> > +	{ "delete", parse_cmd_delete },
> > +	{ "verify", parse_cmd_verify },
> > +	{ "option", parse_cmd_option },
> > +};
> > +
> >  static void update_refs_stdin(struct ref_transaction *transaction)
> >  {
> >  	struct strbuf input = STRBUF_INIT;
> >  	const char *next;
> > +	int i;
> >  
> >  	if (strbuf_read(&input, 0, 1000) < 0)
> >  		die_errno("could not read from stdin");
> >  	next = input.buf;
> >  	/* Read each line dispatch its command */
> >  	while (next < input.buf + input.len) {
> > +		const struct parse_cmd *cmd = NULL;
> > +
> >  		if (*next == line_termination)
> >  			die("empty command in input");
> >  		else if (isspace(*next))
> >  			die("whitespace before command: %s", next);
> > -		else if (skip_prefix(next, "update ", &next))
> > -			next = parse_cmd_update(transaction, &input, next);
> > -		else if (skip_prefix(next, "create ", &next))
> > -			next = parse_cmd_create(transaction, &input, next);
> > -		else if (skip_prefix(next, "delete ", &next))
> > -			next = parse_cmd_delete(transaction, &input, next);
> > -		else if (skip_prefix(next, "verify ", &next))
> > -			next = parse_cmd_verify(transaction, &input, next);
> > -		else if (skip_prefix(next, "option ", &next))
> > -			next = parse_cmd_option(&input, next);
> > -		else
> > +
> > +		for (i = 0; i < ARRAY_SIZE(commands); i++) {
> > +			if (!skip_prefix(next, commands[i].prefix , &next))
> > +				continue;
> > +			cmd = &commands[i];
> > +			break;
> > +		}
> 
> The only reason why you had to sprinkle
> 
> 	if (!skip_prefix(next, " ", &next))
> 		die(_("%s: missing space after command"), cmd);
> 
> all over the place is because the table lacks the trailing SP (which
> makes sense---after all, you are making a table of commands).  In
> other words, it's not like some command called from this dispatcher
> would require " " after the command name and some others would not.
Show 14 quoted lines
> So why not avoid touching the parse_cmd_<cmd>() at all (except for
> the "option" thing that now needs to take the transaction object for
> uniformity), and then verify the presence of " " here, perhaps like
> this:
> 
> 	for (i = 0; i < ARRAY_SIZE(command); i++) {
> 		const char *eoc;
> 		if (!skip_prefix(next, commands[i].prefix, &eoc) ||
> 		    *eoc != ' ')
> 			continue;
> 		cmd = &command[i];
>                 next = eoc;
> 		break;
> 	}

The reason why I moved those `skip_prefix` calls into each of the respective commands is that this patch series introduces calls that do not accept a trailing space at all. Thus we cannot handle the space generically here, as that would was soon as we introduce the set of new commands.

Initially I just had a trailing " " in the `command` array as you hint at, but it felt a bit magical to me and might make readers wonder why some commands are spelt "update " and why some are spelt "commit", without a trailing space.

Show 18 quoted lines
> Note that you cannot reuse &next here to future-proof the code;
> otherwise, you wouldn't be able to add a new command, e.g. "options",
> that sits next to the existing command "option", in the future.
> 
> > +		if (!cmd)
> >  			die("unknown command: %s", next);
> >  
> > +		if (input.buf[strlen(cmd->prefix)] != line_termination &&
> > +		    input.buf[strlen(cmd->prefix)] != '\0' &&
> > +		    input.buf[strlen(cmd->prefix)] != ' ')
> > +			die("%s: no separator after command", cmd->prefix);
> 
> This part of your version does not make sense to me.  If the input
> line began with "update" with some separator that is not a SP, the
> original would not have matched.  But with this code, "update\0"
> would pass this code and cause cmd->fn() to be called, only to get
> the input rejected, because next is not pointing to SP.  So why do
> you even need this if statement?

I hope this makes more sense with the above background of commands without trailing space. It would probably make sense to move this into the command-matching loop though to be able to discern "option" and a potentially new "options" command in the future. Instead of dying, we'd then have to `continue` the loop and check whether any of the remaining commands matches.

Patrick
Junio C Hamano· Mar 30, 2020, 16:55 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 4/9] update-ref: organize commands in an array

Patrick Steinhardt <ps@pks.im> writes:
Show 15 quoted lines
>> 	for (i = 0; i < ARRAY_SIZE(command); i++) {
>> 		const char *eoc;
>> 		if (!skip_prefix(next, commands[i].prefix, &eoc) ||
>> 		    *eoc != ' ')
>> 			continue;
>> 		cmd = &command[i];
>>                 next = eoc;
>> 		break;
>> 	}
>
> The reason why I moved those `skip_prefix` calls into each of the
> respective commands is that this patch series introduces calls that do
> not accept a trailing space at all. Thus we cannot handle the space
> generically here, as that would was soon as we introduce the set of new
> commands.

That's not a good excuse, though, is it? The command[] structure can say "this takes parameters" or even "this takes N parameters", and the field being zero (i.e. "does not take parameters" or "takes zero parameters") would mean you do not want a trailing SP, no?

I also suspect that the "extra lines" thing we'd see in a later step is correlated with this, but we'll see.

Thanks.
Patrick Steinhardt· Mar 30, 2020, 17:37 UTC · re: Junio C Hamano · lore

Re: [PATCH 4/9] update-ref: organize commands in an array

On Mon, Mar 30, 2020 at 09:55:44AM -0700, Junio C Hamano wrote:
Show 27 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> 	for (i = 0; i < ARRAY_SIZE(command); i++) {
> >> 		const char *eoc;
> >> 		if (!skip_prefix(next, commands[i].prefix, &eoc) ||
> >> 		    *eoc != ' ')
> >> 			continue;
> >> 		cmd = &command[i];
> >>                 next = eoc;
> >> 		break;
> >> 	}
> >
> > The reason why I moved those `skip_prefix` calls into each of the
> > respective commands is that this patch series introduces calls that do
> > not accept a trailing space at all. Thus we cannot handle the space
> > generically here, as that would was soon as we introduce the set of new
> > commands.
> 
> That's not a good excuse, though, is it?  The command[] structure
> can say "this takes parameters" or even "this takes N parameters",
> and the field being zero (i.e. "does not take parameters" or "takes
> zero parameters") would mean you do not want a trailing SP, no?
> 
> I also suspect that the "extra lines" thing we'd see in a later step
> is correlated with this, but we'll see.
> 
> Thanks.

You've got a point there. I'll convert this for the next version, thanks!

Patrick
Patrick Steinhardt· Mar 25, 2020, 09:54 UTC · re: Patrick Steinhardt · lore

[PATCH 5/9] update-ref: drop unused argument for `parse_refname`

The `parse_refname` function accepts a `struct strbuf *input` argument that isn't used at all. As we're about to convert commands to not use a strbuf anymore but instead an end pointer, let's drop this argument now to make the converting commit easier to review.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to builtin/update-ref.c +5 −5
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index d2b917a47c..6f4e8e88c1 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -50,7 +50,7 @@ static const char *parse_arg(const char *next, struct strbuf *arg)
  * the argument.  Die if C-quoting is malformed or the reference name
  * is invalid.
  */
-static char *parse_refname(struct strbuf *input, const char **next)
+static char *parse_refname(const char **next)
 {
 	struct strbuf ref = STRBUF_INIT;
 
@@ -189,7 +189,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 	if (!skip_prefix(next, " ", &next))
 		die("update: missing space after command");
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("update: missing <ref>");
 
@@ -226,7 +226,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 	if (!skip_prefix(next, " ", &next))
 		die("create: missing space after command");
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("create: missing <ref>");
 
@@ -262,7 +262,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 	if (!skip_prefix(next, " ", &next))
 		die("delete: missing space after command");
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("delete: missing <ref>");
 
@@ -300,7 +300,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 	if (!skip_prefix(next, " ", &next))
 		die("verify: missing space after command");
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("verify: missing <ref>");
 

-- 
2.26.0
Patrick Steinhardt· Mar 25, 2020, 09:54 UTC · re: Patrick Steinhardt · lore

[PATCH 6/9] update-ref: pass end pointer instead of strbuf

We currently pass both an `strbuf` containing the current command line as well as the `next` pointer pointing to the first argument to commands. This is both confusing and makes code more intertwined. Convert this to use a simple pointer as well as a pointer pointing to the end of the input as a preparatory step to line-wise reading of stdin.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)
Show changes to builtin/update-ref.c +15 −15
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 6f4e8e88c1..6f036ae765 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -95,7 +95,7 @@ static char *parse_refname(const char **next)
  * provided but cannot be converted to a SHA-1, die.  flags can
  * include PARSE_SHA1_OLD and/or PARSE_SHA1_ALLOW_EMPTY.
  */
-static int parse_next_oid(struct strbuf *input, const char **next,
+static int parse_next_oid(const char **next, const char *end,
 			  struct object_id *oid,
 			  const char *command, const char *refname,
 			  int flags)
@@ -103,7 +103,7 @@ static int parse_next_oid(struct strbuf *input, const char **next,
 	struct strbuf arg = STRBUF_INIT;
 	int ret = 0;
 
-	if (*next == input->buf + input->len)
+	if (*next == end)
 		goto eof;
 
 	if (line_termination) {
@@ -128,7 +128,7 @@ static int parse_next_oid(struct strbuf *input, const char **next,
 			die("%s %s: expected NUL but got: %s",
 			    command, refname, *next);
 		(*next)++;
-		if (*next == input->buf + input->len)
+		if (*next == end)
 			goto eof;
 		strbuf_addstr(&arg, *next);
 		*next += arg.len;
@@ -179,7 +179,7 @@ static int parse_next_oid(struct strbuf *input, const char **next,
  */
 
 static const char *parse_cmd_update(struct ref_transaction *transaction,
-				    struct strbuf *input, const char *next)
+				    const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -193,11 +193,11 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 	if (!refname)
 		die("update: missing <ref>");
 
-	if (parse_next_oid(input, &next, &new_oid, "update", refname,
+	if (parse_next_oid(&next, end, &new_oid, "update", refname,
 			   PARSE_SHA1_ALLOW_EMPTY))
 		die("update %s: missing <newvalue>", refname);
 
-	have_old = !parse_next_oid(input, &next, &old_oid, "update", refname,
+	have_old = !parse_next_oid(&next, end, &old_oid, "update", refname,
 				   PARSE_SHA1_OLD);
 
 	if (*next != line_termination)
@@ -217,7 +217,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 }
 
 static const char *parse_cmd_create(struct ref_transaction *transaction,
-				    struct strbuf *input, const char *next)
+				    const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -230,7 +230,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 	if (!refname)
 		die("create: missing <ref>");
 
-	if (parse_next_oid(input, &next, &new_oid, "create", refname, 0))
+	if (parse_next_oid(&next, end, &new_oid, "create", refname, 0))
 		die("create %s: missing <newvalue>", refname);
 
 	if (is_null_oid(&new_oid))
@@ -252,7 +252,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 }
 
 static const char *parse_cmd_delete(struct ref_transaction *transaction,
-				    struct strbuf *input, const char *next)
+				    const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -266,7 +266,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 	if (!refname)
 		die("delete: missing <ref>");
 
-	if (parse_next_oid(input, &next, &old_oid, "delete", refname,
+	if (parse_next_oid(&next, end, &old_oid, "delete", refname,
 			   PARSE_SHA1_OLD)) {
 		have_old = 0;
 	} else {
@@ -291,7 +291,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 }
 
 static const char *parse_cmd_verify(struct ref_transaction *transaction,
-				    struct strbuf *input, const char *next)
+				    const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -304,7 +304,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 	if (!refname)
 		die("verify: missing <ref>");
 
-	if (parse_next_oid(input, &next, &old_oid, "verify", refname,
+	if (parse_next_oid(&next, end, &old_oid, "verify", refname,
 			   PARSE_SHA1_OLD))
 		oidclr(&old_oid);
 
@@ -323,7 +323,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 }
 
 static const char *parse_cmd_option(struct ref_transaction *transaction,
-				    struct strbuf *input, const char *next)
+				    const char *next, const char *end)
 {
 	const char *rest;
 	if (!skip_prefix(next, " ", &next))
@@ -337,7 +337,7 @@ static const char *parse_cmd_option(struct ref_transaction *transaction,
 
 static const struct parse_cmd {
 	const char *prefix;
-	const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);
+	const char *(*fn)(struct ref_transaction *, const char *, const char *);
 } commands[] = {
 	{ "update", parse_cmd_update },
 	{ "create", parse_cmd_create },
@@ -378,7 +378,7 @@ static void update_refs_stdin(struct ref_transaction *transaction)
 		    input.buf[strlen(cmd->prefix)] != ' ')
 			die("%s: no separator after command", cmd->prefix);
 
-		next = cmd->fn(transaction, &input, next);
+		next = cmd->fn(transaction, next, input.buf + input.len);
 		next++;
 	}
 

-- 
2.26.0
Patrick Steinhardt· Mar 25, 2020, 09:54 UTC · re: Patrick Steinhardt · lore

[PATCH 7/9] update-ref: move transaction handling into `update_refs_stdin()`

While the actual logic to update the transaction is handled in `update_refs_stdin()`, the transaction itself is started and committed in `cmd_update_ref()` itself. This makes it hard to handle transaction abortion and commits as part of `update_refs_stdin()` itself, which is required in order to introduce transaction handling features to `git update-refs --stdin`.

Refactor the code to move all transaction handling into `update_refs_stdin()` to prepare for transaction handling features.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)
Show changes to builtin/update-ref.c +14 −13
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 6f036ae765..5f97bb0b75 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -346,15 +346,21 @@ static const struct parse_cmd {
 	{ "option", parse_cmd_option },
 };
 
-static void update_refs_stdin(struct ref_transaction *transaction)
+static void update_refs_stdin(void)
 {
-	struct strbuf input = STRBUF_INIT;
+	struct strbuf input = STRBUF_INIT, err = STRBUF_INIT;
+	struct ref_transaction *transaction;
 	const char *next;
 	int i;
 
 	if (strbuf_read(&input, 0, 1000) < 0)
 		die_errno("could not read from stdin");
 	next = input.buf;
+
+	transaction = ref_transaction_begin(&err);
+	if (!transaction)
+		die("%s", err.buf);
+
 	/* Read each line dispatch its command */
 	while (next < input.buf + input.len) {
 		const struct parse_cmd *cmd = NULL;
@@ -382,6 +388,11 @@ static void update_refs_stdin(struct ref_transaction *transaction)
 		next++;
 	}
 
+	if (ref_transaction_commit(transaction, &err))
+		die("%s", err.buf);
+
+	ref_transaction_free(transaction);
+	strbuf_release(&err);
 	strbuf_release(&input);
 }
 
@@ -416,21 +427,11 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 	}
 
 	if (read_stdin) {
-		struct strbuf err = STRBUF_INIT;
-		struct ref_transaction *transaction;
-
-		transaction = ref_transaction_begin(&err);
-		if (!transaction)
-			die("%s", err.buf);
 		if (delete || argc > 0)
 			usage_with_options(git_update_ref_usage, options);
 		if (end_null)
 			line_termination = '\0';
-		update_refs_stdin(transaction);
-		if (ref_transaction_commit(transaction, &err))
-			die("%s", err.buf);
-		ref_transaction_free(transaction);
-		strbuf_release(&err);
+		update_refs_stdin();
 		return 0;
 	}
 

-- 
2.26.0
Junio C Hamano· Mar 27, 2020, 21:44 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 7/9] update-ref: move transaction handling into `update_refs_stdin()`

Patrick Steinhardt <ps@pks.im> writes:
> While the actual logic to update the transaction is handled in
> `update_refs_stdin()`, the transaction itself is started and committed
> in `cmd_update_ref()` itself. This makes it hard to handle transaction
> abortion and commits as part of `update_refs_stdin()` itself, ...

It is not offhand clear why it makes any difference, especially because there is only a single caller of update_refs_stdin() function, but let's see how this move makes things easier in [8/9] and [9/9].

The patch itself looks like a bug-free no-op change.
Thanks.
Patrick Steinhardt· Mar 25, 2020, 09:54 UTC · re: Patrick Steinhardt · lore

[PATCH 8/9] update-ref: read commands in a line-wise fashion

The git-update-ref(1) supports a `--stdin` mode that allows it to read all reference updates from standard input. This is mainly used to allow for atomic reference updates that are all or nothing, so that either all references will get updated or none.

Currently, git-update-ref(1) reads all commands as a single block of up to 1000 characters and only starts processing after stdin gets closed. This is less flexible than one might wish for, as it doesn't really allow for longer-lived transactions and doesn't allow any verification without committing everything. E.g. one may imagine the following exchange:

    > start
    < start: ok
    > update refs/heads/master $NEWOID1 $OLDOID1
    > update refs/heads/branch $NEWOID2 $OLDOID2
    > prepare
    < prepare: ok
    > commit
    < commit: ok

When reading all input as a whole block, the above interactive protocol is obviously impossible to achieve. But by converting the command to read commands linewise, we can make it more interactive than before.

Obviously, the linewise interface is only a first step in making git-update-ref(1) work in a more transaction-oriented way. Missing is most importantly support for transactional commands that manage the current transaction.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 70 ++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 39 deletions(-)
Show changes to builtin/update-ref.c +31 −39
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 5f97bb0b75..f35e3ca5ae 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -178,8 +178,8 @@ static int parse_next_oid(const char **next, const char *end,
  * line_termination is set.
  */
 
-static const char *parse_cmd_update(struct ref_transaction *transaction,
-				    const char *next, const char *end)
+static void parse_cmd_update(struct ref_transaction *transaction,
+			     const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -212,12 +212,10 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 	update_flags = default_flags;
 	free(refname);
 	strbuf_release(&err);
-
-	return next;
 }
 
-static const char *parse_cmd_create(struct ref_transaction *transaction,
-				    const char *next, const char *end)
+static void parse_cmd_create(struct ref_transaction *transaction,
+			     const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -247,12 +245,10 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 	update_flags = default_flags;
 	free(refname);
 	strbuf_release(&err);
-
-	return next;
 }
 
-static const char *parse_cmd_delete(struct ref_transaction *transaction,
-				    const char *next, const char *end)
+static void parse_cmd_delete(struct ref_transaction *transaction,
+			     const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -286,12 +282,10 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 	update_flags = default_flags;
 	free(refname);
 	strbuf_release(&err);
-
-	return next;
 }
 
-static const char *parse_cmd_verify(struct ref_transaction *transaction,
-				    const char *next, const char *end)
+static void parse_cmd_verify(struct ref_transaction *transaction,
+			     const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -318,12 +312,10 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 	update_flags = default_flags;
 	free(refname);
 	strbuf_release(&err);
-
-	return next;
 }
 
-static const char *parse_cmd_option(struct ref_transaction *transaction,
-				    const char *next, const char *end)
+static void parse_cmd_option(struct ref_transaction *transaction,
+			     const char *next, const char *end)
 {
 	const char *rest;
 	if (!skip_prefix(next, " ", &next))
@@ -332,60 +324,60 @@ static const char *parse_cmd_option(struct ref_transaction *transaction,
 		update_flags |= REF_NO_DEREF;
 	else
 		die("option unknown: %s", next);
-	return rest;
 }
 
 static const struct parse_cmd {
 	const char *prefix;
-	const char *(*fn)(struct ref_transaction *, const char *, const char *);
+	void (*fn)(struct ref_transaction *, const char *, const char *);
+	unsigned extra_lines;
 } commands[] = {
-	{ "update", parse_cmd_update },
-	{ "create", parse_cmd_create },
-	{ "delete", parse_cmd_delete },
-	{ "verify", parse_cmd_verify },
-	{ "option", parse_cmd_option },
+	{ "update", parse_cmd_update, 2 },
+	{ "create", parse_cmd_create, 1 },
+	{ "delete", parse_cmd_delete, 1 },
+	{ "verify", parse_cmd_verify, 1 },
+	{ "option", parse_cmd_option, 0 },
 };
 
 static void update_refs_stdin(void)
 {
 	struct strbuf input = STRBUF_INIT, err = STRBUF_INIT;
 	struct ref_transaction *transaction;
-	const char *next;
-	int i;
-
-	if (strbuf_read(&input, 0, 1000) < 0)
-		die_errno("could not read from stdin");
-	next = input.buf;
+	int i, j;
 
 	transaction = ref_transaction_begin(&err);
 	if (!transaction)
 		die("%s", err.buf);
 
 	/* Read each line dispatch its command */
-	while (next < input.buf + input.len) {
+	while (!strbuf_getwholeline(&input, stdin, line_termination)) {
 		const struct parse_cmd *cmd = NULL;
 
-		if (*next == line_termination)
+		if (*input.buf == line_termination)
 			die("empty command in input");
-		else if (isspace(*next))
-			die("whitespace before command: %s", next);
+		else if (isspace(*input.buf))
+			die("whitespace before command: %s", input.buf);
 
 		for (i = 0; i < ARRAY_SIZE(commands); i++) {
-			if (!skip_prefix(next, commands[i].prefix , &next))
+			if (!starts_with(input.buf, commands[i].prefix))
 				continue;
 			cmd = &commands[i];
 			break;
 		}
 		if (!cmd)
-			die("unknown command: %s", next);
+			die("unknown command: %s", input.buf);
 
 		if (input.buf[strlen(cmd->prefix)] != line_termination &&
 		    input.buf[strlen(cmd->prefix)] != '\0' &&
 		    input.buf[strlen(cmd->prefix)] != ' ')
 			die("%s: no separator after command", cmd->prefix);
 
-		next = cmd->fn(transaction, next, input.buf + input.len);
-		next++;
+		/* Read extra lines if NUL-terminated */
+		for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++)
+			if (strbuf_appendwholeline(&input, stdin, line_termination))
+				break;
+
+		cmd->fn(transaction, input.buf + strlen(cmd->prefix),
+			input.buf + input.len);
 	}
 
 	if (ref_transaction_commit(transaction, &err))

-- 
2.26.0
Junio C Hamano· Mar 27, 2020, 21:58 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 8/9] update-ref: read commands in a line-wise fashion

Patrick Steinhardt <ps@pks.im> writes:
Show 5 quoted lines
>  static const struct parse_cmd {
>  	const char *prefix;
> -	const char *(*fn)(struct ref_transaction *, const char *, const char *);
> +	void (*fn)(struct ref_transaction *, const char *, const char *);
> +	unsigned extra_lines;

Define and explain the meaning of extra_lines in a comment. Without it, the most natural way to infer what the variable means by readers would be that "update" command sometimes receives up to two more lines but it also is perfectly normal if there is no extra line.

But I am not sure if that is what is going on.

"update" _always_ needs exactly two more input "lines" when the input is NUL delimited, perhaps, and it is an _error_ if there are not these two lines, no?

The name of the field should make such details clear.
Show 16 quoted lines
>  } commands[] = {
> -	{ "update", parse_cmd_update },
> -	{ "create", parse_cmd_create },
> -	{ "delete", parse_cmd_delete },
> -	{ "verify", parse_cmd_verify },
> -	{ "option", parse_cmd_option },
> +	{ "update", parse_cmd_update, 2 },
> +	{ "create", parse_cmd_create, 1 },
> +	{ "delete", parse_cmd_delete, 1 },
> +	{ "verify", parse_cmd_verify, 1 },
> +	{ "option", parse_cmd_option, 0 },
>  };
> +		/* Read extra lines if NUL-terminated */
> +		for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++)
> +			if (strbuf_appendwholeline(&input, stdin, line_termination))
> +				break;

And this code does not barf if you get a premature EOF and let the call to cmd->fn() go through, which sounds buggy.

Show 5 quoted lines
> +		cmd->fn(transaction, input.buf + strlen(cmd->prefix),
> +			input.buf + input.len);
>  	}
>  
>  	if (ref_transaction_commit(transaction, &err))
Thanks.
Patrick Steinhardt· Mar 30, 2020, 08:11 UTC · re: Junio C Hamano · lore

Re: [PATCH 8/9] update-ref: read commands in a line-wise fashion

On Fri, Mar 27, 2020 at 02:58:38PM -0700, Junio C Hamano wrote:
Show 18 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >  static const struct parse_cmd {
> >  	const char *prefix;
> > -	const char *(*fn)(struct ref_transaction *, const char *, const char *);
> > +	void (*fn)(struct ref_transaction *, const char *, const char *);
> > +	unsigned extra_lines;
> 
> Define and explain the meaning of extra_lines in a comment.  Without
> it, the most natural way to infer what the variable means by readers
> would be that "update" command sometimes receives up to two more
> lines but it also is perfectly normal if there is no extra line.
> 
> But I am not sure if that is what is going on.
> 
> "update" _always_ needs exactly two more input "lines" when the
> input is NUL delimited, perhaps, and it is an _error_ if there are
> not these two lines, no?

That's extactly right. I pondered on a good name, but I wasn't yet able to come up with any one that brings across its meaning. I originally had `extra_args` but changed it after some internal discussion with Chris.

Show 21 quoted lines
> The name of the field should make such details clear.
> 
> >  } commands[] = {
> > -	{ "update", parse_cmd_update },
> > -	{ "create", parse_cmd_create },
> > -	{ "delete", parse_cmd_delete },
> > -	{ "verify", parse_cmd_verify },
> > -	{ "option", parse_cmd_option },
> > +	{ "update", parse_cmd_update, 2 },
> > +	{ "create", parse_cmd_create, 1 },
> > +	{ "delete", parse_cmd_delete, 1 },
> > +	{ "verify", parse_cmd_verify, 1 },
> > +	{ "option", parse_cmd_option, 0 },
> >  };
> > +		/* Read extra lines if NUL-terminated */
> > +		for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++)
> > +			if (strbuf_appendwholeline(&input, stdin, line_termination))
> > +				break;
> 
> And this code does not barf if you get a premature EOF and let the
> call to cmd->fn() go through, which sounds buggy.

This is in fact by design, but I only change the comment in a later commit. The reasoning is that by passing even incomplete buffers to a command, the command is going to be able to print a much better error message than printing a generic one here.

I'll make sure to move over the comment.
Patrick
Junio C Hamano· Mar 30, 2020, 17:39 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 8/9] update-ref: read commands in a line-wise fashion

Patrick Steinhardt <ps@pks.im> writes:
Show 23 quoted lines
> On Fri, Mar 27, 2020 at 02:58:38PM -0700, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> 
>> >  static const struct parse_cmd {
>> >  	const char *prefix;
>> > -	const char *(*fn)(struct ref_transaction *, const char *, const char *);
>> > +	void (*fn)(struct ref_transaction *, const char *, const char *);
>> > +	unsigned extra_lines;
>> 
>> Define and explain the meaning of extra_lines in a comment.  Without
>> it, the most natural way to infer what the variable means by readers
>> would be that "update" command sometimes receives up to two more
>> lines but it also is perfectly normal if there is no extra line.
>> 
>> But I am not sure if that is what is going on.
>> 
>> "update" _always_ needs exactly two more input "lines" when the
>> input is NUL delimited, perhaps, and it is an _error_ if there are
>> not these two lines, no?
>
> That's extactly right. I pondered on a good name, but I wasn't yet able
> to come up with any one that brings across its meaning. I originally had
> `extra_args` but changed it after some internal discussion with Chris.

Wouldn't it make more sense to store the number of args expected here, not "extra"? The one that does not take any then can say "I am subcommand X and I do not want a SP after my name".

The "sometimes after the command there is line.termination, and sometimes there is NUL, and yet some other times there is SP", which is a sloppy way to check malformed input (i.e. you do not want SP for a command that does not take parameters), that you have after the loop can then go, if you did the parsing that way.

Patrick Steinhardt· Mar 25, 2020, 09:54 UTC · re: Patrick Steinhardt · lore

[PATCH 9/9] update-ref: implement interactive transaction handling

The git-update-ref(1) command can only handle queueing transactions right now via its "--stdin" parameter, but there is no way for users to handle the transaction itself in a more explicit way. E.g. in a replicated scenario, one may imagine a coordinator that spawns git-update-ref(1) for multiple repositories and only if all agree that an update is possible will the coordinator send a commit. Such a transactional session could look like

    > start
    < start: ok
    > update refs/heads/master $OLD $NEW
    > prepare
    < prepare: ok
    # All nodes have returned "ok"
    > commit
    < commit: ok
or
    > start
    < start: ok
    > create refs/heads/master $OLD $NEW
    > prepare
    < fatal: cannot lock ref 'refs/heads/master': reference already exists
    # On all other nodes:
    > abort
    < abort: ok

In order to allow for such transactional sessions, this commit introduces four new commands for git-update-ref(1), which matches those we have internally already with the exception of "start":

    - start: start a new transaction
    - prepare: prepare the transaction, that is try to lock all
               references and verify their current value matches the
               expected one
    - commit: explicitly commit a session, that is update references to
              match their new expected state
    - abort: abort a session and roll back all changes

By design, git-update-ref(1) will commit as soon as standard input is being closed. While fine in a non-transactional world, it is definitely unexpected in a transactional world. Because of this, as soon as any of the new transactional commands is used, the default will change to aborting without an explicit "commit". To avoid a race between queueing updates and the first "prepare" that starts a transaction, the "start" command has been added to start an explicit transaction.

Add some tests to exercise this new functionality.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git-update-ref.txt |  26 ++++++
 builtin/update-ref.c             | 108 ++++++++++++++++++++++---
 t/t1400-update-ref.sh            | 131 +++++++++++++++++++++++++++++++
 3 files changed, 256 insertions(+), 9 deletions(-)
Show changes to 3 files +256 −9

Documentation/git-update-ref.txt, builtin/update-ref.c, t/t1400-update-ref.sh

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 9bd039ce08..3e737c2360 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -66,6 +66,10 @@ performs all modifications together.  Specify commands of the form:
 	delete SP <ref> [SP <oldvalue>] LF
 	verify SP <ref> [SP <oldvalue>] LF
 	option SP <opt> LF
+	start LF
+	prepare LF
+	commit LF
+	abort LF
 
 With `--create-reflog`, update-ref will create a reflog for each ref
 even if one would not ordinarily be created.
@@ -83,6 +87,10 @@ quoting:
 	delete SP <ref> NUL [<oldvalue>] NUL
 	verify SP <ref> NUL [<oldvalue>] NUL
 	option SP <opt> NUL
+	start NUL
+	prepare NUL
+	commit NUL
+	abort NUL
 
 In this format, use 40 "0" to specify a zero value, and use the empty
 string to specify a missing value.
@@ -114,6 +122,24 @@ option::
 	The only valid option is `no-deref` to avoid dereferencing
 	a symbolic ref.
 
+start::
+	Start a transaction. In contrast to a non-transactional session, a
+	transaction will automatically abort if the session ends without an
+	explicit commit.
+
+prepare::
+	Prepare to commit the transaction. This will create lock files for all
+	queued reference updates. If one reference could not be locked, the
+	transaction will be aborted.
+
+commit::
+	Commit all reference updates queued for the transaction, ending the
+	transaction.
+
+abort::
+	Abort the transaction, releasing all locks if the transaction is in
+	prepared state.
+
 If all <ref>s can be locked with matching <oldvalue>s
 simultaneously, all modifications are performed.  Otherwise, no
 modifications are performed.  Note that while each individual
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index f35e3ca5ae..6b870507e0 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -326,21 +326,80 @@ static void parse_cmd_option(struct ref_transaction *transaction,
 		die("option unknown: %s", next);
 }
 
+static void parse_cmd_start(struct ref_transaction *transaction,
+			    const char *next, const char *end)
+{
+	if (*next != line_termination)
+		die("start: extra input: %s", next);
+	puts("start: ok");
+}
+
+static void parse_cmd_prepare(struct ref_transaction *transaction,
+			      const char *next, const char *end)
+{
+	struct strbuf error = STRBUF_INIT;
+	if (*next != line_termination)
+		die("prepare: extra input: %s", next);
+	if (ref_transaction_prepare(transaction, &error))
+		die("prepare: %s", error.buf);
+	puts("prepare: ok");
+}
+
+static void parse_cmd_abort(struct ref_transaction *transaction,
+			    const char *next, const char *end)
+{
+	struct strbuf error = STRBUF_INIT;
+	if (*next != line_termination)
+		die("abort: extra input: %s", next);
+	if (ref_transaction_abort(transaction, &error))
+		die("abort: %s", error.buf);
+	puts("abort: ok");
+}
+
+static void parse_cmd_commit(struct ref_transaction *transaction,
+			     const char *next, const char *end)
+{
+	struct strbuf error = STRBUF_INIT;
+	if (*next != line_termination)
+		die("commit: extra input: %s", next);
+	if (ref_transaction_commit(transaction, &error))
+		die("commit: %s", error.buf);
+	puts("commit: ok");
+	ref_transaction_free(transaction);
+}
+
+enum update_refs_state {
+	/* Non-transactional state open for updates. */
+	UPDATE_REFS_OPEN,
+	/* A transaction has been started. */
+	UPDATE_REFS_STARTED,
+	/* References are locked and ready for commit */
+	UPDATE_REFS_PREPARED,
+	/* Transaction has been committed or closed. */
+	UPDATE_REFS_CLOSED,
+};
+
 static const struct parse_cmd {
 	const char *prefix;
 	void (*fn)(struct ref_transaction *, const char *, const char *);
 	unsigned extra_lines;
+	enum update_refs_state state;
 } commands[] = {
-	{ "update", parse_cmd_update, 2 },
-	{ "create", parse_cmd_create, 1 },
-	{ "delete", parse_cmd_delete, 1 },
-	{ "verify", parse_cmd_verify, 1 },
-	{ "option", parse_cmd_option, 0 },
+	{ "update",  parse_cmd_update,  2, UPDATE_REFS_OPEN },
+	{ "create",  parse_cmd_create,  1, UPDATE_REFS_OPEN },
+	{ "delete",  parse_cmd_delete,  1, UPDATE_REFS_OPEN },
+	{ "verify",  parse_cmd_verify,  1, UPDATE_REFS_OPEN },
+	{ "option",  parse_cmd_option,  0, UPDATE_REFS_OPEN },
+	{ "start",   parse_cmd_start,   0, UPDATE_REFS_STARTED },
+	{ "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED },
+	{ "abort",   parse_cmd_abort,   0, UPDATE_REFS_CLOSED },
+	{ "commit",  parse_cmd_commit,  0, UPDATE_REFS_CLOSED },
 };
 
 static void update_refs_stdin(void)
 {
 	struct strbuf input = STRBUF_INIT, err = STRBUF_INIT;
+	enum update_refs_state state = UPDATE_REFS_OPEN;
 	struct ref_transaction *transaction;
 	int i, j;
 
@@ -371,19 +430,50 @@ static void update_refs_stdin(void)
 		    input.buf[strlen(cmd->prefix)] != ' ')
 			die("%s: no separator after command", cmd->prefix);
 
-		/* Read extra lines if NUL-terminated */
+		/* Read extra lines if NUL-terminated, but let commands handle missing ones. */
 		for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++)
 			if (strbuf_appendwholeline(&input, stdin, line_termination))
 				break;
 
+		switch (state) {
+		case UPDATE_REFS_OPEN:
+		case UPDATE_REFS_STARTED:
+			/* Do not downgrade a transaction to a non-transaction. */
+			if (cmd->state >= state)
+				state = cmd->state;
+			break;
+		case UPDATE_REFS_PREPARED:
+			if (cmd->state != UPDATE_REFS_CLOSED)
+				die("prepared transactions can only be closed");
+			state = cmd->state;
+			break;
+		case UPDATE_REFS_CLOSED:
+			die("transaction is closed");
+			break;
+		}
+
 		cmd->fn(transaction, input.buf + strlen(cmd->prefix),
 			input.buf + input.len);
 	}
 
-	if (ref_transaction_commit(transaction, &err))
-		die("%s", err.buf);
+	switch (state) {
+	case UPDATE_REFS_OPEN:
+		/* Commit by default if no transaction was requested. */
+		if (ref_transaction_commit(transaction, &err))
+			die("%s", err.buf);
+		ref_transaction_free(transaction);
+		break;
+	case UPDATE_REFS_STARTED:
+	case UPDATE_REFS_PREPARED:
+		/* If using a transaction, we want to abort it. */
+		if (ref_transaction_abort(transaction, &err))
+			die("%s", err.buf);
+		break;
+	case UPDATE_REFS_CLOSED:
+		/* Otherwise no need to do anything, the transaction was closed already. */
+		break;
+	}
 
-	ref_transaction_free(transaction);
 	strbuf_release(&err);
 	strbuf_release(&input);
 }
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index a6224ef65f..48d0d42afd 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1404,4 +1404,135 @@ test_expect_success 'handle per-worktree refs in refs/bisect' '
 	! test_cmp main-head worktree-head
 '
 
+test_expect_success 'transaction handles empty commit' '
+	cat >stdin <<-EOF &&
+	start
+	prepare
+	commit
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start prepare commit >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction handles empty commit with missing prepare' '
+	cat >stdin <<-EOF &&
+	start
+	commit
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start commit >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction handles sole commit' '
+	cat >stdin <<-EOF &&
+	commit
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" commit >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction handles empty abort' '
+	cat >stdin <<-EOF &&
+	start
+	prepare
+	abort
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start prepare abort >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction exits on multiple aborts' '
+	cat >stdin <<-EOF &&
+	abort
+	abort
+	EOF
+	test_must_fail git update-ref --stdin <stdin >actual 2>err &&
+	printf "%s: ok\n" abort >expect &&
+	test_cmp expect actual &&
+	grep "fatal: transaction is closed" err
+'
+
+test_expect_success 'transaction exits on start after prepare' '
+	cat >stdin <<-EOF &&
+	prepare
+	start
+	EOF
+	test_must_fail git update-ref --stdin <stdin 2>err >actual &&
+	printf "%s: ok\n" prepare >expect &&
+	test_cmp expect actual &&
+	grep "fatal: prepared transactions can only be closed" err
+'
+
+test_expect_success 'transaction handles empty abort with missing prepare' '
+	cat >stdin <<-EOF &&
+	start
+	abort
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start abort >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction handles sole abort' '
+	cat >stdin <<-EOF &&
+	abort
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" abort >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction can handle commit' '
+	cat >stdin <<-EOF &&
+	start
+	create $a HEAD
+	commit
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start commit >expect &&
+	test_cmp expect actual &&
+	git rev-parse HEAD >expect &&
+	git rev-parse $a >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction can handle abort' '
+	cat >stdin <<-EOF &&
+	start
+	create $b HEAD
+	abort
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start abort >expect &&
+	test_cmp expect actual &&
+	test_path_is_missing .git/$b
+'
+
+test_expect_success 'transaction aborts by default' '
+	cat >stdin <<-EOF &&
+	start
+	create $b HEAD
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start >expect &&
+	test_cmp expect actual &&
+	test_path_is_missing .git/$b
+'
+
+test_expect_success 'transaction with prepare aborts by default' '
+	cat >stdin <<-EOF &&
+	start
+	create $b HEAD
+	prepare
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start prepare >expect &&
+	test_cmp expect actual &&
+	test_path_is_missing .git/$b
+'
+
 test_done

-- 
2.26.0
Junio C Hamano· Mar 27, 2020, 22:00 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 9/9] update-ref: implement interactive transaction handling

Patrick Steinhardt <ps@pks.im> writes:
Show 42 quoted lines
> The git-update-ref(1) command can only handle queueing transactions
> right now via its "--stdin" parameter, but there is no way for users to
> handle the transaction itself in a more explicit way. E.g. in a
> replicated scenario, one may imagine a coordinator that spawns
> git-update-ref(1) for multiple repositories and only if all agree that
> an update is possible will the coordinator send a commit. Such a
> transactional session could look like
>
>     > start
>     < start: ok
>     > update refs/heads/master $OLD $NEW
>     > prepare
>     < prepare: ok
>     # All nodes have returned "ok"
>     > commit
>     < commit: ok
>
> or
>
>     > start
>     < start: ok
>     > create refs/heads/master $OLD $NEW
>     > prepare
>     < fatal: cannot lock ref 'refs/heads/master': reference already exists
>     # On all other nodes:
>     > abort
>     < abort: ok
>
> In order to allow for such transactional sessions, this commit
> introduces four new commands for git-update-ref(1), which matches those
> we have internally already with the exception of "start":
>
>     - start: start a new transaction
>
>     - prepare: prepare the transaction, that is try to lock all
>                references and verify their current value matches the
>                expected one
>
>     - commit: explicitly commit a session, that is update references to
>               match their new expected state
>
>     - abort: abort a session and roll back all changes
OK, this is a fun stuff, and explains why we wanted to do [7/9].
Patrick Steinhardt· Mar 30, 2020, 13:46 UTC · re: Patrick Steinhardt · lore

[PATCH v2 0/9] Support for transactions in `git-update-ref --stdin`

Hi,

this is the second version of this patch series to introduce transaction support in `git update-ref --stdin`. The goal is to make commands available to the user of that command so that he can explicitly control the given transaction by starting, preparing, and finally committing or aborting the transaction.

The second version addresses the feedback of Junio:
    - Renamed the `commands` array to `command`.
    - Adds documentation to the `extra_lines` field to hopefully make
      its intent clearer.
    - Clarify why we ignore EOF when reading `extra_lines`.
    - When reading commands, the code now verifies that a line is not
      only a prefix to the command, but in fact the whole command. This
      allows implementation of multiple commands that have the same
      prefix, e.g. "option" and "options".
Thanks Junio for your review!
Patrick
Patrick Steinhardt (9):
  refs: fix segfault when aborting empty transaction
  git-update-ref.txt: add missing word
  strbuf: provide function to append whole lines
  update-ref: organize commands in an array
  update-ref: drop unused argument for `parse_refname`
  update-ref: pass end pointer instead of strbuf
  update-ref: move transaction handling into `update_refs_stdin()`
  update-ref: read commands in a line-wise fashion
  update-ref: implement interactive transaction handling
 Documentation/git-update-ref.txt |  28 +++-
 builtin/update-ref.c             | 274 +++++++++++++++++++++++--------
 refs/files-backend.c             |  20 ++-
 strbuf.c                         |  10 ++
 strbuf.h                         |   6 +
 t/t1400-update-ref.sh            | 131 +++++++++++++++
 6 files changed, 388 insertions(+), 81 deletions(-)
Range-diff against v1:
 1:  3c7f8c47f3 =  1:  7a297db4da refs: fix segfault when aborting empty transaction
 2:  dd7798abb7 =  2:  15857e1b8c git-update-ref.txt: add missing word
 3:  4b0a963ea5 =  3:  b6546ae44e strbuf: provide function to append whole lines
 4:  50ffc26329 !  4:  bd8c059fbc update-ref: organize commands in an array
    @@ builtin/update-ref.c: static const char *parse_cmd_option(struct strbuf *input,
     +static const struct parse_cmd {
     +	const char *prefix;
     +	const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);
    -+} commands[] = {
    ++} command[] = {
     +	{ "update", parse_cmd_update },
     +	{ "create", parse_cmd_create },
     +	{ "delete", parse_cmd_delete },
    @@ builtin/update-ref.c: static const char *parse_cmd_option(struct strbuf *input,
     -			next = parse_cmd_option(&input, next);
     -		else
     +
    -+		for (i = 0; i < ARRAY_SIZE(commands); i++) {
    -+			if (!skip_prefix(next, commands[i].prefix , &next))
    ++		for (i = 0; i < ARRAY_SIZE(command); i++) {
    ++			const char *prefix = command[i].prefix;
    ++
    ++			if (!skip_prefix(next, prefix, &next))
     +				continue;
    -+			cmd = &commands[i];
    ++
    ++			/*
    ++			 * Check that the command is terminated by an argument
    ++			 * or line terminator and not only a matching prefix.
    ++			 */
    ++			if (input.buf[strlen(prefix)] != line_termination &&
    ++			    input.buf[strlen(prefix)] != '\0' &&
    ++			    input.buf[strlen(prefix)] != ' ')
    ++				continue;
    ++
    ++			cmd = &command[i];
     +			break;
     +		}
     +		if (!cmd)
      			die("unknown command: %s", next);
      
    -+		if (input.buf[strlen(cmd->prefix)] != line_termination &&
    -+		    input.buf[strlen(cmd->prefix)] != '\0' &&
    -+		    input.buf[strlen(cmd->prefix)] != ' ')
    -+			die("%s: no separator after command", cmd->prefix);
    -+
     +		next = cmd->fn(transaction, &input, next);
      		next++;
      	}
 5:  a78043b188 =  5:  49a14d2046 update-ref: drop unused argument for `parse_refname`
 6:  51ebb2f849 !  6:  cbe430029d update-ref: pass end pointer instead of strbuf
    @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction
      	const char *prefix;
     -	const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);
     +	const char *(*fn)(struct ref_transaction *, const char *, const char *);
    - } commands[] = {
    + } command[] = {
      	{ "update", parse_cmd_update },
      	{ "create", parse_cmd_create },
     @@ builtin/update-ref.c: static void update_refs_stdin(struct ref_transaction *transaction)
    - 		    input.buf[strlen(cmd->prefix)] != ' ')
    - 			die("%s: no separator after command", cmd->prefix);
    + 		if (!cmd)
    + 			die("unknown command: %s", next);
      
     -		next = cmd->fn(transaction, &input, next);
     +		next = cmd->fn(transaction, next, input.buf + input.len);
 7:  bd92a649d7 =  7:  d2f68f59a7 update-ref: move transaction handling into `update_refs_stdin()`
 8:  1db63f97ec !  8:  f8786fdeb3 update-ref: read commands in a line-wise fashion
    @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction
      	const char *prefix;
     -	const char *(*fn)(struct ref_transaction *, const char *, const char *);
     +	void (*fn)(struct ref_transaction *, const char *, const char *);
    ++	/*
    ++	 * If using NUL-terminated format, only the first argument will be
    ++	 * available in the first line. In case a command expects more than one
    ++	 * argument, we thus have to fetch an additional `extra_lines` number
    ++	 * of lines.
    ++	 */
     +	unsigned extra_lines;
    - } commands[] = {
    + } command[] = {
     -	{ "update", parse_cmd_update },
     -	{ "create", parse_cmd_create },
     -	{ "delete", parse_cmd_delete },
    @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction
     +		else if (isspace(*input.buf))
     +			die("whitespace before command: %s", input.buf);
      
    - 		for (i = 0; i < ARRAY_SIZE(commands); i++) {
    --			if (!skip_prefix(next, commands[i].prefix , &next))
    -+			if (!starts_with(input.buf, commands[i].prefix))
    + 		for (i = 0; i < ARRAY_SIZE(command); i++) {
    + 			const char *prefix = command[i].prefix;
    + 
    +-			if (!skip_prefix(next, prefix, &next))
    ++			if (!starts_with(input.buf, prefix))
      				continue;
    - 			cmd = &commands[i];
    + 
    + 			/*
    +@@ builtin/update-ref.c: static void update_refs_stdin(void)
      			break;
      		}
      		if (!cmd)
     -			die("unknown command: %s", next);
     +			die("unknown command: %s", input.buf);
      
    - 		if (input.buf[strlen(cmd->prefix)] != line_termination &&
    - 		    input.buf[strlen(cmd->prefix)] != '\0' &&
    - 		    input.buf[strlen(cmd->prefix)] != ' ')
    - 			die("%s: no separator after command", cmd->prefix);
    - 
     -		next = cmd->fn(transaction, next, input.buf + input.len);
     -		next++;
    -+		/* Read extra lines if NUL-terminated */
    ++		/*
    ++		 * Read extra lines if NUL-terminated. Do not raise an error in
    ++		 * case there is an early EOF to let the command handle missing
    ++		 * arguments with a proper error message.
    ++		 */
     +		for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++)
     +			if (strbuf_appendwholeline(&input, stdin, line_termination))
     +				break;
 9:  88c0089bb5 !  9:  c3fffdf9fa update-ref: implement interactive transaction handling
    @@ builtin/update-ref.c: static void parse_cmd_option(struct ref_transaction *trans
      static const struct parse_cmd {
      	const char *prefix;
      	void (*fn)(struct ref_transaction *, const char *, const char *);
    +@@ builtin/update-ref.c: static const struct parse_cmd {
    + 	 * of lines.
    + 	 */
      	unsigned extra_lines;
     +	enum update_refs_state state;
    - } commands[] = {
    + } command[] = {
     -	{ "update", parse_cmd_update, 2 },
     -	{ "create", parse_cmd_create, 1 },
     -	{ "delete", parse_cmd_delete, 1 },
    @@ builtin/update-ref.c: static void parse_cmd_option(struct ref_transaction *trans
      	int i, j;
      
     @@ builtin/update-ref.c: static void update_refs_stdin(void)
    - 		    input.buf[strlen(cmd->prefix)] != ' ')
    - 			die("%s: no separator after command", cmd->prefix);
    - 
    --		/* Read extra lines if NUL-terminated */
    -+		/* Read extra lines if NUL-terminated, but let commands handle missing ones. */
    - 		for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++)
      			if (strbuf_appendwholeline(&input, stdin, line_termination))
      				break;
      
-- 
2.26.0
Patrick Steinhardt· Mar 30, 2020, 13:46 UTC · re: Patrick Steinhardt · lore

[PATCH v2 1/9] refs: fix segfault when aborting empty transaction

When cleaning up a transaction that has no updates queued, then the transaction's backend data will not have been allocated. We correctly handle this for the packed backend, where the cleanup function checks whether the backend data has been allocated at all -- if not, then there is nothing to clean up. For the files backend we do not check this and as a result will hit a segfault due to dereferencing a `NULL` pointer when cleaning up such a transaction.

Fix the issue by checking whether `backend_data` is set in the files backend, too.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)
Show changes to refs/files-backend.c +11 −9
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 561c33ac8a..6516c7bc8c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2565,17 +2565,19 @@ static void files_transaction_cleanup(struct files_ref_store *refs,
 		}
 	}
 
-	if (backend_data->packed_transaction &&
-	    ref_transaction_abort(backend_data->packed_transaction, &err)) {
-		error("error aborting transaction: %s", err.buf);
-		strbuf_release(&err);
+	if (backend_data) {
+		if (backend_data->packed_transaction &&
+		    ref_transaction_abort(backend_data->packed_transaction, &err)) {
+			error("error aborting transaction: %s", err.buf);
+			strbuf_release(&err);
+		}
+
+		if (backend_data->packed_refs_locked)
+			packed_refs_unlock(refs->packed_ref_store);
+
+		free(backend_data);
 	}
 
-	if (backend_data->packed_refs_locked)
-		packed_refs_unlock(refs->packed_ref_store);
-
-	free(backend_data);
-
 	transaction->state = REF_TRANSACTION_CLOSED;
 }
 

-- 
2.26.0
Patrick Steinhardt· Mar 30, 2020, 13:46 UTC · re: Patrick Steinhardt · lore

[PATCH v2 2/9] git-update-ref.txt: add missing word

The description for the "verify" command is lacking a single word "is", which this commit corrects.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git-update-ref.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Documentation/git-update-ref.txt +1 −1
diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 9671423117..9bd039ce08 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -107,7 +107,7 @@ delete::
 
 verify::
 	Verify <ref> against <oldvalue> but do not change it.  If
-	<oldvalue> zero or missing, the ref must not exist.
+	<oldvalue> is zero or missing, the ref must not exist.
 
 option::
 	Modify behavior of the next command naming a <ref>.

-- 
2.26.0
Patrick Steinhardt· Mar 30, 2020, 13:46 UTC · re: Patrick Steinhardt · lore

[PATCH v2 3/9] strbuf: provide function to append whole lines

While the strbuf interface already provides functions to read a line into it that completely replaces its current contents, we do not have an interface that allows for appending lines without discarding current contents.

Add a new function `strbuf_appendwholeline` that reads a line including its terminating character into a strbuf non-destructively. This is a preparatory step for git-update-ref(1) reading standard input line-wise instead of as a block.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 strbuf.c | 10 ++++++++++
 strbuf.h |  6 ++++++
 2 files changed, 16 insertions(+)
Show changes to 2 files +16 −0

strbuf.c, strbuf.h

diff --git a/strbuf.c b/strbuf.c
index bb0065ccaf..6e74901bfa 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -690,6 +690,16 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 }
 #endif
 
+int strbuf_appendwholeline(struct strbuf *sb, FILE *fp, int term)
+{
+	struct strbuf line = STRBUF_INIT;
+	if (strbuf_getwholeline(&line, fp, term))
+		return EOF;
+	strbuf_addbuf(sb, &line);
+	strbuf_release(&line);
+	return 0;
+}
+
 static int strbuf_getdelim(struct strbuf *sb, FILE *fp, int term)
 {
 	if (strbuf_getwholeline(sb, fp, term))
diff --git a/strbuf.h b/strbuf.h
index ce8e49c0b2..411063ca76 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -502,6 +502,12 @@ int strbuf_getline(struct strbuf *sb, FILE *file);
  */
 int strbuf_getwholeline(struct strbuf *sb, FILE *file, int term);
 
+/**
+ * Like `strbuf_getwholeline`, but appends the line instead of
+ * resetting the buffer first.
+ */
+int strbuf_appendwholeline(struct strbuf *sb, FILE *file, int term);
+
 /**
  * Like `strbuf_getwholeline`, but operates on a file descriptor.
  * It reads one character at a time, so it is very slow.  Do not

-- 
2.26.0
Patrick Steinhardt· Mar 30, 2020, 13:46 UTC · re: Patrick Steinhardt · lore

[PATCH v2 4/9] update-ref: organize commands in an array

We currently manually wire up all commands known to `git-update-ref --stdin`, making it harder than necessary to preprocess arguments after the command is determined. To make this more extensible, let's refactor the code to use an array of known commands instead. While this doesn't add a lot of value now, it is a preparatory step to implement line-wise reading of commands.

As we're going to introduce commands without trailing spaces, this commit also moves whitespace parsing into the respective commands.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 73 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 56 insertions(+), 17 deletions(-)
Show changes to builtin/update-ref.c +56 −17
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 2d8f7f0578..559475681e 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -171,11 +171,11 @@ static int parse_next_oid(struct strbuf *input, const char **next,
 /*
  * The following five parse_cmd_*() functions parse the corresponding
  * command.  In each case, next points at the character following the
- * command name and the following space.  They each return a pointer
- * to the character terminating the command, and die with an
- * explanatory message if there are any parsing problems.  All of
- * these functions handle either text or binary format input,
- * depending on how line_termination is set.
+ * command name.  They each return a pointer to the character
+ * terminating the command, and die with an explanatory message if
+ * there are any parsing problems.  All of these functions handle
+ * either text or binary format input, depending on how
+ * line_termination is set.
  */
 
 static const char *parse_cmd_update(struct ref_transaction *transaction,
@@ -186,6 +186,9 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 	struct object_id new_oid, old_oid;
 	int have_old;
 
+	if (!skip_prefix(next, " ", &next))
+		die("update: missing space after command");
+
 	refname = parse_refname(input, &next);
 	if (!refname)
 		die("update: missing <ref>");
@@ -220,6 +223,9 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 	char *refname;
 	struct object_id new_oid;
 
+	if (!skip_prefix(next, " ", &next))
+		die("create: missing space after command");
+
 	refname = parse_refname(input, &next);
 	if (!refname)
 		die("create: missing <ref>");
@@ -253,6 +259,9 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 	struct object_id old_oid;
 	int have_old;
 
+	if (!skip_prefix(next, " ", &next))
+		die("delete: missing space after command");
+
 	refname = parse_refname(input, &next);
 	if (!refname)
 		die("delete: missing <ref>");
@@ -288,6 +297,9 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 	char *refname;
 	struct object_id old_oid;
 
+	if (!skip_prefix(next, " ", &next))
+		die("verify: missing space after command");
+
 	refname = parse_refname(input, &next);
 	if (!refname)
 		die("verify: missing <ref>");
@@ -310,9 +322,12 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 	return next;
 }
 
-static const char *parse_cmd_option(struct strbuf *input, const char *next)
+static const char *parse_cmd_option(struct ref_transaction *transaction,
+				    struct strbuf *input, const char *next)
 {
 	const char *rest;
+	if (!skip_prefix(next, " ", &next))
+		die("option: missing space after command");
 	if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination)
 		update_flags |= REF_NO_DEREF;
 	else
@@ -320,33 +335,57 @@ static const char *parse_cmd_option(struct strbuf *input, const char *next)
 	return rest;
 }
 
+static const struct parse_cmd {
+	const char *prefix;
+	const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);
+} command[] = {
+	{ "update", parse_cmd_update },
+	{ "create", parse_cmd_create },
+	{ "delete", parse_cmd_delete },
+	{ "verify", parse_cmd_verify },
+	{ "option", parse_cmd_option },
+};
+
 static void update_refs_stdin(struct ref_transaction *transaction)
 {
 	struct strbuf input = STRBUF_INIT;
 	const char *next;
+	int i;
 
 	if (strbuf_read(&input, 0, 1000) < 0)
 		die_errno("could not read from stdin");
 	next = input.buf;
 	/* Read each line dispatch its command */
 	while (next < input.buf + input.len) {
+		const struct parse_cmd *cmd = NULL;
+
 		if (*next == line_termination)
 			die("empty command in input");
 		else if (isspace(*next))
 			die("whitespace before command: %s", next);
-		else if (skip_prefix(next, "update ", &next))
-			next = parse_cmd_update(transaction, &input, next);
-		else if (skip_prefix(next, "create ", &next))
-			next = parse_cmd_create(transaction, &input, next);
-		else if (skip_prefix(next, "delete ", &next))
-			next = parse_cmd_delete(transaction, &input, next);
-		else if (skip_prefix(next, "verify ", &next))
-			next = parse_cmd_verify(transaction, &input, next);
-		else if (skip_prefix(next, "option ", &next))
-			next = parse_cmd_option(&input, next);
-		else
+
+		for (i = 0; i < ARRAY_SIZE(command); i++) {
+			const char *prefix = command[i].prefix;
+
+			if (!skip_prefix(next, prefix, &next))
+				continue;
+
+			/*
+			 * Check that the command is terminated by an argument
+			 * or line terminator and not only a matching prefix.
+			 */
+			if (input.buf[strlen(prefix)] != line_termination &&
+			    input.buf[strlen(prefix)] != '\0' &&
+			    input.buf[strlen(prefix)] != ' ')
+				continue;
+
+			cmd = &command[i];
+			break;
+		}
+		if (!cmd)
 			die("unknown command: %s", next);
 
+		next = cmd->fn(transaction, &input, next);
 		next++;
 	}
 

-- 
2.26.0
Patrick Steinhardt· Mar 30, 2020, 13:46 UTC · re: Patrick Steinhardt · lore

[PATCH v2 5/9] update-ref: drop unused argument for `parse_refname`

The `parse_refname` function accepts a `struct strbuf *input` argument that isn't used at all. As we're about to convert commands to not use a strbuf anymore but instead an end pointer, let's drop this argument now to make the converting commit easier to review.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to builtin/update-ref.c +5 −5
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 559475681e..6f2aba916b 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -50,7 +50,7 @@ static const char *parse_arg(const char *next, struct strbuf *arg)
  * the argument.  Die if C-quoting is malformed or the reference name
  * is invalid.
  */
-static char *parse_refname(struct strbuf *input, const char **next)
+static char *parse_refname(const char **next)
 {
 	struct strbuf ref = STRBUF_INIT;
 
@@ -189,7 +189,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 	if (!skip_prefix(next, " ", &next))
 		die("update: missing space after command");
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("update: missing <ref>");
 
@@ -226,7 +226,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 	if (!skip_prefix(next, " ", &next))
 		die("create: missing space after command");
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("create: missing <ref>");
 
@@ -262,7 +262,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 	if (!skip_prefix(next, " ", &next))
 		die("delete: missing space after command");
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("delete: missing <ref>");
 
@@ -300,7 +300,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 	if (!skip_prefix(next, " ", &next))
 		die("verify: missing space after command");
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("verify: missing <ref>");
 

-- 
2.26.0
Patrick Steinhardt· Mar 30, 2020, 13:46 UTC · re: Patrick Steinhardt · lore

[PATCH v2 6/9] update-ref: pass end pointer instead of strbuf

We currently pass both an `strbuf` containing the current command line as well as the `next` pointer pointing to the first argument to commands. This is both confusing and makes code more intertwined. Convert this to use a simple pointer as well as a pointer pointing to the end of the input as a preparatory step to line-wise reading of stdin.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)
Show changes to builtin/update-ref.c +15 −15
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 6f2aba916b..139c3b11fa 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -95,7 +95,7 @@ static char *parse_refname(const char **next)
  * provided but cannot be converted to a SHA-1, die.  flags can
  * include PARSE_SHA1_OLD and/or PARSE_SHA1_ALLOW_EMPTY.
  */
-static int parse_next_oid(struct strbuf *input, const char **next,
+static int parse_next_oid(const char **next, const char *end,
 			  struct object_id *oid,
 			  const char *command, const char *refname,
 			  int flags)
@@ -103,7 +103,7 @@ static int parse_next_oid(struct strbuf *input, const char **next,
 	struct strbuf arg = STRBUF_INIT;
 	int ret = 0;
 
-	if (*next == input->buf + input->len)
+	if (*next == end)
 		goto eof;
 
 	if (line_termination) {
@@ -128,7 +128,7 @@ static int parse_next_oid(struct strbuf *input, const char **next,
 			die("%s %s: expected NUL but got: %s",
 			    command, refname, *next);
 		(*next)++;
-		if (*next == input->buf + input->len)
+		if (*next == end)
 			goto eof;
 		strbuf_addstr(&arg, *next);
 		*next += arg.len;
@@ -179,7 +179,7 @@ static int parse_next_oid(struct strbuf *input, const char **next,
  */
 
 static const char *parse_cmd_update(struct ref_transaction *transaction,
-				    struct strbuf *input, const char *next)
+				    const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -193,11 +193,11 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 	if (!refname)
 		die("update: missing <ref>");
 
-	if (parse_next_oid(input, &next, &new_oid, "update", refname,
+	if (parse_next_oid(&next, end, &new_oid, "update", refname,
 			   PARSE_SHA1_ALLOW_EMPTY))
 		die("update %s: missing <newvalue>", refname);
 
-	have_old = !parse_next_oid(input, &next, &old_oid, "update", refname,
+	have_old = !parse_next_oid(&next, end, &old_oid, "update", refname,
 				   PARSE_SHA1_OLD);
 
 	if (*next != line_termination)
@@ -217,7 +217,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 }
 
 static const char *parse_cmd_create(struct ref_transaction *transaction,
-				    struct strbuf *input, const char *next)
+				    const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -230,7 +230,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 	if (!refname)
 		die("create: missing <ref>");
 
-	if (parse_next_oid(input, &next, &new_oid, "create", refname, 0))
+	if (parse_next_oid(&next, end, &new_oid, "create", refname, 0))
 		die("create %s: missing <newvalue>", refname);
 
 	if (is_null_oid(&new_oid))
@@ -252,7 +252,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 }
 
 static const char *parse_cmd_delete(struct ref_transaction *transaction,
-				    struct strbuf *input, const char *next)
+				    const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -266,7 +266,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 	if (!refname)
 		die("delete: missing <ref>");
 
-	if (parse_next_oid(input, &next, &old_oid, "delete", refname,
+	if (parse_next_oid(&next, end, &old_oid, "delete", refname,
 			   PARSE_SHA1_OLD)) {
 		have_old = 0;
 	} else {
@@ -291,7 +291,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 }
 
 static const char *parse_cmd_verify(struct ref_transaction *transaction,
-				    struct strbuf *input, const char *next)
+				    const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -304,7 +304,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 	if (!refname)
 		die("verify: missing <ref>");
 
-	if (parse_next_oid(input, &next, &old_oid, "verify", refname,
+	if (parse_next_oid(&next, end, &old_oid, "verify", refname,
 			   PARSE_SHA1_OLD))
 		oidclr(&old_oid);
 
@@ -323,7 +323,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 }
 
 static const char *parse_cmd_option(struct ref_transaction *transaction,
-				    struct strbuf *input, const char *next)
+				    const char *next, const char *end)
 {
 	const char *rest;
 	if (!skip_prefix(next, " ", &next))
@@ -337,7 +337,7 @@ static const char *parse_cmd_option(struct ref_transaction *transaction,
 
 static const struct parse_cmd {
 	const char *prefix;
-	const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);
+	const char *(*fn)(struct ref_transaction *, const char *, const char *);
 } command[] = {
 	{ "update", parse_cmd_update },
 	{ "create", parse_cmd_create },
@@ -385,7 +385,7 @@ static void update_refs_stdin(struct ref_transaction *transaction)
 		if (!cmd)
 			die("unknown command: %s", next);
 
-		next = cmd->fn(transaction, &input, next);
+		next = cmd->fn(transaction, next, input.buf + input.len);
 		next++;
 	}
 

-- 
2.26.0
Patrick Steinhardt· Mar 30, 2020, 13:46 UTC · re: Patrick Steinhardt · lore

[PATCH v2 7/9] update-ref: move transaction handling into `update_refs_stdin()`

While the actual logic to update the transaction is handled in `update_refs_stdin()`, the transaction itself is started and committed in `cmd_update_ref()` itself. This makes it hard to handle transaction abortion and commits as part of `update_refs_stdin()` itself, which is required in order to introduce transaction handling features to `git update-refs --stdin`.

Refactor the code to move all transaction handling into `update_refs_stdin()` to prepare for transaction handling features.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)
Show changes to builtin/update-ref.c +14 −13
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 139c3b11fa..1a7906545d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -346,15 +346,21 @@ static const struct parse_cmd {
 	{ "option", parse_cmd_option },
 };
 
-static void update_refs_stdin(struct ref_transaction *transaction)
+static void update_refs_stdin(void)
 {
-	struct strbuf input = STRBUF_INIT;
+	struct strbuf input = STRBUF_INIT, err = STRBUF_INIT;
+	struct ref_transaction *transaction;
 	const char *next;
 	int i;
 
 	if (strbuf_read(&input, 0, 1000) < 0)
 		die_errno("could not read from stdin");
 	next = input.buf;
+
+	transaction = ref_transaction_begin(&err);
+	if (!transaction)
+		die("%s", err.buf);
+
 	/* Read each line dispatch its command */
 	while (next < input.buf + input.len) {
 		const struct parse_cmd *cmd = NULL;
@@ -389,6 +395,11 @@ static void update_refs_stdin(struct ref_transaction *transaction)
 		next++;
 	}
 
+	if (ref_transaction_commit(transaction, &err))
+		die("%s", err.buf);
+
+	ref_transaction_free(transaction);
+	strbuf_release(&err);
 	strbuf_release(&input);
 }
 
@@ -423,21 +434,11 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 	}
 
 	if (read_stdin) {
-		struct strbuf err = STRBUF_INIT;
-		struct ref_transaction *transaction;
-
-		transaction = ref_transaction_begin(&err);
-		if (!transaction)
-			die("%s", err.buf);
 		if (delete || argc > 0)
 			usage_with_options(git_update_ref_usage, options);
 		if (end_null)
 			line_termination = '\0';
-		update_refs_stdin(transaction);
-		if (ref_transaction_commit(transaction, &err))
-			die("%s", err.buf);
-		ref_transaction_free(transaction);
-		strbuf_release(&err);
+		update_refs_stdin();
 		return 0;
 	}
 

-- 
2.26.0
Patrick Steinhardt· Mar 30, 2020, 13:46 UTC · re: Patrick Steinhardt · lore

[PATCH v2 8/9] update-ref: read commands in a line-wise fashion

The git-update-ref(1) supports a `--stdin` mode that allows it to read all reference updates from standard input. This is mainly used to allow for atomic reference updates that are all or nothing, so that either all references will get updated or none.

Currently, git-update-ref(1) reads all commands as a single block of up to 1000 characters and only starts processing after stdin gets closed. This is less flexible than one might wish for, as it doesn't really allow for longer-lived transactions and doesn't allow any verification without committing everything. E.g. one may imagine the following exchange:

    > start
    < start: ok
    > update refs/heads/master $NEWOID1 $OLDOID1
    > update refs/heads/branch $NEWOID2 $OLDOID2
    > prepare
    < prepare: ok
    > commit
    < commit: ok

When reading all input as a whole block, the above interactive protocol is obviously impossible to achieve. But by converting the command to read commands linewise, we can make it more interactive than before.

Obviously, the linewise interface is only a first step in making git-update-ref(1) work in a more transaction-oriented way. Missing is most importantly support for transactional commands that manage the current transaction.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 80 +++++++++++++++++++++++---------------------
 1 file changed, 41 insertions(+), 39 deletions(-)
Show changes to builtin/update-ref.c +41 −39
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 1a7906545d..77cd235dfc 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -178,8 +178,8 @@ static int parse_next_oid(const char **next, const char *end,
  * line_termination is set.
  */
 
-static const char *parse_cmd_update(struct ref_transaction *transaction,
-				    const char *next, const char *end)
+static void parse_cmd_update(struct ref_transaction *transaction,
+			     const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -212,12 +212,10 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 	update_flags = default_flags;
 	free(refname);
 	strbuf_release(&err);
-
-	return next;
 }
 
-static const char *parse_cmd_create(struct ref_transaction *transaction,
-				    const char *next, const char *end)
+static void parse_cmd_create(struct ref_transaction *transaction,
+			     const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -247,12 +245,10 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 	update_flags = default_flags;
 	free(refname);
 	strbuf_release(&err);
-
-	return next;
 }
 
-static const char *parse_cmd_delete(struct ref_transaction *transaction,
-				    const char *next, const char *end)
+static void parse_cmd_delete(struct ref_transaction *transaction,
+			     const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -286,12 +282,10 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 	update_flags = default_flags;
 	free(refname);
 	strbuf_release(&err);
-
-	return next;
 }
 
-static const char *parse_cmd_verify(struct ref_transaction *transaction,
-				    const char *next, const char *end)
+static void parse_cmd_verify(struct ref_transaction *transaction,
+			     const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -318,12 +312,10 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 	update_flags = default_flags;
 	free(refname);
 	strbuf_release(&err);
-
-	return next;
 }
 
-static const char *parse_cmd_option(struct ref_transaction *transaction,
-				    const char *next, const char *end)
+static void parse_cmd_option(struct ref_transaction *transaction,
+			     const char *next, const char *end)
 {
 	const char *rest;
 	if (!skip_prefix(next, " ", &next))
@@ -332,48 +324,49 @@ static const char *parse_cmd_option(struct ref_transaction *transaction,
 		update_flags |= REF_NO_DEREF;
 	else
 		die("option unknown: %s", next);
-	return rest;
 }
 
 static const struct parse_cmd {
 	const char *prefix;
-	const char *(*fn)(struct ref_transaction *, const char *, const char *);
+	void (*fn)(struct ref_transaction *, const char *, const char *);
+	/*
+	 * If using NUL-terminated format, only the first argument will be
+	 * available in the first line. In case a command expects more than one
+	 * argument, we thus have to fetch an additional `extra_lines` number
+	 * of lines.
+	 */
+	unsigned extra_lines;
 } command[] = {
-	{ "update", parse_cmd_update },
-	{ "create", parse_cmd_create },
-	{ "delete", parse_cmd_delete },
-	{ "verify", parse_cmd_verify },
-	{ "option", parse_cmd_option },
+	{ "update", parse_cmd_update, 2 },
+	{ "create", parse_cmd_create, 1 },
+	{ "delete", parse_cmd_delete, 1 },
+	{ "verify", parse_cmd_verify, 1 },
+	{ "option", parse_cmd_option, 0 },
 };
 
 static void update_refs_stdin(void)
 {
 	struct strbuf input = STRBUF_INIT, err = STRBUF_INIT;
 	struct ref_transaction *transaction;
-	const char *next;
-	int i;
-
-	if (strbuf_read(&input, 0, 1000) < 0)
-		die_errno("could not read from stdin");
-	next = input.buf;
+	int i, j;
 
 	transaction = ref_transaction_begin(&err);
 	if (!transaction)
 		die("%s", err.buf);
 
 	/* Read each line dispatch its command */
-	while (next < input.buf + input.len) {
+	while (!strbuf_getwholeline(&input, stdin, line_termination)) {
 		const struct parse_cmd *cmd = NULL;
 
-		if (*next == line_termination)
+		if (*input.buf == line_termination)
 			die("empty command in input");
-		else if (isspace(*next))
-			die("whitespace before command: %s", next);
+		else if (isspace(*input.buf))
+			die("whitespace before command: %s", input.buf);
 
 		for (i = 0; i < ARRAY_SIZE(command); i++) {
 			const char *prefix = command[i].prefix;
 
-			if (!skip_prefix(next, prefix, &next))
+			if (!starts_with(input.buf, prefix))
 				continue;
 
 			/*
@@ -389,10 +382,19 @@ static void update_refs_stdin(void)
 			break;
 		}
 		if (!cmd)
-			die("unknown command: %s", next);
+			die("unknown command: %s", input.buf);
 
-		next = cmd->fn(transaction, next, input.buf + input.len);
-		next++;
+		/*
+		 * Read extra lines if NUL-terminated. Do not raise an error in
+		 * case there is an early EOF to let the command handle missing
+		 * arguments with a proper error message.
+		 */
+		for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++)
+			if (strbuf_appendwholeline(&input, stdin, line_termination))
+				break;
+
+		cmd->fn(transaction, input.buf + strlen(cmd->prefix),
+			input.buf + input.len);
 	}
 
 	if (ref_transaction_commit(transaction, &err))

-- 
2.26.0
Patrick Steinhardt· Mar 30, 2020, 13:47 UTC · re: Patrick Steinhardt · lore

[PATCH v2 9/9] update-ref: implement interactive transaction handling

The git-update-ref(1) command can only handle queueing transactions right now via its "--stdin" parameter, but there is no way for users to handle the transaction itself in a more explicit way. E.g. in a replicated scenario, one may imagine a coordinator that spawns git-update-ref(1) for multiple repositories and only if all agree that an update is possible will the coordinator send a commit. Such a transactional session could look like

    > start
    < start: ok
    > update refs/heads/master $OLD $NEW
    > prepare
    < prepare: ok
    # All nodes have returned "ok"
    > commit
    < commit: ok
or
    > start
    < start: ok
    > create refs/heads/master $OLD $NEW
    > prepare
    < fatal: cannot lock ref 'refs/heads/master': reference already exists
    # On all other nodes:
    > abort
    < abort: ok

In order to allow for such transactional sessions, this commit introduces four new commands for git-update-ref(1), which matches those we have internally already with the exception of "start":

    - start: start a new transaction
    - prepare: prepare the transaction, that is try to lock all
               references and verify their current value matches the
               expected one
    - commit: explicitly commit a session, that is update references to
              match their new expected state
    - abort: abort a session and roll back all changes

By design, git-update-ref(1) will commit as soon as standard input is being closed. While fine in a non-transactional world, it is definitely unexpected in a transactional world. Because of this, as soon as any of the new transactional commands is used, the default will change to aborting without an explicit "commit". To avoid a race between queueing updates and the first "prepare" that starts a transaction, the "start" command has been added to start an explicit transaction.

Add some tests to exercise this new functionality.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git-update-ref.txt |  26 ++++++
 builtin/update-ref.c             | 106 +++++++++++++++++++++++--
 t/t1400-update-ref.sh            | 131 +++++++++++++++++++++++++++++++
 3 files changed, 255 insertions(+), 8 deletions(-)
Show changes to 3 files +255 −8

Documentation/git-update-ref.txt, builtin/update-ref.c, t/t1400-update-ref.sh

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 9bd039ce08..3e737c2360 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -66,6 +66,10 @@ performs all modifications together.  Specify commands of the form:
 	delete SP <ref> [SP <oldvalue>] LF
 	verify SP <ref> [SP <oldvalue>] LF
 	option SP <opt> LF
+	start LF
+	prepare LF
+	commit LF
+	abort LF
 
 With `--create-reflog`, update-ref will create a reflog for each ref
 even if one would not ordinarily be created.
@@ -83,6 +87,10 @@ quoting:
 	delete SP <ref> NUL [<oldvalue>] NUL
 	verify SP <ref> NUL [<oldvalue>] NUL
 	option SP <opt> NUL
+	start NUL
+	prepare NUL
+	commit NUL
+	abort NUL
 
 In this format, use 40 "0" to specify a zero value, and use the empty
 string to specify a missing value.
@@ -114,6 +122,24 @@ option::
 	The only valid option is `no-deref` to avoid dereferencing
 	a symbolic ref.
 
+start::
+	Start a transaction. In contrast to a non-transactional session, a
+	transaction will automatically abort if the session ends without an
+	explicit commit.
+
+prepare::
+	Prepare to commit the transaction. This will create lock files for all
+	queued reference updates. If one reference could not be locked, the
+	transaction will be aborted.
+
+commit::
+	Commit all reference updates queued for the transaction, ending the
+	transaction.
+
+abort::
+	Abort the transaction, releasing all locks if the transaction is in
+	prepared state.
+
 If all <ref>s can be locked with matching <oldvalue>s
 simultaneously, all modifications are performed.  Otherwise, no
 modifications are performed.  Note that while each individual
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 77cd235dfc..120c0a5fc0 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -326,6 +326,59 @@ static void parse_cmd_option(struct ref_transaction *transaction,
 		die("option unknown: %s", next);
 }
 
+static void parse_cmd_start(struct ref_transaction *transaction,
+			    const char *next, const char *end)
+{
+	if (*next != line_termination)
+		die("start: extra input: %s", next);
+	puts("start: ok");
+}
+
+static void parse_cmd_prepare(struct ref_transaction *transaction,
+			      const char *next, const char *end)
+{
+	struct strbuf error = STRBUF_INIT;
+	if (*next != line_termination)
+		die("prepare: extra input: %s", next);
+	if (ref_transaction_prepare(transaction, &error))
+		die("prepare: %s", error.buf);
+	puts("prepare: ok");
+}
+
+static void parse_cmd_abort(struct ref_transaction *transaction,
+			    const char *next, const char *end)
+{
+	struct strbuf error = STRBUF_INIT;
+	if (*next != line_termination)
+		die("abort: extra input: %s", next);
+	if (ref_transaction_abort(transaction, &error))
+		die("abort: %s", error.buf);
+	puts("abort: ok");
+}
+
+static void parse_cmd_commit(struct ref_transaction *transaction,
+			     const char *next, const char *end)
+{
+	struct strbuf error = STRBUF_INIT;
+	if (*next != line_termination)
+		die("commit: extra input: %s", next);
+	if (ref_transaction_commit(transaction, &error))
+		die("commit: %s", error.buf);
+	puts("commit: ok");
+	ref_transaction_free(transaction);
+}
+
+enum update_refs_state {
+	/* Non-transactional state open for updates. */
+	UPDATE_REFS_OPEN,
+	/* A transaction has been started. */
+	UPDATE_REFS_STARTED,
+	/* References are locked and ready for commit */
+	UPDATE_REFS_PREPARED,
+	/* Transaction has been committed or closed. */
+	UPDATE_REFS_CLOSED,
+};
+
 static const struct parse_cmd {
 	const char *prefix;
 	void (*fn)(struct ref_transaction *, const char *, const char *);
@@ -336,17 +389,23 @@ static const struct parse_cmd {
 	 * of lines.
 	 */
 	unsigned extra_lines;
+	enum update_refs_state state;
 } command[] = {
-	{ "update", parse_cmd_update, 2 },
-	{ "create", parse_cmd_create, 1 },
-	{ "delete", parse_cmd_delete, 1 },
-	{ "verify", parse_cmd_verify, 1 },
-	{ "option", parse_cmd_option, 0 },
+	{ "update",  parse_cmd_update,  2, UPDATE_REFS_OPEN },
+	{ "create",  parse_cmd_create,  1, UPDATE_REFS_OPEN },
+	{ "delete",  parse_cmd_delete,  1, UPDATE_REFS_OPEN },
+	{ "verify",  parse_cmd_verify,  1, UPDATE_REFS_OPEN },
+	{ "option",  parse_cmd_option,  0, UPDATE_REFS_OPEN },
+	{ "start",   parse_cmd_start,   0, UPDATE_REFS_STARTED },
+	{ "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED },
+	{ "abort",   parse_cmd_abort,   0, UPDATE_REFS_CLOSED },
+	{ "commit",  parse_cmd_commit,  0, UPDATE_REFS_CLOSED },
 };
 
 static void update_refs_stdin(void)
 {
 	struct strbuf input = STRBUF_INIT, err = STRBUF_INIT;
+	enum update_refs_state state = UPDATE_REFS_OPEN;
 	struct ref_transaction *transaction;
 	int i, j;
 
@@ -393,14 +452,45 @@ static void update_refs_stdin(void)
 			if (strbuf_appendwholeline(&input, stdin, line_termination))
 				break;
 
+		switch (state) {
+		case UPDATE_REFS_OPEN:
+		case UPDATE_REFS_STARTED:
+			/* Do not downgrade a transaction to a non-transaction. */
+			if (cmd->state >= state)
+				state = cmd->state;
+			break;
+		case UPDATE_REFS_PREPARED:
+			if (cmd->state != UPDATE_REFS_CLOSED)
+				die("prepared transactions can only be closed");
+			state = cmd->state;
+			break;
+		case UPDATE_REFS_CLOSED:
+			die("transaction is closed");
+			break;
+		}
+
 		cmd->fn(transaction, input.buf + strlen(cmd->prefix),
 			input.buf + input.len);
 	}
 
-	if (ref_transaction_commit(transaction, &err))
-		die("%s", err.buf);
+	switch (state) {
+	case UPDATE_REFS_OPEN:
+		/* Commit by default if no transaction was requested. */
+		if (ref_transaction_commit(transaction, &err))
+			die("%s", err.buf);
+		ref_transaction_free(transaction);
+		break;
+	case UPDATE_REFS_STARTED:
+	case UPDATE_REFS_PREPARED:
+		/* If using a transaction, we want to abort it. */
+		if (ref_transaction_abort(transaction, &err))
+			die("%s", err.buf);
+		break;
+	case UPDATE_REFS_CLOSED:
+		/* Otherwise no need to do anything, the transaction was closed already. */
+		break;
+	}
 
-	ref_transaction_free(transaction);
 	strbuf_release(&err);
 	strbuf_release(&input);
 }
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index a6224ef65f..48d0d42afd 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1404,4 +1404,135 @@ test_expect_success 'handle per-worktree refs in refs/bisect' '
 	! test_cmp main-head worktree-head
 '
 
+test_expect_success 'transaction handles empty commit' '
+	cat >stdin <<-EOF &&
+	start
+	prepare
+	commit
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start prepare commit >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction handles empty commit with missing prepare' '
+	cat >stdin <<-EOF &&
+	start
+	commit
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start commit >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction handles sole commit' '
+	cat >stdin <<-EOF &&
+	commit
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" commit >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction handles empty abort' '
+	cat >stdin <<-EOF &&
+	start
+	prepare
+	abort
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start prepare abort >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction exits on multiple aborts' '
+	cat >stdin <<-EOF &&
+	abort
+	abort
+	EOF
+	test_must_fail git update-ref --stdin <stdin >actual 2>err &&
+	printf "%s: ok\n" abort >expect &&
+	test_cmp expect actual &&
+	grep "fatal: transaction is closed" err
+'
+
+test_expect_success 'transaction exits on start after prepare' '
+	cat >stdin <<-EOF &&
+	prepare
+	start
+	EOF
+	test_must_fail git update-ref --stdin <stdin 2>err >actual &&
+	printf "%s: ok\n" prepare >expect &&
+	test_cmp expect actual &&
+	grep "fatal: prepared transactions can only be closed" err
+'
+
+test_expect_success 'transaction handles empty abort with missing prepare' '
+	cat >stdin <<-EOF &&
+	start
+	abort
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start abort >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction handles sole abort' '
+	cat >stdin <<-EOF &&
+	abort
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" abort >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction can handle commit' '
+	cat >stdin <<-EOF &&
+	start
+	create $a HEAD
+	commit
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start commit >expect &&
+	test_cmp expect actual &&
+	git rev-parse HEAD >expect &&
+	git rev-parse $a >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction can handle abort' '
+	cat >stdin <<-EOF &&
+	start
+	create $b HEAD
+	abort
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start abort >expect &&
+	test_cmp expect actual &&
+	test_path_is_missing .git/$b
+'
+
+test_expect_success 'transaction aborts by default' '
+	cat >stdin <<-EOF &&
+	start
+	create $b HEAD
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start >expect &&
+	test_cmp expect actual &&
+	test_path_is_missing .git/$b
+'
+
+test_expect_success 'transaction with prepare aborts by default' '
+	cat >stdin <<-EOF &&
+	start
+	create $b HEAD
+	prepare
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start prepare >expect &&
+	test_cmp expect actual &&
+	test_path_is_missing .git/$b
+'
+
 test_done

-- 
2.26.0
Patrick Steinhardt· Apr 2, 2020, 07:09 UTC · re: Patrick Steinhardt · lore

[PATCH v3 0/9] Support for transactions in `git-update-ref --stdin`

Hi,

this is the second version of this patch series to introduce transaction support in `git update-ref --stdin`. The goal is to make commands available to the user of that command so that he can explicitly control the given transaction by starting, preparing, and finally committing or aborting the transaction.

This version improves the argument handling for commands as proposed by Junio. Instead of the hard-to-understand `extra_lines` field, commands now have a `args` field that specifies how many arguments a given command has, including the first one separated by a space. As a result, the initial space for the first argument is now handled in the generic part and not moved into each of the command functions anymore. I've also added a comment that clarifies why we ignore EOF when fetching additional command lines.

The range-diff against v2 is attached below.
Patrick
Patrick Steinhardt (9):
  refs: fix segfault when aborting empty transaction
  git-update-ref.txt: add missing word
  strbuf: provide function to append whole lines
  update-ref: organize commands in an array
  update-ref: drop unused argument for `parse_refname`
  update-ref: pass end pointer instead of strbuf
  update-ref: move transaction handling into `update_refs_stdin()`
  update-ref: read commands in a line-wise fashion
  update-ref: implement interactive transaction handling
 Documentation/git-update-ref.txt |  28 +++-
 builtin/update-ref.c             | 245 ++++++++++++++++++++++---------
 refs/files-backend.c             |  20 +--
 strbuf.c                         |  10 ++
 strbuf.h                         |   6 +
 t/t1400-update-ref.sh            | 131 +++++++++++++++++
 6 files changed, 364 insertions(+), 76 deletions(-)
Range-diff against v2:
 1:  7a297db4da =  1:  7a297db4da refs: fix segfault when aborting empty transaction
 2:  15857e1b8c =  2:  15857e1b8c git-update-ref.txt: add missing word
 3:  b6546ae44e =  3:  b6546ae44e strbuf: provide function to append whole lines
 4:  bd8c059fbc !  4:  0f881d4936 update-ref: organize commands in an array
    @@ Commit message
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## builtin/update-ref.c ##
    -@@ builtin/update-ref.c: static int parse_next_oid(struct strbuf *input, const char **next,
    - /*
    -  * The following five parse_cmd_*() functions parse the corresponding
    -  * command.  In each case, next points at the character following the
    -- * command name and the following space.  They each return a pointer
    -- * to the character terminating the command, and die with an
    -- * explanatory message if there are any parsing problems.  All of
    -- * these functions handle either text or binary format input,
    -- * depending on how line_termination is set.
    -+ * command name.  They each return a pointer to the character
    -+ * terminating the command, and die with an explanatory message if
    -+ * there are any parsing problems.  All of these functions handle
    -+ * either text or binary format input, depending on how
    -+ * line_termination is set.
    -  */
    - 
    - static const char *parse_cmd_update(struct ref_transaction *transaction,
    -@@ builtin/update-ref.c: static const char *parse_cmd_update(struct ref_transaction *transaction,
    - 	struct object_id new_oid, old_oid;
    - 	int have_old;
    - 
    -+	if (!skip_prefix(next, " ", &next))
    -+		die("update: missing space after command");
    -+
    - 	refname = parse_refname(input, &next);
    - 	if (!refname)
    - 		die("update: missing <ref>");
    -@@ builtin/update-ref.c: static const char *parse_cmd_create(struct ref_transaction *transaction,
    - 	char *refname;
    - 	struct object_id new_oid;
    - 
    -+	if (!skip_prefix(next, " ", &next))
    -+		die("create: missing space after command");
    -+
    - 	refname = parse_refname(input, &next);
    - 	if (!refname)
    - 		die("create: missing <ref>");
    -@@ builtin/update-ref.c: static const char *parse_cmd_delete(struct ref_transaction *transaction,
    - 	struct object_id old_oid;
    - 	int have_old;
    - 
    -+	if (!skip_prefix(next, " ", &next))
    -+		die("delete: missing space after command");
    -+
    - 	refname = parse_refname(input, &next);
    - 	if (!refname)
    - 		die("delete: missing <ref>");
    -@@ builtin/update-ref.c: static const char *parse_cmd_verify(struct ref_transaction *transaction,
    - 	char *refname;
    - 	struct object_id old_oid;
    - 
    -+	if (!skip_prefix(next, " ", &next))
    -+		die("verify: missing space after command");
    -+
    - 	refname = parse_refname(input, &next);
    - 	if (!refname)
    - 		die("verify: missing <ref>");
     @@ builtin/update-ref.c: static const char *parse_cmd_verify(struct ref_transaction *transaction,
      	return next;
      }
    @@ builtin/update-ref.c: static const char *parse_cmd_verify(struct ref_transaction
     +				    struct strbuf *input, const char *next)
      {
      	const char *rest;
    -+	if (!skip_prefix(next, " ", &next))
    -+		die("option: missing space after command");
      	if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination)
    - 		update_flags |= REF_NO_DEREF;
    - 	else
     @@ builtin/update-ref.c: static const char *parse_cmd_option(struct strbuf *input, const char *next)
      	return rest;
      }
    @@ builtin/update-ref.c: static const char *parse_cmd_option(struct strbuf *input,
     +		for (i = 0; i < ARRAY_SIZE(command); i++) {
     +			const char *prefix = command[i].prefix;
     +
    -+			if (!skip_prefix(next, prefix, &next))
    -+				continue;
    -+
    -+			/*
    -+			 * Check that the command is terminated by an argument
    -+			 * or line terminator and not only a matching prefix.
    -+			 */
    -+			if (input.buf[strlen(prefix)] != line_termination &&
    -+			    input.buf[strlen(prefix)] != '\0' &&
    -+			    input.buf[strlen(prefix)] != ' ')
    ++			if (!skip_prefix(next, prefix, &next) ||
    ++			    !skip_prefix(next, " ", &next))
     +				continue;
     +
     +			cmd = &command[i];
 5:  49a14d2046 !  5:  c96395a14d update-ref: drop unused argument for `parse_refname`
    @@ builtin/update-ref.c: static const char *parse_arg(const char *next, struct strb
      	struct strbuf ref = STRBUF_INIT;
      
     @@ builtin/update-ref.c: static const char *parse_cmd_update(struct ref_transaction *transaction,
    - 	if (!skip_prefix(next, " ", &next))
    - 		die("update: missing space after command");
    + 	struct object_id new_oid, old_oid;
    + 	int have_old;
      
     -	refname = parse_refname(input, &next);
     +	refname = parse_refname(&next);
    @@ builtin/update-ref.c: static const char *parse_cmd_update(struct ref_transaction
      		die("update: missing <ref>");
      
     @@ builtin/update-ref.c: static const char *parse_cmd_create(struct ref_transaction *transaction,
    - 	if (!skip_prefix(next, " ", &next))
    - 		die("create: missing space after command");
    + 	char *refname;
    + 	struct object_id new_oid;
      
     -	refname = parse_refname(input, &next);
     +	refname = parse_refname(&next);
    @@ builtin/update-ref.c: static const char *parse_cmd_create(struct ref_transaction
      		die("create: missing <ref>");
      
     @@ builtin/update-ref.c: static const char *parse_cmd_delete(struct ref_transaction *transaction,
    - 	if (!skip_prefix(next, " ", &next))
    - 		die("delete: missing space after command");
    + 	struct object_id old_oid;
    + 	int have_old;
      
     -	refname = parse_refname(input, &next);
     +	refname = parse_refname(&next);
    @@ builtin/update-ref.c: static const char *parse_cmd_delete(struct ref_transaction
      		die("delete: missing <ref>");
      
     @@ builtin/update-ref.c: static const char *parse_cmd_verify(struct ref_transaction *transaction,
    - 	if (!skip_prefix(next, " ", &next))
    - 		die("verify: missing space after command");
    + 	char *refname;
    + 	struct object_id old_oid;
      
     -	refname = parse_refname(input, &next);
     +	refname = parse_refname(&next);
 6:  cbe430029d !  6:  192cbf5944 update-ref: pass end pointer instead of strbuf
    @@ builtin/update-ref.c: static const char *parse_cmd_verify(struct ref_transaction
     +				    const char *next, const char *end)
      {
      	const char *rest;
    - 	if (!skip_prefix(next, " ", &next))
    + 	if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination)
     @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction *transaction,
      
      static const struct parse_cmd {
 7:  d2f68f59a7 =  7:  be7bcf3dbd update-ref: move transaction handling into `update_refs_stdin()`
 8:  f8786fdeb3 !  8:  02ff6b7337 update-ref: read commands in a line-wise fashion
    @@ Commit message
     
      ## builtin/update-ref.c ##
     @@ builtin/update-ref.c: static int parse_next_oid(const char **next, const char *end,
    -  * line_termination is set.
    +  * depending on how line_termination is set.
       */
      
     -static const char *parse_cmd_update(struct ref_transaction *transaction,
    @@ builtin/update-ref.c: static const char *parse_cmd_verify(struct ref_transaction
     +			     const char *next, const char *end)
      {
      	const char *rest;
    - 	if (!skip_prefix(next, " ", &next))
    -@@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction *transaction,
    + 	if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination)
      		update_flags |= REF_NO_DEREF;
      	else
      		die("option unknown: %s", next);
    @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction
      	const char *prefix;
     -	const char *(*fn)(struct ref_transaction *, const char *, const char *);
     +	void (*fn)(struct ref_transaction *, const char *, const char *);
    -+	/*
    -+	 * If using NUL-terminated format, only the first argument will be
    -+	 * available in the first line. In case a command expects more than one
    -+	 * argument, we thus have to fetch an additional `extra_lines` number
    -+	 * of lines.
    -+	 */
    -+	unsigned extra_lines;
    ++	unsigned args;
      } command[] = {
     -	{ "update", parse_cmd_update },
     -	{ "create", parse_cmd_create },
     -	{ "delete", parse_cmd_delete },
     -	{ "verify", parse_cmd_verify },
     -	{ "option", parse_cmd_option },
    -+	{ "update", parse_cmd_update, 2 },
    -+	{ "create", parse_cmd_create, 1 },
    -+	{ "delete", parse_cmd_delete, 1 },
    -+	{ "verify", parse_cmd_verify, 1 },
    -+	{ "option", parse_cmd_option, 0 },
    ++	{ "update", parse_cmd_update, 3 },
    ++	{ "create", parse_cmd_create, 2 },
    ++	{ "delete", parse_cmd_delete, 2 },
    ++	{ "verify", parse_cmd_verify, 2 },
    ++	{ "option", parse_cmd_option, 1 },
      };
      
      static void update_refs_stdin(void)
    @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction
      
      		for (i = 0; i < ARRAY_SIZE(command); i++) {
      			const char *prefix = command[i].prefix;
    ++			char c;
      
    --			if (!skip_prefix(next, prefix, &next))
    +-			if (!skip_prefix(next, prefix, &next) ||
    +-			    !skip_prefix(next, " ", &next))
     +			if (!starts_with(input.buf, prefix))
    ++				continue;
    ++
    ++			/*
    ++			 * If the command has arguments, verify that it's
    ++			 * followed by a space. Otherwise, it shall be followed
    ++			 * by a line terminator.
    ++			 */
    ++			c = command[i].args ? ' ' : line_termination;
    ++			if (input.buf[strlen(prefix)] != c)
      				continue;
      
    - 			/*
    -@@ builtin/update-ref.c: static void update_refs_stdin(void)
    + 			cmd = &command[i];
      			break;
      		}
      		if (!cmd)
    @@ builtin/update-ref.c: static void update_refs_stdin(void)
     -		next = cmd->fn(transaction, next, input.buf + input.len);
     -		next++;
     +		/*
    -+		 * Read extra lines if NUL-terminated. Do not raise an error in
    -+		 * case there is an early EOF to let the command handle missing
    -+		 * arguments with a proper error message.
    ++		 * Read additional arguments if NUL-terminated. Do not raise an
    ++		 * error in case there is an early EOF to let the command
    ++		 * handle missing arguments with a proper error message.
     +		 */
    -+		for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++)
    ++		for (j = 1; line_termination == '\0' && j < cmd->args; j++)
     +			if (strbuf_appendwholeline(&input, stdin, line_termination))
     +				break;
     +
    -+		cmd->fn(transaction, input.buf + strlen(cmd->prefix),
    ++		cmd->fn(transaction, input.buf + strlen(cmd->prefix) + !!cmd->args,
     +			input.buf + input.len);
      	}
      
 9:  c3fffdf9fa !  9:  5670bea2b1 update-ref: implement interactive transaction handling
    @@ builtin/update-ref.c: static void parse_cmd_option(struct ref_transaction *trans
      static const struct parse_cmd {
      	const char *prefix;
      	void (*fn)(struct ref_transaction *, const char *, const char *);
    -@@ builtin/update-ref.c: static const struct parse_cmd {
    - 	 * of lines.
    - 	 */
    - 	unsigned extra_lines;
    + 	unsigned args;
     +	enum update_refs_state state;
      } command[] = {
    --	{ "update", parse_cmd_update, 2 },
    --	{ "create", parse_cmd_create, 1 },
    --	{ "delete", parse_cmd_delete, 1 },
    --	{ "verify", parse_cmd_verify, 1 },
    --	{ "option", parse_cmd_option, 0 },
    -+	{ "update",  parse_cmd_update,  2, UPDATE_REFS_OPEN },
    -+	{ "create",  parse_cmd_create,  1, UPDATE_REFS_OPEN },
    -+	{ "delete",  parse_cmd_delete,  1, UPDATE_REFS_OPEN },
    -+	{ "verify",  parse_cmd_verify,  1, UPDATE_REFS_OPEN },
    -+	{ "option",  parse_cmd_option,  0, UPDATE_REFS_OPEN },
    +-	{ "update", parse_cmd_update, 3 },
    +-	{ "create", parse_cmd_create, 2 },
    +-	{ "delete", parse_cmd_delete, 2 },
    +-	{ "verify", parse_cmd_verify, 2 },
    +-	{ "option", parse_cmd_option, 1 },
    ++	{ "update",  parse_cmd_update,  3, UPDATE_REFS_OPEN },
    ++	{ "create",  parse_cmd_create,  2, UPDATE_REFS_OPEN },
    ++	{ "delete",  parse_cmd_delete,  2, UPDATE_REFS_OPEN },
    ++	{ "verify",  parse_cmd_verify,  2, UPDATE_REFS_OPEN },
    ++	{ "option",  parse_cmd_option,  1, UPDATE_REFS_OPEN },
     +	{ "start",   parse_cmd_start,   0, UPDATE_REFS_STARTED },
     +	{ "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED },
     +	{ "abort",   parse_cmd_abort,   0, UPDATE_REFS_CLOSED },
    @@ builtin/update-ref.c: static void update_refs_stdin(void)
     +			break;
     +		}
     +
    - 		cmd->fn(transaction, input.buf + strlen(cmd->prefix),
    + 		cmd->fn(transaction, input.buf + strlen(cmd->prefix) + !!cmd->args,
      			input.buf + input.len);
      	}
      
-- 
2.26.0
Patrick Steinhardt· Apr 2, 2020, 07:09 UTC · re: Patrick Steinhardt · lore

[PATCH v3 1/9] refs: fix segfault when aborting empty transaction

When cleaning up a transaction that has no updates queued, then the transaction's backend data will not have been allocated. We correctly handle this for the packed backend, where the cleanup function checks whether the backend data has been allocated at all -- if not, then there is nothing to clean up. For the files backend we do not check this and as a result will hit a segfault due to dereferencing a `NULL` pointer when cleaning up such a transaction.

Fix the issue by checking whether `backend_data` is set in the files backend, too.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)
Show changes to refs/files-backend.c +11 −9
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 561c33ac8a..6516c7bc8c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2565,17 +2565,19 @@ static void files_transaction_cleanup(struct files_ref_store *refs,
 		}
 	}
 
-	if (backend_data->packed_transaction &&
-	    ref_transaction_abort(backend_data->packed_transaction, &err)) {
-		error("error aborting transaction: %s", err.buf);
-		strbuf_release(&err);
+	if (backend_data) {
+		if (backend_data->packed_transaction &&
+		    ref_transaction_abort(backend_data->packed_transaction, &err)) {
+			error("error aborting transaction: %s", err.buf);
+			strbuf_release(&err);
+		}
+
+		if (backend_data->packed_refs_locked)
+			packed_refs_unlock(refs->packed_ref_store);
+
+		free(backend_data);
 	}
 
-	if (backend_data->packed_refs_locked)
-		packed_refs_unlock(refs->packed_ref_store);
-
-	free(backend_data);
-
 	transaction->state = REF_TRANSACTION_CLOSED;
 }
 

-- 
2.26.0
Patrick Steinhardt· Apr 2, 2020, 07:09 UTC · re: Patrick Steinhardt · lore

[PATCH v3 2/9] git-update-ref.txt: add missing word

The description for the "verify" command is lacking a single word "is", which this commit corrects.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git-update-ref.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Documentation/git-update-ref.txt +1 −1
diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 9671423117..9bd039ce08 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -107,7 +107,7 @@ delete::
 
 verify::
 	Verify <ref> against <oldvalue> but do not change it.  If
-	<oldvalue> zero or missing, the ref must not exist.
+	<oldvalue> is zero or missing, the ref must not exist.
 
 option::
 	Modify behavior of the next command naming a <ref>.

-- 
2.26.0
Patrick Steinhardt· Apr 2, 2020, 07:09 UTC · re: Patrick Steinhardt · lore

[PATCH v3 3/9] strbuf: provide function to append whole lines

While the strbuf interface already provides functions to read a line into it that completely replaces its current contents, we do not have an interface that allows for appending lines without discarding current contents.

Add a new function `strbuf_appendwholeline` that reads a line including its terminating character into a strbuf non-destructively. This is a preparatory step for git-update-ref(1) reading standard input line-wise instead of as a block.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 strbuf.c | 10 ++++++++++
 strbuf.h |  6 ++++++
 2 files changed, 16 insertions(+)
Show changes to 2 files +16 −0

strbuf.c, strbuf.h

diff --git a/strbuf.c b/strbuf.c
index bb0065ccaf..6e74901bfa 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -690,6 +690,16 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 }
 #endif
 
+int strbuf_appendwholeline(struct strbuf *sb, FILE *fp, int term)
+{
+	struct strbuf line = STRBUF_INIT;
+	if (strbuf_getwholeline(&line, fp, term))
+		return EOF;
+	strbuf_addbuf(sb, &line);
+	strbuf_release(&line);
+	return 0;
+}
+
 static int strbuf_getdelim(struct strbuf *sb, FILE *fp, int term)
 {
 	if (strbuf_getwholeline(sb, fp, term))
diff --git a/strbuf.h b/strbuf.h
index ce8e49c0b2..411063ca76 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -502,6 +502,12 @@ int strbuf_getline(struct strbuf *sb, FILE *file);
  */
 int strbuf_getwholeline(struct strbuf *sb, FILE *file, int term);
 
+/**
+ * Like `strbuf_getwholeline`, but appends the line instead of
+ * resetting the buffer first.
+ */
+int strbuf_appendwholeline(struct strbuf *sb, FILE *file, int term);
+
 /**
  * Like `strbuf_getwholeline`, but operates on a file descriptor.
  * It reads one character at a time, so it is very slow.  Do not

-- 
2.26.0
Patrick Steinhardt· Apr 2, 2020, 07:09 UTC · re: Patrick Steinhardt · lore

[PATCH v3 4/9] update-ref: organize commands in an array

We currently manually wire up all commands known to `git-update-ref --stdin`, making it harder than necessary to preprocess arguments after the command is determined. To make this more extensible, let's refactor the code to use an array of known commands instead. While this doesn't add a lot of value now, it is a preparatory step to implement line-wise reading of commands.

As we're going to introduce commands without trailing spaces, this commit also moves whitespace parsing into the respective commands.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)
Show changes to builtin/update-ref.c +29 −12
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 2d8f7f0578..a6ff88b95b 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -310,7 +310,8 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 	return next;
 }
 
-static const char *parse_cmd_option(struct strbuf *input, const char *next)
+static const char *parse_cmd_option(struct ref_transaction *transaction,
+				    struct strbuf *input, const char *next)
 {
 	const char *rest;
 	if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination)
@@ -320,33 +321,49 @@ static const char *parse_cmd_option(struct strbuf *input, const char *next)
 	return rest;
 }
 
+static const struct parse_cmd {
+	const char *prefix;
+	const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);
+} command[] = {
+	{ "update", parse_cmd_update },
+	{ "create", parse_cmd_create },
+	{ "delete", parse_cmd_delete },
+	{ "verify", parse_cmd_verify },
+	{ "option", parse_cmd_option },
+};
+
 static void update_refs_stdin(struct ref_transaction *transaction)
 {
 	struct strbuf input = STRBUF_INIT;
 	const char *next;
+	int i;
 
 	if (strbuf_read(&input, 0, 1000) < 0)
 		die_errno("could not read from stdin");
 	next = input.buf;
 	/* Read each line dispatch its command */
 	while (next < input.buf + input.len) {
+		const struct parse_cmd *cmd = NULL;
+
 		if (*next == line_termination)
 			die("empty command in input");
 		else if (isspace(*next))
 			die("whitespace before command: %s", next);
-		else if (skip_prefix(next, "update ", &next))
-			next = parse_cmd_update(transaction, &input, next);
-		else if (skip_prefix(next, "create ", &next))
-			next = parse_cmd_create(transaction, &input, next);
-		else if (skip_prefix(next, "delete ", &next))
-			next = parse_cmd_delete(transaction, &input, next);
-		else if (skip_prefix(next, "verify ", &next))
-			next = parse_cmd_verify(transaction, &input, next);
-		else if (skip_prefix(next, "option ", &next))
-			next = parse_cmd_option(&input, next);
-		else
+
+		for (i = 0; i < ARRAY_SIZE(command); i++) {
+			const char *prefix = command[i].prefix;
+
+			if (!skip_prefix(next, prefix, &next) ||
+			    !skip_prefix(next, " ", &next))
+				continue;
+
+			cmd = &command[i];
+			break;
+		}
+		if (!cmd)
 			die("unknown command: %s", next);
 
+		next = cmd->fn(transaction, &input, next);
 		next++;
 	}
 

-- 
2.26.0
Patrick Steinhardt· Apr 2, 2020, 07:09 UTC · re: Patrick Steinhardt · lore

[PATCH v3 5/9] update-ref: drop unused argument for `parse_refname`

The `parse_refname` function accepts a `struct strbuf *input` argument that isn't used at all. As we're about to convert commands to not use a strbuf anymore but instead an end pointer, let's drop this argument now to make the converting commit easier to review.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to builtin/update-ref.c +5 −5
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index a6ff88b95b..1bba5ea6c2 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -50,7 +50,7 @@ static const char *parse_arg(const char *next, struct strbuf *arg)
  * the argument.  Die if C-quoting is malformed or the reference name
  * is invalid.
  */
-static char *parse_refname(struct strbuf *input, const char **next)
+static char *parse_refname(const char **next)
 {
 	struct strbuf ref = STRBUF_INIT;
 
@@ -186,7 +186,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 	struct object_id new_oid, old_oid;
 	int have_old;
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("update: missing <ref>");
 
@@ -220,7 +220,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 	char *refname;
 	struct object_id new_oid;
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("create: missing <ref>");
 
@@ -253,7 +253,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 	struct object_id old_oid;
 	int have_old;
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("delete: missing <ref>");
 
@@ -288,7 +288,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 	char *refname;
 	struct object_id old_oid;
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("verify: missing <ref>");
 

-- 
2.26.0
Patrick Steinhardt· Apr 2, 2020, 07:09 UTC · re: Patrick Steinhardt · lore

[PATCH v3 6/9] update-ref: pass end pointer instead of strbuf

We currently pass both an `strbuf` containing the current command line as well as the `next` pointer pointing to the first argument to commands. This is both confusing and makes code more intertwined. Convert this to use a simple pointer as well as a pointer pointing to the end of the input as a preparatory step to line-wise reading of stdin.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)
Show changes to builtin/update-ref.c +15 −15
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 1bba5ea6c2..381d347fb4 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -95,7 +95,7 @@ static char *parse_refname(const char **next)
  * provided but cannot be converted to a SHA-1, die.  flags can
  * include PARSE_SHA1_OLD and/or PARSE_SHA1_ALLOW_EMPTY.
  */
-static int parse_next_oid(struct strbuf *input, const char **next,
+static int parse_next_oid(const char **next, const char *end,
 			  struct object_id *oid,
 			  const char *command, const char *refname,
 			  int flags)
@@ -103,7 +103,7 @@ static int parse_next_oid(struct strbuf *input, const char **next,
 	struct strbuf arg = STRBUF_INIT;
 	int ret = 0;
 
-	if (*next == input->buf + input->len)
+	if (*next == end)
 		goto eof;
 
 	if (line_termination) {
@@ -128,7 +128,7 @@ static int parse_next_oid(struct strbuf *input, const char **next,
 			die("%s %s: expected NUL but got: %s",
 			    command, refname, *next);
 		(*next)++;
-		if (*next == input->buf + input->len)
+		if (*next == end)
 			goto eof;
 		strbuf_addstr(&arg, *next);
 		*next += arg.len;
@@ -179,7 +179,7 @@ static int parse_next_oid(struct strbuf *input, const char **next,
  */
 
 static const char *parse_cmd_update(struct ref_transaction *transaction,
-				    struct strbuf *input, const char *next)
+				    const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -190,11 +190,11 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 	if (!refname)
 		die("update: missing <ref>");
 
-	if (parse_next_oid(input, &next, &new_oid, "update", refname,
+	if (parse_next_oid(&next, end, &new_oid, "update", refname,
 			   PARSE_SHA1_ALLOW_EMPTY))
 		die("update %s: missing <newvalue>", refname);
 
-	have_old = !parse_next_oid(input, &next, &old_oid, "update", refname,
+	have_old = !parse_next_oid(&next, end, &old_oid, "update", refname,
 				   PARSE_SHA1_OLD);
 
 	if (*next != line_termination)
@@ -214,7 +214,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 }
 
 static const char *parse_cmd_create(struct ref_transaction *transaction,
-				    struct strbuf *input, const char *next)
+				    const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -224,7 +224,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 	if (!refname)
 		die("create: missing <ref>");
 
-	if (parse_next_oid(input, &next, &new_oid, "create", refname, 0))
+	if (parse_next_oid(&next, end, &new_oid, "create", refname, 0))
 		die("create %s: missing <newvalue>", refname);
 
 	if (is_null_oid(&new_oid))
@@ -246,7 +246,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 }
 
 static const char *parse_cmd_delete(struct ref_transaction *transaction,
-				    struct strbuf *input, const char *next)
+				    const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -257,7 +257,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 	if (!refname)
 		die("delete: missing <ref>");
 
-	if (parse_next_oid(input, &next, &old_oid, "delete", refname,
+	if (parse_next_oid(&next, end, &old_oid, "delete", refname,
 			   PARSE_SHA1_OLD)) {
 		have_old = 0;
 	} else {
@@ -282,7 +282,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 }
 
 static const char *parse_cmd_verify(struct ref_transaction *transaction,
-				    struct strbuf *input, const char *next)
+				    const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -292,7 +292,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 	if (!refname)
 		die("verify: missing <ref>");
 
-	if (parse_next_oid(input, &next, &old_oid, "verify", refname,
+	if (parse_next_oid(&next, end, &old_oid, "verify", refname,
 			   PARSE_SHA1_OLD))
 		oidclr(&old_oid);
 
@@ -311,7 +311,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 }
 
 static const char *parse_cmd_option(struct ref_transaction *transaction,
-				    struct strbuf *input, const char *next)
+				    const char *next, const char *end)
 {
 	const char *rest;
 	if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination)
@@ -323,7 +323,7 @@ static const char *parse_cmd_option(struct ref_transaction *transaction,
 
 static const struct parse_cmd {
 	const char *prefix;
-	const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);
+	const char *(*fn)(struct ref_transaction *, const char *, const char *);
 } command[] = {
 	{ "update", parse_cmd_update },
 	{ "create", parse_cmd_create },
@@ -363,7 +363,7 @@ static void update_refs_stdin(struct ref_transaction *transaction)
 		if (!cmd)
 			die("unknown command: %s", next);
 
-		next = cmd->fn(transaction, &input, next);
+		next = cmd->fn(transaction, next, input.buf + input.len);
 		next++;
 	}
 

-- 
2.26.0
Patrick Steinhardt· Apr 2, 2020, 07:09 UTC · re: Patrick Steinhardt · lore

[PATCH v3 7/9] update-ref: move transaction handling into `update_refs_stdin()`

While the actual logic to update the transaction is handled in `update_refs_stdin()`, the transaction itself is started and committed in `cmd_update_ref()` itself. This makes it hard to handle transaction abortion and commits as part of `update_refs_stdin()` itself, which is required in order to introduce transaction handling features to `git update-refs --stdin`.

Refactor the code to move all transaction handling into `update_refs_stdin()` to prepare for transaction handling features.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)
Show changes to builtin/update-ref.c +14 −13
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 381d347fb4..0f34b68904 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -332,15 +332,21 @@ static const struct parse_cmd {
 	{ "option", parse_cmd_option },
 };
 
-static void update_refs_stdin(struct ref_transaction *transaction)
+static void update_refs_stdin(void)
 {
-	struct strbuf input = STRBUF_INIT;
+	struct strbuf input = STRBUF_INIT, err = STRBUF_INIT;
+	struct ref_transaction *transaction;
 	const char *next;
 	int i;
 
 	if (strbuf_read(&input, 0, 1000) < 0)
 		die_errno("could not read from stdin");
 	next = input.buf;
+
+	transaction = ref_transaction_begin(&err);
+	if (!transaction)
+		die("%s", err.buf);
+
 	/* Read each line dispatch its command */
 	while (next < input.buf + input.len) {
 		const struct parse_cmd *cmd = NULL;
@@ -367,6 +373,11 @@ static void update_refs_stdin(struct ref_transaction *transaction)
 		next++;
 	}
 
+	if (ref_transaction_commit(transaction, &err))
+		die("%s", err.buf);
+
+	ref_transaction_free(transaction);
+	strbuf_release(&err);
 	strbuf_release(&input);
 }
 
@@ -401,21 +412,11 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 	}
 
 	if (read_stdin) {
-		struct strbuf err = STRBUF_INIT;
-		struct ref_transaction *transaction;
-
-		transaction = ref_transaction_begin(&err);
-		if (!transaction)
-			die("%s", err.buf);
 		if (delete || argc > 0)
 			usage_with_options(git_update_ref_usage, options);
 		if (end_null)
 			line_termination = '\0';
-		update_refs_stdin(transaction);
-		if (ref_transaction_commit(transaction, &err))
-			die("%s", err.buf);
-		ref_transaction_free(transaction);
-		strbuf_release(&err);
+		update_refs_stdin();
 		return 0;
 	}
 

-- 
2.26.0
Patrick Steinhardt· Apr 2, 2020, 07:09 UTC · re: Patrick Steinhardt · lore

[PATCH v3 8/9] update-ref: read commands in a line-wise fashion

The git-update-ref(1) supports a `--stdin` mode that allows it to read all reference updates from standard input. This is mainly used to allow for atomic reference updates that are all or nothing, so that either all references will get updated or none.

Currently, git-update-ref(1) reads all commands as a single block of up to 1000 characters and only starts processing after stdin gets closed. This is less flexible than one might wish for, as it doesn't really allow for longer-lived transactions and doesn't allow any verification without committing everything. E.g. one may imagine the following exchange:

    > start
    < start: ok
    > update refs/heads/master $NEWOID1 $OLDOID1
    > update refs/heads/branch $NEWOID2 $OLDOID2
    > prepare
    < prepare: ok
    > commit
    < commit: ok

When reading all input as a whole block, the above interactive protocol is obviously impossible to achieve. But by converting the command to read commands linewise, we can make it more interactive than before.

Obviously, the linewise interface is only a first step in making git-update-ref(1) work in a more transaction-oriented way. Missing is most importantly support for transactional commands that manage the current transaction.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 85 +++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 40 deletions(-)
Show changes to builtin/update-ref.c +45 −40
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 0f34b68904..348407b896 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -178,8 +178,8 @@ static int parse_next_oid(const char **next, const char *end,
  * depending on how line_termination is set.
  */
 
-static const char *parse_cmd_update(struct ref_transaction *transaction,
-				    const char *next, const char *end)
+static void parse_cmd_update(struct ref_transaction *transaction,
+			     const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -209,12 +209,10 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 	update_flags = default_flags;
 	free(refname);
 	strbuf_release(&err);
-
-	return next;
 }
 
-static const char *parse_cmd_create(struct ref_transaction *transaction,
-				    const char *next, const char *end)
+static void parse_cmd_create(struct ref_transaction *transaction,
+			     const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -241,12 +239,10 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 	update_flags = default_flags;
 	free(refname);
 	strbuf_release(&err);
-
-	return next;
 }
 
-static const char *parse_cmd_delete(struct ref_transaction *transaction,
-				    const char *next, const char *end)
+static void parse_cmd_delete(struct ref_transaction *transaction,
+			     const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -277,12 +273,10 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 	update_flags = default_flags;
 	free(refname);
 	strbuf_release(&err);
-
-	return next;
 }
 
-static const char *parse_cmd_verify(struct ref_transaction *transaction,
-				    const char *next, const char *end)
+static void parse_cmd_verify(struct ref_transaction *transaction,
+			     const char *next, const char *end)
 {
 	struct strbuf err = STRBUF_INIT;
 	char *refname;
@@ -306,71 +300,82 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 	update_flags = default_flags;
 	free(refname);
 	strbuf_release(&err);
-
-	return next;
 }
 
-static const char *parse_cmd_option(struct ref_transaction *transaction,
-				    const char *next, const char *end)
+static void parse_cmd_option(struct ref_transaction *transaction,
+			     const char *next, const char *end)
 {
 	const char *rest;
 	if (skip_prefix(next, "no-deref", &rest) && *rest == line_termination)
 		update_flags |= REF_NO_DEREF;
 	else
 		die("option unknown: %s", next);
-	return rest;
 }
 
 static const struct parse_cmd {
 	const char *prefix;
-	const char *(*fn)(struct ref_transaction *, const char *, const char *);
+	void (*fn)(struct ref_transaction *, const char *, const char *);
+	unsigned args;
 } command[] = {
-	{ "update", parse_cmd_update },
-	{ "create", parse_cmd_create },
-	{ "delete", parse_cmd_delete },
-	{ "verify", parse_cmd_verify },
-	{ "option", parse_cmd_option },
+	{ "update", parse_cmd_update, 3 },
+	{ "create", parse_cmd_create, 2 },
+	{ "delete", parse_cmd_delete, 2 },
+	{ "verify", parse_cmd_verify, 2 },
+	{ "option", parse_cmd_option, 1 },
 };
 
 static void update_refs_stdin(void)
 {
 	struct strbuf input = STRBUF_INIT, err = STRBUF_INIT;
 	struct ref_transaction *transaction;
-	const char *next;
-	int i;
-
-	if (strbuf_read(&input, 0, 1000) < 0)
-		die_errno("could not read from stdin");
-	next = input.buf;
+	int i, j;
 
 	transaction = ref_transaction_begin(&err);
 	if (!transaction)
 		die("%s", err.buf);
 
 	/* Read each line dispatch its command */
-	while (next < input.buf + input.len) {
+	while (!strbuf_getwholeline(&input, stdin, line_termination)) {
 		const struct parse_cmd *cmd = NULL;
 
-		if (*next == line_termination)
+		if (*input.buf == line_termination)
 			die("empty command in input");
-		else if (isspace(*next))
-			die("whitespace before command: %s", next);
+		else if (isspace(*input.buf))
+			die("whitespace before command: %s", input.buf);
 
 		for (i = 0; i < ARRAY_SIZE(command); i++) {
 			const char *prefix = command[i].prefix;
+			char c;
 
-			if (!skip_prefix(next, prefix, &next) ||
-			    !skip_prefix(next, " ", &next))
+			if (!starts_with(input.buf, prefix))
+				continue;
+
+			/*
+			 * If the command has arguments, verify that it's
+			 * followed by a space. Otherwise, it shall be followed
+			 * by a line terminator.
+			 */
+			c = command[i].args ? ' ' : line_termination;
+			if (input.buf[strlen(prefix)] != c)
 				continue;
 
 			cmd = &command[i];
 			break;
 		}
 		if (!cmd)
-			die("unknown command: %s", next);
+			die("unknown command: %s", input.buf);
 
-		next = cmd->fn(transaction, next, input.buf + input.len);
-		next++;
+		/*
+		 * Read additional arguments if NUL-terminated. Do not raise an
+		 * error in case there is an early EOF to let the command
+		 * handle missing arguments with a proper error message.
+		 */
+		for (j = 1; line_termination == '\0' && j < cmd->args; j++)
+			if (strbuf_appendwholeline(&input, stdin, line_termination))
+				break;
+
+		cmd->fn(transaction, input.buf + strlen(cmd->prefix) + !!cmd->args,
+			input.buf + input.len);
 	}
 
 	if (ref_transaction_commit(transaction, &err))

-- 
2.26.0
Patrick Steinhardt· Apr 2, 2020, 07:10 UTC · re: Patrick Steinhardt · lore

[PATCH v3 9/9] update-ref: implement interactive transaction handling

The git-update-ref(1) command can only handle queueing transactions right now via its "--stdin" parameter, but there is no way for users to handle the transaction itself in a more explicit way. E.g. in a replicated scenario, one may imagine a coordinator that spawns git-update-ref(1) for multiple repositories and only if all agree that an update is possible will the coordinator send a commit. Such a transactional session could look like

    > start
    < start: ok
    > update refs/heads/master $OLD $NEW
    > prepare
    < prepare: ok
    # All nodes have returned "ok"
    > commit
    < commit: ok
or
    > start
    < start: ok
    > create refs/heads/master $OLD $NEW
    > prepare
    < fatal: cannot lock ref 'refs/heads/master': reference already exists
    # On all other nodes:
    > abort
    < abort: ok

In order to allow for such transactional sessions, this commit introduces four new commands for git-update-ref(1), which matches those we have internally already with the exception of "start":

    - start: start a new transaction
    - prepare: prepare the transaction, that is try to lock all
               references and verify their current value matches the
               expected one
    - commit: explicitly commit a session, that is update references to
              match their new expected state
    - abort: abort a session and roll back all changes

By design, git-update-ref(1) will commit as soon as standard input is being closed. While fine in a non-transactional world, it is definitely unexpected in a transactional world. Because of this, as soon as any of the new transactional commands is used, the default will change to aborting without an explicit "commit". To avoid a race between queueing updates and the first "prepare" that starts a transaction, the "start" command has been added to start an explicit transaction.

Add some tests to exercise this new functionality.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git-update-ref.txt |  26 ++++++
 builtin/update-ref.c             | 106 +++++++++++++++++++++++--
 t/t1400-update-ref.sh            | 131 +++++++++++++++++++++++++++++++
 3 files changed, 255 insertions(+), 8 deletions(-)
Show changes to 3 files +255 −8

Documentation/git-update-ref.txt, builtin/update-ref.c, t/t1400-update-ref.sh

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 9bd039ce08..3e737c2360 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -66,6 +66,10 @@ performs all modifications together.  Specify commands of the form:
 	delete SP <ref> [SP <oldvalue>] LF
 	verify SP <ref> [SP <oldvalue>] LF
 	option SP <opt> LF
+	start LF
+	prepare LF
+	commit LF
+	abort LF
 
 With `--create-reflog`, update-ref will create a reflog for each ref
 even if one would not ordinarily be created.
@@ -83,6 +87,10 @@ quoting:
 	delete SP <ref> NUL [<oldvalue>] NUL
 	verify SP <ref> NUL [<oldvalue>] NUL
 	option SP <opt> NUL
+	start NUL
+	prepare NUL
+	commit NUL
+	abort NUL
 
 In this format, use 40 "0" to specify a zero value, and use the empty
 string to specify a missing value.
@@ -114,6 +122,24 @@ option::
 	The only valid option is `no-deref` to avoid dereferencing
 	a symbolic ref.
 
+start::
+	Start a transaction. In contrast to a non-transactional session, a
+	transaction will automatically abort if the session ends without an
+	explicit commit.
+
+prepare::
+	Prepare to commit the transaction. This will create lock files for all
+	queued reference updates. If one reference could not be locked, the
+	transaction will be aborted.
+
+commit::
+	Commit all reference updates queued for the transaction, ending the
+	transaction.
+
+abort::
+	Abort the transaction, releasing all locks if the transaction is in
+	prepared state.
+
 If all <ref>s can be locked with matching <oldvalue>s
 simultaneously, all modifications are performed.  Otherwise, no
 modifications are performed.  Note that while each individual
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 348407b896..b74dd9a69d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -312,21 +312,80 @@ static void parse_cmd_option(struct ref_transaction *transaction,
 		die("option unknown: %s", next);
 }
 
+static void parse_cmd_start(struct ref_transaction *transaction,
+			    const char *next, const char *end)
+{
+	if (*next != line_termination)
+		die("start: extra input: %s", next);
+	puts("start: ok");
+}
+
+static void parse_cmd_prepare(struct ref_transaction *transaction,
+			      const char *next, const char *end)
+{
+	struct strbuf error = STRBUF_INIT;
+	if (*next != line_termination)
+		die("prepare: extra input: %s", next);
+	if (ref_transaction_prepare(transaction, &error))
+		die("prepare: %s", error.buf);
+	puts("prepare: ok");
+}
+
+static void parse_cmd_abort(struct ref_transaction *transaction,
+			    const char *next, const char *end)
+{
+	struct strbuf error = STRBUF_INIT;
+	if (*next != line_termination)
+		die("abort: extra input: %s", next);
+	if (ref_transaction_abort(transaction, &error))
+		die("abort: %s", error.buf);
+	puts("abort: ok");
+}
+
+static void parse_cmd_commit(struct ref_transaction *transaction,
+			     const char *next, const char *end)
+{
+	struct strbuf error = STRBUF_INIT;
+	if (*next != line_termination)
+		die("commit: extra input: %s", next);
+	if (ref_transaction_commit(transaction, &error))
+		die("commit: %s", error.buf);
+	puts("commit: ok");
+	ref_transaction_free(transaction);
+}
+
+enum update_refs_state {
+	/* Non-transactional state open for updates. */
+	UPDATE_REFS_OPEN,
+	/* A transaction has been started. */
+	UPDATE_REFS_STARTED,
+	/* References are locked and ready for commit */
+	UPDATE_REFS_PREPARED,
+	/* Transaction has been committed or closed. */
+	UPDATE_REFS_CLOSED,
+};
+
 static const struct parse_cmd {
 	const char *prefix;
 	void (*fn)(struct ref_transaction *, const char *, const char *);
 	unsigned args;
+	enum update_refs_state state;
 } command[] = {
-	{ "update", parse_cmd_update, 3 },
-	{ "create", parse_cmd_create, 2 },
-	{ "delete", parse_cmd_delete, 2 },
-	{ "verify", parse_cmd_verify, 2 },
-	{ "option", parse_cmd_option, 1 },
+	{ "update",  parse_cmd_update,  3, UPDATE_REFS_OPEN },
+	{ "create",  parse_cmd_create,  2, UPDATE_REFS_OPEN },
+	{ "delete",  parse_cmd_delete,  2, UPDATE_REFS_OPEN },
+	{ "verify",  parse_cmd_verify,  2, UPDATE_REFS_OPEN },
+	{ "option",  parse_cmd_option,  1, UPDATE_REFS_OPEN },
+	{ "start",   parse_cmd_start,   0, UPDATE_REFS_STARTED },
+	{ "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED },
+	{ "abort",   parse_cmd_abort,   0, UPDATE_REFS_CLOSED },
+	{ "commit",  parse_cmd_commit,  0, UPDATE_REFS_CLOSED },
 };
 
 static void update_refs_stdin(void)
 {
 	struct strbuf input = STRBUF_INIT, err = STRBUF_INIT;
+	enum update_refs_state state = UPDATE_REFS_OPEN;
 	struct ref_transaction *transaction;
 	int i, j;
 
@@ -374,14 +433,45 @@ static void update_refs_stdin(void)
 			if (strbuf_appendwholeline(&input, stdin, line_termination))
 				break;
 
+		switch (state) {
+		case UPDATE_REFS_OPEN:
+		case UPDATE_REFS_STARTED:
+			/* Do not downgrade a transaction to a non-transaction. */
+			if (cmd->state >= state)
+				state = cmd->state;
+			break;
+		case UPDATE_REFS_PREPARED:
+			if (cmd->state != UPDATE_REFS_CLOSED)
+				die("prepared transactions can only be closed");
+			state = cmd->state;
+			break;
+		case UPDATE_REFS_CLOSED:
+			die("transaction is closed");
+			break;
+		}
+
 		cmd->fn(transaction, input.buf + strlen(cmd->prefix) + !!cmd->args,
 			input.buf + input.len);
 	}
 
-	if (ref_transaction_commit(transaction, &err))
-		die("%s", err.buf);
+	switch (state) {
+	case UPDATE_REFS_OPEN:
+		/* Commit by default if no transaction was requested. */
+		if (ref_transaction_commit(transaction, &err))
+			die("%s", err.buf);
+		ref_transaction_free(transaction);
+		break;
+	case UPDATE_REFS_STARTED:
+	case UPDATE_REFS_PREPARED:
+		/* If using a transaction, we want to abort it. */
+		if (ref_transaction_abort(transaction, &err))
+			die("%s", err.buf);
+		break;
+	case UPDATE_REFS_CLOSED:
+		/* Otherwise no need to do anything, the transaction was closed already. */
+		break;
+	}
 
-	ref_transaction_free(transaction);
 	strbuf_release(&err);
 	strbuf_release(&input);
 }
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index a6224ef65f..48d0d42afd 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1404,4 +1404,135 @@ test_expect_success 'handle per-worktree refs in refs/bisect' '
 	! test_cmp main-head worktree-head
 '
 
+test_expect_success 'transaction handles empty commit' '
+	cat >stdin <<-EOF &&
+	start
+	prepare
+	commit
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start prepare commit >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction handles empty commit with missing prepare' '
+	cat >stdin <<-EOF &&
+	start
+	commit
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start commit >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction handles sole commit' '
+	cat >stdin <<-EOF &&
+	commit
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" commit >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction handles empty abort' '
+	cat >stdin <<-EOF &&
+	start
+	prepare
+	abort
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start prepare abort >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction exits on multiple aborts' '
+	cat >stdin <<-EOF &&
+	abort
+	abort
+	EOF
+	test_must_fail git update-ref --stdin <stdin >actual 2>err &&
+	printf "%s: ok\n" abort >expect &&
+	test_cmp expect actual &&
+	grep "fatal: transaction is closed" err
+'
+
+test_expect_success 'transaction exits on start after prepare' '
+	cat >stdin <<-EOF &&
+	prepare
+	start
+	EOF
+	test_must_fail git update-ref --stdin <stdin 2>err >actual &&
+	printf "%s: ok\n" prepare >expect &&
+	test_cmp expect actual &&
+	grep "fatal: prepared transactions can only be closed" err
+'
+
+test_expect_success 'transaction handles empty abort with missing prepare' '
+	cat >stdin <<-EOF &&
+	start
+	abort
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start abort >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction handles sole abort' '
+	cat >stdin <<-EOF &&
+	abort
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" abort >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction can handle commit' '
+	cat >stdin <<-EOF &&
+	start
+	create $a HEAD
+	commit
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start commit >expect &&
+	test_cmp expect actual &&
+	git rev-parse HEAD >expect &&
+	git rev-parse $a >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'transaction can handle abort' '
+	cat >stdin <<-EOF &&
+	start
+	create $b HEAD
+	abort
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start abort >expect &&
+	test_cmp expect actual &&
+	test_path_is_missing .git/$b
+'
+
+test_expect_success 'transaction aborts by default' '
+	cat >stdin <<-EOF &&
+	start
+	create $b HEAD
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start >expect &&
+	test_cmp expect actual &&
+	test_path_is_missing .git/$b
+'
+
+test_expect_success 'transaction with prepare aborts by default' '
+	cat >stdin <<-EOF &&
+	start
+	create $b HEAD
+	prepare
+	EOF
+	git update-ref --stdin <stdin >actual &&
+	printf "%s: ok\n" start prepare >expect &&
+	test_cmp expect actual &&
+	test_path_is_missing .git/$b
+'
+
 test_done

-- 
2.26.0
Phillip Wood· Apr 3, 2020, 13:40 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v3 9/9] update-ref: implement interactive transaction handling

Hi Patrick
On 02/04/2020 08:10, Patrick Steinhardt wrote:
Show 92 quoted lines
> The git-update-ref(1) command can only handle queueing transactions
> right now via its "--stdin" parameter, but there is no way for users to
> handle the transaction itself in a more explicit way. E.g. in a
> replicated scenario, one may imagine a coordinator that spawns
> git-update-ref(1) for multiple repositories and only if all agree that
> an update is possible will the coordinator send a commit. Such a
> transactional session could look like
> 
>      > start
>      < start: ok
>      > update refs/heads/master $OLD $NEW
>      > prepare
>      < prepare: ok
>      # All nodes have returned "ok"
>      > commit
>      < commit: ok
> 
> or
> 
>      > start
>      < start: ok
>      > create refs/heads/master $OLD $NEW
>      > prepare
>      < fatal: cannot lock ref 'refs/heads/master': reference already exists
>      # On all other nodes:
>      > abort
>      < abort: ok
> 
> In order to allow for such transactional sessions, this commit
> introduces four new commands for git-update-ref(1), which matches those
> we have internally already with the exception of "start":
> 
>      - start: start a new transaction
> 
>      - prepare: prepare the transaction, that is try to lock all
>                 references and verify their current value matches the
>                 expected one
> 
>      - commit: explicitly commit a session, that is update references to
>                match their new expected state
> 
>      - abort: abort a session and roll back all changes
> 
> By design, git-update-ref(1) will commit as soon as standard input is
> being closed. While fine in a non-transactional world, it is definitely
> unexpected in a transactional world. Because of this, as soon as any of
> the new transactional commands is used, the default will change to
> aborting without an explicit "commit". To avoid a race between queueing
> updates and the first "prepare" that starts a transaction, the "start"
> command has been added to start an explicit transaction.
> 
> Add some tests to exercise this new functionality.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>   Documentation/git-update-ref.txt |  26 ++++++
>   builtin/update-ref.c             | 106 +++++++++++++++++++++++--
>   t/t1400-update-ref.sh            | 131 +++++++++++++++++++++++++++++++
>   3 files changed, 255 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
> index 9bd039ce08..3e737c2360 100644
> --- a/Documentation/git-update-ref.txt
> +++ b/Documentation/git-update-ref.txt
> @@ -66,6 +66,10 @@ performs all modifications together.  Specify commands of the form:
>   	delete SP <ref> [SP <oldvalue>] LF
>   	verify SP <ref> [SP <oldvalue>] LF
>   	option SP <opt> LF
> +	start LF
> +	prepare LF
> +	commit LF
> +	abort LF
>   
>   With `--create-reflog`, update-ref will create a reflog for each ref
>   even if one would not ordinarily be created.
> @@ -83,6 +87,10 @@ quoting:
>   	delete SP <ref> NUL [<oldvalue>] NUL
>   	verify SP <ref> NUL [<oldvalue>] NUL
>   	option SP <opt> NUL
> +	start NUL
> +	prepare NUL
> +	commit NUL
> +	abort NUL
>   
>   In this format, use 40 "0" to specify a zero value, and use the empty
>   string to specify a missing value.
> @@ -114,6 +122,24 @@ option::
>   	The only valid option is `no-deref` to avoid dereferencing
>   	a symbolic ref.
>   
> +start::
> +	Start a transaction. In contrast to a non-transactional session,

I found the talk of "non-transactional" sessions a bit confusing because the normal --stdin does update all the refs it is given as a single transaction, so that if it cannot update one ref none of them are updated. If I've understood correctly these changes are about coordinating transactions across several repositories. I'm not sure how best to convey that in the man page - perhaps we could call them single repository transactions and multi repository transaction or something.

Best Wishes
Phillip
Show 300 quoted lines
> a
> +	transaction will automatically abort if the session ends without an
> +	explicit commit.
> +
> +prepare::
> +	Prepare to commit the transaction. This will create lock files for all
> +	queued reference updates. If one reference could not be locked, the
> +	transaction will be aborted.
> +
> +commit::
> +	Commit all reference updates queued for the transaction, ending the
> +	transaction.
> +
> +abort::
> +	Abort the transaction, releasing all locks if the transaction is in
> +	prepared state.
> +
>   If all <ref>s can be locked with matching <oldvalue>s
>   simultaneously, all modifications are performed.  Otherwise, no
>   modifications are performed.  Note that while each individual
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 348407b896..b74dd9a69d 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -312,21 +312,80 @@ static void parse_cmd_option(struct ref_transaction *transaction,
>   		die("option unknown: %s", next);
>   }
>   
> +static void parse_cmd_start(struct ref_transaction *transaction,
> +			    const char *next, const char *end)
> +{
> +	if (*next != line_termination)
> +		die("start: extra input: %s", next);
> +	puts("start: ok");
> +}
> +
> +static void parse_cmd_prepare(struct ref_transaction *transaction,
> +			      const char *next, const char *end)
> +{
> +	struct strbuf error = STRBUF_INIT;
> +	if (*next != line_termination)
> +		die("prepare: extra input: %s", next);
> +	if (ref_transaction_prepare(transaction, &error))
> +		die("prepare: %s", error.buf);
> +	puts("prepare: ok");
> +}
> +
> +static void parse_cmd_abort(struct ref_transaction *transaction,
> +			    const char *next, const char *end)
> +{
> +	struct strbuf error = STRBUF_INIT;
> +	if (*next != line_termination)
> +		die("abort: extra input: %s", next);
> +	if (ref_transaction_abort(transaction, &error))
> +		die("abort: %s", error.buf);
> +	puts("abort: ok");
> +}
> +
> +static void parse_cmd_commit(struct ref_transaction *transaction,
> +			     const char *next, const char *end)
> +{
> +	struct strbuf error = STRBUF_INIT;
> +	if (*next != line_termination)
> +		die("commit: extra input: %s", next);
> +	if (ref_transaction_commit(transaction, &error))
> +		die("commit: %s", error.buf);
> +	puts("commit: ok");
> +	ref_transaction_free(transaction);
> +}
> +
> +enum update_refs_state {
> +	/* Non-transactional state open for updates. */
> +	UPDATE_REFS_OPEN,
> +	/* A transaction has been started. */
> +	UPDATE_REFS_STARTED,
> +	/* References are locked and ready for commit */
> +	UPDATE_REFS_PREPARED,
> +	/* Transaction has been committed or closed. */
> +	UPDATE_REFS_CLOSED,
> +};
> +
>   static const struct parse_cmd {
>   	const char *prefix;
>   	void (*fn)(struct ref_transaction *, const char *, const char *);
>   	unsigned args;
> +	enum update_refs_state state;
>   } command[] = {
> -	{ "update", parse_cmd_update, 3 },
> -	{ "create", parse_cmd_create, 2 },
> -	{ "delete", parse_cmd_delete, 2 },
> -	{ "verify", parse_cmd_verify, 2 },
> -	{ "option", parse_cmd_option, 1 },
> +	{ "update",  parse_cmd_update,  3, UPDATE_REFS_OPEN },
> +	{ "create",  parse_cmd_create,  2, UPDATE_REFS_OPEN },
> +	{ "delete",  parse_cmd_delete,  2, UPDATE_REFS_OPEN },
> +	{ "verify",  parse_cmd_verify,  2, UPDATE_REFS_OPEN },
> +	{ "option",  parse_cmd_option,  1, UPDATE_REFS_OPEN },
> +	{ "start",   parse_cmd_start,   0, UPDATE_REFS_STARTED },
> +	{ "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED },
> +	{ "abort",   parse_cmd_abort,   0, UPDATE_REFS_CLOSED },
> +	{ "commit",  parse_cmd_commit,  0, UPDATE_REFS_CLOSED },
>   };
>   
>   static void update_refs_stdin(void)
>   {
>   	struct strbuf input = STRBUF_INIT, err = STRBUF_INIT;
> +	enum update_refs_state state = UPDATE_REFS_OPEN;
>   	struct ref_transaction *transaction;
>   	int i, j;
>   
> @@ -374,14 +433,45 @@ static void update_refs_stdin(void)
>   			if (strbuf_appendwholeline(&input, stdin, line_termination))
>   				break;
>   
> +		switch (state) {
> +		case UPDATE_REFS_OPEN:
> +		case UPDATE_REFS_STARTED:
> +			/* Do not downgrade a transaction to a non-transaction. */
> +			if (cmd->state >= state)
> +				state = cmd->state;
> +			break;
> +		case UPDATE_REFS_PREPARED:
> +			if (cmd->state != UPDATE_REFS_CLOSED)
> +				die("prepared transactions can only be closed");
> +			state = cmd->state;
> +			break;
> +		case UPDATE_REFS_CLOSED:
> +			die("transaction is closed");
> +			break;
> +		}
> +
>   		cmd->fn(transaction, input.buf + strlen(cmd->prefix) + !!cmd->args,
>   			input.buf + input.len);
>   	}
>   
> -	if (ref_transaction_commit(transaction, &err))
> -		die("%s", err.buf);
> +	switch (state) {
> +	case UPDATE_REFS_OPEN:
> +		/* Commit by default if no transaction was requested. */
> +		if (ref_transaction_commit(transaction, &err))
> +			die("%s", err.buf);
> +		ref_transaction_free(transaction);
> +		break;
> +	case UPDATE_REFS_STARTED:
> +	case UPDATE_REFS_PREPARED:
> +		/* If using a transaction, we want to abort it. */
> +		if (ref_transaction_abort(transaction, &err))
> +			die("%s", err.buf);
> +		break;
> +	case UPDATE_REFS_CLOSED:
> +		/* Otherwise no need to do anything, the transaction was closed already. */
> +		break;
> +	}
>   
> -	ref_transaction_free(transaction);
>   	strbuf_release(&err);
>   	strbuf_release(&input);
>   }
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index a6224ef65f..48d0d42afd 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -1404,4 +1404,135 @@ test_expect_success 'handle per-worktree refs in refs/bisect' '
>   	! test_cmp main-head worktree-head
>   '
>   
> +test_expect_success 'transaction handles empty commit' '
> +	cat >stdin <<-EOF &&
> +	start
> +	prepare
> +	commit
> +	EOF
> +	git update-ref --stdin <stdin >actual &&
> +	printf "%s: ok\n" start prepare commit >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'transaction handles empty commit with missing prepare' '
> +	cat >stdin <<-EOF &&
> +	start
> +	commit
> +	EOF
> +	git update-ref --stdin <stdin >actual &&
> +	printf "%s: ok\n" start commit >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'transaction handles sole commit' '
> +	cat >stdin <<-EOF &&
> +	commit
> +	EOF
> +	git update-ref --stdin <stdin >actual &&
> +	printf "%s: ok\n" commit >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'transaction handles empty abort' '
> +	cat >stdin <<-EOF &&
> +	start
> +	prepare
> +	abort
> +	EOF
> +	git update-ref --stdin <stdin >actual &&
> +	printf "%s: ok\n" start prepare abort >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'transaction exits on multiple aborts' '
> +	cat >stdin <<-EOF &&
> +	abort
> +	abort
> +	EOF
> +	test_must_fail git update-ref --stdin <stdin >actual 2>err &&
> +	printf "%s: ok\n" abort >expect &&
> +	test_cmp expect actual &&
> +	grep "fatal: transaction is closed" err
> +'
> +
> +test_expect_success 'transaction exits on start after prepare' '
> +	cat >stdin <<-EOF &&
> +	prepare
> +	start
> +	EOF
> +	test_must_fail git update-ref --stdin <stdin 2>err >actual &&
> +	printf "%s: ok\n" prepare >expect &&
> +	test_cmp expect actual &&
> +	grep "fatal: prepared transactions can only be closed" err
> +'
> +
> +test_expect_success 'transaction handles empty abort with missing prepare' '
> +	cat >stdin <<-EOF &&
> +	start
> +	abort
> +	EOF
> +	git update-ref --stdin <stdin >actual &&
> +	printf "%s: ok\n" start abort >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'transaction handles sole abort' '
> +	cat >stdin <<-EOF &&
> +	abort
> +	EOF
> +	git update-ref --stdin <stdin >actual &&
> +	printf "%s: ok\n" abort >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'transaction can handle commit' '
> +	cat >stdin <<-EOF &&
> +	start
> +	create $a HEAD
> +	commit
> +	EOF
> +	git update-ref --stdin <stdin >actual &&
> +	printf "%s: ok\n" start commit >expect &&
> +	test_cmp expect actual &&
> +	git rev-parse HEAD >expect &&
> +	git rev-parse $a >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'transaction can handle abort' '
> +	cat >stdin <<-EOF &&
> +	start
> +	create $b HEAD
> +	abort
> +	EOF
> +	git update-ref --stdin <stdin >actual &&
> +	printf "%s: ok\n" start abort >expect &&
> +	test_cmp expect actual &&
> +	test_path_is_missing .git/$b
> +'
> +
> +test_expect_success 'transaction aborts by default' '
> +	cat >stdin <<-EOF &&
> +	start
> +	create $b HEAD
> +	EOF
> +	git update-ref --stdin <stdin >actual &&
> +	printf "%s: ok\n" start >expect &&
> +	test_cmp expect actual &&
> +	test_path_is_missing .git/$b
> +'
> +
> +test_expect_success 'transaction with prepare aborts by default' '
> +	cat >stdin <<-EOF &&
> +	start
> +	create $b HEAD
> +	prepare
> +	EOF
> +	git update-ref --stdin <stdin >actual &&
> +	printf "%s: ok\n" start prepare >expect &&
> +	test_cmp expect actual &&
> +	test_path_is_missing .git/$b
> +'
> +
>   test_done
>
Patrick Steinhardt· Apr 3, 2020, 16:51 UTC · re: Phillip Wood · lore

Re: [PATCH v3 9/9] update-ref: implement interactive transaction handling

On Fri, Apr 03, 2020 at 02:40:26PM +0100, Phillip Wood wrote:
Show 103 quoted lines
> Hi Patrick
> 
> On 02/04/2020 08:10, Patrick Steinhardt wrote:
> > The git-update-ref(1) command can only handle queueing transactions
> > right now via its "--stdin" parameter, but there is no way for users to
> > handle the transaction itself in a more explicit way. E.g. in a
> > replicated scenario, one may imagine a coordinator that spawns
> > git-update-ref(1) for multiple repositories and only if all agree that
> > an update is possible will the coordinator send a commit. Such a
> > transactional session could look like
> > 
> >      > start
> >      < start: ok
> >      > update refs/heads/master $OLD $NEW
> >      > prepare
> >      < prepare: ok
> >      # All nodes have returned "ok"
> >      > commit
> >      < commit: ok
> > 
> > or
> > 
> >      > start
> >      < start: ok
> >      > create refs/heads/master $OLD $NEW
> >      > prepare
> >      < fatal: cannot lock ref 'refs/heads/master': reference already exists
> >      # On all other nodes:
> >      > abort
> >      < abort: ok
> > 
> > In order to allow for such transactional sessions, this commit
> > introduces four new commands for git-update-ref(1), which matches those
> > we have internally already with the exception of "start":
> > 
> >      - start: start a new transaction
> > 
> >      - prepare: prepare the transaction, that is try to lock all
> >                 references and verify their current value matches the
> >                 expected one
> > 
> >      - commit: explicitly commit a session, that is update references to
> >                match their new expected state
> > 
> >      - abort: abort a session and roll back all changes
> > 
> > By design, git-update-ref(1) will commit as soon as standard input is
> > being closed. While fine in a non-transactional world, it is definitely
> > unexpected in a transactional world. Because of this, as soon as any of
> > the new transactional commands is used, the default will change to
> > aborting without an explicit "commit". To avoid a race between queueing
> > updates and the first "prepare" that starts a transaction, the "start"
> > command has been added to start an explicit transaction.
> > 
> > Add some tests to exercise this new functionality.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >   Documentation/git-update-ref.txt |  26 ++++++
> >   builtin/update-ref.c             | 106 +++++++++++++++++++++++--
> >   t/t1400-update-ref.sh            | 131 +++++++++++++++++++++++++++++++
> >   3 files changed, 255 insertions(+), 8 deletions(-)
> > 
> > diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
> > index 9bd039ce08..3e737c2360 100644
> > --- a/Documentation/git-update-ref.txt
> > +++ b/Documentation/git-update-ref.txt
> > @@ -66,6 +66,10 @@ performs all modifications together.  Specify commands of the form:
> >   	delete SP <ref> [SP <oldvalue>] LF
> >   	verify SP <ref> [SP <oldvalue>] LF
> >   	option SP <opt> LF
> > +	start LF
> > +	prepare LF
> > +	commit LF
> > +	abort LF
> >   
> >   With `--create-reflog`, update-ref will create a reflog for each ref
> >   even if one would not ordinarily be created.
> > @@ -83,6 +87,10 @@ quoting:
> >   	delete SP <ref> NUL [<oldvalue>] NUL
> >   	verify SP <ref> NUL [<oldvalue>] NUL
> >   	option SP <opt> NUL
> > +	start NUL
> > +	prepare NUL
> > +	commit NUL
> > +	abort NUL
> >   
> >   In this format, use 40 "0" to specify a zero value, and use the empty
> >   string to specify a missing value.
> > @@ -114,6 +122,24 @@ option::
> >   	The only valid option is `no-deref` to avoid dereferencing
> >   	a symbolic ref.
> >   
> > +start::
> > +	Start a transaction. In contrast to a non-transactional session, 
> 
> I found the talk of "non-transactional" sessions a bit confusing because 
> the normal --stdin does update all the refs it is given as a single 
> transaction, so that if it cannot update one ref none of them are 
> updated. If I've understood correctly these changes are about 
> coordinating transactions across several repositories. I'm not sure how 
> best to convey that in the man page - perhaps we could call them single 
> repository transactions and multi repository transaction or something.

The ultimate goal is to be able to create something that sits atop a set of repos that's able to coordinate multiple reference transactions at the same time and then do an all or nothing commit across all repos or abort in case any of the repos will not be able to perform the update. The proposed change is still about a single repository, only, and have the aim of actually enabling transaction semantics in the first place. Until now, it's only possible to start a transaction via git-update-refs(1), but you can't really control it except for the fnial commit.

So with that being said I'm not quite sure whether it makes sense to document the potential for handling transactions across multiple repos. It does show that I could improve the documentation, though, and make the intent and scope clearer. I'll try to do that for the next version, thanks for your feedback!

Patrick
Junio C Hamano· Apr 3, 2020, 17:33 UTC · re: Phillip Wood · lore

Re: [PATCH v3 9/9] update-ref: implement interactive transaction handling

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 8 quoted lines
> I found the talk of "non-transactional" sessions a bit confusing
> because the normal --stdin does update all the refs it is given as a
> single transaction, so that if it cannot update one ref none of them
> are updated. If I've understood correctly these changes are about
> coordinating transactions across several repositories. I'm not sure
> how best to convey that in the man page - perhaps we could call them
> single repository transactions and multi repository transaction or
> something.

FWIW, I described the topic in my "What's cooking" report as adding an ingredient to implement two-phase commit-style atomic updates across multiple repositories. These "prepare to commit, now if you say 'yes I am ready', there is no taking it back", "now do the commit as you promised", and "Earlier I made you promise that you'd commit all when I tell you, but you no longer should commit" are good enough ingredients, but I am not so sure about the actual implementation in this topic (or it is really possible to make the promise "ok, now I can commit everything without lock conflicts if you tell me" with our ref API, which does not have "now freeze all refs in the repository---from now on until I say otherwise, no refs can be manipulated", in the first place).

Junio C Hamano· Apr 3, 2020, 17:35 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 9/9] update-ref: implement interactive transaction handling

Junio C Hamano <gitster@pobox.com> writes:
> ... but I am not so sure about the actual
> implementation in this topic ...

I take this part back---given that this is merely building on top of the existing atomic update, we should be OK.

Patrick Steinhardt· Apr 6, 2020, 07:10 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 9/9] update-ref: implement interactive transaction handling

On Fri, Apr 03, 2020 at 10:33:58AM -0700, Junio C Hamano wrote:
Show 14 quoted lines
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
> > I found the talk of "non-transactional" sessions a bit confusing
> > because the normal --stdin does update all the refs it is given as a
> > single transaction, so that if it cannot update one ref none of them
> > are updated. If I've understood correctly these changes are about
> > coordinating transactions across several repositories. I'm not sure
> > how best to convey that in the man page - perhaps we could call them
> > single repository transactions and multi repository transaction or
> > something.
> 
> FWIW, I described the topic in my "What's cooking" report as adding
> an ingredient to implement two-phase commit-style atomic updates
> across multiple repositories.

Fair enough. It definitely may serve as a building block for cross-repo transactions, which also is the original intent of this patch series. I'm not sure whether it makes sense to document it as such, as I can imagine usecases acting on a single repository that still want to be in the business of doing two-phase commits.

Patrick

← back to recent threads