[RFC 0/5] Git fetcher changes for resolving tags

[RFC 0/5] Git fetcher changes for resolving tags

[Olof Johansson]

Hi,

We use the git fetcher for our internal recipes, and we are supplying
;tag=<tag> to the SRC_URI. As the tag doesn't look like a sha1 hash, bitbake
treats this as a branch name. After the changes that made bitbake verify that a
commit is present on the specified branch, this led to errors. Richard fixed
this issue, in a patch included in this patch series.

Additionally, the change also uncovered issues we had with git ls-remote not
returning the sha1 we expect, in cases where a branch ends with the tag name
you are looking for, for instance:

$ git ls-remote <url> v1.2.3
981e76c984cb5513be7e8d1b7e3cab979c3a957e        refs/heads/foo/v1.2.3
dc171f81eebc887f72a2a973f5e5fc54ce1bbaca        refs/tags/v1.2.3

The current _latest_revision implementation would just do .split()[0], and
happily return 981e76c. My suggested solution is to first try
refs/tags/v1.2.3^{} and then move on to refs/heads/v1.2.3. This changes the
behavior for the cases were either a branch or a tag is specified: you would
never get the foo/v1.2.3 branch, as it's not an exact match.


Also in this patch series is a unit test suite for the git fetcher and some
minor refactoring of the fetcher itself. These tests should complement the more
high level tests already present in bb.tests.fetch.


Olof Johansson (4):
  bb.fetch2.git: reuse basecmd attribute
  bb.tests.fetch_git: initial set of tests for git fetcher
  bb.fetch2.git: support resolving both tags and branches
  bb.fetch2.git: use the _gen_git_url() function

Richard Purdie (1):
  fetch2/git: Improve handling of unresolved names verses branches

 bin/bitbake-selftest      |   3 +-
 lib/bb/fetch2/git.py      | 104 ++++++++++++++-----
 lib/bb/tests/fetch_git.py | 259 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 337 insertions(+), 29 deletions(-)
 create mode 100644 lib/bb/tests/fetch_git.py

-- 
1.8.4.rc3

[RFC 1/5] fetch2/git: Improve handling of unresolved names verses branches

[Olof Johansson]

From: Richard Purdie <richard.purdie@linuxfoundation.org>

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 lib/bb/fetch2/git.py | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index 00a459d..c391b38 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -106,6 +106,7 @@ class Git(FetchMethod):
         if ud.bareclone:
             ud.nocheckout = 1
   
+        ud.unresolvedrev = {}
         branches = ud.parm.get("branch", "master").split(',')
         if len(branches) != len(ud.names):
             raise bb.fetch2.ParameterError("The number of name and branch parameters is not balanced", ud.url)
@@ -113,6 +114,7 @@ class Git(FetchMethod):
         for name in ud.names:
             branch = branches[ud.names.index(name)]
             ud.branches[name] = branch
+            ud.unresolvedrev[name] = branch
 
         ud.basecmd = data.getVar("FETCHCMD_git", d, True) or "git"
 
@@ -124,7 +126,7 @@ class Git(FetchMethod):
             # Ensure anything that doesn't look like a sha256 checksum/revision is translated into one
             if not ud.revisions[name] or len(ud.revisions[name]) != 40  or (False in [c in "abcdef0123456789" for c in ud.revisions[name]]):
                 if ud.revisions[name]:
-                    ud.branches[name] = ud.revisions[name]
+                    ud.unresolvedrev[name] = ud.revisions[name]
                 ud.revisions[name] = self.latest_revision(ud, d, name)
 
         gitsrcname = '%s%s' % (ud.host.replace(':','.'), ud.path.replace('/', '.').replace('*', '.'))
@@ -300,7 +302,7 @@ class Git(FetchMethod):
         """
         Return a unique key for the url
         """
-        return "git:" + ud.host + ud.path.replace('/', '.') + ud.branches[name]
+        return "git:" + ud.host + ud.path.replace('/', '.') + ud.unresolvedrev[name]
 
     def _latest_revision(self, ud, d, name):
         """
@@ -313,7 +315,7 @@ class Git(FetchMethod):
 
         basecmd = data.getVar("FETCHCMD_git", d, True) or "git"
         cmd = "%s ls-remote %s://%s%s%s %s" % \
-              (basecmd, ud.proto, username, ud.host, ud.path, ud.branches[name])
+              (basecmd, ud.proto, username, ud.host, ud.path, ud.unresolvedrev[name])
         if ud.proto.lower() != 'file':
             bb.fetch2.check_network_access(d, cmd)
         output = runfetchcmd(cmd, d, True)
-- 
1.8.4.rc3

[RFC 2/5] bb.fetch2.git: reuse basecmd attribute

[Olof Johansson]

The basecmd is initialized in urldata_init; there's no need redoing that
work.

Signed-off-by: Olof Johansson <olof.johansson@axis.com>
---
 lib/bb/fetch2/git.py | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index c391b38..bd107db 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -152,7 +152,7 @@ class Git(FetchMethod):
             return True
         os.chdir(ud.clonedir)
         for name in ud.names:
-            if not self._contains_ref(ud.revisions[name], ud.branches[name], d):
+            if not self._contains_ref(ud, d, name):
                 return True
         if ud.write_tarballs and not os.path.exists(ud.fullmirror):
             return True
@@ -199,7 +199,7 @@ class Git(FetchMethod):
         # Update the checkout if needed
         needupdate = False
         for name in ud.names:
-            if not self._contains_ref(ud.revisions[name], ud.branches[name], d):
+            if not self._contains_ref(ud, d, name):
                 needupdate = True
         if needupdate:
             try: 
@@ -217,7 +217,7 @@ class Git(FetchMethod):
             ud.repochanged = True
         os.chdir(ud.clonedir)
         for name in ud.names:
-            if not self._contains_ref(ud.revisions[name], ud.branches[name], d):
+            if not self._contains_ref(ud, d, name):
                 raise bb.fetch2.FetchError("Unable to find revision %s in branch %s even from upstream" % (ud.revisions[name], ud.branches[name]))
 
     def build_mirror_data(self, ud, d):
@@ -287,9 +287,9 @@ class Git(FetchMethod):
     def supports_srcrev(self):
         return True
 
-    def _contains_ref(self, tag, branch, d):
-        basecmd = data.getVar("FETCHCMD_git", d, True) or "git"
-        cmd =  "%s branch --contains %s --list %s 2> /dev/null | wc -l" % (basecmd, tag, branch)
+    def _contains_ref(self, ud, d, name):
+        cmd =  "%s branch --contains %s --list %s 2> /dev/null | wc -l" % (
+            ud.basecmd, ud.revisions[name], ud.branches[name])
         try:
             output = runfetchcmd(cmd, d, quiet=True)
         except bb.fetch2.FetchError:
@@ -313,9 +313,8 @@ class Git(FetchMethod):
         else:
             username = ""
 
-        basecmd = data.getVar("FETCHCMD_git", d, True) or "git"
         cmd = "%s ls-remote %s://%s%s%s %s" % \
-              (basecmd, ud.proto, username, ud.host, ud.path, ud.unresolvedrev[name])
+              (ud.basecmd, ud.proto, username, ud.host, ud.path, ud.unresolvedrev[name])
         if ud.proto.lower() != 'file':
             bb.fetch2.check_network_access(d, cmd)
         output = runfetchcmd(cmd, d, True)
-- 
1.8.4.rc3

[RFC 3/5] bb.tests.fetch_git: initial set of tests for git fetcher

[Olof Johansson]

This commit introduces a set of whitebox unit tests for the git
fetcher, including some minor refactoring of the fetcher itself
to increase testability.

Signed-off-by: Olof Johansson <olof.johansson@axis.com>
---
 bin/bitbake-selftest      |   3 +-
 lib/bb/fetch2/git.py      |  31 +++++++--
 lib/bb/tests/fetch_git.py | 164 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 190 insertions(+), 8 deletions(-)
 create mode 100644 lib/bb/tests/fetch_git.py

diff --git a/bin/bitbake-selftest b/bin/bitbake-selftest
index 48a58fe..7f02fbf 100755
--- a/bin/bitbake-selftest
+++ b/bin/bitbake-selftest
@@ -25,10 +25,11 @@ try:
 except RuntimeError as exc:
     sys.exit(str(exc))
 
-tests = ["bb.tests.codeparser", 
+tests = ["bb.tests.codeparser",
          "bb.tests.cow",
          "bb.tests.data",
          "bb.tests.fetch",
+         "bb.tests.fetch_git",
          "bb.tests.utils"]
 
 for t in tests:
diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index bd107db..81bf282 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -87,14 +87,10 @@ class Git(FetchMethod):
         init git specific variable within url data
         so that the git method like latest_revision() can work
         """
