Patchwork context: name files relative to cwd in warning messages

login
register
mail settings
Submitter Matt Harbison
Date July 12, 2017, 3:33 a.m.
Message ID <e100fdb7e94d0adec9f1.1499830388@Envy>
Download mbox | patch
Permalink /patch/22248/
State Changes Requested
Headers show

Comments

Matt Harbison - July 12, 2017, 3:33 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1499748029 14400
#      Tue Jul 11 00:40:29 2017 -0400
# Node ID e100fdb7e94d0adec9f14254e096e5757fc2f327
# Parent  6a82448038935002b24af767829d3a27908050ef
context: name files relative to cwd in warning messages

I was several directories deep in the kernel tree, ran `hg add`, and got the
warning about the size of one of the files.  I noticed that it suggested undoing
the add with a specific revert command.  The problem is, it would have failed
since the path printed was relative to the repo root instead of cwd.  While
here, I just fixed the other messages too.  As an added benefit, these messages
now look the same as the verbose/inexact messages for the corresponding command.

I don't think most of these messages are reachable (typically the corresponding
cmdutil function does the check).  I wasn't able to figure out why the keyword
tests were failing when using pathto()- I couldn't cause an absolute path to be
used by manipulating the --cwd flag on a regular add.  (I did notice that
keyword is adding the file without holding wlock.)
Yuya Nishihara - July 14, 2017, 1:50 p.m.
On Tue, 11 Jul 2017 23:33:08 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1499748029 14400
> #      Tue Jul 11 00:40:29 2017 -0400
> # Node ID e100fdb7e94d0adec9f14254e096e5757fc2f327
> # Parent  6a82448038935002b24af767829d3a27908050ef
> context: name files relative to cwd in warning messages

Sounds good to me. One nit.

> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -1518,17 +1518,20 @@
>                  (missing and self.deleted()))
>  
>      def add(self, list, prefix=""):
> -        join = lambda f: os.path.join(prefix, f)
>          with self._repo.wlock():
>              ui, ds = self._repo.ui, self._repo.dirstate
> +            uipath = lambda f: ds.pathto(prefix + '/' + f if prefix else f)

pathutil.join() could be used in place of if/else.

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -1518,17 +1518,20 @@ 
                 (missing and self.deleted()))
 
     def add(self, list, prefix=""):
-        join = lambda f: os.path.join(prefix, f)
         with self._repo.wlock():
             ui, ds = self._repo.ui, self._repo.dirstate
+            uipath = lambda f: ds.pathto(prefix + '/' + f if prefix else f)
             rejected = []
             lstat = self._repo.wvfs.lstat
             for f in list:
-                scmutil.checkportable(ui, join(f))
+                # ds.pathto() returns an absolute file when this is invoked from
+                # the keyword extension.  That gets flagged as non-portable on
+                # Windows, since it contains the drive letter and colon.
+                scmutil.checkportable(ui, os.path.join(prefix, f))
                 try:
                     st = lstat(f)
                 except OSError:
-                    ui.warn(_("%s does not exist!\n") % join(f))
+                    ui.warn(_("%s does not exist!\n") % uipath(f))
                     rejected.append(f)
                     continue
                 if st.st_size > 10000000:
@@ -1536,13 +1539,13 @@ 
                               "to manage this file\n"
                               "(use 'hg revert %s' to cancel the "
                               "pending addition)\n")
-                              % (f, 3 * st.st_size // 1000000, join(f)))
+                              % (f, 3 * st.st_size // 1000000, uipath(f)))
                 if not (stat.S_ISREG(st.st_mode) or stat.S_ISLNK(st.st_mode)):
                     ui.warn(_("%s not added: only files and symlinks "
-                              "supported currently\n") % join(f))
+                              "supported currently\n") % uipath(f))
                     rejected.append(f)
                 elif ds[f] in 'amn':
-                    ui.warn(_("%s already tracked!\n") % join(f))
+                    ui.warn(_("%s already tracked!\n") % uipath(f))
                 elif ds[f] == 'r':
                     ds.normallookup(f)
                 else:
@@ -1550,12 +1553,13 @@ 
             return rejected
 
     def forget(self, files, prefix=""):
-        join = lambda f: os.path.join(prefix, f)
         with self._repo.wlock():
+            ds = self._repo.dirstate
+            uipath = lambda f: ds.pathto(prefix + '/' + f if prefix else f)
             rejected = []
             for f in files:
                 if f not in self._repo.dirstate:
-                    self._repo.ui.warn(_("%s not tracked!\n") % join(f))
+                    self._repo.ui.warn(_("%s not tracked!\n") % uipath(f))
                     rejected.append(f)
                 elif self._repo.dirstate[f] != 'a':
                     self._repo.dirstate.remove(f)
@@ -1566,9 +1570,10 @@ 
     def undelete(self, list):
         pctxs = self.parents()
         with self._repo.wlock():
+            ds = self._repo.dirstate
             for f in list:
                 if self._repo.dirstate[f] != 'r':
-                    self._repo.ui.warn(_("%s not removed!\n") % f)
+                    self._repo.ui.warn(_("%s not removed!\n") % ds.pathto(f))
                 else:
                     fctx = f in pctxs[0] and pctxs[0][f] or pctxs[1][f]
                     t = fctx.data()
@@ -1581,11 +1586,13 @@ 
         except OSError as err:
             if err.errno != errno.ENOENT:
                 raise
-            self._repo.ui.warn(_("%s does not exist!\n") % dest)
+            self._repo.ui.warn(_("%s does not exist!\n")
+                               % self._repo.dirstate.pathto(dest))
             return
         if not (stat.S_ISREG(st.st_mode) or stat.S_ISLNK(st.st_mode)):
             self._repo.ui.warn(_("copy failed: %s is not a file or a "
-                                 "symbolic link\n") % dest)
+                                 "symbolic link\n")
+                                 % self._repo.dirstate.pathto(dest))
         else:
             with self._repo.wlock():
                 if self._repo.dirstate[dest] in '?':
diff --git a/tests/test-add.t b/tests/test-add.t
--- a/tests/test-add.t
+++ b/tests/test-add.t
@@ -14,6 +14,11 @@ 
   adding a
   $ hg st
   A a
+  $ mkdir dir
+  $ cd dir
+  $ hg add ../a
+  ../a already tracked!
+  $ cd ..
 
   $ echo b > b
   $ hg add -n b