Re: [edk2-devel] [PATCH 1/2] LegacyBios: set NumberBbsEntries to the size of BbsTable

["Laszlo Ersek" <lersek@redhat.com>]

On 06/20/19 19:32, David Woodhouse wrote:
> On Thu, 2019-06-20 at 18:12 +0200, Laszlo Ersek wrote:
>> If it's possible, please write a non-empty commit message body, for each
>> patch in this series. Just one sentence on this patch would be nice.
> 
> 
> The comnit comment isn't empty. It has one sentence "LegacyBios: set
> NumberBbsEntries to the size of BbsTable".

I was careful to write, "non-empty commit message *body*".

Yes, you have a subject line, and if you look at the commit with "git
show", or another tool that shows the subject (= commit "title") near
the body, things look fine. Things read pretty weird in Thunderbird
however, for example.

> A second sentence would be redundant and render the description larger
> than the patch itself. I can't think of anything useful I'd want to put
> in it. Apparently neither could you when you posted the same patch in
> 2013 after splitting it out from a larger patch of mine. :)
> 
> https://www.mail-archive.com/edk2-devel@lists.sourceforge.net/msg01618.html

My past mistake doesn't excuse the current commit message ;)

Anyway, I intentionally didn't ask for just repeating the commit msg
title in the commit msg body. You could mention why the pre-patch
expression (1 + 2 * MAX_IDE_CONTROLLER) is wrong, or wasteful. It's not
obvious at all from just the patch itself (without looking at the larger
context in the source file). You can save time for reviewers by giving
hints in the commit message. And if you *can* save time for reviewers,
you should. :)

>> I think your note on patch#2 is valuable too and should be captured in
>> the commit message body. Please consider formulating it with a bit more
>> neutral tone :)
> 
> I would prefer to express that particular concern in 'diff -up' form
> when actually fixing it :)
> 

I'm a big fan of detailed commit messages. It's OK (and welcome) to
describe current shortcomings even if you intend to fix them later.

The commit message could also capture in one or two sentences what the
patch does (locate handles with BlockIo instances, filter out some of
them based on their DevicePath instances and other criteria, then for
each handle, take the PCI B/D/F from the most specific PciIo instance on
the device path). Anyway, I wouldn't like to obsess about this.

series
Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo