( )⚙ D3728 grep: adds allfiles mode

This is an archive of the discontinued Mercurial Phabricator instance.

grep: adds allfiles mode
ClosedPublic

Authored by sangeet259 on Jun 13 2018, 8:08 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGb8f45fc27370: grep: adds allfiles mode
Summary

adds an allfiles flag that lets you grep on all files in the revision
and not just the one that were modified in that changeset.
This would work on a single revision and get all the files that were
there in that revision. So it's like grepping on a previous state.
Using this with wdir() :: hg grep -r "wdir()" --allfiles is what the
default behaviour is desired for grep.

Support for multiple revisions to be added later

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

sangeet259 created this revision.Jun 13 2018, 8:08 AM

I am updating the tests

sangeet259 updated this revision to Diff 9049.Jun 13 2018, 9:01 AM
sangeet259 updated this revision to Diff 9050.Jun 13 2018, 9:20 AM
mharbison72 added inline comments.
mercurial/commands.py
2406

I wonder if --allfiles is a better name. --unmodified makes me think unmodified files exclusively.

It's too bad --all is already used, because that would be consistent with the all files meaning in status and revert. (Maybe others?) The plan page mentions BCing plain grep into oblivion, and deprecating --all. Is it worth a BC on this too, for consistency going forward?

yuja added a subscriber: yuja.Jun 14 2018, 11:35 AM

commands.py:2411

_('only search files changed within revision range'), _('REV')),

+ ('', 'unmodified', False,
+ _('include all files in the changeset while grepping')),

I wonder if --allfiles is a better name. --unmodified makes me think unmodified files exclusively.
It's too bad --all is already used, because that would be consistent with the all files meaning in status and revert. (Maybe others?) The plan page mentions BCing plain grep into oblivion, and deprecating --all. Is it worth a BC on this too, for consistency going forward?

Maybe we can keep all new flags hidden as "(EXPERIMENTAL)" until we make
the final BC? I don't think it's time for bikeshedding yet.

pulkit added a subscriber: pulkit.Jun 14 2018, 3:44 PM
In D3728#58540, @yuja wrote:

commands.py:2411

_('only search files changed within revision range'), _('REV')),

+ ('', 'unmodified', False,
+ _('include all files in the changeset while grepping')),

I wonder if --allfiles is a better name. --unmodified makes me think unmodified files exclusively.
It's too bad --all is already used, because that would be consistent with the all files meaning in status and revert. (Maybe others?) The plan page mentions BCing plain grep into oblivion, and deprecating --all. Is it worth a BC on this too, for consistency going forward?

Maybe we can keep all new flags hidden as "(EXPERIMENTAL)" until we make
the final BC? I don't think it's time for bikeshedding yet.

I agree with Yuya and Matt both. How about renaming the flag to --allfiles and mark that as experimental. To mark a flag as experimental, append the help text with '(EXPERIMENTAL)'.

In D3728#58540, @yuja wrote:

commands.py:2411

_('only search files changed within revision range'), _('REV')),

+ ('', 'unmodified', False,
+ _('include all files in the changeset while grepping')),

I wonder if --allfiles is a better name. --unmodified makes me think unmodified files exclusively.
It's too bad --all is already used, because that would be consistent with the all files meaning in status and revert. (Maybe others?) The plan page mentions BCing plain grep into oblivion, and deprecating --all. Is it worth a BC on this too, for consistency going forward?

Maybe we can keep all new flags hidden as "(EXPERIMENTAL)" until we make
the final BC? I don't think it's time for bikeshedding yet.

Agreed. I haven't paid close attention to this, so I wasn't sure what was left. But I wanted to flag it before I forgot.

sangeet259 updated this revision to Diff 9092.Jun 15 2018, 10:12 AM

Looks like you forgot to update the commit message.

yuja added a comment.Jun 16 2018, 9:17 AM

+ allfiles = opts.get('allfiles')

follow = opts.get('follow') or opts.get('follow_first')
revs = _walkrevs(repo, opts)
if not revs:

@@ -1990,7 +1991,11 @@

ctx = change(rev)
if not fns:
    def fns_generator():
  • for f in ctx.files():

+ if allfiles and len(revs) == 1:

Can you make it error out if len(revs) > 1 isn't supported?
For example,

allfiles = opts.get('allfiles')
follow = opts.get('follow') or opts.get('follow_first')
revs = _walkrevs(repo, opts)
if allfiles and len(revs) > 1:
    raise error.Abort(_(...))  # or ProgrammingError if that never happens
sangeet259 updated this revision to Diff 9107.Jun 16 2018, 9:45 AM
sangeet259 edited the summary of this revision. (Show Details)Jun 16 2018, 10:46 AM
sangeet259 retitled this revision from grep: adds unmodified mode to grep: adds allfiles mode.
yuja added a comment.Jun 16 2018, 11:37 PM

+ $ hg init sng
+ $ cd sng
+ $ echo "unmod" >> um
+ $ hg ci -A -m "adds unmod to um"
+ adding um
+ $ echo "something else" >> new
+ $ hg ci -A -m "second commit"
+ adding new
+ $ hg grep -r "." "unmod"
+ [1]
+ $ hg grep -r "." "unmod" --unmodified

--allfiles ?

@@ -1881,6 +1881,9 @@

yielding each context, the iterator will first call the prepare
function on each context in the window in forward order.'''

+ allfiles = opts.get('allfiles')
+ if allfiles and len(revs) > 1:
+ raise error.Abort(_("multiple revisions not supported with --allfiles"))

revs isn't defined yet.

follow = opts.get('follow') or opts.get('follow_first')
revs = _walkrevs(repo, opts)
if not revs:

@yuja Sorry I didn't run the tests after this edit .

sangeet259 updated this revision to Diff 9121.Jun 17 2018, 1:50 AM
yuja added a comment.Jun 17 2018, 4:46 AM

+ if allfiles and len(revs) > 1:
+ raise error.Abort(_("multiple revisions not supported with --allfiles"))

wanted = set()
slowpath = match.anypats() or (not match.always() and opts.get('removed'))
fncache = {}

@@ -1990,7 +1994,11 @@

ctx = change(rev)
if not fns:
    def fns_generator():
  • for f in ctx.files():

+ if allfiles and len(revs) == 1:

Removed len(revs) == 1 here and queued, thanks.

This revision was automatically updated to reflect the committed changes.