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

WIP: Add commit sha to filepath api #13599

Closed
wants to merge 5 commits into from
Closed

Conversation

johanvdw
Copy link
Contributor

This is an attempt to give a solution to #12840

I'm not really satisfied with the tests yet, as the repo that is used in the test has only one commit, it's not really possible to check that the returned commit is correct.

@techknowlogick techknowlogick added the modifies/api This PR adds API routes or modifies them label Nov 23, 2020
@@ -62,6 +62,7 @@ func getExpectedFileResponseForCreate(commitID, treePath string) *api.FileRespon
SHA: sha,
Size: 16,
Type: "file",
Commit: CommitID,
Copy link
Member

Choose a reason for hiding this comment

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

Should this be?

Suggested change
Commit: CommitID,
Commit: commitID,

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

zeripath commented Nov 29, 2020

This looks OK to me. Just would like you to confirm that this matches the github api.

It appears that this does not match the Github API.

So I think we need to be very careful here. This is potentially quite slow to generate.

@@ -146,13 +146,19 @@ func GetContents(repo *models.Repository, treePath, ref string, forList bool) (*
}
selfURLString := selfURL.String()

lastCommit, err := commit.GetCommitByPath(treePath)
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be quite slow depending on the version of git present and the size of the repository.

Internally this code may need to be tied in to the last commit cache.

@johanvdw
Copy link
Contributor Author

This looks OK to me. Just would like you to confirm that this matches the github api.

It appears that this does not match the Github API.
If we want the same functionality as the github api, we should probably go this way to solve the same problem:
#12840 (comment)

Would you prefer that approach?

@johanvdw
Copy link
Contributor Author

in general, I think we would better return 'lastcommit' rather than commit, as this is not obvious very obvious in some cases.

@noerw noerw added the pr/wip This PR is not ready for review label May 14, 2021
@stale
Copy link

stale bot commented Jul 21, 2021

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 Jul 21, 2021
@yardenshoham
Copy link
Member

This is in progress for a while now, I'm closing it to avoid stale pull requests. Please reopen when it's ready for review.

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 1, 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. modifies/api This PR adds API routes or modifies them pr/wip This PR is not ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants