• 
      
    Fish Trophy

    misery got a fish trophy!

    Fish Trophy

    Avoid error message on "New Review Request page" for Mercurial < 3.9

    Review Request #8778 — Created Feb. 24, 2017 and submitted

    Information

    Review Board
    release-2.5.x
    71c8c64...

    Reviewers

    seb
    If the repository hosting uses a Mercurial 3.7 it is possible to
    get branches (/json-branches/) with the JSON API of hgweb. But it leads
    to an error if reviewboard tries to fetch "/json-log/?rev=" as
    it is not implemented.
    
    Also avoid an error if Mercurial is really old and does not support
    to get branches in JSON style with "/json-branches/".
    
    Bugs closed: 4524

    Opened "New Review Request page" and check if branch and commit list
    works properly and does not contain errors.

    Tested versions: 2.6.2, 3.0, 3.7.3, 4.1

    Description From Last Updated

    This doesn't quite look right to me, but I'm very unfamiliar with the code. In line 321 contents is treated …

    SE seb

    A similar problem here. When I look at the original 3.7 map file for the json style, it looks to …

    SE seb

    Col: 80 E501 line too long (90 > 79 characters)

    reviewbotreviewbot

    "type" is a reserved word in python. Can you change this variable name?

    daviddavid

    Add a blank line between these two.

    daviddavid

    Add a blank line between these two.

    daviddavid

    Should we do the check here before json.loads?

    daviddavid
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/scmtools/hg.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/scmtools/hg.py
      
      
    2. 
        
    SE
    1. 
        
    2. reviewboard/scmtools/hg.py (Diff revision 1)
       
       
       
       
       
       
       
       
      Show all issues

      This doesn't quite look right to me, but I'm very unfamiliar with the code. In line 321 contents is treated as a json string which makes me believe it is simply the body of the HTTP response. However, in line 315 you are using in as though it was a dictionary?

      1. Yeah, you are right.... I already try to get the http header.

    3. reviewboard/scmtools/hg.py (Diff revision 1)
       
       
      Show all issues

      A similar problem here. When I look at the original 3.7 map file for the json style, it looks to me as though the 'not yet implemented' is returned as a json string. So it would seem much safer to test for the returned content-type in whatever way is correct (see comment above) and then do something like

      json_contents = json.loads(contents)
      if not json_contents == 'not yet implemented'
      

      or similar. Otherwise you ran into trouble if the response has any characters escaped for instance (although I don't know why it ever would).

      1. Well, it is bad to check for a string here, right. But the Content-Type is incorrect and the result is plaintext "not yet implemented". Maybe it is better to just check the body size. ;-)

    4. 
        
    misery
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/scmtools/hg.py
          reviewboard/scmtools/core.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/scmtools/hg.py
          reviewboard/scmtools/core.py
      
      
    2. reviewboard/scmtools/core.py (Diff revision 2)
       
       
      Show all issues
      Col: 80
       E501 line too long (90 > 79 characters)
      
    3. 
        
    misery
    misery
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/scmtools/hg.py
          reviewboard/scmtools/core.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/scmtools/hg.py
          reviewboard/scmtools/core.py
      
      
    2. 
        
    david
    1. 
        
    2. reviewboard/scmtools/core.py (Diff revision 3)
       
       
      Show all issues

      "type" is a reserved word in python. Can you change this variable name?

    3. reviewboard/scmtools/core.py (Diff revision 3)
       
       
       
      Show all issues

      Add a blank line between these two.

    4. reviewboard/scmtools/core.py (Diff revision 3)
       
       
       
      Show all issues

      Add a blank line between these two.

    5. reviewboard/scmtools/hg.py (Diff revision 3)
       
       
       
      Show all issues

      Should we do the check here before json.loads?

    6. 
        
    misery
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/scmtools/hg.py
          reviewboard/scmtools/core.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/scmtools/hg.py
          reviewboard/scmtools/core.py
      
      
    2. 
        
    david
    1. Ship It!
    2. 
        
    misery
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-2.5.x (8eba94c)