On Fri, Mar 27, 2020 at 02:58:38PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@xxxxxx> 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:
signature.asc
Description: PGP signature