Summary

Adding in a smarter parent-base finding algorithm

Review Request #6797 — Created Jan. 17, 2015 and discarded Oct. 20, 2015, 4:21 p.m.

Information

Owner

tbelaire

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools, students

People

Description

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.
I've worked out a git-rev-list query to find this.
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.

Testing Done

Now with tests!

test_diff_finding_parent is a new test, and I had to modify test_parse_revision_spec_no_args_parent to pass, since the parent base is no longer the same after my modifications.

Issues

Description	From	Last Updated
Col: 80 E501 line too long (98 > 79 characters)	reviewbot	Jan. 18, 2015, 10:58 a.m.
Col: 17 E265 block comment should start with '# '	reviewbot	Feb. 8, 2015, 7:35 p.m.
Col: 17 E113 unexpected indentation	reviewbot	Feb. 8, 2015, 7:35 p.m.
I need to fix this to listen to the upstream_branch if it's explicitly set.	TB tbelaire	Feb. 22, 2015, 10:57 a.m.
Col: 50 E261 at least two spaces before inline comment	reviewbot	Feb. 8, 2015, 7:35 p.m.
Col: 80 E501 line too long (91 > 79 characters)	reviewbot	Feb. 8, 2015, 7:35 p.m.
Going to remove this hardcoded thing later.	TB tbelaire	Feb. 22, 2015, 10:56 a.m.
local variable 'ri' is assigned to but never used	reviewbot	Feb. 8, 2015, 7:35 p.m.
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	Feb. 8, 2015, 7:35 p.m.
Col: 17 E265 block comment should start with '# '	reviewbot	Feb. 8, 2015, 7:35 p.m.
Col: 17 E113 unexpected indentation	reviewbot	Feb. 8, 2015, 7:35 p.m.
Col: 50 E261 at least two spaces before inline comment	reviewbot	Feb. 8, 2015, 7:35 p.m.
Col: 80 E501 line too long (91 > 79 characters)	reviewbot	Feb. 8, 2015, 7:35 p.m.
local variable 'ri' is assigned to but never used	reviewbot	Feb. 8, 2015, 7:34 p.m.
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	Feb. 8, 2015, 7:34 p.m.
local variable 'ri' is assigned to but never used	reviewbot	Feb. 8, 2015, 7:34 p.m.
local variable '_ri' is assigned to but never used	reviewbot	Feb. 8, 2015, 7:34 p.m.
Can we change the terminology here a little bit? I'd like to use: online -> remote offline -> local feature …	david	Feb. 17, 2015, 3:24 p.m.
We should put blank lines around this block, since that's the way Christian likes it.	david	Feb. 22, 2015, 10:52 a.m.
local variable '_' is assigned to but never used	reviewbot	Feb. 8, 2015, 7:34 p.m.
Col: 36 E127 continuation line over-indented for visual indent	reviewbot	Feb. 22, 2015, 10:52 a.m.
_ is often used for gettext. Best to call this unused, or just fetch get_origin()[0].	chipx86	March 15, 2015, 8:30 p.m.
Can you add a docstring covering exactly what this does and how it works?	chipx86	March 15, 2015, 8:37 p.m.
This can pretty easily spit out a large number of commits, particularly for large repositories. I'd give this a try …	chipx86	Feb. 22, 2015, 2:26 p.m.
Should just be: if not local_commits:	chipx86	March 15, 2015, 8:48 p.m.
"a commit" "that's also" "available" Should end with a period.	chipx86	March 15, 2015, 8:48 p.m.
Can use self.assertIsInstance	VT VTL-Developer	March 15, 2015, 8:49 p.m.
Can use self.assertIn	VT VTL-Developer	March 15, 2015, 8:51 p.m.

Tool: Pyflakes
Processed Files:
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/git.py

rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (98 > 79 characters)
```

Groups:: students

Description:: It's still not very smart, and does no error checking.

But it looks like it's working.
+
+
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.
+
+
I've worked out a git-rev-list query to find this.
+
+
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.

Ship it!

```
Ship It!
```

Change Summary:

Now with tests!

Testing Done:

~		I need to learn how to test properly.
	~	Now with tests!
-		It kinda works on github.com/tbelaire/sandbox2.git, but I will make some tests once I learn how.

Commit:

f5a63b01645728ed69b303a75a3f5016f6f92d52

cf99a05cadbeb55d56d87f25155102a8e46c5e26

Diff:

Revision 2 (+41 -4)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests.py

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

rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 17
 E265 block comment should start with '# '
```
rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 17
 E113 unexpected indentation
