Repository: remotemobprogramming/mob
Pull request #329 (2022-10-10 08:49:10)

Git push options
added a config variable to customize git push options on mob branch.



hollesse: Could you give us some information why you implemented it? What is the usecase you are trying to solve with this enhancement?
muskacirca: after a mob start on a branch it pushes mob branch to remote. In my organization it launches a build on the ci. I use this option to add an option "-o ci.skip" that skips the ci.

no issues with mob next as the commit messages already contains "ci skip"
hollesse: Ah okay I see your point. So the push of the new branch should not start a build. In my opinion that is a feature, that would be useful for all mob user. I researched a bit about the push options and did not find any other good usecase and I also researched that setting the git push option does not hurt even if the remote repository does not understand what to do with it. Therefore my suggest would be to just add the git push option `ci.skip` hardcoded and not as configurable parameter. 

What do you think about that @muskacirca @gregorriegler ?  
simonharrer: If we accept the configuration variable, it should be renamed to something like `MOB_START_GIT_PUSH_OPTIONS` as this only affects the mob start command.

If we make this a new default, we really really have to be sure about the side effects - otherwise, a lot of trust will be lost. We need to be sure that people haven't built their own workflows upon this expectation.
hollesse: Or we could have a flag which enables this behavior but have the git push option hardcoded. 
muskacirca: @hollesse @simonharrer I think the flag is good idea, default set to false.
what is the name of the flag : MOB_START_SKIP_CI ?
gregorriegler: Thank you @muskacirca 

https://docs.gitlab.com/ee/user/project/push_options.html
ci.skip is supported since 11.7

We are already making an effort to skip ci on all of our wip commits.
So why not just do the same on this push?

I'd say we keep it simple and just add ci.skip as a default.
It would fit nicely in the next release where we increase the major version.
muskacirca: @gregorriegler ok so no config var, no flag we hardcode it ?
I could do the dev tomorrow
gregorriegler: Yes, please! Would be awesome!
muskacirca: It's done !
gregorriegler: Awesome! I'm thinking whether we should squash the commits since we added some lines only to delete them again.
And we need to resolve the conflict.
But I'm looking forward to merging this.
Great Job @muskacirca you help make this tool become a little better :+1: 
simonharrer: Two effects:

- Does this change the minimum git version users need? We currently rely on a very old git version only, and don't use newer commands. This might change this, and cause issues for the users having older git versions.
- What happens if a remote server doesn't support these push options? As far as I can see, mob will no longer work. Source https://stackoverflow.com/questions/60625197/git-advertisepushoptions-config-is-already-enabled-but-push-options-are-not-work

I'm not sure, we are on the right track here...
hollesse: Okay interesting. I did not found that as I googled for that. But I see the problem with the git version for different topics. I had one workshop where one of the participants had a git version which was already too old. Do we know which version is the oldest version we support? Could we even use this git version within our ci pipeline? 
gregorriegler: What if we turn that 'increasing of the current timer' into a separated UI/UX concept.

So whenever a Timer is started and running, you can click and drag the current timer display, and it will change the time accordingly. So I forgot to start the timer, so I start it now, and then I click inside the timer display (while it already runs) and I drag it to the left. While I'm dragging, the timer visibly changes first to 9 minutes, then to 8, 7, 6, 5 and then I release my mouse button on 5, and the timer continues. 

I think it would be cool. And it would solve our problem. However, people won't easily discover that feature by themselves.
simonharrer: @gregorriegler wrong thread
muskacirca: @simonharrer @hollesse to avoid breaking change may be it’s better to re-add the config variable ? So it’s up to the user !
hollesse: It could be an option but I then would just do a flag for that. And not the configuration option to set any push option as the most user would not think about adding the ci.skip here. IMHO I would add the flag and set the default to enabled. We maybe should check if we could add a good error message git push fails because of the push option that the user should disable this flag. What’s do you think @gregorriegler and @simonharrer 
gregorriegler: > Two effects:
> 
> * Does this change the minimum git version users need? We currently rely on a very old git version only, and don't use newer commands. This might change this, and cause issues for the users having older git versions.
> * What happens if a remote server doesn't support these push options? As far as I can see, mob will no longer work. Source https://stackoverflow.com/questions/60625197/git-advertisepushoptions-config-is-already-enabled-but-push-options-are-not-work
> 
> I'm not sure, we are on the right track here...

Oh no. That is a blocker I did not have on the radar. Need to be more careful about git feature compatibility.
We could do the config, but I'm still exploring whether we can avoid that. I think a well defined default is better than a config var. A config var should be a last resort in my opinion as it increases the cognitive complexity for users. And it increases the paths you have to maintain.

## Possible Alternative
There are two failure cases we would have to address. 
1. The client fails because the git version is too old
2. The server fails because the git version is too old

