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

Remove heads pointing to missing old refs #17076

Merged
merged 10 commits into from
Oct 8, 2021
Merged

Remove heads pointing to missing old refs #17076

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ca427c9
Git rev-list + function for removing heads
99rgosse Sep 21, 2021
d3ed1db
Merge branch 'main' into 15155_remove_heads
zeripath Sep 29, 2021
471114b
Merge branch 'main' into 15155_remove_heads
zeripath Sep 30, 2021
96f9598
Merge branch 'main' into 15155_remove_heads
lafriks Sep 30, 2021
6191ec8
Merge branch 'main' into 15155_remove_heads
zeripath Oct 5, 2021
6eedab4
Merge branch 'main' into 15155_remove_heads
6543 Oct 5, 2021
a4180d4
Merge branch 'main' into 15155_remove_heads
zeripath Oct 6, 2021
bdbff89
Merge branch 'main' into 15155_remove_heads
zeripath Oct 6, 2021
7fe8ed0
Merge branch 'main' into 15155_remove_heads
6543 Oct 7, 2021
32946ab
Merge branch 'main' into 15155_remove_heads
6543 Oct 8, 2021
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
5 changes: 5 additions & 0 deletions modules/migrations/base/pullrequest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ func (p *PullRequest) IsForkPullRequest() bool {
return p.Head.RepoPath() != p.Base.RepoPath()
}

// GetGitRefName returns pull request relative path to head
func (p PullRequest) GetGitRefName() string {
return fmt.Sprintf("refs/pull/%d/head", p.Number)
}

// PullRequestBranch represents a pull request branch
type PullRequestBranch struct {
CloneURL string `yaml:"clone_url"`
Expand Down
17 changes: 17 additions & 0 deletions modules/migrations/gitea_uploader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,6 +690,23 @@ func (g *GiteaLocalUploader) newPullRequest(pr *base.PullRequest) (*models.PullR
}
} else {
head = pr.Head.Ref
// Ensure the closed PR SHA still points to an existing ref
_, err = git.NewCommand("rev-list", "--quiet", "-1", pr.Head.SHA).RunInDir(g.repo.RepoPath())
if err != nil {
if pr.Head.SHA != "" {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if pr.Head.SHA != "" {
if len(pr.Head.SHA) != 0 {

Copy link
Member

Choose a reason for hiding this comment

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

The funny thing is, as far as I have seen:
You are the only one who enforces this.
Almost everyone else uses if str != "".

Copy link
Member

Choose a reason for hiding this comment

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

yeah because it was quite some time ago decided to use len(str) == 0 so I'm manually linting it 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

I also vote for s != "", although there is no difference when the code gets compiled.

  • s != ""
  • len(s) != 0

The first one is much shorter, and easy to read.

Copy link
Member

@delvh delvh Sep 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

yeah because it was quite some time ago decided to use len(str) == 0 so I'm manually linting it laughing

Should we have a(nother) vote on that?
I think that's the fastest way to resolve this, and then everyone knows how to handle this case in the future,

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind for any of options, ex golang stdlib also uses both we just need to keep to the one in our code base.

If I remember correctly len was decided to use because you don't have to think if it's string, byte array or rune array, it will work equally good for all cases. That affects also refactoring later on.

Copy link
Member

Choose a reason for hiding this comment

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

The problem however is that as far as I have seen, both are used currently inside the code.
Sometimes str != "" is used, and sometimes (most likely when you reviewed the PR) len(str) != 0 is used.

Copy link
Contributor

@wxiaoguang wxiaoguang Sep 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

because you don't have to think if it's string, byte array or rune array, it will work equally good for all cases

I think this point is not that important. When we use a string, we do not new s := make(), we do not iterate for x := range s , we do not update s[k] = v, and a string is immutable while array/map are mutable. They differ so much so we do not have to use len to all of them.

Copy link
Member

Choose a reason for hiding this comment

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

I was not the one to propose it back in the days so don't shoot the messenger 😋 I was just pointing out that that's what was agreed on and when I review and see that I just point it out. I'm used to using it myself and for myself it's easier to use len than compare to empty string as I don't have to think so much on a type used. Sadly I can't seem to find where it was discussed as it was so long ago 😂

// Git update-ref remove bad references with a relative path
log.Warn("Deprecated local head, removing : %v", pr.Head.SHA)
relPath := pr.GetGitRefName()
_, err = git.NewCommand("update-ref", "--no-deref", "-d", relPath).RunInDir(g.repo.RepoPath())
} else {
// The SHA is empty, remove the head file
log.Warn("Empty reference, removing : %v", pullHead)
err = os.Remove(filepath.Join(pullHead, "head"))
}
if err != nil {
log.Error("Cannot remove local head ref, %v", err)
}
}
}

if pr.Created.IsZero() {
Expand Down