Adding in a smarter parent-base finding algorithm

Review Request #7985 - Created Feb. 21, 2016 and submitted

Shaurya Sengar
RBTools
master
rbtools

Main idea

Given a tree like this one:

A - * - * - * - * origin/master
* - B - * - * origin/feature
- C - D topic

when we rbt post, it would like to have three things,
'base': A revision to use as the base of the resulting diff.
'tip': A revision to use as the tip of the resulting diff.
'parent_base': (optional) The revision to use as the base of a
parent diff.
Now, parent_base has to be in the remote repository, in other words, a ancestor of a commit of any branch origin/*. We would also like parent_base to be as young an ancestor of tip as possible, given the first constraint.

A git-rev-list query can find this in the following manner -

git rev-list $TIP --not --remotes=origin | tail -1

should give the oldest ancestor that is not reachable from a public branch, so it's parent will be the best parent base.

The algorithm has been tested extensively. More specifically, GitClient.parse_revision_spec has been tested on the conditions outlined in the following comment (https://reviews.reviewboard.org/r/6797/#comment20865). Detailed documentation and visualization have been added to the tests to aid understanding. I hope this will help a reader understand and review the tests better.

Specifically, the conditions being tested are -

0) Testing GitClient.parse_revision_spec with target branch off a tracking branch not aligned with the remote.
1) Testing GitClient.parse_revision_spec with target branch off a tracking branch aligned with the remote.
2) Testing GitClient.parse_revision_spec with target branch off a tracking branch with changes since the remote.
3) Testing GitClient.parse_revision_spec with target branch off a branch not properly tracking the remote.
4) Testing GitClient.parse_revision_spec with a target branch that merged a tracking branch off another tracking branch.
5) Testing GitClient.parse_revision_spec with a target branch posted off a tracking branch that merged another tracking branch.
6) Testing GitClient.parse_revision_spec with a target branch posted off a remote branch without any tracking branches.
7) Testing GitClient.parse_revision_spec with a target branch posted off a remote branch that is aligned to the same commit as another remote branch.
8) Testing GitClient.parse_revision_spec with a target branch not up-to-date with a remote branch.
9) Testing GitClient.parse_revision_spec with a target branch that has branches from different remotes in its path.

Note - The above test descriptions make more sense when we actually view the commit graphs representing each of these scenarios. These commit graphs can be found as part of the documentation of my tests.

Note 2 - The test functions are named in a serial manner (i.e. with suffixes case_one, case_two ... case_nine etc.). This was done because it was not always easy to capture what the test was trying to test in a few words. Hence giving uniform but serial names helped because essentially, all of these tests test the same thing - i.e. whether GitClient.parse_revision_spec can find a good parent, parent_base in each of these scenarios.

I also had to alter one of the existing tests in order to align it with the new algorithm.

  • 0
  • 0
  • 44
  • 31
  • 75
Description From Last Updated
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/git.py
    
    
  2. rbtools/clients/tests.py (Diff revision 1)
     
     
    Col: 55
     W291 trailing whitespace
    
  3. rbtools/clients/tests.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  4. rbtools/clients/tests.py (Diff revision 1)
     
     
    Col: 54
     E262 inline comment should start with '# '
    
  5. rbtools/clients/tests.py (Diff revision 1)
     
     
    Col: 53
     E261 at least two spaces before inline comment
    
  6. rbtools/clients/tests.py (Diff revision 1)
     
     
    Col: 55
     W291 trailing whitespace
    
  7. rbtools/clients/tests.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  8. 
      
Shaurya Sengar
Shaurya Sengar
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/git.py
    
    
  2. rbtools/clients/tests.py (Diff revision 2)
     
     
    Col: 55
     W291 trailing whitespace
    
  3. rbtools/clients/tests.py (Diff revision 2)
     
     
    Col: 55
     W291 trailing whitespace
    
  4. rbtools/clients/tests.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  5. rbtools/clients/tests.py (Diff revision 2)
     
     
    Col: 61
     W291 trailing whitespace
    
  6. rbtools/clients/tests.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (86 > 79 characters)
    
  7. rbtools/clients/tests.py (Diff revision 2)
     
     
    Col: 55
     W291 trailing whitespace
    
  8. rbtools/clients/tests.py (Diff revision 2)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  9. 
      
Shaurya Sengar
Shaurya Sengar
Christian Hammond
  1. Hey,

    Thanks for putting these tests together. I know you wanted another pair of eyes on these, but given the complexity, I think I want more information shown in the tests in order to help with that. Basically, what a review will need for these tests is a visualization, and documentation.

    I see that you have print statements for showing a graph of the resulting branches. We're not going to want those print statements in there for the final review, but I would like to see the output you get for those statements as comments in each test, showing what the tree actually looks like. Sort of in the form of what's in my review on /r/6797/.

    That doesn't mean copy/paste from my review. Rather, I want to see the output you get, but formatted in a way that focuses on the important bits, and shown in a comment. It must be kept updated with any changes you put in.

    Along with that, please have a description at the top of each test about what you're doing and expecting (similar to my notes in my review), so that we (and future developers) will know for sure what's happening in each test.

    The docstrings themselves need to be shorter. The "case 1," "case 2," etc. won't make any sense without knowing about my review, and nobody should care down the road. So, the docstrings should be short, but self-describing, and should be about the function ultimately being tested. So, things like:

    Testing GitClient.parse_revision_spec with target branch off a tracking branch aligned with the remote
    Testing GitClient.parse_revision_spec with target branch off a tracking branch with changes since the remote
    Testing GitClient.parse_revision_spec with target branch off a branch not properly tracking the remote
    

    etc. So, similar to what I already have as the sections in my review, but written to make it clear that we're dealing with GitClient.parse_revision_spec.

    Once that's all done, I'll have a better starting point for verifying that these are doing the right thing.

    Thanks!

  2. 
      
Shaurya Sengar
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/git.py
    
    
  2. rbtools/clients/tests.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. rbtools/clients/tests.py (Diff revision 3)
     
     
    Col: 11
     W291 trailing whitespace
    
  4. rbtools/clients/tests.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  5. rbtools/clients/tests.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  6. rbtools/clients/tests.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  7. rbtools/clients/tests.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  8. rbtools/clients/tests.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  9. rbtools/clients/tests.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (113 > 79 characters)
    
  10. rbtools/clients/tests.py (Diff revision 3)
     
     
    Col: 11
     W291 trailing whitespace
    
  11. rbtools/clients/tests.py (Diff revision 3)
     
     
    Col: 11
     W291 trailing whitespace
    
  12. rbtools/clients/tests.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  13. rbtools/clients/tests.py (Diff revision 3)
     
     
    Col: 11
     W291 trailing whitespace
    
  14. rbtools/clients/tests.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  15. 
      
Shaurya Sengar
Barret Rennie
  1. 
      
  2. rbtools/clients/git.py (Diff revision 3)
     
     

    Since youre modifying the docstring for this function, could you reflow it into our document style.

    e.g.

    ```
    Single line summary

    Multi-line description

    Args:
    arg_name (type of arg_name):
    Description of arg name.

    Returns:
    return_type:
    Description of what is returned.
    """

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

    Needs a docstirng.

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

    Prefer '--remotes=%s' % remote. String interpolation is faster than concatenation in general.

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

    Missing a period.

    "at a commit" ?

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

    Again prefer interpolation here.

  7. 
      
Shaurya Sengar
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/git.py
    
    
  2. rbtools/clients/git.py (Diff revision 4)
     
     
    Col: 28
     W291 trailing whitespace
    
  3. rbtools/clients/tests.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. rbtools/clients/tests.py (Diff revision 4)
     
     
    Col: 11
     W291 trailing whitespace
    
  5. rbtools/clients/tests.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  6. rbtools/clients/tests.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  7. rbtools/clients/tests.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  8. rbtools/clients/tests.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  9. rbtools/clients/tests.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (113 > 79 characters)
    
  10. rbtools/clients/tests.py (Diff revision 4)
     
     
    Col: 11
     W291 trailing whitespace
    
  11. rbtools/clients/tests.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  12. rbtools/clients/tests.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  13. 
      
Shaurya Sengar
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/git.py
    
    
  2. rbtools/clients/git.py (Diff revision 5)
     
     
    Col: 28
     W291 trailing whitespace
    
  3. rbtools/clients/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. rbtools/clients/tests.py (Diff revision 5)
     
     
    Col: 11
     W291 trailing whitespace
    
  5. rbtools/clients/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  6. rbtools/clients/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  7. rbtools/clients/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  8. rbtools/clients/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  9. rbtools/clients/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (113 > 79 characters)
    
  10. rbtools/clients/tests.py (Diff revision 5)
     
     
    Col: 11
     W291 trailing whitespace
    
  11. rbtools/clients/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  12. rbtools/clients/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  13. rbtools/clients/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  14. rbtools/clients/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  15. rbtools/clients/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  16. rbtools/clients/tests.py (Diff revision 5)
     
     
    Col: 11
     W291 trailing whitespace
    
  17. rbtools/clients/tests.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  18. rbtools/clients/tests.py (Diff revision 5)
     
     
    Col: 11
     W291 trailing whitespace
    
  19. 
      
Barret Rennie
  1. 
      
  2. rbtools/clients/git.py (Diff revision 5)
     
     

    "Parse"

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

    "A dictionary with the following keys:

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

    Can you reformat this to be a reStructuredText definition list, e.g.

    ```python
    """

    A dictionary with the following keys:

    ``'base'`` (unicode):
        Description of 'base
    
    ``'tip'`` (unicode):
        Description of tip
    
    ``'commit_id'`` (unicode):
        Description of commit_id
    

    """

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

    Docstrings should be in the following format:

    """Single line summary
    
    Multi-line description
    """
    

    We also want to use the imperitive mood ("Return") here.

    This docstring will also need Args and Returns sections.

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

    Single quotes for strings.

  7. 
      
Shaurya Sengar
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/git.py
    
    
  2. rbtools/clients/tests.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. rbtools/clients/tests.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. rbtools/clients/tests.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  5. rbtools/clients/tests.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  6. rbtools/clients/tests.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  7. rbtools/clients/tests.py (Diff revision 6)
     
     
    Col: 11
     W291 trailing whitespace
    
  8. rbtools/clients/tests.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  9. rbtools/clients/tests.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  10. rbtools/clients/tests.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  11. rbtools/clients/tests.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  12. rbtools/clients/tests.py (Diff revision 6)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  13. 
      
Shaurya Sengar
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/git.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/git.py
    
    
  2. 
      
Shaurya Sengar
David Trowbridge
  1. I've been doing a bunch of testing on this, and after I make some style/documentation changes, I'm going to push this. Thanks for your work!

  2. 
      
Shaurya Sengar
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (b8d1b82)
Loading...