[PATCH 4 of 4 shelve-ext v4] shelve: make shelvestate use simplekeyvaluefile

[Yuya Nishihara]

On Thu, 11 May 2017 21:09:35 +0000, Kostia Balytskyi wrote:
> > -----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.

Thanks for clarification. I'm not a fan of using inheritance excessively,
but yeah that's one way.