From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Thu, 20 Jun 2019 13:35:48 -0700 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2338EC1EB204; Thu, 20 Jun 2019 20:35:38 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-40.ams2.redhat.com [10.36.116.40]) by smtp.corp.redhat.com (Postfix) with ESMTP id DF1F5608A7; Thu, 20 Jun 2019 20:35:35 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 1/2] LegacyBios: set NumberBbsEntries to the size of BbsTable To: David Woodhouse , devel@edk2.groups.io, jljusten@gmail.com, afish@apple.com, Paolo Bonzini References: <9c9d53f5da9c3dec56cff0ff07c399e3034444a1.camel@infradead.org> <584f8af7979d40fd960122fc077fad02ecce09b1.camel@infradead.org> <29111f7d-0f4c-bba6-697d-c061ce1b6488@redhat.com> <3def810039016f9b48deff0526720136a996a7b3.camel@infradead.org> From: "Laszlo Ersek" Message-ID: <3f9de0c1-afc3-1984-fe1c-4ea4021974ba@redhat.com> Date: Thu, 20 Jun 2019 22:35:34 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <3def810039016f9b48deff0526720136a996a7b3.camel@infradead.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Thu, 20 Jun 2019 20:35:43 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 Thanks Laszlo