Re: [PATCH v2 11/11] ls-refs: reject unknown arguments

From: Jeff King <peff@peff.net>
Date: Sep 15, 2021, 16:25 UTC
Message-ID: <YUIedCI/KR3zrTA8@coredump.intra.peff.net>
In-Reply-To: <875yv2ofkq.fsf@evledraar.gmail.com>

On Wed, Sep 15, 2021 at 02:09:16AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 7 quoted lines

> So the below patch isn't anywhere near applying, but you can see the
> test coverage (those tests are new) & what changes if we instrument a
> client to actually send this bad data.
> 
> That packet_client_error() function is new, part of what I'm doing there
> is converting all of this error emitting from die() to properly sending
> ERR packets.

Right, I noticed that none of this is likely to make it to a client (unless they are using file:// or an ssh channel which passes back stderr).

I agree that we should probably be passing these back via ERR packets, but note that the client is racy here. If the server reports an error and dies while the client is still writing, they'll see SIGPIPE and exit without showing the user the ERR packet. The solution may be something along these lines:

  https://public-inbox.org/git/20200422163357.27056-1-chriscool@tuxfamily.org/

Alternatively, the server can pump the client data to /dev/null until we hit a flush, and _then_ write the ERR. But that doesn't work for some protocol-level errors (like "oops, I'm having trouble reading your pkt-lines).

> I think a bug in your versio is that you're using _() here, if your
> server program happens to be started in Chinese you probably still want
> to emit errors to clients in LANG=C.

I'm not sure it's a bug. If you set LANG=zh on your server, that might be harmful (if you serve a multi-lingual international audience), or it might be helpful (if it's a company server where everybody speaks the language). Likewise, for a file:// or ssh:// operation, your local LANG would kick in.

I don't really have a strong opinion either way on whether it's helpful or hindering overall. But I did follow the convention of nearby code in this series, so I think we don't need to worry about it now (it also seemed inconsistent to me; serve.c does not mark for translation, but ls-refs.c does).

> Of course that actually working is subject to various races that I may
> or may not be able to untangle...

Oh good, so you know about them. :)

-Peff