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

Send commit object in http response header for archives #17923

Closed

Conversation

milahu
Copy link

@milahu milahu commented Dec 6, 2021

fix #17834

demo

url=http://localhost:3000/asdfasdf/asdfasdf/archive/master.zip

curl --verbose --header 'X-Commit-Object: 1' --output /dev/null $url 2>&1 | grep X-Commit-Object 
> X-Commit-Object: 1
< X-Commit-Object: dHJlZSAyZTkwMGI5YTFhOTllZWM3YzlmMDFlN2QxMTNmNDg4YWQyOGU0NmVmCmF1dGhvciBhc2RmYXNkZiA8YXNkZkBsb2NhbGhvc3QubG9jYWxkb21haW4+IDE2Mzg3OTkyNzggKzAxMDAKY29tbWl0dGVyIGFzZGZhc2RmIDxhc2RmQGxvY2FsaG9zdC5sb2NhbGRvbWFpbj4gMTYzODc5OTI3OCArMDEwMAoKSW5pdGlhbCBjb21taXQK

echo -n dHJlZSAyZTkwMGI5YTFhOTllZWM3YzlmMDFlN2QxMTNmNDg4YWQyOGU0NmVmCmF1dGhvciBhc2RmYXNkZiA8YXNkZkBsb2NhbGhvc3QubG9jYWxkb21haW4+IDE2Mzg3OTkyNzggKzAxMDAKY29tbWl0dGVyIGFzZGZhc2RmIDxhc2RmQGxvY2FsaG9zdC5sb2NhbGRvbWFpbj4gMTYzODc5OTI3OCArMDEwMAoKSW5pdGlhbCBjb21taXQK | base64 -d
tree 2e900b9a1a99eec7c9f01e7d113f488ad28e46ef
author asdfasdf <asdf@localhost.localdomain> 1638799278 +0100
committer asdfasdf <asdf@localhost.localdomain> 1638799278 +0100

Initial commit

@lafriks lafriks added the type/enhancement An improvement of existing functionality label Dec 6, 2021
@lafriks lafriks changed the title send commit object in http response header Send commit object in http response header for archives Dec 6, 2021
@zeripath
Copy link
Contributor

zeripath commented Dec 6, 2021

You need to run make fmt

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 6, 2021
@zeripath
Copy link
Contributor

zeripath commented Dec 6, 2021

Why not just send the sha instead?

// We use base64 to ensure a lossless encoding of binary data.
// test: gitea/integrations/repo_download_test.go
func addCommitObjectResponseHeader(ctx *context.Context, archiver *repo_model.RepoArchiver) {
if ctx.Req.Header.Get("X-Commit-Object") != "1" {
Copy link
Contributor

@wxiaoguang wxiaoguang Dec 7, 2021

Choose a reason for hiding this comment

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

Using X-Commit-Object for different purpose in request and response is not a good idea.

HTTP has Content-Length in request and response, it has the same meaning.

But now the X-Commit-Object is used for different meaning. I think we can make it like this:

request:

X-Request-Extra-Info: commit-id

response:

X-Extra-Commit-ID: .....

If we want to add more options, we can extend: X-Request-Extra-Info: commit-message, commit-author ...

Copy link
Author

Choose a reason for hiding this comment

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

is not a good idea

why not?

If we want to add more options

then we can add more headers

keep it simple : )

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?

As explained, the X-Commit-Object has different meanings, will mislead users and developers.

keep it simple : )

I don't think X-Request-Extra-Info + X-Extra-Commit-ID is more complex than X-Commit-Object + X-Commit-Object.

Copy link
Author

Choose a reason for hiding this comment

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

thanks for your opinion, but youre just wasting my time. bye : )

Copy link
Author

Choose a reason for hiding this comment

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

unresolve

its interesting that bad taste and hostage-taking so often appear together.
probably both are facets of the same personality type (conservative)

in the real world, i would kill the tyrant, and install someone who is "less bad".
in the digital world? lets just pretend this never happened ... "muh ideals!"

Copy link
Contributor

@wxiaoguang wxiaoguang Dec 10, 2021

Choose a reason for hiding this comment

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

unresolve

its interesting that bad taste and hostage-taking so often appear together.
probably both are facets of the same personality type (conservative)

in the real world, i would kill the tyrant, and install someone who is "less bad".
in the digital world? lets just pretend this never happened ... "muh ideals!"

Are you talking about yourself?

For me, I feel fine. I submitted some PRs, approved many PRs and helped some contributors to improve their PRs, like #17152, #17900, #17929, etc.

If you don't like my suggestions, that's your choice. I won't waste time on non-sense communications.

We can keep this review open and let everyone read it.

@milahu

This comment has been minimized.

@codecov-commenter

This comment has been minimized.

@stale
Copy link

stale bot commented Apr 29, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Apr 29, 2022
@wxiaoguang
Copy link
Contributor

It has been stale for long time and doesn't get interest or update. I will close it.

You are always welcome if there could be a open and friendly discussion.

@wxiaoguang wxiaoguang closed this Oct 13, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/stale lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

archive endpoint should provide commit object
6 participants