[PATCH] run-tests: paths do automatically match on Windows

[Mads Kiilerich]

On 10/05/2012 06:00 AM, Simon Heimberg wrote:
> # HG changeset patch
> # User Simon Heimberg <simohe at besonet.ch>
> # Date 1349409552 -7200
> # Node ID 2bb109db2f30bf6efc7999dc53c7a87c4d612b41
> # Parent  5e641a809b056d42c495dbd69ca991ddb13c3c5d
> run-tests: paths do automatically match on Windows

For reference: This is an alternative to 9910f60a37ee .

The advantage of the current '(glob)' approach is that it requires 
explicit markup ... and that is also the main disadvantage.

The real problem here is IMO that ui.slash doesn't control all the 
places where we display native paths ... and there is no clean simple 
way to achieve that. We just want to handle that in the test suite for now.

Most of the tests do now have the necessary markup and the maintenance 
overhead of the current approach is relatively low. 
http://www.selenic.com/pipermail/mercurial-devel/2012-October/045166.html is 
as an exception. (That kind of need for markup could however also be 
caught by a check-code warning.)

It would perhaps have been better to use the proposed approach from the 
beginning, but I don't think there is any real benefit from switching now.

Anyway, some comments:

> This makes writing tests easier. And it could speed up the test suite a little
> bit because more lines will match without a glob comparison.
>
> When a path MUST be written with / on any os, the (esc) syntax can be used.

But nobody will ever bother enough to use that markup for that purpose 
anywhere.

> also update check-code accordingly and warn about glob with no glob (? or *)

"also" -> separate patch.

> diff -r 5e641a809b05 -r 2bb109db2f30 contrib/check-code.py
> --- a/contrib/check-code.py	Fre Okt 05 05:35:02 2012 +0200
> +++ b/contrib/check-code.py	Fre Okt 05 05:59:12 2012 +0200
> @@ -100,11 +100,12 @@
>        "explicit exit code checks unnecessary"),
>       (uprefix + r'set -e', "don't use set -e"),
>       (uprefix + r'\s', "don't indent commands, use > for continued lines"),
> -    (r'^  saved backup bundle to \$TESTTMP.*\.hg$',
> -     "use (glob) to match Windows paths too"),
>     ],
>     # warnings
> -  []
> +  [
> +    (r'^  [^*?\n]* \(glob\)$',
> +     "warning: remove (glob), Windows paths match automatically"),
> +  ]
>   ]

There is no need for a new warning. Just clean it up once and for all 
(preferably in a separate patch).

> diff -r 5e641a809b05 -r 2bb109db2f30 tests/run-tests.py
> --- a/tests/run-tests.py	Fre Okt 05 05:35:02 2012 +0200
> +++ b/tests/run-tests.py	Fre Okt 05 05:59:12 2012 +0200
> @@ -536,6 +536,13 @@
>               res += re.escape(c)
>       return rematch(res, l)
>   
> +if os.name == 'nt':
> +    def pathmatch(el, l):
> +        return len(el) != len(l) and el == l.replace('\\', '/')

Why the '!=' ? It seems completely wrong.

And it seems like an optimization ... but will it really give any 
optimization that is worth it? s.replace() is "fast" - especially if 
nothing matches.

I am sure there are other and bigger bottlenecks in run-tests.

> +else:
> +    def pathmatch(el, l):
> +        return False
> +
>   def linematch(el, l):
>       if el == l: # perfect match (fast)
>           return True
> @@ -545,7 +552,8 @@
>            el.endswith(" (esc)\n") and
>                (el[:-7].decode('string-escape') + '\n' == l or
>                 el[:-7].decode('string-escape').replace('\r', '') +
> -                  '\n' == l and os.name == 'nt'))):
> +                  '\n' == l and os.name == 'nt') or
> +         pathmatch(el, l))):

That is two completely different approaches to handling 'nt' specific 
functionality on two subsequent lines. Please be consistent.

The new indentation is also not consistent with the existing.

It would perhaps be better to move more to a platform dependent match 
function, but having the test explicit inline is perhaps more readable.

The name "patchmatch" is perhaps also not descriptive. "slashmatch" 
would perhaps be better.

/Mads