diff mbox series

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

Message ID 20210528201014.2175179-11-felipe.contreras@gmail.com (mailing list archive)
State Superseded
Headers show
Series Unconvolutize push.default=simple | expand

Commit Message

Felipe Contreras May 28, 2021, 8:10 p.m. UTC
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(-)

Comments

Elijah Newren May 28, 2021, 9:05 p.m. UTC | #1
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.
Felipe Contreras May 28, 2021, 9:57 p.m. UTC | #2
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
diff mbox series

Patch

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: