Patchwork [11,of,11] sslutil: stop checking for web.cacerts=! (BC)

login
register
mail settings
Submitter Gregory Szorc
Date May 5, 2016, 7:53 a.m.
Message ID <7d66c23aeb70cf2ba70f.1462434808@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/14903/
State Accepted
Headers show

Comments

Gregory Szorc - May 5, 2016, 7:53 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1462434391 25200
#      Thu May 05 00:46:31 2016 -0700
# Node ID 7d66c23aeb70cf2ba70f9f3d59d25091e2957804
# Parent  e2dccc9ec567d9f8aa912a7b33f132772fae3ab4
sslutil: stop checking for web.cacerts=! (BC)

The previous patch stopped setting web.cacerts=! to indicate
--insecure.

That left user configs as the only source that could introduce
web.cacerts=!.

The practical impact of this patch is we no longer honor
web.cacerts=! in configs. Instead, we always treat web.cacerts
as a path. The patch is therefore technically BC. However,
since I don't believe web.cacerts=! is documented, it should be
safe to remove. a939f08fae9c (which introduced --insecure) has
no indication that web.cacerts=! is anything but an implementation
detail, reinforcing my belief it can be removed without major
debate.
Yuya Nishihara - May 6, 2016, 5:32 a.m.
On Thu, 05 May 2016 00:53:28 -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1462434391 25200
> #      Thu May 05 00:46:31 2016 -0700
> # Node ID 7d66c23aeb70cf2ba70f9f3d59d25091e2957804
> # Parent  e2dccc9ec567d9f8aa912a7b33f132772fae3ab4
> sslutil: stop checking for web.cacerts=! (BC)

The series LGTM. As a man who wrote a part of mind-numbing code, I request
2nd review.

> That left user configs as the only source that could introduce
> web.cacerts=!.
> 
> The practical impact of this patch is we no longer honor
> web.cacerts=! in configs. Instead, we always treat web.cacerts
> as a path. The patch is therefore technically BC. However,
> since I don't believe web.cacerts=! is documented, it should be
> safe to remove. a939f08fae9c (which introduced --insecure) has
> no indication that web.cacerts=! is anything but an implementation
> detail, reinforcing my belief it can be removed without major
> debate.

Yeah, it should be implementation detail. Only valid form is web.cacerts=<path>.
Pierre-Yves David - May 9, 2016, 12:52 p.m.
On 05/06/2016 07:32 AM, Yuya Nishihara wrote:
> On Thu, 05 May 2016 00:53:28 -0700, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1462434391 25200
>> #      Thu May 05 00:46:31 2016 -0700
>> # Node ID 7d66c23aeb70cf2ba70f9f3d59d25091e2957804
>> # Parent  e2dccc9ec567d9f8aa912a7b33f132772fae3ab4
>> sslutil: stop checking for web.cacerts=! (BC)
> The series LGTM. As a man who wrote a part of mind-numbing code, I request
> 2nd review.

I double reviewed and pushed.

Patch

diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py
--- a/mercurial/sslutil.py
+++ b/mercurial/sslutil.py
@@ -249,19 +249,16 @@  def sslkwargs(ui, host):
         return kws
 
     # The code below sets up CA verification arguments. If --insecure is
     # used, we don't take CAs into consideration, so return early.
     if ui.insecureconnections:
         return kws
 
     cacerts = ui.config('web', 'cacerts')
-    # TODO remove check when we stop setting this config.
-    if cacerts == '!':
-        return kws
 
     # If a value is set in the config, validate against a path and load
     # and require those certs.
     if cacerts:
         cacerts = util.expandpath(cacerts)
         if not os.path.exists(cacerts):
             raise error.Abort(_('could not find web.cacerts: %s') % cacerts)