launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging ~wgrant/git-build-recipe:bug-1542673 into git-build-recipe:master.

Commit message:
Fix substitution variables to always use the original source commits.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/git-build-recipe/+git/git-build-recipe/+merge/286463

Fix substitution variables to always use the original source commits.

Fetching is rewritten in a unified manner. We manually fetch each branch into its own namespace, and then detach head so we don't mutate any of the refs, so we don't affect any possible future substitutions.

A few parts are still a little dodgy, but they were just as dodgy before this:

 - "checkout" is a bit too porcelainy and  doesn't let us force a commit rather than branch lookup. If someone creates a branch named after a commit hash, confusion will ensure.

 - "rev-parse" doesn't allow namespace restrictions, so a child branch's revspec can reference anything visible in its target repo by that point.

 - Only tags included in branch history are available to refspecs and latest-tag.

 - Child branches with tags that conflict with other branches in the same location may cause confusion.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~wgrant/git-build-recipe:bug-1542673 into git-build-recipe:master.

diff --git a/gitbuildrecipe/recipe.py b/gitbuildrecipe/recipe.py
index 1d0faa3..f1ea7d0 100644
--- a/gitbuildrecipe/recipe.py
+++ b/gitbuildrecipe/recipe.py
@@ -159,7 +159,7 @@ class RevisionVariable(BranchSubstitutionVariable):
     def __init__(self, branch):
         super().__init__(branch.name)
         self.branch = branch
-        self.commit = branch.get_revspec()
+        self.commit = branch.commit
 
 
 class RevnoVariable(RevisionVariable):
@@ -290,64 +290,44 @@ insteadof = {
     }
 
 
-def git_clone(url, target_path):
-    cmd = ["git"]
-    for short, base in insteadof.items():
-        cmd.extend(["-c", "url.%s.insteadOf=%s" % (base, short)])
-    cmd.extend(["clone", "-n", url, target_path])
-    try:
-        output = subprocess.check_output(
-            cmd, stderr=subprocess.STDOUT, universal_newlines=True)
-        sys.stdout.write(output)
-    except subprocess.CalledProcessError as e:
-        raise CloneFailed(e.output)
-    for short, base in insteadof.items():
-        subprocess.check_call(
-            ["git", "-C", target_path, "config", "url.%s.insteadOf" % base,
-             short])
-
-
 def pull_or_clone(base_branch, target_path):
     """Make target_path match base_branch, either by pulling or by cloning."""
     base_branch.path = target_path
-    if os.path.exists(target_path):
-        if not os.path.exists(os.path.join(target_path, ".git")):
-            raise TargetAlreadyExists(target_path)
-        url = base_branch.url
-        parsed_url = urlparse(url)
-        if not parsed_url.scheme and not parsed_url.path.startswith("/"):
-            url = os.path.abspath(url)
-        try:
-            base_branch.git_call("fetch", url)
-        except subprocess.CalledProcessError as e:
-            raise FetchFailed(e.output)
-        if base_branch.revspec is not None:
-            revspec = base_branch.revspec
-        else:
-            revspec = "FETCH_HEAD"
-    else:
-        git_clone(base_branch.url, target_path)
-        revspec = base_branch.get_revspec()
+    if not os.path.exists(target_path):
+        os.makedirs(target_path)
+        base_branch.git_call("init")
+    elif not os.path.exists(os.path.join(target_path, ".git")):
+        raise TargetAlreadyExists(target_path)
     try:
-        base_branch.git_call("checkout", revspec)
+        fetch_branches(base_branch)
+    except subprocess.CalledProcessError as e:
+        raise FetchFailed(e.output)
+    try:
+        base_branch.resolve_commit()
+        # Check out the commit hash directly to detach HEAD and ensure
+        # commits (eg. by a merge instruction) don't affect substitution
+        # variables.
+        # If someone sneakily names a branch after a commit then all
+        # hell will break loose (really, git? there's no way to force
+        # checkout to interpret its argument as a commit?), but that's
+        # their problem.
+        base_branch.git_call("checkout", base_branch.commit)
     except subprocess.CalledProcessError as e:
         raise CheckoutFailed(e.output)
 
 
-def ensure_remote(child_branch):
+def fetch_branches(child_branch):
     url = child_branch.url
     parsed_url = urlparse(url)
     if not parsed_url.scheme and not parsed_url.path.startswith("/"):
         url = os.path.abspath(url)
-    try:
-        child_branch.git_call(
-            "config", "remote.%s.url" % child_branch.name, silent=True)
-    except subprocess.CalledProcessError:
-        child_branch.git_call("remote", "add", child_branch.name, url)
-    else:
-        child_branch.git_call("remote", "set-url", child_branch.name, url)
-    child_branch.git_call("remote", "update", child_branch.name)
-    child_branch.git_call("remote", "set-head", child_branch.name, "--auto")
+    # Fetch all remote branches, the remote HEAD, and (implicitly) any
+    # tags that reference commits in those refs. Tags that aren't on a
+    # branch won't be fetched.
+    child_branch.git_call(
+        "fetch", url,
+        "HEAD:refs/remotes/%s/HEAD" % child_branch.remote_name,
+        "refs/heads/*:refs/remotes/%s/*" % child_branch.remote_name)
 
 
 def merge_branch(child_branch, target_path):
@@ -358,7 +338,7 @@ def merge_branch(child_branch, target_path):
     :param target_path: The tree to merge into.
     """
     child_branch.path = target_path
-    ensure_remote(child_branch)
+    fetch_branches(child_branch)
     try:
         child_branch.resolve_commit()
         child_branch.git_call(
@@ -386,12 +366,12 @@ def nest_part_branch(child_branch, target_path, subpath, target_subdir=None):
     # XXX should handle updating as well
     assert not os.path.exists(target_subdir)
     child_branch.path = target_path
-    ensure_remote(child_branch)
+    fetch_branches(child_branch)
     child_branch.resolve_commit()
     subtree_commit = child_branch.git_output(
         "subtree", "split", "-P", subpath, child_branch.commit)
     child_branch.path = os.path.join(target_path, target_subdir)
-    git_clone(".", child_branch.path)
+    git_clone(".", child_branch.path, child_branch.remote_name)
     child_branch.git_call("checkout", subtree_commit)
 
 
@@ -628,6 +608,12 @@ class RecipeBranch:
         self.commit = None
         self.path = None
 
+    @property
+    def remote_name(self):
+        if self.name is None:
+            return 'source'
+        return 'source-%s' % self.name
+
     def _get_git_path(self):
         if self.path is not None:
             return self.path
@@ -669,14 +655,16 @@ class RecipeBranch:
         # Capturing stderr is a bit dodgy, but it's the most convenient way
         # to capture it for any exceptions.  We know that git rev-parse does
         # not write to stderr on success.
-        if self.name is not None:
-            try:
-                self.commit = self.git_output(
-                    "rev-parse", "%s/%s" % (self.name, self.get_revspec()),
-                    stderr=subprocess.STDOUT).rstrip("\n")
-                return
-            except subprocess.CalledProcessError:
-                pass
+        try:
+            self.commit = self.git_output(
+                "rev-parse", "%s/%s" % (self.remote_name, self.get_revspec()),
+                stderr=subprocess.STDOUT).rstrip("\n")
+            return
+        except subprocess.CalledProcessError:
+            pass
+        # Not a remote-prefixed ref. Try a global search.
+        # XXX: This allows cross-branch pollution, but we don't have
+        # much choice without reimplementing rev-parse ourselves.
         self.commit = self.git_output(
             "rev-parse", self.get_revspec(),
             stderr=subprocess.STDOUT).rstrip("\n")
diff --git a/gitbuildrecipe/tests/test_deb_version.py b/gitbuildrecipe/tests/test_deb_version.py
index fdb814b..f58f56f 100644
--- a/gitbuildrecipe/tests/test_deb_version.py
+++ b/gitbuildrecipe/tests/test_deb_version.py
@@ -31,6 +31,7 @@ from gitbuildrecipe.deb_version import (
     )
 from gitbuildrecipe.recipe import (
     BaseRecipeBranch,
+    build_tree,
     RecipeBranch,
     resolve_revisions,
     )
@@ -248,6 +249,25 @@ class ResolveRevisionsTests(GitTestCase):
             branch2, substitute_branch_vars=substitute_branch_vars)
         self.assertEqual("foo-2", branch2.deb_version)
 
+    def test_uses_source_commit(self):
+        # The original commit is used for variable substitution, even if
+        # the base branch was committed to by instructions such as merge.
+        source = GitRepository("source")
+        source.commit("one")
+        source._git_call("checkout", "-q", "-b", "branch")
+        source.build_tree(["a"])
+        source.add(["a"])
+        source.commit("two")
+        source.commit("three")
+        branch1 = BaseRecipeBranch(
+            "source", "foo-{revno}+{revno:branch}", 0.1, revspec="master")
+        branch2 = RecipeBranch("branch", "source", revspec="branch")
+        branch1.merge_branch(branch2)
+        build_tree(branch1, "target")
+        resolve_revisions(
+            branch1, substitute_branch_vars=substitute_branch_vars)
+        self.assertEqual("foo-1+3", branch1.deb_version)
+
 
 class DebUpstreamVariableTests(GitTestCase):
 
@@ -378,17 +398,21 @@ class RecipeBranchTests(GitTestCase):
         self.assertEqual("1", base_branch.deb_version)
         base_branch = BaseRecipeBranch(
             "base_url", "{revdate}", 0.4, revspec=commit1)
+        base_branch.resolve_commit()
         substitute_branch_vars(base_branch, base_branch)
         self.assertEqual("20150101", base_branch.deb_version)
         base_branch = BaseRecipeBranch(
             "base_url", "{revdate}", 0.4, revspec=commit1)
+        base_branch.resolve_commit()
         child_branch = RecipeBranch("foo", "base_url", revspec=commit2)
+        child_branch.resolve_commit()
         substitute_branch_vars(base_branch, child_branch)
         self.assertEqual("{revdate}", base_branch.deb_version)
         substitute_branch_vars(base_branch, child_branch)
         self.assertEqual("{revdate}", base_branch.deb_version)
         base_branch = BaseRecipeBranch(
             "base_url", "{revdate:foo}", 0.4, revspec=commit1)
+        base_branch.resolve_commit()
         substitute_branch_vars(base_branch, child_branch)
         self.assertEqual("20150102", base_branch.deb_version)