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

rely on try-with-resources #2570

Merged
merged 2 commits into from Oct 3, 2016

Conversation

4 participants
@ndeloof
Member

ndeloof commented Sep 30, 2016

No description provided.

@oleg-nenashev

I have a small concern regarding a.countEntries();, but other changes look good to me.

Thanks for doing it, @ndeloof !

Show outdated Hide outdated core/src/main/java/hudson/FilePath.java
scanner.scan(f,reading(a));
} finally {
a.close();
return a.countEntries();

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

Why have you moved it into try block? now we count something (potentially long) before closing the archiver. 🐜

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

Why have you moved it into try block? now we count something (potentially long) before closing the archiver. 🐜

This comment has been minimized.

@ndeloof

ndeloof Sep 30, 2016

Member

indeed, reverting this one

@ndeloof

ndeloof Sep 30, 2016

Member

indeed, reverting this one

if(mode!=0 && !Functions.isWindows()) // be defensive
_chmod(f,mode);
int mode = te.getMode() & 0777;
if (mode != 0 && !Functions.isWindows()) // be defensive

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

Unrelated formatting, but OK

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

Unrelated formatting, but OK

Show outdated Hide outdated core/src/main/java/hudson/model/AbstractItem.java
} catch (TransformerException e) {
throw new IOException("Failed to persist config.xml", e);
} catch (SAXException e) {
} catch (TransformerException|SAXException e) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

Formatting: spaces

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

Formatting: spaces

}
w.close();

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

So here was a bug, because the stream was not being closed on the exception path.

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

So here was a bug, because the stream was not being closed on the exception path.

}
zos.close();

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

And again. Unlikely there is a real reason for the original code without issue handling

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

And again. Unlikely there is a real reason for the original code without issue handling

@@ -308,7 +308,7 @@ public boolean isFull() {
* Represents the read session of the {@link Source}.
* Methods generally follow the contracts of {@link InputStream}.
*/
private interface Session {
private interface Session extends AutoCloseable {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

IIRC there was an incomplete PR from @kohsuke

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

IIRC there was an incomplete PR from @kohsuke

This comment has been minimized.

@daniel-beck

daniel-beck Oct 1, 2016

Member

That was closed after no response for months I think.

@daniel-beck

daniel-beck Oct 1, 2016

Member

That was closed after no response for months I think.

Show outdated Hide outdated core/src/main/java/hudson/model/View.java
} catch(ConversionException e) {
throw new IOException("Unable to read",e);
} catch(Error e) {// mostly reflection errors
} catch (StreamException|ConversionException|Error e) {// mostly reflection errors

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

Spaces? Some inconsistency in formatting in the PR

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

Spaces? Some inconsistency in formatting in the PR

} catch(ConversionException e) {
throw new IOException("Unable to read",e);
} catch(Error e) {// mostly reflection errors
} catch(StreamException|ConversionException|Error e) {// mostly reflection errors

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

🐛 but for the original code. Errors may be non-recoverable and should not wrapped by common exceptions in such way. they should be propagated instead

@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

🐛 but for the original code. Errors may be non-recoverable and should not wrapped by common exceptions in such way. they should be propagated instead

This comment has been minimized.

@ndeloof

ndeloof Sep 30, 2016

Member

can't tell

@ndeloof

ndeloof Sep 30, 2016

Member

can't tell

This comment has been minimized.

@daniel-beck

daniel-beck Oct 1, 2016

Member

@oleg-nenashev Should probably limit these to LinkageError?

@daniel-beck

daniel-beck Oct 1, 2016

Member

@oleg-nenashev Should probably limit these to LinkageError?

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Oct 1, 2016

Member

@daniel-beck +1. Not required for this PR imho. Maybe a subject for another bulk inspection

@oleg-nenashev

oleg-nenashev Oct 1, 2016

Member

@daniel-beck +1. Not required for this PR imho. Maybe a subject for another bulk inspection

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

But

08:26:17 [INFO] Compiling 1038 source files to /scratch/jenkins/workspace/core/jenkins-core/core/target/classes
08:26:21 [INFO] -------------------------------------------------------------
08:26:21 [ERROR] COMPILATION ERROR : 
08:26:21 [INFO] -------------------------------------------------------------
08:26:21 [ERROR] /scratch/jenkins/workspace/core/jenkins-core/core/src/main/java/hudson/XmlFile.java:[159,35] error: illegal character: \160
08:26:21 [ERROR] /scratch/jenkins/workspace/core/jenkins-core/core/src/main/java/hudson/XmlFile.java:[159,36] error: ')' expected
08:26:21 [ERROR] /scratch/jenkins/workspace/core/jenkins-core/core/src/main/java/hudson/XmlFile.java:[159,41] error: '{' expected
08:26:21 [ERROR] /scratch/jenkins/workspace/core/jenkins-core/core/src/main/java/hudson/XmlFile.java:[159,43] error: illegal start of expression
08:26:21 [ERROR] /scratch/jenkins/workspace/core/jenkins-core/core/src/main/java/hudson/XmlFile.java:[159,44] error: ';' expected
08:26:21 [INFO] 5 errors 
Member

oleg-nenashev commented Sep 30, 2016

But

08:26:17 [INFO] Compiling 1038 source files to /scratch/jenkins/workspace/core/jenkins-core/core/target/classes
08:26:21 [INFO] -------------------------------------------------------------
08:26:21 [ERROR] COMPILATION ERROR : 
08:26:21 [INFO] -------------------------------------------------------------
08:26:21 [ERROR] /scratch/jenkins/workspace/core/jenkins-core/core/src/main/java/hudson/XmlFile.java:[159,35] error: illegal character: \160
08:26:21 [ERROR] /scratch/jenkins/workspace/core/jenkins-core/core/src/main/java/hudson/XmlFile.java:[159,36] error: ')' expected
08:26:21 [ERROR] /scratch/jenkins/workspace/core/jenkins-core/core/src/main/java/hudson/XmlFile.java:[159,41] error: '{' expected
08:26:21 [ERROR] /scratch/jenkins/workspace/core/jenkins-core/core/src/main/java/hudson/XmlFile.java:[159,43] error: illegal start of expression
08:26:21 [ERROR] /scratch/jenkins/workspace/core/jenkins-core/core/src/main/java/hudson/XmlFile.java:[159,44] error: ';' expected
08:26:21 [INFO] 5 errors 
@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

@ndeloof Please do not squash commits during the review. It complicates checking of diffs with changes addressing the review feedback

Member

oleg-nenashev commented Sep 30, 2016

@ndeloof Please do not squash commits during the review. It complicates checking of diffs with changes addressing the review feedback

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev
Member

oleg-nenashev commented Sep 30, 2016

👍 from me
@jenkinsci/code-reviewers WDYT?

@ydubreuil

This comment has been minimized.

Show comment
Hide comment
@ydubreuil

ydubreuil Sep 30, 2016

Member

LGTM! Nice to see this clean up which makes it easier to read source code.

Member

ydubreuil commented Sep 30, 2016

LGTM! Nice to see this clean up which makes it easier to read source code.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Sep 30, 2016

Member

So we have 2 +1s. Let's wait for others for a couple of days (KK will likely cut off the release on Sunday if he comes back by this time)

Member

oleg-nenashev commented Sep 30, 2016

So we have 2 +1s. Let's wait for others for a couple of days (KK will likely cut off the release on Sunday if he comes back by this time)

@daniel-beck

Looks good overall, but please review my suggestions.

try {
try (InputStream in = new FileInputStream(file)) {

This comment has been minimized.

@daniel-beck

daniel-beck Oct 1, 2016

Member

Could this result in different behavior? And if so, was the previous behavior possibly wrong? I.e. relying on get to return what was set?

@daniel-beck

daniel-beck Oct 1, 2016

Member

Could this result in different behavior? And if so, was the previous behavior possibly wrong? I.e. relying on get to return what was set?

@@ -78,12 +78,9 @@ protected int run() throws Exception {
pos = logText.writeLogTo(pos, w);

This comment has been minimized.

@daniel-beck

daniel-beck Oct 1, 2016

Member

Any particular reason you're not changing the try/finally in line ~120 in this file?

@daniel-beck

daniel-beck Oct 1, 2016

Member

Any particular reason you're not changing the try/finally in line ~120 in this file?

}
f.close();

This comment has been minimized.

@daniel-beck

daniel-beck Oct 1, 2016

Member

Since Oleg likes to point out possible bugs in the old code, I'll do the same.

@daniel-beck

daniel-beck Oct 1, 2016

Member

Since Oleg likes to point out possible bugs in the old code, I'll do the same.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Oct 1, 2016

Member

Well, it makes sense to mention them and then sometimes to follow-up in further PRs. And yes, "sometimes" should happen more often

@oleg-nenashev

oleg-nenashev Oct 1, 2016

Member

Well, it makes sense to mention them and then sometimes to follow-up in further PRs. And yes, "sometimes" should happen more often

@@ -308,7 +308,7 @@ public boolean isFull() {
* Represents the read session of the {@link Source}.
* Methods generally follow the contracts of {@link InputStream}.
*/
private interface Session {
private interface Session extends AutoCloseable {

This comment has been minimized.

@daniel-beck

daniel-beck Oct 1, 2016

Member

That was closed after no response for months I think.

@daniel-beck

daniel-beck Oct 1, 2016

Member

That was closed after no response for months I think.

@@ -752,14 +752,11 @@ public void doRestart(StaplerResponse rsp) throws IOException, ServletException
*/
public String getBackupVersion() {
try {
JarFile backupWar = new JarFile(new File(Lifecycle.get().getHudsonWar() + ".bak"));
try {
try (JarFile backupWar = new JarFile(new File(Lifecycle.get().getHudsonWar() + ".bak"))) {

This comment has been minimized.

@daniel-beck

daniel-beck Oct 1, 2016

Member

Wasn't there a problem recently with FarFile being terrible?

@daniel-beck

daniel-beck Oct 1, 2016

Member

Wasn't there a problem recently with FarFile being terrible?

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Oct 1, 2016

Member

Now, the issue was with JarUrlConnection. JarFile is less terrible

@oleg-nenashev

oleg-nenashev Oct 1, 2016

Member

Now, the issue was with JarUrlConnection. JarFile is less terrible

Show outdated Hide outdated core/src/main/java/hudson/model/UpdateCenter.java
Attributes attrs = backupWar.getManifest().getMainAttributes();
String v = attrs.getValue("Jenkins-Version");
if (v==null) v = attrs.getValue("Hudson-Version");
if (v == null) v = attrs.getValue("Hudson-Version");

This comment has been minimized.

@daniel-beck

daniel-beck Oct 1, 2016

Member

🐛 Extra spaces after the condition when the true-case statement is on the same line is intentional, but please move it to the next line.

@daniel-beck

daniel-beck Oct 1, 2016

Member

🐛 Extra spaces after the condition when the true-case statement is on the same line is intentional, but please move it to the next line.

} catch(ConversionException e) {
throw new IOException("Unable to read",e);
} catch(Error e) {// mostly reflection errors
} catch(StreamException|ConversionException|Error e) {// mostly reflection errors

This comment has been minimized.

@daniel-beck

daniel-beck Oct 1, 2016

Member

@oleg-nenashev Should probably limit these to LinkageError?

@daniel-beck

daniel-beck Oct 1, 2016

Member

@oleg-nenashev Should probably limit these to LinkageError?

FileInputStream fin = new FileInputStream(f);
try {
try (PrintWriter out = new PrintWriter(new BufferedWriter(w))) {
try (FileInputStream fin = new FileInputStream(f)) {

This comment has been minimized.

@daniel-beck

daniel-beck Oct 1, 2016

Member

Why are these still nested?

@daniel-beck

daniel-beck Oct 1, 2016

Member

Why are these still nested?

This comment has been minimized.

@ndeloof

ndeloof Oct 1, 2016

Member

I used intellj automated refactoring here, didn't tried to inspect the code in detail, there's room for further improvements

@ndeloof

ndeloof Oct 1, 2016

Member

I used intellj automated refactoring here, didn't tried to inspect the code in detail, there's room for further improvements

try {
Class<?> t = classLoader.loadClass(line);
if (!type.isAssignableFrom(t)) continue; // invalid type
if (!type.isAssignableFrom(t)) continue; // invalid type

This comment has been minimized.

@daniel-beck

daniel-beck Oct 1, 2016

Member

Again, extra spaces are deliberate, but it's better to move to the next line.

@daniel-beck

daniel-beck Oct 1, 2016

Member

Again, extra spaces are deliberate, but it's better to move to the next line.

Certificate certificate;
try {
try (InputStream in = j.servletContext.getResourceAsStream(cert)) {
if (in == null) continue; // our test for paths ending in / should prevent this from happening

This comment has been minimized.

@daniel-beck

daniel-beck Oct 1, 2016

Member

continue should be on separate line.

@daniel-beck

daniel-beck Oct 1, 2016

Member

continue should be on separate line.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Oct 2, 2016

Member

needs-fix since @daniel-beck bugged it

Member

oleg-nenashev commented Oct 2, 2016

needs-fix since @daniel-beck bugged it

@ndeloof

This comment has been minimized.

Show comment
Hide comment
@ndeloof

ndeloof Oct 2, 2016

Member

@daniel-beck @oleg-nenashev 🐛 just for a space on some (undocumented) formatting practice ? This is exactly what makes me nervous with code review :-\

Member

ndeloof commented Oct 2, 2016

@daniel-beck @oleg-nenashev 🐛 just for a space on some (undocumented) formatting practice ? This is exactly what makes me nervous with code review :-\

@ndeloof ndeloof closed this Oct 2, 2016

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Oct 2, 2016

Member

@ndeloof Well, I honestly disagree with @daniel-beck regarding the spaces criticality. Not a blocker IMHO. Do you plan to address changes and reopen the PR btw?

Member

oleg-nenashev commented Oct 2, 2016

@ndeloof Well, I honestly disagree with @daniel-beck regarding the spaces criticality. Not a blocker IMHO. Do you plan to address changes and reopen the PR btw?

@ndeloof ndeloof reopened this Oct 3, 2016

honor undocumented formatting rule
See but very important  as not following this will result in :bugs: during code review
@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Oct 3, 2016

Member

It impact readability. The previous style is already not my favorite but without extra spaces it makes it even more likely to miss that there's another statement on the same line.

If you touch the code (and especially if you change nothing else), it shouldn't become even worse.

Member

daniel-beck commented Oct 3, 2016

It impact readability. The previous style is already not my favorite but without extra spaces it makes it even more likely to miss that there's another statement on the same line.

If you touch the code (and especially if you change nothing else), it shouldn't become even worse.

@ndeloof

This comment has been minimized.

Show comment
Hide comment
@ndeloof

ndeloof Oct 3, 2016

Member

@daniel-beck we definitively disagree here on readability. I consider null checks such a pain in java that I prefer such single line checks and can't see any benefit in extra spaces

Member

ndeloof commented Oct 3, 2016

@daniel-beck we definitively disagree here on readability. I consider null checks such a pain in java that I prefer such single line checks and can't see any benefit in extra spaces

@ndeloof ndeloof removed the needs-fix label Oct 3, 2016

@ndeloof ndeloof merged commit 57fc218 into jenkinsci:master Oct 3, 2016

0 of 2 checks passed

Jenkins Jenkins is validating pull request ...
Details
continuous-integration/jenkins/pr-head This commit is being built
Details
@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Oct 3, 2016

Member

Seems it has been merge without letting the PR builder to pass. Not a problem in this case (especially since the PR builder is borked)

Member

oleg-nenashev commented Oct 3, 2016

Seems it has been merge without letting the PR builder to pass. Not a problem in this case (especially since the PR builder is borked)

@ndeloof

This comment has been minimized.

Show comment
Hide comment
@ndeloof

ndeloof Oct 3, 2016

Member

I indeed noticed PR build was successful but didn't reported status on
github

Le 3 oct. 2016 3:40 PM, "Oleg Nenashev" notifications@github.com a écrit :

Seems it has been merge without letting the PR builder to pass. Not a
problem in this case (especially since the PR builder is borked)


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#2570 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIGlWgDEuD3vg1q2kgLZg3OuNTPkAXtks5qwQW_gaJpZM4KLB_5
.

Member

ndeloof commented Oct 3, 2016

I indeed noticed PR build was successful but didn't reported status on
github

Le 3 oct. 2016 3:40 PM, "Oleg Nenashev" notifications@github.com a écrit :

Seems it has been merge without letting the PR builder to pass. Not a
problem in this case (especially since the PR builder is borked)


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#2570 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIGlWgDEuD3vg1q2kgLZg3OuNTPkAXtks5qwQW_gaJpZM4KLB_5
.

daniel-beck added a commit that referenced this pull request Oct 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment