Skip to content

Commit

Permalink
#44 Create issue for Git.push implicitly pushing to remote origin
Browse files Browse the repository at this point in the history
  • Loading branch information
@schnatterer
schnatterer committed Jun 2, 2020
1 parent 55b08d6 commit 0503bb7
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 108 deletions.
9 changes: 8 additions & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,14 @@ gitWithCreds 'https://your.repo' // Implicitly passed credentials

### Changes to remote repository

* `git.push('master')` - pushes origin
* `git.push('origin master')` - pushes origin
**Note**: This always prepends `origin` if not present for historical reasonse (see #44).
That is, right now it is impossible to push other remotes.
This will change in the next major version of ces-build-lib.
This limitation does not apply to other remote-related operations such as `pull()`, `fetch()` and `pushAndPullOnFailure()`
So it's recommended to explicitly mention the origin and not just the refsepc:
* Do: `git.push('origin master')`
* Don't: `git.push('master')` because this will no longer work in the next major version.
* `git.pushAndPullOnFailure('refspec')` - pushes and pulls if push failed e.g. because local and remote have diverged,
then tries pushing again

Expand Down
21 changes: 4 additions & 17 deletions src/com/cloudogu/ces/cesbuildlib/Git.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,10 @@ class Git implements Serializable {
* @param refSpec branch or tag name
*/
void push(String refSpec = '') {
refSpec = addOriginWhenMissing(refSpec)
// It turned out that it was not a good idea to always add origin at this place as it does not allow for using
// other remotes.
// However, removing "origin" here now breaks backwards compatibility. See #44
refSpec = refSpec.trim().startsWith('origin') ? refSpec : "origin ${refSpec}"
executeGitWithCredentialsAndRetry "push ${refSpec}"
}

Expand All @@ -347,7 +350,6 @@ class Git implements Serializable {
* @param authorEmail
*/
void pull(String refSpec = '', String authorName = commitAuthorName, String authorEmail = commitAuthorEmail) {
refSpec = addOriginWhenMissing(refSpec)
withAuthorAndEmail(authorName, authorEmail) {
executeGitWithCredentials "pull ${refSpec}"
}
Expand All @@ -361,27 +363,12 @@ class Git implements Serializable {
* @param authorEmail
*/
void pushAndPullOnFailure(String refSpec = '', String authorName = commitAuthorName, String authorEmail = commitAuthorEmail) {
refSpec = addOriginWhenMissing(refSpec)
executeGitWithCredentialsAndRetry("push ${refSpec}") {
script.echo "Got error, trying to pull first"
pull(refSpec, authorName, authorEmail)
}
}

/**
* Method exists purely because of downward compatibility. Adding remote to pushes and pulls is preferred,
* but historically git push always added `origin` implicitly.
*
*/
private static String addOriginWhenMissing(String refSpec) {
// if refspec contains more than 1 argument e.g. `upstream master`
if(!refSpec || refSpec.trim().split(' ').length > 1 || refSpec.trim() == 'origin') {
return refSpec
}

return 'origin ' + refSpec
}

/**
* Removes a branch at origin.
*
Expand Down
2 changes: 1 addition & 1 deletion src/com/cloudogu/ces/cesbuildlib/GitFlow.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class GitFlow implements Serializable {
git.checkout(releaseVersion)

// Push changes and tags
git.push("master develop ${releaseVersion}")
git.push("origin master develop ${releaseVersion}")
git.deleteOriginBranch(branchName)
}
}
94 changes: 5 additions & 89 deletions test/com/cloudogu/ces/cesbuildlib/GitTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ class GitTest {
}

@Test
void "pull with empty refspec"() {
void pull() {
def expectedGitCommandWithCredentials = 'git -c credential.helper="!f() { echo username=\'$GIT_AUTH_USR\'; echo password=\'$GIT_AUTH_PSW\'; }; f" pull'
scriptMock.expectedShRetValueForScript.put(expectedGitCommandWithCredentials, 0)
scriptMock.expectedShRetValueForScript.put('git --no-pager show -s --format=\'%an <%ae>\' HEAD', 'User Name <user.name@doma.in>')
Expand All @@ -244,83 +244,7 @@ class GitTest {
assert scriptMock.actualShMapArgs.size() == 3
assert scriptMock.actualShMapArgs.get(2).trim() == expectedGitCommandWithCredentials
}

@Test
void "pull origin"() {
def expectedGitCommandWithCredentials = 'git -c credential.helper="!f() { echo username=\'$GIT_AUTH_USR\'; echo password=\'$GIT_AUTH_PSW\'; }; f" pull origin'
scriptMock.expectedShRetValueForScript.put(expectedGitCommandWithCredentials, 0)
scriptMock.expectedShRetValueForScript.put('git --no-pager show -s --format=\'%an <%ae>\' HEAD', 'User Name <user.name@doma.in>')
git = new Git(scriptMock, 'creds')

git.pull('origin')

def actualWithEnv = scriptMock.actualWithEnvAsMap()
assert actualWithEnv['GIT_AUTHOR_NAME'] == 'User Name'
assert actualWithEnv['GIT_COMMITTER_NAME'] == 'User Name'
assert actualWithEnv['GIT_AUTHOR_EMAIL'] == 'user.name@doma.in'
assert actualWithEnv['GIT_COMMITTER_EMAIL'] == 'user.name@doma.in'

assert scriptMock.actualShMapArgs.size() == 3
assert scriptMock.actualShMapArgs.get(2).trim() == expectedGitCommandWithCredentials
}

@Test
void 'pull master'() {
def expectedGitCommandWithCredentials = 'git -c credential.helper="!f() { echo username=\'$GIT_AUTH_USR\'; echo password=\'$GIT_AUTH_PSW\'; }; f" pull origin master'
scriptMock.expectedShRetValueForScript.put(expectedGitCommandWithCredentials, 0)
scriptMock.expectedShRetValueForScript.put('git --no-pager show -s --format=\'%an <%ae>\' HEAD', 'User Name <user.name@doma.in>')
git = new Git(scriptMock, 'creds')

git.pull 'master'

def actualWithEnv = scriptMock.actualWithEnvAsMap()
assert actualWithEnv['GIT_AUTHOR_NAME'] == 'User Name'
assert actualWithEnv['GIT_COMMITTER_NAME'] == 'User Name'
assert actualWithEnv['GIT_AUTHOR_EMAIL'] == 'user.name@doma.in'
assert actualWithEnv['GIT_COMMITTER_EMAIL'] == 'user.name@doma.in'

assert scriptMock.actualShMapArgs.size() == 3
assert scriptMock.actualShMapArgs.get(2).trim() == expectedGitCommandWithCredentials
}

@Test
void 'pull origin master'() {
def expectedGitCommandWithCredentials = 'git -c credential.helper="!f() { echo username=\'$GIT_AUTH_USR\'; echo password=\'$GIT_AUTH_PSW\'; }; f" pull origin master'
scriptMock.expectedShRetValueForScript.put(expectedGitCommandWithCredentials, 0)
scriptMock.expectedShRetValueForScript.put('git --no-pager show -s --format=\'%an <%ae>\' HEAD', 'User Name <user.name@doma.in>')
git = new Git(scriptMock, 'creds')

git.pull 'origin master'

def actualWithEnv = scriptMock.actualWithEnvAsMap()
assert actualWithEnv['GIT_AUTHOR_NAME'] == 'User Name'
assert actualWithEnv['GIT_COMMITTER_NAME'] == 'User Name'
assert actualWithEnv['GIT_AUTHOR_EMAIL'] == 'user.name@doma.in'
assert actualWithEnv['GIT_COMMITTER_EMAIL'] == 'user.name@doma.in'

assert scriptMock.actualShMapArgs.size() == 3
assert scriptMock.actualShMapArgs.get(2).trim() == expectedGitCommandWithCredentials
}

@Test
void 'pull upstream master'() {
def expectedGitCommandWithCredentials = 'git -c credential.helper="!f() { echo username=\'$GIT_AUTH_USR\'; echo password=\'$GIT_AUTH_PSW\'; }; f" pull upstream master'
scriptMock.expectedShRetValueForScript.put(expectedGitCommandWithCredentials, 0)
scriptMock.expectedShRetValueForScript.put('git --no-pager show -s --format=\'%an <%ae>\' HEAD', 'User Name <user.name@doma.in>')
git = new Git(scriptMock, 'creds')

git.pull 'upstream master'

def actualWithEnv = scriptMock.actualWithEnvAsMap()
assert actualWithEnv['GIT_AUTHOR_NAME'] == 'User Name'
assert actualWithEnv['GIT_COMMITTER_NAME'] == 'User Name'
assert actualWithEnv['GIT_AUTHOR_EMAIL'] == 'user.name@doma.in'
assert actualWithEnv['GIT_COMMITTER_EMAIL'] == 'user.name@doma.in'

assert scriptMock.actualShMapArgs.size() == 3
assert scriptMock.actualShMapArgs.get(2).trim() == expectedGitCommandWithCredentials
}


@Test
void checkout() {
git.checkout("master")
Expand Down Expand Up @@ -403,7 +327,8 @@ class GitTest {
git.push()

assert scriptMock.actualShMapArgs.size() == 1
assert scriptMock.actualShMapArgs.get(0).trim() == 'git push'
// This is somewhat unexpected and to be resolved with #44
assert scriptMock.actualShMapArgs.get(0).trim() == 'git push origin'
}

@Test
Expand All @@ -424,15 +349,6 @@ class GitTest {
assert scriptMock.actualShMapArgs.get(0) == 'git push origin master'
}

@Test
void "push upstream master"() {
scriptMock.expectedDefaultShRetValue = 0
git.push('upstream master')

assert scriptMock.actualShMapArgs.size() == 1
assert scriptMock.actualShMapArgs.get(0) == 'git push upstream master'
}

@Test
void pushNonHttps() {
scriptMock.expectedShRetValueForScript.put('git -c credential.helper="!f() { echo username=\'$GIT_AUTH_USR\'; echo password=\'$GIT_AUTH_PSW\'; }; f" push origin master', 0)
Expand Down Expand Up @@ -487,7 +403,7 @@ class GitTest {
git = new Git(scriptMock, 'creds')

git.retryTimeout = 1
git.pushAndPullOnFailure('master')
git.pushAndPullOnFailure('origin master')

def actualWithEnv = scriptMock.actualWithEnvAsMap()
assert actualWithEnv['GIT_AUTHOR_NAME'] == 'User Name'
Expand Down

0 comments on commit 0503bb7

Please sign in to comment.