Commenting on a diff of a file with spaces separating short tokens makes the PR page inaccessible #6931

@zeripath commented on GitHub (Feb 26, 2021):

yup - https://try.gitea.io/templatepanic/templatepanic/pulls/3/files this doesn't panic.

@zeripath commented on GitHub (Feb 26, 2021): yup - https://try.gitea.io/templatepanic/templatepanic/pulls/3/files this doesn't panic.

GiteaMirror commented

@zeripath commented on GitHub (Feb 26, 2021):

OK so this is not a failure with the code I spent so effing long on making work for rendering of diffs in the first place.

@zeripath commented on GitHub (Feb 26, 2021): OK so this is not a failure with the code I spent so effing long on making work for rendering of diffs in the first place.

GiteaMirror commented

@zeripath commented on GitHub (Feb 26, 2021):

No doubt the problem will be similar - in the end the reality is that there is no way to reliably split up: diff a/filename b/filename2 without forcing it to be always quoted.

@zeripath commented on GitHub (Feb 26, 2021): No doubt the problem will be similar - in the end the reality is that there is no way to reliably split up: diff a/filename b/filename2 without forcing it to be always quoted.

GiteaMirror commented

@zeripath commented on GitHub (Feb 27, 2021):

Just to highlight this issue here is a particularly catastrophic example:

diff --git a/b b/b b/b b/b
deleted file mode 100644
index 92e798b..0000000
--- a/b b/b	
+++ /dev/null
@@ -1 +0,0 @@
-b b/b
diff --git a/b b/b b/b b/b b/b b/b b/b b/b
new file mode 100644
index 0000000..1bc3698
--- /dev/null
+++ b/b b/b b/b b/b	
@@ -0,0 +1 @@
+b b/b is now b b/b b/b b/b
diff --git a/b b/b b/b b/b b/b b/b
similarity index 100%
rename from b
rename to b b/b b/b b/b b/b
diff --git a/regular_text_file.rb b/regular_text_file.rb
new file mode 100644
index 0000000..6aa8f4e
--- /dev/null
+++ b/regular_text_file.rb
@@ -0,0 +1,3 @@
+# frozen_string_literal: true
+
+puts 'Foobar!'

from the diff --git lines alone what are the new and old filenames...

@zeripath commented on GitHub (Feb 27, 2021): Just to highlight this issue here is a particularly catastrophic example: ```diff diff --git a/b b/b b/b b/b deleted file mode 100644 index 92e798b..0000000 --- a/b b/b +++ /dev/null @@ -1 +0,0 @@ -b b/b diff --git a/b b/b b/b b/b b/b b/b b/b b/b new file mode 100644 index 0000000..1bc3698 --- /dev/null +++ b/b b/b b/b b/b @@ -0,0 +1 @@ +b b/b is now b b/b b/b b/b diff --git a/b b/b b/b b/b b/b b/b similarity index 100% rename from b rename to b b/b b/b b/b b/b diff --git a/regular_text_file.rb b/regular_text_file.rb new file mode 100644 index 0000000..6aa8f4e --- /dev/null +++ b/regular_text_file.rb @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +puts 'Foobar!' ``` from the `diff --git` lines alone what are the new and old filenames...

GiteaMirror commented

@zeripath commented on GitHub (Feb 27, 2021):

oh yeah and this particular doozy:

diff --git a/b b/b b/b b/b b/b b/b
similarity index 100%
rename from b b/b b/b b/b b/b
rename to b
diff --git a/b b/b b/b b/b b/b b/b
similarity index 100%
rename from b b/b b/b b/b
rename to b b/b

@zeripath commented on GitHub (Feb 27, 2021): oh yeah and this particular doozy: ```diff diff --git a/b b/b b/b b/b b/b b/b similarity index 100% rename from b b/b b/b b/b b/b rename to b diff --git a/b b/b b/b b/b b/b b/b similarity index 100% rename from b b/b b/b b/b rename to b b/b ```

GiteaMirror commented

@abread commented on GitHub (Feb 27, 2021):

Yesterday I bit the bullet and looked through all the code calling git (started by looking for everything using os/exec and went up from there). I found only 43 places that are not ready to handle quoted paths (1 looks like dead code).

I think I can get a PR fixing this (and potentially other issues with paths) that flips core.quotedPath to true done today

@abread commented on GitHub (Feb 27, 2021): Yesterday I bit the bullet and looked through all the code calling git (started by looking for everything using `os/exec` and went up from there). I found only ~~4~~3 places that are not ready to handle quoted paths (1 looks like dead code). I think I can get a PR fixing this (and potentially other issues with paths) that flips `core.quotedPath` to `true` done today

GiteaMirror commented

@zeripath commented on GitHub (Feb 27, 2021):

