Skip to content

Commit 72804f3

Browse files
authored
fix: fix annotated tags in git detection bug (#149)
1 parent 2e4cf61 commit 72804f3

File tree

17 files changed

+755
-297
lines changed

17 files changed

+755
-297
lines changed
Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
# Plan: Fix Annotated Tag Detection Bug
2+
3+
**Status**: In Progress (Phase 1 & 2 Complete, Phase 3 In Progress)
4+
**Priority**: High
5+
**Context**: zerv flow and zerv version commands do not properly detect annotated tags, causing version detection to fail when repositories use annotated tags instead of lightweight tags.
6+
7+
## Goals
8+
9+
1. Add comprehensive support for annotated tags in test utilities
10+
2. Create test cases that replicate the annotated tag bug
11+
3. Fix any issues in tag detection logic for annotated tags
12+
4. Ensure both annotated and lightweight tags work correctly
13+
14+
## Implementation Plan
15+
16+
### Phase 1: Enhance Test Infrastructure
17+
18+
1. **Add annotated tag support to GitOperations trait**
19+
- Extend the `GitOperations` trait with a new method: `create_annotated_tag(&self, test_dir: &TestDir, tag: &str, message: &str) -> io::Result<()>`
20+
- Implement for both `NativeGit` and `DockerGit`
21+
- Use `git tag -a <tag> -m "<message>"` command
22+
23+
2. **Add annotated tag support to GitRepoFixture**
24+
- Add `create_annotated_tag(self, tag: &str, message: &str) -> Self` method
25+
- Add `with_annotated_tag(self, tag: &str, message: &str) -> Self` builder-style method
26+
- Add static constructor: `GitRepoFixture::tagged_annotated(tag: &str, message: &str) -> Result<Self>`
27+
28+
### Phase 2: Create Comprehensive Test Cases
29+
30+
1. **Create annotated tag test mirroring existing lightweight tag test**
31+
- Duplicate `test_get_latest_tag_comprehensive` as `test_get_latest_tag_comprehensive_annotated`
32+
- Use annotated tags instead of lightweight tags
33+
- Test all scenarios: single tag, multiple tags, mixed formats, commit distances
34+
35+
2. **Test mixed tag type scenarios**
36+
- Repository with both annotated and lightweight tags on same commit
37+
- Repository with annotated tags on different commits
38+
- Timestamp behavior differences between tag types
39+
40+
3. **Test edge cases**
41+
- Annotated tag with special characters in message
42+
- Multiple annotated tags with different timestamps
43+
- Annotated tags with GPG signatures (if applicable)
44+
45+
### Phase 3: Investigate and Fix Bug
46+
47+
1. **Debug current tag detection with annotated tags**
48+
- Run existing git commands against repositories with annotated tags
49+
- Identify which commands fail or return unexpected results
50+
- Check if `git tag --merged` handles annotated tags correctly
51+
- Verify timestamp extraction works for both tag types
52+
53+
2. **Fix tag detection logic if needed**
54+
- Update `get_merged_tags()` if it doesn't include annotated tags
55+
- Fix timestamp handling in `get_tag_timestamp()` if there are issues
56+
- Ensure `get_tag_commit_hash()` works correctly for annotated tags
57+
58+
3. **Update git command execution**
59+
- Check if any git commands need different flags for annotated tags
60+
- Verify error handling covers annotated tag-specific failures
61+
62+
### Phase 4: Integration Testing
63+
64+
1. **Test with zerv flow command**
65+
- Create integration test using `TestCommand` with annotated tag repository
66+
- Verify version output is correct
67+
- Test all format options (auto, semver, pep440)
68+
69+
2. **Test with zerv version command**
70+
- Similar integration test for standalone version command
71+
- Verify consistency between flow and version commands
72+
73+
3. **Test real-world scenarios**
74+
- Import a real git repository with annotated tags
75+
- Test version detection on complex histories
76+
77+
## Testing Strategy
78+
79+
### Unit Tests
80+
81+
- Test new annotated tag creation methods in test utilities
82+
- Test tag type detection (`git cat-file -t`)
83+
- Test timestamp extraction for both tag types
84+
- Test version parsing with annotated tags
85+
86+
### Integration Tests
87+
88+
- Use `ZERV_TEST_NATIVE_GIT=false ZERV_TEST_DOCKER=true` for Docker-based tests
89+
- Create comprehensive test scenarios covering all tag combinations
90+
- Test with multiple version formats
91+
92+
### Manual Verification
93+
94+
- Create test repositories manually with annotated tags
95+
- Verify zerv commands work correctly
96+
- Compare behavior with lightweight tags
97+
98+
## Success Criteria
99+
100+
1. ✅ All tests pass with annotated tags
101+
2. ✅ zerv flow detects annotated tags correctly
102+
3. ✅ zerv version detects annotated tags correctly
103+
4. ✅ Mixed repositories (both tag types) work correctly
104+
5. ✅ No regression in lightweight tag functionality
105+
6. ✅ Test coverage includes annotated tag scenarios
106+
107+
## Documentation Updates
108+
109+
1. Update testing documentation to mention annotated tag support
110+
2. Add examples of creating annotated tags in tests
111+
3. Document any differences in behavior between tag types
112+
113+
## Files to Modify
114+
115+
### New Files
116+
117+
- None
118+
119+
### Modified Files
120+
121+
- `src/test_utils/git/mod.rs` - Add annotated tag method to trait
122+
- `src/test_utils/git/native.rs` - Implement annotated tag creation
123+
- `src/test_utils/git/docker.rs` - Implement annotated tag creation with verification
124+
- `src/test_utils/git/fixtures.rs` - Add annotated tag fixture methods
125+
- `src/vcs/git.rs` - Add comprehensive annotated tag tests
126+
- `tests/integration_tests/` - Add integration tests for annotated tags
127+
128+
## Risk Assessment
129+
130+
**Low Risk**:
131+
132+
- Adding new methods to test utilities doesn't affect existing code
133+
- Tests are additive and don't change existing behavior
134+
135+
**Medium Risk**:
136+
137+
- Bug fixes to tag detection logic might affect lightweight tag behavior
138+
- Need thorough regression testing
139+
140+
**Mitigation**:
141+
142+
- Run full test suite after each change
143+
- Keep lightweight tag tests intact
144+
- Test with real repositories before merging
Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,177 @@
1+
# Plan: Rewrite get_latest_tag to Properly Detect Annotated Tags
2+
3+
**Status**: Planned
4+
**Priority**: High
5+
**Context**: The current `get_latest_tag` implementation in src/vcs/git.rs cannot properly detect annotated tags because it uses `git tag --merged HEAD` which doesn't correctly handle the topological order of commits and their associated tags. The TODO comment in the code outlines the correct approach.
6+
7+
## Goals
8+
9+
1. Rewrite `get_latest_tag` method to use topological commit traversal
10+
2. Ensure both annotated and lightweight tags are detected correctly
11+
3. Make `test_get_latest_tag_comprehensive_annotated` test pass
12+
4. Maintain backward compatibility with existing functionality
13+
14+
## Implementation Plan
15+
16+
### Step 1: Understand the Current Algorithm's Limitations
17+
18+
The current algorithm:
19+
20+
1. Gets all merged tags using `git tag --merged HEAD --sort=-committerdate`
21+
2. Finds the latest valid version tag from this list
22+
3. Gets the commit hash for that tag
23+
4. Gets all tags pointing to that commit
24+
5. Filters and finds the maximum version tag
25+
26+
**Problems:**
27+
28+
- `git tag --merged` doesn't guarantee topological order
29+
- May miss tags on commits that are not direct ancestors
30+
- Doesn't properly handle multiple tags on the same commit
31+
32+
### Step 2: Implement the New Algorithm
33+
34+
Following the TODO comment's guidance, the new algorithm will:
35+
36+
1. **Get all commits from HEAD in topological order**
37+
38+
```bash
39+
git rev-list --topo-order HEAD
40+
```
41+
42+
This returns commits in topological order (parents before children)
43+
44+
2. **Iterate through each commit and find tags pointing at it**
45+
46+
```bash
47+
git tag --points-at <commit-hash>
48+
```
49+
50+
3. **Filter and validate tags for each commit**
51+
- Use `GitUtils::filter_only_valid_tags(&tags, format)`
52+
- If no valid tags found, continue to next commit
53+
54+
4. **Find the maximum version tag among valid tags**
55+
- Use `GitUtils::find_max_version_tag(&valid_tags)`
56+
- Return the first found (since we're in topological order)
57+
58+
### Step 3: Code Implementation
59+
60+
First, add a helper method to get commits in topological order:
61+
62+
```rust
63+
/// Get all commits from HEAD in topological order
64+
fn get_commits_in_topo_order(&self) -> Result<Vec<String>> {
65+
let commits_output = self.run_git_command(&["rev-list", "--topo-order", "HEAD"])?;
66+
Ok(commits_output
67+
.lines()
68+
.map(|line| line.trim().to_string())
69+
.filter(|hash| !hash.is_empty())
70+
.collect())
71+
}
72+
```
73+
74+
Then, the new `get_latest_tag` method will reuse the existing `get_all_tags_from_commit_hash`:
75+
76+
```rust
77+
fn get_latest_tag(&self, format: &str) -> Result<Option<String>> {
78+
// Get all commits from HEAD in topological order
79+
let commits = self.get_commits_in_topo_order()?;
80+
81+
// Process each commit in topological order
82+
for commit_hash in commits {
83+
// Get all tags pointing to this commit (reusing existing function)
84+
let tags = self.get_all_tags_from_commit_hash(&commit_hash)?;
85+
86+
// If no tags, continue to next commit
87+
if tags.is_empty() {
88+
continue;
89+
}
90+
91+
// Filter tags by format
92+
let valid_tags = GitUtils::filter_only_valid_tags(&tags, format);
93+
94+
// If no valid tags, continue to next commit
95+
if valid_tags.is_empty() {
96+
continue;
97+
}
98+
99+
// Find and return the maximum version tag
100+
if let Some(max_tag) = GitUtils::find_max_version_tag(&valid_tags)? {
101+
return Ok(Some(max_tag));
102+
}
103+
}
104+
105+
// No valid tags found
106+
Ok(None)
107+
}
108+
```
109+
110+
Note: No changes needed to `get_tags_pointing_at_commit` or `get_all_tags_from_commit_hash` - we'll reuse the existing `get_all_tags_from_commit_hash` function.
111+
112+
### Step 4: Testing Strategy
113+
114+
1. **Enable the commented-out test cases**
115+
- Uncomment all test scenarios in `test_get_latest_tag_comprehensive_annotated`
116+
- Ensure the test uses proper annotated tag creation
117+
118+
2. **Run the test to verify the fix**
119+
120+
```bash
121+
ZERV_TEST_NATIVE_GIT=false ZERV_TEST_DOCKER=true cargo test test_get_latest_tag_comprehensive_annotated
122+
```
123+
124+
3. **Run existing tests to ensure no regression**
125+
```bash
126+
ZERV_TEST_NATIVE_GIT=false ZERV_TEST_DOCKER=true cargo test
127+
```
128+
129+
### Step 5: Additional Considerations
130+
131+
1. **Performance**: The new algorithm may be slower as it checks each commit
132+
- Consider adding early termination if we find a tag on HEAD
133+
- Cache results if needed
134+
135+
2. **Edge cases to handle**:
136+
- Empty repositories (no commits)
137+
- Repositories with no tags
138+
- Mixed annotated and lightweight tags
139+
- Tags with GPG signatures
140+
141+
3. **Error handling**:
142+
- Handle empty output from `git rev-list`
143+
- Ensure proper error propagation
144+
145+
## Success Criteria
146+
147+
1. ✅ `test_get_latest_tag_comprehensive_annotated` test passes
148+
2. ✅ All existing tag-related tests still pass
149+
3. ✅ Both annotated and lightweight tags work correctly
150+
4. ✅ No performance regression for typical repositories
151+
5. ✅ Proper error handling for edge cases
152+
153+
## Files to Modify
154+
155+
### Modified Files
156+
157+
- `src/vcs/git.rs` - Add the `get_commits_in_topo_order` helper method and rewrite the `get_latest_tag` method (lines 131-151) to use topological commit traversal
158+
159+
## Risk Assessment
160+
161+
**Medium Risk**:
162+
163+
- Changing a core algorithm that affects version detection
164+
- Potential performance impact on large repositories
165+
166+
**Mitigation**:
167+
168+
- Thorough testing with various repository types
169+
- Performance benchmarking on large commit histories
170+
- Keep the existing algorithm as a fallback if needed
171+
172+
## Notes
173+
174+
- The implementation follows the exact guidance from the TODO comment
175+
- This approach is more robust as it checks commits in topological order
176+
- The algorithm will work correctly for both annotated and lightweight tags
177+
- No changes needed to test utilities as they already support annotated tags

.github/workflows/cd.yml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,12 @@ jobs:
3737
"${{ fromJson(needs.zerv-versioning.outputs.versions).v_semver_major }}",
3838
"${{ fromJson(needs.zerv-versioning.outputs.versions).v_semver_major_minor }}"
3939
]
40+
# TODO: update this after release
41+
# tags: >-
42+
# [
43+
# "${{ fromJson(needs.zerv-versioning.outputs.versions).v_major }}",
44+
# "${{ fromJson(needs.zerv-versioning.outputs.versions).v_major_minor }}"
45+
# ]
4046

4147
# ====================================================
4248
# Test and Lint

.github/workflows/shared-zerv-versioning.yml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,13 @@ on:
6060
"v_semver_major": "v{{ major }}",
6161
"v_semver_major_minor": "v{{ major }}.{{ minor }}"
6262
}
63+
# TODO: update this after release
64+
# default: >-
65+
# {
66+
# "docker_tag": "{{ semver_obj.docker }}",
67+
# "v_major": "v{{ major | default(value="0") }}",
68+
# "v_major_minor": "v{{ major | default(value="0") }}{{ prefix_if(value=minor, prefix=".") }}"
69+
# }
6370
custom_outputs:
6471
type: string
6572
required: false

0 commit comments

Comments
 (0)