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

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:

This hunk treats READ_NEVER_DIE as a less quiet version of
GENTRL_ON_EOF, i.e. the new flag allows to continue even after the
"hung up unexpectedly" condition that usually causes the process to
die..

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.

On 3/3/21 2:53 PM, Junio C Hamano wrote:

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:

quoted

@@ -313,6 +316,8 @@ static int get_packet_data(int fd, char **src_buf,
  		if (options & PACKET_READ_GENTLE_ON_EOF)
  			return -1;
  
+		if (options & PACKET_READ_NEVER_DIE)
+			return error(_("the remote end hung up unexpectedly"));
  		die(_("the remote end hung up unexpectedly"));
  	}

This hunk treats READ_NEVER_DIE as a less quiet version of
GENTRL_ON_EOF, i.e. the new flag allows to continue even after the
"hung up unexpectedly" condition that usually causes the process to
die..

quoted

@@ -355,12 +363,19 @@ enum packet_read_status packet_read_with_status(i

...
-	if ((unsigned)len >= size)
+	if ((unsigned)len >= size) {
+		if (options & PACKET_READ_NEVER_DIE)
+			return error(_("protocol error: bad line length %d"),
+				     len);
  		die(_("protocol error: bad line length %d"), len);
+	}
  
  	if (get_packet_data(fd, src_buffer, src_len, buffer, len, options) < 0) {
  		*pktlen = -1;

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.

quoted

diff --git a/pkt-line.h b/pkt-line.h
index a7149429ac35..2e472efaf2c5 100644
--- a/pkt-line.h
+++ b/pkt-line.h

@@ -75,10 +75,14 @@ int write_packetized_from_buf_no_flush(const char *src_in, size_t len, int fd_ou
   *
   * If options contains PACKET_READ_DIE_ON_ERR_PACKET, it dies when it sees an
   * ERR packet.
+ *
+ * With `PACKET_READ_NEVER_DIE`, no errors are allowed to trigger die() (except
+ * an ERR packet, when `PACKET_READ_DIE_ON_ERR_PACKET` is in effect).
   */
  #define PACKET_READ_GENTLE_ON_EOF     (1u<<0)
  #define PACKET_READ_CHOMP_NEWLINE     (1u<<1)
  #define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)
+#define PACKET_READ_NEVER_DIE         (1u<<3)
  int packet_read(int fd, char **src_buffer, size_t *src_len, char
  		*buffer, unsigned size, int options);

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

quoted

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