[PATCH] templates: add {lasttag} and {lasttagdistance} keywords

Mads Kiilerich mads at kiilerich.com
Tue Jul 14 20:02:33 CDT 2009


Some further comments after having looked more at your nice patch.

Gilles Moris wrote, On 07/14/2009 06:45 PM:
>   mercurial/cmdutil.py |  40 ++++++++++++++++++++++++++++++++++++++++
>   mercurial/help.py    |   3 +++
>   tests/test-log       |  44 ++++++++++++++++++++++++++++++++++++++++++++
>   tests/test-log.out   |  50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 137 insertions(+), 0 deletions(-)
>
>
> # HG changeset patch
> # User Gilles Moris<gilles.moris at free.fr>
> # Date 1247087563 -7200
> # Node ID bea99b6921ea2a76921b765cfa5fe4a9f5ffe6e1
> # Parent  e1dacc2c0aaee8bfa4a1749ed504b3afed119adb
> templates: add {lasttag} and {lasttagdistance} keywords.
>
> Useful to build nightly version names and see on which tag a rev is based on.
>
> Among tag ancestors, the one with the latest date, or with the longuest path if
>    

longest?

> there are multiple pathes to the same tag, is chosen.
> This way, lasttag does not depend on the push/pull order and matches the
> user's expectation, in particular in the case of a backout merge.
> Only global tags are considered. The local tags (including 'tip' and automatic
> MQ tags) are discarded.
>
> Both lasttag and lasttagdistance are defined as String, so that they default to
> an empty string if no tag is found.
>    

They are strings, but I wouldn't say that they are "defined". But being 
string doesn't cause it to be empty - it just allows it. So not "so 
that" but just an "an". But as discussed below I would rather never get 
an empty string ...

> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -741,6 +741,35 @@
>                                            'manifest': '{rev}:{node|formatnode}',
>                                            'filecopy': '{name} ({source})'})
>
> +    @util.propertycache
> +    def lasttagmap(self):
> +        '''get an array[rev] of latest tag tuple, each tuple being
> +        (date of the latest tag, the [first] tag, distance to that tag).
> +        Exclude "tip" and MQ tags from the search.
>    

Wouldn't it be better to say what it is instead of what it isn't? It is 
the latest global tag, and here we don't even know mq enough to mention it.

> +        '''
> +        defvalue = 0, '', -1
>    

For revisions without any tagged ancestors I would rather have the 
distance to the first revision than nothing. Using the "null" revision 
would be an obvious choice:

         defvalue = 0, 'null', 0

> +        ltmap = [defvalue] * len(self.repo)
> +
> +        for r in range(len(self.repo)):
> +            ctx = self.repo[r]
> +
> +            tags = [t for t in ctx.tags() if self.repo.tagtype(t) == 'global']
> +            if tags:
> +                ltmap[r] = ctx.date(), tags[0], 0
>    

Is the ordering of multiple tags for a revision well-defined? Is it a 
good choice to use the first?

Perhaps
     '-'.join(sorted(tags))

> +            else:
> +                # trick: use tuple comparison to sort by latest date first
> +                # and if the dates (and by construction also the tag) are
> +                # equal, by longuest path.
>    

I think it is fine to be clever. If it so clever that we have to explain 
it then it is less fine. (Not that I am against comments!) But if it is 
so dirty that we call it a trick then we shouldn't use it.

It took me some time to understand the comment. Especially the use of 
"sort" and "by construction" and that it referred to the _tag_ date 
tricked me.

Perhaps
                 # the last tag of this rev comes from the parent with 
the latest
                 # tag (secondarily the alphabetically latest tag name 
and the
                 # longest distance to the tag), achieved by comparing 
the tuples.

> +                lasttag = reduce(max,
> +                                 [ltmap[p.rev()] for p in ctx.parents()],
> +                                 defvalue)
>    

This looks unnecessary clever and hard to read. Especially because there 
always are 1 or 2 parents.

And be aware that p.rev() will be -1 for "null" and it will thus look at 
the wrong place in ltmap. It works anyway, but if it is intentional then 
it deserves a comment.

So the same can be achieved with
                 ldate, ltag, ldist = max(ltmap[p.rev()] for p in 
ctx.parents())


Or slightly more verbose, but without wrong indexing and without relying 
on initialization of ltmap:
                 ldate, ltag, ldist = max(
                     p.rev() >= 0 and ltmap[p.rev()] or (0, 'null', 0)
                     for p in ctx.parents())

> +                ldate, ltag, ldist = lasttag
> +                # if no previous tag, nothing to do (leave default value)
>    
> +                if ltag:
>    

Why shouldn't we give useful distances for revisions which just happens 
to not have other tags to count from than "null"? However, with "null" 
as default value as suggested above this is redundant.

