Patchwork [1,of,8] exchange: use early return

login
register
mail settings
Submitter Gregory Szorc
Date Aug. 5, 2016, 3:16 a.m.
Message ID <8a046444390debab5372.1470367017@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/16105/
State Accepted
Headers show

Comments

Gregory Szorc - Aug. 5, 2016, 3:16 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1469900681 25200
#      Sat Jul 30 10:44:41 2016 -0700
# Node ID 8a046444390debab5372de0b4f38863c57058f69
# Parent  5684bc429e6ab59fbb2c571509cb532c3a8b51cd
exchange: use early return

Avoid excessive indentation and confusing control flow by returning
early if there is no work to be done.
Pierre-Yves David - Aug. 6, 2016, 12:24 a.m.
On 08/05/2016 05:16 AM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1469900681 25200
> #      Sat Jul 30 10:44:41 2016 -0700
> # Node ID 8a046444390debab5372de0b4f38863c57058f69
> # Parent  5684bc429e6ab59fbb2c571509cb532c3a8b51cd
> exchange: use early return
>
> Avoid excessive indentation and confusing control flow by returning
> early if there is no work to be done.\

I'm not too convinced it is a win. Sure, the first one is clearer, but 
removing the second if introcude a return mid-function that I find much 
more confusing for control flow.

> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
> --- a/mercurial/exchange.py
> +++ b/mercurial/exchange.py
> @@ -1555,39 +1555,42 @@ def getbundle(repo, source, heads=None,
>               **kwargs)
>
>      return util.chunkbuffer(bundler.getchunks())
>
>  @getbundle2partsgenerator('changegroup')
>  def _getbundlechangegrouppart(bundler, repo, source, bundlecaps=None,
>                                b2caps=None, heads=None, common=None, **kwargs):
>      """add a changegroup part to the requested bundle"""
> -    cg = None
> -    if kwargs.get('cg', True):
> -        # build changegroup bundle here.
> -        version = '01'
> -        cgversions = b2caps.get('changegroup')
> -        if cgversions:  # 3.1 and 3.2 ship with an empty value
> -            cgversions = [v for v in cgversions
> -                          if v in changegroup.supportedoutgoingversions(repo)]
> -            if not cgversions:
> -                raise ValueError(_('no common changegroup version'))
> -            version = max(cgversions)
> -        outgoing = changegroup.computeoutgoing(repo, heads, common)
> -        cg = changegroup.getlocalchangegroupraw(repo, source, outgoing,
> -                                                bundlecaps=bundlecaps,
> -                                                version=version)
> +    if not kwargs.get('cg', True):
> +        return
>
> -    if cg:
> -        part = bundler.newpart('changegroup', data=cg)
> -        if cgversions:
> -            part.addparam('version', version)
> -        part.addparam('nbchanges', str(len(outgoing.missing)), mandatory=False)
> -        if 'treemanifest' in repo.requirements:
> -            part.addparam('treemanifest', '1')
> +    # build changegroup bundle here.
> +    version = '01'
> +    cgversions = b2caps.get('changegroup')
> +    if cgversions:  # 3.1 and 3.2 ship with an empty value
> +        cgversions = [v for v in cgversions
> +                      if v in changegroup.supportedoutgoingversions(repo)]
> +        if not cgversions:
> +            raise ValueError(_('no common changegroup version'))
> +        version = max(cgversions)
> +    outgoing = changegroup.computeoutgoing(repo, heads, common)
> +    cg = changegroup.getlocalchangegroupraw(repo, source, outgoing,
> +                                            bundlecaps=bundlecaps,
> +                                            version=version)
> +
> +    if not cg:
> +        return
> +
> +    part = bundler.newpart('changegroup', data=cg)
> +    if cgversions:
> +        part.addparam('version', version)
> +    part.addparam('nbchanges', str(len(outgoing.missing)), mandatory=False)
> +    if 'treemanifest' in repo.requirements:
> +        part.addparam('treemanifest', '1')
>
>  @getbundle2partsgenerator('listkeys')
>  def _getbundlelistkeysparts(bundler, repo, source, bundlecaps=None,
>                              b2caps=None, **kwargs):
>      """add parts containing listkeys namespaces to the requested bundle"""
>      listkeys = kwargs.get('listkeys', ())
>      for namespace in listkeys:
>          part = bundler.newpart('listkeys')
Augie Fackler - Aug. 6, 2016, 2:19 p.m.
> On Aug 5, 2016, at 8:24 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> 
> 
> On 08/05/2016 05:16 AM, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1469900681 25200
>> #      Sat Jul 30 10:44:41 2016 -0700
>> # Node ID 8a046444390debab5372de0b4f38863c57058f69
>> # Parent  5684bc429e6ab59fbb2c571509cb532c3a8b51cd
>> exchange: use early return
>> 
>> Avoid excessive indentation and confusing control flow by returning
>> early if there is no work to be done.\
> 
> I'm not too convinced it is a win. Sure, the first one is clearer, but removing the second if introcude a return mid-function that I find much more confusing for control flow.

I find this patch to be a huge readability win. Early returns are pretty common for cases like this in general.

> 
>> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
>> --- a/mercurial/exchange.py
>> +++ b/mercurial/exchange.py
>> @@ -1555,39 +1555,42 @@ def getbundle(repo, source, heads=None,
>>              **kwargs)
>> 
>>     return util.chunkbuffer(bundler.getchunks())
>> 
>> @getbundle2partsgenerator('changegroup')
>> def _getbundlechangegrouppart(bundler, repo, source, bundlecaps=None,
>>                               b2caps=None, heads=None, common=None, **kwargs):
>>     """add a changegroup part to the requested bundle"""
>> -    cg = None
>> -    if kwargs.get('cg', True):
>> -        # build changegroup bundle here.
>> -        version = '01'
>> -        cgversions = b2caps.get('changegroup')
>> -        if cgversions:  # 3.1 and 3.2 ship with an empty value
>> -            cgversions = [v for v in cgversions
>> -                          if v in changegroup.supportedoutgoingversions(repo)]
>> -            if not cgversions:
>> -                raise ValueError(_('no common changegroup version'))
>> -            version = max(cgversions)
>> -        outgoing = changegroup.computeoutgoing(repo, heads, common)
>> -        cg = changegroup.getlocalchangegroupraw(repo, source, outgoing,
>> -                                                bundlecaps=bundlecaps,
>> -                                                version=version)
>> +    if not kwargs.get('cg', True):
>> +        return
>> 
>> -    if cg:
>> -        part = bundler.newpart('changegroup', data=cg)
>> -        if cgversions:
>> -            part.addparam('version', version)
>> -        part.addparam('nbchanges', str(len(outgoing.missing)), mandatory=False)
>> -        if 'treemanifest' in repo.requirements:
>> -            part.addparam('treemanifest', '1')
>> +    # build changegroup bundle here.
>> +    version = '01'
>> +    cgversions = b2caps.get('changegroup')
>> +    if cgversions:  # 3.1 and 3.2 ship with an empty value
>> +        cgversions = [v for v in cgversions
>> +                      if v in changegroup.supportedoutgoingversions(repo)]
>> +        if not cgversions:
>> +            raise ValueError(_('no common changegroup version'))
>> +        version = max(cgversions)
>> +    outgoing = changegroup.computeoutgoing(repo, heads, common)
>> +    cg = changegroup.getlocalchangegroupraw(repo, source, outgoing,
>> +                                            bundlecaps=bundlecaps,
>> +                                            version=version)
>> +
>> +    if not cg:
>> +        return
>> +
>> +    part = bundler.newpart('changegroup', data=cg)
>> +    if cgversions:
>> +        part.addparam('version', version)
>> +    part.addparam('nbchanges', str(len(outgoing.missing)), mandatory=False)
>> +    if 'treemanifest' in repo.requirements:
>> +        part.addparam('treemanifest', '1')
>> 
>> @getbundle2partsgenerator('listkeys')
>> def _getbundlelistkeysparts(bundler, repo, source, bundlecaps=None,
>>                             b2caps=None, **kwargs):
>>     """add parts containing listkeys namespaces to the requested bundle"""
>>     listkeys = kwargs.get('listkeys', ())
>>     for namespace in listkeys:
>>         part = bundler.newpart('listkeys')
> 
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -1555,39 +1555,42 @@  def getbundle(repo, source, heads=None, 
              **kwargs)
 
     return util.chunkbuffer(bundler.getchunks())
 
 @getbundle2partsgenerator('changegroup')
 def _getbundlechangegrouppart(bundler, repo, source, bundlecaps=None,
                               b2caps=None, heads=None, common=None, **kwargs):
     """add a changegroup part to the requested bundle"""
-    cg = None
-    if kwargs.get('cg', True):
-        # build changegroup bundle here.
-        version = '01'
-        cgversions = b2caps.get('changegroup')
-        if cgversions:  # 3.1 and 3.2 ship with an empty value
-            cgversions = [v for v in cgversions
-                          if v in changegroup.supportedoutgoingversions(repo)]
-            if not cgversions:
-                raise ValueError(_('no common changegroup version'))
-            version = max(cgversions)
-        outgoing = changegroup.computeoutgoing(repo, heads, common)
-        cg = changegroup.getlocalchangegroupraw(repo, source, outgoing,
-                                                bundlecaps=bundlecaps,
-                                                version=version)
+    if not kwargs.get('cg', True):
+        return
 
-    if cg:
-        part = bundler.newpart('changegroup', data=cg)
-        if cgversions:
-            part.addparam('version', version)
-        part.addparam('nbchanges', str(len(outgoing.missing)), mandatory=False)
-        if 'treemanifest' in repo.requirements:
-            part.addparam('treemanifest', '1')
+    # build changegroup bundle here.
+    version = '01'
+    cgversions = b2caps.get('changegroup')
+    if cgversions:  # 3.1 and 3.2 ship with an empty value
+        cgversions = [v for v in cgversions
+                      if v in changegroup.supportedoutgoingversions(repo)]
+        if not cgversions:
+            raise ValueError(_('no common changegroup version'))
+        version = max(cgversions)
+    outgoing = changegroup.computeoutgoing(repo, heads, common)
+    cg = changegroup.getlocalchangegroupraw(repo, source, outgoing,
+                                            bundlecaps=bundlecaps,
+                                            version=version)
+
+    if not cg:
+        return
+
+    part = bundler.newpart('changegroup', data=cg)
+    if cgversions:
+        part.addparam('version', version)
+    part.addparam('nbchanges', str(len(outgoing.missing)), mandatory=False)
+    if 'treemanifest' in repo.requirements:
+        part.addparam('treemanifest', '1')
 
 @getbundle2partsgenerator('listkeys')
 def _getbundlelistkeysparts(bundler, repo, source, bundlecaps=None,
                             b2caps=None, **kwargs):
     """add parts containing listkeys namespaces to the requested bundle"""
     listkeys = kwargs.get('listkeys', ())
     for namespace in listkeys:
         part = bundler.newpart('listkeys')