Patchwork [2,of,7] tags: do not feed dictionaries to 'findglobaltags'

login
register
mail settings
Submitter Pierre-Yves David
Date March 28, 2017, 6:16 a.m.
Message ID <147b98bfa4afbaf608d9.1490681817@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/19770/
State Changes Requested
Headers show

Comments

Pierre-Yves David - March 28, 2017, 6:16 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1490674429 -7200
#      Tue Mar 28 06:13:49 2017 +0200
# Node ID 147b98bfa4afbaf608d9e1f5227a48a46e386ea4
# Parent  1e77e505e7bacf59d1200714dc92e827523d7301
# EXP-Topic tags
# Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
#              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r 147b98bfa4af
tags: do not feed dictionaries to 'findglobaltags'

The code assert that these dictionnary are empty anyway. So we can as well be
more explicit and have the function returns the dictionaries directly.
Ryan McElroy - March 28, 2017, 9:51 a.m.
On 3/28/17 7:16 AM, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1490674429 -7200
> #      Tue Mar 28 06:13:49 2017 +0200
> # Node ID 147b98bfa4afbaf608d9e1f5227a48a46e386ea4
> # Parent  1e77e505e7bacf59d1200714dc92e827523d7301
> # EXP-Topic tags
> tags: do not feed dictionaries to 'findglobaltags'
>
> The code assert that these dictionnary are empty anyway. So we can as well be

s/assert/asserts
nit: drop unnecessary word "anyway"
s/dictionnary/dictionary
nit: drop unnecessary phrase "as well"

> more explicit and have the function returns the dictionaries directly.

s/returns/return

>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -707,10 +707,11 @@ class localrepository(object):
>           # be one tagtype for all such "virtual" tags?  Or is the status
>           # quo fine?
>   
> -        alltags = {}                    # map tag name to (node, hist)
> -        tagtypes = {}
>   
> -        tagsmod.findglobaltags(self.ui, self, alltags, tagtypes)
> +        globaldata = tagsmod.findglobaltags(self.ui, self)
> +        alltags = globaldata[0]   # map tag name to (node, hist)
> +        tagtypes = globaldata[1]  # map tag name to tag type

Why go via the "globaldata" intermediate and not just assign these via 
tuple expansion?
If you want to preserve the comments, but comment above the line.

