Merge lp:~al-maisan/bzr-builddeb/locked-repo-updates into lp:~bzr-builddeb-hackers/bzr-builddeb/trunk-old

Proposed by Muharem Hrnjadovic
Status: Merged
Merge reported by: James Westby
Merged at revision: not available
Proposed branch: lp:~al-maisan/bzr-builddeb/locked-repo-updates
Merge into: lp:~bzr-builddeb-hackers/bzr-builddeb/trunk-old
Diff against target: None lines
To merge this branch: bzr merge lp:~al-maisan/bzr-builddeb/locked-repo-updates
Reviewer Review Type Date Requested Status
James Westby Approve
Review via email: mp+10656@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Muharem Hrnjadovic (al-maisan) wrote :

A test of the "merge-package" command caused bzr exceptions to be thrown. In the process of analyzing the problem it became apparent that repository updates during a held lock are not visible.

This branch fixes the "merge-package" code as well as another piece of code that worked around the problem by unlocking/re-locking the tree in question.

For more information please see also bug #418491.

All tests in the suite ("bzr selftest -s bp.builddeb") pass w/o errors.

Please let me know what you think.

368. By Muharem Hrnjadovic

Fixed import for exception.

369. By Muharem Hrnjadovic

Removed superfluous import.

Revision history for this message
James Westby (james-w) wrote :

20 - # Unlock the tree and lock it again
21 - self.tree.unlock()
22 - self.tree.lock_write()
23 + # Make sure we see any revisions added by the upstream branch
24 + # since self.tree was locked.
25 + self.branch.repository.refresh_data()

There's no need to keep the assertions with this change, as we
don't unlock the state can't change.

Also, remove the locked check, it's fine to do the refresh
unconditionally.

Are you going to wrap the test for the other issue in to this
branch now?

Thanks,

James

review: Needs Fixing
370. By Muharem Hrnjadovic

Removed superfluous checks (James' review comments).

Revision history for this message
Muharem Hrnjadovic (al-maisan) wrote :

James Westby wrote:
> Review: Needs Fixing
> 20 - # Unlock the tree and lock it again
> 21 - self.tree.unlock()
> 22 - self.tree.lock_write()
> 23 + # Make sure we see any revisions added by the upstream branch
> 24 + # since self.tree was locked.
> 25 + self.branch.repository.refresh_data()
>
> There's no need to keep the assertions with this change, as we
> don't unlock the state can't change.

Good point, what have I been thinking? Done.

> Also, remove the locked check, it's fine to do the refresh
> unconditionally.

Done.

> Are you going to wrap the test for the other issue in to this
> branch now?

It's best if I prepare a fresh branch with the tests we discussed on IRC.

> Thanks,

Thank you! Please see the enclosed incremental diff.

Best regards

--
Muharem Hrnjadovic <email address hidden>
Public key id : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F 5602 219F 6B60 B2BB FCFC

1=== modified file 'import_dsc.py'
2--- import_dsc.py 2009-08-25 13:36:01 +0000
3+++ import_dsc.py 2009-08-25 21:13:31 +0000
4@@ -1085,16 +1085,9 @@
5 and that there are no working tree changes, to prevent unexpected
6 things happening if say a commit was done in this time.
7 """
8- if self.tree.is_locked():
9- last_revision = self.branch.last_revision()
10- # Make sure we see any revisions added by the upstream branch
11- # since self.tree was locked.
12- self.branch.repository.refresh_data()
13- assert self.branch.last_revision() == last_revision, \
14- "Branch committed to while refreshing it. Not proceeding."
15- assert not self.tree.changes_from(
16- self.tree.basis_tree()).has_changed(), \
17- "Treee altered while refreshing it. Not proceeding."
18+ # Make sure we see any revisions added by the upstream branch
19+ # since self.tree was locked.
20+ self.branch.repository.refresh_data()
21 self.branch.fetch(self.upstream_branch, last_revision=revid)
22 self.upstream_branch.tags.merge_to(self.branch.tags)
23
Revision history for this message
James Westby (james-w) wrote :

Merged with a fix to the docstring so that it doesn't claim
it is doing something that it isn't.

Thanks,

James

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'import_dsc.py'
2--- import_dsc.py 2009-08-19 12:22:29 +0000
3+++ import_dsc.py 2009-08-25 13:36:01 +0000
4@@ -1081,20 +1081,15 @@
5 def _fetch_upstream_to_branch(self, revid):
6 """Fetch the revision from the upstream branch in to the pacakging one.
7
8- This will unlock self.tree, then re-lock it and fetch. This is
9- necessary as if the two branches share a repository the branch
10- won't see any revisions added by the upstream branch since self.tree
11- was locked.
12-
13- It will check that the last revision is the same before and after,
14+ This will check that the last revision is the same before and after,
15 and that there are no working tree changes, to prevent unexpected
16 things happening if say a commit was done in this time.
17 """
18 if self.tree.is_locked():
19 last_revision = self.branch.last_revision()
20- # Unlock the tree and lock it again
21- self.tree.unlock()
22- self.tree.lock_write()
23+ # Make sure we see any revisions added by the upstream branch
24+ # since self.tree was locked.
25+ self.branch.repository.refresh_data()
26 assert self.branch.last_revision() == last_revision, \
27 "Branch committed to while refreshing it. Not proceeding."
28 assert not self.tree.changes_from(
29
30=== modified file 'merge_package.py'
31--- merge_package.py 2009-08-25 10:27:07 +0000
32+++ merge_package.py 2009-08-25 13:00:30 +0000
33@@ -147,6 +147,9 @@
34 tmp_target_utree.set_parent_ids((ut_revid, us_revid))
35 tmp_target_utree.commit(
36 'Prepared upstream tree for merging into target branch.')
37+ # Repository updates during a held lock are not visible,
38+ # hence the call to refresh the data in the /target/ repo.
39+ tree.branch.repository.refresh_data()
40
41 # Merge shared upstream parent into the target merge branch. This
42 # creates revison L in the digram above.

Subscribers

People subscribed via source and target branches