-        if 'protocol' in ud.parm:
-            ud.proto = ud.parm['protocol']
-        elif not ud.host:
-            ud.proto = 'file'
-        else:
-            ud.proto = "git"
 
-        if not ud.proto in ('git', 'file', 'ssh', 'http', 'https', 'rsync'):
+        ud.proto = self._fetch_url_proto(ud)
+
+        if not self._valid_protocol(ud.proto):
             raise bb.fetch2.ParameterError("Invalid protocol type", ud.url)
 
         ud.nocheckout = ud.parm.get("nocheckout","0") == "1"
@@ -287,6 +283,27 @@ class Git(FetchMethod):
     def supports_srcrev(self):
         return True
 
+    def _fetch_url_proto(self, ud):
+        """
+        Identify protocol for Git URL.
+
+        The scheme prefix is used to couple the URL to this particular fetcher,
+        but it's not necessarily the git protocol we will use. If a protocol
+        URL parameter is supplied we will use that, otherwise we will use
+        git:// if the URL contains a hostname or file:// if it does not.
+
+        """
+        if 'protocol' in ud.parm:
+            return ud.parm['protocol']
+
+        if not ud.host:
+            return 'file'
+
+        return "git"
+
+    def _valid_protocol(self, proto):
+        return proto in ('git', 'file', 'ssh', 'http', 'https', 'rsync')
+
     def _contains_ref(self, ud, d, name):
         cmd =  "%s branch --contains %s --list %s 2> /dev/null | wc -l" % (
             ud.basecmd, ud.revisions[name], ud.branches[name])
diff --git a/lib/bb/tests/fetch_git.py b/lib/bb/tests/fetch_git.py
new file mode 100644
index 0000000..89af515
--- /dev/null
+++ b/lib/bb/tests/fetch_git.py
@@ -0,0 +1,164 @@
+# ex:ts=4:sw=4:sts=4:et
+# -*- tab-width: 4; c-basic-offset: 4; indent-tabs-mode: nil -*-
+#
+# BitBake Tests for the git fetcher (fetch2/git.py)
+#
+# Copyright (C) 2013 Olof Johansson
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 2 as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+import unittest
+from mock import Mock, patch
+import bb.fetch2
+from bb.fetch2.git import Git
+
+def mock_ud(url):
+    """
+    Generate a FetchData like Mock object based on a URL.
+    """
+    ud = Mock()
+    (ud.type, ud.host, ud.path,
+     ud.user, ud.pswd, ud.parm) = bb.fetch2.decodeurl(url)
+    return ud
+
+
+class GitFetcherTest(unittest.TestCase):
+    def setUp(self):
+        bb.fetch2.FetchMethod = Mock()
+        self.fetcher = Git()
+
+
+    def tearDown(self):
+        pass
+
+
+    def test_supports(self):
+        ud = Mock()
+
+        ud.type = 'git'
+        self.assertTrue(self.fetcher.supports(ud, None))
+
+        ud.type = 'ssh'
+        self.assertFalse(self.fetcher.supports(ud, None))
+
+
+    @patch("bb.fetch2.git.Git.latest_revision")
+    def test_urldata_init(self, latest_rev):
+        class MockD(object):
+            """ Mock datasmart class.  """
+            def __init__(self, **kwargs):
+                self.known = kwargs
+
+            def getVar(self, var, expand=False):
+                if var in self.known:
+                    return self.known[var]
+
+
+        bb.fetch2.git.data.getVar = lambda var, d, expand: d.getVar(var, expand)
+
+        d = MockD(
+            DL_DIR='/tmp/dl_dir',
+        )
+
+        # Invalid protocol given as url parameter
+        with self.assertRaises(bb.fetch2.ParameterError):
+            ud = mock_ud("git://example.com/foo.git;protocol=gopher")
+            self.fetcher.urldata_init(ud, d)
+
+        # Unbalanced number of branch/name parameters
+        with self.assertRaises(bb.fetch2.ParameterError):
+            ud = mock_ud(
+                "git://example.com/foo.git;branch=master,foo;name=bar")
+            # The names are usually extracted in FetchMethod constructor
+            ud.names = ['bar']
+            self.fetcher.urldata_init(ud, d)
+
+        ud = mock_ud("git://example.com/foo.git;protocol=ssh")
+        ud.names = ['default']
+        ud.revisions = {'default': None}
+
+        latest_rev.return_value = 'asd'
+
+        self.fetcher.urldata_init(ud, d)
+
+        self.assertFalse(ud.nocheckout)
+        self.assertFalse(ud.rebaseable)
+        self.assertFalse(ud.bareclone)
+        self.assertEqual(ud.branches, {'default': 'master'})
+
+
+    def test_fetch_url_proto(self):
+        for url in [
+            ('git:///foo.git', 'file'),
+            ('git://example.com/foo.git', 'git'),
+            ('git://example.com/foo.git;protocol=ssh', 'ssh')
+        ]:
+            ud = mock_ud(url[0])
+            self.assertEqual(self.fetcher._fetch_url_proto(ud), url[1])
+
+
+    def test_valid_protocol(self):
+        for valid in ['git', 'file', 'ssh', 'http', 'https', 'rsync']:
+            self.assertTrue(self.fetcher._valid_protocol(valid))
+        for invalid in ['dns', '', None]:
+            self.assertFalse(self.fetcher._valid_protocol(invalid))
+
+
+    def test_contains_ref(self):
+        commit_sha1 = '8f453bb11d72afc90a986ac604b3477d97eaf9a8'
+
+        ud = Mock()
+        ud.basecmd = 'git'
+        ud.revisions = {'default': commit_sha1}
+        ud.branches = {'default': 'master'}
+
+        d = Mock()
+
+        bb.fetch2.git.runfetchcmd = Mock()
+
+        # Two output lines from git branch --contains; _contains_ref() == True
+        # Also, while we're at it, verify that ud.basecmd changes take effect.
+        ud.basecmd = 'sudo git'
+        bb.fetch2.git.runfetchcmd.return_value = '2'
+        self.assertTrue(self.fetcher._contains_ref(ud, d, 'default'))
+        bb.fetch2.git.runfetchcmd.assert_called_with(
+            "sudo git branch --contains %s --list %s 2> /dev/null | wc -l" % (
+                ud.revisions['default'], ud.branches['default']),
+            d, quiet=True)
+        ud.basecmd = 'git'
+
+        # No output from git branch --contains; _contains_ref() == False
+        bb.fetch2.git.runfetchcmd.return_value = '0'
+        self.assertFalse(self.fetcher._contains_ref(ud, d, 'default'))
+        bb.fetch2.git.runfetchcmd.assert_called_with(
+            "git branch --contains %s --list %s 2> /dev/null | wc -l" % (
+                ud.revisions['default'], ud.branches['default']),
+            d, quiet=True)
+
+        # Strange wc -l output should throw FetchError exception
+        with self.assertRaises(bb.fetch2.FetchError):
+            bb.fetch2.git.runfetchcmd.return_value = '0 1'
+            self.fetcher._contains_ref(ud, d, 'default')
+            bb.fetch2.git.runfetchcmd.assert_called_with(
+                "git branch --contains %s --list %s 2> /dev/null | wc -l" % (
+                    ud.revisions['default'], ud.branches['default']),
+                d, quiet=True)
+
+
+    def test_supports_checksum(self):
+        self.assertFalse(self.fetcher.supports_checksum(None))
+
+
+    def test_supports_srcrev(self):
+        self.assertTrue(self.fetcher.supports_srcrev())
-- 
1.8.4.rc3

[RFC 4/5] bb.fetch2.git: support resolving both tags and branches

[Olof Johansson]

Before this change, the resolution of a git reference to a commit sha1,
was done in a way that was prone to bogus matches. Looking for a tag
R1.2.3 would potentially get the SHA1 of a branch ending in R1.2.3, like
refs/heads/foo/bar/R1.2.3, instead of refs/tags/R1.2.3.

But since tags and branches are used somewhat ambiguously, we will fall
back to resolve it as a branch with that exact name, if no tag with the
exact name was found.

Signed-off-by: Olof Johansson <olof.johansson@axis.com>
---
 lib/bb/fetch2/git.py      | 47 ++++++++++++++++++++---
 lib/bb/tests/fetch_git.py | 97 ++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 137 insertions(+), 7 deletions(-)

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index 81bf282..c01c82b 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -321,23 +321,58 @@ class Git(FetchMethod):
         """
         return "git:" + ud.host + ud.path.replace('/', '.') + ud.unresolvedrev[name]
 
-    def _latest_revision(self, ud, d, name):
+    def _gen_git_url(self, ud, d):
         """
-        Compute the HEAD revision for the url
+        Based on the FetchData object, return a valid remote url that
+        you can pass to git.
         """
         if ud.user:
             username = ud.user + '@'
         else:
             username = ""
 
-        cmd = "%s ls-remote %s://%s%s%s %s" % \
-              (ud.basecmd, ud.proto, username, ud.host, ud.path, ud.unresolvedrev[name])
+        return "%s://%s%s%s" % (ud.proto, username, ud.host, ud.path)
+
+    def _resolve_ref(self, ud, d, ref):
+        """
+        Try to resolve a reference using git ls-remote. We assume the
+        reference is fully qualified, or otherwise can be uniquely
+        resolved. Will return the commit sha1 of the reference.
+
+        If more than one line of output is returned, we'll raise an
+        FetchError exception. Same if no output is returned.
+        """
+        url = self._gen_git_url(ud, d)
+        cmd = "%s ls-remote %s %s" % (ud.basecmd, url, ref)
+
         if ud.proto.lower() != 'file':
             bb.fetch2.check_network_access(d, cmd)
+
         output = runfetchcmd(cmd, d, True)
         if not output:
-            raise bb.fetch2.FetchError("The command %s gave empty output unexpectedly" % cmd, ud.url)
-        return output.split()[0]
+            raise bb.fetch2.FetchError(
+                "The command %s gave empty output unexpectedly" % cmd, ud.url)
+
+        output = output.split()
+        if len(output) > 2:
+            raise bb.fetch2.FetchError(
+                "The command %s gave more than one result" % cmd, ud.url)
+
+        return output[0]
+
+    def _latest_revision(self, ud, d, name):
+        """
+        Compute the HEAD revision for the url. The reference is first
+        tried as refs/tags/<ref>^{}, which should return a sha1 of the
+        commit object pointed to by the tag. If nothing is found, we'll
+        try to resolve it as refs/heads/<tag> instead.
+        """
+        ref = ud.unresolvedrev[name]
+
+        try:
+            return self._resolve_ref(ud, d, "refs/tags/%s^{}" % ref)
+        except:
+            return self._resolve_ref(ud, d, "refs/heads/%s" % ref)
 
     def _build_revision(self, ud, d, name):
         return ud.revisions[name]
diff --git a/lib/bb/tests/fetch_git.py b/lib/bb/tests/fetch_git.py
index 89af515..b11c335 100644
--- a/lib/bb/tests/fetch_git.py
+++ b/lib/bb/tests/fetch_git.py
@@ -3,7 +3,7 @@
 #
 # BitBake Tests for the git fetcher (fetch2/git.py)
 #
-# Copyright (C) 2013 Olof Johansson
+# Copyright (C) 2013 Olof Johansson <olof.johansson@axis.com>
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 2 as
@@ -30,6 +30,7 @@ def mock_ud(url):
     ud = Mock()
     (ud.type, ud.host, ud.path,
      ud.user, ud.pswd, ud.parm) = bb.fetch2.decodeurl(url)
+    ud.basecmd = 'git'
     return ud
 
 
@@ -156,6 +157,100 @@ class GitFetcherTest(unittest.TestCase):
                 d, quiet=True)
 
 
+    def test_gen_git_url(self):
+        ud = mock_ud("git://example.com/repo.git")
+        ud.proto = 'git'
+        self.assertEqual(
+            self.fetcher._gen_git_url(ud, Mock()),
+            'git://example.com/repo.git')
+
+        ud = mock_ud("git://example.com/repo.git;param=value")
+        ud.proto = 'git'
+        self.assertEqual(
+            self.fetcher._gen_git_url(ud, Mock()),
+            'git://example.com/repo.git')
+
+        ud = mock_ud("git://example.com/repo.git;protocol=ssh")
+        ud.proto = 'ssh'
+        self.assertEqual(
+            self.fetcher._gen_git_url(ud, Mock()),
+            'ssh://example.com/repo.git')
+
+        ud = mock_ud("git://example.com/repo.git?query=param")
+        ud.proto = 'git'
+        self.assertEqual(
+            self.fetcher._gen_git_url(ud, Mock()),
+            'git://example.com/repo.git?query=param')
+
+        ud = mock_ud("git://user@example.com/repo.git")
+        ud.proto = 'git'
+        self.assertEqual(
+            self.fetcher._gen_git_url(ud, Mock()),
+            'git://user@example.com/repo.git')
+
+
+    @patch('bb.fetch2.check_network_access')
+    @patch('bb.fetch2.git.runfetchcmd')
+    def test_resolve_ref(self, mock_runcmd, mock_check_net):
+        ud = mock_ud("git:///repo.git")
+        ud.proto = 'file'
+        d = Mock()
+
+        self.fetcher._resolve_ref(ud, d, "v1.2.3")
+        self.assertFalse(mock_check_net.called)
+        mock_runcmd.assert_called_once_with(
+            "git ls-remote file:///repo.git v1.2.3",
+            d, True)
+        mock_runcmd.reset_mock()
+
+        ud = mock_ud("git://example.com/repo.git")
+        ud.proto = 'git'
+        mock_runcmd.return_value = "%s %s" % ('a'*40, "R1.2.3^{}")
+        self.fetcher._resolve_ref(ud, d, "refs/tags/R1.2.3^{}")
+        self.assertTrue(mock_check_net.called)
+        mock_runcmd.assert_called_once_with(
+            "git ls-remote git://example.com/repo.git refs/tags/R1.2.3^{}",
+            d, True)
+
+        # Two matches; two output lines. Should raise exception
+        with self.assertRaises(bb.fetch2.FetchError):
+            mock_runcmd.return_value = '\n'.join(
+                ["%s %s" % ('a'*40, "R1.2.3^{}")*2])
+            self.fetcher._resolve_ref(ud, d, "refs/tags/R1.2.3^{}")
+
+        # No match; no output. Should raise exception
+        with self.assertRaises(bb.fetch2.FetchError):
+            mock_runcmd.return_value = ''
+            self.fetcher._resolve_ref(ud, d, "refs/tags/R1.2.3^{}")
+
+
+    @patch('bb.fetch2.git.Git._resolve_ref')
+    def test_latest_revision(self, mock_resolve):
+        ud = mock_ud('git://example.com/repo.git')
+        d = Mock()
+        ud.revisions = {'default': 'ref'}
+
+        # Find match for refs/tags/ref^{}.
+        mock_resolve.return_value = 'a'*40
+        sha1 = self.fetcher._latest_revision(ud, d, 'default')
+        self.assertEqual(sha1, 'a'*40)
+        mock_resolve.assert_called_once_with(ud, d, 'refs/tags/ref^{}')
+
+        # Doesn't find match for refs/tags/ref^{}. Moves on to refs/heads/ref.
+        mock_resolve.side_effect = [
+            bb.fetch2.FetchError('fail'), 'b'*40
+        ]
+        sha1 = self.fetcher._latest_revision(ud, d, 'default')
+        self.assertEqual(sha1, 'b'*40)
+        mock_resolve.assert_called_with(ud, d, 'refs/heads/ref')
+
+        # Doesn't find match for anything.
+        with self.assertRaises(bb.fetch2.FetchError):
+            mock_resolve.reset_mock()
+            mock_resolve.side_effect = bb.fetch2.FetchError("fail")
+            self.fetcher._latest_revision(ud, d, 'default')
+
+
     def test_supports_checksum(self):
         self.assertFalse(self.fetcher.supports_checksum(None))
 
-- 
1.8.4.rc3

[RFC 5/5] bb.fetch2.git: use the _gen_git_url() function

[Olof Johansson]

Don't generate the url in several places.

Signed-off-by: Olof Johansson <olof.johansson@axis.com>
---
 lib/bb/fetch2/git.py | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index c01c82b..bc792e6 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -166,11 +166,6 @@ class Git(FetchMethod):
     def download(self, ud, d):
         """Fetch url"""
 
-        if ud.user:
-            username = ud.user + '@'
-        else:
-            username = ""
-
         ud.repochanged = not os.path.exists(ud.fullmirror)
 
         # If the checkout doesn't exist and the mirror tarball does, extract it
@@ -179,7 +174,7 @@ class Git(FetchMethod):
             os.chdir(ud.clonedir)
             runfetchcmd("tar -xzf %s" % (ud.fullmirror), d)
 
-        repourl = "%s://%s%s%s" % (ud.proto, username, ud.host, ud.path)
+        repourl = self._gen_git_url(ud, d)
 
         # If the repo still doesn't exist, fallback to cloning it
         if not os.path.exists(ud.clonedir):
-- 
1.8.4.rc3

Re: [RFC 3/5] bb.tests.fetch_git: initial set of tests for git fetcher

[Richard Purdie]

On Thu, 2013-12-12 at 17:48 +0100, Olof Johansson wrote:
> This commit introduces a set of whitebox unit tests for the git
> fetcher, including some minor refactoring of the fetcher itself
> to increase testability.
> 
> Signed-off-by: Olof Johansson <olof.johansson@axis.com>
> ---
>  bin/bitbake-selftest      |   3 +-
>  lib/bb/fetch2/git.py      |  31 +++++++--
>  lib/bb/tests/fetch_git.py | 164 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 190 insertions(+), 8 deletions(-)
>  create mode 100644 lib/bb/tests/fetch_git.py
> 
> diff --git a/bin/bitbake-selftest b/bin/bitbake-selftest
> index 48a58fe..7f02fbf 100755
> --- a/bin/bitbake-selftest
> +++ b/bin/bitbake-selftest
> @@ -25,10 +25,11 @@ try:
>  except RuntimeError as exc:
>      sys.exit(str(exc))
>  
> -tests = ["bb.tests.codeparser", 
> +tests = ["bb.tests.codeparser",
>           "bb.tests.cow",
>           "bb.tests.data",
>           "bb.tests.fetch",
> +         "bb.tests.fetch_git",
>           "bb.tests.utils"]
>  
>  for t in tests:
> diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
> index bd107db..81bf282 100644
> --- a/lib/bb/fetch2/git.py
> +++ b/lib/bb/fetch2/git.py
> @@ -87,14 +87,10 @@ class Git(FetchMethod):
>          init git specific variable within url data
>          so that the git method like latest_revision() can work
>          """
> -        if 'protocol' in ud.parm:
> -            ud.proto = ud.parm['protocol']
> -        elif not ud.host:
> -            ud.proto = 'file'
> -        else:
> -            ud.proto = "git"
>  
> -        if not ud.proto in ('git', 'file', 'ssh', 'http', 'https', 'rsync'):
> +        ud.proto = self._fetch_url_proto(ud)
> +
> +        if not self._valid_protocol(ud.proto):
>              raise bb.fetch2.ParameterError("Invalid protocol type", ud.url)
>  
>          ud.nocheckout = ud.parm.get("nocheckout","0") == "1"
> @@ -287,6 +283,27 @@ class Git(FetchMethod):
>      def supports_srcrev(self):
>          return True
>  
> +    def _fetch_url_proto(self, ud):
> +        """
> +        Identify protocol for Git URL.
> +
> +        The scheme prefix is used to couple the URL to this particular fetcher,
> +        but it's not necessarily the git protocol we will use. If a protocol
> +        URL parameter is supplied we will use that, otherwise we will use
> +        git:// if the URL contains a hostname or file:// if it does not.
> +
> +        """
> +        if 'protocol' in ud.parm:
> +            return ud.parm['protocol']
> +
> +        if not ud.host:
> +            return 'file'
> +
> +        return "git"
> +
> +    def _valid_protocol(self, proto):
> +        return proto in ('git', 'file', 'ssh', 'http', 'https', 'rsync')
> +
>      def _contains_ref(self, ud, d, name):
>          cmd =  "%s branch --contains %s --list %s 2> /dev/null | wc -l" % (
>              ud.basecmd, ud.revisions[name], ud.branches[name])
> diff --git a/lib/bb/tests/fetch_git.py b/lib/bb/tests/fetch_git.py
> new file mode 100644
> index 0000000..89af515
> --- /dev/null
> +++ b/lib/bb/tests/fetch_git.py
> @@ -0,0 +1,164 @@
> +# ex:ts=4:sw=4:sts=4:et
> +# -*- tab-width: 4; c-basic-offset: 4; indent-tabs-mode: nil -*-
> +#
> +# BitBake Tests for the git fetcher (fetch2/git.py)
> +#
> +# Copyright (C) 2013 Olof Johansson
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License version 2 as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License along
> +# with this program; if not, write to the Free Software Foundation, Inc.,
> +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> +
> +import unittest
> +from mock import Mock, patch

This is going to cause some headaches since we don't have Mock listed in
and of the prerequisite documentation, nor is it present in our prebuilt
tools tarball or do we have a recipe for it.

This is probably enough to block the patch until we can find someone to
look at fixing that :(

Would it be possible to separate out the tests from the other changes?

Cheers,

Richard

Re: [RFC 3/5] bb.tests.fetch_git: initial set of tests for git fetcher

[Olof Johansson]

On 13-12-18 12:43 +0100, Richard Purdie wrote:
> > +import unittest
> > +from mock import Mock, patch
> 
> This is going to cause some headaches since we don't have Mock listed in
> and of the prerequisite documentation, nor is it present in our prebuilt
> tools tarball or do we have a recipe for it.
> 
> This is probably enough to block the patch until we can find someone to
> look at fixing that :(

Ah, yeah, I see. It is a very useful testing library, but I could
take a look at rolling my own alternative. Unless there's a way
to make it acceptable for us with bitbake. Would creating a
recipe for it be enough? Or just skip the tests if mock isn't
available?

> Would it be possible to separate out the tests from the other changes?

Yes. I was planning to do that after our discussions on IRC, but
haven't gotten around to it yet. Will hopefully do that before
going on vacation on friday.


Thanks,
-- 
olofjn

Re: [RFC 3/5] bb.tests.fetch_git: initial set of tests for git fetcher

[Richard Purdie]

On Wed, 2013-12-18 at 12:51 +0100, Olof Johansson wrote:
> On 13-12-18 12:43 +0100, Richard Purdie wrote:
> > > +import unittest
> > > +from mock import Mock, patch
> > 
> > This is going to cause some headaches since we don't have Mock listed in
> > and of the prerequisite documentation, nor is it present in our prebuilt
> > tools tarball or do we have a recipe for it.
> > 
> > This is probably enough to block the patch until we can find someone to
> > look at fixing that :(
> 
> Ah, yeah, I see. It is a very useful testing library, but I could
> take a look at rolling my own alternative. Unless there's a way
> to make it acceptable for us with bitbake. Would creating a
> recipe for it be enough? Or just skip the tests if mock isn't
> available?

How about we check mock into the bitbake tree? Looking at
https://pypi.python.org/pypi/mock it seems we can get away with a 75kb
file from the tarball assuming I understand things correctly...

> > Would it be possible to separate out the tests from the other changes?
> 
> Yes. I was planning to do that after our discussions on IRC, but
> haven't gotten around to it yet. Will hopefully do that before
> going on vacation on friday.

Ok, thanks.

Cheers,

Richard