365 set ahead and behind properties

[jonevn]

Context

Added git.branch.ahead and git.branch.behind, which could be used to verify synced with remote.

#365

Contributor Checklist

• Added relevant integration or unit tests to verify the changes
  Since remote repository is required, no. Have done a bunch of manual local tests.
• Update the Readme or any other documentation (including relevant Javadoc)
  Just added the properties
• Ensured that tests pass locally: mvn clean package
  Verified tests in Eclipse, not able to run mvn clean package, surefire complains about some class missing. Guess it's a jdk-issue for me.
• Ensured that the code meets the current checkstyle coding style definition: mvn clean verify -Pcheckstyle -Dmaven.test.skip=true -B

[jonevn]

Maybe properties should have been named git.status.ahead and git.status.behind.
Not really sure about the "NO_REMOTE" defaults for ahead and behind (when no remote branch found), maybe should be null?

[jonevn]

Was also thinking about creating a parent project, let the mojo be a module and add a new module with integration-tests.

[TheSnoozer]

Hi,
thanks for the PR and adding some already requested stuff :-)
Unfortunately I'm currently quite busy, and thus I haven't had the time for a detailed review yet, but I'll try to find that very soon (sorry in advance!).

Some quick comments:

• yeah, we need a better name for the resulting properties. The naming pattern of git.branch.ahead and git.branch.behind is not quite intuitive - what does it mean? Furthermore, there would be a conflict with this particular naming if we try to create a JSONObject (we had a similar problem with git.commit.id when we introduced git.commit.id.abbrev -- see Changed commit.id to commit.id.full #123 (comment)). Maybe git.local.branch.commit.count.ahead ? Not sure yet, but we would need to change it.
• if null and "NO_REMOTE" would result in the same I'd might slightly prefer "NO_REMOTE" since it would be clearer what is meant by it.
• whats wrong with the integration tests in https://github.com/ktoso/maven-git-commit-id-plugin/blob/master/src/test/java/pl/project13/maven/git/GitCommitIdMojoIntegrationTest.java ?

[jonevn]

Aha, didn't know about that JSONObject thingy. In JGit it is named BranchTrackingStatus.aheadCount, so would like something similar to that, but if we can't use branch, then that idea falls as well.

Nothing really wrong with those integration tests. Just had some ideas of setting up testing to support remote repo stuff. I guess it could be done in the current solution. As a newbie it just seemed easier starting with a fresh slate.

[TheSnoozer]

Hi,
I think this is a really cool feature and thus would like to merge it at some point.
if you need any help with the integration tests please let me know and I would be happy to give more precise instructions on how this could be tested.

[jonevn]

Sorry, have had some things to deal with, so this has lost attention. Will get my focus back on it tomorrow and change the names of the properties. Will also start looking at tests. All help is really appreciated.

[TheSnoozer]

Not a problem! Take all the time you need :-)
I know its hard to find time, especially when its not being payed ;-)

I was thinking of tests that create a remote repository on the fly on the local file system and thus would allow for easier testing (even without internet).

As a really really rough example I would think of something along the lines like this:

  @Test
  @Parameters(method = "useNativeGit")
  public void foo(boolean useNativeGit) throws Exception {
    // given
    mavenSandbox.withParentProject("my-jar-project", "jar")
            .withNoChildProject()
            .withGitRepoInParent(AvailableGitTestRepo.WITH_COMMIT_THAT_HAS_TWO_TAGS)
            .create();
    MavenProject targetProject = mavenSandbox.getParentProject();
    setProjectToExecuteMojoIn(targetProject);

    GitDescribeConfig gitDescribe = createGitDescribeConfig(true, 9);
    gitDescribe.setDirty(null);

    // prepare remote repository for testing
    final File fakeOriginLocation = Files.createTempDirectory("fake-repository").toFile();
    try (final FileRepository fakeOriginFileRepository = new FileRepository(fakeOriginLocation)) {
      fakeOriginFileRepository.create(true);

      try (final Git git = git("my-jar-project")) {
        // set remote
        final RemoteAddCommand remoteAddCommand = git.remoteAdd();
        remoteAddCommand.setName("fake-origin");
        remoteAddCommand.setUri(new URIish(fakeOriginLocation.toURI().toURL()));
        remoteAddCommand.call();

        /*
         * The repository we have looks like this:
         * b6a73ed - (HEAD, master) third addition (32 hours ago) <p>Konrad Malawski</p>
         * d37a598 - (newest-tag, lightweight-tag) second line (32 hours ago) <p>Konrad Malawski</p>
         * 9597545 - (annotated-tag) initial commit (32 hours ago) <p>Konrad Malawski</p>
         */
        // checkout commit d37a598 as new branch and push it to origin

        git.reset().setMode(ResetCommand.ResetType.HARD).setRef("d37a598").call();
        git.checkout().setCreateBranch(true).setName("new-branch").setForce(true).call();

        git.push().setRemote("fake-origin").call();
      }


      mojo.useNativeGit = useNativeGit;
      // when
      mojo.execute();

      // then
      // TODO
    }
  }

depending on what conditions you want to test I guess that should do the trick in most cases. For example if you want to test if you are behind, just push master, don't create a new branch and let the plugin run.

Notice that this is a really hacky draft, not sure if this would work on all systems and would cover all weird conditions (e.g. might contain unclosed resources).

[jonevn]

Hi again, I created a new commit where I changed property names and also added some tests.

