Patchwork grep: warn on censored revisions instead of erroring out

login
register
mail settings
Submitter Jordi Gutiérrez Hermoso
Date Oct. 21, 2019, 8:10 p.m.
Message ID <608193de8560218cc270.1571688658@chloe>
Download mbox | patch
Permalink /patch/42516/
State Superseded
Headers show

Comments

Jordi Gutiérrez Hermoso - Oct. 21, 2019, 8:10 p.m.
# HG changeset patch
# User Jordi Gutiérrez Hermoso <jordigh@octave.org>
# Date 1571688404 14400
#      Mon Oct 21 16:06:44 2019 -0400
# Node ID 608193de8560218cc27032977bc380cb8dc0f3f8
# Parent  d782cce137fd1d50cccfecf8ccb17a623fde8800
grep: warn on censored revisions instead of erroring out

We need most of the grep logic to go through in case we encounter a
censored revision, so we just return a None body for a censored node,
and we stop just short of trying to record matches with the contents
of that censored body. The other parts such as recording that the
censored file has been considered at this revision needs to go into
the proper dicts.

I have also gotten weary of all the abbreviations, so while I did a
small refactor to move the file-data-getting operation into a common
function, I also expanded the abbreviations of the relevant variables
within this little function. Hopefully some day this helps someone
figure out what all the abbreviations mean.
Yuya Nishihara - Oct. 22, 2019, 1:39 a.m.
On Mon, 21 Oct 2019 16:10:58 -0400, Jordi Gutiérrez Hermoso wrote:
> # HG changeset patch
> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> # Date 1571688404 14400
> #      Mon Oct 21 16:06:44 2019 -0400
> # Node ID 608193de8560218cc27032977bc380cb8dc0f3f8
> # Parent  d782cce137fd1d50cccfecf8ccb17a623fde8800
> grep: warn on censored revisions instead of erroring out

Can you add some tests?

> @@ -3575,6 +3578,22 @@ def grep(ui, repo, pattern, *pats, **opt
>  
>      getrenamed = scmutil.getrenamedfn(repo)
>  
> +    def get_file_content(filename, filelog, filenode, context, revision):
> +        try:
> +            content = filelog.read(filenode)
> +        except error.WdirUnsupported:
> +            content = context[filename].data()
> +        except error.Sensurround:
> +            content = None

Might be better to fail if censor.policy != ignore.

The doc says:

 "Censored nodes can interrupt mercurial's typical operation whenever the excised
  data needs to be materialized. Some commands, like ``hg cat``/``hg revert``,
  simply fail when asked to produce censored data. Others, like ``hg verify`` and
  ``hg update``, must be capable of tolerating censored data to continue to
  function in a meaningful way. Such commands only tolerate censored file
  revisions if they are allowed by the "censor.policy=ignore" config option.

I have no idea which command group "hg grep" should belong to.

> +            ui.warn(
> +                _(b'Cannot search in censored file: %(filename)s:%(revnum)s\n')

Nit: messages are all lowercase in general.

> +                % {
> +                    'filename': pycompat.bytestr(filename),

Nit: filename must be bytes. bytestr() shouldn't be needed.
Jordi Gutiérrez Hermoso - Oct. 22, 2019, 2:04 p.m.
On Tue, 2019-10-22 at 10:39 +0900, Yuya Nishihara wrote:
> On Mon, 21 Oct 2019 16:10:58 -0400, Jordi Gutiérrez Hermoso wrote:
> > # HG changeset patch
> > # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> > # Date 1571688404 14400
> > #      Mon Oct 21 16:06:44 2019 -0400
> > # Node ID 608193de8560218cc27032977bc380cb8dc0f3f8
> > # Parent  d782cce137fd1d50cccfecf8ccb17a623fde8800
> > grep: warn on censored revisions instead of erroring out
> 
> Can you add some tests?

Done.

> Might be better to fail if censor.policy != ignore.

I can't see why anyone could want grep to error out early, so I'm
going to keep it as is and I propose an addendum to the censor docs
instead.

> Nit: messages are all lowercase in general.
[..]
> Nit: filename must be bytes. bytestr() shouldn't be needed.

Fixed and fixed.

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -3440,6 +3440,9 @@  def grep(ui, repo, pattern, *pats, **opt
     def grepbody(fn, rev, body):
         matches[rev].setdefault(fn, [])
         m = matches[rev][fn]
+        if body is None:
+            return
+
         for lnum, cstart, cend, line in matchlines(body):
             s = linestate(line, lnum, cstart, cend)
             m.append(s)
@@ -3575,6 +3578,22 @@  def grep(ui, repo, pattern, *pats, **opt
 
     getrenamed = scmutil.getrenamedfn(repo)
 
+    def get_file_content(filename, filelog, filenode, context, revision):
+        try:
+            content = filelog.read(filenode)
+        except error.WdirUnsupported:
+            content = context[filename].data()
+        except error.CensoredNodeError:
+            content = None
+            ui.warn(
+                _(b'Cannot search in censored file: %(filename)s:%(revnum)s\n')
+                % {
+                    'filename': pycompat.bytestr(filename),
+                    'revnum': pycompat.bytestr(revision),
+                }
+            )
+        return content
+
     def prep(ctx, fns):
         rev = ctx.rev()
         pctx = ctx.p1()
@@ -3601,17 +3620,15 @@  def grep(ui, repo, pattern, *pats, **opt
             files.append(fn)
 
             if fn not in matches[rev]:
-                try:
-                    content = flog.read(fnode)
-                except error.WdirUnsupported:
-                    content = ctx[fn].data()
+                content = get_file_content(fn, flog, fnode, ctx, rev)
                 grepbody(fn, rev, content)
 
             pfn = copy or fn
             if pfn not in matches[parent]:
                 try:
-                    fnode = pctx.filenode(pfn)
-                    grepbody(pfn, parent, flog.read(fnode))
+                    pfnode = pctx.filenode(pfn)
+                    pcontent = get_file_content(pfn, flog, pfnode, pctx, parent)
+                    grepbody(pfn, parent, pcontent)
                 except error.LookupError:
                     pass