Merge lp:~parthm/bzr/503670-vila-grep-as-builtin into lp:bzr

[Parth Malwankar]

[Parth Malwankar (parthm)]

Bug #503670

This patch ports the Vincent's grep plugin https://code.launchpad.net/~vila/bzr/grep
to builtins. The grep command is now available as 'bzr grep PATTERN [-- options]'. All grep options are support (followed by '--').
The current implementation requires xargs and grep commands to be available.

This command uses an implementation of os.path.relpath (ported from python 2.6 as cmd_grep._relpath). I tried using osutils.relpath but I found the two to be
inconsistent with the python 2.6 version in argument ordering, having optional arg and handling of '.'. As I don't fully understand the use cases of osutils.relpath I decided to keep cmd_grep._relpath.

Note on windows usage.
I tried this on a windows system with http://unxutils.sourceforge.net/ installed.
The command silently existed for me even though xargs and grep are available. It may be a good idea to disable this command on non-POSIX OSes. We could add a new error type. Comments/suggestions?

Example usage:
[bzrlib]% ../bzr --no-plugins grep main_version -- -i
__init__.py: main_version = '%d.%d' % version_info[:2]
__init__.py: main_version = '%d.%d.%d' % version_info[:3]
__init__.py: return main_version
__init__.py: return main_version + sub_string
tests/blackbox/test_version.py: def test_main_version(self):
[bzrlib]%

[Robert Collins (lifeless)]

 review: needsfixing

So this seems to have very few tests and not work on windows. I don't
think its good enough to merge to core yet.

