Patchwork [1,of,7] tags: extract fnode retrieval in its own function

login
register
mail settings
Submitter Pierre-Yves David
Date March 28, 2017, 6:16 a.m.
Message ID <1e77e505e7bacf59d120.1490681816@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/19765/
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 1490673691 -7200
#      Tue Mar 28 06:01:31 2017 +0200
# Node ID 1e77e505e7bacf59d1200714dc92e827523d7301
# Parent  e6fd7930cf0b37710e379de37b7d87d5c1ea5dc9
# 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 1e77e505e7ba
tags: extract fnode retrieval in its own function

My main goal here is to be able to reuse this logic easily. As a side effect
this important logic is now insulated and the code is clearer.
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 1490673691 -7200
> #      Tue Mar 28 06:01:31 2017 +0200
> # Node ID 1e77e505e7bacf59d1200714dc92e827523d7301
> # Parent  e6fd7930cf0b37710e379de37b7d87d5c1ea5dc9
> # EXP-Topic tags
> tags: extract fnode retrieval in its own function
>
> My main goal here is to be able to reuse this logic easily. As a side effect
> this important logic is now insulated and the code is clearer.
>
> diff --git a/mercurial/tags.py b/mercurial/tags.py
> --- a/mercurial/tags.py
> +++ b/mercurial/tags.py
> @@ -341,15 +341,27 @@ def _readtagcache(ui, repo):
>           # potentially expensive search.
>           return ([], {}, valid, None, True)
>   
> -    starttime = util.timer()
>   
>       # Now we have to lookup the .hgtags filenode for every new head.
>       # This is the most expensive part of finding tags, so performance
>       # depends primarily on the size of newheads.  Worst case: no cache
>       # file, so newheads == repoheads.
> +    cachefnode = _getfnodes(ui, repo, repoheads)
> +
> +    # Caller has to iterate over all heads, but can use the filenodes in
> +    # cachefnode to get to each .hgtags revision quickly.
> +    return (repoheads, cachefnode, valid, None, True)
> +
> +def _getfnodes(ui, repo, nodes):
> +    """return fnode for .hgtags in a list of node

This comment doesn't quite parse for me. Maybe it should be "return list 
of .hgtags fnodes for tags computation"

> +
> +    return value is a {node: fnode} mapping. their will be no entry for node

s/their/there
s/node/nodes (at end of line)

Capitalization nitpicks: Return and There should be capitalized I think.

> +    without a '.hgtags' file.
> +    """
> +    starttime = util.timer()
>       fnodescache = hgtagsfnodescache(repo.unfiltered())
>       cachefnode = {}
> -    for head in reversed(repoheads):
> +    for head in reversed(nodes):
>           fnode = fnodescache.getfnode(head)
>           if fnode != nullid:
>               cachefnode[head] = fnode
> @@ -361,10 +373,7 @@ def _readtagcache(ui, repo):
>              '%d/%d cache hits/lookups in %0.4f '
>              'seconds\n',
>              fnodescache.hitcount, fnodescache.lookupcount, duration)
> -
> -    # Caller has to iterate over all heads, but can use the filenodes in
> -    # cachefnode to get to each .hgtags revision quickly.
> -    return (repoheads, cachefnode, valid, None, True)
> +    return cachefnode
>   
>   def _writetagcache(ui, repo, valid, cachetags):
>       filename = _filename(repo)
>
Ryan McElroy - March 28, 2017, 10:19 a.m.
Overall series mostly looks good to me, but there are enough grammar 
changes and a few questions to be addressed that I think a v2 would be 
worth it. I'll mark as changes requested in patchwork for now.


On 3/28/17 10: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 1490673691 -7200
>> #      Tue Mar 28 06:01:31 2017 +0200
>> # Node ID 1e77e505e7bacf59d1200714dc92e827523d7301
>> # Parent  e6fd7930cf0b37710e379de37b7d87d5c1ea5dc9
>> # EXP-Topic tags
>> tags: extract fnode retrieval in its own function
>>
>> My main goal here is to be able to reuse this logic easily. As a side 
>> effect
>> this important logic is now insulated and the code is clearer.
>>
>> diff --git a/mercurial/tags.py b/mercurial/tags.py
>> --- a/mercurial/tags.py
>> +++ b/mercurial/tags.py
>> @@ -341,15 +341,27 @@ def _readtagcache(ui, repo):
>>           # potentially expensive search.
>>           return ([], {}, valid, None, True)
>>   -    starttime = util.timer()
>>         # Now we have to lookup the .hgtags filenode for every new head.
>>       # This is the most expensive part of finding tags, so performance
>>       # depends primarily on the size of newheads.  Worst case: no cache
>>       # file, so newheads == repoheads.
>> +    cachefnode = _getfnodes(ui, repo, repoheads)
>> +
>> +    # Caller has to iterate over all heads, but can use the 
>> filenodes in
>> +    # cachefnode to get to each .hgtags revision quickly.
>> +    return (repoheads, cachefnode, valid, None, True)
>> +
>> +def _getfnodes(ui, repo, nodes):
>> +    """return fnode for .hgtags in a list of node
>
> This comment doesn't quite parse for me. Maybe it should be "return 
> list of .hgtags fnodes for tags computation"
>
>> +
>> +    return value is a {node: fnode} mapping. their will be no entry 
>> for node
>
> s/their/there
> s/node/nodes (at end of line)
>
> Capitalization nitpicks: Return and There should be capitalized I think.
>
>> +    without a '.hgtags' file.
>> +    """
>> +    starttime = util.timer()
>>       fnodescache = hgtagsfnodescache(repo.unfiltered())
>>       cachefnode = {}
>> -    for head in reversed(repoheads):
>> +    for head in reversed(nodes):
>>           fnode = fnodescache.getfnode(head)
>>           if fnode != nullid:
>>               cachefnode[head] = fnode
>> @@ -361,10 +373,7 @@ def _readtagcache(ui, repo):
>>              '%d/%d cache hits/lookups in %0.4f '
>>              'seconds\n',
>>              fnodescache.hitcount, fnodescache.lookupcount, duration)
>> -
>> -    # Caller has to iterate over all heads, but can use the 
>> filenodes in
>> -    # cachefnode to get to each .hgtags revision quickly.
>> -    return (repoheads, cachefnode, valid, None, True)
>> +    return cachefnode
>>     def _writetagcache(ui, repo, valid, cachetags):
>>       filename = _filename(repo)
>>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=Jw8rundaE7TbmqBYd1txIQ&m=kqVn49crFvd30D__LN7H59AJI5PDtBc_OGhiStGsD0U&s=9YhAyi6V_q5F8YZCOQnIl1yhJlH-jL6EuHDuFfl4-7Y&e=
Pierre-Yves David - March 28, 2017, 11:29 a.m.
On 03/28/2017 11:51 AM, Ryan McElroy wrote:
> On 3/28/17 7:16 AM, Pierre-Yves David wrote:
[…]
>> +def _getfnodes(ui, repo, nodes):
>> +    """return fnode for .hgtags in a list of node
>
> This comment doesn't quite parse for me. Maybe it should be "return list
> of .hgtags fnodes for tags computation"

I rewrote this as:

   return .hgtags fnode for a list of changeset node

Cheers,
Ryan McElroy - March 28, 2017, 11:37 a.m.
On 3/28/17 12:29 PM, Pierre-Yves David wrote:
>
>
> On 03/28/2017 11:51 AM, Ryan McElroy wrote:
>> On 3/28/17 7:16 AM, Pierre-Yves David wrote:
> […]
>>> +def _getfnodes(ui, repo, nodes):
>>> +    """return fnode for .hgtags in a list of node
>>
>> This comment doesn't quite parse for me. Maybe it should be "return list
>> of .hgtags fnodes for tags computation"
>
> I rewrote this as:
>
>   return .hgtags fnode for a list of changeset node

I would add an "s" to both "fnode" and "node" so it reads:

   return .hgtags fnodes for a list of changeset nodes

Patch

diff --git a/mercurial/tags.py b/mercurial/tags.py
--- a/mercurial/tags.py
+++ b/mercurial/tags.py
@@ -341,15 +341,27 @@  def _readtagcache(ui, repo):
         # potentially expensive search.
         return ([], {}, valid, None, True)
 
-    starttime = util.timer()
 
     # Now we have to lookup the .hgtags filenode for every new head.
     # This is the most expensive part of finding tags, so performance
     # depends primarily on the size of newheads.  Worst case: no cache
     # file, so newheads == repoheads.
+    cachefnode = _getfnodes(ui, repo, repoheads)
+
+    # Caller has to iterate over all heads, but can use the filenodes in
+    # cachefnode to get to each .hgtags revision quickly.
+    return (repoheads, cachefnode, valid, None, True)
+
+def _getfnodes(ui, repo, nodes):
+    """return fnode for .hgtags in a list of node
+
+    return value is a {node: fnode} mapping. their will be no entry for node
+    without a '.hgtags' file.
+    """
+    starttime = util.timer()
     fnodescache = hgtagsfnodescache(repo.unfiltered())
     cachefnode = {}
-    for head in reversed(repoheads):
+    for head in reversed(nodes):
         fnode = fnodescache.getfnode(head)
         if fnode != nullid:
             cachefnode[head] = fnode
@@ -361,10 +373,7 @@  def _readtagcache(ui, repo):
            '%d/%d cache hits/lookups in %0.4f '
            'seconds\n',
            fnodescache.hitcount, fnodescache.lookupcount, duration)
-
-    # Caller has to iterate over all heads, but can use the filenodes in
-    # cachefnode to get to each .hgtags revision quickly.
-    return (repoheads, cachefnode, valid, None, True)
+    return cachefnode
 
 def _writetagcache(ui, repo, valid, cachetags):
     filename = _filename(repo)