SCM-917 add untag for svn #88
Conversation
|
I will have a look at it Sunday on the train. Please ping me. |
|
OK. Will ping you sunday morning. Hopefully not forgetting it myself. I am good at at that ;-) |
| @@ -31,18 +31,27 @@ | |||
| public abstract class AbstractUntagCommand | |||
| extends AbstractCommand | |||
| { | |||
| /** | |||
| * SCM-917 - scm:untag for subversion | |||
There was a problem hiding this comment.
This line is redundant, we have git log for.
There was a problem hiding this comment.
Will replace by 'execute untag command'
| public ScmResult executeCommand( ScmProviderRepository repository, ScmFileSet fileSet, | ||
| CommandParameters parameters ) | ||
| throws ScmException | ||
| CommandParameters parameters ) throws ScmException |
| protected abstract ScmResult executeUntagCommand( ScmProviderRepository repository, | ||
| ScmFileSet fileSet, String tagName ) | ||
| throws ScmException; | ||
| ScmFileSet fileSet, String tagName, String message ) throws ScmException; |
There was a problem hiding this comment.
Why aren't you using the same approach as Tag does with ScmTagParameters scmTagParameters instead of the pure message? It would look consistent in my opinion.
There was a problem hiding this comment.
OK. Used untag parameters now.
| { | ||
| String tagName = parameters.getString( CommandParameter.TAG_NAME ); | ||
| String message = parameters.getString( CommandParameter.MESSAGE, "[maven-scm] untag " + tagName ); |
There was a problem hiding this comment.
The opposite says: "copy for tag". This should be similar: "remove tag".
| public UntagScmResult untag( ScmRepository repository, ScmFileSet fileSet, | ||
| CommandParameters parameters ) | ||
| throws ScmException | ||
| public UntagScmResult untag( ScmRepository repository, ScmFileSet fileSet, CommandParameters parameters ) |
There was a problem hiding this comment.
Oh. Overlooked that. Thanks.
| import org.codehaus.plexus.util.cli.Commandline; | ||
|
|
||
| /** | ||
| * SCM-917 provide untag |
| * | ||
| * scm:untag for provider svn is done by removing the tag dir | ||
| * | ||
| * @author Clemens Quoss |
There was a problem hiding this comment.
We don't use this. Authorship is retained on the git commit.
There was a problem hiding this comment.
OK. I am not keen to see my name there. The class I copied from had it so I changed it. Will remove.
| { | ||
| if ( tag == null || tag.trim().isEmpty() ) | ||
| { | ||
| throw new ScmException( "tag name must be specified" ); |
...xe/src/main/java/org/apache/maven/scm/provider/svn/svnexe/command/untag/SvnUntagCommand.java
Outdated
Show resolved
Hide resolved
|
In general this PR looks very decent. |
|
Ping. Made the requested changes. |
|
Thanks, will look at it on the train later this day. |
|
Had a second look at it myself, right now. Maybe ScmUntagParameters is not what you wanted. You wanted me to use ScmTagParameters also for untag, right? Want me to change that? How do I avoid to have too many commits then for one PR? I'm not acquainted with Git/GitHub. |
|
Ping? |
|
Ping?? |
|
Not necessary to ping every two days. I will try again to look at it this week, no promises. It will take an hour or two. Please be patient, this isn't forgotten. |
Hello everyone,
first time handing in a pull request, please be gentle.
PR targets untag functionality for provider subversion in maven-scm.
Implementation tests are provided, though intention of tck test classes not understood. Provided 'normal' ScmTests instead that are executed during build.
Changes made on Provider API, Manager API left unchanged (intention of Manager API unclear, seems outdated).
Anything missing, please advise.
Regards,
Clemens