From c56765ce262fb7a207ede5d6c9a2385ed5188349 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Fri, 17 Jun 2022 01:44:47 +0200 Subject: [PATCH 01/37] Match branches with worktrees always, even with slashes --- src/grm/main.rs | 18 +--------- src/tree.rs | 2 +- src/worktree.rs | 90 ++++++++++++++++++++++++++++++++++++++++++++----- 3 files changed, 83 insertions(+), 27 deletions(-) diff --git a/src/grm/main.rs b/src/grm/main.rs index 6c7ab86..5972102 100644 --- a/src/grm/main.rs +++ b/src/grm/main.rs @@ -502,25 +502,9 @@ fn main() { None => None, }; - let mut name: &str = &action_args.name; - let subdirectory; - let split = name.split_once('/'); - match split { - None => subdirectory = None, - Some(split) => { - if split.0.is_empty() || split.1.is_empty() { - print_error("Worktree name cannot start or end with a slash"); - process::exit(1); - } else { - (subdirectory, name) = (Some(Path::new(split.0)), split.1); - } - } - } - match worktree::add_worktree( &cwd, - name, - subdirectory, + &action_args.name, track, action_args.no_track, ) { diff --git a/src/tree.rs b/src/tree.rs index d765298..e8a0741 100644 --- a/src/tree.rs +++ b/src/tree.rs @@ -222,7 +222,7 @@ fn sync_repo(root_path: &Path, repo: &repo::Repo, init_worktree: bool) -> Result if newly_created && repo.worktree_setup && init_worktree { match repo_handle.default_branch() { Ok(branch) => { - worktree::add_worktree(&repo_path, &branch.name()?, None, None, false)?; + worktree::add_worktree(&repo_path, &branch.name()?, None, false)?; } Err(_error) => print_repo_error( &repo.name, diff --git a/src/worktree.rs b/src/worktree.rs index 35db51f..631aa79 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -21,10 +21,17 @@ pub const GIT_MAIN_WORKTREE_DIRECTORY: &str = ".git-main-working-tree"; pub fn add_worktree( directory: &Path, name: &str, - subdirectory: Option<&Path>, track: Option<(&str, &str)>, no_track: bool, ) -> Result<(), String> { + // A branch name must never start or end with a slash. Everything else is ok. + if name.starts_with('/') || name.ends_with('/') { + return Err(format!( + "Invalid worktree name: {}. It cannot start or end with a slash", + name + )); + } + let repo = repo::RepoHandle::open(directory, true).map_err(|error| match error.kind { repo::RepoErrorKind::NotFound => { String::from("Current directory does not contain a worktree setup") @@ -38,11 +45,6 @@ pub fn add_worktree( return Err(format!("Worktree {} already exists", &name)); } - let path = match subdirectory { - Some(dir) => directory.join(dir).join(name), - None => directory.join(Path::new(name)), - }; - let mut remote_branch_exists = false; let mut target_branch = match repo.find_local_branch(name) { @@ -193,10 +195,80 @@ pub fn add_worktree( } } - if let Some(subdirectory) = subdirectory { - std::fs::create_dir_all(subdirectory).map_err(|error| error.to_string())?; + // We have to create subdirectories first, otherwise adding the worktree + // will fail + if name.contains('/') { + let path = Path::new(&name); + if let Some(base) = path.parent() { + // This is a workaround of a bug in libgit2 (?) + // + // When *not* doing this, we will receive an error from the `Repository::worktree()` + // like this: + // + // > failed to make directory '/{repo}/.git-main-working-tree/worktrees/dir/test + // + // This is a discrepancy between the behaviour of libgit2 and the + // git CLI when creating worktrees with slashes: + // + // The git CLI will create the worktree's configuration directory + // inside {git_dir}/worktrees/{last_path_component}. Look at this: + // + // ``` + // $ git worktree add 1/2/3 -b 1/2/3 + // $ ls .git/worktrees + // 3 + // ``` + // + // Interesting: When adding a worktree with a different name but the + // same final path component, git starts adding a counter suffix to + // the worktree directories: + // + // ``` + // $ git worktree add 1/3/3 -b 1/3/3 + // $ git worktree add 1/4/3 -b 1/4/3 + // $ ls .git/worktrees + // 3 + // 31 + // 32 + // ``` + // + // I *guess* that the mapping back from the worktree directory under .git to the actual + // worktree directory is done via the `gitdir` file inside `.git/worktrees/{worktree}. + // This means that the actual directory would not matter. You can verify this by + // just renaming it: + // + // ``` + // $ mv .git/worktrees/3 .git/worktrees/foobar + // $ git worktree list + // /tmp/ fcc8a2a7 [master] + // /tmp/1/2/3 fcc8a2a7 [1/2/3] + // /tmp/1/3/3 fcc8a2a7 [1/3/3] + // /tmp/1/4/3 fcc8a2a7 [1/4/3] + // ``` + // + // => Still works + // + // Anyway, libgit2 does not do this: It tries to create the worktree + // directory inside .git with the exact name of the worktree, including + // any slashes. It should be this code: + // + // https://github.com/libgit2/libgit2/blob/f98dd5438f8d7bfd557b612fdf1605b1c3fb8eaf/src/libgit2/worktree.c#L346 + // + // As a workaround, we can create the base directory manually for now. + // + // Tracking upstream issue: https://github.com/libgit2/libgit2/issues/6327 + std::fs::create_dir_all( + directory + .join(GIT_MAIN_WORKTREE_DIRECTORY) + .join("worktrees") + .join(base), + ) + .map_err(|error| error.to_string())?; + std::fs::create_dir_all(base).map_err(|error| error.to_string())?; + } } - repo.new_worktree(name, &path, &target_branch)?; + + repo.new_worktree(name, &directory.join(&name), &target_branch)?; Ok(()) } From addff12c17b00967da040d761d6dcbf9f41ba409 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Fri, 17 Jun 2022 01:45:14 +0200 Subject: [PATCH 02/37] Run e2e tests again dynamically linked dev binary This makes the build much faster. --- Cargo.toml | 2 +- Justfile | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 86e463e..c744343 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,7 +28,7 @@ rust-version = "1.57" license = "GPL-3.0-only" [profile.e2e-tests] -inherits = "release" +inherits = "dev" [lib] name = "grm" diff --git a/Justfile b/Justfile index d5071da..d90aaa3 100644 --- a/Justfile +++ b/Justfile @@ -36,7 +36,7 @@ test-binary: env \ GITHUB_API_BASEURL=http://rest:5000/github \ GITLAB_API_BASEURL=http://rest:5000/gitlab \ - cargo build --target {{static_target}} --profile e2e-tests --features=static-build + cargo build --profile e2e-tests install: cargo install --path . @@ -64,7 +64,7 @@ test-e2e +tests=".": test-binary && docker-compose build \ && docker-compose run \ --rm \ - -v $PWD/../target/{{static_target}}/e2e-tests/grm:/grm \ + -v $PWD/../target/e2e-tests/grm:/grm \ pytest \ "GRM_BINARY=/grm ALTERNATE_DOMAIN=alternate-rest python3 -m pytest -p no:cacheprovider --color=yes "$@"" \ && docker-compose rm --stop -f From 23526ae62bc7465aa5bafd2a66ea3a51ede5fdce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Fri, 17 Jun 2022 01:47:16 +0200 Subject: [PATCH 03/37] e2e: Update tests for worktree subdirectory handling --- e2e_tests/test_worktrees.py | 55 ++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 25 deletions(-) diff --git a/e2e_tests/test_worktrees.py b/e2e_tests/test_worktrees.py index 82516bf..e288fa2 100644 --- a/e2e_tests/test_worktrees.py +++ b/e2e_tests/test_worktrees.py @@ -12,9 +12,18 @@ import os.path @pytest.mark.parametrize("has_config", [True, False]) @pytest.mark.parametrize("has_default", [True, False]) @pytest.mark.parametrize("has_prefix", [True, False]) -def test_worktree_add_simple( - remote_branch_already_exists, has_config, has_default, has_prefix +@pytest.mark.parametrize("worktree_with_slash", [True, False]) +def test_worktree_add( + remote_branch_already_exists, + has_config, + has_default, + has_prefix, + worktree_with_slash, ): + if worktree_with_slash: + worktree_name = "dir/test" + else: + worktree_name = "test" with TempGitRepositoryWorktree() as (base_dir, _commit): if has_config: with open(os.path.join(base_dir, "grm.toml"), "w") as f: @@ -42,54 +51,50 @@ def test_worktree_add_simple( touch change git add change git commit -m commit - git push origin HEAD:test + git push origin HEAD:{worktree_name} #git reset --hard 'HEAD@{1}' git branch -va ) git --git-dir ./.git-main-working-tree worktree remove tmp """ ) - cmd = grm(["wt", "add", "test"], cwd=base_dir) + cmd = grm(["wt", "add", worktree_name], cwd=base_dir) assert cmd.returncode == 0 files = os.listdir(base_dir) if has_config is True: assert len(files) == 3 - assert set(files) == {".git-main-working-tree", "grm.toml", "test"} + if worktree_with_slash: + assert set(files) == {".git-main-working-tree", "grm.toml", "dir"} + assert set(os.listdir(os.path.join(base_dir, "dir"))) == {"test"} + else: + assert set(files) == {".git-main-working-tree", "grm.toml", "test"} else: assert len(files) == 2 - assert set(files) == {".git-main-working-tree", "test"} + if worktree_with_slash: + assert set(files) == {".git-main-working-tree", "dir"} + assert set(os.listdir(os.path.join(base_dir, "dir"))) == {"test"} + else: + assert set(files) == {".git-main-working-tree", "test"} - repo = git.Repo(os.path.join(base_dir, "test")) + repo = git.Repo(os.path.join(base_dir, worktree_name)) assert not repo.bare assert not repo.is_dirty() if has_config and has_default: if has_prefix and not remote_branch_already_exists: assert ( - str(repo.active_branch.tracking_branch()) == "origin/myprefix/test" + str(repo.active_branch.tracking_branch()) + == f"origin/myprefix/{worktree_name}" ) else: - assert str(repo.active_branch.tracking_branch()) == "origin/test" + assert ( + str(repo.active_branch.tracking_branch()) + == f"origin/{worktree_name}" + ) else: assert repo.active_branch.tracking_branch() is None -def test_worktree_add_into_subdirectory(): - with TempGitRepositoryWorktree() as (base_dir, _commit): - cmd = grm(["wt", "add", "dir/test"], cwd=base_dir) - assert cmd.returncode == 0 - - files = os.listdir(base_dir) - assert len(files) == 2 - assert set(files) == {".git-main-working-tree", "dir"} - - files = os.listdir(os.path.join(base_dir, "dir")) - assert set(files) == {"test"} - - repo = git.Repo(os.path.join(base_dir, "dir", "test")) - assert not repo.bare - assert not repo.is_dirty() - assert repo.active_branch.tracking_branch() is None def test_worktree_add_into_invalid_subdirectory(): From 2a0a591194fbe0d76d8f4e6beed5ae9826537f87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Fri, 17 Jun 2022 01:47:47 +0200 Subject: [PATCH 04/37] e2e: Add test for invalid worktree names --- e2e_tests/test_worktrees.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/e2e_tests/test_worktrees.py b/e2e_tests/test_worktrees.py index e288fa2..d528aef 100644 --- a/e2e_tests/test_worktrees.py +++ b/e2e_tests/test_worktrees.py @@ -95,6 +95,17 @@ def test_worktree_add( assert repo.active_branch.tracking_branch() is None +def test_worktree_add_invalid_name(): + with TempGitRepositoryWorktree() as (base_dir, _commit): + for worktree_name in ["/absolute/path" "trailingslash/"]: + args = ["wt", "add", worktree_name] + cmd = grm(args, cwd=base_dir) + assert cmd.returncode != 0 + print(cmd.stdout) + print(cmd.stderr) + assert not os.path.exists(worktree_name) + assert not os.path.exists(os.path.join(base_dir, worktree_name)) + assert "invalid worktree name" in str(cmd.stderr.lower()) def test_worktree_add_into_invalid_subdirectory(): From a3f9c9fda1a5e501c614a91bd4cc27443ca5266c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Fri, 17 Jun 2022 01:48:36 +0200 Subject: [PATCH 05/37] e2e: Remove redundant test --- e2e_tests/test_worktrees.py | 61 ------------------------------------- 1 file changed, 61 deletions(-) diff --git a/e2e_tests/test_worktrees.py b/e2e_tests/test_worktrees.py index d528aef..d669a5e 100644 --- a/e2e_tests/test_worktrees.py +++ b/e2e_tests/test_worktrees.py @@ -228,67 +228,6 @@ def test_worktree_add_with_explicit_no_tracking( assert repo.active_branch.tracking_branch() is None -@pytest.mark.parametrize("remote_branch_already_exists", [True, False]) -@pytest.mark.parametrize("has_default", [True, False]) -@pytest.mark.parametrize("has_prefix", [True, False]) -def test_worktree_add_with_config( - remote_branch_already_exists, has_default, has_prefix -): - with TempGitRepositoryWorktree() as (base_dir, _commit): - with open(os.path.join(base_dir, "grm.toml"), "w") as f: - f.write( - f""" - [track] - default = {str(has_default).lower()} - default_remote = "origin" - """ - ) - if has_prefix: - f.write( - """ - default_remote_prefix = "myprefix" - """ - ) - if remote_branch_already_exists: - shell( - f""" - cd {base_dir} - git --git-dir ./.git-main-working-tree worktree add tmp - ( - cd tmp - touch change - git add change - git commit -m commit - git push origin HEAD:test - #git reset --hard 'HEAD@{1}' - git branch -va - ) - git --git-dir ./.git-main-working-tree worktree remove tmp - """ - ) - cmd = grm(["wt", "add", "test"], cwd=base_dir) - print(cmd.stderr) - assert cmd.returncode == 0 - - files = os.listdir(base_dir) - assert len(files) == 3 - assert set(files) == {".git-main-working-tree", "grm.toml", "test"} - - repo = git.Repo(os.path.join(base_dir, "test")) - assert not repo.bare - assert not repo.is_dirty() - assert str(repo.active_branch) == "test" - if has_default: - if has_prefix and not remote_branch_already_exists: - assert ( - str(repo.active_branch.tracking_branch()) == "origin/myprefix/test" - ) - else: - assert str(repo.active_branch.tracking_branch()) == "origin/test" - else: - assert repo.active_branch.tracking_branch() is None - - def test_worktree_delete(): with TempGitRepositoryWorktree() as (base_dir, _commit): cmd = grm(["wt", "add", "test", "--track", "origin/test"], cwd=base_dir) From b77c442f562cfeae3c9ef392e8aee8849b36715d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Fri, 17 Jun 2022 01:49:06 +0200 Subject: [PATCH 06/37] Forbid unsafe code --- src/grm/main.rs | 2 ++ src/lib.rs | 1 + 2 files changed, 3 insertions(+) diff --git a/src/grm/main.rs b/src/grm/main.rs index 5972102..2c46f11 100644 --- a/src/grm/main.rs +++ b/src/grm/main.rs @@ -1,3 +1,5 @@ +#![forbid(unsafe_code)] + use std::path::Path; use std::process; diff --git a/src/lib.rs b/src/lib.rs index 7cbf992..d406b95 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,5 +1,6 @@ #![feature(io_error_more)] #![feature(const_option_ext)] +#![forbid(unsafe_code)] use std::path::Path; From 94bfe971b32d0c3fdea908d9e34d1e6262ae8ee4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Fri, 17 Jun 2022 01:54:49 +0200 Subject: [PATCH 07/37] Add FUNDING.yml --- .github/FUNDING.yml | 1 + 1 file changed, 1 insertion(+) create mode 100644 .github/FUNDING.yml diff --git a/.github/FUNDING.yml b/.github/FUNDING.yml new file mode 100644 index 0000000..e08053a --- /dev/null +++ b/.github/FUNDING.yml @@ -0,0 +1 @@ +github: hakoerber From 10af4d7448cd90b1ecb74a67f79677298a04c225 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Fri, 17 Jun 2022 02:25:39 +0200 Subject: [PATCH 08/37] Cargo.lock: Updating strum_macros v0.24.1 -> v0.24.0 --- Cargo.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 33b7ec8..efa1f6d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1002,9 +1002,9 @@ checksum = "063e6045c0e62079840579a7e47a355ae92f60eb74daaf156fb1e84ba164e63f" [[package]] name = "strum_macros" -version = "0.24.1" +version = "0.24.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9550962e7cf70d9980392878dfaf1dcc3ece024f4cf3bf3c46b978d0bad61d6c" +checksum = "6878079b17446e4d3eba6192bb0a2950d5b14f0ed8424b852310e5a94345d0ef" dependencies = [ "heck", "proc-macro2", From 474e0b60f9c99544d5ee6f893d053f709390aa9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Fri, 17 Jun 2022 02:25:41 +0200 Subject: [PATCH 09/37] Cargo.lock: Updating crossbeam-utils v0.8.8 -> v0.8.9 --- Cargo.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index efa1f6d..523f966 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -155,12 +155,12 @@ dependencies = [ [[package]] name = "crossbeam-utils" -version = "0.8.8" +version = "0.8.9" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0bf124c720b7686e3c2663cf54062ab0f68a88af2fb6a030e87e30bf721fcb38" +checksum = "8ff1f980957787286a554052d03c7aee98d99cc32e09f6d45f0a814133c87978" dependencies = [ "cfg-if", - "lazy_static", + "once_cell", ] [[package]] From 763e014b44f5192d3834ab50577d1e471b057cb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 18:47:21 +0200 Subject: [PATCH 10/37] dependencies: Update clap to 3.2.6 --- Cargo.lock | 32 ++++++++++++++++---------------- Cargo.toml | 2 +- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 523f966..4a90ec7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -80,9 +80,9 @@ checksum = "baf1de4339761588bc0619e3cbc0120ee582ebb74b53b4efbf79117bd2da40fd" [[package]] name = "clap" -version = "3.2.5" +version = "3.2.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d53da17d37dba964b9b3ecb5c5a1f193a2762c700e6829201e645b9381c99dc7" +checksum = "9f1fe12880bae935d142c8702d500c63a4e8634b6c3c57ad72bf978fc7b6249a" dependencies = [ "atty", "bitflags", @@ -97,9 +97,9 @@ dependencies = [ [[package]] name = "clap_derive" -version = "3.2.5" +version = "3.2.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c11d40217d16aee8508cc8e5fde8b4ff24639758608e5374e731b53f85749fb9" +checksum = "ed6db9e867166a43a53f7199b5e4d1f522a1e5bd626654be263c999ce59df39a" dependencies = [ "heck", "proc-macro-error", @@ -110,9 +110,9 @@ dependencies = [ [[package]] name = "clap_lex" -version = "0.2.2" +version = "0.2.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5538cd660450ebeb4234cfecf8f2284b844ffc4c50531e66d584ad5b91293613" +checksum = "87eba3c8c7f42ef17f6c659fc7416d0f4758cd3e58861ee63c5fa4a4dde649e4" dependencies = [ "os_str_bytes", ] @@ -367,9 +367,9 @@ dependencies = [ [[package]] name = "hashbrown" -version = "0.11.2" +version = "0.12.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ab5ef0d4909ef3724cc8cce6ccc8572c5c817592e9285f5464f8e86f8bd3726e" +checksum = "db0d4cf898abf0081f964436dc980e96670a0f36863e4b83aaacdb65c9d7ccc3" [[package]] name = "heck" @@ -410,9 +410,9 @@ dependencies = [ [[package]] name = "indexmap" -version = "1.8.2" +version = "1.9.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e6012d540c5baa3589337a98ce73408de9b5a25ec9fc2c6fd6be8f0d39e0ca5a" +checksum = "10a35a97730320ffe8e2d410b5d3b69279b98d2c14bdb8b70ea89ecf7888d41e" dependencies = [ "autocfg", "hashbrown", @@ -746,18 +746,18 @@ dependencies = [ [[package]] name = "proc-macro2" -version = "1.0.39" +version = "1.0.40" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c54b25569025b7fc9651de43004ae593a75ad88543b17178aa5e1b9c4f15f56f" +checksum = "dd96a1e8ed2596c337f8eae5f24924ec83f5ad5ab21ea8e455d3566c69fbcaf7" dependencies = [ "unicode-ident", ] [[package]] name = "quote" -version = "1.0.18" +version = "1.0.20" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a1feb54ed693b93a84e14094943b84b7c4eae204c512b7ccb95ab0c66d278ad1" +checksum = "3bcdf212e9776fbcb2d23ab029360416bb1706b1aea2d1a5ba002727cbcab804" dependencies = [ "proc-macro2", ] @@ -1015,9 +1015,9 @@ dependencies = [ [[package]] name = "syn" -version = "1.0.96" +version = "1.0.98" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0748dd251e24453cb8717f0354206b91557e4ec8703673a4b30208f2abaf1ebf" +checksum = "c50aef8a904de4c23c788f104b7dddc7d6f79c647c7c8ce4cc8f73eb0ca773dd" dependencies = [ "proc-macro2", "quote", diff --git a/Cargo.toml b/Cargo.toml index c744343..4b4f443 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -54,7 +54,7 @@ version = "=0.14.4" version = "=2.1.0" [dependencies.clap] -version = "=3.2.5" +version = "=3.2.6" features = ["derive", "cargo"] [dependencies.console] From 465f877d6a49c949c06ae15138087a3dea173473 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 18:47:26 +0200 Subject: [PATCH 11/37] Cargo.lock: Updating mio v0.8.3 -> v0.8.4 --- Cargo.lock | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4a90ec7..0e4609d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -578,9 +578,9 @@ checksum = "2a60c7ce501c71e03a9c9c0d35b861413ae925bd979cc7a4e30d060069aaac8d" [[package]] name = "mio" -version = "0.8.3" +version = "0.8.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "713d550d9b44d89174e066b7a6217ae06234c10cb47819a88290d2b353c31799" +checksum = "57ee1c23c7c63b0c9250c339ffdc69255f110b298b901b9f6c82547b7b87caaf" dependencies = [ "libc", "log", @@ -847,9 +847,9 @@ dependencies = [ [[package]] name = "rustversion" -version = "1.0.6" +version = "1.0.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f2cc38e8fa666e2de3c4aba7edeb5ffc5246c1c2ed0e3d17e560aeeba736b23f" +checksum = "a0a5f7c728f5d284929a1cccb5bc19884422bfe6ef4d6c409da2c41838983fcf" [[package]] name = "ryu" From 09606cfc27797cc9f118fbaff1e070142f571415 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 18:47:32 +0200 Subject: [PATCH 12/37] Cargo.lock: Updating crossbeam-utils v0.8.9 -> v0.8.10 --- Cargo.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0e4609d..79e31bc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -155,9 +155,9 @@ dependencies = [ [[package]] name = "crossbeam-utils" -version = "0.8.9" +version = "0.8.10" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8ff1f980957787286a554052d03c7aee98d99cc32e09f6d45f0a814133c87978" +checksum = "7d82ee10ce34d7bc12c2122495e7593a9c41347ecdd64185af4ecf72cb1a7f83" dependencies = [ "cfg-if", "once_cell", From 6dc298146a9e68d7a2edf1293fe1b0731d5377a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 18:47:40 +0200 Subject: [PATCH 13/37] Cargo.lock: Updating openssl-src v111.20.0+1.1.1o -> v111.21.0+1.1.1p --- Cargo.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 79e31bc..37aeae9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -602,9 +602,9 @@ checksum = "ff011a302c396a5197692431fc1948019154afc178baf7d8e37367442a4601cf" [[package]] name = "openssl-src" -version = "111.20.0+1.1.1o" +version = "111.21.0+1.1.1p" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "92892c4f87d56e376e469ace79f1128fdaded07646ddf73aa0be4706ff712dec" +checksum = "6d0a8313729211913936f1b95ca47a5fc7f2e04cd658c115388287f8a8361008" dependencies = [ "cc", ] From ec04618a73b08d8f071b334f1ba09f07f05eff6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 18:54:49 +0200 Subject: [PATCH 14/37] Use release builds for e2e tests --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 4b4f443..ea9a717 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,7 +28,7 @@ rust-version = "1.57" license = "GPL-3.0-only" [profile.e2e-tests] -inherits = "dev" +inherits = "release" [lib] name = "grm" From ba4240720ce82ea51d8ca485548dbe9482c66097 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 18:55:00 +0200 Subject: [PATCH 15/37] Use static binary for e2e tests --- Justfile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Justfile b/Justfile index d90aaa3..b9fcf66 100644 --- a/Justfile +++ b/Justfile @@ -36,7 +36,7 @@ test-binary: env \ GITHUB_API_BASEURL=http://rest:5000/github \ GITLAB_API_BASEURL=http://rest:5000/gitlab \ - cargo build --profile e2e-tests + cargo build --profile e2e-tests --target {{static_target}} --features=static-build install: cargo install --path . @@ -64,7 +64,7 @@ test-e2e +tests=".": test-binary && docker-compose build \ && docker-compose run \ --rm \ - -v $PWD/../target/e2e-tests/grm:/grm \ + -v $PWD/../target/x86_64-unknown-linux-musl/e2e-tests/grm:/grm \ pytest \ "GRM_BINARY=/grm ALTERNATE_DOMAIN=alternate-rest python3 -m pytest -p no:cacheprovider --color=yes "$@"" \ && docker-compose rm --stop -f From 664cfb8965575c414d4fded5f5b8ff5e1a1d9be7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 18:55:39 +0200 Subject: [PATCH 16/37] e2e: Exit on first test error --- Justfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Justfile b/Justfile index b9fcf66..15f68c8 100644 --- a/Justfile +++ b/Justfile @@ -66,7 +66,7 @@ test-e2e +tests=".": test-binary --rm \ -v $PWD/../target/x86_64-unknown-linux-musl/e2e-tests/grm:/grm \ pytest \ - "GRM_BINARY=/grm ALTERNATE_DOMAIN=alternate-rest python3 -m pytest -p no:cacheprovider --color=yes "$@"" \ + "GRM_BINARY=/grm ALTERNATE_DOMAIN=alternate-rest python3 -m pytest --exitfirst -p no:cacheprovider --color=yes "$@"" \ && docker-compose rm --stop -f update-dependencies: update-cargo-dependencies From 95da48b5e61039f1aebee001232c30344f0a422b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 18:56:35 +0200 Subject: [PATCH 17/37] e2e: Don't install recommended packages in docker --- e2e_tests/docker/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e_tests/docker/Dockerfile b/e2e_tests/docker/Dockerfile index b6e75ce..91cc0a7 100644 --- a/e2e_tests/docker/Dockerfile +++ b/e2e_tests/docker/Dockerfile @@ -1,7 +1,7 @@ FROM docker.io/debian:11.3 RUN apt-get update \ - && apt-get install -y \ + && apt-get install -y --no-install-recommends \ python3-pytest \ python3-toml \ python3-git \ From ad7ef9277e89d151688b07d930e3f7a55660bb77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 18:57:58 +0200 Subject: [PATCH 18/37] e2e: Use pipefail for test scripts --- e2e_tests/helpers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e_tests/helpers.py b/e2e_tests/helpers.py index 0218890..610cc37 100644 --- a/e2e_tests/helpers.py +++ b/e2e_tests/helpers.py @@ -25,7 +25,7 @@ def grm(args, cwd=None, is_invalid=False): def shell(script): - script = "set -o errexit\nset -o nounset\n" + script + script = "set -o errexit\nset -o nounset\nset -o pipefail\n" + script subprocess.run(["bash"], input=script, text=True, check=True) From 20535125591d4f23e347b6c32d172d219b7a95ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 18:58:13 +0200 Subject: [PATCH 19/37] e2e: Print stdout/stderr on error --- e2e_tests/helpers.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/e2e_tests/helpers.py b/e2e_tests/helpers.py index 610cc37..a527d6b 100644 --- a/e2e_tests/helpers.py +++ b/e2e_tests/helpers.py @@ -26,7 +26,11 @@ def grm(args, cwd=None, is_invalid=False): def shell(script): script = "set -o errexit\nset -o nounset\nset -o pipefail\n" + script - subprocess.run(["bash"], input=script, text=True, check=True) + cmd = subprocess.run(["bash"], input=script, text=True, capture_output=True) + if cmd.returncode != 0: + print(cmd.stdout) + print(cmd.stderr) + cmd.check_returncode() def checksum_directory(path): From 8c384741b307774eb1f5769dd4ca2781384fa171 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 19:00:22 +0200 Subject: [PATCH 20/37] e2e: Fix warning about default branch name --- e2e_tests/helpers.py | 2 +- e2e_tests/test_repos_find.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/e2e_tests/helpers.py b/e2e_tests/helpers.py index a527d6b..12285a6 100644 --- a/e2e_tests/helpers.py +++ b/e2e_tests/helpers.py @@ -262,7 +262,7 @@ class TempGitFileRemote: shell( f""" cd {self.tmpdir.name} - git init + git -c init.defaultBranch=master init echo test > root-commit-in-remote-1 git add root-commit-in-remote-1 git commit -m "root-commit-in-remote-1" diff --git a/e2e_tests/test_repos_find.py b/e2e_tests/test_repos_find.py index 9f97c6d..59b0078 100644 --- a/e2e_tests/test_repos_find.py +++ b/e2e_tests/test_repos_find.py @@ -73,7 +73,7 @@ def test_repos_find(configtype, default): mkdir repo1 ( cd ./repo1 - git init + git -c init.defaultBranch=master init echo test > test git add test git commit -m "commit1" @@ -83,7 +83,7 @@ def test_repos_find(configtype, default): mkdir repo2 ( cd ./repo2 - git init + git -c init.defaultBranch=master init git checkout -b main echo test > test git add test @@ -203,7 +203,7 @@ def test_repos_find_with_invalid_repo(configtype, default): mkdir repo1 ( cd ./repo1 - git init + git -c init.defaultBranch=master init echo test > test git add test git commit -m "commit1" @@ -213,7 +213,7 @@ def test_repos_find_with_invalid_repo(configtype, default): mkdir repo2 ( cd ./repo2 - git init + git -c init.defaultBranch=master init git checkout -b main echo test > test git add test From 88961e1c6b654bd8674b9f710d3d9036f59d9598 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 19:08:55 +0200 Subject: [PATCH 21/37] e2e: Add caching to git repositories It's very expensive to create new repositories from scratch. To avoid this, a new repo & remotes are only created if necessary (depending on a cache key given on request). If not created, they are simply copied from a stored, clean repository / remote. --- e2e_tests/conftest.py | 6 + e2e_tests/helpers.py | 258 ++++++++++++++---- e2e_tests/test_worktree_clean.py | 18 +- .../test_worktree_config_presistent_branch.py | 8 +- e2e_tests/test_worktree_conversion.py | 2 +- e2e_tests/test_worktree_fetch.py | 4 +- e2e_tests/test_worktree_rebase.py | 2 +- e2e_tests/test_worktree_status.py | 6 +- e2e_tests/test_worktrees.py | 22 +- 9 files changed, 236 insertions(+), 90 deletions(-) diff --git a/e2e_tests/conftest.py b/e2e_tests/conftest.py index ac3ba25..0c23ab6 100644 --- a/e2e_tests/conftest.py +++ b/e2e_tests/conftest.py @@ -1,8 +1,14 @@ import os +from helpers import * + def pytest_configure(config): os.environ["GIT_AUTHOR_NAME"] = "Example user" os.environ["GIT_AUTHOR_EMAIL"] = "user@example.com" os.environ["GIT_COMMITTER_NAME"] = "Example user" os.environ["GIT_COMMITTER_EMAIL"] = "user@example.com" + + +def pytest_unconfigure(config): + pass diff --git a/e2e_tests/helpers.py b/e2e_tests/helpers.py index 12285a6..8718ca5 100644 --- a/e2e_tests/helpers.py +++ b/e2e_tests/helpers.py @@ -5,12 +5,26 @@ import os.path import subprocess import tempfile import hashlib +import shutil +import inspect import git binary = os.environ["GRM_BINARY"] +def funcname(): + return inspect.stack()[1][3] + + +def copytree(src, dest): + shutil.copytree(src, dest, dirs_exist_ok=True) + + +def get_temporary_directory(dir=None): + return tempfile.TemporaryDirectory(dir=dir) + + def grm(args, cwd=None, is_invalid=False): cmd = subprocess.run([binary] + args, cwd=cwd, capture_output=True, text=True) if not is_invalid: @@ -116,78 +130,204 @@ def checksum_directory(path): class TempGitRepository: def __init__(self, dir=None): self.dir = dir - pass def __enter__(self): - self.tmpdir = tempfile.TemporaryDirectory(dir=self.dir) - self.remote_1_dir = tempfile.TemporaryDirectory() - self.remote_2_dir = tempfile.TemporaryDirectory() - shell( - f""" + self.tmpdir = get_temporary_directory(self.dir) + self.remote_1 = get_temporary_directory() + self.remote_2 = get_temporary_directory() + cmd = f""" cd {self.tmpdir.name} - git init + git -c init.defaultBranch=master init echo test > root-commit git add root-commit git commit -m "root-commit" - git remote add origin file://{self.remote_1_dir.name} - git remote add otherremote file://{self.remote_2_dir.name} + git remote add origin file://{self.remote_1.name} + git remote add otherremote file://{self.remote_2.name} """ - ) + + shell(cmd) return self.tmpdir.name def __exit__(self, exc_type, exc_val, exc_tb): - del self.tmpdir - del self.remote_1_dir - del self.remote_2_dir + pass + + +class TempGitRemote: + obj = {} + + def __init__(self, tmpdir, remoteid=None): + self.tmpdir = tmpdir + self.remoteid = remoteid + + @classmethod + def get(cls, cachekey=None, initfunc=None): + if cachekey is None: + tmpdir = get_temporary_directory() + shell( + f""" + cd {tmpdir.name} + git -c init.defaultBranch=master init --bare + """ + ) + newobj = cls(tmpdir) + remoteid = None + if initfunc is not None: + remoteid = newobj.init(initfunc) + newobj.remoteid = remoteid + return newobj, remoteid + else: + refresh = False + if cachekey not in cls.obj: + tmpdir = get_temporary_directory() + shell( + f""" + cd {tmpdir.name} + git -c init.defaultBranch=master init --bare + """ + ) + newobj = cls(tmpdir) + remoteid = newobj.init(initfunc) + newobj.remoteid = remoteid + cls.obj[cachekey] = newobj + return cls.clone(cls.obj[cachekey]) + + @classmethod + def clone(cls, source): + new_remote = get_temporary_directory() + copytree(source.tmpdir.name, new_remote.name) + return cls(new_remote, source.remoteid), source.remoteid + + def init(self, func): + return func(self.tmpdir.name) + + def __enter__(self): + return self.tmpdir + + def __exit__(self, exc_type, exc_val, exc_tb): + pass class TempGitRepositoryWorktree: - def __init__(self): - pass + obj = {} + + def __init__(self, remotes, tmpdir, commit, remote1, remote2, remote1id, remote2id): + self.remotes = remotes + self.tmpdir = tmpdir + self.commit = commit + self.remote1 = remote1 + self.remote2 = remote2 + self.remote1id = remote1id + self.remote2id = remote2id + + @classmethod + def get(cls, cachekey, branch=None, remotes=2, basedir=None, remote_setup=None): + if cachekey not in cls.obj: + tmpdir = get_temporary_directory() + shell( + f""" + cd {tmpdir.name} + git -c init.defaultBranch=master init + echo test > root-commit-in-worktree-1 + git add root-commit-in-worktree-1 + git commit -m "root-commit-in-worktree-1" + echo test > root-commit-in-worktree-2 + git add root-commit-in-worktree-2 + git commit -m "root-commit-in-worktree-2" + + git ls-files | xargs rm -rf + mv .git .git-main-working-tree + git --git-dir .git-main-working-tree config core.bare true + """ + ) + + repo = git.Repo(f"{tmpdir.name}/.git-main-working-tree") + + commit = repo.head.commit.hexsha + if branch is not None: + repo.create_head(branch) + + remote1 = None + remote2 = None + remote1id = None + remote2id = None + + if remotes >= 1: + cachekeyremote, initfunc = (remote_setup or ((None, None),))[0] + remote1, remote1id = TempGitRemote.get( + cachekey=cachekeyremote, initfunc=initfunc + ) + remote1 = remote1 + remote1id = remote1id + shell( + f""" + cd {tmpdir.name} + git --git-dir .git-main-working-tree remote add origin file://{remote1.tmpdir.name} + """ + ) + repo.remotes.origin.fetch() + repo.remotes.origin.push("master") + + if remotes >= 2: + cachekeyremote, initfunc = (remote_setup or (None, (None, None)))[1] + remote2, remote2id = TempGitRemote.get( + cachekey=cachekeyremote, initfunc=initfunc + ) + remote2 = remote2 + remote2id = remote2id + shell( + f""" + cd {tmpdir.name} + git --git-dir .git-main-working-tree remote add otherremote file://{remote2.tmpdir.name} + """ + ) + repo.remotes.otherremote.fetch() + repo.remotes.otherremote.push("master") + + cls.obj[cachekey] = cls( + remotes, tmpdir, commit, remote1, remote2, remote1id, remote2id + ) + + return cls.clone(cls.obj[cachekey], remote_setup=remote_setup) + + @classmethod + def clone(cls, source, remote_setup): + newdir = get_temporary_directory() + + copytree(source.tmpdir.name, newdir.name) + + remote1 = None + remote2 = None + remote1id = None + remote2id = None + repo = git.Repo(os.path.join(newdir.name, ".git-main-working-tree")) + if source.remotes >= 1: + cachekey, initfunc = (remote_setup or ((None, None),))[0] + remote1, remote1id = TempGitRemote.get(cachekey=cachekey, initfunc=initfunc) + if remote1id != source.remote1id: + repo.remotes.origin.fetch() + repo.remotes.origin.push("master") + if source.remotes >= 2: + cachekey, initfunc = (remote_setup or (None, (None, None)))[1] + remote2, remote2id = TempGitRemote.get(cachekey=cachekey, initfunc=initfunc) + if remote2id != source.remote2id: + repo.remotes.otherremote.fetch() + repo.remotes.otherremote.push("master") + + return cls( + source.remotes, + newdir, + source.commit, + remote1, + remote2, + remote1id, + remote2id, + ) def __enter__(self): - self.tmpdir = tempfile.TemporaryDirectory() - self.remote_1_dir = tempfile.TemporaryDirectory() - self.remote_2_dir = tempfile.TemporaryDirectory() - shell( - f""" - cd {self.remote_1_dir.name} - git init --bare - """ - ) - shell( - f""" - cd {self.remote_2_dir.name} - git init --bare - """ - ) - shell( - f""" - cd {self.tmpdir.name} - git init - echo test > root-commit-in-worktree-1 - git add root-commit-in-worktree-1 - git commit -m "root-commit-in-worktree-1" - echo test > root-commit-in-worktree-2 - git add root-commit-in-worktree-2 - git commit -m "root-commit-in-worktree-2" - git remote add origin file://{self.remote_1_dir.name} - git remote add otherremote file://{self.remote_2_dir.name} - git push origin HEAD:master - git ls-files | xargs rm -rf - mv .git .git-main-working-tree - git --git-dir .git-main-working-tree config core.bare true - """ - ) - commit = git.Repo( - f"{self.tmpdir.name}/.git-main-working-tree" - ).head.commit.hexsha - return (self.tmpdir.name, commit) + return (self.tmpdir.name, self.commit) def __exit__(self, exc_type, exc_val, exc_tb): - del self.tmpdir - del self.remote_1_dir - del self.remote_2_dir + pass class RepoTree: @@ -195,7 +335,7 @@ class RepoTree: pass def __enter__(self): - self.root = tempfile.TemporaryDirectory() + self.root = get_temporary_directory() self.config = tempfile.NamedTemporaryFile() with open(self.config.name, "w") as f: f.write( @@ -226,7 +366,7 @@ class EmptyDir: pass def __enter__(self): - self.tmpdir = tempfile.TemporaryDirectory() + self.tmpdir = get_temporary_directory() return self.tmpdir.name def __exit__(self, exc_type, exc_val, exc_tb): @@ -238,7 +378,7 @@ class NonGitDir: pass def __enter__(self): - self.tmpdir = tempfile.TemporaryDirectory() + self.tmpdir = get_temporary_directory() shell( f""" cd {self.tmpdir.name} @@ -258,7 +398,7 @@ class TempGitFileRemote: pass def __enter__(self): - self.tmpdir = tempfile.TemporaryDirectory() + self.tmpdir = get_temporary_directory() shell( f""" cd {self.tmpdir.name} diff --git a/e2e_tests/test_worktree_clean.py b/e2e_tests/test_worktree_clean.py index bdcf22e..b64172a 100644 --- a/e2e_tests/test_worktree_clean.py +++ b/e2e_tests/test_worktree_clean.py @@ -6,7 +6,7 @@ from helpers import * def test_worktree_clean(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test", "--track", "origin/test"], cwd=base_dir) assert cmd.returncode == 0 assert "test" in os.listdir(base_dir) @@ -17,7 +17,7 @@ def test_worktree_clean(): def test_worktree_clean_refusal_no_tracking_branch(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test"], cwd=base_dir) assert cmd.returncode == 0 @@ -31,7 +31,7 @@ def test_worktree_clean_refusal_no_tracking_branch(): def test_worktree_clean_refusal_uncommited_changes_new_file(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test", "--track", "origin/test"], cwd=base_dir) assert cmd.returncode == 0 @@ -47,7 +47,7 @@ def test_worktree_clean_refusal_uncommited_changes_new_file(): def test_worktree_clean_refusal_uncommited_changes_changed_file(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test", "--track", "origin/test"], cwd=base_dir) assert cmd.returncode == 0 @@ -63,7 +63,7 @@ def test_worktree_clean_refusal_uncommited_changes_changed_file(): def test_worktree_clean_refusal_uncommited_changes_cleand_file(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test", "--track", "origin/test"], cwd=base_dir) assert cmd.returncode == 0 @@ -81,7 +81,7 @@ def test_worktree_clean_refusal_uncommited_changes_cleand_file(): def test_worktree_clean_refusal_commited_changes(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test", "--track", "origin/test"], cwd=base_dir) assert cmd.returncode == 0 @@ -99,7 +99,7 @@ def test_worktree_clean_refusal_commited_changes(): def test_worktree_clean_refusal_tracking_branch_mismatch(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test", "--track", "origin/test"], cwd=base_dir) assert cmd.returncode == 0 @@ -117,7 +117,7 @@ def test_worktree_clean_refusal_tracking_branch_mismatch(): def test_worktree_clean_fail_from_subdir(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test"], cwd=base_dir) assert cmd.returncode == 0 @@ -148,7 +148,7 @@ def test_worktree_clean_non_git(): def test_worktree_clean_configured_default_branch( configure_default_branch, branch_list_empty ): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): if configure_default_branch: with open(os.path.join(base_dir, "grm.toml"), "w") as f: if branch_list_empty: diff --git a/e2e_tests/test_worktree_config_presistent_branch.py b/e2e_tests/test_worktree_config_presistent_branch.py index 9174b77..e7d2923 100644 --- a/e2e_tests/test_worktree_config_presistent_branch.py +++ b/e2e_tests/test_worktree_config_presistent_branch.py @@ -6,7 +6,7 @@ from helpers import * def test_worktree_never_clean_persistent_branches(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): with open(os.path.join(base_dir, "grm.toml"), "w") as f: f.write( """ @@ -33,7 +33,7 @@ def test_worktree_never_clean_persistent_branches(): def test_worktree_clean_branch_merged_into_persistent(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): with open(os.path.join(base_dir, "grm.toml"), "w") as f: f.write( """ @@ -72,7 +72,7 @@ def test_worktree_clean_branch_merged_into_persistent(): def test_worktree_no_clean_unmerged_branch(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): with open(os.path.join(base_dir, "grm.toml"), "w") as f: f.write( """ @@ -105,7 +105,7 @@ def test_worktree_no_clean_unmerged_branch(): def test_worktree_delete_branch_merged_into_persistent(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): with open(os.path.join(base_dir, "grm.toml"), "w") as f: f.write( """ diff --git a/e2e_tests/test_worktree_conversion.py b/e2e_tests/test_worktree_conversion.py index 33d5312..7f1448d 100644 --- a/e2e_tests/test_worktree_conversion.py +++ b/e2e_tests/test_worktree_conversion.py @@ -23,7 +23,7 @@ def test_convert(): def test_convert_already_worktree(): - with TempGitRepositoryWorktree() as (git_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (git_dir, _commit): before = checksum_directory(git_dir) cmd = grm(["wt", "convert"], cwd=git_dir) diff --git a/e2e_tests/test_worktree_fetch.py b/e2e_tests/test_worktree_fetch.py index df01462..a30ef7d 100644 --- a/e2e_tests/test_worktree_fetch.py +++ b/e2e_tests/test_worktree_fetch.py @@ -9,7 +9,7 @@ import git def test_worktree_fetch(): - with TempGitRepositoryWorktree() as (base_dir, root_commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, root_commit): with TempGitFileRemote() as (remote_path, _remote_sha): shell( f""" @@ -56,7 +56,7 @@ def test_worktree_fetch(): @pytest.mark.parametrize("has_changes", [True, False]) @pytest.mark.parametrize("stash", [True, False]) def test_worktree_pull(rebase, ffable, has_changes, stash): - with TempGitRepositoryWorktree() as (base_dir, root_commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, root_commit): with TempGitFileRemote() as (remote_path, _remote_sha): shell( f""" diff --git a/e2e_tests/test_worktree_rebase.py b/e2e_tests/test_worktree_rebase.py index a64f755..0c73c79 100644 --- a/e2e_tests/test_worktree_rebase.py +++ b/e2e_tests/test_worktree_rebase.py @@ -14,7 +14,7 @@ import git @pytest.mark.parametrize("has_changes", [True, False]) @pytest.mark.parametrize("stash", [True, False]) def test_worktree_rebase(pull, rebase, ffable, has_changes, stash): - with TempGitRepositoryWorktree() as (base_dir, _root_commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _root_commit): with open(os.path.join(base_dir, "grm.toml"), "w") as f: f.write('persistent_branches = ["mybasebranch"]') diff --git a/e2e_tests/test_worktree_status.py b/e2e_tests/test_worktree_status.py index 68b86e7..d54788f 100644 --- a/e2e_tests/test_worktree_status.py +++ b/e2e_tests/test_worktree_status.py @@ -9,7 +9,7 @@ import pytest @pytest.mark.parametrize("has_config", [True, False]) def test_worktree_status(has_config): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): if has_config: with open(os.path.join(base_dir, "grm.toml"), "w") as f: f.write("") @@ -24,7 +24,7 @@ def test_worktree_status(has_config): def test_worktree_status_fail_from_subdir(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test"], cwd=base_dir) assert cmd.returncode == 0 @@ -51,7 +51,7 @@ def test_worktree_status_non_git(): def test_worktree_status_warn_with_non_worktree_dir(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test"], cwd=base_dir) assert cmd.returncode == 0 diff --git a/e2e_tests/test_worktrees.py b/e2e_tests/test_worktrees.py index d669a5e..67c3812 100644 --- a/e2e_tests/test_worktrees.py +++ b/e2e_tests/test_worktrees.py @@ -96,7 +96,7 @@ def test_worktree_add( def test_worktree_add_invalid_name(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): for worktree_name in ["/absolute/path" "trailingslash/"]: args = ["wt", "add", worktree_name] cmd = grm(args, cwd=base_dir) @@ -127,7 +127,7 @@ def test_worktree_add_into_invalid_subdirectory(): def test_worktree_add_with_tracking( remote_branch_already_exists, has_config, has_default, has_prefix ): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): if has_config: with open(os.path.join(base_dir, "grm.toml"), "w") as f: f.write( @@ -229,7 +229,7 @@ def test_worktree_add_with_explicit_no_tracking( def test_worktree_delete(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test", "--track", "origin/test"], cwd=base_dir) assert cmd.returncode == 0 assert "test" in os.listdir(base_dir) @@ -246,7 +246,7 @@ def test_worktree_delete(): def test_worktree_delete_refusal_no_tracking_branch(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test"], cwd=base_dir) assert cmd.returncode == 0 @@ -262,7 +262,7 @@ def test_worktree_delete_refusal_no_tracking_branch(): def test_worktree_delete_refusal_uncommited_changes_new_file(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test", "--track", "origin/test"], cwd=base_dir) assert cmd.returncode == 0 @@ -280,7 +280,7 @@ def test_worktree_delete_refusal_uncommited_changes_new_file(): def test_worktree_delete_refusal_uncommited_changes_changed_file(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test", "--track", "origin/test"], cwd=base_dir) assert cmd.returncode == 0 @@ -298,7 +298,7 @@ def test_worktree_delete_refusal_uncommited_changes_changed_file(): def test_worktree_delete_refusal_uncommited_changes_deleted_file(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test", "--track", "origin/test"], cwd=base_dir) assert cmd.returncode == 0 @@ -318,7 +318,7 @@ def test_worktree_delete_refusal_uncommited_changes_deleted_file(): def test_worktree_delete_refusal_commited_changes(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test", "--track", "origin/test"], cwd=base_dir) assert cmd.returncode == 0 @@ -338,7 +338,7 @@ def test_worktree_delete_refusal_commited_changes(): def test_worktree_delete_refusal_tracking_branch_mismatch(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test", "--track", "origin/test"], cwd=base_dir) assert cmd.returncode == 0 @@ -358,7 +358,7 @@ def test_worktree_delete_refusal_tracking_branch_mismatch(): def test_worktree_delete_force_refusal(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test"], cwd=base_dir) assert cmd.returncode == 0 @@ -368,7 +368,7 @@ def test_worktree_delete_force_refusal(): def test_worktree_add_delete_add(): - with TempGitRepositoryWorktree() as (base_dir, _commit): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test", "--track", "origin/test"], cwd=base_dir) assert cmd.returncode == 0 assert "test" in os.listdir(base_dir) From 92ec2e1a2d298db5cf3115f03449687a48097b87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 19:13:48 +0200 Subject: [PATCH 22/37] e2e: Test worktree names with whitespace --- e2e_tests/test_worktrees.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/e2e_tests/test_worktrees.py b/e2e_tests/test_worktrees.py index 67c3812..2695b0f 100644 --- a/e2e_tests/test_worktrees.py +++ b/e2e_tests/test_worktrees.py @@ -97,7 +97,13 @@ def test_worktree_add( def test_worktree_add_invalid_name(): with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): - for worktree_name in ["/absolute/path" "trailingslash/"]: + for worktree_name in [ + "/absolute/path", + "trailingslash/", + "with spaces", + "with\t tabs", + "with\nnewline", + ]: args = ["wt", "add", worktree_name] cmd = grm(args, cwd=base_dir) assert cmd.returncode != 0 From eac22148c5dde428d53088c76ae095fc9442d46e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 19:15:28 +0200 Subject: [PATCH 23/37] e2e: Move invalid subdirectory test --- e2e_tests/test_worktrees.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/e2e_tests/test_worktrees.py b/e2e_tests/test_worktrees.py index 2695b0f..4948c43 100644 --- a/e2e_tests/test_worktrees.py +++ b/e2e_tests/test_worktrees.py @@ -114,16 +114,6 @@ def test_worktree_add_invalid_name(): assert "invalid worktree name" in str(cmd.stderr.lower()) -def test_worktree_add_into_invalid_subdirectory(): - with TempGitRepositoryWorktree() as (base_dir, _commit): - cmd = grm(["wt", "add", "/dir/test"], cwd=base_dir) - assert cmd.returncode == 1 - assert "dir" not in os.listdir(base_dir) - assert "dir" not in os.listdir("/") - - cmd = grm(["wt", "add", "dir/"], cwd=base_dir) - assert cmd.returncode == 1 - assert "dir" not in os.listdir(base_dir) @pytest.mark.parametrize("remote_branch_already_exists", [True, False]) @@ -232,6 +222,16 @@ def test_worktree_add_with_explicit_no_tracking( assert not repo.is_dirty() assert str(repo.active_branch) == "test" assert repo.active_branch.tracking_branch() is None +def test_worktree_add_into_invalid_subdirectory(): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): + cmd = grm(["wt", "add", "/dir/test"], cwd=base_dir) + assert cmd.returncode == 1 + assert "dir" not in os.listdir(base_dir) + assert "dir" not in os.listdir("/") + + cmd = grm(["wt", "add", "dir/"], cwd=base_dir) + assert cmd.returncode == 1 + assert "dir" not in os.listdir(base_dir) def test_worktree_delete(): From 09ce9f043ed1705bd8043eb586728c7a882f789b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 19:15:52 +0200 Subject: [PATCH 24/37] e2e: Add test case for invalid tracks --- e2e_tests/test_worktrees.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/e2e_tests/test_worktrees.py b/e2e_tests/test_worktrees.py index 4948c43..8b076ab 100644 --- a/e2e_tests/test_worktrees.py +++ b/e2e_tests/test_worktrees.py @@ -114,6 +114,16 @@ def test_worktree_add_invalid_name(): assert "invalid worktree name" in str(cmd.stderr.lower()) +def test_worktree_add_invalid_track(): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): + for track in ["/absolute/path", "trailingslash/", "/"]: + args = ["wt", "add", "foo", "--track", track] + cmd = grm(args, cwd=base_dir) + assert cmd.returncode != 0 + assert len(cmd.stderr.strip().split("\n")) == 1 + assert not os.path.exists("foo") + assert not os.path.exists(os.path.join(base_dir, "foo")) + assert "tracking branch" in str(cmd.stderr.lower()) @pytest.mark.parametrize("remote_branch_already_exists", [True, False]) From d7ab3c4d6b90f42b6904275f704b2bc6a782457c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 19:16:27 +0200 Subject: [PATCH 25/37] e2e: Remove unnecessary output --- e2e_tests/test_worktrees.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/e2e_tests/test_worktrees.py b/e2e_tests/test_worktrees.py index 8b076ab..27d9f33 100644 --- a/e2e_tests/test_worktrees.py +++ b/e2e_tests/test_worktrees.py @@ -107,8 +107,6 @@ def test_worktree_add_invalid_name(): args = ["wt", "add", worktree_name] cmd = grm(args, cwd=base_dir) assert cmd.returncode != 0 - print(cmd.stdout) - print(cmd.stderr) assert not os.path.exists(worktree_name) assert not os.path.exists(os.path.join(base_dir, worktree_name)) assert "invalid worktree name" in str(cmd.stderr.lower()) @@ -257,7 +255,6 @@ def test_worktree_delete(): cmd = grm(["wt", "add", "check"], cwd=base_dir) assert cmd.returncode == 0 repo = git.Repo(os.path.join(base_dir, ".git-main-working-tree")) - print(repo.branches) assert "test" not in [str(b) for b in repo.branches] From 3eabc0e8f8b02cf40ff6fbe51c29ffb3d5ffde10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 19:17:01 +0200 Subject: [PATCH 26/37] e2e: Update test for invalid remote name --- e2e_tests/test_worktrees.py | 111 ++++++------------------------------ 1 file changed, 18 insertions(+), 93 deletions(-) diff --git a/e2e_tests/test_worktrees.py b/e2e_tests/test_worktrees.py index 27d9f33..1ded432 100644 --- a/e2e_tests/test_worktrees.py +++ b/e2e_tests/test_worktrees.py @@ -124,112 +124,37 @@ def test_worktree_add_invalid_track(): assert "tracking branch" in str(cmd.stderr.lower()) -@pytest.mark.parametrize("remote_branch_already_exists", [True, False]) -@pytest.mark.parametrize("has_config", [True, False]) -@pytest.mark.parametrize("has_default", [True, False]) -@pytest.mark.parametrize("has_prefix", [True, False]) -def test_worktree_add_with_tracking( - remote_branch_already_exists, has_config, has_default, has_prefix +@pytest.mark.parametrize("use_track", [True, False]) +@pytest.mark.parametrize("use_configuration", [True, False]) +@pytest.mark.parametrize("use_configuration_default", [True, False]) +def test_worktree_add_invalid_remote_name( + use_track, use_configuration, use_configuration_default ): with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): - if has_config: + if use_configuration: with open(os.path.join(base_dir, "grm.toml"), "w") as f: f.write( f""" [track] - default = {str(has_default).lower()} - default_remote = "origin" + default = {str(use_configuration_default).lower()} + default_remote = "thisremotedoesnotexist" """ ) - if has_prefix: - f.write( - """ - default_remote_prefix = "myprefix" - """ - ) - if remote_branch_already_exists: - shell( - f""" - cd {base_dir} - git --git-dir ./.git-main-working-tree worktree add tmp - ( - cd tmp - touch change - git add change - git commit -m commit - git push origin HEAD:test - #git reset --hard 'HEAD@{1}' - git branch -va - ) - git --git-dir ./.git-main-working-tree worktree remove tmp - """ - ) - cmd = grm(["wt", "add", "test", "--track", "origin/test"], cwd=base_dir) - print(cmd.stderr) - assert cmd.returncode == 0 + args = ["wt", "add", "foo"] + if use_track: + args.extend(["--track", "thisremotedoesnotexist/master"]) - files = os.listdir(base_dir) - if has_config is True: - assert len(files) == 3 - assert set(files) == {".git-main-working-tree", "grm.toml", "test"} + cmd = grm(args, cwd=base_dir) + + if use_track or (use_configuration and use_configuration_default): + assert cmd.returncode != 0 + assert "thisremotedoesnotexist" in cmd.stderr else: - assert len(files) == 2 - assert set(files) == {".git-main-working-tree", "test"} - - repo = git.Repo(os.path.join(base_dir, "test")) - assert not repo.bare - assert not repo.is_dirty() - assert str(repo.active_branch) == "test" - assert str(repo.active_branch.tracking_branch()) == "origin/test" + assert cmd.returncode == 0 + assert len(cmd.stderr) == 0 -@pytest.mark.parametrize("has_config", [True, False]) -@pytest.mark.parametrize("has_default", [True, False]) -@pytest.mark.parametrize("has_prefix", [True, False]) -@pytest.mark.parametrize("track", [True, False]) -def test_worktree_add_with_explicit_no_tracking( - has_config, has_default, has_prefix, track -): - with TempGitRepositoryWorktree() as (base_dir, _commit): - if has_config: - with open(os.path.join(base_dir, "grm.toml"), "w") as f: - f.write( - f""" - [track] - default = {str(has_default).lower()} - default_remote = "origin" - """ - ) - if has_prefix: - f.write( - """ - default_remote_prefix = "myprefix" - """ - ) - if track is True: - cmd = grm( - ["wt", "add", "test", "--track", "origin/test", "--no-track"], - cwd=base_dir, - ) - else: - cmd = grm(["wt", "add", "test", "--no-track"], cwd=base_dir) - print(cmd.stderr) - assert cmd.returncode == 0 - - files = os.listdir(base_dir) - if has_config is True: - assert len(files) == 3 - assert set(files) == {".git-main-working-tree", "grm.toml", "test"} - else: - assert len(files) == 2 - assert set(files) == {".git-main-working-tree", "test"} - - repo = git.Repo(os.path.join(base_dir, "test")) - assert not repo.bare - assert not repo.is_dirty() - assert str(repo.active_branch) == "test" - assert repo.active_branch.tracking_branch() is None def test_worktree_add_into_invalid_subdirectory(): with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "/dir/test"], cwd=base_dir) From 056480f65a45b68e4ef4b2ce2532017021d47267 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 19:13:30 +0200 Subject: [PATCH 27/37] e2e: Update test for worktree adding --- e2e_tests/test_worktrees.py | 475 +++++++++++++++++++++++++++++++++--- 1 file changed, 435 insertions(+), 40 deletions(-) diff --git a/e2e_tests/test_worktrees.py b/e2e_tests/test_worktrees.py index 1ded432..b2d21a3 100644 --- a/e2e_tests/test_worktrees.py +++ b/e2e_tests/test_worktrees.py @@ -4,92 +4,487 @@ from helpers import * import git import pytest +import datetime import os.path +@pytest.mark.parametrize("config_enabled", [True, False]) +@pytest.mark.parametrize("config_has_default_remote_prefix", [True, False]) +@pytest.mark.parametrize("config_has_default_track_enabled", [True, False]) +@pytest.mark.parametrize("explicit_notrack", [True, False]) +@pytest.mark.parametrize("explicit_track", [True, False]) +@pytest.mark.parametrize("local_branch_exists", [True, False]) +@pytest.mark.parametrize("local_branch_has_tracking_branch", [True, False]) @pytest.mark.parametrize("remote_branch_already_exists", [True, False]) -@pytest.mark.parametrize("has_config", [True, False]) -@pytest.mark.parametrize("has_default", [True, False]) -@pytest.mark.parametrize("has_prefix", [True, False]) +@pytest.mark.parametrize("remote_branch_with_prefix_already_exists", [True, False]) +@pytest.mark.parametrize( + "remote_setup", + ( + (0, "origin", False), + (1, "origin", False), + (2, "origin", False), + (2, "otherremote", False), + (2, "origin", True), + (2, "otherremote", True), + ), +) +@pytest.mark.parametrize("track_differs_from_existing_branch_upstream", [True, False]) @pytest.mark.parametrize("worktree_with_slash", [True, False]) def test_worktree_add( + config_enabled, + config_has_default_remote_prefix, + config_has_default_track_enabled, + explicit_notrack, + explicit_track, + local_branch_exists, + local_branch_has_tracking_branch, remote_branch_already_exists, - has_config, - has_default, - has_prefix, + remote_branch_with_prefix_already_exists, + remote_setup, + track_differs_from_existing_branch_upstream, worktree_with_slash, ): + (remote_count, default_remote, remotes_differ) = remote_setup + has_remotes = True if remote_count > 0 else False + if worktree_with_slash: - worktree_name = "dir/test" + worktree_name = "dir/nested/test" else: worktree_name = "test" - with TempGitRepositoryWorktree() as (base_dir, _commit): - if has_config: + + if track_differs_from_existing_branch_upstream: + explicit_track_branch_name = f"{default_remote}/somethingelse" + else: + explicit_track_branch_name = f"{default_remote}/{worktree_name}" + + timestamp = datetime.datetime.now().replace(microsecond=0).isoformat() + # GitPython has some weird behaviour here. It is not possible to use kwargs + # to set the commit and author date. + # + # `committer_date=x` (which is documented) does not work, as `git commit` + # does not accept --committer-date + # + # `author_date=x` does not work, as it's now called --date in `git commit` + # + # `date=x` should work, but is refused by GitPython, as it does not know + # about the new behaviour in `git commit` + # + # Fortunately, there are env variables that control those timestamps. + os.environ["GIT_COMMITTER_DATE"] = str(timestamp) + os.environ["GIT_AUTHOR_DATE"] = str(timestamp) + + def setup_remote1(directory): + if remote_branch_already_exists: + with tempfile.TemporaryDirectory() as cloned: + repo = git.Repo.clone_from(directory, cloned) + newfile = os.path.join(cloned, "change") + open(newfile, "w").close() + repo.index.add([newfile]) + repo.index.commit("commit") + repo.remotes.origin.push(f"HEAD:{worktree_name}", force=True) + + if remote_branch_with_prefix_already_exists: + with tempfile.TemporaryDirectory() as cloned: + repo = git.Repo.clone_from(directory, cloned) + newfile = os.path.join(cloned, "change2") + open(newfile, "w").close() + repo.index.add([newfile]) + repo.index.commit("commit") + repo.remotes.origin.push(f"HEAD:myprefix/{worktree_name}", force=True) + + return "_".join( + [ + str(worktree_with_slash), + str(remote_branch_already_exists), + str(remote_branch_with_prefix_already_exists), + str(remotes_differ), + ] + ) + + def setup_remote2(directory): + if remote_branch_already_exists: + with tempfile.TemporaryDirectory() as cloned: + repo = git.Repo.clone_from(directory, cloned) + newfile = os.path.join(cloned, "change") + open(newfile, "w").close() + repo.index.add([newfile]) + repo.index.commit("commit") + if remotes_differ: + newfile = os.path.join(cloned, "change_on_second_remote") + open(newfile, "w").close() + repo.index.add([newfile]) + repo.index.commit("commit_on_second_remote") + repo.remotes.origin.push(f"HEAD:{worktree_name}", force=True) + + if remote_branch_with_prefix_already_exists: + with tempfile.TemporaryDirectory() as cloned: + repo = git.Repo.clone_from(directory, cloned) + newfile = os.path.join(cloned, "change2") + open(newfile, "w").close() + repo.index.add([newfile]) + repo.index.commit("commit") + if remotes_differ: + newfile = os.path.join(cloned, "change_on_second_remote2") + open(newfile, "w").close() + repo.index.add([newfile]) + repo.index.commit("commit_on_second_remote2") + repo.remotes.origin.push(f"HEAD:myprefix/{worktree_name}", force=True) + + return "_".join( + [ + str(worktree_with_slash), + str(remote_branch_already_exists), + str(remote_branch_with_prefix_already_exists), + str(remotes_differ), + ] + ) + + cachefn = lambda nr: "_".join( + [ + str(nr), + str(default_remote), + str(local_branch_exists), + str(remote_branch_already_exists), + str(remote_branch_with_prefix_already_exists), + str(remote_count), + str(remotes_differ), + str(worktree_name), + ] + ) + remote1_cache_key = cachefn(1) + remote2_cache_key = cachefn(2) + + cachekey = "_".join( + [ + str(local_branch_exists), + str(local_branch_has_tracking_branch), + str(remote_branch_already_exists), + str(remote_branch_with_prefix_already_exists), + str(remote_count), + str(remotes_differ), + str(worktree_name), + ] + ) + + with TempGitRepositoryWorktree.get( + cachekey=cachekey, + branch=worktree_name if local_branch_exists else None, + remotes=remote_count, + remote_setup=[ + [remote1_cache_key, setup_remote1], + [remote2_cache_key, setup_remote2], + ], + ) as (base_dir, initial_commit): + repo = git.Repo(os.path.join(base_dir, ".git-main-working-tree")) + + if config_enabled: with open(os.path.join(base_dir, "grm.toml"), "w") as f: f.write( f""" - [track] - default = {str(has_default).lower()} - default_remote = "origin" - """ + [track] + default = {str(config_has_default_track_enabled).lower()} + default_remote = "{default_remote}" + """ ) - if has_prefix: + + if config_has_default_remote_prefix: f.write( """ default_remote_prefix = "myprefix" """ ) - if remote_branch_already_exists: - shell( - f""" - cd {base_dir} - git --git-dir ./.git-main-working-tree worktree add tmp - ( - cd tmp - touch change - git add change - git commit -m commit - git push origin HEAD:{worktree_name} - #git reset --hard 'HEAD@{1}' - git branch -va - ) - git --git-dir ./.git-main-working-tree worktree remove tmp - """ - ) - cmd = grm(["wt", "add", worktree_name], cwd=base_dir) + if local_branch_exists: + if has_remotes and local_branch_has_tracking_branch: + origin = repo.remote(default_remote) + if remote_count >= 2: + otherremote = repo.remote("otherremote") + br = list(filter(lambda x: x.name == worktree_name, repo.branches))[0] + assert os.path.exists(base_dir) + if track_differs_from_existing_branch_upstream: + origin.push( + f"{worktree_name}:someothername", force=True, set_upstream=True + ) + if remote_count >= 2: + otherremote.push( + f"{worktree_name}:someothername", + force=True, + set_upstream=True, + ) + br.set_tracking_branch( + list( + filter( + lambda x: x.remote_head == "someothername", origin.refs + ) + )[0] + ) + else: + origin.push( + f"{worktree_name}:{worktree_name}", + force=True, + set_upstream=True, + ) + if remote_count >= 2: + otherremote.push( + f"{worktree_name}:{worktree_name}", + force=True, + set_upstream=True, + ) + br.set_tracking_branch( + list( + filter( + lambda x: x.remote_head == worktree_name, origin.refs + ) + )[0] + ) + + args = ["wt", "add", worktree_name] + if explicit_track: + args.extend(["--track", explicit_track_branch_name]) + if explicit_notrack: + args.extend(["--no-track"]) + cmd = grm(args, cwd=base_dir) + if explicit_track and not explicit_notrack and not has_remotes: + assert cmd.returncode != 0 + assert f'remote "{default_remote}" not found' in cmd.stderr.lower() + return assert cmd.returncode == 0 + assert len(cmd.stdout.strip().split("\n")) == 1 + assert f"worktree {worktree_name} created" in cmd.stdout.lower() + + def check_deviation_error(base): + if ( + not local_branch_exists + and (explicit_notrack or (not explicit_notrack and not explicit_track)) + and ( + remote_branch_already_exists + or ( + config_enabled + and config_has_default_remote_prefix + and remote_branch_with_prefix_already_exists + ) + ) + and remote_count >= 2 + and remotes_differ + ): + assert ( + f"branch exists on multiple remotes, but they deviate" + in cmd.stderr.lower() + ) + assert len(cmd.stderr.strip().split("\n")) == base + 1 + else: + if base == 0: + assert len(cmd.stderr) == base + else: + assert len(cmd.stderr.strip().split("\n")) == base + + if explicit_track and explicit_notrack: + assert "--track will be ignored" in cmd.stderr.lower() + check_deviation_error(1) + else: + check_deviation_error(0) + files = os.listdir(base_dir) - if has_config is True: + if config_enabled is True: + if worktree_with_slash: + assert set(files) == {".git-main-working-tree", "grm.toml", "dir"} + else: + assert set(files) == {".git-main-working-tree", "grm.toml", "test"} assert len(files) == 3 if worktree_with_slash: assert set(files) == {".git-main-working-tree", "grm.toml", "dir"} - assert set(os.listdir(os.path.join(base_dir, "dir"))) == {"test"} + assert set(os.listdir(os.path.join(base_dir, "dir"))) == {"nested"} + assert set(os.listdir(os.path.join(base_dir, "dir/nested"))) == {"test"} else: assert set(files) == {".git-main-working-tree", "grm.toml", "test"} else: assert len(files) == 2 if worktree_with_slash: assert set(files) == {".git-main-working-tree", "dir"} - assert set(os.listdir(os.path.join(base_dir, "dir"))) == {"test"} + assert set(os.listdir(os.path.join(base_dir, "dir"))) == {"nested"} + assert set(os.listdir(os.path.join(base_dir, "dir/nested"))) == {"test"} else: assert set(files) == {".git-main-working-tree", "test"} repo = git.Repo(os.path.join(base_dir, worktree_name)) assert not repo.bare - assert not repo.is_dirty() - if has_config and has_default: - if has_prefix and not remote_branch_already_exists: + # assert not repo.is_dirty() + assert str(repo.head.ref) == worktree_name + + local_commit = repo.head.commit.hexsha + + if not has_remotes: + assert local_commit == initial_commit + elif local_branch_exists: + assert local_commit == initial_commit + elif explicit_track and not explicit_notrack: + assert local_commit == repo.commit(explicit_track_branch_name).hexsha + elif explicit_notrack: + if config_enabled and config_has_default_remote_prefix: + if remote_branch_with_prefix_already_exists: + assert ( + local_commit + == repo.commit( + f"{default_remote}/myprefix/{worktree_name}" + ).hexsha + ) + elif remote_branch_already_exists: + assert ( + local_commit + == repo.commit(f"{default_remote}/{worktree_name}").hexsha + ) + else: + assert local_commit == initial_commit + elif remote_count == 1: + if config_enabled and config_has_default_remote_prefix: + if remote_branch_with_prefix_already_exists: + assert ( + local_commit + == repo.commit( + f"{default_remote}/myprefix/{worktree_name}" + ).hexsha + ) + elif remote_branch_already_exists: + assert ( + local_commit + == repo.commit(f"{default_remote}/{worktree_name}").hexsha + ) + else: + assert local_commit == initial_commit + elif remote_branch_already_exists: + assert ( + local_commit + == repo.commit(f"{default_remote}/{worktree_name}").hexsha + ) + else: + assert local_commit == initial_commit + elif remotes_differ: + if config_enabled: # we have a default remote + if ( + config_has_default_remote_prefix + and remote_branch_with_prefix_already_exists + ): + assert ( + local_commit + == repo.commit( + f"{default_remote}/myprefix/{worktree_name}" + ).hexsha + ) + elif remote_branch_already_exists: + assert ( + local_commit + == repo.commit(f"{default_remote}/{worktree_name}").hexsha + ) + else: + assert local_commit == initial_commit + else: + assert local_commit == initial_commit + + else: + if config_enabled and config_has_default_remote_prefix: + if remote_branch_with_prefix_already_exists: + assert ( + local_commit + == repo.commit( + f"{default_remote}/myprefix/{worktree_name}" + ).hexsha + ) + elif remote_branch_already_exists: + assert ( + local_commit + == repo.commit(f"{default_remote}/{worktree_name}").hexsha + ) + else: + assert local_commit == initial_commit + + elif config_enabled: + if not config_has_default_remote_prefix: + if config_has_default_track_enabled: + assert ( + local_commit + == repo.commit(f"{default_remote}/{worktree_name}").hexsha + ) + else: + if remote_branch_already_exists: + assert ( + local_commit + == repo.commit(f"{default_remote}/{worktree_name}").hexsha + ) + else: + assert local_commit == initial_commit + else: + if remote_branch_with_prefix_already_exists: + assert ( + local_commit + == repo.commit( + f"{default_remote}/myprefix/{worktree_name}" + ).hexsha + ) + elif remote_branch_already_exists: + assert ( + local_commit + == repo.commit(f"{default_remote}/{worktree_name}").hexsha + ) + elif config_has_default_track_enabled: + assert ( + local_commit + == repo.commit( + f"{default_remote}/myprefix/{worktree_name}" + ).hexsha + ) + else: + assert local_commit == initial_commit + elif remote_branch_already_exists and not remotes_differ: + assert ( + local_commit == repo.commit(f"{default_remote}/{worktree_name}").hexsha + ) + else: + assert local_commit == initial_commit + + # Check whether tracking is ok + if not has_remotes: + assert repo.active_branch.tracking_branch() is None + elif explicit_notrack: + if local_branch_exists and local_branch_has_tracking_branch: + if track_differs_from_existing_branch_upstream: + assert ( + str(repo.active_branch.tracking_branch()) + == f"{default_remote}/someothername" + ) + else: + assert ( + str(repo.active_branch.tracking_branch()) + == f"{default_remote}/{worktree_name}" + ) + else: + assert repo.active_branch.tracking_branch() is None + elif explicit_track: + assert ( + str(repo.active_branch.tracking_branch()) == explicit_track_branch_name + ) + elif config_enabled and config_has_default_track_enabled: + if config_has_default_remote_prefix: assert ( str(repo.active_branch.tracking_branch()) - == f"origin/myprefix/{worktree_name}" + == f"{default_remote}/myprefix/{worktree_name}" ) else: assert ( str(repo.active_branch.tracking_branch()) - == f"origin/{worktree_name}" + == f"{default_remote}/{worktree_name}" + ) + elif local_branch_exists and local_branch_has_tracking_branch: + if track_differs_from_existing_branch_upstream: + assert ( + str(repo.active_branch.tracking_branch()) + == f"{default_remote}/someothername" + ) + else: + assert ( + str(repo.active_branch.tracking_branch()) + == f"{default_remote}/{worktree_name}" ) else: assert repo.active_branch.tracking_branch() is None From e78dcf471a8528ef2ccaff589813f8e3e816448b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 19:17:49 +0200 Subject: [PATCH 28/37] Print warning when giving --track and --no-track --- src/grm/main.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/grm/main.rs b/src/grm/main.rs index 2c46f11..f502bd2 100644 --- a/src/grm/main.rs +++ b/src/grm/main.rs @@ -483,6 +483,9 @@ fn main() { match args.action { cmd::WorktreeAction::Add(action_args) => { + if action_args.track.is_some() && action_args.no_track { + print_warning("You are using --track and --no-track at the same time. --track will be ignored"); + } let track = match &action_args.track { Some(branch) => { let split = branch.split_once('/'); From ee44fa40fd4da07946c47fb9d6b4c9db059b521d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 19:18:07 +0200 Subject: [PATCH 29/37] Add method to get owned commit of branch --- src/repo.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/repo.rs b/src/repo.rs index f6b92d5..206b1fe 100644 --- a/src/repo.rs +++ b/src/repo.rs @@ -1435,7 +1435,7 @@ impl<'a> Branch<'a> { } } -impl Branch<'_> { +impl<'a> Branch<'a> { pub fn commit(&self) -> Result { Ok(Commit( self.0 @@ -1445,6 +1445,15 @@ impl Branch<'_> { )) } + pub fn commit_owned(self) -> Result, String> { + Ok(Commit( + self.0 + .into_reference() + .peel_to_commit() + .map_err(convert_libgit2_error)?, + )) + } + pub fn set_upstream(&mut self, remote_name: &str, branch_name: &str) -> Result<(), String> { self.0 .set_upstream(Some(&format!("{}/{}", remote_name, branch_name))) From f02719189601e0e3f3aa77dc3d240a5e4ed5655f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Thu, 23 Jun 2022 19:19:28 +0200 Subject: [PATCH 30/37] Update worktree handling That's a big one, see the module-level comment for details. --- src/grm/main.rs | 9 +- src/worktree.rs | 972 ++++++++++++++++++++++++++++++++++++------------ 2 files changed, 747 insertions(+), 234 deletions(-) diff --git a/src/grm/main.rs b/src/grm/main.rs index f502bd2..d3748cc 100644 --- a/src/grm/main.rs +++ b/src/grm/main.rs @@ -513,7 +513,14 @@ fn main() { track, action_args.no_track, ) { - Ok(_) => print_success(&format!("Worktree {} created", &action_args.name)), + Ok(warnings) => { + if let Some(warnings) = warnings { + for warning in warnings { + print_warning(&warning); + } + } + print_success(&format!("Worktree {} created", &action_args.name)); + } Err(error) => { print_error(&format!("Error creating worktree: {}", error)); process::exit(1); diff --git a/src/worktree.rs b/src/worktree.rs index 631aa79..abbdf61 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -1,30 +1,546 @@ +//! This handles worktrees for repositories. Some considerations to take care +//! of: +//! +//! * Which branch to check out / create +//! * Which commit to check out +//! * Whether to track a remote branch, and which +//! +//! There are a general rules. The main goal is to do the least surprising thing +//! in each situation, and to never change existing setups (e.g. tracking, +//! branch states) except when explicitly told to. In 99% of all cases, the +//! workflow will be quite straightforward. +//! +//! * The name of the worktree (and therefore the path) is **always** the same +//! as the name of the branch. +//! * Never modify existing local branches +//! * Only modify tracking branches for existing local branches if explicitly +//! requested +//! * By default, do not do remote operations. This means that we do no do any +//! tracking setup (but of course, the local branch can already have a +//! tracking branch set up, which will just be left alone) +//! * Be quite lax with finding a remote tracking branch (as using an existing +//! branch is most likely preferred to creating a new branch) +//! +//! There are a few different options that can be given: +//! +//! * Explicit track (`--track`) and explicit no-track (`--no-track`) +//! * A configuration may specify to enable tracking a remote branch by default +//! * A configuration may specify a prefix for remote branches +//! +//! # How to handle the local branch? +//! +//! That one is easy: If a branch with the desired name already exists, all is +//! well. If not, we create a new one. +//! +//! # Which commit should be checked out? +//! +//! The most imporant rule: If the local branch already existed, just leave it +//! as it is. Only if a new branch is created do we need to answer the question +//! which commit to set it to. Generally, we set the branch to whatever the +//! "default" branch of the repository is (something like "main" or "master"). +//! But there are a few cases where we can use remote branches to make the +//! result less surprising. +//! +//! First, if tracking is explicitly disabled, we still try to guess! But we +//! *do* ignore `--track`, as this is how it's done everywhere else. +//! +//! As an example: If `origin/foobar` exists and we run `grm worktree add foobar +//! --no-track`, we create a new worktree called `foobar` that's on the same +//! state as `origin/foobar` (but we will not set up tracking, see below). +//! +//! If tracking is explicitly requested to a certain state, we use that remote +//! branch. If it exists, easy. If not, no more guessing! +//! +//! Now, it's important to select the correct remote. In the easiest case, there +//! is only one remote, so we just use that one. If there is more than one +//! remote, we check whether there is a default remote configured via +//! `track.default_remote`. If yes, we use that one. If not, we have to do the +//! selection process below *for each of them*. If only one of them returns +//! some branch to track, we use that one. If more than one remote returns +//! information, we only use it if it's identical for each. Otherwise we bail, +//! as there is no point in guessing. +//! +//! The commit selection process looks like this: +//! +//! * If a prefix is specified in the configuration, we look for +//! `{remote}/{prefix}/{worktree_name}` +//! +//! * We look for `{remote}/{worktree_name}` (yes, this means that even when a +//! prefix is configured, we use a branch *without* a prefix if one with +//! prefix does not exist) +//! +//! Note that we may select different branches for different remotes when +//! prefixes is used. If remote1 has a branch with a prefix and remote2 only has +//! a branch *without* a prefix, we select them both when a prefix is used. This +//! could lead to the following situation: +//! +//! * There is `origin/prefix/foobar` and `remote2/foobar`, with different +//! states +//! * You set `track.default_prefix = "prefix"` (and no default remote!) +//! * You run `grm worktree add `prefix/foobar` +//! * Instead of just picking `origin/prefix/foobar`, grm will complain because +//! it also selected `remote2/foobar`. +//! +//! This is just emergent behaviour of the logic above. Fixing it would require +//! additional logic for that edge case. I assume that it's just so rare to get +//! that behaviour that it's acceptable for now. +//! +//! Now we either have a commit, we aborted, or we do not have commit. In the +//! last case, as stated above, we check out the "default" branch. +//! +//! # The remote tracking branch +//! +//! First, the only remote operations we do is branch creation! It's +//! unfortunately not possible to defer remote branch creation until the first +//! `git push`, which would be ideal. The remote tracking branch has to already +//! exist, so we have to do the equivalent of `git push --set-upstream` during +//! worktree creation. +//! +//! Whether (and which) remote branch to track works like this: +//! +//! * If `--no-track` is given, we never track a remote branch, except when +//! branch already has a tracking branch. So we'd be done already! +//! +//! * If `--track` is given, we always track this branch, regardless of anything +//! else. If the branch exists, cool, otherwise we create it. +//! +//! If neither is given, we only set up tracking if requested in the +//! configuration file (`track.default = true`) +//! +//! The rest of the process is similar to the commit selection above. The only +//! difference is the remote selection. If there is only one, we use it, as +//! before. Otherwise, we try to use `default_remote` from the configuration, if +//! available. If not, we do not set up a remote tracking branch. It works like +//! this: +//! +//! * If a prefix is specified in the configuration, we use +//! `{remote}/{prefix}/{worktree_name}` +//! +//! * If no prefix is specified in the configuration, we use +//! `{remote}/{worktree_name}` +//! +//! Now that we have a remote, we use the same process as above: +//! +//! * If a prefix is specified in the configuration, we use for +//! `{remote}/{prefix}/{worktree_name}` +//! * We use for `{remote}/{worktree_name}` +//! +//! --- +//! +//! All this means that in some weird situation, you may end up with the state +//! of a remote branch while not actually tracking that branch. This can only +//! happen in repositories with more than one remote. Imagine the following: +//! +//! The repository has two remotes (`remote1` and `remote2`) which have the +//! exact same remote state. But there is no `default_remote` in the +//! configuration (or no configuration at all). There is a remote branch +//! `foobar`. As both `remote1/foobar` and `remote2/foobar` as the same, the new +//! worktree will use that as the state of the new branch. But as `grm` cannot +//! tell which remote branch to track, it will not set up remote tracking. This +//! behaviour may be a bit confusing, but first, there is no good way to resolve +//! this, and second, the situation should be really rare (when having multiple +//! remotes, you would generally have a `default_remote` configured). +//! +//! # Implementation +//! +//! To reduce the chance of bugs, the implementation uses the [typestate +//! pattern](http://cliffle.com/blog/rust-typestate/). Here are the states we +//! are moving through linearily: +//! +//! * Init +//! * A local branch name is set +//! * A local commit to set the new branch to is selected +//! * A remote tracking branch is selected +//! * The new branch is created with all the required settings +//! +//! Don't worry about the lifetime stuff: There is only one single lifetime, as +//! everything (branches, commits) is derived from the single repo::Repo +//! instance +//! +//! # Testing +//! +//! There are two types of input to the tests: +//! +//! 1) The parameters passed to `grm`, either via command line or via +//! configuration file +//! 2) The circumstances in the repository and remotes +//! +//! ## Parameters +//! +//! * The name of the worktree +//! * Whether it contains slashes or not +//! * Whether it is invalid +//! * `--track` and `--no-track` +//! * Whether there is a configuration file and what it contains +//! * Whether `track.default` is enabled or disabled +//! * Whether `track.default_remote_prefix` is there or missing +//! * Whether `track.default_remote` is there or missing +//! * Whether that remote exists or not +//! +//! ## Situations +//! +//! ### The local branch +//! +//! * Whether the branch already exists +//! * Whether the branch has a remote tracking branch and whether it differs +//! from the desired tracking branch (i.e. `--track` or config) +//! +//! ### Remotes +//! +//! * How many remotes there are, if any +//! * If more than two remotes exist, whether their desired tracking branch +//! differs +//! +//! ### The remote tracking branch branch +//! +//! * Whether a remote branch with the same name as the worktree exists +//! * Whether a remote branch with the same name as the worktree plus prefix +//! exists +//! +//! ## Outcomes +//! +//! We have to check the following afterwards: +//! +//! * Does the worktree exist in the correct location? +//! * Does the local branch have the same name as the worktree? +//! * Does the local branch have the correct commit? +//! * Does the local branch track the correct remote branch? +//! * Does that remote branch also exist? +use std::cell::RefCell; use std::path::Path; -use super::output::*; +// use super::output::*; use super::repo; pub const GIT_MAIN_WORKTREE_DIRECTORY: &str = ".git-main-working-tree"; -// The logic about the base branch and the tracking branch is as follows: -// -// * If a branch with the same name does not exist and no track is given, use the default -// branch -// -// * If a branch with the same name exists and no track is given, use that -// -// * If a branch with the same name does not exist and track is given, use the -// local branch that tracks that branch -// -// * If a branch with the same name exists and track is given, use the locally -// existing branch. If the locally existing branch is not the local branch to -// the remote tracking branch, issue a warning -pub fn add_worktree( - directory: &Path, - name: &str, - track: Option<(&str, &str)>, - no_track: bool, -) -> Result<(), String> { - // A branch name must never start or end with a slash. Everything else is ok. +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn invalid_worktree_names() { + assert!(add_worktree(Path::new("/tmp/"), "/leadingslash", None, false).is_err()); + assert!(add_worktree(Path::new("/tmp/"), "trailingslash/", None, false).is_err()); + assert!(add_worktree(Path::new("/tmp/"), "//", None, false).is_err()); + assert!(add_worktree(Path::new("/tmp/"), "test//test", None, false).is_err()); + assert!(add_worktree(Path::new("/tmp/"), "test test", None, false).is_err()); + assert!(add_worktree(Path::new("/tmp/"), "test\ttest", None, false).is_err()); + } +} + +struct Init; + +struct WithLocalBranchName<'a> { + local_branch_name: String, + /// Outer option: Is there a computed value? + /// Inner option: Is there actually a branch? + /// + /// None => No computed value yet + /// Some(None) => No branch + /// Some(Some(_)) => Branch + local_branch: RefCell>>>, +} + +struct WithLocalTargetSelected<'a> { + local_branch_name: String, + local_branch: Option>, + target_commit: Option>>, +} + +struct WithRemoteTrackingBranch<'a> { + local_branch_name: String, + local_branch: Option>, + target_commit: Option>>, + remote_tracking_branch: Option<(String, String)>, + prefix: Option, +} + +struct Worktree<'a, S: WorktreeState> { + repo: &'a repo::RepoHandle, + extra: S, +} + +impl<'a> WithLocalBranchName<'a> { + fn new(name: String) -> Self { + Self { + local_branch_name: name, + local_branch: RefCell::new(None), + } + } +} + +trait WorktreeState {} + +impl WorktreeState for Init {} +impl<'a> WorktreeState for WithLocalBranchName<'a> {} +impl<'a> WorktreeState for WithLocalTargetSelected<'a> {} +impl<'a> WorktreeState for WithRemoteTrackingBranch<'a> {} + +impl<'a> Worktree<'a, Init> { + fn new(repo: &'a repo::RepoHandle) -> Self { + Self { + repo, + extra: Init {}, + } + } + + fn set_local_branch_name(self, name: &str) -> Worktree<'a, WithLocalBranchName<'a>> { + Worktree:: { + repo: self.repo, + extra: WithLocalBranchName::new(name.to_string()), + } + } +} + +impl<'a, 'b> Worktree<'a, WithLocalBranchName<'b>> +where + 'a: 'b, +{ + fn check_local_branch(&self) { + let mut branchref = self.extra.local_branch.borrow_mut(); + if branchref.is_none() { + let branch = self.repo.find_local_branch(&self.extra.local_branch_name); + *branchref = Some(if let Ok(branch) = branch { + Some(branch) + } else { + None + }); + } + } + + fn local_branch_already_exists(&self) -> bool { + if let Some(branch) = &*self.extra.local_branch.borrow() { + return branch.is_some(); + } + self.check_local_branch(); + // As we just called `check_local_branch`, we can be sure that + // `self.extra.local_branch` is set to some `Some` value + (*self.extra.local_branch.borrow()) + .as_ref() + .unwrap() + .is_some() + } + + fn select_commit( + self, + commit: Option>>, + ) -> Worktree<'a, WithLocalTargetSelected<'b>> { + self.check_local_branch(); + + Worktree::<'a, WithLocalTargetSelected> { + repo: self.repo, + extra: WithLocalTargetSelected::<'b> { + local_branch_name: self.extra.local_branch_name, + // As we just called `check_local_branch`, we can be sure that + // `self.extra.local_branch` is set to some `Some` value + local_branch: self.extra.local_branch.into_inner().unwrap(), + target_commit: commit, + }, + } + } +} + +impl<'a> Worktree<'a, WithLocalTargetSelected<'a>> { + fn set_remote_tracking_branch( + self, + branch: Option<(&str, &str)>, + prefix: Option<&str>, + ) -> Worktree<'a, WithRemoteTrackingBranch<'a>> { + Worktree:: { + repo: self.repo, + extra: WithRemoteTrackingBranch { + local_branch_name: self.extra.local_branch_name, + local_branch: self.extra.local_branch, + target_commit: self.extra.target_commit, + remote_tracking_branch: branch.map(|(s1, s2)| (s1.to_string(), s2.to_string())), + prefix: prefix.map(|prefix| prefix.to_string()), + }, + } + } +} + +impl<'a> Worktree<'a, WithRemoteTrackingBranch<'a>> { + fn create(self, directory: &Path) -> Result>, String> { + let mut warnings: Vec = vec![]; + + let mut branch = if let Some(branch) = self.extra.local_branch { + branch + } else { + self.repo.create_branch( + &self.extra.local_branch_name, + // TECHDEBT + // We must not call this with `Some()` without a valid target. + // I'm sure this can be improved, just not sure how. + &*self.extra.target_commit.unwrap(), + )? + }; + + if let Some((remote_name, remote_branch_name)) = self.extra.remote_tracking_branch { + let remote_branch_with_prefix = if let Some(ref prefix) = self.extra.prefix { + if let Ok(remote_branch) = self + .repo + .find_remote_branch(&remote_name, &format!("{prefix}/{remote_branch_name}")) + { + Some(remote_branch) + } else { + None + } + } else { + None + }; + + let remote_branch_without_prefix = if let Ok(remote_branch) = self + .repo + .find_remote_branch(&remote_name, &remote_branch_name) + { + Some(remote_branch) + } else { + None + }; + + let remote_branch = if let Some(ref _prefix) = self.extra.prefix { + remote_branch_with_prefix + } else { + remote_branch_without_prefix + }; + + match remote_branch { + Some(remote_branch) => { + if branch.commit()?.id().hex_string() + != remote_branch.commit()?.id().hex_string() + { + warnings.push(format!("The local branch \"{}\" and the remote branch \"{}/{}\" differ. Make sure to push/pull afterwards!", &self.extra.local_branch_name, &remote_name, &remote_branch_name)); + } + + branch.set_upstream(&remote_name, &remote_branch.basename()?)?; + } + None => { + let mut remote = match self.repo.find_remote(&remote_name)? { + Some(remote) => remote, + None => return Err(format!("Remote \"{remote_name}\" not found")), + }; + + if !remote.is_pushable()? { + return Err(format!( + "Cannot push to non-pushable remote \"{remote_name}\"" + )); + } + + if let Some(prefix) = self.extra.prefix { + remote.push( + &self.extra.local_branch_name, + &format!("{}/{}", prefix, remote_branch_name), + self.repo, + )?; + + branch.set_upstream( + &remote_name, + &format!("{}/{}", prefix, remote_branch_name), + )?; + } else { + remote.push( + &self.extra.local_branch_name, + &remote_branch_name, + self.repo, + )?; + + branch.set_upstream(&remote_name, &remote_branch_name)?; + } + } + } + } + + // We have to create subdirectories first, otherwise adding the worktree + // will fail + if self.extra.local_branch_name.contains('/') { + let path = Path::new(&self.extra.local_branch_name); + if let Some(base) = path.parent() { + // This is a workaround of a bug in libgit2 (?) + // + // When *not* doing this, we will receive an error from the `Repository::worktree()` + // like this: + // + // > failed to make directory '/{repo}/.git-main-working-tree/worktrees/dir/test + // + // This is a discrepancy between the behaviour of libgit2 and the + // git CLI when creating worktrees with slashes: + // + // The git CLI will create the worktree's configuration directory + // inside {git_dir}/worktrees/{last_path_component}. Look at this: + // + // ``` + // $ git worktree add 1/2/3 -b 1/2/3 + // $ ls .git/worktrees + // 3 + // ``` + // + // Interesting: When adding a worktree with a different name but the + // same final path component, git starts adding a counter suffix to + // the worktree directories: + // + // ``` + // $ git worktree add 1/3/3 -b 1/3/3 + // $ git worktree add 1/4/3 -b 1/4/3 + // $ ls .git/worktrees + // 3 + // 31 + // 32 + // ``` + // + // I *guess* that the mapping back from the worktree directory under .git to the actual + // worktree directory is done via the `gitdir` file inside `.git/worktrees/{worktree}. + // This means that the actual directory would not matter. You can verify this by + // just renaming it: + // + // ``` + // $ mv .git/worktrees/3 .git/worktrees/foobar + // $ git worktree list + // /tmp/ fcc8a2a7 [master] + // /tmp/1/2/3 fcc8a2a7 [1/2/3] + // /tmp/1/3/3 fcc8a2a7 [1/3/3] + // /tmp/1/4/3 fcc8a2a7 [1/4/3] + // ``` + // + // => Still works + // + // Anyway, libgit2 does not do this: It tries to create the worktree + // directory inside .git with the exact name of the worktree, including + // any slashes. It should be this code: + // + // https://github.com/libgit2/libgit2/blob/f98dd5438f8d7bfd557b612fdf1605b1c3fb8eaf/src/libgit2/worktree.c#L346 + // + // As a workaround, we can create the base directory manually for now. + // + // Tracking upstream issue: https://github.com/libgit2/libgit2/issues/6327 + std::fs::create_dir_all( + directory + .join(GIT_MAIN_WORKTREE_DIRECTORY) + .join("worktrees") + .join(base), + ) + .map_err(|error| error.to_string())?; + std::fs::create_dir_all(base).map_err(|error| error.to_string())?; + } + } + + self.repo.new_worktree( + &self.extra.local_branch_name, + &directory.join(&self.extra.local_branch_name), + &branch, + )?; + + Ok(if warnings.is_empty() { + None + } else { + Some(warnings) + }) + } +} + +/// A branch name must never start or end with a slash, and it cannot have two +/// consecutive slashes +fn validate_worktree_name(name: &str) -> Result<(), String> { if name.starts_with('/') || name.ends_with('/') { return Err(format!( "Invalid worktree name: {}. It cannot start or end with a slash", @@ -32,6 +548,37 @@ pub fn add_worktree( )); } + if name.contains("//") { + return Err(format!( + "Invalid worktree name: {}. It cannot contain two consecutive slashes", + name + )); + } + + if name.contains(char::is_whitespace) { + return Err(format!( + "Invalid worktree name: {}. It cannot contain whitespace", + name + )); + } + + Ok(()) +} + +// TECHDEBT +// +// Instead of opening the repo & reading configuration inside the function, it +// should be done by the caller and given as a parameter +pub fn add_worktree( + directory: &Path, + name: &str, + track: Option<(&str, &str)>, + no_track: bool, +) -> Result>, String> { + let mut warnings: Vec = vec![]; + + validate_worktree_name(name)?; + let repo = repo::RepoHandle::open(directory, true).map_err(|error| match error.kind { repo::RepoErrorKind::NotFound => { String::from("Current directory does not contain a worktree setup") @@ -39,236 +586,195 @@ pub fn add_worktree( _ => format!("Error opening repo: {}", error), })?; + let remotes = &repo.remotes()?; + let config = repo::read_worktree_root_config(directory)?; if repo.find_worktree(name).is_ok() { return Err(format!("Worktree {} already exists", &name)); } - let mut remote_branch_exists = false; + let track_config = config.and_then(|config| config.track); + let prefix = track_config + .as_ref() + .and_then(|track| track.default_remote_prefix.as_ref()); + let enable_tracking = track_config.as_ref().map_or(false, |track| track.default); + let default_remote = track_config + .as_ref() + .map(|track| track.default_remote.clone()); - let mut target_branch = match repo.find_local_branch(name) { - Ok(branchref) => { - if !no_track { - if let Some((remote_name, remote_branch_name)) = track { - let remote_branch = repo.find_remote_branch(remote_name, remote_branch_name); - if let Ok(remote_branch) = remote_branch { - remote_branch_exists = true; - if let Ok(local_upstream_branch) = branchref.upstream() { - if remote_branch.name()? != local_upstream_branch.name()? { - print_warning(&format!( - "You specified a tracking branch ({}/{}) for an existing branch ({}), but \ - it differs from the current upstream ({}). Will keep current upstream" - , remote_name, remote_branch_name, branchref.name()?, local_upstream_branch.name()?)) - } - } - } - } - } - branchref - } - Err(_) => { - let default_checkout = || repo.default_branch()?.to_commit(); + // Note that we have to define all variables that borrow from `repo` + // *first*, otherwise we'll receive "borrowed value does not live long + // enough" errors. This is due to the `repo` reference inside `Worktree` that is + // passed through each state type. + // + // The `commit` variable will be dropped at the end of the scope, together with all + // worktree variables. It will be done in the opposite direction of delcaration (FILO). + // + // So if we define `commit` *after* the respective worktrees, it will be dropped first while + // still being borrowed by `Worktree`. + let default_branch_head = repo.default_branch()?.commit_owned()?; - let checkout_commit; + let worktree = Worktree::::new(&repo).set_local_branch_name(name); - if no_track { - checkout_commit = default_checkout()?; - } else { - match track { - Some((remote_name, remote_branch_name)) => { - let remote_branch = - repo.find_remote_branch(remote_name, remote_branch_name); - match remote_branch { - Ok(branch) => { - remote_branch_exists = true; - checkout_commit = branch.to_commit()?; - } - Err(_) => { - remote_branch_exists = false; - checkout_commit = default_checkout()?; - } - } - } - None => match &config { - None => checkout_commit = default_checkout()?, - Some(config) => match &config.track { - None => checkout_commit = default_checkout()?, - Some(track_config) => { - if track_config.default { - let remote_branch = - repo.find_remote_branch(&track_config.default_remote, name); - match remote_branch { - Ok(branch) => { - remote_branch_exists = true; - checkout_commit = branch.to_commit()?; - } - Err(_) => { - checkout_commit = default_checkout()?; - } - } - } else { - checkout_commit = default_checkout()?; - } - } - }, - }, - }; - } - - repo.create_branch(name, &checkout_commit)? + let get_remote_head = |remote_name: &str, + remote_branch_name: &str| + -> Result>, String> { + if let Ok(remote_branch) = repo.find_remote_branch(remote_name, remote_branch_name) { + Ok(Some(Box::new(remote_branch.commit_owned()?))) + } else { + Ok(None) } }; - fn push( - remote: &mut repo::RemoteHandle, - branch_name: &str, - remote_branch_name: &str, - repo: &repo::RepoHandle, - ) -> Result<(), String> { - if !remote.is_pushable()? { - return Err(format!( - "Cannot push to non-pushable remote {}", - remote.url() - )); + let worktree = if worktree.local_branch_already_exists() { + worktree.select_commit(None) + } else if let Some((remote_name, remote_branch_name)) = if no_track { None } else { track } { + if let Ok(remote_branch) = repo.find_remote_branch(remote_name, remote_branch_name) { + worktree.select_commit(Some(Box::new(remote_branch.commit_owned()?))) + } else { + worktree.select_commit(Some(Box::new(default_branch_head))) } - remote.push(branch_name, remote_branch_name, repo) - } - - if !no_track { - if let Some((remote_name, remote_branch_name)) = track { - if remote_branch_exists { - target_branch.set_upstream(remote_name, remote_branch_name)?; - } else { - let mut remote = repo - .find_remote(remote_name) - .map_err(|error| format!("Error getting remote {}: {}", remote_name, error))? - .ok_or_else(|| format!("Remote {} not found", remote_name))?; - - push( - &mut remote, - &target_branch.name()?, - remote_branch_name, - &repo, - )?; - - target_branch.set_upstream(remote_name, remote_branch_name)?; - } - } else if let Some(config) = config { - if let Some(track_config) = config.track { - if track_config.default { - let remote_name = track_config.default_remote; - if remote_branch_exists { - target_branch.set_upstream(&remote_name, name)?; + } else { + match remotes.len() { + 0 => worktree.select_commit(Some(Box::new(default_branch_head))), + 1 => { + let remote_name = &remotes[0]; + let commit: Option> = ({ + if let Some(prefix) = prefix { + get_remote_head(remote_name, &format!("{prefix}/{name}"))? } else { - let remote_branch_name = match track_config.default_remote_prefix { - Some(prefix) => { - format!("{}{}{}", &prefix, super::BRANCH_NAMESPACE_SEPARATOR, &name) - } - None => name.to_string(), - }; - - let mut remote = repo - .find_remote(&remote_name) - .map_err(|error| { - format!("Error getting remote {}: {}", remote_name, error) - })? - .ok_or_else(|| format!("Remote {} not found", remote_name))?; - - if !remote.is_pushable()? { - return Err(format!( - "Cannot push to non-pushable remote {}", - remote.url() - )); - } - push( - &mut remote, - &target_branch.name()?, - &remote_branch_name, - &repo, - )?; - - target_branch.set_upstream(&remote_name, &remote_branch_name)?; + None } + }) + .or(get_remote_head(remote_name, name)?) + .or_else(|| Some(Box::new(default_branch_head))); + + worktree.select_commit(commit) + } + _ => { + let commit = if let Some(ref default_remote) = default_remote { + if let Some(ref prefix) = prefix { + if let Ok(remote_branch) = repo + .find_remote_branch(default_remote, &format!("{prefix}/{name}")) + { + Some(Box::new(remote_branch.commit_owned()?)) + } else { + None + } + } else { + None + } + .or({ + if let Ok(remote_branch) = + repo.find_remote_branch(default_remote, name) + { + Some(Box::new(remote_branch.commit_owned()?)) + } else { + None + } + }) + } else { + None + }.or({ + let mut commits = vec![]; + for remote_name in remotes.iter() { + let remote_head: Option> = ({ + if let Some(ref prefix) = prefix { + if let Ok(remote_branch) = repo.find_remote_branch( + remote_name, + &format!("{prefix}/{name}"), + ) { + Some(Box::new(remote_branch.commit_owned()?)) + } else { + None + } + } else { + None + } + }) + .or({ + if let Ok(remote_branch) = + repo.find_remote_branch(remote_name, name) + { + Some(Box::new(remote_branch.commit_owned()?)) + } else { + None + } + }) + .or(None); + commits.push(remote_head); + } + + let mut commits = commits + .into_iter() + .flatten() + // have to collect first because the `flatten()` return + // typedoes not implement `windows()` + .collect::>>(); + // `flatten()` takes care of `None` values here. If all + // remotes return None for the branch, we do *not* abort, we + // continue! + if commits.is_empty() { + Some(Box::new(default_branch_head)) + } else if commits.len() == 1 { + Some(commits.swap_remove(0)) + } else if commits.windows(2).any(|window| { + let c1 = &window[0]; + let c2 = &window[1]; + (*c1).id().hex_string() != (*c2).id().hex_string() + }) { + warnings.push( + // TODO this should also include the branch + // name. BUT: the branch name may be different + // between the remotes. Let's just leave it + // until I get around to fix that inconsistency + // (see module-level doc about), which might be + // never, as it's such a rare edge case. + "Branch exists on multiple remotes, but they deviate. Selecting default branch instead".to_string() + ); + Some(Box::new(default_branch_head)) + } else { + Some(commits.swap_remove(0)) + } + }); + worktree.select_commit(commit) + } + } + }; + + let worktree = if no_track { + worktree.set_remote_tracking_branch(None, prefix.map(|s| s.as_str())) + } else if let Some((remote_name, remote_branch_name)) = track { + worktree.set_remote_tracking_branch( + Some((remote_name, remote_branch_name)), + None, // Always disable prefixing when explicitly given --track + ) + } else if !enable_tracking { + worktree.set_remote_tracking_branch(None, prefix.map(|s| s.as_str())) + } else { + match remotes.len() { + 0 => worktree.set_remote_tracking_branch(None, prefix.map(|s| s.as_str())), + 1 => worktree + .set_remote_tracking_branch(Some((&remotes[0], name)), prefix.map(|s| s.as_str())), + _ => { + if let Some(default_remote) = default_remote { + worktree.set_remote_tracking_branch( + Some((&default_remote, name)), + prefix.map(|s| s.as_str()), + ) + } else { + worktree.set_remote_tracking_branch(None, prefix.map(|s| s.as_str())) } } } - } + }; - // We have to create subdirectories first, otherwise adding the worktree - // will fail - if name.contains('/') { - let path = Path::new(&name); - if let Some(base) = path.parent() { - // This is a workaround of a bug in libgit2 (?) - // - // When *not* doing this, we will receive an error from the `Repository::worktree()` - // like this: - // - // > failed to make directory '/{repo}/.git-main-working-tree/worktrees/dir/test - // - // This is a discrepancy between the behaviour of libgit2 and the - // git CLI when creating worktrees with slashes: - // - // The git CLI will create the worktree's configuration directory - // inside {git_dir}/worktrees/{last_path_component}. Look at this: - // - // ``` - // $ git worktree add 1/2/3 -b 1/2/3 - // $ ls .git/worktrees - // 3 - // ``` - // - // Interesting: When adding a worktree with a different name but the - // same final path component, git starts adding a counter suffix to - // the worktree directories: - // - // ``` - // $ git worktree add 1/3/3 -b 1/3/3 - // $ git worktree add 1/4/3 -b 1/4/3 - // $ ls .git/worktrees - // 3 - // 31 - // 32 - // ``` - // - // I *guess* that the mapping back from the worktree directory under .git to the actual - // worktree directory is done via the `gitdir` file inside `.git/worktrees/{worktree}. - // This means that the actual directory would not matter. You can verify this by - // just renaming it: - // - // ``` - // $ mv .git/worktrees/3 .git/worktrees/foobar - // $ git worktree list - // /tmp/ fcc8a2a7 [master] - // /tmp/1/2/3 fcc8a2a7 [1/2/3] - // /tmp/1/3/3 fcc8a2a7 [1/3/3] - // /tmp/1/4/3 fcc8a2a7 [1/4/3] - // ``` - // - // => Still works - // - // Anyway, libgit2 does not do this: It tries to create the worktree - // directory inside .git with the exact name of the worktree, including - // any slashes. It should be this code: - // - // https://github.com/libgit2/libgit2/blob/f98dd5438f8d7bfd557b612fdf1605b1c3fb8eaf/src/libgit2/worktree.c#L346 - // - // As a workaround, we can create the base directory manually for now. - // - // Tracking upstream issue: https://github.com/libgit2/libgit2/issues/6327 - std::fs::create_dir_all( - directory - .join(GIT_MAIN_WORKTREE_DIRECTORY) - .join("worktrees") - .join(base), - ) - .map_err(|error| error.to_string())?; - std::fs::create_dir_all(base).map_err(|error| error.to_string())?; - } - } + worktree.create(directory)?; - repo.new_worktree(name, &directory.join(&name), &target_branch)?; - - Ok(()) + Ok(if warnings.is_empty() { + None + } else { + Some(warnings) + }) } From 512de5e187016d505442ba6e302ba1f82bdaedff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Wed, 29 Jun 2022 22:47:04 +0200 Subject: [PATCH 31/37] e2e: Reduce number of tests by removing redundant ones --- e2e_tests/test_worktrees.py | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/e2e_tests/test_worktrees.py b/e2e_tests/test_worktrees.py index b2d21a3..f008330 100644 --- a/e2e_tests/test_worktrees.py +++ b/e2e_tests/test_worktrees.py @@ -9,13 +9,21 @@ import datetime import os.path -@pytest.mark.parametrize("config_enabled", [True, False]) -@pytest.mark.parametrize("config_has_default_remote_prefix", [True, False]) -@pytest.mark.parametrize("config_has_default_track_enabled", [True, False]) +@pytest.mark.parametrize( + "config_setup", + ( + (False, False, False), + (True, False, False), + (True, False, True), + (True, True, False), + (True, True, True), + ), +) @pytest.mark.parametrize("explicit_notrack", [True, False]) @pytest.mark.parametrize("explicit_track", [True, False]) -@pytest.mark.parametrize("local_branch_exists", [True, False]) -@pytest.mark.parametrize("local_branch_has_tracking_branch", [True, False]) +@pytest.mark.parametrize( + "local_branch_setup", ((False, False), (True, False), (True, True)) +) @pytest.mark.parametrize("remote_branch_already_exists", [True, False]) @pytest.mark.parametrize("remote_branch_with_prefix_already_exists", [True, False]) @pytest.mark.parametrize( @@ -32,13 +40,10 @@ import os.path @pytest.mark.parametrize("track_differs_from_existing_branch_upstream", [True, False]) @pytest.mark.parametrize("worktree_with_slash", [True, False]) def test_worktree_add( - config_enabled, - config_has_default_remote_prefix, - config_has_default_track_enabled, + config_setup, explicit_notrack, explicit_track, - local_branch_exists, - local_branch_has_tracking_branch, + local_branch_setup, remote_branch_already_exists, remote_branch_with_prefix_already_exists, remote_setup, @@ -46,6 +51,12 @@ def test_worktree_add( worktree_with_slash, ): (remote_count, default_remote, remotes_differ) = remote_setup + ( + config_enabled, + config_has_default_remote_prefix, + config_has_default_track_enabled, + ) = config_setup + (local_branch_exists, local_branch_has_tracking_branch) = local_branch_setup has_remotes = True if remote_count > 0 else False if worktree_with_slash: From 0e9c8d0c0115a6ac619bef53cbc606ca6011f131 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Wed, 29 Jun 2022 23:34:55 +0200 Subject: [PATCH 32/37] dependencies: Update clap to 3.2.7 --- Cargo.lock | 12 ++++++------ Cargo.toml | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 37aeae9..9f0dc68 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -80,9 +80,9 @@ checksum = "baf1de4339761588bc0619e3cbc0120ee582ebb74b53b4efbf79117bd2da40fd" [[package]] name = "clap" -version = "3.2.6" +version = "3.2.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9f1fe12880bae935d142c8702d500c63a4e8634b6c3c57ad72bf978fc7b6249a" +checksum = "5b7b16274bb247b45177db843202209b12191b631a14a9d06e41b3777d6ecf14" dependencies = [ "atty", "bitflags", @@ -97,9 +97,9 @@ dependencies = [ [[package]] name = "clap_derive" -version = "3.2.6" +version = "3.2.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ed6db9e867166a43a53f7199b5e4d1f522a1e5bd626654be263c999ce59df39a" +checksum = "759bf187376e1afa7b85b959e6a664a3e7a95203415dba952ad19139e798f902" dependencies = [ "heck", "proc-macro-error", @@ -110,9 +110,9 @@ dependencies = [ [[package]] name = "clap_lex" -version = "0.2.3" +version = "0.2.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "87eba3c8c7f42ef17f6c659fc7416d0f4758cd3e58861ee63c5fa4a4dde649e4" +checksum = "2850f2f5a82cbf437dd5af4d49848fbdfc27c157c3d010345776f952765261c5" dependencies = [ "os_str_bytes", ] diff --git a/Cargo.toml b/Cargo.toml index ea9a717..49f9edb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,7 +28,7 @@ rust-version = "1.57" license = "GPL-3.0-only" [profile.e2e-tests] -inherits = "release" +inherits = "dev" [lib] name = "grm" @@ -54,7 +54,7 @@ version = "=0.14.4" version = "=2.1.0" [dependencies.clap] -version = "=3.2.6" +version = "=3.2.7" features = ["derive", "cargo"] [dependencies.console] From 4e21a3daadf2cfe54b81a7823b9976359fe644ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Wed, 29 Jun 2022 23:34:55 +0200 Subject: [PATCH 33/37] dependencies: Update serde_json to 1.0.82 --- Cargo.lock | 4 ++-- Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9f0dc68..ff132a5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -895,9 +895,9 @@ dependencies = [ [[package]] name = "serde_json" -version = "1.0.81" +version = "1.0.82" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9b7ce2b32a1aed03c558dc61a5cd328f15aff2dbc17daad8fb8af04d2100e15c" +checksum = "82c2c1fdcd807d1098552c5b9a36e425e42e9fbd7c6a37a8425f390f781f7fa7" dependencies = [ "itoa", "ryu", diff --git a/Cargo.toml b/Cargo.toml index 49f9edb..e6baa45 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -70,7 +70,7 @@ version = "=6.0.0" version = "=0.8.24" [dependencies.serde_json] -version = "=1.0.81" +version = "=1.0.82" [dependencies.isahc] version = "=1.7.2" From 91a37cb12d2106bc0f1d6b22ffa21eb94ed5d66f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Wed, 29 Jun 2022 23:36:46 +0200 Subject: [PATCH 34/37] Cargo.lock: Updating smallvec v1.8.0 -> v1.8.1 --- Cargo.lock | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ff132a5..d780771 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -974,9 +974,9 @@ dependencies = [ [[package]] name = "smallvec" -version = "1.8.0" +version = "1.8.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f2dd574626839106c320a323308629dcb1acfc96e32a8cba364ddc61ac23ee83" +checksum = "cc88c725d61fc6c3132893370cac4a0200e3fedf5da8331c570664b1987f5ca2" [[package]] name = "socket2" @@ -1002,9 +1002,9 @@ checksum = "063e6045c0e62079840579a7e47a355ae92f60eb74daaf156fb1e84ba164e63f" [[package]] name = "strum_macros" -version = "0.24.0" +version = "0.24.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6878079b17446e4d3eba6192bb0a2950d5b14f0ed8424b852310e5a94345d0ef" +checksum = "4faebde00e8ff94316c01800f9054fd2ba77d30d9e922541913051d1d978918b" dependencies = [ "heck", "proc-macro2", From 494c6ecb3e42809d0852e7e03d6d86c31b9a5e60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Wed, 29 Jun 2022 23:36:50 +0200 Subject: [PATCH 35/37] Cargo.lock: Updating linked-hash-map v0.5.4 -> v0.5.6 --- Cargo.lock | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d780771..d0167df 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -535,9 +535,9 @@ dependencies = [ [[package]] name = "linked-hash-map" -version = "0.5.4" +version = "0.5.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7fb9b38af92608140b86b693604b9ffcc5824240a484d1ecd4795bacb2fe88f3" +checksum = "0717cef1bc8b636c6e1c1bbdefc09e6322da8a9321966e8928ef80d20f7f770f" [[package]] name = "lock_api" @@ -1129,9 +1129,9 @@ dependencies = [ [[package]] name = "tracing-core" -version = "0.1.27" +version = "0.1.28" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7709595b8878a4965ce5e87ebf880a7d39c9afc6837721b21a5a816a8117d921" +checksum = "7b7358be39f2f274f322d2aaed611acc57f382e8eb1e5b48cb9ae30933495ce7" dependencies = [ "once_cell", ] @@ -1160,9 +1160,9 @@ checksum = "5bd2fe26506023ed7b5e1e315add59d6f584c621d037f9368fea9cfb988f368c" [[package]] name = "unicode-normalization" -version = "0.1.19" +version = "0.1.20" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d54590932941a9e9266f0832deed84ebe1bf2e4c9e4a3554d393d18f5e854bf9" +checksum = "81dee68f85cab8cf68dec42158baf3a79a1cdc065a8b103025965d6ccb7f6cbd" dependencies = [ "tinyvec", ] From 7d8fbb844ed911674f477d2b4140d8fc3b4ab6e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Wed, 29 Jun 2022 23:40:23 +0200 Subject: [PATCH 36/37] Properly handle deletion of nested worktrees --- e2e_tests/test_worktrees.py | 24 +++++++++++++++ src/grm/main.rs | 5 ++-- src/lib.rs | 2 -- src/repo.rs | 60 +++++++++++++++++++++++++++++++------ 4 files changed, 77 insertions(+), 14 deletions(-) diff --git a/e2e_tests/test_worktrees.py b/e2e_tests/test_worktrees.py index f008330..15f5136 100644 --- a/e2e_tests/test_worktrees.py +++ b/e2e_tests/test_worktrees.py @@ -589,6 +589,30 @@ def test_worktree_delete(): assert "test" not in [str(b) for b in repo.branches] +@pytest.mark.parametrize("has_other_worktree", [True, False]) +def test_worktree_delete_in_subfolder(has_other_worktree): + with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): + cmd = grm(["wt", "add", "dir/test", "--track", "origin/test"], cwd=base_dir) + assert cmd.returncode == 0 + assert "dir" in os.listdir(base_dir) + + if has_other_worktree is True: + cmd = grm( + ["wt", "add", "dir/test2", "--track", "origin/test"], cwd=base_dir + ) + assert cmd.returncode == 0 + assert {"test", "test2"} == set(os.listdir(os.path.join(base_dir, "dir"))) + else: + assert {"test"} == set(os.listdir(os.path.join(base_dir, "dir"))) + + cmd = grm(["wt", "delete", "dir/test"], cwd=base_dir) + assert cmd.returncode == 0 + if has_other_worktree is True: + assert {"test2"} == set(os.listdir(os.path.join(base_dir, "dir"))) + else: + assert "dir" not in os.listdir(base_dir) + + def test_worktree_delete_refusal_no_tracking_branch(): with TempGitRepositoryWorktree.get(funcname()) as (base_dir, _commit): cmd = grm(["wt", "add", "test"], cwd=base_dir) diff --git a/src/grm/main.rs b/src/grm/main.rs index d3748cc..09d142c 100644 --- a/src/grm/main.rs +++ b/src/grm/main.rs @@ -528,8 +528,6 @@ fn main() { } } cmd::WorktreeAction::Delete(action_args) => { - let worktree_dir = cwd.join(&action_args.name); - let worktree_config = match repo::read_worktree_root_config(&cwd) { Ok(config) => config, Err(error) => { @@ -547,8 +545,9 @@ fn main() { }); match repo.remove_worktree( + &cwd, &action_args.name, - &worktree_dir, + Path::new(&action_args.name), action_args.force, &worktree_config, ) { diff --git a/src/lib.rs b/src/lib.rs index d406b95..0c17e62 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -14,8 +14,6 @@ pub mod table; pub mod tree; pub mod worktree; -const BRANCH_NAMESPACE_SEPARATOR: &str = "/"; - /// Find all git repositories under root, recursively /// /// The bool in the return value specifies whether there is a repository diff --git a/src/repo.rs b/src/repo.rs index 206b1fe..494ae62 100644 --- a/src/repo.rs +++ b/src/repo.rs @@ -1153,18 +1153,21 @@ impl RepoHandle { pub fn remove_worktree( &self, + base_dir: &Path, name: &str, worktree_dir: &Path, force: bool, worktree_config: &Option, ) -> Result<(), WorktreeRemoveFailureReason> { - if !worktree_dir.exists() { + let fullpath = base_dir.join(worktree_dir); + + if !fullpath.exists() { return Err(WorktreeRemoveFailureReason::Error(format!( "{} does not exist", name ))); } - let worktree_repo = RepoHandle::open(worktree_dir, false).map_err(|error| { + let worktree_repo = RepoHandle::open(&fullpath, false).map_err(|error| { WorktreeRemoveFailureReason::Error(format!("Error opening repo: {}", error)) })?; @@ -1176,12 +1179,11 @@ impl RepoHandle { WorktreeRemoveFailureReason::Error(format!("Failed getting name of branch: {}", error)) })?; - if branch_name != name - && !branch_name.ends_with(&format!("{}{}", super::BRANCH_NAMESPACE_SEPARATOR, name)) - { + if branch_name != name { return Err(WorktreeRemoveFailureReason::Error(format!( - "Branch \"{}\" is checked out in worktree, this does not look correct", - &branch_name + "Branch \"{}\" is checked out in worktree \"{}\", this does not look correct", + &branch_name, + &worktree_dir.display(), ))); } @@ -1251,13 +1253,47 @@ impl RepoHandle { } } - if let Err(e) = std::fs::remove_dir_all(&worktree_dir) { + // worktree_dir is a relative path, starting from base_dir. We walk it + // upwards (from subdirectory to parent directories) and remove each + // component, in case it is empty. Only the leaf directory can be + // removed unconditionally (as it contains the worktree itself). + if let Err(e) = std::fs::remove_dir_all(&fullpath) { return Err(WorktreeRemoveFailureReason::Error(format!( "Error deleting {}: {}", &worktree_dir.display(), e ))); } + + if let Some(current_dir) = worktree_dir.parent() { + for current_dir in current_dir.ancestors() { + let current_dir = base_dir.join(current_dir); + println!("deleting {}", current_dir.display()); + if current_dir + .read_dir() + .map_err(|error| { + WorktreeRemoveFailureReason::Error(format!( + "Error reading {}: {}", + ¤t_dir.display(), + error + )) + })? + .next() + .is_none() + { + if let Err(e) = std::fs::remove_dir_all(¤t_dir) { + return Err(WorktreeRemoveFailureReason::Error(format!( + "Error deleting {}: {}", + &worktree_dir.display(), + e + ))); + } + } else { + break; + } + } + } + self.prune_worktree(name) .map_err(WorktreeRemoveFailureReason::Error)?; branch @@ -1310,7 +1346,13 @@ impl RepoHandle { { let repo_dir = &directory.join(&worktree.name()); if repo_dir.exists() { - match self.remove_worktree(worktree.name(), repo_dir, false, &config) { + match self.remove_worktree( + directory, + worktree.name(), + Path::new(worktree.name()), + false, + &config, + ) { Ok(_) => print_success(&format!("Worktree {} deleted", &worktree.name())), Err(error) => match error { WorktreeRemoveFailureReason::Changes(changes) => { From fa83063c614a296f8061a77272a7bf12f160f992 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Wed, 29 Jun 2022 23:58:31 +0200 Subject: [PATCH 37/37] Release v0.7.4 --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d0167df..8f83fc5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -332,7 +332,7 @@ dependencies = [ [[package]] name = "git-repo-manager" -version = "0.7.3" +version = "0.7.4" dependencies = [ "clap", "comfy-table", diff --git a/Cargo.toml b/Cargo.toml index e6baa45..97b419b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "git-repo-manager" -version = "0.7.3" +version = "0.7.4" edition = "2021" authors = [