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

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2910 bytes --]

Hi,

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

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

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

The series is structured as follows:

    Patches 1-2: Preparatory patches which make sense as standalone
                 patches.

    Patches 3-7: Preparatory patches that make it easier to convert to
                 reading commands in a line-wise fashion. No functional
                 changes are expected.

    Patch 8: Conversion to read commands line-wise. No functional
             changes are expected, except that Git builds up the transaction
             while reading stdin instead of waiting for stdin to be closed
             first.

    Patch 9: Implementation of transactional commands.

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

Patrick

Patrick Steinhardt (9):
  refs: fix segfault when aborting empty transaction
  git-update-ref.txt: add missing word
  strbuf: provide function to append whole lines
  update-ref: organize commands in an array
  update-ref: drop unused argument for `parse_refname`
  update-ref: pass end pointer instead of strbuf
  update-ref: move transaction handling into `update_refs_stdin()`
  update-ref: read commands in a line-wise fashion
  update-ref: implement interactive transaction handling

 Documentation/git-update-ref.txt |  28 +++-
 builtin/update-ref.c             | 255 ++++++++++++++++++++++---------
 refs/files-backend.c             |  20 +--
 strbuf.c                         |  10 ++
 strbuf.h                         |   6 +
 t/t1400-update-ref.sh            | 131 ++++++++++++++++
 6 files changed, 370 insertions(+), 80 deletions(-)

-- 
2.26.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

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

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 561c33ac8a..6516c7bc8c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2565,17 +2565,19 @@ static void files_transaction_cleanup(struct files_ref_store *refs,
 		}
 	}
 
-	if (backend_data->packed_transaction &&
-	    ref_transaction_abort(backend_data->packed_transaction, &err)) {
-		error("error aborting transaction: %s", err.buf);
-		strbuf_release(&err);
+	if (backend_data) {
+		if (backend_data->packed_transaction &&
+		    ref_transaction_abort(backend_data->packed_transaction, &err)) {
+			error("error aborting transaction: %s", err.buf);
+			strbuf_release(&err);
+		}
+
+		if (backend_data->packed_refs_locked)
+			packed_refs_unlock(refs->packed_ref_store);
+
+		free(backend_data);
 	}
 
-	if (backend_data->packed_refs_locked)
-		packed_refs_unlock(refs->packed_ref_store);
-
-	free(backend_data);
-
 	transaction->state = REF_TRANSACTION_CLOSED;
 }
 
-- 
2.26.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> When cleaning up a transaction that has no updates queued, then the
> transaction's backend data will not have been allocated. We correctly
> handle this for the packed backend, where the cleanup function checks
> whether the backend data has been allocated at all -- if not, then there
> is nothing to clean up. For the files backend we do not check this and
> as a result will hit a segfault due to dereferencing a `NULL` pointer
> when cleaning up such a transaction.
>
> Fix the issue by checking whether `backend_data` is set in the files
> backend, too.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs/files-backend.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)

Makes sense.  Will queue.  Thanks.

>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 561c33ac8a..6516c7bc8c 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2565,17 +2565,19 @@ static void files_transaction_cleanup(struct files_ref_store *refs,
>  		}
>  	}
>  
> -	if (backend_data->packed_transaction &&
> -	    ref_transaction_abort(backend_data->packed_transaction, &err)) {
> -		error("error aborting transaction: %s", err.buf);
> -		strbuf_release(&err);
> +	if (backend_data) {
> +		if (backend_data->packed_transaction &&
> +		    ref_transaction_abort(backend_data->packed_transaction, &err)) {
> +			error("error aborting transaction: %s", err.buf);
> +			strbuf_release(&err);
> +		}
> +
> +		if (backend_data->packed_refs_locked)
> +			packed_refs_unlock(refs->packed_ref_store);
> +
> +		free(backend_data);
>  	}
>  
> -	if (backend_data->packed_refs_locked)
> -		packed_refs_unlock(refs->packed_ref_store);
> -
> -	free(backend_data);
> -
>  	transaction->state = REF_TRANSACTION_CLOSED;
>  }

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 749 bytes --]

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git-update-ref.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 9671423117..9bd039ce08 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -107,7 +107,7 @@ delete::
 
 verify::
 	Verify <ref> against <oldvalue> but do not change it.  If
