[PATCH 1/1] BaseTools: convert diff.order to LF-only

[PATCH 1/1] BaseTools: convert diff.order to LF-only

[Leif Lindholm]

SetupGit.py sets the git config option diff.orderFile to
{edk2 directory}/BaseTools/Conf/diff.order, to override the default order
in which files are shown in a diff/patch/whatever. This is in imitation
of what is done manually in Laszlo's Unkempt Guide.

However, the version currently in the tree is in CRLF format, which makes
git interpret e.g. *.c as matching on *.c<CR>, finding no matches and
failing to apply the desired reordering. Note: this is true regardless of
whether running on Linux or Windows.

Convert the file to LF-only to make it work as expected.

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---

I'm not going to reveal just how much time I wasted on this before I
figured out what was going wrong...

I am intending to start prototyping the overall CRLF->native
conversion shortly, but this needs resolving regardless, and in fact we
will need to override the line ending conversion for this file in
gitattributes. 

Arguably, the same logic could be applied to the gitattributes file
itself (in the same directory), but since every effective line in that
has an explicit option following the glob, it triggers no issues at
present.

This bug is quite likely also behind some accusations I've made on
people not following the correct patch submission process, for which I
apologise.

Finally, a question: did we have some way of overriding the PatchCheck.py
step in mergify? This patch gets an error per line...
If not, should I submit a separate patch adding yet another exception to
PatchCheck.py?

/
    Leif

 BaseTools/Conf/diff.order | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/BaseTools/Conf/diff.order b/BaseTools/Conf/diff.order
index 4361817012c9..f1534f6c187c 100644
--- a/BaseTools/Conf/diff.order
+++ b/BaseTools/Conf/diff.order
@@ -1,13 +1,13 @@
-#
-# Copyright (c) 2019, Linaro Ltd. All rights reserved.
-#
-# SPDX-License-Identifier: BSD-2-Clause-Patent
-#
-*.dec
-*.dsc.inc
-*.dsc
-*.fdf
-*.inf
-*.h
-*.vfr
-*.c
+#
+# Copyright (c) 2019, Linaro Ltd. All rights reserved.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+*.dec
+*.dsc.inc
+*.dsc
+*.fdf
+*.inf
+*.h
+*.vfr
+*.c
-- 
2.20.1

Re: [PATCH 1/1] BaseTools: convert diff.order to LF-only

[Michael D Kinney]

Leif,

PatchCheck.py should only check added lines, not removed lines.

Are you seeing an error on a removed line?

Mike

> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Wednesday, April 22, 2020 8:47 AM
> To: devel@edk2.groups.io
> Cc: Andrew Fish <afish@apple.com>; Laszlo Ersek
> <lersek@redhat.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Feng, Bob C
> <bob.c.feng@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: [PATCH 1/1] BaseTools: convert diff.order to
> LF-only
> 
> SetupGit.py sets the git config option diff.orderFile
> to
> {edk2 directory}/BaseTools/Conf/diff.order, to override
> the default order
> in which files are shown in a diff/patch/whatever. This
> is in imitation
> of what is done manually in Laszlo's Unkempt Guide.
> 
> However, the version currently in the tree is in CRLF
> format, which makes
> git interpret e.g. *.c as matching on *.c<CR>, finding
> no matches and
> failing to apply the desired reordering. Note: this is
> true regardless of
> whether running on Linux or Windows.
> 
> Convert the file to LF-only to make it work as
> expected.
> 
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
> 
> I'm not going to reveal just how much time I wasted on
> this before I
> figured out what was going wrong...
> 
> I am intending to start prototyping the overall CRLF-
> >native
> conversion shortly, but this needs resolving
> regardless, and in fact we
> will need to override the line ending conversion for
> this file in
> gitattributes.
> 
> Arguably, the same logic could be applied to the
> gitattributes file
> itself (in the same directory), but since every
> effective line in that
> has an explicit option following the glob, it triggers
> no issues at
> present.
> 
> This bug is quite likely also behind some accusations
> I've made on
> people not following the correct patch submission
> process, for which I
> apologise.
> 
> Finally, a question: did we have some way of overriding
> the PatchCheck.py
> step in mergify? This patch gets an error per line...
> If not, should I submit a separate patch adding yet
> another exception to
> PatchCheck.py?
> 
> /
>     Leif
> 
>  BaseTools/Conf/diff.order | 26 +++++++++++++----------
> ---
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/BaseTools/Conf/diff.order
> b/BaseTools/Conf/diff.order
> index 4361817012c9..f1534f6c187c 100644
> --- a/BaseTools/Conf/diff.order
> +++ b/BaseTools/Conf/diff.order
> @@ -1,13 +1,13 @@
> -#
> -# Copyright (c) 2019, Linaro Ltd. All rights reserved.
> -#
> -# SPDX-License-Identifier: BSD-2-Clause-Patent
> -#
> -*.dec
> -*.dsc.inc
> -*.dsc
> -*.fdf
> -*.inf
> -*.h
> -*.vfr
> -*.c
> +#
> +# Copyright (c) 2019, Linaro Ltd. All rights reserved.
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +*.dec
> +*.dsc.inc
> +*.dsc
> +*.fdf
> +*.inf
> +*.h
> +*.vfr
> +*.c
> --
> 2.20.1

