Add josh cli tool #1518

christian-schilling · 2025-10-09T21:30:43Z

Change: add-cli

josh-core/src/changes.rs

vlad-ivanov-name · 2025-10-10T18:15:44Z

josh-core/src/changes.rs

+    repo: &git2::Repository,
+    changes: &mut Vec<(String, git2::Oid, String)>,
+    base: git2::Oid,
+) -> JoshResult<()> {


For CLI, we should consider using one of the more established error handling libraries, like miette or anyhow. They provide better means to present errors to users, with extra context etc.

vlad-ivanov-name · 2025-10-10T18:17:44Z

josh-core/src/changes.rs

+
+    trees.insert(0, repo.find_commit(base)?.tree()?);
+
+    let diffs: Vec<git2::Diff> = (1..trees.len())


is this tuple_windows?

or .windows(2) from standard library (although we already have itertools dep)

vlad-ivanov-name · 2025-10-10T18:18:36Z

josh-core/src/changes.rs

+        .map(|commit| commit.tree().unwrap())
+        .collect();
+
+    trees.insert(0, repo.find_commit(base)?.tree()?);


you can do

let base = std::iter::once(repo.find_commit(base)?.tree()?);

and then chain iterators

vlad-ivanov-name · 2025-10-10T18:22:16Z

josh-core/src/changes.rs

+    let mut moved = std::collections::HashSet::new();
+    let mut bases = vec![base];
+    for _ in 0..changes.len() {
+        let mut new_bases = vec![];


kinda difficult to follow with all the mutability here -- perhaps some of these can be done iterator style?

josh-core/src/changes.rs

vlad-ivanov-name · 2025-10-10T18:26:07Z

josh-core/src/changes.rs

+            if seen.contains(&id) {
+                return Err(josh_error(&format!(
+                    "rejecting to push {:?} with duplicate label",
+                    change.commit
+                )));
+            }
+            seen.push(id);


for me this would be easier to read as folding on HashSet but up to you

vlad-ivanov-name · 2025-10-10T18:27:30Z

josh-core/src/changes.rs

+                format!(
+                    "refs/heads/@changes/{}/{}/{}",
+                    baseref.replacen("refs/heads/", "", 1),
+                    change.author,
+                    change.id.as_ref().unwrap_or(&"".to_string()),


it would be cool to move it into a separate function to give all the different pieces meaningful names, be able to test it in unit tests, and to reuse this elsewhere if needed

vlad-ivanov-name · 2025-10-10T18:28:19Z

josh-core/src/changes.rs

+    push_mode: PushMode,
+    baseref: &str,
+    author: &str,
+    ref_with_options: String,


would it make sense for this also to be a &str? looks a bit weird that we're taking previous two by reference but move in this

vlad-ivanov-name · 2025-10-10T18:29:36Z

josh-core/src/changes.rs

+    old: git2::Oid,
+) -> JoshResult<Vec<(String, git2::Oid, String)>> {
+    if let Some(changes) = changes {
+        let mut v = vec![];


not really a meaningful name

vlad-ivanov-name · 2025-10-10T18:30:04Z

josh-core/src/changes.rs

+        if push_mode == PushMode::Split {
+            split_changes(repo, &mut v, old)?;
+        }
+        if push_mode == PushMode::Review {


vlad-ivanov-name · 2025-10-10T18:31:39Z

tests/cli/pull-multiple-remotes.t

nit: other files in the dir are named with underscores as opposed to dashes

vlad-ivanov-name · 2025-10-10T18:35:06Z

josh-cli/src/bin/josh.rs

+        Command::Clone(args) => {
+            if let Err(e) = handle_clone(args) {
+                eprintln!("Error: {}", e);
+                std::process::exit(1);
+            }
+        }
+        Command::Fetch(args) => {
+            if let Err(e) = handle_fetch(args) {
+                eprintln!("Error: {}", e);
+                std::process::exit(1);
+            }
+        }
+        Command::Pull(args) => {
+            if let Err(e) = handle_pull(args) {
+                eprintln!("Error: {}", e);
+                std::process::exit(1);
+            }
+        }
+        Command::Push(args) => {
+            if let Err(e) = handle_push(args) {


if let Err() can/should move outside of match

vlad-ivanov-name · 2025-10-10T18:35:33Z

josh-cli/src/bin/josh.rs

+    repo_shell: &Shell,
+    filter: &str,
+    remote_name: &str,
+) -> Result<(), Box<dyn std::error::Error>> {


see my other comment about error libraries -- this is anyhow/miette/color-eyre

vlad-ivanov-name · 2025-10-10T18:36:28Z

josh-cli/src/bin/josh.rs

+    let original_dir = std::env::current_dir()?;
+    std::env::set_current_dir(repo_shell.cwd.as_path())?;


this is super hacky -- why would we manipulate global state? repo_shell.cwd can be passed as arg pretty much anywhere?

josh-cli/src/bin/josh.rs

vlad-ivanov-name · 2025-10-10T18:37:53Z

josh-cli/src/bin/josh.rs

+
+    // Change to the repository directory and add the remote using handle_remote_add
+    let original_dir = std::env::current_dir()?;
+    std::env::set_current_dir(&output_dir)?;


see other comment about set_current_dir

vlad-ivanov-name · 2025-10-10T18:38:58Z

josh-cli/src/bin/josh.rs

+fn handle_pull(args: &PullArgs) -> Result<(), Box<dyn std::error::Error>> {
+    // Check if we're in a git repository
+    let repo =
+        git2::Repository::open_from_env().map_err(|e| format!("Not in a git repository: {}", e))?;


it doesn't need to be from env here, it can be from path that we pass; we can figure out path at top level

josh-cli/src/bin/josh.rs

vlad-ivanov-name · 2025-10-10T18:40:19Z

josh-cli/src/bin/josh.rs

+    };
+
+    // Get PATH environment variable for shell commands
+    let path_env = std::env::var("PATH").unwrap_or_default();


what purpose does it serve?

Change: add-cli

vlad-ivanov-name · 2025-11-11T02:48:35Z

@claude address unresolved comments from this PR in a new PR

claude · 2025-11-11T02:48:51Z

This commit addresses the unresolved review comments from the merged PR that added the josh CLI tool: ## Error Handling Improvements - Add anyhow and thiserror to workspace dependencies - Replace Box<dyn std::error::Error> with anyhow::Result throughout josh-cli - Use anyhow::Context for better error messages with context ## josh-core/src/changes.rs Improvements - Use into_iter() instead of iter() where changes vec is consumed - Replace manual indexing with .windows(2) for cleaner iteration - Use iterator chaining with base_tree for tree collection - Use HashSet::insert() which returns false for duplicates - Improve variable naming: old -> base_oid, v -> push_refs - Change ref_with_options parameter from String to &str for consistency - Replace if let with match for better pattern matching - Use filter_map instead of filter + map ## josh-cli/src/bin/josh.rs Improvements - Remove all std::env::set_current_dir() calls (hacky global state) - Create internal helper functions that accept repo_path parameter: - handle_fetch_internal(args, repo_path) - handle_remote_add_internal(args, repo_path) - handle_filter_internal(args, repo_path, print_messages) - apply_josh_filtering(repo_path, filter, remote_name) - Remove commented out code - Update all error handling to use anyhow::Context - Fix build_to_push call to pass &remote_ref instead of remote_ref 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Vlad Ivanov <[email protected]>

christian-schilling force-pushed the @changes/master/[email protected]/add-cli branch 4 times, most recently from 8a2518e to ffd2f6c Compare October 10, 2025 17:30