Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: shallow git clone #5514

Merged
merged 1 commit into from Jul 30, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -12,7 +12,7 @@
from conans.client.tools.files import chdir
from conans.errors import ConanException
from conans.model.version import Version
from conans.util.files import decode_text, to_file_bytes, walk
from conans.util.files import decode_text, to_file_bytes, walk, mkdir


def _run_muted(cmd, folder=None):
@@ -36,6 +36,16 @@ def _check_repo(cmd, folder, msg=None):
class SCMBase(object):
cmd_command = None

@classmethod
def get_version(cls):
try:
out, _ = subprocess.Popen([cls.cmd_command, "--version"], stdout=subprocess.PIPE).communicate()
version_line = decode_text(out).split('\n', 1)[0]
version_str = version_line.split(' ', 3)[2]
return Version(version_str)
except Exception as e:
raise ConanException("Error retrieving {} version: '{}'".format(cls.cmd_command, e))

def __init__(self, folder=None, verify_ssl=True, username=None, password=None,
force_english=True, runner=None, output=None):
self.folder = folder or os.getcwd()
@@ -89,23 +99,48 @@ def run(self, command):
command = self._configure_ssl_verify + command
return super(Git, self).run(command)

def clone(self, url, branch=None, args=""):
def _fetch(self, url, branch, shallow):
if not branch:
raise ConanException("The destination folder '%s' is not empty, "
"specify a branch to checkout (not a tag or commit) "
"or specify a 'subfolder' "
"attribute in the 'scm'" % self.folder)

output = self.run("init")
output += self.run('remote add origin "%s"' % url)

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jul 29, 2019

Member

(Just thinking aloud:)

I know these are the same commands as before, but this command fails if the remote is already added to the repo:

⇒  git remote -v
origin	git@github.com:sword-and-sorcery/conanquest-fantastic-adventures.git (fetch)
origin	git@github.com:sword-and-sorcery/conanquest-fantastic-adventures.git (push)
[python3]jgsogo@MacBook-Pro-de-Javier:~/dev/conan/issues/tmp5514|master 
⇒  git remote add origin git@github.com:sword-and-sorcery/conanquest-fantastic-adventures.git
fatal: remoto origin ya existe.

Maybe it is just the name of the function, fetch, I know it is not public and won't be executed on its own, so these two lines probably will be removed in the future to the calling function.

if shallow:
output += self.run('fetch --depth 1 origin "%s"' % branch)
output += self.run('checkout FETCH_HEAD')
else:
output += self.run("fetch")
output += self.run("checkout -t origin/%s" % branch)
return output

def clone(self, url, branch=None, args="", shallow=False):
"""
:param url: repository remote URL to clone from (e.g. https, git or local)
:param branch: actually, can be any valid git ref expression like,
- None, use default branch, usually it's "master"
- branch name
- tag name
- revision sha256
- expression like HEAD~1
:param args: additional arguments to be passed to the git command (e.g. config args)
:param shallow:
:return: output of the clone command
"""
# TODO: rename "branch" -> "element" in Conan 2.0
url = self.get_url_with_credentials(url)
if os.path.exists(url):
url = url.replace("\\", "/") # Windows local directory
if os.path.exists(self.folder) and os.listdir(self.folder):
if not branch:
raise ConanException("The destination folder '%s' is not empty, "
"specify a branch to checkout (not a tag or commit) "
"or specify a 'subfolder' "
"attribute in the 'scm'" % self.folder)
output = self.run("init")
output += self.run('remote add origin "%s"' % url)
output += self.run("fetch ")
output += self.run("checkout -t origin/%s" % branch)
else:
branch_cmd = "--branch %s" % branch if branch else ""
output = self.run('clone "%s" . %s %s' % (url, branch_cmd, args))
mkdir(self.folder) # might not exist in case of shallow clone
if os.listdir(self.folder):
return self._fetch(url, branch, shallow)
if shallow and branch:
return self._fetch(url, branch, shallow)
branch_cmd = "--branch %s" % branch if branch else ""
shallow_cmd = "--depth 1" if shallow else ""
output = self.run('clone "%s" . %s %s %s' % (url, branch_cmd, shallow_cmd, args))

return output

@@ -232,16 +267,6 @@ def runner_no_strip(command):
runner = runner or runner_no_strip
super(SVN, self).__init__(folder=folder, runner=runner, *args, **kwargs)

@staticmethod
def get_version():
try:
out, _ = subprocess.Popen(["svn", "--version"], stdout=subprocess.PIPE).communicate()
version_line = decode_text(out).split('\n', 1)[0]
version_str = version_line.split(' ', 3)[2]
return Version(version_str)
except Exception as e:
raise ConanException("Error retrieving SVN version: '{}'".format(e))

@property
def version(self):
if not hasattr(self, '_version'):
@@ -1,8 +1,10 @@
import json
import os
import subprocess

from conans.client.tools.scm import Git, SVN
from conans.errors import ConanException
from conans.util.files import rmdir


