Bug 4629 - tilde expansion in .hgrc stopped working in version 3.3 on Windows
Summary: tilde expansion in .hgrc stopped working in version 3.3 on Windows
Status: RESOLVED FIXED
Alias: None
Product: Mercurial
Classification: Unclassified
Component: Mercurial (show other bugs)
Version: 3.4-rc
Hardware: PC Windows
: urgent bug
Assignee: Bugzilla
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-28 11:53 UTC by David Hobbs
Modified: 2015-05-12 01:00 UTC (History)
4 users (show)

See Also:
Python Version: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Hobbs 2015-04-28 11:53 UTC
The expansion happens but the path separators are removed from the expanded part, leaving a path like "C:Usersjohn/hgweb.cfg". It worked in version 3.2.4, but doesn't in 3.3, 3.3.2, 3.3.3, and 3.4-rc. I'm using the 64-bit version.
Comment 1 Matt Harbison 2015-04-28 13:29 UTC
Can you be more specific about what section/line/content you have?

I tried changing an extension to using '~' with 3.4-rc+33-1f9127c9239b:

[extensions]
bfiles = ~hgext\bfiles\bfiles

E:\Projects\C_Projects\attocfgd\Agent>hg showconfig extensions --debug
*** failed to import extension bfiles from ~hgext\bfiles\bfiles: [Errno 2] No such file or directory: 'C:/Users/hgext/bfiles/bfiles'
read config from: C:\Program Files\TortoiseHg\hgrc.d\EditorTools.rc
...


[extensions]
bfiles = ~\hgext\bfiles\bfiles

E:\Projects\C_Projects\attocfgd\Agent>hg showconfig extensions --debug
read config from: C:\Program Files\TortoiseHg\hgrc.d\EditorTools.rc
...


[extensions]
bfiles = ~/hgext/bfiles/bfiles

E:\Projects\C_Projects\attocfgd\Agent>hg showconfig extensions --debug
read config from: C:\Program Files\TortoiseHg\hgrc.d\EditorTools.rc
...


So in no case am I losing the path separators.  I'm on 32 bit Win7 now, but I can't see how that would matter.
Comment 2 David Hobbs 2015-04-28 13:42 UTC
Sorry, I should have included that detail. In the [alias] section I have a line like this:

serve = serve --web-conf ~/hgweb.cfg

It worked up until 3.3.
Comment 3 Matt Harbison 2015-04-28 14:22 UTC
Confirmed.

Interestingly, it isn't a problem in MinGW because that expands with '/' (though that targets the home directory in C:/MinGW/msys/1.0/, so likely not what you want as a short term workaround).
Comment 4 Matt Mackall 2015-04-28 14:59 UTC
Regression -> urgent. Matt, do we know who/what broke it?
Comment 5 Matt Harbison 2015-04-28 15:37 UTC
Bisected to here:

changeset:   23832:1642eb429536
user:        FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
date:        Thu Dec 25 23:33:26 2014 +0900
summary:     windows: quote the specified string only when it has to be quoted

dispatch.aliasargs() is calling util.shellquote(), which this cset changed.  The string is returned from shellquote() expanded and unquoted, and then shlex.split() in aliasargs() mangles it.

I'll see if it works to quote when '\' is in the string, but since that can be used as an escape in MinGW (like /c/Program\ Files/), IDK if that will break something else, especially in tests.
Comment 6 Matt Harbison 2015-04-28 16:39 UTC
So this:

diff --git a/mercurial/windows.py b/mercurial/windows.py
--- a/mercurial/windows.py
+++ b/mercurial/windows.py
@@ -167,10 +167,12 @@ def shellquote(s):
         _quotere = re.compile(r'(\\*)("|\\$)')
     global _needsshellquote
     if _needsshellquote is None:
