Merge lp:~andrewfenn/bzr-upload/force-option into lp:bzr-upload

[Andrew Fenn]

[Robert Collins (lifeless)]

I'm not a bzr-upload dev, but this looks unusual to me. Do you mean to ignore *all* errors on the target and just keep trying? That seems fragile to me; what if the fs is readonly, or if the failure is a rename and so on - wouldn't it be better to handle an error by resetting the entire tree to a known good state?

[Andrew Fenn (andrewfenn)]

I've been using my fork at the office without issue. If you want to
re-upload 2 gigs worth of data simply because one folder or file is
missing on the target machine then be my guest.

On Mon, Jun 21, 2010 at 7:30 AM, Robert Collins
<email address hidden> wrote:
> I'm not a bzr-upload dev, but this looks unusual to me. Do you mean to ignore *all* errors on the target and just keep trying? That seems fragile to me; what if the fs is readonly, or if the failure is a rename and so on - wouldn't it be better to handle an error by resetting the entire tree to a known good state?
> --
> https://code.launchpad.net/~andrewfenn/bzr-upload/force-option/+merge/15998
> You are the owner of lp:~andrewfenn/bzr-upload/force-option.
>

[Robert Collins (lifeless)]

No, I don't want to upload 2G of data, I was merely seeking to
understand more of the intent of your patch, hopefully to make it
clearer to the bzr-upload devs whom are the only ones that can merge
the patch to trunk.

-Rob

[Andrew Fenn (andrewfenn)]

Hi Robert,

Basically it fixes this bug..
https://bugs.launchpad.net/bzr-upload/+bug/215612

I submitted a patch and was told to make this branch by the bzr-upload devs. It has since been left ignored. I'm not bitter about this but I don't understand sure why they would make me go to such efforts only to then not even review the patch.

I believe this stems from simply disagreement with the main developers. If I am correct in saying I do not think they want such a feature and (as you can read in the bug) think it is better for a user to completely upload the entire contents of a bazaar repo again if it becomes out of sync; hence my comment about uploading the 2GB of data. :)

Since submitting this patch I've noticed some other things that bzr-plugin does wrong which I have already forked. An example of this being if moving renamed files errors out, it creates problems because the temporary name given while moving is random so there is no way to recover from such an error.

I'd be happy to share such fixes if it hasn't already been fixed upstream of course.

[Martin Pool (mbp)]

To me, this seems basically plausible, except that the bare 'except:' would be better off catching FileExists, which may have been Robert's original point.

This might be better described as 'ignore errors' rather than 'force', which to me would imply specifically deleting things that don't match.

It might be nice to use a higher order function for the 'ignore' behaviour and it would be good to have tests (if upload has tests at the moment.)

On the whole this would be better in than out, though.

[Vincent Ladeuil (vila)]

> Hi Robert,
>
> Basically it fixes this bug..
> https://bugs.launchpad.net/bzr-upload/+bug/215612
>
> I submitted a patch and was told to make this branch by the bzr-upload devs.
> It has since been left ignored.

Sorry about that, dunno why it get lost. In case it occurs again, don't hesitate to ping us via the review comments (and adding a description for the merge proposal also helps getting it reviewed faster).

> I believe this stems from simply disagreement with the main developers.

I don't think so, your patch address *part* of the problems raised in the bug report and as such will not fully fix it, but the part you address is fine.

> If I
> am correct in saying I do not think they want such a feature and (as you can
> read in the bug) think it is better for a user to completely upload the entire
> contents of a bazaar repo again if it becomes out of sync; hence my comment
> about uploading the 2GB of data. :)

If a full upload is 2GB of data, I'm sure you're interested in keeping it well synchronized and to achieve this you have to make sure the remote working tree is not altered in ways that could break the mechanisms used to maintain it.

Deletions are the easy part.

>
> Since submitting this patch I've noticed some other things that bzr-plugin
> does wrong which I have already forked.

Did you file bugs for them ?

On with the review itself now:

- we need tests for this, I can help help you write them if you want,

38 +
39 + if not self.force:
40 + self.to_transport.mkdir(relpath, mode)
41 + else:
42 + try:
43 + self.to_transport.mkdir(relpath, mode)
44 + except:
45 + self.outf.write('Directory %s already exists. Ignoring.\n' % relpath)

How about using make_remote_dir_robustly() in this case which is targeted at this use case ? Or even better, add the force parameter support to make_remote_dir() and get rid of make_remote_dir_robustly() ? That would reduce code duplication.

The bug also discussed alternate names to use instead of force, why did you chose this one ?

review: Needs Fixing

[Jelmer Vernooij (jelmer)]

Setting back to work in progress for now, as it has been requiring followup for > 2 years.

Unmerged revisions

69. By Andrew Fenn on 2009-12-11

- Fix for unit test

68. By Andrew Fenn on 2009-12-10

- Adds force option that ignores delete and mkdir errors.

=== modified file '__init__.py'
--- __init__.py	2009-12-02 10:02:05 +0000
+++ __init__.py	2009-12-11 04:21:13 +0000
@@ -244,6 +244,11 @@
         quiet = False # Default to False if not specified
     return quiet
 
