On 08/06/19 00:01, Kinney, Michael D wrote: > Laszlo, > > The context of this change is only to the PatchCheck.py tool. > and how that tool uses git show. > > I agree with the summary of very flexible capabilities in git > to help developers review different types of files. All of > those settings that were added to support UNI files in UTF-16 > file format were very valuable when we had to review the > text changes to those binary files. We should be using UTF-8 > now, and we can even extend PatchCheck.py to flag an error if > a UNI file is in UTF-16 format. > > I still prefer we make this change only to PatchCheck.py to > prevent false positives and print lines of text that can > not be found in a developer's working directory. I prefer > this one time change to PatchCheck.py instead of having to > update .gitattributes whenever the git show features are > extended to convert more binary files to text files.
More precisely, that would mean that you prefer this one time change to having to update .gitattributes every time a new type of binary file is introduced to edk2. I can see merits in both approaches. From my side, I don't intend to block this patch. Acked-by: Laszlo Ersek <ler...@redhat.com> Thanks Laszlo > My expectation is that EDK II Maintainers need to review > if a binary file is ok or not for a repo. I would also be > ok with adding general rules to PatchCheck.py to generate > an error if a binary file is added/updated in one of the > text only repos (edk2, edk2-platforms) and not generate > an error if a binary file is added/updated in a repo that > supports binaries (edk-non-osi). > > Best regards, > > Mike > > >> -----Original Message----- >> From: devel@edk2.groups.io >> [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek >> Sent: Friday, August 2, 2019 4:58 PM >> To: Justen, Jordan L <jordan.l.jus...@intel.com>; >> Kinney, Michael D <michael.d.kin...@intel.com>; >> devel@edk2.groups.io >> Cc: Feng, Bob C <bob.c.f...@intel.com>; Gao, Liming >> <liming....@intel.com> >> Subject: Re: [edk2-devel] [Patch 3/3] >> BaseTools/PatchCheck: Disable text conversion in 'git >> show' >> >> On 08/02/19 20:29, Jordan Justen wrote: >>> 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. >> >> This is a complex question. >> >> There are files which are binary for all intents and >> purposes, so you never want to see a "textualized" >> version of them. Git can attempt recognizing these >> files, but it can take hints too. >> >> For these files, once they are safely classified as >> "binary", it is a separate question whether you want a >> changeset shown (or >> email-formatted) for these files to contain base64-like >> binary patches, or just a statement that "binary files >> differ". >> >> Then there are files that are binary, but can be >> "textualized" for the purposes of diffing and even >> merging. Automated UCS2<->UTF8 conversion is an >> example, and I used that while edk2 still had UCS2- >> encoded UNI files. >> >> Some examples (not claiming this is going to be an >> exhaustive list -- for that, see gitattributes(5)), to >> be placed in .git/info/attributes (local clone only) or >> .gitattributes (tracked in the repo): >> >> *.efi -diff >> >> This classifies *.efi as binary (turns off the >> heuristics). >> >> Consider gitattributes(5) -- excerpt: >> >> diff >> Unset >> A path to which the diff attribute is >> unset will >> generate Binary files differ (or a >> binary patch, if >> binary patches are enabled). >> Unspecified >> A path to which the diff attribute is >> unspecified first >> gets its contents inspected, and if it >> looks like text >> and is smaller than >> core.bigFileThreshold, it is >> treated as text. Otherwise it would >> generate Binary >> files differ. >> >> git-show will only report "binary files differ", unless >> we pass --binary to it; in which case it will offer a >> base64-like binary patch. >> git-format-patch will produce that binary patch by >> default, unless we pass --no-binary to it. >> >> >> Another example, in .git/info/attributes or >> .gitattributes: >> >> *.uni diff=uni merge=uni >> >> The manual says: >> >> diff >> String >> Diff is shown using the specified diff >> driver. Each >> driver may specify one or more options, >> as described in >> the following section. The options for >> the diff driver >> "foo" are defined by the configuration >> variables in the >> "diff.foo" section of the Git config >> file. >> >> merge >> String >> 3-way merge is performed using the >> specified custom >> merge driver. The built-in 3-way merge >> driver can be >> explicitly specified by asking for >> "text" driver; the >> built-in "take the current branch" >> driver can be >> requested with "binary". >> >> Accordingly, from .git/config: >> >> [diff "uni"] >> textconv = iconv -f UCS-2 -t UTF-8 >> binary = true >> >> [merge "uni"] >> name = merge driver for UCS-2 encoded text files >> driver = uni-merge %O %A %B >> >> And my uni-merge script: >> >> --------- >> #!/bin/bash >> >> # Merge UCS-2 encoded text files. >> # This is a git-merge driver. It will overwrite >> "$MINE". >> # >> # Exit status: >> # 0 -- no conflicts >> # 1 -- conflicts >> # 2 -- trouble >> >> OLD="$1" >> MINE="$2" >> YOURS="$3" >> >> # Set safe defaults for the trap handler. >> OLD_UTF8= >> MY_UTF8= >> YOUR_UTF8= >> NEW_UTF8= >> EX=2 >> >> trap 'rm -f -- "$OLD_UTF8" "$MY_UTF8" "$YOUR_UTF8" >> "$NEW_UTF8"; exit $EX' EXIT set -e -u -C >> >> OLD_UTF8=$(mktemp) >> iconv --from UCS-2 --to UTF-8 <"$OLD" >>"$OLD_UTF8" >> >> MY_UTF8=$(mktemp) >> iconv --from UCS-2 --to UTF-8 <"$MINE" >>"$MY_UTF8" >> >> YOUR_UTF8=$(mktemp) >> iconv --from UCS-2 --to UTF-8 <"$YOURS" >>"$YOUR_UTF8" >> >> NEW_UTF8=$(mktemp) >> set +e >> diff3 --merge --label=mine --label=old --label=yours -- >> \ >> "$MY_UTF8" "$OLD_UTF8" "$YOUR_UTF8" >>"$NEW_UTF8" >> DIFFRET=$? >> set -e >> >> if [ 0 -ne "$DIFFRET" ] && [ 1 -ne "$DIFFRET" ]; then >> exit >> fi >> >> iconv --from UTF-8 --to UCS-2 <"$NEW_UTF8" >|"$MINE" >> EX=$DIFFRET >> --------- >> >> My understanding is that the "--no-textconv" option >> would undo the "textconv" that I set under [diff >> "uni"]. >> >> In other words, it tells git that, even if the >> heuristics suggested >> *some* conversion to text (I can't say what that would >> be, in the general case), git should still treat the >> file as binary. (At which point the --no-binary / -- >> binary options decide whether we get a base64-like >> encoding, or just a message saying "binary files >> differ"). >> Therefore I think (= agree with Jordan) that the effect >> is the same as marking the affected file types in >> .gitattributes as "-diff". >> >> I guess the relevant question is scope. "--no-textconv" >> doesn't require us to assign "-diff" to specific file >> suffixes, such as *.pdf, *.efi, and so on. But, it also >> doesn't allow us to specify *some* text conversions, >> even if we wanted to (it would override & disable them, >> such as my "iconv" conversion when diffing UCS2 encoded >> UNI files). >> >> Adding a .gitattributes file to the root of the >> repository, and updating it whenever necessary (for >> example when we add the first .pdf file, the first .efi >> file...), is likely the best. >> >> ( >> >> My recommendation at >> <https://github.com/tianocore/tianocore.github.io/wiki/ >> Laszlo's-unkempt-git-guide-for-edk2-contributors-and- >> maintainers#contrib-09> >> may have targeted the local ".git/info/attributes" file >> because people can have quite different opinions on how >> git should act on various files. In theory, someone >> having simplistic PDF files might even want: >> >> In the attributes: >> >> *.pdf diff=pdf >> >> In the config: >> >> [diff "pdf"] >> textconv = sh -c 'pdftotext -- "$1" -' pdftotext- >> wrapper >> binary = true >> >> ("pdftotext" converts PDF to text, but its command line >> is awkward -- the hyphen marking stdout must be >> explicit, and must come as second operand. Hence the >> wrapping into "sh" -- and then we need to spell out >> $0 as well, for which I picked "pdftotext-wrapper". The >> "--" separator is a safety measure against filenames >> starting with "-", which could be mis-interpreted as >> options. "--" terminates the option list and starts the >> operand list.) >> >> ) >> >> Thanks >> Laszlo >> >> >>> 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.f...@intel.com> >>>> Cc: Liming Gao <liming....@intel.com> >>>> Cc: Jordan Justen <jordan.l.jus...@intel.com> >>>> Signed-off-by: Michael D Kinney >> <michael.d.kin...@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 >>>> >> >> >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#45005): https://edk2.groups.io/g/devel/message/45005 Mute This Topic: https://groups.io/mt/32685494/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