For 1. we could find out the clients git version and only send -o if its supported.
But 2. is more tricky. We could either just try it, and if it fails, send it again but without the flag.
Or we also try and find out the version of the server.
https://stackoverflow.com/questions/60625197/git-advertisepushoptions-config-is-already-enabled-but-push-options-are-not-work

I understand that this is also about the patience of @muskacirca as we are doing a lot of investigating, designing and changing back and forth.
Please tell me what you think. 





simonharrer: I typically go in favor of less confusion for existing users. That means opt in for such an occasion. I don’t want the tool to stop working after an update because my company git server.
gregorriegler: For the Record:
git push options is available since 2.10 but we would have to use the long version `--push-option` not only because of readability, but also because the short version `-o` is only supported since version 2.18.
https://docs.gitlab.com/ee/user/project/push_options.html 
2.10 is not maintained anymore by git (see https://en.wikipedia.org/wiki/Git)
The oldest still maintained version is 2.17

gregorriegler: Update:

I found out that with squashWip, we support git down to version 2.20.0.
And without squashWip we support git down to 2.13.0. (not lower)
Summary here: https://github.com/remotemobprogramming/mob/pull/330#issuecomment-1277935865

This means, since `--push-option` was introduced in 2.10, we do not have to account for an old git version. mob.sh would already fail on that git version for another reason.
But I think we should still account for a possible older git server, as I haven't tested that yet.
Luckily, this is much simpler. We could just try the push with `--push-option`, and if it fails, we try it again without the options.

What do you think?
@muskacirca @hollesse @simonharrer 
simonharrer: Ok
hollesse: Sounds like a plan :) 
gregorriegler: @muskacirca when would you have time to Incorporate this behavior? I'm asking because we would like to create a new major release and think about having this change in the release.
muskacirca: I'll do it this week. is that okay for you ?
gregorriegler: It shows me that there are still conflicts that must be resolved

