Re: [PATCH v3 03/10] trailer: teach iterator about non-trailer lines

> Christian Couder <christian.couder@gmail.com> writes:
>
>> (Sorry I just realized that I had sent this email to Linus only.)
>>
>> On Fri, Apr 26, 2024 at 2:26 AM Linus Arver via GitGitGadget
>> <gitgitgadget@gmail.com> wrote:
>>>
>>> From: Linus Arver <linusa@google.com>
>>>
>>> Previously the iterator did not iterate over non-trailer lines. This was
>>> somewhat unfortunate, because trailer blocks could have non-trailer
>>> lines in them since 146245063e (trailer: allow non-trailers in trailer
>>> block, 2016-10-21), which was before the iterator was created in
>>> f0939a0eb1 (trailer: add interface for iterating over commit trailers,
>>> 2020-09-27).
>>>
>>> So if trailer API users wanted to iterate over all lines in a trailer
>>> block (including non-trailer lines), they could not use the iterator and
>>> were forced to use the lower-level trailer_info struct directly (which
>>> provides a raw string array that includes all lines in the trailer
>>> block).
>>>
>>> Change the iterator's behavior so that we also iterate over non-trailer
>>> lines, instead of skipping over them. The new "raw" member of the
>>> iterator allows API users to access previously inaccessible non-trailer
>>> lines. Reword the variable "trailer" to just "line" because this
>>> variable can now hold both trailer lines _and_ non-trailer lines.
>>>
>>> The new "raw" member is important because anyone currently not using the
>>> iterator is using trailer_info's raw string array directly to access
>>> lines to check what the combined key + value looks like. If we didn't
>>> provide a "raw" member here, iterator users would have to re-construct
>>> the unparsed line by concatenating the key and value back together again
>>> --- which places an undue burden for iterator users.
>>>
>>> The next commit demonstrates the use of the iterator in sequencer.c as an
>>> example of where "raw" will be useful, so that it can start using the
>>> iterator.
>>>
>>> For the existing use of the iterator in builtin/shortlog.c, we don't
>>> have to change the code there because that code does
>>>
>>>     trailer_iterator_init(&iter, body);
>>>     while (trailer_iterator_advance(&iter)) {
>>>         const char *value = iter.val.buf;
>>>
>>>         if (!string_list_has_string(&log->trailers, iter.key.buf))
>>>             continue;
>>>
>>>         ...
>>>
>>> and the
>>>
>>>         if (!string_list_has_string(&log->trailers, iter.key.buf))
>>>
>>> condition already skips over non-trailer lines (iter.key.buf is empty
>>> for non-trailer lines, making the comparison still work even with this
>>> commit).
>>>
>>> Rename "num_expected_trailers" to "num_expected_objects" in
>>> t/unit-tests/t-trailer.c because the items we iterate over now include
>>> non-trailer lines.
>>
>> I think it would be simpler if the previous patch used just
>> "num_expected" or "expected". It's not like the other fields in the
>> struct ("msg" and "name") are very explicit, so why this one only?
>
> I didn't give it much thought TBH. "num_expected" SGTM. Will update.