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.

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-14575] Fix to make the retry logic work with GitSCM #143

Merged
merged 4 commits into from Apr 4, 2013

Conversation

Projects
None yet
5 participants
@woohgit
Copy link

woohgit commented Mar 28, 2013

The retry logic does not catch the GitException, so we have to wrap into a IOException. Clone and Fetch also affected. As of now the scmRetry will work with the git plugin.

Adam PAPAI
[JENKINS-14575] Wrap GitException into IOException
The retry logic does not catch the GitException, so we have to wrap into a IOException. Clone and Fetch also affected. As of now the scmRetry will work with the git plugin.
@buildhive

This comment has been minimized.

Copy link

buildhive commented Mar 28, 2013

Jenkins » git-plugin #224 SUCCESS
This pull request looks good
(what's this?)

Adam PAPAI added some commits Mar 28, 2013

Adam PAPAI
[JENKINS-14575] Wrap GitException into IOException
Do the same if the repo exists and don't have to clone.
@buildhive

This comment has been minimized.

Copy link

buildhive commented Mar 28, 2013

Jenkins » git-plugin #225 SUCCESS
This pull request looks good
(what's this?)

@kutzi

This comment has been minimized.

AFAI can see fetchFrom never throws GitException, so why catch it here?

This comment has been minimized.

Copy link

kutzi replied Mar 30, 2013

Anyway, if it would throw GitException, it would be IMO be a bad idea to swallow it silently, as you do here

This comment has been minimized.

Copy link
Owner

woohgit replied Mar 30, 2013

It throws the GitException from the fetchFrom.

hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:793)

But it's not swallowed, because the fetched is going to be false, so we'll throw an IOException at the end.

What do you suggest to do?

Because since this modification it works for me. So it seems to be fix the issue.

This comment has been minimized.

Copy link

kutzi replied Mar 30, 2013

793 can throw GitException, but that's already caught (and logged) in line 795. So just remove your catch blocks.

By 'swallowed' I meant that the details (e.g. stacktrace) of the original exception would be lost in your catch blocks.

This comment has been minimized.

Copy link
Owner

woohgit replied Mar 31, 2013

You're absolutely right. I've removed it.

@kutzi

This comment has been minimized.

Copy link

kutzi commented on 33e66e1 Mar 30, 2013

That's not really your fault, but the determineRevisionToBuild method has become really huge and could definitely need some refactoring IMO.

@buildhive

This comment has been minimized.

Copy link

buildhive commented Mar 31, 2013

Jenkins » git-plugin #228 UNSTABLE
Looks like there's a problem with this pull request
(what's this?)

@ndeloof

This comment has been minimized.

Copy link
Member

ndeloof commented Mar 31, 2013

@kutzi agree on requirement for some refactoring. Time to cleanup this plugin codebase

@jooooooon

This comment has been minimized.

Copy link

jooooooon commented Apr 3, 2013

Any chance this can get merged before the refactoring happens? This fixes a pretty damn annoying bug, and quite a few folk have expressed interest in seeing a fix... Here's hoping there's a chance :)

ndeloof added a commit that referenced this pull request Apr 4, 2013

Merge pull request #143 from woohgit/master
[JENKINS-14575] Fix to make the retry logic work with GitSCM

@ndeloof ndeloof merged commit f49bc41 into jenkinsci:master Apr 4, 2013

mshibuya referenced this pull request in mshibuya/git-client-plugin Aug 19, 2013

lastorset added a commit to lastorset/git-plugin that referenced this pull request Mar 18, 2014

JENKINS-14575: Make failed Git fetch throw IOException
This is a port of a fix for version 1 of the plugin. The previous fix
was in [old-fix] and reviewed at [github].

[old-fix] 3ab1db8..f49bc41
[github]  jenkinsci#143

lastorset added a commit to lastorset/git-plugin that referenced this pull request Mar 18, 2014

JENKINS-14575: Make failed Git fetch throw IOException
This is a port of a fix for version 1 of the plugin. The previous fix
was in [old-fix] and reviewed at [github]. This version is smaller,
because a failed clone in version 2 throws AbortException instead of
GitException, and it makes sense to keep it that way.

[old-fix] 3ab1db8..f49bc41
[github]  jenkinsci#143
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment