Colocated workspaces using git worktrees #4588

cormacrelf · 2024-10-05T13:04:57Z

Fixes #4436

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/cli_util.rs

cli/src/commands/workspace/add.rs

cli/src/git_util.rs

cli/src/commands/workspace/add.rs

lib/src/simple_op_store.rs

yuja · 2024-10-06T12:06:17Z

lib/src/revset.rs

@@ -1630,6 +1657,7 @@ impl<'a> DefaultSymbolResolver<'a> {
                .iter()
                .flat_map(|ext| ext.as_ref().new_resolvers(repo))
                .collect(),
+            workspace_ctx,


HEAD@git would have to be translated to CommitRef::GitHead(WorkspaceId) or something at lower_expression() (where we know the current workspace.)

BTW, it might be okay to deprecate HEAD@git symbol since it can no longer be abstracted as a remote ref of the local @git remote. It serves as an indicator of colocated repo, but I've never used the HEAD@git symbol, which should usually point to @-.

I had to add the workspace id to the resolver anyway, because the resolver needs to be able to generate HEAD@git as a did-you-mean suggestion, and must do that only when the current workspace is colocated. Since it had to be added to the resolver anyway, it was simple to resolve GitHead without changing CommitRef.

Hmm, good point. I lean towards deprecating HEAD@git then.

As I said, it's no longer a valid abstraction, and the HEAD revision can be queried by git_head(). IIRC, HEAD@git resolution was added just because git_head template is rendered as such, not because we have a real use case.

martinvonz · 2024-10-06T18:02:05Z

workspace: Refuse to implicitly delete the default workspace
This tripped me up personally. Didn't know what jj workspace forget
did, ran it, and had to jj clone again.

You should be able to undo that operation. Maybe it's enough that we print a hint about that? Or was there some other consequence I'm missing?

…rktrees This also tests that new workspaces area colocated if the repo is. Creating the worktree will not be enough to satisfy this test -- we need JJ to advance commits and use the worktrees.

PhilipMetzger

Here's another batch of mostly minor nits.

cli/src/commands/workspace/add.rs

PhilipMetzger · 2024-10-12T11:37:42Z

cli/src/commands/workspace/add.rs

+    /// This flag works even if a repository was not
+    /// originally initialised with `--colocate`.


Nit: Remove this note and write it positively in the second paragraph.

Updated, following a bit of the wording from jj git init --help.

lib/src/protos/op_store.proto

cli/src/commands/workspace/forget.rs

lib/src/protos/op_store.proto

cli/src/cli_util.rs

PhilipMetzger · 2024-10-12T12:10:19Z

cli/src/cli_util.rs

As a TODO until the PR is ready but this should have a good motivation for the change. See https://github.com/martinvonz/jj/blob/main/docs/contributing.md#code-reviews particularly this section

We are not particularly strict about the style, but please do explain the reason for the change unless it's obvious.

cormacrelf · 2024-10-12T14:15:52Z

The main substantive question I have left is, when we import and export from git, should this always do it for every single worktree? Or just the current one?

The approach I have taken so far is to do it for the current one. But I think this is spreading out the import over too many operations when the worktrees are, in fact, all right there, readable, and iterable.

yuja · 2024-10-13T02:25:47Z

Ah, it would be quite complicated to import all the HEADs at once. If a git HEAD has changed in .git, then WorkspaceCommandHelper::import_git_head will check it out as the working copy, basically jj new it and write the tree to disk. To actually do that in all workspaces, we would need all other workspaces to be accessible at known paths all the time from any other workspace. And we do not currently store, in .jj/repo, where the workspaces' working copies are located, in order to do such a thing. And storing that data would imply we need a jj workspace repair to fix up the mapping of WorkspaceId -> path when stuff has moved on disk, which is perhaps the most annoying thing about git worktrees.

And workspaces might not always be accessible. They might be on USB stick or some network drive. So no, not all worktrees, but just the current one.

Previously, only the default workspace could be colocated. This adds logic to the colocation-detection code to support finding a worktree as well as a full-on .git directory. Secondly, in a couple of places, this discovered worktree is opened instead of always opening the backing repo. This is so that when you import HEAD, you import it from the current worktree. This work can't be finished until we can store multiple git HEADs in the view, but is continued in the following few diffs.

…t worktree Now that workspaces exist, it is necessary to distinguish the two kinds of git repository `jj` might open. There's the git backend (which is either a bare repository or a colocated one) and the worktrees for all the colocated workspaces. Both of these are a `gix/git2::Repository`, the difference is just which path you opened. So you have to distinguish them with names. The worktree git repo is now mostly used instead of always opening the backend repo. This is so that when you import HEAD, you import it from the current worktree.

This forms the basis of `jj workspace add --colocate`. The libraries don't quite have the functionality we need - gix doesn't do it at all - git2 does (Repository::worktree(name, opts)), but it always has to check out a commit. This implementation - can create in an already-existing directory; - writes a dummy HEAD and index; and - does not check out any files. JJ handles the rest.

E.g. you create a workspace, but there is already a git worktree of the same name. JJ should give you a hint as to what to do.

New flag: jj workspace add --colocate. We simply add a worktree.

Previously, JJ's view of the git head (HEAD@git) could only track one HEAD, for only one workspace, specifically the one colocated with the main .jj/repo. Since we now allow workspaces to be colocated, we need to track a separate git HEAD per workspace. This allows us to import HEAD for only the current workspace, and export HEAD to the current workspace when @- moves. Thereby allowing git tools to behave correctly in those workspaces. This affects a lot of stuff. And a lot of tests. A lot more things need to know the current workspace ID, including e.g. the revset resolvers, and the commit templater.

This comes with a couple of tweaks to make `cargo insta review`-ing rehashes easier in future.

…spaces Pretty straightforward. Previously there was only `git_head()`, which a previous diff updated to use the HEAD for the current workspace.

Fails on the second snapshot assertion. The problem with not removing HEADs is that they'll just hang around forever if you don't cull them.

This fixes the test in the previous diff. But we do not remove when merging views, yet. I have another series of patches to do what the TODO describes, but it fails too many tests in pretty perplexing ways, so that's not in this PR.

We need this for when we forget colocated workspaces, so that users do not have to `git worktree remove` manually. The validation / removal separation is so we can two-phase-commit the removals during a multi-workspace forget operation, checking they're all valid and able to be removed during the transaction and removing the worktrees afterwards. Missing worktrees (like when the user has ALREADY run `git worktree remove`) are ignored.

The CLI analogue of the previous diff. We need this for when we forget colocated workspaces, so that users do not have to `git worktree remove` manually.

`jj workspace rename` exists now. This means that people currently using JJ with colocated repos and renaming their default workspace may lose their git HEAD briefly when they next upgrade. This guesswork limits that to cases where they both: - rename their default workspace - AND create a second workspace (which cannot be --colocate as that is being added in the same PR as View.git_heads) But it's not a huge deal -- JJ will re-add the git HEAD when it next does almost anything.

It's long-winded, but at least now we can test deprecated fields in the View end to end.

cormacrelf · 2024-10-13T06:26:40Z

Is there anyone who can test on windows? I don't have a machine handy.

Edit: never mind, I made a failing test and got the CI to tell me how git does it.

Details

command=`"D:\\a\\jj\\jj\\target\\debug\\jj.exe" "workspace" "add" "--colocate" "../second"`
code=1
stdout=""
stderr=```
Created workspace in \"..\\second\"
Error: Git operation failed
Caused by: could not find repository at \'\\\\?\\C:\\Users\\runneradmin\\AppData\\Local\\Temp\\jj-test-fy26a2\\second\'; class=Repository (6); code=NotFound (-3)

That path, de-backslashed, is

\\?\C:\Users\runneradmin\AppData\Local\Temp\jj-test-fy26a2\second

which is a Verbatim path prefix.

I suspect Git likes its paths in the gitfile (gitdir: <path>\n) in a different format, but I don't know which one. It could even be Unix-style slashed format. If someone could run this on a windows machine that would help:

; git init repo
; cd repo
; touch file.txt && git add file.txt && git commit -m 'initial commit'
; git workspace add ../abc
; cat ../abc/.git
; cat .git/worktrees/abc/commondir
; cat .git/worktrees/abc/gitdir

We're getting paths like this, I think. gitdir: \\?\C:\Users\runneradmin\AppData\Local\Temp\jj-test-fy26a2\second But I don't know what git does, and I don't have access to a windows machine. Adding this so the CI windows build can tell us what's wrong.

Before gitdir: \\?\C:\Users\runneradmin\AppData\Local\Temp\jj-test-fy26a2\second After gitdir: C:/Users/runneradmin/AppData/Local/Temp/jj-test-fy26a2/second And same for the other worktree bits and pieces. We also change the way the tests run git commands, because of yet another error to do with Git being unable to handle verbatim paths: command=`"git" "worktree" "add" "\\\\?\\C:\\Users\\runneradmin\\AppData\\Local\\Temp\\jj-test-ti0MHE\\second"` code=128 stdout="" stderr=``` Preparing worktree (new branch \'second\') fatal: could not create leading directories of \'//?/C:/Users/runneradmin/AppData/Local/Temp/jj-test-ti0MHE/second/.git\': Invalid argument

yuja · 2024-10-13T10:03:15Z

Could you somehow split this PR into two or three units? This looks too big to run multiple review rounds, and GitHub UI doesn't support per-patch acceptance.

One idea (not sure if this is the easiest/simplest option):

add minimal command to set up colocated workspace
migrate view
add jj workspace command integration

cormacrelf · 2024-10-14T14:44:28Z

Closing to split in half, next is #4644

cormacrelf force-pushed the workspace-heads branch from aaebbbb to 44d144f Compare October 5, 2024 16:08

samueltardieu reviewed Oct 5, 2024

View reviewed changes

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

cormacrelf force-pushed the workspace-heads branch from 44d144f to 4919138 Compare October 5, 2024 16:22

PhilipMetzger reviewed Oct 5, 2024

View reviewed changes

cli/src/commands/workspace/add.rs Outdated Show resolved Hide resolved

cormacrelf force-pushed the workspace-heads branch 2 times, most recently from 963b134 to 72283b8 Compare October 6, 2024 04:53

yuja reviewed Oct 6, 2024

View reviewed changes

cormacrelf force-pushed the workspace-heads branch 2 times, most recently from 0a149ed to cd66e02 Compare October 12, 2024 10:52

workspace: Add test that colocated workspaces have independent git wo…

402abe7

…rktrees This also tests that new workspaces area colocated if the repo is. Creating the worktree will not be enough to satisfy this test -- we need JJ to advance commits and use the worktrees.

PhilipMetzger reviewed Oct 12, 2024

View reviewed changes

cormacrelf force-pushed the workspace-heads branch 3 times, most recently from 43ef037 to a2ca03c Compare October 12, 2024 14:14

cormacrelf added 2 commits October 13, 2024 15:47

cormacrelf force-pushed the workspace-heads branch from a2ca03c to 19d7bdf Compare October 13, 2024 04:47

cormacrelf added 2 commits October 13, 2024 16:18

workspace: Add tests for workspace/worktree naming conflicts

a42bb2a

E.g. you create a workspace, but there is already a git worktree of the same name. JJ should give you a hint as to what to do.

cormacrelf force-pushed the workspace-heads branch 3 times, most recently from 64c334e to c6ad91e Compare October 13, 2024 06:07

cormacrelf added 5 commits October 13, 2024 17:14

workspace: Implement colocated workspaces with git worktrees

4a74ae1

New flag: jj workspace add --colocate. We simply add a worktree.

view: new operation hashes for insta due to new View.git_heads

f0cb106

This comes with a couple of tweaks to make `cargo insta review`-ing rehashes easier in future.

revset: add git_heads() revset, to enumerate git heads for all work…

be639d3

…spaces Pretty straightforward. Previously there was only `git_head()`, which a previous diff updated to use the HEAD for the current workspace.

workspace: add failing test for workspace forget not removing HEAD

0bad919

Fails on the second snapshot assertion. The problem with not removing HEADs is that they'll just hang around forever if you don't cull them.

cormacrelf added 5 commits October 13, 2024 17:14

git: Remove git worktrees when forgetting workspaces

80fd3e7

The CLI analogue of the previous diff. We need this for when we forget colocated workspaces, so that users do not have to `git worktree remove` manually.

view: test for deprecated protobuf fields using the CLI

3e07532

It's long-winded, but at least now we can test deprecated fields in the View end to end.

cormacrelf force-pushed the workspace-heads branch 4 times, most recently from aadc9e1 to 3db4851 Compare October 13, 2024 08:32

cormacrelf added 2 commits October 13, 2024 19:45

cormacrelf force-pushed the workspace-heads branch from 3db4851 to 6047297 Compare October 13, 2024 08:54

cormacrelf marked this pull request as ready for review October 13, 2024 09:05

workspace: update changelog and docs for colocated workspaces

1d00db3

cormacrelf changed the title ~~Collocated workspaces using git worktrees~~ Colocated workspaces using git worktrees Oct 13, 2024

martinvonz mentioned this pull request Oct 13, 2024

revset: drop support for HEAD@git symbol resolution #4632

Merged

4 tasks

cormacrelf mentioned this pull request Oct 14, 2024

Colocated workspaces, minimal #4644

Open

4 tasks

cormacrelf closed this Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Colocated workspaces using git worktrees #4588

Colocated workspaces using git worktrees #4588

cormacrelf commented Oct 5, 2024 •

edited

Loading

yuja Oct 6, 2024 •

edited

Loading

yuja Oct 6, 2024

cormacrelf Oct 13, 2024

yuja Oct 13, 2024

yuja Oct 13, 2024

martinvonz commented Oct 6, 2024

PhilipMetzger left a comment

PhilipMetzger Oct 12, 2024

cormacrelf Oct 13, 2024

PhilipMetzger Oct 12, 2024

cormacrelf commented Oct 12, 2024

yuja commented Oct 13, 2024

cormacrelf commented Oct 13, 2024 •

edited

Loading

yuja commented Oct 13, 2024

cormacrelf commented Oct 14, 2024

		/// This flag works even if a repository was not
		/// originally initialised with `--colocate`.

Colocated workspaces using git worktrees #4588

Colocated workspaces using git worktrees #4588

Conversation

cormacrelf commented Oct 5, 2024 • edited Loading

Checklist

yuja Oct 6, 2024 • edited Loading

Choose a reason for hiding this comment

yuja Oct 6, 2024

Choose a reason for hiding this comment

cormacrelf Oct 13, 2024

Choose a reason for hiding this comment

yuja Oct 13, 2024

Choose a reason for hiding this comment

yuja Oct 13, 2024

Choose a reason for hiding this comment

martinvonz commented Oct 6, 2024

PhilipMetzger left a comment

Choose a reason for hiding this comment

PhilipMetzger Oct 12, 2024

Choose a reason for hiding this comment

cormacrelf Oct 13, 2024

Choose a reason for hiding this comment

PhilipMetzger Oct 12, 2024

Choose a reason for hiding this comment

cormacrelf commented Oct 12, 2024

yuja commented Oct 13, 2024

cormacrelf commented Oct 13, 2024 • edited Loading

yuja commented Oct 13, 2024

cormacrelf commented Oct 14, 2024

cormacrelf commented Oct 5, 2024 •

edited

Loading

yuja Oct 6, 2024 •

edited

Loading

cormacrelf commented Oct 13, 2024 •

edited

Loading