[PATCH 4 of 4 shelve-ext v4] shelve: make shelvestate use simplekeyvaluefile
Kostia Balytskyi
ikostia at fb.com
Thu May 11 17:09:35 EDT 2017
> -----Original Message-----
> From: Yuya Nishihara [mailto:youjah at gmail.com] On Behalf Of Yuya
> Nishihara
> Sent: Thursday, 11 May, 2017 14:48
> To: Kostia Balytskyi <ikostia at fb.com>
> Cc: mercurial-devel at mercurial-scm.org
> Subject: Re: [PATCH 4 of 4 shelve-ext v4] shelve: make shelvestate use
> simplekeyvaluefile
>
> On Sun, 7 May 2017 06:07:06 -0700, Kostia Balytskyi wrote:
> > # HG changeset patch
> > # User Kostia Balytskyi <ikostia at fb.com> # Date 1494162279 25200
> > # Sun May 07 06:04:39 2017 -0700
> > # Node ID 9f24868156f15473b08a418765411341c96e892b
> > # Parent 497904bddbaa75b9086c168ab2e03381dfaff165
> > shelve: make shelvestate use simplekeyvaluefile
>
> This looks good to me.
>
> > + @classmethod
> > + def _getversion(cls, repo):
> > + """Read version information from shelvestate file"""
> > + fp = repo.vfs(cls._filename)
> > + try:
> > + version = int(fp.readline().strip())
> > + except ValueError as err:
> > + raise error.CorruptedState(str(err))
> > + finally:
> > + fp.close()
> > + return version
> > +
> > + @classmethod
> > + def _readold(cls, repo):
> > + """Read the old position-based version of a shelvestate file"""
> > + # Order is important, because old shelvestate file uses it
> > + # to detemine values of fields (i.g. name is on the second line,
> > + # originalwctx is on the third and so forth). Please do not change.
> > + keys = ['version', 'name', 'originalwctx', 'pendingctx', 'parents',
> > + 'nodestoremove', 'branchtorestore', 'keep', 'activebook']
> > + # this is executed only seldomly, so it is not a big deal
> > + # that we open this file twice
> > + fp = repo.vfs(cls._filename)
> > + d = {}
> > + try:
> > + for key in keys:
> > + d[key] = fp.readline().strip()
> > + finally:
> > + fp.close()
> > + return d
> > +
> > + @classmethod
> > + def load(cls, repo):
> > + version = cls._getversion(repo)
> > + if version < cls._version:
> > + d = cls._readold(repo)
> > + elif version == cls._version:
> > + d = scmutil.simplekeyvaluefile(repo.vfs, cls._filename)\
> > + .read(firstlinenonkeyval=True)
>
> This is okay, but I really don't understand why the simplekeyvaluefile has to:
>
> - be a class instead of load/dump functions (like json)
> - take a vfs instead of a file object
>
> IMHO, these design decisions just make things complicated.
I like it being a class because it allows inheritance, which allows simplekeyvalue users
to add validation. Also, it can serve as a container for firstlinekey.
vfs vs a file object - I don't have an answer to this, maybe file object would've been better, but I'd prefer to not rewrite it.
>
> > diff --git a/tests/test-shelve.t b/tests/test-shelve.t
> > --- a/tests/test-shelve.t
> > +++ b/tests/test-shelve.t
> > @@ -1578,7 +1578,7 @@ shelve on new branch, conflict with prev
> > $ echo "ccc" >> a
> > $ hg status
> > M a
> > - $ hg unshelve
> > + $ hg unshelve --config shelve.oldstatefile=on
>
> Maybe this has to be updated to use a pre-generated v1 state file?
>
> > unshelving change 'default'
> > temporarily committing pending changes (restore with 'hg unshelve --
> abort')
> > rebasing shelved changes
> > @@ -1591,9 +1591,8 @@ shelve on new branch, conflict with prev
> > Removing restore branch information from shelvedstate file(making it
> > looks like in previous versions) and running unshelve --continue
> >
> > - $ head -n 6 < .hg/shelvedstate > .hg/shelvedstate_oldformat
> > - $ rm .hg/shelvedstate
> > - $ mv .hg/shelvedstate_oldformat .hg/shelvedstate
> > + $ cp .hg/shelvedstate .hg/shelvedstate_old $ cat
> > + .hg/shelvedstate_old | grep -v 'branchtorestore' > .hg/shelvedstate
More information about the Mercurial-devel
mailing list