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

[Yuya Nishihara]

On Thu, 11 May 2017 21:09:35 +, Kostia Balytskyi wrote:
> > -Original Message-
> > From: Yuya Nishihara [mailto:you...@gmail.com] On Behalf Of Yuya
> > Nishihara
> > Sent: Thursday, 11 May, 2017 14:48
> > To: Kostia Balytskyi <ikos...@fb.com>
> > Cc: mercurial-devel@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 <ikos...@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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Kostia Balytskyi]

> -Original Message-
> From: Yuya Nishihara [mailto:you...@gmail.com] On Behalf Of Yuya
> Nishihara
> Sent: Thursday, 11 May, 2017 14:48
> To: Kostia Balytskyi <ikos...@fb.com>
> Cc: mercurial-devel@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 <ikos...@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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Yuya Nishihara]

On Sun, 7 May 2017 06:07:06 -0700, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi 
> # 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.

> 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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel