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

["David Woodhouse" <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 1797 bytes --]

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.

> 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).

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]