( )⚙ D5907 copy: respect ui.relative-paths in copy/rename

This is an archive of the discontinued Mercurial Phabricator instance.

copy: respect ui.relative-paths in copy/rename
ClosedPublic

Authored by martinvonz on Feb 8 2019, 3:46 PM.

Details

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.Feb 8 2019, 3:46 PM
martinvonz updated this revision to Diff 13933.Feb 8 2019, 3:59 PM
martinvonz updated this revision to Diff 13934.
martinvonz updated this revision to Diff 13977.Feb 9 2019, 5:25 PM
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Feb 9 2019, 10:45 PM
  • a/mercurial/scmutil.py

+++ b/mercurial/scmutil.py
@@ -51,8 +51,10 @@
)
if pycompat.iswindows:
+ from . import windows as platform

from . import scmwindows as scmplatform

else:
+ from . import posix as platform

from . import scmposix as scmplatform

It's util.localpath().

@@ -757,7 +759,7 @@

    pathto = repo.pathto
    return lambda f: pathto(f, cwd)
else:
  • return lambda f: f

+ return platform.localpath

I've removed this since it seemed unrelated to this specific patch. It might
break some Windows tests, but it should already be broken since a997163e7fae.
Some (or all?) uipathfn()s have to respect ui.slash config on Windows.

In D5907#86561, @yuja wrote:
  • a/mercurial/scmutil.py

+++ b/mercurial/scmutil.py
@@ -51,8 +51,10 @@
)
if pycompat.iswindows:
+ from . import windows as platform

from . import scmwindows as scmplatform

else:
+ from . import posix as platform

from . import scmposix as scmplatform

It's util.localpath().

@@ -757,7 +759,7 @@

    pathto = repo.pathto
    return lambda f: pathto(f, cwd)
else:
  • return lambda f: f

+ return platform.localpath

I've removed this since it seemed unrelated to this specific patch.

That was just something I was experimenting with and must have accidentally amended in. Thanks for fixing!

It might
break some Windows tests, but it should already be broken since a997163e7fae.
Some (or all?) uipathfn()s have to respect ui.slash config on Windows.

Did that commit change any behavior on Windows? I was trying to figure out where the conversion to Windows paths was done and thought it seemed like it was mostly just done in util.pathto(), so e.g. m.rel() would use backslashes and m.abs() would use slashes. (I just learned that there is a deprecated ui.slash config too, but that doesn't seem to change much.)

yuja added a comment.Feb 10 2019, 9:12 PM
> It might
>  break some Windows tests, but it should already be broken since https://phab.mercurial-scm.org/rHGa997163e7fae2fe933f8d0c6d1013205befd1ee4.
>  Some (or all?) `uipathfn()`s have to respect `ui.slash` config on Windows.
Did that commit change any behavior on Windows?

Probably. pathto(f, cwd='') would return backslash path on Windows.

I was trying to figure out where the conversion to Windows paths was done and thought it seemed like it was mostly just done in util.pathto(), so e.g. m.rel() would use backslashes and m.abs() would use slashes. (I just learned that there is a deprecated ui.slash config too, but that doesn't seem to change much.)

Correct. Path handling on Windows seemed inconsistent from the start. Some
commands used to use dirstate.pathto() which always returns backslash/slash
paths depending on ui.slash, and the others using m.rel|abs|uipath() don't.

Since we have scmutil.getuipathfn() now, maybe we can fix the inconsistency?

In D5907#86625, @yuja wrote:
> It might
>  break some Windows tests, but it should already be broken since https://phab.mercurial-scm.org/rHGa997163e7fae2fe933f8d0c6d1013205befd1ee4.
>  Some (or all?) `uipathfn()`s have to respect `ui.slash` config on Windows.
Did that commit change any behavior on Windows?

Probably. pathto(f, cwd='') would return backslash path on Windows.

getuipathfn() uses repo.pathto() when a relative path was requested (including by setting legacyrelativevalue=True or forcerelativevalue=True), so I think there shouldn't be much change in behavior with that commit.

I was trying to figure out where the conversion to Windows paths was done and thought it seemed like it was mostly just done in util.pathto(), so e.g. m.rel() would use backslashes and m.abs() would use slashes. (I just learned that there is a deprecated ui.slash config too, but that doesn't seem to change much.)

Correct. Path handling on Windows seemed inconsistent from the start. Some
commands used to use dirstate.pathto() which always returns backslash/slash
paths depending on ui.slash, and the others using m.rel|abs|uipath() don't.
Since we have scmutil.getuipathfn() now, maybe we can fix the inconsistency?

Yep, I was going to switch from lambda f: f to util.localpath for repo-relative paths. I was leaving that for when I'm done with the rest in case it affects tests on Windows, so they don't need to be updated several times.

In D5907#86625, @yuja wrote:
> It might
>  break some Windows tests, but it should already be broken since https://phab.mercurial-scm.org/rHGa997163e7fae2fe933f8d0c6d1013205befd1ee4.
>  Some (or all?) `uipathfn()`s have to respect `ui.slash` config on Windows.
Did that commit change any behavior on Windows?

Probably. pathto(f, cwd='') would return backslash path on Windows.

I was trying to figure out where the conversion to Windows paths was done and thought it seemed like it was mostly just done in util.pathto(), so e.g. m.rel() would use backslashes and m.abs() would use slashes. (I just learned that there is a deprecated ui.slash config too, but that doesn't seem to change much.)

Correct. Path handling on Windows seemed inconsistent from the start. Some
commands used to use dirstate.pathto() which always returns backslash/slash
paths depending on ui.slash, and the others using m.rel|abs|uipath() don't.
Since we have scmutil.getuipathfn() now, maybe we can fix the inconsistency?

yuja added a comment.Feb 11 2019, 8:43 AM
`getuipathfn()` uses `repo.pathto()` when a relative path was requested (including by setting `legacyrelativevalue=True` or `forcerelativevalue=True`), so I think there shouldn't be much change in behavior with that commit.

But the default of hg status is relative=False, so the status output
would have significant change on Windows.

In D5907#86706, @yuja wrote:
`getuipathfn()` uses `repo.pathto()` when a relative path was requested (including by setting `legacyrelativevalue=True` or `forcerelativevalue=True`), so I think there shouldn't be much change in behavior with that commit.

But the default of hg status is relative=False, so the status output
would have significant change on Windows.

Ohhh, now I see! hg status changed because it used repo.pathto(f, cwd='') in the non-relative case, and repo.pathto() calls uitil.localpath(). I think most or all of the other commands I've changed with recent patches simply use f the non-relative case. So I have hopefully only broken hg status. But I'll hopefully fix almost all of them to use native paths soon :)