attn folk doing reviews.

[John A Meinel]

Martin Pool wrote:
> On 23 Jan 2006, Robert Collins <robertc at robertcollins.net> wrote:
>> Two things... what do you think of some review guidelines as an aid to
>> memory on the wiki ? I think the peer review thing is working well, but
>> that will make it clear to folk submitting code what we are looking for.
> 
> There are already some in the HACKING file.  More guidelines would be
> good.  Either place is OK, but preferably it'll not be spread across
> both.  I'd slightly prefer to add more detail there and post an excerpt
> with pointer to the wiki.
> 
>> Secondly, I've noticed a couple of times though is that new methods are
>> not getting @needs_read_lock or @needs_write_lock decorators. Generally
>> speaking I think any method in the WorkingTree, Branch and Repository
>> classes will need such a decorator *unless* its 
>>  * a trivial wrapper. Note that a wrapper/convenience function that
>> mutates anything will almost certainly need needs_write_lock.
>>  * unrelated to the persistent storage of the object.
>>
>> I.e. most recently the fileid_involved methods - they read the inventory
>> weave, and they dont take out a transaction themselves, and neither does
>> self._get_inventory_weave().
> 
> Perhaps needing these decorators on almost all methods suggests an
> underlying problem?  Perhaps we should reconsider having the lock held
> for the lifetime of the object?
> 

The problem with 'lifetime of the object' is that some functions need
write locks and some functions need read locks. And in the past, we
decided it wasn't safe to upgrade a read lock to a write lock, because
of potential deadlock issues.

And you could do grabs the lock when first needed, and then holds it.
But then you couldn't do:

b = Branch.open()
history = b.revision_history() # Read lock
#check it is out of date
b.put_revision() # Ooops write lock, failure

I also don't think we want to always take out a write lock. Since having
one branch pull from a location shouldn't prevent another branch from
pulling, and probably shouldn't prevent another branch from pushing (as
long as they maintain our invariants).

Now, we are combining two things: needing a read/write lock, and caching
transactions.

According to Robert's musings, we don't need a real read-lock as long as
we don't violate the invariants. (Which we need because neither sftp nor
http have read-locks).
So maybe we should only have '@needs_write_lock', and then we should
*always* have a cache. I'm not sure how we would determine when it would
need to be invalidated. But maybe we should really been doing an
explicit caching request at the higher levels.

So rather than Branch.lock() creating the transaction/session object. We
 have a 'Branch.start_session()' which something like 'compare_trees'
would call, rather than doing it behind the scenes.

John
=:->


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 249 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060122/e6ee4391/attachment.pgp