Fix resetting to a branch when a tag shares the same name, or vice versa #4571
Conversation
|
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 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". |
Okay, sounds good. Might be worth an update to the PR template? It currently says: 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. |
ChrisMcD1
changed the title
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.)
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.
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 |
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 This test is having the odd behavior that when I: with the slow flag, it gets stuck at the very end and gives this error: (And it breaks my terminal, which is odd because it is inside of |
stefanhaller
force-pushed
the
4569-reset-branch-tag
branch
from
4b11054 to
ee25dde
Compare
In the unlikely scenario that you have a remote branch on `origin` called `foo`, and a local tag called `origin/foo`, git changes the behavior of the previous command such that it produces ``` $ git for-each-ref --sort=refname --format=%(refname:short) refs/remotes origin/branch1 remotes/origin/foo ``` with `remotes/` prepended. Presumably this is to disambiguate it from the local tag `origin/foo`. Unfortunately, this breaks the existing behavior of this function, so the remote branch is never shown. By changing the command, we now get ``` $ git for-each-ref --sort=refname --format=%(refname) refs/remotes refs/remotes/origin/branch1 refs/remotes/origin/foo ``` This allows easy parsing based on the `/`, and none of the code outside this function has to change.
stefanhaller
force-pushed
the
4569-reset-branch-tag
branch
from
ee25dde to
2e47902
Compare
stefanhaller
changed the title
Created #4607 for this.
Nice. Unfortunately this broke some existing integration tests, apparently because the new command includes
I wouldn't worry about this too much, we have existing tests with the same behavior. I opened #4608 to track this. |
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.
I've got some half-formed integration tests going that I could finish up if you want, but I figured this was trivial enough that it wasn't worth finishing them.
Fixes #4569
go generate ./...)