RFR: 1690: Make jcheck cli able to use conf from workspace or other commit [v3]

[Erik Joelsson]

On Wed, 30 Nov 2022 19:34:24 GMT, Zhao Song <zsong at openjdk.org> wrote:

>> Currently, the command `git skara jcheck` can only use the `.jcheck/conf` configuration file in the current checking commit.
>> However, sometimes users want to run jcheck with different configurations to validate their commits. Therefore, we need to upgrade `git skara jcheck`.
>> 
>> In this patch, `git skara jcheck` would be able to support following usecases.
>> 
>> 1. Run jcheck on a commit or a series of commits using the .jcheck/conf in the same commit. (what we do by default today)
>> `git skara jcheck`
>> 
>> 2. Run jcheck on a commit or a series of commits using the .jcheck/conf in a different specified commit. 
>> `git skara jcheck --specified-conf-commit <COMMIT HASH>`
>> 
>> 3. Run jcheck on a commit or a series of commits using the .jcheck/conf in my workspace. 
>> `git skara jcheck --workspace-conf`
>> 
>> 4. Run jcheck on a commit or a series of commits using a config file that I point to directly, that may have any name. 
>> `git skara jcheck --workspace-conf --conf-file <FILENAME>`
>> 
>> 5. Run jcheck on the diff in my current workspace, either --staged or not using the .jcheck/conf in my workspace.
>> `git skara jcheck --workspace-diff`
>
> Zhao Song has updated the pull request incrementally with one additional commit since the last revision:
> 
>   implemented case 5

There are a lot of minor things, but all in all this turned out better than I had imagined. Looking forward to using it. 

Have you tried all the options against a repository with various degrees of local changes in it?

cli/src/main/java/org/openjdk/skara/cli/GitJCheck.java line 103:

> 101:                   .fullname("conf-rev")
> 102:                   .describe("REV")
> 103:                   .helptext("Use .jcheck/conf in specified rev")

Suggestion:

                  .helptext("Use .jcheck/conf in the specified revision")

cli/src/main/java/org/openjdk/skara/cli/GitJCheck.java line 148:

> 146:             Switch.shortcut("")
> 147:                   .fullname("staged")
> 148:                   .helptext("Run jcheck includes staged changes and by default jcheck will use staged .jcheck/conf")

Suggestion:

                  .helptext("Check staged changes as if they were committed")

cli/src/main/java/org/openjdk/skara/cli/GitJCheck.java line 153:

> 151:                   .fullname("working-tree")
> 152:                   .helptext("Run jcheck includes changes in working tree and by default " +
> 153:                           "jcheck will use .jcheck/conf in working tree")

Suggestion:

                  .helptext("Check changes in working tree as if they were committed")

cli/src/main/java/org/openjdk/skara/cli/GitJCheck.java line 243:

> 241: 
> 242:         var staged = arguments.contains("staged");
> 243:         var working_tree = arguments.contains("working-tree");

Please use camelCase for local variables.
Suggestion:

        var workingTree = arguments.contains("working-tree");

cli/src/main/java/org/openjdk/skara/cli/GitJCheck.java line 246:

> 244:         // This two flags are mutually exclusive
> 245:         if (staged && working_tree) {
> 246:             System.err.println(String.format("error: you can only choose one from staged or working-tree"));

Suggestion:

        // These two flags are mutually exclusive
        if (staged && working_tree) {
            System.err.println(String.format("error: can only use one of --staged or --working-tree"));

cli/src/main/java/org/openjdk/skara/cli/GitJCheck.java line 268:

> 266:             confFlagCount++;
> 267:         }
> 268:         // This four flags are mutually exclusive

Suggestion:

        // These four flags are mutually exclusive

cli/src/main/java/org/openjdk/skara/cli/GitJCheck.java line 271:

> 269:         if (confFlagCount > 1) {
> 270:             System.err.println(String.format("error: you can only choose one from using jcheck " +
> 271:                     "configuration in work space or in a specified commit"));

Suggestion:

            System.err.println(String.format("error: can only use one source for jcheck configuration"));

vcs/src/main/java/org/openjdk/skara/vcs/git/GitRepository.java line 1664:

> 1662: 
> 1663:     @Override
> 1664:     public Optional<Hash> wholeHash(String rev) {

Isn't this just `::resolve(String)`?

vcs/src/main/java/org/openjdk/skara/vcs/git/GitRepository.java line 1675:

> 1673: 
> 1674:     @Override
> 1675:     public Optional<List<String>> stagedFile(Path path) {

Suggestion:

    public Optional<List<String>> stagedFileContents(Path path) {

vcs/src/main/java/org/openjdk/skara/vcs/git/GitRepository.java line 1684:

> 1682:         return Optional.empty();
> 1683:     }
> 1684: 

Suggestion:

    /**
     * Creates a fake Commit instance representing the currently staged diff.
     */

vcs/src/main/java/org/openjdk/skara/vcs/git/GitRepository.java line 1687:

> 1685:     @Override
> 1686:     public Commit staged() throws IOException {
> 1687:         var author = new Author("jcheck", "jcheck at none.none");

For the default user and email, there is already a `::userName` method. You can create one for email as well, and then use these values as fallbacks (`Optional.orElse`).

vcs/src/main/java/org/openjdk/skara/vcs/git/GitRepository.java line 1692:

> 1690:         return new Commit(commitMetaData, List.of(diffStaged()));
> 1691:     }
> 1692: 

Suggestion:

    /**
     * Creates a fake Commit instance representing the current working tree.
     */

-------------

PR: https://git.openjdk.org/skara/pull/1428