SNAFUed linefeeds between 2.3 and master

[techtonik]

I tried to transplant some changes from master to branch and got nasty linefeed conflict through the whole file. I guess that bd13f67058fa4e247 is the culprit. Carlos, can you clarify what's going on?

In particular:

1. what you was trying to fix?
2. why the file ended up with mixed linefeeds?
3. and why did you choose to have different linefeeds for master and for branch?
4. and why did you choose CRLF linefeeds for master?

I am pretty concerned about that, because the history is messed up, it spoils the blame and gives me a headaches with version control system that's already complicated enough.

The perfectionist in me prefers that we rewrite/rebase the history to use unix linefeeds while importing to git.

Ping @ccordoba12.

[goanpeca]

I think we have 2 options:

• Make the git global config use a default independent on what spyder does on different OS (devs would need to ensure this setup..)
• Have an option to force spyder use only a fixed line ending version id DEV mode (assuming people are developing spyder using spyder)

It is true that something changed at some point recently...

[techtonik]

Or support https://github.com/spyder-ide/spyder/issues/2336, require that Spyder files are all LF and add a hook script that checks this.

[ccordoba12]

I'm sorry for the troubles @techtonik. I don't know exactly what happened there, I just remember that when I opened that file in Spyder (after the previous changes of @SylvainCorlay), all line endings were fixed by Spyder itself.

Given that I develop Spyder with Spyder, I thought I didn't have other alternative than re-committing the whole file again.

Sorry, I didn't give too much thought about it because the same thing had happened a few other times before.

---

@goanpeca and @techtonik, I think at this point we can't move to support only one line-ending type. Else we'll have to recommit all files again (like the case cited by @techtonik), and that will be a terrible headache for all our current contributors.

So the only thing to do is to instruct git to follow us :-)

[techtonik]

I am concerned about the merges from stable 2.3 to master https://github.com/spyder-ide/spyder/wiki/Dev:-Github-Workflow doesn't describe maintenance of stable branches and sync with new development at all. What is the merge strategy?

[ccordoba12]

There are not going to be more releases of the stable (2.3) branch, but the strategy is this:

1. While on maintenance, I develop on the stable branch and merge that branch with master every certain number of commits (between 5 and 10).
2. People is encouraged to make PRs against master and not to the stable branch. If I consider new code is not going to make Spyder unstable, or it's about an important bug fix, I backport it to the stable branch (either manually or by merging master with stable).
3. Translations are committed to the stable branch

I think that's all :-)

[techtonik]

I'd vote for sticking to LF and adding editorconfig instead of messing with Git.

What is the current merge process? Do you merge features from stable to master? Does it work ok for you? If not, then I'd again, think about removing those line changing commits from history. Considering that we have 60 forks, I think it should be accompanied by a blog post that clarifies why people should care about line endings, what happens:

1. stop point or bump on the blame history road that needs vauluable time to pass through
2. screwed merges between branches

These two are significant roadblocks, so considering that Spyder is still young on GitHub, I'll still vote for purging the history for linefeed changes (archiving current repo for those who have forks) and also move all big files out of it to https://github.com/blog/1986-announcing-git-large-file-storage-lfs

[goanpeca]

@ccordoba12 I do not know if need to go as far as @techtonik suggests, but it is a concern that needs to be solved ASAP.

I think it would be best to make spyder used a single line feed for development (DEV) and people should be (strongly) encouraged to develop using spyder itself (The latest dev version...). Even if it is a minor annoyance for contributors, they can always rebase to master.

I think this is important

[ccordoba12]

My thoughts:

I agree with encouraging to use only one type of line endings and making that default for DEV. (@goanpeca, could you make a PR for that?). However:

1. We can't force people to develop Spyder with Spyder. For example, @SylvainCorlay uses Vim :-)
2. Github doesn't report if line-endings have changed in PRs, so that would make much harder to check that. And making people to rebase their PRs just for line-endings seems not good policy to me.
3. The case reported by @techtonik happens like one or twice a year, so I don't think we should overreact about it.
4. I don't know what line-endings use GH for their online editor (with which users can create a PR in a matter of minutes). So that's another uncontrollable factor.
5. I don't see much bigger projects, like IPython, using or recommending a unique line-ending (see here), so I don't see why we should worry too much about that.

I don't agree with re-commiting all files with a different and unique line-ending, and worst, with creating a new repo just for that. That simply is not going to happen :-).

[goanpeca]

:+1:

[goanpeca]

Will start the PR soon, @SylvainCorlay uses linux most of the time, so that should be fine :-)

[goanpeca]

@ccordoba12 would not it be easier if we just add this .gitattributes to our repo? It would make git always use a consistent line end per repo and users would not have to change anything.

https://help.github.com/articles/dealing-with-line-endings/#per-repository-settings

[goanpeca]

• http://stackoverflow.com/questions/170961/whats-the-best-crlf-carriage-return-line-feed-handling-strategy-with-git

[goanpeca]

@ccordoba12 What would be the problem with renormalizing the repo with a fixed line ending?

[Nodd]

.gitattributes looks good, but it will require to recommit all files.

[goanpeca]

It will be once... and it will solve once and for all inconsistencies...

[Nodd]

Some data (You have to use a shell that support recursive globbing or enable it in bash [it has side-effects]):

$ awk -v RS='\r\n' 'END{print NR}' **.py
49590
$ awk -v RS='\n' 'END{print NR}' **.py
99609

So about half of the lines are have windows style endings, and the other half have linux style endings.

Now by files (remove the grep to see file names):

find . -not -type d -name "*.py" -exec file "{}" ";" | grep CRLF
116
joseph@wdemr314z /d/p/spyder.git> find . -not -type d -name "*.py" -exec file "{}" ";" | wc -l
318

That's a lot of files to change ! If we do such a big change, we should also remove trailing whitespace everywhere. But is it woth it ?

Is it possible to add checks in travis to enforce style rules like this ? We could also add line length checks.

[goanpeca]

It is all relative, but is better now than later (or never...).

I would say, yes it is worth it:

Yes we also should remove trailing whitespace, yes we should make all of spyder code pep8 and yes we should change the line ending for good.

---

Travis suggestions seems like a good idea to make sure things are consistent, and sure, we could add line length check, no biggie there

[Nodd]

We can not be fully compliant to pep8 because of the names in Qt, but we could follow most of it.

[goanpeca]

Yes my dear @Nodd... excluding that...

[goanpeca]

I have an entry on the wiki for this

https://github.com/spyder-ide/spyder/wiki/Dev%3A-Coding-Style

[ccordoba12]

This seems a bit excessive to me because it'll make impossible to merge the work of the (62) people that have forks at this moment. And as I said before, it will only cover very sporadic cases (like the one mentioned by @techtonik).

I'd like to call for a vote here from all @spyder-ide/core-developers to make a final decision about it.

My vote is -1 on this change.

[bilderbuchi]

just in case you want a data point from an outsider, I am part of a team with a big repo with developers of many OSes (and line styles), and we previously ran into the same problem - imo, .gitattributes is the way to fix this, as already proposed above.

[bilderbuchi]

also, @ccordoba12, for the issue of spoiling open PRs, the renormalize option for git merge helps with that.

[ccordoba12]

Well, let's take a decision: @goanpeca, @Nodd, @SylvainCorlay and @blink1073, what do you think about this issue? Should we move to use .gitattributes?

[goanpeca]

I would do it, even if it is messy now, it will save us from trouble in the future... when it might become even messier

[goanpeca]

:+1: +1

[Nodd]

After some thoughts about it I'm :+1: on the change, if we normalize many things at the same time (and not line endings only). This could include:

