[JENKINS-49757] Remove redundant fetch #904
There are no files selected for viewing
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -127,8 +127,11 @@ public static void createRepo() throws Exception { | |
| String initialImpl = random.nextBoolean() ? "git" : "jgit"; | ||
| GitClient gitClient = Git.with(TaskListener.NULL, new EnvVars()).in(repoRoot).using(initialImpl).getClient(); | ||
| gitClient.init_().workspace(repoRoot.getAbsolutePath()).execute(); | ||
| String[] expectedResult = {""}; | ||
| CliGitCommand gitCmd = new CliGitCommand(gitClient, "config", "user.name", "ChangeSet Truncation Test"); | ||
| assertThat(gitCmd.run(), is(expectedResult)); | ||
| gitCmd = new CliGitCommand(gitClient, "config", "user.email", "[email protected]"); | ||
| assertThat(gitCmd.run(), is(expectedResult)); | ||
| } | ||
|
|
||
| private ObjectId commitOneFile(GitClient gitClient, final String commitSummary) throws Exception { | ||
|
|
||
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,6 +83,8 @@ | |
| import java.util.*; | ||
| import java.util.logging.Level; | ||
| import java.util.logging.Logger; | ||
| import java.util.regex.Pattern; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| import org.eclipse.jgit.transport.RemoteConfig; | ||
| import static org.hamcrest.MatcherAssert.*; | ||
|
|
@@ -338,6 +340,142 @@ public void testSpecificRefspecs() throws Exception { | |
| build(projectWithFoo, Result.SUCCESS, commitFile1); | ||
| } | ||
|
|
||
| /** | ||
| * This test confirms the behaviour of avoiding the second fetch in GitSCM checkout() | ||
| **/ | ||
| @Test | ||
| @Issue("JENKINS-56404") | ||
| public void testAvoidRedundantFetch() throws Exception { | ||
| List<UserRemoteConfig> repos = new ArrayList<>(); | ||
| repos.add(new UserRemoteConfig(testRepo.gitDir.getAbsolutePath(), "origin", "+refs/heads/*:refs/remotes/*", null)); | ||
|
|
||
| /* Without honor refspec on initial clone */ | ||
| FreeStyleProject projectWithMaster = setupProject(repos, Collections.singletonList(new BranchSpec("master")), null, false, null); | ||
| if (random.nextBoolean()) { | ||
| /* Randomly enable shallow clone, should not alter test assertions */ | ||
| CloneOption cloneOptionMaster = new CloneOption(false, null, null); | ||
| cloneOptionMaster.setDepth(1); | ||
|
There was a problem hiding this comment. I fail to understand, why would need a shallow clone to improve coverage when we already have a full normal clone? There was a problem hiding this comment. Good question. One branch was not being reached when a cloneOption extension is detected but does not have honor refspec enabled. There was a problem hiding this comment. Okay. I didn't consider other CloneOptions. If honor refspec is false, with avoiding the second fetch, we will also avoid other clone options like Shallow Clone, Disable tags, Reference a repo and timeout. The good news is that the first fetch is capable to execute all of these options if we miss the second fetch. It should not make any difference to the user's expectation on git repository information after the checkout. |
||
| ((GitSCM) projectWithMaster.getScm()).getExtensions().add(cloneOptionMaster); | ||
| } | ||
|
|
||
| // create initial commit | ||
| final String commitFile1 = "commitFile1"; | ||
| commit(commitFile1, johnDoe, "Commit in master"); | ||
|
|
||
| FreeStyleBuild build = build(projectWithMaster, Result.SUCCESS); | ||
|
|
||
| assertRedundantFetchIsTrue(build, "+refs/heads/*:refs/remotes/origin/*"); | ||
| } | ||
|
|
||
| /** | ||
| * After avoiding the second fetch call in retrieveChanges(), this test verifies there is no data loss by fetching a repository | ||
| * (git init + git fetch) with a narrow refspec but without CloneOption of honorRefspec = true on initial clone | ||
| * First fetch -> wide refspec | ||
| * Second fetch -> narrow refspec (avoided) | ||
| **/ | ||
| @Test | ||
| @Issue("JENKINS-56404") | ||
| public void testAvoidRedundantFetchWithoutHonorRefSpec() throws Exception { | ||
| List<UserRemoteConfig> repos = new ArrayList<>(); | ||
| repos.add(new UserRemoteConfig(testRepo.gitDir.getAbsolutePath(), "origin", "+refs/heads/foo:refs/remotes/foo", null)); | ||
|
|
||
| /* Without honor refspec on initial clone */ | ||
| FreeStyleProject projectWithMaster = setupProject(repos, Collections.singletonList(new BranchSpec("master")), null, false, null); | ||
| if (random.nextBoolean()) { | ||
| /* Randomly enable shallow clone, should not alter test assertions */ | ||
| CloneOption cloneOptionMaster = new CloneOption(false, null, null); | ||
| cloneOptionMaster.setDepth(1); | ||
| ((GitSCM) projectWithMaster.getScm()).getExtensions().add(cloneOptionMaster); | ||
| } | ||
|
|
||
| // create initial commit | ||
| final String commitFile1 = "commitFile1"; | ||
| commit(commitFile1, johnDoe, "Commit in master"); | ||
| // Add another branch 'foo' | ||
| git.branch("foo"); | ||
| git.checkout().branch("foo"); | ||
| commit(commitFile1, johnDoe, "Commit in foo"); | ||
|
|
||
| // Build will be success because the initial clone disregards refspec and fetches all branches | ||
| FreeStyleBuild build = build(projectWithMaster, Result.SUCCESS); | ||
| FilePath childFile = returnFile(build); | ||
|
|
||
| if (childFile != null) { | ||
| // assert that no data is lost by avoidance of second fetch | ||
| assertThat("master branch was not fetched", childFile.readToString(), containsString("master")); | ||
| assertThat("foo branch was not fetched", childFile.readToString(), containsString("foo")); | ||
| } | ||
|
|
||
| String wideRefSpec = "+refs/heads/*:refs/remotes/origin/*"; | ||
| assertRedundantFetchIsTrue(build, wideRefSpec); | ||
|
|
||
| assertThat(build.getResult(), is(Result.SUCCESS)); | ||
| } | ||
|
|
||
| /** | ||
| * After avoiding the second fetch call in retrieveChanges(), this test verifies there is no data loss by fetching a | ||
| * repository(git init + git fetch) with a narrow refspec with CloneOption of honorRefspec = true on initial clone | ||
| * First fetch -> narrow refspec (since refspec is honored on initial clone) | ||
| * Second fetch -> narrow refspec (avoided) | ||
| **/ | ||
| @Test | ||
| @Issue("JENKINS-56404") | ||
| public void testAvoidRedundantFetchWithHonorRefSpec() throws Exception { | ||
| List<UserRemoteConfig> repos = new ArrayList<>(); | ||
| String refSpec = "+refs/heads/foo:refs/remotes/foo"; | ||
| repos.add(new UserRemoteConfig(testRepo.gitDir.getAbsolutePath(), "origin", refSpec, null)); | ||
|
|
||
| /* With honor refspec on initial clone */ | ||
| FreeStyleProject projectWithMaster = setupProject(repos, Collections.singletonList(new BranchSpec("master")), null, false, null); | ||
| CloneOption cloneOptionMaster = new CloneOption(false, null, null); | ||
| cloneOptionMaster.setHonorRefspec(true); | ||
| ((GitSCM)projectWithMaster.getScm()).getExtensions().add(cloneOptionMaster); | ||
|
|
||
| // create initial commit | ||
| final String commitFile1 = "commitFile1"; | ||
| commit(commitFile1, johnDoe, "Commit in master"); | ||
| // Add another branch 'foo' | ||
| git.branch("foo"); | ||
| git.checkout().branch("foo"); | ||
| commit(commitFile1, johnDoe, "Commit in foo"); | ||
|
|
||
| // Build will be failure because the initial clone regards refspec and fetches branch 'foo' only. | ||
| FreeStyleBuild build = build(projectWithMaster, Result.FAILURE); | ||
|
|
||
| FilePath childFile = returnFile(build); | ||
| assertNotNull(childFile); | ||
| // assert that no data is lost by avoidance of second fetch | ||
| assertThat(childFile.readToString(), not(containsString("master"))); | ||
| assertThat("foo branch was not fetched", childFile.readToString(), containsString("foo")); | ||
| assertRedundantFetchIsTrue(build, refSpec); | ||
|
|
||
| assertThat(build.getResult(), is(Result.FAILURE)); | ||
| } | ||
|
|
||
| // Checks if the second fetch is being avoided | ||
| private void assertRedundantFetchIsTrue(FreeStyleBuild build, String refSpec) throws IOException { | ||
| List<String> values = build.getLog(Integer.MAX_VALUE); | ||
|
|
||
| //String fetchArg = " > git fetch --tags --force --progress -- " + testRepo.gitDir.getAbsolutePath() + argRefSpec + " # timeout=10"; | ||
| Pattern fetchPattern = Pattern.compile(".* git.* fetch .*"); | ||
| List<String> fetchCommands = values.stream().filter(fetchPattern.asPredicate()).collect(Collectors.toList()); | ||
|
|
||
| // After the fix, git fetch is called exactly once | ||
| assertThat("Fetch commands were: " + fetchCommands, fetchCommands, hasSize(1)); | ||
| } | ||
|
|
||
| // Returns the file FETCH_HEAD found in .git | ||
| private FilePath returnFile(FreeStyleBuild build) throws IOException, InterruptedException { | ||
| List<FilePath> files = build.getProject().getWorkspace().list(); | ||
| FilePath resultFile = null; | ||
| for (FilePath s : files) { | ||
| if(s.getName().equals(".git")) { | ||
| resultFile = s.child("FETCH_HEAD"); | ||
| } | ||
| } | ||
| return resultFile; | ||
| } | ||
|
|
||
| /** | ||
| * This test and testSpecificRefspecs confirm behaviors of | ||
| * refspecs on initial clone. Without the CloneOption to honor refspec, all | ||
|
|
@@ -837,36 +975,36 @@ public void testBasicExcludedRegion() throws Exception { | |
| assertFalse("scm polling should not detect any more changes after build", project.poll(listener).hasChanges()); | ||
| } | ||
|
|
||
| private int findLogLineStartsWith(List<String> buildLog, String initialString) { | ||
| int logLine = 0; | ||
| for (String logString : buildLog) { | ||
| if (logString.startsWith(initialString)) { | ||
| return logLine; | ||
| } | ||
| logLine++; | ||
| } | ||
| return -1; | ||
| } | ||
|
|
||
| @Test | ||
| public void testCleanBeforeCheckout() throws Exception { | ||
| FreeStyleProject p = setupProject("master", false, null, null, "Jane Doe", null); | ||
| ((GitSCM)p.getScm()).getExtensions().add(new CleanBeforeCheckout()); | ||
|
|
||
| /* First build should not clean, since initial clone is always clean */ | ||
| final String commitFile1 = "commitFile1"; | ||
| commit(commitFile1, johnDoe, janeDoe, "Commit number 1"); | ||
| final FreeStyleBuild firstBuild = build(p, Result.SUCCESS, commitFile1); | ||
| assertThat(firstBuild.getLog(50), not(hasItem("Cleaning workspace"))); | ||
| /* Second build should clean, since first build might have modified the workspace */ | ||
| final String commitFile2 = "commitFile2"; | ||
| commit(commitFile2, johnDoe, janeDoe, "Commit number 2"); | ||
| final FreeStyleBuild secondBuild = build(p, Result.SUCCESS, commitFile2); | ||
| List<String> secondLog = secondBuild.getLog(50); | ||
| assertThat(secondLog, hasItem("Cleaning workspace")); | ||
| int cleaningLogLine = findLogLineStartsWith(secondLog, "Cleaning workspace"); | ||
| int fetchingLogLine = findLogLineStartsWith(secondLog, "Fetching upstream changes from "); | ||
| assertThat("Cleaning should happen before fetch", cleaningLogLine, is(lessThan(fetchingLogLine))); | ||
| } | ||
|
|
||
| @Issue("JENKINS-8342") | ||
|
|
||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if clause didn't help me to see clearer what you meant. While having a "!" in an
ifclause is perfectly fine, the "!" in theif-elsemight cause confusion. I would have expectedwhile this piece of code is
My personal preference is to have the first approach in terms of readability. But it's just a matter of personal taste, so take this advice as it is, just an advice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great suggestion @fcojfernandez. I have added this in c2e097d.