Skip to content

Commit 2b5016f

Browse files
arnavk23CopilotIanButterworth
authored
Fix : Refactored Git command error handling (#471)
Co-authored-by: Copilot <[email protected]> Co-authored-by: Ian Butterworth <[email protected]>
1 parent eeaee6a commit 2b5016f

File tree

5 files changed

+312
-45
lines changed

5 files changed

+312
-45
lines changed

tagbot/action/git.py

Lines changed: 92 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import re
22
import subprocess
33

4-
from datetime import datetime
4+
from datetime import datetime, timezone
55
from tempfile import mkdtemp
66
from typing import Optional, cast
77
from urllib.parse import urlparse
@@ -10,6 +10,63 @@
1010
from . import Abort
1111

1212

13+
def parse_git_datetime(
14+
date_str: str, _depth: int = 0, _max_depth: int = 2
15+
) -> Optional[datetime]:
16+
"""Parse Git date output into a naive UTC datetime.
17+
18+
Handles common Git formats and normalizes timezone offsets.
19+
Returns None if parsing fails.
20+
"""
21+
22+
def normalize_offset(s: str) -> str:
23+
match = re.search(r"([+-]\d{2})(:?)(\d{2})$", s)
24+
if match and not match.group(2):
25+
# Build prefix separately to avoid an overly long formatted line
26+
prefix = s[: -len(match.group(0))]
27+
return f"{prefix}{match.group(1)}:{match.group(3)}"
28+
return s
29+
30+
cleaned = date_str.strip()
31+
attempts = [cleaned, normalize_offset(cleaned)]
32+
formats = ["%Y-%m-%d %H:%M:%S %z", "%a %b %d %H:%M:%S %Y %z"]
33+
34+
for candidate in attempts:
35+
try:
36+
dt = datetime.fromisoformat(candidate)
37+
except ValueError:
38+
dt = None
39+
if dt:
40+
offset = dt.utcoffset()
41+
if offset:
42+
dt -= offset
43+
return dt.replace(tzinfo=None)
44+
for fmt in formats:
45+
try:
46+
dt = datetime.strptime(candidate, fmt)
47+
except ValueError:
48+
continue
49+
return dt.astimezone(timezone.utc).replace(tzinfo=None)
50+
51+
match = re.search(
52+
r"(\d{4}-\d{2}-\d{2}[T ]\d{2}:\d{2}:\d{2}[+-]\d{2}:?\d{2})",
53+
cleaned,
54+
)
55+
if match:
56+
candidate = normalize_offset(match.group(1))
57+
# Prevent infinite recursion: only recurse if normalization changed
58+
# the matched string and we haven't exceeded a small depth cap.
59+
if (
60+
candidate != match.group(1)
61+
and candidate != date_str
62+
and _depth < _max_depth
63+
):
64+
return parse_git_datetime(candidate, _depth + 1, _max_depth)
65+
return None
66+
67+
return None
68+
69+
1370
class Git:
1471
"""Provides access to a local Git repository."""
1572

@@ -70,18 +127,36 @@ def command(self, *argv: str, repo: Optional[str] = "") -> str:
70127
proc = subprocess.run(args, text=True, capture_output=True)
71128
out = proc.stdout.strip()
72129
if proc.returncode:
73-
error_msg = f"Git command '{self._sanitize_command(cmd)}' failed"
130+
err = proc.stderr.strip()
74131
if out:
75-
stdout = self._sanitize_command(out)
76-
logger.error(f"stdout: {stdout}")
77-
error_msg += f"\nstdout: {stdout}"
78-
if proc.stderr:
79-
stderr = self._sanitize_command(proc.stderr.strip())
80-
logger.error(f"stderr: {stderr}")
81-
error_msg += f"\nstderr: {stderr}"
82-
raise Abort(error_msg)
132+
logger.error(f"stdout: {self._sanitize_command(out)}")
133+
if err:
134+
logger.error(f"stderr: {self._sanitize_command(err)}")
135+
136+
detail = err or out
137+
hint = self._hint_for_failure(detail)
138+
message = f"Git command '{self._sanitize_command(cmd)}' failed"
139+
if detail:
140+
message = f"{message}: {self._sanitize_command(detail)}"
141+
if hint:
142+
message = f"{message} ({hint})"
143+
raise Abort(message)
83144
return out
84145

146+
def _hint_for_failure(self, detail: str) -> Optional[str]:
147+
"""Return a user-facing hint for common git errors."""
148+
lowered = detail.casefold()
149+
if "permission to" in lowered and "denied" in lowered:
150+
return "use a PAT with contents:write or a deploy key"
151+
if "workflow" in lowered or "workflows" in lowered:
152+
if "refusing" in lowered or "permission" in lowered:
153+
return "provide workflow scope or avoid workflow changes"
154+
if "publickey" in lowered or "permission denied (publickey)" in lowered:
155+
return "configure SSH deploy key or switch to https with PAT"
156+
if "bad credentials" in lowered or "authentication failed" in lowered:
157+
return "token is invalid or lacks access"
158+
return None
159+
85160
def check(self, *argv: str, repo: Optional[str] = "") -> bool:
86161
"""Run a Git command, but only return its success status."""
87162
try:
@@ -177,9 +252,10 @@ def time_of_commit(self, sha: str, repo: str = "") -> datetime:
177252
"""Get the time that a commit was made."""
178253
# The format %cI is "committer date, strict ISO 8601 format".
179254
date = self.command("show", "-s", "--format=%cI", sha, repo=repo)
180-
dt = datetime.fromisoformat(date)
181-
# Convert to UTC and remove time zone information.
182-
offset = dt.utcoffset()
183-
if offset:
184-
dt -= offset
185-
return dt.replace(tzinfo=None)
255+
parsed = parse_git_datetime(date)
256+
if not parsed:
257+
logger.warning(
258+
"Could not parse git date '%s', using current UTC", date.strip()
259+
)
260+
return datetime.now(timezone.utc).replace(tzinfo=None)
261+
return parsed

tagbot/action/repo.py

Lines changed: 46 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@
4242
from .. import logger
4343
from . import TAGBOT_WEB, Abort, InvalidProject
4444
from .changelog import Changelog
45-
from .git import Git
45+
from .git import Git, parse_git_datetime
4646

4747
GitlabClient: Any = None
4848
GitlabUnknown: Any = None
@@ -809,12 +809,10 @@ def _build_commit_datetime_cache(self, shas: List[str]) -> None:
809809
if len(parts) == 2:
810810
commit_sha, iso_date = parts
811811
if commit_sha in sha_set:
812-
# Parse ISO 8601 date and convert to UTC without timezone
813-
dt = datetime.fromisoformat(iso_date)
814-
offset = dt.utcoffset()
815-
if offset:
816-
dt = dt - offset
817-
dt = dt.replace(tzinfo=None)
812+
dt = parse_git_datetime(iso_date)
813+
if not dt:
814+
logger.debug("Could not parse git log date '%s'", iso_date)
815+
continue
818816
self.__commit_datetimes[commit_sha] = dt
819817
found += 1
820818
if found >= len(uncached):
@@ -1480,15 +1478,40 @@ def create_release(self, version: str, sha: str, is_latest: bool = True) -> None
14801478
# Use make_latest=False for backfilled old releases to avoid marking them
14811479
# as the "Latest" release on GitHub
14821480
make_latest_str = "true" if is_latest else "false"
1483-
self._repo.create_git_release(
1484-
version_tag,
1485-
version_tag,
1486-
log,
1487-
target_commitish=target,
1488-
draft=self._draft,
1489-
make_latest=make_latest_str,
1490-
generate_release_notes=(self._changelog_format == "github"),
1491-
)
1481+
1482+
def _release_already_exists(exc: GithubException) -> bool:
1483+
data = getattr(exc, "data", {}) or {}
1484+
for err in data.get("errors", []):
1485+
if isinstance(err, dict) and err.get("code") == "already_exists":
1486+
return True
1487+
return "already exists" in str(exc)
1488+
1489+
try:
1490+
self._repo.create_git_release(
1491+
version_tag,
1492+
version_tag,
1493+
log,
1494+
target_commitish=target,
1495+
draft=self._draft,
1496+
make_latest=make_latest_str,
1497+
generate_release_notes=(self._changelog_format == "github"),
1498+
)
1499+
except GithubException as e:
1500+
if e.status == 422 and _release_already_exists(e):
1501+
logger.info(f"Release for tag {version_tag} already exists, skipping")
1502+
return
1503+
elif e.status == 403 and "resource not accessible" in str(e).lower():
1504+
logger.error(
1505+
"Release creation blocked: token lacks required permissions. "
1506+
"Use a PAT with contents:write (and workflows if tagging "
1507+
"workflow changes)."
1508+
)
1509+
elif e.status == 401:
1510+
logger.error(
1511+
"Release creation failed: bad credentials. Refresh the token or "
1512+
"use a PAT with repo scope."
1513+
)
1514+
raise
14921515
logger.info(f"GitHub release {version_tag} created successfully")
14931516

14941517
def _check_rate_limit(self) -> None:
@@ -1523,6 +1546,13 @@ def handle_error(self, e: Exception, *, raise_abort: bool = True) -> None:
15231546
logger.warning("GitHub returned a 5xx error code")
15241547
logger.info(trace)
15251548
allowed = True
1549+
elif e.status == 401:
1550+
logger.error(
1551+
"GitHub returned 401 Bad credentials. Verify that your token "
1552+
"is valid and has access to the repository and registry."
1553+
)
1554+
internal = False
1555+
allowed = False
15261556
elif e.status == 403:
15271557
self._check_rate_limit()
15281558
logger.error(

test/action/test_changelog.py

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
import textwrap
33

44
from datetime import datetime, timedelta, timezone
5-
from unittest.mock import Mock
5+
from unittest.mock import Mock, patch
66

77
import yaml
88

@@ -12,7 +12,12 @@
1212
from tagbot.action.repo import Repo
1313

1414

15-
def _changelog(*, template="", ignore=set(), subdir=None):
15+
@patch("tagbot.action.repo.Github")
16+
def _changelog(mock_gh, *, template="", ignore=set(), subdir=None):
17+
mock_gh_instance = Mock()
18+
mock_gh.return_value = mock_gh_instance
19+
mock_repo = Mock()
20+
mock_gh_instance.get_repo.return_value = mock_repo
1621
r = Repo(
1722
repo="",
1823
registry="",
@@ -31,6 +36,10 @@ def _changelog(*, template="", ignore=set(), subdir=None):
3136
branch=None,
3237
subdir=subdir,
3338
)
39+
# Mock get_all_tags to return empty list (tests override as needed)
40+
r.get_all_tags = Mock(return_value=[])
41+
# Mock _build_tags_cache to return empty dict
42+
r._build_tags_cache = Mock(return_value={})
3443
return r._changelog
3544

3645

@@ -41,12 +50,16 @@ def test_slug():
4150

4251
def test_previous_release():
4352
c = _changelog()
44-
tags = ["ignore", "v1.2.4-ignore", "v1.2.3", "v1.2.2", "v1.0.2", "v1.0.10"]
53+
tags = [
54+
"ignore",
55+
"v1.2.4-ignore",
56+
"v1.2.3",
57+
"v1.2.2",
58+
"v1.0.2",
59+
"v1.0.10",
60+
]
4561
c._repo.get_all_tags = Mock(return_value=tags)
46-
# Mock get_release to return a minimal release-like object
47-
c._repo._repo.get_release = Mock(
48-
side_effect=lambda tag: type("obj", (object,), {"tag_name": tag})()
49-
)
62+
c._repo._repo.get_release = Mock(side_effect=lambda t: Mock(tag_name=t))
5063
assert c._previous_release("v1.0.0") is None
5164
assert c._previous_release("v1.0.2") is None
5265
rel = c._previous_release("v1.2.5")
@@ -95,10 +108,7 @@ def test_previous_release_subdir():
95108
"Foo-v2.0.0",
96109
]
97110
c._repo.get_all_tags = Mock(return_value=tags)
98-
# Mock get_release to return a minimal release-like object
99-
c._repo._repo.get_release = Mock(
100-
side_effect=lambda tag: type("obj", (object,), {"tag_name": tag})()
101-
)
111+
c._repo._repo.get_release = Mock(side_effect=lambda t: Mock(tag_name=t))
102112
assert c._previous_release("Foo-v1.0.0") is None
103113
assert c._previous_release("Foo-v1.0.2") is None
104114
rel = c._previous_release("Foo-v1.2.5")
@@ -249,6 +259,7 @@ def test_collect_data():
249259
c = _changelog()
250260
c._repo._repo = Mock(full_name="A/B.jl", html_url="https://github.com/A/B.jl")
251261
c._repo._project = Mock(return_value="B")
262+
c._repo.is_version_yanked = Mock(return_value=False)
252263
c._previous_release = Mock(
253264
side_effect=[
254265
Mock(tag_name="v1.2.2", created_at=datetime.now(timezone.utc)),

0 commit comments

Comments
 (0)