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
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)
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