Patchwork [6,of,6,V2] tests: add alias to detect external diff invocation via extdiff

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Feb. 9, 2016, 7:40 p.m.
Message ID <ulh6tisfj.wl%foozy@lares.dti.ne.jp>
Download mbox | patch
Permalink /patch/13069/
State Rejected
Delegated to: Yuya Nishihara
Headers show

Comments

Katsunori FUJIWARA - Feb. 9, 2016, 7:40 p.m.
At Tue, 9 Feb 2016 23:44:33 +0900,
Yuya Nishihara wrote:
> 
> On Mon, 08 Feb 2016 18:39:22 +0900, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1454923758 -32400
> > #      Mon Feb 08 18:29:18 2016 +0900
> > # Node ID 8bfab6803b7ef9c4e26c5834c9d2deeccc4f074b
> > # Parent  2d91a08cbf07bcb0413f5de8cc880571d9522987
> > tests: add alias to detect external diff invocation via extdiff
> > 
> > As previous patch described, direct usage of external "diff" command
> > via extdiff extension isn't portable.
> > 
> > To detect unintentional external "diff" command invocation via extdiff
> > immediately, this patch adds HGRCPATH file an alias definition below,
> > which shadows original "hg extdiff".
> > 
> >     [alias]
> >     extdiff = !echo use tests/pdiff via extdiff extension for portability
> 
> Can't it be a check-code rule?
> I think "hg extdiff" with no -p/-o will catch most of errors.

If we add new check-code rule "hg( .+)* extdiff" or so, it should
ignore checking "test-extdiff.t", which tests "hg extdiff" itself.

To ignore it safely, is adding list "checks" a new entry like as below
reasonable ?

====================

BTW, in that case, usage of extdiff in "test-extension.t" should be
fully replaced by another one, too :-)


> The patch 5 looks good but I've not pushed yet as you might want to fold
> check-code rule into it.


----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Yuya Nishihara - Feb. 10, 2016, 1 p.m.
On Wed, 10 Feb 2016 04:40:00 +0900, FUJIWARA Katsunori wrote:
> At Tue, 9 Feb 2016 23:44:33 +0900,
> Yuya Nishihara wrote:
> > 
> > On Mon, 08 Feb 2016 18:39:22 +0900, FUJIWARA Katsunori wrote:
> > > # HG changeset patch
> > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > > # Date 1454923758 -32400
> > > #      Mon Feb 08 18:29:18 2016 +0900
> > > # Node ID 8bfab6803b7ef9c4e26c5834c9d2deeccc4f074b
> > > # Parent  2d91a08cbf07bcb0413f5de8cc880571d9522987
> > > tests: add alias to detect external diff invocation via extdiff
> > > 
> > > As previous patch described, direct usage of external "diff" command
> > > via extdiff extension isn't portable.
> > > 
> > > To detect unintentional external "diff" command invocation via extdiff
> > > immediately, this patch adds HGRCPATH file an alias definition below,
> > > which shadows original "hg extdiff".
> > > 
> > >     [alias]
> > >     extdiff = !echo use tests/pdiff via extdiff extension for portability
> > 
> > Can't it be a check-code rule?
> > I think "hg extdiff" with no -p/-o will catch most of errors.
> 
> If we add new check-code rule "hg( .+)* extdiff" or so, it should
> ignore checking "test-extdiff.t", which tests "hg extdiff" itself.
> 
> To ignore it safely, is adding list "checks" a new entry like as below
> reasonable ?
> 
> ====================
> diff --git a/contrib/check-code.py b/contrib/check-code.py
> --- a/contrib/check-code.py
> +++ b/contrib/check-code.py
> @@ -386,6 +386,7 @@ checks = [
>      ('test script', r'(.*/)?test-[^.~]*$', '', testfilters, testpats),
>      ('c', r'.*\.[ch]$', '', cfilters, cpats),
>      ('unified test', r'.*\.t$', '', utestfilters, utestpats),
> +    ('unified test, r'(?!test-extdiff\.t).*\.t$', utestfilters, extdiffats),
>      ('layering violation repo in revlog', r'mercurial/revlog\.py', '',
>       pyfilters, inrevlogpats),
>      ('layering violation ui in util', r'mercurial/util\.py', '', pyfilters,
> ====================

I meant something like this:

    (r'^  \$ +hg( +-[^ ]+ +[^ ]*)*? +extdiff'
     r'( +(-[^po]|--(?!program|option)[^ ]+|[^-][^ ]*))* *$',

It isn't perfect, but can catch "hg extdiff" without -o/-p/--program/--option.
We can silence false positive by passing one of these options.
Katsunori FUJIWARA - Feb. 10, 2016, 2:47 p.m.
At Wed, 10 Feb 2016 22:00:54 +0900,
Yuya Nishihara wrote:
> 
> On Wed, 10 Feb 2016 04:40:00 +0900, FUJIWARA Katsunori wrote:
> > At Tue, 9 Feb 2016 23:44:33 +0900,
> > Yuya Nishihara wrote:
> > > 
> > > On Mon, 08 Feb 2016 18:39:22 +0900, FUJIWARA Katsunori wrote:
> > > > # HG changeset patch
> > > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > > > # Date 1454923758 -32400
> > > > #      Mon Feb 08 18:29:18 2016 +0900
> > > > # Node ID 8bfab6803b7ef9c4e26c5834c9d2deeccc4f074b
> > > > # Parent  2d91a08cbf07bcb0413f5de8cc880571d9522987
> > > > tests: add alias to detect external diff invocation via extdiff
> > > > 
> > > > As previous patch described, direct usage of external "diff" command
> > > > via extdiff extension isn't portable.
> > > > 
> > > > To detect unintentional external "diff" command invocation via extdiff
> > > > immediately, this patch adds HGRCPATH file an alias definition below,
> > > > which shadows original "hg extdiff".
> > > > 
> > > >     [alias]
> > > >     extdiff = !echo use tests/pdiff via extdiff extension for portability
> > > 
> > > Can't it be a check-code rule?
> > > I think "hg extdiff" with no -p/-o will catch most of errors.
> > 
> > If we add new check-code rule "hg( .+)* extdiff" or so, it should
> > ignore checking "test-extdiff.t", which tests "hg extdiff" itself.
> > 
> > To ignore it safely, is adding list "checks" a new entry like as below
> > reasonable ?
> > 
> > ====================
> > diff --git a/contrib/check-code.py b/contrib/check-code.py
> > --- a/contrib/check-code.py
> > +++ b/contrib/check-code.py
> > @@ -386,6 +386,7 @@ checks = [
> >      ('test script', r'(.*/)?test-[^.~]*$', '', testfilters, testpats),
> >      ('c', r'.*\.[ch]$', '', cfilters, cpats),
> >      ('unified test', r'.*\.t$', '', utestfilters, utestpats),
> > +    ('unified test, r'(?!test-extdiff\.t).*\.t$', utestfilters, extdiffats),
> >      ('layering violation repo in revlog', r'mercurial/revlog\.py', '',
> >       pyfilters, inrevlogpats),
> >      ('layering violation ui in util', r'mercurial/util\.py', '', pyfilters,
> > ====================
> 
> I meant something like this:
> 
>     (r'^  \$ +hg( +-[^ ]+ +[^ ]*)*? +extdiff'
>      r'( +(-[^po]|--(?!program|option)[^ ]+|[^-][^ ]*))* *$',
> 
> It isn't perfect, but can catch "hg extdiff" without -o/-p/--program/--option.
> We can silence false positive by passing one of these options.

Oh, I see.

I'll send the patch adding new check-code rule as revised #6, because
adding such rule needs additional change for false positive case in
test-extdiff.t, AFAIK.

Therefore, please push #5 separately.

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Yuya Nishihara - Feb. 11, 2016, 6:33 a.m.
On Wed, 10 Feb 2016 23:47:58 +0900, FUJIWARA Katsunori wrote:
> At Wed, 10 Feb 2016 22:00:54 +0900,
> Yuya Nishihara wrote:
> > I meant something like this:
> > 
> >     (r'^  \$ +hg( +-[^ ]+ +[^ ]*)*? +extdiff'
> >      r'( +(-[^po]|--(?!program|option)[^ ]+|[^-][^ ]*))* *$',
> > 
> > It isn't perfect, but can catch "hg extdiff" without -o/-p/--program/--option.
> > We can silence false positive by passing one of these options.
> 
> Oh, I see.
> 
> I'll send the patch adding new check-code rule as revised #6, because
> adding such rule needs additional change for false positive case in
> test-extdiff.t, AFAIK.
> 
> Therefore, please push #5 separately.

Queued PATCH 5, thanks.

Patch

====================
diff --git a/contrib/check-code.py b/contrib/check-code.py
--- a/contrib/check-code.py
+++ b/contrib/check-code.py
@@ -386,6 +386,7 @@  checks = [
     ('test script', r'(.*/)?test-[^.~]*$', '', testfilters, testpats),
     ('c', r'.*\.[ch]$', '', cfilters, cpats),
     ('unified test', r'.*\.t$', '', utestfilters, utestpats),
+    ('unified test, r'(?!test-extdiff\.t).*\.t$', utestfilters, extdiffats),
     ('layering violation repo in revlog', r'mercurial/revlog\.py', '',
      pyfilters, inrevlogpats),
     ('layering violation ui in util', r'mercurial/util\.py', '', pyfilters,