Patch review procedure?
Jay Parlar
submitted a patch (http://code.djangoproject.com/ticket/1994) a week
ago, and have had no feedback on it so far.
Is there some kind of monthly IRC meetup where pending patches are
discussed? Or is it just when the core developers have free time for
it?
I'm just going to keep running with the patch locally, but I hate to
run software that's different that the trunk.
Thanks,
Jay P.
Michael Radziej
> Just wondering what the procedure is for getting a patch accepted?
You hit it! It would be so nice to have some kind of process in place.
And not just random wait-and-see, as it is currently.
Currently, it's a lot easier for me to just patch django and not share
my patches. Or try to do it without patches. Then I don't have to polish
them up for everybody's use, I don't have to maintain individual patches
(just my complete patch set) and it just spares all the necessery
communication. I like to contribute, but you are making it harder than
necessary. By the time a patch finally gets accepted, the patch will
probably not apply cleanly, it will then collide with my own patches
upon merge, etc. Consequently, my enthusiasm for writing patches has
been damped greatly by lack of feedback.
Sometimes you're lucky and a patch is picked up within days, others are
lying around for months without any reaction at all. 2 months look
normal, and 2 months ago, that's when I really started with Django.
I fully understand that
* not all patches should go into the trunk
* time is scarce, especially for the core developers
* we don't pay, and it's really great to have Django
Still, I kind of simply dislike that it takes sooooo long for patches.
It looks as if the current way to handle Django has overloaded the core
developers :-( Simple tickets shouldn't take long. And it would also
help to know that your ticket ain't gonna make it, but in close time,
not months later.
I don't have any experience on how to cope with this efficiently, anyone
else? Like, having a few experienced Django hackers who'd volunteer to
review patches, triage old bugs, together with a set of criteria from
Adrian what would make a valid area of patches and not? Together with an
"experimental" branch where patches just get checked in for easy
testing? I'm open to volunteer for this for as much as I can do.
Michael
Vladimir Pouzanov
> I don't have any experience on how to cope with this efficiently, anyone
> else? Like, having a few experienced Django hackers who'd volunteer to
> review patches, triage old bugs, together with a set of criteria from
> Adrian what would make a valid area of patches and not? Together with an
> "experimental" branch where patches just get checked in for easy
> testing? I'm open to volunteer for this for as much as I can do.
It would be nice to have "eperimental" branch, sometimes I have to
hunt down patches that solve my problems in trac and then apply to my
local copy.
--
Sincerely,
Vladimir "Farcaller" Pouzanov
http://www.hackndev.com
jpel...@gmail.com
svk is your friend. It won't help with getting patches reviewed, but it
does make keeping a tree of local patches almost painless. Here's my
recipe:
# set up mirrors of your repository and django trunk
# and copy django trunk under vendor/ in your repository
svk mirror http://code.djangoproject.com/svn/django/trunk/
//mirror/django
svk mirror http://your.svn.repos/project //mirror/project
svk cp //mirror/django //local/django
svk cp //mirror/project //local/project
svk co //local/project
cd project
svk mkdir vendor
svk cp //local/django vendor/django
svk ci vendor
svk push
# make changes to vendor/django and push them back to your repos
cd vendor
emacs django/django/core/management.py
# later pull an update
svk sync --all
svk pull //local/django
cd django
svk smerge //local/django .
svk ci -m "Merged django trunk to ... "
svk push
# see what you've changed
svk diff -v //mirror/django .
Works like a charm. (I'm hardly an svk expert, though, so there are
probably easier/less verbose recipes for doing the same thing.)
JP
Jay Parlar
If only Darcs were the norm...
Jay P.
Adrian Holovaty
> Just wondering what the procedure is for getting a patch accepted? I
> submitted a patch (http://code.djangoproject.com/ticket/1994) a week
> ago, and have had no feedback on it so far.
>
> Is there some kind of monthly IRC meetup where pending patches are
> discussed? Or is it just when the core developers have free time for
> it?
It's the latter. Me, I generally have one afternoon a week in which I
focus on clearing out patches and tickets, plus weeknights as my
schedule allows. I'm pretty sure Jacob works the same way, although he
has been *super* busy traveling and moving lately. Ideas for improving
this system would be much appreciated!
Adrian
--
Adrian Holovaty
holovaty.com | djangoproject.com
Jay Parlar
> It's the latter. Me, I generally have one afternoon a week in which I
> focus on clearing out patches and tickets, plus weeknights as my
> schedule allows. I'm pretty sure Jacob works the same way, although he
> has been *super* busy traveling and moving lately. Ideas for improving
> this system would be much appreciated!
Would there be interest from you core guys for setting up some kind of
monthly "Django Bug Day", like the python-dev guys do every once in
awhile?
Because of the smaller number of people with commit access, maybe on a
bug day a branch could be setup, and everyone participating could get
access to it. Then for the day we all make changes and commits to that
branch, then meet up a week later to discuss if it should get pushed
to the head?
I'm just pulling ideas out of the air here. I have a lot more
experience in corporate environments than I do working with large
groups on an open source project, so I'm not sure how these issues are
usually handled.
Jay P.
Nicola Larosa (tekNico)
>> probably easier/less verbose recipes for doing the same thing.)
> If only Darcs were the norm...
As far as distributed VCSes go, I'd like to cast my vote for Mercurial.
It's a little Python jewel, as fast as Git, more concise than
Bazaar-ng, and command-compatible with Subversion. It also has a plugin
for interaction with Trac, albeit not integrated yet.
--
Nicola Larosa - http://www.tekNico.net/
we do what we're told
we do what we're told
we do what we're told
told to do
-- Peter Gabriel, We Do What We're Told, So, 1986
Michael Radziej
> Ideas for improving
> this system would be much appreciated!
How about spreading the routine work with tickets?
I think you have some people within the non-committers who have a
sufficient deep knowledge of django andf also sufficient trust to
pre-handle tickets. You could set up trac accounts for them with ticket
handling priviledges.
This could probably be done by non-committers:
- closing obviously invalid request
- verify bugs
- review patches
- bring up questionable patches/features/enhancement on the devel list
- test if patch works or breaks django. Perhaps using the patches some
time for their own stuff.
- set severity/priority according to some policy
- then assign tickets further to one of you
Could you trust a patch when some other developers have positively
reviewed it? This might take a time until your ideas of proper patches
have been "revealed", but then we'd have a more or less peer review
system. And it could get the routine jobs off your hands.
I don't count myself within the experienced django developers, but I'd
offer my help for this in any case.
Also, I'm +1 for the approach with the bug day!
Michael
Jay Parlar
> reviewed it? This might take a time until your ideas of proper patches
> have been "revealed", but then we'd have a more or less peer review
> system. And it could get the routine jobs off your hands.
>
That's an interesting comment, and I think that in the open source
world, Django is (unfortunately) in kind of an unique situation.
*Most* open source projects start out very small, and open source from
the get-go. We were lucky that when Django was open sourced, we
immediately got access to a fantastic working system.
Most other open source projects have to build up to the point of being
a fantastic working system. And in that time, they gain more
knowledgeable developers, and a sense of patch and coding "style". (As
well as levels of trust for long time contributers)
I guess this is just going to be a growing pain for Django, but one
I'm sure we're all willing to work through.
Jay P.
Ilias Lazaridis
I had myself some problems during the creation of a small contribution:
http://case.lazaridis.com/multi/wiki/DjangoSchemaEvolution
This contribution is only available as a patch, which is inconvenient,
and thus other people cannot download the code from an svn to test it.
-
to the topic:
Quickly, I would suggest something like this:
a) schedule a "patch away day" (or more) in order to process some
patches (with low effort for the core developers). This should happen as
soon as possible.
b) create a permanent branch "patch" (or a seperate svn) where commit
access is granted essentially on request.
c) the remaining patches can be applied by the contributers to the
"patch" branch.
the patch committers can collaborate to keep the branch stable
the core committers make the verification / merge of files/subsystems on
request.
Users can decide which patches to retrieve, by just merging from the
patch branch.
-
Other steps are shared responsibility, e.g. "Subsystem Leads" which
decide about if a change goes to the.
.
Kenneth Gonsalves
On 31-May-06, at 7:07 PM, Michael Radziej wrote:
> It looks as if the current way to handle Django has overloaded the
> core
> developers :-( Simple tickets shouldn't take long. And it would also
> help to know that your ticket ain't gonna make it, but in close time,
> not months later.
look at it this way - django 'stable' has not yet been released -
patches make more sense when there is a stable version to be patched.
At present, in a sense, everything is a patch including what the core
devels are doing - and this will continue till stable 1.0 is released
when, I assume, someone will be allotted solely to look after tickets
and patches - just my two paise
--
regards
kg
http://lawgon.livejournal.com
http://avsap.org.in
Luke Plant
> b) create a permanent branch "patch" (or a seperate svn) where commit
> access is granted essentially on request.
>
> c) the remaining patches can be applied by the contributers to the
> "patch" branch.
>
> the patch committers can collaborate to keep the branch stable
>
> the core committers make the verification / merge of files/subsystems on
> request.
>
> Users can decide which patches to retrieve, by just merging from the
> patch branch.
Unfortunately, having this as a svn branch is just not practical -- the
cost of maintaining a branch like this, which tracks trunk but just
goes on indefinitely, is way too large. You would have to use a
different VCS system, with better support for merging and cherry
picking and things like that, and then merging into the subversion
trunk would still be a pain.
I'm not sure this proposal addresses the core problems i.e. 1) getting
some timely feedback about whether a patch is likely to be accepted 2)
actually getting the patches into trunk in a timely manner. I'll try
to post some of my own thoughts soon.
Luke
Luke Plant
As a moderately experienced django hacker, I'm happy to help out with
this stuff (I have already been doing a bit, if in a rather haphazard
way), and I've had a think about it and detailed some ideas below.
Problem
=======
The basic problem, as I see it, is:
People have no idea for a long time whether their patch is likely to be
accepted at all, and if it is, when.
This is important because people need to know whether to use another
work around or continue with their patch. Also the wait-and-see game
can drag on people's patience -- there is a currently a ticket with a
patch that was opened 11 months ago, and it's still waiting for some
kind of review. (Of course don't think for a moment that this is a
criticism of Adrian and Jacob -- it is just what has happened with
limited resources and a lot of demand, but it nonetheless can be
frustrating for contributors, as already noted).
Principles
==========
The solutions to this problem have to be:
- low maintenance
- opt in, to allow experienced django developers to
contribute as much or as little as they want, as their
experience and time allows.
- rigorous -- patches shouldn't wait months with no feedback
while others get applied immediately, there should be some
system to ensure everything gets reviewed.
- actually remove work-load from Adrian and Jacob, including the
time spent looking through bug lists etc.
- integrate with existing stuff -- no extra mailing lists
or websites please.
Suggestions
===========
I think it would be good to aim to add an initial comment to each
patch within N days (where Adrian and Jacob should decide the value of
N!). I would suggest that some of the experienced Django hackers should
be allowed to do this initial review (especially for obvious
WONTFIXes). I'll call these developers 'exp devs', Adrian and Jacob
and probably other commiters 'core devs', and both groups together 'all
devs'.
A number of stock answers could be helpful. They would be
different for new feature patches and bug patches, and might be
something like below:
For bugs:
- [core devs] patch looks OK, I aim to apply within N days/week
- [all devs] please add a test that supports the bug [obviously not
always feasible]
- [all devs] needs more review, I aim to do this in N days/weeks
- [exp devs] patch tests OK, I suggest to the core devs that this patch
(or my tidied up version) is applied immediately.
For new features:
- [core devs] this patch is almost certain to be accepted, I aim to
apply it within N days/weeks.
- [all devs] this patch is likely to be accepted at some point, I aim
to review again in N days/weeks.
- [all devs] this is interesting, but will have to wait for Django 1.1
- [all devs] this patch is unlikely to be accepted, as I don't see
that the need is common enough, or it can be implemented in other ways
(featuritis), unless you can convince me otherwise.
- [all devs] clean up your patch, add some documentation etc and I'll
have another look.
- [exp devs] - patch seems Good and Right, I suggest to the core devs
that this patch (or my tidied up version) is applied immediately.
For each case where there is an aim to review or apply, you could add a
todo-[yourname] to the Trac keywords, so you could search for these (I
guess this is what the 'assigned' field is for, but it just defaults
to a component owner, and most of them are 'Adrian' at the moment, far
more than I suppose he can deal with, so 'assigned = Adrian' is not
very useful for him. I think assigned here really means 'I will look
at this and decide what to do if no-one else does'. We need a way to
distinguish between the 'I will definitely do this' and 'this has just
been assigned to me by Trac' situations).
There is the issue of which tickets to handle -- I am a big culprit
here, as my procedure is totally random -- if I happen to notice
something on django-updates I'll have a look. Maybe we need to be
pedantic about using http://code.djangoproject.com/report/12 and
starting from the bottom.
The problem with that report is that it won't distinguish between
tickets where you are waiting for the ticket owner to get back to you
(fix their patch or whatever), tickets that are awaiting some
attention, and tickets that are on your todo list. I think we need
some more specialised reports:
1) active tickets with patches that are awaiting attention from the
django devs i.e. they are brand new, or the owner has uploaded a new
patch. This report is the one that really needs to be kept small,
to avoid frustrations of contributors.
2) tickets that are on my todo list.
3) for the core devs -- tickets that other devs have suggested can
be applied immediately.
The rest (i.e. where the ticket owner needs to clean up his patch etc)
can be ignored -- you don't need a report, except maybe to close them
after 6 months, on the assumption that the owner doesn't care any
more.
Implementation
==============
To implement this, new patches need to appear on (1), and you need a
way to signal a ticket should be removed from (1). I guess you could
do this with keywords of some kind (e.g. 'needswork') perhaps
requiring the ticket owner to change the keyword back when they're
done (remove 'needswork') (alternatively you could just close WONTFIX
until the patch is improved -- a bit drastic perhaps). A ticket
should also be removed from (1) when a developer volunteers to handle
it, e.g. by adding 'todo-[myname]' or 'todo [myname]' to the
keywords. You would also need a keyword such as 'suggestapply' for
the non-core devs to suggest to the core devs that patches are
applied. This would need to be accompanied by a comment from the
developer, who would have to be someone with a Trac login so that Joe
Django-User doesn't try to be sneaky.
There are details to be worked out here, and there may be better
ways of implementing it, but what do people think?
There would also be the responsibility on everyone who 'adopts' a
ticket to 'unadopt' it if they realise they won't be able to look at
it in the time they've said.
It looks like Trac will be able to support reports like these -- I'm
happy to play around creating reports, but I'll need someone else to
add them to the default list.
Phew, that was long, sorry about that!
Regards,
Luke
--
"Trouble: Luck can't last a lifetime, unless you die young."
(despair.com)
Luke Plant || L.Plant.98 (at) cantab.net || http://lukeplant.me.uk/
Ilias Lazaridis
>
>> b) create a permanent branch "patch" (or a seperate svn) where commit
>> access is granted essentially on request.
>>
>> c) the remaining patches can be applied by the contributers to the
>> "patch" branch.
>>
>> the patch committers can collaborate to keep the branch stable
>>
>> the core committers make the verification / merge of files/subsystems on
>> request.
>>
>> Users can decide which patches to retrieve, by just merging from the
>> patch branch.
>
> Unfortunately, having this as a svn branch is just not practical -- the
> cost of maintaining a branch like this, which tracks trunk but just
> goes on indefinitely, is way too large. You would have to use a
> different VCS system, with better support for merging and cherry
> picking and things like that, and then merging into the subversion
> trunk would still be a pain.
I understand.
> I'm not sure this proposal addresses the core problems i.e. 1) getting
> some timely feedback about whether a patch is likely to be accepted 2)
> actually getting the patches into trunk in a timely manner.
You are right, my proposal addresses to avoid patches at all, as it is
an inconvenient way to contribute.
> I'll try to post some of my own thoughts soon.
>
> Luke
>
>
> >
>
Bill de hÓra
I summarized this as:
Adrian/Jacob pick some people they trust to eval/weed patches. Those
people recommend patches for consideration. That way some of the work is
farmed out, but core technical decisions stay centralised.
is that it?
cheers
Bill
Luke Plant
> Luke,
>
> I summarized this as:
>
> Adrian/Jacob pick some people they trust to eval/weed patches. Those
> people recommend patches for consideration. That way some of the work
> is farmed out, but core technical decisions stay centralised.
>
> is that it?
That's almost it, except you put about a million times better :-)
I would just add that I think there should be a process/order for
looking at patches to ensure they do get looked at, and a specific aim
to to have *some* comment on a patch within a certain period of time.
Luke
--
"Underachievement: The tallest blade of grass is the first to be cut by
the lawnmower." (despair.com)
Malcolm Tredinnick
and only catching up on email in some downtime this weekend. The topic
here is something I've been tossing around in my head a bit, too, since
I'm not sure how best to help without making life harder for Adrian and
Jacob. Anyway, some thoughts/additions to Luke's excellent post...
[This might get a bit long; apologies in advance.]
On Thu, 2006-06-01 at 19:24 +0100, Luke Plant wrote:
[...]
> As a moderately experienced django hacker, I'm happy to help out with
> this stuff (I have already been doing a bit, if in a rather haphazard
> way), and I've had a think about it and detailed some ideas below.
[...]
> Principles
> ==========
> The solutions to this problem have to be:
> - low maintenance
> - opt in, to allow experienced django developers to
> contribute as much or as little as they want, as their
> experience and time allows.
> - rigorous -- patches shouldn't wait months with no feedback
> while others get applied immediately, there should be some
> system to ensure everything gets reviewed.
> - actually remove work-load from Adrian and Jacob, including the
> time spent looking through bug lists etc.
> - integrate with existing stuff -- no extra mailing lists
> or websites please.
Agree with all this 100%. In recent years (not so much lately, but
particularly 2000 - 2004), I spent a lot of time working in the GNOME
project which is a couple of orders of magnitude larger than Django in
terms of lines of code, modules, bugs, even mailing lists (although not
quite as many as 100x more than Dango there, thankfully!) At various
points, the bug lists and patches got out of control and only a
concentrated effort got things manageable again. However, when things
were mostly under control and people understood the process, a little
(or even long) delay was understood.
Not killing the volunteers working on the project is a pretty key idea.
I hit the wall a few times in busy GNOME periods, as did other
developers working much harder than me. In times like that, being able
to hand things along without too much trouble or notice is necessary.
>
> Suggestions
> ===========
>
> I think it would be good to aim to add an initial comment to each
> patch within N days (where Adrian and Jacob should decide the value of
> N!). I would suggest that some of the experienced Django hackers should
> be allowed to do this initial review (especially for obvious
> WONTFIXes). I'll call these developers 'exp devs', Adrian and Jacob
> and probably other commiters 'core devs', and both groups together 'all
> devs'.
>
> A number of stock answers could be helpful. They would be
> different for new feature patches and bug patches, and might be
> something like below:
>
> For bugs:
> - [core devs] patch looks OK, I aim to apply within N days/week
> - [all devs] please add a test that supports the bug [obviously not
> always feasible]
> - [all devs] needs more review, I aim to do this in N days/weeks
> - [exp devs] patch tests OK, I suggest to the core devs that this patch
> (or my tidied up version) is applied immediately.
This last one is where some clarification about expectations from the
Adrian & Jacob show might be useful: in theory, most people committing
to svn lately have pretty good sense and coding style (I'll ignore my
own bits and let others evaluate those). So for pure bug fixes, it would
not be unreasonable for your "exp devs" group to commit the fixes. I
know both you and I, Luke, have dropped in small fixes of late that are
"obviously correct", but I've always felt a bit nervous about doing this
just due to lack of feedback. Does Adrian spit out his cereal everytime
a change like this flies past on the change list? Or are we making
things easier?
It's the more complicated things that are correct on their own, but
require a call as to whether they are in scope that I tend to punt
upstairs.
>
> For new features:
> - [core devs] this patch is almost certain to be accepted, I aim to
> apply it within N days/weeks.
> - [all devs] this patch is likely to be accepted at some point, I aim
> to review again in N days/weeks.
> - [all devs] this is interesting, but will have to wait for Django 1.1
> - [all devs] this patch is unlikely to be accepted, as I don't see
> that the need is common enough, or it can be implemented in other ways
> (featuritis), unless you can convince me otherwise.
This one is hard and I've seen a few that are borderline here lately.
Even ignoring the more extreme "Django is useless without this" claims
on some items, evaluating the potential applicability of a change is
sometimes tricky. We might need to maintain a list of "useful, but not
yet core" patches and then ensure we come back and re-evaluate in a few
months. Some more thoughts about how this could work below.
> - [all devs] clean up your patch, add some documentation etc and I'll
> have another look.
> - [exp devs] - patch seems Good and Right, I suggest to the core devs
> that this patch (or my tidied up version) is applied immediately.
>
>
> For each case where there is an aim to review or apply, you could add a
> todo-[yourname] to the Trac keywords, so you could search for these (I
> guess this is what the 'assigned' field is for, but it just defaults
> to a component owner, and most of them are 'Adrian' at the moment, far
> more than I suppose he can deal with, so 'assigned = Adrian' is not
> very useful for him. I think assigned here really means 'I will look
> at this and decide what to do if no-one else does'. We need a way to
> distinguish between the 'I will definitely do this' and 'this has just
> been assigned to me by Trac' situations).
I would suggest using the "assigned" field for this. And, if necessary,
changing the default component owners to more generic users ("default"
or something) to indicate "unclaimed".
> There is the issue of which tickets to handle -- I am a big culprit
> here, as my procedure is totally random -- if I happen to notice
> something on django-updates I'll have a look. Maybe we need to be
> pedantic about using http://code.djangoproject.com/report/12 and
> starting from the bottom.
>
> The problem with that report is that it won't distinguish between
> tickets where you are waiting for the ticket owner to get back to you
> (fix their patch or whatever), tickets that are awaiting some
> attention, and tickets that are on your todo list. I think we need
> some more specialised reports:
The lack of "needinfo" (in Bugzilla terminology) in Trac is annoying
sometimes, agreed.
Agree with using "needswork" (whatever it gets called) and "shouldapply"
or something. Although, as mentioned above, we could maybe using the
"assigned" field more intelligently so that people whose username is
adrian or jacob only need to look at relevant bugs, rather than
everything.
> There are details to be worked out here, and there may be better
> ways of implementing it, but what do people think?
>
> There would also be the responsibility on everyone who 'adopts' a
> ticket to 'unadopt' it if they realise they won't be able to look at
> it in the time they've said.
Another argument for a more generic default assignee. Unclaiming a bug
means assigning back to the default user.
Extra thoughts
===============
Again, drawing on some other experiences, particularly in the GNOME
community: one thing that I have seen work well is having a weekly (or
periodic... weekly is sometimes not possible) summary of bugs sent to
the developers list. Scaling this for Django's open ticket numbers, it
would not be too onerous to produce a list of some tickets grouped into
a few categories, such as
- needs design work / thinking (these guys should probably be
discussed on the mailing lists or have an edict handed down from
Chicago or Kansas).
- good, but needs patch to be improved.
- common or major problem ("common" is not always the same as
"major") with no proposed solution yet. May have a workaround.
- internationalisation issues
- documentation problems
Some of these overlap. Closer to a release we might also want to
introduce a "showstopper" category.
I am happy to volunteer to do this. In fact, I should probably create
one such list just so that people can see what I'm talking about. It's
not as bad as it sounds and may help keep people informed.
I would also be interested in hearing some thoughts from Adrian and
Jacob: do you two feel overworked? Managing? Have any ideas about how
the rest of us can help without just getting in the way?
I hope threads like this don't have you saying things like "get me his
GPS coordinates and some launch codes. Stat!" :-)
Regards,
Malcolm