Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[FIXED JENKINS-14629] return null to notify about checkout failure
relates to [JENKINS-12201]
  • Loading branch information
ndeloof committed Jul 30, 2012
1 parent 9f19244 commit 09f1708
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 2 deletions.
6 changes: 5 additions & 1 deletion src/main/java/hudson/scm/SubversionSCM.java
Expand Up @@ -684,7 +684,7 @@ public boolean checkout(AbstractBuild build, Launcher launcher, FilePath workspa

List<External> externals = checkout(build,workspace,listener,env);

if(externals==null)
if (externals==null)
return false;

// write out the revision file
Expand Down Expand Up @@ -741,6 +741,10 @@ private List<External> checkout(AbstractBuild build, FilePath workspace, TaskLis
List<External> externals = new ArrayList<External>();
for (ModuleLocation location : getLocations(env, build)) {
List<External> externalsFound = workspace.act(new CheckOutTask(build, this, location, build.getTimestamp().getTime(), listener, env));
if (externalsFound == null) {
// An error occurred during checkout
return null;
}
externals.addAll( externalsFound );
// olamy: remove null check at it cause test failure
// see https://github.com/jenkinsci/subversion-plugin/commit/de23a2b781b7b86f41319977ce4c11faee75179b#commitcomment-1551273
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/hudson/scm/subversion/UpdateUpdater.java
Expand Up @@ -164,7 +164,7 @@ public List<External> perform() throws IOException, InterruptedException {
if (e.getErrorMessage().getErrorCode() == SVNErrorCode.WC_NOT_LOCKED) {
listener.getLogger().println("Polled jobs are " + Hudson.getInstance().getDescriptorByType(SCMTrigger.DescriptorImpl.class).getItemsBeingPolled());
}
return Collections.EMPTY_LIST;
return null;

This comment has been minimized.

Copy link
@kutzi

kutzi Aug 8, 2012

Member

Javadoc of UpdateTask#perform explicitly states: '@return Where svn:external mounting happened. Can be empty but never null.'
So either we change the Javadoc or - as I'd prefer - throw an exception to indicate the error.

}

return externals;
Expand Down

4 comments on commit 09f1708

@ndeloof
Copy link
Contributor Author

@ndeloof ndeloof commented on 09f1708 Aug 9, 2012

Choose a reason for hiding this comment

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

throwing an exception would make things cleaner, but I'm confused how it will be thrown through the remote channel and we can catch it a consistent way.

@kutzi
Copy link
Member

@kutzi kutzi commented on 09f1708 Aug 9, 2012

Choose a reason for hiding this comment

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

Add an unit test for it and you'll know ;-)

@dty
Copy link
Member

@dty dty commented on 09f1708 Aug 16, 2012

Choose a reason for hiding this comment

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

The catch block for SVNCancelException already throws an InterruptedException, so throwing an IOException for the general SVNException seems like it would be more consistent than returning null.

@ndeloof
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed at 7f594bc

Please sign in to comment.