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

login
register
mail settings
Submitter pklecha@forcom.com.pl
Date Feb. 20, 2014, 7:18 a.m.
Message ID <a7e7a7e86bb415e04f6f.1392880698@PKLECHA>
Download mbox | patch
Permalink /patch/3720/
State Accepted
Headers show

Comments

pklecha@forcom.com.pl - Feb. 20, 2014, 7:18 a.m.
# 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.
Siddharth Agarwal - Feb. 20, 2014, 9:18 p.m.
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.
Siddharth Agarwal - Feb. 25, 2014, 7:38 a.m.
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.

Patch

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()
     return ret
 
 @command('^push',