Patchwork strip: de-fang --backup option (BC)

login
register
mail settings
Submitter Jordi Gutiérrez Hermoso
Date July 6, 2014, 7:37 p.m.
Message ID <5ae152e7b6df2809f5c6.1404675438@Iris>
Download mbox | patch
Permalink /patch/5118/
State Changes Requested
Headers show

Comments

Jordi Gutiérrez Hermoso - July 6, 2014, 7:37 p.m.
# HG changeset patch
# User Jordi Gutiérrez Hermoso <jordigh@octave.org>
# Date 1404603671 14400
#      Sat Jul 05 19:41:11 2014 -0400
# Node ID 5ae152e7b6df2809f5c6445058850086e1f0175f
# Parent  0022ee690446dcf42075780ff9022d8f7d8e69f0
strip: de-fang --backup option (BC)

The --backup option does the *opposite* of backing up stripped commits
in most cases. Although it's already been deprecated since May 2010
(by cset ee5b112aa529), this option is so dangerous that it should be
completely dummied-out. The danger is that it is very easy to typo `hg
strip -b foo` instead of `hg strip -B foo`. This typo deletes data
with no backup!

The danger of this behaviour is so severe that its removal warrants
breaking backwards compatibility.
Jordi Gutiérrez Hermoso - July 8, 2014, 2:01 p.m.
On Sun, 2014-07-06 at 15:37 -0400, Jordi Gutiérrez Hermoso wrote:
> # HG changeset patch
> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> # Date 1404603671 14400
> #      Sat Jul 05 19:41:11 2014 -0400
> # Node ID 5ae152e7b6df2809f5c6445058850086e1f0175f
> # Parent  0022ee690446dcf42075780ff9022d8f7d8e69f0
> strip: de-fang --backup option (BC)

I have good reason to suspect that at least one Facebook employee
swore hg off forever after getting bitten by `hg strip -b`. This is a
very dangerous hidden land mine. I really urge you to consider
accepting this BC.

- Jordi G. H.
Sean Farley - July 8, 2014, 4:05 p.m.
Jordi Gutiérrez Hermoso writes:

> On Sun, 2014-07-06 at 15:37 -0400, Jordi Gutiérrez Hermoso wrote:
>> # HG changeset patch
>> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
>> # Date 1404603671 14400
>> #      Sat Jul 05 19:41:11 2014 -0400
>> # Node ID 5ae152e7b6df2809f5c6445058850086e1f0175f
>> # Parent  0022ee690446dcf42075780ff9022d8f7d8e69f0
>> strip: de-fang --backup option (BC)
>
> I have good reason to suspect that at least one Facebook employee
> swore hg off forever after getting bitten by `hg strip -b`. This is a
> very dangerous hidden land mine. I really urge you to consider
> accepting this BC.

I'll chime in and say that I am in favor of this.
Augie Fackler - July 9, 2014, 1:49 a.m.
On Tue, Jul 08, 2014 at 11:05:18AM -0500, Sean Farley wrote:
>
> Jordi Gutiérrez Hermoso writes:
>
> > On Sun, 2014-07-06 at 15:37 -0400, Jordi Gutiérrez Hermoso wrote:
> >> # HG changeset patch
> >> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> >> # Date 1404603671 14400
> >> #      Sat Jul 05 19:41:11 2014 -0400
> >> # Node ID 5ae152e7b6df2809f5c6445058850086e1f0175f
> >> # Parent  0022ee690446dcf42075780ff9022d8f7d8e69f0
> >> strip: de-fang --backup option (BC)
> >
> > I have good reason to suspect that at least one Facebook employee
> > swore hg off forever after getting bitten by `hg strip -b`. This is a
> > very dangerous hidden land mine. I really urge you to consider
> > accepting this BC.
>
> I'll chime in and say that I am in favor of this.

I'm +1 on this patch as well, but unwilling to go so far as queueing
it, as I'm not sure what mpm will think.

It may be worth adding a long-form-only --no-backup option for people
that really like blowing their own feet off. Maybe even
--no-backup-please-eat-my-data.

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Siddharth Agarwal - July 9, 2014, 3:22 a.m.
On 07/08/2014 06:49 PM, Augie Fackler wrote:
> On Tue, Jul 08, 2014 at 11:05:18AM -0500, Sean Farley wrote:
>> Jordi Gutiérrez Hermoso writes:
>>
>>> On Sun, 2014-07-06 at 15:37 -0400, Jordi Gutiérrez Hermoso wrote:
>>>> # HG changeset patch
>>>> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
>>>> # Date 1404603671 14400
>>>> #      Sat Jul 05 19:41:11 2014 -0400
>>>> # Node ID 5ae152e7b6df2809f5c6445058850086e1f0175f
>>>> # Parent  0022ee690446dcf42075780ff9022d8f7d8e69f0
>>>> strip: de-fang --backup option (BC)
>>> I have good reason to suspect that at least one Facebook employee
>>> swore hg off forever after getting bitten by `hg strip -b`. This is a
>>> very dangerous hidden land mine. I really urge you to consider
>>> accepting this BC.
>> I'll chime in and say that I am in favor of this.
> I'm +1 on this patch as well, but unwilling to go so far as queueing
> it, as I'm not sure what mpm will think.
>
> It may be worth adding a long-form-only --no-backup option for people
> that really like blowing their own feet off. Maybe even
> --no-backup-please-eat-my-data.

strip already has --no-backup.

I'm an enthusiastic +1 on this.

>
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Matt Mackall - July 12, 2014, 12:01 a.m.
On Sun, 2014-07-06 at 15:37 -0400, Jordi Gutiérrez Hermoso wrote:
> # HG changeset patch
> # User Jordi Gutiérrez Hermoso <jordigh@octave.org>
> # Date 1404603671 14400
> #      Sat Jul 05 19:41:11 2014 -0400
> # Node ID 5ae152e7b6df2809f5c6445058850086e1f0175f
> # Parent  0022ee690446dcf42075780ff9022d8f7d8e69f0
> strip: de-fang --backup option (BC)

I committed an alternate version that just drops the option. Feel free
to follow up with a patch to tidy up the backup code.

Patch

diff --git a/hgext/strip.py b/hgext/strip.py
--- a/hgext/strip.py
+++ b/hgext/strip.py
@@ -42,7 +42,7 @@  def checklocalchanges(repo, force=False,
             raise util.Abort(_("local changed subrepos found" + excsuffix))
     return m, a, r, d
 
-def strip(ui, repo, revs, update=True, backup="all", force=None):
+def strip(ui, repo, revs, update=True, backup=True, force=None):
     wlock = lock = None
     try:
         wlock = repo.wlock()
@@ -70,9 +70,8 @@  def strip(ui, repo, revs, update=True, b
                                'option)'), _('REV')),
           ('f', 'force', None, _('force removal of changesets, discard '
                                  'uncommitted changes (no backup)')),
-          ('b', 'backup', None, _('bundle only changesets with local revision'
-                                  ' number greater than REV which are not'
-                                  ' descendants of REV (DEPRECATED)')),
+          ('b', 'backup', None, _('does nothing, used to do the opposite '
+                                  'of backing up (DEPRECATED)')),
           ('', 'no-backup', None, _('no backups')),
           ('', 'nobackup', None, _('no backups (DEPRECATED)')),
           ('n', '', None, _('ignored  (DEPRECATED)')),
@@ -109,11 +108,12 @@  def stripcmd(ui, repo, *revs, **opts):
 
     Return 0 on success.
     """
-    backup = 'all'
+    backup = True
+    if opts.get('no_backup') or opts.get('nobackup'):
+        backup = False
     if opts.get('backup'):
-        backup = 'strip'
-    elif opts.get('no_backup') or opts.get('nobackup'):
-        backup = 'none'
+        ui.warn(_('(ignoring --backup option, '
+                  'did you mean --bookmark or -B?)\n'))
 
     cl = repo.changelog
     revs = list(revs) + opts.get('rev')
diff --git a/mercurial/repair.py b/mercurial/repair.py
--- a/mercurial/repair.py
+++ b/mercurial/repair.py
@@ -47,7 +47,7 @@  def _collectbrokencsets(repo, files, str
 
     return s
 
-def strip(ui, repo, nodelist, backup="all", topic='backup'):
+def strip(ui, repo, nodelist, backup=True, topic='backup'):
     repo = repo.unfiltered()
     repo.destroying()
 
@@ -58,8 +58,6 @@  def strip(ui, repo, nodelist, backup="al
     striplist = [cl.rev(node) for node in nodelist]
     striprev = min(striplist)
 
-    keeppartialbundle = backup == 'strip'
-
     # Some revisions with rev > striprev may not be descendants of striprev.
     # We have to find these revisions and put them in a bundle, so that
     # we can restore them after the truncations.
@@ -109,7 +107,7 @@  def strip(ui, repo, nodelist, backup="al
     # create a changegroup for all the branches we need to keep
     backupfile = None
     vfs = repo.vfs
-    if backup == "all":
+    if backup:
         backupfile = _bundle(repo, stripbases, cl.heads(), node, topic)
         repo.ui.status(_("saved backup bundle to %s\n") %
                        vfs.join(backupfile))
@@ -118,7 +116,7 @@  def strip(ui, repo, nodelist, backup="al
     if saveheads or savebases:
         # do not compress partial bundle if we remove it from disk later
         chgrpfile = _bundle(repo, savebases, saveheads, node, 'temp',
-                            compress=keeppartialbundle)
+                            compress=False)
 
     mfst = repo.manifest
 
@@ -156,8 +154,8 @@  def strip(ui, repo, nodelist, backup="al
             if not repo.ui.verbose:
                 repo.ui.popbuffer()
             f.close()
-            if not keeppartialbundle:
-                vfs.unlink(chgrpfile)
+
+            vfs.unlink(chgrpfile)
 
         # remove undo files
         for undovfs, undofile in repo.undofiles():