Patchwork [2,of,2] addremove: restore the relative path printing when files are named

login
register
mail settings
Submitter Matt Harbison
Date Dec. 5, 2014, 4:47 a.m.
Message ID <740027022204b6131b19.1417754853@Envy>
Download mbox | patch
Permalink /patch/7010/
State Accepted
Commit e89acf3e8c30e6aa9236e1fc021709344cc9f32f
Headers show

Comments

Matt Harbison - Dec. 5, 2014, 4:47 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1417752507 18000
#      Thu Dec 04 23:08:27 2014 -0500
# Node ID 740027022204b6131b19eab62673b4929524d490
# Parent  6dbf121735cfce8a9c241d1b58f255a4eee218c3
addremove: restore the relative path printing when files are named

This fixes the previously mentioned issue with 3778884197f0, and undoes its
corresponding test change.

The test change demonstrates the correctness when a file is specified (i.e. the
glob is required on Windows because relative paths use '\' and absolute paths
use '/').  It is admittedly very subtle, but there will be a more robust test in
the addremove -S v3 series.
Martin von Zweigbergk - Dec. 5, 2014, 5:03 a.m.
This series looks good to me. Thanks!

On Thu Dec 04 2014 at 8:48:18 PM Matt Harbison <mharbison72@gmail.com>
wrote:

>
> The test change demonstrates the correctness when a file is specified
> (i.e. the
> glob is required on Windows because relative paths use '\' and absolute
> paths
> use '/').
>

Off topic, but it seems weird that absolute paths would use '/'. At first I
thought it was a typo, but then I saw the absolute paths in your mail about
Mads's rebase patch. Do you know why absolute paths use '/'? I don't even
use Windows anymore, so it doesn't matter to me; I'm just curious.
Matt Harbison - Dec. 5, 2014, 5:28 a.m.
On Fri, 05 Dec 2014 00:03:40 -0500, Martin von Zweigbergk  
<martinvonz@google.com> wrote:

> This series looks good to me. Thanks!
>
> On Thu Dec 04 2014 at 8:48:18 PM Matt Harbison <mharbison72@gmail.com>
> wrote:
>
>>
>> The test change demonstrates the correctness when a file is specified
>> (i.e. the
>> glob is required on Windows because relative paths use '\' and absolute
>> paths
>> use '/').
>>
>
> Off topic, but it seems weird that absolute paths would use '/'. At  
> first I
> thought it was a typo, but then I saw the absolute paths in your mail  
> about
> Mads's rebase patch. Do you know why absolute paths use '/'? I don't even
> use Windows anymore, so it doesn't matter to me; I'm just curious.

For the series we've been discussing, "absolute" isn't absolute in the  
sense of 'C:\dir' or '/var/log'- I guess it is just "absolute" from the  
top of the repo.  I think I read a comment somewhere recently that  
dirstate always uses '/' as a separator, and that was carried forth with  
the file names in walk().  Sometimes the '/' also leaks into windows for  
relative paths because there are various bits of code that do '"parent" +  
"/" + "file"' joins instead of os.path.join().

The '/' in some of the paths in that stacktrace I sent in is a bit  
mysterious, because it seems to only be for evolve.py and showconfig says:

$ ../hg showconfig extensions
extensions.evolve=C:\Users\Matt\Projects\hg-evolve\hgext\evolve.py

But the Windows API accepts either style, so it's probably more effort  
than it is worth to completely fix.

--Matt
Martin von Zweigbergk - Dec. 5, 2014, 5:35 a.m.
On Thu Dec 04 2014 at 9:28:40 PM Matt Harbison <mharbison72@gmail.com>
wrote:

> On Fri, 05 Dec 2014 00:03:40 -0500, Martin von Zweigbergk
> <martinvonz@google.com> wrote:
>
> > This series looks good to me. Thanks!
> >
> > On Thu Dec 04 2014 at 8:48:18 PM Matt Harbison <mharbison72@gmail.com>
> > wrote:
> >
> >>
> >> The test change demonstrates the correctness when a file is specified
> >> (i.e. the
> >> glob is required on Windows because relative paths use '\' and absolute
> >> paths
> >> use '/').
> >>
> >
> > Off topic, but it seems weird that absolute paths would use '/'. At
> > first I
> > thought it was a typo, but then I saw the absolute paths in your mail
> > about
> > Mads's rebase patch. Do you know why absolute paths use '/'? I don't even
> > use Windows anymore, so it doesn't matter to me; I'm just curious.
>
> For the series we've been discussing, "absolute" isn't absolute in the
> sense of 'C:\dir' or '/var/log'- I guess it is just "absolute" from the
> top of the repo.  I think I read a comment somewhere recently that
> dirstate always uses '/' as a separator, and that was carried forth with
> the file names in walk().  Sometimes the '/' also leaks into windows for
> relative paths because there are various bits of code that do '"parent" +
> "/" + "file"' joins instead of os.path.join().
>

OK, so not entirely intentional. Thanks for explaining.
Augie Fackler - Dec. 5, 2014, 6:07 p.m.
On Thu, Dec 04, 2014 at 11:47:33PM -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1417752507 18000
> #      Thu Dec 04 23:08:27 2014 -0500
> # Node ID 740027022204b6131b19eab62673b4929524d490
> # Parent  6dbf121735cfce8a9c241d1b58f255a4eee218c3
> addremove: restore the relative path printing when files are named

Queued. Thanks to Martin for doing a quick review on this and floating
it to my inbox.

>
> This fixes the previously mentioned issue with 3778884197f0, and undoes its
> corresponding test change.
>
> The test change demonstrates the correctness when a file is specified (i.e. the
> glob is required on Windows because relative paths use '\' and absolute paths
> use '/').  It is admittedly very subtle, but there will be a more robust test in
> the addremove -S v3 series.
>
> diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
> --- a/mercurial/scmutil.py
> +++ b/mercurial/scmutil.py
> @@ -730,11 +730,10 @@
>      toprint.update(deleted)
>      for abs in sorted(toprint):
>          if repo.ui.verbose or not m.exact(abs):
> -            rel = m.rel(abs)
>              if abs in unknownset:
> -                status = _('adding %s\n') % ((m.anypats() and rel) or abs)
> +                status = _('adding %s\n') % m.uipath(abs)
>              else:
> -                status = _('removing %s\n') % ((m.anypats() and rel) or abs)
> +                status = _('removing %s\n') % m.uipath(abs)
>              repo.ui.status(status)
>
>      renames = _findrenames(repo, m, added + unknown, removed + deleted,
> diff --git a/tests/test-addremove-similar.t b/tests/test-addremove-similar.t
> --- a/tests/test-addremove-similar.t
> +++ b/tests/test-addremove-similar.t
> @@ -91,7 +91,7 @@
>  no copies found here (since the target isn't in d
>
>    $ hg addremove -s80 d
> -  removing d/b
> +  removing d/b (glob)
>
>  copies here
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -730,11 +730,10 @@ 
     toprint.update(deleted)
     for abs in sorted(toprint):
         if repo.ui.verbose or not m.exact(abs):
-            rel = m.rel(abs)
             if abs in unknownset:
-                status = _('adding %s\n') % ((m.anypats() and rel) or abs)
+                status = _('adding %s\n') % m.uipath(abs)
             else:
-                status = _('removing %s\n') % ((m.anypats() and rel) or abs)
+                status = _('removing %s\n') % m.uipath(abs)
             repo.ui.status(status)
 
     renames = _findrenames(repo, m, added + unknown, removed + deleted,
diff --git a/tests/test-addremove-similar.t b/tests/test-addremove-similar.t
--- a/tests/test-addremove-similar.t
+++ b/tests/test-addremove-similar.t
@@ -91,7 +91,7 @@ 
 no copies found here (since the target isn't in d
 
   $ hg addremove -s80 d
-  removing d/b
+  removing d/b (glob)
 
 copies here