Skip to content

fix ambiguous branch name #2224

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.

Sign up for GitHub

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

Jump to bottom
Merged
jesseduffield merged 1 commit into from
Nov 14, 2022
Merged

fix ambiguous branch name #2224

jesseduffield merged 1 commit into from
Nov 14, 2022

Conversation

@Ryooooooga
Copy link
Contributor

@Ryooooooga Ryooooooga commented Oct 16, 2022 •
edited
Loading

  • PR Description

When we try to create a new branch on a branch named @, it is created at HEAD.

  • git branch @ HEAD~~~

  • launch lazygit and n (new branch) or c (checkout by name) on branch @

  • type a branch name and confirm

  • the new branch is created at HEAD not @

  • Please check if the PR fulfills these requirements

  • Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.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 (specifically docs/Config.md) have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

@Ryooooooga Ryooooooga changed the title fix: fix ambiguous branch name fix ambiguous branch name Oct 16, 2022
@Ryooooooga Ryooooooga force-pushed the ambiguous-branch branch 5 times, most recently from 6fd63ac to 4722ac0 Compare October 16, 2022 13:25
jesseduffield
jesseduffield requested changes Oct 16, 2022
View reviewed changes
Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple things :)

pkg/commands/models/branch.go Outdated
}

func (b *Branch) IsDetachedHead() bool {
return b.DisplayName != ""
Copy link
Owner

@jesseduffield jesseduffield Oct 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather add a new field called DetachedHead (bool) than depend on this display name because I can imagine the display name being populated for a branch that's not a detached head

Copy link
Contributor Author

@Ryooooooga Ryooooooga Oct 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 done

pkg/integration/tests/branch/checkout_by_name.go Outdated
)

var CheckoutByName = NewIntegrationTest(NewIntegrationTestArgs{
Description: "Try to checkout branch by name.",
Copy link
Owner

@jesseduffield jesseduffield Oct 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add some more to this description about the specific bug we're trying to defend against? That will help people understand why the test is slightly more complicated than just directly checking out a branch by name

@Ryooooooga Ryooooooga force-pushed the ambiguous-branch branch 2 times, most recently from 4e206fa to 8982166 Compare October 18, 2022 14:01
jesseduffield
jesseduffield reviewed Oct 25, 2022
View reviewed changes
pkg/commands/loaders/branches.go Outdated
*common.Common
getRawBranches func() (string, error)
getCurrentBranchName func() (string, string, error)
getCurrentBranchName func() (string, string, bool, error)
Copy link
Owner

@jesseduffield jesseduffield Oct 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function returns enough values that we should have it return a struct with those values as fields. That will make it clearer at the call site what's being returned. And then we can call it getCurrentBranchInfo

jesseduffield
jesseduffield requested changes Oct 25, 2022
View reviewed changes
Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing :)

Ryooooooga
Ryooooooga commented Oct 29, 2022
View reviewed changes
pkg/commands/loaders/branches.go
Branches() (map[string]*config.Branch, error)
}

type BranchInfo struct {
Copy link
Contributor Author

@Ryooooooga Ryooooooga Oct 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

define in loaders instead of git_commands to avoid import cycles.

Copy link
Owner

@jesseduffield jesseduffield Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sucks that we have to do this: we ought to either merge the loaders directory into the git_commands directory if it's causing these kinds of issues. But we can do that in another PR

Copy link
Owner

@jesseduffield jesseduffield Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created an issue for this: #2248

jesseduffield
jesseduffield reviewed Nov 7, 2022
View reviewed changes
pkg/commands/loaders/commits_test.go Outdated
getCurrentBranchName: func() (string, string, error) {
return scenario.currentBranchName, scenario.currentBranchName, nil
getCurrentBranchInfo: func() (BranchInfo, error) {
return BranchInfo{scenario.currentBranchName, scenario.currentBranchName, false}, nil
Copy link
Owner

@jesseduffield jesseduffield Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use field names here? Just because it makes it clearer what's happening

Copy link
Contributor Author

@Ryooooooga Ryooooooga Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

jesseduffield
jesseduffield requested changes Nov 7, 2022
View reviewed changes
Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ryooooooga just one more thing :)

@jesseduffield jesseduffield mentioned this pull request Nov 7, 2022
Closed
@Ryooooooga @jesseduffield
fix: fix ambiguous branch name
52a2e4c 
test: add an integration test for checkout branch by name

fix: fix full ref name of detached head

refactor: refactor current branch loader

chore: use field name explicitly
@jesseduffield
Copy link
Owner

jesseduffield commented Nov 14, 2022

@Ryooooooga I've fixed some merge conflicts

@jesseduffield jesseduffield merged commit de22238 into jesseduffield:master Nov 14, 2022
@jesseduffield
Copy link
Owner

jesseduffield commented Nov 14, 2022

Nice work!

@Ryooooooga Ryooooooga deleted the ambiguous-branch branch November 14, 2022 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jesseduffield jesseduffield Awaiting requested review from jesseduffield

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Ryooooooga @jesseduffield