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

["Wang, Jian J" <jian.j.wang@intel.com>]

I agree that using smaller granularity patch is better for review. But, since this patch is
just trying to fix typos, separating the EFI_D_* into another patch looks good enough
to me. Personally I don't feel quite different if all typos are in one patch or more patches
or not. So I'll give r-b to v3 series (sorry for the late response).

Regards,
Jian

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, October 23, 2019 7:16 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io;
> Philippe Mathieu-Daudé <philmd@redhat.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_*
> 
> On 10/22/19 20:27, Kinney, Michael D wrote:
> > 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.
> 
> I missed that discussion (or, more likely, I must have skipped it in a
> hurry; sorry about that).
> 
> In retrospect, Phil's request makes sense, especially if a single EFI_D_
> to DEBUG_ change was hidden among hundreds of typo fixes.
> 
> > Would you prefer the first patch only change the one line
> > with the spelling error and not update the rest of the
> > package?
> 
> Yes, I think so. (Although, I certainly defer on this to the SecurityPkg
> maintainers, and Phil.)
> 
> > 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.
> 
> "BaseTools/Scripts/PatchCheck.py" checks for EFI_D_ with the regular
> expression in "old_debug_re".
> 
> However, "old_debug_re" is only referenced in the check_added_line()
> method. I think that's the right behavior: we shouldn't reject a patch
> just because it has EFI_D_ in context (= unchanged lines) or in removed
> lines. EFI_D_ is only wrong in lines that a patch is introducing anew.
> 
> Therefore, I don't think it's necessary to remove all EFI_D_ uses
> eventually. We only need to remove those uses whose lines we touch for
> another reason.
> 
> > 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.
> 
> I prefer option#1. I see value in a large audit mostly if the audit
> finds bugs -- semantic or actual (functional) bugs. EFI_D_* is a
> very-very small semantic issue and it doesn't seem to block or
> complicate anything; so fixing it doesn't justify the review cost, IMO.
> 
> $ git grep EFI_D_ -- OvmfPkg/ ArmVirtPkg/ | wc -l
> 444
> 
> *shudder*
> 
> Thanks!
> Laszlo
> 
> >> -----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
> >