mbox series

[bug#38846,0/4] Move 'HACKING' to the manual, and a proposal for commit access

Message ID 20200101162945.4946-1-ludo@gnu.org
Headers show
Series Move 'HACKING' to the manual, and a proposal for commit access | expand

Message

Ludovic Courtès Jan. 1, 2020, 4:29 p.m. UTC
Hello Guix!

Happy new year, merry 12 nivôse, or whatever celebration is
appropriate for you!  :-)

These patches do three things:

  1. Move text from ‘HACKING’ to ‘doc/contributing.texi’.

  2. Encourage patch review for committers.

  3. Add a tentative policy for granting commit access (the last
     patch of this series).

I expect #1 and #2 to be uncontroversial, but I’d like feedback on #3!

So far, we’ve been giving commit access in a very ad-hoc fashion.
Often it was Ricardo or myself who ended up taking care of that,
even though other people have admin rights on Savannah to add/remove
members.

We briefly discussed it among maintainers after the maintainer
collective expanded, and it seems to me that perhaps now is a good time
to formalize things a bit—to clarify what contributors may expect and
to increase transparency.  Hence this proposal of a simple co-optation
policy.

As you know, Chris Baines has been working towards automated testing
of submitted patches.  One of the goals is to allow part of the
QA to be automated, such that, eventually, approved merges could be
automated.  In that spirit, we would have an incentive to not add more
committers (probably also a good thing security-wise).  That’s why I
added a note on this topic.

What do people think?

Thanks,
Ludo’.

Ludovic Courtès (4):
  doc: Add "Tracking Bugs and Patches" section.
  doc: Move "Commit Access" section from 'HACKING' to the manual.
  doc: Encourage patch review.
  DRAFT doc: Add a cooption policy for commit access.

 HACKING               |  58 +-------------
 doc/contributing.texi | 171 ++++++++++++++++++++++++++++++++++++++++--
 doc/guix.texi         |   2 +-
 3 files changed, 168 insertions(+), 63 deletions(-)

Comments

Ricardo Wurmus Jan. 1, 2020, 6:37 p.m. UTC | #1
Ludovic Courtès <ludo@gnu.org> writes:

> As you know, Chris Baines has been working towards automated testing
> of submitted patches.  One of the goals is to allow part of the
> QA to be automated, such that, eventually, approved merges could be
> automated.  In that spirit, we would have an incentive to not add more
> committers (probably also a good thing security-wise).  That’s why I
> added a note on this topic.

I’m looking forward to this.  Ultimately, it is a good thing to limit
the number of people who can write to the repository.
Ludovic Courtès Jan. 6, 2020, 1:13 p.m. UTC | #2
Hello!

Just a heads-up for fellow maintainers (Tobias, Marius, Maxim): could
you send a “+1” or whatever you deem appropriate :-) to this discussion?
I’d like to make sure we’re on the same page.

  https://issues.guix.gnu.org/issue/38846

Ludo’.

Ludovic Courtès <ludo@gnu.org> skribis:

> Hello Guix!
>
> Happy new year, merry 12 nivôse, or whatever celebration is
> appropriate for you!  :-)
>
> These patches do three things:
>
>   1. Move text from ‘HACKING’ to ‘doc/contributing.texi’.
>
>   2. Encourage patch review for committers.
>
>   3. Add a tentative policy for granting commit access (the last
>      patch of this series).
>
> I expect #1 and #2 to be uncontroversial, but I’d like feedback on #3!
>
> So far, we’ve been giving commit access in a very ad-hoc fashion.
> Often it was Ricardo or myself who ended up taking care of that,
> even though other people have admin rights on Savannah to add/remove
> members.
>
> We briefly discussed it among maintainers after the maintainer
> collective expanded, and it seems to me that perhaps now is a good time
> to formalize things a bit—to clarify what contributors may expect and
> to increase transparency.  Hence this proposal of a simple co-optation
> policy.
>
> As you know, Chris Baines has been working towards automated testing
> of submitted patches.  One of the goals is to allow part of the
> QA to be automated, such that, eventually, approved merges could be
> automated.  In that spirit, we would have an incentive to not add more
> committers (probably also a good thing security-wise).  That’s why I
> added a note on this topic.
>
> What do people think?
>
> Thanks,
> Ludo’.
>
> Ludovic Courtès (4):
>   doc: Add "Tracking Bugs and Patches" section.
>   doc: Move "Commit Access" section from 'HACKING' to the manual.
>   doc: Encourage patch review.
>   DRAFT doc: Add a cooption policy for commit access.
>
>  HACKING               |  58 +-------------
>  doc/contributing.texi | 171 ++++++++++++++++++++++++++++++++++++++++--
>  doc/guix.texi         |   2 +-
>  3 files changed, 168 insertions(+), 63 deletions(-)
Simon Tournier Jan. 6, 2020, 9:44 p.m. UTC | #3
Hi,