Re: [edk2-devel] [PATCH 1/1] BaseTools: convert diff.order to LF-only

[Leif Lindholm]

Hi Mike,

Well, it's a line-ending conversion: it deletes all lines, then it
adds them (this time without <CR>).

/
    Leif

On Wed, Apr 22, 2020 at 16:05:34 +0000, Michael D Kinney wrote:
> Leif,
> 
> PatchCheck.py should only check added lines, not removed lines.
> 
> Are you seeing an error on a removed line?
> 
> Mike
> 
> > -----Original Message-----
> > From: Leif Lindholm <leif@nuviainc.com>
> > Sent: Wednesday, April 22, 2020 8:47 AM
> > To: devel@edk2.groups.io
> > Cc: Andrew Fish <afish@apple.com>; Laszlo Ersek
> > <lersek@redhat.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Feng, Bob C
> > <bob.c.feng@intel.com>; Gao, Liming
> > <liming.gao@intel.com>
> > Subject: [PATCH 1/1] BaseTools: convert diff.order to
> > LF-only
> > 
> > SetupGit.py sets the git config option diff.orderFile
> > to
> > {edk2 directory}/BaseTools/Conf/diff.order, to override
> > the default order
> > in which files are shown in a diff/patch/whatever. This
> > is in imitation
> > of what is done manually in Laszlo's Unkempt Guide.
> > 
> > However, the version currently in the tree is in CRLF
> > format, which makes
> > git interpret e.g. *.c as matching on *.c<CR>, finding
> > no matches and
> > failing to apply the desired reordering. Note: this is
> > true regardless of
> > whether running on Linux or Windows.
> > 
> > Convert the file to LF-only to make it work as
> > expected.
> > 
> > Cc: Bob Feng <bob.c.feng@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > ---
> > 
> > I'm not going to reveal just how much time I wasted on
> > this before I
> > figured out what was going wrong...
> > 
> > I am intending to start prototyping the overall CRLF-
> > >native
> > conversion shortly, but this needs resolving
> > regardless, and in fact we
> > will need to override the line ending conversion for
> > this file in
> > gitattributes.
> > 
> > Arguably, the same logic could be applied to the
> > gitattributes file
> > itself (in the same directory), but since every
> > effective line in that
> > has an explicit option following the glob, it triggers
> > no issues at
> > present.
> > 
> > This bug is quite likely also behind some accusations
> > I've made on
> > people not following the correct patch submission
> > process, for which I
> > apologise.
> > 
> > Finally, a question: did we have some way of overriding
> > the PatchCheck.py
> > step in mergify? This patch gets an error per line...
> > If not, should I submit a separate patch adding yet
> > another exception to
> > PatchCheck.py?
> > 
> > /
> >     Leif
> > 
> >  BaseTools/Conf/diff.order | 26 +++++++++++++----------
> > ---
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> > 
> > diff --git a/BaseTools/Conf/diff.order
> > b/BaseTools/Conf/diff.order
> > index 4361817012c9..f1534f6c187c 100644
> > --- a/BaseTools/Conf/diff.order
> > +++ b/BaseTools/Conf/diff.order
> > @@ -1,13 +1,13 @@
> > -#
> > -# Copyright (c) 2019, Linaro Ltd. All rights reserved.
> > -#
> > -# SPDX-License-Identifier: BSD-2-Clause-Patent
> > -#
> > -*.dec
> > -*.dsc.inc
> > -*.dsc
> > -*.fdf
> > -*.inf
> > -*.h
> > -*.vfr
> > -*.c
> > +#
> > +# Copyright (c) 2019, Linaro Ltd. All rights reserved.
> > +#
> > +# SPDX-License-Identifier: BSD-2-Clause-Patent
> > +#
> > +*.dec
> > +*.dsc.inc
> > +*.dsc
> > +*.fdf
> > +*.inf
> > +*.h
> > +*.vfr
> > +*.c
> > --
> > 2.20.1
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 1/1] BaseTools: convert diff.order to LF-only

[Philippe Mathieu-Daudé]

