Pull requests

#1 Declined
Repository
ojno
Branch
default
Repository
aragost
Branch
default

Support for SSH keys

Author
  1. ojno
Reviewers
Description

I've added support for SSH keys, using Paramiko, to commitsigs. I needed it for signing integrated with mercurial-server; hope it will be useful for others as well.

There are a couple of fixes in there as well; for some reason, probably a bug in Mercurial, the config dictionary was being reset sometimes, so I made it a function and passed around the ui object instead.

(update: Oops, made a mistake in the docstring. Corrected.)

  • Learn about pull requests

Comments (2)

  1. Martin Geisler

    Hi,

    That looks like interesting functionality. However, I have some comments:

    1. You don't have a real username in the changesets, just ojno.
    2. There are some really long lines in the new code -- please keep them below 75 characters to make the code easy to read.
    3. The dependency on Paramiko should be optional. So you will need to import it, access a function or class in the module and catch the ImportError exception thrown if the demandimport system in Mercurial cannot find Paramiko.
    4. The changes to the config system look good, but they should be done in separate changesets. That makes it easier for me to review: I can then pull in the good stuff first and we can talk about the more complicated things later.
    5. The "hi" here looks like temporary test code: 'ssh.publickeydirs': ["hi"].
    6. The new tests (great that you add tests!) should go into a new test-commitsigs-ssh file. But I can see that you just followed the pattern I laid out originally.

    Could you rework the changesets with something like MQ and submit another pull request?

  2. ojno author

    Hey. I'm new to Bitbucket and haven't done anything serious with hg before either, so thanks for the feedback. I'll see what I can do.