bzr Upload plugin

Merge lp://staging/~garyvdm/bzr-upload/check_not_diverged into lp://staging/bzr-upload

  1. check_not_diverged
  2. Merge into trunk
Proposed by Gary van der Merwe
Status: Merged
Merged at revision: not available
Proposed branch: lp://staging/~garyvdm/bzr-upload/check_not_diverged
Merge into: lp://staging/bzr-upload
Diff against target: 297 lines
3 files modified
NEWS (+5/-0)
__init__.py (+101/-55)
tests/test_upload.py (+35/-0)
To merge this branch: bzr merge lp://staging/~garyvdm/bzr-upload/check_not_diverged
Reviewer Review Type Date Requested Status
Vincent Ladeuil Approve
Review via email: mp+12776@code.staging.launchpad.net

This proposal supersedes a proposal from 2009-09-28.

To post a comment you must log in.
Revision history for this message
Gary van der Merwe (garyvdm) wrote : Posted in a previous version of this proposal

This makes bzr-upload check that you are not upload diverged branches. This can be ignored by running bzr upload --overwrite.

Some notes:
* We need to better explain how to resolve the error. This may mean the user does a merge, or runs with --overwrite. There are some complexities with this though. How does the user figure out which branch he needs to merge with. And if he does want to --overwrite, it is quite likely that they don't have the previously uploaded revision in their repo. We should probably detect this, and let them know how the can get the revision into their repo, with out changing their branch tip (bzr pull, ignore the diverged error.) So in all, explaining to the user how to resolve this is quite complex.

* By design, this change does not apply to the auto hook. I has good reasons, but I'll explain tommorow, cause I'm quite tired now.

Revision history for this message
Vincent Ladeuil (vila) wrote : Posted in a previous version of this proposal

At first glance, there are a couple of issues:

- you cache the uploaded revid but I don't think we have code paths
  complex enough to need it, so I'd prefer to not do it,

- why do yo suddenly need to lock_read() the branch ? It makes the
  patch hard to read and does not seem related.

- assertUploadRevid in the tests would be better named assertRevidUploaded
  or assertIsUploadedRevid

- both of your tests starts with the same call, that's what setUps are for :)

review: Needs Information
Revision history for this message
Gary van der Merwe (garyvdm) wrote :

On Tue, Sep 29, 2009 at 12:50 AM, Gary van der Merwe <email address hidden> wrote:
> * By design, this change does not apply to the auto hook. I has good reasons, but I'll explain tommorow, cause I'm quite tired now.

Ok - My reasons for this:
* For some work flows, it won't be necessary, e.g. if I have a branch deployment branch (with a auto upload hook), that I push to, then that branch serves as the check that we are not uploading a diverged revision. How ever some people may have a auto upload hook on their working branch, that upload when they commit. For this case, we should be checking, but:
* It's really difficult to add a way for the user to let us know that it should overwrite when the hook is running. May be we should do the check, which would require that they run bzr upload --overwrite manually if the revisions has diverged, but this would be really irritating for users of the first workflow that I mentioned (that wish to do a --overwrite.)

On Tue, Sep 29, 2009 at 9:55 AM, Vincent Ladeuil <email address hidden> wrote:
> - you cache the uploaded revid but I don't think we have code paths
> complex enough to need it, so I'd prefer to not do it,

I have added a call to get_uploaded_revid in cmd_upload.run, and it was previously been called in BzrUploader.upload_tree. I could pass the revid retrived in cmd_upload.run to BzrUploader.upload_tree, but I felt this makes the interface more messy, and that making get_uploaded_revid cache it's value was much cleaner.

> - why do yo suddenly need to lock_read() the branch ? It makes the
> patch hard to read and does not seem related.

I needed to add lock for graph = branch.repository.get_graph(), graph.is_ancestor(). When I looked I realised that there are a number of other calls that are taking out their own locks automaticly, e.g. wt.changes_from(wt.basis_tree()), branch.repository.revision_tree(rev_id), so I think that it is better that there is one lock for all of this. Am I wrong?

> - assertUploadRevid in the tests would be better named assertRevidUploaded
> or assertIsUploadedRevid

Fixed.

> - both of your tests starts with the same call, that's what setUps are for :)

Fixed.

lp://staging/~garyvdm/bzr-upload/check_not_diverged updated
70. By Gary van der Merwe

Fix formating in DivergedError.

Revision history for this message
Vincent Ladeuil (vila) wrote :

I'll merge.

There are some points I'll like to discuss but I'll send another mail for that.

review: Approve
Revision history for this message
Vincent Ladeuil (vila) wrote :

There are parts I want to evolve in your patch but nothing
blocking its landing, so I've merged it as is.

That being said, your patch raises many interesting issues that
deserve to be dug, I comment on them below.

>>>>> "Gary" == Gary van der Merwe <email address hidden> writes:

    Gary> On Tue, Sep 29, 2009 at 12:50 AM, Gary van der Merwe
    Gary> <email address hidden> wrote:
    >> * By design, this change does not apply to the auto hook. I has
    Gary> good reasons, but I'll explain tommorow, cause I'm quite tired
    Gary> now.

    Gary> Ok - My reasons for this:

Right, I think we can summarize that the --overwrite option is a
way to correct some rough edges in some workflows.

    Gary> * For some work flows, it won't be necessary, e.g. if I have a
    Gary> branch deployment branch (with a auto upload hook), that I push
    Gary> to, then that branch serves as the check that we are not
    Gary> uploading a diverged revision. How ever some people may have a
    Gary> auto upload hook on their working branch, that upload when they
    Gary> commit.

Yes, that's the expected workflow for a single developer.

    Gary> For this case, we should be checking, but:
    Gary> * It's really difficult to add a way for the user to let us know
    Gary> that it should overwrite when the hook is running.

I don't think we need that for a start.

So the hook should fail in that case, whether it makes the commit
abort or just emit some sort of warning can be discussed or even
controlled by an option, but it should be defined nevertheless.

I'm fine with a default behavior that simply refuse to upload
yet let the commit succeed.

That scenario is not the simplest one so I'm fine with having to
set an option somewhere to further control the hook. Config files
are fine for that.

If you look at the recent history of the plugin (with say
qlog <wink>) you'll see that I implemented such an option
(upload_auto_quiet). So, no big deal, no urgency.

    Gary> May be we should do the check, which would require that
    Gary> they run bzr upload --overwrite manually if the
    Gary> revisions has diverged, but this would be really
    Gary> irritating for users of the first workflow that I
    Gary> mentioned (that wish to do a --overwrite.)

I don't think so, --auto is to avoid doubling each commit with a
bzr upload, nothing against issuing one once in a while if
needed.

<snip/>

    >> - why do yo suddenly need to lock_read() the branch ? It makes
    Gary> the
    >> patch hard to read and does not seem related.

    Gary> I needed to add lock for graph = branch.repository.get_graph(),
    Gary> graph.is_ancestor().

Ha ! Lost in the too big diff !

    Gary> When I looked I realised that there are a number of
    Gary> other calls that are taking out their own locks
    Gary> automaticly, e.g. wt.changes_from(wt.basis_tree()),
    Gary> branch.repository.revision_tree(rev_id), so I think
    Gary> that it is better that there is one lock for all of
    Gary> this. Am I wrong?

No, it's ok, I just got lost in the diff.

We need to add some doc for the --overwrite option though.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-09-05 12:33:03 +0000
3+++ NEWS 2009-10-04 17:10:21 +0000
4@@ -28,6 +28,11 @@
5 defaults to '.bzr-upload.revid'.
6 (Vincent Ladeuil, #423331)
7
8+* Upload now checks that the revision we are uploading is a descendent
9+ from the revision that was uploaded, and hence that the branchs that
10+ they were uploaded from have not diverged. This can be ignored by passing
11+ the --overwrite option. (Gary van der Merwe)
12+
13
14 Bug Fixes
15 *********
16
17=== modified file '__init__.py'
18--- __init__.py 2009-09-05 12:33:03 +0000
19+++ __init__.py 2009-10-04 17:10:21 +0000
20@@ -39,6 +39,15 @@
21
22 bzr upload --full sftp://user@host/location/on/webserver
23
24+This command only works on the revision beening uploaded is a decendent of the
25+revision that was previously uploaded, and that they are hence from branches
26+that have not diverged. Branches are considered diverged if the destination
27+branch's most recent commit is one that has not been merged (directly or
28+indirectly) by the source branch.
29+
30+If branches have diverged, you can use 'bzr upload --overwrite' to replace
31+the other branch completely, discarding its unmerged changes.
32+
33
34 Automatically Uploading
35 -----------------------
36@@ -153,6 +162,7 @@
37 osutils,
38 urlutils,
39 workingtree,
40+ revision
41 )
42 """)
43
44@@ -243,15 +253,23 @@
45 self.quiet = quiet
46 self._pending_deletions = []
47 self._pending_renames = []
48+ self._uploaded_revid = None
49
50 def set_uploaded_revid(self, rev_id):
51 # XXX: Add tests for concurrent updates, etc.
52 revid_path = get_upload_revid_location(self.branch)
53 self.to_transport.put_bytes(revid_path, rev_id)
54+ self._uploaded_revid = rev_id
55
56 def get_uploaded_revid(self):
57- revid_path = get_upload_revid_location(self.branch)
58- return self.to_transport.get_bytes(revid_path)
59+ if self._uploaded_revid is None:
60+ revid_path = get_upload_revid_location(self.branch)
61+ try:
62+ self._uploaded_revid = self.to_transport.get_bytes(revid_path)
63+ except errors.NoSuchFile:
64+ # We have not upload to here.
65+ self._uploaded_revid = revision.NULL_REVISION
66+ return self._uploaded_revid
67
68 def upload_file(self, relpath, id, mode=None):
69 if mode is None:
70@@ -388,9 +406,9 @@
71 def upload_tree(self):
72 # If we can't find the revid file on the remote location, upload the
73 # full tree instead
74- try:
75- rev_id = self.get_uploaded_revid()
76- except errors.NoSuchFile:
77+ rev_id = self.get_uploaded_revid()
78+
79+ if rev_id == revision.NULL_REVISION:
80 if not self.quiet:
81 self.outf.write('No uploaded revision id found,'
82 ' switching to full upload\n')
83@@ -483,6 +501,7 @@
84 takes_options = [
85 'revision',
86 'remember',
87+ 'overwrite',
88 option.Option('full', 'Upload the full working tree.'),
89 option.Option('quiet', 'Do not output what is being done.',
90 short_name='q'),
91@@ -498,7 +517,7 @@
92 ]
93
94 def run(self, location=None, full=False, revision=None, remember=None,
95- directory=None, quiet=False, auto=None
96+ directory=None, quiet=False, auto=None, overwrite=False
97 ):
98 if directory is None:
99 directory = u'.'
100@@ -509,57 +528,73 @@
101
102 (wt, branch,
103 relpath) = bzrdir.BzrDir.open_containing_tree_or_branch(directory)
104-
105+
106 if wt:
107- changes = wt.changes_from(wt.basis_tree())
108-
109- if revision is None and changes.has_changed():
110- raise errors.UncommittedChanges(wt)
111-
112- if location is None:
113- stored_loc = get_upload_location(branch)
114- if stored_loc is None:
115- raise errors.BzrCommandError('No upload location'
116- ' known or specified.')
117- else:
118- # FIXME: Not currently tested
119- display_url = urlutils.unescape_for_display(stored_loc,
120- self.outf.encoding)
121- self.outf.write("Using saved location: %s\n" % display_url)
122- location = stored_loc
123-
124- to_transport = transport.get_transport(location)
125-
126- # Check that we are not uploading to a existing working tree.
127+ wt.lock_read()
128+ else:
129+ branch.lock_read()
130 try:
131- to_bzr_dir = bzrdir.BzrDir.open_from_transport(to_transport)
132- has_wt = to_bzr_dir.has_workingtree()
133- except errors.NotBranchError:
134- has_wt = False
135- except errors.NotLocalUrl:
136- # The exception raised is a bit weird... but that's life.
137- has_wt = True
138-
139- if has_wt:
140- raise CannotUploadToWorkingTreeError(url=location)
141-
142- if revision is None:
143- rev_id = branch.last_revision()
144- else:
145- if len(revision) != 1:
146- raise errors.BzrCommandError(
147- 'bzr upload --revision takes exactly 1 argument')
148- rev_id = revision[0].in_history(branch).rev_id
149-
150- tree = branch.repository.revision_tree(rev_id)
151-
152- uploader = BzrUploader(branch, to_transport, self.outf, tree,
153- rev_id, quiet=quiet)
154-
155- if full:
156- uploader.upload_full_tree()
157- else:
158- uploader.upload_tree()
159+ if wt:
160+ changes = wt.changes_from(wt.basis_tree())
161+
162+ if revision is None and changes.has_changed():
163+ raise errors.UncommittedChanges(wt)
164+
165+ if location is None:
166+ stored_loc = get_upload_location(branch)
167+ if stored_loc is None:
168+ raise errors.BzrCommandError('No upload location'
169+ ' known or specified.')
170+ else:
171+ # FIXME: Not currently tested
172+ display_url = urlutils.unescape_for_display(stored_loc,
173+ self.outf.encoding)
174+ self.outf.write("Using saved location: %s\n" % display_url)
175+ location = stored_loc
176+
177+ to_transport = transport.get_transport(location)
178+
179+ # Check that we are not uploading to a existing working tree.
180+ try:
181+ to_bzr_dir = bzrdir.BzrDir.open_from_transport(to_transport)
182+ has_wt = to_bzr_dir.has_workingtree()
183+ except errors.NotBranchError:
184+ has_wt = False
185+ except errors.NotLocalUrl:
186+ # The exception raised is a bit weird... but that's life.
187+ has_wt = True
188+
189+ if has_wt:
190+ raise CannotUploadToWorkingTreeError(url=location)
191+
192+ if revision is None:
193+ rev_id = branch.last_revision()
194+ else:
195+ if len(revision) != 1:
196+ raise errors.BzrCommandError(
197+ 'bzr upload --revision takes exactly 1 argument')
198+ rev_id = revision[0].in_history(branch).rev_id
199+
200+ tree = branch.repository.revision_tree(rev_id)
201+
202+ uploader = BzrUploader(branch, to_transport, self.outf, tree,
203+ rev_id, quiet=quiet)
204+
205+ if not overwrite:
206+ prev_uploaded_rev_id = uploader.get_uploaded_revid()
207+ graph = branch.repository.get_graph()
208+ if not graph.is_ancestor(prev_uploaded_rev_id, rev_id):
209+ raise DivergedError(rev_id, prev_uploaded_rev_id)
210+
211+ if full:
212+ uploader.upload_full_tree()
213+ else:
214+ uploader.upload_tree()
215+ finally:
216+ if wt:
217+ wt.unlock()
218+ else:
219+ branch.unlock()
220
221 # We uploaded successfully, remember it
222 if get_upload_location(branch) is None or remember:
223@@ -583,6 +618,17 @@
224 else:
225 auto_hook_available = False
226
227+class DivergedError(errors.BzrError):
228+
229+ _fmt = ("The revision upload to this location is from a branch that is "
230+ "diverged from this branch: %(upload_revid)s")
231+
232+ # We should probably tell the user to use merge on the diverged branch,
233+ # but how do we explan which branch they need to merge from!
234+
235+ def __init__(self, revid, upload_revid):
236+ self.revid = revid
237+ self.upload_revid = upload_revid
238
239 def load_tests(basic_tests, module, loader):
240 # This module shouldn't define any tests but I don't know how to report
241
242=== modified file 'tests/test_upload.py'
243--- tests/test_upload.py 2009-09-02 22:31:06 +0000
244+++ tests/test_upload.py 2009-10-04 17:10:21 +0000
245@@ -29,6 +29,7 @@
246 tests,
247 transport,
248 workingtree,
249+ uncommit,
250 )
251 from bzrlib.smart import server as smart_server
252
253@@ -43,6 +44,7 @@
254 cmd_upload,
255 get_upload_auto,
256 CannotUploadToWorkingTreeError,
257+ DivergedError,
258 )
259
260
261@@ -617,3 +619,36 @@
262 self.do_full_upload(directory=remote_branch_url)
263 self.assertUpFileEqual('foo', 'hello')
264
265+class TestUploadDiverged(tests.TestCaseWithTransport,
266+ UploadUtilsMixin):
267+
268+ def setUp(self):
269+ super(TestUploadDiverged, self).setUp()
270+ self.diverged_tree = self.create_diverged_tree_and_upload_location()
271+
272+ def create_diverged_tree_and_upload_location(self):
273+ tree_a = self.make_branch_and_tree('tree_a')
274+ tree_a.commit('message 1', rev_id='rev1')
275+ tree_a.commit('message 2', rev_id='rev2a')
276+ tree_b = tree_a.bzrdir.sprout('tree_b').open_workingtree()
277+ uncommit.uncommit(tree_b.branch, tree=tree_b)
278+ tree_b.commit('message 2', rev_id='rev2b')
279+
280+ # upload tree a
281+ self.do_full_upload(directory=tree_a.basedir)
282+ return tree_b
283+
284+ def assertRevidUploaded(self, revid):
285+ t = self.get_transport(self.upload_dir)
286+ uploaded_revid = t.get_bytes('.bzr-upload.revid')
287+ self.assertEqual(revid, uploaded_revid)
288+
289+ def test_cant_upload_diverged(self):
290+ self.assertRaises(DivergedError, self.do_incremental_upload,
291+ directory=self.diverged_tree.basedir)
292+ self.assertRevidUploaded('rev2a')
293+
294+ def test_upload_diverged_with_overwrite(self):
295+ self.do_incremental_upload(directory=self.diverged_tree.basedir,
296+ overwrite=True)
297+ self.assertRevidUploaded('rev2b')

Subscribers

People subscribed via source and target branches

to status/vote changes: