Patchwork [6,of,7] histedit: unify strip backup files on success (BC)

login
register
mail settings
Submitter Jun Wu
Date July 8, 2017, 11:51 p.m.
Message ID <d75e65724d2bbcf17fca.1499557894@x1c>
Download mbox | patch
Permalink /patch/22141/
State Accepted
Headers show

Comments

Jun Wu - July 8, 2017, 11:51 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1499557831 25200
#      Sat Jul 08 16:50:31 2017 -0700
# Node ID d75e65724d2bbcf17fcaaad705e463e84a3de5d2
# Parent  9a325ae88021e0e02a87ef1ae6baa8a199405140
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r d75e65724d2b
histedit: unify strip backup files on success (BC)

Previously we wrote two different strip backup files on success. This patch
unifies them. It will make scmutil.cleanupnodes migration more smooth.
via Mercurial-devel - July 9, 2017, 6:24 p.m.
On Sat, Jul 8, 2017 at 4:51 PM, Jun Wu <quark@fb.com> wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1499557831 25200
> #      Sat Jul 08 16:50:31 2017 -0700
> # Node ID d75e65724d2bbcf17fcaaad705e463e84a3de5d2
> # Parent  9a325ae88021e0e02a87ef1ae6baa8a199405140
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r d75e65724d2b
> histedit: unify strip backup files on success (BC)
>
> Previously we wrote two different strip backup files on success. This patch
> unifies them. It will make scmutil.cleanupnodes migration more smooth.

Makes sense to me. I'm queuing the series, thanks.

Augie, do you remember if there was a reason to keep them separate?
Augie Fackler - July 9, 2017, 7:28 p.m.
> On Jul 9, 2017, at 2:24 PM, Martin von Zweigbergk <martinvonz@google.com> wrote:
> 
> On Sat, Jul 8, 2017 at 4:51 PM, Jun Wu <quark@fb.com> wrote:
>> # HG changeset patch
>> # User Jun Wu <quark@fb.com>
>> # Date 1499557831 25200
>> #      Sat Jul 08 16:50:31 2017 -0700
>> # Node ID d75e65724d2bbcf17fcaaad705e463e84a3de5d2
>> # Parent  9a325ae88021e0e02a87ef1ae6baa8a199405140
>> # Available At https://bitbucket.org/quark-zju/hg-draft
>> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r d75e65724d2b
>> histedit: unify strip backup files on success (BC)
>> 
>> Previously we wrote two different strip backup files on success. This patch
>> unifies them. It will make scmutil.cleanupnodes migration more smooth.
> 
> Makes sense to me. I'm queuing the series, thanks.
> 
> Augie, do you remember if there was a reason to keep them separate?

The only reason was structural in the code: histedit has “intermediate” nodes which are an implementation detail, and “old” nodes, which are the precursors to the end state. Internally, the lists are separate because you always want to reap “intermediate" nodes, but might want to preserve “old” and in --keep or --abort cases.

It might be nice to keep them separate but have one be marked “histedit-internal” and “histedit-before” or something.
Jun Wu - July 9, 2017, 7:54 p.m.
Excerpts from Augie Fackler's message of 2017-07-09 15:28:15 -0400:
> 
> > On Jul 9, 2017, at 2:24 PM, Martin von Zweigbergk <martinvonz@google.com> wrote:
> > 
> > On Sat, Jul 8, 2017 at 4:51 PM, Jun Wu <quark@fb.com> wrote:
> >> # HG changeset patch
> >> # User Jun Wu <quark@fb.com>
> >> # Date 1499557831 25200
> >> #      Sat Jul 08 16:50:31 2017 -0700
> >> # Node ID d75e65724d2bbcf17fcaaad705e463e84a3de5d2
> >> # Parent  9a325ae88021e0e02a87ef1ae6baa8a199405140
> >> # Available At https://bitbucket.org/quark-zju/hg-draft 
> >> #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r d75e65724d2b
> >> histedit: unify strip backup files on success (BC)
> >> 
> >> Previously we wrote two different strip backup files on success. This patch
> >> unifies them. It will make scmutil.cleanupnodes migration more smooth.
> > 
> > Makes sense to me. I'm queuing the series, thanks.
> > 
> > Augie, do you remember if there was a reason to keep them separate?
> 
> The only reason was structural in the code: histedit has “intermediate”
> nodes which are an implementation detail, and “old” nodes, which are the
> precursors to the end state. Internally, the lists are separate because
> you always want to reap “intermediate" nodes, but might want to preserve
> “old” and in --keep or --abort cases.
> 
> It might be nice to keep them separate but have one be marked
> “histedit-internal” and “histedit-before” or something.

Good point.

Going through the "delayedstrip" code path, it could be a bit tricky to
separate backup files [1]. I wish we can count on in-memory-merge work so
those intermediate nodes disappear automatically. But that won't happen
soon. Maybe "delayedstrip" API could be changed, I've added this to my
backlog to resolve.

[1]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-June/100404.html
via Mercurial-devel - July 9, 2017, 8:44 p.m.
On Sun, Jul 9, 2017 at 12:54 PM, Jun Wu <quark@fb.com> wrote:
> Excerpts from Augie Fackler's message of 2017-07-09 15:28:15 -0400:
>>
>> > On Jul 9, 2017, at 2:24 PM, Martin von Zweigbergk <martinvonz@google.com> wrote:
>> >
>> > On Sat, Jul 8, 2017 at 4:51 PM, Jun Wu <quark@fb.com> wrote:
>> >> # HG changeset patch
>> >> # User Jun Wu <quark@fb.com>
>> >> # Date 1499557831 25200
>> >> #      Sat Jul 08 16:50:31 2017 -0700
>> >> # Node ID d75e65724d2bbcf17fcaaad705e463e84a3de5d2
>> >> # Parent  9a325ae88021e0e02a87ef1ae6baa8a199405140
>> >> # Available At https://bitbucket.org/quark-zju/hg-draft
>> >> #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r d75e65724d2b
>> >> histedit: unify strip backup files on success (BC)
>> >>
>> >> Previously we wrote two different strip backup files on success. This patch
>> >> unifies them. It will make scmutil.cleanupnodes migration more smooth.
>> >
>> > Makes sense to me. I'm queuing the series, thanks.
>> >
>> > Augie, do you remember if there was a reason to keep them separate?
>>
>> The only reason was structural in the code: histedit has “intermediate”
>> nodes which are an implementation detail, and “old” nodes, which are the
>> precursors to the end state. Internally, the lists are separate because
>> you always want to reap “intermediate" nodes, but might want to preserve
>> “old” and in --keep or --abort cases.
>>
>> It might be nice to keep them separate but have one be marked
>> “histedit-internal” and “histedit-before” or something.
>
> Good point.
>
> Going through the "delayedstrip" code path, it could be a bit tricky to
> separate backup files [1]. I wish we can count on in-memory-merge work so
> those intermediate nodes disappear automatically. But that won't happen
> soon. Maybe "delayedstrip" API could be changed, I've added this to my
> backlog to resolve.
>
> [1]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-June/100404.html

If we wanted to, we could drop support for the case you in the mention
there (with the AD and BC strips) and make each call to delayedstrip()
result in one backup (with its own name/topic). I think I prefer that
anyway, unless we can think of a concrete example where that case
actually occurs in practice.
Jun Wu - July 9, 2017, 9:32 p.m.
Excerpts from Martin von Zweigbergk's message of 2017-07-09 13:44:26 -0700:
> If we wanted to, we could drop support for the case you in the mention
> there (with the AD and BC strips) and make each call to delayedstrip()
> result in one backup (with its own name/topic). I think I prefer that
> anyway, unless we can think of a concrete example where that case
> actually occurs in practice.

In the split case,
                         B  o
                            |
  B o      B o o A2      A2 o
    |  ->    | |     ->     |
  A o      A x o A1      A1 o
    |        |/             |
  Z o      Z o           Z  o

Rebase calls delayedstrip([B], 'rebase'), and strip calls delayedstrip([A],
'split'). It seems better to put them in a single backup file since B
depends on A. That could reduce bundle size, and the user seems to always
want to restore B and A together.

It seems the only difference we care about is whether the node to strip is
intermediate or not. I guess we could figure that out automatically from a
transaction, or a snapshot of all changelog heads for multiple transaction
case.

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -1172,11 +1172,15 @@  def _finishhistedit(ui, repo, state):
                         ui.debug(m % node.short(n))
 
-    safecleanupnode(ui, repo, tmpnodes)
-
     if not state.keep:
         if mapping:
             movebookmarks(ui, repo, mapping, state.topmost, ntm)
             # TODO update mq state
-        safecleanupnode(ui, repo, mapping)
+    else:
+        mapping = {}
+
+    for n in tmpnodes:
+        mapping[n] = ()
+
+    safecleanupnode(ui, repo, mapping)
 
     state.clear()
diff --git a/tests/test-histedit-arguments.t b/tests/test-histedit-arguments.t
--- a/tests/test-histedit-arguments.t
+++ b/tests/test-histedit-arguments.t
@@ -266,5 +266,4 @@  short hash. This tests issue3893.
   HG: changed alpha
   saved backup bundle to $TESTTMP/foo/.hg/strip-backup/*-backup.hg (glob)
-  saved backup bundle to $TESTTMP/foo/.hg/strip-backup/*-backup.hg (glob)
 
   $ hg update -q 2
diff --git a/tests/test-histedit-bookmark-motion.t b/tests/test-histedit-bookmark-motion.t
--- a/tests/test-histedit-bookmark-motion.t
+++ b/tests/test-histedit-bookmark-motion.t
@@ -88,6 +88,5 @@ 
   > pick 652413bf663e 5 f
   > EOF
-  saved backup bundle to $TESTTMP/r/.hg/strip-backup/96e494a2d553-3c6c5d92-backup.hg (glob)
-  saved backup bundle to $TESTTMP/r/.hg/strip-backup/d2ae7f538514-48787b8d-backup.hg (glob)
+  saved backup bundle to $TESTTMP/r/.hg/strip-backup/96e494a2d553-45c027ab-backup.hg (glob)
   $ hg log --graph
   @  changeset:   3:cacdfd884a93
diff --git a/tests/test-histedit-commute.t b/tests/test-histedit-commute.t
--- a/tests/test-histedit-commute.t
+++ b/tests/test-histedit-commute.t
@@ -419,5 +419,4 @@  Now, let's try to fold the second commit
   $ HGEDITOR="sh ./editor.sh" hg histedit 0
   saved backup bundle to $TESTTMP/issue4251/.hg/strip-backup/*-backup.hg (glob)
-  saved backup bundle to $TESTTMP/issue4251/.hg/strip-backup/*-backup.hg (glob)
 
   $ hg --config diff.git=yes export 0
diff --git a/tests/test-histedit-fold.t b/tests/test-histedit-fold.t
--- a/tests/test-histedit-fold.t
+++ b/tests/test-histedit-fold.t
@@ -383,6 +383,5 @@  dropped revision.
   HG: branch 'default'
   HG: changed file
-  saved backup bundle to $TESTTMP/fold-with-dropped/.hg/strip-backup/55c8d8dc79ce-4066cd98-backup.hg (glob)
-  saved backup bundle to $TESTTMP/fold-with-dropped/.hg/strip-backup/617f94f13c0f-a35700fc-backup.hg (glob)
+  saved backup bundle to $TESTTMP/fold-with-dropped/.hg/strip-backup/617f94f13c0f-3d69522c-backup.hg (glob)
   $ hg logt -G
   @  1:10c647b2cdd5 +4
diff --git a/tests/test-histedit-obsolete.t b/tests/test-histedit-obsolete.t
--- a/tests/test-histedit-obsolete.t
+++ b/tests/test-histedit-obsolete.t
@@ -171,6 +171,4 @@  Base setup for the rest of the testing
   
   $ hg debugobsolete
-  96e494a2d553dd05902ba1cee1d94d4cb7b8faed 0 {b346ab9a313db8537ecf96fca3ca3ca984ef3bd7} (*) {'user': 'test'} (glob)
-  b558abc46d09c30f57ac31e85a8a3d64d2e906e4 0 {96e494a2d553dd05902ba1cee1d94d4cb7b8faed} (*) {'user': 'test'} (glob)
   d2ae7f538514cd87c17547b0de4cea71fe1af9fb 0 {cb9a9f314b8b07ba71012fcdbc544b5a4d82ff5b} (*) {'user': 'test'} (glob)
   177f92b773850b59254aa5e923436f921b55483b b346ab9a313db8537ecf96fca3ca3ca984ef3bd7 0 (*) {'user': 'test'} (glob)
@@ -178,4 +176,6 @@  Base setup for the rest of the testing
   e860deea161a2f77de56603b340ebbb4536308ae 59d9f330561fd6c88b1a6b32f0e45034d88db784 0 (*) {'user': 'test'} (glob)
   652413bf663ef2a641cab26574e46d5f5a64a55a cacdfd884a9321ec4e1de275ef3949fa953a1f83 0 (*) {'user': 'test'} (glob)
+  96e494a2d553dd05902ba1cee1d94d4cb7b8faed 0 {b346ab9a313db8537ecf96fca3ca3ca984ef3bd7} (*) {'user': 'test'} (glob)
+  b558abc46d09c30f57ac31e85a8a3d64d2e906e4 0 {96e494a2d553dd05902ba1cee1d94d4cb7b8faed} (*) {'user': 'test'} (glob)