-
-
Notifications
You must be signed in to change notification settings - Fork 25
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.
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
Added a more aggressive sorting mechanism for git tags #9
Added a more aggressive sorting mechanism for git tags #9
Conversation
This should ensure that tags that are more recent topologically will always take precendence. Additionally we prefer using the timestamp from the tag annotation if present.
@@ -113,6 +113,9 @@ def test__version__from_git__with_annotated_tags(tmp_path) -> None: | |||
with pytest.raises(ValueError): | |||
from_vcs(latest_tag=True) | |||
|
|||
# check that we find the expected tag that has the most recent tag creation time | |||
avoid_identical_ref_timestamps() | |||
run("git tag v0.2.0b1 -m Annotated") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously in this case v0.2.0b1
would be matched instead of v0.2.0
Thanks for this! I like the fix for the v0.2.0b1 vs v0.2.0 matching, and the time zone comment sounds promising, but I'd like to make sure I understand exactly how this improvement works and maybe if we can simplify some of it. DatesIn my testing, the
Time zonesI thought the original implementation would have also handled different time zones because of the Test with v0.2.0b1I verified that adding v0.2.0b1 made |
So there are a two parts going on here datesThe new priority of dates results in
topo oderThe Say we have the following commits
We'd detect HEAD correctly as if The topology changes means we'd always prefer tags closer to the current HEAD, and the timestamp logic is used mostly to resolve ties where multiple tags exist on a single commit. |
Ah, I think you're using This minimal change also fixes the v0.2.0b1 test: diff --git a/dunamai/__init__.py b/dunamai/__init__.py
index 0cd245f..714fff3 100644
--- a/dunamai/__init__.py
+++ b/dunamai/__init__.py
@@ -393,16 +393,17 @@ class Version:
detailed_tags = []
for line in msg.strip().splitlines():
parts = line.split("@{")
+ creator_date = _parse_git_timestamp_iso_strict(parts[1])
detailed_tags.append(
(
parts[0].replace("refs/tags/", "", 1),
- _parse_git_timestamp_iso_strict(parts[1]),
- None if parts[2] == "" else _parse_git_timestamp_iso_strict(parts[2]),
+ creator_date,
+ creator_date if parts[2] == "" else _parse_git_timestamp_iso_strict(parts[2]),
)
)
tags = [
t[0]
- for t in reversed(sorted(detailed_tags, key=lambda x: x[1] if x[2] is None else x[2]))
+ for t in reversed(sorted(detailed_tags, key=lambda x: (x[2], x[1])))
]
tag, base, stage, unmatched, tagged_metadata = _match_version_pattern(
pattern, tags, latest_tag
Can you think of any cases where the above would behave differently than your proposed changes? |
Since you can specify and amend both committer and author dates for git commits it could lead to some strange cases if you just ignore the graph structure. |
Additionally any repo that makes use of git-am or other similar patch based workflow would not have commits in chronological order. Rebases can also easily do this. |
Got it; that's a great point. Thanks for the explanation 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for this improvement! I'll plan to add some non-chronological commit tests and maybe tweak some stylistic stuff before releasing, but it shouldn't take long.
This should ensure that tags that are more recent topologically will always take precedence. Additionally we prefer using the timestamp from the tag (if annotated), which ensures that annotated tags created more recently for the same commit are selected.
This should prevent edge cases arising from multiple timezones being used to commit to the same git repository.