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

DiffPlug supports git core.eol, but does not support core.autocrlf #540

Closed
jackwhelpton opened this issue Mar 15, 2020 · 10 comments · Fixed by #1114
Closed

DiffPlug supports git core.eol, but does not support core.autocrlf #540

jackwhelpton opened this issue Mar 15, 2020 · 10 comments · Fixed by #1114
Labels

Comments

@jackwhelpton
Copy link

jackwhelpton commented Mar 15, 2020

I'm developing on a Windows machine against a Unix repo, and have core.autocrlf=input set globally. My understanding based on #22 (comment) was that this was a supported scenario, without requiring further configuration.

Running spotless 3.18.0 on Gradle 5.0, spotless fails my build and appears to demand that I change line endings to \r\n.

My spotless config block is pretty simple:

spotless {
  java {
    importOrderFile 'build/packages.importorder'
    removeUnusedImports()
    eclipse().configFile 'build/PDSJavaCodeFormatter.xml'
  }
}

I've tried explicitly setting lineEndings: 'GIT_ATTRIBUTES' (no change in behavior), and lineEndings: 'UNIX' (this works, but I'm not able to update every repo I have to work with).

Am I doing something daft here?

@nedtwigg
Copy link
Member

We delegate to JGit for this, and the latest Spotless 3.27.2 uses JGit 5.5.0.201909110433-r. I know that JGit had a bug around core.autocrlf handling which was fixed at some point.

I haven't done the archaeology to lookup what version of JGit was in Spotless 3.18.0, but the first thing I would try is to update Spotless to latest and see if that fixes it. If it doesn't, then the next step is to see if there is any mention of core.autocrlf in the JGit changelogs from 5.5.0 until now.

@nedtwigg nedtwigg added the bug label Mar 16, 2020
@jackwhelpton
Copy link
Author

I've upgraded to 3.27.2 and see the same problem. As for JGit release notes, the only thing I turned up was:

5.6.0: Remove an old work-around for core.autocrlf = input

I don't know if that would help, to me it reads a bit like "autocrlf didn't used to work, now it does, so we no longer need this other code path".

@jackwhelpton
Copy link
Author

I did find a bug related to Git for Windows' use of an additional git config file, although that wasn't what I was using. I've gone through git config --list --show-origin and added core.autocrlf=input to every source that isn't repo-specific. Here's what I now have:

file:"C:\\ProgramData/Git/config"	core.autocrlf=input
file:C:/Program Files/Git/mingw64/etc/gitconfig	core.autocrlf=input
file:C:/Users/jack.whelpton/.gitconfig	core.autocrlf=input
file:.git/config	core.autocrlf=<not set>

Unfortunately the behavior is still the same.

@nedtwigg
Copy link
Member

Can you try one more thing?

buildscript {
  configurations.classpath {
    resolutionStrategy {
      force 'org.eclipse.jgit:org.eclipse.jgit:5.7.0.202003110725-r'
    }
  }
}

This will force your build to use the very latest git. If it still persists, then we'll have to file a bug with JGit.

@jackwhelpton
Copy link
Author

Same error, unfortunately: I definitely saw it pull down a fresh version of jgit as part of that run too.

@nedtwigg nedtwigg added enhancement and removed bug labels Mar 17, 2020
@nedtwigg nedtwigg changed the title EOL problems using core.autocrlf=input DiffPlug supports git core.eol, but does not support core.autocrlf Mar 17, 2020
@nedtwigg
Copy link
Member

nedtwigg commented Mar 17, 2020

After digging in a bit more, I think this is a bug/missing-feature in Spotless, not JGit. Reading from https://git-scm.com/docs/gitattributes

Note that setting core.autocrlf to true or input overrides core.eol

But it looks like we don't respect that anywhere:

this.defaultEnding = fromEol(config.getEnum(ConfigConstants.CONFIG_CORE_SECTION, null, ConfigConstants.CONFIG_KEY_EOL, EOL.NATIVE)).str();

String infoResult = findAttributeInRules(subpath, IS_FOLDER, KEY_EOL, infoRules);

The nice thing about core.eol is that it can take these three values, which are self-explanatory: lf, crlf, and native. The problem with core.autocrlf is that it can take these three values: true, input, and false, which are decidedly not self-explanatory.

It would be great to support core.autocrlf, it is definitely widely used. But it's not an easy fix, and there is an easy workaround, core.eol, which is imo a better-designed feature than autocrlf. Our documentation points people to the eol. Our PR you linked to falsely claims to fully support Git's line-ending behavior, but thanks to this issue it now loudly displays that autocrlf is not implemented.

I would love to merge a PR for this, but writing one will probably not make it to the top of my todo list.

@jackwhelpton
Copy link
Author

jackwhelpton commented Mar 17, 2020

That works perfectly, thanks. I'd skimmed that documentation and assumed it meant adding a .gitattributes to every repo, but as you say I can define core.eol in my Git config (I've done it at user level, as that seems to be the most clearly supported).

As autocrlf overrides this value, I'm actually even more confident that setting it won't break anything else.

It would be worth supporting autocrlf, I think, although with this workaround clearly documented it's less crucial. For example, the Git Extensions tool (pretty common in Windows environments) has configuration screens for line endings that write that setting behind the scenes.

@nedtwigg
Copy link
Member

nedtwigg commented Feb 1, 2022

Fixed in plugin-gradle 6.2.1 and plugin-maven 2.20.1, thanks to @rweisleder!

@arulrajnet
Copy link
Contributor

@nedtwigg This is broken in plugin-gradle >= 6.2.2.

Working in 6.2.1. I can reproduce this in my windows machine.

@nedtwigg nedtwigg added bug and removed enhancement labels Mar 29, 2022
@nedtwigg
Copy link
Member

nedtwigg commented Mar 29, 2022

Thanks for bug report @arulrajnet. Must have been broken by #1119. Happy to merge a PR for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants