Code review comment for ~racb/git-ubuntu:changelog-parents

> 2. Add test: test_get_existing_import_tags_ordering
> - 'repo' parameter could use a doc, e.g.:
> ":param gitubuntu.git_repository.GitUbuntuRepository repo: the
> repository to use."

Done, but note that importantly it's a fixture, so I noted that also.

> - Add comment to explain "repo_builder.Repo(...)", e.g.
> "# 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_all_reimport_tags. Since
this is quite long, it busted the line length limit so I also had to
introduce mock_get_all_reimport_tags_patch and wrap a few other lines
too.

> 4. Allow for multiple changelog parents
> √ Previously the code checked that changelog_parent_commit is not
> None, but now it checks len(changelog_parent_commits) != 0.
> Logically this works fine, the one caveat being that this now
> implies an assumption that changelog_parent_commits MUST be a
> 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_parent_commits is always a list
now, and we use an empty list instead of None now, in all cases, to
represent that there aren't any.

> - Given that changelog_parent_commits is always assumed to be a list,
> 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_import_commit_msg: multiple parent case
> √ 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_changelog_parent_commits
> - Param docs for test_get_changelog_parent_commits() would be nice to add

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.