Fetch only the recent commits when cloning a remote Git repository for mirroring

[thachlp]

Motivation

From jgit 6.3.x we can limit the fetch graph by setDepth, it will help improve performance.

Modification

• Add new module server-jgit6 for jgit 6.x version.
• Abstract GitWithAuth for support GitWithAuth5 and GitWithAuth6.
• Add a new class GitWithAuth5 that implements GitWithAuth with jGit5.
• Add a new class GitWithAuth6 that implements GitWithAuth with jGit6.
• Adding an abstract method GitWithAuth.fetch(int depth) for adapting jgit update.

Result

• We have jgit 6.x with performance improvement running on jre 11 and jgit 5.x for jre 8.

[CLAassistant]

All committers have signed the CLA.

[trustin]

As you told me via chat, jgit requires Java 11+. Central Dogma builds everything with Java 8, and thus we get compilation errors.

To fix this, we need to update the Java version from 8 to 11, but we cannot update the client library's Java version just yet, because many users are still using Java 8.

What's tricky about the upgrade is that our users sometimes pull in Central Dogma server as a dependency for testing purposes. We can't force them to upgrade their tests to Java 11.

Therefore, I'd like to suggest the following workaround:

• Keep jGit version at 5.x
• Call setDepth() using reflection.
  • If setDepth() method is not available, log a debug message.
• Override the jGit version in the dependencies section of server/build.gradle only when rootProject.ext.testJavaVersion >= 11, e.g.

  dependencies {
    ...
    if (rootProject.ext.testJavaVersion >= 11) {
      testImplementation("....:jgit") {
        version {
            strictly "6.x.y"
        }
      }
    }
    ...
  }

• See if your JUnit test is run with jGit 6, not 5.

What do you think?

[trustin]

I heard from @minwoox et al that there might be a few more breaking changes between jGit 5 and 6. Those changes are basically package renaming. Please let me know if you ever encounter such compilation or runtime error. We'll need to use reflection again there as well.

[minwoox]

There were two classes when I was creating the PR: https://github.com/line/centraldogma/pull/702/files#diff-858d35514fceb503e11dca2402c0c61a0d21b4f3481d6955ba5af10391b069e7R44

We probably need two separate classes (e.g. GitWithAuthJGit5, GitWithAuthJGit6) and call them depending on the jGit version.

[thachlp]

I heard from @minwoox et al that there might be a few more breaking changes between jGit 5 and 6. Those changes are basically package renaming. Please let me know if you ever encounter such compilation or runtime error. We'll need to use reflection again there as well.

Thanks @minwoox, yes @trustin, there are 2 classes I have updated in this pr too, I am dealing with it.

[trustin]

1. Could you also try running GitWithAuthTest with the system properties explained here? https://github.com/trustin/centraldogma/blob/e93bc5a04eb3d38a0f315d9c5fdb0db3b0af1f52/it/src/test/java/com/linecorp/centraldogma/it/mirror/git/GitMirrorAuthTest.java#L65-L78 I already gave you the read permission to the test repository, so it should just work.
2. Please add the test case that makes sure setDepth works as expected.

[codecov]

Codecov Report

Patch coverage: 31.64% and project coverage change: -0.16 ⚠️

Comparison is base (6f13d12) 65.66% compared to head (25414f7) 65.50%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #808      +/-   ##
============================================
- Coverage     65.66%   65.50%   -0.16%     
- Complexity     3292     3293       +1     
============================================
  Files           351      353       +2     
  Lines         13751    13796      +45     
  Branches       1491     1492       +1     
============================================
+ Hits           9029     9037       +8     
- Misses         3876     3909      +33     
- Partials        846      850       +4     

Impacted Files	Coverage Δ
...traldogma/server/internal/mirror/Git5WithAuth.java	0.00% <0.00%> (ø)
...traldogma/server/internal/mirror/Git6WithAuth.java	43.75% <43.75%> (ø)
...centraldogma/server/internal/mirror/GitMirror.java	80.58% <71.42%> (-0.69%)	⬇️
...ntraldogma/server/internal/mirror/GitWithAuth.java	63.63% <100.00%> (+6.73%)	⬆️
...ient/armeria/legacy/LegacyCentralDogmaBuilder.java	81.48% <0.00%> (-3.71%)	⬇️
...centraldogma/server/internal/api/WatchService.java	76.27% <0.00%> (-3.39%)	⬇️
.../linecorp/centraldogma/client/AbstractWatcher.java	78.66% <0.00%> (+1.33%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[trustin]

So far so good! Could you:

• Update dist/build.gradle so we use jGit6 in our tarball distribution?
  • Add evaluationDependsOn(':server-jgit6')
  • Add the following to task copyLib:

    from project(':server-jgit6').configurations.runtimeClasspath
    from project(':server-jgit6').tasks.jar
    

• Remove the draft status from this pull request; and
• Update the pull request subject and content?

[trustin]

Oh and please make sure the distribution directory (dist/build/dist/lib) populated by ./gradlew :dist:distDir contains the jgit 6 JAR file.

[thachlp]

please make sure the distribution directory (dist/build/dist/lib) populated by ./gradlew :dist:distDir contains the jgit 6 JAR file.

OK, how can I do this?

[ikhoon]

Should we set the Java version java11 flag instead which will automatically set the compile target version https://github.com/line/gradle-scripts#setting-a-target-version-with-the-javad-flag

[ikhoon]

You need to update master branch first and refer to the following link as an example.
https://github.com/line/armeria/blob/ab7488bafc2912445f026fb6d02420f134937073/settings.gradle#L28

[thachlp]

Hello, what do you mean about update master branch first, you mean create another pull request? I guess I need to update on /setting.gradle in this branch?

[trustin]

It looks like we can't use java11 flag in this case because other non-java11 modules optionally depends on it, e.g. :it and :dist.

[trustin]

I guess he meant doing git merge master in this PR. I did it for you 😉

[ikhoon]

It is likely we can fix buildJdkVersion to 17 and run tests with 8, 11, 17 to take advantage of javaN flag.

./gradlew test -PbuildJdkVersion=17 -PtestJavaVersion=8

Let me update actions_build.yml and gradle scripts and push them to this PR.

[thachlp]

Great! Thank you

[ikhoon]

@thachlp Could you resolve the conflicts?

[thachlp]

@thachlp Could you resolve the conflicts?

sure

[trustin]

👍

[ikhoon]

Thanks, @thachlp! 🚀🥳

[jrhee17]

Looks good, left some minor comments/questions

[jrhee17]

Verified the flaky tests CI job passes in my local. Thanks @thachlp ! 🙇 👍 🚀