From ef8a57c60e4ecc63c9ad56bd4d67de3eb3662d13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20K=C3=B6rber?= Date: Wed, 29 Dec 2021 19:02:42 +0100 Subject: [PATCH] Add rebase option for worktrees --- docs/src/worktrees.md | 34 ++++ e2e_tests/test_worktree_rebase.py | 250 ++++++++++++++++++++++++++++++ src/grm/cmd.rs | 10 ++ src/grm/main.rs | 70 +++++++++ src/repo.rs | 82 +++++++++- 5 files changed, 445 insertions(+), 1 deletion(-) create mode 100644 e2e_tests/test_worktree_rebase.py diff --git a/docs/src/worktrees.md b/docs/src/worktrees.md index 5c2daff..847cdad 100644 --- a/docs/src/worktrees.md +++ b/docs/src/worktrees.md @@ -312,6 +312,40 @@ grm wt pull --rebase This will rebase your changes onto the upstream branch. This is mainly helpful for persistent branches that change on the remote side. +There is a similar rebase feature that rebases onto the **default** branch instead: + +``` +grm wt rebase +[✔] master: Done +[✔] my-cool-branch: Done +``` + +This is super helpful for feature branches. If you want to incorporate changes +made on the remote branches, use `grm wt rebase` and all your branches will +be up to date. If you want to also update to remote tracking branches in one go, +use the `--pull` flag, and `--rebase` if you want to rebase instead of aborting +on non-fast-forwards: + +``` +grm wt rebase --pull --rebase +[✔] master: Done +[✔] my-cool-branch: Done +``` + +"So, what's the difference between `pull --rebase` and `rebase --pull`? Why the +hell is there a `--rebase` flag in the `rebase` command?" + +Yes, it's kind of weird. Remember that `pull` only ever updates each worktree +to their remote branch, if possible. `rebase` rabases onto the **default** branch +instead. The switches to `rebase` are just convenience, so you do not have to +run two commands. + +* `rebase --pull` is the same as `pull` && `rebase` +* `rebase --pull --rebase` is the same as `pull --rebase` && `rebase` + +I understand that the UX is not the most intuitive. If you can think of an +improvement, please let me know (e.g. via an GitHub issue)! + ### Manual access GRM isn't doing any magic, it's just git under the hood. If you need to have access diff --git a/e2e_tests/test_worktree_rebase.py b/e2e_tests/test_worktree_rebase.py new file mode 100644 index 0000000..c913aaf --- /dev/null +++ b/e2e_tests/test_worktree_rebase.py @@ -0,0 +1,250 @@ +#!/usr/bin/env python3 + +from helpers import * + +import pytest + +import git + + +@pytest.mark.parametrize("pull", [True, False]) +@pytest.mark.parametrize("rebase", [True, False]) +@pytest.mark.parametrize("ffable", [True, False]) +def test_worktree_rebase(pull, rebase, ffable): + with TempGitRepositoryWorktree() as (base_dir, _root_commit): + with open(os.path.join(base_dir, "grm.toml"), "w") as f: + f.write('persistent_branches = ["mybasebranch"]') + + repo = git.Repo(f"{base_dir}/.git-main-working-tree") + + grm( + ["wt", "add", "mybasebranch", "--track", "origin/mybasebranch"], + cwd=base_dir, + ) + + shell( + f""" + cd {base_dir}/mybasebranch + echo change > mychange-root + git add mychange-root + git commit -m "commit-root" + echo change > mychange-base-local + git add mychange-base-local + git commit -m "commit-in-base-local" + git push origin mybasebranch + """ + ) + + grm( + ["wt", "add", "myfeatbranch", "--track", "origin/myfeatbranch"], + cwd=base_dir, + ) + shell( + f""" + cd {base_dir}/myfeatbranch + git reset --hard mybasebranch^ # root + echo change > mychange-feat-local + git add mychange-feat-local + git commit -m "commit-in-feat-local" + git push origin HEAD:myfeatbranch + """ + ) + + grm(["wt", "add", "tmp"], cwd=base_dir) + shell( + f""" + cd {base_dir}/tmp + git reset --hard mybasebranch + echo change > mychange-base-remote + git add mychange-base-remote + git commit -m "commit-in-base-remote" + git push origin HEAD:mybasebranch + + git reset --hard myfeatbranch + echo change > mychange-feat-remote + git add mychange-feat-remote + git commit -m "commit-in-feat-remote" + git push origin HEAD:myfeatbranch + """ + ) + + if not ffable: + shell( + f""" + cd {base_dir}/mybasebranch + echo change > mychange-base-no-ff + git add mychange-base-no-ff + git commit -m "commit-in-base-local-no-ff" + + cd {base_dir}/myfeatbranch + echo change > mychange-feat-no-ff + git add mychange-feat-no-ff + git commit -m "commit-in-feat-local-no-ff" + """ + ) + + grm(["wt", "delete", "--force", "tmp"], cwd=base_dir) + + repo = git.Repo(f"{base_dir}/.git-main-working-tree") + if ffable: + assert repo.commit("mybasebranch~1").message.strip() == "commit-root" + assert ( + repo.refs.mybasebranch.commit.message.strip() == "commit-in-base-local" + ) + assert ( + repo.remote("origin").refs.mybasebranch.commit.message.strip() + == "commit-in-base-remote" + ) + assert ( + repo.refs.myfeatbranch.commit.message.strip() == "commit-in-feat-local" + ) + assert ( + repo.remote("origin").refs.myfeatbranch.commit.message.strip() + == "commit-in-feat-remote" + ) + else: + assert ( + repo.commit("mybasebranch").message.strip() + == "commit-in-base-local-no-ff" + ) + assert ( + repo.commit("mybasebranch~1").message.strip() == "commit-in-base-local" + ) + assert repo.commit("mybasebranch~2").message.strip() == "commit-root" + assert ( + repo.commit("myfeatbranch").message.strip() + == "commit-in-feat-local-no-ff" + ) + assert ( + repo.commit("myfeatbranch~1").message.strip() == "commit-in-feat-local" + ) + assert repo.commit("myfeatbranch~2").message.strip() == "commit-root" + assert ( + repo.remote("origin").refs.mybasebranch.commit.message.strip() + == "commit-in-base-remote" + ) + assert ( + repo.remote("origin").refs.myfeatbranch.commit.message.strip() + == "commit-in-feat-remote" + ) + + args = ["wt", "rebase"] + if pull: + args += ["--pull"] + if rebase: + args += ["--rebase"] + cmd = grm(args, cwd=base_dir) + + print(args) + if rebase and not pull: + assert cmd.returncode != 0 + assert len(cmd.stderr) != 0 + else: + assert cmd.returncode == 0 + repo = git.Repo(f"{base_dir}/myfeatbranch") + if pull: + if rebase: + if ffable: + assert ( + repo.commit("HEAD").message.strip() + == "commit-in-feat-remote" + ) + assert ( + repo.commit("HEAD~1").message.strip() + == "commit-in-feat-local" + ) + assert ( + repo.commit("HEAD~2").message.strip() + == "commit-in-base-remote" + ) + assert ( + repo.commit("HEAD~3").message.strip() + == "commit-in-base-local" + ) + assert repo.commit("HEAD~4").message.strip() == "commit-root" + else: + assert ( + repo.commit("HEAD").message.strip() + == "commit-in-feat-local-no-ff" + ) + assert ( + repo.commit("HEAD~1").message.strip() + == "commit-in-feat-remote" + ) + assert ( + repo.commit("HEAD~2").message.strip() + == "commit-in-feat-local" + ) + assert ( + repo.commit("HEAD~3").message.strip() + == "commit-in-base-local-no-ff" + ) + assert ( + repo.commit("HEAD~4").message.strip() + == "commit-in-base-remote" + ) + assert ( + repo.commit("HEAD~5").message.strip() + == "commit-in-base-local" + ) + assert repo.commit("HEAD~6").message.strip() == "commit-root" + else: + if ffable: + assert ( + repo.commit("HEAD").message.strip() + == "commit-in-feat-remote" + ) + assert ( + repo.commit("HEAD~1").message.strip() + == "commit-in-feat-local" + ) + assert ( + repo.commit("HEAD~2").message.strip() + == "commit-in-base-remote" + ) + assert ( + repo.commit("HEAD~3").message.strip() + == "commit-in-base-local" + ) + assert repo.commit("HEAD~4").message.strip() == "commit-root" + else: + assert ( + repo.commit("HEAD").message.strip() + == "commit-in-feat-local-no-ff" + ) + assert ( + repo.commit("HEAD~1").message.strip() + == "commit-in-feat-local" + ) + assert ( + repo.commit("HEAD~2").message.strip() + == "commit-in-base-local-no-ff" + ) + assert ( + repo.commit("HEAD~3").message.strip() + == "commit-in-base-local" + ) + assert repo.commit("HEAD~4").message.strip() == "commit-root" + else: + if ffable: + assert repo.commit("HEAD").message.strip() == "commit-in-feat-local" + assert ( + repo.commit("HEAD~1").message.strip() == "commit-in-base-local" + ) + assert repo.commit("HEAD~2").message.strip() == "commit-root" + else: + assert ( + repo.commit("HEAD").message.strip() + == "commit-in-feat-local-no-ff" + ) + assert ( + repo.commit("HEAD~1").message.strip() == "commit-in-feat-local" + ) + assert ( + repo.commit("HEAD~2").message.strip() + == "commit-in-base-local-no-ff" + ) + assert ( + repo.commit("HEAD~3").message.strip() == "commit-in-base-local" + ) + assert repo.commit("HEAD~4").message.strip() == "commit-root" diff --git a/src/grm/cmd.rs b/src/grm/cmd.rs index 5d247c7..017357b 100644 --- a/src/grm/cmd.rs +++ b/src/grm/cmd.rs @@ -91,6 +91,8 @@ pub enum WorktreeAction { Fetch(WorktreeFetchArgs), #[clap(about = "Fetch refs from remotes and update local branches")] Pull(WorktreePullArgs), + #[clap(about = "Rebase worktree onto default branch")] + Rebase(WorktreeRebaseArgs), } #[derive(Parser)] @@ -137,6 +139,14 @@ pub struct WorktreePullArgs { pub rebase: bool, } +#[derive(Parser)] +pub struct WorktreeRebaseArgs { + #[clap(long = "--pull", about = "Perform a pull before rebasing")] + pub pull: bool, + #[clap(long = "--rebase", about = "Perform a rebase when doing a pull")] + pub rebase: bool, +} + pub fn parse() -> Opts { Opts::parse() } diff --git a/src/grm/main.rs b/src/grm/main.rs index 20d33a6..d54f45c 100644 --- a/src/grm/main.rs +++ b/src/grm/main.rs @@ -362,6 +362,76 @@ fn main() { } } } + cmd::WorktreeAction::Rebase(args) => { + if args.rebase && !args.pull { + print_error("There is no point in using --rebase without --pull"); + process::exit(1); + } + let repo = grm::Repo::open(&cwd, true).unwrap_or_else(|error| { + if error.kind == grm::RepoErrorKind::NotFound { + print_error("Directory does not contain a git repository"); + } else { + print_error(&format!("Opening repository failed: {}", error)); + } + process::exit(1); + }); + + if args.pull { + repo.fetchall().unwrap_or_else(|error| { + print_error(&format!("Error fetching remotes: {}", error)); + process::exit(1); + }); + } + + let config = + grm::repo::read_worktree_root_config(&cwd).unwrap_or_else(|error| { + print_error(&format!( + "Failed to read worktree configuration: {}", + error + )); + process::exit(1); + }); + + let worktrees = repo.get_worktrees().unwrap_or_else(|error| { + print_error(&format!("Error getting worktrees: {}", error)); + process::exit(1); + }); + + for worktree in &worktrees { + if args.pull { + if let Some(warning) = worktree + .forward_branch(args.rebase) + .unwrap_or_else(|error| { + print_error(&format!( + "Error updating worktree branch: {}", + error + )); + process::exit(1); + }) + { + print_warning(&format!("{}: {}", worktree.name(), warning)); + } + } + } + + for worktree in &worktrees { + if let Some(warning) = + worktree + .rebase_onto_default(&config) + .unwrap_or_else(|error| { + print_error(&format!( + "Error rebasing worktree branch: {}", + error + )); + process::exit(1); + }) + { + print_warning(&format!("{}: {}", worktree.name(), warning)); + } else { + print_success(&format!("{}: Done", worktree.name())); + } + } + } } } } diff --git a/src/repo.rs b/src/repo.rs index 35f8d79..91a5b01 100644 --- a/src/repo.rs +++ b/src/repo.rs @@ -219,8 +219,16 @@ impl Worktree { .map_err(convert_libgit2_error)?; let committer = rebased_commit.committer(); - if rebase.commit(None, &committer, None).is_err() { + // This is effectively adding all files to the index explicitly. + // Normal files are already staged, but changed submodules are not. + let mut index = repo.0.index().map_err(convert_libgit2_error)?; + index + .add_all(["."].iter(), git2::IndexAddOption::CHECK_PATHSPEC, None) + .map_err(convert_libgit2_error)?; + + if let Err(error) = rebase.commit(None, &committer, None) { rebase.abort().map_err(convert_libgit2_error)?; + return Err(convert_libgit2_error(error)); } } @@ -251,6 +259,78 @@ impl Worktree { }; Ok(None) } + + pub fn rebase_onto_default( + &self, + config: &Option, + ) -> Result, String> { + let repo = Repo::open(Path::new(&self.name), false) + .map_err(|error| format!("Error opening worktree: {}", error))?; + + let guess_default_branch = || { + repo.default_branch() + .map_err(|_| "Could not determine default branch")? + .name() + .map_err(|error| format!("Failed getting default branch name: {}", error)) + }; + + let default_branch_name = match &config { + None => guess_default_branch()?, + Some(config) => match &config.persistent_branches { + None => guess_default_branch()?, + Some(persistent_branches) => { + if persistent_branches.is_empty() { + guess_default_branch()? + } else { + persistent_branches[0].clone() + } + } + }, + }; + + let base_branch = repo.find_local_branch(&default_branch_name)?; + let base_annotated_commit = repo + .0 + .find_annotated_commit(base_branch.commit()?.id().0) + .map_err(convert_libgit2_error)?; + + let mut rebase = repo + .0 + .rebase( + None, // use HEAD + Some(&base_annotated_commit), + None, // figure out the base yourself, libgit2! + Some(&mut git2::RebaseOptions::new()), + ) + .map_err(convert_libgit2_error)?; + + while let Some(operation) = rebase.next() { + let operation = operation.map_err(convert_libgit2_error)?; + + // This is required to preserve the commiter of the rebased + // commits, which is the expected behaviour. + let rebased_commit = repo + .0 + .find_commit(operation.id()) + .map_err(convert_libgit2_error)?; + let committer = rebased_commit.committer(); + + // This is effectively adding all files to the index explicitly. + // Normal files are already staged, but changed submodules are not. + let mut index = repo.0.index().map_err(convert_libgit2_error)?; + index + .add_all(["."].iter(), git2::IndexAddOption::CHECK_PATHSPEC, None) + .map_err(convert_libgit2_error)?; + + if let Err(error) = rebase.commit(None, &committer, None) { + rebase.abort().map_err(convert_libgit2_error)?; + return Err(convert_libgit2_error(error)); + } + } + + rebase.finish(None).map_err(convert_libgit2_error)?; + Ok(None) + } } impl RepoStatus {