Patchwork [1,of,5,flagprocessor,v6] documentation: better censor flag documentation

login
register
mail settings
Submitter Remi Chaintron
Date Dec. 24, 2016, 4:36 p.m.
Message ID <c1bd9b7750cdfe1f2a14.1482597387@remi-mbp2>
Download mbox | patch
Permalink /patch/18022/
State Accepted
Headers show

Comments

Remi Chaintron - Dec. 24, 2016, 4:36 p.m.
# HG changeset patch
# User Remi Chaintron <remi@fb.com>
# Date 1482451718 18000
#      Thu Dec 22 19:08:38 2016 -0500
# Node ID c1bd9b7750cdfe1f2a1437e4ba0ed8f6e48b2dd1
# Parent  4f72f28e527c72cae072880b8fa1610c0289dded
documentation: better censor flag documentation.
Pierre-Yves David - Dec. 24, 2016, 6:30 p.m.
On 12/24/2016 05:36 PM, Remi Chaintron wrote:
> # HG changeset patch
> # User Remi Chaintron <remi@fb.com>
> # Date 1482451718 18000
> #      Thu Dec 22 19:08:38 2016 -0500
> # Node ID c1bd9b7750cdfe1f2a1437e4ba0ed8f6e48b2dd1
> # Parent  4f72f28e527c72cae072880b8fa1610c0289dded
> documentation: better censor flag documentation.
>
> diff --git a/mercurial/help/internals/revlogs.txt b/mercurial/help/internals/revlogs.txt
> --- a/mercurial/help/internals/revlogs.txt
> +++ b/mercurial/help/internals/revlogs.txt
> @@ -89,7 +89,7 @@
>     Absolute offset of revision data from beginning of revlog.
>  6-7 (2 bytes)
>     Bit flags impacting revision behavior. The following bit offsets define:
> -   0: 'censor' extension flag.
> +   0: REVIDX_ISCENSORED indicates the revision has censored metadata.

 From my understanding of censor, I would have expected something more 
like  "the revision content is censored". If I remember correctly, the 
censors flag means the revision content has been nuked, the metadata are 
more a secondary things that give a bit more details about that nuking.

What do you think ?

>  8-11 (4 bytes)
>     Compressed length of revision data / chunk as stored in revlog.
>  12-15 (4 bytes)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Rémi Chaintron - Dec. 24, 2016, 6:34 p.m.
Sounds good to me, I'm happy to follow the comment in revlog.py

On Sat, 24 Dec 2016 at 13:31 Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 12/24/2016 05:36 PM, Remi Chaintron wrote:
> > # HG changeset patch
> > # User Remi Chaintron <remi@fb.com>
> > # Date 1482451718 18000
> > #      Thu Dec 22 19:08:38 2016 -0500
> > # Node ID c1bd9b7750cdfe1f2a1437e4ba0ed8f6e48b2dd1
> > # Parent  4f72f28e527c72cae072880b8fa1610c0289dded
> > documentation: better censor flag documentation.
> >
> > diff --git a/mercurial/help/internals/revlogs.txt
> b/mercurial/help/internals/revlogs.txt
> > --- a/mercurial/help/internals/revlogs.txt
> > +++ b/mercurial/help/internals/revlogs.txt
> > @@ -89,7 +89,7 @@
> >     Absolute offset of revision data from beginning of revlog.
> >  6-7 (2 bytes)
> >     Bit flags impacting revision behavior. The following bit offsets
> define:
> > -   0: 'censor' extension flag.
> > +   0: REVIDX_ISCENSORED indicates the revision has censored metadata.
>
>  From my understanding of censor, I would have expected something more
> like  "the revision content is censored". If I remember correctly, the
> censors flag means the revision content has been nuked, the metadata are
> more a secondary things that give a bit more details about that nuking.
>
> What do you think ?
>
> >  8-11 (4 bytes)
> >     Compressed length of revision data / chunk as stored in revlog.
> >  12-15 (4 bytes)
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >
>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Pierre-Yves David - Dec. 24, 2016, 7:27 p.m.
On 12/24/2016 07:34 PM, Rémi Chaintron wrote:
> Sounds good to me, I'm happy to follow the comment in revlog.py

hum, the comment in revlog.py is also talking about metadata…
I've updated your patch to reuse the very same wording as in revlog.py 
(for consistency), and pushed the first one. (test-check-commit.t says hi)

I'll review the rest later.

