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

Revert "Fix EOL handling in web editor" #28101

Merged
merged 2 commits into from Nov 22, 2023
Merged

Conversation

lng2020
Copy link
Member

@lng2020 lng2020 commented Nov 17, 2023

Reverts #27141
close #28097

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 17, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 17, 2023
@lng2020 lng2020 added skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. backport/v1.21 This PR should be backported to Gitea 1.21 and removed modifies/frontend labels Nov 17, 2023
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 17, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 17, 2023
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

I can't accept a full revert, the editorconfig detection has to stay, that is valuable in any case.

Also the linked issue seems odd, unless monaco uses browser OS, in which case we should override it to LF for cases with no .editorconfig and add a ini setting for it.

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Nov 17, 2023
@wxiaoguang
Copy link
Contributor

This behavior already effects end users.

If there is no quick fix (eg: in 1 or 2 days), let's revert it to old behavior. Then you have enough time to propose a new fix.

@silverwind
Copy link
Member

silverwind commented Nov 17, 2023

It's a trivial fix, I will provide soon. That's assuming the current behaviour can even be considered "broken", I think monaco is doing the sensible thing by defaulting to user's platform, thought I guess in terms of git content, more users prefer LF.

@silverwind
Copy link
Member

#28119

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 21, 2023

#28119 is not right. Will merge this reverting PR in 1 day.

@silverwind
Copy link
Member

I'm strongly against this because it removes editorconfig support.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 21, 2023

I'm strongly against this because it removes editorconfig support.

No, it doesn't "remove editorconfig support" because #27141 is not right either. Actually:

  • Before #27141: all edited files are stored as \n, few users complain.
  • After #27141: all edited files are stored as \r\n, many users complain.

I have explained above: if there is no quick fix, reverting is the best choice.

@wxiaoguang
Copy link
Contributor

@silverwind the last call for the merge in 12h, if you still have objection, please show a real case to help understanding the "problem" you are worrying about.

@lunny
Copy link
Member

lunny commented Nov 21, 2023

I'm strongly against this because it removes editorconfig support.

But it broke many users' online editors. I was surprised when I edit a readme and only add one new line then all lines were changed with no warning until I take a look at diff.

@silverwind
Copy link
Member

silverwind commented Nov 21, 2023

The only "necessary" revert is this line which previously did the replacement on the server:

https://github.com/go-gitea/gitea/pull/27141/files#diff-898533f7342f43a8390c353ae398fa85578aaa93667b3ff99a5dd889c048e1c7L290

Ideally we should just convert line endings based on the new server pref.

@silverwind
Copy link
Member

silverwind commented Nov 21, 2023

In #28119, I added server-side convertion in e58dcc0, this will always be correct, no matter what format the client side delivers.

Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

Revert if you want a quick fix. I guess I will then make another PR the cherry-picks #27141 and #28119 in that sequence which can then land only on 1.22.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Nov 22, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Nov 22, 2023
@lunny lunny enabled auto-merge (squash) November 22, 2023 08:47
@lunny lunny merged commit 37ed92d into go-gitea:main Nov 22, 2023
25 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Nov 22, 2023
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Nov 22, 2023
@GiteaBot GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Nov 22, 2023
silverwind pushed a commit that referenced this pull request Nov 22, 2023
Backport #28101 by @lng2020

Reverts #27141
close #28097

Co-authored-by: Nanguan Lin <70063547+lng2020@users.noreply.github.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Nov 23, 2023
* giteaofficial/main:
  Revert "Fix EOL handling in web editor" (go-gitea#28101)
  Fix swagger title (go-gitea#28164)
fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Feb 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1.21 broke Linux EOL in code editor
6 participants