def get_scm_data(conanfile):
@@ -86,7 +88,13 @@ def excluded_files(self):
def checkout(self):
output = ""
if self._data.type == "git":
output += self.repo.clone(url=self._data.url)
try:
output += self.repo.clone(url=self._data.url, branch=self._data.revision, shallow=True)
This conversation was marked as resolved by jgsogo

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jul 29, 2019

Member

⚠️ This is turning shallow=True into the default behavior, right? Can we think about a broken use-case?

This comment has been minimized.

Copy link
@SSE4

SSE4 Jul 29, 2019

Author Contributor

yeah, that was suggestion to use shallow by default, and fallback to the regular otherwise

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jul 29, 2019

Member

Oh! Ok, mention this change of the default behavior in the PR text, please. I get surprised by this kind of things if they are not stated before.

This comment has been minimized.

Copy link
@lasote

lasote Jul 29, 2019

Contributor

@jgsogo We decided to take the risk, we don't see it necessarily breaking but we are prepared to introduce opt-in/out in the recipe scm dict maybe.

except subprocess.CalledProcessError:
# remove the .git directory, otherwise, fallback clone cannot be successful
# it's completely safe to do here, as clone without branch expects empty directory
rmdir(os.path.join(self.repo_folder, ".git"))
output += self.repo.clone(url=self._data.url, shallow=False)
output += self.repo.checkout(element=self._data.revision,
submodule=self._data.submodule)
else:
@@ -2,8 +2,11 @@
import os
import six
import unittest
import subprocess

from mock import patch
from nose.plugins.attrib import attr
from parameterized import parameterized

from conans.client import tools
from conans.client.tools.scm import Git
@@ -31,6 +34,18 @@ def test_remove_credentials(self):
@attr('git')
class GitToolTest(unittest.TestCase):

@patch('subprocess.Popen')
def test_version(self, mocked_open):
mocked_open.return_value.communicate.return_value = ('git version 2.21.0'.encode(), None)
version = Git.get_version()
self.assertEqual(version, "2.21.0")

@patch('subprocess.Popen')
def test_version_invalid(self, mocked_open):
mocked_open.return_value.communicate.return_value = ('failed'.encode(), None)
with self.assertRaises(ConanException):
Git.get_version()

def test_repo_root(self):
root_path, _ = create_local_git_repo({"myfile": "anything"})

@@ -74,6 +89,61 @@ def test_clone_git(self):
git.clone(path)
self.assertTrue(os.path.exists(os.path.join(tmp, "myfile")))

@parameterized.expand([(None,), # default
("develop",), # branch name
("1.0",), # tag name
("HEAD",), # expression
])
def test_clone_git_shallow(self, element):
path, revision = create_local_git_repo({"myfile": "contents"}, commits=3, tags=["1.0"], branch="develop")
tmp = temp_folder()
git = Git(tmp)
git.clone("file://" + path, branch=element, shallow=True) # --depth is ignored in local clones
with self.assertRaises(subprocess.CalledProcessError):
git.checkout(element="HEAD~1")
self.assertTrue(os.path.exists(os.path.join(tmp, "myfile")))
This conversation was marked as resolved by SSE4

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jul 29, 2019

Member

I would like to check that the shallow has worked: git rev-list --all --count should be 1.

And also, is the output for all the parameterized use cases returning the same revision? Can we check git.get_revision() against the revision returned by the create_local_git_repo?

This comment has been minimized.

Copy link
@SSE4

SSE4 Jul 29, 2019

Author Contributor

okay, adding that

This comment has been minimized.

Copy link
@SSE4

SSE4 Jul 29, 2019

Author Contributor

added additional validation

self.assertEqual(git.get_revision(), revision)
self.assertEqual(git.run("rev-list --all --count"), "1")

def test_clone_git_shallow_revision(self):
path, revision = create_local_git_repo({"myfile": "contents"}, commits=3, tags=["1.0"], branch="develop")
tmp = temp_folder()
git = Git(tmp)
if Git.get_version() < "2.13":
# older Git versions have known bugs with "git fetch origin <sha>":
# https://github.com/git/git/blob/master/Documentation/RelNotes/2.13.0.txt
# * "git fetch" that requests a commit by object name, when the other
# side does not allow such an request, failed without much
# explanation.
# https://github.com/git/git/blob/master/Documentation/RelNotes/2.14.0.txt
# * There is no good reason why "git fetch $there $sha1" should fail
# when the $sha1 names an object at the tip of an advertised ref,
# even when the other side hasn't enabled allowTipSHA1InWant.
with self.assertRaises(subprocess.CalledProcessError):
git.clone("file://" + path, branch=revision, shallow=True)
else:
git.clone("file://" + path, branch=revision, shallow=True)
with self.assertRaises(subprocess.CalledProcessError):
git.checkout(element="HEAD~1")
self.assertTrue(os.path.exists(os.path.join(tmp, "myfile")))
This conversation was marked as resolved by SSE4

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jul 29, 2019

Member

I would like to check that I'm actually in the desired revision and that the shallow clone has worked:

    self.assertEquals(git.get_commit(), revision)
    self.assertEquals("git rev-list --all --count", 1)

This comment has been minimized.

Copy link
@SSE4

SSE4 Jul 29, 2019

Author Contributor

okay, adding that

This comment has been minimized.

Copy link
@SSE4

SSE4 Jul 29, 2019

Author Contributor

added additional validation

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jul 29, 2019

Member

I think you added them to the wrong place...

This comment has been minimized.

Copy link
@SSE4

SSE4 Jul 29, 2019

Author Contributor

what do you mean?

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jul 29, 2019

Member

In this block I think that the two last lines are not executed:

            with self.assertRaises(subprocess.CalledProcessError):
                git.clone("file://" + path, branch=revision, shallow=True)
                self.assertEqual(git.get_revision(), revision)
                self.assertEqual(git.run("rev-list --all --count"), "1")

You probably wanted to add them below:

        else:
            git.clone("file://" + path, branch=revision, shallow=True)
            with self.assertRaises(subprocess.CalledProcessError):
                git.checkout(element="HEAD~1")
            self.assertTrue(os.path.exists(os.path.join(tmp, "myfile")))

            self.assertEqual(git.get_revision(), revision)
            self.assertEqual(git.run("rev-list --all --count"), "1")

This comment has been minimized.

Copy link
@SSE4

SSE4 Jul 29, 2019

Author Contributor

ah, right

self.assertEqual(git.get_revision(), revision)
self.assertEqual(git.run("rev-list --all --count"), "1")

def test_clone_git_shallow_with_local(self):
path, revision = create_local_git_repo({"repofile": "contents"}, commits=3)
tmp = temp_folder()
save(os.path.join(tmp, "localfile"), "contents")
save(os.path.join(tmp, "indexfile"), "contents")
git = Git(tmp)
This conversation was marked as resolved by SSE4

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jul 29, 2019

Member

I would add the localfile to the repo:

Suggested change
git = Git(tmp)
git = Git(tmp)
git.run("add localfile")

This comment has been minimized.

Copy link
@SSE4

SSE4 Jul 29, 2019

Author Contributor

but that would be a different case, right? may be another test?

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jul 29, 2019

Member

if you want to check an untracked file too, just add another one in this test, I think it would be enough

This comment has been minimized.

Copy link
@SSE4

SSE4 Jul 29, 2019

Author Contributor

okay

This comment has been minimized.

Copy link
@SSE4

SSE4 Jul 29, 2019

Author Contributor

added

git.run("init")
git.run("add indexfile")
git.clone("file://" + path, branch="master", shallow=True) # --depth is ignored in local clones
self.assertTrue(os.path.exists(os.path.join(tmp, "repofile")))
self.assertTrue(os.path.exists(os.path.join(tmp, "localfile")))
self.assertTrue(os.path.exists(os.path.join(tmp, "indexfile")))
self.assertEqual(git.get_revision(), revision)
self.assertEqual(git.run("rev-list --all --count"), "1")

def test_clone_existing_folder_git(self):
path, commit = create_local_git_repo({"myfile": "contents"}, branch="my_release")

@@ -39,6 +39,20 @@ def test_remove_credentials(self):
@attr('svn')
class SVNToolTestsBasic(SVNLocalRepoTestCase):

@patch('subprocess.Popen')
def test_version(self, mocked_open):
svn_version_string = """svn, version 1.10.3 (r1842928)
compiled Apr 5 2019, 18:59:58 on x86_64-apple-darwin17.0.0"""
mocked_open.return_value.communicate.return_value = (svn_version_string.encode(), None)
version = SVN.get_version()
self.assertEqual(version, "1.10.3")

@patch('subprocess.Popen')
def test_version_invalid(self, mocked_open):
mocked_open.return_value.communicate.return_value = ('failed'.encode(), None)
with self.assertRaises(ConanException):
SVN.get_version()

def test_check_svn_repo(self):
project_url, _ = self.create_project(files={'myfile': "contents"})
tmp_folder = self.gimme_tmp()
@@ -495,7 +495,7 @@ def __contains__(self, value):
return value in self.__repr__()


def create_local_git_repo(files=None, branch=None, submodules=None, folder=None):
def create_local_git_repo(files=None, branch=None, submodules=None, folder=None, commits=1, tags=None):
tmp = folder or temp_folder()
tmp = get_cased_path(tmp)
if files:
@@ -509,7 +509,12 @@ def create_local_git_repo(files=None, branch=None, submodules=None, folder=None)
git.run("checkout -b %s" % branch)

git.run("add .")
git.run('commit -m "commiting"')
for i in range(0, commits):
git.run('commit --allow-empty -m "commiting"')

tags = tags or []
for tag in tags:
git.run("tag %s" % tag)

if submodules:
for submodule in submodules:
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.