Patchwork [4,of,7] tags: make argument 'tagtype' optional in '_updatetags'

login
register
mail settings
Submitter Pierre-Yves David
Date March 28, 2017, 6:16 a.m.
Message ID <e49ee337ec51b64e4405.1490681819@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/19767/
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 1490679550 -7200
#      Tue Mar 28 07:39:10 2017 +0200
# Node ID e49ee337ec51b64e440585d44e6c7df736164e98
# Parent  f0c93dd8d018c9f6828c97be8ccb80dbfca694b8
# 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 e49ee337ec51
tags: make argument 'tagtype' optional in '_updatetags'

This is the next step from the previous changesets, we are now ready to use this
function in a simpler way.
Ryan McElroy - March 28, 2017, 9:58 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 1490679550 -7200
> #      Tue Mar 28 07:39:10 2017 +0200
> # Node ID e49ee337ec51b64e440585d44e6c7df736164e98
> # Parent  f0c93dd8d018c9f6828c97be8ccb80dbfca694b8
> # EXP-Topic tags
> tags: make argument 'tagtype' optional in '_updatetags'
>
> This is the next step from the previous changesets, we are now ready to use this
> function in a simpler way.
>
> diff --git a/mercurial/tags.py b/mercurial/tags.py
> --- a/mercurial/tags.py
> +++ b/mercurial/tags.py
> @@ -223,15 +223,20 @@ def _readtags(ui, repo, lines, fn, recod
>           newtags[tag] = (taghist[-1], taghist[:-1])
>       return newtags
>   
> -def _updatetags(filetags, alltags, tagtype, tagtypes):
> +def _updatetags(filetags, alltags, tagtype=None, tagtypes=None):
>       '''Incorporate the tag info read from one file into the two
>       dictionaries, alltags and tagtypes, that contain all tag
> -    info (global across all heads plus local).'''
> +    info (global across all heads plus local).
> +
> +    The second dictionnary is optional.'''

The last two parameters are optional now, and it's an error to set 
tagtypes and not tagtype. I think this message can be updated to be more 
clear about how to use this in various circumstances.

> +    if tagtype is None:
> +        assert tagtypes is None

Is it more appropriate these days to raise an error.ProgrammingError()?

>   
>       for name, nodehist in filetags.iteritems():
>           if name not in alltags:
>               alltags[name] = nodehist
> -            tagtypes[name] = tagtype
> +            if tagtype is not None:
> +                tagtypes[name] = tagtype
>               continue
>   
>           # we prefer alltags[name] if:
> @@ -243,7 +248,7 @@ def _updatetags(filetags, alltags, tagty
>           if (bnode != anode and anode in bhist and
>               (bnode not in ahist or len(bhist) > len(ahist))):
>               anode = bnode
> -        else:
> +        elif tagtype is not None:
>               tagtypes[name] = tagtype
>           ahist.extend([n for n in bhist if n not in ahist])
>           alltags[name] = anode, ahist
>
Pierre-Yves David - March 28, 2017, 11:45 a.m.
On 03/28/2017 11:58 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 1490679550 -7200
>> #      Tue Mar 28 07:39:10 2017 +0200
>> # Node ID e49ee337ec51b64e440585d44e6c7df736164e98
>> # Parent  f0c93dd8d018c9f6828c97be8ccb80dbfca694b8
>> # EXP-Topic tags
>> tags: make argument 'tagtype' optional in '_updatetags'
>>
>> This is the next step from the previous changesets, we are now ready
>> to use this
>> function in a simpler way.
>>
>> diff --git a/mercurial/tags.py b/mercurial/tags.py
>> --- a/mercurial/tags.py
>> +++ b/mercurial/tags.py
>> @@ -223,15 +223,20 @@ def _readtags(ui, repo, lines, fn, recod
>>           newtags[tag] = (taghist[-1], taghist[:-1])
>>       return newtags
>>   -def _updatetags(filetags, alltags, tagtype, tagtypes):
>> +def _updatetags(filetags, alltags, tagtype=None, tagtypes=None):
>>       '''Incorporate the tag info read from one file into the two
>>       dictionaries, alltags and tagtypes, that contain all tag
>> -    info (global across all heads plus local).'''
>> +    info (global across all heads plus local).
>> +
>> +    The second dictionnary is optional.'''
>
> The last two parameters are optional now, and it's an error to set
> tagtypes and not tagtype. I think this message can be updated to be more
> clear about how to use this in various circumstances.

I've updated it to:

     """Incorporate the tag info read from one file into dictionnaries

     The first one, 'alltags', is a "tagmaps" (see 'findglobaltags' for 
details).

     The second one, 'tagtypes', is optional and will be updated to 
track the "tagtype" of entries
     in the tagmaps. When set, the 'tagtype' argument also needs to be 
set."""


>
>> +    if tagtype is None:
>> +        assert tagtypes is None
>
> Is it more appropriate these days to raise an error.ProgrammingError()?

Not really. I see ProgrammingError as something useful widely used API 
that have a change to get missused. This assert is mostly to catch early 
a silly error from someone hacking this file.

>
>>         for name, nodehist in filetags.iteritems():
>>           if name not in alltags:
>>               alltags[name] = nodehist
>> -            tagtypes[name] = tagtype
>> +            if tagtype is not None:
>> +                tagtypes[name] = tagtype
>>               continue
>>             # we prefer alltags[name] if:
>> @@ -243,7 +248,7 @@ def _updatetags(filetags, alltags, tagty
>>           if (bnode != anode and anode in bhist and
>>               (bnode not in ahist or len(bhist) > len(ahist))):
>>               anode = bnode
>> -        else:
>> +        elif tagtype is not None:
>>               tagtypes[name] = tagtype
>>           ahist.extend([n for n in bhist if n not in ahist])
>>           alltags[name] = anode, ahist
>>
>

Patch

diff --git a/mercurial/tags.py b/mercurial/tags.py
--- a/mercurial/tags.py
+++ b/mercurial/tags.py
@@ -223,15 +223,20 @@  def _readtags(ui, repo, lines, fn, recod
         newtags[tag] = (taghist[-1], taghist[:-1])
     return newtags
 
-def _updatetags(filetags, alltags, tagtype, tagtypes):
+def _updatetags(filetags, alltags, tagtype=None, tagtypes=None):
     '''Incorporate the tag info read from one file into the two
     dictionaries, alltags and tagtypes, that contain all tag
-    info (global across all heads plus local).'''
+    info (global across all heads plus local).
+
+    The second dictionnary is optional.'''
+    if tagtype is None:
+        assert tagtypes is None
 
     for name, nodehist in filetags.iteritems():
         if name not in alltags:
             alltags[name] = nodehist
-            tagtypes[name] = tagtype
+            if tagtype is not None:
+                tagtypes[name] = tagtype
             continue
 
         # we prefer alltags[name] if:
@@ -243,7 +248,7 @@  def _updatetags(filetags, alltags, tagty
         if (bnode != anode and anode in bhist and
             (bnode not in ahist or len(bhist) > len(ahist))):
             anode = bnode
-        else:
+        elif tagtype is not None:
             tagtypes[name] = tagtype
         ahist.extend([n for n in bhist if n not in ahist])
         alltags[name] = anode, ahist