Repository navigation
Conversation
jimklimov
force-pushed
the
refrepo-args
branch
2 times, most recently
from
December 8, 2020 12:34
191ca38 to
694ee90
Compare
jimklimov
marked this pull request as draft
December 10, 2020 01:43
…RL} to replace by url => funny dir subtree in filesystem
…atibleGitAPIImpl.java so its logic (expected to grow in complexity) can be shared by both JGitAPIImpl.java and CliGitAPIImpl.java
…d ref-repos in submodule checkouts (only CliGitAPIImpl.java has it)
… findParameterizedReferenceRepository()
…SHA256} reference repos
… name for the mirrored repo
…6() with real sanity checks
…(): do not bother normalizing the URL if the string is not with supported suffix
…intsToLocal*Mirror() with custom paths and bare vs workspace repos
…6() to find expected paths
…refactor getObjectPath(referencePath) to check on git dirs elsewhere later
…erenceRepository() and isParameterizedReferenceRepository() taking a File reference (not only a String) object
…ceRepository() processing to stderr
… keep original reference intact, and as indicator to recreate referencePath object once for many cases
…256_FALLBACK suffixes for using unsuffixed directory if expanded path points nowhere useful
…ith fallback modes of operation
…d GIT_SUBMODULES_FALLBACK support
There was a problem hiding this comment.
Pull request overview
This PR introduces support for parameterized reference repository paths (fan-out layouts) so a single configured ref-repo base can resolve to smaller per-URL (or per-hash) reference repositories, reducing contention on a combined “monolith” refrepo. It also extends the GitClient API to query all remote URLs and to create same-implementation clients for other worktrees.
Changes:
- Add parameterized refrepo resolution (e.g.,
${GIT_URL_BASENAME},${GIT_URL_SHA256},${GIT_SUBMODULES}+ fallback variants) and helpers likegetObjectsFile()/ URL normalization. - Extend
GitClientwithgetRemoteUrls(),getRemotePushUrls(), andnewGit(String); implement in CLI/JGit/remoting wrappers. - Add / adjust tests and helpers to exercise parameterized refrepo behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/org/jenkinsci/plugins/gitclient/WorkspaceWithRepo.java | Test helper updates for configurable mirror directory names and path normalization. |
| src/test/java/org/jenkinsci/plugins/gitclient/GitClientCloneTest.java | New tests for parameterized refrepo modes and refactored alternates-file assertions. |
| src/main/java/org/jenkinsci/plugins/gitclient/RemoteGitImpl.java | Delegate new GitClient API methods through the remoting proxy. |
| src/main/java/org/jenkinsci/plugins/gitclient/LegacyCompatibleGitAPIImpl.java | Core implementation of parameterized refrepo resolution, URL normalization, and repo-scanning helpers. |
| src/main/java/org/jenkinsci/plugins/gitclient/JGitAPIImpl.java | Implement new APIs and parameterized refrepo handling for the JGit path. |
| src/main/java/org/jenkinsci/plugins/gitclient/GitClient.java | Define new GitClient interface methods (getRemoteUrls, getRemotePushUrls, newGit). |
| src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java | Implement new APIs and parameterized refrepo handling for the CLI path; improve error message with workdir. |
| src/main/java/hudson/plugins/git/GitAPI.java | Route newGit(String) to CLI or JGit implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
|
Thanks @MarkEWaite for the added review points. Most of them did hit what Claude already suggested (so far shelved in the other branch), but a few comments seem new to me. Hope to review them closer later. |
Contributor
Author
|
The plugin built with recent changes at least was not caught misbehaving in the past week or so, updating this PR source branch with new tests and fixes for older implementation bugs found by AI (Claude, sponsored by dayjob - Provys). |
…il methods Addresses @MarkEWaite's Sep 2022 review request for test coverage of the new methods introduced by the PR: - GitClientTest.java: five new parameterized tests (run for git/jgit/jgitapache) covering getRemoteUrls() with one and two remotes, getRemotePushUrls() with and without an explicit pushurl configured, and newGit() verifying same-class client creation for a different directory. - LegacyCompatibleGitAPIImplStaticTest.java: new test class for the static utility methods that need no git implementation parameter — getObjectsFile() for null/missing/normal-workspace/bare-repo/submodule-pointer inputs, isParameterizedReferenceRepository() for all six recognized magic suffixes and negative cases, and normalizeGitUrl() for trailing-slash stripping, .git suffix handling, scheme inference (file://, ssh://), and URL canonicalization.
testNewGit: The assertion comparing getClass() of the gitClient field (which
is a hudson.plugins.git.GitAPI wrapper) to the class returned by newGit()
(which is the unwrapped CliGitAPIImpl) always fails. Replace the class-equality
check with the functionally meaningful assertion: the returned client can
actually init a git repo in the specified directory.
isParameterizedReferenceRepository_fileOverload: On Windows, File.getPath()
uses backslash separators while the parameterized-suffix checks use forward
slashes, so new File("/.../${GIT_URL_BASENAME}").getPath() does not end with
"/${GIT_URL_BASENAME}". Restrict the File-overload test to platform-independent
cases (null → false, non-parameterized → false).
…am; drop redundant public on interface methods GitClientCloneTest.assertAlternateFilePointsToLocalMirror(): the method takes a _testGitDir parameter but line 708 read the alternates file using the class field testGitDir instead, silently ignoring the parameter. Any caller passing a different directory would assert the wrong file content. Fix: use new File(_testGitDir).toPath() so the parameter is honoured. GitClient.java: interface methods are implicitly public; the explicit 'public' modifier on getRemoteUrls() and getRemotePushUrls() was inconsistent with every other method declaration in the interface. Remove the redundant modifier.
…t<List<String>>
String[] uses identity-based equals/hashCode, so a LinkedHashSet<String[]>
never actually removes duplicate entries — every new String[] {} is a
distinct object. The comment at the merge point ("Hopefully the Set should
deduplicate entries if something matched twice") was therefore never true.
Change the element type from String[] to List<String> throughout
getSubmodulesUrls() and its callers. List.equals() / List.hashCode() are
content-based, so the LinkedHashSet now correctly deduplicates entries
with the same (dirname, url, urlNormalized, remoteName) tuple.
Mechanical changes:
- LinkedHashSet<String[]> → LinkedHashSet<List<String>> (10 sites)
- new String[] {a,b,c,d} → List.of(a,b,c,d) (4 sites)
- entry[N] → entry.get(N) (9 sites)
- for (String[] ...) → for (List<String> ...) (4 sites)
- Javadoc updated: [N] → get(N)
…lesUrls() and findParameterizedReferenceRepository() [JENKINS-64383] Signed-off-by: Evgeny Klimov <klimov@provys.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
Oh those dalmatians! :} |
Signed-off-by: Jim Klimov <jimklimov+jenkinsci@gmail.com>
Contributor
Author
|
Discussion on Gitter reminded me of a loose thread in this world, needed to pull it: by now we have (along with https://github.com/jimklimov/git-refrepo-scripts helper to maintain the refrepo location) a way to store a map of URLs to dirs, speeding up the checkouts and reducing log noise. Finally got around to implementing the missing bits. Another fast-track improvement is to parse the git config files directly (via jGit) to look for remote definitions and not waste time and resources to spawn numerous git processes for that by default (only do so if such parsing attempt fails). Thanks again to Claude for heavy-lifting, especially around tests. And code. And javadocs. Going back to sip coffee and test the result... |
…fast-track correct refrepo discovery [JENKINS-64383] The https://github.com/jimklimov/git-refrepo-scripts helper can maintain a file which maps Git URLs to refrepo directory names, and now the plugin can use that file to speed up discovery of the correct dir with less noise in build log when many projects are cached and had to all/many be walked until now. Signed-off-by: Jim Klimov <jimklimov+jenkinsci@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…forking git for remote lookups during refrepo walk [JENKINS-64383] getSubmodulesUrls() previously called GitClient.getRemoteUrls() for every candidate directory while walking a fanned-out refrepo, which forks a `git config --local --list` subprocess (CliGit) or opens a full JGit Repository (JGit) per directory. Add getRemoteUrlsFast(), which first tries reading each dir's "config" file directly and parsing it in memory with JGit's own Config/RemoteConfig classes (no subprocess, no Repository object), falling back to the previous GitClient-based lookup when the config is missing, unreadable, unparseable, or uses an include/includeIf directive we do not attempt to resolve here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Jim Klimov <jimklimov+jenkinsci@gmail.com>
Contributor
Author
|
The updated code seems to find the correct refrepo faster (at least much less noisily) when there are many fanned-out ref repo dirs to look at - if done pedantically :)
|
Signed-off-by: Jim Klimov <jimklimov+jenkinsci@gmail.com>
…ame() and fix replaceAll() path corruption it exposed [JENKINS-64383] Code review (PR jenkinsci#644, discussion_r3595155033) flagged that this test only asserted a log message was written and left the real functional checks (alternates file, branches, no stray objects) commented out behind a TODO, so a regression in ${GIT_URL_BASENAME} resolution would still pass. Completing the test surfaced a real bug in LegacyCompatibleGitAPIImpl.findParameterizedReferenceRepository(): the computed basename was passed as the *replacement* argument to String.replaceAll(), but that argument is interpreted as a replacement pattern (backslashes/`$` are escape sequences), not literal text. Any basename containing a backslash - guaranteed for local Windows paths, and possible in principle for any URL - was silently mangled (backslashes stripped), corrupting the resolved reference-repo directory. Fixed all six affected call sites (GIT_URL_BASENAME, its _FALLBACK, and GIT_SUBMODULES/_FALLBACK) by wrapping the replacement with Matcher.quoteReplacement(). The test itself now clones from a forward-slash-spelled local mirror URL (portable on every OS, and avoids the Windows case where a backslash-only path has no '/' to derive a basename from at all), pre-creates the exact refrepo subdirectory production will resolve to - mirroring the existing test_clone_reference_parameterized_sha256() pattern - and re-enables the real assertions instead of the commented-out block. Verified: LegacyCompatibleGitAPIImpl{,JGit,Static}Test (72 tests) and the full GitClientCloneTest suite (69 tests across CliGit/JGit/JGitApache) all pass after `mvn spotless:apply`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
@MarkEWaite : thanks again for earlier reviews, some potential bugs have been put to rest as a result of new scrutiny. |
Contributor
Author
|
Gentle bump to maintainers/reviewers, now that summer breaks are over :) |
Contributor
Author
|
@MarkEWaite Are there any blockers remaining here, please? |
Contributor
I'm unavailable to review the pull request for at least several months. I'll be traveling with my wife for 2+ weeks and then have other priorities that need my attention. Other maintainers of the plugin are welcome to review if they have time. |
Contributor
Author
|
I see, safe travels and have a good time - and a special thanks for the very prompt reply despite the circumstances! @git-client-plugin-developers then, I'm eagerly awaiting your positive answer! ;) |
…tag into refrepo-args Signed-off-by: Jim Klimov <jimklimov+jenkinsci@gmail.com>
Contributor
Author
|
Some CI infra trouble, will try to close and reopen. UPDATE: Unlike earlier experience, this did not need 10 minutes before reopening. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JENKINS-64383 - combined refrepo became our bottleneck
As detailed in the JIRA issue, our heavy use of a single combined reference repository made it more a bottleneck and cause of job timeouts than a speedup and reliability improvement which it once was. This PR explores a way to keep the single point of configuration of the reference repository directory, suffixed with some "magic variable" to substitute a path to subdirectory with a smaller-scope reference repository for a particular source Git URL. On file systems with symlinks it is possible to maintain several such names that would point to the same directory, for closely-related repositories or different URLs of the same repository.
This PoC introduces trivial support for reference repository paths ending with
/${GIT_URL}to replace by url => funny dir subtree in filesystem. Its limitation at the moment is that the URL is pasted in verbatim - this works for Linux and Unix like systems that only forbid a 0x00 and a slash from being characters in a filename, and slash suits us as a directory subtree separator. This code likely won't run on Windows as is (colon inhttps:and likely other chars - Microsoft has an extensive list of invalid chars).The next ideas, commented but not yet PoCed, are to either escape such characters (non-ASCII and offensive to at least one popular filesystem), or convert URLs into base64 strings or sha/md5/... hashes. Using submodules and finding a way to map several URLs to a certain submodule might be a good idea if they keep indexes separately. This all can be built on top of this PoCed code by introducing further suffixes and handling for them.
It was tested on a MultiBranch pipeline job, where an original definition of the reference repository was suffixed with the new magic string, yielding
/home/abuild/jenkins-gitcache/${GIT_URL}(verbatim in "Advanced clone behaviours"). During the checkout into a wiped workspace, with this plugin variant installed:This completed quickly, much faster than the usual checkout with huge refrepo in original
/home/abuild/jenkins-gitcache/, and did automatically find the "funny"/home/abuild/jenkins-gitcache/https://github.com/zeromq/czmq.gitdirectory prepared with the single repo's reference cache:DOCS NOTE: With 2.36.x and newer Git versions, if your reference repository maintenance script runs as a different user account than the Jenkins server (or Jenkins agent), safety checks about
safe.directory(see https://github.blog/2022-04-18-highlights-from-git-2-36/) can be disabled by configuring each such user account:UPDATE: My repository at https://github.com/jimklimov/git-refrepo-scripts provides the shell scripts and Jenkinsfile jobs I use to maintain the servers using this modification of the Git Client plugin. One of the jobs there allows to automatically discover and register Git repositories used by known builds on the server it runs on (might run daily or so), and another can run more regularly to update the known refrepos.