git-branchless icon indicating copy to clipboard operation
git-branchless copied to clipboard

Start to add support for interactively checking out a branch

Open bcspragu opened this issue 2 years ago • 3 comments

This is a follow-up from the discussion here. It updates the git co command to check if the selected commit is associated with a single branch, and checks that out if so.

A few caveats:

  • My Rust is gross (lots of copying/mut) and any pointers are greatly
  • It doesn't currently work - instead of checking out <branch name>, it checks out refs/heads/<branch name>. I don't understand enough about git/git-branchless to know why this is the case, or what the appropriate fix is.

bcspragu avatar Sep 09 '22 16:09 bcspragu

Thanks for looking into this! Can we try instead implementing this logic in core::check_out::check_out_commit? That way, we can benefit from this behavior in other places too.

My Rust is gross (lots of copying/mut) and any pointers are greatly

Generally, rather than write

let mut foo = 1;
if cond {
    foo = 2;
}

you would write

let foo = 1;
let foo = if cond { 2 } else { foo };

Similarly, to return multiple values from a conditional statement, you can return a tuple (or even a struct if it gets to be particularly complicated):

let (foo, bar) = if cond {
    (true, "hello")
} else {
    (false, "world")
}

instead of checking out , it checks out refs/heads/.

I think check_out_commit has a fix for this anyways (where it calls remove_prefix).

You may need to update tests for this change. If necessary, to review snapshot tests interactively, you can install cargo-insta and run cargo insta review.

arxanas avatar Sep 10 '22 19:09 arxanas

Also, I added a refactoring for the checkout target in https://github.com/arxanas/git-branchless/pull/514 which might help, so you may want to rebase your work on top of that.

arxanas avatar Sep 10 '22 19:09 arxanas

Thanks! Pulled in your changes, and moved the code into core check_out. Made some of the code less gross, but I'm sure there's plenty of room for improvement.

Currently, I'm just looking for all local branches then checking for an oid match, which may be expensive for large repos. If there's an cheaper way to say "give me all the branches with this oid", we should probably do that.

Looks like the tests are passing, but it seemed like lots of tests required updates. Some of them made sense, we're no longer checking out a detached commit, so the hash is replaced with the branch name, but others were more puzzling (e.g. why does test_reword need to change?)

bcspragu avatar Sep 18 '22 04:09 bcspragu

Not sure why the tests are failing, they work fine locally and produce no diffs in need of review. Given the failing output, looks like the feature might be broken in CI, I'll take a look at this later today

bcspragu avatar Sep 29 '22 16:09 bcspragu

My bet is that you just need to apply this patch:

diff --git a/git-branchless/tests/command/test_navigation.rs b/git-branchless/tests/command/test_navigation.rs
index 3aa0d90..5eb0153 100644
--- a/git-branchless/tests/command/test_navigation.rs
+++ b/git-branchless/tests/command/test_navigation.rs
@@ -859,6 +859,7 @@ fn test_checkout_auto_switch_interactive() -> eyre::Result<()> {
             PtyAction::WaitUntilContains("> "),
             PtyAction::Write("test1"),
             PtyAction::WaitUntilContains("> test1"),
+            PtyAction::WaitUntilContains("> 62fc20d"),
             PtyAction::Write(CARRIAGE_RETURN),
         ],
     )?;
@@ -902,6 +903,7 @@ fn test_checkout_auto_switch_interactive_disabled() -> eyre::Result<()> {
             PtyAction::WaitUntilContains("> "),
             PtyAction::Write("test1"),
             PtyAction::WaitUntilContains("> test1"),
+            PtyAction::WaitUntilContains("> 62fc20d"),
             PtyAction::Write(CARRIAGE_RETURN),
         ],
     )?;

The failure case on CI (or a slow machine in general) is that the process can exit before the UI has a chance to select the given item, which means that nothing will happen when you exit the process because you haven't selected a switch target. It's not clear at all from just the test case, but there are two carets of interest in the UI. Example screenshot:

Screen Shot 2022-09-29 at 11 05 59

One of them is the prompt where you're typing, and the other is the actual selection. You need to wait for the actual selection to complete before exiting. (I guess it would be possible to not have the wait for > test1 at all; I don't remember if there's a reason that the other test cases have that.)

arxanas avatar Sep 29 '22 18:09 arxanas

Other than the failing tests, everything looks good to me. When you're done, please squash and force-push your changes and I'll merge this PR. Thanks for working on this!

arxanas avatar Sep 29 '22 18:09 arxanas

We have some failing Nix tests already for PTY-related stuff, so I just added the new failing tests introduced in this PR to the list of skipped tests. @bcspragu thanks for your contribution!

arxanas avatar Oct 02 '22 20:10 arxanas