Patchwork [01,of,11] util: put compression code next to each other

login
register
mail settings
Submitter Gregory Szorc
Date Nov. 2, 2016, 12:08 a.m.
Message ID <60f180c9a030ebcee6c6.1478045314@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/17244/
State Accepted
Headers show

Comments

Gregory Szorc - Nov. 2, 2016, 12:08 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1476577441 25200
#      Sat Oct 15 17:24:01 2016 -0700
# Node ID 60f180c9a030ebcee6c6f4f8584fdb94c73ac337
# Parent  e5cc44ea12de681d971fcbebb65a7fb71fd1c3c7
util: put compression code next to each other

ctxmanager was injecting itself between the compression and
decompression code. Let's restore some order.
Pierre-Yves David - Nov. 2, 2016, 2:26 p.m.
On 11/02/2016 01:08 AM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1476577441 25200
> #      Sat Oct 15 17:24:01 2016 -0700
> # Node ID 60f180c9a030ebcee6c6f4f8584fdb94c73ac337
> # Parent  e5cc44ea12de681d971fcbebb65a7fb71fd1c3c7
> util: put compression code next to each other
>
> ctxmanager was injecting itself between the compression and
> decompression code. Let's restore some order.

patch 1 is pushed, patch 2 is yet to be reviewed.

Some feedback on your submition scheme, given the size of patches 2 (and 
therefor likeness of comment), it would have been better to keep the 
series small in the form of:

1) preparatory patches
2) large/new/complex patch
3) minimal usage of the new code

Then you can send a larger series of more trivial usage of the new code 
once it is accepted.
Gregory Szorc - Nov. 2, 2016, 9:52 p.m.
On Wed, Nov 2, 2016 at 7:26 AM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 11/02/2016 01:08 AM, Gregory Szorc wrote:
>
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1476577441 25200
>> #      Sat Oct 15 17:24:01 2016 -0700
>> # Node ID 60f180c9a030ebcee6c6f4f8584fdb94c73ac337
>> # Parent  e5cc44ea12de681d971fcbebb65a7fb71fd1c3c7
>> util: put compression code next to each other
>>
>> ctxmanager was injecting itself between the compression and
>> decompression code. Let's restore some order.
>>
>
> patch 1 is pushed, patch 2 is yet to be reviewed.
>
> Some feedback on your submition scheme, given the size of patches 2 (and
> therefor likeness of comment), it would have been better to keep the series
> small in the form of:
>
> 1) preparatory patches
> 2) large/new/complex patch
> 3) minimal usage of the new code
>
> Then you can send a larger series of more trivial usage of the new code
> once it is accepted.


From my experience, submitting code to introduce a new API without clear
examples of how that API is used makes things harder because it isn't clear
how the new code will be used and/or why it is necessary. A reviewer could
justifiably drag their feet until these questions are answered. Code
answers those questions.

I agree that part 2 could have been split. However, I thought it best to
introduce a semi-complete snapshot of the API in a single go so reviewers
didn't have to piece it together from several diffs. Now that you've seen
the general scope of what I'm doing, if you want me to split it up, I can.

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -2789,42 +2789,16 @@  if safehasattr(parsers, 'dirs'):
     dirs = parsers.dirs
 
 def finddirs(path):
     pos = path.rfind('/')
     while pos != -1:
         yield path[:pos]
         pos = path.rfind('/', 0, pos)
 
-# compression utility
-
-class nocompress(object):
-    def compress(self, x):
-        return x
-    def flush(self):
-        return ""
-
-compressors = {
-    None: nocompress,
-    # lambda to prevent early import
-    'BZ': lambda: bz2.BZ2Compressor(),
-    'GZ': lambda: zlib.compressobj(),
-    }
-# also support the old form by courtesies
-compressors['UN'] = compressors[None]
-
-def _makedecompressor(decompcls):
-    def generator(f):
-        d = decompcls()
-        for chunk in filechunkiter(f):
-            yield d.decompress(chunk)
-    def func(fh):
-        return chunkbuffer(generator(fh))
-    return func
-
 class ctxmanager(object):
     '''A context manager for use in 'with' blocks to allow multiple
     contexts to be entered at once.  This is both safer and more
     flexible than contextlib.nested.
 
     Once Mercurial supports Python 2.7+, this will become mostly
     unnecessary.
     '''
@@ -2875,16 +2849,42 @@  class ctxmanager(object):
             except BaseException:
                 pending = sys.exc_info()
                 exc_type, exc_val, exc_tb = pending = sys.exc_info()
         del self._atexit
         if pending:
             raise exc_val
         return received and suppressed
 
+# compression utility
+
+class nocompress(object):
+    def compress(self, x):
+        return x
+    def flush(self):
+        return ""
+
+compressors = {
+    None: nocompress,
+    # lambda to prevent early import
+    'BZ': lambda: bz2.BZ2Compressor(),
+    'GZ': lambda: zlib.compressobj(),
+    }
+# also support the old form by courtesies
+compressors['UN'] = compressors[None]
+
+def _makedecompressor(decompcls):
+    def generator(f):
+        d = decompcls()
+        for chunk in filechunkiter(f):
+            yield d.decompress(chunk)
+    def func(fh):
+        return chunkbuffer(generator(fh))
+    return func
+
 def _bz2():
     d = bz2.BZ2Decompressor()
     # Bzip2 stream start with BZ, but we stripped it.
     # we put it back for good measure.
     d.decompress('BZ')
     return d
 
 decompressors = {None: lambda fh: fh,