Re: [PATCH v2] ArmVirtPkg: Fix various typos

[Laszlo Ersek <lersek@redhat.com>]

On 02/07/19 18:48, Philippe Mathieu-Daudé wrote:
> Hi Antoine,
> 
> On 2/7/19 6:13 PM, Antoine Coeur wrote:
>> Thank you Laszlo.
>>
>> Do you have any recommendations regarding the maximum size of a patch for smooth reviewing on this mailing list?
>> I have about 9000 lines of additional typos corrections in queue at https://github.com/Coeur/edk2/tree/typo, but I'm afraid that big patches will simply be ignored.
> 
> I tagged your "BaseTools: Fix various typos" patch for review, but it is
> true than after reading the diff stats "172 files changed, 513
> insertions(+), 518 deletions(-)" I procrastinated and skipped to the
> next patch to review...
> 
> The rule of thumb I learned is the limit to the magic number 20,
> probably related to our number of fingers/toes :P
> 
> For example you could split the previous patch in:
> BaseTools/Source/C/Common
> BaseTools/Source/C/VfrCompile
> BaseTools/Source/C/* (other)
> BaseTools/Source/Python
> 
> And it would be more digest.
> 
>> If I split them in small patches, how many patches can I post every day? Or should I post a hundred patches at the same time and reviewers won't freak out?
> 
> The list shouldn't have limition in how many patches you can send, the
> bottleneck here is the human brain and how many patches a reviewer can take.
> 
> Since your patches are related (all fixes typos), what would help is if
> you group your patches in series, instead of sending separately.
> 
> Same here, I recommend the 20 limit. For example you can send various
> series of up-to 20 patches, like:
> - Board X + Y
> - Board Z
> - Package X
> - Module Y
> - Arm
> - Intel
> 
>> Or could we exceptionally allow pull requests on GitHub for those typos changes, as it doesn't involve actual code-change? That could be an interesting experiment that would prevent cluttering the mailing-list.
> 
> There is a study in progress to use another workflow than mailing list,
> but we are not there yet.
> Anyway, don't worry about stressing the mailing list ;)

Many small patches work a lot better for most reviewers than a few large
patches.

In this particular case I would suggest (in agreement with Phil) to post
one series per Package, and (as a rule of thumb) one patch per module
within the series.

The ArmVirtPkg patch was on the limit where I could still reasonably
review it. What helped was that I applied it locally for review, and
then I could display it with "git show --color --word-diff" as well. In
some cases, adding "-b", and/or "--word-diff", and/or "-W", and/or
"--find-copies-harder", is extremely helpful for review.

Note that this doesn't immediately imply that github pull requests would
be better. I much prefer giving feedback on the list, quoting the patch
for context, and inserting comments wherever appropriate. My personal
conviction remains that the mailing list based workflow is the most
effective. OTOH I realize the email setup is difficult for many people
(and their numbers could even be growing), so I'm not at all opposed to
adopting a web-based workflow. As Phil says, we've evaluated github (and
some other websites). While there's definitely room for improvement,
github looks like the most acceptable solution to me at this point,
after the mailing list. Some other tools are still being evaluated though.

For now I too would say, "go clutter the mailing list" -- for the list
settings as well, many small patches work better than a few huge ones.
Just please make sure they are organized in logical series. Also, please
CC the maintainers directly (from Maintainers.txt). No need to repost
just for this; it's a hint for the future.

Thank you for the contribution!
Laszlo