-	<oldvalue> zero or missing, the ref must not exist.
+	<oldvalue> is zero or missing, the ref must not exist.
 
 option::
 	Modify behavior of the next command naming a <ref>.
-- 
2.26.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]

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

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

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

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

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

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

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

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

Having said all that, I am fine with this version, at least for now.

> +int strbuf_appendwholeline(struct strbuf *sb, FILE *fp, int term)
> +{
> +	struct strbuf line = STRBUF_INIT;
> +	if (strbuf_getwholeline(&line, fp, term))
> +		return EOF;

So, if caller wants to read the stream until it runs out, it can do

	strbuf_init(&sb);
	while (strbuf_appendwholeline(&sb, fp, '\n') != EOF)
		; /* keep going */

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

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

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

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

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

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

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1746 bytes --]

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

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

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

Ahh, I see.  Thanks for a clarification.

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5687 bytes --]

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

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 66 ++++++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 17 deletions(-)

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> +static const struct parse_cmd {
> +	const char *prefix;
> +	const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);
> +} commands[] = {

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

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

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

The only reason why you had to sprinkle

	if (!skip_prefix(next, " ", &next))
		die(_("%s: missing space after command"), cmd);

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

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

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

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

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

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

> +
> +		next = cmd->fn(transaction, &input, next);
>  		next++;
>  	}

Thanks.

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4976 bytes --]

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

> So why not avoid touching the parse_cmd_<cmd>() at all (except for
> the "option" thing that now needs to take the transaction object for
> uniformity), and then verify the presence of " " here, perhaps like
> this:
> 
> 	for (i = 0; i < ARRAY_SIZE(command); i++) {
> 		const char *eoc;
> 		if (!skip_prefix(next, commands[i].prefix, &eoc) ||
> 		    *eoc != ' ')
> 			continue;
> 		cmd = &command[i];
>                 next = eoc;
> 		break;
> 	}

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

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

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

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

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

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.

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1197 bytes --]

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

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

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2180 bytes --]

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index d2b917a47c..6f4e8e88c1 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -50,7 +50,7 @@ static const char *parse_arg(const char *next, struct strbuf *arg)
  * the argument.  Die if C-quoting is malformed or the reference name
  * is invalid.
  */
-static char *parse_refname(struct strbuf *input, const char **next)
+static char *parse_refname(const char **next)
 {
 	struct strbuf ref = STRBUF_INIT;
 
@@ -189,7 +189,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 	if (!skip_prefix(next, " ", &next))
 		die("update: missing space after command");
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("update: missing <ref>");
 
@@ -226,7 +226,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 	if (!skip_prefix(next, " ", &next))
 		die("create: missing space after command");
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("create: missing <ref>");
 
@@ -262,7 +262,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 	if (!skip_prefix(next, " ", &next))
 		die("delete: missing space after command");
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("delete: missing <ref>");
 
@@ -300,7 +300,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 	if (!skip_prefix(next, " ", &next))
 		die("verify: missing space after command");
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("verify: missing <ref>");
 
-- 
2.26.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5642 bytes --]

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2473 bytes --]

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

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

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.

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 6626 bytes --]

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

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

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

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

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 70 ++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 39 deletions(-)

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>  static const struct parse_cmd {
>  	const char *prefix;
> -	const char *(*fn)(struct ref_transaction *, const char *, const char *);
> +	void (*fn)(struct ref_transaction *, const char *, const char *);
> +	unsigned extra_lines;

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

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

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

The name of the field should make such details clear.

>  } commands[] = {
> -	{ "update", parse_cmd_update },
> -	{ "create", parse_cmd_create },
> -	{ "delete", parse_cmd_delete },
> -	{ "verify", parse_cmd_verify },
> -	{ "option", parse_cmd_option },
> +	{ "update", parse_cmd_update, 2 },
> +	{ "create", parse_cmd_create, 1 },
> +	{ "delete", parse_cmd_delete, 1 },
> +	{ "verify", parse_cmd_verify, 1 },
> +	{ "option", parse_cmd_option, 0 },
>  };
> +		/* Read extra lines if NUL-terminated */
> +		for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++)
> +			if (strbuf_appendwholeline(&input, stdin, line_termination))
> +				break;

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

