Patchwork D1336: Summary: hg rm -A option prints the message of every file in the repo. This is not very user friendly for a big repository with thousands of files. So enabling this feature only when run in --verbose mode (hg rm -Av)

login
register
mail settings
Submitter phabricator
Date Nov. 8, 2017, 12:11 p.m.
Message ID <differential-rev-PHID-DREV-rhwnwujvr5mw6jl5e2pz-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/25413/
State Superseded
Headers show

Comments

phabricator - Nov. 8, 2017, 12:11 p.m.
pavanpc created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

TEST PLAN
  check if the message is printed in verbose mode.
  
    cd  hg-crew/tests
    rt test-remove.t

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

AFFECTED FILES
  mercurial/cmdutil.py
  tests/test-remove.t

CHANGE DETAILS




To: pavanpc, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 9, 2017, 3:48 a.m.
mharbison72 added a comment.


  Thank you for the proposed change.  Does --quiet work for you?
  
  I don't think I necessarily agree with the premise here.  I think the only way every file in the repo is printed is if you delete every file first.  Many file operations like add, remove/forget, and addremove all follow the style of quiet if files are explicitly named, and verbose for inexact matches.  I've come to rely on that to catch mistakes when I'm not explicit about file paths.  I'm not sure if it's worse to dump a ton of output to the screen, or not consistently follow the rules.
  
  I'm sure others will chime in, but in the meantime, there's an inline comment too.

INLINE COMMENTS

> cmdutil.py:2970
>          list = modified + deleted + clean + added
> -    elif after:
> +    elif after or ui.verbose:
>          list = deleted

Should ui.verbose be nested inside the elif instead?  The -v without -A case would have taken the 'else' case before, so this change seems to affect other things unintentionally.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers
Cc: mharbison72, mercurial-devel
phabricator - Nov. 9, 2017, 11:56 a.m.
mitrandir added a comment.


  @mharbison72: the problem is that `hg rm -A` prints a line for every file in the repo even if you didn't touch anything. With a clean working directory in a repo with 10k files, 10k lines will be printed. The lines look like:
  
    not removing <filename>: file still exists

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers
Cc: mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 10, 2017, 3:17 a.m.
mharbison72 added a comment.


  @mitrandir Oh, sorry, I see what you mean now.
  
  I wonder if the nice thing for people used to the current behavior is to terse the directories that don't have deleted files.  That way the message is still there, but the sheer volume is lower.  If an entire subtree is excluded, only the highest directory level is mentioned.
  
    not removing dirname/: no deleted files
    not removing dirname2/: no deleted files
    not removing dirname3/subdir/filename: file still exists
    ...
  
  But for just the 'still exists' warnings, I guess I don't feel that strongly.  Status can always be checked.  I do think the first added ui.verbose check is wrong though.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers
Cc: mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 10, 2017, 8:56 a.m.
pavanpc added a comment.


  @mharbison72  Thank you for reviewing this diff. The diff checks for "not removing <filename>: file still exists". I have removed the ui.verbose check from elif.

INLINE COMMENTS

> mharbison72 wrote in cmdutil.py:2970
> Should ui.verbose be nested inside the elif instead?  The -v without -A case would have taken the 'else' case before, so this change seems to affect other things unintentionally.

@mharbison72  We should remove the ui.verbose check and check for verbose inside elif. I have made the change.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers
Cc: mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 10, 2017, 12:48 p.m.
mharbison72 added a comment.


  @pavanpc this might just be me misreading phabricator, but you will want your summary to reflect the overall change, because all of the iterations on a single patch will be queued as a single commit. So maybe something like:
  
  remove: change the extant file warning with -A to verbose output
  
  Otherwise, there’s output for every file in the path on which the operation occurs.
  
  ——— (end of commit message) ———
  
  See here for formatting requirements: https://www.mercurial-scm.org/wiki/ContributingChanges

INLINE COMMENTS

> cmdutil.py:2981
> +                        % m.rel(f))
> +                ret = 1
> +            ui.progress(_('skipping'), None)

Sorry I missed this before, but it looks like the return code is changed based on if this case happens. So the “ret = 1” line needs to happen outside of the ui.verbose check.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers
Cc: mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 10, 2017, 3:02 p.m.
pavanpc marked an inline comment as done.
pavanpc added a comment.


  @mharbison72  Thank you for suggesting that. I have updated the phabricator titile.

INLINE COMMENTS

> mharbison72 wrote in cmdutil.py:2981
> Sorry I missed this before, but it looks like the return code is changed based on if this case happens. So the “ret = 1” line needs to happen outside of the ui.verbose check.

@mharbison72  I have pulled ret = 1 out of ui.verbose check.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers
Cc: mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 11, 2017, 1:12 a.m.
mharbison72 added a comment.


  @pavanpc Looks pretty good- just a few things that popped up in the last revision.

INLINE COMMENTS

> cmdutil.py:2982
> +            ui.progress(_('skipping'), None)
> +        ret = 1
>      else:

This will need to be protected by seeing if there are any files in modified + added + clean, like it was before.  Otherwise, using -A will always return non zero, even if it succeeded without warning cases.  Maybe hoist the 'remaining = ...' line out of the conditional?

> test-remove.t:236
>                                                                \r (no-eol) (esc)
> -  exit code: 0
> +  exit code: 1
>    R foo

Here's the evidence of the exit code problem.

> test-remove.t:451
>    $ rm d1/a
> -  $ hg rm --after d1
> +  $ hg rm --after   d1
>    \r (no-eol) (esc)

Please avoid unnecessary changes like this.  It may seem trivial, but if everyone did it, it would make annotating more of a chore.

> test-remove.t:458
>                                                                \r (no-eol) (esc)
> -  removing d1/a (glob)
> +  removing d1/a
> +  [1]

I don't see any reason for the glob to go away.  I suspect it may have been due to the exit code changing in the next line.  In general, be careful about not dropping these.  The test runner generally doesn't stick them in when run on anything but Windows, and not having them causes test failures on Windows.  It's a work around for Windows printing 'removing d1\a' on this same line.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers
Cc: mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 11, 2017, 3:48 p.m.
mharbison72 added a comment.


  Looks good to me, thanks.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers
Cc: mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 13, 2017, 7:42 a.m.
lothiraldan added a comment.


  Could we have a test for `hg rm -A` without the verbose mode?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers
Cc: lothiraldan, mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 13, 2017, 10:40 a.m.
pavanpc added a comment.


  @lothiraldan  I modified a test case for hg rm -A <filename> case where we get the message 'not removing <filename>: file still exists' . For other cases, test cases already exist with 'hg rm -A' and 'hg rm --after' without verbose mode.

INLINE COMMENTS

> mharbison72 wrote in cmdutil.py:2982
> This will need to be protected by seeing if there are any files in modified + added + clean, like it was before.  Otherwise, using -A will always return non zero, even if it succeeded without warning cases.  Maybe hoist the 'remaining = ...' line out of the conditional?

Moved the 'remaining ' outside the conditional. Making the ui.verbose check only while adding it to the warnings list

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers
Cc: lothiraldan, mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 13, 2017, 2:52 p.m.
lothiraldan accepted this revision.
lothiraldan added a comment.


  In https://phab.mercurial-scm.org/D1336#22841, @pavanpc wrote:
  
  > @lothiraldan  I modified a test case for hg rm -A <filename> case where we get the message 'not removing <filename>: file still exists' . For other cases, test cases already exist with 'hg rm -A' and 'hg rm --after' without verbose mode.
  
  
  Thx, LGTM

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers, lothiraldan
Cc: lothiraldan, mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 13, 2017, 10:22 p.m.
durin42 added a comment.


  I think this probably violates our BC policy. We could probably stand to add a `--quiet` for suppressing this output, I suppose.
  
  I'll leave this open for other reviewers to mull over, and if it's still open in a week without comment I'll probably pass final judgement.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers, lothiraldan
