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

CB-9739 Prevent coho from creating broken packages including CRLF on … #95

Merged
merged 1 commit into from Oct 6, 2015
Merged

CB-9739 Prevent coho from creating broken packages including CRLF on … #95

merged 1 commit into from Oct 6, 2015

Conversation

daserge
Copy link
Contributor

@daserge daserge commented Oct 2, 2015

Windows

We may also need to add .gitattributes with

* text eol=lf

to force LF line endings in the repos.

Jira issue

@@ -76,6 +77,49 @@ exports.createCommand = function*(argv) {
var absOutDir = path.resolve(outDir);

yield repoutil.forEachRepo(repos, function*(repo) {
if(isWin) {
Copy link
Member

Choose a reason for hiding this comment

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

Why the windows check?
Can a mac or linux users have a bad git config? and then the check will not run for them.

Copy link
Contributor

Choose a reason for hiding this comment

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

We only have evidence of a problem with committers using Windows machine. In theory, this could happen on a Linux/mac machine - but the defaults ensure that this does not happen. Unless this is trivial - I would like us to move forward with getting this fix in for Windows.

Copy link
Member

Choose a reason for hiding this comment

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

@nikhilkh I'm Ok with the change I was just curious of why the isWin, that's all.
I was not trying to blocked the merge. Merge away..
+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@csantanapr, git does not seem to change eols to CRLF on OS X given that core.autocrlf=false and/or core.eol=crlf. It probably could be changed by .gitattributes only (text eol=crlf).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification @daserge
On Mac by default using git from XCode Tools, both of the settings core.autocrlf and core.eol are not set at all, so I would think let's leave this check only for Win as you have it here, go ahead and merge this PR

@TimBarham
Copy link
Contributor

+1 to * text eol=lf in .gitattributes - most commonly used tools on Windows (other than notepad 😄) should handle Unix line endings fine.

@asfgit asfgit merged commit 2fd896e into apache:master Oct 6, 2015
@daserge daserge deleted the CB-9739 branch October 6, 2015 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants