Raw file API: deprecate {ref} path parameter + allow shortened commit refs #17185

delvh · 2021-09-29T21:21:20Z

noerw commented

2021-09-29 21:21:20 +00:00

(Migrated from github.com)

Looking into #17179 I noticed a couple oddities with the raw file API:

it supports an undocumented path parameter to specify the ref ({repo_api_url}/raw/{ref}/{tree_path}).
Parsing this is a mess, see context/repo.go getRefName()
due to lacking docs, a new query param ?ref= was added in #14602, now providing a duplicate parameter (and a deprecation path! 😄)
#13686 brought support for abbreviated commit SHAs, but omitted support for that in the API route, I'm not sure why (@lafriks?)

All this culminates in the weird failure mode of #17179.

This PR aims to fix #17179, but also to find a way out of this mess:

adds support for short commit SHAs on the old {ref} path parameter, to fix above bug
documents the old path parameter {ref}
deprecates the old path parameter

Backporting the actual bugfix is simpler, as mentioned in df834a7eb0

Looking into #17179 I noticed a couple oddities with the raw file API: - it supports an undocumented path parameter to specify the ref (`{repo_api_url}/raw/{ref}/{tree_path}`). Parsing this is a mess, see `context/repo.go getRefName()` - due to lacking docs, a new query param `?ref=` was added in #14602, now providing a duplicate parameter (and a deprecation path! :smile:) - #13686 brought support for abbreviated commit SHAs, but omitted support for that in the API route, I'm not sure why (@lafriks?) All this culminates in the weird failure mode of #17179. This PR aims to fix #17179, but also to find a way out of this mess: - adds support for short commit SHAs on the old `{ref}` path parameter, to fix above bug - documents the old path parameter `{ref}` - deprecates the old path parameter Backporting the actual bugfix is simpler, as mentioned in https://github.com/go-gitea/gitea/commit/df834a7eb0b93ce09f2fc3fc446a17d3b10dcb74

delvh reviewed 2021-09-29 21:48:33 +00:00

modules/context/api.go

						
				@ -415,3 +415,3 @@

							ctx.Repo.CommitID = ctx.Repo.Commit.ID.String()

						} else if len(refName) == 40 {

						} else if len(refName) >= 7 && len(refName) <= 40 {

							ctx.Repo.CommitID = refName

delvh commented

2021-09-29 21:42:22 +00:00

I doubt that that will improve the situation.
"Great. Now I cannot even get the README.md".
Maybe it would be better to check whether that file is tracked when the if three lines below is entered before returning the error.

Or do I misunderstand what this is supposed to do?
Regarding the else block below, I am fairly certain that I am misunderstanding something here.

I doubt that that will improve the situation. "Great. Now I cannot even get the README.md". Maybe it would be better to check whether that file is tracked when the if three lines below is entered before returning the error. Or do I misunderstand what this is supposed to do? Regarding the else block below, I am fairly certain that I am misunderstanding something here.

wxiaoguang reviewed 2021-09-30 04:18:21 +00:00

modules/context/api.go

						
				@ -415,3 +415,3 @@

							ctx.Repo.CommitID = ctx.Repo.Commit.ID.String()

						} else if len(refName) == 40 {

						} else if len(refName) >= 7 && len(refName) <= 40 {

							ctx.Repo.CommitID = refName

wxiaoguang commented

2021-09-30 04:18:21 +00:00

I don't think hard-coding the length is a good idea. Maybe git will use SHA256 in future.

https://stackoverflow.com/questions/28159071/why-doesnt-git-use-more-modern-sha

I don't think hard-coding the length is a good idea. Maybe git will use SHA256 in future. https://stackoverflow.com/questions/28159071/why-doesnt-git-use-more-modern-sha

noerw (Migrated from github.com) reviewed 2021-09-30 08:19:43 +00:00

modules/context/api.go

						
				@ -415,3 +415,3 @@

							ctx.Repo.CommitID = ctx.Repo.Commit.ID.String()

						} else if len(refName) == 40 {

						} else if len(refName) >= 7 && len(refName) <= 40 {

							ctx.Repo.CommitID = refName

noerw (Migrated from github.com) commented

2021-09-30 08:19:43 +00:00

@delvh no this won't happen, as we check if such a commit exists. If not, getRefName() falls back to treating the entire thing as a TreePath and returns Repo.DefaultBranch as ref, in which case we don't enter this if-clause at all.
@wxiaoguang this is all over the codebase, definitily a different issue.

@delvh no this won't happen, as we check if such a commit exists. If not, `getRefName()` falls back to treating the entire thing as a TreePath and returns Repo.DefaultBranch as ref, in which case we don't enter this if-clause at all. @wxiaoguang this is all over the codebase, definitily a different issue.

wxiaoguang reviewed 2021-09-30 08:21:53 +00:00

modules/context/api.go

						
				@ -415,3 +415,3 @@

							ctx.Repo.CommitID = ctx.Repo.Commit.ID.String()

						} else if len(refName) == 40 {

						} else if len(refName) >= 7 && len(refName) <= 40 {

							ctx.Repo.CommitID = refName