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

From: Patrick Steinhardt <ps@pks.im>
To: Junio C Hamano <gitster@pobox.com>
Cc: git <git@vger.kernel.org>, Christian Couder <christian.couder@gmail.com>
Subject: Re: [PATCH 8/9] update-ref: read commands in a line-wise fashion
Date: Mon, 30 Mar 2020 10:11:44 +0200	[thread overview]
Message-ID: <20200330081144.GB186019@ncase.pks.im> (raw)
In-Reply-To: <xmqqo8she9yp.fsf@gitster.c.googlers.com>

[-- 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 --]