D2938: grep: make grep search on working directory by default

[martinvonz (Martin von Zweigbergk)]

martinvonz added inline comments.

INLINE COMMENTS

> sangeet259 wrote in commands.py:2584
> I could have made the call by simply passing `''` instead of `opts.get('rev')` but I chose this because I am planning to build upon this to handle revisions as well.

I agree with Pulkit: it's clearer to change this to

  ctx = repo[None]
  m = scmutil.match(ctx, pats, opts, default='relglob')

for now and start calling revsingle() in a later patch

> commands.py:2575
>      fm = ui.formatter('grep', opts)
> -    for ctx in cmdutil.walkchangerevs(repo, match, opts, prep):
> -        rev = ctx.rev()
> -        parent = ctx.p1().rev()
> -        for fn in sorted(revfiles.get(rev, [])):
> -            states = matches[rev][fn]
> -            copy = copies.get(rev, {}).get(fn)
> -            if fn in skip:
> -                if copy:
> -                    skip[copy] = True
> +    # This if part handles the default situation,\
> +    # when nothing is passed in -r or --all

nit: can you change 'if part' to 'if-part' or '"if" part' to make it easier to parse? Also drop the trailing backslash.

Actually, the comment doesn't seem to add any useful information, so I'd suggest either removing it or changing it to something like "When neither -r nor --all is passed, search the working copy" (if that's accurate)

> commands.py:2577
> +    # when nothing is passed in -r or --all
> +    if not (opts.get('rev')  or opts.get('all')):
> +        rev = scmutil.revsingle(repo, opts.get('rev'), None).node()

drop one of the spaces before "or"

> commands.py:2585
> +        for fn in ctx.matches(m):
> +            if rev is None and ds[fn] == 'r':
>                  continue

can you drop "rev is None" for now (since it's always true)?

> commands.py:2585
> +        for fn in ctx.matches(m):
> +            if rev is None and ds[fn] == 'r':
>                  continue

It's surprising to me that ctx.matches() can include files that are not tracked. I'll see if I can send a patch to change that. (No action expected from you)

> test-grep.t:293
>    $ hg ci -Am rename
>    $ hg grep octarine
> +  colour:octarine

Perhaps it's better to preserve the old behavior here (by adding --all, I think)? I'm not sure what the reason for this call is, though, so perhaps it could instead be removed?

REPOSITORY
  rHG Mercurial

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

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