Adding in a smarter parent-base finding algorithm
Review Request #6797 — Created Jan. 17, 2015 and discarded
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 branchorigin/*
. We would also likeparent_base
to be as young an ancestor oftip
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.
Now with tests!
test_diff_finding_parent
is a new test, and I had to modifytest_parse_revision_spec_no_args_parent
to pass, since the parent base is no longer the same after my modifications.
Description | From | Last Updated |
---|---|---|
Col: 80 E501 line too long (98 > 79 characters) |
![]() |
|
Col: 17 E265 block comment should start with '# ' |
![]() |
|
Col: 17 E113 unexpected indentation |
![]() |
|
I need to fix this to listen to the upstream_branch if it's explicitly set. |
TB tbelaire | |
Col: 50 E261 at least two spaces before inline comment |
![]() |
|
Col: 80 E501 line too long (91 > 79 characters) |
![]() |
|
Going to remove this hardcoded thing later. |
TB tbelaire | |
local variable 'ri' is assigned to but never used |
![]() |
|
Col: 80 E501 line too long (84 > 79 characters) |
![]() |
|
Col: 17 E265 block comment should start with '# ' |
![]() |
|
Col: 17 E113 unexpected indentation |
![]() |
|
Col: 50 E261 at least two spaces before inline comment |
![]() |
|
Col: 80 E501 line too long (91 > 79 characters) |
![]() |
|
local variable 'ri' is assigned to but never used |
![]() |
|
Col: 80 E501 line too long (84 > 79 characters) |
![]() |
|
local variable 'ri' is assigned to but never used |
![]() |
|
local variable '_ri' is assigned to but never used |
![]() |
|
Can we change the terminology here a little bit? I'd like to use: online -> remote offline -> local feature … |
|
|
We should put blank lines around this block, since that's the way Christian likes it. |
|
|
local variable '_' is assigned to but never used |
![]() |
|
Col: 36 E127 continuation line over-indented for visual indent |
![]() |
|
_ is often used for gettext. Best to call this unused, or just fetch get_origin()[0]. |
|
|
Can you add a docstring covering exactly what this does and how it works? |
|
|
This can pretty easily spit out a large number of commits, particularly for large repositories. I'd give this a try … |
|
|
Should just be: if not local_commits: |
|
|
"a commit" "that's also" "available" Should end with a period. |
|
|
Can use self.assertIsInstance |
VT VTL-Developer | |
Can use self.assertIn |
VT VTL-Developer |
Description: |
|
---|
Change Summary:
Now with tests!
Testing Done: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||
Diff: |
Revision 2 (+41 -4) |

-
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) I need to fix this to listen to the upstream_branch if it's explicitly set.
-
-
What's happened to this patch? We haven't seen a code drop in a while. Theo, can you please update this today?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+54 -6) |

-
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 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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+55 -6) |

-
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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+55 -6) |

-
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 5) 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) We should put blank lines around this block, since that's the way Christian likes it.
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 6 (+57 -6) |

-
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) Col: 36 E127 continuation line over-indented for visual indent
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+57 -6) |

-
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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+55 -6) |

-
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
-
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 trackorigin/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 returnorigin/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 fromorigin/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) _
is often used for gettext. Best to call thisunused
, or just fetchget_origin()[0]
. -
rbtools/clients/git.py (Diff revision 8) Can you add a docstring covering exactly what this does and how it works?
-
rbtools/clients/git.py (Diff revision 8) 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.
-
-
rbtools/clients/git.py (Diff revision 8) "a commit"
"that's also"
"available"
Should end with a period.