| 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
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 >
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 >
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 >
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
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.
(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.
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)