kaxil commented on code in PR #62905:
URL: https://github.com/apache/airflow/pull/62905#discussion_r2887260026
##########
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}
Review Comment:
**[MEDIUM] YAML values not quoted — paths with spaces would break**
The `source` and `target` values are unquoted. A path with spaces (e.g.,
`/home/user/my projects/.git`) would produce invalid YAML.
Suggestion: quote the values:
```yaml
source: "{main_git_directory}"
target: "{main_git_directory}"
```
##########
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:
**[HIGH] Relative gitdir paths not handled**
The `.git` file can contain a relative path (e.g., `gitdir:
../../main-repo/.git/worktrees/name`). This returns `Path(...)` without
resolving it to an absolute path. Docker bind mounts require absolute paths, so
a relative gitdir would produce an invalid mount.
Suggestion:
```python
gitdir = Path(git_content.removeprefix("gitdir: "))
if not gitdir.is_absolute():
gitdir = (AIRFLOW_ROOT_PATH / gitdir).resolve()
return gitdir
```
##########
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)
Review Comment:
**[MEDIUM] No tests for new functionality**
There are no tests for `get_git_worktree_root()` or the mount logic — not
for the worktree case, not for the non-worktree case, not for relative paths.
The `.parent.parent` derivation and path parsing are easy to break silently.
Would be good to add tests that mock `AIRFLOW_ROOT_PATH / ".git"` as:
- A file with an absolute `gitdir:` path (worktree)
- A file with a relative `gitdir:` path
- A directory (non-worktree, should return None)
##########
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")
Review Comment:
**[MEDIUM] Function name and return value are misleading**
This is named `get_git_worktree_root` and the docstring says "returns the
root of the main git repository", but it actually returns the `gitdir` path
(e.g., `/home/user/airflow/.git/worktrees/my-worktree`). Callers then do
`.parent.parent` to derive the `.git` directory.
Consider either:
- Renaming to `get_git_worktree_gitdir()` with an accurate docstring, or
- Having the function return the main `.git` directory directly (what
callers actually want) and renaming to `get_main_git_dir_for_worktree()`
--
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]
