Patch review/application process

Patch review/application process

[Daniel Schoepe]

[-- Attachment #1: Type: text/plain, Size: 3121 bytes --]

Hello,

as many of you have probably noticed, the time after which patches are
reviewed and/or applied is considerably higher lately than it was, for
example, earlier this year. My subjective impression is that there is
also a recent increase in contributions and general activity for/about
notmuch. Since long waiting times between sending a patch and receiving
a response will probably deter some (potential) contributors from
working / continuing to work on notmuch, I find this to be an important
issue. There is also a number of patches that have been reviewed by
long-term contributors, but are then seemingly forgotten (I can find
some concrete examples of this, if this claim is in doubt).

For me notmuch is a huge improvement compared to existing clients (with
the somewhat obvious exception of sup which comes close), so I'd really
hate to see this project stagnate or "wither" because of this.

I am aware that this is a volunteer project and hence the intent of this
post is not to urge Carl Worth or anyone else to "hurry up!" with the
patch review. Instead I'd like to discuss approaches on how to deal with
this problem. Here a few ideas I was able to come up with:

- Further delegate responsibility for the various parts, specifically
  the emacs UI, which has a large number of outstanding patches. I'd be
  in favor (if Carl is okay with it, of course) of giving one or more
  people (Jameson and Austin came up as possible candidates when
  discussing this on IRC, if they are willing) the authority to apply
  patches for the emacs UI, similar to how patches for bindings are
  handled.

- (Re)try some patch/issue management software: Since patches are easily
  forgotten if they just float around in several months old mails, it
  might be prudent to use something to keep track of patches or issues
  these patches address. I know that the patchwork instance didn't work
  out so well, partly because it didn't recognize new versions of sent
  patches. An alternative might be an issue-based system, which would be
  comfortably usable if it supported discussing issues via mail instead
  of having to use some web interface. I think this is supported by
  redmine.
  
  A mechanism to share notmuch tags between users could probably also be
  adapted for this purpose, but this would make it harder for
  non-notmuch users to discuss issues / see existing with the same
  comfort. (Package maintainers or people who want to check what
  outstanding flaws exist before migrating to notmuch come to mind).

- Some kind of "voting system" that gets a patch applied if some
  number of "trusted" contributors reviewed a patch and think it is
  good. I haven't given this idea much thought and I guess it might
  lead to a "lack of direction / guiding principles" in the development
  of notmuch.

I'm probably overlooking some downsides of those ideas, so I'd like to
hear any responses and/or other approaches to deal with this (Of course,
I'm also open to arguments showing that I'm making too big a deal out of
this :)).

Cheers,
Daniel

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: Patch review/application process

[David Bremner]

On Tue, 25 Oct 2011 22:42:33 +0200, Daniel Schoepe <daniel@schoepe.org> wrote:

>   A mechanism to share notmuch tags between users could probably also be
>   adapted for this purpose, but this would make it harder for
>   non-notmuch users to discuss issues / see existing with the same
>   comfort. (Package maintainers or people who want to check what
>   outstanding flaws exist before migrating to notmuch come to mind).

FWIW, I'm currently working on such a mechanism, and it should be done
before _too_ long.

d

Re: Patch review/application process

[Jani Nikula]

On Tue, 25 Oct 2011 22:42:33 +0200, Daniel Schoepe <daniel@schoepe.org> wrote:
> as many of you have probably noticed, the time after which patches are
> reviewed and/or applied is considerably higher lately than it was, for
> example, earlier this year. My subjective impression is that there is
> also a recent increase in contributions and general activity for/about
> notmuch. Since long waiting times between sending a patch and receiving
> a response will probably deter some (potential) contributors from
> working / continuing to work on notmuch, I find this to be an important
> issue. There is also a number of patches that have been reviewed by
> long-term contributors, but are then seemingly forgotten (I can find
> some concrete examples of this, if this claim is in doubt).

The good thing is, there are contributions and review. The bad thing is,
unless you've hung around long enough, you don't know if the reviewers
are people whose comments you should really pay attention to or not, and
either way, fixing the patches seems pointless and frustrating if they
don't get applied anyway.

A MAINTAINERS file might be helpful in identifying some of the key
people. AUTHORS could be updated to include people with not
insignificant contributions.

