[PATCH] Re: 2 bugs in tag removal

[Matt Mackall]

On Sun, Dec 09, 2007 at 10:24:31AM +0900, Osku Salerma wrote:
> On Sat, 8 Dec 2007, Matt Mackall wrote:
> 
> >On Sat, Dec 08, 2007 at 06:02:38PM +0900, Osku Salerma wrote:
> 
> >>+    def gettagtype(self, tagname):
> >>+        '''
> >>+        return the type of the given tag. result can be:
> >>+
> >>+        'local'  : a local tag
> >>+        'global' : a global tag
> >>+        None     : tag does not exist
> >>+        '''
> >>+
> >>+        self.tags()
> >>+
> >>+        return self.tagstypecache.get(tagname)
> >
> >Do we actually need this function? Can't the one user of tagtypes just
> >use the dict directly?
> 
> That place would then have to remember to have a useless-looking call to
> localrepo.tags() first to make sure the cache is initialized, which
> doesn't seem like a good idea. And we have one place now but I'm pretty
> sure we'll end up with more in the future (for example, I'd like "hg tags"
> to tell me which of the tags are local...).

That second point is good. Perhaps with a -v flag.

> New patch:
> 
> # HG changeset patch
> # User Osku Salerma <osku at iki.fi>
> # Date 1197163199 -32400
> # Node ID 671665f4161d69fa62d46967ea57c3159dcf5354
> # Parent  feac5b0bf9bad2c125ebd5f3e133bcd46ecb8c7c
> Properly check tag's existence as a local/global tag when removing it.
> 
> diff -r feac5b0bf9ba -r 671665f4161d mercurial/commands.py
> --- a/mercurial/commands.py	Wed Nov 28 13:58:31 2007 -0800
> +++ b/mercurial/commands.py	Sun Dec 09 10:19:59 2007 +0900
> @@ -2628,8 +2628,13 @@ def tag(ui, repo, name, rev_=None, **opt
>          rev_ = opts['rev']
>      message = opts['message']
>      if opts['remove']:
> -        if not name in repo.tags():
> -            raise util.Abort(_('tag %s does not exist') % name)
> +        tagtype = repo.gettagtype(name)

If we're going to keep this function, let's drop the "get".

> +        found = ((opts['local'] and tagtype == 'local') or
> +                 (not opts['local'] and tagtype == 'global'))
> +
> +        if not found:
> +            raise util.Abort(_('%s tag %s does not exist')
> +                             % (opts['local'] and 'local' or 'global', 
> name))

How about:

if opts['local'] and tagtype == 'global':
   raise util.Abort(_('%s tag is global) % name)
if not opts['local'] and tagtype == 'local':
   raise util.Abort(_('%s tag is local) % name)

..which is a better message and less code.

Otherwise, looks good.

-- 
Mathematics is the supreme nostalgia of our time.