Merge ~racb/git-ubuntu:changelog-parents into git-ubuntu:master
- Git
- lp:~racb/git-ubuntu
- changelog-parents
- Merge into master
Status: | Merged |
---|---|
Merged at revision: | fd197c345d0230cb721e62f62dbf01f0569a4003 |
Proposed branch: | ~racb/git-ubuntu:changelog-parents |
Merge into: | git-ubuntu:master |
Diff against target: |
972 lines (+357/-156) 2 files modified
gitubuntu/importer.py (+155/-106) gitubuntu/importer_test.py (+202/-50) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Bryce Harrington | Approve | ||
Server Team CI bot | continuous-integration | Approve | |
Review via email: mp+378744@code.launchpad.net |
Commit message
Make Jenkins happy
Description of the change
Server Team CI bot (server-team-bot) wrote : | # |
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:7e2fc4a379c
https:/
Executed test runs:
SUCCESS: VM Setup
SUCCESS: Build
SUCCESS: Unit Tests
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
Bryce Harrington (bryce) wrote : | # |
I have a few very minor suggestions, but looks ok overall.
While this is a big patch, most of the code changes were straightforward to review; there were a few pytest mechanisms I wasn't familiar with, and there were a couple chunks of code that took me some time to untangle, but in the end I didn't spot any logical errors.
Since it's nicely broken up into conceptually distinct patches, I reviewed the code sequentially by patch, so am giving feedback thusly organized, hopefully it'll be clear what lines I'm referring to.
1. importer: drop unused variable
√ LGTM. Simple cleanup
2. Add test: test_get_
- 'repo' parameter could use a doc, e.g.:
":param gitubuntu.
- Add comment to explain "repo_builder.
"# Construct a synthetic git repository containing tags"
- Rename 'mock' variable to 'mock_repo'
3. Sort reimport tags before using them
√ LGTM. Interesting illustration of splitting the test from the fix via xfail.
4. Allow for multiple changelog parents
√ Previously the code checked that changelog_
now it checks len(changelog_
works fine, the one caveat being that this now implies an assumption
that changelog_
in then things will break. I did not spot any places where this is
likely to happen, though.
- Given that changelog_
there is a check around line 607 in _commit_import() that is probably
unnecessary. It used to be a None check, and now is effectively a
len()>0 check, however l.extend([]) will be a no-op so no need for a check.
5. test_get_
√ LGTM. I wondered if this could be done as an xfail test case, but it's fine as is.
6. Generalise test_get_
- Param docs for test_get_
√ Otherwise, LGTM
7. Add multiple changelog parent tests
√ LGTM
8. Support multiple changelog parents
√ The list comprehension for calculating changelog_
dense pythonically but on further review it's just using idioms already
common in importer.py.
As mentioned in #6, some of the test cases in importer_test.py are documented but not all; would be nice to ensure each of the 3 or 4 test cases added or changed in this patchset also gain docs while we're at it.
Robie Basak (racb) wrote : | # |
> 2. Add test: test_get_
> - 'repo' parameter could use a doc, e.g.:
> ":param gitubuntu.
> repository to use."
Done, but note that importantly it's a fixture, so I noted that also.
> - Add comment to explain "repo_builder.
> "# Construct a synthetic git repository containing tags"
Done.
> - Rename 'mock' variable to 'mock_repo'
Actually it's not a mock Repo; it's a mocked method on the Repo class.
But I take your point, so I called it mock_get_
this is quite long, it busted the line length limit so I also had to
introduce mock_get_
too.
> 4. Allow for multiple changelog parents
> √ Previously the code checked that changelog_
> None, but now it checks len(changelog_
> Logically this works fine, the one caveat being that this now
> implies an assumption that changelog_
> list; if None ever got passed in then things will break. I did
> not spot any places where this is likely to happen, though.
Yes - that's the intention. changelog_
now, and we use an empty list instead of None now, in all cases, to
represent that there aren't any.
> - Given that changelog_
> there is a check around line 607 in _commit_import() that is probably
> unnecessary. It used to be a None check, and now is effectively a
> len()>0 check, however l.extend([]) will be a no-op so no need
> for a check.
Good point. Check removed.
> 5. test_get_
> √ LGTM. I wondered if this could be done as an xfail test case,
> but it's fine as is.
This was a case that the code was already correct but it lacked a test,
so the xfail process didn't apply. I'm not sure if you already said
that.
> 6. Generalise test_get_
> - Param docs for test_get_
Done.
> √ Otherwise, LGTM
> As mentioned in #6, some of the test cases in importer_test.py are
> documented but not all; would be nice to ensure each of the 3 or 4
> test cases added or changed in this patchset also gain docs while
> we're at it.
Done. I've ensured that every test case touched has a docstring. I did
this in a new commit at the end of the patchset. Some of this new
documentation exposes some shortcomings in the implementation, but in
the interest of making progress I documented the code as-is, leaving
improvements to the code for the future.
Robie Basak (racb) wrote : | # |
Here's the range-diff for your reference: http://
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:fd197c345d0
https:/
Executed test runs:
SUCCESS: VM Setup
SUCCESS: Build
SUCCESS: Unit Tests
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
Bryce Harrington (bryce) wrote : | # |
Thanks for the range-diff, all looks good, +1.
Preview Diff
1 | diff --git a/gitubuntu/importer.py b/gitubuntu/importer.py |
2 | index 98a0997..495a22d 100644 |
3 | --- a/gitubuntu/importer.py |
4 | +++ b/gitubuntu/importer.py |
5 | @@ -488,15 +488,19 @@ def main( |
6 | def get_changelog_for_commit( |
7 | repo, |
8 | tree_hash, |
9 | - changelog_parent_commit, |
10 | + changelog_parent_commits, |
11 | ): |
12 | """Extract changes to debian/changelog in this publish |
13 | |
14 | LP: #1633114 -- favor the changelog parent's diff |
15 | """ |
16 | raw_clog_entry = b'' |
17 | - if changelog_parent_commit is not None: |
18 | - cmd = ['diff-tree', '-p', changelog_parent_commit, |
19 | + # If there is more than one changelog parent then don't attempt to combine |
20 | + # their entries, and instead return nothing (raw_clog_entry will remain |
21 | + # b''). This can be addressed later when the specification better defines |
22 | + # what the commit message should say in this case, but will do for now. |
23 | + if len(changelog_parent_commits) == 1: |
24 | + cmd = ['diff-tree', '-p', changelog_parent_commits[0], |
25 | tree_hash, '--', 'debian/changelog'] |
26 | raw_clog_entry, _ = repo.git_run(cmd, decode=False) |
27 | |
28 | @@ -529,10 +533,32 @@ def get_import_commit_msg( |
29 | target_head_name, |
30 | namespace, |
31 | tree_hash, |
32 | - changelog_parent_commit, |
33 | + changelog_parent_commits, |
34 | upload_parent_commit, |
35 | unapplied_parent_commit, |
36 | ): |
37 | + """Compose an appropriate commit message |
38 | + |
39 | + When the importer synthesizes a commit, it needs to provide an appropriate |
40 | + commit message. This function centralizes the generation of that message |
41 | + based on the parameters given. |
42 | + |
43 | + :param GitUbuntuRepository repo: the repository in which the tree being |
44 | + committed can be found |
45 | + :param str version: the package version string being committed |
46 | + :param str target_head_name: the nominal name of the branch to which the |
47 | + commit is being made |
48 | + :param str namespace: the namespace name to prefix to the nominal branch |
49 | + name to determine the final branch name |
50 | + :param str tree_hash: the hash of the tree object being committed |
51 | + :param list(str) changelog_parent_commits: any changelog parents to use in |
52 | + the commit |
53 | + :param str upload_parent_commit: the upload parent to use in the commit, or |
54 | + None if there isn't one |
55 | + :param str unapplied_parent_commit: the unapplied parent to use in the |
56 | + commit, or None if this is an applied branch commit |
57 | + |
58 | + """ |
59 | if unapplied_parent_commit: |
60 | import_type = 'patches-applied' |
61 | else: |
62 | @@ -543,7 +569,7 @@ def get_import_commit_msg( |
63 | changelog_entry = get_changelog_for_commit( |
64 | repo, |
65 | tree_hash, |
66 | - changelog_parent_commit |
67 | + changelog_parent_commits |
68 | ) |
69 | |
70 | msg = ( |
71 | @@ -554,10 +580,10 @@ def get_import_commit_msg( |
72 | ) |
73 | ) |
74 | |
75 | - if any([changelog_parent_commit, upload_parent_commit, unapplied_parent_commit]): |
76 | + if any([changelog_parent_commits, upload_parent_commit, unapplied_parent_commit]): |
77 | msg += b'\n' |
78 | |
79 | - if changelog_parent_commit is not None: |
80 | + for changelog_parent_commit in changelog_parent_commits: |
81 | msg += b'\nChangelog parent: %b' % changelog_parent_commit.encode() |
82 | if upload_parent_commit is not None: |
83 | msg += b'\nUpload parent: %b' % upload_parent_commit.encode() |
84 | @@ -574,7 +600,7 @@ def _commit_import( |
85 | target_head_name, |
86 | tree_hash, |
87 | namespace, |
88 | - changelog_parent_commit, |
89 | + changelog_parent_commits, |
90 | upload_parent_commit, |
91 | unapplied_parent_commit, |
92 | commit_date, |
93 | @@ -594,8 +620,8 @@ def _commit_import( |
94 | :param target_head_name str Git branch name to import to |
95 | :param namespace str Namespace under which relevant refs can be |
96 | found. |
97 | - :param changelog_parent_commit str Git commit hash of |
98 | - changelog parent. |
99 | + :param list(str) changelog_parent_commits Git commit hashes of changelog |
100 | + parents. |
101 | :param upload_parent_commit str Git commit hash of |
102 | upload parent. Should be None if patches-applied import. |
103 | :param unapplied_parent_commit str Git commit hash of unapplied |
104 | @@ -605,16 +631,12 @@ def _commit_import( |
105 | :rtype str |
106 | :returns Hash of created commit |
107 | """ |
108 | - changelog_entry = get_changelog_for_commit( |
109 | - repo, |
110 | - tree_hash, |
111 | - changelog_parent_commit |
112 | - ) |
113 | - |
114 | parents = [] |
115 | |
116 | - if changelog_parent_commit is not None: |
117 | - parents.append(pygit2.Oid(hex=changelog_parent_commit)) |
118 | + parents.extend( |
119 | + pygit2.Oid(hex=changelog_parent_commit) |
120 | + for changelog_parent_commit in changelog_parent_commits |
121 | + ) |
122 | if upload_parent_commit is not None: |
123 | parents.append(pygit2.Oid(hex=upload_parent_commit)) |
124 | if unapplied_parent_commit is not None: |
125 | @@ -629,7 +651,7 @@ def _commit_import( |
126 | target_head_name=target_head_name, |
127 | namespace=namespace, |
128 | tree_hash=tree_hash, |
129 | - changelog_parent_commit=changelog_parent_commit, |
130 | + changelog_parent_commits=changelog_parent_commits, |
131 | upload_parent_commit=upload_parent_commit, |
132 | unapplied_parent_commit=unapplied_parent_commit, |
133 | ), |
134 | @@ -644,7 +666,7 @@ def commit_unapplied_patches_import( |
135 | head_name, |
136 | import_tree_hash, |
137 | namespace, |
138 | - changelog_parent_commit, |
139 | + changelog_parent_commits, |
140 | upload_parent_commit, |
141 | commit_date, |
142 | ): |
143 | @@ -654,7 +676,7 @@ def commit_unapplied_patches_import( |
144 | head_name, |
145 | import_tree_hash, |
146 | namespace, |
147 | - changelog_parent_commit, |
148 | + changelog_parent_commits, |
149 | upload_parent_commit, |
150 | # unapplied trees do not have a distinct unapplied parent |
151 | None, |
152 | @@ -667,7 +689,7 @@ def commit_applied_patches_import( |
153 | head_name, |
154 | import_tree_hash, |
155 | namespace, |
156 | - changelog_parent_commit, |
157 | + changelog_parent_commits, |
158 | unapplied_parent_commit, |
159 | commit_date, |
160 | ): |
161 | @@ -677,7 +699,7 @@ def commit_applied_patches_import( |
162 | head_name, |
163 | import_tree_hash, |
164 | namespace, |
165 | - changelog_parent_commit, |
166 | + changelog_parent_commits, |
167 | # uploads will be unapplied trees currently, so applied trees |
168 | # can not have them as direct parents |
169 | None, |
170 | @@ -985,16 +1007,35 @@ def get_existing_import_tags( |
171 | namespace, |
172 | patch_state=PatchState.UNAPPLIED, |
173 | ): |
174 | + """Find and return any import tags that already exist for the given version |
175 | + |
176 | + :param GitUbuntuRepository repo: repository to search for import tags |
177 | + :param str version: the package version string to find |
178 | + :param str namespace: the name to prefix to the import tag names searched |
179 | + :param PatchState patch_state: whether to look for unapplied or applied |
180 | + tags |
181 | + :rtype: list(pygit2.Reference) |
182 | + :returns: a list of import tags found. If reimport tags exist, this is the |
183 | + list of matching reimport tags in order; the original import tag is not |
184 | + included as it is already represented by the first reimport tag. If |
185 | + reimport tags do not exist, just the sole import tag is returned. If |
186 | + none of these are found, the empty list is returned. |
187 | + """ |
188 | existing_import_tag = repo.get_import_tag(version, namespace, patch_state) |
189 | if existing_import_tag: |
190 | - # check if there are reimports |
191 | + # check if there are reimports; if no import tag exists, then by the |
192 | + # specification there can be no reimports |
193 | existing_reimport_tags = repo.get_all_reimport_tags( |
194 | version, |
195 | namespace, |
196 | patch_state, |
197 | ) |
198 | if existing_reimport_tags: |
199 | - return existing_reimport_tags |
200 | + return sorted( |
201 | + existing_reimport_tags, |
202 | + # Sort on the integer of the last '/'-separated component |
203 | + key=lambda tag: int(tag.name.split('/')[-1]), |
204 | + ) |
205 | else: |
206 | return [existing_import_tag] |
207 | return [] |
208 | @@ -1002,10 +1043,10 @@ def get_existing_import_tags( |
209 | |
210 | def override_parents(parent_overrides, repo, version, namespace): |
211 | """ |
212 | - returns: (unapplied, applied) where both elements of the tuple are commit |
213 | - hash strings of the changelog parent to use, incorporating any defined |
214 | - overrides. |
215 | - rtype: tuple(str, str) |
216 | + returns: (unapplied, applied) where both elements of the tuple are lists of |
217 | + commit hash strings of the changelog parents to use, incorporating any |
218 | + defined overrides. |
219 | + rtype: tuple(list(str), list(str)) |
220 | """ |
221 | unapplied_changelog_parent_commit = None |
222 | applied_changelog_parent_commit = None |
223 | @@ -1057,8 +1098,8 @@ def override_parents(parent_overrides, repo, version, namespace): |
224 | ) |
225 | ) |
226 | return ( |
227 | - unapplied_changelog_parent_commit, |
228 | - applied_changelog_parent_commit, |
229 | + [unapplied_changelog_parent_commit], |
230 | + [applied_changelog_parent_commit], |
231 | ) |
232 | |
233 | def _devel_branch_updates( |
234 | @@ -1296,72 +1337,83 @@ def create_import_tag(repo, commit_hash, version, namespace): |
235 | return repo.get_import_tag(version, namespace) |
236 | |
237 | |
238 | -def get_changelog_parent_commit( |
239 | +def get_changelog_parent_commits( |
240 | repo, |
241 | namespace, |
242 | parent_overrides, |
243 | import_tree_versions, |
244 | - patches_applied, |
245 | + patch_state, |
246 | ): |
247 | - """Walk changelog backwards until we find an imported version, |
248 | - skipping the current version |
249 | - |
250 | - :param repo gitubuntu.git_repository.GitUbuntuRepository The |
251 | - repository to use |
252 | - :param parent_overrides dict See parse_parentfile |
253 | - :param import_tree_versions list(str) List of versions to iterate. |
254 | - :param patches_applied bool If true, look for patches-applied |
255 | - imports |
256 | - |
257 | - :rtype str |
258 | - :returns Git commit hash of the nearest (chronologically) imported |
259 | - changelog version |
260 | + """Given a changelog history, find the changelog parent commits |
261 | + |
262 | + "Changelog parents" are defined in the import specification, which refers |
263 | + to source package data instances. Since this importer runs incrementally in |
264 | + Launchpad publication order, the commits associated with the source package |
265 | + data instances can be located by traversing the matching reimport tags in |
266 | + order. If there are no reimport tags, the sole import tag (if it exists) |
267 | + will match the sole corresponding source package data instance. Otherwise |
268 | + we find no changelog parent and return an empty list. |
269 | + |
270 | + This function performs this search and returns a list of commit hashes that |
271 | + are the changelog parents. |
272 | + |
273 | + :param gitubuntu.git_repository.GitUbuntuRepository repo: the repository to |
274 | + use |
275 | + :param dict parent_overrides: see parse_parentfile |
276 | + :param list(str) import_tree_versions: list of package version strings as |
277 | + parsed from debian/changelog in the same order |
278 | + :param PatchState patch_state: whether to look for previous unapplied or |
279 | + applied imports |
280 | + |
281 | + :rtype: list(str) |
282 | + :returns: a list of commit hashes of any existing imports of the changelog |
283 | + parents as defined by the import specification |
284 | """ |
285 | # skip current version |
286 | import_tree_versions = import_tree_versions[1:] |
287 | |
288 | for version in import_tree_versions: |
289 | - if patches_applied: |
290 | - changelog_parent_tag = repo.get_import_tag( |
291 | - version, |
292 | - namespace, |
293 | - PatchState.APPLIED, |
294 | - ) |
295 | - else: |
296 | - changelog_parent_tag = repo.get_import_tag( |
297 | - version, |
298 | - namespace, |
299 | - PatchState.UNAPPLIED, |
300 | - ) or repo.get_orphan_tag( |
301 | - version, |
302 | - namespace, |
303 | - ) |
304 | + changelog_parent_tags = get_existing_import_tags( |
305 | + repo, |
306 | + version, |
307 | + namespace, |
308 | + patch_state, |
309 | + ) |
310 | |
311 | - if not changelog_parent_tag: |
312 | + if not changelog_parent_tags: |
313 | continue |
314 | |
315 | # sanity check that version from d/changelog of the |
316 | # tagged parent matches ours |
317 | - changelog_parent_version, _ = repo.get_changelog_versions_from_treeish( |
318 | - str(changelog_parent_tag.peel(pygit2.Tree).id) |
319 | - ) |
320 | + changelog_parent_versions = [ |
321 | + repo.get_changelog_versions_from_treeish( |
322 | + str(tag.peel(pygit2.Tree).id) |
323 | + )[0] for tag in changelog_parent_tags |
324 | + ] |
325 | |
326 | # if the parent was imported via a parent override |
327 | # itself, assume it is valid without checking its |
328 | # tree's debian/changelog, as it wasn't used |
329 | if (version in parent_overrides or |
330 | - changelog_parent_version == version |
331 | - ): |
332 | - changelog_parent_commit = str( |
333 | - changelog_parent_tag.peel(pygit2.Commit).id |
334 | + any( |
335 | + [changelog_parent_version == version |
336 | + for changelog_parent_version in changelog_parent_versions] |
337 | ) |
338 | - |
339 | - logging.debug("Changelog parent (tag) is %s", |
340 | - repo.tag_to_pretty_name(changelog_parent_tag) |
341 | + ): |
342 | + changelog_parent_commits = [ |
343 | + str(tag.peel(pygit2.Commit).id) |
344 | + for tag in changelog_parent_tags |
345 | + ] |
346 | + |
347 | + logging.debug("Changelog parent(s) (tag(s)) are %s", |
348 | + [ |
349 | + repo.tag_to_pretty_name(tag) |
350 | + for tag in changelog_parent_tags |
351 | + ] |
352 | ) |
353 | - return changelog_parent_commit |
354 | + return changelog_parent_commits |
355 | |
356 | - return None |
357 | + return [] |
358 | |
359 | |
360 | def import_unapplied_dsc( |
361 | @@ -1463,7 +1515,6 @@ def import_unapplied_dsc( |
362 | |
363 | logging.debug('Tip version is %s', unapplied_tip_version) |
364 | |
365 | - unapplied_changelog_parent_commit = None |
366 | upload_parent_commit = None |
367 | |
368 | if version in parent_overrides: |
369 | @@ -1474,7 +1525,7 @@ def import_unapplied_dsc( |
370 | |
371 | try: |
372 | ( |
373 | - unapplied_changelog_parent_commit, |
374 | + unapplied_changelog_parent_commits, |
375 | _ |
376 | ) = override_parents( |
377 | parent_overrides, |
378 | @@ -1494,12 +1545,12 @@ def import_unapplied_dsc( |
379 | unapplied_tip_version, |
380 | ) |
381 | |
382 | - unapplied_changelog_parent_commit = get_changelog_parent_commit( |
383 | + unapplied_changelog_parent_commits = get_changelog_parent_commits( |
384 | repo, |
385 | namespace, |
386 | parent_overrides, |
387 | import_tree_versions, |
388 | - patches_applied=False, |
389 | + patch_state=PatchState.UNAPPLIED, |
390 | ) |
391 | |
392 | # check if the version to import has already been uploaded to |
393 | @@ -1515,7 +1566,7 @@ def import_unapplied_dsc( |
394 | "does not match the published version. Will orphan commit.", |
395 | repo.tag_to_pretty_name(existing_import_tag), |
396 | ) |
397 | - unapplied_changelog_parent_commit = None |
398 | + unapplied_changelog_parent_commits = [] |
399 | else: |
400 | repo.update_head_to_commit( |
401 | head_name, |
402 | @@ -1549,31 +1600,31 @@ def import_unapplied_dsc( |
403 | else: |
404 | upload_parent_commit = str(existing_upload_tag.peel().id) |
405 | |
406 | - if unapplied_changelog_parent_commit is not None: |
407 | + if unapplied_changelog_parent_commits: |
408 | try: |
409 | repo.git_run( |
410 | [ |
411 | 'merge-base', |
412 | '--is-ancestor', |
413 | - unapplied_changelog_parent_commit, |
414 | + unapplied_changelog_parent_commits[0], |
415 | upload_parent_commit, |
416 | ], |
417 | verbose_on_failure=False, |
418 | ) |
419 | - unapplied_changelog_parent_commit = None |
420 | + unapplied_changelog_parent_commits = [] |
421 | except CalledProcessError as e: |
422 | if e.returncode != 1: |
423 | raise |
424 | |
425 | commit_hash = commit_unapplied_patches_import( |
426 | - repo, |
427 | - version, |
428 | - head_name, |
429 | - unapplied_import_tree_hash, |
430 | - namespace, |
431 | - unapplied_changelog_parent_commit, |
432 | - upload_parent_commit, |
433 | - commit_date, |
434 | + repo=repo, |
435 | + version=version, |
436 | + head_name=head_name, |
437 | + import_tree_hash=unapplied_import_tree_hash, |
438 | + namespace=namespace, |
439 | + changelog_parent_commits=unapplied_changelog_parent_commits, |
440 | + upload_parent_commit=upload_parent_commit, |
441 | + commit_date=commit_date, |
442 | ) |
443 | logging.debug( |
444 | "Committed patches-unapplied import of %s as %s in %s", |
445 | @@ -1587,7 +1638,7 @@ def import_unapplied_dsc( |
446 | # Not imported before |
447 | tag = import_tag(version, namespace) |
448 | elif ( |
449 | - unapplied_changelog_parent_commit is None and |
450 | + not unapplied_changelog_parent_commits and |
451 | upload_parent_commit is None and |
452 | head_name in repo.local_branch_names |
453 | ): |
454 | @@ -1726,8 +1777,6 @@ def import_applied_dsc( |
455 | |
456 | logging.debug('Tip version is %s', applied_tip_version) |
457 | |
458 | - applied_changelog_parent_commit = None |
459 | - |
460 | if version in parent_overrides: |
461 | logging.debug('%s is specified in the parent override file.' % |
462 | version |
463 | @@ -1735,7 +1784,7 @@ def import_applied_dsc( |
464 | |
465 | ( |
466 | _, |
467 | - applied_changelog_parent_commit, |
468 | + applied_changelog_parent_commits, |
469 | ) = override_parents( |
470 | parent_overrides, |
471 | repo, |
472 | @@ -1749,12 +1798,12 @@ def import_applied_dsc( |
473 | ) |
474 | |
475 | |
476 | - applied_changelog_parent_commit = get_changelog_parent_commit( |
477 | + applied_changelog_parent_commits = get_changelog_parent_commits( |
478 | repo, |
479 | namespace, |
480 | parent_overrides, |
481 | import_tree_versions, |
482 | - patches_applied=True, |
483 | + patch_state=PatchState.APPLIED, |
484 | ) |
485 | |
486 | existing_applied_tag = repo.get_import_tag( |
487 | @@ -1774,7 +1823,7 @@ def import_applied_dsc( |
488 | # "version. Will orphan commit.", |
489 | # repo.tag_to_pretty_name(applied_tag), |
490 | # ) |
491 | - # applied_changelog_parent_commit = None |
492 | + # applied_changelog_parent_commits = [] |
493 | #else: |
494 | repo.update_head_to_commit( |
495 | head_name, |
496 | @@ -1822,14 +1871,14 @@ def import_applied_dsc( |
497 | raise |
498 | |
499 | commit_hash = commit_applied_patches_import( |
500 | - repo, |
501 | - version, |
502 | - head_name, |
503 | - applied_import_tree_hash, |
504 | - namespace, |
505 | - applied_changelog_parent_commit, |
506 | - str(unapplied_parent_commit), |
507 | - commit_date, |
508 | + repo=repo, |
509 | + version=version, |
510 | + head_name=head_name, |
511 | + import_tree_hash=applied_import_tree_hash, |
512 | + namespace=namespace, |
513 | + changelog_parent_commits=applied_changelog_parent_commits, |
514 | + unapplied_parent_commit=str(unapplied_parent_commit), |
515 | + commit_date=commit_date, |
516 | ) |
517 | logging.debug( |
518 | "Committed patches-applied import of %s as %s in %s", |
519 | @@ -1843,7 +1892,7 @@ def import_applied_dsc( |
520 | # Not imported before |
521 | tag = import_tag(version, namespace, PatchState.APPLIED) |
522 | elif ( |
523 | - applied_changelog_parent_commit is None and |
524 | + not applied_changelog_parent_commits and |
525 | unapplied_parent_commit is None and |
526 | head_name in repo.local_branch_names |
527 | ): |
528 | diff --git a/gitubuntu/importer_test.py b/gitubuntu/importer_test.py |
529 | index b1dcdc3..95744d9 100644 |
530 | --- a/gitubuntu/importer_test.py |
531 | +++ b/gitubuntu/importer_test.py |
532 | @@ -73,39 +73,46 @@ def test_get_import_tag_msg(): |
533 | |
534 | |
535 | @pytest.mark.parametrize( |
536 | - 'head_name_value, changelog_parent_commit, upload_parent_commit, unapplied_parent_commit, expected', |
537 | + 'head_name_value, changelog_parent_commits, upload_parent_commit, unapplied_parent_commit, expected', |
538 | [ |
539 | ( |
540 | 'importer/ubuntu/trusty', |
541 | - None, |
542 | + [], |
543 | None, |
544 | None, |
545 | b'Import patches-unapplied version 1-1 to ubuntu/trusty\n\nImported using git-ubuntu import.', |
546 | ), |
547 | ( |
548 | 'importer/ubuntu/trusty', |
549 | - '123456', |
550 | + ['123456'], |
551 | None, |
552 | None, |
553 | b'Import patches-unapplied version 1-1 to ubuntu/trusty\n\nImported using git-ubuntu import.\n\nChangelog parent: 123456', |
554 | ), |
555 | ( |
556 | 'importer/ubuntu/trusty', |
557 | - None, |
558 | + [], |
559 | '123456', |
560 | None, |
561 | b'Import patches-unapplied version 1-1 to ubuntu/trusty\n\nImported using git-ubuntu import.\n\nUpload parent: 123456', |
562 | ), |
563 | ( |
564 | 'importer/ubuntu/trusty', |
565 | + ['123456', '234567'], |
566 | + None, |
567 | None, |
568 | + b'Import patches-unapplied version 1-1 to ubuntu/trusty\n\nImported using git-ubuntu import.\n\nChangelog parent: 123456\nChangelog parent: 234567', |
569 | + ), |
570 | + ( |
571 | + 'importer/ubuntu/trusty', |
572 | + [], |
573 | None, |
574 | '123456', |
575 | b'Import patches-applied version 1-1 to ubuntu/trusty\n\nImported using git-ubuntu import.\n\nUnapplied parent: 123456', |
576 | ), |
577 | ( |
578 | 'importer/ubuntu/trusty', |
579 | - '123456', |
580 | + ['123456'], |
581 | '789123', |
582 | '456789', |
583 | b'Import patches-applied version 1-1 to ubuntu/trusty\n\nImported using git-ubuntu import.\n\nChangelog parent: 123456\nUpload parent: 789123\nUnapplied parent: 456789' |
584 | @@ -115,11 +122,30 @@ def test_get_import_tag_msg(): |
585 | def test_get_import_commit_msg( |
586 | repo, |
587 | head_name_value, |
588 | - changelog_parent_commit, |
589 | + changelog_parent_commits, |
590 | upload_parent_commit, |
591 | unapplied_parent_commit, |
592 | expected, |
593 | ): |
594 | + """Test that get_import_commit_msg is generally correct |
595 | + |
596 | + This is the general parameterised test for the common case uses of |
597 | + target.get_import_commit_msg. |
598 | + |
599 | + This test uses a default source_builder.SourceSpec() to construct a test |
600 | + source tree to provide to get_import_commit_msg, so defaults from |
601 | + SourceSpec() will be used such as a package version string of '1-1'. |
602 | + |
603 | + :param GitUbuntuRepository repo: fixture providing a temporary |
604 | + GitUbuntuRepository instance to use |
605 | + :param str head_name_value: passed through to get_import_commit_msg as |
606 | + target_head_name |
607 | + :param list(str) changelog_parent_commits: passed through to |
608 | + get_import_commit_msg |
609 | + :param str upload_parent_commit: passed through to get_import_commit_msg |
610 | + :param str unapplied_parent_commit: passed through to get_import_commit_msg |
611 | + :param bytes expected: the expected return value from get_import_commit_msg |
612 | + """ |
613 | target.get_changelog_for_commit = Mock() |
614 | target.get_changelog_for_commit.return_value = b'' |
615 | |
616 | @@ -137,22 +163,24 @@ def test_get_import_commit_msg( |
617 | head_name_value, |
618 | 'importer', |
619 | publish_tree_hash, |
620 | - changelog_parent_commit, |
621 | + changelog_parent_commits, |
622 | upload_parent_commit, |
623 | unapplied_parent_commit, |
624 | ) == expected |
625 | |
626 | |
627 | @pytest.mark.parametrize( |
628 | - 'input_repo, expected', [ |
629 | + 'input_repo, patch_state, expected', [ |
630 | ( |
631 | repo_builder.Repo(), |
632 | + PatchState.UNAPPLIED, |
633 | [], |
634 | ), |
635 | ( |
636 | repo_builder.Repo( |
637 | tags={'importer/import/1-1': repo_builder.Commit()}, |
638 | ), |
639 | + PatchState.UNAPPLIED, |
640 | ['refs/tags/importer/import/1-1'], |
641 | ), |
642 | ( |
643 | @@ -167,32 +195,22 @@ def test_get_import_commit_msg( |
644 | 'importer/reimport/import/1-1/1': repo_builder.Commit(), |
645 | }, |
646 | ), |
647 | + PatchState.UNAPPLIED, |
648 | [ |
649 | 'refs/tags/importer/reimport/import/1-1/0', |
650 | 'refs/tags/importer/reimport/import/1-1/1', |
651 | ], |
652 | ), |
653 | - ], |
654 | -) |
655 | -def test_get_existing_import_tags(repo, input_repo, expected): |
656 | - input_repo.write(repo.raw_repo) |
657 | - |
658 | - assert [ |
659 | - ref.name for ref in |
660 | - target.get_existing_import_tags(repo, '1-1', 'importer') |
661 | - ] == expected |
662 | - |
663 | - |
664 | -@pytest.mark.parametrize( |
665 | - 'input_repo, expected', [ |
666 | ( |
667 | repo_builder.Repo(), |
668 | + PatchState.APPLIED, |
669 | [], |
670 | ), |
671 | ( |
672 | repo_builder.Repo( |
673 | tags={'importer/applied/1-1': repo_builder.Commit()}, |
674 | ), |
675 | + PatchState.APPLIED, |
676 | ['refs/tags/importer/applied/1-1'], |
677 | ), |
678 | ( |
679 | @@ -207,6 +225,7 @@ def test_get_existing_import_tags(repo, input_repo, expected): |
680 | 'importer/reimport/applied/1-1/1': repo_builder.Commit(), |
681 | }, |
682 | ), |
683 | + PatchState.APPLIED, |
684 | [ |
685 | 'refs/tags/importer/reimport/applied/1-1/0', |
686 | 'refs/tags/importer/reimport/applied/1-1/1', |
687 | @@ -214,18 +233,78 @@ def test_get_existing_import_tags(repo, input_repo, expected): |
688 | ), |
689 | ], |
690 | ) |
691 | -def test_get_existing_applied_tags(repo, input_repo, expected): |
692 | +def test_get_existing_import_tags(repo, patch_state, input_repo, expected): |
693 | + """Test that get_existing_import_tags is generally correct |
694 | + |
695 | + This is the general parameterised test for the common case uses of |
696 | + target.get_existing_import_tags. |
697 | + |
698 | + :param repo gitubuntu.git_repository.GitUbuntuRepository: fixture to hold a |
699 | + temporary output repository |
700 | + :param PatchState patch_state: passed through to get_existing_import_tags |
701 | + :param repo_builder.Repo input_repo: input repository data |
702 | + :param list(str) expected: the names of the references that are expected to |
703 | + be returned, in order. |
704 | + """ |
705 | input_repo.write(repo.raw_repo) |
706 | |
707 | assert [ |
708 | ref.name for ref in |
709 | - target.get_existing_import_tags( |
710 | - repo, |
711 | + target.get_existing_import_tags(repo, '1-1', 'importer', patch_state) |
712 | + ] == expected |
713 | + |
714 | + |
715 | +def test_get_existing_import_tags_ordering(repo): |
716 | + """Test that get_existing_import_tags returns results in the correct order |
717 | + |
718 | + To maintain hash stability, the spec defines that multiple changelog |
719 | + parents must appear in the order that they were published. For this to |
720 | + work, get_existing_import_tags must return the tags in the correct order |
721 | + even if the underlying git repository tags appear in an arbitrary order. |
722 | + |
723 | + :param GitUbuntuRepository repo: fixture of a temporary repository to use |
724 | + """ |
725 | + |
726 | + # Construct a synthetic git repository containing tags |
727 | + repo_builder.Repo( |
728 | + tags={ |
729 | + 'importer/import/1-1': repo_builder.Commit(), |
730 | + 'importer/reimport/import/1-1/0': repo_builder.Commit(), |
731 | + 'importer/reimport/import/1-1/1': repo_builder.Commit(), |
732 | + } |
733 | + ).write(repo.raw_repo) |
734 | + |
735 | + mock_get_all_reimport_tags_patch = patch.object( |
736 | + repo, |
737 | + 'get_all_reimport_tags', |
738 | + autospec=True, |
739 | + ) |
740 | + with mock_get_all_reimport_tags_patch as mock_get_all_reimport_tags: |
741 | + # Intentionally arrange for repo.get_all_reimport_tags to return the |
742 | + # expected result but in reverse order |
743 | + mock_get_all_reimport_tags.return_value = [ |
744 | + repo.raw_repo.lookup_reference('refs/tags/%s' % name) |
745 | + for name in [ |
746 | + 'importer/reimport/import/1-1/1', |
747 | + 'importer/reimport/import/1-1/0', |
748 | + ] |
749 | + ] |
750 | + |
751 | + # Call function under test |
752 | + tags = target.get_existing_import_tags(repo, '1-1', 'importer') |
753 | + |
754 | + # Check that our mock was called as expected |
755 | + mock_get_all_reimport_tags.assert_called_once_with( |
756 | '1-1', |
757 | 'importer', |
758 | - PatchState.APPLIED, |
759 | + PatchState.UNAPPLIED, |
760 | ) |
761 | - ] == expected |
762 | + |
763 | + # Result tags should be in numeric order |
764 | + assert [ref.name for ref in tags] == [ |
765 | + 'refs/tags/importer/reimport/import/1-1/0', |
766 | + 'refs/tags/importer/reimport/import/1-1/1', |
767 | + ] |
768 | |
769 | |
770 | @pytest.mark.parametrize( |
771 | @@ -442,16 +521,16 @@ def test_create_applied_tag( |
772 | 'input_repo', |
773 | 'parent_overrides', |
774 | 'changelog_versions', |
775 | - 'patches_applied', |
776 | - 'expected_ref', |
777 | + 'patch_state', |
778 | + 'expected_refs', |
779 | ], |
780 | [ |
781 | ( |
782 | repo_builder.Repo(), |
783 | {}, |
784 | ['1-2', '1-1',], |
785 | - False, |
786 | - None, |
787 | + PatchState.UNAPPLIED, |
788 | + [], |
789 | ), |
790 | ( |
791 | repo_builder.Repo( |
792 | @@ -460,8 +539,28 @@ def test_create_applied_tag( |
793 | ), |
794 | {}, |
795 | ['1-2', '1-1'], |
796 | - False, |
797 | - 'refs/tags/importer/import/1-1', |
798 | + PatchState.UNAPPLIED, |
799 | + ['refs/tags/importer/import/1-1'], |
800 | + ), |
801 | + ( |
802 | + repo_builder.Repo( |
803 | + commits=[ |
804 | + repo_builder.Commit.from_spec(name='import'), |
805 | + repo_builder.Commit.from_spec(name='reimport', mutate=1), |
806 | + ], |
807 | + tags={ |
808 | + 'importer/import/1-1': Placeholder('import'), |
809 | + 'importer/reimport/import/1-1/0': Placeholder('import'), |
810 | + 'importer/reimport/import/1-1/1': Placeholder('reimport'), |
811 | + }, |
812 | + ), |
813 | + {}, |
814 | + ['1-2', '1-1'], |
815 | + PatchState.UNAPPLIED, |
816 | + [ |
817 | + 'refs/tags/importer/reimport/import/1-1/0', |
818 | + 'refs/tags/importer/reimport/import/1-1/1', |
819 | + ], |
820 | ), |
821 | ( |
822 | repo_builder.Repo( |
823 | @@ -470,15 +569,15 @@ def test_create_applied_tag( |
824 | ), |
825 | {}, |
826 | ['1-3', '1-2', '1-1'], |
827 | - False, |
828 | - 'refs/tags/importer/import/1-1', |
829 | + PatchState.UNAPPLIED, |
830 | + ['refs/tags/importer/import/1-1'], |
831 | ), |
832 | ( |
833 | repo_builder.Repo(), |
834 | {}, |
835 | ['1-2', '1-1',], |
836 | - True, |
837 | - None, |
838 | + PatchState.APPLIED, |
839 | + [], |
840 | ), |
841 | ( |
842 | repo_builder.Repo( |
843 | @@ -487,8 +586,28 @@ def test_create_applied_tag( |
844 | ), |
845 | {}, |
846 | ['1-2', '1-1'], |
847 | - True, |
848 | - 'refs/tags/importer/applied/1-1', |
849 | + PatchState.APPLIED, |
850 | + ['refs/tags/importer/applied/1-1'], |
851 | + ), |
852 | + ( |
853 | + repo_builder.Repo( |
854 | + commits=[ |
855 | + repo_builder.Commit.from_spec(name='import'), |
856 | + repo_builder.Commit.from_spec(name='reimport', mutate=1), |
857 | + ], |
858 | + tags={ |
859 | + 'importer/applied/1-1': Placeholder('import'), |
860 | + 'importer/reimport/applied/1-1/0': Placeholder('import'), |
861 | + 'importer/reimport/applied/1-1/1': Placeholder('reimport'), |
862 | + }, |
863 | + ), |
864 | + {}, |
865 | + ['1-2', '1-1'], |
866 | + PatchState.APPLIED, |
867 | + [ |
868 | + 'refs/tags/importer/reimport/applied/1-1/0', |
869 | + 'refs/tags/importer/reimport/applied/1-1/1', |
870 | + ], |
871 | ), |
872 | ( |
873 | repo_builder.Repo( |
874 | @@ -497,30 +616,51 @@ def test_create_applied_tag( |
875 | ), |
876 | {}, |
877 | ['1-3', '1-2', '1-1'], |
878 | - True, |
879 | - 'refs/tags/importer/applied/1-1', |
880 | + PatchState.APPLIED, |
881 | + ['refs/tags/importer/applied/1-1'], |
882 | ), |
883 | ], |
884 | ) |
885 | -def test_get_changelog_parent_commit( |
886 | +def test_get_changelog_parent_commits( |
887 | repo, |
888 | input_repo, |
889 | parent_overrides, |
890 | changelog_versions, |
891 | - patches_applied, |
892 | - expected_ref, |
893 | + patch_state, |
894 | + expected_refs, |
895 | ): |
896 | + """Test that get_changelog_parent_commits is generally correct |
897 | + |
898 | + This is the general parameterised test for the common case uses of |
899 | + target.get_changelog_parent_commits. |
900 | + |
901 | + :param GitUbuntuRepository repo: fixture providing a temporary |
902 | + GitUbuntuRepository instance to use |
903 | + :param repo_builder.Repo input_repo: the input repository data to use that |
904 | + will be populated into @repo before @repo is passed through to |
905 | + get_changelog_parent_commits |
906 | + :param dict parent_overrides: passed through to |
907 | + get_changelog_parent_commits. |
908 | + :param PatchState patch_state: passed through to |
909 | + get_changelog_parent_commits |
910 | + :param list(str) expected_refs: the expected return value of |
911 | + get_changelog_parent_commits expressed using a list of reference names. |
912 | + Since get_changelog_parent_commits returns a list of commit hash |
913 | + strings, the reference names will need to be dereferenced before |
914 | + comparison; this way the test parameters don't need to be opaque hash |
915 | + strings. |
916 | + """ |
917 | input_repo.write(repo.raw_repo) |
918 | - assert target.get_changelog_parent_commit( |
919 | + assert target.get_changelog_parent_commits( |
920 | repo, |
921 | 'importer', |
922 | parent_overrides, |
923 | changelog_versions, |
924 | - patches_applied, |
925 | - ) == ( |
926 | + patch_state, |
927 | + ) == ([ |
928 | str(repo.raw_repo.lookup_reference(expected_ref).peel(pygit2.Commit).id) |
929 | - if expected_ref else expected_ref |
930 | - ) |
931 | + for expected_ref in expected_refs |
932 | + ]) |
933 | |
934 | @patch('gitubuntu.git_repository.GitUbuntuRepository.add_base_remotes') |
935 | def test_importer_main_cleanup_on_exception(add_base_remotes_mock): |
936 | @@ -703,6 +843,19 @@ def test_import_unapplied_spi_quilt_patches( |
937 | ], |
938 | [ |
939 | pytest.param( |
940 | + repo_builder.Repo(), |
941 | + ['1-1'], |
942 | + { |
943 | + 'add_commits': [ |
944 | + repo_builder.Commit.from_spec(name='publish'), |
945 | + ], |
946 | + 'update_tags': {'importer/import/1-1': Placeholder('publish')}, |
947 | + }, |
948 | + [ |
949 | + 'refs/tags/importer/import/1-1', |
950 | + ] |
951 | + ), |
952 | + pytest.param( |
953 | repo_builder.Repo( |
954 | commits=[repo_builder.Commit.from_spec(name='import')], |
955 | tags={'importer/import/1-1': Placeholder('import')}, |
956 | @@ -744,7 +897,7 @@ def test_import_unapplied_spi_quilt_patches( |
957 | 'refs/tags/importer/import/1-3', |
958 | ], |
959 | ), |
960 | - pytest.param( |
961 | + ( |
962 | repo_builder.Repo( |
963 | commits=[ |
964 | repo_builder.Commit.from_spec(name='import'), |
965 | @@ -779,7 +932,6 @@ def test_import_unapplied_spi_quilt_patches( |
966 | 'refs/tags/importer/reimport/import/1-1/1', |
967 | 'refs/tags/importer/import/1-2', |
968 | ], |
969 | - marks=pytest.mark.xfail(reason='LP: #1761332'), |
970 | ), |
971 | ] |
972 | ) |
FAILED: Continuous integration, rev:969c7def726 6a904c1a306d743 503a7083563f5f /jenkins. ubuntu. com/server/ job/git- ubuntu- ci/469/
https:/
Executed test runs:
SUCCESS: VM Setup
SUCCESS: Build
FAILED: Unit Tests
Click here to trigger a rebuild: /jenkins. ubuntu. com/server/ job/git- ubuntu- ci/469/ /rebuild
https:/