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@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? Which one is v2? :) Just the remote-curl cleanups from me, and then your "die on server-side errors" patch? -Peff
