[PATCH V2 STABLE] strip: do not overwrite existing backup bundles

[Matt Mackall]

On Wed, 2011-12-28 at 18:55 +0100, Laurens Holst wrote:
> # HG changeset patch
> # User Laurens Holst <laurens.hg at grauw.nl>
> # Date 1325036331 -3600
> # Node ID 894516ae07e4353893e69a3bfabffaf07aa38ccf
> # Parent  3317d5c34f384c6d1d8440e19980a2261beecc7a
> strip: do not overwrite existing backup bundles
> 
> I ran into a data loss issue where a strips backup overwrote a previous backup
> which had more descendants. This happens because the backup file name composed
> of just the hash of the first stripped changeset. The issue is fixed by
> suffixing the backup file with a - and a number as needed.
> 
> diff -r 3317d5c34f38 -r 894516ae07e4 mercurial/repair.py
> --- a/mercurial/repair.py	Wed Dec 21 18:20:15 2011 +0100
> +++ b/mercurial/repair.py	Wed Dec 28 02:38:51 2011 +0100
> @@ -9,7 +9,7 @@
>  from mercurial import changegroup, bookmarks, phases
>  from mercurial.node import short
>  from mercurial.i18n import _
> -import os
> +import os, itertools

While itertools is an interesting module, Mercurial has thus far avoided
using it. I don't think a construct as trivial as itertools.count()
justifies pulling it in[1].

>  def _bundle(repo, bases, heads, node, suffix, compress=True):
>      """create a bundle with the specified revisions as a backup"""
> @@ -17,7 +17,17 @@
>      backupdir = repo.join("strip-backup")
>      if not os.path.isdir(backupdir):
>          os.mkdir(backupdir)
> -    name = os.path.join(backupdir, "%s-%s.hg" % (short(node), suffix))
> +
> +    # never overwrite existing bundle
> +    for i in itertools.count(0):
> +        if i < 1:
> +            filename = "%s-%s.hg" % (short(node), suffix)
> +        else:
> +            filename = "%s-%s-%d.hg" % (short(node), suffix, i)
> +        name = os.path.join(backupdir, filename)
> +        if not os.path.exists(name):
> +            break

Compare with this:

name = os.path.join(backupdir, "%s-%s.hg" % (short(node), suffix))
i = 0
while os.path.exists(name):
    i += 1
    name = os.path.join(backupdir,
                        "%s-%s-%d.hg" % (short(node), suffix, i))

But really, this probably wants to be a utility function:

basename = os.path.join(backupdir, "%s-%s" % (short(node), suffix))
name = util.uniquename(basename, ".hg")

..so that we don't end up rewriting this loop in various places.

[1] Really some clever person should have just built this into xrange:
xrange() == xrange(None) -> 0,1,2,3..
xrange(5, None) -> 5,6,7,8...

-- 
Mathematics is the supreme nostalgia of our time.