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

Commit Message

Patrick Steinhardt March 25, 2020, 9:54 a.m. UTC

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 <[e-mail]>
---
 builtin/update-ref.c | 70 ++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 39 deletions(-)

Comments

Junio C Hamano March 27, 2020, 9:58 p.m. UTC | #1

Patrick Steinhardt <[e-mail]> writes:

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

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

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

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

The name of the field should make such details clear.

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

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

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

Thanks.

Patrick Steinhardt March 30, 2020, 8:11 a.m. UTC | #2

On Fri, Mar 27, 2020 at 02:58:38PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <[e-mail]> 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

Junio C Hamano March 30, 2020, 5:39 p.m. UTC | #3

Patrick Steinhardt <[e-mail]> writes:

> On Fri, Mar 27, 2020 at 02:58:38PM -0700, Junio C Hamano wrote:
>> Patrick Steinhardt <[e-mail]> 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

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))