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 <ler...@redhat.com> Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#42661): https://edk2.groups.io/g/devel/message/42661 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] -=-=-=-=-=-=-=-=-=-=-=-