> +		cmd->fn(transaction, input.buf + strlen(cmd->prefix),
> +			input.buf + input.len);
>  	}
>  
>  	if (ref_transaction_commit(transaction, &err))

Thanks.

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2235 bytes --]

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

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

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

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

I'll make sure to move over the comment.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

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

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

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 12314 bytes --]

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

    > start
    < start: ok
    > update refs/heads/master $OLD $NEW
    > prepare
    < prepare: ok
    # All nodes have returned "ok"
    > commit
    < commit: ok

or

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

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

    - start: start a new transaction

    - prepare: prepare the transaction, that is try to lock all
               references and verify their current value matches the
               expected one

    - commit: explicitly commit a session, that is update references to
              match their new expected state

    - abort: abort a session and roll back all changes

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

Add some tests to exercise this new functionality.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git-update-ref.txt |  26 ++++++
 builtin/update-ref.c             | 108 ++++++++++++++++++++++---
 t/t1400-update-ref.sh            | 131 +++++++++++++++++++++++++++++++
 3 files changed, 256 insertions(+), 9 deletions(-)

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

OK, this is a fun stuff, and explains why we wanted to do [7/9].

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 8630 bytes --]

Hi,

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

The second version addresses the feedback of Junio:

    - Renamed the `commands` array to `command`.

    - Adds documentation to the `extra_lines` field to hopefully make
      its intent clearer.

    - Clarify why we ignore EOF when reading `extra_lines`.

    - When reading commands, the code now verifies that a line is not
      only a prefix to the command, but in fact the whole command. This
      allows implementation of multiple commands that have the same
      prefix, e.g. "option" and "options".

Thanks Junio for your review!

Patrick

Patrick Steinhardt (9):
  refs: fix segfault when aborting empty transaction
  git-update-ref.txt: add missing word
  strbuf: provide function to append whole lines
  update-ref: organize commands in an array
  update-ref: drop unused argument for `parse_refname`
  update-ref: pass end pointer instead of strbuf
  update-ref: move transaction handling into `update_refs_stdin()`
  update-ref: read commands in a line-wise fashion
  update-ref: implement interactive transaction handling

 Documentation/git-update-ref.txt |  28 +++-
 builtin/update-ref.c             | 274 +++++++++++++++++++++++--------
 refs/files-backend.c             |  20 ++-
 strbuf.c                         |  10 ++
 strbuf.h                         |   6 +
 t/t1400-update-ref.sh            | 131 +++++++++++++++
 6 files changed, 388 insertions(+), 81 deletions(-)

Range-diff against v1:
 1:  3c7f8c47f3 =  1:  7a297db4da refs: fix segfault when aborting empty transaction
 2:  dd7798abb7 =  2:  15857e1b8c git-update-ref.txt: add missing word
 3:  4b0a963ea5 =  3:  b6546ae44e strbuf: provide function to append whole lines
 4:  50ffc26329 !  4:  bd8c059fbc update-ref: organize commands in an array
    @@ builtin/update-ref.c: static const char *parse_cmd_option(struct strbuf *input,
     +static const struct parse_cmd {
     +	const char *prefix;
     +	const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);
    -+} commands[] = {
    ++} command[] = {
     +	{ "update", parse_cmd_update },
     +	{ "create", parse_cmd_create },
     +	{ "delete", parse_cmd_delete },
    @@ builtin/update-ref.c: static const char *parse_cmd_option(struct strbuf *input,
     -			next = parse_cmd_option(&input, next);
     -		else
     +
    -+		for (i = 0; i < ARRAY_SIZE(commands); i++) {
    -+			if (!skip_prefix(next, commands[i].prefix , &next))
    ++		for (i = 0; i < ARRAY_SIZE(command); i++) {
    ++			const char *prefix = command[i].prefix;
    ++
    ++			if (!skip_prefix(next, prefix, &next))
     +				continue;
    -+			cmd = &commands[i];
    ++
    ++			/*
    ++			 * Check that the command is terminated by an argument
    ++			 * or line terminator and not only a matching prefix.
    ++			 */
    ++			if (input.buf[strlen(prefix)] != line_termination &&
    ++			    input.buf[strlen(prefix)] != '\0' &&
    ++			    input.buf[strlen(prefix)] != ' ')
    ++				continue;
    ++
    ++			cmd = &command[i];
     +			break;
     +		}
     +		if (!cmd)
      			die("unknown command: %s", next);
      
    -+		if (input.buf[strlen(cmd->prefix)] != line_termination &&
    -+		    input.buf[strlen(cmd->prefix)] != '\0' &&
    -+		    input.buf[strlen(cmd->prefix)] != ' ')
    -+			die("%s: no separator after command", cmd->prefix);
    -+
     +		next = cmd->fn(transaction, &input, next);
      		next++;
      	}
 5:  a78043b188 =  5:  49a14d2046 update-ref: drop unused argument for `parse_refname`
 6:  51ebb2f849 !  6:  cbe430029d update-ref: pass end pointer instead of strbuf
    @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction
      	const char *prefix;
     -	const char *(*fn)(struct ref_transaction *, struct strbuf *, const char *);
     +	const char *(*fn)(struct ref_transaction *, const char *, const char *);
    - } commands[] = {
    + } command[] = {
      	{ "update", parse_cmd_update },
      	{ "create", parse_cmd_create },
     @@ builtin/update-ref.c: static void update_refs_stdin(struct ref_transaction *transaction)
    - 		    input.buf[strlen(cmd->prefix)] != ' ')
    - 			die("%s: no separator after command", cmd->prefix);
    + 		if (!cmd)
    + 			die("unknown command: %s", next);
      
     -		next = cmd->fn(transaction, &input, next);
     +		next = cmd->fn(transaction, next, input.buf + input.len);
 7:  bd92a649d7 =  7:  d2f68f59a7 update-ref: move transaction handling into `update_refs_stdin()`
 8:  1db63f97ec !  8:  f8786fdeb3 update-ref: read commands in a line-wise fashion
    @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction
      	const char *prefix;
     -	const char *(*fn)(struct ref_transaction *, const char *, const char *);
     +	void (*fn)(struct ref_transaction *, const char *, const char *);
    ++	/*
    ++	 * If using NUL-terminated format, only the first argument will be
    ++	 * available in the first line. In case a command expects more than one
    ++	 * argument, we thus have to fetch an additional `extra_lines` number
    ++	 * of lines.
    ++	 */
     +	unsigned extra_lines;
    - } commands[] = {
    + } command[] = {
     -	{ "update", parse_cmd_update },
     -	{ "create", parse_cmd_create },
     -	{ "delete", parse_cmd_delete },
    @@ builtin/update-ref.c: static const char *parse_cmd_option(struct ref_transaction
     +		else if (isspace(*input.buf))
     +			die("whitespace before command: %s", input.buf);
      
    - 		for (i = 0; i < ARRAY_SIZE(commands); i++) {
    --			if (!skip_prefix(next, commands[i].prefix , &next))
    -+			if (!starts_with(input.buf, commands[i].prefix))
    + 		for (i = 0; i < ARRAY_SIZE(command); i++) {
    + 			const char *prefix = command[i].prefix;
    + 
    +-			if (!skip_prefix(next, prefix, &next))
    ++			if (!starts_with(input.buf, prefix))
      				continue;
    - 			cmd = &commands[i];
    + 
    + 			/*
    +@@ builtin/update-ref.c: static void update_refs_stdin(void)
      			break;
      		}
      		if (!cmd)
     -			die("unknown command: %s", next);
     +			die("unknown command: %s", input.buf);
      
    - 		if (input.buf[strlen(cmd->prefix)] != line_termination &&
    - 		    input.buf[strlen(cmd->prefix)] != '\0' &&
    - 		    input.buf[strlen(cmd->prefix)] != ' ')
    - 			die("%s: no separator after command", cmd->prefix);
    - 
     -		next = cmd->fn(transaction, next, input.buf + input.len);
     -		next++;
    -+		/* Read extra lines if NUL-terminated */
    ++		/*
    ++		 * Read extra lines if NUL-terminated. Do not raise an error in
    ++		 * case there is an early EOF to let the command handle missing
    ++		 * arguments with a proper error message.
    ++		 */
     +		for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++)
     +			if (strbuf_appendwholeline(&input, stdin, line_termination))
     +				break;
 9:  88c0089bb5 !  9:  c3fffdf9fa update-ref: implement interactive transaction handling
    @@ builtin/update-ref.c: static void parse_cmd_option(struct ref_transaction *trans
      static const struct parse_cmd {
      	const char *prefix;
      	void (*fn)(struct ref_transaction *, const char *, const char *);
    +@@ builtin/update-ref.c: static const struct parse_cmd {
    + 	 * of lines.
    + 	 */
      	unsigned extra_lines;
     +	enum update_refs_state state;
    - } commands[] = {
    + } command[] = {
     -	{ "update", parse_cmd_update, 2 },
     -	{ "create", parse_cmd_create, 1 },
     -	{ "delete", parse_cmd_delete, 1 },
    @@ builtin/update-ref.c: static void parse_cmd_option(struct ref_transaction *trans
      	int i, j;
      
     @@ builtin/update-ref.c: static void update_refs_stdin(void)
    - 		    input.buf[strlen(cmd->prefix)] != ' ')
    - 			die("%s: no separator after command", cmd->prefix);
    - 
    --		/* Read extra lines if NUL-terminated */
    -+		/* Read extra lines if NUL-terminated, but let commands handle missing ones. */
    - 		for (j = 0; line_termination == '\0' && j < cmd->extra_lines; j++)
      			if (strbuf_appendwholeline(&input, stdin, line_termination))
      				break;
      
-- 
2.26.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

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

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 561c33ac8a..6516c7bc8c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2565,17 +2565,19 @@ static void files_transaction_cleanup(struct files_ref_store *refs,
 		}
 	}
 
-	if (backend_data->packed_transaction &&
-	    ref_transaction_abort(backend_data->packed_transaction, &err)) {
-		error("error aborting transaction: %s", err.buf);
-		strbuf_release(&err);
+	if (backend_data) {
+		if (backend_data->packed_transaction &&
+		    ref_transaction_abort(backend_data->packed_transaction, &err)) {
+			error("error aborting transaction: %s", err.buf);
+			strbuf_release(&err);
+		}
+
+		if (backend_data->packed_refs_locked)
+			packed_refs_unlock(refs->packed_ref_store);
+
+		free(backend_data);
 	}
 
-	if (backend_data->packed_refs_locked)
-		packed_refs_unlock(refs->packed_ref_store);
-
-	free(backend_data);
-
 	transaction->state = REF_TRANSACTION_CLOSED;
 }
 
-- 
2.26.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 749 bytes --]

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git-update-ref.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 9671423117..9bd039ce08 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -107,7 +107,7 @@ delete::
 
 verify::
 	Verify <ref> against <oldvalue> but do not change it.  If
-	<oldvalue> zero or missing, the ref must not exist.
+	<oldvalue> is zero or missing, the ref must not exist.
 
 option::
 	Modify behavior of the next command naming a <ref>.
-- 
2.26.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]

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

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

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

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5805 bytes --]

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

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 73 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 56 insertions(+), 17 deletions(-)

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2180 bytes --]

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 559475681e..6f2aba916b 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -50,7 +50,7 @@ static const char *parse_arg(const char *next, struct strbuf *arg)
  * the argument.  Die if C-quoting is malformed or the reference name
  * is invalid.
  */
-static char *parse_refname(struct strbuf *input, const char **next)
+static char *parse_refname(const char **next)
 {
 	struct strbuf ref = STRBUF_INIT;
 
@@ -189,7 +189,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 	if (!skip_prefix(next, " ", &next))
 		die("update: missing space after command");
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("update: missing <ref>");
 
@@ -226,7 +226,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 	if (!skip_prefix(next, " ", &next))
 		die("create: missing space after command");
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("create: missing <ref>");
 
@@ -262,7 +262,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 	if (!skip_prefix(next, " ", &next))
 		die("delete: missing space after command");
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("delete: missing <ref>");
 
@@ -300,7 +300,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 	if (!skip_prefix(next, " ", &next))
 		die("verify: missing space after command");
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("verify: missing <ref>");
 
-- 
2.26.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5590 bytes --]

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2473 bytes --]

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

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 6872 bytes --]

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

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

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

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

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 80 +++++++++++++++++++++++---------------------
 1 file changed, 41 insertions(+), 39 deletions(-)

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 12083 bytes --]

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

    > start
    < start: ok
    > update refs/heads/master $OLD $NEW
    > prepare
    < prepare: ok
    # All nodes have returned "ok"
    > commit
    < commit: ok

