Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SCM-976] GitExe Changelog works if the user has defined a custom format. #134

Closed

Conversation

nielsbasjes
Copy link
Contributor

My proposed fix for https://issues.apache.org/jira/browse/SCM-976

I'm an Apache Committer (Avro, Flink) so the licensing part should be fine.

If I run the current version on the maven-scm git repo I see:

mvn org.apache.maven.plugins:maven-scm-plugin:2.0.0-M1:changelog

[INFO] --- maven-scm-plugin:2.0.0-M1:changelog (default-cli) @ maven-scm ---
[INFO] Executing: /bin/sh -c cd '/home/nbasjes/workspace/Apache/maven-scm' && 'git' 'whatchanged' '--date=iso' '--' '.'
[INFO] Working directory: /home/nbasjes/workspace/Apache/maven-scm
[INFO]     Michael Osipov <michaelo@apache.org>
null 

with these changes I see

mvn org.apache.maven.plugins:maven-scm-plugin:2.0.0-M2-SNAPSHOT:changelog

[INFO] Executing: /bin/sh -c cd '/home/nbasjes/workspace/Apache/maven-scm' && 'git' 'whatchanged' '--date=iso' '--' '.'
[INFO] Working directory: /home/nbasjes/workspace/Apache/maven-scm
[INFO] Michael Osipov <michaelo@apache.org>
Sat Jan 08 21:02:12 CET 2022
[modified]:maven-scm-api/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:maven-scm-client/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:maven-scm-managers/maven-scm-manager-plexus/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:maven-scm-managers/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:maven-scm-plugin/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:maven-scm-providers/maven-scm-provider-hg/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:maven-scm-providers/maven-scm-provider-local/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:maven-scm-providers/maven-scm-providers-git/maven-scm-provider-git-commons/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:maven-scm-providers/maven-scm-providers-git/maven-scm-provider-gitexe/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:maven-scm-providers/maven-scm-providers-git/maven-scm-provider-gittest/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:maven-scm-providers/maven-scm-providers-git/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:maven-scm-providers/maven-scm-providers-standard/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:maven-scm-providers/maven-scm-providers-svn/maven-scm-provider-svn-commons/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:maven-scm-providers/maven-scm-providers-svn/maven-scm-provider-svnexe/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:maven-scm-providers/maven-scm-providers-svn/maven-scm-provider-svntest/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:maven-scm-providers/maven-scm-providers-svn/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:maven-scm-providers/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:maven-scm-test/pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[modified]:pom.xml, 4fc34cfa14f2e72506187b03a492ce55ed459d4c
[maven-release-plugin] prepare for next development iteration
[INFO] Michael Osipov <michaelo@apache.org>
Sat Jan 08 21:02:01 CET 2022
[modified]:maven-scm-api/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:maven-scm-client/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:maven-scm-managers/maven-scm-manager-plexus/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:maven-scm-managers/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:maven-scm-plugin/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:maven-scm-providers/maven-scm-provider-hg/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:maven-scm-providers/maven-scm-provider-local/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:maven-scm-providers/maven-scm-providers-git/maven-scm-provider-git-commons/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:maven-scm-providers/maven-scm-providers-git/maven-scm-provider-gitexe/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:maven-scm-providers/maven-scm-providers-git/maven-scm-provider-gittest/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:maven-scm-providers/maven-scm-providers-git/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:maven-scm-providers/maven-scm-providers-standard/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:maven-scm-providers/maven-scm-providers-svn/maven-scm-provider-svn-commons/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:maven-scm-providers/maven-scm-providers-svn/maven-scm-provider-svnexe/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:maven-scm-providers/maven-scm-providers-svn/maven-scm-provider-svntest/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:maven-scm-providers/maven-scm-providers-svn/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:maven-scm-providers/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:maven-scm-test/pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[modified]:pom.xml, 3a6d9817fe809c43eca588d7c0f4428254eae17c
[maven-release-plugin] prepare release maven-scm-2.0.0-M1
[INFO] Michael Osipov <michaelo@apache.org>
Sat Jan 08 20:49:53 CET 2022
[modified]:src/site/apt/guide/index.apt, 6cf3ed02e19db47f20d95dac4a24f2c40121e67f
[modified]:src/site/apt/guide/new_provider.apt, 6cf3ed02e19db47f20d95dac4a24f2c40121e67f
[modified]:src/site/apt/guide/usage.apt, 6cf3ed02e19db47f20d95dac4a24f2c40121e67f
[modified]:src/site/site.xml, 6cf3ed02e19db47f20d95dac4a24f2c40121e67f
Use canonical name 'Maven SCM'
[INFO] Michael Osipov <michaelo@apache.org>
Sat Jan 08 20:42:33 CET 2022
[modified]:maven-scm-api/src/main/java/org/apache/maven/scm/command/changelog/ChangeLogScmRequest.java, 8b64075b41174b581be3606aea08a88f9f06f73c
[modified]:maven-scm-api/src/main/java/org/apache/maven/scm/manager/BasicScmManager.java, 8b64075b41174b581be3606aea08a88f9f06f73c
[modified]:maven-scm-providers/maven-scm-providers-git/maven-scm-provider-git-commons/src/main/java/org/apache/maven/scm/provider/git/util/GitUtil.java, 8b64075b41174b581be3606aea08a88f9f06f73c
[modified]:maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/main/java/org/apache/maven/scm/provider/git/jgit/command/JGitUtils.java, 8b64075b41174b581be3606aea08a88f9f06f73c
[modified]:maven-scm-providers/maven-scm-providers-svn/maven-scm-provider-svn-commons/src/main/java/org/apache/maven/scm/provider/svn/util/SvnUtil.java, 8b64075b41174b581be3606aea08a88f9f06f73c
Fix issues reported by Checkstyle

