On Thu, 2008-10-23 at 01:59 -0700, Greg Stein wrote:
> On Wed, Oct 22, 2008 at 6:04 PM, <neels_at_tigris.org> wrote:
> > Author: neels
> > Date: Wed Oct 22 18:04:30 2008
> > New Revision: 33855
> >
> > Log:
> > On branch: tree-conflicts-notify
> >
> > *** This is just a start. Don't waste too much time reviewing it! ***
> > *** Some parts of this may be misguided and go away again. ***
>
> Neels,
>
> EVERY commit should be fully-reviewed, and fully reviewable.
Not so, not on a "light weight" branch. We wrote in our "hacking.html":
[[[
Use lightweight branches
------------------------
If you're working on a feature or bugfix in stages involving multiple
commits, and some of the intermediate stages aren't stable enough to go
on trunk, then create a temporary branch in /branches. There's no need
to ask — just do it. It's fine to try out experimental ideas in a
temporary branch, too. And all the preceding applies to partial as well
as full committers.
If you're just using the branch to "checkpoint" your code, and don't
feel it's ready for review, please put some sort of notice at the top of
the log message, such as:
*** checkpoint commit -- please don't waste your time reviewing it ***
[...]
]]]
So what Neels is doing is fine.
Now, you may be right in terms of defining a better policy. That's
certainly one good policy for working on a branch, especially on a
feature development that is shared and/or long-lived. However, there are
other uses for a branch.
I think Neels intended this branch to be for developing a small change,
no bigger than a patch that would otherwise be sent to the mailing list
(and it still can be). It's like a developer-private branch, an
alternative to saving "svn diff" patches every hour or local commits
into SVK. The fact that it is publically visible is a bonus.
Some of us "think" by writing a chunk of code, looking at how it turns
out, trying to work with it, and then re-writing it as necessary. If we
forbid developers from working in that way, they are likely to go back
to saving local patch files or check in to SVK, with less visibility and
less incentive to think about the reviewability and wholeness of each
such stage. It would probably not encourage them to write more complete
intermediate steps when the aim is itself just a small change.
Does that matter? Maybe that would be better because we would all know
exactly which commits are supposed to be reviewed (all of them) which
would avoid the tendency for us to start ignoring all commits on
branches. If that's the main problem (and it certainly could become a
problem - we already review branches far less than we should), then we
should at least consider addressing it in a different way, such as using
a "/branches/scratch/" name space.
> Why? The answer is simple. If a review is not performed, then *when*
> should be it be done? We all agree reviews should be done, so then
> when, if not when it first gets committed?
>
> "Branch merge" is not the right answer. At that point, the overall
> change will be too large to properly review.
That's the crux here: that's not necessarily true. Yes, it's easy to get
into that situation, and it often happens, but must we assume it will
happen?
I guess you're saying that we can't trust most of ourselves most of the
time to stick to a reviewable-sized change, and then make sure to get it
reviewed. And historically that's been so. So maybe overall it would be
better not to work in this way.
Is it the question of being easily able to know what to review that's
most important here?
- Julian
> This is the second or third time, I've seen your log message basically
> say "don't bother to review this". I think that is an incorrect
> procedure/direction, so I wanted to raise this point about reviewable
> code. Also note this means you should consider your commits to a
> branch to be just as focused, complete, and working as any commit you
> would make to /trunk.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-10-23 12:24:05 CEST