From: Brandon Williams <bmwill@google.com>
Date: Tue, 13 Mar 2018 21:35:42 GMT
Subject: Re: [PATCH v4 12/35] serve: introduce git-serve
Message-ID: <20180313213542.GE7638@google.com>
In-Reply-To: <xmqqzi3qb3if.fsf@gitster-ct.c.googlers.com>

On 03/02, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> > +static int is_command(const char *key, struct protocol_capability **command)
> > +{
> > +	const char *out;
> > +
> > +	if (skip_prefix(key, "command=", &out)) {
> > +		struct protocol_capability *cmd = get_capability(out);
> > +
> > +		if (!cmd || !cmd->advertise(the_repository, NULL) || !cmd->command)
> > +			die("invalid command '%s'", out);
> > +		if (*command)
> > +			die("command already requested");
> 
> Shouldn't these two checks that lead to die the other way around?
> When they give us "command=frotz" and we already have *command, it
> would be an error whether we understand 'frotz' or not.
> 
> Who are the target audience of these "die"?  Are they meant to be
> communicated back to the other side of the connection, or are they
> only to be sent to the "server log"?
> 
> The latter one may want to say what two conflicting commands are in
> the log message, perhaps?

Yeah I'll switch the order of these checks as well as print out the two
commands requested for better logging.

> 
> > +		*command = cmd;
> 

-- 
Brandon Williams