Allow to switch branches in Commit View (#4115)

[seflue]

• PR Description

When the user checks out a commit which has a local branch ref attached to it, they can select between checking out the branch or checking out the commit as detached head. If no local branch is attached to the commit, the behavior is like before: They are asked to confirm, if they want to checkout the commit as detached head.

Requested in #4115.

Note: I tried also to consider remote branches, but because I wasn't able to correlate remote branches to their commits, I deferred it and leave it open for later improvement.

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go generate ./...)
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
• Text is internationalised (see here)
• Docs have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[stefanhaller]

This is a great start. Did you say you had to learn go for this? That must have been an understatement. :)

A few thoughts on the UX:

• In the menu, the option to check out a detached head needs to come first. I do realize that this is probably not going to be the most common choice, but for the muscle memory of existing users it is important that space, enter still does the same thing as before.
• Instead of just showing the raw hash, I'd probably say something like "detached head at selected commit", or "detached head at fa1afe1" (use short hash)
• For menus we prefer UIs that don't change based on context; we prefer to always show all potentially available options, and strike out the ones that are not applicable (see the "delete branch" menu for a good example). In this case this means that we should always show the menu; when there are no branches at the selected commit, the second menu entry could say "branch at selected commit", with a DisabledReason explaining that there is none.
• I always find it useful to have keybindings for menu entries. We could use d for detached head; it's less obvious what to use for branches since they are dynamic, but we could assign 1, 2 and so on (stopping at 9 when there are more branches, which should be unlikely in practice...)

Other remarks:

• An integration test would be nice. I haven't checked yet whether there's an existing test for checking out a commit that could be extended.
• Can we please say "they" instead of "he" when talking about users in the commit message? :)

[seflue]

This is a great start. Did you say you had to learn go for this? That must have been an understatement. :)

Thanks, but those were actually my first lines of Go code I wrote. But it's not my first programming language, I have to admit. ;)

A few thoughts on the UX:

• In the menu, the option to check out a detached head needs to come first. I do realize that this is probably not going to be the most common choice, but for the muscle memory of existing users it is important that space, enter still does the same thing as before.

Even it is not as convenient for me, I understand, that we do not want to break with the users habits, so I implement it as you requested. My question is: do you think, we could add a configuration option, which would give the user the choice, what comes first? So we can keep the current behavior as default, but guys like me can just configure the application to let branches come first in this menu?

• Instead of just showing the raw hash, I'd probably say something like "detached head at selected commit", or "detached head at fa1afe1" (use short hash)
• For menus we prefer UIs that don't change based on context; we prefer to always show all potentially available options, and strike out the ones that are not applicable (see the "delete branch" menu for a good example). In this case this means that we should always show the menu; when there are no branches at the selected commit, the second menu entry could say "branch at selected commit", with a DisabledReason explaining that there is none.

I understand, that this makes sense in a lot of context, but because there is an option, which is always available (detached head) and depending on the number of branches (or in future even tags) a variable number of other options, I am not sure, if a striked out "default" option is really helpful. If we combine that with the configuration option I just described, it would be inconvenient, because it would force me to manually select the only available option, even when there is no other sensible option.

• I always find it useful to have keybindings for menu entries. We could use d for detached head; it's less obvious what to use for branches since they are dynamic, but we could assign 1, 2 and so on (stopping at 9 when there are more branches, which should be unlikely in practice...)

Agreed, I will see, that I add this to the implementation.

Other remarks:

• An integration test would be nice. I haven't checked yet whether there's an existing test for checking out a commit that could be extended.

Absolutely, as we just saw, that would have helped me to detect the mistake I made by choosing the wrong checkout method, and which I oversaw on manual testing (but I have to admit, that I rushed a bit for fast feedback 😅 ).

• Can we please say "they" instead of "he" when talking about users in the commit message? :)

Of course. Actually I was careful about this in my feature request, but when typing the commit message my german brain subconsciously sneaked in a "generic maskulinum". 😉

[stefanhaller]

Hm ok, if you feel so strongly about the detached head option not being the default one, then I should reconsider whether my suggestion is important enough. In general, muscle memory is an important consideration for lazygit, but maybe in this case it's not so much of a deal that it should outweigh the drawbacks. I don't think a configuration option is justified for something like this. Also, personally I very rarely check out a commit as a detached head; most of the time when I want to check out a commit (e.g. in order to test whether it cleanly builds) I just press e on it. That's simpler because it's easier to get back to where you came from. Finally, thinking about it, it's actually questionable that people deliberately want to check out a detached head if there's also a local branch pointing to the same commit, so it doesn't seem like an annoying mistake if they check out the real branch accidentally.

Another reason why I thought the detached head should come first is for consistency with checking out a remote branch, or checking out a branch by name (c in the branches panel). But I misremembered that; in those cases, "new local branch" actually comes first, and "detached head" second, so this is actually another reason for doing it your way.

So, enough reasons to put the local branch(es) first, so let's keep that the way you have it in your branch now.

For the other issue though, I feel rather strongly about it: I find it confusing if a keybindings sometimes shows a confirmation and other times a menu, depending on context. And I wonder how much of a problem it is that you have to type space, down, enter (or space d) if you want to check out a detached head, when the discussion above has just concluded that this is a pretty rare operation anyway. 😄

Also, we have precedence for this kind of thing: if you press r on the current branch in the branches panel, you get a menu where only the third entry is available, the first two are disabled. I use that menu often, and it doesn't bother me at all, because I got used to typing r b to invoke that third command.

[jesseduffield]

I'm torn about what to show first: even if in this case users are more likely to want to switch to the branch than the commit, the intention is to roll this pattern out for more actions (drop, etc) and if we do that, we should be consistent in the ordering across those actions. I very much want to preserve the muscle memory for dropping commits, and so I think we should just always have the commit as the first option in the menu.

Worth mentioning some alternative approaches before we commit to this one:

1. add a new keybinding for bringing up a menu for the selected branch (with switch/drop/etc)
2. add a keybinding for jumping to the selected branch

Option 2 sounds especially appealing to me as it means we get all the branch options for free without needing to maintain a separate way of interacting with branches, and it's easy to get back to the commits view with a single keypress if you misclicked.

[seflue]

I'm torn about what to show first: even if in this case users are more likely to want to switch to the branch than the commit, the intention is to roll this pattern out for more actions (drop, etc) and if we do that, we should be consistent in the ordering across those actions. I very much want to preserve the muscle memory for dropping commits, and so I think we should just always have the commit as the first option in the menu.

I understand that and with shortcuts I'm actually agnostic about what comes first. Pushed my current implementation as suggested by @stefanhaller, but with the detached option first.
But I actually struggle to get the strikethrough styling to work for this "no branches available" entry - I'm still not completely convinced, that we actually need this. 🙄

Worth mentioning some alternative approaches before we commit to this one:

1. add a new keybinding for bringing up a menu for the selected branch (with switch/drop/etc)
2. add a keybinding for jumping to the selected branch

Option 2 sounds especially appealing to me as it means we get all the branch options for free without needing to maintain a separate way of interacting with branches, and it's easy to get back to the commits view with a single keypress if you misclicked.

Actually this was what I tried to describe here with Option2. The <c-b> keybinding is still available in the commit view, so I could also go for that. @stefanhaller what do you think?

What I like about the first approach: The command is named "checkout" and having all checkout options discoverable behind this command would match the expectation of most users (especially non-git-experts). Checking out a commit in detached head mode, when branches are available at this very commit is probably a very rare use case. I actually never had the desire to do so and I'm using Git for about 15 years now.

The advantage of the second option would be, that we wouldn't need to overload existing commands with different semantics (dropping a commit is something different than deleting a branch).

How about having both? Enhancing checkout as I already did (with improvements and polishing as you guys suggest) and having a dedicated branch operation menu for everything else I listed at #4115.
Switching branches is the operation I am missing the most in the commit view so it makes sense to have it at your fingertips with <Space>. And "Checkout" is somehow a synonym for switching a branch - at least for old-school Git users like me. 🧙‍♂️ 😉

[stefanhaller]

I'm torn about what to show first: even if in this case users are more likely to want to switch to the branch than the commit, the intention is to roll this pattern out for more actions (drop, etc) and if we do that, we should be consistent in the ordering across those actions. I very much want to preserve the muscle memory for dropping commits, and so I think we should just always have the commit as the first option in the menu.

