launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-recipe-stale-implicit-revspecs into lp:launchpad.

Commit message:
Handle looking up Git recipes with implicit revspecs.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-recipe-stale-implicit-revspecs/+merge/285258

Handle looking up Git recipes with implicit revspecs.  These refer to the default branch ("HEAD"), but were previously incorrectly not marked as stale when pushing to the default branch.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-recipe-stale-implicit-revspecs into lp:launchpad.

=== modified file 'lib/lp/code/model/gitref.py'
--- lib/lp/code/model/gitref.py	2016-01-12 03:54:38 +0000
+++ lib/lp/code/model/gitref.py	2016-02-06 02:53:48 +0000
@@ -249,8 +249,11 @@
         from lp.code.model.sourcepackagerecipedata import (
             SourcePackageRecipeData,
             )
+        revspecs = set([self.path, self.name])
+        if self.path == self.repository.default_branch:
+            revspecs.add(None)
         recipes = SourcePackageRecipeData.findRecipes(
-            self.repository, revspecs=list(set([self.path, self.name])))
+            self.repository, revspecs=list(revspecs))
         hook = SourcePackageRecipe.preLoadDataForSourcePackageRecipes
         return DecoratedResultSet(recipes, pre_iter_hook=hook)
 

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2016-01-12 04:34:09 +0000
+++ lib/lp/code/model/gitrepository.py	2016-02-06 02:53:48 +0000
@@ -965,6 +965,8 @@
                 revspecs.add(path)
                 if path.startswith("refs/heads/"):
                     revspecs.add(path[len("refs/heads/"):])
+                if path == self.default_branch:
+                    revspecs.add(None)
             revspecs = list(revspecs)
         else:
             revspecs = None

=== modified file 'lib/lp/code/model/sourcepackagerecipedata.py'
--- lib/lp/code/model/sourcepackagerecipedata.py	2016-01-15 11:43:28 +0000
+++ lib/lp/code/model/sourcepackagerecipedata.py	2016-02-06 02:53:48 +0000
@@ -31,6 +31,7 @@
     And,
     In,
     Int,
+    Or,
     Reference,
     ReferenceSet,
     Select,
@@ -271,11 +272,21 @@
             raise AssertionError(
                 "Unsupported source: %r" % (branch_or_repository,))
         if revspecs is not None:
-            data_clause = And(
-                data_clause, In(SourcePackageRecipeData.revspec, revspecs))
-            insn_clause = And(
-                insn_clause,
-                In(_SourcePackageRecipeDataInstruction.revspec, revspecs))
+            concrete_revspecs = [
+                revspec for revspec in revspecs if revspec is not None]
+            data_revspec_clause = In(
+                SourcePackageRecipeData.revspec, concrete_revspecs)
+            insn_revspec_clause = In(
+                _SourcePackageRecipeDataInstruction.revspec, concrete_revspecs)
+            if None in revspecs:
+                data_revspec_clause = Or(
+                    data_revspec_clause,
+                    SourcePackageRecipeData.revspec == None)
+                insn_revspec_clause = Or(
+                    insn_revspec_clause,
+                    _SourcePackageRecipeDataInstruction.revspec == None)
+            data_clause = And(data_clause, data_revspec_clause)
+            insn_clause = And(insn_clause, insn_revspec_clause)
         return store.find(
             SourcePackageRecipe,
             SourcePackageRecipe.id.is_in(Union(

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2016-02-05 16:51:12 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2016-02-06 02:53:48 +0000
@@ -1873,8 +1873,24 @@
             {ref2.path: {u"sha1": u"0" * 40, u"type": GitObjectType.COMMIT}})
         self.assertFalse(recipe.is_stale)
 
+    def test_base_repository_default_branch_recipe(self):
+        # On ref changes to the default branch, recipes where this
+        # repository is the base with no explicit revspec become stale.
+        repository = self.factory.makeGitRepository()
+        ref1, ref2 = self.factory.makeGitRefs(
+            repository=repository, paths=[u"refs/heads/a", u"refs/heads/b"])
+        removeSecurityProxy(repository)._default_branch = u"refs/heads/a"
+        recipe = self.factory.makeSourcePackageRecipe(branches=[repository])
+        removeSecurityProxy(recipe).is_stale = False
+        repository.createOrUpdateRefs(
+            {ref2.path: {u"sha1": u"0" * 40, u"type": GitObjectType.COMMIT}})
+        self.assertFalse(recipe.is_stale)
+        repository.createOrUpdateRefs(
+            {ref1.path: {u"sha1": u"0" * 40, u"type": GitObjectType.COMMIT}})
+        self.assertTrue(recipe.is_stale)
+
     def test_instruction_repository_recipe(self):
-        # On ref changes, recipes including this repository become stale.
+        # On ref changes, recipes including this ref become stale.
         [base_ref] = self.factory.makeGitRefs()
         [ref] = self.factory.makeGitRefs()
         recipe = self.factory.makeSourcePackageRecipe(branches=[base_ref, ref])
@@ -1896,6 +1912,24 @@
             {ref2.path: {u"sha1": u"0" * 40, u"type": GitObjectType.COMMIT}})
         self.assertFalse(recipe.is_stale)
 
+    def test_instruction_repository_default_branch_recipe(self):
+        # On ref changes to the default branch, recipes including this
+        # repository with no explicit revspec become stale.
+        [base_ref] = self.factory.makeGitRefs()
+        repository = self.factory.makeGitRepository()
+        ref1, ref2 = self.factory.makeGitRefs(
+            repository=repository, paths=[u"refs/heads/a", u"refs/heads/b"])
+        removeSecurityProxy(repository)._default_branch = u"refs/heads/a"
+        recipe = self.factory.makeSourcePackageRecipe(
+            branches=[base_ref, repository])
+        removeSecurityProxy(recipe).is_stale = False
+        repository.createOrUpdateRefs(
+            {ref2.path: {u"sha1": u"0" * 40, u"type": GitObjectType.COMMIT}})
+        self.assertFalse(recipe.is_stale)
+        repository.createOrUpdateRefs(
+            {ref1.path: {u"sha1": u"0" * 40, u"type": GitObjectType.COMMIT}})
+        self.assertTrue(recipe.is_stale)
+
     def test_unrelated_repository_recipe(self):
         # On ref changes, unrelated recipes are left alone.
         [ref] = self.factory.makeGitRefs()

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2016-02-02 18:26:28 +0000
+++ lib/lp/testing/factory.py	2016-02-06 02:53:48 +0000
@@ -121,6 +121,7 @@
 from lp.code.interfaces.codeimportresult import ICodeImportResultSet
 from lp.code.interfaces.gitnamespace import get_git_namespace
 from lp.code.interfaces.gitref import IGitRef
+from lp.code.interfaces.gitrepository import IGitRepository
 from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
 from lp.code.interfaces.revision import IRevisionSet
 from lp.code.interfaces.sourcepackagerecipe import (
@@ -2902,6 +2903,13 @@
         other_branches = branches[1:]
         if IBranch.providedBy(base_branch):
             text = MINIMAL_RECIPE_TEXT_BZR % base_branch.identity
+        elif IGitRepository.providedBy(base_branch):
+            # The UI normally guides people towards using an explicit branch
+            # name, but it's also possible to leave the branch name empty
+            # which is equivalent to the repository's default branch.  This
+            # makes that mode easier to test.
+            text = "%s\n%s\n" % (
+                MINIMAL_RECIPE_TEXT_GIT.splitlines()[0], base_branch.identity)
         elif IGitRef.providedBy(base_branch):
             text = MINIMAL_RECIPE_TEXT_GIT % (
                 base_branch.repository.identity, base_branch.name)
@@ -2909,7 +2917,7 @@
             raise AssertionError(
                 "Unsupported base_branch: %r" % (base_branch,))
         for i, branch in enumerate(other_branches):
-            if IBranch.providedBy(branch):
+            if IBranch.providedBy(branch) or IGitRepository.providedBy(branch):
                 text += 'merge dummy-%s %s\n' % (i, branch.identity)
             elif IGitRef.providedBy(branch):
                 text += 'merge dummy-%s %s %s\n' % (