Update following Trishank's initial code review

tuf/client/updater.py

@@ -719,8 +719,8 @@ def _update_metadata(self, metadata_role, fileinfo=None, compression=None):
       downloaded_version = metadata_signable['signed']['version']
       if downloaded_version < current_version:
         message = repr(mirror_url)+' is older than the version currently '+\
-          'installed.\nDownloaded version: '+str(downloaded_version)+'\n'+\
-          'Current version: '+str(current_version)
+          'installed.\nDownloaded version: '+repr(downloaded_version)+'\n'+\
+          'Current version: '+repr(current_version)
         raise tuf.RepositoryError(message)
       
     # Reject the metadata if any specified targets are not allowed.
@@ -943,18 +943,23 @@ def _ensure_all_targets_allowed(self, metadata_role, metadata_object):
     if role_index is not None:
       role = roles[role_index] 
       allowed_child_paths = role['paths']
-      delegated_targets = metadata_object['targets'].keys()
+      actual_child_targets = metadata_object['targets'].keys()
       
       # Check that each delegated target is either explicitly listed or a parent
       # directory is found under role['paths'], otherwise raise an exception.
-      for delegated_target in delegated_targets:
+      # If the parent role explicitly lists target file paths in 'paths',
+      # this loop will run in O(n^2), the worst-case.  The repository
+      # maintainer will likely delegate entire directories, and opt for
+      # explicit file paths if the targets in a directory are delegated to 
+      # different roles/developers.
+      for child_target in actual_child_targets:
         for allowed_child_path in allowed_child_paths:
-          prefix = os.path.commonprefix([delegated_target, allowed_child_path])
+          prefix = os.path.commonprefix([child_target, allowed_child_path])
           if prefix == allowed_child_path:
             break
         else: 
           message = 'Role '+repr(metadata_role)+' specifies target '+\
-            repr(delegated_target)+' which is not an allowed path according '+\
+            repr(child_target)+' which is not an allowed path according '+\
             'to the delegations set by '+repr(parent_role)+'.'
           raise tuf.RepositoryError(message)
     

tuf/repo/signercli.py

@@ -53,7 +53,6 @@
 import optparse
 import getpass
 import time
-import datetime
 import sys
 import logging
 import errno
@@ -413,7 +412,8 @@ def _get_metadata_version(metadata_filename):
     If 'metadata_filename' exists, load it and extract the current version.
     This version number is incremented by one prior to returning.  If
     'metadata_filename' does not exist, return a version value of 1.
-    Return 1 if 'metadata_filename' cannot be read or validated.
+    Raise 'tuf.RepositoryError' if 'metadata_filename' cannot be read or
+    validated.
   
   """
   
@@ -425,11 +425,13 @@ def _get_metadata_version(metadata_filename):
   # Open 'metadata_filename', extract the version number, and return it
   # incremented by 1.  A metadata's version is used to determine newer metadata
   # from older.  The client should only accept newer metadata.
-  signable = tuf.repo.signerlib.read_metadata_file(metadata_filename)
   try:
+    signable = tuf.repo.signerlib.read_metadata_file(metadata_filename)
     tuf.formats.check_signable_object_format(signable)
-  except tuf.FormatError, e:
-    return 1
+  except (tuf.FormatError, tuf.Error), e:
+    message = repr(metadata_filename)+' could not be opened or is invalid.'+\
+      '  Backup or replace it and try again.'
+    raise tuf.RepositoryError(message)
   current_version = signable['signed']['version']
 
   return current_version+1
@@ -441,8 +443,7 @@ def _get_metadata_version(metadata_filename):
 def _get_metadata_expiration():
   """
     Prompt the user for the expiration date of the metadata file.
-    If the entered date is valid, it is returned in seconds till
-    expiration.
+    If the entered date is valid, it is returned unmodified.
 
     <Exceptions>
       tuf.Error, if the entered expiration date is invalid.
@@ -451,19 +452,18 @@ def _get_metadata_expiration():
 
   message = '\nCurrent time: '+tuf.formats.format_time(time.time())+'.\n'+\
     'Enter the expiration date, in UTC, of the metadata file (yyyy-mm-dd HH:MM:SS): '
+    
   try:
     input_date = _prompt(message, str)
-    try:
-      input_date = input_date+' UTC'
-      expiration_date = tuf.formats.parse_time(input_date)
-    except tuf.FormatError, e:
-      raise tuf.Error('Invalid date entered.')
-    if expiration_date < time.time():
-      message = 'The expiration date must occur after the current date.'
-      raise tuf.Error(message)
-  except ValueError, e:
+    input_date = input_date+' UTC'
+    expiration_date = tuf.formats.parse_time(input_date)
+  except (tuf.FormatError, ValueError), e:
     raise tuf.Error('Invalid date entered.')
   
+  if expiration_date < time.time():
+    message = 'The expiration date must occur after the current date.'
+    raise tuf.Error(message)
+  
   return input_date
 
 
@@ -816,7 +816,9 @@ def make_targets_metadata(keystore_directory):
   # Verify the 'keystore_directory' argument.
   keystore_directory = _check_directory(keystore_directory)
 
-  # Retrieve the target files.
+  # Retrieve the target files.  The target paths entered by the user should be
+  # separated by white space.  'targets' is a list of the target path strings
+  # extracted from user input.
   prompt_targets = '\nInput may be a directory, directories, or any '+\
     'number of file paths.\nEnter the target files: '
   targets_input = _prompt(prompt_targets, str)

tuf/repo/signerlib.py

@@ -599,6 +599,8 @@ def read_metadata_file(filename):
   <Exceptions>
     tuf.FormatError, if 'filename' is improperly formatted.
 
+    tuf.Error, if 'filename' cannot be opened.
+
   <Side Effects>
     The contents of 'filename' are extracted.