[Gnu-arch-users] what's all the fuss about merge requests?

[Tom Lord]

Here is a message that illustrates some of why we're talking about
merge request formats these days.  Various names left off since this
is (less than, even) a draft and the discussion and work are still
ongoing.


Date: Fri, 2 Jul 2004 20:37:45 -0700 (PDT)
From: Tom Lord <address@hidden>
To: address@hidden
CC: xxxxxx, xxxxxx, address@hidden, xxxxxx
Subject: Re: tla stable branch


Ok.... so --- revising based on your feedback, here's a revised set of
rules.

I've added a friend of mine (and the project's) to the CC list who I
would like to keep informed about this discussion.



        The Arch Project Co-Maintainer Merge Approval Process
                     by maintainers:
                        xxxxxx
                        James Blackwell
                        xxxxxx
                        Tom Lord (Chief Architect, document author)
                        Andrew Suffield


Bug Goo gives us a pool of pending merge requests.   I assume that
votes are recorded there?   When an issue receives a vote that
determines the fate of the bug, it's either queued for merging or
rejected.   

I think that the system needs a combination of "votes" and
"directives".  Votes are roll-calls of maintainers voting in various
ways (most recent vote per maintainer counts).  Directives can trigger
actions that the voting system makes only optional rather than
deterministic.

One example of a directive is a redirection of the merge source.
Suppose that a patch arrives.  It's almost right and I want to fix it
rather than kick it back for replay.  I should be able to redirect the
merge source.

I think we want to offer users a definate service-level guarantee:
they get a reply for the patch in 72 hours.   The reply can be any
of the following things:

        ~ accepted
          Your merge went through, perhaps with revisions.

        ~ rejected
          We are not interested in this change at all.

        ~ returned
          We are interested in this but will not merge it 
          right now.  You're encouraged to make the changes
          suggested in the Bug Goo discussion (especially the
          messages containing votes) and resubmit, no sooner than
          48 hours from now (unless the maintainers have said
          otherwise).

        ~ confused,holding
          We are confused about your patch and haven't come to
          a resolution yet, however, for now we're holding on 
          to it.   You should join the mailing list for the 
          issue [*] if you want to help resolve the confusion.

        ~ confused,returned
          We are cnofused about your patch and haven't come to a
          resolution yet.  We're returning it to you with our
          various comments attached.   If you would like to try 
          again, please wait 48 hours unless the maintainers
          have said otherwise.


To a first approximation, we're all equals and we're interested in
pair programming.  We're all conservative about changes to arch.  We
all want to give excellent turnaround time and a friendly face to each
other and to other contributors.

Putting those generalities together, I think that "by default", we
want a rule that if two of us agree about something, and there is no
substantial disagreement from others, then whatever those two agree on 
happens.   However, it's better to err by not merging than by merging
(since we're conservative) and it's better to err by returning or
expressing confusion than to err by rejecting (since we want to to be
efficient and friendly).

A straw-man first idea is that we can each vote:

        accept
        reject
        return

and if any of those categories wins by N>=2 to 0, the pair programming
rule is satisfied, and the result is determined.

Strict pair programming will never work.  If we leave it at that, we
will devalue the meaning of all three kinds of votes.   Those three
possible votes, "accept", "reject", "return" should ideally mean "I
have looked at this carefully and I say we should ____".

What will really happen, if we leave it at that, is that we'll all
start cheating.   I'll say "Hey, Andrew, I just submitted 5 changes
and approved them.   Trust me, they're fine, please approve them."
And he'll think to himself "Yeah, right." and look at them and decide
for himself -- but even so, when he looks at them, he'll be biased.
He'll think it probable that I'm telling the truth and that my
judgement is right.   He _won't_ look at them all that carefully.

I happen to think that that dynamic is fine --- it isn't bad that
the maintainer's will cut corners that way.   What would be bad is for
it to happen by accident.

After months of that corner-cutting, it will be habitual.  It won't
have to be negotiated in each case.   People will just "know" when to
go ahead and approve something for someone else.   And it'll start
happening that some things get "casually approved" by two people and
"carefully approved by none".   Which violates our conservativism.

So, I want to make the short-cut official and safe.   I want people
to be able to say "I'll approve, but the other approver should look at
this more carefully than I did."

So, the next straw-man idea is the votes:

        accept
        weak-accept
        return
        weak-return
        reject
        weak-reject

so that these are clear votes:

                        results:
                        accept    accept    return  return    reject reject
      vote counts:
        accept          >=2       1         0       0         0      0
        weak-accept     d/c       >= 1      0       0         0      0
        return          0         0         >=2     1         0      0
        weak-return     0         0         d/c     >= 1      0      0
        reject          0         0         0       0         >=2    1
        weak-reject     0         0         0       0         d/c    >=1



That strawman has a forseeable bug, too.  Some maintainers might vote
weakly on a patch followed by others voting unamimously and stongly.
It shouldn't always be necessary to get a weak voter to retract his
vote.  On the other hand, lots of dissenting but weak votes does
suggest that something is up and human attention is required.   So, I
would balance those with the third strawman:



          results:
                      accept             return             reject
  vote counts:                     |                  |                    
   accept          >=2   >=2   >=1 |  0     0     0   |  0     0     0
   weak-accept     d/c   d/c   >=1 |  <=1   0     0   |  <=1   0     0
   return          0     0     0   |  >=2   >=2   >=1 |  0     0     0
   weak-return     <=1   0     0   |  d/c   d/c   >=1 |  0     <=1   0
   reject          0     0     0      0     0     0      >=2   >=2   >=1
   weak-reject     0     <=1   0      0     <=1   0      d/c   d/c   >=1


which makes the voting system immune to up to one stray weak-* vote.

Sometimes patches will arrive that many would agree need my review.
Perhaps sometimes there will be disagreements among maintainers and
someone will think "Screw it: this is up to Tom".   One choice is to
handle that all informally: just send me email.  But that's
unreliable.  I might not have time to reply just then.  That kind of
email can get dropped.   It's better to make "get Tom" a persistent
part of the voting system.   So, straw-man 4 adds the possibility of
voting "flag":

          results:
                      accept             return             reject
  vote counts:                     |                  |                    
   accept          >=2   >=2   >=1 |  0     0     0   |  0     0     0
   weak-accept     d/c   d/c   >=1 |  <=1   0     0   |  <=1   0     0
   return          0     0     0   |  >=2   >=2   >=1 |  0     0     0
   weak-return     <=1   0     0   |  d/c   d/c   >=1 |  0     <=1   0
   reject          0     0     0      0     0     0      >=2   >=2   >=1
   weak-reject     0     <=1   0      0     <=1   0      d/c   d/c   >=1
   flag            0     0     0      0     0     0      0     0     0


After responding to a flag, it shouldn't be necessary for the flag
raisers to explicitly lower the flags.  At the same time, I should be
free to reply in some way other than voting a (too be added later)
"trump" vote.   For example, I might want to clear a flag raised
because "I'm not sure this option name is right" and I might want to
reply "the option name is fine although I haven't reviewed the patch
beyond that".

Therefore, I'd like to ask for a chief-architect-only _directive_ (not
vote) by which I can convert either a specific or all "flag" votes
into non-votes.

Speaking of my special powers:  please indulge me by giving me trump
cards.   That way I can just "do stuff" either to clear outstanding
deadlocks or because I'm really sure it's necessary.   (For example:
perhaps the FSF wants me to quickly update the copyright notices in
some way.)

So: strawman 5 adds the "trump" votes, available only to the c.a.:

          results:
                      accept            return            reject
  vote counts:                   |                 |                 |
   trump-accept  1   0   0   0   | 1   0   0   0   | 1   0   0   0   |
   trump-return  0   0   0   0   | 0   0   0   0   | 0   0   0   0   |
   trump-reject  0   0   0   0   | 0   0   0   0   | 0   0   0   0   |
   accept        d/c >=2 >=2 >=1 | d/c 0   0   0   | d/c 0   0   0   |
   weak-accept   d/c d/c d/c >=1 | d/c <=1 0   0   | d/c 0   <=1 0   |
   return        d/c 0   0   0   | d/c >=2 >=2 >=1 | d/c 0   0   0   |
   weak-return   d/c <=1 0   0   | d/c d/c d/c >=1 | d/c <=1 0   0   |
   reject        d/c 0   0   0     d/c 0   0   0     d/c >=2 >=2 >=1
   weak-reject   d/c 0   <=1 0     d/c 0   <=1 0     d/c d/c d/c >=1 
   flag          d/c 0   0   0     d/c 0   0   0     d/c 0   0   0    

Again, given considerations such my historical relation to the project
and, perhaps more importantly, my obligations to the GNU project, 
I will need some directives:

        freeze  --- only c.a. trump votes have effect for now
        thaw    --- back to normal

I don't personally have an immediate need for "slush" mode (whatever
that might mean) but it might be good to keep in mind.

The above rules give us conservativism.   I think they are likely to
give us good throughput among ourselves.  What about good throughput
and friendly face?

The goal was to give an informative reply for every patch w/in 72
hours.   What the voting system strawmen still don't solve is what do
if 72 hours pass without a conclusive vote.

Gratuitously kicking back an unresolved patch to someone only to have
them resubmit is not a friendly face.   By default, we should hold
onto most confusing patches, at least for a little while, and try to
resolve them.

On the other hand, sometimes we'll generally agree about an unresolved
patch that no resolution is worth working on and so, kick it back
labeled "confused" to the submitter.

Some of that can be automated.  So, the next thing to add to the
voting system, in addition to the above rules, is:


       If the table for accept/return/reject doesn't
       apply after 72 hours then:

          results:
                 confused-hold       confused-return
  vote counts:
   trump-accept     0    0    0   
   trump-return     0    0    0   
   trump-reject     0    0    0   
   accept           >=1  d/c  d/c
   weak-accept      d/c  >=2  d/c       otherwise
   return           d/c  d/c  d/c 
   weak-return      d/c  d/c  d/c 
   reject           d/c  d/c  d/c 
   weak-reject      d/c  d/c  d/c 
   flag             d/c  d/c  1 


Finally, what happens _after_ 72 hours to "confused-hold" patches?

Just a periodic email reminder to maintainers and submitters is _ok_
except that, as noted in previous discussion, we want two levels 
of "begging off" from such reminders, hence two more possible votes
(for which all of the columns in the tables above have "d/c") are:

   unable
     Meaning: "I'm not voting on this one and never will."

   abstain
     Meaning: "I'd rather not have to vote on this one."

When the periodic pass over older-than-72-hour patches happens:

    1. If nobody has not voted, all "abstain" votes are converted
       to non-votes.

    2. If, after that, everyone has still voted (there were no
       abstains) then all "unable" votes are converted to non-votes.

    3. Finally, all non-"unable" maintainers are reminded about the 
       patch.


Next topic: automatic testing.

Asynchronously with merging, a separate process repeatedly runs the
tests on the mainline.  It maintains a tag-only branch with the
outcomes of each test result.

The latest revision in tag-only is always of the latest
revision that passes the test suite.   There may be gaps:

        stable                          tests

        patch-a      ----------->       patch-b
        patch-a+1          ,---->       patch-b+1
        patch-a+2         /
        patch-a+3    ----'

As a final rule for the voting system, a final modification to the
rules above:

If the latest revision in stable has not passed for at least 24 hours
then the patch manager is said to be in "regression mode".

Two additional possible votes are available:

    fix-regression
    weak-fix-regression
      These normally behave like "approve" and "weak-approve".

      When the patch manager is in regression mode, the usual
      rules apply with the qualification that, to be merged, 
      a patch must have either a "trump-approve" vote or at least
      one "fix-regression" vote.


And, as a final directive, any maintainer should be able to send
the directive:

        test $patch

and receieve in return mail (and on a web page) the result of a test
run of a recent mainline revision, with $patch applied.  The
directive:

        test-approve $patch

should run and return tests and, if the tests are a clear pass,
automatically register an "approve" vote for the sender.


-t