[PATCH] AuthenticationConfig patch to provide expected keys to credential store plug-ins

[Vincent Ladeuil]

>>>>> "jfroy" == Jean-Francois Roy <bahamut at macstorm.org> writes:

    jfroy> On Mar 9, 2009, at 5:47 PM, Vincent Ladeuil wrote:

    >>>>>>> "jfroy" == Jean-Francois Roy <bahamut at macstorm.org> writes:
    >> 
    jfroy> In addition, the netrc credential store plug-in as well as
    jfroy> its tests are designed under the assumption the credentials
    jfroy> dictionary will contain a "host" key, when it fact
    >> this  would
    jfroy> never be the case.
    >> 
    >> This is the case for the default section only since all sections in
    >> .netrc are by host.

    jfroy> That is incorrect.

Oh yes, I see.

<snip/>

    >> 
    jfroy> === modified file 'bzrlib/config.py'
    jfroy> --- bzrlib/config.py	2009-01-17 01:30:58 +0000
    jfroy> +++ bzrlib/config.py	2009-03-05 18:15:54 +0000
    >> 
    >> <snip/>
    >> 
    jfroy> @@ -1057,12 +1060,26 @@
    >> 
    >> <snip/>
    >> 
    jfroy> +            # Delete all extra keys from the
    >> credentials  dictionary
    jfroy> +            del credentials['scheme']
    jfroy> +            del credentials['host']
    jfroy> +            del credentials['port']
    jfroy> +            del credentials['path']
    jfroy> +            del credentials['realm']
    >> 
    >> I understand you comply with the doc string here, I wonder if
    >> we  should
    >> left these keys in place an update the doc string instead :)

    jfroy> I think it's fine to delete the keys. They're not required past
    jfroy> the  decode password key and keeping them around would only
    jfroy> increase memory  usage.

Memory usage is not a concern here, credentials are used only once for a
given connection and there is rarely more than one for a given bzr
command (or even any usage of bzrlib) and they will not last long in the
call stack anyway since they are of no use (yet) as soon the password is
consumed.

Yet, the day we'll implement a save mechanism, the deleted keys will be
needed, so better left them in and update the doc string.

    jfroy> # Bazaar merge directive format 2 (Bazaar 0.90)
    jfroy> # revision_id: jeanfrancois at apple.com-20090310182355-rz00psthdhtbllsi
    jfroy> # target_branch: file:///Volumes/Crossroads/bahamut/Documents/Source\
    jfroy> #   /bzr/bzr.dev/
    jfroy> # testament_sha1: 0996b69cd7676da50c6bc42c939ef33885e1b09b
    jfroy> # timestamp: 2009-03-10 11:29:29 -0700
    jfroy> # base_revision_id: pqm at pqm.ubuntu.com-20090310095511-627bx4kwirxdx12y
    jfroy> # 
    jfroy> # Begin patch

    jfroy> === modified file 'bzrlib/config.py'
    jfroy> --- bzrlib/config.py	2009-01-17 01:30:58 +0000
    jfroy> +++ bzrlib/config.py	2009-03-05 18:15:54 +0000

<snip/>

    jfroy> +            # Delete all extra keys from the credentials dictionary
    jfroy> +            del credentials['scheme']
    jfroy> +            del credentials['host']
    jfroy> +            del credentials['port']
    jfroy> +            del credentials['path']
    jfroy> +            del credentials['realm']

As said above, let's keep them.

    jfroy> === modified file 'bzrlib/plugins/netrc_credential_store/tests/test_netrc.py'
    jfroy> --- bzrlib/plugins/netrc_credential_store/tests/test_netrc.py	2008-10-06 10:06:53 +0000
    jfroy> +++ bzrlib/plugins/netrc_credential_store/tests/test_netrc.py	2009-03-10 18:21:20 +0000
    jfroy> @@ -46,26 +46,53 @@
    jfroy>              f.write(netrc_content)
    jfroy>          finally:
    jfroy>              f.close()
    jfroy> +        
    jfroy> +        # Create a test AuthenticationConfig object
    jfroy> +        ac_content = """
    jfroy> +[host1]
    jfroy> +host = host
    jfroy> +user = joe
    jfroy> +password_encoding = netrc
    jfroy> +
    jfroy> +[host2]
    jfroy> +host = host
    jfroy> +user = jim
    jfroy> +password_encoding = netrc
    jfroy> +
    jfroy> +[other]
    jfroy> +host = other
    jfroy> +user = anonymous
    jfroy> +password_encoding = netrc
    jfroy> +"""

This is rather confusing, all tests were standing on their own before
your patch, now a lot of information is centralized here which reduce
their defect localization (i.e. an update here may make several tests
fail).

That's one reason why I asked for a single test checking the additional
layer.

Another is that it will make your match smaller and easier to review.

    jfroy> +        ac_path = osutils.pathjoin(self.test_home_dir,
    jfroy> +        'netrc-authentication.conf')

    jfroy> +        f = open(ac_path, 'wb')
    jfroy> +        try:
    jfroy> +            f.write(ac_content)
    jfroy> +        finally:
    jfroy> +            f.close()
    jfroy> +        self.ac = config.AuthenticationConfig(_file=ac_path)

_file can be a StringIO(ac_content), no need to create a real file here. 

Almost good:

BB:tweak

        Vincent