JENKINS-75316: Make Git build summary clickable links #3874#3924
JENKINS-75316: Make Git build summary clickable links #3874#3924Amit-kumar80844 wants to merge 1 commit intojenkinsci:masterfrom
Conversation
|
Thanks for your interest in contributing to the git plugin. Please resolve the spotbugs issue that has been flagged in the GitHub checks. The existence of the spot bugs issue likely means that you didn't run the automated tests of the plugin. Please read and follow the contributing guide. It describes how to build the plugin, including running tests and performing static analysis with spotbugs. I'm not sure that I'm willing to accept an addition to the BuildData. We've had issues in the past with BuildData using more memory than it should. Adding another field to the BuildData seems like it might make memory use worse. I've asked GitHub Copilot to review the pull request as well. Sometimes it offers very helpful insights. |
There was a problem hiding this comment.
Pull request overview
This pull request adds clickable hyperlinks to the Git build summary by making revision, repository, and branch references linkable. The implementation persists the Git repository browser configuration in BuildData and uses it to generate appropriate links based on the configured browser type (e.g., GitHub, GitLab).
Changes:
- Added browser field and URL generation methods to BuildData for creating commit, ref, and repository links
- Modified GitSCM.copyBuildData to persist the browser configuration in BuildData
- Added getRefLink method to GitRepositoryBrowser base class with GithubWeb implementation
- Updated summary.jelly template to render hyperlinks when browser URLs are available
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/hudson/plugins/git/util/BuildData.java | Added browser field and three URL generation methods (getCommitUrl, getRefUrl, getRepositoryUrl) with sanitization and normalization logic |
| src/main/java/hudson/plugins/git/GitSCM.java | Modified copyBuildData to persist browser configuration into BuildData |
| src/main/java/hudson/plugins/git/browser/GitRepositoryBrowser.java | Added getRefLink method to base class, changed getUrl from final to non-final |
| src/main/java/hudson/plugins/git/browser/GithubWeb.java | Implemented getRefLink for GitHub branch links |
| src/main/resources/hudson/plugins/git/util/BuildData/summary.jelly | Updated template to render hyperlinks for revision, repository, and branches using new BuildData methods |
| src/test/java/hudson/plugins/git/util/BuildDataTest.java | Added tests for SSH URL handling, commit URL sanitization, and ref normalization with stub browser implementation |
| src/test/java/hudson/plugins/git/util/BuildDataBrowserTest.java | Added test for browser persistence getter/setter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private static class StubGitRepositoryBrowser extends GitRepositoryBrowser { | ||
| private static final long serialVersionUID = 1L; | ||
| private final String baseUrl; | ||
|
|
||
| StubGitRepositoryBrowser(String baseUrl) { | ||
| this.baseUrl = baseUrl; | ||
| } | ||
|
|
||
| @Override | ||
| public URL getUrl() throws IOException { | ||
| return new URL(baseUrl); | ||
| } | ||
|
|
||
| // Standard abstract method implementation | ||
| @Override | ||
| public URL getChangeSetLink(GitChangeSet changeSet) throws IOException { | ||
| return new URL(baseUrl.replaceAll("/$", "") + ".git/commit/" + changeSet.getId()); | ||
| } | ||
|
|
||
| // Standard abstract method implementation | ||
| @Override | ||
| public URL getDiffLink(GitChangeSet.Path path) throws IOException { | ||
| return null; | ||
| } | ||
|
|
||
| // Standard abstract method implementation | ||
| @Override | ||
| public URL getFileLink(GitChangeSet.Path path) throws IOException { | ||
| return null; | ||
| } | ||
|
|
||
| // Custom method for BuildData (removed @Override to avoid errors if not in base class) | ||
| public URL getChangeSetLink(String changeSetId) throws IOException { | ||
| return new URL(baseUrl.replaceAll("/$", "") + ".git/commit/" + changeSetId); | ||
| } | ||
|
|
||
| // Custom method for BuildData (removed @Override to avoid errors if not in base class) | ||
| public URL getRefLink(String ref) throws IOException { | ||
| return new URL(baseUrl.replaceAll("/$", "") + ".git/tree/" + ref); | ||
| } |
There was a problem hiding this comment.
The test stub intentionally adds .git to URLs (lines 637, 654, 659) to test sanitization, but this creates misleading tests because the actual GitRepositoryBrowser implementations (like GithubWeb) don't add .git to their URLs. The sanitization logic in BuildData (lines 318, 342) is therefore removing something that real browsers never add in the first place.
This makes the tests not representative of actual browser behavior. Consider either:
- Verifying that real GitRepositoryBrowser implementations actually need this .git sanitization, or
- Removing the .git sanitization logic if it's not needed for real browsers
If the sanitization is genuinely needed for some browsers, the test should document which browsers add .git and why this is necessary.
| <t:summary icon="symbol-git-logo plugin-git"> | ||
|
|
||
| <strong>${%Revision}</strong>: ${it.lastBuiltRevision.sha1.name()} | ||
| <j:set var="browser" value="${it.browser}"/> |
There was a problem hiding this comment.
Unused variable: The 'browser' variable is set on line 7 but never used in the template. The code directly accesses 'it.browser' through the method calls (getCommitUrl, getRefUrl, getRepositoryUrl), so this variable declaration is unnecessary and can be removed.
| <j:set var="browser" value="${it.browser}"/> |
| * The Git console browser (if any) used to link to the repository. | ||
| */ | ||
| @CheckForNull | ||
| public GitRepositoryBrowser browser; |
There was a problem hiding this comment.
The newly added 'browser' field is declared as public, unlike other public fields in this class (buildsByBranchName, lastBuild, scmName, remoteUrls) which are annotated with @SuppressFBWarnings for "PA_PUBLIC_PRIMITIVE_ATTRIBUTE". While browser is not a primitive type, having a public mutable field is generally discouraged for API design.
Consider making this field private and only exposing it through the getBrowser() and setBrowser() methods, similar to how the 'index' field is handled. This would be more consistent with modern Java best practices and prevent external modification of the browser reference.
| public GitRepositoryBrowser browser; | |
| private GitRepositoryBrowser browser; |
| public String getCommitUrl(String commitId) { | ||
| if (browser == null || commitId == null) { | ||
| return null; | ||
| } | ||
| try { | ||
| URL url = browser.getChangeSetLink(commitId); | ||
| if (url == null) { | ||
| return null; | ||
| } | ||
| return url.toExternalForm().replace(".git/commit/", "/commit/"); | ||
| } catch (IOException e) { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| public String getRefUrl(String ref) { | ||
| if (browser == null || ref == null) { | ||
| return null; | ||
| } | ||
| // Normalization | ||
| if (ref.startsWith("refs/remotes/")) { | ||
| String[] parts = ref.split("/", 4); | ||
| if (parts.length == 4) { | ||
| ref = parts[3]; | ||
| } | ||
| } else if (ref.startsWith("refs/heads/")) { | ||
| ref = ref.substring(11); | ||
| } | ||
| try { | ||
| URL url = browser.getRefLink(ref); | ||
| if (url == null) { | ||
| return null; | ||
| } | ||
| return url.toExternalForm().replace(".git/tree/", "/tree/"); | ||
| } catch (IOException e) { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| public String getRepositoryUrl() { | ||
| if (browser == null) { | ||
| return null; | ||
| } | ||
| try { | ||
| URL url = browser.getUrl(); | ||
| return url == null ? null : url.toExternalForm(); | ||
| } catch (IOException e) { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
The URL strings returned by getCommitUrl, getRefUrl, and getRepositoryUrl are used directly in href attributes in the Jelly template without any explicit XSS protection. While Jelly's escape-by-default='true' at line 1 of summary.jelly should provide protection, the URL generation methods perform string replacements (e.g., replace(".git/commit/", "/commit/")) on URL strings.
If a malicious repository browser implementation returns a URL containing special characters or JavaScript, the string replacement could potentially create an XSS vulnerability. Consider validating that the returned URLs have safe schemes (http/https) and don't contain JavaScript or other dangerous protocols.
| if (ref.startsWith("refs/remotes/")) { | ||
| String[] parts = ref.split("/", 4); | ||
| if (parts.length == 4) { | ||
| ref = parts[3]; | ||
| } | ||
| } else if (ref.startsWith("refs/heads/")) { |
There was a problem hiding this comment.
The ref normalization logic assumes that refs starting with "refs/remotes/" will always have exactly 4 parts when split by "/" (line 330-332). However, if a ref like "refs/remotes/origin" (only 3 parts) is passed, the normalization will be silently skipped and the full ref will be passed to the browser.
Consider handling this edge case more explicitly, either by:
- Logging a warning when the split doesn't produce the expected number of parts
- Falling back to a safe default normalization
- Documenting the expected ref format in a comment
Similarly, what happens if someone has a branch name containing slashes, like "refs/remotes/origin/feature/sub-feature"? The current logic would only strip "refs/remotes/origin/" leaving "feature/sub-feature", which might be correct, but should be documented.
| if (ref.startsWith("refs/remotes/")) { | |
| String[] parts = ref.split("/", 4); | |
| if (parts.length == 4) { | |
| ref = parts[3]; | |
| } | |
| } else if (ref.startsWith("refs/heads/")) { | |
| if (ref.startsWith("refs/remotes/")) { | |
| // Expected format: refs/remotes/<remote>/<branch-path> | |
| // where <branch-path> may itself contain '/' (e.g. feature/sub-feature). | |
| // We strip the "refs/remotes/<remote>/" prefix and keep the branch path. | |
| String[] parts = ref.split("/", 4); | |
| if (parts.length == 4) { | |
| ref = parts[3]; | |
| } else { | |
| // Unexpected remote ref format; skip normalization but log for diagnostics. | |
| Logger.getLogger(BuildData.class.getName()).log( | |
| Level.FINE, | |
| "Unexpected remote ref format ''{0}''; skipping normalization", | |
| ref | |
| ); | |
| } | |
| } else if (ref.startsWith("refs/heads/")) { | |
| // Expected format: refs/heads/<branch-path>; strip the "refs/heads/" prefix. |
| <j:if test="${branch!=''}"> | ||
| <li>${branch.name}</li> | ||
| <j:set var="branchLink" value="${it.getRefUrl(branch.name)}"/> | ||
| <j:choose> |
There was a problem hiding this comment.
Inconsistent indentation: This line uses 20 spaces for indentation while it should use 16 spaces to match the indentation of the j:when and j:otherwise tags at lines 40 and 43.
| <j:choose> | |
| <j:choose> |
| buildData = base.clone(); | ||
| buildData.setScmName(scmName); | ||
| return buildData; |
There was a problem hiding this comment.
Critical bug: The browser is only set when base is null (line 2018), but when base is not null and gets cloned (lines 2014-2016), the method returns early without setting the browser. This means that for all builds after the first one, the browser will not be persisted in the BuildData, and the clickable links feature will not work.
The setBrowser call on line 2018 should be moved before the early return on line 2016, or the return statement should be removed and the setBrowser call should be placed after the if-else block to ensure the browser is always set on the BuildData before returning.
| buildData = base.clone(); | |
| buildData.setScmName(scmName); | |
| return buildData; | |
| buildData = base.clone(); | |
| buildData.setScmName(scmName); |
| public URL getChangeSetLink(String changeSetId) throws IOException { | ||
| return new URL(baseUrl.replaceAll("/$", "") + ".git/commit/" + changeSetId); | ||
| } | ||
|
|
||
| // Custom method for BuildData (removed @Override to avoid errors if not in base class) |
There was a problem hiding this comment.
The getChangeSetLink(String) method at line 653 doesn't call the superclass method. The base class GitRepositoryBrowser.getChangeSetLink(String) at line 103-108 checks if the commitId is null or blank before delegating to getChangeSetLink(GitChangeSet). This stub bypasses that null/blank check and creates a different implementation pattern.
While this works for testing, it would be better to add @OverRide and ensure the stub properly extends the base class behavior, or remove this method entirely and rely on the base class implementation which will call getChangeSetLink(GitChangeSet) anyway.
| public URL getChangeSetLink(String changeSetId) throws IOException { | |
| return new URL(baseUrl.replaceAll("/$", "") + ".git/commit/" + changeSetId); | |
| } | |
| // Custom method for BuildData (removed @Override to avoid errors if not in base class) |
| } | ||
|
|
||
| public final URL getUrl() throws IOException { | ||
| public URL getUrl() throws IOException { |
There was a problem hiding this comment.
The getUrl() method visibility was changed from 'final' to non-final. This is a breaking API change that allows subclasses to override this method. While this change appears intentional to allow the StubGitRepositoryBrowser test implementation to override it, there's no clear justification in the PR description for why this API change is necessary.
The existing implementation already allows configuration through the constructor, and the test stub could work by passing the baseUrl to the super constructor instead of overriding getUrl(). Consider whether this breaking change is truly necessary, or if the test can be refactored to avoid it.
| public URL getUrl() throws IOException { | |
| public final URL getUrl() throws IOException { |
| this.baseUrl = baseUrl; | ||
| } | ||
|
|
||
| @Override | ||
| public URL getUrl() throws IOException { | ||
| return new URL(baseUrl); | ||
| } |
There was a problem hiding this comment.
The StubGitRepositoryBrowser constructor doesn't call super(baseUrl), so the parent class's 'url' field is never initialized. This means getRepoUrl() would return null and the base class's getUrl() would fail if called. While the stub overrides getUrl() to return the baseUrl directly, this creates an inconsistent state.
For proper testing, the constructor should call super(baseUrl) instead of storing baseUrl in a separate field, or the constructor should be changed to:
StubGitRepositoryBrowser(String baseUrl) {
super(baseUrl);
this.baseUrl = baseUrl;
}
This ensures the base class is properly initialized even if it's not strictly necessary for these tests.
| this.baseUrl = baseUrl; | |
| } | |
| @Override | |
| public URL getUrl() throws IOException { | |
| return new URL(baseUrl); | |
| } | |
| super(baseUrl); | |
| this.baseUrl = baseUrl; | |
| } | |
| @Override | |
| public URL getUrl() throws IOException { | |
| return new URL(baseUrl); |
|
Thank you for your valuable feedback. I will carefully read all the points that creates issue and solve it after deep research at the earliest to ensure full comprehension. |
2 participants
Description
This pull request improves the Git Build Data summary by adding clickable hyperlinks for the revision, repository, and branch references. This allows users to directly navigate to diffs, repository pages, and branch views from the Jenkins build summary UI. The implementation supports both HTTP and SSH repository configurations and only renders links when a Git repository browser is configured.
GitHub Issue: #3874
Jira: https://issues.jenkins.io/browse/JENKINS-75285
Changes:
1.GitSCM.java:
2.BuildData.java:
These methods:
*Safely handle IOException from the browser.•Sanitize URLs by stripping .git suffixes to ensure
compatibility with web interfaces (e.g., GitHub).
*Normalize branch names (stripping refs/remotes/origin/ or refs/heads/) so links point to the correct branch view.
*Handle the conversion of SSH remote URLs to HTTP browser URLs via the configured browser.
3.summary.jelly:
4.BuildDataTest.java:
Testing done
Executed unit tests locally.
Added new automated tests to cover browser link generation logic.
Verified clickable links render correctly in Jenkins build summary UI.
Submitter checklist