When ratcheting, check dirty files from Git first

[eriklumme]

Regarding #701

Spent a lot of time getting the thing set up. IntelliJ refused to cooperate, Gradle isn't my strong suit, and debugging Maven when run through Gradle had its challenges too...

At first it wouldn't run the Maven plugin build at all, but I realized it was because I was using Java 11.

The SpecificFilesTest fail because I'm on Windows, they can probably be fixed by adding enough pattern.replace("/", "\\\\\\\\") statements.

There is still an issue with this code that I haven't been able to figure out. The diff picks up a lot more files than I have actually changed (~400 instead of 3). I believe it's related to line ending normalization, if I run git add --renormalize . it changes a lot of files, probably the same files that the diff picks up.

But if I run git diff manually against the same merge base, or if I run the old plugin version, they don't have this problem.

So that still needs to be fixed, if you have any idea do tell. Even so, it's about three times faster for me than before.

[nedtwigg]

Thanks, looks like a great PR. I agree the maven/gradle interface is tricky, #554 could improve that. I am swamped for the next several days, but I will come back and help make sure this gets merged sometime next week, unless someone else on the team beats me to it :)

[nedtwigg]

.setPathFilter would speed this up a bunch for multimodule projects

[nedtwigg]

Somewhere or other, we set a jgit version. The newest version of JGit includes support for a new index format which cgit has had for a while. So updating that might fix some things...

[eriklumme]

Thanks for the comments! I'll have a look at both things when I have the time 🙂

[eriklumme]

Still trying to figure out why too many files are returned.. so far I've noticed that the FileTreeIterator representing the current working tree returns the "wrong" object ID for some files.

I put wrong in quotation marks because I can get the same SHA by running
git hash-object <my-file>, which is different from git ls-files -s <my-file>.

Using ls-files, I get the same SHA in the working tree as I get in the revision I'm ratcheting from.

[nedtwigg]

For the files that disagree, does git hash-object --no-filters <my-file> match what the FileTreeIterator is getting? Could be line-endings clean/smudge. That would indicate that JGit is not respecting clean/smudge. Not sure why it would have this problem only with git diff though. Again, could be that latest JGit fixes it.

[eriklumme]

Yes, git hash-object --no-filters <my-file> matches git hash-object <my-file> which matches what the FileTreeIterator is getting.

I've tried setting up JGit in another project too, and eventually ran into the same issues. The line endings are the same in both branches (using another branch for the ratchetFrom in this case), and as hash-object also returns the same for both branches it leads me to believe they are identical.

But the CanonicalTreeParser gets the same hash as I get from git ls-files -s <my-file>, which doesn't match, although it is also the same for both branches.

The old code works correctly, but maybe that's because workingTreeIterator.isModified(...) works differently. I think the worktreeIsCleanCheckout(treeWalk) of the GitRachet would be closer to what the DiffCommand actually does.

[eriklumme]

So.. this was a good time for me to dive deeper into Git internals.

It does indeed seem like it was a smudge/clean problem. Maybe it could've been done with the DiffCommand with a custom FileTreeIterator, but I decided to go with the IndexDiff instead, as it seems to more closely match what we're after.

That was quite straight-forward, but then there were the special cases of e.g. a file that was modified in the index and in the working tree, but where the working tree matched the local repository. Luckily you had test cases for this (and a couple other ones), please take a look to see if you think they're handled appropriately.

It's still a lot faster for me than before, with 1-10 modified files the Maven execution took 3.5-4.8 seconds, when previously it was around 15 seconds.

[nedtwigg]

Wow, great PR. Thanks for digging in, sorry for slow feedback. I have just one teensy change I'd like to request. Once this last bit gets fixed, I'll merge and release.

[eriklumme]

Your comments all make sense, I've implemented the changes!

[nedtwigg]

Oops, I made a mistake. Easy for me to fix, just commenting here to document:

• I tried to deploy, but (luckily!) got a compile error
• Because this PR changes the lib-extra plugin without adding any entries the lib changelog
• So I didn't deploy a new version of lib, and the attempted deploy tried to link against our last release lib, which doesn't have the APIs that this PR created/needs

Super easy for me to fix, but I should not have merged this before asking for an entry in the root CHANGES.md. We should probably fix this by implementing diffplug/spotless-changelog#15 someday.

[eriklumme]

Sorry for that, glad you got a compile error!

[nedtwigg]

My fault, not yours :) Released in plugin-maven 2.4.2