Skip to content

Serve .patch for pull requests #3305

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 7, 2018
Merged

Serve .patch for pull requests #3305

merged 3 commits into from
Jan 7, 2018

Conversation

strk
Copy link
Member

@strk strk commented Jan 5, 2018

Closes #3259
@strk strk mentioned this pull request Jan 5, 2018
Closed
6 tasks
@strk
Copy link
Member Author

strk commented Jan 5, 2018

I don't know why the patch is served as type octect-stream instead of text/plain, ideas @bkcsoft ?
@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 5, 2018
lafriks
lafriks reviewed Jan 5, 2018
View reviewed changes
routers/repo/pull.go Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong comment
@lafriks lafriks added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jan 5, 2018
@lafriks lafriks added this to the 1.4.0 milestone Jan 5, 2018
@codecov-io
Copy link

codecov-io commented Jan 5, 2018 •
edited
Loading

Codecov Report

Merging #3305 into master will decrease coverage by 0.01%.
The diff coverage is 22.22%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #3305 +/- ## ========================================== - Coverage 35.08% 35.06% -0.02%  ========================================== Files 279 279 Lines 40526 40562 +36 ========================================== + Hits 14220 14225 +5  - Misses 24206 24229 +23  - Partials 2100 2108 +8
Impacted Files Coverage Δ
routers/routes/routes.go 87.1% <100%> (+0.02%) ⬆️
routers/repo/pull.go 33.75% <20%> (-0.58%) ⬇️
models/repo_indexer.go 52.91% <0%> (-0.98%) ⬇️
models/repo.go 43.19% <0%> (-0.19%) ⬇️
models/repo_list.go 67.18% <0%> (+1.56%) ⬆️

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 18bb0f8...ab378fc. Read the comment docs.
@tboerger 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 6, 2018
@lunny
Copy link
Member

lunny commented Jan 6, 2018

@strk I would like this based on go-gitea/git#105
thehowl
thehowl approved these changes Jan 6, 2018
View reviewed changes
Copy link
Contributor

@thehowl thehowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor formatting nitpicks, otherwise looking good
integrations/pull_create_test.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not `\+Hello, World \(Edited\)` (raw string)? Seems cleaner to me
integrations/pull_create_test.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.
@strk
Copy link
Member Author

strk commented Jan 6, 2018 via email

@lafriks
Copy link
Member

lafriks commented Jan 6, 2018

@strk he was suggesting to use golang raw strings to not to have escape backslash characters in it
@strk
Copy link
Member Author

strk commented Jan 6, 2018

Use of raw strings pushed. As per @lunny PR I'll wait for it to be merged before changing this one.
As usual, I'd rather not block things, and if @lunny adds to, rather than change the code, nothing will break...
@strk
Serve .patch for pull requests
3ce7728 
Closes go-gitea#3259 Updates "git" module, for GetFormatPatch
@strk
Copy link
Member Author

strk commented Jan 7, 2018

I've updated to new git method and squash-rebased to master.
The updated code fixes inlinling of patch into browser, so ready to merge for me.
@tboerger 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 7, 2018
@lunny
Copy link
Member

lunny commented Jan 7, 2018

@strk you should handle io.Copy returned error.
@strk
Copy link
Member Author

strk commented Jan 7, 2018

error handled with 8243e4f
@lafriks lafriks merged commit 4405353 into go-gitea:master Jan 7, 2018
@strk strk deleted the serve-patch branch January 7, 2018 13:51
bkcsoft
bkcsoft reviewed Jan 8, 2018
View reviewed changes
routers/repo/pull.go
return
}

_, err = io.Copy(ctx, patch)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@strk You need to set the approriate Content-Type before this :)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is merged, if you can handle to test Content-Type in the existing integration test that'd be very useful (Content-Type was automatically added in previous implementation via ServeContent)
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.

7 participants

@strk @codecov-io @lunny @lafriks @thehowl @bkcsoft @tboerger