• line endings
• trailing spaces
• line length < 79 (I think we don't have anything to change here actually)
• add all above as pep8 checks to travis
• use .gitattributes

To facilitate the work, we can use tools like autopep8 and/or yapf. yapf is quite young (yet already well known) but autopep8 is stable, well tested and by default enables only safe checks.

To ease the transition, the amount of opened PRs should be at a minimum, but that's rather obvious.

[goanpeca]

@SylvainCorlay and @blink1073?

[blink1073]

I agree with all of @Nodd's suggestions.

[SylvainCorlay]

I agree with normalizing things with the .gitattributes. Although, I would not go as far as @Nodd on forcibly coercing line length. I would rather keep it as a loose constraint. I think that there are legitimate cases where having very long lines is the most elegant way.

[Nodd]

We could enforce that checks should be specifically disables for long line, to show that it's intentional. But I don't know how to to that other than by adding a comment at the end of the line, which would make it even longer... Anyway this point can be decided (and added) later, it is a detail.

[SylvainCorlay]

@nodd, agreed

Is it something we could do after the new icon theme gets in? My branch here has over 60 modified files. Rebasing after something like this is going to be very very long.

[techtonik]

I am against doing PEP8 normalization all at once. The merge tools can ignore linefeed changes, but more changes can break the merges again.

Also, I haven't seen the answer on merge strategy and how are you merging stuff. Is it from stable to master? How do you handle linefeed differences ATM? I am only concerned about the merges ATM.

[Nodd]

@ccordoba12 already explained the merge strategy before in this thread (https://github.com/spyder-ide/spyder/issues/2335#issuecomment-94197832). If you want more precisions you'll have to ask them more specifically.

About pep8 I agree that we shouldn't normalize everything everywhere at once, but currently in each PR the pep8 errors are left, so things will never improve this way. For example a new startegy would be that when a file is comitted, all its trailing whitespaces should be removed, this way things will improve little by little. I know it's harder to spot the changes on review, but it could be a separate commit to help keep things clean.

[goanpeca]

So, we all agreed on this right @spyder-ide/core-developers?, now... the more time we let pass by... the more forks will be made and the harder (annoying..) it will become to make changes.

On my side I will finish with the corrections on my active PRs and stop making new ones, until this situation is solved.

Can we all agree on this?

[blink1073]

Sounds good to me.

[Nodd]

I agree for waiting before creating more PRs (except if they are very simple), but what about pep8 corrections ?

[goanpeca]

I think we should do the gitattributes thing first, then after that is done, remove trailing spaces and empty lines and then we can start fixing pep8 little by little...

[techtonik]

There are reports that automatic linefeeds can corrupt binary files if detects them incorrectly. http://stackoverflow.com/questions/170961/whats-the-best-crlf-carriage-return-line-feed-handling-strategy-with-git I tend to agree that it is not a job of version control system to mess with the content. I'd prefer that Spyder repository just had a hook script so that people were aware of linefeed problem existence. It also a good test for Spyder itself.

[Nodd]

Yes it can, but we're using only very common binary files that are recognized without any problem by git (.png, .mo, ...). We can also activate this for .py, .rst and .txt files only to be on the safe side.

[techtonik]

This brings additional complexity to the issue. I'd better use http://editorconfig.org/ to control new files and convert old ones manually, because it is more explicit and less magical. Also, add a Travis to detect linefeeds problem, which may be a problem with Spyder itself.

Also, the link above says that Eclipse and alternative Git implementations (hg-git / dulwich) may not support this feature, so if we're to integrate Python plugin for Git into Spyder, the problem will manifest itself again. Somebody will commit the CRLF file and then somebody else will get the conflict. Travis is the best as for me.

[Nodd]

We can also add a Travis test in case someone uses an alternative git implementation to be on the safe side. But for those who use vanilla git, line edings are manages automatically and they don't have to worry about it at all, while Travis just warns you afterwards. Why should we do something manually if it can be automated ? Also a .gitattribute file is quite explicit.

[goanpeca]

I think the decision on using gitattributes is already made, it will offer way more advantages than disadvantages, it is explicit and will care for most of Spyder devs/maintainers needs.

The travis check can be added as a safety net for those corner cases.

I prefer to have a couple of disgruntled spyder users because something does not work in their specific setup, that having to tell the users/casual-devs to enforce some configuration that could have been transparently (and automatically) handled on their behalf.

---

On the spyder plugin for git.... when we get to that stage, then that plugin should make sure this is not a problem, and if it is, it should then solve it.

[techtonik]

But for those who use vanilla git, line edings are manages automatically and they don't have to worry about it at all, while Travis just warns you afterwards. Why should we do something manually if it can be automated ?

Because you're writing editor, and the line ending should be controlled be editor, and not by your version control system.

I think the decision on using gitattributes is already made, it will offer way more advantages than disadvantages, it is explicit and will care for most of Spyder devs/maintainers needs.

Well, I am not a developer anymore, but I disagree with you. It is implicit, the advantages/disadvantages are not completely clear, and in context of editor development, the editorconfig alternative is not even addressed.

[techtonik]

Anyway, I probably don't need to be so interested in long-term maintenance. =) I am just interested that you fix the repository so that merge of https://github.com/spyder-ide/spyder/pull/2400 from 2.3 to master is possible.

[goanpeca]

gitattributes will only affect the Spyder repo... When Spyder supports git via a plugin, each repo and each user will decide what they wish to do with their personal/public repos and git.

This change will make spyder development and maintenance easier in the long run cause it will remove both a burden on the developer and the maintainer.

[Nodd]

For reference:

• Mercurial has the same problem and the same kind of fix (via an extension): http://mercurial.selenic.com/wiki/FAQ#FAQ.2FTechnicalDetails.What_about_Windows_line_endings_vs._Unix_line_endings.3F
• SVN has svn:eol-style: http://svnbook.red-bean.com/en/1.7/svn.advanced.props.file-portability.html

Supporting EditorConfig would be nice, but this is a separate issue. Not all spyder developer and contributor use editors or IDEs supporting this extension, while all of them have to use git.

@techtonik Could you create a PR which adds a Travis check for line endings ?

[techtonik]

@Nodd let me see..

[techtonik]

Instead of messing with linefeeds in 2.3, it is better to #2469