Copilot commented on code in PR #62905:
URL: https://github.com/apache/airflow/pull/62905#discussion_r2887279222
##########
dev/breeze/src/airflow_breeze/utils/path_utils.py:
##########
@@ -435,3 +435,16 @@ def cleanup_python_generated_files():
get_console().print("You can also remove those files manually
using sudo.")
if get_verbose():
get_console().print("[info]Cleaned")
+
+
+def get_git_worktree_root() -> Path | None:
+ """
+ Detects if we are in a git worktree and returns the root of the main git
repository.
+ :return: Path to the main git repository .git directory or None if not in
a worktree.
+ """
+ git_path = AIRFLOW_ROOT_PATH / ".git"
+ if git_path.is_file():
+ git_content = git_path.read_text().strip()
+ if git_content.startswith("gitdir: "):
+ return Path(git_content.removeprefix("gitdir: "))
Review Comment:
The `gitdir:` value parsed from the `.git` file could theoretically be a
relative path. If `Path(git_content.removeprefix("gitdir: "))` is a relative
path and later used as a Docker bind-mount source, it would be interpreted
relative to the current working directory rather than relative to
`AIRFLOW_ROOT_PATH`. This could result in either a silently incorrect mount
(wrong directory mounted) or a Docker error. The path should be resolved to an
absolute path, for example: `(AIRFLOW_ROOT_PATH /
Path(git_content.removeprefix("gitdir: "))).resolve()` to handle both absolute
and relative gitdir references safely.
```suggestion
gitdir_value = git_content.removeprefix("gitdir: ").strip()
return (AIRFLOW_ROOT_PATH / Path(gitdir_value)).resolve()
```
##########
dev/breeze/src/airflow_breeze/utils/path_utils.py:
##########
@@ -435,3 +435,16 @@ def cleanup_python_generated_files():
get_console().print("You can also remove those files manually
using sudo.")
if get_verbose():
get_console().print("[info]Cleaned")
+
+
+def get_git_worktree_root() -> Path | None:
+ """
+ Detects if we are in a git worktree and returns the root of the main git
repository.
+ :return: Path to the main git repository .git directory or None if not in
a worktree.
+ """
+ git_path = AIRFLOW_ROOT_PATH / ".git"
+ if git_path.is_file():
+ git_content = git_path.read_text().strip()
+ if git_content.startswith("gitdir: "):
+ return Path(git_content.removeprefix("gitdir: "))
Review Comment:
The function name `get_git_worktree_root` and its docstring are misleading.
The function actually returns the `gitdir:` path from the `.git` worktree file,
which is the worktree-specific directory inside `.git/worktrees/<name>` (e.g.,
`/home/user/airflow/.git/worktrees/my-worktree`). This is neither the "root of
the main git repository" (which would be `/home/user/airflow`) nor the "main
git repository .git directory" (which would be `/home/user/airflow/.git`), as
the docstring claims. The function should be renamed to something like
`get_git_worktree_gitdir` (and its return description updated accordingly), or
it should instead return the actual main `.git` directory by computing
`parent.parent` internally and renaming to `get_main_git_dir_for_worktree`.
##########
dev/breeze/src/airflow_breeze/utils/path_utils.py:
##########
@@ -435,3 +435,16 @@ def cleanup_python_generated_files():
get_console().print("You can also remove those files manually
using sudo.")
if get_verbose():
get_console().print("[info]Cleaned")
+
+
+def get_git_worktree_root() -> Path | None:
+ """
+ Detects if we are in a git worktree and returns the root of the main git
repository.
+ :return: Path to the main git repository .git directory or None if not in
a worktree.
+ """
+ git_path = AIRFLOW_ROOT_PATH / ".git"
+ if git_path.is_file():
+ git_content = git_path.read_text().strip()
+ if git_content.startswith("gitdir: "):
+ return Path(git_content.removeprefix("gitdir: "))
+ return None
Review Comment:
There are no tests covering the new `get_git_worktree_root()` function.
Given that the codebase has dedicated test files for path utilities (e.g.,
`dev/breeze/tests/test_find_airflow_directory.py` tests
`find_airflow_root_path_to_operate_on()` from the same module), this new
function should also have tests. At minimum, there should be a test covering:
(1) a normal `.git` directory (returns `None`), (2) a `.git` file with a valid
`gitdir:` prefix (returns the expected `Path`), and (3) a `.git` file without a
`gitdir:` prefix (returns `None`).
##########
dev/breeze/src/airflow_breeze/utils/docker_command_utils.py:
##########
@@ -671,21 +672,29 @@ def fix_ownership_using_docker(quiet: bool = True):
"run",
"-v",
f"{AIRFLOW_ROOT_PATH}:/opt/airflow/",
- "-e",
- f"HOST_OS={get_host_os()}",
- "-e",
- f"HOST_USER_ID={get_host_user_id()}",
- "-e",
- f"HOST_GROUP_ID={get_host_group_id()}",
- "-e",
- f"VERBOSE={str(get_verbose()).lower()}",
- "-e",
- f"DOCKER_IS_ROOTLESS={is_docker_rootless()}",
- "--rm",
- "-t",
- OWNERSHIP_CLEANUP_DOCKER_TAG,
- "/opt/airflow/scripts/in_container/run_fix_ownership.py",
]
+ git_worktree_root = get_git_worktree_root()
+ if git_worktree_root:
+ main_git_directory = git_worktree_root.parent.parent
+ cmd.extend(["-v", f"{main_git_directory}:{main_git_directory}:ro"])
Review Comment:
The `parent.parent` path derivation to find the main `.git` directory is
duplicated in two places: `docker_command_utils.py` (line 678) and
`shell_params.py` (line 542). This fragile logic assumes the git worktree path
is always exactly two levels deep from the main `.git` directory (i.e., always
`.git/worktrees/<name>`). If this assumption needs to change, it must be
updated in two places. This logic would be better encapsulated inside
`get_git_worktree_root()` itself (which could instead return the main `.git`
directory directly), or extracted into a helper function. Additionally, there
is no validation that `main_git_directory` actually exists on disk before it is
passed to Docker as a bind mount, which would cause a confusing Docker error if
the derived path is wrong.
##########
dev/breeze/src/airflow_breeze/params/shell_params.py:
##########
@@ -534,6 +536,28 @@ def add_docker_in_docker(self, compose_file_list:
list[Path]):
# the /var/run/docker.sock is available. See
https://github.com/docker/for-mac/issues/6545
compose_file_list.append(SCRIPTS_CI_DOCKER_COMPOSE_DOCKER_SOCKET_PATH)
+ def add_git_worktree_mount(self, compose_file_list: list[Path]):
+ git_worktree_root = get_git_worktree_root()
+ if git_worktree_root:
+ main_git_directory = git_worktree_root.parent.parent
+ get_console().print(
+ f"[info]Detected git worktree. Mounting main git directory:
{main_git_directory}[/]"
+ )
+ generated_compose_file = SCRIPTS_CI_DOCKER_COMPOSE_PATH /
"_generated_git_worktree_mount.yml"
+ generated_compose_file.write_text(
+ f"""
+---
+services:
+ airflow:
+ volumes:
+ - type: bind
+ source: {main_git_directory}
+ target: {main_git_directory}
+ read_only: true
+"""
+ )
Review Comment:
The generated compose file content starts with a leading newline before
`---`, because the triple-quoted f-string begins immediately after the opening
`"""` with a newline. This produces a file that starts with a blank line before
the YAML document separator. While Docker Compose may tolerate this, it
deviates from the YAML convention and is inconsistent with other compose files
in the repository (e.g., `scripts/ci/docker-compose/base.yml` starts with the
license header or `---` directly). The `"""` should be followed immediately by
`---` (with the leading `\n` removed), or the content should be dedented.
##########
dev/breeze/src/airflow_breeze/params/shell_params.py:
##########
@@ -534,6 +536,28 @@ def add_docker_in_docker(self, compose_file_list:
list[Path]):
# the /var/run/docker.sock is available. See
https://github.com/docker/for-mac/issues/6545
compose_file_list.append(SCRIPTS_CI_DOCKER_COMPOSE_DOCKER_SOCKET_PATH)
+ def add_git_worktree_mount(self, compose_file_list: list[Path]):
+ git_worktree_root = get_git_worktree_root()
+ if git_worktree_root:
+ main_git_directory = git_worktree_root.parent.parent
Review Comment:
Same duplicated `parent.parent` logic as in `docker_command_utils.py` line
678. See that comment for the full analysis. This logic should be moved inside
`get_git_worktree_root()` or a new helper function so it isn't repeated in both
callers.
```suggestion
def _get_main_git_directory() -> Path | None:
git_worktree_root = get_git_worktree_root()
if not git_worktree_root:
return None
return git_worktree_root.parent.parent
def add_git_worktree_mount(self, compose_file_list: list[Path]):
main_git_directory = self._get_main_git_directory()
if main_git_directory:
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
