Merge ~racb/git-ubuntu:importer-cleanup-tmpdir-always into git-ubuntu:master

[Robie Basak]

Make jenkins happy

This supersedes Nish's MP as agreed.

I've retained test_importer_main_cleanup_on_exception for now, but I think that the combination of test_importer_close_repository_on_exception and the GitUbuntuRepository.close() method tests makes it redundant, so I propose to drop it. It is passing so we can leave it if you wish though.

I have just realised that I've obliterated Nish's careful separation of getting a test failing with xfail first. I can restore that separation and rebase tomorrow if needed.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:61bc3182f1846b165c4bdf6453c6557625613282
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/413/
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/413/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

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

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

review: Approve (continuous-integration)

[Robie Basak (racb)]

review: Needs Fixing

[Nish Aravamudan (nacc)]

On Mon, Apr 23, 2018 at 10:41 AM, Robie Basak <email address hidden> wrote:
> Review: Needs Fixing
>

Code fixes addressed in latest commit.

>> +
>> def get_changelog_for_commit(
>> repo,
>> tree_hash,
>> diff --git a/gitubuntu/test_importer.py b/gitubuntu/test_importer.py
>> index b7f24a1..bf7d5ff 100644
>> --- a/gitubuntu/test_importer.py
>> +++ b/gitubuntu/test_importer.py
>> @@ -633,3 +635,39 @@ def test_get_changelog_parent_commit(
>> str(repo.raw_repo.lookup_reference(expected_ref).peel(pygit2.Commit).id)
>> if expected_ref else expected_ref
>> )
>> +
>> +@patch('gitubuntu.git_repository.GitUbuntuRepository.add_base_remotes')
>> +def test_importer_main_cleanup_on_exception(add_base_remotes_mock):
>
> I'm a little concerned that this test involves knowing so much about the internals
> that it'll be more of a pain to maintain than its usefulness in detecting a real
> problem in the future. I'm not sure I have a better answer. I'm not sure it's worth
> having a test at all in this case. I'd be interested in others' views on this.
> I'm on the fence.

Perhaps we have different definitions of 'so much' :) I simply need a
way to cause main() to exit early. I'm not sure how else to test error
paths, withoiut invoking the network and/or making Launchpad
publishing data or faking a filesystem.

I'm also not sure how to square 'not worth having a test' with ...
everything new should have a test.

>> + # leverage that the importer creates temporary directories
>> + local_tmpdir = tempfile.mkdtemp()
>> + assert not os.listdir(local_tmpdir)
>> +
>> + orig_mkdtemp = tempfile.mkdtemp
>> + def new_mkdtemp(suffix=None, prefix=None, dir=None):
>> + return orig_mkdtemp(suffix, prefix, dir=local_tmpdir)
>> +
>> + # patch here rather than as a decorator so we can use the original
>> + # function above
>> + with patch('tempfile.mkdtemp') as mkdtemp_mock:
>
> What if the function under test (basically the entire program) creates temporary
> files in some other way, or leaves junk somewhere else?

That would be a code change in the importer, no? And if that was to
happen, afaict, this test would presumably break.

I suppose we could change the git_repository code (refactor) to not
invoke mkdtemp but a helper function to create the repository and mock
that?

The real reason for this, and not just passing a directory in
directly, is that in the current code passing a directory argument
implies no_clean. So we wouldn't clenaup in that case.

We could alternatively abstract out the "should I clean" decision
(which would take the directory and no_clean cli values as arguments)
and then mock that only. Then we would be able to pass in a directory
argument and expect it to be removed in the error path regardless of
what else the importer writes.

Note, that your point above about 'junk' is outside the scope of what
I'm testing here. If we do create other junk, then that would (imo) be
a different test. What I'm testing here is that the repository created
by the importer is removed on failure/error.

>> + mkdtemp_mock.side_effect = new_mkdtemp
>> +
>> + add_base_remotes_mock.side_effe...

[Server Team CI bot (server-team-bot)]

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

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

review: Approve (continuous-integration)

[Robie Basak (racb)]

On Mon, Apr 23, 2018 at 07:51:19PM -0000, Nish Aravamudan wrote:
> I'm also not sure how to square 'not worth having a test' with ...
> everything new should have a test.

I read it as "everything new should have a test where it makes sense in
the code to do so". Pragmatically, there's no point in writing a test if
we don't see a way to write it such that the potential benefit outweighs
the cost. In this case I think this is quite close to the line. But
given that you've written it and it works, I'm thinking that there's no
harm done in landing it. Perhaps if it causes a future problem, I'll
propose to remove it then rather than fixing it.

> > What if the function under test (basically the entire program) creates temporary
> > files in some other way, or leaves junk somewhere else?
>
> That would be a code change in the importer, no? And if that was to
> happen, afaict, this test would presumably break.

I think in this case, depending on the specifics, the test is likely to
continue passing while not testing what it is supposed to be testing.
This is why I think it is of limited benefit for us to have it.

> I suppose we could change the git_repository code (refactor) to not
> invoke mkdtemp but a helper function to create the repository and mock
> that?

We could, but then we'd need to remember in the future that this is
required to make the test work. Refactoring it later without that in
mind will result to the same I-broke-it-but-the-test-still-passes
result.

> The real reason for this, and not just passing a directory in
> directly, is that in the current code passing a directory argument
> implies no_clean. So we wouldn't clenaup in that case.
>
> We could alternatively abstract out the "should I clean" decision
> (which would take the directory and no_clean cli values as arguments)
> and then mock that only. Then we would be able to pass in a directory
> argument and expect it to be removed in the error path regardless of
> what else the importer writes.

What if we give GitUbuntuRepository a close() method, move to it a
no_clean (or more clearly delete_on_close) attribute, and give the class
the responsibility of cleaning up?

We could then separately unit test that the class does clean up and does
not clean up as required based on the attribute, and that the main
function does correctly call the close() method on some specific causes
of failure.

> Note, that your point above about 'junk' is outside the scope of what
> I'm testing here. If we do create other junk, then that would (imo) be
> a different test. What I'm testing here is that the repository created
> by the importer is removed on failure/error.

Fair enough. I hadn't got that from
test_importer_main_cleanup_on_exception. We should make it clear then
that this is exactly the scope of the test (by name or docstring).

> > What if the function fails before it does anything due to the way it is
> > called from this test? Might be better to look for a more specifically
> > defined exception (which we raise from the mock) rather than doing
> > a catch-all on it.
>
> Yeah I can make this a RuntimeError.

I suggest something even more specific like:

    class MockError(Runt...

[Nish Aravamudan (nacc)]

On Tue, Apr 24, 2018 at 3:27 AM, Robie Basak <email address hidden>
wrote:

> On Mon, Apr 23, 2018 at 07:51:19PM -0000, Nish Aravamudan wrote:
> > I'm also not sure how to square 'not worth having a test' with ...
> > everything new should have a test.
>
> I read it as "everything new should have a test where it makes sense in
> the code to do so". Pragmatically, there's no point in writing a test if
> we don't see a way to write it such that the potential benefit outweighs
> the cost. In this case I think this is quite close to the line. But
> given that you've written it and it works, I'm thinking that there's no
> harm done in landing it. Perhaps if it causes a future problem, I'll
> propose to remove it then rather than fixing it.
>

Yes that makes sense. On some level, I wanted a programmatic way to check
that my changes did what I wanted and a unit test for it made sense, even
if it's fugly :) I am also happy to not land the test if it's too fragile
in your opinion, as I think it demonstrates the fix for the purpose of
review still.

> > > What if the function under test (basically the entire program) creates
> temporary
> > > files in some other way, or leaves junk somewhere else?
> >
> > That would be a code change in the importer, no? And if that was to
> > happen, afaict, this test would presumably break.
>
> I think in this case, depending on the specifics, the test is likely to
> continue passing while not testing what it is supposed to be testing.
> This is why I think it is of limited benefit for us to have it.
>

Ok -- I'm having trouble following the potential hypothetical, but I
believe you and understand your perspective.

>
> > I suppose we could change the git_repository code (refactor) to not
> > invoke mkdtemp but a helper function to create the repository and mock
> > that?
>
> We could, but then we'd need to remember in the future that this is
> required to make the test work. Refactoring it later without that in
> mind will result to the same I-broke-it-but-the-test-still-passes
> result.
>

Right, I meant before landing it now, but I see your point.

> > The real reason for this, and not just passing a directory in
> > directly, is that in the current code passing a directory argument
> > implies no_clean. So we wouldn't clenaup in that case.
> >
> > We could alternatively abstract out the "should I clean" decision
> > (which would take the directory and no_clean cli values as arguments)
> > and then mock that only. Then we would be able to pass in a directory
> > argument and expect it to be removed in the error path regardless of
> > what else the importer writes.
>
> What if we give GitUbuntuRepository a close() method, move to it a
> no_clean (or more clearly delete_on_close) attribute, and give the class
> the responsibility of cleaning up?
>
> We could then separately unit test that the class does clean up and does
> not clean up as required based on the attribute, and that the main
> function does correctly call the close() method on some specific causes
> of failure.
>

This seems reasonable and I can work on that today, I think.

> > Note, that your point above about 'junk' is outside the scope of w...

[Robie Basak (racb)]

Quickly, to correct an error:

On Wed, Apr 25, 2018 at 06:54:23PM -0000, Nish Aravamudan wrote:
> > > > What if the function fails before it does anything due to the way it is
> > > > called from this test? Might be better to look for a more specifically
> > > > defined exception (which we raise from the mock) rather than doing
> > > > a catch-all on it.
> > >
> > > Yeah I can make this a RuntimeError.
> >
> > I suggest something even more specific like:
> >
> > class MockError(RuntimeError): pass
> >
> > ...because otherwise a real RuntimeError would elide the test in the
> > same way as my objection to using Exception.
> >
>
> Of course!

I realised later that deriving from RuntimeError would also elide a real
RuntimeError being caught. I concluded that MockError should derive from
either BaseException or Exception (need to look at the definitions).

[Thomas Ward (teward)]

I can confirm based on a cursory overview of the code that MockError having a superclass of RuntimeError will result in any `except RuntimeError` captures picking up MockError in that class.

And per Robie's comment, MockError should probably inherit from Exception or BaseException if you want it to be handled as its own class of exceptions outside that of RuntimeError.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:d68d0377995365debd619b1ec6e92b2f1de18349
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/420/
Executed test runs:
    SUCCESS: VM Setup
    FAILED: Build

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

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

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

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

review: Approve (continuous-integration)

[Server Team CI bot (server-team-bot)]

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

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

review: Approve (continuous-integration)

[Robie Basak (racb)]

Sorry I've been so long in responding to this. I was tasked on other projects. I've finally been able to context switch back to git-ubuntu and hope to be on it for the next week or so.

I've made a bunch of comments inline. I'm sorry I didn't notice the kwargs thing before; I realise that was present in my original review.

In any case, given that it's been so long, I don't think it's fair to ask you to take all of this on again. So I've prepared a branch that addresses my comments, which I can post as a separate MP and supersede this MP if you're happy with that as an approach to proceeding?

In that branch I've also written a proposed replacement for your test_importer_main_cleanup_on_exception called test_importer_close_repository_on_exception. I've left test_importer_main_cleanup_on_exception there for now, but I propose (weakly) that it can be dropped because test_importer_close_repository_on_exception is simpler and combined with your unit tests on GitUbuntuRepository.close() establish the same regression check that test_importer_main_cleanup_on_exception does.

I've pushed my branch to https://code.launchpad.net/~racb/usd-importer/+git/usd-importer/+ref/importer-cleanup-tmpdir-always - feel free to resubmit this MP using that branch if you'd like, or I can do it, or we can find some other way forward.

review: Needs Fixing

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:b2043b09d61ed83f967fe1cafcf927615723073b
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/424/
Executed test runs:
    SUCCESS: VM Setup
    FAILED: Build

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

review: Needs Fixing (continuous-integration)

[Robie Basak (racb)]

> I have just realised that I've obliterated Nish's careful separation of getting a test failing with xfail first. I can restore that separation and rebase tomorrow if needed.

I spent some time this morning looking at doing this, but came to the conclusion that it isn't worth the effort on this occasion. The tests as finally written won't work in that form prior to the refactor. So I'd have to write some other kind of tests (perhaps Nish's original ones) and then change the tests to the new form later.

Having said that, if you'd like it done in that form or another, I can still do it - just let me know.

[Nish Aravamudan (nacc)]

I think I'm fine with you landing this as-is (ish). I think you need a rebase once you land the build fix into master, at a minimum, and perhaps you need to squash that last commit in somewhere. But the overall changes look fine to me.

review: Approve

[Andreas Hasenack (ahasenack)]

review: Needs Information

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:e9758286a266662e7dca76ffe2d1ea4deda711ea
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/426/
Executed test runs:
    SUCCESS: VM Setup
    FAILED: Build

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

review: Needs Fixing (continuous-integration)

[Andreas Hasenack (ahasenack)]

Thanks for the inline explanation, +1.

review: Approve

[Robie Basak (racb)]

Thanks! I've rebased on master to pick up a CI fix, and squashed e975828 into d9764fe while I was there. No other changes.

[Server Team CI bot (server-team-bot)]

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

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

review: Approve (continuous-integration)

diff --git a/gitubuntu/git_repository.py b/gitubuntu/git_repository.py
index 509b8d9..9d2ca66 100644
--- a/gitubuntu/git_repository.py
+++ b/gitubuntu/git_repository.py
@@ -874,7 +874,13 @@ class GitUbuntuRepository:
 
     """
 
-    def __init__(self, local_dir, lp_user=None, fetch_proto=None):
+    def __init__(
+        self,
+        local_dir,
+        lp_user=None,
+        fetch_proto=None,
+        delete_on_close=True,
+    ):
         """
         If fetch_proto is None, the default value from
         gitubuntu.__main__ will be used (top_level_defaults.proto).
@@ -935,6 +941,20 @@ class GitUbuntuRepository:
             fetch_proto = top_level_defaults.proto
 
         self._fetch_proto = fetch_proto
+        self._delete_on_close = delete_on_close
+
+    def close(self):
+        """Free resources associated with this instance
+
+        If delete_on_close was True on instance construction, local_dir (as
+        specified on instance construction) will be deleted.
+
+        After this method is called, the instance is invalid and can no longer
+        be used.
+        """
+        if self.raw_repo and self._delete_on_close:
+            shutil.rmtree(self.local_dir)
+        self.raw_repo = None
 
     def create_orphan_branch(self, branch_name, msg):
         if self.get_head_by_name(branch_name) is None:
diff --git a/gitubuntu/importer.py b/gitubuntu/importer.py
index 8bb1ca2..d6b9db8 100644
--- a/gitubuntu/importer.py
+++ b/gitubuntu/importer.py
@@ -25,7 +25,6 @@
 #       + the last imported version from debian/changelog
 
 import argparse
-import atexit
 import functools
 import getpass
 import logging
@@ -130,30 +129,19 @@ def dsc_to_tree_hash(pygit2_repo, dsc_path, patches_applied=False):
         )
 
 
-def cleanup(no_clean, local_dir):
-    '''Remove a local directory, conditionally
-
-    no_clean: if True, do not remove @local_dir and print a message instead
-    local_dir: directory to remove
-    '''
-    if not no_clean:
-        shutil.rmtree(local_dir)
-    else:
-        logging.info('Leaving %s as directed' % local_dir)
-
-
 # XXX: need a namedtuple to hold common arguments
