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

[Pierre-Yves David]

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 at 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 at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

-- 
Pierre-Yves David