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

[Gilles Moris]

On Wed July 15 2009 03:02:33 Mads Kiilerich wrote:
> 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

OK, I just put some parenthesis around null to show it is not an actual tag.

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

Actually, this is probably more a theoretical question, as I doubt that many
repo with a double global tag exist. I have used the '=' sign to join and show
this is the same rev: '='.join(sorted(tags))
Note however that this tag list is created once, and the distance is just
incremented after.  it is only in case of a merge that the same rev can join
with different distances. So it works even with a random ordering.

> 
> > +            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.
> 

It is not a totally obscure feature of Python. I remember reading that from
the Python Doc somewhere. However, the comment was possibly confusing so I
restate it following your directions to:
                # the last tag of this rev comes from the ancestor with the
                # latest tag (secondarily the longest distance to the tag),
                # achieved by comparing the tuples.
I will also set the second tuple element to the distance to be even clearer.

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

Well OK, I sometime like to experiment languages possibilities. I agree yours
is clearer. 

> > +                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.
> 

OK, changed for (null) as default value.

> > +                    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?
> 

Personally I prefer it with a '+' to show that it's after the tag.
So I think it's better to stay like that and let the people choose their
format.

> > +
> >           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.

Now, it will just be an Integer, as I do not need it to be empty.

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

I didn't saw that most keyword were tested there at first. Changed.

Thx for your input.
Gilles.