Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
35 changes: 31 additions & 4 deletions apps/predbat/download.py
Original file line number Diff line number Diff line change
Expand Up @@ -212,15 +212,42 @@ def predbat_update_download(version):
print("Error: Failed to get file list from GitHub")
return None

# Download all files
# Download all files (skip if local file has matching SHA)
downloaded_files = []
Copy link

Copilot AI Jan 4, 2026

Choose a reason for hiding this comment

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

The variable name downloaded_files is misleading because it includes both downloaded files and skipped files. Files that are skipped (due to matching SHA) are added to this list at line 246, even though they weren't actually downloaded from GitHub. Consider renaming this to processed_files or staged_files to better reflect that it includes all files prepared for installation, regardless of whether they were downloaded or copied from local.

Copilot uses AI. Check for mistakes.
skipped_files = []
for file_info in file_list:
filename = file_info["name"]
if not download_predbat_file_from_github(tag, filename, os.path.join(this_path, filename + "." + tag)):
print("Error: Failed to download {}".format(filename))
return None
expected_sha = file_info.get("sha")
local_filepath = os.path.join(this_path, filename)
download_filepath = os.path.join(this_path, filename + "." + tag)

# Check if local file exists and has matching SHA
skip_download = False
if expected_sha and os.path.exists(local_filepath):
local_sha = compute_file_sha1(local_filepath)
if local_sha == expected_sha:
print("Skipping {}: local file matches remote SHA ({})".format(filename, expected_sha[:8]))
# Copy local file to download location so move operation works
try:
with open(local_filepath, "rb") as src:
with open(download_filepath, "wb") as dst:
Comment on lines +232 to +233
Copy link

Copilot AI Jan 4, 2026

Choose a reason for hiding this comment

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

File mode inconsistency detected. The copy operation uses binary mode ("rb"/"wb") while download_predbat_file_from_github uses text mode ("w"). This inconsistency could cause different line endings on Windows (CRLF vs LF) between downloaded files and copied files, potentially leading to SHA mismatches on subsequent checks. For consistency, either both should use binary mode, or both should use text mode. Recommend using binary mode for both since the SHA computation in compute_file_sha1 operates on binary data.

Suggested change
with open(local_filepath, "rb") as src:
with open(download_filepath, "wb") as dst:
with open(local_filepath, "r") as src:
with open(download_filepath, "w") as dst:

Copilot uses AI. Check for mistakes.
dst.write(src.read())
skip_download = True
skipped_files.append(filename)
except Exception as e:
print("Warn: Failed to copy local file {}: {}".format(filename, e))
skip_download = False

if not skip_download:
if not download_predbat_file_from_github(tag, filename, download_filepath):
print("Error: Failed to download {}".format(filename))
return None

downloaded_files.append(filename)

if skipped_files:
print("\nSkipped downloading {} file(s) (already up to date): {}".format(len(skipped_files), ", ".join(skipped_files)))

# Sort files alphabetically
file_list_sorted = sorted(file_list, key=lambda x: x["name"])

Expand Down
120 changes: 120 additions & 0 deletions apps/predbat/tests/test_download.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ def test_download(my_predbat):
("update_move_empty", _test_predbat_update_move_empty_files, "Move files empty list"),
("update_move_none", _test_predbat_update_move_none_files, "Move files none list"),
("update_move_invalid_version", _test_predbat_update_move_invalid_version, "Move files invalid version"),
("update_download_skip_matching", _test_predbat_update_download_skip_matching_sha, "Update download skips files with matching SHA"),
("update_download_skip_mixed", _test_predbat_update_download_skip_mixed, "Update download skips some files, downloads others"),
]

print("\n" + "=" * 70)
Expand Down Expand Up @@ -543,3 +545,121 @@ def _test_predbat_update_move_invalid_version(my_predbat):
finally:
shutil.rmtree(temp_dir)
return 0


def _test_predbat_update_download_skip_matching_sha(my_predbat):
"""
Test download skips files when local SHA matches remote SHA
"""
temp_dir = tempfile.mkdtemp()

try:
# Create local file with known content
local_file = os.path.join(temp_dir, "predbat.py")
test_content = "print('existing file')\n"
with open(local_file, "w") as f:
f.write(test_content)

# Compute its SHA
local_sha = compute_file_sha1(local_file)

# Mock GitHub API with file that has matching SHA
mock_files = [{"name": "predbat.py", "size": len(test_content), "sha": local_sha, "type": "file"}]

download_called = False

def mock_download(tag, filename, output_path):
nonlocal download_called
download_called = True
return "downloaded content"

with patch("download.os.path.dirname", return_value=temp_dir):
with patch("download.get_github_directory_listing", return_value=mock_files):
with patch("download.download_predbat_file_from_github", side_effect=mock_download):
result = predbat_update_download("v8.30.8")

# Verify download was skipped (not called)
assert download_called is False, "download_predbat_file_from_github should not be called when SHA matches"

# Verify file list still includes the file
assert result is not None
assert "predbat.py" in result
assert "manifest.yaml" in result

# Verify staged file exists (copied from local)
staged_file = os.path.join(temp_dir, "predbat.py.v8.30.8")
assert os.path.exists(staged_file), "Staged file should exist after copy"
with open(staged_file, "r") as f:
staged_content = f.read()
assert staged_content == test_content, "Staged file should match local file"

finally:
shutil.rmtree(temp_dir)
return 0


def _test_predbat_update_download_skip_mixed(my_predbat):
"""
Test download skips files with matching SHA but downloads files with mismatched SHA
"""
temp_dir = tempfile.mkdtemp()

try:
# Create two local files
file1 = os.path.join(temp_dir, "predbat.py")
file1_content = "print('file1')\n"
with open(file1, "w") as f:
f.write(file1_content)

file2 = os.path.join(temp_dir, "config.py")
file2_content = "print('file2 old')\n"
with open(file2, "w") as f:
f.write(file2_content)

# Compute SHAs
file1_sha = compute_file_sha1(file1)
file2_sha_wrong = "abc123different" # Different SHA to force download

# Mock GitHub API with two files - one matching, one different
mock_files = [{"name": "predbat.py", "size": len(file1_content), "sha": file1_sha, "type": "file"}, {"name": "config.py", "size": 100, "sha": file2_sha_wrong, "type": "file"}]

download_calls = []

def mock_download(tag, filename, output_path):
download_calls.append(filename)
with open(output_path, "w") as f:
f.write("downloaded content for {}\n".format(filename))
return "downloaded content"

with patch("download.os.path.dirname", return_value=temp_dir):
with patch("download.get_github_directory_listing", return_value=mock_files):
with patch("download.download_predbat_file_from_github", side_effect=mock_download):
result = predbat_update_download("v8.30.8")

# Verify only config.py was downloaded (not predbat.py)
assert len(download_calls) == 1, "Should download only 1 file"
assert "config.py" in download_calls, "Should download config.py"
assert "predbat.py" not in download_calls, "Should NOT download predbat.py (SHA matches)"

# Verify both files in result
assert result is not None
assert "predbat.py" in result
assert "config.py" in result

# Verify predbat.py was copied (not downloaded)
staged_file1 = os.path.join(temp_dir, "predbat.py.v8.30.8")
assert os.path.exists(staged_file1)
with open(staged_file1, "r") as f:
content = f.read()
assert content == file1_content, "Staged predbat.py should be copied from local"

# Verify config.py was downloaded (new content)
staged_file2 = os.path.join(temp_dir, "config.py.v8.30.8")
assert os.path.exists(staged_file2)
with open(staged_file2, "r") as f:
content = f.read()
assert content == "downloaded content for config.py\n", "Staged config.py should be downloaded"

finally:
shutil.rmtree(temp_dir)
return 0