Also as this doesn't look in history at all, there is really little
benefit in putting it in the core unless its going to make windows users
lives easier (which it won't). Right now its little more than 'bzr ls |
xargs greap'.

I rather suspect you'd be better of writing one from scratch with full
tests at appropriate unit and blackbox layers, to get windows support
and test coverage.

-Rob

review: Needs Fixing

[Martin Packman (gz)]

As a windows user, I have no problem with the idea of a `bzr grep` that depends on an external grep tool, I'd just want you to lose the xargs dependency.

The -- thing is ugly, why not just pull out the bazaar-specific arguments from the list given and pass the rest onto the external tool?

review: Needs Fixing

[Parth Malwankar (parthm)]

> As a windows user, I have no problem with the idea of a `bzr grep` that
> depends on an external grep tool, I'd just want you to lose the xargs
> dependency.
>

Having grep.exe shipped with bzr is one option. But for something like history
and view support, we would still need to do use bzrlib API to filter files,
get text of older versions and pipe it to grep. And as you mentioned below, I also
think '--' looks quite ugly so it might be good to process options via bzr's
command line processing.

I feel going with 'bzr ls [opts] | grep' may be a better option than the above. For history we would probably need to use 'bzr cat'?

Considering the above, IMO going with a plugin gives us more flexibility (e.g the --from-root option in the plugin) along with the possibility of easily enhancing it in the future.

> The -- thing is ugly, why not just pull out the bazaar-specific arguments from
> the list given and pass the rest onto the external tool?

I am wondering if there is a consensus on the approach to bug #503670.
The options which seem ok to me are:
 (1) 'bzr ls [opts] | grep': bzr could [based on user selection] install grep.exe on windows. 'bzr cat' could be used for history search.
 (2) self contained grep [a]: This should probably be a plugin (at least to start with).
 (3) ???
Whatever option is chosen should have support for views and history IMO.

Once there is some agreement on the approach thats preferred, we should be
able to close bug #503670

[a] https://code.launchpad.net/~parthm/bzr/503670-grep-plugin/+merge/20067

[Martin Pool (mbp)]

I think it's reasonable to have a selfcontained grep, so that
 * it can efficiently search history
 * it's easier for windows users

Building on Python's re module a basic grep should be very small.

I don't strongly care if this is in a plugin or core. I don't think
core needs to be minimal and I don't think it needs to be maximal and
merge every useful feature.

It would be nice to also have the option to pipe files through an
external grep so people can get extra features from grep.
--
Martin <http://launchpad.net/~mbp/>

[Parth Malwankar (parthm)]

> It would be nice to also have the option to pipe files through an
> external grep so people can get extra features from grep.
> --
> Martin <http://launchpad.net/~mbp/>

Sounds like a good idea. '--cat-only'.

[Martin Pool (mbp)]

On 26 February 2010 16:54, Parth Malwankar <email address hidden> wrote:
>
>
>> It would be nice to also have the option to pipe files through an
>> external grep so people can get extra features from grep.
>> --
>> Martin <http://launchpad.net/~mbp/>
>
> Sounds like a good idea. '--cat-only'.

Well, actually I meant --external-grep=PROG, and then that can be run
for each file, using eg gnu grep's --label argument to indicate which
version of which file it is.

--
Martin <http://launchpad.net/~mbp/>

[Parth Malwankar (parthm)]

This proposal should probably be rejected over lp:bzr-grep.

review: Disapprove

Unmerged revisions

5054. By Parth Malwankar on 2010-02-22

ported tests

5053. By Parth Malwankar on 2010-02-22

move grep command into core (from plugin https://code.launchpad.net/~vila/bzr/grep)

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-02-15 20:23:34 +0000
+++ bzrlib/builtins.py	2010-02-22 11:16:18 +0000
@@ -24,6 +24,7 @@
 import cStringIO
 import sys
 import time
+import subprocess
 
 import bzrlib
 from bzrlib import (
@@ -5916,6 +5917,132 @@
         for path, location in sorted(ref_list):
             self.outf.write('%s %s\n' % (path, location))
 
+class cmd_grep(Command):
+    """Grep through your working tree, but only files managed by bzr.
+
+    By default this is a recursive grep, unless you specify --non-recursive.
+
+    To pass options to grep use "--" to signal the end of options to bzr, eg:
+
+    # bzr grep pattern -- -i file
+
+    This does a case-insensitive grep on file.
+    """
+
+    takes_args = ['pattern', 'file*']
+    takes_options = [
+        Option(
+            'non-recursive',
+            help="Don't recursively grep the contents of directories."),
+        ]
+
+    # TODO: Separate file selection from file processing
+    # TODO: avoid xargs completely ?
+    # TODO: separate file selection from file processing to get a true streamed
+    # process (everything is is buffered so far, inputs as well as outputs and will
+    # blow up for big trees one day or the other)
+    @display_command
+    def run(self, pattern, non_recursive=False, file_list=None):
+
+        curdir = osutils.getcwd()
+
+        options, file_list = self._read_options(file_list)
+
+        versioned = 'V'
+
+        file_list = tree_files(file_list)[1]
+        tree, b, reldir = bzrdir.BzrDir.open_containing_tree_or_branch('.')
+        if reldir:
+            reldir += '/'
+        tree.lock_read()
+
+        try:
+            cmd = ['xargs', '-0', 'grep', '-e', pattern]
+            cmd.extend(options)
+
+            xargs = subprocess.Popen(cmd, stdin=subprocess.PIPE,
+                                     stdout=subprocess.PIPE,
+                                     stderr=subprocess.PIPE,
+                                     )
+
+            for fpath, fclass, fkind, fid, entry \
+                    in tree.list_files(include_root=False):
+                # XXX: this selection part is messy, more tests are needed and
+                # a better flow too
+                if fkind == 'directory':
+                    continue
+                if file_list:
+                    if non_recursive:
+                        if not fpath in file_list:
+                            continue
+                    elif not osutils.is_inside_any(file_list, fpath):
+                        continue
+                elif reldir and not fpath.startswith(reldir):
+                    continue
+                if (non_recursive and reldir
+                    and fpath.startswith(reldir)
+                    and '/' in fpath[len(reldir):]):
+                    continue
+                if fclass != versioned:
+                    continue
+                rpath = self._relpath(tree.abspath(fpath))
+                xargs.stdin.write(rpath + '\0')
+            xargs.stdin.flush()
+            out, err = xargs.communicate()
+            if out:
+                sys.stdout.write(out)
+            if err:
+                sys.stderr.write(err)
+        finally:
+            tree.unlock()
+
+    def _read_options(self, list):
+        # This is a bit of a hack, but if we scan the file list for
+        # options we  can do:
+        #   bzr grep foo -- -i file
+        # and pass the -i through to grep.
+
+        if list is None:
+            return [], []
+
+        files = []
+        options = []
+
+        while len(list) > 0:
+            item = list.pop(0)
+
+            if item.startswith('-'):
+                options.append(item)
+                if item in ['-A', '-B', '-C', '-m']:
+                    options.append(list.pop(0))
+            else:
+                files.append(item)
+
+        return options, files
+
+    # Borrowed from python2.6 until we get there
+    def _relpath(self, path, start=os.path.curdir):
+        """Return a relative version of a path.
+
+        This a slighty simplified and modified version of the 2.6
+        version. Windows specific error handling deleted since we always calls
+        this function for paths on the same volume.
+        """
+
+        if not path:
+            raise ValueError("no path specified")
+
+        start_list = os.path.abspath(start).split(os.path.sep)
+        path_list = os.path.abspath(path).split(os.path.sep)
+
+        # Work out how much of the filepath is shared by start and path.
+        i = len(os.path.commonprefix([start_list, path_list]))
+
+        rel_list = [os.path.pardir] * (len(start_list)-i) + path_list[i:]
+        return os.path.join(*rel_list)
+
+
+
 
 # these get imported and then picked up by the scan for cmd_*
 # TODO: Some more consistent way to split command definitions across files;

=== modified file 'bzrlib/tests/blackbox/__init__.py'
--- bzrlib/tests/blackbox/__init__.py	2009-04-29 20:31:34 +0000
+++ bzrlib/tests/blackbox/__init__.py	2010-02-22 11:16:18 +0000
@@ -60,6 +60,7 @@
                      'bzrlib.tests.blackbox.test_filesystem_cicp',
                      'bzrlib.tests.blackbox.test_filtered_view_ops',
                      'bzrlib.tests.blackbox.test_find_merge_base',
+                     'bzrlib.tests.blackbox.test_grep',
                      'bzrlib.tests.blackbox.test_help',
                      'bzrlib.tests.blackbox.test_hooks',
                      'bzrlib.tests.blackbox.test_ignore',

=== added file 'bzrlib/tests/blackbox/test_grep.py'
--- bzrlib/tests/blackbox/test_grep.py	1970-01-01 00:00:00 +0000
+++ bzrlib/tests/blackbox/test_grep.py	2010-02-22 11:16:18 +0000
@@ -0,0 +1,55 @@
+# Copyright (C) 2010 Canonical Ltd
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+import os
+
+from bzrlib.tests import TestCaseWithTransport
+
+class TestGrepInDirs(TestCaseWithTransport):
+
+    def setUp(self):
+        super(TestGrepInDirs, self).setUp()
+        tree = self.make_branch_and_tree('.')
+        file_list = ['top', 'subdir/', 'subdir/down']
+        self.build_tree(file_list)
+        tree.add(file_list)
+        tree.commit('setup')
+
+    def assertGrepResultIs(self, expected_output, grep_args,
+                           working_dir='.',
+                           expected_error_re='.*'):
+        out, err = self.run_bzr(['grep'] + grep_args, working_dir=working_dir)
+        self.assertEquals(expected_output, out)
+        self.assertContainsRe(err, expected_error_re)
+
+    def test_grep_in_root_dir(self):
+        self.assertGrepResultIs('subdir/down:contents of subdir/down\n'
+                                'top:contents of top\n' ,
+                                ['contents'])
+        self.assertGrepResultIs('contents of top\n', ['top', 'top'])
+        self.assertGrepResultIs('contents of subdir/down\n', ['down',
+                                                              'subdir/down'])
+
+    def test_grep_in_subir(self):
+        self.assertGrepResultIs('contents of subdir/down\n', ['down', 'down'],
+                                working_dir='subdir')
+        self.assertGrepResultIs('contents of top\n', ['top', '../top'],
+                                working_dir='subdir')
+
+    def test_grep_in_subdir_doesnt_go_up(self):
+        self.assertGrepResultIs('contents of subdir/down\n', ['contents'],
+                                working_dir='subdir')
+