Re: [PATCH v3 3/8] reftable: check for trailing newline in 'tables.list'

From: Karthik Nayak <karthik.188@gmail.com>
Date: Sep 24, 2025, 10:02 UTC
Message-ID: <CAOLa=ZQMDjpMLeyHxeePY3VQjD1GhotXA6-GDhTNY_BDu4zSVQ@mail.gmail.com>
In-Reply-To: <aNOHjdVEbCufSCPw@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 18 quoted lines

> 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?
>

I thought about that too, I couldn't find enough consistency or reason to warrant one over the other. So I picked the one with the least change. Let me change it.

Show 9 quoted lines

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

I think that's fair. But I'll avoid making this change now, I've already added a few commits which are mostly tangential.

Show 10 quoted lines

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

Yeah, I could definitely add this in.

> Patrick