Re: [PATCH 03/11] pkt-line: create gentle packet_read_line functions

[Junio C Hamano <gitster@pobox.com>]

Kevin Wern <kevin.m.wern@gmail.com> writes:

>  	/* And complain if we didn't get enough bytes to satisfy the read. */
>  	if (ret < size) {
> -		if (options & PACKET_READ_GENTLE_ON_EOF)
> +		if (options & (PACKET_READ_GENTLE_ON_EOF | PACKET_READ_GENTLE_ALL))
>  			return -1;

The name _ALL suggested to me that there may be multiple "under this
condition, be gentle", "under that condition, be gentle", and _ALL
is used as a catch-all "under any condition, be gentle".  If you
defined _ALL symbol to have all GENTLE bits on, this line could have
become

	if (options & PACKET_READ_GENTLE_ALL)

> @@ -205,15 +209,23 @@ int packet_read(int fd, char **src_buf, size_t *src_len,
>  	if (ret < 0)
>  		return ret;
>  	len = packet_length(linelen);
> -	if (len < 0)
> +	if (len < 0) {
> +		if (options & PACKET_READ_GENTLE_ALL)
> +			return -1;

On the other hand, however, you do want to die here when only
GENTLE_ON_EOF is set.

Taking the above two observations together, I'd have to say that
_ALL is probably a misnomer.  I agree with a need for a flag with
the behaviour you defined in this patch, though.

>  		die("protocol error: bad line length character: %.4s", linelen);

>  static char *packet_read_line_generic(int fd,
>  				      char **src, size_t *src_len,
> -				      int *dst_len)
> +				      int *dst_len, int flags)

The original one is called options, not flags, and it would be
easier to follow if it is consistently called options, instead of
requiring the reader to keep track of "ah, it is called flags here
but the callee renames it to options".

> +/*
> + * Same as packet_read_line, but does not die on any errors (uses
> + * PACKET_READ_GENTLE_ALL).
> + */
> +char *packet_read_line_gentle(int fd, int *len_p);
> +
> +/*
> + * Same as packet_read_line_buf, but does not die on any errors (uses
> + * PACKET_READ_GENTLE_ALL).
> + */
> +char *packet_read_line_buf_gentle(char **src_buf, size_t *src_len, int *size);

I think most if not all "do the same thing as do_something() but
report errors instead of dying" variant of functions are named
do_something_gently(), not do_something_gentle().