D2593: state: add logic to parse the state file in old way if cbor fails

[pulkit (Pulkit Goyal)]

pulkit added inline comments.

INLINE COMMENTS

> martinvonz wrote in state.py:84
> > I don't have a strong reason. I just found this an easy way.
> 
> It seems the other way is still slightly easier :) You'd replace this:
> 
>   def __getitem__(self, key):
>       return self.opts[key]
>   
>   def __setitem__(self, key, value):
>       updates = {key: value}
>       self.opts.update(updates)
> 
> by this:
> 
>   def get(self):
>       return self.state
>   
>   def set(self, state):
>       self.state = state
> 
> I'm not even sure we'd need those two methods. It might be enough to make load() return the state and make save() accept (and require) a new state.
> 
> The constructor would change from:
> 
>   if not opts:
>       self.opts = {}
>   else:
>       self.opts = opts
> 
> to:
> 
>   self.state = state
> 
> The graft code for reading would change from:
> 
>   cmdstate.load()
>   if cmdstate['version'] < 2:
>       nodes = cmdstate['nodes']
>       revs = [repo[node].rev() for node in nodes]
>   else:
>       # state-file is written by a newer mercurial than what we are
>       # using
>       raise error.Abort(_("unable to read to read the state file"))
> 
> to:
> 
>   state = cmdstate.load()
>   if state['version'] < 2:
>       nodes = state['nodes']
>       revs = [repo[node].rev() for node in nodes]
>   else:
>       # state-file is written by a newer mercurial than what we are
>       # using
>       raise error.Abort(_("unable to read to read the state file"))
> 
> The graft code for writing would change from:
> 
>   cmdstate.addopts({'version': 1, 'nodes': nodelines})
>   cmdstate.save()
> 
> to:
> 
>   cmdstate.set({'version': 1, 'nodes': nodelines})
>   cmdstate.save()
> 
> 
> 
> > The one benefit is that we can pass the state object everywhere, lookup for values and update values on fly.
> 
> We can still pass the "cmdstate" instance around if we wanted to do that. I think it's a small plus that we can pass *just* the state around, though. That means we can pass the state into a function and we can look at the call site and know that it won't be calling .save() on it.

I don't feel convinced on what can be the benefit of it and my opinion can be influenced by how I used it in evolve extension. But I can change it the other way around too, let me know if you want me to change.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2593

To: pulkit, #hg-reviewers
Cc: martinvonz, indygreg, durin42, mercurial-devel