No don't worry I'm there already

@zeripath commented on GitHub (Feb 27, 2021): No don't worry I'm there already

GiteaMirror commented

@zeripath commented on GitHub (Feb 27, 2021):

Just thinking about fixing the comment as patch code to make it quote or not but I guess that needs to be done as a different PR.

@zeripath commented on GitHub (Feb 27, 2021): Just thinking about fixing the comment as patch code to make it quote or not but I guess that needs to be done as a different PR.

GiteaMirror commented

@zeripath commented on GitHub (Feb 27, 2021):

Hmm. I think I'm gonna merge the fix into #14804

@zeripath commented on GitHub (Feb 27, 2021): Hmm. I think I'm gonna merge the fix into #14804

GiteaMirror commented

@abread commented on GitHub (Feb 27, 2021):

I think it's easier to just assume paths are quoted in ParsePatch and change the ~3 callers to call git -c core.quotedpath=true ... no?

@abread commented on GitHub (Feb 27, 2021): I think it's easier to just assume paths are quoted in `ParsePatch` and change the ~3 callers to call `git -c core.quotedpath=true ...` no?

GiteaMirror commented

@zeripath commented on GitHub (Feb 27, 2021):

ah you've made the mistake of assuming that these particular "diffs" came from git - they're from some code that hacks them out of git diffs.

The reality is that these diffs are stored in plaintext in your db so we still need to be able to parse them.

In any case core.quotedpath doesn't guarantee what you think it does. The answer for forcing every git from 1.7.2+ to quote its paths is to pass git diff --src-prefix=\\a/ --dst-prefix=\\b/ and then trim off the \\.

Honestly it's just crazy.

@zeripath commented on GitHub (Feb 27, 2021): ah you've made the mistake of assuming that these particular "diffs" came from git - they're from some code that hacks them out of git diffs. The reality is that these diffs are stored in plaintext in your db so we still need to be able to parse them. In any case core.quotedpath doesn't guarantee what you think it does. The answer for forcing every git from 1.7.2+ to quote its paths is to pass `git diff --src-prefix=\\a/ --dst-prefix=\\b/` and then trim off the `\\`. Honestly it's just crazy.

GiteaMirror commented

@abread commented on GitHub (Feb 27, 2021):

Ah right I was forgetting those

At one point I had the idea of gitea doctoring them into sane quotedness, but now that I think about it the only reason they're in the DB is because git gc could throw away the originals right?

On February 27, 2021 1:30:42 PM UTC, zeripath notifications@github.com wrote:

ah you've made the mistake of assuming that these particular "diffs" came from git - they're from some code that hacks them out of git diffs.

The reality is that these diffs are stored in plaintext in your db so we still need to be able to parse them.

In any case core.quotedpath doesn't guarantee what you think it does. The answer for forcing every git from 1.7.2+ to quote its paths is to pass git diff --src-prefix=\\a/ --dst-prefix=\\b/ and then trim off the \\.

Honestly it's just crazy.

--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
https://github.com/go-gitea/gitea/issues/14812#issuecomment-787073079

@abread commented on GitHub (Feb 27, 2021): Ah right I was forgetting those At one point I had the idea of `gitea doctor`ing them into sane quotedness, but now that I think about it the only reason they're in the DB is because `git gc` could throw away the originals right? On February 27, 2021 1:30:42 PM UTC, zeripath <notifications@github.com> wrote: >ah you've made the mistake of assuming that these particular "diffs" came from git - they're from some code that hacks them out of git diffs. > >The reality is that these diffs are stored in plaintext in your db so we still need to be able to parse them. > >In any case core.quotedpath doesn't guarantee what you think it does. The answer for forcing every git from 1.7.2+ to quote its paths is to pass `git diff --src-prefix=\\a/ --dst-prefix=\\b/` and then trim off the `\\`. > >Honestly it's just crazy. > >-- >You are receiving this because you authored the thread. >Reply to this email directly or view it on GitHub: >https://github.com/go-gitea/gitea/issues/14812#issuecomment-787073079

GiteaMirror commented

@skillcoder commented on GitHub (Mar 18, 2021):

I'm updatated gitea 1.13.2 -> 1.13.4
But I still have see broken PR

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

It's normal ?

@skillcoder commented on GitHub (Mar 18, 2021): I'm updatated gitea 1.13.2 -> 1.13.4 But I still have see broken PR ``` 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 ``` It's normal ?

GiteaMirror commented

@miili commented on GitHub (Mar 18, 2021):

Same here. Is there any way to fix this?
Can we fix the bad PR or throw it away?

@miili commented on GitHub (Mar 18, 2021): Same here. Is there any way to fix this? Can we fix the bad PR or throw it away?

GiteaMirror commented