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

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

Comments

Pierre-Yves David - March 28, 2017, 6:17 a.m.
# 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.
Ryan McElroy - March 28, 2017, 10:18 a.m.
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
Pierre-Yves David - March 28, 2017, 11:53 a.m.
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)

Patch

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)
-
-        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
+    important as reading tags in different order provides different results."""
+    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