Merge pull request #285 from jakub-nt/CFE-4089

Code quality improvements in preparation for CFE-4089

Author: olehermanse

cfbs/cfbs_config.py

@@ -25,6 +25,7 @@
 from cfbs.utils import (
     CFBSExitError,
     CFBSUserError,
+    is_a_commit_hash,
     read_file,
     write_json,
     load_bundlenames,
@@ -149,16 +150,25 @@ def _add_using_url(
         checksum=None,
         explicit_build_steps=None,
     ):
+        """The `url` argument can optionally also have the form `<url>@<commit>`."""
         url_commit = None
         if url.endswith(SUPPORTED_ARCHIVES):
             config_path, url_commit = fetch_archive(url, checksum)
         else:
             assert url.startswith(SUPPORTED_URI_SCHEMES)
-            config_path, url_commit = clone_url_repo(url)
 
-        if "@" in url and (url.rindex("@") > url.rindex(".")):
-            assert url.split("@")[-1] == url_commit
-            url = url[0 : url.rindex("@")]
+            commit = None
+            if "@" in url and (url.rindex("@") > url.rindex(".")):
+                # commit specified in the url
+                url, commit = url.rsplit("@", 1)
+                if "@" in url and (url.rindex("@") > url.rindex(".")):
+                    raise CFBSUserError(
+                        "Cannot specify more than one commit for one add URL"
+                    )
+                if not is_a_commit_hash(commit):
+                    raise CFBSExitError("'%s' is not a commit reference" % commit)
+
+            config_path, url_commit = clone_url_repo(url, commit)
 
         remote_config = CFBSJson(path=config_path, url=url, url_commit=url_commit)
 

cfbs/commands.py

@@ -262,7 +262,6 @@ def init_command(
     """
     CFBSConfig.reload()
 
-    branch = None
     to_add = []
     if masterfiles is None:
         if prompt_user_yesno(
@@ -282,16 +281,8 @@ def init_command(
         log.debug("--masterfiles=%s appears to be a version number" % masterfiles)
         to_add = ["masterfiles@%s" % masterfiles]
     elif masterfiles != "no":
-        """This appears to be a branch. Thus we'll add masterfiles normally
-        and try to do the necessary modifications needed afterwards. I.e.
-        changing the 'repo' attribute to be 'url' and changing the commit to
-        be the current HEAD of the upstream branch."""
-
         log.debug("--masterfiles=%s appears to be a branch" % masterfiles)
         branch = masterfiles
-        to_add = ["masterfiles"]
-
-    if branch is not None:
         remote = "https://github.com/cfengine/masterfiles"
         commit = ls_remote(remote, branch)
         if commit is None:

cfbs/git.py

@@ -33,6 +33,7 @@ def git_exists():
 def ls_remote(remote, branch):
     """Returns the hash of the commit that the current HEAD of a given branch
     on a given remote is pointing to.
+    Returns `None` in case of error (e.g. the branch does not exist).
 
     :param remote: the remote to list
     :param branch: the branch on the remote

cfbs/internal_file_management.py

@@ -11,6 +11,7 @@
 import os
 import re
 import shutil
+from typing import Optional
 
 from cfbs.utils import (
     cfbs_dir,
@@ -148,22 +149,20 @@ def _get_git_repo_commit_sha(repo_path):
         return f.read().strip()
 
 
-def _clone_and_checkout(url, path, commit):
+def _clone_and_checkout(url, path, treeish):
     # NOTE: If any of these shell (git) commands fail, we will exit
     if not os.path.exists(os.path.join(path, ".git")):
         sh("git clone --no-checkout %s %s" % (url, path))
-    sh("git checkout " + commit, directory=path)
+    sh("git checkout " + treeish, directory=path)
 
 
-def clone_url_repo(repo_url: str):
-    assert repo_url.startswith(SUPPORTED_URI_SCHEMES)
+def clone_url_repo(repo_url: str, commit: Optional[str] = None):
+    """Clones a Git repository at `repo_url` URL, optionally checking out the `commit` commit.
 
-    commit = None
-    if "@" in repo_url and (repo_url.rindex("@") > repo_url.rindex(".")):
-        # commit specified in the url
-        repo_url, commit = repo_url.rsplit("@", 1)
-        if not is_a_commit_hash(commit):
-            raise CFBSExitError("'%s' is not a commit reference" % commit)
+    Returns path to the `cfbs.json` located in the cloned Git repository, and the Git commit hash.
+    """
+    assert repo_url.startswith(SUPPORTED_URI_SCHEMES)
+    assert "@" not in repo_url or (repo_url.rindex("@") < repo_url.rindex("."))
 
     downloads = os.path.join(cfbs_dir(), "downloads")
 

cfbs/module.py

@@ -1,6 +1,7 @@
 from collections import OrderedDict
 
 from cfbs.pretty import pretty
+from cfbs.utils import CFBSUserError
 
 
 def is_module_added_manually(added_by: str):
@@ -24,6 +25,10 @@ def __init__(
         """Initialize from argument with format `NAME[@VERSION]`"""
         assert isinstance(arg, str)
         if "@" in arg:
+            if arg.count("@") > 1:
+                raise CFBSUserError(
+                    "Cannot specify more than one version of the same module"
+                )
             self.name, self.version = arg.split("@")
         else:
             self.name = arg