Re: [Patch 0/4] Normalize line endings to CRLF in ARM packages

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

Leif,

PatchCheck.py is complaining because lines in the file
header that contain the license text for some files are
using the wrong line endings.  The script I wrote preserves
the line ending style from the old license text when
adding the SPDX identifier so the script would not have
to figure out what line ending style is required.

Details are in the BZs I entered.

https://bugzilla.tianocore.org/show_bug.cgi?id=1659
https://bugzilla.tianocore.org/show_bug.cgi?id=1658
https://bugzilla.tianocore.org/show_bug.cgi?id=1657

Best regards,

Mike

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Thursday, April 4, 2019 8:25 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>;
> devel@edk2.groups.io; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Andrew Fish
> <afish@apple.com>
> Subject: Re: [Patch 0/4] Normalize line endings to CRLF
> in ARM packages
> 
> On Thu, Apr 04, 2019 at 05:00:36PM +0000, Kinney,
> Michael D wrote:
> > Leif,
> >
> > There were a few files when the license change
> patches
> > were created that failed PatchCheck.py.
> >
> > I am trying to resolve those before the license
> change
> > so the license change patches do not have any
> PatchCheck.py
> > failures.
> >
> > If I do these as part of the license change, then the
> > change will look larger than just the license change
> in
> > these files.
> >
> > I prefer to have a separate patch to resolve the line
> > endings.
> 
> Oh, it needs to be separate patches. Apologies if I was
> unclear on
> that. It should not even form part of the same set.
> 
> What I meant was that since a substantial bit of churn
> is about to hit
> the repository, we might as well go about doing the
> line ending
> conversion shortly before or shortly after the license
> change.
> 
> If we do it shortly before, we don't need *this* set.
> If we do it
> shortly after, we do.
> 
> > There may be other files in edk2 repo that have mixed
> > line endings in lines not associated with the license
> > change itself.  We can consider cleaning those up in
> > a separate series.
> >
> > The other option is to abandon these patches to
> normalize
> > line endings and allow the license change patches to
> not
> > pass PatchCheck.py.
> >
> > Which do you prefer?
> 
> I think it would be completely valid for the license
> change patches
> to not pass PatchCheck.py.
> 
> But why are they not passing? Is it complaining about
> context lines?
> 
> Regards,
> 
> Leif
> 
> > Thanks,
> >
> > Mike
> >
> >
> >
> > > -----Original Message-----
> > > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > > Sent: Thursday, April 4, 2019 3:39 AM
> > > To: Leif Lindholm <leif.lindholm@linaro.org>;
> Kinney,
> > > Michael D <michael.d.kinney@intel.com>
> > > Cc: devel@edk2.groups.io; Ard Biesheuvel
> > > <ard.biesheuvel@linaro.org>; Andrew Fish
> > > <afish@apple.com>
> > > Subject: Re: [Patch 0/4] Normalize line endings to
> CRLF
> > > in ARM packages
> > >
> > > On 04/04/19 05:54, Leif Lindholm wrote:
> > > > Hi Mike,
> > > >
> > > > This looks fine. But I did have one thought:
> > > > If we're just about to touch pretty much every
> file in
> > > the tree anyway
> > > > - can we consider doing the full conversion to
> native
> > > line endings at
> > > > the same time?
> > > >
> > > > (This doesn't help the 'git blame' problem, it
> just
> > > reminded me.)
> > >
> > > The git-blame issue that such a conversion
> introduces is
> > > not terrible.
> > > Let's say you run
> > >
> > >   git blame -- ArmPkg/Library/GccLto/liblto-arm.s
> > >
> > > and it gives you a commit for a specific line you
> care
> > > about. Let's call
> > > that commit C1.
> > >
> > > Then you do
> > >
> > >   git show --color C1 | less
> > >
> > > and expect to see the commit message & patch that
> > > introduced the *logic*
> > > on that line. But, you only see a "useless" CRLF
> > > conversion, in that
> > > commit. So what do you do? Simple, repeat the
> process as
> > > follows:
> > >
> > >   git blame C1^ -- ArmPkg/Library/GccLto/liblto-
> arm.s
> > >
> > > This will assign blame on the lines of the file as
> the
> > > file was right
> > > before C1 changed the line terminators.
> > >
> > >
> > > The problem that I think *is* worse is that,
> jumping
> > > about in the git
> > > history (with git checkout / git bisect), around
> the
> > > large conversion
> > > commit, will expose the user in a brief time to
> files
> > > that are both LF
> > > and CRLF encoded, internally. Therefore, whatever
> their
> > > local
> > > auto-conversion-on-checkout settings are, some
> commits
> > > in such a work
> > > session will not match those settings.
> > >
> > > For example, assuming you are on Windows, and set
> up
> > > LF-to-CRLF-on-checkout (as you should), then git-
> > > checkout a commit from
> > > before the "LF only" conversion commit, you might
> see
> > > double CRs. I
> > > guess. I never tested it.
> > >
> > > Thanks
> > > Laszlo
> > >
> > >
> > >
> > > >
> > > > /
> > > >     Leif
> > > >
> > > > On Wed, Apr 03, 2019 at 03:00:10PM -0700, Kinney,
> > > Michael D wrote:
> > > >>
> https://bugzilla.tianocore.org/show_bug.cgi?id=1659
> > > >>
> > > >> Normalize line endings to use CRLF to pass
> > > PatchCheck.py
> > > >>
> > > >> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > > >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > >> Cc: Laszlo Ersek <lersek@redhat.com>
> > > >> Contributed-under: TianoCore Contribution
> Agreement
> > > 1.1
> > > >> Signed-off-by: Michael D Kinney
> > > <michael.d.kinney@intel.com>
> > > >>
> > > >> Kinney, Michael D (4):
> > > >>   ArmPkg/ArmScmiDxe: Remove non-ASCII character
> in
> > > comment
> > > >>   ArmPkg: Normalize line endings to CRLF
> > > >>   ArmVirtPkg: Normalize line endings to CRLF
> > > >>   ArmPlatformPkg: Normalize line endings to CRLF
> > > >>
> > > >>  .../ArmScmiPerformanceProtocolPrivate.h       |
> 2
> > > +-
> > > >>  ArmPkg/Library/ArmSmcLibNull/ArmSmcLibNull.c  |
> 44
> > > +-
> > > >>  .../ArmSmcPsciResetSystemLib/AArch64/Reset.S  |
> 60
> > > +-
> > > >>  .../AArch64/Reset.asm                         |
> 70
> > > +-
> > > >>  .../ArmSmcPsciResetSystemLib/Arm/Reset.S      |
> 58
> > > +-
> > > >>  .../ArmSmcPsciResetSystemLib/Arm/Reset.asm    |
> 68
> > > +-
> > > >>  ArmPkg/Library/CompilerIntrinsicsLib/memset.c |
> 124
> > > ++--
> > > >>  ArmPkg/Library/GccLto/liblto-aarch64.s        |
> 54
> > > +-
> > > >>  ArmPkg/Library/GccLto/liblto-arm.s            |
> 122
> > > ++--
> > > >>  ArmPlatformPkg/Scripts/Ds5/profile.py         |
> 668
> > > +++++++++---------
> > > >>  ArmVirtPkg/Include/Platform/Hidden.h          |
> 56
> > > +-
> > > >>  11 files changed, 663 insertions(+), 663
> deletions(-
> > > )
> > > >>
> > > >> --
> > > >> 2.21.0.windows.1
> > > >>
> >