On 4/22/20 5:46 PM, Leif Lindholm wrote:
> SetupGit.py sets the git config option diff.orderFile to
> {edk2 directory}/BaseTools/Conf/diff.order, to override the default order
> in which files are shown in a diff/patch/whatever. This is in imitation
> of what is done manually in Laszlo's Unkempt Guide.
> 
> However, the version currently in the tree is in CRLF format, which makes
> git interpret e.g. *.c as matching on *.c<CR>, finding no matches and
> failing to apply the desired reordering. Note: this is true regardless of
> whether running on Linux or Windows.
> 
> Convert the file to LF-only to make it work as expected.
> 
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
> 
> I'm not going to reveal just how much time I wasted on this before I
> figured out what was going wrong...

*sigh*

> 
> I am intending to start prototyping the overall CRLF->native
> conversion shortly, but this needs resolving regardless, and in fact we
> will need to override the line ending conversion for this file in
> gitattributes.
> 
> Arguably, the same logic could be applied to the gitattributes file
> itself (in the same directory), but since every effective line in that
> has an explicit option following the glob, it triggers no issues at
> present.
> 
> This bug is quite likely also behind some accusations I've made on
> people not following the correct patch submission process, for which I
> apologise.
> 
> Finally, a question: did we have some way of overriding the PatchCheck.py
> step in mergify? This patch gets an error per line...
> If not, should I submit a separate patch adding yet another exception to
> PatchCheck.py
> 
> /
>      Leif
> 
>   BaseTools/Conf/diff.order | 26 +++++++++++++-------------
>   1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/BaseTools/Conf/diff.order b/BaseTools/Conf/diff.order
> index 4361817012c9..f1534f6c187c 100644
> --- a/BaseTools/Conf/diff.order
> +++ b/BaseTools/Conf/diff.order
> @@ -1,13 +1,13 @@
> -#
> -# Copyright (c) 2019, Linaro Ltd. All rights reserved.
> -#
> -# SPDX-License-Identifier: BSD-2-Clause-Patent
> -#
> -*.dec
> -*.dsc.inc
> -*.dsc
> -*.fdf
> -*.inf
> -*.h
> -*.vfr
> -*.c
> +#
> +# Copyright (c) 2019, Linaro Ltd. All rights reserved.

Could be worth extending to 2020.

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +*.dec
> +*.dsc.inc
> +*.dsc
> +*.fdf
> +*.inf
> +*.h
> +*.vfr
> +*.c
> 

Re: [edk2-devel] [PATCH 1/1] BaseTools: convert diff.order to LF-only

[Leif Lindholm]

On Thu, Apr 23, 2020 at 13:01:01 +0200, Philippe Mathieu-Daudé wrote:
> On 4/22/20 5:46 PM, Leif Lindholm wrote:
> > SetupGit.py sets the git config option diff.orderFile to
> > {edk2 directory}/BaseTools/Conf/diff.order, to override the default order
> > in which files are shown in a diff/patch/whatever. This is in imitation
> > of what is done manually in Laszlo's Unkempt Guide.
> > 
> > However, the version currently in the tree is in CRLF format, which makes
> > git interpret e.g. *.c as matching on *.c<CR>, finding no matches and
> > failing to apply the desired reordering. Note: this is true regardless of
> > whether running on Linux or Windows.
> > 
> > Convert the file to LF-only to make it work as expected.
> > 
> > Cc: Bob Feng <bob.c.feng@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > ---
> > 
> > I'm not going to reveal just how much time I wasted on this before I
> > figured out what was going wrong...
> 
> *sigh*
> 
> > 
> > I am intending to start prototyping the overall CRLF->native
> > conversion shortly, but this needs resolving regardless, and in fact we
> > will need to override the line ending conversion for this file in
> > gitattributes.
> > 
> > Arguably, the same logic could be applied to the gitattributes file
> > itself (in the same directory), but since every effective line in that
> > has an explicit option following the glob, it triggers no issues at
> > present.
> > 
> > This bug is quite likely also behind some accusations I've made on
> > people not following the correct patch submission process, for which I
> > apologise.
> > 
> > Finally, a question: did we have some way of overriding the PatchCheck.py
> > step in mergify? This patch gets an error per line...
> > If not, should I submit a separate patch adding yet another exception to
> > PatchCheck.py
> > 
> > /
> >      Leif
> > 
> >   BaseTools/Conf/diff.order | 26 +++++++++++++-------------
> >   1 file changed, 13 insertions(+), 13 deletions(-)
> > 
> > diff --git a/BaseTools/Conf/diff.order b/BaseTools/Conf/diff.order
> > index 4361817012c9..f1534f6c187c 100644
> > --- a/BaseTools/Conf/diff.order
> > +++ b/BaseTools/Conf/diff.order
> > @@ -1,13 +1,13 @@
> > -#
> > -# Copyright (c) 2019, Linaro Ltd. All rights reserved.
> > -#
> > -# SPDX-License-Identifier: BSD-2-Clause-Patent
> > -#
> > -*.dec
> > -*.dsc.inc
> > -*.dsc
> > -*.fdf
> > -*.inf
> > -*.h
> > -*.vfr
> > -*.c
> > +#
> > +# Copyright (c) 2019, Linaro Ltd. All rights reserved.
> 
> Could be worth extending to 2020.

