Patchwork [V2] pull: close peer repo on completion (issue2797)

# HG changeset patch
# User Piotr Klecha <pklecha@forcom.com.pl>
# Date 1392879902 -3600
#      Thu Feb 20 08:05:02 2014 +0100
# Node ID a7e7a7e86bb415e04f6f9527fcc367ff6c1a03ad
# Parent  87e52e6425625ea4f7645cfe2fc491a21f9a6b51
pull: close peer repo on completion (issue2797)

When pulling changes from a compressed bundle Mercurial first uncompresses it
to a temporary file in .hg directory. This file will not be deleted unless
the bundlerepo (other) is explicitly closed.

This is similar to cleanup that occurs after incoming.

On 02/19/2014 11:18 PM, pklecha@forcom.com.pl wrote:
> # HG changeset patch
> # User Piotr Klecha <pklecha@forcom.com.pl>
> # Date 1392879902 -3600
> #      Thu Feb 20 08:05:02 2014 +0100
> # Node ID a7e7a7e86bb415e04f6f9527fcc367ff6c1a03ad
> # Parent  87e52e6425625ea4f7645cfe2fc491a21f9a6b51
> pull: close peer repo on completion (issue2797)
>
> When pulling changes from a compressed bundle Mercurial first uncompresses it
> to a temporary file in .hg directory. This file will not be deleted unless
> the bundlerepo (other) is explicitly closed.
>
> This is similar to cleanup that occurs after incoming.
>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -4508,47 +4508,50 @@ def pull(ui, repo, source="default", **o
>       """
>       source, branches = hg.parseurl(ui.expandpath(source), opts.get('branch'))
>       other = hg.peer(repo, opts, source)
> -    ui.status(_('pulling from %s\n') % util.hidepassword(source))
> -    revs, checkout = hg.addbranchrevs(repo, other, branches, opts.get('rev'))
> -
> -    remotebookmarks = other.listkeys('bookmarks')
> -
> -    if opts.get('bookmark'):
> -        if not revs:
> -            revs = []
> -        for b in opts['bookmark']:
> -            if b not in remotebookmarks:
> -                raise util.Abort(_('remote bookmark %s not found!') % b)
> -            revs.append(remotebookmarks[b])
> -
> -    if revs:
> +    try:
> +        ui.status(_('pulling from %s\n') % util.hidepassword(source))
> +        revs, checkout = hg.addbranchrevs(repo, other, branches, opts.get('rev'))
> +
> +        remotebookmarks = other.listkeys('bookmarks')
> +
> +        if opts.get('bookmark'):
> +            if not revs:
> +                revs = []
> +            for b in opts['bookmark']:
> +                if b not in remotebookmarks:
> +                    raise util.Abort(_('remote bookmark %s not found!') % b)
> +                revs.append(remotebookmarks[b])
> +
> +        if revs:
> +            try:
> +                revs = [other.lookup(rev) for rev in revs]
> +            except error.CapabilityError:
> +                err = _("other repository doesn't support revision lookup, "
> +                        "so a rev cannot be specified.")
> +                raise util.Abort(err)
> +
> +        modheads = repo.pull(other, heads=revs, force=opts.get('force'))
> +        bookmarks.updatefromremote(ui, repo, remotebookmarks, source)
> +        if checkout:
> +            checkout = str(repo.changelog.rev(other.lookup(checkout)))
> +        repo._subtoppath = source
>           try:
> -            revs = [other.lookup(rev) for rev in revs]
> -        except error.CapabilityError:
> -            err = _("other repository doesn't support revision lookup, "
> -                    "so a rev cannot be specified.")
> -            raise util.Abort(err)
> -
> -    modheads = repo.pull(other, heads=revs, force=opts.get('force'))
> -    bookmarks.updatefromremote(ui, repo, remotebookmarks, source)
> -    if checkout:
> -        checkout = str(repo.changelog.rev(other.lookup(checkout)))
> -    repo._subtoppath = source
> -    try:
> -        ret = postincoming(ui, repo, modheads, opts.get('update'), checkout)
> -
> +            ret = postincoming(ui, repo, modheads, opts.get('update'), checkout)
> +
> +        finally:
> +            del repo._subtoppath
> +
> +        # update specified bookmarks
> +        if opts.get('bookmark'):
> +            marks = repo._bookmarks
> +            for b in opts['bookmark']:
> +                # explicit pull overrides local bookmark if any
> +                ui.status(_("importing bookmark %s\n") % b)
> +                marks[b] = repo[remotebookmarks[b]].node()
> +            marks.write()
>       finally:
> -        del repo._subtoppath
> -
> -    # update specified bookmarks
> -    if opts.get('bookmark'):
> -        marks = repo._bookmarks
> -        for b in opts['bookmark']:
> -            # explicit pull overrides local bookmark if any
> -            ui.status(_("importing bookmark %s\n") % b)
> -            marks[b] = repo[remotebookmarks[b]].node()
> -        marks.write()
> -
> +        if other is not None:
> +            other.close()

When can other be None? It isn't clear to me.

On 02/24/2014 11:01 PM, Piotr Klecha wrote:
>
> Perhaps never, but since my knowledge of Mercurial is fairly limited, 
> I wanted to make this fix as safe as possible. If you could confirm 
> this is actually unnecessary, I will gladly remove the redundant 
> condition. 

The surrounding code assumes other is not None, so it's very likely 
other being None is a programming error and should not be explicitly 
detected. I definitely think it's unnecessary.

Can you add a test for this? Seems this has user-visible effects so it 
should be fairly easy to add one.