Skip to content

Conversation

@lunny
Copy link
Member

@lunny lunny commented Sep 27, 2025

In Git 2.38, the merge-tree command introduced the --write-tree option, which works directly on bare repositories. This option produces the merged tree object ID, allowing us to perform diffs between commits without creating a temporary repository. By avoiding the overhead of setting up and tearing down temporary repos, this approach delivers a notable performance improvement.

@lunny lunny added this to the 1.26.0 milestone Sep 27, 2025
@lunny lunny added the performance/speed performance issues with slow downs label Sep 27, 2025
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 27, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Sep 27, 2025
@lunny lunny marked this pull request as ready for review September 29, 2025 23:22
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 2, 2025
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

All old code (for SupportGitMergeTree=false) loses there test coverage?

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 2, 2025
@lunny
Copy link
Member Author

lunny commented Oct 4, 2025

All old code (for SupportGitMergeTree=false) loses there test coverage?

af79992 and e8636b7 added the two types of tests.

@lunny
Copy link
Member Author

lunny commented Oct 15, 2025

  • I don't think it's right to use integration tests to test "gitrepo" related code.

  • I don't think it's right to use a general name like "DiffTree" but its implementation is for a special purpose.

  • Why "doGitCommit" comes again? I have told you. So many many times, don't just copy-paste code!!!

All of these have been resolved.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 27, 2025
return false
}

func ExitCode(err error) (int, bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need such duplicate function

// It uses 'git merge-tree' if supported by the Git version, otherwise it falls back to using a temporary repository.
// This function updates the pr.Status, pr.MergeBase and pr.ConflictedFiles fields as necessary.
// The pull request parameter may not be created yet in the database, so do not assume it has an ID.
func checkPullRequestMergeableAndUpdateStatus(ctx context.Context, pr *issues_model.PullRequest) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have told you, you need to clearly test this function's behavior, but not keep copy-pasting or polluting other unrelated tests

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

Not maintainable

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Oct 28, 2025
@wxiaoguang
Copy link
Contributor

  • I don't think it's right to use integration tests to test "gitrepo" related code.

  • I don't think it's right to use a general name like "DiffTree" but its implementation is for a special purpose.

  • Why "doGitCommit" comes again? I have told you. So many many times, don't just copy-paste code!!!

All of these have been resolved.

I don't see how it's right to use integration tests to test "gitrepo" related code. is resolved. You need to test the function's behavior clearly, but not use the unclear (unrelated) integration tests.

Integration test can be used to make sure "everything works well together", it shouldn't and is not able to be used to test a specific function for its all edge cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged modifies/go Pull requests that update Go code performance/speed performance issues with slow downs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants