| Submitter | phabricator |
|---|---|
| Date | Sept. 21, 2020, 8:38 p.m. |
| Message ID | <differential-rev-PHID-DREV-xlldmrknt4aw3yhalcpb-req@mercurial-scm.org> |
| Download | mbox | patch |
| Permalink | /patch/47236/ |
| State | Superseded |
| Headers | show |
Comments
[Moving a discussion we are having on #hg-evolve here to reach more people] The clarification of interface seems great overall. However I am a bit worried about the drop of the initial `repo` argument to these function. Passing the repository explicitly seems safer to me. It make sure we are updating the right repository, and more importantly with the right repoview (filtering level). It would be easy for some fault code to reuse output from a function using relaxed filtering to use in a more filtered context. explicitly passing the repository catch that. What do you think ? On 9/21/20 10:38 PM, martinvonz (Martin von Zweigbergk) wrote: > martinvonz created this revision. > Herald added a reviewer: hg-reviewers. > Herald added a subscriber: mercurial-patches. > > REVISION SUMMARY > This is different from the `update()` function that I just made > private. The new function is specifically for the normal `hg update` > use case. It doesn't do a merge and it doesn't do a clean (forced) > update. > > REPOSITORY > rHG Mercurial > > BRANCH > default > > REVISION DETAIL > https://phab.mercurial-scm.org/D9066 > > AFFECTED FILES > hgext/transplant.py > mercurial/merge.py > > CHANGE DETAILS > > diff --git a/mercurial/merge.py b/mercurial/merge.py > --- a/mercurial/merge.py > +++ b/mercurial/merge.py > @@ -2133,6 +2133,23 @@ > ) > > > +def update(ctx, updatecheck=None, wc=None): > + """Do a regular update to the given commit, aborting if there are conflicts. > + > + The 'updatecheck' argument can be used to control what to do in case of > + conflicts. > + """ > + return _update( > + ctx.repo(), > + ctx.rev(), > + branchmerge=False, > + force=False, > + labels=[b'working copy', b'destination'], > + updatecheck=updatecheck, > + wc=wc, > + ) > + > + > def clean_update(ctx, wc=None): > """Do a clean update to the given commit. > > diff --git a/hgext/transplant.py b/hgext/transplant.py > --- a/hgext/transplant.py > +++ b/hgext/transplant.py > @@ -198,9 +198,7 @@ > if pulls: > if source != repo: > exchange.pull(repo, source.peer(), heads=pulls) > - merge._update( > - repo, pulls[-1], branchmerge=False, force=False > - ) > + merge.update(repo[pulls[-1]]) > p1 = repo.dirstate.p1() > pulls = [] > > @@ -275,7 +273,7 @@ > tr.close() > if pulls: > exchange.pull(repo, source.peer(), heads=pulls) > - merge._update(repo, pulls[-1], branchmerge=False, force=False) > + merge.update(repo[pulls[-1]]) > finally: > self.saveseries(revmap, merges) > self.transplants.write() > > > > To: martinvonz, #hg-reviewers > Cc: mercurial-patches, mercurial-devel > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel >
martinvonz added a comment. In D9066#137315 <https://phab.mercurial-scm.org/D9066#137315>, @marmoute wrote: > [Moving a discussion we are having on #hg-evolve here to reach more people] > The clarification of interface seems great overall. However I am a bit > worried about the drop of the initial `repo` argument to these function. > Passing the repository explicitly seems safer to me. It make sure we are > updating the right repository, and more importantly with the right > repoview (filtering level). It would be easy for some fault code to > reuse output from a function using relaxed filtering to use in a more > filtered context. explicitly passing the repository catch that. > What do you think ? I can see the concern at some abstract level, but I don't know what you mean more concretely. What would such bad callers look like? Also, does your concern apply to most places we pass around context objects? REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D9066/new/ REVISION DETAIL https://phab.mercurial-scm.org/D9066 To: martinvonz, #hg-reviewers, pulkit Cc: marmoute, mercurial-devel, pulkit, mercurial-patches
Patch
diff --git a/mercurial/merge.py b/mercurial/merge.py --- a/mercurial/merge.py +++ b/mercurial/merge.py @@ -2133,6 +2133,23 @@ ) +def update(ctx, updatecheck=None, wc=None): + """Do a regular update to the given commit, aborting if there are conflicts. + + The 'updatecheck' argument can be used to control what to do in case of + conflicts. + """ + return _update( + ctx.repo(), + ctx.rev(), + branchmerge=False, + force=False, + labels=[b'working copy', b'destination'], + updatecheck=updatecheck, + wc=wc, + ) + + def clean_update(ctx, wc=None): """Do a clean update to the given commit. diff --git a/hgext/transplant.py b/hgext/transplant.py --- a/hgext/transplant.py +++ b/hgext/transplant.py @@ -198,9 +198,7 @@ if pulls: if source != repo: exchange.pull(repo, source.peer(), heads=pulls) - merge._update( - repo, pulls[-1], branchmerge=False, force=False - ) + merge.update(repo[pulls[-1]]) p1 = repo.dirstate.p1() pulls = [] @@ -275,7 +273,7 @@ tr.close() if pulls: exchange.pull(repo, source.peer(), heads=pulls) - merge._update(repo, pulls[-1], branchmerge=False, force=False) + merge.update(repo[pulls[-1]]) finally: self.saveseries(revmap, merges) self.transplants.write()