-
Notifications
You must be signed in to change notification settings - Fork 332
Support non-colocated Jujutsu repositories #1847
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ use self::license::LicenseInfo; | |
| use self::loc::LocInfo; | ||
| use self::pending::PendingInfo; | ||
| use self::project::ProjectInfo; | ||
| use self::repository::Repository as DiscoveredRepository; | ||
| use self::size::SizeInfo; | ||
| use self::title::Title; | ||
| use self::url::UrlInfo; | ||
|
|
@@ -46,6 +47,7 @@ mod license; | |
| mod loc; | ||
| mod pending; | ||
| mod project; | ||
| mod repository; | ||
| mod size; | ||
| mod title; | ||
| mod url; | ||
|
|
@@ -109,8 +111,9 @@ impl std::fmt::Display for Info { | |
| } | ||
|
|
||
| pub fn build_info(cli_options: &CliOptions) -> Result<Info> { | ||
| let repo = gix::discover(&cli_options.input)?; | ||
| let repo_path = get_work_dir(&repo)?; | ||
| let discovered_repo = DiscoveredRepository::discover(&cli_options.input)?; | ||
| let repo = discovered_repo.git(); | ||
| let repo_path = discovered_repo.work_dir().to_owned(); | ||
| // Compute LOC in a separate thread so it runs in parallel with commit-graph traversal. | ||
| let loc_by_language_sorted_handle = std::thread::spawn({ | ||
| let globs_to_exclude = cli_options.info.exclude.clone(); | ||
|
|
@@ -127,19 +130,17 @@ pub fn build_info(cli_options: &CliOptions) -> Result<Info> { | |
| } | ||
| }); | ||
| let git_metrics = traverse_commit_graph( | ||
| &repo, | ||
| repo, | ||
| discovered_repo.head_id()?, | ||
| discovered_repo.is_jujutsu(), | ||
| cli_options.info.no_bots.clone(), | ||
| cli_options.info.churn_pool_size, | ||
| cli_options.info.no_merges, | ||
| ) | ||
| .context("Failed to traverse Git commit history")?; | ||
| let manifest = get_manifest(&repo_path)?; | ||
| let repo_url = get_repo_url( | ||
| &repo, | ||
| cli_options.info.hide_token, | ||
| cli_options.info.http_url, | ||
| ) | ||
| .context("Failed to determine repository URL")?; | ||
| let repo_url = get_repo_url(repo, cli_options.info.hide_token, cli_options.info.http_url) | ||
| .context("Failed to determine repository URL")?; | ||
| let true_color = match cli_options.ascii.true_color { | ||
| When::Always => true, | ||
| When::Never => false, | ||
|
|
@@ -169,12 +170,12 @@ pub fn build_info(cli_options: &CliOptions) -> Result<Info> { | |
| let show_email = cli_options.info.email; | ||
|
|
||
| Ok(InfoBuilder::new(cli_options) | ||
| .title(&repo, no_bold, &text_colors) | ||
| .project(&repo, &repo_url, manifest.as_ref(), number_separator)? | ||
| .title(repo, no_bold, &text_colors) | ||
| .project(repo, &repo_url, manifest.as_ref(), number_separator)? | ||
| .description(manifest.as_ref()) | ||
| .head(&repo)? | ||
| .pending(&repo)? | ||
| .version(&repo, manifest.as_ref())? | ||
| .head(repo, discovered_repo.jujutsu_head())? | ||
| .pending(repo, discovered_repo.is_jujutsu())? | ||
| .version(repo, manifest.as_ref())? | ||
| .created(&git_metrics, iso_time) | ||
| .languages( | ||
| loc_by_language.as_ref(), | ||
|
|
@@ -201,7 +202,7 @@ pub fn build_info(cli_options: &CliOptions) -> Result<Info> { | |
| number_separator, | ||
| )? | ||
| .loc(loc_by_language.as_ref(), number_separator) | ||
| .size(&repo, number_separator) | ||
| .size(repo, number_separator, discovered_repo.is_jujutsu()) | ||
| .license(&repo_path, manifest.as_ref())? | ||
| .build(cli_options, text_colors, dominant_language, ascii_colors)) | ||
| } | ||
|
|
@@ -238,8 +239,8 @@ impl InfoBuilder { | |
| self | ||
| } | ||
|
|
||
| fn pending(mut self, repo: &Repository) -> Result<Self> { | ||
| if !self.disabled_fields.contains(&InfoType::Pending) { | ||
| fn pending(mut self, repo: &Repository, is_jujutsu: bool) -> Result<Self> { | ||
| if !is_jujutsu && !self.disabled_fields.contains(&InfoType::Pending) { | ||
| let pending = PendingInfo::new(repo)?; | ||
| self.info_fields.push(Box::new(pending)); | ||
| } | ||
|
|
@@ -268,9 +269,12 @@ impl InfoBuilder { | |
| Ok(self) | ||
| } | ||
|
|
||
| fn head(mut self, repo: &Repository) -> Result<Self> { | ||
| fn head(mut self, repo: &Repository, jujutsu_head: Option<gix::ObjectId>) -> Result<Self> { | ||
| if !self.disabled_fields.contains(&InfoType::Head) { | ||
| let head = HeadInfo::new(repo)?; | ||
| let head = match jujutsu_head { | ||
| Some(head_id) => HeadInfo::from_id(repo, head_id)?, | ||
| None => HeadInfo::new(repo)?, | ||
| }; | ||
| self.info_fields.push(Box::new(head)); | ||
| } | ||
| Ok(self) | ||
|
|
@@ -284,8 +288,13 @@ impl InfoBuilder { | |
| Ok(self) | ||
| } | ||
|
|
||
| fn size(mut self, repo: &Repository, number_separator: NumberSeparator) -> Self { | ||
| if !self.disabled_fields.contains(&InfoType::Size) { | ||
| fn size( | ||
| mut self, | ||
| repo: &Repository, | ||
| number_separator: NumberSeparator, | ||
| is_jujutsu: bool, | ||
| ) -> Self { | ||
| if !is_jujutsu && !self.disabled_fields.contains(&InfoType::Size) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 I'm a bit concerned that some Jujutsu users might be confused if they don't get the size field. I wonder if it would be enough to just simply
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I added a stderr notice in 8751184 stating that Jujutsu support is experimental and that pending changes and size are not yet supported. Keeping it on stderr leaves normal and serialized stdout intact. |
||
| let size = SizeInfo::new(repo, number_separator); | ||
| self.info_fields.push(Box::new(size)); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| use anyhow::{Context, Result, bail}; | ||
| use gix::ObjectId; | ||
| use std::path::{Path, PathBuf}; | ||
| use std::process::Command; | ||
|
|
||
| pub struct Repository { | ||
| git: gix::Repository, | ||
| work_dir: PathBuf, | ||
| jujutsu_head: Option<ObjectId>, | ||
| } | ||
|
|
||
| impl Repository { | ||
| pub fn discover(input: &Path) -> Result<Self> { | ||
| if let Some(root) = find_jujutsu_root(input)? | ||
| && !root.join(".git").exists() | ||
| { | ||
| return Self::open_jujutsu(root); | ||
| } | ||
|
|
||
| let git = gix::discover(input)?; | ||
| let work_dir = git | ||
| .workdir() | ||
| .context("please run onefetch inside of a non-bare git repository")? | ||
| .to_owned(); | ||
|
|
||
| Ok(Self { | ||
| git, | ||
| work_dir, | ||
| jujutsu_head: None, | ||
| }) | ||
| } | ||
|
|
||
| fn open_jujutsu(root: PathBuf) -> Result<Self> { | ||
| let git_dir = run_jj(&root, &["git", "root"]) | ||
| .context("Failed to locate the Git store backing the Jujutsu repository")?; | ||
| let git = gix::open(git_dir.trim()).context("Failed to open the Jujutsu Git store")?; | ||
|
|
||
| let head = run_jj(&root, &["log", "-r", "@", "--no-graph", "-T", "commit_id"]) | ||
| .context("Failed to determine the Jujutsu working-copy commit")?; | ||
| let head_id = ObjectId::from_hex(head.trim().as_bytes()) | ||
| .context("Jujutsu returned an invalid working-copy commit ID")?; | ||
|
|
||
| Ok(Self { | ||
| git, | ||
| work_dir: root, | ||
| jujutsu_head: Some(head_id), | ||
| }) | ||
| } | ||
|
|
||
| pub fn git(&self) -> &gix::Repository { | ||
| &self.git | ||
| } | ||
|
|
||
| pub fn work_dir(&self) -> &Path { | ||
| &self.work_dir | ||
| } | ||
|
|
||
| pub fn head_id(&self) -> Result<ObjectId> { | ||
| match self.jujutsu_head { | ||
| Some(head_id) => Ok(head_id), | ||
| None => Ok(self | ||
| .git | ||
| .head_id() | ||
| .context("Failed to retrieve HEAD ID")? | ||
| .detach()), | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stylistic nitpick: instead of using
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. I simplified this with Option::map_or_else in 8751184. |
||
| } | ||
|
|
||
| pub fn jujutsu_head(&self) -> Option<ObjectId> { | ||
| self.jujutsu_head | ||
| } | ||
|
|
||
| pub fn is_jujutsu(&self) -> bool { | ||
| self.jujutsu_head.is_some() | ||
| } | ||
| } | ||
|
|
||
| fn find_jujutsu_root(input: &Path) -> Result<Option<PathBuf>> { | ||
| let input = input | ||
| .canonicalize() | ||
| .with_context(|| format!("Failed to resolve repository path '{}'.", input.display()))?; | ||
| let start = if input.is_dir() { | ||
| input.as_path() | ||
| } else { | ||
| input | ||
| .parent() | ||
| .context("The repository path has no parent directory")? | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we allow inputs to be file paths? Should we just always fail here, instead, as the user has failed to provide a path to a directory?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. INPUT is documented as a directory path, so accepting files only through Jujutsu discovery would be inconsistent. find_jujutsu_root now rejects file inputs with a clear error, and the behavior is covered by the unit test in 8751184. |
||
| }; | ||
|
|
||
| Ok(start | ||
| .ancestors() | ||
| .find(|path| path.join(".jj").is_dir()) | ||
| .map(Path::to_owned)) | ||
| } | ||
|
|
||
| fn run_jj(root: &Path, args: &[&str]) -> Result<String> { | ||
| let output = Command::new("jj") | ||
| .args([ | ||
| "--ignore-working-copy", | ||
| "--no-pager", | ||
| "--color", | ||
| "never", | ||
| "-R", | ||
| ]) | ||
| .arg(root) | ||
| .args(args) | ||
| .output() | ||
| .context("Failed to execute `jj`; is Jujutsu installed?")?; | ||
|
|
||
| if !output.status.success() { | ||
| let stderr = String::from_utf8_lossy(&output.stderr); | ||
| bail!("`jj` exited with {}: {}", output.status, stderr.trim()); | ||
| } | ||
|
|
||
| String::from_utf8(output.stdout).context("Jujutsu returned non-UTF-8 output") | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn finds_jujutsu_root_from_nested_directory() -> Result<()> { | ||
| let fixture = | ||
| std::env::temp_dir().join(format!("onefetch-jj-root-test-{}", std::process::id())); | ||
| let nested = fixture.join("a/b"); | ||
| std::fs::create_dir_all(fixture.join(".jj"))?; | ||
| std::fs::create_dir_all(&nested)?; | ||
|
|
||
| assert_eq!(find_jujutsu_root(&nested)?, Some(fixture.canonicalize()?)); | ||
|
|
||
| std::fs::remove_dir_all(fixture)?; | ||
| Ok(()) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's pretty obvious, but, just to be clear: we're going to skip the pending changes info for Jujutsu, and this is because Jujutsu's concept of "pending changes" is different from Git's, right? IIRC Jujutsu always tracks changes?
If so, maybe add a comment explaining why we're skipping the pending field for Jujutsu.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Jujutsu snapshots working-copy changes into the @ commit, while the existing pending field reads Git index/worktree status. A native Jujutsu repository has a bare backing Git store, so that calculation is not meaningful. I added a comment explaining why the field is skipped in 8751184.