Patchwork [3,of,3,V2] rebase: handle revtodo as a special value when storing/restoring state

login
register
mail settings
Submitter Pierre-Yves David
Date Dec. 4, 2014, 3:29 p.m.
Message ID <e3b289dd518b326d927f.1417706997@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/7005/
State Accepted
Headers show

Comments

Pierre-Yves David - Dec. 4, 2014, 3:29 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1417551792 28800
#      Tue Dec 02 12:23:12 2014 -0800
# Node ID e3b289dd518b326d927f1ec6b61f025a6c74c12a
# Parent  f1632814e8d3733acec372dcc17620ab0a8617bf
rebase: handle revtodo as a special value when storing/restoring state

Revtodo happens to share its value with nullrev but this is an implementation
details. So we move away from it.

After this changeset one can successfully change the values for all the constant
and have the test passing. But doing so would require more refactoring if we
want to avoid breaking backward compatibility on the state file.
Mads Kiilerich - Dec. 4, 2014, 4:06 p.m.
On 12/04/2014 04:29 PM, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1417551792 28800
> #      Tue Dec 02 12:23:12 2014 -0800
> # Node ID e3b289dd518b326d927f1ec6b61f025a6c74c12a
> # Parent  f1632814e8d3733acec372dcc17620ab0a8617bf
> rebase: handle revtodo as a special value when storing/restoring state
>
> Revtodo happens to share its value with nullrev but this is an implementation
> details. So we move away from it.
>
> After this changeset one can successfully change the values for all the constant
> and have the test passing. But doing so would require more refactoring if we
> want to avoid breaking backward compatibility on the state file.
>
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -16,11 +16,11 @@ http://mercurial.selenic.com/wiki/Rebase
>   
>   from mercurial import hg, util, repair, merge, cmdutil, commands, bookmarks
>   from mercurial import extensions, patch, scmutil, phases, obsolete, error
>   from mercurial import copies
>   from mercurial.commands import templateopts
> -from mercurial.node import nullrev
> +from mercurial.node import nullrev, nullid, hex
>   from mercurial.lock import release
>   from mercurial.i18n import _
>   import os, errno
>   
>   revtodo = -1
> @@ -728,12 +728,16 @@ def storestatus(repo, originalwd, target
>       f.write('%d\n' % int(keep))
>       f.write('%d\n' % int(keepbranches))
>       f.write('%s\n' % (activebookmark or ''))
>       for d, v in state.iteritems():
>           oldrev = repo[d].hex()
> -        if v > nullmerge:
> +        if v >= 0:
>               newrev = repo[v].hex()
> +        elif v == revtodo:
> +            # To maintain format compatibility, we have to use nullid.
> +            # Please do remove this special case when upgrading the format.
> +            newrev = hex(nullid)
>           else:
>               newrev = v
>           f.write("%s:%s\n" % (oldrev, newrev))
>       f.close()
>       repo.ui.debug('rebase status stored\n')
> @@ -771,10 +775,13 @@ def restorestatus(repo):
>                   activebookmark = l
>               else:
>                   oldrev, newrev = l.split(':')
>                   if newrev in (str(nullmerge), str(revignored)):
>                       state[repo[oldrev].rev()] = int(newrev)
> +                elif newrev == nullid:

I guess that should be newrev == hex(nullid)?

Besides that, LGTM.

/Mads
Pierre-Yves David - Dec. 5, 2014, 12:44 a.m.
On 12/04/2014 08:06 AM, Mads Kiilerich wrote:
> On 12/04/2014 04:29 PM, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1417551792 28800
>> #      Tue Dec 02 12:23:12 2014 -0800
>> # Node ID e3b289dd518b326d927f1ec6b61f025a6c74c12a
>> # Parent  f1632814e8d3733acec372dcc17620ab0a8617bf
>> rebase: handle revtodo as a special value when storing/restoring state
>>
>> Revtodo happens to share its value with nullrev but this is an
>> implementation
>> details. So we move away from it.
>>
>> After this changeset one can successfully change the values for all
>> the constant
>> and have the test passing. But doing so would require more refactoring
>> if we
>> want to avoid breaking backward compatibility on the state file.
>>
>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>> --- a/hgext/rebase.py
>> +++ b/hgext/rebase.py
>> @@ -16,11 +16,11 @@ http://mercurial.selenic.com/wiki/Rebase
>>   from mercurial import hg, util, repair, merge, cmdutil, commands,
>> bookmarks
>>   from mercurial import extensions, patch, scmutil, phases, obsolete,
>> error
>>   from mercurial import copies
>>   from mercurial.commands import templateopts
>> -from mercurial.node import nullrev
>> +from mercurial.node import nullrev, nullid, hex
>>   from mercurial.lock import release
>>   from mercurial.i18n import _
>>   import os, errno
>>   revtodo = -1
>> @@ -728,12 +728,16 @@ def storestatus(repo, originalwd, target
>>       f.write('%d\n' % int(keep))
>>       f.write('%d\n' % int(keepbranches))
>>       f.write('%s\n' % (activebookmark or ''))
>>       for d, v in state.iteritems():
>>           oldrev = repo[d].hex()
>> -        if v > nullmerge:
>> +        if v >= 0:
>>               newrev = repo[v].hex()
>> +        elif v == revtodo:
>> +            # To maintain format compatibility, we have to use nullid.
>> +            # Please do remove this special case when upgrading the
>> format.
>> +            newrev = hex(nullid)
>>           else:
>>               newrev = v
>>           f.write("%s:%s\n" % (oldrev, newrev))
>>       f.close()
>>       repo.ui.debug('rebase status stored\n')
>> @@ -771,10 +775,13 @@ def restorestatus(repo):
>>                   activebookmark = l
>>               else:
>>                   oldrev, newrev = l.split(':')
>>                   if newrev in (str(nullmerge), str(revignored)):
>>                       state[repo[oldrev].rev()] = int(newrev)
>> +                elif newrev == nullid:
>
> I guess that should be newrev == hex(nullid)?

Indeed. I think I need a "no uncommitted change before patchbomb" check :-/.
(Note that is keeps working because revtodo is still -1, so the 
'repo[hex(nullid)].rev()' call eventually return -1)

I can send a V3 is reviewer request do not want to fix this inflight.
Mads Kiilerich - Dec. 5, 2014, 12:48 a.m.
On 12/05/2014 01:44 AM, Pierre-Yves David wrote:
>
>
> On 12/04/2014 08:06 AM, Mads Kiilerich wrote:
>> On 12/04/2014 04:29 PM, Pierre-Yves David wrote:
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>> # Date 1417551792 28800
>>> #      Tue Dec 02 12:23:12 2014 -0800
>>> # Node ID e3b289dd518b326d927f1ec6b61f025a6c74c12a
>>> # Parent  f1632814e8d3733acec372dcc17620ab0a8617bf
>>> rebase: handle revtodo as a special value when storing/restoring state
>>>
>>> Revtodo happens to share its value with nullrev but this is an
>>> implementation
>>> details. So we move away from it.
>>>
>>> After this changeset one can successfully change the values for all
>>> the constant
>>> and have the test passing. But doing so would require more refactoring
>>> if we
>>> want to avoid breaking backward compatibility on the state file.
>>>
>>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>>> --- a/hgext/rebase.py
>>> +++ b/hgext/rebase.py
>>> @@ -16,11 +16,11 @@ http://mercurial.selenic.com/wiki/Rebase
>>>   from mercurial import hg, util, repair, merge, cmdutil, commands,
>>> bookmarks
>>>   from mercurial import extensions, patch, scmutil, phases, obsolete,
>>> error
>>>   from mercurial import copies
>>>   from mercurial.commands import templateopts
>>> -from mercurial.node import nullrev
>>> +from mercurial.node import nullrev, nullid, hex
>>>   from mercurial.lock import release
>>>   from mercurial.i18n import _
>>>   import os, errno
>>>   revtodo = -1
>>> @@ -728,12 +728,16 @@ def storestatus(repo, originalwd, target
>>>       f.write('%d\n' % int(keep))
>>>       f.write('%d\n' % int(keepbranches))
>>>       f.write('%s\n' % (activebookmark or ''))
>>>       for d, v in state.iteritems():
>>>           oldrev = repo[d].hex()
>>> -        if v > nullmerge:
>>> +        if v >= 0:
>>>               newrev = repo[v].hex()
>>> +        elif v == revtodo:
>>> +            # To maintain format compatibility, we have to use nullid.
>>> +            # Please do remove this special case when upgrading the
>>> format.
>>> +            newrev = hex(nullid)
>>>           else:
>>>               newrev = v
>>>           f.write("%s:%s\n" % (oldrev, newrev))
>>>       f.close()
>>>       repo.ui.debug('rebase status stored\n')
>>> @@ -771,10 +775,13 @@ def restorestatus(repo):
>>>                   activebookmark = l
>>>               else:
>>>                   oldrev, newrev = l.split(':')
>>>                   if newrev in (str(nullmerge), str(revignored)):
>>>                       state[repo[oldrev].rev()] = int(newrev)
>>> +                elif newrev == nullid:
>>
>> I guess that should be newrev == hex(nullid)?
>
> Indeed. I think I need a "no uncommitted change before patchbomb" 
> check :-/.

We kind of have that - http://selenic.com/repo/hg/rev/dbff8c119cf6 .

> (Note that is keeps working because revtodo is still -1, so the 
> 'repo[hex(nullid)].rev()' call eventually return -1)

I agree. It is only relevant to get the full decoupling from nullrev 
that you wanted.

/Mads
Augie Fackler - Dec. 5, 2014, 9:22 p.m.
On Thu, Dec 04, 2014 at 07:29:57AM -0800, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1417551792 28800
> #      Tue Dec 02 12:23:12 2014 -0800
> # Node ID e3b289dd518b326d927f1ec6b61f025a6c74c12a
> # Parent  f1632814e8d3733acec372dcc17620ab0a8617bf
> rebase: handle revtodo as a special value when storing/restoring state
>

queued this one (don't know if I mentioned this)

>
> Revtodo happens to share its value with nullrev but this is an implementation
> details. So we move away from it.
>
> After this changeset one can successfully change the values for all the constant
> and have the test passing. But doing so would require more refactoring if we
> want to avoid breaking backward compatibility on the state file.
>
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -16,11 +16,11 @@ http://mercurial.selenic.com/wiki/Rebase
>
>  from mercurial import hg, util, repair, merge, cmdutil, commands, bookmarks
>  from mercurial import extensions, patch, scmutil, phases, obsolete, error
>  from mercurial import copies
>  from mercurial.commands import templateopts
> -from mercurial.node import nullrev
> +from mercurial.node import nullrev, nullid, hex
>  from mercurial.lock import release
>  from mercurial.i18n import _
>  import os, errno
>
>  revtodo = -1
> @@ -728,12 +728,16 @@ def storestatus(repo, originalwd, target
>      f.write('%d\n' % int(keep))
>      f.write('%d\n' % int(keepbranches))
>      f.write('%s\n' % (activebookmark or ''))
>      for d, v in state.iteritems():
>          oldrev = repo[d].hex()
> -        if v > nullmerge:
> +        if v >= 0:
>              newrev = repo[v].hex()
> +        elif v == revtodo:
> +            # To maintain format compatibility, we have to use nullid.
> +            # Please do remove this special case when upgrading the format.
> +            newrev = hex(nullid)
>          else:
>              newrev = v
>          f.write("%s:%s\n" % (oldrev, newrev))
>      f.close()
>      repo.ui.debug('rebase status stored\n')
> @@ -771,10 +775,13 @@ def restorestatus(repo):
>                  activebookmark = l
>              else:
>                  oldrev, newrev = l.split(':')
>                  if newrev in (str(nullmerge), str(revignored)):
>                      state[repo[oldrev].rev()] = int(newrev)
> +                elif newrev == nullid:
> +                    state[repo[oldrev].rev()] = revtodo
> +                    # Legacy compat special case
>                  else:
>                      state[repo[oldrev].rev()] = repo[newrev].rev()
>
>          if keepbranches is None:
>              raise util.Abort(_('.hg/rebasestate is incomplete'))
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -16,11 +16,11 @@  http://mercurial.selenic.com/wiki/Rebase
 
 from mercurial import hg, util, repair, merge, cmdutil, commands, bookmarks
 from mercurial import extensions, patch, scmutil, phases, obsolete, error
 from mercurial import copies
 from mercurial.commands import templateopts
-from mercurial.node import nullrev
+from mercurial.node import nullrev, nullid, hex
 from mercurial.lock import release
 from mercurial.i18n import _
 import os, errno
 
 revtodo = -1
@@ -728,12 +728,16 @@  def storestatus(repo, originalwd, target
     f.write('%d\n' % int(keep))
     f.write('%d\n' % int(keepbranches))
     f.write('%s\n' % (activebookmark or ''))
     for d, v in state.iteritems():
         oldrev = repo[d].hex()
-        if v > nullmerge:
+        if v >= 0:
             newrev = repo[v].hex()
+        elif v == revtodo:
+            # To maintain format compatibility, we have to use nullid.
+            # Please do remove this special case when upgrading the format.
+            newrev = hex(nullid)
         else:
             newrev = v
         f.write("%s:%s\n" % (oldrev, newrev))
     f.close()
     repo.ui.debug('rebase status stored\n')
@@ -771,10 +775,13 @@  def restorestatus(repo):
                 activebookmark = l
             else:
                 oldrev, newrev = l.split(':')
                 if newrev in (str(nullmerge), str(revignored)):
                     state[repo[oldrev].rev()] = int(newrev)
+                elif newrev == nullid:
+                    state[repo[oldrev].rev()] = revtodo
+                    # Legacy compat special case
                 else:
                     state[repo[oldrev].rev()] = repo[newrev].rev()
 
         if keepbranches is None:
             raise util.Abort(_('.hg/rebasestate is incomplete'))