Re: [PATCH 00/45] ArmVirtPkg, OvmfPkg: list module-internal headers in INF files

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 12 March 2018 at 12:06, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/11/18 12:54, Ard Biesheuvel wrote:
>
>> I am merely saying that it is not always necessary to share your
>> personal journey resulting in the patches at this level of detail,
>> simply because it doesn't scale.
>
> Doesn't scale for me, or doesn't scale for reviewers?
>
> If the latter, do you suggest that I keep such detailed notes out of the
> v1 posting as well? (Because, I imagine, if I edit them down for v2
> only, then I may have wasted reviewer time already.)
>
> The recurrent bottleneck for me is trying to figure out what this or
> that part of the patch was meant to solve, and why that way. I've also
> encouraged contributors to capture their exact scenario / use case in
> commit messages; the more specific the better. (IIRC, one example is
> commit f5404a3eba1d, "OvmfPkg: Increase the maximum size for
> Authenticated variables", 2016-03-25.) IOW, I tend to find the focus too
> wide, and the information lacking.
>
> However, if I end up wasting your time instead of saving it, then I'm
> doing it wrong. I wrote up the commit messages the way I did because I
> thought it would save time for me, if I had to review the patches (I
> tend to verify patches maybe a bit too pedantically too, and I
> appreciate when the commit messages give me crutches for that). If it
> has the opposite effect on you, then I'm doing it wrong.
>
>> In any case, I am happy with this to go in as is, if you prefer.
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Thank you -- peeking ahead at Jordan's review as well, I think I'll save
> you guys another round of this.
>
> I'm honestly confused now about how I should word my future commit
> messages. Therefore I can't simply promise "I'll keep them short"; I
> might not know *how* (i.e. what to leave out). I'll need to actively
> work on that.
>

The issue you are addressing is the fact that changes to header files
will not trigger rebuilds if they are not listed in the module's .INF
file. So while I fully expect you to confirm that any .h files you add
to those .INF files are in fact depended upon, a reviewer or other
'consumer' of the patch is highly unlikely to be interested in reading
novels about how each individual .h file is used exactly, and simply
dropping the unused ones and adding the used ones should suffice.

As I see it, the commit log should explain the rationale of the
change, not a stream-of-consciousness account of how it came into
being (and I know I am exaggerating here, but only to clarify the
distinction)