>
> On Sat, 24 Dec 2016 at 13:31 Pierre-Yves David
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
>
>
>
>     On 12/24/2016 05:36 PM, Remi Chaintron wrote:
>     > # HG changeset patch
>     > # User Remi Chaintron <remi@fb.com <mailto:remi@fb.com>>
>     > # Date 1482451718 18000
>     > #      Thu Dec 22 19:08:38 2016 -0500
>     > # Node ID c1bd9b7750cdfe1f2a1437e4ba0ed8f6e48b2dd1
>     > # Parent  4f72f28e527c72cae072880b8fa1610c0289dded
>     > documentation: better censor flag documentation.
>     >
>     > diff --git a/mercurial/help/internals/revlogs.txt
>     b/mercurial/help/internals/revlogs.txt
>     > --- a/mercurial/help/internals/revlogs.txt
>     > +++ b/mercurial/help/internals/revlogs.txt
>     > @@ -89,7 +89,7 @@
>     >     Absolute offset of revision data from beginning of revlog.
>     >  6-7 (2 bytes)
>     >     Bit flags impacting revision behavior. The following bit
>     offsets define:
>     > -   0: 'censor' extension flag.
>     > +   0: REVIDX_ISCENSORED indicates the revision has censored metadata.
>
>      From my understanding of censor, I would have expected something more
>     like  "the revision content is censored". If I remember correctly, the
>     censors flag means the revision content has been nuked, the metadata are
>     more a secondary things that give a bit more details about that nuking.
>
>     What do you think ?
>
>     >  8-11 (4 bytes)
>     >     Compressed length of revision data / chunk as stored in revlog.
>     >  12-15 (4 bytes)
>     > _______________________________________________
>     > Mercurial-devel mailing list
>     > Mercurial-devel@mercurial-scm.org
>     <mailto:Mercurial-devel@mercurial-scm.org>
>     > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>     >
>
>     --
>     Pierre-Yves David
>     _______________________________________________
>     Mercurial-devel mailing list
>     Mercurial-devel@mercurial-scm.org
>     <mailto:Mercurial-devel@mercurial-scm.org>
>     https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
> --
> Rémi
>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Augie Fackler - Dec. 24, 2016, 7:48 p.m.
On Sat, Dec 24, 2016 at 07:30:57PM +0100, Pierre-Yves David wrote:
>
>
> On 12/24/2016 05:36 PM, Remi Chaintron wrote:
> > # HG changeset patch
> > # User Remi Chaintron <remi@fb.com>
> > # Date 1482451718 18000
> > #      Thu Dec 22 19:08:38 2016 -0500
> > # Node ID c1bd9b7750cdfe1f2a1437e4ba0ed8f6e48b2dd1
> > # Parent  4f72f28e527c72cae072880b8fa1610c0289dded
> > documentation: better censor flag documentation.
> >
> > diff --git a/mercurial/help/internals/revlogs.txt b/mercurial/help/internals/revlogs.txt
> > --- a/mercurial/help/internals/revlogs.txt
> > +++ b/mercurial/help/internals/revlogs.txt
> > @@ -89,7 +89,7 @@
> >     Absolute offset of revision data from beginning of revlog.
> >  6-7 (2 bytes)
> >     Bit flags impacting revision behavior. The following bit offsets define:
> > -   0: 'censor' extension flag.
> > +   0: REVIDX_ISCENSORED indicates the revision has censored metadata.
>
> From my understanding of censor, I would have expected something more like
> "the revision content is censored". If I remember correctly, the censors
> flag means the revision content has been nuked, the metadata are more a
> secondary things that give a bit more details about that nuking.
>
> What do you think ?

My recollection matches, for anyone following along. The metadata is a
hack that predates changegroup3 being able to exchange revlog flags,
so the in-content revlog flagging is a hack so that censored nodes can
be exchanged.

>
> >  8-11 (4 bytes)
> >     Compressed length of revision data / chunk as stored in revlog.
> >  12-15 (4 bytes)
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >
>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - Dec. 28, 2016, 4:20 p.m.
On 12/24/2016 08:48 PM, Augie Fackler wrote:
> On Sat, Dec 24, 2016 at 07:30:57PM +0100, Pierre-Yves David wrote:
>>
>>
>> On 12/24/2016 05:36 PM, Remi Chaintron wrote:
>>> # HG changeset patch
>>> # User Remi Chaintron <remi@fb.com>
>>> # Date 1482451718 18000
>>> #      Thu Dec 22 19:08:38 2016 -0500
>>> # Node ID c1bd9b7750cdfe1f2a1437e4ba0ed8f6e48b2dd1
>>> # Parent  4f72f28e527c72cae072880b8fa1610c0289dded
>>> documentation: better censor flag documentation.
>>>
>>> diff --git a/mercurial/help/internals/revlogs.txt b/mercurial/help/internals/revlogs.txt
>>> --- a/mercurial/help/internals/revlogs.txt
>>> +++ b/mercurial/help/internals/revlogs.txt
>>> @@ -89,7 +89,7 @@
>>>     Absolute offset of revision data from beginning of revlog.
>>>  6-7 (2 bytes)
>>>     Bit flags impacting revision behavior. The following bit offsets define:
>>> -   0: 'censor' extension flag.
>>> +   0: REVIDX_ISCENSORED indicates the revision has censored metadata.
>>
>> From my understanding of censor, I would have expected something more like
>> "the revision content is censored". If I remember correctly, the censors
>> flag means the revision content has been nuked, the metadata are more a
>> secondary things that give a bit more details about that nuking.
>>
>> What do you think ?
>
> My recollection matches, for anyone following along. The metadata is a
> hack that predates changegroup3 being able to exchange revlog flags,
> so the in-content revlog flagging is a hack so that censored nodes can
> be exchanged.

