Christian Couder <christian.couder@xxxxxxxxx> writes: > On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget > <gitgitgadget@xxxxxxxxx> wrote: >> >> This patch series is the first 10 patches of a larger cleanup/bugfix series >> (henceforth "larger series") I've been working on. > > There are now 28 patches in this series. > > 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. > For example perhaps one series until the "trailer: move > interpret_trailers() to interpret-trailers.c" patch, then another one > until "trailer: finish formatting unification". etc. > > Also I think it might be possible to avoid some test failures > introduced by some patches. If it's not possible, I agree with Junio > that it would be nice if the failing tests were changed to use > 'test_expect_failure'. As the breakages are deliberate, I will have to go with using "test_expect_failure". > 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. Thanks. >> The main goal of this >> series is to begin the process of "libifying" the trailer API. By "API" I >> mean the interface exposed in trailer.h. The larger series brings a number >> of additional cleanups (exposing and fixing some bugs along the way), and >> builds on top of this series. > > [...] > >> With the libification-focused goals out of the way, let's turn to this patch >> series in more detail. > > I like the goal of libifying Git the trailer API, and the way you want > to do it seems reasonable to me. > > [...] > >> In summary this series breaks up "process_trailers()" into smaller pieces, >> exposing many of the parts relevant to trailer-related processing in >> trailer.h. This will force us to eventually introduce unit tests for these >> API functions, but that is a good thing for API stability. > > 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". > [...] > >> 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. >> * "interpret-trailers: do not modify the input if NOP" (Refrain from >> subtracting or adding a newline around the patch divider "---" if we are >> not adding new trailers.) > > It could be a feature to be able to normalize this too. OK, but it would again be undocumented behavior. >> * "trailer formatter: split up format_trailer() monolith" (Fix a bug in >> git-log where we still printed a blank newline even if we didn't want to >> format anything.) > > I am not sure this is a bug fix either. It could perhaps be a normalization too. See my comment above. >> * "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.
