[PATCH] shelve: always backup shelves instead of deleting them

[Pierre-Yves David]

On 06/24/2015 12:16 PM, Colin Chan wrote:
> # HG changeset patch
> # User Colin Chan <colinchan at fb.com>
> # Date 1434989752 25200
> #      Mon Jun 22 09:15:52 2015 -0700
> # Node ID 7a02a3b3941cb62203b6efc145e58c543e8b8909
> # Parent  7fdd1782fc4ee9da87d8af13e806dc9055db2c38
> shelve: always backup shelves instead of deleting them
>
> Instead of being deleted, shelve files are now moved into the
> .hg/shelve-backup directory. This is designed to be very similar to how strip
> saves backpus into .ht/strip-backup. The goal is to prevent data loss especially
> when using unshelve (e.g., http://bz.selenic.com/show_bug.cgi?id=4732) at the
> expense of using more disk space over time.

This changes seems strange to me, We should never delete a shelve until 
the whole unshelving process is done. It seems to be the current intend 
of the code, if it happen to currently not be the case, it should be 
easy to fix.

Especially, this change seems unrelated to issue4732 and I fail to see 
how it improve the situation three


>
> diff --git a/hgext/shelve.py b/hgext/shelve.py
> --- a/hgext/shelve.py
> +++ b/hgext/shelve.py
> @@ -22,6 +22,7 @@
>   """
>
>   import collections
> +import itertools
>   from mercurial.i18n import _
>   from mercurial.node import nullid, nullrev, bin, hex
>   from mercurial import changegroup, cmdutil, scmutil, phases, commands
> @@ -48,6 +49,7 @@
>           self.repo = repo
>           self.name = name
>           self.vfs = scmutil.vfs(repo.join('shelved'))
> +        self.backupvfs = scmutil.vfs(repo.join('shelve-backup'))
>           self.ui = self.repo.ui
>           if filetype:
>               self.fname = name + '.' + filetype
> @@ -60,8 +62,21 @@
>       def filename(self):
>           return self.vfs.join(self.fname)
>
> -    def unlink(self):
> -        util.unlink(self.filename())
> +    def backupfilename(self):

This seems to be internal method only, consider renaming them with a 
leading _ to highlight that.

> +        def gennames(base):
> +            yield base
> +            for i in itertools.count(1):
> +                yield '%s-%d' % (base, i)
> +
> +        name = self.backupvfs.join(self.fname)
> +        for n in gennames(name):
> +            if not self.backupvfs.exists(n):
> +                return n
> +
> +    def movetobackup(self):
> +        if not self.backupvfs.isdir():
> +            self.backupvfs.makedir()
> +        util.rename(self.filename(), self.backupfilename())
>
>       def stat(self):
>           return self.vfs.stat(self.fname)
> @@ -281,7 +296,7 @@
>           for (name, _type) in repo.vfs.readdir('shelved'):
>               suffix = name.rsplit('.', 1)[-1]
>               if suffix in ('hg', 'patch'):
> -                shelvedfile(repo, name).unlink()
> +                shelvedfile(repo, name).movetobackup()
>       finally:
>           lockmod.release(wlock)
>
> @@ -293,7 +308,7 @@
>       try:
>           for name in pats:
>               for suffix in 'hg patch'.split():
> -                shelvedfile(repo, name, suffix).unlink()
> +                shelvedfile(repo, name, suffix).movetobackup()
>       except OSError, err:
>           if err.errno != errno.ENOENT:
>               raise
> @@ -442,7 +457,7 @@
>       """remove related files after an unshelve"""
>       if not opts['keep']:
>           for filetype in 'hg patch'.split():
> -            shelvedfile(repo, name, filetype).unlink()
> +            shelvedfile(repo, name, filetype).movetobackup()
>
>   def unshelvecontinue(ui, repo, state, opts):
>       """subcommand to continue an in-progress unshelve"""
> @@ -505,18 +520,19 @@
>       restore. If none is given, the most recent shelved change is used.
>
>       If a shelved change is applied successfully, the bundle that
> -    contains the shelved changes is deleted afterwards.
> +    contains the shelved changes is moved to a backup location
> +    (.hg/shelve-backup).
>
>       Since you can restore a shelved change on top of an arbitrary
>       commit, it is possible that unshelving will result in a conflict
>       between your changes and the commits you are unshelving onto. If
>       this occurs, you must resolve the conflict, then use
>       ``--continue`` to complete the unshelve operation. (The bundle
> -    will not be deleted until you successfully complete the unshelve.)
> +    will not be moved until you successfully complete the unshelve.)
>
>       (Alternatively, you can use ``--abort`` to abandon an unshelve
>       that causes a conflict. This reverts the unshelved changes, and
> -    does not delete the bundle.)
> +    leaves the bundle in place.)
>       """
>       abortf = opts['abort']
>       continuef = opts['continue']
> diff --git a/tests/test-shelve.t b/tests/test-shelve.t
> --- a/tests/test-shelve.t
> +++ b/tests/test-shelve.t
> @@ -85,6 +85,12 @@
>     nothing changed
>     [1]
>
> +make sure shelve files were backed up
> +
> +  $ ls .hg/shelve-backup
> +  default.hg
> +  default.patch
> +
>   create an mq patch - shelving should work fine with a patch applied
>
>     $ echo n > n
> @@ -154,6 +160,14 @@
>     $ hg shelve -d default
>     $ hg qfinish -a -q
>
> +ensure shelve backups aren't overwritten
> +
> +  $ ls .hg/shelve-backup/
> +  default.hg
> +  default.hg-1
> +  default.patch
> +  default.patch-1
> +
>   local edits should not prevent a shelved change from applying
>
>     $ printf "z\na\n" > a/a

-- 
Pierre-Yves David