+def get_upload_force(branch):
+    force = _get_branch_bool_option(branch, 'force')
+    if force is None:
+        force = False # Default to False if not specified
+    return force
 
 def set_upload_auto_quiet(branch, quiet):
     _set_branch_option(branch, 'upload_auto_quiet', quiet)
@@ -252,13 +257,15 @@
 class BzrUploader(object):
 
     def __init__(self, branch, to_transport, outf, tree, rev_id,
-                 quiet=False):
+                 quiet=False,
+                 force=False):
         self.branch = branch
         self.to_transport = to_transport
         self.outf = outf
         self.tree = tree
         self.rev_id = rev_id
         self.quiet = quiet
+        self.force = force
         self._pending_deletions = []
         self._pending_renames = []
         self._uploaded_revid = None
@@ -325,7 +332,14 @@
         if relpath not in ignored_files:
             if mode is None:
                 mode = 0775
-            self.to_transport.mkdir(relpath, mode)
+            
+            if not self.force:
+                self.to_transport.mkdir(relpath, mode)
+            else:
+                try:
+                    self.to_transport.mkdir(relpath, mode)
+                except:
+                    self.outf.write('Directory %s already exists. Ignoring.\n' % relpath)
         else:
             if not self.quiet:
                 self.outf.write('Ignoring %s\n' % relpath)
@@ -353,15 +367,31 @@
     def delete_remote_file(self, relpath):
         if not self.quiet:
             self.outf.write('Deleting %s\n' % relpath)
-        self.to_transport.delete(relpath)
+
+        if not self.force:
+            self.to_transport.delete(relpath)
+        else:
+            try:
+                self.to_transport.delete(relpath)
+            except errors.NoSuchFile:
+                self.outf.write('Error: %s does not exist\n' % relpath)
 
     def delete_remote_dir(self, relpath):
         if not self.quiet:
             self.outf.write('Deleting %s\n' % relpath)
-        self.to_transport.rmdir(relpath)
+        
+        if not self.force:
+            self.to_transport.rmdir(relpath)
+        else:
+            try:
+                self.to_transport.rmdir(relpath)
+            except errors.PathError:
+                self.outf.write('Error: %s can not be deleted\n' % relpath)
 
     def delete_remote_dir_maybe(self, relpath):
         """Try to delete relpath, keeping failures to retry later."""
+        if not self.quiet:
+            self.outf.write('Maybe deleting %s\n' % relpath)
         try:
             self.to_transport.rmdir(relpath)
         # any kind of PathError would be OK, though we normally expect
@@ -374,7 +404,13 @@
             # Process the previously failed deletions in reverse order to
             # delete children before parents
             for relpath in reversed(self._pending_deletions):
-                self.to_transport.rmdir(relpath)
+                if not self.force:
+                    self.to_transport.rmdir(relpath)
+                else:
+                    try:
+                        self.to_transport.rmdir(relpath)
+                    except errors.PathError:
+                        self.outf.write('Error: %s can not be deleted\n' % relpath)
             # The following shouldn't be needed since we use it once per
             # upload, but better safe than sorry ;-)
             self._pending_deletions = []
@@ -545,11 +581,13 @@
                       ),
         option.Option('auto',
                       'Trigger an upload from this branch whenever the tip '
-                      'revision changes.')
+                      'revision changes.'),
+        option.Option('force', 'Continues uploading even when files are out of sync.',
+                      short_name='f')
        ]
 
     def run(self, location=None, full=False, revision=None, remember=None,
-            directory=None, quiet=False, auto=None, overwrite=False
+            directory=None, quiet=False, force=False, auto=None, overwrite=False
             ):
         if directory is None:
             directory = u'.'
@@ -611,7 +649,7 @@
             tree = branch.repository.revision_tree(rev_id)
 
             uploader = BzrUploader(branch, to_transport, self.outf, tree,
-                                   rev_id, quiet=quiet)
+                                   rev_id, quiet=quiet, force=force)
 
             if not overwrite:
                 prev_uploaded_rev_id = uploader.get_uploaded_revid()

=== modified file 'auto_upload_hook.py'
--- auto_upload_hook.py	2009-09-02 17:05:27 +0000
+++ auto_upload_hook.py	2009-12-11 04:21:13 +0000
@@ -36,6 +36,7 @@
     if not auto_upload:
         return
     quiet = upload.get_upload_auto_quiet(source_branch)
+    force = upload.get_upload_force(source_branch)
     if not quiet:
         display_url = urlutils.unescape_for_display(
             destination, osutils.get_terminal_encoding())
@@ -44,5 +45,5 @@
     last_revision = source_branch.last_revision()
     last_tree = source_branch.repository.revision_tree(last_revision)
     uploader = upload.BzrUploader(source_branch, to_transport, sys.stdout,
-                                  last_tree, last_revision, quiet=quiet)
+                                  last_tree, last_revision, quiet=quiet, force=force)
     uploader.upload_tree()