Allow uploading simple linear history with Git.

Review Request #6787 — Created Jan. 16, 2015 and submitted

brennie
RBTools
dvcs
6816
5c9866c...
rbtools
chipx86, smacleod

The default behaviour for posting is now to determine the capabilities
of the server to see if it supports creating a review request with
commit history. The user can specify the -S/--squash-history
option or SQUASH_HISTORY config option in .reviewboardrc to
override this. However, the configuration option can itself be
overridden by the -H/--with-history flag.

Currently the only SCMClients that "supports" this flag is Git.

Add the DiffCommitResource and DiffCommitListResource resources to
the RBTools web API resources. The DiffCommitListResource handles
the uploading of commits via the upload_commit method.

Add the create_empty_diffset method to the DiffListResource class.
This method is used to create an empty DiffSet so that a review
request can be created with commit history.

Add the get_history method to the SCMClient class. This method
gets a linear history of commit metadata given some revisions.

The commit histories that can be uploaded by this change are simple
linear ones; that is, they must be linear histories (with no merges)
and diffs in commits should not overlap with respect to files.

Uploaded commits containing diffs to Review Board on the dvcs
branch.

Description From Last Updated

One blank line.

chipx86chipx86

Is it commits or diff-commits? I thought we had the issue of conflicts in naming.

chipx86chipx86

That's just too new!

chipx86chipx86

This is the bit that's all placeholders I'm guessing. Maybe add a big # XXX This is all bogus data. ...

chipx86chipx86

This can end up returning two different results. We should pre-calculate once.

chipx86chipx86

% on the next line (same line as the variable).

chipx86chipx86

Might be worth adding a config_key for this, in case any projects or users want to default to this behavior.

chipx86chipx86

No blank line here.

chipx86chipx86

Looks like this will always be True if --squash-history is not set.

chipx86chipx86

undefined name 'merge_parent_id'

reviewbotreviewbot

Col: 13 E265 block comment should start with '# '

reviewbotreviewbot

Col: 23 W292 no newline at end of file

reviewbotreviewbot

'datetime' imported but unused

reviewbotreviewbot

Col: 26 E225 missing whitespace around operator

reviewbotreviewbot

"committer"

chipx86chipx86

Worth maybe just doing a full-on strip()?

chipx86chipx86

Trailing period.

chipx86chipx86

This can be: for sha in reversed(history_lines): That's a little bit faster, since it can just iterate instead of rebuilding ...

chipx86chipx86

Can we get a list of all SHAs we need, and then do a single execute() for this, to reduce ...

chipx86chipx86

This will also result in a lot of slow, expensive calls. Can we grab everything we need for all SHAs ...

chipx86chipx86

You can probably just put the dictionary right in the append call instead.

chipx86chipx86

'datetime' imported but unused

reviewbotreviewbot

Col: 26 E225 missing whitespace around operator

reviewbotreviewbot

A nice Python utility function is enumerate. You can use that here to get both the index and the history ...

chipx86chipx86

not diff will be the same as len(diff) == 0.

chipx86chipx86

This is kind of hard to follow (and sometimes can lead to unexpected values). Can you break this out into ...

chipx86chipx86

We probably need to check for NotImplementedError, right?

chipx86chipx86

We ignore the item, but then we try fetching it in several places below. We should just use the item ...

chipx86chipx86

Should be two blank lines.

daviddavid

Blank line after this.

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
  2. 
      
brennie
brennie
chipx86
  1. I'm excited! I know it's too early, but just even seeing a prototype on the RBTools side is cool.

  2. rbtools/api/resource.py (Diff revision 1)
     
     
     
     
     

    One blank line.

  3. rbtools/api/resource.py (Diff revision 1)
     
     

    Is it commits or diff-commits? I thought we had the issue of conflicts in naming.

    1. I'll fix this in https://reviews.reviewboard.org/r/6779/.

  4. rbtools/commands/post.py (Diff revision 1)
     
     

    That's just too new!

  5. rbtools/commands/post.py (Diff revision 1)
     
     

    This is the bit that's all placeholders I'm guessing. Maybe add a big # XXX This is all bogus data. Don't pay too much attention to it. or something. (I almost commented on the commit IDs before I realized :)

  6. rbtools/commands/post.py (Diff revision 1)
     
     
     

    This can end up returning two different results. We should pre-calculate once.

  7. rbtools/commands/post.py (Diff revision 1)
     
     
     

    % on the next line (same line as the variable).

  8. 
      
brennie
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
  2. 
      
brennie
chipx86
  1. 
      
  2. rbtools/commands/post.py (Diff revision 4)
     
     

    Might be worth adding a config_key for this, in case any projects or users want to default to this behavior.

    1. I made that the default behaviour by checking if the server supports commit histories via the API capabilities.

  3. rbtools/commands/post.py (Diff revision 4)
     
     
     
     

    No blank line here.

  4. rbtools/commands/post.py (Diff revision 4)
     
     
     

    Looks like this will always be True if --squash-history is not set.

    1. The commit history posting is intended to be the default behaviour unless one of -S/--squash-history/SQUASH_HISTORY=True is set. In case that IS true, we let -H override it.

  5. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
  2. rbtools/api/resource.py (Diff revision 5)
     
     
     undefined name 'merge_parent_id'
    
  3. rbtools/clients/git.py (Diff revision 5)
     
     
    Col: 13
     E265 block comment should start with '# '
    
  4. rbtools/clients/git.py (Diff revision 5)
     
     
    Col: 23
     W292 no newline at end of file
    
  5. rbtools/commands/post.py (Diff revision 5)
     
     
     'datetime' imported but unused
    
  6. rbtools/commands/post.py (Diff revision 5)
     
     
    Col: 26
     E225 missing whitespace around operator
    
  7. 
      
brennie
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
  2. rbtools/commands/post.py (Diff revision 6)
     
     
     'datetime' imported but unused
    
  3. rbtools/commands/post.py (Diff revision 6)
     
     
    Col: 26
     E225 missing whitespace around operator
    
  4. 
      
chipx86
  1. A few comments on performance, and some small things here and there, but this looks like it's basically good to go :)

  2. rbtools/api/resource.py (Diff revision 6)
     
     

    "committer"

  3. rbtools/clients/git.py (Diff revision 6)
     
     

    Worth maybe just doing a full-on strip()?

  4. rbtools/clients/git.py (Diff revision 6)
     
     

    Trailing period.

  5. rbtools/clients/git.py (Diff revision 6)
     
     
     
     

    This can be:

    for sha in reversed(history_lines):
    

    That's a little bit faster, since it can just iterate instead of rebuilding the list.

  6. rbtools/clients/git.py (Diff revision 6)
     
     

    Can we get a list of all SHAs we need, and then do a single execute() for this, to reduce the time needed?

  7. rbtools/clients/git.py (Diff revision 6)
     
     
     
     
     
     
     
     

    This will also result in a lot of slow, expensive calls. Can we grab everything we need for all SHAs in a single call, in some serialized format we can parse?

  8. rbtools/clients/git.py (Diff revision 6)
     
     

    You can probably just put the dictionary right in the append call instead.

  9. rbtools/commands/post.py (Diff revision 6)
     
     

    A nice Python utility function is enumerate. You can use that here to get both the index and the history item:

    for i, item in enumerate(history):
    
  10. rbtools/commands/post.py (Diff revision 6)
     
     

    not diff will be the same as len(diff) == 0.

  11. rbtools/commands/post.py (Diff revision 6)
     
     
     

    This is kind of hard to follow (and sometimes can lead to unexpected values). Can you break this out into explicit if/else blocks?

  12. 
      
brennie
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
  2. 
      
brennie
chipx86
  1. 
      
  2. rbtools/commands/post.py (Diff revision 9)
     
     

    We probably need to check for NotImplementedError, right?

    1. It should never be the case that with_history is True and self.tool.get_history would throw a NotImplementedError. Earlier in the code (lines 645--652) we check if we have specified the -H option and the tool doesn't support history (via the SCMTool's supports_post_with_history field.

  3. rbtools/commands/post.py (Diff revision 9)
     
     

    We ignore the item, but then we try fetching it in several places below. We should just use the item that we fetch here, to avoid all the extra lookups.

  4. 
      
brennie
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/clients/__init__.py
        rbtools/api/resource.py
        rbtools/clients/git.py
    
    
  2. 
      
brennie
david
  1. 
      
  2. rbtools/api/resource.py (Diff revision 10)
     
     

    Should be two blank lines.

  3. rbtools/api/resource.py (Diff revision 10)
     
     

    Blank line after this.

  4. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to dvcs (12b1d7e)
Loading...