Mailing List Archive: Wikipedia: Wikitech

Please don't commit broken code

Index | Next | Previous | View Threaded

brion at wikimedia

Jul 28, 2011, 2:49 PM

Post #1 of 21 (536 views)
Permalink

Please don't commit broken code

Hi all --

Please don't commit broken code to trunk; if you think your code may be
broken please consider asking about it first. This is especially true if
you're committing a fix for a bug that's gone back and forth over the years
about how it should be solved.

And it's even more true if the particular thing you're committing has been
previously committed and reverted several times due to ongoing issues.

Folks that have a history of having commits reverted for problems -- please
start considering this. It's easier to fix your code before it goes in than
after.

-- brion vibber (brion @ wikimedia.org)
_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

kaminski at istori

Jul 28, 2011, 3:09 PM

Post #2 of 21 (533 views)
Permalink

Re: Please don't commit broken code [In reply to]

Great reminder, Brion.

Do any devs have suggestions of what to do instead of checking in broken
code? Or even code you're not sure about? Tips, reminders, places/ways
to ask for help?

Pete

On 7/28/11 14:49 PM, Brion Vibber wrote:

> Please don't commit broken code to trunk; if you think your code may be
> broken please consider asking about it first. This is especially true if
> you're committing a fix for a bug that's gone back and forth over the years
> about how it should be solved.
>
> And it's even more true if the particular thing you're committing has been
> previously committed and reverted several times due to ongoing issues.
>
> Folks that have a history of having commits reverted for problems -- please
> start considering this. It's easier to fix your code before it goes in than
> after.
>
> -- brion vibber (brion @ wikimedia.org)
>

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

brion at wikimedia

Jul 28, 2011, 3:13 PM

Post #3 of 21 (530 views)
Permalink

Re: Please don't commit broken code [In reply to]

On Thu, Jul 28, 2011 at 3:09 PM, Peter Kaminski <kaminski [at] istori> wrote:

> Great reminder, Brion.
>
> Do any devs have suggestions of what to do instead of checking in broken
> code? Or even code you're not sure about? Tips, reminders, places/ways
> to ask for help?
>

Here are a few good places to ask for assistance in:

* this mailing list
* #mediawiki channel on irc.freenode.net
* #wikimedia-dev channel on irc.freenode.net
* Code Review comment area on mediawiki.org
* bugzilla comment area on bugzilla.wikimedia.org

If you're not sure, those are *all* better places to try something out than
committing directly to trunk without talking to anybody or getting any
feedback.

-- brion
_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

z at mzmcbride

Jul 28, 2011, 5:05 PM

Post #4 of 21 (527 views)
Permalink

Re: Please don't commit broken code [In reply to]

Brion Vibber wrote:
> On Thu, Jul 28, 2011 at 3:09 PM, Peter Kaminski <kaminski [at] istori> wrote:
>> Do any devs have suggestions of what to do instead of checking in broken
>> code? Or even code you're not sure about? Tips, reminders, places/ways
>> to ask for help?
>
> Here are a few good places to ask for assistance in:
>
> * this mailing list
> * #mediawiki channel on irc.freenode.net
> * #wikimedia-dev channel on irc.freenode.net
> * Code Review comment area on mediawiki.org
> * bugzilla comment area on bugzilla.wikimedia.org
>
> If you're not sure, those are *all* better places to try something out than
> committing directly to trunk without talking to anybody or getting any
> feedback.

It's a bit difficult to get comments/review in the CodeReview comments area
if you haven't made the commit yet. ;-)

Sometimes the only way people can get their code reviewed is to commit it.
This is an old practice. Not to beat a dead horse, but this is all related
to the same "patches sit unreviewed" issue, etc.

MZMcBride

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

innocentkiller at gmail

Jul 28, 2011, 5:17 PM

Post #5 of 21 (526 views)
Permalink

Re: Please don't commit broken code [In reply to]

On Thu, Jul 28, 2011 at 2:49 PM, Brion Vibber <brion [at] wikimedia> wrote:
> Please don't commit broken code to trunk; if you think your code may be
> broken please consider asking about it first. This is especially true if
> you're committing a fix for a bug that's gone back and forth over the years
> about how it should be solved.
>

+100. If a bug is really old and has the back-and-forth that Brion
describes, there's probably a reason it hasn't been fixed yet. So you
should be extra extra careful when trying to fix it :)

