Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for interactively checking out a branch #526

Merged
merged 1 commit into from
Oct 2, 2022
Merged

Add support for interactively checking out a branch #526

merged 1 commit into from
Oct 2, 2022

Conversation

bcspragu
Copy link
Contributor

@bcspragu bcspragu commented Sep 9, 2022

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.

@arxanas
Copy link
Owner

arxanas commented Sep 10, 2022

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
Copy link
Owner

arxanas commented Sep 10, 2022

Also, I added a refactoring for the checkout target in #514 which might help, so you may want to rebase your work on top of that.

@bcspragu
Copy link
Contributor Author

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?)

git-branchless-lib/src/core/check_out.rs Outdated Show resolved Hide resolved
git-branchless-lib/src/core/check_out.rs Outdated Show resolved Hide resolved
git-branchless-lib/src/core/check_out.rs Outdated Show resolved Hide resolved
git-branchless-lib/src/core/check_out.rs Outdated Show resolved Hide resolved
git-branchless/tests/command/test_reword.rs Show resolved Hide resolved
@bcspragu bcspragu changed the title Start to add support for interactively checking out a branch Add support for interactively checking out a branch Sep 19, 2022
@bcspragu bcspragu marked this pull request as ready for review September 29, 2022 16:50
@bcspragu
Copy link
Contributor Author

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

@arxanas
Copy link
Owner

arxanas commented Sep 29, 2022

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
Copy link
Owner

arxanas commented Sep 29, 2022

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 arxanas enabled auto-merge (rebase) October 2, 2022 20:16
@arxanas
Copy link
Owner

arxanas commented Oct 2, 2022

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 arxanas merged commit 7ce51cc into arxanas:master Oct 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants