Patchwork [2,of,3] histedit: make finishfold not depend on parentctx

login
register
mail settings
Submitter Mateusz Kwapich
Date Feb. 5, 2015, 9:59 p.m.
Message ID <9ea70194adf7d075f310.1423173555@dev1429.prn1.facebook.com>
Download mbox | patch
Permalink /patch/7690/
State Changes Requested
Headers show

Comments

Mateusz Kwapich - Feb. 5, 2015, 9:59 p.m.
# HG changeset patch
# User Mateusz Kwapich <mitrandir@fb.com>
# Date 1423170607 28800
#      Thu Feb 05 13:10:07 2015 -0800
# Node ID 9ea70194adf7d075f31044c082f70f7c436a392a
# Parent  d68972b5e29e920571d976fee41a385d4f6baa95
histedit: make finishfold not depend on parentctx

To make histedit more robust to changes between actions, let's make finishfold
not depend on having access to the original parentctx. This has the slight side
effect of not merging the commit dates or phases anymore. Instead we just keep
the date and phase of the original commit, which arguably the right choice in
the beginning since the user is choosing to get rid of the second commit.

Future patches will remove this dependency from other areas as well.
Augie Fackler - Feb. 10, 2015, 6:32 p.m.
On Thu, Feb 05, 2015 at 01:59:15PM -0800, Mateusz Kwapich wrote:
> # HG changeset patch
> # User Mateusz Kwapich <mitrandir@fb.com>
> # Date 1423170607 28800
> #      Thu Feb 05 13:10:07 2015 -0800
> # Node ID 9ea70194adf7d075f31044c082f70f7c436a392a
> # Parent  d68972b5e29e920571d976fee41a385d4f6baa95
> histedit: make finishfold not depend on parentctx
>
> To make histedit more robust to changes between actions, let's make finishfold
> not depend on having access to the original parentctx. This has the slight side
> effect of not merging the commit dates or phases anymore. Instead we just keep
> the date and phase of the original commit, which arguably the right choice in
> the beginning since the user is choosing to get rid of the second commit.

No, the user is folding the two commits together.

Can we do this without dropping the merging of date and phase?

>
> Future patches will remove this dependency from other areas as well.
>
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -397,9 +397,11 @@
>      if n is None:
>          ui.warn(_('%s: empty changeset') % ha[:12])
>          return ctx, []
> -    return finishfold(ui, repo, ctx, oldctx, n, opts, [])
> +    return finishfold(ui, repo, ctx, oldctx.node(), oldctx.description(),
> +                      n, opts, [])
>
> -def finishfold(ui, repo, ctx, oldctx, newnode, opts, internalchanges):
> +def finishfold(ui, repo, ctx, oldctxnode, oldctxdescription, newnode,
> +               opts, internalchanges):
>      parent = ctx.parents()[0].node()
>      hg.update(repo, parent)
>      ### prepare new commit data
> @@ -412,19 +414,19 @@
>          newmessage = '\n***\n'.join(
>              [ctx.description()] +
>              [repo[r].description() for r in internalchanges] +
> -            [oldctx.description()]) + '\n'
> +            [oldctxdescription]) + '\n'
>      commitopts['message'] = newmessage
>      # date
> -    commitopts['date'] = max(ctx.date(), oldctx.date())
> +    commitopts['date'] = ctx.date()
>      extra = ctx.extra().copy()
>      # histedit_source
>      # note: ctx is likely a temporary commit but that the best we can do here
>      #       This is sufficient to solve issue3681 anyway
> -    extra['histedit_source'] = '%s,%s' % (ctx.hex(), oldctx.hex())
> +    extra['histedit_source'] = '%s,%s' % (ctx.hex(), node.hex(oldctxnode))
>      commitopts['extra'] = extra
>      phasebackup = repo.ui.backupconfig('phases', 'new-commit')
>      try:
> -        phasemin = max(ctx.phase(), oldctx.phase())
> +        phasemin = ctx.phase()
>          repo.ui.setconfig('phases', 'new-commit', phasemin, 'histedit')
>          n = collapse(repo, ctx, repo[newnode], commitopts)
>      finally:
> @@ -432,7 +434,7 @@
>      if n is None:
>          return ctx, []
>      hg.update(repo, n)
> -    replacements = [(oldctx.node(), (newnode,)),
> +    replacements = [(oldctxnode, (newnode,)),
>                      (ctx.node(), (n,)),
>                      (newnode, (n,)),
>                     ]
> @@ -779,8 +781,9 @@
>              if action in ('r', 'roll'):
>                  foldopts = foldopts.copy()
>                  foldopts['rollup'] = True
> -            parentctx, repl = finishfold(ui, repo, parentctx, ctx, new,
> -                                         foldopts, newchildren)
> +            parentctx, repl = finishfold(ui, repo, parentctx, ctx.node(),
> +                                         ctx.description(), new, foldopts,
> +                                         newchildren)
>              replacements.extend(repl)
>          else:
>              # newchildren is empty if the fold did not result in any commit
> 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
> @@ -433,7 +433,7 @@
>    $ hg log -G
>    @  19:f9daec13fb98 (secret) i
>    |
> -  o  18:49807617f46a (secret) g
> +  o  18:49807617f46a (draft) g
>    |
>    o  17:050280826e04 (draft) h
>    |
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Durham Goode - Feb. 11, 2015, 5:09 a.m.
On 2/10/15 10:32 AM, Augie Fackler wrote:
> On Thu, Feb 05, 2015 at 01:59:15PM -0800, Mateusz Kwapich wrote:
>> # HG changeset patch
>> # User Mateusz Kwapich <mitrandir@fb.com>
>> # Date 1423170607 28800
>> #      Thu Feb 05 13:10:07 2015 -0800
>> # Node ID 9ea70194adf7d075f31044c082f70f7c436a392a
>> # Parent  d68972b5e29e920571d976fee41a385d4f6baa95
>> histedit: make finishfold not depend on parentctx
>>
>> To make histedit more robust to changes between actions, let's make finishfold
>> not depend on having access to the original parentctx. This has the slight side
>> effect of not merging the commit dates or phases anymore. Instead we just keep
>> the date and phase of the original commit, which arguably the right choice in
>> the beginning since the user is choosing to get rid of the second commit.
> No, the user is folding the two commits together.
>
> Can we do this without dropping the merging of date and phase?
>
Removing the dependency on the parentctx without dropping that state 
would require storing that data in the histedit state, which requires a 
format change.  Personally I don't think the date and phase are 
important here.  We're making an arbitrary choice already, so some other 
arbitrary choice is about as good as the first.

However, now that we're preventing strips in the middle of the histedit, 
patches 2 and 3 here become less important (since the parentctx should 
always be valid).  The initial goal was to prevent hg histedit --abort 
from crashing, and we can do that gracefully now that patch 1 is in, 
without completely breaking the dependency on parentctx.

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -397,9 +397,11 @@ 
     if n is None:
         ui.warn(_('%s: empty changeset') % ha[:12])
         return ctx, []
-    return finishfold(ui, repo, ctx, oldctx, n, opts, [])
+    return finishfold(ui, repo, ctx, oldctx.node(), oldctx.description(),
+                      n, opts, [])
 
-def finishfold(ui, repo, ctx, oldctx, newnode, opts, internalchanges):
+def finishfold(ui, repo, ctx, oldctxnode, oldctxdescription, newnode,
+               opts, internalchanges):
     parent = ctx.parents()[0].node()
     hg.update(repo, parent)
     ### prepare new commit data
@@ -412,19 +414,19 @@ 
         newmessage = '\n***\n'.join(
             [ctx.description()] +
             [repo[r].description() for r in internalchanges] +
-            [oldctx.description()]) + '\n'
+            [oldctxdescription]) + '\n'
     commitopts['message'] = newmessage
     # date
-    commitopts['date'] = max(ctx.date(), oldctx.date())
+    commitopts['date'] = ctx.date()
     extra = ctx.extra().copy()
     # histedit_source
     # note: ctx is likely a temporary commit but that the best we can do here
     #       This is sufficient to solve issue3681 anyway
-    extra['histedit_source'] = '%s,%s' % (ctx.hex(), oldctx.hex())
+    extra['histedit_source'] = '%s,%s' % (ctx.hex(), node.hex(oldctxnode))
     commitopts['extra'] = extra
     phasebackup = repo.ui.backupconfig('phases', 'new-commit')
     try:
-        phasemin = max(ctx.phase(), oldctx.phase())
+        phasemin = ctx.phase()
         repo.ui.setconfig('phases', 'new-commit', phasemin, 'histedit')
         n = collapse(repo, ctx, repo[newnode], commitopts)
     finally:
@@ -432,7 +434,7 @@ 
     if n is None:
         return ctx, []
     hg.update(repo, n)
-    replacements = [(oldctx.node(), (newnode,)),
+    replacements = [(oldctxnode, (newnode,)),
                     (ctx.node(), (n,)),
                     (newnode, (n,)),
                    ]
@@ -779,8 +781,9 @@ 
             if action in ('r', 'roll'):
                 foldopts = foldopts.copy()
                 foldopts['rollup'] = True
-            parentctx, repl = finishfold(ui, repo, parentctx, ctx, new,
-                                         foldopts, newchildren)
+            parentctx, repl = finishfold(ui, repo, parentctx, ctx.node(),
+                                         ctx.description(), new, foldopts,
+                                         newchildren)
             replacements.extend(repl)
         else:
             # newchildren is empty if the fold did not result in any commit
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
@@ -433,7 +433,7 @@ 
   $ hg log -G
   @  19:f9daec13fb98 (secret) i
   |
-  o  18:49807617f46a (secret) g
+  o  18:49807617f46a (draft) g
   |
   o  17:050280826e04 (draft) h
   |