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 <ler...@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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#42759): https://edk2.groups.io/g/devel/message/42759 Mute This Topic: https://groups.io/mt/32122467/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
