Patchwork annotate: split the logic to two functions

login
register
mail settings
Submitter Jun Wu
Date June 11, 2016, 1 a.m.
Message ID <ce07999816426aeb5696.1465606846@x1c>
Download mbox | patch
Permalink /patch/15468/
State Changes Requested
Headers show

Comments

Jun Wu - June 11, 2016, 1 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1465605860 -3600
#      Sat Jun 11 01:44:20 2016 +0100
# Node ID ce07999816426aeb56965c9ae9ffd25f82ce5102
# Parent  baab9ea4426ca658d191119bfa8715eb40a39d82
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r ce0799981642
annotate: split the logic to two functions

Before this patch, fctx.annotate will visit all reachable revisions in its
dfs (depth-first search) process.

This patch splits the search logic to a standalone function, receiving
"parents", "decorate", "pair" as parameters, making it much more flexible.
The change is minimal but opens up a lot of possibilities.

Customizing "parents" will allow us to:
- stop dfs early, ex. the user only care about "draft()"
- manually mark a "move", ex. the user forgot "hg mv"
- enforce linear history, ex. the user just want to see merges
- skip revs, also doable using "pair"

Customizing "decorate" will allow us to:
- change "content", ex. the user wants to annotate a single line
- add other fields to the tuple associated with each line

Customizing "pair" will allow us to:
- choose a diff algorithm, in git world, diff algorithm is an option
- ignore non-whitespace changes, ex. ignore iteritems -> items
Jun Wu - June 15, 2016, 10:15 a.m.
Side notes:

I know "_annotate" is a bad name but I am open to ideas.

As the commit message says, the split will make annotate much more flexible,
I'd like to have it tested somewhere so the API is somehow "supported". If
that means to have some user interfaces, I'd like to help as well.

I think making -r accept a revset and limit the output to the revset make
sense to the end user. But I had difficulty handling the real world
situation: where fr1, fr2 are filerevs where fr2 is fr1.parents[0] and
fr2.linknode is not an ancestor of fr1.linknode:

  hg log -G mercurial/commands.py
  
  | |
  | o  changeset:   28771:2e1bceeea520
  |/|  user:        FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
  | |  date:        Sat Mar 12 04:35:42 2016 +0900
  | .  summary:     update: omit redundant activating message for ...
  | .
  | | o  changeset:   28658:a66cfb1153ec
  | | |  user:        Jun Wu <quark@fb.com>
  | | |  date:        Mon Mar 14 10:24:06 2016 +0000
  | | .  summary:     debug message
  | | .
  | | x  changeset:   28597:bed73b2b5504
  | | |  user:        Jun Wu <quark@fb.com>
  | | |  date:        Wed Mar 09 02:07:40 2016 +0000
  | | .  summary:     serve: accept multiple values for --daemon-postexec
  | | .
  | | x  changeset:   28587:0293fb3825c5
  | |/   user:        timeless <timeless@mozdev.org>
  | |    date:        Wed Mar 09 18:58:51 2016 +0000
  | |    summary:     debuginstall: convert to formatter
  | |

  hg debugshell
  
  In [19]: repo[28771]['mercurial/commands.py'].parents()[0].linkrev()
  Out[19]: 28597

Rev 28597 will not be in revset 'ancestors(@)' and breaks the annotate
dfs.

Other than -r REVSET, I considered something like --stop-rev to make
the dfs stop at some revision. However, it is tricky for the user
to figure out whether a revision is in filelog or not.

Excerpts from Jun Wu's message of 2016-06-11 02:00:46 +0100:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1465605860 -3600
> #      Sat Jun 11 01:44:20 2016 +0100
> # Node ID ce07999816426aeb56965c9ae9ffd25f82ce5102
> # Parent  baab9ea4426ca658d191119bfa8715eb40a39d82
> # Available At https://bitbucket.org/quark-zju/hg-draft 
> #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r ce0799981642
> annotate: split the logic to two functions
> 
> Before this patch, fctx.annotate will visit all reachable revisions in its
> dfs (depth-first search) process.
> 
> This patch splits the search logic to a standalone function, receiving
> "parents", "decorate", "pair" as parameters, making it much more flexible.
> The change is minimal but opens up a lot of possibilities.
> 
> Customizing "parents" will allow us to:
> - stop dfs early, ex. the user only care about "draft()"
> - manually mark a "move", ex. the user forgot "hg mv"
> - enforce linear history, ex. the user just want to see merges
> - skip revs, also doable using "pair"
> 
> Customizing "decorate" will allow us to:
> - change "content", ex. the user wants to annotate a single line
> - add other fields to the tuple associated with each line
> 
> Customizing "pair" will allow us to:
> - choose a diff algorithm, in git world, diff algorithm is an option
> - ignore non-whitespace changes, ex. ignore iteritems -> items
> 
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -993,6 +993,9 @@
>                  ac = cl.ancestors([introrev], inclusive=True)
>              base._ancestrycontext = ac
>  
> +        return base._annotate(parents, decorate, pair)
> +
> +    def _annotate(base, parents, decorate, pair):
>          # This algorithm would prefer to be recursive, but Python is a
>          # bit recursion-hostile. Instead we do an iterative
>          # depth-first search.
Pierre-Yves David - June 17, 2016, 5:12 p.m.
On 06/11/2016 03:00 AM, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1465605860 -3600
> #      Sat Jun 11 01:44:20 2016 +0100
> # Node ID ce07999816426aeb56965c9ae9ffd25f82ce5102
> # Parent  baab9ea4426ca658d191119bfa8715eb40a39d82
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r ce0799981642
> annotate: split the logic to two functions
> 
> Before this patch, fctx.annotate will visit all reachable revisions in its
> dfs (depth-first search) process.
> 
> This patch splits the search logic to a standalone function, receiving
> "parents", "decorate", "pair" as parameters, making it much more flexible.
> The change is minimal but opens up a lot of possibilities.
> 
> Customizing "parents" will allow us to:
> - stop dfs early, ex. the user only care about "draft()"
> - manually mark a "move", ex. the user forgot "hg mv"
> - enforce linear history, ex. the user just want to see merges
> - skip revs, also doable using "pair"
> 
> Customizing "decorate" will allow us to:
> - change "content", ex. the user wants to annotate a single line
> - add other fields to the tuple associated with each line
> 
> Customizing "pair" will allow us to:
> - choose a diff algorithm, in git world, diff algorithm is an option
> - ignore non-whitespace changes, ex. ignore iteritems -> items
> 
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -993,6 +993,9 @@
>                  ac = cl.ancestors([introrev], inclusive=True)
>              base._ancestrycontext = ac
>  
> +        return base._annotate(parents, decorate, pair)
> +
> +    def _annotate(base, parents, decorate, pair):
>          # This algorithm would prefer to be recursive, but Python is a
>          # bit recursion-hostile. Instead we do an iterative
>          # depth-first search.

I would like to suguest to take advantage of this rework to document the
thing. As 'annotate' and '_annotate' are each doing a defined subset of
the logic, having a docstring stating there purpose would help us to
ensure the split is right and en-light future contributor (and also help
me to review this patch).

Can you send a V2?

Cheers,

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -993,6 +993,9 @@ 
                 ac = cl.ancestors([introrev], inclusive=True)
             base._ancestrycontext = ac
 
+        return base._annotate(parents, decorate, pair)
+
+    def _annotate(base, parents, decorate, pair):
         # This algorithm would prefer to be recursive, but Python is a
         # bit recursion-hostile. Instead we do an iterative
         # depth-first search.