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

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

On 06/24/19 13:48, David Woodhouse wrote:
> On Thu, 2019-06-20 at 22:35 +0200, Laszlo Ersek wrote:
>> 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. :)
> 
> With specific requests for things that weren't already in the subject
> line, I'm perfectly happy to add more.
> 
> Better now?
> 
>>>> 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.
> 
> Done.

Thanks!

>> series
>> Acked-by: Laszlo Ersek <lersek@redhat.com>
> 
> Out of interest, why Acked-by: for these two vs. Reviewed-by: for the
> first one? It's all under OvmfPkg now (which is made clearer in the
> series I posted on Friday than it is in the subject line here).
> 

For me anyway, R-b means a (reasonably) deep understanding & agreement.
Acked-by means "looks okay, or at least I'm not worried about
regressions in this area -- I'll let you do whatever you need to do". :)

Thanks
Laszlo