Beep6581 / RawTherapee

A powerful cross-platform raw photo processing program
https://rawtherapee.com
GNU General Public License v3.0
2.91k stars 324 forks source link

Line ending consistency #1190

Closed Beep6581 closed 9 years ago

Beep6581 commented 9 years ago

Originally reported on Google Code with ID 1205

The source files have all sorts of various line endings. Some contain a mixture of line
endings within a single file, and this can cause problems. I would like to unify them
all to LF, and the Windows-only ones to CRLF.
Anyone against?

Tested locally, went fine, RT compiles and runs fine.
There should be no risk or downside to this.

These files would be modified:
M AUTHORS.txt
M About-Windows.cmake
M AboutThisBuild.txt.in
M COMPILE.txt
M ProcessorTargets.cmake
M Win32CMakeOptions-Sample.txt
M WindowsEnvironmentSetup.html
M header
M languagePack.nsi
M rtdata/languages/Catala
M rtdata/languages/Chinese (Simplified)
M rtdata/languages/Chinese (Traditional)
M rtdata/languages/Dansk
M rtdata/languages/Deutsch
M rtdata/languages/English
M rtdata/languages/English (UK)
M rtdata/languages/English (US)
M rtdata/languages/Espanol
M rtdata/languages/Euskara
M rtdata/languages/Francais
M rtdata/languages/Greek
M rtdata/languages/Hebrew
M rtdata/languages/Italian
M rtdata/languages/Japanese
M rtdata/languages/LICENSE
M rtdata/languages/Latvian
M rtdata/languages/Magyar
M rtdata/languages/Nederlands
M rtdata/languages/Norsk BM
M rtdata/languages/Polish
M rtdata/languages/Polish (Latin Characters)
M rtdata/languages/Portugues (Brasil)
M rtdata/languages/README
M rtdata/languages/Russian
M rtdata/languages/Serbian (Cyrilic Characters)
M rtdata/languages/Serbian (Latin Characters)
M rtdata/languages/Slovak
M rtdata/languages/Suomi
M rtdata/languages/Swedish
M rtdata/languages/Turkish
M rtdata/languages/default
M rtdata/profiles/BW-1.pp3
M rtdata/profiles/BW-2.pp3
M rtdata/profiles/BW-3.pp3
M rtdata/profiles/BW-4.pp3
M rtdata/profiles/Natural-1.pp3
M rtdata/profiles/Natural-2.pp3
M rtdata/profiles/Punchy-1.pp3
M rtdata/profiles/Punchy-2.pp3
M rtdata/profiles/Tuned-1.pp3
M rtdata/profiles/default-ISO-high.pp3
M rtdata/profiles/default-ISO-medium.pp3
M rtdata/profiles/default.pp3
M rtdata/profiles/neutral.pp3
M rtengine/EdgePreservingDecomposition.cc
M rtengine/EdgePreservingDecomposition.h
M rtengine/dcraw.h
M rtengine/dcraw.patch
M rtengine/dfmanager.cc
M rtengine/dfmanager.h
M rtengine/expo_before_b.cc
M rtengine/imageio.cc
M rtengine/ipvibrance.cc
M rtengine/klt/README.txt
M rtengine/klt/base.h
M rtengine/klt/klt.h
M rtengine/klt/main.cpp
M rtengine/klt/speed.txt
M rtengine/klt/trackFeatures.cc
M rtengine/rawimagesource.cc
M rtengine/safegtk.cc
M rtengine/safegtk.h
M rtengine/slicer.cc
M rtengine/slicer.h
M rtgui/batchqueuepanel.cc
M rtgui/batchtoolpanelcoord.cc
M rtgui/cachemanager.cc
M rtgui/colorprovider.h
M rtgui/curveeditor.h
M rtgui/curveeditorgroup.h
M rtgui/darkframe.cc
M rtgui/darkframe.h
M rtgui/diagonalcurveeditorsubgroup.cc
M rtgui/diagonalcurveeditorsubgroup.h
M rtgui/distortion.cc
M rtgui/distortion.h
M rtgui/editorpanel.cc
M rtgui/editwindow.cc
M rtgui/filecatalog.cc
M rtgui/filepanel.h
M rtgui/flatcurveeditorsubgroup.cc
M rtgui/flatcurveeditorsubgroup.h
M rtgui/flatfield.cc
M rtgui/flatfield.h
M rtgui/guiutils.cc
M rtgui/labcurve.cc
M rtgui/main.cc
M rtgui/mycurve.cc
M rtgui/myicon.rc
M rtgui/options.cc
M rtgui/paramsedited.cc
M rtgui/placesbrowser.cc
M rtgui/popupbutton.cc
M rtgui/popupbutton.h
M rtgui/popupcommon.cc
M rtgui/popupcommon.h
M rtgui/popuptogglebutton.cc
M rtgui/popuptogglebutton.h
M rtgui/ppversion.h
M rtgui/preferences.cc
M rtgui/preferences.h
M rtgui/preprocess.h
M rtgui/previewmodepanel.cc
M rtgui/previewmodepanel.h
M rtgui/previewwindow.cc
M rtgui/rawcacorrection.cc
M rtgui/rawcacorrection.h
M rtgui/rawexposure.cc
M rtgui/rawexposure.h
M rtgui/rawprocess.cc
M rtgui/rawprocess.h
M rtgui/resize.cc
M rtgui/rgbcurves.cc
M rtgui/rtimage.cc
M rtgui/rtimage.h
M rtgui/sharpenedge.cc
M rtgui/sharpenmicro.cc
M rtgui/soundman.cc
M rtgui/soundman.h
M rtgui/thumbbrowserbase.cc
M rtgui/thumbnail.cc
M rtgui/tonecurve.cc
M rtgui/toolpanel.cc
M rtgui/vibrance.cc
M rtgui/vibrance.h
M rtinstaller.nsi
M tools/RTProfileBuilderSample.cs
M tools/RTProfileBuilderSample.exe.config

