| Submitter | Siddharth Agarwal |
|---|---|
| Date | March 21, 2016, 3:18 a.m. |
| Message ID | <421c17c403814ee62717.1458530322@dev666.prn1.facebook.com> |
| Download | mbox | patch |
| Permalink | /patch/14012/ |
| State | Changes Requested |
| Delegated to: | Martin von Zweigbergk |
| Headers | show |
Comments
On Sun, Mar 20, 2016 at 8:18 PM, Siddharth Agarwal <sid0@fb.com> wrote: > # HG changeset patch > # User Siddharth Agarwal <sid0@fb.com> > # Date 1458530037 25200 > # Sun Mar 20 20:13:57 2016 -0700 > # Node ID 421c17c403814ee627170de854d1628350439895 > # Parent 9df7db49e7bc5f0e9a1630e048fdbe8191e04da0 > resolve: wrap implementation in a ui.tempconfig context > > This makes sure that the --tool config applies to the merge driver preprocess > and conclude steps, and also ensures that it applies to whatever other new > potential resolve calls we make in the future. I like patches 1&4, but I don't see how they're related to 2&3. Could 4 be done before the move instead so it's not held hostage by patch 2? I was also wondering if 1 config option is always enough, but we can of course always add support for passing a list of options later if there even are such cases (I could not find any after spending ~2 min looking).
On 3/22/16 10:22, Martin von Zweigbergk wrote: > I like patches 1&4, but I don't see how they're related to 2&3. Could > 4 be done before the move instead so it's not held hostage by patch 2? > > I was also wondering if 1 config option is always enough, but we can > of course always add support for passing a list of options later if > there even are such cases (I could not find any after spending ~2 min > looking). Not without massive indentation diffs, which were part of my goal with 2 and 3 anyway.
On Tue, Mar 22, 2016 at 10:30 AM, Siddharth Agarwal <sid0@fb.com> wrote: > On 3/22/16 10:22, Martin von Zweigbergk wrote: >> >> I like patches 1&4, but I don't see how they're related to 2&3. Could >> 4 be done before the move instead so it's not held hostage by patch 2? >> >> I was also wondering if 1 config option is always enough, but we can >> of course always add support for passing a list of options later if >> there even are such cases (I could not find any after spending ~2 min >> looking). > > > Not without massive indentation diffs, which were part of my goal with 2 and > 3 anyway. I see. Follow-up questions: The methods in cmdutil are meant to be reusable, right? Does it make sense to call your new cmdutil.resolve() without wrapping it in that "with ui.tempconfig()" thing? Maybe it does, I don't know. Just checking that this last patch doesn't make the interface worse in the name of less indentation.
On 3/22/16 10:36, Martin von Zweigbergk via Mercurial-devel wrote: > I see. Follow-up questions: The methods in cmdutil are meant to be > reusable, right? Much of the time, though not always I think. There are a bunch of specialized functions in cmdutil. > Does it make sense to call your new cmdutil.resolve() > without wrapping it in that "with ui.tempconfig()" thing? Yeah -- in particular different callers might want to set different ui.forcemerge tools. > Maybe it > does, I don't know. Just checking that this last patch doesn't make > the interface worse in the name of less indentation.
Patch
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py --- a/mercurial/cmdutil.py +++ b/mercurial/cmdutil.py @@ -2944,15 +2944,12 @@ def resolve(ui, repo, ms, pats, opts): try: # preresolve file - ui.setconfig('ui', 'forcemerge', opts.get('tool', ''), - 'resolve') complete, r = ms.preresolve(f, wctx) if not complete: tocomplete.append(f) elif r: ret = 1 finally: - ui.setconfig('ui', 'forcemerge', '', 'resolve') ms.commit() # replace filemerge's .orig file with our resolve file, but only @@ -2968,13 +2965,10 @@ def resolve(ui, repo, ms, pats, opts): for f in tocomplete: try: # resolve file - ui.setconfig('ui', 'forcemerge', opts.get('tool', ''), - 'resolve') r = ms.resolve(f, wctx) if r: ret = 1 finally: - ui.setconfig('ui', 'forcemerge', '', 'resolve') ms.commit() # replace filemerge's .orig file with our resolve file diff --git a/mercurial/commands.py b/mercurial/commands.py --- a/mercurial/commands.py +++ b/mercurial/commands.py @@ -6025,7 +6025,8 @@ def resolve(ui, repo, *pats, **opts): with repo.wlock(): ms = mergemod.mergestate.read(repo) - ret, showhint = cmdutil.resolve(ui, repo, ms, pats, opts) + with ui.tempconfig('ui', 'forcemerge', opts.get('tool', ''), 'resolve'): + ret, showhint = cmdutil.resolve(ui, repo, ms, pats, opts) if showhint is None: pass