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

[martinvonz (Martin von Zweigbergk)]

martinvonz added inline comments.

INLINE COMMENTS

> pulkit wrote in state.py:84
> I don't have a strong reason. I just found this an easy way. I was going to add __setitem__() too so that we can store more new values or change existing values.
> 
> For graft case, it won't be just a list of nodes, we are going to store info about `--no-commit` flag in graftstate. Moreover we need `graft --abort` too which needs to store more things.
> 
> The one benefit is that we can pass the state object everywhere, lookup for values and update values on fly.

> 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.

REPOSITORY
  rHG Mercurial

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

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