Patch Process

Hey guys,

In the interest of streamlining the patch-submission process, we've
been thinking about writing down the informal vetting process we've
been using for patches. The first cut of this is written down in the
wiki:

http://wiki.rubyonrails.org/rails/pages/PatchRequirements

Any comments or questions?

Sounds cool.

You can probably mention #rails-contrib as well for finding potential reviewer.

And how about having a rule to close the ticket as invalid/wontfix if
it's been there for over 2-3 months without any updates ? And may be
get a core member to reopen it if one feels the patch has a chance of
making it to core ever. I was just going through some of the old
tickets, and I feel that very small % of them will ever make it to
core. Probably closing the remaining tickets would be good to keep
things clean on trac as some of them are even 2 years old.

One more thing I always wonder about is if I should assign the ticket
to a core member or just leave it with default assignee "core". You
think it might be a good idea to assign ticket to the particular core
member who has been the most active in the area that patch affects ?
If yes, may be you can put the mapping at that wiki page ? Just a
thought.

Thanks,
Pratik

Requiring peer review is great. Also, multiple peer review can help
mitigate the tyranny of the zealous :slight_smile:

I don't personally have time to make core patches any more but would
be happy to help the community in this way by moderating. Great ideas!

Looks about right.

Maybe expand on the warning bitsweat added to the trac home page.

"Expect your ticket to be closed with an untested, undocumented, or
incomplete resolution if it's missing tests, documentation, or
implementation. Don't panic; the ticket hasn't been killed! These
resolutions are the pathway to commit. Update your patch and reopen
the ticket."

This seems heavy-handed for patches which fix defects. I mean, if a defect
report didn't have a patch at all, it'd stay open, but if it's a *better*
report (ie closer to being fixed, just not quite all the way there) then it
gets closed? That seems... counterproductive. (Disclaimer: I'm not a
neutral party on this topic -- I've had a couple of bugs with patches
treated this way recently; it *really* made me less interested in
contributing future patches). I understand why patches get triaged, and
there is the possibility of bad patches hanging around forever, but surely
the policy of expiring old bug reports could also apply to half-baked
patches, rather than instant closure?

Also, I presume that this PatchProcess page will be rolled into or otherwise
linked from http://dev.rubyonrails.org once it's considered acceptable?

- Matt

I understand why patches get triaged, and
there is the possibility of bad patches hanging around forever, but surely
the policy of expiring old bug reports could also apply to half-baked
patches, rather than instant closure?

It's not just the possibility of patches sitting around, we had
hundreds of patches just sitting around and the submitters commented
that the lack of feedback was more frustrating than having a patch
'rejected'.

What was it about the closure that made it irksome? Tickets get
reopened all the time, closure isn't a death sentence. But it's more
honest than just having an atrophied patch sitting in the queue.

Also, I presume that this PatchProcess page will be rolled into or otherwise
linked from http://dev.rubyonrails.org once it's considered acceptable?

Yep.

So, if I’m the 3rd reviewer on someone’s patch and I leave a thumbs-up comment, it’s free for me to add the “verified” keyword?

@Josh Peek: +1 for the “don’t panic” notice. First-time contributors are easily discouraged otherwise.

Really? Cuz I had patches that got ignored, so I stopped
contributing. Having them rejected with a reason would have helped.

That said, I like the new wiki page. Simple and to the point. The
patch process becoming more transparent is a real win, thanks guys.

So, if I'm the 3rd reviewer on someone's patch and I leave a thumbs-up
comment, it's free for me to add the "verified" keyword?

That's the intention yeah.

@Josh Peek: +1 for the "don't panic" notice. First-time contributors are
easily discouraged otherwise.

It's probably worthwhile coming up with some 'canned' text to paste in
when taking certain actions. I'd guess it's much more reassuring if
you can see a path to getting your patch applied.

Really? Cuz I had patches that got ignored, so I stopped
contributing. Having them rejected with a reason would have helped.

That's the conclusion we've come to. Better to reject something
quickly, than let it atrophy forever for fear of offending someone.

That said, I like the new wiki page. Simple and to the point. The
patch process becoming more transparent is a real win, thanks guys.

So far this is just a plan, hopefully it'll help out.

So far this is just a plan, hopefully it'll help out.

To clarify, we're doing this. Time will tell if it improves the
turnaround time on patches. :slight_smile:

> I understand why patches get triaged, and
> there is the possibility of bad patches hanging around forever, but surely
> the policy of expiring old bug reports could also apply to half-baked
> patches, rather than instant closure?

It's not just the possibility of patches sitting around, we had
hundreds of patches just sitting around and the submitters commented
that the lack of feedback was more frustrating than having a patch
'rejected'.

Yes, lack of feedback sucks, but there are forms of feedback other than
"closed defect: incomplete".

What was it about the closure that made it irksome? Tickets get
reopened all the time, closure isn't a death sentence.

'Closed' is one of the fundamental states of a ticket, and it means
something fairly definite to me -- something along the lines of "absent some
exceptional condition, this is done with".

The thing that really got to me was that, if I had just reported the bug and
left off the patch, it would have stayed open for someone else to deal with.
However, having taken the extra time to hunt down a solution and post the
patch (however incomplete it may have been), the ticket was closed and,
absent further interaction from me, it was as though the defect was never
reported. If the patch is dodgy, then reject the *patch*, not the entire
defect report. I very nearly opened a completely separate bug report
without a patch just to see what would happen.

What would happen if a bug was reported by one person but an incomplete
patch contributed by someone else? Would the report get closed (which to
the initial reporter suggests the issue is solved) because the patch is
dodgy?

- Matt

I have nothing against feedback. Feedback is good. The only way
you're going to get better patches over time is if you communicate how the
patches you're getting are insufficient. My *only* gripe is with the manner
in which the communication is being performed. I have a particular
definition of what 'closed' means to me, and it's not "OK, time to write a
test case".

- Matt

Does the fact that it's being said that a closure isn't a death
sentence, just a matter of feedback change your opinion of 'closed'?
Do the 'untested', 'undocumented' resolutions hurt less?

I agree there, because nothing gets peoples attention like closing
their ticket :wink:

Usually patches with good units tests that prove a bug are fixed
rather quickly. The problem is we have tons of unverified (and bogus)
defect reports with no easy way to prove them. No one has the time to
personally debug someone elses problems.

We should post a note to try the Mailing List or IRC channel first if
you are unsure about your patch.

> I have a particular
> definition of what 'closed' means to me, and it's not "OK, time to write a
> test case".
>
> - Matt

Does the fact that it's being said that a closure isn't a death
sentence, just a matter of feedback change your opinion of 'closed'?

No, because it's an emotional response, not a purely intellectual one.

Do the 'untested', 'undocumented' resolutions hurt less?

They're not "resolutions", they're states. You set them as flags or
something. The very meaning of word "resolution" implies some sort of
finality, but a defect report that isn't fixed isn't resolved.

- Matt

> What would happen if a bug was reported by one person but an incomplete
> patch contributed by someone else? Would the report get closed (which to
> the initial reporter suggests the issue is solved) because the patch is
> dodgy?

Usually patches with good units tests that prove a bug are fixed
rather quickly.

(Good thing you put 'usually' in there, because I've got a patch in the
system that fixes a bug in the *test suite* that's been hanging for over a
week now)

Is it the policy of the Rails project that defect reports without a
verifiable test case are invalid and will be closed?

If a project was going to adopt that policy, something like Rails would be
the most likely to be able to pull it off, because every user should be a
developer. Having spent a few hours a day the last few weeks helping on the
#rubyonrails IRC channel, though, I'm not sure it's a practical proposition
-- most people don't "get" testing, and plenty of Rails' users aren't
sufficiently good programmers to be able to write a solid test case for
something as complex as Rails.

The problem is we have tons of unverified (and bogus)
defect reports with no easy way to prove them.

So mark them "more info required" with a note that they need to have more
info attached or they'll be closed in N days/weeks/months.

No one has the time to
personally debug someone elses problems.

<advocate type="devil">
That cuts both ways, though -- I, as a developer, don't have time to debug a
problem in Rails. That problem isn't mine -- I didn't write the buggy code
that's causing the hassle. It's *your* fault, dammit.
</advocate>

In reality, I know all about the OSS project philosophy, I'm a great
believer in it myself. If you want support, take out a support contract,
and all that.

Going back to my original point, though, about actively *closing* tickets
that document *real* bugs that are trivially verifiable[1] simply because
the patch isn't sufficient is only reasonable in one circumstance -- if
defect reports without test cases are summarily closed. I haven't seen that
policy documented anywhere (and I would imagine something that it would want
to be in big letters on the front page), and in fact the exact opposite is
stated on the dev front page -- "Tickets are fine". I'm just reporting an
irritating inconsistency in the handling of tickets, biased against
well-meaning but insufficiently clued contributors (which are the sort of
people I'd be wanting to encourage, not discourage, if it were my project).

- Matt

[1] one of the bugs I found I didn't even have to run the code to find it --
a desk check was enough to make it perfectly obvious.

Is it the policy of the Rails project that defect reports without a
verifiable test case are invalid and will be closed?

I guess that'll be a nice policy to have considering number of open
tickets trac has. Well, that seems to be an undocumented policy, a
good thing anyways.

I'm not sure it's a practical proposition
-- most people don't "get" testing, and plenty of Rails' users aren't
sufficiently good programmers to be able to write a solid test case for
something as complex as Rails.

Let them use core mailing list then. Which is a perfect medium.

So mark them "more info required" with a note that they need to have more
info attached or they'll be closed in N days/weeks/months.

That'd be same as closing as "incomplete".

<advocate type="devil">
That cuts both ways, though -- I, as a developer, don't have time to debug a
problem in Rails. That problem isn't mine -- I didn't write the buggy code
that's causing the hassle. It's *your* fault, dammit.
</advocate>

Oh well. That wasn't really needed. May be you can elaborate on *your* part.

Going back to my original point, though, about actively *closing* tickets
that document *real* bugs that are trivially verifiable[1] simply because
the patch isn't sufficient is only reasonable in one circumstance -- if
defect reports without test cases are summarily closed. I haven't seen that
policy documented anywhere (and I would imagine something that it would want
to be in big letters on the front page), and in fact the exact opposite is
stated on the dev front page -- "Tickets are fine". I'm just reporting an
irritating inconsistency in the handling of tickets, biased against
well-meaning but insufficiently clued contributors (which are the sort of
people I'd be wanting to encourage, not discourage, if it were my project).

Perfect in theory. But practically speaking, if there is a problem
that affects majority of people, it won't go unnoticed. And otherwise,
people can always use this very mailing list. The reason why I'm in
favor of actively closing tickets without patches/verifying tests is
the current state of trac. 100s of open tickets. And a tiny % of them
are valid bugs. And because of that, many of the proper patches go
unnoticed for long time. They just get "lost".

(Good thing you put 'usually' in there, because I've got a patch in the
system that fixes a bug in the *test suite* that's been hanging for over a
week now)

It's not on the verified patches list, so it's unlikely to get much attention.

http://dev.rubyonrails.org/report/12

Is it the policy of the Rails project that defect reports without a
verifiable test case are invalid and will be closed?

No, defect reports without test cases aren't supposed to be
automatically closed. A great way for people to help is to find
defect reports, then attach failing test cases, or at the very least
confirm that they experience the same behaviour.

However I believe you're overstating the impact of closing a defect.
There are two possibilities when someone reports a bug:

a) The bug was invalid, and caused either by the user's system, the
alignment of jupiter's moons or some other freak occurrence
b) The bug is valid, someone else will be able to verify it

Prematurely closing defects has no adverse impact on the first case.
In the second case, the submitter can simply reopen them. Now even
if you assume that closing the ticket has so completely enraged the
submitter that they've gone off to write web frameworks in ocaml,
we're still ok. Someone else will hit the same bug, and will either
reopen the original report (unlikely because trac's search is shit) or
submit a new one.

The new ticket can then be fixed, or we can repeat this process again.

The reality is that there are bugs in every system, and if there's a
defect which is so rare that only one person has ever hit it, then I'm
not going to lose any sleep over continuing to ship that bug.

> The problem is we have tons of unverified (and bogus)
> defect reports with no easy way to prove them.

So mark them "more info required" with a note that they need to have more
info attached or they'll be closed in N days/weeks/months.

That's exactly the same as closing them, as they're unlikely to
continue to receive any attention. I think lying to our submitters by
leaving an issue 'open' when no one intends to even look at it is
worse than closing it prematurely.

<advocate type="devil">
That cuts both ways, though -- I, as a developer, don't have time to debug a
problem in Rails. That problem isn't mine -- I didn't write the buggy code
that's causing the hassle. It's *your* fault, dammit.
</advocate>

In reality, I know all about the OSS project philosophy, I'm a great
believer in it myself. If you want support, take out a support contract,
and all that.

That's a complete mischaracterisation of the situation we're in, I'm
more than willing to help people fix the problems they have. I spend
a good portion of my time doing just that.

We have too many reports, so many that it's impossible to make a dent
in them. Reporters hear nothing back about their bug report, get
disenchanted and leave. The patch queue fills up to the point we
don't know where to start. If we get better turnaround time on
patches and encourage contributors, and in the process prematurely
close a few edge-case defects. So be it.

Going back to my original point, though, about actively *closing* tickets
that document *real* bugs that are trivially verifiable[1] simply because
the patch isn't sufficient is only reasonable in one circumstance -- if
defect reports without test cases are summarily closed. I haven't seen that
policy documented anywhere (and I would imagine something that it would want
to be in big letters on the front page), and in fact the exact opposite is
stated on the dev front page -- "Tickets are fine". I'm just reporting an
irritating inconsistency in the handling of tickets, biased against
well-meaning but insufficiently clued contributors (which are the sort of
people I'd be wanting to encourage, not discourage, if it were my project).

Indeed, while reports without failing test cases aren't as likely to
get attention, they're still valuable information, and could provide
indications of where to start contributing for people who are keen to
help out. Please don't close them unless you've tried to reproduce
the bug, and are unable to do so.

However I don't buy the sky is falling hypothesis of closing a ticket
prematurely, just reopen it, explain it's still a bug. If the
disagreement continues, start a thread here.