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

pulkit abandoned this revision. pulkit added a comment. Thanks for all the reviews and helping me improving this. The improved version of state file which is already pushed to core should raise error.CorruptedState() if an old version of state file is found and caller should catch it and

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

yuja added inline comments. INLINE COMMENTS > martinvonz wrote in state.py:84 > 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

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

martinvonz added inline comments. INLINE COMMENTS > martinvonz wrote in state.py:84 > 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

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

martinvonz added inline comments. INLINE COMMENTS > pulkit wrote in state.py:84 > 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

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

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

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

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

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

pulkit updated this revision to Diff 7306. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D2593?vs=7300=7306 REVISION DETAIL https://phab.mercurial-scm.org/D2593 AFFECTED FILES mercurial/state.py CHANGE DETAILS diff --git a/mercurial/state.py

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

pulkit updated this revision to Diff 7300. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D2593?vs=6759=7300 REVISION DETAIL https://phab.mercurial-scm.org/D2593 AFFECTED FILES mercurial/state.py CHANGE DETAILS diff --git a/mercurial/state.py

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

pulkit added a comment. In https://phab.mercurial-scm.org/D2593#47511, @martinvonz wrote: > In https://phab.mercurial-scm.org/D2593#44291, @indygreg wrote: > > > Not sure where to record this comment in this series. So I'll pick this commit. > > > > I think we want an explicit

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

pulkit added inline comments. INLINE COMMENTS > martinvonz wrote in state.py:84 > 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.

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

martinvonz added a comment. In https://phab.mercurial-scm.org/D2593#44291, @indygreg wrote: > Not sure where to record this comment in this series. So I'll pick this commit. > > I think we want an explicit version header in the state files so clients know when they may be reading a

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

martinvonz added a comment. INLINE COMMENTS > durin42 wrote in state.py:84 > Probably treat not-a-dict as corrupt and fall back to the other format? 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

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

indygreg added a comment. Not sure where to record this comment in this series. So I'll pick this commit. I think we want an explicit version header in the state files so clients know when they may be reading a file in an old format. For example, if I start a merge in one terminal

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

pulkit updated this revision to Diff 6759. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D2593?vs=6618=6759 REVISION DETAIL https://phab.mercurial-scm.org/D2593 AFFECTED FILES mercurial/state.py CHANGE DETAILS diff --git a/mercurial/state.py

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

pulkit added inline comments. INLINE COMMENTS > durin42 wrote in state.py:86 > 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

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

pulkit updated this revision to Diff 6618. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D2593?vs=6444=6618 REVISION DETAIL https://phab.mercurial-scm.org/D2593 AFFECTED FILES mercurial/state.py tests/test-check-code.t CHANGE DETAILS diff --git

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

pulkit added inline comments. INLINE COMMENTS > state.py:84 > +if not isinstance(ret, dict): > +raise Exception("cbor parsed it wrong") > +return ret I am not confident about this part. Like to have some suggestions here. >

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

pulkit created this revision. Herald added a subscriber: mercurial-devel. Herald added a reviewer: hg-reviewers. REVISION SUMMARY With updating the format of statefiles to use cbor, there can be cases when a user updates hg in between a unresolved graft, rebase, histedit, cbor load function