Consider branch name with '/' when downloading source codes

[SIkebe]

Before submitting a pull-request to GitBucket I have first:

• read the contribution guidelines
• rebased my branch over master
• verified that project is compiling
• verified that tests are passing
• squashed my commits as appropriate (keep several commits if it is relevant to understand the PR)
• marked as closed using commit message all issue ID that this PR should correct

Fixes #2442

[SIkebe]

When branch name includes /, this endpoint was called even if you want to download whole repo. This causes NullPointerException later.

[SIkebe]

Is there any way to distinguish these routes without renaming?

[takezoe]

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)
	}
	})

[SIkebe]

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.

[takezoe]

I meant using the same path structure with tree by including branch or tag name in the download URL.

[SIkebe]

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)
  }
})

[takezoe]

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.

[SIkebe]

Need to investigate if this scalatra behavior change also affects routing here.

[SIkebe]

@takezoe This PR is similar to #2519. Route "/:owner/:repository/archive/:name" is not called if name contains /.
It needs to be "/:owner/:repository/archive/*", however the new route conflicts with "/:owner/:repository/archive/*/:name".
So I divided into two routes:

• "/:owner/:repository/archive-whole-repo/*"
• "/:owner/:repository/archive-partial-repo/*"

[SIkebe]

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

@takezoe I applied your suggestion.

we need to design new URL very carefully if we will change it.

Yeah, I completely agree with you, so I described as breaking changes.

I'm wondering how GitBucket distinguishes branch name and path correctly. There are two cases to consider.

• You want to download v1.0.0/dir1/dir2 as a zip file from release branch.
• You want to download dir1/dir2 as a zip file from release/v1.0.0 branch.

Does splitPath assume that the former case is too rare?

[SIkebe]

Rebased and resolved conflicts.

[takezoe]

Sorry for very late reply!

Does splitPath assume that the former case is too rare?

Certainly, if the repository has both release and release/v1.0.0 branch (or tag), these two cases cannot be distinguished. Actually, many of repository viewer endpoints have the same issue. Since this is not an this endpoint specific issue, I think it's okay to follow other endpoints for now. It should be considered as an entire GitBucket issue.

[takezoe]

@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?

[takezoe]

Could you also update zipball_url and tarball_url added in #2544? 🙇

[takezoe]

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.