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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 27, 2020, 21:58 UTC
Message-ID: <xmqqo8she9yp.fsf@gitster.c.googlers.com>
In-Reply-To: <1db63f97ec78fad794cec51c5d96b093f7cd2440.1585129843.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 5 quoted lines

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

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

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

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

The name of the field should make such details clear.

Show 16 quoted lines

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

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

Show 5 quoted lines

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

Thanks.