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

From: Josh Steadmon <[email-masked]>
Date: Thu, 13 Dec 2018 14:18:26 -0800

On 2018.12.12 17:17, Masaya Suzuki wrote:
> On Wed, Dec 12, 2018 at 3:02 AM Jeff King <[email-masked]> 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?