Merge ~racb/git-ubuntu:fix-rich-history-unescaped-patch into git-ubuntu:master
- Git
- lp:~racb/git-ubuntu
- fix-rich-history-unescaped-patch
- Merge into master
Status: | Merged |
---|---|
Merged at revision: | 0376feae802c37ba6e71b7eca899230c32bc7904 |
Proposed branch: | ~racb/git-ubuntu:fix-rich-history-unescaped-patch |
Merge into: | git-ubuntu:master |
Diff against target: |
262 lines (+81/-58) 4 files modified
doc/STYLE.md (+0/-9) gitubuntu/rich_history.py (+17/-15) gitubuntu/rich_history_test.py (+64/-14) gitubuntu/test_fixtures.py (+0/-20) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Server Team CI bot | continuous-integration | Approve | |
Bryce Harrington | Approve | ||
Review via email: mp+383597@code.launchpad.net |
Commit message
Make Jenkins happy
Description of the change
Server Team CI bot (server-team-bot) wrote : | # |
Bryce Harrington (bryce) wrote : | # |
8f84439e71276f9
- Test refactoring, to make it use a single repo rather than source
and dest ones.
- Drops the repo_fixture
- LGTM
√ Testsuite result: passed
1c52d81028861d1
- Adds xfail test for bug about patch in message
- I would suggest some verbage changes to the test description:
"""Commit messages with patch-like contents should round trip
If a commit message in rich history contains text in a patch-like
format (such as a diff of some file), it should not prevent correct
round-tripping. "git format-patch" followed by "git am" fails this
test, for example.
...
"""
- + :param repo: our standard repo fixture.
Should be ":param Repo repo: our..."
√ Testsuite result: passed
c2555935fcb8fc6
- Changes from using git am to git cherry-pick, the main implication
is needing to hand it commit id's rather than patch mbox's.
- This feels like it will be far more robust
- One xfail test case is re-enabled
√ Testsuite result: passed
- LGTM
e30452d73045c9a
- LGTM
This MP is well organized, first introducing an xfail test case that catches the bug, then the fix, and separating out a preliminary refactoring to support the test, and a cleanup after. I verified the xfail case does indeed fail initially, and is fixed in subsequent commits.
The fix itself seems a lot more robust to me, because it shifts to rely on git cherry-pick rather than git am and in my experience the former is a lot stricter and thus more reliable for our use case. cherry-pick leverages git's internal functionality, while git am's nature has it limited by text file i/o and the patch file format.
I have a couple suggestions on improvements for test case pydocs, but nothing major and no need for re-review.
Robie Basak (racb) wrote : | # |
Thanks. Agreed with the docstring change. I have updated the patchset and pushed. Now awaiting CI result before landing.
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:0376feae802
https:/
Executed test runs:
SUCCESS: VM Setup
SUCCESS: Build
SUCCESS: Unit Tests
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
Preview Diff
1 | diff --git a/doc/STYLE.md b/doc/STYLE.md | |||
2 | index 8a54d95..eb1e166 100644 | |||
3 | --- a/doc/STYLE.md | |||
4 | +++ b/doc/STYLE.md | |||
5 | @@ -101,12 +101,3 @@ We use [pytest fixtures](https://docs.pytest.org/en/latest/fixture.html) | |||
6 | 101 | to help us with this. Use of pytest's built-in `tmpdir` fixture is very | 101 | to help us with this. Use of pytest's built-in `tmpdir` fixture is very |
7 | 102 | common, as is `repo` and `pygit2_repo` as defined in | 102 | common, as is `repo` and `pygit2_repo` as defined in |
8 | 103 | `gitubuntu.test_fixtures`. | 103 | `gitubuntu.test_fixtures`. |
9 | 104 | |||
10 | 105 | Sometimes a test needs multiple instances of these fixtures. | ||
11 | 106 | `gitubuntu.test_fixtures.repo_factory` achieves this for the `repo` | ||
12 | 107 | fixture, and | ||
13 | 108 | `gitubuntu.rich_history_test::test_rich_history_preservation` is an | ||
14 | 109 | example of its use. A similar pattern may be used if multiple instances | ||
15 | 110 | of other fixtures become necessary. In time it may make sense to | ||
16 | 111 | refactor this into a generic "make a factory out of this fixture" | ||
17 | 112 | function. | ||
18 | diff --git a/gitubuntu/rich_history.py b/gitubuntu/rich_history.py | |||
19 | index ac178fb..c474f3f 100644 | |||
20 | --- a/gitubuntu/rich_history.py | |||
21 | +++ b/gitubuntu/rich_history.py | |||
22 | @@ -63,11 +63,13 @@ def export_upload_tag(repo, path, upload_tag, commit_map): | |||
23 | 63 | """Export a single upload tag for later reconstruction | 63 | """Export a single upload tag for later reconstruction |
24 | 64 | 64 | ||
25 | 65 | If successful, two files will be created in the output directory: one with | 65 | If successful, two files will be created in the output directory: one with |
27 | 66 | extension '.base', and one with extension '.mbox'. The base name of these | 66 | extension '.base', and one with extension '.pick'. The base name of these |
28 | 67 | files is the version string from the upload tag. The '.base' file will | 67 | files is the version string from the upload tag. The '.base' file will |
29 | 68 | contain a single LF-terminated string which is the version string from the | 68 | contain a single LF-terminated string which is the version string from the |
32 | 69 | import tag on which the upload tag patchset is based. The '.mbox' file will | 69 | import tag on which the upload tag patchset is based. The '.pick' file will |
33 | 70 | contain the patchset as exported using 'git format-patch --stdout'. | 70 | contain the patchset described as a series of commit hash strings that can |
34 | 71 | be picked in order, one on each line, as described by 'git rev-list | ||
35 | 72 | --reverse'. | ||
36 | 71 | 73 | ||
37 | 72 | Note that the upload and import tag names are already escaped according to | 74 | Note that the upload and import tag names are already escaped according to |
38 | 73 | dep14, so the output filenames will have their version parts escaped | 75 | dep14, so the output filenames will have their version parts escaped |
39 | @@ -81,7 +83,7 @@ def export_upload_tag(repo, path, upload_tag, commit_map): | |||
40 | 81 | :raises BaseNotFoundError: if a base import tag cannot be found. | 83 | :raises BaseNotFoundError: if a base import tag cannot be found. |
41 | 82 | :raises MultipleParentError: if multiple parents for a commit are found. | 84 | :raises MultipleParentError: if multiple parents for a commit are found. |
42 | 83 | :raises subprocess.CalledProcessError: if the underlying call to "git | 85 | :raises subprocess.CalledProcessError: if the underlying call to "git |
44 | 84 | format-patch" fails. | 86 | rev-list" fails. |
45 | 85 | :returns: None. | 87 | :returns: None. |
46 | 86 | """ | 88 | """ |
47 | 87 | import_tag = get_upload_tag_base(repo, upload_tag, commit_map) | 89 | import_tag = get_upload_tag_base(repo, upload_tag, commit_map) |
48 | @@ -96,12 +98,12 @@ def export_upload_tag(repo, path, upload_tag, commit_map): | |||
49 | 96 | initial_env=repo._initial_env, | 98 | initial_env=repo._initial_env, |
50 | 97 | update_env=repo.env, | 99 | update_env=repo.env, |
51 | 98 | ) | 100 | ) |
53 | 99 | with open(os.path.join(path, export_name) + '.mbox', 'w') as f: | 101 | with open(os.path.join(path, export_name) + '.pick', 'w') as f: |
54 | 100 | subprocess.check_call( | 102 | subprocess.check_call( |
55 | 101 | [ | 103 | [ |
56 | 102 | 'git', | 104 | 'git', |
59 | 103 | 'format-patch', | 105 | 'rev-list', |
60 | 104 | '--stdout', | 106 | '--reverse', |
61 | 105 | '%s..%s' % (import_tag.name, upload_tag.name), | 107 | '%s..%s' % (import_tag.name, upload_tag.name), |
62 | 106 | ], | 108 | ], |
63 | 107 | env=env, | 109 | env=env, |
64 | @@ -155,8 +157,8 @@ def import_single(repo, path, version, namespace='importer'): | |||
65 | 155 | :raises FileNotFoundError: if rich history cannot be found for this | 157 | :raises FileNotFoundError: if rich history cannot be found for this |
66 | 156 | version. | 158 | version. |
67 | 157 | :raises BaseNotFoundError: if a base import tag cannot be found. | 159 | :raises BaseNotFoundError: if a base import tag cannot be found. |
70 | 158 | :raises subprocess.CalledProcessError: if the underlying call to "git | 160 | :raises subprocess.CalledProcessError: if any of the underlying calls to |
71 | 159 | am" or "git tag" fail. | 161 | "git cherry-pick" or "git tag" fail. |
72 | 160 | :returns: None. | 162 | :returns: None. |
73 | 161 | """ | 163 | """ |
74 | 162 | version_name = gitubuntu.git_repository.git_dep14_tag(version) | 164 | version_name = gitubuntu.git_repository.git_dep14_tag(version) |
75 | @@ -170,17 +172,17 @@ def import_single(repo, path, version, namespace='importer'): | |||
76 | 170 | except KeyError as e: | 172 | except KeyError as e: |
77 | 171 | raise BaseNotFoundError() from e | 173 | raise BaseNotFoundError() from e |
78 | 172 | 174 | ||
80 | 173 | with open(os.path.join(path, version_name) + '.mbox', 'r') as f: | 175 | with open(os.path.join(path, version_name) + '.pick', 'r') as pick: |
81 | 174 | with repo.temporary_worktree(import_tag_name, 'rich-history-import'): | 176 | with repo.temporary_worktree(import_tag_name, 'rich-history-import'): |
82 | 175 | env = { | 177 | env = { |
83 | 176 | 'GIT_COMMITTER_NAME': spec.SYNTHESIZED_COMMITTER_NAME, | 178 | 'GIT_COMMITTER_NAME': spec.SYNTHESIZED_COMMITTER_NAME, |
84 | 177 | 'GIT_COMMITTER_EMAIL': spec.SYNTHESIZED_COMMITTER_EMAIL, | 179 | 'GIT_COMMITTER_EMAIL': spec.SYNTHESIZED_COMMITTER_EMAIL, |
85 | 178 | } | 180 | } |
91 | 179 | subprocess.check_call( | 181 | for commit_string in pick: |
92 | 180 | ['git', 'am'], | 182 | subprocess.check_call( |
93 | 181 | stdin=f, | 183 | ['git', 'cherry-pick', commit_string.strip()], |
94 | 182 | env=env, | 184 | env=env, |
95 | 183 | ) | 185 | ) |
96 | 184 | upload_tag_name = '%s/upload/%s' % (namespace, version_name) | 186 | upload_tag_name = '%s/upload/%s' % (namespace, version_name) |
97 | 185 | subprocess.check_call( | 187 | subprocess.check_call( |
98 | 186 | [ | 188 | [ |
99 | diff --git a/gitubuntu/rich_history_test.py b/gitubuntu/rich_history_test.py | |||
100 | index 5223f2d..1716546 100644 | |||
101 | --- a/gitubuntu/rich_history_test.py | |||
102 | +++ b/gitubuntu/rich_history_test.py | |||
103 | @@ -5,12 +5,12 @@ import pytest | |||
104 | 5 | 5 | ||
105 | 6 | from gitubuntu.repo_builder import Blob, Commit, Placeholder, Repo, Tree | 6 | from gitubuntu.repo_builder import Blob, Commit, Placeholder, Repo, Tree |
106 | 7 | import gitubuntu.spec as spec | 7 | import gitubuntu.spec as spec |
108 | 8 | from gitubuntu.test_fixtures import repo, repo_factory | 8 | from gitubuntu.test_fixtures import repo |
109 | 9 | 9 | ||
110 | 10 | import gitubuntu.rich_history as target | 10 | import gitubuntu.rich_history as target |
111 | 11 | 11 | ||
112 | 12 | 12 | ||
114 | 13 | def test_rich_history_preservation(tmpdir, repo_factory): | 13 | def test_rich_history_preservation(tmpdir, repo): |
115 | 14 | """An export followed by an import should preserve rich history | 14 | """An export followed by an import should preserve rich history |
116 | 15 | 15 | ||
117 | 16 | Given a minimal repository that should be able to have upload tags be | 16 | Given a minimal repository that should be able to have upload tags be |
118 | @@ -19,11 +19,8 @@ def test_rich_history_preservation(tmpdir, repo_factory): | |||
119 | 19 | the result should be a correctly reconstructed upload tag. | 19 | the result should be a correctly reconstructed upload tag. |
120 | 20 | 20 | ||
121 | 21 | :param py.path tmpdir: the pytest standard tmpdir fixture. | 21 | :param py.path tmpdir: the pytest standard tmpdir fixture. |
123 | 22 | :param repo_factory: our standard repo_factory fixture. | 22 | :param repo: our standard repo fixture. |
124 | 23 | """ | 23 | """ |
125 | 24 | source_repo = next(repo_factory) | ||
126 | 25 | dest_repo = next(repo_factory) | ||
127 | 26 | |||
128 | 27 | Repo( | 24 | Repo( |
129 | 28 | commits=[ | 25 | commits=[ |
130 | 29 | Commit( | 26 | Commit( |
131 | @@ -51,8 +48,9 @@ def test_rich_history_preservation(tmpdir, repo_factory): | |||
132 | 51 | 'importer/import/1': Placeholder('1'), | 48 | 'importer/import/1': Placeholder('1'), |
133 | 52 | 'importer/upload/2': Placeholder('3'), | 49 | 'importer/upload/2': Placeholder('3'), |
134 | 53 | } | 50 | } |
137 | 54 | ).write(source_repo.raw_repo) | 51 | ).write(repo.raw_repo) |
138 | 55 | target.export_all(source_repo, str(tmpdir)) | 52 | target.export_all(repo, str(tmpdir)) |
139 | 53 | repo.delete_tags_in_namespace('importer') | ||
140 | 56 | Repo( | 54 | Repo( |
141 | 57 | commits=[ | 55 | commits=[ |
142 | 58 | Commit( | 56 | Commit( |
143 | @@ -64,9 +62,9 @@ def test_rich_history_preservation(tmpdir, repo_factory): | |||
144 | 64 | ) | 62 | ) |
145 | 65 | ], | 63 | ], |
146 | 66 | tags={'importer/import/1': Placeholder('1')}, | 64 | tags={'importer/import/1': Placeholder('1')}, |
150 | 67 | ).write(dest_repo.raw_repo) | 65 | ).write(repo.raw_repo) |
151 | 68 | target.import_single(dest_repo, tmpdir, '2') | 66 | target.import_single(repo, tmpdir, '2') |
152 | 69 | new_tag = dest_repo.raw_repo.lookup_reference( | 67 | new_tag = repo.raw_repo.lookup_reference( |
153 | 70 | 'refs/tags/importer/upload/2', | 68 | 'refs/tags/importer/upload/2', |
154 | 71 | ).peel(pygit2.Tag) | 69 | ).peel(pygit2.Tag) |
155 | 72 | assert new_tag.tagger.name == spec.SYNTHESIZED_COMMITTER_NAME | 70 | assert new_tag.tagger.name == spec.SYNTHESIZED_COMMITTER_NAME |
156 | @@ -75,8 +73,8 @@ def test_rich_history_preservation(tmpdir, repo_factory): | |||
157 | 75 | assert new_commit.committer.name == spec.SYNTHESIZED_COMMITTER_NAME | 73 | assert new_commit.committer.name == spec.SYNTHESIZED_COMMITTER_NAME |
158 | 76 | assert new_commit.committer.email == spec.SYNTHESIZED_COMMITTER_EMAIL | 74 | assert new_commit.committer.email == spec.SYNTHESIZED_COMMITTER_EMAIL |
159 | 77 | new_tree = new_commit.peel(pygit2.Tree) | 75 | new_tree = new_commit.peel(pygit2.Tree) |
162 | 78 | assert dest_repo.raw_repo.get(new_tree['a'].id).data == b'abc' | 76 | assert repo.raw_repo.get(new_tree['a'].id).data == b'abc' |
163 | 79 | assert dest_repo.raw_repo.get(new_tree['b'].id).data == b'b' | 77 | assert repo.raw_repo.get(new_tree['b'].id).data == b'b' |
164 | 80 | 78 | ||
165 | 81 | 79 | ||
166 | 82 | def test_rich_history_preservation_multiple_parents(tmpdir, repo): | 80 | def test_rich_history_preservation_multiple_parents(tmpdir, repo): |
167 | @@ -89,7 +87,7 @@ def test_rich_history_preservation_multiple_parents(tmpdir, repo): | |||
168 | 89 | code. | 87 | code. |
169 | 90 | 88 | ||
170 | 91 | :param py.path tmpdir: the pytest standard tmpdir fixture. | 89 | :param py.path tmpdir: the pytest standard tmpdir fixture. |
172 | 92 | :param repo_factory: our standard repo_factory fixture. | 90 | :param repo: our standard repo fixture. |
173 | 93 | """ | 91 | """ |
174 | 94 | Repo( | 92 | Repo( |
175 | 95 | commits=[ | 93 | commits=[ |
176 | @@ -147,3 +145,55 @@ def test_rich_history_import_no_base(tmpdir, repo): | |||
177 | 147 | tmpdir.join('2.base').write("1\n") | 145 | tmpdir.join('2.base').write("1\n") |
178 | 148 | with pytest.raises(target.BaseNotFoundError): | 146 | with pytest.raises(target.BaseNotFoundError): |
179 | 149 | target.import_single(repo, str(tmpdir), '2') | 147 | target.import_single(repo, str(tmpdir), '2') |
180 | 148 | |||
181 | 149 | |||
182 | 150 | def test_rich_history_preservation_patch_in_message(tmpdir, repo): | ||
183 | 151 | """Commit messages with patch-like contents should round trip | ||
184 | 152 | |||
185 | 153 | If a commit message in rich history contains text in a patch-like | ||
186 | 154 | format (such as a diff of some file), it should not prevent correct | ||
187 | 155 | round-tripping. "git format-patch" followed by "git am" fails this | ||
188 | 156 | test, for example. | ||
189 | 157 | |||
190 | 158 | :param py.path tmpdir: the pytest standard tmpdir fixture. | ||
191 | 159 | :param GitUbuntuRepository repo: our standard repo fixture. | ||
192 | 160 | """ | ||
193 | 161 | Repo( | ||
194 | 162 | commits=[ | ||
195 | 163 | Commit( | ||
196 | 164 | name='1', | ||
197 | 165 | message='a', | ||
198 | 166 | tree=Tree({ | ||
199 | 167 | 'a': Blob(b'a'), | ||
200 | 168 | }), | ||
201 | 169 | ), | ||
202 | 170 | Commit( | ||
203 | 171 | name='2', | ||
204 | 172 | message='''First line | ||
205 | 173 | |||
206 | 174 | Inline patch that isn't part of the real patch starts here | ||
207 | 175 | |||
208 | 176 | --- a/foo | ||
209 | 177 | +++ b/foo | ||
210 | 178 | @@ -1 +1,2 @@ | ||
211 | 179 | foo | ||
212 | 180 | +bar | ||
213 | 181 | ''', | ||
214 | 182 | tree=Tree({ | ||
215 | 183 | 'a': Blob(b'ab'), | ||
216 | 184 | }), | ||
217 | 185 | parents=[Placeholder('1')], | ||
218 | 186 | ), | ||
219 | 187 | ], | ||
220 | 188 | tags={ | ||
221 | 189 | 'importer/import/1': Placeholder('1'), | ||
222 | 190 | 'importer/upload/2': Placeholder('2'), | ||
223 | 191 | } | ||
224 | 192 | ).write(repo.raw_repo) | ||
225 | 193 | target.export_all(repo, str(tmpdir)) | ||
226 | 194 | repo.raw_repo.lookup_reference('refs/tags/importer/upload/2').delete() | ||
227 | 195 | |||
228 | 196 | # We already test in test_rich_history_preservation() that the import works | ||
229 | 197 | # correctly; this test solely checks that the following call does not raise | ||
230 | 198 | # an exception, so no further assertion is required. | ||
231 | 199 | target.import_single(repo, tmpdir, '2') | ||
232 | diff --git a/gitubuntu/test_fixtures.py b/gitubuntu/test_fixtures.py | |||
233 | index bbeb8f6..60a5457 100644 | |||
234 | --- a/gitubuntu/test_fixtures.py | |||
235 | +++ b/gitubuntu/test_fixtures.py | |||
236 | @@ -35,26 +35,6 @@ def repo(): | |||
237 | 35 | 35 | ||
238 | 36 | 36 | ||
239 | 37 | @pytest.fixture | 37 | @pytest.fixture |
240 | 38 | def repo_factory(): | ||
241 | 39 | """A fixture that generates a series of repo fixtures | ||
242 | 40 | |||
243 | 41 | This can be used by tests that require multiple independent repo fixtures. | ||
244 | 42 | """ | ||
245 | 43 | |||
246 | 44 | # See https://stackoverflow.com/q/21583833/478206 | ||
247 | 45 | with contextlib.ExitStack() as stack: | ||
248 | 46 | class Factory: | ||
249 | 47 | def __iter__(self): | ||
250 | 48 | return self | ||
251 | 49 | |||
252 | 50 | def __next__(self): | ||
253 | 51 | fixture_generator = repo() | ||
254 | 52 | stack.callback(fixture_generator.close) | ||
255 | 53 | return next(fixture_generator) | ||
256 | 54 | yield Factory() | ||
257 | 55 | |||
258 | 56 | |||
259 | 57 | @pytest.fixture | ||
260 | 58 | def pygit2_repo(): | 38 | def pygit2_repo(): |
261 | 59 | """An empty pygit2 repository fixture in a temporary directory""" | 39 | """An empty pygit2 repository fixture in a temporary directory""" |
262 | 60 | with tempfile.TemporaryDirectory() as tmp_dir: | 40 | with tempfile.TemporaryDirectory() as tmp_dir: |
PASSED: Continuous integration, rev:e30452d7304 5c9a44dae172dae 0029d79bbd58e8 /jenkins. ubuntu. com/server/ job/git- ubuntu- ci/512/
https:/
Executed test runs:
SUCCESS: VM Setup
SUCCESS: Build
SUCCESS: Unit Tests
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild: /jenkins. ubuntu. com/server/ job/git- ubuntu- ci/512/ /rebuild
https:/