Submitter | Pierre-Yves David |
---|---|
Date | March 22, 2017, 4:31 p.m. |
Message ID | <72c820f77671d4495d96.1490200274@nodosa.octopoid.net> |
Download | mbox | patch |
Permalink | /patch/19582/ |
State | Accepted |
Headers | show |
Comments
On 3/22/17 4:31 PM, Pierre-Yves David wrote: > # HG changeset patch > # User Pierre-Yves David <pierre-yves.david@ens-lyon.org> > # Date 1490135413 -3600 > # Tue Mar 21 23:30:13 2017 +0100 > # Node ID 72c820f77671d4495d96e7aeeba9237573dbce85 > # Parent 66c3ae6d886cae0e3a3cff6a0058e2d2a866fd9d > # EXP-Topic checkheads > > The checkheads function is long and complex, extract that logic in a subfunction > is win in itself. As the comment in the code says, this postprocessing is > currently very basic and either misbehave or fails to detect valid push in many > cases. My deeper motive for this extraction is to be make it easier to provide > extensive testing of this case and strategy to cover them. Final test and logic > will makes it to core once done. > > diff --git a/mercurial/discovery.py b/mercurial/discovery.py > --- a/mercurial/discovery.py > +++ b/mercurial/discovery.py > @@ -343,38 +343,13 @@ def checkheads(pushop): > oldhs.update(unsyncedheads) > candidate_newhs.update(unsyncedheads) > dhs = None # delta heads, the new heads on branch > - discardedheads = set() > if not repo.obsstore: > + discardedheads = set() > newhs = candidate_newhs > else: > - # remove future heads which are actually obsoleted by another > - # pushed element: > - # > - # XXX as above, There are several cases this code does not handle > - # XXX properly > - # > - # (1) if <nh> is public, it won't be affected by obsolete marker > - # and a new is created > - # > - # (2) if the new heads have ancestors which are not obsolete and > - # not ancestors of any other heads we will have a new head too. > - # > - # These two cases will be easy to handle for known changeset but > - # much more tricky for unsynced changes. > - # > - # In addition, this code is confused by prune as it only looks for > - # successors of the heads (none if pruned) leading to issue4354 > - newhs = set() > - for nh in candidate_newhs: > - if nh in repo and repo[nh].phase() <= phases.public: > - newhs.add(nh) > - else: > - for suc in obsolete.allsuccessors(repo.obsstore, [nh]): > - if suc != nh and suc in allfuturecommon: > - discardedheads.add(nh) > - break > - else: > - newhs.add(nh) > + newhs, discardedheads = _postprocessobsolete(pushop, > + allfuturecommon, > + candidate_newhs) > unsynced = sorted(h for h in unsyncedheads if h not in discardedheads) > if unsynced: > if None in unsynced: > @@ -434,3 +409,42 @@ def checkheads(pushop): > repo.ui.note((" %s\n") % short(h)) > if errormsg: > raise error.Abort(errormsg, hint=hint) > + > +def _postprocessobsolete(pushop, futurecommon, candidate_newhs): Indeed, passing pushop does seem better. > + """post process the list of new heads with obsolescence information > + > + Exist as a subfunction to contains the complexity and allow extensions to Grammar nit: "Exists as a subfunction to contain..." (move the s from contains to exist). That can be fixed in flight and isn't too important for understanding anyway. > + experiment with smarter logic. > + Returns (newheads, discarded_heads) tuple > + """ > + # remove future heads which are actually obsoleted by another > + # pushed element: > + # > + # XXX as above, There are several cases this code does not handle > + # XXX properly > + # > + # (1) if <nh> is public, it won't be affected by obsolete marker > + # and a new is created > + # > + # (2) if the new heads have ancestors which are not obsolete and > + # not ancestors of any other heads we will have a new head too. > + # > + # These two cases will be easy to handle for known changeset but > + # much more tricky for unsynced changes. > + # > + # In addition, this code is confused by prune as it only looks for > + # successors of the heads (none if pruned) leading to issue4354 > + repo = pushop.repo > + newhs = set() > + discarded = set() > + for nh in candidate_newhs: > + if nh in repo and repo[nh].phase() <= phases.public: > + newhs.add(nh) > + else: > + for suc in obsolete.allsuccessors(repo.obsstore, [nh]): > + if suc != nh and suc in futurecommon: > + discarded.add(nh) > + break > + else: > + newhs.add(nh) > + return newhs, discarded > Nice cleanup, lgtm.
On Thu, Mar 23, 2017 at 09:52:59AM +0000, Ryan McElroy wrote: > On 3/22/17 4:31 PM, Pierre-Yves David wrote: > ># HG changeset patch > ># User Pierre-Yves David <pierre-yves.david@ens-lyon.org> > ># Date 1490135413 -3600 > ># Tue Mar 21 23:30:13 2017 +0100 > ># Node ID 72c820f77671d4495d96e7aeeba9237573dbce85 > ># Parent 66c3ae6d886cae0e3a3cff6a0058e2d2a866fd9d > ># EXP-Topic checkheads > > > >The checkheads function is long and complex, extract that logic in a subfunction > >is win in itself. As the comment in the code says, this postprocessing is > >currently very basic and either misbehave or fails to detect valid push in many > >cases. My deeper motive for this extraction is to be make it easier to provide > >extensive testing of this case and strategy to cover them. Final test and logic > >will makes it to core once done. [...] > Nice cleanup, lgtm. Queued, thanks. > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Patch
diff --git a/mercurial/discovery.py b/mercurial/discovery.py --- a/mercurial/discovery.py +++ b/mercurial/discovery.py @@ -343,38 +343,13 @@ def checkheads(pushop): oldhs.update(unsyncedheads) candidate_newhs.update(unsyncedheads) dhs = None # delta heads, the new heads on branch - discardedheads = set() if not repo.obsstore: + discardedheads = set() newhs = candidate_newhs else: - # remove future heads which are actually obsoleted by another - # pushed element: - # - # XXX as above, There are several cases this code does not handle - # XXX properly - # - # (1) if <nh> is public, it won't be affected by obsolete marker - # and a new is created - # - # (2) if the new heads have ancestors which are not obsolete and - # not ancestors of any other heads we will have a new head too. - # - # These two cases will be easy to handle for known changeset but - # much more tricky for unsynced changes. - # - # In addition, this code is confused by prune as it only looks for - # successors of the heads (none if pruned) leading to issue4354 - newhs = set() - for nh in candidate_newhs: - if nh in repo and repo[nh].phase() <= phases.public: - newhs.add(nh) - else: - for suc in obsolete.allsuccessors(repo.obsstore, [nh]): - if suc != nh and suc in allfuturecommon: - discardedheads.add(nh) - break - else: - newhs.add(nh) + newhs, discardedheads = _postprocessobsolete(pushop, + allfuturecommon, + candidate_newhs) unsynced = sorted(h for h in unsyncedheads if h not in discardedheads) if unsynced: if None in unsynced: @@ -434,3 +409,42 @@ def checkheads(pushop): repo.ui.note((" %s\n") % short(h)) if errormsg: raise error.Abort(errormsg, hint=hint) + +def _postprocessobsolete(pushop, futurecommon, candidate_newhs): + """post process the list of new heads with obsolescence information + + Exist as a subfunction to contains the complexity and allow extensions to + experiment with smarter logic. + Returns (newheads, discarded_heads) tuple + """ + # remove future heads which are actually obsoleted by another + # pushed element: + # + # XXX as above, There are several cases this code does not handle + # XXX properly + # + # (1) if <nh> is public, it won't be affected by obsolete marker + # and a new is created + # + # (2) if the new heads have ancestors which are not obsolete and + # not ancestors of any other heads we will have a new head too. + # + # These two cases will be easy to handle for known changeset but + # much more tricky for unsynced changes. + # + # In addition, this code is confused by prune as it only looks for + # successors of the heads (none if pruned) leading to issue4354 + repo = pushop.repo + newhs = set() + discarded = set() + for nh in candidate_newhs: + if nh in repo and repo[nh].phase() <= phases.public: + newhs.add(nh) + else: + for suc in obsolete.allsuccessors(repo.obsstore, [nh]): + if suc != nh and suc in futurecommon: + discarded.add(nh) + break + else: + newhs.add(nh) + return newhs, discarded