Colocated workspaces, minimal #4644

cormacrelf · 2024-10-14T14:27:59Z

Only the worktree recognition and HEAD reading/writing parts of #4588.

In other PRs:

The CLI changes: create/destroy git worktrees during jj workspace add/forget
The view changes to track multiple git heads and make the git_head() revset work correctly

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
I have updated the config schema (cli/src/config-schema.json)
I have added tests to cover my changes

cli/src/commands/git/init.rs

yuja

Reviewed the first half. Thanks for splitting the PRs, but I think this is still big. I would extract patches after "git: Implement git_worktree_add" to new PR.

cli/src/commands/workspace/add.rs

cli/tests/test_git_colocated.rs

cli/src/git_util.rs

cli/src/cli_util.rs

lib/src/git.rs

PhilipMetzger

I left some minor stuff as Yuya already has begun thoroughly reviewing.

lib/src/git.rs

cormacrelf · 2024-10-19T10:42:27Z

cli/tests/test_git_init.rs

+    insta::assert_snapshot!(stderr, @r#"
+    Warning: This workspace has a .git directory that isn't managed by JJ.
+    "#);


This felt a bit iffy because if your workflow is currently to have a completely unrelated git repo in your jj workspace root, we are going to warn about it every time you run a command. I do feel that is unlikely though.

cli/tests/test_git_colocated.rs

martinvonz

Sorry that I'm so slow to review this PR. I haven't finished reviewing it but what I've seen looks good to me.

cli/tests/test_git_colocated.rs

cli/src/git_util.rs

lib/src/git.rs

cli/src/commands/git/init.rs

lib/src/workspace.rs

lib/src/git.rs

yuja · 2024-11-11T13:37:28Z

lib/src/git.rs

+        //
+        // So try to open the workspace root (/second) as a git repository.
+        let worktree_repo =
+            match gix::ThreadSafeRepository::open_opts(workspace_root.join(".git"), open_opts) {


Can we use the main repo to iterate worktrees?
https://docs.rs/gix/latest/gix/struct.Repository.html#method.worktrees

Yes, but it comes with disadvantages. You would have to:

Either

Plumb WorkspaceId down through the store factories as well as the workspace root (we would need both, as we already need the workspace root to detect non-worktree colocated repos)

Decide on a naming convention for JJ-managed worktree identifiers, which we should be loath to change later as it will break people's repos

Canonicalize and compare with workspace root anyway, to detect if someone has messed with the worktrees or their names

Or

Read the worktree gitdir file for O(N) worktrees. Pretty sure there will be people out there with dozens, who make one per branch they ever work on and never clean up. Or people who keep worktrees on network drives.

Canonicalize and compare with workspace root anyway, as part of the filtering

The way it is now is as simple and fast as it gets, really, modulo the warnings. We already have the benefits of gix checking all the worktree stuff, so iterating worktrees would not be any more correct. As I noted in another thread, git does not iterate its worktrees in normal operation, probably for similar reasons.

If you really want me to do it, I will.

Canonicalize and compare with workspace root anyway, to detect if someone has messed with the worktrees or their names

I meant this. My feeling is that it's slightly better than instantiating an arbitrary repo at <workspace_root>/.git (which might be unrelated), but I'm not sure if that makes the code shorter.

I actually think that, with the strategy of just opening workspace root/.git, we can probably just delete most of the previous colocation checking code that I ported over to MaybeColocatedGitRepo. I just wanted to keep these changes clean and reviewable, so I didn't touch it. But I actually think if you delete everything above the gix open call, it will all still work.

Edit: yes! The tests all pass. If the tests written for the symlink behaviour etc are comprehensive, then this is much better.

lib/src/git.rs

All backends now have access to the workspace root if they so wish. This is not the cleanest thing ever, since the store should not need to know about the checked-out working copy on disk. However, it is very convenient if GitBackend knows it, so that it can be colocation-aware. This does the plumbing down to GitBackend::load, but does not actually use it yet. That will come later in these diffs.

This allows us to have a differently configured GitBackend in the newly- added workspace, i.e. one which is opened at the workspace root if we are creating a colocated workspace. This was easier than I anticipated -- we can use RepoLoader by itself to load the store from disk, and then create the new working copy commit immediately.

martinvonz · 2024-11-13T05:47:14Z

@martinvonz, are you okay with this change?
8b586b0

Yes, I don't have a better suggestion at least, and I don't want to block these PRs any more than I already have (sorry that it took me two days to even notice your question). Thank you both! I'm happy we're getting this feature soon.

We want to check for colocated workspaces in the GitBackend initialization code. So it has to move to jj_lib. GitBackend also needs this in a few places, so we add a helper struct that can open and check colocation in one shot. The original is_colocated_git_workspace() will be replaced with code that inspects the git_backend. So it is left unchanged for now.

GitBackend now knows if it's colocated or not. This lays the foundation for GitBackend opening the worktree version of a git repo. Later in these diffs, we add worktree support. Then, automatically, all HEADs will be written to those worktrees instead. We won't have to add special code to re-open the git repo at the worktree (= workspace root) in a dozen places in the CLI, and then maintain discipline in selecting which repo to use when.

We will be glad to have added this when people are using worktrees and breaking them, as worktrees are a bit more sensitive to things like moving the repo around on disk.

A colocated workspace is one that also happens to have a valid .git file or directory. Currently, only the default workspace can be colocated, but we also want secondary workspaces to have this ability. To create a *secondary* colocated workspace in an existing JJ repo, we need to add a corresponding git _worktree_. JJ doesn't have the ability to create git worktrees yet. We want to test a bunch of code around colocating worktrees with a workspace, assuming we will build the functionality to actually create one in JJ itself later. So here's a helper function to do it the really hacky way with the `git` CLI, moving a .git file into place, and running `git worktree repair`.

This adds logic to the colocation-detection code to support finding a worktree with a "gitfile" as well as a full-on .git directory. Done by just trusting gix to open the workspace root/.git. It handles symlinks like the old code did, and now also handles worktrees. This means GitBackend is now opened at the worktree repo, which in turn means that colocated workspaces are somewhat independent. You can move @ in a workspace and JJ will write HEAD = @- to the git worktree in that workspace. And you can run mutating git commands in a workspace, and JJ will import the new HEAD only in that workspace. There are some new tests for what happens when you `jj git init --git-repo=...` either in or pointing at an existing worktree. I do not expect these to be common workflows, but there is new behaviour here that we need to track. There are also FIXMEs in the tests for places where we need to store one HEAD per colocated workspace in the view, as well as having independent import/export. These view changes are unwieldy and will come later.

yuja

Thanks for these works.

yuja · 2024-11-14T03:48:47Z

lib/src/workspace.rs

@@ -75,6 +77,18 @@ pub enum WorkspaceInitError {
    Backend(#[from] BackendInitError),
    #[error(transparent)]
    SignInit(#[from] SignInitError),
+    #[error("Could not load newly created workspace: {0}")]


nit: remove : {0}. The source error will be printed separately

yuja · 2024-11-14T03:49:50Z

lib/src/git.rs

+///
+/// Also represents a newly created git repo in one of those configurations, if
+/// constructed manually.
+pub(crate) struct MaybeColocatedGitRepo {


nit: I would put this into git_backend module, but I don't have a strong opinion about that.

Good abstraction, btw.

yuja · 2024-11-14T03:50:54Z

lib/tests/test_git.rs

@@ -1334,6 +1334,7 @@ impl GitRepoData {
                    settings,
                    store_path,
                    git_repo.path(),
+                    Some(&jj_repo_dir),


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

yuja · 2024-11-14T03:53:40Z

cli/src/git_util.rs

+        return true;
+    }
+
+    if let Some(ui) = ui {


Better to add a separate function to print warning instead of passing Option<&Ui>?

yuja · 2024-11-14T03:54:07Z

cli/src/git_util.rs

+                 repo",
+                target.display()
+            )
+            .ok();


nit: no need to suppress io::Error

What would you do instead? If we can't write a warning to stderr, we aren't going to be able to print a panic message either. So I figured there was no point panicking. All other instances of writeln!(ui.xxx()) in the codebase either use ? or if not in a ->Result function .ok(), so I followed that.

What I meant was just make this function return Result<(), CommandError>. All callers can propagate the error.

yuja · 2024-11-14T04:02:20Z

cli/tests/test_git_colocated.rs

+        );
+        assert_eq!(
+            stderr,
+            "Reset the working copy parent to the new Git HEAD.\n",


nit: better to use insta::allow_duplicates!? It's tedious to update the expectation on wording changes.

yuja · 2024-11-14T04:03:59Z

cli/tests/test_git_colocated.rs

+        test_env: &TestEnvironment,
+        commit_in: &Path,
+        no_import_in_workspace: &Path,
+        workspace_at: &str,


nit: workspace_at sounds like a path. Rename to wc_commit or something?

yuja · 2024-11-14T04:04:27Z

cli/tests/test_git_init.rs

+    );
+
+    // This is similar to a normal `jj git init --git-repo=` -- we import the
+    // commits, but in this case our HEAD@git comes from the worktree.


nit: HEAD@git -> Git HEAD

yuja · 2024-11-14T04:04:52Z

cli/tests/test_git_init.rs

+const OP_LOG_COMPACT: &[&str] = &[
+    "op",
+    "log",
+    "-Tself.id().short() ++ ' ' ++ separate(\"\\n\", self.description().first_line(), self.tags())",


nit: r#".." to simplify escape

yuja · 2024-11-14T04:05:31Z

cli/tests/test_git_init.rs

+];
+
+/// This one is a bit weird, but technically you can do it. Should be roughly
+/// equivalent to the --git-repo=. case, but with a different git_target file.


Indeed. It might be eligible for warning, but we don't have to within this PR.

(No action needed)

PhilipMetzger

LG, left some last minor nits.

PhilipMetzger · 2024-11-14T16:40:36Z

lib/src/git.rs

+        };
+
+        // i.e. (/symlink_to_repo -> /repo).canonicalize() == (/repo/.git).parent()
+        if let Some(gbw) = git_backend_workdir_canonical.as_deref() {


Nit: s/gbw/git_backend_workdir as we don't use the initial binding

PhilipMetzger · 2024-11-14T16:44:00Z

cli/src/git_util.rs

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

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

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

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

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.

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.

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.

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.

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.

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.

PhilipMetzger · 2024-11-14T16:44:54Z

cli/src/git_util.rs

+            let repair_dir = git_repo.work_dir().unwrap_or(git_repo.common_dir());
+            writeln!(
+                ui.hint_default(),
+                "If this is meant to be a colocated JJ workspace, you may like to try `git -C {} \


Nit: s/JJ/jj/g, orient yourself on the previous uses in the project.

PhilipMetzger · 2024-11-14T16:47:25Z

cli/tests/test_git_colocated.rs

+    let tmp_path = test_env.env_root().join("__tmp_worktree__");
+    if tmp_path.exists() {
+        std::fs::remove_dir_all(&tmp_path).unwrap();
+    }


Nit: Either it is jj or Jujutsu (full name) in the commit message.

PhilipMetzger · 2024-11-14T16:52:02Z

cli/tests/test_git_colocated.rs

+fn test_colocated_workspace_wrong_gitdir() {
+    // TODO: Remove when this stops requiring git (stopgap_workspace_colocate)
+    if Command::new("git").arg("--version").status().is_err() {
+        eprintln!("Skipping because git command might fail to run");


Nit: I think "Skipping test because Git prerequisite is missing" may be a more appropriate message for CI

I am just copying what's there already, so someone can grep for all uses.

PhilipMetzger · 2024-11-14T16:52:48Z

cli/tests/test_git_colocated.rs

+fn test_colocated_workspace_invalid_gitdir() {
+    // TODO: Remove when this stops requiring git (stopgap_workspace_colocate)
+    if Command::new("git").arg("--version").status().is_err() {
+        eprintln!("Skipping because git command might fail to run");


cormacrelf force-pushed the workspace-colocate-minimal branch from 63c7b57 to de0b28c Compare October 14, 2024 14:42

cormacrelf mentioned this pull request Oct 14, 2024

Colocated workspaces using git worktrees #4588

Closed

4 tasks

cormacrelf marked this pull request as ready for review October 14, 2024 15:00

cormacrelf commented Oct 15, 2024

View reviewed changes

cli/src/commands/git/init.rs Outdated Show resolved Hide resolved

yuja reviewed Oct 15, 2024

View reviewed changes

PhilipMetzger reviewed Oct 15, 2024

View reviewed changes

lib/src/git.rs Outdated Show resolved Hide resolved

lib/src/git.rs Outdated Show resolved Hide resolved

cormacrelf force-pushed the workspace-colocate-minimal branch 3 times, most recently from 0234894 to 7b00abb Compare October 19, 2024 10:18

cormacrelf commented Oct 19, 2024

View reviewed changes

cormacrelf force-pushed the workspace-colocate-minimal branch from 7b00abb to 1f362de Compare October 19, 2024 12:27

martinvonz reviewed Oct 20, 2024

View reviewed changes

cli/tests/test_git_colocated.rs Show resolved Hide resolved

cormacrelf mentioned this pull request Oct 20, 2024

Workspace colocate 2 #4678

Draft

4 tasks

martinvonz reviewed Oct 20, 2024

View reviewed changes

cli/tests/test_git_colocated.rs Show resolved Hide resolved

cli/src/git_util.rs Outdated Show resolved Hide resolved

lib/src/git.rs Outdated Show resolved Hide resolved

cli/src/commands/git/init.rs Outdated Show resolved Hide resolved

cormacrelf mentioned this pull request Nov 8, 2024

POC for GitBackend opening worktrees #4801

Closed

4 tasks

cormacrelf force-pushed the workspace-colocate-minimal branch 6 times, most recently from 3d83cdb to fb7e8c2 Compare November 10, 2024 12:47

yuja reviewed Nov 11, 2024

View reviewed changes

cormacrelf added 3 commits November 13, 2024 01:37

git: box all gix::open::Error as clippy keeps warning about it

0d72f0b

cormacrelf force-pushed the workspace-colocate-minimal branch 5 times, most recently from 4f4a015 to 1194103 Compare November 12, 2024 16:01

cormacrelf force-pushed the workspace-colocate-minimal branch 6 times, most recently from 757e34c to d23d59c Compare November 13, 2024 10:18

cormacrelf added 5 commits November 13, 2024 21:21

cli: warn when workspace/.git exists, but is not colocated

64adf64

We will be glad to have added this when people are using worktrees and breaking them, as worktrees are a bit more sensitive to things like moving the repo around on disk.

cormacrelf force-pushed the workspace-colocate-minimal branch from d23d59c to 2923211 Compare November 13, 2024 10:21

yuja approved these changes Nov 14, 2024

View reviewed changes

PhilipMetzger approved these changes Nov 14, 2024

View reviewed changes

Colocated workspaces, minimal #4644

Are you sure you want to change the base?

Colocated workspaces, minimal #4644

Conversation

cormacrelf commented Oct 14, 2024 • edited Loading

Checklist

yuja left a comment

Choose a reason for hiding this comment

PhilipMetzger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinvonz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cormacrelf Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

martinvonz commented Nov 13, 2024

yuja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilipMetzger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cormacrelf commented Oct 14, 2024 •

edited

Loading

cormacrelf Nov 13, 2024 •

edited

Loading