Patchwork D9066: merge: add a higher-level update() for the common `hg update` use case

login
register
mail settings
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

phabricator - Sept. 21, 2020, 8:38 p.m.
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




To: martinvonz, #hg-reviewers
Cc: mercurial-patches, mercurial-devel
Pierre-Yves David - Oct. 2, 2020, 6:34 p.m.
[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
>
phabricator - Oct. 2, 2020, 7:41 p.m.
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()