Patchwork [04,of,11,sparse] sparse: refactor activeprofiles into a generic function (API)

login
register
mail settings
Submitter Gregory Szorc
Date July 8, 2017, 11:28 p.m.
Message ID <663d36aeb183dfce677f.1499556539@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/22129/
State Accepted
Headers show

Comments

Gregory Szorc - July 8, 2017, 11:28 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1499547692 25200
#      Sat Jul 08 14:01:32 2017 -0700
# Node ID 663d36aeb183dfce677f52a27cc371272f15b5b7
# Parent  43da636d83fc5e4eae97a6c3b526552407a4ec77
sparse: refactor activeprofiles into a generic function (API)

activeprofiles() is a special case of a more generic function.
Furthermore, that generic function is essentially already
implemented inline in the sparse extension.

So, refactor activeprofiles() to a generic activeconfig(). Change
the only consumer of activeprofiles() to use it. And have the
inline implementation in the sparse extension use it.
via Mercurial-devel - July 10, 2017, 10:07 p.m.
On Sat, Jul 8, 2017 at 4:28 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> diff --git a/mercurial/sparse.py b/mercurial/sparse.py
> --- a/mercurial/sparse.py
> +++ b/mercurial/sparse.py
> @@ -124,15 +124,26 @@ def patternsforrev(repo, rev):
>
>      return includes, excludes, profiles
>
> -def activeprofiles(repo):
> +def activeconfig(repo):
> +    """Determine the active sparse config rules.
> +
> +    Rules are constructed by reading the current sparse config and bringing in
> +    referenced profiles from parents of the working directory.
> +    """
>      revs = [repo.changelog.rev(node) for node in
>              repo.dirstate.parents() if node != nullid]
>
> -    profiles = set()
> +    allincludes = set()
> +    allexcludes = set()
> +    allprofiles = set()
> +
>      for rev in revs:
> -        profiles.update(patternsforrev(repo, rev)[2])
> +        includes, excludes, profiles = patternsforrev(repo, rev)
> +        allincludes |= includes
> +        allexcludes |= excludes
> +        allprofiles |= set(profiles)

Feel like changing parseconfig() to return profiles as set too (in a
followup)? I couldn't see a reason that it shouldn't be.
Gregory Szorc - July 11, 2017, 4:31 a.m.
On Mon, Jul 10, 2017 at 3:07 PM, Martin von Zweigbergk <
martinvonz@google.com> wrote:

> On Sat, Jul 8, 2017 at 4:28 PM, Gregory Szorc <gregory.szorc@gmail.com>
> wrote:
> > diff --git a/mercurial/sparse.py b/mercurial/sparse.py
> > --- a/mercurial/sparse.py
> > +++ b/mercurial/sparse.py
> > @@ -124,15 +124,26 @@ def patternsforrev(repo, rev):
> >
> >      return includes, excludes, profiles
> >
> > -def activeprofiles(repo):
> > +def activeconfig(repo):
> > +    """Determine the active sparse config rules.
> > +
> > +    Rules are constructed by reading the current sparse config and
> bringing in
> > +    referenced profiles from parents of the working directory.
> > +    """
> >      revs = [repo.changelog.rev(node) for node in
> >              repo.dirstate.parents() if node != nullid]
> >
> > -    profiles = set()
> > +    allincludes = set()
> > +    allexcludes = set()
> > +    allprofiles = set()
> > +
> >      for rev in revs:
> > -        profiles.update(patternsforrev(repo, rev)[2])
> > +        includes, excludes, profiles = patternsforrev(repo, rev)
> > +        allincludes |= includes
> > +        allexcludes |= excludes
> > +        allprofiles |= set(profiles)
>
> Feel like changing parseconfig() to return profiles as set too (in a
> followup)? I couldn't see a reason that it shouldn't be.
>