On Wed, 1 Jan 2020 at 17:31, Ludovic Courtès <ludo@gnu.org> wrote:

>   1. Move text from ‘HACKING’ to ‘doc/contributing.texi’.
>
>   2. Encourage patch review for committers.
>
>   3. Add a tentative policy for granting commit access (the last
>      patch of this series).
>
> I expect #1 and #2 to be uncontroversial, but I’d like feedback on #3!
>
> So far, we’ve been giving commit access in a very ad-hoc fashion.
> Often it was Ricardo or myself who ended up taking care of that,
> even though other people have admin rights on Savannah to add/remove
> members.
>
> We briefly discussed it among maintainers after the maintainer
> collective expanded, and it seems to me that perhaps now is a good time
> to formalize things a bit—to clarify what contributors may expect and
> to increase transparency.  Hence this proposal of a simple co-optation
> policy.
>
> As you know, Chris Baines has been working towards automated testing
> of submitted patches.  One of the goals is to allow part of the
> QA to be automated, such that, eventually, approved merges could be
> automated.  In that spirit, we would have an incentive to not add more
> committers (probably also a good thing security-wise).  That’s why I
> added a note on this topic.
>
> What do people think?


Personally, I find this proposal nice. As I already said when
commenting on each patch. :-)

However, let point 2 minor weak points for further discussions:

 a- if the number of committers with access is more or less fixed,
then access could be transferred (less active, less time, less
motivation, etc.);
 and b- the bottleneck is the patch review (even if it should be
improved in the future with the Guix Data Service).
Well, a- is more about human relationship, hard to fix. However, we
should work on b- but how? What does it mean "encourage patch review"?
Candy or beer at Guix Days? ;-)

For example, this patch [1] has fallen in the crack. Or this one [2]
or this other one [3].

[1] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=31973
[2] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=33041
[3] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=33318


From the v1.0.1 to now, the repartition of committers which are not the authors:

   361
     78
     65
     61
     59
     54
     52
     47
     44
     43
     37
     21  (x2)
     11
      9
      8
      7  (x2)
      6
      5  (x3)
      4  (x2)
      3
      2  (x3)
      1  (x3)

which should be compared to the number of commits per author also
committer (first 10):

   1463
   1162
    886
    670
    618
    335
    204
    166
    161
    150

--8<---------------cut here---------------start------------->8---
# committer and not the author
git log v1.0.1..origin/HEAD --pretty=format:"%an~%cn"     \
    | awk -F '~' '{if ($1 != $2) {++cnt; print "#"$2};}' \
    | sort                                               \
    | uniq -c                                            \
    | sort -nr                                           \
    | cut -f1 -d'#'
--8<---------------cut here---------------end--------------->8---

--8<---------------cut here---------------start------------->8---
# committer and the author
git log v1.0.1..origin/HEAD --pretty=format:"%an~%cn"     \
    | awk -F '~' '{if ($1 == $2) {++cnt; print "#"$2};}' \
    | sort                                               \
    | uniq -c                                            \
    | sort -nr                                           \
    | cut -f1 -d'#' | head -n10
--8<---------------cut here---------------end--------------->8---


It is easy to produce more stats, for the example the author time
versus the committer time, or the number of newcomers (first commit),
or the hour when committing.

Do not take me wrong, it is not about blaming but it is about health
and how it could be improved. All these numbers show how Guix is
healthy. :-)


