Merge ~nacc/usd-importer:further-orphan-tag-fix-lp1753800 into usd-importer:master

Proposed by Nish Aravamudan on 2018-03-06
Status: Superseded
Proposed branch: ~nacc/usd-importer:further-orphan-tag-fix-lp1753800
Merge into: usd-importer:master
Diff against target: 295 lines (+148/-28)
6 files modified
gitubuntu/git_repository.py (+24/-19)
gitubuntu/importer.py (+29/-5)
gitubuntu/importlocal.py (+0/-1)
gitubuntu/importppa.py (+0/-1)
gitubuntu/source_builder.py (+10/-1)
gitubuntu/test_importer.py (+85/-1)
Reviewer Review Type Date Requested Status
Robie Basak 2018-03-06 Pending
Ubuntu Server Dev import team 2018-03-06 Pending
Review via email: mp+340869@code.launchpad.net

This proposal has been superseded by a proposal from 2018-03-06.

Description of the change

Make jenkins happy.

To post a comment you must log in.
Nish Aravamudan (nacc) wrote :

A few thoughts before I forget them:

I think we need some basic import_unapplied_spi testing before these changes land.

Given three (distinctly versioned) launchpad publishes (source packages) A, B, C; Y <- X implies X is a parent of Y in the changelog

1) if the sequence is A, B <- A, C <- B, we should see three import tags, no orphan tags. If we are able to integrate publishing info into our tests, then if all three are to the same series/pocket, we would expect that branch to be at C.

2) if the sequence is A, B, C <- A, we should see two import tags, one orphan tag. Again, if all are to the same series/pocket, branch should be at C.

3) if the sequence is A, A', C <- A', where A' is the same version as A but with different tree, we should see two import tags, one orphan tag and branch at C.

4) if the sequence is A, A', A' to a different series, C <- A', we should see two import tags, one orphan tag (first fix in this branch).

5) if the sequence is A, B, C, but there is a U upload tag for B already in the repo, we should see three import tags, but the B import tag should point at U.

I am sure there are more to consider, but I think this sort of verbiage is what we want to encapsulate in the import_unapplied_spi tests.

2abdfd7... by Nish Aravamudan on 2018-03-06

git_repository: drop ensure_importer_branches_exist

d1b595b... by Nish Aravamudan on 2018-03-06

source_builder: add second assertion

complementary to the other

6d9dd10... by Nish Aravamudan on 2018-03-06

source_builder: if passed in a non-native version, set the internal variable accordingly

0797933... by Nish Aravamudan on 2018-03-06

add functionality to the test

794eb72... by Nish Aravamudan on 2018-03-06

if SourceSpec is given a generator as changelog_versions

Things subtly break, as we do not ensure it is a list anywhere

6f933a7... by Nish Aravamudan on 2018-03-06

add test for case 1 in MP

Unmerged commits

6f933a7... by Nish Aravamudan on 2018-03-06

add test for case 1 in MP

794eb72... by Nish Aravamudan on 2018-03-06

if SourceSpec is given a generator as changelog_versions

Things subtly break, as we do not ensure it is a list anywhere

67ded3c... by Nish Aravamudan on 2018-03-06

import: make upload tags first-class

It is possible for us to modify our algorithm such that upload tags simply
get used directly, rather than as a parent. Effectively, when we
find a matching upload tag, we should see if it has the changelog
parent commit as ancestor (already done as a safety check),
and if so, then just repoint the import tag there.

Fully untested, but for discussion for Robie.

LP: #1734883

c01a167... by Nish Aravamudan on 2018-03-06

import: check if an orphan tag matches the imported tree in all cases

If we attempt to use an import tag, but the tree does not match, we will
orphan, but it's possible this is the *second* time seeing the same
publish event, in which case we should possibly use the orphan tag's
tree this time.

LP: #1753800

0797933... by Nish Aravamudan on 2018-03-06

add functionality to the test

6d9dd10... by Nish Aravamudan on 2018-03-06

source_builder: if passed in a non-native version, set the internal variable accordingly

d1b595b... by Nish Aravamudan on 2018-03-06

source_builder: add second assertion

complementary to the other

2abdfd7... by Nish Aravamudan on 2018-03-06

git_repository: drop ensure_importer_branches_exist

895d80f... by Robie Basak on 2017-12-22

Initial test for import_unapplied_spi

Not got the verification at the end though.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/gitubuntu/git_repository.py b/gitubuntu/git_repository.py
2index 4425823..c85034d 100644
3--- a/gitubuntu/git_repository.py
4+++ b/gitubuntu/git_repository.py
5@@ -917,15 +917,6 @@ class GitUbuntuRepository:
6 self.git_run(['commit', '--allow-empty', '-m', msg])
7 self.git_run(['checkout', '--orphan', 'master'])
8
9- def ensure_importer_branches_exist(self, prefix):
10- self.create_orphan_branch('do-not-push', 'Initial upstream branch.')
11- self.create_orphan_branch('%s/importer/ubuntu/pristine-tar' % prefix,
12- 'Initial Ubuntu pristine-tar branch.'
13- )
14- self.create_orphan_branch('%s/importer/debian/pristine-tar' % prefix,
15- 'Initial Debian pristine-tar branch.'
16- )
17-
18 @contextmanager
19 def pristine_tar_branches(self, dist, namespace='pkg'):
20 """Context manager wrapping pristine-tar branch manipulation
21@@ -957,16 +948,30 @@ class GitUbuntuRepository:
22 old_pt_branch_commit = old_pt_branch.peel(pygit2.Commit)
23 old_pt_branch.delete()
24 local_pt_branch = self.raw_repo.lookup_branch(pt_branch)
25- if local_pt_branch:
26- local_pt_branch.rename('pristine-tar')
27- else:
28- self.raw_repo.create_branch(
29- 'pristine-tar',
30- self.raw_repo.lookup_branch(
31- pt_branch,
32- pygit2.GIT_BRANCH_REMOTE,
33- ).peel(pygit2.Commit),
34+ remote_pt_branch = self.raw_repo.lookup_branch(
35+ pt_branch,
36+ pygit2.GIT_BRANCH_REMOTE,
37 )
38+ # This should only be possible when importing and the first
39+ # pristine-tar usage
40+ if not local_pt_branch and not remote_pt_branch:
41+ self.create_orphan_branch(
42+ pt_branch,
43+ 'Initial %s pristine-tar branch.' % dist,
44+ )
45+ if not self.raw_repo.lookup_branch('do-not-push'):
46+ self.create_orphan_branch(
47+ 'do-not-push',
48+ 'Initial upstream branch.',
49+ )
50+ else:
51+ if local_pt_branch:
52+ local_pt_branch.rename('pristine-tar')
53+ else:
54+ self.raw_repo.create_branch(
55+ 'pristine-tar',
56+ remote_pt_branch.peel(pygit2.Commit),
57+ )
58 try:
59 yield
60 except:
61@@ -974,7 +979,7 @@ class GitUbuntuRepository:
62 finally:
63 if local_pt_branch:
64 self.raw_repo.lookup_branch('pristine-tar').rename(pt_branch)
65- else:
66+ elif remote_pt_branch:
67 self.raw_repo.lookup_branch('pristine-tar').delete()
68 if old_pt_branch_commit:
69 self.raw_repo.create_branch(
70diff --git a/gitubuntu/importer.py b/gitubuntu/importer.py
71index c3e2800..e30f6f9 100644
72--- a/gitubuntu/importer.py
73+++ b/gitubuntu/importer.py
74@@ -250,8 +250,6 @@ def main(
75 except GitUbuntuRepositoryFetchError:
76 pass
77
78- repo.ensure_importer_branches_exist(namespace)
79-
80 debian_sinfo = GitUbuntuSourceInformation(
81 'debian',
82 pkgname,
83@@ -1257,7 +1255,9 @@ def import_unapplied_spi(
84 str(import_tag.peel().id),
85 )
86 return
87- elif orphan_tag:
88+ # if we do not have a changelog parent yet, see if an orphan tag
89+ # matches
90+ if not unapplied_changelog_parent_commit and orphan_tag:
91 if str(orphan_tag.peel().tree.id) != unapplied_import_tree_hash:
92 raise Exception(
93 "Found orphan tag %s, but the corresponding tree "
94@@ -1272,7 +1272,9 @@ def import_unapplied_spi(
95 str(orphan_tag.peel().id),
96 )
97 return
98- elif upload_tag:
99+ # if we do not have a changelog parent yet, see if an upload tag
100+ # matches
101+ if upload_tag:
102 if str(upload_tag.peel().tree.id) != unapplied_import_tree_hash:
103 logging.warn(
104 "Found upload tag %s, but the corresponding tree "
105@@ -1295,11 +1297,33 @@ def import_unapplied_spi(
106 ],
107 verbose_on_failure=False,
108 )
109- unapplied_changelog_parent_commit = None
110 except CalledProcessError as e:
111 if e.returncode != 1:
112 raise
113
114+ repo.update_head_to_commit(
115+ spi.head_name(namespace),
116+ str(upload_tag.peel().id),
117+ )
118+ # should be annotated to use create_tag API
119+ tag = import_tag(spi.version, namespace)
120+ logging.debug(
121+ "Creating tag %s pointing to %s",
122+ tag,
123+ upload_parent_commit,
124+ )
125+ repo.git_run(
126+ [
127+ 'tag',
128+ '-a',
129+ '-m',
130+ 'git-ubuntu import v%s' % VERSION,
131+ tag,
132+ upload_parent_commit,
133+ ]
134+ )
135+ return
136+
137 commit_unapplied_patches_import(
138 repo,
139 spi,
140diff --git a/gitubuntu/importlocal.py b/gitubuntu/importlocal.py
141index b514edf..8adca74 100644
142--- a/gitubuntu/importlocal.py
143+++ b/gitubuntu/importlocal.py
144@@ -42,7 +42,6 @@ def main(
145 if not directory:
146 logging.info('Using git repository at %s', repo.local_dir)
147
148- repo.ensure_importer_branches_exist(namespace)
149 if len(repo.raw_repo.status()) != 0:
150 logging.error('Working tree must be clean to continue.')
151 run(['git', 'status'], stdout=None)
152diff --git a/gitubuntu/importppa.py b/gitubuntu/importppa.py
153index a2d27d3..024143f 100644
154--- a/gitubuntu/importppa.py
155+++ b/gitubuntu/importppa.py
156@@ -57,7 +57,6 @@ def main(
157 repo = GitUbuntuRepository(directory, user, proto)
158 if not directory:
159 logging.info('Using git repository at %s', repo.local_dir)
160- repo.ensure_importer_branches_exist(namespace)
161
162 source_information = GitUbuntuSourceInformation(
163 ppa,
164diff --git a/gitubuntu/source_builder.py b/gitubuntu/source_builder.py
165index f991be3..8894cb5 100644
166--- a/gitubuntu/source_builder.py
167+++ b/gitubuntu/source_builder.py
168@@ -89,6 +89,12 @@ class SourceSpec:
169 if 'version' not in kwargs and not self.native:
170 self.version = '1-1'
171
172+ if 'version' in kwargs and '-' in self.version:
173+ self.native = False
174+
175+ if self.native and '-' in self.version:
176+ raise ValueError("Version must not have a '-' in a native package")
177+
178 if not self.native and '-' not in self.version:
179 raise ValueError("Version must have a '-' in a non-native package")
180
181@@ -133,7 +139,10 @@ class SourceFiles:
182
183 :rtype: str
184 """
185- versions = self.spec.changelog_versions or [self.spec.version]
186+ if self.spec.changelog_versions:
187+ versions = list(self.spec.changelog_versions)
188+ else:
189+ versions = [self.spec.version]
190
191 return "\n".join(
192 CHANGELOG_TEMPLATE.format(version=version)
193diff --git a/gitubuntu/test_importer.py b/gitubuntu/test_importer.py
194index 5ce59da..5d23a55 100644
195--- a/gitubuntu/test_importer.py
196+++ b/gitubuntu/test_importer.py
197@@ -1,9 +1,17 @@
198-from unittest.mock import sentinel
199+from unittest.mock import (
200+ call,
201+ Mock,
202+ sentinel,
203+)
204
205 import pytest
206+import re
207
208+import gitubuntu.source_builder as source_builder
209 import gitubuntu.importer as target
210
211+# XXX
212+from gitubuntu.test_git_repository import pygit2, repo
213
214 @pytest.mark.parametrize('head_versions,expected', [
215 (
216@@ -47,3 +55,79 @@ def test_devel_branch_updates(head_versions, expected):
217 ['yakkety', 'xenial', 'trusty'],
218 )
219 assert set(result) == set(expected.items())
220+
221+
222+def test_import_unapplied_spi(repo):
223+ spec = source_builder.SourceSpec(version='1-1')
224+ source = source_builder.Source(spec)
225+ with source as dsc_path:
226+ spi = Mock()
227+ spi.dsc_pathname = dsc_path
228+ spi.distribution.name = 'Ubuntu'
229+ spi.version = spec.version
230+ # avoid automatic parenting for head_name to make the assertion later
231+ # easier
232+ head_name = Mock(name='head_name')
233+ head_name.return_value = 'importer/importer/ubuntu/trusty'
234+ spi.head_name = head_name
235+ spi.date_published = '1970-01-01T00:00:00'
236+ target.import_unapplied_spi(
237+ repo=repo,
238+ spi=spi,
239+ namespace='importer',
240+ skip_orig=False,
241+ parent_overrides={},
242+ )
243+ # The only call recorded, apart from head_name calls above, should be a
244+ # single pull() call and nothing else. If there is something else, we
245+ # aren't mocking spi as fully as needed.
246+ assert spi.mock_calls == [call.pull()]
247+ import_tag = repo.get_import_tag('1-1', 'importer')
248+ assert import_tag is not None
249+ version, prev_version = repo.get_changelog_versions_from_treeish(str(import_tag.peel().id))
250+ assert version == '1-1'
251+ assert prev_version is None
252+ assert (
253+ repo.get_commitish('importer/importer/ubuntu/trusty').peel(pygit2.Commit).id ==
254+ import_tag.peel(pygit2.Commit).id
255+ )
256+
257+def test_linear_import_unapplied_spi(repo):
258+ versions = ['1-1', '1-2', '2-1',]
259+ for idx, version in enumerate(versions, start=1):
260+ spec = source_builder.SourceSpec(version=version, changelog_versions=reversed(versions[:idx]))
261+ source = source_builder.Source(spec)
262+ with source as dsc_path:
263+ spi = Mock()
264+ spi.dsc_pathname = dsc_path
265+ spi.distribution.name = 'Ubuntu'
266+ spi.version = spec.version
267+ # avoid automatic parenting for head_name to make the assertion later
268+ # easier
269+ head_name = Mock(name='head_name')
270+ head_name.return_value = 'importer/importer/ubuntu/trusty'
271+ spi.head_name = head_name
272+ spi.date_published = '1970-01-01T00:00:00'
273+ target.import_unapplied_spi(
274+ repo=repo,
275+ spi=spi,
276+ namespace='importer',
277+ skip_orig=False,
278+ parent_overrides={},
279+ )
280+
281+ import_tagA = repo.get_import_tag('1-1', 'importer')
282+ import_tagB = repo.get_import_tag('1-2', 'importer')
283+ import_tagC = repo.get_import_tag('2-1', 'importer')
284+ assert all(tag is not None for tag in
285+ [import_tagA, import_tagB, import_tagC]
286+ )
287+ # no orphan tags
288+ assert not list(filter(
289+ lambda r: re.match(r'^refs/tags/importer/orphan', r),
290+ repo.raw_repo.listall_references()
291+ ))
292+ assert (
293+ repo.get_commitish('importer/importer/ubuntu/trusty').peel(pygit2.Commit).id ==
294+ import_tagC.peel(pygit2.Commit).id
295+ )

Subscribers

People subscribed via source and target branches