Re: [edk2] [PATCH v2] ArmVirtPkg: Fix various typos
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 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] ArmVirtPkg: Fix various typos
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 ;) Regards, Phil. > > Coeur > > > > Gesendet: Donnerstag, 07. Februar 2019 um 21:33 Uhr > Von: "Laszlo Ersek" > An: "Antoine Coeur" , edk2-devel@lists.01.org > Betreff: Re: [edk2] [PATCH v2] ArmVirtPkg: Fix various typos > > Pushed as commit 7a90895306ac. > > Thanks, > Laszlo > ___ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] ArmVirtPkg: Fix various typos
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. 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? 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. Coeur Gesendet: Donnerstag, 07. Februar 2019 um 21:33 Uhr Von: "Laszlo Ersek" An: "Antoine Coeur" , edk2-devel@lists.01.org Betreff: Re: [edk2] [PATCH v2] ArmVirtPkg: Fix various typos Pushed as commit 7a90895306ac. Thanks, Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] ArmVirtPkg: Fix various typos
On 02/06/19 16:48, Antoine Coeur wrote: > Fix various typos in ArmVirtPkg. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Coeur > --- > ArmVirtPkg/ArmVirt.dsc.inc | 2 +- > ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c| 2 +- > ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c | 4 ++-- > .../FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c | 6 +++--- > .../Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c | 6 +++--- > ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 4 ++-- > 6 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc > index c47955be94..304a4b4d21 100644 > --- a/ArmVirtPkg/ArmVirt.dsc.inc > +++ b/ArmVirtPkg/ArmVirt.dsc.inc > @@ -333,7 +333,7 @@ ># Values are in EFI Pages (4K). DXE Core will make sure that ># at least this much of each type of memory can be allocated ># from a single memory range. This way you only end up with > - # maximum of two fragements for each type in the memory map > + # maximum of two fragments for each type in the memory map ># (the memory used, and the free memory that was prereserved ># but not used). ># > diff --git a/ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c > b/ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c > index 73a0fbd8a4..758bc91ab1 100644 > --- a/ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c > +++ b/ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c > @@ -1,5 +1,5 @@ > /** @file > - HOB Library implemenation for Dxe Phase with DebugLib dependency removed > + HOB Library implementation for Dxe Phase with DebugLib dependency removed > > Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved. > Copyright (c) 2014, Linaro Ltd. All rights reserved. > diff --git a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > index 6479f53b37..a083826bab 100644 > --- a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > +++ b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > @@ -1238,7 +1238,7 @@ PciExpressBitFieldAndThenOr32 ( >Size into the buffer specified by Buffer. This function only allows the PCI >configuration registers from a single PCI function to be read. Size is >returned. When possible 32-bit PCI configuration read cycles are used to > read > - from StartAdress to StartAddress + Size. Due to alignment restrictions, > 8-bit > + from StartAddress to StartAddress + Size. Due to alignment restrictions, > 8-bit >and 16-bit PCI configuration read cycles may be used at the beginning and > the >end of the range. > > @@ -1338,7 +1338,7 @@ PciExpressReadBuffer ( >Size from the buffer specified by Buffer. This function only allows the PCI >configuration registers from a single PCI function to be written. Size is >returned. When possible 32-bit PCI configuration write cycles are used to > - write from StartAdress to StartAddress + Size. Due to alignment > restrictions, > + write from StartAddress to StartAddress + Size. Due to alignment > restrictions, >8-bit and 16-bit PCI configuration write cycles may be used at the > beginning >and the end of the range. > > diff --git > a/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c > b/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c > index f0924dacbc..16c0e61816 100644 > --- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c > +++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c > @@ -234,13 +234,13 @@ SerialPortGetControl ( > } > > /** > - Sets the baud rate, receive FIFO depth, transmit/receice time out, parity, > + Sets the baud rate, receive FIFO depth, transmit/receive time out, parity, >data bits, and stop bits on a serial device. > >@param BaudRate The requested baud rate. A BaudRate value of 0 > will use the > device's default interface speed. > On output, the value actually set. > - @param ReveiveFifoDepth The requested depth of the FIFO on the receive > side of the > + @param ReceiveFifoDepth The requested depth of the FIFO on the receive > side of the > serial interface. A ReceiveFifoDepth value of 0 > will use > the device's default FIFO depth. > On output, the value actually set. > @@ -253,7 +253,7 @@ SerialPortGetControl ( > DefaultParity will use the device's default > parity value. > On output, the value actually set. >@param DataBits The number of data bits to use on the serial > device. A DataBits > -vaule of 0 will use the device's default data > bit setting. > +
Re: [edk2] [PATCH v2] ArmVirtPkg: Fix various typos
On 2/6/19 4:48 PM, Antoine Coeur wrote: > Fix various typos in ArmVirtPkg. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Coeur Reviewed-by: Philippe Mathieu-Daude > --- > ArmVirtPkg/ArmVirt.dsc.inc | 2 +- > ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c| 2 +- > ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c | 4 ++-- > .../FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c | 6 +++--- > .../Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c | 6 +++--- > ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 4 ++-- > 6 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc > index c47955be94..304a4b4d21 100644 > --- a/ArmVirtPkg/ArmVirt.dsc.inc > +++ b/ArmVirtPkg/ArmVirt.dsc.inc > @@ -333,7 +333,7 @@ ># Values are in EFI Pages (4K). DXE Core will make sure that ># at least this much of each type of memory can be allocated ># from a single memory range. This way you only end up with > - # maximum of two fragements for each type in the memory map > + # maximum of two fragments for each type in the memory map ># (the memory used, and the free memory that was prereserved ># but not used). ># > diff --git a/ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c > b/ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c > index 73a0fbd8a4..758bc91ab1 100644 > --- a/ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c > +++ b/ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c > @@ -1,5 +1,5 @@ > /** @file > - HOB Library implemenation for Dxe Phase with DebugLib dependency removed > + HOB Library implementation for Dxe Phase with DebugLib dependency removed > > Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved. > Copyright (c) 2014, Linaro Ltd. All rights reserved. > diff --git a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > index 6479f53b37..a083826bab 100644 > --- a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > +++ b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > @@ -1238,7 +1238,7 @@ PciExpressBitFieldAndThenOr32 ( >Size into the buffer specified by Buffer. This function only allows the PCI >configuration registers from a single PCI function to be read. Size is >returned. When possible 32-bit PCI configuration read cycles are used to > read > - from StartAdress to StartAddress + Size. Due to alignment restrictions, > 8-bit > + from StartAddress to StartAddress + Size. Due to alignment restrictions, > 8-bit >and 16-bit PCI configuration read cycles may be used at the beginning and > the >end of the range. > > @@ -1338,7 +1338,7 @@ PciExpressReadBuffer ( >Size from the buffer specified by Buffer. This function only allows the PCI >configuration registers from a single PCI function to be written. Size is >returned. When possible 32-bit PCI configuration write cycles are used to > - write from StartAdress to StartAddress + Size. Due to alignment > restrictions, > + write from StartAddress to StartAddress + Size. Due to alignment > restrictions, >8-bit and 16-bit PCI configuration write cycles may be used at the > beginning >and the end of the range. > > diff --git > a/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c > b/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c > index f0924dacbc..16c0e61816 100644 > --- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c > +++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c > @@ -234,13 +234,13 @@ SerialPortGetControl ( > } > > /** > - Sets the baud rate, receive FIFO depth, transmit/receice time out, parity, > + Sets the baud rate, receive FIFO depth, transmit/receive time out, parity, >data bits, and stop bits on a serial device. > >@param BaudRate The requested baud rate. A BaudRate value of 0 > will use the > device's default interface speed. > On output, the value actually set. > - @param ReveiveFifoDepth The requested depth of the FIFO on the receive > side of the > + @param ReceiveFifoDepth The requested depth of the FIFO on the receive > side of the > serial interface. A ReceiveFifoDepth value of 0 > will use > the device's default FIFO depth. > On output, the value actually set. > @@ -253,7 +253,7 @@ SerialPortGetControl ( > DefaultParity will use the device's default > parity value. > On output, the value actually set. >@param DataBits The number of data bits to use on the serial > device. A DataBits > -vaule of 0 will use the device's
Re: [edk2] [PATCH v2] ArmVirtPkg: Fix various typos
On 02/06/19 16:48, Antoine Coeur wrote: > Fix various typos in ArmVirtPkg. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Coeur > --- > ArmVirtPkg/ArmVirt.dsc.inc | 2 +- > ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c| 2 +- > ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c | 4 ++-- > .../FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c | 6 +++--- > .../Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c | 6 +++--- > ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 4 ++-- > 6 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc > index c47955be94..304a4b4d21 100644 > --- a/ArmVirtPkg/ArmVirt.dsc.inc > +++ b/ArmVirtPkg/ArmVirt.dsc.inc > @@ -333,7 +333,7 @@ ># Values are in EFI Pages (4K). DXE Core will make sure that ># at least this much of each type of memory can be allocated ># from a single memory range. This way you only end up with > - # maximum of two fragements for each type in the memory map > + # maximum of two fragments for each type in the memory map ># (the memory used, and the free memory that was prereserved ># but not used). ># > diff --git a/ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c > b/ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c > index 73a0fbd8a4..758bc91ab1 100644 > --- a/ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c > +++ b/ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c > @@ -1,5 +1,5 @@ > /** @file > - HOB Library implemenation for Dxe Phase with DebugLib dependency removed > + HOB Library implementation for Dxe Phase with DebugLib dependency removed > > Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved. > Copyright (c) 2014, Linaro Ltd. All rights reserved. > diff --git a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > index 6479f53b37..a083826bab 100644 > --- a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > +++ b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > @@ -1238,7 +1238,7 @@ PciExpressBitFieldAndThenOr32 ( >Size into the buffer specified by Buffer. This function only allows the PCI >configuration registers from a single PCI function to be read. Size is >returned. When possible 32-bit PCI configuration read cycles are used to > read > - from StartAdress to StartAddress + Size. Due to alignment restrictions, > 8-bit > + from StartAddress to StartAddress + Size. Due to alignment restrictions, > 8-bit >and 16-bit PCI configuration read cycles may be used at the beginning and > the >end of the range. > > @@ -1338,7 +1338,7 @@ PciExpressReadBuffer ( >Size from the buffer specified by Buffer. This function only allows the PCI >configuration registers from a single PCI function to be written. Size is >returned. When possible 32-bit PCI configuration write cycles are used to > - write from StartAdress to StartAddress + Size. Due to alignment > restrictions, > + write from StartAddress to StartAddress + Size. Due to alignment > restrictions, >8-bit and 16-bit PCI configuration write cycles may be used at the > beginning >and the end of the range. > > diff --git > a/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c > b/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c > index f0924dacbc..16c0e61816 100644 > --- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c > +++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c > @@ -234,13 +234,13 @@ SerialPortGetControl ( > } > > /** > - Sets the baud rate, receive FIFO depth, transmit/receice time out, parity, > + Sets the baud rate, receive FIFO depth, transmit/receive time out, parity, >data bits, and stop bits on a serial device. > >@param BaudRate The requested baud rate. A BaudRate value of 0 > will use the > device's default interface speed. > On output, the value actually set. > - @param ReveiveFifoDepth The requested depth of the FIFO on the receive > side of the > + @param ReceiveFifoDepth The requested depth of the FIFO on the receive > side of the > serial interface. A ReceiveFifoDepth value of 0 > will use > the device's default FIFO depth. > On output, the value actually set. > @@ -253,7 +253,7 @@ SerialPortGetControl ( > DefaultParity will use the device's default > parity value. > On output, the value actually set. >@param DataBits The number of data bits to use on the serial > device. A DataBits > -vaule of 0 will use the device's default data > bit setting. > +
