launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/code/model/gitref.py'
> --- lib/lp/code/model/gitref.py	2016-02-06 02:20:04 +0000
> +++ lib/lp/code/model/gitref.py	2016-05-13 21:13:56 +0000
> @@ -242,6 +247,63 @@
>          """See `IGitRef`."""
>          return self.repository.pending_writes
>  
> +    def _getLog(self, start, limit=None, stop=None, logger=None):
> +        hosting_client = getUtility(IGitHostingClient)
> +        memcache_client = getUtility(IMemcacheClient)
> +        path = self.repository.getInternalPath()
> +        memcache_key = "%s:git-revisions:%s:%s" % (

This value is called all of "log", "revisions" and "commits".

> +            config.instance_name, path, start)
> +        if limit is not None:
> +            memcache_key += ":limit=%s" % limit
> +        if stop is not None:
> +            memcache_key += ":stop=%s" % stop
> +        if isinstance(memcache_key, unicode):
> +            memcache_key = memcache_key.encode("UTF-8")
> +        log = None
> +        cached_log = memcache_client.get(memcache_key)

I would be a lot more comfortable if there was a feature flag around all of the the memcached usage here. As you say, we really don't know how it will behave.

A separate flag to disable log retrieval from the service at all would also be a good idea. Or we may find that Google ruins our everything and we'll have no way to unruin it.

> +        if cached_log is not None:
> +            try:
> +                log = json.loads(cached_log)
> +            except Exception as e:
> +                logger.info(
> +                    "Cannot load cached log information for %s:%s (%s); "
> +                    "deleting" % (path, start, str(e)))

logger.exception? Otherwise we suppress all the interesting bits.

> +                memcache_client.delete(memcache_key)
> +        if log is None:
> +            log = removeSecurityProxy(hosting_client.getLog(
> +                path, start, limit=limit, stop=stop, logger=logger))
> +            memcache_client.set(memcache_key, json.dumps(log))
> +        return log
> +
> +    def getCommits(self, start, limit=None, stop=None,
> +                   start_date=None, end_date=None, logger=None):
> +        # Circular import.
> +        from lp.code.model.gitrepository import parse_git_commits
> +
> +        log = self._getLog(start, limit=limit, stop=stop, logger=logger)
> +        parsed_commits = parse_git_commits(log)
> +        revisions = []
> +        for commit in log:
> +            if "sha1" not in commit:
> +                continue
> +            parsed_commit = parsed_commits[commit["sha1"]]
> +            author_date = parsed_commit.get("author_date")
> +            if start_date is not None:
> +                if author_date is None or author_date < start_date:
> +                    continue
> +            if end_date is not None:
> +                if author_date is None or author_date > end_date:
> +                    continue
> +            revisions.append(parsed_commit)
> +        return revisions
> +
> +    def getLatestCommits(self, quantity=10):
> +        return self.getCommits(self.commit_sha1, limit=quantity)
> +
> +    @property
> +    def has_commits(self):
> +        return len(self.getLatestCommits())

This is secretly quite expensive. Can we just check whether commit_sha1 is set?

> +
>      @property
>      def recipes(self):
>          """See `IHasRecipes`."""


-- 
https://code.launchpad.net/~cjwatson/launchpad/git-ref-commits/+merge/294631
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.