Improve pull request compare performance by making mergeability and diff loading manually triggerable, with configurable default mode (#3996)

This commit is contained in:
Shreejan Shrestha
2026-04-05 20:40:14 +09:00
committed by GitHub
parent 9c4c2dfc81
commit 2587f2bd19
13 changed files with 219 additions and 94 deletions

View File

@@ -375,6 +375,11 @@ trait PullRequestsControllerBase extends ControllerBase {
get("/:owner/:repository/compare")(referrersOnly { forkedRepository =>
val headBranch = params.get("head")
val quickLoad = params
.get("quick")
.map(_.equalsIgnoreCase("true"))
.getOrElse(context.settings.basicBehavior.compareNoCheckByDefault)
val quickQuery = if (quickLoad) "?quick=true" else ""
(forkedRepository.repository.originUserName, forkedRepository.repository.originRepositoryName) match {
case (Some(originUserName), Some(originRepositoryName)) =>
getRepository(originUserName, originRepositoryName).map { originRepository =>
@@ -388,7 +393,7 @@ trait PullRequestsControllerBase extends ControllerBase {
.getOrElse(JGitUtil.getDefaultBranch(oldGit, originRepository).get._2)
redirect(
s"/${forkedRepository.owner}/${forkedRepository.name}/compare/$originUserName:$oldBranch...$newBranch"
s"/${forkedRepository.owner}/${forkedRepository.name}/compare/${originUserName}:${oldBranch}...${newBranch}${quickQuery}"
)
}
} getOrElse NotFound()
@@ -396,7 +401,7 @@ trait PullRequestsControllerBase extends ControllerBase {
Using.resource(Git.open(getRepositoryDir(forkedRepository.owner, forkedRepository.name))) { git =>
JGitUtil.getDefaultBranch(git, forkedRepository).map { case (_, defaultBranch) =>
redirect(
s"/${forkedRepository.owner}/${forkedRepository.name}/compare/$defaultBranch...${headBranch.getOrElse(defaultBranch)}"
s"/${forkedRepository.owner}/${forkedRepository.name}/compare/${defaultBranch}...${headBranch.getOrElse(defaultBranch)}${quickQuery}"
)
} getOrElse {
redirect(s"/${forkedRepository.owner}/${forkedRepository.name}")
@@ -436,76 +441,112 @@ trait PullRequestsControllerBase extends ControllerBase {
val Seq(origin, forked) = multiParams("splat")
val (originOwner, originId) = parseCompareIdentifier(origin, forkedRepository.owner)
val (forkedOwner, forkedId) = parseCompareIdentifier(forked, forkedRepository.owner)
val requestedCheck = params.get("check").contains("true")
val quickLoad = params
.get("quick")
.map(_.equalsIgnoreCase("true"))
.getOrElse(!requestedCheck && context.settings.basicBehavior.compareNoCheckByDefault)
(for (
originRepositoryName <- getOriginRepositoryName(originOwner, forkedOwner, forkedRepository);
originRepository <- getRepository(originOwner, originRepositoryName)
) yield {
val (oldId, newId) =
getPullRequestCommitFromTo(originRepository, forkedRepository, originId, forkedId)
val members =
((forkedRepository.repository.originUserName, forkedRepository.repository.originRepositoryName) match {
case (Some(userName), Some(repositoryName)) =>
getRepository(userName, repositoryName) match {
case Some(x) => x.repository :: getForkedRepositories(userName, repositoryName)
case None => getForkedRepositories(userName, repositoryName)
}
case _ =>
forkedRepository.repository :: getForkedRepositories(forkedRepository.owner, forkedRepository.name)
}).map { repository =>
(repository.userName, repository.repositoryName, repository.defaultBranch)
}
(oldId, newId) match {
case (Some(oldId), Some(newId)) =>
val (commits, diffs) = getRequestCompareInfo(
originRepository.owner,
originRepository.name,
oldId.getName,
forkedRepository.owner,
forkedRepository.name,
newId.getName,
context.settings
)
val text = forkedId.replaceAll("[\\-_]", " ")
val fallbackTitle = text.substring(0, 1).toUpperCase + text.substring(1)
val title = if (commits.flatten.length == 1) {
commits.flatten.head.shortMessage
} else {
val text = forkedId.replaceAll("[\\-_]", " ")
text.substring(0, 1).toUpperCase + text.substring(1)
}
if (quickLoad) {
html.compare(
fallbackTitle,
Seq.empty,
Seq.empty,
members,
List.empty,
originId,
forkedId,
"",
"",
getContentTemplate(originRepository, "PULL_REQUEST_TEMPLATE"),
forkedRepository,
originRepository,
forkedRepository,
hasDeveloperRole(originRepository.owner, originRepository.name, context.loginAccount),
getAssignableUserNames(originRepository.owner, originRepository.name),
getMilestones(originRepository.owner, originRepository.name),
getPriorities(originRepository.owner, originRepository.name),
getDefaultPriority(originRepository.owner, originRepository.name),
getLabels(originRepository.owner, originRepository.name),
getCustomFields(originRepository.owner, originRepository.name).filter(_.enableForPullRequests),
quickLoad
)
} else {
val (oldId, newId) =
getPullRequestCommitFromTo(originRepository, forkedRepository, originId, forkedId)
html.compare(
title,
commits,
diffs,
((forkedRepository.repository.originUserName, forkedRepository.repository.originRepositoryName) match {
case (Some(userName), Some(repositoryName)) =>
getRepository(userName, repositoryName) match {
case Some(x) => x.repository :: getForkedRepositories(userName, repositoryName)
case None => getForkedRepositories(userName, repositoryName)
}
case _ =>
forkedRepository.repository :: getForkedRepositories(forkedRepository.owner, forkedRepository.name)
}).map { repository =>
(repository.userName, repository.repositoryName, repository.defaultBranch)
},
commits.flatten
.flatMap(commit =>
getCommitComments(forkedRepository.owner, forkedRepository.name, commit.id, includePullRequest = false)
)
.toList,
originId,
forkedId,
oldId.getName,
newId.getName,
getContentTemplate(originRepository, "PULL_REQUEST_TEMPLATE"),
forkedRepository,
originRepository,
forkedRepository,
hasDeveloperRole(originRepository.owner, originRepository.name, context.loginAccount),
getAssignableUserNames(originRepository.owner, originRepository.name),
getMilestones(originRepository.owner, originRepository.name),
getPriorities(originRepository.owner, originRepository.name),
getDefaultPriority(originRepository.owner, originRepository.name),
getLabels(originRepository.owner, originRepository.name),
getCustomFields(originRepository.owner, originRepository.name).filter(_.enableForPullRequests)
)
case (oldId, newId) =>
redirect(
s"/${forkedRepository.owner}/${forkedRepository.name}/compare/" +
s"$originOwner:${oldId.map(_ => originId).getOrElse(originRepository.repository.defaultBranch)}..." +
s"$forkedOwner:${newId.map(_ => forkedId).getOrElse(forkedRepository.repository.defaultBranch)}"
)
(oldId, newId) match {
case (Some(oldId), Some(newId)) =>
val (commits, diffs) = getRequestCompareInfo(
originRepository.owner,
originRepository.name,
oldId.getName,
forkedRepository.owner,
forkedRepository.name,
newId.getName,
context.settings
)
val title = if (commits.flatten.length == 1) {
commits.flatten.head.shortMessage
} else {
fallbackTitle
}
val commitComments = commits.flatten
.flatMap(commit => getCommitComments(forkedRepository.owner, forkedRepository.name, commit.id, false))
.toList
html.compare(
title,
commits,
diffs,
members,
commitComments,
originId,
forkedId,
oldId.getName,
newId.getName,
getContentTemplate(originRepository, "PULL_REQUEST_TEMPLATE"),
forkedRepository,
originRepository,
forkedRepository,
hasDeveloperRole(originRepository.owner, originRepository.name, context.loginAccount),
getAssignableUserNames(originRepository.owner, originRepository.name),
getMilestones(originRepository.owner, originRepository.name),
getPriorities(originRepository.owner, originRepository.name),
getDefaultPriority(originRepository.owner, originRepository.name),
getLabels(originRepository.owner, originRepository.name),
getCustomFields(originRepository.owner, originRepository.name).filter(_.enableForPullRequests),
quickLoad
)
case (oldId, newId) =>
redirect(
s"/${forkedRepository.owner}/${forkedRepository.name}/compare/" +
s"${originOwner}:${oldId.map(_ => originId).getOrElse(originRepository.repository.defaultBranch)}..." +
s"${forkedOwner}:${newId.map(_ => forkedId).getOrElse(forkedRepository.repository.defaultBranch)}"
)
}
}
}) getOrElse NotFound()
})

View File

@@ -49,6 +49,7 @@ trait SystemSettingsControllerBase extends AccountManagementControllerBase {
"gravatar" -> trim(label("Gravatar", boolean())),
"notification" -> trim(label("Notification", boolean())),
"limitVisibleRepositories" -> trim(label("limitVisibleRepositories", boolean())),
"compareNoCheckByDefault" -> trim(label("Default compare mode", boolean())),
)(BasicBehavior.apply),
"ssh" -> mapping(
"enabled" -> trim(label("SSH access", boolean())),

View File

@@ -30,6 +30,7 @@ trait SystemSettingsService {
props.setProperty(Gravatar, settings.basicBehavior.gravatar.toString)
props.setProperty(Notification, settings.basicBehavior.notification.toString)
props.setProperty(LimitVisibleRepositories, settings.basicBehavior.limitVisibleRepositories.toString)
props.setProperty(CompareNoCheckByDefault, settings.basicBehavior.compareNoCheckByDefault.toString)
props.setProperty(SshEnabled, settings.ssh.enabled.toString)
settings.ssh.bindAddress.foreach { bindAddress =>
props.setProperty(SshBindAddressHost, bindAddress.host.trim())
@@ -128,7 +129,8 @@ trait SystemSettingsService {
),
getValue(props, Gravatar, false),
getValue(props, Notification, false),
getValue(props, LimitVisibleRepositories, false)
getValue(props, LimitVisibleRepositories, false),
getValue(props, CompareNoCheckByDefault, false)
),
Ssh(
enabled = getValue(props, SshEnabled, false),
@@ -281,6 +283,7 @@ object SystemSettingsService {
gravatar: Boolean,
notification: Boolean,
limitVisibleRepositories: Boolean,
compareNoCheckByDefault: Boolean,
)
case class RepositoryOperation(
@@ -413,6 +416,7 @@ object SystemSettingsService {
private val Gravatar = "gravatar"
private val Notification = "notification"
private val LimitVisibleRepositories = "limitVisibleRepositories"
private val CompareNoCheckByDefault = "compare_no_check_by_default"
private val SshEnabled = "ssh"
private val SshHost = "ssh.host"
private val SshPort = "ssh.port"

View File

@@ -240,6 +240,21 @@
</label>
</fieldset>
<!--====================================================================-->
<!-- Compare default mode -->
<!--====================================================================-->
<hr>
<label class="strong">Pull request compare default mode</label>
<fieldset>
<label class="radio">
<input type="radio" name="basicBehavior.compareNoCheckByDefault" value="true"@if(context.settings.basicBehavior.compareNoCheckByDefault){ checked}>
<span class="strong">No check (fast)</span> <span class="normal">- Open compare quickly and run checks only when clicking Check mergeability.</span>
</label>
<label class="radio">
<input type="radio" name="basicBehavior.compareNoCheckByDefault" value="false"@if(!context.settings.basicBehavior.compareNoCheckByDefault){ checked}>
<span class="strong">Check by default</span> <span class="normal">- Load full compare and mergeability behavior by default.</span>
</label>
</fieldset>
<!--====================================================================-->
<!-- Show mail address -->
<!--====================================================================-->
<hr>

View File

@@ -37,7 +37,7 @@
<a class="btn btn-success" href="@helpers.url(repository)/issues/new">New issue</a>
}
@if(target == "pulls"){
<a class="btn btn-success" href="@helpers.url(repository)/compare">New pull request</a>
<a class="btn btn-success" href="@helpers.url(repository)/compare@if(context.settings.basicBehavior.compareNoCheckByDefault){?quick=true}">New pull request</a>
}
}
</form>

View File

@@ -197,7 +197,7 @@
@if(target == "issues"){
<a href="@helpers.url(repository.get)/issues/new">Create a new issue.</a>
} else {
<a href="@helpers.url(repository.get)/compare">Create a new pull request.</a>
<a href="@helpers.url(repository.get)/compare@if(context.settings.basicBehavior.compareNoCheckByDefault){?quick=true}">Create a new pull request.</a>
}
}
@*

View File

@@ -17,11 +17,11 @@
priorities: List[gitbucket.core.model.Priority],
defaultPriority: Option[gitbucket.core.model.Priority],
labels: List[gitbucket.core.model.Label],
customFields: List[gitbucket.core.model.CustomField])(implicit context: gitbucket.core.controller.Context)
customFields: List[gitbucket.core.model.CustomField],
quickLoad: Boolean)(implicit context: gitbucket.core.controller.Context)
@import gitbucket.core.view.helpers
@gitbucket.core.html.main(s"Pull requests - ${repository.owner}/${repository.name}", Some(repository)){
@gitbucket.core.html.menu("pulls", repository){
<form method="POST" action="@context.path/@originRepository.owner/@originRepository.name/pulls/new" validate="true">
<div class="pullreq-info">
<div id="compare-edit">
@gitbucket.core.helper.html.dropdown(originRepository.owner + "/" + originRepository.name, "base fork", filter=("origin_repo", "Find Repository...")) {
@@ -54,14 +54,25 @@
}
<span class="error" id="error-requestBranch"></span>
</div>
@if(originRepository.branchList.contains(originId) && forkedRepository.branchList.contains(forkedId)){
<div class="check-conflict" style="display: none;">
<img src="@helpers.assets("/common/images/indicator.gif")"/> Checking...
</div>
}
</div>
@if(commits.nonEmpty && context.loginAccount.isDefined && originRepository.branchList.contains(originId) && forkedRepository.branchList.contains(forkedId)){
<div id="pull-request-form" style="margin-bottom: 20px;">
@if(originRepository.branchList.contains(originId) && forkedRepository.branchList.contains(forkedId)){
<div style="margin-top: 8px;">
<button type="button" class="btn btn-primary btn-sm" id="check-conflict-button">Check mergeability</button>
</div>
<div class="check-conflict" style="display: none;">
<img src="@helpers.assets("/common/images/indicator.gif")"/> Checking...
</div>
}
<div id="compare-state"
data-quick-load="@quickLoad"
data-has-valid-branches="@(originRepository.branchList.contains(originId) && forkedRepository.branchList.contains(forkedId))"
data-default-no-check="@context.settings.basicBehavior.compareNoCheckByDefault"
data-can-auto-mergecheck="@(context.loginAccount.isDefined && originRepository.branchList.contains(originId) && forkedRepository.branchList.contains(forkedId))"
style="display: none;"></div>
</div>
@if(commits.nonEmpty && context.loginAccount.isDefined && originRepository.branchList.contains(originId) && forkedRepository.branchList.contains(forkedId)){
<div id="pull-request-form" style="margin-bottom: 20px;">
<form method="POST" action="@context.path/@originRepository.owner/@originRepository.name/pulls/new" validate="true">
<div class="row">
<div class="col-md-9">
<span class="error" id="error-title"></span>
@@ -115,10 +126,15 @@
)
</div>
</div>
</div>
}
</form>
@if(commits.isEmpty){
</form>
</div>
}
@if(quickLoad){
<div class="panel panel-default" style="padding: 20px; background-color: #eee; text-align: center;">
<h4>Comparison is ready to load.</h4>
<span class="muted">Click <span class="strong">Check mergeability</span> to compute changes and show merge status.</span>
</div>
} else if(commits.isEmpty){
<div class="panel panel-default" style="padding: 20px; background-color: #eee; text-align: center;">
<h4>There isn't anything to compare.</h4>
<span class="strong">@originRepository.owner:@originId</span> and <span class="strong">@forkedRepository.owner:@forkedId</span> are identical.
@@ -194,6 +210,17 @@ $(function(){
$(function(){
var compareState = $('#compare-state');
var isQuickLoad = compareState.data('quick-load') === true || compareState.data('quick-load') === 'true';
var defaultNoCheck = compareState.data('default-no-check') === true || compareState.data('default-no-check') === 'true';
var autoMergecheck =
/(?:\?|&)check=true(?:&|$)/.test(window.location.search) || (!isQuickLoad && !defaultNoCheck);
var hasValidBranches =
compareState.data('has-valid-branches') === true || compareState.data('has-valid-branches') === 'true';
var canAutoMergecheck =
compareState.data('can-auto-mergecheck') === true || compareState.data('can-auto-mergecheck') === 'true';
var compareQuery = (defaultNoCheck || isQuickLoad) ? '?quick=true' : '';
function updateSelector(e){
e.parents('ul').find('i').attr('class', 'octicon');
e.find('i').addClass('octicon-check');
@@ -209,7 +236,7 @@ $(function(){
$.trim($('i.octicon-check').parents('a.origin-owner' ).data('owner')) + ':' +
$.trim($('i.octicon-check').parents('a.origin-owner' ).data('default-branch')) + '...' +
$.trim($('i.octicon-check').parents('a.forked-owner' ).data('owner')) + ':' +
$.trim($('i.octicon-check').parents('a.forked-branch').data('branch'));
$.trim($('i.octicon-check').parents('a.forked-branch').data('branch')) + compareQuery;
});
$('a.forked-owner').click(function(){
@@ -221,7 +248,7 @@ $(function(){
$.trim($('i.octicon-check').parents('a.origin-owner' ).data('owner')) + ':' +
$.trim($('i.octicon-check').parents('a.origin-branch').data('branch')) + '...' +
$.trim($('i.octicon-check').parents('a.forked-owner' ).data('owner')) + ':' +
$.trim($('i.octicon-check').parents('a.forked-owner' ).data('default-branch'));
$.trim($('i.octicon-check').parents('a.forked-owner' ).data('default-branch')) + compareQuery;
});
$('a.origin-branch, a.forked-branch').click(function(){
@@ -233,22 +260,44 @@ $(function(){
$.trim($('i.octicon-check').parents('a.origin-owner' ).data('owner')) + ':' +
$.trim($('i.octicon-check').parents('a.origin-branch').data('branch')) + '...' +
$.trim($('i.octicon-check').parents('a.forked-owner' ).data('owner')) + ':' +
$.trim($('i.octicon-check').parents('a.forked-branch').data('branch'));
$.trim($('i.octicon-check').parents('a.forked-branch').data('branch')) + compareQuery;
});
@if(context.loginAccount.isDefined && originRepository.branchList.contains(originId) && forkedRepository.branchList.contains(forkedId)){
if (isQuickLoad && hasValidBranches) {
$('#check-conflict-button').click(function(){
location.href = '@helpers.url(forkedRepository)/compare/' +
$.trim($('i.octicon-check').parents('a.origin-owner' ).data('owner')) + ':' +
$.trim($('i.octicon-check').parents('a.origin-branch').data('branch')) + '...' +
$.trim($('i.octicon-check').parents('a.forked-owner' ).data('owner')) + ':' +
$.trim($('i.octicon-check').parents('a.forked-branch').data('branch')) + '?check=true';
});
} else if (canAutoMergecheck) {
function checkConflict(from, to){
var button = $('#check-conflict-button');
button.attr('disabled', 'disabled');
$('.check-conflict').show();
$('.check-conflict').html('<img src="' + '@helpers.assets("/common/images/indicator.gif")' + '"/> Checking...');
$.get('@helpers.url(forkedRepository)/compare/' + from + '...' + to + '/mergecheck',
function(data){ $('.check-conflict').html(data); });
function(data){ $('.check-conflict').html(data); }
).fail(function(){
$('.check-conflict').html('<span class="strong" style="color: #bd2c00;">Failed to check mergeability.</span>');
}).always(function(){
button.removeAttr('disabled');
});
}
checkConflict(
$.trim($('i.octicon-check').parents('a.origin-owner' ).data('owner')) + ":" +
$.trim($('i.octicon-check').parents('a.origin-branch').data('branch')),
$.trim($('i.octicon-check').parents('a.forked-owner' ).data('owner')) + ":" +
$.trim($('i.octicon-check').parents('a.forked-branch').data('branch'))
);
$('#check-conflict-button').click(function(){
checkConflict(
$.trim($('i.octicon-check').parents('a.origin-owner' ).data('owner')) + ":" +
$.trim($('i.octicon-check').parents('a.origin-branch').data('branch')),
$.trim($('i.octicon-check').parents('a.forked-owner' ).data('owner')) + ":" +
$.trim($('i.octicon-check').parents('a.forked-branch').data('branch'))
);
});
if (autoMergecheck) {
$('#check-conflict-button').click();
}
}
});
</script>

View File

@@ -19,7 +19,7 @@
<a class="btn btn-default" href="#" id="edit">Edit</a>
}
@if(context.loginAccount.isDefined) {
<a class="btn btn-success" href="@helpers.url(repository)/compare">New pull request</a>
<a class="btn btn-success" href="@helpers.url(repository)/compare@if(context.settings.basicBehavior.compareNoCheckByDefault){?quick=true}">New pull request</a>
}
</div>
<div class="edit-title pull-right" style="display: none;">

View File

@@ -7,7 +7,7 @@
<div class="box-content" style="line-height: 20pt; margin-bottom: 6px; padding: 10px 6px 10px 10px; background-color: #fff9ea">
<strong><i class="menu-icon octicon octicon-git-branch"></i><span class="muted">@branch</span></strong>
<a class="pull-right btn btn-success" style="position: relative; top: -4px;"
href="@helpers.url(repository)/compare/@{parent.owner}:@{helpers.encodeRefName(parent.repository.defaultBranch)}...@{repository.owner}:@{helpers.encodeRefName(branch)}">Compare & pull request</a>
href="@helpers.url(repository)/compare/@{parent.owner}:@{helpers.encodeRefName(parent.repository.defaultBranch)}...@{repository.owner}:@{helpers.encodeRefName(branch)}@if(context.settings.basicBehavior.compareNoCheckByDefault){?quick=true}">Compare & pull request</a>
</div>
}
}

View File

@@ -64,7 +64,7 @@
helpers.urlEncode(parent) + ":" + helpers.encodeRefName(repository.repository.defaultBranch)
}.getOrElse {
helpers.encodeRefName(repository.repository.defaultBranch)
}}...@{helpers.encodeRefName(branch.name)}" class="btn btn-default btn-sm">@if(context.loginAccount.isDefined){Create pull request} else {Compare}</a>
}}...@{helpers.encodeRefName(branch.name)}@if(context.settings.basicBehavior.compareNoCheckByDefault){?quick=true}" class="btn btn-default btn-sm">@if(context.loginAccount.isDefined){Create pull request} else {Compare}</a>
}
@if(hasWritePermission){
<span style="margin-left: 8px;">

View File

@@ -55,6 +55,7 @@ trait ServiceSpecBase {
gravatar = false,
notification = false,
limitVisibleRepositories = false,
compareNoCheckByDefault = false,
),
ssh = Ssh(
enabled = false,

View File

@@ -68,6 +68,19 @@ class SystemSettingsServiceSpec extends AnyWordSpecLike with Matchers {
settings.ssh.bindAddress shouldNot be(empty)
settings.ssh.publicAddress shouldNot be(empty)
}
"default compare_no_check_by_default to false if not specified" in new SystemSettingsService {
val props = new Properties()
val settings = loadSystemSettings(props)
settings.basicBehavior.compareNoCheckByDefault shouldBe false
}
"read compare_no_check_by_default configuration when true" in new SystemSettingsService {
val props = new Properties()
props.setProperty("compare_no_check_by_default", "true")
val settings = loadSystemSettings(props)
settings.basicBehavior.compareNoCheckByDefault shouldBe true
}
}
"SshAddress" can {

View File

@@ -165,7 +165,8 @@ class AvatarImageProviderSpec extends AnyFunSpec {
),
gravatar = useGravatar,
notification = false,
limitVisibleRepositories = false
limitVisibleRepositories = false,
compareNoCheckByDefault = false
),
ssh = Ssh(
enabled = false,