Skip to content
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.

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

Serve .patch for pull requests #3305

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

// Redirect elsewhere if it's not a pull request
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

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

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

// check .patch can be accessed and matches performed change
req = NewRequest(t, "GET", url+".patch")
resp = session.MakeRequest(t, req, http.StatusOK)
assert.Regexp(t, "\\+Hello, World \\(Edited\\)", resp.Body)
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

resp = session.MakeRequest(t, req, http.StatusOK)
assert.Regexp(t, "\\+Hello, World \\(Edited\\)", resp.Body)
assert.Regexp(t, "diff", resp.Body)
assert.Regexp(t, "Subject: \\[PATCH\\] Update 'README.md'", resp.Body)
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...

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
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web hook pull notifications .diff and .patch point to different pull and are not diffs
7 participants