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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 16, 2016, 22:17 UTC
Message-ID: <xmqqoa3nij5z.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1473984742-12516-4-git-send-email-kevin.m.wern@gmail.com>

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

Show 5 quoted lines

>  	/* 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)

Show 8 quoted lines

> @@ -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".

Show 11 quoted lines

> +/*
> + * 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().