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