Skip to content

fix: Add functions to clean Git lock files locally and remotely #152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tomchon wants to merge 4 commits into from
Closed

fix: Add functions to clean Git lock files locally and remotely #152

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d8be706
fix: Add functions to clean Git lock files locally and remotely
tomchon Feb 4, 2026
42f859c
fix: Update checkout action reference to use fix/clean-git-locks bran…
tomchon Feb 4, 2026
5e5d711
fix: Refactor clean_git_locks methods to improve logging and code str…
tomchon Feb 4, 2026
0193fb1
fix: Enhance error handling in clean_git_locks_remote method
tomchon Feb 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 53 additions & 2 deletions .github/scripts/prepare_test_env.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import concurrent.futures
import glob
import json
import logging
import os
Expand Down Expand Up @@ -127,7 +128,53 @@ def _set_branch_variables(self):
self.utils.set_env_var(
"GITHUB_RUN_ATTEMPT", self.run_attempt, os.getenv("GITHUB_ENV", "")
)


def clean_git_locks(repo_path):
"""清理 Git 锁文件"""
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The docstring is in Chinese. For consistency with the rest of the codebase and the pull request's title/description, please translate it to English.

Suggested change
"""清理 Git 锁文件"""
"""Clean Git lock files locally."""

Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring is written in Chinese ("清理 Git 锁文件"), which is inconsistent with the English docstrings used throughout the rest of the codebase. All other docstrings in this file are in English. For consistency and maintainability, this should be changed to English (e.g., "Clean Git lock files").

Copilot uses AI. Check for mistakes.
git_dir = os.path.join(repo_path, '.git')
if not os.path.exists(git_dir):
return

lock_files = glob.glob(os.path.join(git_dir, '*.lock'))
for lock_file in lock_files:
try:
os.remove(lock_file)
Comment on lines +136 to +143
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function accepts repo_path but doesn't validate its type or convert it to a string. Looking at line 232 where this function is called, repo_path is a Path object (as seen from self.wk and self.wkc being Path objects at lines 45-46). However, os.path.join() and os.path.exists() work with both Path objects and strings in Python 3, but glob.glob() expects a string pattern. While this may work due to implicit string conversion, it would be more explicit and safer to convert the Path to a string or use Path methods consistently (e.g., Path(repo_path) / '.git').

Suggested change
git_dir = os.path.join(repo_path, '.git')
if not os.path.exists(git_dir):
return
lock_files = glob.glob(os.path.join(git_dir, '*.lock'))
for lock_file in lock_files:
try:
os.remove(lock_file)
git_dir = Path(repo_path) / ".git"
if not git_dir.exists():
return
lock_files = git_dir.glob("*.lock")
for lock_file in lock_files:
try:
lock_file.unlink()

Copilot uses AI. Check for mistakes.
print(f"Removed lock file: {lock_file}")
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using print statements is inconsistent with the logging approach (logger.info, logger.error) used elsewhere in this file. Please use logger.info for informational messages.

Suggested change
print(f"Removed lock file: {lock_file}")
logger.info(f"Removed lock file: {lock_file}")

except Exception as e:
print(f"Failed to remove {lock_file}: {e}")
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using print statements is inconsistent with the logging approach (logger.info, logger.error) used elsewhere in this file. Please use logger.error for error messages.

Suggested change
print(f"Failed to remove {lock_file}: {e}")
logger.error(f"Failed to remove {lock_file}: {e}")

Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code uses print() for logging messages, which is inconsistent with the logging convention used throughout the rest of the codebase. Other methods in this class use the logger object (e.g., logger.info(), logger.error()) for logging. This should be updated to use logger.info() for informational messages and logger.error() or logger.warning() for error messages to maintain consistency.

Copilot uses AI. Check for mistakes.

def clean_git_locks_remote(host: str, username: str, repo_path: str, password: str = None, key_filename: str = None):
"""
Clean .git/*.lock files in a remote repository via SSH.
:param host: Remote host IP or domain
:param username: SSH username
:param repo_path: Path to the repository on the remote host
:param password: SSH password (optional, for password login)
:param key_filename: Path to private key file (optional, for key-based login)
"""
import paramiko
ssh = paramiko.SSHClient()
ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AutoAddPolicy allows any SSH host key without verification, which is a security risk. This could make the connection vulnerable to man-in-the-middle attacks. However, I note that the existing _execute_remote_command method (line 346) also uses AutoAddPolicy, so this is consistent with the current codebase pattern. If this pattern is acceptable for your infrastructure (e.g., trusted internal network), this is fine. Otherwise, consider using a more secure approach like RejectPolicy with properly configured known_hosts.

Suggested change
ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())
# Load known host keys and reject unknown hosts to avoid MITM vulnerabilities
ssh.load_system_host_keys()
user_known_hosts = os.path.expanduser("~/.ssh/known_hosts")
if os.path.exists(user_known_hosts):
ssh.load_host_keys(user_known_hosts)
ssh.set_missing_host_key_policy(paramiko.RejectPolicy())

Copilot uses AI. Check for mistakes.
try:
ssh.connect(
hostname=host,
username=username,
password=password,
key_filename=key_filename,
)
git_dir = f"{repo_path}/.git"
cmd = f"find {git_dir} -name '*.lock' -type f -exec rm -f {{}} \\;"
stdin, stdout, stderr = ssh.exec_command(cmd)
out = stdout.read().decode()
err = stderr.read().decode()
print(f"[{host}] Cleaned locks in {git_dir}")
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using print statements is inconsistent with the logging approach. Please use logger.info for informational messages.

Suggested change
print(f"[{host}] Cleaned locks in {git_dir}")
logger.info(f"[{host}] Cleaned locks in {git_dir}")

if out:
print(out)
if err:
print(err)
finally:
ssh.close()

def run_git_ref_lock_cleaner(self, repo_path):
logger.info(f"Running cleaner script: {script_path}")
if not repo_path.exists():
Expand Down Expand Up @@ -165,6 +212,7 @@ def prepare_repositories(self):
logger.info(f"Preparing TDinternal in {self.wkdir}...")
self.run_git_ref_lock_cleaner(self.wk)
self.run_git_ref_lock_cleaner(self.wkc)

if (
self.inputs.get("specified_source_branch") == "unavailable"
and self.inputs.get("specified_target_branch") == "unavailable"
Expand All @@ -181,7 +229,7 @@ def _prepare_repo(self, repo_path, branch):
"""Prepare a single repository"""
if not repo_path.exists():
raise FileNotFoundError(f"Repository path not found: {repo_path}")

self.clean_git_locks(repo_path)
cmds = [
f"cd {repo_path} && git reset --hard",
f"cd {repo_path} && git clean -f",
Expand Down Expand Up @@ -339,7 +387,9 @@ def _prepare_repositories_remote(self, host_config):
logger.error(f"[{host}] Failed to send cleaner script: {e}")
return False


# 2. Prepare TDinternal
self.clean_git_locks_remote(host, username, f"{workdir}/TDinternal")
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function calls self.clean_git_locks_remote(host, username, ...) but doesn't provide the required password or key_filename parameters that are needed for SSH authentication. Looking at the host_config structure used elsewhere in the code (e.g., line 349-352), these parameters should be extracted from host_config if available, or the function should handle authentication properly. Without proper authentication parameters, the SSH connection will fail.

Copilot uses AI. Check for mistakes.
branch = self.target_branch if all(
self.inputs.get(k) == "unavailable"
for k in ["specified_source_branch", "specified_target_branch", "specified_pr_number"]
Expand All @@ -355,6 +405,7 @@ def _prepare_repositories_remote(self, host_config):
return False

# 3. Prepare community
self.clean_git_locks_remote(host, username, f"{workdir}/TDinternal/community")
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function calls self.clean_git_locks_remote(host, username, ...) but doesn't provide the required password or key_filename parameters that are needed for SSH authentication. Looking at the host_config structure used elsewhere in the code (e.g., line 349-352), these parameters should be extracted from host_config if available, or the function should handle authentication properly. Without proper authentication parameters, the SSH connection will fail.

Copilot uses AI. Check for mistakes.
comm_cmd = (
f"cd {workdir}/TDinternal/community && "
f"python3 {remote_script} && "
Expand Down
Loading