Re: [PATCH v4 19/28] trailer: make trailer_info struct private

From: Linus Arver <linusa@google.com>
Date: Feb 13, 2024, 17:41 UTC
Message-ID: <owlyil2sb6tf.fsf@fine.c.googlers.com>
In-Reply-To: <CAP8UFD1twELGKvvesxgCrZrypKZpgSt04ira3mvurG1UbpDfxQ@mail.gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 25 quoted lines

> On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>
>> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
>> index 11f4ce9e4a2..6bf8cec005a 100644
>> --- a/builtin/interpret-trailers.c
>> +++ b/builtin/interpret-trailers.c
>> @@ -141,7 +141,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
>>         LIST_HEAD(head);
>>         struct strbuf sb = STRBUF_INIT;
>>         struct strbuf trailer_block = STRBUF_INIT;
>> -       struct trailer_info info;
>> +       struct trailer_info *info;
>>         FILE *outfile = stdout;
>>
>>         trailer_config_init();
>> @@ -151,13 +151,13 @@ static void interpret_trailers(const struct process_trailer_options *opts,
>>         if (opts->in_place)
>>                 outfile = create_in_place_tempfile(file);
>>
>> -       parse_trailers(opts, &info, sb.buf, &head);
>> +       info = parse_trailers(opts, sb.buf, &head);
>
> I think this patch might be doing too much at once and could have been
> split into 3 or more patches to make reviews easier.

Ack, will do.

> For example the first patch could introduce trailer_info_new() and
> make interpret_trailers() use it. Then the second patch could modify
> parse_trailers() so that it returns a 'struct trailer_info *'. Then
> the third patch could make the trailer_info struct private.

Thanks, I will try to use this breakdown in the next reroll.