Re: [edk2-devel] [Patch v2 1/2] SecurityPkg: Replace EFI_D_* with DEBUG_*

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Laszlo,

I agree with the challenges in reviewing these types of
code changes.  The spelling errors in comments are easier
to review because we know if it is in a comment there is
no change to code functionality.

The original patch only changed a single EFI_D_ to DEBUG_
for the one DEBUG() statement that had a spelling error.

Philippe requested that be split out into its own patch.

Would you prefer the first patch only change the one line
with the spelling error and not update the rest of the
package?

The source of this bug is for CI checks being enabled
in edk2-staging/edk2-ci.  Since we are using PatchCheck.py
as one of the checks, any updates to any package where
the patch file includes a line with EFI_D_* will fail,
so all packages will need to do the conversion as some
point.  We need to decide if this will be done as needed
only to lines in affected patches, or if we want to do it
to whole packages so everything is cleaned up.

Thanks,

Mike


> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, October 22, 2019 11:06 AM
> To: devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Zhang, Chao B
> <chao.b.zhang@intel.com>
> Subject: Re: [edk2-devel] [Patch v2 1/2] SecurityPkg:
> Replace EFI_D_* with DEBUG_*
> 
> Hi Mike,
> 
> On 10/22/19 19:37, Michael D Kinney wrote:
> > Update all DEBUG() macros in the SecurityPkg to use
> DEBUG_ instead of
> > EFI_D_.  This is required to pass PatchCheck.py
> checks.
> 
> [...]
> 
> >  45 files changed, 410 insertions(+), 410 deletions(-
> )
> 
> (
> 
> If the SecurityPkg maintainers are happy with this
> patch, then it's not my place to complain.
> 
> I'd just like to point out that I'd object to such a
> patch for OvmfPkg.
> Such sweeping conversions are difficult to review (they
> are also difficult to implement -- I think mass
> search&replace is not too safe without human review).
> 
> New code should not add EFI_D_* usage, of course.
> 
> I'd expect PatchCheck.py to complain about EFI_D_* only
> on lines that are added by a patch, not on lines being
> removed, or present in the context. Is that not the
> case?
> 
> ... Hm, looking at patch#2, it seems that some spelling
> errors affect debug messages. Therefore, some of the
> typo fixes do turn EFI_D_* macros into new lines. Given
> that there is a huge number of typo fixes (205 lines,
> apparently), I guess it makes sense to separate out the
> EFI_D_* conversion. It's up to the package owners
> whether they prefer reviewing
> - 410 lines of EFI_D_* massaging, plus 205 lines of
> typo fixes,
> - or 205 lines of { EFI_D_* conversion, plus typo fix
> }.
> 
> For OvmfPkg, my choice would likely be (assuming such a
> large diffstat):
> - fix EFI_D_*, one patch per module, and only on lines
> affected by typos,
> - fix typos, one patch per module.
> 
> I could suspend and resume a review like that more
> easily.
> 
> )
> 
> Thanks
> Laszlo