[PATCH 4 of 4] keyring: add new extension keyring

Markus Zapke-Gründemann markuszapke at gmx.net
Mon Sep 17 08:49:56 CDT 2012


Bryan O'Sullivan schrieb:
> On Fri, Sep 14, 2012 at 4:00 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> 
>> This way of posting it now makes it clear that a lot is changed compared
>> to the "known" and tested extension. Reviewing the changes this way is not
>> feasible.
>>
Right, it's hard to review the code this way. This is why I excluded the
original code in the first place.

The mercurial_keyring extension is well "known" and used by many. This is my
motivation to bring it to the core. Because it's one of the most popular third
party extensions. But it is not tested. Only if you talk of testing as manually
testing by users. The original code has no test suite at all.

> Yep. The diff was too squirrelly for me to look at in detail. The only
> thing that caught my attention in my brief glance was some custom
> monkeypatching code, which always scares me.
> 
The monkeypatch code is only part of the original mercurial_keyring extension.
I removed it completely and tried to replace it with hooks in the http modules.
But it looks like I can improve them a little bit.

In general I did four things when porting the mercurial_keyring extension to
the core:

1. Add a test suite
2. Apply Mercurial's coding conventions
3. Remove the monkeypatch code and add new hooks to the http modules
4. Remove old and unused code (because the keyring extension won't be used with
older Mercurial clients)

Maybe I should first try to improve the original extension before moving it to
the core. I will write about this in a second email.


Markus


More information about the Mercurial-devel mailing list