Well, if anything, I'd be adding a NUVIA entry :)
I did consider it, but couldn't quite bring myself to claiming
copyright for having executed dos2unix.

Especially as that would set a horrific precedent for the upcoming
global line ending conversion set...

> 
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Thanks!

/
    Leif

> 
> > +#
> > +# SPDX-License-Identifier: BSD-2-Clause-Patent
> > +#
> > +*.dec
> > +*.dsc.inc
> > +*.dsc
> > +*.fdf
> > +*.inf
> > +*.h
> > +*.vfr
> > +*.c
> > 
> 
> 
> 
> 

Re: [PATCH 1/1] BaseTools: convert diff.order to LF-only

[Laszlo Ersek]

On 04/22/20 17:46, Leif Lindholm wrote:
> SetupGit.py sets the git config option diff.orderFile to
> {edk2 directory}/BaseTools/Conf/diff.order, to override the default order
> in which files are shown in a diff/patch/whatever. This is in imitation
> of what is done manually in Laszlo's Unkempt Guide.
> 
> However, the version currently in the tree is in CRLF format, which makes
> git interpret e.g. *.c as matching on *.c<CR>, finding no matches and
> failing to apply the desired reordering. Note: this is true regardless of
> whether running on Linux or Windows.
> 
> Convert the file to LF-only to make it work as expected.
> 
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
> 
> I'm not going to reveal just how much time I wasted on this before I
> figured out what was going wrong...
> 
> I am intending to start prototyping the overall CRLF->native
> conversion shortly, but this needs resolving regardless, and in fact we
> will need to override the line ending conversion for this file in
> gitattributes. 
> 
> Arguably, the same logic could be applied to the gitattributes file
> itself (in the same directory), but since every effective line in that
> has an explicit option following the glob, it triggers no issues at
> present.
> 
> This bug is quite likely also behind some accusations I've made on
> people not following the correct patch submission process, for which I
> apologise.
> 
> Finally, a question: did we have some way of overriding the PatchCheck.py
> step in mergify? This patch gets an error per line...
> If not, should I submit a separate patch adding yet another exception to
> PatchCheck.py
> 
> /
>     Leif
> 
>  BaseTools/Conf/diff.order | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/BaseTools/Conf/diff.order b/BaseTools/Conf/diff.order
> index 4361817012c9..f1534f6c187c 100644
> --- a/BaseTools/Conf/diff.order
> +++ b/BaseTools/Conf/diff.order
> @@ -1,13 +1,13 @@
> -#
> -# Copyright (c) 2019, Linaro Ltd. All rights reserved.
> -#
> -# SPDX-License-Identifier: BSD-2-Clause-Patent
> -#
> -*.dec
> -*.dsc.inc
> -*.dsc
> -*.fdf
> -*.inf
> -*.h
> -*.vfr
> -*.c
> +#
> +# Copyright (c) 2019, Linaro Ltd. All rights reserved.
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +*.dec
> +*.dsc.inc
> +*.dsc
> +*.fdf
> +*.inf
> +*.h
> +*.vfr
> +*.c
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

(I must admit that I use my multi-project shared order file, configured
in my ~/.gitconfig file.)

Thanks
Laszlo

Re: [edk2-devel] [PATCH 1/1] BaseTools: convert diff.order to LF-only

[Liming Gao]

