Christian Couder <christian.couder@xxxxxxxxx> writes: > On Tue, Feb 13, 2024 at 8:39 PM Linus Arver <linusa@xxxxxxxxxx> wrote: >> >> Christian Couder <christian.couder@xxxxxxxxx> 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!