@pzygielo
Copy link

I think SCM-976 is caused by

  1. probably non-default git configuration in reporter's env for format
  2. here - by the fact that does not set the format explicitly, and uses the one that might also be altered by the user in configuration.

I also don't think the git built-in formats change between versions.

IMO - instead of proposed changes - it would be better to explicitly use --format=... - setting the one that extracting changelog is done from (medium now I guess).

@pzygielo
Copy link

@nielsbasjes just to check - could you share result of

git config --show-origin --get format.pretty

please?

@nielsbasjes
Copy link
Contributor Author

git config --show-origin --get format.pretty

$ git config --show-origin --get format.pretty
file:/home/nbasjes/.gitconfig fuller

@nielsbasjes
Copy link
Contributor Author

@pzygielo You were absolutely right. I have updated the pull request to set the format explicitly.

@pzygielo
Copy link

@michael-o @cstamas
Could we have this change reviewed, please?

@michael-o
Copy link
Member

Please ping me on Friday.

@nielsbasjes
Copy link
Contributor Author

@michael-o ping ...

@pzygielo
Copy link

pzygielo commented Apr 1, 2022

ping @michael-o

@pzygielo
Copy link

May I ask to have it finalized, please?

@slawekjaranowski
Copy link
Member

@nielsbasjes build was failed, please rebase with current basecode, we will can see the result of build

@nielsbasjes
Copy link
Contributor Author

nielsbasjes commented May 18, 2022

On my machine (Intel i7, Ubuntu 20.04, javac 17.0.3, mvn 3.8.3) the mvn clean verify -P run-its passes.

@nielsbasjes
Copy link
Contributor Author

Hmmm, the readme states Run all the tests with mvn -Prun-its verify to assure nothing else was accidentally broken. ... yet I cannot find a run-its profile in this project.

@michael-o
Copy link
Member

-Ptck-git

@nielsbasjes
Copy link
Contributor Author

@pzygielo @slawekjaranowski @michael-o FYI: Everything passed.

@michael-o
Copy link
Member

Will check too this night

@michael-o
Copy link
Member

I don't understand this change. My manpage says:
See the "PRETTY FORMATS" section for some additional details for each format. When = part is omitted, it defaults to medium.

and this is what I see with the command right now:

D:\Entwicklung\Projekte\maven-site-plugin [MSITE-901-3.x ≡ +0 ~1 -0 !]> git whatchanged --date=iso --max-count=40 -- .
commit e2774627651c7d7a4760a8229c2cda85dd383e7a (HEAD -> MSITE-901-3.x, origin/MSITE-901-3.x)
Author: Matt Nelson <matt.nelson@cerner.com>
Date:   2020-03-20 18:05:04 -0500

    [MSITE-901] If precending standalone report has been run, site:jar does not reinvoke site:site

    This closes #82

