Patchwork histedit: don't allow to strip nodes which are necessary to continue histedit

login
register
mail settings
Submitter Mateusz Kwapich
Date Feb. 6, 2015, 1:56 a.m.
Message ID <1e028495b7e6e34f1be1.1423187776@dev1429.prn1.facebook.com>
Download mbox | patch
Permalink /patch/7699/
State Accepted
Headers show

Comments

Mateusz Kwapich - Feb. 6, 2015, 1:56 a.m.
# HG changeset patch
# User Mateusz Kwapich <mitrandir@fb.com>
# Date 1422665255 28800
#      Fri Jan 30 16:47:35 2015 -0800
# Node ID 1e028495b7e6e34f1be12e3c78ff50165e9810b7
# Parent  c53bc2e52514825cbd40d29dd08693c8a640256e
histedit: don't allow to strip nodes which are necessary to continue histedit

During histedit we don't want user to do any operation resulting in
stripping nodes needed to continue history editing. This patch
wraps the strip function to detect such situations.
Augie Fackler - Feb. 9, 2015, 11:52 p.m.
On Thu, Feb 05, 2015 at 05:56:16PM -0800, Mateusz Kwapich wrote:
> # HG changeset patch
> # User Mateusz Kwapich <mitrandir@fb.com>
> # Date 1422665255 28800
> #      Fri Jan 30 16:47:35 2015 -0800
> # Node ID 1e028495b7e6e34f1be12e3c78ff50165e9810b7
> # Parent  c53bc2e52514825cbd40d29dd08693c8a640256e
> histedit: don't allow to strip nodes which are necessary to continue histedit

Queued, because this is obviously correct.

That said, should we be blocking strip at all points when you have to
run a hg $SOMETHING --continue? It might mess with rebase to do strip
mid-rebase (if you had to resolve conflicts, for example).

>
> During histedit we don't want user to do any operation resulting in
> stripping nodes needed to continue history editing. This patch
> wraps the strip function to detect such situations.
>
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -158,6 +158,7 @@
>  from mercurial import error
>  from mercurial import copies
>  from mercurial import context
> +from mercurial import extensions
>  from mercurial import hg
>  from mercurial import node
>  from mercurial import repair
> @@ -674,6 +675,7 @@
>          actfunc = actiontable[action]
>          state.parentctx, replacement_ = actfunc(ui, state, ha, opts)
>          state.replacements.extend(replacement_)
> +    state.write()
>
>      hg.update(repo, state.parentctx.node())
>
> @@ -971,6 +973,22 @@
>      finally:
>          release(lock)
>
> +def stripwrapper(orig, ui, repo, nodelist, *args, **kwargs):
> +    if isinstance(nodelist, str):
> +        nodelist = [nodelist]
> +    if os.path.exists(os.path.join(repo.path, 'histedit-state')):
> +        state = histeditstate(repo)
> +        state.read()
> +        histedit_nodes= set([ctx for (action, ctx) in state.rules])
> +        strip_nodes = set([str(repo[n]) for n in nodelist])
> +        common_nodes = histedit_nodes & strip_nodes
> +        if common_nodes:
> +            raise util.Abort(_('unable to strip %s. Nodes are '
> +                               'used by history edit in progress.'% ', '.join(common_nodes)))
> +    return orig(ui, repo, nodelist, *args, **kwargs)
> +
> +extensions.wrapfunction(repair, 'strip', stripwrapper)
> +
>  def summaryhook(ui, repo):
>      if not os.path.exists(repo.join('histedit-state')):
>          return
> diff --git a/tests/test-histedit-edit.t b/tests/test-histedit-edit.t
> --- a/tests/test-histedit-edit.t
> +++ b/tests/test-histedit-edit.t
> @@ -3,6 +3,7 @@
>    $ cat >> $HGRCPATH <<EOF
>    > [extensions]
>    > histedit=
> +  > strip=
>    > EOF
>
>    $ initrepo ()
> @@ -72,6 +73,11 @@
>    (use 'hg histedit --continue' or 'hg histedit --abort')
>    [255]
>
> +Try to delete necessary commit
> +  $ hg strip -r 652413bf663e
> +  abort: unable to strip 652413bf663e. Nodes are used by history edit in progress.
> +  [255]
> +
>  commit, then edit the revision
>    $ hg ci -m 'wat'
>    created new head
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Augie Fackler - Feb. 10, 2015, 3:35 p.m.
On Feb 9, 2015, at 6:52 PM, Augie Fackler <raf@durin42.com> wrote:

