[Treepkg-devel] [Treepkg-commits] r464 - trunk/treepkg

[Bernhard Herzog]

scm-commit at wald.intevation.org writes:

> Modified: trunk/treepkg/packager.py
> ===================================================================
> --- trunk/treepkg/packager.py	2010-10-25 08:17:31 UTC (rev 463)
> +++ trunk/treepkg/packager.py	2010-10-25 08:18:21 UTC (rev 464)
> @@ -108,7 +110,11 @@
>          revision = self.revision
>          rules_revision = self.parent.rules_revision
>          pkg_revision = self.parent.pkg_revision
> +        short_revision = revision
> +        if len(pkg_revision) > 7:
> +            short_revision = short_revision[:7]

This is for git and other DVCSs, I suppose.  Ideally, how revision
numbers can be shortened should be defined by the version control
system, which in TreePKG would be the working copy class.  Hard-coding
conversion from long revisions to short-revisions should be at least
accompanied by a comment, indicating why it's done and why it's done
that way.  There's a difference between git and mercurial, for instance:
git takes the first 7 hex-digits for the short revision, whereas
mercurial takes the first 12 hex-digits, by default, anyway.

>          pkg_date = time.strftime("%Y%m%d", time.localtime())
> +        pkg_time = time.strftime("%H%M", time.localtime())

That change introduces a race by evaluating time.localtime() twice.  It
could happen that pkg_date is computed immediately before midnight and
pkg_time immediately afterwards, which together give a timestamp that's
about 24 hours too early.

>          upstream_version = self.determine_upstream_version(directory)
>          version_dict = locals().copy()
>          if type(additionals).__name__=='dict':

That last line looks fishy.  Looking at the code, it seems "if
additionals is not None:" or even "if additionals:" would be much more
appropriate.

  Bernhard

-- 
Bernhard Herzog  |  ++49-541-335 08 30  |  http://www.intevation.de/
Intevation GmbH, Neuer Graben 17, 49074 Osnabrück | AG Osnabrück, HR B 18998
Geschäftsführer: Frank Koormann, Bernhard Reiter, Dr. Jan-Oliver Wagner