From 8d58567eb417b1c18c62198f5bfe65d7c7bb85be Mon Sep 17 00:00:00 2001 From: Rishabh Budhouliya Date: Tue, 25 Feb 2020 02:52:42 +0530 Subject: [PATCH 01/26] Add flag to avoid redundant fetch in GitSCM checkout --- src/main/java/hudson/plugins/git/GitSCM.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/main/java/hudson/plugins/git/GitSCM.java b/src/main/java/hudson/plugins/git/GitSCM.java index 9aa053790b..2e3159df88 100644 --- a/src/main/java/hudson/plugins/git/GitSCM.java +++ b/src/main/java/hudson/plugins/git/GitSCM.java @@ -1093,6 +1093,7 @@ public EnvVars getEnvironment() { private void retrieveChanges(Run build, GitClient git, TaskListener listener) throws IOException, InterruptedException { final PrintStream log = listener.getLogger(); + boolean redundantFetchCheck = false; List repos = getParamExpandedRepos(build, listener); if (repos.isEmpty()) return; // defensive check even though this is an invalid configuration @@ -1106,6 +1107,7 @@ private void retrieveChanges(Run build, GitClient git, TaskListener listener) th log.println("Cloning the remote Git repository"); RemoteConfig rc = repos.get(0); + redundantFetchCheck = true; try { CloneCommand cmd = git.clone_().url(rc.getURIs().get(0).toPrivateString()).repositoryName(rc.getName()); for (GitSCMExtension ext : extensions) { @@ -1119,6 +1121,9 @@ private void retrieveChanges(Run build, GitClient git, TaskListener listener) th } for (RemoteConfig remoteRepository : repos) { + if (remoteRepository == repos.get(0) && redundantFetchCheck){ + continue; + } try { fetchFrom(git, build, listener, remoteRepository); } catch (GitException ex) { From 23158ea1289900a4bfda8f3275eb9ff97d4346a5 Mon Sep 17 00:00:00 2001 From: Rishabh Budhouliya Date: Tue, 25 Feb 2020 13:56:08 +0530 Subject: [PATCH 02/26] Automated test to check redundance fetch call: testRedundantFetchCallFromLogs --- .../java/hudson/plugins/git/GitSCMTest.java | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/test/java/hudson/plugins/git/GitSCMTest.java b/src/test/java/hudson/plugins/git/GitSCMTest.java index df5e9c16df..9b6a81bb65 100644 --- a/src/test/java/hudson/plugins/git/GitSCMTest.java +++ b/src/test/java/hudson/plugins/git/GitSCMTest.java @@ -2738,6 +2738,38 @@ public void testCommitMessageIsPrintedToLogs() throws Exception { assertThat(values, hasItem("Commit message: \"test commit\"")); } + @Issue("JENKINS-49757") + @Test + public void testRedundantFetchCallFromLogs() throws Exception { + sampleRepo.init(); + sampleRepo.write("file", "v1"); + sampleRepo.git("commit", "--all", "--message=test commit"); + FreeStyleProject p = setupSimpleProject("master"); + Run run = rule.buildAndAssertSuccess(p); + TaskListener mockListener = Mockito.mock(TaskListener.class); + Mockito.when(mockListener.getLogger()).thenReturn(Mockito.spy(StreamTaskListener.fromStdout().getLogger())); + + p.getScm().checkout(run, new Launcher.LocalLauncher(listener), + new FilePath(run.getRootDir()).child("tmp-" + "master"), + mockListener, null, SCMRevisionState.NONE); + + ArgumentCaptor logCaptor = ArgumentCaptor.forClass(String.class); + verify(mockListener.getLogger(), atLeastOnce()).println(logCaptor.capture()); + List values = logCaptor.getAllValues(); + int countFetches = 0; + String fetchArg = " > git fetch --tags --force --progress -- " + sampleRepo.getRoot().getAbsolutePath() + " +refs/heads/*:refs/remotes/origin/*" + " # timeout=10"; + for (String value : values) { + if(value.equals(fetchArg)){ + countFetches++; + } + } + // Before the flag check, countFetches was "2" because git fetch was called twice + assertThat(2, is(not(countFetches))); + + // After the fix, git fetch is called exactly once + assertThat(1, is(countFetches)); + } + /** * Method performs HTTP get on "notifyCommit" URL, passing it commit by SHA1 * and tests for build data consistency. From 9b25935af127bc4a46a502063688d9294e7097aa Mon Sep 17 00:00:00 2001 From: Rishabh Budhouliya Date: Mon, 2 Mar 2020 17:14:35 +0530 Subject: [PATCH 03/26] Added tests that confirm no data loss with avoiding second fetch --- .../java/hudson/plugins/git/GitSCMTest.java | 164 ++++++++++++++---- 1 file changed, 132 insertions(+), 32 deletions(-) diff --git a/src/test/java/hudson/plugins/git/GitSCMTest.java b/src/test/java/hudson/plugins/git/GitSCMTest.java index 9b6a81bb65..4168e65d68 100644 --- a/src/test/java/hudson/plugins/git/GitSCMTest.java +++ b/src/test/java/hudson/plugins/git/GitSCMTest.java @@ -77,6 +77,7 @@ import java.util.logging.Logger; import org.eclipse.jgit.transport.RemoteConfig; +import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.*; import static org.hamcrest.CoreMatchers.instanceOf; import org.jvnet.hudson.test.Issue; @@ -285,6 +286,137 @@ 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 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); + + // 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 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); + + // 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("FAILURE", is(not(build.getResult().toString()))); + } + + /** + * 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 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); + if (childFile != null) { + // 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("FAILURE", is((build.getResult().toString()))); + } + + // Checks if the second fetch is being avoided + private void assertRedundantFetchIsTrue(FreeStyleBuild build, String refSpec) throws IOException { + List values = build.getLog(Integer.MAX_VALUE); + int countFetches = 0; + String argRefSpec = " " + refSpec; + String fetchArg = " > git fetch --tags --force --progress -- " + testRepo.gitDir.getAbsolutePath() + argRefSpec + " # timeout=10"; + for (String value : values) { + if(value.equals(fetchArg)){ + countFetches++; + } + } + // Before the flag check, countFetches was "2" because git fetch was called twice + assertThat(countFetches, is(not(2))); + + // After the fix, git fetch is called exactly once + assertThat(countFetches, is(1)); + } + + // Returns the file FETCH_HEAD found in .git + private FilePath returnFile(FreeStyleBuild build) throws IOException, InterruptedException { + List 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 @@ -2738,38 +2870,6 @@ public void testCommitMessageIsPrintedToLogs() throws Exception { assertThat(values, hasItem("Commit message: \"test commit\"")); } - @Issue("JENKINS-49757") - @Test - public void testRedundantFetchCallFromLogs() throws Exception { - sampleRepo.init(); - sampleRepo.write("file", "v1"); - sampleRepo.git("commit", "--all", "--message=test commit"); - FreeStyleProject p = setupSimpleProject("master"); - Run run = rule.buildAndAssertSuccess(p); - TaskListener mockListener = Mockito.mock(TaskListener.class); - Mockito.when(mockListener.getLogger()).thenReturn(Mockito.spy(StreamTaskListener.fromStdout().getLogger())); - - p.getScm().checkout(run, new Launcher.LocalLauncher(listener), - new FilePath(run.getRootDir()).child("tmp-" + "master"), - mockListener, null, SCMRevisionState.NONE); - - ArgumentCaptor logCaptor = ArgumentCaptor.forClass(String.class); - verify(mockListener.getLogger(), atLeastOnce()).println(logCaptor.capture()); - List values = logCaptor.getAllValues(); - int countFetches = 0; - String fetchArg = " > git fetch --tags --force --progress -- " + sampleRepo.getRoot().getAbsolutePath() + " +refs/heads/*:refs/remotes/origin/*" + " # timeout=10"; - for (String value : values) { - if(value.equals(fetchArg)){ - countFetches++; - } - } - // Before the flag check, countFetches was "2" because git fetch was called twice - assertThat(2, is(not(countFetches))); - - // After the fix, git fetch is called exactly once - assertThat(1, is(countFetches)); - } - /** * Method performs HTTP get on "notifyCommit" URL, passing it commit by SHA1 * and tests for build data consistency. From 1cfb5396d4d749ceb2b3a9350fa61043f416afb9 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 6 Jun 2020 22:59:46 +0200 Subject: [PATCH 04/26] Use equals rather than == Co-authored-by: Francisco Javier Fernandez <31063239+fcojfernandez@users.noreply.github.com> --- src/main/java/hudson/plugins/git/GitSCM.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/hudson/plugins/git/GitSCM.java b/src/main/java/hudson/plugins/git/GitSCM.java index 89c2a4cc0b..a5b5c63d59 100644 --- a/src/main/java/hudson/plugins/git/GitSCM.java +++ b/src/main/java/hudson/plugins/git/GitSCM.java @@ -1132,7 +1132,7 @@ private void retrieveChanges(Run build, GitClient git, TaskListener listener) th } for (RemoteConfig remoteRepository : repos) { - if (remoteRepository == repos.get(0) && redundantFetchCheck){ + if (remoteRepository.equals(repos.get(0)) && redundantFetchCheck){ continue; } try { From e6ac2ed29f615610f9b6e2f6a69c21324e9be011 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 6 Jun 2020 23:10:00 +0200 Subject: [PATCH 05/26] Use assertThat and is for better msgs Co-authored-by: Francisco Javier Fernandez <31063239+fcojfernandez@users.noreply.github.com> --- src/test/java/hudson/plugins/git/GitSCMTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/hudson/plugins/git/GitSCMTest.java b/src/test/java/hudson/plugins/git/GitSCMTest.java index 5ec86b56ef..470f0dfc06 100644 --- a/src/test/java/hudson/plugins/git/GitSCMTest.java +++ b/src/test/java/hudson/plugins/git/GitSCMTest.java @@ -347,7 +347,7 @@ public void testAvoidRedundantFetchWithoutHonorRefSpec() throws Exception { String wideRefSpec = "+refs/heads/*:refs/remotes/origin/*"; assertRedundantFetchIsTrue(build, wideRefSpec); - assertThat("FAILURE", is(not(build.getResult().toString()))); + assertThat(build.getResult(), is(Result.SUCCESS); } /** From eaded93a2f00fa0e25b89882ca16b911a577643a Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 6 Jun 2020 23:10:52 +0200 Subject: [PATCH 06/26] Clarify assertion Co-authored-by: Francisco Javier Fernandez <31063239+fcojfernandez@users.noreply.github.com> --- src/test/java/hudson/plugins/git/GitSCMTest.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/test/java/hudson/plugins/git/GitSCMTest.java b/src/test/java/hudson/plugins/git/GitSCMTest.java index 470f0dfc06..dc4b0252a5 100644 --- a/src/test/java/hudson/plugins/git/GitSCMTest.java +++ b/src/test/java/hudson/plugins/git/GitSCMTest.java @@ -381,11 +381,10 @@ public void testAvoidRedundantFetchWithHonorRefSpec() throws Exception { FreeStyleBuild build = build(projectWithMaster, Result.FAILURE); FilePath childFile = returnFile(build); - if (childFile != null) { - // 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")); - } + 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("FAILURE", is((build.getResult().toString()))); From 21ead1ea5ec8f9f0765d83972f93229d269959d1 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 6 Jun 2020 23:11:09 +0200 Subject: [PATCH 07/26] Simpler assertThat Co-authored-by: Francisco Javier Fernandez <31063239+fcojfernandez@users.noreply.github.com> --- src/test/java/hudson/plugins/git/GitSCMTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/hudson/plugins/git/GitSCMTest.java b/src/test/java/hudson/plugins/git/GitSCMTest.java index dc4b0252a5..a853a6f071 100644 --- a/src/test/java/hudson/plugins/git/GitSCMTest.java +++ b/src/test/java/hudson/plugins/git/GitSCMTest.java @@ -387,7 +387,7 @@ public void testAvoidRedundantFetchWithHonorRefSpec() throws Exception { assertThat("foo branch was not fetched", childFile.readToString(), containsString("foo")); assertRedundantFetchIsTrue(build, refSpec); - assertThat("FAILURE", is((build.getResult().toString()))); + assertThat(build.getResult(), is(Result.SUCCESS); } // Checks if the second fetch is being avoided From 995d803f9e587e101c4a228ce2ffa2a632dc7853 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 6 Jun 2020 15:15:11 -0600 Subject: [PATCH 08/26] Fix compilation error --- src/test/java/hudson/plugins/git/GitSCMTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/hudson/plugins/git/GitSCMTest.java b/src/test/java/hudson/plugins/git/GitSCMTest.java index 9083df2918..e6ac2921f3 100644 --- a/src/test/java/hudson/plugins/git/GitSCMTest.java +++ b/src/test/java/hudson/plugins/git/GitSCMTest.java @@ -395,7 +395,7 @@ public void testAvoidRedundantFetchWithoutHonorRefSpec() throws Exception { String wideRefSpec = "+refs/heads/*:refs/remotes/origin/*"; assertRedundantFetchIsTrue(build, wideRefSpec); - assertThat(build.getResult(), is(Result.SUCCESS); + assertThat(build.getResult(), is(Result.SUCCESS)); } /** @@ -435,7 +435,7 @@ public void testAvoidRedundantFetchWithHonorRefSpec() throws Exception { assertThat("foo branch was not fetched", childFile.readToString(), containsString("foo")); assertRedundantFetchIsTrue(build, refSpec); - assertThat(build.getResult(), is(Result.SUCCESS); + assertThat(build.getResult(), is(Result.SUCCESS)); } // Checks if the second fetch is being avoided From ae036cce9ae67126ef564b3a63f1239648f9dabd Mon Sep 17 00:00:00 2001 From: Rishabh Budhouliya Date: Mon, 8 Jun 2020 03:50:34 +0530 Subject: [PATCH 09/26] Fix redundant fetch test failure The build is supposed to fail as the narrow refspec only fetches "foo" branch (honor initial refspec) and we are commiting in a branch which doesnt exist for the project, the master branch. --- src/test/java/hudson/plugins/git/GitSCMTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/hudson/plugins/git/GitSCMTest.java b/src/test/java/hudson/plugins/git/GitSCMTest.java index e6ac2921f3..c9cc3a55b5 100644 --- a/src/test/java/hudson/plugins/git/GitSCMTest.java +++ b/src/test/java/hudson/plugins/git/GitSCMTest.java @@ -435,7 +435,7 @@ public void testAvoidRedundantFetchWithHonorRefSpec() throws Exception { assertThat("foo branch was not fetched", childFile.readToString(), containsString("foo")); assertRedundantFetchIsTrue(build, refSpec); - assertThat(build.getResult(), is(Result.SUCCESS)); + assertThat(build.getResult(), is(Result.FAILURE)); } // Checks if the second fetch is being avoided From 82ffbbf2ca4438ce0cabe8b046e75b2f15b07fa3 Mon Sep 17 00:00:00 2001 From: Rishabh Budhouliya Date: Wed, 10 Jun 2020 15:10:09 +0530 Subject: [PATCH 10/26] Enable CleanBeforeCheckout to be decorated in the CloneCommand The concern of removing the second fetch call is a possible repository data loss or misconfiguration in terms of the extra behaviors applied during checkout. If the second fetch call is ignored, CleanBeforeCheckout option will also be ignored as it doesn't implement the decorateCloneCommand which is used by the git fetch call. To ensure that it is not ignored, decorateCloneCommand has been implemented for this particular extension. The unit test GitSCMTest#testCleanBeforeCheckout doesn't fail after the removal of second fetch call now. --- .../git/extensions/impl/CleanBeforeCheckout.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/main/java/hudson/plugins/git/extensions/impl/CleanBeforeCheckout.java b/src/main/java/hudson/plugins/git/extensions/impl/CleanBeforeCheckout.java index 03f1c3b86d..0e588b35e3 100644 --- a/src/main/java/hudson/plugins/git/extensions/impl/CleanBeforeCheckout.java +++ b/src/main/java/hudson/plugins/git/extensions/impl/CleanBeforeCheckout.java @@ -1,6 +1,7 @@ package hudson.plugins.git.extensions.impl; import hudson.Extension; +import hudson.model.Run; import hudson.model.TaskListener; import hudson.plugins.git.GitException; import hudson.plugins.git.GitSCM; @@ -9,6 +10,7 @@ import java.io.IOException; import java.util.Objects; +import org.jenkinsci.plugins.gitclient.CloneCommand; import org.jenkinsci.plugins.gitclient.FetchCommand; import org.jenkinsci.plugins.gitclient.GitClient; import org.kohsuke.stapler.DataBoundConstructor; @@ -49,6 +51,17 @@ public void decorateFetchCommand(GitSCM scm, GitClient git, TaskListener listene } } + @Override + public void decorateCloneCommand(GitSCM scm, Run build, GitClient git, TaskListener listener, + CloneCommand cmd) throws IOException, InterruptedException { + listener.getLogger().println("Cleaning workspace"); + git.clean(deleteUntrackedNestedRepositories); + // TODO: revisit how to hand off to SubmoduleOption + for (GitSCMExtension ext : scm.getExtensions()) { + ext.onClean(scm, git); + } + } + /** * {@inheritDoc} */ From 3bc0059142a1c227b5bee9b9ddbdc623fc0f0df1 Mon Sep 17 00:00:00 2001 From: Rishabh Budhouliya Date: Wed, 10 Jun 2020 18:43:18 +0530 Subject: [PATCH 11/26] Fix assertRedundantFetchIsTrue by reducing the scope of fetch argument to be searched in the build logs --- src/test/java/hudson/plugins/git/GitSCMTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/hudson/plugins/git/GitSCMTest.java b/src/test/java/hudson/plugins/git/GitSCMTest.java index c9cc3a55b5..a7af03859c 100644 --- a/src/test/java/hudson/plugins/git/GitSCMTest.java +++ b/src/test/java/hudson/plugins/git/GitSCMTest.java @@ -443,9 +443,9 @@ private void assertRedundantFetchIsTrue(FreeStyleBuild build, String refSpec) th List values = build.getLog(Integer.MAX_VALUE); int countFetches = 0; String argRefSpec = " " + refSpec; - String fetchArg = " > git fetch --tags --force --progress -- " + testRepo.gitDir.getAbsolutePath() + argRefSpec + " # timeout=10"; + //String fetchArg = " > git fetch --tags --force --progress -- " + testRepo.gitDir.getAbsolutePath() + argRefSpec + " # timeout=10"; for (String value : values) { - if(value.equals(fetchArg)){ + if(value.contains("git fetch")){ countFetches++; } } From 05daa5ef3b06dca73901595d0a759e4f5925abe0 Mon Sep 17 00:00:00 2001 From: Rishabh Budhouliya Date: Fri, 12 Jun 2020 16:58:57 +0530 Subject: [PATCH 12/26] Update assertion of git fetch arg from build logs with pattern matching --- .../java/hudson/plugins/git/GitSCMTest.java | 26 ++++++++----------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/src/test/java/hudson/plugins/git/GitSCMTest.java b/src/test/java/hudson/plugins/git/GitSCMTest.java index a7af03859c..6fe90d99e8 100644 --- a/src/test/java/hudson/plugins/git/GitSCMTest.java +++ b/src/test/java/hudson/plugins/git/GitSCMTest.java @@ -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.*; @@ -441,19 +443,13 @@ public void testAvoidRedundantFetchWithHonorRefSpec() throws Exception { // Checks if the second fetch is being avoided private void assertRedundantFetchIsTrue(FreeStyleBuild build, String refSpec) throws IOException { List values = build.getLog(Integer.MAX_VALUE); - int countFetches = 0; - String argRefSpec = " " + refSpec; + //String fetchArg = " > git fetch --tags --force --progress -- " + testRepo.gitDir.getAbsolutePath() + argRefSpec + " # timeout=10"; - for (String value : values) { - if(value.contains("git fetch")){ - countFetches++; - } - } - // Before the flag check, countFetches was "2" because git fetch was called twice - assertThat(countFetches, is(not(2))); + Pattern fetchPattern = Pattern.compile(".* git.* fetch .*"); + List fetchCommands = values.stream().filter(fetchPattern.asPredicate()).collect(Collectors.toList()); // After the fix, git fetch is called exactly once - assertThat(countFetches, is(1)); + assertThat("Fetch commands were: " + fetchCommands, fetchCommands, hasSize(1)); } // Returns the file FETCH_HEAD found in .git @@ -986,15 +982,15 @@ public void testCleanBeforeCheckout() throws Exception { git.checkout(branch1); p.poll(listener).hasChanges(); assertThat(firstBuild.getLog(175), hasItem("Cleaning workspace")); - assertTrue(firstBuild.getLog().indexOf("Cleaning") > firstBuild.getLog().indexOf("Cloning")); //clean should be after clone - assertTrue(firstBuild.getLog().indexOf("Cleaning") < firstBuild.getLog().indexOf("Checking out")); //clean before checkout - assertTrue(firstBuild.getWorkspace().child(commitFile1).exists()); + assertThat(firstBuild.getLog().indexOf("Cleaning") > firstBuild.getLog().indexOf("Cloning"), is(true)); //clean should be after clone + assertThat(firstBuild.getLog().indexOf("Cleaning") < firstBuild.getLog().indexOf("Checking out"), is(true)); //clean before checkout + assertThat(firstBuild.getWorkspace().child(commitFile1).exists(), is(true)); git.checkout(branch1); final FreeStyleBuild secondBuild = build(p, Result.SUCCESS, commitFile2); p.poll(listener).hasChanges(); assertThat(secondBuild.getLog(175), hasItem("Cleaning workspace")); - assertTrue(secondBuild.getLog().indexOf("Cleaning") < secondBuild.getLog().indexOf("Fetching upstream changes")); - assertTrue(secondBuild.getWorkspace().child(commitFile2).exists()); + assertThat(secondBuild.getLog().indexOf("Cleaning") < secondBuild.getLog().indexOf("Fetching upstream changes"), is(true)); + assertThat(secondBuild.getWorkspace().child(commitFile2).exists(), is(true)); } From deb1b7801a6fbb698c7468397d106c3d975a6943 Mon Sep 17 00:00:00 2001 From: Rishabh Budhouliya Date: Wed, 17 Jun 2020 08:51:49 +0530 Subject: [PATCH 13/26] Do not decorate clone command with a clean The reason to add a "clean" before checkout behaviour is to clean the working directory of a repository before checking out a branch. In this case, the remote repository is being cloned for the first time which means there is no local git repository. --- .../git/extensions/impl/CleanBeforeCheckout.java | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/main/java/hudson/plugins/git/extensions/impl/CleanBeforeCheckout.java b/src/main/java/hudson/plugins/git/extensions/impl/CleanBeforeCheckout.java index 0e588b35e3..575ad6df75 100644 --- a/src/main/java/hudson/plugins/git/extensions/impl/CleanBeforeCheckout.java +++ b/src/main/java/hudson/plugins/git/extensions/impl/CleanBeforeCheckout.java @@ -51,17 +51,6 @@ public void decorateFetchCommand(GitSCM scm, GitClient git, TaskListener listene } } - @Override - public void decorateCloneCommand(GitSCM scm, Run build, GitClient git, TaskListener listener, - CloneCommand cmd) throws IOException, InterruptedException { - listener.getLogger().println("Cleaning workspace"); - git.clean(deleteUntrackedNestedRepositories); - // TODO: revisit how to hand off to SubmoduleOption - for (GitSCMExtension ext : scm.getExtensions()) { - ext.onClean(scm, git); - } - } - /** * {@inheritDoc} */ From c4c47cb77cae845e228c57738dfab6b700c0b925 Mon Sep 17 00:00:00 2001 From: Rishabh Budhouliya Date: Wed, 17 Jun 2020 09:10:40 +0530 Subject: [PATCH 14/26] Fix testCleanBeforeCheckout assertions First build is known to have a clean workspace so there is no need to clean it before checkout. Second build should clean because the workspace might be cluttered with extra files from the first build. Also removes several useless operations in the test (related to buildLog) --- .../java/hudson/plugins/git/GitSCMTest.java | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/src/test/java/hudson/plugins/git/GitSCMTest.java b/src/test/java/hudson/plugins/git/GitSCMTest.java index 6fe90d99e8..f07c648ea3 100644 --- a/src/test/java/hudson/plugins/git/GitSCMTest.java +++ b/src/test/java/hudson/plugins/git/GitSCMTest.java @@ -963,36 +963,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 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"; - final String commitFile2 = "commitFile2"; commit(commitFile1, johnDoe, janeDoe, "Commit number 1"); - commit(commitFile2, johnDoe, janeDoe, "Commit number 2"); final FreeStyleBuild firstBuild = build(p, Result.SUCCESS, commitFile1); - final String branch1 = "Branch1"; - final String branch2 = "Branch2"; - List branches = new ArrayList<>(); - branches.add(new BranchSpec("master")); - branches.add(new BranchSpec(branch1)); - branches.add(new BranchSpec(branch2)); - git.branch(branch1); - git.checkout(branch1); - p.poll(listener).hasChanges(); - assertThat(firstBuild.getLog(175), hasItem("Cleaning workspace")); - assertThat(firstBuild.getLog().indexOf("Cleaning") > firstBuild.getLog().indexOf("Cloning"), is(true)); //clean should be after clone - assertThat(firstBuild.getLog().indexOf("Cleaning") < firstBuild.getLog().indexOf("Checking out"), is(true)); //clean before checkout - assertThat(firstBuild.getWorkspace().child(commitFile1).exists(), is(true)); - git.checkout(branch1); + 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); - p.poll(listener).hasChanges(); - assertThat(secondBuild.getLog(175), hasItem("Cleaning workspace")); - assertThat(secondBuild.getLog().indexOf("Cleaning") < secondBuild.getLog().indexOf("Fetching upstream changes"), is(true)); - assertThat(secondBuild.getWorkspace().child(commitFile2).exists(), is(true)); - - + List 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") From e8d1bca5eabfb59ebe1dd4d97f4adcc51ec2269a Mon Sep 17 00:00:00 2001 From: Rishabh Budhouliya Date: Wed, 24 Jun 2020 14:59:08 +0530 Subject: [PATCH 15/26] Determine if second fetch call is redundant After introducing the fix (removal of second fetch), references apart from "refs/heads" were missed by the first fetch call. To not break existing use-cases, the safest way right now is to call the second fetch whenever user provides a refspec while honor refspec and force clone is false. Just to be clear, refspec != null && honorRefSpec==false. --- src/main/java/hudson/plugins/git/GitSCM.java | 43 ++++++++++++++------ 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/src/main/java/hudson/plugins/git/GitSCM.java b/src/main/java/hudson/plugins/git/GitSCM.java index d6add0a226..131c8b81d6 100644 --- a/src/main/java/hudson/plugins/git/GitSCM.java +++ b/src/main/java/hudson/plugins/git/GitSCM.java @@ -31,14 +31,7 @@ import hudson.plugins.git.browser.GitRepositoryBrowser; import hudson.plugins.git.extensions.GitSCMExtension; import hudson.plugins.git.extensions.GitSCMExtensionDescriptor; -import hudson.plugins.git.extensions.impl.AuthorInChangelog; -import hudson.plugins.git.extensions.impl.BuildChooserSetting; -import hudson.plugins.git.extensions.impl.BuildSingleRevisionOnly; -import hudson.plugins.git.extensions.impl.ChangelogToBranch; -import hudson.plugins.git.extensions.impl.PathRestriction; -import hudson.plugins.git.extensions.impl.LocalBranch; -import hudson.plugins.git.extensions.impl.RelativeTargetDirectory; -import hudson.plugins.git.extensions.impl.PreBuildMerge; +import hudson.plugins.git.extensions.impl.*; import hudson.plugins.git.opt.PreBuildMergeOptions; import hudson.plugins.git.util.Build; import hudson.plugins.git.util.*; @@ -110,7 +103,6 @@ import hudson.plugins.git.browser.GithubWeb; import static hudson.scm.PollingResult.*; import hudson.Util; -import hudson.plugins.git.extensions.impl.ScmName; import hudson.util.LogTaskListener; import hudson.util.ReflectionUtils; import java.util.Map.Entry; @@ -1111,7 +1103,7 @@ public EnvVars getEnvironment() { private void retrieveChanges(Run build, GitClient git, TaskListener listener) throws IOException, InterruptedException { final PrintStream log = listener.getLogger(); - boolean redundantFetchCheck = false; + boolean removeRedundantFetch = false; List repos = getParamExpandedRepos(build, listener); if (repos.isEmpty()) return; // defensive check even though this is an invalid configuration @@ -1125,13 +1117,15 @@ private void retrieveChanges(Run build, GitClient git, TaskListener listener) th log.println("Cloning the remote Git repository"); RemoteConfig rc = repos.get(0); - redundantFetchCheck = true; try { CloneCommand cmd = git.clone_().url(rc.getURIs().get(0).toPrivateString()).repositoryName(rc.getName()); for (GitSCMExtension ext : extensions) { ext.decorateCloneCommand(this, build, git, listener, cmd); } cmd.execute(); + // determine if second fetch is required + CloneOption option = extensions.get(CloneOption.class); + removeRedundantFetch = determineRedundantFetch(option, rc); } catch (GitException ex) { ex.printStackTrace(listener.error("Error cloning remote repo '" + rc.getName() + "'")); throw new AbortException("Error cloning remote repo '" + rc.getName() + "'"); @@ -1139,7 +1133,8 @@ private void retrieveChanges(Run build, GitClient git, TaskListener listener) th } for (RemoteConfig remoteRepository : repos) { - if (remoteRepository.equals(repos.get(0)) && redundantFetchCheck){ + if (remoteRepository.equals(repos.get(0)) && removeRedundantFetch){ + log.println("Avoid second fetch"); continue; } try { @@ -1153,6 +1148,30 @@ private void retrieveChanges(Run build, GitClient git, TaskListener listener) th } } + private boolean determineRedundantFetch(CloneOption option, RemoteConfig rc) { + List initialFetchRefSpecs = rc.getFetchRefSpecs(); + boolean isDefaultRefspec = true; // default refspec is any refspec with "refs/heads/" mapping + boolean removeSecondFetch = true; + if (initialFetchRefSpecs != null) { + for (RefSpec ref:initialFetchRefSpecs) { + if (!ref.toString().contains("refs/heads")) { + isDefaultRefspec = false; // if refspec is not of default type, preserve second fetch + } + } + if (option == null) { + removeSecondFetch = isDefaultRefspec; + } else { + if (!option.isHonorRefspec()) { + removeSecondFetch = isDefaultRefspec; + } else { + removeSecondFetch = true; // avoid second fetch call if honor refspec is enabled + } + } + } + // if initial fetch refspec contains "refs/heads/*" (default refspec), ignore the second fetch call + return removeSecondFetch; + } + @Override public void checkout(Run build, Launcher launcher, FilePath workspace, TaskListener listener, File changelogFile, SCMRevisionState baseline) throws IOException, InterruptedException { From b54faebac9bb53f470b1eaff3f920e72fd3fcef7 Mon Sep 17 00:00:00 2001 From: Rishabh Budhouliya Date: Wed, 24 Jun 2020 15:21:30 +0530 Subject: [PATCH 16/26] Do not aggregate GitSCMExtension imports --- src/main/java/hudson/plugins/git/GitSCM.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/main/java/hudson/plugins/git/GitSCM.java b/src/main/java/hudson/plugins/git/GitSCM.java index 131c8b81d6..a27acbe8e5 100644 --- a/src/main/java/hudson/plugins/git/GitSCM.java +++ b/src/main/java/hudson/plugins/git/GitSCM.java @@ -31,7 +31,15 @@ import hudson.plugins.git.browser.GitRepositoryBrowser; import hudson.plugins.git.extensions.GitSCMExtension; import hudson.plugins.git.extensions.GitSCMExtensionDescriptor; -import hudson.plugins.git.extensions.impl.*; +import hudson.plugins.git.extensions.impl.AuthorInChangelog; +import hudson.plugins.git.extensions.impl.BuildChooserSetting; +import hudson.plugins.git.extensions.impl.BuildSingleRevisionOnly; +import hudson.plugins.git.extensions.impl.ChangelogToBranch; +import hudson.plugins.git.extensions.impl.PathRestriction; +import hudson.plugins.git.extensions.impl.LocalBranch; +import hudson.plugins.git.extensions.impl.RelativeTargetDirectory; +import hudson.plugins.git.extensions.impl.PreBuildMerge; +import hudson.plugins.git.extensions.impl.CloneOption; import hudson.plugins.git.opt.PreBuildMergeOptions; import hudson.plugins.git.util.Build; import hudson.plugins.git.util.*; From 1be3697c4aaa40d76854023107d701d3686fab9b Mon Sep 17 00:00:00 2001 From: Rishabh Budhouliya Date: Wed, 24 Jun 2020 18:24:45 +0530 Subject: [PATCH 17/26] Correct spacing in foreach loop --- src/main/java/hudson/plugins/git/GitSCM.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/hudson/plugins/git/GitSCM.java b/src/main/java/hudson/plugins/git/GitSCM.java index b34a1765d2..e18112e5fd 100644 --- a/src/main/java/hudson/plugins/git/GitSCM.java +++ b/src/main/java/hudson/plugins/git/GitSCM.java @@ -1161,7 +1161,7 @@ private boolean determineRedundantFetch(CloneOption option, RemoteConfig rc) { boolean isDefaultRefspec = true; // default refspec is any refspec with "refs/heads/" mapping boolean removeSecondFetch = true; if (initialFetchRefSpecs != null) { - for (RefSpec ref:initialFetchRefSpecs) { + for (RefSpec ref : initialFetchRefSpecs) { if (!ref.toString().contains("refs/heads")) { isDefaultRefspec = false; // if refspec is not of default type, preserve second fetch } From 15e2f52b8c1ce828ba7abf521c922337af86fb3d Mon Sep 17 00:00:00 2001 From: Rishabh Budhouliya Date: Wed, 24 Jun 2020 18:25:01 +0530 Subject: [PATCH 18/26] Remove unused imports --- .../hudson/plugins/git/extensions/impl/CleanBeforeCheckout.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/main/java/hudson/plugins/git/extensions/impl/CleanBeforeCheckout.java b/src/main/java/hudson/plugins/git/extensions/impl/CleanBeforeCheckout.java index 575ad6df75..03f1c3b86d 100644 --- a/src/main/java/hudson/plugins/git/extensions/impl/CleanBeforeCheckout.java +++ b/src/main/java/hudson/plugins/git/extensions/impl/CleanBeforeCheckout.java @@ -1,7 +1,6 @@ package hudson.plugins.git.extensions.impl; import hudson.Extension; -import hudson.model.Run; import hudson.model.TaskListener; import hudson.plugins.git.GitException; import hudson.plugins.git.GitSCM; @@ -10,7 +9,6 @@ import java.io.IOException; import java.util.Objects; -import org.jenkinsci.plugins.gitclient.CloneCommand; import org.jenkinsci.plugins.gitclient.FetchCommand; import org.jenkinsci.plugins.gitclient.GitClient; import org.kohsuke.stapler.DataBoundConstructor; From f2fecc8aa934adc4c73bb760b9820e552b545119 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 27 Jun 2020 18:09:13 -0600 Subject: [PATCH 19/26] Add missing import --- src/main/java/hudson/plugins/git/GitSCM.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/hudson/plugins/git/GitSCM.java b/src/main/java/hudson/plugins/git/GitSCM.java index e18112e5fd..bc6f7204e3 100644 --- a/src/main/java/hudson/plugins/git/GitSCM.java +++ b/src/main/java/hudson/plugins/git/GitSCM.java @@ -40,6 +40,7 @@ import hudson.plugins.git.extensions.impl.RelativeTargetDirectory; import hudson.plugins.git.extensions.impl.PreBuildMerge; import hudson.plugins.git.extensions.impl.CloneOption; +import hudson.plugins.git.extensions.impl.ScmName; import hudson.plugins.git.opt.PreBuildMergeOptions; import hudson.plugins.git.util.Build; import hudson.plugins.git.util.*; From a7685db9f483e70e3c88b09de20e8ba2caba0f25 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 27 Jun 2020 18:19:05 -0600 Subject: [PATCH 20/26] Fix imports Reduce differences to master branch --- src/main/java/hudson/plugins/git/GitSCM.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main/java/hudson/plugins/git/GitSCM.java b/src/main/java/hudson/plugins/git/GitSCM.java index bc6f7204e3..2c442cd5ac 100644 --- a/src/main/java/hudson/plugins/git/GitSCM.java +++ b/src/main/java/hudson/plugins/git/GitSCM.java @@ -27,7 +27,6 @@ import hudson.model.Run; import hudson.model.Saveable; import hudson.model.TaskListener; -import hudson.model.queue.Tasks; import hudson.plugins.git.browser.GitRepositoryBrowser; import hudson.plugins.git.extensions.GitSCMExtension; import hudson.plugins.git.extensions.GitSCMExtensionDescriptor; @@ -40,7 +39,6 @@ import hudson.plugins.git.extensions.impl.RelativeTargetDirectory; import hudson.plugins.git.extensions.impl.PreBuildMerge; import hudson.plugins.git.extensions.impl.CloneOption; -import hudson.plugins.git.extensions.impl.ScmName; import hudson.plugins.git.opt.PreBuildMergeOptions; import hudson.plugins.git.util.Build; import hudson.plugins.git.util.*; @@ -112,6 +110,7 @@ import hudson.plugins.git.browser.GithubWeb; import static hudson.scm.PollingResult.*; import hudson.Util; +import hudson.plugins.git.extensions.impl.ScmName; import hudson.util.LogTaskListener; import hudson.util.ReflectionUtils; import java.util.Map.Entry; From 162325309cbee1abbe4fd87ef725b17de3ee9b60 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 27 Jun 2020 22:53:56 -0600 Subject: [PATCH 21/26] Run the git command to configure test repo --- .../java/hudson/plugins/git/GitChangeSetTruncateTest.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/test/java/hudson/plugins/git/GitChangeSetTruncateTest.java b/src/test/java/hudson/plugins/git/GitChangeSetTruncateTest.java index 1c1f7e2084..c00578b279 100644 --- a/src/test/java/hudson/plugins/git/GitChangeSetTruncateTest.java +++ b/src/test/java/hudson/plugins/git/GitChangeSetTruncateTest.java @@ -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(); - new CliGitCommand(gitClient, "config", "user.name", "ChangeSet Truncation Test"); - new CliGitCommand(gitClient, "config", "user.email", "ChangeSetTruncation@example.com"); + 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", "ChangeSetTruncation@mail.example.com"); + assertThat(gitCmd.run(), is(expectedResult)); } private ObjectId commitOneFile(GitClient gitClient, final String commitSummary) throws Exception { From 09726f13532666a34eec8d87083c0f0bfb6febde Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 27 Jun 2020 22:56:52 -0600 Subject: [PATCH 22/26] Use shallow clone randomly to improve coverage One of the branches was not being reached by tests prior to this change. --- src/test/java/hudson/plugins/git/GitSCMTest.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/test/java/hudson/plugins/git/GitSCMTest.java b/src/test/java/hudson/plugins/git/GitSCMTest.java index f07c648ea3..1141967cd5 100644 --- a/src/test/java/hudson/plugins/git/GitSCMTest.java +++ b/src/test/java/hudson/plugins/git/GitSCMTest.java @@ -351,6 +351,12 @@ public void testAvoidRedundantFetch() throws Exception { /* 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"; @@ -375,6 +381,12 @@ public void testAvoidRedundantFetchWithoutHonorRefSpec() throws Exception { /* 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"; From c7b7ec63ed5d8749cf45fce10927bb93f7fe65a0 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sun, 28 Jun 2020 05:04:23 -0600 Subject: [PATCH 23/26] Place CloneOption import in its sorted location --- src/main/java/hudson/plugins/git/GitSCM.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/hudson/plugins/git/GitSCM.java b/src/main/java/hudson/plugins/git/GitSCM.java index 2c442cd5ac..4ed9772974 100644 --- a/src/main/java/hudson/plugins/git/GitSCM.java +++ b/src/main/java/hudson/plugins/git/GitSCM.java @@ -34,11 +34,11 @@ import hudson.plugins.git.extensions.impl.BuildChooserSetting; import hudson.plugins.git.extensions.impl.BuildSingleRevisionOnly; import hudson.plugins.git.extensions.impl.ChangelogToBranch; +import hudson.plugins.git.extensions.impl.CloneOption; import hudson.plugins.git.extensions.impl.PathRestriction; import hudson.plugins.git.extensions.impl.LocalBranch; import hudson.plugins.git.extensions.impl.RelativeTargetDirectory; import hudson.plugins.git.extensions.impl.PreBuildMerge; -import hudson.plugins.git.extensions.impl.CloneOption; import hudson.plugins.git.opt.PreBuildMergeOptions; import hudson.plugins.git.util.Build; import hudson.plugins.git.util.*; From f7b77e642d2d4e7522b34f59d1c153a490c9fec0 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sun, 28 Jun 2020 05:13:40 -0600 Subject: [PATCH 24/26] Add test to check second fetch is used when needed Also reuses the existing assertion to provide a new assertion. --- .../java/hudson/plugins/git/GitSCMTest.java | 84 +++++++++++++++++-- 1 file changed, 79 insertions(+), 5 deletions(-) diff --git a/src/test/java/hudson/plugins/git/GitSCMTest.java b/src/test/java/hudson/plugins/git/GitSCMTest.java index 1141967cd5..8b3c0b127f 100644 --- a/src/test/java/hudson/plugins/git/GitSCMTest.java +++ b/src/test/java/hudson/plugins/git/GitSCMTest.java @@ -364,7 +364,7 @@ public void testAvoidRedundantFetch() throws Exception { FreeStyleBuild build = build(projectWithMaster, Result.SUCCESS); - assertRedundantFetchIsTrue(build, "+refs/heads/*:refs/remotes/origin/*"); + assertRedundantFetchIsSkipped(build, "+refs/heads/*:refs/remotes/origin/*"); } /** @@ -407,7 +407,7 @@ public void testAvoidRedundantFetchWithoutHonorRefSpec() throws Exception { } String wideRefSpec = "+refs/heads/*:refs/remotes/origin/*"; - assertRedundantFetchIsTrue(build, wideRefSpec); + assertRedundantFetchIsSkipped(build, wideRefSpec); assertThat(build.getResult(), is(Result.SUCCESS)); } @@ -447,13 +447,87 @@ public void testAvoidRedundantFetchWithHonorRefSpec() throws Exception { // 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); + assertRedundantFetchIsSkipped(build, refSpec); assertThat(build.getResult(), is(Result.FAILURE)); } + @Test + @Issue("JENKINS-49757") + public void testAvoidRedundantFetchWithNullRefspec() throws Exception { + String nullRefspec = null; + List repos = new ArrayList<>(); + repos.add(new UserRemoteConfig(testRepo.gitDir.getAbsolutePath(), "origin", nullRefspec, 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"); + + FreeStyleBuild build = build(projectWithMaster, Result.SUCCESS); + + assertRedundantFetchIsSkipped(build, "+refs/heads/*:refs/remotes/origin/*"); + } + + /* + * When initial clone does not honor the refspec and a custom refspec is used + * that is not part of the default refspec, then the second fetch is not + * redundant and must not be fetched. + * + * This example uses the format to reference GitHub pull request 553. Other + * formats would apply as well, but the case is illustrated well enough by + * using the GitHub pull request as an example of this type of problem. + */ + @Test + @Issue("JENKINS-49757") + public void testRetainRedundantFetch() throws Exception { + String refspec = "+refs/heads/*:refs/remotes/origin/* +refs/pull/553/head:refs/remotes/origin/pull/553"; + List repos = new ArrayList<>(); + repos.add(new UserRemoteConfig(testRepo.gitDir.getAbsolutePath(), "origin", refspec, 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"); + + /* Create a ref for the fake pull in the source repository */ + String[] expectedResult = {""}; + CliGitCommand gitCmd = new CliGitCommand(testRepo.git, "update-ref", "refs/pull/553/head", "HEAD"); + assertThat(gitCmd.run(), is(expectedResult)); + + FreeStyleBuild build = build(projectWithMaster, Result.SUCCESS); + + assertRedundantFetchIsUsed(build, refspec); + } + + // Checks if the second fetch is being avoided + private void assertRedundantFetchIsSkipped(FreeStyleBuild build, String refSpec) throws IOException { + assertRedundantFetchCount(build, refSpec, 1); + } + + // Checks if the second fetch is being called + private void assertRedundantFetchIsUsed(FreeStyleBuild build, String refSpec) throws IOException { + assertRedundantFetchCount(build, refSpec, 2); + } + // Checks if the second fetch is being avoided - private void assertRedundantFetchIsTrue(FreeStyleBuild build, String refSpec) throws IOException { + private void assertRedundantFetchCount(FreeStyleBuild build, String refSpec, int expectedFetchCount) throws IOException { List values = build.getLog(Integer.MAX_VALUE); //String fetchArg = " > git fetch --tags --force --progress -- " + testRepo.gitDir.getAbsolutePath() + argRefSpec + " # timeout=10"; @@ -461,7 +535,7 @@ private void assertRedundantFetchIsTrue(FreeStyleBuild build, String refSpec) th List 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)); + assertThat("Fetch commands were: " + fetchCommands, fetchCommands, hasSize(expectedFetchCount)); } // Returns the file FETCH_HEAD found in .git From 534818aa6a3b9b9f7753e093c0c7297e149c1be0 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sun, 28 Jun 2020 05:17:36 -0600 Subject: [PATCH 25/26] Mark the rc arg as NonNull, caller checks for null Code was already assuming that argument is non-null. The annotation makes it explicit that the value must be non-null and can be checked by spotbugs that it is non-null. --- src/main/java/hudson/plugins/git/GitSCM.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/hudson/plugins/git/GitSCM.java b/src/main/java/hudson/plugins/git/GitSCM.java index 4ed9772974..431cac39df 100644 --- a/src/main/java/hudson/plugins/git/GitSCM.java +++ b/src/main/java/hudson/plugins/git/GitSCM.java @@ -1156,7 +1156,7 @@ private void retrieveChanges(Run build, GitClient git, TaskListener listener) th } } - private boolean determineRedundantFetch(CloneOption option, RemoteConfig rc) { + private boolean determineRedundantFetch(CloneOption option, @NonNull RemoteConfig rc) { List initialFetchRefSpecs = rc.getFetchRefSpecs(); boolean isDefaultRefspec = true; // default refspec is any refspec with "refs/heads/" mapping boolean removeSecondFetch = true; From c2e097dac1aa4128b0fdc77e79afe321dba76d35 Mon Sep 17 00:00:00 2001 From: Rishabh Budhouliya Date: Sun, 28 Jun 2020 18:06:24 +0530 Subject: [PATCH 26/26] Non breaking change: simplification of if-else clause --- src/main/java/hudson/plugins/git/GitSCM.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/hudson/plugins/git/GitSCM.java b/src/main/java/hudson/plugins/git/GitSCM.java index 431cac39df..92ac6eaacb 100644 --- a/src/main/java/hudson/plugins/git/GitSCM.java +++ b/src/main/java/hudson/plugins/git/GitSCM.java @@ -1169,10 +1169,10 @@ private boolean determineRedundantFetch(CloneOption option, @NonNull RemoteConfi if (option == null) { removeSecondFetch = isDefaultRefspec; } else { - if (!option.isHonorRefspec()) { - removeSecondFetch = isDefaultRefspec; - } else { + if (option.isHonorRefspec()) { removeSecondFetch = true; // avoid second fetch call if honor refspec is enabled + } else { + removeSecondFetch = isDefaultRefspec; } } }