Submitter | Kostia Balytskyi |
---|---|
Date | March 26, 2017, 10:32 p.m. |
Message ID | <43cd56f38c1ca2ad1920.1490567575@devvm1416.lla2.facebook.com> |
Download | mbox | patch |
Permalink | /patch/19699/ |
State | Rejected |
Headers | show |
Comments
I think this is a good direction. See inline comments. Not related directly to this patch, but I was thinking introducing some "post-transaction" mechanism for transaction. So the caller won't need to put the "strip" outside a transaction. Compare: Currently: if obsmarker-enabled: with repo.transaction(): .... create commits create markers for strip # ideally inside the transaction else: with repo.transaction(): create commits strip # must be outside a transaction If we have "post-transaction" for repair.strip. The code could be written in a cleaner form without the "if", and there is only one transaction as expected. Excerpts from Kostia Balytskyi's message of 2017-03-26 15:32:55 -0700: > # HG changeset patch > # User Kostia Balytskyi <ikostia@fb.com> > # Date 1490567500 25200 > # Sun Mar 26 15:31:40 2017 -0700 > # Node ID 43cd56f38c1ca2ad1920ed6804e1a90a5e263c0d > # Parent c60091fa1426892552dd6c0dd4b9c49e3c3da045 > repo: add an ability to hide nodes in an appropriate way > > Potentially frequent usecase is to hide nodes in the most appropriate > for current repo configuration way. Examples of things which use this > are rebase (strip or obsolete old nodes), shelve (strip of obsolete > temporary nodes) and probably others. > Jun Wu had an idea of having one place in core where this functionality > is implemented so that others can utilize it. This patch > implements this idea. > > 'insistonstripping' is necessary for cases when caller might need > to enforce or not enforce stripping depending on some configuration > (like shelve needs now). That sounds against the motivation of introducing the method. Could the caller just call repair.strip, or disable createmarkers (using configoverride) if it insists to? > > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py > --- a/mercurial/localrepo.py > +++ b/mercurial/localrepo.py > @@ -50,6 +50,7 @@ from . import ( > phases, > pushkey, > pycompat, > + repair, > repoview, > revset, > revsetlang, > @@ -1884,6 +1885,23 @@ class localrepository(object): > # tag cache retrieval" case to work. > self.invalidate() > > + def hidenodes(self, nodes, insistonstripping=False): Since it may call "strip", "hide" is not accurate. Maybe just call it "strip". > + '''Remove nodes from visible repoview in the most appropriate Since strip won't support pruning a commit in the middle, and root-based hidden won't support that either. Maybe make it clear nodes' descendants will also be affected. > + supported way. When obsmarker creation is enabled, this > + function will obsolete these nodes without successors > + (unless insistonstripping is True). Otherwise, it will > + strip nodes. > + > + In future we can add other node-hiding capabilities to this > + function.''' > + with self.lock(): Then select "nodes::" here to get their descendants hidden. > + if obsolete.isenabled(self, obsolete.createmarkersopt)\ > + and not insistonstripping: > + relations = [(self[n], ()) for n in nodes] > + obsolete.createmarkers(self, relations) > + else: > + repair.strip(self.ui, self, nodes, backup=False) > + > def walk(self, match, node=None): > ''' > walk recursively through the directory tree or a given > diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t > --- a/tests/test-obsolete.t > +++ b/tests/test-obsolete.t > @@ -1259,3 +1259,43 @@ Test the --delete option of debugobsolet > 1ab51af8f9b41ef8c7f6f3312d4706d870b1fb74 29346082e4a9e27042b62d2da0e2de211c027621 0 \(.*\) {'user': 'test'} (re) > $ cd .. > > +Test that repo.hidenodes respects obsolescense configs > + $ hg init hidenodesrepo && cd hidenodesrepo > + $ cat <<EOF > debughidenodes.py > + > from __future__ import absolute_import > + > from mercurial import ( > + > cmdutil, > + > ) > + > cmdtable = {} > + > command = cmdutil.command(cmdtable) > + > @command('debughidenodes', > + > [('', 'insiststrip', None, 'pass insistonstripping=True')]) > + > def debughidenodes(ui, repo, *args, **opts): > + > nodes = [repo[n].node() for n in args] > + > repo.hidenodes(nodes, insistonstripping=bool(opts.get('insiststrip'))) > + > EOF > + $ cat <<EOF > .hg/hgrc > + > [extensions] > + > debughidenodes=debughidenodes.py > + > EOF > + $ echo a > a > + $ hg add a && hg ci -m a && hg up null > + 0 files updated, 0 files merged, 1 files removed, 0 files unresolved > + $ hg --config experimental.evolution=! debughidenodes 0 > + $ hg log -qr "all()" --hidden | wc -l # commit was actually stripped > + \s*0 (re) > + $ hg debugobsolete | wc -l # no markers were created > + \s*0 (re) > + $ echo a > a && hg add a && hg ci -m a && hg up null > + 0 files updated, 0 files merged, 1 files removed, 0 files unresolved > + $ hg --config experimental.evolution=createmarkers debughidenodes 0 > + $ hg log -qr "all()" --hidden | wc -l # commit was not stripped > + \s*1 (re) > + $ hg debugobsolete | wc -l # a marker was created > + \s*1 (re) > + $ hg --config experimental.evolution=createmarkers --hidden debughidenodes 0 --insiststrip > + $ hg log -qr "all()" --hidden | wc -l # commit was actually stripped > + \s*0 (re) > + $ hg debugobsolete | wc -l # no new markers were created > + \s*1 (re) > + $ cd ..
On Sun, Mar 26, 2017 at 04:44:11PM -0700, Jun Wu wrote: > I think this is a good direction. See inline comments. > > Not related directly to this patch, but I was thinking introducing some > "post-transaction" mechanism for transaction. So the caller won't need to > put the "strip" outside a transaction. Compare: > > Currently: > > if obsmarker-enabled: > with repo.transaction(): > .... > create commits > create markers for strip # ideally inside the transaction > else: > with repo.transaction(): > create commits > strip # must be outside a transaction > > If we have "post-transaction" for repair.strip. The code could be written in > a cleaner form without the "if", and there is only one transaction as > expected. This seems like a good thing to have in a world where we expect strip to continue to be a thing we have to use, but I'd rather the engineering energy that could go into this API instead went into a safer append-only hiding mechanism. > > Excerpts from Kostia Balytskyi's message of 2017-03-26 15:32:55 -0700: > > # HG changeset patch > > # User Kostia Balytskyi <ikostia@fb.com> > > # Date 1490567500 25200 > > # Sun Mar 26 15:31:40 2017 -0700 > > # Node ID 43cd56f38c1ca2ad1920ed6804e1a90a5e263c0d > > # Parent c60091fa1426892552dd6c0dd4b9c49e3c3da045 > > repo: add an ability to hide nodes in an appropriate way > > > > Potentially frequent usecase is to hide nodes in the most appropriate > > for current repo configuration way. Examples of things which use this > > are rebase (strip or obsolete old nodes), shelve (strip of obsolete > > temporary nodes) and probably others. > > Jun Wu had an idea of having one place in core where this functionality > > is implemented so that others can utilize it. This patch > > implements this idea. > > > > 'insistonstripping' is necessary for cases when caller might need > > to enforce or not enforce stripping depending on some configuration > > (like shelve needs now). > > That sounds against the motivation of introducing the method. Could the > caller just call repair.strip, or disable createmarkers (using > configoverride) if it insists to? > > > > > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py > > --- a/mercurial/localrepo.py > > +++ b/mercurial/localrepo.py > > @@ -50,6 +50,7 @@ from . import ( > > phases, > > pushkey, > > pycompat, > > + repair, > > repoview, > > revset, > > revsetlang, > > @@ -1884,6 +1885,23 @@ class localrepository(object): > > # tag cache retrieval" case to work. > > self.invalidate() > > > > + def hidenodes(self, nodes, insistonstripping=False): > > Since it may call "strip", "hide" is not accurate. Maybe just call it > "strip". > > > + '''Remove nodes from visible repoview in the most appropriate > > Since strip won't support pruning a commit in the middle, and root-based > hidden won't support that either. Maybe make it clear nodes' descendants > will also be affected. > > > + supported way. When obsmarker creation is enabled, this > > + function will obsolete these nodes without successors > > + (unless insistonstripping is True). Otherwise, it will > > + strip nodes. > > + > > + In future we can add other node-hiding capabilities to this > > + function.''' > > + with self.lock(): > > Then select "nodes::" here to get their descendants hidden. > > > + if obsolete.isenabled(self, obsolete.createmarkersopt)\ > > + and not insistonstripping: > > + relations = [(self[n], ()) for n in nodes] > > + obsolete.createmarkers(self, relations) > > + else: > > + repair.strip(self.ui, self, nodes, backup=False) > > + > > def walk(self, match, node=None): > > ''' > > walk recursively through the directory tree or a given > > diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t > > --- a/tests/test-obsolete.t > > +++ b/tests/test-obsolete.t > > @@ -1259,3 +1259,43 @@ Test the --delete option of debugobsolet > > 1ab51af8f9b41ef8c7f6f3312d4706d870b1fb74 29346082e4a9e27042b62d2da0e2de211c027621 0 \(.*\) {'user': 'test'} (re) > > $ cd .. > > > > +Test that repo.hidenodes respects obsolescense configs > > + $ hg init hidenodesrepo && cd hidenodesrepo > > + $ cat <<EOF > debughidenodes.py > > + > from __future__ import absolute_import > > + > from mercurial import ( > > + > cmdutil, > > + > ) > > + > cmdtable = {} > > + > command = cmdutil.command(cmdtable) > > + > @command('debughidenodes', > > + > [('', 'insiststrip', None, 'pass insistonstripping=True')]) > > + > def debughidenodes(ui, repo, *args, **opts): > > + > nodes = [repo[n].node() for n in args] > > + > repo.hidenodes(nodes, insistonstripping=bool(opts.get('insiststrip'))) > > + > EOF > > + $ cat <<EOF > .hg/hgrc > > + > [extensions] > > + > debughidenodes=debughidenodes.py > > + > EOF > > + $ echo a > a > > + $ hg add a && hg ci -m a && hg up null > > + 0 files updated, 0 files merged, 1 files removed, 0 files unresolved > > + $ hg --config experimental.evolution=! debughidenodes 0 > > + $ hg log -qr "all()" --hidden | wc -l # commit was actually stripped > > + \s*0 (re) > > + $ hg debugobsolete | wc -l # no markers were created > > + \s*0 (re) > > + $ echo a > a && hg add a && hg ci -m a && hg up null > > + 0 files updated, 0 files merged, 1 files removed, 0 files unresolved > > + $ hg --config experimental.evolution=createmarkers debughidenodes 0 > > + $ hg log -qr "all()" --hidden | wc -l # commit was not stripped > > + \s*1 (re) > > + $ hg debugobsolete | wc -l # a marker was created > > + \s*1 (re) > > + $ hg --config experimental.evolution=createmarkers --hidden debughidenodes 0 --insiststrip > > + $ hg log -qr "all()" --hidden | wc -l # commit was actually stripped > > + \s*0 (re) > > + $ hg debugobsolete | wc -l # no new markers were created > > + \s*1 (re) > > + $ cd .. > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
On 03/30/2017 07:08 PM, Ryan McElroy wrote: > On 3/30/17 3:15 PM, Pierre-Yves David wrote: >> […] >> I hope this long message help to clarify various concept. We have way >> forward to reduce the use of stripping without abusing the >> obsolescence concept in a way that will create issue for users. These >> way forward are in reach and would not take too long to build. >> >> > > For what it's worth, I find this essay pretty convincing: obsmarkers > have their use-case, they are lightly abused now (for temp-amend-commit) > but it probably makes sense to not abuse them further. > > It sounds like an "internal" phase above "secret" would actually map > fairly well onto the temp-amend-commit, hidden shelves never meant to be > exchanged, aborted rebase, etc places. > > Pierre-Yves, since you've thought about this a lot, does a phase here > make sense (note that I'm not saying a general hiding place like > "archive" phase, but just for internal nodes that we generally don't > want shown or exchanged, but are also not "technically obsolete". Yes, phase could actually work when we talk about internal changesets. They property makes is * They are irrelevant to the users, So, we do not care about preserving draft/secret phase, * We should never-ever base anything on them, So the inheritance to parent will not be an issue * We should never-ever access them ever again, So we can thrown them in the phase and never think about them again. * They should never-ever leave the repository. So their should not be any distributed related issue with them (and even if they are hard copied, it is fine to never-ever mention them during exchange. * They never stop being internal-changesets That one is a bit less "phasee", we would have to adds some boundary. --- You I are right, given the property of internal changesets phases may be a good pick for them. I'll do a full review of the problem space and its possible solution, if you want the summary skip to the end Problem Space ============= Possible approach ----------------- Since introducing a new "internal" concept will requires a new repository requirement. Lets discussion all options we have at hands. 1) dedicated 'internal' phases, 2) ad-hoc mechanism (lets says bytemap), 3) Something in changeset extra, (If someone think about something else, let me know) Internal changeset property --------------------------- Now let us summarize their property of internal changesets A) They are irrelevant to the users, B) Nothing will be based on them, C) Once done with their purpose, we should not need them again, D) They never leave the repository, E) They never stop being internal changesets, Life Cycle of Internal Changeset --------------------------------- And lets look at the life cycle of internal changeset we know: amend ````` 1) transaction is open, 2) a temporary commit is created, 3) and used to build the result of the amend, 4) obsmarkers are created (between old and new), 5) temporary commit is disposed of (currently obsmarkers), 6) transaction is committed. The commit is created and "hidden" in the same transaction. It is never visible to anyone. Actually, we would fully remove this change set with appropriate in-memory-ctx capabilities. histedit ```````` Scenario: pick second-changeset pick first-changeset roll fourth changeset # the important part is the fold pick third changeset (Assuming single transaction for simplicity) 1) transaction is open, 2) <second-changeset> is rebased on base (possible conflict and associated transaction open/close) -> creates <final-first>, 3) <first-changeset> is rebased on the result (possible conflict and associated transaction open/close) -> creates <temporary-second>, 4) <fourth-changeset> is rebased on the result (no commit), (possible conflict and associated transaction open/close) 5) result is folded in <temporary-second> as <final-second> 6) <third-changeset> is rebased on the result (possible conflict and associated transaction open/close) -> creates <final-third>, 7) transaction is committed. Possible conflict during (4) will expose <temporary-second> as visible to the user, part of the merge conflict and working directory parent. shelve `````` shelving: 1) transaction is open, 2) shelved change are committed, 3) commit is recorded as a shelve-changeset, 4) apply hidding on this shelve, 5) transaction is committed unshelving (I might be wrong, I've not followed everything): 1) transaction is open 2) uncommitted changes are put in an temporary commit *) access the shelve-changeset (somehow), 3) hg rebase -r <shelve> -d <tmp> <) possible conflict resolution that requires transaction commit >) possible transaction reopen (if conflict) =) rebase complete 4) grab the result of the shelve and restore working copy parent 5) hide <tmp> and <rebased> 6) close transaction Note: during possible rebase conflict, the "shelve" is currently visible to the user. Can we change this ? * At minimum, it needs to be at least visible to the "hg resolve command". This is easy to achieve in all case. * Having it visible seems valuable for the user experience. conclusion in life cycle ```````````````````````` Internal: visible -> not visible (rest there). / visible not visible Additional note about shelve ----------------------------- We could skip using the full rebase command and skip one of the temporary changeset. XXX skip the temp commit entirely Only remain shelve changesets - deleted shelve It all happens in Analysis of solution ==================== Phases ------ ad-hoc ------ changeset data -------------- New space, Need caching, Conclusion ========== * two phases (redefined + hard boundary) * Still use extra.
grmlm, this half-written draft was sent by mistake, please disregard On 03/31/2017 10:46 PM, Pierre-Yves David wrote: > > > On 03/30/2017 07:08 PM, Ryan McElroy wrote: >> On 3/30/17 3:15 PM, Pierre-Yves David wrote: >>> […] >>> I hope this long message help to clarify various concept. We have way >>> forward to reduce the use of stripping without abusing the >>> obsolescence concept in a way that will create issue for users. These >>> way forward are in reach and would not take too long to build. >>> >>> >> >> For what it's worth, I find this essay pretty convincing: obsmarkers >> have their use-case, they are lightly abused now (for temp-amend-commit) >> but it probably makes sense to not abuse them further. >> >> It sounds like an "internal" phase above "secret" would actually map >> fairly well onto the temp-amend-commit, hidden shelves never meant to be >> exchanged, aborted rebase, etc places. >> >> Pierre-Yves, since you've thought about this a lot, does a phase here >> make sense (note that I'm not saying a general hiding place like >> "archive" phase, but just for internal nodes that we generally don't >> want shown or exchanged, but are also not "technically obsolete". > > Yes, phase could actually work when we talk about internal changesets. > They property makes is > > * They are irrelevant to the users, > > So, we do not care about preserving draft/secret phase, > > * We should never-ever base anything on them, > > So the inheritance to parent will not be an issue > > * We should never-ever access them ever again, > > So we can thrown them in the phase and never think about them again. > > * They should never-ever leave the repository. > > So their should not be any distributed related issue with them (and even > if they are hard copied, it is fine to never-ever mention them during > exchange. > > * They never stop being internal-changesets > > That one is a bit less "phasee", we would have to adds some boundary. > > --- > > You I are right, given the property of internal changesets phases may be > a good pick for them. > > I'll do a full review of the problem space and its possible solution, if > you want the summary skip to the end > > > Problem Space > ============= > > Possible approach > ----------------- > > Since introducing a new "internal" concept will requires a new > repository requirement. Lets discussion all options we have at hands. > > 1) dedicated 'internal' phases, > 2) ad-hoc mechanism (lets says bytemap), > 3) Something in changeset extra, > > (If someone think about something else, let me know) > > Internal changeset property > --------------------------- > > Now let us summarize their property of internal changesets > > A) They are irrelevant to the users, > B) Nothing will be based on them, > C) Once done with their purpose, we should not need them again, > D) They never leave the repository, > E) They never stop being internal changesets, > > Life Cycle of Internal Changeset > --------------------------------- > > And lets look at the life cycle of internal changeset we know: > > amend > ````` > > 1) transaction is open, > 2) a temporary commit is created, > 3) and used to build the result of the amend, > 4) obsmarkers are created (between old and new), > 5) temporary commit is disposed of (currently obsmarkers), > 6) transaction is committed. > > The commit is created and "hidden" in the same transaction. It is never > visible to anyone. > > Actually, we would fully remove this change set with appropriate > in-memory-ctx capabilities. > > > histedit > ```````` > > Scenario: > > pick second-changeset > pick first-changeset > roll fourth changeset # the important part is the fold > pick third changeset > > (Assuming single transaction for simplicity) > > 1) transaction is open, > 2) <second-changeset> is rebased on base > (possible conflict and associated transaction open/close) > -> creates <final-first>, > 3) <first-changeset> is rebased on the result > (possible conflict and associated transaction open/close) > -> creates <temporary-second>, > 4) <fourth-changeset> is rebased on the result (no commit), > (possible conflict and associated transaction open/close) > 5) result is folded in <temporary-second> as <final-second> > 6) <third-changeset> is rebased on the result > (possible conflict and associated transaction open/close) > -> creates <final-third>, > 7) transaction is committed. > > Possible conflict during (4) will expose <temporary-second> as visible > to the user, part of the merge conflict and working directory parent. > > > shelve > `````` > > shelving: > > 1) transaction is open, > 2) shelved change are committed, > 3) commit is recorded as a shelve-changeset, > 4) apply hidding on this shelve, > 5) transaction is committed > > > > unshelving (I might be wrong, I've not followed everything): > > 1) transaction is open > 2) uncommitted changes are put in an temporary commit > *) access the shelve-changeset (somehow), > 3) hg rebase -r <shelve> -d <tmp> > <) possible conflict resolution that requires transaction commit > >) possible transaction reopen (if conflict) > =) rebase complete > 4) grab the result of the shelve and restore working copy parent > 5) hide <tmp> and <rebased> > 6) close transaction > > Note: during possible rebase conflict, the "shelve" is currently visible > to the user. Can we change this ? > > * At minimum, it needs to be at least visible to the "hg resolve > command". This is easy to achieve in all case. > * Having it visible seems valuable for the user experience. > > conclusion in life cycle > ```````````````````````` > > Internal: > > > visible -> not visible (rest there). > / visible not visible > > > > > > > > > > Additional note about shelve > ----------------------------- > We could skip using the full rebase command and skip one of the > temporary changeset. > > XXX skip the temp commit entirely > > Only remain shelve changesets > - deleted shelve > > > It all happens in > > > Analysis of solution > ==================== > > Phases > ------ > > > ad-hoc > ------ > > > changeset data > -------------- > > New space, > > Need caching, > > > Conclusion > ========== > > * two phases (redefined + hard boundary) > * Still use extra. > > > > >
Excerpts from Pierre-Yves David's message of 2017-04-03 13:11:10 +0200:
> Local-hidding still needs its own solution.
If we do have root-based hidden, and it's used for both "strip" and this
"local-only" thing. What problem will that cause? Worst case we add a bit
per root saying "whether this is by user or by system". But I don't think
they are fundamentally different that requires two different solutions.
On 4/3/17 4:11 AM, Pierre-Yves David wrote: > On 04/02/2017 05:56 AM, Jun Wu wrote: >> IIRC at the sprint that people tend to agree on root based hidden >> separated from phase root. > > There seems to be a large confusion here. This thread is talking about > "internal" changeset only. As explain in my two previous large emails, > in practice, today, the "internal" changeset operate in their own > "space" and the users do not need to care about them. Their various > properties make phases a possible option and even probably a good choice. > > This is -distinct- from what we have discussed at the sprint: local-only > hiding of changeset in the -real-space-. I've been part of these people > cautious about using phases for local-only-hiding on changesets in the > -real-space-, but this proposal is unrelated and does not contradict it. > Local-hidding still needs its own solution. While I understand that you're referring to a specific use case here, I think introducing a distinction between internal commits and normally hidden commits adds unnecessary conceptual overhead and complexity. I think we should avoid making that distinction, and instead head towards a world where hidden is a single unified concept, separate from obsolescence, and therefore we shouldn't go down the path of introducing new concepts like internal-commits right now. In the mean time though, having an API for hiding commits seems like a reasonable API regardless of its eventual implementation. Since obsolescence is the only way we have of hiding commits today, and since shelve's abuse of transactions is so terrible, I am in favor of introducing this API, then changing the implementation later when we've developed a better story for hiding. This way we can at least move all current 'bad' uses of obsolescence (i.e. uses that are just for hiding commits) to this new API, and they can be easily migrated to the future hiding mechanism. From a timeline perspective, I expect us to work on the hidden root feature within the next 2-3 months, so we're not kicking the can too far down the road.
On Mon, Apr 03, 2017 at 12:55:41PM -0700, Durham Goode wrote: > > > On 4/3/17 4:11 AM, Pierre-Yves David wrote: > > On 04/02/2017 05:56 AM, Jun Wu wrote: > > > IIRC at the sprint that people tend to agree on root based hidden > > > separated from phase root. > > > > There seems to be a large confusion here. This thread is talking about > > "internal" changeset only. As explain in my two previous large emails, > > in practice, today, the "internal" changeset operate in their own > > "space" and the users do not need to care about them. Their various > > properties make phases a possible option and even probably a good choice. > > > > This is -distinct- from what we have discussed at the sprint: local-only > > hiding of changeset in the -real-space-. I've been part of these people > > cautious about using phases for local-only-hiding on changesets in the > > -real-space-, but this proposal is unrelated and does not contradict it. > > Local-hidding still needs its own solution. > > While I understand that you're referring to a specific use case here, I > think introducing a distinction between internal commits and normally hidden > commits adds unnecessary conceptual overhead and complexity. I think we > should avoid making that distinction, and instead head towards a world where > hidden is a single unified concept, separate from obsolescence, and > therefore we shouldn't go down the path of introducing new concepts like > internal-commits right now. > > In the mean time though, having an API for hiding commits seems like a > reasonable API regardless of its eventual implementation. Since obsolescence > is the only way we have of hiding commits today, and since shelve's abuse of > transactions is so terrible, I am in favor of introducing this API, then > changing the implementation later when we've developed a better story for > hiding. This way we can at least move all current 'bad' uses of obsolescence > (i.e. uses that are just for hiding commits) to this new API, and they can > be easily migrated to the future hiding mechanism. I'm lightly in favor of this plan, with the caveat that I'd like more certainty (read: a brief[0], clear, and concrete plan) around the future of hiding things and what that means before we take the API. I'm actually pretty lost right now and would appreciate an executive summary of the state of where we think this is headed from someone involved. Right now the overhead of getting up to speed on this is so high that I'm not sure when I'll have a sufficiently large block of time to read the body of text that's been produced in the last two weeks. :( 0: no more than 2 printed pages, let's say. > > From a timeline perspective, I expect us to work on the hidden root feature > within the next 2-3 months, so we're not kicking the can too far down the > road. > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
TL;DR; having an internal changeset concept help building simpler UI. Implementation-wise, phases offer a fitting home for the concept while allow simpler implementation now and later. Moreover, this can be done sooner and independently from other discussions. On 04/03/2017 06:02 PM, Jun Wu wrote: > Excerpts from Pierre-Yves David's message of 2017-04-03 13:11:10 +0200: >> Local-hidding still needs its own solution. > > If we do have root-based hidden, and it's used for both "strip" and this > "local-only" thing. Let us make a small vocabulary point first (because I think it is important to be on the same pages here). * "strip" is the action of "physically" removing data from the repository. And this should remains so. The word is accurate and describe an operation that is sometimes useful and will stay around in the future. I know that Facebook use this word for something else but we should stick to strict semantic here to avoid confusion. * "root-based hidden" is a possible implementation for hiding. At that point of the discussion it would better to stay at the concept level "local-hiding" or "local-only-hiding" seems describe the concept you have in mind well. "root-based" is one of the many possible implementation. Until we have actually reviewed use-cases and option we are not sure we'll use that implementation for that concept, in practice a bitemap-based or ranged-based approach might turn out a better choice. In addition such description could match implementation for other concept. For example this internal phases is also "root-based" since phases are "root-based" > What problem will that cause? Worst case we add a bit > per root saying "whether this is by user or by system". But I don't think > they are fundamentally different that requires two different solutions. As mentioned in previous messages, we could use a generic local-hiding mechanism to hide internal changesets. I do not think this will create major problem. They are still multiple advantages to have handled by a new phases: * It fits well into the phases concept "public, draft, secret, internal" feel natural, * We already have all the visualization and manipulation we want for phases, so the "internal-changeset" comes at a very low overhead extension to an existing concept, * clear distinction between hidden internal-changeset and hidden real-changesets will help offer a clean UI around locally-hidden changesets. (To avoid internals polluting user output related to hidden-ess) * Adding extra bits related to internal in local-hiding will make local-hiding implementation and UI more complex. (while extending phases does not requires any changes) Implementation-wise, adding "internal" through phase should be quite trivial, we already have a solid phase implementation. Include fast C code to computes revision in each phases. And the hiding API support multiple source for feeding "internal" phase to it will be trivial. To conclude, having "internal-changeset" as an explicit concept will help building better/simpler UI. From my analysis, using phases for them provides us with a simpler implementation both -right- -now- and -later- even in the presence of a local hiding mechanism. What do you think ? Cheers,
Since most people want a separate hidden storage that handles wider cases, I don't think this incomplete (internal-only) solution worths investment. Excerpts from Pierre-Yves David's message of 2017-04-04 20:26:29 +0200: > TL;DR; having an internal changeset concept help building simpler UI. > Implementation-wise, phases offer a fitting home for the concept while > allow simpler implementation now and later. Moreover, this can be done > sooner and independently from other discussions. > > On 04/03/2017 06:02 PM, Jun Wu wrote: > > Excerpts from Pierre-Yves David's message of 2017-04-03 13:11:10 +0200: > >> Local-hidding still needs its own solution. > > > > If we do have root-based hidden, and it's used for both "strip" and this > > "local-only" thing. > > Let us make a small vocabulary point first (because I think it is > important to be on the same pages here). > > * "strip" is the action of "physically" removing data from the > repository. And this should remains so. The word is accurate and > describe an operation that is sometimes useful and will stay around in > the future. > I know that Facebook use this word for something else but we should > stick to strict semantic here to avoid confusion. > > * "root-based hidden" is a possible implementation for hiding. At that > point of the discussion it would better to stay at the concept level > "local-hiding" or "local-only-hiding" seems describe the concept you > have in mind well. "root-based" is one of the many possible > implementation. Until we have actually reviewed use-cases and option we > are not sure we'll use that implementation for that concept, in practice > a bitemap-based or ranged-based approach might turn out a better choice. > In addition such description could match implementation for other > concept. For example this internal phases is also "root-based" since > phases are "root-based" > > > What problem will that cause? Worst case we add a bit > > per root saying "whether this is by user or by system". But I don't think > > they are fundamentally different that requires two different solutions. > > As mentioned in previous messages, we could use a generic local-hiding > mechanism to hide internal changesets. I do not think this will create > major problem. > > They are still multiple advantages to have handled by a new phases: > > * It fits well into the phases concept "public, draft, secret, internal" > feel natural, > > * We already have all the visualization and manipulation we want for > phases, so the "internal-changeset" comes at a very low overhead > extension to an existing concept, > > * clear distinction between hidden internal-changeset and hidden > real-changesets will help offer a clean UI around locally-hidden > changesets. (To avoid internals polluting user output related to hidden-ess) > > * Adding extra bits related to internal in local-hiding will make > local-hiding implementation and UI more complex. (while extending phases > does not requires any changes) > > Implementation-wise, adding "internal" through phase should be quite > trivial, we already have a solid phase implementation. Include fast C > code to computes revision in each phases. And the hiding API support > multiple source for feeding "internal" phase to it will be trivial. > > > To conclude, having "internal-changeset" as an explicit concept will > help building better/simpler UI. From my analysis, using phases for them > provides us with a simpler implementation both -right- -now- and -later- > even in the presence of a local hiding mechanism. > > What do you think ? > > Cheers, >
On Tue, Apr 4, 2017 at 12:06 PM, Jun Wu <quark@fb.com> wrote: > Since most people want a separate hidden storage that handles wider cases, I > don't think this incomplete (internal-only) solution worths investment. I can't say I've followed everything in this long discussion (including the multiple threads), but my gut feeling is that I agree with Jun. I really like the idea of separating hiddenness from obsolescence and I feel like introducing that separation would be a good first step. I'm sure there will be plenty of corner cases related to just that piece (Pierre-Yves pointed out the case of pruning and then re-pulling, for example), so getting that in (most likely hidden behind an [experimental] option) and seeing how it works out would be very useful. It does seem like most people agree on that separation being the right thing to do. I'd prefer to get that reasonably done before we spend too much time discussing the next step (unless the next step seems to make the first step obsolete, but it hasn't seemed that way so far). > > Excerpts from Pierre-Yves David's message of 2017-04-04 20:26:29 +0200: >> TL;DR; having an internal changeset concept help building simpler UI. >> Implementation-wise, phases offer a fitting home for the concept while >> allow simpler implementation now and later. Moreover, this can be done >> sooner and independently from other discussions. >> >> On 04/03/2017 06:02 PM, Jun Wu wrote: >> > Excerpts from Pierre-Yves David's message of 2017-04-03 13:11:10 +0200: >> >> Local-hidding still needs its own solution. >> > >> > If we do have root-based hidden, and it's used for both "strip" and this >> > "local-only" thing. >> >> Let us make a small vocabulary point first (because I think it is >> important to be on the same pages here). >> >> * "strip" is the action of "physically" removing data from the >> repository. And this should remains so. The word is accurate and >> describe an operation that is sometimes useful and will stay around in >> the future. >> I know that Facebook use this word for something else but we should >> stick to strict semantic here to avoid confusion. >> >> * "root-based hidden" is a possible implementation for hiding. At that >> point of the discussion it would better to stay at the concept level >> "local-hiding" or "local-only-hiding" seems describe the concept you >> have in mind well. "root-based" is one of the many possible >> implementation. Until we have actually reviewed use-cases and option we >> are not sure we'll use that implementation for that concept, in practice >> a bitemap-based or ranged-based approach might turn out a better choice. >> In addition such description could match implementation for other >> concept. For example this internal phases is also "root-based" since >> phases are "root-based" >> >> > What problem will that cause? Worst case we add a bit >> > per root saying "whether this is by user or by system". But I don't think >> > they are fundamentally different that requires two different solutions. >> >> As mentioned in previous messages, we could use a generic local-hiding >> mechanism to hide internal changesets. I do not think this will create >> major problem. >> >> They are still multiple advantages to have handled by a new phases: >> >> * It fits well into the phases concept "public, draft, secret, internal" >> feel natural, >> >> * We already have all the visualization and manipulation we want for >> phases, so the "internal-changeset" comes at a very low overhead >> extension to an existing concept, >> >> * clear distinction between hidden internal-changeset and hidden >> real-changesets will help offer a clean UI around locally-hidden >> changesets. (To avoid internals polluting user output related to hidden-ess) >> >> * Adding extra bits related to internal in local-hiding will make >> local-hiding implementation and UI more complex. (while extending phases >> does not requires any changes) >> >> Implementation-wise, adding "internal" through phase should be quite >> trivial, we already have a solid phase implementation. Include fast C >> code to computes revision in each phases. And the hiding API support >> multiple source for feeding "internal" phase to it will be trivial. >> >> >> To conclude, having "internal-changeset" as an explicit concept will >> help building better/simpler UI. From my analysis, using phases for them >> provides us with a simpler implementation both -right- -now- and -later- >> even in the presence of a local hiding mechanism. >> >> What do you think ? >> >> Cheers, >> > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
On 4/4/17 9:07 PM, Martin von Zweigbergk via Mercurial-devel wrote: > On Tue, Apr 4, 2017 at 12:06 PM, Jun Wu <quark@fb.com> wrote: >> Since most people want a separate hidden storage that handles wider cases, I >> don't think this incomplete (internal-only) solution worths investment. > I can't say I've followed everything in this long discussion > (including the multiple threads), but my gut feeling is that I agree > with Jun. I really like the idea of separating hiddenness from > obsolescence and I feel like introducing that separation would be a > good first step. I'm sure there will be plenty of corner cases related > to just that piece (Pierre-Yves pointed out the case of pruning and > then re-pulling, for example), so getting that in (most likely hidden > behind an [experimental] option) and seeing how it works out would be > very useful. It does seem like most people agree on that separation > being the right thing to do. I'd prefer to get that reasonably done > before we spend too much time discussing the next step (unless the > next step seems to make the first step obsolete, but it hasn't seemed > that way so far). > Sorry for being late here, I was swamped with interviews at the beginning of the week. Finally getting some space to catch up here. I've managed to read the various threads and the back-and-forth here. My overall thoughts are: (1) I also agree that having a internal-core-hg hiding system that is fast, scalable, and independent of all other concepts is highly desirable. I imagine this being built on top of a bitmap-style system because that's how I can see it being fast, but this is an implementation detail. (2) I think that new concepts like "internal-only", "extinct", and "archived" will be able to interact with this internal hiddenness concept however they wish -- and we can try new things without getting wrapped up discussions about whether we're abusing one concept for one of it's side effects (eg, abusing obsolescence markers because they happen to hide things today). I think avoiding abusing obsmarkers this way is also highly desirable, and having a distinct hiding mechanism allows us to avoid this abuse. As far as I can tell, everyone is okay with an internal-core-hg hiding concept that their systems will choose how to interact with -- be it obsolescence or arching or whatever else we come up with. (3) Thanks to Pierre-Yves for the detailed analysis of verious mechanisms for implementing internal-only changesets. This is super valuable, especially the parts about why a changeset marker alone is insufficient, and the analysis of the work involved to implement various ideas. You went way above and beyond what my question was intended to solicit. I was honestly just looking for something like "yeah, I've thought about this and I think phases could work" or "I think phases won't work because of this problem" -- but you wrote up a design doc! Amazing. (4) In response to the design proposal, I agree that it feels over-complicated to me for what I would be trying to accomplish with the feature. I agree with Jun that we can use "dynamic unhiding" for finding the changesets we're interested in, so I don't think we need two new phases, and I think that losing the strict ordering of phases would be too much pain to introduce for this feature. So, my proposal would be a single phase just above "secret" called "internal" that, like secret, would never be exchanged, and would be hidden by default (unless dynamically unhidden via being checked out or directly accessed, for example). I'm against forbidding the user to do things with this changeset -- I always opt to let the user do things, in general, that they ask to do. If they want or run diff or update or whatever to it, sure, let them. The nice thing about having a generic hiding mechanism [item (2) above] is that we don't have to worry about the details of how things are hidden. Overall, I think having a "default hidden phase that doesn't get exchanged like secret" would get us everything we want, and we don't need to introduce new flags or hard concepts into phases. It's just another phase. In conclusion, I propose that we shelve (heh) this discussion about specific things that might use hiddenness until we something like #2 above, after which we can experiment much more easily in extensions with the right ways to hide and unhide things, without abusing concepts like obsolescence. That way, we can build these things in a clean and composable manner instead of twisting things up like we've been doing with inhibit at FB (painfully, I might add). I think Durham recently sent out a new thread about something like concept #2 above, so I'll go read that now and hope it's more or less what I'm thinking :-) ~Ryan
On 04/04/2017 09:06 PM, Jun Wu wrote: > Since most people want a separate hidden storage that handles wider cases, I > don't think this incomplete (internal-only) solution worths investment. They seems to be misunderstanding here. We should probably jump on a Face to Face medium What I've been trying to point out here is that separating internal-changesets from real-changesets has value (and even seems necessary to build a good UI). And that phases seems a good choice to make "internal" distinction. And I do not see you answering these two points. I understand you want another independent hiding mechanism for change-set, This is not incompatible with the current proposal. As we do not seems to make progress with email, would you be available to discuss this over Video Conference? Cheers,
On 4/5/17 11:40 AM, Ryan McElroy wrote: > On 4/4/17 9:07 PM, Martin von Zweigbergk via Mercurial-devel wrote: >> On Tue, Apr 4, 2017 at 12:06 PM, Jun Wu <quark@fb.com> wrote: >>> Since most people want a separate hidden storage that handles wider >>> cases, I >>> don't think this incomplete (internal-only) solution worths investment. >> I can't say I've followed everything in this long discussion >> (including the multiple threads), but my gut feeling is that I agree >> with Jun. I really like the idea of separating hiddenness from >> obsolescence and I feel like introducing that separation would be a >> good first step. I'm sure there will be plenty of corner cases related >> to just that piece (Pierre-Yves pointed out the case of pruning and >> then re-pulling, for example), so getting that in (most likely hidden >> behind an [experimental] option) and seeing how it works out would be >> very useful. It does seem like most people agree on that separation >> being the right thing to do. I'd prefer to get that reasonably done >> before we spend too much time discussing the next step (unless the >> next step seems to make the first step obsolete, but it hasn't seemed >> that way so far). >> > > Sorry for being late here, I was swamped with interviews at the > beginning of the week. Finally getting some space to catch up here. > > I've managed to read the various threads and the back-and-forth here. > > My overall thoughts are: > > (1) I also agree that having a internal-core-hg hiding system that is > fast, scalable, and independent of all other concepts is highly > desirable. I imagine this being built on top of a bitmap-style system > because that's how I can see it being fast, but this is an > implementation detail. > > (2) I think that new concepts like "internal-only", "extinct", and > "archived" will be able to interact with this internal hiddenness > concept however they wish -- and we can try new things without getting > wrapped up discussions about whether we're abusing one concept for one > of it's side effects (eg, abusing obsolescence markers because they > happen to hide things today). I think avoiding abusing obsmarkers this > way is also highly desirable, and having a distinct hiding mechanism > allows us to avoid this abuse. > > As far as I can tell, everyone is okay with an internal-core-hg hiding > concept that their systems will choose how to interact with -- be it > obsolescence or arching or whatever else we come up with. > > (3) Thanks to Pierre-Yves for the detailed analysis of verious > mechanisms for implementing internal-only changesets. This is super > valuable, especially the parts about why a changeset marker alone is > insufficient, and the analysis of the work involved to implement > various ideas. You went way above and beyond what my question was > intended to solicit. I was honestly just looking for something like > "yeah, I've thought about this and I think phases could work" or "I > think phases won't work because of this problem" -- but you wrote up a > design doc! Amazing. > > (4) In response to the design proposal, I agree that it feels > over-complicated to me for what I would be trying to accomplish with > the feature. I agree with Jun that we can use "dynamic unhiding" for > finding the changesets we're interested in, so I don't think we need > two new phases, and I think that losing the strict ordering of phases > would be too much pain to introduce for this feature. So, my proposal > would be a single phase just above "secret" called "internal" that, > like secret, would never be exchanged, and would be hidden by default > (unless dynamically unhidden via being checked out or directly > accessed, for example). I'm against forbidding the user to do things > with this changeset -- I always opt to let the user do things, in > general, that they ask to do. If they want or run diff or update or > whatever to it, sure, let them. The nice thing about having a generic > hiding mechanism [item (2) above] is that we don't have to worry about > the details of how things are hidden. Overall, I think having a > "default hidden phase that doesn't get exchanged like secret" would > get us everything we want, and we don't need to introduce new flags or > hard concepts into phases. It's just another phase. Re-reading this, I think it might be confusing that I'm suggesting this phase be a generic way to hide things. I'm not. To be clear, I'm proposing this new phase as a possible user of the generic hiding system -- and only as a way to give shelve (and things like shelve -- eg, amend) a non-stripping, non-obsmarker path forward. Still, see below. I think it's worth shelving this until we have a generic hiding system. > > In conclusion, I propose that we shelve (heh) this discussion about > specific things that might use hiddenness until we something like #2 > above, after which we can experiment much more easily in extensions > with the right ways to hide and unhide things, without abusing > concepts like obsolescence. That way, we can build these things in a > clean and composable manner instead of twisting things up like we've > been doing with inhibit at FB (painfully, I might add). > > I think Durham recently sent out a new thread about something like > concept #2 above, so I'll go read that now and hope it's more or less > what I'm thinking :-) > > ~Ryan
Excerpts from Pierre-Yves David's message of 2017-04-05 13:28:57 +0200: > > On 04/04/2017 09:06 PM, Jun Wu wrote: > > Since most people want a separate hidden storage that handles wider cases, I > > don't think this incomplete (internal-only) solution worths investment. > > They seems to be misunderstanding here. We should probably jump on a > Face to Face medium > > What I've been trying to point out here is that separating > internal-changesets from real-changesets has value (and even seems > necessary to build a good UI). And that phases seems a good choice to > make "internal" distinction. And I do not see you answering these two > points. > > I understand you want another independent hiding mechanism for > change-set, This is not incompatible with the current proposal. > > As we do not seems to make progress with email, would you be available > to discuss this over Video Conference? There are some questions that I'm waiting for your answers: - Define "user intention" about adding a obsmarker formally [1] - Why "an unified hidden store" does not work [2] I'd appreciate if you can answer them directly and publicly. Thanks! Besides, I think you need to convince others, other than just me, because most people seem to like the unified hidden store idea very much. [1]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-March/095686.html [2]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-April/096268.html > > Cheers, >
Excerpts from Pierre-Yves David's message of 2017-04-05 13:28:57 +0200: > > On 04/04/2017 09:06 PM, Jun Wu wrote: > > Since most people want a separate hidden storage that handles wider cases, I > > don't think this incomplete (internal-only) solution worths investment. > > They seems to be misunderstanding here. We should probably jump on a > Face to Face medium > > What I've been trying to point out here is that separating > internal-changesets from real-changesets has value (and even seems > necessary to build a good UI). And that phases seems a good choice to > make "internal" distinction. And I do not see you answering these two > points. To answer these, the planned general-purposed, root-based, non-phase hidden store will cover most use-cases of the internal-only, phase-based hidden. And it's much more useful than your proposal. I think it's unnecessary to have "internal changeset" concept separated from the general-purposed hidden. But if the community do decide to implement the internal changeset concept, I think that could be doable, but it should be done in the new non-phase store, instead of adding a new phase. Therefore, I'm -1 on adding a new phase in all cases, since there are better choices. > I understand you want another independent hiding mechanism for > change-set, This is not incompatible with the current proposal. > > As we do not seems to make progress with email, would you be available > to discuss this over Video Conference? > > Cheers, >
Patch
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py --- a/mercurial/localrepo.py +++ b/mercurial/localrepo.py @@ -50,6 +50,7 @@ from . import ( phases, pushkey, pycompat, + repair, repoview, revset, revsetlang, @@ -1884,6 +1885,23 @@ class localrepository(object): # tag cache retrieval" case to work. self.invalidate() + def hidenodes(self, nodes, insistonstripping=False): + '''Remove nodes from visible repoview in the most appropriate + supported way. When obsmarker creation is enabled, this + function will obsolete these nodes without successors + (unless insistonstripping is True). Otherwise, it will + strip nodes. + + In future we can add other node-hiding capabilities to this + function.''' + with self.lock(): + if obsolete.isenabled(self, obsolete.createmarkersopt)\ + and not insistonstripping: + relations = [(self[n], ()) for n in nodes] + obsolete.createmarkers(self, relations) + else: + repair.strip(self.ui, self, nodes, backup=False) + def walk(self, match, node=None): ''' walk recursively through the directory tree or a given diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t --- a/tests/test-obsolete.t +++ b/tests/test-obsolete.t @@ -1259,3 +1259,43 @@ Test the --delete option of debugobsolet 1ab51af8f9b41ef8c7f6f3312d4706d870b1fb74 29346082e4a9e27042b62d2da0e2de211c027621 0 \(.*\) {'user': 'test'} (re) $ cd .. +Test that repo.hidenodes respects obsolescense configs + $ hg init hidenodesrepo && cd hidenodesrepo + $ cat <<EOF > debughidenodes.py + > from __future__ import absolute_import + > from mercurial import ( + > cmdutil, + > ) + > cmdtable = {} + > command = cmdutil.command(cmdtable) + > @command('debughidenodes', + > [('', 'insiststrip', None, 'pass insistonstripping=True')]) + > def debughidenodes(ui, repo, *args, **opts): + > nodes = [repo[n].node() for n in args] + > repo.hidenodes(nodes, insistonstripping=bool(opts.get('insiststrip'))) + > EOF + $ cat <<EOF > .hg/hgrc + > [extensions] + > debughidenodes=debughidenodes.py + > EOF + $ echo a > a + $ hg add a && hg ci -m a && hg up null + 0 files updated, 0 files merged, 1 files removed, 0 files unresolved + $ hg --config experimental.evolution=! debughidenodes 0 + $ hg log -qr "all()" --hidden | wc -l # commit was actually stripped + \s*0 (re) + $ hg debugobsolete | wc -l # no markers were created + \s*0 (re) + $ echo a > a && hg add a && hg ci -m a && hg up null + 0 files updated, 0 files merged, 1 files removed, 0 files unresolved + $ hg --config experimental.evolution=createmarkers debughidenodes 0 + $ hg log -qr "all()" --hidden | wc -l # commit was not stripped + \s*1 (re) + $ hg debugobsolete | wc -l # a marker was created + \s*1 (re) + $ hg --config experimental.evolution=createmarkers --hidden debughidenodes 0 --insiststrip + $ hg log -qr "all()" --hidden | wc -l # commit was actually stripped + \s*0 (re) + $ hg debugobsolete | wc -l # no new markers were created + \s*1 (re) + $ cd ..