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

[Christian Couder <christian.couder@gmail.com>]

On Tue, Feb 13, 2024 at 8:39 PM Linus Arver <linusa@google.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:

> > I took a look at all of them, and I think that this series should be
> > split into 4 or more series.
>
> This sounds fine to me. IIRC this means for the 2nd, 3rd+ series I have
> to remember to say "this series builds on top of the other topic
> branches '...'" in the cover letter. Now that I've written this out I
> will hopefully not forget to do this...
>
> Or, I suppose I could just introduce the 1st sub-series, wait for that
> to get queued to next, then (re)introduce the 2nd sub-series, etc, in
> order. Hmm. I think this will be simpler.

Yeah, sure.

> > Also it seems to me that many patches towards the end of this series
> > should be split.
>
> In hindsight, I fully agree.
>
> Aside: I am delighted with the quality of reviews on this project. It's
> not something I am used to, so please bear with me while I try to break
> old habits.

Sure no worries.

> Thanks.

[...]

> > I am a bit sad that this series doesn't introduce unit tests using the
> > new test framework in C yet. I understand that this series is mostly a
> > big refactoring and maybe it's better to introduce unit tests only
> > when the refactoring is finished though.
>
> This was my original goal as well, and still is.
>
> > Anyway I hope the next series will introduce such tests.
>
> I will see which API functions are stable enough, and add tests
> accordingly (in a patch series sooner than later).
>
> Probably the "biggest" (?) thing that is coming from the larger series
> is the introduction of a complete separation between parsing (without
> any modification of the input) and formatting. The parser/formatter is
> a large chunk of the trailer implementation, so I would expect unit
> tests for those bits to have to wait until those improvements are merged
> into "next".

Ok.

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

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