Re: [PATCH] pathutil: add doctests for canonpath()

[Yuya Nishihara]

On Sat, 04 Nov 2017 22:23:32 -0400, Matt Harbison wrote:
> On Sat, 04 Nov 2017 22:11:21 -0400, Yuya Nishihara  wrote:
> > My concern is that the doctest is too limited in scope to cover possible  
> > bugs
> > caused by Windows drive letter. Maybe we could add a safer alternative to
> > os.path.relpath() and ban the use by check-code.
> 
> Seems like a good idea.  There are a few more uses of it, and I'm pretty  
> sure that at least cmdutil._conflictsmsg() is broke too, based on passing  
> repo.root and cwd.  (I guess thg doesn't use this function?)
> 
> I haven't been able to create the issue with the origpath code, and like I  
> said in the 'share --relative' patch, the error is useful there.  Is this  
> one of those things we can ban, but just record the few places it's needed  
> in test-check-code.t?

Perhaps os.path.relpath() can be replaced with two functions, which are:

 a) always return a relative path; raise Abort if not possible
 b) try to return a relative path only if possible; an absolute path otherwise

On Unix, they both are mapped to os.path.relpath.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH] pathutil: add doctests for canonpath()

[Matt Harbison]

On Sat, 04 Nov 2017 22:11:21 -0400, Yuya Nishihara  wrote:


On Sat, 4 Nov 2017 09:16:03 -0400, Matt Harbison wrote:

> On Nov 4, 2017, at 4:03 AM, Yuya Nishihara  wrote:
>> On Fri, 03 Nov 2017 23:26:40 -0400, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison 
>> # Date 1509762170 14400
>> #  Fri Nov 03 22:22:50 2017 -0400
>> # Node ID 7d9abeb82f021b4b96162a719f4a44a0175c0828
>> # Parent  3649c3f2cd90c8aec395ca8af5adae33defff12c
>> pathutil: add doctests for canonpath()
>>
>> This is a followup to f445b10dc7fb.  Since there's no way to ensure  
that more
>> drive letters than C: exist, this seems like the only way to test  
it.  This is
>> enough to catch the f445b10dc7fb scenario, as well as CWD outside of  
the repo

>> when the path isn't prefixed with path/to/repo.
>
> Maybe we can add an hghave rule to check if d: drive exists, is  
different from

> the TESTTMP, and can chdir into it?

I thought about that, but this allows everyone running tests on Windows  
to be able to see the error if it regresses, regardless of their  
setup.  That seems important, given how few people run Windows tests.   
I’m not sure what the Windows test system is currently configured to  
do, for instance (it’s been a WIP because of some unexpected errors  
running under buildbot, and I don’t think it is totally squared away  
yet).


Okay, that makes some sense. I'll queue this patch, thanks.

My concern is that the doctest is too limited in scope to cover possible  
bugs

caused by Windows drive letter. Maybe we could add a safer alternative to
os.path.relpath() and ban the use by check-code.


Seems like a good idea.  There are a few more uses of it, and I'm pretty  
sure that at least cmdutil._conflictsmsg() is broke too, based on passing  
repo.root and cwd.  (I guess thg doesn't use this function?)


I haven't been able to create the issue with the origpath code, and like I  
said in the 'share --relative' patch, the error is useful there.  Is this  
one of those things we can ban, but just record the few places it's needed  
in test-check-code.t?

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH] pathutil: add doctests for canonpath()

[Yuya Nishihara]

On Sat, 4 Nov 2017 09:16:03 -0400, Matt Harbison wrote:
> > On Nov 4, 2017, at 4:03 AM, Yuya Nishihara  wrote:
> >> On Fri, 03 Nov 2017 23:26:40 -0400, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison 
> >> # Date 1509762170 14400
> >> #  Fri Nov 03 22:22:50 2017 -0400
> >> # Node ID 7d9abeb82f021b4b96162a719f4a44a0175c0828
> >> # Parent  3649c3f2cd90c8aec395ca8af5adae33defff12c
> >> pathutil: add doctests for canonpath()
> >> 
> >> This is a followup to f445b10dc7fb.  Since there's no way to ensure that 
> >> more
> >> drive letters than C: exist, this seems like the only way to test it.  
> >> This is
> >> enough to catch the f445b10dc7fb scenario, as well as CWD outside of the 
> >> repo
> >> when the path isn't prefixed with path/to/repo.
> > 
> > Maybe we can add an hghave rule to check if d: drive exists, is different 
> > from
> > the TESTTMP, and can chdir into it?
> 
> I thought about that, but this allows everyone running tests on Windows to be 
> able to see the error if it regresses, regardless of their setup.  That seems 
> important, given how few people run Windows tests.  I’m not sure what the 
> Windows test system is currently configured to do, for instance (it’s been a 
> WIP because of some unexpected errors running under buildbot, and I don’t 
> think it is totally squared away yet).

Okay, that makes some sense. I'll queue this patch, thanks.

My concern is that the doctest is too limited in scope to cover possible bugs
caused by Windows drive letter. Maybe we could add a safer alternative to
os.path.relpath() and ban the use by check-code.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH] pathutil: add doctests for canonpath()

[Matt Harbison]

> On Nov 4, 2017, at 4:03 AM, Yuya Nishihara  wrote:
> 
>> On Fri, 03 Nov 2017 23:26:40 -0400, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison 
>> # Date 1509762170 14400
>> #  Fri Nov 03 22:22:50 2017 -0400
>> # Node ID 7d9abeb82f021b4b96162a719f4a44a0175c0828
>> # Parent  3649c3f2cd90c8aec395ca8af5adae33defff12c
>> pathutil: add doctests for canonpath()
>> 
>> This is a followup to f445b10dc7fb.  Since there's no way to ensure that more
>> drive letters than C: exist, this seems like the only way to test it.  This 
>> is
>> enough to catch the f445b10dc7fb scenario, as well as CWD outside of the repo
>> when the path isn't prefixed with path/to/repo.
> 
> Maybe we can add an hghave rule to check if d: drive exists, is different from
> the TESTTMP, and can chdir into it?

I thought about that, but this allows everyone running tests on Windows to be 
able to see the error if it regresses, regardless of their setup.  That seems 
important, given how few people run Windows tests.  I’m not sure what the 
Windows test system is currently configured to do, for instance (it’s been a 
WIP because of some unexpected errors running under buildbot, and I don’t think 
it is totally squared away yet).
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH] pathutil: add doctests for canonpath()

[Yuya Nishihara]

On Fri, 03 Nov 2017 23:26:40 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison 
> # Date 1509762170 14400
> #  Fri Nov 03 22:22:50 2017 -0400
> # Node ID 7d9abeb82f021b4b96162a719f4a44a0175c0828
> # Parent  3649c3f2cd90c8aec395ca8af5adae33defff12c
> pathutil: add doctests for canonpath()
> 
> This is a followup to f445b10dc7fb.  Since there's no way to ensure that more
> drive letters than C: exist, this seems like the only way to test it.  This is
> enough to catch the f445b10dc7fb scenario, as well as CWD outside of the repo
> when the path isn't prefixed with path/to/repo.

Maybe we can add an hghave rule to check if d: drive exists, is different from
the TESTTMP, and can chdir into it?
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

[PATCH] pathutil: add doctests for canonpath()

[Matt Harbison]

# HG changeset patch
# User Matt Harbison 
# Date 1509762170 14400
#  Fri Nov 03 22:22:50 2017 -0400
# Node ID 7d9abeb82f021b4b96162a719f4a44a0175c0828
# Parent  3649c3f2cd90c8aec395ca8af5adae33defff12c
pathutil: add doctests for canonpath()

This is a followup to f445b10dc7fb.  Since there's no way to ensure that more
drive letters than C: exist, this seems like the only way to test it.  This is
enough to catch the f445b10dc7fb scenario, as well as CWD outside of the repo
when the path isn't prefixed with path/to/repo.

diff --git a/mercurial/pathutil.py b/mercurial/pathutil.py
--- a/mercurial/pathutil.py
+++ b/mercurial/pathutil.py
@@ -135,7 +135,46 @@
 return False
 
 def canonpath(root, cwd, myname, auditor=None):
-'''return the canonical path of myname, given cwd and root'''
+'''return the canonical path of myname, given cwd and root
+>>> def check(root, cwd, myname):
+... a = pathauditor(root, realfs=False)
+... try:
+... return canonpath(root, cwd, myname, a)
+... except error.Abort:
+... return 'aborted'
+>>> def unixonly(root, cwd, myname, expected='aborted'):
+... if pycompat.iswindows:
+... return expected
+... return check(root, cwd, myname)
+>>> def winonly(root, cwd, myname, expected='aborted'):
+... if not pycompat.iswindows:
+... return expected
+... return check(root, cwd, myname)
+>>> winonly(b'd:repo', b'c:dir', b'filename')
+'aborted'
+>>> winonly(b'c:repo', b'c:dir', b'filename')
+'aborted'
+>>> winonly(b'c:repo', b'c:', b'filename')
+'aborted'
+>>> winonly(b'c:repo', b'c:', b'repofilename',
+... b'filename')
+'filename'
+>>> winonly(b'c:repo', b'c:repo', b'filename', b'filename')
+'filename'
+>>> winonly(b'c:repo', b'c:reposubdir', b'filename',
+... b'subdir/filename')
+'subdir/filename'
+>>> unixonly(b'/repo', b'/dir', b'filename')
+'aborted'
+>>> unixonly(b'/repo', b'/', b'filename')
+'aborted'
+>>> unixonly(b'/repo', b'/', b'repo/filename', b'filename')
+'filename'
+>>> unixonly(b'/repo', b'/repo', b'filename', b'filename')
+'filename'
+>>> unixonly(b'/repo', b'/repo/subdir', b'filename', b'subdir/filename')
+'subdir/filename'
+'''
 if util.endswithsep(root):
 rootsep = root
 else:
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel