Support non-colocated Jujutsu repositories - #1847
Conversation
spenserblack
left a comment
spenserblack
left a comment
There was a problem hiding this comment.
This looks very good, thanks! Just have some nitpicks and comments.
| 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) { |
There was a problem hiding this comment.
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.
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.
| number_separator: NumberSeparator, | ||
| is_jujutsu: bool, | ||
| ) -> Self { | ||
| if !is_jujutsu && !self.disabled_fields.contains(&InfoType::Size) { |
There was a problem hiding this comment.
🤔 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 eprintln!("Jujutsu support is experimental: some fields are not yet supported") or something like that.
There was a problem hiding this comment.
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.
| match self.jujutsu_head { | ||
| Some(head_id) => Ok(head_id), | ||
| None => Ok(self | ||
| .git | ||
| .head_id() | ||
| .context("Failed to retrieve HEAD ID")? | ||
| .detach()), | ||
| } |
There was a problem hiding this comment.
Stylistic nitpick: instead of using match to convert an Option to a Result, I think we can use some of the Option methods like ok_or_else.
There was a problem hiding this comment.
Agreed. I simplified this with Option::map_or_else in 8751184.
| input | ||
| .parent() | ||
| .context("The repository path has no parent directory")? |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
2 participants
Closes #1846.
Native Jujutsu repositories keep their Git store inside
.jj, so normal Git discovery cannot find them.This change:
jj git rootto locate the backing Git store;The transient JJ working-copy commit is displayed as HEAD but excluded from commit statistics. Its ancestors, including multiple merge parents, are still traversed.
PendingandSizeare omitted because the hidden backing repository has no Git worktree or index.Testing:
cargo test -p onefetch— 133 passedjj git clone --no-colocate