> - Further delegate responsibility for the various parts, specifically
>   the emacs UI, which has a large number of outstanding patches. I'd be
>   in favor (if Carl is okay with it, of course) of giving one or more
>   people (Jameson and Austin came up as possible candidates when
>   discussing this on IRC, if they are willing) the authority to apply
>   patches for the emacs UI, similar to how patches for bindings are
>   handled.

Agreed. I sincerely hope Carl and the candidates are willing. And if
not, a favorable review from the long-term contributors (see AUTHORS
above) should carry more weight in getting the patches applied in a
timely manner.

> - (Re)try some patch/issue management software: Since patches are easily
>   forgotten if they just float around in several months old mails, it
>   might be prudent to use something to keep track of patches or issues
>   these patches address. I know that the patchwork instance didn't work
>   out so well, partly because it didn't recognize new versions of sent
>   patches. An alternative might be an issue-based system, which would be
>   comfortably usable if it supported discussing issues via mail instead
>   of having to use some web interface. I think this is supported by
>   redmine.

If the problem is lack of time, I'm not sure if setting up and
maintaining some world facing web service would help things.

>   A mechanism to share notmuch tags between users could probably also be
>   adapted for this purpose, but this would make it harder for
>   non-notmuch users to discuss issues / see existing with the same
>   comfort. (Package maintainers or people who want to check what
>   outstanding flaws exist before migrating to notmuch come to mind).

Hmm, if there was a way to reference messages in Mailman/pipermail
archive using message IDs, I'm sure it would be trivial to export the
tags to simple html with links to the mails in the mailing list archive.

> - Some kind of "voting system" that gets a patch applied if some
>   number of "trusted" contributors reviewed a patch and think it is
>   good. I haven't given this idea much thought and I guess it might
>   lead to a "lack of direction / guiding principles" in the development
>   of notmuch.

I wouldn't put too much emphasis on creating a voting system or a
process. I do have hopes for the tag sharing mechanism helping in
tracking the reviewed patches, though. That means figuring out whose
tags to trust anyway.

> I'm probably overlooking some downsides of those ideas, so I'd like to
> hear any responses and/or other approaches to deal with this (Of course,
> I'm also open to arguments showing that I'm making too big a deal out of
> this :)).

Thanks for bringing this up.


BR,
Jani.

Re: Patch review/application process

[Daniel Schoepe]

[-- Attachment #1: Type: text/plain, Size: 1889 bytes --]

On Wed, 26 Oct 2011 18:29:37 +0000, Jani Nikula <jani@nikula.org> wrote:
> The good thing is, there are contributions and review. The bad thing is,
> unless you've hung around long enough, you don't know if the reviewers
> are people whose comments you should really pay attention to or not, and
> either way, fixing the patches seems pointless and frustrating if they
> don't get applied anyway.
> 
> A MAINTAINERS file might be helpful in identifying some of the key
> people. AUTHORS could be updated to include people with not
> insignificant contributions.

I agree, that sounds like a good idea.

> If the problem is lack of time, I'm not sure if setting up and
> maintaining some world facing web service would help things.

This idea was mainly intended to prevent patches from being forgotten,
an issue not entirely orthogonal to the main point.

> > - Some kind of "voting system" that gets a patch applied if some
> >   number of "trusted" contributors reviewed a patch and think it is
> >   good. I haven't given this idea much thought and I guess it might
> >   lead to a "lack of direction / guiding principles" in the development
> >   of notmuch.
> 
> I wouldn't put too much emphasis on creating a voting system or a
> process. I do have hopes for the tag sharing mechanism helping in
> tracking the reviewed patches, though. That means figuring out whose
> tags to trust anyway.

Yes, I didn't envision some process that's formalized down to every
detail, but more of a general guideline like "if at least n people out
of {set of trusted contributors} agree and there's no controversy about
the patch, anyone with commit access is allowed to apply the patch". I
think this idea would help mainly with getting small patches like [1]
applied more quickly.

[1] id:"1309890780-8214-1-git-send-email-pieter@praet.org"

Cheers,
Daniel

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: Patch review/application process

[David Bremner]

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]

On Tue, 25 Oct 2011 22:42:33 +0200, Daniel Schoepe <daniel@schoepe.org> wrote:

>. There is also a number of patches that have been reviewed by
> long-term contributors, but are then seemingly forgotten (I can find
> some concrete examples of this, if this claim is in doubt).

<pet-project-promotion>
Maybe you can tag those patches as "notmuch::reviewed" using nmbug? [1]
My idea is that 

   notmuch search tag:notmuch::patch and tag:notmuch::reviewed

should give a kind of consensus set of "ready to go" patch sets. Don't
worry about if I or someone else disagrees with your assessment, we can
always untag it, and leave a comment in the commit log. [2]  

</pet-project-promotion>

There are also plenty of patches that are not reviewed at all. I'm not
defending the state of patch integration, but I think we could use some
more reviews as well.

d

[1]: http://notmuchmail.org/nmbug/
[2]: nmbug log $id could be defined as something like "cd $HOME/.nmbug && git log -- tags/$(echo $id | sha1sum -)"

[-- Attachment #2: Type: application/pgp-signature, Size: 315 bytes --]

Re: Patch review/application process

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 584 bytes --]

On Tue, 01 Nov 2011 11:28:45 -0300, David Bremner <david@tethera.net> wrote:
> Maybe you can tag those patches as "notmuch::reviewed" using nmbug? [1]
> My idea is that 
> 
>    notmuch search tag:notmuch::patch and tag:notmuch::reviewed
> 
> should give a kind of consensus set of "ready to go" patch sets.

Don't forget notmuch::applied, which I think is actually one of the most
important.  Review is easy to see by looking at the patch thread.  Being
able to easily find which patches have not yet been applied to master is
what I'm most looking forward to.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: Patch review/application process

[David Bremner]

[-- Attachment #1: Type: text/plain, Size: 1932 bytes --]

On Tue, 01 Nov 2011 08:55:09 -0700, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> On Tue, 01 Nov 2011 11:28:45 -0300, David Bremner <david@tethera.net> wrote:
> > Maybe you can tag those patches as "notmuch::reviewed" using nmbug? [1]
> > My idea is that 
> > 
> >    notmuch search tag:notmuch::patch and tag:notmuch::reviewed
> > 
> > should give a kind of consensus set of "ready to go" patch sets.
> 
> Don't forget notmuch::applied, which I think is actually one of the most
> important.  Review is easy to see by looking at the patch thread.  Being
> able to easily find which patches have not yet been applied to master is
> what I'm most looking forward to.

Jamie knows this from IRC, but I have been using "notmuch::pushed" for
this.

One thing I think we need to clarify a bit is are we tagging whole
threads or individual messages in the database.  Because of the way
notmuch search works, I had been tagging whole threads with
notmuch::pushed (effectively to "mute" them) in the search

                notmuch search tag:notmuch::patch and \
                        not tag:notmuch::pushed

This is a bit aesthetically unappealing, anad every time someone replies
to a thread it is effectively unmuted.

Since we don't have thread tagging yet (where e.g. tags are
automagically applied to new messages I was thinking it might work have
a tag like "notmuch::todo" and something like the following workflow
(for patches)

initially tag +notmuch::patch +notmuch::todo

then when we "dispose" of the patch somehow, remove the notmuch::todo tag and
replace with
        notmuch::pushed
        notmuch::obsolete
        or...

Then we could do e.g.

     notmuch search tag:notmuch::patch and tag:notmuch::todo and tag:notmuch::reviewed

to get an "integrators queue" (Well, the more like an unordered pile
than a queue. One problem at a time).

David





[-- Attachment #2: Type: application/pgp-signature, Size: 315 bytes --]

Re: Patch review/application process

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 2047 bytes --]

On Tue, 01 Nov 2011 16:55:03 -0300, David Bremner <david@tethera.net> wrote:
> One thing I think we need to clarify a bit is are we tagging whole
> threads or individual messages in the database.  Because of the way
> notmuch search works, I had been tagging whole threads with
> notmuch::pushed (effectively to "mute" them) in the search
> 
>                 notmuch search tag:notmuch::patch and \
>                         not tag:notmuch::pushed
> 
> This is a bit aesthetically unappealing, anad every time someone replies
> to a thread it is effectively unmuted.

I have to say that I am very much against tagging threads with tags that
are really only applicable to specific messages, ie. ::patch, ::pushed
(I prefer ::applied, but whatever), etc.  For instance, a thread maybe
contain multiple patches, but it is not itself a patch.  Tagging entire
threads as ::patch means you can't do something like this:

notmuch show --format=mbox tag:notmuch::patch and not tag:notmuch::applied | git am

which would in my opinion be a shame.

> Since we don't have thread tagging yet (where e.g. tags are
> automagically applied to new messages I was thinking it might work have
> a tag like "notmuch::todo" and something like the following workflow
> (for patches)

I'm not sure why this is needed, since it seems to me that the whole
argument for tagging entire threads is that the individual messages are
*not* distinguishable from the thread.

> initially tag +notmuch::patch +notmuch::todo
> 
> then when we "dispose" of the patch somehow, remove the notmuch::todo tag and
> replace with
>         notmuch::pushed
>         notmuch::obsolete
>         or...

Please do *not* remove the ::patch tag.  There is really no reason to.
The message still contain a patch whether or not it is applied
upstream.  I really think this is important.  The addition of something
From a set of "resolution" tags (::pushed, ::applied, ::obsolete,
::rejected, etc.) should indicate resolution of the issue.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: Patch review/application process

[David Bremner]

[-- Attachment #1: Type: text/plain, Size: 931 bytes --]

On Tue, 01 Nov 2011 14:27:53 -0700, Jameson Graef Rollins <jrollins@finestructure.net> wrote:

> I'm not sure why this is needed, since it seems to me that the whole
> argument for tagging entire threads is that the individual messages are
> *not* distinguishable from the thread.

The problem is that "notmuch search foo and not bar" will return all
threads containing a message satisfying "foo" and a message satisfying 
"not bar". This makes 

     notmuch search tag:notmuch::patch and not notmuch::pushed

notmuch use.

> Please do *not* remove the ::patch tag.  

No, I don't suggest/intend to remove the ::patch tag, only a ::unresolved tag,
since there seems to be no nice way to search for (not notmuch::resolved).

>  The addition of something
> From a set of "resolution" tags (::pushed, ::applied, ::obsolete,
> ::rejected, etc.) should indicate resolution of the issue.

Here I think we agree.

[-- Attachment #2: Type: application/pgp-signature, Size: 315 bytes --]

Re: Patch review/application process

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 1223 bytes --]

On Tue, 01 Nov 2011 20:22:58 -0300, David Bremner <david@tethera.net> wrote:
> The problem is that "notmuch search foo and not bar" will return all
> threads containing a message satisfying "foo" and a message satisfying 
> "not bar". This makes 
> 
>      notmuch search tag:notmuch::patch and not notmuch::pushed
> 
> notmuch use.

Actually, I don't think that's true.  Notmuch will only return threads
that contain individual messages that satisfy the given search term.  I
actually just verified this to make sure.

> > Please do *not* remove the ::patch tag.  
> 
> No, I don't suggest/intend to remove the ::patch tag, only a ::unresolved tag,
> since there seems to be no nice way to search for (not notmuch::resolved).

We could just have a notmuch::resolved tag, which would make that easy.
The we could have additional tags to indicate how it was resolved, if
necessary.

But honestly I really think that for most effectiveness we should keep
the tag space as small as possible.  I think this is going to be
basically impossible, and that things are going to get confusing quickly
(I think this is one of the problems with tagging in general) but I
still think we should try.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: Patch review/application process

[Philip Hands]

[-- Attachment #1: Type: text/plain, Size: 1309 bytes --]

On Tue, 25 Oct 2011 22:42:33 +0200, Daniel Schoepe <daniel@schoepe.org> wrote:
...
> - (Re)try some patch/issue management software: Since patches are easily
>   forgotten if they just float around in several months old mails, it
>   might be prudent to use something to keep track of patches or issues
>   these patches address. I know that the patchwork instance didn't work
>   out so well, partly because it didn't recognize new versions of sent
>   patches. An alternative might be an issue-based system, which would be
>   comfortably usable if it supported discussing issues via mail instead
>   of having to use some web interface. I think this is supported by
>   redmine.

Since the wiki is ikiwiki, it might be worth using the bug tracking
available in ikiwiki:

  http://ikiwiki.info/tips/integrated_issue_tracking_with_ikiwiki/

or just create a dedicated instance of ikiwiki just for tracking bugs
(possibly as part of the source repository, so that bugs get carried
around with the source in git, and can be closed with the commit that
fixes them, etc.)

Cheers, Phil
-- 
|)|  Philip Hands [+44 (0)20 8530 9560]    http://www.hands.com/
|-|  HANDS.COM Ltd.                    http://www.uk.debian.org/
|(|  10 Onslow Gardens, South Woodford, London  E18 1NE  ENGLAND

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]