:000000 100644 00000000 a644cb7e A      src/it/projects/MSITE-901/invoker.properties
:000000 100644 00000000 a89260c7 A      src/it/projects/MSITE-901/pom.xml
:000000 100644 00000000 1cf15bed A      src/it/projects/MSITE-901/verify.groovy
:100644 100644 26be6d40 8471aed9 M      src/main/java/org/apache/maven/plugins/site/render/SiteJarMojo.java

commit 3c6ff2e285063231b7042bfe7875871c9d339830 (origin/maven-site-plugin-3.x, maven-site-plugin-3.x)
Author: Michael Osipov <michaelo@apache.org>
Date:   2022-04-28 20:45:25 +0200

    Update CI URL

:100644 100644 494ce904 9fbf6435 M      pom.xml

Did you configure Git in a way that modifies the output by default?

@pzygielo
Copy link

Did you configure Git in a way that modifies the output by default?

Yes he did, as stated in #134 (comment) already.

I don't understand this change. My manpage says:
See the "PRETTY FORMATS" section for some additional details for each format. When = part is omitted, it defaults to medium.

This change is for maven-scm to work correctly regardless of local/global/system git configuration on user machine, to enforce the format expected by this component.

@michael-o
Copy link
Member

Did you configure Git in a way that modifies the output by default?

Yes he did, as stated in #134 (comment) already.

I don't understand this change. My manpage says:
See the "PRETTY FORMATS" section for some additional details for each format. When = part is omitted, it defaults to medium.

This change is for maven-scm to work correctly regardless of local/global/system git configuration on user machine, to enforce the format expected by this component.

Then this makes sense, but the issue summary is wrong and does not relate to the problem. Problem is that the current call does not enforce default output and relies on default, unmodified config.

@pzygielo
Copy link

the issue summary is wrong and does not relate to the problem. Problem is that the current call does not enforce default output and relies on default, unmodified config.

I think the commit message is fine now, so it would be the matter to sync PR title and JIRA, right?

@michael-o
Copy link
Member

the issue summary is wrong and does not relate to the problem. Problem is that the current call does not enforce default output and relies on default, unmodified config.

I think the commit message is fine now, so it would be the matter to sync PR title and JIRA, right?

Yes, I'd more expect something like:
Explicitly provide a format to GitExe bla bla
or similar.

Something which shows the intent of the change. REason is that there is exactly one default format, a user-defined format is a modified format.

@nielsbasjes nielsbasjes changed the title [SCM-976] GitExe Changelog supports current versions of git [SCM-976] GitExe Changelog works if the user has defined a custom format. May 19, 2022
@nielsbasjes
Copy link
Contributor Author

@michael-o I agree, the title of the jira issue and the pull request were created at a moment I misunderstood the real problem at hand. Just now I have updated these titles to reflect what is really happening. Are you happy with what you see now?

@michael-o
Copy link
Member

Yes, I will move forward with this today

@michael-o
Copy link
Member

Looking into now. @nielsbasjes Would you mind also to check other commands whether they are prone to this issue as well?

asfgit pushed a commit that referenced this pull request May 19, 2022
@asfgit asfgit closed this in 178b111 May 19, 2022
@nielsbasjes
Copy link
Contributor Author

@michael-o I checked the git documentation and the --format only applies to log and whatchanged. Then as a local test I forced --format=fuller into all git log and whatchanged commandlines and none of the tests broke.

Other than the changelog I only found the GitUpdateCommand to use git log and these tests kept working.

As far as I can tell no other git commands are affected.

@nielsbasjes nielsbasjes deleted the SCM-976-GitexeChangelog branch May 19, 2022 20:40
@michael-o
Copy link
Member

Great, thanks for the investigation.

@nielsbasjes
Copy link
Contributor Author

-Ptck-git

I found that there is also a -Ptck and if you run that it truly runs all tests ... and several fail.

I have added a new Jira ticket about this: https://issues.apache.org/jira/browse/SCM-981

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants