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

CSV diff: unable to load file from base commit #21184

Closed
otbutz opened this issue Sep 16, 2022 · 7 comments · Fixed by #21189
Closed

CSV diff: unable to load file from base commit #21184

otbutz opened this issue Sep 16, 2022 · 7 comments · Fixed by #21189
Labels
issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself type/bug

Comments

@otbutz
Copy link

otbutz commented Sep 16, 2022

Description

Creating a new CSV file as part of a pull request doesn't display the files content in the PRs Files changed tab:

https://try.gitea.io/otbutz99/test/pulls/1/files

The file is valid and renders just fine if viewed directly:

https://try.gitea.io/otbutz99/test/src/commit/988460b06f64270788a9f30ed02947e4ab1e6f5a/SampleCSVFile_11kb.csv

The CSV diff works fine for me if the content of the CSV is just changed in the PR:

https://try.gitea.io/otbutz99/test/pulls/2/files

Gitea Version

1.17.2

Can you reproduce the bug on the Gitea demo site?

Yes

Log Gist

No response

Screenshots

image

Git Version

No response

Operating System

No response

How are you running Gitea?

Release binary via systemd

Database

PostgreSQL

@KN4CK3R
Copy link
Member

KN4CK3R commented Sep 16, 2022

That's a regression from #19552. This PR enforces base and head to be valid csv files which is not the case if a csv gets created.

@KN4CK3R KN4CK3R added the issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself label Sep 16, 2022
@delvh
Copy link
Member

delvh commented Sep 16, 2022

Or deleted?

@KN4CK3R
Copy link
Member

KN4CK3R commented Sep 16, 2022

That may fail too.

@KN4CK3R
Copy link
Member

KN4CK3R commented Sep 16, 2022

What #19530 (comment) states is wrong because CreateCsvDiff is intended to handle nil readers. I could revert the other PR but without test case for the other issue there may be a new error.

@otbutz
Copy link
Author

otbutz commented Sep 16, 2022

That may fail too.

Confirmed: https://try.gitea.io/otbutz99/test/pulls/3/files

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 16, 2022

Oh yes, I misunderstood the csvReaderFromCommit and CreateCsvDiff logic at that time. Then the question is whether #19552 really fixed the problem (#19530 (comment) said that the 500 error occurs on deleted files)


Update (2023-02-17): #22946, CreateCsvDiff should be able to process the situation when base and head are both nil.

@KN4CK3R
Copy link
Member

KN4CK3R commented Sep 16, 2022

I will add a new PR with better error handling which should work in these cases.

lunny pushed a commit that referenced this issue Sep 17, 2022
Fixes #21184
Regression of #19552

Instead of using `GetBlobByPath` I use the already existing instances.

We need more information from #19530 if that error is still present.
wxiaoguang pushed a commit to wxiaoguang/gitea that referenced this issue Sep 17, 2022
Fixes go-gitea#21184
Regression of go-gitea#19552

Instead of using `GetBlobByPath` I use the already existing instances.

We need more information from go-gitea#19530 if that error is still present.
wxiaoguang added a commit that referenced this issue Sep 17, 2022
Backport #21189
Fixes #21184
Regression of #19552

Instead of using `GetBlobByPath`, use the already existing instances.
@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/regression Issue needs no code to be fixed, only a description on how to fix it yourself type/bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants