Add gclient grep for git repos

Add gclient grep for git repos

- Adds a gclient grep command to search through git repos.

BUG=157950
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167007

[Isaac (away), 2012-11-07 07:14:53]

Continuing from https://chromiumcodereview.appspot.com/11270056/

In latest patchset I addressed comments, and also added support to print cwd on
git grep commands that have output.  PTAL.

[M-A Ruel, 2012-11-07 14:22:54]

I just ran gclient grep TODO with your CL. It's interesting but I don't see much
value add compared to "gclient recurse -s git git grep $*".

What I'd love is to have the output be reformatted. Instead of printing:

--- CUT HERE ---
$ gclient grep TODO
(...)
win8/metro_driver/toast_notification_handler.cc:  // TODO(ananta)
win8/metro_driver/winrt_utils.cc:    // TODO(robertshield): Get this stuff from
BrowserDistribution.
****** /path/to/src/chrome/src/breakpad/src ******
client/ios/Breakpad.h:// TODO (nealsid): Investigate how necessary this is if we
don't
client/ios/Breakpad.h:// TODO (nealsid): separate server parameter dictionary
from the
(...)
--- CUT HERE ---

Have the file paths to be reformatted based on the gclient checkout root. IMHO
it'd be terrific. E.g.

--- CUT HERE ---
$ gclient grep TODO
(...)
src/win8/metro_driver/toast_notification_handler.cc:  // TODO(ananta)
src/win8/metro_driver/winrt_utils.cc:    // TODO(robertshield): Get this stuff
from BrowserDistribution.
src/breakpad/src/client/ios/Breakpad.h:// TODO (nealsid): Investigate how
necessary this is if we don't
src/breakpad/src/client/ios/Breakpad.h:// TODO (nealsid): separate server
parameter dictionary from the
--- CUT HERE ---

And remove the header thingy. That gives a clear value add and makes
post-processing the output *much* easier.

I was concerned about breaking a few uses cases, so I tried "--context 2",
--count, --heading, --function-context, --show-function, --line-number, and they
all seem to be quite deterministic.

The file path is standalone for --heading, -l --files-with-matches, --name-only,
-L, --files-without-match.
The file path ends with a '-' for --function-context, -W.
The file path ends with either '-' or '=' for --show-function, -p.
The file path ends with a ':' for everything else.
In the case of --context, -C, lines with '--' are inserted.

But all of this doesn't matter because there's --null!

for dep in deps:
  prefix = deps.name
  output = run_git(['grep', '--null'] + args)
  for line in output:
    match = re.match(r'^([^\w]+?)\\0(.*)$', file)
    if match:
      print prefix + match.group(1) + ':' + match.group(2)
    else:
      print line

wdyt? It has the disadvantage of slightly modifying the output, for example
using --show-function will lose the difference between the context and the
match, both '-' and '=' are replaced with '\0' but I don't think it's a big deal
compared to the usability improvement. 

This also makes using --jobs 32 a non issue. "gclient grep TODO | sort" becomes
useful.

[Isaac (away), 2012-11-07 19:50:19]

I considered that and I think it's a nice enhancement.  But it makes the code
more fragile and will also (I assume) be more complex to implement.  I'd rather
see how popular this is first.

The other enhancement I'd like to do is getting recurse to enqueue the dep
commands before running grep on the src repo.

We're migrating to submodules -- next year, perhaps? -- and that would deprecate
this work.  So I'm striving to get 75% of the way rather than 100% given gclient
with git is not forever on this planet.

[Isaac (away), 2012-11-07 21:32:45]

PTAL.  Notes:

- This does work for lines from git grep that say 'Binary file matches'.  Should
I special case them or just ignore?
- I left the changes to CheckCallAndFilterAndHeader() because I might add back
the header and I think they were an improvement is usefulness of the function.
- Still hitting gclient recurse limitation for order in which it enqueues
commands (greps first, then enqueues subcmds).  Can you give me a pointer to
where to fix that?
- I removed default of jobs=1.

[Isaac (away), 2012-11-07 21:36:44]

- sorry, typo: This does not* work for binary file matches

[Isaac (away), 2012-11-08 02:33:10]

OK, fixed the dependency requirements.  Only left is that git 'binary file
matches' output doesn't have full path.  Will probably ignore that.

[Isaac (away), 2012-11-08 03:12:55]

OK, fixed the dependency requirements.  Only left is that git 'binary file
matches' output doesn't have full path.  Will probably ignore that.

[M-A Ruel, 2012-11-08 19:44:13]

https://chromiumcodereview.appspot.com/11312116/diff/8004/gclient.py
File gclient.py (right):

https://chromiumcodereview.appspot.com/11312116/diff/8004/gclient.py#newcode650
gclient.py:650: items = line.split('\0')
if '\0' in line:
...
else:
  items = line.split('\0', 1)
  print ':'.join(os.path.join(self.name, item[0]), item[1])

and ignore any remaining null if any.

https://chromiumcodereview.appspot.com/11312116/diff/8004/gclient_utils.py
File gclient_utils.py (right):

https://chromiumcodereview.appspot.com/11312116/diff/8004/gclient_utils.py#ne...
gclient_utils.py:589: kwargs.get('ignore_requirements')):
I prefer this to be in the state itself, e.g. in the class constructor. It's a
violation of layering of mixing scheduling  tuning and task parameters in
kwargs.

[Isaac (away), 2012-11-09 03:58:52]

https://chromiumcodereview.appspot.com/11312116/diff/8004/gclient.py
File gclient.py (right):

https://chromiumcodereview.appspot.com/11312116/diff/8004/gclient.py#newcode650
gclient.py:650: items = line.split('\0')
On 2012/11/08 19:44:13, Marc-Antoine Ruel wrote:
> if '\0' in line:
> ...
> else:
>   items = line.split('\0', 1)
>   print ':'.join(os.path.join(self.name, item[0]), item[1])
> 
> and ignore any remaining null if any.

I'm confused -- what are you suggesting here?  Will it cover the same use cases
as I have below?

The else clause is needed for git grep --null -l where git does not print
newlines, but instead separates multiple matchs with just a nullbyte.

https://chromiumcodereview.appspot.com/11312116/diff/8004/gclient_utils.py
File gclient_utils.py (right):

https://chromiumcodereview.appspot.com/11312116/diff/8004/gclient_utils.py#ne...
gclient_utils.py:589: kwargs.get('ignore_requirements')):
On 2012/11/08 19:44:13, Marc-Antoine Ruel wrote:
> I prefer this to be in the state itself, e.g. in the class constructor. It's a
> violation of layering of mixing scheduling  tuning and task parameters in
> kwargs.

Seems reasonable.  Fixed.

[M-A Ruel, 2012-11-09 23:18:09]

lgtm. Note that the new functionality doesn't have test so you'll be on the hook
if it breaks silently.

https://chromiumcodereview.appspot.com/11312116/diff/7004/gclient_utils.py
File gclient_utils.py (right):

https://chromiumcodereview.appspot.com/11312116/diff/7004/gclient_utils.py#ne...
gclient_utils.py:591: self.ignore_requirements):
put self.ignore_requirement check first since it's much faster to execute.

[Isaac (away), 2012-11-09 23:36:53]

nit fixed, committing.

Yes I'm aware it's not tested.  We're going to try on the clank team for a while
and then announce if it's stable

https://chromiumcodereview.appspot.com/11312116/diff/7004/gclient_utils.py
File gclient_utils.py (right):

https://chromiumcodereview.appspot.com/11312116/diff/7004/gclient_utils.py#ne...
gclient_utils.py:591: self.ignore_requirements):
On 2012/11/09 23:18:09, Marc-Antoine Ruel wrote:
> put self.ignore_requirement check first since it's much faster to execute.

Done.

[commit-bot: I haz the power, 2012-11-09 23:37:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ilevy@chromium.org/11312116/6006

[commit-bot: I haz the power, 2012-11-09 23:39:50]

Change committed as 167007

[M-A Ruel, 2012-11-09 23:44:10]

On 2012/11/09 23:36:53, Isaac wrote:
> Yes I'm aware it's not tested.  We're going to try on the clank team for a
while
> and then announce if it's stable

np, it's not a core functionality; it won't kill the slaves if it break so I
don't mind. For core functionality where gclient sync is involved it's a
requirement, there it's fine.