Good point, So if I get that right, the source of truth is the meta-data 
and the flag is just and "optimisation" then. We should update the 
internal doc to point this trickery if its not already the case.
Augie Fackler - Dec. 28, 2016, 4:25 p.m.
(bcc adgar in case he can provide insight, but I don’t know if he remembers or if he’s on vacation this/next week at all)

> On Dec 28, 2016, at 11:20 AM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
>>>> diff --git a/mercurial/help/internals/revlogs.txt b/mercurial/help/internals/revlogs.txt
>>>> --- a/mercurial/help/internals/revlogs.txt
>>>> +++ b/mercurial/help/internals/revlogs.txt
>>>> @@ -89,7 +89,7 @@
>>>>    Absolute offset of revision data from beginning of revlog.
>>>> 6-7 (2 bytes)
>>>>    Bit flags impacting revision behavior. The following bit offsets define:
>>>> -   0: 'censor' extension flag.
>>>> +   0: REVIDX_ISCENSORED indicates the revision has censored metadata.
>>> 
>>> From my understanding of censor, I would have expected something more like
>>> "the revision content is censored". If I remember correctly, the censors
>>> flag means the revision content has been nuked, the metadata are more a
>>> secondary things that give a bit more details about that nuking.
>>> 
>>> What do you think ?
>> 
>> My recollection matches, for anyone following along. The metadata is a
>> hack that predates changegroup3 being able to exchange revlog flags,
>> so the in-content revlog flagging is a hack so that censored nodes can
>> be exchanged.
> 
> Good point, So if I get that right, the source of truth is the meta-data and the flag is just and "optimisation" then. We should update the internal doc to point this trickery if its not already the case.

I think the flag is authoritative, but in order to work with cg < 3 the metadata is sometimes “authoritative” when being moved through a bundle.

I’m far more comfortable treating the flag as authoritative, and documenting the metadata structure as an important hack, which serves two purposes:

1) It gives us a tombstone to put in the revlog contents
2) When using “old” transfer mechanisms, we don’t lose the censor state.
Pierre-Yves David - Dec. 30, 2016, 9:59 a.m.
On 12/28/2016 05:25 PM, Augie Fackler wrote:
> (bcc adgar in case he can provide insight, but I don’t know if he
> remembers or if he’s on vacation this/next week at all)
>
>> On Dec 28, 2016, at 11:20 AM, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org
>> <mailto:pierre-yves.david@ens-lyon.org>> wrote:
>>
>>>>> diff --git a/mercurial/help/internals/revlogs.txt
>>>>> b/mercurial/help/internals/revlogs.txt
>>>>> --- a/mercurial/help/internals/revlogs.txt
>>>>> +++ b/mercurial/help/internals/revlogs.txt
>>>>> @@ -89,7 +89,7 @@
>>>>>    Absolute offset of revision data from beginning of revlog.
>>>>> 6-7 (2 bytes)
>>>>>    Bit flags impacting revision behavior. The following bit offsets
>>>>> define:
>>>>> -   0: 'censor' extension flag.
>>>>> +   0: REVIDX_ISCENSORED indicates the revision has censored metadata.
>>>>
>>>> From my understanding of censor, I would have expected something
>>>> more like
>>>> "the revision content is censored". If I remember correctly, the censors
>>>> flag means the revision content has been nuked, the metadata are more a
>>>> secondary things that give a bit more details about that nuking.
>>>>
>>>> What do you think ?
>>>
>>> My recollection matches, for anyone following along. The metadata is a
>>> hack that predates changegroup3 being able to exchange revlog flags,
>>> so the in-content revlog flagging is a hack so that censored nodes can
>>> be exchanged.
>>
>> Good point, So if I get that right, the source of truth is the
>> meta-data and the flag is just and "optimisation" then. We should
>> update the internal doc to point this trickery if its not already the
>> case.
>
> I think the flag is authoritative, but in order to work with cg < 3 the
> metadata is sometimes “authoritative” when being moved through a bundle.
>
> I’m far more comfortable treating the flag as authoritative, and
> documenting the metadata structure as an important hack, which serves
> two purposes:
>
> 1) It gives us a tombstone to put in the revlog contents
> 2) When using “old” transfer mechanisms, we don’t lose the censor state.

This make sense but it start to be subtle enough that I think it is 
worth documenting for the next poor souls looking into this. Can I get 
you or Adgar to send a documentation patch about this. I think we are at 
a point where mercurial/help/internals/censors.txt would make sense.

Cheers,

Patch

diff --git a/mercurial/help/internals/revlogs.txt b/mercurial/help/internals/revlogs.txt
--- a/mercurial/help/internals/revlogs.txt
+++ b/mercurial/help/internals/revlogs.txt
@@ -89,7 +89,7 @@ 
    Absolute offset of revision data from beginning of revlog.
 6-7 (2 bytes)
    Bit flags impacting revision behavior. The following bit offsets define:
-   0: 'censor' extension flag.
+   0: REVIDX_ISCENSORED indicates the revision has censored metadata.
 8-11 (4 bytes)
    Compressed length of revision data / chunk as stored in revlog.
 12-15 (4 bytes)