Good point about dropping, I totally agree that the commit needs to come first in that case. However, I'm not convinced it is necessary for the order to be consistent across dropping and checkout; those are very different operations. I'd rather think it should be consistent across pressing space in the commits view, versus pressing space in the remote branches view; in the latter case, local branch comes first, and detached head second.

1. add a new keybinding for bringing up a menu for the selected branch (with switch/drop/etc)
2. add a keybinding for jumping to the selected branch

Option 2 sounds especially appealing to me as it means we get all the branch options for free without needing to maintain a separate way of interacting with branches, and it's easy to get back to the commits view with a single keypress if you misclicked.

I find Option 2 mainly attractive from an implementation point of view (it's the least amount of work). From a user's point of view I find it the least desirable, because it's the least immediate interaction; I find it preferable to stay in the commits panel for these operations. Option 1 is mainly attractive because it keeps the key bindings clean (there is only a single one to add, vs. potentially several if we go this route). Apart from that I'm on the fence whether I like it better or worse than what's proposed here.

[stefanhaller]

But I actually struggle to get the strikethrough styling to work for this "no branches available" entry

Looks like you figured that out meanwhile.

The advantage of the second option would be, that we wouldn't need to overload existing commands with different semantics (dropping a commit is something different than deleting a branch).

I don't find this to be a problem. d is lazygit's standard key binding to delete things, so if there are two "things" at the current selection, it makes sense to be able to delete either by pressing d.

[seflue]

@stefanhaller I implemented your suggestions (I hope I didn't forget anything) and cleaned up the commits a bit. If you are satisfied with the current stage, we can merge it. But I can also squash everything together, if you want to avoid the back-and-forth in the history. If there is anything else to change, I appreciate your feedback.

[stefanhaller]

Much better. Yes, I'd prefer everything to be squashed into one commit at this point. (For the future, I prefer fixup! commits in cases like this; they make the iterative development transparent, but at the same time they make it clear what the final history is going to look like.)

Still quite a few comments below, but many are at the level of nitpicking now.

The order question is still unresolved; I feel I'd still prefer the local branch to come first, but I can live with either order. We need @jesseduffield to make the final call here.

And finally, adding an integration test or two would be great. Let me know if you need help with that.

[seflue]

Much better. Yes, I'd prefer everything to be squashed into one commit at this point. (For the future, I prefer fixup! commits in cases like this; they make the iterative development transparent, but at the same time they make it clear what the final history is going to look like.)

I reworded the commits, is it in the form you would like to have it? The disadvantage of automatically squashable fixup commits is that you cannot distinguish them from their title.

Still quite a few comments below, but many are at the level of nitpicking now.

I addressed most of them, but please have a look at my comments to finalize it.

The order question is still unresolved; I feel I'd still prefer the local branch to come first, but I can live with either order. We need @jesseduffield to make the final call here.

I put the detached checkout first as @jesseduffield requested and I am not sure, what your last take was on that. You seemed rather agnostic with a tendency to "be consistent with the muscle memory of the users". With the short keys I am fine with keeping the detached head first as it is now implemented.

And finally, adding an integration test or two would be great. Let me know if you need help with that.

Some entry point and an example would be helpful. Are there some guidelines or special rules I need to obey?

[stefanhaller]

I reworded the commits, is it in the form you would like to have it?

Yes, perfect. (For next time, that is; in this case I thought you'd already squash the ones that we dealt with already, and only add new ones for new changes; no big deal either way though, these things are a bit of a matter of taste.)

The disadvantage of automatically squashable fixup commits is that you cannot distinguish them from their title.

Adding a brief description in the body like you did is a perfect way of dealing with this. You can even see these in github by clicking on the little ... icon:

I put the detached checkout first as @jesseduffield requested and I am not sure, what your last take was on that.

I thought I was pretty elaborate on that. Maybe I wrote too much, as I sometimes have a tendency of doing. I was originally hung up on the muscle memory thing, not realizing that it's not really so important in this particular case; then you convinced me that having the local branch first is better, and I agreed with that, and still do.

Let me try to summarize the reasons again:

• Checking out a branch at a commit is a much more useful operation than checking out a detached head, so it makes sense to put this on space, enter (in most cases there won't be more than one branch)
• While in general it's pretty important not to mess with people's muscle memory, it's not so bad in this case, for two reasons:
  • it's unlikely that checking out a detached head is a very common operation, so it won't affect many people (this is just my guess though)
  • even if some people are used to doing it often, and they now accidentally do the other operation, it won't cause any harm. Checking out a branch instead of a detached head doesn't lose data, and since you have to manually go back to where you came from anyway, it doesn't really make a lot of a difference whether you go back from a detached head or from another branch.
• it's consistent with checking out a remote branch, in which case the "local branch" options comes first, too
• it's true that it won't be consistent with the d command (when we change this as planned), because in this case we do have to keep the commit first for backward compatibility. But I don't think this consistency is very important.

And finally, adding an integration test or two would be great. Let me know if you need help with that.

Some entry point and an example would be helpful. Are there some guidelines or special rules I need to obey?

I just whipped up a quick example here: dad705d. I found this easier than explaining what I would do...

[stefanhaller]

Ok, I took the liberty of squashing the fixups so far and rebasing onto master (we were behind quite a bit). I added a few new fixups as mentioned above, plus one that removes some unused text again.

This all looks good now (with the order decision still pending), there's just one last thing that I'm unsure about: the menu uses a table layout and color coding, and I'm not sure that's needed. Currently it looks like this:

I guess I would have expected something more like this, without colors:

+- Checkout branch or commit ------------------+
| d Checkout commit ba6b7506 as detached head  |
| 1 Checkout branch 'branch1'                  |
| 2 Checkout branch 'master'                   |
+----------------------------------------------+

I think this would be more in line with some of the existing menus related to branches, and I actually find it easier to read without the gaps between columns. Any thoughts?

[jesseduffield]

Re: order of menu items: I'm happy to go with your suggestion @stefanhaller. I feel like we might need to revise this once we've rolled out more of these branch-specific options but I'm happy to go with your approach now.

Re: menu layout: I agree, no need for colours or separate columns

[seflue]

So just to recap the open points, I still need to do:

• adjust the order of the menu items: put the branches before the detached checkout
• replace the colorful table layout with the simpler suggestion from @stefanhaller
• implement an integration test based on the wip commit from @stefanhaller

I will try to fulfill at least the first two points tomorrow. The third point I don't know if I can realize it tomorrow. Then I will be away from my computer for the next 5 days.

@stefanhaller Sorry for the naive question: How do I incorporate your rebase + fixup + wip into my pull request? This is the first time I'm working with git wizards who have far more sophisticated git workflows than I do 😅
Okay, I have that figured out, I didn't want to bother you.

[stefanhaller]

I pushed a few fixups again, see their commit messages. I'm happy with this now, I'd say this is ready to merge.

Regarding the test that I added, I didn't really consider that a WIP commit. I think we can leave it as it is (unless you feel it should test more cases; I'll leave that up to you).

[seflue]

I pushed a few fixups again, see their commit messages.

Many thanks. It is good to see, that you use the placeholder pattern for strings already - I just wasn't sure if I'm on the right track (but honestly I didn't look for similar strings systematically).

Regarding the test that I added, I didn't really consider that a WIP commit. I think we can leave it as it is (unless you feel it should test more cases; I'll leave that up to you).

I now actually see, that the test was pretty complete. And no, I currently don't see any additions needed.

I'm happy with this now, I'd say this is ready to merge.

So am I. 👍 Thanks for your support @stefanhaller, it was a very nice experience to collaborate with you, and I learned quite a bit. I felt very welcome, so I would be happy to continue contributing to the project. ❤️

Should I squash the fixups or would you do it before merging?

[stefanhaller]

So am I. 👍 Thanks for your support @stefanhaller, it was a very nice experience to collaborate with you, and I learned quite a bit. I felt very welcome, so I would be happy to continue contributing to the project. ❤️

Glad to hear this. Looking forward to your next contribution!

Should I squash the fixups or would you do it before merging?

Either way is fine for me. If you do, please also rebase on master again.

[seflue]

Should I squash the fixups or would you do it before merging?

Either way is fine for me. If you do, please also rebase on master again.

Squashed and rebased, ready to merge. 👍

[YikChingTsui]

This new menu ordering broke my muscle memory. But first, I want to clarify if this prompt is only supposed to show up when trying to checkout a commit that is also the last commit of a local branch? The original PR says:

If no local branch is attached to the commit, the behavior is like before: They are asked to confirm, if they want to checkout the commit as detached head.

But the code doesn't match that, it will insert a useless option if there are no local branches:

lazygit/pkg/gui/controllers/helpers/refs_helper.go

Lines 298 to 305 in 40d6800

	} else {
	menuItems = append(menuItems, &types.MenuItem{
	LabelColumns: []string{self.c.Tr.Actions.CheckoutBranch},
	OnPress: func() error { return nil },
	DisabledReason: &types.DisabledReason{Text: self.c.Tr.NoBranchesFoundAtCommitTooltip},
	Key: '1',
	})
	}

My workflow often involves checking out individual commits as detached head. I always get this popup asking for checking out a branch or the commit as detached head. Of course, the first option - selected by default - is disabled because there is "no branch found at the selected commit".

Given that it knows the first option cannot be chosen, why is it the default? It should select the first non-disabled option by default, so the old muscle memory of enter space would checkout the detached head.

[stefanhaller]

We went a little back and forth on the behavior during development, discussing various aspects of it, including the muscle memory thing, and eventually forgot to update the PR description, so no, it doesn't match the behavior any more. Sorry about that. You can learn more about the decisions we made by reading the discussion above.

My workflow often involves checking out individual commits as detached head

I'm very interested to learn more about this. Can you describe this workflow more?

[YikChingTsui]

In the discussion above, I just saw you did consider my usage above but thought it would be uncommon:

it's unlikely that checking out a detached head is a very common operation, so it won't affect many people (this is just my guess though)

Anyway it seems odd to me to add a useless option that does nothing, and also select it by default.

I'm very interested to learn more about this. Can you describe this workflow more?

Sometimes I would fix a bug first before writing a test. In that situation, I would move the test commit to be before the bug fix commit, then checkout the bug fix commit and run the test to ensure it fails and that the bug fix commit successfully fixed the bug.

Or sometimes I would just want to look at a previous state of some code. I have also checked out different commits to compile a different version to them run them.

I suppose in all those situations, you could create a branch at that commit and checkout that branch. But why do that when you only need to "check out" that commit? I only create a branch when I actually want to "branch" the code. I create tags for releases, but surely it's not uncommon to run different unreleased development builds.

(For the first use case, I am indeed branching the code, but rebasing and rearranging commits should be common; a bug fix commit and its test commit is quite linear and if it's just 2 or so commits, it won't cause any confusion to edit linear history)

If you search "git checkout specific commit", there are many stack overflow questions and blogs, etc. It doesn't seem like a niche feature to me. Maybe people like having a branch to avoid losing changes? I do indeed create a branch in that case, but I would have checked out to the commit first, discover I need to edit, then create a branch.

[stefanhaller]

Sometimes I would fix a bug first before writing a test. In that situation, I would move the test commit to be before the bug fix commit, then checkout the bug fix commit and run the test to ensure it fails and that the bug fix commit successfully fixed the bug.

Or sometimes I would just want to look at a previous state of some code.

See, that's what I suspected 😄. I do both of these things all the time (several times every day, no exaggeration). However, I don't press space to do that; I press e to "edit" the commit I want to check out.

There are many reasons why that's better:

• it's a single key press with no confirmation, so it's faster
• it's much easier to get back to where you were. With a detached head you need to go to the branches panel, find your branch again, and check it out using space; with e you either continue the rebase (m enter) or abort it (esc enter), both of which is easier and allows you to stay in the commits panel.
• it makes it easier to do this for multiple commits of your branch. Just mark them all with e and then go from one to the next using m enter.
• if you find there's actually something broken in the commit you checked out, it's much easier to fix it up. Just make the necessary changes, amend them right in using shift-A, and continue the rebase. I would have to think for a while to figure out how to even do this when I'm on a detached head.

So, I feel that sometimes it's a good thing to break people's muscle memory to make them realize there are better ways to achieve what they want to do. 😄

[YikChingTsui]

Thanks for the suggestion, I'll have to try out your workflow for a while first. I guess it's not such a big change just for checking out things.

For editing I prefer a slower and more manual way of commit first, review the diff, then edit history. It gives me the opportunity to double check my edits. But I can just use edit as a checkout replacement and avoid actually editing with it. Thanks

(Git's use of names is just odd: using "rebase edit" to "check out" a commit is better than using checkout? Git frontends are supposed to provide a better abstraction, but maybe only something like jj can properly fix it)