Discussion:
[MERGE][#144421][#144300] Use write lock in log and status with '-r branch:URL'
Lukáš Lalinský
2007-12-29 17:53:47 UTC
Permalink
This patch replaces lock_read with lock_write in log and status if the
'branch:' revision specifier is used. I realize it would be probably
better to just get rid of the implicit fetching, but I think fixing the
lock is good enough as an interim solution.

Lukas
Bundle Buggy
2007-12-29 17:56:17 UTC
Permalink
Bundle Buggy has detected this merge request.

For details, see:
http://bundlebuggy.aaronbentley.com/request/%3C1198950827.6593.3.camel%40nemo%3E
Ian Clatworthy
2008-01-03 07:41:49 UTC
Permalink
Post by Lukáš Lalinský
This patch replaces lock_read with lock_write in log and status if the
'branch:' revision specifier is used. I realize it would be probably
better to just get rid of the implicit fetching, but I think fixing the
lock is good enough as an interim solution.
bb:tweak

A NEWS entry is needed. I also think the new blackbox tests need names
that reflect what they are testing (branch specifiers for those
operations) instead of the particular bug being fixed (*_write_lock).

Ian C.
John Arbash Meinel
2008-02-20 20:20:10 UTC
Permalink
John Arbash Meinel has voted tweak.
Status is now: Conditionally approved
Comment:
I'll tweak this and merge it.

For details, see:
http://bundlebuggy.aaronbentley.com/request/%3C1198950827.6593.3.camel%40nemo%3E
John Arbash Meinel
2008-02-20 21:32:54 UTC
Permalink
John Arbash Meinel has voted comment.
Status is now: Semi-approved
Comment:
I changed my mind....
I'm hesitant about this patch because I see it as a hack on top of a
hack. The first being that "branch:" pulls revisions into the local
repository, and the second making a readonly operation like status/diff
become a read-write operation as a side-effect of what revision you are
comparing against.

I think the correct fix is to have revision specs which reference a
second branch have the ability to add a revision source to the rest of
the query functions. That way we don't have to do any writing during a
read-only operation.

For details, see:
http://bundlebuggy.aaronbentley.com/request/%3C1198950827.6593.3.camel%40nemo%3E
Aaron Bentley
2008-03-25 14:24:42 UTC
Permalink
Aaron Bentley has voted abstain.
Status is now: Semi-approved
Comment:
I find this too hacky to merge.

For details, see:
http://bundlebuggy.aaronbentley.com/request/%3C1198950827.6593.3.camel%40nemo%3E
Alexander Belchenko
2008-03-25 19:46:13 UTC
Permalink
Post by Lukáš Lalinský
This patch replaces lock_read with lock_write in log and status if the
'branch:' revision specifier is used. I realize it would be probably
better to just get rid of the implicit fetching, but I think fixing the
lock is good enough as an interim solution.
Today I discover that bzr diff -rbranch:XXX works, but bzr status -rbranch:XXX
is not. I found workaround to use `bzr diff -rbranch:XXX | grep '^==='` instead
of status.

Why status does not work? What makes status so special that it can't work? Why diff can?
Could anybody shed some light on this? Looking at this patch I don't understand
why for write lock needed.
Lukáš Lalinský
2008-03-25 20:14:29 UTC
Permalink
Post by Alexander Belchenko
Post by Lukáš Lalinský
This patch replaces lock_read with lock_write in log and status if the
'branch:' revision specifier is used. I realize it would be probably
better to just get rid of the implicit fetching, but I think fixing the
lock is good enough as an interim solution.
Today I discover that bzr diff -rbranch:XXX works, but bzr status -rbranch:XXX
is not. I found workaround to use `bzr diff -rbranch:XXX | grep '^==='` instead
of status.
Why status does not work? What makes status so special that it can't work? Why diff can?
Could anybody shed some light on this? Looking at this patch I don't understand
why for write lock needed.
"bzr diff" works because it doesn't have a global lock_read around it
(which makes is slower and there were at least two patches for it, but
they break the test suite because of this bug), which means it can do
lock_write anytime it can. But in "bzr status" the code is called when
the repository is already locked, and it can't upgrade a read lock to a
write lock.

The reason why it fails at all is that "branch:" loads data from the
branch to the local repository and then loads the head revision, but to
put anything to the repository it needs a write lock.

Lukas
Aaron Bentley
2008-04-02 03:19:43 UTC
Permalink
Aaron Bentley has voted reject.
Status is now: Vetoed
Comment:
No one seems willing to merge this kind of fix.

For details, see:
http://bundlebuggy.aaronbentley.com/request/%3C1198950827.6593.3.camel%40nemo%3E
John Arbash Meinel
2008-08-28 14:51:03 UTC
Permalink
Robert Collins wrote:
...
This patch looks to be in a write direction.
right? :)

...
I realise that revision specs that pull is not particularly pretty, but
we made that choice some time ago as a decent approach for some
problems. Remember that back then we didn't _have_ read locks, only
write locks. Merging this patch won't make status more complex or error
prone, and we can always improve things later.
That said, teaching status to not pull data should be very straight
- add a method to revisionspec called as_revision_tree
- use that method in status.py rather than as_revision_id.
(so that
rev_id = revision[0].as_revision_id(wt.branch)
old = wt.branch.repository.revision_tree(rev_id)
raise errors.BzrCommandError(str(e))
becomes
old = revision[0].as_revision_tree()
raise errors.BzrCommandError(str(e))
and similarly immediately after that.
This is probably a couple of hours work, and I'm happy to offer more
advice on what tests are needed etc.
-Rob
The pain problem with "as_revision_tree()" is that we would really like to
have the branch/repository locked *before* we ask for something as involved as
a RevisionTree. This is because RT requires an inventory, etc. So we have to
read a bunch of indexes and extract texts from the repository before we are
able to return a value.

I suppose if "as_revision_tree()" tries to use the local branch/repository
first (which should be locked anyway), and then falls back to using the remote
one. Then you get the functionality, with only a small amount of "waste".

I do agree that it makes the api seem clearer, as it helps push the knowledge
about a different branch/repository down into a lower level.

That said, stuff like Graph.find_unique_ancestry() will fail if you don't give
it the remote repository. So "bzr missing" needs to know about 2 repositories.
I *think* bzr diff only needs to know a snapshot (at least at the moment).

One other possibility... have "-r branch:" only fetch if the revision is *not*
in the local repository. For people with shared repos, or who have merged the
branch before, it will succeed. It is unfortunate that it would still fail
sometimes. And perhaps failing sometimes is worse than failing always.

I'm hesitant to say that we need to update all the code to handle the
possibility of 2 repositories. But that *is* reality.

John
=:->
Robert Collins
2008-08-28 09:29:38 UTC
Permalink
Post by Lukáš Lalinský
This patch replaces lock_read with lock_write in log and status if the
'branch:' revision specifier is used. I realize it would be probably
better to just get rid of the implicit fetching, but I think fixing the
lock is good enough as an interim solution.
This patch looks to be in a write direction.

I would bb:approve, though that would only get it part-approved, if:
- there was a helper function that diff and other -r using commands
could call that will return a read-locked, or write-locked if needed
- the new method on RevisionSpec was unit tested

I realise that revision specs that pull is not particularly pretty, but
we made that choice some time ago as a decent approach for some
problems. Remember that back then we didn't _have_ read locks, only
write locks. Merging this patch won't make status more complex or error
prone, and we can always improve things later.

That said, teaching status to not pull data should be very straight
forward:
- add a method to revisionspec called as_revision_tree
- use that method in status.py rather than as_revision_id.

(so that
try:
rev_id = revision[0].as_revision_id(wt.branch)
old = wt.branch.repository.revision_tree(rev_id)
except errors.NoSuchRevision, e:
raise errors.BzrCommandError(str(e))
becomes
try:
old = revision[0].as_revision_tree()
except errors.NoSuchRevision, e:
raise errors.BzrCommandError(str(e))
and similarly immediately after that.

This is probably a couple of hours work, and I'm happy to offer more
advice on what tests are needed etc.

-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.
Loading...