I am not confident about this part. Like to have some suggestions here.

Should I change thirdparty/cbor/ to raise specific errors?

Probably treat not-a-dict as corrupt and fall back to the other format?

Ugh. Yeah, maybe see if they'd take a patch upstream to raise a more explicit exception type.

@durin42 I feel sorry about adding a TODO but I did that because I won't be able to work for a week or so dur to laptop issue. If you are not okay with that, I am fine with these lying here.

bad ident

How about not requiring it to be a dict? I imagine practically all callers will want to pass a dict, but why does this class have to enforce it? I think the API would be simpler if it was an opaque object. In the simple case of graft, the state is simply a list of nodes. However, for more complex states, we could have nested structures. I don't see why cmdstate should be involved in lookups into the top-level structure. The difference is subtle, but here's an example:

# With dict-aware cmdstate
cmdstate = ...
cmdstate.load()
version = cmdstate['version']
for car in cmdstate['cars']:
   for wheel in car['wheels']:
       # whatever

# With agnostic cmdstate
cmdstate = ...
parking = cmdstate.load()
version = parking['version']
for car in parking['cars']:
   for wheel in car['wheels']:
       # whatever

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.

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.

If it's up to me, I'd definitely say that you should change it. I don't see any benefits of the current proposal. I'd be happy to hear what other reviewers think.

Oh, and I should clarify that I think the root of the problem is that you're mixing the class for reading with state itself (but only the top-level items of it). Let's say someone realizes they want to iterate over the items in the state, would we also add a .items() then? (I don't think that's a likely scenario, but it shows what I mean.)

Sorry if it's already discussed, but what's the benefit of cmdstate
not being a plain dict?

I think plain load/save functions are much simpler than a dict-like object
backed by file, i.e.

state = statemod.load(repo, fname)
state = {'version': 1, 'nodes': nodelines}
statemod.save(repo, fname, state)

(statemod.load/save could be a class for serialization/deserialization)

The state dict would be wrapped anyway depending on application logic.
If we can find out a common behavior between "state"s, we can extract it to a state class,
but that will still be separated from the storage format.