From: Karthik Nayak <karthik.188@gmail.com>
Date: Mon, 06 Oct 2025 12:02:27 GMT
Subject: Re: [PATCH v4 3/7] reftable: check for trailing newline in 'tables.list'
Message-ID: <CAOLa=ZT48BV5Xs8R0Mwi1tpMHKC1jrR8AGScHHAq37xU4_UpOw@mail.gmail.com>
In-Reply-To: <aN5lmEx7Q57YEyHH@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Sep 26, 2025 at 09:25:46AM +0200, Karthik Nayak wrote:
>> diff --git a/reftable/basics.c b/reftable/basics.c
>> index 9988ebd635..e969927b61 100644
>> --- a/reftable/basics.c
>> +++ b/reftable/basics.c
>> @@ -195,44 +195,55 @@ size_t names_length(const char **names)
>>  	return p - names;
>>  }
>>
>> -char **parse_names(char *buf, int size)
>> +int parse_names(char *buf, int size, char ***out)
>>  {
>
> Yup, this changed function signature reads a lot nicer to me now and is
> more in line with our usual coding style.
>

I have to agree with that. It would be so much nicer, if we could return
(value, err) in C. Anyways this seems a lot more consistent.

>> diff --git a/reftable/basics.h b/reftable/basics.h
>> index 7d22f96261..693db9524f 100644
>> --- a/reftable/basics.h
>> +++ b/reftable/basics.h
>> @@ -167,10 +167,9 @@ void free_names(char **a);
>>
>>  /*
>>   * Parse a newline separated list of names. `size` is the length of the buffer,
>> - * without terminating '\0'. Empty names are discarded. Returns a `NULL`
>> - * pointer when allocations fail.
>> + * without terminating '\0'. Empty names are discarded.
>>   */
>> -char **parse_names(char *buf, int size);
>> +int parse_names(char *buf, int size, char ***out);
>
> Tiny nit, not worth a reroll: we may still want to document that a
> return value of 0 means success, and that it otherwise returns a
> reftable error code.
>

Yes, I'll add that in my re-roll.

> Patrick