From f6a4146161fda22341c17dc74d42fd13ad181e1f Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 14 Nov 2023 19:24:20 +0100 Subject: [PATCH] [GITEA] Accept shorter commit IDs in web route - Be more liberal in what Forgejo accepts, by reducing the minimum amount of characters for SHA to 4 characters, which is the minimum amount that Git needs in order to figure out which commit was meant. - It's safe to reduce this requirements, as commits are passed to Git which will error if the given commit ID results in more than one Git object. Forgejo will catch this error as that the Commit doesn't exist, which is a error that's already being handled in most places gracefully. - Added integration testing. - Resolves https://codeberg.org/forgejo/forgejo/issues/1760 (cherry picked from commit 0d655c7384b081c36aa4c6b7167280f52c1c42d3) (cherry picked from commit 9b9aca2a02b06f41f6db847a77ea29f6385b46d2) (cherry picked from commit 0d0ab1af1fb05e26168c112523f1400fef67f9b0) (cherry picked from commit d3b352c85482e59c9d1da24a8fe0eb68b0f5858e) (cherry picked from commit d6af2094df4611d590d8c5062743f5e39f2a7bd8) (cherry picked from commit f96e55a7a9f06ff987a5e9663da492720d162b76) (cherry picked from commit bb6261f8479ee8925ddc7f0079b414ef85f04d73) --- routers/web/web.go | 20 ++++++++--------- tests/integration/repo_test.go | 40 ++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 10 deletions(-) diff --git a/routers/web/web.go b/routers/web/web.go index 6ca4277faf..5172d33d03 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1229,7 +1229,7 @@ func registerRoutes(m *web.Route) { Post(web.Bind(forms.UploadRepoFileForm{}), repo.UploadFilePost) m.Combo("/_diffpatch/*").Get(repo.NewDiffPatch). Post(web.Bind(forms.EditRepoFileForm{}), repo.NewDiffPatchPost) - m.Combo("/_cherrypick/{sha:([a-f0-9]{7,40})}/*").Get(repo.CherryPick). + m.Combo("/_cherrypick/{sha:([a-f0-9]{4,40})}/*").Get(repo.CherryPick). Post(web.Bind(forms.CherryPickForm{}), repo.CherryPickPost) }, repo.MustBeEditable) m.Group("", func() { @@ -1371,8 +1371,8 @@ func registerRoutes(m *web.Route) { m.Combo("/*"). Get(repo.Wiki). Post(context.RepoMustNotBeArchived(), reqSignIn, reqRepoWikiWriter, web.Bind(forms.NewWikiForm{}), repo.WikiPost) - m.Get("/commit/{sha:[a-f0-9]{7,40}}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.Diff) - m.Get("/commit/{sha:[a-f0-9]{7,40}}.{ext:patch|diff}", repo.RawDiff) + m.Get("/commit/{sha:[a-f0-9]{4,40}}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.Diff) + m.Get("/commit/{sha:[a-f0-9]{4,40}}.{ext:patch|diff}", repo.RawDiff) }, repo.MustEnableWiki, func(ctx *context.Context) { ctx.Data["PageIsWiki"] = true ctx.Data["CloneButtonOriginLink"] = ctx.Repo.Repository.WikiCloneLink() @@ -1432,7 +1432,7 @@ func registerRoutes(m *web.Route) { m.Group("/commits", func() { m.Get("", context.RepoRef(), repo.SetWhitespaceBehavior, repo.GetPullDiffStats, repo.ViewPullCommits) m.Get("/list", context.RepoRef(), repo.GetPullCommits) - m.Get("/{sha:[a-f0-9]{7,40}}", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForSingleCommit) + m.Get("/{sha:[a-f0-9]{4,40}}", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForSingleCommit) }) m.Post("/merge", context.RepoMustNotBeArchived(), web.Bind(forms.MergePullRequestForm{}), repo.MergePullRequest) m.Post("/cancel_auto_merge", context.RepoMustNotBeArchived(), repo.CancelAutoMergePullRequest) @@ -1441,8 +1441,8 @@ func registerRoutes(m *web.Route) { m.Post("/cleanup", context.RepoMustNotBeArchived(), context.RepoRef(), repo.CleanUpPullRequest) m.Group("/files", func() { m.Get("", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForAllCommitsOfPr) - m.Get("/{sha:[a-f0-9]{7,40}}", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesStartingFromCommit) - m.Get("/{shaFrom:[a-f0-9]{7,40}}..{shaTo:[a-f0-9]{7,40}}", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForRange) + m.Get("/{sha:[a-f0-9]{4,40}}", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesStartingFromCommit) + m.Get("/{shaFrom:[a-f0-9]{4,40}}..{shaTo:[a-f0-9]{4,40}}", context.RepoRef(), repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.SetShowOutdatedComments, repo.ViewPullFilesForRange) m.Group("/reviews", func() { m.Get("/new_comment", repo.RenderNewCodeCommentForm) m.Post("/comments", web.Bind(forms.CodeCommentForm{}), repo.SetShowOutdatedComments, repo.CreateCodeComment) @@ -1492,9 +1492,9 @@ func registerRoutes(m *web.Route) { m.Group("", func() { m.Get("/graph", repo.Graph) - m.Get("/commit/{sha:([a-f0-9]{7,40})$}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.Diff) - m.Get("/commit/{sha:([a-f0-9]{7,40})$}/load-branches-and-tags", repo.LoadBranchesAndTags) - m.Get("/cherry-pick/{sha:([a-f0-9]{7,40})$}", repo.SetEditorconfigIfExists, repo.CherryPick) + m.Get("/commit/{sha:([a-f0-9]{4,40})$}", repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.Diff) + m.Get("/commit/{sha:([a-f0-9]{4,40})$}/load-branches-and-tags", repo.LoadBranchesAndTags) + m.Get("/cherry-pick/{sha:([a-f0-9]{4,40})$}", repo.SetEditorconfigIfExists, repo.CherryPick) }, repo.MustBeNotEmpty, context.RepoRef(), reqRepoCodeReader) m.Get("/rss/branch/*", repo.MustBeNotEmpty, context.RepoRefByType(context.RepoRefBranch), feedEnabled, feed.RenderBranchFeed("rss")) @@ -1511,7 +1511,7 @@ func registerRoutes(m *web.Route) { m.Group("", func() { m.Get("/forks", repo.Forks) }, context.RepoRef(), reqRepoCodeReader) - m.Get("/commit/{sha:([a-f0-9]{7,40})}.{ext:patch|diff}", repo.MustBeNotEmpty, reqRepoCodeReader, repo.RawDiff) + m.Get("/commit/{sha:([a-f0-9]{4,40})}.{ext:patch|diff}", repo.MustBeNotEmpty, reqRepoCodeReader, repo.RawDiff) }, ignSignIn, context.RepoAssignment, context.UnitTypes()) m.Post("/{username}/{reponame}/lastcommit/*", ignSignInAndCsrf, context.RepoAssignment, context.UnitTypes(), context.RepoRefByType(context.RepoRefCommit), reqRepoCodeReader, repo.LastCommit) diff --git a/tests/integration/repo_test.go b/tests/integration/repo_test.go index 8176fea5e8..ed5466bc63 100644 --- a/tests/integration/repo_test.go +++ b/tests/integration/repo_test.go @@ -592,3 +592,43 @@ func TestViewCommit(t *testing.T) { resp := MakeRequest(t, req, http.StatusNotFound) assert.True(t, test.IsNormalPageCompleted(resp.Body.String()), "non-existing commit should render 404 page") } + +func TestCommitView(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + t.Run("Non-existent commit", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "GET", "/user2/repo1/commit/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") + MakeRequest(t, req, http.StatusNotFound) + }) + + t.Run("Too short commit ID", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "GET", "/user2/repo1/commit/65f") + MakeRequest(t, req, http.StatusNotFound) + }) + + t.Run("Short commit ID", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "GET", "/user2/repo1/commit/65f1") + resp := MakeRequest(t, req, http.StatusOK) + + doc := NewHTMLParser(t, resp.Body) + commitTitle := doc.Find(".commit-summary").Text() + assert.Contains(t, commitTitle, "Initial commit") + }) + + t.Run("Full commit ID", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "GET", "/user2/repo1/commit/65f1bf27bc3bf70f64657658635e66094edbcb4d") + resp := MakeRequest(t, req, http.StatusOK) + + doc := NewHTMLParser(t, resp.Body) + commitTitle := doc.Find(".commit-summary").Text() + assert.Contains(t, commitTitle, "Initial commit") + }) +}