Leif:
  I am OK for this change. Reviewed-by: Liming Gao <liming.gao@intel.com>

  As you say, this patch doesn't pass patchcheck. I suggest PatchCheck CI pipe line can support the exception.

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Friday, April 24, 2020 9:30 PM
> To: Leif Lindholm <leif@nuviainc.com>; devel@edk2.groups.io
> Cc: Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Gao,
> Liming <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/1] BaseTools: convert diff.order to LF-only
> 
> On 04/22/20 17:46, Leif Lindholm wrote:
> > SetupGit.py sets the git config option diff.orderFile to
> > {edk2 directory}/BaseTools/Conf/diff.order, to override the default order
> > in which files are shown in a diff/patch/whatever. This is in imitation
> > of what is done manually in Laszlo's Unkempt Guide.
> >
> > However, the version currently in the tree is in CRLF format, which makes
> > git interpret e.g. *.c as matching on *.c<CR>, finding no matches and
> > failing to apply the desired reordering. Note: this is true regardless of
> > whether running on Linux or Windows.
> >
> > Convert the file to LF-only to make it work as expected.
> >
> > Cc: Bob Feng <bob.c.feng@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > ---
> >
> > I'm not going to reveal just how much time I wasted on this before I
> > figured out what was going wrong...
> >
> > I am intending to start prototyping the overall CRLF->native
> > conversion shortly, but this needs resolving regardless, and in fact we
> > will need to override the line ending conversion for this file in
> > gitattributes.
> >
> > Arguably, the same logic could be applied to the gitattributes file
> > itself (in the same directory), but since every effective line in that
> > has an explicit option following the glob, it triggers no issues at
> > present.
> >
> > This bug is quite likely also behind some accusations I've made on
> > people not following the correct patch submission process, for which I
> > apologise.
> >
> > Finally, a question: did we have some way of overriding the PatchCheck.py
> > step in mergify? This patch gets an error per line...
> > If not, should I submit a separate patch adding yet another exception to
> > PatchCheck.py
> >
> > /
> >     Leif
> >
> >  BaseTools/Conf/diff.order | 26 +++++++++++++-------------
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/BaseTools/Conf/diff.order b/BaseTools/Conf/diff.order
> > index 4361817012c9..f1534f6c187c 100644
> > --- a/BaseTools/Conf/diff.order
> > +++ b/BaseTools/Conf/diff.order
> > @@ -1,13 +1,13 @@
> > -#
> > -# Copyright (c) 2019, Linaro Ltd. All rights reserved.
> > -#
> > -# SPDX-License-Identifier: BSD-2-Clause-Patent
> > -#
> > -*.dec
> > -*.dsc.inc
> > -*.dsc
> > -*.fdf
> > -*.inf
> > -*.h
> > -*.vfr
> > -*.c
> > +#
> > +# Copyright (c) 2019, Linaro Ltd. All rights reserved.
> > +#
> > +# SPDX-License-Identifier: BSD-2-Clause-Patent
> > +#
> > +*.dec
> > +*.dsc.inc
> > +*.dsc
> > +*.fdf
> > +*.inf
> > +*.h
> > +*.vfr
> > +*.c
> >
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> (I must admit that I use my multi-project shared order file, configured
> in my ~/.gitconfig file.)
> 
> Thanks
> Laszlo
> 
> 
> 

Re: [edk2-devel] [PATCH 1/1] BaseTools: convert diff.order to LF-only

[Michael D Kinney]

Liming,

Should we update PatchCheck.py to allow LF only for files with
a diff.order extension?  We already have exceptions for .sh and
.gitmodules files.

                if self.filename.endswith('.sh'):
                    #
                    # Do not enforce CR/LF line endings for linux shell scripts.
                    #
                    self.force_crlf = False
                if self.filename == '.gitmodules':
                    #
                    # .gitmodules is updated by git and uses tabs and LF line
                    # endings.  Do not enforce no tabs and do not enforce
                    # CR/LF line endings.
                    #
                    self.force_crlf = False
                    self.force_notabs = False


Mike