Since at the end of the day the only things that matter are includes and
excludes (which are sets), I agree that profiles could be a set. I do find
it somewhat odd that the existing code treats them as a list. So I want to
run this by Durham (CCd) in case there is something subtle that wants
profiles to be a list. Any non-determinism issues could be fixed by
throwing a sorted() around profiles traversal, of course.
Durham Goode - July 11, 2017, 5:19 p.m.
On 7/10/17 9:31 PM, Gregory Szorc wrote:
> On Mon, Jul 10, 2017 at 3:07 PM, Martin von Zweigbergk
> <martinvonz@google.com <mailto:martinvonz@google.com>> wrote:
>
>     On Sat, Jul 8, 2017 at 4:28 PM, Gregory Szorc
>     <gregory.szorc@gmail.com <mailto:gregory.szorc@gmail.com>> wrote:
>     > diff --git a/mercurial/sparse.py b/mercurial/sparse.py
>     > --- a/mercurial/sparse.py
>     > +++ b/mercurial/sparse.py
>     > @@ -124,15 +124,26 @@ def patternsforrev(repo, rev):
>     >
>     >      return includes, excludes, profiles
>     >
>     > -def activeprofiles(repo):
>     > +def activeconfig(repo):
>     > +    """Determine the active sparse config rules.
>     > +
>     > +    Rules are constructed by reading the current sparse config
>     and bringing in
>     > +    referenced profiles from parents of the working directory.
>     > +    """
>     >      revs = [repo.changelog.rev(node) for node in
>     >              repo.dirstate.parents() if node != nullid]
>     >
>     > -    profiles = set()
>     > +    allincludes = set()
>     > +    allexcludes = set()
>     > +    allprofiles = set()
>     > +
>     >      for rev in revs:
>     > -        profiles.update(patternsforrev(repo, rev)[2])
>     > +        includes, excludes, profiles = patternsforrev(repo, rev)
>     > +        allincludes |= includes
>     > +        allexcludes |= excludes
>     > +        allprofiles |= set(profiles)
>
>     Feel like changing parseconfig() to return profiles as set too (in a
>     followup)? I couldn't see a reason that it shouldn't be.
>
>
> Since at the end of the day the only things that matter are includes and
> excludes (which are sets), I agree that profiles could be a set. I do
> find it somewhat odd that the existing code treats them as a list. So I
> want to run this by Durham (CCd) in case there is something subtle that
> wants profiles to be a list. Any non-determinism issues could be fixed
> by throwing a sorted() around profiles traversal, of course.

I can't think of any reason why profiles shouldn't be returned as sets. 
I explicitly made the sparse configs non-ordered to give us some more 
leeway in terms of parallelization and performance in the future.

Patch

diff --git a/hgext/sparse.py b/hgext/sparse.py
--- a/hgext/sparse.py
+++ b/hgext/sparse.py
@@ -75,7 +75,6 @@  certain files::
 from __future__ import absolute_import
 
 from mercurial.i18n import _
-from mercurial.node import nullid
 from mercurial import (
     cmdutil,
     commands,
@@ -448,23 +447,13 @@  def _config(ui, repo, pats, opts, includ
 
 def _import(ui, repo, files, opts, force=False):
     with repo.wlock():
-        # load union of current active profile
-        revs = [repo.changelog.rev(node) for node in
-                repo.dirstate.parents() if node != nullid]
-
         # read current configuration
         raw = repo.vfs.tryread('sparse')
         oincludes, oexcludes, oprofiles = sparse.parseconfig(ui, raw)
         includes, excludes, profiles = map(
                 set, (oincludes, oexcludes, oprofiles))
 
-        # all active rules
-        aincludes, aexcludes, aprofiles = set(), set(), set()
-        for rev in revs:
-            rincludes, rexcludes, rprofiles = sparse.patternsforrev(repo, rev)
-            aincludes.update(rincludes)
-            aexcludes.update(rexcludes)
-            aprofiles.update(rprofiles)
+        aincludes, aexcludes, aprofiles = sparse.activeconfig(repo)
 
         # import rules on top; only take in rules that are not yet
         # part of the active rules.
diff --git a/mercurial/sparse.py b/mercurial/sparse.py
--- a/mercurial/sparse.py
+++ b/mercurial/sparse.py
@@ -124,15 +124,26 @@  def patternsforrev(repo, rev):
 
     return includes, excludes, profiles
 
-def activeprofiles(repo):
+def activeconfig(repo):
+    """Determine the active sparse config rules.
+
+    Rules are constructed by reading the current sparse config and bringing in
+    referenced profiles from parents of the working directory.
+    """
     revs = [repo.changelog.rev(node) for node in
             repo.dirstate.parents() if node != nullid]
 
-    profiles = set()
+    allincludes = set()
+    allexcludes = set()
+    allprofiles = set()
+
     for rev in revs:
-        profiles.update(patternsforrev(repo, rev)[2])
+        includes, excludes, profiles = patternsforrev(repo, rev)
+        allincludes |= includes
+        allexcludes |= excludes
+        allprofiles |= set(profiles)
 
-    return profiles
+    return allincludes, allexcludes, allprofiles
 
 def configsignature(repo, includetemp=True):
     """Obtain the signature string for the current sparse configuration.
@@ -362,7 +373,7 @@  def filterupdatesactions(repo, wctx, mct
         for file, flags, msg in actions:
             dirstate.normal(file)
 
-    profiles = activeprofiles(repo)
+    profiles = activeconfig(repo)[2]
     changedprofiles = profiles & files
     # If an active profile changed during the update, refresh the checkout.
     # Don't do this during a branch merge, since all incoming changes should