Serve pull request .diff files #3293

strk · 2018-01-03T13:26:34Z

lafriks · 2018-01-03T13:57:04Z

routers/routes/routes.go

@@ -591,6 +591,7 @@ func RegisterRoutes(m *macaron.Macaron) {
 		m.Group("", func() {
 			m.Get("/^:type(issues|pulls)$", repo.RetrieveLabels, repo.Issues)
 			m.Get("/^:type(issues|pulls)$/:index", repo.ViewIssue)
+			m.Get("/pulls/:index.diff", repo.DownloadPullDiff)


There should be check that user has rights to access models.UnitTypePullRequests

why isn't it done for the route before ? How's .diff different from the non-diff URL ?

If you would move it to this part of route than you would not need to check rights as all group has right check already:
https://github.com/strk/gitea/blob/43c2bfa2d016d899936a178b5b5676e27d13c8dc/routers/routes/routes.go#L629

There is no check for group as these routes are common for issues and PR and each have it's own rights

moved with c60fa34b

lafriks · 2018-01-03T13:58:04Z

routers/repo/pull.go

+
+	// Redirect elsewhere if it's not a pull request
+	if !issue.IsPull {
+		ctx.Redirect(ctx.Repo.RepoLink + "/issues/" + com.ToStr(issue.Index))


It should not redirect as this will be confusing when using from command line to download patch, instead it should return 404

Done with 9149d562

codecov-io · 2018-01-03T14:14:11Z

Codecov Report

Merging #3293 into master will decrease coverage by <.01%.
The diff coverage is 20.68%.

@@            Coverage Diff             @@
##           master    #3293      +/-   ##
==========================================
- Coverage   34.81%   34.81%   -0.01%     
==========================================
  Files         277      277              
  Lines       40271    40303      +32     
==========================================
+ Hits        14021    14031      +10     
- Misses      24189    24207      +18     
- Partials     2061     2065       +4

Impacted Files	Coverage Δ
models/repo.go	`43.2% <ø> (ø)`	⬆️
routers/routes/routes.go	`87.08% <100%> (+0.02%)`	⬆️
routers/repo/pull.go	`33.73% <17.85%> (-0.57%)`	⬇️
modules/avatar/avatar.go	`100% <0%> (+18.75%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce7ae17...e15748f. Read the comment docs.

strk · 2018-01-03T14:50:55Z

For implementing .patch we'll need go-gitea/git#103, please also review that one

strk · 2018-01-03T14:51:58Z

@lafriks I believe I addressed all your concerns here

lunny · 2018-01-04T00:56:21Z

routers/routes/routes.go

@@ -624,6 +624,7 @@ func RegisterRoutes(m *macaron.Macaron) {
 		}, repo.MustBeNotBare, context.RepoRef(), context.CheckUnit(models.UnitTypeCode))

 		m.Group("/pulls/:index", func() {
+			m.Get(".diff", repo.DownloadPullDiff)


I think maybe it's not work?

sapk · 2018-01-04T01:27:10Z

A test would be great. Maybe add it to https://github.com/go-gitea/gitea/blob/master/integrations/pull_create_test.go#L47

strk · 2018-01-04T10:13:08Z

@sapk how do I run that test in isolation ?

strk · 2018-01-04T10:42:07Z

@sapk afaaa991 adds integration test.
@lunny you can see with that test that the code works

lunny · 2018-01-04T11:31:23Z

routers/repo/pull.go

+
+// DownloadPullDiff render a pull's raw diff
+func DownloadPullDiff(ctx *context.Context) {
+


blank line is unnecessary.

lunny · 2018-01-04T11:33:28Z

routers/repo/pull.go

+
+	pr := issue.PullRequest
+
+	if pr.BaseRepo == nil {


use err = pr.GetBaseRepo() is better.

See if you like 8150dbb9 (I'm not sure GetBaseRepo was much better...)

lafriks · 2018-01-04T19:55:03Z

routers/repo/pull.go

+		return
+	}
+	if pr.BaseRepo == nil {
+		ctx.Handle(500, "BaseRepo",


Just move this condition to previous if and you will not need this section. err != nil || pr.BaseRepo == nil

upon reading the correct GetBaseRepo method I realized there's no way pr.BaseRepo is nil after calling GetBaseRepo and getting no error, so I removed the whole check for null in lattest commit

Closes go-gitea#3259

…seRepo

strk · 2018-01-05T09:46:09Z

Can I get some LGTM and a merge here please ?

strk mentioned this pull request Jan 3, 2018

Web hook pull notifications .diff and .patch point to different pull and are not diffs #3259

Closed

6 tasks

strk changed the title ~~Serve pull request .patch files~~ Serve pull request .diff files Jan 3, 2018

lafriks requested changes Jan 3, 2018

View reviewed changes

tboerger added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 3, 2018

lafriks added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jan 3, 2018

lafriks added this to the 1.4.0 milestone Jan 3, 2018

lunny reviewed Jan 4, 2018

View reviewed changes

lunny requested changes Jan 4, 2018

View reviewed changes

lafriks requested changes Jan 4, 2018

View reviewed changes

strk added 2 commits January 4, 2018 21:42

Serve pull request .diff files

1ce38a4

Closes go-gitea#3259

Add test for pull request redirection and .diff access

e8c2c92

strk force-pushed the serve-patch branch from 8150dbb to e8c2c92 Compare January 4, 2018 20:42

strk added 2 commits January 4, 2018 22:21

Typo

a422a78

There's no need to test for pr.BaseRepo being nil after calling GetBa…

e15748f

…seRepo

lunny approved these changes Jan 5, 2018

View reviewed changes

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 5, 2018

lafriks approved these changes Jan 5, 2018

View reviewed changes

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 5, 2018

sapk approved these changes Jan 5, 2018

View reviewed changes

lafriks merged commit a192f30 into go-gitea:master Jan 5, 2018

strk deleted the serve-patch branch January 5, 2018 18:57

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serve pull request .diff files #3293

Serve pull request .diff files #3293

strk commented Jan 3, 2018

lafriks Jan 3, 2018

strk Jan 3, 2018

lafriks Jan 3, 2018

lafriks Jan 3, 2018

strk Jan 3, 2018

lafriks Jan 3, 2018

strk Jan 3, 2018

codecov-io commented Jan 3, 2018 •

edited

strk commented Jan 3, 2018

strk commented Jan 3, 2018

lunny Jan 4, 2018

sapk commented Jan 4, 2018

strk commented Jan 4, 2018

strk commented Jan 4, 2018

lunny Jan 4, 2018

lunny Jan 4, 2018

strk Jan 4, 2018

lafriks Jan 4, 2018

strk Jan 4, 2018

strk commented Jan 5, 2018 via email


		// DownloadPullDiff render a pull's raw diff
		func DownloadPullDiff(ctx *context.Context) {

Serve pull request .diff files #3293

Serve pull request .diff files #3293

Conversation

strk commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 3, 2018 • edited

Codecov Report

strk commented Jan 3, 2018

strk commented Jan 3, 2018

Choose a reason for hiding this comment

sapk commented Jan 4, 2018

strk commented Jan 4, 2018

strk commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strk commented Jan 5, 2018 via email

codecov-io commented Jan 3, 2018 •

edited