Add josh cli tool #1518

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

Change: add-cli

vlad-ivanov-name · 2025-10-10T18:12:23Z

josh-core/src/changes.rs

+    let commits: Vec<git2::Commit> = changes
+        .iter()
+        .map(|(_, commit, _)| repo.find_commit(*commit).unwrap())
+        .collect();


Suggested change

let commits: Vec<git2::Commit> = changes

.iter()

.map(|(_, commit, _)| repo.find_commit(*commit).unwrap())

.collect();

let commits: Vec<git2::Commit> = changes

.iter()

.map(|(_, commit, _)| repo.find_commit(*commit))

.collect::<Result<Vec<_>, _>>()?;

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

josh-core/src/changes.rs

+
+    let mut trees: Vec<git2::Tree> = commits
+        .iter()
+        .map(|commit| commit.tree().unwrap())


same as above

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?

vlad-ivanov-name · 2025-10-10T18:23:43Z

josh-core/src/changes.rs

+    }
+
+    Ok(changes
+        .iter()


into_iter()? changes vec isn't used anymore

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?

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

josh-cli/src/bin/josh.rs

+    // Create filtered refs in refs/remotes/{remote_name}/* and refs/heads/*
+    //let mut head_branch_name = None;
+
+    //// First pass: find what HEAD points to


commented out code?

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

josh-cli/src/bin/josh.rs

+        }
+    }
+
+    //  // Set HEAD to point to the default branch and checkout the working directory


commented out code

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

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

josh-cli/src/bin/josh.rs

+    repo.set_head(&head_target)
+        .map_err(|e| format!("Failed to set HEAD: {}", e))?;


we need crates mentioned above and/or thiserror crate for this

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

christian-schilling force-pushed the @changes/master/christian.schilling.de@gmail.com/add-cli branch 4 times, most recently from 8a2518e to ffd2f6c Compare October 10, 2025 17:30

vlad-ivanov-name reviewed Oct 10, 2025

View reviewed changes

josh-core/src/changes.rs

}

Ok(changes

.iter()

Copy link

Collaborator

vlad-ivanov-name Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

into_iter()? changes vec isn't used anymore

vlad-ivanov-name reviewed Oct 10, 2025

View reviewed changes

tests/cli/pull-multiple-remotes.t

Copy link

Collaborator

vlad-ivanov-name Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

vlad-ivanov-name reviewed Oct 10, 2025

View reviewed changes

christian-schilling force-pushed the @changes/master/christian.schilling.de@gmail.com/add-cli branch 4 times, most recently from 77053c0 to d860188 Compare October 12, 2025 10:33

christian-schilling force-pushed the @changes/master/christian.schilling.de@gmail.com/add-cli branch 5 times, most recently from 8d10bb4 to 3f3669d Compare October 14, 2025 19:05

Add josh cli tool

4ebd860

Change: add-cli

christian-schilling force-pushed the @changes/master/christian.schilling.de@gmail.com/add-cli branch from 3f3669d to 4ebd860 Compare October 14, 2025 19:12


		trees.insert(0, repo.find_commit(base)?.tree()?);

		let diffs: Vec<git2::Diff> = (1..trees.len())

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

		repo.set_head(&head_target)
		.map_err(\|e\| format!("Failed to set HEAD: {}", e))?;

Uh oh!

Add josh cli tool #1518

Are you sure you want to change the base?

Add josh cli tool #1518

Uh oh!

Conversation

christian-schilling commented Oct 9, 2025

Uh oh!

vlad-ivanov-name Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vlad-ivanov-name Oct 10, 2025 •

edited

Loading