Reported by entertheyoni on 2012-01-14 03:56:48

Beep6581 commented 9 years ago
I fully support this unification.
The next step is probably to work out some kind of a formatting style of the source
files, and use this style in every RT source file.

Reported by lebedev.ri on 2012-01-14 10:39:08

Beep6581 commented 9 years ago
Some coders don't care about formatting, so i think it's pretty useless to battle for
that, even if i would support it.

DrSlony, what problem occures with the actual different line endings? Your patch seems
to be HUUUUUGE, and will break all developper's work (i.e. they'll need to resynchronize
everything after that), so i really hope that it's really worth it. Before committing
it, you should alert developers about that and ask them to stick to one line endings...

Is the patch available?

Reported by natureh.510 on 2012-01-15 23:35:15

Beep6581 commented 9 years ago
I agree with Hombre here.  While I would like some consistency regarding line endings,
we should wait until a lull in coding activity.  For instance I am working (slowly)
on a major reworking of noise reduction, and such a change to line endings will make
it hugely complicated to apply the patch -- the merge will have to be analyzed line
by line, since my working directory will have different line endings from the repository
and no lines of changes in the patch will match to the changed repo.

On top of that, who will be the enforcer of line ending uniformity in the future, so
that we don't drift back to a hodgepodge of different conventions?

Reported by ejm.60657 on 2012-01-16 00:21:55

Beep6581 commented 9 years ago
Hombre: there is no patch available, it would just be a modification of line ending
code that "hg diff -c xxxx" doens't report, only "hg status" does.

Emil: seems to me that this is the lull :] Subjectively this is a very low activity
period.

Since mercurial doesn't treat line endings as part of the code (proof: "diff -c tip"
reports nothing), I don't think your patch will need to be changed at all. If it will,
it's a very simple change and I'm here to help you. Of course I don't want to cause
a line-for-line headache, and I don't think this will cause one (but, to be honest,
I have no empirical proof that it won't. Then again, it's so easy to rollback in case
it does. Or run dos2unix on your source file before committing.)

There will be no enforcer. Devs will continue to use all sorts of tools that handle
a mixture of line endings styles in different ways. I think it's a good thing to make
them consistent from time to time.

Reported by entertheyoni on 2012-01-16 01:20:31

Beep6581 commented 9 years ago
Not seeing commits doesn't mean that we're lazy DrSlony ;). I for one have 2 patch opened
that are a bit complicated, so it takes time, and i wouldn't have to loose some precious
time to synch everything by hand, like Emil said.

Could you make some test about that on your side? I think it would reassure us...

Reported by natureh.510 on 2012-01-16 01:43:59

Beep6581 commented 9 years ago
I know that! It's the reason I didn't include commit statistics in my statistics post.
I had three choices - either include statistics on the number of commits per dev, or
the number of commits taking into account the number of lines per commit, or nothing.
I think the best choice was nothing, as we have this unusual policy of getting patches
right before committing them via Google Code, and therefore one person commits the
cumulative work of a few people - no good for statistics.

On my side there were no issues at all. Again, I don't want to cause any extra work
for you. I don't think this consistency "patch" will cause that, and it is low priority
- happy to wait for a better time.

Reported by entertheyoni on 2012-01-16 01:55:56

Beep6581 commented 9 years ago
I think the appropriate test is not whether there are no issues when you apply your
EOL patch to the current code base, rather the test is whether when you consider an
outstanding patch for some open issue, whether the EOL patch causes the other patch
to fail when it is applied.

Reported by ejm.60657 on 2012-01-16 02:17:23

Beep6581 commented 9 years ago
I think this kind of change should not be committed to the default branch at least until
the merge of the XMP branch, as it would substantially over complicate the merge.

Reported by michaelezra000 on 2012-01-21 16:17:44

Beep6581 commented 9 years ago
Sure, low priority.

Reported by entertheyoni on 2012-01-21 19:54:38

Beep6581 commented 9 years ago
As michael suggests I add a block on the XMP issue.

Reported by rinni@gmx.net on 2012-06-20 21:00:24

Beep6581 commented 9 years ago
Let's wait for all major patches to be committed, which AFAIK are now XMP and denoising.

Reported by entertheyoni on 2012-06-20 23:12:32

Beep6581 commented 9 years ago
Mercurial is able to enforce a chosen kind of line ending style for each file. This
could cause headaches for devs with large uncommitted patches, so I'm putting this
info out here for some possible future time if ever, e.g. after the XMP merge.

You must read the whole file before doing anything: http://mercurial.selenic.com/wiki/EolExtension
The Mercurial folk stressed:
First thing is first: "This is considered a feature of last resort"

Should we choose to enable this extension, every person with commit access should enable
the "eol" extension by adding the following to .hgrc
[extensions]
eol =
[eol]
only-consistent = False

The .hgrc file should be in:
$HOME/.hgrc (Mac/Linux)
%USERPROFILE%\.hgrc (Windows)

Then we should create a .hgeol file in the root. Once this file is committed, the line
ending management takes effect. The .hgeol file should contain:
[patterns]
**.1 = LF
**.bash = LF
**.bat = CRLF
**.bmp = BIN
**.c = LF
**.cc = LF
**.cmake = LF
**.config = LF
**.cpp = LF
**.cs = CRLF
**.dcp = BIN
**.gtkrc = LF
**.h = LF
**.hgignore = native
**.hgtags = native
**.html = CRLF
**.icc = BIN
**.icm = BIN
**.icns = LF
**.ico = BIN
**.iconset = native
**.in = native
**.json = native
**.lin = LF
**.osx = LF
**.patch = LF
**.pdf = BIN
**.png = BIN
**.pp3 = LF
**.rc = native
**.README = LF
**.sh = LF
**.svg = BIN
**.theme = LF
**.txt = LF
**.wav = BIN
**.win = CRLF
**.xcf = BIN

Reported by entertheyoni on 2013-11-14 01:23:18

Beep6581 commented 9 years ago
If .txt has LF the LICENSE.txt is very bad to read for normal windows-users with notepad.exe.
Actually it has CRLF.

Reported by heckflosse@i-weyrich.de on 2013-11-14 17:55:14

Beep6581 commented 9 years ago

Reported by entertheyoni on 2014-03-24 21:16:52