This is an archive of the discontinued Mercurial Phabricator instance.

match: make sure `root` argument is always an absolute path (API)
ClosedPublic

Authored by martinvonz on Dec 13 2019, 2:50 PM.

Details

Summary

The root argument should already be an absolute path, but we had
tests that passed a relative path. This patch fixes up the tests and
adds an assertion.

This assumes that os.path.isabs('/repo') will be True on all
platforms we care to run tests on. Augie tested for me that it does
work on Windows, so that's good enough for me.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

martinvonz created this revision.Dec 13 2019, 2:50 PM
mharbison72 accepted this revision.Dec 16 2019, 9:00 PM
mharbison72 added a subscriber: mharbison72.

Windows has some very strange rules for paths, and I remember fixing a subrepo bug with a path in the form C:foo. I tried these, and can't think of anything more we'd need to worry about.

>>> import os
>>> os.path.isabs("/")
True
>>> os.path.isabs("C:/")
True
>>> os.path.isabs("C:tests")
False

(The last one being at the root of the hg repo.)

Windows has some very strange rules for paths, and I remember fixing a subrepo bug with a path in the form C:foo. I tried these, and can't think of anything more we'd need to worry about.

>>> import os
>>> os.path.isabs("/")
True
>>> os.path.isabs("C:/")
True
>>> os.path.isabs("C:tests")
False

(The last one being at the root of the hg repo.)

Thanks for checking. I think that means we're okay with this series now. I won't queue it because I wrote some of it.

pulkit added a subscriber: pulkit.Dec 20 2019, 7:16 AM

I am bit confused with /repo everywhere that whether that's the only valid or other forms can be valid also. I will prefer making existing root passed as absolute instead of replacing them with /repo (provided I understood correctly). Also I am not sure, but does this counts as API change?

mercurial/match.py
186

Hm, does this has to be always /repo or /foo is also a valid parameter here?

tests/test-match.py
69

This and below ones can be /x too right?

I am bit confused with /repo everywhere that whether that's the only valid or other forms can be valid also. I will prefer making existing root passed as absolute instead of replacing them with /repo (provided I understood correctly).

I switched to /repo only to try to clarify that the path is the path to the repo (not some subdirectory). Want me to switch them back?

Also I am not sure, but does this counts as API change?

I'm not sure if any extensions might have passed a relative path without noticing. Sure, we can add the (API) tag. Can you do that in flight? Or I can do it later if you want me to change some of the paths anyway.

I am bit confused with /repo everywhere that whether that's the only valid or other forms can be valid also. I will prefer making existing root passed as absolute instead of replacing them with /repo (provided I understood correctly).

I switched to /repo only to try to clarify that the path is the path to the repo (not some subdirectory). Want me to switch them back?

Yes, that will be much appreciated.

mercurial/match.py
155

We can mention here that it must be an absolute path.

martinvonz retitled this revision from match: make sure `root` argument is always an absolute path to match: make sure `root` argument is always an absolute path (API).Dec 20 2019, 9:52 AM
martinvonz updated this revision to Diff 18893.
martinvonz marked an inline comment as done.Dec 20 2019, 9:53 AM
martinvonz added inline comments.
mercurial/match.py
186

I've changed this one back.

tests/test-match.py
69

I've left this one as /repo because I think it's clearer that it's a repo path that way.

martinvonz marked an inline comment as done.Dec 20 2019, 9:54 AM
martinvonz added inline comments.
mercurial/match.py
155

I think "canonical" already implied "absolute".

pulkit accepted this revision.Dec 20 2019, 11:33 AM
This revision is now accepted and ready to land.Dec 20 2019, 11:33 AM