I'm really sorry, but the tests doesn't look like you suggested. This is my bad. I read your comment on the phone (mail) and sadly did not see the example code (did not open the mail, just read first few lines). Now that I finished these tests I saw you comment here. So I will look into this type of test next time I sit down.

Think I messed up the formatting on the tests I added, so will fix that next time as well.

[TheSnoozer]

Sorry for going silent on this, I'm currently extremely busy with other stuff (not related to this project).

In general I only glimpsed over your current Merge request, so I can't comment about any in-depth details. However my previous comment was more meant like a starting point / suggestion in general. I strongly believe that there are multiple different ways to solve the same issue. In the end of the day what matters is that the new feature is tested somehow reasonable. What I saw from your merge request is pretty much that you build the local and remote repository on-the-fly, which is pretty much the only thing I was asking for.

Please let me know if you plan further additions. Based on my current time schedule I don't expect to find any time before the weekend, so please bare with me (sorry). Also it would be great if you could solve the merge conflict (sorry for that one)....

[jonevn]

Hi again, merged from master and fixed conflict, no biggie. I don't really know if there is something more to do. Would be great if you could do a full review when you have the time. No rush from my side.

[TheSnoozer]

Hi,
thanks for the changes and adding the test case!

What I could see looks good, but I think there are two items that should be addressed.
The less concerning one would be, that there is a try-catch essentially not needed, and the more problematic (but with really minimal effort solvable) is that the AheadBehind should not be evaluated for HEAD. Instead it should be the <evaluateCommit> configuration.

[TheSnoozer]

I think instead of HEAD we would need to take the property <evaluateOnCommit>HEAD</evaluateOnCommit> into account. That property currently allows the plugin to get the properties from any arbitrary commit/branch.

Not sure if there would be any change for the jGit implementation required (doesn't seem so, but would need to double check)

[jonevn]

I had no idea about that property, my bad. Will need to figure out a way to handle it. If I run the command towards the next to last commit on the branch, the command does not return anything. So then it won't be possible to read ahead and behind status.

[TheSnoozer]

No problem.
I think the fix would be truly trivial with

String remoteRef = runQuietGitCommand(canonical, nativeGitTimeoutInMs, "symbolic-ref -q " + evaluateOnCommit);

That symbolic-ref call is also being used for getBranch (https://github.com/git-commit-id/maven-git-commit-id-plugin/blob/master/src/main/java/pl/project13/maven/git/NativeGitProvider.java#L100)...that mes me wonder why do we have the -q argument here. Maybe there is a reason...not sure, but don't we want to fetch the current branch? Then this could be replaced with String remoteRef = getBranch();. No?

[jonevn]

Updated to use evaluateOnCommit. The problem with that is that these errors occur:

git symbolic-ref -q HEAD^1
fatal: No such ref: HEAD^1
git symbolic-ref -q HEAD1
fatal: No such ref: HEAD1
git symbolic-ref -q 681210a
"no output"

So not really sure how to use that parameter.
Documentation:
"In general this property can be set to something generic like HEAD^1 or point to a branch or tag-name. To support any kind or use-case this configuration can also be set to an entire commit-hash or it's abbreviated version."

Has this changed somehow?

[jonevn]

I could not use getBranch() either, since that strips refs/heads/ from the branch name and then the next command won't work.

[TheSnoozer]

Ouch....you seem absolutely right. I'm surprised we never noticed this before.
Well you now discovered an existing bug :/

The evaluateOnCommit thing should really allow everything like HEAD, HEAD^1 or a specific branch name (e.g. branch).

Our existing integration-tests do actually not verify that the branch name is correct:
See here and here....

I did some tests locally and it seems that the symbolic-ref truly works for HEADS, which is not what we want. Giving the command two arguments is also not what we want (e.g. Given two arguments, creates or updates a symbolic ref <name> to point at the given branch <ref>.).
I was digging through the internet and came across git rev-parse --abbrev-ref BRANCH. That at least seems to work for branches, but does not work for commit hashes and also would indicate tags :-(
Maybe we need git show-ref --heads ${evaluateOnCommit}? But that also does not seem to work with commit hashes pointing to branches.
Actually not sure what command we could use, I thought it would be easier to tell a branch name based on an commitish :/

[TheSnoozer]

Also I quite don't understand why we don't use --short for getBranch(), but that would be a different question, that might become obsolete, because the implementation is not doing what we want / need.

[TheSnoozer]

Maybe git name-rev --name-only ${evaluateOnCommit} works quite in the direction. But that would also give a result for HEAD^1, but maybe we just could check the output it it contains ~ or ^...would need to look into that more later.

See also https://stackoverflow.com/a/19585361

[TheSnoozer]

Thinking more about this, it seems reasonable to track this problem in a separate issue.
That would increase the visibility.
I could merge this as is.
WDYT?

[jonevn]

I agree, should be a separate issue. I can create and start looking into it.
About this issue, if you are ok to merge, then I'm fine with it.

[TheSnoozer]

Just a heads-up:
I'll be on vacation starting tomorrow and most likely won't have any internet. After that I guess its getting time consuming with Christmas. I try my best to find some time and get your changes integrated, just saying that the actual response time from my side will get worse over the next week(s).

After that I hope everything should be back to normal :-)

[TheSnoozer]

Thanks again for the changes.
Looks good. I'll address the issue you discovered in #403.
Thanks for helping to improve this plugin and this feature / property will become available in 3.0.