On 2018.12.12 17:17, Masaya Suzuki wrote: > On Wed, Dec 12, 2018 at 3:02 AM Jeff King <peff@xxxxxxxx> 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?