> +                    ltmap[r] = ldate, ltag, ldist + 1
> +
> +        return ltmap
> +
>       def use_template(self, t):
>           '''set template string to use'''
>           self.t.cache['changeset'] = t
> @@ -874,6 +903,15 @@
>                   removes += i[2]
>               return '%s: +%s/-%s' % (files, adds, removes)
>
> +        def showlasttag(**args):
> +            return self.lasttagmap[ctx.rev()][1]
> +        def showlasttagdistance(**args):
> +            dist = self.lasttagmap[ctx.rev()][2]
> +            if dist>= 0:
> +                return str(dist)
> +            else:
> +                return ""
>    

As discussed above I would rather always have the distance.

Or actually ... for some uses it would be nice to have a simple template 
which gives "1.3" instead of "1.3-0" or "1.3-" - but it should still 
give for example "1.3-117".

Perhaps a {lasttaganddistance} would be useful too?

> +
>           defprops = {
>               'author': ctx.user(),
>               'branches': showbranches,
> @@ -891,6 +929,8 @@
>               'tags': showtags,
>               'extras': showextras,
>               'diffstat': showdiffstat,
> +            'lasttag': showlasttag,
> +            'lasttagdistance': showlasttagdistance,
>               }
>           props = props.copy()
>           props.update(defprops)
> diff --git a/mercurial/help.py b/mercurial/help.py
> --- a/mercurial/help.py
> +++ b/mercurial/help.py
> @@ -369,6 +369,9 @@
>       - file_adds: List of strings. Files added by this changeset.
>       - file_mods: List of strings. Files modified by this changeset.
>       - file_dels: List of strings. Files removed by this changeset.
> +    - lasttag: String. Most recent global tag in the ancestors of this
> +          changeset.
> +    - lasttagdistance: String. Longuest path to the last tag.
>       - node: String. The changeset identification hash, as a 40-character
>             hexadecimal string.
>       - parents: List of strings. The parents of the changeset.
> diff --git a/tests/test-log b/tests/test-log
> --- a/tests/test-log
> +++ b/tests/test-log
>    

Do this test really belong in the test of "log"? Wouldn't it be more 
natural in test-command-template ?

> @@ -118,4 +118,48 @@
>   hg log -u "user1" -u "user2"
>   hg log -u "user3"
>
> +cd ..
> +
> +echo % lasttag
> +hg init lasttag
> +cd lasttag
> +
> +echo a>  file
> +hg ci -Am a -d '0 0'
> +
> +echo b>>  file
> +hg ci -m b -d '1 0'
> +
> +echo c>>  head1
> +hg ci -Am h1c -d '2 0'
> +
> +hg update -q 1
> +echo d>>  head2
> +hg ci -Am h2d -d '3 0'
> +
> +echo e>>  head2
> +hg ci -m h2e -d '4 0'
> +
> +hg merge -q
> +hg ci -m merge -d '5 0'
> +
> +echo % No tag set
> +hg log --template '{rev}: {lasttag} {lasttagdistance}\n'
> +
> +echo % one common tag: longuest path wins
> +hg tag -r 1 -m t1 -d '6 0' t1
> +hg log --template '{rev}: {lasttag} {lasttagdistance}\n'
> +
> +echo % one ancestor tag: more recent wins
> +hg tag -r 2 -m t2 -d '7 0' t2
> +hg log --template '{rev}: {lasttag} {lasttagdistance}\n'
> +
> +echo % two branch tags: more recent wins
> +hg tag -r 3 -m t3 -d '8 0' t3
> +hg log --template '{rev}: {lasttag} {lasttagdistance}\n'
> +
> +echo % merged tag overrides
> +hg tag -r 5 -m t5 -d '9 0' t5
> +hg log --template '{rev}: {lasttag} {lasttagdistance}\n'
> +
>   exit 0
> diff --git a/tests/test-log.out b/tests/test-log.out
> --- a/tests/test-log.out
> +++ b/tests/test-log.out
> @@ -257,3 +257,53 @@
>   date:        Thu Jan 01 00:00:00 1970 +0000
>   summary:     a
>
> +% lasttag
> +adding file
> +adding head1
> +adding head2
> +created new head
> +% No tag set
> +5:
> +4:
> +3:
> +2:
> +1:
> +0:
> +% one common tag: longuest path wins
> +6: t1 4
> +5: t1 3
> +4: t1 2
> +3: t1 1
> +2: t1 1
> +1: t1 0
> +0:
> +% one ancestor tag: more recent wins
> +7: t2 3
> +6: t2 2
> +5: t2 1
> +4: t1 2
> +3: t1 1
> +2: t2 0
> +1: t1 0
> +0:
> +% two branch tags: more recent wins
> +8: t3 5
> +7: t3 4
> +6: t3 3
> +5: t3 2
> +4: t3 1
> +3: t3 0
> +2: t2 0
> +1: t1 0
> +0:
> +% merged tag overrides
> +9: t5 4
> +8: t5 3
> +7: t5 2
> +6: t5 1
> +5: t5 0
> +4: t3 1
> +3: t3 0
> +2: t2 0
> +1: t1 0
> +0:
>    



More information about the Mercurial-devel mailing list