Cc: durin42, lothiraldan, mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 14, 2017, 1:58 a.m.
mharbison72 added a comment.


  If this can't be taken as-is, what about something in tweakdefaults to hide this by default?  I just assumed that --quiet already worked, and the problem was that this throws away the other more useful warnings in the non --after case.  (Or people don't remember to use --quiet before running it.)
  
  FWIW, I watched someone run `hg rm -A` on a repo with 67K files on Friday, and that made me much more sympathetic.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers, lothiraldan
Cc: durin42, lothiraldan, mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 14, 2017, 11:09 a.m.
mitrandir added a comment.


  @durin42: I'd say that listing *all the files in the repo* when you run `hg rm -A` should be considered a bug. This can't possibly be the intended behavior. Also: running `hg rm -A` without a `file` argument is not even covered by any testcase.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers, lothiraldan
Cc: durin42, lothiraldan, mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 15, 2017, 2:05 p.m.
yuja requested changes to this revision.
yuja added a comment.
This revision now requires changes to proceed.


  In https://phab.mercurial-scm.org/D1336#23145, @mitrandir wrote:
  
  > @durin42: I'd say that listing *all the files in the repo* when you run `hg rm -A` should be considered a bug.
  
  
  Agreed. Only explicitly-specified files should be warned by default. And suppressing
  all warnings seems wrong as Augie said and is BC.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers, lothiraldan, yuja
Cc: yuja, durin42, lothiraldan, mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 16, 2017, 9:41 p.m.
pavanpc added a comment.


  @yuja @mitrandir
  
  I can think of below cases. Could you please check and let me know your thoughts on these?
  
  1. hg rm -A                                    -  suppress warnings
  2. hg rm -Av                                  -  do not suppress warnings (could be helpful for small repos)
  3. hg rm -A <file1> <file2>          -  do not suppress warnings
  4. hg rm -A  <dir>                        -  What should be the behavior here? What if the directory has thousands of
  
    						         files . May be we can suppress warnings based on number of files in the warnings list, like if len(warnings_list) > 25 supprress warnings ?  
                                                                         or  
                                                             always suppress warnings for this case
  
  5. hg rm -Av <dir>                      -  do not suppress warnings

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers, lothiraldan, yuja
Cc: yuja, durin42, lothiraldan, mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 17, 2017, 11:02 a.m.
mitrandir added a comment.


  I think that the cases mentioned by you are right :) In case of directory I'd prefer so suppress warns. What do you think @yuja ?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers, lothiraldan, yuja
Cc: yuja, durin42, lothiraldan, mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 17, 2017, 12:38 p.m.
mharbison72 added a comment.


  I guess the other case is when -I PATTERN or -X PATTERN are used instead of file names.  These will be inexact matches like when a directory is specified.  I don’t like the dual behavior controlled by how many files are in the directory, because the it will seem inconsistent.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers, lothiraldan, yuja
Cc: yuja, durin42, lothiraldan, mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 17, 2017, 1:47 p.m.
yuja added a comment.


  In https://phab.mercurial-scm.org/D1336#23872, @pavanpc wrote:
  
  > 4. hg rm -A  <dir>                             -  What should be the behavior here? What if the directory has thousands of  files . May be we can suppress warnings based on number of files in the warnings list, like if len(warnings_list) > 25 suppress warnings ? or always suppress warnings for this case
  
  
  I think "always suppress" is the right thing to do because I would run `hg rm -A <dir>`
  to record deleted files under the directory. In matcher, I think only `m.files()` needs to
  be warned.
  
  > -I PATTERN or -X PATTERN
  
  I see these patterns are not "explicit" files, so won't be warned.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers, lothiraldan, yuja
Cc: yuja, durin42, lothiraldan, mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 17, 2017, 11:03 p.m.
pavanpc added a comment.


  @yuja Thank you for your input. Now, warnings are suppressed for the directory case. Also, I have added test cases for the above cases.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers, lothiraldan, yuja
Cc: yuja, durin42, lothiraldan, mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 21, 2017, 11:10 a.m.
mitrandir added a comment.


  @durin42: I think it should be as good as possible BC-wise. What do you think?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers, lothiraldan, yuja
Cc: yuja, durin42, lothiraldan, mitrandir, mharbison72, mercurial-devel
phabricator - Nov. 21, 2017, 1:06 p.m.
yuja accepted this revision.
yuja added a comment.
This revision is now accepted and ready to land.


  Queued, thanks. I've added "(BC)" to the commit message.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1336

To: pavanpc, #hg-reviewers, lothiraldan, yuja
Cc: yuja, durin42, lothiraldan, mitrandir, mharbison72, mercurial-devel

Patch

diff --git a/tests/test-remove.t b/tests/test-remove.t
--- a/tests/test-remove.t
+++ b/tests/test-remove.t
@@ -168,11 +168,11 @@ 
                                                               \r (no-eol) (esc)
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
 
-20 state added, options -A
+20 state added, options -Av
 
   $ echo b > bar
   $ hg add bar
-  $ remove -A bar
+  $ remove -Av bar
   \r (no-eol) (esc)
   deleting [===========================================>] 1/1\r (no-eol) (esc)
                                                               \r (no-eol) (esc)
@@ -189,9 +189,9 @@ 
                                                               \r (no-eol) (esc)
   0 files updated, 0 files merged, 0 files removed, 0 files unresolved
 
-21 state clean, options -A
+21 state clean, options -Av
 
-  $ remove -A foo
+  $ remove -Av foo
   \r (no-eol) (esc)
   deleting [===========================================>] 1/1\r (no-eol) (esc)
                                                               \r (no-eol) (esc)
@@ -205,10 +205,10 @@ 
   ./foo
   0 files updated, 0 files merged, 0 files removed, 0 files unresolved
 
-22 state modified, options -A
+22 state modified, options -Av
 
   $ echo b >> foo
-  $ remove -A foo
+  $ remove -Av foo
   \r (no-eol) (esc)
   deleting [===========================================>] 1/1\r (no-eol) (esc)
                                                               \r (no-eol) (esc)
@@ -357,10 +357,10 @@ 
                                                               \r (no-eol) (esc)
   2 files updated, 0 files merged, 0 files removed, 0 files unresolved
 
-dir, options -A
+dir, options -Av
 
   $ rm test/bar
-  $ remove -A test
+  $ remove -Av test
   \r (no-eol) (esc)
   deleting [===========================================>] 1/1\r (no-eol) (esc)
                                                               \r (no-eol) (esc)
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -2967,18 +2967,19 @@ 
 
     if force:
         list = modified + deleted + clean + added
-    elif after:
+    elif after or ui.verbose:
         list = deleted
-        remaining = modified + added + clean
-        total = len(remaining)
-        count = 0
-        for f in remaining:
-            count += 1
-            ui.progress(_('skipping'), count, total=total, unit=_('files'))
-            warnings.append(_('not removing %s: file still exists\n')
-                    % m.rel(f))
-            ret = 1
-        ui.progress(_('skipping'), None)
+        if ui.verbose:
+            remaining = modified + added + clean
+            total = len(remaining)
+            count = 0
+            for f in remaining:
+                count += 1
+                ui.progress(_('skipping'), count, total=total, unit=_('files'))
+                warnings.append(_('not removing %s: file still exists\n')
+                        % m.rel(f))
+                ret = 1
+            ui.progress(_('skipping'), None)
     else:
         list = deleted + clean
         total = len(modified) + len(added)