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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Linus

On 05/05/2024 02:37, Linus Arver wrote:
> 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.

Right but this series is changing the trailer iterator api to 
accommodate the sub-optimal sequencer code. My thought was that if the 
sequencer did the right thing we wouldn't need to expose the raw line in 
the iterator in the first place.

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

So the worry is that we'd create a "Signed-off-by: " trailer that we 
then couldn't parse because the user didn't have ':' in trailer.separators?

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

There is no regression - the issue is with my suggestion. We only want 
to add an SOB trailer if the last trailer does not match the SOB we're 
adding. If we were to use the existing trailer iterator api in the 
sequencer we would not know that we should add an SOB in the first 
example above as we'd only see the last trailer which matches the SOB 
we're trying to add. We'd still need some way to tell the caller that 
there was a non-trailer line following the last trailer.

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

For me the main benefit would be that you don't have to spend time 
explaining why the changes are safe for existing callers because they 
would keep the existing iterator behavor.

Best Wishes

Phillip