Fixed access to uninitialized changeset variable

Review Request #2151 — Created Feb. 21, 2011 and submitted


Review Board


Fixed access to uninitialized changeset variable.
Problem occurs in line 184 if changeset is not initialized in line 173 due to exception NotImplementedError.

  1. I'm a little unsure how you're filling in the changenum field if the SCMTool doesn't support changesets. Can you give some background on why this fix is necessary?
    1. I stumbled about the "uninitialized changeset" problem while extending Review Board for web-based post reviews. In my opinion the code in not clean even if it is not relevant in official releases at the moment. Taking your comment into account my proposal seems to be a workaround only. If changenum + changeset support belong together logically (as indicated by you) I recommend to no longer ignore the NotImplementError exception. 
      My new proposal is to remove the following lines
                  except NotImplementedError:
                      # This scmtool doesn't have changesets
      Did I overlook something?
    2. I'd like to hear more about what you're doing. Having a web UI for post-commit reviews is one of our most requested features, so it would be great if we could get a general solution.
      Having the changenum field tied to changesets is an artifact of the way that we've done things so far, but it's not necessarily how things need to be. If there's a good reason to change it (and post-commit reviews would be a good reason), we definitely can.
    3. I think it's fine to remove "pass" and to properly validate whether changesets are supported (when passing changenum) is ideal. So maybe replace "pass" with raising some new error result saying that changesets aren't supported.
      This needs to then be hooked up in reviews/ and webapi/ to provide the errors in a meaningful way.
      The code dealing with all this is really old. As in, one of the very first things ever written, back when forms, our error processing, our API, and really half of Django either didn't exist or didn't work the same way. So it shouldn't be a surprise that it's not the cleanest and needs some love.
    4. @David:
      In 2009 and 2010 I implemented post-commit reviews for both Perforce and TFS at Nero. Unfortunately I had not the time release the code to the public due to various impediments (your switch from subversion to git / job change). Currently my company is working with Subversion and I started to publish my modifications right from the beginning. Please have a look at
      I'm on holiday next week and would to start a mailing list discussion about post commits afterwards. It would be great if we find a way to smoothly integrate post commit reviews into the official master. I'm sure your have a lot of requirements and ideas about this topic.
    5. @Chris:
      Maybe we can re-use the ChangeSetError exception? 
                      changeset = repository.get_scmtool().get_changeset(changenum)
                  except NotImplementedError:
                      # This scmtool doesn't have changesets
                      self.errors['changenum'] = forms.util.ErrorList(['Changesets are not supported.'])
                      raise ChangeSetError()
      It gives a clear error message and should not need any additional code changes. How do you think?
    6. That sounds great. Can you post that in a new diff here?
  2. reviewboard/reviews/ (Diff revision 2)
    A cursory test suggests that this is going to get caught by the "except ChangeSetError" below, which isn't correct. Can we reorder the except blocks to avoid this?
Review request changed
Change Summary:
Good point. I moved the "except ChangeSetError" block to the top.
  1. Pushed to master as b98e3e0. Thanks!