weixin_39614675
weixin_39614675
2020-11-29 14:06

When ratcheting, check dirty files from Git first

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.

该提问来源于开源项目:diffplug/spotless

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享
  • 邀请回答

11条回答

  • weixin_39968760 weixin_39968760 5月前

    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 :)

    点赞 评论 复制链接分享
  • weixin_39968760 weixin_39968760 5月前

    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...

    点赞 评论 复制链接分享
  • weixin_39614675 weixin_39614675 5月前

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

    点赞 评论 复制链接分享
  • weixin_39614675 weixin_39614675 5月前

    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.

    点赞 评论 复制链接分享
  • weixin_39968760 weixin_39968760 5月前

    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.

    点赞 评论 复制链接分享
  • weixin_39614675 weixin_39614675 5月前

    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.

    点赞 评论 复制链接分享
  • weixin_39614675 weixin_39614675 5月前

    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.

    点赞 评论 复制链接分享
  • weixin_39614675 weixin_39614675 5月前

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

    点赞 评论 复制链接分享
  • weixin_39968760 weixin_39968760 5月前

    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 https://github.com/diffplug/spotless-changelog/issues/15 someday.

    点赞 评论 复制链接分享
  • weixin_39614675 weixin_39614675 5月前

    Sorry for that, glad you got a compile error!

    点赞 评论 复制链接分享
  • weixin_39968760 weixin_39968760 5月前

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

    点赞 评论 复制链接分享

相关推荐