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.

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

Fix loadOneBranch panic #29938

Merged
merged 2 commits into from
Mar 20, 2024
Merged

Fix loadOneBranch panic #29938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2938ed
fix
wxiaoguang Mar 20, 2024
6eeab7e
Merge branch 'main' into fix-git-dash
lafriks Mar 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/git/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ type DivergeObject struct {
// GetDivergingCommits returns the number of commits a targetBranch is ahead or behind a baseBranch
func GetDivergingCommits(ctx context.Context, repoPath, baseBranch, targetBranch string) (do DivergeObject, err error) {
cmd := NewCommand(ctx, "rev-list", "--count", "--left-right").
AddDynamicArguments(baseBranch + "..." + targetBranch)
delvh marked this conversation as resolved.
Show resolved Hide resolved
AddDynamicArguments(baseBranch + "..." + targetBranch).AddArguments("--")
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, meant this change.
(Did GitHub change its mechanism what is displayed when?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And GetDivergingCommits is covered by tests, adding -- is right here.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should add a comment for future readers:

Suggested change
AddDynamicArguments(baseBranch + "..." + targetBranch).AddArguments("--")
AddDynamicArguments(baseBranch + "..." + targetBranch).AddArguments("--") // '--' needed so that git knows what is a reference and what is a file in case of an error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not the only place using it. I don't think it's worth to repeat such comments. It is a git's standard mechanism.

image

stdout, _, err := cmd.RunStdString(&RunOpts{Dir: repoPath})
if err != nil {
return do, err
Expand Down
10 changes: 6 additions & 4 deletions services/repository/branch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,7 @@ func loadOneBranch(ctx context.Context, repo *repo_model.Repository, dbBranch *g
p := protectedBranches.GetFirstMatched(branchName)
isProtected := p != nil

divergence := &git.DivergeObject{
Ahead: -1,
Behind: -1,
}
var divergence *git.DivergeObject

// it's not default branch
if repo.DefaultBranch != dbBranch.Name && !dbBranch.IsDeleted {
Expand All @@ -180,6 +177,11 @@ func loadOneBranch(ctx context.Context, repo *repo_model.Repository, dbBranch *g
}
}

if divergence == nil {
// tolerate the error that we cannot get divergence
divergence = &git.DivergeObject{Ahead: -1, Behind: -1}
}

pr, err := issues_model.GetLatestPullRequestByHeadInfo(ctx, repo.ID, branchName)
if err != nil {
return nil, fmt.Errorf("GetLatestPullRequestByHeadInfo: %v", err)
Expand Down