fix 'NoneType' object has no attribute 'scan_for_server' in rbt login/logout

Review Request #7794 — Created Dec. 3, 2015 and discarded

tterrace
RBTools
master
rbtools

rbt login/logout commands crash if the server url is set in the SCM config

The rbt login and logout commands do not initialize the scm tool like other
commands do. This means that if the server url is set in the scm (through
.gitconfig for example), rather than the command line then login and logout
will crash with an AttributeError.

Tests pass except for the MercurialClient (+svn) tests, where I'm seeing this issue with hgsubversion: https://bitbucket.org/durin42/hgsubversion/issues/449/extension-breaks-mercurial-36-rc2

Additionally, it seems the MercurialClient (+svn) test failure will leave the suite in a bad state because all subsequent tests will fail with an OSError, but when run in isolation they pass just fine. The travis-ci build hit the same issue: https://travis-ci.org/reviewboard/rbtools/jobs/94449505

reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/__init__.py
    
    
  2. 
      
TT
chipx86
  1. The fix seems like it'd work, but it feels like a bandaid for the real issue, which is that this is being called at all with a None value for the tool. That shouldn't be valid at all. Can you look at how we're getting to that state?

    Also, I'd like more info in the description (which we use as the commit message) as to the original problem being fixed and how it was caused.

    1. Looks like the original cause was my fault. I think given the use case here, your fix is fine, but some documentation and function argument changes would do a lot to make this clearer down the road.

      Can you make the repository_url and tool arguments optional, and document in the docstring that, if not provided, this will only take into consideration the server URL provided on the comand line?

      RBTools docstrings are kind of a mess, but we're transitionining to the following format:

      """One-line summary.
      
      Multi-line description.
      
      Args:
          argname1 (argtype):
              Description
      
          argname2 (argtype, optional):
              Description
      
          ...
      
      Returns:
          returntype:
          Description.
      """
      

      (In this case, both would have the ', optional' bit.)

    2. (was writing this as you responded, take a look at the last part and let me know if you'd still like to keep login/logout from trying to initialize the scm)

      Okay I've updated the description with the root cause, hopefully it's clearer now. You can reproduce the error by setting your server url in .gitconfig (or your scm of choice) and calling rbt login or logout. According to login.py the intention is to allow the user to login even if they're outside of the scm directory:

      ...allowing the user to log in and save a session cookie 
      without needing to be in a repository or posting to
      the server.
      

      One option is to do something like close.py does: attempt to initialize the scm only if the server url hasn't been set via .reviewboardrc or --server.

  2. 
      
TT
TT
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/__init__.py
    
    
  2. 
      
david
Review request changed

Status: Discarded

Loading...