This is an archive of the discontinued Mercurial Phabricator instance.

shelve: reduce scope of merge tool config override
ClosedPublic

Authored by martinvonz on May 9 2018, 8:14 PM.

Details

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. The
rebase step also cares about the merge tool, but we seem to be
explicitly passing it to rebase (around line 755).

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.May 9 2018, 8:14 PM
yuja added a subscriber: yuja.May 10 2018, 9:15 AM

+ 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?

In 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.

martinvonz edited the summary of this revision. (Show Details)May 10 2018, 11:22 AM
martinvonz retitled this revision from shelve: reduce scope of config override to shelve: reduce scope of merge tool config override.
yuja added a comment.May 11 2018, 8:33 AM
> > +        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.

This revision was automatically updated to reflect the committed changes.