Patchwork D7932: [RFC]debugbackups: introduce command to interact with strip backups

login
register
mail settings
Submitter phabricator
Date Jan. 17, 2020, 6:25 p.m.
Message ID <differential-rev-PHID-DREV-l7v5q2gitjstftlm74lk-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/44508/
State Superseded
Headers show

Comments

phabricator - Jan. 17, 2020, 6:25 p.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This vendors backups extension from hg-experimental.
  
  Listing backups and having some utility to apply them is nice. I know we have
  obsmarkers now, but this will help a lot of end users who still uses strip until
  we get evolve out of experimental.
  
  This is RFC because:
  
  - I am not sure whether we want this in core
  - I copied the code from hg-experimental and looks like there is some code
  
  deduplication which needs to be done

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D7932

AFFECTED FILES
  mercurial/debugcommands.py
  tests/test-debugbackup.t

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 22, 2020, 4:40 p.m.
durin42 added a comment.


  I'm +1 on having this functionality in core, FYI.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7932/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7932

To: pulkit, #hg-reviewers
Cc: durin42, mercurial-devel
phabricator - Jan. 27, 2020, 9:59 a.m.
marmoute added a comment.


  Can we call it `debugbackupbundle` for clarify?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7932/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7932

To: pulkit, #hg-reviewers
Cc: marmoute, durin42, mercurial-devel
phabricator - Feb. 12, 2020, 7:29 p.m.
This revision now requires changes to proceed.
durin42 added a comment.
durin42 requested changes to this revision.


  I think I'd prefer debugbackupbundle per marmoute's suggestion.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7932/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7932

To: pulkit, #hg-reviewers, durin42
Cc: marmoute, durin42, mercurial-devel
phabricator - March 9, 2020, 4:26 p.m.
martinvonz added a comment.


  I know this has already been queue and I'm just slow, but could you send follow-up patches to address my comments?

INLINE COMMENTS

> debugcommands.py:3416
> +    ]
> +    + cmdutil.logopts,
> +    _(b"hg debugbackupbundle [--recover HASH]"),

Do we really want to support all of these? (FYI, they include `--stat`, `--graph`, `--style`, `--patch`, and a few more.)

> debugcommands.py:3432
> +    backups = filter(
> +        os.path.isfile, glob.glob(repo.vfs.join("strip-backup") + "/*.hg")
> +    )

I think Windows doesn't care much about `/` vs `\`, but should we ideally use `os.path.join(..., ".hg")` here?

> debugcommands.py:3436
> +
> +    opts["bundle"] = ""
> +    opts["force"] = None

Clearer to pass `bundlename=""` in the call to `getremotechanges()`?

> debugcommands.py:3437
> +    opts["bundle"] = ""
> +    opts["force"] = None
> +    limit = logcmdutil.getlimit(opts)

Clearer to pass `force=None` (or `False`?) in the call to `getremotechanges()`?

> debugcommands.py:3441
> +    def display(other, chlist, displayer):
> +        if opts.get("newest_first"):
> +            chlist.reverse()

Always false, so delete it?

> debugcommands.py:3518-3528
> +                backupdate = time.strftime(
> +                    "%a %H:%M, %Y-%m-%d",
> +                    time.localtime(os.path.getmtime(source)),
> +                )
> +                ui.status("\n%s\n" % (backupdate.ljust(50)))
> +                if ui.verbose:
> +                    ui.status("%s%s\n" % ("bundle:".ljust(13), source))

Perhaps the  template here should apply at the bundle level instead of the changeset level, so the user can also template the date (and maybe bundle name).

