Hi Linus
On 05/05/2024 02:37, Linus Arver wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
On 02/05/2024 05:54, Linus Arver via GitGitGadget wrote:
From: Linus Arver <linus@xxxxxxxx>
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@xxxxxxxxxxx>
non-trailer-line
but not when they look like
Signed-off-by: C.O. Mitter <c.o.mitter@xxxxxxxxxxx>
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
