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 :)
When ratcheting, check dirty files from Git first
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.
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.
- 点赞 评论 复制链接分享
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...点赞 评论 复制链接分享
Thanks for the comments! I'll have a look at both things when I have the time 🙂点赞 评论 复制链接分享
Still trying to figure out why too many files are returned.. so far I've noticed that the
FileTreeIteratorrepresenting 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>.
ls-files, I get the same SHA in the working tree as I get in the revision I'm ratcheting from.点赞 评论 复制链接分享
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 diffthough. Again, could be that latest JGit fixes it.点赞 评论 复制链接分享
git hash-object --no-filters <my-file>matches
git hash-object <my-file>which matches what the
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
ratchetFromin this case), and as
hash-objectalso returns the same for both branches it leads me to believe they are identical.
CanonicalTreeParsergets 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
GitRachetwould be closer to what the
DiffCommandactually does.点赞 评论 复制链接分享
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
DiffCommandwith a custom
FileTreeIterator, but I decided to go with the
IndexDiffinstead, 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.点赞 评论 复制链接分享
Your comments all make sense, I've implemented the changes!点赞 评论 复制链接分享
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-extraplugin 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 https://github.com/diffplug/spotless-changelog/issues/15 someday.点赞 评论 复制链接分享
Sorry for that, glad you got a compile error!点赞 评论 复制链接分享
My fault, not yours :) Released in
plugin-maven 2.4.2点赞 评论 复制链接分享