Oh no no no... I wasn't suggesting that you change anything other than
to stop saying "don't bother reviewing this". But that was my own lack
of awareness of the intended size of your patch (as Julian rightfully
and insightfully pointed out).
So: keep going! :-)
Cheers,
-g
On Thu, Oct 23, 2008 at 3:59 AM, Neels J. Hofmeyr <neels_at_elego.de> wrote:
> What Julian said, and (until told otherwise), I assumed I would need to
> write a new, complete log message for all the changes upon `merge
> --reintegrate'. Wouldn't that be a good solution for the reviewing problem?
>
> Then again, when following that scheme, `blame' would show a revision number
> that doesn't lead to the final (merging) log message for that line, but only
> to the (committing) one that was deemed intermediate. Might get wormy.
>
> So, Greg, should I continue like this or would you like me to change my
> branching behaviour?
>
> ~Neels
>
> Julian Foad wrote:
>> 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
>>
>
> --
> Neels Hofmeyr -- elego Software Solutions GmbH
> Gustav-Meyer-Allee 25 / Gebäude 12, 13355 Berlin, Germany
> phone: +49 30 23458696 mobile: +49 177 2345869 fax: +49 30 23458695
> http://www.elegosoft.com | Geschäftsführer: Olaf Wagner | Sitz: Berlin
> Handelsreg: Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
>
>
---------------------------------------------------------------------
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 13:11:44 CEST