From f4d3fbc4aad79ce80d7ca94c8bdec40f3fcf5253 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 29 Apr 2024 13:35:17 +0100 Subject: [PATCH 01/17] Add script to move files to the framework Signed-off-by: David Horstmann --- tools/bin/mbedtls-move-to-framework | 212 ++++++++++++++++++++++++++++ 1 file changed, 212 insertions(+) create mode 100755 tools/bin/mbedtls-move-to-framework diff --git a/tools/bin/mbedtls-move-to-framework b/tools/bin/mbedtls-move-to-framework new file mode 100755 index 0000000..e6a9ada --- /dev/null +++ b/tools/bin/mbedtls-move-to-framework @@ -0,0 +1,212 @@ +#!/usr/bin/env python3 +"""Move files from the Mbed TLS repository to the framework repository. + +Invoke this script from a Git working directory with a branch of Mbed TLS. +""" + +# Copyright The Mbed TLS Contributors +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import argparse +import os +import pathlib +import re +import subprocess +import sys +from typing import List, Optional, Tuple, Dict + + +class RepoFileMover: + """Move files between repos while retaining history.""" + + GIT_EXE = 'git' + DEFAULT_BRANCH = 'main' + + def __init__(self, source_repo: str, dest_repo: str, + source_branch_name: str, dest_branch_name: str, + file_map: Dict[str, str]): + self._source_repo = os.path.abspath(source_repo) + self._dest_repo = os.path.abspath(dest_repo) + + self._src_branch_name = source_branch_name + self._tmp_framework_branch_name = 'tmp-branch-move-files-to-framework' + self._framework_branch_name = dest_branch_name + + self._file_map = {os.path.normpath(k): os.path.normpath(v) for k, v in file_map.items()} + + def run_git(self, git_cmd: List[str], **kwargs) -> str: + """Run a git command and return its output.""" + if 'universal_newlines' not in kwargs: + kwargs['universal_newlines'] = True + cmd = [self.GIT_EXE] + git_cmd + return subprocess.check_output(cmd, **kwargs) + + def add_file_move(self, src_path: str, dst_path: str) -> None: + """Move file at relative path src_path in the source repo to dst_path + in the destination repo""" + self._file_map[src_path] = dst_path + + def ensure_dirs_exist(self, path: str) -> None: + """Make sure all of the required directories exist for + moving a given file or directory to it.""" + + # Remove any trailing slashes + while path[-1] == os.sep: + path = path[:-1] + + # Get the directory names + dirs, file = os.path.split(path) + # Make all required directories + os.makedirs(dirs, exist_ok=True) + + def create_dest_repo_branch(self): + """Create the branch containing the moved files only""" + + # Create a new branch + self.run_git(['checkout', '-b', self._tmp_framework_branch_name]) + + # Get a list of all files in the repo + repo_files = self.run_git(['ls-files']).split() + + # Get a set of the files we are moving (taking account of directories) + files_to_retain = set() + for f in self._file_map: + if os.path.isdir(f): + dir_files = self.run_git(['ls-files', f]).split() + dir_files = [os.path.normpath(dir_file) for dir_file in dir_files] + files_to_retain.update(dir_files) + else: + files_to_retain.add(os.path.normpath(f)) + + # Delete all files except the ones we are moving + for f in repo_files: + if os.path.normpath(f) not in files_to_retain: + self.run_git(['rm', f]) + + # Rename files as requested + for f in self._file_map: + # Git won't let us move things to themselves + if self._file_map[f] != f: + self.ensure_dirs_exist(self._file_map[f]) + self.run_git(['mv', f, self._file_map[f]]) + + # Commit the result + commit_message = "Move some files to framework repository" + self.run_git(['commit', '-asm', commit_message]) + + def create_src_repo_branch(self): + """Create the branch deleting the moved files""" + + # Create a new branch + self.run_git(['checkout', '-b', self._src_branch_name]) + + # Delete the moved files + files_to_delete = self._file_map.keys() + for f in files_to_delete: + if os.path.isdir(f): + self.run_git(['rm', '-r', f]) + else: + self.run_git(['rm', f]) + + # Commit the result + commit_message = "Move some files to framework repository" + self.run_git(['commit', '-asm', commit_message]) + + def merge_files_into_framework(self): + """Add the source repo as a remote, pull in the destination branch + and merge into a new branch""" + # Change to the desination repo + os.chdir(self._dest_repo) + + # Fetch/checkout the branch that has the moved files on it + self.run_git(['fetch', self._source_repo, + self._tmp_framework_branch_name + ':' + self._tmp_framework_branch_name]) + + # Checkout the default branch + self.run_git(['checkout', self.DEFAULT_BRANCH]) + + # Create a new branch with checkout -b + self.run_git(['checkout', '-b', self._framework_branch_name]) + + # Merge in the previously-fetched branch using --allow-unrelated-histories + self.run_git(['merge', self._tmp_framework_branch_name, '--allow-unrelated-histories']) + + def move_files(self): + os.chdir(self._source_repo) + + base_commit = self.run_git(['rev-parse', 'HEAD']).strip() + + self.create_dest_repo_branch() + # Reset state of repo + self.run_git(['checkout', base_commit]) + + self.create_src_repo_branch() + # Reset state of repo + self.run_git(['checkout', base_commit]) + + self.merge_files_into_framework() + + # Delete the temporary branches in both repos + self.run_git(['branch', '-D', self._tmp_framework_branch_name]) + + os.chdir(self._source_repo) + self.run_git(['branch', '-D', self._tmp_framework_branch_name]) + + +def main() -> int: + """Command line entry point.""" + parser = argparse.ArgumentParser() + parser.add_argument('--path', action='append', nargs='?', + help='Path to move, (colon-separate for renames)' + '(e.g. "tests/scripts" or "tests/scripts:foo")') + parser.add_argument('--from', dest='from_repo', + metavar='FROM', required=True, + help='Path to the Mbed TLS repo to move files from') + parser.add_argument('--to', dest='to_repo', metavar='TO', required=True, + help='Path to the framework repo to move files to') + parser.add_argument('--src-branch', + default='move-files-from-mbedtls-to-framework', + required=False, + help='Name of the new branch create in the Mbed TLS repo') + parser.add_argument('--dst-branch', default='move-files-into-framework', + required=False, + help='Name of the new branch create in the framework repo') + args = parser.parse_args() + + file_map = {} + for p in args.path: + # If the name contains a colon it is a rename, + # otherwise just a plain path. + if ':' in p: + rename = p.split(':') + if len(rename) != 2: + print('Error: Path rename must be of the form "src:dest"', + file=sys.stderr) + return 1 + + file_map[rename[0]] = rename[1] + + else: + file_map[p] = p + + file_mover = RepoFileMover(args.from_repo, args.to_repo, + args.src_branch, args.dst_branch, + file_map) + file_mover.move_files() + + return 0 + +if __name__ == '__main__': + sys.exit(main()) From 58bfb923de7706df4dea7754c8a40f920c80e541 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 8 May 2024 14:58:39 +0100 Subject: [PATCH 02/17] Add customisable destination base branch This is useful for building file-moves on top of other not-yet-merged file moves. Signed-off-by: David Horstmann --- tools/bin/mbedtls-move-to-framework | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/tools/bin/mbedtls-move-to-framework b/tools/bin/mbedtls-move-to-framework index e6a9ada..635ffae 100755 --- a/tools/bin/mbedtls-move-to-framework +++ b/tools/bin/mbedtls-move-to-framework @@ -32,11 +32,12 @@ class RepoFileMover: """Move files between repos while retaining history.""" GIT_EXE = 'git' - DEFAULT_BRANCH = 'main' + FRAMEWORK_DEFAULT_BASE_BRANCH = 'main' def __init__(self, source_repo: str, dest_repo: str, source_branch_name: str, dest_branch_name: str, - file_map: Dict[str, str]): + file_map: Dict[str, str], + dest_base_branch_name: Optional[str] = None): self._source_repo = os.path.abspath(source_repo) self._dest_repo = os.path.abspath(dest_repo) @@ -44,6 +45,11 @@ class RepoFileMover: self._tmp_framework_branch_name = 'tmp-branch-move-files-to-framework' self._framework_branch_name = dest_branch_name + if dest_base_branch_name is None: + self._framework_base_branch_name = self.FRAMEWORK_DEFAULT_BASE_BRANCH + else: + self._framework_base_branch_name = dest_base_branch_name + self._file_map = {os.path.normpath(k): os.path.normpath(v) for k, v in file_map.items()} def run_git(self, git_cmd: List[str], **kwargs) -> str: @@ -135,7 +141,7 @@ class RepoFileMover: self._tmp_framework_branch_name + ':' + self._tmp_framework_branch_name]) # Checkout the default branch - self.run_git(['checkout', self.DEFAULT_BRANCH]) + self.run_git(['checkout', self._framework_base_branch_name]) # Create a new branch with checkout -b self.run_git(['checkout', '-b', self._framework_branch_name]) @@ -183,6 +189,8 @@ def main() -> int: parser.add_argument('--dst-branch', default='move-files-into-framework', required=False, help='Name of the new branch create in the framework repo') + parser.add_argument('--dst-base-branch', default=None, required=False, + help='Base branch in the framework repo to make changes from') args = parser.parse_args() file_map = {} @@ -203,7 +211,8 @@ def main() -> int: file_mover = RepoFileMover(args.from_repo, args.to_repo, args.src_branch, args.dst_branch, - file_map) + file_map, + args.dst_base_branch) file_mover.move_files() return 0 From 4c095e2156aea51aca987a9dfd4c1a1aa66d38da Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 8 May 2024 15:06:32 +0100 Subject: [PATCH 03/17] Fix bug in directory building When there are no dirs to make we try anyway. This is bad and causes an error. Fix this. Signed-off-by: David Horstmann --- tools/bin/mbedtls-move-to-framework | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tools/bin/mbedtls-move-to-framework b/tools/bin/mbedtls-move-to-framework index 635ffae..9abe8ef 100755 --- a/tools/bin/mbedtls-move-to-framework +++ b/tools/bin/mbedtls-move-to-framework @@ -74,8 +74,10 @@ class RepoFileMover: # Get the directory names dirs, file = os.path.split(path) - # Make all required directories - os.makedirs(dirs, exist_ok=True) + + if dirs != '': + # Make all required directories + os.makedirs(dirs, exist_ok=True) def create_dest_repo_branch(self): """Create the branch containing the moved files only""" From ddd8c7ae51a3a8d9f5d99e18c48b287052cc1e21 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 9 May 2024 14:56:13 +0100 Subject: [PATCH 04/17] Add conflict-resolution for deleted files To allow branches to be built on other branches, resolve conflicts 'manually' when the 2 branches delete each others' files. Signed-off-by: David Horstmann --- tools/bin/mbedtls-move-to-framework | 52 +++++++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 2 deletions(-) diff --git a/tools/bin/mbedtls-move-to-framework b/tools/bin/mbedtls-move-to-framework index 9abe8ef..b2ec61e 100755 --- a/tools/bin/mbedtls-move-to-framework +++ b/tools/bin/mbedtls-move-to-framework @@ -132,6 +132,45 @@ class RepoFileMover: commit_message = "Move some files to framework repository" self.run_git(['commit', '-asm', commit_message]) + def resolve_deletion_conflicts(self): + file_statuses = self.run_git(['status', '--porcelain']) + + # Get conflicting files + files_by_status = {} + for line in file_statuses.splitlines(): + # Process lines like: + # D dir1/myfile.txt + # M myfile.csv + # etc + line = line.strip().split() + + if len(line) != 2: + continue + status = line[0] + filename = line[1] + + if len(status) == 2: + # Check cases where conflicts need to be resolved + if status not in ('AD', 'DA', 'UD', 'DU'): + # Resolve conflicts only if one file has been deleted. + # We can't deal with multiple-modifications etc + return False + + if status in files_by_status: + files_by_status[status].append(filename) + else: + files_by_status[status] = [filename] + + # Restore any deleted files + if 'D' in files_by_status: + for f in files_by_status['D']: + self.run_git(['checkout', 'HEAD', '--', f]) + + # Add all files + self.run_git(['add', '.']) + + return True + def merge_files_into_framework(self): """Add the source repo as a remote, pull in the destination branch and merge into a new branch""" @@ -148,8 +187,17 @@ class RepoFileMover: # Create a new branch with checkout -b self.run_git(['checkout', '-b', self._framework_branch_name]) - # Merge in the previously-fetched branch using --allow-unrelated-histories - self.run_git(['merge', self._tmp_framework_branch_name, '--allow-unrelated-histories']) + try: + # Merge in the previously-fetched branch using --allow-unrelated-histories + self.run_git(['merge', self._tmp_framework_branch_name, '--allow-unrelated-histories']) + except subprocess.CalledProcessError as git_error: + # We have conflicts, resolve them if possible + if not self.resolve_deletion_conflicts(): + raise git_error + + # Continue the merge without opening an editor + self.run_git(['-c', 'core.editor=/bin/true', + 'merge', '--continue']) def move_files(self): os.chdir(self._source_repo) From ffee03637110a45d2517d5ac8ae1c10cc75933a2 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 7 Jun 2024 18:12:23 +0100 Subject: [PATCH 05/17] Add exceptions to file-moving Allow passing the --except flag, which stops a file from being moved (and perform the necessary path manipulation acrobatics to make that work). Created to be able to move tests/include except for tests/include/alt-dummy, like so: --path tests/include:include --except tests/include/alt-dummy Signed-off-by: David Horstmann --- tools/bin/mbedtls-move-to-framework | 48 ++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/tools/bin/mbedtls-move-to-framework b/tools/bin/mbedtls-move-to-framework index b2ec61e..d90a928 100755 --- a/tools/bin/mbedtls-move-to-framework +++ b/tools/bin/mbedtls-move-to-framework @@ -64,6 +64,44 @@ class RepoFileMover: in the destination repo""" self._file_map[src_path] = dst_path + def _path_parts(self, path: str) -> List[str]: + return path.split(os.path.sep) + + def _direct_children(self, dir_path: str) -> List[str]: + """Get the direct children of a subdirectory (at least the ones + that matter to git)""" + leaf_files = self.run_git(['ls-files', dir_path]).split() + path_len = len(self._path_parts(dir_path)) + 1 + direct_children = set([ + os.path.join( + *self._path_parts(f)[:path_len] + ) + for f in leaf_files + ]) + return list(direct_children) + + def _expand_dir_move(self, dir_path: str) -> None: + """Expand a directory move into the moves of its children""" + if dir_path in self._file_map: + # Get the direct children of this directory + expanded_paths = self._direct_children(dir_path) + # Make an equivalent mapping for each direct child + for path in expanded_paths: + self._file_map[path] = (self._file_map[dir_path] + + path[len(dir_path):]) + # Remove the mapping for the original directory, since we want + # some files to remain behind in it. + self._file_map.pop(dir_path, None) + + def add_exception(self, src_path: str) -> None: + path_components = self._path_parts(src_path) + # Expand the directories prefixing the exception, + # starting with the shortest prefix + for i in range(1, len(path_components)): + self._expand_dir_move(os.path.join(*path_components[:i])) + # Remove the exception + self._file_map.pop(src_path, None) + def ensure_dirs_exist(self, path: str) -> None: """Make sure all of the required directories exist for moving a given file or directory to it.""" @@ -226,7 +264,11 @@ def main() -> int: parser = argparse.ArgumentParser() parser.add_argument('--path', action='append', nargs='?', help='Path to move, (colon-separate for renames)' - '(e.g. "tests/scripts" or "tests/scripts:foo")') + ' (e.g. "tests/scripts" or "tests/scripts:foo")') + parser.add_argument('--except', action='append', nargs='?', + dest='file_exceptions', + help='Exceptions to --path arguments' + ' (Files or directories not to move') parser.add_argument('--from', dest='from_repo', metavar='FROM', required=True, help='Path to the Mbed TLS repo to move files from') @@ -263,6 +305,10 @@ def main() -> int: args.src_branch, args.dst_branch, file_map, args.dst_base_branch) + + for e in args.file_exceptions: + file_mover.add_exception(e) + file_mover.move_files() return 0 From 6afe50f89d42f99ccb6045f96a35e5806cb04801 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 11 Jun 2024 17:22:42 +0100 Subject: [PATCH 06/17] Add default empty list if no exceptions This was failing if no exceptions were supplied. Signed-off-by: David Horstmann --- tools/bin/mbedtls-move-to-framework | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/bin/mbedtls-move-to-framework b/tools/bin/mbedtls-move-to-framework index d90a928..32af9b3 100755 --- a/tools/bin/mbedtls-move-to-framework +++ b/tools/bin/mbedtls-move-to-framework @@ -267,6 +267,7 @@ def main() -> int: ' (e.g. "tests/scripts" or "tests/scripts:foo")') parser.add_argument('--except', action='append', nargs='?', dest='file_exceptions', + default=[], help='Exceptions to --path arguments' ' (Files or directories not to move') parser.add_argument('--from', dest='from_repo', From 02e7a14f0d78dcc6be7ad83e30308de4328ccbcd Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 17 Jul 2024 17:57:01 +0100 Subject: [PATCH 07/17] Add --print-file-map option This causes the script to not perform the operation, but simply print the self._file_map dictionary of files to rename (this is useful for debugging the file-moving logic, especially with combinations of --path and --except options). Signed-off-by: David Horstmann --- tools/bin/mbedtls-move-to-framework | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tools/bin/mbedtls-move-to-framework b/tools/bin/mbedtls-move-to-framework index 32af9b3..d483355 100755 --- a/tools/bin/mbedtls-move-to-framework +++ b/tools/bin/mbedtls-move-to-framework @@ -284,6 +284,9 @@ def main() -> int: help='Name of the new branch create in the framework repo') parser.add_argument('--dst-base-branch', default=None, required=False, help='Base branch in the framework repo to make changes from') + parser.add_argument('--print-file-map', action='store_true', + help='Don\'t perform the move, just print out' + 'the file map created (used for debugging)') args = parser.parse_args() file_map = {} @@ -310,7 +313,11 @@ def main() -> int: for e in args.file_exceptions: file_mover.add_exception(e) - file_mover.move_files() + if args.print_file_map: + for k, v in file_mover._file_map.items(): + print(f'{k}: {v}') + else: + file_mover.move_files() return 0 From b77588778c30068285ac23719d1725e5fd00c1e7 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 17 Jul 2024 17:58:35 +0100 Subject: [PATCH 08/17] Deal with no-name-change edge case When files are moved to the exact same location in the framework repo as they were in Mbed TLS, there is a problem. Since no git change has taken place on the Mbed TLS side, the most recent change to the file is its deletion when a previous set of files were moved to the framework (since all other files are deleted in this kind of move). As a result the moved files are deleted and do not appear in the final branch. Deal with this edge case explicitly by taking files which are not renamed from the file map and checking them out to the state in the temporary branch, before amending the resulting commit. Signed-off-by: David Horstmann --- tools/bin/mbedtls-move-to-framework | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tools/bin/mbedtls-move-to-framework b/tools/bin/mbedtls-move-to-framework index d483355..f53c11c 100755 --- a/tools/bin/mbedtls-move-to-framework +++ b/tools/bin/mbedtls-move-to-framework @@ -237,6 +237,16 @@ class RepoFileMover: self.run_git(['-c', 'core.editor=/bin/true', 'merge', '--continue']) + # Edge case: when we are not renaming the files, their deletion in + # previous branches moved to the framework will take precedence without + # merge conficts. Treat these files specially and restore them. + for f in self._file_map: + if self._file_map[f] == f: + self.run_git(['checkout', self._tmp_framework_branch_name, '--', f]) + self.run_git(['add', f]) + self.run_git(['-c', 'core.editor=/bin/true', + 'commit', '--amend']) + def move_files(self): os.chdir(self._source_repo) From 60d0808ffe0caf0719741aeab7f9d6168e3ffd2b Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 22 Oct 2024 18:31:36 +0100 Subject: [PATCH 09/17] Do git commits interactively This allows the user to write a message that they prefer. While we're at it, we can supply a helpful template so that it's a bit clearer what each commit message is for. Signed-off-by: David Horstmann --- tools/bin/mbedtls-move-to-framework | 38 ++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/tools/bin/mbedtls-move-to-framework b/tools/bin/mbedtls-move-to-framework index f53c11c..656db15 100755 --- a/tools/bin/mbedtls-move-to-framework +++ b/tools/bin/mbedtls-move-to-framework @@ -34,6 +34,18 @@ class RepoFileMover: GIT_EXE = 'git' FRAMEWORK_DEFAULT_BASE_BRANCH = 'main' + FRAMEWORK_SIDE_TEMPLATE = '''Move files from into the framework + +# This will be the commit message for the commit in the framework +# repository that adds the files that were moved. +''' + + MBEDTLS_SIDE_TEMPLATE = '''Move files out of Mbed TLS + +# This will be the commit message for the commit in the Mbed TLS +# repository that removes the files that were moved. +''' + def __init__(self, source_repo: str, dest_repo: str, source_branch_name: str, dest_branch_name: str, file_map: Dict[str, str], @@ -59,6 +71,14 @@ class RepoFileMover: cmd = [self.GIT_EXE] + git_cmd return subprocess.check_output(cmd, **kwargs) + def run_git_interactive(self, git_cmd: List[str], **kwargs) -> int: + """Run a git command that may interact with the user and return + its return code.""" + if 'universal_newlines' not in kwargs: + kwargs['universal_newlines'] = True + cmd = [self.GIT_EXE] + git_cmd + return subprocess.run(cmd, **kwargs).returncode + def add_file_move(self, src_path: str, dst_path: str) -> None: """Move file at relative path src_path in the source repo to dst_path in the destination repo""" @@ -117,6 +137,18 @@ class RepoFileMover: # Make all required directories os.makedirs(dirs, exist_ok=True) + def commit_interactively_with_template(self, commit_template: str, + *extra_flags) -> None: + template_name = 'commit-msg-template-{pid}'.format(pid=os.getpid()) + + with open(template_name, 'w') as template_file: + template_file.write(commit_template) + + self.run_git_interactive(['commit', '-t', template_name] + list(extra_flags)) + + os.remove(template_name) + + def create_dest_repo_branch(self): """Create the branch containing the moved files only""" @@ -149,8 +181,7 @@ class RepoFileMover: self.run_git(['mv', f, self._file_map[f]]) # Commit the result - commit_message = "Move some files to framework repository" - self.run_git(['commit', '-asm', commit_message]) + self.commit_interactively_with_template(self.FRAMEWORK_SIDE_TEMPLATE, '-as') def create_src_repo_branch(self): """Create the branch deleting the moved files""" @@ -167,8 +198,7 @@ class RepoFileMover: self.run_git(['rm', f]) # Commit the result - commit_message = "Move some files to framework repository" - self.run_git(['commit', '-asm', commit_message]) + self.commit_interactively_with_template(self.MBEDTLS_SIDE_TEMPLATE, '-as') def resolve_deletion_conflicts(self): file_statuses = self.run_git(['status', '--porcelain']) From 7847e777b564379d477d873e9a6150ea8d3b1d5d Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 22 Oct 2024 19:01:31 +0100 Subject: [PATCH 10/17] Improvements to commit message template Signed-off-by: David Horstmann --- tools/bin/mbedtls-move-to-framework | 40 ++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/tools/bin/mbedtls-move-to-framework b/tools/bin/mbedtls-move-to-framework index 656db15..6cc0d21 100755 --- a/tools/bin/mbedtls-move-to-framework +++ b/tools/bin/mbedtls-move-to-framework @@ -34,16 +34,21 @@ class RepoFileMover: GIT_EXE = 'git' FRAMEWORK_DEFAULT_BASE_BRANCH = 'main' - FRAMEWORK_SIDE_TEMPLATE = '''Move files from into the framework + # Note that commit message templates must be comments only + # as git requires that the user edit the template before committing. + FRAMEWORK_SIDE_TEMPLATE = '''# Move files into the framework + +# This will be the commit message for the commit that takes Mbed TLS +# and removes everything except the moved files. This will then be +# merged into a branch in the framework repository. -# This will be the commit message for the commit in the framework -# repository that adds the files that were moved. ''' - MBEDTLS_SIDE_TEMPLATE = '''Move files out of Mbed TLS + MBEDTLS_SIDE_TEMPLATE = '''# Move files out of Mbed TLS # This will be the commit message for the commit in the Mbed TLS # repository that removes the files that were moved. + ''' def __init__(self, source_repo: str, dest_repo: str, @@ -148,6 +153,25 @@ class RepoFileMover: os.remove(template_name) + def commit_template_file_list_src(self) -> str: + template_file_msg = ('# The following files will be deleted' + ' (moved to the framework):\n\n') + for src_repo_file in self._file_map.keys(): + template_file_msg += '# {f}\n'.format(f=src_repo_file) + + template_file_msg += '\n' + + return template_file_msg + + def commit_template_file_list_dst(self) -> str: + template_file_msg = ('# The following files will be added' + ' (moved from Mbed TLS):\n\n') + for dst_repo_file in self._file_map.values(): + template_file_msg += '# {f}\n'.format(f=dst_repo_file) + + template_file_msg += '\n' + + return template_file_msg def create_dest_repo_branch(self): """Create the branch containing the moved files only""" @@ -181,7 +205,9 @@ class RepoFileMover: self.run_git(['mv', f, self._file_map[f]]) # Commit the result - self.commit_interactively_with_template(self.FRAMEWORK_SIDE_TEMPLATE, '-as') + self.commit_interactively_with_template(self.FRAMEWORK_SIDE_TEMPLATE + + self.commit_template_file_list_dst(), + '-as') def create_src_repo_branch(self): """Create the branch deleting the moved files""" @@ -198,7 +224,9 @@ class RepoFileMover: self.run_git(['rm', f]) # Commit the result - self.commit_interactively_with_template(self.MBEDTLS_SIDE_TEMPLATE, '-as') + self.commit_interactively_with_template(self.MBEDTLS_SIDE_TEMPLATE + + self.commit_template_file_list_src(), + '-as') def resolve_deletion_conflicts(self): file_statuses = self.run_git(['status', '--porcelain']) From cff001e4995ca72a270a26e295d69017eaa8d2e7 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 29 Oct 2024 17:53:00 +0000 Subject: [PATCH 11/17] Normalize paths in the add_file_move() method Signed-off-by: David Horstmann --- tools/bin/mbedtls-move-to-framework | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/bin/mbedtls-move-to-framework b/tools/bin/mbedtls-move-to-framework index 6cc0d21..306900d 100755 --- a/tools/bin/mbedtls-move-to-framework +++ b/tools/bin/mbedtls-move-to-framework @@ -87,7 +87,7 @@ class RepoFileMover: def add_file_move(self, src_path: str, dst_path: str) -> None: """Move file at relative path src_path in the source repo to dst_path in the destination repo""" - self._file_map[src_path] = dst_path + self._file_map[os.path.normpath(src_path)] = os.path.normpath(dst_path) def _path_parts(self, path: str) -> List[str]: return path.split(os.path.sep) From 16a1e492a05d0c16bf427402316749b15a2abc2e Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Tue, 29 Oct 2024 17:57:01 +0000 Subject: [PATCH 12/17] Remove file_map parameter from constructor Instead of passing a dictionary file map when creating a RepoFileMover object, remove this from the constructor. The correct way to add a file move is via the add_file_move() method (as this properly encapsulates the file map). Signed-off-by: David Horstmann --- tools/bin/mbedtls-move-to-framework | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/tools/bin/mbedtls-move-to-framework b/tools/bin/mbedtls-move-to-framework index 306900d..84e605c 100755 --- a/tools/bin/mbedtls-move-to-framework +++ b/tools/bin/mbedtls-move-to-framework @@ -53,7 +53,6 @@ class RepoFileMover: def __init__(self, source_repo: str, dest_repo: str, source_branch_name: str, dest_branch_name: str, - file_map: Dict[str, str], dest_base_branch_name: Optional[str] = None): self._source_repo = os.path.abspath(source_repo) self._dest_repo = os.path.abspath(dest_repo) @@ -67,7 +66,7 @@ class RepoFileMover: else: self._framework_base_branch_name = dest_base_branch_name - self._file_map = {os.path.normpath(k): os.path.normpath(v) for k, v in file_map.items()} + self._file_map = {} def run_git(self, git_cmd: List[str], **kwargs) -> str: """Run a git command and return its output.""" @@ -357,7 +356,9 @@ def main() -> int: 'the file map created (used for debugging)') args = parser.parse_args() - file_map = {} + file_mover = RepoFileMover(args.from_repo, args.to_repo, + args.src_branch, args.dst_branch, + args.dst_base_branch) for p in args.path: # If the name contains a colon it is a rename, # otherwise just a plain path. @@ -368,15 +369,11 @@ def main() -> int: file=sys.stderr) return 1 - file_map[rename[0]] = rename[1] + file_mover.add_file_move(rename[0], rename[1]) else: - file_map[p] = p + file_mover.add_file_move(p, p) - file_mover = RepoFileMover(args.from_repo, args.to_repo, - args.src_branch, args.dst_branch, - file_map, - args.dst_base_branch) for e in args.file_exceptions: file_mover.add_exception(e) From 7e1748b3e74c4808116ddc679e7b7e43468070e5 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 28 Nov 2024 16:20:15 +0000 Subject: [PATCH 13/17] Simplify _direct_children and _expand_dir_move Change _direct_children() to return only the child directory names (sans prefix), which can be done easily with: git -C dir ls-files Also reconstruct paths more explicitly in _expand_dir_move(). Signed-off-by: David Horstmann --- tools/bin/mbedtls-move-to-framework | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tools/bin/mbedtls-move-to-framework b/tools/bin/mbedtls-move-to-framework index 84e605c..3bc577b 100755 --- a/tools/bin/mbedtls-move-to-framework +++ b/tools/bin/mbedtls-move-to-framework @@ -94,12 +94,9 @@ class RepoFileMover: def _direct_children(self, dir_path: str) -> List[str]: """Get the direct children of a subdirectory (at least the ones that matter to git)""" - leaf_files = self.run_git(['ls-files', dir_path]).split() - path_len = len(self._path_parts(dir_path)) + 1 + leaf_files = self.run_git(['-C', dir_path, 'ls-files']).split() direct_children = set([ - os.path.join( - *self._path_parts(f)[:path_len] - ) + self._path_parts(f)[0] for f in leaf_files ]) return list(direct_children) @@ -108,11 +105,12 @@ class RepoFileMover: """Expand a directory move into the moves of its children""" if dir_path in self._file_map: # Get the direct children of this directory - expanded_paths = self._direct_children(dir_path) + direct_children = self._direct_children(dir_path) # Make an equivalent mapping for each direct child - for path in expanded_paths: - self._file_map[path] = (self._file_map[dir_path] - + path[len(dir_path):]) + for child in direct_children: + src_path = os.path.join(dir_path, child) + dst_path = os.path.join(self._file_map[dir_path], child) + self._file_map[src_path] = dst_path # Remove the mapping for the original directory, since we want # some files to remain behind in it. self._file_map.pop(dir_path, None) From 5a7909494f720c920230cbdc6e05cfa556043624 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 28 Nov 2024 16:57:53 +0000 Subject: [PATCH 14/17] Python-related simplifcations Replace some statements with equivalent, simpler alternatives. Signed-off-by: David Horstmann --- tools/bin/mbedtls-move-to-framework | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/tools/bin/mbedtls-move-to-framework b/tools/bin/mbedtls-move-to-framework index 3bc577b..7d59ea8 100755 --- a/tools/bin/mbedtls-move-to-framework +++ b/tools/bin/mbedtls-move-to-framework @@ -113,7 +113,7 @@ class RepoFileMover: self._file_map[src_path] = dst_path # Remove the mapping for the original directory, since we want # some files to remain behind in it. - self._file_map.pop(dir_path, None) + del self._file_map.pop[dir_path] def add_exception(self, src_path: str) -> None: path_components = self._path_parts(src_path) @@ -129,11 +129,10 @@ class RepoFileMover: moving a given file or directory to it.""" # Remove any trailing slashes - while path[-1] == os.sep: - path = path[:-1] + path = path.rstrip(os.sep) # Get the directory names - dirs, file = os.path.split(path) + dirs = os.path.dirname(path) if dirs != '': # Make all required directories @@ -249,10 +248,8 @@ class RepoFileMover: # We can't deal with multiple-modifications etc return False - if status in files_by_status: - files_by_status[status].append(filename) - else: - files_by_status[status] = [filename] + files_by_status.setdefault(status, []) + files_by_status[status].append(filename) # Restore any deleted files if 'D' in files_by_status: From a17eb640cd77b128d4a33cc8ebf93299819b3441 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 4 Dec 2024 12:27:17 +0000 Subject: [PATCH 15/17] Document limitation of separate checkout Update the description to be more accurate. In particular, document the limitation that a separate checkout is required. Signed-off-by: David Horstmann --- tools/bin/mbedtls-move-to-framework | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tools/bin/mbedtls-move-to-framework b/tools/bin/mbedtls-move-to-framework index 7d59ea8..a43c6c0 100755 --- a/tools/bin/mbedtls-move-to-framework +++ b/tools/bin/mbedtls-move-to-framework @@ -1,7 +1,10 @@ #!/usr/bin/env python3 -"""Move files from the Mbed TLS repository to the framework repository. +"""Move files from the Mbed TLS repository to the framework repository while +perserving their git history. -Invoke this script from a Git working directory with a branch of Mbed TLS. +Note: This script requires a separate checkout of the framework repository, +it cannot be used to move files from an Mbed TLS checkout to its internal +framework. """ # Copyright The Mbed TLS Contributors From a801f3172c2a4c5f2da4009df947cdcc2593adde Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 4 Dec 2024 14:39:32 +0000 Subject: [PATCH 16/17] Add check for an internal framework being supplied If the user tries to move files to the internal framework checkout, print an error and exit. Signed-off-by: David Horstmann --- tools/bin/mbedtls-move-to-framework | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tools/bin/mbedtls-move-to-framework b/tools/bin/mbedtls-move-to-framework index a43c6c0..9f8738c 100755 --- a/tools/bin/mbedtls-move-to-framework +++ b/tools/bin/mbedtls-move-to-framework @@ -354,6 +354,13 @@ def main() -> int: 'the file map created (used for debugging)') args = parser.parse_args() + # Check that the destination is not internal to the source + if os.path.samefile(os.path.join(args.from_repo, 'framework'), + os.path.normpath(args.to_repo)): + print("Error: Can't move files to an internal framework repository." + " A separate checkout of the framework is required.") + return 1 + file_mover = RepoFileMover(args.from_repo, args.to_repo, args.src_branch, args.dst_branch, args.dst_base_branch) From f824c884b16dfd09770bf6c88eb1aee30aba8f7b Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 4 Dec 2024 15:38:05 +0000 Subject: [PATCH 17/17] Force exactly one argument for --path and --except Set nargs=1 for each of these arguments. Since this will return a single-element list containing the argument (not the argument itself), change the action to 'extend', which will concatenate lists rather than appending individual values to a list. Signed-off-by: David Horstmann --- tools/bin/mbedtls-move-to-framework | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/bin/mbedtls-move-to-framework b/tools/bin/mbedtls-move-to-framework index 9f8738c..fb7cafc 100755 --- a/tools/bin/mbedtls-move-to-framework +++ b/tools/bin/mbedtls-move-to-framework @@ -327,10 +327,10 @@ class RepoFileMover: def main() -> int: """Command line entry point.""" parser = argparse.ArgumentParser() - parser.add_argument('--path', action='append', nargs='?', + parser.add_argument('--path', action='extend', nargs=1, help='Path to move, (colon-separate for renames)' ' (e.g. "tests/scripts" or "tests/scripts:foo")') - parser.add_argument('--except', action='append', nargs='?', + parser.add_argument('--except', action='extend', nargs=1, dest='file_exceptions', default=[], help='Exceptions to --path arguments'