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

[Linus Arver <linus@ucla.edu>]

Phillip Wood <phillip.wood123@gmail.com> writes:

Sorry for the delay.

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

Well, having familiarized myself with the trailer machinery I was more
comfortable updating this area than reworking the details of the
sequencer code.

To me the sequencer code is a bit hard to read so I feel more
comfortable updating the trailer code as I did here in this series. I
also have another 40, 50 patches in the trailer area I want to continue
pushing up for review, so I would rather focus on that first before
coming back to the sequencer to try to clean it up. My other trailer
patches make the trailer API more precise and aware of the separator and
spaces around it, so using those richer interfaces later would make it
easier to clean up the sequencer area, I think.

So in summary I would rather not get into refactoring the sequencer at
this time.

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

Even if the user currently doesn't have ':' in trailer.separators, we
still hardcode it in as a separator. So the trailer.separators setting
doesn't matter.

The worry is that any further refactorings of sequencer would be quickly
obsoleted by the to-be-reviewed patches which enrich the trailer API
further which are sitting in my local branch.

>>> 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 there is no regression then I don't understand the concern.

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

Hmm, both the original code and the code in this patch iterate over
non-trailer lines. So the behavior is the same.

> We'd still need some way to tell the caller that
> there was a non-trailer line following the last trailer.

FWIW in one of the patches I already have currently (to be sent in a
future series), I expand the trailer API to let the caller check if the
current iteration is on a trailer or non-trailer object (they can do the
check by looking into the key and value). And in another patch I make it
so that the key field is never populated if the line is a non-trailer
line. So the capability you seek is achievable with those patches.

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

But, I've already written the explanation so this justification seems a bit
moot, no?

But ultimately I think it makes sense for the iterator to be able to
iterate over non-trailer lines because that just brings more power to
the callers that need or want it. The sequencer code is such an example
--- whether it is suboptimal or not is a separate matter, I think
(indeed I did not look too much into why the sequencer stuff wanted to
iterate over non-trailer lines when I was writing this patch).

So in summary, I'd prefer to keep this series as is. We can of course
revisit the sequencer code's use of the trailer API in the future. Thanks.

> Best Wishes
>
> Phillip