-def main(
+def _main_with_repo(
+    repo,
     pkgname,
     owner,
-    no_clean,
-    no_fetch,
-    no_push,
-    active_series_only,
-    skip_orig,
-    skip_applied,
-    reimport,
-    allow_applied_failures,
+    no_clean=False,
+    no_fetch=False,
+    no_push=False,
+    active_series_only=False,
+    skip_orig=False,
+    skip_applied=False,
+    reimport=False,
+    allow_applied_failures=False,
     directory=None,
     dl_cache=None,
     user=None,
@@ -163,41 +151,14 @@ def main(
     retries=top_level_defaults.retries,
     retry_backoffs=top_level_defaults.retry_backoffs,
 ):
-    """Main entry point to the importer
+    """Internal main function of the importer
 
     Arguments:
-    @pkgname: string name of source package
-    @owner: string owner of repository
-    @no_clean: if True, do not delete the local directory
-    @no_fetch: if True, do not fetch the target repository before
-    importing
-    @no_push: if True, do not push to the target repository
-    @active_series_only: if True, only import active series
-    @skip_orig: if True, do not attempt to use gbp to import orig
-    tarballs
-    @skip_applied: if True, do not attempt to import patches-applied
-    publishes
-    @reimport: if True, import the source package from scratch and
-    delete/recreate the target repository.
-    @allow_import_failures: if True, and patches fail to apply for any
-    publish, that patches-applied import will be skipped rather than an
-    error
-    @directory: string path for local repository
-    @user: string user to authenticate to Launchpad as
-    @proto: string protocol to use (one of 'http', 'https', 'git')
-    @dl_cache: string path where downloaded files should be cached
-    @pullfile: string path to file specifying pull overrides
-    @parentfile: string path to file specifying parent overrides
-    @retries: integer number of download retries to attempt
-    @retry_backoffs: list of backoff durations to use between retries
-
-    If directory is None, a temporary directory is created and used.
+    @repo: GitUbuntuRepository to import to. Unlike the main function, this
+    object will not be closed when this function returns.
 
-    If user is None, value of `git config gitubuntu.lpuser` will be
-    used.
-
-    If dl_cache is None, CACHE_PATH in the local repository will be
-    used.
+    For the meaning of all other arguments, see the docstring of the main
+    function.
 
     Returns 0 on successful import (which includes non-fatal failures);
     1 otherwise.
@@ -207,14 +168,6 @@ def main(
     else:
         namespace = owner
 
-    logging.info('Ubuntu Server Team importer v%s' % VERSION)
-
-    repo = GitUbuntuRepository(directory, user, proto)
-    if not directory:
-        logging.info('Using git repository at %s', repo.local_dir)
-
-    atexit.register(cleanup, no_clean, repo.local_dir)
-
     repo.add_base_remotes(pkgname, repo_owner=owner)
     if not no_fetch and not reimport:
         try:
@@ -431,6 +384,108 @@ def main(
     return 0
 
 
+# XXX: need a namedtuple to hold common arguments
+def main(
+    pkgname,
+    owner,
+    no_clean=False,
+    no_fetch=False,
+    no_push=False,
+    active_series_only=False,
+    skip_orig=False,
+    skip_applied=False,
+    reimport=False,
+    allow_applied_failures=False,
+    directory=None,
+    dl_cache=None,
+    user=None,
+    proto=top_level_defaults.proto,
+    pullfile=top_level_defaults.pullfile,
+    parentfile=top_level_defaults.parentfile,
+    retries=top_level_defaults.retries,
+    retry_backoffs=top_level_defaults.retry_backoffs,
+    repo=None,
+):
+    """Main entry point to the importer
+
+    Arguments:
+    @pkgname: string name of source package
+    @owner: string owner of repository
+    @no_clean: if True, do not delete the local directory
+    @no_fetch: if True, do not fetch the target repository before
+    importing
+    @no_push: if True, do not push to the target repository
+    @active_series_only: if True, only import active series
+    @skip_orig: if True, do not attempt to use gbp to import orig
+    tarballs
+    @skip_applied: if True, do not attempt to import patches-applied
+    publishes
+    @reimport: if True, import the source package from scratch and
+    delete/recreate the target repository.
+    @allow_applied_failures: if True, and patches fail to apply for any
+    publish, that patches-applied import will be skipped rather than an
+    error
+    @directory: string path for local repository
+    @user: string user to authenticate to Launchpad as
+    @proto: string protocol to use (one of 'http', 'https', 'git')
+    @dl_cache: string path where downloaded files should be cached
+    @pullfile: string path to file specifying pull overrides
+    @parentfile: string path to file specifying parent overrides
+    @retries: integer number of download retries to attempt
+    @retry_backoffs: list of backoff durations to use between retries
+    @repo: if specified, a GitUbuntuRepository instance. If not specified, a
+    GitUbuntuRepository instance will be created using the provided parameters.
+    Regardless of whether it was specified or not, the instance will be closed
+    before this function returns.
+
+    If directory is None, a temporary directory is created and used.
+
+    If user is None, value of `git config gitubuntu.lpuser` will be
+    used.
+
+    If dl_cache is None, CACHE_PATH in the local repository will be
+    used.
+
+    Returns 0 on successful import (which includes non-fatal failures);
+    1 otherwise.
+    """
+    logging.info('Ubuntu Server Team importer v%s' % VERSION)
+
+    if not repo:
+        repo = GitUbuntuRepository(
+            directory,
+            user,
+            proto,
+            delete_on_close=not no_clean,
+        )
+    if not directory:
+        logging.info('Using git repository at %s', repo.local_dir)
+
+    try:
+        _main_with_repo(
+            repo=repo,
+            pkgname=pkgname,
+            owner=owner,
+            no_clean=no_clean,
+            no_fetch=no_fetch,
+            no_push=no_push,
+            active_series_only=active_series_only,
+            skip_orig=skip_orig,
+            skip_applied=skip_applied,
+            reimport=reimport,
+            allow_applied_failures=allow_applied_failures,
+            directory=directory,
+            user=user,
+            dl_cache=dl_cache,
+            proto=proto,
+            pullfile=pullfile,
+            parentfile=parentfile,
+            retries=retries,
+            retry_backoffs=retry_backoffs,
+        )
+    finally:
+        repo.close()
+
 def get_changelog_for_commit(
     repo,
     tree_hash,
diff --git a/gitubuntu/test_git_repository.py b/gitubuntu/test_git_repository.py
index dc30994..2bbb25d 100644
--- a/gitubuntu/test_git_repository.py
+++ b/gitubuntu/test_git_repository.py
@@ -1,6 +1,7 @@
 import copy
 import itertools
 import os
+import shutil
 import tempfile
 import unittest
 import unittest.mock
@@ -680,3 +681,29 @@ def test_repo_find_ubuntu_merge(
     ) == str(
         repo.get_commitish(expected).peel(pygit2.Commit).id
     )
+
+
+def test_repo_does_cleanup():
+    path = tempfile.mkdtemp()
+    try:
+        repo = target.GitUbuntuRepository(
+            path,
+            delete_on_close=True,
+        )
+        repo.close()
+        assert not os.path.exists(path)
+    finally:
+        shutil.rmtree(path, ignore_errors=True)
+
+
+def test_repo_does_not_cleanup():
+    path = tempfile.mkdtemp()
+    try:
+        repo = target.GitUbuntuRepository(
+            path,
+            delete_on_close=False,
+        )
+        repo.close()
+        assert os.path.exists(path)
+    finally:
+        shutil.rmtree(path, ignore_errors=True)
diff --git a/gitubuntu/test_importer.py b/gitubuntu/test_importer.py
index b7f24a1..df011e3 100644
--- a/gitubuntu/test_importer.py
+++ b/gitubuntu/test_importer.py
@@ -4,7 +4,10 @@ from unittest.mock import (
     sentinel,
 )
 
+import os
 import pytest
+import shutil
+import tempfile
 
 import gitubuntu.repo_builder as repo_builder
 import gitubuntu.repo_comparator as repo_comparator
@@ -633,3 +636,62 @@ def test_get_changelog_parent_commit(
         str(repo.raw_repo.lookup_reference(expected_ref).peel(pygit2.Commit).id)
         if expected_ref else expected_ref
     )
+
+@patch('gitubuntu.git_repository.GitUbuntuRepository.add_base_remotes')
+def test_importer_main_cleanup_on_exception(add_base_remotes_mock):
+    """Ensure that importer.main cleans up the temporary directory on errors
+
+    If there are other files used/created by the importer outside of the
+    repository, they are not tested for removal by this test.
+    """
+    # leverage that the importer creates temporary directories
+    local_tmpdir = tempfile.mkdtemp()
+    try:
+        assert not os.listdir(local_tmpdir)
+
+        orig_mkdtemp = tempfile.mkdtemp
+        def new_mkdtemp(suffix=None, prefix=None, dir=None):
+            return orig_mkdtemp(suffix, prefix, dir=local_tmpdir)
+
+        # patch here rather than as a decorator so we can use the original
+        # function above
+        with patch('tempfile.mkdtemp') as mkdtemp_mock:
+            class MockError(Exception): pass
+
+            mkdtemp_mock.side_effect = new_mkdtemp
+
+            add_base_remotes_mock.side_effect = MockError()
+            with pytest.raises(MockError):
+                target.main(pkgname='dummy', owner='dummy')
+
+        assert not os.listdir(local_tmpdir)
+    finally:
+        shutil.rmtree(local_tmpdir)
+
+
+@patch('gitubuntu.importer._main_with_repo')
+def test_importer_close_repository_on_exception(main_with_repo_mock):
+    """Ensure that importer.main closes the repository in error cases
+
+    Together with unit tests on the behaviour of GitUbuntuRepository.close(),
+    this ensures that the repository directory is correctly cleaned up when
+    required.
+
+    This is effectively a white box test as we rely on the following
+    implementation details of importer.main:
+
+     * That it calls _main_with_repo to do the actual work such that we can
+       mock failure of its entire call with an exception.
+
+     * That it will still function enough to correctly close the repository
+       on _main_with_repo failure and when given no other arguments.
+
+    If these implementation assumptions stop being true, this test will stop
+    passing and will need reworking.
+    """
+    class MockError(Exception): pass
+    main_with_repo_mock.side_effect = MockError()
+    repo = Mock()
+    with pytest.raises(MockError):
+        target.main(pkgname='dummy', owner='dummy', repo=repo)
+    repo.close.assert_called()