Unify Author type, optimize performance and fix parsing accuracy #11

eugener · 2025-09-21T23:25:19Z

No description provided.

Remove duplicate Author struct from tag module and use the existing Author type from log module to avoid API ambiguity and maintain consistency across the codebase.

- Replace O(n) git show calls with single git for-each-ref in tag listing - Fix stash branch name parsing by correcting splitn logic - Add proper Unix timestamp parsing for tags and stashes - Update CLAUDE.md to reflect timestamp behavior documentation

Copilot

Pull Request Overview

This PR unifies the Author type across modules, optimizes tag parsing performance, and improves timestamp accuracy. The changes eliminate code duplication by using a shared Author type and switch to more efficient Git commands for better performance.

Unified Author type by removing duplicate struct and importing from log module
Optimized tag listing performance by replacing multiple git show calls with single git for-each-ref command
Enhanced timestamp parsing accuracy by implementing proper Unix timestamp parsing for stashes

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
src/commands/tag.rs	Removes duplicate Author struct, implements efficient git for-each-ref parsing, adds Unix timestamp parsing
src/commands/stash.rs	Adds Unix timestamp parsing and updates stash list format to include actual timestamps
CLAUDE.md	Updates documentation to reflect unified Author type and clarifies default behavior

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-21T23:26:06Z

src/commands/tag.rs

+/// Parse Unix timestamp to DateTime<Utc>
+fn parse_unix_timestamp(timestamp_str: &str) -> Result<DateTime<Utc>> {


The parse_unix_timestamp function is duplicated between tag.rs and stash.rs. Consider moving this utility function to a shared module to avoid code duplication.

Copilot · 2025-09-21T23:26:07Z

src/commands/tag.rs

+
+    if parts.len() < 9 {
+        return Err(GitError::CommandFailed(
+            "Invalid for-each-ref format".to_string(),


The error message 'Invalid for-each-ref format' is not specific enough. Consider including the expected number of parts (9) and actual number received to help with debugging.

Suggested change

"Invalid for-each-ref format".to_string(),

format!(

"Invalid for-each-ref format: expected 9 parts, got {}. Line: '{}'",

parts.len(),

line

),

Copilot · 2025-09-21T23:26:07Z

src/commands/tag.rs

+    // Build tagger information for annotated tags
+    let tagger =
+        if tag_type == TagType::Annotated && !tagger_name.is_empty() && !tagger_email.is_empty() {
+            let timestamp = parse_unix_timestamp(tagger_date).unwrap_or_else(|_| Utc::now());


Using unwrap_or_else(|_| Utc::now()) silently ignores parsing errors. Consider logging the error or using a more explicit fallback strategy for better debugging.

Suggested change

let timestamp = parse_unix_timestamp(tagger_date).unwrap_or_else(|_| Utc::now());

let timestamp = parse_unix_timestamp(tagger_date).unwrap_or_else(|err| {

eprintln!("Failed to parse tagger date '{}': {}. Falling back to Utc::now()", tagger_date, err);

Utc::now()

});

Copilot · 2025-09-21T23:26:07Z

src/commands/stash.rs

+    // Format: "stash@{0} hash timestamp On branch: message"
    let parts: Vec<&str> = line.splitn(4, ' ').collect();

    if parts.len() < 4 {


The parsing logic expects 4 parts but the format comment indicates 'stash@{0} hash timestamp On branch: message' which would be 5+ parts. The splitn(4, ' ') will merge everything after the 4th space into the last element, but the validation should check for at least 4 parts after adding timestamp.

Suggested change

if parts.len() < 4 {

if parts.len() < 4 || !parts[3].contains(':') {

Copilot · 2025-09-21T23:26:07Z

src/commands/stash.rs

    let hash = Hash::from(parts[1]);

+    // Parse timestamp
+    let timestamp = parse_unix_timestamp(parts[2]).unwrap_or_else(|_| Utc::now());


Using unwrap_or_else(|_| Utc::now()) silently ignores parsing errors. Consider logging the error or using a more explicit fallback strategy for better debugging.

Suggested change

let timestamp = parse_unix_timestamp(parts[2]).unwrap_or_else(|_| Utc::now());

let timestamp = parse_unix_timestamp(parts[2])?;

- Extract duplicate parse_unix_timestamp function from tag and stash modules - Add shared implementation to utils module with comprehensive documentation - Update imports in tag.rs and stash.rs to use shared function - Add test coverage for the shared utility function

- Include expected and actual parts count in error message for better debugging - Add test coverage for invalid format error handling - Change from generic "Invalid format" to specific "expected 9 parts, got X"

- Add explicit comments explaining timestamp parsing error handling - Document when and why fallback to current time occurs - Add test for invalid timestamp parsing with fallback verification - Clarify that fallback indicates corrupted git metadata edge case

- Include expected and actual parts count in validation error message - Add validation for empty remainder part containing branch and message - Add comprehensive test coverage for parsing edge cases - Change from generic "Invalid format" to specific "expected 4 parts, got X"

Copilot

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-21T23:48:45Z

src/commands/tag.rs

+            let timestamp = parse_unix_timestamp(tagger_date).unwrap_or_else(|_| {
+                // Timestamp parsing failed - this indicates malformed git metadata
+                // Fall back to current time to avoid breaking the API, but this should be rare
+                Utc::now()
+            });


The fallback to current time when timestamp parsing fails may mask data corruption issues. Consider logging the error or using a more appropriate fallback like epoch time to indicate invalid data."

Copilot · 2025-09-21T23:48:45Z

src/commands/stash.rs

    let hash = Hash::from(parts[1]);

+    // Parse timestamp
+    let timestamp = parse_unix_timestamp(parts[2]).unwrap_or_else(|_| Utc::now());


Using current time as fallback for invalid timestamps is inconsistent with the error handling pattern. This silent fallback may hide data corruption issues and make debugging difficult."

Suggested change

let timestamp = parse_unix_timestamp(parts[2]).unwrap_or_else(|_| Utc::now());

let timestamp = match parse_unix_timestamp(parts[2]) {

Ok(ts) => ts,

Err(e) => {

return Err(GitError::CommandFailed(format!(

"Failed to parse stash timestamp '{}': {}",

parts[2], e

)));

}

};

- Add explicit comments explaining timestamp parsing error handling - Document when and why fallback to current time occurs - Add test for invalid timestamp parsing with fallback verification - Clarify that fallback indicates corrupted git stash metadata edge case

…tion - Replace Utc::now() fallback with Unix epoch (1970-01-01) for invalid timestamps - Make data corruption obvious instead of silently masking with current time - Add test verification for epoch fallback behavior - Ensure consistent error handling pattern across both modules

Copilot

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

src/commands/tag.rs

The DateTime::from_timestamp(0, 0).unwrap_or_else(Utc::now) chain is problematic. If DateTime::from_timestamp(0, 0) fails (which is extremely unlikely), it falls back to the current time, making it inconsistent with the documented behavior of using Unix epoch as a fallback for data corruption. Consider using DateTime::from_timestamp(0, 0).unwrap() or a more explicit error handling approach. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

eugener added 2 commits September 21, 2025 18:59

refactor(tags): unify Author type with log module

02ed63b

Remove duplicate Author struct from tag module and use the existing Author type from log module to avoid API ambiguity and maintain consistency across the codebase.

Copilot AI review requested due to automatic review settings September 21, 2025 23:25

Copilot AI reviewed Sep 21, 2025

View reviewed changes

eugener added 4 commits September 21, 2025 19:29

fix(tags): improve error message specificity in parse_for_each_ref_line

a709047

- Include expected and actual parts count in error message for better debugging - Add test coverage for invalid format error handling - Change from generic "Invalid format" to specific "expected 9 parts, got X"

eugener requested a review from Copilot September 21, 2025 23:48

Copilot AI reviewed Sep 21, 2025

View reviewed changes

eugener added 2 commits September 22, 2025 07:56

eugener requested a review from Copilot September 22, 2025 12:02

Copilot AI reviewed Sep 22, 2025

View reviewed changes

src/commands/tag.rs Outdated Show resolved Hide resolved

eugener and others added 2 commits September 22, 2025 08:06

Bump version from 0.4.0 to 0.5.0

e2681d9

eugener merged commit 543f49e into master Sep 22, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Unify Author type, optimize performance and fix parsing accuracy #11

Unify Author type, optimize performance and fix parsing accuracy #11

Uh oh!

eugener commented Sep 21, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 21, 2025

Uh oh!

Copilot AI Sep 21, 2025

Uh oh!

Copilot AI Sep 21, 2025

Uh oh!

Copilot AI Sep 21, 2025

Uh oh!

Copilot AI Sep 21, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 21, 2025

Uh oh!

Copilot AI Sep 21, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		/// Parse Unix timestamp to DateTime<Utc>
		fn parse_unix_timestamp(timestamp_str: &str) -> Result<DateTime<Utc>> {

-            "Invalid for-each-ref format".to_string(),
+            format!(
+                "Invalid for-each-ref format: expected 9 parts, got {}. Line: '{}'",
+                parts.len(),
+                line
+            ),

	if parts.len() < 4 {
	if parts.len() < 4 \|\| !parts[3].contains(':') {

	let timestamp = parse_unix_timestamp(parts[2]).unwrap_or_else(\|_\| Utc::now());
	let timestamp = parse_unix_timestamp(parts[2])?;

-    let timestamp = parse_unix_timestamp(parts[2]).unwrap_or_else(|_| Utc::now());
+    let timestamp = match parse_unix_timestamp(parts[2]) {
+        Ok(ts) => ts,
+        Err(e) => {
+            return Err(GitError::CommandFailed(format!(
+                "Failed to parse stash timestamp '{}': {}",
+                parts[2], e
+            )));
+        }
+    };

Unify Author type, optimize performance and fix parsing accuracy #11

Unify Author type, optimize performance and fix parsing accuracy #11

Uh oh!

Conversation

eugener commented Sep 21, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants