| Submitter | via Mercurial-devel |
|---|---|
| Date | May 28, 2017, 6:15 a.m. |
| Message ID | <aef3194855e763ecced8.1495952131@martinvonz.svl.corp.google.com> |
| Download | mbox | patch |
| Permalink | /patch/20968/ |
| State | Changes Requested |
| Headers | show |
Comments
On 05/28/2017 08:15 AM, Martin von Zweigbergk wrote: > # HG changeset patch > # User Martin von Zweigbergk <martinvonz@google.com> > # Date 1495945026 25200 > # Sat May 27 21:17:06 2017 -0700 > # Node ID aef3194855e763ecced80c2df6bd9672207aefbc > # Parent a0434758fd9a95ea3807ff4766eaedff6852c628 > hidden: change _domainancestors() to _revealancestors() > > This change makes the function will actually reveal the ancestors by > removing them from the hidden set. This prepares for further > simplification. That change looks good to me but for a small feedback on the function signature. > diff --git a/mercurial/repoview.py b/mercurial/repoview.py > --- a/mercurial/repoview.py > +++ b/mercurial/repoview.py > @@ -59,9 +59,11 @@ > break > return blockers > > -def _domainancestors(pfunc, revs, domain): > - """return ancestors of 'revs' within 'domain' > +def _revealancestors(hidden, pfunc, revs, domain): small nit: other functions that use "pfunc" always takes it as first parameter. I would rather see "pfunc" remains the first parameters here too.
On May 29, 2017 9:50 AM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote: On 05/28/2017 08:15 AM, Martin von Zweigbergk wrote: > # HG changeset patch > # User Martin von Zweigbergk <martinvonz@google.com> > # Date 1495945026 25200 > # Sat May 27 21:17:06 2017 -0700 > # Node ID aef3194855e763ecced80c2df6bd9672207aefbc > # Parent a0434758fd9a95ea3807ff4766eaedff6852c628 > hidden: change _domainancestors() to _revealancestors() > > This change makes the function will actually reveal the ancestors by > removing them from the hidden set. This prepares for further > simplification. > That change looks good to me but for a small feedback on the function signature. diff --git a/mercurial/repoview.py b/mercurial/repoview.py > --- a/mercurial/repoview.py > +++ b/mercurial/repoview.py > @@ -59,9 +59,11 @@ > break > return blockers > > -def _domainancestors(pfunc, revs, domain): > - """return ancestors of 'revs' within 'domain' > +def _revealancestors(hidden, pfunc, revs, domain): > small nit: other functions that use "pfunc" always takes it as first parameter. I would rather see "pfunc" remains the first parameters here too. 'hidden' gets mutated, and mutated parameters are usually first, I think (including, but probably not limited to, 'self'). Don't you think that should trump the pfunc-first convention?
On 05/29/2017 11:54 PM, Martin von Zweigbergk wrote: > > > On May 29, 2017 9:50 AM, "Pierre-Yves David" > <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>> > wrote: > > > > On 05/28/2017 08:15 AM, Martin von Zweigbergk wrote: > > # HG changeset patch > # User Martin von Zweigbergk <martinvonz@google.com > <mailto:martinvonz@google.com>> > # Date 1495945026 25200 > # Sat May 27 21:17:06 2017 -0700 > # Node ID aef3194855e763ecced80c2df6bd9672207aefbc > # Parent a0434758fd9a95ea3807ff4766eaedff6852c628 > hidden: change _domainancestors() to _revealancestors() > > This change makes the function will actually reveal the ancestors by > removing them from the hidden set. This prepares for further > simplification. > > > That change looks good to me but for a small feedback on the > function signature. > > > diff --git a/mercurial/repoview.py b/mercurial/repoview.py > --- a/mercurial/repoview.py > +++ b/mercurial/repoview.py > @@ -59,9 +59,11 @@ > break > return blockers > > -def _domainancestors(pfunc, revs, domain): > - """return ancestors of 'revs' within 'domain' > +def _revealancestors(hidden, pfunc, revs, domain): > > > small nit: other functions that use "pfunc" always takes it as first > parameter. I would rather see "pfunc" remains the first parameters > here too. > > > 'hidden' gets mutated, and mutated parameters are usually first, I think > (including, but probably not limited to, 'self'). Don't you think that > should trump the pfunc-first convention? I don't think we follow the "mutated" first convention here[1]. On the other hand I can think of multiples instance of argument that has "canonical" location (eg: ui, repo) [1] you might have data saying otherwise. I'll be happy to change my mind if you have some. Cheers,
On Tue, May 30, 2017 at 1:53 AM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote: > On 05/29/2017 11:54 PM, Martin von Zweigbergk wrote: >> >> >> >> On May 29, 2017 9:50 AM, "Pierre-Yves David" >> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>> >> wrote: >> >> >> >> On 05/28/2017 08:15 AM, Martin von Zweigbergk wrote: >> >> # HG changeset patch >> # User Martin von Zweigbergk <martinvonz@google.com >> <mailto:martinvonz@google.com>> >> >> # Date 1495945026 25200 >> # Sat May 27 21:17:06 2017 -0700 >> # Node ID aef3194855e763ecced80c2df6bd9672207aefbc >> # Parent a0434758fd9a95ea3807ff4766eaedff6852c628 >> hidden: change _domainancestors() to _revealancestors() >> >> This change makes the function will actually reveal the ancestors >> by >> removing them from the hidden set. This prepares for further >> simplification. >> >> >> That change looks good to me but for a small feedback on the >> function signature. >> >> >> diff --git a/mercurial/repoview.py b/mercurial/repoview.py >> --- a/mercurial/repoview.py >> +++ b/mercurial/repoview.py >> @@ -59,9 +59,11 @@ >> break >> return blockers >> >> -def _domainancestors(pfunc, revs, domain): >> - """return ancestors of 'revs' within 'domain' >> +def _revealancestors(hidden, pfunc, revs, domain): >> >> >> small nit: other functions that use "pfunc" always takes it as first >> parameter. I would rather see "pfunc" remains the first parameters >> here too. >> >> >> 'hidden' gets mutated, and mutated parameters are usually first, I think >> (including, but probably not limited to, 'self'). Don't you think that >> should trump the pfunc-first convention? > > > I don't think we follow the "mutated" first convention here[1]. On the other > hand I can think of multiples instance of argument that has "canonical" > location (eg: ui, repo) I can't think of any examples of mutable arguments at all (which is good), so I'm not sure if we're following any convention for them. I'll switch to your preferred form. > > [1] you might have data saying otherwise. I'll be happy to change my mind if > you have some. > > Cheers, > > -- > Pierre-Yves David
Patch
diff --git a/mercurial/repoview.py b/mercurial/repoview.py --- a/mercurial/repoview.py +++ b/mercurial/repoview.py @@ -59,9 +59,11 @@ break return blockers -def _domainancestors(pfunc, revs, domain): - """return ancestors of 'revs' within 'domain' +def _revealancestors(hidden, pfunc, revs, domain): + """reveals contiguous chains of hidden ancestors of 'revs' within 'domain' + by removing them from 'hidden' + - hidden: the (preliminary) hidden revisions, to be updated - pfunc(r): a funtion returning parent of 'r', - revs: iterable of revnum, - domain: consistent set of revnum. @@ -85,16 +87,16 @@ If C, D, E and F are in the domain but B is not, A cannot be ((A) is an ancestors disconnected subset disconnected of (C+D)). - (Ancestors are returned inclusively) + (Ancestors are revealed inclusively, i.e. the elements in 'revs' are + also revealed) """ stack = list(revs) - ancestors = set(stack) + hidden -= set(stack) while stack: for p in pfunc(stack.pop()): - if p != nullrev and p in domain and p not in ancestors: - ancestors.add(p) + if p != nullrev and p in domain and p in hidden: + hidden.remove(p) stack.append(p) - return ancestors def computehidden(repo): """compute the set of hidden revision to filter @@ -114,7 +116,9 @@ # changesets and remove those. blockers |= (hidden & anchorrevs(repo)) if blockers: - hidden = hidden - _domainancestors(pfunc, blockers, mutable) + # don't modify possibly cached result of hideablerevs() + hidden = hidden.copy() + _revealancestors(hidden, pfunc, blockers, mutable) return frozenset(hidden) def computeunserved(repo):