Re: [edk2] [PATCH v2] CryptoPkg: Fix various typos

[Wang, Jian J]

Reviewed-by: Jian J Wang 

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Antoine Coeur
> Sent: Friday, February 08, 2019 12:12 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH v2] CryptoPkg: Fix various typos
> 
> Fix various typos in CryptoPkg.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Coeur 
> ---
>  CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c | 2 +-
>  CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c
> b/CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c
> index d63c23df09..540c5715cb 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c
> @@ -142,7 +142,7 @@ IMPLEMENT_ASN1_FUNCTIONS (TS_TST_INFO)
>@param[in]  Asn1Time Pointer to the ASN.1 GeneralizedTime to be
> converted.
>@param[out] SigningTime  Return the corresponding EFI Time.
> 
> -  @retval  TRUE   The time convertion succeeds.
> +  @retval  TRUE   The time conversion succeeds.
>@retval  FALSE  Invalid parameters.
> 
>  **/
> diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> index 9510a4a383..80f5c2578e 100644
> --- a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> @@ -84,14 +84,14 @@ QuickSortWorker (
>  }
>}
>//
> -  // Swap pivot to it's final position (NextSwapLocaiton)
> +  // Swap pivot to its final position (NextSwapLocation)
>//
>CopyMem (Buffer, Pivot, ElementSize);
>CopyMem (Pivot, (UINT8 *)BufferToSort + (NextSwapLocation * ElementSize),
> ElementSize);
>CopyMem ((UINT8 *)BufferToSort + (NextSwapLocation * ElementSize), Buffer,
> ElementSize);
> 
>//
> -  // Now recurse on 2 paritial lists.  Neither of these will have the 'pivot'
> element.
> +  // Now recurse on 2 partial lists.  Neither of these will have the 'pivot' 
> element.
>// IE list is sorted left half, pivot element, sorted right half...
>//
>QuickSortWorker (
> --
> 2.17.2 (Apple Git-113)
> 
> ___
> 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] [edk2-announce] Community Meeting Minutes

[Laszlo Ersek]

On 02/08/19 18:33, Rebecca Cran wrote:
> 
> On February 8, 2019 at 2:01:59 AM, Laszlo Ersek 
> (ler...@redhat.com(mailto:ler...@redhat.com)) wrote:
> 
>> I don't see the workflow modification as viable. The "patch series"
>> concept is integral to every single open source project that I've ever
>> worked with. The evolution of a feature or a bug fix over a series of
>> patches is a core facet of programming and reviewing. It communicates a
>> thinking process, and that's what programming is about.  
> 
> I don’t recall coming across the patch series (e.g. the 1/5 email patches) in 
> other projects. In other projects people post a single patch and then update 
> it following feedback on the same review. This can be either in a single, 
> rebased commit, or new commits on a bug/feature branch - review systems deal 
> with both.

How do they contribute a feature consisting of 1500-3000 lines, in one
well-structured, coherent "package"? I don't think that any single patch
can carry that weight. Only a patch series can.

Regarding "new commits on a bug/feature branch" -- that really doesn't
look good to me, as a way to develop a focused, larger feature. Even if
the initial patch looks good (in separation), it cannot really be
evaluated without getting some kind of perspective, i.e. seeing where
the whole thing leads in mid-distance. Sometimes we find a design bug in
patch 08/12 that invalidates patch 03/12. I wouldn't want to push 03/12
until I review (and maybe even test / regression-test) the full dozen.

We need this to scale to 50+ patches in a series. Such a series is not
posted every day, but it does happen. That's when we need the tooling to
carry us the most.

[...]

Obviously: I welcome all comments on this, in disagreement too!

Thanks!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Hello Andrew query on BdsSetMemoryTypeInformationVariable

[Kinney, Michael D]

You can avoid the reboot on the first boot if the HOB
is initialized with sizes that match the usage on that
specific platform.

If you look at the boot logs, you will see messages that
show the memory type information table.  You can use that
information after a couple of boots to see the memory usage
and adjust the values used to produce the HOB in PEI.

Then rebuild with those new values and the extra reset on
first boot should go away.

The only time a reboot is required is if the memory usage
changes significantly.

Mike

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-
> boun...@lists.01.org] On Behalf Of Andrew Fish via
> edk2-devel
> Sent: Friday, February 8, 2019 9:36 AM
> To: galla rao ; edk2-
> de...@lists.01.org
> Subject: Re: [edk2] Hello Andrew query on
> BdsSetMemoryTypeInformationVariable
> 
> Forwarding to the edk2 list 
> 
> > On Feb 8, 2019, at 8:28 AM, galla rao
>  wrote:
> >
> > Hi Andrew,
> >
> > Am sorry for direct message!
> >
> > There is a function
> BdsSetMemoryTypeInformationVariable which causes a
> reset
> > when i enabled Secureboot related drivers
> >
> > Any clue why this function is added in EDK2?
> >
> 
> Yea it writes a variable that records how many pages of
> each memory type are used. This variable is read in PEI
> and used to pass a HOB into the DXE Core. The DXE Core
> uses these memory buckets to preallocate ranges for
> each memory type. This scheme prevents memory
> fragmentation and makes sure the runtime memory regions
> are in the same location when the system does an S4
> resume from disk.
> 
> 
> > is this a serious error, making the
> PcdResetOnMemoryTypeInformationChange to FALSE will
> resolve and boots to OS
> >
> 
> I think the idea behind that reboot, is the memory map
> could be different on the next boot and if that was an
> S4 the S4 could fail.
> 
> Thanks,
> 
> Andrew Fish
> 
> 
> > shed some knowledge if you are aware of this feature
> >
> > Thanks & Regards
> > Galla
> 
> ___
> 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] Self-replicating image

[Kinney, Michael D]

Hi Thomas,

I have been looking into this topic of multiple controllers this
week.  I have some prototype code that I think resolves the issues
but needs a bit more work on some corner cases.

I am using the PCI Option ROM use case to evaluate the changes.
PCI Option ROM can use Bus Specific Driver Override Protocol so
the driver from the option ROM manages the PCI controller the option
ROM is attached.  PCI Option ROMs can also use the Driver Family
Override Protocol so one of the PCI Option ROMs can manage a group
of PCI controllers.

It is also possible for an FMP driver for integrated devices to
manage multiple integrated devices if there is more than one of
the same device with FW storage.  The multiple controller use case
is not limited to busses like PCI.

The current version of the FmpDeviceLib is optimized for an FMP
instance that manages a single FW storage device.  If an FmpDeviceLib
is intended to manage multiple FW storage devices, then a few
extra services in the FmpDeviceLib are required.

The concept is to extend the FmpDeviceLib with a couple extra
APIs that add support for Stop()/Unload() operations and to
to set the context for the current FmpDeviceLib actions.

Have you entered a BZ for this issue yet?  I can start adding
details in the BZ and post some proposed changes soon.

Best regards,

Mike

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-
> boun...@lists.01.org] On Behalf Of Andrew Fish via
> edk2-devel
> Sent: Friday, February 8, 2019 9:16 AM
> To: Tomas Pilar (tpilar) 
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] Self-replicating image
> 
> 
> 
> > On Feb 8, 2019, at 6:42 AM, Tomas Pilar (tpilar)
>  wrote:
> >
> > Hi,
> >
> > I am currently pondering the most elegant way to
> implement capsule update for our devices that would
> work in the presence of multiple devices in the host.
> >
> > Capsule allows embedding a driver that is executed
> prior to the update, which is very handy. Crypto
> library is quite large and would not fit into an
> OptionROM, so being able to supply FMP driver in the
> capsule is great.
> >
> > However, if only one instance of the driver loads,
> the FMP upstream is currently written to support only
> one device per instance. So I wonder if there is a
> easy, neat way for my image to replicate on
> DriverBinding so that I end up with one instance per
> device.
> >
> 
> 
> Tom,
> 
> The usually model in EFI is to have one driver handle
> multiple things.
> 
> > It looks like I should be able to do it with gBS-
> >LoadImage() and passing information about currently
> loaded image though I might have to CopyMem() the image
> itself to new location.
> >
> 
> gBS->LoadImage() will load and relocate the image to a
> malloced address of the correct memory type for the
> image. The inputs are just the source of the image, so
> no need to CopyMem() anything. gBS->StartImage() calls
> the entry point.
> 
> Thanks,
> 
> Andrew Fish
> 
> > Thoughts?
> >
> > Cheers,
> > Tom
> > ___
> > 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [edk2-announce] Community Meeting Minutes

[Andrew Fish via edk2-devel]

> On Feb 8, 2019, at 9:33 AM, Rebecca Cran via edk2-devel 
>  wrote:
> 
> 
> On February 8, 2019 at 2:01:59 AM, Laszlo Ersek 
> (ler...@redhat.com(mailto:ler...@redhat.com)) wrote:
> 
>> I don't see the workflow modification as viable. The "patch series"
>> concept is integral to every single open source project that I've ever
>> worked with. The evolution of a feature or a bug fix over a series of
>> patches is a core facet of programming and reviewing. It communicates a
>> thinking process, and that's what programming is about.  
> 
> I don’t recall coming across the patch series (e.g. the 1/5 email patches) in 
> other projects. In other projects people post a single patch and then update 
> it following feedback on the same review. This can be either in a single, 
> rebased commit, or new commits on a bug/feature branch - review systems deal 
> with both.
> 

Rebecca,

I think the patch workflow is kind of like a coding standards. Some folks 
advocate for lots of small patches (common in open source projects), and some 
folks advocate for a patch per bug. I think the biggest upside to the patch 
granularity is it is much easier to bisect a failure. 

So I've used Bitbucket with a branch per commit (you name your branch with a 
standard pattern and the bugzilla  ) model and if your branch has a patch 
series (set of commits) you can view each commit independently from the UI and 
the default view is the entire patch series. So you can see both. 

Thanks,

Andrew Fish


>> So how long do we wait?
>> 
> 
> 
> Good point!  
> 
>> 
>> 
>> What I find practical at this moment is what Stephano has been working
>> on (thank you for all that Stephano) -- collect & file official
>> improvement requests with GitHub, and then see how those things are
>> addressed. In my opinion (not having seen Gerrit anyway, which remains
>> to be evaluated, but not by me), GitHub is the direct runner up to the
>> mailing list, so improving GitHub would be the most practical. In
>> particular I envision the context improvements for the GitHub email
>> notifications as something very doable for GitHub.  
> 
> 
> 
> 
> 
> I’d certainly be happy to use Github, but I do worry about tieing ourselves 
> to such a closed system.
> 
> 
> 
> 
> 
> 
> Rebecca
> 
> ___
> 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] [edk2-announce] Community Meeting Minutes

[Rebecca Cran via edk2-devel]

On February 8, 2019 at 2:01:59 AM, Laszlo Ersek 
(ler...@redhat.com(mailto:ler...@redhat.com)) wrote:

> I don't see the workflow modification as viable. The "patch series"
> concept is integral to every single open source project that I've ever
> worked with. The evolution of a feature or a bug fix over a series of
> patches is a core facet of programming and reviewing. It communicates a
> thinking process, and that's what programming is about.  

I don’t recall coming across the patch series (e.g. the 1/5 email patches) in 
other projects. In other projects people post a single patch and then update it 
following feedback on the same review. This can be either in a single, rebased 
commit, or new commits on a bug/feature branch - review systems deal with both.

> So how long do we wait?
>  


Good point!  

>  
>  
> What I find practical at this moment is what Stephano has been working
> on (thank you for all that Stephano) -- collect & file official
> improvement requests with GitHub, and then see how those things are
> addressed. In my opinion (not having seen Gerrit anyway, which remains
> to be evaluated, but not by me), GitHub is the direct runner up to the
> mailing list, so improving GitHub would be the most practical. In
> particular I envision the context improvements for the GitHub email
> notifications as something very doable for GitHub.  





I’d certainly be happy to use Github, but I do worry about tieing ourselves to 
such a closed system.






Rebecca

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Hello Andrew query on BdsSetMemoryTypeInformationVariable

[Andrew Fish via edk2-devel]

Forwarding to the edk2 list 

> On Feb 8, 2019, at 8:28 AM, galla rao  wrote:
> 
> Hi Andrew,
> 
> Am sorry for direct message!
>  
> There is a function BdsSetMemoryTypeInformationVariable which causes a reset 
> when i enabled Secureboot related drivers
> 
> Any clue why this function is added in EDK2?
> 

Yea it writes a variable that records how many pages of each memory type are 
used. This variable is read in PEI and used to pass a HOB into the DXE Core. 
The DXE Core uses these memory buckets to preallocate ranges for each memory 
type. This scheme prevents memory fragmentation and makes sure the runtime 
memory regions are in the same location when the system does an S4 resume from 
disk. 


> is this a serious error, making the PcdResetOnMemoryTypeInformationChange to 
> FALSE will resolve and boots to OS
> 

I think the idea behind that reboot, is the memory map could be different on 
the next boot and if that was an S4 the S4 could fail. 

Thanks,

Andrew Fish


> shed some knowledge if you are aware of this feature
> 
> Thanks & Regards
> Galla

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Self-replicating image

[Andrew Fish via edk2-devel]

