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)

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)

Show all issues

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

rbtools/clients/git.py (Diff revision 2)
Show all issues
```
Col: 17
 E113 unexpected indentation
```

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

Show all issues

Col: 50
 E261 at least two spaces before inline comment

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

Show all issues

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

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

Show all issues

 local variable 'ri' is assigned to but never used

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

Show all issues

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

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

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)

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)

Show all issues

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

rbtools/clients/git.py (Diff revision 3)
Show all issues
```
Col: 17
 E113 unexpected indentation
```

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

Show all issues

Col: 50
 E261 at least two spaces before inline comment

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

Show all issues

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

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

Show all issues

 local variable 'ri' is assigned to but never used

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

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)

Show all issues

 local variable 'ri' is assigned to but never used

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

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)

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)

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)

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)

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)

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)

Show all issues

Can you add a docstring covering exactly what this does and how it works?

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

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)

Show all issues

Should just be:

if not local_commits:

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

Show all issues

"a commit"

"that's also"

"available"

Should end with a period.

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

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)

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.

Review Board 6.0.1 alpha 0 (dev)

Adding in a smarter parent-base finding algorithm

Information

Reviewers

Groups:

Description:

Change Summary:

Testing Done:

Commit:

Diff:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Commit:

Diff:

Summary:

Description:

Testing Done:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

1. Simple case: Posting a feature branch off a tracking branch aligned with the remote.

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

3. Posting a feature branch off a branch not properly tracking the remote.

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

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

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

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

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

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

Others?

Status: Discarded