Patchwork [2,of,3,v2] remove: queue warnings until after status messages (issue5140) (API)

login
register
mail settings
Submitter timeless@mozdev.org
Date March 21, 2016, 8:21 p.m.
Message ID <3a5be722f31727d64496.1458591706@waste.org>
Download mbox | patch
Permalink /patch/14017/
State Accepted
Commit a88959ae5938c862a9fc683ee1e6fac091d32654
Headers show

Comments

timeless@mozdev.org - March 21, 2016, 8:21 p.m.
# HG changeset patch
# User timeless <timeless@mozdev.org>
# Date 1458238776 0
#      Thu Mar 17 18:19:36 2016 +0000
# Node ID 3a5be722f31727d64496d00dd5dd11e02bc7bc72
# Parent  9923fa5a10511ddd1207f9019e8d50b29f49aaca
remove: queue warnings until after status messages (issue5140) (API)

Before this change, warnings were interspersed with (and easily drowned out by)
status messages.

API:
abstractsubrepo.removefiles has an extra argument warnings,
into which callees should append their warnings.
  Note: Callees should not assume that there will be items in the list,
  today, I'm lazily including any other subrepos warnings, but
  that may change.

cmdutil.remove has an extra optional argument warnings,
into which it will place warnings.
If warnings is omitted, warnings will be reported via ui.warn()
as before this change (albeit, after any status messages).
Martijn Pieters - March 24, 2016, 10:22 p.m.
On 21 March 2016 at 13:21, timeless <timeless@mozdev.org> wrote:
> # HG changeset patch
> # User timeless <timeless@mozdev.org>
> # Date 1458238776 0
> #      Thu Mar 17 18:19:36 2016 +0000
> # Node ID 3a5be722f31727d64496d00dd5dd11e02bc7bc72
> # Parent  9923fa5a10511ddd1207f9019e8d50b29f49aaca
> remove: queue warnings until after status messages (issue5140) (API)
>
> Before this change, warnings were interspersed with (and easily drowned out by)
> status messages.
>
> API:
> abstractsubrepo.removefiles has an extra argument warnings,
> into which callees should append their warnings.
>   Note: Callees should not assume that there will be items in the list,
>   today, I'm lazily including any other subrepos warnings, but
>   that may change.
>
> cmdutil.remove has an extra optional argument warnings,
> into which it will place warnings.
> If warnings is omitted, warnings will be reported via ui.warn()
> as before this change (albeit, after any status messages).
>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -2393,7 +2393,7 @@
>
>      return ret
>
> -def remove(ui, repo, m, prefix, after, force, subrepos):
> +def remove(ui, repo, m, prefix, after, force, subrepos, warnings=None):
>      join = lambda f: os.path.join(prefix, f)
>      ret = 0
>      s = repo.status(match=m, clean=True)
> @@ -2401,6 +2401,12 @@
>
>      wctx = repo[None]
>
> +    if warnings is None:
> +        warnings = []
> +        warn = True
> +    else:
> +        warn = False
> +

Why the separate `warn` flag? Why not use `if warnings` instead?

