feat: manual repository onboarding (diddo onboard)

[drugoi]

Summary

Adds diddo onboard to backfill commit history for the current repo from a user-chosen cutoff date (on or after), with identity selection and optional saved aliases under [onboarding] in config.

What’s included

• CLI: onboard subcommand (no summary output flags)
• Config: OnboardingConfig, IdentityAlias, save_onboarding_aliases
• Core: git log scan (GIT_LOG_FORMAT), filter/import, preselection from current git user + aliases + detected authors
• Interactive flow: prompts for cutoff, numbered identities, comma-separated selection, optional save to config
• Tests: unit tests + injected run_with integration-style tests
• Docs: README section, design + implementation plan under docs/plans/

How to try

cargo run -- onboard

(from a git repo root; uses real git in cwd)

Verification

• cargo test
• cargo clippy -- -D warnings

Made with Cursor

[maximgorbatyuk]

Review summary

Well-structured PR with good separation of concerns, solid test coverage, and thorough documentation. The run_with injection pattern is great for testability.

However, there's one critical data-loss bug (onboarding overwrites hook-recorded diff stats) and a high-severity dedup issue (hash length change) that should be fixed before merge. Details in line comments.

[maximgorbatyuk]

Critical: onboarding will silently destroy hook-recorded diff stats

to_db_commit hardcodes files_changed: 0, insertions: 0, deletions: 0. Since db::insert_commit uses ON CONFLICT(repo_path, hash) DO UPDATE SET ... unconditionally, re-importing a commit that the hook already recorded will overwrite its real diff stats with zeroes.

The test import_is_deduped_when_commit_already_exists_from_hook_style_row passes only because commit_count stays the same — but the row was silently mutated.

Suggested fix: add a separate insert method (or flag) that uses INSERT ... ON CONFLICT DO NOTHING:

// in db.rs
pub fn insert_commit_if_new(&self, commit: &Commit) -> Result<bool> {
    let rows = self.connection.execute(
        "INSERT INTO commits (
            hash, message, repo_path, repo_name, branch,
            files_changed, insertions, deletions, committed_at, author_email
        ) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10)
        ON CONFLICT(repo_path, hash) DO NOTHING",
        params![
            &commit.hash, &commit.message, &commit.repo_path,
            &commit.repo_name, &commit.branch, commit.files_changed,
            commit.insertions, commit.deletions,
            commit.committed_at.to_rfc3339(), &commit.author_email,
        ],
    )?;
    Ok(rows > 0)
}

Then use it in import_commits and count inserted directly:

let mut inserted = 0;
for c in commits {
    let row = to_db_commit(c, repo_path, repo_name, branch);
    if database.insert_commit_if_new(&row)? {
        inserted += 1;
    }
}

This also eliminates the commit_count before/after race.

Prompt: Apply the fix above — add insert_commit_if_new to db.rs, use it in import_commits, remove the commit_count before/after logic, and update the dedup test to assert the existing row's files_changed was NOT overwritten.

[drugoi]

Addressed in 7ce9f4a: added Database::insert_commit_if_new (INSERT … ON CONFLICT DO NOTHING) and switched onboarding import_commits to use it. Counts now come from rows inserted, not commit_count before/after. Extended import_is_deduped_when_commit_already_exists_from_hook_style_row to assert files_changed / insertions / deletions stay intact after a duplicate import attempt.

[maximgorbatyuk]

High: hash length change breaks dedup with existing database rows

Switching from rev-parse --short HEAD (7-char) to rev-parse HEAD (40-char SHA) is a good move for correctness, but it's a silent breaking change for existing databases.

Old hook-recorded rows have short hashes (e.g. "abc1234"). Onboarding scans with %H (full SHA). The unique constraint is on (repo_path, hash), so these won't conflict — onboarding will insert duplicate entries for every commit that was recorded before this upgrade.

Suggested fix options (pick one):

1. Prefix-match on conflict — before importing, check if a row with a matching hash prefix already exists:

fn commit_exists_by_prefix(db: &Database, repo_path: &str, full_hash: &str) -> Result<bool> {
    // check both exact match and short-prefix match
    db.connection.query_row(
        "SELECT COUNT(*) FROM commits WHERE repo_path = ?1 AND (hash = ?2 OR hash = ?3)",
        params![repo_path, full_hash, &full_hash[..7.min(full_hash.len())]],
        |row| row.get::<_, i64>(0),
    ).map(|c| c > 0)
}

2. One-time migration — expand all short hashes in the DB to full hashes on startup (via git rev-parse <short>).

3. At minimum — document this in the README/changelog so users know to expect duplicates if they onboard a repo with pre-upgrade hook data.

Prompt: Choose an approach and implement it. If going with option 1, integrate the prefix check into import_commits. If going with option 2, add a migration step in Database::open. If option 3, add a note to the README under the onboarding section.

[drugoi]

We went with a one-time migration on DB open: run_commit_hash_migration expands abbreviated OIDs via git rev-parse when the repo path still exists, and drops the short row if a full-hash row already exists. Together with the hook now storing full rev-parse HEAD, onboarding imports align on the same key without duplicate rows after migration runs.

[maximgorbatyuk]

Medium: case-sensitivity inconsistency between identity_same and identity_matches

identity_same uses exact ==, but identity_matches (line 414) uses eq_ignore_ascii_case for both email and name. This means:

• An alias saved as "Me@Example.com" and a detected identity "me@example.com" won't show as preselected in the UI (exact mismatch)
• But they will match during import filtering (case-insensitive match)
• Alias dedup in the save flow will also miss case variants, creating near-duplicate saved aliases

Suggested fix:

fn identity_same(a: &IdentityCandidate, b: &IdentityCandidate) -> bool {
    let email_eq = match (&a.email, &b.email) {
        (Some(ae), Some(be)) => ae.eq_ignore_ascii_case(be),
        (None, None) => true,
        _ => false,
    };
    let name_eq = match (&a.name, &b.name) {
        (Some(an), Some(bn)) => an.eq_ignore_ascii_case(bn),
        (None, None) => true,
        _ => false,
    };
    email_eq && name_eq
}

Prompt: Replace identity_same with the case-insensitive version above and add a test that verifies identity_same treats "Me@X.com" and "me@x.com" as equal.

[drugoi]

Fixed in 7ce9f4a: shared optional_str_eq_ignore_case for email and name on identity_same, plus identity_alias_same for the “save aliases” merge / has_new_alias checks so preselection, import filtering, and config dedup stay consistent. Added identity_same_treats_ascii_case_variants_as_equal.

[maximgorbatyuk]

Medium: fixed temp file paths can collide in parallel test runs

These integration tests use hard-coded filenames in std::env::temp_dir() (e.g. "diddo-onboard-empty.toml"). The rest of the test suite uses a temp_dir() helper that creates unique directories with timestamps.

With cargo test running tests in parallel, two runs hitting the same file can cause flaky failures.

Suggested fix: reuse the existing temp_dir helper pattern from config.rs tests:

fn temp_dir(label: &str) -> std::path::PathBuf {
    let dir = std::env::temp_dir().join(format!(
        "diddo-onboard-{}-{}",
        label,
        std::time::SystemTime::now()
            .duration_since(std::time::UNIX_EPOCH)
            .unwrap()
            .as_nanos()
    ));
    std::fs::create_dir_all(&dir).unwrap();
    dir
}

// then:
let temp = temp_dir("empty");
let config_path = temp.join("config.toml");
// ... at end:
let _ = std::fs::remove_dir_all(temp);

Apply to all three integration tests.

Prompt: Add a temp_dir helper to the onboarding test module and update all three integration tests to use unique temp directories with cleanup.

[drugoi]

Done in 7ce9f4a: onboarding integration tests now use a temp_dir_onboard helper (pid + nanos under std::env::temp_dir()) and remove the directory after each test, matching the pattern used in config tests.