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

# 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 627b344d81f64444216dc0ac7cfc33d63d7fcdff
# Parent  0e50f872225ac6082796871f236c05c2e51a4d25
rebase: handle revtodo as a special value when storing/restoring state

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

On 12/02/2014 09:32 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 627b344d81f64444216dc0ac7cfc33d63d7fcdff
> # Parent  0e50f872225ac6082796871f236c05c2e51a4d25
> rebase: handle revtodo as a special value when storing/restoring state
>
> Revtodo happen to share its value with nullrev but this is an implementation
> details. So we move away from it.
>
> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -728,11 +728,11 @@ 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()
>           else:
>               newrev = v
>           f.write("%s:%s\n" % (oldrev, newrev))

This will change the file format from storing revtodo as '0'*40 to '-1'. 
Is that intentional and a good idea? AFAICS that means that old hg will 
fail to read a new rebasestate file correctly. I don't think that is OK.

You can make revtodo independent of nullrev if you want, but I think you 
still have to store it as hex(nullid). I thus don't see much point in 
using a different constant than nullrev.

/Mads

>       f.close()
> @@ -769,11 +769,11 @@ def restorestatus(repo):
>                   # line 6 is a recent addition, so for backwards compatibility
>                   # check that the line doesn't look like the oldrev:newrev lines
>                   activebookmark = l
>               else:
>                   oldrev, newrev = l.split(':')
> -                if newrev in (str(nullmerge), str(revignored)):
> +                if newrev in (str(revtodo), str(nullmerge), str(revignored)):
>                       state[repo[oldrev].rev()] = int(newrev)
>                   else:
>                       state[repo[oldrev].rev()] = repo[newrev].rev()
>   
>           if keepbranches is None:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

On 12/02/2014 05:03 PM, Mads Kiilerich wrote:
> On 12/02/2014 09:32 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 627b344d81f64444216dc0ac7cfc33d63d7fcdff
>> # Parent  0e50f872225ac6082796871f236c05c2e51a4d25
>> rebase: handle revtodo as a special value when storing/restoring state
>>
>> Revtodo happen to share its value with nullrev but this is an
>> implementation
>> details. So we move away from it.
>>
>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>> --- a/hgext/rebase.py
>> +++ b/hgext/rebase.py
>> @@ -728,11 +728,11 @@ 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()
>>           else:
>>               newrev = v
>>           f.write("%s:%s\n" % (oldrev, newrev))
>
> This will change the file format from storing revtodo as '0'*40 to '-1'.
> Is that intentional and a good idea? AFAICS that means that old hg will
> fail to read a new rebasestate file correctly. I don't think that is OK.

I would like to avoid breaking compat if it is easy to do so. I've added 
a special case to store it as nullid.



>
> You can make revtodo independent of nullrev if you want, but I think you
> still have to store it as hex(nullid). I thus don't see much point in
> using a different constant than nullrev.

The goal is to make the code clearer. Calling different thing with 
different name is a good way to achieve that. The on disk format isn't 
too relevant.