>      for subpath in sorted(wctx.substate):
>          def matchessubrepo(matcher, subpath):
>              if matcher.exact(subpath):
> @@ -2414,10 +2420,11 @@
>              sub = wctx.sub(subpath)
>              try:
>                  submatch = matchmod.subdirmatcher(subpath, m)
> -                if sub.removefiles(submatch, prefix, after, force, subrepos):
> +                if sub.removefiles(submatch, prefix, after, force, subrepos,
> +                                   warnings):
>                      ret = 1
>              except error.LookupError:
> -                ui.status(_("skipping missing subrepository: %s\n")
> +                warnings.append(_("skipping missing subrepository: %s\n")
>                                 % join(subpath))
>
>      # warn about failure to delete explicit files/dirs
> @@ -2435,10 +2442,10 @@
>
>          if repo.wvfs.exists(f):
>              if repo.wvfs.isdir(f):
> -                ui.warn(_('not removing %s: no tracked files\n')
> +                warnings.append(_('not removing %s: no tracked files\n')
>                          % m.rel(f))
>              else:
> -                ui.warn(_('not removing %s: file is untracked\n')
> +                warnings.append(_('not removing %s: file is untracked\n')
>                          % m.rel(f))
>          # missing files will generate a warning elsewhere
>          ret = 1
> @@ -2448,16 +2455,16 @@
>      elif after:
>          list = deleted
>          for f in modified + added + clean:
> -            ui.warn(_('not removing %s: file still exists\n') % m.rel(f))
> +            warnings.append(_('not removing %s: file still exists\n') % m.rel(f))
>              ret = 1
>      else:
>          list = deleted + clean
>          for f in modified:
> -            ui.warn(_('not removing %s: file is modified (use -f'
> +            warnings.append(_('not removing %s: file is modified (use -f'
>                        ' to force removal)\n') % m.rel(f))
>              ret = 1
>          for f in added:
> -            ui.warn(_('not removing %s: file has been marked for add'
> +            warnings.append(_('not removing %s: file has been marked for add'
>                        ' (use forget to undo)\n') % m.rel(f))
>              ret = 1
>
> @@ -2473,6 +2480,10 @@
>                  util.unlinkpath(repo.wjoin(f), ignoremissing=True)
>          repo[None].forget(list)
>
> +    if warn:
> +        for warning in warnings:
> +            ui.warn(warning)
> +
>      return ret
>
>  def cat(ui, repo, ctx, matcher, prefix, **opts):
> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
> --- a/mercurial/subrepo.py
> +++ b/mercurial/subrepo.py
> @@ -575,11 +575,13 @@
>      def forget(self, match, prefix):
>          return ([], [])
>
> -    def removefiles(self, matcher, prefix, after, force, subrepos):
> +    def removefiles(self, matcher, prefix, after, force, subrepos, warnings):
>          """remove the matched files from the subrepository and the filesystem,
>          possibly by force and/or after the file has been removed from the
>          filesystem.  Return 0 on success, 1 on any warning.
>          """
> +        warnings.append(_("warning: removefiles not implemented (%s)")
> +                        % self._path)
>          return 1
>
>      def revert(self, substate, *pats, **opts):
> @@ -991,7 +993,7 @@
>                                self.wvfs.reljoin(prefix, self._path), True)
>
>      @annotatesubrepoerror
> -    def removefiles(self, matcher, prefix, after, force, subrepos):
> +    def removefiles(self, matcher, prefix, after, force, subrepos, warnings):
>          return cmdutil.remove(self.ui, self._repo, matcher,
>                                self.wvfs.reljoin(prefix, self._path),
>                                after, force, subrepos)
> diff --git a/tests/test-remove.t b/tests/test-remove.t
> --- a/tests/test-remove.t
> +++ b/tests/test-remove.t
> @@ -277,8 +277,8 @@
>
>    $ rm test/bar
>    $ remove -A test
> +  removing test/bar (glob)
>    not removing test/foo: file still exists (glob)
> -  removing test/bar (glob)
>    exit code: 1
>    R test/bar
>    ./foo
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
timeless - March 25, 2016, 2:58 a.m.
Martijn Pieters wrote:
> Why the separate `warn` flag? Why not use `if warnings` instead?

if you have two subrepos, with this design the first subrepo would be
called with warnings=[], the second would be called with
warnings=[warnings-from-first-subrepo].

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -2393,7 +2393,7 @@ 
 
     return ret
 
-def remove(ui, repo, m, prefix, after, force, subrepos):
+def remove(ui, repo, m, prefix, after, force, subrepos, warnings=None):
     join = lambda f: os.path.join(prefix, f)
     ret = 0
     s = repo.status(match=m, clean=True)
@@ -2401,6 +2401,12 @@ 
 
     wctx = repo[None]
 
+    if warnings is None:
+        warnings = []
+        warn = True
+    else:
+        warn = False
+
     for subpath in sorted(wctx.substate):
         def matchessubrepo(matcher, subpath):
             if matcher.exact(subpath):
@@ -2414,10 +2420,11 @@ 
             sub = wctx.sub(subpath)
             try:
                 submatch = matchmod.subdirmatcher(subpath, m)
-                if sub.removefiles(submatch, prefix, after, force, subrepos):
+                if sub.removefiles(submatch, prefix, after, force, subrepos,
+                                   warnings):
                     ret = 1
             except error.LookupError:
-                ui.status(_("skipping missing subrepository: %s\n")
+                warnings.append(_("skipping missing subrepository: %s\n")
                                % join(subpath))
 
     # warn about failure to delete explicit files/dirs
@@ -2435,10 +2442,10 @@ 
 
         if repo.wvfs.exists(f):
             if repo.wvfs.isdir(f):
-                ui.warn(_('not removing %s: no tracked files\n')
+                warnings.append(_('not removing %s: no tracked files\n')
                         % m.rel(f))
             else:
-                ui.warn(_('not removing %s: file is untracked\n')
+                warnings.append(_('not removing %s: file is untracked\n')
                         % m.rel(f))
         # missing files will generate a warning elsewhere
         ret = 1
@@ -2448,16 +2455,16 @@ 
     elif after:
         list = deleted
         for f in modified + added + clean:
-            ui.warn(_('not removing %s: file still exists\n') % m.rel(f))
+            warnings.append(_('not removing %s: file still exists\n') % m.rel(f))
             ret = 1
     else:
         list = deleted + clean
         for f in modified:
-            ui.warn(_('not removing %s: file is modified (use -f'
+            warnings.append(_('not removing %s: file is modified (use -f'
                       ' to force removal)\n') % m.rel(f))
             ret = 1
         for f in added:
-            ui.warn(_('not removing %s: file has been marked for add'
+            warnings.append(_('not removing %s: file has been marked for add'
                       ' (use forget to undo)\n') % m.rel(f))
             ret = 1
 
@@ -2473,6 +2480,10 @@ 
                 util.unlinkpath(repo.wjoin(f), ignoremissing=True)
         repo[None].forget(list)
 
+    if warn:
+        for warning in warnings:
+            ui.warn(warning)
+
     return ret
 
 def cat(ui, repo, ctx, matcher, prefix, **opts):
diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
--- a/mercurial/subrepo.py
+++ b/mercurial/subrepo.py
@@ -575,11 +575,13 @@ 
     def forget(self, match, prefix):
         return ([], [])
 
-    def removefiles(self, matcher, prefix, after, force, subrepos):
+    def removefiles(self, matcher, prefix, after, force, subrepos, warnings):
         """remove the matched files from the subrepository and the filesystem,
         possibly by force and/or after the file has been removed from the
         filesystem.  Return 0 on success, 1 on any warning.
         """
+        warnings.append(_("warning: removefiles not implemented (%s)")
+                        % self._path)
         return 1
 
     def revert(self, substate, *pats, **opts):
@@ -991,7 +993,7 @@ 
                               self.wvfs.reljoin(prefix, self._path), True)
 
     @annotatesubrepoerror
-    def removefiles(self, matcher, prefix, after, force, subrepos):
+    def removefiles(self, matcher, prefix, after, force, subrepos, warnings):
         return cmdutil.remove(self.ui, self._repo, matcher,
                               self.wvfs.reljoin(prefix, self._path),
                               after, force, subrepos)
diff --git a/tests/test-remove.t b/tests/test-remove.t
--- a/tests/test-remove.t
+++ b/tests/test-remove.t
@@ -277,8 +277,8 @@ 
 
   $ rm test/bar
   $ remove -A test
+  removing test/bar (glob)
   not removing test/foo: file still exists (glob)
-  removing test/bar (glob)
   exit code: 1
   R test/bar
   ./foo