> -----Original Message-----
> From: Gao, Liming <liming.gao@intel.com>
> Sent: Monday, April 27, 2020 8:45 AM
> To: devel@edk2.groups.io; lersek@redhat.com; Leif
> Lindholm <leif@nuviainc.com>
> Cc: Andrew Fish <afish@apple.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Feng, Bob C
> <bob.c.feng@intel.com>; Sean Brogan
> <sean.brogan@microsoft.com>
> Subject: RE: [edk2-devel] [PATCH 1/1] BaseTools:
> convert diff.order to LF-only
> 
> Leif:
>   I am OK for this change. Reviewed-by: Liming Gao
> <liming.gao@intel.com>
> 
>   As you say, this patch doesn't pass patchcheck. I
> suggest PatchCheck CI pipe line can support the
> exception.
> 
> Thanks
> Liming
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Laszlo Ersek
> > Sent: Friday, April 24, 2020 9:30 PM
> > To: Leif Lindholm <leif@nuviainc.com>;
> devel@edk2.groups.io
> > Cc: Andrew Fish <afish@apple.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Feng, Bob C
> <bob.c.feng@intel.com>; Gao,
> > Liming <liming.gao@intel.com>
> > Subject: Re: [edk2-devel] [PATCH 1/1] BaseTools:
> convert diff.order to LF-only
> >
> > On 04/22/20 17:46, Leif Lindholm wrote:
> > > SetupGit.py sets the git config option
> diff.orderFile to
> > > {edk2 directory}/BaseTools/Conf/diff.order, to
> override the default order
> > > in which files are shown in a diff/patch/whatever.
> This is in imitation
> > > of what is done manually in Laszlo's Unkempt Guide.
> > >
> > > However, the version currently in the tree is in
> CRLF format, which makes
> > > git interpret e.g. *.c as matching on *.c<CR>,
> finding no matches and
> > > failing to apply the desired reordering. Note: this
> is true regardless of
> > > whether running on Linux or Windows.
> > >
> > > Convert the file to LF-only to make it work as
> expected.
> > >
> > > Cc: Bob Feng <bob.c.feng@intel.com>
> > > Cc: Liming Gao <liming.gao@intel.com>
> > > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > > ---
> > >
> > > I'm not going to reveal just how much time I wasted
> on this before I
> > > figured out what was going wrong...
> > >
> > > I am intending to start prototyping the overall
> CRLF->native
> > > conversion shortly, but this needs resolving
> regardless, and in fact we
> > > will need to override the line ending conversion
> for this file in
> > > gitattributes.
> > >
> > > Arguably, the same logic could be applied to the
> gitattributes file
> > > itself (in the same directory), but since every
> effective line in that
> > > has an explicit option following the glob, it
> triggers no issues at
> > > present.
> > >
> > > This bug is quite likely also behind some
> accusations I've made on
> > > people not following the correct patch submission
> process, for which I
> > > apologise.
> > >
> > > Finally, a question: did we have some way of
> overriding the PatchCheck.py
> > > step in mergify? This patch gets an error per
> line...
> > > If not, should I submit a separate patch adding yet
> another exception to
> > > PatchCheck.py
> > >
> > > /
> > >     Leif
> > >
> > >  BaseTools/Conf/diff.order | 26 +++++++++++++------
> -------
> > >  1 file changed, 13 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/BaseTools/Conf/diff.order
> b/BaseTools/Conf/diff.order
> > > index 4361817012c9..f1534f6c187c 100644
> > > --- a/BaseTools/Conf/diff.order
> > > +++ b/BaseTools/Conf/diff.order
> > > @@ -1,13 +1,13 @@
> > > -#
> > > -# Copyright (c) 2019, Linaro Ltd. All rights
> reserved.
> > > -#
> > > -# SPDX-License-Identifier: BSD-2-Clause-Patent
> > > -#
> > > -*.dec
> > > -*.dsc.inc
> > > -*.dsc
> > > -*.fdf
> > > -*.inf
> > > -*.h
> > > -*.vfr
> > > -*.c
> > > +#
> > > +# Copyright (c) 2019, Linaro Ltd. All rights
> reserved.
> > > +#
> > > +# SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +#
> > > +*.dec
> > > +*.dsc.inc
> > > +*.dsc
> > > +*.fdf
> > > +*.inf
> > > +*.h
> > > +*.vfr
> > > +*.c
> > >
> >
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> >
> > (I must admit that I use my multi-project shared
> order file, configured
> > in my ~/.gitconfig file.)
> >
> > Thanks
> > Laszlo
> >
> >
> > 

Re: [edk2-devel] [PATCH 1/1] BaseTools: convert diff.order to LF-only

[Leif Lindholm]

Hi Mike,

I was thinking (for post-conversion) we may end up needing something
like
  .git*  text eol=lf
in our gitattributes file (which itself unfortunately currently does
not follow that format).

If we were to rename BaseTools/Conf/diff.order and
BaseTools/Conf/gitattributes (to say .git-difforder and
.gitattributes) and update SetupGit.py to use the new names, that
would let us cover these (and future ones) with a single test.

/
    Leif

