From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 24 Sep 2025 05:54:21 GMT
Subject: Re: [PATCH v3 3/8] reftable: check for trailing newline in 'tables.list'
Message-ID: <aNOHjdVEbCufSCPw@pks.im>
In-Reply-To: <20250918-228-reftable-introduce-consistency-checks-v3-3-271af03eb34d@gmail.com>

On Thu, Sep 18, 2025 at 10:11:44AM +0200, Karthik Nayak wrote:
> diff --git a/reftable/basics.c b/reftable/basics.c
> index 9988ebd635..75d4086769 100644
> --- a/reftable/basics.c
> +++ b/reftable/basics.c
> @@ -195,7 +195,7 @@ size_t names_length(const char **names)
>  	return p - names;
>  }
>  
> -char **parse_names(char *buf, int size)
> +char **parse_names(char *buf, int size, int *err)
>  {
>  	char **names = NULL;
>  	size_t names_cap = 0;

Nit: Wouldn't it be more natural to return an `int` and assign the
result to an out-pointer?

> @@ -205,30 +205,40 @@ char **parse_names(char *buf, int size)
>  
>  	while (p < end) {
>  		char *next = strchr(p, '\n');

Not a new issue, but it's kind of broken that we use strchr(3p) here. We
really should be using `memchr(p, '\n', size - (end - p))` as the user
provides the size to us. And the provided size should be `size_t`.

> -		if (next && next < end) {
> +		if (!next) {
> +			*err = REFTABLE_FORMAT_ERROR;
> +			goto done;
> +		} else if (next < end) {
>  			*next = 0;

Can we maybe convert this line to `*next = '\0'` while at it? It made my
reading hiccup a bit.

Patrick