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

> Phillip Wood <phillip.wood123@gmail.com> writes:
>> On 02/05/2024 05:54, Linus Arver via GitGitGadget wrote:
>>> From: Linus Arver <linus@ucla.edu>
>>>
>>> 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.
>>
>> Comparing the raw line is error prone as it ignores custom separators
>> and variations in the amount of space between the key and the value.
>> Therefore I'd argue that the sequencer should in fact be comparing the
>> trailer key and value separately rather than comparing the whole line.
> 
> I agree, but that is likely beyond the scope of this series as the
> behavior of comparing the whole line was preserved (not introduced) by
> this series.

> For reference, the "Signed-off-by: " is hardcoded in "sign_off_header"
> in sequencer.c, and it is again hardcoded in "git_generated_prefixes" in
> trailer.c. We always use the hardcoded key and colon ":" separator in a
> few areas, so changing the code to be more precise to check for only the
> key (to account for variability in the separator and space around it as
> you pointed out) would be a more involved change (I think many tests
> would need to be updated).

>> There is an issue that we want to add a new Signed-off-by: trailer for
>> "C.O. Mitter" when the trailers look like
>>
>> 	Signed-off-by: C.O. Mitter <c.o.mitter@example.com>
>> 	non-trailer-line
>>
>> but not when they look like
>>
>> 	Signed-off-by: C.O. Mitter <c.o.mitter@example.com>
>>
>> so we still need some way of indicating that there was a non-trailer
>> line after the last trailer though.
> 
> What is the issue, exactly? Also can you clarify if the issue is
> introduced by this series (did you spot a regression)?

>>> 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
>>
>> An interface that lets the caller pass a flag if they want to know about
>> non-trailer lines might be easier to use for the callers that don't want
>> to worry about such lines and wouldn't need a justification as to why it
>> was safe for existing callers.
> 
> Makes sense. But perhaps such API enhancements belong in a future
> series, when other callers that need such flexibility could benefit from
> it?