-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Consider branch name with '/' when downloading source codes #2509
base: master
Are you sure you want to change the base?
Conversation
get("/:owner/:repository/archive/*/:name")(referrersOnly { repository => | ||
val name = params("name") | ||
val path = multiParams("splat").head | ||
archiveRepository(name, repository, path) |
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.
When branch name includes /
, this endpoint was called even if you want to download whole repo. This causes NullPointerException
later.
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.
Is there any way to distinguish these routes without renaming?
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.
Can't we use the same method as this action?
gitbucket/src/main/scala/gitbucket/core/controller/RepositoryViewerController.scala
Lines 240 to 247 in 5e4d041
get("/:owner/:repository/tree/*")(referrersOnly { repository => | |
val (id, path) = repository.splitPath(multiParams("splat").head) | |
if (path.isEmpty) { | |
fileList(repository, id) | |
} else { | |
fileList(repository, id, path) | |
} | |
}) |
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.
Can't we use the same method as this action?
I guess, not. The structures of tree
route and the one of archive
route are a bit different. The former route is for directory and always includes branch (tag) name in its sub-directory, but latter route is for file.
(e.g.) If you get http://localhost:8080/root/repo/archive/release/v1.0.0.zip
, GitBucket can't decide a: release/v1.0.0
is the branch name, or b: v1.0.0
is the branch name and you want to download release
directory.
One way to deal with this issue is to divede into two routes as I did. Another way is to give branch name as a query string. Both are breaking changes, though.
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.
I meant using the same path structure with tree
by including branch or tag name in the download URL.
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.
I meant using the same path structure with tree by including branch or tag name in the download URL.
In that case, path calculation can be a bit tricky. Like this?
get("/:owner/:repository/archive/*")(referrersOnly { repository =>
val param = multiParams("splat").head
val (name, path) = repository.splitPath(param)
if (name.split('/').length == path.split('/').length) {
archiveRepository(path, repository, "")
} else {
val fileName = name + param.split(name).last
val pathList = path.substring(0, path.length - (fileName.length + 1))
archiveRepository(fileName, repository, pathList)
}
})
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.
I thought we might be able to use the same logic as tree
if URL has the following structure, for example:
- http://localhost:8080/root/repo/archive/release/v1.0.0?format=zip
- http://localhost:8080/root/repo/archive/release/v1.0.0/dir1/dir2?format=zip
However, this is just an idea. Since URL needs to be consistent and stable as much as possible, we need to design new URL very carefully if we will change it.
Need to investigate if this scalatra behavior change also affects routing here. |
@takezoe This PR is similar to #2519. Route
|
@takezoe I applied your suggestion.
Yeah, I completely agree with you, so I described as I'm wondering how GitBucket distinguishes branch name and path correctly. There are two cases to consider.
Does |
Rebased and resolved conflicts. |
Sorry for very late reply!
Certainly, if the repository has both |
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.
@SIkebe Thanks for fixing the pull request. While the new path strategy for archiving looks good to me, I left some trivial comments. Could you take a look?
<li><a href="@helpers.url(repository)/archive/@{helpers.encodeRefName(release.tag)}.zip"><i class="octicon octicon-file-zip"></i>Source code (zip)</a></li> | ||
<li><a href="@helpers.url(repository)/archive/@{helpers.encodeRefName(release.tag)}.tar.gz"><i class="octicon octicon-file-zip"></i>Source code (tar.gz)</a></li> | ||
<li><a href="@helpers.url(repository)/archive/@{helpers.encodeRefName(release.tag)}?format=zip"><i class="octicon octicon-file-zip"></i>Source code (zip)</a></li> | ||
<li><a href="@helpers.url(repository)/archive/@{helpers.encodeRefName(release.tag)}?format=tar.gz"><i class="octicon octicon-file-zip"></i>Source code (tar.gz)</a></li> |
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.
Could you also update zipball_url
and tarball_url
added in #2544? 🙇
val path = multiParams("splat").head | ||
archiveRepository(name, repository, path) | ||
get("/:owner/:repository/archive/*")(referrersOnly { repository => | ||
val format = params("format") |
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.
How about offering the previous behavior if format
parameter is not available for the backward compatibility? Generating a warning, in that case, would be fine.
Before submitting a pull-request to GitBucket I have first:
Fixes #2442