Merge ~racb/git-ubuntu:changelog-parents into git-ubuntu:master

FAILED: Continuous integration, rev:969c7def7266a904c1a306d743503a7083563f5f
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/469/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    FAILED: Unit Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/469//rebuild

PASSED: Continuous integration, rev:7e2fc4a379cee2d411b880700e8c621e0a86066c
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/470/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/470//rebuild

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_existing_import_tags_ordering
    - 'repo' parameter could use a doc, e.g.:
      ":param gitubuntu.git_repository.GitUbuntuRepository repo: the repository to use."
    - Add comment to explain "repo_builder.Repo(...)", e.g.
      "# 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_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.
    - 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.

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.

6. Generalise test_get_changelog_parent_commits
    - Param docs for test_get_changelog_parent_commits() would be nice to add
    √ Otherwise, LGTM

7. Add multiple changelog parent tests
    √ LGTM

8. Support multiple changelog parents
    √ The list comprehension for calculating changelog_parent_commits feels a bit
      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.

> 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.

Here's the range-diff for your reference: http://paste.ubuntu.com/p/bWR7zShPxp/

PASSED: Continuous integration, rev:fd197c345d0230cb721e62f62dbf01f0569a4003
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/475/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/475//rebuild

Thanks for the range-diff, all looks good, +1.