Patchwork [4,of,4,accept-scripts] land: fix logic so that reviewers work as intended

login
register
mail settings
Submitter Augie Fackler
Date June 12, 2019, 4:52 a.m.
Message ID <e696629f153d7fcfa11f.1560315145@augie-macbookpro2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/40436/
State New
Headers show

Comments

Augie Fackler - June 12, 2019, 4:52 a.m.
# HG changeset patch
# User Augie Fackler <raf@durin42.com>
# Date 1560313366 14400
#      Wed Jun 12 00:22:46 2019 -0400
# Node ID e696629f153d7fcfa11f7046c43dff1fb1a79373
# Parent  eca3ad69186b71aceb657be4e884f2a199fa784c
land: fix logic so that reviewers work as intended

The intent was for 1 accepter + 1 reviewer _or_ two accepters to be
able to land things. Somehow, the reviewer-and-accepter case was never
implemented, and mysteriously went unnoticed until now.
Augie Fackler - June 12, 2019, 5:07 a.m.
These resolve the mysterious "why aren't things landing" situation we've currently got. I wanted to get them reviewed because this matches my memory of how we intended things to work, but I wanted confirmation before just running with it.

Thanks!

> On Jun 12, 2019, at 00:52, Augie Fackler <raf@durin42.com> wrote:
> 
> # HG changeset patch
> # User Augie Fackler <raf@durin42.com>
> # Date 1560313366 14400
> #      Wed Jun 12 00:22:46 2019 -0400
> # Node ID e696629f153d7fcfa11f7046c43dff1fb1a79373
> # Parent  eca3ad69186b71aceb657be4e884f2a199fa784c
> land: fix logic so that reviewers work as intended
> 
> The intent was for 1 accepter + 1 reviewer _or_ two accepters to be
> able to land things. Somehow, the reviewer-and-accepter case was never
> implemented, and mysteriously went unnoticed until now.
> 
> diff --git a/accept.py b/accept.py
> --- a/accept.py
> +++ b/accept.py
> @@ -124,6 +124,10 @@ class commitqueue(object):
>     def accepted(self, commit):
>         al = self.accepters(commit)
>         if len(al) >= 2:
> +            # Two valid accept stamps
> +            return True
> +        if al and len(set(self.reviewers(commit) + al)) >= 2:
> +            # pushed by a reviewer, stamped by a committer
>             return True
>         return False
> 
> diff --git a/accepted b/accepted
> --- a/accepted
> +++ b/accepted
> @@ -9,4 +9,4 @@ cq = accept.commitqueue(config.load().so
> for n in cq.commits():
>     a = cq.accepted(n)
>     if a:
> -        print n, " ".join(cq.accepters(n))
> +        print n, " ".join(sorted(set(cq.reviewers(n) + cq.accepters(n))))
> diff --git a/land b/land
> --- a/land
> +++ b/land
> @@ -128,7 +128,8 @@ if take:
>                 f.write('%(node)s %(author)s %(accepters)s\n' % {
>                     'node': node,
>                     'author': cq.author(node),
> -                    'accepters': ' '.join(cq.accepted(node)),
> +                    'accepters': ' '.join(
> +                        sorted(set(cq.reviewers(node) + cq.accepters(node)))),
>                 })
> else:
>     logging.debug("not landing any changes")
> diff --git a/tests/test-land.t b/tests/test-land.t
> --- a/tests/test-land.t
> +++ b/tests/test-land.t
> @@ -170,17 +170,26 @@ pushed by a reviewer, not a full accepte
>   o  0:1ea7 default
> 
>   $ accepted
> +  01241442b3c2bf3211e593b549c655ea65b295e3 bob fred
>   ad9193bbd7242c2bb6df4bb3cf71d0ce2e68457c alice bob
>   0d90bd0be154baaf1b9e9eb4906e031d2874ece2 alice bob
>   a500f39c86e6e58d237748a861d3a5567101bd48 alice bob
>   eefd39cebb36c0cbc653ebf033972850e44801fd alice bob
> -BUG: we don't accept 1 reviewer and one accepter as being accepted.
>   $ blocker
> -  01241442b3c2bf3211e593b549c655ea65b295e3 bob
>   $ land
>   $ hg log -G -T'{rev}:{node|shortest} {branch} {tags}\n' -R ../complextgt
> -  o  2:0acc stable tip
> +  o  7:eefd default tip
>   |
> +  o    6:a500 default
> +  |\
> +  | o  5:0d90 stable
> +  | |
> +  o |  4:ad91 default
> +  |\|
> +  o |  3:0124 default
> +  | |
> +  | o  2:0acc stable
> +  |/
>   o  1:66f7 default
>   |
>   o  0:1ea7 default
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
via Mercurial-devel - June 17, 2019, 5:50 a.m.
Sorry that I forgot to reply earlier, but feel free to push this series (as
far as I'm concerned anyway -- up to you if want Kevin's approval too).

On Tue, Jun 11, 2019 at 10:10 PM Augie Fackler <raf@durin42.com> wrote:

> These resolve the mysterious "why aren't things landing" situation we've
> currently got. I wanted to get them reviewed because this matches my memory
> of how we intended things to work, but I wanted confirmation before just
> running with it.
>
> Thanks!
>
> > On Jun 12, 2019, at 00:52, Augie Fackler <raf@durin42.com> wrote:
> >
> > # HG changeset patch
> > # User Augie Fackler <raf@durin42.com>
> > # Date 1560313366 14400
> > #      Wed Jun 12 00:22:46 2019 -0400
> > # Node ID e696629f153d7fcfa11f7046c43dff1fb1a79373
> > # Parent  eca3ad69186b71aceb657be4e884f2a199fa784c
> > land: fix logic so that reviewers work as intended
> >
> > The intent was for 1 accepter + 1 reviewer _or_ two accepters to be
> > able to land things. Somehow, the reviewer-and-accepter case was never
> > implemented, and mysteriously went unnoticed until now.
> >
> > diff --git a/accept.py b/accept.py
> > --- a/accept.py
> > +++ b/accept.py
> > @@ -124,6 +124,10 @@ class commitqueue(object):
> >     def accepted(self, commit):
> >         al = self.accepters(commit)
> >         if len(al) >= 2:
> > +            # Two valid accept stamps
> > +            return True
> > +        if al and len(set(self.reviewers(commit) + al)) >= 2:
> > +            # pushed by a reviewer, stamped by a committer
> >             return True
> >         return False
> >
> > diff --git a/accepted b/accepted
> > --- a/accepted
> > +++ b/accepted
> > @@ -9,4 +9,4 @@ cq = accept.commitqueue(config.load().so
> > for n in cq.commits():
> >     a = cq.accepted(n)
> >     if a:
> > -        print n, " ".join(cq.accepters(n))
> > +        print n, " ".join(sorted(set(cq.reviewers(n) +
> cq.accepters(n))))
> > diff --git a/land b/land
> > --- a/land
> > +++ b/land
> > @@ -128,7 +128,8 @@ if take:
> >                 f.write('%(node)s %(author)s %(accepters)s\n' % {
> >                     'node': node,
> >                     'author': cq.author(node),
> > -                    'accepters': ' '.join(cq.accepted(node)),
> > +                    'accepters': ' '.join(
> > +                        sorted(set(cq.reviewers(node) +
> cq.accepters(node)))),
> >                 })
> > else:
> >     logging.debug("not landing any changes")
> > diff --git a/tests/test-land.t b/tests/test-land.t
> > --- a/tests/test-land.t
> > +++ b/tests/test-land.t
> > @@ -170,17 +170,26 @@ pushed by a reviewer, not a full accepte
> >   o  0:1ea7 default
> >
> >   $ accepted
> > +  01241442b3c2bf3211e593b549c655ea65b295e3 bob fred
> >   ad9193bbd7242c2bb6df4bb3cf71d0ce2e68457c alice bob
> >   0d90bd0be154baaf1b9e9eb4906e031d2874ece2 alice bob
> >   a500f39c86e6e58d237748a861d3a5567101bd48 alice bob
> >   eefd39cebb36c0cbc653ebf033972850e44801fd alice bob
> > -BUG: we don't accept 1 reviewer and one accepter as being accepted.
> >   $ blocker
> > -  01241442b3c2bf3211e593b549c655ea65b295e3 bob
> >   $ land
> >   $ hg log -G -T'{rev}:{node|shortest} {branch} {tags}\n' -R
> ../complextgt
> > -  o  2:0acc stable tip
> > +  o  7:eefd default tip
> >   |
> > +  o    6:a500 default
> > +  |\
> > +  | o  5:0d90 stable
> > +  | |
> > +  o |  4:ad91 default
> > +  |\|
> > +  o |  3:0124 default
> > +  | |
> > +  | o  2:0acc stable
> > +  |/
> >   o  1:66f7 default
> >   |
> >   o  0:1ea7 default
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Augie Fackler - June 28, 2019, 8:20 p.m.
Thanks, I'm going to go ahead and push this and deploy it so I don't have to manually fix in the future.

> On Jun 17, 2019, at 01:50, Martin von Zweigbergk <martinvonz@google.com> wrote:
> 
> Sorry that I forgot to reply earlier, but feel free to push this series (as far as I'm concerned anyway -- up to you if want Kevin's approval too).
> 
> On Tue, Jun 11, 2019 at 10:10 PM Augie Fackler <raf@durin42.com <mailto:raf@durin42.com>> wrote:
> These resolve the mysterious "why aren't things landing" situation we've currently got. I wanted to get them reviewed because this matches my memory of how we intended things to work, but I wanted confirmation before just running with it.
> 
> Thanks!
> 
> > On Jun 12, 2019, at 00:52, Augie Fackler <raf@durin42.com <mailto:raf@durin42.com>> wrote:
> > 
> > # HG changeset patch
> > # User Augie Fackler <raf@durin42.com <mailto:raf@durin42.com>>
> > # Date 1560313366 14400
> > #      Wed Jun 12 00:22:46 2019 -0400
> > # Node ID e696629f153d7fcfa11f7046c43dff1fb1a79373
> > # Parent  eca3ad69186b71aceb657be4e884f2a199fa784c
> > land: fix logic so that reviewers work as intended
> > 
> > The intent was for 1 accepter + 1 reviewer _or_ two accepters to be
> > able to land things. Somehow, the reviewer-and-accepter case was never
> > implemented, and mysteriously went unnoticed until now.
> > 
> > diff --git a/accept.py b/accept.py
> > --- a/accept.py
> > +++ b/accept.py
> > @@ -124,6 +124,10 @@ class commitqueue(object):
> >     def accepted(self, commit):
> >         al = self.accepters(commit)
> >         if len(al) >= 2:
> > +            # Two valid accept stamps
> > +            return True
> > +        if al and len(set(self.reviewers(commit) + al)) >= 2:
> > +            # pushed by a reviewer, stamped by a committer
> >             return True
> >         return False
> > 
> > diff --git a/accepted b/accepted
> > --- a/accepted
> > +++ b/accepted
> > @@ -9,4 +9,4 @@ cq = accept.commitqueue(config.load().so
> > for n in cq.commits():
> >     a = cq.accepted(n)
> >     if a:
> > -        print n, " ".join(cq.accepters(n))
> > +        print n, " ".join(sorted(set(cq.reviewers(n) + cq.accepters(n))))
> > diff --git a/land b/land
> > --- a/land
> > +++ b/land
> > @@ -128,7 +128,8 @@ if take:
> >                 f.write('%(node)s %(author)s %(accepters)s\n' % {
> >                     'node': node,
> >                     'author': cq.author(node),
> > -                    'accepters': ' '.join(cq.accepted(node)),
> > +                    'accepters': ' '.join(
> > +                        sorted(set(cq.reviewers(node) + cq.accepters(node)))),
> >                 })
> > else:
> >     logging.debug("not landing any changes")
> > diff --git a/tests/test-land.t b/tests/test-land.t
> > --- a/tests/test-land.t
> > +++ b/tests/test-land.t
> > @@ -170,17 +170,26 @@ pushed by a reviewer, not a full accepte
> >   o  0:1ea7 default
> > 
> >   $ accepted
> > +  01241442b3c2bf3211e593b549c655ea65b295e3 bob fred
> >   ad9193bbd7242c2bb6df4bb3cf71d0ce2e68457c alice bob
> >   0d90bd0be154baaf1b9e9eb4906e031d2874ece2 alice bob
> >   a500f39c86e6e58d237748a861d3a5567101bd48 alice bob
> >   eefd39cebb36c0cbc653ebf033972850e44801fd alice bob
> > -BUG: we don't accept 1 reviewer and one accepter as being accepted.
> >   $ blocker
> > -  01241442b3c2bf3211e593b549c655ea65b295e3 bob
> >   $ land
> >   $ hg log -G -T'{rev}:{node|shortest} {branch} {tags}\n' -R ../complextgt
> > -  o  2:0acc stable tip
> > +  o  7:eefd default tip
> >   |
> > +  o    6:a500 default
> > +  |\
> > +  | o  5:0d90 stable
> > +  | |
> > +  o |  4:ad91 default
> > +  |\|
> > +  o |  3:0124 default
> > +  | |
> > +  | o  2:0acc stable
> > +  |/
> >   o  1:66f7 default
> >   |
> >   o  0:1ea7 default
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org <mailto:Mercurial-devel@mercurial-scm.org>
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel <https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel>
> 
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org <mailto:Mercurial-devel@mercurial-scm.org>
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel <https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel>

Patch

diff --git a/accept.py b/accept.py
--- a/accept.py
+++ b/accept.py
@@ -124,6 +124,10 @@  class commitqueue(object):
     def accepted(self, commit):
         al = self.accepters(commit)
         if len(al) >= 2:
+            # Two valid accept stamps
+            return True
+        if al and len(set(self.reviewers(commit) + al)) >= 2:
+            # pushed by a reviewer, stamped by a committer
             return True
         return False
 
diff --git a/accepted b/accepted
--- a/accepted
+++ b/accepted
@@ -9,4 +9,4 @@  cq = accept.commitqueue(config.load().so
 for n in cq.commits():
     a = cq.accepted(n)
     if a:
-        print n, " ".join(cq.accepters(n))
+        print n, " ".join(sorted(set(cq.reviewers(n) + cq.accepters(n))))
diff --git a/land b/land
--- a/land
+++ b/land
@@ -128,7 +128,8 @@  if take:
                 f.write('%(node)s %(author)s %(accepters)s\n' % {
                     'node': node,
                     'author': cq.author(node),
-                    'accepters': ' '.join(cq.accepted(node)),
+                    'accepters': ' '.join(
+                        sorted(set(cq.reviewers(node) + cq.accepters(node)))),
                 })
 else:
     logging.debug("not landing any changes")
diff --git a/tests/test-land.t b/tests/test-land.t
--- a/tests/test-land.t
+++ b/tests/test-land.t
@@ -170,17 +170,26 @@  pushed by a reviewer, not a full accepte
   o  0:1ea7 default
   
   $ accepted
+  01241442b3c2bf3211e593b549c655ea65b295e3 bob fred
   ad9193bbd7242c2bb6df4bb3cf71d0ce2e68457c alice bob
   0d90bd0be154baaf1b9e9eb4906e031d2874ece2 alice bob
   a500f39c86e6e58d237748a861d3a5567101bd48 alice bob
   eefd39cebb36c0cbc653ebf033972850e44801fd alice bob
-BUG: we don't accept 1 reviewer and one accepter as being accepted.
   $ blocker
-  01241442b3c2bf3211e593b549c655ea65b295e3 bob
   $ land
   $ hg log -G -T'{rev}:{node|shortest} {branch} {tags}\n' -R ../complextgt
-  o  2:0acc stable tip
+  o  7:eefd default tip
   |
+  o    6:a500 default
+  |\
+  | o  5:0d90 stable
+  | |
+  o |  4:ad91 default
+  |\|
+  o |  3:0124 default
+  | |
+  | o  2:0acc stable
+  |/
   o  1:66f7 default
   |
   o  0:1ea7 default