[PATCH] push: make default consistent

* [PATCH] push: make default consistent
@ 2021-07-14 16:49 Felipe Contreras
  2021-07-19  3:47 ` [PATCH v2 0/2] push: fix default action Felipe Contreras
  0 siblings, 1 reply; 4+ messages in thread
From: Felipe Contreras @ 2021-07-14 16:49 UTC (permalink / raw)
  To: git; +Cc: Mathias Kunter, Elijah Newren, Felipe Contreras

The default action "simple" doesn't make sense. It's supposed to be the
same as "current", except with an extra safety in case the name of the
upstream branch doesn't match the name of the current branch (and we are
pushing to the same remote). But if there's no upstream configured
there's no need for any check.

If this works:

  git clone $central .
  ...
  git push

Then this should too:

  git clone $central .
  git checkout -b fix-1
  ...
  git push

Cloning automatically sets up an upstream branch for "master", and
therefore it passes the safety check, but that is much more dangerous
than pushing any other branch.

Why do we barf with "fix-1", but not "master"?

Instead of behaving like "current" if the current branch doesn't have an
upstream configured, `git push` fails like "upstream", so it's a
Frankensteinian action.

If the upstream branch isn't configured, "simple" should not abort, just
like "current".

Reported-by: Mathias Kunter <mathiaskunter@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---

Now that my push reorganization patches have landed on master, it's
clear why the current behavior doesn't really match the behavior of
"current".

As was discussed previously:
https://lore.kernel.org/git/60b15cd2c4136_2183bc20893@natae.notmuch/

 Documentation/config/push.txt |  5 +++--
 Documentation/git-push.txt    |  6 +++---
 builtin/push.c                | 17 ++++++++++++-----
 t/t5528-push-default.sh       |  2 +-
 4 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt
index 632033638c..d267d05e7a 100644
--- a/Documentation/config/push.txt
+++ b/Documentation/config/push.txt
@@ -27,8 +27,9 @@ push.default::
 * `simple` - pushes the current branch with the same name on the remote.
 +
 If you are working on a centralized workflow (pushing to the same repository you
-pull from, which is typically `origin`), then you need to configure an upstream
-branch with the same name.
+pull from, which is typically `origin`), and you have configured an upstream
+branch, then the name must be the same as the current branch, otherwise this
+action will fail as a precaution.
 +
 This mode is the default since Git 2.0, and is the safest option suited for
 beginners.
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index a953c7c387..58352bbf88 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -39,9 +39,9 @@ what to push (See linkgit:git-config[1] for the meaning of `push.default`).
 
 When neither the command-line nor the configuration specify what to
 push, the default behavior is used, which corresponds to the `simple`
-value for `push.default`: the current branch is pushed to the
-corresponding upstream branch, but as a safety measure, the push is
-aborted if the upstream branch does not have the same name as the
+value for `push.default`: the current branch is pushed to a remote
+branch with the same name, but as a safety measure the push is aborted
+if there's a configured upstream branch with a different name than the
 local one.
 
 
diff --git a/builtin/push.c b/builtin/push.c
index e8b10a9b7e..6062edb6f6 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -185,9 +185,11 @@ static const char message_detached_head_die[] =
 	   "\n"
 	   "    git push %s HEAD:<name-of-remote-branch>\n");
 
-static const char *get_upstream_ref(struct branch *branch, const char *remote_name)
+static const char *get_upstream_ref(struct branch *branch, const char *remote_name, int fatal)
 {
-	if (!branch->merge_nr || !branch->merge || !branch->remote_name)
+	if (!branch->merge_nr || !branch->merge || !branch->remote_name) {
+		if (!fatal)
+			return NULL;
 		die(_("The current branch %s has no upstream branch.\n"
 		    "To push the current branch and set the remote as upstream, use\n"
 		    "\n"
@@ -195,6 +197,7 @@ static const char *get_upstream_ref(struct branch *branch, const char *remote_na
 		    branch->name,
 		    remote_name,
 		    branch->name);
+	}
 	if (branch->merge_nr != 1)
 		die(_("The current branch %s has multiple upstream branches, "
 		    "refusing to push."), branch->name);
@@ -231,12 +234,16 @@ static void setup_default_push_refspecs(struct remote *remote)
 	switch (push_default) {
 	default:
 	case PUSH_DEFAULT_UNSPECIFIED:
-	case PUSH_DEFAULT_SIMPLE:
+	case PUSH_DEFAULT_SIMPLE: {
+		const char *upstream;
+
 		if (!same_remote)
 			break;
-		if (strcmp(branch->refname, get_upstream_ref(branch, remote->name)))
+		upstream = get_upstream_ref(branch, remote->name, 0);
+		if (upstream && strcmp(branch->refname, upstream))
 			die_push_simple(branch, remote);
 		break;
+	}
 
 	case PUSH_DEFAULT_UPSTREAM:
 		if (!same_remote)
@@ -244,7 +251,7 @@ static void setup_default_push_refspecs(struct remote *remote)
 			      "your current branch '%s', without telling me what to push\n"
 			      "to update which remote branch."),
 			    remote->name, branch->name);
-		dst = get_upstream_ref(branch, remote->name);
+		dst = get_upstream_ref(branch, remote->name, 1);
 		break;
 
 	case PUSH_DEFAULT_CURRENT:
diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
index f280e00eb7..ba38256ef4 100755
--- a/t/t5528-push-default.sh
+++ b/t/t5528-push-default.sh
@@ -126,7 +126,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.main.remote repo1 &&
 	git checkout main &&
-	test_push_failure simple &&
+	test_push_success simple main &&
 	test_push_failure upstream
 '
 
-- 
2.32.0.38.g1d70fa854e


^ permalink raw reply related	[flat|nested] 4+ messages in thread