You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/12/07 17:43:11 UTC
[GitHub] [maven-scm] cquoss opened a new pull request #110: [SCM-948] Make the limit parameter work for git and svn exe
cquoss opened a new pull request #110:
URL: https://github.com/apache/maven-scm/pull/110
These changes would be sufficient for me to make parameter limit work with goal changelog for providers git and svn exe.
This might seem to be an egoistic point of view and i am open to discussion and willing to spend some more time to it when someone points me to what else might be useful to the community here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] cquoss commented on pull request #110: [SCM-948] Make the limit parameter work for git and svn exe
Posted by GitBox <gi...@apache.org>.
cquoss commented on pull request #110:
URL: https://github.com/apache/maven-scm/pull/110#issuecomment-883621726
At the moment we use a customized version of maven-scm as well containing my first approach to make the limit parameter work by using deprecated methods. That was not in your favor as well, with my second approach i overstepped quite a bit. So OK.
This is the relevant code in used our customized release phase (we called it scm-extract-comment):
ScmRepository repository = configureScmRepository( configurator, descriptor, environment );
ScmProvider provider = getProvider( configurator, repository );
ScmRevision headRevision = new ScmRevision( "HEAD" );
ChangeLogScmResult logResult;
try
{
ChangeLogScmRequest logRequest = new ChangeLogScmRequest( repository, fileSet );
logRequest.setLimit( 1 );
logRequest.setRevision( headRevision );
logResult = provider.changeLog( logRequest );
...
As you can see we hand down a log request containing the limit option of 1, since we are only interested in the latest log entry.
So my solution should contain amendments to hand over this log request to the svn and git command line, without using the deprecated methods. Correct? Just trying to make sure not to burn more useless hours on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] cquoss commented on a change in pull request #110: [SCM-948] Make the limit parameter work for git and svn exe
Posted by GitBox <gi...@apache.org>.
cquoss commented on a change in pull request #110:
URL: https://github.com/apache/maven-scm/pull/110#discussion_r561288091
##########
File path: maven-scm-api/src/main/java/org/apache/maven/scm/command/changelog/AbstractChangeLogCommand.java
##########
@@ -46,6 +46,15 @@ protected abstract ChangeLogScmResult executeChangeLogCommand( ScmProviderReposi
String datePattern )
throws ScmException;
+ @Deprecated
+ protected ChangeLogScmResult executeChangeLogCommand( ScmProviderRepository repository, ScmFileSet fileSet,
Review comment:
Well, i come from a different angle here. I just want to get the limit parameter to work. Because we have a use case for this. I am not here to refactor this to use ChangeLogScmRequest. So i simply duplicated deprecated methods to make it work. And yes - made them deprecated from the start so that someone doing the refactoring part sees these also have to be taken into account.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] cquoss edited a comment on pull request #110: [SCM-948] Make the limit parameter work for git and svn exe
Posted by GitBox <gi...@apache.org>.
cquoss edited a comment on pull request #110:
URL: https://github.com/apache/maven-scm/pull/110#issuecomment-832017583
Thanks. That is very much appreciated. I pushed my work in progress so you see what i am heading at. And can stop me in my tracks if i run astray in your opinion.
Two more things to think about:
1. I change the changelog command implementation to: provider->request / command->parameters. In some cases (svnexe f.i.) request has been handed down to command. I changed that back to parameters.
2. Why do i have to install svn.exe to make the svnexe unit tests run, but no other provider needs an installation for that (OK, i do not know about git.exe since this was installed already)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] cquoss commented on pull request #110: [SCM-948] Make the limit parameter work for git and svn exe
Posted by GitBox <gi...@apache.org>.
cquoss commented on pull request #110:
URL: https://github.com/apache/maven-scm/pull/110#issuecomment-883612987
Sorry for not coming back to this earlier. I understand the concerns though i invested a lot of time removing everything deprecated and make it work again. I will now hard reset the branch back to upstream HEAD and start anew.
Let me clarify my motivation here: we use a customized version of maven-release at our company. We added some extra phases to talk to Jira (checking if issue exist etc.). The issue key to check we get from the latest scm commit comment of the maven root project to release.
So I believe though I did not measure it properly it will save us some performance when the limit parameter would be functional at least for the two scms we are using, git and svn.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on pull request #110: [SCM-948] Make the limit parameter work for git and svn exe
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #110:
URL: https://github.com/apache/maven-scm/pull/110#issuecomment-883657838
I will happily review and merge your code -- as I have done before -- when I clearly can see what has changed. If you think your change requires code removal then we should discuss this first and continue. I am absolutely committed to your quality PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] cquoss edited a comment on pull request #110: [SCM-948] Make the limit parameter work for git and svn exe
Posted by GitBox <gi...@apache.org>.
cquoss edited a comment on pull request #110:
URL: https://github.com/apache/maven-scm/pull/110#issuecomment-883621726
At the moment we use a customized version of maven-scm as well containing my first approach to make the limit parameter work by using deprecated methods. That was not in your favor as well, with my second approach i overstepped quite a bit. So OK.
This is the relevant code used in our customized release phase (we called it scm-extract-comment):
ScmRepository repository = configureScmRepository( configurator, descriptor, environment );
ScmProvider provider = getProvider( configurator, repository );
ScmRevision headRevision = new ScmRevision( "HEAD" );
ChangeLogScmResult logResult;
try
{
ChangeLogScmRequest logRequest = new ChangeLogScmRequest( repository, fileSet );
logRequest.setLimit( 1 );
logRequest.setRevision( headRevision );
logResult = provider.changeLog( logRequest );
...
As you can see we hand down a log request containing the limit option of 1, since we are only interested in the latest log entry.
So my solution should contain amendments to hand over this log request to the svn and git command line, without using the deprecated methods. Correct? Just trying to make sure not to burn more useless hours on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] cquoss closed pull request #110: [SCM-948] Make the limit parameter work for git and svn exe
Posted by GitBox <gi...@apache.org>.
cquoss closed pull request #110:
URL: https://github.com/apache/maven-scm/pull/110
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] elharo commented on a change in pull request #110: [SCM-948] Make the limit parameter work for git and svn exe
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #110:
URL: https://github.com/apache/maven-scm/pull/110#discussion_r551908378
##########
File path: maven-scm-api/src/main/java/org/apache/maven/scm/command/changelog/AbstractChangeLogCommand.java
##########
@@ -46,6 +46,15 @@ protected abstract ChangeLogScmResult executeChangeLogCommand( ScmProviderReposi
String datePattern )
throws ScmException;
+ @Deprecated
+ protected ChangeLogScmResult executeChangeLogCommand( ScmProviderRepository repository, ScmFileSet fileSet,
Review comment:
Is it just the git diff being funky, or are is this PR adding new methods and immediately deprecating them?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] cquoss edited a comment on pull request #110: [SCM-948] Make the limit parameter work for git and svn exe
Posted by GitBox <gi...@apache.org>.
cquoss edited a comment on pull request #110:
URL: https://github.com/apache/maven-scm/pull/110#issuecomment-832017583
Thanks. That is very much appreciated. I pushed my work in progress so you see what i am heading at. And can stop me in my tracks if i run astray in your opinion.
Two more things to think about:
1. I change the changelog command implementation to: provider->request / command->parameters. In some cases (svnexe f.i.) request has been handed down to command. I changed that back to parameters.
2. Why do i have to install svn.exe to make the svnexe unit tests run, but no other provider needs an installation for that (OK, i do not know about git.exe since this was installed already)?
Latest commit compiles again and unit tests are without fault or error.
I will check my changes and remove any unneccessary ones. Then the commits can be squashed to one.
Maybe no ned to merge SCM-947 and SCM-948 but integrate them both one after the other, SCM-948 first. But SCM-947 might need rework. Haven't checked yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on a change in pull request #110: [SCM-948] Make the limit parameter work for git and svn exe
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #110:
URL: https://github.com/apache/maven-scm/pull/110#discussion_r552122460
##########
File path: maven-scm-api/src/main/java/org/apache/maven/scm/command/changelog/AbstractChangeLogCommand.java
##########
@@ -96,11 +122,25 @@ public ScmResult executeCommand( ScmProviderRepository repository, ScmFileSet fi
if ( versionOnly )
{
- return executeChangeLogCommand( repository, fileSet, version, datePattern );
+ if ( limit == null )
+ {
+ return executeChangeLogCommand( repository, fileSet, version, datePattern );
+ }
+ else
+ {
+ return executeChangeLogCommand( repository, fileSet, version, datePattern, limit );
Review comment:
Why this if else pain? Why not pass a null and the short method call the longer one? A concrete implementation can still throw an exception with a non-null arg has been passed.
##########
File path: maven-scm-api/src/main/java/org/apache/maven/scm/command/changelog/AbstractChangeLogCommand.java
##########
@@ -46,6 +46,15 @@ protected abstract ChangeLogScmResult executeChangeLogCommand( ScmProviderReposi
String datePattern )
throws ScmException;
+ @Deprecated
+ protected ChangeLogScmResult executeChangeLogCommand( ScmProviderRepository repository, ScmFileSet fileSet,
Review comment:
Correct. One should use the `ChangeLogScmRequest` class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] cquoss edited a comment on pull request #110: [SCM-948] Make the limit parameter work for git and svn exe
Posted by GitBox <gi...@apache.org>.
cquoss edited a comment on pull request #110:
URL: https://github.com/apache/maven-scm/pull/110#issuecomment-832017583
Thanks. That is very much appreciated. I pushed my work in progress so you see what i am heading at. And can stop me in my tracks if i run astray in your opinion.
Two more things to think about:
1. I change the changelog command implementation to: provider->request / command->parameters. In some cases (svnexe f.i.) request has been handed down to command. I changed that back to parameters.
2. Why do i have to install svn.exe to make the svnexe unit tests run, but no other provider needs an installation for that (OK, i do not know about git.exe since this was installed already)?
Latest commit compiles again and unit tests are without fault or error.
I will check my changes and remove any unneccessary ones. Then the commits can be squashed to one.
Maybe no ned to merge SCM-947 and SCM-948 but integrate them both one after the other, SCM-948 first. But SCM-947 might need rework. Haven't checked yet.
Should this below be tackled, too?
[WARNING] The project org.apache.maven.scm:maven-scm:pom:1.11.3-SNAPSHOT uses prerequisites which is only intended for maven-plugin projects but not for non maven-plugin projects. For such purposes you should use the maven-enforcer-plugin. See https://maven.apache.org/enforcer/enforcer-rules/requireMavenVersion.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] michael-o commented on pull request #110: [SCM-948] Make the limit parameter work for git and svn exe
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #110:
URL: https://github.com/apache/maven-scm/pull/110#issuecomment-831905968
Will have a look next week. Resolver first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] cquoss commented on a change in pull request #110: [SCM-948] Make the limit parameter work for git and svn exe
Posted by GitBox <gi...@apache.org>.
cquoss commented on a change in pull request #110:
URL: https://github.com/apache/maven-scm/pull/110#discussion_r561282580
##########
File path: maven-scm-api/src/main/java/org/apache/maven/scm/command/changelog/AbstractChangeLogCommand.java
##########
@@ -96,11 +122,25 @@ public ScmResult executeCommand( ScmProviderRepository repository, ScmFileSet fi
if ( versionOnly )
{
- return executeChangeLogCommand( repository, fileSet, version, datePattern );
+ if ( limit == null )
+ {
+ return executeChangeLogCommand( repository, fileSet, version, datePattern );
+ }
+ else
+ {
+ return executeChangeLogCommand( repository, fileSet, version, datePattern, limit );
Review comment:
OK. Will look into this and change it as requested.
##########
File path: maven-scm-api/src/main/java/org/apache/maven/scm/command/changelog/AbstractChangeLogCommand.java
##########
@@ -46,6 +46,15 @@ protected abstract ChangeLogScmResult executeChangeLogCommand( ScmProviderReposi
String datePattern )
throws ScmException;
+ @Deprecated
+ protected ChangeLogScmResult executeChangeLogCommand( ScmProviderRepository repository, ScmFileSet fileSet,
Review comment:
Well, i come from a different angle here. I just want to get the limit parameter to work. Because we have a use case for this. I am not here to refactor this to use ChangeLogScmRequest. So i simply duplicated deprecated methods to make it work. And yes - made them deprecated from the start so that someone doing the refactoring part sees these also have to be taken into account.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] cquoss commented on pull request #110: [SCM-948] Make the limit parameter work for git and svn exe
Posted by GitBox <gi...@apache.org>.
cquoss commented on pull request #110:
URL: https://github.com/apache/maven-scm/pull/110#issuecomment-832017583
Thanks. That is very much appreciated. I pushed my work in progress so you see what i am heading at. And can stop me in my tracks if i run astray in your opinion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] cquoss commented on a change in pull request #110: [SCM-948] Make the limit parameter work for git and svn exe
Posted by GitBox <gi...@apache.org>.
cquoss commented on a change in pull request #110:
URL: https://github.com/apache/maven-scm/pull/110#discussion_r561282580
##########
File path: maven-scm-api/src/main/java/org/apache/maven/scm/command/changelog/AbstractChangeLogCommand.java
##########
@@ -96,11 +122,25 @@ public ScmResult executeCommand( ScmProviderRepository repository, ScmFileSet fi
if ( versionOnly )
{
- return executeChangeLogCommand( repository, fileSet, version, datePattern );
+ if ( limit == null )
+ {
+ return executeChangeLogCommand( repository, fileSet, version, datePattern );
+ }
+ else
+ {
+ return executeChangeLogCommand( repository, fileSet, version, datePattern, limit );
Review comment:
OK. Will look into this and change it as requested.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-scm] cquoss commented on pull request #110: [SCM-948] Make the limit parameter work for git and svn exe
Posted by GitBox <gi...@apache.org>.
cquoss commented on pull request #110:
URL: https://github.com/apache/maven-scm/pull/110#issuecomment-831856681
I resolved all this above because i started working again on this PR and now try to remove all the deprecated methods for changelog. But that includes changing the mojo so merging SCM-947 and SCM-948 would be the best solution in my opinion.
Also i am not sure what will break in the provider implementations. TckTests started failing (generic one and accurev). I fear i have to change all of them for every provider. That will take some time and some bugs may slip.
Do you have any suggestions that might help me?
TIA
Clemens Quoß
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org