Fix resetting to a branch when a tag shares the same name, or vice versa

[ChrisMcD1]

• PR Description

Allows the reset menu to have a different name that is displayed, and a fully qualified name that git will unambiguously know what it refers about. We could totally squash this back down to 1 input, and display to the user the precise full ref name that we are resetting to, but I think the context they are in (branches tab versus tag tab), means that we don't need to do that, and can continue to just show the branch name and the tag name to the end users.

Fixes #4569

• 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)
• If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
• Docs have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[stefanhaller]

The code changes look great.

As for tests, the last time we fixed a similar problem (in #3179) I did add a test (and I went to the trouble of adding the test first to demonstrate the problem, like I often do, with "expected/actual" comments). Of course, in that case I only needed a single test; it seems in this case you would need 3 (branches, remote branches, tags). Files and commits seem trivial enough to not need a test. Actually for remote branches it should already have worked? Unless you have a tag that's called origin/branch-name I suppose.

Anyway, I'll leave this for you to decide if you want to add them, but I do think it would be good.

One note about the PR title: it's always a good idea to phrase this from a release notes point of view. The current title doesn't describe what problem we are fixing; I'd suggest something along the lines of "Fix resetting to a tag when a branch with the same name exists, or vice versa".

[ChrisMcD1]

One note about the PR title: it's always a good idea to phrase this from a release notes point of view.

Okay, sounds good. Might be worth an update to the PR template? It currently says:

Be sure to name your PR with an imperative e.g. 'Add worktrees view'
see https://github.com/jesseduffield/lazygit/releases/tag/v0.40.0 for examples

Which made me think that I should be naming it in terms of the changes it makes to the code. (I guess adding worktrees is both the changes to the code, but also a good thing for a release notes 😆 )

---

On the note of tests, I added one for a branch reset and a tag reset. (The tag reset would have never failed prior, but nice to have for the future) I didn't add one for a remote branch because it isn't quite clear to me how the remotes work in integration tests. Are they actually pushing up to GitHub? I wouldn't want to break something without knowing more about that.

[stefanhaller]

Be sure to name your PR with an imperative

That's really just about the tense used, it doesn't say much about how the changes are described. (Personally I find this particular tense thing more important for commit messages than for release notes -- shrug.)

Which made me think that I should be naming it in terms of the changes it makes to the code.

No, this is totally irrelevant in release notes. Release notes are for users, they want to know what features are new or how the behavior has changed; they don't care about the code. And the example 'Add worktrees view' is a good one, this totally describes it from a user's point of view.

I didn't add one for a remote branch because it isn't quite clear to me how the remotes work in integration tests. Are they actually pushing up to GitHub?

No, it's easy to set up a local clone as a remote, and that's what we do in integration tests. There's the shell.CloneIntoRemote helper function for this (it simply clones it into a sibling directory of the current repo, I think). We have quite a lot of tests that use this; maybe filter_and_search/filter_remote_branches.go is a good example for what you need here.

[ChrisMcD1]

And the example 'Add worktrees view' is a good one, this totally describes it from a user's point of view.

Agreed. My point was that the PR description doesn't remind contributors that their PR title will be used in the release notes. Not a huge deal, but a different template would have improved some of my previous PR titles.

---

Thanks for pushing me to write an integration test for the remote branch scenario as well, it made me realize that we wouldn't even display remote branches when we had a tag that matched the name origin/<branch-name> because of we sent the format command to git for-each-ref. This has been updated in the new commits I just pushed.

This test is having the odd behavior that when I:

go run cmd/integration_test/main.go cli --slow branch/reset_to_duplicate_named_upstream

with the slow flag, it gets stuck at the very end and gives this error:

2025/05/27 23:40:35 40 seconds is up, lazygit recording took too long to complete. exit status 1

(And it breaks my terminal, which is odd because it is inside of utils.Safe)
This doesn't happen when I run it without the --slow flag. Any idea what could be causing this, and if it is a real problem?

[stefanhaller]

My point was that the PR description doesn't remind contributors that their PR title will be used in the release notes. Not a huge deal, but a different template would have improved some of my previous PR titles.

Created #4607 for this.

it made me realize that we wouldn't even display remote branches when we had a tag that matched the name origin/<branch-name> because of we sent the format command to git for-each-ref. This has been updated in the new commits I just pushed.

Nice. Unfortunately this broke some existing integration tests, apparently because the new command includes HEAD whereas the old one didn't, or something.

with the slow flag, it gets stuck at the very end

I wouldn't worry about this too much, we have existing tests with the same behavior. I opened #4608 to track this.

[ChrisMcD1]

apparently because the new command includes HEAD whereas the old one didn't, or something.

Got some exciting things coming on this point! Ended up being a bit tricky! Git as of git/git@3f763dd automatically creates refs/remotes/<remote>/HEAD when you do a basic git fetch. We hadn't been seeing this in our integration tests though, because something else changed in Git between 2.30.8 and 2.49 which meant that our formatting of refname:short would result in HEAD being dropped by our code, because it didn't contain a slash.

(ins)chrismcdonnell@Chris-Laptop:~/linux-repos/lazygit/test/_results/sync/push/actual/repo$ PATH=/usr/bin:$PATH git for-each-ref --format="%(refname:short)" refs/remotes/origin
origin/HEAD
origin/master
(cmd)chrismcdonnell@Chris-Laptop:~/linux-repos/lazygit/test/_results/sync/push/actual/repo$ git for-each-ref --format="%(refname:short)" refs/remotes/origin
origin
origin/master

my user bin has the older version of git vs my normal PATH

I'm updating the tests to have a git remote set-head <name> --auto to match what the commit in Git itself says they were trying to replicate with the v2.48.0 behavior going forward. The consequences of that have turned out to be a bit further reaching than I would hope. (How does on deal with sorting of remote branches when you have both HEAD and third-branch referencing the same commit? Git decided to change the behavior of their sort between versions :| )

[stefanhaller]

I don't fully understand all of this yet (don't have time to dig into it right now), but shouldn't we just filter out HEAD and be done with it? I don't think it's useful to see it in the list of remote branches. Something like this

