Re: [Patch 3/3] BaseTools/PatchCheck: Disable text conversion in 'git show'

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

Jordan,

I agree we should not adding PDFs or other binary files
to the edk2 or edk2-platforms repos.

The PDF was just an example, and it looks like git has some
auto conversion behavior in the show command and that could
impact binary file types other than pdf.  This patch only
removes the false positives that are difficult to understand
because I could not find the text it was complaining about.

I also want PatchCheck to be used to check the commit
Messages to repos like edk2-non-osi that does allow binaries.
So removing false positives is important for that use case.

I think we should address patches that add binaries as its
own topic and should be part of code review for maintainers
for repos that should not have binaries.

Thanks,

Mike

> -----Original Message-----
> From: Justen, Jordan L
> Sent: Friday, August 2, 2019 11:29 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> Cc: Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: Re: [Patch 3/3] BaseTools/PatchCheck: Disable
> text conversion in 'git show'
> 
> First, I hope we don't add lots of large .pdf files
> into the source tree. I see two duplicated > 200k .pdf
> files in edk2, which seems like a waste of space in the
> edk2 tree.
> 
> BaseTools/Source/C/BrotliCompress/docs/brotli-
> comparison-study-2015-09-22.pdf
> MdeModulePkg/Library/BrotliCustomDecompressLib/docs/bro
> tli-comparison-study-2015-09-22.pdf
> 
> Second, I'm not too sure about this change. It seems
> like it might have unintended consequences.
> 
> One thought I had is that it might break UTF-16 unicode
> files diffs, but luckily I think we've pretty much
> gotten rid of UTF-16 files at this point.
> 
> I also wonder if adding a .pdf attribute like Laszlo
> recommends for .efi might be an alternative solution.
> 
> https://github.com/tianocore/tianocore.github.io/wiki/L
> aszlo's-unkempt-git-guide-for-edk2-contributors-and-
> maintainers#contrib-09
> 
> We could actually add these setting in the git tree in
> a .gitattributes file, right? Has this been suggested?
> I think Laszlo documented this before we had converted
> edk2 to git.
> 
> -Jordan
> 
> On 2019-08-01 17:13:14, Michael D Kinney wrote:
> > https://bugzilla.tianocore.org/show_bug.cgi?id=2044
> >
> > 'git show' is used to extrat the patch contents for
> analysis.
> > Add the flag '--no-textconv' to the 'git show'
> command to disable the
> > conversion from some binary file types to text
> content.
> >
> > Without this change, binary files such as .pdf files
> are converted to
> > text in the show command and PatchCheck complains
> that the wrong line
> > endings are used in the patch.
> >
> > Cc: Bob Feng <bob.c.feng@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Signed-off-by: Michael D Kinney
> <michael.d.kinney@intel.com>
> > ---
> >  BaseTools/Scripts/PatchCheck.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/BaseTools/Scripts/PatchCheck.py
> > b/BaseTools/Scripts/PatchCheck.py index
> 6b07241bfe..2a4e6f603e 100755
> > --- a/BaseTools/Scripts/PatchCheck.py
> > +++ b/BaseTools/Scripts/PatchCheck.py
> > @@ -543,7 +543,7 @@ class CheckGitCommits:
> >
> >      def read_patch_from_git(self, commit):
> >          # Run git to get the commit patch
> > -        return self.run_git('show', '--
> pretty=email', commit)
> > +        return self.run_git('show', '--
> pretty=email',
> > + '--no-textconv', commit)
> >
> >      def run_git(self, *args):
> >          cmd = [ 'git' ]
> > --
> > 2.21.0.windows.1
> >