Adding in a smarter parent-base finding algorithm

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

Information

RBTools
master

Reviewers

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.

Description From Last Updated

Col: 55 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 54 E262 inline comment should start with '# '

reviewbotreviewbot

Col: 53 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 55 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 55 W291 trailing whitespace

reviewbotreviewbot

Col: 55 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 61 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 55 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Since youre modifying the docstring for this function, could you reflow it into our document style. e.g. ``` Single line …

brenniebrennie

Needs a docstirng.

brenniebrennie

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

brenniebrennie

Missing a period. "at a commit" ?

brenniebrennie

Again prefer interpolation here.

brenniebrennie

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

reviewbotreviewbot

Col: 11 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 11 W291 trailing whitespace

reviewbotreviewbot

Col: 11 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 11 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 28 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 11 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 11 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

"Parse"

brenniebrennie

Col: 28 W291 trailing whitespace

reviewbotreviewbot

"A dictionary with the following keys:

brenniebrennie

Can you reformat this to be a reStructuredText definition list, e.g. ```python """ A dictionary with the following keys: ``'base'`` …

brenniebrennie

Docstrings should be in the following format: """Single line summary Multi-line description """ We also want to use the imperitive …

brenniebrennie

Single quotes for strings.

brenniebrennie

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

reviewbotreviewbot

Col: 11 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 11 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 11 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

Col: 11 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 11 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot
reviewbot
  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)
     
     
    Show all issues
    Col: 55
     W291 trailing whitespace
    
  3. rbtools/clients/tests.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  4. rbtools/clients/tests.py (Diff revision 1)
     
     
    Show all issues
    Col: 54
     E262 inline comment should start with '# '
    
  5. rbtools/clients/tests.py (Diff revision 1)
     
     
    Show all issues
    Col: 53
     E261 at least two spaces before inline comment
    
  6. rbtools/clients/tests.py (Diff revision 1)
     
     
    Show all issues
    Col: 55
     W291 trailing whitespace
    
  7. rbtools/clients/tests.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  8. 
      
SS
SS
reviewbot
  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)
     
     
    Show all issues
    Col: 55
     W291 trailing whitespace
    
  3. rbtools/clients/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 55
     W291 trailing whitespace
    
  4. rbtools/clients/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  5. rbtools/clients/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 61
     W291 trailing whitespace
    
  6. rbtools/clients/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  7. rbtools/clients/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 55
     W291 trailing whitespace
    
  8. rbtools/clients/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  9. 
      
SS
SS
chipx86
  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. 
      
SS
reviewbot
  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)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. rbtools/clients/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 11
     W291 trailing whitespace
    
  4. rbtools/clients/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  5. rbtools/clients/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  6. rbtools/clients/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  7. rbtools/clients/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  8. rbtools/clients/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  9. rbtools/clients/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (113 > 79 characters)
    
  10. rbtools/clients/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 11
     W291 trailing whitespace
    
  11. rbtools/clients/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 11
     W291 trailing whitespace
    
  12. rbtools/clients/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  13. rbtools/clients/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 11
     W291 trailing whitespace
    
  14. rbtools/clients/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  15. 
      
SS
brennie
  1. 
      
  2. rbtools/clients/git.py (Diff revision 3)
     
     
    Show all issues

    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)
     
     
    Show all issues

    Needs a docstirng.

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

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

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

    Missing a period.

    "at a commit" ?

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

    Again prefer interpolation here.

  7. 
      
SS
reviewbot
  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)
     
     
    Show all issues
    Col: 28
     W291 trailing whitespace
    
  3. rbtools/clients/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. rbtools/clients/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 11
     W291 trailing whitespace
    
  5. rbtools/clients/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  6. rbtools/clients/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  7. rbtools/clients/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  8. rbtools/clients/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  9. rbtools/clients/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (113 > 79 characters)
    
  10. rbtools/clients/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 11
     W291 trailing whitespace
    
  11. rbtools/clients/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  12. rbtools/clients/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  13. 
      
SS
reviewbot
  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)
     
     
    Show all issues
    Col: 28
     W291 trailing whitespace
    
  3. rbtools/clients/tests.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. rbtools/clients/tests.py (Diff revision 5)
     
     
    Show all issues
    Col: 11
     W291 trailing whitespace
    
  5. rbtools/clients/tests.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  6. rbtools/clients/tests.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  7. rbtools/clients/tests.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  8. rbtools/clients/tests.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  9. rbtools/clients/tests.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (113 > 79 characters)
    
  10. rbtools/clients/tests.py (Diff revision 5)
     
     
    Show all issues
    Col: 11
     W291 trailing whitespace
    
  11. rbtools/clients/tests.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  12. rbtools/clients/tests.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  13. rbtools/clients/tests.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  14. rbtools/clients/tests.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  15. rbtools/clients/tests.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  16. rbtools/clients/tests.py (Diff revision 5)
     
     
    Show all issues
    Col: 11
     W291 trailing whitespace
    
  17. rbtools/clients/tests.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  18. rbtools/clients/tests.py (Diff revision 5)
     
     
    Show all issues
    Col: 11
     W291 trailing whitespace
    
  19. 
      
brennie
  1. 
      
  2. rbtools/clients/git.py (Diff revision 5)
     
     
    Show all issues

    "Parse"

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

    "A dictionary with the following keys:

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

    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)
     
     
     
     
    Show all issues

    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)
     
     
    Show all issues

    Single quotes for strings.

  7. 
      
SS
reviewbot
  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)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. rbtools/clients/tests.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. rbtools/clients/tests.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  5. rbtools/clients/tests.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  6. rbtools/clients/tests.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  7. rbtools/clients/tests.py (Diff revision 6)
     
     
    Show all issues
    Col: 11
     W291 trailing whitespace
    
  8. rbtools/clients/tests.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  9. rbtools/clients/tests.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  10. rbtools/clients/tests.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  11. rbtools/clients/tests.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  12. rbtools/clients/tests.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  13. 
      
SS
reviewbot
  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. 
      
SS
david
  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. 
      
SS
Review request changed
Status:
Completed
Change Summary:
Pushed to master (b8d1b82)