-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
[JEP-202] Extending VirtualFile #3302
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
Changes from 10 commits
3f01a77
8e603a5
e773bdb
4911c5c
e3e6d57
2e070ca
9735a34
a9430d3
e0fab1a
9ccffba
7131067
9b9f81d
23f12fd
63885e6
64ee6fd
c7372cc
fca93db
d32b9b1
beea2aa
7223632
d3f5346
2e1af78
90fd89f
9b75cd2
8e60538
2090468
a345051
547fd6f
a54d49b
d25c0b2
8f5d1d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,9 +29,11 @@ | |
| import java.io.InputStream; | ||
| import java.io.OutputStream; | ||
| import java.io.Serializable; | ||
| import java.net.URL; | ||
| import java.text.Collator; | ||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.Collection; | ||
| import java.util.Collections; | ||
| import java.util.Comparator; | ||
| import java.util.List; | ||
|
|
@@ -51,6 +53,7 @@ | |
| import org.kohsuke.accmod.Restricted; | ||
| import org.kohsuke.accmod.restrictions.NoExternalUse; | ||
| import org.kohsuke.stapler.HttpResponse; | ||
| import org.kohsuke.stapler.HttpResponses; | ||
| import org.kohsuke.stapler.StaplerRequest; | ||
| import org.kohsuke.stapler.StaplerResponse; | ||
|
|
||
|
|
@@ -295,6 +298,14 @@ private void serveFile(StaplerRequest req, StaplerResponse rsp, VirtualFile root | |
| return; | ||
| } | ||
|
|
||
| URL external = baseFile.toExternalURL(); | ||
| if (external != null) { | ||
| // or this URL could be emitted directly from dir.jelly | ||
| // though we would prefer to delay toExternalURL calls unless and until needed | ||
| rsp.sendRedirect2(external.toExternalForm()); | ||
| return; | ||
| } | ||
|
|
||
| long lastModified = baseFile.lastModified(); | ||
| long length = baseFile.length(); | ||
|
|
||
|
|
@@ -355,7 +366,7 @@ private static String createBackRef(int times) { | |
| private static void zip(OutputStream outputStream, VirtualFile dir, String glob) throws IOException { | ||
| try (ZipOutputStream zos = new ZipOutputStream(outputStream)) { | ||
| zos.setEncoding(System.getProperty("file.encoding")); // TODO JENKINS-20663 make this overridable via query parameter | ||
| for (String n : dir.list(glob.length() == 0 ? "**" : glob)) { | ||
| for (String n : dir.list(glob.isEmpty() ? "**" : glob, null, /* TODO what is the user expectation? */true)) { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Retaining default excludes functionality here to maintain semantics but I am unsure whether users would want this or not. |
||
| String relativePath; | ||
| if (glob.length() == 0) { | ||
| // JENKINS-19947: traditional behavior is to prepend the directory name | ||
|
|
@@ -535,10 +546,10 @@ private static List<List<Path>> buildChildPaths(VirtualFile cur, Locale locale) | |
| * @param baseRef String like "../../../" that cancels the 'rest' portion. Can be "./" | ||
| */ | ||
| private static List<List<Path>> patternScan(VirtualFile baseDir, String pattern, String baseRef) throws IOException { | ||
| String[] files = baseDir.list(pattern); | ||
| Collection<String> files = baseDir.list(pattern, null, /* TODO what is the user expectation? */true); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that there was a JEP-200 regression in the similar code recently
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, in |
||
|
|
||
| if (files.length > 0) { | ||
| List<List<Path>> r = new ArrayList<List<Path>>(files.length); | ||
| if (!files.isEmpty()) { | ||
| List<List<Path>> r = new ArrayList<List<Path>>(files.size()); | ||
| for (String match : files) { | ||
| List<Path> file = buildPathList(baseDir, baseDir.child(match), baseRef); | ||
| r.add(file); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,6 @@ | |
| import java.io.File; | ||
| import java.io.FileFilter; | ||
| import java.io.IOException; | ||
| import java.io.InterruptedIOException; | ||
| import java.io.Serializable; | ||
|
|
||
| import static hudson.Util.fixEmpty; | ||
|
|
@@ -31,19 +30,15 @@ public abstract class DirScanner implements Serializable { | |
| */ | ||
| protected final void scanSingle(File f, String relative, FileVisitor visitor) throws IOException { | ||
| if (visitor.understandsSymlink()) { | ||
| String target; | ||
| try { | ||
| String target; | ||
| try { | ||
| target = Util.resolveSymlink(f); | ||
| } catch (IOException x) { // JENKINS-13202 | ||
| target = null; | ||
| } | ||
| if (target != null) { | ||
| visitor.visitSymlink(f, target, relative); | ||
| return; | ||
| } | ||
| } catch (InterruptedException e) { | ||
| throw (IOException) new InterruptedIOException().initCause(e); | ||
| target = Util.resolveSymlink(f); | ||
| } catch (IOException x) { // JENKINS-13202 | ||
| target = null; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add logging while you are around?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Preferred to avoid making unrelated changes.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed |
||
| } | ||
| if (target != null) { | ||
| visitor.visitSymlink(f, target, relative); | ||
| return; | ||
| } | ||
| } | ||
| visitor.visit(f, relative); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Binary compatible simplification—the body never actually threw that exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not source compatible IIRC (javac fails when there is a catch block for impossible exception), may impact PCT. There are usages of it externally, e.g. in the Support Core plugin: https://github.com/search?p=1&q=org%3Ajenkinsci+resolveSymlink&type=Code
I would propose to detach it to a separate PR if you feel it's important enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oleg-nenashev Jesse already disputed this in #3340 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, PCT should be unaffected, though it could impede
Why the Java language treats an exception which cannot be thrown as a fatal error rather than a warning (ditto unreachable statements etc.), I do not know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The simple workaround for the
buildPlugincase would be to justcatch (ExceptionI guess.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be better to fix it tho