[PATCH] Re: 2 bugs in tag removal
Matt Mackall
mpm at selenic.com
Sun Dec 9 06:33:32 UTC 2007
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.
More information about the Mercurial-devel
mailing list