Patch:
diff --git a/mob.go b/mob.go
index cfd9549..5344f55 100644
--- a/mob.go
+++ b/mob.go
@@ -920,7 +920,21 @@ func startNewMobSession(configuration config.Configuration) {
 
 	say.Info("starting new session from " + currentBaseBranch.remote(configuration).String())
 	git("checkout", "-B", currentWipBranch.Name, currentBaseBranch.remote(configuration).Name)
-	gitWithoutEmptyStrings("push", gitHooksOption(configuration), "--set-upstream", configuration.RemoteName, currentWipBranch.Name)
+	gitPush(gitHooksOption(configuration), "--set-upstream", configuration.RemoteName, currentWipBranch.Name)
+}
+
+func gitPush(args ...string) {
+
+	pushArgs := []string{"push"}
+	pushArgsWithOptions := append(pushArgs, "--push-option", "ci.skip")
+	argsWithoutEmptyStrings := deleteEmptyStrings(args)
+
+	allArgs := append(pushArgsWithOptions, argsWithoutEmptyStrings...)
+	err := gitignorefailure(allArgs...)
+	if err != nil {
+		retryArgs := append(pushArgs, argsWithoutEmptyStrings...)
+		gitWithoutEmptyStrings(retryArgs...)
+	}
 }
 
 func getUntrackedFiles() string {


Test patch:
diff --git a/configuration/configuration_test.go b/configuration/configuration_test.go
index 4375fa8..97ed79a 100644
--- a/configuration/configuration_test.go
+++ b/configuration/configuration_test.go
@@ -74,7 +74,6 @@ func TestParseArgsMessage(t *testing.T) {
 
 func TestMobRemoteNameEnvironmentVariable(t *testing.T) {
 	configuration := setEnvVarAndParse("MOB_REMOTE_NAME", "GITHUB")
-
 	test.Equals(t, "GITHUB", configuration.RemoteName)
 }
 
diff --git a/mob_test.go b/mob_test.go
index 6ef5cda..f1cbf41 100644
--- a/mob_test.go
+++ b/mob_test.go
@@ -21,6 +21,9 @@ var (
 )
 
 type GitStatus = map[string]string
+type TestOptions struct {
+	enablePushOptions bool
+}
 
 func TestCurrentCliName(t *testing.T) {
 	equals(t, "mob", currentCliName("mob"))
@@ -697,6 +700,20 @@ func TestStartCreateOnPushedFeatureBranchWhichIsBehind(t *testing.T) {
 	assertOutputContains(t, output, "git push origin feature1")
 }
 
+func TestStartPushOnWIPBranchWithOptions(t *testing.T) {
+	output, configuration := setup(t)
+	start(configuration)
+	assertOutputContains(t, output, "git push --push-option ci.skip --no-verify --set-upstream origin mob-session")
+}
+
+func TestStartPushOnWIPBranchWithOptionsShouldFailAndRetry(t *testing.T) {
+	output, configuration := setupWithOptions(t, TestOptions{enablePushOptions: false})
+	start(configuration)
+	assertOutputContains(t, output, "git push --push-option ci.skip --no-verify --set-upstream origin mob-session")
+	assertOutputContains(t, output, "git push --no-verify --set-upstream origin mob-session")
+	assertOutputContains(t, output, "you are on wip branch 'mob-session' (base branch 'master')")
+}
+
 func TestStartNextBackToMaster(t *testing.T) {
 	_, configuration := setup(t)
 	start(configuration)
@@ -1570,11 +1587,12 @@ func gitStatus() GitStatus {
 	return statusMap
 }
 
-func setup(t *testing.T) (output *string, configuration config.Configuration) {
+func setupWithOptions(t *testing.T, options TestOptions) (output *string, configuration config.Configuration) {
+
 	configuration = config.GetDefaultConfiguration()
 	configuration.NextStay = false
 	output = captureOutput(t)
-	createTestbed(t, configuration)
+	createTestbed(t, configuration, options)
 	assertOnBranch(t, "master")
 	equals(t, []string{"master"}, gitBranches())
 	equals(t, []string{"origin/master"}, gitRemoteBranches())
@@ -1583,6 +1601,12 @@ func setup(t *testing.T) (output *string, configuration config.Configuration) {
 	return output, configuration
 }
 
+func setup(t *testing.T) (output *string, configuration config.Configuration) {
+	return setupWithOptions(t, TestOptions{
+		enablePushOptions: true,
+	})
+}
+
 func captureOutput(t *testing.T) *string {
 	messages := ""
 	say.PrintToConsole = func(text string) {
@@ -1603,20 +1627,20 @@ func run(t *testing.T, name string, args ...string) *string {
 	return &output
 }
 
-func createTestbed(t *testing.T, configuration config.Configuration) {
+func createTestbed(t *testing.T, configuration config.Configuration, testOptions TestOptions) {
 	workingDir = ""
 
 	tempDir = t.TempDir()
-	say.Say("Creating testbed in temporary directory " + tempDir)
 
-	createTestbedIn(t, tempDir)
+	say.Say("Creating testbed in temporary directory " + tempDir)
+	createTestbedIn(t, tempDir, testOptions)
 
 	setWorkingDir(tempDir + "/local")
 	assertOnBranch(t, "master")
 	assertNoMobSessionBranches(t, configuration, "mob-session")
 }
 
-func createTestbedIn(t *testing.T, temporaryDirectory string) {
+func createTestbedIn(t *testing.T, temporaryDirectory string, options TestOptions) {
 	say.Debug("Creating temporary test assets in " + temporaryDirectory)
 	err := os.MkdirAll(temporaryDirectory, 0755)
 	if err != nil {
@@ -1627,7 +1651,7 @@ func createTestbedIn(t *testing.T, temporaryDirectory string) {
 	say.Debug("Create remote repository")
 	remoteDirectory := getRemoteDirectory(temporaryDirectory)
 	cleanRepository(remoteDirectory)
-	createRemoteRepository(remoteDirectory)
+	createRemoteRepository(remoteDirectory, options)
 
 	say.Debug("Create first local repository")
 	localDirectory := getLocalDirectory(temporaryDirectory)
@@ -1824,7 +1848,7 @@ func cleanRepository(path string) {
 	}
 }
 
-func createRemoteRepository(path string) {
+func createRemoteRepository(path string, options TestOptions) {
 	branch := "master" // fixed to master for now
 	say.Debug("createremoterepository: Creating remote repository " + path)
 	err := os.MkdirAll(path, 0755)
@@ -1838,6 +1862,7 @@ func createRemoteRepository(path string) {
 	git("--bare", "init")
 	say.Debug("before symbolic-ref")
 	git("symbolic-ref", "HEAD", "refs/heads/"+branch)
+	git("config", "receive.advertisePushOptions", strconv.FormatBool(options.enablePushOptions))
 	say.Debug("finished")
 }
 
diff --git a/squash_wip_test.go b/squash_wip_test.go
index dd0c606..4c6440e 100644
--- a/squash_wip_test.go
+++ b/squash_wip_test.go
@@ -298,8 +298,9 @@ manual commit
 }
 
 func TestSquashWipCommitGitEditor(t *testing.T) {
+	testOptions := TestOptions{enablePushOptions: true}
 	configuration := config.GetDefaultConfiguration()
-	createTestbed(t, configuration)
+	createTestbed(t, configuration, testOptions)
 	input := createFile(t, "commits", fmt.Sprintf(
 		`# This is a combination of 2 commits.
 # This is the 1st commit message:
@@ -331,7 +332,7 @@ new file
 
 func TestSquashWipCommitGitSequenceEditor(t *testing.T) {
 	configuration := config.GetDefaultConfiguration()
-	createTestbed(t, configuration)
+	createTestbed(t, configuration, TestOptions{enablePushOptions: true})
 	input := createFile(t, "rebase", fmt.Sprintf(
 		`pick 01a9a31 %[1]s
 pick 01a9a32 %[1]s