> On Thu, Feb 05, 2015 at 05:56:16PM -0800, Mateusz Kwapich wrote:
>> # HG changeset patch
>> # User Mateusz Kwapich <mitrandir@fb.com>
>> # Date 1422665255 28800
>> #      Fri Jan 30 16:47:35 2015 -0800
>> # Node ID 1e028495b7e6e34f1be12e3c78ff50165e9810b7
>> # Parent  c53bc2e52514825cbd40d29dd08693c8a640256e
>> histedit: don't allow to strip nodes which are necessary to continue histedit
> 
> Queued, because this is obviously correct.

Also, check-code wants you to be its valentine.

> 
> That said, should we be blocking strip at all points when you have to
> run a hg $SOMETHING --continue? It might mess with rebase to do strip
> mid-rebase (if you had to resolve conflicts, for example).
> 
>> 
>> During histedit we don't want user to do any operation resulting in
>> stripping nodes needed to continue history editing. This patch
>> wraps the strip function to detect such situations.
>> 
>> diff --git a/hgext/histedit.py b/hgext/histedit.py
>> --- a/hgext/histedit.py
>> +++ b/hgext/histedit.py
>> @@ -158,6 +158,7 @@
>> from mercurial import error
>> from mercurial import copies
>> from mercurial import context
>> +from mercurial import extensions
>> from mercurial import hg
>> from mercurial import node
>> from mercurial import repair
>> @@ -674,6 +675,7 @@
>>         actfunc = actiontable[action]
>>         state.parentctx, replacement_ = actfunc(ui, state, ha, opts)
>>         state.replacements.extend(replacement_)
>> +    state.write()
>> 
>>     hg.update(repo, state.parentctx.node())
>> 
>> @@ -971,6 +973,22 @@
>>     finally:
>>         release(lock)
>> 
>> +def stripwrapper(orig, ui, repo, nodelist, *args, **kwargs):
>> +    if isinstance(nodelist, str):
>> +        nodelist = [nodelist]
>> +    if os.path.exists(os.path.join(repo.path, 'histedit-state')):
>> +        state = histeditstate(repo)
>> +        state.read()
>> +        histedit_nodes= set([ctx for (action, ctx) in state.rules])
>> +        strip_nodes = set([str(repo[n]) for n in nodelist])
>> +        common_nodes = histedit_nodes & strip_nodes
>> +        if common_nodes:
>> +            raise util.Abort(_('unable to strip %s. Nodes are '
>> +                               'used by history edit in progress.'% ', '.join(common_nodes)))
>> +    return orig(ui, repo, nodelist, *args, **kwargs)
>> +
>> +extensions.wrapfunction(repair, 'strip', stripwrapper)
>> +
>> def summaryhook(ui, repo):
>>     if not os.path.exists(repo.join('histedit-state')):
>>         return
>> diff --git a/tests/test-histedit-edit.t b/tests/test-histedit-edit.t
>> --- a/tests/test-histedit-edit.t
>> +++ b/tests/test-histedit-edit.t
>> @@ -3,6 +3,7 @@
>>   $ cat >> $HGRCPATH <<EOF
>>> [extensions]
>>> histedit=
>> +  > strip=
>>> EOF
>> 
>>   $ initrepo ()
>> @@ -72,6 +73,11 @@
>>   (use 'hg histedit --continue' or 'hg histedit --abort')
>>   [255]
>> 
>> +Try to delete necessary commit
>> +  $ hg strip -r 652413bf663e
>> +  abort: unable to strip 652413bf663e. Nodes are used by history edit in progress.
>> +  [255]
>> +
>> commit, then edit the revision
>>   $ hg ci -m 'wat'
>>   created new head
>> _______________________________________________
>> 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
Pierre-Yves David - Feb. 25, 2015, 2:24 p.m.
On 02/10/2015 03:35 PM, Augie Fackler wrote:
> On Feb 9, 2015, at 6:52 PM, Augie Fackler <raf@durin42.com
> <mailto:raf@durin42.com>> wrote:
>
>> On Thu, Feb 05, 2015 at 05:56:16PM -0800, Mateusz Kwapich wrote:
>>> # HG changeset patch
>>> # User Mateusz Kwapich <mitrandir@fb.com <mailto:mitrandir@fb.com>>
>>> # Date 1422665255 28800
>>> #      Fri Jan 30 16:47:35 2015 -0800
>>> # Node ID 1e028495b7e6e34f1be12e3c78ff50165e9810b7
>>> # Parent  c53bc2e52514825cbd40d29dd08693c8a640256e
>>> histedit: don't allow to strip nodes which are necessary to continue
>>> histedit
>>
>> Queued, because this is obviously correct.
>
> Also, check-code wants you to be its valentine.