> And it's even more true if the particular thing you're committing has been
> previously committed and reverted several times due to ongoing issues.
>

Yes! If you (or someone) was reverted on a feature, it does not mean
you should fix the one or two minor issues that were noticed and then
push it right back in again. We're not on a deadline for developing
MediaWiki, so I'd encourage people to take the time to do it right. Self-
review your code to make sure it all works the way you think (and claim)
it does. Ask questions if you're unsure. We've got a great community of
really smart developers, pretty much all of whom are more than willing to
answer questions in their area(s) of expertise.

-Chad

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

brion at wikimedia

Jul 28, 2011, 7:01 PM

Post #6 of 21 (527 views)
Permalink

Re: Please don't commit broken code [In reply to]

On Jul 28, 2011 6:01 PM, "MZMcBride" <z [at] mzmcbride> wrote:
>
> Brion Vibber wrote:
> > On Thu, Jul 28, 2011 at 3:09 P
> > If you're not sure, those are *all* better places to try something out
than
> > committing directly to trunk without talking to anybody or getting any
> > feedback.
>
> It's a bit difficult to get comments/review in the CodeReview comments
area
> if you haven't made the commit yet. ;-)

If you made the commit and received feedback about it in CR, continuing
discussion there before making an amended commit -- especially when fixing
something that was reverted -- seems a good fit.

That can save committers and reviewers alike from the pain of a second or
third breakage+revert/rushed fix cycle.

In many cases these are patches for a bug, in which case bugzilla is a good
place to stash an updated patch version to be looked over. (In a DVCS
workflow this might be a branch on a fork rather than a flat patch, but the
principle is the same and we *ought* to be able to do ok with patches -- 20+
years of free/open source devs have reviewed, iterated, and landed or
clearly rejected changes this way, including us!)

> Sometimes the only way people can get their code reviewed is to commit it.
> This is an old practice. Not to beat a dead horse, but this is all related
> to the same "patches sit unreviewed" issue, etc.

I find that reverting and yelling at people for broken commits is *not* a
sustainable practice, even if it's old.

We need to show that we can hold up our end of the bargain: give timely
feedback, keep up with both commits already done and with patches coming in
through other channels.

It's a process, and we're all still feeling our way along.

Part of that is improving reviewer responsiveness to commits after they come
in. Part is improving responsiveness to uncommitted patches.

And part of it is making sure that we send people and code through the
review paths that will best fit them.

-- brion

>
> MZMcBride
>
>
>
> _______________________________________________
> Wikitech-l mailing list
> Wikitech-l [at] lists
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

z at mzmcbride

Jul 28, 2011, 7:19 PM

Post #7 of 21 (526 views)
Permalink

Re: Please don't commit broken code [In reply to]

Brion Vibber wrote:
> In many cases these are patches for a bug, in which case bugzilla is a good
> place to stash an updated patch version to be looked over. (In a DVCS
> workflow this might be a branch on a fork rather than a flat patch, but the
> principle is the same and we *ought* to be able to do ok with patches -- 20+
> years of free/open source devs have reviewed, iterated, and landed or
> clearly rejected changes this way, including us!)

Someone uploads a patch to Bugzilla and doesn't use IRC (and so they can't
nag for review there). What's your recommendation for that scenario? This
mailing list? If so, is there a concern about diffused responsibility?

I haven't seen many requests for patch review on this list in the past, as
far as I remember. A separate list or a direct line to the Bugmeister might
make more sense, though in an ideal world, people wouldn't need to nag at
all...

The reality is that uploading a patch on Bugzilla should be sufficient to
trigger review. At that point, the uploader has put in his or her share of
the work. But very often uploading a patch or even prodding in bug comments
leads to no response at all for months or years or ever. Sometimes you'll
get lucky and someone on the CC list will be able to help you (or find
someone to help you). Or you'll get lucky and someone will notice the bug in
#mediawiki and take an interest. It's a crapshoot, though. And it seems to
me that a lot of the people who commit patches are the same people who don't
use IRC or don't know who to nag if they do.

I'm not trying to be negative here, though it may come off that way. I'm
just trying to understand how people are supposed to be able to help
currently and going forward.

MZMcBride

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

p858snake at gmail

Jul 28, 2011, 7:23 PM

Post #8 of 21 (523 views)
Permalink

Re: Please don't commit broken code [In reply to]

What about for people that want to commit some broken code to get eyes
onto or someone else might want it finish if, how about they commit it
and clearly mark that its broken and its so that people can look/work
at it then immediately revert it? (Which I have seen done a couple of
times in the past)

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

brion at wikimedia

Jul 28, 2011, 7:57 PM

Post #9 of 21 (522 views)
Permalink

Re: Please don't commit broken code [In reply to]

On Jul 28, 2011 7:20 PM, "MZMcBride" <z [at] mzmcbride> wrote:
>
> Brion Vibber wrote:
> > In many cases these are patches for a bug, in which case bugzilla is a
good
> > place to stash an updated patch version to be looked over. (In a DVCS
> > workflow this might be a branch on a fork rather than a flat patch, but
the
> > principle is the same and we *ought* to be able to do ok with patches --
20+
> > years of free/open source devs have reviewed, iterated, and landed or
> > clearly rejected changes this way, including us!)
>
> Someone uploads a patch to Bugzilla and doesn't use IRC (and so they can't
> nag for review there). What's your recommendation for that scenario?

Improve bug & patch review responsiveness so they get our feedback on
bugzilla (and thus in their email).

This
> mailing list? If so, is there a concern about diffused responsibility?
>
> I haven't seen many requests for patch review on this list in the past, as
> far as I remember. A separate list or a direct line to the Bugmeister
might
> make more sense, though in an ideal world, people wouldn't need to nag at
> all...
>
> The reality is that uploading a patch on Bugzilla should be sufficient to
> trigger review.

Exactly. If we haven't reached that point that is our failure as reviewers
and maintainers of a FOSS project, and our job to do better at it.

-- brion

>At that point, the uploader has put in his or her share of
> the work. But very often uploading a patch or even prodding in bug
comments
> leads to no response at all for months or years or ever. Sometimes you'll
> get lucky and someone on the CC list will be able to help you (or find
> someone to help you). Or you'll get lucky and someone will notice the bug
in
> #mediawiki and take an interest. It's a crapshoot, though. And it seems to
> me that a lot of the people who commit patches are the same people who
don't
> use IRC or don't know who to nag if they do.
>
> I'm not trying to be negative here, though it may come off that way. I'm
> just trying to understand how people are supposed to be able to help
> currently and going forward.
>
> MZMcBride
>
>
>
> _______________________________________________
> Wikitech-l mailing list
> Wikitech-l [at] lists
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

mail at tgries

Jul 28, 2011, 11:50 PM

Post #10 of 21 (517 views)
Permalink

Re: Please don't commit broken code [In reply to]

I suggest to add a new bugzilla category "Request for Comments" for
discussions of not-yet-committed-code and similar things, where label
"enhancement" or "normal" would not be suited..

Filed as https://bugzilla.wikimedia.org/show_bug.cgi?id=30111 - where
this proposal itself can now be discussed.

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

hashar+wmf at free

Jul 29, 2011, 2:33 AM

Post #11 of 21 (514 views)
Permalink

Re: Please don't commit broken code [In reply to]

On 29/07/11 02:05, MZMcBride wrote:
<snip>
> Sometimes the only way people can get their code reviewed is to commit it.

In a BRANCH! :-)

Although, you will have to find out someone to review your changes, but
at least it saves you the hassle of being reverted on sight.

If you don't have commit access, use something like github.com which
offers a friendly interface to comment on patch / fork etc ...

--
Ashar Voultoiz

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

hashar+wmf at free

Jul 29, 2011, 2:57 AM

Post #12 of 21 (512 views)
Permalink

Re: Please don't commit broken code [In reply to]

On 29/07/11 04:57, Brion Vibber wrote:
>> Someone uploads a patch to Bugzilla and doesn't use IRC (and so they can't
>> > nag for review there). What's your recommendation for that scenario?
> Improve bug& patch review responsiveness so they get our feedback on
> bugzilla (and thus in their email).

Maybe bugzilla can send wikitech-l a list of bugs with attachments? We
already have a weekly mail listing the number of bugs opened/resolved.
That might attract reviewers.

--
Ashar Voultoiz

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

midom.lists at gmail

Jul 29, 2011, 3:55 AM

