[
https://issues.apache.org/jira/browse/SCM-925?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17569939#comment-17569939
]
ASF GitHub Bot commented on SCM-925:
------------------------------------
kwin commented on code in PR #144:
URL: https://github.com/apache/maven-scm/pull/144#discussion_r927524789
##########
maven-scm-providers/maven-scm-providers-git/maven-scm-provider-gittest/src/main/java/org/apache/maven/scm/provider/git/command/remove/GitRemoveCommandTckTest.java:
##########
@@ -0,0 +1,85 @@
+package org.apache.maven.scm.provider.git.command.remove;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.io.File;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.StandardOpenOption;
+import java.util.Arrays;
+
+import org.apache.maven.scm.ScmFileSet;
+import org.apache.maven.scm.ScmTckTestCase;
+import org.apache.maven.scm.command.remove.RemoveScmResult;
+import org.apache.maven.scm.provider.git.GitScmTestUtils;
+
+/**
+ * This test tests the remove command.
+ *
+ * @author Georg Tsakumagos
+ */
+public abstract class GitRemoveCommandTckTest
+ extends ScmTckTestCase
+{
+ /** {@inheritDoc} */
+ public void initRepo()
+ throws Exception
+ {
+ GitScmTestUtils.initRepo( "src/test/resources/repository/",
getRepositoryRoot(), getWorkingDirectory() );
+ }
+
+ public void testCommandRemoveWithFile()
+ throws Exception
+ {
+ File toBeRemoved = new File( getWorkingDirectory().getAbsolutePath() +
File.separator + "toto.xml" );
+
+ Files.write( toBeRemoved.getAbsoluteFile().toPath(),
Review Comment:
why not using an empty file created with
https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#createFile(java.nio.file.Path,%20java.nio.file.attribute.FileAttribute...),
the content does not matter in this case.
##########
maven-scm-providers/maven-scm-providers-git/maven-scm-provider-gittest/src/main/java/org/apache/maven/scm/provider/git/command/remove/GitRemoveCommandTckTest.java:
##########
@@ -0,0 +1,85 @@
+package org.apache.maven.scm.provider.git.command.remove;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.io.File;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.StandardOpenOption;
+import java.util.Arrays;
+
+import org.apache.maven.scm.ScmFileSet;
+import org.apache.maven.scm.ScmTckTestCase;
+import org.apache.maven.scm.command.remove.RemoveScmResult;
+import org.apache.maven.scm.provider.git.GitScmTestUtils;
+
+/**
+ * This test tests the remove command.
+ *
+ * @author Georg Tsakumagos
+ */
+public abstract class GitRemoveCommandTckTest
+ extends ScmTckTestCase
+{
+ /** {@inheritDoc} */
+ public void initRepo()
+ throws Exception
+ {
+ GitScmTestUtils.initRepo( "src/test/resources/repository/",
getRepositoryRoot(), getWorkingDirectory() );
+ }
+
+ public void testCommandRemoveWithFile()
Review Comment:
I think we use JUnit4, so a `@Test` annotation is necessary for m-surefire-p
to pick this up.
##########
maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/main/java/org/apache/maven/scm/provider/git/jgit/command/JGitUtils.java:
##########
@@ -527,4 +529,61 @@ public static List<String> getTags( Repository repo,
RevCommit commit ) throws I
}
return result;
}
+
+ /**
+ * Remove all files in the given fileSet to the repository.
+ *
+ * @param git the repo to remove the files from
+ * @param fileSet the set of files within the workspace, the files are
removed
+ * relative to the basedir of this fileset
+ * @return a list of removed files
+ * @throws GitAPIException
+ * @throws NoFilepatternException
+ */
+ public static List<ScmFile> removeAllFiles( Git git, ScmFileSet fileSet )
+ throws GitAPIException, NoFilepatternException
+ {
+ URI baseUri = fileSet.getBasedir().toURI();
+ RmCommand remove = git.rm();
+ for ( File file : fileSet.getFileList() )
+ {
+ if ( !file.isAbsolute() )
+ {
+ file = new File( fileSet.getBasedir().getPath(),
file.getPath() );
+ }
+
+ if ( file.exists() )
+ {
+ URI workingCopyRootUri =
git.getRepository().getWorkTree().toURI();
+ String path = FilenameUtils.normalizeFilename( relativize(
workingCopyRootUri, file ) );
Review Comment:
I think you figured out in the other PR that normalizeFilename is redundant
in this case, as relativize always returns a path with forward slashes.
##########
maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/main/java/org/apache/maven/scm/provider/git/jgit/command/JGitUtils.java:
##########
@@ -527,4 +529,61 @@ public static List<String> getTags( Repository repo,
RevCommit commit ) throws I
}
return result;
}
+
+ /**
+ * Remove all files in the given fileSet to the repository.
+ *
+ * @param git the repo to remove the files from
+ * @param fileSet the set of files within the workspace, the files are
removed
+ * relative to the basedir of this fileset
+ * @return a list of removed files
+ * @throws GitAPIException
+ * @throws NoFilepatternException
+ */
+ public static List<ScmFile> removeAllFiles( Git git, ScmFileSet fileSet )
+ throws GitAPIException, NoFilepatternException
+ {
+ URI baseUri = fileSet.getBasedir().toURI();
+ RmCommand remove = git.rm();
+ for ( File file : fileSet.getFileList() )
+ {
+ if ( !file.isAbsolute() )
+ {
+ file = new File( fileSet.getBasedir().getPath(),
file.getPath() );
Review Comment:
```suggestion
file = new File( fileSet.getBasedir(), file.getPath() );
```
##########
maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/main/java/org/apache/maven/scm/provider/git/jgit/command/JGitUtils.java:
##########
@@ -527,4 +529,61 @@ public static List<String> getTags( Repository repo,
RevCommit commit ) throws I
}
return result;
}
+
+ /**
+ * Remove all files in the given fileSet to the repository.
+ *
+ * @param git the repo to remove the files from
+ * @param fileSet the set of files within the workspace, the files are
removed
+ * relative to the basedir of this fileset
+ * @return a list of removed files
+ * @throws GitAPIException
+ * @throws NoFilepatternException
+ */
+ public static List<ScmFile> removeAllFiles( Git git, ScmFileSet fileSet )
+ throws GitAPIException, NoFilepatternException
+ {
+ URI baseUri = fileSet.getBasedir().toURI();
+ RmCommand remove = git.rm();
+ for ( File file : fileSet.getFileList() )
+ {
+ if ( !file.isAbsolute() )
+ {
+ file = new File( fileSet.getBasedir().getPath(),
file.getPath() );
+ }
+
+ if ( file.exists() )
+ {
+ URI workingCopyRootUri =
git.getRepository().getWorkTree().toURI();
+ String path = FilenameUtils.normalizeFilename( relativize(
workingCopyRootUri, file ) );
+ remove.addFilepattern( path );
+ }
+ }
+ remove.call();
+
+ Status status = git.status().call();
+
+ Set<String> allInIndex = new HashSet<String>();
+ allInIndex.addAll( status.getRemoved() );
+ List<ScmFile> removedFiles = new ArrayList<ScmFile>( allInIndex.size()
);
+
+ // rewrite all detected files to now have status 'checked_in'
+ for ( String entry : allInIndex )
+ {
+ ScmFile scmFile = new ScmFile( entry, ScmFileStatus.DELETED );
+
+ // if a specific fileSet is given, we have to check if the file is
+ // really tracked
+ for ( Iterator<File> itfl = fileSet.getFileList().iterator();
itfl.hasNext(); )
+ {
+ String path = FilenameUtils.normalizeFilename( relativize(
baseUri, itfl.next() ) );
+ if ( path.equals( FilenameUtils.normalizeFilename(
scmFile.getPath() ) ) )
Review Comment:
this logic is IMHO incorrect, as `scmFile` has repository relative path
names (already with forward slashes) while `path` is relative to basedir.
##########
maven-scm-providers/maven-scm-providers-git/maven-scm-provider-gittest/src/main/java/org/apache/maven/scm/provider/git/command/remove/GitRemoveCommandTckTest.java:
##########
@@ -0,0 +1,85 @@
+package org.apache.maven.scm.provider.git.command.remove;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.io.File;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.StandardOpenOption;
+import java.util.Arrays;
+
+import org.apache.maven.scm.ScmFileSet;
+import org.apache.maven.scm.ScmTckTestCase;
+import org.apache.maven.scm.command.remove.RemoveScmResult;
+import org.apache.maven.scm.provider.git.GitScmTestUtils;
+
+/**
+ * This test tests the remove command.
+ *
+ * @author Georg Tsakumagos
+ */
+public abstract class GitRemoveCommandTckTest
+ extends ScmTckTestCase
+{
+ /** {@inheritDoc} */
+ public void initRepo()
+ throws Exception
+ {
+ GitScmTestUtils.initRepo( "src/test/resources/repository/",
getRepositoryRoot(), getWorkingDirectory() );
+ }
+
+ public void testCommandRemoveWithFile()
+ throws Exception
+ {
+ File toBeRemoved = new File( getWorkingDirectory().getAbsolutePath() +
File.separator + "toto.xml" );
+
+ Files.write( toBeRemoved.getAbsoluteFile().toPath(),
+ "data".getBytes( StandardCharsets.UTF_8 ),
StandardOpenOption.APPEND, StandardOpenOption.CREATE );
+
+ ScmFileSet fileSet = new ScmFileSet( getWorkingCopy(), toBeRemoved );
+ RemoveScmResult removeResult = getScmManager().remove(
getScmRepository(), fileSet, getBasedir() );
+ assertResultIsSuccess( removeResult );
+ }
+
+ public void testCommandRemoveWithDirectory()
+ throws Exception
+ {
+ File toBeRemoved = new File( getWorkingDirectory().getAbsolutePath() +
File.separator + "toto" );
+ toBeRemoved.mkdir();
+
+ ScmFileSet fileSet = new ScmFileSet( getWorkingCopy(), toBeRemoved );
+ RemoveScmResult removeResult = getScmManager().remove(
getScmRepository(), fileSet, getBasedir() );
+ assertResultIsSuccess( removeResult );
+ }
+
+ public void testCommandRemoveWithTwoDirectories()
+ throws Exception
+ {
+ File toBeRemoved1 = new File( getWorkingDirectory().getAbsolutePath()
+ File.separator + "toto" );
+ toBeRemoved1.mkdir();
+
+ File toBeRemoved2 = new File( getWorkingDirectory().getAbsolutePath()
+ File.separator + "tata" );
+ toBeRemoved2.mkdir();
+
+ ScmFileSet fileSet = new ScmFileSet( getWorkingCopy(), Arrays.asList(
toBeRemoved1, toBeRemoved2 ) );
+ RemoveScmResult removeResult = getScmManager().remove(
getScmRepository(), fileSet, getBasedir() );
+ assertResultIsSuccess( removeResult );
+ }
Review Comment:
we should add a test for a basedir != repository root
##########
maven-scm-providers/maven-scm-providers-git/maven-scm-provider-gittest/src/main/java/org/apache/maven/scm/provider/git/command/remove/GitRemoveCommandTckTest.java:
##########
@@ -0,0 +1,85 @@
+package org.apache.maven.scm.provider.git.command.remove;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.io.File;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.StandardOpenOption;
+import java.util.Arrays;
+
+import org.apache.maven.scm.ScmFileSet;
+import org.apache.maven.scm.ScmTckTestCase;
+import org.apache.maven.scm.command.remove.RemoveScmResult;
+import org.apache.maven.scm.provider.git.GitScmTestUtils;
+
+/**
+ * This test tests the remove command.
+ *
+ * @author Georg Tsakumagos
+ */
+public abstract class GitRemoveCommandTckTest
+ extends ScmTckTestCase
+{
+ /** {@inheritDoc} */
+ public void initRepo()
+ throws Exception
+ {
+ GitScmTestUtils.initRepo( "src/test/resources/repository/",
getRepositoryRoot(), getWorkingDirectory() );
+ }
+
+ public void testCommandRemoveWithFile()
+ throws Exception
+ {
+ File toBeRemoved = new File( getWorkingDirectory().getAbsolutePath() +
File.separator + "toto.xml" );
+
+ Files.write( toBeRemoved.getAbsoluteFile().toPath(),
+ "data".getBytes( StandardCharsets.UTF_8 ),
StandardOpenOption.APPEND, StandardOpenOption.CREATE );
+
+ ScmFileSet fileSet = new ScmFileSet( getWorkingCopy(), toBeRemoved );
+ RemoveScmResult removeResult = getScmManager().remove(
getScmRepository(), fileSet, getBasedir() );
+ assertResultIsSuccess( removeResult );
Review Comment:
I think we need a consecutive checkout to be sure that `toto.xml` is really
removed.
> Implement RemoveCommand in maven-scm-provider-jgit
> --------------------------------------------------
>
> Key: SCM-925
> URL: https://issues.apache.org/jira/browse/SCM-925
> Project: Maven SCM
> Issue Type: Improvement
> Components: maven-scm-provider-jgit
> Affects Versions: 1.11.2
> Reporter: Georg Tsakumagos
> Assignee: Michael Osipov
> Priority: Major
> Fix For: 2.0.0-M2
>
> Attachments: maven-scm-provider-jgit.log
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The git scm plugins differs in support for the scm command _remove_. This
> breaks the goal _prepare-with-pom_ of the _maven-release-plugin_ using the
> _maven-scm-provider-jgit_. The method is not fully implemented and throws an
> UnsupportedOperationException.
> Referring to the ["Maven SCM Providers
> Matrix"|https://maven.apache.org/scm/matrix.html] this feature is documented
> as supported and should be implemented in the _jgit_ provider like in
> maven-scm-provider-gitexe.
>
> ||Variant||remove supported||Source||
> |maven-scm-provider-gitexe|(/)|[Github - GitExeScmProvider.java -
> getRemoveCommand()|https://github.com/apache/maven-scm/blob/a1f7e9857076940c878a370ff599b394df768d33/maven-scm-providers/maven-scm-providers-git/maven-scm-provider-gitexe/src/main/java/org/apache/maven/scm/provider/git/gitexe/GitExeScmProvider.java#L97]|
> |maven-scm-provider-jgit|(x)|[Github - JGitScmProvider.java -
> getRemoveCommand()|https://github.com/apache/maven-scm/blob/a1f7e9857076940c878a370ff599b394df768d33/maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/main/java/org/apache/maven/scm/provider/git/jgit/JGitScmProvider.java#L113]|
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
