Re: [PATCH v4 03/12] pkt-line: (optionally) libify the packet readers
- From
- Jeff Hostetler <git@jeffhostetler.com>
- Date
- Mar 4, 2021, 14:17 UTC
- Message-ID
- <6b1ce8c0-0881-77a0-deda-677e34560cc0@jeffhostetler.com>
- In-Reply-To
- <xmqqwnuohv4t.fsf@gitster.c.googlers.com>
On 3/3/21 2:53 PM, Junio C Hamano wrote:
Show 47 quoted lines
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> @@ -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..
>
>> @@ -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.
Show 19 quoted lines
> >> 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);