> debugcommands.py:3528
> +                        "template"
> +                    ] = "{label('status.modified', node|short)} {desc|firstline}\n"
> +                displayer = logcmdutil.changesetdisplayer(

heh, `status.modified` seems like quite an abuse of the labeling system (I assume it was picked because the author thought that the color they had configured for that looked good in this context too). Could you switch to a different label?

> debugcommands.py:3530
> +                displayer = logcmdutil.changesetdisplayer(
> +                    ui, other, opts, False
> +                )

Does `False` (for the `differ` argument) produce different output than the default (`None`) would? If not, remove the `False`? If it does, could you pass it as a keyword argument instead (`differ=False`)?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7932/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7932

To: pulkit, #hg-reviewers, durin42
Cc: martinvonz, marmoute, durin42, mercurial-devel
phabricator - March 10, 2020, 7:02 p.m.
mharbison72 added inline comments.

INLINE COMMENTS

> martinvonz wrote in debugcommands.py:3432
> I think Windows doesn't care much about `/` vs `\`, but should we ideally use `os.path.join(..., ".hg")` here?

I thought we were trying to avoid `os.path`, so maybe just `repo.vfs.join("strip-backup", ".hg")`?

(And don't these need to be `b''` prefixed for py3?)

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7932/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7932

To: pulkit, #hg-reviewers, durin42
Cc: mharbison72, martinvonz, marmoute, durin42, mercurial-devel
phabricator - March 11, 2020, 12:22 a.m.
marmoute added inline comments.

INLINE COMMENTS

> mharbison72 wrote in debugcommands.py:3432
> I thought we were trying to avoid `os.path`, so maybe just `repo.vfs.join("strip-backup", ".hg")`?
> 
> (And don't these need to be `b''` prefixed for py3?)

+1 on using `vfs` here, this is the abstraction we use everywhere else.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7932/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7932

To: pulkit, #hg-reviewers, durin42
Cc: mharbison72, martinvonz, marmoute, durin42, mercurial-devel

Patch

diff --git a/tests/test-debugbackup.t b/tests/test-debugbackup.t
new file mode 100644
--- /dev/null
+++ b/tests/test-debugbackup.t
@@ -0,0 +1,36 @@ 
+  $ cat >> $HGRCPATH << EOF
+  > [extensions]
+  > strip=
+  > EOF
+
+Setup repo
+
+  $ hg init repo
+  $ cd repo
+
+Test backups list and recover
+
+  $ mkcommit() {
+  >    echo "$1" > "$1"
+  >    hg add "$1"
+  >    hg ci -l $1
+  > }
+  $ mkcommit a
+  $ mkcommit b
+  $ hg strip .
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  saved backup bundle to $TESTTMP/repo/.hg/strip-backup/d2ae7f538514-2953539b-backup.hg (glob)
+  $ hg debugbackups
+  Recover changesets using: hg debugbackups --recover <changeset hash>
+  
+  Available backup changesets:
+  * (glob)
+  d2ae7f538514 b
+
+  $ hg debugbackups --recover d2ae7f538514
+  Unbundling d2ae7f538514
+  adding changesets
+  adding manifests
+  adding file changes
+  added 1 changesets with 1 changes to 1 files
+  new changesets d2ae7f538514 (1 drafts)
diff --git a/mercurial/debugcommands.py b/mercurial/debugcommands.py
--- a/mercurial/debugcommands.py
+++ b/mercurial/debugcommands.py
@@ -11,6 +11,7 @@ 
 import collections
 import difflib
 import errno
+import glob
 import operator
 import os
 import random
@@ -38,6 +39,7 @@ 
 )
 from . import (
     bundle2,
+    bundlerepo,
     changegroup,
     cmdutil,
     color,
@@ -130,6 +132,137 @@ 
 
 
 @command(
+    "debugbackups",
+    [
+        (
+            "",
+            "recover",
+            "",
+            "brings the specified changeset back into the repository",
+        )
+    ]
+    + cmdutil.logopts,
+    _("hg debugbackups [--recover HASH]"),
+)
+def debugbackups(ui, repo, *pats, **opts):
+    """lists the changesets available in backup bundles
+
+    Without any arguments, this command prints a list of the changesets in each
+    backup bundle.
+
+    --recover takes a changeset hash and unbundles the first bundle that
+    contains that hash, which puts that changeset back in your repository.
+
+    --verbose will print the entire commit message and the bundle path for that
+    backup.
+    """
+    backuppath = repo.vfs.join("strip-backup")
+    backups = filter(os.path.isfile, glob.glob(backuppath + "/*.hg"))
+    backups.sort(key=lambda x: os.path.getmtime(x), reverse=True)
+
+    opts["bundle"] = ""
+    opts["force"] = None
+
+    def display(other, chlist, displayer):
+        limit = logcmdutil.getlimit(opts)
+        if opts.get("newest_first"):
+            chlist.reverse()
+        count = 0
+        for n in chlist:
+            if limit is not None and count >= limit:
+                break
+            parents = [p for p in other.changelog.parents(n) if p != nullid]
+            if opts.get("no_merges") and len(parents) == 2:
+                continue
+            count += 1
+            displayer.show(other[n])
+
+    recovernode = opts.get("recover")
+    if recovernode:
+        if scmutil.isrevsymbol(repo, recovernode):
+            ui.warn(_("%s already exists in the repo\n") % recovernode)
+            return
+    else:
+        msg = _(
+            "Recover changesets using: hg debugbackups --recover "
+            "<changeset hash>\n\nAvailable backup changesets:"
+        )
+        ui.status(msg, label="status.removed")
+
+    for backup in backups:
+        # Much of this is copied from the hg incoming logic
+        source = os.path.relpath(backup, encoding.getcwd())
+        source = ui.expandpath(source)
+        source, branches = hg.parseurl(source, opts.get("branch"))
+        try:
+            other = hg.peer(repo, opts, source)
+        except error.LookupError as ex:
+            msg = _("\nwarning: unable to open bundle %s") % source
+            hint = _("\n(missing parent rev %s)\n") % short(ex.name)
+            ui.warn(msg)
+            ui.warn(hint)
+            continue
+        revs, checkout = hg.addbranchrevs(
+            repo, other, branches, opts.get("rev")
+        )
+
+        if revs:
+            revs = [other.lookup(rev) for rev in revs]
+
+        quiet = ui.quiet
+        try:
+            ui.quiet = True
+            other, chlist, cleanupfn = bundlerepo.getremotechanges(
+                ui, repo, other, revs, opts["bundle"], opts["force"]
+            )
+        except error.LookupError:
+            continue
+        finally:
+            ui.quiet = quiet
+
+        try:
+            if chlist:
+                if recovernode:
+                    tr = lock = None
+                    try:
+                        lock = repo.lock()
+                        if scmutil.isrevsymbol(other, recovernode):
+                            ui.status(_("Unbundling %s\n") % (recovernode))
+                            f = hg.openpath(ui, source)
+                            gen = exchange.readbundle(ui, f, source)
+                            tr = repo.transaction("unbundle")
+                            if not isinstance(gen, bundle2.unbundle20):
+                                gen.apply(repo, "unbundle", "bundle:" + source)
+                            if isinstance(gen, bundle2.unbundle20):
+                                bundle2.applybundle(
+                                    repo,
+                                    gen,
+                                    tr,
+                                    source="unbundle",
+                                    url="bundle:" + source,
+                                )
+                            tr.close()
+                            break
+                    finally:
+                        lockmod.release(lock, tr)
+                else:
+                    backupdate = os.path.getmtime(source)
+                    backupdate = time.strftime(
+                        "%a %H:%M, %Y-%m-%d", time.localtime(backupdate)
+                    )
+                    ui.status("\n%s\n" % (backupdate.ljust(50)))
+                    if not ui.verbose:
+                        opts["template"] = "{label('status.modified', node|short)} {desc|firstline}\n"
+                    else:
+                        ui.status("%s%s\n" % ("bundle:".ljust(13), source))
+                    displayer = logcmdutil.changesetdisplayer(ui, other, opts, False)
+                    display(other, chlist, displayer)
+                    displayer.close()
+        finally:
+            cleanupfn()
+
+
+@command(
     b'debugbuilddag',
     [
         (