Post #13 of 21 (514 views)
Permalink

Re: Please don't commit broken code [In reply to]

Hi!

just wanted to point out that there's open-source software for code-review that is designed for this (code reviews before commit), it supports both SVN and Git:

http://phabricator.org/

Domas
_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

jra at baylink

Jul 29, 2011, 11:07 AM

Post #14 of 21 (506 views)
Permalink

Re: Please don't commit broken code [In reply to]

----- Original Message -----
> From: "Brion Vibber" <brion [at] wikimedia>
> > [MZM:]
> > Someone uploads a patch to Bugzilla and doesn't use IRC (and so they
> > can't nag for review there). What's your recommendation for that scenario?
>
> Improve bug & patch review responsiveness so they get our feedback on
> bugzilla (and thus in their email).

I believe MZ's reply to this will parse as "that's a good goal. But it's a
goal, and what we need is a *process*". And yes, I know Process is in process.

:-)

> > The reality is that uploading a patch on Bugzilla should be
> > sufficient to trigger review.
>
> Exactly. If we haven't reached that point that is our failure as reviewers
> and maintainers of a FOSS project, and our job to do better at it.

I want to take a moment here and applaud Brion for that response. It's
far too uncommon in FOSS projects - I would never hear it from the MythTV
people, I don't think, at least not in so many words.

And there's some reason to that; there is a line you cross in FOSS when
you are no longer scratching just your own itch (and can't these people
just go buy Lanacaine or something? :-)...but when you've crossed that
line is clearly a matter of dispute between devs and users on some
projects; I'm very happy to see that Mediawiki's dev team agree with we
users on which side of that line they're on.

Huzzah!

Cheers,
-- jra
--
Jay R. Ashworth Baylink jra [at] baylink
Designer The Things I Think RFC 2100
Ashworth & Associates http://baylink.pitas.com 2000 Land Rover DII
St Petersburg FL USA http://photo.imageinc.us +1 727 647 1274

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

hashar+wmf at free

Jul 29, 2011, 11:52 AM

Post #15 of 21 (509 views)
Permalink

Re: Please don't commit broken code [In reply to]

On 29/07/11 04:23, K. Peachey wrote:
> What about for people that want to commit some broken code to get eyes
> onto or someone else might want it finish if, how about they commit it
> and clearly mark that its broken and its so that people can look/work
> at it then immediately revert it? (Which I have seen done a couple of
> times in the past)

They can still commit their broken code but NOT IN TRUNK :)

--
Ashar Voultoiz

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

en.wp.st47 at gmail

Jul 29, 2011, 11:59 AM

Post #16 of 21 (509 views)
Permalink

Re: Please don't commit broken code [In reply to]

On Fri, Jul 29, 2011 at 2:52 PM, Ashar Voultoiz <hashar+wmf [at] free> wrote:
> They can still commit their broken code but NOT IN TRUNK :)

Many developers (me) don't understand what there is other than trunk.
I'm aware of tags and branches, and vaguely recall someone being
yelled at for changing one of those, and was confused as to why there
would be a whole part of the repo that couldn't be changed, and that
was what old revisions were for. But is there also another special
place to commit testing code?

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

innocentkiller at gmail

Jul 29, 2011, 12:05 PM

Post #17 of 21 (506 views)
Permalink

Re: Please don't commit broken code [In reply to]

You're correct that Mediawiki tags and release branches (REL1_xx) shouldn't
usually be touched. However you are more than welcome to create your own
development branches.

-Chad
On Jul 29, 2011 12:00 PM, "Dan Collins" <en.wp.st47 [at] gmail> wrote:
> On Fri, Jul 29, 2011 at 2:52 PM, Ashar Voultoiz <hashar+wmf [at] free>
wrote:
>> They can still commit their broken code but NOT IN TRUNK :)
>
> Many developers (me) don't understand what there is other than trunk.
> I'm aware of tags and branches, and vaguely recall someone being
> yelled at for changing one of those, and was confused as to why there
> would be a whole part of the repo that couldn't be changed, and that
> was what old revisions were for. But is there also another special
> place to commit testing code?
>
> _______________________________________________
> Wikitech-l mailing list
> Wikitech-l [at] lists
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

jra at baylink

Jul 29, 2011, 12:30 PM

Post #18 of 21 (502 views)
Permalink

Re: Please don't commit broken code [In reply to]

----- Original Message -----
> From: "Dan Collins" <en.wp.st47 [at] gmail>

> On Fri, Jul 29, 2011 at 2:52 PM, Ashar Voultoiz <hashar+wmf [at] free>
> wrote:
> > They can still commit their broken code but NOT IN TRUNK :)
>
> Many developers (me) don't understand what there is other than trunk.
> I'm aware of tags and branches, and vaguely recall someone being
> yelled at for changing one of those, and was confused as to why there
> would be a whole part of the repo that couldn't be changed, and that
> was what old revisions were for. But is there also another special
> place to commit testing code?

More pointedly, is there a really *good* "Software Development Version
Management HOWTO" anywhere at all? I've looked at the git book, and it
just knocked me on my ass from the first chapter. I *almost* understand
BZR... but I haven't seen a good 30,000ft explanation of any of them,
written for people who are good programmers, but have never worked with
one before.

Cheers,
-- jra
--
Jay R. Ashworth Baylink jra [at] baylink
Designer The Things I Think RFC 2100
Ashworth & Associates http://baylink.pitas.com 2000 Land Rover DII
St Petersburg FL USA http://photo.imageinc.us +1 727 647 1274

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

hashar+wmf at free

Jul 29, 2011, 2:52 PM

Post #19 of 21 (499 views)
Permalink

Re: Please don't commit broken code [In reply to]

On 29/07/11 20:59, Dan Collins wrote:
> On Fri, Jul 29, 2011 at 2:52 PM, Ashar Voultoiz<hashar+wmf [at] free> wrote:
>> They can still commit their broken code but NOT IN TRUNK :)
>
> Many developers (me) don't understand what there is other than trunk.
> I'm aware of tags and branches, and vaguely recall someone being
> yelled at for changing one of those, and was confused as to why there
> would be a whole part of the repo that couldn't be changed, and that
> was what old revisions were for. But is there also another special
> place to commit testing code?

A branch is just a copy of a directory with all history. You can copy
trunk almost where ever you want under the /branches/ directory. There
is a rough guide on mw.org:

http://www.mediawiki.org/wiki/Subversion/branching_guide

I myself put my branches under /branches/hashar/ As an example have a
look at:

http://svn.wikimedia.org/viewvc/mediawiki/branches/hashar/

You will find 4 sub directories there which got forked from trunk at
various point in the time:
- node-qunit
- old_stuff
- prettyURL
- syslog

--
Ashar Voultoiz

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

mhershberger at wikimedia

Jul 29, 2011, 3:24 PM

Post #20 of 21 (499 views)
Permalink

Re: Please don't commit broken code [In reply to]

Peter Kaminski <kaminski [at] istori> writes:

> Great reminder, Brion.
>
> Do any devs have suggestions of what to do instead of checking in broken
> code? Or even code you're not sure about? Tips, reminders, places/ways
> to ask for help?

Last year, I was a new committer and was getting some of the same
complaints about my code over and over again. I wrote up the Pre-commit
checklist (http://www.mediawiki.org/wiki/Manual:Pre-commit_checklist) to
remind myself of the gotcha's that got me and how to avoid them.

And yesterday, after a coding hiatus, I made a few commits, forgot about
my pre-commit checklist, and made some of those same bone-headed
mistakes.

For people like me, such a list is invaluable. It is like forcing
spell-check before sending an email.

Mark.

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Platonides at gmail

Jul 30, 2011, 8:37 AM

Post #21 of 21 (482 views)
Permalink

Re: Please don't commit broken code [In reply to]

Ashar Voultoiz wrote:
> On 29/07/11 02:05, MZMcBride wrote:
> <snip>
>> Sometimes the only way people can get their code reviewed is to commit it.
>
> In a BRANCH! :-)
>
> Although, you will have to find out someone to review your changes, but
> at least it saves you the hassle of being reverted on sight.
>
> If you don't have commit access, use something like github.com which
> offers a friendly interface to comment on patch / fork etc ...

I don't mind if people provides sample in github or pastebin, but don't
expect me to comment and follow up there (I might do, but don't take
that for granted).
Those sites are designed to be the central place, but they are not for
our community, so anything there will be less viewed than eg. a CR comment.

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads