| Message ID | 50ffc263293571f8af71fd1d253ac238c6909229.1585129842.git.ps@pks.im (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support for transactions in `git-update-ref --stdin` | expand |
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.
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
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.
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
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++; }
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(-)