[PATCH 0 of 3] hgkeyring extension

[Mads Kiilerich]

On 24/08/12 11:40, Markus Zapke-Gründemann wrote:
> This is the port of the mercurial_keyring extension to the core.

Has this been discussed and coordinated with the mercurial_keyring author?

> >> The new extension is named hgkeyring.
> > Why not just “keyring”?
> Because keyring is the name of the Python library[1] which provides the
> integration with the different keyring services.

That would matter if it was a standalone extension. Now you are pushing 
for getting it included in the "extensions shipped with Mercurial" 
namespace. "keyring" seems like the most obvious choice there.

> It has the same functionality as the original extension. I removed some code for older versions of mercurial as well as the monkeypatch code.

It would perhaps help review and future history digging if it was posted 
as one patch which simply moved the old extension to the new name and 
another that made the other changes you mention. (Another option could 
be to include the full history of the old project ... but at the end of 
the day it is not a good idea.)

> The extension uses now new hooks in the http modules. The hooks can also be used by similar extensions like factotum which still use monkeypatching.

Messing with internals in other modules is still monkeypatching. 
Introducing pwmgrclass seems to add complexity without any real benefit.


But the Python http authentication API is really not suitable for 
Mercurial's needs. The implementations of the API in the supported 
Python versions is also problematic because of the changes done over 
time - they make it hard to extend in a maintainable way. Adding 
"setrequest" to the API helps a bit - something like that could also be 
used for the retry counter.

It seems like a big part of keyring extension is a reimplementation of 
generic password handling, replacing the standard python implementation. 
It could perhaps make sense to implement our own password handler in 
core Mercurial and stop using the one from Python. That could perhaps 
make the keyring extension simpler and more obviously correct ... and 
make it possible to fix other http authentication issues.

Changes in this area could perhaps be coordinated a full switch to using 
Augie's httpclient ... but it seems like he prefer to stay compatible 
with the Python API's.

On a related note: It would probably be relevant to (re)test the 
extension with 2.4, 2.5, 2.5.latest, 2.6, 2.6.1, 2.6.2, 2.6.latest and 
2.7.latest before 'upgrading' the extension to 'official'.

/Mads