[haiku-development] Re: Change patches workflow [was Final Set*UIColor Patch, Version 3e]

  • From: looncraz <looncraz@xxxxxxxxxxx>
  • To: haiku-development@xxxxxxxxxxxxx
  • Date: Tue, 1 Dec 2015 11:24:01 -0600

On 11/30/2015 00:06, Adrien Destugues wrote:

Sorry, wanted to reply sooner, been busy, still busy ;-)

1. Submit a proposal through the dev mailing list / BugTracker
A dedicated tool such as gerrit would be more suitable.

Sure.

2. Await community feedback re worthiness and implementation basics
"community feedback" can often become a lot of bikeshedding. mmu_man can
comment on how he tried to submit a patch to QEmu, and they asked him to
rewrite half of their code first.

Then don't do that. This will happen no matter what - the community must remain involved. In addition, hearing about these issues will give you a good idea of what someone may intend to do in the future and may inform the direction you should go when designing a patch.


3. Initial patches should be review incrementally for implementation of
design
4. A vote is held to assign responsible reviewer(s), volunteers welcome
A vote for each submitted patch? That's a good way for things to never
get merged. After two weeks people will stop voting because the process
is too boring, and/or people will get assigned patches they don't care
about (which is not motivating to have a look at those).

Poorly worded on my part, the vote would just be by whomever wanted to say something, or if someone wanted to volunteer. It would not a full vote, just a way to nominate someone to be the responsible reviewer for a certain patch. It is immensely helpful for a potential contributor to know with whom they will be dealing - so, for example, they can email them privately and spark up a conversation in IRC or something.


5. Submission candidate patches are reviewed for bugs, design issues
6. Bugs are fixed, reworkings made
7. Patches are reviewed for style violations and cleanup
8. Style violations are repaired exclusively by originating author(s).
If he didn't move to other things already after the time it took to go
through the first 7 steps. What if someone else wants to takeover the
patch? Are they not allowed to fix the existing one?

Most of these steps can actually happen at once and are already happening anyway one way or another. And, yes, ownership of the patch can be transferred, and other people can always submit patches to patches (since these patches will be maintained in their own branch somewhere). However, so long as the original author is working on it, only the original author should be making changes (or, at least, directing changes). The point is that the reviewer doesn't make code changes, because sometimes code is doing something other than what the reviewer may think (one of my earlier submissions was modified in such a way that it broke because of this).


9. Final review
10. Commit to experimental branch
11. Experimental branch patches moved to master after community testing
This means you expect "the community" to run the experimental branch.
Two possible outcomes:
* The master branch is useless, because everyone runs experimental (same
thing we have with our 3-year-old release currently). Everyone expects
the experimental branch to be stable and complain when it isn't, or:
* Everyone uses the master branch, and 50% of bugreports get closed with
"already fixed in the experimental branch".

No, the experimental branch is a staging area for new commits. The master branch will have all patches that are in exp that have been shown to be stable, functional, safe, etc. added on a regular basis. However, you will certainly have a few times where bugs are fixed/existing in one branch and not the other. That's a minor issue, even more so when you have a shorter-lived testing cycle.

For example, if my color updating patch was applied to exp right now I'd be getting feedback about any possible issues that could never be discovered in review. People who are interested in staying to the bleeding edge will use nightly/exp, those who care more about a stable, clean, environment will use nightly/stable This seems pretty par for the course.

Style fixes are nearly LAST, but glaring issues are pointed out when
noticed, which will help eliminate the duplication of effort and submitter
frustration.
You can work any way you like. You can post small patches whenever you
have something working and ask for early review. Expect people to point
out both style and design issues - why should they ignore style issues
as they spot them?

I specifically stated that glaring issues should be pointed out when noticed, but minor issues such as missing spaces, unimportant typos, etc., should be ignored. There's nothing more annoying than going through a file many times repairing minor styling issues and then, days later, being told to just delete the file outright, or to completely implement it another way.

As a submitter, try to find a good balance: submitting a patch too often
will make the devs think "oh no, not again", especially if it doesn't
include all the feedback from the previous run. Not submitting often
enough, you risk your next iteration to be in a wrong direction in a
fundamental way, which means major rework for you and frustration on
your side.

Submission should be an interactive process. This is the hand-holding you need to do if you want to get more quality submissions. As a submitter gains experience in this process, the need for review declines and the process becomes:

1. Submit proposal
2. Feedback
3. Build patch
4. Review
5. Live testing in exp branch
6. Application to master/stable after any repairs performed

--The loon


Other related posts:

  1. Home
  2. haiku-development
  3. Archive
  4. 12-2015