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.

Sign up for GitHub

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

CHE-3760: Fix git commit not staged specified path with amend #3833

Merged
merged 2 commits into from
Mar 3, 2017
Merged

CHE-3760: Fix git commit not staged specified path with amend #3833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2742540
CHE-3760: Fix Git commit with amend
Mar 2, 2017
2b4bf05
CHE-3760: Fix Git commit with amend
Mar 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions wsagent/che-core-api-git/src/test/java/org/eclipse/che/git/impl/CommitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,25 @@ public void testChangeMessageOfLastCommit(GitConnectionFactory connectionFactory
assertEquals(connection.log(LogParams.create()).getCommits().get(0).getMessage(), commitParams.getMessage());
}

@Test(dataProvider = "GitConnectionFactory", dataProviderClass = org.eclipse.che.git.impl.GitConnectionFactoryProvider.class)
public void testChangeMessageOfLastCommitWithSpecifiedPath(GitConnectionFactory connectionFactory) throws GitException, IOException {
//given
GitConnection connection = connectToGitRepositoryWithContent(connectionFactory, repository);
addFile(connection, "NewFile.txt", CONTENT);
connection.add(AddParams.create(ImmutableList.of("NewFile.txt")));
connection.commit(CommitParams.create("First commit"));
int beforeCommitsCount = connection.log(LogParams.create()).getCommits().size();

//when
CommitParams commitParams = CommitParams.create("Changed message").withFiles(singletonList("NewFile.txt")).withAmend(true);
connection.commit(commitParams);

//then
int afterCommitsCount = connection.log(LogParams.create()).getCommits().size();
assertEquals(beforeCommitsCount, afterCommitsCount);
assertEquals(connection.log(LogParams.create()).getCommits().get(0).getMessage(), commitParams.getMessage());
}

@Test(dataProvider = "GitConnectionFactory", dataProviderClass = org.eclipse.che.git.impl.GitConnectionFactoryProvider.class)
public void testCommitSeparateFiles(GitConnectionFactory connectionFactory) throws GitException, IOException {
//given
Expand All @@ -148,7 +167,7 @@ public void testCommitSeparateFiles(GitConnectionFactory connectionFactory) thro
}