diff --git i/pkg/commands/git_commands/remote_loader.go w/pkg/commands/git_commands/remote_loader.go
index 96abe767d..1b2e33682 100644
--- i/pkg/commands/git_commands/remote_loader.go
+++ w/pkg/commands/git_commands/remote_loader.go
@@ -110,6 +110,10 @@ func (self *RemoteLoader) getRemoteBranchesByRemoteName() (map[string][]*models.
 		remoteName := split[2]
 		name := split[3]
 
+		if name == "HEAD" {
+			return false, nil
+		}
+
 		_, ok := remoteBranchesByRemoteName[remoteName]
 		if !ok {
 			remoteBranchesByRemoteName[remoteName] = []*models.RemoteBranch{}

as a fixup for your "Use full refname instead of short ..." commit seems like an appropriate solution to me.

[ChrisMcD1]

but shouldn't we just filter out HEAD and be done with it?

I personally wasn't aware that origin/HEAD was a thing until I began looking into these broken tests yesterday. It seems that the point of it is to be a reference to the remote server's default branch (master/main/trunk/whatever). https://stackoverflow.com/questions/8839958/how-does-origin-head-get-set

So I think it is a reasonable thing to display. If you frequently work in places with different default branches and find yourself manually rebasing off of origin/HEAD just to keep your life simpler.

Futhermore, we do currently display HEAD in our current release of lazygit if you are on a version of git prior to the changes of %(refname:short) (Git 2.25.0 in my case). We only lose them in later git version because we silently drop them because they don't have a slash when formatted with short.

[ChrisMcD1]

Okay, got the tests passing in all git versions!

Couple notes that I didn't include in commit comments:

1. I moved all of the shell.CloneIntoRemote calls to not be the first method of the integration test. Having an upstream that doesn't have any branches breaks the git remote set-head <upstream> --auto. (Which is likely one of the reasons git/git@3f763dd has it fail silently if that operation fails when fetching)
2. We definitely could make more tests use the shell.RemoveRemoteHead method, or even make that the default of the tests if we wanted to. I left it in to make the tests more representative of a latest git user's experience with LazyGit (where the HEAD will be silently created in the background when they set up their remotes)

[stefanhaller]

I personally wasn't aware that origin/HEAD was a thing

No, me neither.

It seems that the point of it is to be a reference to the remote server's default branch (master/main/trunk/whatever).

Yes, but this is only needed on the command line. It allows using remote-name as a short hand for remote-name/master, e.g. in commands like git reset --hard origin or git rebase remote-name, and that's useful because you don't have to go and look up what branches this remote has. In lazygit, where you plainly see the remote's branches, it is super easy to see whether it has a main or a master, and work with that directly. (It is very unlikely that it has both, and that you need HEAD to tell you which one you want to use.)

So I think it is a reasonable thing to display.

I disagree. It's extra noise that is not useful (and it looks really ugly). The change in git 2.25 that you explain below supports this point of view: %(refname:short) used to include origin/HEAD, but they improved this by making it return just origin. This supports the position that origin/HEAD is an implementation detail that shouldn't be shown to users.

If you frequently work in places with different default branches and find yourself manually rebasing off of origin/HEAD just to keep your life simpler.

In lazygit, it's much simpler to type r b in the branches panel, no need to go to the remotes panel at all for this.

Rebasing onto the main branch of a remote that is not origin is an extremely rare thing for me, I can't remember a single time I did that. If I had to, I would find it completely acceptable to open the remote and see if it has a main or a master branch.

Futhermore, we do currently display HEAD in our current release of lazygit if you are on a version of git prior to the changes of %(refname:short) (Git 2.25.0 in my case).

I'd call that a bug.

To sum it up, it's great that you found ways to make this all work, but honestly I'd be in favor of dropping all these changes and go with the simple patch I posted above instead.

[ChrisMcD1]

Okay, I can agree that most users will never find a need to reference their remote HEAD in their daily flow, but I don't think I can agree that showing it is inherently a bug. git for-each-ref outputs it for a reason, and it's really a previous LazyGit parsing bug that has resulted in it being dropped silently on newer git versions.

I put in a configuration option that will hide it by default, but allows users to show it if they wish.

[stefanhaller]

I'm against adding the config. Nobody asked for it, we shouldn't add configs just because somebody might potentially want to turn it on.

[ChrisMcD1]

Sure, I can agree with that principle, but nobody has also asked us to remove remote HEADs. It's a breaking change for some subset of users, and makes us differ from the behavior of the underlying git tools.

I'm just imagining we will get an issue like this #4272 (comment) upon our next release asking why we made a breaking change and the only reason I'm seeing in this thread is that aesthetically we think it's ugly.

I'm open to being wrong on this, so I've removed the config and pushed

[stefanhaller]

I'm just imagining we will get an issue like this #4272 (comment) upon our next release asking why we made a breaking change

I'm willing to take the risk in this case, and add the option if somebody complains about the change. This will be easy in this case.

I've removed the config and pushed

Great, thanks. Looks good, going to merge.