Re: [PATCH v3 1/4] pack-protocol.txt: accept error packets in any context

[Jeff King <peff@peff.net>]

On Thu, Dec 13, 2018 at 02:18:26PM -0800, Josh Steadmon wrote:

> On 2018.12.12 17:17, Masaya Suzuki wrote:
> > On Wed, Dec 12, 2018 at 3:02 AM Jeff King <peff@peff.net> wrote:
> > > This ERR handling has been moved to a very low level. What happens if
> > > we're passing arbitrary data via the packet_read() code? Could we
> > > erroneously trigger an error if a packfile happens to have the bytes
> > > "ERR " at a packet boundary?
> > >
> > > For packfiles via upload-pack, I _think_ we're OK, because we only
> > > packetize it when a sideband is in use. In which case this would never
> > > match, because we'd have "\1" in the first byte slot.
> > >
> > > But are there are other cases we need to worry about? Just
> > > brainstorming, I can think of:
> > >
> > >   1. We also pass packetized packfiles between git-remote-https and
> > >      the stateless-rpc mode of fetch-pack/send-pack. And I don't think
> > >      we use sidebands there.
> > >
> > >   2. The packet code is used for long-lived clean/smudge filters these
> > >      days, which also pass arbitrary data.
> > >
> > > So I think it's probably not a good idea to unconditionally have callers
> > > of packet_read_with_status() handle this. We'd need a flag like
> > > PACKET_READ_RESPECT_ERR, and to trigger it from the appropriate callers.
> > 
> > This is outside of the Git pack protocol so having a separate parsing
> > mode makes sense to me.
> 
> This sounds like it could be a significant refactoring. Should we go
> back to V2 of this series, and then work on the new parsing mode
> separately?

Which one is v2? :)

Just the remote-curl cleanups from me, and then your "die on server-side
errors" patch?

-Peff