Handle too long PR titles correctly #16517

zeripath · 2021-07-22T17:17:55Z

zeripath commented

2021-07-22 17:17:55 +00:00

The CompareAndPullRequestPost handler for POST to /compare
incorrectly handles returning errors to the user. For a start
it does not set the necessary markers to switch SimpleMDE
but it also does not immediately return to the form.

This PR fixes this by setting the appropriate values, fixing
the templates and preventing the suggestion of a too long
title.

Fix #16507

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

The CompareAndPullRequestPost handler for POST to /compare incorrectly handles returning errors to the user. For a start it does not set the necessary markers to switch SimpleMDE but it also does not immediately return to the form. This PR fixes this by setting the appropriate values, fixing the templates and preventing the suggestion of a too long title. Fix #16507 Signed-off-by: Andrew Thornton <art27@cantab.net>

👍 2

lafriks (Migrated from github.com) reviewed 2021-07-22 19:45:08 +00:00

routers/web/repo/compare.go

lafriks (Migrated from github.com) commented

2021-07-22 19:45:08 +00:00

Could use Unicode character instead of "..." and use 254 characters for title

zeripath reviewed 2021-07-22 19:57:15 +00:00

routers/web/repo/compare.go

zeripath commented

2021-07-22 19:57:15 +00:00

So I initially considered that but remembered - perhaps incorrectly - that github doesn't use … U+2026 for some reason itself.

So I initially considered that but remembered - perhaps incorrectly - that github doesn't use … `U+2026` for some reason itself.

lafriks (Migrated from github.com) reviewed 2021-07-23 06:35:38 +00:00

routers/web/repo/compare.go

lafriks (Migrated from github.com) commented

2021-07-23 06:35:38 +00:00

We use in all UI parts it already, on other hand this will go to database so idk ?

zeripath reviewed 2021-07-23 07:16:10 +00:00

routers/web/repo/compare.go

zeripath commented

2021-07-23 07:16:10 +00:00

I mean if we're willing to risk the mysql and mssql users' wrath and/or people who insist on non utf8 in their commit logs then switching to use ellipsis instead would be easy.

lafriks (Migrated from github.com) reviewed 2021-07-23 07:25:09 +00:00

routers/web/repo/compare.go

lafriks (Migrated from github.com) commented

2021-07-23 07:25:09 +00:00

I don't understand this to be honest ?... Not that there is still software that does not support utf8
Anyway this was just suggestion and is non blocking from my side

I don't understand this to be honest ?... Not that there is still software that does not support utf8 Anyway this was just suggestion and is non blocking from my side

zeripath reviewed 2021-07-23 07:51:01 +00:00

routers/web/repo/compare.go

zeripath commented

2021-07-23 07:51:00 +00:00

lets try the ellipsis.

lafriks (Migrated from github.com) reviewed 2021-07-23 07:57:00 +00:00

routers/web/repo/pull.go

lafriks (Migrated from github.com) commented

2021-07-23 07:57:00 +00:00

same here should be changed then

zeripath reviewed 2021-07-23 08:05:31 +00:00

routers/web/repo/pull.go

zeripath commented

2021-07-23 08:05:31 +00:00

			form.Content = "…" + form.Title[254:] + "\n\n" + form.Content

```suggestion form.Content = "…" + form.Title[254:] + "\n\n" + form.Content ```

zeripath reviewed 2021-07-23 08:06:10 +00:00

routers/web/repo/pull.go

zeripath commented

2021-07-23 08:06:10 +00:00

			form.Title = form.Title[:254] + "…"

```suggestion form.Title = form.Title[:254] + "…" ```

lafriks (Migrated from github.com) approved these changes 2021-07-23 08:28:54 +00:00

lunny requested changes 2021-07-24 03:46:21 +00:00

routers/web/repo/compare.go

lunny commented

2021-07-24 03:46:16 +00:00

This is still not correct because you have to consider utf8. Simply use title[254:] will break some of utf8 string.

This is still not correct because you have to consider utf8. Simply use `title[254:]` will break some of utf8 string.

zeripath reviewed 2021-07-24 06:24:38 +00:00

routers/web/repo/compare.go

zeripath commented

2021-07-24 06:24:38 +00:00

Yup looks like we'd need to cast to []rune and then drop the last rune.

Probably also need to consider if the last rune is a combining character too.

Yup looks like we'd need to cast to []rune and then drop the last rune. Probably also need to consider if the last rune is a combining character too.

zeripath reviewed 2021-07-24 08:08:48 +00:00

routers/web/repo/compare.go

zeripath commented

2021-07-24 08:08:48 +00:00

and we would need to consider this in chi-binding too.

zeripath reviewed 2021-07-24 08:39:22 +00:00

routers/web/repo/compare.go

zeripath commented

2021-07-24 08:39:22 +00:00

I've switched the code to use byte length and truncation algorithm

techknowlogick approved these changes 2021-07-24 20:52:31 +00:00

lunny reviewed 2021-07-25 02:03:29 +00:00

modules/util/truncate.go

						
				@ -0,0 +14,4 @@

					}

					if !utf8.ValidString(input) {

						left = input[:n-3] + "..."

lunny commented

2021-07-25 02:03:29 +00:00

ValidString don't mean it's totally ascii. Maybe we should correct it.

`ValidString` don't mean it's totally ascii. Maybe we should correct it.

lunny approved these changes 2021-07-25 02:04:00 +00:00

zeripath reviewed 2021-07-25 02:59:03 +00:00

modules/util/truncate.go

						
				@ -0,0 +14,4 @@

					}

					if !utf8.ValidString(input) {

						left = input[:n-3] + "..."

zeripath commented

2021-07-25 02:59:02 +00:00

The test here is to simply abort trying to be nice if the string is not a valid utf8 string. (Which could be ASCII, Big5, UTF-16...)

If you don't have a utf-8 string in your commit header - getting it truncated nicely is going to be the least of your worries. We really don't handle other encodings in git commit headers.

The test here is to simply abort trying to be nice if the string is not a valid utf8 string. (Which could be ASCII, Big5, UTF-16...) If you don't have a utf-8 string in your commit header - getting it truncated nicely is going to be the least of your worries. We really don't handle other encodings in git commit headers.

This repo is archived. You cannot comment on pull requests.

No reviewers