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

PL/SQL double dash comments in diff #14711

Closed
bartkaptur opened this issue Feb 17, 2021 · 5 comments · Fixed by #14804
Closed

PL/SQL double dash comments in diff #14711

bartkaptur opened this issue Feb 17, 2021 · 5 comments · Fixed by #14804
Labels
Milestone

Comments

@bartkaptur
Copy link

bartkaptur commented Feb 17, 2021

Hi.
I use Gitea 1.13.2 on top of PostgreSQL.
A comment on a pull request including an Oracle pl/sql file with "--" comments results in something like this in DB in comment.patch:

diff --git a/sql/FUND/PACKAGES/CORRECTIONS.pkb b/sql/FUND/PACKAGES/CORRECTIONS.pkb
--- a/sql/FUND/PACKAGES/CORRECTIONS.pkb
+++ b/sql/FUND/PACKAGES/CORRECTIONS.pkb
---  some comment
---  some other comment
@@ -2061,839 +2119,733 @@
     END LOOP;
 
     IF gv_test_type THEN
-      Dr.begin ('Debit correction summary');
+      dr.begin ('Debit correction summary');
@@ -2703,0 +2522,4 @@
+                            l_register,
+                            v_hssr,
+                            vt_events (i_zd).oper_type,
+                            correction_cost.GC_ALG_PERC_SUM_BR_M, -- TODO

What happens next is that presumably the pulls panel treats the two "some comment" lines like the file locations above it and tries to link to them. The effect:

template: repo/issue/view_content/comments:470:20: executing "repo/issue/view_content/comments" at <CommentMustAsDiff (index $comms 0)>: error calling CommentMustAsDiff: runtime error: invalid memory address or nil pointer dereference

Manual removal of the two lines results in the pull request displayed properly.
Please check.

@lunny lunny added the type/bug label Feb 17, 2021
@lunny
Copy link
Member

lunny commented Feb 17, 2021

The patch on master(fe628d8) may resolve the problem.

diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go
index d5c392351..caba8365b 100644
--- a/services/gitdiff/gitdiff.go
+++ b/services/gitdiff/gitdiff.go
@@ -802,9 +802,9 @@ parsingLoop:
                                curFile.Type = DiffFileRename
                        case strings.HasPrefix(line, "Binary"):
                                curFile.IsBin = true
-                       case strings.HasPrefix(line, "--- "):
+                       case strings.HasPrefix(line, "--- a/"+curFile.OldName):
                                // Do nothing with this line
-                       case strings.HasPrefix(line, "+++ "):
+                       case strings.HasPrefix(line, "+++ b/"+curFile.Name):
                                // Do nothing with this line
                                lineBytes, isFragment, err := parseHunks(curFile, maxLines, maxLineCharacters, input)
                                diff.TotalAddition += curFile.Addition

@lunny lunny added this to the 1.13.3 milestone Feb 17, 2021
@zeripath
Copy link
Contributor

Please can you try to replicate on try.gitea.io. I do not believe that master should be affected by this problem.

@bartkaptur
Copy link
Author

Seems to work fine on try.gitea.io. No error. Thank you.
There is, hovewer, another problem. When I go to the "Files changed" tab and add a single comment on a deleted -- comment in a commited file, I see the -- comment twice in the Conversation tab, like this:
screenshot_gitea

@zeripath
Copy link
Contributor

Can you make arandomer@mailinator.com a collaborator on that repo so I can test.

@bartkaptur
Copy link
Author

Done. bkaptur/test_oracle

zeripath added a commit to zeripath/gitea that referenced this issue Feb 27, 2021
… always represent part of the header of a diff.

This PR adds a flag to its parsing to prevent this problem and adds a streaming parsing technique to CutDiffAroundLine using an io.pipe instead of just sending data to an unbounded buffer.

Fix go-gitea#14711

Signed-off-by: Andrew Thornton <art27@cantab.net>
6543 pushed a commit that referenced this issue Feb 27, 2021
* CutDiffAroundLine makes the incorrect assumption that `---` and `+++` always represent part of the header of a diff.

This PR adds a flag to its parsing to prevent this problem and adds a streaming parsing technique to CutDiffAroundLine using an io.pipe instead of just sending data to an unbounded buffer.

Fix #14711

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Handle unquoted comment patch files

When making comment patches unfortunately the patch does not always quote the filename
This makes the diff --git header ambiguous again.

This PR finally adds handling for ambiguity in to parse patch

Fix #14812

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Add in testing for no error

There is no way currently for CutDiffAroundLine in this test to cause an
error however, it should still be tested.

Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath added a commit to zeripath/gitea that referenced this issue Feb 27, 2021
Backport go-gitea#14804

* CutDiffAroundLine makes the incorrect assumption that `---` and `+++` always represent part of the header of a diff.

This PR adds a flag to its parsing to prevent this problem and adds a streaming parsing technique to CutDiffAroundLine using an io.pipe instead of just sending data to an unbounded buffer.

Fix go-gitea#14711

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Handle unquoted comment patch files

When making comment patches unfortunately the patch does not always quote the filename
This makes the diff --git header ambiguous again.

This PR finally adds handling for ambiguity in to parse patch

Fix go-gitea#14812

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Add in testing for no error

There is no way currently for CutDiffAroundLine in this test to cause an
error however, it should still be tested.

Signed-off-by: Andrew Thornton <art27@cantab.net>
lafriks pushed a commit that referenced this issue Feb 28, 2021
Backport #14804

* CutDiffAroundLine makes the incorrect assumption that `---` and `+++` always represent part of the header of a diff.

This PR adds a flag to its parsing to prevent this problem and adds a streaming parsing technique to CutDiffAroundLine using an io.pipe instead of just sending data to an unbounded buffer.

Fix #14711

* Handle unquoted comment patch files

When making comment patches unfortunately the patch does not always quote the filename
This makes the diff --git header ambiguous again.

This PR finally adds handling for ambiguity in to parse patch

Fix #14812

* Add in testing for no error

There is no way currently for CutDiffAroundLine in this test to cause an
error however, it should still be tested.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants