Page MenuHomePhabricator

remove: print message for each file in verbose mode only while using `-A`
ClosedPublic

Authored by pavanpc on Nov 8 2017, 7:11 AM.

Details

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)

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pavanpc created this revision.Nov 8 2017, 7:11 AM
pavanpc retitled this revision from 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) to HG: hg rm -A option prints the message of every file in the repo.Nov 8 2017, 7:12 AM
pavanpc edited the summary of this revision. (Show Details)

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.

mercurial/cmdutil.py
2977

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: 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

@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.

pavanpc edited the test plan for this revision. (Show Details)Nov 10 2017, 3:52 AM
pavanpc retitled this revision from HG: hg rm -A option prints the message of every file in the repo to Summary: Removing ui.verbose check in the elif comdition..
pavanpc updated this revision to Diff 3390.

@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.

mercurial/cmdutil.py
2977

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

@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

mercurial/cmdutil.py
2998

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.

pavanpc updated this revision to Diff 3391.Nov 10 2017, 9:56 AM
pavanpc retitled this revision from Summary: Removing ui.verbose check in the elif comdition. to cmdutil: hg rm -A option prints the message of every file in the repo.Nov 10 2017, 9:57 AM
pavanpc marked an inline comment as done.Nov 10 2017, 10:02 AM

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

mercurial/cmdutil.py
2998

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

pulkit retitled this revision from cmdutil: hg rm -A option prints the message of every file in the repo to remove: print message for each file in verbose mode only while using `-A`.Nov 10 2017, 12:33 PM

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

mercurial/cmdutil.py
2999

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?

tests/test-remove.t
236

Here's the evidence of the exit code problem.

493

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

500

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.

pavanpc marked an inline comment as done.Nov 11 2017, 10:02 AM
pavanpc updated this revision to Diff 3423.
pavanpc updated this revision to Diff 3424.Nov 11 2017, 10:15 AM

Looks good to me, thanks.

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

pavanpc updated this revision to Diff 3435.Nov 13 2017, 5:34 AM

@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.

mercurial/cmdutil.py
2999

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

lothiraldan accepted this revision.Nov 13 2017, 9:52 AM

@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

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.

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.

@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.

yuja requested changes to this revision.Nov 15 2017, 9:05 AM
yuja added a subscriber: yuja.

@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.

This revision now requires changes to proceed.Nov 15 2017, 9:05 AM
pavanpc added a comment.EditedNov 16 2017, 4:41 PM

@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 suppress warnings ?  
                                                                        or
                                                  always suppress warnings for this case

5. hg rm -Av <dir>                             -  do not suppress warnings

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 ?

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.

yuja added a comment.Nov 17 2017, 8:47 AM
  1. 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.

pavanpc updated this revision to Diff 3621.Nov 17 2017, 5:53 PM

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

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

yuja accepted this revision.Nov 21 2017, 8:06 AM

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

This revision is now accepted and ready to land.Nov 21 2017, 8:06 AM
This revision was automatically updated to reflect the committed changes.