[PATCH 00/11] Unconvolutize push.default=simple

[PATCH 00/11] Unconvolutize push.default=simple

[Felipe Contreras]

Tired of jumping through hoops trying to understand what the "simple"
mode does, I decided to reorganize it up for good so it's crystal
clear.

There are no functional changes.

Basically the simple mode pushes the current branch with the same name
on the remote.

Except... when there's no upstream branch configured with the same name.

Now the code and the documentation are clear.

This has the additional advantage of making `current` based on `simple`,
rather than the other way around; `current` is basically `simple`
but assuming we are never in a centralized workflow.

Felipe Contreras (11):
  push: hedge code of default=simple
  push: move code to setup_push_simple()
  push: reorganize setup_push_simple()
  push: simplify setup_push_simple()
  push: remove unused code in setup_push_upstream()
  push: merge current and simple
  push: remove redundant check
  push: fix Yoda condition
  push: remove trivial function
  push: flip !triangular for centralized
  doc: push: explain default=simple correctly

 Documentation/config/push.txt | 13 +++++-----
 builtin/push.c                | 47 +++++++++++++++++++----------------
 2 files changed, 31 insertions(+), 29 deletions(-)

-- 
2.32.0.rc0

[PATCH 01/11] push: hedge code of default=simple

[Felipe Contreras]

`simple` is the most important mode so move the relevant code to its own
function to make it easier to see what it's doing.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/push.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 194967ed79..7045e4ef0c 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -223,6 +223,14 @@ static void setup_push_current(struct remote *remote, struct branch *branch)
 	refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
 }
 
