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

Gitea should not convert newlines when editing a file through the web interface #27136

Closed
anthonyvdotbe opened this issue Sep 19, 2023 · 9 comments · Fixed by #27141
Closed
Labels

Comments

@anthonyvdotbe
Copy link
Contributor

Description

When editing a file through the web interface, Gitea converts CRLF newlines to LF.

The expected behavior would be for Gitea to apply the actual changes I make in the editor, use the original newlines from the file, and commit the file. Then Git (via properties such as core.autocrlf) would decide whether the newlines must be converted.

In other words, Gitea shouldn't be opinionated about newlines (even for new files, Gitea could offer a dropdown to choose the newlines to use).

Gitea Version

1.20.4

Can you reproduce the bug on the Gitea demo site?

Yes

Log Gist

No response

Screenshots

No response

Git Version

2.41 (git version 2.41.0.windows.1)

Operating System

Windows 11 (Microsoft Windows 10.0.22621)

How are you running Gitea?

Using the official gitea-1.20.4-windows-4.0-amd64.exe, running as a Windows service as described in the Gitea docs.

Database

SQLite

@silverwind
Copy link
Member

What if the file is mixed LF and CRLF newlines? It would need a (potential expensive) heuristic.

@delvh
Copy link
Member

delvh commented Sep 19, 2023

I think @anthonyvdotbe has described it quite well:

In other words, Gitea shouldn't be opinionated about newlines

We should simply accept whatever the user tells us to use.

@silverwind
Copy link
Member

A user option might be nice, but without an option, we can only guess the dominant ending. Browsers default to LF endings, by the way, even on Windows.

@silverwind
Copy link
Member

Actually I think a user option is a bad idea. Preferred ending for a repo should be set in .gitattributes or .editorconfig and users are bound to honor these config files when editing, not introduce line ending inconsistency in otherwise consistent repos.

@delvh
Copy link
Member

delvh commented Sep 19, 2023

I repeat myself:

We should simply accept whatever the user tells us to use.

And nothing else. End of discussion.

@silverwind
Copy link
Member

silverwind commented Sep 19, 2023

The problem is, users likely don't even know which format they want or in which format the repo they are editing is. GitHub also does not have an option for this. If an option is provided, it should default to a heuristics-based value on the edited file.

In any case, I see the discussion as pointless because LF is the only sane configuration.

@anthonyvdotbe
Copy link
Contributor Author

It seems like the discussion is focused on new files now, but my main issue is with modification of existing files: if I've already committed a file with CRLF line endings, maybe I have explicitly configured Git and/or EditorConfig to use CRLF, or maybe I haven't configured anything and I don't care about line endings, or ... but no matter if I care or not: I don't want tools to silently convert newlines. For example, diff viewers like gitk will not give meaningful diffs because of this.

So when editing existing files, Gitea should just use the newlines as they already exist. In 99.9...% of the cases, a file will only have either LF or CRLF, not a mix of both. In the edge case of mixed newlines, Gitea can either refuse to allow editing (which makes sense to me: if I've managed to create a file with mixed newlines, I'd find it reasonable that I have to edit the file in a full-blown editor as well), or it can warn the user about the mixed newlines and force the user to choose one of them, or it can warn the user and just do a best-effort modification (e.g. if I fix a typo, it should still be doable to retain the mixed newlines correctly).

Sure, ideally Gitea would have support for all ways to specify newlines (Git/EditorConfig/...) and, when modifying a file with inconsistent newlines would warn about the inconsistency and propose the user to align with the specified newlines, but that would clearly be a feature, whereas the current behavior is, in my opinion, a bug.

@silverwind
Copy link
Member

silverwind commented Sep 19, 2023

Should present a dropdown that defaults to a value derived from these in descending order:

  • Editorconfig matching the current file path
  • For existing files with at least 1 LF character in it, the file's dominant ending
  • Gitattributes matching the current file path
  • LF

No persistent user setting needed.

@silverwind
Copy link
Member

For reference, monaco features this dominant line ending detection and APIs to override. Are we sure it's not already preserving the dominant line ending when editing existing files?

https://microsoft.github.io/monaco-editor/docs.html#interfaces/editor.ITextModel.html#getEOL
https://microsoft.github.io/monaco-editor/docs.html#interfaces/editor.ITextModel.html#setEOL
https://microsoft.github.io/monaco-editor/docs.html#interfaces/editor.ITextModel.html#pushEOL

silverwind added a commit that referenced this issue Sep 24, 2023
Fixes #27136.

This does the following for Monaco's EOL setting:

1. Use editorconfig setting if present
2. Use the file's dominant line ending as detected by monaco, which uses
LF for empty file
GiteaBot pushed a commit to GiteaBot/gitea that referenced this issue Sep 24, 2023
Fixes go-gitea#27136.

This does the following for Monaco's EOL setting:

1. Use editorconfig setting if present
2. Use the file's dominant line ending as detected by monaco, which uses
LF for empty file
silverwind added a commit that referenced this issue Sep 24, 2023
Backport #27141 by @silverwind

Fixes #27136.

This does the following for Monaco's EOL setting:

1. Use editorconfig setting if present
2. Use the file's dominant line ending as detected by monaco, which uses
LF for empty file

Co-authored-by: silverwind <me@silverwind.io>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 9, 2023
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