On Tue, Nov 02 2021, Johannes Schindelin wrote:
> Hi Taylor,
>
> On Tue, 2 Nov 2021, Taylor Blau wrote:
>
>> On Tue, Nov 02, 2021 at 04:01:57PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> > There's a parallel discussion about doing something to detect this in
>> > "git am", which for the git project seems like a better place to put
>> > this.
>>
>> I don't think that one impacts the other necessarily. Having `git am`
>> guard against this would probably be sufficient to protect Junio
>> accidentally apply something containing directional formatting to his
>> tree unknowingly.
>>
>> But the idea that we rely on the import mechanism to protect against
>> this doesn't sit well with me. Ultimately, we should be relying on a
>> static check like below to ensure that directional formatting hasn't
>> entered the tree by any mechanism (not just 'git am').
>
> Yep, the `git am` change and the CI change are addressing two very
> different concerns.
>
>> > > diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
>> > > index 6ed6a9e8076..7b4b4df03c3 100644
>> > > --- a/.github/workflows/main.yml
>> > > +++ b/.github/workflows/main.yml
>> > > @@ -289,6 +289,13 @@ jobs:
>> > > - uses: actions/checkout@v2
>> > > - run: ci/install-dependencies.sh
>> > > - run: ci/run-static-analysis.sh
>> > > + - name: disallow Unicode directional formatting
>> > > + run: |
>> > > + # Use UTF-8-aware `printf` to feed a byte pattern to non-UTF-8-aware `git grep`
>> > > + # (Ubuntu's `git grep` is compiled without support for libpcre, otherwise we
>> > > + # could use `git grep -P` with the `\u` syntax).
>> > > + ! LANG=C git grep -Il "$(LANG=C.UTF-8 printf \
>> > > + '\\(\u202a\\|\u202b\\|\u202c\\|\u202d\\|\u202e\\|\u2066\\|\u2067\\|\u2068\\|\u2069\\)')"
>> > > sparse:
>> > > needs: ci-config
>> > > if: needs.ci-config.outputs.enabled == 'yes'
>> > >
>> > > base-commit: 0cddd84c9f3e9c3d793ec93034ef679335f35e49
>> >
>> > It would be easier to maintain this if were added to
>> > ci/run-static-analysis.sh instead, where we have some similar tests, and
>> > if it lives there we could do away with the double-escaping, then it can
>> > also be run manually.
>> >
>> > Also, can't we just pipe "git ls-files -z" into "perl -0ne" here and use
>> > its unconditional support for e.g. unicode properties in regexes.
>>
>> I agree that the double-escaping is ugly. I think that this would be
>> easier to maintain if it lived in ci/run-static-analysis.sh or its own
>> script like ci/check-directional-formatting.sh.
>
> That's a good idea, I will put it into its own script in ci/.
>
>> And yes, constructing a byte pattern is a little odd as well, but I
>> think that it's the best you can do if you're limited to running 'git
>> grep' without libpcre. I wondered if we could depend on perl being
>> around during CI, but as far as I know we can since install Perl modules
>> in ci/install-dependencies.sh and use Perl extensively through the test
>> suite.
>
> Perl alone won't fix anything. A crucial part of the `git grep` invocation
> is the `-I` option: ignore binary files.
As noted in [1] I'm rather skeptical of us needing this at all, but why
is "-I" needed over asking "git grep" or "git ls-files" to exclude
binary files?
git ls-files ':!(attr:binary)'
I.e. why do ad-hoc binary detection on the fly in git.git when we should
already be marking what files are binary?
If this check shows a false positive due to a binary file isn't that a
good thing (sans LTR issues I mentioned upthread), i.e. we should be
adding that file to .gitattributes, no?
In any case, I meant that the match on the RHS might be easier with
Perl, in such a pipeline you could always farm out the binary detection
to GNU grep on the LHS or whatever. Maybe you don't want to do it with
Perl, but using the -I option seems like a bad idea in either case.
1. https://lore.kernel.org/git/211102.86lf261q2e.gmgdl@xxxxxxxxxxxxxxxxxxx/