or

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

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

    - start: start a new transaction

    - prepare: prepare the transaction, that is try to lock all
               references and verify their current value matches the
               expected one

    - commit: explicitly commit a session, that is update references to
              match their new expected state

    - abort: abort a session and roll back all changes

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

Add some tests to exercise this new functionality.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git-update-ref.txt |  26 ++++++
 builtin/update-ref.c             | 106 +++++++++++++++++++++++--
 t/t1400-update-ref.sh            | 131 +++++++++++++++++++++++++++++++
 3 files changed, 255 insertions(+), 8 deletions(-)

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 15162 bytes --]

Hi,

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

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

The range-diff against v2 is attached below.

Patrick

Patrick Steinhardt (9):
  refs: fix segfault when aborting empty transaction
  git-update-ref.txt: add missing word
  strbuf: provide function to append whole lines
  update-ref: organize commands in an array
  update-ref: drop unused argument for `parse_refname`
  update-ref: pass end pointer instead of strbuf
  update-ref: move transaction handling into `update_refs_stdin()`
  update-ref: read commands in a line-wise fashion
  update-ref: implement interactive transaction handling

 Documentation/git-update-ref.txt |  28 +++-
 builtin/update-ref.c             | 245 ++++++++++++++++++++++---------
 refs/files-backend.c             |  20 +--
 strbuf.c                         |  10 ++
 strbuf.h                         |   6 +
 t/t1400-update-ref.sh            | 131 +++++++++++++++++
 6 files changed, 364 insertions(+), 76 deletions(-)

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

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

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 561c33ac8a..6516c7bc8c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2565,17 +2565,19 @@ static void files_transaction_cleanup(struct files_ref_store *refs,
 		}
 	}
 