On Mon, Apr 27, 2020 at 16:22:11 +0000, Kinney, Michael D wrote:
> Liming,
> 
> Should we update PatchCheck.py to allow LF only for files with
> a diff.order extension?  We already have exceptions for .sh and
> .gitmodules files.
> 
>                 if self.filename.endswith('.sh'):
>                     #
>                     # Do not enforce CR/LF line endings for linux shell scripts.
>                     #
>                     self.force_crlf = False
>                 if self.filename == '.gitmodules':
>                     #
>                     # .gitmodules is updated by git and uses tabs and LF line
>                     # endings.  Do not enforce no tabs and do not enforce
>                     # CR/LF line endings.
>                     #
>                     self.force_crlf = False
>                     self.force_notabs = False
> 
> 
> Mike
> 
> > -----Original Message-----
> > From: Gao, Liming <liming.gao@intel.com>
> > Sent: Monday, April 27, 2020 8:45 AM
> > To: devel@edk2.groups.io; lersek@redhat.com; Leif
> > Lindholm <leif@nuviainc.com>
> > Cc: Andrew Fish <afish@apple.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Feng, Bob C
> > <bob.c.feng@intel.com>; Sean Brogan
> > <sean.brogan@microsoft.com>
> > Subject: RE: [edk2-devel] [PATCH 1/1] BaseTools:
> > convert diff.order to LF-only
> > 
> > Leif:
> >   I am OK for this change. Reviewed-by: Liming Gao
> > <liming.gao@intel.com>
> > 
> >   As you say, this patch doesn't pass patchcheck. I
> > suggest PatchCheck CI pipe line can support the
> > exception.
> > 
> > Thanks
> > Liming
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On
> > Behalf Of Laszlo Ersek
> > > Sent: Friday, April 24, 2020 9:30 PM
> > > To: Leif Lindholm <leif@nuviainc.com>;
> > devel@edk2.groups.io
> > > Cc: Andrew Fish <afish@apple.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Feng, Bob C
> > <bob.c.feng@intel.com>; Gao,
> > > Liming <liming.gao@intel.com>
> > > Subject: Re: [edk2-devel] [PATCH 1/1] BaseTools:
> > convert diff.order to LF-only
> > >
> > > On 04/22/20 17:46, Leif Lindholm wrote:
> > > > SetupGit.py sets the git config option
> > diff.orderFile to
> > > > {edk2 directory}/BaseTools/Conf/diff.order, to
> > override the default order
> > > > in which files are shown in a diff/patch/whatever.
> > This is in imitation
> > > > of what is done manually in Laszlo's Unkempt Guide.
> > > >
> > > > However, the version currently in the tree is in
> > CRLF format, which makes
> > > > git interpret e.g. *.c as matching on *.c<CR>,
> > finding no matches and
> > > > failing to apply the desired reordering. Note: this
> > is true regardless of
> > > > whether running on Linux or Windows.
> > > >
> > > > Convert the file to LF-only to make it work as
> > expected.
> > > >
> > > > Cc: Bob Feng <bob.c.feng@intel.com>
> > > > Cc: Liming Gao <liming.gao@intel.com>
> > > > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > > > ---
> > > >
> > > > I'm not going to reveal just how much time I wasted
> > on this before I
> > > > figured out what was going wrong...
> > > >
> > > > I am intending to start prototyping the overall
> > CRLF->native
> > > > conversion shortly, but this needs resolving
> > regardless, and in fact we
> > > > will need to override the line ending conversion
> > for this file in
> > > > gitattributes.
> > > >
> > > > Arguably, the same logic could be applied to the
> > gitattributes file
> > > > itself (in the same directory), but since every
> > effective line in that
> > > > has an explicit option following the glob, it
> > triggers no issues at
> > > > present.
> > > >
> > > > This bug is quite likely also behind some
> > accusations I've made on
> > > > people not following the correct patch submission
> > process, for which I
> > > > apologise.
> > > >
> > > > Finally, a question: did we have some way of
> > overriding the PatchCheck.py
> > > > step in mergify? This patch gets an error per
> > line...
> > > > If not, should I submit a separate patch adding yet
> > another exception to
> > > > PatchCheck.py
> > > >
> > > > /
> > > >     Leif
> > > >
> > > >  BaseTools/Conf/diff.order | 26 +++++++++++++------
> > -------
> > > >  1 file changed, 13 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/BaseTools/Conf/diff.order
> > b/BaseTools/Conf/diff.order
> > > > index 4361817012c9..f1534f6c187c 100644
> > > > --- a/BaseTools/Conf/diff.order
> > > > +++ b/BaseTools/Conf/diff.order
> > > > @@ -1,13 +1,13 @@
> > > > -#
> > > > -# Copyright (c) 2019, Linaro Ltd. All rights
> > reserved.
> > > > -#
> > > > -# SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > -#
> > > > -*.dec
> > > > -*.dsc.inc
> > > > -*.dsc
> > > > -*.fdf
> > > > -*.inf
> > > > -*.h
> > > > -*.vfr
> > > > -*.c
> > > > +#
> > > > +# Copyright (c) 2019, Linaro Ltd. All rights
> > reserved.
> > > > +#
> > > > +# SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > +#
> > > > +*.dec
> > > > +*.dsc.inc
> > > > +*.dsc
> > > > +*.fdf
> > > > +*.inf
> > > > +*.h
> > > > +*.vfr
> > > > +*.c
> > > >
> > >
> > > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > >
> > > (I must admit that I use my multi-project shared
> > order file, configured
> > > in my ~/.gitconfig file.)
> > >
> > > Thanks
> > > Laszlo
> > >
> > >
> > > 
> 

Re: [edk2-devel] [PATCH 1/1] BaseTools: convert diff.order to LF-only

[Leif Lindholm]

Hi Liming,

Thanks!
I'm sorry but I don't know what "PatchCheck CI pipe line can support
the exception." means. I did ask before if there was a way to
bypass that particular test.

Anyway, I created
https://github.com/tianocore/edk2/pull/549
Grateful for any help.

Regards,

LEif

On Mon, Apr 27, 2020 at 15:44:48 +0000, Gao, Liming wrote:
> Leif:
>   I am OK for this change. Reviewed-by: Liming Gao <liming.gao@intel.com>
> 
>   As you say, this patch doesn't pass patchcheck. I suggest PatchCheck CI pipe line can support the exception.
> 
> Thanks
> Liming
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> > Sent: Friday, April 24, 2020 9:30 PM
> > To: Leif Lindholm <leif@nuviainc.com>; devel@edk2.groups.io
> > Cc: Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Gao,
> > Liming <liming.gao@intel.com>
> > Subject: Re: [edk2-devel] [PATCH 1/1] BaseTools: convert diff.order to LF-only
> > 
> > On 04/22/20 17:46, Leif Lindholm wrote:
> > > SetupGit.py sets the git config option diff.orderFile to
> > > {edk2 directory}/BaseTools/Conf/diff.order, to override the default order
> > > in which files are shown in a diff/patch/whatever. This is in imitation
> > > of what is done manually in Laszlo's Unkempt Guide.
> > >
> > > However, the version currently in the tree is in CRLF format, which makes
> > > git interpret e.g. *.c as matching on *.c<CR>, finding no matches and
> > > failing to apply the desired reordering. Note: this is true regardless of
> > > whether running on Linux or Windows.
> > >
> > > Convert the file to LF-only to make it work as expected.
> > >
> > > Cc: Bob Feng <bob.c.feng@intel.com>
> > > Cc: Liming Gao <liming.gao@intel.com>
> > > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > > ---
> > >
> > > I'm not going to reveal just how much time I wasted on this before I
> > > figured out what was going wrong...
> > >
> > > I am intending to start prototyping the overall CRLF->native
> > > conversion shortly, but this needs resolving regardless, and in fact we
> > > will need to override the line ending conversion for this file in
> > > gitattributes.
> > >
> > > Arguably, the same logic could be applied to the gitattributes file
> > > itself (in the same directory), but since every effective line in that
> > > has an explicit option following the glob, it triggers no issues at
> > > present.
> > >
> > > This bug is quite likely also behind some accusations I've made on
> > > people not following the correct patch submission process, for which I
> > > apologise.
> > >
> > > Finally, a question: did we have some way of overriding the PatchCheck.py
> > > step in mergify? This patch gets an error per line...
> > > If not, should I submit a separate patch adding yet another exception to
> > > PatchCheck.py
> > >
> > > /
> > >     Leif
> > >
> > >  BaseTools/Conf/diff.order | 26 +++++++++++++-------------
> > >  1 file changed, 13 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/BaseTools/Conf/diff.order b/BaseTools/Conf/diff.order
> > > index 4361817012c9..f1534f6c187c 100644
> > > --- a/BaseTools/Conf/diff.order
> > > +++ b/BaseTools/Conf/diff.order
> > > @@ -1,13 +1,13 @@
> > > -#
> > > -# Copyright (c) 2019, Linaro Ltd. All rights reserved.
> > > -#
> > > -# SPDX-License-Identifier: BSD-2-Clause-Patent
> > > -#
> > > -*.dec
> > > -*.dsc.inc
> > > -*.dsc
> > > -*.fdf
> > > -*.inf
> > > -*.h
> > > -*.vfr
> > > -*.c
> > > +#
> > > +# Copyright (c) 2019, Linaro Ltd. All rights reserved.
> > > +#
> > > +# SPDX-License-Identifier: BSD-2-Clause-Patent
> > > +#
> > > +*.dec
> > > +*.dsc.inc
> > > +*.dsc
> > > +*.fdf
> > > +*.inf
> > > +*.h
> > > +*.vfr
> > > +*.c
> > >
> > 
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > 
> > (I must admit that I use my multi-project shared order file, configured
> > in my ~/.gitconfig file.)
> > 
> > Thanks
> > Laszlo
> > 
> > 
> > 
> 

Re: [edk2-devel] [PATCH 1/1] BaseTools: convert diff.order to LF-only

[Laszlo Ersek]

On 04/27/20 18:53, Leif Lindholm wrote:
> Hi Liming,
> 
> Thanks!
> I'm sorry but I don't know what "PatchCheck CI pipe line can support
> the exception." means. I did ask before if there was a way to
> bypass that particular test.
> 
> Anyway, I created
> https://github.com/tianocore/edk2/pull/549
> Grateful for any help.

Probably not the "help" you're looking for, but can we (re)consider this
a git bug?

For example, git copes with CRLFs in ".gitignore" just fine. If we
remove the CRs from "BaseTools/Conf/diff.order", Windows-based
contributors will have difficulty editing it.

NB I'm still OK with this patch, if we can get it merged, somehow.

Thanks
Laszlo