> On Feb 8, 2019, at 6:42 AM, Tomas Pilar (tpilar)  
> wrote:
> 
> Hi,
> 
> I am currently pondering the most elegant way to implement capsule update for 
> our devices that would work in the presence of multiple devices in the host.
> 
> Capsule allows embedding a driver that is executed prior to the update, which 
> is very handy. Crypto library is quite large and would not fit into an 
> OptionROM, so being able to supply FMP driver in the capsule is great.
> 
> However, if only one instance of the driver loads, the FMP upstream is 
> currently written to support only one device per instance. So I wonder if 
> there is a easy, neat way for my image to replicate on DriverBinding so that 
> I end up with one instance per device.
> 


Tom,

The usually model in EFI is to have one driver handle multiple things. 

> It looks like I should be able to do it with gBS->LoadImage() and passing 
> information about currently loaded image though I might have to CopyMem() the 
> image itself to new location.
> 

gBS->LoadImage() will load and relocate the image to a malloced address of the 
correct memory type for the image. The inputs are just the source of the image, 
so no need to CopyMem() anything. gBS->StartImage() calls the entry point.

Thanks,

Andrew Fish

> Thoughts?
> 
> Cheers,
> Tom
> ___
> 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

[edk2] Self-replicating image

[Tomas Pilar (tpilar)]

Hi,

I am currently pondering the most elegant way to implement capsule update for 
our devices that would work in the presence of multiple devices in the host.

Capsule allows embedding a driver that is executed prior to the update, which 
is very handy. Crypto library is quite large and would not fit into an 
OptionROM, so being able to supply FMP driver in the capsule is great.

However, if only one instance of the driver loads, the FMP upstream is 
currently written to support only one device per instance. So I wonder if there 
is a easy, neat way for my image to replicate on DriverBinding so that I end up 
with one instance per device.

It looks like I should be able to do it with gBS->LoadImage() and passing 
information about currently loaded image though I might have to CopyMem() the 
image itself to new location.

Thoughts?

Cheers,
Tom
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [edk2-announce] Community Meeting Minutes

[Ard Biesheuvel]

On Thu, 7 Feb 2019 at 18:52, Jeremiah Cox via edk2-devel
 wrote:
>
> Apologies on the late reply, I was on vacation for several weeks and just got 
> back to this.
>
> Regarding "Patch Review System Evaluation", on the call, I disagreed with 
> your conclusion, but that note is not captured below.  My reading of the 
> email and call discussions, I did not hear our community reject GitHub, 
> rather observations that it was not "perfect", that it does not transparently 
> interact with folks who prefer mailing list patch systems, but it would be 
> acceptable to try.  On the call you mentioned a second justification for 
> staying with the mailing list system, that GitHub (really all modern patch 
> management systems) exclude folks who have limited internet connectivity.  I 
> hypothesize that this theoretical group of Tianocore contributors would be a 
> very small group of folks.  Should our community optimize our systems for a 
> very small, theoretical group, penalizing the overwhelming majority?  I would 
> propose that we try a modern patch management system, GitHub had the best 
> reviews in my reading, and folks with limited internet connectivity find a 
> friend to act as a go between translating their email diffs into GitHub PRs.

I find this unnecessarily condescending. Finding a friend, seriously?

Very serious concerns have been raised about the lack of transparency
with the various systems, and the fact that I am able to consult my
own local copy of the entire review history, including all email
exchanges is a very important aspect of the current model to me, as
opposed to GitHub deciding what is important enough to keep and what
is not. In an open source project, the code base is *not* the HEAD
commit, it is the entire repository, including history, and logged
email threads with technical discussions, since they are usually not
captured in other ways.

The push to GitHub is being sold to us as a way to attract more
contributors, but it seems to me (and I have stated this multiple
times) that the mailing list is not the steep part of the learning
curve when contributing to TianoCore. So is this really about getting
outsiders to contribute to Tianocore? Or is it about reducing the
impedance mismatch with what internal teams at MicroSoft (and Intel?)
are doing, which probably already went through the learning curve when
it comes to other aspects of Tianocore.

From a high level, it might seem that using a mailing list is the
impediment here. But in reality, contributing to open source in
general is not about whether you use GitHub or a mailing list to throw
your stuff over the fence. It is about collaborating with the
community to find common ground between the various sometimes
conflicting interests, and permitting your engineers to engage with
the community.

Tianocore has always been a rather peculiar open source project, since
it wasn't more than just that, i.e., openly available source code.
This has been changing for the better during the time I have been
involved, and we have worked very hard with the Intel firmware team
and other contributors to collaborate better on the mailing list.

To summarize my concern here: it seems that this push is not about
making it easier for contributors that already know how to do open
source collaboration to contribute to Tianocore, it is about making it
easier for currently closed code to be contributed to Tianocore by
teams who have no prior experience with open source.

Apologies if I have the wrong end of the stick here. If not, why don't
we consult a few casual contributors (which should be easy to find on
the mailing list) and ask them what their biggest issues were with
contributing to Tianocore?






>
> -Original Message-
> From: edk2-devel  On Behalf Of stephano
> Sent: Friday, January 11, 2019 11:27 AM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D ; Laszlo Ersek 
> 
> Subject: [edk2] [edk2-announce] Community Meeting Minutes
>
> An HTML version is available here:
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.tianocore.org%2Fminutes%2FCommunity-2019-01.htmldata=02%7C01%7Cjerecox%40microsoft.com%7C8998468d395f444243ed08d677fbe381%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636828321081803213sdata=QuHaAW3%2Fw3lPV8JnHskquCRJ6VlVCDNV2rptymjvCFY%3Dreserved=0
>
> Community Updates
> -
> Several conferences are coming up that we will be attending.
>
> FOSDEM 2019
> Stephano will be giving a talk with Alexander Graf (SUSE) on UEFI usage on 
> the UP Squared board and Beagle Bone Black.
>
> More info on FOSDEM here:
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Ffosdem.org%2F2019%2Fdata=02%7C01%7Cjerecox%40microsoft.com%7C8998468d395f444243ed08d677fbe381%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636828321081803213sdata=rECfPlMrOzcpi5GSCBEHUFmycKMA7gshN82bAPcXw0I%3Dreserved=0
>
> Info on the talk here:
> 

Re: [edk2] [edk2-announce] Community Meeting Minutes

[Laszlo Ersek]

On 02/08/19 07:41, Rebecca Cran wrote:
> On Thursday, 7 February 2019 11:30:38 MST stephano wrote:
> 
>> My apologies if I was not clear in the minutes. We are not rejecting 
>> Github, but rather taking time to evaluate how we can supplement 
>> Github's features to emulate our current patch review requirements. We 
>> do not want to rush into change and risk losing data or causing 
>> frustration for those developers currently contributing on a regular basis.
>>
>> I am currently working off this list of issues that Laszlo brought up:
>>
>> https://lists.01.org/pipermail/edk2-devel/2018-December/033509.html
>>
>> To be clear, Laszlo is not the only package maintainer that has voiced 
>> these concerns. The longevity of pull request branches and the fact that 
>> email notifications lack context are top on my list. There are several 
>> ways to overcome these obstacles, and finding the best solution will 
>> ensure that if we transition to Github, that transition is successful.
>>
>> The ability to allow developers to work offline (or with intermittent 
>> connections) is an important aspect as well. We cannot practice 
>> exclusionary or ostracizing behaviors if we expect to grow and maintain 
>> a community. I cannot imagine that Github has become as popular as it is 
>> if it cannot facilitate ease of offline use.
> 
> I wonder if Phabricator could be considered again, since I believe it 
> supports 
> all the features mentioned: the only thing it doesn't support as a 
> first-class 
> feature is mutli-patch reviews, which need to be done by linking separate 
> reviews together using the dependency feature. I wonder if it could either be 
> enhanced to support that, or people's workflow modified?

I don't see the workflow modification as viable. The "patch series"
concept is integral to every single open source project that I've ever
worked with. The evolution of a feature or a bug fix over a series of
patches is a core facet of programming and reviewing. It communicates a
thinking process, and that's what programming is about.

Enhancing Phabricator is of course an option, but I'm not sure how
practical that is. Then we start talking time frames and it becomes sort
of a competition. If GitLab added features of fixed the grave issues we
had found with it, we'd have to re-evaluate GitLab too. Or else, if we
had a completely leisurely time frame at our disposal, I would 100% vote
 -- see the introduction at .
 gets right *everything* right from the design principles; too
bad it's only Alpha at this point. So how long do we wait?

What I find practical at this moment is what Stephano has been working
on (thank you for all that Stephano) -- collect & file official
improvement requests with GitHub, and then see how those things are
addressed. In my opinion (not having seen Gerrit anyway, which remains
to be evaluated, but not by me), GitHub is the direct runner up to the
mailing list, so improving GitHub would be the most practical. In
particular I envision the context improvements for the GitHub email
notifications as something very doable for GitHub.

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

[Laszlo Ersek]

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