The change that landed in the crew repo (0c1cf9f6b1be) have a suspicious 
unrelated changes to :

   mercurial/templates/static/style-paper.css

Augie, did you too eagerly amend while fixing checkcode issue ?
Augie Fackler - Feb. 25, 2015, 2:50 p.m.
Very likely. I'll look into this in a minute.

On Wed, Feb 25, 2015 at 9:24 AM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 02/10/2015 03:35 PM, Augie Fackler wrote:
>>
>> On Feb 9, 2015, at 6:52 PM, Augie Fackler <raf@durin42.com
>> <mailto:raf@durin42.com>> wrote:
>>
>>> On Thu, Feb 05, 2015 at 05:56:16PM -0800, Mateusz Kwapich wrote:
>>>>
>>>> # HG changeset patch
>>>> # User Mateusz Kwapich <mitrandir@fb.com <mailto:mitrandir@fb.com>>
>>>> # Date 1422665255 28800
>>>> #      Fri Jan 30 16:47:35 2015 -0800
>>>> # Node ID 1e028495b7e6e34f1be12e3c78ff50165e9810b7
>>>> # Parent  c53bc2e52514825cbd40d29dd08693c8a640256e
>>>> histedit: don't allow to strip nodes which are necessary to continue
>>>> histedit
>>>
>>>
>>> Queued, because this is obviously correct.
>>
>>
>> Also, check-code wants you to be its valentine.
>
>
> The change that landed in the crew repo (0c1cf9f6b1be) have a suspicious
> unrelated changes to :
>
>   mercurial/templates/static/style-paper.css
>
> Augie, did you too eagerly amend while fixing checkcode issue ?
>
> --
> Pierre-Yves David
Augie Fackler - Feb. 27, 2015, 5:17 p.m.
On Feb 25, 2015, at 9:24 AM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:

> The change that landed in the crew repo (0c1cf9f6b1be) have a suspicious unrelated changes to :
> 
>  mercurial/templates/static/style-paper.css
> 
> Augie, did you too eagerly amend while fixing checkcode issue ?

I did, and it's fixed in crew as of sometime a couple of days ago, I just forgot to respond to the mail.

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -158,6 +158,7 @@ 
 from mercurial import error
 from mercurial import copies
 from mercurial import context
+from mercurial import extensions
 from mercurial import hg
 from mercurial import node
 from mercurial import repair
@@ -674,6 +675,7 @@ 
         actfunc = actiontable[action]
         state.parentctx, replacement_ = actfunc(ui, state, ha, opts)
         state.replacements.extend(replacement_)
+    state.write()
 
     hg.update(repo, state.parentctx.node())
 
@@ -971,6 +973,22 @@ 
     finally:
         release(lock)
 
+def stripwrapper(orig, ui, repo, nodelist, *args, **kwargs):
+    if isinstance(nodelist, str):
+        nodelist = [nodelist]
+    if os.path.exists(os.path.join(repo.path, 'histedit-state')):
+        state = histeditstate(repo)
+        state.read()
+        histedit_nodes= set([ctx for (action, ctx) in state.rules])
+        strip_nodes = set([str(repo[n]) for n in nodelist])
+        common_nodes = histedit_nodes & strip_nodes
+        if common_nodes:
+            raise util.Abort(_('unable to strip %s. Nodes are '
+                               'used by history edit in progress.'% ', '.join(common_nodes)))
+    return orig(ui, repo, nodelist, *args, **kwargs)
+
+extensions.wrapfunction(repair, 'strip', stripwrapper)
+
 def summaryhook(ui, repo):
     if not os.path.exists(repo.join('histedit-state')):
         return
diff --git a/tests/test-histedit-edit.t b/tests/test-histedit-edit.t
--- a/tests/test-histedit-edit.t
+++ b/tests/test-histedit-edit.t
@@ -3,6 +3,7 @@ 
   $ cat >> $HGRCPATH <<EOF
   > [extensions]
   > histedit=
+  > strip=
   > EOF
 
   $ initrepo ()
@@ -72,6 +73,11 @@ 
   (use 'hg histedit --continue' or 'hg histedit --abort')
   [255]
 
+Try to delete necessary commit
+  $ hg strip -r 652413bf663e
+  abort: unable to strip 652413bf663e. Nodes are used by history edit in progress.
+  [255]
+
 commit, then edit the revision
   $ hg ci -m 'wat'
   created new head