Patchwork [4,of,4,V2] resolve: wrap implementation in a ui.tempconfig context

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

Siddharth Agarwal - March 21, 2016, 3:18 a.m.
# 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.
via Mercurial-devel - March 22, 2016, 5:22 p.m.
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).
Siddharth Agarwal - March 22, 2016, 5:30 p.m.
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.
via Mercurial-devel - March 22, 2016, 5:36 p.m.
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.
Siddharth Agarwal - March 22, 2016, 5:38 p.m.
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