Patchwork [7,of,7] tags: extract filenode filtering in it own function

# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1490675008 -7200
#      Tue Mar 28 06:23:28 2017 +0200
# Node ID dd6a04d26b611ac8d192f868daba734622e97528
# Parent  3d8a09214760799868b472b97e964e88f7ec8fd5
# 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 dd6a04d26b61
tags: extract filenode filtering in it own function

We'll also need to reuse this logic so we extract it in its own function. We
document some of the logic in the process.

On 3/28/17 7:17 AM, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1490675008 -7200
> #      Tue Mar 28 06:23:28 2017 +0200
> # Node ID dd6a04d26b611ac8d192f868daba734622e97528
> # Parent  3d8a09214760799868b472b97e964e88f7ec8fd5
> # EXP-Topic tags
> tags: extract filenode filtering in it own function
s/in it/into its (or to its)

>
> We'll also need to reuse this logic so we extract it in its own function. We
s/in its/into its (or just "to its")

> document some of the logic in the process.
>
> diff --git a/mercurial/tags.py b/mercurial/tags.py
> --- a/mercurial/tags.py
> +++ b/mercurial/tags.py
> @@ -95,17 +95,10 @@ def findglobaltags(ui, repo):
>           _updatetags(cachetags, alltags)
>           return alltags
>   
> -    seen = set()  # set of fnode
> -    fnodes = []
>       for head in reversed(heads):  # oldest to newest
>           assert head in repo.changelog.nodemap, \
>                  "tag cache returned bogus head %s" % short(head)
Unrelated musing: why is this an assert? Who cares if the tags cache was 
corrupted somehow? We should be more robust here. (Doesn't affect your 
series as this is unchanged).

> -
> -        fnode = tagfnode.get(head)
> -        if fnode and fnode not in seen:
> -            seen.add(fnode)
> -            fnodes.append(fnode)
> -
> +    fnodes = _filterfnodes(tagfnode, reversed(heads))
>       alltags = _tagsfromfnodes(ui, repo, fnodes)
>   
>       # and update the cache (if necessary)
> @@ -113,6 +106,20 @@ def findglobaltags(ui, repo):
>           _writetagcache(ui, repo, valid, alltags)
>       return alltags
>   
> +def _filterfnodes(tagfnode, nodes):
> +    """return a list of unique fnodes
> +
> +    The order of that list match the order "nodes". Preserving this order is
s/that/this
s/match/matches
s/order/order of

> +    important as reading tags in different order provides different results."""
s/order provides/orders can produce

> +    seen = set()  # set of fnode
> +    fnodes = []
> +    for no in nodes:  # oldest to newest
> +        fnode = tagfnode.get(no)
> +        if fnode and fnode not in seen:
> +            seen.add(fnode)
> +            fnodes.append(fnode)
> +    return fnodes
> +
>   def _tagsfromfnodes(ui, repo, fnodes):
>       """return a tagsmap from a list of file-node
>   
>

code changes lgtm

On 03/28/2017 12:18 PM, Ryan McElroy wrote:
> On 3/28/17 7:17 AM, Pierre-Yves David wrote:
[…]
>> diff --git a/mercurial/tags.py b/mercurial/tags.py
>> --- a/mercurial/tags.py
>> +++ b/mercurial/tags.py
>> @@ -95,17 +95,10 @@ def findglobaltags(ui, repo):
>>           _updatetags(cachetags, alltags)
>>           return alltags
>>   -    seen = set()  # set of fnode
>> -    fnodes = []
>>       for head in reversed(heads):  # oldest to newest
>>           assert head in repo.changelog.nodemap, \
>>                  "tag cache returned bogus head %s" % short(head)
> Unrelated musing: why is this an assert? Who cares if the tags cache was
> corrupted somehow? We should be more robust here. (Doesn't affect your
> series as this is unchanged).

Actually, if the tag cache is used we won't reach that point. This get 
used if the cache in invalid and we compute by hand. I don't think the 
assert is really need, I just preserved it from Greg this is must have 
been useful to catch error while editing the code.

>> -
>> -        fnode = tagfnode.get(head)
>> -        if fnode and fnode not in seen:
>> -            seen.add(fnode)
>> -            fnodes.append(fnode)
>> -
>> +    fnodes = _filterfnodes(tagfnode, reversed(heads))
>>       alltags = _tagsfromfnodes(ui, repo, fnodes)
>>         # and update the cache (if necessary)