Junio C Hamano <gitster@xxxxxxxxx> writes: > "Linus Arver via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > >> From: Linus Arver <linusa@xxxxxxxxxx> >> >> This patch allows for the removal of "trailer_info_get()" from the >> trailer.h API, which will be in the next patch. > > Hmph, do you mean "shortlog" and the sequencer were the only two > external callers and with this we can make it file-scope static to > trailer.c? This was what I meant (originally ... > Or do you mean the next step will be more than a removal > of a declaration from trailer.h plus adding "static" in front of its > definition in trailer.c, because there need other adjustments before > that happens? ... but now I realize that the operation adds a few small tweaks, such as tweaking the parameters it expects and also what it returns). In the spirit of breaking up patch 3, I will also break this up into preparatory patches as well. >> Also, teach the iterator about non-trailer lines, by adding a new field >> called "raw" to hold both trailer and non-trailer lines. This is >> necessary because a "trailer block" is a list of trailer lines of at >> least 25% trailers (see 146245063e (trailer: allow non-trailers in >> trailer block, 2016-10-21)), such that it may hold non-trailer lines. > > That sounds like a task larger than something we would want in a > patch that focuses on another task (e.g. update sequencer not to > call trailer_info_get()) while at it. It seems from a casual glance > that the change to shortlog.c is to accomodate this change in the > semantics of what the iterator could return? It smells that this > patch does two more or less unrelated things at the same time? I think you're correct. Hopefully breaking this up will make things easier to review. I am learning very quickly from your review comments in patch 3 and in here that, in the absence of area experts, the existing tests/CI tests cannot be trusted alone (after all some tests could be missing), which makes it more important to do so-called "micro-commits". But overall, breaking things up is a good thing anyway as a general practice, so, I think this is a good lesson. TBH I have a (bad) habit of saying "is the diff ~100 lines?" and if so I don't spend any time thinking of breaking these up. X-< Thanks.
