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. 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'. Also it seems to me that many patches towards the end of this series should be split. > 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. Anyway I hope the next series will introduce such tests. > In the future after libification is "complete", users external to Git will > be able to use the same trailer processing API used by the > interpret-trailers builtin. For example, a web server may want to parse > trailers the same way that Git would parse them, without having to call > interpret-trailers as a subprocess. This use case was the original > motivation behind my work in this area. Thanks for telling us about this use case. > 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. > * "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. > * "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. > * "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. > * "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.
