Patchwork [3,of,3] revlog: give EXTSTORED flag value to narrowhg

# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1484681102 28800
#      Tue Jan 17 11:25:02 2017 -0800
# Node ID 2fbfddbea687ad8627b26dc856694f70078eb2b4
# Parent  7e933c9a4009d942b88bfbcb4e579a4b3f4dceca
revlog: give EXTSTORED flag value to narrowhg

Narrowhg has been using "1 << 14" as its revlog flag value for a long
time. We (Google) have many repos with that value in production
already. When the same value was reserved for EXTSTORED, it made those
repos invalid. Upgrading them will be a little painful. We should
clearly have reserved the value for narrowhg a long time ago. Since
the EXTSTORED flag is not yet in any release and Facebook also says
they have not started using it in production, so it should be okay to
change it. This patch gives the current value (1 << 14) back to
narrowhg and gives a new value (1 << 13) to EXTSTORED.

On Tue, Jan 17, 2017 at 12:10:31PM -0800, Martin von Zweigbergk via Mercurial-devel wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1484681102 28800
> #      Tue Jan 17 11:25:02 2017 -0800
> # Node ID 2fbfddbea687ad8627b26dc856694f70078eb2b4
> # Parent  7e933c9a4009d942b88bfbcb4e579a4b3f4dceca
> revlog: give EXTSTORED flag value to narrowhg

I'm (obviously?) in favor of this patch series, especially this last
one, but I won't pretend to be an impartial reviewer.

It occurred to me while talking this over with Martin that ellipsis
nodes might actually be able to overload the ISCENSORED flag - censor
wants to look for special metadata, but perhaps narrow's "just trust
me on the hash of this revision" behavior is similar enough to censor
that we should try and consolidate down to a single flag for that
purpose. Thoughts?

I'm happy to talk at length about narrowhg implementation details if
it'll help think this through.

(Note that censor doesn't even pretend to work on anything other than
filelogs, so you'd never (today) see censor outside a
filelog. Ellipsis nodes, on the other hand, can exist in any revlog as
currently implemented in narrowhg.)

>
> Narrowhg has been using "1 << 14" as its revlog flag value for a long
> time. We (Google) have many repos with that value in production
> already. When the same value was reserved for EXTSTORED, it made those
> repos invalid. Upgrading them will be a little painful. We should
> clearly have reserved the value for narrowhg a long time ago. Since
> the EXTSTORED flag is not yet in any release and Facebook also says
> they have not started using it in production, so it should be okay to
> change it. This patch gives the current value (1 << 14) back to
> narrowhg and gives a new value (1 << 13) to EXTSTORED.
>
> diff -r 7e933c9a4009 -r 2fbfddbea687 mercurial/help/internals/revlogs.txt
> --- a/mercurial/help/internals/revlogs.txt	Tue Jan 17 11:45:10 2017 -0800
> +++ b/mercurial/help/internals/revlogs.txt	Tue Jan 17 11:25:02 2017 -0800
> @@ -94,7 +94,10 @@
>
>     0: REVIDX_ISCENSORED revision has censor metadata, must be verified.
>
> -   1: REVIDX_EXTSTORED revision data is stored externally.
> +   1: REVIDX_ELLIPSIS revision hash does not match its data. Used by
> +   narrowhg
> +
> +   2: REVIDX_EXTSTORED revision data is stored externally.
>
>  8-11 (4 bytes)
>     Compressed length of revision data / chunk as stored in revlog.
> diff -r 7e933c9a4009 -r 2fbfddbea687 mercurial/revlog.py
> --- a/mercurial/revlog.py	Tue Jan 17 11:45:10 2017 -0800
> +++ b/mercurial/revlog.py	Tue Jan 17 11:25:02 2017 -0800
> @@ -54,11 +54,13 @@
>
>  # revlog index flags
>  REVIDX_ISCENSORED = (1 << 15) # revision has censor metadata, must be verified
> -REVIDX_EXTSTORED = (1 << 14) # revision data is stored externally
> +REVIDX_ELLIPSIS = (1 << 14) # revision hash does not match data (narrowhg)
> +REVIDX_EXTSTORED = (1 << 13) # revision data is stored externally
>  REVIDX_DEFAULT_FLAGS = 0
>  # stable order in which flags need to be processed and their processors applied
>  REVIDX_FLAGS_ORDER = [
>      REVIDX_ISCENSORED,
> +    REVIDX_ELLIPSIS,
>      REVIDX_EXTSTORED,
>  ]
>  REVIDX_KNOWN_FLAGS = util.bitsfrom(REVIDX_FLAGS_ORDER)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Not a problem for lfs as far as I'm concerned.
On Tue, 17 Jan 2017 at 12:34, Augie Fackler <raf@durin42.com> wrote:

