Merge branch 'fix-checkupdates-one-MR-per-app' into 'master'

checkupdates: reuse per-app branches when making merge requests; plus bug fixes

See merge request fdroid/fdroidserver!1551

fdroidserver/checkupdates.py

@@ -18,6 +18,7 @@
 # You should have received a copy of the GNU Affero General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import configparser
 import git
 import os
 import re
@@ -41,6 +42,10 @@ from . import net
 from .exception import VCSException, NoSubmodulesException, FDroidException, MetaDataException
 
 
+# https://gitlab.com/fdroid/checkupdates-runner/-/blob/1861899262a62a4ed08fa24e5449c0368dfb7617/.gitlab-ci.yml#L36
+BOT_EMAIL = 'fdroidci@bubu1.eu'
+
+
 def check_http(app: metadata.App) -> tuple[Optional[str], Optional[int]]:
     """Check for a new version by looking at a document retrieved via HTTP.
 
@@ -684,17 +689,73 @@ def get_last_build_from_app(app: metadata.App) -> metadata.Build:
         return metadata.Build()
 
 
-def get_git_repo_and_main_branch():
+def get_upstream_main_branch(git_repo):
+    if len(git_repo.remotes.upstream.refs) == 1:
+        return git_repo.remotes.upstream.refs[0].name
+    for name in ('main', 'master'):
+        if name in git_repo.remotes.upstream.refs:
+            return f'upstream/{name}'
+    try:
+        with git_repo.config_reader() as reader:
+            return 'upstream/%s' % reader.get_value('init', 'defaultBranch')
+    except configparser.NoSectionError:
+        return 'upstream/main'
+
+
+def checkout_appid_branch(appid):
+    """Prepare the working branch named after the appid.
+
+    This sets up everything for checkupdates_app() to run and add
+    commits.  If there is an existing branch named after the appid,
+    and it has commits from users other than the checkupdates-bot,
+    then this will return False.  Otherwise, it returns True.
+
+    The checkupdates-runner must set the committer email address in
+    the git config.  Then any commit with a committer or author that
+    does not match that will be considered to have human edits.  That
+    email address is currently set in:
+    https://gitlab.com/fdroid/checkupdates-runner/-/blob/1861899262a62a4ed08fa24e5449c0368dfb7617/.gitlab-ci.yml#L36
+
+    """
+    logging.debug(f'Creating merge request branch for {appid}')
     git_repo = git.Repo.init('.')
-    main_branch = 'main'
+    upstream_main = get_upstream_main_branch(git_repo)
-    if main_branch not in git_repo.heads:
+    for remote in git_repo.remotes:
-        main_branch = 'master'
+        remote.fetch()
-    return git_repo, main_branch
+    try:
+        git_repo.remotes.origin.fetch(f'{appid}:refs/remotes/origin/{appid}')
+    except Exception as e:
+        logging.warning('"%s" branch not found on origin remote:\n\t%s', appid, e)
+    if appid in git_repo.remotes.origin.refs:
+        start_point = f"origin/{appid}"
+        for commit in git_repo.iter_commits(
+            f'{upstream_main}...{start_point}', right_only=True
+        ):
+            if commit.committer.email != BOT_EMAIL or commit.author.email != BOT_EMAIL:
+                return False
+    else:
+        start_point = upstream_main
+    git_repo.git.checkout('-B', appid, start_point)
+    git_repo.git.rebase(upstream_main, strategy_option='ours', kill_after_timeout=120)
+    return True
 
 
-def push_commits(remote_name='origin', branch_name='checkupdates', verbose=False):
+def get_changes_versus_ref(git_repo, ref, f):
+    changes = []
+    for m in re.findall(
+        r"^[+-].*", git_repo.git.diff(f"{ref}", '--', f), flags=re.MULTILINE
+    ):
+        if not re.match(r"^(\+\+\+|---) ", m):
+            changes.append(m)
+    return changes
+
+
+def push_commits(branch_name='checkupdates', verbose=False):
     """Make git branch then push commits as merge request.
 
+    The appid is parsed from the actual file that was changed so that
+    only the right branch is ever updated.
+
     This uses the appid as the standard branch name so that there is
     only ever one open merge request per-app.  If multiple apps are
     included in the branch, then 'checkupdates' is used as branch
@@ -709,11 +770,10 @@ def push_commits(remote_name='origin', branch_name='checkupdates', verbose=False
     * https://docs.gitlab.com/ee/user/project/push_options.html
 
     """
-    git_repo, default = get_git_repo_and_main_branch()
+    git_repo = git.Repo.init('.')
+    upstream_main = get_upstream_main_branch(git_repo)
     files = set()
-    upstream_main = default if default in git_repo.remotes.upstream.refs else 'main'
+    for commit in git_repo.iter_commits(f'{upstream_main}...HEAD', right_only=True):
-    local_main = default if default in git_repo.refs else 'main'
-    for commit in git_repo.iter_commits(f'upstream/{upstream_main}...{local_main}'):
         files.update(commit.stats.files.keys())
 
     files = list(files)
@@ -723,6 +783,30 @@ def push_commits(remote_name='origin', branch_name='checkupdates', verbose=False
             branch_name = m.group(1)  # appid
     if not files:
         return
+
+    remote = git_repo.remotes.origin
+    if branch_name in remote.refs:
+        if not get_changes_versus_ref(git_repo, f'origin/{branch_name}', files[0]):
+            return
+
+    git_repo.create_head(branch_name, force=True)
+    push_options = [
+        'merge_request.create',
+        'merge_request.remove_source_branch',
+        'merge_request.title=bot: ' + git_repo.branches[branch_name].commit.summary,
+        'merge_request.description='
+        + '~%s checkupdates-bot run %s' % (branch_name, os.getenv('CI_JOB_URL')),
+    ]
+
+    # mark as draft if there are only changes to CurrentVersion:
+    current_version_only = True
+    for m in get_changes_versus_ref(git_repo, upstream_main, files[0]):
+        if not re.match(r"^[-+]CurrentVersion", m):
+            current_version_only = False
+            break
+    if current_version_only:
+        push_options.append('merge_request.draft')
+
     progress = None
     if verbose:
         import clint.textui
@@ -736,23 +820,12 @@ def push_commits(remote_name='origin', branch_name='checkupdates', verbose=False
 
         progress = MyProgressPrinter()
 
-    git_repo.create_head(branch_name, force=True)
-    remote = git_repo.remotes[remote_name]
-    pushinfos = remote.push(
-        branch_name, force=True, set_upstream=True, progress=progress
-    )
     pushinfos = remote.push(
         branch_name,
         progress=progress,
         force=True,
         set_upstream=True,
-        push_option=[
+        push_option=push_options,
-            'merge_request.create',
-            'merge_request.remove_source_branch',
-            'merge_request.title=' + 'bot: checkupdates for ' + branch_name,
-            'merge_request.description='
-            + 'checkupdates-bot run %s' % os.getenv('CI_JOB_URL'),
-        ],
     )
 
     for pushinfo in pushinfos:
@@ -777,8 +850,9 @@ def push_commits(remote_name='origin', branch_name='checkupdates', verbose=False
 def prune_empty_appid_branches(git_repo=None, main_branch='main'):
     """Remove empty branches from checkupdates-bot git remote."""
     if git_repo is None:
-        git_repo, main_branch = get_git_repo_and_main_branch()
+        git_repo = git.Repo.init('.')
-    upstream_main = 'upstream/' + main_branch
+    upstream_main = get_upstream_main_branch(git_repo)
+    main_branch = upstream_main.split('/')[1]
 
     remote = git_repo.remotes.origin
     remote.update(prune=True)
@@ -861,10 +935,14 @@ def main():
 
         try:
             if options.merge_request:
-                logging.info(f'Creating merge request branch for {appid}')
+                if not checkout_appid_branch(appid):
-                git_repo, main_branch = get_git_repo_and_main_branch()
+                    msg = _("...checkupdate failed for {appid} : {error}").format(
-                git_repo.create_head(appid, f"upstream/{main_branch}", force=True)
+                        appid=appid,
-                git_repo.git.checkout(appid)
+                        error='Open merge request with human edits, skipped.',
+                    )
+                    logging.warning(msg)
+                    failed[appid] = msg
+                    continue
 
             checkupdates_app(app, options.auto, options.commit or options.merge_request)
             processed.append(appid)

tests/test_checkupdates.py

@@ -324,6 +324,9 @@ class CheckupdatesTest(unittest.TestCase):
         for f in (basedir / 'metadata').glob('*.yml'):
             shutil.copy(f, 'metadata')
         git_repo = git.Repo.init(testdir)
+        with git_repo.config_writer() as cw:
+            cw.set_value('user', 'name', 'Foo Bar')
+            cw.set_value('user', 'email', 'foo@bar.com')
         git_repo.git.add(all=True)
         git_repo.index.commit("all metadata files")
 
@@ -341,6 +344,40 @@ class CheckupdatesTest(unittest.TestCase):
 
         return git_repo, origin_repo, upstream_repo
 
+    def test_get_changes_versus_ref(self):
+        def _make_commit_new_app(git_repo, metadata_file):
+            app = fdroidserver.metadata.App()
+            fdroidserver.metadata.write_metadata(metadata_file, app)
+            git_repo.git.add(metadata_file)
+            git_repo.git.commit(metadata_file, message=f'changed {metadata_file}')
+
+        git_repo, origin_repo, upstream_repo = self._get_test_git_repos()
+        for remote in git_repo.remotes:
+            remote.push(git_repo.active_branch)
+        appid = 'com.testvalue'
+        metadata_file = f'metadata/{appid}.yml'
+
+        # set up remote branch with change to app
+        git_repo.git.checkout('-b', appid)
+        _make_commit_new_app(git_repo, metadata_file)
+        git_repo.remotes.origin.push(appid)
+
+        # reset local branch and there should be differences
+        upstream_main = fdroidserver.checkupdates.get_upstream_main_branch(git_repo)
+        git_repo.git.reset(upstream_main)
+        self.assertTrue(
+            fdroidserver.checkupdates.get_changes_versus_ref(
+                git_repo, f'origin/{appid}', metadata_file
+            )
+        )
+        # make new commit that matches the previous, different commit, no diff
+        _make_commit_new_app(git_repo, metadata_file)
+        self.assertFalse(
+            fdroidserver.checkupdates.get_changes_versus_ref(
+                git_repo, f'origin/{appid}', metadata_file
+            )
+        )
+
     def test_push_commits(self):
         git_repo, origin_repo, upstream_repo = self._get_test_git_repos()
         for remote in git_repo.remotes:
@@ -453,21 +490,173 @@ class CheckupdatesTest(unittest.TestCase):
                 fdroidserver.checkupdates.main()
         sys_exit.assert_not_called()
 
-    def test_get_git_repo_and_main_branch(self):
+    def test_get_upstream_main_branch(self):
         os.chdir(self.testdir.name)
-        git_repo = git.Repo.init()
+        testvalue = 'foo'
+        git_repo = git.Repo.init('.', initial_branch=testvalue)
+
         open('foo', 'w').close()
         git_repo.git.add(all=True)
         git_repo.index.commit("all files")
+        git_repo.create_remote('upstream', os.getcwd()).fetch()
 
-        repo, branch = fdroidserver.checkupdates.get_git_repo_and_main_branch()
+        branch = fdroidserver.checkupdates.get_upstream_main_branch(git_repo)
-        self.assertTrue(branch in ('main', 'master'))
+        self.assertEqual(
-        self.assertTrue(branch in repo.heads)
+            f'upstream/{testvalue}',
+            branch,
+            f'The default branch should be called {testvalue}!',
+        )
 
+    def test_get_upstream_main_branch_git_config(self):
+        os.chdir(self.testdir.name)
+        testvalue = 'foo'
+        git_repo = git.Repo.init('.', initial_branch=testvalue)
+        with git_repo.config_writer() as cw:
+            cw.set_value('init', 'defaultBranch', testvalue)
+
+        open('foo', 'w').close()
+        git_repo.git.add(all=True)
+        git_repo.index.commit("all files")
+        git_repo.git.branch('somethingelse')  # make another remote branch
+        git_repo.create_remote('upstream', os.getcwd()).fetch()
+
+        branch = fdroidserver.checkupdates.get_upstream_main_branch(git_repo)
+        self.assertEqual(
+            f'upstream/{testvalue}',
+            branch,
+            f'The default branch should be called {testvalue}!',
+        )
+
+    def test_checkout_appid_branch_does_not_exist(self):
+        appid = 'com.example'
+        os.chdir(self.testdir.name)
+        git_repo = git.Repo.init('.')
+        open('foo', 'w').close()
+        git_repo.git.add(all=True)
+        git_repo.index.commit("all files")
+        # --merge-request assumes remotes called 'origin' and 'upstream'
+        git_repo.create_remote('origin', os.getcwd()).fetch()
+        git_repo.create_remote('upstream', os.getcwd()).fetch()
+        self.assertNotIn(appid, git_repo.heads)
+        fdroidserver.checkupdates.checkout_appid_branch(appid)
+        self.assertIn(appid, git_repo.heads)
+
+    def test_checkout_appid_branch_exists(self):
+        appid = 'com.example'
+
+        upstream_dir = os.path.join(self.testdir.name, 'upstream_git')
+        os.mkdir(upstream_dir)
+        upstream_repo = git.Repo.init(upstream_dir)
+        (Path(upstream_dir) / 'README').write_text('README')
+        upstream_repo.git.add(all=True)
+        upstream_repo.index.commit("README")
+        upstream_repo.create_head(appid)
+
+        local_dir = os.path.join(self.testdir.name, 'local_git')
+        git.Repo.clone_from(upstream_dir, local_dir)
+        os.chdir(local_dir)
+        git_repo = git.Repo.init('.')
+        # --merge-request assumes remotes called 'origin' and 'upstream'
+        git_repo.create_remote('upstream', upstream_dir).fetch()
+
+        self.assertNotIn(appid, git_repo.heads)
+        fdroidserver.checkupdates.checkout_appid_branch(appid)
+        self.assertIn(appid, git_repo.heads)
+
+    def test_checkout_appid_branch_skip_bot_commit(self):
+        appid = 'com.example'
+
+        upstream_dir = os.path.join(self.testdir.name, 'upstream_git')
+        os.mkdir(upstream_dir)
+        upstream_repo = git.Repo.init(upstream_dir)
+        (Path(upstream_dir) / 'README').write_text('README')
+        upstream_repo.git.add(all=True)
+        upstream_repo.index.commit("README")
+        upstream_repo.create_head(appid)
+
+        local_dir = os.path.join(self.testdir.name, 'local_git')
+        git.Repo.clone_from(upstream_dir, local_dir)
+        os.chdir(local_dir)
+        git_repo = git.Repo.init('.')
+        # --merge-request assumes remotes called 'origin' and 'upstream'
+        git_repo.create_remote('upstream', upstream_dir).fetch()
+
+        os.mkdir('metadata')
+        git_repo.create_head(appid, f'origin/{appid}', force=True)
+        git_repo.git.checkout(appid)
+
+        # fake checkupdates-bot commit
+        Path(f'metadata/{appid}.yml').write_text('AutoName: Example\n')
+        with git_repo.config_writer() as cw:
+            cw.set_value('user', 'email', fdroidserver.checkupdates.BOT_EMAIL)
+        git_repo.git.add(all=True)
+        git_repo.index.commit("Example")
+
+        # set up starting from remote branch
+        git_repo.remotes.origin.push(appid)
+        upstream_main = fdroidserver.checkupdates.get_upstream_main_branch(git_repo)
+        git_repo.git.checkout(upstream_main.split('/')[1])
+        git_repo.delete_head(appid, force=True)
+
+        self.assertTrue(
+            fdroidserver.checkupdates.checkout_appid_branch(appid),
+            'This should have been true since there are only bot commits.',
+        )
+
+    def test_checkout_appid_branch_skip_human_edits(self):
+        appid = 'com.example'
+
+        upstream_dir = os.path.join(self.testdir.name, 'upstream_git')
+        os.mkdir(upstream_dir)
+        upstream_repo = git.Repo.init(upstream_dir)
+        (Path(upstream_dir) / 'README').write_text('README')
+        upstream_repo.git.add(all=True)
+        upstream_repo.index.commit("README")
+        upstream_repo.create_head(appid)
+
+        local_dir = os.path.join(self.testdir.name, 'local_git')
+        git.Repo.clone_from(upstream_dir, local_dir)
+        os.chdir(local_dir)
+        git_repo = git.Repo.init('.')
+        # --merge-request assumes remotes called 'origin' and 'upstream'
+        git_repo.create_remote('upstream', upstream_dir).fetch()
+
+        os.mkdir('metadata')
+        git_repo.create_head(appid, f'origin/{appid}', force=True)
+        git_repo.git.checkout(appid)
+
+        with git_repo.config_writer() as cw:
+            cw.set_value('user', 'email', fdroidserver.checkupdates.BOT_EMAIL)
+
+        # fake checkupdates-bot commit
+        Path(f'metadata/{appid}.yml').write_text('AutoName: Example\n')
+        git_repo.git.add(all=True)
+        git_repo.index.commit("Example")
+
+        # fake commit added on top by a human
+        Path(f'metadata/{appid}.yml').write_text('AutoName: Example\nName: Foo\n')
+        with git_repo.config_writer() as cw:
+            cw.set_value('user', 'email', 'human@bar.com')
+        git_repo.git.add(all=True)
+        git_repo.index.commit("Example")
+
+        # set up starting from remote branch
+        git_repo.remotes.origin.push(appid)
+        upstream_main = fdroidserver.checkupdates.get_upstream_main_branch(git_repo)
+        git_repo.git.reset(upstream_main.split('/')[1])
+
+        self.assertFalse(
+            fdroidserver.checkupdates.checkout_appid_branch(appid),
+            'This should have been false since there are human edits.',
+        )
+
+    @mock.patch('git.remote.Remote.push')
     @mock.patch('sys.exit')
     @mock.patch('fdroidserver.common.read_app_args')
     @mock.patch('fdroidserver.checkupdates.checkupdates_app')
-    def test_merge_requests_branch(self, checkupdates_app, read_app_args, sys_exit):
+    def test_merge_requests_branch(
+        self, checkupdates_app, read_app_args, sys_exit, push
+    ):
         def _sys_exit(return_code=0):
             self.assertEqual(return_code, 0)
 
@@ -499,5 +688,6 @@ class CheckupdatesTest(unittest.TestCase):
         self.assertNotIn(appid, git_repo.heads)
         with mock.patch('sys.argv', ['fdroid checkupdates', '--merge-request', appid]):
             fdroidserver.checkupdates.main()
+        push.assert_called_once()
         sys_exit.assert_called_once()
         self.assertIn(appid, git_repo.heads)