Patchwork [5,of,5,V2] streamclone: use context manager for writing files

login
register
mail settings
Submitter Gregory Szorc
Date Jan. 3, 2016, 12:45 a.m.
Message ID <3523527da243b2d150d5.1451781933@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/12499/
State Accepted
Delegated to: Matt Mackall
Headers show

Comments

Gregory Szorc - Jan. 3, 2016, 12:45 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1451776198 28800
#      Sat Jan 02 15:09:58 2016 -0800
# Node ID 3523527da243b2d150d54d77aa630b23b2e9ad7d
# Parent  438dce208879a651879d123d646c6782e3936f42
streamclone: use context manager for writing files

These are the file writes that have the most to gain from background
I/O. Plug in a context manager so I can design the background I/O
mechanism with context managers in mind.
Augie Fackler - Jan. 5, 2016, 5:09 p.m.
On Sat, Jan 02, 2016 at 04:45:33PM -0800, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1451776198 28800
> #      Sat Jan 02 15:09:58 2016 -0800
> # Node ID 3523527da243b2d150d54d77aa630b23b2e9ad7d
> # Parent  438dce208879a651879d123d646c6782e3936f42
> streamclone: use context manager for writing files

I'm a huge fan of this series. Are we okay with starting to use
context managers for stuff like this? It sounds like a nice win for
the abstraction needed for Greg's background IO business.

>
> These are the file writes that have the most to gain from background
> I/O. Plug in a context manager so I can design the background I/O
> mechanism with context managers in mind.
>
> diff --git a/mercurial/streamclone.py b/mercurial/streamclone.py
> --- a/mercurial/streamclone.py
> +++ b/mercurial/streamclone.py
> @@ -314,22 +314,22 @@ def consumev1(repo, fp, filecount, bytec
>                      size = int(size)
>                  except (ValueError, TypeError):
>                      raise error.ResponseError(
>                          _('unexpected response from remote server:'), l)
>                  if repo.ui.debugflag:
>                      repo.ui.debug('adding %s (%s)\n' %
>                                    (name, util.bytecount(size)))
>                  # for backwards compat, name was partially encoded
> -                ofp = repo.svfs(store.decodedir(name), 'w')
> -                for chunk in util.filechunkiter(fp, limit=size):
> -                    handled_bytes += len(chunk)
> -                    repo.ui.progress(_('clone'), handled_bytes, total=bytecount)
> -                    ofp.write(chunk)
> -                ofp.close()
> +                with repo.svfs(store.decodedir(name), 'w') as ofp:
> +                    for chunk in util.filechunkiter(fp, limit=size):
> +                        handled_bytes += len(chunk)
> +                        repo.ui.progress(_('clone'), handled_bytes,
> +                                         total=bytecount)
> +                        ofp.write(chunk)
>              tr.close()
>          finally:
>              tr.release()
>
>          # Writing straight to files circumvented the inmemory caches
>          repo.invalidate()
>
>          elapsed = time.time() - start
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Matt Mackall - Jan. 11, 2016, 10:58 p.m.
On Tue, 2016-01-05 at 12:09 -0500, Augie Fackler wrote:
> On Sat, Jan 02, 2016 at 04:45:33PM -0800, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1451776198 28800
> > #      Sat Jan 02 15:09:58 2016 -0800
> > # Node ID 3523527da243b2d150d54d77aa630b23b2e9ad7d
> > # Parent  438dce208879a651879d123d646c6782e3936f42
> > streamclone: use context manager for writing files
> 
> I'm a huge fan of this series. Are we okay with starting to use
> context managers for stuff like this?

Yes.

-- 
Mathematics is the supreme nostalgia of our time.
Augie Fackler - Jan. 12, 2016, 2:55 a.m.
On Sat, Jan 02, 2016 at 04:45:33PM -0800, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1451776198 28800
> #      Sat Jan 02 15:09:58 2016 -0800
> # Node ID 3523527da243b2d150d54d77aa630b23b2e9ad7d
> # Parent  438dce208879a651879d123d646c6782e3936f42
> streamclone: use context manager for writing files

Queued these. I like how this one in particular avoids potential bugs
(it wasn't even a try/finally before.)

>
> These are the file writes that have the most to gain from background
> I/O. Plug in a context manager so I can design the background I/O
> mechanism with context managers in mind.
>
> diff --git a/mercurial/streamclone.py b/mercurial/streamclone.py
> --- a/mercurial/streamclone.py
> +++ b/mercurial/streamclone.py
> @@ -314,22 +314,22 @@ def consumev1(repo, fp, filecount, bytec
>                      size = int(size)
>                  except (ValueError, TypeError):
>                      raise error.ResponseError(
>                          _('unexpected response from remote server:'), l)
>                  if repo.ui.debugflag:
>                      repo.ui.debug('adding %s (%s)\n' %
>                                    (name, util.bytecount(size)))
>                  # for backwards compat, name was partially encoded
> -                ofp = repo.svfs(store.decodedir(name), 'w')
> -                for chunk in util.filechunkiter(fp, limit=size):
> -                    handled_bytes += len(chunk)
> -                    repo.ui.progress(_('clone'), handled_bytes, total=bytecount)
> -                    ofp.write(chunk)
> -                ofp.close()
> +                with repo.svfs(store.decodedir(name), 'w') as ofp:
> +                    for chunk in util.filechunkiter(fp, limit=size):
> +                        handled_bytes += len(chunk)
> +                        repo.ui.progress(_('clone'), handled_bytes,
> +                                         total=bytecount)
> +                        ofp.write(chunk)
>              tr.close()
>          finally:
>              tr.release()
>
>          # Writing straight to files circumvented the inmemory caches
>          repo.invalidate()
>
>          elapsed = time.time() - start
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/streamclone.py b/mercurial/streamclone.py
--- a/mercurial/streamclone.py
+++ b/mercurial/streamclone.py
@@ -314,22 +314,22 @@  def consumev1(repo, fp, filecount, bytec
                     size = int(size)
                 except (ValueError, TypeError):
                     raise error.ResponseError(
                         _('unexpected response from remote server:'), l)
                 if repo.ui.debugflag:
                     repo.ui.debug('adding %s (%s)\n' %
                                   (name, util.bytecount(size)))
                 # for backwards compat, name was partially encoded
-                ofp = repo.svfs(store.decodedir(name), 'w')
-                for chunk in util.filechunkiter(fp, limit=size):
-                    handled_bytes += len(chunk)
-                    repo.ui.progress(_('clone'), handled_bytes, total=bytecount)
-                    ofp.write(chunk)
-                ofp.close()
+                with repo.svfs(store.decodedir(name), 'w') as ofp:
+                    for chunk in util.filechunkiter(fp, limit=size):
+                        handled_bytes += len(chunk)
+                        repo.ui.progress(_('clone'), handled_bytes,
+                                         total=bytecount)
+                        ofp.write(chunk)
             tr.close()
         finally:
             tr.release()
 
         # Writing straight to files circumvented the inmemory caches
         repo.invalidate()
 
         elapsed = time.time() - start