Re: [PATCH v4 00/28] Enrich Trailer API

[Linus Arver <linusa@google.com>]

Christian Couder <christian.couder@gmail.com> writes:

> On Tue, Feb 13, 2024 at 8:39 PM Linus Arver <linusa@google.com> wrote:
>>
>> Christian Couder <christian.couder@gmail.com> writes:
>
> [...]
> 
>> >> Thanks to the aggressive refactoring in this series, I've been able to
>> >> identify and fix several bugs in our existing implementation. Those fixes
>> >> build on top of this series but were not included here, in order to keep
>> >> this series small. Below is a "shortlog" of those fixes I have locally:
>> >>
>> >>  * "trailer: trailer replacement should not change its position" (If we
>> >>    found a trailer we'd like to replace, preserve its position relative to
>> >>    the other trailers found in the trailer block, instead of always moving
>> >>    it to the beginning or end of the entire trailer block.)
>> >
>> > I believe there was a reason why it was done this way. I don't
>> > remember what it was though.
>>
>> Noted. I'll see what I can find in our commit history.
>>
>> >>  * "interpret-trailers: preserve trailers coming from the input" (Sometimes,
>> >>    the parsed trailers from the input will be formatted differently
>> >>    depending on whether we provide --only-trailers or not. Make the trailers
>> >>    that were not modified and which are coming directly from the input get
>> >>    formatted the same way, regardless of this flag.)
>> >
>> > It could be a feature to be able to normalize trailers in a certain way.
>>
>> True. But doing such normalization silently is undocumented behavior,
>> and we should provide explicit flags for this sort of thing. Adding such
>> flags might be the right thing to do (let's discuss more when this patch
>> gets proposed). FWIW the behavior I observed is that this normalization
>> only happens for *some* trailers that have configuration options, not
>> all trailers in the input. So it's a special kind of (limited)
>> normalization.
>
> Perhaps because we consider that having some configuration means that
> the user consistently expects things in a certain way.

Yes, this was one possibility I considered after sending my reply. If a
user has gone out of their way to configure something, maybe they do
want things (for those bits) to be normalized.

And adding a flag to disable normalization seems like a good feature to
have also (while keeping the behavior of the interpret-trailers that has
been relatively untouched since its introduction). But anyway I'm
getting a little bit ahead of myself.

>> >>  * "interpret-trailers: fail if given unrecognized arguments" (E.g., for
>> >>    "--where", only accept recognized WHERE_* enum values. If we get
>> >>    something unrecognized, fail with an error instead of silently doing
>> >>    nothing. Ditto for "--if-exists" and "--if-missing".)
>> >
>> > It's possible that there was a reason why it was done this way.
>> >
>> > I think you might want to take a look at the discussions on the
>> > mailing list when "interpret-trailers" was developed. There were a lot
>> > of discussions over a long time, and there were a lot of requests and
>> > suggestions about what it should do.
>>
>> I confess I haven't looked too deeply into the original threads
>> surrounding the introduction of "interpret-trailers". But all of the
>> changes which I categorize as bugfixes above have a theme of
>> undocumented modifications.
>>
>> While working on this (and the larger) series around trailers, I only
>> looked into some (not all) of the discussions on the mailing list in
>> this area. Instead, I deferred to
>> Documentation/git-interpret-trailers.txt as the official (authoritative)
>> source of truth. This is partly why I first started out on this project
>> last year by making improvements to that doc. And, this is why seeing so
>> many edge cases where we perform such undocumented modifications smelled
>> more like bugs to me than features.
>>
>> That being said, I cannot disagree with your position that the so-called
>> bugfixes I've reported above could be misguided (and should rather be
>> just updates to missing documentation). Let's not try to decide that
>> here, but instead later when I get to introduce those changes on the
>> list, one at a time. Thanks.
>
> Yeah, it might seem like undocumented features are bad, and I agree
> that reading original discussions can be tiring. But if the latter
> makes it possible to fix undocumented features by just properly
> documenting them, then I think it might just be the best thing to do.
> Ok not to decide about it now though.

Thanks!