> +
>           tagsmod.readlocaltags(self.ui, self, alltags, tagtypes)
>   
>           # Build the return dicts.  Have to re-encode tag names because
> diff --git a/mercurial/tags.py b/mercurial/tags.py
> --- a/mercurial/tags.py
> +++ b/mercurial/tags.py
> @@ -78,23 +78,18 @@ from . import (
>   # The most recent changeset (in terms of revlog ordering for the head
>   # setting it) for each tag is last.
>   
> -def findglobaltags(ui, repo, alltags, tagtypes):
> -    '''Find global tags in a repo.
> +def findglobaltags(ui, repo):
> +    '''Find global tags in a repo: return (alltags, tagtypes)
>   
>       "alltags" maps tag name to (node, hist) 2-tuples.
>   
>       "tagtypes" maps tag name to tag type. Global tags always have the
>       "global" tag type.
>   
> -    The "alltags" and "tagtypes" dicts are updated in place. Empty dicts
> -    should be passed in.
> -
>       The tags cache is read and updated as a side-effect of calling.
>       '''
> -    # This is so we can be lazy and assume alltags contains only global
> -    # tags when we pass it to _writetagcache().
> -    assert len(alltags) == len(tagtypes) == 0, \
> -           "findglobaltags() should be called first"
> +    alltags = {}
> +    tagtypes = {}
>   
>       (heads, tagfnode, valid, cachetags, shouldwrite) = _readtagcache(ui, repo)
>       if cachetags is not None:
> @@ -103,7 +98,7 @@ def findglobaltags(ui, repo, alltags, ta
>           # cases where a global tag should outrank a local tag but won't,
>           # because cachetags does not contain rank info?
>           _updatetags(cachetags, 'global', alltags, tagtypes)
> -        return
> +        return alltags, tagtypes
>   
>       seen = set()  # set of fnode
>       fctx = None
> @@ -125,6 +120,7 @@ def findglobaltags(ui, repo, alltags, ta
>       # and update the cache (if necessary)
>       if shouldwrite:
>           _writetagcache(ui, repo, valid, alltags)
> +    return alltags, tagtypes
>   
>   def readlocaltags(ui, repo, alltags, tagtypes):
>       '''Read local tags in repo. Update alltags and tagtypes.'''
>

Overall, this looks like a good cleanup
Pierre-Yves David - March 28, 2017, 11:31 a.m.
On 03/28/2017 11:51 AM, Ryan McElroy wrote:
> On 3/28/17 7:16 AM, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1490674429 -7200
>> #      Tue Mar 28 06:13:49 2017 +0200
>> # Node ID 147b98bfa4afbaf608d9e1f5227a48a46e386ea4
>> # Parent  1e77e505e7bacf59d1200714dc92e827523d7301
>> # EXP-Topic tags
>> tags: do not feed dictionaries to 'findglobaltags'
>>
>> The code assert that these dictionnary are empty anyway. So we can as
>> well be
>
> s/assert/asserts
> nit: drop unnecessary word "anyway"
> s/dictionnary/dictionary
> nit: drop unnecessary phrase "as well"
>
>> more explicit and have the function returns the dictionaries directly.
>
> s/returns/return

Thanks.

>
>>
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> @@ -707,10 +707,11 @@ class localrepository(object):
>>           # be one tagtype for all such "virtual" tags?  Or is the status
>>           # quo fine?
>>   -        alltags = {}                    # map tag name to (node, hist)
>> -        tagtypes = {}
>>   -        tagsmod.findglobaltags(self.ui, self, alltags, tagtypes)
>> +        globaldata = tagsmod.findglobaltags(self.ui, self)
>> +        alltags = globaldata[0]   # map tag name to (node, hist)
>> +        tagtypes = globaldata[1]  # map tag name to tag type
>
> Why go via the "globaldata" intermediate and not just assign these via
> tuple expansion?
> If you want to preserve the comments, but comment above the line.

Yep, to preserve the comment. This got rewritten in a couple changeset 
further so I don't thinks this is worth updating.

>
>> +
>>           tagsmod.readlocaltags(self.ui, self, alltags, tagtypes)
>>             # Build the return dicts.  Have to re-encode tag names
>> because
>> diff --git a/mercurial/tags.py b/mercurial/tags.py
>> --- a/mercurial/tags.py
>> +++ b/mercurial/tags.py
>> @@ -78,23 +78,18 @@ from . import (
>>   # The most recent changeset (in terms of revlog ordering for the head
>>   # setting it) for each tag is last.
>>   -def findglobaltags(ui, repo, alltags, tagtypes):
>> -    '''Find global tags in a repo.
>> +def findglobaltags(ui, repo):
>> +    '''Find global tags in a repo: return (alltags, tagtypes)
>>         "alltags" maps tag name to (node, hist) 2-tuples.
>>         "tagtypes" maps tag name to tag type. Global tags always have the
>>       "global" tag type.
>>   -    The "alltags" and "tagtypes" dicts are updated in place. Empty
>> dicts
>> -    should be passed in.
>> -
>>       The tags cache is read and updated as a side-effect of calling.
>>       '''
>> -    # This is so we can be lazy and assume alltags contains only global
>> -    # tags when we pass it to _writetagcache().
>> -    assert len(alltags) == len(tagtypes) == 0, \
>> -           "findglobaltags() should be called first"
>> +    alltags = {}
>> +    tagtypes = {}
>>         (heads, tagfnode, valid, cachetags, shouldwrite) =
>> _readtagcache(ui, repo)
>>       if cachetags is not None:
>> @@ -103,7 +98,7 @@ def findglobaltags(ui, repo, alltags, ta
>>           # cases where a global tag should outrank a local tag but
>> won't,
>>           # because cachetags does not contain rank info?
>>           _updatetags(cachetags, 'global', alltags, tagtypes)
>> -        return
>> +        return alltags, tagtypes
>>         seen = set()  # set of fnode
>>       fctx = None
>> @@ -125,6 +120,7 @@ def findglobaltags(ui, repo, alltags, ta
>>       # and update the cache (if necessary)
>>       if shouldwrite:
>>           _writetagcache(ui, repo, valid, alltags)
>> +    return alltags, tagtypes
>>     def readlocaltags(ui, repo, alltags, tagtypes):
>>       '''Read local tags in repo. Update alltags and tagtypes.'''
>>
>
> Overall, this looks like a good cleanup
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -707,10 +707,11 @@  class localrepository(object):
         # be one tagtype for all such "virtual" tags?  Or is the status
         # quo fine?
 
-        alltags = {}                    # map tag name to (node, hist)
-        tagtypes = {}
 
-        tagsmod.findglobaltags(self.ui, self, alltags, tagtypes)
+        globaldata = tagsmod.findglobaltags(self.ui, self)
+        alltags = globaldata[0]   # map tag name to (node, hist)
+        tagtypes = globaldata[1]  # map tag name to tag type
+
         tagsmod.readlocaltags(self.ui, self, alltags, tagtypes)
 
         # Build the return dicts.  Have to re-encode tag names because
diff --git a/mercurial/tags.py b/mercurial/tags.py
--- a/mercurial/tags.py
+++ b/mercurial/tags.py
@@ -78,23 +78,18 @@  from . import (
 # The most recent changeset (in terms of revlog ordering for the head
 # setting it) for each tag is last.
 
-def findglobaltags(ui, repo, alltags, tagtypes):
-    '''Find global tags in a repo.
+def findglobaltags(ui, repo):
+    '''Find global tags in a repo: return (alltags, tagtypes)
 
     "alltags" maps tag name to (node, hist) 2-tuples.
 
     "tagtypes" maps tag name to tag type. Global tags always have the
     "global" tag type.
 
-    The "alltags" and "tagtypes" dicts are updated in place. Empty dicts
-    should be passed in.
-
     The tags cache is read and updated as a side-effect of calling.
     '''
-    # This is so we can be lazy and assume alltags contains only global
-    # tags when we pass it to _writetagcache().
-    assert len(alltags) == len(tagtypes) == 0, \
-           "findglobaltags() should be called first"
+    alltags = {}
+    tagtypes = {}
 
     (heads, tagfnode, valid, cachetags, shouldwrite) = _readtagcache(ui, repo)
     if cachetags is not None:
@@ -103,7 +98,7 @@  def findglobaltags(ui, repo, alltags, ta
         # cases where a global tag should outrank a local tag but won't,
         # because cachetags does not contain rank info?
         _updatetags(cachetags, 'global', alltags, tagtypes)
-        return
+        return alltags, tagtypes
 
     seen = set()  # set of fnode
     fctx = None
@@ -125,6 +120,7 @@  def findglobaltags(ui, repo, alltags, ta
     # and update the cache (if necessary)
     if shouldwrite:
         _writetagcache(ui, repo, valid, alltags)
+    return alltags, tagtypes
 
 def readlocaltags(ui, repo, alltags, tagtypes):
     '''Read local tags in repo. Update alltags and tagtypes.'''