launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

There is something very weird about the history of this branch which has caused a bunch of changes from devel to be reverted (see the preview diff here).  The last commit is in fact allegedly a merge from devel but contains a bunch of your own changes too.  Could you sort this out?  It's probably best to uncommit the merge and recommit just your change properly; we normally wouldn't edit the history that way but it makes sense for this sort of history fix.

I'm happy to advise on development process kind of stuff generally, in case there's some root problem behind this.

As for the substantive change, it mostly looks like the right approach but there are a few details that need fixing up as per my inline comments.  Thanks!

Diff comments:

> 
> === modified file 'lib/lp/code/interfaces/githosting.py'
> --- lib/lp/code/interfaces/githosting.py	2015-06-15 10:01:55 +0000
> +++ lib/lp/code/interfaces/githosting.py	2016-04-29 06:21:15 +0000
> @@ -88,3 +88,13 @@
>          :param path: Physical path of the repository on the hosting service.
>          :param logger: An optional logger.
>          """
> +
> +    def getBlob(self, path, filename, rev=None, logger=None):

Unlike the model, the method declaration in the interface shouldn't include the "self" parameter.

> +        """Get a blob by file name from a repository.
> +
> +        :param path: Physical path of the repository on the hosting service.
> +        :param filename: Relative path of a file in the repository.
> +        :param rev: An optional revision. Defaults to 'HEAD'.
> +        :param logger: An optional logger.
> +        :return: A dict with keys 'data' and 'size'.
> +        """
> 
> === modified file 'lib/lp/code/model/githosting.py'
> --- lib/lp/code/model/githosting.py	2016-04-19 17:18:39 +0000
> +++ lib/lp/code/model/githosting.py	2016-04-29 06:21:15 +0000
> @@ -147,3 +147,16 @@
>          except Exception as e:
>              raise GitRepositoryDeletionFault(
>                  "Failed to delete Git repository: %s" % unicode(e))
> +
> +    def getBlob(self, path, filename, rev=None, logger=None):
> +        """See `IGitHostingClient`."""
> +        try:
> +            if logger is not None:
> +                logger.info(
> +                    "Fetching file %s from repository %s" % (filename, path))
> +            return self._get(
> +                "/repo/%s/blob/%s%s" % (
> +                    path, filename, '?rev=%s' % rev if rev else ''))

The other GitHostingClient methods somewhat lazily assume that revisions don't require URL quoting, which we can get away with there because they're only used internally.  However, this one is exposed indirectly via a public API, so we need to be more careful now.  quote_plus(rev) should be good enough.  Also have a think about what quoting is required for the filename; what if the filename contains "?", for instance?  It's still fine to use the path without quoting.

(It would not be terrible to fix up the other hosting client methods to match whatever approach you take once you settle on it.)

> +        except Exception as e:
> +            raise GitRepositoryScanFault(
> +                "Failed to get file from Git repository: %s" % unicode(e))
> 
> === modified file 'lib/lp/code/model/tests/test_gitrepository.py'
> --- lib/lp/code/model/tests/test_gitrepository.py	2016-02-06 02:20:04 +0000
> +++ lib/lp/code/model/tests/test_gitrepository.py	2016-04-29 06:21:15 +0000
> @@ -2033,6 +2033,37 @@
>                  for event in events[:2]))
>  
>  
> +

The extra blank line here is superfluous; we use two blank lines between classes.

> +class TestGitRepositoryGetBlob(TestCaseWithFactory):
> +    """Tests for retrieving files from a Git repository."""
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def test_getBlob_with_default_rev(self):
> +        repository = self.factory.makeGitRepository()
> +        hosting_client = FakeMethod()
> +        expected_result = {
> +            'data': u'Some text'.encode('base64'),
> +            'size': len(u'Some Text'),
> +            }
> +        hosting_client.getBlob = FakeMethod(result=expected_result)
> +        self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
> +        ret = repository.getBlob('src/README.txt')
> +        self.assertEqual(ret, expected_result)

Unlike some other projects, the LP style for equality tests is (expected, observed).

> +
> +    def test_getBlob_with_rev(self):
> +        repository = self.factory.makeGitRepository()
> +        hosting_client = FakeMethod()
> +        expected_result = {
> +            'data': u'Some text'.encode('base64'),
> +            'size': len(u'Some Text'),
> +            }
> +        hosting_client.getBlob = FakeMethod(result=expected_result)
> +        self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
> +        ret = repository.getBlob('src/README.txt', 'some-rev')
> +        self.assertEqual(ret, expected_result)
> +
> +
>  class TestGitRepositorySet(TestCaseWithFactory):
>  
>      layer = DatabaseFunctionalLayer


-- 
https://code.launchpad.net/~maxiberta/launchpad/git-get-blob/+merge/293337
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.