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