-	if (backend_data->packed_transaction &&
-	    ref_transaction_abort(backend_data->packed_transaction, &err)) {
-		error("error aborting transaction: %s", err.buf);
-		strbuf_release(&err);
+	if (backend_data) {
+		if (backend_data->packed_transaction &&
+		    ref_transaction_abort(backend_data->packed_transaction, &err)) {
+			error("error aborting transaction: %s", err.buf);
+			strbuf_release(&err);
+		}
+
+		if (backend_data->packed_refs_locked)
+			packed_refs_unlock(refs->packed_ref_store);
+
+		free(backend_data);
 	}
 
-	if (backend_data->packed_refs_locked)
-		packed_refs_unlock(refs->packed_ref_store);
-
-	free(backend_data);
-
 	transaction->state = REF_TRANSACTION_CLOSED;
 }
 
-- 
2.26.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 749 bytes --]

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git-update-ref.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt
index 9671423117..9bd039ce08 100644
--- a/Documentation/git-update-ref.txt
+++ b/Documentation/git-update-ref.txt
@@ -107,7 +107,7 @@ delete::
 
 verify::
 	Verify <ref> against <oldvalue> but do not change it.  If
-	<oldvalue> zero or missing, the ref must not exist.
+	<oldvalue> is zero or missing, the ref must not exist.
 
 option::
 	Modify behavior of the next command naming a <ref>.
