Skip to content
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.

Sign up for GitHub

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

Colocated workspaces, minimal #4644

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+1,033 −67
Open

Colocated workspaces, minimal #4644

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
92bbf56
workspace: Make backend factories aware of the workspace root
cormacrelf Nov 8, 2024
7e34182
workspace: Reload the repo from disk in `workspace add`
cormacrelf Nov 10, 2024
0d72f0b
git: box all `gix::open::Error` as clippy keeps warning about it
cormacrelf Nov 10, 2024
6c62985
git: Add MaybeColocatedGitRepo helper
cormacrelf Nov 8, 2024
899ac30
git: make GitBackend colocation-aware
cormacrelf Sep 11, 2024
64adf64
cli: warn when workspace/.git exists, but is not colocated
cormacrelf Sep 11, 2024
3e4b0c1
workspace: add stopgap test helper to create a colocated workspace
cormacrelf Oct 18, 2024
2923211
workspace: Add worktree support to MaybeColocatedGitRepo
cormacrelf Nov 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 3 additions & 13 deletions cli/src/git_util.rs
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this warning could be very noisy, so we should probably move it behind the future config flag.

Copy link
Author

Choose a reason for hiding this comment

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

I had the same thought. Want me to add a flag in this PR? git.suppress-colocation-warning or something?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it can be verbose, I would remove the warning at all. Maybe the warning can be inserted to initialization/debug commands?

Copy link
Contributor

Choose a reason for hiding this comment

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

Want me to add a flag in this PR? git.suppress-colocation-warning or something?

No, I think landing it as is or with Yuya's suggestion is fine, but it should definitely move behind the auto-colocate workspace flag in the follow-up

Copy link
Author

Choose a reason for hiding this comment

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

  1. We currently hard error if you try to jj git init in an existing Git repo. To accomplish that you have to manually move files around. git init in an existing JJ repo is what this is most likely to warn about.
  2. I think we should have a config flag, and I think the flag should also control whether jj reads/writes HEAD and the index after mutating jj/git operations. If you want jj to stop touching the HEAD and the index for a while, because you urgently need to push to production using git and some script using jj keeps messing with everything (e.g. your prompt uses jj but doesn't ignore wc), then I think you should be able to do that by temporarily disabling the flag.
  3. I would call it git.sync-with-colocated or similar. I think that flag should be separate from git.auto-colocate because of the use case I just described.
  4. It's also very simple to implement, all the HEAD mutation stuff is already contingent on colocation being discovered, so just don't discover.
    • I will move MaybeColocatedGitRepo to git_backend.rs because that's indeed where it will need to end up.
  5. However I would probably want to disable these warnings specifically for .git directories / symlinks for now, just in case this PR makes it to 0.24 and we're still debating the name. Worktree warnings can stay because nobody is doing that yet, and I have written so many tests for it anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we would want a flag to disable import/export in colocated workspace, but that's a separate issue, so let's not discuss here.

Regarding the warning, suppose we have some concerns, I would remove it at least from this PR. I personally don't think we'll need these warnings, and there are no bug report about that iirc.

Original file line number Diff line number Diff line change
Expand Up @@ -73,22 +73,12 @@ pub fn get_git_repo(store: &Store) -> Result<git2::Repository, CommandError> {
}
}

pub fn is_colocated_git_workspace(workspace: &Workspace, repo: &ReadonlyRepo) -> bool {
pub fn is_colocated_git_workspace(_workspace: &Workspace, repo: &ReadonlyRepo) -> bool {
let Some(git_backend) = repo.store().backend_impl().downcast_ref::<GitBackend>() else {
return false;
};
let Some(git_workdir) = git_backend.git_workdir() else {
return false; // Bare repository
};
if git_workdir == workspace.workspace_root() {
return true;
}
// Colocated workspace should have ".git" directory, file, or symlink. Compare
// its parent as the git_workdir might be resolved from the real ".git" path.
let Ok(dot_git_path) = workspace.workspace_root().join(".git").canonicalize() else {
return false;
};
git_workdir.canonicalize().ok().as_deref() == dot_git_path.parent()

git_backend.is_colocated()
}

fn terminal_get_username(ui: &Ui, url: &str) -> Option<String> {
Expand Down
65 changes: 49 additions & 16 deletions lib/src/git_backend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ use crate::backend::TreeId;
use crate::backend::TreeValue;
use crate::file_util::IoResultExt as _;
use crate::file_util::PathError;
use crate::git::MaybeColocatedGitRepo;
use crate::index::Index;
use crate::lock::FileLock;
use crate::merge::Merge;
Expand Down Expand Up @@ -159,14 +160,19 @@ pub struct GitBackend {
empty_tree_id: TreeId,
extra_metadata_store: TableStore,
cached_extra_metadata: Mutex<Option<Arc<ReadonlyTable>>>,
colocated: bool,
}

impl GitBackend {
pub fn name() -> &'static str {
"git"
}

fn new(base_repo: gix::ThreadSafeRepository, extra_metadata_store: TableStore) -> Self {
fn new(base_repo: MaybeColocatedGitRepo, extra_metadata_store: TableStore) -> Self {
let MaybeColocatedGitRepo {
git_repo: base_repo,
colocated,
} = base_repo;
let repo = Mutex::new(base_repo.to_thread_local());
let root_commit_id = CommitId::from_bytes(&[0; HASH_LENGTH]);
let root_change_id = ChangeId::from_bytes(&[0; CHANGE_ID_LENGTH]);
Expand All @@ -179,6 +185,7 @@ impl GitBackend {
empty_tree_id,
extra_metadata_store,
cached_extra_metadata: Mutex::new(None),
colocated,
}
}

Expand All @@ -194,7 +201,14 @@ impl GitBackend {
gix_open_opts_from_settings(settings),
)
.map_err(GitBackendInitError::InitRepository)?;
Self::init_with_repo(store_path, git_repo_path, git_repo)
Self::init_with_repo(
store_path,
git_repo_path,
MaybeColocatedGitRepo {
git_repo,
colocated: false,
},
)
}

/// Initializes backend by creating a new Git repo at the specified
Expand All @@ -218,34 +232,42 @@ impl GitBackend {
)
.map_err(GitBackendInitError::InitRepository)?;
let git_repo_path = workspace_root.join(".git");
Self::init_with_repo(store_path, &git_repo_path, git_repo)
Self::init_with_repo(
store_path,
&git_repo_path,
MaybeColocatedGitRepo {
git_repo,
colocated: true,
},
)
}

/// Initializes backend with an existing Git repo at the specified path.
pub fn init_external(
settings: &UserSettings,
store_path: &Path,
git_repo_path: &Path,
workspace_root: Option<&Path>,
) -> Result<Self, Box<GitBackendInitError>> {
let canonical_git_repo_path = {
let path = store_path.join(git_repo_path);
canonicalize_git_repo_path(&path)
.context(&path)
.map_err(GitBackendInitError::Path)?
};
let git_repo = gix::ThreadSafeRepository::open_opts(
canonical_git_repo_path,
let git_repo = MaybeColocatedGitRepo::open_automatic(
&canonical_git_repo_path,
workspace_root,
gix_open_opts_from_settings(settings),
)
.map_err(Box::new)
.map_err(GitBackendInitError::OpenRepository)?;
Self::init_with_repo(store_path, git_repo_path, git_repo)
}

fn init_with_repo(
store_path: &Path,
git_repo_path: &Path,
git_repo: gix::ThreadSafeRepository,
git_repo: MaybeColocatedGitRepo,
) -> Result<Self, Box<GitBackendInitError>> {
let extra_path = store_path.join("extra");
fs::create_dir(&extra_path)
Expand Down Expand Up @@ -278,7 +300,7 @@ impl GitBackend {
pub fn load(
settings: &UserSettings,
store_path: &Path,
_workspace_root: Option<&Path>,
workspace_root: Option<&Path>,
) -> Result<Self, Box<GitBackendLoadError>> {
let git_repo_path = {
let target_path = store_path.join("git_target");
Expand All @@ -290,11 +312,11 @@ impl GitBackend {
.context(&git_repo_path)
.map_err(GitBackendLoadError::Path)?
};
let repo = gix::ThreadSafeRepository::open_opts(
git_repo_path,
let repo = MaybeColocatedGitRepo::open_automatic(
&git_repo_path,
workspace_root,
gix_open_opts_from_settings(settings),
)
.map_err(Box::new)
.map_err(GitBackendLoadError::OpenRepository)?;
let extra_metadata_store = TableStore::load(store_path.join("extra"), HASH_LENGTH);
Ok(GitBackend::new(repo, extra_metadata_store))
Expand Down Expand Up @@ -324,6 +346,12 @@ impl GitBackend {
self.base_repo.work_dir()
}

/// Whether the git repo is colocated and we can therefore import/export
/// HEAD
pub fn is_colocated(&self) -> bool {
self.colocated
}

fn cached_extra_metadata_table(&self) -> BackendResult<Arc<ReadonlyTable>> {
let mut locked_head = self.cached_extra_metadata.lock().unwrap();
match locked_head.as_ref() {
Expand Down Expand Up @@ -1608,7 +1636,8 @@ mod tests {
.unwrap();
let commit_id2 = CommitId::from_bytes(git_commit_id2.as_bytes());

let backend = GitBackend::init_external(&settings, store_path, git_repo.path()).unwrap();
let backend =
GitBackend::init_external(&settings, store_path, git_repo.path(), None).unwrap();

// Import the head commit and its ancestors
backend
Expand Down Expand Up @@ -1730,7 +1759,8 @@ mod tests {
)
.unwrap();

let backend = GitBackend::init_external(&settings, store_path, git_repo.path()).unwrap();
let backend =
GitBackend::init_external(&settings, store_path, git_repo.path(), None).unwrap();

// read_commit() without import_head_commits() works as of now. This might be
// changed later.
Expand Down Expand Up @@ -1779,7 +1809,8 @@ mod tests {
.commit_signed(commit_buf, secure_sig, None)
.unwrap();

let backend = GitBackend::init_external(&settings, store_path, git_repo.path()).unwrap();
let backend =
GitBackend::init_external(&settings, store_path, git_repo.path(), None).unwrap();

let commit = backend
.read_commit(&CommitId::from_bytes(git_commit_id.as_bytes()))
Expand Down Expand Up @@ -1848,7 +1879,8 @@ mod tests {
let git_repo_path = temp_dir.path().join("git");
let git_repo = git2::Repository::init(git_repo_path).unwrap();

let backend = GitBackend::init_external(&settings, store_path, git_repo.path()).unwrap();
let backend =
GitBackend::init_external(&settings, store_path, git_repo.path(), None).unwrap();
let mut commit = Commit {
parents: vec![],
predecessors: vec![],
Expand Down Expand Up @@ -1917,7 +1949,8 @@ mod tests {
let git_repo_path = temp_dir.path().join("git");
let git_repo = git2::Repository::init(git_repo_path).unwrap();

let backend = GitBackend::init_external(&settings, store_path, git_repo.path()).unwrap();
let backend =
GitBackend::init_external(&settings, store_path, git_repo.path(), None).unwrap();
let create_tree = |i| {
let blob_id = git_repo.blob(b"content {i}").unwrap();
let mut tree_builder = git_repo.treebuilder(None).unwrap();
Expand Down
1 change: 1 addition & 0 deletions lib/src/workspace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ impl Workspace {
settings,
store_path,
&store_relative_git_repo_path,
Some(workspace_root),
)?;
Ok(Box::new(backend))
};
Expand Down
3 changes: 3 additions & 0 deletions lib/tests/test_git.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1334,6 +1334,7 @@ impl GitRepoData {
settings,
store_path,
git_repo.path(),
Some(&jj_repo_dir),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be None because we don't create a jj workspace here. (applies to the other two changes in this file.)

)?))
},
Signer::from_settings(&settings).unwrap(),
Expand Down Expand Up @@ -2242,6 +2243,7 @@ fn test_init() {
settings,
store_path,
git_repo.path(),
Some(&jj_repo_dir),
)?))
},
Signer::from_settings(&settings).unwrap(),
Expand Down Expand Up @@ -2601,6 +2603,7 @@ fn set_up_push_repos(settings: &UserSettings, temp_dir: &TempDir) -> PushTestSet
settings,
store_path,
clone_repo.path(),
Some(&jj_repo_dir),
)?))
},
Signer::from_settings(settings).unwrap(),
Expand Down