Re: [PATCH v4 03/12] pkt-line: (optionally) libify the packet readers

From: Jeff King <peff@peff.net>
Date: Mar 4, 2021, 14:40 UTC
Message-ID: <YEDxUFJmPH4nP6Qk@coredump.intra.peff.net>
In-Reply-To: <6b1ce8c0-0881-77a0-deda-677e34560cc0@jeffhostetler.com>

On Thu, Mar 04, 2021 at 09:17:41AM -0500, Jeff Hostetler wrote:

Show 26 quoted lines

> > In the post-context of this hunk, there is this code:
> > 
> > 	if ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&
> > 	    starts_with(buffer, "ERR "))
> > 		die(_("remote error: %s"), buffer + 4);
> > 
> > 	*pktlen = len;
> > 	return PACKET_READ_NORMAL;
> > 
> > But here, there is no way to override the DIE_ON_ERR with
> > READ_NEVER_DIE.
> > 
> > The asymmetry is somewhat annoying (i.e. if "if you do not want to
> > die upon ERR, don't pass DIE_ON_ERR" could be a valid suggestion to
> > the callers, then "if you do not want to die upon an unexpected
> > hung-up, pass GENTLE_ON_EOF" would equally be valid suggestion),
> > but I'll let it pass.
> 
> I agree that there is something odd about all of these flags,
> but I don't have the context on all the various caller combinations
> to make a better suggestion at this time.  And I certainly don't
> want to stir up a bigger mess than I already have. :-)
> 
> We did document in the .h that READ_NEVER_DIE excludes ERR packets
> when READ_DIE_ON_ERR is set, so I think we're safe from unexpected
> surprises.

I think the flag is doing sensible things; it's just that the word "never" in the name is confusing, since it is "never except this one time".

Would PACKET_READ_GENTLE_ON_READ_ERROR be a better name, to match GENTLE_ON_EOF? I was tempted to just call it "ON_ERROR", since it also include parsing errors, but maybe somebody would think that includes ERR packets (that is more of a stretch, though, I think).

Likewise, I kind of wonder if callers would really prefer suppressing the error() calls, too. Saying "error: the remote end hung up unexpectedly" is not that helpful if the "remote end" we are talking about is fsmonitor, and not the server side of a fetch.

-Peff