Well, all this comment is about "2. Encourage patch review for
committers.". Patch review is about trust, so what could be done to
reduce the workload of "committers"?  and smooth everything? Team
and/or delegate? Formalize "maintainer per field" (R, Lisp, Python,
OCaml, maths, etc.)? Obviously without adding too formal stuff. :-)


All the best,
simon
Ludovic Courtès Jan. 7, 2020, 11:17 a.m. UTC | #4
Hi!

zimoun <zimon.toutoune@gmail.com> skribis:

> However, let point 2 minor weak points for further discussions:
>
>  a- if the number of committers with access is more or less fixed,
> then access could be transferred (less active, less time, less
> motivation, etc.);

Sure, we should (and do) remove people who “disappeared” or retired, and
of course welcome new people willing to participate.

>  and b- the bottleneck is the patch review (even if it should be
> improved in the future with the Guix Data Service).
> Well, a- is more about human relationship, hard to fix. However, we
> should work on b- but how? What does it mean "encourage patch review"?
> Candy or beer at Guix Days? ;-)

Heh.  :-)  This is the hard part for all free software projects.  As you
noticed, there’s no shortage of patches that have fallen through the
cracks, unfortunately.

The “encourage patch review” bit in this series, in my view, is to tell
newcomers that the deal is not just that they can push their own stuff
more quickly, but also that they can help others get their stuff in in
return.  It may sound obvious to some, but it’s probably better if
explicitly stated.  Also, perhaps we’ve had the problem where committers
did not feel “entitled” to review and push other people’s patches; this
paragraph is also a way to tell them that yes, they _are_ entitled to
that.

It won’t solve the problem overnight; offering a beverage at the Guix
Days won’t solve it either but it’s a good idea anyway.  :-)

Ludo’.
Marius Bakke Jan. 7, 2020, 10:50 p.m. UTC | #5
Ludovic Courtès <ludo@gnu.org> writes:

> Hello!
>
> Just a heads-up for fellow maintainers (Tobias, Marius, Maxim): could
> you send a “+1” or whatever you deem appropriate :-) to this discussion?
> I’d like to make sure we’re on the same page.
>
>   https://issues.guix.gnu.org/issue/38846

I've read through the changes and all LGTM.

Thanks!
Ludovic Courtès Jan. 9, 2020, 10:05 p.m. UTC | #6
Hello!

zimoun <zimon.toutoune@gmail.com> skribis:

>>From the v1.0.1 to now, the repartition of committers which are not the authors:
>
>    361
>      78
>      65
>      61
>      59
>      54
>      52
>      47
>      44
>      43
>      37
>      21  (x2)
>      11
>       9
>       8
>       7  (x2)
>       6
>       5  (x3)
>       4  (x2)
>       3
>       2  (x3)
>       1  (x3)
>
> which should be compared to the number of commits per author also
> committer (first 10):
>
>    1463
>    1162
>     886
>     670
>     618
>     335
>     204
>     166
>     161
>     150

I had overlooked that; interesting, though I’m not sure what conclusion(s)
to draw.  Perhaps we should look at how these numbers evolve over time?

Related to that, attached is a script I wrote a while back to view the
number of reviews per committer (ah ha!), like so:

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)> (define r (reviewers repo))
scheme@(guile-user)> ,pp (sort (map car r) <)
$3 = (0
 ;; long tail omitted…
 1
 1
 1
 1
 2
 2
 2
 2
 2
 4
 5
 5
 6
 9
 9
 9
 10
 11
 13
 13
 17
 18
 19
 30
 37
 37
 38
 40
 40
 45
 48
 51
 59
 63
 72
 84
 85
 88
 99
 181
 186
 264
 287
 506
 526
 1620)
--8<---------------cut here---------------end--------------->8---

This is for all-time commits, so not all that representative, but could
be used as a starting point for the statistician in you.  :-)

Ludo’.
(use-modules (git)
             (git repository)
             (git reference)
             (git oid)
             (git tag)
             (git commit)
             (git structs)                        ;signature-email, etc.
             (srfi srfi-1)
             (srfi srfi-26)
             (ice-9 match)
             (ice-9 vlist))

(define commit-author*
  (compose signature-name commit-author))
(define commit-committer*
  (compose signature-name commit-committer))

(define-syntax-rule (false-if-git-error exp)
  (catch 'git-error
    (lambda () exp)
    (const #f)))

(define* (fold-commits proc seed repo
                       #:key
                       (start (reference-target
                               (repository-head repo)))
                       end)
  "Call PROC on each commit of REPO, starting at START (an OID), and until
END if specified."
  (let loop ((commit (commit-lookup repo start))
             (result seed))
    (let ((parent (false-if-git-error (commit-parent commit))))
      (if parent
          (if (and end (oid=? (commit-id parent) end))
              (proc parent result)
              (loop parent (proc parent result)))
          result))))

(define (reviewers repo)
  "Return a list of review count/committer pairs."
  (define vhash
    (fold-commits (lambda (commit result)
                    (if (string=? (commit-author* commit)
                                  (commit-committer* commit))
                        result
                        (vhash-cons (commit-committer* commit) #t
                                    result)))
                  vlist-null
                  repo))

  (define committers
    (delete-duplicates
     (fold-commits (lambda (commit result)
                     (cons (commit-committer* commit)
                           result))
                   '()
                   repo)))

  (map (lambda (committer)
         (cons (vhash-fold* (lambda (_ count)
                              (+ 1 count))
                            0
                            committer
                            vhash)
               committer))
       committers))

(define (reviewer< r1 r2)
  (match r1
    ((count1 . name1)
     (match r2
       ((count2 . name2)
        (< count1 count2))))))

(libgit2-init!)

(define repo
  (repository-open "."))
Simon Tournier Jan. 10, 2020, 3:49 p.m. UTC | #7
Hi Ludo,

On Thu, 9 Jan 2020 at 23:05, Ludovic Courtès <ludo@gnu.org> wrote:

> >>From the v1.0.1 to now, the repartition of committers which are not the authors:
> >
> >    361
> >      78
> >      65
> >      61
> >      59
> >      54
> >      52
> >      47
> >      44
> >      43
> >      37
> >      21  (x2)
> >      11
> >       9
> >       8
> >       7  (x2)
> >       6
> >       5  (x3)
> >       4  (x2)
> >       3
> >       2  (x3)
> >       1  (x3)
> >
> > which should be compared to the number of commits per author also
> > committer (first 10):
> >
> >    1463
> >    1162
> >     886
> >     670
> >     618
> >     335
> >     204
> >     166
> >     161
> >     150
>
> I had overlooked that; interesting, though I’m not sure what conclusion(s)
> to draw.  Perhaps we should look at how these numbers evolve over time?

If one re-associates the number of commits as committers to the number
of commits as authors, and computes the ratio (in percent and sorted),
the result is:

248.00
128.26
105.00
100.00
100.00
67.16
53.61
45.49
42.86
41.60
40.00
28.28
28.13
23.12
23.08
19.40
14.45
10.23
8.57
7.00
6.60
6.25
5.88
5.75
5.75
5.19
3.21
2.63
1.84
1.47
1.31
.73

To be clear, the person with the score of 53.61% "reviews" half as
many commits they authors.

And the mean is almost 37%. Cool, no?


Well, it is not possible to draw an strong conclusion, just a weak
one: Guix is healthy. ;-)

(For example, the number of commits should be weighted with the number
of lines changed.)


And yes, it should be interesting to see how evolves the commits rate
(as authors, as committers) over the time.


> Related to that, attached is a script I wrote a while back to view the
> number of reviews per committer (ah ha!), like so:

Cool!

Maybe this kind of metrics could be reported to the Guix Data Service.
Well, let start by play with your script. ;-)


> This is for all-time commits, so not all that representative, but could
> be used as a starting point for the statistician in you.  :-)

Héhé! You got me. ;-)


Cheers,
simon
Ludovic Courtès Jan. 13, 2020, 10:01 a.m. UTC | #8
Hello,

zimoun <zimon.toutoune@gmail.com> skribis:

> To be clear, the person with the score of 53.61% "reviews" half as
> many commits they authors.
>
> And the mean is almost 37%. Cool, no?
>
>
> Well, it is not possible to draw an strong conclusion, just a weak
> one: Guix is healthy. ;-)

Oh yes, that’s an interesting (and nice!) conclusion.

Thanks,
Ludo’.