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. Ciao, Dscho
