Patchwork D3517: shelve: reduce scope of config override

login
register
mail settings
Submitter phabricator
Date May 10, 2018, 12:14 a.m.
Message ID <differential-rev-PHID-DREV-tsb4iz26peos4kpv6dny-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/31438/
State Superseded
Headers show

Comments

phabricator - May 10, 2018, 12:14 a.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The config override seems to have a much greater scope than it needed
  to. I *think* it's only relevant in the while merging files.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3517

AFFECTED FILES
  hgext/shelve.py

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - May 10, 2018, 1:15 p.m.
> +        shelvectx = _rebaserestoredcommit(ui, repo, opts, tr, oldtiprev,
> +                                          basename, pctx, tmpwctx,
> +                                          shelvectx, branchtorestore,
> +                                          activebookmark)
>          overrides = {('ui', 'forcemerge'): opts.get('tool', '')}
>          with ui.configoverride(overrides, 'unshelve'):

Maybe I'm wrong, but doesn't rebase do merge?
phabricator - May 10, 2018, 1:15 p.m.
yuja added a comment.


  > +        shelvectx = _rebaserestoredcommit(ui, repo, opts, tr, oldtiprev,
  >  +                                          basename, pctx, tmpwctx,
  >  +                                          shelvectx, branchtorestore,
  >  +                                          activebookmark)
  > 
  >   overrides = {('ui', 'forcemerge'): opts.get('tool', '')}
  >   with ui.configoverride(overrides, 'unshelve'):
  
  Maybe I'm wrong, but doesn't rebase do merge?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3517

To: martinvonz, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - May 10, 2018, 3:20 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D3517#55774, @yuja wrote:
  
  > > +        shelvectx = _rebaserestoredcommit(ui, repo, opts, tr, oldtiprev,
  > >  +                                          basename, pctx, tmpwctx,
  > >  +                                          shelvectx, branchtorestore,
  > >  +                                          activebookmark)
  > > 
  > >   overrides = {('ui', 'forcemerge'): opts.get('tool', '')}
  > >   with ui.configoverride(overrides, 'unshelve'):
  >
  > Maybe I'm wrong, but doesn't rebase do merge?
  
  
  Correct, but we seem to be passing the tool explicitly to the rebase command on line 755. I had checked this before, and should have mentioned it in the commit message. Sorry.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3517

To: martinvonz, #hg-reviewers
Cc: yuja, mercurial-devel
Yuya Nishihara - May 11, 2018, 12:33 p.m.
>   > > +        shelvectx = _rebaserestoredcommit(ui, repo, opts, tr, oldtiprev,
>   > >  +                                          basename, pctx, tmpwctx,
>   > >  +                                          shelvectx, branchtorestore,
>   > >  +                                          activebookmark)
>   > > 
>   > >   overrides = {('ui', 'forcemerge'): opts.get('tool', '')}
>   > >   with ui.configoverride(overrides, 'unshelve'):
>   >
>   > Maybe I'm wrong, but doesn't rebase do merge?
>   
>   
>   Correct, but we seem to be passing the tool explicitly to the rebase command on line 755.

Right. Queued this, thanks.
phabricator - May 11, 2018, 7:31 p.m.
yuja added a comment.


  >   > > +        shelvectx = _rebaserestoredcommit(ui, repo, opts, tr, oldtiprev,
  >   > >  +                                          basename, pctx, tmpwctx,
  >   > >  +                                          shelvectx, branchtorestore,
  >   > >  +                                          activebookmark)
  >   > > 
  >   > >   overrides = {('ui', 'forcemerge'): opts.get('tool', '')}
  >   > >   with ui.configoverride(overrides, 'unshelve'):
  >   >
  >   > Maybe I'm wrong, but doesn't rebase do merge?
  >   
  >   
  >   Correct, but we seem to be passing the tool explicitly to the rebase command on line 755.
  
  Right. Queued this, thanks.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3517

To: martinvonz, #hg-reviewers
Cc: yuja, mercurial-devel

Patch

diff --git a/hgext/shelve.py b/hgext/shelve.py
--- a/hgext/shelve.py
+++ b/hgext/shelve.py
@@ -933,27 +933,27 @@ 
         # to the original pctx.
 
         activebookmark = _backupactivebookmark(repo)
+        tmpwctx, addedbefore = _commitworkingcopychanges(ui, repo, opts,
+                                                         tmpwctx)
+        repo, shelvectx = _unshelverestorecommit(ui, repo, basename)
+        _checkunshelveuntrackedproblems(ui, repo, shelvectx)
+        branchtorestore = ''
+        if shelvectx.branch() != shelvectx.p1().branch():
+            branchtorestore = shelvectx.branch()
+
+        shelvectx = _rebaserestoredcommit(ui, repo, opts, tr, oldtiprev,
+                                          basename, pctx, tmpwctx,
+                                          shelvectx, branchtorestore,
+                                          activebookmark)
         overrides = {('ui', 'forcemerge'): opts.get('tool', '')}
         with ui.configoverride(overrides, 'unshelve'):
-            tmpwctx, addedbefore = _commitworkingcopychanges(ui, repo, opts,
-                                                             tmpwctx)
-            repo, shelvectx = _unshelverestorecommit(ui, repo, basename)
-            _checkunshelveuntrackedproblems(ui, repo, shelvectx)
-            branchtorestore = ''
-            if shelvectx.branch() != shelvectx.p1().branch():
-                branchtorestore = shelvectx.branch()
+            mergefiles(ui, repo, pctx, shelvectx)
+        restorebranch(ui, repo, branchtorestore)
+        _forgetunknownfiles(repo, shelvectx, addedbefore)
 
-            shelvectx = _rebaserestoredcommit(ui, repo, opts, tr, oldtiprev,
-                                              basename, pctx, tmpwctx,
-                                              shelvectx, branchtorestore,
-                                              activebookmark)
-            mergefiles(ui, repo, pctx, shelvectx)
-            restorebranch(ui, repo, branchtorestore)
-            _forgetunknownfiles(repo, shelvectx, addedbefore)
-
-            shelvedstate.clear(repo)
-            _finishunshelve(repo, oldtiprev, tr, activebookmark)
-            unshelvecleanup(ui, repo, basename, opts)
+        shelvedstate.clear(repo)
+        _finishunshelve(repo, oldtiprev, tr, activebookmark)
+        unshelvecleanup(ui, repo, basename, opts)
     finally:
         if tr:
             tr.release()