[PATCH 2 of 2 shelve-ext] shelve: add shelve type saving and loading

[Kostia Balytskyi]

The line can be added later, but it's less extension-friendly this way. Someone may add some new lines to shelvestate and extension-writers would need to adopt for position changes. Reserving this position will mean that core shelve changes affect extensions to a smaller degree.

This was one of the reasons I wanted to get simple key-value file in core, but I do not want to port shelvestate to this format now.

-----Original Message-----
From: Ryan McElroy 
Sent: Friday, 7 April, 2017 14:58
To: Kostia Balytskyi <ikostia at fb.com>; mercurial-devel at mercurial-scm.org
Subject: Re: [PATCH 2 of 2 shelve-ext] shelve: add shelve type saving and loading

On 4/7/17 2:46 PM, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia at fb.com> # Date 1491572377 25200
> #      Fri Apr 07 06:39:37 2017 -0700
> # Node ID 3ef1bf4e61ef5c5957a8c5978dfc2785e31dcc42
> # Parent  c70f98920d904c94abe17f0b4fd141928dfa3e84
> shelve: add shelve type saving and loading
>
> I would like to reserve a line in shelvestate file for shelve type used.
> It looks like whenever we go with the alternative approach, we will 
> have to support traditional shelve for some time, so this makes sense.
>
> diff --git a/hgext/shelve.py b/hgext/shelve.py
> --- a/hgext/shelve.py
> +++ b/hgext/shelve.py
> @@ -173,6 +173,7 @@ class shelvedstate(object):
>       _nokeep = 'nokeep'
>       # colon is essential to differentiate from a real bookmark name
>       _noactivebook = ':no-active-bookmark'
> +    _traditional = 'traditional'
>   
>       @classmethod
>       def load(cls, repo):
> @@ -191,6 +192,9 @@ class shelvedstate(object):
>               branchtorestore = fp.readline().strip()
>               keep = fp.readline().strip() == cls._keep
>               activebook = fp.readline().strip()
> +            # we do not use shelvekind anywhere now,
> +            # this is just to reserve a line in a file
> +            shelvekind = fp.readline().strip()
>           except (ValueError, TypeError) as err:
>               raise error.CorruptedState(str(err))
>           finally:
> @@ -208,6 +212,7 @@ class shelvedstate(object):
>               obj.activebookmark = ''
>               if activebook != cls._noactivebook:
>                   obj.activebookmark = activebook
> +            obj.shelvekind = shelvekind
>           except error.RepoLookupError as err:
>               raise error.CorruptedState(str(err))
>   
> @@ -228,6 +233,8 @@ class shelvedstate(object):
>           fp.write('%s\n' % branchtorestore)
>           fp.write('%s\n' % (cls._keep if keep else cls._nokeep))
>           fp.write('%s\n' % (activebook or cls._noactivebook))
> +        # for now we only have traditional shelves
> +        fp.write('%s\n' % cls._traditional)
>           fp.close()
>   
>       @classmethod
>

I don't see much harm here, but I also don't see any benefit.

Is there a disadvantage to just adding this line later? Like, is it important to get a line reserved in the file before the freeze, for example?

My guess would be "no" because if the line is missing, you can assume "traditional" shelve, which you will have to do anyway for back-compat. 
So I'm -1 on this patch at this time.

However, if there's a stronger reason to reserve the line now, please state that in the commit message clearly and send a v2.

For now, I've marked this series as "changes requested" in patchwork.