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
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