+static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
+{
+	if (triangular)
+		setup_push_current(remote, branch);
+	else
+		setup_push_upstream(remote, branch, triangular, 1);
+}
+
 static int is_workflow_triangular(struct remote *remote)
 {
 	struct remote *fetch_remote = remote_get(NULL);
@@ -242,10 +250,7 @@ static void setup_default_push_refspecs(struct remote *remote)
 
 	case PUSH_DEFAULT_UNSPECIFIED:
 	case PUSH_DEFAULT_SIMPLE:
-		if (triangular)
-			setup_push_current(remote, branch);
-		else
-			setup_push_upstream(remote, branch, triangular, 1);
+		setup_push_simple(remote, branch, triangular);
 		break;
 
 	case PUSH_DEFAULT_UPSTREAM:
-- 
2.32.0.rc0

[PATCH 02/11] push: move code to setup_push_simple()

[Felipe Contreras]

In order to avoid doing unnecessary things and simplify it in further
patches.

The code is copied exactly as-is; no functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/push.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 7045e4ef0c..d173c39283 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -225,10 +225,38 @@ static void setup_push_current(struct remote *remote, struct branch *branch)
 
 static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
 {
-	if (triangular)
-		setup_push_current(remote, branch);
-	else
-		setup_push_upstream(remote, branch, triangular, 1);
+	if (triangular) {
+		if (!branch)
+			die(_(message_detached_head_die), remote->name);
+		refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
+	} else {
+		if (!branch)
+			die(_(message_detached_head_die), remote->name);
+		if (!branch->merge_nr || !branch->merge || !branch->remote_name)
+			die(_("The current branch %s has no upstream branch.\n"
+			    "To push the current branch and set the remote as upstream, use\n"
+			    "\n"
+			    "    git push --set-upstream %s %s\n"),
+			    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);
+		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 (1) {
+			/* Additional safety */
+			if (strcmp(branch->refname, branch->merge[0]->src))
+				die_push_simple(branch, remote);
+		}
+
+		refspec_appendf(&rs, "%s:%s", branch->refname, branch->merge[0]->src);
+	}
 }
 
 static int is_workflow_triangular(struct remote *remote)
-- 
2.32.0.rc0

[PATCH 03/11] push: reorganize setup_push_simple()

[Felipe Contreras]

Simply move the code around.

No functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/push.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index d173c39283..9c807ed707 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -225,13 +225,14 @@ static void setup_push_current(struct remote *remote, struct branch *branch)
 
 static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
 {
+	const char *dst;
+
+	if (!branch)
+		die(_(message_detached_head_die), remote->name);
+
 	if (triangular) {
-		if (!branch)
-			die(_(message_detached_head_die), remote->name);
-		refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
+		dst = branch->refname;
 	} else {
-		if (!branch)
-			die(_(message_detached_head_die), remote->name);
 		if (!branch->merge_nr || !branch->merge || !branch->remote_name)
 			die(_("The current branch %s has no upstream branch.\n"
 			    "To push the current branch and set the remote as upstream, use\n"
@@ -243,20 +244,14 @@ static void setup_push_simple(struct remote *remote, struct branch *branch, int
 		if (branch->merge_nr != 1)
 			die(_("The current branch %s has multiple upstream branches, "
 			    "refusing to push."), branch->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 (1) {
-			/* Additional safety */
-			if (strcmp(branch->refname, branch->merge[0]->src))
-				die_push_simple(branch, remote);
-		}
 
-		refspec_appendf(&rs, "%s:%s", branch->refname, branch->merge[0]->src);
+		/* Additional safety */
+		if (strcmp(branch->refname, branch->merge[0]->src))
+			die_push_simple(branch, remote);
+
+		dst = branch->merge[0]->src;
 	}
+	refspec_appendf(&rs, "%s:%s", branch->refname, dst);
 }
 
 static int is_workflow_triangular(struct remote *remote)
-- 
2.32.0.rc0

[PATCH 04/11] push: simplify setup_push_simple()

[Felipe Contreras]

branch->refname can never be different from branch->merge[0]->src.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/push.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 9c807ed707..73fe083682 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -225,14 +225,10 @@ static void setup_push_current(struct remote *remote, struct branch *branch)
 
 static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
 {
-	const char *dst;
-
 	if (!branch)
 		die(_(message_detached_head_die), remote->name);
 
-	if (triangular) {
-		dst = branch->refname;
-	} else {
+	if (!triangular) {
 		if (!branch->merge_nr || !branch->merge || !branch->remote_name)
 			die(_("The current branch %s has no upstream branch.\n"
 			    "To push the current branch and set the remote as upstream, use\n"
@@ -248,10 +244,8 @@ static void setup_push_simple(struct remote *remote, struct branch *branch, int
 		/* Additional safety */
 		if (strcmp(branch->refname, branch->merge[0]->src))
 			die_push_simple(branch, remote);
-
-		dst = branch->merge[0]->src;
 	}
-	refspec_appendf(&rs, "%s:%s", branch->refname, dst);
+	refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
 }
 
 static int is_workflow_triangular(struct remote *remote)
-- 
2.32.0.rc0

[PATCH 05/11] push: remove unused code in setup_push_upstream()

[Felipe Contreras]

Now it's not used for the simple mode.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/push.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 73fe083682..0961826269 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -186,7 +186,7 @@ static const char message_detached_head_die[] =
 	   "    git push %s HEAD:<name-of-remote-branch>\n");
 
 static void setup_push_upstream(struct remote *remote, struct branch *branch,
-				int triangular, int simple)
+				int triangular)
 {
 	if (!branch)
 		die(_(message_detached_head_die), remote->name);
@@ -207,12 +207,6 @@ static void setup_push_upstream(struct remote *remote, struct branch *branch,
 		      "to update which remote branch."),
 		    remote->name, branch->name);
 
-	if (simple) {
-		/* Additional safety */
-		if (strcmp(branch->refname, branch->merge[0]->src))
-			die_push_simple(branch, remote);
-	}
-
 	refspec_appendf(&rs, "%s:%s", branch->refname, branch->merge[0]->src);
 }
 
@@ -271,7 +265,7 @@ static void setup_default_push_refspecs(struct remote *remote)
 		break;
 
 	case PUSH_DEFAULT_UPSTREAM:
-		setup_push_upstream(remote, branch, triangular, 0);
+		setup_push_upstream(remote, branch, triangular);
 		break;
 
 	case PUSH_DEFAULT_CURRENT:
-- 
2.32.0.rc0

[PATCH 06/11] push: merge current and simple

[Felipe Contreras]

`current` is basically the same as `simple` except always assuming we
are in a triangular workflow.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/push.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 0961826269..2d6358f776 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -210,13 +210,6 @@ static void setup_push_upstream(struct remote *remote, struct branch *branch,
 	refspec_appendf(&rs, "%s:%s", branch->refname, branch->merge[0]->src);
 }
 
-static void setup_push_current(struct remote *remote, struct branch *branch)
-{
-	if (!branch)
-		die(_(message_detached_head_die), remote->name);
-	refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
-}
-
 static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
 {
 	if (!branch)
@@ -269,7 +262,7 @@ static void setup_default_push_refspecs(struct remote *remote)
 		break;
 
 	case PUSH_DEFAULT_CURRENT:
-		setup_push_current(remote, branch);
+		setup_push_simple(remote, branch, 1);
 		break;
 
 	case PUSH_DEFAULT_NOTHING:
-- 
2.32.0.rc0

[PATCH 07/11] push: remove redundant check

[Felipe Contreras]

If fetch_remote is NULL (i.e. the branch remote is invalid), then it
can't possibly be same as remote, which can't be NULL.

The check is redundant, and so is the extra variable.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/push.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 2d6358f776..f008cd624f 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -237,8 +237,7 @@ static void setup_push_simple(struct remote *remote, struct branch *branch, int
 
 static int is_workflow_triangular(struct remote *remote)
 {
-	struct remote *fetch_remote = remote_get(NULL);
-	return (fetch_remote && fetch_remote != remote);
+	return remote_get(NULL) != remote;
 }
 
 static void setup_default_push_refspecs(struct remote *remote)
-- 
2.32.0.rc0

[PATCH 08/11] push: fix Yoda condition

[Felipe Contreras]

We want to check if remote is the same as the branch remote, not the
other way around.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/push.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/push.c b/builtin/push.c
index f008cd624f..2dda1724cc 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -237,7 +237,7 @@ static void setup_push_simple(struct remote *remote, struct branch *branch, int
 
 static int is_workflow_triangular(struct remote *remote)
 {
-	return remote_get(NULL) != remote;
+	return remote != remote_get(NULL);
 }
 
 static void setup_default_push_refspecs(struct remote *remote)
-- 
2.32.0.rc0

[PATCH 09/11] push: remove trivial function

[Felipe Contreras]

It's a single line that is used in a single place, and the variable has
the same name as the function.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/push.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 2dda1724cc..8ecfbe8d63 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -235,15 +235,10 @@ static void setup_push_simple(struct remote *remote, struct branch *branch, int
 	refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
 }
 
-static int is_workflow_triangular(struct remote *remote)
-{
-	return remote != remote_get(NULL);
-}
-
 static void setup_default_push_refspecs(struct remote *remote)
 {
 	struct branch *branch = branch_get(NULL);
-	int triangular = is_workflow_triangular(remote);
+	int triangular = remote != remote_get(NULL);
 
 	switch (push_default) {
 	default:
-- 
2.32.0.rc0

[PATCH 10/11] push: flip !triangular for centralized

[Felipe Contreras]

Git is a *distributed* version control system, centralized workflows are
the uncommon case, where we do need to do extra checks.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/push.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 8ecfbe8d63..1856f62861 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -186,7 +186,7 @@ static const char message_detached_head_die[] =
 	   "    git push %s HEAD:<name-of-remote-branch>\n");
 
 static void setup_push_upstream(struct remote *remote, struct branch *branch,
-				int triangular)
+				int centralized)
 {
 	if (!branch)
 		die(_(message_detached_head_die), remote->name);
@@ -201,7 +201,7 @@ static void setup_push_upstream(struct remote *remote, struct branch *branch,
 	if (branch->merge_nr != 1)
 		die(_("The current branch %s has multiple upstream branches, "
 		    "refusing to push."), branch->name);
-	if (triangular)
+	if (!centralized)
 		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."),
@@ -210,12 +210,12 @@ static void setup_push_upstream(struct remote *remote, struct branch *branch,
 	refspec_appendf(&rs, "%s:%s", branch->refname, branch->merge[0]->src);
 }
 
-static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
+static void setup_push_simple(struct remote *remote, struct branch *branch, int centralized)
 {
 	if (!branch)
 		die(_(message_detached_head_die), remote->name);
 
-	if (!triangular) {
+	if (centralized) {
 		if (!branch->merge_nr || !branch->merge || !branch->remote_name)
 			die(_("The current branch %s has no upstream branch.\n"
 			    "To push the current branch and set the remote as upstream, use\n"
@@ -238,7 +238,7 @@ static void setup_push_simple(struct remote *remote, struct branch *branch, int
 static void setup_default_push_refspecs(struct remote *remote)
 {
 	struct branch *branch = branch_get(NULL);
-	int triangular = remote != remote_get(NULL);
+	int centralized = remote == remote_get(NULL);
 
 	switch (push_default) {
 	default:
@@ -248,15 +248,15 @@ static void setup_default_push_refspecs(struct remote *remote)
 
 	case PUSH_DEFAULT_UNSPECIFIED:
 	case PUSH_DEFAULT_SIMPLE:
-		setup_push_simple(remote, branch, triangular);
+		setup_push_simple(remote, branch, centralized);
 		break;
 
 	case PUSH_DEFAULT_UPSTREAM:
-		setup_push_upstream(remote, branch, triangular);
+		setup_push_upstream(remote, branch, centralized);
 		break;
 
 	case PUSH_DEFAULT_CURRENT:
-		setup_push_simple(remote, branch, 1);
+		setup_push_simple(remote, branch, 0);
 		break;
 
 	case PUSH_DEFAULT_NOTHING:
-- 
2.32.0.rc0

[PATCH 11/11] doc: push: explain default=simple correctly

[Felipe Contreras]

Now that the code has been unconvolutized and it's clear what it's
actually doing, update the documentation to reflect that.

Namely; the simple mode only barfs when working on a centralized
workflow, and there's no configured upstream branch with the same name.

Cc: Elijah Newren <newren@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/config/push.txt | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt
index f2667b2689..632033638c 100644
--- a/Documentation/config/push.txt
+++ b/Documentation/config/push.txt
@@ -24,15 +24,14 @@ push.default::
 
 * `tracking` - This is a deprecated synonym for `upstream`.
 
-* `simple` - in centralized workflow, work like `upstream` with an
-  added safety to refuse to push if the upstream branch's name is
-  different from the local one.
+* `simple` - pushes the current branch with the same name on the remote.
 +
-When pushing to a remote that is different from the remote you normally
-pull from, work as `current`.  This is the safest option and is suited
-for beginners.
+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.
 +
-This mode has become the default in Git 2.0.
+This mode is the default since Git 2.0, and is the safest option suited for
+beginners.
 
 * `matching` - push all branches having the same name on both ends.
   This makes the repository you are pushing to remember the set of
-- 
2.32.0.rc0

Re: [PATCH 03/11] push: reorganize setup_push_simple()

[Elijah Newren]

On Fri, May 28, 2021 at 1:10 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Simply move the code around.

Not quite, you also deleted dead code.  Made the patch a bit harder to
read because I was trying to verify you did what the commit message
said and it took me longer than it should have to realize that you
were also deleting dead code.  Might be worth including that fact in
this sentence here.

> No functional changes.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/push.c | 29 ++++++++++++-----------------
>  1 file changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index d173c39283..9c807ed707 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -225,13 +225,14 @@ static void setup_push_current(struct remote *remote, struct branch *branch)
>
>  static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
>  {
> +       const char *dst;
> +
> +       if (!branch)
> +               die(_(message_detached_head_die), remote->name);
> +
>         if (triangular) {
> -               if (!branch)
> -                       die(_(message_detached_head_die), remote->name);
> -               refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
> +               dst = branch->refname;
>         } else {
> -               if (!branch)
> -                       die(_(message_detached_head_die), remote->name);
>                 if (!branch->merge_nr || !branch->merge || !branch->remote_name)
>                         die(_("The current branch %s has no upstream branch.\n"
>                             "To push the current branch and set the remote as upstream, use\n"
> @@ -243,20 +244,14 @@ static void setup_push_simple(struct remote *remote, struct branch *branch, int
>                 if (branch->merge_nr != 1)
>                         die(_("The current branch %s has multiple upstream branches, "
>                             "refusing to push."), branch->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);

This if-block is safe to delete because we're already in the !triangular case.


> -
> -               if (1) {
> -                       /* Additional safety */
> -                       if (strcmp(branch->refname, branch->merge[0]->src))
> -                               die_push_simple(branch, remote);
> -               }
>
> -               refspec_appendf(&rs, "%s:%s", branch->refname, branch->merge[0]->src);
> +               /* Additional safety */
> +               if (strcmp(branch->refname, branch->merge[0]->src))
> +                       die_push_simple(branch, remote);
> +
> +               dst = branch->merge[0]->src;
>         }
> +       refspec_appendf(&rs, "%s:%s", branch->refname, dst);
>  }
>
>  static int is_workflow_triangular(struct remote *remote)
> --
> 2.32.0.rc0

Everything else is, as you say, just moving code around.

Re: [PATCH 04/11] push: simplify setup_push_simple()

[Elijah Newren]

On Fri, May 28, 2021 at 1:10 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> branch->refname can never be different from branch->merge[0]->src.

This statement isn't true without additional qualifications.  Perhaps
extend your commit message with "...since the 'Additional safety'
check dies if they differ." or some other wording that qualifies why
it's true at the point of the code in question.


> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/push.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index 9c807ed707..73fe083682 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -225,14 +225,10 @@ static void setup_push_current(struct remote *remote, struct branch *branch)
>
>  static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
>  {
> -       const char *dst;
> -
>         if (!branch)
>                 die(_(message_detached_head_die), remote->name);
>
> -       if (triangular) {
> -               dst = branch->refname;
> -       } else {
> +       if (!triangular) {
>                 if (!branch->merge_nr || !branch->merge || !branch->remote_name)
>                         die(_("The current branch %s has no upstream branch.\n"
>                             "To push the current branch and set the remote as upstream, use\n"
> @@ -248,10 +244,8 @@ static void setup_push_simple(struct remote *remote, struct branch *branch, int
>                 /* Additional safety */
>                 if (strcmp(branch->refname, branch->merge[0]->src))
>                         die_push_simple(branch, remote);
> -
> -               dst = branch->merge[0]->src;
>         }
> -       refspec_appendf(&rs, "%s:%s", branch->refname, dst);
> +       refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
>  }
>
>  static int is_workflow_triangular(struct remote *remote)
> --
> 2.32.0.rc0

Simple transformation allowed by the "Additional safety" check; makes sense.

Re: [PATCH 10/11] push: flip !triangular for centralized

[Elijah Newren]

On Fri, May 28, 2021 at 1:10 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Git is a *distributed* version control system, centralized workflows are
> the uncommon case, where we do need to do extra checks.

The commit message seemed slightly funny to me, though I'm not sure
why.  However...

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/push.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index 8ecfbe8d63..1856f62861 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -186,7 +186,7 @@ static const char message_detached_head_die[] =
>            "    git push %s HEAD:<name-of-remote-branch>\n");
>
>  static void setup_push_upstream(struct remote *remote, struct branch *branch,
> -                               int triangular)
> +                               int centralized)
>  {
>         if (!branch)
>                 die(_(message_detached_head_die), remote->name);
> @@ -201,7 +201,7 @@ static void setup_push_upstream(struct remote *remote, struct branch *branch,
>         if (branch->merge_nr != 1)
>                 die(_("The current branch %s has multiple upstream branches, "
>                     "refusing to push."), branch->name);
> -       if (triangular)
> +       if (!centralized)
>                 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."),
> @@ -210,12 +210,12 @@ static void setup_push_upstream(struct remote *remote, struct branch *branch,
>         refspec_appendf(&rs, "%s:%s", branch->refname, branch->merge[0]->src);
>  }
>
> -static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
> +static void setup_push_simple(struct remote *remote, struct branch *branch, int centralized)
>  {
>         if (!branch)
>                 die(_(message_detached_head_die), remote->name);
>
> -       if (!triangular) {
> +       if (centralized) {
>                 if (!branch->merge_nr || !branch->merge || !branch->remote_name)
>                         die(_("The current branch %s has no upstream branch.\n"
>                             "To push the current branch and set the remote as upstream, use\n"
> @@ -238,7 +238,7 @@ static void setup_push_simple(struct remote *remote, struct branch *branch, int
>  static void setup_default_push_refspecs(struct remote *remote)
>  {
>         struct branch *branch = branch_get(NULL);
> -       int triangular = remote != remote_get(NULL);
> +       int centralized = remote == remote_get(NULL);
>
>         switch (push_default) {
>         default:
> @@ -248,15 +248,15 @@ static void setup_default_push_refspecs(struct remote *remote)
>
>         case PUSH_DEFAULT_UNSPECIFIED:
>         case PUSH_DEFAULT_SIMPLE:
> -               setup_push_simple(remote, branch, triangular);
> +               setup_push_simple(remote, branch, centralized);
>                 break;
>
>         case PUSH_DEFAULT_UPSTREAM:
> -               setup_push_upstream(remote, branch, triangular);
> +               setup_push_upstream(remote, branch, centralized);
>                 break;
>
>         case PUSH_DEFAULT_CURRENT:
> -               setup_push_simple(remote, branch, 1);
> +               setup_push_simple(remote, branch, 0);
>                 break;
>
>         case PUSH_DEFAULT_NOTHING:
> --
> 2.32.0.rc0

...I think the code is slightly easier to read and reason about since
it removes the double negative.  In particular, when someone reading
the code sees !triangular, and doesn't know or remember the meaning,
they have to translate that to !(remote != remote_get(NULL)).
centralized and !centralized do not have that same problem.  So, I
like the newer version.

Re: [PATCH 11/11] doc: push: explain default=simple correctly

[Elijah Newren]

On Fri, May 28, 2021 at 1:10 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Now that the code has been unconvolutized and it's clear what it's
> actually doing, update the documentation to reflect that.
>
> Namely; the simple mode only barfs when working on a centralized
> workflow, and there's no configured upstream branch with the same name.
>
> Cc: Elijah Newren <newren@gmail.com>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/config/push.txt | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt
> index f2667b2689..632033638c 100644
> --- a/Documentation/config/push.txt
> +++ b/Documentation/config/push.txt
> @@ -24,15 +24,14 @@ push.default::
>
>  * `tracking` - This is a deprecated synonym for `upstream`.
>
> -* `simple` - in centralized workflow, work like `upstream` with an
> -  added safety to refuse to push if the upstream branch's name is
> -  different from the local one.
> +* `simple` - pushes the current branch with the same name on the remote.
>  +
> -When pushing to a remote that is different from the remote you normally
> -pull from, work as `current`.  This is the safest option and is suited
> -for beginners.
> +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.
>  +
> -This mode has become the default in Git 2.0.
> +This mode is the default since Git 2.0, and is the safest option suited for
> +beginners.
>
>  * `matching` - push all branches having the same name on both ends.
>    This makes the repository you are pushing to remember the set of
> --
> 2.32.0.rc0

Much clearer.  Well done.

Re: [PATCH 00/11] Unconvolutize push.default=simple

[Elijah Newren]

On Fri, May 28, 2021 at 1:10 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:

Is "Unconvolutize" a convoluted synonym of simplify, untwist, or
perhaps deconvolute?  ;-)

> Tired of jumping through hoops trying to understand what the "simple"
> mode does, I decided to reorganize it up for good so it's crystal
> clear.
>
> There are no functional changes.

A couple tweaks to some commit messages would make it easier to verify
that you have introduced no functional changes (at least for reviewers
like me who aren't that familiar with this code area).

> Basically the simple mode pushes the current branch with the same name
> on the remote.
>
> Except... when there's no upstream branch configured with the same name.
>
> Now the code and the documentation are clear.

I've read through the series and agree they do as you say.  The code
and documentation changes look good to me.

> This has the additional advantage of making `current` based on `simple`,
> rather than the other way around; `current` is basically `simple`
> but assuming we are never in a centralized workflow.
>
> Felipe Contreras (11):
>   push: hedge code of default=simple
>   push: move code to setup_push_simple()
>   push: reorganize setup_push_simple()
>   push: simplify setup_push_simple()
>   push: remove unused code in setup_push_upstream()
>   push: merge current and simple
>   push: remove redundant check
>   push: fix Yoda condition
>   push: remove trivial function
>   push: flip !triangular for centralized
>   doc: push: explain default=simple correctly
>
>  Documentation/config/push.txt | 13 +++++-----
>  builtin/push.c                | 47 +++++++++++++++++++----------------
>  2 files changed, 31 insertions(+), 29 deletions(-)
>
> --
> 2.32.0.rc0

Re: [PATCH 03/11] push: reorganize setup_push_simple()

[Felipe Contreras]

Elijah Newren wrote:
> On Fri, May 28, 2021 at 1:10 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Simply move the code around.
> 
> Not quite, you also deleted dead code.  Made the patch a bit harder to
> read because I was trying to verify you did what the commit message
> said and it took me longer than it should have to realize that you
> were also deleting dead code.  Might be worth including that fact in
> this sentence here.

OK. I thought that was obvious.

Shall I update the commit message to include that fact, or shall I add a
separate patch to remove the dead code?

Either are fine by me.

> > -               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);
> 
> This if-block is safe to delete because we're already in the !triangular case.
> 
> > -
> > -               if (1) {

Techically this is removing dead code too.

-- 
Felipe Contreras

Re: [PATCH 04/11] push: simplify setup_push_simple()

[Felipe Contreras]

Elijah Newren wrote:
> On Fri, May 28, 2021 at 1:10 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > branch->refname can never be different from branch->merge[0]->src.
> 
> This statement isn't true without additional qualifications.  Perhaps
> extend your commit message with "...since the 'Additional safety'
> check dies if they differ." or some other wording that qualifies why
> it's true at the point of the code in question.

Will do. That in fact was in the back of my mind.

-- 
Felipe Contreras

Re: [PATCH 03/11] push: reorganize setup_push_simple()

[Elijah Newren]

On Fri, May 28, 2021 at 2:27 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Elijah Newren wrote:
> > On Fri, May 28, 2021 at 1:10 PM Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
> > >
> > > Simply move the code around.
> >
> > Not quite, you also deleted dead code.  Made the patch a bit harder to
> > read because I was trying to verify you did what the commit message
> > said and it took me longer than it should have to realize that you
> > were also deleting dead code.  Might be worth including that fact in
> > this sentence here.
>
> OK. I thought that was obvious.

It is if you are familiar with the code, or if you still remember that
condition when you get to that point in the review, or if you happen
to look at the right line of code while mulling it over.
Unfortunately, I'm not familiar with this code, didn't happen to
remember the outer if-block when I got to that point in the
comparison, and didn't at first look back to the relevant line to
realize this.  So I started looking elsewhere for why removing that
condition didn't amount to functional changes and started trying to
figure out a testcase that would give different behavior.  I suspect I
would have realized sooner if not for the claim that you "simply moved
code around", so I just flagged it.  Not a big deal, just something
that I think could make it easier for other reviewers.

> Shall I update the commit message to include that fact, or shall I add a
> separate patch to remove the dead code?

I'd just tweak the commit message to mention you are just deleting
dead code and moving code around.

> Either are fine by me.
>
> > > -               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);
> >
> > This if-block is safe to delete because we're already in the !triangular case.
> >
> > > -
> > > -               if (1) {
>
> Techically this is removing dead code too.

Yep, true.

Re: [PATCH 10/11] push: flip !triangular for centralized

[Felipe Contreras]

Elijah Newren wrote:
> On Fri, May 28, 2021 at 1:10 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Git is a *distributed* version control system, centralized workflows are
> > the uncommon case, where we do need to do extra checks.
> 
> The commit message seemed slightly funny to me, though I'm not sure
> why.

I mean, in my mind I have present two facts:

  1. Linus Torvalds explained that the whole point of Git was to have a
     decentralized VCS [1]. It's not just an important aspect; it's
     crucial.

    If we never get past [the part where I explain distribution], I'd be
    fine with that...

    If you are not distributed, you are not worth using. It's that
    simple.

    I think this is a huge issue and that *alone* should mean that every
    single open source [project] should never use anything but a
    distributed model...

  2. Git doesn't actually have great support for "triangular workflows".

So I do find it funny that the boolean is called "triangular", when
that's supposed to be the whole point from the creator.

Maybe some of that spills out into the rhetoric of the commit message.

> ...I think the code is slightly easier to read and reason about since
> it removes the double negative.  In particular, when someone reading
> the code sees !triangular, and doesn't know or remember the meaning,
> they have to translate that to !(remote != remote_get(NULL)).
> centralized and !centralized do not have that same problem.  So, I
> like the newer version.

To my mind it's not a slight thing at all. Every time I read if
(triangular) I have to stop and translate... Oh, they mean using git in
the way it was the whole point of starting the project... So they
actually mean not using it in that *other* way.

Cheers.

[1] https://www.youtube.com/watch?v=4XpnKHJAok8

-- 
Felipe Contreras

Re: [PATCH 00/11] Unconvolutize push.default=simple

[Felipe Contreras]

Elijah Newren wrote:
> On Fri, May 28, 2021 at 1:10 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> 
> Is "Unconvolutize" a convoluted synonym of simplify, untwist, or
> perhaps deconvolute?  ;-)

Indeed. Does it give you an urge to remedy that?

> > Tired of jumping through hoops trying to understand what the "simple"
> > mode does, I decided to reorganize it up for good so it's crystal
> > clear.
> >
> > There are no functional changes.
> 
> A couple tweaks to some commit messages would make it easier to verify
> that you have introduced no functional changes (at least for reviewers
> like me who aren't that familiar with this code area).

I'll wait a bit for futher comments and resend with those changes.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 00/11] Unconvolutize push.default=simple

[Elijah Newren]

On Fri, May 28, 2021 at 3:22 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Elijah Newren wrote:
> > On Fri, May 28, 2021 at 1:10 PM Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
> >
> > Is "Unconvolutize" a convoluted synonym of simplify, untwist, or
> > perhaps deconvolute?  ;-)
>
> Indeed. Does it give you an urge to remedy that?

Nah, it's fine.

Re: [PATCH 11/11] doc: push: explain default=simple correctly

[Bagas Sanjaya]

On 29/05/21 03.10, Felipe Contreras wrote:
> diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt
> index f2667b2689..632033638c 100644
> --- a/Documentation/config/push.txt
> +++ b/Documentation/config/push.txt
> @@ -24,15 +24,14 @@ push.default::
>   
>   * `tracking` - This is a deprecated synonym for `upstream`.
>   
> -* `simple` - in centralized workflow, work like `upstream` with an
> -  added safety to refuse to push if the upstream branch's name is
> -  different from the local one.
> +* `simple` - pushes the current branch with the same name on the remote.
>   +
> -When pushing to a remote that is different from the remote you normally
> -pull from, work as `current`.  This is the safest option and is suited
> -for beginners.
> +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.
>   +
> -This mode has become the default in Git 2.0.
> +This mode is the default since Git 2.0, and is the safest option suited for
> +beginners.
>   
>   * `matching` - push all branches having the same name on both ends.
>     This makes the repository you are pushing to remember the set of
> 

Grammar looks OK.

Reviewed-by: Bagas Sanjaya <bagasdotme@gmail.com>

-- 
An old man doll... just what I always wanted! - Clara