Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-49757] Remove redundant fetch #904

Merged
merged 36 commits into from
Jul 2, 2020
Merged

[JENKINS-49757] Remove redundant fetch #904

Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
8d58567
Add flag to avoid redundant fetch in GitSCM checkout
rishabhBudhouliya Feb 24, 2020
23158ea
Automated test to check redundance fetch call: testRedundantFetchCall…
rishabhBudhouliya Feb 25, 2020
9b25935
Added tests that confirm no data loss with avoiding second fetch
rishabhBudhouliya Mar 2, 2020
2cf858e
Merge pull request #1 from jenkinsci/master
rishabhBudhouliya Mar 6, 2020
11a87d4
Merge branch 'master' of https://github.com/jenkinsci/git-plugin
rishabhBudhouliya Mar 18, 2020
5d94f2e
Merge branch 'master' into JENKINS-49757
MarkEWaite May 14, 2020
1cfb539
Use equals rather than ==
MarkEWaite Jun 6, 2020
e6ac2ed
Use assertThat and is for better msgs
MarkEWaite Jun 6, 2020
eaded93
Clarify assertion
MarkEWaite Jun 6, 2020
21ead1e
Simpler assertThat
MarkEWaite Jun 6, 2020
67ced0e
Merge branch 'master' into JENKINS-49757
MarkEWaite Jun 6, 2020
995d803
Fix compilation error
MarkEWaite Jun 6, 2020
ae036cc
Fix redundant fetch test failure
rishabhBudhouliya Jun 7, 2020
82ffbbf
Enable CleanBeforeCheckout to be decorated in the CloneCommand
rishabhBudhouliya Jun 10, 2020
3bc0059
Fix assertRedundantFetchIsTrue by reducing the scope of fetch argumen…
rishabhBudhouliya Jun 10, 2020
57ab0d0
Merge branch 'master' into CleanBeforeCheckout
rishabhBudhouliya Jun 12, 2020
05daa5e
Update assertion of git fetch arg from build logs with pattern matching
rishabhBudhouliya Jun 12, 2020
d3a1e22
Merge remote-tracking branch 'upstream/master'
rishabhBudhouliya Jun 17, 2020
36e3e7d
Merge branch 'master' into CleanBeforeCheckout
rishabhBudhouliya Jun 17, 2020
deb1b78
Do not decorate clone command with a clean
rishabhBudhouliya Jun 17, 2020
c4c47cb
Fix testCleanBeforeCheckout assertions
rishabhBudhouliya Jun 17, 2020
e8d1bca
Determine if second fetch call is redundant
rishabhBudhouliya Jun 24, 2020
b54faeb
Do not aggregate GitSCMExtension imports
rishabhBudhouliya Jun 24, 2020
79cf010
Merge branch 'master' of https://github.com/jenkinsci/git-plugin
rishabhBudhouliya Jun 24, 2020
a38f1e2
Merge branch 'master' into CleanBeforeCheckout
rishabhBudhouliya Jun 24, 2020
1be3697
Correct spacing in foreach loop
rishabhBudhouliya Jun 24, 2020
15e2f52
Remove unused imports
rishabhBudhouliya Jun 24, 2020
7957017
Merge branch 'master' into CleanBeforeCheckout
MarkEWaite Jun 28, 2020
f2fecc8
Add missing import
MarkEWaite Jun 28, 2020
a7685db
Fix imports
MarkEWaite Jun 28, 2020
1623253
Run the git command to configure test repo
MarkEWaite Jun 28, 2020
09726f1
Use shallow clone randomly to improve coverage
MarkEWaite Jun 28, 2020
c7b7ec6
Place CloneOption import in its sorted location
MarkEWaite Jun 28, 2020
f7b77e6
Add test to check second fetch is used when needed
MarkEWaite Jun 28, 2020
534818a
Mark the rc arg as NonNull, caller checks for null
MarkEWaite Jun 28, 2020
c2e097d
Non breaking change: simplification of if-else clause
rishabhBudhouliya Jun 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion src/main/java/hudson/plugins/git/GitSCM.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
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.*;
Expand Down Expand Up @@ -110,7 +111,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;
Expand Down Expand Up @@ -1111,6 +1111,7 @@ public EnvVars getEnvironment() {
private void retrieveChanges(Run build, GitClient git, TaskListener listener) throws IOException, InterruptedException {
final PrintStream log = listener.getLogger();

boolean removeRedundantFetch = false;
List<RemoteConfig> repos = getParamExpandedRepos(build, listener);
if (repos.isEmpty()) return; // defensive check even though this is an invalid configuration

Expand All @@ -1130,13 +1131,20 @@ private void retrieveChanges(Run build, GitClient git, TaskListener listener) th
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() + "'");
}
}

for (RemoteConfig remoteRepository : repos) {
if (remoteRepository.equals(repos.get(0)) && removeRedundantFetch){
log.println("Avoid second fetch");
continue;
}
try {
fetchFrom(git, build, listener, remoteRepository);
} catch (GitException ex) {
Expand All @@ -1148,6 +1156,30 @@ private void retrieveChanges(Run build, GitClient git, TaskListener listener) th
}
}

private boolean determineRedundantFetch(CloneOption option, RemoteConfig rc) {
List<RefSpec> 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
}
Copy link
Member

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 if clause is perfectly fine, the "!" in the if-else might cause confusion. I would have expected

if something is true
   sentence1
else
   sentence2

while this piece of code is

if something is not false
   sentence2
else
   sentence1

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.

Copy link
Contributor Author

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.

}
}
// if initial fetch refspec contains "refs/heads/*" (default refspec), ignore the second fetch call
return removeSecondFetch;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if initialFetchRefSpecs is null, then you're removing the second fetch. At this moment, I cannot think about a situation when it will be null, but ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test that passed in a null refspec but the UserRemoteConfig constructor converts nulls to empty strings. I think that is unreachable code due to the UserRemoteConfig use of fixEmpty().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to my assumptions, the need to add the null check was to check for empty refspecs, as Mark as correctly pointed out, the fixEmpty() converts any empty refspec into a null refspec.

If the UserRemoteConfig was not fixing empty refspecs, the logic presented by me would gladly pass empty refspecs as a narrow refspec and will not avoid the second fetch. But since it is there, it should not be a concern for us.

}

@Override
public void checkout(Run<?, ?> build, Launcher launcher, FilePath workspace, TaskListener listener, File changelogFile, SCMRevisionState baseline)
throws IOException, InterruptedException {
Expand Down
170 changes: 148 additions & 22 deletions src/test/java/hudson/plugins/git/GitSCMTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.*;
Expand Down Expand Up @@ -338,6 +340,130 @@ 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);

// 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);

// 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
Expand Down Expand Up @@ -837,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<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";
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<BranchSpec> 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"));
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());
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"));
assertTrue(secondBuild.getLog().indexOf("Cleaning") < secondBuild.getLog().indexOf("Fetching upstream changes"));
assertTrue(secondBuild.getWorkspace().child(commitFile2).exists());


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")
Expand Down