> On Tue, Jan 17, 2017 at 12:10:31PM -0800, Martin von Zweigbergk via
> Mercurial-devel wrote:
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@google.com>
> > # Date 1484681102 28800
> > #      Tue Jan 17 11:25:02 2017 -0800
> > # Node ID 2fbfddbea687ad8627b26dc856694f70078eb2b4
> > # Parent  7e933c9a4009d942b88bfbcb4e579a4b3f4dceca
> > revlog: give EXTSTORED flag value to narrowhg
>
> I'm (obviously?) in favor of this patch series, especially this last
> one, but I won't pretend to be an impartial reviewer.
>
> It occurred to me while talking this over with Martin that ellipsis
> nodes might actually be able to overload the ISCENSORED flag - censor
> wants to look for special metadata, but perhaps narrow's "just trust
> me on the hash of this revision" behavior is similar enough to censor
> that we should try and consolidate down to a single flag for that
> purpose. Thoughts?
>
> I'm happy to talk at length about narrowhg implementation details if
> it'll help think this through.
>
> (Note that censor doesn't even pretend to work on anything other than
> filelogs, so you'd never (today) see censor outside a
> filelog. Ellipsis nodes, on the other hand, can exist in any revlog as
> currently implemented in narrowhg.)
>
> >
> > Narrowhg has been using "1 << 14" as its revlog flag value for a long
> > time. We (Google) have many repos with that value in production
> > already. When the same value was reserved for EXTSTORED, it made those
> > repos invalid. Upgrading them will be a little painful. We should
> > clearly have reserved the value for narrowhg a long time ago. Since
> > the EXTSTORED flag is not yet in any release and Facebook also says
> > they have not started using it in production, so it should be okay to
> > change it. This patch gives the current value (1 << 14) back to
> > narrowhg and gives a new value (1 << 13) to EXTSTORED.
> >
> > diff -r 7e933c9a4009 -r 2fbfddbea687 mercurial/help/internals/revlogs.txt
> > --- a/mercurial/help/internals/revlogs.txt    Tue Jan 17 11:45:10 2017
> -0800
> > +++ b/mercurial/help/internals/revlogs.txt    Tue Jan 17 11:25:02 2017
> -0800
> > @@ -94,7 +94,10 @@
> >
> >     0: REVIDX_ISCENSORED revision has censor metadata, must be verified.
> >
> > -   1: REVIDX_EXTSTORED revision data is stored externally.
> > +   1: REVIDX_ELLIPSIS revision hash does not match its data. Used by
> > +   narrowhg
> > +
> > +   2: REVIDX_EXTSTORED revision data is stored externally.
> >
> >  8-11 (4 bytes)
> >     Compressed length of revision data / chunk as stored in revlog.
> > diff -r 7e933c9a4009 -r 2fbfddbea687 mercurial/revlog.py
> > --- a/mercurial/revlog.py     Tue Jan 17 11:45:10 2017 -0800
> > +++ b/mercurial/revlog.py     Tue Jan 17 11:25:02 2017 -0800
> > @@ -54,11 +54,13 @@
> >
> >  # revlog index flags
> >  REVIDX_ISCENSORED = (1 << 15) # revision has censor metadata, must be
> verified
> > -REVIDX_EXTSTORED = (1 << 14) # revision data is stored externally
> > +REVIDX_ELLIPSIS = (1 << 14) # revision hash does not match data
> (narrowhg)
> > +REVIDX_EXTSTORED = (1 << 13) # revision data is stored externally
> >  REVIDX_DEFAULT_FLAGS = 0
> >  # stable order in which flags need to be processed and their processors
> applied
> >  REVIDX_FLAGS_ORDER = [
> >      REVIDX_ISCENSORED,
> > +    REVIDX_ELLIPSIS,
> >      REVIDX_EXTSTORED,
> >  ]
> >  REVIDX_KNOWN_FLAGS = util.bitsfrom(REVIDX_FLAGS_ORDER)
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

(+marmoute for comment as well)

On Tue, Jan 17, 2017 at 4:32 PM, Rémi Chaintron
<remi.chaintron@gmail.com> wrote:
> Not a problem for lfs as far as I'm concerned.

Any feelings on trying to merge the ellipsis and censor flags? Should
We proceed with this series and then free up the ellipsis flag if that
path works out?

(I'm not quite sure where the "don't waste flags" vs "too complicated"
tradeoff point is.)

On 01/17/2017 09:34 PM, Augie Fackler wrote:
> On Tue, Jan 17, 2017 at 12:10:31PM -0800, Martin von Zweigbergk via Mercurial-devel wrote:
>> # HG changeset patch
>> # User Martin von Zweigbergk <martinvonz@google.com>
>> # Date 1484681102 28800
>> #      Tue Jan 17 11:25:02 2017 -0800
>> # Node ID 2fbfddbea687ad8627b26dc856694f70078eb2b4
>> # Parent  7e933c9a4009d942b88bfbcb4e579a4b3f4dceca
>> revlog: give EXTSTORED flag value to narrowhg
>
> I'm (obviously?) in favor of this patch series, especially this last
> one, but I won't pretend to be an impartial reviewer.

I went through the IRC log of the conversation between Martin von 
Zweigbergk and Durham Goode and checked the Rémi Chaintron email reply.
It seems like LFS user are fine with this early change so I'm pushing it.

> It occurred to me while talking this over with Martin that ellipsis
> nodes might actually be able to overload the ISCENSORED flag - censor
> wants to look for special metadata, but perhaps narrow's "just trust
> me on the hash of this revision" behavior is similar enough to censor
> that we should try and consolidate down to a single flag for that
> purpose. Thoughts?
>
> I'm happy to talk at length about narrowhg implementation details if
> it'll help think this through.
>
> (Note that censor doesn't even pretend to work on anything other than
> filelogs, so you'd never (today) see censor outside a
> filelog. Ellipsis nodes, on the other hand, can exist in any revlog as
> currently implemented in narrowhg.)

Given that "censor" and "ellipsis" are distinct use case, I think it 
make sense to use different flag. We have a limited amount of them, but 
we still have some room.

In addition, LFS don't really care about which flag they get as long as 
they get one and it stay stable over version. So we still have the whole 
freeze to decide on a potential drop of the REVIDX_ELLIPSIS flag.