[tor-bugs] #9264 [BridgeDB]: Problem with transport lines in BridgeDB's bridge pool assignment files

[Tor Bug Tracker & Wiki]

#9264: Problem with transport lines in BridgeDB's bridge pool assignment files
--------------------------+--------------------------------------------
     Reporter:  karsten   |      Owner:  isis
         Type:  defect    |     Status:  needs_revision
     Priority:  major     |  Milestone:
    Component:  BridgeDB  |    Version:
   Resolution:            |   Keywords:  bridgedb-0.1.3, bridgedb-0.1.5
Actual Points:            |  Parent ID:
       Points:            |
--------------------------+--------------------------------------------
Changes (by isis):

 * keywords:  bridgedb-0.1.3 => bridgedb-0.1.3, bridgedb-0.1.5
 * status:  reopened => needs_revision


Comment:

 Reviewing. I also merged #11127 and #10809's
 [https://gitweb.torproject.org/user/isis/bridgedb.git/shortlog/refs/heads/fix/11127
 -recaptcha-ssl_10809r1_r1 fix/11127-recaptcha-ssl_10809r1_r1] branch into
 it, in order to [https://travis-
 ci.org/isislovecruft/bridgedb/builds/20654021 test your changes on top of
 those other ones].

 Comments:

  1. In
 [https://gitweb.torproject.org/user/sysrqb/bridgedb.git/commitdiff/ff3a2641ac3447238511ce3436386d9ee2553011
 commit ff3a2641ac3447238511ce3436386d9ee2553011]... somewhere a while ago
 (not in this commit specifically) I think the `COLLECT_TIMESTAMPS` option
 for #10724 got lost. I think I remember seeing it re-added in your #5232
 branch though.

  2. In 13fc557b8e4bd094457ca954129dceaa4445d744, the
 `test_splitterBridgeInsertion` is great!

  3. In 92eb6fe9e7d4e1e7cbb72687b9f41f5f827fa182:

     {{{
 -        self.bridges.append(bridge)
 +        index = 0
 +        logging.debug("Inserting %s into %s ring" % (bridge.fingerprint,
 self.key))
 +        for old_bridge in self.bridges:
 +            if bridge.fingerprint == old_bridge.fingerprint:
 +                self.bridges[index] = bridge
 +                break
 +            index += 1
 +        else:
 +            self.bridges.append(bridge)
 }}}

  Don't you want to use `Util.logSafely()`? And doesn't the
 `self.bridges[index] = bridge` mean that, if there were already duplicates
 in `self.bridges`, that only the first duplicate is replaced and the
 others remain in the list?

  Also, it's a bit odd, but you might want to do `for old_bridge in
 self.bridges[:]:` instead of `for old_bridge in self.bridges:`, because of
 a list comprehension bug in Python (I don't recall off the top of my head
 what version it was fixed in...) where there is an off-by-one error in
 calculating the number of elements in the list (but only with really large
 lists like this one).

  4. In general, don't worry about the space taken up by splitting
 unittests up into many smaller tests, because the first thing that raises
 an exception means all the rest of the test doesn't get run.

 ---------

 Other than the tiny things mentioned above, I think this should be merged
 immediately for version 0.1.5.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9264#comment:34>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online