Patchwork D3728: grep: adds unmodified mode

login
register
mail settings
Submitter phabricator
Date June 13, 2018, 12:09 p.m.
Message ID <differential-rev-PHID-DREV-mgnd4fiy4rp6mj44nxx3-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/32105/
State Superseded
Headers show

Comments

phabricator - June 13, 2018, 12:09 p.m.
sangeet259 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  adds an unmodified flag that lets you grep on all files in the revision
  and not just the one that were modified in that changeset

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/cmdutil.py
  mercurial/commands.py
  tests/test-grep.t

CHANGE DETAILS




To: sangeet259, #hg-reviewers
Cc: mercurial-devel
phabricator - June 13, 2018, 12:44 p.m.
sangeet259 added a comment.


  I am updating the tests

REPOSITORY
  rHG Mercurial

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

To: sangeet259, #hg-reviewers
Cc: mercurial-devel
phabricator - June 14, 2018, 1:11 a.m.
mharbison72 added inline comments.

INLINE COMMENTS

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

REPOSITORY
  rHG Mercurial

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

To: sangeet259, #hg-reviewers
Cc: mharbison72, mercurial-devel
Yuya Nishihara - June 14, 2018, 2:39 p.m.
> > 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.
phabricator - June 14, 2018, 3:35 p.m.
yuja added a comment.


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

REPOSITORY
  rHG Mercurial

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

To: sangeet259, #hg-reviewers
Cc: yuja, mharbison72, mercurial-devel
phabricator - June 14, 2018, 7:44 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/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)'.

REPOSITORY
  rHG Mercurial

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

To: sangeet259, #hg-reviewers
Cc: pulkit, yuja, mharbison72, mercurial-devel
phabricator - June 14, 2018, 10:40 p.m.
mharbison72 added a comment.


  In https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

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

To: sangeet259, #hg-reviewers
Cc: pulkit, yuja, mharbison72, mercurial-devel
Sean Farley - June 15, 2018, 12:52 a.m.
Yuya Nishihara <yuya@tcha.org> writes:

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

Yeah, that sounds like a good way forward to me.
phabricator - June 15, 2018, 2:14 p.m.
pulkit added a comment.


  Looks like you forgot to update the commit message.

REPOSITORY
  rHG Mercurial

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

To: sangeet259, #hg-reviewers
Cc: pulkit, yuja, mharbison72, mercurial-devel
phabricator - June 17, 2018, 5:42 a.m.
sangeet259 added a comment.


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

REPOSITORY
  rHG Mercurial

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

To: sangeet259, #hg-reviewers
Cc: pulkit, yuja, mharbison72, mercurial-devel

Patch

diff --git a/tests/test-grep.t b/tests/test-grep.t
--- a/tests/test-grep.t
+++ b/tests/test-grep.t
@@ -368,3 +368,15 @@ 
   binfile.bin:0:+: Binary file matches
 
   $ cd ..
+
+Test for showing working of unmodified flag
+
+  $ hg init sng
+  $ cd sng
+  $ echo "unmod" >> um
+  $ hg ci -A -m "adds unmod to um"
+  adding um
+  $ hg grep -r "-1" "unmod"
+  um:0:unmod
+
+  $ cd ..
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -2408,6 +2408,8 @@ 
     ('n', 'line-number', None, _('print matching line numbers')),
     ('r', 'rev', [],
      _('only search files changed within revision range'), _('REV')),
+    ('', 'unmodified', False,
+     _('include all files in the changeset while grepping')),
     ('u', 'user', None, _('list the author (long with -v)')),
     ('d', 'date', None, _('list the date (short with -q)')),
     ] + formatteropts + walkopts,
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -1881,6 +1881,7 @@ 
     yielding each context, the iterator will first call the prepare
     function on each context in the window in forward order.'''
 
+    unmodified = opts.get('unmodified')
     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 unmodified and len(revs) == 1:
+                            fiter = iter(ctx)
+                        else:
+                            fiter = ctx.files()
+                        for f in fiter:
                             if match(f):
                                 yield f
                     fns = fns_generator()