@Test(dataProvider = "GitConnectionFactory", dataProviderClass = org.eclipse.che.git.impl.GitConnectionFactoryProvider.class,
expectedExceptions = GitException.class)
expectedExceptions = GitException.class, expectedExceptionsMessageRegExp = "No changes added to commit")
public void testCommitWithNotStagedChanges(GitConnectionFactory connectionFactory) throws GitException, IOException {
//given
GitConnection connection = connectToGitRepositoryWithContent(connectionFactory, repository);
Expand All @@ -167,7 +186,7 @@ public void testCommitWithNotStagedChanges(GitConnectionFactory connectionFactor
}

@Test(dataProvider = "GitConnectionFactory", dataProviderClass = org.eclipse.che.git.impl.GitConnectionFactoryProvider.class,
expectedExceptions = GitException.class)
expectedExceptions = GitException.class, expectedExceptionsMessageRegExp = "Nothing to commit, working directory clean")
public void testCommitWithCleanIndex(GitConnectionFactory connectionFactory) throws GitException, IOException {
//given
GitConnection connection = connectToGitRepositoryWithContent(connectionFactory, repository);
Expand Down
87 changes: 62 additions & 25 deletions ...nt/che-core-git-impl-jgit/src/main/java/org/eclipse/che/git/impl/jgit/JGitConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,49 +547,85 @@ protected void onEndTask(String taskName, int workCurr, int workTotal, int perce
@Override
public Revision commit(CommitParams params) throws GitException {
try {
String message = params.getMessage();
GitUser committer = getUser();
if (message == null) {
throw new GitException("Message wasn't set");
// Check repository state
RepositoryState repositoryState = repository.getRepositoryState();
if (!repositoryState.canCommit()) {
throw new GitException(format(MESSAGE_COMMIT_NOT_POSSIBLE, repositoryState.getDescription()));
}
if (committer == null) {
throw new GitException("Committer can't be null");
if (params.isAmend() && !repositoryState.canAmend()) {
throw new GitException(format(MESSAGE_COMMIT_AMEND_NOT_POSSIBLE, repositoryState.getDescription()));
}

//Check that there are staged changes present for commit, or any changes if is 'isAll' enabled, otherwise throw exception
Status status = status(StatusFormat.SHORT);
if (!params.isAmend() && !params.isAll()
&& status.getAdded().isEmpty() && status.getChanged().isEmpty() && status.getRemoved().isEmpty()) {
throw new GitException("No changes added to commit");
} else if (!params.isAmend() && params.isAll() && status.isClean()) {
throw new GitException("Nothing to commit, working directory clean");
// Check committer
GitUser committer = getUser();
if (committer == null) {
throw new GitException("Committer can't be null");
}

String committerName = committer.getName();
String committerEmail = committer.getEmail();
if (committerName == null || committerEmail == null) {
throw new GitException("Git user name and (or) email wasn't set", ErrorCodes.NO_COMMITTER_NAME_OR_EMAIL_DEFINED);
}
if (!repository.getRepositoryState().canCommit()) {
Revision rev = newDto(Revision.class);
rev.setMessage(format(MESSAGE_COMMIT_NOT_POSSIBLE, repository.getRepositoryState().getDescription()));
return rev;

// Check commit message
String message = params.getMessage();
if (message == null) {
throw new GitException("Message wasn't set");
}

if (params.isAmend() && !repository.getRepositoryState().canAmend()) {
Revision rev = newDto(Revision.class);
rev.setMessage(format(MESSAGE_COMMIT_AMEND_NOT_POSSIBLE, repository.getRepositoryState().getDescription()));
return rev;
Status status = status(StatusFormat.SHORT);
List<String> specified = params.getFiles();

List<String> staged = new ArrayList<>();
staged.addAll(status.getAdded());
staged.addAll(status.getChanged());
staged.addAll(status.getRemoved());

List<String> changed = new ArrayList<>(staged);
changed.addAll(status.getModified());
changed.addAll(status.getMissing());

List<String> specifiedStaged = specified.stream()
.filter(path -> staged.stream().anyMatch(s -> s.startsWith(path)))
.collect(Collectors.toList());

List<String> specifiedChanged = specified.stream()
.filter(path -> changed.stream().anyMatch(c -> c.startsWith(path)))
.collect(Collectors.toList());

// Check that there are changes present for commit, if 'isAmend' is disabled
if (!params.isAmend()) {
// Check that there are staged changes present for commit, or any changes if 'isAll' is enabled
if (status.isClean()) {
throw new GitException("Nothing to commit, working directory clean");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we throw exception that are related to changes and then about missing commiter's name and email?
Please check behavior of native git and do in the same way.
Any way I think we should check repository state and https://github.com/eclipse/che/pull/3833/files#diff-7f323c0167c366f41270f82a35ed368dR579 and only then throw exception related to changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the check

} else if (!params.isAll() && (specified.isEmpty() ? staged.isEmpty() : specifiedStaged.isEmpty())) {
throw new GitException("No changes added to commit");
}
} else {
/*
By default Jgit doesn't allow to commit not changed specified paths. According to setAllowEmpty method documentation,
setting this flag to true must allow such commit, but it won't because Jgit has a bug:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=510685. As a workaround, specified paths of the commit command will contain
only changed and specified paths. If other changes are present, but the list of changed and specified paths is empty,
throw exception to prevent committing other paths. TODO Remove this check when the bug will be fixed.
*/
if (!specified.isEmpty() && !(params.isAll() ? changed.isEmpty() : staged.isEmpty()) && specifiedChanged.isEmpty()) {
throw new GitException(format("Changes are present but not changed path%s specified for commit.",
specified.size() > 1 ? "s were" : " was"));
}
}

// TODO add 'setAllowEmpty(params.isAmend())' when https://bugs.eclipse.org/bugs/show_bug.cgi?id=510685 will be fixed
CommitCommand commitCommand = getGit().commit()
.setCommitter(committerName, committerEmail).setAuthor(committerName, committerEmail)
.setCommitter(committerName, committerEmail)
.setAuthor(committerName, committerEmail)
.setMessage(message)
.setAll(params.isAll())
.setAmend(params.isAmend());

if (!params.isAll()) {
params.getFiles().forEach(commitCommand::setOnly);
// TODO change to 'specified.forEach(commitCommand::setOnly)' when https://bugs.eclipse.org/bugs/show_bug.cgi?id=510685 will be fixed. See description above.
specifiedChanged.forEach(commitCommand::setOnly);
}

// Check if repository is configured with Gerrit Support
Expand All @@ -602,7 +638,8 @@ public Revision commit(CommitParams params) throws GitException {

return newDto(Revision.class).withBranch(getCurrentBranch())
.withId(result.getId().getName()).withMessage(result.getFullMessage())
.withCommitTime(MILLISECONDS.convert(result.getCommitTime(), SECONDS)).withCommitter(gitUser);
.withCommitTime(MILLISECONDS.convert(result.getCommitTime(), SECONDS))
.withCommitter(gitUser);
} catch (GitAPIException exception) {
throw new GitException(exception.getMessage(), exception);
}
Expand Down
60 changes: 56 additions & 4 deletions ...he-core-git-impl-jgit/src/test/java/org/eclipse/che/git/impl/jgit/JGitConnectionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,31 +13,40 @@

import org.eclipse.che.api.git.CredentialsLoader;
import org.eclipse.che.api.git.GitUserResolver;
import org.eclipse.che.api.git.exception.GitException;
import org.eclipse.che.api.git.params.CommitParams;
import org.eclipse.che.api.git.shared.GitUser;
import org.eclipse.che.api.git.shared.Status;
import org.eclipse.che.plugin.ssh.key.script.SshKeyProvider;
import org.eclipse.jgit.api.TransportCommand;
import org.eclipse.jgit.api.TransportConfigCallback;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.lib.RepositoryState;
import org.eclipse.jgit.transport.SshTransport;
import org.eclipse.jgit.transport.Transport;
import org.eclipse.jgit.transport.TransportHttp;
import org.eclipse.jgit.transport.UsernamePasswordCredentialsProvider;
import org.mockito.ArgumentCaptor;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.testng.MockitoTestNGListener;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Listeners;
import org.testng.annotations.Test;

import java.lang.reflect.Field;

import static java.util.Collections.singletonList;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyObject;
import static org.mockito.Matchers.anyString;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyZeroInteractions;
import static org.mockito.Mockito.when;
Expand All @@ -48,7 +57,7 @@
*
* @author Igor Vinokur
*/
@Listeners(value = {MockitoTestNGListener.class})
@Listeners(value = MockitoTestNGListener.class)
public class JGitConnectionTest {

@Mock
Expand All @@ -61,8 +70,24 @@ public class JGitConnectionTest {
private GitUserResolver gitUserResolver;
@Mock
private TransportCommand transportCommand;
@InjectMocks
private JGitConnection jGitConnection;
@Mock
private GitUserResolver userResolver;

private JGitConnection jGitConnection;

@BeforeMethod
public void setup() {
jGitConnection = spy(new JGitConnection(repository, credentialsLoader, sshKeyProvider, userResolver));

RepositoryState repositoryState = mock(RepositoryState.class);
GitUser gitUser = mock(GitUser.class);
when(repositoryState.canAmend()).thenReturn(true);
when(repositoryState.canCommit()).thenReturn(true);
when(repository.getRepositoryState()).thenReturn(repositoryState);
when(gitUser.getName()).thenReturn("username");
when(gitUser.getEmail()).thenReturn("email");
when(userResolver.getUser()).thenReturn(gitUser);
}

@DataProvider(name = "gitUrlsWithCredentialsProvider")
private static Object[][] gitUrlsWithCredentials() {
Expand Down Expand Up @@ -181,4 +206,31 @@ public void checkCurrentBranch() throws Exception {

assertEquals(branchName, branchTest);
}

/** Test for workaround related to https://bugs.eclipse.org/bugs/show_bug.cgi?id=510685.*/
@Test(expectedExceptions = GitException.class,
expectedExceptionsMessageRegExp = "Changes are present but not changed path was specified for commit.")
public void testCommitNotChangedSpecifiedPathsWithAmendWhenOtherStagedChangesArePresent() throws Exception {
//given
Status status = mock(Status.class);
when(status.getChanged()).thenReturn(singletonList("ChangedNotSpecified"));
doReturn(status).when(jGitConnection).status(anyObject());

//when
jGitConnection.commit(CommitParams.create("message").withFiles(singletonList("NotChangedSpecified")).withAmend(true));
}

/** Test for workaround related to https://bugs.eclipse.org/bugs/show_bug.cgi?id=510685.*/
@Test(expectedExceptions = GitException.class,
expectedExceptionsMessageRegExp = "Changes are present but not changed path was specified for commit.")
public void testCommitNotChangedSpecifiedPathsWithAmendAndWithAllWhenOtherUnstagedChangesArePresent()
throws Exception {
//given
Status status = mock(Status.class);
when(status.getModified()).thenReturn(singletonList("ChangedNotSpecified"));
doReturn(status).when(jGitConnection).status(anyObject());

//when
jGitConnection.commit(CommitParams.create("message").withFiles(singletonList("NotChangedSpecified")).withAmend(true).withAll(true));
}
}