Re: [PATCH 3/6] push: change `simple` to accommodate triangular workflows

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Without any configuration the current branch is pushed out, which
> loosens the safety we implemented in the current 'safer upstream'.
>
> I am not convinced this is a good change.  I am not convinced this is
> a bad change, either, yet, but this loosening smells bad.

Provided that we would want to keep the "Push the current one to the
same name but you have to have it set up as your integration source"
safety for central workflow (which I am starting to think we
should), we would want something like this on top of your entire
series, I think.  The behaviour change can be seen in the revert of
one test you made to the test that expects "simple" to fail due to
the safety.

This patch is somewhat minimal in that it does not address other
issues I raised in the review of the series; it only addresses the
"simple must be safe" issue.

 builtin/push.c          | 60 +++++++++++++++++++++++++------------------------
 t/t5528-push-default.sh |  2 +-
 2 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 783bacf..84c4a90 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -120,29 +120,11 @@ static const char message_detached_head_die[] =
 	   "\n"
 	   "    git push %s HEAD:<name-of-remote-branch>\n");
 
-static void setup_push_simple(struct remote *remote)
-{
-	struct branch *branch = branch_get(NULL);
-	if (!branch)
-		die(_(message_detached_head_die), remote->name);
-	if (!branch->merge_nr || !branch->merge || !branch->remote_name)
-		/* No upstream configured */
-		goto end;
-	if (branch->merge_nr != 1)
-		die(_("The current branch %s has multiple upstream branches, "
-		    "refusing to push."), branch->name);
-	if (!strcmp(branch->remote_name, remote->name) &&
-		strcmp(branch->refname, branch->merge[0]->src))
-		/* Central workflow safety feature */
-		die_push_simple(branch, remote);
-end:
-	add_refspec(branch->name);
-}
-
-static void setup_push_upstream(struct remote *remote)
+static void setup_push_upstream(struct remote *remote, struct branch *branch,
+				int triangular)
 {
 	struct strbuf refspec = STRBUF_INIT;
-	struct branch *branch = branch_get(NULL);
+
 	if (!branch)
 		die(_(message_detached_head_die), remote->name);
 	if (!branch->merge_nr || !branch->merge || !branch->remote_name)
@@ -156,16 +138,29 @@ static void setup_push_upstream(struct remote *remote)
 	if (branch->merge_nr != 1)
 		die(_("The current branch %s has multiple upstream branches, "
 		    "refusing to push."), branch->name);
-	if (strcmp(branch->remote_name, remote->name))
+	if (triangular)
 		die(_("You are pushing to remote '%s', which is not the upstream of\n"
 		      "your current branch '%s', without telling me what to push\n"
 		      "to update which remote branch."),
 		    remote->name, branch->name);
 
+	if (push_default == PUSH_DEFAULT_SIMPLE) {
+		/* Additional safety */
+		if (strcmp(branch->refname, branch->merge[0]->src))
+			die_push_simple(branch, remote);
+	}
+
 	strbuf_addf(&refspec, "%s:%s", branch->name, branch->merge[0]->src);
 	add_refspec(refspec.buf);
 }
 
+static void setup_push_current(struct remote *remote, struct branch *branch)
+{
+	if (!branch)
+		die(_(message_detached_head_die), remote->name);
+	add_refspec(branch->name);
+}
+
 static char warn_unspecified_push_default_msg[] =
 N_("push.default is unset; its implicit value is changing in\n"
    "Git 2.0 from 'matching' to 'simple'. To squelch this message\n"
@@ -190,9 +185,16 @@ static void warn_unspecified_push_default_configuration(void)
 	warning("%s\n", _(warn_unspecified_push_default_msg));
 }
 
+static int is_workflow_triagular(struct remote *remote)
+{
+	struct remote *fetch_remote = remote_get(NULL);
+	return (fetch_remote != remote);
+}
+
 static void setup_default_push_refspecs(struct remote *remote)
 {
-	struct branch *branch;
+	struct branch *branch = branch_get(NULL);
+	int triangular = is_workflow_triagular(remote);
 
 	switch (push_default) {
 	default:
@@ -205,18 +207,18 @@ static void setup_default_push_refspecs(struct remote *remote)
 		break;
 
 	case PUSH_DEFAULT_SIMPLE:
-		setup_push_simple(remote);
+		if (triangular)
+			setup_push_current(remote, branch);
+		else
+			setup_push_upstream(remote, branch, triangular);
 		break;
 
 	case PUSH_DEFAULT_UPSTREAM:
-		setup_push_upstream(remote);
+		setup_push_upstream(remote, branch, triangular);
 		break;
 
 	case PUSH_DEFAULT_CURRENT:
-		branch = branch_get(NULL);
-		if (!branch)
-			die(_(message_detached_head_die), remote->name);
-		add_refspec(branch->name);
+		setup_push_current(remote, branch);
 		break;
 
 	case PUSH_DEFAULT_NOTHING:
diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
index eabc09d..36f5a63 100755
--- a/t/t5528-push-default.sh
+++ b/t/t5528-push-default.sh
@@ -107,7 +107,7 @@ test_expect_success 'push from/to new branch with current creates remote branch'
 test_expect_success 'push to existing branch, with no upstream configured' '
 	test_config branch.master.remote repo1 &&
 	git checkout master &&
-	test_push_success simple master &&
+	test_push_failure simple &&
 	test_push_failure upstream
 '