```
rbtools/clients/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 50
 E261 at least two spaces before inline comment
```
rbtools/clients/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (91 > 79 characters)
```
rbtools/clients/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 local variable 'ri' is assigned to but never used
```
rbtools/clients/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (84 > 79 characters)
```

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

The issue has been resolved. Show all issues

I need to fix this to listen to the upstream_branch if it's explicitly set.

rbtools/clients/tests.py (Diff revision 2)

The issue has been resolved. Show all issues

Going to remove this hardcoded thing later.

tbelaire Feb. 22, 2015, 10:56 a.m.

I'm not sure if I should just delete this line. It was a useful sanity check when setting up the test, but it makes it rather brittle to changes later on.

Eh, I'll just delete it.

I haven't looked at the code yet, but you can add bug 3510 to this.

tbelaire Jan. 18, 2015, 11:11 a.m.

https://code.google.com/p/reviewboard/issues/detail?can=2&q=3510
Hmm, I'm not trying to fix that explicitly.

All this does it use the normal calculation for base, and try and find the most optimal parent_base.

david Jan. 18, 2015, 11:20 a.m.
```
Ah yes, I misread the bug.
```

What's happened to this patch? We haven't seen a code drop in a while. Theo, can you please update this today?

tbelaire Feb. 8, 2015, 4:28 p.m.

Sorry, I had forgottan to publish the draft after I rbt pushed it.

Commit:

cf99a05cadbeb55d56d87f25155102a8e46c5e26

9983dd039d857797a268d6a03ed5efb1fe8569d5

Diff:

Revision 3 (+54 -6)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests.py

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

rbtools/clients/git.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 17
 E265 block comment should start with '# '
```
rbtools/clients/git.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 17
 E113 unexpected indentation
```
rbtools/clients/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 50
 E261 at least two spaces before inline comment
```
rbtools/clients/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (91 > 79 characters)
```
rbtools/clients/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
 local variable 'ri' is assigned to but never used
```
rbtools/clients/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (84 > 79 characters)
```

rbtools/clients/tests.py (Diff revisions 2 - 3)

This is because we are now selecting a better parent_base. I wouldn't mind someone doublechecking that this is a correct change to the test.

Change Summary:

Fixed ReviewBot Issues.

Commit:

9983dd039d857797a268d6a03ed5efb1fe8569d5

74e01b5d67ef539bf607ab591cec52526242924c

Diff:

Revision 4 (+55 -6)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests.py

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

rbtools/clients/tests.py (Diff revision 4)
The issue has been resolved. Show all issues
```
 local variable 'ri' is assigned to but never used
```
rbtools/clients/tests.py (Diff revision 4)
The issue has been resolved. Show all issues
```
 local variable '_ri' is assigned to but never used
```

Commit:

74e01b5d67ef539bf607ab591cec52526242924c

42667578d32de1e6ff9b1fb272c6f7f5d5f8f86d

Diff:

Revision 5 (+55 -6)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests.py

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

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

The issue has been dropped. Show all issues

 local variable '_' is assigned to but never used

tbelaire Feb. 8, 2015, 7:34 p.m.

I'm pretty sure that's a bug, _ is totally allowed to be assigned to and not used.

david Feb. 17, 2015, 12:37 p.m.

Yeah, but it's probably more appropriate to just not save the result of the function to any variable (just have self.client.get_repository_info()).

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

The issue has been resolved. Show all issues

Can we change the terminology here a little bit?
I'd like to use:

online -> remote

offline -> local

feature -> local_branch

origin -> remote

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

The issue has been resolved. Show all issues

We should put blank lines around this block, since that's the way Christian likes it.

Summary:

WIP: Adding in a smarter parent-base finding algorithm

Adding in a smarter parent-base finding algorithm

Description:

-		It's still not very smart, and does no error checking.
-
-		But it looks like it's working.
-
		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.

		I've worked out a `git-rev-list` query to find this.

		`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.

Testing Done:

		Now with tests!
	+
	+
	+
	+	`test_diff_finding_parent` is a new test, and I had to modify `test_parse_revision_spec_no_args_parent` to pass, since the parent base is no longer the same after my modifications.

Commit:

42667578d32de1e6ff9b1fb272c6f7f5d5f8f86d

f3e2320da11bcdb032cb9a9671eb17a613f66983

Diff:

Revision 6 (+57 -6)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests.py

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

rbtools/clients/git.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 36
 E127 continuation line over-indented for visual indent
```

Commit:

f3e2320da11bcdb032cb9a9671eb17a613f66983

8b95ef2362922fd4aeb37854149b9c570c25ca30

Diff:

Revision 7 (+57 -6)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests.py

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

Commit:

8b95ef2362922fd4aeb37854149b9c570c25ca30

a59c4e8e53a903cb4ce576b19dca9d73c0eb2c6b

Diff:

Revision 8 (+55 -6)

Show changes

	rbtools/clients/git.py
	rbtools/clients/tests.py

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

Fix it!

Good work so far!
Since this is a pretty fundamental, hard-to-get-right change, I want to see a lot more than one test. I can think of several more that need to be added:

1. Simple case: Posting a feature branch off a tracking branch aligned with the remote.
This is the basic case of:

o [feature-branch]
|
o [origin/remote-branch] [remote-branch]
|
|  o [origin/master] [master]
|  |


It should detect origin/remote-branch.

2. Posting a feature branch off a tracking branch with changes since the remote.

o [feature-branch]
|
o [remote-branch]
|
o [origin/remote-branch]
|
|  o [origin/master] [master]
|  |


It should still detect origin/remote-branch.

3. Posting a feature branch off a branch not properly tracking the remote.
Same as #1 above, but with remote-branch not being set to track origin/remote-branch (meaning it's a regular branch and not set to track in the git config).
It should still detect origin/remote-branch.

4. Posting a feature branch with a merged tracking branch off another tracking branch.

o [feature-branch]
|\
| \
|  o [origin/merged-branch]
|  |
| ...
|
o [origin/remote-branch] [remote-branch]
|


This is harder, because it could go either way. However, I think the user would expect that it would show the merged changes, meaning that origin/remote-branch would be the right branch to detect.

5. Posting a feature branch off a tracking branch that merged another tracking branch.

o [feature-branch]
|
o [origin/remote-branch] [remote-branch]
|\
| \
|  o [origin/merged-branch]
|  |


Should be fairly straight-forward, but good to have for testing. Should be the same as #1 above, really.

6. Posting a feature branch off a remote branch without any tracking branches.

o [feature-branch]
|
o [origin/remote-branch]
|


The tracking branch shouldn't matter so much. It should still return origin/remote-branch.

7. Posting a feature branch off a remote branch aligned to the same commit as another remote branch.

o [feature-branch]
|
o [origin/remote-branch1] [origin/remote-branch2]
|


Obviously either one of these is fine, but it should reliably choose one of these in a predictable way.

8. Posting a feature branch not up-to-date with the remote branch.

o [feature-branch]
|
|   o [origin/remote-branch] [remote-branch]
|  /
| /
o
|
o [origin/master]
|


I wish there was a good way to have this detect origin/remote-branch, but that's unlikely to be possible. It should return origin/master if auto-detecting.
However, if the user has hard-coded TRACKING_BRANCH = 'origin/remote-branch', then it should be smart and use the common commit they share, to avoid a lot of the problems people hit today where the diff gets all screwed up with things reversed from origin/remote-branch.

9. Posting a feature branch with branches from different remotes in the path.

o [feature-branch]
|
o [other-remote/remote-branch]
|
o [origin/remote-branch]
|


The other remotes should be ignored. It should only fetch origin/remote-branch.

Others?
There may be other situations that come up, but this is a good set of cases that all need to be handled properly in order to not break things.
We also need to ensure that performance is fine with large (multi-gigabyte) repositories.

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

The issue has been resolved. Show all issues

_ is often used for gettext. Best to call this unused, or just fetch get_origin()[0].

tbelaire March 15, 2015, 8:51 p.m.

Sorry about that. I was used to languages that enforce _ as a throwaway like /dev/null.

rbtools/clients/git.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Can you add a docstring covering exactly what this does and how it works?
```

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

An issue was opened. Show all issues

This can pretty easily spit out a large number of commits, particularly for large repositories. I'd give this a try against some known large repositories, like the KDE or Linux kernel repositories, and see how it handles.

When run against the Review Board repository with a commit on master, I get over 6,300 commits back.

tbelaire March 15, 2015, 8:59 p.m.

I downloaded a copy of the linux source code, but I am unable to reproduce this. 

It should only return the comments that exist locally, but not in the remote.
$ cd linux
$ time git rev-list master --not --remotes=origin

real    0m0.088s
user    0m0.007s
sys     0m0.081s

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

The issue has been resolved. Show all issues

Should just be:

if not local_commits:

rbtools/clients/git.py (Diff revision 8)
The issue has been resolved. Show all issues
```
"a commit"
"that's also"
"available"
Should end with a period.
```

rbtools/clients/tests.py (Diff revision 8)

The issue has been dropped. Show all issues

Can use self.assertIsInstance

brennie March 3, 2015, 10:40 p.m.

Actually, we can't use self.assertIsInstance; it was added in Python 2.7 and we have to support Python 2.6.

rbtools/clients/tests.py (Diff revision 8)

The issue has been dropped. Show all issues

Can use self.assertIn

tbelaire March 15, 2015, 8:59 p.m.

I'm going to keep with the style used in the rest of the tests for now, though maybe I'll come back and change the whole file to use that.

Status:: Discarded