[RFC] fix for 335033 and 300001

[Martin Pool]

On 15 February 2010 18:57, Parth Malwankar <parth.malwankar at gmail.com> wrote:
> Hello,
>
> I had decided to try and fix two somewhat easy issues
> 335033 and 300001 [1,2].
>
> The fix[3] is incomplete right now.
>
> The basic idea is that instead of just creating a backup.bzr
> file, a backup.bzr.~N~ file is created. This file (*~) is already
> ignored by bzr and the N ensures that there is no conflict
> with and existing directory.
>
> Two things that still remain to be done are, adding tests
> and replacing  os.path.exists below with something that
> works with different transports.
>
> Do bzr already have an API that can be used in case of
> os.path.exists? If not, just adding a '~' would still fix
> 335033.

Hi,

You should use transport.has(name) not os.path.exists.

> Whats the best place to add tests for this fix.

test_upgrade would be a good place to look.

>
> Is there anything else that I am missing?

local variables are lower case.

>
> Regards,
> Parth
>
> [1] https://bugs.launchpad.net/bzr/+bug/335033
> [2] https://bugs.launchpad.net/bzr/+bug/335033
> [3] https://code.launchpad.net/~parthm/bzr/335033
>
>
> [bzr.dev]% bzr diff --old trunk --new 335033
> === modified file 'bzrlib/bzrdir.py'
> --- bzrlib/bzrdir.py    2010-02-10 17:52:08 +0000
> +++ bzrlib/bzrdir.py    2010-02-15 07:29:02 +0000
> @@ -575,6 +575,15 @@
>
>         :return: Tuple with old path name and new path name
>         """
> +        def name_gen(base='backup.bzr'):
> +            counter = 1
> +            name = "%s.~%d~" % (base, counter)
> +            while os.path.exists(name):
> +                counter += 1
> +                name = "%s.~%d~" % (base, counter)
> +            return name
> +
> +        BACKUP_DIR=name_gen()
>         pb = ui.ui_factory.nested_progress_bar()
>         try:
>             # FIXME: bug 300001 -- the backup fails if the backup directory
> @@ -584,9 +593,9 @@
>             # FIXME: bug 262450 -- the backup directory should have the same
>             # permissions as the .bzr directory (probably a bug in copy_tree)
>             old_path = self.root_transport.abspath('.bzr')
> -            new_path = self.root_transport.abspath('backup.bzr')
> +            new_path = self.root_transport.abspath(BACKUP_DIR)
>             ui.ui_factory.note('making backup of %s\n  to %s' %
> (old_path, new_path,))
> -            self.root_transport.copy_tree('.bzr', 'backup.bzr')
> +            self.root_transport.copy_tree('.bzr', BACKUP_DIR)
>             return (old_path, new_path)
>         finally:
>             pb.finished()




-- 
Martin <http://launchpad.net/~mbp/>