-        # ":" and "\\" are also treated as "safe character", because
-        # they are used as a part of path name (and the latter doesn't
-        # work as "escape character", like one on posix) on Windows
-        _needsshellquote = re.compile(r'[^a-zA-Z0-9._:/\\-]').search
+        # ":" is also treated as "safe character", because it is used as a part
+        # of path name on Windows.  "\" is also part of a path name, but isn't
+        # safe because shlex.split() (kind of) treats it as an escape char and
+        # drops it.  It will leave the next character, even if it is another
+        # "\".
+        _needsshellquote = re.compile(r'[^a-zA-Z0-9._:/-]').search
     if s and not _needsshellquote(s) and not _quotere.search(s):
         # "s" shouldn't have to be quoted
         return s

causes test changes like this:

diff --git a/tests/test-extdiff.t b/tests/test-extdiff.t
--- a/tests/test-extdiff.t
+++ b/tests/test-extdiff.t
@@ -59,7 +59,7 @@ Should diff cloned directories:
 Should diff cloned files directly:

   $ hg falabala -r 0:1
-  diffing */extdiff.*/a.8a5febb7f867/a a.34eed99112ab/a (glob)
+  diffing "f:\\tempdir\\extdiff.mffkvs\\a.8a5febb7f867\\a" "a.34eed99112ab\\a"\r (esc)
   [1]

 Test diff during merge:


We should probably endeavor to quote strings with '\' but not '\\'.  But that is beyond my regex capabilities.  We could probably conditionalize the tests that are not '#if windows' for now because Windows *seems* to be OK with '\\' as a path separator (in cmd.exe anyway), but I'd want another Windows guy to confirm.  The only real change in the test is the addition of quotes- these were double slashed before.
Comment 7 FUJIWARA Katsunori 2015-04-29 12:54 UTC
(In reply to comment #6)

Thank you for figuring out problem basing on my change, Matt.

As you said, dropping "\" from "windows._needsshellquote" seems
reasonable.

And IMHO, expected output lines in test-extdiff.t should be
written separately for POSIX and Windows by "#if windows"
directive, because "quote string containing '\'" itself makes
expected lines on POSIX and Windows differ from each other
(and tricky unification is difficult to maintain).
Comment 8 HG Bot 2015-04-30 19:01 UTC
Fixed by http://selenic.com/repo/hg/rev/eea3977e6fca
Matt Harbison <matt_harbison@yahoo.com>
windows: make shellquote() quote any path containing '\' (issue4629)

The '~' in the bug report is being expanded to a path with Windows style slashes
before being passed to shellquote() via util.shellquote().  But shlex.split()
strips '\' out of the string, leaving an invalid path in dispatch.aliasargs().

This regressed in 1642eb429536.

For now, the tests need to be conditionalized for Windows (because those paths
are quoted).  In the future, a more complex regex could probably skip the quotes
if all component separators are double '\'.  I opted to glob away the quotes in
test-rename-merge2.t and test-up-local-change.t (which only exist on Windows),
because they are in very large blocks of output and there are way too many diffs
to conditionalize with #if directives.  Maybe the entire path should be globbed
away like the following paths in each changed line.  Or, letting #if directives
sit in the middle of the output as was mentioned a few months back would work
too.

Unfortunately, I couldn't figure out how to test the specific bug.  All of the
'hg serve' tests have a #require serve declaration, causing them to be skipped
on Windows.  Adding an alias for 'expandtest = outgoing ~/bogusrepo' prints the
repo as '$TESTTMP/bogusrepo', so the test runner must be changing the
environment somehow.

(please test the fix)
Comment 9 HG Bot 2015-05-04 14:30 UTC
Fixed by http://selenic.com/repo/hg/rev/30b910fea244
Matt Harbison <matt_harbison@yahoo.com>
windows: add doctest for shellquote()

This is actual test coverage for issue4629.  The test changes in eea3977e6fca
were simply the addition of quotes to the output, not ensuring that strings with
backslashes are quoted.

(please test the fix)
Comment 10 Bugzilla 2015-05-12 01:00 UTC
Bug was set to TESTING for 7 days, resolving