-- 
2.26.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]

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

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

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

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3173 bytes --]

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

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2027 bytes --]

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index a6ff88b95b..1bba5ea6c2 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -50,7 +50,7 @@ static const char *parse_arg(const char *next, struct strbuf *arg)
  * the argument.  Die if C-quoting is malformed or the reference name
  * is invalid.
  */
-static char *parse_refname(struct strbuf *input, const char **next)
+static char *parse_refname(const char **next)
 {
 	struct strbuf ref = STRBUF_INIT;
 
@@ -186,7 +186,7 @@ static const char *parse_cmd_update(struct ref_transaction *transaction,
 	struct object_id new_oid, old_oid;
 	int have_old;
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("update: missing <ref>");
 
@@ -220,7 +220,7 @@ static const char *parse_cmd_create(struct ref_transaction *transaction,
 	char *refname;
 	struct object_id new_oid;
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("create: missing <ref>");
 
@@ -253,7 +253,7 @@ static const char *parse_cmd_delete(struct ref_transaction *transaction,
 	struct object_id old_oid;
 	int have_old;
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("delete: missing <ref>");
 
@@ -288,7 +288,7 @@ static const char *parse_cmd_verify(struct ref_transaction *transaction,
 	char *refname;
 	struct object_id old_oid;
 
-	refname = parse_refname(input, &next);
+	refname = parse_refname(&next);
 	if (!refname)
 		die("verify: missing <ref>");
 
-- 
2.26.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5625 bytes --]

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2473 bytes --]

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

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 6877 bytes --]

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

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

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

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

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

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/update-ref.c | 85 +++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 40 deletions(-)

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 12013 bytes --]

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

    > start
    < start: ok
    > update refs/heads/master $OLD $NEW
    > prepare
    < prepare: ok
    # All nodes have returned "ok"
    > commit
    < commit: ok

or

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

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

    - start: start a new transaction

    - prepare: prepare the transaction, that is try to lock all
               references and verify their current value matches the
               expected one

    - commit: explicitly commit a session, that is update references to
              match their new expected state

    - abort: abort a session and roll back all changes

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

Add some tests to exercise this new functionality.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git-update-ref.txt |  26 ++++++
 builtin/update-ref.c             | 106 +++++++++++++++++++++++--
 t/t1400-update-ref.sh            | 131 +++++++++++++++++++++++++++++++
 3 files changed, 255 insertions(+), 8 deletions(-)

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Phillip Wood]

Hi Patrick

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

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

Best Wishes

Phillip

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

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5285 bytes --]

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

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

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

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

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

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

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

[Junio C Hamano]

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.

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1164 bytes --]

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

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

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]