Subject: Re: [PATCH v3 04/10] sequencer: use the trailer iterator

From: Linus Arver <[email-masked]>
Date: Thu, 01 Feb 2024 11:14:35 -0800

Junio C Hamano <[email-masked]> writes:

> "Linus Arver via GitGitGadget" <[email-masked]> writes:
>
>> From: Linus Arver <[email-masked]>
>>
>> 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.