PR 2730, "Fix" line-endings

Post here if you have re-based and finalised code to integrate into master, which was discussed, agreed to and tested in other forums. You can also submit your PR directly on github.
User avatar
bernd
Posts: 9045
Joined: Sun Sep 08, 2013 8:07 pm
Location: Zürich, Switzerland

Re: PR 2730, "Fix" line-endings

Postby bernd » Tue Dec 03, 2019 11:38 am

This is on my TODO for years ... We need to do this step by step because a rebase does not work after a huge line endings change. As an example ATM I run around 10 automatic rebases to have these changes (which are not finished yet) still on top of master.

But lets get started ...

In FEM there are only some C++ files from VTK post processing which still uses windows line endings. I would acceppt a change in FEM to unix line endings for all code files. What we need to take care of is some git preference which automaticly converts any windows lineending automaticly in unix line ending in any later commit. But only for FEM at the moment. There is such pref in git but I do not know about it.

cheers bernd
ezzieyguywuf
Posts: 637
Joined: Tue May 19, 2015 1:11 am

Re: PR 2730, "Fix" line-endings

Postby ezzieyguywuf » Tue Dec 03, 2019 2:31 pm

bernd wrote:
Tue Dec 03, 2019 11:38 am
But lets get started ...
Thanks for your support bernd!

I'll put together a PR specifically for FEM, and include the necessary .gitattributes info to ensure that anyone using our git repo in the future will have the appropriate git settings to prevent future issues.
ezzieyguywuf
Posts: 637
Joined: Tue May 19, 2015 1:11 am

Re: PR 2730, "Fix" line-endings

Postby ezzieyguywuf » Tue Dec 03, 2019 9:25 pm

PR2751 attempts to fix the line-endings in all FEM files while maintaining the existing git-history.

What do I mean by this?

In other words, let's say that src/Mod/Fem/App/FemConstraints.cpp currently has CRLF line-endings.

I can easily fix the line endings:

Code: Select all

dos2unix src/Mod/Fem/App/FemConstraints.cpp
git commit -a 'Fixed line endings in FemConstraints.cpp'
However, now, if we do:

Code: Select all

git blame src/Mod/Fem/App/FemConstraints.cpp
It looks like I wrote the entire file - in other words, we can't see the history of each line using "git blame".

"git log" will still show the relevant history.

Instead, what I've done is:

Code: Select all

git filter-branch --tree-filter "if [ -f src/Mod/Fem/App/FemConstraints.cpp ]; then; dos2unix src/Mod/Fem/App/FemConstraints.cpp; fi" -- --all
This steps through every single commit in the repos history (it took over an hour!) and, if the given file exists, rewrites it with the appropriate line endings, preserving the commit message at the time.

Now, we can do "git blame" and get the expected results.

However, what's the tradeoff?

All of history was rewritten! This means, for starters, that this PR cannot simply be merged. Rather, it must be force-pushed to the github repository.

Further, it means that everyones local copy of the git repository will be invalid.

In other words - everyone will need to re-clone the new force-pushed repository, and then manually copy over files relevant to whatever local branches they are working on. They'll likely lose their git history on these local branches, because I cannot think of a way of copying over files+history from one git repo to another.

Therefore: is the tradeoff worth it? If so, do we really wish to do this multiple times, one small chunk of the code base at a time? Or should we rip off the bandaid and fix the line endings in the whole code base?

It seems like if we do it this way, we gain nothing by limiting the scope to just the Fem workbench, since everything gets re-written anyway.
User avatar
bernd
Posts: 9045
Joined: Sun Sep 08, 2013 8:07 pm
Location: Zürich, Switzerland

Re: PR 2730, "Fix" line-endings

Postby bernd » Tue Dec 03, 2019 9:46 pm

I do not know the opinion of the other developer, but I am against rewriting the history of the repo. IMHO rewriting the history should not happen on a master branch of a open project like FreeCAD, never ever.

Neverless I rewrite history for my own development all the time before merging into FreeCAD master, but never ever the FreeCAD master is rewritten. BTW the FreeCAD master is set to not accept force pushes.

What I would accept is a commit which just changes the line endings of Fem. Sure for the repo it looks like the files has changed but AFAIK this is the only possibility without rewriting the history.

cheers bernd
ezzieyguywuf
Posts: 637
Joined: Tue May 19, 2015 1:11 am

Re: PR 2730, "Fix" line-endings

Postby ezzieyguywuf » Tue Dec 03, 2019 9:48 pm

bernd wrote:
Tue Dec 03, 2019 9:46 pm
IMHO rewriting the history should not happen on a master branch of a open project like FreeCAD, never ever.
+1 I agree with this.
bernd wrote:
Tue Dec 03, 2019 9:46 pm
BTW the FreeCAD master is set to not accept force pushes.
I did not know that.
bernd wrote:
Tue Dec 03, 2019 9:46 pm
What I would accept is a commit which just changes the line endings of Fem. Sure for the repo it looks like the files has changed but AFAIK this is the only possibility without rewriting the history.
Yes, I believe you are right, this is the only option.

I can easily submit a PR that changes just the line endings in FEM, and will do so, however I wanted to present the other alternative as well as I believe some folks mentioned in this thread a concern regarding the "git blame" functionality.

update:

PR 2752 submitted. strike that, I accidentally didn't base this PR off of master.
User avatar
bernd
Posts: 9045
Joined: Sun Sep 08, 2013 8:07 pm
Location: Zürich, Switzerland

Re: PR 2730, "Fix" line-endings

Postby bernd » Tue Dec 03, 2019 10:13 pm

ezzieyguywuf wrote:
Sun Nov 24, 2019 6:30 pm
... or if we include a .gitattribute file), then git will convert to windows style line ending upon checkout, and back to Unix-style when committing.
This is what we need to do for sure for Fem if line endings arch changed to unix-style. With this we will never ever get a windows line ending into Fem source code in the future. Which means any further line ending problem will be gone for Fem regardless what the people use for git preferences or editors they use or what line ending the code has they make a PR with. Any Fem code will just automaticly converted to unix-style on commiting to FreeCAD master.

Some years back I did change all Fem code files to unix-style already. git commit 14eb6869a9 But I did not know how to guard them and thus some new files came into Fem with windwos line endings.
ezzieyguywuf
Posts: 637
Joined: Tue May 19, 2015 1:11 am

Re: PR 2730, "Fix" line-endings

Postby ezzieyguywuf » Tue Dec 03, 2019 10:20 pm

PR 2752 has been fixed, it is rebased against the latest master.
bernd wrote:
Tue Dec 03, 2019 10:13 pm
This is what we need to do for sure for Fem if line endings arch changed to unix-style.
I've taken care of this, see this first commit.

I'm only handling .h, .cpp, and .csv files, because those are the only files that were changed by the dos2unix utility.

It may be prudent to add some other text-based line-endings to this list if they are used in the Fem workbench...
User avatar
bernd
Posts: 9045
Joined: Sun Sep 08, 2013 8:07 pm
Location: Zürich, Switzerland

Re: PR 2730, "Fix" line-endings

Postby bernd » Tue Dec 03, 2019 10:28 pm

ezzieyguywuf wrote:
Tue Dec 03, 2019 10:20 pm

It may be prudent to add some other text-based line-endings to this list if they are used in the Fem workbench...
see my comment. We should for sure guard them too. ATM I am guarding them manually, but it seams git could do a better job on this than me.
ezzieyguywuf
Posts: 637
Joined: Tue May 19, 2015 1:11 am

Re: PR 2730, "Fix" line-endings

Postby ezzieyguywuf » Wed Dec 04, 2019 2:52 am

bernd wrote:
Tue Dec 03, 2019 10:28 pm
see my comment
Done.
wmayer
Site Admin
Posts: 15297
Joined: Thu Feb 19, 2009 10:32 am

Re: PR 2730, "Fix" line-endings

Postby wmayer » Wed Dec 04, 2019 9:36 am

It looks like I wrote the entire file - in other words, we can't see the history of each line using "git blame".
git blame has the option -w:
git wrote: Ignore whitespace when comparing the parent’s version and the child’s to find where the lines came from.
I tested this with a local file that I converted to UNIX-style and without -w all lines where commented to be touched today. When using -w then the output was the same as before using dos2unix.
I do not know the opinion of the other developer, but I am against rewriting the history of the repo. IMHO rewriting the history should not happen on a master branch of a open project like FreeCAD, never ever.
+1